Channel reorg #109
Channel reorg #109
Conversation
Frontend ======== - monitor: header reorg backend ======= - Nodes: lightPreset_MH19x15W24: has a white channel - WIP - Physical layer: add nrOfLights_t_MAX, reorg header, 'park' white calculation in loopDrivers - WIP - Virtual layer: add forEachLightIndex, set/get functions use forEachLightIndex - Lights Control, use updated header structure - MoonLight effects: remove white in solid and fixed rectangle
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Header Structure & Layout src/MoonLight/Layers/PhysicalLayer.h, src/MoonLight/Layers/PhysicalLayer.cpp |
Rewrote LightsHeader public layout: added isPositions, channelsPerLight, explicit offset fields (offsetRGB, offsetRed, offsetGreen, offsetBlue, offsetWhite, offsetBrightness, pan/tilt/zoom/rotate/gobo, RGB1/2/3, brightness2), added nrOfLights_t_MAX macro, updated reset/defaults and public header size comment. |
Monitor & Protocol Header interface/src/routes/moonbase/monitor/Monitor.svelte, src/MoonLight/Modules/ModuleLightsControl.h |
Increased monitor/protocol header length checks from 41 → 47; adjusted parsing offsets for lightPreset, channelsPerLight, offsetRGB, offsetWhite, and related static assertions. |
Virtual Layer API & Mapping src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp |
Added forEachLightIndex iterator; introduced new setLight(indexV, offset, value) and getLight(indexV, offset) APIs; reimplemented getRGB/getRGB1/2/3 and RGB setters to aggregate/write across mapped physical lights; removed older pointer-based set/get methods. |
Call Site Updates & Drivers src/MoonLight/Modules/ModuleChannels.h, src/MoonLight/Nodes/Drivers/D_ArtnetIn.h, src/MoonBase/Nodes.cpp |
Updated all setLight call sites to new signature (indexV, offset, value); replaced some direct calls with forEachLightIndex lambdas and memcpy loops in Art-Net/DDP handlers; adjusted preset header offset initializations in DriverNode (moved/init offsetRGB, added offsetWhite for one preset). |
Effects / White Channel Removal src/MoonLight/Nodes/Effects/E_MoonLight.h |
Removed public white members and associated UI registration/usage from SolidEffect and FixedRectangleEffect. |
Commented Reference Code src/MoonLight/Layers/PhysicalLayer.cpp |
Added a commented-out white-channel stripping routine adjacent to layout mapping (no functional change). |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- Board presets p4 #74 — Direct overlap: LightsHeader layout redesign and monitor/header parsing offset adjustments.
- starsky effect #96 — Related API change to VirtualLayer
setLightand its call sites. - Board preset add presets #70 — Related VirtualLayer mapping and representation changes affecting mapping iteration.
Poem
🐰 In headers grown and offsets clear,
I hop through mappings far and near.
ForEachLight guides each tiny sprite,
White fades from two effects' sight.
Cheers — the lights align tonight! ✨
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 7.55% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The title 'Channel reorg' is vague and generic. It does not convey specific information about what channel reorganization occurred or which components were affected, making it difficult for teammates to understand the main change from the title alone. | Use a more descriptive title that specifies the nature of the reorganization, such as 'Refactor channel offset handling in light header and DMX mapping' or 'Reorganize light channel offsets and virtual layer mapping'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/MoonBase/Nodes.cpp`:
- Around line 526-529: The preset lightPreset_MH19x15W24 currently assigns both
header->offsetWhite and header->offsetBrightness to channel 3, which likely
conflicts; verify the MH 19x15W-24 DMX chart to confirm whether channel 3 is a
shared white/master brightness or if white is not present. If white is not a
separate channel, set header->offsetWhite = UINT8_MAX (disabled) for
lightPreset_MH19x15W24; otherwise change header->offsetWhite to the correct
channel number per the fixture manual so it no longer duplicates
header->offsetBrightness.
In `@src/MoonLight/Layers/VirtualLayer.h`:
- Around line 214-219: The getLight function leaves local variable value
uninitialized if forEachLightIndex never calls the callback; initialize value to
a safe default (e.g., 0) before the forEachLightIndex call so getLight always
returns a defined uint8_t. Update the getLight implementation (referencing
getLight and forEachLightIndex) to assign a default to value prior to invoking
the lambda and then proceed with the existing callback-based assignment.
- Around line 245-263: getRGB1/getRGB2/getRGB3 currently memcpy from
layerP->lights.channelsE using offsets that may be UINT8_MAX (disabled) or
out-of-range; add guards to return a defined default color when mapping is
missing or invalid. For each of getRGB1/getRGB2/getRGB3, check
layerP->lights.header.offsetRGB1/offsetRGB2/offsetRGB3 != UINT8_MAX and ensure
offset + sizeof(CRGB) <= layerP->lights.header.channelsPerLight before calling
forEachLightIndex/memcpy; if the check fails return a default CRGB (e.g.,
{0,0,0}) immediately. Ensure the memcpy remains inside the existing lambda but
only executed when the checks pass.
- Around line 112-142: The unmapped branch of forEachLightIndex passes a channel
index (computed as indexV * channelsPerLight) to callback, but callback expects
a physical light index; change the unmapped branch to pass the light index
(indexV) instead and keep the same bounds check using
layerP->lights.header.channelsPerLight and layerP->lights.maxChannels to ensure
the light exists. Locate the unmapped branch in forEachLightIndex (check
mappingTableSize, layerP->lights.header.channelsPerLight,
layerP->lights.maxChannels) and replace the callback(index) call with
callback(indexV) while preserving the existing bounds guard.
🧹 Nitpick comments (1)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
123-134: Potential offset calculation issue in commented white-channel stripping code.When this code is enabled, the offset calculation may be incorrect. The code sets
lightto point atoffsetRGBwithin the light's channels, then accesseslight[offsetRed],light[offsetGreen], etc. IfoffsetRed/Green/Blueare absolute offsets from the light's start (which appears to be the convention elsewhere), this would double-count the offset.For example, with RGBW preset:
offsetRGB=0,offsetRed=0,offsetGreen=1,offsetBlue=2,offsetWhite=3. The current code would work. But for WRGB:offsetRGB=?,offsetWhite=0,offsetRed=1, etc. — the semantics become unclear.Consider clarifying the offset semantics before enabling this code:
Suggested clarification
- uint8_t* light = &lights.channelsD[indexP * lights.header.channelsPerLight + lights.header.offsetRGB]; - uint8_t white = MIN(MIN(light[lights.header.offsetRed], light[lights.header.offsetGreen]), light[lights.header.offsetBlue]); + uint8_t* light = &lights.channelsD[indexP * lights.header.channelsPerLight]; + uint8_t white = MIN(MIN(light[lights.header.offsetRed], light[lights.header.offsetGreen]), light[lights.header.offsetBlue]);
src/MoonBase/Nodes.cpp
Outdated
| header->offsetWhite = 3; | ||
| header->offsetPan = 0; | ||
| header->offsetTilt = 1; | ||
| header->offsetBrightness = 3; |
There was a problem hiding this comment.
Potential conflict: offsetWhite and offsetBrightness both set to 3.
Both offsetWhite (line 526) and offsetBrightness (line 529) are assigned to channel 3 for the lightPreset_MH19x15W24 preset. This may cause unintended behavior if code writes to both offsets expecting different channels.
If channel 3 serves dual purpose (brightness control that also affects white output), this might be intentional. Otherwise, one of these assignments may be incorrect.
Please verify the DMX channel mapping for the MH 19x15W-24 moving head fixture:
- Is channel 3 used for both white and master brightness?
- Or should
offsetWhitebeUINT8_MAX(disabled) for this fixture?
🤖 Prompt for AI Agents
In `@src/MoonBase/Nodes.cpp` around lines 526 - 529, The preset
lightPreset_MH19x15W24 currently assigns both header->offsetWhite and
header->offsetBrightness to channel 3, which likely conflicts; verify the MH
19x15W-24 DMX chart to confirm whether channel 3 is a shared white/master
brightness or if white is not present. If white is not a separate channel, set
header->offsetWhite = UINT8_MAX (disabled) for lightPreset_MH19x15W24; otherwise
change header->offsetWhite to the correct channel number per the fixture manual
so it no longer duplicates header->offsetBrightness.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/MoonLight/Layers/VirtualLayer.h`:
- Around line 220-235: The local variable color in getRGB may be returned
uninitialized if the forEachLightIndex callback never runs; initialize color to
a safe default (e.g., CRGB::Black) before calling forEachLightIndex so that the
returned value is always well-defined; update the code path under the else
branch (where forEachLightIndex, layerP->lights.channelsE, and
channelsPerLight/offsetRGB are used) to set CRGB color = CRGB::Black; then leave
the forEachLightIndex lambda unchanged so it can overwrite color when invoked.
- Around line 112-141: In forEachLightIndex, the unmapped-branch bounds check
incorrectly uses indexV * (channelsPerLight + 1), which can reject valid tail
lights; replace that math with a proper channel base calculation and compare its
end against maxChannels: compute channelBase = indexV *
layerP->lights.header.channelsPerLight and use channelBase +
layerP->lights.header.channelsPerLight <= layerP->lights.maxChannels (then call
callback(indexV) as before). Update the else branch in forEachLightIndex to use
these symbols (indexV, layerP->lights.header.channelsPerLight,
layerP->lights.maxChannels) so valid lights near the end are not dropped.
♻️ Duplicate comments (1)
src/MoonBase/Nodes.cpp (1)
521-534: VerifyoffsetWhitevsoffsetBrightnesschannel overlap for MH19x15W24.Line 527 sets
offsetWhite = 3while Line 530 setsoffsetBrightness = 3. If channel 3 isn’t a shared white/brightness control per the fixture’s DMX chart, this will conflict. Please confirm the mapping and adjustoffsetWhite(or disable it) accordingly.
| template <typename Callback> | ||
| void forEachLightIndex(const nrOfLights_t indexV, Callback&& callback, bool onlyOne = false) { | ||
| if (indexV < mappingTableSize) { | ||
| switch (mappingTable[indexV].mapType) { | ||
| case m_oneLight: { | ||
| nrOfLights_t indexP = mappingTable[indexV].indexP; | ||
| // if (layerP->lights.header.lightPreset == lightPreset_RGB2040) { | ||
| // indexP += (indexP / 20) * 20; | ||
| // } | ||
| callback(indexP); | ||
| break; | ||
| } | ||
| case m_moreLights: | ||
| if (mappingTable[indexV].indexesIndex < mappingTableIndexes.size()) { | ||
| for (nrOfLights_t indexP : mappingTableIndexes[mappingTable[indexV].indexesIndex]) { | ||
| // if (layerP->lights.header.lightPreset == lightPreset_RGB2040) { | ||
| // indexP += (indexP / 20) * 20; | ||
| // } | ||
| callback(indexP); | ||
| if (onlyOne) return; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } else { // no mappnig | ||
| if (indexV * (layerP->lights.header.channelsPerLight + 1) <= layerP->lights.maxChannels) { // make sure the light is in the channels array | ||
| callback(indexV); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix unmapped bounds guard math (skips valid lights).
Line 137 uses (channelsPerLight + 1) which rejects valid indices and can silently drop updates near the tail. Use channelBase + channelsPerLight <= maxChannels to match the intended bounds check.
🐛 Proposed fix
- if (indexV * (layerP->lights.header.channelsPerLight + 1) <= layerP->lights.maxChannels) { // make sure the light is in the channels array
- callback(indexV);
- }
+ const uint32_t channelBase = indexV * layerP->lights.header.channelsPerLight;
+ if (channelBase + layerP->lights.header.channelsPerLight <= layerP->lights.maxChannels) { // make sure the light is in the channels array
+ callback(indexV);
+ }🤖 Prompt for AI Agents
In `@src/MoonLight/Layers/VirtualLayer.h` around lines 112 - 141, In
forEachLightIndex, the unmapped-branch bounds check incorrectly uses indexV *
(channelsPerLight + 1), which can reject valid tail lights; replace that math
with a proper channel base calculation and compare its end against maxChannels:
compute channelBase = indexV * layerP->lights.header.channelsPerLight and use
channelBase + layerP->lights.header.channelsPerLight <=
layerP->lights.maxChannels (then call callback(indexV) as before). Update the
else branch in forEachLightIndex to use these symbols (indexV,
layerP->lights.header.channelsPerLight, layerP->lights.maxChannels) so valid
lights near the end are not dropped.
There was a problem hiding this comment.
indexV will be used to point to the beginning of the array of channels for one light, so I need to make sure whatever is done withhin that light is not accessing channels out of the bounds. Agree?
There was a problem hiding this comment.
@ewowi, Yes, your principle is correct—you need to ensure all channels for light indexV stay within bounds. However, the current formula doesn't achieve that.
The issue:
- Light
indexVhas channels from positionindexV * channelsPerLighttoindexV * channelsPerLight + channelsPerLight - 1 - Current code:
indexV * (channelsPerLight + 1)expands toindexV * channelsPerLight + indexV - This incorrectly adds
indexVinstead ofchannelsPerLightto the base offset
Example where it fails:
- Say
maxChannels = 300,channelsPerLight = 3,indexV = 99 - Light 99's channels occupy positions 297, 298, 299 (all valid)
- Current check:
99 * 4 = 396 <= 300→ fails (rejects valid light) - Correct check:
99 * 3 + 3 = 300 <= 300→ passes ✓
The fix:
const uint32_t channelBase = indexV * layerP->lights.header.channelsPerLight;
if (channelBase + layerP->lights.header.channelsPerLight <= layerP->lights.maxChannels) {This correctly ensures the last channel at channelBase + channelsPerLight - 1 is < maxChannels. Does this clarify the math?
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:06.470Z
Learning: In MoonLight: VirtualLayer.size is ONLY synchronized with PhysicalLayer.lights.header.size during pass 2 (VirtualLayer::onLayoutPre at line 349 sets size = layerP->lights.header.size). If pass 2 (requestMapVirtual) doesn't run, VirtualLayer.size remains at its default {16, 16, 1}, causing effects to allocate buffers and iterate over the wrong dimensions. Pass 2 must always run after pass 1 to ensure VirtualLayer gets the correct size from PhysicalLayer.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T15:13:55.357Z
Learning: In MoonLight effectTask (src/main.cpp): The memcpy(channelsE, channelsD) at line 137 happens AFTER releasing swapMutex, allowing driverTask to swap the channelsD/channelsE pointers mid-copy, causing buffer corruption. This specifically breaks effects like GameOfLife that use getRGB() to read previous-frame data for blending decisions. Other effects work because they only read current-frame data they just wrote. Fix: hold swapMutex during the entire memcpy operation to prevent pointer swap during copy.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR `#84`: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
| CRGB getRGB(const nrOfLights_t indexV) { | ||
| if (mappingTable[indexV].mapType == m_zeroLights) { | ||
| #ifdef BOARD_HAS_PSRAM | ||
| return *(CRGB*)&mappingTable[indexV].rgb; | ||
| #else | ||
| CRGB result; | ||
| ((uint8_t*)&result)[0] = (mappingTable[indexV].rgb >> 9) << 3; // R: bits [13:9] | ||
| ((uint8_t*)&result)[1] = ((mappingTable[indexV].rgb >> 4) & 0x1F) << 3; // G: bits [8:4] | ||
| ((uint8_t*)&result)[2] = (mappingTable[indexV].rgb & 0x0F) << 4; // B: bits [3:0] | ||
| return result; | ||
| #endif | ||
| } else { | ||
| CRGB color; | ||
| forEachLightIndex(indexV, [&](nrOfLights_t indexP) { memcpy(&color, &layerP->lights.channelsE[indexP * layerP->lights.header.channelsPerLight + layerP->lights.header.offsetRGB], sizeof(color)); }, true); | ||
| return color; | ||
| } |
There was a problem hiding this comment.
Initialize color before forEachLightIndex to avoid UB.
If the callback isn’t invoked, color is uninitialized and you return garbage. Initialize to CRGB::Black first.
🐛 Proposed fix
- CRGB color;
+ CRGB color = CRGB::Black;
forEachLightIndex(indexV, [&](nrOfLights_t indexP) { memcpy(&color, &layerP->lights.channelsE[indexP * layerP->lights.header.channelsPerLight + layerP->lights.header.offsetRGB], sizeof(color)); }, true);
return color;🤖 Prompt for AI Agents
In `@src/MoonLight/Layers/VirtualLayer.h` around lines 220 - 235, The local
variable color in getRGB may be returned uninitialized if the forEachLightIndex
callback never runs; initialize color to a safe default (e.g., CRGB::Black)
before calling forEachLightIndex so that the returned value is always
well-defined; update the code path under the else branch (where
forEachLightIndex, layerP->lights.channelsE, and channelsPerLight/offsetRGB are
used) to set CRGB color = CRGB::Black; then leave the forEachLightIndex lambda
unchanged so it can overwrite color when invoked.
|
@coderabbitai, part of this PR is solving an issue with RGBW lights (or WRGB etc). I used a white calculator as follows in setRGB: if (layerP->lights.header.offsetWhite != UINT8_MAX && layerP->lights.header.channelsPerLight == 4) { // W set but this caused flickering. In this PR I removed it and placed it in driverLoop, but that also caused flickering so I commented it: // if (lights.header.channelsPerLight == 4 && lights.header.offsetWhite != UINT8_MAX) { I think the problem is that it does the correction in the channels array so when iterating through frames it pollutes the channel array, also a getRGB after a setRGB will give wrong data (as setRGB lowered the RGB channels). So I think I need to solve this in each of the drivers separately:
Can you do a thorough analysis of the problem and analyze my proposal to solve it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6635 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 13863 🏁 Script executed: Length of output: 75 🏁 Script executed: Length of output: 657 🏁 Script executed: Length of output: 5901 🏁 Script executed: Length of output: 1458 🏁 Script executed: Length of output: 2080 🏁 Script executed: Length of output: 49 🏁 Script executed: Length of output: 3632
Why Both Previous Approaches Failed❌ White Calc in
|
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.