introduce psram-aware malloc functions (backport of upstream #4895)#342
Merged
softhack007 merged 52 commits intomdevfrom Feb 18, 2026
Merged
introduce psram-aware malloc functions (backport of upstream #4895)#342softhack007 merged 52 commits intomdevfrom
softhack007 merged 52 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:
📝 WalkthroughWalkthroughImplements DRAM/PSRAM-aware allocator wrappers (d_/p_), consolidates many utility declarations into Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Module (FX / JSON / Preset / Usermod)
participant AllocAPI as Alloc API (d_/p_)
participant HeapChk as Heap Validator
participant MemPool as Memory Pools (DRAM / PSRAM / RTC)
Module->>AllocAPI: request allocate(size, preference)
AllocAPI->>HeapChk: measure free heap & contiguous, check MIN_HEAP_SIZE / PSRAM_THRESHOLD
alt PSRAM preferred & psramFound()
AllocAPI->>MemPool: p_malloc / p_calloc / p_realloc_malloc (PSRAM)
else DRAM/RTC chosen
AllocAPI->>MemPool: d_malloc / d_calloc / d_realloc_malloc (DRAM/RTC)
end
MemPool-->>AllocAPI: pointer or NULL
AllocAPI-->>Module: return pointer or NULL (handle OOM / fallback)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Alloc errors are only reported for d_malloc() and d_calloc(). PSRAM alloc is assumed to always succeed as PSRAM is much bigger than DRAM.
size_t is `unsigned` not ùnsigned long` => %u
removed unnecessary code, aded a comment hinting at possible OOM errors in simulateSound, minor ARTI correction.
* improved OOM stability * (future support) adjust effect-related "for" loops to use uint16_t
plus: fixed a "i can't count to 3" error in busNetwork
Collaborator
Author
|
@DedeHai my first tests are looking extremely good 👍 I managed to push an S3 with psram into a corner with up to 96% "max used ram", but it still was responsive and even recovered quickly to less than 80% ram usage. No crashes or low-ram related reboots yet. |
|
yes, that was the point of these functions, I ran hundreds of tests and many variations and this one turned out to be working most reliably. IDF V5 has much better heap control, it allows to make "heap pools" with custom HEAP_CAP's so we could use that to reduce fragmentation |
reducing the size of the RTCRAM ifdef part in d_malloc and d_calloc
mode_oops is also called when a 2D effect cannot run in 1D.
Collaborator
Author
|
tests look very good, ready to merge :-) |
…to separate header
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/fcn_declare.h (1)
433-446:⚠️ Potential issue | 🟠 MajorMove assignment can leak the JSON buffer lock.
operator=usesholding_lock |= r.holding_lockand never releases an existing lock held by*this; if both guards own the mutex, the recursive lock count leaks and the buffer can stay locked indefinitely.🛠️ Suggested fix
- inline JSONBufferGuard& operator=(JSONBufferGuard&& r) { holding_lock |= r.holding_lock; r.holding_lock = false; return *this; }; + inline JSONBufferGuard& operator=(JSONBufferGuard&& r) { + if (this != &r) { + if (holding_lock) releaseJSONBufferLock(); + holding_lock = r.holding_lock; + r.holding_lock = false; + } + return *this; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/fcn_declare.h` around lines 433 - 446, The move assignment operator for JSONBufferGuard currently does "holding_lock |= r.holding_lock" which can leak an existing lock; change operator=(JSONBufferGuard&&) to first release any lock owned by *this (call releaseJSONBufferLock() if holding_lock) before taking ownership from r, then set holding_lock = r.holding_lock and clear r.holding_lock; also handle self-assignment-safety by checking for self-assignment or just performing the release/take sequence safely. Update JSONBufferGuard::operator=(JSONBufferGuard&&) accordingly to avoid recursive lock leaks.
🧹 Nitpick comments (3)
usermods/audioreactive/audio_reactive.h (2)
476-496: Centralized cleanup indestroyFFTBuffers— solid, but consider braces for maintainability.The single-line
if (ptr) free(ptr); ptr = nullptr;pattern on lines 479, 481, 482 works correctly because thenullptrassignment is an unconditional separate statement. However, this formatting is easy to misread as thenullptrassignment being inside theifbranch. Using braces or splitting into two lines would make the intent clearer and reduce risk of future misedits.Suggested formatting for clarity
- if (pinkFactors) p_free(pinkFactors); pinkFactors = nullptr; + if (pinkFactors) p_free(pinkFactors); + pinkFactors = nullptr;Same for lines 481–482 (
vImag,vReal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.h` around lines 476 - 496, The current destroyFFTBuffers function uses single-line if statements like "if (pinkFactors) p_free(pinkFactors); pinkFactors = nullptr;" which is easy to misread; update destroyFFTBuffers to use explicit braces or separate the free and null-assignment into two statements for each pointer (pinkFactors with p_free, and vImag/vReal with d_free) so the free() call is clearly conditional and the pointer reset is always executed, keeping the existing panicOOM handling and SR_DEBUG logging unchanged.
549-574: Static allocations (oldSamples,windowWeighingFactors) intentionally excluded fromdestroyFFTBuffers.These are one-time "permanent" allocations (guarded by
if (!ptr)) that persist for the lifetime of the FFT task and are reused acrossFFTcodere-entries. Since the FFT task is never deleted (only suspended/resumed), this is not a leak. However, if a future refactor ever introduces task deletion, these would leak silently.Consider adding a brief comment near these allocations noting they are intentionally not freed by
destroyFFTBuffers, to prevent someone from "fixing" them later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.h` around lines 549 - 574, Add a short clarifying comment next to the one-time heap allocations (oldSamples and windowWeighingFactors) stating they are intentionally persistent and not freed by destroyFFTBuffers (e.g., "intentionally permanent for FFT task lifetime — not freed by destroyFFTBuffers; free only if task is deleted") and mention the potential for a leak if the FFT task is ever deleted; place this comment immediately adjacent to the allocation lines for oldSamples and windowWeighingFactors so future maintainers see it when editing FFTcode, destroyFFTBuffers, or related cleanup.usermods/artifx/arti.h (1)
2638-2656: Commented-outfree(programText)is stale — should referenced_freeon ARDUINO path.Lines 2639, 2649, and 2655 each have
//if (nullptr != programText) free(programText);. SinceprogramTextis now allocated viad_malloconARTI_ARDUINO, any future attempt to uncomment these to fix the leak must used_free, otherwise the comment will introduce a mismatched deallocation.♻️ Suggested update for all three occurrences
- //if (nullptr != programText) free(programText); // softhack007 needed to prevent memory leak? lexer has a pointer to programText so its still in use maybe? + //if (nullptr != programText) { /* lexer still holds programText pointer; delete lexer first */ + // `#if` ARTI_PLATFORM == ARTI_ARDUINO + // d_free(programText); + // `#else` + // free(programText); + // `#endif` + // programText = nullptr; + //}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/artifx/arti.h` around lines 2638 - 2656, The commented-out free(programText) calls are stale because programText is allocated with d_malloc on the ARDUINO build; update the three occurrences around the parse/ResultFail checks (references: programText, free(programText), parse(...), ResultFail, lexer->text) to perform a conditional deallocation that uses d_free(programText) when ARTI_ARDUINO is defined and free(programText) otherwise (or wrap with a helper like arti_free_programText that picks the right deallocator), and ensure you check for nullptr before freeing and set programText to nullptr after freeing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/artifx/arti.h`:
- Around line 2619-2629: The guard checking "if (stages < 1)" is dead and
doesn't free programText for the stages==1 path; update the logic so the
stages==1 early-exit cleans up both programText and lexer: either change the
condition to "if (stages < 2)" and keep the existing free/delete of programText,
or explicitly delete the Lexer instance (lexer) before calling close() and then
free programText; also ensure close() either deletes lexer or you delete lexer
prior to calling close() so no leak remains (references: programText, lexer,
close(), stages).
In `@wled00/wled.cpp`:
- Around line 372-376: The computed dram32_free (using heap_caps_get_free_size
and dram_free) can underflow on dual-core ESP32 due to non-atomic calls; update
the calculation around the dram32_free variable in wled.cpp so after obtaining
the 32-bit free size with heap_caps_get_free_size you compare it to dram_free
and clamp it to zero if it's less than dram_free, then pass that clamped value
to DEBUG_PRINTF_P (references: dram32_free, dram_free, heap_caps_get_free_size,
DEBUG_PRINTF_P).
In `@wled00/wled.h`:
- Around line 210-218: PSRAM_Allocator::reallocate currently calls
p_realloc_malloc (which can free the original buffer on failure) and therefore
violates ArduinoJson's allocator contract; replace it with a preserving-realloc
implementation that preserves the original pointer on failure: allocate a new
block with p_malloc(new_size), if allocation succeeds memcpy the
min(old_size,new_size) bytes from the old pointer, p_free the old pointer and
return the new pointer, and if allocation fails return nullptr leaving the
original buffer untouched; reference PSRAM_Allocator::reallocate,
PSRAM_Allocator::allocate, PSRAM_Allocator::deallocate, and p_realloc_malloc
when locating the change and use p_malloc/p_free/memcpy to implement the safe
behavior (ensure you can obtain the old allocation size as needed or copy only
up to new_size if size tracking exists).
---
Outside diff comments:
In `@wled00/fcn_declare.h`:
- Around line 433-446: The move assignment operator for JSONBufferGuard
currently does "holding_lock |= r.holding_lock" which can leak an existing lock;
change operator=(JSONBufferGuard&&) to first release any lock owned by *this
(call releaseJSONBufferLock() if holding_lock) before taking ownership from r,
then set holding_lock = r.holding_lock and clear r.holding_lock; also handle
self-assignment-safety by checking for self-assignment or just performing the
release/take sequence safely. Update
JSONBufferGuard::operator=(JSONBufferGuard&&) accordingly to avoid recursive
lock leaks.
---
Duplicate comments:
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h`:
- Around line 347-353: The code calls re_initIndexArray(strip.getPaletteCount())
to set palettes_alpha_indexes but doesn't check for nullptr before passing it to
re_sortModes; reproduce the null-check pattern used for modes_alpha_indexes and
palettes_qstrings: after palettes_alpha_indexes = re_initIndexArray(...), verify
palettes_alpha_indexes is non-null (and return/skip the sorting if it is null)
before computing skipPaletteCount and calling re_sortModes(palettes_qstrings,
palettes_alpha_indexes, strip.getPaletteCount(), skipPaletteCount); this
prevents qsort from being invoked with a nullptr.
In `@wled00/util.cpp`:
- Around line 517-520: The allocation for fftResult using d_malloc in the block
where um_data is null may return nullptr and later code writes to fftResult; add
a null-check immediately after the d_malloc call (the allocation for fftResult)
and handle failure by cleaning up any partially initialized state and
returning/aborting from the current function (or setting a safe flag) so
subsequent writes to fftResult are avoided; reference fftResult, d_malloc and
um_data to locate the allocation and ensure all later uses of fftResult are
guarded behind a non-null check or early exit.
- Around line 877-916: The realloc wrappers (d_realloc_malloc and
p_realloc_malloc) should avoid calling heap_caps_realloc_prefer/realloc (which
can transiently require double memory) and instead follow the project's
free-then-alloc pattern: drop the use of
heap_caps_realloc_prefer/heap_caps_realloc, call d_free(ptr) (or p_free(ptr) for
p_realloc_malloc) first, then allocate a new buffer with d_malloc(size) (or
p_malloc(size)) and return validateFreeHeap on the result; keep the psramFound()
short-circuit in p_realloc_malloc to route to d_realloc_malloc when PSRAM isn't
present and preserve use of validateFreeHeap for the returned buffer.
---
Nitpick comments:
In `@usermods/artifx/arti.h`:
- Around line 2638-2656: The commented-out free(programText) calls are stale
because programText is allocated with d_malloc on the ARDUINO build; update the
three occurrences around the parse/ResultFail checks (references: programText,
free(programText), parse(...), ResultFail, lexer->text) to perform a conditional
deallocation that uses d_free(programText) when ARTI_ARDUINO is defined and
free(programText) otherwise (or wrap with a helper like arti_free_programText
that picks the right deallocator), and ensure you check for nullptr before
freeing and set programText to nullptr after freeing.
In `@usermods/audioreactive/audio_reactive.h`:
- Around line 476-496: The current destroyFFTBuffers function uses single-line
if statements like "if (pinkFactors) p_free(pinkFactors); pinkFactors =
nullptr;" which is easy to misread; update destroyFFTBuffers to use explicit
braces or separate the free and null-assignment into two statements for each
pointer (pinkFactors with p_free, and vImag/vReal with d_free) so the free()
call is clearly conditional and the pointer reset is always executed, keeping
the existing panicOOM handling and SR_DEBUG logging unchanged.
- Around line 549-574: Add a short clarifying comment next to the one-time heap
allocations (oldSamples and windowWeighingFactors) stating they are
intentionally persistent and not freed by destroyFFTBuffers (e.g.,
"intentionally permanent for FFT task lifetime — not freed by destroyFFTBuffers;
free only if task is deleted") and mention the potential for a leak if the FFT
task is ever deleted; place this comment immediately adjacent to the allocation
lines for oldSamples and windowWeighingFactors so future maintainers see it when
editing FFTcode, destroyFFTBuffers, or related cleanup.
Comment on lines
2619
to
2629
| if (stages < 1) { | ||
| if (nullptr != programText) free(programText); // softhack007 prevent memory leak | ||
| // softhack007 prevent memory leak | ||
| #if ARTI_PLATFORM == ARTI_ARDUINO | ||
| if (nullptr != programText) d_free(programText); | ||
| #else | ||
| if (nullptr != programText) free(programText); | ||
| #endif | ||
| programText = nullptr; | ||
| close(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
stages < 1 guard is dead code — the intended stages==1 leak remains.
The first stages < 1 check at line 2484 already returns before programText is ever allocated. By the time execution reaches line 2619, stages >= 1 is guaranteed, so programText is never freed here. The if (nullptr != programText) guard prevents a crash but the condition is always false.
The genuine concern is stages == 1: programText is allocated, the Lexer is created (line 2635), and the stages < 2 branch at line 2638 calls close() and returns — leaking both programText and lexer (which close() does not delete). Fixing that path requires deleting lexer inside close() before freeing programText, which is a separate refactor. The current guard is a no-op.
💡 Clarification — the dead guard vs. the real leak
stages value |
programText allocated? |
Freed by this block? |
|---|---|---|
| 0 | No (line 2484 returns first) | Never reached |
| 1 | Yes | No (stages < 1 is false) — leaked at stages < 2 path |
| 5 (production) | Yes | Freed correctly at lines 2685–2688 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/artifx/arti.h` around lines 2619 - 2629, The guard checking "if
(stages < 1)" is dead and doesn't free programText for the stages==1 path;
update the logic so the stages==1 early-exit cleans up both programText and
lexer: either change the condition to "if (stages < 2)" and keep the existing
free/delete of programText, or explicitly delete the Lexer instance (lexer)
before calling close() and then free programText; also ensure close() either
deletes lexer or you delete lexer prior to calling close() so no leak remains
(references: programText, lexer, close(), stages).
p_realloc_malloc() violated the "realloc" specs, and may lead to broken JSON
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
If heap_caps_realloc_prefer succeeds, the old ptr is invalidated by realloc (per its contract). If validateFreeHeap then decides DRAM is too low and frees the new buffer, d_free(ptr) frees an already-invalid pointer — heap corruption / crash.
softhack007
commented
Feb 17, 2026
* SoundSim back to regular malloc(), the function cannot handle d_malloc failures * removed two unnecessary "else" * DEBUG_PRINTF added in d_realloc_malloc
* use heap_caps_get_allocated_size() to get the current size * prefer PSRAM if remaining DRAM heap is low
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
* revert change in AsyncJSON - causes instabilities * comment out "early acceptance" in isOkForDRAMHeap()
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
What is does
Summary by CodeRabbit
Refactor
Stability
Diagnostics