search

firmata / arduino

Firmata firmware for Arduino
GNU Lesser General Public License v2.1
1.54k stars 516 forks source link

[v3.0.0] Sysex String Callback #353

Closed zfields closed 7 years ago

zfields commented 7 years ago

https://github.com/firmata/arduino/blob/master/FirmataParser.cpp#L420

This seems like it should be passing the string without the from index 1 instead of 0. It is a small and easy change, but it would change the behavior*, so it probably needs to wait until v3.

*It has always been this way...

soundanalogous commented 7 years ago

It shouldn't make any difference to change since the first byte is skipped here: https://github.com/firmata/arduino/blob/master/FirmataParser.cpp#L403. Couldn't you simply change i to 0 on line 403 and then change the index to 1 on line 420? Either way I think achieves the same result.

soundanalogous commented 7 years ago

I wonder if there was a regression when you refactored to use bufferDataAtPosition. The way I used to do at (and in hindsight I have no idea why I handled it this way) was shift the data back one byte in the storedInputData array: https://github.com/firmata/arduino/blob/2.5.3/Firmata.cpp#L246-L254

zfields commented 7 years ago

If it is a regression then we should just change it back (#356).

soundanalogous commented 7 years ago

I'll have to test it to know for sure. The string callback isn't used in any of the StandardFirmata variants, so I overlooked it during all of the refactoring work.

soundanalogous commented 7 years ago

I guess a unit test would catch this as well.

zfields commented 7 years ago

We should definitely have unit tests, they would have shown us that it is correct! :-P