Fix MoonLed whole-frame stall; harden LED-driver status + DMA budget#53
Open
ewowi wants to merge 4 commits into
Open
Fix MoonLed whole-frame stall; harden LED-driver status + DMA budget#53ewowi wants to merge 4 commits into
ewowi wants to merge 4 commits into
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:
✨ 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
| Each driver (a MoonModule) speaks one protocol: | ||
|
|
||
| - **LED drivers**: WS2812 via RMT (multi-pin), plus two parallel-output paths on the newer chips. The S3's LCD_CAM i80 bus ([MultiPinLedDriver](moonmodules/light/moxygen/MultiPinLedDriver.md)) drives exactly 8 data GPIOs — the i80 bus claims every data line of its width, so a partial set is rejected. The P4's Parlio peripheral ([ParlioLedDriver](moonmodules/light/moxygen/ParlioLedDriver.md)) drives 1–8 lanes — it takes the data GPIOs directly, so any count up to 8 is valid. Both are DMA-driven. Platform-specific; all behind the platform boundary. | ||
| - **LED drivers**: WS2812 via RMT (multi-pin), plus one DMA-driven parallel driver ([ParallelLedDriver](moonmodules/light/moxygen/ParallelLedDriver.md)) whose `peripheral` control picks the bus backend the chip supports — the S3/P4 LCD_CAM i80 bus, our own-GDMA MoonI80 (LCD_CAM, adds the streaming ring + 74HCT595 expander), or the P4's Parlio. All are DMA-driven and behind the platform boundary; the driver rounds an i80 bus up around whatever pin count is configured (any count from 1) and parks unused lanes on a pin already driven. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the classic ESP32 parallel backend.
This list omits the classic ESP32 path, while docs/history/plans/Plan-20260723 - Consolidate parallel LED drivers into one module + peripheral strategy.md states that the MultiPin backend supports classic I2S and that the classic build links the i80 backend. Add that backend or explicitly explain its exclusion so the architecture contract does not imply classic ESP32 lacks parallel output.
🤖 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/architecture.md` at 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.
Comment on lines
+202
to
+203
| > **Updating from an older build?** Skim the [migration notes](MIGRATING.md) first. Most updates need nothing — the device keeps your settings — but a breaking change is listed there with the one action it costs you (usually re-setting or re-adding a control). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside this blockquote.
The empty > line triggers markdownlint MD028.
Proposed fix
> **Updating from an older build?** Skim the [migration notes](MIGRATING.md) first. Most updates need nothing — the device keeps your settings — but a breaking change is listed there with the one action it costs you (usually re-setting or re-adding a control).
->
> [FirmwareUpdateModule](moonmodules/core/system.md#firmware-update)📝 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
| > **Updating from an older build?** Skim the [migration notes](MIGRATING.md) first. Most updates need nothing — the device keeps your settings — but a breaking change is listed there with the one action it costs you (usually re-setting or re-adding a control). | |
| > **Updating from an older build?** Skim the [migration notes](MIGRATING.md) first. Most updates need nothing — the device keeps your settings — but a breaking change is listed there with the one action it costs you (usually re-setting or re-adding a control). |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 203-203: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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/gettingstarted.md` around lines 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.
Source: Linters/SAST tools
|
|
||
| - **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 neighbours 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 PASS on the S31 for both [RmtLedDriver](../moonmodules/light/drivers.md#rmtled) and [ParlioLedDriver](../moonmodules/light/drivers.md#parlioled)** — the two WS2812 output drivers the S31 supports. [LcdLedDriver](../moonmodules/light/drivers.md#led-drivers) is **not** one of them: it's the ESP32-S3-specific LCD_CAM i80 driver, and the RISC-V S31 has no LCD_CAM peripheral (it reports "no valid 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. | ||
| - **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 PASS on the S31 for both [RMT](../moonmodules/light/drivers.md#rmtled) and the [Parallel LED driver with peripheral `Parlio`](../moonmodules/light/drivers.md#parlioled)** — the two WS2812 output paths the S31 supports. The `i80` / `MoonI80` peripherals are **not** among them: they need the ESP32-S3/P4 LCD_CAM, and the RISC-V S31 has no LCD_CAM peripheral (the `peripheral` selector doesn't offer them, and forcing one reports "no valid 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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate files ---\n'
git ls-files | grep -E '(^|/)esp32-s31-coreboard\.md$|moonmodules/light/drivers\.md$|\.md$' | sed -n '1,120p'
printf '\n--- esp32-s31-coreboard.md around relevant lines ---\n'
sed -n '100,145p' docs/reference/esp32-s31-coreboard.md
printf '\n--- relevant search terms ---\n'
rg -n "LCD_CAM|LCD.?\s*(CAM|cam)|i80|MoonI80|Parlio|RMT|S31|ESP32-S3|GPIO 48|loopbackTest|selector" docs/reference/esp32-s31-coreboard.md moonmodules light docs || trueRepository: MoonModules/projectMM
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)esp32-s31-coreboard\.md$|moonmodules/light/drivers\.md$|\.md$' | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- esp32-s31-coreboard.md around relevant lines ---'
sed -n '100,145p' docs/reference/esp32-s31-coreboard.md
printf '%s\n' ''
printf '%s\n' '--- relevant search terms ---'
rg -n "LCD_CAM|LCD.? CAM|LCD.?Cam|lcd.?cam|i80|MoonI80|Parlio|RMT|S31|ESP32-S3|GPIO 48|loopbackTest|selector" docs/reference/esp32-s31-coreboard.md moonmodules light docs || trueRepository: MoonModules/projectMM
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- esp32-s31-coreboard.md line 126 and nearby ---'
sed -n '110,135p' docs/reference/esp32-s31-coreboard.md | nl -ba -v110
printf '%s\n' ''
printf '%s\n' '--- precise LCD_CAM/i80 mentions in esp32-s31-coreboard.md ---'
rg -n "LCD_CAM|LCD.? CAM|LCD.?Cam|lcd.?cam|i80|MoonI80|LED_CAM|LED.?CAM|peripheral selector|peripheral" docs/reference/esp32-s31-coreboard.md
printf '%s\n' ''
printf '%s\n' '--- references to config macros around SOC_LCDCAM_I80_LCD_SUPPORTED ---'
rg -n "SOC_LCDCAM_I80_LCD_SUPPORTED|CONFIG_LCDCAM|LCD_CAM_SUPPORTED|LcdCam|esp32s31|ESP32-S31|S31" --glob '!docs/backlog/*' --glob '!src/**/build/**' .
sed -n '1,220p' docs/reference/esp32-s31-coreboard.mdRepository: MoonModules/projectMM
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- esp32-s31-coreboard.md around 118-132 (with line numbers) ---'
sed -n '118,132p' docs/reference/esp32-s31-coreboard.md | awk '{printf "%d\t%s\n", NR+117, $0}'
printf '%s\n' ''
printf '%s\n' '--- precise LCD_CAM/i80/S31 mentions in esp32-s31-coreboard.md ---'
rg -n "LCD_CAM|LCD.? CAM|LCD.?Cam|lcd.?cam|i80|MoonI80|LED_CAM|peripheral selector|peripheral|ESP32-S31|S31" docs/reference/esp32-s31-coreboard.md
printf '%s\n' ''
printf '%s\n' '--- config/selector references for S31 or LED_CAP/LCD_CAP ---'
rg -n "LED_CAP|LCD_CAP|LCD_CAM_I80|SOC_LCDCAM_I80_LCD_SUPPORTED|esp32s31|ESP32-S31|S31|MoonI80|i80Ws" --glob '!docs/backlog/*' --glob '!build/**' --glob '!dist/**' .
printf '%s\n' ''
printf '%s\n' '--- full esp32-s31-coreboard.md first 220 lines for board reference context ---'
sed -n '1,220p' docs/reference/esp32-s31-coreboard.mdRepository: MoonModules/projectMM
Length of output: 50378
🌐 Web query:
ESP32-S31 LCD_CAM i80 LCD support IDF esp32s31 LED_CAM peripheral SOCs
💡 Result:
The ESP32-S31 SoC features a dedicated LCD and Camera controller (LCD_CAM) that provides comprehensive support for parallel display interfaces [1][2][3]. LCD_CAM Peripheral Support: The ESP32-S31 LCD_CAM peripheral includes an independent LCD control module and a camera control module [2]. It supports several display interface modes, including 8/16/24-bit parallel output and specific LCD formats such as RGB, MOTO6800, and I8080 [2][3]. ESP-IDF Support: Official support for the ESP32-S31 is integrated into the ESP-IDF framework [4][5]. The esp_lcd component and related hardware abstraction layers fully support the chip's native LCD_CAM capabilities for both RGB and I8080 LCDs [6][7][8]. Key technical details for ESP32-S31 LCD support include: - Native I80 and RGB LCD support via the LCD_CAM peripheral [9][6]. - Capability macros verify this support within the IDF: SOC_LCDCAM_I80_LCD_SUPPORTED and SOC_LCDCAM_RGB_LCD_SUPPORTED are both enabled (set to 1) [9][6]. - The ESP-IDF also allows for Parallel IO (PARLIO) simulation of SPI or I80 interfaced LCDs, providing additional flexibility for display connectivity [10][8]. Documentation and resources for the ESP32-S31, including programming guides and hardware references, are available in the official Espressif documentation and the ESP-IDF repository [11][8][4].
Citations:
- 1: https://documentation.espressif.com/esp32-s31_datasheet_en.pdf
- 2: https://documentation.espressif.com/esp32-s31-wroom-3_datasheet_en.pdf
- 3: https://www.espressif.com/en/products/socs/esp32-s31
- 4: espressif/esp-idf@a3f14c8
- 5: espressif/esp-idf@7ed6c83
- 6: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s31/api-reference/system/soc_caps.html
- 7: https://github.com/espressif/esp-iot-solution/blob/9971a4692/docs/en/display/lcd/lcd_development_guide.rst
- 8: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s31/api-reference/peripherals/lcd/index.html
- 9: https://github.com/espressif/esp-idf/blob/master/components/soc/esp32s31/include/soc/soc_caps.h
- 10: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s31/api-reference/peripherals/lcd/parl_lcd.html
- 11: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s31/hw-reference/index.html
Reconcile the S31 LCD_CAM claim.
The same reference already lists LCD_CAM i80 as an S31 capability, so line 126 should not say the S31 has no LCD_CAM peripheral. Document the precise supported limitation instead, such as unavailable/unusable loopback pin mappings or a missing board/catalog configuration for that path, or update the SoC capabilities list if it does not apply to this board.
🤖 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` at 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.
Comment on lines
+25
to
+29
| The LED-data outputs. Each terminal is `O<gpio>` on the silkscreen; a level shifter drives the 5 V strand from the P4's 3.3 V. The catalog wires the Parallel LED driver (peripheral `Parlio`) to the first eight (`21,20,25,5,22,23,24,27`). | ||
|
|
||
| | Terminal | GPIO | Note | | ||
| |---|---|---| | ||
| | O21 O20 O25 O5 O23 O27 O22 O24 | 21 20 25 5 23 27 22 24 | LED lanes (ParlioLedDriver default) | | ||
| | O21 O20 O25 O5 O23 O27 O22 O24 | 21 20 25 5 23 27 22 24 | LED lanes (Parallel LED, peripheral `Parlio`, default) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)web-installer/deviceModels\.json$|docs/reference/mhc-wled-esp32-p4-shield\.md$' || true
echo
echo "Reference doc lines 20-35:"
if [ -f docs/reference/mhc-wled-esp32-p4-shield.md ]; then
nl -ba docs/reference/mhc-wled-esp32-p4-shield.md | sed -n '20,35p'
fi
echo
echo "Device model candidates matching MHC-WLED/P4/WLED/LED Parlio/Wireframe:"
if [ -f web-installer/deviceModels.json ]; then
rg -n "MHC|WLED|Parlio|Parallel LED|wled|" web-installer/deviceModels.json || true
fiRepository: MoonModules/projectMM
Length of output: 310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Reference doc lines 20-35:"
awk 'NR>=20 && NR<=35 { printf "%d\t%s\n", NR, $0 }' docs/reference/mhc-wled-esp32-p4-shield.md
echo
echo "Parlio / LED lane mentions in device model:"
rg -n "Parlio|Parallel LED|wled|MHC|ESP32|21|20|25|5|22|23|24|27" web-installer/deviceModels.json || true
echo
echo "Extract JSON objects containing MHC-WLED/P4/WLED matches:"
python3 - <<'PY'
import json, re
from pathlib import Path
p=Path('web-installer/deviceModels.json')
text=p.read_text()
data=json.loads(text)
# Handle top-level object with nested arrays/objects or array of objects
def walk(x, path=''):
if isinstance(x, dict):
s=json.dumps(x, ensure_ascii=False)
if re.search(r'MHC|WLED|Parlio|Parallel LED|21|20|25|5|22|23|24|27', s):
print('--- ' + path)
print(s[:4000])
for k,v in x.items():
walk(v, path+'.'+str(k))
elif isinstance(x, list):
for i,v in enumerate(x):
walk(v, path+'['+str(i)+']')
else:
s=str(x)
if re.search(r'MHC|WLED|Parlio|Parallel LED|21|20|25|5|22|23|24|27', s):
print('--- ' + path)
print(s[:2000])
walk(data)
PYRepository: MoonModules/projectMM
Length of output: 28983
Use one authoritative Parlio lane order.
docs/reference/mhc-wled-esp32-p4-shield.md uses 21,20,25,5,22,23,24,27 in the prose but lists 23,27,22,24 in the table. web-installer/deviceModels.json also uses 21,20,25,5,22,23,24,27; reconcile the table with that and the physical terminal labels so users won’t wire lanes out of order.
🤖 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/mhc-wled-esp32-p4-shield.md` around lines 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.
Comment on lines
+319
to
+343
| const int hi = c.max > 0 ? c.max - 1 : 0; | ||
| // A Select value may be given as the option LABEL (a string) instead of the index. This is | ||
| // what makes a catalog config board-portable: the index into a board-FILTERED option list | ||
| // varies per chip (an S3 offers fewer peripherals than a P4), but the label is stable. Match | ||
| // the string against the options and use that row; fall back to the numeric index otherwise. | ||
| // Select-only: a Select's aux IS the options array (const char* const*); Palette's aux is a | ||
| // PaletteOptionsFn (a function pointer), so it must not reach this reinterpret_cast. | ||
| char label[24] = {}; | ||
| mm::json::parseString(json, key, label, sizeof(label)); | ||
| if (label[0]) { | ||
| auto* options = reinterpret_cast<const char* const*>(c.aux); | ||
| if (options) | ||
| 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 (e.g. a peripheral this board can't run) is not an | ||
| // error in Lenient policy — the driver keeps its default; Strict rejects it. | ||
| if (policy == ApplyPolicy::Strict) return ApplyResult::OutOfRange; | ||
| return ApplyResult::Ok; | ||
| } | ||
| int v = mm::json::parseInt(json, key); | ||
| if (policy == ApplyPolicy::Strict && (v < 0 || v > hi)) { | ||
| return ApplyResult::OutOfRange; | ||
| } | ||
| return clampInto(static_cast<uint8_t*>(c.ptr), v, 0, hi); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not manufacture a valid index for empty option lists.
Both branches map c.max == 0 to hi == 0, so strict numeric index 0 is accepted despite there being no options.
src/core/Control.cpp#L319-L343: guard empty Select controls before numeric parsing.src/core/Control.cpp#L348-L349: apply the same guard to Palette controls.
🧰 Tools
🪛 Clang (14.0.6)
[note] 319-319: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 328-328: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 330-330: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 332-332: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 332-332: +1
(clang)
[note] 336-336: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 340-340: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 340-340: +1
(clang)
[note] 340-340: +1
(clang)
[warning] 319-319: variable name 'hi' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 326-326: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 328-328: implicit conversion 'char' -> bool
(readability-implicit-bool-conversion)
[warning] 329-329: 'auto *options' can be declared as 'const auto *options'
(readability-qualified-auto)
[warning] 330-330: implicit conversion 'const char *const *' -> bool
(readability-implicit-bool-conversion)
[warning] 330-330: statement should be inside braces
(readability-braces-around-statements)
[warning] 331-331: statement should be inside braces
(readability-braces-around-statements)
[warning] 332-332: implicit conversion 'const char *' -> bool
(readability-implicit-bool-conversion)
[warning] 332-332: statement should be inside braces
(readability-braces-around-statements)
[warning] 336-336: statement should be inside braces
(readability-braces-around-statements)
[warning] 339-339: variable name 'v' is too short, expected at least 3 characters
(readability-identifier-length)
🪛 Cppcheck (2.21.0)
[style] 322-322: The function 'addInt16' is never used.
(unusedFunction)
[style] 339-339: The function 'addPin' is never used.
(unusedFunction)
📍 Affects 1 file
src/core/Control.cpp#L319-L343(this comment)src/core/Control.cpp#L348-L349
🤖 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 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.
Comment on lines
+326
to
+327
| char label[24] = {}; | ||
| mm::json::parseString(json, key, label, sizeof(label)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not truncate Select labels before matching.
parseString(..., sizeof(label)) limits matching to 23 characters, so any longer option label can never match; strict restores reject it and lenient restores silently keep the previous value. Use a length-aware comparison/parser or explicitly reject overlong labels instead of silently truncating.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 326-326: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
🤖 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 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.
| size_t dataBytes, uint8_t rowBits) override { | ||
| // The private bus is built from the orchestrator's bus pin list (which appends the latch in | ||
| // shift mode — the latch is a bus lane) and at the shift-mode pclk, so the test transmits | ||
| // exactly what the render loop does. In direct mode both reduce to today's behaviour. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use American English spelling in comments. These changed comment lines use British spellings, violating the American-English guideline (the same class of issue was flagged and fixed earlier in this PR).
src/light/drivers/MultiPinLedDriver.h#L240-L240: change "behaviour" to "behavior".src/light/drivers/ParallelLedDriver.h#L337-L337: change "labelled" to "labeled".test/unit/light/unit_MoonLedDriver.cpp#L33-L33: change "honours" to "honors".
As per coding guidelines: "Use American English spelling, including identifiers, ... comments, and documentation."
📍 Affects 3 files
src/light/drivers/MultiPinLedDriver.h#L240-L240(this comment)src/light/drivers/ParallelLedDriver.h#L337-L337test/unit/light/unit_MoonLedDriver.cpp#L33-L33
🤖 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` at 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.”
Source: Coding guidelines
Comment on lines
+1325
to
1341
| "type": "ParallelLedDriver", | ||
| "id": "ParallelLed", | ||
| "parent_id": "Drivers", | ||
| "controls": { | ||
| "peripheral": "i80", | ||
| "pins": "9,10", | ||
| "pinExpander": true, | ||
| "latchPin": 46, | ||
| "clockPin": 3, | ||
| "ledsPerPin": "96", | ||
| "dcPin": 21, | ||
| "doubleBuffer": false | ||
| "doubleBuffer": false, | ||
| "loopbackTest": true, | ||
| "loopbackStrand": 8, | ||
| "loopbackRxPin": 16 | ||
| } | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
loopbackTest: true ships enabled by default on the "hpwit shift-register 15" device profile — disables all normal LED output.
This is newly added in this diff (not a carried-over setting, unlike every other field in this entry). Per ParallelLedDriver::tick()'s contract ("Loopback mode owns the peripheral EXCLUSIVELY... the render loop must not transmit"), a device provisioned from this catalog entry boots with the render path permanently short-circuited (if (loopbackTest) return;) until someone manually finds and disables loopbackTest — which the PR restricts to expert mode, making it even less discoverable. No other device profile in this catalog (including the clearly test-only "MM testbench P4", which only sets the loopback pins, not the test flag) ships with this enabled.
🐛 Proposed fix
- "doubleBuffer": false,
- "loopbackTest": true,
- "loopbackStrand": 8,
- "loopbackRxPin": 16
+ "doubleBuffer": false,
+ "loopbackStrand": 8,
+ "loopbackRxPin": 16📝 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
| "type": "ParallelLedDriver", | |
| "id": "ParallelLed", | |
| "parent_id": "Drivers", | |
| "controls": { | |
| "peripheral": "i80", | |
| "pins": "9,10", | |
| "pinExpander": true, | |
| "latchPin": 46, | |
| "clockPin": 3, | |
| "ledsPerPin": "96", | |
| "dcPin": 21, | |
| "doubleBuffer": false | |
| "doubleBuffer": false, | |
| "loopbackTest": true, | |
| "loopbackStrand": 8, | |
| "loopbackRxPin": 16 | |
| } | |
| }, | |
| "type": "ParallelLedDriver", | |
| "id": "ParallelLed", | |
| "parent_id": "Drivers", | |
| "controls": { | |
| "peripheral": "i80", | |
| "pins": "9,10", | |
| "pinExpander": true, | |
| "latchPin": 46, | |
| "clockPin": 3, | |
| "ledsPerPin": "96", | |
| "dcPin": 21, | |
| "doubleBuffer": false, | |
| "loopbackStrand": 8, | |
| "loopbackRxPin": 16 | |
| } | |
| }, |
🤖 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 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.
Source: Path instructions
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
Fixes the MoonI80 whole-frame stall and lands a batch of LED-driver hardening.
The stall: in whole-frame (non-expander) mode the MoonI80 driver dropped to ~5 FPS after a second — a lost/coalesced DMA end-of-frame interrupt left the bus marked
busyforever, and only the ring path had recovery. Both paths now share one stall-recovery routine (finalizeStalledTransfer), so a lost interrupt self-heals on either. This is a net-subtraction: the recovery lives once, not duplicated per path.Changes
Core
finalizeStalledTransferis the single stop-and-clear both the ring and whole-frame wait-timeout backstops call; added the whole-frame backstop the driver lacked. It 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 can't hand out a spuriousdonetoken. Helper lives in the file's anonymous namespace.Light domain
frameFitsDmaBudget+ admaBudgetBytesCRTP hook — 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; thepinExpandercontrol is hidden on drivers that don't support it.dmaBudgetBytesreports the classic-ESP32 i80 (I2S, internal-RAM-only) budget so the frame-fit gate engages there; PSRAM-capable chips and the streaming ring report no bound.loopbackTestis expert-mode only.prepare(), so a small grid settles in ~1s instead of ~10s.Tests
Docs
Verification
Not verified here (product-owner, on the expander wall)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation