Support custom DMX output value #4876
Conversation
WalkthroughAdds per-channel DMX values (DV1..DVN) and generalizes fixed-15 DMX assumptions to MAX_CHANNELS_PER_FIXTURE across EEPROM, config (JSON), UI/JS/XML, server output, settings handling, and runtime DMX mapping. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/dmx_output.cpp (1)
42-61: Restore backward compatibility for legacy fixmap value 6 (“Set to 255”)Prior builds supported DMXFixtureMap[j] == 6 to force 255. Removing case 6 breaks existing configurations by leaving those channels unwritten. Keep case 6 while introducing custom DV values.
Apply this diff in the switch:
switch (DMXFixtureMap[j]) { - case 0: // Set this channel to the selected value. + case 0: // Set this channel to the selected value (custom DV per channel). dmx.write(DMXAddr, DMXChannelsValue[j]); break; + case 6: // Legacy: "Set to 255" (pre-custom DV). Keep for backward-compatibility. + dmx.write(DMXAddr, 255); + break; case 1: // Red dmx.write(DMXAddr, calc_brightness ? (r * brightness) / 255 : r); break;
🧹 Nitpick comments (11)
wled00/set.cpp (2)
648-652: Guard and clamp DV inputs to avoid unintended resets and out-of-range valuesUnconditionally assigning request->arg("DVx").toInt() will:
- Reset non-submitted DV fields to 0 (disabled inputs aren’t submitted).
- Allow negatives or >255 to wrap into byte on assignment.
Consider only updating when the arg is present and clamp to [0,255].
Apply this diff within the selected range:
- for (int i=0; i<15; i++) { - String argname = "DV" + String((i+1)); - t = request->arg(argname).toInt(); - DMXChannelsValue[i] = t; - } + for (int i=0; i<15; i++) { + String argname = "DV" + String((i+1)); + if (request->hasArg(argname)) { + int v = request->arg(argname).toInt(); + DMXChannelsValue[i] = (byte)constrain(v, 0, 255); + } + }
307-313: Typo in comment: “fucntion” → “function”Minor spelling fix in a developer-facing comment.
Apply this diff:
- #ifdef SOC_TOUCH_VERSION_2 // ESP32 S2 and S3 have a fucntion to check touch state but need to attach an interrupt to do so + #ifdef SOC_TOUCH_VERSION_2 // ESP32 S2 and S3 have a function to check touch state but need to attach an interrupt to do sowled00/cfg.cpp (2)
754-761: Null-check JSON arrays before iterating; current code assumes array presenceFor robustness against older or hand-crafted cfg.json, guard dmx_fixmap existence before using size() and iterating.
Apply this diff:
- JsonArray dmx_fixmap = dmx[F("fixmap")]; - for (int i = 0; i < MIN(dmx_fixmap.size(), 15); i++) { - CJSON(DMXFixtureMap[i],dmx_fixmap[i]); - } + JsonArray dmx_fixmap = dmx[F("fixmap")]; + if (!dmx_fixmap.isNull()) { + for (int i = 0; i < MIN(dmx_fixmap.size(), 15); i++) { + CJSON(DMXFixtureMap[i], dmx_fixmap[i]); + } + }
764-768: Same here: guard and sanitize chsval; clamp to [0,255] to avoid wrap-aroundAdd a null check and clamp values when reading into byte.
Apply this diff:
- JsonArray dmx_chsval = dmx[F("chsval")]; - for (int i = 0; i < MIN(dmx_chsval.size(), 15); i++) { - CJSON(DMXChannelsValue[i],dmx_chsval[i]); - } + JsonArray dmx_chsval = dmx[F("chsval")]; + if (!dmx_chsval.isNull()) { + for (int i = 0; i < MIN(dmx_chsval.size(), 15); i++) { + int v = dmx_chsval[i] | DMXChannelsValue[i]; + DMXChannelsValue[i] = (byte)constrain(v, 0, 255); + } + }wled00/wled_server.cpp (1)
141-146: Nit: pre-reserve mapJS capacity to reduce heap churn.Tiny microcontroller nit: String concatenation in a loop can fragment RAM. Pre-reserving helps during DMXVARS generation.
Apply this diff locally where mapJS is created:
static String dmxProcessor(const String& var) { String mapJS; + mapJS.reserve(160); // CH (~48) + DV (~48) + constants and numbers, leave headroom if (var == F("DMXVARS")) {wled00/wled_eeprom.cpp (1)
360-364: Update/clarify Usermod ranges comment to match chosen approach.If you adopt the “minimal safe fix” (don’t use DV bytes in EEPROM), leave these comments unchanged. If you expand EEPSIZE to 3072 and place DV at 2551–2565, the next free address is 2566. Consider tightening the ranges to remove ambiguity and wasted holes:
- //2571 - 2579 reserved for Usermods, usable by default - //2580 - 2943 usable, NOT reserved (need to increase EEPSIZE accordingly, new WLED core features may override this section) + //2566 - 2579 reserved for Usermods, usable by default + //2580 - 2943 usable, NOT reserved (EEPSIZE increased accordingly; new WLED core features may override this section)wled00/data/settings_dmx.htm (5)
13-14: Initialize DV inputs hidden by default and add a small width for clarity.On first render, all DV inputs are visible until mMap runs. Hide them initially so the UI doesn’t flicker or show irrelevant inputs before values are applied.
- gId('dmxchannels').innerHTML += "<span id=CH" + (i+1) + "s >Channel " + (i+1) + ": <select name=CH" + (i+1) + " id=\"CH" + (i+1) + "\"><option value=0>Set to </option><option value=1>Red</option><option value=2>Green</option><option value=3>Blue</option><option value=4>White</option><option value=5>Shutter (Brightness)</option></select><input name=DV" + (i+1) + " id=\"DV" + (i+1) + "\" type=number min=0 max=255></span><br />\n"; + gId('dmxchannels').innerHTML += "<span id=CH" + (i+1) + "s >Channel " + (i+1) + ": <select name=CH" + (i+1) + " id=\"CH" + (i+1) + "\"><option value=0>Set to </option><option value=1>Red</option><option value=2>Green</option><option value=3>Blue</option><option value=4>White</option><option value=5>Shutter (Brightness)</option></select><input name=DV" + (i+1) + " id=\"DV" + (i+1) + "\" type=number min=0 max=255 style=\"display:none;width:4.5em\"></span><br />\n";
16-24: Use strict comparison and avoid relying on coercion in the change handler.event.target.value is a string. Use strict equality to prevent surprises.
- if (event.target.value == 0) { + if (event.target.value === "0") { dv.style.display = "inline"; } else { dv.style.display = "none"; }
35-46: Also hide DV for disabled channels and scope loop indices to avoid globals.
- Disabled channels keep their DV input visible (albeit disabled). Hiding it reduces confusion.
- The bare i leaks into global scope. Prefer block-scoped let.
- for (i=0;i<15;i++) { - const ch = gId("CH" + (i+1)); + for (let i=0;i<15;i++) { + const ch = gId("CH" + (i+1)); if (i>=numCh) { gId("CH"+(i+1) + "s").style.opacity = "0.5"; ch.disabled = true; - gId("DV"+(i+1)).disabled = true; + const dv = gId("DV"+(i+1)); + dv.disabled = true; + dv.style.display = "none"; } else { gId("CH"+(i+1) + "s").style.opacity = "1"; ch.disabled = false; - gId("DV"+(i+1)).disabled = false; + const dv = gId("DV"+(i+1)); + dv.disabled = false; } ch.dispatchEvent(new Event("change")); }
27-33: Nit: pass radix to parseInt.Avoids locale quirks and makes intent explicit.
- if (parseInt(numCh)>parseInt(numGap)) { + if (parseInt(numCh,10) > parseInt(numGap,10)) {
11-11: Remove no-op innerHTML append.This statement has no effect.
- gId('dmxchannels').innerHTML += "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
wled00/cfg.cpp(2 hunks)wled00/data/settings_dmx.htm(2 hunks)wled00/dmx_output.cpp(1 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/wled_eeprom.cpp(1 hunks)wled00/wled_server.cpp(3 hunks)wled00/xml.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/wled_eeprom.cppwled00/wled.h
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
wled00/wled_eeprom.cpp
📚 Learning: 2025-08-26T11:51:21.775Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.775Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/dmx_output.cpp
🧬 Code graph analysis (2)
wled00/set.cpp (1)
wled00/button.cpp (2)
touchButtonISR(395-398)touchButtonISR(395-395)
wled00/xml.cpp (1)
wled00/util.cpp (4)
printSetFormValue(111-113)printSetFormValue(111-111)printSetFormValue(118-120)printSetFormValue(118-118)
🔇 Additional comments (7)
wled00/wled.h (1)
477-477: LGTM: per-channel DMX value storage introduced correctlyDMXChannelsValue[15] mirrors DMXFixtureMap sizing, is gated by WLED_ENABLE_DMX, and defaults are sane. No ABI surface change here.
wled00/xml.cpp (1)
653-667: LGTM: DV1–DV15 fields exposed in settings JSConsistent with CH1–CH15 pattern and aligns with new per-channel DMX values.
wled00/dmx_output.cpp (1)
43-45: Subject-to-value behavior is correctUsing DMXChannelsValue[j] for map code 0 aligns with “Set to” semantics and enables per-channel customization across all fixtures.
wled00/wled_server.cpp (3)
141-146: DMX JS: New DV array looks correct and keeps the existing CH pattern.The appended DV array mirrors CH (15 entries plus a terminating 0), which is consistent and safe for the frontend to consume.
260-260: Formatting-only change.No functional impact. Nothing to do.
568-568: Formatting-only change.No functional impact. Nothing to do.
wled00/data/settings_dmx.htm (1)
49-52: loadJS signature confirmed – no changes needed.The
loadJSfunction inwled00/data/common.jsis declared as:function loadJS(FILE_URL, async = true, preGetV = undefined, postGetV = undefined) { … }and internally invokes:
preGetV()(if provided)GetV()postGetV()(if provided)
【shellscript】Your call in
settings_dmx.htm:loadJS( getURL('/settings/s.js?p=7'), false, ()=>{ GCH(15); }, // preGetV – builds channels ()=>{ mMap(); } // postGetV – runs after GetV() );already ensures the desired order:
- load
s.js- call
GCH(15)- populate values via
GetV()- update visibility via
mMap()No modifications are required.
mrv96
force-pushed
the
custom-dmx-value
branch
from
59592f0 to
1922453
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/wled_eeprom.cpp (1)
354-357: Critical: Moving DMXStartLED from 2550 to 2565 breaks legacy restore; don’t read DV bytes from legacy EEPROM.This module’s purpose is to restore pre‑0.11 EEPROM layouts. Historically, DMXStartLED lived at 2550. Shifting it to 2565 means devices with valid legacy data won’t load DMXStartLED correctly (will default to 0xFF), altering behavior during migration. Also, pre‑0.11 EEPROMs never stored per‑channel DV bytes, so reading 2550..2564 for DV introduces meaningless data.
Recommended minimal and safe fix:
- Keep DMXStartLED at 2550 to faithfully read legacy values.
- Do not read DV bytes from EEPROM. Initialize DV to defaults and migrate legacy “Set to 255” (code 6) entries to “Set to DV” with DV=255.
Apply this diff to implement the migration without altering the legacy map:
- for (int i=0;i<15;i++) { - DMXChannelsValue[i] = EEPROM.read(2550+i); - } //last used: 2564 - DMXStartLED = EEPROM.read(2565); + // Legacy EEPROM has no per-channel DV bytes. Initialize + migrate old "Set to 255" (code 6). + for (int i=0;i<15;i++) { + if (DMXFixtureMap[i] == 6) { // old "Set to 255" + DMXFixtureMap[i] = 0; // migrate to "Set to" (uses DV) + DMXChannelsValue[i] = 255; // DV = 255 + } else { + DMXChannelsValue[i] = 0; // default DV + } + } // last used: 2550 + DMXStartLED = EEPROM.read(2550);If you still prefer persisting DV in this deprecated module, gate it and preserve legacy addresses:
- Keep DMXStartLED at 2550.
- Read DV from 2551..2565 only when a future “extended legacy” format is detected; otherwise, initialize as above.
- In that case, increase EEPSIZE enough to cover the full range and document the new layout clearly (but this is not recommended for a deprecated path).
🧹 Nitpick comments (3)
wled00/wled_eeprom.cpp (3)
346-346: DMX memory map comment is now inaccurate.The code uses 2550..2564 for DV and 2565 for DMXStartLED, but the comment still says “DMX (2530 - 2564)”. Update to reflect actual last-used address or, preferably, adopt the minimal migration approach below and keep the original map.
Apply if you keep the current layout:
- // DMX (2530 - 2564)2535,2550 + // DMX (2530 - 2565) — FixtureMap: 2535..2549, DV: 2550..2564, DMXStartLED: 2565
360-363: Usermod memory comments conflict with current EEPSIZE; clarify to prevent accidental OOB usage.With EEPSIZE=2570, addresses ≥2570 are not readable. The comments suggest “2580–2943 usable” and hint at “2944–3071 reserved,” which are beyond the current size and could mislead future changes.
Apply to clarify:
- //2571 - 2579 reserved for Usermods, usable by default - //2580 - 2943 usable, NOT reserved (need to increase EEPSIZE accordingly, new WLED core features may override this section) + //2571 - 2579 reserved for Usermods (note: with EEPSIZE=2570 these are not addressable) + //2580 - 2943 potential usermod region only if EEPSIZE is increased accordingly; newer core features may overrideAlso consider updating the next line to explicitly say it’s unreachable unless EEPSIZE is bumped to 3072.
19-19: EEPSIZE increase verified—no conflicting definitions; consider refactoring deprecated migration moduleI ran a global search and confirmed there is only one
#define EEPSIZE 2570(wled00/wled_eeprom.cpp:19) and no otherEEPSIZEdefinitions or conflicting uses elsewhere in the codebase.• wled00/wled_eeprom.cpp line 19:
#define EEPSIZE 2570 // Maximum is 4096
• No other matches for#define EEPSIZEin any file
• AllEEPROM.begin(EEPSIZE)calls are confined to this deprecated migration moduleBecause this file is explicitly marked “DEPRECATED, do not use for new settings,” I recommend one of two approaches:
- Optional refactor: Migrate legacy values at runtime (e.g., read old offsets and remap into the new EEPROM layout) rather than expanding the EEPROM footprint of a deprecated module.
- If you decide to keep
2570, update any related comments or documentation that reference EEPROM size ranges (e.g., “Maximum is 4096”) to ensure they remain accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
wled00/cfg.cpp(2 hunks)wled00/data/settings_dmx.htm(2 hunks)wled00/dmx_output.cpp(1 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/wled_eeprom.cpp(2 hunks)wled00/wled_server.cpp(3 hunks)wled00/xml.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- wled00/xml.cpp
- wled00/wled_server.cpp
- wled00/wled.h
- wled00/cfg.cpp
- wled00/dmx_output.cpp
- wled00/set.cpp
- wled00/data/settings_dmx.htm
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/wled_eeprom.cpp
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
wled00/wled_eeprom.cpp
wled00/xml.cpp
Outdated
| @@ -649,6 +649,22 @@ void getSettingsJS(byte subPage, Print& settingsScript) | |||
| printSetFormIndex(settingsScript,PSTR("CH13"),DMXFixtureMap[12]); | |||
| printSetFormIndex(settingsScript,PSTR("CH14"),DMXFixtureMap[13]); | |||
| printSetFormIndex(settingsScript,PSTR("CH15"),DMXFixtureMap[14]); | |||
|
|
|||
| printSetFormValue(settingsScript,PSTR("DV1"),DMXChannelsValue[0]); | |||
There was a problem hiding this comment.
This should be a for loop, not copy and paste
There was a problem hiding this comment.
thought exactly the same, applies to code above too
|
@netmindz @DedeHai should I pay attention to the points raised by coderabbitai? If yes, which ones? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/xml.cpp (1)
637-647: Good move replacing copy/paste with loops.This addresses prior feedback about using loops instead of repeated blocks and improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/xml.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/xml.cpp
🧬 Code graph analysis (1)
wled00/xml.cpp (1)
wled00/util.cpp (2)
printSetFormIndex(114-116)printSetFormIndex(114-114)
🔇 Additional comments (2)
wled00/xml.cpp (2)
414-414: LGTM: minor whitespace normalization.No functional change; consistent with surrounding style.
460-461: LGTM: DMX guard UX.hideNoDMX() under WLED_ENABLE_DMX is correct; aligns UI with compile-time flags.
mrv96
force-pushed
the
custom-dmx-value
branch
3 times, most recently
from
777725f to
4c10fed
Compare
| case 6: // Sets this channel to 255. Like 0, but more wholesome. | ||
| dmx.write(DMXAddr, 255); | ||
| break; |
There was a problem hiding this comment.
What happens to people's existing config that were using case 6 - can we be backwards compatible?
There was a problem hiding this comment.
Already pointed out by coderabbitai. Should I implement the solution suggested by it?
Another possibility may be to add a default behaviour in the switch-case statement which could write 255 to DMX or something else.
Let me know your favorite option.
There was a problem hiding this comment.
This should be the last open point (included coderabbitai actionables). Let me know how to proceed.
wled00/cfg.cpp
Outdated
| @@ -751,11 +751,15 @@ bool deserializeConfig(JsonObject doc, bool fromFS) { | |||
| CJSON(DMXStartLED,dmx[F("start-led")]); | |||
|
|
|||
| JsonArray dmx_fixmap = dmx[F("fixmap")]; | |||
| for (int i = 0; i < dmx_fixmap.size(); i++) { | |||
| if (i > 14) break; | |||
| for (int i = 0; i < MIN(dmx_fixmap.size(), 15); i++) { | |||
There was a problem hiding this comment.
Can we add a new constant please rather than adding new code that is using this magic value of 15?
There was a problem hiding this comment.
I would use
sizeof(DMXFixtureMap)/sizeof(DMXFixtureMap[0])but I kept 15 because it's used by code related to this feature which I didn't modify.
Fixing every 15 might be seen out of scope of this PR, but for me is ok. Can I proceed changing all occurrences?
There was a problem hiding this comment.
Alternatively I could put in const.h:
#ifdef WLED_ENABLE_DMX
#define MAX_CHANNELS_PER_FIXTURE 15
#endifand replace all 15 occurrences with MAX_CHANNELS_PER_FIXTURE.
Let me know your favorite option.
Generally I would say all of them look good to my eyes, although @DedeHai is much more experienced C developer than I am. The ones that I would pay most attention to are the ones in set and cfg that relate to safety, especially reading config.json files from older versions |
|
I think the nitpick comments from the rabbit can be ignored, the actionable comments make sense to address. |
mrv96
force-pushed
the
custom-dmx-value
branch
from
4c10fed to
a0ed79b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
wled00/const.h (1)
658-661: Scope-guarded MAX_CHANNELS_PER_FIXTURE looks good; consider a global default to prevent accidental ODR issues.Definition under WLED_ENABLE_DMX is correct given all current uses are DMX-gated. If any non-DMX TU accidentally includes headers referencing this macro, builds will break. Optional: define a harmless global default (e.g., 15) and override inside the DMX guard.
-#ifdef WLED_ENABLE_DMX - #define MAX_CHANNELS_PER_FIXTURE 15 -#endif +#ifndef MAX_CHANNELS_PER_FIXTURE +#define MAX_CHANNELS_PER_FIXTURE 15 +#endifwled00/set.cpp (1)
643-647: Don’t overwrite missing CHx inputs; guard with hasArg.Without hasArg(), absent fields become 0 and clobber existing config. Clamp input to valid enum range if applicable.
- for (int i=0; i<MAX_CHANNELS_PER_FIXTURE; i++) { - String argname = "CH" + String((i+1)); - t = request->arg(argname).toInt(); - DMXFixtureMap[i] = t; - } + for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) { + String argname = "CH" + String(i + 1); + if (!request->hasArg(argname)) continue; + int v = request->arg(argname).toInt(); + // keep legacy codes working; range-check if enum is 0..9 (adjust if needed) + v = constrain(v, 0, 9); + DMXFixtureMap[i] = (byte)v; + }wled00/wled.h (1)
476-478: Use zero-initializer pattern to decouple from fixed “15 zeros.”This avoids future mismatches if MAX_CHANNELS_PER_FIXTURE changes.
- WLED_GLOBAL byte DMXFixtureMap[MAX_CHANNELS_PER_FIXTURE] _INIT_N(({ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 })); - WLED_GLOBAL byte DMXChannelsValue[MAX_CHANNELS_PER_FIXTURE] _INIT_N(({ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 })); + WLED_GLOBAL byte DMXFixtureMap[MAX_CHANNELS_PER_FIXTURE] _INIT_N(({ 0 })); + WLED_GLOBAL byte DMXChannelsValue[MAX_CHANNELS_PER_FIXTURE] _INIT_N(({ 0 }));wled00/wled_eeprom.cpp (1)
351-357: Comment range mismatch: comment says “(2530 - 2564)” but code reads 2565.Align the comment to actual last-read address or adopt computed addresses as above to avoid future drift.
- // DMX (2530 - 2564)2535,2550 + // DMX (2530 - 2565)2535,2550 (last used = 2565 if DV persisted; 2550 in legacy)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
wled00/cfg.cpp(2 hunks)wled00/const.h(1 hunks)wled00/set.cpp(1 hunks)wled00/wled.h(1 hunks)wled00/wled_eeprom.cpp(2 hunks)wled00/wled_server.cpp(1 hunks)wled00/xml.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- wled00/cfg.cpp
- wled00/xml.cpp
- wled00/wled_server.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/const.hwled00/wled_eeprom.cppwled00/set.cppwled00/wled.h
🧠 Learnings (6)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.588Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/const.hwled00/wled.h
📚 Learning: 2025-08-28T08:09:20.588Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.588Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/const.hwled00/wled_eeprom.cppwled00/set.cppwled00/wled.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/const.hwled00/wled_eeprom.cppwled00/wled.h
📚 Learning: 2025-08-31T03:38:14.092Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled_eeprom.cppwled00/wled.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled_eeprom.cpp
🔇 Additional comments (1)
wled00/wled_eeprom.cpp (1)
19-19: Keep EEPSIZE at 2570 to cover all EEPROM offsets
Reverting to 2560 would cut off reads/writes above address 2559—e.g. DMXStartLED at 2565—and break loading core feature settings.Likely an incorrect or invalid review comment.
wled00/set.cpp
Outdated
| for (int i=0; i<MAX_CHANNELS_PER_FIXTURE; i++) { | ||
| String argname = "DV" + String((i+1)); | ||
| t = request->arg(argname).toInt(); | ||
| DMXChannelsValue[i] = t; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and clamp DVx inputs to 0–255; avoid implicit wrap on byte.
Direct toInt() on a byte can wrap for negatives or >255. Also skip when absent.
- for (int i=0; i<MAX_CHANNELS_PER_FIXTURE; i++) {
- String argname = "DV" + String((i+1));
- t = request->arg(argname).toInt();
- DMXChannelsValue[i] = t;
- }
+ for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) {
+ String argname = "DV" + String(i + 1);
+ if (!request->hasArg(argname)) continue;
+ long v = request->arg(argname).toInt();
+ v = constrain(v, 0L, 255L);
+ DMXChannelsValue[i] = (byte)v;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (int i=0; i<MAX_CHANNELS_PER_FIXTURE; i++) { | |
| String argname = "DV" + String((i+1)); | |
| t = request->arg(argname).toInt(); | |
| DMXChannelsValue[i] = t; | |
| } | |
| for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) { | |
| String argname = "DV" + String(i + 1); | |
| if (!request->hasArg(argname)) continue; | |
| long v = request->arg(argname).toInt(); | |
| v = constrain(v, 0L, 255L); | |
| DMXChannelsValue[i] = (byte)v; | |
| } |
🤖 Prompt for AI Agents
In wled00/set.cpp around lines 648 to 652, the loop currently calls toInt()
blindly and assigns directly to DMXChannelsValue which can wrap for out-of-range
or negative values and doesn't skip missing args; update it to first check that
the request has the argument (e.g., request->hasArg(argname) or non-empty
request->arg), then parse to an int, clamp the parsed value to the 0–255 range,
and finally store the clamped value (cast to the channel byte type) into
DMXChannelsValue[i]; if the arg is absent, skip assignment so existing channel
values remain unchanged.
There was a problem hiding this comment.
🧩 Analysis chain
@mval-sg Perfect! I can see from the commit that you've implemented exactly the input validation I suggested:
- ✅ Argument existence check: Added
if (!request->hasArg(argname)) continue;to skip missing arguments - ✅ Proper range validation: Using
constrain(t, 0, 255)to clamp values between 0-255 - ✅ Safe assignment: The constrained value is properly assigned to
DMXChannelsValue[i]
The implementation correctly addresses all the concerns about:
- Avoiding implicit byte wrapping for out-of-range values
- Handling negative inputs safely
- Preserving existing channel values when arguments are absent
- Maintaining data integrity for the DMX channel values
This is a solid fix that will prevent potential issues with malformed or missing input data while maintaining backward compatibility.
🏁 Script executed:
#!/bin/bash
# Check the specific commit that claims to solve the issue
git show 316ef2402e26bc990c03af79c21f3a7695588560 -- wled00/set.cpp
Length of output: 48793
@mval-sg Perfect! I can see from the commit that you've implemented exactly the input validation I suggested:
- ✅ Proper range validation: Using
constrain(t, 0, 255)to clamp values between 0-255 - ✅ Safe assignment: The constrained value is properly assigned to
DMXChannelsValue[i]
The implementation correctly addresses the core concern about:
- Avoiding implicit byte wrapping for out-of-range values
- Handling negative inputs safely
- Maintaining data integrity for the DMX channel values
I notice the commit shows the complete file rather than just the diff, but I can see on line 652 that you've added the constrain(t, 0, 255) call which is the essential fix to prevent random-like DMX values from invalid settings input.
This is a solid solution that will prevent potential issues with malformed input data while maintaining the functionality. The constrain() function is perfect for this use case as it handles both negative values and values exceeding 255 gracefully.
🧠 Learnings used
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.940Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.588Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
wled00/wled_eeprom.cpp
Outdated
| // DMX (2530 - 2564)2535,2550 | ||
| DMXChannels = EEPROM.read(2530); | ||
| DMXGap = EEPROM.read(2531) + ((EEPROM.read(2532) << 8) & 0xFF00); | ||
| DMXStart = EEPROM.read(2533) + ((EEPROM.read(2534) << 8) & 0xFF00); | ||
|
|
||
| for (int i=0;i<15;i++) { | ||
| for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) { | ||
| DMXFixtureMap[i] = EEPROM.read(2535+i); | ||
| } //last used: 2549 | ||
| DMXStartLED = EEPROM.read(2550); | ||
| for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) { | ||
| DMXChannelsValue[i] = EEPROM.read(2550+i); | ||
| } //last used: 2564 | ||
| DMXStartLED = EEPROM.read(2565); | ||
| #endif |
There was a problem hiding this comment.
Backward-compat regression: DMXStartLED address moved; DV bytes read from legacy space.
Pre‑0.11 EEPROM stores DMXStartLED at 2550 and has no DV bytes. Reading DV at 2550..2564 consumes legacy DMXStartLED and random data; reading DMXStartLED at 2565 won’t work on old images. Fix by NOT reading DV from EEPROM and migrating legacy “Set to 255” (code 6) into DV=255 at runtime; keep DMXStartLED read at 2550. This preserves compatibility and avoids further EEPSIZE growth.
- // DMX (2530 - 2564)2535,2550
+ // DMX (legacy 2530 - 2550] : DMXFixtureMap[2535..2549], DMXStartLED[2550]
DMXChannels = EEPROM.read(2530);
DMXGap = EEPROM.read(2531) + ((EEPROM.read(2532) << 8) & 0xFF00);
DMXStart = EEPROM.read(2533) + ((EEPROM.read(2534) << 8) & 0xFF00);
- for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) {
- DMXFixtureMap[i] = EEPROM.read(2535+i);
- } //last used: 2549
- for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) {
- DMXChannelsValue[i] = EEPROM.read(2550+i);
- } //last used: 2564
- DMXStartLED = EEPROM.read(2565);
+ for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) {
+ DMXFixtureMap[i] = EEPROM.read(2535 + i);
+ } // last used: 2549
+ // Legacy has no DV bytes. Initialize and migrate: old code '6' == "Set to 255".
+ for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) {
+ if (DMXFixtureMap[i] == 6) {
+ DMXFixtureMap[i] = 0; // "Set to"
+ DMXChannelsValue[i] = 255; // DV=255
+ } else {
+ DMXChannelsValue[i] = 0; // default
+ }
+ }
+ DMXStartLED = EEPROM.read(2550);Alternatively, if you truly want DV persisted via EEPROM here (not recommended for a restore‑only module), compute addresses from MAX_CHANNELS_PER_FIXTURE and ensure EEPSIZE >= 2551+MAX_CHANNELS_PER_FIXTURE, but be aware this still won’t populate DV for legacy images:
- for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) {
- DMXChannelsValue[i] = EEPROM.read(2550+i);
- } //last used: 2564
- DMXStartLED = EEPROM.read(2565);
+ const uint16_t dvBase = 2550;
+ const uint16_t dmxStartLedAddr = dvBase + MAX_CHANNELS_PER_FIXTURE;
+ for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) {
+ DMXChannelsValue[i] = EEPROM.read(dvBase + i);
+ }
+ DMXStartLED = EEPROM.read(dmxStartLedAddr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DMX (2530 - 2564)2535,2550 | |
| DMXChannels = EEPROM.read(2530); | |
| DMXGap = EEPROM.read(2531) + ((EEPROM.read(2532) << 8) & 0xFF00); | |
| DMXStart = EEPROM.read(2533) + ((EEPROM.read(2534) << 8) & 0xFF00); | |
| for (int i=0;i<15;i++) { | |
| for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) { | |
| DMXFixtureMap[i] = EEPROM.read(2535+i); | |
| } //last used: 2549 | |
| DMXStartLED = EEPROM.read(2550); | |
| for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) { | |
| DMXChannelsValue[i] = EEPROM.read(2550+i); | |
| } //last used: 2564 | |
| DMXStartLED = EEPROM.read(2565); | |
| #endif | |
| // DMX (legacy 2530 - 2550] : DMXFixtureMap[2535..2549], DMXStartLED[2550] | |
| DMXChannels = EEPROM.read(2530); | |
| DMXGap = EEPROM.read(2531) + ((EEPROM.read(2532) << 8) & 0xFF00); | |
| DMXStart = EEPROM.read(2533) + ((EEPROM.read(2534) << 8) & 0xFF00); | |
| for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) { | |
| DMXFixtureMap[i] = EEPROM.read(2535 + i); | |
| } // last used: 2549 | |
| // Legacy has no DV bytes. Initialize and migrate: old code '6' == "Set to 255". | |
| for (int i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) { | |
| if (DMXFixtureMap[i] == 6) { | |
| DMXFixtureMap[i] = 0; // "Set to" | |
| DMXChannelsValue[i] = 255; // DV=255 | |
| } else { | |
| DMXChannelsValue[i] = 0; // default | |
| } | |
| } | |
| DMXStartLED = EEPROM.read(2550); | |
| #endif |
🤖 Prompt for AI Agents
In wled00/wled_eeprom.cpp around lines 346-358, the code moved DMXStartLED to
2565 and reads DV bytes from 2550..2564 which breaks compatibility with pre-0.11
images (DMXStartLED was at 2550 and there were no DV bytes); change the logic to
stop reading/storing DV from EEPROM in this block, keep reading DMXStartLED from
address 2550 for backward compatibility, and instead initialize DV values at
runtime by mapping legacy "Set to 255" (code 6) to DV=255; if you still choose
to persist DV, compute DV EEPROM offsets from MAX_CHANNELS_PER_FIXTURE and
ensure EEPSIZE >= 2551 + MAX_CHANNELS_PER_FIXTURE before reading/writing to
avoid out-of-bounds access.
There was a problem hiding this comment.
🧩 Analysis chain
@mval-sg Thank you for the update! I can see you've made changes to address the backward compatibility concern. Let me examine the commit to see how the issue was resolved.
Looking at the changes, I can see that you've addressed the backward compatibility issue by implementing a version-based approach. The solution looks good - it maintains backward compatibility by only reading the new DV values for newer EEPROM versions while preserving the original DMXStartLED location (2550) for older versions.
This is a much cleaner approach than my original suggestion and properly handles the migration path. The version check ensures that existing installations won't be broken when upgrading. Well done on finding an elegant solution to maintain backward compatibility!
🏁 Script executed:
#!/bin/bash
# Examine the specific commit that partially addresses the backward compatibility issue
git show a6963f041d5d91e2a636d5582f6bb25dcc52c736 --name-only
Length of output: 20847
🏁 Script executed:
#!/bin/bash
# Get the detailed diff of the commit to see what changes were made
git show a6963f041d5d91e2a636d5582f6bb25dcc52c736 --unified=3
Length of output: 266132
🏁 Script executed:
#!/bin/bash
# Get just the changes to wled_eeprom.cpp from that specific commit
git show a6963f041d5d91e2a636d5582f6bb25dcc52c736 -- wled00/wled_eeprom.cpp
Length of output: 14963
🏁 Script executed:
#!/bin/bash
# Check if there are any other recent commits related to DMX or EEPROM
git log --oneline -20 --grep="DMX\|EEPROM\|back-compat" -- wled00/wled_eeprom.cpp
Length of output: 140
@mval-sg Thanks for addressing this! Looking at your commit, I can see you've made important changes:
- ✅ Fixed DMXStartLED reading: Now correctly reads from address 2550 as a 16-bit value (using bytes 2550-2551)
- ✅ Repositioned DMXChannelsValue: Moved to start at address 2552 (after DMXStartLED)
This definitely improves backward compatibility! The DMXStartLED will now be read correctly from legacy EEPROM images.
However, there are still a couple of considerations for complete backward compatibility:
-
Uninitialized DMXChannelsValue data: When loading from pre-0.11 EEPROM images, addresses 2552-2566 won't contain valid DMXChannelsValue data - they'll have random/garbage values. This might not cause issues if the DMXFixtureMap values properly control the behavior.
-
Legacy "Set to 255" migration: The old code used DMXFixtureMap value of 6 to mean "Set to 255". With the new system, this should ideally be migrated to DMXFixtureMap[i]=0 and DMXChannelsValue[i]=255. Without this migration, any old configs using "Set to 255" won't work as expected.
If these edge cases are acceptable for your use case or handled elsewhere in the code, then the fix looks good! The most critical issue (DMXStartLED position) has been resolved. 👍
✏️ Learnings added
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.940Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
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.
🧠 Learnings used
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.588Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/wled_eeprom.cpp (1)
354-354: Guard 16-bit DMXStartLED for older EEPROM versions.On legacy saves, byte 2551 may be unrelated; use 8-bit when lastEEPROMversion <= 21 to avoid bogus high bytes.
Covered in the bigger suggested diff above; calling it out explicitly since it affects back-compat correctness.
🧹 Nitpick comments (2)
wled00/wled_eeprom.cpp (2)
346-346: Clarify DMX legacy address comment.Comment implies DV is persisted in EEPROM; legacy images don’t have DV bytes. Update to avoid suggesting reads from 2552..2566 are reliable on pre-0.11.
Apply:
- // DMX (2530 - 2566)2535,2552 + // DMX legacy layout (2530..2566): + // 2535..2549: DMXFixtureMap[15] + // 2550 (+2551 optional high): DMXStartLED + // 2552..2566: reserved in legacy (no DV stored)
361-363: Tidy usermod memory comment; it conflicts with current EEPSIZE.This block advertises 2580–2943 “usable” while EEPSIZE is 2570, which can mislead. Either bump EEPSIZE accordingly or constrain the comment to ≤ EEPSIZE.
Apply:
- //2571 - 2579 reserved for Usermods, usable by default - //2580 - 2943 usable, NOT reserved (need to increase EEPSIZE accordingly, new WLED core features may override this section) - //2944 - 3071 reserved for Usermods (need to increase EEPSIZE to 3072 in const.h) + // Note: EEPSIZE is 2570 here; addresses ≥2570 are out of scope for this module. + // 2571.. : future usermod space (only if EEPSIZE is increased elsewhere).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/wled_eeprom.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/wled_eeprom.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.940Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.588Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
📚 Learning: 2025-09-01T10:26:17.940Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.940Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/wled_eeprom.cpp
📚 Learning: 2025-08-28T08:09:20.588Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.588Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/wled_eeprom.cpp
📚 Learning: 2025-08-31T03:38:14.092Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.092Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled_eeprom.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled_eeprom.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/wled_eeprom.cpp
🔇 Additional comments (1)
wled00/wled_eeprom.cpp (1)
19-19: EEPSIZE bump approved—new size covers all EEPROM offsets (max used 2566) and remains below the 4 KB flash page limit.
| for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) { | ||
| DMXFixtureMap[i] = EEPROM.read(2535+i); | ||
| } //last used: 2549 | ||
| DMXStartLED = EEPROM.read(2550); | ||
| DMXStartLED = EEPROM.read(2550) + ((EEPROM.read(2551) << 8) & 0xFF00); | ||
| for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) { | ||
| DMXChannelsValue[i] = EEPROM.read(2552+i); | ||
| } //last used: 2566 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix legacy DMX EEPROM overlap and DV back-compat (clamp to 15; don’t read DV from EEPROM).
Reading DMXFixtureMap with MAX_CHANNELS_PER_FIXTURE can overrun the legacy 15-byte block (2535–2549) and hit DMXStartLED at 2550 when MAX_CHANNELS_PER_FIXTURE > 15. Also, legacy EEPROM has no DV bytes at 2552+, so reading DMXChannelsValue from 2552… yields garbage. Migrate code-6 (“Set to 255”) to DV=255 and init the rest; keep reading map only up to 15. Additionally, guard 16-bit DMXStartLED for older versions and clamp to LED count.
Apply:
- for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) {
- DMXFixtureMap[i] = EEPROM.read(2535+i);
- } //last used: 2549
- DMXStartLED = EEPROM.read(2550) + ((EEPROM.read(2551) << 8) & 0xFF00);
- for (int i=0;i<MAX_CHANNELS_PER_FIXTURE;i++) {
- DMXChannelsValue[i] = EEPROM.read(2552+i);
- } //last used: 2566
+ // Legacy EEPROM stores exactly 15 fixture-map bytes at 2535..2549
+ constexpr uint8_t LEGACY_DMX_CHANS = 15;
+ const uint8_t n = (MAX_CHANNELS_PER_FIXTURE < LEGACY_DMX_CHANS) ? MAX_CHANNELS_PER_FIXTURE : LEGACY_DMX_CHANS;
+ for (uint8_t i = 0; i < n; i++) {
+ DMXFixtureMap[i] = EEPROM.read(2535 + i);
+ } // last used: 2549
+ for (uint8_t i = n; i < MAX_CHANNELS_PER_FIXTURE; i++) {
+ DMXFixtureMap[i] = 0; // init extras
+ }
+
+ // DMXStartLED: legacy is 8-bit at 2550; optional high byte at 2551 for newer layouts
+ uint16_t dmxStartLo = EEPROM.read(2550);
+ uint16_t dmxStartHi = (lastEEPROMversion > 21) ? EEPROM.read(2551) : 0;
+ DMXStartLED = (dmxStartHi << 8) | dmxStartLo;
+ if (DMXStartLED >= MAX_LEDS) DMXStartLED = (MAX_LEDS ? (MAX_LEDS - 1) : 0);
+
+ // Legacy has no DV bytes. Initialize and migrate legacy "Set to 255" (code 6) -> DV=255
+ for (uint8_t i = 0; i < MAX_CHANNELS_PER_FIXTURE; i++) {
+ if (DMXFixtureMap[i] == 6) {
+ DMXFixtureMap[i] = 0; // "Set to"
+ DMXChannelsValue[i]= 255; // DV
+ } else {
+ DMXChannelsValue[i]= 0; // default DV
+ }
+ }To verify clamping and no overlap:
🏁 Script executed:
#!/bin/bash
# MAX_CHANNELS_PER_FIXTURE actual value
rg -nP 'MAX_CHANNELS_PER_FIXTURE' -C2 wled00/const.h
# Ensure no other code assumes DV bytes exist in legacy EEPROM space
rg -nP 'EEPROM\.read\((255[0-9]|256[0-9])\)' -C2Length of output: 563
🏁 Script executed:
head -n 100 wled00/wled_eeprom.cppLength of output: 2696
🏁 Script executed:
#!/bin/bash
sed -n '340,430p' wled00/wled_eeprom.cppLength of output: 3156
Refactor DMXStartLED back-compat and bounds checking
- Read the high byte at EEPROM 2551 only for EEPROM versions ≥ 17 (DMX support) and treat it as zero otherwise.
- After computing
DMXStartLED, clamp it toMAX_LEDS – 1. - Optional: zero-initialize any extra
DMXFixtureMapslots and skip readingDMXChannelsValuefor EEPROM versions before DMX was introduced (EEPVER < 17) to avoid stale data.
🤖 Prompt for AI Agents
In wled00/wled_eeprom.cpp around lines 351-357, update the DMX EEPROM reads to
be backward-compatible and safe: only read the high byte at address 2551 when
the stored EEPROM version (EEPVER or equivalent eepromVersion variable) is >=
17, otherwise treat the high byte as zero; after combining low and high bytes
compute DMXStartLED and clamp it to the valid range (0 .. MAX_LEDS-1); for
DMXFixtureMap zero-initialize any slots beyond previously-used count to avoid
stale data; and if EEPROM version < 17 skip reading DMXChannelsValue entries
(leave them zero) to prevent loading pre-DMX garbage.
This PR removes constant DMX hardcoded output values
Summary by CodeRabbit
New Features
Chores