Faster, wider LED output: SWAR encode, async DMA, a second core, 16 lanes, and many-tube network send

[ewowi]

Eight commits of LED-output work: the drivers get faster (a SWAR transpose, async DMA, a second core), wider (16 lanes, a classic-ESP32 i80 driver), and able to feed a wall of network tubes from one driver. Along the way the light-preset model became a reusable library, the 1 Hz WebSocket push became a diff, and the module UI stopped growing without limit.

Measured on hardware (P4 + S3, 128×128 grid): the multicore split alone took the board from 32 → 46 fps (+44%) and moved 85% of the output stage off the render core (Drivers 15,404 → 2,261 µs). The async DMA double-buffer took the P4 from 48 → 76 fps by hiding the WS2812 wire behind the encode. Both are switches, both default on.

What's in it

LED output — faster

• SWAR transpose (ParallelSlots.h) — the bit→slot encode is 85% of driver time; the branch-free 3-delta-swap (Warren, Hacker's Delight) replaced a per-bit-per-lane gather. Zero table, less code, ~20% whole-board fps.
• Async DMA double-buffer (asyncTransmit, default on) — encode frame N+1 while the peripheral clocks out frame N. A new wireUs KPI reports the WS2812 wire floor directly, so the real output ceiling is visible rather than inferred.
• Multicore render↔encode split (multicore, default on) — a core-1 worker runs the whole output stage (LED encode, ArtNet build, preview build) while core 0 renders the next frame and serves the network. One rule, no per-driver opt-out: the container owns the mechanism. This also fixed a real failure — a heavy encode on core 0 was starving the network stack until HTTP stopped answering.

LED output — wider

• 16 lanes on LCD_CAM (S3/P4) and Parlio (P4); bus width derives from the pin count.
• Classic-ESP32 i80 driver — the same I80LedDriver now drives the classic chip's I2S peripheral in i80 mode (IDF routes one API to I2S on classic, LCD_CAM on S3/P4). Measured ceiling 2,048 lights, and it degrades with a status rather than crashing.
• Any-channel-count fixtures — an RGBW panel and a GRB strip can share one board, each with its own preset and brightness.

Network output — many tubes, one driver

• NetworkSendDriver feeds one or more receivers: ips takes a range (192.168.1.70-74) or a list (192.168.1.60,61,62,65), and lightsPerIp splits the window across them using the same broadcasting idiom as an LED driver's ledsPerPin. Each tube is unicast only its own slice.
• The destination now defaults to unset. Broadcasting a 128×128 grid is ~97 Art-Net universes × 50 fps ≈ 4,850 pkt/s — measured starving an ESP32's network stack until it stopped answering HTTP. Art-Net 4 requires unicast ("there are no conditions in which broadcast is allowed"); an unconfigured driver now idles with a status instead of flooding the LAN.

Light presets

• A reusable, editable library instead of a per-driver enum: named channel-role wirings referenced by a stable id, resolved once at config time into the flat correction (the render loop never touches a preset). Ships 13 built-in fixtures.

Core

• Diff-on-the-wire state push — the 1 Hz WebSocket update sends only changed leaves instead of re-serialising the whole ~34 KB tree. This fixed a visible 1 Hz LED stutter: the periodic push runs inline on the render thread, so a heavy tick1s steals from the render budget at exactly its cadence.
• MoonModule::quiesce() — core parks a module's worker before any structural mutation of its children, so deleting a driver mid-encode can't free memory the worker is inside. Fixes a use-after-free.
• A worker-task seam (spawnPinnedTask/notifyTask/waitNotify) — textbook FreeRTOS SPSC on ESP32, std::thread+condvar on desktop so the cross-core invariants are host-testable.

UI

• Every top-level module shows its children as tabs (one at a time, drag to reorder) instead of a stack that grows without limit. Tabs carry a fault dot, so an error on a background tab is still visible.

Testing

• A sanitizer gate that did not exist — ASan + TSan, locally (check_sanitizers.py) and in CI. The multicore split put a second thread beside code that had been single-threaded for its whole life; hand-written tests prove the races we thought of, sanitizers find the rest. On macOS, Apple's ASan runtime hangs during its own startup while Homebrew's works, and TSan crashes under both — the script probes each runtime first and reports SKIPPED rather than a misleading pass.

Breaking changes

No migration code (per ADR-0013); each break is documented and the lost value re-populates on next use.

• NetworkSendDriver.ip → ips: a persisted NetworkSend driver is dropped on first boot after this change. Re-add it once per board.
• DevicesModule persisted key colour → color: a cached Hue bridge's light count reads as 0 until the bridge is next heard.
• .config/LayoutGroup.json → Layouts.json, DriverGroup.json → Drivers.json; localStorage mm.selectedModule → mm_selected.

Verification

• 778 unit tests, 16 scenarios, zero build warnings (-Wall -Wextra -Werror).
• ASan clean on the concurrency suites. TSan has not run against the multicore code yet — it can't run on macOS, and the CI job that covers it is new in this PR; it gets its first real exercise here.
• Hardware: ESP32-P4, ESP32-S3 (SE16 + LightCrafter 16), ESP32-WROVER. All three chips build clean; the drivers were verified driving real strips.

[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: 350b5224-3e4a-4c17-93b5-3655a74d1d01

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

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.39% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is clearly related to the main changes: faster/wider LED output, 16-lane support, async DMA, multicore split, and multi-destination network send.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch next-iteration

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 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/ChannelRole.h`:
- Around line 34-39: Add a compile-time static_assert near kChannelRoleCount
that verifies the options array length matches the ChannelRole enum’s
final/sentinel value, preserving the index-alignment contract used by
Correction.h and LightPresetsModule.h. Ensure the assertion catches either an
enum entry or option string being added without updating the other.

In `@src/light/drivers/Correction.h`:
- Around line 151-160: Update the white-channel handling in apply() so
out[offWhite] is explicitly set to 0 when whiteMode is WhiteMode::None, while
preserving the existing Accurate and other enabled-mode behavior that writes the
computed white component.

In `@src/light/drivers/LightPresetsModule.h`:
- Around line 238-243: Replace the channel-index conversion in the field-update
logic with strtol, validating errno for range errors, the end pointer for a
fully numeric suffix, and the parsed value before using it as an index. Reject
malformed or out-of-range channel names instead of coercing them to zero, while
preserving the existing p.channelCount bounds check and role assignment
behavior.

In `@test/unit/core/unit_FilesystemModule_persistence.cpp`:
- Line 330: Update the inline comment on scheduler.setup() to state that it
seeds 13 built-ins, matching the seeded count used by the subsequent assertions.

🪄 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: 0ea404a8-ee7e-4e29-ae13-d5431d0aec69

📥 Commits

Reviewing files that changed from the base of the PR and between d07d870 and 5920ae6.

⛔ Files ignored due to path filters (5)

• docs/assets/deviceModels/serg-minishield pinout.png is excluded by !**/*.png
• docs/assets/deviceModels/serg-minishield.jpg is excluded by !**/*.jpg
• docs/assets/deviceModels/serg-unishield-v5 pinout.png is excluded by !**/*.png
• docs/assets/deviceModels/serg-unishield-v5.jpg is excluded by !**/*.jpg
• docs/assets/light/drivers/lightpresets.png is excluded by !**/*.png

📒 Files selected for processing (39)

• CLAUDE.md
• docs/backlog/backlog-light.md
• docs/history/plans/Plan-20260711 - Flexible light profile (channel-role offsets).md
• docs/history/plans/Plan-20260711 - LightPresets reusable named-preset library.md
• docs/history/plans/Plan-20260711 - Migrate MoonLight fixture presets as built-ins (shipped).md
• docs/moonmodules/light/supporting.md
• src/core/Control.h
• src/core/FileManagerModule.cpp
• src/core/FilesystemModule.cpp
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/JsonUtil.h
• src/light/ChannelRole.h
• src/light/drivers/Correction.h
• src/light/drivers/DriverBase.h
• src/light/drivers/Drivers.h
• src/light/drivers/HueDriver.h
• src/light/drivers/LightPresetsModule.h
• src/light/drivers/NetworkSendDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/PreviewDriver.h
• src/light/drivers/RmtLedDriver.h
• src/main.cpp
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/unit/core/unit_Control_list.cpp
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_JsonUtil_parse.cpp
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_Drivers_container.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_LightPresetsModule.cpp
• test/unit/light/unit_NetworkSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_ParlioLedDriver.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp
• web-installer/deviceModels.json

[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/core/HttpServerModule.cpp (3)

1910-1935: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

afterListMutation() doesn't request a full resync, but its own comment says a mutation reshapes other modules' option metadata.

The comment here explicitly states: "adding/removing a light preset changes the option set of every driver's preset Select." That's exactly the kind of change requestFullResync() exists for (see the identical rationale at the 5 module-CRUD call sites). Without it, only the client that performed the mutation sees the update — via its own refetchState() HTTP GET in app.js. Any other connected WS client (a second tab, another observer) keeps receiving only value-patches and never learns that a driver's preset dropdown grew/shrank/renamed, until an unrelated structural change or reconnect happens to trigger a resync.

🐛 Proposed fix

     if (auto* drivers = static_cast<Drivers*>(findModuleByName("Drivers")))
         drivers->rebuildAllCorrections();
+    // A list edit can reshape other modules' Select option sets (e.g. every driver's `preset`
+    // dropdown) even when no leaf VALUE changed — the diff-on-wire patch can't describe that, so
+    // force a full resync for every connected client, not just the one that made the edit.
+    requestFullResync();
 }

🤖 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/HttpServerModule.cpp` around lines 1910 - 1935, Update
HttpServerModule::afterListMutation() to call scheduler_->requestFullResync()
after rebuilding controls and corrections, ensuring every connected WebSocket
client receives changed option metadata after a list mutation. Keep the existing
dirty-marking, control rebuilding, and correction refresh behavior unchanged.

---

1958-1963: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reorder to isn't bounds-checked before the uint8_t cast — can silently move a row to the wrong index.

to < 0 is checked, but a to beyond 255 wraps on static_cast<uint8_t>(to) (e.g. to=261 → 5) before moveListRow's internal if (to >= n) to = n - 1; clamp ever sees the original value. If the wrapped value happens to land inside [0, n), the row moves to an unintended position instead of being rejected or clamped to the list end. handleMoveModule (a few hundred lines up) guards this correctly with to >= parent->childCount(); this path lacks the equivalent check.

🐛 Proposed fix

     if (mm::json::hasKey(jsonBody, "to")) {
         int to = mm::json::parseInt(jsonBody, "to");
-        if (to < 0 || !src->moveListRow(id, static_cast<uint8_t>(to))) {
+        if (to < 0 || to > 255 || !src->moveListRow(id, static_cast<uint8_t>(to))) {
             sendResponse(conn, 400, "application/json", "{\"error\":\"move failed\"}");
             return;
         }

🤖 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/HttpServerModule.cpp` around lines 1958 - 1963, Update the reorder
handling in the JSON “to” branch of HttpServerModule to validate the parsed
integer before converting it to uint8_t, rejecting values above the valid list
boundary instead of allowing wraparound. Mirror the bounds-checking behavior
used by handleMoveModule, then call moveListRow only with a validated index and
preserve the existing 400 response for invalid moves.

---

1884-1887: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Row id parsed with strtoul but never validated as clean digits.

/api/list/<mod>/<ctrl>/<id> accepts any garbage after the second slash: a non-numeric id (".../xyz") silently resolves to outId = 0, and trailing junk after digits (".../5abc") silently truncates to 5. Elsewhere in this same file, Content-Length parsing deliberately rejects non-numeric/trailing-junk input with strtol + errno/end-pointer checks rather than tolerating a garbage prefix — this path skips that same rigor, so a malformed id can silently target row 0 (or an unintended row) instead of failing with 400.

🛡️ Proposed fix

     if (s2 && s2[1]) {   // an id segment follows the control
-        outId = static_cast<uint32_t>(std::strtoul(s2 + 1, nullptr, 10));
-        outHasId = true;
+        char* idEnd = nullptr;
+        const unsigned long parsedId = std::strtoul(s2 + 1, &idEnd, 10);
+        if (idEnd == s2 + 1 || *idEnd != 0) {
+            sendResponse(conn, 400, "application/json", "{\"error\":\"bad id\"}");
+            return nullptr;
+        }
+        outId = static_cast<uint32_t>(parsedId);
+        outHasId = 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/core/HttpServerModule.cpp` around lines 1884 - 1887, Validate the id
substring in the parsing block before setting outId or outHasId: parse with an
end pointer and errno checks, require at least one digit, reject overflow and
any trailing characters, and return the existing bad-request response for
malformed ids. Preserve successful parsing for clean decimal ids and avoid
marking outHasId when validation fails.

src/ui/app.js (1)

1783-1796: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

uint8 field editor doesn't guard against NaN or clamp to f.min/f.max before sending.

Unlike the other numeric control renderers in this file (e.g. the pin and stepper cases, which fall back on NaN and clamp with Math.max/Math.min), this path sends parseInt(inp.value) straight to listSetField. An empty/invalid input yields NaN, which JSON.stringify turns into value:null — sent to the server instead of a sane fallback, and the browser's min/max attributes alone don't stop a typed out-of-range value from being submitted in every browser.

🛡️ Proposed fix

             inp.addEventListener("change", () => {
                 dragTs[dragKey] = Date.now();
-                listSetField(moduleName, ctrlName, id, f.name, parseInt(inp.value));
+                let v = parseInt(inp.value, 10);
+                if (Number.isNaN(v)) v = f.value ?? 0;
+                if (f.min !== undefined) v = Math.max(f.min, v);
+                if (f.max !== undefined) v = Math.min(f.max, v);
+                listSetField(moduleName, ctrlName, id, f.name, v);
             });

🤖 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/ui/app.js` around lines 1783 - 1796, Update the uint8 editor’s change
handler in the uint8 field renderer to parse the input with a safe fallback and
clamp the resulting value to f.min and f.max before calling listSetField.
Preserve the existing timestamp update and ensure invalid or empty input never
sends NaN.

src/core/HttpServerModule.h (1)

42-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Class doc still describes the pre-diff-on-wire behavior.

"Server pushes full state JSON as text frames from tick1s()" is no longer accurate — tick1s()/pushStateToWebSockets() now sends the full state only on connect or after a structural change, and a {"patch":[...]} of changed leaves otherwise (see buildStatePatch in the .cpp). This doc block wasn't touched by this PR and now misdescribes current behavior to future readers.

As per coding guidelines, "Use present tense in code and comments; do not describe historical removals, former behavior, plans, or future behavior."

📝 Proposed fix

-/// base64), up to 4 concurrent clients. Server pushes full state JSON as text frames from
-/// `tick1s()`. Binary frames take two paths, both without a frame-sized buffer: a synchronous
+/// base64), up to 4 concurrent clients. `tick1s()` pushes the full state as text once — on
+/// connect or after a structural change — and a small `{"patch":[...]}` of changed leaves
+/// otherwise (see `buildStatePatch`), never re-serialising the whole ~30 KB tree every second.
+/// Binary frames take two paths, both without a frame-sized buffer: a synchronous

🤖 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/HttpServerModule.h` around lines 42 - 50, The HttpServerModule class
documentation should describe the current WebSocket state-update behavior:
update the tick1s()/pushStateToWebSockets() description to state that full state
JSON is sent on connection or structural changes, while changed leaves are
otherwise sent as a {"patch":[...]} message built by buildStatePatch. Keep the
existing framing, buffering, and client-limit details unchanged.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@CLAUDE.md`:
- Around line 40-41: Update the wording in the CLAUDE.md paragraph around
writeListOptionSets to use present tense instead of “was,” and use the British
spelling “serialise” for consistency with the surrounding text. Preserve the
existing explanation and performance details.

In `@src/core/Control.cpp`:
- Around line 193-195: Remove the detailed optionSets/optionsRef contract
comments near the shared option-set declaration in Control.cpp, leaving only a
brief reference to the authoritative documentation on ListSource in Control.h.
Preserve the declaration and behavior unchanged.

In `@src/core/HttpServerModule.cpp`:
- Line 1519: Remove the duplicated explanatory comments from all five
requestFullResync() call sites in HttpServerModule.cpp, leaving the calls
unchanged. Rely on the authoritative rationale documented with
requestFullResync() in HttpServerModule.h.
- Around line 2115-2145: Ensure the full-state resync branch around
fullResyncPending_ can proceed even when preview traffic keeps the shared
buffered sender busy. Give the resync priority by using a separate queue/sender
or preempting the preview transfer, while preserving chunked non-blocking
delivery and ensuring a newly connected client does not remain blank.
- Around line 855-879: Ensure metadata changes from rebuildControls() trigger a
full state resync instead of relying solely on buildStatePatch() control-value
hashing. Update the NetworkModule and HueDriver runtime rebuild paths to request
resynchronization whenever visible schema metadata changes, including hidden,
readonly, or options fields.

In `@src/light/drivers/Correction.h`:
- Around line 151-167: Update Correction::rebuild() so ChannelRole::WarmWhite is
cached alongside ChannelRole::White, ensuring apply() writes the warm-white
channel for warm-white-only and RGB+CCT presets. Preserve the existing
white-mode behavior and add a regression test covering a warm-white-only preset.

In `@test/unit/light/unit_Correction.cpp`:
- Around line 134-136: Update the whiteMode None comment in the RGBW correction
test to state that the white channel is forced or written to 0 each frame,
matching the out[3] == 0 assertion and the authoritative inline comment; remove
the misleading “untouched” wording.

---

Outside diff comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 1910-1935: Update HttpServerModule::afterListMutation() to call
scheduler_->requestFullResync() after rebuilding controls and corrections,
ensuring every connected WebSocket client receives changed option metadata after
a list mutation. Keep the existing dirty-marking, control rebuilding, and
correction refresh behavior unchanged.
- Around line 1958-1963: Update the reorder handling in the JSON “to” branch of
HttpServerModule to validate the parsed integer before converting it to uint8_t,
rejecting values above the valid list boundary instead of allowing wraparound.
Mirror the bounds-checking behavior used by handleMoveModule, then call
moveListRow only with a validated index and preserve the existing 400 response
for invalid moves.
- Around line 1884-1887: Validate the id substring in the parsing block before
setting outId or outHasId: parse with an end pointer and errno checks, require
at least one digit, reject overflow and any trailing characters, and return the
existing bad-request response for malformed ids. Preserve successful parsing for
clean decimal ids and avoid marking outHasId when validation fails.

In `@src/core/HttpServerModule.h`:
- Around line 42-50: The HttpServerModule class documentation should describe
the current WebSocket state-update behavior: update the
tick1s()/pushStateToWebSockets() description to state that full state JSON is
sent on connection or structural changes, while changed leaves are otherwise
sent as a {"patch":[...]} message built by buildStatePatch. Keep the existing
framing, buffering, and client-limit details unchanged.

In `@src/ui/app.js`:
- Around line 1783-1796: Update the uint8 editor’s change handler in the uint8
field renderer to parse the input with a safe fallback and clamp the resulting
value to f.min and f.max before calling listSetField. Preserve the existing
timestamp update and ensure invalid or empty input never sends NaN.

🪄 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: 95a87e12-fd64-46f0-aba3-21237d544bae

📥 Commits

Reviewing files that changed from the base of the PR and between 5920ae6 and 0bcb3c5.

📒 Files selected for processing (21)

• CLAUDE.md
• docs/history/plans/Plan-20260711 - Diff-on-the-wire state push (send only what changed) (shipped).md
• docs/history/plans/Plan-20260711 - List detail on demand (fix 1Hz state-push stutter) (attempted, abandoned).md
• docs/history/plans/Plan-20260711 - State push off the render tick (resumable text frame) (shipped).md
• src/core/Control.cpp
• src/core/Control.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/JsonSink.h
• src/light/ChannelRole.h
• src/light/drivers/Correction.h
• src/light/drivers/LightPresetsModule.h
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/unit/core/unit_Control_list.cpp
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_HttpServerModule_apply.cpp
• test/unit/core/unit_JsonSink_detach.cpp
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_LightPresetsModule.cpp

[coderabbitai]

Actionable comments posted: 15

Caution

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

⚠️ Outside diff range comments (6)

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

14-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use American English spelling in comments.

Five instances in changed lines violate the coding guideline for **/*.{cpp,h,hpp,cc,cxx}: "colour" (lines 14, 232), "behaviour" (lines 134, 234), and "synthesised" (line 231) should be "color", "behavior", and "synthesized" respectively.

✏️ Proposed fixes

 // outChannels bytes with a named offset per colour role (offRed/offGreen/offBlue, and
-// outChannels bytes with a named offset per colour role (offRed/offGreen/offBlue, and
+// outChannels bytes with a named offset per color role (offRed/offGreen/offBlue, and

 // this is the byte-identical behaviour the earlier RGBW tests already pin. whiteMode =
-// this is the byte-identical behaviour the earlier RGBW tests already pin. whiteMode =
+// this is the byte-identical behavior the earlier RGBW tests already pin. whiteMode =

-// WarmWhite / Yellow / UV are synthesised from RGB off the SAME whiteMode as White, so a fixture
-// carrying them lights up (best-effort approximations, not a colour model yet): WW ≈ min(RGB),
-// Yellow ≈ min(R,G), UV ≈ the blue-excess max(0, B-max(R,G)). This is the "all channels burn so
-// you can eyeball a fixture" behaviour the finding asked for; a real per-emitter model comes later.
+// WarmWhite / Yellow / UV are synthesized from RGB off the SAME whiteMode as White, so a fixture
+// carrying them lights up (best-effort approximations, not a color model yet): WW ≈ min(RGB),
+// Yellow ≈ min(R,G), UV ≈ the blue-excess max(0, B-max(R,G)). This is the "all channels burn so
+// you can eyeball a fixture" behavior the finding asked for; a real per-emitter model comes later.

Also applies to: 134-134, 231-234

🤖 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_Correction.cpp` at line 14, Update the changed comments
in unit_Correction.cpp to use American English spelling: replace “colour” with
“color,” “behaviour” with “behavior,” and “synthesised” with “synthesized.”

Source: Coding guidelines

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

115-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Yellow/UV pick up the white-subtracted RGB when White + Yellow/UV roles coexist in Accurate mode.

r -= w; g -= w; b -= w; (inside the offWhite != kAbsent block) mutates r/g/b in place, and the offYellow/offUV lines run AFTER it, reading the mutated values. The comment right above states the other emitters "are additive stand-ins only ... so they don't subtract," but for a fixture that carries both a cold-White channel and a Yellow/UV channel, Yellow/UV silently end up computed from the desaturated RGB when whiteMode == Accurate. offWarmWhite is unaffected (it only reads the pre-subtraction w).

🐛 Proposed fix — compute the additive stand-ins before the Accurate subtraction

         } else {
             const uint8_t w = r < g ? (r < b ? r : b) : (g < b ? g : b);  // min(r,g,b): the white component
-            if (offWhite != kAbsent) {
-                if (whiteMode == WhiteMode::Accurate) { r -= w; g -= w; b -= w; }  // pull white out of RGB
-                out[offWhite] = w;
-            }
             // warm white ≈ the white component (same as cold white for a warm-white-only strip).
             if (offWarmWhite != kAbsent) out[offWarmWhite] = w;
             // yellow ≈ min(R,G) (the shared red+green component).
             if (offYellow != kAbsent)    out[offYellow] = r < g ? r : g;
             // UV is out of gamut...
             if (offUV != kAbsent) {
                 const uint8_t rg = r > g ? r : g;
                 out[offUV] = b > rg ? static_cast<uint8_t>(b - rg) : 0;
             }
+            // White (and its Accurate-mode RGB subtraction) runs LAST so it doesn't leak into the
+            // additive stand-ins above, which must read the ORIGINAL RGB per their own contract.
+            if (offWhite != kAbsent) {
+                if (whiteMode == WhiteMode::Accurate) { r -= w; g -= w; b -= w; }  // pull white out of RGB
+                out[offWhite] = w;
+            }
         }

🤖 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/Correction.h` around lines 115 - 150, Update apply so the
additive stand-ins for offYellow and offUV are computed from the original
corrected RGB values before WhiteMode::Accurate subtracts w from r, g, and b.
Preserve the existing offWhite subtraction and RGB output behavior, and leave
offWarmWhite based on the pre-subtraction w.

docs/backlog/backlog-light.md (1)

11-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

"optimisation" → "optimization" in freshly-written text.

This whole increment section (folding in the old "1..8-pin LCD" item) is new prose in this PR; docs/coding-standards.md (added in this same PR) requires US spelling everywhere, converted opportunistically when the file is touched.

🤖 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-light.md` around lines 11 - 23, Change the newly added
prose in the “16-lane parallel output + variable 1..16 lanes + transpose
optimization” section to use the US spelling “optimization” instead of
“optimisation,” including the heading and any occurrences in that section.

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

318-337: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Stale "Grow-only" comment now contradicts the new exact-match reuse logic.

The unchanged header comment at line 320 ("Grow-only; the buffer is re-zeroed on every reinit so a shrunken frame's latch pad can't hold stale row bytes") describes the OLD >=-based reuse (never shrinks, reuses whenever big enough). The new logic below it requires an EXACT match (==), meaning it now rebuilds on shrink AND on an oversized-but-not-exact capacity — no longer "grow-only" reuse semantics. Two comments now state overlapping, conflicting facts about the same reinit() behavior.

✏️ Proposed fix

     // --- bus + DMA buffer (hardware; this peripheral's targets only). Fused on
     // purpose: max_transfer_bytes is fixed at bus creation, so growing the frame
-    // means re-creating the bus and its buffer together. Grow-only; the buffer is
-    // re-zeroed on every reinit so a shrunken frame's latch pad can't hold stale
-    // row bytes. ---
+    // means re-creating the bus and its buffer together; see reinit() below for the
+    // exact-match reuse rule and why grow-only reuse was replaced. ---

As per coding guidelines, **/*.{cpp,h,md}: "Do not duplicate logic, facts, or explanatory comments; keep a fact in one authoritative location and reference it elsewhere."

🤖 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/ParallelLedDriver.h` around lines 318 - 337, Update the
header comment above reinit() to remove the outdated “Grow-only” behavior and
avoid duplicating the detailed exact-match reuse explanation below. Keep only
the authoritative facts that the bus and DMA buffer are rebuilt together when
required and that the buffer is cleared during reinitialization, consistent with
the current exact-match condition in reinit().

Source: Coding guidelines

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

213-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Treat a zero preset id as unresolved here src/light/drivers/DriverBase.h:213-239 — on a fresh driver, presetId_ starts at 0 while constructors like RmtLedDriver/ParallelLedDriver set presetRef_ to "GRB". The current presetSel_ < n && idAt(presetSel_) != presetId_ check fires immediately and rewrites the durable reference to the first built-in ("RGB"), so the driver loses its intended default. Existing tests cover the "RGB" path and a user-picked preset, but not this fresh-"GRB" case.

🤖 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/DriverBase.h` around lines 213 - 239, The user-pick
detection in buildPresetOptions must not treat presetId_ == 0 as an established
selection. Update the condition around presetSel_, presetId_, and lib->idAt() so
a zero id is resolved through the persisted presetRef_ first, preserving
constructor defaults such as "GRB"; retain the existing user-picked behavior
once presetId_ is nonzero.

src/core/HttpServerModule.cpp (1)

2149-2169: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Don't preempt an in-flight owned state frame here. cancelBufferedSend() drops the current WS message and resets the cursors, so a second resync can splice a new header into the middle of an already-partially-sent frame and corrupt the stream for any client that has started draining the old one. Only cancel borrowed preview frames, and leave fullResyncPending_ set until startBufferedTextSend() can actually take ownership.

🤖 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/HttpServerModule.cpp` around lines 2149 - 2169, Update the
fullResyncPending_ handling to avoid canceling an in-flight owned state frame:
only cancel buffered borrowed preview frames, and preserve an active owned state
send. Keep fullResyncPending_ true until startBufferedTextSend() successfully
accepts the new payload, then establish the baseline and clear the flag.

🤖 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/backlog/backlog-light.md`:
- Around line 161-167: Update the Parlio chunked transfer bullet to use the
American spelling “behavior” instead of “behaviour” in the parallel-I2S driver
sentence, leaving the rest of the bullet unchanged.

In `@docs/backlog/multicore-analysis-bottom-up.md`:
- Line 3: Fix the broken Appendix A link in the document introduction by
changing the fragment to the generated Markdown anchor for “Appendix A — frame
pacing, decided against,” or add and reference an explicit stable anchor for
that appendix. Ensure the resulting fragment satisfies markdownlint MD051.

In `@docs/backlog/multicore-analysis-top-down.md`:
- Around line 11-17: Add the text language identifier to the opening fenced
block containing the Step 0–Step 3 diagram, changing it from an untyped fence to
a text fence while preserving the diagram content and formatting.

In `@docs/performance.md`:
- Around line 239-251: Update the performance section to use current-state,
present-tense wording throughout, removing historical phrases such as “after ...
landed” and future or backlog references such as “coming” and “backlogged.” Keep
only currently verified behavior, limits, diagnostics, and acceptance floors;
move pending work or deferred status details to the appropriate backlog
documentation.

In `@src/core/DevicesModule.h`:
- Around line 309-310: Update the documentation comment for colorCount to use
the American spelling “color-capable” instead of “colour-capable,” leaving the
declaration and remaining comment text unchanged.
- Around line 162-166: The restoreList() color-count migration must preserve
values stored under the legacy "colour" key. Read "color" first, fall back to
"colour" when it is absent, retain the existing 0–127 clamping, and add a
regression test covering restoration from a legacy persisted file.

In `@src/core/HttpServerModule.h`:
- Around line 43-74: Update the rewritten class documentation in
HttpServerModule, replacing the British spellings “recognisable,”
“re-serialising,” and both occurrences of “serialise/serialises” with their
American spellings while preserving the surrounding meaning and formatting.
- Around line 137-147: Update cancelBufferedSend() so preview rebuild
cancellation cannot discard an in-flight state resync. Track or preserve whether
the active buffered frame is a full-state send, and when cancelling that frame
restore fullResyncPending_ (or equivalent pending-state behavior) so the resync
is retried; keep preview buffer ownership cleanup unchanged.

In `@src/core/MoonModule.h`:
- Around line 209-216: The rebuildControls() hook currently triggers full
WebSocket resynchronization for ordinary value updates. Update the
schemaChangedHook_ invocation so it runs only when control metadata or options
change, or move the notification to callers that perform schema changes;
preserve the existing value-patch path for Scheduler::setControl() updates.

In `@src/light/drivers/Correction.h`:
- Around line 62-73: Update the newly added comments near the synthesized
emitter handling to use US spelling: replace “synthesised” with “synthesized”
and “behaviour” with “behavior” in the comments around the
offWarmWhite/offYellow/offUV declarations and the related lines near the “Every
synthesised emitter” and “standard RGBW auto-white behaviour” references.

In `@src/light/drivers/LcdLedDriver.h`:
- Around line 68-85: The validateBusPins method must also reject an identical
configured clockPin and dcPin before iterating over data lanes. Add a direct
check for both pins being set and equal, return an appropriate validation error,
and preserve the existing lane-collision checks and nullptr success path.

In `@src/platform/esp32/platform_esp32_parlio.cpp`:
- Around line 135-157: Update the comments in parlioWs2812Init and its
surrounding transfer-limit explanation to use American English spelling by
replacing both occurrences of “colour” with “color”; leave the guard logic and
kParlioMaxTransferBytes unchanged.

In `@test/unit/core/unit_HttpServerModule_apply.cpp`:
- Around line 312-330: Add a test-only setup seam in HttpServerModule that
installs the schema-changed hook without binding or starting the TCP listener,
then update the “schema-changed hook: rebuildControls() requests a full WS
resync” test to use it instead of http.setup(). Preserve the existing hook,
resync assertions, and cleanup behavior.

In `@test/unit/light/correction_presets.h`:
- Line 3: Update the comment describing the Correction helper to use the
American English spelling “color” instead of “colour”; do not change the
helper’s behavior or surrounding documentation.

In `@test/unit/light/unit_LcdLedEncoder.cpp`:
- Around line 115-139: Update the comment above the test case to replace the
British spelling “behaviour-identical” with the American spelling
“behavior-identical”; leave the exhaustive validation logic unchanged.

---

Outside diff comments:
In `@docs/backlog/backlog-light.md`:
- Around line 11-23: Change the newly added prose in the “16-lane parallel
output + variable 1..16 lanes + transpose optimization” section to use the US
spelling “optimization” instead of “optimisation,” including the heading and any
occurrences in that section.

In `@src/core/HttpServerModule.cpp`:
- Around line 2149-2169: Update the fullResyncPending_ handling to avoid
canceling an in-flight owned state frame: only cancel buffered borrowed preview
frames, and preserve an active owned state send. Keep fullResyncPending_ true
until startBufferedTextSend() successfully accepts the new payload, then
establish the baseline and clear the flag.

In `@src/light/drivers/Correction.h`:
- Around line 115-150: Update apply so the additive stand-ins for offYellow and
offUV are computed from the original corrected RGB values before
WhiteMode::Accurate subtracts w from r, g, and b. Preserve the existing offWhite
subtraction and RGB output behavior, and leave offWarmWhite based on the
pre-subtraction w.

In `@src/light/drivers/DriverBase.h`:
- Around line 213-239: The user-pick detection in buildPresetOptions must not
treat presetId_ == 0 as an established selection. Update the condition around
presetSel_, presetId_, and lib->idAt() so a zero id is resolved through the
persisted presetRef_ first, preserving constructor defaults such as "GRB";
retain the existing user-picked behavior once presetId_ is nonzero.

In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 318-337: Update the header comment above reinit() to remove the
outdated “Grow-only” behavior and avoid duplicating the detailed exact-match
reuse explanation below. Keep only the authoritative facts that the bus and DMA
buffer are rebuilt together when required and that the buffer is cleared during
reinitialization, consistent with the current exact-match condition in reinit().

In `@test/unit/light/unit_Correction.cpp`:
- Line 14: Update the changed comments in unit_Correction.cpp to use American
English spelling: replace “colour” with “color,” “behaviour” with “behavior,”
and “synthesised” with “synthesized.”

🪄 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: 6734cb73-b382-4524-accb-ab04a42c3ec4

📥 Commits

Reviewing files that changed from the base of the PR and between 0bcb3c5 and 5e7f525.

📒 Files selected for processing (39)

• CLAUDE.md
• docs/backlog/README.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/multicore-analysis-bottom-up.md
• docs/backlog/multicore-analysis-top-down.md
• docs/backlog/pins-analysis-top-down.md
• docs/coding-standards.md
• docs/performance.md
• docs/reference/gpio-usage.md
• src/core/Control.cpp
• src/core/DevicesModule.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/MoonModule.h
• src/light/drivers/Correction.h
• src/light/drivers/DriverBase.h
• src/light/drivers/HueDriver.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/LcdSlots.h
• src/light/drivers/LightPresetsModule.h
• src/light/drivers/ParallelLedDriver.h
• src/light/effects/TextEffect.h
• src/platform/esp32/platform_esp32_parlio.cpp
• src/ui/app.js
• test/unit/core/unit_DevicesModule_hue.cpp
• test/unit/core/unit_HttpServerModule_apply.cpp
• test/unit/light/correction_presets.h
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_HueDriver.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_LcdLedEncoder.cpp
• test/unit/light/unit_LightPresetsModule.cpp
• test/unit/light/unit_NetworkSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_ParlioLedDriver.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp
• test/unit/light/unit_RmtLedEncoder.cpp
• web-installer/deviceModels.json

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve the legacy persisted Hue count during the key rename.

restoreList() reads only "color", while devices persisted by the previous schema contain "colour". On upgrade, the missing key becomes zero, so layout sizing and UI metadata lose the cached count until a live Hue report arrives—and may never recover while the bridge is offline. Read "color" first and fall back to "colour", and add a legacy-file regression test.

Proposed migration

-                const long color = mm::json::readInt(mm::json::member(doc, el, "color"));
+                const mm::json::JsonNode* colorNode = mm::json::member(doc, el, "color");
+                if (!colorNode) colorNode = mm::json::member(doc, el, "colour");
+                const long color = mm::json::readInt(colorNode);

📝 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

	// Clamp the persisted count to the valid range (0..127, the HueDriver's
	// colourCount_ range) so a corrupt or hand-edited entry can't wrap into a bogus
	// colorCount_ range) so a corrupt or hand-edited entry can't wrap into a bogus
	// value when narrowed. 0 for non-bridge rows (the key is absent → readInt = 0).
	const long colour = mm::json::readInt(mm::json::member(doc, el, "colour"));
	d.colourCount = static_cast<uint8_t>(colour < 0 ? 0 : (colour > 127 ? 127 : colour));
	const long color = mm::json::readInt(mm::json::member(doc, el, "color"));
	d.colorCount = static_cast<uint8_t>(color < 0 ? 0 : (color > 127 ? 127 : color));
	// Clamp the persisted count to the valid range (0..127, the HueDriver's
	// colorCount_ range) so a corrupt or hand-edited entry can't wrap into a bogus
	// value when narrowed. 0 for non-bridge rows (the key is absent → readInt = 0).
	const mm::json::JsonNode* colorNode = mm::json::member(doc, el, "color");
	if (!colorNode) colorNode = mm::json::member(doc, el, "colour");
	const long color = mm::json::readInt(colorNode);
	d.colorCount = static_cast<uint8_t>(color < 0 ? 0 : (color > 127 ? 127 : color));

🤖 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/DevicesModule.h` around lines 162 - 166, The restoreList()
color-count migration must preserve values stored under the legacy "colour" key.
Read "color" first, fall back to "colour" when it is absent, retain the existing
0–127 clamping, and add a regression test covering restoration from a legacy
persisted file.

[coderabbitai]

Actionable comments posted: 16

Caution

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

⚠️ Outside diff range comments (1)

moondeck/moondeck_ui/app.js (1)

676-708: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Hint can describe a port no longer in the dropdown.

If the persisted current port isn't in the new ports list, the <select> falls back to the blank "--" option, but updatePortDeviceHint(current) still renders the hint for the (now absent) stale port — a mismatch between what's shown selected and what the hint describes.

🔧 Proposed fix

-    updatePortDeviceHint(current);
+    updatePortDeviceHint(select.value);

🤖 Prompt for AI Agents

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

In `@moondeck/moondeck_ui/app.js` around lines 676 - 708, Update renderPortOptions
so the hint uses the port actually represented by the rebuilt select: detect
whether current matches an entry in ports, use current only when present,
otherwise use the blank selection value, and pass that effective value to
updatePortDeviceHint.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In
`@docs/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md`:
- Around line 7-11: The ADR narrative uses past-tense descriptions of migration
code and repository history. Update the affected sentences to describe the
current repository state in present tense, including the statements about the
two migrations and their removal, while preserving the existing meaning and
surrounding rationale.
- Around line 23-25: Update the “Known breaking changes” section to document the
persistence key rename from `colour` to `color` used by
DevicesModule::restoreList(), noting that cached Hue counts stored under
`colour` reset to zero until the bridge reconnects.

In `@docs/backlog/backlog-core.md`:
- Line 228: Update the backlog documentation wording from “do not
over-generalise” to “do not over-generalize,” preserving the surrounding scope
and meaning of the lane-ceiling guidance.

In `@docs/backlog/backlog-light.md`:
- Line 13: Update the design-record link in the backlog entry to reference the
shipped plan filename, preserving the existing relative path and link text while
adding the “(shipped)” suffix before the .md extension.

In `@docs/backlog/multicore-analysis-top-down.md`:
- Line 16: Update the dependency-graph documentation line to use the American
spelling “optimization” instead of “optimisation,” leaving the diagram structure
and surrounding text unchanged.

In `@docs/history/plans/Plan-20260712` - 16-lane parallel LED output (shipped).md:
- Around line 21-80: Add blank lines after each flagged Markdown heading and
before and after the fenced code block in the plan document, including the
headings around the listed sections. Preserve all existing text and code content
while making the file satisfy MD022 and MD031.

In `@moondeck/moondeck_ui/app.js`:
- Around line 710-750: Update refreshPorts and the identify-ports click handler
to catch fetch, non-2xx, JSON, and malformed-response failures instead of
allowing them to throw silently. Log a clear user-visible error through
appendLog; keep the Identify handler’s finally block so the button is always
restored, and validate the expected ports data before calling renderPortOptions.

In `@moondeck/moondeck_ui/index.html`:
- Around line 64-72: Update the new buttons identified by `identify-ports` and
`watch-ports` to explicitly declare `type="button"`, preserving their existing
labels, titles, and behavior.

In `@moondeck/moondeck.py`:
- Around line 1123-1135: Update the /api/identify-ports request handling around
probe_port_chip to validate requested paths against the current
list_serial_ports() result before probing. Build a known-port set and only call
probe_port_chip for requested paths present in that set, preserving the existing
response behavior for valid ports and excluding unknown paths.

In `@src/light/drivers/DriverBase.h`:
- Around line 253-263: Update DriverBase’s kWindowAll/count_ and the
windowSlice() handling so the default window cannot cap output at 65535 when
nrOfLightsType supports larger buffers. Use a non-colliding sentinel or preserve
the 0-as-all convention, ensuring a fresh driver expands its window to the full
bufN while explicit real counts remain unchanged.

In `@src/light/drivers/LcdLedDriver.h`:
- Around line 85-99: Update parseConfig() to detect the clockPin == dcPin
collision separately and route it through the configuration error path instead
of setConfigWarn. Preserve warning handling for per-lane collisions, and retain
validateBusPins()’s specific “they must differ” message.

In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 201-225: Initialize the local wire buffer in encodeRows with
zeroed storage, matching the analogous buffer in runLoopbackSelfTest. Keep the
existing lane population and encodeWs2812LcdSlots flow unchanged.

In `@src/platform/esp32/platform_config.h`:
- Around line 84-91: Update the comments immediately above lcdLanes to remove
the stale 8-lane/half-DMA-footprint rationale and describe the current 16-lane
limit. Revise the note about widening so it accurately states that the
configured maximum is controlled by changing this constant, while preserving the
existing explanation of valid LCD data-pin widths.

In `@src/platform/esp32/platform_esp32_lcd.cpp`:
- Around line 142-154: Update the pre-allocation check in lcdWs2812Init() so
initialization is allowed when either the internal DMA-capable heap or PSRAM can
provide bufferBytes, while retaining HEAP_RESERVE enforcement for internal RAM.
Ensure boards whose draw buffer fits only in PSRAM reach the PSRAM-first
createState() allocation path.

In `@test/unit/light/unit_Correction.cpp`:
- Line 196: Rename the affected test descriptions in the Correction roles array
tests from British spelling (“colour”/“colours”) to American spelling
(“color”/“colors”), including the tests at the referenced locations, without
changing their behavior.

In `@web-installer/deviceModels.json`:
- Around line 899-904: Update the SE 16 V1 device-model entry identified by id
"LcdLed" to explicitly configure clockPin and dcPin to GPIOs not used by its
16-lane controls.pins mapping, rather than relying on LcdLedDriver defaults of
10 and 11. Preserve the existing data-lane pin list and use the board’s correct
dedicated control-pin assignments.

---

Outside diff comments:
In `@moondeck/moondeck_ui/app.js`:
- Around line 676-708: Update renderPortOptions so the hint uses the port
actually represented by the rebuilt select: detect whether current matches an
entry in ports, use current only when present, otherwise use the blank selection
value, and pass that effective value to updatePortDeviceHint.

🪄 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: e7218ff5-ce63-47c5-84f1-3bf7f7652146

📥 Commits

Reviewing files that changed from the base of the PR and between 5e7f525 and fe8fd0d.

📒 Files selected for processing (44)

• docs/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md
• docs/adr/README.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/multicore-analysis-bottom-up.md
• docs/backlog/multicore-analysis-top-down.md
• docs/history/plans/Plan-20260712 - 16-lane parallel LED output (shipped).md
• docs/moonmodules/light/drivers.md
• docs/performance.md
• moondeck/moondeck.py
• moondeck/moondeck_ui/app.js
• moondeck/moondeck_ui/index.html
• moondeck/moondeck_ui/style.css
• src/core/DevicesModule.h
• src/core/FilesystemModule.cpp
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/MoonModule.h
• src/light/drivers/Correction.h
• src/light/drivers/DriverBase.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/LcdSlots.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/PinList.h
• src/light/drivers/RmtLedDriver.h
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_lcd.cpp
• src/platform/esp32/platform_esp32_parlio.cpp
• src/ui/app.js
• src/ui/install-picker-boards.js
• src/ui/install-picker.js
• test/python/test_moondeck_port_identity.py
• test/unit/core/unit_HttpServerModule_apply.cpp
• test/unit/core/unit_MoonModule.cpp
• test/unit/light/correction_presets.h
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_LcdLedEncoder.cpp
• test/unit/light/unit_ParlioLedDriver.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp
• web-installer/deviceModels.json
• web-installer/install.js

💤 Files with no reviewable changes (2)

• src/core/FilesystemModule.cpp
• src/core/FilesystemModule.h

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the ADR narrative in present tense.

These lines narrate prior actions (“had accumulated”, “were removed”), contrary to the documentation rule. Describe the current repository state instead.

As per coding guidelines, “docs/**/*.md: Write code, comments, and documentation in present tense.”

Also applies to: 21-21

🤖 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/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md`
around lines 7 - 11, The ADR narrative uses past-tense descriptions of migration
code and repository history. Update the affected sentences to describe the
current repository state in present tense, including the statements about the
two migrations and their removal, while preserving the existing meaning and
surrounding rationale.

Source: Coding guidelines

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (1)

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

270-272: ⚠️ Potential issue | 🟠 Major

Use a non-colliding “all lights” sentinel.

This repeats the previous finding: kWindowAll == 65535 is a valid explicit uint16_t count, but windowSlice() interprets it as “all.” On buffers larger than 65535 lights, setWindow(start, 65535) therefore drives the entire remainder instead of exactly 65535 lights. Preserve the existing 0 = all convention or use a wider/non-colliding representation.

Proposed fix

-    static constexpr uint16_t kWindowAll = 65535;
+    static constexpr uint16_t kWindowAll = 0;

-    uint16_t count_ = kWindowAll;
+    uint16_t count_ = kWindowAll;

-        outLen = (count_ == 0 || count_ == kWindowAll || count_ > avail)
+        outLen = (count_ == kWindowAll || count_ > avail)
                      ? avail
                      : static_cast<nrOfLightsType>(count_);

Also applies to: 297-305

🤖 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/DriverBase.h` around lines 270 - 272, Replace the colliding
kWindowAll sentinel used by DriverBase::count_ with the existing non-colliding 0
= all convention, and update windowSlice() plus setWindow() handling
accordingly. Preserve default behavior of driving all lights while ensuring an
explicit count of 65535 remains exactly 65535, including when buffers exceed
that size.

🤖 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/backlog/backlog-light.md`:
- Line 13: Update the deferred backlog entries for PSRAM-first DMA buffering and
async transmit/double-buffering to reflect that the work has shipped, removing
them from future implementation scope or deleting the obsolete draft entries.
Keep the shipped status and references consistent with the existing 16-lane
implementation and design record.

In `@docs/history/lessons.md`:
- Line 290: In the lesson text around the wireUs KPI, replace the British
spelling “optimisation” with the American spelling “optimization”; leave the
surrounding explanation unchanged.

In `@src/core/IrService.h`:
- Around line 220-223: Update IrService::reportReady() to release any previously
active IR channel via platform::irStop() before handling the current pin or
attempting platform::irChannelReady(). Ensure this runs for both valid-to--1
transitions and valid-to-valid pin changes, while preserving the existing status
reporting and readiness checks.

In `@test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp`:
- Around line 17-22: Update the comments near the double-buffer and
single-buffer behavior description to use the American spelling “behavior”
instead of “behaviour,” without changing the documented semantics.

---

Duplicate comments:
In `@src/light/drivers/DriverBase.h`:
- Around line 270-272: Replace the colliding kWindowAll sentinel used by
DriverBase::count_ with the existing non-colliding 0 = all convention, and
update windowSlice() plus setWindow() handling accordingly. Preserve default
behavior of driving all lights while ensuring an explicit count of 65535 remains
exactly 65535, including when buffers exceed that size.

🪄 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: 0b4f98dd-5d85-4a59-bb1e-927ca8e580af

📥 Commits

Reviewing files that changed from the base of the PR and between fe8fd0d and 3422f60.

📒 Files selected for processing (34)

• docs/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/multicore-analysis-bottom-up.md
• docs/backlog/multicore-analysis-top-down.md
• docs/history/lessons.md
• docs/history/plans/Plan-20260712 - 16-lane parallel LED output (shipped).md
• docs/history/plans/Plan-20260712 - Step 1.5 async transmit double-buffer (shipped).md
• docs/performance.md
• mkdocs.yml
• moondeck/moondeck.py
• moondeck/moondeck_ui/app.js
• moondeck/moondeck_ui/index.html
• src/core/IrService.h
• src/core/PinsModule.h
• src/light/drivers/DriverBase.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/LcdSlots.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32_ir.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• src/platform/esp32/platform_esp32_parlio.cpp
• src/platform/platform.h
• test/CMakeLists.txt
• test/scenarios/light/scenario_perf_full.json
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_LcdLedEncoder.cpp
• test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp
• web-installer/deviceModels.json

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove or update the shipped work from the deferred backlog.

Line 13 marks PSRAM-first DMA buffering as shipped, while the later PSRAM item still describes it as future work. Line 167 likewise keeps async transmit and double-buffering deferred despite the shipped plan and PR objective. Synchronize these entries to avoid directing duplicate implementation.

As per coding guidelines, “Use backlog documents for forward-looking work and delete a draft when the module ships.”

Also applies to: 167-167

🤖 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-light.md` at line 13, Update the deferred backlog
entries for PSRAM-first DMA buffering and async transmit/double-buffering to
reflect that the work has shipped, removing them from future implementation
scope or deleting the obsolete draft entries. Keep the shipped status and
references consistent with the existing 16-lane implementation and design
record.

Source: Coding guidelines

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use American English spelling: "optimisation" → "optimization".

As per coding guidelines, docs/**/*.{md,markdown} must use American English spelling.

🤖 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/lessons.md` at line 290, In the lesson text around the wireUs
KPI, replace the British spelling “optimisation” with the American spelling
“optimization”; leave the surrounding explanation unchanged.

Source: Coding guidelines

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Release the previous IR channel before recomputing readiness.

When a valid pin changes to -1, Line 221 returns without calling platform::irStop(), leaving the old RX channel active while the ownership map can treat that GPIO as free. Valid-to-valid pin changes also need the previous binding released before opening the new one.

Proposed fix

 void reportReady() {
+    platform::irStop();
     if (pin_ < 0) { setStatus("set pin to receive", Severity::Warning); return; }

As per coding guidelines, “Apply configuration changes live on the next render tick; configuration changes must not require a reboot or restart.”

📝 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

	void reportReady() {
	if (pin_ < 0) setStatus("set pin to receive", Severity::Warning);
	else setStatus("ready");
	if (pin_ < 0) { setStatus("set pin to receive", Severity::Warning); return; }
	if (platform::irChannelReady(static_cast<uint16_t>(pin_))) setStatus("ready");
	else setStatus("IR channel failed to open — pin busy or invalid?", Severity::Error);
	void reportReady() {
	platform::irStop();
	if (pin_ < 0) { setStatus("set pin to receive", Severity::Warning); return; }
	if (platform::irChannelReady(static_cast<uint16_t>(pin_))) setStatus("ready");
	else setStatus("IR channel failed to open — pin busy or invalid?", 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/core/IrService.h` around lines 220 - 223, Update IrService::reportReady()
to release any previously active IR channel via platform::irStop() before
handling the current pin or attempting platform::irChannelReady(). Ensure this
runs for both valid-to--1 transitions and valid-to-valid pin changes, while
preserving the existing status reporting and readiness checks.

Source: Coding guidelines

[coderabbitai]

Actionable comments posted: 9

Caution

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

⚠️ Outside diff range comments (9)

docs/backlog/multicore-analysis-top-down.md (3)

35-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Mark classic ESP32 I2S/i80 as shipped.

Both references still describe parallel I2S as forthcoming, contradicting the shipped driver record in backlog-light.md.

Also applies to: 124-124

🤖 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/multicore-analysis-top-down.md` at line 35, Update the classic
ESP32 I2S/i80 references in this backlog entry to mark the parallel-I2S driver
as shipped rather than forthcoming, matching the shipped driver status recorded
in backlog-light.md. Preserve the existing investigation and fix details.

---

90-92: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Resolve the contradictory multicore gate.

Line 90 says measured network starvation has opened the gate, but Line 92 says not to build until LED glitches are independently reproduced. Decide whether network starvation alone is an accepted trigger and state one gate.

🤖 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/multicore-analysis-top-down.md` around lines 90 - 92, Resolve
the contradiction in the multicore gate text by choosing one explicit acceptance
criterion: either make the measured network starvation sufficient to open the
gate and remove the LED-glitch prerequisite, or require the sigrok LED-output
glitch confirmation before implementation. Update the adjacent statements
consistently so they describe a single gate and next action.

---

75-84: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Recalculate Step 2 from the post-async measurements.

This section still uses encode ≈ 22 ms, DMA wait ≈ 0, and a 45-fps pipeline floor, while Lines 26–27 and 65–67 establish the current effect ≈ 7.3 ms, driver ≈ 3.8 ms, async-overlapped model. The table and conclusion therefore no longer support the proposed pipeline.

🤖 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/multicore-analysis-top-down.md` around lines 75 - 84,
Recalculate the “Why the pipeline, not split-encode” section using the
post-async measurements established in the surrounding document, including
effect ≈ 7.3 ms and driver ≈ 3.8 ms. Replace the outdated encode ≈ 22 ms,
DMA-wait ≈ 0, table values, and 45-fps pipeline conclusion with timings and
conclusions derived from the current async-overlapped model, while preserving
the section’s comparison of pipeline versus hybrid approaches.

docs/backlog/backlog-light.md (1)

169-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the stale “coming parallel-I2S driver” statement.

Lines 21–25 record classic ESP32 I2S/i80 as shipped, so its behavior is no longer TBD.

As per coding guidelines, backlog files contain forward-looking specifications and completed items are removed.

🤖 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-light.md` at line 169, Update the Parlio chunking
backlog entry to remove the stale statement that the parallel-I2S driver’s
behavior is TBD, since the driver is now shipped. Keep the remaining
Parlio-specific chunking details and completed-item guidance unchanged.

Source: Coding guidelines

docs/backlog/multicore-analysis-bottom-up.md (1)

7-24: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Update or archive this analysis; its core model is now obsolete.

It still claims 8-lane LCD/Parlio limits, negligible blocking transmit, single-buffer operation, a forced Parlio ring, and forthcoming classic I2S. The supplied implementation records 16-lane support, shipped whole-frame async double buffering, and shipped classic I2S/i80. These contradictions also invalidate the derived ceilings and build order.

As per coding guidelines, shared facts must live in one authoritative location, and backlog documents must remain forward-looking.

Also applies to: 31-40, 59-70, 72-129

🤖 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/multicore-analysis-bottom-up.md` around lines 7 - 24, The
analysis is obsolete and contradicts shipped driver capabilities and buffering
behavior. Update or archive this document rather than preserving its current
conclusions; remove or replace claims about 8-lane limits, negligible blocking
transmit, single-buffer operation, forced Parlio ring behavior, and forthcoming
classic I2S/i80, and revise derived ceilings and build ordering to reflect
16-lane support, whole-frame asynchronous double buffering, and shipped classic
I2S/i80. Keep shared facts aligned with their authoritative documentation and
ensure any remaining backlog content is explicitly forward-looking.

Source: Coding guidelines

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

14-14: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Document the preset reference as a stable ID, not a name.

The preset is editable and may be renamed; persistence survives because drivers store its stable ID.

Proposed correction

-- `preset` — the [light preset](supporting.md) this driver applies per light (channel order / RGBW synthesis), referenced by name so it survives a reboot.
+- `preset` — the [light preset](supporting.md) this driver applies per light (channel order / RGBW synthesis), referenced by stable ID so renaming it does not break persisted driver configuration.

🤖 Prompt for AI Agents

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

In `@docs/moonmodules/light/drivers.md` at line 14, Update the `preset` field
documentation in the light driver reference to state that it stores the preset’s
stable ID rather than its name, while retaining that it identifies the light
preset used for channel order and RGBW synthesis and persists across reboots.

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

36-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the class rationale to the same 1..16 lane limit.

Line 36 now permits 16 lanes, but the class comment above still says 1..8 lanes. Keep the documented contract consistent.

As per coding guidelines, “Do not duplicate logic or facts; centralize shared behavior and link to the single authoritative documentation location.”

🤖 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/ParlioLedDriver.h` around lines 36 - 39, Update the
class-level rationale associated with ParlioLedDriver to document the supported
1–16 lane range instead of 1–8, keeping it consistent with kExactLaneCount.
Avoid duplicating the lane-limit fact elsewhere; reference the authoritative
documentation if one already exists.

Source: Coding guidelines

docs/reference/gpio-usage.md (1)

22-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the P4 usable-pin count.

The listed ranges contain 20 pins, not approximately 24: 20–27 (8), 32–33 (2), and 39–48 (10).

-| **ESP32-P4 (P4-NANO)** | 20, 21, 22, 23, 24, 25, 26, 27, 32, 33, 39-48 | ~24 |
+| **ESP32-P4 (P4-NANO)** | 20, 21, 22, 23, 24, 25, 26, 27, 32, 33, 39-48 | 20 |

🤖 Prompt for AI Agents

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

In `@docs/reference/gpio-usage.md` at line 22, Update the ESP32-P4 (P4-NANO) row
in the GPIO usage table so its usable-pin count is 20 instead of approximately
24, leaving the listed pin ranges and board-specific notes unchanged.

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

838-848: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Exercise every supported i80 backend or explicitly exclude it.

The platform contract now supports classic ESP32, S3, and P4, but this scenario still says “S3 only.” Because the step is optional, classic/P4 can silently record measure-i80 without an active I80 driver. Add target-valid configurations or explicitly gate unsupported scenario targets.

As per path instructions, “Verify tests cover edge cases and match the specifications in docs/moonmodules/.”

🤖 Prompt for AI Agents

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

In `@test/scenarios/light/scenario_perf_full.json` around lines 838 - 848, Update
the add-i80-driver scenario step to exercise every supported I80 backend:
provide target-valid configurations for classic ESP32, S3, and P4, or explicitly
gate the step so unsupported targets cannot record measure-i80 without an active
I80LedDriver. Keep the existing S3 pin and LED configuration valid, and verify
coverage against docs/moonmodules/ specifications.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In
`@docs/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md`:
- Line 26: Update the ADR text describing the Hue bridge light count to use
American spelling: retain the legacy persisted key `colour`, but change the
surrounding “colour-capable” wording to “color-capable.”

In `@docs/backlog/backlog-light.md`:
- Line 23: Correct the classic ESP32 capacity statement in the backlog entry:
distinguish 8×256 as 2,048 lights from the RGB channel count of 6,144, and
update the hardware verification wording to identify which metric is being
recorded. Preserve the separate ~4096-light ceiling claim only if it is
accurately labeled as lights.

In `@docs/backlog/multicore-analysis-top-down.md`:
- Line 69: Resolve the contradictory Parlio chunking status between the
shipped-design statement and the later requirements section. Update the affected
documentation so it consistently either defers ring/chunking indefinitely or
identifies it as required for scaling from roughly 4K to 16K lights, while
preserving the chosen implementation scope throughout the document.

In `@docs/performance.md`:
- Line 247: Correct the 16384-light LCD_CAM performance entry to report
approximately 20 fps based on the 49,916 µs encode tick, while retaining the
separate wireUs measurement and async-overlap details. Update only the incorrect
frame-rate calculation in the LCD_CAM 16-lane row.

In `@src/light/drivers/I80LedDriver.h`:
- Around line 9-19: Update the documentation comment above the I80 LED driver to
reference the actual ESP-IDF symbols esp_lcd_new_i80_bus and
esp_lcd_panel_io_tx_color, and mention esp_lcd_panel_io_i80.c where the backend
implementation is described. Do not change the driver implementation or the
explanation of chip-specific peripherals.

In `@src/light/drivers/PinList.h`:
- Line 12: Update the documentation comment describing I80LedDriver to call the
resource an “i80 data lane” instead of an “LCD_CAM lane,” while preserving the
existing explanation that each pin uses a separate lane.

In `@src/light/drivers/RmtLedDriver.h`:
- Line 191: Centralize the grow-only wire scratch-buffer allocation and cleanup
in shared protected helpers on DriverBase, including wire_ and wireCap_. Update
RmtLedDriver::resizeSymbols(), release(), and readiness checks, plus
ParallelLedDriver::prepareWire() and teardown paths, to use the shared
ensureWire/freeWire implementation while preserving allocation-failure and
capacity behavior.

In `@src/platform/esp32/platform_esp32_parlio.cpp`:
- Line 82: Update the transfer timing logic around lastTransmitUs and txStartUs
so each buffer’s wire-time measurement begins when hardware starts transmitting
it, not when it is merely enqueued. Record the next transfer’s start timestamp
from the preceding completion, or use the available hardware-start event, and
ensure queued buffer 1 excludes buffer 0’s remaining transmission time while
preserving the existing completion measurement.
- Around line 258-263: The parlioWs2812Wait path must report whether the
semaphore was actually acquired and prevent buffer reuse after a timeout. Update
parlioWs2812Wait and ParallelLedDriver::busWaitIfBusy to propagate completion
status, only clear inFlight_ after successful completion, and abort/reset the TX
unit before allowing reuse or teardown when the wait times out.

---

Outside diff comments:
In `@docs/backlog/backlog-light.md`:
- Line 169: Update the Parlio chunking backlog entry to remove the stale
statement that the parallel-I2S driver’s behavior is TBD, since the driver is
now shipped. Keep the remaining Parlio-specific chunking details and
completed-item guidance unchanged.

In `@docs/backlog/multicore-analysis-bottom-up.md`:
- Around line 7-24: The analysis is obsolete and contradicts shipped driver
capabilities and buffering behavior. Update or archive this document rather than
preserving its current conclusions; remove or replace claims about 8-lane
limits, negligible blocking transmit, single-buffer operation, forced Parlio
ring behavior, and forthcoming classic I2S/i80, and revise derived ceilings and
build ordering to reflect 16-lane support, whole-frame asynchronous double
buffering, and shipped classic I2S/i80. Keep shared facts aligned with their
authoritative documentation and ensure any remaining backlog content is
explicitly forward-looking.

In `@docs/backlog/multicore-analysis-top-down.md`:
- Line 35: Update the classic ESP32 I2S/i80 references in this backlog entry to
mark the parallel-I2S driver as shipped rather than forthcoming, matching the
shipped driver status recorded in backlog-light.md. Preserve the existing
investigation and fix details.
- Around line 90-92: Resolve the contradiction in the multicore gate text by
choosing one explicit acceptance criterion: either make the measured network
starvation sufficient to open the gate and remove the LED-glitch prerequisite,
or require the sigrok LED-output glitch confirmation before implementation.
Update the adjacent statements consistently so they describe a single gate and
next action.
- Around line 75-84: Recalculate the “Why the pipeline, not split-encode”
section using the post-async measurements established in the surrounding
document, including effect ≈ 7.3 ms and driver ≈ 3.8 ms. Replace the outdated
encode ≈ 22 ms, DMA-wait ≈ 0, table values, and 45-fps pipeline conclusion with
timings and conclusions derived from the current async-overlapped model, while
preserving the section’s comparison of pipeline versus hybrid approaches.

In `@docs/moonmodules/light/drivers.md`:
- Line 14: Update the `preset` field documentation in the light driver reference
to state that it stores the preset’s stable ID rather than its name, while
retaining that it identifies the light preset used for channel order and RGBW
synthesis and persists across reboots.

In `@docs/reference/gpio-usage.md`:
- Line 22: Update the ESP32-P4 (P4-NANO) row in the GPIO usage table so its
usable-pin count is 20 instead of approximately 24, leaving the listed pin
ranges and board-specific notes unchanged.

In `@src/light/drivers/ParlioLedDriver.h`:
- Around line 36-39: Update the class-level rationale associated with
ParlioLedDriver to document the supported 1–16 lane range instead of 1–8,
keeping it consistent with kExactLaneCount. Avoid duplicating the lane-limit
fact elsewhere; reference the authoritative documentation if one already exists.

In `@test/scenarios/light/scenario_perf_full.json`:
- Around line 838-848: Update the add-i80-driver scenario step to exercise every
supported I80 backend: provide target-valid configurations for classic ESP32,
S3, and P4, or explicitly gate the step so unsupported targets cannot record
measure-i80 without an active I80LedDriver. Keep the existing S3 pin and LED
configuration valid, and verify coverage against docs/moonmodules/
specifications.

🪄 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: 4c1e01c7-3af9-479f-aa02-9e6d32d06079

📥 Commits

Reviewing files that changed from the base of the PR and between fe8fd0d and f96cf22.

⛔ Files ignored due to path filters (3)

• docs/assets/deviceModels/esp32-d0-wrover.jpg is excluded by !**/*.jpg
• docs/assets/deviceModels/esp32-wrover.jpg is excluded by !**/*.jpg
• moondeck/build/build_esp32.py is excluded by !**/build/**

📒 Files selected for processing (52)

• docs/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/multicore-analysis-bottom-up.md
• docs/backlog/multicore-analysis-top-down.md
• docs/history/lessons.md
• docs/history/plans/Plan-20260712 - 16-lane parallel LED output (shipped).md
• docs/history/plans/Plan-20260712 - Step 1.5 async transmit double-buffer (shipped).md
• docs/moonmodules/light/drivers.md
• docs/performance.md
• docs/reference/gpio-usage.md
• esp32/main/CMakeLists.txt
• esp32/sdkconfig.defaults.wrover
• mkdocs.yml
• moondeck/moondeck.py
• moondeck/moondeck_ui/app.js
• moondeck/moondeck_ui/index.html
• src/core/AudioLevel.h
• src/core/IrService.h
• src/core/PinList.h
• src/core/PinsModule.h
• src/core/SystemModule.h
• src/light/drivers/DriverBase.h
• src/light/drivers/I80LedDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParallelSlots.h
• src/light/drivers/ParlioLedDriver.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.cpp
• src/platform/esp32/platform_esp32_i80.cpp
• src/platform/esp32/platform_esp32_ir.cpp
• src/platform/esp32/platform_esp32_lcd.cpp
• src/platform/esp32/platform_esp32_parlio.cpp
• src/platform/esp32/platform_esp32_rmt.cpp
• src/platform/platform.h
• test/CMakeLists.txt
• test/scenarios/light/scenario_Driver_mutation.json
• test/scenarios/light/scenario_perf_full.json
• test/unit/core/unit_PinsModule.cpp
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_I80LedDriver.cpp
• test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp
• test/unit/light/unit_ParallelSlots.cpp
• test/unit/light/unit_ParlioLedDriver.cpp
• test/unit/light/unit_RmtLedDriver_pins.cpp
• web-installer/deviceModels.json
• web-installer/firmwares.json

💤 Files with no reviewable changes (1)

• src/platform/esp32/platform_esp32_lcd.cpp

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Choose one status for Parlio chunking.

Line 69 defers the ring/chunking work indefinitely, while Line 123 calls chunking the required path from roughly 4K to 16K lights. These are mutually exclusive implementation decisions.

Also applies to: 123-123

🤖 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/multicore-analysis-top-down.md` at line 69, Resolve the
contradictory Parlio chunking status between the shipped-design statement and
the later requirements section. Update the affected documentation so it
consistently either defers ring/chunking indefinitely or identifies it as
required for scaling from roughly 4K to 16K lights, while preserving the chosen
implementation scope throughout the document.

[coderabbitai]

📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

🧩 Analysis chain

🌐 Web query:

ESP-IDF esp_lcd i80 bus API function name esp_lcd_new_i80_bus

💡 Result:

The function esp_lcd_new_i80_bus is part of the ESP-IDF esp_lcd component and is used to create an Intel 8080 (I80) parallel bus handle for LCD interfacing [1][2]. Function Signature: esp_err_t esp_lcd_new_i80_bus(const esp_lcd_i80_bus_config_t *bus_config, esp_lcd_i80_bus_handle_t *ret_bus); Key Details: - Purpose: It initializes the I80 bus, which is required before creating an LCD panel IO device [1][3]. - Configuration: It accepts a pointer to an esp_lcd_i80_bus_config_t structure, where you define parameters such as the data bus width (8 or 16 bits), GPIO assignments (DC, WR, and data lines), the clock source, and maximum transfer size [1][2]. - Return Value: It returns an esp_err_t (e.g., ESP_OK on success, or error codes like ESP_ERR_INVALID_ARG or ESP_ERR_NO_MEM) [2]. The resulting bus handle is stored in the variable provided via ret_bus [2]. To use this function, ensure your component's CMakeLists.txt lists esp_lcd as a dependency [1][4]. For more information, refer to the official ESP-IDF Programming Guide documentation under the "I80 Interfaced LCD" section [1][5][4].

Citations:

• 1: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/lcd/i80_lcd.html
• 2: https://github.com/espressif/esp-idf/blob/v6.0.1/components/esp_lcd/include/esp_lcd_io_i80.h
• 3: https://docs.espressif.com/projects/esp-idf/en/v5.5.3/esp32/api-reference/peripherals/lcd/i80_lcd.html
• 4: https://docs.espressif.com/projects/esp-idf/en/v6.0/esp32/api-reference/peripherals/lcd/i80_lcd.html
• 5: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/lcd/i80_lcd.html

---

🏁 Script executed:

sed -n '1,80p' src/light/drivers/I80LedDriver.h

Repository: MoonModules/projectMM

Length of output: 1965

---

Fix the ESP-IDF symbol names in the doc comment. The /// block should reference esp_lcd_new_i80_bus / esp_lcd_panel_io_tx_color (and esp_lcd_panel_io_i80.c) instead of esp_i80_*; the current names mislead readers grepping for the real API.

🤖 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/I80LedDriver.h` around lines 9 - 19, Update the
documentation comment above the I80 LED driver to reference the actual ESP-IDF
symbols esp_lcd_new_i80_bus and esp_lcd_panel_io_tx_color, and mention
esp_lcd_panel_io_i80.c where the backend implementation is described. Do not
change the driver implementation or the explanation of chip-specific
peripherals.

[coderabbitai]

Actionable comments posted: 13

Caution

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

⚠️ Outside diff range comments (7)

src/core/HttpServerModule.cpp (1)

2122-2136: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Use cancelBufferedSend() before accepting the new client. previewSend_.active = false; skips the cleanup path that frees an owned full-state body. If a resync is mid-drain, the old JSON buffer gets orphaned and the next send overwrites previewSend_.body. Replace the direct flag write with cancelBufferedSend().

🤖 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/HttpServerModule.cpp` around lines 2122 - 2136, Replace the direct
previewSend_.active assignment in the new-client slot logic with a call to
cancelBufferedSend() before incrementing the generation and requesting resync.
Preserve the existing client initialization and full-resync behavior while
ensuring any owned in-flight buffer is cleaned up.

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

83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated maxPerPin clamp loop between the broadcast and list branches.

Both branches run the identical "if counts[i] > maxPerPin, clamp + set *warn" loop. Extracting it into a small local helper (e.g. clampToMax(counts, nPins, maxPerPin, warn)) would remove the duplication and keep future changes (e.g. adjusting the warning condition) to one place.

♻️ Proposed refactor

+static void clampToMax(nrOfLightsType* counts, uint8_t nPins, nrOfLightsType maxPerPin, const char** warn) {
+    if (maxPerPin == 0) return;
+    for (uint8_t i = 0; i < nPins; i++)
+        if (counts[i] > maxPerPin) { counts[i] = maxPerPin; if (warn) *warn = kClampedWarning; }
+}
+
 inline const char* assignCounts(...) {
     ...
-                if (maxPerPin > 0)
-                    for (uint8_t i = 0; i < nPins; i++)
-                        if (counts[i] > maxPerPin) { counts[i] = maxPerPin; if (warn) *warn = kClampedWarning; }
+                clampToMax(counts, nPins, maxPerPin, warn);
                 return nullptr;
     ...
-    if (maxPerPin > 0)
-        for (uint8_t i = 0; i < nPins; i++)
-            if (counts[i] > maxPerPin) {
-                counts[i] = maxPerPin;
-                if (warn) *warn = kClampedWarning;
-            }
+    clampToMax(counts, nPins, maxPerPin, warn);
     return nullptr;
 }

Based on learnings: "Refactor hacks, workarounds, duplicated blocks, and bespoke one-offs when discovered; do not leave them as the destination."

Also applies to: 116-121

🤖 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/PinList.h` around lines 83 - 85, Extract the duplicated
maxPerPin clamping loop from the broadcast and list branches into a shared local
helper, such as clampToMax, that accepts counts, nPins, maxPerPin, and warn.
Replace both inline loops with calls to this helper while preserving the
existing positive-limit check, clamping behavior, and kClampedWarning
assignment.

Source: Learnings

docs/reference/gpio-usage.md (1)

40-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

16-lane strap-pin count looks short by two.

13 non-strap output pins (per this doc's own table) + WR/DC (2 pins) leaves 11 non-strap pins for data; reaching 16 data lanes needs 5 more from the strap set. The classic-ESP32 boot-strap set is 0, 2, 5, 12, 15 (also per this doc, line 35) — 5 pins — but the text only names (0, 12, 15) as the ones a 16-lane config would borrow, omitting 2 and 5. Since this section exists specifically to flag boot-brick risk (GPIO 12), the omitted straps should be named too, or the sentence should say "all 5 straps" rather than enumerate an incomplete subset.

🤖 Prompt for AI Agents

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

In `@docs/reference/gpio-usage.md` around lines 40 - 45, The 16-lane WROVER
description incompletely lists the strap pins required for the configuration.
Update the 16-lane bullet to identify all five strap pins—GPIO 0, 2, 5, 12, and
15—or state that all five straps must be borrowed, while retaining the explicit
GPIO 12 boot-voltage warning and recommendation to use an S3/P4.

src/platform/platform.h (1)

571-583: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make rmtWs2812Wait report timeout status

RmtLedDriver::tick() re-encodes the same symbols_ buffer every frame, but this wait returns void, so a timeout gives the caller no way to know the previous transfer may still be in flight. That breaks the buffer-valid-until-wait-completes contract; return bool like the i80/Parlio waits or add a separate guard before reusing 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/platform/platform.h` around lines 571 - 583, Change rmtWs2812Wait to
return a bool timeout/completion status, matching the existing i80/Parlio wait
APIs, and update its implementations and callers such as RmtLedDriver::tick() to
handle the result before reusing symbols_. Preserve the
buffer-valid-until-wait-completes contract and ensure timed-out transfers are
not treated as safely complete.

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

213-227: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Allow the primary frame buffer to fit in PSRAM.

Line 223 rejects initialization whenever internal DMA memory is low, even when the PSRAM allocation at Line 156 can succeed. Check both regions, as the i80 path does.

Proposed fix

-    if (heap_caps_get_free_size(MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL)
-        < bufferBytes + HEAP_RESERVE) {
-        return false;
-    }
+    const bool fitsInternal =
+        heap_caps_get_free_size(MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL)
+            >= bufferBytes + HEAP_RESERVE;
+    const bool fitsPsram =
+        heap_caps_get_free_size(MALLOC_CAP_DMA | MALLOC_CAP_SPIRAM) >= bufferBytes;
+    if (!fitsInternal && !fitsPsram) return false;

As per coding guidelines, large ESP32 buffers use PSRAM via heap_caps_malloc(..., MALLOC_CAP_SPIRAM).

🤖 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 213 - 227, Update
parlioWs2812Init’s memory-availability check to account for PSRAM, matching the
i80 path: permit initialization when either the internal DMA-capable region or
PSRAM can provide bufferBytes while preserving HEAP_RESERVE requirements for
internal memory. Keep the existing transfer-size and invalid-argument checks
unchanged, and ensure the primary buffer allocation in createState continues
using the appropriate PSRAM capability.

Source: Coding guidelines

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

146-150: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Move freeWire() into DriverBase::release(). DriverBase already owns the scratch allocation lifecycle, so the shared teardown belongs there; then drop the duplicate freeWire() calls from ParallelLedDriver::release() and RmtLedDriver::release().

🤖 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/DriverBase.h` around lines 146 - 150, Move the shared
scratch cleanup call freeWire() into DriverBase::release(), alongside the
existing teardown operations. Remove the duplicate freeWire() calls from
ParallelLedDriver::release() and RmtLedDriver::release(), leaving their
remaining release behavior unchanged; apply these changes in
src/light/drivers/DriverBase.h (lines 146-150 and 162-185) and
src/light/drivers/ParallelLedDriver.h (lines 184-187).

Sources: Path instructions, Learnings

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

41-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use “colors” in the constructor documentation.

Proposed fix

-    /// preset by default (a strip attached to a freshly-flashed board shows correct colours). The
+    /// preset by default (a strip attached to a freshly-flashed board shows correct colors). The

As per coding guidelines, “Use American English spelling, including identifiers, JSON and wire keys, comments, and documentation.”

🤖 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 41 - 44, Update the
constructor documentation above RmtLedDriver() to use the American English
spelling “colors” instead of “colours”; leave the preset behavior and remaining
documentation unchanged.

Source: Coding guidelines

♻️ Duplicate comments (1)

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

295-330: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use 0 as the non-colliding all-lights sentinel.

65535 is also a valid explicit count. On a larger buffer, setWindow(start, 65535) incorrectly drives every remaining light instead of 65535.

Proposed fix

-    static constexpr uint16_t kWindowAll = 65535;
+    static constexpr uint16_t kWindowAll = 0;
     uint16_t start_ = 0;
     uint16_t count_ = kWindowAll;
...
-        outLen = (count_ == 0 || count_ == kWindowAll || count_ > avail)
+        outLen = (count_ == kWindowAll || count_ > avail)

🤖 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/DriverBase.h` around lines 295 - 330, Update the window
sentinel in DriverBase’s addWindowControls/windowSlice implementation so 0, not
kWindowAll (65535), represents all remaining lights. Preserve explicit count
values including 65535, and remove the kWindowAll-based default or comparisons
so setWindow(start, 65535) limits output to exactly 65535 lights.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/architecture.md`:
- Line 149: Fix the reference in the MoonModule::quiesce() documentation so its
link fragment targets an existing visible heading, using that heading’s actual
Markdown slug. Preserve the surrounding explanation and link text.

In `@docs/backlog/backlog-light.md`:
- Line 13: The ghost-pin description should distinguish the WR and DC control
pins. Update the LCD_CAM unused-lane guidance so unused data lanes may overlap
WR or DC only when no strand uses that lane, while explicitly preserving the
requirement that WR and DC remain separate.

In `@docs/backlog/multicore-analysis-bottom-up.md`:
- Around line 3-17: Update the adjacent blockquotes at the document header:
replace the bare blank line between the dated correction blockquote and the
forward-looking research blockquote with a quoted blank line or a short
separating paragraph, preserving both blockquotes as distinct sections and
satisfying markdownlint MD028.

In `@docs/history/plans/Plan-20260713` - Multicore Step 2 render-encode
pipeline.md:
- Around line 68-69: Mark the Step 2 plan as shipped and update its scope guard:
revise the “Offload only pure encode+transmit drivers” language and the
corresponding guard near the later scope section so it no longer incorrectly
excludes Preview and other output-stage work now assigned to core 1, or
explicitly reference the follow-up amendment that changes this split.

In `@src/light/drivers/Drivers.h`:
- Around line 326-329: Update the render-split startup and failure handling
around renderSplitActive_ and startEncodeTask() so a failed worker creation
deallocates or resets the split-only outputBuffer_. Ensure passBufferToDrivers()
falls back to the original inline source when renderSplitActive_ becomes false,
including the corresponding paths around the other referenced split-start logic.
- Around line 360-368: The render-path handoff in the tick method must not call
blocking quiesceEncode(). Replace it with a one-shot, non-blocking
readiness/try-lock check; when core 1 remains busy, skip or drop this output
handoff and record the bounded outcome without waiting. Keep blocking join
behavior confined to lifecycle teardown, including the corresponding logic
around the additional reported location.

In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 289-310: Update drainInFlight() and the reinit/release teardown
paths so a timed-out busWait never clears inFlight_ or frees DMA buffers while
transfers or callbacks may still be active. Either keep retrying until both
buffers complete or use an abort-and-quiesce primitive that guarantees DMA and
callbacks are finished before deinit; preserve the rule that timed-out buffers
cannot be re-encoded or retransmitted until a later successful wait, and add
regression coverage for timeout during reinit/release.

In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 263-267: Update notifyTask to avoid blocking on w->mtx: use the
platform’s established nonblocking handoff or try-lock pattern, setting
w->pending and notifying only when the lock is acquired. Preserve the
null-worker guard and ensure the render-side call never waits on worker or
shutdown synchronization.
- Around line 254-260: Update spawnPinnedTask so std::thread construction
failure returns false without leaving stale state or leaking DesktopWorker.
Start the thread before assigning t.impl, or catch construction exceptions,
delete w, and preserve the existing successful startup behavior.

In `@src/platform/esp32/platform_esp32_worker.cpp`:
- Around line 42-55: Update spawnPinnedTask to assign t.impl to the newly
allocated EspWorker before calling xTaskCreatePinnedToCore, so workerTrampoline
cannot observe a null implementation during startup. If task creation fails,
clear t.impl before deleting the worker and returning false; preserve the
existing success and inline-degrade behavior.

In `@src/platform/platform.h`:
- Around line 121-140: Move the platform-independent TryLock and LockGuard
classes out of platform.h into the appropriate src/core header, preserving their
APIs and RAII behavior. Update HttpServerModule.h and any other consumers to
include the new core header, and remove the duplicated definitions from
platform.h.

In `@test/unit/light/unit_Drivers_rendersplit.cpp`:
- Around line 146-152: The regression comments in
test/unit/light/unit_Drivers_rendersplit.cpp:146-152 and
test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp:318-322 must use
present-tense documentation. Update the rendersplit comment to state the current
quiescence invariant and the use-after-free that occurs if it is violated;
update the double-buffer comment to state the current timeout ownership rule,
removing obsolete “used to” and reviewer-history narration.
- Around line 42-52: The SlowDriver test uses timing sleeps and an unbounded
spin, making it flaky and potentially hanging forever. Replace the fixed sleeps
and polling around SlowDriver::tick and the affected test sections with explicit
condition-variable or latch handshakes: signal when tick starts and completes,
wait with bounded timeouts, and retain the existing assertions and
use-after-free coverage.

---

Outside diff comments:
In `@docs/reference/gpio-usage.md`:
- Around line 40-45: The 16-lane WROVER description incompletely lists the strap
pins required for the configuration. Update the 16-lane bullet to identify all
five strap pins—GPIO 0, 2, 5, 12, and 15—or state that all five straps must be
borrowed, while retaining the explicit GPIO 12 boot-voltage warning and
recommendation to use an S3/P4.

In `@src/core/HttpServerModule.cpp`:
- Around line 2122-2136: Replace the direct previewSend_.active assignment in
the new-client slot logic with a call to cancelBufferedSend() before
incrementing the generation and requesting resync. Preserve the existing client
initialization and full-resync behavior while ensuring any owned in-flight
buffer is cleaned up.

In `@src/light/drivers/DriverBase.h`:
- Around line 146-150: Move the shared scratch cleanup call freeWire() into
DriverBase::release(), alongside the existing teardown operations. Remove the
duplicate freeWire() calls from ParallelLedDriver::release() and
RmtLedDriver::release(), leaving their remaining release behavior unchanged;
apply these changes in src/light/drivers/DriverBase.h (lines 146-150 and
162-185) and src/light/drivers/ParallelLedDriver.h (lines 184-187).

In `@src/light/drivers/PinList.h`:
- Around line 83-85: Extract the duplicated maxPerPin clamping loop from the
broadcast and list branches into a shared local helper, such as clampToMax, that
accepts counts, nPins, maxPerPin, and warn. Replace both inline loops with calls
to this helper while preserving the existing positive-limit check, clamping
behavior, and kClampedWarning assignment.

In `@src/light/drivers/RmtLedDriver.h`:
- Around line 41-44: Update the constructor documentation above RmtLedDriver()
to use the American English spelling “colors” instead of “colours”; leave the
preset behavior and remaining documentation unchanged.

In `@src/platform/esp32/platform_esp32_parlio.cpp`:
- Around line 213-227: Update parlioWs2812Init’s memory-availability check to
account for PSRAM, matching the i80 path: permit initialization when either the
internal DMA-capable region or PSRAM can provide bufferBytes while preserving
HEAP_RESERVE requirements for internal memory. Keep the existing transfer-size
and invalid-argument checks unchanged, and ensure the primary buffer allocation
in createState continues using the appropriate PSRAM capability.

In `@src/platform/platform.h`:
- Around line 571-583: Change rmtWs2812Wait to return a bool timeout/completion
status, matching the existing i80/Parlio wait APIs, and update its
implementations and callers such as RmtLedDriver::tick() to handle the result
before reusing symbols_. Preserve the buffer-valid-until-wait-completes contract
and ensure timed-out transfers are not treated as safely complete.

---

Duplicate comments:
In `@src/light/drivers/DriverBase.h`:
- Around line 295-330: Update the window sentinel in DriverBase’s
addWindowControls/windowSlice implementation so 0, not kWindowAll (65535),
represents all remaining lights. Preserve explicit count values including 65535,
and remove the kWindowAll-based default or comparisons so setWindow(start,
65535) limits output to exactly 65535 lights.

🪄 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: 08c8c42b-dc61-4748-beb7-37cdb3a374e4

📥 Commits

Reviewing files that changed from the base of the PR and between f96cf22 and 0ba46fb.

📒 Files selected for processing (40)

• docs/adr/0013-no-migration-code-robust-persistence-plus-documented-breaks.md
• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog-light.md
• docs/backlog/multicore-analysis-bottom-up.md
• docs/backlog/multicore-analysis-top-down.md
• docs/backlog/nrf-zephyr-target-analysis.md
• docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md
• docs/moonmodules/light/drivers.md
• docs/moonmodules/light/supporting.md
• docs/performance.md
• docs/reference/gpio-usage.md
• esp32/main/CMakeLists.txt
• src/core/BinaryBroadcaster.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/MoonModule.h
• src/light/drivers/DriverBase.h
• src/light/drivers/Drivers.h
• src/light/drivers/I80LedDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/PinList.h
• src/light/drivers/PreviewDriver.h
• src/light/drivers/RmtLedDriver.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32_i80.cpp
• src/platform/esp32/platform_esp32_parlio.cpp
• src/platform/esp32/platform_esp32_worker.cpp
• src/platform/platform.h
• test/CMakeLists.txt
• test/scenarios/light/scenario_MoonLiveEffect_livescript.json
• test/scenarios/light/scenario_perf_full.json
• test/unit/core/unit_platform_worker.cpp
• test/unit/light/unit_Drivers_rendersplit.cpp
• test/unit/light/unit_Layers_container.cpp
• test/unit/light/unit_Layouts_toggle_cycle.cpp
• test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp
• test/unit/light/unit_PreviewDriver.cpp

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep WR and DC distinct in the ghost-pin description.

Line 13 says WR, DC, and unused lanes share one GPIO, but the driver rejects clockPin == dcPin. State that unused data lanes may overlap WR or DC only when no strand uses that lane; WR and DC must remain separate.

🤖 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-light.md` at line 13, The ghost-pin description should
distinguish the WR and DC control pins. Update the LCD_CAM unused-lane guidance
so unused data lanes may overlap WR or DC only when no strand uses that lane,
while explicitly preserving the requirement that WR and DC remain separate.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Blockquote separated by a bare blank line (MD028).

The correction blockquote (lines 3-15) is followed by a plain blank line (16) then a second, unrelated blockquote (17) — markdownlint's MD028 flags this because some renderers merge two blockquotes separated only by a blank, non-quoted line. Insert a > on the blank line (or a short paragraph) to make the boundary unambiguous.

🧰 Tools

🪛 markdownlint-cli2 (0.23.0)

[warning] 16-16: Blank line inside blockquote

(MD028, no-blanks-blockquote)

🤖 Prompt for AI Agents

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

In `@docs/backlog/multicore-analysis-bottom-up.md` around lines 3 - 17, Update the
adjacent blockquotes at the document header: replace the bare blank line between
the dated correction blockquote and the forward-looking research blockquote with
a quoted blank line or a short separating paragraph, preserving both blockquotes
as distinct sections and satisfying markdownlint MD028.

Source: Linters/SAST tools

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== File list ==\n'
git ls-files 'docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md' \
             'src/core/BinaryBroadcaster.h' \
             'src/light/drivers/PreviewDriver.h' \
             'docs/architecture.md' \
             'docs/moonmodules/light/supporting.md' \
             'test/CMakeLists.txt'

printf '\n== Plan file outline ==\n'
wc -l 'docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md'
sed -n '1,220p' 'docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md'

printf '\n== Referenced code/docs snippets ==\n'
for f in \
  'src/core/BinaryBroadcaster.h' \
  'src/light/drivers/PreviewDriver.h' \
  'docs/architecture.md' \
  'docs/moonmodules/light/supporting.md' \
  'test/CMakeLists.txt'
do
  printf '\n--- %s ---\n' "$f"
  wc -l "$f"
done

printf '\n== Search for multicore/Preview/offload markers ==\n'
rg -n --no-heading 'multicore split|offloaded PreviewDriver|PreviewDriver|SendLease|tryAcquireSend|releaseSend|NOT offloading Preview|offload only pure encode|shipped|attempted, abandoned' \
  'src/core/BinaryBroadcaster.h' \
  'src/light/drivers/PreviewDriver.h' \
  'docs/architecture.md' \
  'docs/moonmodules/light/supporting.md' \
  'docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md' \
  'test/CMakeLists.txt'

Repository: MoonModules/projectMM

Length of output: 23550

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the most relevant slices with line numbers.
for f in \
  'src/core/BinaryBroadcaster.h' \
  'src/light/drivers/PreviewDriver.h' \
  'docs/architecture.md' \
  'docs/moonmodules/light/supporting.md' \
  'test/CMakeLists.txt'
do
  printf '\n== %s ==\n' "$f"
  nl -ba "$f" | sed -n '1,260p'
done

Repository: MoonModules/projectMM

Length of output: 236

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== Plan lines around the contested scope ==\n'
sed -n '60,115p' 'docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md' | cat -n

printf '\n== BinaryBroadcaster lines around multicore split ==\n'
sed -n '58,90p' 'src/core/BinaryBroadcaster.h' | cat -n

printf '\n== PreviewDriver lines around core-1 note ==\n'
sed -n '92,110p' 'src/light/drivers/PreviewDriver.h' | cat -n

printf '\n== Architecture lines around preview offload ==\n'
sed -n '144,156p' 'docs/architecture.md' | cat -n

printf '\n== Supporting doc lines around preview offload ==\n'
sed -n '48,62p' 'docs/moonmodules/light/supporting.md' | cat -n

Repository: MoonModules/projectMM

Length of output: 16429

---

Mark this plan shipped and update the scope guard

• docs/history/plans/Plan-20260713 - Multicore Step 2 render-encode pipeline.md (lines 68-69, 108-109) still says NOT offloading Preview/NetworkSend/Hue, but the multicore docs now place Preview and other output-stage work on core 1. Add the required (shipped) marker and either revise this guard or point it at the follow-up amendment that changes the split.

🤖 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/plans/Plan-20260713` - Multicore Step 2 render-encode
pipeline.md around lines 68 - 69, Mark the Step 2 plan as shipped and update its
scope guard: revise the “Offload only pure encode+transmit drivers” language and
the corresponding guard near the later scope section so it no longer incorrectly
excludes Preview and other output-stage work now assigned to core 1, or
explicitly reference the follow-up amendment that changes this split.

Source: Path instructions

[coderabbitai]

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Keep render-side notification nonblocking.

notifyTask() takes a blocking std::mutex; contention with the worker or shutdown can pause the render tick. Use the platform’s nonblocking handoff/TryLock pattern.

As per path instructions, multicore hot paths “never block on peer synchronization; use try-lock semantics instead.”

🧰 Tools

🪛 Clang (14.0.6)

[warning] 263-263: parameter name 't' is too short, expected at least 3 characters

(readability-identifier-length)

---

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

(readability-identifier-length)

---

[warning] 265-265: implicit conversion 'mm::platform::(anonymous namespace)::DesktopWorker *' -> bool

(readability-implicit-bool-conversion)

---

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

(readability-braces-around-statements)

---

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

(readability-identifier-length)

🪛 Cppcheck (2.21.0)

[style] 263-263: The function 'notifyTask' 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 `@src/platform/desktop/platform_desktop.cpp` around lines 263 - 267, Update
notifyTask to avoid blocking on w->mtx: use the platform’s established
nonblocking handoff or try-lock pattern, setting w->pending and notifying only
when the lock is acquired. Preserve the null-worker guard and ensure the
render-side call never waits on worker or shutdown synchronization.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

t.impl is assigned after the task is already created — a startup race.

xTaskCreatePinnedToCore can start workerTrampoline (and thus the caller's fn) running before t.impl = w; on line 53 executes and is visible cross-core. If fn's first action is waitNotify(t, …), it reads t.impl as still nullptr and gets a spurious immediate false — harmless here only because callers are written to retry, but it's an avoidable unsynchronized write/read race on a plain pointer.

🔒 Proposed fix — set `t.impl` before creating the task

     auto* w = new (std::nothrow) EspWorker();
     if (!w) return false;
     w->fn = fn;
     w->user = user;
+    t.impl = w;   // visible before the task can possibly start
     const BaseType_t coreId = (core < 0) ? tskNO_AFFINITY : static_cast<BaseType_t>(core);
     const BaseType_t ok = xTaskCreatePinnedToCore(
         &workerTrampoline, name, static_cast<uint32_t>(stackBytes), w,
         static_cast<UBaseType_t>(priority), &w->handle, coreId);
-    if (ok != pdPASS) { delete w; return false; }   // caller runs inline (degrade)
-    t.impl = w;
+    if (ok != pdPASS) { t.impl = nullptr; delete w; return false; }   // caller runs inline (degrade)
     return true;

📝 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

	bool spawnPinnedTask(WorkerTask& t, const char* name, WorkerFn fn, void* user,
	size_t stackBytes, uint8_t priority, int core) {
	auto* w = new (std::nothrow) EspWorker();
	if (!w) return false;
	w->fn = fn;
	w->user = user;
	const BaseType_t coreId = (core < 0) ? tskNO_AFFINITY : static_cast<BaseType_t>(core);
	const BaseType_t ok = xTaskCreatePinnedToCore(
	&workerTrampoline, name, static_cast<uint32_t>(stackBytes), w,
	static_cast<UBaseType_t>(priority), &w->handle, coreId);
	if (ok != pdPASS) { delete w; return false; } // caller runs inline (degrade)
	t.impl = w;
	return true;
	}
	bool spawnPinnedTask(WorkerTask& t, const char* name, WorkerFn fn, void* user,
	size_t stackBytes, uint8_t priority, int core) {
	auto* w = new (std::nothrow) EspWorker();
	if (!w) return false;
	w->fn = fn;
	w->user = user;
	t.impl = w; // visible before the task can possibly start
	const BaseType_t coreId = (core < 0) ? tskNO_AFFINITY : static_cast<BaseType_t>(core);
	const BaseType_t ok = xTaskCreatePinnedToCore(
	&workerTrampoline, name, static_cast<uint32_t>(stackBytes), w,
	static_cast<UBaseType_t>(priority), &w->handle, coreId);
	if (ok != pdPASS) { t.impl = nullptr; delete w; return false; } // caller runs inline (degrade)
	return true;
	}

🧰 Tools

🪛 Clang (14.0.6)

[warning] 42-42: use a trailing return type for this function

(modernize-use-trailing-return-type)

---

[warning] 42-42: parameter name 't' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 42-42: parameter name 'fn' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 43-43: 3 adjacent parameters of 'spawnPinnedTask' of convertible types are easily swapped by mistake

(bugprone-easily-swappable-parameters)

---

[note] 43-43: the first parameter in the range is 'stackBytes'

(clang)

---

[note] 43-43: the last parameter in the range is 'core'

(clang)

---

[note] 43-43:

(clang)

---

[note] 43-43: 'size_t' and 'uint8_t' may be implicitly converted: 'size_t' (as 'unsigned long') -> 'uint8_t' (as 'unsigned char'), 'uint8_t' (as 'unsigned char') -> 'size_t' (as 'unsigned long')

(clang)

---

[note] 43-43:

(clang)

---

[note] 43-43: 'size_t' and 'int' may be implicitly converted: 'size_t' (as 'unsigned long') -> 'int', 'int' -> 'size_t' (as 'unsigned long')

(clang)

---

[note] 43-43:

(clang)

---

[note] 43-43: 'uint8_t' and 'int' may be implicitly converted: 'uint8_t' (as 'unsigned char') -> 'int', 'int' -> 'uint8_t' (as 'unsigned char')

(clang)

---

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

(readability-identifier-length)

---

[warning] 45-45: implicit conversion 'mm::platform::(anonymous namespace)::EspWorker *' -> bool

(readability-implicit-bool-conversion)

---

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

(readability-braces-around-statements)

---

[warning] 48-48: variable 'coreId' is not initialized

(cppcoreguidelines-init-variables)

---

[warning] 49-49: variable 'ok' is not initialized

(cppcoreguidelines-init-variables)

---

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

(readability-identifier-length)

🪛 Cppcheck (2.21.0)

[style] 42-42: The function 'spawnPinnedTask' 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 `@src/platform/esp32/platform_esp32_worker.cpp` around lines 42 - 55, Update
spawnPinnedTask to assign t.impl to the newly allocated EspWorker before calling
xTaskCreatePinnedToCore, so workerTrampoline cannot observe a null
implementation during startup. If task creation fails, clear t.impl before
deleting the worker and returning false; preserve the existing success and
inline-degrade behavior.

[coderabbitai]

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider src/core/ for TryLock/LockGuard — they're not actually a platform seam.

The comment on these classes explicitly states they have "no platform backing at all" and are "the same code on every platform," yet they live in platform.h, whose charter (per path instructions) is being "the ONLY place... hardware API calls are allowed." They're already consumed by a core file (HttpServerModule.h), reinforcing that this is a domain-neutral, reusable primitive rather than a platform abstraction. Moving it to src/core/ would better match where genuinely portable, reusable mechanisms belong.

🤖 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/platform.h` around lines 121 - 140, Move the
platform-independent TryLock and LockGuard classes out of platform.h into the
appropriate src/core header, preserving their APIs and RAII behavior. Update
HttpServerModule.h and any other consumers to include the new core header, and
remove the duplicated definitions from platform.h.

Source: Path instructions

[coderabbitai]

Actionable comments posted: 10

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/NetworkSendDriver.h (1)

223-250: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not turn a zero-channel correction into raw passthrough.

outCh == 0 is explicitly a valid “Custom preset with no roles emits nothing” state, but this branch sends the uncorrected source. The removed nullable correction state now conflates “not wired” with “configured for zero output”; preserve an explicit wired/configured state and idle for the latter.

🤖 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/NetworkSendDriver.h` around lines 223 - 250, The correction
path in the NetworkSendDriver hot path currently treats correction_.outChannels
== 0 as passthrough, but zero channels is a valid configured correction that
must emit nothing. Preserve an explicit wired/configured state to distinguish it
from an unwired correction, and update the branch around correction_.outChannels
and corrected_ so configured zero-output corrections idle without sending raw
source data while genuinely unwired corrections retain passthrough behavior.

🤖 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 @.github/workflows/test.yml:
- Line 85: Update the actions/checkout step in the workflow to reference a
reviewed, immutable commit SHA instead of the mutable `@v4` tag, while preserving
the existing checkout behavior.

In `@docs/backlog/backlog-light.md`:
- Line 43: Update the backlog wording around the ArtPoll increment and the
“multi-destination unicast” reference to reflect that the prerequisite has
already landed, removing the misleading future “after ... lands” phrasing while
preserving the intended sequencing context.

In `@docs/backlog/led-driver-psram-ring-analysis.md`:
- Line 11: Reconcile the default ring-memory estimate across the documented
claims and formulas, especially the streaming refill ring discussion and the
references on lines 30, 53, and 77. Either include the four-times-size
terminator and descriptor overhead in the formula to substantiate the
approximately 1.6 KB figure, or update the estimate to approximately 1.1 KiB
when calculating only (nbDmaBuffer + 2) × 144 bytes.

In `@moondeck/check/check_sanitizers.py`:
- Around line 58-67: Update the sanitizer probe around the existing subprocess
calls to use tempfile.TemporaryDirectory for an isolated source and executable
location instead of predictable /tmp/_san_probe_* paths. Keep the compile/run
behavior and failure handling unchanged, and ensure temporary files are cleaned
up automatically.
- Around line 73-77: Update the sanitizer probe logic around runtime_works and
the both-mode aggregation so only the explicitly unsupported macOS TSan case is
treated as a successful skip; return failure for all other compiler or runtime
probe failures. Ensure both ASan and TSan probes execute before combining their
results, avoiding short-circuiting through all().

In `@src/light/drivers/NetworkSendDriver.h`:
- Around line 270-304: Move the per-destination packet loop in the
NetworkSendDriver tick path into a bounded sender task or resumable nonblocking
mechanism. Ensure tick() only enqueues frame work or advances a limited send
step, while packet serialization and socket_.sendToAddr calls execute outside
the render thread; preserve destination ordering and prevent unbounded queue
growth or render-thread blocking.
- Around line 166-184: Update the destination validation flow around parseIpList
and assignCounts to use a local destination count throughout validation. Do not
publish nDest_ until both parsing and assignCounts succeed; ensure all error
returns leave the existing destination state unchanged and only then copy the
validated count into nDest_.

In `@src/ui/app.js`:
- Around line 497-514: Update the tab interaction logic in renderChildTabs and
its tab click handler to avoid calling renderCards() for local child switching.
Re-render only the selected panel by clearing it and invoking renderModuleTree
for the chosen child, then update tab-active and aria-selected on the existing
tab buttons in place. Restore focus to the clicked tab, including the "+"
add-tab, so keyboard focus and ARIA tab behavior remain intact.

In `@test/unit/light/unit_NetworkSendDriver_packet.cpp`:
- Line 170: Replace the unbounded strcpy calls initializing d.ips at the
referenced test cases with bounded string-copy operations such as snprintf or
the project’s established safe-copy helper, preserving the existing literal
values and null termination.
- Around line 184-206: Add a test subcase to “NetworkSendDriver: lightsPerIp
follows the ledsPerPin idiom” covering a lightsPerIp list whose length differs
from destinationCount(), including the expected behavior for the mismatch.
Exercise at least one shorter or longer list and assert lightsAt() results for
the destination tubes according to the control’s intended handling.

---

Outside diff comments:
In `@src/light/drivers/NetworkSendDriver.h`:
- Around line 223-250: The correction path in the NetworkSendDriver hot path
currently treats correction_.outChannels == 0 as passthrough, but zero channels
is a valid configured correction that must emit nothing. Preserve an explicit
wired/configured state to distinguish it from an unwired correction, and update
the branch around correction_.outChannels and corrected_ so configured
zero-output corrections idle without sending raw source data while genuinely
unwired corrections retain passthrough behavior.

🪄 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: 373e74e8-97b0-4835-aecd-15bc61fd97e9

📥 Commits

Reviewing files that changed from the base of the PR and between 0ba46fb and 46abaa0.

📒 Files selected for processing (22)

• .github/workflows/test.yml
• docs/architecture.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/led-driver-psram-ring-analysis.md
• docs/moonmodules/light/drivers.md
• moondeck/check/check_sanitizers.py
• src/core/IpList.h
• src/light/drivers/DriverBase.h
• src/light/drivers/Drivers.h
• src/light/drivers/NetworkSendDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/PinList.h
• src/light/drivers/RmtLedDriver.h
• src/platform/esp32/platform_esp32_parlio.cpp
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/scenarios/light/scenario_MoonLiveEffect_livescript.json
• test/unit/core/unit_IpList.cpp
• test/unit/light/unit_Drivers_rendersplit.cpp
• test/unit/light/unit_NetworkSendDriver_packet.cpp

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Pin actions/checkout to a reviewed commit SHA.

The mutable @v4 tag allows upstream changes to alter this security-sensitive sanitizer gate without a repository change.

🧰 Tools

🪛 zizmor (1.26.1)

[error] 85-85: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 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 @.github/workflows/test.yml at line 85, Update the actions/checkout step in
the workflow to reference a reviewed, immutable commit SHA instead of the
mutable `@v4` tag, while preserving the existing checkout behavior.

Source: Linters/SAST tools

[coderabbitai]

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Move multi-destination sends off the render tick.

This loop serializes every receiver's packet burst inside tick(), so render latency grows with destination count and network backpressure. Queue bounded frame work to a sender task or use a nonblocking resumable send mechanism.

As per path instructions, every periodic tick is render-thread hot-path work and must avoid blocking writes or unconditional expensive serialization.

🤖 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/NetworkSendDriver.h` around lines 270 - 304, Move the
per-destination packet loop in the NetworkSendDriver tick path into a bounded
sender task or resumable nonblocking mechanism. Ensure tick() only enqueues
frame work or advances a limited send step, while packet serialization and
socket_.sendToAddr calls execute outside the render thread; preserve destination
ordering and prevent unbounded queue growth or render-thread blocking.

Source: Path instructions

[coderabbitai]

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Tab click triggers a full renderCards() rebuild and drops keyboard focus.

Clicking a tab (or the "+" add-tab) tears down and rebuilds the entire module tree via renderCards() just to switch the active child panel. Since the tabs carry role="tab"/aria-selected and support :focus-visible, this reads as deliberate ARIA-tab semantics — but the full rebuild destroys the clicked <button>, so keyboard focus falls back to the document body instead of staying on the newly-active tab. It's also more DOM churn than needed for a purely local UI-state change.

Consider re-rendering only panel (clear + renderModuleTree(child, panel, depth+1)) and toggling tab-active/aria-selected in place, then restoring focus to the clicked tab.

Also applies to: 576-633

🤖 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/ui/app.js` around lines 497 - 514, Update the tab interaction logic in
renderChildTabs and its tab click handler to avoid calling renderCards() for
local child switching. Re-render only the selected panel by clearing it and
invoking renderModuleTree for the chosen child, then update tab-active and
aria-selected on the existing tab buttons in place. Restore focus to the clicked
tab, including the "+" add-tab, so keyboard focus and ARIA tab behavior remain
intact.

[ewowi]

Processing the open review threads. Verified each against current code; here are the ones I'm not taking, with reasons.

ParallelLedDriver.h — "don't free DMA buffers after a timed-out wait" (🔴 Critical): invalid. The teardown already quiesces before freeing. destroyState() on both backends disables the peripheral first, then frees:

• Parlio: parlio_tx_unit_disable(st->unit) → parlio_del_tx_unit() → heap_caps_free(buf)
• i80: esp_lcd_panel_io_del() → esp_lcd_del_i80_bus() → heap_caps_free(buf)

The "abort-and-quiesce primitive that guarantees DMA and callbacks are finished before deinit" is the IDF deinit call itself, and it is already ordered correctly. drainInFlight()'s comment ("deinit below cancels the wedged transfer") is accurate, not an assumption.

platform_desktop.cpp — "keep render-side notification nonblocking" (make notifyTask use a try-lock): invalid, and it would introduce a bug. That's a condition_variable; the lock is held for exactly one bool store, and the only other holders are the worker's wait (which releases it) and shutdown. A try-lock there would lose wakeups — the worker would miss the notify and stall a frame. This is the textbook condvar pattern; the suggestion trades a non-problem for a real one.

platform_esp32_worker.cpp — "t.impl assigned after task creation is a startup race": invalid. workerTrampoline receives the worker as its void* parameter (xTaskCreatePinnedToCore(..., w, ...)); it never reads t.impl. There is no cross-thread read of that pointer to race with.

IrService.h — "release the previous IR channel before recomputing readiness": stale. Already fixed — reportReady() calls platform::irStop() on the pin_ < 0 path, with a comment explaining exactly this case.

.github/workflows/test.yml — pin actions/checkout to a SHA: not in this PR. All three jobs use @v4; pinning only the new one would be inconsistent, and pinning all of them is a repo-wide supply-chain policy decision that deserves its own change.

test/unit/.../unit_NetworkSendDriver_packet.cpp — unbounded strcpy (CWE-120): accepted as-is. Test-only, string literals of known length into fixed buffers. Switched to snprintf anyway where I touched those lines.

Everything else in the batch is fixed — including two real bugs it caught: NetworkSendDriver::prepare() was publishing a partial destination list on a parse error (so a typo in tube 3 of 5 still sent packets to tubes 1–2), and check_sanitizers.py was turning every probe failure into a green skip, which would have silently un-guarded the sanitizer gate on Linux.