Livescripts tutorial, WiFi handling, Network in/out, rest API#161
Merged
Livescripts tutorial, WiFi handling, Network in/out, rest API#161
Conversation
docs ==== - update live scripts tutorial firmware ======= - latest ESPLiveScript (fix setRGB(random16(NUM_LEDS), CRGB(0, 0, 255))) backend ======= - WiFiSettings Service: add _connecting add WiFi.onEvent, manageSTA/configureNetwork/onStationModeDisconnected: set and check _connecting - Live scripts: file update: startCompile instead of compileAndRun - Network in: writePixels: Resolve bounds against the target layer's own light count, handleDDP: Reject mid-pixel offsets
WalkthroughUpdates across docs, LiveScripts, drivers, WiFi service, UI, and build pins: adds new LiveScript examples/externals and async compile, hardens NetworkIn/Out/DMX virtual-layer bounds and status, prevents WiFi reconnect re-entrancy with a Changes
Sequence Diagram(s)sequenceDiagram
participant Mgmt as Connection Manager\n(manageSTA)
participant WiFi as WiFi Stack
participant Event as Event Handler
rect rgba(100,150,200,0.5)
Note over Mgmt,Event: Start connection attempt
Mgmt->>Mgmt: set _connecting = true
Mgmt->>WiFi: WiFi.begin(ssid,pwd)
end
rect rgba(150,200,100,0.5)
Note over Mgmt: Prevent re-entry while connecting
Mgmt->>Mgmt: if _connecting == true → early return
end
rect rgba(200,150,100,0.5)
Note over WiFi,Event: On successful connect
WiFi->>Event: ARDUINO_EVENT_WIFI_STA_CONNECTED
Event->>Mgmt: clear _connecting = false
Event->>Mgmt: set _lastConnectionAttempt = millis()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Drivers/D_NetworkIn.h (1)
215-218: Consider aligning the pre-check with the new layer-aware bounds logic.Line 216 checks
startPixelUagainst the physical layer'snrOfLights, butwritePixels()now correctly handles virtual layers that may have a different light count. If a virtual layer is selected (layer > 0) and has more lights than the physical layer, this pre-check would incorrectly reject valid DDP packets before they reachwritePixels().This is pre-existing behavior, but the new
writePixels()logic makes the inconsistency more apparent. Art-Net and E1.31 handlers don't have this pre-check and rely entirely onwritePixels()for bounds validation.♻️ Suggested fix to align with other handlers
const uint32_t startPixelU = offset / channelsPerLight; - if (startPixelU >= static_cast<uint32_t>(layerP.lights.header.nrOfLights)) return; writePixels(static_cast<int>(startPixelU), safeDataLen / static_cast<int>(channelsPerLight), pixelData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_NetworkIn.h` around lines 215 - 218, The pre-check comparing startPixelU to layerP.lights.header.nrOfLights incorrectly rejects packets for virtual layers; remove or change this check so bounds validation is delegated to writePixels (which already handles virtual vs physical layer counts). Specifically, update the logic around the startPixelU calculation and the early return so it either uses the layer-aware light count (the virtual layer's nrOfLights) or omits the check entirely and lets writePixels(static_cast<int>(startPixelU), ...) perform the validation; adjust references to startPixelU, layerP.lights.header.nrOfLights, and writePixels accordingly.docs/moonlight/effects-tutorial.md (2)
366-366: Remove debugging comment.The comment
// this works !!appears to be a testing artifact and doesn't add instructional value. Consider removing it to keep the documentation clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/moonlight/effects-tutorial.md` at line 366, Remove the debugging/testing comment appended to the example call so the documentation stays clean: edit the line containing the call to setRGBXYZ(x, y, z, ColorFromPalette(n, bright)) and delete the trailing comment "// this works !!" (leave the function call and parameters intact).
186-187: Inconsistent ColorFromPalette usage pattern.Some examples use intermediate variables before calling
setRGBXY(lines 186-187, 261-262), while others passColorFromPalette(...)inline (line 160). If the inline constructor limitation mentioned in the Important notes has been resolved by the library update, these intermediate variables are unnecessary. If workarounds are still needed in some contexts, document when to use which pattern.Also applies to: 261-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/moonlight/effects-tutorial.md` around lines 186 - 187, The examples show mixed usage of ColorFromPalette: some assign CRGB color = ColorFromPalette(...) then call setRGBXY(x,y,color) (eg. the blocks around ColorFromPalette/setRGBXY at lines referenced) while others call setRGBXY(x,y, ColorFromPalette(...)) inline; decide and unify: if the library limitation preventing inline temporaries has been fixed, remove the intermediate CRGB variable (keep setRGBXY(x,y, ColorFromPalette(...)) everywhere) and update the two occurrences that currently use a temporary; if the limitation still applies, keep the temporary pattern and add a brief note near the examples (mentioning ColorFromPalette and setRGBXY) explaining when the workaround (temporary CRGB variable) is required vs when inline calls are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/moonlight/effects-tutorial.md`:
- Line 269: The watchdog note is vague; update the docs around the E_Noise3D
example to explicitly explain that heavy computation inside Arduino's loop()
(e.g., triple-nested loops calling inoise8 per-pixel) can trigger watchdog
resets, and provide concrete fixes: either insert a yield/delay(1) inside the
per-frame loop, reduce workload by lowering scale or processing fewer pixels per
frame, or move expensive noise computation off the tight loop; reference the
loop() function, the inoise8 call, and the E_Noise3D example so readers can find
and apply the workaround.
In `@docs/moonlight/livescripts.md`:
- Line 44: The doc note about inline constructor arguments (example: setRGB(i,
CRGB(0, 0, 255))) is ambiguous given the updated ESPLiveScript in platformio.ini
and working examples like E_hello.sc and the tutorial final example; either
update the sentence to state that the inline-CRGB issue was fixed in the current
ESPLiveScript version (reference ESPLiveScript/platformio.ini) or change all
examples that use inline CRGB(...) to use the intermediate variable form (CRGB
color = CRGB(...); setRGB(..., color)) so the docs and examples are consistent;
locate occurrences of setRGB, CRGB, E_hello.sc and the tutorial final example
and apply the chosen consistency fix.
In `@src/MoonLight/Modules/ModuleLiveScripts.h`:
- Around line 54-55: The current code sets _moduleEffects->requestUIUpdate
immediately after calling liveScriptNode->startCompile(), but startCompile() is
asynchronous (it may queue needsCompile/compileInProgress), so move the UI
refresh to the async compile completion path instead: remove the immediate
assignment after startCompile() and instead re-arm
_moduleEffects->requestUIUpdate at the point where the deferred compile/execute
finishes (the callback/worker or the code path that runs the deferred execute
for liveScriptNode), so the UI is updated only after the new script has actually
been compiled/executed.
---
Nitpick comments:
In `@docs/moonlight/effects-tutorial.md`:
- Line 366: Remove the debugging/testing comment appended to the example call so
the documentation stays clean: edit the line containing the call to setRGBXYZ(x,
y, z, ColorFromPalette(n, bright)) and delete the trailing comment "// this
works !!" (leave the function call and parameters intact).
- Around line 186-187: The examples show mixed usage of ColorFromPalette: some
assign CRGB color = ColorFromPalette(...) then call setRGBXY(x,y,color) (eg. the
blocks around ColorFromPalette/setRGBXY at lines referenced) while others call
setRGBXY(x,y, ColorFromPalette(...)) inline; decide and unify: if the library
limitation preventing inline temporaries has been fixed, remove the intermediate
CRGB variable (keep setRGBXY(x,y, ColorFromPalette(...)) everywhere) and update
the two occurrences that currently use a temporary; if the limitation still
applies, keep the temporary pattern and add a brief note near the examples
(mentioning ColorFromPalette and setRGBXY) explaining when the workaround
(temporary CRGB variable) is required vs when inline calls are safe.
In `@src/MoonLight/Nodes/Drivers/D_NetworkIn.h`:
- Around line 215-218: The pre-check comparing startPixelU to
layerP.lights.header.nrOfLights incorrectly rejects packets for virtual layers;
remove or change this check so bounds validation is delegated to writePixels
(which already handles virtual vs physical layer counts). Specifically, update
the logic around the startPixelU calculation and the early return so it either
uses the layer-aware light count (the virtual layer's nrOfLights) or omits the
check entirely and lets writePixels(static_cast<int>(startPixelU), ...) perform
the validation; adjust references to startPixelU,
layerP.lights.header.nrOfLights, and writePixels accordingly.
🪄 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: 12867819-f5c5-4a46-907b-86802e78e67f
⛔ Files ignored due to path filters (11)
docs/media/moonlight/effects/E_Ball2D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Cosmic3D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Geq2D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Hello1D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Noise2D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Noise3D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Orbit2D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Oscillate2D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Random1D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Sweep2D.gifis excluded by!**/*.gifdocs/media/moonlight/effects/E_Vu1D.gifis excluded by!**/*.gif
📒 Files selected for processing (9)
docs/moonlight/effects-tutorial.mddocs/moonlight/livescripts.mdlib/framework/WiFiSettingsService.cpplib/framework/WiFiSettingsService.hlivescripts/Effects/E_hello.scplatformio.inisrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Modules/ModuleLiveScripts.hsrc/MoonLight/Nodes/Drivers/D_NetworkIn.h
Doc updates Live scripts ========== - Add E_PaintBrush.sc Backend ======= - Live scripts: add drawLine3D and blur2D and sqrt, file Update: no requestUIUpdate - LayerManager: check for old style presets - DMX in / Network in: store layers in virtualChannels - Network in/out: add status
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Nodes/Drivers/D_NetworkIn.h (1)
239-252:⚠️ Potential issue | 🟠 MajorDDP still caps virtual-layer offsets at the physical light count.
This handler rejects
startPixelUagainstlayerP.lights.header.nrOfLightsbeforewritePixels()gets to use the target layer's own size. For virtual layouts larger than the physical fixture, valid DDP offsets in the upper part of the virtual grid now return early. Use the same per-layer max-light calculation here before the early return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_NetworkIn.h` around lines 239 - 252, The handler currently rejects startPixelU against the physical count (layerP.lights.header.nrOfLights) which incorrectly drops valid DDP offsets for virtual layers; change the early-return check to use the per-layer maximum lights calculation used by the target layer (the same calculation used before calling writePixels) instead of layerP.lights.header.nrOfLights. Compute the layer-specific max lights (using the existing per-layer calc/function used elsewhere in this file/logic) and replace the if (startPixelU >= static_cast<uint32_t>(layerP.lights.header.nrOfLights)) return; with a check against that per-layer max before calling writePixels, leaving the rest of the offset/dataLen/safeDataLen logic intact.
🧹 Nitpick comments (1)
livescripts/Effects/E_PaintBrush.sc (1)
39-48: Avoidsqrtin the hot loop when only checking> 1.Line 42 and Line 47 can use squared-distance checks to reduce per-frame compute cost.
♻️ Suggested optimization
- float dx = x2 - x1; - float dy = y2 - y1; - float dz = z2 - z1; - float len = sqrt(dx * dx + dy * dy + dz * dz); - if (len > 1) drawLine3D(x1, y1, z1, x2, y2, z2, color); + int dx = (int)x2 - (int)x1; + int dy = (int)y2 - (int)y1; + int dz = (int)z2 - (int)z1; + int dist2 = dx * dx + dy * dy + dz * dz; + if (dist2 > 1) drawLine3D(x1, y1, z1, x2, y2, z2, color); } else { - float dx = x2 - x1; - float dy = y2 - y1; - float len = sqrt(dx * dx + dy * dy); - if (len > 1) drawLine(x1, y1, x2, y2, color); + int dx = (int)x2 - (int)x1; + int dy = (int)y2 - (int)y1; + int dist2 = dx * dx + dy * dy; + if (dist2 > 1) drawLine(x1, y1, x2, y2, color); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/E_PaintBrush.sc` around lines 39 - 48, The code currently computes len = sqrt(dx*dx + dy*dy (+ dz*dz)) and checks len > 1 before calling drawLine3D/drawLine; replace the sqrt with a squared-distance check to avoid the hot-loop sqrt: compute lenSq = dx*dx + dy*dy (+ dz*dz) and use if (lenSq > 1.0) to decide calling drawLine3D(x1,y1,z1,x2,y2,z2,color) or drawLine(x1,y1,x2,y2,color). Update both branches where dx, dy, dz and len are used (the blocks that call drawLine3D and drawLine) and remove the unused sqrt and len variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Nodes/Drivers/D_DMXIn.h`:
- Around line 242-249: This branch must not write directly into
VirtualLayer::virtualChannels from driverTask because
compositeTo()/compositeLayers() reads that buffer outside swapMutex and the
layer may be unallocated; instead stop the memcpy into vLayer->virtualChannels
here and route the incoming DMX into the mapped channelsD path or hand it off to
the effect task for safe mutation. Concretely: remove the memcpy to
vLayer->virtualChannels in this block (the code touching VirtualLayer* vLayer,
virtualChannels, and nrOfLights) and replace with either (a) writing the input
into the corresponding channelsD mapping for that layer/header, or (b)
enqueueing the input data to the effect task which will take swapMutex and
update virtualChannels safely; ensure you still check for vLayer existence but
never mutate virtualChannels from driverTask and preserve use of swapMutex only
in the effect/composite update path.
In `@src/MoonLight/Nodes/Drivers/D_NetworkIn.h`:
- Around line 183-190: The loop currently writes incoming DMX into
vLayer->virtualChannels under the driverTask protected only by swapMutex, which
races with compositeLayers()/compositeTo() and can miss-unset when
virtualChannels is unallocated; instead stop routing network input into
virtualChannels from this loop—always write into the physical buffer
layerP.lights.channelsD (using the same ledIndex and channelsPerLight math) or,
if you need to update a virtual layer, enqueue the update to the effect/driver
task that owns virtualChannels so the write happens under the same context that
allocates and reads it; update the branch that references vLayer,
virtualChannels, swapMutex, compositeTo(), and channelsD accordingly.
In `@src/MoonLight/Nodes/Drivers/D_NetworkOut.h`:
- Around line 501-510: The status is set to "Sending DDP" before calling
loopDDP(), which can immediately return for unsupported layouts and thus show a
misleading state; modify the branch handling protocol == 1 in D_NetworkOut (use
symbols: protocol, lastStatusCode, status, updateControl, loopDDP, header) to
only change lastStatusCode and call updateControl("status", status) after
verifying the frame/layout is actually supported by loopDDP (i.e., perform the
same channel-count/layout check used inside loopDDP before setting the status),
or alternatively set an explicit unsupported-layout status when the check fails
so the UI reflects dropped/unsupported frames rather than "Sending DDP".
---
Outside diff comments:
In `@src/MoonLight/Nodes/Drivers/D_NetworkIn.h`:
- Around line 239-252: The handler currently rejects startPixelU against the
physical count (layerP.lights.header.nrOfLights) which incorrectly drops valid
DDP offsets for virtual layers; change the early-return check to use the
per-layer maximum lights calculation used by the target layer (the same
calculation used before calling writePixels) instead of
layerP.lights.header.nrOfLights. Compute the layer-specific max lights (using
the existing per-layer calc/function used elsewhere in this file/logic) and
replace the if (startPixelU >=
static_cast<uint32_t>(layerP.lights.header.nrOfLights)) return; with a check
against that per-layer max before calling writePixels, leaving the rest of the
offset/dataLen/safeDataLen logic intact.
---
Nitpick comments:
In `@livescripts/Effects/E_PaintBrush.sc`:
- Around line 39-48: The code currently computes len = sqrt(dx*dx + dy*dy (+
dz*dz)) and checks len > 1 before calling drawLine3D/drawLine; replace the sqrt
with a squared-distance check to avoid the hot-loop sqrt: compute lenSq = dx*dx
+ dy*dy (+ dz*dz) and use if (lenSq > 1.0) to decide calling
drawLine3D(x1,y1,z1,x2,y2,z2,color) or drawLine(x1,y1,x2,y2,color). Update both
branches where dx, dy, dz and len are used (the blocks that call drawLine3D and
drawLine) and remove the unused sqrt and len variable.
🪄 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: f059cae9-106f-4256-b14e-1dbf13cb78f2
📒 Files selected for processing (13)
CLAUDE.mddocs/moonlight/drivers.mddocs/moonlight/effects-tutorial.mdlivescripts/Effects/E_PaintBrush.scmisc/instructions/MoonLight.mdmisc/instructions/svelte.instructions.mdplatformio.inisrc/MoonBase/LiveScriptNode.cppsrc/MoonLight/Layers/LayerManager.hsrc/MoonLight/Modules/ModuleLiveScripts.hsrc/MoonLight/Nodes/Drivers/D_DMXIn.hsrc/MoonLight/Nodes/Drivers/D_NetworkIn.hsrc/MoonLight/Nodes/Drivers/D_NetworkOut.h
✅ Files skipped from review due to trivial changes (4)
- misc/instructions/MoonLight.md
- CLAUDE.md
- misc/instructions/svelte.instructions.md
- docs/moonlight/drivers.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MoonLight/Modules/ModuleLiveScripts.h
- docs/moonlight/effects-tutorial.md
Docs ==== - Rework moonlight and moonbase overview.md: add Reporting issues and refer to it across docs frontend ======= - add link to (rest) api on all module pages backend ======= - latest FastLED - Network in: ddp: Compute per-layer max - Network out: ddp light preset check - DMX in: processChannels: validates vLayer->virtualChannels inside the mutex
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
interface/src/routes/ethernet/Ethernet.svelte (1)
562-565: Consider using protocol-relative or current protocol URL.The hardcoded
http://protocol may cause mixed-content issues if the UI is served over HTTPS. The link would either fail or trigger a browser security warning.Proposed fix to use the current page protocol
- <a href="http://{page.url.host}/rest/ethernetSettings" target="_blank" rel="noopener noreferrer" title="API: http://{page.url.host}/rest/ethernetSettings"><Api class="mr-2 h-6 w-6 shrink-0 self-end" /></a> + <a href="{page.url.protocol}//{page.url.host}/rest/ethernetSettings" target="_blank" rel="noopener noreferrer" title="API: {page.url.protocol}//{page.url.host}/rest/ethernetSettings"><Api class="mr-2 h-6 w-6 shrink-0 self-end" /></a>Alternatively, use a simple relative path
/rest/ethernetSettingswithout the host, which inherits the current origin automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/ethernet/Ethernet.svelte` around lines 562 - 565, The anchor uses a hardcoded "http://" which can cause mixed-content issues; update the <a> element that currently builds href/title with "http://{page.url.host}/rest/ethernetSettings" (and its title attribute) to use a protocol-agnostic URL—preferably a relative path "/rest/ethernetSettings" so it inherits the current origin, and ensure the title string is updated accordingly; adjust the anchor around the Api component to use the new href/title values.interface/src/routes/wifi/ap/Accesspoint.svelte (1)
426-429: Same protocol hardcoding issue as other files.Using hardcoded
http://may cause mixed-content issues when served over HTTPS. Consider usingpage.url.protocolor a relative path/rest/apSettingsfor consistency and to avoid mixed-content browser warnings.Proposed fix
- <a href="http://{page.url.host}/rest/apSettings" target="_blank" rel="noopener noreferrer" title="API: http://{page.url.host}/rest/apSettings"><Api class="mr-2 h-6 w-6 shrink-0 self-end" /></a> + <a href="{page.url.protocol}//{page.url.host}/rest/apSettings" target="_blank" rel="noopener noreferrer" title="API: {page.url.protocol}//{page.url.host}/rest/apSettings"><Api class="mr-2 h-6 w-6 shrink-0 self-end" /></a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/wifi/ap/Accesspoint.svelte` around lines 426 - 429, The anchor uses a hardcoded "http://" in the href (the <a href="http://{page.url.host}/rest/apSettings" ...> containing the Api component), which causes mixed-content when served over HTTPS; update the href to either use the page.url.protocol value (e.g., construct with "{page.url.protocol}//{page.url.host}/rest/apSettings") or, better, use a protocol-relative or relative path like "/rest/apSettings" so the link follows the current page protocol and avoids mixed-content issues.interface/src/routes/moonbase/module/Module.svelte (1)
214-223: Handle null module parameter and protocol hardcoding.Two issues with this link:
Null module param:
page.url.searchParams.get('module')returnsnullif the parameter is missing, resulting in a link to/rest/null. Consider adding a fallback or conditionally rendering the link.Hardcoded
http://: Same as other files—may cause mixed-content issues over HTTPS.Proposed fix
<!-- 🌙 link to api --> + {`@const` moduleName = page.url.searchParams.get('module')} + {`#if` moduleName} <div class="flex justify-end px-4 pb-3"> <a - href="http://{page.url.host}/rest/{page.url.searchParams.get('module')}" + href="{page.url.protocol}//{page.url.host}/rest/{moduleName}" target="_blank" rel="noopener noreferrer" - title="API: http://{page.url.host}/rest/{page.url.searchParams.get('module')}" + title="API: {page.url.protocol}//{page.url.host}/rest/{moduleName}" ><Api class="mr-2 h-6 w-6 shrink-0 self-end" /></a > </div> + {/if}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/moonbase/module/Module.svelte` around lines 214 - 223, The anchor currently builds a URL using hardcoded "http://" and page.url.searchParams.get('module') which can be null; update Module.svelte to (1) only render the <a> (the anchor that wraps <Api>) when page.url.searchParams.get('module') is non-null/non-empty or supply a safe fallback (e.g., omit "null" by using the param value || ''), and (2) derive the scheme from the runtime URL (use page.url.protocol or the runtime location.protocol to prefix "//" or `${protocol}//`) instead of hardcoding "http://", so the anchor always uses the correct protocol and avoids mixed-content issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/moonlight/overview.md`:
- Around line 169-181: The documentation currently mixes REST endpoint casing
("/rest/ModuleEffects" vs lowercase examples "/rest/effects", "/rest/drivers",
"/rest/lightscontrol"); pick one canonical format (prefer lowercase plural
endpoints like "/rest/effects") and update the earlier URL example that mentions
"/rest/ModuleEffects" to match the chosen canonical form, and ensure the three
example endpoints (effects, drivers, lightscontrol) and any explanatory text all
use the same casing and path style (adjust references in the paragraph that
constructs the URL and the tip about browser access).
- Line 108: Typo: change the frontend stack entry that currently reads "TailWind
4" to the correct casing "Tailwind 4" in the overview text (look for the string
"TailWind 4" in the document where the frontend stack is listed alongside
"Svelte 5, DaisyUI 5"); update that token to "Tailwind 4" to fix the naming
consistency.
---
Nitpick comments:
In `@interface/src/routes/ethernet/Ethernet.svelte`:
- Around line 562-565: The anchor uses a hardcoded "http://" which can cause
mixed-content issues; update the <a> element that currently builds href/title
with "http://{page.url.host}/rest/ethernetSettings" (and its title attribute) to
use a protocol-agnostic URL—preferably a relative path "/rest/ethernetSettings"
so it inherits the current origin, and ensure the title string is updated
accordingly; adjust the anchor around the Api component to use the new
href/title values.
In `@interface/src/routes/moonbase/module/Module.svelte`:
- Around line 214-223: The anchor currently builds a URL using hardcoded
"http://" and page.url.searchParams.get('module') which can be null; update
Module.svelte to (1) only render the <a> (the anchor that wraps <Api>) when
page.url.searchParams.get('module') is non-null/non-empty or supply a safe
fallback (e.g., omit "null" by using the param value || ''), and (2) derive the
scheme from the runtime URL (use page.url.protocol or the runtime
location.protocol to prefix "//" or `${protocol}//`) instead of hardcoding
"http://", so the anchor always uses the correct protocol and avoids
mixed-content issues.
In `@interface/src/routes/wifi/ap/Accesspoint.svelte`:
- Around line 426-429: The anchor uses a hardcoded "http://" in the href (the <a
href="http://{page.url.host}/rest/apSettings" ...> containing the Api
component), which causes mixed-content when served over HTTPS; update the href
to either use the page.url.protocol value (e.g., construct with
"{page.url.protocol}//{page.url.host}/rest/apSettings") or, better, use a
protocol-relative or relative path like "/rest/apSettings" so the link follows
the current page protocol and avoids mixed-content issues.
🪄 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: bc2492f7-c36c-4e4a-8d37-ab4467b6f674
📒 Files selected for processing (19)
docs/gettingstarted/installer.mddocs/index.mddocs/moonbase/overview.mddocs/moonlight/overview.mdinterface/src/routes/connections/mqtt/MQTT.svelteinterface/src/routes/connections/ntp/NTP.svelteinterface/src/routes/ethernet/Ethernet.svelteinterface/src/routes/moonbase/filemanager/FileManager.svelteinterface/src/routes/moonbase/module/Module.svelteinterface/src/routes/system/coredump/+page.svelteinterface/src/routes/system/status/SystemStatus.svelteinterface/src/routes/wifi/ap/Accesspoint.svelteinterface/src/routes/wifi/sta/Wifi.sveltelib/framework/WWWData.hlivescripts/Effects/E_PaintBrush.scplatformio.inisrc/MoonLight/Nodes/Drivers/D_DMXIn.hsrc/MoonLight/Nodes/Drivers/D_NetworkIn.hsrc/MoonLight/Nodes/Drivers/D_NetworkOut.h
✅ Files skipped from review due to trivial changes (9)
- interface/src/routes/moonbase/filemanager/FileManager.svelte
- interface/src/routes/system/coredump/+page.svelte
- interface/src/routes/wifi/sta/Wifi.svelte
- docs/gettingstarted/installer.md
- interface/src/routes/connections/ntp/NTP.svelte
- interface/src/routes/connections/mqtt/MQTT.svelte
- interface/src/routes/system/status/SystemStatus.svelte
- docs/index.md
- docs/moonbase/overview.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Drivers/D_NetworkIn.h
Collaborator
Author
|
@coderabbitai, I am about to merge this PR. Please produce three outputs:
|
|
Tip For best results, initiate chat on the files or code changes.
1 · PR ReviewSummary of changes
Merge recommendation
Prioritised follow-up actionsP0 – Address before or immediately after merge
P1 – Short follow-up PR
P2 – Nice-to-have
🔴 Ready-to-paste Claude Code prompt — pre-merge hardening2 · End-user docs prompt3 · Developer docs prompt🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, please do your review again, also looking at comments I made (which I now all set "resolved') and latest commits including 5b887e8, because I think your report does not reflect latest status |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 46013
Updated PR
|
| Area | Change | Status |
|---|---|---|
| WiFiSettingsService | _connecting flag prevents re-entrant WiFi.begin(); manageSTA() early-returns; onStationModeDisconnected() deferred via _lastConnectionAttempt |
✅ Merged logic sound |
D_NetworkIn handleDDP() |
Per-layer ddpMaxLights computed correctly for virtual layers (commit 5b887e8) |
✅ Fixed |
D_NetworkIn writePixels() |
Direct virtualChannels[] write; mapping-table warning log not yet added |
|
D_DMXIn processChannels() |
Bounds checked before taking mutex; null-check moved inside mutex (TOCTOU fix in 5b887e8) | ✅ Fixed |
| D_NetworkOut | Per-protocol status strings + lastStatusCode=3 for unsupported DDP layout |
✅ Fixed |
| ModuleLiveScripts | compileAndRun() → startCompile() (async) |
✅ |
| LayerManager | Backward-compat guard for bare brightness/start/end; no warning log |
|
| LiveScriptNode | Added sqrt, drawLine3D, blur2d external bindings |
✅ |
| E_PaintBrush.sc | New audio-reactive 2D/3D paintbrush effect; sqrt→integer dist2 (5b887e8) |
✅ |
| E_hello.sc | rgb() → CRGB() |
✅ |
| ESPLiveScript pin | Updated to 48715099 (20260402) |
✅ |
| FastLED pin | Updated to eca4af57 (20260404) |
✅ |
| UI: API icon links | All 10+ module pages now show REST endpoint link | ✅ (note below) |
| Docs: moonlight/overview.md | Full rework — Why MoonLight table, emoji key table, Reporting Issues section | ✅ |
| Docs: moonbase/overview.md | Full rework — File Manager, Devices, Tasks, IO, Status Bar table | ✅ |
| Docs: livescripts.md | Important notes section added | ✅ |
| Docs: effects-tutorial.md | Script renames, beatsin8 args, VU meter sensitivity fix | ✅ |
| Docs: drivers.md | Network Out examples clarified | ✅ |
Merge recommendation
✅ Ready to merge. The two remaining open items are low-risk and appropriate for a follow-up PR. The high-risk WiFi and virtual-layer changes are solid; the latest commit closed the remaining gaps in DMXIn and DDP bounds checking.
Remaining open items (post-merge follow-up)
P1 — Short follow-up PR
-
WiFi
_connectingliveness escape —manageSTA()still has no safety timeout for the case whereARDUINO_EVENT_WIFI_STA_CONNECTEDis never delivered. If the IDF event loop misses the event,_connectingstaystruepermanently and WiFi recovery is blocked indefinitely. Amillis() - _lastConnectionAttempt > 30000reset inmanageSTA()would close this. -
Virtual-layer write with non-flat mapping table —
D_NetworkIn::writePixels()writes directly intovirtualChannels[ledIndex]and ignores the mapping table whenmappingTableSize > 0. This is harmless for flat/sequential virtual layers but silently misplaces pixels on zigzag or segment maps. A one-timeEXT_LOGWwhen entering this path with a non-empty mapping table would make regressions visible during testing. -
LayerManager: silent discard of legacy-preset updates — When
brightness_0/start_0/end_0keys are absent, the update is silently dropped. Timing bugs (keys not yet written) look identical to a genuine old-preset. AEXT_LOGDline distinguishing the two would help.
P2 — Nice-to-have
http://hardcoded in all API icon links — All 10 pages usehttp://{page.url.host}/rest/.... This breaks on HTTPS deployments. Changing to{page.url.protocol}//{page.url.host}/rest/...would be future-proof.
Ready-to-paste Claude Code prompt — post-merge follow-up
You are working on MoonModules/MoonLight (main branch, post-PR-161 merge).
Make minimal, targeted changes only. Do NOT commit — show a unified diff.
## Fix 1 — WiFiSettingsService: _connecting liveness escape
File: lib/framework/WiFiSettingsService.cpp
In manageSTA(), inside the `if (_connecting) return;` early-return block,
add a timeout escape BEFORE the return:
if (millis() - _lastConnectionAttempt > 30000) {
EXT_LOGW(ML_TAG, "WiFi: _connecting stuck >30s, resetting flag");
_connecting = false;
} else return;
This prevents permanent lockout when ARDUINO_EVENT_WIFI_STA_CONNECTED
is never delivered.
## Fix 2 — D_NetworkIn: log warning for non-flat virtual-layer writes
File: src/MoonLight/Nodes/Drivers/D_NetworkIn.h
In writePixels(), in the virtual-layer (layer > 0) path, after resolving
vLayer, add a ONE-TIME log (use a static bool logged = false guard) when
vLayer->mappingTableSize > 0:
static bool mappingTableWarned = false;
if (!mappingTableWarned && vLayer->mappingTableSize > 0) {
EXT_LOGW(ML_TAG, "NetworkIn: direct virtualChannels write ignores non-flat mapping table");
mappingTableWarned = true;
}
## Fix 3 — LayerManager: log skipped legacy-preset updates
File: src/MoonLight/Layers/LayerManager.h
In handleUpdate(), in each branch that detects the _0 key is null and
returns early, add:
EXT_LOGD(ML_TAG, "LayerManager: skipping bare '%s' update — _0 key absent (old preset or timing)", updatedItem.name.c_str());
so contributors can distinguish a genuine old-preset from a race condition.
## Fix 4 — UI: use page.url.protocol instead of hardcoded http://
In every Svelte file that contains `http://{page.url.host}/rest/`:
Replace all occurrences of `http://{page.url.host}` with
`{page.url.protocol}//{page.url.host}`.
Affected files (search with rg):
interface/src/routes/connections/mqtt/MQTT.svelte
interface/src/routes/connections/ntp/NTP.svelte
interface/src/routes/ethernet/Ethernet.svelte
interface/src/routes/moonbase/filemanager/FileManager.svelte
interface/src/routes/moonbase/module/Module.svelte
interface/src/routes/system/coredump/+page.svelte
interface/src/routes/system/status/SystemStatus.svelte
interface/src/routes/wifi/ap/Accesspoint.svelte
interface/src/routes/wifi/sta/Wifi.svelte
Would you like me to open a GitHub issue for the follow-up items so they are tracked?
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-19T12:50:37.875Z
Learning: In MoonModules/MoonLight PR `#153` (livescripts branch): The final merged set of 6 commits correctly implements: (1) counting semaphore (max 4) for LiveScript frame-sync with guarded scriptsToSync increment; (2) async startCompile() with compileInProgress/needsCompile deferred queue; (3) hasLoopTask gate to prevent loop() signalling when no task is running; (4) lightPreset_RGBCCT 5-channel support via offsetWhite2 sentinel (UINT8_MAX) through LightsHeader→parlio chain; (5) audio (bands[16], volume) and IMU (gravityX/Y/Z) external bindings for LiveScripts. All critical review items were resolved before merge.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-19T12:12:24.260Z
Learning: In MoonModules/MoonLight (src/MoonBase/LiveScriptNode.cpp + src/main.cpp): The LiveScript frame-sync uses a counting semaphore (max 4) gating script tasks, and a `scriptsToSync` counter consumed by `effectTask` via `ulTaskNotifyTake`. The critical invariant is: `scriptsToSync` MUST only be incremented when `xSemaphoreGive` succeeds. If `xSemaphoreGive` fails (semaphore full, 5th+ script), the script task is never unblocked and will never call `xTaskNotifyGive(effectTaskHandle)`, so incrementing `scriptsToSync` unconditionally causes a deadlock in `effectTask`. The correct behavior (already implemented) is to skip the frame for scripts beyond the 4-slot cap and log a warning. Do NOT suggest "always increment scriptsToSync" or increasing the cap as a fix — the guarded increment is the intentional deadlock-prevention mechanism.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 161
File: docs/moonlight/livescripts.md:44-44
Timestamp: 2026-04-03T12:18:07.382Z
Learning: In MoonModules/MoonLight (docs/moonlight/livescripts.md + ESPLiveScript library): The inline constructor argument limitation (e.g., `setRGB(i, CRGB(0, 0, 255))`) is only partially resolved as of PR `#161` (livescript-tuning). Some patterns like `setRGB(random16(NUM_LEDS), CRGB(0, 0, 255))` now work with the updated ESPLiveScript commit (48715099), but the fix is not complete for all inline constructor argument cases. The intermediate variable workaround (`CRGB color = CRGB(...); setRGB(i, color);`) is still the recommended fallback. Do not suggest the inline constructor form is fully fixed.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:02:02.378Z
Learning: In MoonModules/MoonLight (interface/src/routes/moonbase/module/Module.svelte), the `updateRecursive` function merges incoming WebSocket/HTTP state into the Svelte `$state` store by array INDEX (not by control `name` or `p`). With `pruneMissing=false` (the default for WS partial updates), keys absent in new data are never deleted from old objects. This causes stale properties (like `ro: true`) from one control to persist on a different control when the controls array changes length/order between node setup cycles. The fix is to replace array items entirely when their `p` (pointer) or `name` changes, rather than merging.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.598Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-04-01T14:08:59.261Z
Learning: In MoonModules/MoonLight LayerManager (src/MoonLight/Layers/LayerManager.h): restoreNonSelectedLayers() must suspend effectTaskHandle for its entire duration — from the first ensureLayer() call through all nm.addNode() pushes — because ensureLayer() immediately makes layerP.layers[i] non-null, so effectTask (Core 0) can call layer->loop() and iterate layer->nodes while addNode() push_back is in progress (vector realloc = use-after-free). The fix (added in PR `#159` commit b21a42d) wraps the full for-loop with `#ifdef` ARDUINO vTaskSuspend(effectTaskHandle) / vTaskResume(effectTaskHandle). Same suspend pattern is used in prepareForPresetLoad() and onNodeRemoved().
Learnt from: MoonModules
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-28T16:20:39.049Z
Learning: In MoonModules/MoonLight, the project enforces a "never copy code into test files" policy (documented in CLAUDE.md). If a function lives in a header with ESP32/Arduino dependencies and needs native testing, it must be extracted into a pure-C++ header under `src/MoonBase/utilities/` (no Arduino/ESP32 includes), then `#include`d from both the original file and the test. The BoardNames.h extraction in PR `#158` (commit a2a6e41) is the canonical example of this pattern.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-27T13:03:49.950Z
Learning: In MoonModules/MoonLight ModuleIO.onUpdate: During file load, compareRecursive processes JSON keys in order they appear. If boardPreset appears before modded in the file, checking _state.data["modded"] in the boardPreset handler reads the old code default (false), not the value being restored from file (true), causing custom pins to be overwritten. Fix: Skip boardPreset handling during backend/file updates (originId.toInt()==false). Instead, handle boardPreset in loop20ms() after full file load completes, where final modded state is available. Check: if modded==false, apply setBoardPresetDefaults(); if modded==true, skip and preserve custom pins from file.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 153
File: src/MoonBase/LiveScriptNode.cpp:50-51
Timestamp: 2026-03-19T10:14:00.024Z
Learning: In MoonModules/MoonLight LiveScriptNode (src/MoonBase/LiveScriptNode.cpp): `effectTaskHandle` is used without a null check in `sync()` via `xTaskNotifyGive(effectTaskHandle)`. This is intentional — MoonLight follows a "fail-fast at boot" / "minimal code for max speed" philosophy: if `effectTaskHandle` is not initialized, the application is already in an unrecoverable state and defensive null checks in hot paths are explicitly unwanted. Do not suggest adding null/nullptr guards for `effectTaskHandle` in `sync()` or similar critical-path code.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 136
File: src/MoonBase/LiveScriptNode.cpp:22-39
Timestamp: 2026-03-04T09:15:48.019Z
Learning: In MoonModules/MoonLight LiveScriptNode (src/MoonBase/LiveScriptNode.cpp): The global `gNode` (Node*) and `WaitAnimationSync` (binary semaphore) are intentionally global because ESPLiveScript's `addExternalFunction()` only accepts plain C function pointers with no void* userData/context parameter, and `setFunctionToSync()` accepts a single void(*)() with no per-instance binding. There is no way to attach per-instance state to callbacks within the current ESPLiveScript API. This is a known limitation acknowledged with a `// todo: this is not working well with multiple scripts running!!!` comment at line 211. Fixing it would require modifying the ESPLiveScript library itself to support userData/context on callbacks.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 136
File: src/MoonBase/LiveScriptNode.cpp:174-178
Timestamp: 2026-03-04T09:22:15.040Z
Learning: In MoonModules/MoonLight LiveScriptNode (src/MoonBase/LiveScriptNode.cpp): `compileAndRun()` is always called from the SvelteKit loop task (Core 1) — either via `LiveScriptNode::setup()` during node creation or from ModuleLiveScripts file-change handlers. The commented-out `runInAppTask` deferral wrapper (lines 177-178, 222) is an abandoned TODO from an earlier architecture where `compileAndRun()` might have been called from an HTTP handler. There is no deferred task queue (`runInAppTask`) in the current codebase. Deferring via a lambda would also introduce object-lifetime hazards because the `LiveScriptNode` could be destroyed before the lambda runs. Do not suggest adding a deferred task queue or lambda capture for `compileAndRun()`.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-01T14:12:53.662Z
Learning: Applies to src/lib/components/moonbase/**/*.test.ts : Add unit tests for pure, side-effect-free TypeScript functions in MoonLight-specific frontend files (under `src/routes/moonbase/`, `src/lib/components/moonbase/`, stores, and types) with tests co-located next to source
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
platformio.ini (1)
229-229: Consider removing the commented dependency with known link errors.The commented FastLED commit at line 229 is noted as having "link errors." Keeping broken dependencies in the
lib_depssection (even commented) can cause confusion for future maintainers who might attempt to uncomment it. Consider either:
- Removing the line entirely, or
- Moving it to a historical comment block outside
lib_deps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 229, Remove or relocate the commented broken dependency entry "; https://github.com/FastLED/FastLED#eca4af576737353e703866fa54d4c897cebb9208 ; 20260404 third party Yves link errors" from the lib_deps area in platformio.ini: either delete the commented line entirely to avoid confusion, or move it into a clearly marked historical/comments section outside lib_deps (e.g., a "HISTORY" or "NOTES" block) with a short note about the link error so maintainers don't accidentally re-enable it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Around line 284-285: Update the comment on the commit URL line to state that
the CRGB constructor fix at commit 48715099a is only partial: note that patterns
like setRGB(random16(NUM_LEDS), CRGB(0, 0, 255)) now work but other inline CRGB
constructor uses still fail; recommend the workaround of creating a temporary
CRGB variable (e.g., CRGB color = CRGB(...); setRGB(i, color);) where needed,
and keep mention that stack size / PSRAM task support remains present (see
__LS_STACK_CAPS usage in xTaskCreateWithCaps).
---
Nitpick comments:
In `@platformio.ini`:
- Line 229: Remove or relocate the commented broken dependency entry ";
https://github.com/FastLED/FastLED#eca4af576737353e703866fa54d4c897cebb9208 ;
20260404 third party Yves link errors" from the lib_deps area in platformio.ini:
either delete the commented line entirely to avoid confusion, or move it into a
clearly marked historical/comments section outside lib_deps (e.g., a "HISTORY"
or "NOTES" block) with a short note about the link error so maintainers don't
accidentally re-enable it.
🪄 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: c4396a72-873f-41a0-a4be-871120aceb7a
📒 Files selected for processing (15)
docs/moonlight/livescripts.mddocs/moonlight/overview.mdinterface/src/routes/connections/mqtt/MQTT.svelteinterface/src/routes/connections/ntp/NTP.svelteinterface/src/routes/ethernet/Ethernet.svelteinterface/src/routes/moonbase/filemanager/FileManager.svelteinterface/src/routes/moonbase/module/Module.svelteinterface/src/routes/system/coredump/+page.svelteinterface/src/routes/system/status/SystemStatus.svelteinterface/src/routes/wifi/ap/Accesspoint.svelteinterface/src/routes/wifi/sta/Wifi.sveltelib/framework/WWWData.hlib/framework/WiFiSettingsService.cppplatformio.inisrc/MoonLight/Nodes/Drivers/D_NetworkIn.h
✅ Files skipped from review due to trivial changes (9)
- interface/src/routes/connections/ntp/NTP.svelte
- interface/src/routes/system/status/SystemStatus.svelte
- interface/src/routes/connections/mqtt/MQTT.svelte
- interface/src/routes/moonbase/filemanager/FileManager.svelte
- interface/src/routes/ethernet/Ethernet.svelte
- docs/moonlight/livescripts.md
- interface/src/routes/wifi/sta/Wifi.svelte
- interface/src/routes/moonbase/module/Module.svelte
- interface/src/routes/system/coredump/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- interface/src/routes/wifi/ap/Accesspoint.svelte
- src/MoonLight/Nodes/Drivers/D_NetworkIn.h
- lib/framework/WiFiSettingsService.cpp
Comment on lines
+284
to
+285
| ; https://github.com/ewowi/ESPLiveScript.git#bd364c3bf08af2fb3eb4c142c86268403b6ce7e6 ; 20260328 stack size argument and tasks in psram | ||
| https://github.com/ewowi/ESPLiveScript.git#48715099a9c82cc914efe576b70c7a7364f258ce ; 20260402 fix setRGB(random16(NUM_LEDS), CRGB(0, 0, 255)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify ESPLiveScript API usage in LiveScriptNode and check for inline CRGB constructor patterns in scripts
echo "=== Checking LiveScriptNode.cpp for ESPLiveScript API usage ==="
rg -n -A3 -B3 'addExternalFunction|setFunctionToSync|startCompile|compileAndRun' src/MoonBase/LiveScriptNode.cpp
echo -e "\n=== Checking for inline CRGB constructor patterns in LiveScript examples ==="
fd -e sc -x rg -l 'setRGB.*CRGB\(' {} \;
echo -e "\n=== Checking for CRGB/CHSV usage in LiveScriptNode external bindings ==="
rg -n 'static.*CRGB|static.*CHSV' src/MoonBase/LiveScriptNode.cppRepository: MoonModules/MoonLight
Length of output: 5244
Update line 285 comment to clarify the CRGB constructor fix is partial and recommend the workaround.
The ESPLiveScript commit upgrade from bd364c3bf to 48715099a retains stack size and PSRAM task features (verified by __LS_STACK_CAPS usage in xTaskCreateWithCaps). However, the setRGB CRGB constructor fix at commit 48715099a is only partial—patterns like setRGB(random16(NUM_LEDS), CRGB(0, 0, 255)) now work, but not all inline constructor cases are resolved. Since inline CRGB patterns are actively used across multiple livescripts (E_lines.sc, E_random.sc, E_hello.sc, E_Kitt.sc, E_FireworkDust.sc), update the comment on line 285 to clarify the limitation and recommend the intermediate variable workaround (CRGB color = CRGB(...); setRGB(i, color);) where needed.
Suggested comment update
https://github.com/ewowi/ESPLiveScript.git#48715099a9c82cc914efe576b70c7a7364f258ce ; 20260402 partial fix: setRGB(random16(NUM_LEDS), CRGB(0, 0, 255)) now works; use CRGB temp var for other patterns
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` around lines 284 - 285, Update the comment on the commit URL
line to state that the CRGB constructor fix at commit 48715099a is only partial:
note that patterns like setRGB(random16(NUM_LEDS), CRGB(0, 0, 255)) now work but
other inline CRGB constructor uses still fail; recommend the workaround of
creating a temporary CRGB variable (e.g., CRGB color = CRGB(...); setRGB(i,
color);) where needed, and keep mention that stack size / PSRAM task support
remains present (see __LS_STACK_CAPS usage in xTaskCreateWithCaps).
Collaborator
Author
|
@coderabbitai, regarding
please review da791b6 as they are addressed here and update your merge readiness table This are the responses to your open items: │ Item │ Verdict │ Action │ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12853
Updated merge readiness table — PR
|
| Item | Previous status | Verdict | Resolution in da791b6 |
|---|---|---|---|
WiFi _connecting liveness escape |
✅ Closed | 30 s timeout + ESP_LOGW added in manageSTA() before resetting _connecting = false |
|
| Virtual-layer mapping-table "misplacement" | ✅ Closed — review framing was wrong | compositeTo() applies the mapping table when compositing to channelsD; data written in virtual-pixel order to virtualChannels[] is the correct contract. Explanatory comment added in writePixels() |
|
| LayerManager silent discard of legacy presets | ✅ Already fixed | EXT_LOGD lines were already present from a prior commit; no change needed |
|
UI API links hardcoded http:// |
✅ Closed | All 10 pages switched to relative /rest/... paths — cleaner than page.url.protocol and protocol/port agnostic |
Additional noteworthy change in da791b6
FastLED pin reverted — eca4af57 (20260404) caused link errors with third-party code (FASTLED_BUILD comment updated to 20260331); the stable 682179f8 pin is now active again. Worth keeping an eye on when the newer commit is re-evaluated.
Overall merge readiness
✅ Ready to merge — all P1 and P2 items resolved. No blocking issues remain.
I'll update my learning on the virtual-layer write path to reflect the correct architecture:
✏️ Learnings added
Learnt from: ewowi
URL: https://github.com/MoonModules/MoonLight/pull/161
Timestamp: 2026-04-04T09:23:16.541Z
Learning: In MoonModules/MoonLight D_NetworkIn and D_DMXIn (src/MoonLight/Nodes/Drivers/D_NetworkIn.h, D_DMXIn.h): Writing pixel data to VirtualLayer::virtualChannels[] in virtual-pixel (0-based) order is architecturally correct. compositeTo() applies the full mapping table (zigzag/segment/etc.) when compositing virtualChannels into channelsD. The write is NOT a pixel-misplacement risk — the mapping is handled downstream. A sender expecting physical-LED order should target layer 0 instead. Do NOT flag direct virtualChannels writes in these drivers as a mapping regression.
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: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-19T12:50:37.875Z
Learning: In MoonModules/MoonLight PR `#153` (livescripts branch): The final merged set of 6 commits correctly implements: (1) counting semaphore (max 4) for LiveScript frame-sync with guarded scriptsToSync increment; (2) async startCompile() with compileInProgress/needsCompile deferred queue; (3) hasLoopTask gate to prevent loop() signalling when no task is running; (4) lightPreset_RGBCCT 5-channel support via offsetWhite2 sentinel (UINT8_MAX) through LightsHeader→parlio chain; (5) audio (bands[16], volume) and IMU (gravityX/Y/Z) external bindings for LiveScripts. All critical review items were resolved before merge.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 161
File: docs/moonlight/livescripts.md:44-44
Timestamp: 2026-04-03T12:18:07.382Z
Learning: In MoonModules/MoonLight (docs/moonlight/livescripts.md + ESPLiveScript library): The inline constructor argument limitation (e.g., `setRGB(i, CRGB(0, 0, 255))`) is only partially resolved as of PR `#161` (livescript-tuning). Some patterns like `setRGB(random16(NUM_LEDS), CRGB(0, 0, 255))` now work with the updated ESPLiveScript commit (48715099), but the fix is not complete for all inline constructor argument cases. The intermediate variable workaround (`CRGB color = CRGB(...); setRGB(i, color);`) is still the recommended fallback. Do not suggest the inline constructor form is fully fixed.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 161
File: src/MoonLight/Nodes/Drivers/D_NetworkIn.h:183-190
Timestamp: 2026-04-04T07:35:48.139Z
Learning: In MoonModules/MoonLight NetworkInDriver and DMXInDriver (src/MoonLight/Nodes/Drivers/D_NetworkIn.h, D_DMXIn.h): For VIRTUAL layers (layer > 0), incoming pixel data MUST be written to vLayer->virtualChannels (not to layerP.lights.channelsD). Writing to channelsD directly would be silently zeroed every frame because compositeLayers() recomposes channelsD from virtualChannels on every pass. The write is safe under swapMutex because compositeLayers() also holds swapMutex when it reads virtualChannels — the two are mutually exclusive. virtualChannels reallocation is gated by isPositions, not by swapMutex. For PHYSICAL layer (layer == 0), writing directly to channelsD is correct (bypasses compositing). Do NOT flag virtual-layer writes to virtualChannels in these drivers as a data race — the swapMutex serialization makes it safe and channelsD is the wrong target for virtual-layer input.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-19T12:12:24.260Z
Learning: In MoonModules/MoonLight (src/MoonBase/LiveScriptNode.cpp + src/main.cpp): The LiveScript frame-sync uses a counting semaphore (max 4) gating script tasks, and a `scriptsToSync` counter consumed by `effectTask` via `ulTaskNotifyTake`. The critical invariant is: `scriptsToSync` MUST only be incremented when `xSemaphoreGive` succeeds. If `xSemaphoreGive` fails (semaphore full, 5th+ script), the script task is never unblocked and will never call `xTaskNotifyGive(effectTaskHandle)`, so incrementing `scriptsToSync` unconditionally causes a deadlock in `effectTask`. The correct behavior (already implemented) is to skip the frame for scripts beyond the 4-slot cap and log a warning. Do NOT suggest "always increment scriptsToSync" or increasing the cap as a fix — the guarded increment is the intentional deadlock-prevention mechanism.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-04-01T14:08:59.261Z
Learning: In MoonModules/MoonLight LayerManager (src/MoonLight/Layers/LayerManager.h): restoreNonSelectedLayers() must suspend effectTaskHandle for its entire duration — from the first ensureLayer() call through all nm.addNode() pushes — because ensureLayer() immediately makes layerP.layers[i] non-null, so effectTask (Core 0) can call layer->loop() and iterate layer->nodes while addNode() push_back is in progress (vector realloc = use-after-free). The fix (added in PR `#159` commit b21a42d) wraps the full for-loop with `#ifdef` ARDUINO vTaskSuspend(effectTaskHandle) / vTaskResume(effectTaskHandle). Same suspend pattern is used in prepareForPresetLoad() and onNodeRemoved().
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:36:33.812Z
Learning: In MoonModules/MoonLight D_FastLEDDriver: FastLED RMT5 (default on IDF >= 5) lacks the dual-buffer WiFi-resistance design of FastLED RMT4, causing WebSocket/WiFi throughput degradation at high LED frame rates. FastLED I2S is also incompatible with WiFi on ESP32-S3. MoonLight's D_ParallelLEDDriver (parlio.cpp) avoids this entirely by using its own DMA-streaming PARLIO implementation with zero per-frame ISR callbacks. Fix: add `-D FASTLED_RMT5=0` to platformio.ini for IDF5-based boards to force RMT4's dual-buffer design. This flag may need to be re-enabled even when a specific FastLED commit appeared stable, as RMT5 behavior varies across FastLED master commits.
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-03-08T10:36:33.812Z
Learning: In MoonModules/MoonLight D_FastLEDDriver vs D_ParallelLEDDriver: The PARLIO driver (parlio.cpp) uses `queue_nonblocking = 1` in `parlio_transmit_config_t`, making LED transmission fully fire-and-forget via hardware DMA — zero ISR callbacks during TX, CPU is free for WiFi. FastLED RMT5 (default on IDF5+) uses ISR-driven segment streaming with ISR priority capped at 1-3, which causes constant WiFi interrupt contention and WebSocket throughput degradation at high frame rates. I2S has the same ISR-driven problem. Fix: add `-D FASTLED_RMT5=0` to platformio.ini to force FastLED RMT4, which has an explicit dual-buffer WiFi-resistant design. WARNING: RMT4 and RMT5 cannot coexist in the same binary (startup abort). The affinity="PARLIO" option in FastLED Driver uses FastLED's own PARLIO wrapper, not MoonLight's custom parlio.cpp.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-27T12:33:23.675Z
Learning: In MoonModules/MoonLight ModuleIO.onUpdate: The boardPreset handler must be placed BEFORE the if (!originId.toInt()) return; check to handle both UI updates (originId="1") and boot/file-load updates (originId="module"). When modded=false and boardPreset changes on boot, setBoardPresetDefaults() must be called to apply current code-defined defaults (not just pins from file), ensuring board preset definitions can evolve across code updates. Only UI-specific handlers (modded, switch1/2, maxPower, usage, index, i2cFreq) should come after the originId check.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-27T13:03:49.950Z
Learning: In MoonModules/MoonLight ModuleIO.onUpdate: During file load, compareRecursive processes JSON keys in order they appear. If boardPreset appears before modded in the file, checking _state.data["modded"] in the boardPreset handler reads the old code default (false), not the value being restored from file (true), causing custom pins to be overwritten. Fix: Skip boardPreset handling during backend/file updates (originId.toInt()==false). Instead, handle boardPreset in loop20ms() after full file load completes, where final modded state is available. Check: if modded==false, apply setBoardPresetDefaults(); if modded==true, skip and preserve custom pins from file.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:02:02.378Z
Learning: In MoonModules/MoonLight (interface/src/routes/moonbase/module/Module.svelte), the `updateRecursive` function merges incoming WebSocket/HTTP state into the Svelte `$state` store by array INDEX (not by control `name` or `p`). With `pruneMissing=false` (the default for WS partial updates), keys absent in new data are never deleted from old objects. This causes stale properties (like `ro: true`) from one control to persist on a different control when the controls array changes length/order between node setup cycles. The fix is to replace array items entirely when their `p` (pointer) or `name` changes, rather than merging.
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-05T07:10:15.113Z
Learning: In MoonModules/MoonLight (lib/PsychicHttp/src/PsychicWebSocket.cpp): `PsychicWebSocketClient::sendMessage()` must guard with `httpd_ws_get_fd_info()` before calling `httpd_ws_send_frame_async()`. Without the guard, when a client disconnects and reconnects (reusing the same socket fd) while a browser tab is hidden, repeated failed sends degrade the lwIP netconn state until `lwip_netconn_do_write` hard-asserts `(state!)` instead of returning ESP_FAIL, causing a reboot. Fix: check `httpd_ws_get_fd_info(server, socket) == HTTPD_WS_CLIENT_WEBSOCKET` first and return ESP_FAIL early if not. Also, in EventSocket.cpp, erase stale clients from subscriptions on ESP_FAIL instead of keeping them.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 136
File: src/MoonBase/LiveScriptNode.cpp:22-39
Timestamp: 2026-03-04T09:15:48.019Z
Learning: In MoonModules/MoonLight LiveScriptNode (src/MoonBase/LiveScriptNode.cpp): The global `gNode` (Node*) and `WaitAnimationSync` (binary semaphore) are intentionally global because ESPLiveScript's `addExternalFunction()` only accepts plain C function pointers with no void* userData/context parameter, and `setFunctionToSync()` accepts a single void(*)() with no per-instance binding. There is no way to attach per-instance state to callbacks within the current ESPLiveScript API. This is a known limitation acknowledged with a `// todo: this is not working well with multiple scripts running!!!` comment at line 211. Fixing it would require modifying the ESPLiveScript library itself to support userData/context on callbacks.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
ewowi
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
Bug Fixes
New Features / Improvements