search

crownstone / bluenet

Bluenet is the in-house firmware on Crownstone hardware. Functions: switching, dimming, energy monitoring, presence detection, indoor localization, switchcraft.
https://crownstone.rocks
91 stars 62 forks source link

Writing to serial should be limited #98

Closed mrquincle closed 3 years ago

mrquincle commented 3 years ago

A call to cs_log_printf is not restricted in size.

#1  0x000301e6 in cs_log_printf (str=0x60e66 "[%-30.30s : %-4d] heapEnd=0x%X maxHeapEnd=0x%X minStackEnd=0x%X minFree=%u sbrkFails=%u\r\n") at /home/anne/workspace/bluenet/source/src/logging/cs_Logger.cpp:53

It should make use of the outgoing uart buffer. The C and C++ code should not use different buffers. There should not be dynamic memory allocation for print statements.

mrquincle commented 3 years ago

This particular call allocates 520 bytes and frees 520 bytes in quick succession.

mrquincle commented 3 years ago

Another reason not do memory allocation in print statements, is that it makes it harder to put print statements in memory allocation routines.

vliedel commented 3 years ago

cs_log_printf is meant as a convenience in case you don't want to use binary logging. If you care about memory and speed, use binary logging instead. An option would be to simply ignore any cs_log_printf that would need more than 128B, or maybe we could use cs_clog instead, which streams directly to uart instead of to a buffer first.

On Thu, 4 Feb 2021 at 21:50, Anne van Rossum notifications@github.com wrote:

Another reason not do memory allocation in print statements, is that it makes it harder to put print statements in memory allocation routines.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/crownstone/bluenet/issues/98#issuecomment-773595376, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPLGQJMQSX3SWB46VOQ6OLS5MB7VANCNFSM4XDIUNKA .

mrquincle commented 3 years ago

Check. The buffer of size UART_TX_BUFFER_SIZE has a different function that I anticipated. It is used as an intermediate buffer to write into from, for example, the ADC. This can be done asynchronously. That is, in the meantime other things can be written to the console. Hence, this buffer is hard to re-use for this purpose.

Easy to implement would then be the removal of malloc for non binary logging. Hence, just use snprintf and write up to n. Due to the fact that __func__ takes many bytes, it might be worthwhile to only print the data without prefix.

vliedel commented 3 years ago

Fixed with 45b984ce3b2f005cbd7fa3a674722cc1b902e00d