introduce psram-aware malloc functions (backport of upstream #4895)#342
Open
softhack007 wants to merge 19 commits intomdevfrom
Open
introduce psram-aware malloc functions (backport of upstream #4895)#342softhack007 wants to merge 19 commits intomdevfrom
softhack007 wants to merge 19 commits intomdevfrom
Conversation
* Improved heap and PSRAM handling - Segment `allocateData()` uses more elaborate DRAM checking to reduce fragmentation and allow for larger setups to run on low heap - Segment data allocation fails if minimum contiguous block size runs low to keep the UI working - Increased `MAX_SEGMENT_DATA` to account for better segment data handling - Memory allocation functions try to keep enough DRAM for segment data - Added constant `PSRAM_THRESHOLD` to improve PSARM usage - Increase MIN_HEAP_SIZE to reduce risk of breaking UI due to low memory for JSON response - ESP32 makes use of IRAM (no 8bit access) for pixeluffers, freeing up to 50kB of RAM - Fix to properly get available heap on all platforms: added function `getFreeHeapSize()` - Bugfix for effects that divide by SEGLEN: don't run FX in service() if segment is not active -Syntax fix in AR: calloc() uses (numelements, size) as arguments * Added new functions for allocation and heap checking - added `allocate_buffer()` function that can be used to allocate large buffers: takes parameters to set preferred ram location, including 32bit accessible RAM on ESP32. Returns null if heap runs low or switches to PSRAM - getFreeHeapSize() and getContiguousFreeHeap() helper functions for all platforms to correctly report free useable heap - updated some constants - updated segment data allocation to free the data if it is large - replaced "psramsafe" variable with it's #ifdef: BOARD_HAS_PSRAM and made accomodating changes - added some compile-time checks to handle invalid env. definitions - updated all allocation functions and some of the logic behind them - added use of fast RTC-Memory where available - increased MIN_HEAP_SIZE for all systems (improved stability in tests) - updated memory calculation in web-UI to account for required segment buffer - added UI alerts if buffer allocation fails - made getUsedSegmentData() non-private (used in buffer alloc function) - changed MAX_SEGMENT_DATA - added more detailed memory log to DEBUG output - added debug output to buffer alloc function
* p_malloc() functions always available, to simplify code by avoiding ifdef's * simplify extern definitions * always use SPIRAM as fallback * implement calloc(), instead of relying on malloc() * we don't (yet) use allocate_buffer() * remove some commented-out code
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DRAM-biased (d_) and PSRAM-preferring (p_) allocation APIs and platform-aware wrappers, replaces direct heap calls across many modules to route allocations through these helpers, exposes allocator declarations publicly, and tightens visibility for several date string constants. Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Caller (FX / JSON / Preset / Usermod)
participant AllocAPI as Allocator API (d_/p_)
participant HeapChk as Heap Validator
participant MemPool as Memory Pools (DRAM / PSRAM / RTC)
Module->>AllocAPI: request allocate(size, preferences)
AllocAPI->>HeapChk: check MIN_HEAP_SIZE / contiguous space
alt PSRAM preferred & available
AllocAPI->>MemPool: p_malloc / p_calloc / p_realloc_malloc
else DRAM/RTC fallback
AllocAPI->>MemPool: d_malloc / d_calloc / d_realloc_malloc
end
MemPool-->>AllocAPI: pointer or NULL
AllocAPI-->>Module: pointer or NULL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (1)
366-383:⚠️ Potential issue | 🟠 MajorUse 16‑bit counters for mode indices (palette stays 8‑bit).
modeIndexisuint8_t, so when mode count exceeds 255 it wraps and the new bounds check can still overwrite earlier slots. Switch mode-related indices/arrays touint16_t(keep palette indices asuint8_t) and add a null guard formodeStrings.🛠️ Proposed fix
- const char **modeStrings = (const char **)d_calloc(numModes, sizeof(const char *)); - uint8_t modeIndex = 0; + const char **modeStrings = (const char **)d_calloc(numModes, sizeof(const char *)); + if (!modeStrings) return nullptr; + uint16_t modeIndex = 0;Based on learnings: In MoonModules/WLED-MM PR
#270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
🤖 Fix all issues with AI agents
In `@usermods/artifx/arti.h`:
- Around line 2567-2571: The allocation for programText using d_malloc can
return nullptr; before calling programFile.read or dereferencing programText,
check the result of d_malloc and handle failure (e.g., log an error via the
existing error/reporting mechanism and return or exit cleanly). Specifically,
after programText = (char *)d_malloc(programFileSize+1) validate programText !=
nullptr, avoid calling programFile.read((byte *)programText, programFileSize) if
null, and ensure any resources (like programFile) are closed or cleaned up
before bailing; reference programFileSize, programText, d_malloc, and
programFile.read to locate and implement this guard.
In `@usermods/audioreactive/audio_reactive.h`:
- Around line 457-464: The allocation sequence allocates vReal then vImag then
pinkFactors but returns early on failure without freeing any earlier
allocations; update the failure paths in the allocation block so that if vImag
allocation fails you call d_free(vReal) before returning false, and if
pinkFactors allocation fails you call d_free(vReal) and d_free(vImag) (and
p_free(pinkFactors) only if non-null) before returning false; use the existing
d_free and p_free helpers and the same null-check style used elsewhere to avoid
leaks (references: vReal, vImag, pinkFactors, d_calloc, p_calloc, d_free,
p_free).
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`:
- Around line 354-358: The function RotaryEncoderUIUsermod::re_initIndexArray
calls d_calloc and immediately dereferences the returned pointer; add a null
check after the allocation (check if indexes == nullptr) and if allocation
failed return nullptr (or handle the error) before entering the for loop that
writes to indexes, so the loop doesn't dereference a null pointer.
In `@wled00/fcn_declare.h`:
- Line 518: The comment for the macro BFRALLOC_ENFORCE_PSRAM is inaccurate: the
allocation logic in util.cpp treats this flag as "PSRAM-only" and returns
nullptr if PSRAM is not available rather than falling back to DRAM. Update the
macro comment for BFRALLOC_ENFORCE_PSRAM to state that it enforces PSRAM-only
allocation and will fail (return nullptr) when PSRAM is unavailable, or
alternatively change the allocation logic in util.cpp to implement a DRAM
fallback; reference the macro name BFRALLOC_ENFORCE_PSRAM and the allocation
routine in util.cpp that checks this flag when making your change.
In `@wled00/presets.cpp`:
- Around line 80-86: The code allocates tmpRAMbuffer with p_malloc but some
deallocation paths still call free(); update those deallocation sites (e.g., the
temp preset load path) to call p_free(tmpRAMbuffer) instead of
free(tmpRAMbuffer) so the PSRAM allocator pairing is consistent; search for any
other occurrences of free(tmpRAMbuffer) and replace them with p_free to ensure
all allocations via p_malloc are freed with p_free.
In `@wled00/util.cpp`:
- Around line 817-827: The current d_realloc_malloc uses heap_caps_realloc_*
which can transiently allocate extra heap; change it to free the old buffer
before allocating the new one to avoid double-heap spikes: in function
d_realloc_malloc, remove the direct heap_caps_realloc_* path and instead call
d_free(ptr) first and then allocate a new buffer with d_malloc(size) (or the
appropriate heap_caps_malloc variant matching previous caps), validate the
allocation with validateFreeHeap, and return the new buffer (or NULL on
failure); ensure you only free once (use ptr only before allocation) and
preserve the existing validateFreeHeap and fallback behavior previously returned
by heap_caps_realloc_*.
🧹 Nitpick comments (3)
wled00/const.h (1)
531-550: AllowMIN_HEAP_SIZEoverrides and remove the redundant fallback.
MIN_HEAP_SIZEis defined unconditionally, then a fallback for the undefined case remains. This blocks build-flag overrides and can trigger macro redefinition warnings. Wrap the new block in#ifndef MIN_HEAP_SIZEand remove (or relocate) the later fallback.♻️ Suggested refactor
-// minimum heap size required to process web requests: try to keep free heap above this value -#ifdef ESP8266 - `#define` MIN_HEAP_SIZE (9*1024) -#else - `#define` MIN_HEAP_SIZE (15*1024) // WLED allocation functions (util.cpp) try to keep this much contiguous heap free for other tasks -#endif +// minimum heap size required to process web requests: try to keep free heap above this value +#ifndef MIN_HEAP_SIZE + `#ifdef` ESP8266 + `#define` MIN_HEAP_SIZE (9*1024) + `#else` + `#define` MIN_HEAP_SIZE (15*1024) // WLED allocation functions (util.cpp) try to keep this much contiguous heap free for other tasks + `#endif` +#endif @@ -#if !defined(MIN_HEAP_SIZE) -#define MIN_HEAP_SIZE 8192 -#endifwled00/util.cpp (1)
840-845: Remove unreachable return inp_calloc
return buffer;afterreturn validateFreeHeap(buffer);is dead code.🧹 Cleanup
void *p_calloc(size_t count, size_t size) { // similar to p_malloc bus uses heap_caps_calloc void *buffer = heap_caps_calloc_prefer(count, size, 3, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT, MALLOC_CAP_DEFAULT); return validateFreeHeap(buffer); - return buffer; }wled00/FX_fcn.cpp (1)
2704-2712: Prefer free‑then‑alloc for mapping table growth to avoid transient double allocations.
customMappingTableis rebuilt immediately afterward, so preserving the old contents isn’t necessary. Avoiding realloc-style growth reduces peak heap usage on low-memory targets.♻️ Suggested adjustment
- if ((size > 0) && (customMappingTable != nullptr)) { - customMappingTable = (uint16_t*) d_realloc_malloc(customMappingTable, sizeof(uint16_t) * size); // reallocf will free memory if it cannot resize - } - if ((size > 0) && (customMappingTable == nullptr)) { // second try - DEBUG_PRINTLN("deserializeMap: trying to get fresh memory block."); - customMappingTable = (uint16_t*) d_calloc(size, sizeof(uint16_t)); - if (customMappingTable == nullptr) { - DEBUG_PRINTLN("deserializeMap: alloc failed!"); - errorFlag = ERR_LOW_MEM; // WLEDMM raise errorflag - } - } + if (size > 0) { + if (customMappingTable) { d_free(customMappingTable); customMappingTable = nullptr; } + DEBUG_PRINTLN("deserializeMap: trying to get fresh memory block."); + customMappingTable = (uint16_t*) d_calloc(size, sizeof(uint16_t)); + if (customMappingTable == nullptr) { + DEBUG_PRINTLN("deserializeMap: alloc failed!"); + errorFlag = ERR_LOW_MEM; // WLEDMM raise errorflag + } + }Based on learnings: “In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers … always free the old buffer before allocating the new one.”
Comment on lines
2567
to
2571
| programFileSize = programFile.size(); | ||
| programText = (char *)malloc(programFileSize+1); | ||
| programText = (char *)d_malloc(programFileSize+1); | ||
| programFile.read((byte *)programText, programFileSize); | ||
| programText[programFileSize] = '\0'; | ||
| #else |
There was a problem hiding this comment.
Handle d_malloc failure before reading the program file
d_malloc can return nullptr (especially with MIN_HEAP_SIZE constraints). The current code immediately reads into programText, which will crash on allocation failure. Please guard and bail out cleanly.
🛠️ Suggested fix
programFileSize = programFile.size();
programText = (char *)d_malloc(programFileSize+1);
+ if (programText == nullptr) {
+ ERROR_ARTI("ARTI-FX: Out of memory allocating program text (%u bytes)\n", programFileSize + 1);
+ programFile.close();
+ return false;
+ }
programFile.read((byte *)programText, programFileSize);
programText[programFileSize] = '\0';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| programFileSize = programFile.size(); | |
| programText = (char *)malloc(programFileSize+1); | |
| programText = (char *)d_malloc(programFileSize+1); | |
| programFile.read((byte *)programText, programFileSize); | |
| programText[programFileSize] = '\0'; | |
| #else | |
| programFileSize = programFile.size(); | |
| programText = (char *)d_malloc(programFileSize+1); | |
| if (programText == nullptr) { | |
| ERROR_ARTI("ARTI-FX: Out of memory allocating program text (%u bytes)\n", programFileSize + 1); | |
| programFile.close(); | |
| return false; | |
| } | |
| programFile.read((byte *)programText, programFileSize); | |
| programText[programFileSize] = '\0'; | |
| `#else` |
🤖 Prompt for AI Agents
In `@usermods/artifx/arti.h` around lines 2567 - 2571, The allocation for
programText using d_malloc can return nullptr; before calling programFile.read
or dereferencing programText, check the result of d_malloc and handle failure
(e.g., log an error via the existing error/reporting mechanism and return or
exit cleanly). Specifically, after programText = (char
*)d_malloc(programFileSize+1) validate programText != nullptr, avoid calling
programFile.read((byte *)programText, programFileSize) if null, and ensure any
resources (like programFile) are closed or cleaned up before bailing; reference
programFileSize, programText, d_malloc, and programFile.read to locate and
implement this guard.
Comment on lines
+457
to
464
| if (vReal) d_free(vReal); // should not happen | ||
| if (vImag) d_free(vImag); // should not happen | ||
| if ((vReal = (float*) d_calloc(samplesFFT, sizeof(float))) == nullptr) return false; // calloc or die | ||
| if ((vImag = (float*) d_calloc(samplesFFT, sizeof(float))) == nullptr) return false; | ||
| #ifdef FFT_MAJORPEAK_HUMAN_EAR | ||
| if (pinkFactors) free(pinkFactors); | ||
| if ((pinkFactors = (float*) calloc(samplesFFT, sizeof(float))) == nullptr) return false; | ||
| if (pinkFactors) p_free(pinkFactors); | ||
| if ((pinkFactors = (float*) p_calloc(samplesFFT, sizeof(float))) == nullptr) return false; | ||
| #endif |
There was a problem hiding this comment.
Free partially allocated FFT buffers on failure.
If vImag or pinkFactors allocation fails, vReal stays allocated and you return false, keeping the heap under pressure and hindering recovery. Free any already-allocated buffers before returning.
🛠️ Proposed fix
- if ((vImag = (float*) d_calloc(samplesFFT, sizeof(float))) == nullptr) return false;
+ if ((vImag = (float*) d_calloc(samplesFFT, sizeof(float))) == nullptr) {
+ d_free(vReal);
+ vReal = nullptr;
+ return false;
+ }
`#ifdef` FFT_MAJORPEAK_HUMAN_EAR
- if ((pinkFactors = (float*) p_calloc(samplesFFT, sizeof(float))) == nullptr) return false;
+ if ((pinkFactors = (float*) p_calloc(samplesFFT, sizeof(float))) == nullptr) {
+ d_free(vReal);
+ d_free(vImag);
+ vReal = nullptr;
+ vImag = nullptr;
+ return false;
+ }
`#endif`🤖 Prompt for AI Agents
In `@usermods/audioreactive/audio_reactive.h` around lines 457 - 464, The
allocation sequence allocates vReal then vImag then pinkFactors but returns
early on failure without freeing any earlier allocations; update the failure
paths in the allocation block so that if vImag allocation fails you call
d_free(vReal) before returning false, and if pinkFactors allocation fails you
call d_free(vReal) and d_free(vImag) (and p_free(pinkFactors) only if non-null)
before returning false; use the existing d_free and p_free helpers and the same
null-check style used elsewhere to avoid leaks (references: vReal, vImag,
pinkFactors, d_calloc, p_calloc, d_free, p_free).
Comment on lines
354
to
358
| byte *RotaryEncoderUIUsermod::re_initIndexArray(int numModes) { | ||
| byte *indexes = (byte *)malloc(sizeof(byte) * numModes); | ||
| byte *indexes = (byte *)d_calloc(numModes, sizeof(byte)); | ||
| for (byte i = 0; i < numModes; i++) { | ||
| indexes[i] = i; | ||
| } |
There was a problem hiding this comment.
Guard d_calloc failure before populating the index array.
If d_calloc returns nullptr, the loop dereferences it and crashes. Add an early return on allocation failure.
🛠️ Proposed fix
- byte *indexes = (byte *)d_calloc(numModes, sizeof(byte));
- for (byte i = 0; i < numModes; i++) {
+ byte *indexes = (byte *)d_calloc(numModes, sizeof(byte));
+ if (!indexes) return nullptr;
+ for (byte i = 0; i < numModes; i++) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| byte *RotaryEncoderUIUsermod::re_initIndexArray(int numModes) { | |
| byte *indexes = (byte *)malloc(sizeof(byte) * numModes); | |
| byte *indexes = (byte *)d_calloc(numModes, sizeof(byte)); | |
| for (byte i = 0; i < numModes; i++) { | |
| indexes[i] = i; | |
| } | |
| byte *RotaryEncoderUIUsermod::re_initIndexArray(int numModes) { | |
| byte *indexes = (byte *)d_calloc(numModes, sizeof(byte)); | |
| if (!indexes) return nullptr; | |
| for (byte i = 0; i < numModes; i++) { | |
| indexes[i] = i; | |
| } |
🤖 Prompt for AI Agents
In `@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`
around lines 354 - 358, The function RotaryEncoderUIUsermod::re_initIndexArray
calls d_calloc and immediately dereferences the returned pointer; add a null
check after the allocation (check if indexes == nullptr) and if allocation
failed return nullptr (or handle the error) before entering the for loop that
writes to indexes, so the loop doesn't dereference a null pointer.
| #define BFRALLOC_PREFER_DRAM (1 << 1) // prefer DRAM over PSRAM | ||
| #define BFRALLOC_ENFORCE_DRAM (1 << 2) // use DRAM only, no PSRAM | ||
| #define BFRALLOC_PREFER_PSRAM (1 << 3) // prefer PSRAM over DRAM | ||
| #define BFRALLOC_ENFORCE_PSRAM (1 << 4) // use PSRAM if available, otherwise uses DRAM |
There was a problem hiding this comment.
Align BFRALLOC_ENFORCE_PSRAM comment with behavior
In util.cpp this flag allocates PSRAM-only and returns nullptr if PSRAM is unavailable; the comment says it falls back to DRAM. Please update the comment (or the behavior) to avoid misuse.
🔧 Suggested comment fix
-#define BFRALLOC_ENFORCE_PSRAM (1 << 4) // use PSRAM if available, otherwise uses DRAM
+#define BFRALLOC_ENFORCE_PSRAM (1 << 4) // use PSRAM only; returns nullptr if unavailable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #define BFRALLOC_ENFORCE_PSRAM (1 << 4) // use PSRAM if available, otherwise uses DRAM | |
| `#define` BFRALLOC_ENFORCE_PSRAM (1 << 4) // use PSRAM only; returns nullptr if unavailable |
🤖 Prompt for AI Agents
In `@wled00/fcn_declare.h` at line 518, The comment for the macro
BFRALLOC_ENFORCE_PSRAM is inaccurate: the allocation logic in util.cpp treats
this flag as "PSRAM-only" and returns nullptr if PSRAM is not available rather
than falling back to DRAM. Update the macro comment for BFRALLOC_ENFORCE_PSRAM
to state that it enforces PSRAM-only allocation and will fail (return nullptr)
when PSRAM is unavailable, or alternatively change the allocation logic in
util.cpp to implement a DRAM fallback; reference the macro name
BFRALLOC_ENFORCE_PSRAM and the allocation routine in util.cpp that checks this
flag when making your change.
add fallback (d_malloc()) if psram not found (can happen before setup(), or when a BOARD_HAS_PSRAM build runs on boards without PSRAM)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
* wrong logic for RTC_RAM_THRESHOLD * match p_malloc with p_free
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
* bugfix for p_malloc compilation guard * enable validateFreeHeap * comments
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
only measures heap size when !strip.isUpdating(); otherwise returns last measured value. -> A bit inexact, but still better than regular flickering.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
isOkForDRAMHeap() rejects allocations when the remaining heap might go below MIN_FREE_HEAP. -> prevents fragmentation when a block is first allocated, but then undone in validateFreeHeap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
introduces new memory allocation functions, based on wled#4895 by @DedeHai
Summary by CodeRabbit
Refactor
Stability
Diagnostics