Adding usermod palettes and fix UI palette display#5548
Merged
Conversation
Contributor
WalkthroughAdds a usermod-scoped palette registry ( ChangesPalette ID space, plumbing, UI, and AudioReactive usermod integration
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Server as WLED Server
participant Colors as Colors Module
participant UM as Usermod (AudioReactive)
UI->>Server: GET /json/info or /json/palettes
Server->>Colors: serializeInfo()/serializePalettes()
Colors->>UM: read `usermodPalettes` entries
Colors-->>Server: JSON (fixed → usermod → custom, umpalcount, umpalnames)
Server-->>UI: palette JSON
UI->>UI: render usermod + custom palettes (validate cache pcount+vid)
UI->>Server: DELETE palette request
Server->>Colors: removeUsermodPalettes(name) or custom removal
Colors->>UM: remove matching usermod palette entries
Colors-->>Server: updated palette state
Server-->>UI: delayed refresh (client clears cached wledPalx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/index.js (1)
2831-2854:⚠️ Potential issue | 🟠 MajorThe new
palcountcache key still leaves same-session palette previews stale.
d.pcountonly invalidateslocalStorageon a fresh load. OncepalettesDatais set, Line 2832 returns early forever, so enabling/disabling usermod palettes or adding/removing custom palettes after connect never refetches/json/palx; the preview gradients stay stale until a full page reload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/index.js` around lines 2831 - 2854, The early-return "if (palettesData) return resolve();" causes same-session palette changes to be ignored; instead, validate the in-memory cache against the stored cache and current device metadata (lastinfo.vid and lastinfo.palcount) and only skip fetching if they match. Update the logic around palettesData/localStorage.getItem("wledPalx") to: if palettesData exists and the cached meta (d.vid and d.pcount) equals lastinfo.vid and lastinfo.palcount then resolve immediately; otherwise clear/refetch by calling getPalettesData(0, ...) and then set localStorage and call redrawPalPrev. Ensure the check uses the same symbols (palettesData, lastinfo.vid, lastinfo.palcount, getPalettesData, redrawPalPrev, and "wledPalx") so enabling/disabling usermod or adding palettes during a session triggers a refresh.
🧹 Nitpick comments (2)
usermods/audioreactive/audio_reactive.cpp (1)
2196-2199: Minor: comment example doesn't matchpalIndexstarting at 0.The loop pushes
palIndex = 0, 1, 2, so the first generated name will be"AudioReactive 0", not"AudioReactive 1"as the comment suggests. Tweak the comment for accuracy.📝 Proposed comment fix
- // reuse _name ("AudioReactive") as base; palIndex 0/1/2... builds "AudioReactive 1" etc. + // reuse _name ("AudioReactive") as base; palIndex 0/1/2... builds "AudioReactive 0", "AudioReactive 1", ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 2196 - 2199, Comment mismatch: the loop pushing entries into usermodnPalettes uses (uint8_t)i as the palette index so names will be "AudioReactive 0", "AudioReactive 1", ... not starting at 1; update the inline comment near the usermodnPalettes.push_back call (and any mention of palIndex) to accurately state that palIndex starts at 0 and the first generated name will be "AudioReactive 0".wled00/colors.h (1)
64-70: Minor: comment typos and stale example.
- "palette name name will be um_name + index" — duplicated word "name".
- Example reads "AudioReactive 1" but with
palIndexstarting at 0 the first generated label is "AudioReactive 0".📝 Proposed comment fix
// Palette registered by a usermod at fixed IDs (255, 254, 253... 201) -// palette name name will be um_name + index (e.g. "AudioReactive 1"), see util.cpp +// Palette name will be um_name + palIndex (e.g. "AudioReactive 0"), see util.cpp. struct UsermodnPalette { CRGBPalette16 palette; const char *name; // PROGMEM base name string (must not be nullptr) - uint8_t palIndex; // index of the palette for a usermod + uint8_t palIndex; // index of the palette for a usermod };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.h` around lines 64 - 70, Fix the comment on the UsermodnPalette struct: remove the duplicated word "name", clarify construction of the palette name (it's um_name + index as a suffix) and correct the example to reflect that palIndex starts at 0 (e.g., "AudioReactive 0"); update the comment near the struct UsermodnPalette (referencing fields palette, name, palIndex and util.cpp/um_name) so it accurately documents that name is a PROGMEM base name and the generated label uses palIndex starting from 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/index.js`:
- Around line 992-1023: The dynamic palette sections append usermod/custom rows
after requestJson() has already called updateSelectedPalette(selectedPal), so if
the active palette lives in those newly-created ID ranges it isn't visually
selected; fix by reapplying the selection after building those lists—i.e., after
the usermod loop and the custom palette loop (where generateListItemHtml,
genPalPrevCss, 'pallist', lastinfo and palettesData are used), call
updateSelectedPalette(selectedPal) (or the existing selection-reapply helper) so
the active item is visually marked.
In `@wled00/FX_fcn.cpp`:
- Around line 236-242: The palette validation block that checks pal against
FIXED_PALETTE_COUNT/WLED_CUSTOM_PALETTE_ID_BASE/WLED_USERMOD_PALETTE_ID_BASE
using custCount/umCount must be moved to after the code that resolves pal == 0
to _default_palette (the code path from setMode()), so that the resolved
_default_palette is also validated before indexing customPalettes or
usermodPalettes; update the same duplicated guard in the other function (the one
around lines 278–281) as well, ensuring pal (or resolved _default_palette) is
clamped to 0 when out of range before any access to customPalettes or
usermodPalettes.
In `@wled00/util.cpp`:
- Around line 320-329: The code dereferences usermodnPalettes[umIdx] without
checking that umIdx is within usermodnPalettes.size(), which can read past the
vector when a usermod is disabled or exposes fewer palettes; update the
JSON_palette_names branch where umIdx is computed (using
WLED_USERMOD_PALETTE_ID_BASE - mode) to validate that umIdx <
usermodnPalettes.size() before accessing usermodnPalettes[umIdx], and if out of
range use a safe fallback (e.g., empty or placeholder base name and palIndex 0)
when calling snprintf to format dest so no invalid indexing occurs.
---
Outside diff comments:
In `@wled00/data/index.js`:
- Around line 2831-2854: The early-return "if (palettesData) return resolve();"
causes same-session palette changes to be ignored; instead, validate the
in-memory cache against the stored cache and current device metadata
(lastinfo.vid and lastinfo.palcount) and only skip fetching if they match.
Update the logic around palettesData/localStorage.getItem("wledPalx") to: if
palettesData exists and the cached meta (d.vid and d.pcount) equals lastinfo.vid
and lastinfo.palcount then resolve immediately; otherwise clear/refetch by
calling getPalettesData(0, ...) and then set localStorage and call
redrawPalPrev. Ensure the check uses the same symbols (palettesData,
lastinfo.vid, lastinfo.palcount, getPalettesData, redrawPalPrev, and "wledPalx")
so enabling/disabling usermod or adding palettes during a session triggers a
refresh.
---
Nitpick comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 2196-2199: Comment mismatch: the loop pushing entries into
usermodnPalettes uses (uint8_t)i as the palette index so names will be
"AudioReactive 0", "AudioReactive 1", ... not starting at 1; update the inline
comment near the usermodnPalettes.push_back call (and any mention of palIndex)
to accurately state that palIndex starts at 0 and the first generated name will
be "AudioReactive 0".
In `@wled00/colors.h`:
- Around line 64-70: Fix the comment on the UsermodnPalette struct: remove the
duplicated word "name", clarify construction of the palette name (it's um_name +
index as a suffix) and correct the example to reflect that palIndex starts at 0
(e.g., "AudioReactive 0"); update the comment near the struct UsermodnPalette
(referencing fields palette, name, palIndex and util.cpp/um_name) so it
accurately documents that name is a PROGMEM base name and the generated label
uses palIndex starting from 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4bc47a8-f688-42ad-8329-585d2d0c31c0
📒 Files selected for processing (10)
usermods/audioreactive/audio_reactive.cppwled00/FX_fcn.cppwled00/colors.cppwled00/colors.hwled00/const.hwled00/data/cpal/cpal.htmwled00/data/index.jswled00/json.cppwled00/util.cppwled00/wled.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
236-244:⚠️ Potential issue | 🔴 CriticalValidate
_default_paletteafter resolvingpal == 0.The guard at Lines 236-242 still runs before Line 244 replaces
palwith_default_palette, so a stale default palette in the custom/usermod range bypasses validation and can still drive the unchecked indexing at Lines 278-281.Suggested fix
- const int umCount = usermodPalettes.size(); - const int custCount = customPalettes.size(); - if (pal >= FIXED_PALETTE_COUNT) { - if (pal > WLED_CUSTOM_PALETTE_ID_BASE) { // usermod range (IDs 201-255) - if ((WLED_USERMOD_PALETTE_ID_BASE - pal) >= umCount) pal = 0; - } else { // custom range - if ((WLED_CUSTOM_PALETTE_ID_BASE - pal) >= custCount) pal = 0; - } - } //default palette. Differs depending on effect if (pal == 0) pal = _default_palette; // _default_palette is set in setMode() + const int umCount = usermodPalettes.size(); + const int custCount = customPalettes.size(); + if (pal >= FIXED_PALETTE_COUNT) { + if (pal > WLED_CUSTOM_PALETTE_ID_BASE) { // usermod range (IDs 201-255) + if ((WLED_USERMOD_PALETTE_ID_BASE - pal) >= umCount) pal = 0; + } else { // custom range + if ((WLED_CUSTOM_PALETTE_ID_BASE - pal) >= custCount) pal = 0; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 236 - 244, After assigning pal = _default_palette when pal==0, re-run the same validation logic against _default_palette to ensure it doesn't fall into the custom/usermod ranges unchecked; specifically, after the line that sets pal = _default_palette, apply the same checks used above (compare pal against FIXED_PALETTE_COUNT, then handle ranges using WLED_USERMOD_PALETTE_ID_BASE, WLED_CUSTOM_PALETTE_ID_BASE and the counts umCount/custCount) and reset pal to 0 (or another safe fallback) if the default is out of bounds to prevent invalid indexing later in the code.
🧹 Nitpick comments (2)
wled00/colors.h (1)
64-69: Verify the struct nameUsermodnPalette— looks like a typo.The lowercase
nbetweenUsermodandPalettereads as a typo. If it stands for "named" or "numbered", considerUsermodNamedPalette/UsermodNumberedPalette; otherwiseUsermodPalettewould be more idiomatic and matches the surroundingusermodPalettesvector. Renaming now (before this lands and ripples to public usermod-facing examples) is much cheaper than later.♻️ Suggested rename
-struct UsermodnPalette { +struct UsermodPalette { CRGBPalette16 palette; const char *name; // PROGMEM base name string (must not be nullptr), this name is used in removeusermodPalettes() uint8_t palIndex; // index of the palette for a usermod }; @@ -extern std::vector<UsermodnPalette> usermodPalettes; +extern std::vector<UsermodPalette> usermodPalettes;Mirror in
wled00/wled.h(WLED_GLOBALdeclaration) and any usermod call sites.As per coding guidelines: "Use PascalCase for classes and structs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.h` around lines 64 - 69, The struct name UsermodnPalette appears to be a typo; rename it to a clearer PascalCase name (e.g., UsermodPalette or UsermodNamedPalette) and update all references (the struct declaration containing CRGBPalette16 palette, const char *name, uint8_t palIndex) accordingly, including the WLED_GLOBAL declaration in wled.h and any uses in the usermodPalettes vector or usermod call sites and helper utilities (e.g., util.cpp) so symbol names remain consistent across the codebase and public usermod examples.wled00/data/index.js (1)
992-1023: LGTM — usermod & custom palette appending matches the firmware ID layout.The descending IDs (255-j and 200-j) match
wled00/json.cpp(lines 971-980) andwled00/const.h(WLED_USERMOD_PALETTE_ID_BASE/WLED_CUSTOM_PALETTE_ID_BASE). The!isEmpty(li)guard correctly prevents this code from running on the initialloadPalettes()call (beforerequestJson()populateslastinfo), and the gray-placeholder skip behavior is preserved. TheupdateSelectedPalette(selectedPal)call at line 1024 correctly addresses the previous review feedback.Minor optional cleanup: the
createElement('div') → appendChild → outerHTML =dance can be replaced withgId('pallist').insertAdjacentHTML('beforeend', generateListItemHtml(...)), which avoids the throwaway wrapper element. Two near-identical loops could also be deduplicated into a small helper. Not blocking — current behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/index.js` around lines 992 - 1023, Replace the throwaway-div pattern (createElement + appendChild + div.outerHTML =) in both loops with gId('pallist').insertAdjacentHTML('beforeend', generateListItemHtml(...)) to append items directly; keep the same arguments used with generateListItemHtml (palette, id, label, 'setPalette', genPalPrevCss(id) HTML) and preserve the gray-placeholder skip using palettesData and the li checks; optionally factor the repeated logic into a small helper (e.g., renderPaletteListItem(id, label)) that calls generateListItemHtml and insertAdjacentHTML to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/colors.h`:
- Line 72: The function name removeusermodPalettes is inconsistent with
camelCase; rename it to removeUsermodPalettes across the codebase. Update the
declaration in colors.h (change removeusermodPalettes to removeUsermodPalettes),
the definition in wled00/colors.cpp (function implementation name), and all call
sites (e.g., wled00/colors.cpp references and
usermods/audioreactive/audio_reactive.cpp) to use removeUsermodPalettes; ensure
any forward declarations, headers, and usages are updated and recompiled to fix
linker/reference errors.
---
Duplicate comments:
In `@wled00/FX_fcn.cpp`:
- Around line 236-244: After assigning pal = _default_palette when pal==0,
re-run the same validation logic against _default_palette to ensure it doesn't
fall into the custom/usermod ranges unchecked; specifically, after the line that
sets pal = _default_palette, apply the same checks used above (compare pal
against FIXED_PALETTE_COUNT, then handle ranges using
WLED_USERMOD_PALETTE_ID_BASE, WLED_CUSTOM_PALETTE_ID_BASE and the counts
umCount/custCount) and reset pal to 0 (or another safe fallback) if the default
is out of bounds to prevent invalid indexing later in the code.
---
Nitpick comments:
In `@wled00/colors.h`:
- Around line 64-69: The struct name UsermodnPalette appears to be a typo;
rename it to a clearer PascalCase name (e.g., UsermodPalette or
UsermodNamedPalette) and update all references (the struct declaration
containing CRGBPalette16 palette, const char *name, uint8_t palIndex)
accordingly, including the WLED_GLOBAL declaration in wled.h and any uses in the
usermodPalettes vector or usermod call sites and helper utilities (e.g.,
util.cpp) so symbol names remain consistent across the codebase and public
usermod examples.
In `@wled00/data/index.js`:
- Around line 992-1023: Replace the throwaway-div pattern (createElement +
appendChild + div.outerHTML =) in both loops with
gId('pallist').insertAdjacentHTML('beforeend', generateListItemHtml(...)) to
append items directly; keep the same arguments used with generateListItemHtml
(palette, id, label, 'setPalette', genPalPrevCss(id) HTML) and preserve the
gray-placeholder skip using palettesData and the li checks; optionally factor
the repeated logic into a small helper (e.g., renderPaletteListItem(id, label))
that calls generateListItemHtml and insertAdjacentHTML to avoid duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 042906d8-0485-499d-9775-423c66d61248
📒 Files selected for processing (8)
usermods/audioreactive/audio_reactive.cppwled00/FX_fcn.cppwled00/colors.cppwled00/colors.hwled00/data/index.jswled00/json.cppwled00/util.cppwled00/wled.h
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/wled.h
- usermods/audioreactive/audio_reactive.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
usermods/audioreactive/audio_reactive.cpp (1)
1956-1962: Optional: simplify the capacity guard.
createAudioPalettes()already enforces the per-entry capacity check on eachpush_back, so the additional(int)(usermodPalettes.size() + MAX_PALETTES) <= WLED_MAX_USERMOD_PALETTEShere is redundant — and slightly stricter than needed (it refuses to re-create any palettes if there isn't room for allMAX_PALETTES). The(int)cast on asize_tsum is also unnecessary. Given today's configuration this is unreachable, but the redundancy is worth tidying.♻️ Proposed simplification
void onStateChange(uint8_t callMode) override { - if (initDone && enabled && addPalettes && palettes==0 - && (int)(usermodPalettes.size() + MAX_PALETTES) <= WLED_MAX_USERMOD_PALETTES) { + if (initDone && enabled && addPalettes && palettes == 0) { // if palettes were removed during JSON call re-add them createAudioPalettes(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1956 - 1962, The onStateChange method contains a redundant and overly strict capacity guard: remove the "(int)(usermodPalettes.size() + MAX_PALETTES) <= WLED_MAX_USERMOD_PALETTES" check and the cast inside onStateChange (function onStateChange) so it simply checks initDone, enabled, addPalettes and palettes==0 then calls createAudioPalettes(); rely on createAudioPalettes() and its per-push_back capacity checks to enforce limits for usermodPalettes and MAX_PALETTES instead of pre-checking here.wled00/colors.cpp (1)
315-322: LGTM — minor polish on the loop counter type.The reverse-iteration approach correctly avoids the index-shift issue from
erase()and the pointer-identity comparison matches the documented contract onname. One nit:int i = usermodPalettes.size() - 1relies on thesize_t(-1) → int(-1)narrowing when the vector is empty (implementation-defined), and while it works in practice, an unsigned counter avoids the wart.♻️ Optional polish
-size_t removeUsermodPalettes(const char *name) { - size_t before = usermodPalettes.size(); - for (int i = usermodPalettes.size() - 1; i >= 0; i--) { - if (usermodPalettes[i].name == name) - usermodPalettes.erase(usermodPalettes.begin() + i); - } - return before - usermodPalettes.size(); -} +size_t removeUsermodPalettes(const char *name) { + const size_t before = usermodPalettes.size(); + for (size_t i = before; i-- > 0;) { + if (usermodPalettes[i].name == name) + usermodPalettes.erase(usermodPalettes.begin() + i); + } + return before - usermodPalettes.size(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 315 - 322, In removeUsermodPalettes, avoid using a signed int that underflows when the vector is empty; change the loop counter to an unsigned size type (size_t) and iterate backwards in a safe pattern (start from usermodPalettes.size() and decrement while checking bounds, using i-1 as the index) so erase() still works correctly; keep the existing comparison usermodPalettes[i].name == name and the return value logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1956-1962: The onStateChange method contains a redundant and
overly strict capacity guard: remove the "(int)(usermodPalettes.size() +
MAX_PALETTES) <= WLED_MAX_USERMOD_PALETTES" check and the cast inside
onStateChange (function onStateChange) so it simply checks initDone, enabled,
addPalettes and palettes==0 then calls createAudioPalettes(); rely on
createAudioPalettes() and its per-push_back capacity checks to enforce limits
for usermodPalettes and MAX_PALETTES instead of pre-checking here.
In `@wled00/colors.cpp`:
- Around line 315-322: In removeUsermodPalettes, avoid using a signed int that
underflows when the vector is empty; change the loop counter to an unsigned size
type (size_t) and iterate backwards in a safe pattern (start from
usermodPalettes.size() and decrement while checking bounds, using i-1 as the
index) so erase() still works correctly; keep the existing comparison
usermodPalettes[i].name == name and the return value logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0dfb1e84-daa8-4712-ad66-9fcb5c3ff810
📒 Files selected for processing (3)
usermods/audioreactive/audio_reactive.cppwled00/colors.cppwled00/colors.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/colors.h`:
- Line 75: getPaletteCount() no longer equals "max valid palette ID + 1" because
IDs are non-contiguous (fixed, custom, usermod ranges), so callers that do
things like updateVal(..., 0, getPaletteCount()-1) (e.g., paletteChanged in
set.cpp using updateVal) must stop treating the return as a contiguous upper
bound; change those call sites to validate against the actual ID ranges (fixed:
0..FIXED_PALETTE_COUNT-1, custom:
FIXED_PALETTE_COUNT..FIXED_PALETTE_COUNT+custCount-1 or up to 200, usermod:
201..255 or using umCount) or split the input path to separately clamp/accept
fixed vs custom vs usermod IDs (follow the pattern used in FX_fcn.cpp which
checks umCount/custCount separately) so valid custom/usermod IDs are not
silently clamped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af06a7fa-620a-4c26-a455-3c75cdc6dafc
📒 Files selected for processing (3)
wled00/colors.hwled00/util.cppwled00/wled.h
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled.h
Collaborator
Author
|
@softhack007 this PR is kind of mendatory for 16.0 release, without it, AR palettes mess with the ID numbering of palettes of the now "unrestricted" amount of custom palettes. It takes a fully modular approach by splitting custom palettes and usermod palettes into their own ID space, it is a different "divergence" from MM but it may also be less messy to merge once we move to the out of tree AR version - not sure you would want to adopt this approach into MM but please have a look. cc @netmindz |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
usermods/audioreactive/audio_reactive.cpp (1)
2184-2188: Removal accounting: cast is safe today, but a bit fragile.
removeUsermodPalettes()returnssize_t; casting toint8_twould wrap if it ever exceeds 127. Today it can't (MAX_PALETTES = 3), and the post-clamp at Line 2187 hides any glitch, so this is fine. Minor stylistic suggestion: do the arithmetic in a wider type to remove the clamp altogether and avoid masking real bugs in caseMAX_PALETTESchanges later.♻️ Optional cleanup
- palettes -= (int8_t)removeUsermodPalettes(_name); - if (palettes < 0) palettes = 0; // safeguard + const size_t removed = removeUsermodPalettes(_name); + palettes = (removed >= (size_t)palettes) ? 0 : (int8_t)(palettes - removed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 2184 - 2188, The subtraction currently casts removeUsermodPalettes(_name) to int8_t which can wrap if the returned size_t ever exceeds 127; instead compute the removal in a wider signed type and then adjust palettes safely: call removeUsermodPalettes(_name) into a size_t or int variable (e.g., removed), subtract that from palettes using a signed conversion, and clamp palettes to >= 0 (and optionally <= MAX_PALETTES) without relying on an int8_t cast; update the removeAudioPalettes function to use these symbols: removeAudioPalettes, removeUsermodPalettes, and palettes.wled00/util.cpp (1)
320-340: Bounds check now in place; minor consistency nits.The previously‑flagged out‑of‑bounds dereference of
usermodPalettesis now guarded at Line 324 — good.Two small, optional refinements:
- The custom branch at Line 337 uses
snprintf_P(dest, maxLen, ...)(size =maxLen) while the new usermod branch usessnprintf(dest, maxLen + 1, ...)(size =maxLen + 1). Both produce a null‑terminated string within the caller'sbuffersize = maxLen + 1buffer, but mixing the two conventions in the same function is easy to misread later — consider aligning them (bothmaxLen + 1, since that matches the actual buffer size).- The format string
"%s %u"is a RAM literal; usingsnprintf_PwithPSTR("%s %u")would be consistent with thesnprintf_Pcall just below and save a few bytes on ESP8266.♻️ Optional tweak
- snprintf(dest, maxLen + 1, "%s %u", base, (unsigned)ump.palIndex); // palette name is um name + index (e.g. "AudioReactive 1") + snprintf_P(dest, maxLen + 1, PSTR("%s %u"), base, (unsigned)ump.palIndex); // palette name is um name + index (e.g. "AudioReactive 1") return strlen(dest); } if (mode >= FIXED_PALETTE_COUNT && mode <= WLED_CUSTOM_PALETTE_ID_BASE) { // user custom palette (IDs FIXED_PALETTE_COUNT up to WLED_CUSTOM_PALETTE_ID_BASE=200) - snprintf_P(dest, maxLen, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode); - dest[maxLen] = '\0'; + snprintf_P(dest, maxLen + 1, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` around lines 320 - 340, Align the two branches to use the same buffer-size convention and place format strings in flash: change snprintf(dest, maxLen + 1, "%s %u", (unsigned)...) in the usermod-palette branch to snprintf_P(dest, maxLen + 1, PSTR("%s %u"), base, (unsigned)ump.palIndex) (referencing usermodPalettes and ump.name), and change the custom-palette snprintf_P call to pass maxLen + 1 as the size (snprintf_P(dest, maxLen + 1, PSTR("~ Custom %d ~"), ...)) so both branches consistently use the full caller buffer size; keep the existing explicit null-termination logic as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 2184-2188: The subtraction currently casts
removeUsermodPalettes(_name) to int8_t which can wrap if the returned size_t
ever exceeds 127; instead compute the removal in a wider signed type and then
adjust palettes safely: call removeUsermodPalettes(_name) into a size_t or int
variable (e.g., removed), subtract that from palettes using a signed conversion,
and clamp palettes to >= 0 (and optionally <= MAX_PALETTES) without relying on
an int8_t cast; update the removeAudioPalettes function to use these symbols:
removeAudioPalettes, removeUsermodPalettes, and palettes.
In `@wled00/util.cpp`:
- Around line 320-340: Align the two branches to use the same buffer-size
convention and place format strings in flash: change snprintf(dest, maxLen + 1,
"%s %u", (unsigned)...) in the usermod-palette branch to snprintf_P(dest, maxLen
+ 1, PSTR("%s %u"), base, (unsigned)ump.palIndex) (referencing usermodPalettes
and ump.name), and change the custom-palette snprintf_P call to pass maxLen + 1
as the size (snprintf_P(dest, maxLen + 1, PSTR("~ Custom %d ~"), ...)) so both
branches consistently use the full caller buffer size; keep the existing
explicit null-termination logic as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5f1544a-46cc-4085-ae50-2bf1aaf9b930
📒 Files selected for processing (4)
usermods/audioreactive/audio_reactive.cppwled00/colors.hwled00/util.cppwled00/wled.h
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/wled.h
- wled00/colors.h
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/util.cpp (1)
337-338: UsemaxLen + 1consistently for the custom placeholder branch.This path is safe today, but it unnecessarily trims one extra character versus the rest of
extractModeName()behavior.♻️ Proposed tweak
- snprintf_P(dest, maxLen, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode); + snprintf_P(dest, maxLen + 1, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode); dest[maxLen] = '\0';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` around lines 337 - 338, The custom placeholder branch in extractModeName() calls snprintf_P(dest, maxLen, ...) and then sets dest[maxLen] = '\0', which trims one character compared to other branches; change the snprintf_P call to use maxLen + 1 (snprintf_P(dest, maxLen + 1, ...)) and keep null-termination consistent by ensuring the terminator index matches (i.e., dest[maxLen] = '\0' remains correct after using maxLen + 1). Update references to WLED_CUSTOM_PALETTE_ID_BASE and the variables dest/maxLen accordingly so this branch matches the buffer sizing used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/util.cpp`:
- Around line 337-338: The custom placeholder branch in extractModeName() calls
snprintf_P(dest, maxLen, ...) and then sets dest[maxLen] = '\0', which trims one
character compared to other branches; change the snprintf_P call to use maxLen +
1 (snprintf_P(dest, maxLen + 1, ...)) and keep null-termination consistent by
ensuring the terminator index matches (i.e., dest[maxLen] = '\0' remains correct
after using maxLen + 1). Update references to WLED_CUSTOM_PALETTE_ID_BASE and
the variables dest/maxLen accordingly so this branch matches the buffer sizing
used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c90ca8c-0736-4530-a926-5cc87650b5ba
📒 Files selected for processing (4)
usermods/audioreactive/audio_reactive.cppwled00/colors.hwled00/util.cppwled00/wled.h
Collaborator
Author
|
@softhack007 any objections on merging this? |
Member
|
Does it need to be any more complex than this? |
Member
|
just trying to test and can't actually see either the custom nor usermod show up, don't see any errors in the console either |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1960-1963: The current guard only calls createAudioPalettes() when
palettes == 0 which leaves partially-created palettes stuck; change the
condition to detect incomplete palettes and retry creation (e.g., use palettes <
WLED_MAX_USERMOD_PALETTES or a similar “not full” check) so
createAudioPalettes() runs whenever addPalettes is true and the palette count is
below the configured maximum; apply the same fix to the other similar check
around createAudioPalettes() later in the file so both spots use palettes <
WLED_MAX_USERMOD_PALETTES (or equivalent) instead of palettes == 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 734b3981-f62f-4457-becb-9d916ff45098
📒 Files selected for processing (3)
usermods/audioreactive/audio_reactive.cppwled00/colors.hwled00/util.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/colors.h
Comment on lines
+1960
to
1963
| if (initDone && enabled && addPalettes && palettes==0) { | ||
| // if palettes were removed during JSON call re-add them | ||
| createAudioPalettes(); | ||
| } |
Contributor
There was a problem hiding this comment.
Partial palette creation can get stuck and never backfill.
When WLED_MAX_USERMOD_PALETTES is temporarily near full, this can create only some AR palettes, set palettes > 0, and then permanently skip adding the missing ones (if (palettes) return; + palettes == 0 gate).
Suggested fix
void AudioReactive::createAudioPalettes(void) {
- if (palettes) return;
+ bool present[MAX_PALETTES] = {false};
+ palettes = 0;
+ for (const auto &ump : usermodPalettes) {
+ if (ump.name != _name || ump.palIndex >= MAX_PALETTES) continue;
+ if (!present[ump.palIndex]) {
+ present[ump.palIndex] = true;
+ palettes++;
+ }
+ }
+ if (palettes >= MAX_PALETTES) return;
DEBUG_PRINTLN(F("Adding audio palettes."));
static const char *const palNames[MAX_PALETTES] PROGMEM = {_palName0, _palName1, _palName2};
- for (int i=0; i<MAX_PALETTES; i++) {
+ for (uint8_t i = 0; i < MAX_PALETTES; i++) {
+ if (present[i]) continue;
if (usermodPalettes.size() < WLED_MAX_USERMOD_PALETTES) {
usermodPalettes.push_back({CRGBPalette16(CRGB(BLACK)), _name, (uint8_t)i, palNames[i]}); // start black, filled each loop by fillAudioPalettes()
palettes++;
DEBUG_PRINTLN(palettes);
} else break;
}
}
void onStateChange(uint8_t callMode) override {
- if (initDone && enabled && addPalettes && palettes==0) {
+ if (initDone && enabled && addPalettes && palettes < MAX_PALETTES) {
// if palettes were removed during JSON call re-add them
createAudioPalettes();
}
}Also applies to: 2194-2203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/audioreactive/audio_reactive.cpp` around lines 1960 - 1963, The
current guard only calls createAudioPalettes() when palettes == 0 which leaves
partially-created palettes stuck; change the condition to detect incomplete
palettes and retry creation (e.g., use palettes < WLED_MAX_USERMOD_PALETTES or a
similar “not full” check) so createAudioPalettes() runs whenever addPalettes is
true and the palette count is below the configured maximum; apply the same fix
to the other similar check around createAudioPalettes() later in the file so
both spots use palettes < WLED_MAX_USERMOD_PALETTES (or equivalent) instead of
palettes == 0.
Palette display names now follow the format "UMName: palName" (e.g. "AudioReactive: Audio Responsive Hue") when a usermod supplies a specific palName, keeping the usermod name as a consistent prefix. Fallback when palName is nullptr remains "UMName index". Update AudioReactive palette names to the full descriptive names from the original WLED-MM port: "Audio Responsive Ratio", "Audio Responsive Hue", and "Audio Responsive Spectrum".
Since the display format is now "UMName: palName", shorten the palette names to just "Ratio", "Hue", and "Spectrum" — rendering as "AudioReactive: Ratio" etc. rather than "AudioReactive: Audio Responsive Ratio".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/util.cpp`:
- Around line 344-349: The snprintf_P call in the custom-palette branch uses
maxLen as the size but the buffer is maxLen+1 (others use maxLen+1), and then
redundantly writes dest[maxLen] = '\0'; fix by passing maxLen + 1 to snprintf_P
(like the other branches) and remove the manual NUL assignment; update the call
involving snprintf_P(dest, maxLen + 1, PSTR("~ Custom %d ~"),
WLED_CUSTOM_PALETTE_ID_BASE - mode) and drop the dest[maxLen] = '\0' line so
snprintf_P handles termination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 772d168f-965c-4f2b-b0e7-a66ce4745605
📒 Files selected for processing (2)
wled00/colors.hwled00/util.cpp
✅ Files skipped from review due to trivial changes (1)
- wled00/colors.h
Comment on lines
+344
to
+349
| if (mode >= FIXED_PALETTE_COUNT && mode <= WLED_CUSTOM_PALETTE_ID_BASE) { | ||
| // user custom palette (IDs FIXED_PALETTE_COUNT up to WLED_CUSTOM_PALETTE_ID_BASE=200) | ||
| snprintf_P(dest, maxLen, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode); | ||
| dest[maxLen] = '\0'; | ||
| return strlen(dest); | ||
| } |
Contributor
There was a problem hiding this comment.
snprintf_P size argument is off-by-one — inconsistent with the rest of the function.
Line 346 passes maxLen as the buffer size, while the actual buffer is maxLen + 1 bytes. Lines 337 and 340 both correctly pass maxLen + 1. The discrepancy means this path can write one fewer character than the buffer permits. For the ~ Custom %d ~ template the result is ≤ 15 chars in practice, so no truncation occurs today, but it's a correctness inconsistency. The redundant dest[maxLen] = '\0' on line 347 can also be removed once the size is corrected (snprintf always NUL-terminates within its size argument).
🛠️ Proposed fix
- snprintf_P(dest, maxLen, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode);
- dest[maxLen] = '\0';
+ snprintf_P(dest, maxLen + 1, PSTR("~ Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/util.cpp` around lines 344 - 349, The snprintf_P call in the
custom-palette branch uses maxLen as the size but the buffer is maxLen+1 (others
use maxLen+1), and then redundantly writes dest[maxLen] = '\0'; fix by passing
maxLen + 1 to snprintf_P (like the other branches) and remove the manual NUL
assignment; update the call involving snprintf_P(dest, maxLen + 1, PSTR("~
Custom %d ~"), WLED_CUSTOM_PALETTE_ID_BASE - mode) and drop the dest[maxLen] =
'\0' line so snprintf_P handles termination.
Collaborator
Author
we can roll with this for now, I was thinking of adding naming feature for other custom palettes too but did not look into it. Since UM and custom are now independent arrays, there should not be any conflict when adding that feature at a later point. |
Member
|
picked into 16_x |
This was referenced May 10, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Usermods get their own palette "space" right after the built-in palettes, named by the usermod name (for example "AudioReactive 0". Custom palettes now start at palette ID 200 and downwards, keeping IDs consistent to not break presets when enabling/disabling AR palettes.
Also added a fix to reload the palette gradients when the number of total palettes changes so the palette list should now always properly update.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
UI