fixed memory leak in string buffer

[soundanalogous]

See this issue.

The string buffer memory was never freed in the past. This is a long standing bug. I'd just merge this but want to make sure that no one has any concerns about the implementation. The buffer is freed after the stringCallback returns. If anyone still needed a reference to the string after the callback returns, the reference would be lost (maybe I've just been writing too much JavaScript lately and this is a non-issue in c since it's procedural).

The alternative however is less attractive. That is to rely on the user to free the pointer in their callback function. If they fail to do this and are sending multiple strings from their client application, they'll eventually crash their arduino sketch.

Also added a unit test to verify memory is now properly freed. Uncommented memory test for setFirmwareVersion as well so now all allocated memory is tested that it is properly freed.

Any comments?

[soundanalogous]

updated this so I could test in my client application. All examples should probably be updated to use these constants.

[franklinatportland]

Thanks for fixing it. I ended up not using String Sysex because of that.

[soundanalogous]

I've spent a few hours researching potential issues of using malloc and free with Arduino (and AVR in general). Based on this research I'm not sure that the approach I've taken here is safe. The problem is an application may have other libraries that are also using malloc and free (for example if the user is using the Arduino String method). There is a risk then of ending up with fragmented memory and eventually the program will crash if there are no free blocks of memory large enough for a larger incoming string. For this reason I think a better approach would be to allocate an array of 32 chars for incoming strings. Currently the largest string you can receive is 15 characters (due to the 32 byte limit on the input data buffer). I'm going to raise this limit to 64 bytes so a 32 char buffer for incoming strings will be sufficient. This would take up an additional 32 bytes of RAM but I'm also going to reduce current ram usage in StandardFirmata by 93 bytes by moving the error strings to program memory so it may be a fare tradeoff.

The only issue then is if anyone is currently storing references to the incoming strings in their application (does not apply to anyone using StandardFirmata since incoming strings have never been supported). Any of the changes I've described would break such code since they would only allow a single incoming string to be in memory at a time.

[ntruchsess]

since the decodet string is only half the size of the sysex-message, it would be possible to decode the string within firmatas input buffer not needing to allocate any additional memory. With all solutions that do not pass a newly allocatet block of memory to the application, the application would have to copy incoming strings to its own memory anyway.

[soundanalogous]

Good idea! I've updated per your suggested. Also ensuring string is null-terminated (this was an issue with client libraries in languages that don't null-terminate strings). Increased input data buffer to 64 bytes as well to accommodate up to 29 byte strings (Arduino serial buffer is limited to 64 bytes. START_SYSEX, STRING_DATA and END_SYSEX take up 3 bytes).

Also, I've been in communication with the Arduino team and will be merging the latest version of Firmata (master) into the Arduino repo within the next week or two.