Improvements to heap-memory and PSRAM handling #4791
Conversation
- 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 `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
WalkthroughCentralized DRAM/PSRAM-aware allocation added (new allocate_buffer API, allocator wrappers and flags), heap-query helpers introduced, many allocations switched to PSRAM-preferred paths and p_free, runtime psram checks replaced with BOARD_HAS_PSRAM compile-time gating, expanded memory diagnostics, and a calloc argument bugfix in the audio FFT. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
usermods/audioreactive/audio_reactive.cpp(1 hunks)wled00/FX.h(2 hunks)wled00/FX_fcn.cpp(8 hunks)wled00/cfg.cpp(1 hunks)wled00/const.h(1 hunks)wled00/fcn_declare.h(2 hunks)wled00/json.cpp(2 hunks)wled00/util.cpp(3 hunks)wled00/wled.cpp(7 hunks)wled00/wled_server.cpp(1 hunks)wled00/ws.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
wled00/cfg.cpp (1)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
wled00/ws.cpp (3)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
wled00/json.cpp (6)
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: KrX3D
PR: #4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: blazoncek
PR: #4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
usermods/audioreactive/audio_reactive.cpp (1)
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
wled00/wled.cpp (3)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
wled00/fcn_declare.h (1)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
wled00/const.h (1)
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
wled00/FX.h (14)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: willmmiles
PR: #4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: blazoncek
PR: #4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the getSegment() function guards against out-of-bounds segment IDs, and getFirstSelectedSegId() falls back to getMainSegmentId() if no segments are selected, ensuring no crashes when used through the setValuesFromFirstSelectedSeg() macro.
Learnt from: blazoncek
PR: #4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the getSegment() function guards against out-of-bounds segment IDs, and getFirstSelectedSegId() falls back to getMainSegmentId() if no segments are selected, ensuring no crashes when used through the setValuesFromFirstSelectedSeg() macro.
Learnt from: blazoncek
PR: #4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the getSegment() function guards against out-of-bounds segment IDs by falling back to the main segment ID, and getFirstSelectedSegId() falls back to getMainSegmentId() if no segments are selected, ensuring no crashes when used through the setValuesFromFirstSelectedSeg() macro.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED's Segment class (FX_fcn.cpp) handles all necessary validation internally, including:
- Null pointer checks
- Length validation against WLED_MAX_SEGNAME_LEN
- Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
Learnt from: DedeHai
PR: #4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
PR: #4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.296Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dxdx + dydy to be a practical concern in particle system effects.
wled00/util.cpp (4)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
wled00/FX_fcn.cpp (13)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: willmmiles
PR: #4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED's Segment class (FX_fcn.cpp) handles all necessary validation internally, including:
- Null pointer checks
- Length validation against WLED_MAX_SEGNAME_LEN
- Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
Learnt from: blazoncek
PR: #4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the getSegment() function guards against out-of-bounds segment IDs, and getFirstSelectedSegId() falls back to getMainSegmentId() if no segments are selected, ensuring no crashes when used through the setValuesFromFirstSelectedSeg() macro.
Learnt from: blazoncek
PR: #4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the getSegment() function guards against out-of-bounds segment IDs, and getFirstSelectedSegId() falls back to getMainSegmentId() if no segments are selected, ensuring no crashes when used through the setValuesFromFirstSelectedSeg() macro.
Learnt from: blazoncek
PR: #4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the getSegment() function guards against out-of-bounds segment IDs by falling back to the main segment ID, and getFirstSelectedSegId() falls back to getMainSegmentId() if no segments are selected, ensuring no crashes when used through the setValuesFromFirstSelectedSeg() macro.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, bri is a global variable used for brightness control.
Learnt from: DedeHai
PR: #4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.296Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dxdx + dydy to be a practical concern in particle system effects.
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
🧬 Code Graph Analysis (5)
wled00/cfg.cpp (1)
wled00/fcn_declare.h (2)
getFreeHeapSize(586-586)getFreeHeapSize(589-589)
wled00/ws.cpp (1)
wled00/fcn_declare.h (2)
getFreeHeapSize(586-586)getFreeHeapSize(589-589)
wled00/wled_server.cpp (1)
wled00/fcn_declare.h (2)
getFreeHeapSize(586-586)getFreeHeapSize(589-589)
wled00/fcn_declare.h (2)
wled00/wled.h (1)
size(170-178)wled00/util.cpp (2)
allocate_buffer(719-776)allocate_buffer(719-719)
wled00/FX.h (1)
wled00/util.cpp (2)
allocate_buffer(719-776)allocate_buffer(719-719)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (36)
wled00/cfg.cpp (1)
204-204: LGTM! Good platform abstraction improvement.The replacement of
ESP.getFreeHeap()withgetFreeHeapSize()provides better platform consistency and aligns with the broader memory management improvements in this PR.wled00/ws.cpp (2)
127-128: LGTM! Consistent platform abstraction for heap queries.The replacement of
ESP.getFreeHeap()withgetFreeHeapSize()maintains the same memory management logic while improving platform compatibility. The heap size measurement and debug logging functionality remains unchanged.
137-138: LGTM! Proper heap monitoring after buffer allocation.The changes maintain the critical memory validation logic that ensures WebSocket buffer allocation succeeded on ESP8266. Using
getFreeHeapSize()instead ofESP.getFreeHeap()improves platform abstraction without affecting the memory management strategy.usermods/audioreactive/audio_reactive.cpp (1)
226-229: No other reversed calloc() argument instances foundI’ve verified that all calloc() calls use the correct
calloc(count, size)order, including the FFT buffers in usermods/audioreactive/audio_reactive.cpp. No additional misordered calls were detected across the codebase.wled00/wled_server.cpp (1)
369-369: LGTM - Clean abstraction of heap size queryThe replacement of
ESP.getFreeHeap()withgetFreeHeapSize()properly abstracts the platform-specific heap query and aligns with the broader memory management refactoring across the codebase.wled00/json.cpp (3)
815-815: LGTM - Standardizes ESP32 max allocation heap queryThe replacement of
ESP.getMaxAllocHeap()withgetContiguousFreeHeap()provides a consistent cross-platform interface for querying the largest contiguous heap block available for allocation.
826-826: LGTM - Standardizes ESP8266 max free block queryThe replacement of
ESP.getMaxFreeBlockSize()withgetContiguousFreeHeap()unifies the API for querying maximum contiguous heap across ESP32 and ESP8266 platforms.
832-832: LGTM - Consistent with heap query abstractionThe replacement of
ESP.getFreeHeap()withgetFreeHeapSize()maintains the same functionality while using the new standardized heap query interface.wled00/wled.cpp (9)
171-171: LGTM - Consistent heap query in connection handlingThe replacement of
ESP.getFreeHeap()withgetFreeHeapSize()maintains functionality while using the new standardized heap query interface.
243-247: Enhanced debug output provides valuable memory diagnosticsThe improved debug output now shows both total free heap and contiguous free heap, plus ESP32-specific 32-bit heap information. This provides much better visibility into memory fragmentation and allocation patterns, which is essential for the PSRAM handling improvements.
373-373: LGTM - Standardized heap reporting in setupThe replacement maintains the same debug functionality while using the new heap query abstraction.
401-401: LGTM - Consistent with memory management refactoring
431-431: LGTM - Heap monitoring after config deserialization
443-443: LGTM - Heap monitoring after strip initialization
448-448: LGTM - Heap monitoring after usermod setup
513-513: LGTM - Heap monitoring after server initialization
519-519: LGTM - Heap monitoring after IR initializationwled00/const.h (2)
549-554: LGTM! Platform-appropriate minimum heap thresholds.The defined values align well with the existing
WLED_REQUEST_MIN_HEAPconstants and provide appropriate buffers for UI responsiveness while accounting for ESP8266's more limited RAM.
556-563: Well-designed platform-specific PSRAM thresholds.The threshold values are appropriately tailored to each platform's capabilities:
- ESP32S3 (5120): Highest threshold for best PSRAM support
- ESP32 (4096): Good balance for standard ESP32
- Others (1024): Appropriate fallback, correctly noting C3/ESP8266 lack PSRAM
The allocation strategy comments clearly explain the heap-based decision logic.
wled00/FX.h (3)
91-91: Accurate comment update reflecting new memory strategy.The updated comment correctly describes the heap memory limit approach and PSRAM reservation strategy for pixel buffers.
95-101: Appropriate increases to segment data limits.The increased
MAX_SEGMENT_DATAvalues properly scale with platform capabilities:
- ESP8266: 10KB (conservative for limited RAM)
- ESP32S2: 20KB (balanced for S2's constraints)
- Other ESP32: 60KB (leveraging full capabilities)
These increases align with the PR objective of supporting larger pixel setups while maintaining platform-appropriate limits.
602-603: Excellent transition to new buffer allocation system.The change from
d_calloctoallocate_bufferis well-implemented:
BFRALLOC_PREFER_PSRAM: Aligns with PR goal of PSRAM utilizationBFRALLOC_NOBYTEACCESS: Appropriate for 32-bit pixel dataBFRALLOC_CLEAR: Maintains zero-initialization behavior- Informative comment about minimal FPS impact (<2%) with PSRAM usage
This properly integrates with the enhanced memory management strategy described in the PR objectives.
wled00/fcn_declare.h (2)
585-591: Well-designed heap query abstractions.The platform-specific implementations correctly use heap_caps functions for ESP32 variants to get accurate DRAM-only measurements, while falling back to ESP functions for ESP8266. The inline declarations ensure optimal performance.
592-598: Clean and extensible allocation flag design.The bit flag pattern allows flexible combination of allocation preferences. The naming is clear and consistent, making the API intuitive for users.
wled00/util.cpp (4)
648-658: Well-implemented calloc variants.The p_calloc implementation correctly mirrors the p_malloc logic while properly handling zero-initialization through heap_caps_calloc_prefer.
688-696: Conservative DRAM threshold for calloc operations.Good design choice using
3*MIN_HEAP_SIZEthreshold (vs 2x for malloc) as calloc operations typically involve larger allocations that require zeroing.
624-634: PSRAM_THRESHOLD correctly defined and allocation strategy is sound
- PSRAM_THRESHOLD is defined in wled00/const.h for each target (ESP32S3: 5120, ESP32: 4096, others: 1024).
- MIN_HEAP_SIZE is defined (8 KB or 12 KB) in const.h as expected.
- The p_malloc logic using these thresholds operates independently of Segment::allocateData() optimizations and poses no conflict.
718-776: Review allocation alignment and heap exhaustion handling.Several observations about the
allocate_bufferimplementation:
Potential alignment issue (line 728): Casting to
uint32_t*assumes 4-byte alignment, but the size parameter might not be a multiple of 4.Heap exhaustion risk: The MIN_HEAP_SIZE check occurs after allocation (line 767), which could temporarily exhaust the heap and affect system stability.
Consider these improvements:
void *allocate_buffer(size_t size, uint32_t type) { void *buffer = nullptr; + // Pre-check heap to avoid temporary exhaustion + if (getFreeHeapSize() < MIN_HEAP_SIZE + size) { + return nullptr; + } #if defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) buffer = d_malloc(size); #else #ifdef CONFIG_IDF_TARGET_ESP32 if(type & BFRALLOC_NOBYTEACCESS) { + // Ensure size is aligned to 4 bytes for 32-bit access + size_t aligned_size = (size + 3) & ~3; - buffer = static_cast<uint32_t*>(heap_caps_malloc(size, MALLOC_CAP_INTERNAL | MALLOC_CAP_32BIT)); + buffer = heap_caps_malloc(aligned_size, MALLOC_CAP_INTERNAL | MALLOC_CAP_32BIT);⛔ Skipped due to learnings
Learnt from: blazoncek PR: wled/WLED#4667 File: usermods/user_fx/user_fx.cpp:27-30 Timestamp: 2025-04-30T05:41:03.633Z Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.Learnt from: DedeHai PR: wled/WLED#4750 File: usermods/audioreactive/audio_reactive.cpp:311-321 Timestamp: 2025-06-27T06:04:44.652Z Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.wled00/FX_fcn.cpp (8)
71-80: LGTM! Good PSRAM utilization for pixel buffers.The implementation correctly uses the new
allocate_bufferfunction with appropriate flags for pixel buffer allocation, and properly handles allocation failures.
113-122: Consistent implementation with copy constructor.The copy assignment operator maintains the same allocation strategy and error handling as the copy constructor, which is good for code consistency.
216-220: Good implementation of smart buffer deallocation strategy.The code correctly implements the memory management strategy by deallocating large buffers to free memory while retaining small buffers to reduce fragmentation. The debug output is helpful for monitoring buffer management.
469-480: Smart approach to avoid heap fragmentation.Good decision to use
free()instead ofrealloc()to prevent blocking larger heap segments. The pixel buffer allocation correctly uses PSRAM preference consistent with other pixel buffer allocations.
1214-1214: Correct use of new heap abstraction function.The code properly uses the new
getFreeHeapSize()wrapper function for platform-independent heap size queries.
1226-1230: Appropriate PSRAM enforcement for main pixel buffer.The code correctly enforces PSRAM allocation for the main pixel buffer, which aligns with the PR's memory optimization goals. The buffer is properly cleared on allocation and the debug output uses the new heap abstraction.
1274-1274: Important bugfix for segment transition safety.Good addition to check if the old segment is active before running its effect. This prevents potential crashes from running effects on inactive segments with invalid parameters.
147-191: All memory-management logic looks solid—constant is defined
FAIR_DATA_PER_SEGis declared inwled00/FX.h(#define FAIR_DATA_PER_SEG (MAX_SEGMENT_DATA / WS2812FX::getMaxSegments())), so no undefined-symbol issue.LGTM.
wled00/fcn_declare.h
Outdated
| #ifdef CONFIG_IDF_TARGET_ESP32 | ||
| void *pixelbuffer_malloc(size_t size, bool enforcePSRAM = false); // prefer IRAM for pixel buffers if possible | ||
| #endif |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the implementation of pixelbuffer_malloc.
The function pixelbuffer_malloc is declared here but I need to verify its implementation exists in the codebase.
🏁 Script executed:
#!/bin/bash
# Search for the implementation of pixelbuffer_malloc
ast-grep --pattern 'void *pixelbuffer_malloc(size_t $_, $_) {
$$$
}'
# Also search with ripgrep for any definition
rg -A 5 'pixelbuffer_malloc.*\{' --type cppLength of output: 118
🏁 Script executed:
#!/usr/bin/env bash
# Search for any declaration or definition of pixelbuffer_malloc across the repository
rg -n 'pixelbuffer_malloc' .Length of output: 166
Missing implementation for pixelbuffer_malloc
The function pixelbuffer_malloc(size_t, bool) is declared under #ifdef CONFIG_IDF_TARGET_ESP32 in wled00/fcn_declare.h:555 but no matching definition exists in the repository. This will cause linker errors when building for ESP32.
• Add the corresponding function definition (for example in wled00/fcn_declare.cpp or another appropriate source file).
• Ensure the implementation handles the enforcePSRAM flag and IRAM allocation preference correctly.
🤖 Prompt for AI Agents
In wled00/fcn_declare.h around lines 554 to 556, the function pixelbuffer_malloc
is declared but lacks a corresponding implementation, causing linker errors for
ESP32 builds. Add the function definition in an appropriate source file like
wled00/fcn_declare.cpp, implementing the logic to allocate memory for pixel
buffers with preference for IRAM and correctly handling the enforcePSRAM flag to
decide whether to allocate from PSRAM or not.
wled00/FX.h
Outdated
| #elif defined(CONFIG_IDF_TARGET_ESP32S2) | ||
| #define MAX_NUM_SEGMENTS 20 | ||
| #define MAX_SEGMENT_DATA (MAX_NUM_SEGMENTS*512) // 10k by default (S2 is short on free RAM) | ||
| #define MAX_SEGMENT_DATA (MAX_NUM_SEGMENTS*1024) // 20k by default (S2 is short on free RAM) |
There was a problem hiding this comment.
This will not cut it on every build for S2. Mine is one example, having only about 20-30k of free heap after everything is loaded and operational.
There was a problem hiding this comment.
its no longer used in the same way: it now tries to keep this much ram free for FX data if there is PSRAM, otherwise it will limit the use to this amount but if heap runs low, it will respect minheap. so even if you set this to 100k nothing bad happens. one exception (which is no different in current implementation): if a usermod or something else uses heap to deplete it.
wled00/FX.h
Outdated
| #define MAX_NUM_SEGMENTS 16 | ||
| /* How much data bytes all segments combined may allocate */ | ||
| #define MAX_SEGMENT_DATA 5120 | ||
| #define MAX_SEGMENT_DATA (MAX_NUM_SEGMENTS*640) // 10k by default |
There was a problem hiding this comment.
That's a lot (and I mean a lot) for ESP8266.
There was a problem hiding this comment.
if you can give me a scenario that is critical in ram usage, I can test.
There was a problem hiding this comment.
4 usermods (PIR, temperature, 4LD, AR) and 16x16 matrix
There was a problem hiding this comment.
works fine:
Free heap: 16384, Contiguous: 11344
Main:
Free heap: 15720, Contiguous: 7272
There was a problem hiding this comment.
Did you enable MQTT and use GPIO3?
There was a problem hiding this comment.
no, I dont have the hardware to do that, you'd need to check with your setup.
edit:
since this is all about free heap : if it works in main, I see no reason it should not work with this PR
There was a problem hiding this comment.
maybe the condition to delete and reallocate of the segmentdata should not be FAIR_DATA_PER_SEG but something a bit larger, might help with fragmentation on ESP8266
|
@troyhacks - You might be interested in this |
netmindz
added
enhancement
awesome
Awaiting testing
optimization
Absolutely - I've been doing this sort of thing in all my personal WLED-MM builds for the last year and agree with the findings. 😁 PSRAM is the solution for more pixels and the speed decrease is negligible - especially compared to it not working. 🤣 @DedeHai has done it more in-depth than my liberal sprinkling of |
|
I've sifted through code and can say that these are solid changes. However ... Please use space after the keywords. I.e. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/util.cpp (1)
716-776: Please verify the SOC_DRAM_HIGH comparison and add documentation.The new allocate_buffer function implements a robust multi-region allocation strategy, but the check at line 728 may be inverted:
- It currently does
Please confirm against the actual definitions of SOC_DRAM_HIGH (and SOC_IRAM_LOW) that this condition correctly detects when the pointer lies outside the desired 32-bit DRAM region. It may need to beif ((uintptr_t)buffer < SOC_DRAM_HIGH) { // “buffer did not fit into 32-bit DRAM region…” }>=instead.Additionally, this entire allocation flow (flags, fallbacks, safety heap check) is complex—please add a doc comment above allocate_buffer explaining:
- The meaning of each BFRALLOC_* flag
- Which memory regions each path targets
- How the MIN_HEAP_SIZE safeguard works
File to update:
- wled00/util.cpp (around lines 720–740 for the macro check, and the function header around line 716 for documentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/FX_fcn.cpp(8 hunks)wled00/fcn_declare.h(2 hunks)wled00/util.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/fcn_declare.h
- wled00/FX_fcn.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
wled00/util.cpp (4)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (2)
wled00/util.cpp (2)
624-658: LGTM! Improved PSRAM allocation strategy with intelligent threshold-based decisions.The updated allocation functions now use explicit
MALLOC_CAP_INTERNALinstead ofMALLOC_CAP_DEFAULTand implement smart threshold logic that prefers DRAM for small allocations when heap is abundant, while falling back to PSRAM when needed. The newp_callocfunction maintains consistency with the existing allocation pattern.
688-696: Verify the different heap threshold in d_calloc.The
d_callocfunction uses3*MIN_HEAP_SIZEas the threshold whiled_mallocandd_reallocuse2*MIN_HEAP_SIZE. Is this intentional due to the additional overhead of zero-initialization, or should it be consistent?
|
Using my development ESP I get the following (excerpt): When UI comes to life there is about 80k heap free. |
|
heap is reported incorrectly on ESP32 unless you use this PR. |
That's this PR's code. ESP8266 |
|
Yes, pulled to my dev. |
|
ESP32 looks similar to what I saw in tests, heap should be increased by about 35k minus all ledbuffers |
|
ESP32 with PSRAM (rev.3) reports 75k/3972k heap/PSRAM. 32x32 matrix, 3 usermods, no MQTT. |
Debug will tell you what's happening. I would guess output failed to initialise. |
- Bugfix: `allocate_buffer` did double allocations because of a fallback else that should not be there: if no type is defined, should not allocate anything - Bugfix2: same place: checked for minheap instead of min cotiguous heap - updated comments to make the intent of each memory type clearer
|
Just for posterity: My tests with WROVER rev.3 ESP32 (4MB PSRAM) showed that FPS is running at 26 for 64x32 matrix using effect (Soap, Octopus) that uses effect data buffer. With buffers in DRAM FPS is at stable 42. |
|
Can you clarify: which buffers were in PSRAM? I also saw a FPS drop if effect-data is in PSRAM but only minor influence if just the pixel-buffers are in PSRAM. Since I have no ESP32 with PSRAM (Only S2, S3) this would be interesting to distinguish. |
|
Pixel buffers (effect data is always in DRAM). |
|
thanks for the clarification. This means that for ESP32 maybe the parameters need to be adjusted to keep pixelbuffers in DRAM and use PSRAM more carefully. |
|
@DedeHai I have to take my words back. It looks like my tests were flawed. EDIT: This is for rev.3 ESP32. rev.1 is still pending. |
|
@blazoncek There's also "things" that I've found during my extensive (and poorly documented) performance testing that were less intuitive. I'd have to rerun the tests (and actually write them down this time), but in general I've noticed some random combinations of compiler optimizations and other build alternations that caused bigger FPS losses than I'd expected. I remain positive on PSRAM because "works" versus "crash" is often down to just having enough of any kind of RAM. I haven't seen major impacts on speed, but also I approve of a lower speed over it crashing. Sure, 21845 pixels isn't the fastest on an original ESP32 CPU, but it works just fine when you give it enough RAM. (21845 isn't a random number. That's the actual max limit when you have 16-bit integers and RGB pixels. 😁 My ESP32-P4 build has mostly been modified to break that barrier - but I don't have 22000+ pixels at home so I can only test at the venue where we have an installation of that size in active use, ha.) |
|
|
|
So are you saying its a useless parameter and it should be a #ifdef? |
|
If you say so. 😄 |
|
Using an ifdef would save some flash. All p_alloc functions can be skipped |
- 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
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
I consider this PR ready to merge.
I tested this code on all platforms to confirm it works as intended. (ESP32, ESP32-WROVER v1.0, S3, S2, C3 and ESP8266) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Any objections to merge this to get more feedback from users? Otherwise it will just sit here waiting ;) |
|
From my perspective it looks ready for merge, I'm not marking at code review approved as there are others more experience with C than me, but from the comment thread it looks like there is approval even if that hasn't specifically been flagged in GitHub so I'm happy for you to merge |
|
Well, I do not approve changes to MAX_NUM_SEGMENTS and MAX_SEGMENT_DATA. Even with improved heap handling. |
If this turns out to be a bad choice, there is nothing preventing us from updating it. Actually, I increased MAX_NUM_SEGMENTS to account for PSRAM, now that I added the BOARD_HAS_PSRAM as a compile time check, I will change that. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/FX.h (1)
91-106: Platform limits: recheck ESP8266 headroom and PSRAM gatingThe new MAX_SEGMENT_DATA defaults look reasonable overall. On ESP8266, 6 KB may still be tight with heavier configs (MQTT + multiple usermods + 16x16). Please confirm on such setups and ensure BOARD_HAS_PSRAM is consistently defined in all PSRAM-enabled envs so the 64‑segment path isn’t accidentally selected on non‑PSRAM builds.
🧹 Nitpick comments (1)
wled00/FX.h (1)
605-606: Optional: consider skipping CLEAR for large pixel buffersZeroing can add noticeable startup latency on very large matrices. If not required by logic, consider dropping BFRALLOC_CLEAR or making it conditional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.h(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/FX.h
🧠 Learnings (9)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
📚 Learning: 2025-02-19T12:43:34.200Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Applied to files:
wled00/FX.h
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.h
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX.h
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.h
🧬 Code graph analysis (1)
wled00/FX.h (3)
wled00/util.cpp (2)
allocate_buffer(745-806)allocate_buffer(745-745)wled00/fcn_declare.h (1)
p_free(451-451)wled00/FX_fcn.cpp (2)
Segment(60-82)Segment(85-93)
🔇 Additional comments (3)
wled00/FX.h (3)
110-110: FAIR_DATA_PER_SEG switch to macro: LGTMMatches constexpr getMaxSegments behavior; no functional risk detected.
605-606: PSRAM/IRAM‑aware pixel buffer allocation: LGTMGood use of BFRALLOC_NOBYTEACCESS and prefer‑PSRAM heuristics aligned with util.cpp.
677-677: Public getUsedSegmentData(): LGTMNeeded for allocator heuristics; interface exposure is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.h (1)
873-877: Free _pixels with p_free in the destructorallocate_buffer assigns _pixels and FX_fcn.cpp uses p_free when reallocating; the destructor currently calls d_free — switch to p_free to match the allocation.
- d_free(_pixels); + p_free(_pixels);
♻️ Duplicate comments (1)
wled00/FX.h (1)
651-651: clearName() now uses p_free — thanks for aligning with allocatorThis addresses the prior review ask to unify frees with allocate_buffer.
🧹 Nitpick comments (1)
wled00/FX.h (1)
605-607: Comment doesn’t match allocator preference (IRAM first, then PSRAM)With BFRALLOC_NOBYTEACCESS, allocate_buffer prefers MALLOC_CAP_32BIT (IRAM) on classic ESP32, then PSRAM, then DRAM. Update the comment to avoid confusion.
- // allocate render buffer (always entire segment), prefer PSRAM if DRAM is running low. Note: impact on FPS with PSRAM buffer is low (<2% with QSPI PSRAM) + // allocate render buffer (always entire segment): prefer IRAM (ESP32 classic), then PSRAM (if available), else DRAM. + // Note: PSRAM buffer FPS impact is low (<2% with QSPI PSRAM).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.h(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/FX.h
🧠 Learnings (9)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/FX.h
📚 Learning: 2025-02-19T12:43:34.200Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.h
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX.h
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.h
🧬 Code graph analysis (1)
wled00/FX.h (3)
wled00/util.cpp (2)
allocate_buffer(745-806)allocate_buffer(745-745)wled00/fcn_declare.h (1)
p_free(451-451)wled00/FX_fcn.cpp (2)
Segment(60-82)Segment(85-93)
🔇 Additional comments (4)
wled00/FX.h (4)
110-110: Using MAX_NUM_SEGMENTS for FAIR_DATA_PER_SEG is correctCompile‑time constant keeps it coherent with platform presets. LGTM.
628-628: Free pixels with p_free: good allocator symmetryMatches allocate_buffer/heap_caps_* usage and avoids region mismatch.
677-677: Expose getUsedSegmentData(): needed by allocator heuristicsPublic accessor matches util.cpp heuristics. LGTM.
91-106: Confirm ESP32‑S2 headroom (FAIR_DATA_PER_SEG = 20k / 32 = 640 B) and test heavy effectsFX.h defines FAIR_DATA_PER_SEG (wled00/FX.h:110). FX.cpp uses it to cap per‑segment allocations (e.g. wled00/FX.cpp:3268–3272, 3441, 3565) and FX_fcn.cpp enforces MAX_SEGMENT_DATA / deallocates oversized buffers (wled00/FX_fcn.cpp:163–167, 214–217). util.cpp will prefer PSRAM if DRAM would exceed MAX_SEGMENT_DATA (wled00/util.cpp:770–773). Several effects still attempt allocations >640 B (popcorn/spark/matrix buffers), so run runtime tests on an ESP32‑S2 without PSRAM using heavy effects (matrix/popcorn/starburst, large cols/rows). If you observe allocation failures/fallbacks to mode_static, reduce MAX_NUM_SEGMENTS for S2 in wled00/FX.h (currently 32) to 24 or 28.
- free heap check in main loop should not be free contiguous heap check, it leads to random strip resets if heap runs low.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
wled00/wled.cpp (2)
245-265: Debug: guard against potential underflow in DRAM 32‑bit math.The subtraction can underflow if 32‑bit DRAM total < dram_free, yielding a large bogus value. Previously discussed as acceptable for dev logs, but the safer form is trivial if you want it:
- 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_total = heap_caps_get_free_size(MALLOC_CAP_32BIT | MALLOC_CAP_INTERNAL); + size_t dram32_free = dram32_total > dram_free ? (dram32_total - dram_free) : 0; // avoid underflow
396-401: JSON PSRAM alloc: add nothrow + fallback.To harden boot on marginal PSRAM, allocate with nothrow and fall back to a static JSON buffer if allocation fails.
- pDoc = new PSRAMDynamicJsonDocument(2 * JSON_BUFFER_SIZE); + pDoc = new (std::nothrow) PSRAMDynamicJsonDocument(2 * JSON_BUFFER_SIZE); + if (!pDoc) { + static StaticJsonDocument<JSON_BUFFER_SIZE> s_fallback; + pDoc = &s_fallback; + DEBUG_PRINTLN(F("PSRAM JSON alloc failed, using static fallback.")); + }wled00/util.cpp (1)
680-689: Include SOC memory layout headers for SOC_ region checks.*validateFreeHeap() uses SOC_DRAM_LOW/HIGH; include the defining headers explicitly to avoid toolchain/version fragility.
#include <Update.h> +#if defined(ARDUINO_ARCH_ESP32) +#include "soc/soc.h" +#include "soc/soc_memory_layout.h" // IDF4; on IDF5 this maps to esp_memory_layout.h +#endif
🧹 Nitpick comments (2)
wled00/util.cpp (2)
683-685: Free with the platform’s deallocator to match allocator.Use d_free() instead of free() so DRAM allocations done via heap_caps_* are released with heap_caps_free on ESP32.
- free(buffer); + d_free(buffer);Also applies to: 651-653
747-811: Second‑chance fallback when DRAM validation fails.In rare races, d_malloc may succeed but validateFreeHeap() frees it because contiguous DRAM dropped meanwhile. For PREFER_DRAM builds with PSRAM, try a PSRAM allocation before returning nullptr.
else if (type & BFRALLOC_PREFER_PSRAM) { @@ else if (type & BFRALLOC_ENFORCE_PSRAM) buffer = heap_caps_malloc(size, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT); // use PSRAM only, otherwise return nullptr buffer = validateFreeHeap(buffer); +#ifdef BOARD_HAS_PSRAM + if (!buffer && (type & BFRALLOC_PREFER_DRAM)) { + // DRAM validation failed after alloc; attempt PSRAM as a last resort + buffer = p_malloc(size); + } +#endifIf you want this, I can propagate the same pattern to the BFRALLOC_NOBYTEACCESS branch guarded by CONFIG_IDF_TARGET_ESP32.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/util.cpp(1 hunks)wled00/wled.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/util.cppwled00/wled.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
📚 Learning: 2025-08-29T15:38:46.208Z
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
Applied to files:
wled00/util.cppwled00/wled.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-08-29T01:34:34.358Z
Learnt from: willmmiles
PR: wled/WLED#4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/wled.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled.cpp
🧬 Code graph analysis (2)
wled00/util.cpp (3)
wled00/fcn_declare.h (5)
getContiguousFreeHeap(461-461)getContiguousFreeHeap(464-464)d_free(442-442)d_free(444-444)p_free(451-451)wled00/wled.h (1)
size(171-177)wled00/bus_manager.cpp (8)
d_malloc(45-45)d_free(49-49)d_free(49-49)d_free(51-51)d_free(51-51)p_malloc(55-55)p_free(58-58)p_free(58-58)
wled00/wled.cpp (1)
wled00/fcn_declare.h (4)
getFreeHeapSize(460-460)getFreeHeapSize(463-463)getContiguousFreeHeap(461-461)getContiguousFreeHeap(464-464)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (99)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (5)
wled00/wled.cpp (2)
174-184: Good switch to platform-correct heap check.Using getFreeHeapSize() here aligns reconnection heuristics with the new DRAM-only metric and keeps ESP8266/ESP32 behavior consistent.
394-395: Consistent heap diagnostics.Replacing ESP.getFreeHeap()/ESP.getMaxFreeBlockSize() printouts with getFreeHeapSize()/getContiguousFreeHeap() across init stages is consistent with your new helpers. LGTM.
Also applies to: 418-418, 456-456, 468-468, 474-474, 539-539, 545-545
wled00/util.cpp (3)
669-679: ESP8266 d_realloc_malloc unconditionally frees the old buffer — causes silent data loss; audit callers or preserve data
- wled00/util.cpp (≈line 670) — ESP8266 branch does free(ptr) then d_malloc(size) (contents are not preserved).
- Repo search returned only definitions/declarations (wled00/util.cpp:670, 716; fcn_declare.h prototypes; bus_manager.cpp prototypes) — no callers were found by the quick grep.
- Action (choose one): (a) implement realloc-like preservation on resize (allocate new, memcpy min(old,new), free old), or (b) rename/document the function as a destructive realloc and perform a full audit of all call sites to ensure no code expects preserved contents.
632-646: Use CONFIG_IDF_TARGET_ (not IDF_TARGET_*) for ESP-IDF target checks*ESP-IDF defines CONFIG_IDF_TARGET_; guards using IDF_TARGET_ will not detect ESP32‑C3. Replace as shown:
-#if defined(BOARD_HAS_PSRAM) - #if defined(IDF_TARGET_ESP32C3) || defined(ESP8266) +#if defined(BOARD_HAS_PSRAM) + #if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(ESP8266) #error "ESP32-C3 and ESP8266 with PSRAM is not supported, please remove BOARD_HAS_PSRAM definition" #else // BOARD_HAS_PSRAM also means that compiler flag "-mfix-esp32-psram-cache-issue" has to be used #warning "BOARD_HAS_PSRAM defined, make sure to use -mfix-esp32-psram-cache-issue to prevent issues on rev.1 ESP32 boards \ see https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-guides/external-ram.html#esp32-rev-v1-0" #endif #else - #if !defined(IDF_TARGET_ESP32C3) && !defined(ESP8266) + #if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(ESP8266) #pragma message("BOARD_HAS_PSRAM not defined, not using PSRAM.") #endif #endifRepo search returned no results; run this to verify which macros are present in-tree:
git grep -n -e 'CONFIG_IDF_TARGET_' -e 'IDF_TARGET_' || true
715-722: d_realloc_malloc frees original on realloc failure — data-loss risk
- Repo scan: prototypes in wled00/fcn_declare.h and definitions for d_realloc_malloc / p_realloc_malloc in wled00/util.cpp; no callers found by the provided search.
- util.cpp already notes an ESP8266-specific realloc limitation — if destructive fallback is intentional, either (A) preserve original on failure and return nullptr (safer), or (B) keep the destructive behavior but rename the function and document it clearly in the header, or (C) provide both a non-destructive realloc-or-null API and a separate “replace on fail” helper.
- Required next step: pick one of the options above and add explicit documentation in wled00/fcn_declare.h; verify callers/platform-specific expectations before changing behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/FX.h (1)
651-651: Use p_free(name) in clearName(): addressedThis aligns with earlier guidance to unify alloc/free families.
🧹 Nitpick comments (2)
wled00/FX.h (2)
91-106: MAX_ caps look sensible; minor doc tweak + S2 sanity check*
- Good split: 64 segs with PSRAM, 32 otherwise; 8266 at 6 KB cap is conservative.
- The S2 comment “limit does not apply if PSRAM is available” can be misread as compile-time. It’s enforced at allocation time. Suggest clarifying the comment.
Please confirm S2 builds with typical feature sets (MQTT, a couple usermods) still retain ≥8–12 KB contiguous free after init.
Apply this tiny comment-only tweak:
- #define MAX_SEGMENT_DATA (20*1024) // 20k by default (S2 is short on free RAM), limit does not apply if PSRAM is available + #define MAX_SEGMENT_DATA (20*1024) // 20k by default (S2 is short on free RAM); DRAM limit only—PSRAM allocations are not artificially capped
110-110: Avoid tying “fairness” strictly to compile-time segment maxUsing MAX_NUM_SEGMENTS makes FAIR_DATA_PER_SEG constant even when only a few segments are active. This can force premature reallocation/eviction on PSRAM boards compiled for 64 segs but running 1–4 segs, which may increase fragmentation on low-heap devices (esp. 8266). Consider a runtime heuristic (e.g., clamp by active segments or apply a floor/ceiling) inside allocateData(), not necessarily here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.h(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/FX.h
🧠 Learnings (9)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/FX.h
📚 Learning: 2025-02-19T12:43:34.200Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.h
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX.h
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.h
🧬 Code graph analysis (1)
wled00/FX.h (3)
wled00/util.cpp (2)
allocate_buffer(750-811)allocate_buffer(750-750)wled00/fcn_declare.h (1)
p_free(451-451)wled00/FX_fcn.cpp (2)
Segment(60-82)Segment(85-93)
🔇 Additional comments (4)
wled00/FX.h (4)
605-606: Pixel buffer allocation path: LGTMGood use of allocate_buffer with BFRALLOC_PREFER_PSRAM|BFRALLOC_NOBYTEACCESS|BFRALLOC_CLEAR. This enables IRAM on classic ESP32 when available, falls back cleanly, and preserves prior calloc semantics.
628-628: Symmetric deallocation with p_free: LGTMDestructor now frees pixels with p_free, matching PSRAM/heap_caps allocations.
677-677: Expose getUsedSegmentData(): LGTMMaking this public supports allocator heuristics (util.cpp) without leaking mutability.
874-876: WS2812FX destructor: switch to p_free is correct — allocation symmetry verified_pixels and _pixelCCT are allocated with allocate_buffer in wled00/FX_fcn.cpp (lines 1224 and 1580) and freed with p_free (lines 1222 and 1614); no other assignments detected.
Besides better use of PSRAM and free heap handling in general there is a huge improvement for ESP32: with these changes it can work up to 64x64 pixels reliably without PSRAM (UI starts failing at half that size currently). Even got it to work up to about 80x64 pixels but UI access becomes unstable
allocateData()uses more elaborate DRAM checking to reduce fragmentation and allow for larger setups to run on low heapallocateData()does not limit amount if PSRAM is available (put segment data in PSRAM if DRAM is depleted) -> prefer functionality over speedMAX_SEGMENT_DATAto account for better segment data handlingPSRAM_THRESHOLDto improve PSARM usage: anything smaller is kept in DRAM, anything larger is put in PSRAM depending on how much heap is available. If heap is plenty, no PSRAM is used_pixelsis always put in PSRAM if available (except if it fits in ESP32's 32bit IRAM)getFreeHeapSize()andgetContiguousFreeHeap()the code may need some polishing (add debug outputs) and some fine-tuning of parameters as I did only test for borderline large 2D setups to see how well it performs under memory starvation but tested on all platforms.
There is a significant improvement if a board has PSRAM: there is virtually no limit to effect data and the heap is kept healthy and it is very stable, even with 10 overlapping segments of 32x32 pixels each.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores