Fix for #5201 #5215
Open
Fix for #5201 #5215
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
Contributor
WalkthroughRefactors palette handling: increments Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)wled00/**/*.cpp📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (12)📓 Common learnings📚 Learning: 2025-11-14T13:37:11.994ZApplied to files:
📚 Learning: 2025-08-28T08:09:20.630ZApplied to files:
📚 Learning: 2025-11-14T13:37:30.955ZApplied to files:
📚 Learning: 2025-08-26T11:51:21.817ZApplied to files:
📚 Learning: 2025-10-10T18:34:06.550ZApplied to files:
📚 Learning: 2025-12-15T20:25:25.807ZApplied to files:
📚 Learning: 2025-11-16T19:40:46.260ZApplied to files:
📚 Learning: 2025-10-05T15:24:05.545ZApplied to files:
📚 Learning: 2025-08-31T03:38:14.114ZApplied to files:
📚 Learning: 2025-11-27T06:33:11.436ZApplied to files:
📚 Learning: 2025-09-16T18:08:42.848ZApplied to files:
⏰ 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). (20)
🔇 Additional comments (9)
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 (2)
wled00/json.cpp (2)
966-1001: Critical: Loop bounds and JSON key calculation bugs will cause incorrect behavior or crashes.Several issues with the palette serialization logic:
Loop condition
i <= end(line 966): When there are no custom palettes,end = palettesCount = 72. The loop iterates withi = 72, which triggers the custom palette branch (line 996:i >= palettesCount), attempting to accesscustomPalettes[0]on an empty vector — undefined behavior or crash.JSON key calculation (line 967): The condition
i <= palettesCountis true wheni = 72, producing key72. However, palette index 72 is the first custom palette, which should have key255, not72. Custom palettes use keys counting down from 255.Off-by-one in custom palette key formula: When
i = 73(second custom palette), the formula255 - (i - (palettesCount + 1))=255 - (73 - 73)=255, but it should be254.🔎 Proposed fix
- for (int i = start; i <= end; i++) { - JsonArray curPalette = palettes.createNestedArray(String(i<=palettesCount ? i : 255 - (i - (palettesCount + 1)))); + for (int i = start; i < end; i++) { + JsonArray curPalette = palettes.createNestedArray(String(i < palettesCount ? i : 255 - (i - palettesCount)));The fixes:
- Change
i <= endtoi < endto prevent iterating past valid palette indices- Change
i <= palettesCounttoi < palettesCountto correctly identify custom palettes starting at index 72- Change formula from
255 - (i - (palettesCount + 1))to255 - (i - palettesCount)for correct custom palette keys
954-961: Minor:maxPagecalculation may produce an extra empty page.The formula change from
(count - 1) / itemPerPagetocount / itemPerPagecan result in an extra page when the total count is exactly divisible byitemPerPage.For example, with 72 fixed palettes and 0 custom palettes using 8 items per page:
- Old:
(72 - 1) / 8 = 8→ pages 0-8 (9 pages, last page has 8 items)- New:
72 / 8 = 9→ pages 0-9 (10 pages, page 9 would be empty)If the loop bounds issue above is fixed to use
i < end, this becomes a minor UI issue (empty last page) rather than a crash, but it's still incorrect.🔎 Proposed fix
- int maxPage = (palettesCount + customPalettesCount) / itemPerPage; + int maxPage = (palettesCount + customPalettesCount - 1) / itemPerPage;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(1 hunks)wled00/FX_fcn.cpp(2 hunks)wled00/const.h(1 hunks)wled00/json.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.h
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX_fcn.cppwled00/json.cpp
🧠 Learnings (17)
📓 Common learnings
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
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: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
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: 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.
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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
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.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/const.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
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/const.hwled00/json.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
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/const.hwled00/json.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/const.hwled00/FX_fcn.cppwled00/json.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/const.hwled00/FX_fcn.cpp
📚 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/const.hwled00/FX_fcn.cppwled00/json.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/const.hwled00/FX_fcn.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/const.hwled00/FX_fcn.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/const.hwled00/FX_fcn.cppwled00/json.cpp
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
wled00/const.h
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/const.h
📚 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/const.h
📚 Learning: 2025-12-15T20:25:25.807Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-15T20:25:25.807Z
Learning: The CSS palette preview in wled00/data/index.js genPalPrevCss() function uses raw RGB values in CSS linear-gradient() without applying WLED's gamma correction, while actual LED output goes through NeoGammaWLEDMethod gamma correction. This causes inherent discrepancies between the web UI palette preview and actual LED colors, especially noticeable with different gamma settings.
Applied to files:
wled00/FX_fcn.cppwled00/json.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_fcn.cppwled00/json.cpp
📚 Learning: 2025-11-14T05:48:44.673Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
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_fcn.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
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.
Applied to files:
wled00/json.cpp
🧬 Code graph analysis (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
wled00/colors.h (1)
getPaletteCount(127-127)
🪛 Clang (14.0.6)
wled00/const.h
[error] 9-9: unknown type name 'constexpr'
(clang-diagnostic-error)
[error] 9-9: expected ';' after top level declarator
(clang-diagnostic-error)
[error] 10-10: unknown type name 'constexpr'
(clang-diagnostic-error)
[error] 10-10: expected ';' after top level declarator
(clang-diagnostic-error)
[error] 11-11: unknown type name 'constexpr'
(clang-diagnostic-error)
[error] 11-11: expected ';' after top level declarator
(clang-diagnostic-error)
⏰ 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). (20)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Test cdata.js
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
🔇 Additional comments (4)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
404-416: LGTM! Palette handling correctly updated to useFIXED_PALETTE_COUNT.The changes properly align with the palette categorization:
- Custom palettes are now correctly inserted starting at index
FIXED_PALETTE_COUNT(72)- Sorting is limited to fixed palettes only, which prevents custom palette indices from being reordered
- The
getPaletteCount()function (fromcolors.h) correctly computes total palettes asFIXED_PALETTE_COUNT + customPalettes.size()This follows the learnings about replacing magic numbers with defined constants for maintainability.
wled00/const.h (1)
9-12: LGTM! Updated palette count constants with clear documentation.The constants now correctly reflect the palette categorization:
DYNAMIC_PALETTE_COUNT = 6covers palettes 0-5 (dynamic/color-derived)FASTLED_PALETTE_COUNT = 7covers palettes 6-12 (FastLED built-ins)GRADIENT_PALETTE_COUNT = 59covers palettes 13-71 (gradient palettes)FIXED_PALETTE_COUNT = 72is the total of all built-in palettesThe updated comments documenting the index ranges improve maintainability. Based on learnings, replacing magic numbers with these constants is the recommended approach.
Note: The static analysis warnings about
constexprbeing an "unknown type" are false positives - the analyzer appears to be running in C mode rather than C++11+ mode. These can be safely ignored.wled00/FX_fcn.cpp (2)
233-233: LGTM! Correct bounds check for out-of-range palettes.The condition
pal >= FIXED_PALETTE_COUNT && pal <= 255-customPalettes.size()correctly identifies invalid palette indices:
- Valid fixed palettes: 0 to 71 (FIXED_PALETTE_COUNT - 1)
- Valid custom palettes: 255 down to (256 - customPalettes.size())
- Gap (invalid): 72 to (255 - customPalettes.size()) → reset to default palette 0
The change from
>to>=correctly rejects palette index 72 which doesn't exist.
271-275: LGTM! FastLED and gradient palette indexing correctly uses constants.The indexing logic correctly replaces magic numbers with defined constants:
- FastLED palettes (6-12):
pal - DYNAMIC_PALETTE_COUNTmaps to indices 0-6- Gradient palettes (13-71):
pal - (DYNAMIC_PALETTE_COUNT + FASTLED_PALETTE_COUNT)maps to indices 0-58This aligns with the updated constants in
const.hand follows the project learnings about using defined constants instead of hardcoded values.
Collaborator
|
I think the rabbit is right about there now being a one-off in custom palettes, at least its logic makes sense to me |
Collaborator
|
@coderabbitai is the cusom palette logic correct now? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8224 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 996 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 3170 @DedeHai Yes, the custom palette logic appears to be correct now! Let me walk through the indexing to confirm: Custom Palette Indexing (with FIXED_PALETTE_COUNT = 72)In
|
DedeHai
approved these changes
Dec 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.