Core #345
Merged
Core #345
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Core functionality as described by the protocol https://github.com/firmata/protocol/blob/master/protocol.md#message-types Provided contextual callbacks to FirmataParser to allow multiple instances to run simultaneously.
|
Passes all of my IDE compile tests. Looks like arduino-builder is broken for IDE versions older than 1.6.12 (but that's Arduino's problem). Still need to test on actual hardware. Will do that tomorrow or Sat. After that I'm out for a couple of weeks. |
| @@ -398,7 +437,11 @@ void FirmataClass::attach(uint8_t command, systemCallbackFunction newFunction) | |||
| */ | |||
| void FirmataClass::attach(uint8_t command, stringCallbackFunction newFunction) | |||
| { | |||
| parser.attach(command, (stringCallbackFunction)newFunction); | |||
| switch (command) { | |||
There was a problem hiding this comment.
Does this need to be a switch statement? Would an if statement be more performant (I'm not sure if an if or switch statement is better performance wise when there is only a single condition)? I don't foresee having other types of string callback functions.
There was a problem hiding this comment.
I can't imagine a more performant branch than an if, but I was assuming they would distill down into the same assembly; I will check into it and get back. I just wanted to keep the same semantics, but I wouldn't sacrifice performance for it.
There was a problem hiding this comment.
I just confirmed the binary is the same exact size, when using the single case switch or an if statement.
| * (16384). To increase the pin range or value, see the documentation for the EXTENDED_ANALOG | ||
| * message. | ||
| * @param pin The analog pin for which to request the value (limited to pins 0 - 15). | ||
| * @param stream_enable A zero value will disable the stream, a non-zero will enable the stream |
There was a problem hiding this comment.
remove stream_enable description
There was a problem hiding this comment.
also the @note on the next line
| @@ -194,16 +207,35 @@ int FirmataParser::setDataBufferOfSize(uint8_t * dataBuffer, size_t dataBufferSi | |||
| * DIGITAL_MESSAGE, REPORT_ANALOG, REPORT DIGITAL, SET_PIN_MODE and SET_DIGITAL_PIN_VALUE). | |||
There was a problem hiding this comment.
Can you add a description of when and why to use the context parameter? Also is this a required parameter, and if not, what is the default value?
FirmataMarshaller.cpp
Outdated
| void FirmataMarshaller::reportAnalogDisable(uint8_t pin) | ||
| const | ||
| { | ||
| reportAnalog(pin, false); |
There was a problem hiding this comment.
nit, spacing. Same in next few methods.
|
Non-serial transports aren't working. Not sure why as I don't see anything here that would affect it. |
|
Non-serial transports are working fine in master. |
|
The serial transports are a head scratcher. I'm going to dig in, then I'll report here with whatever I find. @soundanalogous Can you tell me how to reproduce the transport error? I want to dig in, but I realized I don't know where to start. |
zfields
commented
Feb 10, 2017
FirmataParser.cpp
Outdated
| @@ -87,33 +98,35 @@ void FirmataParser::parse(uint8_t inputData) | |||
| switch (executeMultiByteCommand) { | |||
| case ANALOG_MESSAGE: | |||
| if (currentAnalogCallback) { | |||
| (*currentAnalogCallback)(multiByteChannel, | |||
| (*currentAnalogCallback)(this, | |||
There was a problem hiding this comment.
this should be currentAnalogCallbackContext
same for all other this context parameters
Do you have a MKR1000, an ESP8266 board or an Arduino 101? If you have a MKR1000 or ESP8266 board you can run StandardFirmataWiFi (note the options in wifiConfig.h) on the board and then run the a corresponding client or server firmata.js example from here: https://github.com/soundanalogous/firmata-client-examples/tree/master/firmata-js/network. It's helpful to enable SERIAL_DEBUG to get the DHCP address, etc from the board. If you have an Arduino 101, load StandardFirmataBLE and then run this firmata.js example: https://github.com/soundanalogous/firmata-client-examples/blob/master/firmata-js/ble/ble-ard101-test.js. Run these from master first to see what to expect. Sometimes the network connection can be a bit testy. |
|
I broke out my Adafruit Huzzah Feather ESP8266, but I haven't had much luck. Admittedly, I'm a novice with Node, but I cloned Do I need to install v8 seperately, or does it come down as a dependency? Any help, advice or README's would be appreciated. node --version = v7.5.0 Here's my error: |
|
I think @turkycat wrote a TCP client for remote wiring so you may want to see if that is still compatible if you don't want to deal with node (plus it would be nice to have more Firmata network examples for languages other than JavaScript). Regarding Node, try using node version 6 (whatever is latest v6). You can use node version manager to easily switch between versions. Even node.js version numbers are stable releases and odd numbered are experimental releases. You should run either the blink-firmata-esp-client.js or blink-firmata-esp-server.js script. Which one depends on how StandardFirmataWiFi is configured. By default it's configured as a TCP server so you wold run blink-firmata-esp-client.js. You can configure as a TCP client by uncommenting this define. Also enable serial debugging by uncommenting out this line. The ESP HUZZAH board loads so fast that you'll probably have to reset the board once after flashing (and you have USB connection). Then look at the output in the serial console. |
|
Okay, switching down to node --version = 6.9.5 made things happen in the console, but no lights on the board were blinking. Interestingly, I did get the same console spew, regardless of whether I was using the "core" or "master" branch. However, I still got this extremely long error before I got the |
|
It looks like the errors are coming from the node-serialport module. When you run Make sure you run |
|
I have pretty much installed everything brand new. I'm getting loads of Install spew |
|
The pin you chose, number 8, is not the built-in LED for the Adafruit Feather HUZZAH. Once I changed it to 0, everything worked as expected. All the errors and warning messages were coming from Boron release of Node. Once, I changed back to Argon everything seems to work without issue. |
|
In conclusion, I am unable to reproduce the error you originally encountered when testing my change set. ;-) @soundanalogous I know I just compiler bombed this thread, so I just wanted to pull your eye down here to show you I got everything working in both the |
|
Pulled your latest and tested with hardware again and had no issues this time. Merging... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the core functionality of Firmata (https://github.com/firmata/protocol/blob/master/protocol.md#message-types) in FirmataMarshaller. It also provides full blown contextual callbacks in the FirmataParser. It does not disrupt the original FirmataClass API.
Provides a stop gap for issues #318, #320, #321
Also accidently contains PR #330 (I believe it will automatically close that PR if accepted).
I have checked to see that it compiles for all the flavors I have in my IDE. It fails for the robot and then NG citing space as the issue. It passes your test cases and I've confirmed it works using the Windows Remote Arduino Experience app in the Windows app store.
I would love a code review, and can you please run your battery of tests once you have a chance?