WIP:Reduce amount of RAM statically allocated by CommandHandler

[zserg8]

This PR reduces the amount of RAM allocated by CommandHandler.

The current memory footprint of CommandHandler is about 230 bytes which might not seem a lot. However, when multiple devices are being used through CommandManager this quickly becomes a problem as every device carries it's own copy of CommandHandler, thus exhausting the free RAM on the MCU pretty quickly.

Most of the RAM allocated by CommandHandler is occupied by three buffers, 65 bytes each:

Arduino-CommandHandler/CommandHandler.h

Line 132 in c978d37

char buffer[COMMANDHANDLER_BUFFER + 1]; // Buffer of stored characters while waiting for terminator character

and the next few lines.

The current PR does the following:

• Made command[COMMANDHANDLER_BUFFER + 1] local and dynamically allocated to getOutCmd() as it is the only place where it is being used.

• Replaced remaining() function with the inline code inside processChar(). This allowed to make remains[COMMANDHANDLER_BUFFER + 1] buffer dynamically allocated as well. The only other place except processChar() where remaining() was used was the example sketch which has been updated accordingly.

• The potential problem with dynamic allocation can be heap fragmentation, especially on microcontrollers without dedicated MMU, so this is something to watch out for. To make it easier, extra debug output was added to indicate the amount of free SRAM left after each processChar() iteration.

Finally, some testing.

Two test sketches was used, one for RAMPS shield, another one for SensorHub shield. The hardware configuration was as following:

• RAMPS: 5 stepper motors, 2 PWM out pins, 2 ADC in pins, 2 digital in pins, 3 digital out pins. 14 objects in total.

• SensorHub: 12 PWM out pins, 8 ADC in pins, 8 digital in pins, 8 digital out pins. 36 objects in total.

RAMPS, before optimization: 4385 bytes in RAM
RAMPS, after optimization: 2435 bytes in RAM, 45% decrease.

SensorHub before optimization: 9159 bytes in RAM (didn't fit into 8K on Mega2560).
SensorHub after optimization: 4349 bytes in ram, 52% decrease.

As usual, more hardware testing is much welcome before merge.

[jgrizou]

Great idea to tackle this. I am obviously a bit rusted haven't touched this code for 3 years and I cannot test this at home so take my inputs as such.

As you said the risk with dynamic allocation is that you do not have enough when required or run into heap fragmentation. I guess you will only know after testing for few weeks with various setups and communication load.

Another option is to keep all memory pre-allocated and reduce the size of the buffers. I can't remember if there was any specific reason to pick the current size.

Other than that, I don't see obvious issues in the code.

[zserg8]

Turns out it still shits itself with SensorHub firmware - needs more RAM, only 328 bytes left after creating local variables. :/

[Tyrannican]

The FIXME should be addressed

[Tyrannican]

This should probably be addressed

[Tyrannican]

Seems grand, unable to test however as the one board i have is fried...
Minor fix there and accepted.

[jgrizou]

Turns out it still shits itself with SensorHub firmware - needs more RAM, only 328 bytes left after creating local variables. :/

Then I propose we do not merge this on the master branch?

It is important for a user of this library to be sure that if the program fit in RAM, then will be no memory issues coming from this library. When they occur, theses issues are very painful to identify when they arise from a sub-library like this one.

[zserg8]

Sure we don't, that's why I've added WIP & already chilled down Graham's enthusiasm a bit. I'll come back to this issue after some time.