Runtime Ethernet PHY (RMII + W5500) + firmware-variant collapse + catalog provisioning

[ewowi]

What this does

Makes Ethernet runtime-configurable per board and collapses the classic firmware matrix, on top of the catalog-driven install work this branch started with.

• Runtime Ethernet PHY/pins. The Ethernet driver stays compiled per chip (internal-EMAC RMII on classic/P4, W5500 SPI on the S3), but which PHY a board uses and on which pins is now runtime config — a per-chip default in platform_config.h, overridable per board from boards.json (ethType + pin controls → platform::setEthConfig → ethInit() dispatch). No more Olimex-pins-baked-into-the-binary.
• W5500 over SPI on the S3 — new path (the S3 has no EMAC), via the espressif/w5500 managed component, gated to the S3.
• Firmware-variant collapse — the default classic esp32 is now WiFi and Ethernet in one binary (was esp32-eth-wifi), esp32-eth kept as the WiFi-stripped option, the WiFi-only key dropped. esp32-16mb added.
• Catalog-driven module provisioning + 16 MB partition dedup (the branch's original work) — a board entry can create the modules it needs and configure their pins at install time.
• A persistence-overlay bug fix — a saved file omitting a control's key no longer zeroes that control's non-zero default (this was the root cause of the P4 losing Ethernet after the refactor; see below).
• A dedicated ControlType::Pin — GPIO controls render as plain number inputs (one-byte int8_t storage, −1 = unused), not sliders.

tick: unchanged — Ethernet is boot/loop1s-path, not the render tick.

Why

• One pin set per chip was baked into the binary, so a second board with different RMII pins (or any W5500 board) couldn't work; the S3 had no Ethernet at all. Moving pins/PHY to runtime config fixes all three without a firmware-per-board explosion.
• The persistence overlay used parseInt, which returns 0 for an absent key — indistinguishable from a real 0 — and wrote that over the control's default. Harmless when defaults were 0 (the old controls), but the new eth controls have meaningful non-zero defaults (P4 ethType = 2 = IP101), so a partial saved file silently disabled Ethernet. Fixed with a json::hasKey presence guard.

Headline changes

Core / platform — runtime EthPinConfig + setEthConfig/ethStop; ethInit() dispatches RMII (CONFIG_ETH_USE_ESP32_EMAC) vs W5500 (MM_ETH_W5500); NetworkModule eth controls (live-applied for W5500, save-and-restart for RMII); new ControlType::Pin; applyControlValue absent-key guard + json::hasKey/parseBool(1/0).

esp32 build — FIRMWARES collapse; espressif/w5500 + CONFIG_MM_P4_WIFI gating in idf_component.yml (keeps the C6 WiFi stack out of the eth-only P4 build); sdkconfig.defaults.eth-spi; build_esp32.py stale-feature-cache guard (a cached MM_NO_ETH -D isn't cleared by omission).

UI / installer — app.js renders pin as a number input; install-picker/orchestrator comments reframed for the collapse.

Scripts / MoonDeck — catalog module-then-controls injection; profile save/apply scoped to the active network; control-type filtering on captured profiles.

Catalog — per-board eth config (Olimex/Dig-Octa RMII, SE16/LightCrafter W5500, Waveshare/ABC P4 IP101); variant keys updated; 30 board images.

Tests — unit_Control_apply_absent_key (persistence regression), unit_NetworkModule_ethernet (enum/seam contract), scenario_NetworkModule_eth_reconfigure (live ethType cycle robustness), driver-mutation scenarios.

Docs — runtime-PHY model across README/architecture/building/MoonDeck/SystemModule/NetworkModule/FirmwareUpdateModule; dropped variants framed as legacy/OTA-mapped; decisions.md lessons (persistence absent-key, Pin type, devices.json schema-vs-grouping).

Verification

• Hardware, end-to-end on the bench: Olimex Gateway (RMII Ethernet up, DHCP, web UI), esp32-16mb testbench (RMII no-cable → clean WiFi cascade → provisioned STA), ESP32-S3 (W5500 driver compiled+linked; no-eth → WiFi fallback + mic OK), Waveshare P4-NANO (IP101 RMII Ethernet up, DHCP, web UI — after the persistence fix). The P4 regression was bisected with git worktree builds (round-1 ✓, main ✓) to prove it was our code, then root-caused via a stdout printf over the P4's secondary USB-JTAG console.
• All commit gates pass: spec check (35/35), desktop build (zero warnings, -Werror), unit tests, scenarios, platform boundary, board/firmware checks; all 5 ESP32 variants build clean.

Notes for review

• The Opus Reviewer agent ran over the whole branch; two CodeRabbit rounds were processed (fixes + a few accepted/skipped with reasons recorded in the commit bodies).
• esp32p4-eth-wifi remains ships:false (the C6 esp_hosted slave-firmware path isn't CI-reproducible yet) — tracked in the backlog.
• Some scenario JSONs carry observed-block writeback (drift tracking) from running the scenario gate.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

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

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d27752af-ea48-492b-bb11-c090b3efb92a

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

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

[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 `@docs/backlog/installer-3layer-plan.md`:
- Around line 299-305: Update the bench example docs so they match the actual
injects in docs/install/boards.json: in docs/backlog/installer-3layer-plan.md
(lines 299-305) change the S3 testbench description from "RmtLed-only" to show
AudioModule with mic pins (WS=4/SD=5/SCK=6), RmtLed loopback pins (pins=12,
rx=13) and the injected LcdLed; change the P4 description from "RmtLed-only" to
include the injected ParlioLed plus the RmtLed loopback (pins 4/5 and 32/33 as
applicable) and note that ESP32-16MB remains only RmtLed loopback; and in
docs/install/README.md (lines 66-104) make the corresponding edits so the S3 row
lists AudioModule + LcdLed + RmtLed (with mic pin values) and the P4 row lists
ParlioLed + RmtLed, ensuring all pin names and values match boards.json exactly.

In `@docs/moonmodules/core/AudioModule.md`:
- Line 5: The documentation uses future-tense phrases; update the sentences that
mention "as they are added" and any "will be experimenting" wording so the
description stays in present tense (e.g., change "as they are added" to "as they
exist" or remove the temporal implication) or move those roadmap statements into
a clearly marked "Future work" or "Roadmap" section; specifically, edit the
sentence beginning "It is named for what it does, audio acquisition plus
analysis..." and the block containing "will be experimenting" / lines describing
planned sources to be present-tense descriptions of current capabilities or
relocate them to a new future-work subsection.

In `@src/ui/app.js`:
- Around line 195-213: sendAddModule currently only logs failures; change it to
return a boolean (true on 2xx, false on non-2xx or exception) so callers can
react, and update consumePendingBoardParam to stop the injection batch and abort
further sendControl/sendAddModule calls when sendAddModule returns false.
Specifically, modify sendAddModule to return true after a successful res.ok and
return false in both the !res.ok branch and the catch block, and change
consumePendingBoardParam (the batch loop that calls sendAddModule and
sendControl) to check the return value and break/return immediately on a false
result to prevent continuing the batch.

🪄 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: 94da9811-eeb7-450e-b3f8-179a61bb8f31

📥 Commits

Reviewing files that changed from the base of the PR and between cdea1c4 and 3139716.

⛔ Files ignored due to path filters (4)

• esp32/partitions/esp32_16mb.csv is excluded by !**/*.csv
• esp32/partitions/esp32p4_16mb.csv is excluded by !**/*.csv
• esp32/partitions/esp32s3_n16r8.csv is excluded by !**/*.csv
• esp32/partitions/ota_16mb.csv is excluded by !**/*.csv

📒 Files selected for processing (15)

• docs/backlog/README.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/install-orchestrator.js
• docs/moonmodules/core/AudioModule.md
• esp32/sdkconfig.defaults.16mb
• esp32/sdkconfig.defaults.esp32p4-eth
• esp32/sdkconfig.defaults.esp32s3-n16r8
• scripts/moondeck.py
• src/ui/app.js
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/scenarios/light/scenario_Driver_mutation.json

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep this section strictly in present tense.

Line 5 and Lines 62-70 include forward-looking phrasing (for example, “as they are added” / “will be experimenting”). Reword to present tense or move roadmap statements into a clearly marked future-work section.

As per coding guidelines, "**/*.{cpp,h,hpp,js,ts,tsx,md}: Present tense only in code, comments, and documentation: describe the system as it is now, not future plans or changelogs".

Also applies to: 62-70

🤖 Prompt for AI Agents

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

In `@docs/moonmodules/core/AudioModule.md` at line 5, The documentation uses
future-tense phrases; update the sentences that mention "as they are added" and
any "will be experimenting" wording so the description stays in present tense
(e.g., change "as they are added" to "as they exist" or remove the temporal
implication) or move those roadmap statements into a clearly marked "Future
work" or "Roadmap" section; specifically, edit the sentence beginning "It is
named for what it does, audio acquisition plus analysis..." and the block
containing "will be experimenting" / lines describing planned sources to be
present-tense descriptions of current capabilities or relocate them to a new
future-work subsection.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Stop the board-injection batch on a failed module create.

sendAddModule() only logs non-2xx responses, so consumePendingBoardParam() still sends controls after /api/modules rejects. That leaves the device half-injected and turns a single catalog/backend problem into a cascade of 404s. Return success/failure from the helper and abort the batch on the first failed module add.

🛠 Proposed fix

 async function sendAddModule(m) {
@@
-        if (!res.ok) {
-            console.warn(`[board-inject] add module ${m.type} failed (status=${res.status})`);
-        }
+        if (!res.ok) return false;
+        return true;
@@
     for (const m of entry.modules ?? []) {
         if (!m || typeof m !== "object" || !m.type) continue;
-        await sendAddModule(m);
+        if (!(await sendAddModule(m))) return;
     }

Also applies to: 268-275

🤖 Prompt for AI Agents

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

In `@src/ui/app.js` around lines 195 - 213, sendAddModule currently only logs
failures; change it to return a boolean (true on 2xx, false on non-2xx or
exception) so callers can react, and update consumePendingBoardParam to stop the
injection batch and abort further sendControl/sendAddModule calls when
sendAddModule returns false. Specifically, modify sendAddModule to return true
after a successful res.ok and return false in both the !res.ok branch and the
catch block, and change consumePendingBoardParam (the batch loop that calls
sendAddModule and sendControl) to check the return value and break/return
immediately on a false result to prevent continuing the batch.

[coderabbitai]

Actionable comments posted: 6

♻️ Duplicate comments (2)

docs/backlog/installer-3layer-plan.md (1)

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

Testbench wiring details are stale versus the live catalog.

Line 304 documents S3 loopback as pins=12/rx=12, and Lines 311-314 say loopbackTxPin was dropped. Current catalog values in docs/install/boards.json are pins="18", loopbackTxPin=13, loopbackRxPin=12 for S3, and loopbackTxPin is also present on the ESP32-16MB/P4 testbench entries. Please update this block to avoid drift in bench reproduction guidance.

🤖 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/installer-3layer-plan.md` around lines 303 - 315, The
documentation in this backlog file contains stale testbench wiring details that
diverge from the live catalog in docs/install/boards.json. Update the S3 entry
to reflect the current catalog values where pins is 18 (not 12), and acknowledge
that loopbackTxPin is actually present in the current catalog entries for S3
(value 13), ESP32-16MB, and P4, contrary to the statement in lines 311-314 that
says it was dropped. Revise the entire block to accurately describe the current
board configurations to prevent confusion in bench reproduction guidance.

docs/install/install-orchestrator.js (1)

260-268: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate modules[] shape and required module identifiers before POST fan-out.

At Line 260, iterating entry.modules ?? [] assumes an array; malformed catalog data can turn this into a runtime failure instead of a clean false return.
At Line 262 and Line 283, m.id is not validated before /api/modules and /api/control, which breaks the idempotent-create contract and can send control writes to an undefined module target.

Suggested hardening

-    for (const m of entry.modules ?? []) {
+    if (!Array.isArray(entry.modules)) return false;
+    for (const m of entry.modules) {
         if (!m || typeof m !== "object") continue;
-        if (m.parent_id && m.type) {
+        if (typeof m.id !== "string" || !m.id || typeof m.type !== "string" || !m.type) {
+            return false;
+        }
+        if (m.parent_id) {
             try {
                 const res = await fetch(new URL("api/modules", deviceUrl), {
                     method: "POST",
                     headers: { "Content-Type": "application/json" },
                     body: JSON.stringify({ type: m.type, id: m.id, parent_id: m.parent_id }),
@@
-        const controls = m.controls;
+        const controls = m.controls;
         if (!controls || typeof controls !== "object") continue;

Also applies to: 283-287

🤖 Prompt for AI Agents

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

In `@docs/install/install-orchestrator.js` around lines 260 - 268, The code
iterates over entry.modules without validating that it is actually an array, and
it uses m.id in POST requests to both the /api/modules endpoint (around line
262) and /api/control endpoint (around line 283) without first verifying that
m.id exists and is valid. Add validation to ensure entry.modules is an array
before iteration, and add checks to confirm that m.id is a non-empty string or
valid identifier before using it in the JSON body of either POST request. Apply
these validations at all affected POST call sites to prevent runtime failures
and ensure the idempotent-create contract is maintained.

🤖 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.md`:
- Around line 181-183: The heading and content of this backlog section use
future/conditional tense (such as "(future)" and "would let") which violates the
documentation style rule requiring present tense. Remove "(future)" from the
heading on line 181 and reword the entire section to use present tense
throughout, changing conditional phrases like "would let" to "lets" to describe
the proposed system as it would function in the present tense rather than as a
future possibility.

In `@docs/install/boards.json`:
- Line 436: Update the JSON control entry for ledsPerPin so it stays numeric
instead of a string; replace the quoted value with a number in the board control
definition where ledsPerPin is set, keeping the schema/type consistent with the
other controls and with the injector handling.

In `@docs/moonmodules/light/drivers/LcdLedDriver.md`:
- Line 31: Rewrite the affected driver docs to use present tense consistently:
update the LcdLedDriver, NetworkSendDriver, and ParlioLedDriver descriptions so
“Added as a child” becomes “is added as a child,” and revise the RmtLedDriver
wording so “the bench used 5” is expressed in present tense. Apply the wording
changes at the listed anchor and sibling sites only, keeping the rest of each
description unchanged.

In `@docs/moonmodules/light/drivers/RmtLedDriver.md`:
- Around line 28-30: The documentation for the `loopbackTest` field is
incomplete. The description lists `pins`, `loopbackRxPin`, and `loopbackFrame`
as the controls that trigger a re-run of the self-test, but `loopbackTxPin` also
triggers a re-run while test mode is on according to the source code. Add
`loopbackTxPin` to the list of controls mentioned in the `loopbackTest`
description to align the documentation with the actual implementation.

In `@test/unit/light/unit_LcdLedDriver.cpp`:
- Around line 201-214: The test "LcdLedDriver loopbackTxPin tracks the
loopbackTest toggle" claims to verify toggle behavior but only asserts the
default-hidden state. After the initial check that loopbackTxPin is hidden by
default (hidden == true), add code to enable the loopbackTest control by finding
and toggling it (or setting its value), then add a second assertion verifying
that loopbackTxPin.hidden becomes false when loopbackTest is active. This will
complete the test to match its stated contract of tracking toggle behavior.

In `@test/unit/light/unit_ParlioLedDriver.cpp`:
- Around line 248-259: The test for ParlioLedDriver loopbackTxPin toggle
tracking only verifies the default hidden state but does not actually test the
toggle behavior. Extend the test to also toggle the loopbackTest control to true
and then re-check that loopbackTxPin visibility changes accordingly. After the
initial CHECK for the default hidden state (hidden == true), locate the
loopbackTest control in the controls array, toggle its value to true, and then
verify that the loopbackTxPin control's hidden state changes to false to confirm
the toggle tracking works as intended.

---

Duplicate comments:
In `@docs/backlog/installer-3layer-plan.md`:
- Around line 303-315: The documentation in this backlog file contains stale
testbench wiring details that diverge from the live catalog in
docs/install/boards.json. Update the S3 entry to reflect the current catalog
values where pins is 18 (not 12), and acknowledge that loopbackTxPin is actually
present in the current catalog entries for S3 (value 13), ESP32-16MB, and P4,
contrary to the statement in lines 311-314 that says it was dropped. Revise the
entire block to accurately describe the current board configurations to prevent
confusion in bench reproduction guidance.

In `@docs/install/install-orchestrator.js`:
- Around line 260-268: The code iterates over entry.modules without validating
that it is actually an array, and it uses m.id in POST requests to both the
/api/modules endpoint (around line 262) and /api/control endpoint (around line
283) without first verifying that m.id exists and is valid. Add validation to
ensure entry.modules is an array before iteration, and add checks to confirm
that m.id is a non-empty string or valid identifier before using it in the JSON
body of either POST request. Apply these validations at all affected POST call
sites to prevent runtime failures and ensure the idempotent-create contract is
maintained.

🪄 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: af4252ab-685c-49bd-8da1-c3b00e99a4c6

📥 Commits

Reviewing files that changed from the base of the PR and between 3139716 and ee45102.

⛔ Files ignored due to path filters (18)

• docs/assets/boards/ch9102.jpg is excluded by !**/*.jpg
• docs/assets/boards/cp210x-ch34x.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-c3-supermini.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-c3.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-d0-azdelivery.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-d0-pico2.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-d0-wrover.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-d0.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-p4-eth.png is excluded by !**/*.png
• docs/assets/boards/esp32-p4-olimex.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-p4.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-atoms3r.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-lightcrafter16.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-n8r8v.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-seeed_xiao.png is excluded by !**/*.png
• docs/assets/boards/esp32-s3-stephanelec-16p.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-zero-n4r2.jpg is excluded by !**/*.jpg
• docs/assets/boards/others.jpg is excluded by !**/*.jpg

📒 Files selected for processing (19)

• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/install-orchestrator.js
• docs/moonmodules/core/AudioModule.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/NetworkSendDriver.md
• docs/moonmodules/light/drivers/ParlioLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• scripts/moondeck.py
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/main.cpp
• src/ui/app.js
• src/ui/install-picker.js
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_ParlioLedDriver.cpp
• test/unit/light/unit_RmtLedDriver_lifecycle.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use present tense phrasing in this heading/section.

Line 181 (“future”) and Line 183 (“would let”) are future/conditional; reword to present tense to match the docs style rule.

As per coding guidelines, “Present tense only in code, comments, and documentation: describe the system as it is now, not future plans or changelogs.”

🤖 Prompt for AI Agents

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

In `@docs/backlog/backlog.md` around lines 181 - 183, The heading and content of
this backlog section use future/conditional tense (such as "(future)" and "would
let") which violates the documentation style rule requiring present tense.
Remove "(future)" from the heading on line 181 and reword the entire section to
use present tense throughout, changing conditional phrases like "would let" to
"lets" to describe the proposed system as it would function in the present tense
rather than as a future possibility.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep ledsPerPin numeric in JSON controls.

Line 436 uses a string ("64") while this control is numeric elsewhere. Prefer 64 (number) to keep injector/schema handling type-stable across clients.

🤖 Prompt for AI Agents

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

In `@docs/install/boards.json` at line 436, Update the JSON control entry for
ledsPerPin so it stays numeric instead of a string; replace the quoted value
with a number in the board control definition where ledsPerPin is set, keeping
the schema/type consistent with the other controls and with the injector
handling.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Normalize past-tense phrasing to present tense across updated driver docs. The same style-rule issue appears in multiple changed sentences; please rewrite each in present tense for consistency.

• docs/moonmodules/light/drivers/LcdLedDriver.md#L31-L31: change “Added as a child …” to present tense (“The driver is added …”).
• docs/moonmodules/light/drivers/NetworkSendDriver.md#L37-L37: change “Added as a child …” to present tense.
• docs/moonmodules/light/drivers/ParlioLedDriver.md#L49-L49: change “Added as a child …” to present tense.
• docs/moonmodules/light/drivers/RmtLedDriver.md#L29-L29: rewrite “the bench used 5” in present tense wording.

As per coding guidelines, "Present tense only in code, comments, and documentation: describe the system as it is now, not future plans or changelogs".

📍 Affects 4 files

• docs/moonmodules/light/drivers/LcdLedDriver.md#L31-L31 (this comment)
• docs/moonmodules/light/drivers/NetworkSendDriver.md#L37-L37
• docs/moonmodules/light/drivers/ParlioLedDriver.md#L49-L49
• docs/moonmodules/light/drivers/RmtLedDriver.md#L29-L29

🤖 Prompt for AI Agents

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

In `@docs/moonmodules/light/drivers/LcdLedDriver.md` at line 31, Rewrite the
affected driver docs to use present tense consistently: update the LcdLedDriver,
NetworkSendDriver, and ParlioLedDriver descriptions so “Added as a child”
becomes “is added as a child,” and revise the RmtLedDriver wording so “the bench
used 5” is expressed in present tense. Apply the wording changes at the listed
anchor and sibling sites only, keeping the rest of each description unchanged.

Source: Coding guidelines

[coderabbitai]

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (4)

scripts/run/preview_installer.py (1)

229-236: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use uv run for manifest generation subprocess calls.

Line 230 invokes Python via sys.executable instead of uv run, which diverges from the script-invocation policy and can bypass the managed environment.

Suggested fix

-        result = subprocess.run(
-            [sys.executable, str(GENERATE_MANIFEST),
+        result = subprocess.run(
+            ["uv", "run", str(GENERATE_MANIFEST),
              "--firmware", firmware,
              "--version", LOCAL_VERSION,
              "--release-url", ".",
              "--flasher-args", str(flasher_args),
              "--out", str(manifest_path)],

As per coding guidelines, “Use uv run for every Python invocation in scripts and CMake commands; never type python or python3 directly.”

🤖 Prompt for AI Agents

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

In `@scripts/run/preview_installer.py` around lines 229 - 236, The subprocess.run
call for manifest generation is using sys.executable to invoke Python directly
instead of using uv run as required by coding guidelines. In the command list
passed to subprocess.run, replace the sys.executable approach with uv run by
changing the command array to start with "uv" and "run" before the
GENERATE_MANIFEST script path, ensuring the script is executed through the
managed environment rather than the raw Python executable.

Source: Coding guidelines

src/ui/install-picker.js (1)

717-724: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate TX-power bounds before returning the value.

This currently accepts any number. Invalid values can flow into pre-association provisioning and silently break the intended brown-out mitigation path. Enforce integer 0..21 here.

Suggested patch

     getSelectedBoardTxPower() {
         if (!_lastState || !_lastState.selectedBoard || !_lastState.boards) return null;
         const entry = _lastState.boards.find(b => b.name === _lastState.selectedBoard);
         // New catalog shape: each board's modules list carries its own controls;
         // the WiFi TX-power cap lives on the Network module's controls block.
         const net = entry && (entry.modules || []).find(m => m && m.id === "Network");
         const v = net && net.controls && net.controls.txPowerSetting;
-        return (typeof v === "number") ? v : null;
+        if (!Number.isInteger(v) || v < 0 || v > 21) return null;
+        return 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/install-picker.js` around lines 717 - 724, The
getSelectedBoardTxPower() function currently validates only that the TX-power
value is a number, but does not enforce the required bounds. Update the return
statement to validate that the value is an integer within the valid range of 0
to 21 inclusive. If the value fails validation (is not an integer, is negative,
or exceeds 21), return null instead of the invalid value to prevent it from
breaking the brown-out mitigation path.

docs/history/decisions.md (1)

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

Fix the repeated list number.

Line 709 restarts the ordered list at 3. immediately after the previous 3. item. Renumber it so the sequence stays unambiguous.

🤖 Prompt for AI Agents

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

In `@docs/history/decisions.md` at line 709, The ordered list in decisions.md has
a numbering error where item 3 appears twice in succession. Change the second
occurrence of the list number from "3." to "4." to maintain proper sequential
numbering throughout the list.

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

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

Keep the bench wiring note in present tense.

The new sentence uses "was", which violates the repo rule that comments stay in present tense. Please rephrase it as current state (for example, "is pins ..."). As per coding guidelines, **/*.{cpp,h,hpp,js,ts,tsx,md}: Present tense only in code, 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/LcdLedDriver.h` around lines 32 - 39, The comment about the
ESP32-S3 N16R8 Dev bench wiring in the LcdLedDriver.h file violates the
repository rule requiring present tense in all code comments and documentation.
Change the phrase "The ESP32-S3 N16R8 Dev bench wiring was pins" to use present
tense by replacing "was" with "is" to maintain consistency with the coding
guidelines that mandate present tense only in comments.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/install-alt/index.html`:
- Line 506: The input element with id "board-search" lacks an accessible name
for screen readers. Add either a label element with a for attribute matching the
input's id, or add an aria-label attribute directly to the input element to
provide a descriptive accessible name such as "Search boards" so screen readers
can announce the field's purpose to users.
- Around line 1600-1604: The card function creates a non-focusable div element
that only responds to mouse clicks via onclick, making it inaccessible to
keyboard-only users. To fix this, add tabindex="0" to the div element to make it
keyboard-focusable, and add a keyboard event handler (such as onkeydown) that
triggers the pickBoard function when the user presses Enter or Space keys. This
should be done in addition to the existing onclick handler to ensure both mouse
and keyboard interactions work for selecting board cards.

---

Outside diff comments:
In `@docs/history/decisions.md`:
- Line 709: The ordered list in decisions.md has a numbering error where item 3
appears twice in succession. Change the second occurrence of the list number
from "3." to "4." to maintain proper sequential numbering throughout the list.

In `@scripts/run/preview_installer.py`:
- Around line 229-236: The subprocess.run call for manifest generation is using
sys.executable to invoke Python directly instead of using uv run as required by
coding guidelines. In the command list passed to subprocess.run, replace the
sys.executable approach with uv run by changing the command array to start with
"uv" and "run" before the GENERATE_MANIFEST script path, ensuring the script is
executed through the managed environment rather than the raw Python executable.

In `@src/light/drivers/LcdLedDriver.h`:
- Around line 32-39: The comment about the ESP32-S3 N16R8 Dev bench wiring in
the LcdLedDriver.h file violates the repository rule requiring present tense in
all code comments and documentation. Change the phrase "The ESP32-S3 N16R8 Dev
bench wiring was pins" to use present tense by replacing "was" with "is" to
maintain consistency with the coding guidelines that mandate present tense only
in comments.

In `@src/ui/install-picker.js`:
- Around line 717-724: The getSelectedBoardTxPower() function currently
validates only that the TX-power value is a number, but does not enforce the
required bounds. Update the return statement to validate that the value is an
integer within the valid range of 0 to 21 inclusive. If the value fails
validation (is not an integer, is negative, or exceeds 21), return null instead
of the invalid value to prevent it from breaking the brown-out mitigation path.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a6e12ade-d223-49a2-a37b-51667e152e69

📥 Commits

Reviewing files that changed from the base of the PR and between ee45102 and c892466.

⛔ Files ignored due to path filters (17)

• docs/assets/boards/esp32-s3-n16r8-dev.jpg is excluded by !**/*.jpg
• docs/assets/boards/esp32-s3-n8r8-dev.jpg is excluded by !**/*.jpg
• docs/assets/boards/generic-esp32-dev.jpg is excluded by !**/*.jpg
• docs/assets/boards/lightcrafter-16.jpg is excluded by !**/*.jpg
• docs/assets/boards/lolin-d32.jpg is excluded by !**/*.jpg
• docs/assets/boards/olimex-esp32-gateway-rev-g.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-2-go.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-next-2.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-octa-32-8l.png is excluded by !**/*.png
• docs/assets/boards/quinled-dig-quad-v3.png is excluded by !**/*.png
• docs/assets/boards/quinled-dig-uno-v3.png is excluded by !**/*.png
• docs/assets/boards/serg-minishield.jpg is excluded by !**/*.jpg
• docs/assets/boards/waveshare-esp32-p4-nano.jpg is excluded by !**/*.jpg
• docs/assets/screenshots/installer-board-picker-collapsed.png is excluded by !**/*.png
• docs/assets/screenshots/installer-board-picker-expanded.png is excluded by !**/*.png
• docs/install-alt/favicon.png is excluded by !**/*.png
• scripts/build/improv_provision.py is excluded by !**/build/**

📒 Files selected for processing (37)

• .github/workflows/release.yml
• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/history/decisions.md
• docs/install-alt/devices.js
• docs/install-alt/index.html
• docs/install/README.md
• docs/install/boards.json
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/NetworkSendDriver.md
• docs/moonmodules/light/drivers/ParlioLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/performance.md
• scripts/MoonDeck.md
• scripts/moondeck.py
• scripts/moondeck_ui/index.html
• scripts/run/preview_installer.py
• src/core/ImprovProvisioningModule.h
• src/core/NetworkModule.h
• src/light/drivers/LcdLedDriver.h
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/platform.h
• src/ui/install-picker.js
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/scenarios/light/scenario_Driver_mutation.json
• test/unit/core/unit_BoardModule.cpp
• test/unit/light/unit_LcdLedDriver.cpp
• test/unit/light/unit_ParlioLedDriver.cpp

[coderabbitai]

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (1)

docs/history/decisions.md (1)

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

Keep this lesson in past tense.

will introduce reads like roadmap text, which conflicts with the repo’s present-tense doc rule. Rephrase this section as a past event/lesson so the history file stays consistent.

As per coding guidelines, docs stay in present tense.

🤖 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/decisions.md` around lines 772 - 780, The phrase "will
introduce" in the opening sentence reads as future-tense roadmap text, which
conflicts with the repository's present-tense documentation style. Rephrase the
section starting with "Round 1 recorded that round 3 'will introduce a WiFi
abstraction seam...'" to describe this as a past event that already occurred,
converting future-tense language like "will introduce" to past tense to maintain
consistency with the document's historical narrative and the repo's style
guidelines for documentation.

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 @.github/workflows/release.yml:
- Line 80: The actions/checkout action at line 80 in the `firmwares` job uses a
mutable version tag (`@v4`) and retains default credential persistence. Replace
the mutable tag with a pinned commit SHA and add the parameter
persist-credentials: false to the checkout action configuration to reduce
supply-chain and token-exposure risk.

In `@docs/install/boards.json`:
- Around line 723-727: The P4 board entries in docs/install/boards.json
incorrectly advertise WiFi support in the "supported" field when only an
Ethernet-only firmware variant (esp32p4-eth) is available. Remove "WiFi" from
the "supported" array at lines 723-727 (anchor location) and at lines 758-762
(sibling location) to accurately reflect that only Ethernet capability is
currently installable for P4 boards.

In `@scripts/check/check_boards.py`:
- Around line 123-124: The code at the isinstance(mods, list) check silently
skips invalid entries using continue, which allows schema violations to pass
undetected. Replace the continue statement with proper error handling by raising
an appropriate exception or logging an error to ensure that non-list modules
fields are caught and reported as schema validation failures rather than being
silently ignored.
- Around line 89-93: The validation at the location where firmwares membership
is checked only validates the default_firmware when firmwares is a list, but
skips validation entirely if firmwares is not a list type. This allows malformed
entries where firmwares is not an array to pass through unchecked. Add a
separate validation check that ensures the firmwares field obtained from
e.get("firmwares") is actually a list type, and if it is not a list, append an
error message to the errors list indicating that firmwares must be an array.
This validation should occur before or alongside the existing default_firmware
membership check.

---

Outside diff comments:
In `@docs/history/decisions.md`:
- Around line 772-780: The phrase "will introduce" in the opening sentence reads
as future-tense roadmap text, which conflicts with the repository's
present-tense documentation style. Rephrase the section starting with "Round 1
recorded that round 3 'will introduce a WiFi abstraction seam...'" to describe
this as a past event that already occurred, converting future-tense language
like "will introduce" to past tense to maintain consistency with the document's
historical narrative and the repo's style guidelines for documentation.

🪄 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: d1a9fe0f-706f-4763-96ae-84235fda40ca

📥 Commits

Reviewing files that changed from the base of the PR and between c892466 and d08612e.

⛔ Files ignored due to path filters (12)

• docs/assets/boards/abc-wled-esp32-p4-shield.jpg is excluded by !**/*.jpg
• docs/assets/boards/mhc-v57-pro.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-2-go.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-next-2.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-octa-32-8l.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-quad-v3.jpg is excluded by !**/*.jpg
• docs/assets/boards/quinled-dig-uno-v3.jpg is excluded by !**/*.jpg
• docs/assets/boards/serg-minishield.jpg is excluded by !**/*.jpg
• docs/assets/boards/serg-unishield-v5.jpg is excluded by !**/*.jpg
• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/generate_firmwares.py is excluded by !**/build/**
• scripts/build/generate_manifest.py is excluded by !**/build/**

📒 Files selected for processing (19)

• .github/workflows/release.yml
• CLAUDE.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/building.md
• docs/history/decisions.md
• docs/install-alt/index.html
• docs/install/README.md
• docs/install/boards.json
• docs/install/firmwares.json
• scripts/check/check_boards.py
• scripts/check/check_firmwares.py
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/run/preview_installer.py
• src/light/drivers/LcdLedDriver.h
• src/ui/install-picker.js
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify unpinned and credential-persisting checkout usage in this workflow.
rg -n 'uses:\s*actions/checkout@' .github/workflows/release.yml -A3
rg -n 'persist-credentials:\s*false' .github/workflows/release.yml

Repository: MoonModules/projectMM

Length of output: 1291

---

Harden checkout in the new firmwares job.

The checkout at line 80 uses a mutable tag (@v4) and retains default credential persistence. Pin to a commit SHA and disable persisted credentials to reduce supply-chain and token-exposure risk.

🧰 Tools

🪛 zizmor (1.25.2)

[warning] 80-80: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)

---

[error] 80-80: 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/release.yml at line 80, The actions/checkout action at
line 80 in the `firmwares` job uses a mutable version tag (`@v4`) and retains
default credential persistence. Replace the mutable tag with a pinned commit SHA
and add the parameter persist-credentials: false to the checkout action
configuration to reduce supply-chain and token-exposure risk.

Source: Linters/SAST tools

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (1)

scripts/check/check_boards.py (1)

90-93: ⚠️ Potential issue | 🟠 Major

Strengthen firmware schema validation beyond container type.

At Line 90, this only enforces that firmwares is a list. A malformed entry like firmwares: [null] plus default_firmware: null can still pass membership checks, while downstream catalog consumers expect firmware identifiers as strings.

Proposed fix

         fws = e.get("firmwares")
-        if not isinstance(fws, list):
-            errors.append(f"{where}: firmwares must be a list, got {type(fws).__name__}")
-        elif e.get("default_firmware") not in fws:
+        default_fw = e.get("default_firmware")
+        if (
+            not isinstance(fws, list)
+            or not fws
+            or not all(isinstance(fw, str) and fw for fw in fws)
+        ):
+            errors.append(f"{where}: firmwares must be a non-empty list of non-empty strings")
+        elif not isinstance(default_fw, str) or not default_fw:
+            errors.append(f"{where}: default_firmware must be a non-empty string")
+        elif default_fw not in fws:
             errors.append(f"{where}: default_firmware '{e.get('default_firmware')}' "
                           f"not in firmwares {fws}")

🤖 Prompt for AI Agents

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

In `@scripts/check/check_boards.py` around lines 90 - 93, The firmware schema
validation at the isinstance check for fws being a list is incomplete. Beyond
verifying that fws is a list, you must also validate that each firmware
identifier within that list is a string, since downstream catalog consumers
expect firmware identifiers to be strings rather than null or other types. Add
validation logic to iterate through the list and check that all items are
strings, appending an appropriate error message to the errors list if any
non-string entries are found (similar to how the current type checking is done
with type(fws).__name__).

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/install/README.md`:
- Around line 42-44: The description of the board picker currently presents both
`image` and `url` as required catalog fields, but the schema documentation below
indicates that `url` is optional. Reword the sentence in the board picker
description to clarify that while the `image` field is used, the `url` field is
optional. This ensures the documentation doesn't create a contract mismatch
between the prose description and the schema definition below it.

In `@scripts/moondeck_ui/app.js`:
- Around line 976-981: The profile creation logic at lines 976-981 uses a
placeholder with empty modules array that will be persisted when saveState() is
called later, clobbering the real profile modules already saved to the server
via /api/save-profile. Instead of pushing a local placeholder with {name,
modules: []}, refresh the authoritative profile state from the server by making
a request to fetch the updated device state after the profile is created on the
server, then update device.profiles with the real data returned from the server
before calling renderDevices().

In `@scripts/moondeck.py`:
- Line 316: The nested helper functions in this file are missing return type
annotations, which triggers the Ruff ANN202 lint rule. Add return type
annotations of `-> None` to the private helper functions `_collect` and its
sibling nested helper that was flagged. Simply append `-> None` to the function
definition line after the closing parenthesis of the parameters and before the
colon to indicate these functions don't return a value.
- Around line 321-325: The control processing loop in moondeck.py needs to
filter out non-replayable control types before adding them to the controls
dictionary. Modify the loop that iterates through m.get("controls", []) to check
the control type (obtained via c.get("type")) and skip adding controls that have
type values of Password, ReadOnly, ReadOnlyInt, or Progress to the controls
dictionary. This prevents obfuscated password values and read-only/progress
controls from being saved and replayed, which would cause password encoding
issues or profile restore errors.
- Around line 1021-1025: The profile lookup in the nested loop (iterating
through networks, devices, and profiles) currently matches by name alone across
all devices, which causes incorrect pin maps to be restored when multiple
devices share the same profile name like "default". Scope the profile lookup to
the source device by first identifying which device the profile should come from
(either by capturing the source device/IP when the profile is initially selected
or by passing it as a parameter), then modify the profile matching logic to
verify that the found profile belongs to the correct source device before
retrieving its modules. This ensures that only profiles from the intended device
are used during restore operations.

---

Duplicate comments:
In `@scripts/check/check_boards.py`:
- Around line 90-93: The firmware schema validation at the isinstance check for
fws being a list is incomplete. Beyond verifying that fws is a list, you must
also validate that each firmware identifier within that list is a string, since
downstream catalog consumers expect firmware identifiers to be strings rather
than null or other types. Add validation logic to iterate through the list and
check that all items are strings, appending an appropriate error message to the
errors list if any non-string entries are found (similar to how the current type
checking is done with type(fws).__name__).

🪄 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: 9947c461-3252-4f2e-9297-ab5612694da6

📥 Commits

Reviewing files that changed from the base of the PR and between d08612e and c348350.

⛔ Files ignored due to path filters (3)

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

📒 Files selected for processing (13)

• .github/workflows/release.yml
• docs/architecture.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/index.html
• docs/install/install-orchestrator.js
• scripts/MoonDeck.md
• scripts/check/check_boards.py
• scripts/moondeck.py
• scripts/moondeck_ui/app.js
• scripts/run/preview_installer.py

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

README.md (1)

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

Fix the P4 lane-count claim.

Line 29 says the P4 Parlio path can drive up to 20 strands, but the architecture doc caps Parlio at 1–8 lanes. Please reconcile the numbers so the headline matches the actual hardware limit.

🤖 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 `@README.md` at line 29, The README.md file currently claims that the P4's
Parlio engine can drive up to 20 simultaneous strands, but this contradicts the
actual hardware limit documented in the architecture documentation which caps
Parlio at 1–8 lanes. Update the P4 Parlio claim in the parallel WS2812 output
section to reflect the correct lane count of 1–8 instead of 20 to ensure the
documentation accurately represents the hardware capabilities.

docs/history/decisions.md (1)

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

Reconcile the devices.json decision.

Line 820 says there is no devices.json split, but architecture.md and backlog.md still describe that split as deferred rather than rejected. Please make the docs say one thing here. As per coding guidelines, "Single source of truth. CLAUDE.md (rules), architecture.md (design), docs/modules/*.md (specs). Nothing else. If it's duplicated, delete one copy."

🤖 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/decisions.md` around lines 820 - 823, The decision documented in
decisions.md clearly states there is no `devices.json` split (one catalog, one
entry type), but architecture.md and backlog.md still describe this
`devices.json` split as deferred rather than rejected. Search for references to
the `devices.json` split being deferred in both architecture.md and backlog.md,
then update those references to either explicitly state it is rejected (to align
with the decision in decisions.md), or alternatively update the statement in
decisions.md to say it is deferred instead. Ensure only one consistent statement
of this decision exists across all architecture documentation files to maintain
single source of truth.

Source: Coding guidelines

docs/install/index.html (1)

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

Update the Ethernet note for runtime pin configuration.

This still points boards with different RMII pins at a local rebuild, but this PR moves Ethernet pin/PHY choice into the catalog-driven runtime config path.

Proposed wording

-        but different pins (e.g. WT32-ETH01: reset on GPIO 16) need a local rebuild.
+        but different pins (e.g. WT32-ETH01: reset on GPIO 16) need a matching catalog entry
+        so the installer can apply the runtime Ethernet pin 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/install/index.html` around lines 560 - 565, Update the Ethernet
documentation note at the end of the paragraph to reflect that boards with
different RMII pins (like WT32-ETH01) can now be configured at runtime through
the catalog-driven configuration system instead of requiring a local rebuild.
Replace the statement about needing a local rebuild with information about how
to configure Ethernet pins and PHY selection at runtime.

src/ui/install-picker.js (1)

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

Align the TX-power JSDoc with the modules schema.

The implementation no longer reads controls.Network; leaving that path in the public helper docs can send the next catalog edit back to the removed schema. As per coding guidelines, comments document intent and context and outdated/factually wrong comments should not be carried forward.

Proposed fix

-     * The picked board's boards.json TX-power cap
-     * (controls.Network.txPowerSetting), or null when the board has none /
+     * The picked board's boards.json TX-power cap
+     * (`modules[].id === "Network"` → `controls.txPowerSetting`), or null when the board has none /

🤖 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/install-picker.js` around lines 710 - 713, The JSDoc comment for the
TX-power property in the install-picker.js file (around line 710-713) contains
an outdated reference to controls.Network.txPowerSetting, which is no longer
part of the implementation schema. Update the JSDoc documentation to remove this
incorrect schema path reference and ensure the comment accurately reflects the
current implementation without mentioning the removed controls.Network path, so
that future catalog edits are not misled by outdated documentation.

Source: Coding guidelines

scripts/run/preview_installer.py (1)

91-91: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Redundant import of json inside function.

json is already imported at module level (line 37). The local import at line 91 is unnecessary.

Suggested fix

 def _stage_referenced_board_images(dst_dir: Path):
     """Stage only the board images a boards.json entry references (mirrors the
     deploy's filtered copy), under <dst_dir>/assets/boards/."""
     boards_json = INSTALL_DIR / "boards.json"
     if not boards_json.exists():
         return
-    import json
     try:
         boards = json.loads(boards_json.read_text())

🤖 Prompt for AI Agents

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

In `@scripts/run/preview_installer.py` at line 91, The `json` module is imported
twice: once at module level and again locally inside a function at line 91.
Remove the redundant local import statement since the module is already
available at the module scope and can be used directly without reimporting.

.github/workflows/release.yml (1)

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

Use uv run for Python invocations per coding guidelines.

The release job invokes python scripts/build/generate_manifest.py directly without going through uv run. The coding guidelines require all Python invocations to use uv run. Add astral-sh/setup-uv@v3 and change to uv run python scripts/build/generate_manifest.py.

Same issue at line 397 in the deploy-pages job.

Suggested fix for release job (lines 268-289)

Add setup-uv step and update the command:

     steps:
+      - uses: astral-sh/setup-uv@v3
       # NB: this checkout KEEPS persisted credentials ...
       - uses: actions/checkout@v4
       ...
       - name: Generate ESP Web Tools manifests (release-asset URLs)
         ...
         run: |
           ...
           for F in $(jq -r '.firmwares[] | select(.ships) | .name' docs/install/firmwares.json); do
-            python scripts/build/generate_manifest.py \
+            uv run python scripts/build/generate_manifest.py \

🤖 Prompt for AI Agents

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

In @.github/workflows/release.yml at line 283, Update the release and
deploy-pages jobs in the GitHub Actions workflow to comply with coding
guidelines that require all Python invocations to use uv run. In the release job
(around line 283), add a setup step using astral-sh/setup-uv@v3 and change the
python scripts/build/generate_manifest.py invocation to uv run python
scripts/build/generate_manifest.py. Apply the same fix to the deploy-pages job
at line 397 by adding the setup-uv action and updating the command to use uv
run.

Source: Coding guidelines

♻️ Duplicate comments (2)

scripts/moondeck.py (1)

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

Add missing -> None annotations for nested private helpers (ANN202).

This lint finding is still present for _collect and _store.

#!/bin/bash
uv run ruff check scripts/moondeck.py --select ANN202

Also applies to: 1015-1015

🤖 Prompt for AI Agents

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

In `@scripts/moondeck.py` at line 323, The nested private helper functions
`_collect` and `_store` are missing return type annotations, triggering the
ANN202 lint rule. Add `-> None` return type annotations to the function
signatures: add `-> None` after the closing parenthesis of the `_collect`
function at line 323 and after the closing parenthesis of the `_store` function
at line 1015 to explicitly declare that both functions return None.

Source: Linters/SAST tools

scripts/check/check_boards.py (1)

93-99: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce non-empty firmware IDs in firmwares entries.

Line 93 only type-checks entries, so empty strings still pass. That can let schema-invalid firmware keys through and break downstream firmware selection assumptions.

Proposed fix

-            non_str = [f for f in fws if not isinstance(f, str)]
+            non_str = [f for f in fws if not isinstance(f, str) or not f]
             if non_str:
-                errors.append(f"{where}: firmwares entries must be strings, got "
+                errors.append(f"{where}: firmwares entries must be non-empty strings, got "
                               f"{[type(f).__name__ for f in non_str]}")

🤖 Prompt for AI Agents

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

In `@scripts/check/check_boards.py` around lines 93 - 99, The firmware validation
in the firmwares entries check only validates that entries are strings but does
not check if those strings are non-empty. Add a validation step after the
existing type check (the non_str list comprehension) to identify any empty
string entries in the firmwares list, and if found, append an appropriate error
message to the errors list indicating that firmware entries must be non-empty
strings. This ensures that invalid empty firmware IDs cannot slip through the
validation.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/architecture.md`:
- Around line 232-233: The documentation uses inconsistent casing for the WiFi
flag: line 232 contains `hasWifi` while an earlier section in the same document
uses `hasWiFi`. Choose one spelling convention (either `hasWifi` or `hasWiFi`)
and update all occurrences throughout the document to use that spelling
consistently.

In `@docs/backlog/backlog.md`:
- Around line 190-191: The documentation currently references a "short hardcoded
board list" as a fallback mechanism, but the implementation has moved to a
catalog-driven approach using boards.json for both the board picker and
fallback. Remove the "hardcoded list" wording and update the text to clarify
that MoonDeck now uses the boards.json catalog to both let users pick a board
and provide fallback options when the firmware does not uniquely identify the
hardware.

In `@docs/install/index.html`:
- Around line 1743-1747: In the MutationObserver callback, the line assigning to
`selected` uses a fallback that prevents clearing stale board selection when the
picker is reset. When `installPicker.getSelectedBoard()` returns an empty string
(indicating the picker was cleared for a detected family with multiple matches),
the `|| selected` causes the old board value to persist in the summary display,
misleading the user about the current selection state. Update the assignment to
`selected` to directly use the value from `installPicker.getSelectedBoard()`
without the fallback operator, so that an empty return value properly clears the
stale selection and reflects the actual generic mode state of the picker.

In `@scripts/moondeck.py`:
- Around line 1015-1023: The _store function matches devices by IP address
across all networks, which causes collisions when multiple networks have
overlapping private subnets (like 192.168.1.x). Modify the _store function to
accept or determine a specific network context, then only iterate through and
modify devices within that single network rather than searching across all
networks. This same scoping issue also needs to be applied to the similar
profile apply handler function to prevent replaying profiles from the wrong
network.

In `@src/core/JsonUtil.h`:
- Around line 43-52: The hasKey function uses a naive std::strstr search that
cannot distinguish between actual JSON object members and text that happens to
appear inside string values, causing false positives for absent keys when
escaped patterns exist in values. Implement a JSON-aware search in hasKey that
iterates through the JSON string while tracking whether the current position is
inside a string value (handling escaped quotes), and only matches the key
pattern when positioned at the object member level outside of any string values.
This ensures the function correctly identifies genuinely-present keys without
matching similar patterns embedded in value strings.

In `@src/core/NetworkModule.h`:
- Line 477: The `appliedEthSig_` member variable is declared as `long` but
experiences signed overflow during hash calculations in the `ethSig()` function,
causing undefined behavior. Change the type of `appliedEthSig_` from `long` to
`uint32_t` to guarantee deterministic wrapping behavior instead of undefined
signed overflow. Update the initialization value from -1 to an appropriate value
for the unsigned type (typically 0 or the maximum uint32_t value depending on
the logic intent).

In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 526-543: The functions `setEthConfig()` and `ethStop()` are
exposed in the public platform API but lack corresponding stub implementations
in the `#else // MM_NO_ETH` fallback block, causing link errors in builds
without Ethernet support. Add no-op stub implementations for both
`setEthConfig()` and `ethStop()` within the `#else // MM_NO_ETH` preprocessor
block (at lines 564-573) to maintain API symmetry with the main implementation
and ensure shared code that calls these Ethernet functions can compile
successfully on all build configurations.
- Around line 334-336: The Ethernet initialization code in the RMII and W5500
bring-up paths uses fatal ESP_ERROR_CHECK calls and lacks null checks on
esp_netif_new() return values. Replace all ESP_ERROR_CHECK calls throughout the
initialization sequence with proper error checking that logs the issue and
returns false, and add explicit null checks after esp_netif_new() to verify the
ethNetif_ pointer is valid before proceeding; this pattern needs to be applied
consistently across all the Ethernet initialization code paths (including the
sections around lines 394-399, 434-436, and 487-489) to ensure the device fails
gracefully and allows NetworkModule to cascade to WiFi/AP fallback instead of
aborting under memory pressure or transient failures.

In `@test/unit/core/unit_NetworkModule_ethernet.cpp`:
- Around line 52-69: The test case "Desktop Ethernet seam is a safe no-op"
modifies shared platform state via multiple setEthConfig() calls but fails to
restore the configuration at the end, which can cause other tests to fail if
they depend on the default ethernet configuration. Add a cleanup statement at
the end of the test (after the final CHECK_FALSE call) to reset the ethernet
config to ethConfigDefault by calling setEthConfig(ethConfigDefault) to ensure
tests remain order-independent and prevent cross-test state leakage.

---

Outside diff comments:
In @.github/workflows/release.yml:
- Line 283: Update the release and deploy-pages jobs in the GitHub Actions
workflow to comply with coding guidelines that require all Python invocations to
use uv run. In the release job (around line 283), add a setup step using
astral-sh/setup-uv@v3 and change the python scripts/build/generate_manifest.py
invocation to uv run python scripts/build/generate_manifest.py. Apply the same
fix to the deploy-pages job at line 397 by adding the setup-uv action and
updating the command to use uv run.

In `@docs/history/decisions.md`:
- Around line 820-823: The decision documented in decisions.md clearly states
there is no `devices.json` split (one catalog, one entry type), but
architecture.md and backlog.md still describe this `devices.json` split as
deferred rather than rejected. Search for references to the `devices.json` split
being deferred in both architecture.md and backlog.md, then update those
references to either explicitly state it is rejected (to align with the decision
in decisions.md), or alternatively update the statement in decisions.md to say
it is deferred instead. Ensure only one consistent statement of this decision
exists across all architecture documentation files to maintain single source of
truth.

In `@docs/install/index.html`:
- Around line 560-565: Update the Ethernet documentation note at the end of the
paragraph to reflect that boards with different RMII pins (like WT32-ETH01) can
now be configured at runtime through the catalog-driven configuration system
instead of requiring a local rebuild. Replace the statement about needing a
local rebuild with information about how to configure Ethernet pins and PHY
selection at runtime.

In `@README.md`:
- Line 29: The README.md file currently claims that the P4's Parlio engine can
drive up to 20 simultaneous strands, but this contradicts the actual hardware
limit documented in the architecture documentation which caps Parlio at 1–8
lanes. Update the P4 Parlio claim in the parallel WS2812 output section to
reflect the correct lane count of 1–8 instead of 20 to ensure the documentation
accurately represents the hardware capabilities.

In `@scripts/run/preview_installer.py`:
- Line 91: The `json` module is imported twice: once at module level and again
locally inside a function at line 91. Remove the redundant local import
statement since the module is already available at the module scope and can be
used directly without reimporting.

In `@src/ui/install-picker.js`:
- Around line 710-713: The JSDoc comment for the TX-power property in the
install-picker.js file (around line 710-713) contains an outdated reference to
controls.Network.txPowerSetting, which is no longer part of the implementation
schema. Update the JSDoc documentation to remove this incorrect schema path
reference and ensure the comment accurately reflects the current implementation
without mentioning the removed controls.Network path, so that future catalog
edits are not misled by outdated documentation.

---

Duplicate comments:
In `@scripts/check/check_boards.py`:
- Around line 93-99: The firmware validation in the firmwares entries check only
validates that entries are strings but does not check if those strings are
non-empty. Add a validation step after the existing type check (the non_str list
comprehension) to identify any empty string entries in the firmwares list, and
if found, append an appropriate error message to the errors list indicating that
firmware entries must be non-empty strings. This ensures that invalid empty
firmware IDs cannot slip through the validation.

In `@scripts/moondeck.py`:
- Line 323: The nested private helper functions `_collect` and `_store` are
missing return type annotations, triggering the ANN202 lint rule. Add `-> None`
return type annotations to the function signatures: add `-> None` after the
closing parenthesis of the `_collect` function at line 323 and after the closing
parenthesis of the `_store` function at line 1015 to explicitly declare that
both functions return None.

🪄 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: 1b10adbc-0f6b-41a1-aee3-b317fa859476

📥 Commits

Reviewing files that changed from the base of the PR and between d08612e and 1161e77.

⛔ Files ignored due to path filters (4)

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

📒 Files selected for processing (44)

• .github/workflows/release.yml
• README.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/building.md
• docs/history/decisions.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/firmwares.json
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/FirmwareUpdateModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/core/SystemModule.md
• docs/performance.md
• esp32/main/Kconfig.projbuild
• esp32/main/idf_component.yml
• esp32/sdkconfig.defaults.esp32p4-eth
• esp32/sdkconfig.defaults.esp32p4-eth-wifi
• esp32/sdkconfig.defaults.eth
• esp32/sdkconfig.defaults.eth-spi
• scripts/MoonDeck.md
• scripts/check/check_boards.py
• scripts/moondeck.py
• scripts/moondeck_ui/app.js
• scripts/run/preview_installer.py
• src/core/Control.cpp
• src/core/JsonUtil.h
• src/core/NetworkModule.h
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• src/ui/install-picker.js
• test/CMakeLists.txt
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/core/scenario_NetworkModule_eth_reconfigure.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Driver_mutation.json
• test/unit/core/unit_Control_apply_absent_key.cpp
• test/unit/core/unit_NetworkModule_ethernet.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Drop the hardcoded-list wording.

Line 190 still says MoonDeck falls back to a short hardcoded board list, but the new catalog-driven flow reads boards.json for the picker and fallback too. Please update the wording so this section doesn't imply a second source of truth.

🤖 Prompt for AI Agents

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

In `@docs/backlog/backlog.md` around lines 190 - 191, The documentation currently
references a "short hardcoded board list" as a fallback mechanism, but the
implementation has moved to a catalog-driven approach using boards.json for both
the board picker and fallback. Remove the "hardcoded list" wording and update
the text to clarify that MoonDeck now uses the boards.json catalog to both let
users pick a board and provide fallback options when the firmware does not
uniquely identify the hardware.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

hasKey can match key-like text inside string values and still misapply absent-key overlays.

std::strstr("\"key\":") is not object-member aware; it can match escaped text inside a string value (e.g., {\"note\":\"...\\\"ethType\\\":0...\"}), causing hasKey to return true for an actually absent control key and reintroducing unintended value writes downstream.

Suggested fix (member-aware flat scan without allocations)

 inline bool hasKey(const char* json, const char* key) {
     if (!json || !key) return false;
-    char search[48];
-    std::snprintf(search, sizeof(search), "\"%s\":", key);
-    return std::strstr(json, search) != nullptr;
+    const size_t keyLen = std::strlen(key);
+    if (keyLen == 0) return false;
+
+    bool inString = false;
+    for (const char* p = json; *p; ++p) {
+        if (*p == '"' && (p == json || p[-1] != '\\')) {
+            inString = !inString;
+            if (inString) {
+                const char* s = p + 1;
+                if (std::strncmp(s, key, keyLen) == 0 && s[keyLen] == '"') {
+                    const char* q = s + keyLen + 1;
+                    while (*q == ' ' || *q == '\t' || *q == '\n' || *q == '\r') ++q;
+                    if (*q == ':') return true;
+                }
+            }
+        }
+    }
+    return false;
 }

🤖 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/JsonUtil.h` around lines 43 - 52, The hasKey function uses a naive
std::strstr search that cannot distinguish between actual JSON object members
and text that happens to appear inside string values, causing false positives
for absent keys when escaped patterns exist in values. Implement a JSON-aware
search in hasKey that iterates through the JSON string while tracking whether
the current position is inside a string value (handling escaped quotes), and
only matches the key pattern when positioned at the object member level outside
of any string values. This ensures the function correctly identifies
genuinely-present keys without matching similar patterns embedded in value
strings.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid panic paths in runtime Ethernet init; fail closed and return false.

Both RMII and W5500 bring-up still use fatal ESP_ERROR_CHECK calls and skip esp_netif_new() null checks. Under memory pressure or transient init failures, this can abort the device instead of letting NetworkModule cascade to WiFi/AP.

Suggested fix pattern

 static bool ethInitRmii() {
     esp_netif_config_t netif_cfg = ESP_NETIF_DEFAULT_ETH();
     ethNetif_ = esp_netif_new(&netif_cfg);
+    if (!ethNetif_) {
+        ESP_LOGE(NET_TAG, "Ethernet netif create failed");
+        return false;
+    }

@@
-    ESP_ERROR_CHECK(esp_netif_attach(ethNetif_, esp_eth_new_netif_glue(eth_handle)));
-    ESP_ERROR_CHECK(esp_event_handler_register(ETH_EVENT, ESP_EVENT_ANY_ID,
-                                               &ethEventHandler, nullptr));
-    ESP_ERROR_CHECK(esp_event_handler_register(IP_EVENT, IP_EVENT_ETH_GOT_IP,
-                                               &ethEventHandler, nullptr));
+    esp_err_t err = esp_netif_attach(ethNetif_, esp_eth_new_netif_glue(eth_handle));
+    if (err != ESP_OK) {
+        esp_eth_driver_uninstall(eth_handle);
+        if (ethNetif_) { esp_netif_destroy(ethNetif_); ethNetif_ = nullptr; }
+        return false;
+    }
+    err = esp_event_handler_register(ETH_EVENT, ESP_EVENT_ANY_ID, &ethEventHandler, nullptr);
+    if (err != ESP_OK) {
+        esp_eth_driver_uninstall(eth_handle);
+        if (ethNetif_) { esp_netif_destroy(ethNetif_); ethNetif_ = nullptr; }
+        return false;
+    }
+    err = esp_event_handler_register(IP_EVENT, IP_EVENT_ETH_GOT_IP, &ethEventHandler, nullptr);
+    if (err != ESP_OK) {
+        esp_event_handler_unregister(ETH_EVENT, ESP_EVENT_ANY_ID, &ethEventHandler);
+        esp_eth_driver_uninstall(eth_handle);
+        if (ethNetif_) { esp_netif_destroy(ethNetif_); ethNetif_ = nullptr; }
+        return false;
+    }
 }

Also applies to: 394-399, 434-436, 487-489

🤖 Prompt for AI Agents

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

In `@src/platform/esp32/platform_esp32.cpp` around lines 334 - 336, The Ethernet
initialization code in the RMII and W5500 bring-up paths uses fatal
ESP_ERROR_CHECK calls and lacks null checks on esp_netif_new() return values.
Replace all ESP_ERROR_CHECK calls throughout the initialization sequence with
proper error checking that logs the issue and returns false, and add explicit
null checks after esp_netif_new() to verify the ethNetif_ pointer is valid
before proceeding; this pattern needs to be applied consistently across all the
Ethernet initialization code paths (including the sections around lines 394-399,
434-436, and 487-489) to ensure the device fails gracefully and allows
NetworkModule to cascade to WiFi/AP fallback instead of aborting under memory
pressure or transient failures.