LED drivers (RMT / LCD_CAM / Parlio) + multi-protocol net I/O + P4 + audio input

[ewowi]

The LED-driver + multi-platform output increment: WS2812 across three peripherals (RMT, S3 LCD_CAM i80, P4 Parlio), multi-protocol network I/O (Art-Net / E1.31 / DDP both directions), ESP32-P4 board support, and an audio-reactive input module (I²S mic + esp-dsp FFT). Built contract-first → test-first → implement-to-green throughout, each peripheral proven on real hardware (CI encoder test + on-chip loopback self-test + a real strip/panel).

What's in it

LED drivers — three peripherals, one shared encoder

• RmtLedDriver (classic / S3 / P4): multi-pin WS2812 over the RMT peripheral, one TX channel per strand.
• LcdLedDriver (S3): 8-lane parallel WS2812 over the LCD_CAM i80 bus, one autonomous DMA transfer.
• ParlioLedDriver (P4): 1–8-lane parallel WS2812 over the Parlio peripheral.
• ParallelLedDriver (CRTP base): the LCD + Parlio drivers were ~250 of ~370 lines byte-identical; a CRTP static-polymorphism base now holds the one copy (no vtable, hot-path-safe), with each derived supplying ~6 peripheral hooks. Status-lifecycle shared up to DriverBase.
• The slot/symbol encoders stay domain-side (RmtSymbol.h, LcdSlots.h) so they're host-testable with no ESP32. Each driver has an on-device loopback self-test that bit-verifies the wire signal (RMT-RX capture, transmitter-agnostic), shared via detail::captureAndVerifyFrame.

Multi-protocol network I/O

• NetworkSendDriver: streams the buffer over UDP as Art-Net, E1.31/sACN, or DDP (one node, protocol selected by control). Compatible with Falcon/Advatek/xLights/LedFx.
• NetworkReceiveEffect: binds all three ports at once and writes received frames into the layer buffer — the receive side for Resolume/Madrix/xLights.

ESP32-P4 support — P4-NANO board (Ethernet-only landed; WiFi-via-C6-coprocessor scaffolded, C6 slave-firmware provisioning tracked in backlog). All three LED peripherals coexist on the P4, each driver auto-wired off its SOC-capability gate.

Audio-reactive input — AudioModule reads an INMP441-class I²S mic and publishes an AudioFrame (RMS level + 16-band log spectrum + dominant peak) via esp-dsp's float FFT; AudioVolumeEffect / AudioSpectrumEffect consume it through a static accessor (add/remove in any order). Designed fresh from the datasheet + textbook DSP (DC-blocker, Hann window, RMS, geometric bands), not traced from prior projects.

Pin-default safety — user-soldered pins (LED strands, mic) default unset: adding a driver or the mic never grabs a GPIO the user wired elsewhere ("assign a default only when it cannot do harm"). Peripheral-/chip-fixed pins (i80 WR/DC, the Ethernet map) keep their defaults. This fixed a classic-ESP32 boot-loop (auto-wired mic → blocking I²S init on a mic-less board).

Methodology

Every peripheral followed lock the contract → write the proof as a failing (red) test → implement to green → prove on hardware. Success is a verified red→green transition plus an on-device loopback going green, not an opinion. The CRTP dedup was proven zero-overhead by an A/B tick measurement on the same S3.

Testing

• Unit: encoder contracts, lane slicing, frame-byte math, lifecycle (buffer ownership, setup/teardown, empty-pin idle), audio DSP + module lifecycle. Host-run in CI.
• Scenario: full-pipeline add/configure/remove mutation incl. the audio producer/consumer pair (remove the producer while a consumer is live → keeps rendering on silence).
• Hardware: RMT on classic, LCD_CAM on S3, Parlio on P4 — each loopback bit-perfect, real strips/panels lit.

Reviews

CodeRabbit findings processed across the branch (line-level bugs). The final commit processed the latest batch (7 fixed, 3 accepted-with-reason — see commit body). Event-2 Reviewer pass over the whole branch diff run at merge.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c3b5d17d-5d04-4fb6-9037-36673c021627

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch led-driver-rmt

[coderabbitai]

Actionable comments posted: 5

🤖 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/RmtLedDriver.h`:
- Around line 48-49: teardown()/deinit() currently never releases the heap
allocated by resizeSymbols(): free symbols_ during shutdown by deleting or
freeing the buffer and resetting symbols_ to nullptr inside deinit() (or
teardown() if you prefer teardown() to call the cleanup) to avoid retained
memory; also add a guard in resizeSymbols() to release any existing symbols_
before reallocating and ensure any code paths that replace symbols_ (e.g.,
reinitialization) likewise delete the old buffer to prevent leaks (refer to the
symbols_, resizeSymbols(), teardown(), and deinit() symbols to locate the
changes).

In `@src/platform/esp32/platform_esp32_rmt.cpp`:
- Around line 173-184: The rmtWs2812RxCapture path can call
rmt_del_channel(rxChan) without first disabling the channel if
rmt_enable(rxChan) succeeds but rmt_receive(...) fails; modify the control flow
so that whenever rmt_enable(rxChan) returns ESP_OK you always call
rmt_disable(rxChan) before rmt_del_channel(rxChan) (even on rmt_receive failure
or timeout). Specifically, after calling rmt_enable(rxChan) and before any early
exits, ensure rmt_disable(rxChan) is invoked (or use a boolean flag like enabled
= true and in cleanup check enabled to call rmt_disable), then proceed to
vQueueDelete(q) and rmt_del_channel(rxChan); keep existing xQueueReceive and got
logic intact.
- Around line 96-98: rmtWs2812Show currently ignores
rmt_transmit/rmt_tx_wait_all_done return values and waits forever (-1); change
it to check the return of rmt_transmit and call rmt_tx_wait_all_done with a
finite timeout (configurable constant or parameter) and handle timeout/error
returns by cancelling the in-flight transfer (call rmt_tx_stop(st->channel) and
ensure the channel is quiesced) before returning an error; update the function
signature/return value so callers (e.g., RmtLedDriver::loop()) can detect
failure and avoid overwriting symbols_ while a transmit is still in-flight, and
ensure symbols_ is only reused after a successful completion or after the cancel
path has fully completed.

In `@test/device/device_RmtLoopback.cpp`:
- Around line 99-106: Replace the volatile polling and unchecked task creation
with a proper RTOS synchronization and lifetime handling: change the local Cap/
cap usage so rxTask signals the creator with a task notification or event
group/queue (reference the rxTask function and mm::platform::rmtWs2812RxCapture
call), check the return value of xTaskCreate and handle failure, and make the
creating task wait for the notification (or use e.g. ulTaskNotifyTake/notify or
an event group) before returning so the RX task cannot reference the stack after
the creator exits; ensure the RX task deletes itself only after sending the
notification and that the creator joins/waits for that signal before using cap
results.

In `@test/unit/light/unit_RmtLedEncoder.cpp`:
- Around line 54-68: The test claims multi-light ordering but only asserts
symbols for the first light; either expand assertions to cover the second light
or rename the test to per-light behavior. To expand, after the existing checks
add assertions for light 1 at per-light offset = channels*8 = 3*8 = 24 symbols:
checkBit(out[24], T0H) and checkBit(out[31], T0H) for light1 byte0 MSB/LSB
(0x00), and checkBit(out[40], T1H) for light1 byte2 MSB (0xFF); use the same
mm::encodeWs2812Symbols/wire/out/checkBit symbols so ordering is fully
validated. Alternatively, rename the TEST_CASE string to indicate it only
verifies a single light’s encoding.

🪄 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: 7f408553-9d62-40cb-b72e-f8d49ddf39f7

📥 Commits

Reviewing files that changed from the base of the PR and between 5a0d5df and 7386bd0.

⛔ Files ignored due to path filters (1)

• scripts/build/build_esp32.py is excluded by !**/build/**

📒 Files selected for processing (17)

• docs/backlog/leddriver-increment-1-plan.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• esp32/main/CMakeLists.txt
• esp32/main/main.cpp
• src/light/drivers/Drivers.h
• src/light/drivers/LedDriverConfig.h
• src/light/drivers/RmtLedDriver.h
• src/light/drivers/RmtSymbol.h
• src/main.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• test/CMakeLists.txt
• test/device/device_RmtLoopback.cpp
• test/unit/light/unit_RmtLedEncoder.cpp

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (1)

src/platform/esp32/platform_esp32_rmt.cpp (1)

96-105: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Propagate TX failures instead of falling through after a failed wait.

Line 104 still ignores rmt_transmit(...), and Line 105 ignores rmt_tx_wait_all_done(...). If submission fails or the 1 s wait times out, this function returns without cancelling/quiescing the channel, so the caller can reuse or overwrite symbols while the prior transfer is still in flight. This needs an explicit failure path that aborts/re-enables the channel and reports failure back to the caller.

🤖 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_rmt.cpp` around lines 96 - 105, Check and
propagate errors from rmt_transmit(...) and rmt_tx_wait_all_done(...): capture
their return values and if either indicates failure or timeout, abort the
in-flight transfer on st->channel (e.g. stop/disable/clear the channel or DMA
for that channel, reinitialize or re-enable it so it is no longer actively using
symbols), ensure any resources/flags that indicate an ongoing transmit are
cleared, and return a failure status to the caller so it won't reuse/overwrite
symbols while the previous transfer remains active; modify the function around
rmt_transmit, rmt_tx_wait_all_done, st->channel, st->encoder, symbols,
symbolCount, and txCfg to implement this error-path and propagate the error.

🤖 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/RmtLedDriver.md`:
- Around line 22-23: The docs state gpio and loopbackRxPin are uint8_t but the
implementation in RmtLedDriver.h defines both controls as uint16_t; update the
documentation to reflect the actual control types by changing the types for
`gpio` and `loopbackRxPin` to uint16_t so they match the implementation (and UI
behavior), or alternatively change the implementation in RmtLedDriver.h
(symbols: gpio, loopbackRxPin) to uint8_t if the intent is to use 8-bit
types—ensure the type in the docs and the type in RmtLedDriver.h are identical.

In `@src/light/drivers/RmtLedDriver.h`:
- Around line 245-249: reinit() currently calls platform::rmtWs2812Init and
assigns its result directly to inited_, which silently disables output on
failure; change it so that a failed platform::rmtWs2812Init does not silently
turn off output but instead surfaces the failure (e.g., log an error/record an
error status or return a failure bool) so callers and loop() can report why
output died. Update reinit() (and if necessary deinit()) to detect
platform::rmtWs2812Init failure, emit a clear diagnostic (using your project
logger or an error/status field), and return or set an explicit error state
rather than just flipping inited_. Ensure references to inited_, reinit(),
platform::rmtWs2812Init, and loop() are updated so loop() can observe and report
the init failure.
- Around line 8-10: Add a direct `#include` <cstring> to RmtLedDriver.h so use of
std::strcmp is valid and the header is self-contained; then update the
initialization path in RmtLedDriver (where platform::rmtWs2812Init(...) is
called and inited_ is set) to detect a failed init and surface that failure to
users—set inited_ accordingly and emit a visible status/log message or expose
the error via the existing status string/loopback result instead of silently
no-oping in loop(), so callers and logs can observe the rmtWs2812Init failure
during startup or status reporting.

---

Duplicate comments:
In `@src/platform/esp32/platform_esp32_rmt.cpp`:
- Around line 96-105: Check and propagate errors from rmt_transmit(...) and
rmt_tx_wait_all_done(...): capture their return values and if either indicates
failure or timeout, abort the in-flight transfer on st->channel (e.g.
stop/disable/clear the channel or DMA for that channel, reinitialize or
re-enable it so it is no longer actively using symbols), ensure any
resources/flags that indicate an ongoing transmit are cleared, and return a
failure status to the caller so it won't reuse/overwrite symbols while the
previous transfer remains active; modify the function around rmt_transmit,
rmt_tx_wait_all_done, st->channel, st->encoder, symbols, symbolCount, and txCfg
to implement this error-path and propagate the error.

🪄 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: d46e365d-67a0-4ee1-91e4-70ff3905654d

📥 Commits

Reviewing files that changed from the base of the PR and between 7386bd0 and 6c3e269.

📒 Files selected for processing (16)

• docs/backlog/backlog.md
• docs/backlog/leddriver-increment-1-plan.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• esp32/main/CMakeLists.txt
• src/core/HttpServerModule.cpp
• src/core/NetworkModule.h
• src/light/drivers/RmtLedDriver.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• src/ui/app.js
• test/CMakeLists.txt
• test/unit/core/conditional_controls.h
• test/unit/core/unit_NetworkModule.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp
• test/unit/light/unit_RmtLedEncoder.cpp

💤 Files with no reviewable changes (1)

• esp32/main/CMakeLists.txt

[coderabbitai]

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 (1)

src/light/drivers/RmtLedDriver.h (1)

215-231: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve init-failure status after loopback reinit.

After Line 215, reinit() can set kInitFailMsg, but Lines 216-231 then overwrite status with loopback PASS/FAIL/jumper results. That masks the real failure while loop() is disabled (!inited_), which is misleading.

Suggested fix

     deinit();
     const auto r = platform::rmtWs2812Loopback(gpio, loopbackRxPin);
     reinit();
+    if (!inited_) {
+        // reinit() already published the init-failure status.
+        return;
+    }
     if (!r.jumperDetected) {
         clearFailBuf();
         setStatus("loopback: jumper not detected", Severity::Warning);

🤖 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 215 - 231, reinit() can set
the initial failure status (kInitFailMsg) but the subsequent loopback logic (in
the block that calls clearFailBuf(), setStatus(...) based on r.jumperDetected /
r.pass / failBuf_) overwrites it; update the loopback handling in the function
containing reinit() and the loopback block so that if inited_ is false or
current status equals kInitFailMsg you skip/return early and do not call
setStatus for loopback results—i.e., check inited_ or compare against
kInitFailMsg before executing the jumper/pass/fail setStatus logic (leave
failBuf_ allocation and messaging unchanged for real loopback failures when
inited_ is true).

🤖 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/RmtLedDriver.md`:
- Line 24: The documentation for RmtLedDriver incorrectly states that
loopbackTest is a one-shot auto-reset flag; update the docs to match the
implementation by describing loopbackTest as a persistent mode on RmtLedDriver
that, while true, reruns the RMT TX→RX loopback test whenever relevant controls
change and only stops when loopbackTest is cleared; mention that results are
written to the module's status field and that the flag must be manually reset to
disable repeated runs.

In `@docs/moonmodules/light/effects/RingsEffect.md`:
- Line 16: The docs and code disagree on the allowed range for the "thickness"
control: the docs say 1-16 but the code call controls_.addUint8("thickness",
thickness, 1, 255) (see RingsEffect controls setup). Fix by either updating the
documentation to state the actual 1-255 range, or change the control definition
in src/light/effects/RingsEffect.h to clamp the max to 16 (e.g., change the
addUint8 max from 255 to 16 and ensure any internal uses of thickness respect
that range); reference the controls_.addUint8("thickness", ...) invocation and
the RingsEffect control handling when making the change.

In `@docs/tests/unit-tests.md`:
- Line 72: The test description line "RipplesEffect spatial variation" is a
fragment and must be completed or removed; update the tests documentation by
either expanding that entry into a full sentence explaining what the
RipplesEffect spatial variation test verifies (e.g., expected behavior, inputs,
and assertions) or delete the placeholder line so it matches the surrounding
complete descriptions; search for the exact phrase "RipplesEffect spatial
variation" to locate the entry to edit.

In `@scripts/docs/screenshot_modules.py`:
- Around line 361-364: The three print calls that currently use f-strings
without any interpolation should be changed to plain string literals: locate the
print statements that start with print(f"      The running binary..."), print(f"
on :8080. Fix:  pkill -f projectMM  then rebuild + run ONE server:"), and
print(f"        cmake --build build -j && ./build/projectMM") in
screenshot_modules.py and remove the leading f from each string so they become
normal print("...") calls (no other changes).

---

Outside diff comments:
In `@src/light/drivers/RmtLedDriver.h`:
- Around line 215-231: reinit() can set the initial failure status
(kInitFailMsg) but the subsequent loopback logic (in the block that calls
clearFailBuf(), setStatus(...) based on r.jumperDetected / r.pass / failBuf_)
overwrites it; update the loopback handling in the function containing reinit()
and the loopback block so that if inited_ is false or current status equals
kInitFailMsg you skip/return early and do not call setStatus for loopback
results—i.e., check inited_ or compare against kInitFailMsg before executing the
jumper/pass/fail setStatus logic (leave failBuf_ allocation and messaging
unchanged for real loopback failures when inited_ is true).

🪄 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: 8d9ab62c-f382-4556-a867-f2f946ed405c

📥 Commits

Reviewing files that changed from the base of the PR and between 6c3e269 and 1b14bee.

⛔ Files ignored due to path filters (11)

• docs/assets/screenshots/CheckerboardModifier.gif is excluded by !**/*.gif
• docs/assets/screenshots/CheckerboardModifier.png is excluded by !**/*.png
• docs/assets/screenshots/GameOfLifeEffect.gif is excluded by !**/*.gif
• docs/assets/screenshots/GameOfLifeEffect.png is excluded by !**/*.png
• docs/assets/screenshots/MultiplyModifier.gif is excluded by !**/*.gif
• docs/assets/screenshots/MultiplyModifier.png is excluded by !**/*.png
• docs/assets/screenshots/RingsEffect.gif is excluded by !**/*.gif
• docs/assets/screenshots/RingsEffect.png is excluded by !**/*.png
• docs/assets/screenshots/RipplesEffect.gif is excluded by !**/*.gif
• docs/assets/screenshots/RipplesEffect.png is excluded by !**/*.png
• scripts/build/flash_esp32.py is excluded by !**/build/**

📒 Files selected for processing (21)

• .github/workflows/release.yml
• docs/backlog/backlog.md
• docs/landing/index.html
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/GameOfLifeEffect.md
• docs/moonmodules/light/effects/RingsEffect.md
• docs/moonmodules/light/effects/RipplesEffect.md
• docs/moonmodules/light/modifiers/CheckerboardModifier.md
• docs/moonmodules/light/modifiers/MultiplyModifier.md
• docs/performance.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/docs/screenshot_modules.py
• src/light/drivers/RmtLedDriver.h
• src/light/effects/RingsEffect.h
• src/light/effects/RipplesEffect.h
• src/main.cpp
• test/scenario_runner.cpp
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/unit/light/unit_CheckerboardEffect.cpp
• test/unit/light/unit_Layer_zero_grid.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Documentation-code mismatch: thickness range.

The documentation states thickness range is 1-16, but the code at src/light/effects/RingsEffect.h:29 declares controls_.addUint8("thickness", thickness, 1, 255). Either update the docs to reflect the 1-255 range or clamp the control range in code to 1-16 if that's the intended visual constraint.

🤖 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/effects/RingsEffect.md` at line 16, The docs and code
disagree on the allowed range for the "thickness" control: the docs say 1-16 but
the code call controls_.addUint8("thickness", thickness, 1, 255) (see
RingsEffect controls setup). Fix by either updating the documentation to state
the actual 1-255 range, or change the control definition in
src/light/effects/RingsEffect.h to clamp the max to 16 (e.g., change the
addUint8 max from 255 to 16 and ensure any internal uses of thickness respect
that range); reference the controls_.addUint8("thickness", ...) invocation and
the RingsEffect control handling when making the change.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove extraneous f prefixes from strings without placeholders.

Five print statements use f-string syntax but contain no {…} interpolation. Static analysis (Ruff F541) flags these correctly. Use plain strings instead.

🧰 Tools

🪛 Ruff (0.15.15)

[error] 361-361: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

[error] 362-362: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

[error] 363-363: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

[error] 364-364: f-string without any placeholders

Remove extraneous f prefix

(F541)

🤖 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 `@scripts/docs/screenshot_modules.py` around lines 361 - 364, The three print
calls that currently use f-strings without any interpolation should be changed
to plain string literals: locate the print statements that start with print(f"  
The running binary..."), print(f"      on :8080. Fix:  pkill -f projectMM  then
rebuild + run ONE server:"), and print(f"        cmake --build build -j &&
./build/projectMM") in screenshot_modules.py and remove the leading f from each
string so they become normal print("...") calls (no other changes).

Source: Linters/SAST tools

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

test/unit/light/unit_RmtLedDriver_lifecycle.cpp (1)

27-27: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Root cause: helper setup does not assert buffer allocation success. Both wire(...) helpers proceed after src.allocate(...) without checking the result, which can mask setup failures and produce misleading downstream assertions.

🤖 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_RmtLedDriver_lifecycle.cpp` at line 27, The helper setup
calls src.allocate(lights, 3) but does not assert its return value, so
allocation failures can be masked by subsequent wire(...) helpers; update the
test setup to check the result of src.allocate (e.g., assertTrue/assert that it
returns success) immediately after calling src.allocate(lights, 3) and fail the
test if allocation fails so downstream wire(...) calls only run when the buffer
is actually allocated.

src/platform/esp32/platform_esp32_rmt.cpp (1)

238-294: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Static rxSymbols buffer is not thread-safe for concurrent calls.

The static uint32_t rxSymbols[kCapMax] at line 263 would cause data races if rmtWs2812Loopback were called concurrently from multiple threads. Since this is a test-only function invoked from a UI control, concurrent calls are unlikely but possible if a user rapidly triggers the test.

Consider either documenting this limitation or adding a guard (e.g., a static flag to reject concurrent calls).

🤖 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_rmt.cpp` around lines 238 - 294, The static
rxSymbols buffer in rmtWs2812Loopback (static uint32_t rxSymbols[kCapMax]) can
race if rmtWs2812Loopback is called concurrently; fix by removing the static
buffer and instead allocate the capture buffer per-call and pass its pointer
through the Cap struct into the rxTask (or alternatively add a static
atomic_flag/mutex guard inside rmtWs2812Loopback to reject/serialize concurrent
calls), update Cap to hold a uint32_t* buffer and capacity, set cap.buffer = new
uint32_t[kCapMax] (or use a stack/std::vector if allowed), and ensure the rxTask
and caller use that per-call buffer and free it after rmtWs2812Deinit to avoid
leaks.

♻️ Duplicate comments (1)

docs/tests/unit-tests.md (1)

56-56: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Complete the incomplete test description.

The line "RipplesEffect spatial variation" is a fragment without explanation. This was flagged in a previous review and remains unresolved. Every other test description in this file is either a complete sentence or paired with explanatory text.

Either expand this into a complete sentence explaining what the test verifies (e.g., "RipplesEffect shows spatial variation across the grid with different pixels at different positions"), or remove the placeholder if the description is redundant with the line above.

🤖 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/tests/unit-tests.md` at line 56, The test description line
"RipplesEffect spatial variation" is an incomplete fragment; update it to a
complete sentence that explains what the test verifies (for example:
"RipplesEffect shows spatial variation across the grid, producing different
pixel values at different positions"), or remove the placeholder if it
duplicates the previous line; edit the line in docs/tests/unit-tests.md so the
test description is a full, self-contained sentence.

🤖 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 `@scripts/scenario/_preview_ws.py`:
- Line 97: Remove the redundant socket.timeout from the exception tuple and
catch only TimeoutError; locate the except clause that currently reads "except
(TimeoutError, socket.timeout):" in scripts/scenario/_preview_ws.py and replace
it with a single "except TimeoutError:" so you don't redundantly handle the same
exception alias.

In `@test/unit/light/unit_NetworkReceiveEffect_protocols.cpp`:
- Around line 252-287: The test relies on real mm::platform::UdpSocket
round-trips and timing; replace the network-dependent sends and polling with
direct packet injection so the test is deterministic. Remove creation/connection
of artTx/e131Tx/ddpTx and their sendTo calls and instead build packets with
mm::buildArtDmxPacket / mm::buildE131Packet / mm::buildDdpPacket and pass them
directly to the NetworkReceiveEffect processing entrypoint (e.g., call the layer
or NetworkReceiveEffect method that accepts a packet buffer such as
r.layer.loop/receivePacket/processPacket or a mocked receivePacket function) to
simulate arrival on the correct protocol/port; also eliminate delayMs polling by
invoking the processing synchronously and then assert the buffer contents via
r.layer.buffer().data() and r.fx.status(). Ensure any socket class usage in the
test (mm::platform::UdpSocket) is removed or replaced by a lightweight mock that
calls the same packet handler.

---

Outside diff comments:
In `@src/platform/esp32/platform_esp32_rmt.cpp`:
- Around line 238-294: The static rxSymbols buffer in rmtWs2812Loopback (static
uint32_t rxSymbols[kCapMax]) can race if rmtWs2812Loopback is called
concurrently; fix by removing the static buffer and instead allocate the capture
buffer per-call and pass its pointer through the Cap struct into the rxTask (or
alternatively add a static atomic_flag/mutex guard inside rmtWs2812Loopback to
reject/serialize concurrent calls), update Cap to hold a uint32_t* buffer and
capacity, set cap.buffer = new uint32_t[kCapMax] (or use a stack/std::vector if
allowed), and ensure the rxTask and caller use that per-call buffer and free it
after rmtWs2812Deinit to avoid leaks.

In `@test/unit/light/unit_RmtLedDriver_lifecycle.cpp`:
- Line 27: The helper setup calls src.allocate(lights, 3) but does not assert
its return value, so allocation failures can be masked by subsequent wire(...)
helpers; update the test setup to check the result of src.allocate (e.g.,
assertTrue/assert that it returns success) immediately after calling
src.allocate(lights, 3) and fail the test if allocation fails so downstream
wire(...) calls only run when the buffer is actually allocated.

---

Duplicate comments:
In `@docs/tests/unit-tests.md`:
- Line 56: The test description line "RipplesEffect spatial variation" is an
incomplete fragment; update it to a complete sentence that explains what the
test verifies (for example: "RipplesEffect shows spatial variation across the
grid, producing different pixel values at different positions"), or remove the
placeholder if it duplicates the previous line; edit the line in
docs/tests/unit-tests.md so the test description is a full, self-contained
sentence.

🪄 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: 1b54c4d6-cbf4-45b9-a30e-79219964272f

📥 Commits

Reviewing files that changed from the base of the PR and between 1b14bee and f15eb01.

⛔ Files ignored due to path filters (1)

• docs/assets/screenshots/NetworkSendDriver.png is excluded by !**/*.png

📒 Files selected for processing (52)

• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/backlog/moonmodules_draft/light/effects/ArtNetReceiveEffect.md
• docs/moonmodules/core/FilesystemModule.md
• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/moonmodules/light/drivers/NetworkSendDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/NetworkReceiveEffect.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/MoonDeck.md
• scripts/docs/screenshot_modules.py
• scripts/moondeck_config.json
• scripts/scenario/_preview_ws.py
• scripts/scenario/run_network_live.py
• src/light/ArtNetPacket.h
• src/light/DdpPacket.h
• src/light/E131Packet.h
• src/light/drivers/Correction.h
• src/light/drivers/NetworkSendDriver.h
• src/light/drivers/RmtLedDriver.h
• src/light/effects/NetworkReceiveEffect.h
• src/main.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_resize.json
• test/scenarios/light/scenario_Layer_base_pipeline.json
• test/scenarios/light/scenario_Layer_buildup.json
• test/scenarios/light/scenario_Layer_memory_1to1.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/scenarios/light/scenario_MultiplyModifier_memory_lut.json
• test/scenarios/light/scenario_MultiplyModifier_pipeline.json
• test/scenarios/light/scenario_modifier_swap.json
• test/unit/light/unit_ArtNetSendDriver_packet.cpp
• test/unit/light/unit_NetworkReceiveEffect.cpp
• test/unit/light/unit_NetworkReceiveEffect_protocols.cpp
• test/unit/light/unit_NetworkSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_NetworkSendDriver_packet.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp

💤 Files with no reviewable changes (3)

• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/backlog/moonmodules_draft/light/effects/ArtNetReceiveEffect.md
• test/unit/light/unit_ArtNetSendDriver_packet.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Move the localhost socket round-trip out of unit tests.

Line 252 to Line 287 depends on real UDP sockets and timing (delayMs polling), which makes this unit case nondeterministic and environment-sensitive.
As per coding guidelines, "test/**: Unit and integration tests using doctest. Verify tests cover edge cases and match the specifications in docs/moonmodules/. Tests should not depend on timing or network."

🧰 Tools

🪛 Clang (14.0.6)

[warning] 252-252: function 'DOCTEST_ANON_FUNC_20' has cognitive complexity of 38 (threshold 25)

(readability-function-cognitive-complexity)

---

[note] 255-255: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 255-255: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 255-255: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 261-261: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 261-261: +1

(clang)

---

[note] 261-261: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 261-261: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 262-262: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 262-262: +1

(clang)

---

[note] 262-262: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 262-262: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 263-263: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 263-263: +1

(clang)

---

[note] 263-263: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 263-263: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 274-274: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 274-274: +1

(clang)

---

[note] 277-277: +1

(clang)

---

[note] 278-278: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 280-280: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 280-280: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 280-280: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 281-281: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 281-281: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 281-281: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[warning] 253-253: variable name 'r' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 257-257: do not declare C-style arrays, use std::array<> instead

(modernize-avoid-c-arrays)

---

[warning] 260-260: multiple declarations in a single statement reduces readability

(readability-isolate-declaration)

---

[warning] 267-267: do not declare C-style arrays, use std::array<> instead

(modernize-avoid-c-arrays)

---

[warning] 267-267: multiple declarations in a single statement reduces readability

(readability-isolate-declaration)

---

[warning] 267-267: variable name 'a' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 267-267: variable name 'e' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 267-267: variable name 'd' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 268-268: do not declare C-style arrays, use std::array<> instead

(modernize-avoid-c-arrays)

---

[warning] 278-278: statement should be inside braces

(readability-braces-around-statements)

🪛 Cppcheck (2.20.0)

[error] 269-269: Uninitialized variable

(uninitvar)

---

[style] 263-263: The function 'replaceChildAt' is never used.

(unusedFunction)

---

[style] 275-275: The function 'moveChildTo' is never used.

(unusedFunction)

🤖 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_NetworkReceiveEffect_protocols.cpp` around lines 252 -
287, The test relies on real mm::platform::UdpSocket round-trips and timing;
replace the network-dependent sends and polling with direct packet injection so
the test is deterministic. Remove creation/connection of artTx/e131Tx/ddpTx and
their sendTo calls and instead build packets with mm::buildArtDmxPacket /
mm::buildE131Packet / mm::buildDdpPacket and pass them directly to the
NetworkReceiveEffect processing entrypoint (e.g., call the layer or
NetworkReceiveEffect method that accepts a packet buffer such as
r.layer.loop/receivePacket/processPacket or a mocked receivePacket function) to
simulate arrival on the correct protocol/port; also eliminate delayMs polling by
invoking the processing synchronously and then assert the buffer contents via
r.layer.buffer().data() and r.fx.status(). Ensure any socket class usage in the
test (mm::platform::UdpSocket) is removed or replaced by a lightweight mock that
calls the same packet handler.

Source: Coding guidelines

[coderabbitai]

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)

test/scenarios/light/scenario_AllEffects_grid_sizes.json (1)

26-26: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale driver reference in scenario description.

Line 26 still says the scenario rebuilds with “ArtNet”, but the fixture/build now wires NetworkSendDriver (Line 84, Line 159). Please align the description with the actual module setup to keep test intent unambiguous.

🤖 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_AllEffects_grid_sizes.json` at line 26, Update
the scenario description string to reflect the current driver name: replace the
outdated "ArtNet" reference with "NetworkSendDriver" so the prose matches the
actual setup where the fixture wires NetworkSendDriver; keep other terms like
PreviewDriver, clear_children, replace_module and set_control unchanged to
preserve behavior description and clarify the test intent.

🤖 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/install/install-orchestrator.js`:
- Around line 183-186: sendSetTxPowerFrame (and the other spots that construct a
frame with IMPROV_CMD_SET_TX_POWER) currently coerces dBm with a bitwise
operation (dBm & 0xFF) which silently accepts invalid inputs; update these
places to validate and normalize txPower first: coerce input using
Number(txPower), ensure Number.isInteger(value) and that it falls within the
allowed bounds (e.g. 0–255 or the device-specific min/max), and if invalid
reject/throw or return an error before building the frame instead of encoding a
bad byte; reference sendSetTxPowerFrame and IMPROV_CMD_SET_TX_POWER to find all
occurrences to fix.

In `@scripts/moondeck.py`:
- Around line 1048-1057: The fallback to derive a board via _deduce_board()
never fires under the current pass_board contract (firmware is not supplied), so
change the pass_board handling to require an explicit board: in the pass_board
branch stop calling _deduce_board(params.get("firmware", "")) and only use
params.get("board"); if board is missing emit a clear warning/error (or raise)
so --board is not silently omitted; update any tests/docs referencing pass_board
behavior accordingly.

In `@src/light/drivers/LcdLedDriver.h`:
- Around line 3-8: LcdLedDriver.h currently pulls in platform/platform.h and
performs hardware-facing lifecycle operations from the non-platform layer;
remove the direct platform include and any direct hardware calls from
LcdLedDriver.h and instead call into platform-abstraction entry points (e.g.
add/replace calls with platform::initLedHardware(...),
platform::startLedCycle(...), platform::stopLedCycle(...) or similarly named
functions implemented under src/platform) so non-platform code only references
abstract platform APIs; if you need compile-time branching use platform_config.h
flags with if constexpr in the header (or provide a small thin inline wrapper
that dispatches to the platform implementation) and keep functions like
encodeWs2812LcdSlots, DriverBase/Correction usage, and parsePinList/assignCounts
unchanged in the non-platform layer.
- Around line 138-139: The call to platform::lcdWs2812Transmit(lcd_,
frameBytes_) currently ignores its boolean return so a transmit failure is
masked; modify the loop in LcdLedDriver to capture the bool result from
platform::lcdWs2812Transmit and handle failure explicitly (e.g., log an error,
set/clear a driver error state, and break/return/throw to avoid continuing as if
the frame was sent). Use the existing symbols lcdWs2812Transmit, lcdWs2812Wait,
lcd_, and frameBytes_ to locate the code and ensure the handler calls the
appropriate error reporting path in this class (or returns a failure status)
before calling platform::lcdWs2812Wait or retrying.
- Around line 84-87: The code currently calls clearStatus() whenever
isTestControl is true and loopbackTest is false, which clears real driver errors
when loopback is toggled off; fix this by only clearing status when entering
test-control mode (i.e., on the transition into isTestControl) and not on a
loopback toggle—introduce a stored previous flag (e.g., prevIsTestControl) and
change the condition to something like: if (isTestControl && !loopbackTest &&
!prevIsTestControl) { clearFailBuf(); clearStatus(); } then update
prevIsTestControl each cycle so toggling loopbackTest off while already in
test-control does not clear existing non-loopback errors; keep the existing
else-if branch for (loopbackTest && (isTestControl || isPinControl)) untouched.

In `@src/platform/esp32/platform_esp32_lcd.cpp`:
- Around line 167-181: lcdWs2812Wait currently drops the xSemaphoreTake() result
allowing a late DMA completion to leave the semaphore set and let the next wait
return early while the prior transfer still owns st->buf; change lcdWs2812Wait
to return a bool, capture the xSemaphoreTake result, and on timeout (pdFALSE)
drain any late completion tokens by repeatedly calling xSemaphoreTake(st->done,
0) until it returns pdFALSE, then return false so callers can avoid reusing
st->buf; update callers of lcdWs2812Wait to check the returned status before
starting a new lcdWs2812Transmit, and ensure lcdWs2812Transmit either checks the
completion status or is only called when lcdWs2812Wait returned true.

In `@test/scenarios/light/scenario_AllEffects_grid_sizes.json`:
- Around line 4123-4125: Replace future-dated measurement timestamps
"2026-06-11" with the actual run date "2026-06-10" wherever they appear in the
JSON "at" arrays (specifically the entries that currently read
["2026-06-07","2026-06-11"] around the "at" arrays at the two affected records);
update both occurrences referenced (the one near line 4125 and the one near line
4361/4359-4361) so measurement provenance is chronologically consistent.

---

Outside diff comments:
In `@test/scenarios/light/scenario_AllEffects_grid_sizes.json`:
- Line 26: Update the scenario description string to reflect the current driver
name: replace the outdated "ArtNet" reference with "NetworkSendDriver" so the
prose matches the actual setup where the fixture wires NetworkSendDriver; keep
other terms like PreviewDriver, clear_children, replace_module and set_control
unchanged to preserve behavior description and clarify the test intent.

🪄 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: 538746b3-6d4b-457e-9258-ad13561d4ad8

📥 Commits

Reviewing files that changed from the base of the PR and between f15eb01 and b868fa2.

⛔ Files ignored due to path filters (2)

• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/improv_provision.py is excluded by !**/build/**

📒 Files selected for processing (39)

• docs/backlog/leddriver-increment-2-plan.md
• docs/history/decisions.md
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• esp32/main/CMakeLists.txt
• scripts/MoonDeck.md
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/index.html
• scripts/scenario/_preview_ws.py
• src/core/ImprovProvisioningModule.h
• src/core/NetworkModule.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/LcdSlots.h
• src/light/drivers/PinList.h
• src/light/drivers/RmtLedDriver.h
• src/main.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• src/ui/install-picker.js
• test/CMakeLists.txt
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/scenarios/light/scenario_modifier_swap.json
• test/unit/light/unit_CheckerboardEffect.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_LcdLedEncoder.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate txPower before frame encoding.

Line 185 coerces with dBm & 0xFF, so invalid inputs (e.g., "abc", -1, 20.5) become unintended bytes. This can silently bypass or misapply the brownout mitigation path.

Proposed fix

 async function sendSetTxPowerFrame(port, dBm) {
+    if (!Number.isInteger(dBm) || dBm < 0 || dBm > 21) {
+        throw new Error(`txPower must be an integer in 0..21 dBm, got: ${dBm}`);
+    }
     const frame = buildImprovFrame(IMPROV_FRAME_TYPE_RPC,
-                                   new Uint8Array([IMPROV_CMD_SET_TX_POWER, 1, dBm & 0xFF]));
+                                   new Uint8Array([IMPROV_CMD_SET_TX_POWER, 1, dBm]));
@@
             if (txPower != null) {
                 trackProgress("set-tx-power");
+                if (!Number.isInteger(txPower) || txPower < 0 || txPower > 21) {
+                    throw new Error(`invalid txPower: ${txPower} (expected integer 0..21)`);
+                }
                 if (onLog) onLog(`[orchestrator] SET_TX_POWER ${txPower} dBm (boards.json cap)`);
                 await sendSetTxPowerFrame(port, txPower);
                 await new Promise(r => setTimeout(r, 200));
             }

Also applies to: 664-668, 787-791

🤖 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/install/install-orchestrator.js` around lines 183 - 186,
sendSetTxPowerFrame (and the other spots that construct a frame with
IMPROV_CMD_SET_TX_POWER) currently coerces dBm with a bitwise operation (dBm &
0xFF) which silently accepts invalid inputs; update these places to validate and
normalize txPower first: coerce input using Number(txPower), ensure
Number.isInteger(value) and that it falls within the allowed bounds (e.g. 0–255
or the device-specific min/max), and if invalid reject/throw or return an error
before building the frame instead of encoding a bad byte; reference
sendSetTxPowerFrame and IMPROV_CMD_SET_TX_POWER to find all occurrences to fix.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Move hardware-facing lifecycle calls behind src/platform implementation boundaries.

This file directly includes platform/platform.h and performs hardware-facing operations from src/light/.... That breaks the repository’s platform-boundary rule for non-platform code.

As per coding guidelines, "src/**/!(platform)/**/*.{h,cpp}: Platform abstraction: no #ifdef, platform-specific #include, or hardware API call outside src/platform/; use if constexpr on platform_config.h flags for compile-time branching."

Also applies to: 111-140, 240-275

🤖 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/LcdLedDriver.h` around lines 3 - 8, LcdLedDriver.h
currently pulls in platform/platform.h and performs hardware-facing lifecycle
operations from the non-platform layer; remove the direct platform include and
any direct hardware calls from LcdLedDriver.h and instead call into
platform-abstraction entry points (e.g. add/replace calls with
platform::initLedHardware(...), platform::startLedCycle(...),
platform::stopLedCycle(...) or similarly named functions implemented under
src/platform) so non-platform code only references abstract platform APIs; if
you need compile-time branching use platform_config.h flags with if constexpr in
the header (or provide a small thin inline wrapper that dispatches to the
platform implementation) and keep functions like encodeWs2812LcdSlots,
DriverBase/Correction usage, and parsePinList/assignCounts unchanged in the
non-platform layer.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle lcdWs2812Transmit failure explicitly.

Line 138 drops the boolean return from platform::lcdWs2812Transmit. If transmit fails, the loop silently proceeds and only waits, masking a hard output fault.

Proposed fix

-        platform::lcdWs2812Transmit(lcd_, frameBytes_);
+        if (!platform::lcdWs2812Transmit(lcd_, frameBytes_)) {
+            setStatus("LCD transmit failed", Severity::Error);
+            return;
+        }
         platform::lcdWs2812Wait(lcd_, 1000 /* ms */);

📝 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

	platform::lcdWs2812Transmit(lcd_, frameBytes_);
	platform::lcdWs2812Wait(lcd_, 1000 /* ms */);
	if (!platform::lcdWs2812Transmit(lcd_, frameBytes_)) {
	setStatus("LCD transmit failed", Severity::Error);
	return;
	}
	platform::lcdWs2812Wait(lcd_, 1000 /* ms */);

🤖 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/LcdLedDriver.h` around lines 138 - 139, The call to
platform::lcdWs2812Transmit(lcd_, frameBytes_) currently ignores its boolean
return so a transmit failure is masked; modify the loop in LcdLedDriver to
capture the bool result from platform::lcdWs2812Transmit and handle failure
explicitly (e.g., log an error, set/clear a driver error state, and
break/return/throw to avoid continuing as if the frame was sent). Use the
existing symbols lcdWs2812Transmit, lcdWs2812Wait, lcd_, and frameBytes_ to
locate the code and ensure the handler calls the appropriate error reporting
path in this class (or returns a failure status) before calling
platform::lcdWs2812Wait or retrying.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

A timed-out transfer can make the next wait return early while DMA still owns the buffer.

lcdWs2812Wait() drops the xSemaphoreTake() result, and lcdWs2812Transmit() starts the next async transfer without draining any late completion token from st->done. If one frame times out and completes afterward, the callback leaves the semaphore set; the next lcdWs2812Wait() can then succeed immediately and the caller will re-encode into st->buf while the prior transfer is still in flight. This backend needs a real quiesce/error path on timeout, or at minimum a drained completion token plus a returned status the caller can honor before reusing the DMA buffer.

🤖 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_lcd.cpp` around lines 167 - 181,
lcdWs2812Wait currently drops the xSemaphoreTake() result allowing a late DMA
completion to leave the semaphore set and let the next wait return early while
the prior transfer still owns st->buf; change lcdWs2812Wait to return a bool,
capture the xSemaphoreTake result, and on timeout (pdFALSE) drain any late
completion tokens by repeatedly calling xSemaphoreTake(st->done, 0) until it
returns pdFALSE, then return false so callers can avoid reusing st->buf; update
callers of lcdWs2812Wait to check the returned status before starting a new
lcdWs2812Transmit, and ensure lcdWs2812Transmit either checks the completion
status or is only called when lcdWs2812Wait returned true.

[coderabbitai]

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 (2)

scripts/MoonDeck.md (1)

349-349: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use uv run instead of bare python3 in documentation examples.

The example command uses python3 scripts/build/host_wifi.py, but the coding guideline requires all Python invocations to go through uv run. As per coding guidelines, "never type python or python3 directly; always use uv run (applies to shell commands, CMake add_custom_command/execute_process, documentation examples)".

📝 Proposed fix

-The host-WiFi reader lives at [scripts/build/host_wifi.py](build/host_wifi.py) and runs standalone for diagnosis (`python3 scripts/build/host_wifi.py` prints the resolved SSID + password).
+The host-WiFi reader lives at [scripts/build/host_wifi.py](build/host_wifi.py) and runs standalone for diagnosis (`uv run scripts/build/host_wifi.py` prints the resolved SSID + password).

🤖 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 `@scripts/MoonDeck.md` at line 349, Update the documentation example that runs
the host-WiFi reader to use the project's wrapper command instead of calling
Python directly: replace the example invocation "python3
scripts/build/host_wifi.py" with "uv run scripts/build/host_wifi.py" (or the
project's canonical "uv run" form) in the MoonDeck.md section describing
scripts/build/host_wifi.py so all Python invocations follow the coding
guideline.

Source: Coding guidelines

src/light/drivers/RmtLedDriver.h (1)

355-403: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

loopbackFrame PASS case allocates failBuf_ unnecessarily for the bit count message.

When loopbackFrame is true and the test passes, the code allocates failBuf_ (line 376) just to format a status with the bit count. This conflicts with the design comment stating "failBuf_ is only allocated when a loopback or channel init FAILs" (lines 245-248). The allocation persists until teardown or a non-FAIL result explicitly clears it.

This is minor since clearFailBuf() is called at line 374 before the allocation, but the naming/documentation mismatch could cause confusion. Consider using a flash literal for PASS messages or updating the comment to reflect that PASS with bit counts also uses the buffer.

🤖 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 355 - 403, The PASS branch for
loopbackFrame currently allocates failBuf_ just to format a "loopback PASS (%u
bits)" message, violating the invariant that failBuf_ is only created on FAIL;
change the PASS handling in the r.pass && loopbackFrame branch to avoid
assigning to failBuf_ (remove the platform::alloc call and failBuf_ assignment)
and instead format the message into a temporary local buffer or use a literal
status message before calling setStatus; keep the existing clearFailBuf() call
and only allocate failBuf_ in the failure branches (or alternatively update the
comment that documents failBuf_ usage if you deliberately want to allow PASS
allocations).

🤖 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 `@scripts/moondeck_ui/app.js`:
- Around line 592-612: renderBoardSelect is adding a new change listener on the
"board-select" element every render which causes duplicate saveState() calls;
replace the stacked addEventListener with a single assigned handler (e.g., set
select.onchange = async () => { state.provisionBoard = select.value; await
saveState(); }) or install the listener once outside renderBoardSelect so the
handler for change is not reattached on each call; ensure the handler still
updates state.provisionBoard and awaits saveState().

In `@scripts/scenario/run_network_roundtrip.py`:
- Around line 116-128: Read and store the current Grid width/height via
_grid_value(client, "Width", ...) and _grid_value(client, "Height", ...) before
any POST /api/modules call, and always attempt to restore those saved values in
a finally block (or unconditionally after the try/except) rather than gating
restore on the local added flag; change _grid_value so it does not silently
return a hardcoded 16 on any exception (instead return None or propagate the
error) so the caller can detect failed reads and avoid restoring an incorrect
default, and make the restore path check for None and log/report failures from
POST /api/state or RESTores while still attempting to restore even if module add
failed.

In `@src/light/drivers/LcdLedDriver.h`:
- Around line 350-357: Populate and propagate the first corrupted light index:
in platform_esp32_lcd.cpp’s lcdWs2812Loopback() compute the first mismatch (you
already compute mismatch) and assign it into RmtLoopbackResult::firstBadBit
before returning; then update src/light/drivers/LcdLedDriver.h’s fail formatting
to include that index (use r.firstBadBit or convert bits->light index the same
way as RmtLedDriver) when composing failBuf_ passed to setStatus so the status
reads the required “first corrupted light” value per the docs.

In `@src/platform/esp32/platform_config.h`:
- Around line 42-58: Add a decision entry in docs/history/decisions.md
documenting that the esp_lcd i80 driver must gate on
CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED (not CONFIG_SOC_LCD_I80_SUPPORTED), explain
that CONFIG_SOC_LCD_I80_SUPPORTED refers to the unrelated classic I2S-LCD on
older ESP32 chips which caused boot hangs when used, and note that
CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED is defined only for LCD_CAM-capable chips
(S3/P4); reference the lcdLanes gating in src/platform/esp32/platform_config.h
and the esp_lcd i80 initialization behavior so future maintainers understand why
the specific SOC macro is required.

In `@src/platform/esp32/platform_esp32_rmt.cpp`:
- Around line 314-317: The short loopback currently sets r.firstBadBit = 0 on
failure which is inconsistent with the frame loopback that sets the actual
mismatch index; update the short loopback logic (the block that assigns
r.bitsChecked, r.firstBadBit, and r.pass in the short loopback routine) to
compute and store the actual first mismatched bit index (like the frame loopback
does — use the same mismatch detection approach/name) and set r.firstBadBit to
that index when r.pass is false so RmtLedDriver's reporting (which reads
r.firstBadBit) shows the real bad-bit position rather than 0.

---

Outside diff comments:
In `@scripts/MoonDeck.md`:
- Line 349: Update the documentation example that runs the host-WiFi reader to
use the project's wrapper command instead of calling Python directly: replace
the example invocation "python3 scripts/build/host_wifi.py" with "uv run
scripts/build/host_wifi.py" (or the project's canonical "uv run" form) in the
MoonDeck.md section describing scripts/build/host_wifi.py so all Python
invocations follow the coding guideline.

In `@src/light/drivers/RmtLedDriver.h`:
- Around line 355-403: The PASS branch for loopbackFrame currently allocates
failBuf_ just to format a "loopback PASS (%u bits)" message, violating the
invariant that failBuf_ is only created on FAIL; change the PASS handling in the
r.pass && loopbackFrame branch to avoid assigning to failBuf_ (remove the
platform::alloc call and failBuf_ assignment) and instead format the message
into a temporary local buffer or use a literal status message before calling
setStatus; keep the existing clearFailBuf() call and only allocate failBuf_ in
the failure branches (or alternatively update the comment that documents
failBuf_ usage if you deliberately want to allow PASS allocations).

🪄 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: 2cda25ee-7274-445d-bde5-44b478e0c5c0

📥 Commits

Reviewing files that changed from the base of the PR and between f15eb01 and c5170df.

⛔ Files ignored due to path filters (5)

• docs/assets/boards/esp32-d0-16mb.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-p4-nano.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-n16r8v.jpg is excluded by !**/*.jpg
• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/improv_provision.py is excluded by !**/build/**

📒 Files selected for processing (46)

• docs/backlog/backlog.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/history/decisions.md
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• esp32/main/CMakeLists.txt
• scripts/MoonDeck.md
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/index.html
• scripts/scenario/_net_probe.py
• scripts/scenario/_preview_ws.py
• scripts/scenario/run_network_live.py
• scripts/scenario/run_network_roundtrip.py
• src/core/ImprovProvisioningModule.h
• src/core/NetworkModule.h
• src/light/drivers/Drivers.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/LcdSlots.h
• src/light/drivers/PinList.h
• src/light/drivers/RmtLedDriver.h
• src/main.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• src/ui/install-picker.js
• test/CMakeLists.txt
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/scenarios/light/scenario_modifier_swap.json
• test/unit/light/unit_CheckerboardEffect.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_LcdLedEncoder.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp

👮 Files not reviewed due to content moderation or server errors (7)

• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• src/core/ImprovProvisioningModule.h
• src/core/NetworkModule.h
• src/platform/esp32/platform_esp32_improv.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

docs/building.md (1)

137-137: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale variant count wording.

Line 137 still says “all four variants can coexist on disk,” but this section now includes additional variants (including esp32p4-eth). Please make this count-neutral (e.g., “all listed variants”) to avoid confusion.

🤖 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/building.md` at line 137, Update the sentence that currently reads “all
four variants can coexist on disk” to a count-neutral phrasing such as “all
listed variants can coexist on disk” in the documentation text referencing
build/esp32-<firmware>/; ensure the surrounding lines that mention
build_esp32.py pointing idf.py -B to the per-firmware dir and the clean command
scripts/build/clean_esp32.py --firmware (or --all) remain unchanged.

src/light/drivers/RmtLedDriver.h (2)

383-388: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Compute the bad-light index from the active channel count.

The FAIL message hardcodes 24 bits per light, but loopbackFrame passes correction_->outChannels into the platform loopback. RGBW frames are 32 bits per light, so this reports the wrong light number when 4-channel output is active.

Suggested fix

             if (failBuf_ && loopbackFrame) {
+                const unsigned bitsPerLight =
+                    static_cast<unsigned>(correction_ ? correction_->outChannels : 3) * 8u;
                 std::snprintf(failBuf_, kFailBufLen,
                               "loopback FAIL: bad bit %u/%u (light %u)",
                               static_cast<unsigned>(r.firstBadBit),
                               static_cast<unsigned>(r.bitsChecked),
-                              static_cast<unsigned>(r.firstBadBit / 24));
+                              bitsPerLight ? static_cast<unsigned>(r.firstBadBit / bitsPerLight) : 0u);
                 setStatus(failBuf_, Severity::Error);

🤖 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 383 - 388, The FAIL log
computes the light index using a hardcoded 24 bits per light; change it to use
the active channel count from correction_->outChannels used when building
loopbackFrame. Update the snprintf call that writes to failBuf_ (using
kFailBufLen) so the last formatted value is computed as
static_cast<unsigned>(r.firstBadBit / correction_->outChannels) (or store
correction_->outChannels into a local unsigned variable first), leaving
r.firstBadBit and r.bitsChecked unchanged so the message reports the correct
light index for RGBW (32-bit) and other channel counts.

---

200-208: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Skip rmtWs2812Wait() for channels whose transmit never started.

If platform::rmtWs2812Transmit() fails on any pin, this code still waits that channel for 1000 ms. One bad pin can therefore stall the render tick for up to pinCount_ seconds while no frame is actually in flight.

Suggested fix

+        bool started[kMaxPins] = {};
         for (uint8_t i = 0; i < pinCount_; i++) {
             if (pinCounts_[i] == 0) continue;
-            platform::rmtWs2812Transmit(rmt_[i], symbols_ + pinOffsets_[i],
-                                        static_cast<size_t>(pinCounts_[i]) * outCh * 8);
+            started[i] = platform::rmtWs2812Transmit(
+                rmt_[i], symbols_ + pinOffsets_[i],
+                static_cast<size_t>(pinCounts_[i]) * outCh * 8);
         }
         for (uint8_t i = 0; i < pinCount_; i++) {
-            if (pinCounts_[i] == 0) continue;
+            if (!started[i]) continue;
             platform::rmtWs2812Wait(rmt_[i], 1000 /* ms */);
         }

As per coding guidelines, "<rmtWs2812Transmit()> starts transmission and returns immediately" and "<rmtWs2812Wait()> must be bounded by timeoutMs; timeouts drop the frame so the caller can re-encode next tick."

🤖 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 200 - 208, The loop calls
platform::rmtWs2812Wait() for every channel even when
platform::rmtWs2812Transmit() failed, allowing a bad pin to stall the tick;
modify the transmit loop (using rmt_, pinCounts_, pinOffsets_, symbols_, outCh)
to capture/record which transmit calls actually started (e.g., check and store
the return status from rmtWs2812Transmit for each index) and then only call
platform::rmtWs2812Wait(rmt_[i], 1000) for those indices where transmit
succeeded; ensure the transmit return is checked and any failed transmits are
skipped so waits are bounded by the existing timeout.

Source: Coding guidelines

scripts/scenario/run_network_roundtrip.py (1)

166-169: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the real median for even repeat counts.

latencies[len(latencies) // 2] picks the upper middle sample. With the default --repeats 10, the printed “median” is biased high.

Suggested fix

+import statistics
@@
-            median = latencies[len(latencies) // 2]
+            median = statistics.median(latencies)

🤖 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 `@scripts/scenario/run_network_roundtrip.py` around lines 166 - 169, The
printed median is using the upper-middle element (latencies[len(latencies) //
2]) which biases results for even repeat counts; change the median calculation
to compute the true median: let n = len(latencies), if n is odd set median =
latencies[n // 2], else set median = (latencies[n // 2 - 1] + latencies[n // 2])
/ 2.0 (ensure float division) before the print; reference the existing variables
latencies and repeats in run_network_roundtrip.py.

src/platform/esp32/platform_esp32.cpp (1)

269-321: ⚠️ Potential issue | 🟠 Major

Fix ethInit() failure cleanup to avoid leaking/poisoning Ethernet state

• On esp_eth_driver_install() failure, ethNetif_ (from esp_netif_new) and the mac/phy instances are never torn down, and the factory calls aren’t checked for nullptr.
• On esp_eth_start() failure (after event handlers are registered), the code returns false without stopping/uninstalling the driver (esp_eth_driver_uninstall(eth_handle)), without unregistering ethEventHandler from ETH_EVENT/IP_EVENT_ETH_GOT_IP, and without deleting mac/phy and destroying ethNetif_ (esp_netif_destroy for esp_netif_new).

🤖 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.cpp` around lines 269 - 321, The ethInit()
path leaks resources and doesn't check factory nulls: ensure
esp_eth_mac_new_esp32 and the phy creators (esp_eth_phy_new_ip101 /
esp_eth_phy_new_generic) are checked for nullptr and on any failure clean up
ethNetif_ via esp_netif_destroy and free any created mac/phy; if
esp_eth_driver_install fails, uninstall/cleanup mac, phy and destroy ethNetif_;
if esp_eth_start fails (after registering handlers), call
esp_event_handler_unregister for both ETH_EVENT and IP_EVENT_ETH_GOT_IP, call
esp_eth_driver_uninstall(eth_handle) and esp_eth_stop/esp_eth_driver_uninstall
as appropriate, delete mac/phy and destroy ethNetif_; reference these symbols
when adding checks and teardown: ethInit, esp_netif_new/esp_netif_destroy,
ethNetif_, esp_eth_mac_new_esp32, esp_eth_phy_new_ip101,
esp_eth_phy_new_generic, esp_eth_driver_install, esp_eth_driver_uninstall,
esp_eth_start, esp_eth_stop, esp_event_handler_register,
esp_event_handler_unregister.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@docs/building.md`:
- Line 137: Update the sentence that currently reads “all four variants can
coexist on disk” to a count-neutral phrasing such as “all listed variants can
coexist on disk” in the documentation text referencing build/esp32-<firmware>/;
ensure the surrounding lines that mention build_esp32.py pointing idf.py -B to
the per-firmware dir and the clean command scripts/build/clean_esp32.py
--firmware (or --all) remain unchanged.

In `@scripts/scenario/run_network_roundtrip.py`:
- Around line 166-169: The printed median is using the upper-middle element
(latencies[len(latencies) // 2]) which biases results for even repeat counts;
change the median calculation to compute the true median: let n =
len(latencies), if n is odd set median = latencies[n // 2], else set median =
(latencies[n // 2 - 1] + latencies[n // 2]) / 2.0 (ensure float division) before
the print; reference the existing variables latencies and repeats in
run_network_roundtrip.py.

In `@src/light/drivers/RmtLedDriver.h`:
- Around line 383-388: The FAIL log computes the light index using a hardcoded
24 bits per light; change it to use the active channel count from
correction_->outChannels used when building loopbackFrame. Update the snprintf
call that writes to failBuf_ (using kFailBufLen) so the last formatted value is
computed as static_cast<unsigned>(r.firstBadBit / correction_->outChannels) (or
store correction_->outChannels into a local unsigned variable first), leaving
r.firstBadBit and r.bitsChecked unchanged so the message reports the correct
light index for RGBW (32-bit) and other channel counts.
- Around line 200-208: The loop calls platform::rmtWs2812Wait() for every
channel even when platform::rmtWs2812Transmit() failed, allowing a bad pin to
stall the tick; modify the transmit loop (using rmt_, pinCounts_, pinOffsets_,
symbols_, outCh) to capture/record which transmit calls actually started (e.g.,
check and store the return status from rmtWs2812Transmit for each index) and
then only call platform::rmtWs2812Wait(rmt_[i], 1000) for those indices where
transmit succeeded; ensure the transmit return is checked and any failed
transmits are skipped so waits are bounded by the existing timeout.

In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 269-321: The ethInit() path leaks resources and doesn't check
factory nulls: ensure esp_eth_mac_new_esp32 and the phy creators
(esp_eth_phy_new_ip101 / esp_eth_phy_new_generic) are checked for nullptr and on
any failure clean up ethNetif_ via esp_netif_destroy and free any created
mac/phy; if esp_eth_driver_install fails, uninstall/cleanup mac, phy and destroy
ethNetif_; if esp_eth_start fails (after registering handlers), call
esp_event_handler_unregister for both ETH_EVENT and IP_EVENT_ETH_GOT_IP, call
esp_eth_driver_uninstall(eth_handle) and esp_eth_stop/esp_eth_driver_uninstall
as appropriate, delete mac/phy and destroy ethNetif_; reference these symbols
when adding checks and teardown: ethInit, esp_netif_new/esp_netif_destroy,
ethNetif_, esp_eth_mac_new_esp32, esp_eth_phy_new_ip101,
esp_eth_phy_new_generic, esp_eth_driver_install, esp_eth_driver_uninstall,
esp_eth_start, esp_eth_stop, esp_event_handler_register,
esp_event_handler_unregister.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 525e038f-f26f-449b-8991-2bbf6d4e73f8

📥 Commits

Reviewing files that changed from the base of the PR and between c5170df and f8c6a58.

⛔ Files ignored due to path filters (3)

• esp32/partitions/esp32p4_16mb.csv is excluded by !**/*.csv
• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/setup_esp_idf.py is excluded by !**/build/**

📒 Files selected for processing (15)

• docs/backlog/backlog.md
• docs/building.md
• docs/history/decisions.md
• docs/install/boards.json
• esp32/main/idf_component.yml
• esp32/sdkconfig.defaults.esp32p4-eth
• scripts/MoonDeck.md
• scripts/moondeck_ui/app.js
• scripts/scenario/run_network_roundtrip.py
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• test/scenarios/light/scenario_AllEffects_grid_sizes.json

[coderabbitai]

Actionable comments posted: 7

♻️ Duplicate comments (1)

docs/moonmodules/light/drivers/RmtLedDriver.md (1)

29-29: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update loopbackTest behavior description to match implementation.

The documentation still describes loopbackTest as "one-shot" with "Auto-resets after running," but the unresolved past review comment indicates the implementation is a persistent mode that reruns when relevant controls change and must be manually cleared.

🤖 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/RmtLedDriver.md` at line 29, The docs
description for loopbackTest is inaccurate: instead of a one-shot auto-resetting
flag, update the RmtLedDriver.md text for `loopbackTest` to state that it is a
persistent mode that remains set until explicitly cleared, that the loopback
will re-run whenever relevant controls or inputs change, and that the test
result is written to the module's status field; reference the `loopbackTest`
control name and the module's status field in the description and remove wording
about "one-shot" and "auto-resets after running."

🤖 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/effects/AudioVolumeEffect.md`:
- Line 13: The RGBW note in AudioVolumeEffect.md is incorrect: update the
sentence that says "On an RGBW grid the white channel is left off" to reflect
the Correction contract and coding guidelines — state that lightPreset controls
physical channel order and RGBW support, and that RGBW presets emit four
channels where the white channel is derived from RGB (min(R,G,B)) after
brightness scaling/correction rather than simply being left off; keep the rest
of the description about the green→red ramp and modifiers/layouts unchanged.

In `@scripts/scenario/run_network_roundtrip.py`:
- Around line 167-169: Replace the manual median calculation using index math
with statistics.median(latencies); update the code that assigns to the variable
median (referencing the local variable latencies) to call
statistics.median(latencies) instead, and ensure the statistics module is
imported at the top of the file (add "import statistics" if it’s not already
present).

In `@src/light/effects/AudioSpectrumEffect.h`:
- Around line 85-113: The gradient branch in AudioSpectrumEffect (colorMode ==
0) computes frac using (h > 1) ? (row * 255 / h) : mag, which makes 1D (h==1)
frames use mag but multi-row gradients mis-scale and the current 1D case still
ends up as bright green for mag==0; change the frac calculation so that when h
== 1 it is driven directly by mag, and when h > 1 compute the vertical gradient
using row * 255 / (h - 1) (use denominator h-1 to map bottom→0 and top→255).
Update the frac assignment in the colour-height branch (the code that assigns r
= frac; g = 255 - frac; b = 0) accordingly so silent 1D frames are dark and
multi-row gradients scale correctly.

In `@src/platform/esp32/platform_esp32_i2s.cpp`:
- Around line 73-81: The comment states the populated slot is the RIGHT slot but
the code sets slotCfg.slot_mask = I2S_STD_SLOT_LEFT; fix by changing the slot
mask to I2S_STD_SLOT_RIGHT (update the assignment on the slotCfg instance
created with I2S_STD_PHILIPS_SLOT_DEFAULT_CONFIG) so the populated INMP441 slot
is captured; keep or update the comment to note that wiring the mic L/R-to-GND
would require flipping back to I2S_STD_SLOT_LEFT.

In `@src/ui/app.js`:
- Around line 1012-1020: The number input handler can send NaN or out-of-range
values; change the debounce callback so it parses with an explicit radix
(parseInt(input.value, 10)), then normalize the result into the uint16 range: if
parseInt yields NaN, set to 0, and clamp negatives and values >65535 to 0..65535
before calling sendControl(moduleName, ctrl.name, ...); keep updating
dragTs[key] and still call debounceSend(key, 500, ...) but pass the sanitized
integer instead of raw parseInt(input.value).

In `@test/unit/light/unit_AudioBands.cpp`:
- Line 28: Replace the non-portable M_PI usage in the expression computing s
(const double s = amp24 * std::sin(2.0 * M_PI * cycles * ...)) with the C++20
constant std::numbers::pi_v<double>, and add `#include` <numbers> to the test
file(s); update the same change in test/unit/light/unit_AudioLevel.cpp as well
so both tests use std::numbers::pi_v<double> for portability on MSVC.

In `@test/unit/light/unit_AudioLevel.cpp`:
- Around line 29-31: The current assignment does a left shift on a signed value
(v[i] = static_cast<int32_t>(s) << 8), which is UB for negative samples; change
the code to perform the shift on an unsigned type (or on a wider signed type)
and then cast back. For example, take the int32_t sample =
static_cast<int32_t>(s) (or round/clamp as needed), then do uint32_t shifted =
static_cast<uint32_t>(sample) << 8 (or int64_t tmp =
static_cast<int64_t>(sample) << 8) and finally assign v[i] =
static_cast<int32_t>(shifted); update the code around v[i], s and the sample
conversion in unit_AudioLevel.cpp accordingly.

---

Duplicate comments:
In `@docs/moonmodules/light/drivers/RmtLedDriver.md`:
- Line 29: The docs description for loopbackTest is inaccurate: instead of a
one-shot auto-resetting flag, update the RmtLedDriver.md text for `loopbackTest`
to state that it is a persistent mode that remains set until explicitly cleared,
that the loopback will re-run whenever relevant controls or inputs change, and
that the test result is written to the module's status field; reference the
`loopbackTest` control name and the module's status field in the description and
remove wording about "one-shot" and "auto-resets after running."

🪄 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: 1569188e-1868-400c-975a-31b4e7c1fa11

📥 Commits

Reviewing files that changed from the base of the PR and between f8c6a58 and 7a36bfb.

⛔ Files ignored due to path filters (1)

• docs/assets/boards/ESP32-P4-NANO-details-inter.jpg is excluded by !**/*.jpg

📒 Files selected for processing (41)

• docs/architecture.md
• docs/backlog/backlog.md
• docs/building.md
• docs/history/decisions.md
• docs/moonmodules/core/MicModule.md
• docs/moonmodules/light/drivers/ParlioLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/AudioSpectrumEffect.md
• docs/moonmodules/light/effects/AudioVolumeEffect.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• esp32/main/CMakeLists.txt
• esp32/main/idf_component.yml
• scripts/scenario/run_network_roundtrip.py
• src/core/Control.cpp
• src/core/Control.h
• src/core/MicModule.h
• src/light/AudioBands.h
• src/light/AudioFrame.h
• src/light/AudioLevel.h
• src/light/drivers/LcdSlots.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/light/effects/AudioSpectrumEffect.h
• src/light/effects/AudioVolumeEffect.h
• src/main.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_i2s.cpp
• src/platform/esp32/platform_esp32_parlio.cpp
• src/platform/platform.h
• src/ui/app.js
• test/CMakeLists.txt
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• test/scenarios/light/scenario_modifier_swap.json
• test/unit/light/unit_AudioBands.cpp
• test/unit/light/unit_AudioLevel.cpp
• test/unit/light/unit_ParlioLedDriver.cpp

[coderabbitai]

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 (5)

src/platform/esp32/platform_esp32.cpp (2)

747-759: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail bind() if the socket never becomes non-blocking.

The contract in Line 756 says this path must not stall the render loop, but fcntl(F_GETFL/F_SETFL) failures are ignored and the method still returns true. If that happens, recvFrom() runs on a blocking UDP socket and can wedge the tick exactly where this helper is meant to protect it.

💡 Suggested fix

 bool UdpSocket::bind(uint16_t port) {
     if (fd_ < 0) return false;
     int reuse = 1;
-    setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse));
+    if (setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse)) != 0) return false;
     sockaddr_in addr{};
     addr.sin_family = AF_INET;
     addr.sin_port = htons(port);
     addr.sin_addr.s_addr = htonl(INADDR_ANY);
     if (::bind(fd_, reinterpret_cast<const sockaddr*>(&addr), sizeof(addr)) != 0) return false;
     // Non-blocking so the render loop's drain never stalls waiting for a packet.
     int flags = fcntl(fd_, F_GETFL, 0);
-    fcntl(fd_, F_SETFL, flags | O_NONBLOCK);
+    if (flags < 0) return false;
+    if (fcntl(fd_, F_SETFL, flags | O_NONBLOCK) != 0) return false;
     return true;
 }

🤖 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.cpp` around lines 747 - 759,
UdpSocket::bind currently ignores errors from fcntl so the socket may remain
blocking; change it to check the return of fcntl(fd_, F_GETFL, 0) and treat -1
as failure (return false), and after computing newflags call fcntl(fd_, F_SETFL,
newflags) and if that returns -1 also return false (optionally close fd_ or mark
fd_ invalid per the class convention) so that bind() only returns true when the
socket is successfully set to non-blocking; reference symbols: UdpSocket::bind,
fd_, fcntl, F_GETFL, F_SETFL, O_NONBLOCK, and recvFrom().

Source: Coding guidelines

---

55-57: ⚠️ Potential issue | 🟡 Minor

Add explicit fcntl header for O_NONBLOCK usage in ESP32 platform.

src/platform/esp32/platform_esp32.cpp calls fcntl(..., F_GETFL/F_SETFL, ...) and uses O_NONBLOCK (around lines 755-759, 923-926, 935-936) but has no direct include of <fcntl.h>/<sys/fcntl.h>, making the build dependent on transitive header order under -Werror.

🤖 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.cpp` around lines 55 - 57, The file
platform_esp32.cpp uses fcntl (F_GETFL/F_SETFL) and O_NONBLOCK but lacks an
explicit fcntl header; add an appropriate include (e.g. <fcntl.h> or
<sys/fcntl.h>) at the top of src/platform/esp32/platform_esp32.cpp so the uses
in the fcntl(...) calls (around the code paths handling socket non-blocking
behavior using O_NONBLOCK) compile reliably without relying on transitive
headers.

Source: Coding guidelines

src/core/MicModule.h (2)

178-203: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear published audio state when the mic is reinitialized or torn down.

After a successful run, a bad pin/rate change leaves filled_ and frame_ untouched. latestFrame() then keeps serving the last non-silent frame indefinitely, so audio-reactive effects can freeze on stale data even though capture is no longer running.

🐛 Proposed fix

     void deinit() {
         if constexpr (!platform::hasI2sMic) return;
         if (inited_) platform::audioMicDeinit(mic_);
         inited_ = false;
+        filled_ = 0;
+        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/core/MicModule.h` around lines 178 - 203, The reinit() and deinit() paths
must clear the published audio state so latestFrame() won't return stale data:
when reinitializing (in reinit()) after deinit() or when teardown (in deinit())
set filled_ = false and reset/zero the frame_ buffer (and any related published
pointers) so no old frame is served; keep the existing dc_.reset() behavior but
ensure the same clearing happens on both failure-to-init and on normal deinit()
so functions like latestFrame() and any audio-reactive effects see silence
instead of stale frames.

---

19-23: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

MicModule does not fit in src/core/ in its current shape.

This header pulls platform/platform.h directly into src/core/ and owns the I2S/FFT platform seams itself, so the module is no longer platform-independent. Please move it out of src/core/ or invert this dependency behind a platform-neutral audio-source interface.

As per coding guidelines, src/core/**: Must be platform-independent — no platform includes.

🤖 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/MicModule.h` around lines 19 - 23, MicModule currently pulls
platform/platform.h and owns I2S/FFT seams, violating the platform-independent
rule for src/core; either move MicModule out of src/core or refactor it to
depend on a platform-neutral audio source interface. Remove the direct include
of platform/platform.h from MicModule.h and introduce an abstract core-facing
interface (e.g., IAudioSource or AudioInput with methods like
readFrame()/start()/stop()/getLevels()) in src/core, keep MicModule referencing
only that interface (and MoonModule/AudioFrame/AudioLevel), then implement the
platform-specific I2S/FFT code in a platform folder that provides a concrete
adapter class implementing the new interface and include that adapter only in
platform-specific build units. Ensure all references to platform/platform.h and
any I2S/FFT symbols are moved into the adapter implementation so
src/core/MicModule.h remains platform-neutral.

Source: Coding guidelines

src/platform/desktop/platform_desktop.cpp (1)

396-402: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Update the desktop Improv stub to the new signature.

platform.h now declares improvProvisioningInit(..., uint8_t* txPowerOut, std::atomic<bool>* txPowerReady), but this definition still stops at boardReady. That declaration/definition mismatch is a compile break for desktop builds.

🐛 Proposed fix

 bool improvProvisioningInit(const ImprovDeviceInfo& /*info*/,
                             char* /*ssidOut*/, size_t /*ssidOutLen*/,
                             char* /*passwordOut*/, size_t /*passwordOutLen*/,
                             std::atomic<bool>* /*ready*/,
                             char* statusBuf, size_t statusBufLen,
                             char* /*boardOut*/, size_t /*boardOutLen*/,
-                            std::atomic<bool>* /*boardReady*/) {
+                            std::atomic<bool>* /*boardReady*/,
+                            uint8_t* /*txPowerOut*/,
+                            std::atomic<bool>* /*txPowerReady*/) {

🤖 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/desktop/platform_desktop.cpp` around lines 396 - 402, The
function definition for improvProvisioningInit in platform_desktop.cpp does not
match the updated declaration in platform.h (missing txPowerOut and
txPowerReady), causing a build error; update the improvProvisioningInit
signature to include the new trailing parameters uint8_t* txPowerOut and
std::atomic<bool>* txPowerReady (keeping existing parameters like
statusBuf/statusBufLen and boardReady), and if the stub doesn't use them simply
accept the parameters (and mark as unused) so the declaration/definition match.

🤖 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`:
- Around line 276-277: This paragraph incorrectly advises pinning
Buffer*/Correction* at wiring time; update it to state that Drivers must not
hold raw intra-tree pointers across rebuilds but must re-resolve per-build
references (e.g., in onBuildState()) or use a safe indirection (lookup by
id/weak ref) so mutation-safe rebuilds (add/delete/replace) won’t dangle;
mention the specific symbols Drivers, Layer, Buffer*, Correction*, and
onBuildState() to guide contributors to rebind rather than cache raw pointers.

In `@docs/history/decisions.md`:
- Around line 768-780: The paragraph that states "prefer not to look at prior
art at all" in the "Designed fresh..." section should be revised to avoid
contradicting the repo principle; change that sentence to a
branch-scoped/qualified recommendation (e.g., mark it "branch-local/superseded")
or rewrite it as "don't trace structure; do reference behaviour" so it advocates
referencing proven behaviour without copying structure; update the surrounding
text to include a short note that this lesson is a branch-specific retrospective
and add the alternative guidance "Reference, don't copy" (or "Common patterns
first") to make the intent consistent with the repository's ongoing process.

In `@docs/history/README.md`:
- Line 13: Several edited docs still contain em dashes (—) despite the new repo
prose rule banning them; update the touched files (docs/history/README.md,
docs/moonmodules/light/effects/AudioVolumeEffect.md, CLAUDE.md,
docs/backlog/backlog.md) to replace every em dash with the approved alternative
(e.g., a hyphen "-", comma, parenthesis, or rephrase the sentence) so the branch
is internally consistent with the new "no-em-dash" standard; search each file
for "—" and apply consistent replacements, then run any prose linting/CI checks
to confirm compliance.

In `@esp32/main/idf_component.yml`:
- Around line 28-35: The idf_component.yml entries for espressif/esp_wifi_remote
and espressif/esp_hosted currently use version: "*" which allows unpinned,
drifting dependencies for target == esp32p4; update these two components
(espressif/esp_wifi_remote and espressif/esp_hosted) to exact version strings or
commit a generated idf_component.lock (checked-in lockfile) and reference it so
the resolved versions are frozen for esp32p4 builds; ensure the change updates
the version fields for those component entries or adds the lockfile to the repo
and adjusts CI/build to use it.

In `@src/light/AudioLevel.h`:
- Around line 43-50: The process(...) method can produce y values outside the
int32_t range before narrowing, causing undefined behavior when casting; fix by
clamping the computed float y to [INT32_MIN, INT32_MAX] (e.g., using std::clamp
or manual bounds checks) before assigning back into samples[i], and ensure you
reference the same locals (xPrev, yPrev, r, n, samples) so semantics are
unchanged except for the safe clamp prior to the static_cast<int32_t>.

In `@src/light/effects/AudioSpectrumEffect.h`:
- Around line 107-116: The height-gradient math uses (h - 1) and checks (h > 1)
but the spectrum only spans specH rows (specH == h - 1 when the bottom row is a
level meter), so change the condition and denominator to use specH instead of h:
compute frac using (specH > 1) ? static_cast<uint8_t>(static_cast<uint32_t>(row)
* 255u / (specH - 1)) : mag, and update the r/g assignments to check (specH > 1)
and use frac (or the scaled mag branch) accordingly in the blocks that set r and
g (the code around variables frac, r, g and the ternary expressions).

In `@src/light/effects/AudioVolumeEffect.h`:
- Around line 43-48: The loop in AudioVolumeEffect that writes per-light
channels only sets the first three bytes (r,g,b) and leaves any additional
channels from channelsPerLight() unchanged, causing stale data to persist;
update the per-light write in the for (size_t off = 0; off < total; off += cpl)
loop to explicitly clear or set all remaining channels beyond RGB (for indices 3
.. cpl-1) to a defined logical value (e.g., 0) after assigning r,g,b so the
buffer has no leftover data; locate this logic in the AudioVolumeEffect
class/function and add a small inner loop that sets buf[off + idx] = 0 for idx =
3 to cpl-1 (respecting channelsPerLight()), leaving any driver-side RGBW
correction behavior unchanged.

---

Outside diff comments:
In `@src/core/MicModule.h`:
- Around line 178-203: The reinit() and deinit() paths must clear the published
audio state so latestFrame() won't return stale data: when reinitializing (in
reinit()) after deinit() or when teardown (in deinit()) set filled_ = false and
reset/zero the frame_ buffer (and any related published pointers) so no old
frame is served; keep the existing dc_.reset() behavior but ensure the same
clearing happens on both failure-to-init and on normal deinit() so functions
like latestFrame() and any audio-reactive effects see silence instead of stale
frames.
- Around line 19-23: MicModule currently pulls platform/platform.h and owns
I2S/FFT seams, violating the platform-independent rule for src/core; either move
MicModule out of src/core or refactor it to depend on a platform-neutral audio
source interface. Remove the direct include of platform/platform.h from
MicModule.h and introduce an abstract core-facing interface (e.g., IAudioSource
or AudioInput with methods like readFrame()/start()/stop()/getLevels()) in
src/core, keep MicModule referencing only that interface (and
MoonModule/AudioFrame/AudioLevel), then implement the platform-specific I2S/FFT
code in a platform folder that provides a concrete adapter class implementing
the new interface and include that adapter only in platform-specific build
units. Ensure all references to platform/platform.h and any I2S/FFT symbols are
moved into the adapter implementation so src/core/MicModule.h remains
platform-neutral.

In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 396-402: The function definition for improvProvisioningInit in
platform_desktop.cpp does not match the updated declaration in platform.h
(missing txPowerOut and txPowerReady), causing a build error; update the
improvProvisioningInit signature to include the new trailing parameters uint8_t*
txPowerOut and std::atomic<bool>* txPowerReady (keeping existing parameters like
statusBuf/statusBufLen and boardReady), and if the stub doesn't use them simply
accept the parameters (and mark as unused) so the declaration/definition match.

In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 747-759: UdpSocket::bind currently ignores errors from fcntl so
the socket may remain blocking; change it to check the return of fcntl(fd_,
F_GETFL, 0) and treat -1 as failure (return false), and after computing newflags
call fcntl(fd_, F_SETFL, newflags) and if that returns -1 also return false
(optionally close fd_ or mark fd_ invalid per the class convention) so that
bind() only returns true when the socket is successfully set to non-blocking;
reference symbols: UdpSocket::bind, fd_, fcntl, F_GETFL, F_SETFL, O_NONBLOCK,
and recvFrom().
- Around line 55-57: The file platform_esp32.cpp uses fcntl (F_GETFL/F_SETFL)
and O_NONBLOCK but lacks an explicit fcntl header; add an appropriate include
(e.g. <fcntl.h> or <sys/fcntl.h>) at the top of
src/platform/esp32/platform_esp32.cpp so the uses in the fcntl(...) calls
(around the code paths handling socket non-blocking behavior using O_NONBLOCK)
compile reliably without relying on transitive headers.

🪄 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: b1b58476-7582-44ca-bca0-9d513dfee11c

📥 Commits

Reviewing files that changed from the base of the PR and between 7a36bfb and b79d54f.

⛔ Files ignored due to path filters (1)

• scripts/build/build_esp32.py is excluded by !**/build/**

📒 Files selected for processing (32)

• CLAUDE.md
• README.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/building.md
• docs/coding-standards.md
• docs/history/README.md
• docs/history/decisions.md
• docs/moonmodules/core/MicModule.md
• docs/moonmodules/core/SystemModule.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/AudioVolumeEffect.md
• esp32/main/idf_component.yml
• esp32/sdkconfig.defaults.esp32p4-eth-wifi
• scripts/scenario/run_network_roundtrip.py
• src/core/MicModule.h
• src/core/SystemModule.h
• src/light/AudioLevel.h
• src/light/drivers/RmtLedDriver.h
• src/light/effects/AudioSpectrumEffect.h
• src/light/effects/AudioVolumeEffect.h
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_i2s.cpp
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• src/ui/app.js
• test/unit/light/unit_AudioBands.cpp
• test/unit/light/unit_AudioLevel.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the pointer-lifetime story in this pipeline note.

This says the Buffer*/Correction* consumers are “set once at wiring time,” but the same architecture doc later treats Drivers → Layer as a rebuild-time link that must be re-resolved in onBuildState() to stay mutation-safe. Please align this section with the robustness contract so it does not tell contributors to pin an intra-tree pointer across add/delete/replace rebuilds.

🤖 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` around lines 276 - 277, This paragraph incorrectly
advises pinning Buffer*/Correction* at wiring time; update it to state that
Drivers must not hold raw intra-tree pointers across rebuilds but must
re-resolve per-build references (e.g., in onBuildState()) or use a safe
indirection (lookup by id/weak ref) so mutation-safe rebuilds
(add/delete/replace) won’t dangle; mention the specific symbols Drivers, Layer,
Buffer*, Correction*, and onBuildState() to guide contributors to rebind rather
than cache raw pointers.

Source: Learnings

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Apply the new no-em-dash prose rule consistently across docs/history/README.md, docs/moonmodules/light/effects/AudioVolumeEffect.md, CLAUDE.md, and docs/backlog/backlog.md.

The shared root cause is that this PR introduces a repo-wide prose rule banning em dashes, but several touched docs still add or retain — in the newly edited text. Please normalize the touched prose in each of those files so the new standards doc does not immediately contradict the rest of the branch.

🧰 Tools

🪛 LanguageTool

[grammar] ~13-~13: Ensure spelling is correct
Context: ...ource LED projects — the live landscape projectMM watches to sharpen its own designs unde...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 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/history/README.md` at line 13, Several edited docs still contain em
dashes (—) despite the new repo prose rule banning them; update the touched
files (docs/history/README.md,
docs/moonmodules/light/effects/AudioVolumeEffect.md, CLAUDE.md,
docs/backlog/backlog.md) to replace every em dash with the approved alternative
(e.g., a hyphen "-", comma, parenthesis, or rephrase the sentence) so the branch
is internally consistent with the new "no-em-dash" standard; search each file
for "—" and apply consistent replacements, then run any prose linting/CI checks
to confirm compliance.

Source: Coding guidelines

[coderabbitai]

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 (3)

test/unit/light/unit_AudioBands.cpp (1)

31-31: ⚠️ Potential issue | 🟠 Major

Avoid UB in signed sample left-shift (AudioBands/AudioLevel).
static_cast<int64_t>(s) << 8 shifts a potentially negative value (sine-based s), which is undefined behavior in C++. Apply the same change in both test/unit/light/unit_AudioBands.cpp (line 31) and test/unit/light/unit_AudioLevel.cpp (line 33).

Proposed fix

-        const int64_t sample = static_cast<int64_t>(s) << 8;   // avoid signed <<8 UB
+        const int64_t sample = static_cast<int64_t>(s) * 256;  // defined for negative samples

🤖 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_AudioBands.cpp` at line 31, The left-shift is applied to
a signed value causing undefined behavior; change the shift to operate on an
unsigned type and then cast back to int64_t. Locate the `sample` initialization
(currently `const int64_t sample = static_cast<int64_t>(s) << 8;`) in
unit_AudioBands.cpp and mirror the same edit in unit_AudioLevel.cpp: perform the
<< 8 on a uint64_t (e.g. cast `s` to uint64_t, shift, then static_cast<int64_t>`
the result) so the shift is defined while preserving the intended numeric value.

src/core/AudioModule.h (2)

23-28: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Keep src/core platform-independent; this module currently crosses the platform boundary.

AudioModule is under src/core/ but directly includes platform headers and invokes platform audio APIs. That couples core to hardware-facing seams and breaks the repository’s core/domain boundary contract.

Please move this module to the light domain (or isolate platform calls behind a non-core boundary) so src/core/** stays platform-agnostic.

As per coding guidelines, src/core/** “must be platform-independent — no platform includes.” Based on learnings, core infrastructure and light-domain code should stay separated as much as practical.

Also applies to: 121-123, 129-130, 149-150, 188-193, 209-210

🤖 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/AudioModule.h` around lines 23 - 28, AudioModule in src/core
currently includes platform headers (platform/platform.h) and uses platform
audio APIs, violating the core platform-independence rule; either move the
entire AudioModule (the class defined in src/core/AudioModule.h and any related
uses) into the light domain, or refactor by extracting a pure-core abstraction
(e.g., IAudioDevice / AudioPlatformInterface) placed in core with no platform
includes and implement the platform-specific adapter in light which includes
platform/platform.h; update references so AudioModule depends only on the core
interface and the platform implementation is wired in from the light layer (this
also applies to the other platform-using spots you noted around the file).

Sources: Coding guidelines, Learnings

---

192-197: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset published audio state on deinit/reinit failure to avoid stale frames.

On Line 192, reinit() deinitializes before re-init. If Line 193 init fails, latestFrame() still exposes prior frame_ contents because deinit() only clears inited_. This leaves stale non-silent output after mic init failure.

Suggested fix

 void deinit() {
     if constexpr (!platform::hasI2sMic) return;
     if (inited_) platform::audioMicDeinit(mic_);
     inited_ = false;
+    filled_ = 0;
+    dc_.reset();
+    frame_ = AudioFrame{};
 }

Also applies to: 208-212

🤖 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/AudioModule.h` around lines 192 - 197, When reinit() calls deinit()
then fails to reinitialize (platform::audioMicInit returns false) the previous
non-silent frame_ remains exposed via latestFrame(); fix by resetting published
audio state on deinit/reinit failure: in the failure path after deinit() and a
failed platform::audioMicInit (and likewise in the other reinit failure path
around the second init call), clear or zero-out frame_, mark it as
silent/uninitialized and update any timestamp/state so latestFrame() returns a
silent frame until init succeeds (alternatively modify deinit() to always clear
frame_ and related flags like inited_ so stale frames cannot be returned).

♻️ Duplicate comments (2)

src/platform/esp32/platform_esp32_lcd.cpp (1)

183-189: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Handle LCD transfer timeout as an explicit failure before buffer reuse.

Line 188 drops the xSemaphoreTake result. If a wait times out and completion arrives later, the next wait can consume a stale token and allow reuse of st->buf while DMA still owns it.

Suggested direction

-void lcdWs2812Wait(LcdWs2812Handle& h, uint32_t timeoutMs) {
+bool lcdWs2812Wait(LcdWs2812Handle& h, uint32_t timeoutMs) {
     auto* st = static_cast<LcdState*>(h.impl);
-    if (!st) return;
-    xSemaphoreTake(st->done, pdMS_TO_TICKS(timeoutMs));
+    if (!st) return false;
+    const bool done = (xSemaphoreTake(st->done, pdMS_TO_TICKS(timeoutMs)) == pdTRUE);
+    if (!done) {
+        // Drain any late completion token so the next wait cannot succeed immediately
+        // on a previous transfer's callback.
+        while (xSemaphoreTake(st->done, 0) == pdTRUE) {}
+    }
+    return done;
 }

Then update callers to branch on the returned bool and avoid starting/reusing frame buffers when completion timed out.

#!/bin/bash
# Verify API shape and timeout handling in LCD wait path.
rg -nP 'void\s+lcdWs2812Wait\s*\(|xSemaphoreTake\s*\(\s*st->done' src/platform/esp32/platform_esp32_lcd.cpp src/platform/platform.h

# Find all call sites that currently cannot observe timeout success/failure.
rg -nP '\blcdWs2812Wait\s*\(' src/platform src/light test

🤖 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_lcd.cpp` around lines 183 - 189, Change
lcdWs2812Wait to return a boolean success flag instead of void: check
xSemaphoreTake(st->done, pdMS_TO_TICKS(timeoutMs)) against pdTRUE and return
true on success, false on timeout; do not ignore the result so a late completion
cannot be consumed by the next wait. Update all callers of lcdWs2812Wait to
branch on the returned bool (avoid reusing or restarting a frame and avoid
reusing st->buf when the wait returned false) and ensure any cleanup or retry
logic handles the timeout case explicitly; reference lcdWs2812Wait, st->done,
and st->buf when changing the implementation and call sites.

src/light/AudioLevel.h (1)

50-55: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clamp still lets a UB cast path through at the positive limit.

On Line 53-Line 55, 2147483647.0f rounds to 2147483648.0f in float32, and the strict > check can let y == 2147483648.0f reach static_cast<int32_t>(...), which is undefined behavior.

Suggested fix

+#include <limits>
@@
-            const float clamped = y < -2147483648.0f ? -2147483648.0f
-                                : y >  2147483647.0f ?  2147483647.0f : y;
-            samples[i] = static_cast<int32_t>(clamped);
+            const double yd = static_cast<double>(y);
+            if (yd >= static_cast<double>(std::numeric_limits<int32_t>::max())) {
+                samples[i] = std::numeric_limits<int32_t>::max();
+            } else if (yd <= static_cast<double>(std::numeric_limits<int32_t>::min())) {
+                samples[i] = std::numeric_limits<int32_t>::min();
+            } else {
+                samples[i] = static_cast<int32_t>(y);
+            }

#!/bin/bash
python - <<'PY'
import struct

def f32(x: float) -> float:
    return struct.unpack('!f', struct.pack('!f', x))[0]

a = f32(2147483647.0)
b = f32(2147483648.0)
print("f32(2147483647.0) =", a)
print("f32(2147483648.0) =", b)
print("same-representation =", a == b)
PY

🤖 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/AudioLevel.h` around lines 50 - 55, The clamp still misses a UB
path because the float literal 2147483647.0f has the same float representation
as 2147483648.0f; change the positive bound check to use >= so values that
compare equal to that rounded representation are saturated too. In the clamping
expression that produces clamped (the ternary using y and
2147483647.0f/2147483648.0f) replace the second operator from ">" to ">=" so
samples[i] = static_cast<int32_t>(clamped) cannot receive an out‑of‑range float;
alternatively you can compute the positive bound from
static_cast<float>(std::numeric_limits<int32_t>::max()) and use >= for clarity.

🤖 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`:
- Around line 396-397: The wording about LED driver lane counts is misleading;
update the text to state that LcdLedDriver (LCD_CAM/i80) requires all 8 data
GPIOs (not variable lanes), whereas Parlio is the peripheral that supports 1–8
lanes (DMA-driven), and change the phrase “up to 8 lanes each” to clearly
distinguish the two backends and their lane requirements (reference LcdLedDriver
and Parlio).

In `@docs/backlog/backlog.md`:
- Around line 291-301: Move the completed "Audio-reactive input on ESP32 — DONE
(level + FFT 16-band), with follow-ups deferred" section out of
docs/backlog/backlog.md into a new or existing docs/history/ entry: copy the
entire heading and its body (including bullets about per-band noise-floor,
adaptive conditioning, adaptive noise gate, pin auto-scan, beat/onset detection
and the GyroDriver note), remove that block from backlog.md so the backlog
contains only open work, and ensure any relative links remain correct after
relocation; use the heading text to locate the block to move.

In `@docs/moonmodules/core/AudioModule.md`:
- Line 3: Update the module description to clarify that AudioModule exposes an
AudioFrame each render tick but only recomputes the analysed values (overall
sound level, 16-band spectrum, dominant peak) when a full sample block has been
accumulated; reference AudioModule and AudioFrame in the sentence and replace
"one analysed AudioFrame per render tick" with wording that distinguishes frame
emission cadence from analysis recomputation cadence (e.g., emit frame every
render tick, analyses updated only on completed sample blocks).

In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 230-236: The fast-path reuse condition in ParallelLedDriver (the
if using inited_, derived()->busCapacity(), frameBytes_, busPinsCurrent(), and
memset(dmaBuf_,...)) must also verify that the current lane count matches the
lane count that was used when the bus was initialized; add a stored
initialized_lane_count_ (or similar) member, compare derived()->laneCount() (or
the actual lane-count accessor) against initialized_lane_count_ in that if
condition so a lane-count change forces a rebuild, update
initialized_lane_count_ whenever the bus is rebuilt/initialized, and apply the
same change to both fast-path locations (the blocks around dmaBuf_ memset at the
two shown spots).
- Around line 72-86: onUpdate currently calls runLoopbackSelfTest() when
loopbackTest is true for both test and pin changes, but for pin-related updates
(isPinControl) the internal lane config (laneList_, laneCount_, frameBytes_) may
be stale; modify onUpdate so that when isPinControl is true you first call
parseConfig() and reinit() to refresh laneList_/laneCount_/frameBytes_ before
invoking runLoopbackSelfTest(), while preserving the existing behavior for
toggling the test (isTestControl) and the clearFailBuf()/clearStatus() branch;
update the conditional branches around isPinControl/isTestControl accordingly to
ensure the config is reloaded prior to runLoopbackSelfTest().

---

Outside diff comments:
In `@src/core/AudioModule.h`:
- Around line 23-28: AudioModule in src/core currently includes platform headers
(platform/platform.h) and uses platform audio APIs, violating the core
platform-independence rule; either move the entire AudioModule (the class
defined in src/core/AudioModule.h and any related uses) into the light domain,
or refactor by extracting a pure-core abstraction (e.g., IAudioDevice /
AudioPlatformInterface) placed in core with no platform includes and implement
the platform-specific adapter in light which includes platform/platform.h;
update references so AudioModule depends only on the core interface and the
platform implementation is wired in from the light layer (this also applies to
the other platform-using spots you noted around the file).
- Around line 192-197: When reinit() calls deinit() then fails to reinitialize
(platform::audioMicInit returns false) the previous non-silent frame_ remains
exposed via latestFrame(); fix by resetting published audio state on
deinit/reinit failure: in the failure path after deinit() and a failed
platform::audioMicInit (and likewise in the other reinit failure path around the
second init call), clear or zero-out frame_, mark it as silent/uninitialized and
update any timestamp/state so latestFrame() returns a silent frame until init
succeeds (alternatively modify deinit() to always clear frame_ and related flags
like inited_ so stale frames cannot be returned).

In `@test/unit/light/unit_AudioBands.cpp`:
- Line 31: The left-shift is applied to a signed value causing undefined
behavior; change the shift to operate on an unsigned type and then cast back to
int64_t. Locate the `sample` initialization (currently `const int64_t sample =
static_cast<int64_t>(s) << 8;`) in unit_AudioBands.cpp and mirror the same edit
in unit_AudioLevel.cpp: perform the << 8 on a uint64_t (e.g. cast `s` to
uint64_t, shift, then static_cast<int64_t>` the result) so the shift is defined
while preserving the intended numeric value.

---

Duplicate comments:
In `@src/light/AudioLevel.h`:
- Around line 50-55: The clamp still misses a UB path because the float literal
2147483647.0f has the same float representation as 2147483648.0f; change the
positive bound check to use >= so values that compare equal to that rounded
representation are saturated too. In the clamping expression that produces
clamped (the ternary using y and 2147483647.0f/2147483648.0f) replace the second
operator from ">" to ">=" so samples[i] = static_cast<int32_t>(clamped) cannot
receive an out‑of‑range float; alternatively you can compute the positive bound
from static_cast<float>(std::numeric_limits<int32_t>::max()) and use >= for
clarity.

In `@src/platform/esp32/platform_esp32_lcd.cpp`:
- Around line 183-189: Change lcdWs2812Wait to return a boolean success flag
instead of void: check xSemaphoreTake(st->done, pdMS_TO_TICKS(timeoutMs))
against pdTRUE and return true on success, false on timeout; do not ignore the
result so a late completion cannot be consumed by the next wait. Update all
callers of lcdWs2812Wait to branch on the returned bool (avoid reusing or
restarting a frame and avoid reusing st->buf when the wait returned false) and
ensure any cleanup or retry logic handles the timeout case explicitly; reference
lcdWs2812Wait, st->done, and st->buf when changing the implementation and call
sites.

🪄 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: e0301fad-3aea-4342-8a8b-71a21544e96c

📥 Commits

Reviewing files that changed from the base of the PR and between b79d54f and 6049ffa.

📒 Files selected for processing (32)

• docs/architecture.md
• docs/backlog/backlog.md
• docs/building.md
• docs/history/decisions.md
• docs/moonmodules/core/AudioModule.md
• docs/moonmodules/core/SystemModule.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/ParlioLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/AudioSpectrumEffect.md
• docs/moonmodules/light/effects/AudioVolumeEffect.md
• esp32/main/idf_component.yml
• esp32/sdkconfig.defaults.esp32p4-eth-wifi
• scripts/check/check_specs.py
• src/core/AudioModule.h
• src/light/AudioFrame.h
• src/light/AudioLevel.h
• src/light/drivers/Drivers.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/light/effects/AudioSpectrumEffect.h
• src/light/effects/AudioVolumeEffect.h
• src/main.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_i2s.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• test/unit/light/unit_AudioBands.cpp
• test/unit/light/unit_AudioLevel.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Move this shipped item out of the backlog.

docs/backlog/backlog.md says completed items are removed, but this section is marked DONE and still lives in the backlog. Please move the summary to docs/history/ and keep this file for open work only.

🤖 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/backlog/backlog.md` around lines 291 - 301, Move the completed
"Audio-reactive input on ESP32 — DONE (level + FFT 16-band), with follow-ups
deferred" section out of docs/backlog/backlog.md into a new or existing
docs/history/ entry: copy the entire heading and its body (including bullets
about per-band noise-floor, adaptive conditioning, adaptive noise gate, pin
auto-scan, beat/onset detection and the GyroDriver note), remove that block from
backlog.md so the backlog contains only open work, and ensure any relative links
remain correct after relocation; use the heading text to locate the block to
move.

[coderabbitai]

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 (6)

src/core/AudioModule.h (1)

31-31: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Move platform-coupled audio orchestration out of src/core/** (or invert through a core-owned interface).

AudioModule directly includes platform/platform.h and invokes platform seams across lifecycle/hot path. That breaks the core platform-independence contract and hard-couples src/core to platform details.

As per coding guidelines: files under src/core/** must be platform-independent and should not include platform-specific dependencies.

Also applies to: 128-129, 135-136, 155-156, 194-207, 221-224

🤖 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/AudioModule.h` at line 31, AudioModule currently includes the
platform-specific header "platform/platform.h" and calls platform seams
directly, violating core platform-independence; remove the direct include and
platform calls from src/core by defining a core-owned interface (e.g.,
IAudioPlatform or IAudioDriver) in the core layer and replace all direct
platform interactions in AudioModule (constructor, init/shutdown/hot-path
methods inside class AudioModule) with calls to that interface; inject an
implementation of the interface from the platform layer (via constructor
parameter or setter) and have the platform-specific code implement the interface
in platform/ so AudioModule no longer includes platform/platform.h or references
platform symbols.

Source: Coding guidelines

src/platform/esp32/platform_esp32_rmt.cpp (1)

112-129: ⚠️ Potential issue | 🟠 Major

Avoid rmt_disable() in rmtWs2812Deinit when rmtWs2812Wait may have timed out (src/platform/esp32/platform_esp32_rmt.cpp:112-141)

• rmtWs2812Wait() intentionally does not cancel a timed-out transfer via rmt_disable(), but rmtWs2812Deinit() unconditionally calls rmt_disable(st->channel) before rmt_del_channel(). If the TX is still active after rmt_tx_wait_all_done(st->channel, timeoutMs) times out, teardown/reinit can hit the same classic ESP32 panic path (esp-idf#17692).
• rmtWs2812Loopback() still sets firstBadBit to 0 for any failure (r.firstBadBit = r.pass ? kBits : 0;) instead of the actual first mismatch index. (src/platform/esp32/platform_esp32_rmt.cpp:422-424)

🤖 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_rmt.cpp` around lines 112 - 129,
rmtWs2812Deinit currently always calls rmt_disable(st->channel) which can
trigger the classic-ESP32 panic if rmt_tx_wait_all_done timed out; update
rmtWs2812Deinit to observe the result of rmt_tx_wait_all_done (or otherwise
check whether the channel is still actively transmitting) and only call
rmt_disable when the transfer has completed successfully, otherwise skip
rmt_disable and proceed to rmt_del_channel(); also fix rmtWs2812Loopback so that
when a loopback check fails you set r.firstBadBit to the actual mismatch index
instead of unconditionally setting it to 0 (use the computed mismatch position
when r.pass is false, otherwise leave r.firstBadBit = kBits).

docs/moonmodules/core/AudioModule.md (1)

62-100: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Move forward-looking design content out of module spec docs.

The “Adaptive noise gate: forward-looking” section introduces planned behavior in docs/moonmodules/core/AudioModule.md; this should live in backlog/history docs, with a short link here, to keep module docs strictly “current-state” and avoid spec drift.

Based on learnings: "Code, comments, and documentation describe the system as it is now (present tense only). Exceptions: docs/backlog/ and 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/moonmodules/core/AudioModule.md` around lines 62 - 100, The "Adaptive
noise gate: forward-looking" section in AudioModule.md is future design prose
and should be moved out of the current-state module spec: remove or replace the
full section headed "Adaptive noise gate: forward-looking" with a short one-line
pointer to the backlog/history (e.g., docs/backlog or docs/history) and migrate
the detailed concept, constraints, verdict, decomposition, and sequencing text
into a new backlog/history doc; ensure the AudioModule.md retains only
present-tense shipped behavior and a brief link to the new backlog entry.

Source: Learnings

test/unit/light/unit_LcdLedDriver.cpp (1)

37-41: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Comment/code mismatch on latch pad constant.

The comment on Line 37 says "784 latch pad" but the code on Line 40 uses 800 + 64 = 864. If 800 is the latch pad and 64 is alignment slack, the comment should say "800 latch pad + 64 slack" (or similar). If 784 is correct, the code is wrong.

Proposed fix (assuming 800 + 64 is correct)

-// frameBytes = maxLaneLights × outCh × 24 + 784 latch pad, rounded up to 64.
+// frameBytes = maxLaneLights × outCh × 24 + 800 latch pad + 64 slack, rounded up to 64.

🤖 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_LcdLedDriver.cpp` around lines 37 - 41, The comment and
calculation in expectFrame disagree: the comment says "784 latch pad" but the
code uses "800 + 64" (864) as latch+slack. Fix by making them consistent: either
change the comment to "800 latch pad + 64 alignment slack" to match the
arithmetic in expectFrame, or if 784 is the intended latch pad, change the
calculation to use 784 + 64 (i.e., replace 800 with 784) so the code matches the
comment; update the comment and/or constant in the expectFrame function
accordingly.

test/scenarios/light/scenario_AllEffects_grid_sizes.json (1)

22-24: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

scenario_AllEffects_grid_sizes no longer matches its “every effect” contract.

RipplesEffect was removed from the sweep while the scenario name/description still claims all effects. That drops coverage for one live effect and makes perf history incomplete.

Please either re-add a RipplesEffect measurement block or rename/reword the scenario to reflect a subset sweep.

As per coding guidelines, tests should match the specifications in docs/moonmodules/.

Also applies to: 26-26

🤖 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_AllEffects_grid_sizes.json` around lines 22 -
24, The scenario named scenario_AllEffects_grid_sizes no longer includes
RipplesEffect but still claims to cover "all effects"; either restore a
RipplesEffect measurement block into the effects list (so the array that
currently lists "RingsEffect","LavaLampEffect","GameOfLifeEffect" also includes
"RipplesEffect") or rename/reword scenario_AllEffects_grid_sizes and its
description to accurately describe the subset of effects tested; update any doc
string or test metadata that references scenario_AllEffects_grid_sizes to keep
tests and docs in sync with the new effect list.

Source: Coding guidelines

docs/architecture.md (1)

276-277: ⚠️ Potential issue | 🟠 Major

Fix Buffer/Correction lifetime contract in docs/architecture.md (line 276-277)**
Code shows mm::Drivers::onBuildState() re-resolves layer_ from Layers::activeLayer() and then calls passBufferToDrivers() every rebuild, which updates each child driver’s setSourceBuffer(...) and setLayer(...) (and clears them to nullptr when there’s no active Layer). So consumers are not “set once at wiring time” across mutations; update line 276-277 to state the pointers are (re)bound during Drivers::onBuildState and are only guaranteed valid until the next rebuild, aligning with the robustness rule at line 184.

🤖 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` around lines 276 - 277, Update the statement about
Buffer*/Correction* lifetime to reflect that mm::Drivers::onBuildState()
re-resolves layer_ via Layers::activeLayer() and calls passBufferToDrivers() on
each rebuild, which rebinds child drivers via setSourceBuffer(...) and
setLayer(...) (and clears them to nullptr when no active Layer); say the
pointers are (re)bound during Drivers::onBuildState() and are only guaranteed
valid until the next rebuild, matching the robustness rule at line 184.

Source: Learnings

🤖 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/LcdLedDriver.md`:
- Line 34: The relative link in docs/moonmodules/light/drivers/LcdLedDriver.md
currently points to ../../tests/unit-tests.md which resolves incorrectly from
this directory; update the link to go up one more level (use
../../../tests/unit-tests.md) so the "unit tests § LcdLedDriver" anchor resolves
correctly.

In `@docs/moonmodules/light/drivers/ParlioLedDriver.md`:
- Line 56: Update the ParlioLedDriver documentation to present-tense by removing
or relocating the forward-looking sentence "Driving a real strip is a later
increment." to the docs/backlog area; keep the current text describing the
tick-scaling and whole-frame loopback self-test (jumper GPIO 32 → 33) as-is but
delete the roadmap sentence from this file and add it to docs/backlog with a
short note that it’s future work.

In `@src/core/AudioModule.h`:
- Around line 199-205: The status "mic: set wsPin / sdPin / sckPin" can remain
stale after a successful mic init because only kInitFailMsg is cleared; update
the mic initialization success path (the function that checks wsPin, sdPin,
sckPin and calls setStatus) to unconditionally clear any prior status message
after a successful init — not just kInitFailMsg — by invoking the module's
status-clear method (or call setStatus with a neutral/OK message) in the success
branch so the earlier "mic: set wsPin / sdPin / sckPin" is removed; look for
symbols wsPin, sdPin, sckPin, setStatus and kInitFailMsg to locate the code to
change.

In `@src/light/drivers/RmtLedDriver.h`:
- Around line 69-70: loopbackRxPin should not default to 0 because GPIO0 is a
valid pin; change its default to an explicit "unset" sentinel (e.g., uint16_t
loopbackRxPin = UINT16_MAX or an optional/invalid value) and update the code
that consumes loopbackRxPin (the loopback setup/receive logic that currently
uses loopbackRxPin directly) to check for that sentinel before performing any
loopback/attach actions; only use a real pin (or default to the bench pin) when
loopbackRxPin has been explicitly configured and avoid touching GPIO0
implicitly.

In `@src/platform/esp32/platform_esp32_parlio.cpp`:
- Around line 231-239: The transmitOnce lambda currently ignores the return
value of parlio_tx_unit_transmit and xSemaphoreTake(st->done,...), so failures
to enqueue TX or timeouts are silent and waste time; update transmitOnce to
capture parlio_tx_unit_transmit's result and, if it indicates failure,
log/report the error and abort the loopback early, and check xSemaphoreTake's
return (expect pdTRUE) and treat a timeout as a failure (log/report and abort or
set a failure flag) instead of proceeding; reference the transmitOnce lambda,
parlio_tx_unit_transmit, xSemaphoreTake, and st->done when making the checks and
ensure the calling loop observes the failure flag to stop further cadence
iterations.

---

Outside diff comments:
In `@docs/architecture.md`:
- Around line 276-277: Update the statement about Buffer*/Correction* lifetime
to reflect that mm::Drivers::onBuildState() re-resolves layer_ via
Layers::activeLayer() and calls passBufferToDrivers() on each rebuild, which
rebinds child drivers via setSourceBuffer(...) and setLayer(...) (and clears
them to nullptr when no active Layer); say the pointers are (re)bound during
Drivers::onBuildState() and are only guaranteed valid until the next rebuild,
matching the robustness rule at line 184.

In `@docs/moonmodules/core/AudioModule.md`:
- Around line 62-100: The "Adaptive noise gate: forward-looking" section in
AudioModule.md is future design prose and should be moved out of the
current-state module spec: remove or replace the full section headed "Adaptive
noise gate: forward-looking" with a short one-line pointer to the
backlog/history (e.g., docs/backlog or docs/history) and migrate the detailed
concept, constraints, verdict, decomposition, and sequencing text into a new
backlog/history doc; ensure the AudioModule.md retains only present-tense
shipped behavior and a brief link to the new backlog entry.

In `@src/core/AudioModule.h`:
- Line 31: AudioModule currently includes the platform-specific header
"platform/platform.h" and calls platform seams directly, violating core
platform-independence; remove the direct include and platform calls from
src/core by defining a core-owned interface (e.g., IAudioPlatform or
IAudioDriver) in the core layer and replace all direct platform interactions in
AudioModule (constructor, init/shutdown/hot-path methods inside class
AudioModule) with calls to that interface; inject an implementation of the
interface from the platform layer (via constructor parameter or setter) and have
the platform-specific code implement the interface in platform/ so AudioModule
no longer includes platform/platform.h or references platform symbols.

In `@src/platform/esp32/platform_esp32_rmt.cpp`:
- Around line 112-129: rmtWs2812Deinit currently always calls
rmt_disable(st->channel) which can trigger the classic-ESP32 panic if
rmt_tx_wait_all_done timed out; update rmtWs2812Deinit to observe the result of
rmt_tx_wait_all_done (or otherwise check whether the channel is still actively
transmitting) and only call rmt_disable when the transfer has completed
successfully, otherwise skip rmt_disable and proceed to rmt_del_channel(); also
fix rmtWs2812Loopback so that when a loopback check fails you set r.firstBadBit
to the actual mismatch index instead of unconditionally setting it to 0 (use the
computed mismatch position when r.pass is false, otherwise leave r.firstBadBit =
kBits).

In `@test/scenarios/light/scenario_AllEffects_grid_sizes.json`:
- Around line 22-24: The scenario named scenario_AllEffects_grid_sizes no longer
includes RipplesEffect but still claims to cover "all effects"; either restore a
RipplesEffect measurement block into the effects list (so the array that
currently lists "RingsEffect","LavaLampEffect","GameOfLifeEffect" also includes
"RipplesEffect") or rename/reword scenario_AllEffects_grid_sizes and its
description to accurately describe the subset of effects tested; update any doc
string or test metadata that references scenario_AllEffects_grid_sizes to keep
tests and docs in sync with the new effect list.

In `@test/unit/light/unit_LcdLedDriver.cpp`:
- Around line 37-41: The comment and calculation in expectFrame disagree: the
comment says "784 latch pad" but the code uses "800 + 64" (864) as latch+slack.
Fix by making them consistent: either change the comment to "800 latch pad + 64
alignment slack" to match the arithmetic in expectFrame, or if 784 is the
intended latch pad, change the calculation to use 784 + 64 (i.e., replace 800
with 784) so the code matches the comment; update the comment and/or constant in
the expectFrame function accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fa6f93e8-2f2b-403b-a505-dfcc82e8ba77

📥 Commits

Reviewing files that changed from the base of the PR and between 6049ffa and c7fab1e.

📒 Files selected for processing (29)

• docs/architecture.md
• docs/backlog/backlog.md
• docs/history/decisions.md
• docs/moonmodules/core/AudioModule.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/NetworkSendDriver.md
• docs/moonmodules/light/drivers/ParlioLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/NetworkReceiveEffect.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• src/core/AudioModule.h
• src/light/AudioLevel.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/main.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• src/platform/esp32/platform_esp32_parlio.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/unit/light/unit_AudioModule.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_ParlioLedDriver.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix broken relative link to unit-test docs.

Line 34 points to ../../tests/unit-tests.md, which resolves under docs/moonmodules/tests/... and breaks from this directory. It should go up one more level.

Suggested fix

-Full case list in the generated [unit tests § LcdLedDriver](../../tests/unit-tests.md#lcdleddriver) (regenerated from the test files, never drifts). What's covered:
+Full case list in the generated [unit tests § LcdLedDriver](../../../tests/unit-tests.md#lcdleddriver) (regenerated from the test files, never drifts). What's covered:

📝 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

	Full case list in the generated [unit tests § LcdLedDriver](../../tests/unit-tests.md#lcdleddriver) (regenerated from the test files, never drifts). What's covered:
	Full case list in the generated [unit tests § LcdLedDriver](../../../tests/unit-tests.md#lcdleddriver) (regenerated from the test files, never drifts). What's covered:

🤖 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/LcdLedDriver.md` at line 34, The relative link
in docs/moonmodules/light/drivers/LcdLedDriver.md currently points to
../../tests/unit-tests.md which resolves incorrectly from this directory; update
the link to go up one more level (use ../../../tests/unit-tests.md) so the "unit
tests § LcdLedDriver" anchor resolves correctly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep module docs present-tense and move roadmap text to backlog.

Line 56 says “Driving a real strip is a later increment.” In docs/moonmodules/**, this should describe current behavior; move forward-looking text to docs/backlog/.

Based on learnings: documentation outside docs/backlog/ and docs/history/ should describe the system as it is now (present tense only).

🤖 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/ParlioLedDriver.md` at line 56, Update the
ParlioLedDriver documentation to present-tense by removing or relocating the
forward-looking sentence "Driving a real strip is a later increment." to the
docs/backlog area; keep the current text describing the tick-scaling and
whole-frame loopback self-test (jumper GPIO 32 → 33) as-is but delete the
roadmap sentence from this file and add it to docs/backlog with a short note
that it’s future work.

Source: Learnings

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clear stale status unconditionally after successful mic init.

If the prior state was “mic: set wsPin / sdPin / sckPin”, successful init does not clear it because Line 218 only clears kInitFailMsg. That leaves a stale status after a healthy init.

Suggested patch

-        if (status() == kInitFailMsg) clearStatus();
+        clearStatus();

Also applies to: 218-219

🤖 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/AudioModule.h` around lines 199 - 205, The status "mic: set wsPin /
sdPin / sckPin" can remain stale after a successful mic init because only
kInitFailMsg is cleared; update the mic initialization success path (the
function that checks wsPin, sdPin, sckPin and calls setStatus) to
unconditionally clear any prior status message after a successful init — not
just kInitFailMsg — by invoking the module's status-clear method (or call
setStatus with a neutral/OK message) in the success branch so the earlier "mic:
set wsPin / sdPin / sckPin" is removed; look for symbols wsPin, sdPin, sckPin,
setStatus and kInitFailMsg to locate the code to change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

loopbackRxPin is documented as unset, but defaults to GPIO 0.

Line 69 sets loopbackRxPin = 0, and Line 351 consumes it directly. That is not an unset state; it can run loopback against GPIO0 before the user configures RX.

Suggested fix

+    static constexpr uint16_t kUnsetPin = 0xFFFF;
-    uint16_t loopbackRxPin = 0;     // jumper this to the first pin for the test
+    uint16_t loopbackRxPin = kUnsetPin;  // jumper this to the first pin for the test
                                     // (unset by default; bench used pin 5)
@@
     void runLoopbackSelfTest() {
@@
+        if (loopbackRxPin == kUnsetPin) {
+            clearFailBuf();
+            setStatus("loopback: set loopbackRxPin", Severity::Warning);
+            return;
+        }
         const uint8_t txPin = static_cast<uint8_t>(pinList_[0]);
         const uint8_t rxPin = static_cast<uint8_t>(loopbackRxPin);

Based on learnings: “Default only when it cannot do harm.”

Also applies to: 350-352

🤖 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 69 - 70, loopbackRxPin should
not default to 0 because GPIO0 is a valid pin; change its default to an explicit
"unset" sentinel (e.g., uint16_t loopbackRxPin = UINT16_MAX or an
optional/invalid value) and update the code that consumes loopbackRxPin (the
loopback setup/receive logic that currently uses loopbackRxPin directly) to
check for that sentinel before performing any loopback/attach actions; only use
a real pin (or default to the bench pin) when loopbackRxPin has been explicitly
configured and avoid touching GPIO0 implicitly.

Source: Learnings

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the file and show relevant surrounding code
FILE="src/platform/esp32/platform_esp32_parlio.cpp"
if [ ! -f "$FILE" ]; then
  echo "Missing file: $FILE"
  exit 1
fi

echo "=== Context around lines 200-280 ==="
nl -ba "$FILE" | sed -n '200,280p'

echo "=== Search for st->done usage in this file ==="
rg -n "st->done|xSemaphoreTake|xSemaphoreGive|done\s*=" "$FILE" || true

echo "=== Search for parlio_tx_unit_transmit usage in this file ==="
rg -n "parlio_tx_unit_transmit" "$FILE" || true

echo "=== Search for done callback / ISR that likely gives the semaphore ==="
rg -n "done.*callback|callback.*done|parlio.*cb|on_.*done|SemaphoreGive" "$FILE" || true

Repository: MoonModules/projectMM

Length of output: 164

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/platform/esp32/platform_esp32_parlio.cpp"
test -f "$FILE"

echo "=== Context around the referenced section (lines ~200-280) ==="
# cat -n is available; limit output
cat -n "$FILE" | sed -n '200,280p'

echo "=== Search for semaphore wait / give usage ==="
rg -n "xSemaphoreTake|xSemaphoreGive|st->done|done\s*=|SemaphoreGive|SemaphoreHandle_t" "$FILE" || true

echo "=== Search for parlio_tx_unit_transmit call sites ==="
rg -n "parlio_tx_unit_transmit" "$FILE" || true

echo "=== Search for the done callback / ISR path that likely signals st->done ==="
rg -n "parlio.*callback|callback.*parlio|done.*callback|tx.*done|on_.*done|SemaphoreGive" "$FILE" || true

Repository: MoonModules/projectMM

Length of output: 4835

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/platform/esp32/platform_esp32_parlio.cpp"

echo "=== Context around detail helpers and captureAndVerifyFrame (lines 1-220) ==="
cat -n "$FILE" | sed -n '1,220p'

echo "=== Extract captureAndVerifyFrame body only (if any) ==="
# Use a narrower range likely covering implementation
cat -n "$FILE" | sed -n '220,360p'

Repository: MoonModules/projectMM

Length of output: 13766

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "void captureAndVerifyFrame\\(" -S . || true
rg -n "captureAndVerifyFrame\\(" src/platform -S || true

# If found, print a focused window around the definition.
FILE="$(rg -n "void captureAndVerifyFrame\\(" -S . | head -n 1 | cut -d: -f1 || true)"
if [ -n "${FILE:-}" ] && [ -f "$FILE" ]; then
  echo "=== Definition file: $FILE ==="
  start=$(($(rg -n "void captureAndVerifyFrame\\(" "$FILE" | head -n 1 | cut -d: -f1)-20)) || true
  start=$(( start<1 ? 1 : start ))
  end=$(( start+120 ))
  cat -n "$FILE" | sed -n "${start},${end}p"
fi

# Also find RmtLoopbackResult handling to see how failure is represented.
rg -n "struct RmtLoopbackResult|RmtLoopbackResult" src/platform -S || true

Repository: MoonModules/projectMM

Length of output: 10770

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/platform/platform.h"
test -f "$FILE"
echo "=== RmtLoopbackResult definition ==="
cat -n "$FILE" | sed -n '320,430p'

Repository: MoonModules/projectMM

Length of output: 7830

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/platform/esp32/platform_esp32_lcd.cpp"
test -f "$FILE"

echo "=== Context around lcdWs2812Loopback transmitOnce ==="
cat -n "$FILE" | sed -n '200,320p'

echo "=== Search for transmitOnce lambdas in lcd file ==="
rg -n "transmitOnce" "$FILE" || true

Repository: MoonModules/projectMM

Length of output: 4760

---

Handle Parlio loopback TX enqueue/wait failures explicitly.

The loopback transmitOnce ignores both parlio_tx_unit_transmit(...) and the return value of xSemaphoreTake(st->done, ...). If TX enqueue fails or the done-callback never arrives, the code still proceeds after the full timeout without any diagnostics (and with potentially wasted time during the loopback cadence), even though the final pass/fail is derived from captured/verified symbols.

Suggested patch

-    auto transmitOnce = [st, frameBytes, &xcfg]() {
-        parlio_tx_unit_transmit(st->unit, st->buf, frameBytes * 8, &xcfg);
-        xSemaphoreTake(st->done, pdMS_TO_TICKS(1000));
-    };
+    auto transmitOnce = [st, frameBytes, &xcfg]() {
+        if (parlio_tx_unit_transmit(st->unit, st->buf, frameBytes * 8, &xcfg) != ESP_OK) {
+            ESP_LOGE(PAR_TAG, "loopback: transmit failed");
+            return;
+        }
+        if (xSemaphoreTake(st->done, pdMS_TO_TICKS(1000)) != pdTRUE) {
+            ESP_LOGE(PAR_TAG, "loopback: transmit timeout");
+        }
+    };

📝 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

	// The Parlio-specific transmit: ship one frame (length in BITS, not bytes)
	// and wait for its done-callback. Everything else (capture, cadence, bit-
	// verify) is the shared helper.
	parlio_transmit_config_t xcfg = {};
	xcfg.idle_value = 0; // lines rest LOW between frames (the latch)
	auto transmitOnce = [st, frameBytes, &xcfg]() {
	parlio_tx_unit_transmit(st->unit, st->buf, frameBytes * 8, &xcfg);
	xSemaphoreTake(st->done, pdMS_TO_TICKS(1000));
	};
	// The Parlio-specific transmit: ship one frame (length in BITS, not bytes)
	// and wait for its done-callback. Everything else (capture, cadence, bit-
	// verify) is the shared helper.
	parlio_transmit_config_t xcfg = {};
	xcfg.idle_value = 0; // lines rest LOW between frames (the latch)
	auto transmitOnce = [st, frameBytes, &xcfg]() {
	if (parlio_tx_unit_transmit(st->unit, st->buf, frameBytes * 8, &xcfg) != ESP_OK) {
	ESP_LOGE(PAR_TAG, "loopback: transmit failed");
	return;
	}
	if (xSemaphoreTake(st->done, pdMS_TO_TICKS(1000)) != pdTRUE) {
	ESP_LOGE(PAR_TAG, "loopback: transmit timeout");
	}
	};

🤖 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_parlio.cpp` around lines 231 - 239, The
transmitOnce lambda currently ignores the return value of
parlio_tx_unit_transmit and xSemaphoreTake(st->done,...), so failures to enqueue
TX or timeouts are silent and waste time; update transmitOnce to capture
parlio_tx_unit_transmit's result and, if it indicates failure, log/report the
error and abort the loopback early, and check xSemaphoreTake's return (expect
pdTRUE) and treat a timeout as a failure (log/report and abort or set a failure
flag) instead of proceeding; reference the transmitOnce lambda,
parlio_tx_unit_transmit, xSemaphoreTake, and st->done when making the checks and
ensure the calling loop observes the failure flag to stop further cadence
iterations.