Board injection + LOLIN S3 N16R8 enablement

[ewowi]

Summary

Multi-commit feature: get the physical-board identity onto the device (persisted, reported via /api/state, settable from MoonDeck and the web installer) so the device knows what hardware it's running on. The device cannot self-identify board (no readable PCB ID on classic ESP32), so the value is injected by an outside tool.

The branch grew during the work: after the first four commits landed the board-injection core, the next three commits added LOLIN S3 N16R8 support (which exposed three real bugs the board-injection feature happened to depend on) and folded in two CodeRabbit review rounds.

Seven commits, each a working stop along the way:

Board injection core (commits 1-4)

1. 8a76be2 — BoardModule (code-wired child of System, board Text control with the new readonly UI flag, persisted via FilesystemModule), docs/install/boards.json catalog (single source of truth, shared across all consumers), MoonDeck pushes via the existing POST /api/control. Bonus: new readonly flag on ControlDescriptor lets a Text control render display-only in the UI without losing persistence semantics.

2. 058d979 — Web installer board picker, firmware-name collision fix on SystemModule (was binding firmware twice with different types).

3. 7354923 — Replaced ESP Web Tools with a custom orchestrator (docs/install/install-orchestrator.js) that owns the SerialPort across flash → Improv WiFi provision → SET_BOARD vendor RPC (command 0xFE, raw frame). Same root cause that broke EWT's board-injection also broke devices.js's "Your devices" auto-add — both fixed by owning the port end-to-end.

4. 9f888af — Board-injection follow-ups: HTTP fallback for cases where Improv isn't available (Ethernet-only firmware, device already provisioned), Inject button on the "Your devices" row, picker module renamed from release-picker → install-picker to reflect its grown scope.

LOLIN S3 N16R8 enablement (commit 5)

5. 751f1d5 — New "LOLIN S3 N16R8" board entry in boards.json. The board exposed three connected bugs that the board-injection feature was masking:

   • WiFi brown-out at full TX power. The LOLIN's on-module LDO can't sustain full radio TX; the device drops WiFi during association. Fix: writable Network.txPowerSetting control (0..21 dBm, 0=no override), new platform::wifiSetTxPower setter, applied via syncTxPower() re-checked on every loop1s tick. boards.json injects Network.txPowerSetting: 8 for the LOLIN entry.
   • USB-Serial-JTAG, not UART0. S3 native-USB boards expose USB-C via the chip's USB-Serial-JTAG peripheral, not via an external bridge to UART0. ESP-IDF's secondary-console feature mirrors stdio out both, so boot logs look fine — but the Improv listener-on-UART0 was deaf to host writes. Fix: install BOTH drivers, read symmetric non-blocking from both transports per loop iteration. Guarded by #if SOC_USB_SERIAL_JTAG_SUPPORTED so ESP32-classic builds stay clean.
   • CORS preflight silently blocked every cross-origin POST. The device's HTTP server returned 405 to OPTIONS, so the browser silently dropped subsequent /api/control POSTs from the web installer. Root cause of every "inject didn't work" symptom for the board-injection feature itself. Fix: OPTIONS handler returning 204 + Access-Control-Allow-* headers.

   Plus: Monitor button + serial-monitor modal in the web installer (live read, RTS-pulse reset), Retry on "Improv not detected" dialog (slow-boot races), lazy-render WiFi-creds form (avoids macOS iCloud Passwords prompt on page load), maxInternalAllocBlock split (the diagnostic maxBlock was reporting PSRAM ~8MB instead of the useful internal-block KPI), MoonDeck full controls fan-out (was Board.board only).

Review-driven cleanup (commits 6-7)

6. eca3c2c — CodeRabbit round 1: 8 findings folded in. Most important: TX-power cap not re-applied after radio restart — cached appliedTxPowerSetting_ invariant was violated whenever ESP-IDF stops the radio (AP→STA cascade, STA reconnect, AP shutdown). New noteRadioStopped() helper called at every wifiStaStop/wifiApStop site invalidates the cache.

7. 6212fc0 — CodeRabbit round 2: 3 findings folded in, 1 deferred with explicit doc note. Most important: syncTxPower() now called immediately after each successful wifiStaInit/wifiApInit — closes the up-to-1s full-power window between WiFi-up and the next loop1s tick (exactly the LOLIN brown-out scenario). Plus per-transport Improv reply routing (replies now go only on the requesting transport instead of broadcast), needs-ip dialog whitespace rejection via setCustomValidity, BoardModule.h validation-asymmetry doc note. Also added NanoDLA v1.3 wiring + capture instructions to leddriver-analysis-top-down.md.

Net: a fresh device flashed from the web installer ends up with BoardModule.board = "LOLIN S3 N16R8" (or whichever the user picked) persisted to /.config/BoardModule.json, surviving reboot. The LOLIN-specific TX-power cap is injected via the same generic controls.* fan-out and lands within ~50ms of WiFi-up. MoonDeck mirrors the same values via HTTP. Future per-board state (Ethernet pin maps, default module-config overrides) uses the same controls.<Module>.<control> shape and needs no code changes.

KPI (head of branch, esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm)

PC:375KB | tick:354/100/303/99/99/98/45/35/34us
          (FPS:2824/10000/3300/10101/10101/10204/22222/28571/29411)
ESP32:1171KB | tick:138506us(FPS:7)
heap free: 8358927  maxBlock (internal): 163840 (160 KB)
ArtNetSend: ~93 ms/tick (LOLIN at 8 dBm — known hardware trade-off)
src:62(10592) | test:38(5410) | lizard:40w

The LOLIN S3 at 8 dBm is fundamentally slower than Olimex Ethernet on ArtNet (cut TX power = slower UDP throughput on association margin). Known hardware-level trade-off of the brown-out fix, not a code regression. Use Ethernet-capable boards if FPS matters; use LOLIN S3 N16R8 if you need PSRAM headroom and accept the WiFi compromise.

Test plan

• Desktop build clean (cmake --build build, -Werror)
• ctest — 207 unit tests pass (8 BoardModule tests added)
• mm_scenarios — 8/8 pass
• Platform boundary check PASS
• Spec check — 26/26 (BoardModule.md + NetworkModule.md txPowerSetting bullet)
• ESP32 builds — esp32, esp32-eth, esp32-eth-wifi, esp32s3-n16r8 all compile clean
• Hardware verification on LOLIN S3 N16R8:
  • Step 1: MoonDeck deduces board for unique-firmware boards; pushes via POST /api/control on discover/refresh; /.config/BoardModule.json survives reboot
  • Step 2: web installer picks the LOLIN board, flashes via custom orchestrator, provisions WiFi via Improv-over-USB-JTAG, sets board via SET_BOARD, HTTP-fan-outs Network.txPowerSetting: 8
  • Step 3: device's txPower reports 8 dBm post-association, persists across reboot, re-applies on AP→STA cascade
  • Step 4: cross-origin POSTs from preview_installer to device API now succeed (CORS OPTIONS handler returns 204)
• CodeRabbit findings: 21 threads, 15 fixed, 6 rejected/deferred with explicit rationale; all resolved on PR.
• On-demand pre-commit Reviewer (commit eca3c2c): 22 findings, all addressed in 6212fc0.
• PR-merge Reviewer agent: running at merge time over git diff main...HEAD.

🤖 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: 72654444-cc7a-441c-8c4d-742697ea77a5

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: 5

Caution

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

⚠️ Outside diff range comments (1)

scripts/moondeck.py (1)

928-960: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider skipping redundant board pushes.

The refresh flow pushes board to every online device with a non-empty board, even when the device already reports the same value. While the device's 2-second debounce coalesces redundant writes, this still generates unnecessary HTTP traffic on every refresh.

Consider adding the same check used in discover:

                for dev in merged:
-                   if dev.get("online") and dev.get("board") and dev.get("ip"):
+                   if dev.get("online") and dev.get("board") and dev.get("ip"):
+                       # Only push when MoonDeck's board differs from device's
+                       device_board = ""
+                       # dev came from refresh_devices which merges probe data
+                       for prior in (target.get("devices") or []):
+                           if prior.get("ip") == dev.get("ip"):
+                               # prior's board is what probe returned
+                               break
                        pushes.append((dev["ip"], dev["board"]))

Alternatively, store the probed board separately in refresh_devices results so you can compare.

🤖 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` around lines 928 - 960, The _merge_refresh currently
enqueues pushes for every online device with a non-empty board regardless of
whether the device already reports that same board; update the push decision to
skip redundant pushes by comparing the merged device's board to the device's
previously-known board before appending to pushes. Inside _merge_refresh (where
merged and prior device list are available), find the prior device entry for the
same ip (from the earlier target.get("devices") or the prior loop) and only
append (dev["ip"], dev["board"]) to pushes when dev.get("board") is truthy and
dev.get("board") != prior_board (or if no prior entry existed). Keep using
mutate_state/_merge_refresh and _push_boards_in_parallel unchanged otherwise.

🤖 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 181: Update the sentence that mentions the web installer as future work:
remove the timeline-relative phrase “(next commit)” and rephrase to state that
the catalog at docs/install/boards.json is shared between MoonDeck and the web
installer today; keep references to BoardModule, the device-side readonly
`board` Text control, and the mirror behavior (POST /api/control after
discover/dropdown change) unchanged so the architecture text reflects current
installer integration.

In `@docs/install/install-orchestrator.js`:
- Around line 354-361: Add an inline comment above the string match in the catch
block that explains this fragile check relies on the pinned SDK version
(referencing the pinned SDK at the top of the file) — specifically note that the
condition msg.includes("Improv Wi-Fi Serial not detected") is dependent on that
SDK error text and should be revisited if the SDK version is changed; leave the
logic setting alreadyOnline as-is but document the dependency for future
maintainers.

In `@docs/moonmodules/core/BoardModule.md`:
- Line 3: Update the stale future-tense wording in the BoardModule description
to present tense and reflect that installer integration (Step 3) is live:
rewrite the paragraph so it states that the physical board identity (e.g.,
`Olimex ESP32-Gateway Rev G`, `LOLIN D32`) is injected by external tools today
(MoonDeck and the web installer), that the `board` Text control is bound with
the `readonly` UI flag (display-only in the device web UI while HTTP writes
still accept injector updates), and that the catalog
(`docs/install/boards.json`) is the source of truth with MoonDeck mirroring the
picked/deduced value back on discovery if it drifts; ensure any "tomorrow/Step
2" phrasing is removed and the language is present-tense throughout.

In `@scripts/MoonDeck.md`:
- Line 12: The sentence in scripts/MoonDeck.md describing the board catalog is
in future tense ("the web installer will use"); update it to present tense to
reflect current behavior—e.g., change "the web installer will use" to "the web
installer uses" (the rest of the sentence mentioning docs/install/boards.json,
BoardModule and the POST /api/control flow should remain unchanged so the text
continues to say the dropdown uses docs/install/boards.json and MoonDeck mirrors
the chosen board to the device's BoardModule via POST /api/control).

In `@src/core/HttpServerModule.cpp`:
- Line 390: Wrap the single-line conditional that appends the readonly JSON
field in braces to satisfy -Wall -Wextra -Werror; locate the if (c.readonly)
sink.append(",\"readonly\":true"); statement in HttpServerModule.cpp and change
it to a braced block (open brace before the append call and close brace after)
preserving existing indentation and formatting so the behavior is unchanged.

---

Outside diff comments:
In `@scripts/moondeck.py`:
- Around line 928-960: The _merge_refresh currently enqueues pushes for every
online device with a non-empty board regardless of whether the device already
reports that same board; update the push decision to skip redundant pushes by
comparing the merged device's board to the device's previously-known board
before appending to pushes. Inside _merge_refresh (where merged and prior device
list are available), find the prior device entry for the same ip (from the
earlier target.get("devices") or the prior loop) and only append (dev["ip"],
dev["board"]) to pushes when dev.get("board") is truthy and dev.get("board") !=
prior_board (or if no prior entry existed). Keep using
mutate_state/_merge_refresh and _push_boards_in_parallel unchanged otherwise.

🪄 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: 79239a4f-23e2-4d30-aa59-06cc7d5e5923

📥 Commits

Reviewing files that changed from the base of the PR and between 015bd02 and 7354923.

📒 Files selected for processing (35)

• .github/workflows/release.yml
• docs/architecture.md
• docs/install/boards.json
• docs/install/devices.js
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/SystemModule.md
• docs/plan.md
• scripts/MoonDeck.md
• scripts/moondeck.py
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/style.css
• scripts/run/preview_installer.py
• src/core/BoardModule.h
• src/core/Control.h
• src/core/HttpServerModule.cpp
• src/core/ImprovProvisioningModule.h
• src/core/SystemModule.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/release-picker.js
• test/CMakeLists.txt
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_resize.json
• test/scenarios/light/scenario_Layer_buildup.json
• test/scenarios/light/scenario_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/unit/core/unit_BoardModule.cpp
• test/unit/core/unit_SystemModule.cpp

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (8)

test/unit/core/unit_BoardModule.cpp (1)

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

Assert readonly on the board control to lock the UI contract.

This test verifies type/wiring/max but misses c.readonly == true, which is core behavior for BoardModule::onBuildControls(). Add it here to prevent regressions that make board editable again.

Suggested patch

     CHECK(c.type == mm::ControlType::Text);
+    CHECK(c.readonly);
     // The control's ptr points at the module's internal boardKey_ buffer;

As per coding guidelines, "test/**: ... 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/unit/core/unit_BoardModule.cpp` around lines 23 - 33, The test is
missing an assertion that the board control is marked readonly; update the unit
test in unit_BoardModule.cpp (the block that inspects board.controls()[0]) to
assert c.readonly == true so it verifies the UI contract produced by
BoardModule::onBuildControls(); locate the control check around
board.controls().count(), the local const auto& c = board.controls()[0], and add
the readonly check alongside the existing checks that validate c.name, c.type,
c.ptr (board.board()), and c.max to prevent regressions that make the board
editable.

docs/install/README.md (1)

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

Align intro wording with the custom installer architecture.

The opening line still brands this as an “ESP Web Tools installer page,” but this flow is now orchestrator-driven with EWT removed from the install-button path. Tightening the intro avoids mixed messaging in the same doc.

Suggested wording

-This directory holds the source for the **ESP Web Tools installer page** at
+This directory holds the source for the **projectMM web installer page** at

🤖 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/README.md` around lines 3 - 4, Update the README's opening line
that currently says "This directory holds the source for the **ESP Web Tools
installer page**..." to reflect the new orchestrator-driven custom installer
architecture: remove the "ESP Web Tools installer page" branding and replace it
with a concise description such as "This directory holds the source for the
custom installer page driven by the orchestrator" (or equivalent wording that
emphasizes the orchestrator-driven flow and that EWT is no longer on the
install-button path); update the top-level introductory sentence so the document
consistently conveys the new installer flow.

scripts/MoonDeck.md (1)

65-66: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Update flash-ready flow text to the current installer runtime.

This still says the flow opens the ESP Web Tools Improv modal, but the branch replaced EWT with the custom install orchestrator. The current wording misstates what developers should expect during local validation.

Suggested doc patch

-- **Flash-ready.** When at least one ESP32 build exists, the script additionally stages every `build/esp32-*/projectMM.bin` it finds into `releases/local-dev/` and generates matching Pages-relative manifests via the same `generate_manifest.py` the release workflow uses. The picker shows `local-dev` as the newest tag; clicking **Install** flashes a USB-connected ESP32 and opens the ESP Web Tools Improv WiFi modal — end-to-end, same code paths as the public installer. This is the developer's test ground for the install flow before deploying to GitHub Pages: Web Serial works on `http://localhost` without the secure-origin requirement that gates the public site.
+- **Flash-ready.** When at least one ESP32 build exists, the script additionally stages every `build/esp32-*/projectMM.bin` it finds into `releases/local-dev/` and generates matching Pages-relative manifests via the same `generate_manifest.py` the release workflow uses. The picker shows `local-dev` as the newest tag; clicking **Install** flashes a USB-connected ESP32 and runs the custom install orchestrator (flash → provision → board injection) — end-to-end, same code paths as the public installer. This is the developer's test ground for the install flow before deploying to GitHub Pages: Web Serial works on `http://localhost` without the secure-origin requirement that gates the public site.

🤖 Prompt for AI Agents

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

In `@scripts/MoonDeck.md` around lines 65 - 66, Update the "Flash-ready."
paragraph so it no longer says the flow opens the "ESP Web Tools Improv WiFi
modal" and instead describes the current installer runtime: when at least one
ESP32 build exists the script stages build/esp32-*/projectMM.bin into
releases/local-dev/ and generates Pages manifests via generate_manifest.py, the
picker shows "local-dev" and clicking "Install" flashes a USB-connected ESP32
and invokes the repository's custom install orchestrator UI (not ESP Web Tools),
following the same end-to-end code paths as the public installer for local
validation.

src/ui/app.js (1)

174-181: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't use the fire-and-forget control helper for the one-shot board handoff.

consumePendingBoardParam() deletes ?board= before posting anything, but sendControl() swallows network errors and ignores non-2xx responses. A failed /api/control write therefore looks like success and the retry token is already gone, so board injection can silently disappear.

Also applies to: 206-247

🤖 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 174 - 181, The current sendControl function
swallows network errors and ignores non-2xx responses, causing
consumePendingBoardParam to delete the ?board= token before ensuring the
/api/control write succeeded; update sendControl to await fetch, check
response.ok and reject/throw on non-ok or network errors so callers can detect
failure, and modify consumePendingBoardParam to only remove the retry token (the
query param) after sendControl resolves successfully, otherwise preserve the
token and surface/log the error so the board handoff can be retried.

docs/install/devices.js (1)

277-302: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear stale pendingBoard when a later install no longer needs it.

For existing rows, pendingBoard is only written when pendingBoardPush is true and never cleared otherwise. Because buildInjectUrl() prefers pendingBoard, a device that was once pending and later reinstalled successfully can still inject the old board config.

Suggested fix

         if (existing) {
             existing.lastSeen = now;
             // Only overwrite board when caller supplied a value — re-flashing
             // with "(any board)" mustn't blank a previously-set entry.
             if (board) existing.board = board;
-            if (pendingBoardPush) existing.pendingBoard = board;
+            if (pendingBoardPush) existing.pendingBoard = board;
+            else delete existing.pendingBoard;
         } else {

🤖 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/devices.js` around lines 277 - 302, In addProvisionedDevice,
existing devices never have their pendingBoard cleared when pendingBoardPush is
false, so later calls can still use stale pendingBoard; update the
existing-device branch in addProvisionedDevice (and ensure new entries don't
inherit it) to explicitly remove or unset existing.pendingBoard when
pendingBoardPush is falsy (use delete existing.pendingBoard or set to
undefined/null) and only set existing.pendingBoard = board when pendingBoardPush
is true so buildInjectUrl() will not prefer an out-of-date pendingBoard.

docs/install/index.html (1)

605-620: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear the Wi‑Fi password between install sessions.

#wifi-password is never reset when the modal opens or when the Wi‑Fi step completes, so the previous network password stays in the DOM and is prefilled on the next install attempt. That unnecessarily retains sensitive data after the flow is over.

Suggested fix

 function openModal(titleText) {
   title.textContent = titleText;
   backdrop.classList.add("open");
+  document.getElementById("wifi-password").value = "";
   // Reset the log per install session so users see only the current run.
   document.getElementById("install-log").textContent = "";
@@
         case "wifi-creds-form": {
           showSection("wifiForm");
           // Prefill the last-used SSID (not password — that stays out of
           // localStorage as a privacy / security-scanner concession). Focus
           // password if SSID is prefilled (user only needs to type the
           // password), else focus SSID.
           const ssidEl = document.getElementById("wifi-ssid");
           const passEl = document.getElementById("wifi-password");
+          passEl.value = "";

Also applies to: 716-729, 768-779

🤖 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 605 - 620, The Wi-Fi password input
(`#wifi-password`) is never cleared and retains sensitive data between installs;
update the modal open logic (function openModal) to explicitly clear the
password element by setting document.getElementById("wifi-password").value = ""
and also clear any value attribute or textContent as appropriate; additionally,
ensure the same clearing happens when the Wi‑Fi step completes or when the flow
ends (e.g., in the handlers that run on done / error / cancel or in
disarmUnloadGuard-related completion functions) so the password is removed from
the DOM regardless of how the modal closes.

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

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

Update the stale buffer-size comment.

The comment at line 54 says "module-owned BoardModule buffer (24 bytes)" but BoardModule::boardKey_ is now 32 bytes (changed in this PR). The comment should reflect the current size.

📝 Proposed fix

     // Vendor SET_BOARD RPC (command 0xFE): module-owned BoardModule buffer
-    // (24 bytes) + ready flag. Same producer/consumer dance as ssid/password.
+    // (32 bytes) + ready flag. Same producer/consumer dance as ssid/password.

🤖 Prompt for AI Agents

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

In `@src/platform/esp32/platform_esp32_improv.cpp` around lines 53 - 59, The
comment describing the vendor SET_BOARD buffer size is stale—update the comment
referencing the module-owned BoardModule buffer to reflect that
BoardModule::boardKey_ is now 32 bytes (not 24); locate the block with boardOut,
boardOutLen and boardReady and change the "(24 bytes)" remark to "(32 bytes)"
and/or reword to derive size from BoardModule::boardKey_ to avoid future drift.

src/core/BoardModule.h (1)

53-71: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Document the rationale for the ASCII-printable constraint.

The validation enforces ASCII-printable bytes (0x20–0x7E), which is a bespoke convention not widely recognized from standard frameworks. As per coding guidelines, "Document exception reasons for bespoke conventions (not recognized from widely-used projects/frameworks) in a one-line comment at the introduction site."

Please add a brief justification explaining why this constraint exists (e.g., JSON safety, display compatibility, wire protocol requirements, catalog alignment).

📝 Example justification

     // External setter for transports that bypass /api/control (today: Improv
-    // vendor RPC SET_BOARD from the web installer). Validates: 1..31 chars,
-    // ASCII-printable (0x20–0x7E), no embedded NUL. Returns false on
+    // vendor RPC SET_BOARD from the web installer). Validates: 1..31 chars,
+    // ASCII-printable (0x20–0x7E) for JSON/display safety + catalog alignment,
+    // no embedded NUL. Returns false on

As per coding guidelines: documentation for bespoke conventions must be present.

🤖 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/BoardModule.h` around lines 53 - 71, Add a one-line comment above
the setBoard function explaining the ASCII-printable (0x20–0x7E) constraint and
why it is required (e.g., to ensure safe JSON/HTTP/wire encoding, human-readable
display, and to avoid embedded NULs that would break C-string semantics); update
the comment near the setBoard declaration (referencing setBoard and boardKey_)
so future readers know this bespoke validation rationale while leaving the
existing validation logic and calls to markDirty() and
FilesystemModule::noteDirty() unchanged.

♻️ Duplicate comments (1)

src/core/HttpServerModule.cpp (1)

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

Wrap the single-statement if in braces.

This is the same issue flagged in a previous review. The coding guideline requires building with -Wall -Wextra -Werror, and Clang flags this single-statement if as missing braces (readability-braces-around-statements). As per coding guidelines, no warning is allowed — either fix it or silence it explicitly with a justified -Wno-….

🔧 Proposed fix

-        if (c.readonly) sink.append(",\"readonly\":true");
+        if (c.readonly) {
+            sink.append(",\"readonly\":true");
+        }

As per coding guidelines: build with -Wall -Wextra -Werror.

🤖 Prompt for AI Agents

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

In `@src/core/HttpServerModule.cpp` at line 398, The single-statement if checking
c.readonly should be wrapped in braces to satisfy
readability-braces-around-statements; update the expression that currently reads
"if (c.readonly) sink.append(...)" to use a braced block (e.g., if (c.readonly)
{ sink.append(...); }) so the compiler warnings are eliminated; locate the
occurrence using the symbols c and sink in HttpServerModule.cpp and apply the
brace fix.

🤖 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/devices.js`:
- Around line 178-185: Update the erase confirmation text in the erase button
callback (the erase variable created via makeBtn) to remove the outdated
reference to "ESP Web Tools will offer to flash a fresh firmware" and instead
describe the current flow that runs installer.eraseOnly(); e.g., warn that this
will wipe WiFi credentials and module state and that a fresh firmware install
will not be performed automatically, so the user must re-install firmware
manually or via the installer after erase; keep references to device.name and
the existing state.onErase(device) callback intact.

In `@docs/install/install-orchestrator.js`:
- Around line 355-360: The saved prePickedPort handle is used unconditionally
which prevents reprompting if the handle is stale; update both start() and
eraseOnly() to first attempt to open the prePickedPort (call its open/connect
method) inside a try/catch, and if that attempt throws or fails, clear or ignore
prePickedPort and then call navigator.serial.requestPort({}) to show the native
picker; ensure the same pattern is applied to the other affected block (the
similar code around lines 621-642) so any stale opts.port triggers a re-prompt
and the existing reconnect fallback can operate correctly.
- Around line 536-547: After uiWaitForCreds() returns, detect a Skip by checking
if ssid is an empty string (e.g., const { ssid, password } = await
uiWaitForCreds(); if (!ssid) ...). If ssid is empty, do not call
improvClient.provision(); instead transition to the SoftAP/Skip path (e.g.,
trackProgress for the softAP flow or call the existing softAP handler) and avoid
throwing a provisioning error. Only call improvClient.provision(ssid, password,
30000) and check improvClient.nextUrl/deviceUrl when ssid is non-empty.

---

Outside diff comments:
In `@docs/install/devices.js`:
- Around line 277-302: In addProvisionedDevice, existing devices never have
their pendingBoard cleared when pendingBoardPush is false, so later calls can
still use stale pendingBoard; update the existing-device branch in
addProvisionedDevice (and ensure new entries don't inherit it) to explicitly
remove or unset existing.pendingBoard when pendingBoardPush is falsy (use delete
existing.pendingBoard or set to undefined/null) and only set
existing.pendingBoard = board when pendingBoardPush is true so buildInjectUrl()
will not prefer an out-of-date pendingBoard.

In `@docs/install/index.html`:
- Around line 605-620: The Wi-Fi password input (`#wifi-password`) is never
cleared and retains sensitive data between installs; update the modal open logic
(function openModal) to explicitly clear the password element by setting
document.getElementById("wifi-password").value = "" and also clear any value
attribute or textContent as appropriate; additionally, ensure the same clearing
happens when the Wi‑Fi step completes or when the flow ends (e.g., in the
handlers that run on done / error / cancel or in disarmUnloadGuard-related
completion functions) so the password is removed from the DOM regardless of how
the modal closes.

In `@docs/install/README.md`:
- Around line 3-4: Update the README's opening line that currently says "This
directory holds the source for the **ESP Web Tools installer page**..." to
reflect the new orchestrator-driven custom installer architecture: remove the
"ESP Web Tools installer page" branding and replace it with a concise
description such as "This directory holds the source for the custom installer
page driven by the orchestrator" (or equivalent wording that emphasizes the
orchestrator-driven flow and that EWT is no longer on the install-button path);
update the top-level introductory sentence so the document consistently conveys
the new installer flow.

In `@scripts/MoonDeck.md`:
- Around line 65-66: Update the "Flash-ready." paragraph so it no longer says
the flow opens the "ESP Web Tools Improv WiFi modal" and instead describes the
current installer runtime: when at least one ESP32 build exists the script
stages build/esp32-*/projectMM.bin into releases/local-dev/ and generates Pages
manifests via generate_manifest.py, the picker shows "local-dev" and clicking
"Install" flashes a USB-connected ESP32 and invokes the repository's custom
install orchestrator UI (not ESP Web Tools), following the same end-to-end code
paths as the public installer for local validation.

In `@src/core/BoardModule.h`:
- Around line 53-71: Add a one-line comment above the setBoard function
explaining the ASCII-printable (0x20–0x7E) constraint and why it is required
(e.g., to ensure safe JSON/HTTP/wire encoding, human-readable display, and to
avoid embedded NULs that would break C-string semantics); update the comment
near the setBoard declaration (referencing setBoard and boardKey_) so future
readers know this bespoke validation rationale while leaving the existing
validation logic and calls to markDirty() and FilesystemModule::noteDirty()
unchanged.

In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 53-59: The comment describing the vendor SET_BOARD buffer size is
stale—update the comment referencing the module-owned BoardModule buffer to
reflect that BoardModule::boardKey_ is now 32 bytes (not 24); locate the block
with boardOut, boardOutLen and boardReady and change the "(24 bytes)" remark to
"(32 bytes)" and/or reword to derive size from BoardModule::boardKey_ to avoid
future drift.

In `@src/ui/app.js`:
- Around line 174-181: The current sendControl function swallows network errors
and ignores non-2xx responses, causing consumePendingBoardParam to delete the
?board= token before ensuring the /api/control write succeeded; update
sendControl to await fetch, check response.ok and reject/throw on non-ok or
network errors so callers can detect failure, and modify
consumePendingBoardParam to only remove the retry token (the query param) after
sendControl resolves successfully, otherwise preserve the token and surface/log
the error so the board handoff can be retried.

In `@test/unit/core/unit_BoardModule.cpp`:
- Around line 23-33: The test is missing an assertion that the board control is
marked readonly; update the unit test in unit_BoardModule.cpp (the block that
inspects board.controls()[0]) to assert c.readonly == true so it verifies the UI
contract produced by BoardModule::onBuildControls(); locate the control check
around board.controls().count(), the local const auto& c = board.controls()[0],
and add the readonly check alongside the existing checks that validate c.name,
c.type, c.ptr (board.board()), and c.max to prevent regressions that make the
board editable.

---

Duplicate comments:
In `@src/core/HttpServerModule.cpp`:
- Line 398: The single-statement if checking c.readonly should be wrapped in
braces to satisfy readability-braces-around-statements; update the expression
that currently reads "if (c.readonly) sink.append(...)" to use a braced block
(e.g., if (c.readonly) { sink.append(...); }) so the compiler warnings are
eliminated; locate the occurrence using the symbols c and sink in
HttpServerModule.cpp and apply the brace fix.

🪄 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: 20308dd3-8bbe-4744-9cf1-f1d3a6416426

📥 Commits

Reviewing files that changed from the base of the PR and between 7354923 and 9f888af.

⛔ Files ignored due to path filters (2)

• docs/install/favicon.png is excluded by !**/*.png
• scripts/build/improv_smoke_test.py is excluded by !**/build/**

📒 Files selected for processing (28)

• .github/workflows/release.yml
• CLAUDE.md
• CMakeLists.txt
• docs/architecture.md
• docs/history/decisions.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/devices.js
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/FirmwareUpdateModule.md
• docs/performance.md
• docs/plan.md
• esp32/main/CMakeLists.txt
• scripts/MoonDeck.md
• scripts/run/preview_installer.py
• src/core/BoardModule.h
• src/core/HttpServerModule.cpp
• src/core/ImprovProvisioningModule.h
• src/core/SystemModule.h
• src/core/ui_embedded.h
• src/main.cpp
• src/platform/esp32/platform_esp32_improv.cpp
• src/ui/app.js
• src/ui/embed_ui.cmake
• src/ui/install-picker.js
• test/unit/core/unit_BoardModule.cpp

[coderabbitai]

Actionable comments posted: 11

Caution

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

⚠️ Outside diff range comments (1)

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

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

Validate the inner SET_BOARD length fields exactly.

This accepts any payload where strLen <= len - 3, even when payload[1] disagrees with the actual body size or extra trailing bytes are present. For a raw vendor RPC, that is too lax and can persist a board name from a malformed frame.

Suggested fix

-    uint8_t strLen = payload[2];
-    if (strLen == 0 || strLen >= g_improv.boardOutLen || 3u + strLen > len) {
+    uint8_t dataLen = payload[1];
+    uint8_t strLen = payload[2];
+    if (strLen == 0 ||
+        strLen >= g_improv.boardOutLen ||
+        dataLen != static_cast<uint8_t>(1u + strLen) ||
+        len != static_cast<uint8_t>(3u + strLen)) {
         improvSendError(static_cast<improv::Error>(IMPROV_ERROR_INVALID_BOARD));
         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/platform/esp32/platform_esp32_improv.cpp` around lines 259 - 267, The
code accepts frames where payload[1] (data_len) disagrees with payload[2]
(strLen) or where extra trailing bytes exist; update the validation in
platform_esp32_improv.cpp so that after reading uint8_t strLen = payload[2] you
(1) require strLen > 0 && strLen < g_improv.boardOutLen, (2) require payload[1]
== static_cast<uint8_t>(1u + strLen) to ensure data_len matches the strLen+the
str_len byte, and (3) require len == static_cast<size_t>(3u + strLen) so the
frame length exactly matches the expected size; on any mismatch call
improvSendError(static_cast<improv::Error>(IMPROV_ERROR_INVALID_BOARD)) and
return.

🤖 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/devices.js`:
- Around line 295-301: The current logic clears existing.pendingBoard whenever
pendingBoardPush is false, which removes the outstanding-injection marker on an
"(any board)" refresh; change the branch so pendingBoard is only deleted when a
real board was pushed/changed. In the block that uses pendingBoardPush, update
the else to only delete existing.pendingBoard when board is truthy (or when the
refresh represents an actual board change) — e.g., keep existing.pendingBoard
intact on empty/any-board refresh by checking board before deleting; references:
pendingBoardPush, existing.pendingBoard, existing.board, board.

In `@docs/install/index.html`:
- Around line 1108-1112: The fallback path calls openPortPicker() even when Web
Serial is not available; update the logic around pickedPort and openPortPicker()
to first check for navigator.serial (or an existing isWebSerialAvailable helper)
and if missing, show the unsupported-browser warning/message and return early
instead of calling openPortPicker(); ensure pickedPort remains falsy and no
requestPort() is attempted when navigator.serial is undefined so Monitor serial
port clicks on Safari/Firefox fail gracefully.

In `@docs/install/install-orchestrator.js`:
- Around line 615-618: The branch handling ipResult.action === "ip" incorrectly
treats malformed addresses as success: after calling
normalizeDeviceUrl(ipResult.url||"") it sets deviceUrl, viaHttp = true and
breaks even when normalizeDeviceUrl returns an empty string. Update the branch
so you first assign deviceUrl = normalizeDeviceUrl(...), then only set viaHttp =
true and break if deviceUrl is non-empty; if deviceUrl is empty, do not set
viaHttp or break (allow the needs-IP flow to remain open and let the user retry
or handle the error). Ensure you reference normalizeDeviceUrl, ipResult,
deviceUrl and viaHttp when making the change.

In `@docs/moonmodules/core/BoardModule.md`:
- Line 79: The paragraph uses future/roadmap phrasing ("future per-board state …
lands"); change it to present-tense, behavior-only wording that describes
current behavior and examples. Edit the sentence mentioning per-board state so
it states that per-board state is represented as additional leaves in the
controls map today (e.g., Ethernet RMII clock GPIO, MDIO/MDC pins, default
module-config overrides are examples of per-board leaves), and remove "future"
phrasing; keep references to controls, the two-level map shape, the fact that
every fan-out path (MoonDeck's _push_board_to_device, the web-installer
post-Improv push, and the device-side ?board= consumer) iterates the map
generically, and retain that catalog fields (name, firmwares, default_firmware)
remain top-level.

In `@scripts/moondeck.py`:
- Around line 188-191: The refresh currently can re-queue stale probed values
and overwrite a newer user selection because refresh_devices() and
_merge_refresh() treat the probed value as authoritative; change the logic to
persist a per-device pending desired board when a /api/push-board attempt fails
and make refresh_devices()/_merge_refresh() prefer MoonDeck's stored board (or
the persisted pending desired board) over the probed value until a push
succeeds. Concretely: when the push-board call (where /api/push-board is
invoked) fails, record the attempted board into a new pending_desired_board map
keyed by device id; update _merge_refresh() to check pending_desired_board first
and skip replacing it with the probed value, and clear pending_desired_board for
that device only when a subsequent push to /api/push-board succeeds; also ensure
refresh_devices() will attempt to re-push any pending_desired_board entries on
the next cycle so retries occur instead of reverting to stale values.
- Around line 197-198: The handler currently returns True when the incoming
picker value is empty, which leaves the previous selection persisted; instead,
detect the falsy/empty "board" in the push handler and explicitly clear the
stored value by setting the model's Board.board to an empty value (e.g., "" or
None) and persist that change (call the existing save/update/persist method for
the Board record) before returning success so the device's persisted state is
actually cleared.

In `@src/core/BoardModule.h`:
- Around line 54-57: Update the comment on the ASCII-printable constraint (the
block starting "ASCII-printable (0x20–0x7E), no embedded NUL.") to remove the
incorrect claim that this makes raw JSON embedding safe; instead state that the
intent is to reject control bytes and embedded NULs to protect downstream
consumers (JSON serializers, device UI, etc.), but explicitly note that
printable ASCII still contains characters like double-quote and backslash that
require normal JSON escaping. Keep the focus on rejecting control bytes/embedded
NULs rather than "no escaping required."

In `@src/core/NetworkModule.h`:
- Around line 291-292: The code currently skips reapplying TX power because
appliedTxPowerSetting_ is never invalidated when the radio stops; update the
radio stop/restart handling (where STA/AP stop and restart occur) to
reset/invalidate appliedTxPowerSetting_ (e.g., set to an impossible/ sentinel
value) so syncTxPower() will not short-circuit on the equality check with
txPowerSetting_ and will reapply persisted caps; also ensure any equality gating
logic in syncTxPower() (the block that tests txPowerSetting_ ==
appliedTxPowerSetting_) is reached after restart by calling syncTxPower() during
radio start and by clearing appliedTxPowerSetting_ during radio stop in the same
places referenced around the existing checks (the STA/AP stop handlers and the
restart path).

In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 430-466: The current loop uses a single ImprovFrameParser instance
(ImprovFrameParser parser) for both UART and USB-Serial-JTAG, letting bytes from
one transport finish frames started on the other; create and use separate parser
instances (e.g., ImprovFrameParser parser_uart and ImprovFrameParser
parser_jtag) and feed each transport's bytes only to its corresponding parser in
the loops where uartReady and g_jtagReady are handled (replace calls to
improvFeedByte(parser, b) with improvFeedByte(parser_uart, b) for UART and
improvFeedByte(parser_jtag, b) for JTAG), ensuring each parser retains its own
payload buffer and lifetime.

In `@src/platform/esp32/platform_esp32.cpp`:
- Line 582: The wifiSetTxPower stub currently returns false for all inputs, but
per the new contract wifiSetTxPower(0) must be a successful no-op; update the
wifiSetTxPower(int8_t /*quarterDbm*/) implementation to return true when the
argument is 0 (indicating “no override”) and return false for any non-zero value
so Ethernet-only builds treat 0 as success while still indicating failure for
actual power-setting attempts.

In `@src/ui/app.js`:
- Around line 174-191: sendControl currently swallows errors (logs warnings)
which allows consumePendingBoardParam to continue and consume the board handoff
even when some POSTs fail; change sendControl to accept an options param (e.g.,
{throwOnError}) and, when true, throw on non-ok responses or network errors
instead of only console.warn, and update callers (notably
consumePendingBoardParam) to await sendControl(moduleName, controlName, value,
{throwOnError: true}) so the board-injection path can stop and surface the first
failed write.

---

Outside diff comments:
In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 259-267: The code accepts frames where payload[1] (data_len)
disagrees with payload[2] (strLen) or where extra trailing bytes exist; update
the validation in platform_esp32_improv.cpp so that after reading uint8_t strLen
= payload[2] you (1) require strLen > 0 && strLen < g_improv.boardOutLen, (2)
require payload[1] == static_cast<uint8_t>(1u + strLen) to ensure data_len
matches the strLen+the str_len byte, and (3) require len ==
static_cast<size_t>(3u + strLen) so the frame length exactly matches the
expected size; on any mismatch call
improvSendError(static_cast<improv::Error>(IMPROV_ERROR_INVALID_BOARD)) and
return.

🪄 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: 085a2f4b-c112-41da-b1ae-0db106ba7ce1

📥 Commits

Reviewing files that changed from the base of the PR and between 9f888af and 751f1d5.

📒 Files selected for processing (24)

• docs/history/decisions.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/devices.js
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/NetworkModule.md
• scripts/MoonDeck.md
• scripts/moondeck.py
• src/core/BoardModule.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/NetworkModule.h
• src/core/SystemModule.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/install-picker.js
• test/scenario_runner.cpp
• test/unit/core/unit_BoardModule.cpp

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

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

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

Preserve transport identity through dispatch and replies.

These per-link parsers stop RX frame mixing, but they still feed a transport-agnostic improvDispatchFrame(), and improvSend() mirrors every response/error to both links. A valid RPC on UART therefore still injects unsolicited frames onto USB-Serial-JTAG (and vice versa), so concurrent clients can desynchronize even after this fix. Pass the source transport through improvFeedByte()/improvDispatchFrame() and reply only on the originating interface.

🤖 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_improv.cpp` around lines 438 - 472, The
parser instances (parser_uart, parser_jtag) feed bytes into improvFeedByte()
which currently dispatches via a transport-agnostic improvDispatchFrame() and
improvSend(), causing replies to be broadcast to both links; change the flow to
carry the source transport through the parser -> feed -> dispatch path and
restrict replies to that transport. Concretely: extend improvFeedByte() and
improvDispatchFrame() to accept a transport identifier (e.g., an enum or pointer
identifying UART vs JTAG), have parser_uart pass UART and parser_jtag pass JTAG
when calling improvFeedByte(), and modify improvSend()/reply logic to send only
on the provided transport rather than both links; update any call sites of
improvDispatchFrame()/improvSend() accordingly and ensure existing parser state
structs (ImprovFrameParser) carry or relay the transport id as needed.

♻️ Duplicate comments (1)

src/core/NetworkModule.h (1)

59-69: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply the TX-power cap in the same call that starts Wi‑Fi.

noteRadioStopped() reopens the equality gate, but the first re-apply still waits for loop1s(). In the Improv path, setWifiCredentials() can start STA after this tick’s syncTxPower() already ran, so the radio spends its first association/AP-start window at full power. On the affected LOLIN boards, that is the exact window where the brown-out happens.

syncTxPower() already tolerates an early false and retries later, so it is safe to call it immediately after every successful wifiStaInit() / wifiApInit() once state_ is set to WaitingSta or AP.

Suggested fix

         if (platform::wifiStaInit(ssid_, password_)) {
             state_ = State::WaitingSta;
             stateChangeTime_ = platform::millis();
+            syncTxPower();
             std::snprintf(statusBuf_, sizeof(statusBuf_), "WiFi STA: %s", ssid_);
             setStatus(statusBuf_, Severity::Status);
@@
             if (ssid_[0] != 0 && platform::wifiStaInit(ssid_, password_)) {
                 state_ = State::WaitingSta;
+                syncTxPower();
                 std::printf("NetworkModule: WiFi STA init started, SSID: %s\n", ssid_);
             } else {
                 startAP();
@@
                         if (ssid_[0] != 0 && platform::wifiStaInit(ssid_, password_)) {
                             state_ = State::WaitingSta;
                             stateChangeTime_ = now;
+                            syncTxPower();
                         } else {
                             startAP();
                         }
@@
                         if (ssid_[0] != 0 && platform::wifiStaInit(ssid_, password_)) {
                             state_ = State::WaitingSta;
                             stateChangeTime_ = now;
+                            syncTxPower();
                         } else {
                             startAP();
                         }
@@
         if (platform::wifiApInit(apName, "4.3.2.1")) {
             state_ = State::AP;
             stateChangeTime_ = platform::millis();
+            syncTxPower();
             apShutdownPending_ = true;
             std::snprintf(statusBuf_, sizeof(statusBuf_), "AP: %s @ 4.3.2.1", apName); setStatus(statusBuf_, Severity::Status);
             std::printf("NetworkModule: AP started: %s\n", apName);

Also applies to: 90-94, 186-190, 218-222, 387-392, 480-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/core/NetworkModule.h` around lines 59 - 69, After successfully starting
Wi‑Fi (e.g. after a true return from platform::wifiStaInit(...) or
platform::wifiApInit(...)) and after setting state_ to State::WaitingSta or
State::AP, immediately call syncTxPower() so TX-power caps are applied before
the first association/AP window; update the wifiStaInit and wifiApInit success
branches (the blocks that set state_ = State::WaitingSta or State::AP and call
rebuildControls()/setStatus(...)) to invoke syncTxPower() right there, and
mirror the same change wherever noteRadioStopped() or setWifiCredentials() can
cause an immediate wifi init so the radio never spends its initial connect/start
period at full power.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/install/install-orchestrator.js`:
- Around line 615-630: When normalizeDeviceUrl(ipResult.url || "") returns an
empty/unparseable result, instead of only writing to onLog and looping, surface
a visible validation error in the host UI: call a visible-error callback (e.g.,
add and invoke an onValidationError/onUserError callback) with a clear message
like "Malformed device address" and keep the existing onLog call; leave
deviceUrl, viaHttp and the continue logic unchanged. Update callers to accept
and render that onValidationError message in the modal so the user sees the
validation feedback.

In `@src/core/BoardModule.h`:
- Around line 52-65: The HTTP /api/control write path currently bypasses
BoardModule::setBoard() and its board-name validation, so unify validation by
either (A) extracting the validation logic from setBoard() into a single
callable function (e.g. BoardModule::validateBoardName(const std::string&)) and
calling that from both the HTTP control handler and setBoard(), or (B) changing
the HTTP control handler to route board-name updates through
BoardModule::setBoard() directly; ensure the chosen approach still copies the
accepted value into boardKey_ and triggers FilesystemModule's debounced save
(same behavior as NetworkModule::setWifiCredentials).

---

Outside diff comments:
In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 438-472: The parser instances (parser_uart, parser_jtag) feed
bytes into improvFeedByte() which currently dispatches via a transport-agnostic
improvDispatchFrame() and improvSend(), causing replies to be broadcast to both
links; change the flow to carry the source transport through the parser -> feed
-> dispatch path and restrict replies to that transport. Concretely: extend
improvFeedByte() and improvDispatchFrame() to accept a transport identifier
(e.g., an enum or pointer identifying UART vs JTAG), have parser_uart pass UART
and parser_jtag pass JTAG when calling improvFeedByte(), and modify
improvSend()/reply logic to send only on the provided transport rather than both
links; update any call sites of improvDispatchFrame()/improvSend() accordingly
and ensure existing parser state structs (ImprovFrameParser) carry or relay the
transport id as needed.

---

Duplicate comments:
In `@src/core/NetworkModule.h`:
- Around line 59-69: After successfully starting Wi‑Fi (e.g. after a true return
from platform::wifiStaInit(...) or platform::wifiApInit(...)) and after setting
state_ to State::WaitingSta or State::AP, immediately call syncTxPower() so
TX-power caps are applied before the first association/AP window; update the
wifiStaInit and wifiApInit success branches (the blocks that set state_ =
State::WaitingSta or State::AP and call rebuildControls()/setStatus(...)) to
invoke syncTxPower() right there, and mirror the same change wherever
noteRadioStopped() or setWifiCredentials() can cause an immediate wifi init so
the radio never spends its initial connect/start period at full power.

🪄 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: 2982bbe0-69ee-4615-ae81-09ab19770414

📥 Commits

Reviewing files that changed from the base of the PR and between 751f1d5 and eca3c2c.

📒 Files selected for processing (8)

• docs/install/devices.js
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/BoardModule.md
• src/core/BoardModule.h
• src/core/NetworkModule.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_improv.cpp