Closed jonathanperret closed 3 months ago
[!CAUTION]
Review failed
The pull request is closed.
The recent changes in src/ayab/main.cpp
involve a significant refactor of singleton instance management. Instead of using constexpr
pointers to dynamically allocated instances, the code now utilizes stack-allocated instances, enhancing memory management safety and performance by eliminating potential memory leaks and overhead from heap allocations.
Files | Change Summary |
---|---|
src/ayab/main.cpp |
Replaced constexpr pointers to singleton classes with direct stack-allocated instances. |
Objective | Addressed | Explanation |
---|---|---|
Memory gets corrupted when processing serial messages (#190) | ✅ |
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Issue
The classes that make up the AYAB firmware are currently dynamically allocated with
new
at the top level ofmain.cpp
:https://github.com/AllYarnsAreBeautiful/ayab-firmware/blob/e960ad4d9cd86daa4b3abbafad90de51ad102da5/src/ayab/main.cpp#L50-L56
Since these objects are allocated at the start of the program and never freed, there is no advantage to having them dynamically allocated. On the other hand, there are disadvantages:
That last point in particular can hide the fact that the memory requirements for the firmware exceed the target microcontroller's RAM size (2048 bytes for the Atmega328p on an Arduino Uno).
Proposed solution
This PR replaces the use of
new
by the creation of statically allocated instances. The global pointers are then initialized to point to these instances.Before this PR, here is what the output of the PlatformIO builder looks like:
Which looks reasonable. But after including the firmware's main objects in the statically allocated area as this PR does, here is the output:
Revealing that the memory actually available for the stack is much less than was suggested by the earlier output, causing issues like #190.
The memory saved here seems to fix #190 partially. At least, even with stack overflow detection enabled, the firmware can now survive
reqInit
and even knitting, from my quick test.Note that this PR also deletes a series of uninitialized constant pointers that appear to have been remains from an earlier attempt at these global pointers.
Summary by CodeRabbit
new
for singleton instances.