Conversation
…value has changed.
This reverts commit f754c9c.
|
Thanks for your contribution! Could you provide a comparison of the memory footprint (previous & now)? Thanks in advance! |
|
I measure free heap and max heap block and here it is the result:
It is a memory reduction of over 5000 bytes. |
|
Hold on, since I am experiencing memory leaks. It needs further research. |
|
I forgot to update... it is was a false alarm, my devices was restarting from time to time and I thought it was the new code, but it wasn't that 👍 Please, review it and let me know if the PR requires some refinement. |
|
Hi again! Will you please review and advice? |
| static const char OFF_REASONS_13[] PROGMEM = "Unknown: Bit 14"; | ||
| static const char OFF_REASONS_14[] PROGMEM = "Unknown: Bit 15"; | ||
| static const char OFF_REASONS_15[] PROGMEM = "Unknown: Bit 16"; | ||
| static const char *const OFF_REASONS[OFF_REASONS_SIZE] PROGMEM = { |
There was a problem hiding this comment.
Please don't touch the OFF_REASONS_SIZE/OFF_REASONS. There is no much benefit here.
There was a problem hiding this comment.
I implemented it to be coherent.
There was a problem hiding this comment.
In addition, it removes 297 bytes from main memory, that is, the size of all the strings.
| OFF_REASONS_12, OFF_REASONS_13, OFF_REASONS_14, OFF_REASONS_15}; | ||
|
|
||
| void VictronComponent::dump_config() { // NOLINT(google-readability-function-size,readability-function-size) | ||
| static const char *prefix = " "; |
There was a problem hiding this comment.
Could you revert this change too? It gets optimized by the compiler already and has no benefit.
There was a problem hiding this comment.
I think I tried it before and it didn't reduce the memory footprint, but I will check it again.
| } | ||
|
|
||
| static std::string charging_mode_text(int value) { | ||
| static const char *charging_mode_text(int value) { |
There was a problem hiding this comment.
We could use inline PROGMEM strings here:
static const char *charging_mode_text(int value) {
static char buffer[30];
const char *result;
switch (value) {
case 0: result = PSTR("Off"); break;
case 1: result = PSTR("Low power"); break;
case 2: result = PSTR("Fault"); break;
case 3: result = PSTR("Bulk"); break;
// ...
default: result = PSTR("Unknown"); break;
}
strncpy_P(buffer, result, sizeof(buffer) - 1);
return buffer;
}
What do you think about it?
There was a problem hiding this comment.
I had a look at it again, and it is already using PROGMEM, see at the beginning of the file for the definition of each CHARGING_MODE_X.
There was a problem hiding this comment.
I think moving all the strings to the beginning of the file helps identifying the strings and improves code readability.
I tried the strncpy_P but I got an error while compiling on the CI, see 93b60c4
2 participants
This PR provides a heavy optimization over the strings used for text sensors, moving them from main SRAM memory to PROGMEM. In order to do it, it allocates static buffers for each text sensor up to the size of the max length of the strings. Although it requires static allocation, it prevents memory fragmentation and allow freeing a significant amount of memory.
In addition it includes an optimization on the management of the loop to show stats when the loop is taking too long.
And it also solves a recurrent "Too old data" issue by updating last_transmission when data is not available.