Segment fixes #4811
Segment fixes #4811
Conversation
allocated array size was not defined well: if set matrix size is bigger than number of LEDs, it might have been accessed OoB.
WalkthroughThe updates modify memory allocation, error handling, and transition logic in the LED segment and matrix mapping code. Improvements include safer allocation and initialization of mapping tables, enhanced debug output, more robust transition management, and refined handling of segment names and blending during transitions. No public API signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
wled00/FX_2Dfcn.cpp
Outdated
|
|
||
| customMappingSize = 0; // prevent use of mapping if anything goes wrong | ||
| uint16_t matrixSize = Segment::maxWidth * Segment::maxHeight; | ||
| customMappingSize = max(getLengthTotal(), matrixSize); // is reset to 0 in case of error |
There was a problem hiding this comment.
getLengthTotal() does that.
I don't think it is wise what you did here.
There was a problem hiding this comment.
you are totally right. will revert.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/FX_2Dfcn.cpp (1)
86-86: Consider validating gap table size against customMappingSizeThe gap table validation checks if
gapSize >= matrixSize, but sincecustomMappingSizecan be larger thanmatrixSize(whengetLengthTotal() > matrixSize), you might want to validate againstcustomMappingSizeinstead to ensure the gap table covers all allocated entries.- if (!map.isNull() && gapSize >= matrixSize) { // not an empty map + if (!map.isNull() && gapSize >= customMappingSize) { // not an empty map
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX_2Dfcn.cpp(2 hunks)wled00/FX_fcn.cpp(13 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: blazoncek
PR: wled/WLED#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: wled/WLED#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: wled/WLED#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: 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: 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.
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.
Learnt from: DedeHai
PR: wled/WLED#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: KrX3D
PR: wled/WLED#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: wled/WLED#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:
1. Null pointer checks
2. Length validation against WLED_MAX_SEGNAME_LEN
3. Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
Learnt from: DedeHai
PR: wled/WLED#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 dx*dx + dy*dy to be a practical concern in particle system effects.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
📚 Learning: in wled, the segment::allocatedata() method already includes optimization to check if data is alloca...
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_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in wled segments, the backing pixel buffer is always allocated to the full physical segment size, bu...
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_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: wled does not support matrices large enough (≥1024 px) for 32-bit overflow in dx*dx + dy*dy to be a ...
Learnt from: DedeHai
PR: wled/WLED#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 dx*dx + dy*dy to be a practical concern in particle system effects.
Applied to files:
wled00/FX_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in the wled codebase, the `getsegment()` function guards against out-of-bounds segment ids, and `get...
Learnt from: blazoncek
PR: wled/WLED#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.
Applied to files:
wled00/FX_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in the wled codebase, the `getsegment()` function guards against out-of-bounds segment ids by fallin...
Learnt from: blazoncek
PR: wled/WLED#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.
Applied to files:
wled00/FX_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in wled's fx.h, mode_count represents the highest fx_mode_ id + 1, not the total count of fx_mode_ d...
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_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in the wled codebase, segment.custom3 is always constrained to the range 0-31 and will not exceed th...
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/FX_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: the setname() method in wled's segment class (fx_fcn.cpp) handles all necessary validation internall...
Learnt from: blazoncek
PR: wled/WLED#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:
1. Null pointer checks
2. Length validation against WLED_MAX_SEGNAME_LEN
3. Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
Applied to files:
wled00/FX_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in the seven segment display reloaded usermod, the dimension mismatch between the default array (ums...
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
Applied to files:
wled00/FX_2Dfcn.cpp
📚 Learning: using progmem for the seven-segment font array (umssdrnumbers) in the wled ssdr usermod causes compi...
Learnt from: KrX3D
PR: wled/WLED#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.
Applied to files:
wled00/FX_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in wled, maximum segment name length varies by platform: - esp8266: 32 characters (wled_max_segname_...
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_2Dfcn.cppwled00/FX_fcn.cpp
📚 Learning: in wled, the wled_max_panels macro is intentionally defined as a fixed constant value (18) with no r...
Learnt from: blazoncek
PR: wled/WLED#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.
Applied to files:
wled00/FX_2Dfcn.cpp
📚 Learning: esp8266 and esp32 platforms have different maximum segment name lengths in wled, which can cause tru...
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_fcn.cpp
📚 Learning: in wled's audioreactive usermod, fft buffer memory (valfft, windowfft) is allocated once during init...
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.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code Graph Analysis (1)
wled00/FX_2Dfcn.cpp (3)
wled00/FX_fcn.cpp (2)
getLengthTotal(1795-1799)getLengthTotal(1795-1795)wled00/fcn_declare.h (1)
d_free(565-565)wled00/util.cpp (2)
d_malloc(662-670)d_malloc(662-662)
🔇 Additional comments (14)
wled00/FX_2Dfcn.cpp (2)
52-56: Good fix for preventing out-of-bounds access!The change to set
customMappingSizeto the maximum ofgetLengthTotal()andmatrixSizeproperly addresses the potential crash issue mentioned in the PR objectives. This ensures adequate memory allocation for the mapping table.
127-127: Good defensive programming practiceSetting
customMappingSize = 0on allocation failure is a proper safeguard to prevent use of an invalid mapping table.wled00/FX_fcn.cpp (12)
70-80: Good defensive memory allocation patternThe deferred allocation of
nameanddatauntil after successful pixel buffer allocation prevents partial resource allocation on failure. This is a solid improvement to the error handling.
113-119: Consistent error handling in copy assignmentGood to see the same defensive allocation pattern applied here as in the copy constructor.
288-290: Critical crash prevention check addedThe check for
!_t->_oldSegment->isActive()before continuing the transition is crucial. This prevents potential crashes when the old segment copy fails to allocate properly, which aligns with the PR's crash fix objective.
306-311: Consistent transition safety checksGood to see the same safety pattern applied when starting transitions.
432-441: Proper transition handling during geometry changesStopping transitions when segment dimensions change is essential to prevent out-of-bounds access. The use of
deallocateData()andp_free()for cleanup is cleaner than direct free calls.
463-475: Robust pixel buffer allocation with proper error handlingThe allocation pattern with IRAM/PSRAM preference and comprehensive error handling (deallocating data, setting error flag, marking segment inactive) prevents the segment from operating in an invalid state.
584-585: Smart transition handling for scrolling textGood addition to trigger transitions with segment copy when the scrolling text content changes, ensuring smooth visual transitions.
1269-1270: Safe name comparison for transition handlingThe additional check for differing segment names with proper null checks ensures correct transition behavior for effects that depend on segment names (like scrolling text).
1222-1223: Smart memory allocation strategy for large buffersUsing explicit free + malloc instead of realloc for the pixel buffer is a good choice to reduce heap fragmentation, especially since the old data doesn't need to be preserved during initialization.
1630-1631: Essential null check prevents crashesThis early return when
_pixelsis null is a critical safety check that prevents crashes ifshow()is called after pixel buffer allocation fails.
1473-1477: Consistent name-aware blending logicThe name comparison in fade blending is consistent with the service() logic, ensuring proper visual transitions for name-dependent effects.
2028-2029: No DRAM allocation issueThe
d_mallocmacro is defined asmalloc(see wled00/fcn_declare.h), so it already allocates from the standard heap (DRAM) and not PSRAM. The existing comment correctly reflects the allocation behavior—no change needed.
This reverts commit b96638d.
|
is there any other changes / fixes that I missed or is this good to merge? |
|
I don't see anything particularly wrong but you have to be careful and do extensive testing when pulling from my fork. It might be good to consolidate DEBUG statements (I use differentiating DEBUG statements, i.e. DEBUGFX_... for effect debugging whereas upstream does not). |
|
I am running these changes and so far it looks good, but will review the changes again. |
|
my opening "statement" sais it wont fix it. |
* Pulling in proper segment memory handling&fixes from @blazoncek dev branch
3 participants
there is another discrepancy in the gap table allocation in
setUpMatrix():if the gap file has fewer entries than the matrix size, that also may lead to out of bounds access but I was not sure what the proper way to fix it is: ignore the file if it does not match size or just fill it with "1"?
Summary by CodeRabbit
Bug Fixes
Other Improvements