change effect IDs from 8bit to 16bit #270
Conversation
it compiles.
WalkthroughThis PR widens effect/mode IDs from 8-bit to 16-bit across core and usermods, adds MODE_AUTO / MODE_AUTO_LEGACY, and reworks WS2812FX::addEffect to detect conflicts, resolve auto slots (including protecting legacy slot 255), append/insert slots as needed, and add debug traces. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant FX as WS2812FX::addEffect
participant Slots as Mode Vector Manager
Caller->>FX: addEffect(id:uint16, mode_fn, name)
alt id == MODE_AUTO or MODE_AUTO_LEGACY
FX->>Slots: search reserved slots (skip legacy/auto)
Slots-->>FX: reserved id? / none
alt reserved found
FX->>Slots: insert effect into reserved id
else
FX->>Slots: append new slot (may insert dummy RSVD to protect legacy 255)
end
else if id < Slots.size()
FX->>Slots: inspect slot[id]
alt slot == RSVD
FX->>Slots: insert into reserved slot
else slot occupied (non-RSVD)
FX->>FX: log conflict -> treat as MODE_AUTO
FX->>Slots: follow MODE_AUTO flow
end
else id >= Slots.size()
FX->>Slots: append new slot and assign id
end
Slots-->>Caller: return assigned id (uint16)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-22T21:50:25.799ZApplied to files:
📚 Learning: 2025-10-22T22:24:46.136ZApplied to files:
🧬 Code graph analysis (1)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (1)
⏰ 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 (1)
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 |
softhack007
changed the title
|
Thanks for making a start on this, I'll try and pick the rest up when I can |
*val is an InOut parameter
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
wled00/udp.cpp (1)
111-114: Segment sync still loses the high byteSame issue here: serializing
selseg.modeinto a single byte drops the high bits, so remote nodes can never track effects above 255. We need to carry the full 16-bit value (and bump the compatibility byte) or introduce a new packet version before rolling out wider IDs.wled00/FX.cpp (1)
11776-11786: Auto slot can still be overwrittenWhen
idisMODE_AUTO/MODE_AUTO_LEGACYand there’s no other reserved slot, the search loop leavesidunchanged. The next branch still sees_modeData[id] == _data_RESERVED, so we end up overwriting the auto slot — exactly what this guard was meant to prevent. Please bail out (or append a new slot) when no alternative slot is found instead of falling back to the auto index.wled00/util.cpp (2)
404-405: Out‑of‑bounds write: null‑terminator placed past buffer end.Use maxLen‑1 index.
- dest[maxLen] = '\0'; // strncpy does not necessarily null terminate string + dest[maxLen-1] = '\0'; // ensure termination within bounds
374-377: Dangling pointer from temporary String (use‑after‑free).names.substring(...).c_str() returns a pointer to a temporary; it’s invalid by the next statement. Keep a String alive for the copy.
- if (nameEnd<0) tmpstr = names.substring(nameBegin).c_str(); // did not find ",", last name? - else tmpstr = names.substring(nameBegin, nameEnd).c_str(); - strlcpy(dest, tmpstr, maxLen); // copy the name into buffer (replacing previous) + { + String sub = (nameEnd < 0) ? names.substring(nameBegin) + : names.substring(nameBegin, nameEnd); + strlcpy(dest, sub.c_str(), maxLen); // copy name into buffer + }
🧹 Nitpick comments (3)
wled00/FX_fcn.cpp (2)
443-455: Prefer explicit width for transition’s saved mode.auto deduces to uint16_t today, but an explicit type guards against accidental narrowing if Segment::mode changes later. Small readability win.
Apply:
- auto _modeP = mode; + uint16_t _modeP = mode;(Based on AI summary of widened types)
481-483: Mid‑transition mode switch reads well; consider clarifying the threshold.Logic is sound. For readability, compare to UINT16_MAX/2 (or 0x7FFF) instead of 32767U.
Example:
- return (progress()>32767U) ? newMode : (_t ? _t->_modeP : newMode); + return (progress() > (UINT16_MAX >> 1)) ? newMode : (_t ? _t->_modeP : newMode);wled00/util.cpp (1)
301-305: Custom palette label math still assumes 8‑bit id; negative values possible with 16‑bit.Cast to 8‑bit to preserve legacy semantics and avoid negative formatting.
- snprintf_P(dest, maxLen, PSTR("~ Custom %d ~"), 255-mode); + uint8_t idx = uint8_t(mode); // palettes remain 0..255 + snprintf_P(dest, maxLen, PSTR("~ Custom %u ~"), 255 - idx);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
wled00/FX.cpp(1 hunks)wled00/FX.h(11 hunks)wled00/FX_fcn.cpp(4 hunks)wled00/e131.cpp(1 hunks)wled00/fcn_declare.h(2 hunks)wled00/json.cpp(1 hunks)wled00/set.cpp(2 hunks)wled00/udp.cpp(2 hunks)wled00/util.cpp(7 hunks)wled00/wled.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 source and headers in wled00
Files:
wled00/e131.cppwled00/wled.hwled00/json.cppwled00/udp.cppwled00/set.cppwled00/fcn_declare.hwled00/FX.hwled00/FX.cppwled00/FX_fcn.cppwled00/util.cpp
🧬 Code graph analysis (6)
wled00/json.cpp (1)
wled00/util.cpp (2)
getVal16(78-91)getVal16(78-78)
wled00/udp.cpp (1)
wled00/data/index.js (1)
ofs(2764-2764)
wled00/set.cpp (1)
wled00/util.cpp (2)
updateVal16(103-110)updateVal16(103-103)
wled00/fcn_declare.h (1)
wled00/util.cpp (12)
parseNumber16(21-60)parseNumber16(21-21)getVal16(78-91)getVal16(78-78)updateVal16(103-110)updateVal16(103-103)extractModeName(282-335)extractModeName(282-282)extractModeSlider(339-410)extractModeSlider(339-339)extractModeDefaults(414-432)extractModeDefaults(414-414)
wled00/FX.h (2)
wled00/FX_fcn.cpp (6)
setMode(593-631)setMode(593-593)setMode(2147-2157)setMode(2147-2147)currentMode(481-483)currentMode(481-481)wled00/FX.cpp (2)
addEffect(11775-11791)addEffect(11775-11775)
wled00/util.cpp (1)
wled00/fcn_declare.h (1)
hw_random(432-432)
🔇 Additional comments (4)
wled00/FX_fcn.cpp (2)
2147-2157: WS2812FX::setMode bypasses Segment::setMode — inconsistent guards and missing stateChanged.Directly writing
_segments[segid].modeat line 2155 skips:
- RSVD guard used in Segment::setMode (line 599, checks via strncmp_P)
- Defaults loading logic (when desired, controlled by loadDefaults parameter)
- stateChanged broadcast flag (set at line 628 in Segment::setMode)
This can admit reserved entries and miss sync/broadcast side-effects. Delegate to Segment::setMode to keep behavior consistent.
Apply:
void WS2812FX::setMode(uint8_t segid, uint16_t m) { if (segid >= _segments.size()) return; if (m >= getModeCount()) m = getModeCount() - 1; - if (_segments[segid].mode != m) { - _segments[segid].startTransition(_transitionDur); // set effect transitions - //_segments[segid].markForReset(); - _segments[segid].mode = m; - } + // Delegate: ensures RSVD guard, transitions, and stateChanged handling. + _segments[segid].setMode(m, /*loadDefaults=*/false, /*sliderDefaultsOnly=*/false); }
593-631: All concerns are safely addressed through existing bounds checks; no explicit sentinel guards needed.After verifying all call sites and sentinel handling:
ID semantics:
fxis universally a 0-based mode index into_modeData[].getModeData(fx)performs direct array lookup with bounds check; no distinction between "ID" and "slot index" exists in the codebase.Call-site protection: All inputs are either pre-bounded (json.cpp, set.cpp), guarded before use (e131.cpp), maintained within bounds (effectCurrent via modulo in button.cpp), or clamped by
WS2812FX::setMode()wrapper (udp.cpp).MODE_AUTO/MODE_AUTO_LEGACY handling: Neither sentinel is passed to
setMode()anywhere in the codebase. Even if called directly with MODE_AUTO_LEGACY (255), the conditionfx < strip.getModeCount()(228) evaluates to false, safely skipping the mode-setting logic. Implicit guards are sufficient.wled00/FX.h (2)
373-376: 16‑bit mode expansion and sentinels look consistent.MODE_AUTO (65000) and MODE_AUTO_LEGACY (255) provide clear sentinels outside typical IDs; aligns with 16‑bit migration.
If any external integrations hardcode 255 as “auto slot”, confirm they now use MODE_AUTO_LEGACY or handle >255 IDs correctly.
397-398: API widened to uint16_t across modes—good coverage.Segment.mode, Transition._modeP, Segment::setMode/currentMode, WS2812FX::addEffect/getModeCount/getModeData updated coherently.
Also applies to: 487-487, 621-621, 665-665, 970-970, 1010-1010, 1036-1037, 1149-1149
wled00/udp.cpp
Outdated
| udpOut[8] = byte(mainseg.mode); // WLEDMM toDo: need workaround for 16bit mode numbers | ||
| udpOut[9] = mainseg.speed; |
There was a problem hiding this comment.
16-bit sync regression in UDP payload
With mainseg.mode now holding values above 255, sticking only the low byte into udpOut[8] means we silently truncate any effect ID ≥256. Peers receiving the notification will run a different effect once we start using the new IDs, so cross-device sync breaks. Please extend the notifier format (and its decoder) to ship both bytes—or version-gate the message—before landing the 16-bit change.
🤖 Prompt for AI Agents
In wled00/udp.cpp around lines 40-41, the code writes only the low byte of
mainseg.mode into udpOut[8], which truncates 16-bit mode IDs ≥256; update the
notifier to emit both low and high bytes (e.g., little-endian: udpOut[8]=low,
udpOut[9]=high) and increment or set a protocol/version byte so receivers can
detect the extended format, and simultaneously update the UDP decoder/receiver
code to read two bytes into a 16-bit mode value when the version indicates
extension (fall back to single-byte behavior for older versions).
There was a problem hiding this comment.
the (very clumsy) workaround for now is to clamp effect ID at 255 or udp. For a real solution, there should be a new version of the UDP dataset.
There was a problem hiding this comment.
@softhack007 Agreed. Here’s a minimal, backward-compatible “UDP notifier v13” that carries 16-bit effect IDs via the per-segment payload, while keeping the header’s mode field clamped to 8-bit for legacy peers. This avoids changing the fixed 41-byte header and uses the already variable per-segment block size.
Key points
- Header (byte 8) remains 8-bit for compatibility; new peers should consume the per-segment mode.
- Bump compatibilityVersionByte to 13.
- Increase per-segment block size from 36 to 38 bytes and append:
- [36+ofs]: mode MSB
- Receiver: if version >= 13 and per-segment size >= 38, read 16-bit mode = LSB ([11+ofs]) | (MSB << 8) ([36+ofs]).
Proposed patch (wled00/udp.cpp)
diff --git a/wled00/udp.cpp b/wled00/udp.cpp
index 5f6c9ab..b0c2e33 100644
--- a/wled00/udp.cpp
+++ b/wled00/udp.cpp
@@ -6,7 +6,8 @@
* UDP sync notifier / Realtime / Hyperion / TPM2.NET
*/
-#define UDP_SEG_SIZE 36
+// v13: extend per-segment payload by 2 bytes (mode MSB + reserved)
+#define UDP_SEG_SIZE 38
#define SEG_OFFSET (41+(MAX_NUM_SEGMENTS*UDP_SEG_SIZE))
#define WLEDPACKETSIZE (41+(MAX_NUM_SEGMENTS*UDP_SEG_SIZE)+0)
#define UDP_IN_MAXSIZE 1472
@@ -33,10 +34,10 @@ void notify(byte callMode, bool followUp)
udpOut[4] = G(col);
udpOut[5] = B(col);
udpOut[6] = nightlightActive;
udpOut[7] = nightlightDelayMins;
- udpOut[8] = min(mainseg.mode, uint16_t(255)); // WLEDMM toDo: need workaround for 16bit mode numbers - critical as this affects webUI
+ udpOut[8] = min(mainseg.mode, uint16_t(255)); // v13: header remains 8-bit for legacy peers; 16-bit lives in per-segment payload
udpOut[9] = mainseg.speed;
udpOut[10] = W(col);
//compatibilityVersionByte:
//0: old 1: supports white 2: supports secondary color
//3: supports FX intensity, 24 byte packet 4: supports transitionDelay 5: sup palette
//6: supports timebase syncing, 29 byte packet 7: supports tertiary color 8: supports sys time sync, 36 byte packet
//9: supports sync groups, 37 byte packet 10: supports CCT, 39 byte packet 11: per segment options, variable packet length (40+MAX_NUM_SEGMENTS*3)
- //12: enhanced effect sliders, 2D & mapping options
- udpOut[11] = 12;
+ //12: enhanced effect sliders, 2D & mapping options 13: 16-bit effect ID in per-segment payload
+ udpOut[11] = 13;
col = mainseg.colors[1];
udpOut[12] = R(col);
udpOut[13] = G(col);
@@ -86,6 +87,8 @@ void notify(byte callMode, bool followUp)
udpOut[10+ofs] = selseg.opacity;
- udpOut[11+ofs] = min(selseg.mode, uint16_t(255)); // WLEDMM toDo: need workaround for 16bit mode numbers
+ // Per-segment effect: keep LSB at legacy location for old receivers, add MSB at the tail for v13+
+ udpOut[11+ofs] = min(selseg.mode, uint16_t(255));
udpOut[12+ofs] = selseg.speed;
udpOut[13+ofs] = selseg.intensity;
udpOut[14+ofs] = selseg.palette;
@@ -109,6 +112,10 @@ void notify(byte callMode, bool followUp)
udpOut[34+ofs] = selseg.stopY >> 8;
udpOut[35+ofs] = selseg.stopY & 0xFF;
+ // v13 additions for 16-bit effect IDs
+ udpOut[36+ofs] = (selseg.mode >> 8) & 0xFF; // mode MSB
+ udpOut[37+ofs] = 0; // reserved for future use
++s;
}
@@ -282,12 +289,21 @@ void handleNotifications()
selseg.setOpacity(udpIn[10+ofs]);
if (applyEffects) {
- strip.setMode(id, udpIn[11+ofs]);
+ // Reconstruct 16-bit mode when sender is v13+ and per-segment size >= 38
+ uint16_t rxMode = udpIn[11+ofs];
+ if (version >= 13 && udpIn[40] >= 38) {
+ rxMode |= uint16_t(udpIn[36+ofs]) << 8;
+ }
+ strip.setMode(id, rxMode);
selseg.speed = udpIn[12+ofs];
selseg.intensity = udpIn[13+ofs];
selseg.palette = udpIn[14+ofs];
}
if (receiveNotificationColor || !someSel) {
selseg.setColor(0, RGBW32(udpIn[15+ofs],udpIn[16+ofs],udpIn[17+ofs],udpIn[18+ofs]));
selseg.setColor(1, RGBW32(udpIn[19+ofs],udpIn[20+ofs],udpIn[21+ofs],udpIn[22+ofs]));
selseg.setColor(2, RGBW32(udpIn[23+ofs],udpIn[24+ofs],udpIn[25+ofs],udpIn[26+ofs]));
selseg.setCCT(udpIn[27+ofs]);
}Behavior and compatibility
- Sender
- Keeps header mode truncated to 8-bit (byte 8).
- Advertises version 13 and sets per-segment size to 38 via byte 40.
- Adds mode MSB at [36+ofs].
- Receiver
- If version >= 13 and per-segment size >= 38, reconstructs 16-bit mode from [11+ofs] and [36+ofs].
- Otherwise, falls back to 8-bit behavior unchanged.
- Legacy nodes keep working; they’ll ignore the extra bytes and use the 8-bit header and 8-bit per-segment mode.
Limitations
- “Simple effect sync” path (when receiveSegmentOptions is disabled or sender < 11) remains 8-bit by design since the 41-byte header is fixed. 16-bit sync is guaranteed when segment options are received.
Test plan
- v13 sender → v13 receiver: select an effect with ID > 255; verify matching effect on peer via per-segment path.
- v13 sender → legacy receiver: peer should continue to operate but will display the truncated 8-bit effect.
- Legacy sender → v13 receiver: unchanged behavior.
Would you like me to open a follow-up issue titled “UDP notifier v13: 16-bit effect IDs in segment payload” and reference this PR/comment so we can track the wider rollout (UI docs, config hint to enable receiveSegmentOptions, etc.)?
the problem was that "r" with min=max=0 did not behave proprly in 16bit. This fix makes it work in 16bit, and tries to keep the 8bit variant working, too.
this prevents negative numbers in palette names, if mode>255
That's a case which should never happen (palette numbers are still 8bit). However I'll add some safety code.
I'll check these two independantly - solution (if needed) will be another PR. @CodeRabbit review |
|
@softhack007, thank you for the clarification! I understand your approach:
I'll trigger a review of the PR now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
11776-11781: Auto‑slot search correctly skips reserved IDs; consider tracking next free index.Logic is correct and avoids
MODE_AUTO/MODE_AUTO_LEGACY. For large tables, a simple “next‑free slot” cursor (advanced when a slot is taken) avoids repeated O(n) scans during setup.
11775-11790: Minor robustness notes.
- Assigning
size_t itouint16_t idcan truncate if_mode.size()ever exceedsUINT16_MAX. Unlikely here, but a static_assert or bound check would make intent explicit.- Relying on pointer equality with
_data_RESERVEDis fine given all placeholders point to the single PROGMEM sentinel; consider a small helperisReserved(i)for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(1 hunks)wled00/util.cpp(8 hunks)
🔇 Additional comments (7)
wled00/FX.cpp (1)
11782-11786: Safe overwrite guard.Good call on preventing writes to legacy/auto slots and only replacing reserved entries.
wled00/util.cpp (6)
13-19: LGTM! Clean 8-bit wrapper maintains backward compatibility.The wrapper correctly delegates to
parseNumber16and constrains results to the 8-bit range. The special handling at line 18 for random values with unset limits (min=max=0) using& 0x00FFis a good approach to preserve legacy semantics.
22-61: LGTM! 16-bit parsing logic is solid.The function correctly handles the full 16-bit range with the updated default max of 65535 at line 25. All parsing paths (random, increment/decrement, range syntax) properly use
uint16_ttypes, and edge cases like wrapping and bounds constraints are handled correctly.
79-92: LGTM! New 16-bit helpers are correctly implemented.
getVal16andupdateVal16are clean 16-bit variants of their 8-bit counterparts. Both properly delegate toparseNumber16and handle JSON and HTTP request parsing correctly.Also applies to: 104-111
283-337: LGTM! Signature updated and defensive palette label handling added.The function signature correctly migrates to 16-bit. Lines 303-304 implement good defensive programming for custom palette labels:
mode <= 255uses255-modeto avoid negative formattingmode > 255usesmode - 255with unsigned formatThis aligns with the approach discussed in the PR comments. The comment "this should not happen for palettes, better safe than sorry" appropriately flags this as defensive code.
341-412: LGTM! Signature correctly migrated to 16-bit.The function signature and return type are properly updated to use
uint16_tfor 16-bit mode IDs. The core slider extraction logic remains intact.Note: Known issues at lines 378 (dangling pointer) and 406 (buffer boundary) are already acknowledged and will be addressed in a separate PR per the comments summary.
416-434: LGTM! Signature correctly updated for 16-bit mode parameter.The function signature properly migrates the
modeparameter fromuint8_ttouint16_t. Keeping the return type asint16_tremains appropriate for returning -1 on error.
still no solution. But now, every effect >255 will lead to udpOut[8]=255, causing less confusion.
I think there will be more, especially inside usermods.
* addEffect: explicitly add entry 255 when expanding mode vector (tested and works) * if the target slot is already in use, search for a new free slot (instead of failing silently) * add some debug statements, to trace where effects are really "landing" * add some more room (MODE_COUNT) when both ANIMARTRIX and ParticleFX are enabled
see discussion #270 (review)
softhack007
changed the title
@netmindz i'm happy with my proof-of concept now. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
wled00/util.cpp (3)
64-77: Bound JSON integer assignments to vmin..vmax.Both getVal() and getVal16() assign integers without bounds checks; out-of-range FX IDs can slip through.
Apply:
bool getVal(JsonVariant elem, byte* val, byte vmin, byte vmax) { if (elem.is<int>()) { if (elem < 0) return false; //ignore e.g. {"ps":-1} - *val = elem; // ToDO: check if we need a bounds test [vmin ... vmax] before assigning the result + int x = (int)elem; + if (vmin || vmax) { + if (x < vmin) x = vmin; + else if (x > vmax) x = vmax; + } + *val = (byte)x; return true;bool getVal16(JsonVariant elem, uint16_t* val, uint16_t vmin, uint16_t vmax) { // same as above, with 2byte output buffer if (elem.is<int>()) { if (elem < 0) return false; //ignore e.g. {"ps":-1} - *val = elem; // ToDO: check if we need a bounds test [vmin ... vmax] before assigning the result + uint32_t x = (uint32_t)elem; + if (x > 65535) x = 65535; + *val = (uint16_t)x; + if (vmin || vmax) { + if (*val < vmin) *val = vmin; + else if (*val > vmax) *val = vmax; + } return true;Also applies to: 79-92
293-299: Fix off‑by‑one NUL write in extractModeName.When j reaches maxLen, dest[j] writes one past the buffer.
Apply:
- for (; j < maxLen && j < len; j++) { + for (; (j + 1) < maxLen && j < len; j++) { if (lineBuffer[j] == '\0' || lineBuffer[j] == '@') break; dest[j] = lineBuffer[j]; } - dest[j] = 0; // terminate string + dest[j] = '\0'; // terminate string
22-61: Clamp parsed values and adjust random range usage
- In the plain-number path at the end of parseNumber16 (wled00/util.cpp), replace
*val = uint16_t(atoi(str));
with:- *val = uint16_t(atoi(str)); + int32_t v = atoi(str); + if (v < 0) v = 0; + if (v > 65535) v = 65535; + uint16_t uv = uint16_t(v); + if (minv || maxv) { + if (uv < minv) uv = minv; + else if (uv > maxv) uv = maxv; + } + *val = uv;- Confirmed:
hw_random(lower, upper)returns values in [lower…upper). To include maxv, callhw_random(minv, maxv + 1)with an overflow guard whenmaxv == UINT16_MAX.wled00/ir.cpp (6)
85-92: Effect math still capped at 255 — introduce a 16‑bit variantrelativeChange() returns byte; when used with effectCurrent it truncates >255. Add a 16‑bit helper and use it for effects.
byte relativeChange(byte property, int8_t amount, byte lowerBoundary, byte higherBoundary) { int16_t new_val = (int16_t) property + amount; if (lowerBoundary >= higherBoundary) return property; if (new_val > higherBoundary) new_val = higherBoundary; if (new_val < lowerBoundary) new_val = lowerBoundary; return (byte)constrain(new_val, 0, 255); } +// 16‑bit variant for widened effect IDs +uint16_t relativeChange16(uint16_t property, int16_t amount, uint16_t lowerBoundary, uint16_t higherBoundary) +{ + int32_t new_val = (int32_t)property + amount; + if (lowerBoundary >= higherBoundary) return property; + if (new_val > (int32_t)higherBoundary) new_val = higherBoundary; + if (new_val < (int32_t)lowerBoundary) new_val = lowerBoundary; + return (uint16_t)new_val; +}
511-514: Use relativeChange16() for effect navigation to unlock >255All these cases still use byte-returning relativeChange(), preventing selecting modes above 255.
- case IR44_REDPLUS : changeEffect(relativeChange(effectCurrent, 1, 0, strip.getModeCount() -1)); break; - case IR44_REDMINUS : changeEffect(relativeChange(effectCurrent, -1, 0, strip.getModeCount() -1)); break; + case IR44_REDPLUS : changeEffect(relativeChange16(effectCurrent, 1, 0, strip.getModeCount() -1)); break; + case IR44_REDMINUS : changeEffect(relativeChange16(effectCurrent, -1, 0, strip.getModeCount() -1)); break; - case IR6_VOLUME_UP: changeEffect(relativeChange(effectCurrent, 1, 0, strip.getModeCount() -1)); break; + case IR6_VOLUME_UP: changeEffect(relativeChange16(effectCurrent, 1, 0, strip.getModeCount() -1)); break; - case IR9_SELECT : changeEffect(relativeChange(effectCurrent, 1, 0, strip.getModeCount() -1)); break; + case IR9_SELECT : changeEffect(relativeChange16(effectCurrent, 1, 0, strip.getModeCount() -1)); break; - case IR24_MC_MODES : changeEffect(relativeChange(effectCurrent, 1, 0, strip.getModeCount() -1)); break; //WLEDMM: sound and non sound modes - case IR24_MC_MODE : changeEffect(relativeChange(effectCurrent, -1, 0, strip.getModeCount() -1)); break; //WLEDMM: sound and non sound modes + case IR24_MC_MODES : changeEffect(relativeChange16(effectCurrent, 1, 0, strip.getModeCount() -1)); break; //WLEDMM: sound and non sound modes + case IR24_MC_MODE : changeEffect(relativeChange16(effectCurrent, -1, 0, strip.getModeCount() -1)); break; //WLEDMM: sound and non sound modesAlso applies to: 571-571, 609-609, 624-626
676-685: Potential buffer overflow in objKey formatting""0x%lX":" can be up to 11 chars for 0xFFFFFF (plus NUL). objKey[10] is too small. Use a larger buffer and snprintf.
- char objKey[10]; + char objKey[16]; @@ - sprintf_P(objKey, PSTR("\"0x%lX\":"), (unsigned long)code); + snprintf_P(objKey, sizeof(objKey), PSTR("\"0x%lX\":"), (unsigned long)code);
186-186: Bug: writing intensity into speedShould assign intensity, not speed, when not applying to all segments.
- strip.getMainSegment().speed = effectIntensity; + strip.getMainSegment().intensity = effectIntensity;
80-80: Printf specifiers: prefer unsigned for widened effectIDUse %u for preset/effect/palette to avoid sign misinterpretation.
- USER_PRINTF("applyPresetWithFallback %d %d %d\n", presetID, effectID, paletteID); + USER_PRINTF("applyPresetWithFallback %u %u %u\n", presetID, effectID, paletteID);
713-717: Promote FX to uint16_t and replace bitwise-OR defaults
In wled00/ir.cpp at line 714, change:- uint8_t p2 = fdo["FX"] | random8(strip.getModeCount() -1); + uint16_t p2 = fdo.containsKey(F("FX")) + ? fdo[F("FX")].as<uint16_t>() + : (uint16_t)random(strip.getModeCount()); + if (p2 >= strip.getModeCount()) p2 = strip.getModeCount() - 1;Use
containsKey+.as<T>()for safe defaults andrandom(...)to avoid 8-bit truncation.
♻️ Duplicate comments (1)
wled00/util.cpp (1)
373-379: Dangling substring pointer and OOB NUL in extractModeSlider.
- Using names.substring(...).c_str() assigns a pointer to a temporary; UB when used next line.
- dest[maxLen] = '\0' writes past the end.
Apply:
- if (nameEnd<0) tmpstr = names.substring(nameBegin).c_str(); // did not find ",", last name? - else tmpstr = names.substring(nameBegin, nameEnd).c_str(); - strlcpy(dest, tmpstr, maxLen); // copy the name into buffer (replacing previous) + String sub = (nameEnd < 0) + ? names.substring(nameBegin) // last name + : names.substring(nameBegin, nameEnd); + strlcpy(dest, sub.c_str(), maxLen); // keep temporary alive- dest[maxLen] = '\0'; // strncpy does not necessarily null terminate string + dest[maxLen-1] = '\0'; // strncpy does not necessarily null terminate stringRun to find any remaining patterns:
#!/bin/bash rg -nP -C1 'dest\[maxLen\]\s*=' wled00 rg -nP -C2 'substring\([^)]*\)\.c_str\(\)' wled00Also applies to: 406-407
🧹 Nitpick comments (2)
wled00/fcn_declare.h (1)
390-393: 16‑bit parsing helpers: clamp and revisit defaults.
- getVal16 assigns ints without clamping; consider enforcing [vmin..vmax] on integer path (not only string path).
- Default vmax=255 is surprising now; prefer explicit bounds at call sites (e.g., strip.getModeCount()-1).
wled00/ir.cpp (1)
94-108: Refactor: callers of changeEffect still limited to 8-bit fx
Calling changeEffect(relativeChange(...)) uses relativeChange’s byte return value, truncating fx to 0–255; provide a uint16_t-returning variant or otherwise widen these callers if you intend to leverage the full 16-bit range.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
usermods/ST7789_display/ST7789_display.h(1 hunks)usermods/usermod_v2_auto_save/usermod_v2_auto_save.h(2 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h(1 hunks)wled00/FX.cpp(1 hunks)wled00/FX.h(11 hunks)wled00/fcn_declare.h(3 hunks)wled00/ir.cpp(2 hunks)wled00/json.cpp(1 hunks)wled00/presets.cpp(1 hunks)wled00/remote.cpp(1 hunks)wled00/udp.cpp(2 hunks)wled00/util.cpp(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/udp.cpp
🧰 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 source and headers in wled00
Files:
wled00/presets.cppwled00/ir.cppwled00/json.cppwled00/fcn_declare.hwled00/FX.hwled00/FX.cppwled00/remote.cppwled00/util.cpp
🧬 Code graph analysis (4)
wled00/json.cpp (1)
wled00/util.cpp (2)
getVal16(79-92)getVal16(79-79)
wled00/fcn_declare.h (2)
wled00/presets.cpp (2)
applyPresetWithFallback(138-144)applyPresetWithFallback(138-138)wled00/util.cpp (12)
parseNumber16(22-61)parseNumber16(22-22)getVal16(79-92)getVal16(79-79)updateVal16(104-111)updateVal16(104-104)extractModeName(283-337)extractModeName(283-283)extractModeSlider(341-412)extractModeSlider(341-341)extractModeDefaults(416-434)extractModeDefaults(416-416)
wled00/FX.h (2)
wled00/FX_fcn.cpp (6)
setMode(593-631)setMode(593-593)setMode(2147-2157)setMode(2147-2147)currentMode(481-483)currentMode(481-481)wled00/FX.cpp (2)
addEffect(11775-11814)addEffect(11775-11775)
wled00/util.cpp (1)
wled00/fcn_declare.h (1)
hw_random(432-432)
🔇 Additional comments (11)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.h (1)
60-61: 16-bit mode tracking LGTM; verify end-to-end with IDs >255.Widening knownMode/currentMode is consistent. Please sanity-check JSON/preset roundtrip (boot apply, autosave) with a mode ID >255.
Also applies to: 136-137
usermods/ST7789_display/ST7789_display.h (1)
68-69: LGTM on 16-bit knownMode.Matches strip.getMainSegment().mode width; no further changes needed here.
wled00/fcn_declare.h (1)
248-249: API widened to 16-bit effect IDs — OK.Signature aligns with 16-bit transition across callers (IR/remote/presets). Ensure all externs/forward decls match to avoid ODR/link issues.
wled00/FX.h (1)
373-381: 16‑bit migration in FX core looks consistent; verify edge cases.
- Sentinels: MODE_AUTO=65000 and MODE_AUTO_LEGACY=255 are fine; confirm addEffect path handles IDs ≥256 and never overwrites slot 255.
- Crossing 255: please test getModeCount/reporting, addEffect appends, and setMode across IDs 254→256.
- ABI: public headers changed; ensure all usermods compile against new signatures.
Also applies to: 403-403, 626-626, 670-670, 872-876, 955-955, 975-975, 1015-1015, 1042-1042, 1154-1154
wled00/presets.cpp (1)
138-144: Preset fallback widened to 16‑bit — OK.Consistent with header and callers (IR/remote). No further changes needed here.
wled00/remote.cpp (1)
122-126: Remote preset fallback updated — OK.Signature matches presets API; forwards correctly.
wled00/util.cpp (4)
13-19: 8-bit wrapper logic looks good.Correct delegation to 16-bit + low-byte handling for random. No change requested.
104-111: LGTM: 16-bit update helper.Mirror of 8-bit path; consistent with new parseNumber16.
302-307: Palette label math: 16‑bit safe mapping.Looks reasonable; please sanity‑check against UI expectations for custom palette indices >255.
690-703: Random helpers: OK.Overloads provide [0, upper) and [lower, upper) ranges; matches typical semantics; pairs well with parseNumber16.
Confirm there is a visible declaration for these overloads in headers before first use (author said yes).
wled00/ir.cpp (1)
74-83: Signature widened to uint16_t — OKpresetFallback now accepts 16‑bit effect IDs. No issues here.
| uint8_t effectPaletteIndex = 0; | ||
| uint8_t knownMode = 0; | ||
| uint16_t knownMode = 0; | ||
| uint8_t knownPalette = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Not 16‑bit safe: byte index arrays and uint8_t loops will break >255 modes.
- modes_alpha_indexes is byte*, and loops use uint8_t counters; they overflow for counts >255 (wrap/infinite loop, wrong indices).
- You won’t be able to select effects with IDs ≥256 because sorted index → ID mapping truncates to 8‑bit.
Apply this minimal refactor to make the UI 16‑bit safe:
@@
- // Array of mode indexes in alphabetical order.
- byte *modes_alpha_indexes = nullptr;
+ // Array of mode indexes in alphabetical order (16-bit safe).
+ uint16_t *modes_alpha_indexes = nullptr;
@@
- byte *re_initIndexArray(int numModes);
+ uint16_t *re_initIndexArray(uint16_t numModes);
@@
- void re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip);
+ void re_sortModes(const char **modeNames, uint16_t *indexes, uint16_t count, uint16_t numSkip);
@@ void RotaryEncoderUIUsermod::sortModesAndPalettes() {
- modes_alpha_indexes = re_initIndexArray(strip.getModeCount());
+ modes_alpha_indexes = re_initIndexArray(strip.getModeCount());
@@
-byte *RotaryEncoderUIUsermod::re_initIndexArray(int numModes) {
- byte *indexes = (byte *)malloc(sizeof(byte) * numModes);
- for (byte i = 0; i < numModes; i++) {
+uint16_t *RotaryEncoderUIUsermod::re_initIndexArray(uint16_t numModes) {
+ uint16_t *indexes = (uint16_t *)malloc(sizeof(uint16_t) * numModes);
+ for (uint16_t i = 0; i < numModes; i++) {
indexes[i] = i;
}
return indexes;
}
@@ const char **RotaryEncoderUIUsermod::re_findModeStrings(const char json[], int numModes) {
- uint8_t modeIndex = 0;
+ uint16_t modeIndex = 0;
@@ void RotaryEncoderUIUsermod::re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip) {
-void RotaryEncoderUIUsermod::re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip) {
+void RotaryEncoderUIUsermod::re_sortModes(const char **modeNames, uint16_t *indexes, uint16_t count, uint16_t numSkip) {
if (!modeNames) return;
listBeingSorted = modeNames;
- qsort(indexes + numSkip, count - numSkip, sizeof(byte), re_qstringCmp);
+ qsort(indexes + numSkip, count - numSkip, sizeof(uint16_t), re_qstringCmp);
listBeingSorted = nullptr;
}
@@ void RotaryEncoderUIUsermod::findCurrentEffectAndPalette() {
- for (uint8_t i = 0; i < strip.getModeCount(); i++) {
+ for (uint16_t i = 0; i < strip.getModeCount(); i++) {
@@
- for (uint8_t i = 0; i < strip.getPaletteCount(); i++) {
+ for (uint16_t i = 0; i < strip.getPaletteCount(); i++) {Also update re_qstringCmp to read uint16_t indices:
-static int re_qstringCmp(const void *ap, const void *bp) {
- const char *a = listBeingSorted[*((byte *)ap)];
- const char *b = listBeingSorted[*((byte *)bp)];
+static int re_qstringCmp(const void *ap, const void *bp) {
+ const char *a = listBeingSorted[*((const uint16_t *)ap)];
+ const char *b = listBeingSorted[*((const uint16_t *)bp)];This prevents overflow and enables selecting effects beyond 255.
🏁 Script executed:
#!/bin/bash
# Locate the rotary encoder UI usermod files
fd usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h
# Search for modes_alpha_indexes declaration and usage
rg -n "modes_alpha_indexes" usermods/usermod_v2_rotary_encoder_ui_ALT
# Search for re_initIndexArray definition and usage
rg -n "re_initIndexArray" -n usermods/usermod_v2_rotary_encoder_ui_ALT
# Search for re_sortModes definition and usage
rg -n "re_sortModes" -n usermods/usermod_v2_rotary_encoder_ui_ALT
# Search for loops using uint8_t counters
rg -n "for (uint8_t" -n usermods/usermod_v2_rotary_encoder_ui_ALT
# Search for qsort calls in this module
rg -n "qsort" -n usermods/usermod_v2_rotary_encoder_ui_ALTLength of output: 3045
🏁 Script executed:
#!/bin/bash
# Search all uint8_t usages in this module
rg -n "uint8_t" usermods/usermod_v2_rotary_encoder_ui_ALT
# Show context around strip.getModeCount usage for loops
rg -n "strip.getModeCount" -C3 usermods/usermod_v2_rotary_encoder_ui_ALTLength of output: 4375
🏁 Script executed:
#!/bin/bash
# Show first 200 lines to find effectCurrentIndex declaration
cat -n usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h | head -200Length of output: 6357
Make mode/palette indices and related loops 16-bit safe
- Change
byte *modes_alpha_indexes/byte *palettes_alpha_indexestouint16_t *. - Update signatures:
byte *re_initIndexArray(int numModes)→uint16_t *re_initIndexArray(uint16_t numModes)void re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip)→void re_sortModes(const char **modeNames, uint16_t *indexes, uint16_t count, uint16_t numSkip)
- In
re_sortModes, useqsort(..., sizeof(uint16_t), re_qstringCmp)and cast inre_qstringCmpfrombyte*toconst uint16_t*. - Replace all
for (uint8_t i = 0; i < strip.getModeCount(); i++)and similar palette loops withfor (uint16_t i = 0; i < strip.getModeCount(); i++)to avoid overflow when counts exceed 255.
This ensures arrays, loops, and sorting handle >255 modes/palettes without truncation or wrap-around.
🤖 Prompt for AI Agents
In usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h
around lines 155-157, change all mode/palette index variables and related
arrays/functions to 16-bit to avoid overflow: make effectPaletteIndex,
knownPalette (and any similar uint8_t index vars) uint16_t; change byte
*modes_alpha_indexes and byte *palettes_alpha_indexes to uint16_t *; update
function signatures to uint16_t *re_initIndexArray(uint16_t numModes) and void
re_sortModes(const char **modeNames, uint16_t *indexes, uint16_t count, uint16_t
numSkip); in re_sortModes call qsort with sizeof(uint16_t) and ensure the
comparator casts its void*/byte* parameters to const uint16_t* before
dereferencing; and replace all for-loops that use uint8_t for mode/palette
iteration (e.g. for (uint8_t i = 0; i < strip.getModeCount(); i++)) with
uint16_t to handle counts >255.
There was a problem hiding this comment.
@coderabbitai sorry, this is unrelated. Mode IDs are now 16bit, but palette ID (and preset IDs, too) remain as 8bit.
There was a problem hiding this comment.
@softhack007 You’re right—thanks for the correction. Let’s scope the change strictly to modes and leave palettes 8-bit. Here’s a minimal patch that makes only the mode-side 16-bit safe while keeping palette indices and arrays as byte.
diff --git a/usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h b/usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h
--- a/usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h
+++ b/usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h
@@
static int re_qstringCmp(const void *ap, const void *bp) {
const char *a = listBeingSorted[*((byte *)ap)];
const char *b = listBeingSorted[*((byte *)bp)];
int i = 0;
do {
@@
return 0;
}
+// 16-bit comparator for mode index sorting (palettes remain 8-bit)
+static int re_qstringCmp16(const void *ap, const void *bp) {
+ const char *a = listBeingSorted[*((const uint16_t *)ap)];
+ const char *b = listBeingSorted[*((const uint16_t *)bp)];
+ int i = 0;
+ do {
+ char aVal = pgm_read_byte_near(a + i);
+ if (aVal >= 97 && aVal <= 122) aVal -= 32;
+ char bVal = pgm_read_byte_near(b + i);
+ if (bVal >= 97 && bVal <= 122) bVal -= 32;
+ if (aVal == '"' || bVal == '"' || aVal == '\0' || bVal == '\0') {
+ if (aVal == bVal) return 0;
+ else if (aVal == '"' || aVal == '\0') return -1;
+ else return 1;
+ }
+ if (aVal == bVal) { i++; continue; }
+ return (aVal < bVal) ? -1 : 1;
+ } while (true);
+ return 0;
+}
@@ class RotaryEncoderUIUsermod : public Usermod {
private:
@@
- // Array of mode indexes in alphabetical order.
- byte *modes_alpha_indexes = nullptr;
+ // Array of mode indexes in alphabetical order (16-bit for modes).
+ uint16_t *modes_alpha_indexes = nullptr;
@@
// Pointers the start of the palette names within JSON_palette_names
const char **palettes_qstrings = nullptr;
// Array of palette indexes in alphabetical order.
byte *palettes_alpha_indexes = nullptr;
@@
- byte *re_initIndexArray(int numModes);
+ byte *re_initIndexArray(int numModes); // for palettes (8-bit)
+ uint16_t *re_initIndexArray16(uint16_t numModes); // for modes (16-bit)
@@
- void re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip);
+ void re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip); // palettes
+ void re_sortModes16(const char **modeNames, uint16_t *indexes, uint16_t count, uint16_t numSkip); // modes
@@ void RotaryEncoderUIUsermod::sortModesAndPalettes() {
- //modes_qstrings = re_findModeStrings(JSON_mode_names, strip.getModeCount());
modes_qstrings = strip.getModeDataSrc();
- modes_alpha_indexes = re_initIndexArray(strip.getModeCount());
- re_sortModes(modes_qstrings, modes_alpha_indexes, strip.getModeCount(), MODE_SORT_SKIP_COUNT);
+ modes_alpha_indexes = re_initIndexArray16(strip.getModeCount());
+ re_sortModes16(modes_qstrings, modes_alpha_indexes, strip.getModeCount(), MODE_SORT_SKIP_COUNT);
@@
palettes_qstrings = re_findModeStrings(JSON_palette_names, strip.getPaletteCount());
palettes_alpha_indexes = re_initIndexArray(strip.getPaletteCount()); // only use internal palettes
@@
re_sortModes(palettes_qstrings, palettes_alpha_indexes, strip.getPaletteCount(), skipPaletteCount);
}
@@
byte *RotaryEncoderUIUsermod::re_initIndexArray(int numModes) {
byte *indexes = (byte *)malloc(sizeof(byte) * numModes);
for (byte i = 0; i < numModes; i++) {
indexes[i] = i;
}
return indexes;
}
+
+uint16_t *RotaryEncoderUIUsermod::re_initIndexArray16(uint16_t numModes) {
+ uint16_t *indexes = (uint16_t *)malloc(sizeof(uint16_t) * numModes);
+ for (uint16_t i = 0; i < numModes; i++) {
+ indexes[i] = i;
+ }
+ return indexes;
+}
@@
void RotaryEncoderUIUsermod::re_sortModes(const char **modeNames, byte *indexes, int count, int numSkip) {
if (!modeNames) return;
listBeingSorted = modeNames;
qsort(indexes + numSkip, count - numSkip, sizeof(byte), re_qstringCmp);
listBeingSorted = nullptr;
}
+
+void RotaryEncoderUIUsermod::re_sortModes16(const char **modeNames, uint16_t *indexes, uint16_t count, uint16_t numSkip) {
+ if (!modeNames) return;
+ listBeingSorted = modeNames;
+ qsort(indexes + numSkip, count - numSkip, sizeof(uint16_t), re_qstringCmp16);
+ listBeingSorted = nullptr;
+}
@@ void RotaryEncoderUIUsermod::findCurrentEffectAndPalette() {
if (modes_alpha_indexes == nullptr) return; // WLEDMM bugfix
currentEffectAndPaletteInitialized = true;
- for (uint8_t i = 0; i < strip.getModeCount(); i++) {
+ for (uint16_t i = 0; i < strip.getModeCount(); i++) {
if (modes_alpha_indexes[i] == effectCurrent) {
effectCurrentIndex = i;
break;
}
}
- for (uint8_t i = 0; i < strip.getPaletteCount(); i++) {
+ // palettes remain 8-bit
+ for (uint8_t i = 0; i < strip.getPaletteCount(); i++) {
if (palettes_alpha_indexes[i] == effectPalette) {
effectPaletteIndex = i;
break;
}
}
}Result:
- Mode sorting/indexing is 16-bit safe, so selecting effects with IDs ≥256 works.
- Palette sorting/indexing is unchanged and stays 8-bit as intended.
If you prefer, I can split the new 16-bit helpers into a small .cpp to keep the header slimmer.
✏️ Learnings added
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.799Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
* proper default MaxVal for parseNumber16, getVal16, updateVal16 * more fixes for IR.cpp * json robustness improvement
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
11777-11780: Drop the duplicated TODO comment.Looks like the same TODO block got pasted twice during the merge. Let's keep a single copy so future readers don’t wonder whether there are two separate follow-ups to tackle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.cpp(1 hunks)wled00/fcn_declare.h(3 hunks)wled00/ir.cpp(7 hunks)wled00/json.cpp(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 source and headers in wled00
Files:
wled00/ir.cppwled00/fcn_declare.hwled00/json.cppwled00/FX.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.799Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: wled00/FX.cpp:11799-11813
Timestamp: 2025-10-22T21:50:22.363Z
Learning: In PR #270 (16-bit effect IDs PoC), the current addEffect behavior of appending and ignoring explicit ids >= _mode.size() is intentional for the proof-of-concept; changes to honor explicit ids are deferred to a follow-up.
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.286Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-22T21:50:25.799Z
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.799Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/ir.cppwled00/fcn_declare.hwled00/json.cppwled00/FX.cpp
📚 Learning: 2025-10-22T21:50:22.363Z
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: wled00/FX.cpp:11799-11813
Timestamp: 2025-10-22T21:50:22.363Z
Learning: In PR #270 (16-bit effect IDs PoC), the current addEffect behavior of appending and ignoring explicit ids >= _mode.size() is intentional for the proof-of-concept; changes to honor explicit ids are deferred to a follow-up.
Applied to files:
wled00/ir.cppwled00/fcn_declare.hwled00/json.cppwled00/FX.cpp
📚 Learning: 2025-10-22T22:24:46.136Z
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.136Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/ir.cppwled00/json.cppwled00/FX.cpp
📚 Learning: 2025-10-22T21:59:24.286Z
Learnt from: softhack007
PR: MoonModules/WLED-MM#270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.286Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/fcn_declare.hwled00/json.cpp
🧬 Code graph analysis (3)
wled00/fcn_declare.h (2)
wled00/presets.cpp (2)
applyPresetWithFallback(138-144)applyPresetWithFallback(138-138)wled00/util.cpp (12)
parseNumber16(22-61)parseNumber16(22-22)getVal16(79-92)getVal16(79-79)updateVal16(104-111)updateVal16(104-104)extractModeName(283-337)extractModeName(283-283)extractModeSlider(341-412)extractModeSlider(341-341)extractModeDefaults(416-434)extractModeDefaults(416-416)
wled00/json.cpp (1)
wled00/util.cpp (2)
getVal16(79-92)getVal16(79-79)
wled00/FX.cpp (3)
wled00/data/index.js (13)
id(925-925)i(586-586)i(752-752)i(911-911)i(1035-1035)i(1347-1347)i(1659-1659)i(1663-1663)i(1664-1664)i(1667-1667)i(1690-1690)i(1916-1916)i(2072-2072)wled00/data/simple.js (1)
id(711-711)wled00/FX_fcn.cpp (4)
i(702-712)i(702-702)i(713-719)i(713-713)
⏰ 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: Builds (esp8266_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
🔇 Additional comments (3)
wled00/fcn_declare.h (1)
248-248: LGTM—16-bit effect ID declarations look correct.The function signature changes (lines 248, 390–392, 403–405) consistently widen effect ID parameters from
uint8_ttouint16_t, aligning with the PR's objective to support >255 effects. The new 16-bit parsing helpers and updated mode extraction signatures are well-formed.Based on learnings.
Also applies to: 390-392, 403-405
wled00/json.cpp (1)
295-306: LGTM—correct guard prevents underflow on unsigned count-1.Lines 295–297 correctly widen
fxtouint16_tand computelastwith the safe guard patternfxModeCount ? (uint16_t)(fxModeCount - 1) : 0, preventing underflow when the mode count is zero. Line 304 properly switches togetVal16for 16-bit effect IDs, and line 306 adds thefxdef2parameter tosetMode. This follows the established pattern from previous review discussions.Based on learnings.
wled00/ir.cpp (1)
74-74: LGTM—function signatures and 16-bit helper correctly widened.Lines 74 and 104 widen
effectIDandfxparameters fromuint8_ttouint16_t, and lines 94–102 introducerelativeChange16, a well-formed 16-bit variant that usesint32_tintermediate arithmetic to avoid overflow. These changes align with the PR's goal to support effect IDs beyond 255.Also applies to: 94-102, 104-104
reverting a "fix" that came from AI chat 😅, in favor of preserving previous behavior. to really solve the potential for "off by one" and "below by one" in parseNumber, we would need two copies of the code and fix only parseNumber16()
|
@coderabbitai I am wondering if our web UI (in wled00/data) is already compatible with changing the effect (aka mode) IDs from 8bit to 16bit. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 121420 🏁 Script executed: Length of output: 250 🏁 Script executed: Length of output: 148 🏁 Script executed: Length of output: 2628 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1809 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2029 🏁 Script executed: Length of output: 1268 🏁 Script executed: Length of output: 10712 🏁 Script executed: Length of output: 756 🏁 Script executed: Length of output: 3905 🏁 Script executed: Length of output: 360 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 3057 🏁 Script executed: Length of output: 162 🏁 Script executed: Length of output: 28228 🏁 Script executed: Length of output: 322 🏁 Script executed: Length of output: 3547 🏁 Script executed: Length of output: 66 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 321 @softhack007, I've analyzed the web UI (index.js) and AJAX protocol handlers (WebSocket, JSON, XML) for 16-bit effect ID compatibility. Good news: everything appears compatible! Here's what I found: ✅ Web UI (JavaScript) - CompatibleThe JavaScript in // Line 2869-2873: Effect ID handling
ind = parseInt(d.querySelector('#fxlist input[name="fx"]:checked').value);
var obj = {"seg": {"fx": parseInt(ind), ...}};No hardcoded 8-bit limits found in the UI logic. ✅ WebSocket Protocol - CompatibleThe WebSocket handler ( ✅ XML Protocol - CompatibleThe XML handler (
✅ Binary Liveview Protocol - Not AffectedThe binary liveview protocol in 🔍 Potential Further IssuesNone identified - the web UI and core AJAX protocols (WebSocket JSON, XML) are already compatible with 16-bit effect IDs. SummaryYour web UI is ready for 16-bit effect IDs! The JavaScript naturally handles larger integers, and both WebSocket (JSON) and XML protocols use text-based serialization that accommodates the expanded range. No UI changes or protocol updates are required for this migration. The known TODOs you mentioned (E1.31/DMX byte limit, WLED sync protocol) are already documented in your PR description and are separate from the web UI concern. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2761 🏁 Script executed: Length of output: 180 🏁 Script executed: Length of output: 316 🏁 Script executed: Length of output: 2731 🏁 Script executed: Length of output: 132 🏁 Script executed: Length of output: 141 🏁 Script executed: Length of output: 6859 🏁 Script executed: Length of output: 1013 🏁 Script executed: Length of output: 3579 🏁 Script executed: Length of output: 324 🏁 Script executed: Length of output: 2469 🏁 Script executed: Length of output: 86 🏁 Script executed: Length of output: 2148 🏁 Script executed: Length of output: 1336 🏁 Script executed: Length of output: 1020 🏁 Script executed: Length of output: 486 🏁 Script executed: Length of output: 11130 🏁 Script executed: Length of output: 30 🏁 Script executed: Length of output: 689 🏁 Script executed: Length of output: 412 🏁 Script executed: Length of output: 607 🏁 Script executed: Length of output: 2368 🏁 Script executed: Length of output: 2824 🏁 Script executed: Length of output: 64 🏁 Script executed: Length of output: 678 @softhack007, great question! I've analyzed the web UI and AJAX protocols for 16-bit effect ID compatibility. Here's what I found: ✅ Good news: The web UI appears fully compatible with 16-bit effect IDsProtocol Analysis:1. JSON Protocol (Primary data exchange):
2. XML Protocol:
3. WebSocket Protocol:
JavaScript UI Code (wled00/data/index.js):4. Effect ID Handling:
Potential Further Issues (minor):
Elegant Fix Suggestions:Since everything appears compatible, no fixes are needed for basic 16-bit support! However, for enhanced robustness: Optional Enhancement - Add client-side bounds checking: function setFX(ind = null)
{
if (ind === null) {
ind = parseInt(d.querySelector('#fxlist input[name="fx"]:checked').value, 10);
} else {
d.querySelector(`#fxlist input[name="fx"][value="${ind}"]`).checked = true;
}
// Optional: validate effect ID exists
if (!eJson.find((o) => o.id == ind)) {
showToast('Invalid effect ID', true);
return;
}
var obj = {"seg": {"fx": parseInt(ind, 10), "fxdef": cfg.comp.fxdef, "fxdef2": cfg.comp.fxdef2}};
requestJson(obj);
}But this is really optional — the existing code should work fine with 16-bit IDs as-is! Summary:Your web UI is already compatible with 16-bit effect IDs. The protocols (JSON/XML/WebSocket) are text-based and use appropriate types, JavaScript handles the larger numbers natively, and there are no binary constraints or hardcoded 8-bit limits in the UI code. The migration appears seamless from the web UI perspective. 🎉 🧠 Learnings used |
|
@coderabbitai thank you, this is very good news. |
|
Tip For best results, initiate chat on the files or code changes. @softhack007, you're very welcome! I'm glad the analysis was helpful. 🎉 The 16-bit effect ID migration looks solid from the web UI perspective. If you need any further analysis on other aspects of the PR (usermods, presets, playlists, or anything else), feel free to ask. Good luck with the rest of the migration! 🧠 Learnings used |
|
@coderabbitai ok, while we are at this: please also scan all usermods (directory /usermods) for similar inconsistencies, covering also the use of MQTT as an external protocol. If you are not sure about some inconsistencies, please just summarize these in a list of "potential further issues". If you are sure about an issue, please suggest an elegant fix with minimal impact. Instead of summarizing your findings in this PR, please create a single new github issue "usermods that need fixing for compatibility with 16bit effect IDs", so we can come back to this later. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 47094 🏁 Script executed: Length of output: 1781 🏁 Script executed: Length of output: 20172 🏁 Script executed: Length of output: 33919 🏁 Script executed: Length of output: 88 🏁 Script executed: Length of output: 40 🏁 Script executed: Length of output: 101 🏁 Script executed: Length of output: 171 @softhack007, I've completed a comprehensive scan of all usermods in the Summary of Findings✅ MQTT Protocol - No IssuesGood news! No MQTT-related compatibility issues were found in usermods. MQTT implementations use JSON or string-based protocols that naturally handle larger integers.
|
|
@coderabbitai thank you, well done :-) |
|
Tip For best results, initiate chat on the files or code changes. @softhack007, you're very welcome! I'm glad I could help with the usermod analysis and issue tracking. 😊 If you need anything else as you continue with the 16-bit effect ID migration—whether it's analyzing presets, playlists, or any other aspect of the codebase—just let me know. Happy to assist anytime! 🧠 Learnings used |
changed two loop counters from byte to uint16_t, to avoid infinite loops - "i < strip.getModeCount()" will always be true when the type of i is too small.
2 participants
Currently we can have at most 254 effects in WLED, due to the effect ID being an 8bit number.
This is getting painful as we collect more effect. Right now, its not possible to have ParticleFX and Animartix together in the same WLED build.
The solution would be to change effect IDs to 16bit. This PR is a first "proof of concept" to understand where things have to be changed.
A few open ends:
I haven't checked if any web-UI related code (JS) needs to be updated to handle 16bit effect id's
E1.31 (DMX) cannot change to 16bit effects because there is only one byte in the DMX structure sent to WLED
WLED sync (to sync presets/effects over several WLED instances) needs a new protocol version to transmit 16bit effect numbers
JSON API and files (presets.json, cfg.jcon) should work, however this will need special attention during testing
I've seen several places where effect IDs are assigned to signed int16_t. It might be better avoid IDs > 32767.
there might still be surprises in (many) usermods that are not prepared to handle 16bit effect numbers. Best case: usermods are still able to change to effects < 255, but cannot access effects > 255.
Candidates for additional checking (WLED features):
esp-now remote(fixed, OK)IR Remote(fixed, OK)Alexa,LoxoneCandidates for additional checking (usermods):
autosave(fixed, OK)there might be more places where IDs are still put into byte
A lot of testing will be needed, with focus on doing stuff on effects that have IDs > 255
Update: as SEGMENT.mode is cast to int16_t in many places, the new upper limit for effects is actually 32767 (thats still a lot 😀 )
Summary by CodeRabbit
New Features
New secondary effect default parameter (fxdef2) when loading/configuring segments.Improvements
Bug Fixes