Complete base API #350

zfields · 2017-02-24T04:55:12Z

This commit is intended to be the first of many addressing any missing APIs listed in issue #349

soundanalogous · 2017-02-26T17:32:09Z

FirmataMarshaller.cpp

+void FirmataMarshaller::sendPinStateQuery(uint8_t pin)
+const
+{
+  sendSysex(PIN_STATE_QUERY, sizeof(pin), &pin);


sendSysex will send pin as 2 bytes.

great catch! fixed.

includes support function transformByteStreamToMessageBytes(), which replaces sendValueAsTwo7bitBytes()

soundanalogous · 2017-02-26T20:27:17Z

FirmataMarshaller.cpp

+ * @param bytev A pointer to the array of data bytes to send in the message.
+ * @param max_bytes Force message to be n bytes, regardless of data bits.
+ */
+void FirmataMarshaller::transformByteStreamToMessageBytes (size_t bytec, uint8_t * bytev, size_t max_bytes)


There is a similar Encoder7Bit class in ConfigurableFirmata that both reads and writes: https://github.com/firmata/ConfigurableFirmata/blob/master/src/Encoder7Bit.cpp

and example use to read and write

Cool, I wish I had seen it before! ;-) However, just looking at it (and mulling it over) briefly, it looks like another dependency that will need to be managed.

Since this is a self-contained, private function, we will be able to refactor it at any time, without impacting the external API. For now, I would like to move forward using the current implementation, at least until ConfigurableFirmata becomes a little more mainstream (or Encoder7Bit is refactored to consume/extend the FirmataMarshaller API) and gets pulled under the firmata namespace.

zfields · 2017-02-26T20:51:26Z

@soundanalogous I responded inline to your suggestion. In short, I think it's a great idea, but is non-blocking and would be better suited during a larger refactoring.

Otherwise, I have tested this with every means I have available, and everything looks good.

Windows Remote Arduino -> Uno
firmata_tests.ino
ESP8266 StandardFirmataWiFi from node.

soundanalogous · 2017-02-26T21:07:53Z

FirmataMarshaller.cpp

 * @param value The value of the analog pin (0 - 1024 for 10-bit analog, 0 - 4096 for 12-bit, etc).
- * The maximum value is 14-bits (16384).


Add this back in with the updated max. I assume it's 32 bits. However different firmata client libraries may interpret this in different ways (I think some only expect a max of 16 bits), but that shouldn't break anything unless a library looks for a specific number of bytes rather than accepting all bytes up to END_SYSEX (but that would be the fault of the client library).

It is, in fact, still 14-bits. I removed it when I thought it would be changing. I have replaced the note. I amended the last commit because it was aptly named.

It's limited to 14 bits when using ANALOG_MESSAGE, but sendAnalog now calls sendExtendedAnalog so larger values should be allowed. No cap is defined in the protocol documentation but I think 32-bit is probably reasonable (although not sure I've ever seen hardware that supports that resolution).

here's the equivalent in firmata.js for example: https://github.com/firmata/firmata.js/blob/master/lib/firmata.js#L776-L806

soundanalogous · 2017-02-26T21:09:57Z

FirmataMarshaller.cpp

-  FirmataStream->write(ANALOG_MESSAGE | (pin & 0xF));
-  sendValueAsTwo7bitBytes(value);
+
+  if ( (0xF >= pin) && (0x3FFF >= value) ) {


this should be || rather than && since it's possible to have a pin number > 15 with a value > 14 bits

oh wait, this is fine as is. I'm not used to the static value being on the left side of the comparison so it confused me for a second.

Ha! Yeah, I used to be teaching assistant, so I always taught the kids to put it on the left because you can't accidentally set it when trying to compare.

zfields · 2017-02-26T22:07:11Z

Should we add something around the reset functionality while we are doing this? Got any names in mind?

soundanalogous · 2017-02-26T22:46:37Z

It's purely a software reset. A user crafting their own Firmata sketch can do anything they want in the callback. StandardFirmata initializes everything to a known state. FirmataParser resets the buffer.

So naming is a bit tricky. systemReset could imply a HW reset so that may be misleading. Perhaps softReset or maybe systemReset is okay and we just document that it doesn't reset the physical board, it simply resets the buffer and the user can do anything else they want in the systemResetCallback.

zfields · 2017-02-27T03:08:30Z

Please review the three new functions queryFirmwareVersion, queryVersion, systemReset.
I have rerun all the tests I am able to and I'm still passing. I think we should pull this in (as the marshalling functionality is "complete"), and decide the fate of the parser (whether to add or subtract callback support) over the next week/weekend.

soundanalogous · 2017-02-27T03:12:52Z

Sounds good. Have you compiled against a bunch of boards in the IDE (even if you don't have those actual boards - including ones like the Intel boards which have the fussier c++ compiler)?

zfields · 2017-02-27T03:49:19Z

It compiles on ESP8266, 101 and AVR without warning (at least not caused by me ;-) ).

zfields · 2017-02-27T03:54:31Z

Just downloaded the SAMD toolchain, and it builds for MKR1000

soundanalogous · 2017-02-27T06:00:45Z

Ran through my usual compile tests in Arduino 1.8.1 (I really need to spend time to figure out a way to automate this at some point). Also tested with Arduino 1.6.12, 1.5.8 and 1.0.6. Fails with Arduino 1.6.11 but it appears to be an issue with arduino-builder, not Firmata (have noticed this previously as well... I guess Arduino is not testing older versions anymore, or maybe they only test the most recent 2 versions).

One thing to be mindful of is the increase in program memory. Compiling StandardFirmataPlus for an Arduino Uno in Arduino 1.8.1 with Firmata v2.5.4 consumes 13924 bytes of Flash and 1244 bytes of SRAM. However with this most recent PR we are up to 15278 bytes of Flash (increase of 1354 bytes - this is all changes since v2.5.4 was released... not just this PR) and 1298 bytes of RAM. I'm not too concerned about the increased RAM since it's only +54 bytes, but we should keep and eye on the Flash increase since we've added 1354 bytes so far without adding any new Firmata features.

zfields · 2017-02-27T06:38:48Z

Fair point. I expect much of this has to do with allowing the callbacks to have context, which automatically doubles the storage required for each callback, but allows for multiple instances and provides enormous functional gains. Not to mention, I had to make a callback abstraction to patch between the old static callbacks and the new stateful callbacks. Depending on where you are looking to go with the FirmataParser I could see it taking on a little more water or shedding a little weight. However, I believe an honest, deliberate refactor can produce a significant memory footprint improvement. It's usually stuff you don't expect, but I would advise against cutting functionality as it is usually not necessary.

soundanalogous · 2017-02-27T07:07:23Z

Yeah I figure we can reduce usage through a refactor in the future. Cleaning up the StandardFirmata variants at some point will probably also help bring the Flash usage size down.

zfields force-pushed the mp_complete branch from eb7f2d1 to c2877ae Compare February 26, 2017 09:37

zfields changed the title ~~[DO NOT PULL] sendPinStateQuery~~ [DO NOT PULL] complete base API Feb 26, 2017

soundanalogous reviewed Feb 26, 2017

View reviewed changes

zfields added 2 commits February 26, 2017 11:05

sendPinStateQuery

7165b6a

setSamplingInterval

2e09889

zfields force-pushed the mp_complete branch 3 times, most recently from ce5e8b2 to 4006e1d Compare February 26, 2017 19:49

sendExtendedAnalog

d0bef9e

includes support function transformByteStreamToMessageBytes(), which replaces sendValueAsTwo7bitBytes()

zfields force-pushed the mp_complete branch from 4006e1d to d1ccdd9 Compare February 26, 2017 19:53

soundanalogous reviewed Feb 26, 2017

View reviewed changes

zfields changed the title ~~[DO NOT PULL] complete base API~~ Complete base API Feb 26, 2017

soundanalogous reviewed Feb 26, 2017

View reviewed changes

Address bug caught code review

fa5b3d6

zfields force-pushed the mp_complete branch from d1ccdd9 to fa5b3d6 Compare February 26, 2017 21:33

zfields force-pushed the mp_complete branch 5 times, most recently from 292aa1e to 42960d3 Compare February 27, 2017 02:24

Add system calls to Marshaller

9433b27

zfields force-pushed the mp_complete branch from 42960d3 to 9433b27 Compare February 27, 2017 03:46

soundanalogous merged commit 44e9026 into firmata:master Feb 28, 2017

zfields deleted the mp_complete branch February 28, 2017 07:47

		* @param value The value of the analog pin (0 - 1024 for 10-bit analog, 0 - 4096 for 12-bit, etc).
		* The maximum value is 14-bits (16384).

Complete base API #350

Complete base API #350

Uh oh!

Conversation

zfields commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields commented Feb 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields commented Feb 26, 2017

Uh oh!

soundanalogous commented Feb 26, 2017

Uh oh!

zfields commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soundanalogous commented Feb 27, 2017

Uh oh!

zfields commented Feb 27, 2017

Uh oh!

zfields commented Feb 27, 2017

Uh oh!

soundanalogous commented Feb 27, 2017

Uh oh!

zfields commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soundanalogous commented Feb 27, 2017

Uh oh!

Uh oh!

zfields commented Feb 24, 2017 •

edited

Loading

zfields Feb 26, 2017 •

edited

Loading

zfields Feb 26, 2017 •

edited

Loading

zfields commented Feb 27, 2017 •

edited

Loading

zfields commented Feb 27, 2017 •

edited

Loading