using segment buffer instead of local buffer for PS #4776
Conversation
|
""" WalkthroughThis change updates the particle system rendering in the WLED codebase from RGB (CRGB) to RGBW (CRGBW) color representation, affecting both 1D and 2D particle systems. It modifies particle vertical position initialization in one effect and adds friend class declarations to the Changes
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧠 Learnings (2)📓 Common learningswled00/FXparticleSystem.cpp (13)Learnt from: willmmiles Learnt from: blazoncek Learnt from: netmindz Learnt from: netmindz Learnt from: DedeHai Learnt from: KrX3D Learnt from: DedeHai Learnt from: blazoncek Learnt from: blazoncek Learnt from: blazoncek Learnt from: blazoncek
Learnt from: DedeHai Learnt from: blazoncek 🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningswled00/FXparticleSystem.cpp (13)Learnt from: willmmiles Learnt from: blazoncek Learnt from: netmindz Learnt from: netmindz Learnt from: DedeHai Learnt from: KrX3D Learnt from: DedeHai Learnt from: blazoncek Learnt from: blazoncek Learnt from: blazoncek Learnt from: blazoncek
Learnt from: DedeHai Learnt from: blazoncek 🔇 Additional comments (15)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
wled00/FXparticleSystem.cpp (1)
1796-1796: Critical: Insufficient buffer allocation.The buffer allocation uses
sizeof(CRGB)but should usesizeof(CRGBW)since the framebuffer is nowCRGBW*. This will cause buffer overruns.Apply this fix:
-requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength(); +requiredmemory += sizeof(CRGBW) * SEGMENT.virtualLength();
🧹 Nitpick comments (3)
wled00/FX.h (1)
811-813: Friendship widens Segment’s private surface – consider a narrower contractMaking both
ParticleSystem2DandParticleSystem1Dfull friends gives them unrestricted access to every private / protected member ofSegment, far beyond the pixel buffer that the new PS code really needs (getPixels()and maybe geometry helpers). This:
- Breaks encapsulation and tight-couples the particle systems to
Segment’s internal layout.- Increases maintenance risk – any future sensitive field added to
Segmentinstantly becomes reachable by the PS classes.Suggested lighter alternatives:
- friend class ParticleSystem2D; - friend class ParticleSystem1D; +// expose only the minimal API needed by particle systems +public: + inline uint32_t* pixelBuffer() { return pixels; } + inline unsigned pixelBufferLength() const { return length(); } + +private: + // keep internals hidden from ParticleSystem classesor
Convert the PS classes’ hot paths to call the existing protected
getPixels()/geometry helpers and addfriendfunctions (not the whole class) if absolutely unavoidable.This retains the performance benefit while limiting trust to the bare essentials.
wled00/FXparticleSystem.cpp (2)
1105-1105: Consider removing obsolete commented code.This commented line for CRGB buffer allocation is no longer needed since the framebuffer now uses the segment's pixel buffer. Consider removing it in a future cleanup.
981-982: Architectural consideration: Shared buffer usage.The particle systems now use the segment's pixel buffer directly instead of allocating their own. While this saves memory and can improve performance, ensure that:
- The segment buffer remains valid throughout the particle system's lifetime
- No concurrent access issues arise if effects are switched or segments are modified
- The friend class relationship (mentioned in PR) is properly documented
Also applies to: 1735-1736
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.cpp(1 hunks)wled00/FX.h(1 hunks)wled00/FXparticleSystem.cpp(18 hunks)wled00/FXparticleSystem.h(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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: 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: 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#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: 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: 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: 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: 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.
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.
Learnt from: KrX3D
PR: wled/WLED#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: KrX3D
PR: wled/WLED#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: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
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.
wled00/FX.h (2)
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: 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.
wled00/FXparticleSystem.h (2)
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: 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.
wled00/FXparticleSystem.cpp (6)
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: 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.
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.
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: 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: 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.
🔇 Additional comments (12)
wled00/FX.cpp (1)
8840-8840: Good positioning adjustment for particle emission.Setting particles to start at y=0 (bottom edge) instead of an offset makes sense for this effect, providing a cleaner emission point that aligns with the segment boundaries.
wled00/FXparticleSystem.h (5)
191-191: LGTM! Consistent color type update.The parameter type change from
CRGB&toCRGBW&is correct and consistent with the migration to RGBW color support.
203-203: LGTM! Framebuffer pointer type updated for RGBW.The framebuffer pointer type change to
CRGBW*is necessary to match the segment's pixel buffer type.
209-209: Verify the type change for wallRoughness.The change to
uint32_tforwallRoughnessseems unrelated to the RGBW migration. Please confirm this type change is intentional and necessary.
222-222: LGTM! Blur function updated for RGBW.The blur2D function signature correctly updated to accept
CRGBW*buffer.
354-354: LGTM! 1D particle system consistently updated.The 1D particle system methods and pointers are correctly updated to use
CRGBWtypes, maintaining consistency with the 2D system changes.Also applies to: 366-366, 387-387
wled00/FXparticleSystem.cpp (6)
20-21: LGTM! Color utility functions updated.The fast_color_add and fast_color_scale function declarations correctly updated to use
CRGBW.
981-982: LGTM! Using segment buffer as intended.The framebuffer now correctly points to the segment's pixel buffer via
SEGMENT.getPixels(), eliminating the need for separate buffer allocation. This aligns with the PR's objective of using the segment buffer instead of a local buffer.
596-598: LGTM! Color conversion properly updated.The rgb2hsv/hsv2rgb calls correctly use the
.color32member to access the full color value in CRGBW format.
1005-1006: LGTM! Blur2D function properly updated.The blur2D function signature and local variables correctly use
CRGBWtypes.
680-680: LGTM! Render buffers properly sized for RGBW.The local render buffers in both 2D (100 pixels) and 1D (10 pixels) particle systems are correctly declared as
CRGBWarrays.Also applies to: 1520-1520
1771-1771: LGTM! Missing semicolon added.Fixed syntax error by adding the missing semicolon.
|
FYI: PS Vortex runs at an impressive 33FPS at 128x64 pixels. |
still need to figure out a good way to make 1D->2D expansions work
|
1D->2D expansion is not implemented yet. |
You will need to use regular |
|
I have already implemented 1D->2D locally using setPixelColor(), crashes all the time though and need to investigate on where I am overrunning a buffer. |
I believe that's the design intent, but I think the current implementation might technically permit some races. The write on FX_fcn.cpp:1210 is not a memory fence; there is no strict requirement that Ideally |
a race condition could explain the erratic crash dumps I see in #4779 |
The proper place to check for is
Please do. That's beyond my knowledge. Would adding Check if you can see debug output specified here. As |
- fix rendering if gamma correction is disabled - some code cleanup
If PS memory allocation fails, try several times, reducing the number of particles rather than just quitting. This helps with larger setups that may not have enough memory in a contiguous block. Now chances of a PS running are much higher.
- using references is slightly faster but using function calls by value is safer - this is in perparation to optimize ram usage on ESP32 for larger matrix setups - optimized by dropping the scale check, it is faster overall - FPS is now more consistent and on average about the same as it was
|
FPS comparison shows a 15% increase in speed overall |
3 participants
tested up to 8000 LEDs and PS still works on a single segment.
Speed is about the same, even slightly improved but insignificant.
Summary by CodeRabbit
New Features
Improvements
Compatibility