Consolidate parallel LED drivers into one runtime-peripheral module; fix MoonI80 double-buffer freeze + robustness#53
Merged
Merged
Conversation
The MoonI80 driver stalled to ~5 FPS after a second in whole-frame (non-expander) mode: a lost DMA end-of-frame interrupt left the bus marked busy forever, and only the ring path had recovery. Both paths now share one stall-recovery routine, so a lost interrupt self-heals on either. Also lands a batch of driver hardening: the classic-ESP32 i80 driver refuses an oversized frame with a clear message instead of crashing, the RMT and preview drivers report honest status, and expert-only controls are hidden by default.
KPI: 16384lights | Desktop:755KB | tick:131/107/2/5/131/21/3/286/73/18/23/169/129/23/7/49us(FPS:7633/9345/500000/200000/7633/47619/333333/3496/13698/55555/43478/5917/7751/43478/142857/20408) | ESP32:1498KB | tick:3516us(FPS:284) | heap:58KB | src:194(45635) | test:137(24364) | lizard:160w
Core:
- platform_esp32_moon_i80: extracted finalizeStalledTransfer as the single stop-and-clear both the ring and whole-frame wait-timeout backstops call, and added the whole-frame backstop the driver lacked (a lost/coalesced EOF left busy stuck true with no recovery, wedging every later transmit to its full timeout). The whole-frame backstop stops the LCD + GDMA before draining its completion FIFO, and the EOF ISR now drops a firing against a structurally empty FIFO (fifoTail == fifoHead), so a late EOF after the drain can't hand out a spurious done token. finalizeStalledTransfer lives in the file's anonymous namespace alongside the other internal helpers.
Light domain:
- ParallelLedDriver: added frameFitsDmaBudget + a dmaBudgetBytes CRTP hook so a whole-frame driver whose DMA can't reach PSRAM refuses an oversized frame with an actionable status ("frame NKB over i80 DMA MKB: fewer lights/pin") instead of busy-waiting to a watchdog reset; the pinExpander control is hidden on drivers that don't support it.
- MultiPinLedDriver: dmaBudgetBytes reports the classic-ESP32 i80 (I2S, internal-RAM-only) DMA budget so the frame-fit gate engages there; PSRAM-capable chips and the streaming ring report no bound.
- RmtLedDriver: sized the symbol buffer to the driven light count rather than the whole window (was over-allocating ~550KB and failing on classic); report the resting "driving N of M lights" status on boot; loopbackTest is expert-mode only.
- PreviewDriver: the adaptive-resolution downscale recovers multiplicatively (halves toward full res) instead of one step per clean run, and re-anchors to full res on prepare(), so a small grid settles in ~1s instead of ~10s.
Tests:
- unit_MultiPinLedDriver: frame-fit-gate and pinExpander-hidden cases.
- unit_PreviewDriver: re-anchor and fast-recovery cases.
- unit_RmtLedDriver_lifecycle: symbol-buffer sizing and driving-status-on-boot cases.
Docs / CI:
- backlog-core: recorded the prime-only ring stall-backstop gap (the third completion path the whole-frame fix leaves uncovered) with the sibling-path fix named.
Reviews:
- 👾 F1 (fixed): RmtLed's resting-status re-assert masked a per-pin RMT init failure (Severity::Error without setting configErr_/configWarn_), so a dead strand read "healthy" — now gated on inited_.
- 👾 F2 (fixed): deleted a verbatim-duplicated 3-line comment in ParallelLedDriver.
- 👾 F3 (fixed): whole-frame FIFO drain could race a late EOF into a spurious done token — guarded the ISR pop on a non-empty FIFO and stop the hardware before draining.
- 👾 F4 (fixed): finalizeStalledTransfer had external linkage outside the anonymous namespace — moved it inside.
- 👾 F5 (deferred, backlogged): prime-only ring has no stall backstop; same wedge class, far rarer trigger (one EOF/frame), needs expander-wall verification — recorded in backlog-core with the fix named.
- 👾 F6 (fixed): trimmed a redundant deinit()/inited_ re-call in the budget-fail branch and a dead false-arm in the Preview downscale ternary.
- 👾 F7 (accepted): MultiPin dmaBudgetBytes halves unconditionally for a possible double-buffer; deliberate and commented, single-buffer configs near the budget edge are conservatively refused.
tick: 131us, 107us, 2us, 5us, 131us, 21us, 3us, 286us, 73us, 18us, 23us, 169us, 129us, 23us, 7us, 49us (FPS: 7633, 9345, 500000, 200000, 7633, 47619, 333333, 3496, 13698, 55555, 43478, 5917, 7751, 43478, 142857, 20408) (per scenario)
tick: 3516us (FPS: 284) heap free: 60224
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request consolidates parallel LED drivers behind runtime-selectable peripherals, adds log-level controls and render/persistence safeguards, updates transfer recovery and adaptive behavior, and refreshes tests, tooling, installer content, device models, documentation, and scenario measurements. ChangesRuntime parallel LED driver
Platform and behavior updates
Documentation and scenario support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant UI
participant ParallelLedDriver
participant LedPeripheral
participant PlatformBus
UI->>ParallelLedDriver: change peripheral control
ParallelLedDriver->>LedPeripheral: select backend and rebuild controls
ParallelLedDriver->>LedPeripheral: initialize and transmit frame
LedPeripheral->>PlatformBus: start backend-specific DMA transfer
PlatformBus-->>LedPeripheral: completion or timeout
LedPeripheral-->>ParallelLedDriver: wait result and bus state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 9
🤖 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 `@src/light/drivers/MultiPinLedDriver.h`:
- Around line 99-115: Update dmaBudgetBytes() so the classic-I2S path never
returns 0: reserve space, derive the bounded budget from one frame’s capacity
without halving for double buffering, and return a small positive fallback when
block <= kReserve. Keep the LCD_CAM path returning 0 for unbounded capacity, and
leave busInit() responsible for downgrading the optional second buffer.
In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 1272-1278: Update the comment above frameFitsDmaBudget() to state
that bounded drivers call it from reinit(), replacing the incorrect
parseConfig() reference while preserving the surrounding DMA-budget behavior
description.
In `@src/light/drivers/PreviewDriver.h`:
- Around line 208-213: Update the recovery comments near the refinement logic at
lines 191 and 609 to describe multiplicative halving of downscale_ rather than
linear decrementing with downscale_--. Keep the comments consistent with the
implementation in the surrounding recovery paths.
- Around line 100-109: Update the adaptive-state reset at the rebuild seam in
PreviewDriver so framesWaiting_ is also reset alongside downscale_, slowStreak_,
and cleanStreak_. This ensures cancelBufferedSend() cannot carry a slow-frame
count into the first frame of the new grid.
In `@src/light/drivers/RmtLedDriver.h`:
- Around line 418-430: Update the buffer-sizing logic near windowSlice so a zero
txLightCount_ produces no allocation instead of falling back to win. Preserve
the window cap for configured outputs, and ensure allocation is deferred until
the prepare path runs after valid pins populate txLightCount_, while tick()
remains idle with no buffer.
In `@src/platform/esp32/platform_esp32_moon_i80.cpp`:
- Around line 1673-1678: Extend the stalled-transfer recovery around
finalizeStalledTransfer so it also handles prime-only ring frames currently
excluded by the isRing && nSlices <= ringBufs condition. Base the
elapsed-wire-time threshold on the prime-only frame duration and include its
optional reset-tail slice, ensuring a lost terminator EOF clears busy and allows
subsequent prime/arm/transmit calls.
- Around line 1689-1692: Update the recovery logic around
finalizeStalledTransfer and the fifoTail/fifoHead reset so late GDMA EOF
handling is serialized with recovery and rearming. Ensure any ISR that has
already passed the empty-queue guard cannot clear the new busy state or call
wireFree for the next whole-frame transfer; synchronously stop and clear pending
EOFs, or use an explicit recovery/rearm state before accepting a new frame.
In `@test/unit/light/unit_MultiPinLedDriver.cpp`:
- Line 109: Update the comment near the classic i80 DMA behavior note to use the
American English spelling “behavior” instead of “behaviour,” without changing
its meaning.
- Around line 103-123: Update test/unit/light/unit_MultiPinLedDriver.cpp lines
103-123 to exercise frameFitsDmaBudget() with synthetic finite and zero budgets,
asserting rejection for oversized frames only under a finite budget and no size
rejection when the active target budget is zero; update lines 317-337 to assert
the control’s hidden value equals !kSupportsPinExpander instead of assuming pin
expanders are unsupported.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8afede06-6ec0-427a-a80d-15e01c3a6ab5
📒 Files selected for processing (9)
docs/backlog/backlog-core.mdsrc/light/drivers/MultiPinLedDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/PreviewDriver.hsrc/light/drivers/RmtLedDriver.hsrc/platform/esp32/platform_esp32_moon_i80.cpptest/unit/light/unit_MultiPinLedDriver.cpptest/unit/light/unit_PreviewDriver.cpptest/unit/light/unit_RmtLedDriver_lifecycle.cpp
Comment on lines
+418
to
+430
| // Size for the lights this driver actually CLOCKS OUT, not the whole window. The window (start, | ||
| // count) can be far larger than the pins encode: `ledsPerPin` (or fewer pins than the window has | ||
| // lights) caps the transmitted total at `txLightCount_` (Σ pinCounts_), and tick() only ever | ||
| // encodes that many (n = min(txLightCount_, winLen_) there). Sizing to the window instead made an | ||
| // 8×8 strip on one pin (ledsPerPin 64) inside a 70×82 grid (count=all, window 5740) try to alloc | ||
| // ~550 KB of symbols for lights it never encodes — the alloc failed on a small-heap classic ESP32, | ||
| // symbols_ stayed null, and tick() bailed → the strip went dark even though only 64 lights were | ||
| // wanted. Bound to txLightCount_ so the buffer matches the real output. Fall back to the window | ||
| // when no pins are parsed yet (txLightCount_ == 0), so the buffer is ready before pins are set. | ||
| nrOfLightsType winStart, win; | ||
| windowSlice(sourceBuffer_->count(), winStart, win); | ||
| nrOfLightsType n = txLightCount_ > 0 ? txLightCount_ : win; | ||
| if (n > win) n = win; // never exceed the window's own light count |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not allocate a full-window buffer when no outputs are configured.
With an empty/invalid pin list, txLightCount_ is zero and tick() idles, but this fallback allocates for win. A 16K RGB window attempts a ~1.5 MB symbol allocation solely because no pins exist; if it succeeds, it unnecessarily retains that memory. Keep the idle state allocation-free and allocate on the prepare sweep once valid pins are configured.
Proposed fix
- nrOfLightsType n = txLightCount_ > 0 ? txLightCount_ : win;
+ if (pinCount_ == 0 || txLightCount_ == 0) {
+ freeSymbols();
+ return;
+ }
+ nrOfLightsType n = txLightCount_;
if (n > win) n = win; // never exceed the window's own light count📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Size for the lights this driver actually CLOCKS OUT, not the whole window. The window (start, | |
| // count) can be far larger than the pins encode: `ledsPerPin` (or fewer pins than the window has | |
| // lights) caps the transmitted total at `txLightCount_` (Σ pinCounts_), and tick() only ever | |
| // encodes that many (n = min(txLightCount_, winLen_) there). Sizing to the window instead made an | |
| // 8×8 strip on one pin (ledsPerPin 64) inside a 70×82 grid (count=all, window 5740) try to alloc | |
| // ~550 KB of symbols for lights it never encodes — the alloc failed on a small-heap classic ESP32, | |
| // symbols_ stayed null, and tick() bailed → the strip went dark even though only 64 lights were | |
| // wanted. Bound to txLightCount_ so the buffer matches the real output. Fall back to the window | |
| // when no pins are parsed yet (txLightCount_ == 0), so the buffer is ready before pins are set. | |
| nrOfLightsType winStart, win; | |
| windowSlice(sourceBuffer_->count(), winStart, win); | |
| nrOfLightsType n = txLightCount_ > 0 ? txLightCount_ : win; | |
| if (n > win) n = win; // never exceed the window's own light count | |
| // Size for the lights this driver actually CLOCKS OUT, not the whole window. The window (start, | |
| // count) can be far larger than the pins encode: `ledsPerPin` (or fewer pins than the window has | |
| // lights) caps the transmitted total at `txLightCount_` (Σ pinCounts_), and tick() only ever | |
| // encodes that many (n = min(txLightCount_, winLen_) there). Sizing to the window instead made an | |
| // 8×8 strip on one pin (ledsPerPin 64) inside a 70×82 grid (count=all, window 5740) try to alloc | |
| // ~550 KB of symbols for lights it never encodes — the alloc failed on a small-heap classic ESP32, | |
| // symbols_ stayed null, and tick() bailed → the strip went dark even though only 64 lights were | |
| // wanted. Bound to txLightCount_ so the buffer matches the real output. Fall back to the window | |
| // when no pins are parsed yet (txLightCount_ == 0), so the buffer is ready before pins are set. | |
| nrOfLightsType winStart, win; | |
| windowSlice(sourceBuffer_->count(), winStart, win); | |
| if (pinCount_ == 0 || txLightCount_ == 0) { | |
| freeSymbols(); | |
| return; | |
| } | |
| nrOfLightsType n = txLightCount_; | |
| if (n > win) n = win; // never exceed the window's own light count |
🤖 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 `@src/light/drivers/RmtLedDriver.h` around lines 418 - 430, Update the
buffer-sizing logic near windowSlice so a zero txLightCount_ produces no
allocation instead of falling back to win. Preserve the window cap for
configured outputs, and ensure allocation is deferred until the prepare path
runs after valid pins populate txLightCount_, while tick() remains idle with no
buffer.
Source: Coding guidelines
Comment on lines
+1673
to
1678
| finalizeStalledTransfer(st); // the shared stop-and-clear; the next arm re-links the chain | ||
| // Latch whatever this frame's refills managed, so the ea readout isn't stuck at a half window. | ||
| st->dbgEncAvgUs = st->dbgEncCount ? st->dbgEncSumUs / st->dbgEncCount : st->dbgEncAvgUs; | ||
| st->dbgEncSumUs = 0; | ||
| st->dbgEncCount = 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Recover stalled prime-only ring frames too.
isRing && nSlices <= ringBufs bypasses this backstop, so a lost terminator EOF leaves busy set forever and all later prime/arm/transmit calls reject the bus. Extend the elapsed-wire-time recovery to prime-only frames, accounting for its optional reset-tail slice.
🤖 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 `@src/platform/esp32/platform_esp32_moon_i80.cpp` around lines 1673 - 1678,
Extend the stalled-transfer recovery around finalizeStalledTransfer so it also
handles prime-only ring frames currently excluded by the isRing && nSlices <=
ringBufs condition. Base the elapsed-wire-time threshold on the prime-only frame
duration and include its optional reset-tail slice, ensuring a lost terminator
EOF clears busy and allows subsequent prime/arm/transmit calls.
Comment on lines
+1689
to
+1692
| if (!st->isRing && st->busy) { | ||
| finalizeStalledTransfer(st); // stop LCD + GDMA FIRST, so no EOF can fire during the drain below | ||
| st->fifoTail = st->fifoHead; // then drop the un-completed entry; the ISR guard ignores a late EOF | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 \
'moonI80EofCb|finalizeStalledTransfer|moonI80Ws2812Wait|moonI80Ws2812Transmit|fifoHead|fifoTail|wireFree|busy' \
src/platform/esp32/platform_esp32_moon_i80.cppRepository: MoonModules/projectMM
Length of output: 32022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant whole-frame/wait/recovery/stall and ISR code around the reported lines.
sed -n '480,585p' src/platform/esp32/platform_esp32_moon_i80.cpp
printf '\n--- lines 1596-1705 ---\n'
sed -n '1596,1705p' src/platform/esp32/platform_esp32_moon_i80.cpp
printf '\n--- references to startTransfer/fifoHead/fifoTail/busy ---\n'
rg -n -C2 'startTransfer|fifoHead|fifoTail|busy|done\[|wireFree' src/platform/esp32/platform_esp32_moon_i80.cppRepository: MoonModules/projectMM
Length of output: 34076
Block new whole-frame transfers until the late-EF floodgates are closed.
Draining fifoTail = fifoHead only makes the current EOF drop future arrivals; it does not prevent an ISR that already passed st->fifoTail == st->fifoHead from returning and subsequently clearing the new busy flag and issuing wireFree. Move recovery to a single serialized EOF/recovery/rearm state, or synchronously stop and clear any queued/pending GDMA EOF before rearming the next frame.
🤖 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 `@src/platform/esp32/platform_esp32_moon_i80.cpp` around lines 1689 - 1692,
Update the recovery logic around finalizeStalledTransfer and the
fifoTail/fifoHead reset so late GDMA EOF handling is serialized with recovery
and rearming. Ensure any ISR that has already passed the empty-queue guard
cannot clear the new busy state or call wireFree for the next whole-frame
transfer; synchronously stop and clear pending EOFs, or use an explicit
recovery/rearm state before accepting a new frame.
Adds a serial log-level control so a resting device stops writing the once-a-second KPI line (an S3's onboard LED flickered on that serial TX); default Warn keeps warnings and errors, and the first 60 s always logs at Info for the installer's IP read. The web installer gains WLED Native and Home Assistant links with official store badges and a browser-access chip. Documentation: the getting-started page gets those two how-to-control sections plus a fixed right-side TOC, the MHC-WLED P4-shield hardware reference is rewritten from the builder's schematics (the board does have a mechanical RS-485 direction switch, and the loopback works through it), and a Release 4 scope plan is recorded. KPI: 16384lights | Desktop:755KB | tick:132/105/2/7/131/21/3/288/73/18/23/172/130/23/7/47us(FPS:7575/9523/500000/142857/7633/47619/333333/3472/13698/55555/43478/5813/7692/43478/142857/21276) | tick:3522us(FPS:283) | heap:58KB | src:194(45709) | test:137(24384) | lizard:160w Core: - platform.h / platform_esp32 / platform_desktop: added a LogLevel enum + setLogLevel seam (ESP32 maps straight to esp_log_level_set; desktop is a no-op). - SystemModule: a logLevel select control (None/Error/Warn/Info/Debug/Verbose, default Warn), applied to the platform logger on change and at boot; advanced-mode. - main.cpp: gate the once-a-second KPI printf on logLevel >= Info OR the first 60 s of uptime, so a resting device makes no periodic serial write while warnings/errors still surface. Light domain: - MultiPinLedDriver: dmaBudgetBytes on the bounded classic-i80 path never returns 0 (which means "no bound") and no longer halves for double-buffering (busInit downgrades the second buffer on its own); a starved board now still rejects an oversized frame. - ParallelLedDriver: corrected the frameFitsDmaBudget comment (called from reinit, not parseConfig). - PreviewDriver: recovery comments now describe the multiplicative halving the code does; reset framesWaiting_ at the rebuild seam so the old grid's drain count can't make the new grid's first frame read slow. Scripts / MoonDeck: - _moondeck_config (new): shared active-device-IP + logLevel-toggle helpers (mirrors the _net_probe shared-module pattern), imported by both consumers. - collect_kpi + monitor_esp32: raise the device to Info over /api/control before reading serial and restore Warn after, so KPI capture and monitoring still see the tick line. - preview_installer: stage web-installer/ recursively (+ .svg) so a subdirectory of assets (the app-store badges) is served in preview, matching the deploy's recursive copy. UI: - web-installer: WLED Native (App Store + Google Play official badges) and Home Assistant links in the "control your device" card, a vendor-neutral globe chip for browser access, and the Step 3 heading reworded to cover all three; the help link points at the getting-started page. Badges committed under web-installer/assets/. Tests: - unit_MultiPinLedDriver: exercise frameFitsDmaBudget directly with synthetic finite/zero budgets; assert the pinExpander control's hidden state equals !kSupportsPinExpander; US spelling fix. Docs / CI: - gettingstarted: added "control from your phone (WLED Native)" and "smart home (Home Assistant)" sections with images and store links; fixed the missing right-side TOC by demoting the two Chapter H1s so the page has a single title H1. - system.md: documented the logLevel control. - architecture: removed the 🚧 designed-not-implemented tagging now that the two-core render/output split has shipped (the clock-sync/light-distribution items describe themselves as not-yet-wired in prose). - reference/mhc-wled-esp32-p4-shield: rewritten from the builder's schematics + terminal maps — the board has a mechanical RS-485 direction switch (SW5) on the GPIO 3 channel, the RS-485 outputs are a range extender + DMX-512 output, and the loopback works Tx=GPIO4 -> Rx=GPIO3 through that switch; new images replace the old pinout SVG. - backlog-light: reconciled the RS-485 DE/RE note with the shield's mechanical switch. - backlog-core: recorded the MoonI80 prime-only ring stall-backstop gap and the whole-frame late-EOF serialization hardening (both need the expander wall). - history/plans: added the Release 4 scope plan (effect-breadth migration + rename runway + the deferred hardware work). Reviews: - 🐇 MultiPin dmaBudgetBytes (fixed): starved classic path returned 0 = "no bound"; now a positive floor, no double-buffer halving. - 🐇 ParallelLedDriver comment (fixed): frameFitsDmaBudget is called from reinit, not parseConfig. - 🐇 PreviewDriver comments + framesWaiting_ (fixed): comments now match the multiplicative recovery; framesWaiting_ reset at the rebuild seam. - 🐇 RmtLed zero-txLightCount buffer (skipped): the window fallback is intentional pre-warm codified by the lifecycle tests (their wire() helper sets no pins); forcing no-allocation broke 6 tests for a non-bug, reverted. - 🐇 MoonI80 prime-only backstop + late-EOF serialization (deferred): both are ISR/ring changes that must be proven on the expander wall; backlogged with the fix named. - 🐇 unit_MultiPin tests (fixed): synthetic-budget assertions, hidden == !kSupportsPinExpander, US spelling. tick: 132us, 105us, 2us, 7us, 131us, 21us, 3us, 288us, 73us, 18us, 23us, 172us, 130us, 23us, 7us, 47us (FPS: 7575, 9523, 500000, 142857, 7633, 47619, 333333, 3472, 13698, 55555, 43478, 5813, 7692, 43478, 142857, 21276) (per scenario) tick: 3522us (FPS: 283) heap free: 60224 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@docs/architecture.md`:
- Line 3: Update the architecture contract introduction to use present-tense
wording: replace the future-tense “how it will work” phrasing with language
stating that the described behavior is intended and code is written toward it,
while preserving the surrounding meaning.
In `@docs/backlog/backlog-core.md`:
- Around line 506-507: Update the wording in the related-description paragraph
to claim only that the empty-FIFO check rejects late EOFs after recovery, rather
than saying it closes the common lost/late-EOF race. Preserve the following
sentence documenting the remaining race from an ISR already past the empty-FIFO
guard, and make no implementation changes.
In `@docs/backlog/backlog-light.md`:
- Line 94: Update the “platform:: UART-RS485 seam” requirement to make DE/RE
control necessary only for bidirectional RS-485 channels or boards without fixed
transmit wiring. Explicitly exclude fixed-transmit channels such as GPIO 4, 22,
and 24 on the MHC-WLED ESP32-P4 shield, while retaining the requirement for its
switchable GPIO 3 channel.
In `@docs/gettingstarted.md`:
- Around line 221-255: The WLED Native and Home Assistant guidance must have one
canonical location: retain the detailed sections in docs/gettingstarted.md lines
221-255, and update web-installer/index.html lines 204-241 to use only concise
badges or calls to action linking to that guide, removing repeated product
claims and external integration links there.
In `@docs/history/plans/Plan-20260722` - Release 4 scope - effect breadth + rename
runway.md:
- Around line 17-18: Update the plan prose in the “Shared palette” bullet to use
the American spelling “color” instead of “colour”; leave the `ColorFromPalette`
identifier unchanged.
In `@docs/reference/mhc-wled-esp32-p4-shield.md`:
- Around line 3-17: The hardware reference currently mixes a V2 render with
V1-specific wiring without defining compatibility. Update the document header
and related pinout, RS-485, and loopback sections to explicitly state whether V2
shares the V1 wiring; if not, separate the V1 and V2 hardware details so the
applicable pinout is unambiguous.
In `@moondeck/_moondeck_config.py`:
- Around line 35-46: Restore each device’s original System.logLevel after
temporary monitoring changes. In moondeck/_moondeck_config.py lines 35-46, add a
scoped helper alongside set_log_level that captures each target’s current level
and restores it safely. Update moondeck/check/collect_kpi.py lines 297-316 to
use the saved state on serial-open failure and normal cleanup, and update
moondeck/run/monitor_esp32.py lines 29-57 to restore it on monitor failure and
normal cleanup; preserve the required temporary Info level.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6c429465-cb4a-47f3-a105-f8a9d0e67294
⛔ Files ignored due to path filters (9)
docs/assets/reference/mhc-wled-esp32-p4-shield-gpio-terminal-map.pngis excluded by!**/*.pngdocs/assets/reference/mhc-wled-esp32-p4-shield-inout-header.pngis excluded by!**/*.pngdocs/assets/reference/mhc-wled-esp32-p4-shield-pinout.svgis excluded by!**/*.svgdocs/assets/reference/mhc-wled-esp32-p4-shield-rs485-gpio3-switchable-schematic.pngis excluded by!**/*.pngdocs/assets/reference/mhc-wled-esp32-p4-shield-rs485-loopback-wiring.pngis excluded by!**/*.pngdocs/assets/reference/mhc-wled-esp32-p4-shield-rs485-transmit-schematic.pngis excluded by!**/*.pngweb-installer/assets/app-store-badge.svgis excluded by!**/*.svgweb-installer/assets/google-play-badge.pngis excluded by!**/*.pngweb-installer/assets/home-assistant-icon.pngis excluded by!**/*.png
📒 Files selected for processing (22)
docs/architecture.mddocs/backlog/backlog-core.mddocs/backlog/backlog-light.mddocs/gettingstarted.mddocs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.mddocs/moonmodules/core/system.mddocs/reference/mhc-wled-esp32-p4-shield.mdmoondeck/_moondeck_config.pymoondeck/check/collect_kpi.pymoondeck/run/monitor_esp32.pymoondeck/run/preview_installer.pysrc/core/SystemModule.hsrc/light/drivers/MultiPinLedDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/PreviewDriver.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/unit/light/unit_MultiPinLedDriver.cppweb-installer/index.htmlweb-installer/install.css
…docs
Adding a layer no longer blacks out the scene: the default blend mode is now additive (adds light) instead of alpha-over (which painted a sparse layer's black pixels over everything below). Also processes a round of review findings across the drivers and installer, rewrites the MHC-WLED P4-shield hardware reference from the builder's schematics, and records two Release-4-direction plans.
KPI: 16384lights | Desktop:755KB | tick:184/122/2/103/128/38/3/301/66/19/23/170/125/20/5/43us(FPS:5434/8196/500000/9708/7812/26315/333333/3322/15151/52631/43478/5882/8000/50000/200000/23255) | tick:3529us(FPS:283) | heap:55KB | src:194(45714) | test:137(24384) | lizard:160w
Light domain:
- Layer: default blendMode is now additive (index 1) instead of alpha. A newly-added layer adds light onto the layers below and never blacks them out (the common case: a sparse effect stacked on a background). Alpha (over) stays opt-in for full-frame layers that mean to cover what's below. The enum index order is unchanged, so a persisted preset's stored blendMode keeps its meaning.
Scripts / MoonDeck:
- _moondeck_config: added a raised_log_level() context manager that captures each device's current logLevel and restores it on exit (serial-open failure and normal cleanup alike), instead of forcing Warn — a device the user set to Debug/Error keeps its choice.
- collect_kpi + monitor_esp32: use the context manager for the temporary Info-during-capture / restore-after flow.
UI:
- web-installer: trimmed the duplicated WLED Native / Home Assistant product prose in the Step 3 card to one concise call-to-action linking the getting-started guide; the store/HA badges stay as the affordance (the detailed claims live once, in gettingstarted.md).
Tests:
- scenario_Layers_composition: description updated to note the default blend is now additive.
Docs / CI:
- architecture: present-tense fix ("this is the intended behavior, and code is written toward it").
- reference/mhc-wled-esp32-p4-shield: rewritten from the builder's schematics — the board has a mechanical RS-485 direction switch (SW5) on the GPIO 3 channel, the RS-485 outputs serve as a range extender + DMX-512 output, and the loopback works Tx=GPIO4 -> Rx=GPIO3 through that switch; added a board-revision note (the map is V1-specific; V2 compatibility unconfirmed).
- backlog-core: softened the MoonI80 late-EOF wording to "rejects a late EOF after recovery" (a race remains, documented in the next sentence).
- backlog-light: RS-485 DE/RE control is needed only for a bidirectional channel; the shield's GPIO 4/22/24 are fixed-transmit (no DE/RE), only the switchable GPIO 3 is bidirectional.
- history/plans: added the Release 4 scope plan (effect-breadth migration + rename runway + hardware-verified driver work); the R4 plan's "colour" spelling fixed to "color". Added the approved plan to consolidate the parallel LED drivers into one module + a peripheral strategy interface.
Reviews:
- 🐇 architecture.md present-tense (fixed): removed "how it will work".
- 🐇 backlog-core late-EOF wording (fixed): "closes the race" -> "rejects a late EOF after recovery".
- 🐇 backlog-light DE/RE requirement (fixed): scoped to bidirectional channels; fixed-transmit channels excluded.
- 🐇 installer / gettingstarted duplication (fixed): product claims live once in gettingstarted; installer is concise CTAs + badges.
- 🐇 R4 plan spelling (fixed): colour -> color.
- 🐇 P4-shield V1/V2 ambiguity (fixed): board-revision note added; V2 compatibility stated as unconfirmed rather than invented.
- 🐇 _moondeck_config restore (fixed): capture+restore the device's original logLevel instead of a hardcoded Warn.
tick: 184us, 122us, 2us, 103us, 128us, 38us, 3us, 301us, 66us, 19us, 23us, 170us, 125us, 20us, 5us, 43us (FPS: 5434, 8196, 500000, 9708, 7812, 26315, 333333, 3322, 15151, 52631, 43478, 5882, 8000, 50000, 200000, 23255) (per scenario)
tick: 3529us (FPS: 283) heap free: 57000
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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)
moondeck/_moondeck_config.py (1)
70-76: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
set_log_level()non-throwing for malformed device entries.
Request(...)is outsidetry. A malformed nonempty IP can abort the loop after earlier devices were switched to Info;raised_log_level()then never reaches its restorationfinally.Proposed fix
for ip in ips: - req = urllib.request.Request(f"http://{ip}/api/control", data=body, - headers={"Content-Type": "application/json"}, method="POST") try: - urllib.request.urlopen(req, timeout=3).read() + req = urllib.request.Request( + f"http://{ip}/api/control", + data=body, + headers={"Content-Type": "application/json"}, + method="POST", + ) + with urllib.request.urlopen(req, timeout=3) as response: + response.read() except Exception: pass🤖 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 `@moondeck/_moondeck_config.py` around lines 70 - 76, Move Request construction inside the try block in set_log_level(), so malformed nonempty IP entries are caught and skipped without aborting iteration. Preserve processing of remaining devices and ensure raised_log_level() can reach its restoration finally block.
🤖 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 `@docs/history/plans/Plan-20260722` - Release 4 scope - effect breadth + rename
runway.md:
- Line 17: Update the plan text near the “Shared palette” bullet to use the
American spelling “Generalize” instead of “Generalise”; leave the existing
ColorFromPalette identifier unchanged.
In `@docs/history/plans/Plan-20260723` - Consolidate parallel LED drivers into one
module + peripheral strategy.md:
- Around line 5-15: Correct the parallel-driver count in the opening description
from five classes to four: one ParallelLedDriver<Derived> base and three
concrete subclasses. Update the corresponding net-effect statement from “5
classes → 1” to “4 classes → 1,” while keeping RmtLedDriver excluded as
established by the separate-scope statement.
In `@web-installer/index.html`:
- Around line 232-234: Update the Home Assistant badge image in the anchor
containing class "ha-badge" to use an empty alt attribute, since the adjacent
span already provides the accessible name; leave the visible span label
unchanged.
---
Outside diff comments:
In `@moondeck/_moondeck_config.py`:
- Around line 70-76: Move Request construction inside the try block in
set_log_level(), so malformed nonempty IP entries are caught and skipped without
aborting iteration. Preserve processing of remaining devices and ensure
raised_log_level() can reach its restoration finally block.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ca8cdf4e-4504-4ac0-a98e-d3425d5c8c82
📒 Files selected for processing (12)
docs/architecture.mddocs/backlog/backlog-core.mddocs/backlog/backlog-light.mddocs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.mddocs/history/plans/Plan-20260723 - Consolidate parallel LED drivers into one module + peripheral strategy.mddocs/reference/mhc-wled-esp32-p4-shield.mdmoondeck/_moondeck_config.pymoondeck/check/collect_kpi.pymoondeck/run/monitor_esp32.pysrc/light/layers/Layer.htest/scenarios/light/scenario_Layers_composition.jsonweb-installer/index.html
The three parallel LED drivers (MultiPin/Moon/Parlio) become one registered ParallelLedDriver whose `peripheral` control picks the DMA bus backend at runtime, plus fixes for two crash/robustness bugs the pre-commit reviewer found on the feature's headline paths (a live peripheral swap left the control list dangling; a string palette value dereferenced a function pointer). Verified live on the S3 and classic Olimex.
KPI: 16384lights | Desktop:755KB | tick:130/106/2/6/131/21/3/287/73/18/23/169/129/23/6/47us(FPS:7692/9433/500000/166666/7633/47619/333333/3484/13698/55555/43478/5917/7751/43478/166666/21276) | src:195(46120) | test:138(24852) | lizard:160w
(ESP32 live tick omitted: the configured KPI port is stale and boards were on other ports; all 3 ESP32 variants built clean and the fixes were hardware-verified on S3 + classic over the API.)
Core:
- ParallelLedDriver: the CRTP base + 3 driver subclasses collapse into one plain MoonModule holding a runtime LedPeripheral*; the `peripheral` Select picks the backend, board-filtered to lanesAvailable()>0, with a per-hardware-block claim guard so two same-block drivers can't corrupt each other.
- ParallelLedDriver: fix the live peripheral swap double-swapping (rebuildControls already swaps in the new backend; onControlChanged now calls ensurePeripheralMatchesSelection, a no-op post-rebuild, instead of a second swapPeripheral that freed the just-bound backend and dangled the control list).
- ParallelLedDriver: gate hwBlock() on inited_ so a refused/uninited sibling no longer phantom-claims its peripheral block and darks a live wall on an unrelated prepare sweep.
- LedPeripheral: new runtime strategy interface (bus lifecycle + descriptors + ring/bus-pin cluster) with a self-registering per-backend factory registry gated by CONFIG_SOC_*, so a chip links only its usable backends.
- Control.cpp: split the Select/Palette apply so the option-label string match runs for Select only; a Palette's aux is a PaletteOptionsFn (function pointer), so a string palette value now falls to numeric apply instead of dereferencing code bytes (crash on ESP32; Robust to any input).
- SystemModule: correct a stale comment (logLevel re-applies via onControlChanged, not affectsPrepare).
Light domain:
- I80Peripheral / MoonI80Peripheral / ParlioPeripheral: the three backends become LedPeripheral implementations, each self-registering under its label; reframed the class docs from "thin driver subclass" to "peripheral backend".
Tests:
- unit_ParallelLedDriver_swap: new regression pinning the double-swap (registers two owned mock backends, drives the real setControl sequence; counts attached-backend destructions, which is 2 with the bug and 1 with the fix — verified green->red).
- unit_Control_apply_absent_key: added a string-palette-value case (must not crash, applies numerically) and a Select-label case (label match still works).
- unit_{MultiPin,Moon,Parlio}LedDriver + the ring/doublebuffer/pinexpander mocks: retargeted to construct ParallelLedDriver + inject the matching backend.
Docs / CI:
- MIGRATING: the 3-drivers-merge entry; and the older 2026-07-16 rename entry now points forward to the current type (the two intermediate names it named no longer exist).
- drivers.md: the comparison table is now peripheral rows under the one Parallel LED driver, not three separate driver cards.
- architecture.md, mhc-p4-shield, s31-coreboard: old driver names -> the Parallel LED driver + peripheral model.
- CLAUDE.md: added the Researcher role row + the "delegate the mechanical roles" heuristic note.
Reviews:
- 👾 peripheral swap double-free / dangling control list: fixed (ensurePeripheralMatchesSelection in onControlChanged), regression-tested, verified live on S3.
- 👾 string palette value crash (function-pointer deref): fixed (Select-only label match), regression-tested, verified live on S3 + classic.
- 👾 claim guard phantom-claim: fixed (hwBlock gated on inited_).
- 🐇 Plan-20260722/23 wording (Generalise->Generalize, five->four classes), index.html ha-badge empty alt, _moondeck_config Request-inside-try: fixed.
KPI Details:
Desktop: 16,384 lights | 755 KB | 837 test cases pass | 21 scenarios pass | boundary PASS | specs 87/87
ESP32: image 1,493,812 bytes (64% partition free)
Code: 195 src (46120 lines), 138 test (24852 lines), 146 specs, 21 scenarios, lizard 160w
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@docs/architecture.md`:
- Line 472: Update the LED drivers entry in docs/architecture.md to document the
classic ESP32 parallel backend, including its MultiPin/classic I2S support and
i80 linkage, or explicitly explain why it is excluded. Ensure the architecture
contract no longer implies classic ESP32 lacks parallel output.
In `@docs/gettingstarted.md`:
- Around line 202-203: Remove the empty blockquote line immediately after the
migration-notes paragraph in the getting-started documentation, keeping the
surrounding blockquote content unchanged so markdownlint MD028 passes.
In `@docs/reference/esp32-s31-coreboard.md`:
- Line 126: Reconcile the LCD_CAM statement in the loopback self-test
documentation with the existing S31 capability reference. Update the explanation
to describe the actual limitation, such as unavailable loopback pin mappings or
missing board/catalog configuration, rather than claiming the S31 lacks LCD_CAM;
if the capability listing is incorrect for this board, update that listing
consistently.
In `@docs/reference/mhc-wled-esp32-p4-shield.md`:
- Around line 25-29: The Parlio lane ordering in the shield reference is
inconsistent between the prose and table. Update the table’s terminal and GPIO
sequences to use the authoritative order 21,20,25,5,22,23,24,27, matching the
physical terminal labels and the existing device model configuration.
In `@src/core/Control.cpp`:
- Around line 326-327: Update the label parsing and matching logic in
Control.cpp around parseString so Select labels are never silently truncated to
the 23-character buffer limit. Use a length-aware parser/comparison, or
explicitly reject labels exceeding the buffer before matching, while preserving
strict-restore rejection and lenient-restore behavior.
- Around line 319-343: Prevent empty option lists from accepting or
manufacturing index 0: in src/core/Control.cpp lines 319-343, update the Select
handling around the c.max/hi calculation to reject c.max == 0 before label or
numeric parsing, returning the appropriate policy result; apply the same
empty-control guard to the Palette handling at src/core/Control.cpp lines
348-349. Preserve existing strict and lenient behavior for non-empty controls.
In `@src/light/drivers/MultiPinLedDriver.h`:
- Line 240: Update the comments at src/light/drivers/MultiPinLedDriver.h:240,
src/light/drivers/ParallelLedDriver.h:337, and
test/unit/light/unit_MoonLedDriver.cpp:33 to use American English spellings:
replace “behaviour” with “behavior,” “labelled” with “labeled,” and “honours”
with “honors.”
In `@web-installer/deviceModels.json`:
- Around line 1325-1341: Remove the newly added loopbackTest setting from the
“hpwit shift-register 15” device profile in the ParallelLed configuration, or
ensure it is disabled by default. Preserve the loopback pin configuration while
keeping normal LED output active for real hardware provisioned from this catalog
entry.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 863175f4-6d54-4c2a-9ad7-1c1167a832f1
📒 Files selected for processing (31)
CLAUDE.mddocs/MIGRATING.mddocs/architecture.mddocs/gettingstarted.mddocs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.mddocs/history/plans/Plan-20260723 - Consolidate parallel LED drivers into one module + peripheral strategy.mddocs/moonmodules/light/drivers.mddocs/reference/esp32-s31-coreboard.mddocs/reference/mhc-wled-esp32-p4-shield.mdmoondeck/_moondeck_config.pysrc/core/Control.cppsrc/core/SystemModule.hsrc/light/drivers/DriverBase.hsrc/light/drivers/LedPeripheral.hsrc/light/drivers/MoonLedDriver.hsrc/light/drivers/MultiPinLedDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/ParlioLedDriver.hsrc/main.cpptest/CMakeLists.txttest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_Control_apply_absent_key.cpptest/unit/light/unit_MoonLedDriver.cpptest/unit/light/unit_MultiPinLedDriver.cpptest/unit/light/unit_ParallelLedDriver_doublebuffer.cpptest/unit/light/unit_ParallelLedDriver_pinexpander.cpptest/unit/light/unit_ParallelLedDriver_ring.cpptest/unit/light/unit_ParallelLedDriver_swap.cpptest/unit/light/unit_ParlioLedDriver.cppweb-installer/deviceModels.jsonweb-installer/index.html
Renames the per-driver correction Select from `preset` to `lightPreset` for a clearer UI label, and folds in a batch of review findings on top of the parallel-driver consolidation: two hostile-input guards in the control-apply path, an S31 LCD_CAM correction across docs and code comments, a doubleBuffer backlog note, and assorted spelling/doc fixes. Hardware-verified: the renamed control on the S3, and the giant 48x256 wall driving on MoonI80. KPI: 16384lights | Desktop:755KB | tick:131/106/2/5/131/21/3/286/73/18/25/169/129/23/6/48us(FPS:7633/9433/500000/200000/7633/47619/333333/3496/13698/55555/40000/5917/7751/43478/166666/20833) | src:195(46132) | test:138(24891) | lizard:160w (ESP32 live tick omitted: the configured KPI port is stale; all 3 ESP32 variants built clean and the renamed control was hardware-verified on the S3 at 192.168.1.158.) Core: - Control.cpp: applyControlValue now rejects an empty option list (c.max == 0) before parsing for both Select and Palette, so a zero-option control can't manufacture or accept index 0 (Strict → OutOfRange, Lenient → no-op). - Control.cpp: a Select label longer than the parse buffer no longer prefix-matches a real option — the buffer widened to 64 and a value that fills it is treated as "no such option" (parseString truncates silently and has no truncation signal; the buffer-fill check avoids a core signature change). - platform_config.h: correct the LCD_CAM chip enumeration in the lane-gate comments from "(S3/P4)" to "(S3/P4/S31)" — the S31 has a real LCD_CAM, so the i80/MoonI80 backends run on it too. Light domain: - DriverBase: rename the correction `preset` Select to `lightPreset` (the addSelect name + the onControlChanged/isCorrectionControl/affectsPrepare matches + comments). UI-only rename: the durable reference persists under the separate hidden `presetRef` key, so no saved config breaks and no migration is needed. - MultiPinLedDriver / MoonLedDriver: same "(S3/P4)" → "(S3/P4/S31)" LCD_CAM comment correction; spelling behaviour→behavior, labelled→labeled (in ParallelLedDriver). Tests: - unit_LightPresetsModule: the 11 control-name lookups updated preset→lightPreset (the rename's incompleteness was caught here — the test failed until fixed). - unit_Control_apply_absent_key: added regression cases for the empty-Select and overlong-label guards. - unit_MoonLedDriver: spelling honours→honors. Docs / CI: - architecture.md: document the classic-ESP32 I2S-i80 parallel backend so the entry no longer implies classic lacks parallel output. - esp32-s31-coreboard.md: correct the wrong "S31 has no LCD_CAM" claim — the S31 has LCD_CAM and offers i80/MoonI80/Parlio, bench-verified (8x8 panel on i80/GPIO 60); spelling neighbours→neighbors. - mhc-wled-esp32-p4-shield.md: fix the Parlio lane table order to the authoritative 21,20,25,5,22,23,24,27. - drivers.md: the lightPreset rename in the control bullet + img alt. - backlog-light.md: new item — MoonI80 async double-buffer corrupts output on the ESP32-S31 (chip-specific, clean on S3/P4); PO decision to leave doubleBuffer default-on and backlog. - gettingstarted.md: the migration note becomes prose (MD028: no blockquote-blank-blockquote). - deviceModels.json: hpwit shift-register 15 → peripheral MoonI80, ledsPerPin 256, doubleBuffer true, loopbackTest false (drives on install), dcPin removed (i80-only) — matches the bench-proven config. Reviews: - 👾 stale "(S3/P4)" LCD_CAM comments contradicting the S31 correction: fixed (swept to S3/P4/S31 across platform_config.h + the two driver headers). - 👾 architecture.md redundant classic-parallel sentence: removed. - 👾 s31 doc "neighbours" US-spelling: fixed. - 👾 hpwit shift-register 48 stays on i80/96 while 15 moved to MoonI80/256: accepted/deferred per PO (the 48 entry is parked for a later bench pass; the 48x256 wall was verified working on MoonI80/256 this session). - 🐇 Control.cpp empty-list + overlong-label guards, spelling, shield table order, MD028, architecture classic-I2S mention: fixed. KPI Details: Desktop: 16,384 lights | 755 KB | 837 test cases pass | 21 scenarios pass | boundary PASS | specs 87/87 ESP32: image 1,493,812 bytes (64% partition free) Code: 195 src (46132 lines), 138 test (24891 lines), 146 specs, 21 scenarios, lizard 160w
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/moonmodules/light/drivers.md (2)
52-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse American English spelling.
Change “labelled indicator” to “labeled indicator.”
As per coding guidelines, “Use American English spelling in identifiers, JSON and wire keys, comments, documentation, and scripts.”
🤖 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 `@docs/moonmodules/light/drivers.md` at line 52, Update the documentation text in the peripheral description to use the American English spelling “labeled indicator” instead of “labelled indicator,” leaving the surrounding explanation unchanged.Source: Coding guidelines
140-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the obsolete separate-driver selection guidance.
The preceding table still directs users to MultiPin or Moon as separate drivers and says they are registered module types. They are now
peripheralselections ofParallelLedDriver; update that table and its follow-up paragraph to match this section and the catalog migration.🤖 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 `@docs/moonmodules/light/drivers.md` at line 140, Update the preceding driver-selection table and its follow-up paragraph to remove MultiPin and Moon as separate registered module types. Describe them as peripheral selections of ParallelLedDriver, consistent with the comparison here and the catalog migration, while preserving RMT as its own driver.src/light/drivers/MultiPinLedDriver.h (1)
155-169: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReject unset WR/DC pins before bus initialization.
clockPin/dcPinaccept-1, but this validation permits it. The bus path converts the value touint16_t, so-1becomes65535; the header documents that this can cause unsafe GPIO-ROM behavior on ESP32-P4 rather than a clean initialization failure.Proposed fix
const char* validateBusFatal() const override { + if (clockPin < 0 || dcPin < 0) + return "clockPin (WR) and dcPin must be valid GPIOs"; if (clockPin >= 0 && clockPin == dcPin) return "clockPin (WR) and dcPin are the same GPIO — they must differ";🤖 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 `@src/light/drivers/MultiPinLedDriver.h` around lines 155 - 169, Update validateBusFatal() to reject unset clockPin or dcPin values before any bus initialization or uint16_t conversion. Return a fatal validation message when either pin is below zero, while preserving the existing duplicate-pin checks for valid configured pins.
🤖 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 `@docs/architecture.md`:
- Line 472: The LED drivers entry currently links to an untracked
ParallelLedDriver document. Update the ParallelLedDriver link in the LED drivers
documentation to the canonical existing drivers.md#parallelled anchor,
preserving the surrounding description.
In `@docs/moonmodules/light/drivers.md`:
- Line 18: Update the lightPreset documentation to distinguish the runtime
presetId_ handle from the persisted presetRef value: state that presetRef is
stored by preset name, while the live ID mapping is resolved at runtime. Remove
claims that a stable ID reference survives reboot or preset renames, and
preserve the description of the preset’s per-light behavior.
In `@src/platform/esp32/platform_config.h`:
- Line 91: Update the adjacent LCD_CAM summary near the existing “ESP32-S3 / P4”
wording to also mention S31, keeping the documentation consistent with the
supported real LCD_CAM platforms described nearby.
In `@test/unit/core/unit_Control_apply_absent_key.cpp`:
- Around line 219-236: Add a unit test alongside the existing empty Select test
covering an empty Palette created with addPalette(..., 0). Verify Clamp accepts
the input without changing the sentinel value, including a representative
palette value, and Strict returns OutOfRange; ensure the test guards against an
underflow writing 255.
---
Outside diff comments:
In `@docs/moonmodules/light/drivers.md`:
- Line 52: Update the documentation text in the peripheral description to use
the American English spelling “labeled indicator” instead of “labelled
indicator,” leaving the surrounding explanation unchanged.
- Line 140: Update the preceding driver-selection table and its follow-up
paragraph to remove MultiPin and Moon as separate registered module types.
Describe them as peripheral selections of ParallelLedDriver, consistent with the
comparison here and the catalog migration, while preserving RMT as its own
driver.
In `@src/light/drivers/MultiPinLedDriver.h`:
- Around line 155-169: Update validateBusFatal() to reject unset clockPin or
dcPin values before any bus initialization or uint16_t conversion. Return a
fatal validation message when either pin is below zero, while preserving the
existing duplicate-pin checks for valid configured pins.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 466fa3d4-c24a-4c02-b083-5fc5942e245b
📒 Files selected for processing (16)
docs/architecture.mddocs/backlog/backlog-light.mddocs/gettingstarted.mddocs/moonmodules/light/drivers.mddocs/reference/esp32-s31-coreboard.mddocs/reference/mhc-wled-esp32-p4-shield.mdsrc/core/Control.cppsrc/light/drivers/DriverBase.hsrc/light/drivers/MoonLedDriver.hsrc/light/drivers/MultiPinLedDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/platform/esp32/platform_config.htest/unit/core/unit_Control_apply_absent_key.cpptest/unit/light/unit_LightPresetsModule.cpptest/unit/light/unit_MoonLedDriver.cppweb-installer/deviceModels.json
…s; review findings Fixes two robustness bugs found on the bench and improves the UI. A live module replace/delete (e.g. a layout) on a split-render device was a use-after-free crash (the encode worker walks the tree a mutation frees); and a reboot silently reverted every peripheral-backend control (clockPin, the MoonI80 ring geometry) to its default, because persistence restored those values onto the wrong bus backend. Both are core-level fixes with regression tests, hardware-verified. Plus: adding a module now selects+focuses it, the Tasks list holds still (projectMM tasks on top), and both Reviewer passes' findings landed. KPI: 16384lights | Desktop:755KB | tick:167/131/4/8/165/22/6/399/85/25/31/221/165/25/7/53us(FPS:5988/7633/250000/125000/6060/45454/166666/2506/11764/40000/32258/4524/6060/40000/142857/18867) | src:195(46253) | test:138(25203) | lizard:160w (ESP32 live tick omitted: stale KPI port; all 3 ESP32 variants built clean and both fixes were hardware-verified on the S3 testbench + the 48x256 giant wall.) Core: - MoonModule + Drivers + main.cpp: a structural tree mutation (add/remove/replace/move child) now stops the core-1 encode worker first via a quiesce-render hook (a function-pointer seam mirroring setSchemaChangedHook, wired once in main.cpp to Drivers::quiesceRenderSplit). The old per-parent quiesce() only stopped a worker the mutated node's PARENT owned, but the encode worker walks the WHOLE tree through the drivers, so mutating a sibling subtree (a layout) freed a node it was reading — a LoadProhibited crash. quiesceForMutation() lifts the rule into core for all four mutators. - FilesystemModule::applyNode: overlay saved values -> rebuildControls() -> overlay again, so a module whose control SET depends on a control VALUE (ParallelLedDriver's `peripheral`, which swaps the bus backend) restores its backend-owned controls. Without it, clockPin/dcPin and the whole MoonI80 ring cluster reverted to defaults on every reboot (the value landed on the default backend, then the swap discarded it). - HttpServerModule: applyAddModule reports the created module's final (post-disambiguation) name via an optional out-param; the response JSON-escapes it via writeJsonString (a client-supplied id isn't quote-safe). - TasksModule: sort the RTOS task snapshot so projectMM's own tasks (the render task + mm-prefixed workers) float to the top and system tasks sink below, stable within each group — the list stops jumping every second (the snapshot order is unstable). Uses platform::renderTaskName(), not a hardcoded name. - MultiPinLedDriver: validateBusFatal rejects an unset clockPin/dcPin (-1) before the int8_t->uint16_t cast that would slip 65535 past IDF's own >= 0 guard. - Control.cpp: reject an empty option list (c.max == 0) for Select and Palette before parsing; a Select label longer than the parse buffer no longer prefix-matches a real option. UI: - app.js: pressing "+" to add a module now selects the new module's tab, scrolls it into view, opens its collapsed controls, and focuses its first control (persisting the tab like a click); focus skips the status row. Docs: - drivers.md: reframe the decision table + paragraphs so MultiPin/Moon read as `peripheral` choices of the one Parallel LED driver, not separate module types; de-jargon the lightPreset bullet; drop a duplicate sentence; "labelled"->"labeled". - architecture.md: link ParallelLedDriver to the canonical drivers.md#parallelled anchor; document the classic-ESP32 I2S-i80 parallel backend. - core/supporting.md: drop `controls_` private-member leaks from the Control/MoonModule summaries. Catalog: - deviceModels.json: hpwit shift-register 48 -> peripheral MoonI80, ledsPerPin 256, dcPin removed (i80-only) — matches the bench-verified 48x256 giant-wall config, mirroring the earlier hpwit-15 fix. Tests: - unit_Drivers_rendersplit: two new cases (mutating a layout, reordering children) pinning the crash fix through the real worker thread (verified green->red); an RAII HookGuard makes the process-global hook failure-safe. - unit_FilesystemModule_persistence: a value-dependent-control-set roundtrip pinning the reload bug (verified green->red). - unit_HttpServerModule_apply: the created-name out-param, disambiguated. - unit_Control_apply_absent_key: empty-Palette + overlong-Select-label guards. - unit_MultiPinLedDriver: unset clockPin/dcPin fatal. - unit_TasksModule: the grouped sort order. Reviews: - 👾 crash fix's fourth mutator (moveChildTo) missed the quiesce: fixed + tested. - 👾 add-module response name not quote-safe: escaped via writeJsonString. - 👾 focusModule picked the status row: now queries the first focusable control. - 👾 TasksModule hardcoded "main": uses renderTaskName(). - 👾 drivers.md table/paragraph still called MultiPin/Moon module types; lightPreset mechanism claim; duplicate RMT sentence: fixed. - 👾 test hook cleanup not failure-safe: RAII guard. - 👾 second-Drivers hook boundary: recorded (no change, one-Drivers topology). - 🐇 empty-Select/Palette + overlong-label guards, spelling, architecture link, S31 LCD_CAM doc: fixed. KPI Details: Desktop: 16,384 lights | 755 KB | 837 test cases pass | 21 scenarios pass | boundary PASS | specs 87/87 ESP32: image 1,497,380 bytes (64% partition free) Code: 195 src (46253 lines), 138 test (25203 lines), lizard 160w
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/moonmodules/light/drivers.md (1)
35-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument S31 support consistently across parallel-driver overviews. The platform/backend wiring includes S31, but these public descriptions still limit i80 and MoonI80 to S3/P4.
docs/moonmodules/light/drivers.md#L35-L39: list S31 for the LCD_CAM-backedi80andMoonI80options.docs/moonmodules/light/drivers.md#L149-L154: change the i80 and MoonI80 chip columns to include S31.src/light/drivers/MultiPinLedDriver.h#L9-L19: add S31 to the LCD_CAM i80 backend overview.src/light/drivers/MoonLedDriver.h#L10-L20: add S31 to the MoonI80 LCD_CAM backend overview.🤖 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 `@docs/moonmodules/light/drivers.md` around lines 35 - 39, Document S31 consistently for the LCD_CAM-backed parallel LED drivers: update docs/moonmodules/light/drivers.md lines 35-39 and 149-154, src/light/drivers/MultiPinLedDriver.h lines 9-19, and src/light/drivers/MoonLedDriver.h lines 10-20 to include S31 for the i80 and MoonI80 backends, without changing other platform support descriptions.Source: Coding guidelines
src/main.cpp (1)
93-103: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove target-specific backend gating into
src/platform/.These new
CONFIG_SOC_*branches violate the platform-boundary rule. Keep capability macros and conditional backend-include glue insrc/platform/, then use a platform flag withif constexprhere for registration.Also applies to: 226-233
🤖 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 `@src/main.cpp` around lines 93 - 103, Move the CONFIG_SOC_* capability checks and conditional backend includes from the main registration flow into the appropriate src/platform glue. Expose a platform-level compile-time flag representing supported parallel-WS2812 backends, then update the registration logic around ParallelLedDriver to use if constexpr with that flag while preserving backend self-registration and peripheral options.Source: Coding guidelines
🤖 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 `@docs/reference/esp32-s31-coreboard.md`:
- Around line 125-126: Rewrite the hardware claims in the LED strip and loopback
documentation to use present-tense capability statements, replacing
“Bench-confirmed” and “Bench-verified” historical wording. Preserve the stated
RMT, Parallel LED, i80, and Parlio support details; move any necessary
validation provenance to the appropriate history documentation rather than
keeping it in this reference.
In `@src/core/Control.cpp`:
- Around line 334-346: Align Select label handling with the documented maximum:
update the Select parsing and overlong detection in Control.cpp so valid labels
at the supported limit are accepted and only truncated input is rejected, or
explicitly lower the documented limit. Add boundary tests in
unit_Control_apply_absent_key.cpp covering the exact supported length and one
byte beyond it, including the applicable Lenient and Strict policy behavior.
In `@src/core/MoonModule.h`:
- Line 526: Ensure failed mutations do not disable render splitting by moving
validation ahead of quiesceForMutation() in the affected MoonModule methods: at
src/core/MoonModule.h:526-526 locate the child and return if absent; at
src/core/MoonModule.h:543-543 validate i and fresh; and at
src/core/MoonModule.h:558-561 locate the child and reject an unchanged index
before quiescing. Add regression coverage confirming renderSplitActive_ remains
enabled for each failed mutation.
In `@src/core/TasksModule.h`:
- Around line 97-103: Update isProjectMMTask in TasksModule.h to remove its
direct dependency on platform::renderTaskName(). Introduce and use a
core-neutral task snapshot/classification provider at the core/platform
boundary, with its implementation supplied by src/platform, while preserving the
existing render-task and “mm” worker classification behavior.
In `@src/light/drivers/Drivers.h`:
- Around line 112-118: Replace historical crash language with present-tense
safety invariants: in src/light/drivers/Drivers.h lines 112-118, document that
structural tree mutations require quiescing the core-1 encode worker before
freeing nodes; in src/core/MoonModule.h lines 493-502, describe the worker’s
whole-tree access hazard without incident history; in
test/unit/light/unit_Drivers_rendersplit.cpp lines 237-243, describe the
regression condition in current behavioral terms. Move any retained incident
details to docs/history/.
In `@src/ui/app.js`:
- Around line 450-464: Update focusModule so the card-controls-collapse details
element is opened before calling scrollIntoView, then locate and focus the first
real control as currently implemented. Keep the existing selector and
preventScroll focus behavior unchanged.
In `@test/unit/core/unit_FilesystemModule_persistence.cpp`:
- Around line 477-484: Rewrite the regression comment above the persistence test
in present tense, replacing historical wording such as “found” and “reset” while
preserving its explanation of the value-dependent controls and the applyNode
overlay → rebuildControls → overlay-again behavior.
In `@test/unit/light/unit_Drivers_rendersplit.cpp`:
- Around line 262-278: Replace the sleep-based releaser synchronization in both
affected ranges of test/unit/light/unit_Drivers_rendersplit.cpp (anchor lines
262-278 and sibling lines 306-319) with a condition-variable handshake. Signal
when the hook or mutation begins, wait for hook entry or mutation completion
before releasing SlowDriver, then retain the quiescence assertions so the test
deterministically verifies the worker is out without relying on timing.
In `@web-installer/deviceModels.json`:
- Around line 1328-1336: Update the MoonI80 configuration’s ledsPerPin value to
represent 15 active 256-light strands and one unused strand, matching the
3,840-light 5×3 layout across the two expander pins.
---
Outside diff comments:
In `@docs/moonmodules/light/drivers.md`:
- Around line 35-39: Document S31 consistently for the LCD_CAM-backed parallel
LED drivers: update docs/moonmodules/light/drivers.md lines 35-39 and 149-154,
src/light/drivers/MultiPinLedDriver.h lines 9-19, and
src/light/drivers/MoonLedDriver.h lines 10-20 to include S31 for the i80 and
MoonI80 backends, without changing other platform support descriptions.
In `@src/main.cpp`:
- Around line 93-103: Move the CONFIG_SOC_* capability checks and conditional
backend includes from the main registration flow into the appropriate
src/platform glue. Expose a platform-level compile-time flag representing
supported parallel-WS2812 backends, then update the registration logic around
ParallelLedDriver to use if constexpr with that flag while preserving backend
self-registration and peripheral options.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 23f405bc-315f-4e62-b47e-afee8c170389
📒 Files selected for processing (32)
docs/architecture.mddocs/backlog/backlog-light.mddocs/gettingstarted.mddocs/moonmodules/core/supporting.mddocs/moonmodules/light/drivers.mddocs/reference/esp32-s31-coreboard.mddocs/reference/mhc-wled-esp32-p4-shield.mdsrc/core/Control.cppsrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MoonModule.hsrc/core/TasksModule.hsrc/light/drivers/DriverBase.hsrc/light/drivers/Drivers.hsrc/light/drivers/MoonLedDriver.hsrc/light/drivers/MultiPinLedDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/main.cppsrc/platform/esp32/platform_config.hsrc/ui/app.jstest/scenarios/light/scenario_GridBlacks_blackpixel.jsontest/unit/core/unit_Control_apply_absent_key.cpptest/unit/core/unit_FilesystemModule_persistence.cpptest/unit/core/unit_HttpServerModule_apply.cpptest/unit/core/unit_TasksModule.cpptest/unit/light/unit_Drivers_rendersplit.cpptest/unit/light/unit_LightPresetsModule.cpptest/unit/light/unit_MoonLedDriver.cpptest/unit/light/unit_MultiPinLedDriver.cppweb-installer/deviceModels.json
Comment on lines
+125
to
+126
| - **LED strip data:** the onboard WS2812 is on **GPIO 60** (the catalog default). For an *external* strand, use **GPIO 42** as the single-lane pick; a parallel rig (RMT/Parlio) takes the free block (**36–49**, skipping 41 which isn't broken out) for several lanes. **GPIO 4** (col 16 top) also works as an LED data pin and sits one column from the `G` / `3V3` / `5V` power rail (cols 17–20), so a single strip's data + ground + 5 V wires land close together — handy for a tidy 3-wire pigtail. It's a plain I/O with no strap or peripheral tie on this board (the SD lines beside it, D0–D3 / CLK / CMD, are broken out by function name, not GPIO number, so GPIO 4 is *not* one of them; it just neighbors that cluster on the header). The only reason it reads as "distinct" from the rest of the free run is its header position — it's over by the SD/power group rather than in the low-block on cols 8–12. | ||
| - **Loopback self-test:** **Tx = GPIO 48, Rx = GPIO 47** — the two pins of **column 7** (48 top, 47 bottom), so a single jumper cap shorts them. A driver transmits a known WS2812 frame out Tx and reads it back on Rx to verify output on real silicon (same pattern as the P4-NANO bench's 32↔33). They sit at the top of the free run, clear of the operational LED pins so the strip wiring and the jumper don't interfere. **Bench-confirmed on the S31 for [RMT](../moonmodules/light/drivers.md#rmtled) and the [Parallel LED driver](../moonmodules/light/drivers.md#parallelled) across all three of its peripherals.** The S31 SOC has a real LCD_CAM (`SOC_LCDCAM_I80_LCD_SUPPORTED`), so its `i80` backend is LCD_CAM-driven (not the classic ESP32's I2S-in-i80-mode, which the platform layer excludes on any chip with real LCD_CAM) and can draw its frame from PSRAM. The `peripheral` selector therefore offers all three: **`i80`, `MoonI80`, and `Parlio`** — the RGMII Ethernet does not take the LCD_CAM block. Bench-verified: an 8x8 panel driving on `i80` (data GPIO 60), and Parlio on the free-block pins. Testing several drivers in a row, they all default loopback to GPIO 48, so only one can hold the pin at a time — toggle each driver's `loopbackTest` off before testing the next. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the hardware reference in present tense.
“Bench-confirmed” and “Bench-verified” are historical validation records. Move their provenance to docs/history/ or state the current capability directly (for example, “RMT and all Parallel LED peripherals work on this loopback”). As per coding guidelines, “Use present tense only … Put … historical content in docs/history/.”
🤖 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 `@docs/reference/esp32-s31-coreboard.md` around lines 125 - 126, Rewrite the
hardware claims in the LED strip and loopback documentation to use present-tense
capability statements, replacing “Bench-confirmed” and “Bench-verified”
historical wording. Preserve the stated RMT, Parallel LED, i80, and Parlio
support details; move any necessary validation provenance to the appropriate
history documentation rather than keeping it in this reference.
Source: Coding guidelines
Comment on lines
+334
to
+346
| char label[64] = {}; | ||
| mm::json::parseString(json, key, label, sizeof(label)); | ||
| const bool overlong = std::strlen(label) >= sizeof(label) - 1; | ||
| if (label[0]) { | ||
| auto* options = reinterpret_cast<const char* const*>(c.aux); | ||
| if (options && !overlong) | ||
| for (int i = 0; i <= hi; i++) | ||
| if (options[i] && std::strcmp(options[i], label) == 0) | ||
| return clampInto(static_cast<uint8_t*>(c.ptr), i, 0, hi); | ||
| // A label that names no current option (a peripheral this board can't run, or one too long | ||
| // to be any real option) is not an error in Lenient policy — the driver keeps its default; | ||
| // Strict rejects it. | ||
| if (policy == ApplyPolicy::Strict) return ApplyResult::OutOfRange; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B3 '\baddSelect\s*\(' src test
rg -n -A8 -B3 'char label\[64\]|overlong|parseString\(json, key, label' src/core/Control.cppRepository: MoonModules/projectMM
Length of output: 42261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Control.h outline/ControlDescriptor relevant sections ---\n'
wc -l src/core/Control.h src/core/Control.cpp
sed -n '1,440p' src/core/Control.h | cat -n
sed -n '1,80p' src/core/Control.cpp | cat -n
sed -n '310,360p' src/core/Control.cpp | cat -n
printf '\n--- Candidate tests around Select overlong/missing ---\n'
sed -n '200,280p' test/unit/core/unit_Control_apply_absent_key.cpp | cat -n
printf '\n--- locate json implementation ---\n'
rg -n 'namespace json|parseString' src testRepository: MoonModules/projectMM
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Control.h outline/ControlDescriptor relevant sections ---'
wc -l src/core/Control.h src/core/Control.cpp
sed -n '1,440p' src/core/Control.h | cat -n
sed -n '1,80p' src/core/Control.cpp | cat -n
sed -n '310,360p' src/core/Control.cpp | cat -n
printf '%s\n' ''
printf '%s\n' '--- Candidate tests around Select overlong/missing ---'
sed -n '200,280p' test/unit/core/unit_Control_apply_absent_key.cpp | cat -n
printf '%s\n' ''
printf '%s\n' '--- locate json implementation ---'
rg -n 'namespace json|parseString' src testRepository: MoonModules/projectMM
Length of output: 48759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A120 'inline void parseString' src/core/JsonUtil.h
rg -n -A40 'Select labels|option label|length limit|64|Select' docs src test | sed -n '1,220p'Repository: MoonModules/projectMM
Length of output: 33284
Document and enforce the Select label cap consistently. parseString only guarantees maxLen - 1 bytes of copied content plus NUL, so Select’s 64-byte buffer already rejects any valid option label of 60+ bytes here; the parser also doesn’t signal “was truncated” separately from a legitimate 63-byte label. Either lower the supported option-label length and add guard tests at the real limit plus one, or make the buffer/policy explicitly support the documented maximum and reject only truncated input.
🧰 Tools
🪛 Clang (14.0.6)
[note] 337-337: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 339-339: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 339-339: +1
(clang)
[note] 341-341: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 341-341: +1
(clang)
[note] 346-346: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[warning] 334-334: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 337-337: implicit conversion 'char' -> bool
(readability-implicit-bool-conversion)
[warning] 338-338: 'auto *options' can be declared as 'const auto *options'
(readability-qualified-auto)
[warning] 339-339: implicit conversion 'const char *const *' -> bool
(readability-implicit-bool-conversion)
[warning] 339-339: statement should be inside braces
(readability-braces-around-statements)
[warning] 340-340: statement should be inside braces
(readability-braces-around-statements)
[warning] 341-341: implicit conversion 'const char *' -> bool
(readability-implicit-bool-conversion)
[warning] 341-341: statement should be inside braces
(readability-braces-around-statements)
[warning] 346-346: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.21.0)
[style] 339-339: The function 'addPin' is never used.
(unusedFunction)
[style] 344-344: The function 'addBool' is never used.
(unusedFunction)
📍 Affects 2 files
src/core/Control.cpp#L334-L346(this comment)test/unit/core/unit_Control_apply_absent_key.cpp#L257-L274
🤖 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 `@src/core/Control.cpp` around lines 334 - 346, Align Select label handling
with the documented maximum: update the Select parsing and overlong detection in
Control.cpp so valid labels at the supported limit are accepted and only
truncated input is rejected, or explicitly lower the documented limit. Add
boundary tests in unit_Control_apply_absent_key.cpp covering the exact supported
length and one byte beyond it, including the applicable Lenient and Strict
policy behavior.
Comment on lines
+97
to
+103
| // A projectMM-created task: the render task (whatever platform::renderTaskName() reports — the | ||
| // same seam writeListRowDetail uses to nest the modules, NOT a hardcoded "main") or a worker we | ||
| // spawn (our convention names them with an "mm" prefix — mmEncode, mmSnap). Everything else is an | ||
| // RTOS/system task. | ||
| static bool isProjectMMTask(const char* name) { | ||
| const char* render = platform::renderTaskName(); | ||
| return (render[0] && std::strcmp(name, render) == 0) || std::strncmp(name, "mm", 2) == 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep task classification behind the core/platform boundary.
isProjectMMTask() now directly depends on platform::renderTaskName() from src/core. Inject a core-neutral task snapshot/classification provider, implemented by src/platform, instead of extending this dependency.
As per path instructions, “Core infrastructure … must be platform-independent,” and platform-specific dependencies belong in src/platform/. Based on learnings, runtime platform handling must remain behind a core/platform boundary.
🤖 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 `@src/core/TasksModule.h` around lines 97 - 103, Update isProjectMMTask in
TasksModule.h to remove its direct dependency on platform::renderTaskName().
Introduce and use a core-neutral task snapshot/classification provider at the
core/platform boundary, with its implementation supplied by src/platform, while
preserving the existing render-task and “mm” worker classification behavior.
Sources: Path instructions, Learnings
Comment on lines
+112
to
+118
| /// Stop the core-1 encode worker so a STRUCTURAL TREE MUTATION (a module replace / delete / add) can | ||
| /// free tree nodes without the worker dereferencing them mid-tick. The worker ticks the driver | ||
| /// children, and a driver walks the whole Layouts/Layer tree (PreviewDriver::sendFrame → | ||
| /// Layouts::forEachCoord), so freeing ANY layout/layer/driver node while core 1 runs is a | ||
| /// use-after-free — a LoadProhibited crash seen replacing a layout on a running split device. The | ||
| /// mutation path (HttpServerModule) calls this before the free; the trailing prepareTree() re-engages | ||
| /// the split. Idempotent + safe when the split is off (stopEncodeTask guards on the task handle). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move incident history out of source comments.
These comments describe a previously observed LoadProhibited crash. State the current safety invariant here and move the incident record to docs/history/ if it needs preservation.
src/light/drivers/Drivers.h#L112-L118: describe the present quiesce requirement without “crash seen.”src/core/MoonModule.h#L493-L502: describe the current whole-tree worker hazard without historical crash language.test/unit/light/unit_Drivers_rendersplit.cpp#L237-L243: describe the regression condition without incident history.
As per coding guidelines, C++ comments must use present tense and historical content belongs in docs/history/.
📍 Affects 3 files
src/light/drivers/Drivers.h#L112-L118(this comment)src/core/MoonModule.h#L493-L502test/unit/light/unit_Drivers_rendersplit.cpp#L237-L243
🤖 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 `@src/light/drivers/Drivers.h` around lines 112 - 118, Replace historical crash
language with present-tense safety invariants: in src/light/drivers/Drivers.h
lines 112-118, document that structural tree mutations require quiescing the
core-1 encode worker before freeing nodes; in src/core/MoonModule.h lines
493-502, describe the worker’s whole-tree access hazard without incident
history; in test/unit/light/unit_Drivers_rendersplit.cpp lines 237-243, describe
the regression condition in current behavioral terms. Move any retained incident
details to docs/history/.
Source: Coding guidelines
Comment on lines
+262
to
+278
| std::atomic<bool> releaseFired{false}; | ||
| std::thread releaser([&] { | ||
| std::this_thread::sleep_for(30ms); | ||
| releaseFired.store(true); | ||
| slow->letGo(); | ||
| }); | ||
|
|
||
| // Mutate a node OUTSIDE the Drivers subtree: remove the grid from Layouts. quiesceForMutation() | ||
| // must reach the render worker through the hook and block until it is out — WITHOUT the hook this | ||
| // returns immediately and a later free of `grid` would be a use-after-free (ASan) while the worker | ||
| // is still walking it. | ||
| r.layouts.removeChild(&r.grid); | ||
|
|
||
| CHECK(releaseFired.load()); // it waited for the encode (fails without the hook) | ||
| CHECK_FALSE(slow->isInTick()); // the worker is provably out | ||
|
|
||
| releaser.join(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace sleep-based mutation ordering with a condition-variable handshake.
A delayed scheduler can run the releaser before these assertions, letting an unquiesced mutation pass. Signal when the hook/mutation starts, wait for either hook entry or mutation completion, then release SlowDriver; this makes the old behavior fail deterministically. As per coding guidelines, “Tests should not depend on timing or network.”
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 275-275: The function 'audioFrame' is never used.
(unusedFunction)
[style] 270-270: The function 'setSchemaChangedHook' is never used.
(unusedFunction)
[style] 275-275: The function 'nearestForRgb' is never used.
(unusedFunction)
📍 Affects 1 file
test/unit/light/unit_Drivers_rendersplit.cpp#L262-L278(this comment)test/unit/light/unit_Drivers_rendersplit.cpp#L306-L319
🤖 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 `@test/unit/light/unit_Drivers_rendersplit.cpp` around lines 262 - 278, Replace
the sleep-based releaser synchronization in both affected ranges of
test/unit/light/unit_Drivers_rendersplit.cpp (anchor lines 262-278 and sibling
lines 306-319) with a condition-variable handshake. Signal when the hook or
mutation begins, wait for hook entry or mutation completion before releasing
SlowDriver, then retain the quiescence assertions so the test deterministically
verifies the worker is out without relying on timing.
Source: Coding guidelines
Comment on lines
+1328
to
+1336
| "peripheral": "MoonI80", | ||
| "pins": "9,10", | ||
| "pinExpander": true, | ||
| "latchPin": 46, | ||
| "clockPin": 3, | ||
| "ledsPerPin": "96", | ||
| "dcPin": 21, | ||
| "doubleBuffer": false | ||
| "ledsPerPin": "256", | ||
| "doubleBuffer": true, | ||
| "loopbackTest": false, | ||
| "loopbackStrand": 8, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match configured strand capacity to the 3,840-light layout.
Two expander pins create 16 strands, so ledsPerPin: "256" configures 4,096 lights while the 5×3 panel layout contains 3,840. Configure the unused sixteenth strand as zero-length.
Proposed fix
- "ledsPerPin": "256",
+ "ledsPerPin": "256,256,256,256,256,256,256,256,256,256,256,256,256,256,256,0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "peripheral": "MoonI80", | |
| "pins": "9,10", | |
| "pinExpander": true, | |
| "latchPin": 46, | |
| "clockPin": 3, | |
| "ledsPerPin": "96", | |
| "dcPin": 21, | |
| "doubleBuffer": false | |
| "ledsPerPin": "256", | |
| "doubleBuffer": true, | |
| "loopbackTest": false, | |
| "loopbackStrand": 8, | |
| "peripheral": "MoonI80", | |
| "pins": "9,10", | |
| "pinExpander": true, | |
| "latchPin": 46, | |
| "clockPin": 3, | |
| "ledsPerPin": "256,256,256,256,256,256,256,256,256,256,256,256,256,256,256,0", | |
| "doubleBuffer": true, | |
| "loopbackTest": false, | |
| "loopbackStrand": 8, |
🤖 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 `@web-installer/deviceModels.json` around lines 1328 - 1336, Update the MoonI80
configuration’s ledsPerPin value to represent 15 active 256-light strands and
one unused strand, matching the 3,840-light 5×3 layout across the two expander
pins.
…pass
Processes the CodeRabbit batch and a fresh persistence-robustness review before the parallel-driver-consolidation branch merges, plus a user-reported audio status bug. The headline fixes are three persistence robustness holes in the light-preset library (a preset name with a quote silently wiped all custom presets on reboot; a low-memory restore crashed on a null pointer; corrupt role bytes survived unclamped) and an audio-status leak where a Local-mode mic message lingered on the card after switching to receive-network mode. All fixes are pinned by green-to-red regression tests.
KPI: 16384lights | Desktop:755KB | tick:131/106/2/5/131/21/3/287/73/18/23/169/129/23/7/47us(FPS:7633/9433/500000/200000/7633/47619/333333/3484/13698/55555/43478/5917/7751/43478/142857/21276) | ESP32:1498KB | src:195(46309) | test:138(25531) | lizard:160w
Core
- LightPresetsModule: writeListRow/writeListRowDetail now emit the preset name via writeJsonString (escapes " / \ / control bytes) instead of a raw %s — a name with a quote produced malformed JSON that failed to parse on the next boot, silently wiping every custom preset. DevicesModule already wrote its name this way; this closes the asymmetry.
- LightPresetsModule::restoreList: bail to an empty list (built-ins re-seed) when the role-pool alloc fails, instead of writing roles through a null base pointer — a low-memory / fragmented-heap boot on ESP32 hard-faulted rather than degrading.
- LightPresetsModule::restoreList: clamp each persisted role byte to the valid ChannelRole range, matching setListRowField's live-edit validation — a hand-edited/corrupt file could store an out-of-range role the UI mis-rendered and the Correction silently dropped.
- AudioService: clear the module status when leaving Local mode (prepare's non-Local branch) — a Local-mode mic diagnostic ("mic: set sckPin / wsPin / sdPin") lingered on the status row in receive-network / simulate mode, which report through the separate sync-status row and have no mic to diagnose.
- MoonModule: removeChild / moveChildTo now resolve the target and reject a no-op (child not found, or an unchanged index) BEFORE quiescing the render worker, so a failed or idempotent mutation can't needlessly disengage the render split.
- FilesystemModule::applyNode: rewrote the child-reconcile comments to match the current pos/i-decoupled loop (removed three references to an "align pass" that no longer exists — a stale comment describing removed code) and dropped past-incident narration for present-tense mechanism.
- Control.cpp: no code change; documented the Select overlong-label boundary via new tests.
Light domain
- Drivers.h: present-tense rewrite of the quiesceRenderSplit rationale (a LoadProhibited fault, not a past "crash seen").
- drivers.md / MultiPinLedDriver.h / MoonLedDriver.h: the i80 and MoonI80 peripheral descriptions now include the ESP32-S31 (a real LCD_CAM chip) alongside the S3/P4, consistent across the catalog page, the two table rows, and the header summaries.
UI
- app.js focusModule: open the card's collapsed details BEFORE scrollIntoView, so the scroll targets the expanded height rather than the collapsed geometry.
Tests
- unit_LightPresetsModule: a preset name with a quote/backslash survives persistence (was a silent full wipe); an out-of-range persisted role clamps to a default on restore. Both green-to-red verified.
- unit_AudioService_sync: switching out of Local mode clears the mic status. Green-to-red verified.
- unit_Control_apply_absent_key: the Select overlong-label guard boundary is exactly the 64-byte parse buffer (62 matches, 63 is fenced off).
Docs / CI
- ADR-0016: the one-ParallelLedDriver runtime-peripheral-strategy decision (added to the ADR index).
- MIGRATING / performance / lessons: the consolidation's migration entries, driver-name-to-peripheral perf mapping, and the four consolidation-branch debugging lessons.
Reviews
- 🐇 R2 Control.cpp label limit: accepted the buffer size (64), added the boundary tests the finding asked for; a documented core constant for one call site would be core bloat.
- 🐇 R3 MoonModule failed-mutation quiesce ordering: fixed (validate before quiescing).
- 🐇 R5 Drivers.h present-tense: fixed.
- 🐇 R6 app.js details-before-scroll: fixed.
- 🐇 R7 persistence test comment present-tense: fixed.
- 🐇 R-drivers.md S31 consistency: fixed across the page + headers.
- 🐇 R4 TasksModule task-classification provider: skipped — platform::renderTaskName() is already the platform-boundary seam; a new provider is indirection for one caller.
- 🐇 R-main.cpp move CONFIG_SOC gates to platform glue: skipped — main.cpp is the composition root; moving light-driver headers behind a platform seam would make platform depend on the light domain (a worse boundary violation).
- 🐇 R-deviceModels hpwit-15 ledsPerPin: skipped — misread; "256" broadcast over 16 strands with a 3840-light window already computes [256x15, 0], the 15-active-strand rig the name describes.
- 👾 persistence-robustness review (6-axis fault-injection, 14 raw / 5 refuted): fixed the 3 confirmed light-preset holes above; skipped the key-length write/read asymmetry (latent, unreachable at current names/depth), the Select index-vs-label transplant case (out of the robustness contract's input scope), and the built-in-lost-on-upgrade / deviceModel-validator-skew cases (hardening opportunities, not present-code drops).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/moonmodules/light/drivers.md (2)
18-18: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not promise preset references survive preset renames.
The persistence contract stores the durable reference by preset name, so renaming the preset can invalidate it. Limit this statement to supported control renames/reorders, or distinguish the runtime ID from the persisted preset name.
🤖 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 `@docs/moonmodules/light/drivers.md` at line 18, Update the lightPreset documentation to remove the claim that references survive preset renames, since persistence uses the preset name. Limit the durability statement to supported control renames or reordering, or explicitly distinguish runtime IDs from persisted preset names.
51-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument
pinExpanderas MoonI80-specific.The text calls
pinExpanderinvariant across peripherals, but Parlio and classic i80 cannot host it and unsupported controls are hidden or cleared. Describe it under MoonI80-specific controls so users do not expect it for unsupported selections.🤖 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 `@docs/moonmodules/light/drivers.md` around lines 51 - 53, Update the controls description in the peripheral configuration documentation so pinExpander is listed as MoonI80-specific rather than invariant across peripherals. Keep doubleBuffer documented as invariant, and ensure the MoonI80-specific section reflects pinExpander alongside its existing controls.test/unit/light/unit_ParallelLedDriver_pinexpander.cpp (3)
44-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse American English and present-tense comments.
Change
recognisabletorecognizable, and replace the dated2026-07-14 bugnarrative with the current enqueue/completion contract. Historical incident details belong in the designated history documentation, not this test comment.As per coding guidelines, C++ comments use American English and present tense, and historical content belongs in designated history locations.
Also applies to: 53-55
🤖 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 `@test/unit/light/unit_ParallelLedDriver_pinexpander.cpp` at line 44, Update the comments around clockPinForBus() and the related lines to use American English and present tense: change “recognisable” to “recognizable” and replace the historical 2026-07-14 bug narrative with a concise description of the current enqueue/completion contract. Remove incident-history details from these test comments.Source: Coding guidelines
374-407: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert that the in-flight buffer remains unchanged.
transmitCount()detects an extrabusTransmit, but the test still passes if the driver re-encodes over the buffer while the first transfer remains stuck. Snapshot the transmitted bytes after the initial send and compare them after the timeout ticks.As per path instructions, tests must cover edge cases and match the specification; this regression must verify buffer immutability, not only call counts.
🤖 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 `@test/unit/light/unit_ParallelLedDriver_pinexpander.cpp` around lines 374 - 407, The timed-out transfer test must verify buffer immutability, not only transmit count. In the “shift register: a timed-out transfer never gets its buffer re-encoded” test, snapshot the bytes sent during the first transfer after the initial d.tick(), run the subsequent timeout ticks, and compare the transmitted bytes with that snapshot while preserving the existing transmitCount assertion.Source: Path instructions
46-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor the requested second DMA buffer or make single-buffer mode explicit.
wire()leavesdriver.doubleBufferat the defaulttrue, andParallelLedDriver::prepare()callsperipheral_->busInit(frameBytes_, doubleBuffer).MockPeripheralignores that request andbusBuffer(1)is alwaysnullptr, so every pin-expander setup exercises the single-buffer fallback. Implement the requested second buffer, passcanSecond=falsefrom the helper, or resetdriver.doubleBuffer = falseinwire()and cover double buffering with the existing host mock.🤖 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 `@test/unit/light/unit_ParallelLedDriver_pinexpander.cpp` around lines 46 - 52, Update MockPeripheral::busInit and busBuffer to honor the requested double-buffer mode by allocating and returning a second DMA buffer when canSecond is true, while preserving single-buffer behavior when false. Alternatively, make wire() explicitly set driver.doubleBuffer = false and add coverage for double-buffer mode using the host mock; ensure the test does not silently exercise only the fallback path.
🤖 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 `@docs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.md`:
- Line 39: The ADR and lessons log use inconsistent issue counts. In
docs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.md at lines
39-39, revise the “two robustness bugs” wording to explicitly explain how the
fixes group into four lessons, or use the same count as the lessons log; in
docs/history/lessons.md at lines 108-110, update the section introduction to
match the four lessons listed below.
In `@docs/MIGRATING.md`:
- Line 33: Update the migration instructions to make restored controls depend on
the selected peripheral: document that Parlio uses peripheral=Parlio and has no
clock/DC pins, so users should restore only supported pins, lengths, and clock
settings. Correct the description of ParlioLedDriver to state that it was merged
into ParallelLedDriver rather than remaining unchanged.
In `@docs/moonmodules/light/drivers.md`:
- Line 152: The Parallel LED row’s classic i80 DMA limit is incorrectly
presented as a fixed 2048 lights; update the description to make the ceiling
depend on bus width, identifying 2048 lights as the 8-lane result and reflecting
the higher limit verified for 16 lanes. Preserve the existing LCD_CAM limit and
idle-on-overflow behavior.
In `@src/core/FilesystemModule.cpp`:
- Around line 203-238: In src/core/FilesystemModule.cpp#L203-L238, update
applyNode’s stale code-wired child handling to search remaining JSON entries for
one matching live->typeName(), apply that entry’s childPrefix to the wired child
before advancing pos, then continue preserving the existing reconciliation
behavior. In test/unit/core/unit_FilesystemModule_persistence.cpp#L561-L625,
extend the reordered-siblings test with a distinctive persisted control value on
a wired effect and assert that value survives reload.
In `@test/unit/light/unit_ParallelLedDriver_swap.cpp`:
- Around line 198-241: Update the swap-ordering test to use a mock backend whose
destructor records whether the quiesce hook has already fired, wiring that state
into g_hookFiredBeforeLastDtor. Replace the g_hookFires-only assertion with a
CHECK on g_hookFiredBeforeLastDtor, and ensure the mock is actually installed
and destroyed during changePeripheralTo so the test verifies quiescing occurs
before the outgoing backend is freed.
---
Outside diff comments:
In `@docs/moonmodules/light/drivers.md`:
- Line 18: Update the lightPreset documentation to remove the claim that
references survive preset renames, since persistence uses the preset name. Limit
the durability statement to supported control renames or reordering, or
explicitly distinguish runtime IDs from persisted preset names.
- Around line 51-53: Update the controls description in the peripheral
configuration documentation so pinExpander is listed as MoonI80-specific rather
than invariant across peripherals. Keep doubleBuffer documented as invariant,
and ensure the MoonI80-specific section reflects pinExpander alongside its
existing controls.
In `@test/unit/light/unit_ParallelLedDriver_pinexpander.cpp`:
- Line 44: Update the comments around clockPinForBus() and the related lines to
use American English and present tense: change “recognisable” to “recognizable”
and replace the historical 2026-07-14 bug narrative with a concise description
of the current enqueue/completion contract. Remove incident-history details from
these test comments.
- Around line 374-407: The timed-out transfer test must verify buffer
immutability, not only transmit count. In the “shift register: a timed-out
transfer never gets its buffer re-encoded” test, snapshot the bytes sent during
the first transfer after the initial d.tick(), run the subsequent timeout ticks,
and compare the transmitted bytes with that snapshot while preserving the
existing transmitCount assertion.
- Around line 46-52: Update MockPeripheral::busInit and busBuffer to honor the
requested double-buffer mode by allocating and returning a second DMA buffer
when canSecond is true, while preserving single-buffer behavior when false.
Alternatively, make wire() explicitly set driver.doubleBuffer = false and add
coverage for double-buffer mode using the host mock; ensure the test does not
silently exercise only the fallback path.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 3a3146a3-d7c2-4d1e-b59e-c09fe4f16576
📒 Files selected for processing (21)
docs/MIGRATING.mddocs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.mddocs/adr/README.mddocs/history/lessons.mddocs/moonmodules/light/drivers.mddocs/performance.mdsrc/core/AudioService.hsrc/core/FilesystemModule.cppsrc/core/MoonModule.hsrc/light/drivers/Drivers.hsrc/light/drivers/LightPresetsModule.hsrc/light/drivers/MoonLedDriver.hsrc/light/drivers/MultiPinLedDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/ui/app.jstest/unit/core/unit_AudioService_sync.cpptest/unit/core/unit_Control_apply_absent_key.cpptest/unit/core/unit_FilesystemModule_persistence.cpptest/unit/light/unit_LightPresetsModule.cpptest/unit/light/unit_ParallelLedDriver_pinexpander.cpptest/unit/light/unit_ParallelLedDriver_swap.cpp
| | Output | `peripheral` | Chip | Strands | Extra controls | Notes | | ||
| |---|---|---|---|---|---| | ||
| | **RMT** ([detail](moxygen/RmtLedDriver.md)) | *(own driver)* | any ESP32 (classic 8 ch, S3 4, P4 4 DMA) | one per RMT TX channel | `loopbackFrame` | The general single-/few-strand output; default for classic + S3 board entries. `loopbackFrame` bit-verifies a *whole frame*, catching frame-rate / RF corruption a 24-bit burst misses. | | ||
| | Parallel LED | **`i80`** | S3 / P4 / S31 (LCD_CAM) · classic (I2S) | **1–16** | `clockPin` `dcPin` | Over IDF's `esp_lcd` i80 bus. The **bus** is 8 or 16 bits wide (≤8 pins → 8-bit, 9–16 → 16-bit) — but the **pin count is free**: configure only the pins that drive something and the driver rounds the bus up around them, parking the spare lanes on a pin the peripheral already drives. `clockPin`/`dcPin` are i80 bus lines the LEDs ignore. **Capped by one contiguous DMA buffer**: the classic backend is internal-RAM only (I2S can't reach PSRAM) → **2048 lights**; LCD_CAM draws from PSRAM → **16384**. Over the cap it idles with a status rather than crashing. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the classic i80 DMA limit by lane count.
This row states a fixed 2048-light ceiling, while the performance record verifies 4096 lights at 16 lanes. Describe the limit as width-dependent; 2048 is the 8-lane result.
🤖 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 `@docs/moonmodules/light/drivers.md` at line 152, The Parallel LED row’s
classic i80 DMA limit is incorrectly presented as a fixed 2048 lights; update
the description to make the ceiling depend on bus width, identifying 2048 lights
as the 8-lane result and reflecting the higher limit verified for 16 lanes.
Preserve the existing LCD_CAM limit and idle-on-overflow behavior.
…guard Fixes a hard freeze where switching the ParallelLedDriver to the MoonI80 peripheral with doubleBuffer on wedged the bus at ~200 ms per frame after a couple of frames (a completion-signal race in MoonI80's own-GDMA whole-frame two-buffer handshake). MoonI80 now runs single-buffer; its speed comes from the streaming ring, not from double-buffering a whole frame. Also hardens the live-scenario runner so a scenario restores the board's config when done, adds a peripheral-switch scenario that guards the fix, and processes a second review batch plus a persistence-reconcile improvement. KPI: 16384lights | Desktop:755KB | tick:181/144/4/9/180/25/8/431/94/27/34/246/188/30/9/62/8us(FPS:5524/6944/250000/111111/5555/40000/125000/2320/10638/37037/29411/4065/5319/33333/111111/16129/125000) | ESP32(classic):2450us(FPS:408) | heap:118KB | src:195(46364) | test:138(25640) | lizard:160w Light domain - LedPeripheral / MoonLedDriver / ParallelLedDriver: added supportsDoubleBuffer() (default true; MoonI80 false). The driver gates the second-buffer request on it, so MoonI80 always runs single-buffer regardless of the saved doubleBuffer value — its own-GDMA two-buffer completion handshake races and wedges the bus at the 200 ms backstop. i80/Parlio keep double-buffer (esp_lcd / the Parlio driver own a real transaction queue). doubleBuffer is now a peripheral-specific control (hidden on MoonI80), moved below the peripheral divider, with its visibility computed after the peripheral swap (fixes a stale-visibility bug across an i80→MoonI80→i80 switch). Core - FilesystemModule::applyNode: a reordered code-wired child now restores its own persisted controls (applyWiredChildFromJson finds the JSON entry matching the wired child's type and overlays it), so a code-wired child whose saved index differs from its boot index keeps its persisted state across a reboot instead of reverting to defaults. Scripts / MoonDeck - run_live_scenario.py: snapshot the board's user-added tree before each scenario and restore anything a clear_children destroyed afterward, so a live scenario leaves the bench board exactly as found (no residue, no lost driver config). An optional set_control the device rejects with 400 (a peripheral value the chip doesn't offer, e.g. Parlio on an S3) now skips instead of failing. Tests - scenario_peripheral_switch (new): cycles a live ParallelLedDriver through i80 / MoonI80 / Parlio, toggling doubleBuffer and pinExpander, measuring each. The MoonI80-with-doubleBuffer-on measure is the freeze regression guard (a ~200 ms tick fails it). Verified live on S3, P4 (all three peripherals incl. Parlio), and the host runner; auto-discovered by the C++ scenario gate. - scenario_Audio_mutation: fixed the fixture (old pinned-single-layer shape → standard Layers container) so it runs live, not only on the host runner. - Deleted scenario_DevicesModule_scan: obsolete — it tested a `scan` button + subnet sweep ADR-0006 removed (discovery is advertise-only now; there is no scan control). - unit_ParallelLedDriver_doublebuffer: a peripheral reporting supportsDoubleBuffer()==false stays single-buffer with doubleBuffer on (green→red verified). - unit_ParallelLedDriver_swap: the swap-quiesce test now asserts the render-worker hook fired BEFORE the outgoing backend was freed (ordering, not just that it fired). - unit_ParallelLedDriver_pinexpander: the timed-out-transfer test now asserts the buffer bytes are unchanged (immutability), not only the transmit count; present-tensed the mock's enqueue/completion comment; American-English spelling. - unit_FilesystemModule_persistence: the reordered-code-wired-siblings test asserts a distinctive persisted control value (speed=137) survives reload. - unit_Drivers_rendersplit: the render-split reorder cases carry forward unchanged. Docs / CI - drivers.md: doubleBuffer and pinExpander documented as peripheral-specific (below the divider); the lightPreset reference-durability note corrected (rename breaks the persisted-name reference across a reboot); i80/MoonI80 note the S31; the classic i80 light cap left as an illustrative order-of-magnitude. - MIGRATING: the ParallelLedDriver re-add action lists the per-peripheral controls (i80 clockPin/dcPin, MoonI80 clockPin+ring, Parlio none); scoped the "ParlioLedDriver unchanged" note to the 2026-07-16 rename. - ADR-0016 / lessons.md: aligned the consolidation-branch bug count to four. Reviews - 🐇 R3 MoonModule failed-mutation quiesce order, R6 app.js details-before-scroll, R2 Control.cpp label boundary tests, R5/R7 present-tense, S31 doc consistency: fixed. R4 (TasksModule provider) + R-main.cpp (CONFIG_SOC to platform glue) skipped — the seam already exists / main.cpp is the composition root. R-deviceModels hpwit-15 skipped — the "256" broadcast already computes the 15-active-strand rig. - 🐇 second batch: FilesystemModule reordered-wired-child restore, swap-ordering assertion, pinexpander buffer-immutability + spelling, MIGRATING/drivers.md peripheral-control docs: fixed. Classic-i80 8-vs-16-lane light-cap change skipped — the byte budget makes the total-lights ceiling width-independent, so the finding's premise is wrong. - 👾 persistence-robustness review (earlier commit) carried forward; no new findings this batch. Gates - Skipped: the 4 scenario perf-contract checks (baseline/mirror/grid-sweep) fail on desktop-macos — the contracts were calibrated 2026-06-02 at ~97 µs and the measurement environment has since drifted to ~550 µs (observed 660 µs already on 2026-07-10, pre-dating this branch). All are tick-over-contract with zero correctness failures, on render-hot-path code this branch does not touch. Reason to skip: stale contracts, not a regression; renegotiation is separate maintenance. - Not run: the pre-commit Reviewer pass on this (large, multi-area) staged diff — committed directly at the product owner's request. - ESP32 tick in the KPI line is the classic Olimex (fresh serial capture); the S3/P4 native-USB boards reset on serial connect, so their live ticks (~3.7 ms S3 at 64² Noise, ~210 µs P4) are noted here rather than scraped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/light/drivers/MoonLedDriver.h (1)
437-445: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnforce single-buffer in
busInititself, not only at the call site.
supportsDoubleBuffer()returningfalsedocuments that this backend can never safely run double-buffered, butbusInitstill forwards the caller'swantSecondBufferverbatim into the platform call instead of refusing it locally. The only safety here is the orchestrator'swantSecond = doubleBuffer && peripheral_->supportsDoubleBuffer()computation inParallelLedDriver::reinit()— any future direct call tobusInit(a test, a new call site) that passestruewould silently re-introduce the whole-frame stall/wedge bug this PR fixes.🔒 Proposed defensive fix
bool busInit(size_t frameBytes, bool wantSecondBuffer) override { platform::moonI80SetShiftClockDiv(shiftOverclock ? 3 : 4); // ON = 26.67 MHz, OFF = 20 MHz - // wantSecondBuffer is already forced false for this backend by the orchestrator (see - // supportsDoubleBuffer + the busInit call site) — MoonI80 runs single-buffer, its speed comes - // from the streaming ring, not from double-buffering a whole frame. + // Defense-in-depth: this backend can never safely double-buffer (see supportsDoubleBuffer) — + // refuse any caller request for a second buffer here instead of relying solely on the + // orchestrator's gating in reinit(). + wantSecondBuffer = false; return platform::moonI80Ws2812Init(bus_, owner_->busPinList(), owner_->busPinCount(), static_cast<uint16_t>(clockPin), frameBytes, wantSecondBuffer, owner_->busClockMultiplier()); }🤖 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 `@src/light/drivers/MoonLedDriver.h` around lines 437 - 445, Update MoonLedDriver::busInit to enforce this backend’s single-buffer contract locally by ignoring or overriding the wantSecondBuffer argument before calling platform::moonI80Ws2812Init. Ensure the platform call always receives false, while preserving the existing clock, buffer, pin, and multiplier arguments.
🤖 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 `@docs/moonmodules/light/drivers.md`:
- Around line 15-18: The persistence explanation in
docs/moonmodules/light/drivers.md lines 15-18 is authoritative and requires no
direct change. Update docs/MIGRATING.md lines 35-43 to remove the duplicated
stable-ID, persisted-name, reordering, reboot, and rename mechanics, retaining
only the migration action and adding a generic link to the driver persistence
contract.
In `@moondeck/scenario/run_live_scenario.py`:
- Around line 229-247: Add explicit return-type annotations to controls_of,
walk, and _restore_tree, matching the file’s existing helper annotation style.
Update the walk invocation and definition to avoid the bare boolean positional
argument by using the project’s preferred keyword or equivalent explicit form
for inside_container, without changing traversal behavior.
- Around line 251-274: Update _restore_tree to log failures for each
module-creation and control POST instead of silently swallowing exceptions.
Include the affected module ID, and for control failures also include the
control name and exception details; retain the existing restore-count behavior
and continue attempting remaining modules and controls.
- Around line 358-367: Initialize live_state before the pre-flight try block so
it is always bound before the snapshot section around _snapshot_tree. If the
/api/state request fails, preserve the existing early-failure behavior or skip
snapshot restoration rather than calling _snapshot_tree with an unavailable
state; ensure the warning reflects the original state-fetch failure instead of a
misleading NameError.
In `@src/core/FilesystemModule.cpp`:
- Around line 239-243: The wired-child reconciliation currently advances past
the wrong JSON entry, allowing the later matched entry to factory-create a
duplicate. Update applyWiredChildFromJson and its caller to return or track the
matched JSON index, consume that entry exactly once, and prevent it from being
treated as a user-created child; update src/core/FilesystemModule.cpp lines
239-243 and 165-176 accordingly. Add a regression fixture in
test/unit/core/unit_FilesystemModule_persistence.cpp lines 681-721 with an
unknown entry before a boot-wired child and assert exactly one restored wired
child.
In `@test/scenarios/light/scenario_perf_full.json`:
- Around line 912-927: Exclude unsupported optional-driver benchmark
measurements from scenario_perf_full.json: at
test/scenarios/light/scenario_perf_full.json lines 912-927, remove or mark the
classic ESP32 i80 observations as unsupported; at lines 979-1013, remove or mark
the ESP32-S3 and classic ESP32 Parlio observations as unsupported. Ensure the
remaining measurements comply with the supported-platform contract.
In `@test/unit/light/unit_Drivers_rendersplit.cpp`:
- Around line 147-152: Remove the std::this_thread::sleep_for(1ms) call from the
30-frame loop in the render-split regression test. Assert renderSplitActive()
immediately after each drivers.tick(), or use an existing deterministic
render-completion signal if direct assertion is insufficient, while preserving
the 30-frame validation.
---
Outside diff comments:
In `@src/light/drivers/MoonLedDriver.h`:
- Around line 437-445: Update MoonLedDriver::busInit to enforce this backend’s
single-buffer contract locally by ignoring or overriding the wantSecondBuffer
argument before calling platform::moonI80Ws2812Init. Ensure the platform call
always receives false, while preserving the existing clock, buffer, pin, and
multiplier arguments.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7ff9820d-17b7-4b2e-a3e6-659dbfa592cb
📒 Files selected for processing (31)
docs/MIGRATING.mddocs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.mddocs/history/lessons.mddocs/moonmodules/light/drivers.mdmoondeck/scenario/run_live_scenario.pysrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/light/drivers/LedPeripheral.hsrc/light/drivers/MoonLedDriver.hsrc/light/drivers/ParallelLedDriver.htest/scenarios/core/scenario_DevicesModule_scan.jsontest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/core/scenario_MqttModule_haDiscovery_toggle.jsontest/scenarios/core/scenario_NetworkModule_eth_reconfigure.jsontest/scenarios/core/scenario_NetworkModule_mdns_toggle.jsontest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_GridBlacks_blackpixel.jsontest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_MoonLiveEffect_livescript.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/scenarios/light/scenario_peripheral_switch.jsontest/unit/core/unit_FilesystemModule_persistence.cpptest/unit/light/unit_Drivers_rendersplit.cpptest/unit/light/unit_ParallelLedDriver_doublebuffer.cpptest/unit/light/unit_ParallelLedDriver_pinexpander.cpptest/unit/light/unit_ParallelLedDriver_swap.cpp
💤 Files with no reviewable changes (1)
- test/scenarios/core/scenario_DevicesModule_scan.json
Comment on lines
+15
to
+18
| <img src="../../assets/light/drivers/RmtLedDriver.png" width="300" alt="Shared driver controls: localBrightness, lightPreset, whiteMode, start, count"> | ||
|
|
||
| - `localBrightness` — this driver's dim (0–255), multiplied with the global brightness into one LUT; both sliders reach the output. | ||
| - `preset` — the [light preset](supporting.md) this driver applies per light (channel order / RGBW synthesis), referenced by its stable id (not its name), so renaming or reordering presets never breaks a driver's reference and it survives a reboot. | ||
| - `lightPreset` — the [light preset](supporting.md) this driver applies per light (channel order / RGBW synthesis). At runtime the driver holds the preset's stable id, so **reordering** presets never disturbs the reference; the reference **survives a reboot** because the preset's *name* is persisted and re-resolved on load. The one caveat is **renaming**: within a session the id keeps the link, but after a reboot a renamed preset no longer matches the persisted name, so the driver falls back to the default preset — re-pick it if you rename a preset a driver uses. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep preset persistence mechanics in one authoritative document.
The technical driver page and migration guide duplicate the same runtime stable-ID, persisted-name, and rename behavior. Keep the detailed contract in docs/moonmodules/light/drivers.md and reduce docs/MIGRATING.md to the migration action plus a generic link.
docs/moonmodules/light/drivers.md#L15-L18: retain the authoritative persistence explanation.docs/MIGRATING.md#L35-L43: remove the duplicated mechanics and link to the technical contract.
As per coding guidelines, documentation should keep each fact in one authoritative location and link to it generically elsewhere.
📍 Affects 2 files
docs/moonmodules/light/drivers.md#L15-L18(this comment)docs/MIGRATING.md#L35-L43
🤖 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 `@docs/moonmodules/light/drivers.md` around lines 15 - 18, The persistence
explanation in docs/moonmodules/light/drivers.md lines 15-18 is authoritative
and requires no direct change. Update docs/MIGRATING.md lines 35-43 to remove
the duplicated stable-ID, persisted-name, reordering, reboot, and rename
mechanics, retaining only the migration action and adding a generic link to the
driver persistence contract.
Source: Coding guidelines
Comment on lines
+229
to
+247
| def controls_of(m): | ||
| return {c["name"]: c.get("value") for c in m.get("controls", []) | ||
| if c.get("name") and not c.get("readonly")} | ||
|
|
||
| def walk(modules, parent_name, inside_container): | ||
| for m in modules: | ||
| name = m.get("name") | ||
| typ = m.get("type") | ||
| # Capture a module that sits INSIDE a snapshot container and is user-editable | ||
| # (a driver/effect/modifier/layout/service the scenario could clear). Skip the | ||
| # boot-wired ones the device owns (userEditable false is not in /api/state, so | ||
| # gate on the known singletons by name instead). | ||
| if inside_container and name and typ and name not in ("Preview", "LightPresets"): | ||
| snap.append({"type": typ, "id": name, | ||
| "parent_id": parent_name, "controls": controls_of(m)}) | ||
| walk(m.get("children", []), name, | ||
| inside_container or name in _SNAPSHOT_CONTAINERS) | ||
|
|
||
| walk(state.get("modules", []), None, False) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Minor style gaps flagged by static analysis on the new helpers.
controls_of (L229), walk (L233), and _restore_tree (L251) lack return-type annotations (ANN202) — inconsistent with the file's other helpers (e.g. _collect_module_names -> set); and walk(..., False) (L247) passes a bare boolean positional argument (FBT003).
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 229-229: Missing return type annotation for private function controls_of
(ANN202)
[warning] 233-233: Missing return type annotation for private function walk
Add return type annotation: None
(ANN202)
[warning] 247-247: Boolean positional value in function call
(FBT003)
🤖 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 `@moondeck/scenario/run_live_scenario.py` around lines 229 - 247, Add explicit
return-type annotations to controls_of, walk, and _restore_tree, matching the
file’s existing helper annotation style. Update the walk invocation and
definition to avoid the bare boolean positional argument by using the project’s
preferred keyword or equivalent explicit form for inside_container, without
changing traversal behavior.
Source: Linters/SAST tools
Comment on lines
+251
to
+274
| def _restore_tree(client, snapshot: list, current_state: dict): | ||
| """Re-create any snapshotted module that a scenario removed, restoring the board to | ||
| the tree it had before the run. Adds parents before children (snapshot order) and | ||
| re-applies control values. A module still present is left untouched.""" | ||
| present = _collect_module_names(current_state) | ||
| restored = 0 | ||
| for entry in snapshot: | ||
| if entry["id"] in present: | ||
| continue | ||
| try: | ||
| client.post("/api/modules", {"type": entry["type"], "id": entry["id"], | ||
| "parent_id": entry["parent_id"]}) | ||
| for cname, val in entry["controls"].items(): | ||
| try: | ||
| client.post("/api/control", {"module": entry["id"], | ||
| "control": cname, "value": val}) | ||
| except Exception: | ||
| pass | ||
| restored += 1 | ||
| except Exception: | ||
| pass | ||
| if restored: | ||
| print(f" restored {restored} module(s) the scenario had cleared") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silent failure swallowing in _restore_tree hides which module/control failed to restore.
Both the module-creation POST (line 260) and the per-control POST (line 264) are wrapped in bare except Exception: pass, so a restore failure for any specific module or control produces no diagnostic at all — only the aggregate "restored N module(s)" count at the end. Given this function's entire purpose is leaving the bench board in a known-good state, a silent partial failure here means the operator has no way to know the board wasn't fully restored. Static analysis flags this as S110/BLE001.
🛡️ Proposed fix: log failures instead of swallowing them
try:
client.post("/api/modules", {"type": entry["type"], "id": entry["id"],
"parent_id": entry["parent_id"]})
for cname, val in entry["controls"].items():
try:
client.post("/api/control", {"module": entry["id"],
"control": cname, "value": val})
- except Exception:
- pass
+ except Exception as e:
+ print(f" WARN — restore {entry['id']}.{cname} failed: {e}")
restored += 1
- except Exception:
- pass
+ except Exception as e:
+ print(f" WARN — restore of {entry['id']} ({entry['type']}) failed: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _restore_tree(client, snapshot: list, current_state: dict): | |
| """Re-create any snapshotted module that a scenario removed, restoring the board to | |
| the tree it had before the run. Adds parents before children (snapshot order) and | |
| re-applies control values. A module still present is left untouched.""" | |
| present = _collect_module_names(current_state) | |
| restored = 0 | |
| for entry in snapshot: | |
| if entry["id"] in present: | |
| continue | |
| try: | |
| client.post("/api/modules", {"type": entry["type"], "id": entry["id"], | |
| "parent_id": entry["parent_id"]}) | |
| for cname, val in entry["controls"].items(): | |
| try: | |
| client.post("/api/control", {"module": entry["id"], | |
| "control": cname, "value": val}) | |
| except Exception: | |
| pass | |
| restored += 1 | |
| except Exception: | |
| pass | |
| if restored: | |
| print(f" restored {restored} module(s) the scenario had cleared") | |
| def _restore_tree(client, snapshot: list, current_state: dict): | |
| """Re-create any snapshotted module that a scenario removed, restoring the board to | |
| the tree it had before the run. Adds parents before children (snapshot order) and | |
| re-applies control values. A module still present is left untouched.""" | |
| present = _collect_module_names(current_state) | |
| restored = 0 | |
| for entry in snapshot: | |
| if entry["id"] in present: | |
| continue | |
| try: | |
| client.post("/api/modules", {"type": entry["type"], "id": entry["id"], | |
| "parent_id": entry["parent_id"]}) | |
| for cname, val in entry["controls"].items(): | |
| try: | |
| client.post("/api/control", {"module": entry["id"], | |
| "control": cname, "value": val}) | |
| except Exception as e: | |
| print(f" WARN — restore {entry['id']}.{cname} failed: {e}") | |
| restored += 1 | |
| except Exception as e: | |
| print(f" WARN — restore of {entry['id']} ({entry['type']}) failed: {e}") | |
| if restored: | |
| print(f" restored {restored} module(s) the scenario had cleared") |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 251-251: Missing return type annotation for private function _restore_tree
Add return type annotation: None
(ANN202)
[warning] 264-268: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 267-268: try-except-pass detected, consider logging the exception
(S110)
[warning] 267-268: try-except within a loop incurs performance overhead
(PERF203)
[warning] 267-267: Do not catch blind exception: Exception
(BLE001)
[error] 270-271: try-except-pass detected, consider logging the exception
(S110)
[warning] 270-270: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@moondeck/scenario/run_live_scenario.py` around lines 251 - 274, Update
_restore_tree to log failures for each module-creation and control POST instead
of silently swallowing exceptions. Include the affected module ID, and for
control failures also include the control name and exception details; retain the
existing restore-count behavior and continue attempting remaining modules and
controls.
Source: Linters/SAST tools
Comment on lines
+358
to
+367
| # Snapshot the board's user-added tree so we can restore it after the scenario: | ||
| # a scenario that clear_children's a container (to get a known canvas) destroys the | ||
| # board's real config, and the created_modules cleanup only removes what the scenario | ||
| # ADDED, not what it CLEARED. Restoring the snapshot leaves the bench board as found. | ||
| tree_snapshot = [] | ||
| try: | ||
| tree_snapshot = _snapshot_tree(live_state) | ||
| except Exception as e: | ||
| print(f" WARN — couldn't snapshot tree for restore: {e}") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
live_state may be unbound when snapshotting, producing a confusing NameError message instead of a real warning.
live_state is only assigned inside the pre-flight try block (line 325). If client.get("/api/state") there raises, execution doesn't return early — it falls through to line 364's _snapshot_tree(live_state), which then raises NameError: name 'live_state' is not defined, caught generically and printed as "couldn't snapshot tree for restore: name 'live_state' is not defined" — a misleading message that hides the real cause (the earlier /api/state failure).
🛡️ Proposed fix
target = "unknown"
+ live_state = {}
try:
live_state = client.get("/api/state")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Snapshot the board's user-added tree so we can restore it after the scenario: | |
| # a scenario that clear_children's a container (to get a known canvas) destroys the | |
| # board's real config, and the created_modules cleanup only removes what the scenario | |
| # ADDED, not what it CLEARED. Restoring the snapshot leaves the bench board as found. | |
| tree_snapshot = [] | |
| try: | |
| tree_snapshot = _snapshot_tree(live_state) | |
| except Exception as e: | |
| print(f" WARN — couldn't snapshot tree for restore: {e}") | |
| # Snapshot the board's user-added tree so we can restore it after the scenario: | |
| # a scenario that clear_children's a container (to get a known canvas) destroys the | |
| # board's real config, and the created_modules cleanup only removes what the scenario | |
| # ADDED, not what it CLEARED. Restoring the snapshot leaves the bench board as found. | |
| tree_snapshot = [] | |
| live_state = {} | |
| try: | |
| tree_snapshot = _snapshot_tree(live_state) | |
| except Exception as e: | |
| print(f" WARN — couldn't snapshot tree for restore: {e}") |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 365-365: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@moondeck/scenario/run_live_scenario.py` around lines 358 - 367, Initialize
live_state before the pre-flight try block so it is always bound before the
snapshot section around _snapshot_tree. If the /api/state request fails,
preserve the existing early-failure behavior or skip snapshot restoration rather
than calling _snapshot_tree with an unavailable state; ensure the warning
reflects the original state-fetch failure instead of a misleading NameError.
Comment on lines
+239
to
+243
| if (live && live->isWiredByCode()) { | ||
| applyWiredChildFromJson(live, json, prefix); | ||
| pos++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not re-create a JSON entry already consumed for a wired child.
A mismatched wired child is restored from a later matching JSON entry, but pos advances for the current entry. When the loop reaches that later entry, it sees no live child and factory-creates a duplicate. For example, saved [GoneEffect, RainbowEffect] with a boot-wired RainbowEffect produces two Rainbows.
src/core/FilesystemModule.cpp#L239-L243: reconcile the current JSON entry and wired child without advancing past the wired child until its corresponding saved entry is consumed.src/core/FilesystemModule.cpp#L165-L176: return or track the matched JSON index so it cannot later be treated as a user-created child.test/unit/core/unit_FilesystemModule_persistence.cpp#L681-L721: add a fixture with an unknown entry before a boot-wired child and assert exactly one restored wired child remains.
🧰 Tools
🪛 Clang (14.0.6)
[note] 239-239: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 239-239: +1
(clang)
[warning] 239-239: implicit conversion 'mm::MoonModule *' -> bool
(readability-implicit-bool-conversion)
📍 Affects 2 files
src/core/FilesystemModule.cpp#L239-L243(this comment)src/core/FilesystemModule.cpp#L165-L176test/unit/core/unit_FilesystemModule_persistence.cpp#L681-L721
🤖 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 `@src/core/FilesystemModule.cpp` around lines 239 - 243, The wired-child
reconciliation currently advances past the wrong JSON entry, allowing the later
matched entry to factory-create a duplicate. Update applyWiredChildFromJson and
its caller to return or track the matched JSON index, consume that entry exactly
once, and prevent it from being treated as a user-created child; update
src/core/FilesystemModule.cpp lines 239-243 and 165-176 accordingly. Add a
regression fixture in test/unit/core/unit_FilesystemModule_persistence.cpp lines
681-721 with an unknown entry before a boot-wired child and assert exactly one
restored wired child.
Comment on lines
+912
to
+927
| "esp32": { | ||
| "tick_us": [ | ||
| 310, | ||
| 193, | ||
| 311 | ||
| ], | ||
| "free_heap": [ | ||
| 119224, | ||
| 121220 | ||
| ], | ||
| "max_alloc_block": [ | ||
| 102400, | ||
| 94208, | ||
| 110592 | ||
| ], | ||
| "at": [ | ||
| "2026-07-22", | ||
| "2026-07-22" | ||
| "2026-07-24" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Exclude unsupported platforms from optional-driver benchmarks.
The i80 and Parlio steps record measurements on targets where those peripherals are unavailable, so the values capture initialization/bailout overhead rather than the advertised frame cost.
test/scenarios/light/scenario_perf_full.json#L912-L927: remove or mark the classic ESP32 i80 observations as unsupported.test/scenarios/light/scenario_perf_full.json#L979-L1013: remove or mark the ESP32-S3 and classic ESP32 Parlio observations as unsupported.
As per path instructions, scenario measurements must match the supported-platform contract.
📍 Affects 1 file
test/scenarios/light/scenario_perf_full.json#L912-L927(this comment)test/scenarios/light/scenario_perf_full.json#L979-L1013
🤖 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 `@test/scenarios/light/scenario_perf_full.json` around lines 912 - 927, Exclude
unsupported optional-driver benchmark measurements from scenario_perf_full.json:
at test/scenarios/light/scenario_perf_full.json lines 912-927, remove or mark
the classic ESP32 i80 observations as unsupported; at lines 979-1013, remove or
mark the ESP32-S3 and classic ESP32 Parlio observations as unsupported. Ensure
the remaining measurements comply with the supported-platform contract.
Source: Path instructions
Comment on lines
+147
to
+152
| // Tick 30 frames WITHOUT touching config. The split must not disengage on its own. | ||
| for (int i = 0; i < 30; i++) { | ||
| drivers.tick(); | ||
| std::this_thread::sleep_for(1ms); | ||
| CHECK(drivers.renderSplitActive()); // any false here reproduces the bench flap | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the timing dependency from this regression test.
std::this_thread::sleep_for(1ms) does not synchronize with render work and can make this test flaky; assert after tick() directly or wait on a deterministic observable event. As per coding guidelines, “Tests should not depend on timing or network.”
Proposed fix
for (int i = 0; i < 30; i++) {
drivers.tick();
- std::this_thread::sleep_for(1ms);
CHECK(drivers.renderSplitActive()); // any false here reproduces the bench flap
}🤖 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 `@test/unit/light/unit_Drivers_rendersplit.cpp` around lines 147 - 152, Remove
the std::this_thread::sleep_for(1ms) call from the 30-frame loop in the
render-split regression test. Assert renderSplitActive() immediately after each
drivers.tick(), or use an existing deterministic render-completion signal if
direct assertion is insufficient, while preserving the 30-frame validation.
Source: Coding guidelines
Processes the pre-merge Reviewer pass and the CodeRabbit batch on PR #53: a real duplicate-wired-child persistence bug, live-scenario runner hardening, a MoonI80 single-buffer belt-and-suspenders, and doc/test cleanups. Records the carry-forward lessons and perf note for the merge. Core - FilesystemModule::applyNode: a code-wired child restored out of order (its saved index differs from its boot index) no longer spawns a DUPLICATE. A new hasWiredChildOfType() guard drops a JSON entry whose type already exists as a live wired child — a type-singleton can't legitimately produce a second instance, so that entry is the singleton's own saved slot, already restored by the type-search. Keeps the positional wired-absorb the reorder case depends on; the trim loop is untouched. Light domain - MoonLedDriver: busInit forces single-buffer locally (passes wantSecondBuffer=false regardless of the request) so a future caller that bypasses the supportsDoubleBuffer() gate still can't reach the freeze-prone whole-frame double-buffer path. - ParallelLedDriver: corrected the registry doc comment — backends self-register via `inline const bool kXxxPeripheralRegistered = registerPeripheral(...)` in each backend header, and the CONFIG_SOC_* gating lives in main.cpp's #include guards, not the backend (there is no MM_REGISTER_LED_PERIPHERAL macro). Scripts / MoonDeck - run_live_scenario.py: bind live_state before the pre-flight try so a failed /api/state can't NameError the snapshot; _restore_tree logs each failed module/control restore (id + error) instead of swallowing it; added return-type annotations to the new snapshot/restore helpers and made walk's inside_container keyword-only. Tests - unit_FilesystemModule_persistence: a regression case — an unknown entry before a boot-wired child restores exactly one wired child (speed survives), no duplicate. Green→red verified. - unit_Drivers_rendersplit: dropped the sleep from the single-layer no-flap loop — renderSplitActive() is a synchronous state flag, so the sleep only added flake. - scenario_perf_full: removed misleading unsupported-platform observations (classic esp32 on measure-i80; S3 + classic on measure-parlio) that recorded init-bailout overhead, not the real peripheral. Docs / CI - lessons.md: the MoonI80 whole-frame double-buffer freeze lesson (the completion-signal race + the subtract-don't-patch supportsDoubleBuffer() fix). - performance.md: async double-buffer is peripheral-specific (i80/Parlio via a real transaction queue; MoonI80 single-buffer only, ring is its scale path). - MIGRATING: the lightPreset entry links the authoritative persistence contract instead of restating it. Reviews - 👾 Reviewer (pre-merge whole-branch): architecturally clean — non-leaky LedPeripheral interface, supportsDoubleBuffer() is a real capability seam, snapshot/restore reuses existing primitives, no hot-path violations, net subtraction. Fixed the one comment finding (stale macro reference); accepted the ADR mechanism-nuance finding (ADRs are immutable; the decision is accurately recorded, the mechanism is now correct in the code docstrings). - 🐇 duplicate-wired-child (Major): fixed with the hasWiredChildOfType guard + regression test. run_live_scenario.py live_state/logging/annotations, MoonLedDriver single-buffer, MIGRATING dedup, perf_full observations, rendersplit sleep: fixed. The 18 pre-existing ruff ANN warnings in run_live_scenario.py: skipped (the finding was about the 3 new helpers, now annotated; the rest predate this branch). Stale/already-fixed findings from an earlier snapshot (Control label boundary, failed-mutation quiesce, Drivers.h present-tense, app.js focus order, ADR count, classic-i80 lane cap): already landed in prior commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
Consolidates the three parallel-LED driver classes into one
ParallelLedDriverwith a runtime peripheral strategy, then hardens the result: a MoonI80 double-buffer freeze, driver stall recovery, persistence/audio robustness, and a live-scenario test suite that guards it all.The defining change is the consolidation (ADR-0016):
MultiPinLedDriver,MoonLedDriver, andParlioLedDriverwere the same driver with a different DMA backend. They become one registered module whoseperipheralcontrol selects aLedPeripheral*backend at runtime (i80/MoonI80/Parlio), so the add-module picker shows one "Parallel LED" card on every board and the dropdown offers only the peripherals the chip supports. Net subtraction: 4 factory-registered classes → 1 module + 1 interface + 3 stripped backends, one control set, one UI card.Changes
Light domain — the driver consolidation + fixes
MoonModuleholding a runtimeLedPeripheral*(CRTP removed). One vtable dispatch per frame to reach the backend, never per light, so the per-light encode stays a direct call. Backends self-register into a static registry; which link is#if CONFIG_SOC_*-gated at the#includes inmain.cpp. A peripheral-block claim guard (hwBlock(), RTTI-free) refuses a second driver on the same hardware block.doubleBufferon wedged the bus at ~200 ms/frame (5 fps) after a couple of clean frames — a completion-signal race in its own-GDMA whole-frame two-buffer handshake (a binarywireFreesemaphore + a blind pre-drain that could swallow the real completion). Fix subtracts rather than patches: aLedPeripheral::supportsDoubleBuffer()seam (default true; MoonI80 false), the orchestrator gates the second-buffer request on it, MoonI80 runs single-buffer (its speed is the streaming ring, not double-buffering a whole frame), and thedoubleBuffercontrol hides on a peripheral that can't run it (below theperipheraldivider, visibility computed after the swap).busyforever (~5 fps); both the ring and whole-frame backstops now share onefinalizeStalledTransferstop-and-clear, so a lost interrupt self-heals on either path.loopbackTestexpert-only. PreviewDriver: adaptive downscale settles in ~1 s instead of ~10 s.preset.Core — robustness
peripheralswap) restores its backend-owned controls via overlay → rebuildControls → overlay; a positional child reconciler no longer drops the tail on the first unresolvable entry (a renamed/removed type, or a reordered code-wired sibling); a reordered code-wired child restores its own persisted controls; a live backend free fires the same render-worker quiesce a tree mutation does.%svs an escape-decoding reader) — now escaped; an out-of-memory restore no longer writes through a null role pool; corrupt role bytes clamp on restore.synccontrol becamemode+send audio.quiesceForMutation()router stops the core-1 encode worker before any of the four child-array mutators (add/remove/replace/moveChildTo) runs, reaching the worker wherever it lives via a function-pointer hook wired inmain.cpp.UI / installer
Tests
ParallelLedDriverthrough i80 / MoonI80 / Parlio, togglingdoubleBufferandpinExpander— the MoonI80-with-double-buffer-on measure is the freeze regression guard (a ~200 ms tick fails it). Verified live on S3, P4 (all three peripherals incl. Parlio), and the host runner; auto-discovered by the C++ scenario gate.clear_childrendestroyed, so a live scenario leaves the bench board exactly as found.Verification
LedPeripheralstrategy is a non-leaky interface,supportsDoubleBuffer()is a real capability seam, snapshot/restore reuses existing primitives, no hot-path violations, net subtraction. One comment fix applied (a stale macro reference in the registry doc); one ADR mechanism-nuance accepted (ADRs are immutable; the decision is accurately recorded).Notes for review
desktop-macosscenario perf contracts (baseline/mirror/grid-sweep) fail on the current measurement machine — calibrated 2026-06-02 at ~97 µs, drifted to ~550 µs (already 660 µs by 2026-07-10, pre-dating this branch). All are tick-over-contract with zero correctness failures, on render-hot-path code this branch does not touch. Renegotiating them is separate contract maintenance.MultiPinLedDriver/MoonLedDriver/ParlioLedDriverpersisted type will drop that driver on boot (the type no longer resolves) — re-add a Parallel LED driver and pick the peripheral. Documented in MIGRATING.md; the installer catalog already names the new type.🤖 Generated with Claude Code