Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static dbg buffer #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Static dbg buffer #1

wants to merge 1 commit into from

Conversation

eddyp
Copy link
Contributor

@eddyp eddyp commented Feb 15, 2020

Runtime allocated data, be it on the stack or heap, is often a problem for memory safety, since large enough data or stacks can overwrite each other or maybe stuff in other memory regions such as .data or .bss, depending on the memory layout (i.e. linker script and program's data).

These allocations' sizes aren't that obvious at compile, and such a memory corruption error could silently exist in the system, but symptoms of it iaren't necessarily obvious or visible (yay for subtle bugs /sarcasm).

In order to make the system more predictable/obvious from compile time (e.g. the static RAM usage vs. how much is left for heap/stack) it is desirable to change stack allocations into statically allocated buffers, use the heap as little as possible, and so on.

The DEBUG* macros are a source of such dynamic allocation on the stack, but given the typical linear execution of the code, it should be safe to reuse a global buffer for the DEBUG messages instead of allocating on the stack.

Even our main objects (WaterSystem, SensorAndPump, WaterSystemSM and the future logging and GeneROMst objects) could be statically allocated and use an init function instead of the constructor to do the runtime intitalization, so the RAM size usage and dynamic needs would be a lot more easier to estimate from compile time data, since only a small amount of objects/data would be on the heap and the stack. This avenue should be explored, too.

@eddyp
Copy link
Contributor Author

eddyp commented Feb 15, 2020

This change was intentionally NOT merged/added directly due to known failing unit tests - MAX_DEBUG_MSG_LEN definition is missing from the stubs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant