FX.cpp: Update "colorful" for flexibility in # of colors from palette #5078
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of hard-coding an override to use 5 colors for C9_2, add a function to automatically detect the number of colors in the palette (up to 16), so it can be used with custom palettes of more than 4 colors. Add a slider to manually specify number of colors, and change description of saturation slider to indicate its actual function (switching between "easter" colors, default colors, and the three color slots).
Contributor
WalkthroughMode Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
1058-1061: Possible out-of-bounds writes when SEGLEN is not a multiple of numColorsInner loop writes i + j without a bound check. If SEGLEN % numColors != 0, the last block can index past segment end. Add a guard to avoid OOB writes.
Apply this diff:
- for (unsigned i = 0; i < SEGLEN; i+= numColors) - { - for (unsigned j = 0; j < numColors; j++) SEGMENT.setPixelColor(i + j, cols[SEGENV.aux0 + j]); - } + for (unsigned i = 0; i < SEGLEN; i += numColors) { + for (unsigned j = 0; j < numColors && (i + j) < SEGLEN; j++) { + SEGMENT.setPixelColor(i + j, cols[SEGENV.aux0 + j]); + } + }
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
1029-1035: Palette “non-repeating” can still duplicate non-adjacent colorsCurrent check only prevents immediate repeats. With palettes that reuse colors, later entries may still duplicate earlier ones. If the goal is unique colors up to 16, consider tracking all previously chosen colors.
Example (small footprint set since max 16):
- numColors = 1; - for (size_t i = 1; i < 16; ++i) { - uint32_t iCol = RGBW32(SEGPALETTE[i].r, SEGPALETTE[i].g, SEGPALETTE[i].b, 0); - if (iCol && (iCol != cols[numColors-1])) cols[numColors++] = iCol; - } + numColors = 1; + for (size_t i = 1; i < 16 && numColors < 16; ++i) { + uint32_t iCol = RGBW32(SEGPALETTE[i].r, SEGPALETTE[i].g, SEGPALETTE[i].b, 0); + if (!iCol) continue; + bool seen = false; + for (size_t k = 0; k < numColors; ++k) { if (cols[k] == iCol) { seen = true; break; } } + if (!seen) cols[numColors++] = iCol; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (2)
wled00/FX.cpp (2)
1015-1066: Flexible Colorful: Implementation looks good
- Cleanly supports 0..16 colors via SEGMENT.custom3 with sensible defaults for palette/slots/pastels.
- cols[31] + 2*N−1 duplication prevents wrap issues when indexing cols[aux0 + j].
Please sanity-check:
- custom3=0 with a few real palettes yields expected “auto” distinct colors on hardware.
- Intensity bands (<80 pastels, 80–160 default, >160 slots/palette) match intended UX.
1065-1066: Metadata text aligns with behavior; default matches custom3 constraintsThe updated data string (“Colors (pastels/default/color slots)” and “# of colors (0=auto); … c3=4”) reflects the new controls and sets a sensible default. No issues.
Change the logic so the 0th step isn't coded separately, only access SEGPALETTE once per loop instead of three times.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
1057-1060: Guard against out-of-bounds writes in the final block loop.The inner loop writes i + j without checking the tail remainder when SEGLEN is not a multiple of numColors. While Segment::setPixelColor likely protects, it’s better to avoid out-of-range indices up front.
Apply this diff to bound the inner loop:
- for (unsigned i = 0; i < SEGLEN; i+= numColors) - { - for (unsigned j = 0; j < numColors; j++) SEGMENT.setPixelColor(i + j, cols[SEGENV.aux0 + j]); - } + for (unsigned i = 0; i < SEGLEN; i += numColors) { + for (unsigned j = 0; j < numColors && (i + j) < SEGLEN; j++) { + SEGMENT.setPixelColor(i + j, cols[SEGENV.aux0 + j]); + } + }
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
1064-1064: Metadata text OK; consider hinting intensity behavior explicitly.The label “Colors (pastels/default/color slots)” reflects the three intensity bands (<80 pastels, 80–160 default, >160 slots/palette). For clarity in the UI, consider a shorter hint like “Color mode (pastels/default/slots)” to reduce truncation on smaller screens. Behavior is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, the ColorMap entries are limited per stop to a manageable set, so unbounded growth is not a practical concern. The colormap can be reset if lots of changing of stops is done.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
Collaborator
|
I like the addition, there are a few issues:
actual saturation could be implemented using the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
1061-1064: Fix out-of-bounds writes when SEGLEN isn’t a multiple of numColors.When numColors > SEGLEN or SEGLEN % numColors != 0,
i + jcan exceedSEGLEN-1. Guard the inner loop to prevent OOB writes.- for (unsigned i = 0; i < SEGLEN; i+= numColors) - { - for (unsigned j = 0; j < numColors; j++) SEGMENT.setPixelColor(i + j, cols[SEGENV.aux0 + j]); - } + for (unsigned i = 0; i < SEGLEN; i += numColors) { + for (unsigned j = 0; j < numColors && (i + j) < SEGLEN; j++) { + SEGMENT.setPixelColor(i + j, cols[SEGENV.aux0 + j]); + } + }
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
1030-1038: Prefer a visible fallback color when auto-palette collects none.Using black (
RGBW32(0,0,0,0)) can render “no output.” Falling back toSEGCOLOR(0)(or the default 4-color set) keeps the effect visible and consistent with user color expectations.- if (numColors == 0) { - numColors = 1; - cols[0] = RGBW32(0, 0, 0, 0); - } + if (numColors == 0) { + numColors = 1; + cols[0] = SEGCOLOR(0); + }
1025-1027: Tightenfactyping and remove unnecessary cast.
facfits in a byte for numColors ∈ [2,16]. Dropping the(int)cast clarifies intent and avoids widening.- unsigned fac = numColors > 1 ? (int)(255/(numColors - 1)) : 0; + uint8_t fac = numColors > 1 ? uint8_t(255 / (numColors - 1)) : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (15)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T13:37:11.973Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.973Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T13:37:30.945Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.945Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T05:48:44.658Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.658Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Applied to files:
wled00/FX.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (1)
wled00/FX.cpp (1)
1068-1068: Good:c3=4preserves legacy presets. Verify preset load behavior.Setting the new “# of colors” slider to 4 by default avoids changing existing Colorful presets that didn’t store
c3. Please confirm that:
- Presets saved before this PR load with
c3defaulting to 4 (not the UI midpoint), and- Live UI center-position default (16) won’t override absent
c3in older presets.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
1035-1038: Fallback to primary color instead of black when palette yields nothing.If a palette is empty/black, setting cols[0] to pure black turns the effect off. Falling back to SEGCOLOR(0) is a safer user-facing default.
- if (numColors == 0) { - numColors++; - cols[0] = RGBW32(0, 0, 0, 0); - } + if (numColors == 0) { cols[0] = SEGCOLOR(0); numColors = 1; }
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
1021-1039: Palette branch: preserve preset behavior and simplify flow (treat slider center as “auto”).
Today, in palette mode a center/default slider value (16) becomes “16 colors”, which can change the look of older presets that never set c3 and thus default to center. To avoid breaking existing presets while still enabling flexibility, treat 16 as “auto” (0) in palette mode, then keep your existing auto-detect. This makes center safe across modes and aligns with the review note about not breaking presets when new sliders are added.
Also, folding the palette handling under one block improves readability.
Suggested change:
- } else if (numColors > 0) { //get numColors of colors from palette - unsigned fac = numColors > 1 ? (int)(255/(numColors - 1)) : 0; - for (size_t i = 0; i < numColors; i++) { - cols[i] = SEGMENT.color_from_palette(i*fac, false, false, 255); - } - } else { //get all non-repeating colors from palette (up to 16) - for (uint8_t i = 0; i < 16; ++i) { - CRGB c = SEGPALETTE[i]; - uint32_t iCol = RGBW32(c.r, c.g, c.b, 0); - if (numColors == 0 || (iCol && iCol != cols[numColors-1])) cols[numColors++] = iCol; - } - if (numColors == 0) { - numColors++; - cols[0] = RGBW32(0, 0, 0, 0); - } - } + } else { // palette mode + // Treat slider center (16) as 'auto' in palette mode to keep center-default presets stable. + if (numColors == 16) numColors = 0; + if (numColors > 0) { + unsigned fac = (numColors > 1) ? (uint8_t)(255 / (numColors - 1)) : 0; + for (size_t i = 0; i < numColors; i++) { + cols[i] = SEGMENT.color_from_palette(i * fac, false, false, 255); + } + } else { // auto: collect non-repeating palette entries (up to 16) + for (uint8_t i = 0; i < 16; ++i) { + CRGB c = SEGPALETTE[i]; + uint32_t iCol = RGBW32(c.r, c.g, c.b, 0); + if (iCol && (numColors == 0 || iCol != cols[numColors - 1])) cols[numColors++] = iCol; + } + if (numColors == 0) { cols[0] = SEGCOLOR(0); numColors = 1; } // safe fallback + } + }If you prefer a different “center” sentinel than 16, we can key off whatever the UI defaults to. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (15)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, the ColorMap entries are limited per stop to a manageable set, so unbounded growth is not a practical concern. The colormap can be reset if lots of changing of stops is done.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T13:37:11.973Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.973Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T13:37:30.945Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.945Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T05:48:44.658Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.658Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Applied to files:
wled00/FX.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (2)
wled00/FX.cpp (2)
1068-1068: UI metadata is concise; consider default intent note.“# of colors” is short (good). Since presets default sliders to center (≈16), keep c3 default aligned with your code’s “safe” behavior to avoid surprises in palette mode (see palette branch note). If you adopt “center==auto” in code, c3=4 is fine to keep the out‑of‑box look consistent.
1061-1064: Segment::setPixelColor() performs defensive bounds checking; the suggested guard is optional.The implementation in FX_fcn.cpp:712-727 includes explicit bounds validation:
if (i >= vL) return;wherevL = vLength()andSEGLENis defined asSegment::vLength()(FX.h:119). This guard prevents out-of-bounds writes even when the loop's tail iteration produces indices exceeding SEGLEN.Since setPixelColor() is already defensive, the suggested
(i + j) < SEGLENguard in the inner loop is optional—the original code is safe without it.
When numColors > SEGLEN or SEGLEN % numColors != 0, i + j can exceed SEGLEN-1. Guard the inner loop to prevent OOB writes.
Contributor
Author
|
@DedeHai Thanks for the feedback.
I shortened the names. Would the details just be included in https://kno.wled.ge/features/effects/ in the "Description" column for the effect as a whole, or is there some other place where more detailed information on effects is presented?
That seems like a bug in presets that should be fixed (have them default to the default value if there is one and nothing is specified for that slider), but I understand that that's often easier said than done. Any ideas of another way to implement this without changing the current behavior of grabbing 4 evenly-spaced colors? I'd prefer not to do something unintuitive like have it interpret 16 as 4. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
1035-1038: Safer fallback when palette yields zero usable colors.If the palette is all black (or filtered out), the current fallback sets cols[0] to BLACK, which can turn the effect “off.” Prefer falling back to the primary color to keep the effect visible and consistent with user color slots.
Apply this diff:
- if (numColors == 0) { - numColors++; - cols[0] = RGBW32(0, 0, 0, 0); - } + if (numColors == 0) { + cols[0] = SEGCOLOR(0); + numColors = 1; + }This also guarantees the subsequent duplication/fill logic never sees numColors==0.
Also applies to: 1050-1050
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
1029-1034: Auto-palette “non-repeating” logic only de-duplicates consecutive entries.The check
iCol != cols[numColors-1]avoids only adjacent duplicates; palettes with repeating colors spaced apart will still duplicate. If the goal is “unique palette colors up to 16,” consider tracking a small seen bitset/hash (e.g., compare against all collected entries) to avoid non-consecutive duplicates. Keeps behavior closer to “colors present in the palette” stated in the PR.
1070-1071: UI text and defaults.
- “Color scheme” is concise and addresses the prior concern about long UI text. Good.
- Revisit
c3=4vs usingc3=0(auto) to avoid divergent defaults between new selections and legacy presets (center=16). If you keepc3=4, consider documenting the legacy-mapping in code comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (17)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.973Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.945Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T13:37:11.973Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.973Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T13:37:30.945Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.945Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T05:48:44.658Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.658Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-11T04:36:42.869Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4754
File: usermods/user_fx/README.md:169-179
Timestamp: 2025-09-11T04:36:42.869Z
Learning: In WLED, SEGENV.data is already typed as uint8_t* and can be accessed directly without casting when storing per-pixel state data.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
🔇 Additional comments (1)
wled00/FX.cpp (1)
1018-1026: Preset compatibility: center-default (16) may change palette behavior in old presets.
- Existing presets that don’t specify custom3 load sliders at center (16). Your non-palette branches normalize that to 3/4 (good), but when a palette is active the code uses all 16 colors, which may change the look of old “Colorful+palette” presets.
- To minimize surprises, consider one of:
- Treat custom3==16 as “unspecified” for this mode when palette is set and map to previous default (e.g., 4 or 5 for legacy C9_2) unless user changed it.
- Default metadata to c3=0 (auto) and document that 0 means “derive from palette,” then (optionally) special-case 16 as legacy center only if palette is active.
- Or clamp to a smaller default (e.g., 4) when custom3>=16 and palette is active, unless the user explicitly moved the slider.
Please confirm intended backward-compat behavior and whether you want to keep legacy palette looks by default.
Also applies to: 1041-1049, 1061-1066
Collaborator
that is where I would suggest to add info for FX yes. You can update the description and do a PR with a comment that it should only be merged after 0.16 is released as KB should document released versions. Also include that these new functionality is only available >0.16
all slider and option values are included in a preset, if the slider is not used, it saves the default value, which is just the slider center and "false" for unused options. So not something that is fixable unfortunately for existing presets.
rename the slider to "colors" so its not tied to a slider number but just arbitrary and map the slider such that the center is 4. I would also do the "auto" at max value and keep a minimum of 2 at a zero input value, which seems more intuitive since auto does 16 colors on most palettes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of hard-coding the "Colorful" effect to have an override to use 5 colors for C9_2, add a slider to manually specify the number of colors to select from the palette, and if that slider is set to 0, automatically detect the number of colors in the palette (up to 16). This allows it to be used with custom palettes with more than 4 colors.
Also, change description of saturation slider to indicate its actual function (switching between "easter" colors, default colors, and the three color slots). I wasn't even aware that "saturation" could allow using the color slots until I looked at the source code.
Summary by CodeRabbit
New Features
Documentation