Segment layers and better effect transitions (blending) #4658
Conversation
- Photoshop-style segment/layer blending - return of strip ABL - remove global LED buffer in favour of segment-local buffer - new effect blending modes/transitions - custom palettes moved out of WS2812FX class - increased limits (matrix size, LED RAM) - added "rainbow"-mode colorwheel - replaced palettes with gamma unmodified ones - move gamma adjustment to last step before sending to LEDs - Segment & WS2812FX class reorganisation (mutable members, reordered members, protected members)
WalkthroughThis update implements extensive refactoring and modernization across the codebase, focusing on memory management, palette handling, and segment logic. Key changes include the introduction of PSRAM/DRAM-aware memory allocation wrappers, removal of deprecated global LED buffer usage, consolidation and extension of palette management (including custom palettes and blend modes), and improvements to segment and bus configuration logic. Numerous function signatures were updated for type safety and clarity, and several UI and JSON interface components were revised to reflect new features and configuration options. The update also includes bug fixes, code cleanup, and adjustments to support more robust and flexible LED output and palette operations. Changes
Assessment against linked issues
Suggested reviewers
✨ 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:
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 (
|
blazoncek
requested review from
DedeHai,
netmindz,
willmmiles,
Aircoookie and
softhack007
|
There are a lot of changes in Segment and WS2812FX classes so it may be too tedious to go through diff. The limitations are mainly based on amount of free RAM and ESP8266 may only work with 2 to 3 segments of 256 pixels (16x16 matrix) overlaid. S2 may struggle too if many usermods are loaded. The side effect of the new approach is more than 3x speed improvement compared to old versions. |
|
I started reviewing, I saw a lot of changes unrelated to layering, in scrolling text for example. Would it make sense to keep those to a minimum? I can also add comments and suggestions. |
Quite true, however, as I said previously, these are collateral damage from developing in my fork. All of them are intended to make WLED code cleaner and introduce "separations of duties". I do not like interdependencies and prefer modularity. As for Scrolling text, the change does not affect final output. As a matter of fact I have modification to "Overlay" option for other effects ready. It is no longer needed and can be removed as any effect can be overlaid. Some other workarounds implemented (i.e. Matrix effect etc) can be removed too. |
|
Ok, I keep that in mind during the review and will ad my 2 cents :) |
blazoncek
added
enhancement
Awaiting testing
optimization
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (5)
wled00/udp.cpp (1)
329-338:⚠️ Potential issueIncorrect assignment overwrites
check2/check3flags
check1is being assigned three times, which clobbers the values that should go intocheck2andcheck3.selseg.check1 = (udpIn[31+ofs]>>5) & 0x1; selseg.check1 = (udpIn[31+ofs]>>6) & 0x1; // should be check2 selseg.check1 = (udpIn[31+ofs]>>7) & 0x1; // should be check3Proposed fix:
- selseg.check1 = (udpIn[31+ofs]>>5) & 0x1; - selseg.check1 = (udpIn[31+ofs]>>6) & 0x1; - selseg.check1 = (udpIn[31+ofs]>>7) & 0x1; + selseg.check1 = (udpIn[31+ofs]>>5) & 0x1; + selseg.check2 = (udpIn[31+ofs]>>6) & 0x1; + selseg.check3 = (udpIn[31+ofs]>>7) & 0x1;Because the flags are later consumed by effects, the current code silently disables two of them – a hard-to-trace bug.
wled00/set.cpp (1)
148-219: 🛠️ Refactor suggestionPotential accumulation of stale bus configurations
busConfigs.emplace_back(...)appends new entries but the vector is not cleared before repopulating it.
Editing LED settings multiple times during the same uptime will create duplicate bus entries and undefined behaviour whenWLED::loop()re-initialises buses.Add a clear before the loop:
+ busConfigs.clear(); + busConfigs.shrink_to_fit(); // optional for (int s = 0; s < 36; s++) {wled00/led.cpp (1)
7-22:⚠️ Potential issueGracefully handle invalid segment index
setValuesFromSegment()dereferencesstrip.getSegment(s)without range checking.
Ifs == UINT8_MAX(returned bygetFirstSelectedSegId()when nothing is selected) this becomes UB.
Quick fix:void setValuesFromSegment(uint8_t s) { - const Segment& seg = strip.getSegment(s); + if (s >= strip.getSegmentsNum()) return; + const Segment& seg = strip.getSegment(s);wled00/FX_2Dfcn.cpp (1)
373-397: 🛠️ Refactor suggestionLarge VLA on the stack can blow up small cores
uint32_t newPxCol[vW];(and the equivalent inmoveY) is a variable-length array that lives on the task stack.
On ESP8266 the default stack is 4 KiB – a 1024-pixel wide matrix would allocate 4 KiB twice per call, risking a hard crash.Portable, low-risk alternative:
- uint32_t newPxCol[vW]; + std::vector<uint32_t> newPxCol(vW); // allocates in heap/PSRAM + if (newPxCol.empty()) return; // graceful out-of-memoryIf heap allocation worries you, keep a single static scratch buffer in the class instead of per-call VLAs.
wled00/bus_manager.cpp (1)
26-26: 🛠️ Refactor suggestionPrefer portable
<memory>over the lib-internal<bits/unique_ptr.h>
<bits/…>headers are implementation details of GCC’s libstdc++ and are not guaranteed to exist (or be reachable) on all tool-chains that can build WLED, e.g. ESP-IDF or Arduino-CLI when they switch to libc++/newlib.
Please include the standard<memory>header instead;std::unique_ptris defined there.-#include <bits/unique_ptr.h> +#include <memory> // std::unique_ptr
🧹 Nitpick comments (20)
wled00/file.cpp (2)
42-42: Fixed format specifier for timing values.Changed format specifier from
%dto%lufor printing timing values frommillis(), which is an unsigned long. This prevents potential format string warnings or errors.Also applies to: 72-73, 114-115, 154-155, 179-180, 206-207, 254-255, 324-325, 359-360
422-423: Debug output prefix updated for file system operations.Changed from
DEBUG_PRINTtoDEBUGFS_PRINTto use the file system-specific debug print macro, providing more contextual logging information.wled00/FX.cpp (1)
5056-5060: Hardcoded fire palette replaces dynamic loadingThe code replaces a dynamic palette loading mechanism with a hardcoded fire-themed palette. While this might be more efficient, it reduces flexibility and customization options.
Consider maintaining the dynamic palette loading capability alongside the hardcoded default:
-//CRGBPalette16 pal = SEGMENT.check1 ? SEGPALETTE : SEGMENT.loadPalette(pal, 35); -CRGBPalette16 pal = SEGMENT.check1 ? SEGPALETTE : CRGBPalette16(CRGB::Black, CRGB::Black, CRGB::Black, CRGB::Black, - CRGB::Red, CRGB::Red, CRGB::Red, CRGB::DarkOrange, - CRGB::DarkOrange,CRGB::DarkOrange, CRGB::Orange, CRGB::Orange, - CRGB::Yellow, CRGB::Orange, CRGB::Yellow, CRGB::Yellow); +CRGBPalette16 pal; +if (SEGMENT.check1) { + pal = SEGPALETTE; +} else if (SEGMENT.palette == 255) { + // Use hardcoded fire palette + pal = CRGBPalette16(CRGB::Black, CRGB::Black, CRGB::Black, CRGB::Black, + CRGB::Red, CRGB::Red, CRGB::Red, CRGB::DarkOrange, + CRGB::DarkOrange,CRGB::DarkOrange, CRGB::Orange, CRGB::Orange, + CRGB::Yellow, CRGB::Orange, CRGB::Yellow, CRGB::Yellow); +} else { + pal = SEGMENT.loadPalette(pal, 35); +}wled00/bus_manager.h (1)
117-117: Added humorous comment to destructorA joke comment "throw the bus under the bus" was added to the destructor. While humorous, it doesn't add technical value but also doesn't hurt readability.
wled00/udp.cpp (1)
415-428:realtimeLock()unconditionally brightens an “off” stripWhen
bri==0butbriT==0, the code sets brightness to the last non-zero level:if (briT == 0) { strip.setBrightness(scaledBri(briLast), true); }This means simply receiving a realtime frame on a turned-off device will light it up, which may surprise the user.
Consider guarding this with a user preference (e.g.arlsRespectOffState) or honouringbri==0unless the sender explicitly requests power-on.wled00/util.cpp (1)
622-632: Missing<utility>include forstd::swap
w_malloc()/w_realloc()/w_calloc()usestd::swap, but this header doesn’t include<utility>or<algorithm>.
Some tool-chains fail to compile if the header isn’t pulled in transitively.+#include <utility> // for std::swapPlace it near the top, before the helpers.
wled00/set.cpp (2)
212-218:maMaxsilently disabled when per-port ABL is offmaMax = request->arg(ma).toInt() * request->hasArg(F("PPL"));Multiplying by the boolean result of
hasArg()masks intent and makes the expression hard to read.
Explicitly set to zero when PP-ABL is disabled for clarity:- maMax = request->arg(ma).toInt() * request->hasArg(F("PPL")); + maMax = request->hasArg(F("PPL")) ? request->arg(ma).toInt() : 0;
773-802:panelvector cleared butreserve()ed size not validated
strip.panel.clear();empties the vector, then youreserve(panels)and push new entries.
If the user reduces the panel count, old data is lost – fine – but when increasing the count, there is no upper-bound check forwidth,height, offsets etc. This can result in out-of-range LED indices later indeserializeMap().Consider validating each pushed
Panel:if (p.width == 0 || p.height == 0 || p.bottomStart >= strip.getLengthTotal() ) continue; // skip invalid panelFail-fast here saves chasing mysterious mapping artefacts.
wled00/const.h (4)
80-88: Off-by-one hack for GPIO16 deserves an explanatory comment
WLED_NUM_PINS (GPIO_PIN_COUNT+1)compensates for missing GPIO16 in the ESP8266 IDF.
Please add a short “WHY” comment and astatic_assert(GPIO_PIN_COUNT==17)to avoid surprises if the core ever fixes the constant.
135-136:WLED_MAX_PANELSshould be compile-time guardedThe note “must not be more than 32” is easy to violate during refactors.
Recommend:static_assert(WLED_MAX_PANELS <= 32, "Panel count capped at 32");
451-459: Large defaultMAX_LEDSmay trip memory on vanilla ESP3216 384 LEDs * 4 bytes ≈ 64 KiB, before effect buffers and overhead.
On non-PSRAM boards this is perilously close to the free DRAM ceiling.
Please revisit the default or gate it behindpsramFound().
463-472:MAX_LED_MEMORYvalues out of sync withMAX_LEDSFor ESP32 the memory cap is 65 536 bytes (same as 16 384 × 4); any extra per-pixel scratch buffers (e.g. frame-to-frame) will overflow this limit.
Consider raisingMAX_LED_MEMORYin tandem or deriving it fromMAX_LEDS * sizeof(pixel) + headroom.wled00/led.cpp (2)
68-73: Document new realtime-override behaviourAllowing brightness changes when
realtimeOverrideis set is a behavioural change that may surprise integrations expecting the old lock-out.
Please add a comment or release-note entry explaining the new rule.
125-137: Transition logic may double-trigger.show()Inside the “no transition” branch you call both
applyFinalBri()(which already performsstrip.trigger()) and an extrastrip.trigger().
This results in two immediate frame sends back-to-back.- applyFinalBri(); - strip.trigger(); + applyFinalBri(); // already triggers oncewled00/fcn_declare.h (2)
229-238: Forward declaration ofSegmentduplicatedA forward declaration already exists in other headers (e.g.
FX.h).
Keeping multiple can cause ODR or include-order confusion.
Consider moving to a single public header (e.g.segment_types.h).
279-281: Macros obscure call-sites
#define setValuesFromMainSeg()etc. hide parameter passing and hinder debugging (no type safety).
Recommend converting tostatic inlinewrappers instead.static inline void setValuesFromMainSeg() { setValuesFromSegment(strip.getMainSegmentId()); }wled00/json.cpp (2)
534-541:rmcpalbranch leaves stale RAM objects when file-removal failsIf
WLED_FS.remove()cannot delete the palette file (e.g., FS full/readonly) the in-RAMcustomPalettesvector is still reloaded, silently desynchronising FS and RAM.
Consider checking the return value ofWLED_FS.remove()and only invokingloadCustomPalettes()when the deletion was successful, or at least log an error so users know something went wrong.
936-945: Potential (but unlikely) truncation when counting palettes
customPalettes.size()is assigned to a signed 32-bitint. On typical builds this is fine, yet the type changes tosize_ton both AVR and ESP32, and future UI extensions could legitimately exceed 32 k.
A safer pattern keeps everything insize_tuntil the value is emitted to JSON:- int customPalettesCount = customPalettes.size(); - int palettesCount = getPaletteCount() - customPalettesCount; + const size_t customPalettesCount = customPalettes.size(); + const size_t palettesCount = getPaletteCount() - customPalettesCount;This is a nit but prevents signed/unsigned warnings with stricter compilers.
wled00/FX.h (2)
109-110: MacroMIN_SHOW_DELAYcaptures a member variable – risks ODR / compile errors
_frametimeis a non-static member ofWS2812FX, but the macro is defined at global scope.
Using the macro outside of a member function (or before an object named_frametimeis visible) will not compile and may violate the One-Definition-Rule if included in several translation units with different meanings.Consider:
- Replacing the macro with an
inlinehelper inWS2812FX, or- Using the already available
strip.getMinShowDelay()in client code.
601-606: Possible confusion ind_calloc()parameter order
C/POSIXcallocexpects(num, size), yet the call passessizeof(uint32_t)as the first parameter andlength()as the second.
Whilenum*sizehappens to be the same either way for 32-bit words, the reversed order is error-prone and hinders readability for future maintainers.- pixels = static_cast<uint32_t*>(d_calloc(sizeof(uint32_t), length())); + pixels = static_cast<uint32_t*>(d_calloc(length(), sizeof(uint32_t)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
usermods/Analog_Clock/Analog_Clock.cpp(2 hunks)usermods/Cronixie/Cronixie.cpp(1 hunks)usermods/audioreactive/audio_reactive.cpp(4 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(3 hunks)wled00/FX.cpp(4 hunks)wled00/FX.h(21 hunks)wled00/FX_2Dfcn.cpp(16 hunks)wled00/bus_manager.cpp(17 hunks)wled00/bus_manager.h(9 hunks)wled00/button.cpp(2 hunks)wled00/cfg.cpp(11 hunks)wled00/colors.cpp(2 hunks)wled00/const.h(4 hunks)wled00/data/index.css(1 hunks)wled00/data/index.htm(2 hunks)wled00/data/index.js(9 hunks)wled00/data/settings_leds.htm(2 hunks)wled00/e131.cpp(8 hunks)wled00/fcn_declare.h(5 hunks)wled00/file.cpp(12 hunks)wled00/image_loader.cpp(1 hunks)wled00/ir.cpp(4 hunks)wled00/json.cpp(24 hunks)wled00/led.cpp(5 hunks)wled00/mqtt.cpp(4 hunks)wled00/overlay.cpp(1 hunks)wled00/palettes.h(3 hunks)wled00/presets.cpp(7 hunks)wled00/set.cpp(24 hunks)wled00/udp.cpp(16 hunks)wled00/util.cpp(4 hunks)wled00/wled.cpp(2 hunks)wled00/wled.h(2 hunks)wled00/wled_eeprom.cpp(1 hunks)wled00/wled_server.cpp(1 hunks)wled00/xml.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
wled00/json.cpp (1)
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().
🧬 Code Graph Analysis (8)
wled00/wled_server.cpp (2)
wled00/fcn_declare.h (1)
loadCustomPalettes(175-175)wled00/colors.cpp (2)
loadCustomPalettes(235-281)loadCustomPalettes(235-235)
wled00/mqtt.cpp (2)
wled00/fcn_declare.h (3)
w_free(551-551)w_free(551-551)w_malloc(548-548)wled00/util.cpp (2)
w_malloc(623-631)w_malloc(623-623)
wled00/presets.cpp (3)
wled00/fcn_declare.h (3)
w_free(551-551)w_free(551-551)w_malloc(548-548)wled00/bus_manager.cpp (3)
w_free(43-43)w_free(43-43)w_malloc(40-40)wled00/util.cpp (2)
w_malloc(623-631)w_malloc(623-623)
wled00/button.cpp (1)
wled00/led.cpp (2)
colorUpdated(188-191)colorUpdated(188-188)
wled00/set.cpp (7)
wled00/bus_manager.h (9)
n(83-86)n(83-83)ablMilliampsMax(444-444)ablMilliampsMax(444-444)setMilliampsMax(445-445)setMilliampsMax(445-445)start(73-73)start(148-148)start(148-148)wled00/fcn_declare.h (9)
loadCustomPalettes(175-175)getNumVal(492-492)savePreset(337-337)updateVal(496-496)applyPreset(333-333)colorKtoRGB(182-182)colorFromDecOrHexString(186-186)unloadPlaylist(323-323)setTimeFromAPI(314-314)wled00/colors.cpp (6)
loadCustomPalettes(235-281)loadCustomPalettes(235-235)colorKtoRGB(344-366)colorKtoRGB(344-344)colorFromDecOrHexString(458-476)colorFromDecOrHexString(458-458)wled00/util.cpp (4)
getNumVal(7-10)getNumVal(7-7)updateVal(85-92)updateVal(85-85)wled00/presets.cpp (4)
savePreset(218-278)savePreset(218-218)applyPreset(127-134)applyPreset(127-127)wled00/playlist.cpp (2)
unloadPlaylist(45-53)unloadPlaylist(45-45)wled00/ntp.cpp (2)
setTimeFromAPI(537-550)setTimeFromAPI(537-537)
wled00/e131.cpp (2)
wled00/fcn_declare.h (1)
setRealtimePixel(357-357)wled00/udp.cpp (2)
setRealtimePixel(675-679)setRealtimePixel(675-675)
wled00/fcn_declare.h (2)
wled00/wled.h (2)
ptr(172-175)ptr(172-172)wled00/util.cpp (12)
w_malloc(623-631)w_malloc(623-623)w_calloc(643-651)w_calloc(643-643)w_realloc(633-641)w_realloc(633-633)d_malloc(653-661)d_malloc(653-653)d_calloc(673-681)d_calloc(673-673)d_realloc(663-671)d_realloc(663-663)
wled00/util.cpp (2)
wled00/fcn_declare.h (6)
hw_random8(542-542)hw_random8(542-542)hw_random8(543-543)hw_random8(543-543)hw_random8(544-544)hw_random8(544-544)wled00/wled.h (4)
size(168-171)size(168-168)ptr(172-175)ptr(172-172)
🔇 Additional comments (92)
wled00/wled_eeprom.cpp (1)
228-228: Relocated palette blend management from strip to global variable.This change correctly aligns with the broader refactoring where palette-related functionality is being decoupled from the
stripobject, improving modularity and maintainability.wled00/data/index.css (2)
356-357: Added styling for blend mode selection container.Added the
#bsp(blend settings panel) selector to the list of elements with a max-width of 280px, ensuring consistent UI styling with other containers.
361-364: Added margin and height styling for blend mode selector.Added the
#bspselector to elements that receive consistent min-height and margin styling, maintaining visual consistency with other UI controls.wled00/image_loader.cpp (1)
81-81:❓ Verification inconclusive
Removed effect transition check that was preventing image rendering.
By commenting out this check, image rendering will now proceed during effect transitions. This aligns with the broader segment refactoring in the PR, but could potentially reintroduce the issues mentioned in the comment above.
Verify that the segment layer blending refactoring has addressed the original issues (flickering, multiple allocations, partial remains of old effects) that this check was meant to prevent. If these issues persist in specific scenarios, consider implementing a more targeted solution.
🏁 Script executed:
#!/bin/bash # Look for any other files that might handle transition state checks # or implement protection against the issues mentioned in the comment echo "Searching for files that handle segment transitions or effect changes..." rg -l "transition|effect.*change|flickering" --glob "wled00/*.{cpp,h}" echo "Checking other instances of renderImageToSegment usage..." rg -A 5 "renderImageToSegment" --glob "wled00/*.{cpp,h}"Length of output: 1798
Verify segment transition handling
I didn’t find any other guard against rendering during effect transitions in the codebase—no new checks around
seg.modeorseg.currentMode()were added elsewhere. Please confirm that the segment-layer blending refactoring fully prevents the original issues (flickering, extra allocations, stale FX frames) before permanently removing this check. If any of those problems still occur under certain images or effects, reintroduce a more targeted guard or adjust the blending logic accordingly.• wled00/image_loader.cpp – around line 81, the mode check is commented out:
//if (seg.mode != seg.currentMode()) return IMAGE_ERROR_WAITING;usermods/Cronixie/Cronixie.cpp (1)
250-251:✅ Verification successful
Removed gamma correction from color retrieval.
This change removes the gamma32() function when retrieving the segment color, which aligns with the PR's overall approach of centralizing gamma correction. Instead of applying gamma correction at individual color retrieval points, it appears that gamma correction is now applied at a more centralized point.
This change assumes that gamma correction is now handled elsewhere in the codebase. Let's verify this assumption:
🏁 Script executed:
#!/bin/bash # Check for gamma correction changes in the PR echo "Searching for gamma-related changes..." rg "gamma" --glob "wled00/FX*.{cpp,h}" echo "Looking for centralized color correction..." rg "setPixelColor" --glob "wled00/bus_*.cpp"Length of output: 2094
🏁 Script executed:
#!/bin/bash # Inspect Cronixie usermod code around color retrieval and pixel writes echo "=== strip usage ===" rg -n "strip\." usermods/Cronixie/Cronixie.cpp || true echo "=== setPixelColor usage ===" rg -n "setPixelColor" usermods/Cronixie/Cronixie.cpp || true echo "=== Context around lines 240-300 ===" sed -n '240,300p' usermods/Cronixie/Cronixie.cppLength of output: 2410
]
🏁 Script executed:
#!/bin/bash # Locate the definition of strip and Segment echo "=== Searching for 'extern' declarations of strip ===" rg -n "extern.*strip" -g "wled00/*.h" || true echo "=== Searching for getSegment definition ===" rg -n "getSegment(" -g "wled00" || true echo "=== Inspecting Segment class ===" # If getSegment found in a header, extract its class context rg -n "class Segment" -g "wled00/*.h" || trueLength of output: 606
🏁 Script executed:
#!/bin/bash # Examine BusDigital::setPixelColor implementation to check for gamma correction echo "=== Locating BusDigital class ===" rg -n "class BusDigital" -g "wled00/bus_manager.cpp" echo "=== Context around BusDigital::setPixelColor ===" # Extract lines around the method definition rg -n "BusDigital::setPixelColor" -g "wled00/bus_manager.cpp" -A 10 -B 10Length of output: 1832
Centralized gamma correction now handles segment colors
Manual gamma32() around segment color retrieval has been removed because the FX pipeline applies gamma centrally (see FX_fcn.cpp’s BusManager::setPixelColor calls using gamma32()). Cronixie writes raw segment colors into the pixel buffer and relies on the centralized correction.
- File: usermods/Cronixie/Cronixie.cpp
Lines ~250:- uint32_t col = gamma32(strip.getSegment(0).colors[1]);
- uint32_t col = strip.getSegment(0).colors[1];
</details> <details> <summary>wled00/button.cpp (2)</summary> `77-77`: **Good refactoring: Using global palette function instead of strip-specific method** This change aligns with the PR objective of moving custom palettes out of the WS2812FX class. Using the new global `getPaletteCount()` function instead of `strip.getPaletteCount()` improves modularity and maintainability. --- `229-230`: **Good refactoring: Consistent use of global palette function** Similar to the previous change, these lines now properly use the global `getPaletteCount()` function. The implementation also correctly preserves the validation logic with `constrain()` to ensure palette indices remain valid. </details> <details> <summary>wled00/wled_server.cpp (1)</summary> `179-179`: **Good refactoring: Using global palette loading function** This change properly uses the global `loadCustomPalettes()` function instead of `strip.loadCustomPalettes()`, consistent with moving palette management out of the WS2812FX class as outlined in the PR objectives. </details> <details> <summary>usermods/Analog_Clock/Analog_Clock.cpp (2)</summary> `105-107`: **Improved color handling: Gamma correction moved to final stage** The removal of `gamma32()` calls is appropriate, as per the PR objective to move gamma adjustment to the final step before sending data to LEDs. This ensures consistent color rendering across the application and improves performance by removing redundant gamma corrections. --- `194-194`: **Consistent removal of gamma correction in commented code** Good attention to detail in also updating the currently commented out code for potential future use. This maintains consistency with the overall approach of moving gamma correction to the final output stage. </details> <details> <summary>wled00/overlay.cpp (1)</summary> `94-94`: **Improved segment handling: Using accessor method instead of direct array access** This change properly uses `strip.getSegment(i)` with a const reference instead of directly accessing the segments array. This enhancement improves: 1. Encapsulation - Segment internals are accessed through proper methods 2. Type safety - Using const reference prevents accidental modifications 3. Maintainability - Consistent with the segment layer refactoring in the PR The code is also more resilient to future changes in segment implementation. </details> <details> <summary>wled00/wled.cpp (2)</summary> `532-533`: **Performance optimization: Temporarily disabling transitions during strip initialization.** This change prevents unnecessary segment copies during initialization by temporarily disabling transitions, which is a good optimization that should reduce memory operations when setting up the strip. --- `561-562`: **Good: Restoring transition delay after initialization.** Properly restores the transition delay to its default value after completing the initialization steps, ensuring that the temporary optimization doesn't affect normal operation. </details> <details> <summary>wled00/ir.cpp (4)</summary> `428-429`: **Architecture improvement: Using global palette count accessor.** Changed from `strip.getPaletteCount()` to `getPaletteCount()` to align with the architectural change of moving custom palettes out of the WS2812FX class. This makes palette management more consistent across the codebase. --- `438-438`: **Formatting fix for preset fallback parameter alignment.** Minor formatting improvement to align parameters consistently with other similar calls. --- `487-487`: **Consistent use of global palette count accessor.** Similar to the earlier change, using `getPaletteCount()` instead of `strip.getPaletteCount()` for consistent palette management throughout the codebase. --- `533-533`: **Documentation URL update.** Updated the URL to point to the correct WLED GitHub wiki location by removing the "-dev" suffix from the organization name. </details> <details> <summary>wled00/mqtt.cpp (4)</summary> `71-72`: **Memory management improvement: Using PSRAM-aware allocation.** Replaced standard `free()` with `w_free()` and `malloc()` with `w_malloc()` which are PSRAM-aware allocation wrappers. This change improves memory management by potentially utilizing PSRAM when available, which can help prevent heap fragmentation and memory issues on ESP32 devices. --- `97-97`: **Consistent use of PSRAM-aware memory deallocation.** Using `w_free()` instead of standard `free()` for consistent memory management throughout the codebase. --- `127-127`: **Consistent use of PSRAM-aware memory deallocation.** Using `w_free()` instead of standard `free()` for consistent memory management throughout the codebase. --- `199-200`: **Improved MQTT client allocation using PSRAM.** Using placement new with `w_malloc()` to allocate the AsyncMqttClient in potentially more abundant PSRAM memory. The comment correctly notes that this client will never be deleted, avoiding memory issues if the client were to be re-created. </details> <details> <summary>wled00/data/index.htm (3)</summary> `271-272`: **UI improvement: Better HTML structure for blend mode selector.** Replaced paragraph tag with a div using class "sel-p" and updated the select element to use class "sel-ple", improving the structure and styling consistency of the UI. --- `273-292`: **Enhanced blend modes selection with new 2D options.** Added six new 2D blend modes to the transition blend options UI: - "Swipe TL", "Swipe TR", "Swipe BR", "Swipe BL" (diagonal swipes) - "Circular Out" and "Circular In" (circular transitions) Also renamed "Pinch-out" to the more intuitive "Outside-in" for better user understanding. These additions align with the PR objective of implementing Photoshop-style segment and layer blending and providing more transition options. --- `366-366`: **Documentation URL update.** Updated GitHub URL by removing the "-dev" suffix from the organization name for consistency with other URL updates in this PR. </details> <details> <summary>usermods/audioreactive/audio_reactive.cpp (4)</summary> `1739-1740`: **References updated to use global customPalettes.** This code now references the global `customPalettes` vector instead of the previous `strip.customPalettes`. This change aligns with the PR's refactoring that moves custom palettes out of the WS2812FX class. --- `1967-1973`: **Palette management moved to global scope.** The `removeAudioPalettes()` method now operates on the global `customPalettes` vector instead of `strip.customPalettes`. This centralizes palette management and reduces coupling between the audio reactive component and the strip implementation. --- `1976-1986`: **Palette creation updated to use global palette store.** The `createAudioPalettes()` method has been updated to use the global `customPalettes` vector. The change maintains the same functionality (adding custom palettes) while integrating with the new centralized palette management system. --- `2048-2048`: **Dynamic gradient palette loading now uses global palette collection.** This line now loads the dynamic gradient palette into the global `customPalettes` vector instead of `strip.customPalettes`, completing the refactoring of palette management. </details> <details> <summary>usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (4)</summary> `399-406`: **Palette sorting now uses global palette management.** The code has been updated to use global `getPaletteCount()` and `customPalettes` instead of strip-specific methods. This change: 1. Adds better debug output showing both the total palette count and custom palette count separately 2. Ensures consistent integration with the new palette management system 3. Maintains the same functionality for marking and sorting palettes --- `412-412`: **Sort function now uses global palette count function.** Updated to use `getPaletteCount()` and `customPalettes.size()` for determining the number of palettes to sort, maintaining compatibility with the new global palette management system. --- `701-707`: **Current palette finding utilizes global palette count.** The iteration range has been updated to use `getPaletteCount()+customPalettes.size()` to ensure all palettes (including custom ones) are considered when finding the current palette. --- `891-891`: **Palette index bounds calculation updated.** Changed to use global `getPaletteCount()` and `customPalettes.size()` for calculating the bounds of the palette index when changing palettes, ensuring compatibility with the new palette management system. </details> <details> <summary>wled00/file.cpp (2)</summary> `395-397`: **Memory management improved with PSRAM-aware wrappers.** Replaced standard `free()` with `w_free()`, which is likely a wrapper that handles deallocation properly for both standard RAM and PSRAM, improving memory management on ESP32 platforms. --- `406-407`: **PSRAM-preferential allocation for preset cache.** Changed from standard `malloc()` to `w_malloc()`, which is a memory allocation wrapper that prefers PSRAM on ESP32 when available. This is particularly beneficial for storing large preset data, and aligns with the PR's focus on improving memory management. </details> <details> <summary>wled00/wled.h (2)</summary> `605-607`: **New global palette management variables added** The introduction of a global `customPalettes` vector and `paletteBlend` variable improves the modularity of palette management by moving palette storage out of the WS2812FX class. This aligns with the PR objectives of better effect transitions and blending. --- `617-617`: **New color wheel option added** Adding the `useRainbowWheel` flag provides users with an alternative color wheel option. This is a good enhancement that increases flexibility in color selection without disrupting existing functionality. </details> <details> <summary>wled00/presets.cpp (7)</summary> `32-33`: **Improved synchronization with strip updates** Changed to use `strip.getFrameTime()` for calculating wait time instead of a fixed value, and replaced `yield()` with `delay(1)` in the wait loop. This provides more accurate timing based on the actual strip update cycle, which should help prevent glitches when accessing the filesystem. --- `60-63`: **Memory management improvements with PSRAM-aware allocation** Replaced standard C memory functions with PSRAM-aware wrappers (`w_malloc`/`w_free`). This change enables better memory management, especially on ESP32 which has external SPI RAM (PSRAM). --- `80-82`: **Consistent memory management with PSRAM-aware deallocation** Using `w_free` for deallocation ensures consistency with the allocation system, preventing potential memory leaks when using PSRAM. --- `168-171`: **ESP32-specific timing improvements** Similar wait loop changes for ESP32S2/ESP32C3 platforms ensure consistent behavior across different ESP32 variants. The dynamic frame time calculation is especially important for these platforms which may have different timing characteristics. --- `206-207`: **Consistent PSRAM-aware memory deallocation** Another instance of replacing `free` with `w_free` to maintain consistent memory management. --- `220-222`: **PSRAM-aware memory allocation for name storage** Using `w_malloc` for allocating the name storage buffers ensures they can potentially use PSRAM on ESP32, which helps manage limited internal RAM. --- `267-271`: **Consistent PSRAM-aware memory deallocation for API calls** Using `w_free` for deallocation in the API call path maintains consistency with the PSRAM-aware allocation system. </details> <details> <summary>wled00/data/settings_leds.htm (2)</summary> `328-329`: **Updated GPIO pin validation constraint** Reduced the maximum allowed GPIO pin number by 1 to align with updated hardware constraints. This prevents users from selecting invalid pin numbers. --- `902-903`: **Added UI controls for new palette features** Moved "Use harmonic Random Cycle palette" to the Advanced section and added a new "Use 'rainbow' color wheel" checkbox to expose the new global `useRainbowWheel` setting. This ensures the UI reflects all available functionality. </details> <details> <summary>wled00/colors.cpp (2)</summary> `211-219`: **Removed gamma correction from palette generation** The removal of gamma correction (`gamma32`) in the palette generation process is likely part of the PR's change to move gamma adjustment to the final step before sending data to LEDs, as mentioned in the PR objectives. This should result in more consistent color handling. --- `235-281`: **Added custom palette loading function** Implemented the `loadCustomPalettes()` function to load palette files from the filesystem. This function supports two different palette formats, provides good error handling, and populates the global `customPalettes` vector. The implementation is robust and aligns with the PR's goals of improving palette management. Two minor suggestions for future improvement: 1. Consider adding a return value to indicate success/failure 2. Add documentation for the supported palette formats </details> <details> <summary>wled00/xml.cpp (4)</summary> `297-298`: **Type safety improvements for bus iteration.** Changed from int to size_t for iteration variable and added const qualifier to Bus pointer, which are both good practices for improved type safety. --- `382-383`: **Palette management and UI improvements.** Two changes here: 1. Using global `paletteBlend` variable instead of `strip.paletteBlend`, which aligns with moving palette management out of the WS2812FX class. 2. Added a new checkbox for the rainbow color wheel mode. These changes align with the PR objectives for better palette handling and effect transitions. --- `669-671`: **Improved panel dimension access.** Changed from direct integer property access (`strip.panels`) to a size method call (`strip.panel.size()`), which suggests panel handling has been refactored to use a proper collection instead of simple counters. --- `673-677`: **Improved panel iteration.** Changed from using an integer counter to accessing the panel collection size and updated type to unsigned for array indices. This change is consistent with the panel handling refactoring and improves type safety. </details> <details> <summary>wled00/e131.cpp (7)</summary> `41-45`: **Simplified conditional for realtime override handling.** Cleaner and more direct check for realtime override status, making the code more readable and maintainable. --- `152-153`: **Simplified conditional check for realtime override.** Early return pattern simplifies the code and makes the control flow clearer. --- `164-165`: **Simplified conditional check for realtime override.** Consistent with other similar changes, using the early return pattern for better readability. --- `228-237`: **Direct segment property assignment.** Changed from using bit flags and `setOption()` calls to direct property assignments on segment objects. This change reflects a redesign of the Segment class API to expose properties directly rather than using bit manipulation, making the code more readable and maintainable. --- `271-273`: **Added const qualifier to is4Chan variable.** Good practice to mark this as const since its value doesn't change throughout its scope. --- `303-304`: **Simplified conditional check for realtime override.** Consistent with other similar changes in the file, using early return for cleaner control flow. --- `521-522`: **Improved version string formatting.** Using `versionString` variable instead of direct version number insertion makes the code more maintainable and centralizes version information. </details> <details> <summary>wled00/FX.cpp (5)</summary> `6095-6096`: **LGTM: Improved rotation handling for scrolling text** The rotate value is now properly mapped from SEGMENT.custom3 with a range of -2 to 2, allowing more rotation options. --- `6134-6134`: **LGTM: Added support for #DAYL token** Good addition of the #DAYL token for displaying the full day name, expanding text formatting options. --- `6168-6179`: **Improved gradient color handling logic** The new implementation simplifies trail rendering and provides clearer logic for gradient color selection based on segment flags. --- `6184-6184`: **LGTM: Updated drawCharacter call with rotation parameter** The function call has been updated to use the new rotation mapping, which aligns with the changes in Hunk 2. --- `6189-6189`: **LGTM: Updated effect metadata string** The metadata string has been updated, presumably to reflect changes in the effect parameters. </details> <details> <summary>wled00/data/index.js (8)</summary> `38-41`: **Updated holiday dates to stay current** The holiday list has been updated to remove past Easter dates (2024, 2025) and add future ones (2026, 2027), with 2028 commented out for future use. This is good maintenance to keep the seasonal themes relevant. --- `61-61`: **Performance improvement in isEmpty function** The `isEmpty` function has been rewritten to use a more efficient implementation that exits early when a property is found, rather than creating an array of all keys first. ```diff -function isEmpty(o) {return Object.keys(o).length === 0;} +function isEmpty(o) {for (const i in o) return false; return true;}
809-828: Added UI for segment blend modesNew blend mode selection UI has been implemented with 16 options, providing more advanced layer/segment blending capabilities. This aligns with the PR objective to implement "Photoshop-style segment and layer blending."
1474-1499: Improved segment capability detection logicThe segment state reading logic has been refactored to use an object approach for tracking the first selected segment, resulting in more robust capability detection. The code now properly handles cases when no segments are selected by showing a toast notification.
2355-2360: Added blend mode control functionThe new
setBm()function handles blend mode changes by sending the selected blend mode index for a segment via JSON request. This provides the necessary backend communication for the UI blend mode selector.
2785-2785: Fixed GitHub wiki URLCorrected the GitHub wiki URL in
openGH()by removing the-devsuffix, ensuring users are directed to the current documentation.
3178-3215: Improved tooltip function formattingThe tooltip function has been reformatted with consistent indentation while maintaining the same functionality. This improves code readability.
445-446: Removed unnecessary WebSocket commentA comment about WebSocket URL formatting was removed as it was unnecessary.
wled00/palettes.h (7)
1-2: Standardized include guardsThe include guards have been updated to use a more consistent format. This is a good practice for header file organization.
13-17: Modernized palette data types and updated sourcesAll palette color data arrays have been changed from
const byte[]toconst uint8_t[]for more explicit type clarity. The URLs for palette sources have also been updated to point to a consistent repository at seaviewsensing.com.Also applies to: 21-26, 30-38, 42-47
213-222: Added new fierce-ice paletteA new gradient palette "fierce-ice_gp" has been added, providing additional visual options. This enhances the variety of available effects.
655-658: Added traffic light paletteA new trafficlight_gp palette has been added with black, green, yellow, and red colors, mimicking a traffic light sequence.
660-665: Added Aurora2 paletteA new Aurora2_gp palette has been added with a sequence of colors transitioning from greenish to purple, expanding the available visual options.
682-742: Updated gradient palette array with new entriesThe gGradientPalettes array has been updated to include the newly added palettes and correctly reference all palette data arrays as uint8_t pointers.
457-475: Adjusted C9 color transition pointsThe color transition points in C9-related palettes (C9_2_gp and C9_new_gp) have been fine-tuned with comments clarifying start positions. This improves the visual appearance of these palettes.
wled00/bus_manager.h (8)
120-127: Standardized return types to size_tVirtual methods like
getPins(),getNumberOfChannels(), andgetBusSize()now consistently usesize_tas their return type instead ofunsigned. This improves type safety and consistency, especially on 64-bit platforms where size_t may be larger than unsigned.
160-161: Standardized static method return typesStatic methods like
getNumberOfPins()andgetNumberOfChannels()have been updated to usesize_treturn type for consistency with their instance method counterparts.
246-246: Consistent return types in BusDigital classReturn types for
getPins()andgetBusSize()inBusDigitalclass were changed tosize_tfor consistency with the base class.Also applies to: 252-252
292-294: Consistent return types in BusPwm classReturn types for
getPins()andgetBusSize()inBusPwmclass were changed tosize_tfor consistency with the base class.
320-321: Consistent return types in BusOnOff classReturn types for
getPins()andgetBusSize()inBusOnOffclass were changed tosize_tfor consistency with the base class.
341-344: Improved BusNetwork method declarationsReturn types for
getPins()andgetBusSize()inBusNetworkclass were changed tosize_t, and theshow()andcleanup()methods were explicitly marked with theoverridekeyword for better code clarity.
372-372: Removed doubleBuffer parameter from BusConfig constructorThe
doubleBufferparameter was removed from the constructor, indicating a design change in the buffer management approach. This aligns with the PR objective to remove the global LED buffer in favor of segment-local buffers.
411-411: Updated memUsage return typeThe return type of
memUsage()method was changed tosize_tfor better type safety and consistency with other memory-related functions.wled00/udp.cpp (1)
978-985: Hard-codedMAX_NUM_SEGMENTSis inconsistent with new dynamic limitMost of this file was migrated to
WS2812FX::getMaxSegments(), but the ESP-NOW re-assembly code still compares againstMAX_NUM_SEGMENTS.
If a board supports more segments than that legacy macro, packets for higher IDs will be discarded:if (segsReceived >= MAX_NUM_SEGMENTS) { … }Replace with the dynamic getter (and adjust buffer sizes if necessary):
- if (segsReceived >= MAX_NUM_SEGMENTS) { + if (segsReceived >= WS2812FX::getMaxSegments()) {The same constant is used a few lines below – update all occurrences.
wled00/util.cpp (1)
623-651: Verify availability ofheap_caps_*_prefer()APIThe ESP-IDF “prefer” variants (
heap_caps_malloc_prefer,heap_caps_realloc_prefer,heap_caps_calloc_prefer) were added in later IDF versions.
Building with an older Arduino-ESP32 core (< 2.0.8) will fail.If backward-compatibility is required, fall back to classic
heap_caps_malloc()when the prefer APIs are missing, e.g.:#if ESP_ARDUINO_VERSION_MAJOR >= 3 || defined(ESP_IDF_VERSION_MAJOR) && ESP_IDF_VERSION_MAJOR >= 4 return heap_caps_malloc_prefer(size, 2, caps1, caps2); #else return heap_caps_malloc(size, caps1); // no preference API #endifPlease double-check CI targets.
wled00/const.h (2)
409-410: New error code collides with existing enumeration holes?
ERR_NORAM_PXis inserted as value 7. Previous codes jumped from 4→8; tools or scripts might already rely on 5-7 being “reserved”.
Double-check for downstream assumptions (e.g. UI strings table) and document the new code in client-facing enums.
47-71:❓ Verification inconclusive
Risk of overshooting analog-channel limit on some ESP32 variants
WLED_MAX_ANALOG_CHANNELSis derived fromLEDC_CHANNEL_MAX * LEDC_SPEED_MODE_MAX, which equals16on most ESP32 targets.
However, the commented-out per-variant constants (// #define WLED_MAX_ANALOG_CHANNELS 6, …) suggest the intention is to reserve fewer channels on C3/S2/S3.
Keeping the generic “16” may:
- reserve RAM for non-existent PWM channels,
- mis-report capabilities in the UI/JSON,
- cause
WLED_MAX_BUSSES(below) to exceed real hardware limits.Consider re-defining the macro inside each
#elifbranch or adding a static‐assert that caps it to the true number of LEDC channels per target.
Review analog-channel capacity for ESP32 variants
The generic
#define WLED_MAX_ANALOG_CHANNELS (LEDC_CHANNEL_MAX * LEDC_SPEED_MODE_MAX)may overestimate available PWM channels on ESP32-C3/S2/S3 if their
LEDC_CHANNEL_MAXis lower than on the original ESP32. Please:• Confirm each target’s true
LEDC_CHANNEL_MAXandLEDC_SPEED_MODE_MAXvalues.
• OverrideWLED_MAX_ANALOG_CHANNELSinside the#elif defined(CONFIG_IDF_TARGET_…)branches when needed.
• Optionally add astatic_assertor#errorto catch configurations where the computed channel count exceeds real hardware limits.Location to review:
- wled00/const.h, lines 47–71
wled00/fcn_declare.h (1)
493-497: Parameter type drift – update documentation
parseNumber,updateVal, etc., switched fromString&to C-strings.
Ensure all call-sites were updated; mismatches silently selectStringoverloads in other headers.wled00/cfg.cpp (1)
161-169: Risk of emptypanelvector when onlympcis supplied
isMatrixis set to true andstrip.panel.clear()is called unconditionally, but when the JSON lacks thepanelsarray the subsequent loop never pushes anyPanelobjects.
Down-stream code (e.g.FX_2Dfcn.cpp, text rendering) frequently accessespanel[0]without re-checking the size, which can dereferencenullptrand crash.Suggested minimal guard:
- if (!panels.isNull()) { - strip.panel.reserve(numPanels); + if (!panels.isNull() && panels.size() > 0) { + strip.panel.reserve(numPanels); ... + } else { + // create a single default panel so that vector is never empty + strip.panel.emplace_back(); + }Alternatively you could bail out with
strip.isMatrix = false;if no panel information is provided.
Please verify which behaviour matches user expectations.
| if ((e131_data[dataOffset+5] & 0b00110000) >> 4 != seg.map1D2D) { | ||
| seg.map1D2D = (e131_data[dataOffset+5] & 0b00110000) >> 4; | ||
| } |
There was a problem hiding this comment.
Corrected bit shift operation for map1D2D.
Changed the bit operation from division by 8 to a right shift by 4, which is more appropriate for extracting bits 4-5 from the byte.
The previous code was likely calculating an incorrect value. The right shift by 4 (>> 4) properly extracts bits 4-5 from the mask 0b00110000.
| waitForIt(); | ||
|
|
||
| customMappingSize = 0; // prevent use of mapping if anything goes wrong | ||
|
|
||
| if (customMappingTable) free(customMappingTable); | ||
| customMappingTable = static_cast<uint16_t*>(malloc(sizeof(uint16_t)*getLengthTotal())); | ||
| d_free(customMappingTable); | ||
| customMappingTable = static_cast<uint16_t*>(d_malloc(sizeof(uint16_t)*getLengthTotal())); // prefer to not use SPI RAM | ||
|
|
There was a problem hiding this comment.
Check customMappingTable allocation before dereferencing
d_malloc() may legally return nullptr on low-RAM boards; the code currently dereferences customMappingTable (lines 60-65) unconditionally right after the if (customMappingTable) block, which only sets customMappingSize.
Either move the filling loop inside the existing if (customMappingTable) or add an early return when allocation fails to avoid a null‐pointer write.
| return true; | ||
| } | ||
| //DEBUG_PRINTF_P(PSTR("-- Allocating data (%d): %p\n", len, this); | ||
| deallocateData(); // if the old buffer was smaller release it first |
There was a problem hiding this comment.
I dont fully understand the new mem handlng yet but: is it useful to keep memory reserved that cannot be used by the caller?
There was a problem hiding this comment.
Consider heap fragmentation and the tradeoff of keeping data allocated.
|
I played around with it a bit:
Edit: Edit2: |
|
I hope I have addressed all important issues. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
wled00/FX.h (2)
109-110:MIN_SHOW_DELAYrelies on an un-scoped identifierThe macro accesses
_frametime, a private member ofWS2812FX.
Outside class scope this is undefined and can unexpectedly capture a different symbol or fail to compile.Consider turning it into a private constexpr method on
WS2812FXinstead of a global macro.
That also avoids name pollution and honours encapsulation.
537-544: Inline lambdas insetPixelColorXYRawhurt code sizeThe two one-liner helper lambdas are instantiated every time the functions are called.
A simple arithmetic expression is cheaper and allows the compiler to inline trivially:- auto XY = [](unsigned X, unsigned Y){ return X + Y*Segment::vWidth(); }; - pixels[XY(x,y)] = c; + pixels[x + y * Segment::vWidth()] = c;Same applies to
getPixelColorXYRaw.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
wled00/FX.h(21 hunks)wled00/FX_2Dfcn.cpp(18 hunks)wled00/bus_manager.cpp(17 hunks)wled00/const.h(5 hunks)wled00/fcn_declare.h(5 hunks)wled00/file.cpp(12 hunks)wled00/mqtt.cpp(4 hunks)wled00/pin_manager.cpp(1 hunks)wled00/pin_manager.h(0 hunks)wled00/presets.cpp(7 hunks)wled00/util.cpp(4 hunks)wled00/wled.cpp(3 hunks)wled00/wled.h(3 hunks)
💤 Files with no reviewable changes (1)
- wled00/pin_manager.h
✅ Files skipped from review due to trivial changes (3)
- wled00/pin_manager.cpp
- wled00/mqtt.cpp
- wled00/file.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- wled00/presets.cpp
- wled00/wled.h
- wled00/wled.cpp
- wled00/util.cpp
- wled00/fcn_declare.h
- wled00/FX_2Dfcn.cpp
- wled00/bus_manager.cpp
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- 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 (nodemcuv2_160)
🔇 Additional comments (1)
wled00/const.h (1)
77-84:WLED_MAX_BUSSEScompile-time guard looks goodNice to see the
static_assertthat guarantees the sum of digital + analog buses never silently exceeds 32 – that directly addresses the concern raised in an earlier review. 👍
| #define WLED_MAX_PANELS 18 // must not be more than 32 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add the same safety net for WLED_MAX_PANELS
WLED_MAX_PANELS is documented as “must not be more than 32”, but unlike WLED_MAX_BUSSES it is not protected by a static_assert.
A future change (or a board definition that bumps the value) will compile happily and only fail at runtime.
+#if WLED_MAX_PANELS > 32
+#error "WLED_MAX_PANELS exceeds 32 – this breaks bit-field layouts"
+#endif
+// Or, if you prefer a style consistent with BUSSES:
+static_assert(WLED_MAX_PANELS <= 32, "WLED_MAX_PANELS exceeds hard limit");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define WLED_MAX_PANELS 18 // must not be more than 32 | |
| #define WLED_MAX_PANELS 18 // must not be more than 32 | |
| +#if WLED_MAX_PANELS > 32 | |
| +#error "WLED_MAX_PANELS exceeds 32 – this breaks bit-field layouts" | |
| +#endif | |
| +// Or, if you prefer a style consistent with BUSSES: | |
| +static_assert(WLED_MAX_PANELS <= 32, "WLED_MAX_PANELS exceeds hard limit"); |
There was a problem hiding this comment.
This is unoverridable macro so no need for guard.
There was a problem hiding this comment.
@blazoncek I understand your point - since WLED_MAX_PANELS is defined as a fixed constant (18) with no redefinition mechanism in the code (unlike other macros that have #ifdef/#undef patterns), it can't be overridden by users or through build flags. The value is hardcoded and will never exceed 32, making a static assertion redundant in this case.
✏️ Learnings added
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.579Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| /* How much data bytes each segment should max allocate to leave enough space for other segments, | ||
| assuming each segment uses the same amount of data. 256 for ESP8266, 640 for ESP32. */ | ||
| #define FAIR_DATA_PER_SEG (MAX_SEGMENT_DATA / strip.getMaxSegments()) | ||
| #define FAIR_DATA_PER_SEG (MAX_SEGMENT_DATA / WS2812FX::getMaxSegments()) | ||
|
|
There was a problem hiding this comment.
Macro uses a class that isn’t declared yet – breaks the build
FAIR_DATA_PER_SEG is defined before the WS2812FX class exists, so the first translation unit that includes FX.h fails with: error: ‘WS2812FX’ has not been declared.
#define FAIR_DATA_PER_SEG (MAX_SEGMENT_DATA / WS2812FX::getMaxSegments())Move the macro below the class definition or, better, turn it into an inline constexpr helper:
// after WS2812FX definition
inline constexpr unsigned fairDataPerSeg() {
return MAX_SEGMENT_DATA / WS2812FX::getMaxSegments();
}| ~Segment() { | ||
| #ifdef WLED_DEBUG | ||
| //Serial.printf("-- Destroying segment: %p", this); | ||
| //if (name) Serial.printf(" %s (%p)", name, name); | ||
| //if (data) Serial.printf(" %d->(%p)", (int)_dataLen, data); | ||
| //Serial.println(); | ||
| DEBUGFX_PRINTF_P(PSTR("-- Destroying segment: %p [%d,%d:%d,%d]"), this, (int)start, (int)stop, (int)startY, (int)stopY); | ||
| if (name) DEBUGFX_PRINTF_P(PSTR(" %s (%p)"), name, name); | ||
| if (data) DEBUGFX_PRINTF_P(PSTR(" %u->(%p)"), _dataLen, data); | ||
| DEBUGFX_PRINTF_P(PSTR(" T[%p]"), _t); | ||
| DEBUGFX_PRINTLN(); | ||
| #endif | ||
| if (name) { free(name); name = nullptr; } | ||
| stopTransition(); | ||
| clearName(); | ||
| deallocateData(); | ||
| d_free(pixels); | ||
| } | ||
|
|
There was a problem hiding this comment.
Memory leak: transition object never destroyed
Segment::~Segment() frees name, data, and pixels but forgets _t (pointer to the active Transition).
If a segment is deleted while a transition is still in progress the heap block stays leaked.
- clearName();
- deallocateData();
- d_free(pixels);
+ clearName();
+ deallocateData();
+ if (_t) { delete _t; _t = nullptr; }
+ d_free(pixels);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ~Segment() { | |
| #ifdef WLED_DEBUG | |
| //Serial.printf("-- Destroying segment: %p", this); | |
| //if (name) Serial.printf(" %s (%p)", name, name); | |
| //if (data) Serial.printf(" %d->(%p)", (int)_dataLen, data); | |
| //Serial.println(); | |
| DEBUGFX_PRINTF_P(PSTR("-- Destroying segment: %p [%d,%d:%d,%d]"), this, (int)start, (int)stop, (int)startY, (int)stopY); | |
| if (name) DEBUGFX_PRINTF_P(PSTR(" %s (%p)"), name, name); | |
| if (data) DEBUGFX_PRINTF_P(PSTR(" %u->(%p)"), _dataLen, data); | |
| DEBUGFX_PRINTF_P(PSTR(" T[%p]"), _t); | |
| DEBUGFX_PRINTLN(); | |
| #endif | |
| if (name) { free(name); name = nullptr; } | |
| stopTransition(); | |
| clearName(); | |
| deallocateData(); | |
| d_free(pixels); | |
| } | |
| ~Segment() { | |
| #ifdef WLED_DEBUG | |
| DEBUGFX_PRINTF_P(PSTR("-- Destroying segment: %p [%d,%d:%d,%d]"), this, (int)start, (int)stop, (int)startY, (int)stopY); | |
| if (name) DEBUGFX_PRINTF_P(PSTR(" %s (%p)"), name, name); | |
| if (data) DEBUGFX_PRINTF_P(PSTR(" %u->(%p)"), _dataLen, data); | |
| DEBUGFX_PRINTF_P(PSTR(" T[%p]"), _t); | |
| DEBUGFX_PRINTLN(); | |
| #endif | |
| clearName(); | |
| deallocateData(); | |
| if (_t) { | |
| delete _t; | |
| _t = nullptr; | |
| } | |
| d_free(pixels); | |
| } |
Segment layering & effect blending improvements
Fulfils #4550
Fixes: #4610 #4603 (may) #4553 #4520 #4140 #3712 #3417
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
User Interface
Refactor
Chores