"unrestricted" number of custom palettes #4932
Conversation
- add TRANSPARENT() marco for uint32_t color conversion - allow more than 10 custom palettes via WLED_MAX_CUSTOM_PALETTES - add setColor(CRGBA) method - move palettes into CPP file - tune index.js
…er palettes - shortened names in cpal, saves about 400 bytes of Flash after packing - removed async from common.js loading to prevent errors on page loads if the file is not cached - restricted nubmer of user palettes on ESP8266 to 5 - unrestricted number of user palettes on all other platforms - added a warning when adding more than 10 palettes to let the user decide to risk it
WalkthroughRenames generated PAGE_* length symbols to PAGE_*_length, updates server static routes, converts CPAL asset generation to chunk-based output, refactors palette counting and selection with new constants and caps, removes direct palettes.h includes, exposes max custom palettes in JSON, synchronizes common.js loading, and restructures the CPAL editor UI/JS. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @DedeHai. * #4932 (comment) The following files were modified: * `tools/cdata.js` * `wled00/FX_fcn.cpp` * `wled00/colors.cpp` * `wled00/colors.h` * `wled00/data/index.js` * `wled00/json.cpp` * `wled00/util.cpp` * `wled00/wled_server.cpp`
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
enumeration was wrong, last normal palette was not loaded into UI gradient. updated the enumeration to hopefully be more readable/clear. AR palettes are now also added if there are more than 10 custom palettes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/data/cpal/cpal.htm (1)
14-14: common.js path/load order looks correct.Served at root; synchronous include is fine given prior async issues.
🧹 Nitpick comments (12)
wled00/data/cpal/cpal.htm (11)
16-115: Indent with tabs per project guideline.This file uses spaces in most HTML/CSS blocks; guidelines require tabs under wled00/data.
Example (apply across file):
- <style> - body { - font-family: Arial, sans-serif; + <style> + body { + font-family: Arial, sans-serif;
166-166: Invalid SVG style: negative stroke-width.
stroke-width: -2px;is invalid and ignored. Use a positive width or omit.-var svgTrash = '<svg viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" width="30px" height="30px"><path style="fill:#880000; stroke: #888888; stroke-width: -2px;stroke-dasharray: 0.1, 8;" d="M9,3V4H4V6H5V19A2,2 0 0,0 7,21H17A2,2 0 0,0 19,19V6H20V4H15V3H9M7,6H17V19H7V6M9,8V17H11V8H9M13,8V17H15V8H13Z"/></svg>' +var svgTrash = '<svg viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" width="30px" height="30px"><path style="fill:#880000; stroke:#888888; stroke-width:2px; stroke-dasharray:0.1,8;" d="M9,3V4H4V6H5V19A2,2 0 0,0 7,21H17A2,2 0 0,0 19,19V6H20V4H15V3H9M7,6H17V19H7V6M9,8V17H11V8H9M13,8V17H15V8H13Z"/></svg>'
173-177: recOf() is never called.Either remove it or call on load to ensure fresh metrics if layout changes.
-window.addEventListener('load', chkW); +window.addEventListener('load', () => { recOf(); chkW(); });
305-346: Drag code: shadowed globals and unused var.Avoid shadowing
rect/gLenand drop unusedposNew.-function mkDrag(el) { // makeMeDrag - var posNew=0, mPos=0; - var rect=gBox.getBoundingClientRect(); - var maxX=rect.right, minX=rect.left, gLen=maxX-minX+1; +function mkDrag(el) { // makeMeDrag + let mPos=0; + const dragRect = gBox.getBoundingClientRect(); + const maxX = dragRect.right, minX = dragRect.left, dragLen = maxX - minX + 1; @@ - var mInG=mPos-(minX+1); - var tPos=Math.round((mInG/gLen)*256); + var mInG=mPos-(minX+1); + var tPos=Math.round((mInG/dragLen)*256);
386-394: Ensure deterministic JSON order.Object key iteration may vary; sort by position.
- function calcJSON() { - let rStr='{"palette":['; - Object.entries(tCol).forEach(([p,c],i)=>{ + function calcJSON() { + let rStr='{"palette":['; + const entries = Object.keys(tCol).map(k => [Number(k), tCol[k]]).sort((a,b)=>a[0]-b[0]); + entries.forEach(([p,c],i)=>{ if (i>0) rStr+=','; rStr+=`${p},"${c.slice(1)}"`; }); rStr+=']}'; return rStr; }
400-425: XHR callbacks use incorrect this; use req.Arrow functions don’t bind
this. Logging currently readsundefined.- req.addEventListener('load', ()=>{ - console.log(this.responseText, ' - ', this.status) + req.addEventListener('load', ()=>{ + console.log(req.responseText, ' - ', req.status); @@ - req.addEventListener('error', (e)=>{ - console.log('Error: ', e); console.log(' Status: ', this.status); + req.addEventListener('error', (e)=>{ + console.log('Error: ', e); console.log(' Status: ', req.status);Optional: add timeout handling.
+ req.timeout = 15000; + req.ontimeout = () => console.warn('Upload timed out');
427-441: Remove unused variable.
var arr = [];is unused.- try { - var arr = []; + try {
469-470: alert() only takes one argument.Second parameter is ignored. Consider a toast if available.
- alert("Palette cache missing from browser. Return to main page first.","Missing cache!") + alert("Palette cache missing from browser. Return to main page first.");
575-578: Class overwrite drops .pal styles.
className = "pGradPar"removes the earlier.palclass. Add instead.- palDiv.className = "pGradPar"; + palDiv.classList.add("pGradPar");
606-621: Avoid scrolling inside loop.Scroll once after populating markers.
- addC(pos, hex); - window.scroll(0, 0); + addC(pos, hex); @@ - } + window.scroll(0, 0); + }
623-638: Shadowing global tCol in distrib().Rename local to avoid confusion.
- const tCol = e.getAttribute("data-tcol"); + const col = e.getAttribute("data-tcol"); @@ - addC(spacing * (i + 1), tCol); + addC(spacing * (i + 1), col);usermods/audioreactive/audio_reactive.cpp (1)
1742-1745: Replace lingering magic number 10 with WLED_MAX_CUSTOM_PALETTESThe re-add guard still uses a hardcoded 10, which diverges from the new global cap and can block palette re-creation unnecessarily on platforms where the limit differs.
Apply:
- if (initDone && enabled && addPalettes && palettes==0 && customPalettes.size()<10) { + if (initDone && enabled && addPalettes && palettes==0 && customPalettes.size() < WLED_MAX_CUSTOM_PALETTES) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/audioreactive/audio_reactive.cpp(1 hunks)wled00/FX_fcn.cpp(3 hunks)wled00/data/cpal/cpal.htm(8 hunks)wled00/json.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/FX_fcn.cpp
- wled00/json.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/cpal/cpal.htm
🧠 Learnings (3)
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
usermods/audioreactive/audio_reactive.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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:
usermods/audioreactive/audio_reactive.cpp
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
PR: wled/WLED#4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
wled00/data/cpal/cpal.htm
🔇 Additional comments (1)
usermods/audioreactive/audio_reactive.cpp (1)
1983-1989: Good guard: cap additions by WLED_MAX_CUSTOM_PALETTES — verified
WLED_MAX_CUSTOM_PALETTES is defined in wled00/const.h (non-ESP8266: #define WLED_MAX_CUSTOM_PALETTES (255 - FIXED_PALETTE_COUNT) — line 14; ESP8266: #define WLED_MAX_CUSTOM_PALETTES 5 — line 16). The usage in usermods/audioreactive/audio_reactive.cpp:1984 is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/const.h (2)
12-12: Consider using constexpr calculation for FIXED_PALETTE_COUNT.While the current implementation is correct, using
constexprcalculation makes the dependency more explicit and maintains consistency.-constexpr size_t FIXED_PALETTE_COUNT = DYNAMIC_PALETTE_COUNT + FASTLED_PALETTE_COUNT + GRADIENT_PALETTE_COUNT; // total number of fixed palettes +constexpr size_t FIXED_PALETTE_COUNT = DYNAMIC_PALETTE_COUNT + FASTLED_PALETTE_COUNT + GRADIENT_PALETTE_COUNT;
13-17: Consider adding static_assert for palette limit validation.Adding compile-time validation would help catch configuration errors early and document the constraints.
#ifndef ESP8266 #define WLED_MAX_CUSTOM_PALETTES (255 - FIXED_PALETTE_COUNT) // allow up to 255 total palettes, user is warned about stability issues when adding more than 10 #else #define WLED_MAX_CUSTOM_PALETTES 10 // ESP8266: limit custom palettes to 10 #endif +static_assert(FIXED_PALETTE_COUNT + WLED_MAX_CUSTOM_PALETTES <= 255, "Total palette count exceeds maximum limit of 255");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/const.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/const.h
🧠 Learnings (4)
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/const.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/const.h
🔇 Additional comments (3)
wled00/const.h (3)
9-17: LGTM! Well-structured palette constant refactoring.The changes replace a single macro with a comprehensive set of typed constants that improve maintainability and type safety. The ESP8266-specific handling for custom palette limits is appropriate given memory constraints.
14-17: Leave limits as-is. The 255 total‐palette cap aligns with an 8-bit palette ID (0–255) and subtracting the fixed palettes, and the ESP8266’s 10-slot custom-palette limit matches WLED’s JSON backend defaults and avoids the ~2 KB JSON parsing buffer and heap churn of higher counts.
9-10: Verify hardcoded palette counts match actual array sizes.FASTLED_PALETTE_COUNT (wled00/const.h:9) and GRADIENT_PALETTE_COUNT (wled00/const.h:10) must equal the element counts of fastledPalettes and gGradientPalettes (wled00/palettes.cpp — arrays defined near lines ~695 and ~709). Automated parsing failed during verification; confirm with sizeof(fastledPalettes)/sizeof(fastledPalettes[0]) and sizeof(gGradientPalettes)/sizeof(gGradientPalettes[0]) and update the constants or replace them with those sizeof expressions.
- allow more than 10 custom palettes - move palettes into CPP file - Fix for minimizing cpal.htm (saves 2k of flash) - shortened names in cpal, saves about 400 bytes of Flash after packing - removed async from common.js loading to prevent errors on page loads if the file is not cached - restricted nubmer of user palettes on ESP8266 to 10 - unrestricted number of user palettes on all other platforms (total max palettes: 256) - added a warning when adding more than 10 palettes to let the user decide to risk it - Bugfixes in palette enumeration, fixed AR palette adding - AR palettes are now also added if there are more than 10 custom palettes Co-authored-by: Blaž Kristan <blaz@kristan-sp.si>
|
Does this now allow us to name custom palettes ? |
|
no, they are still named custom0...customx. |
Cherry picked from @blazoncek fork, extended, tested and cleaned up.
asyncloading of common.js as in cpal.htm that was an issue when testing, I also found it can be an issue in settings pages. @w00000dy please comment if that was a bad idea.the proper minifying of cpal.htm and the refactoring saves a total of 2.5k of flash.
solves #4933
Summary by CodeRabbit
New Features
Bug Fixes
Refactor