introduce psram-aware malloc functions (backport of upstream #4895)#342
introduce psram-aware malloc functions (backport of upstream #4895)#342softhack007 wants to merge 14 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.”
| 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.
| 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.
| 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).
| 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.
| 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.
| #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)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wled00/util.cpp`:
- Line 797: Fix the minor typo in the comments that read "similar to d_malloc
bus uses" and "similar to p_malloc bus uses" by changing "bus" to "but" in both
occurrences; search for the exact comment strings near the d_malloc/p_malloc
helper implementations (around the comments at the spots corresponding to those
lines) and update them to "similar to d_malloc but uses" and "similar to
p_malloc but uses" respectively so the wording is correct.
- Around line 796-806: The RTC RAM threshold logic in d_calloc incorrectly
compares the per-element size variable to RTC_RAM_THRESHOLD and the contiguous
heap check uses size instead of the total allocation size; update d_calloc to
compute total = count * size (safely checking for overflow) and use total for
the RTC_RAM_THRESHOLD comparison and the getContiguousFreeHeap() comparison
(e.g., compare getContiguousFreeHeap() < 2*MIN_HEAP_SIZE + total), keeping the
existing heap_caps_calloc_prefer and heap_caps_calloc calls but passing count
and size as before; ensure you detect and handle multiplication overflow (return
nullptr or appropriate error) before using total in these checks.
🧹 Nitpick comments (2)
wled00/util.cpp (2)
755-765: ESP32validateFreeHeapis a no-op — min-heap guard disabled.The
#if 0meansd_malloc/d_callocon ESP32 will never refuse an allocation based on remaining DRAM, unlike the ESP8266 path. The TODO acknowledges this, but it effectively removes the safety net that prevents the web server from running out of heap. Worth tracking as a follow-up.Would you like me to open an issue to track re-enabling the ESP32 min-heap validation?
865-909: Disabledallocate_buffer— no review concerns while#if 0.The guarded code won't compile, so no issues for now. When enabled, note that the
validateFreeHeapcall on line 902 is outside the#else/ PSRAM blocks and would apply even to PSRAM-only allocations — verify that's intended when activating this function.
* wrong logic for RTC_RAM_THRESHOLD * match p_malloc with p_free
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wled00/util.cpp`:
- Around line 515-519: In simulateSound(), guard the call to d_malloc for
fftResult: after fftResult = (uint8_t *)d_malloc(...) check for nullptr and
handle it (either return early from simulateSound or allocate/point to a safe
fallback buffer) so fftResult is never dereferenced when allocation fails;
ensure you also avoid initializing or using um_data when fftResult is null and
free/clean up any partially initialized state if you choose an early return.
In `@wled00/wled.cpp`:
- Around line 369-379: The code uses an undefined identifier dram_free when
computing dram32_free; fix by first obtaining the 8-bit DRAM free size into a
variable (e.g., call heap_caps_get_free_size with MALLOC_CAP_8BIT |
MALLOC_CAP_INTERNAL) and subtract that from the 32-bit DRAM total, or else
remove the subtraction and use the correct existing variable; update the
expression that computes dram32_free (the line using heap_caps_get_free_size,
MALLOC_CAP_32BIT, MALLOC_CAP_INTERNAL and dram_free) so it references the
properly declared variable name (or the direct heap_caps_get_free_size call
result) to eliminate the undefined identifier.
| if (!um_data) { | ||
| //claim storage for arrays | ||
| fftResult = (uint8_t *)malloc(sizeof(uint8_t) * 16); | ||
| fftResult = (uint8_t *)d_malloc(sizeof(uint8_t) * 16); | ||
|
|
||
| // initialize um_data pointer structure |
There was a problem hiding this comment.
Guard against d_malloc() returning nullptr in simulateSound().
d_malloc() can return nullptr when MIN_HEAP_SIZE is enforced; fftResult is then dereferenced unconditionally. Add a safe fallback or early return to avoid a crash.
🔧 Suggested fix
- fftResult = (uint8_t *)d_malloc(sizeof(uint8_t) * 16);
+ fftResult = (uint8_t *)d_malloc(sizeof(uint8_t) * 16);
+ if (!fftResult) {
+ static uint8_t fftResultFallback[16] = {0};
+ fftResult = fftResultFallback;
+ }🤖 Prompt for AI Agents
In `@wled00/util.cpp` around lines 515 - 519, In simulateSound(), guard the call
to d_malloc for fftResult: after fftResult = (uint8_t *)d_malloc(...) check for
nullptr and handle it (either return early from simulateSound or allocate/point
to a safe fallback buffer) so fftResult is never dereferenced when allocation
fails; ensure you also avoid initializing or using um_data when fftResult is
null and free/clean up any partially initialized state if you choose an early
return.
| #if defined(CONFIG_IDF_TARGET_ESP32) | ||
| // 32-bit DRAM (not byte accessible, only available on ESP32) | ||
| size_t dram32_free = heap_caps_get_free_size(MALLOC_CAP_32BIT | MALLOC_CAP_INTERNAL) - dram_free; // returns all 32bit DRAM, subtract 8bit DRAM | ||
| //size_t dram32_largest = heap_caps_get_largest_free_block(MALLOC_CAP_32BIT | MALLOC_CAP_INTERNAL); // returns largest DRAM block -> not useful | ||
| DEBUG_PRINTF_P(PSTR("DRAM 32-bit: Free: %7u bytes | Largest block: N/A\n"), dram32_free); | ||
| #else | ||
| // Fast RTC Memory (not available on ESP32) | ||
| size_t rtcram_free = heap_caps_get_free_size(MALLOC_CAP_RTCRAM); | ||
| size_t rtcram_largest = heap_caps_get_largest_free_block(MALLOC_CAP_RTCRAM); | ||
| DEBUG_PRINTF_P(PSTR("RTC RAM: Free: %7u bytes | Largest block: %7u bytes\n"), rtcram_free, rtcram_largest); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "dram_free" -B 5 -A 5 wled00/wled.cppRepository: MoonModules/WLED-MM
Length of output: 900
🏁 Script executed:
rg -n "dram_free\s*=" wled00/wled.cpp | head -20Repository: MoonModules/WLED-MM
Length of output: 45
🏁 Script executed:
sed -n '300,380p' wled00/wled.cpp | cat -nRepository: MoonModules/WLED-MM
Length of output: 4492
🏁 Script executed:
sed -n '1,370p' wled00/wled.cpp | grep -n "dram_free" -B 5 -A 2Repository: MoonModules/WLED-MM
Length of output: 45
🏁 Script executed:
rg -n "dram_free|dram.*free" wled00/wled.cppRepository: MoonModules/WLED-MM
Length of output: 471
🏁 Script executed:
rg -n "dram_free" wled00/ --include="*.h" --include="*.cpp"Repository: MoonModules/WLED-MM
Length of output: 504
🏁 Script executed:
fd -e h -e cpp . wled00/ | xargs rg "dram_free"Repository: MoonModules/WLED-MM
Length of output: 228
🏁 Script executed:
sed -n '340,375p' wled00/wled.cppRepository: MoonModules/WLED-MM
Length of output: 2046
🏁 Script executed:
sed -n '300,400p' wled00/wled.cpp | cat -nRepository: MoonModules/WLED-MM
Length of output: 5590
🏁 Script executed:
sed -n '350,385p' wled00/wled.cpp | cat -nRepository: MoonModules/WLED-MM
Length of output: 2556
🏁 Script executed:
sed -n '1,360p' wled00/wled.cpp | grep -n "dram"Repository: MoonModules/WLED-MM
Length of output: 45
🏁 Script executed:
rg -n "size_t.*free" wled00/wled.cppRepository: MoonModules/WLED-MM
Length of output: 532
🏁 Script executed:
sed -n '365,385p' wled00/wled.cppRepository: MoonModules/WLED-MM
Length of output: 1700
dram_free is undefined and will cause a compilation error.
Line 371 references dram_free in the calculation but this variable is never declared anywhere in the file. This will fail to compile with an "undefined identifier" error. Either declare dram_free before line 371 (likely by calculating it from a prior heap_caps_get_free_size call for 8-bit DRAM) or replace it with the correct variable name.
🤖 Prompt for AI Agents
In `@wled00/wled.cpp` around lines 369 - 379, The code uses an undefined
identifier dram_free when computing dram32_free; fix by first obtaining the
8-bit DRAM free size into a variable (e.g., call heap_caps_get_free_size with
MALLOC_CAP_8BIT | MALLOC_CAP_INTERNAL) and subtract that from the 32-bit DRAM
total, or else remove the subtraction and use the correct existing variable;
update the expression that computes dram32_free (the line using
heap_caps_get_free_size, MALLOC_CAP_32BIT, MALLOC_CAP_INTERNAL and dram_free) so
it references the properly declared variable name (or the direct
heap_caps_get_free_size call result) to eliminate the undefined identifier.
* bugfix for p_malloc compilation guard * enable validateFreeHeap * comments
There was a problem hiding this comment.
🧹 Nitpick comments (3)
wled00/util.cpp (3)
796-806:size * countcan silently overflow on 32-bit targets.Line 800 computes
size * countwithout an overflow guard. On ESP32 (32-bitsize_t), large inputs could wrap to a small value, passing the threshold check and requesting an undersized allocation. Consider adding a simple overflow check.🛡️ Proposed overflow guard
void *d_calloc(size_t count, size_t size) { // similar to d_malloc but uses heap_caps_calloc void *buffer = nullptr; + size_t total = count * size; + if (count != 0 && total / count != size) return nullptr; // overflow `#if` !defined(CONFIG_IDF_TARGET_ESP32) - if ((size * count) <= RTC_RAM_THRESHOLD || getContiguousFreeHeap() < 2*MIN_HEAP_SIZE + (size * count)) + if (total <= RTC_RAM_THRESHOLD || getContiguousFreeHeap() < 2*MIN_HEAP_SIZE + total) buffer = heap_caps_calloc_prefer(count, size, 2, MALLOC_CAP_RTCRAM, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
755-765:#if 1 // WLEDMM TODO need to think about this— resolve or document the TODO.This conditional guard is permanently enabled and the TODO suggests uncertainty about the approach. If the logic is validated, remove the
#if 1wrapper and the TODO comment; otherwise, document the known risk (e.g., TOCTOU between allocation and the free-heap check).
835-855:p_mallocandp_callocenforce MIN_HEAP_SIZE viavalidateFreeHeap, butp_realloc_mallocdoes not.
p_realloc_malloc(line 849) skips thevalidateFreeHeapcall after a successful realloc, unlike thed_realloc_mallocpath and thep_malloc/p_callocsiblings. If the realloc returns a DRAM buffer (fallback), it could push DRAM belowMIN_HEAP_SIZEunchecked.That said, adding
validateFreeHeaphere would reintroduce the same double-free risk discussed above. If you adopt the free-then-malloc approach ford_realloc_malloc, consider doing the same here for consistency:♻️ Proposed consistency fix
void *p_realloc_malloc(void *ptr, size_t size) { - if (!psramFound()) return d_realloc_malloc(ptr, size); - void *buffer = heap_caps_realloc_prefer(ptr, size, 3, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT, MALLOC_CAP_DEFAULT); - if (buffer) return buffer; // realloc successful - p_free(ptr); // free old buffer if realloc failed - return p_malloc(size); // fallback to malloc + p_free(ptr); + return p_malloc(size); }
2 participants
introduces new memory allocation functions, based on wled#4895 by @DedeHai
Summary by CodeRabbit
Refactor
Stability
Diagnostics