Support custom DMX output value#4876
Conversation
WalkthroughIntroduce per‑fixture, per‑channel DMX default values and a size constant: add ChangesDMX per‑channel defaults (single cohesive cohort)
sequenceDiagram
participant SettingsUI as Client (Settings UI)
participant Server as WLED HTTP/Config
participant Config as Persistent Config
participant DMXOutput as DMX output task
participant Hardware as DMX Hardware
SettingsUI->>Server: POST CH1..CHN, DV1..DVN
Server->>Server: parse JSON -> update DMXFixtureMap[], DMXChannelsValue[] (bounded)
Server->>Config: serialize `fixmap` and `chsval`
Server->>SettingsUI: respond with DMXVARS JSON (CH[], DV[])
DMXOutput->>Server: DMX processing reads mappings and values
DMXOutput->>Hardware: write channel values (case 0 uses DMXChannelsValue[])
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
@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
netmindz
left a comment
netmindz
left a comment
There was a problem hiding this comment.
Generally looking good, a few minor points.
It would also be good if you remove your whitespace changes, but not 100% essential IMHO for this PR to be accepted
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.
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.
netmindz
added
connectivity
|
@netmindz all the open points have just been closed. Are code conflicts supposed to be solved by me or by repo maintainer? They would happen at any time depending on PR merging order. |
Preferably by you. It looks like I don't even have that option to fix for you at the moment, at least from the website. |
|
I already checked the option which enables maintainer to modify the branch. Indeed it seems that someone already merged upstream in my branch. Can you check if it works? Otherwise I'll rebase my branch on the current main |
mrv96
force-pushed
the
custom-dmx-value
branch
from
2e44ffd to
714c5cb
Compare
|
I've just solved all the conflicts. If everything is ok please merge ASAP to prevent the arise of new conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/data/settings_dmx.htm (1)
23-32: Consider refactoring the event listener setup for clarity.The current pattern captures the
dvvariable in each iteration's closure, which works correctly due to block scoping but is subtle. Consider making the relationship between channel select and DV input more explicit:♻️ Suggested refactor for clarity
for (i=0;i<num;i++) { - const dv = gId("DV" + (i+1)); - gId("CH" + (i+1)).addEventListener("change", (event) => { + gId("CH" + (i+1)).addEventListener("change", function(event) { + const channelNum = this.id.substring(2); // Extract channel number from "CHx" + const dv = gId("DV" + channelNum); if (event.target.value == 0) { dv.style.display = "inline"; } else { dv.style.display = "none"; } }); }This approach uses the channel select element itself to derive the DV input ID, making the relationship more explicit and reducing reliance on closure semantics.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wled00/cfg.cppwled00/const.hwled00/data/settings_dmx.htmwled00/dmx_output.cppwled00/set.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/const.h
- wled00/wled.h
- wled00/cfg.cpp
- wled00/xml.cpp
🧰 Additional context used
📓 Path-based instructions (4)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_dmx.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_dmx.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_dmx.htm
🧠 Learnings (27)
📓 Common learnings
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cppwled00/data/settings_dmx.htm
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-12-28T14:06:48.772Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/dmx_output.cppwled00/set.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/dmx_output.cppwled00/wled_server.cppwled00/set.cpp
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/dmx_output.cppwled00/set.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-05-26T16:09:34.325Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-11T17:56:24.923Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue #4929 to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2026-01-01T13:18:14.738Z
Learnt from: softhack007
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:225-227
Timestamp: 2026-01-01T13:18:14.738Z
Learning: In WLED effects, when computing time differences between unsigned long timestamps (e.g., `strip.now`, `lastBumpUpdate`), always cast the subtraction result to `int` before converting to `float` to ensure correct behavior on ESP32-C3 (RISC-V). Use pattern: `float(int(timestamp1 - timestamp2))` instead of `float(timestamp1 - timestamp2)`.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/settings_dmx.htm
🧬 Code graph analysis (1)
wled00/wled_server.cpp (1)
wled00/ota_update.cpp (2)
initOTA(156-166)initOTA(156-156)
🔇 Additional comments (4)
wled00/dmx_output.cpp (1)
43-44: LGTM! Per-channel custom DMX values correctly implemented.The change successfully replaces hardcoded constant 0 with per-channel configurable values from
DMXChannelsValue[j], while maintaining backward compatibility for legacy "Set to 255" configurations through case 6.Also applies to: 61-62
wled00/set.cpp (2)
643-647: Good practice: replaced magic number with constant.The use of
WLED_DMX_MAX_CHANNELS_PER_FIXTUREinstead of a hardcoded loop bound improves maintainability and aligns with coding best practices.Based on learnings, eliminating magic numbers by using defined constants is a recommended practice in WLED code reviews.
648-652: LGTM! DV input processing correctly implemented.The parallel loop structure for DV (default value) inputs is consistent with the CH processing above, and
constrain(t, 0, 255)appropriately ensures values remain within the valid DMX range.wled00/wled_server.cpp (1)
147-155: LGTM! DMX serialization correctly updated for per-channel values.The changes properly emit both the CH (channel map) and new DV (default values) arrays to the client, with consistent formatting and correct loop bounds.
|
the settings_dmx still has a couple of references to the 15-channel limit. Not sure how to fix it to use a variable that's pulled from the length of the array, but been playing with your branch tonight to try to get it to work (was unsuccessful thus far as it stopped loading the /json/fxdata, /json/effects and /json/si when I got the html side hard coded to my specific channel count). https://github.com/mrv96/WLED/blob/714c5cbc0987a4b48b9af8955f032204fe33e49b/wled00/data/settings_dmx.htm#L42C14-L42C16 |
|
The scope of this PR is to remove the limitation of playing only 0 or 255 in dmx channels with a fixed value. To solve other issues should be better to open a dedicated PR after merging this one. |
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
Bot
added
the
stale
|
@netmindz this PR looks ready to be merged. Can you merge it please? |
softhack007
removed
stale
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/xml.cpp (1)
681-691: ⚡ Quick winIncrease buffer size to handle larger channel numbers.
Buffer size of 5 accommodates channel IDs up to 99. If
WLED_DMX_MAX_CHANNELS_PER_FIXTUREgrows beyond 99,snprintf_Pwill truncate, causing form ID mismatches in the UI.🛡️ Increase buffer to 8 bytes
for (int i = 0; i < WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) { - char buf[5]; + char buf[8]; // accommodate up to 999 channels snprintf_P(buf, sizeof(buf), PSTR("CH%d"), i+1); printSetFormIndex(settingsScript,buf,DMXFixtureMap[i]); } for (int i = 0; i < WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) { - char buf[5]; + char buf[8]; // accommodate up to 999 channels snprintf_P(buf, sizeof(buf), PSTR("DV%d"), i+1); printSetFormValue(settingsScript,buf,DMXChannelsValue[i]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/xml.cpp` around lines 681 - 691, The fixed-size char buf[5] used when formatting "CH%d" and "DV%d" can overflow/truncate when WLED_DMX_MAX_CHANNELS_PER_FIXTURE exceeds two digits; increase the buffer to at least 8 bytes and keep using snprintf_P with sizeof(buf) to safely format the channel IDs in the two loops that call printSetFormIndex(settingsScript, buf, DMXFixtureMap[i]) and printSetFormValue(settingsScript, buf, DMXChannelsValue[i]); this ensures "CHxxx"/"DVxxx" IDs are not truncated for larger channel counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wled00/xml.cpp`:
- Around line 681-691: The fixed-size char buf[5] used when formatting "CH%d"
and "DV%d" can overflow/truncate when WLED_DMX_MAX_CHANNELS_PER_FIXTURE exceeds
two digits; increase the buffer to at least 8 bytes and keep using snprintf_P
with sizeof(buf) to safely format the channel IDs in the two loops that call
printSetFormIndex(settingsScript, buf, DMXFixtureMap[i]) and
printSetFormValue(settingsScript, buf, DMXChannelsValue[i]); this ensures
"CHxxx"/"DVxxx" IDs are not truncated for larger channel counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1334162a-e38b-4a86-a73e-daad1477187e
📒 Files selected for processing (6)
wled00/cfg.cppwled00/const.hwled00/set.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- wled00/wled.h
- wled00/const.h
- wled00/set.cpp
- wled00/cfg.cpp
- wled00/wled_server.cpp
|
@coderabbitai re-review this PR. Summarize suggested improvements in a prioritized list. |
🧩 Analysis chain🏁 Script executed: Length of output: 11998
🔴 Priority 1 — Critical (Correctness & Backward Compatibility)1.
|
| # | File | Severity | Issue |
|---|---|---|---|
| 1 | dmx_output.cpp / xml.cpp / set.cpp |
🔴 Critical | Case-6 backward compat break — old "Set to 255" channels silently become 0 |
| 2 | set.cpp + settings_dmx.htm |
🔴 Critical | Disabled DV inputs not submitted → DMXChannelsValue reset to 0 on every save |
| 3 | cfg.cpp |
🟡 Moderate | DMXChannelsValue not clamped to 0–255 when loaded from JSON |
| 4 | settings_dmx.htm |
🟡 Moderate | mMap hardcodes 15 instead of using dynamic channel count |
| 5 | wled_server.cpp |
🟡 Moderate | Double semicolon ;; in emitted JavaScript |
| 6 | xml.cpp |
🟢 Minor | buf[5] fragile if constant ever exceeds 99 |
| 7 | const.h |
🟢 Minor | Constant defined outside #ifdef WLED_ENABLE_DMX guard |
| 8 | wled.h |
🟢 Minor | Hardcoded 15 literal zeros in initializer lists |
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/set.cpp (1)
700-704:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add missing hasArg() check for DVx inputs (regression from previous fix).
Lines 700-704 read DVx form arguments without checking their existence. Missing arguments will be read as empty strings, converted to 0 by
toInt(), then clamped to 0 and assigned toDMXChannelsValue[i]. This overwrites existing channel values with 0 for any form fields not submitted.Note: This issue was previously raised and marked as solved in commit 316ef24, where both
hasArg()check andconstrain()were added. The current code shows onlyconstrain(), suggesting thehasArg()check was lost—possibly during the merge conflict resolution mentioned in PR comments. The fix needs to be re-applied.🛡️ Proposed fix to skip missing arguments
for (int i=0; i<WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) { String argname = "DV" + String((i+1)); + if (!request->hasArg(argname)) continue; t = request->arg(argname).toInt(); DMXChannelsValue[i] = constrain(t, 0, 255); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/set.cpp` around lines 700 - 704, The loop that reads DVx form fields (for int i=0; i<WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) is missing a request->hasArg(...) check so absent DVx fields are being read as 0 and overwrite DMXChannelsValue[i]; update the loop to first check request->hasArg(argname) (where argname is "DV"+String((i+1))) and only call request->arg(argname).toInt() and assign DMXChannelsValue[i] = constrain(t,0,255) when the arg exists, otherwise leave DMXChannelsValue[i] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/set.cpp`:
- Around line 695-699: The loop that sets DMXFixtureMap currently calls
request->arg(argname).toInt() for every CHx and unconditionally writes the
result, which overwrites existing mappings when the form omits an argument;
modify the loop to first check request->hasArg(argname) (using the same argname
"CH" + String((i+1))) and only parse and assign DMXFixtureMap[i] when hasArg
returns true, otherwise skip assignment so the existing DMXFixtureMap[i] value
is preserved.
---
Duplicate comments:
In `@wled00/set.cpp`:
- Around line 700-704: The loop that reads DVx form fields (for int i=0;
i<WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) is missing a request->hasArg(...)
check so absent DVx fields are being read as 0 and overwrite
DMXChannelsValue[i]; update the loop to first check request->hasArg(argname)
(where argname is "DV"+String((i+1))) and only call
request->arg(argname).toInt() and assign DMXChannelsValue[i] =
constrain(t,0,255) when the arg exists, otherwise leave DMXChannelsValue[i]
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 648ec52a-95b8-4fc0-aad1-a749bdc15a05
📒 Files selected for processing (6)
wled00/cfg.cppwled00/const.hwled00/set.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
✅ Files skipped from review due to trivial changes (1)
- wled00/const.h
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/wled.h
- wled00/xml.cpp
- wled00/cfg.cpp
- wled00/wled_server.cpp
| for (int i=0; i<WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) { | ||
| String argname = "CH" + String((i+1)); | ||
| t = request->arg(argname).toInt(); | ||
| DMXFixtureMap[i] = t; | ||
| } |
There was a problem hiding this comment.
Add hasArg() check to preserve existing CHx mappings when arguments are absent.
Lines 695-699 read CHx form arguments directly without checking whether they exist. When an argument is missing, request->arg(argname) returns an empty string, toInt() returns 0, and the code overwrites DMXFixtureMap[i] with 0. This will reset any channel mappings that weren't included in the form submission, which is likely unintended.
Add an existence check so that only present arguments update the mapping, preserving existing values for absent ones.
🛡️ Proposed fix to skip missing arguments
for (int i=0; i<WLED_DMX_MAX_CHANNELS_PER_FIXTURE; i++) {
String argname = "CH" + String((i+1));
+ if (!request->hasArg(argname)) continue;
t = request->arg(argname).toInt();
DMXFixtureMap[i] = t;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/set.cpp` around lines 695 - 699, The loop that sets DMXFixtureMap
currently calls request->arg(argname).toInt() for every CHx and unconditionally
writes the result, which overwrites existing mappings when the form omits an
argument; modify the loop to first check request->hasArg(argname) (using the
same argname "CH" + String((i+1))) and only parse and assign DMXFixtureMap[i]
when hasArg returns true, otherwise skip assignment so the existing
DMXFixtureMap[i] value is preserved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/cfg.cpp (1)
747-747:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp deserialized
DMXChannelsto the fixture-map array bound.
DMXChannelsis read from config but not bounded here. If a config value exceedsWLED_DMX_MAX_CHANNELS_PER_FIXTURE, later DMX output loops can read pastDMXFixtureMap[]/DMXChannelsValue[].🔧 Proposed fix
CJSON(DMXChannels, dmx[F("chan")]); + DMXChannels = constrain(DMXChannels, 1, WLED_DMX_MAX_CHANNELS_PER_FIXTURE); CJSON(DMXGap,dmx[F("gap")]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/cfg.cpp` at line 747, The deserialized DMXChannels value from CJSON(DMXChannels, dmx[F("chan")]) isn’t clamped and may exceed the DMXFixtureMap/DMXChannelsValue arrays; after reading DMXChannels, clamp it to the maximum allowed using WLED_DMX_MAX_CHANNELS_PER_FIXTURE (e.g., DMXChannels = min(DMXChannels, WLED_DMX_MAX_CHANNELS_PER_FIXTURE)) so subsequent DMX output loops that index DMXFixtureMap[]/DMXChannelsValue[] cannot read out of bounds; apply the clamp immediately after the CJSON read for DMXChannels.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wled00/cfg.cpp`:
- Line 747: The deserialized DMXChannels value from CJSON(DMXChannels,
dmx[F("chan")]) isn’t clamped and may exceed the DMXFixtureMap/DMXChannelsValue
arrays; after reading DMXChannels, clamp it to the maximum allowed using
WLED_DMX_MAX_CHANNELS_PER_FIXTURE (e.g., DMXChannels = min(DMXChannels,
WLED_DMX_MAX_CHANNELS_PER_FIXTURE)) so subsequent DMX output loops that index
DMXFixtureMap[]/DMXChannelsValue[] cannot read out of bounds; apply the clamp
immediately after the CJSON read for DMXChannels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 142d2620-0031-4770-8d1b-8ef829b9524f
📒 Files selected for processing (8)
wled00/cfg.cppwled00/const.hwled00/data/settings_dmx.htmwled00/dmx_output.cppwled00/set.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
@mrv96 @mval-sg in case you want to add changes to your PR, please don't force-push. It looks like we lost review comments, and I just re-started the review bot because it also got confused. https://github.com/wled/WLED?tab=contributing-ov-file#updating-your-code |
@coderabbitai this recommendation is a false-positive in our repo: we generally allow such constants to be defined, even when they are not used in some configurations. Constants (#define or constexpr) do not increase the binary size, but it generally makes the code more readable if they are not guarded by a feature flag. See #4876 (comment) |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
@softhack007 ok, I'll never do it again. Should I do something now? |
yes, do look into the remaining review comments, plus the list created by the rabbit. Check if these need a fix. Commit a fix, or make a short comment why no fix is needed. Then test on your own hardware, and report back when you have tested successfully. Final step: wait because we usually expect two maintainers to approve before merging. |
7 participants
This PR removes constant DMX hardcoded output values
Summary by CodeRabbit
New Features
Improvements