Device identity (deviceName + deviceModel), mDNS discovery, broadcast send + installer UX#22
Merged
Conversation
Makes deviceName the single source of truth for every network name (mDNS / SoftAP / DHCP all derive from it, sanitised at the source and following a live rename), and gives the web installer a clickable <deviceName>.local success link. Also fixes a UI flicker where typing in a number field got reverted by a state push, adds at-a-glance device-age dots, splits the board-only installer code out of the firmware, and shows the installer's fields immediately on a slow connection.
KPI: 16384lights | PC:365KB | tick:110/87/111/9/1/310/36/15/18/110/11us(FPS:9090/11494/9009/111111/1000000/3225/27777/66666/55555/9090/90909) | ESP32:1210KB | src:97(19434) | test:67(9905) | lizard:76w
Core:
- SystemModule: deviceName is the sole-owned single network identity; sanitise it to a valid RFC-1123 hostname (mm::sanitizeHostname) in setup() + loop1s() so any typed/persisted value ("My Room!" -> "My-Room") is safe for mDNS/AP/DHCP, with a MAC-MM-XXXX fallback when empty.
- NetworkModule: deleted the misnamed hostName() + its duplicate fallback; all three network names (mDNS, AP SSID, DHCP hostname) now read SystemModule's deviceName through one null-guarded readDeviceName(). syncMdns() re-advertises when deviceName changes live (tracks lastMdnsName_), so a rename takes effect within a tick with no reboot.
- Control.h: added sanitizeHostname() next to formatDottedQuad — RFC-1123 coercion (keep [A-Za-z0-9-], collapse invalid runs to one '-', trim leading/trailing).
- main.cpp: the per-second tick line now also emits MM_DEVICE=<deviceName>.local next to MM_IP, read from SystemModule (the owner), so the installer can offer a .local link the REST path can't (mixed-content-blocked).
Light domain:
- RotateModifier: documented the per-step LUT-rebuild rate (speed/64 per second, ~4/s at speed 255) at its introduction site, distinct from RandomMapModifier's bpm-capped <=1/s.
UI:
- app.js: stamp dragTs on the number-input handlers (uint8/uint16/int16) so a WS state push can't revert a value mid-type — fixes the 16->8->16->8 flicker. Hoisted the per-control userActive guard in updateModuleControls into one EDITABLE_CONTROL_TYPES early-continue (folds ipv4 onto the same rule) and added the missing 'pin' case so a pin value live-updates. Added generic device-age dots (green <1min / orange <1h / red beyond) driven by any *Sec duration field — no per-module knowledge in the list renderer.
- install-picker.js: show the full field skeleton (each select spins) immediately on mount instead of a lone "Loading…", so a slow GitHub fetch doesn't leave the form looking empty under the Port row. Board-catalog + chip-detection moved out (see below) and injected via init({ boardSupport }) — dependency injection, so the firmware-embedded picker carries no board code.
- install-picker-boards.js (new): the board catalog / chip-detection half of the picker, WEB INSTALLER ONLY — imported by the Pages page, never by the embedded picker, so it doesn't ship in the firmware.
- index.html: success popup prefers the clickable http://<deviceName>.local link (survives a DHCP IP change) over the bare IP; imports install-picker-boards.js and passes it as boardSupport; .rp-spinner keyframe for the skeleton.
- style.css: .age-dot + age-fresh/recent/stale buckets and the skeleton spinner (reuses --green/--yellow/--red).
Scripts / MoonDeck:
- preview_installer.py: stage install-picker-boards.js alongside install-picker.js for the local preview.
Tests:
- unit_sanitizeHostname (new): pins the hostname rule (spaces/punctuation -> '-', collapse, trim leading+trailing, idempotent, empty stays empty). Caught a leading-literal-dash bug in the first sanitiser pass.
Docs / CI:
- architecture.md: new "Device name: one identity, every network name derives from it" section (sole owner SystemModule, always-valid hostname, follows a live rename) + TOC entry.
- CLAUDE.md: new "Continuous refactor, no hacks" principle — improvement happens on sight, the fix is the recognisable standard one; references Common-patterns-first / Industry-standards / Minimalism rather than restating them.
- release.yml: copy install-picker-boards.js to the Pages site next to install-picker.js.
Reviews:
- 🐇 RotateModifier rebuild-rate not stated at its site -> documented (fixed).
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/app.js (1)
1421-1452:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip
ageSecwhen the detail row is cached.The comment says
cachedreplaces the fake relative “last seen” time, but the loop still renders and now tints a siblingageSec, producing conflictinglast seenrows for restored devices.🐛 Proposed fix
function fillListDetail(panel, detail) { panel.replaceChildren(); if (!detail || typeof detail !== "object") return; + const isCached = detail.cached === true; for (const [k, v] of Object.entries(detail)) { + if (isCached && k === "ageSec") continue; const isScalarArray = Array.isArray(v) && v.every(e => typeof e !== "object"); if (typeof v === "object" && !isScalarArray) continue;🤖 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 1421 - 1452, The fillListDetail function currently renders both a "cached" row and an "ageSec" row as "last seen" when a device is cached, creating conflicting detail rows. Add a condition at the beginning of the for loop that iterates through Object.entries(detail) to skip the ageSec field when the detail object has a truthy cached property. This ensures that only the "cached" label is shown as the "last seen" value for restored devices, not both conflicting rows.
🤖 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 241-247: The code in the return statement within the if (m) block
returns prematurely when MM_IP is found, without waiting for MM_DEVICE which may
arrive in a subsequent serial read due to arbitrary chunking. Instead of
returning immediately after finding MM_IP, refactor to collect data from
multiple serial chunks by continuing to read until both MM_IP and MM_DEVICE are
present in the buffer, or until a reasonable timeout is reached. Only then
should the function return with both the ip and mdns values populated
appropriately.
In `@src/core/NetworkModule.h`:
- Line 111: The call to platform::setHostname(readDeviceName()) in NetworkModule
happens only once during setup, but does not get re-invoked when deviceName
mutates in SystemModule::loop1s(). To keep DHCP hostname synchronized with
runtime deviceName changes, you must either trigger a network reconnect after a
deviceName rename (to allow the next DHCP renewal to use the new hostname) or
add proper synchronization (mutex/atomic) and call platform::setHostname() again
from SystemModule::loop1s() when deviceName changes, ensuring compliance with
the threading contract documented in platform_esp32.cpp.
In `@test/CMakeLists.txt`:
- Line 26: In addition to the helper test unit_sanitizeHostname.cpp registered
on line 26 of test/CMakeLists.txt, add module-level tests that verify the new
core behavior added to SystemModule::setup()/loop1s() and
NetworkModule::readDeviceName()/syncMdns(). These module tests should cover the
sanitized fallback behavior and live rename propagation across these integration
paths. Register these new module tests in the CMakeLists.txt file following the
same pattern as the existing helper test to satisfy the coding guideline
requiring module tests for all new core logic additions.
---
Outside diff comments:
In `@src/ui/app.js`:
- Around line 1421-1452: The fillListDetail function currently renders both a
"cached" row and an "ageSec" row as "last seen" when a device is cached,
creating conflicting detail rows. Add a condition at the beginning of the for
loop that iterates through Object.entries(detail) to skip the ageSec field when
the detail object has a truthy cached property. This ensures that only the
"cached" label is shown as the "last seen" value for restored devices, not both
conflicting rows.
🪄 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: 97f11b02-cff8-402c-9c7e-68f8f0e84832
📒 Files selected for processing (18)
.github/workflows/release.ymlCLAUDE.mddocs/architecture.mddocs/install/index.htmldocs/install/install-orchestrator.jsscripts/run/preview_installer.pysrc/core/Control.hsrc/core/NetworkModule.hsrc/core/SystemModule.hsrc/light/modifiers/RotateModifier.hsrc/main.cppsrc/ui/app.jssrc/ui/install-picker-boards.jssrc/ui/install-picker.jssrc/ui/style.csstest/CMakeLists.txttest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_sanitizeHostname.cpp
| // netif the platform creates (eth, the wifi cascade, a later reconnect) reads | ||
| // it. Same name as mDNS/SoftAP: deviceName, default MM-XXXX. | ||
| platform::setHostname(hostName()); | ||
| platform::setHostname(readDeviceName()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== setHostname call sites =="
rg -n -C3 '\bsetHostname\s*\(' src
echo
echo "== platform setHostname implementations =="
fd -i 'platform*.{h,hpp,cpp,cc}' src/platform | xargs -r rg -n -C3 '\bsetHostname\s*\(|hostname'
echo
echo "== runtime rename paths in System/Network modules =="
rg -n -C3 'readDeviceName|deviceName|setHostname|syncMdns|loop1s' src/core/SystemModule.h src/core/NetworkModule.hRepository: MoonModules/projectMM
Length of output: 4080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Platform setHostname implementations (corrected) =="
find src/platform -type f \( -name "*.h" -o -name "*.hpp" -o -name "*.cpp" -o -name "*.cc" \) -exec rg -n -C5 'void setHostname|applyHostname|hostname_\[' {} +
echo
echo "== deviceName mutations and sync paths =="
rg -n -C3 'deviceName|readDeviceName|setHostname' src/core/SystemModule.h src/core/NetworkModule.h
echo
echo "== mDNS and SoftAP rename handling =="
rg -n -C3 'syncMdns|loop1s|mdns|AP_SSID' src/core/NetworkModule.h src/core/SystemModule.hRepository: MoonModules/projectMM
Length of output: 33125
DHCP hostname will not follow runtime deviceName renames until next reconnect.
Line 111 calls platform::setHostname(readDeviceName()) once during setup. The platform contract (documented in src/platform/platform.h:178–182) explicitly states: "A later rename takes effect on the next DHCP renewal/reconnect." However, after deviceName mutates in SystemModule::loop1s(), setHostname() is never re-invoked. This leaves DHCP hostname stale while mDNS (synced via syncMdns() each loop1s) and SoftAP (using current name at startup) follow the new value.
The threading contract in src/platform/esp32/platform_esp32.cpp:300–306 forbids calling setHostname() after interface bring-up without synchronization (mutex/atomic). To keep DHCP hostname in sync with live renames would require either (a) triggering a reconnect after rename, or (b) adding synchronization to allow post-bring-up setHostname() calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/NetworkModule.h` at line 111, The call to
platform::setHostname(readDeviceName()) in NetworkModule happens only once
during setup, but does not get re-invoked when deviceName mutates in
SystemModule::loop1s(). To keep DHCP hostname synchronized with runtime
deviceName changes, you must either trigger a network reconnect after a
deviceName rename (to allow the next DHCP renewal to use the new hostname) or
add proper synchronization (mutex/atomic) and call platform::setHostname() again
from SystemModule::loop1s() when deviceName changes, ensuring compliance with
the threading contract documented in platform_esp32.cpp.
| unit/core/unit_NetworkModule.cpp | ||
| unit/core/unit_NetworkModule_ethernet.cpp | ||
| unit/core/unit_Sort.cpp | ||
| unit/core/unit_sanitizeHostname.cpp |
There was a problem hiding this comment.
Add module tests for the new live-name integration paths.
Line 26 only registers the helper test. This cohort also adds core behavior in SystemModule::setup()/loop1s() and NetworkModule::readDeviceName()/syncMdns(), so please add module-level tests for sanitized fallback + live rename propagation, not just the sanitizer helper.
As per coding guidelines, src/core/**/*.{cpp,h}: Every new core logic addition must have a corresponding module test in test/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/CMakeLists.txt` at line 26, In addition to the helper test
unit_sanitizeHostname.cpp registered on line 26 of test/CMakeLists.txt, add
module-level tests that verify the new core behavior added to
SystemModule::setup()/loop1s() and NetworkModule::readDeviceName()/syncMdns().
These module tests should cover the sanitized fallback behavior and live rename
propagation across these integration paths. Register these new module tests in
the CMakeLists.txt file following the same pattern as the existing helper test
to satisfy the coding guideline requiring module tests for all new core logic
additions.
Source: Coding guidelines
Makes mDNS-discovered projectMM peers show correctly in the device list (right name, right type, fresh green dot) without waiting for the HTTP scan, reframes the hardware-provenance docs as MCU→deviceModel (board is now the bare-PCB term only), and re-applies the PR #22 review fixes. Verified live on the S3 and P4: each sees the other named, typed projectMM, and green purely over mDNS. KPI: 16384lights | PC:365KB | tick:111/93/113/9/1/313/36/15/18/112/11us(FPS:9009/10752/8849/111111/1000000/3194/27777/66666/55555/8928/90909) | src:97(19491) | test:67(9956) | lizard:77w Core: - DevicesModule: an mDNS hit carrying the `mm=1` TXT marker classifies the peer as projectMM straight from the browse (no HTTP probe); a genuine mDNS name (the peer's deviceName, via instance_name) now overwrites a dotted-quad IP placeholder so the row shows the real name. New isIpPlaceholder() helper guards that overwrite so a real HTTP-probe name still wins. - NetworkModule: documented the DHCP-hostname live-rename boundary at the setHostname call site (single-writer-before-readers contract + DHCP only changes on lease renewal; mDNS follows a rename live, DHCP on next renewal). Light domain: (none) UI: - app.js: the freshness dot now reads the list row's DETAIL object (where `ageSec`/`cached` live), not the summary — so an mDNS-fresh peer actually colours green; previously the dot logic looked at the summary, which carries only name/ip/type, and never found a duration. Also skip an `ageSec` detail row when `cached` is present (no double "last seen"). Core / platform: - platform_esp32: advertise a `mm=1` TXT record on the `_http._tcp` service (lets a peer tell projectMM apart from a generic web box without probing); the mDNS browse reads that TXT into MdnsHost.isProjectMM and prefers the service instance_name (the advertised deviceName) over the host record for the display name. - platform.h: MdnsHost gains isProjectMM. Scripts / MoonDeck: (none) Tests: - unit_SystemModule: deviceName sanitisation through the live path — a typed "My Living Room!" coerces to "My-Living-Room" on the next loop1s, an all-invalid name falls back to the MAC name, a valid name is left unchanged. Docs / CI: - architecture.md: reframed "Firmware vs board" as "Firmware vs deviceModel vs board" — firmware = the binary, deviceModel = the assembled product (the catalog entry; still named `board` in code, with one clearly-marked temporary note bridging the gap), board = the bare PCB only (on-board LED/peripherals). Collapsed the config-provenance model from MCU→Board→Device to a two-level MCU→deviceModel (the devkit-vs-product split was documentation only; the data carries the "default only where fixed" rule). Sharpened the Ethernet provenance: driver = firmware, pin map = firmware-seeded but deviceModel-authoritative. - README.md: physical-thing prose "board" → "device" (the umbrella noun a user holds). - building.md / testing.md / MoonDeck.md / backlog.md: updated the inbound anchor links to the renamed architecture.md section. - install-orchestrator.js: readBootIp returns once BOTH MM_IP and MM_DEVICE are seen, with a ~1.5s grace after the IP so older firmware without the device token still returns the IP promptly (the tokens chunk separately on the wire). Reviews: - 🐇 readBootIp returned on MM_IP before MM_DEVICE could arrive in a later serial chunk → wait for both with a grace window (fixed) - 🐇 fillListDetail could render two "last seen" rows (cached + ageSec) → skip ageSec when cached (fixed) - 🐇 new SystemModule sanitise logic lacked module tests → added three (fixed) - 👾 architecture.md stated the deviceModel is stored "on SystemModule" — wrong for the current code (BoardModule still owns it) and self-contradictory with the code-wired-children section → restored the BoardModule attribution (fixed) - 👾 backlog.md "Runtime board presets" still uses board in the product sense → deferred to the board→deviceModel rename step (backlog is forward-looking; noted, not fixed) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture.md`:
- Line 233: The documentation contains inconsistent references to the device
model catalog file. At lines 233 and 250, the prose text references
"deviceModels.json" while the link target points to "install/boards.json".
Update both occurrences to ensure the link label text and the actual link target
are consistent with each other, choosing one filename and using it uniformly
throughout both the display text and the href target.
- Line 229: The paragraph starting at line 229 in the architecture documentation
uses forward-looking roadmap language ("Temporary", "remove at", "this paragraph
is deleted") which violates the present-tense documentation standard for
docs/architecture.md. Rewrite this paragraph to describe the current state using
present tense only, removing all references to future changes and temporary
notes. Simply state that the code currently uses the term board while the
documentation uses deviceModel, without mentioning planned renames or when the
note will be removed.
In `@src/core/DevicesModule.h`:
- Around line 407-413: The isProjectMM promotion in the DevType assignment is
too broad and will incorrectly mislabel non-_http services that carry the mm=1
marker. Modify the ternary expression to gate the ProjectMM promotion on both
conditions: check that host.isProjectMM is true AND that the base type from
kMdnsServices[mdnsIndex_].type is DevType::Generic, which represents the _http
service. Only when both conditions are met should the type be promoted to
DevType::ProjectMM, otherwise use the base type from
kMdnsServices[mdnsIndex_].type.
In `@src/ui/app.js`:
- Around line 1430-1434: The condition that skips rendering the ageSec row
checks for truthiness of detail.cached, but the later cached branch renders
whenever the key exists, regardless of its value. Change the skip condition on
the line checking for ageSec to match how the cached branch actually renders by
checking for the existence of the cached key rather than its truthiness, so that
both ageSec and cached branches remain mutually exclusive regardless of whether
cached is true or false.
In `@test/unit/core/unit_SystemModule.cpp`:
- Around line 53-62: The writeDeviceName() helper function silently returns
without signaling failure when the "deviceName" control is not found during
iteration, which allows tests to mask setup regressions. After the for loop
completes in writeDeviceName(), add a REQUIRE() statement or assertion that
explicitly fails if the "deviceName" control was never located, ensuring the
test fails fast instead of silently proceeding with default values.
🪄 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: a0293989-f7e5-4639-8044-04b69f7b6090
📒 Files selected for processing (13)
README.mddocs/architecture.mddocs/backlog/backlog.mddocs/building.mddocs/install/install-orchestrator.jsdocs/testing.mdscripts/MoonDeck.mdsrc/core/DevicesModule.hsrc/core/NetworkModule.hsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jstest/unit/core/unit_SystemModule.cpp
Removes the single-control BoardModule and moves the physical-hardware identity onto SystemModule as a `deviceModel` control, then renames the whole "board" hardware-profile concept to `deviceModel` across code, the device catalog, the wire/URL contracts, and the docs. A device now has two clear identity facets: `deviceName` (which unit) and `deviceModel` (which product). "board" survives only in its literal bare-PCB sense (on-board LED, board-soldered pins). Verified live on the S3 and P4: both expose a `deviceModel` control and discover each other over mDNS. KPI: 16384lights | PC:365KB | tick:112/88/112/10/2/343/36/15/18/112/11us(FPS:8928/11363/8928/100000/500000/2915/27777/66666/55555/8928/90909) | ESP32:1211KB | src:96(19434) | test:66(9830) | lizard:77w Core: - SystemModule: gained the `deviceModel` Text control + a validating `setDeviceModel()` (1..31 chars, printable-ASCII, NUL-safe) moved verbatim from the deleted BoardModule. SystemModule now owns the device identity (deviceName + deviceModel) directly. - BoardModule deleted (header, spec, unit test) — it was a single control + setter; folding it is net subtraction. - ImprovProvisioningModule: routes the vendor RPC to `systemModule_->setDeviceModel()`; dropped `setBoardModule`/`boardModule_`; buffers renamed `pendingDeviceModel_`. - main.cpp: removed BoardModule create/register/wire. - DevicesModule: the mDNS `isProjectMM` promotion is now gated on the base service type being Generic (`_http`), so a definite service (`_wled`) can't be relabelled. UI: - app.js: top-level (depth 0) module cards no longer collapse their controls — System accepts peripherals so it was hiding its own settings behind a disclosure. `?deviceModel=` inject param + `consumePendingDeviceModelParam`. cached/ageSec "last seen" skip now gates on `"cached" in detail` (key existence), matching the cached render branch. - install-picker / index.html: picker label, search placeholder, dialog title → "Device" vocabulary (element IDs unchanged). Core / platform: - platform.h / platform_esp32 / platform_desktop: the Improv vendor RPC `SET_BOARD` → `SET_DEVICE_MODEL` (symbol-only; the wire byte 0xFE is unchanged), `boardOut*` → `deviceModelOut*`, `IMPROV_ERROR_INVALID_BOARD` → `..._DEVICE_MODEL`. Scripts / MoonDeck: - MoonDeck: device-facing reads/pushes use `deviceModel` (reads it from the System module's /api/state, custom-push fallback emits a System/deviceModel block, profile-capture uses SystemModule). MoonDeck's own internal `board` record key is kept (self-contained, not a device contract). UI label "Board:" → "Device:". - check_boards.py → check_devices.py (validates `System.deviceModel == entry name`, BOOT_WIRED_TYPES drops Board). preview_installer / release.yml / generate_firmwares / improv_provision updated for the renamed catalog file. Tests: - unit_SystemModule: deviceName sanitisation tests now use a `writeDeviceName` helper that REQUIREs the control exists (fails loud, not silent). Docs / CI: - docs/install/boards.json → deviceModels.json (catalog's 22 `type:Board` blocks → top-level `type:System` with a `deviceModel` control). - architecture.md: the deviceModel naming is now the live state (the temporary board↔deviceModel bridge note removed); links retarget deviceModels.json. SystemModule.md documents the `deviceModel` control. BoardModule.md deleted; inbound links repointed. README / building / MoonDeck / driver specs swept (board→deviceModel where it meant the product; bare-PCB "board" kept). Reviews: - 👾 devices.js wrote `?board=` while the device reads `?deviceModel=` — silent break of the installer Inject handoff → fixed to `?deviceModel=` (MUST-FIX). - 👾 DevicesModule isProjectMM promotion too broad → gated on base type Generic (fixed). - 👾 stale comments (HttpServerModule `?board=`, orchestrator/app.js/moondeck `Board.board`, devices.js fn-name) → updated. - 🐇 app.js cached/ageSec skip used truthiness not key existence → `"cached" in detail` (fixed). - 🐇 writeDeviceName test helper silently no-opped if control missing → REQUIRE (fixed). - 🐇 architecture.md link-target / temporary-note findings → already resolved by this rename (links target deviceModels.json, note removed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ewowi
changed the title
There was a problem hiding this comment.
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 (5)
docs/install/README.md (1)
75-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOutdated catalog schema: Board module entry no longer exists.
The PR removes
BoardModuleentirely and migrates hardware identity toSystemModule.deviceModel, but the catalog schema example (lines 75–76) and explanation (lines 88–90) still reference a "Board" module entry. This is now incorrect and will mislead integrators about the catalog format.The example JSON shows:
{ "type": "Board", "id": "Board", "parent_id": "System", "controls": { "board": "projectMM testbench S3" } },And the explanation states: "Even
Boardis a module entry (it's a boot-wired child ofSystem, so the add is an idempotent no-op and only itsboardcontrol applies)."Since
BoardModuleis removed anddeviceModelis now a control onSystemModuleitself (injected via the ImprovSET_DEVICE_MODELRPC or HTTP/api/control), this example and explanation should be updated to either:
- Remove the Board entry from the example JSON, leaving only the driver/audio modules, or
- Replace the explanation to document how
deviceModelis now injected as part ofSystemModule(not as a separate Board module in the modules list).🤖 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 75 - 90, The catalog schema documentation in the README contains outdated references to the BoardModule which has been removed and replaced with deviceModel control on SystemModule. Remove the Board module entry from the example JSON (the object with "type": "Board", "id": "Board" and the board control) since it no longer exists in the catalog format. Then update the explanation text that describes how Board is a boot-wired child of System to instead document how deviceModel is now injected as a control on SystemModule itself, either through the Improv SET_DEVICE_MODEL RPC or via HTTP /api/control endpoints, making clear that deviceModel is no longer a separate module entry in the modules list.src/core/Control.h (1)
73-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrim all trailing hyphens after compaction.
Line 75 removes only one literal trailing
-, soDesk--sanitizes toDesk-, which still violates the hostname invariant used by mDNS/AP/DHCP names.🐛 Proposed fix
- if (w != buf && w[-1] == '-') --w; + while (w != buf && w[-1] == '-') --w;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/Control.h` around lines 73 - 76, The current code in the trim section only removes a single trailing hyphen using a single conditional check, which leaves multiple consecutive trailing hyphens untouched. Replace the single if-statement that checks and removes one hyphen character with a while-loop that continues decrementing the write pointer w as long as w points to valid buffer data and the preceding character is a hyphen, ensuring all trailing hyphens are removed before null-terminating the string with *w = '\0'.src/platform/desktop/platform_desktop.cpp (1)
562-568:⚠️ Potential issue | 🟠 MajorDesktop
improvProvisioningInitstub is missing two parameters required by the platform API contract.The platform.h declaration has 11 parameters ending with
uint8_t* txPowerOutandstd::atomic<bool>* txPowerReady, but the desktop stub only defines 9 parameters, stopping atdeviceModelReady. The ESP32 implementation and MM_NO_WIFI stub both have the full 11-parameter signature. This mismatch violates the platform API contract:src/platform/**implementations must match the API declared inplatform.h, and the desktop stub defines a different overload rather than the same function signature.Add the missing parameters to the desktop stub:
bool improvProvisioningInit(const ImprovDeviceInfo& /*info*/, char* /*ssidOut*/, size_t /*ssidOutLen*/, char* /*passwordOut*/, size_t /*passwordOutLen*/, std::atomic<bool>* /*ready*/, char* statusBuf, size_t statusBufLen, char* /*deviceModelOut*/, size_t /*deviceModelOutLen*/, - std::atomic<bool>* /*deviceModelReady*/) { + std::atomic<bool>* /*deviceModelReady*/, + uint8_t* /*txPowerOut*/, + std::atomic<bool>* /*txPowerReady*/) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/desktop/platform_desktop.cpp` around lines 562 - 568, The improvProvisioningInit function in the desktop platform stub is missing two parameters that are required by the platform API contract defined in platform.h. The function currently has 9 parameters ending with deviceModelReady, but it should have 11 parameters total. Add the two missing parameters uint8_t* txPowerOut and std::atomic<bool>* txPowerReady after the deviceModelReady parameter in the improvProvisioningInit function signature in platform_desktop.cpp. This will align the desktop stub implementation with the API contract that the ESP32 and MM_NO_WIFI implementations already correctly follow.Source: Coding guidelines
docs/install/index.html (1)
1313-1329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the pending
deviceModelhandoff to the success link.When HTTP fan-out is skipped or fails,
pendingBoardPushis only attached to the saved device row. The immediate “Device is online!” link still opens the device without?deviceModel=..., so the first visit can bypass the fallback that applies the selected device-model controls.🔧 Proposed fix
- const linkUrl = mdns ? `http://${mdns}/` : url; - a.href = linkUrl; - a.textContent = linkUrl; // Pending-board flag: set whenever the in-orchestrator HTTP push @@ - const pendingBoardPush = !!(board && !httpBoardOk); + const pendingBoardPush = !!(board && !httpBoardOk); + const linkUrl = mdns ? `http://${mdns}/` : url; + const visitUrl = new URL(linkUrl); + if (pendingBoardPush) visitUrl.searchParams.set("deviceModel", board); + a.href = visitUrl.href; + a.textContent = visitUrl.href; myDevices.addProvisionedDevice(url, board, { pendingBoardPush });🤖 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 1313 - 1329, The linkUrl being set as the anchor href does not include the deviceModel query parameter when pendingBoardPush is true, which prevents the fallback device-model control application on first visit. When constructing the linkUrl (from either mdns or url), check if pendingBoardPush is set and conditionally append a ?deviceModel= query parameter to the URL before assigning it to the anchor href attribute. This ensures that the immediate success link includes the necessary device model information to properly apply controls even when HTTP fan-out has failed.docs/install/install-orchestrator.js (1)
1013-1074:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
mdnsafter successful WiFi provisioning.For a fresh device, the pre-provision
readBootIp()call cannot seeMM_DEVICEbecause the device has no IP yet. Afterprovision()succeeds,deviceMdnsstays empty, so the success callback falls back to the DHCP IP for the normal install path despite the newMM_DEVICE=<deviceName>.localserial token.🔧 Proposed direction
await improvClient.provision(ssid, password, 30000); deviceUrl = improvClient.nextUrl; if (!deviceUrl) { throw new Error("provision succeeded but no device URL returned"); } + // Improv holds the reader; close it before reading the periodic + // MM_IP/MM_DEVICE line emitted after WiFi comes up. + await improvClient.close(); + improvClient = null; + const boot = await readBootIp(port, 3000); + if (boot.mdns) deviceMdns = boot.mdns; // Push SET_DEVICE_MODEL vendor RPC if the user picked a board.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/install/install-orchestrator.js` around lines 1013 - 1074, After the provision() call succeeds and before the onSuccess callback is invoked, read the mDNS name from the device serial output. The device sends the MM_DEVICE token in its serial response after WiFi provisioning completes, but the current code only reads this during the initial readBootIp() call before the device has an IP. Extract the MM_DEVICE token from the provision response or post-provision serial data and assign it to the deviceMdns variable so it gets populated in the onSuccess callback with the actual device mDNS name (e.g., "deviceName.local") instead of remaining empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 115: The board-catalog gate description contains outdated identity key
references that no longer reflect the current code structure. In the validation
description that mentions "each `Board.board` control equals its entry `name`",
replace the reference to `Board.board` with `System.deviceModel` to accurately
reflect that the identity key was migrated from Board.board to
System.deviceModel.
In `@docs/install/install-orchestrator.js`:
- Around line 147-157: The encodeSetDeviceModelPayload function accepts any
UTF-8 sequence for the board parameter, but the ESP32 handler requires printable
ASCII bytes in the range 0x20 to 0x7E. Add validation after encoding the board
name with TextEncoder to check that every byte in nameBytes falls within the
printable ASCII range, and throw an error with a descriptive message if any byte
is outside this range. This validation should occur before the length check to
catch invalid characters early.
In `@scripts/check/check_devices.py`:
- Around line 167-168: The validation for System module's deviceModel identity
control is currently guarded by a condition that checks if mods is truthy, which
causes it to be skipped when modules is an empty list. Remove the mods check
from the if condition so that the error message about missing deviceModel
identity control is appended whenever board_control_seen is False, regardless of
whether the modules list is empty or not. Only the board_control_seen check
should determine whether the validation error is triggered.
In `@src/core/SystemModule.h`:
- Around line 98-99: The deviceModel text control added to controls_ bypasses
the setDeviceModel() validator when written through the /api/control endpoint,
allowing invalid values that violate the 1-31 printable-ASCII constraint. Add a
per-control validator or setter callback to the deviceModel control to enforce
the same validation rules applied in setDeviceModel() before allowing the value
to persist, or alternatively refactor deviceModel_ as a setter-only persisted
state that cannot be directly written through the generic controls interface.
In `@src/ui/app.js`:
- Around line 260-267: The code does not validate that entry.modules is an array
before using it in iteration, which can cause an unhandled rejection when
malformed payloads are received. After finding the entry in the catalog (where
entry is assigned from catalog.find), add a defensive check to ensure that
entry.modules is a valid array before passing it to any iteration logic. If
entry.modules exists but is not an array, normalize it to an empty array so that
subsequent for...of loops that consume entry.modules (particularly around line
291-305) will not throw errors when processing non-array values from the
malformed JSON payload.
---
Outside diff comments:
In `@docs/install/index.html`:
- Around line 1313-1329: The linkUrl being set as the anchor href does not
include the deviceModel query parameter when pendingBoardPush is true, which
prevents the fallback device-model control application on first visit. When
constructing the linkUrl (from either mdns or url), check if pendingBoardPush is
set and conditionally append a ?deviceModel= query parameter to the URL before
assigning it to the anchor href attribute. This ensures that the immediate
success link includes the necessary device model information to properly apply
controls even when HTTP fan-out has failed.
In `@docs/install/install-orchestrator.js`:
- Around line 1013-1074: After the provision() call succeeds and before the
onSuccess callback is invoked, read the mDNS name from the device serial output.
The device sends the MM_DEVICE token in its serial response after WiFi
provisioning completes, but the current code only reads this during the initial
readBootIp() call before the device has an IP. Extract the MM_DEVICE token from
the provision response or post-provision serial data and assign it to the
deviceMdns variable so it gets populated in the onSuccess callback with the
actual device mDNS name (e.g., "deviceName.local") instead of remaining empty.
In `@docs/install/README.md`:
- Around line 75-90: The catalog schema documentation in the README contains
outdated references to the BoardModule which has been removed and replaced with
deviceModel control on SystemModule. Remove the Board module entry from the
example JSON (the object with "type": "Board", "id": "Board" and the board
control) since it no longer exists in the catalog format. Then update the
explanation text that describes how Board is a boot-wired child of System to
instead document how deviceModel is now injected as a control on SystemModule
itself, either through the Improv SET_DEVICE_MODEL RPC or via HTTP /api/control
endpoints, making clear that deviceModel is no longer a separate module entry in
the modules list.
In `@src/core/Control.h`:
- Around line 73-76: The current code in the trim section only removes a single
trailing hyphen using a single conditional check, which leaves multiple
consecutive trailing hyphens untouched. Replace the single if-statement that
checks and removes one hyphen character with a while-loop that continues
decrementing the write pointer w as long as w points to valid buffer data and
the preceding character is a hyphen, ensuring all trailing hyphens are removed
before null-terminating the string with *w = '\0'.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 562-568: The improvProvisioningInit function in the desktop
platform stub is missing two parameters that are required by the platform API
contract defined in platform.h. The function currently has 9 parameters ending
with deviceModelReady, but it should have 11 parameters total. Add the two
missing parameters uint8_t* txPowerOut and std::atomic<bool>* txPowerReady after
the deviceModelReady parameter in the improvProvisioningInit function signature
in platform_desktop.cpp. This will align the desktop stub implementation with
the API contract that the ESP32 and MM_NO_WIFI implementations already correctly
follow.
🪄 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: 83b17bf5-11f5-4235-941a-19601d135ebf
⛔ Files ignored due to path filters (3)
scripts/build/build_esp32.pyis excluded by!**/build/**scripts/build/generate_firmwares.pyis excluded by!**/build/**scripts/build/improv_provision.pyis excluded by!**/build/**
📒 Files selected for processing (53)
.github/workflows/release.ymlCLAUDE.mddocs/architecture.mddocs/building.mddocs/install/README.mddocs/install/deviceModels.jsondocs/install/devices.jsdocs/install/firmwares.jsondocs/install/index.htmldocs/install/install-orchestrator.jsdocs/moonmodules/core/BoardModule.mddocs/moonmodules/core/ImprovProvisioningModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/SystemModule.mddocs/moonmodules/light/drivers/LcdLedDriver.mddocs/moonmodules/light/drivers/NetworkSendDriver.mddocs/moonmodules/light/drivers/ParlioLedDriver.mddocs/moonmodules/light/drivers/RmtLedDriver.mddocs/performance.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/MoonDeck.mdscripts/check/check_devices.pyscripts/check/check_firmwares.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/moondeck_ui/index.htmlscripts/run/preview_installer.pysrc/core/AudioModule.hsrc/core/BoardModule.hsrc/core/Control.hsrc/core/DevicesModule.hsrc/core/HttpServerModule.cppsrc/core/ImprovProvisioningModule.hsrc/core/JsonUtil.hsrc/core/NetworkModule.hsrc/core/SystemModule.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32.cppsrc/platform/esp32/platform_esp32_improv.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/install-picker-boards.jssrc/ui/install-picker.jstest/CMakeLists.txttest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_BoardModule.cpptest/unit/core/unit_NetworkModule_ethernet.cpptest/unit/core/unit_SystemModule.cpp
💤 Files with no reviewable changes (4)
- test/unit/core/unit_BoardModule.cpp
- docs/moonmodules/core/BoardModule.md
- src/core/BoardModule.h
- test/CMakeLists.txt
Comment on lines
+98
to
+99
| controls_.addText("deviceModel", deviceModel_, sizeof(deviceModel_)); | ||
| controls_.setReadOnly(controls_.count() - 1, true); |
There was a problem hiding this comment.
Route every deviceModel write through the validator.
Lines 239-247 document that /api/control bypasses setDeviceModel(), so the new deviceModel control can persist/render values that violate the 1..31 printable-ASCII contract. Please add a per-control validator/setter path before mutating deviceModel_, or expose this as setter-only persisted state instead of a generic writable Text control.
Also applies to: 239-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/core/SystemModule.h` around lines 98 - 99, The deviceModel text control
added to controls_ bypasses the setDeviceModel() validator when written through
the /api/control endpoint, allowing invalid values that violate the 1-31
printable-ASCII constraint. Add a per-control validator or setter callback to
the deviceModel control to enforce the same validation rules applied in
setDeviceModel() before allowing the value to persist, or alternatively refactor
deviceModel_ as a setter-only persisted state that cannot be directly written
through the generic controls interface.
…fixes
Adds a beginner-facing end-to-end install walkthrough, makes the testbench S3 first-flash come up as an 8x8 audio-reactive setup (via a new catalog "replace the boot defaults" mechanism), shows both the IP and the .local link on the installer success screen, and processes a CodeRabbit batch.
KPI: 16384lights | PC:365KB | tick:111/89/268/11/2/313/36/15/18/111/11us(FPS:9009/11235/3731/90909/500000/3194/27777/66666/55555/9009/90909) | ESP32:1212KB | src:96(19436) | test:66(9833) | lizard:77w
Core:
- main.cpp: the boot Layer now wires only one default effect (NoiseEffect, 16x16) and NO default modifier — a bare device still shows lights, but a device-model catalog entry can replace the effect/modifier set (see replaceChildren).
- Control.h: sanitizeHostname trims ALL trailing hyphens (was a single one), so e.g. "name--" → "name".
- platform_desktop: the improvProvisioningInit stub gains the txPowerOut/txPowerReady params to match the platform.h contract (the ESP32 path already had them).
UI:
- app.js: catalog inject supports a `replaceChildren` flag on a container unit (Layer/Layouts/Drivers) — it DELETEs the container's current children before adding the entry's, so a device's catalog effects replace the boot defaults instead of stacking behind them (a Layer renders only its first enabled effect/modifier). Also guards `entry.modules` is an array before iterating.
- install-orchestrator.js: encodeSetDeviceModelPayload rejects non-printable-ASCII bytes before sending (the device-side validator would otherwise reject after the wire round-trip).
- index.html: the "Device is online!" success screen now ALWAYS shows the IP link and ADDS the <deviceName>.local link below it when the boot serial reported one (was: showed only one, preferring mdns). When the HTTP control push didn't land, the clicked link carries `?deviceModel=` so first-visit applies the controls.
Scripts:
- check_devices.py: validate `replaceChildren` is a bool; the "entry must set System.deviceModel" check now also fails an entry with an empty modules list (was skipped when empty).
Tests:
- unit_sanitizeHostname: cases for multiple leading/trailing hyphens and preserved interior hyphens.
Docs / CI:
- docs/gettingstarted.md (new): a short, beginner-tone end-to-end install walkthrough (open the web installer → pick port → pick device → install → join network → open the device), one screenshot per step, links out to the overview / architecture / MoonDeck / building for depth. Screenshots under docs/assets/gettingstarted/ named semantically (01-installer-start … 12-online-wifi).
- install/README.md: catalog schema documents `replaceChildren` + the testbench example (Grid 8x8 + Layer{replaceChildren} + AudioSpectrum + RandomMap); fixed a stale Board/board example left from the deviceModel rename.
- deviceModels.json: testbench S3 entry adds Grid 8x8 + Layer{replaceChildren:true} + AudioSpectrumEffect + RandomMapModifier, so it flashes as an audio-reactive 8x8 device.
- CLAUDE.md: the device-catalog gate description now says System.deviceModel (was Board.board).
Reviews:
- 🐇 desktop improvProvisioningInit stub missing txPowerOut/txPowerReady → added (matches contract) (fixed).
- 🐇 check_devices deviceModel-identity check skipped on empty modules → gated only on board_control_seen (fixed).
- 🐇 app.js entry.modules not validated as array → Array.isArray guard (fixed).
- 🐇 encodeSetDeviceModelPayload accepted non-printable bytes → rejects before length check (fixed).
- 🐇 sanitizeHostname trimmed only one trailing hyphen → while-loop (fixed).
- 🐇 CLAUDE.md Board.board gate desc → System.deviceModel (fixed).
- 🐇 README stale Board schema example → already fixed by the deviceModel rename (no-op).
- 🐇 success link missing ?deviceModel= when push pending → decorate the clicked link (fixed).
- 🐇 deviceModel /api/control bypasses setDeviceModel validation → skipped: documented accepted limitation; the right fix is a per-control validator hook on ControlDescriptor, not a one-off (HTTP-write callers source from the project-controlled catalog, no user-typed input).
- 🐇 read MM_DEVICE post-provision for the .local link on the WiFi path → skipped: the orchestrator has handed the port to ImprovSerial by then; re-acquiring to scrape serial is fiddly for a nicety (the IP link works; Ethernet/already-online paths already get .local). Backlogged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
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/install/README.md (1)
186-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify "Board unit" terminology in the catalog-hardware discussion.
Lines 186–187 state "vendor entries (the QuinLED Dig-2-Go, …) carry only their
Boardunit plus the default LED driver" but the firmware no longer has aBoardmodule—it was folded intoSystem.deviceModel. This reference is confusing in the context of the new catalog schema.Revise to describe these entries as carrying only a
Systemmodule withdeviceModelcontrol (matching the schema example above) plus the default LED driver, to reflect the migrated architecture.🤖 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 186 - 187, The text on lines 186–187 references a "Board unit" which no longer exists in the current firmware architecture since it was consolidated into the System module's deviceModel control. Replace the phrase describing vendor entries as carrying "Board unit plus the default LED driver" with language that accurately reflects the migrated architecture, specifically stating they carry a System module with deviceModel control plus the default LED driver to align with the new catalog schema.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 115: The ordered list item is incorrectly numbered as 8 when it should be
7 according to the markdown linter rule MD029. Fix the list numbering by
changing the list item number from 8 to 7 at the beginning of the device-model
catalog gate item to ensure the ordered list maintains strict incremental
numbering throughout the sequence.
In `@docs/gettingstarted.md`:
- Around line 8-9: The browser requirements section currently lists only "Google
Chrome or Microsoft Edge" as supported browsers for the Web Serial API feature.
Update this list to include Firefox 151+ (released May 2026) and Opera 76+,
which now support the Web Serial API. Note that Safari remains unsupported.
Replace the text that currently states "Google Chrome or Microsoft Edge" with an
updated list that reflects the current support matrix for the Web Serial API
across these four browsers.
In `@src/ui/app.js`:
- Around line 336-345: The findByName recursive function can throw when
tree.modules or m.children contain non-iterable/non-array values from malformed
API responses. Add defensive Array.isArray() checks before the for loops
iterating over mods and m.children to ensure they are actual arrays, using empty
arrays as fallback for non-array values. Apply similar defensive checks to the
parent.children access on line 343 to handle cases where parent exists but
children is not an iterable, ensuring the function gracefully handles malformed
payload structures without throwing errors.
In `@src/ui/install-picker-boards.js`:
- Line 90: The detection status text in the template string contains incorrect
pluralization where it always displays "match" regardless of the matches.length
value. Fix this by conditionally rendering either "match" for singular (when
matches.length equals 1) or "matches" for plural (when matches.length is not 1).
Use a ternary operator within the template string to check matches.length and
select the appropriate plural form.
---
Outside diff comments:
In `@docs/install/README.md`:
- Around line 186-187: The text on lines 186–187 references a "Board unit" which
no longer exists in the current firmware architecture since it was consolidated
into the System module's deviceModel control. Replace the phrase describing
vendor entries as carrying "Board unit plus the default LED driver" with
language that accurately reflects the migrated architecture, specifically
stating they carry a System module with deviceModel control plus the default LED
driver to align with the new catalog schema.
🪄 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: a4ca5dd3-a7c8-4462-a4b6-59f4807305ab
⛔ Files ignored due to path filters (12)
docs/assets/gettingstarted/01-installer-start.pngis excluded by!**/*.pngdocs/assets/gettingstarted/02-select-port.pngis excluded by!**/*.pngdocs/assets/gettingstarted/03-port-selected.pngis excluded by!**/*.pngdocs/assets/gettingstarted/04-pick-device.pngis excluded by!**/*.pngdocs/assets/gettingstarted/05-device-details.pngis excluded by!**/*.pngdocs/assets/gettingstarted/06-device-setup.pngis excluded by!**/*.pngdocs/assets/gettingstarted/07-erasing.pngis excluded by!**/*.pngdocs/assets/gettingstarted/08-installing.pngis excluded by!**/*.pngdocs/assets/gettingstarted/09-wifi-credentials.pngis excluded by!**/*.pngdocs/assets/gettingstarted/10-online-existing-ip.pngis excluded by!**/*.pngdocs/assets/gettingstarted/11-online-ethernet.pngis excluded by!**/*.pngdocs/assets/gettingstarted/12-online-wifi.pngis excluded by!**/*.png
📒 Files selected for processing (13)
CLAUDE.mddocs/gettingstarted.mddocs/install/README.mddocs/install/deviceModels.jsondocs/install/index.htmldocs/install/install-orchestrator.jsscripts/check/check_devices.pysrc/core/Control.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/ui/app.jssrc/ui/install-picker-boards.jstest/unit/core/unit_sanitizeHostname.cpp
| 6. ESP32 build, `build_esp32.py`, if any file under `src/` (excluding `src/platform/desktop/`), `esp32/`, `CMakeLists.txt`, or `library.json` changed. | ||
| 7. KPI collection, `collect_kpi.py --commit`, if any file under `src/` changed. **The one-liner MUST include `tick:Xus(FPS:Y)` for every supported target** (PC + ESP32 today; Teensy/RPi when added). If a target's tick/FPS is missing (e.g. ESP32 wasn't monitored recently and `esp32/monitor.log` is stale), re-run a short live capture before committing, or note explicitly in the commit body why the value is absent. | ||
| 8. Board catalog, `check_boards.py`, fast (<1s), if `docs/install/boards.json` or `scripts/check/check_boards.py` changed. Validates the installer catalog: required fields, `firmwares` a non-empty list of non-empty strings (`firmwares[0]` is the default), every `image` resolves on disk, each `Board.board` control equals its entry `name`, module `type`s are factory-registered (or boot-wired singletons), `pins` controls live only on `*LedDriver` modules, and `supported` capabilities stay within the known vocabulary. | ||
| 8. Device-model catalog, `check_devices.py`, fast (<1s), if `docs/install/deviceModels.json` or `scripts/check/check_devices.py` changed. Validates the installer catalog: required fields, `firmwares` a non-empty list of non-empty strings (`firmwares[0]` is the default), every `image` resolves on disk, each entry's `System.deviceModel` control equals its entry `name`, module `type`s are factory-registered (or boot-wired singletons), `pins` controls live only on `*LedDriver` modules, and `supported` capabilities stay within the known vocabulary. |
There was a problem hiding this comment.
Fix ordered-list numbering at this gate item (MD029).
markdownlint-cli2 flags this line with MD029 (Expected: 7; Actual: 8). Please renumber this list item sequence so the ordered list is strictly incremental for the configured rule.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 115-115: Ordered list item prefix
Expected: 7; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
🤖 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 `@CLAUDE.md` at line 115, The ordered list item is incorrectly numbered as 8
when it should be 7 according to the markdown linter rule MD029. Fix the list
numbering by changing the list item number from 8 to 7 at the beginning of the
device-model catalog gate item to ensure the ordered list maintains strict
incremental numbering throughout the sequence.
Source: Linters/SAST tools
Comment on lines
+8
to
+9
| and **Google Chrome or Microsoft Edge** on a computer (the installer uses a | ||
| browser feature only those two support). |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the current browser support for Web Serial API?
💡 Result:
As of June 19, 2026, the Web Serial API is supported in several major desktop browsers, though it remains unavailable in Safari and on most mobile platforms [1][2][3]. Key support details include: Desktop Browser Support: - Chrome: Supported since version 89 (March 2021) [1][3]. - Edge: Supported since version 89 [1][3]. - Opera: Supported since version 76 [1][3]. - Firefox: Added support in version 151 (released May 2026) for desktop [4][3]. Mobile and Other Platforms: - Safari: Does not support the Web Serial API on any platform (including macOS, iOS, or iPadOS), and the WebKit team has maintained an opposed position citing privacy and security concerns [1][2][3]. - Android: While Chrome for Android 148 (April 2026) added support, it is not universally available across all Android browsers [1][3]. Firefox for Android and other mobile browsers generally do not support it [3]. Usage Requirements: The API is restricted to secure contexts (HTTPS) and requires explicit user permission for every site-to-device interaction [5][4]. Websites must use navigator.serial.requestPort to initiate a connection, ensuring that users maintain control over which ports are accessible [4][6][7]. Due to the lack of cross-browser support—particularly in Safari—the Web Serial API is not currently considered a "Baseline" feature [5][2]. Developers should implement feature detection using 'serial' in navigator to ensure graceful degradation in unsupported environments [6].
Citations:
- 1: https://caniuse.com/web-serial
- 2: https://web-platform-dx.github.io/web-features-explorer/features/serial/
- 3: https://www.testmuai.com/learning-hub/web-serial-api-browser-support/
- 4: https://hacks.mozilla.org/2026/05/web-serial-support-in-firefox/
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Web%5FSerial%5FAPI
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Web_Serial_API
- 7: https://wicg.github.io/serial/
🏁 Script executed:
# Search for Web Serial API usage in the codebase
rg "navigator\.serial|WebSerialAPI|web-serial" --type js --type ts --type jsx --type tsx -iRepository: MoonModules/projectMM
Length of output: 93
🏁 Script executed:
# Also search for installer-related code and USB functionality
rg "installer|usb|port.*select" --type js --type ts --type jsx --type tsx -i -A 3 -B 1 | head -100Repository: MoonModules/projectMM
Length of output: 93
🏁 Script executed:
# Check the docs file to understand the full context
cat -n docs/gettingstarted.mdRepository: MoonModules/projectMM
Length of output: 3978
🏁 Script executed:
# Find installer-related files
fd -t f -i "install" | head -20Repository: MoonModules/projectMM
Length of output: 559
🏁 Script executed:
# Search for web-related files/directories
fd -t d "web\|dist\|src" | head -20Repository: MoonModules/projectMM
Length of output: 47
🏁 Script executed:
# Look for package.json or main config to understand project structure
git ls-files | grep -E "package\.json|tsconfig|vite|webpack" | head -10Repository: MoonModules/projectMM
Length of output: 47
🏁 Script executed:
# Check file sizes and then read the installer files
wc -l docs/install/install-orchestrator.js src/ui/install-picker.js src/ui/install-picker-boards.jsRepository: MoonModules/projectMM
Length of output: 193
🏁 Script executed:
# Search for Web Serial API usage in installer files
rg "navigator\.serial|requestPort|SerialPort" docs/install/ src/ui/Repository: MoonModules/projectMM
Length of output: 4718
Update browser requirements to include Firefox, which added Web Serial API support in May 2026.
The installer requires Web Serial API for USB port selection (confirmed in the code). However, the documentation stating only "Google Chrome or Microsoft Edge" is now incomplete. Firefox 151+ (released May 2026) now supports the API, so it should be added to the supported browsers list. Opera 76+ also supports it. Safari remains unsupported.
Update lines 8–9 to reflect the current support matrix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/gettingstarted.md` around lines 8 - 9, The browser requirements section
currently lists only "Google Chrome or Microsoft Edge" as supported browsers for
the Web Serial API feature. Update this list to include Firefox 151+ (released
May 2026) and Opera 76+, which now support the Web Serial API. Note that Safari
remains unsupported. Replace the text that currently states "Google Chrome or
Microsoft Edge" with an updated list that reflects the current support matrix
for the Web Serial API across these four browsers.
…cal link
Lets a device spray lights to the whole LAN with no IP typed (the sender now defaults to broadcast), adds an installer checkbox so a re-flash no longer silently overwrites a configured device's setup, shows the device's .local link after WiFi provisioning, and processes a CodeRabbit batch.
KPI: 16384lights | PC:365KB | tick:113/87/112/9/1/312/36/15/18/111/11us(FPS:8849/11494/8928/111111/1000000/3205/27777/66666/55555/9009/90909) | ESP32:1212KB | src:96(19452) | test:66(9833) | lizard:77w
Core:
- UdpSocket (desktop + esp32): set SO_BROADCAST on open() so a send to a broadcast address (255.255.255.255) is allowed instead of rejected; no effect on unicast/multicast sends.
Light domain:
- NetworkSendDriver: default `ip` is now 255.255.255.255 (limited broadcast) — a fresh sender reaches every receiver on the LAN with no IP to type; set a unicast IP to target one device. Verified live S3->P4.
UI:
- install/index.html + install-orchestrator.js: new "Apply device defaults" checkbox. It gates the whole catalog inject (SET_DEVICE_MODEL RPC, the HTTP /api/control fan-out, and the ?deviceModel= first-visit handoff) so re-flashing a configured device no longer silently re-injects defaults (the catalog's replaceChildren would otherwise delete the user's effects). Auto-ticks with "Erase chip first", starts unticked otherwise; the board's txPower brown-out cap still applies either way (hardware trait, not a default).
- install-orchestrator.js: after WiFi provisioning, build <deviceName>.local from the device name the Improv GET_DEVICE_INFO already reports (improvClient.info.name) and pass it as the success mdns — the WiFi-provision path now gets the .local link the Ethernet/already-online paths already had, with no serial re-scrape.
- install/index.html: the "Device is online!" success screen shows the IP and the <name>.local links on two lines (block instead of inline-block).
- app.js: clearModuleChildren's findByName guards every level with Array.isArray so a malformed /api/state payload can't throw.
- install-picker-boards.js: fix the detection-status pluralization ("1 match" vs "N matches").
Docs / CI:
- gettingstarted.md: note that a device boots with the modules/settings shown under "details" and that any of it can be changed later in the UI; browser list corrected to Chromium-based (Chrome / Edge / Opera; Safari + Firefox unsupported).
- install/README.md: document the "Apply device defaults" checkbox and its inject gating; the catalog example carries a System unit with the deviceModel control (not a removed Board unit).
- deviceModels.json: testbench P4 gets a Layer{replaceChildren} + NetworkReceiveEffect (receiver as first-layer effect); testbench S3 gets a NetworkSendDriver; all testbench entries set System.deviceName.
- NetworkSendDriver.md: document the broadcast default and the unicast/broadcast-not-multicast interop note.
- check_devices.py: validate the optional `replaceChildren` field is a bool; the deviceModel-identity check no longer skips an empty modules list.
- backlog: drop the shipped ".local link on the WiFi-provision path" follow-up.
Reviews:
- 🐇 app.js findByName could throw on a non-array payload -> Array.isArray guards at each level (fixed).
- 🐇 install-picker-boards always printed "match" -> singular/plural ternary (fixed).
- 🐇 README "Board unit" stale after the deviceModel rename -> System unit with deviceModel control (fixed).
- 🐇 gettingstarted browser list -> added Opera; did NOT add "Firefox 151+ (May 2026)" — Firefox has not shipped Web Serial and that version is unverified/future-dated (skipped that part).
- 🐇 CLAUDE.md:115 renumber 8->7 (MD029) -> skipped: the list is Always-run=1 then conditional 2..9, strictly incremental; item is correctly numbered 8, no violation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/install/install-orchestrator.js (1)
1093-1102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t report an unapplied model as the device identity.
When
applyDefaultsis false, the code above skips bothSET_DEVICE_MODELand the HTTP control fan-out, but this success payload still returnsboard, so host pages can persist/display a model that was intentionally not applied. Return an empty applied model here, or add a separateselectedDeviceModelfield if callers need the original picker selection.Suggested fix
onSuccess({ url: deviceUrl, mdns: deviceMdns, // "<deviceName>.local" from the boot serial, "" if unknown - board: board || "", + board: applyDefaults ? (board || "") : "", applyDefaults, // false → host page skips the ?deviceModel= first-visit handoff viaHttp, httpBoardOk,🤖 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 1093 - 1102, The onSuccess callback in the device configuration block returns the board field regardless of whether applyDefaults is true or false, which allows host pages to persist an unapplied model when applyDefaults is false. Modify the onSuccess call to conditionally set the board field to an empty string when applyDefaults is false, ensuring that only applied models are returned as the device identity. Alternatively, if callers need access to the original picker selection, add a separate selectedDeviceModel field while setting board to empty string when the model was not applied.
♻️ Duplicate comments (1)
docs/gettingstarted.md (1)
8-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate browser support to include Firefox 151+, which added Web Serial API support in May 2026.
Line 8-10 states that Safari and Firefox don't support the Web Serial API. However, Firefox 151 (released May 2026) added support for the API, and the current date is June 19, 2026. The supported browser list should include Chrome, Edge, Opera, and Firefox 151+. Safari remains unsupported.
🔧 Proposed fix
-and a **Chromium-based browser** on a computer — Google Chrome, Microsoft Edge, -or Opera (the installer uses the Web Serial API, which Safari and Firefox don't -support). +and a browser with Web Serial API support on a computer — **Google Chrome 89+**, +**Microsoft Edge 89+**, **Opera 76+**, or **Firefox 151+** (released May 2026). +Safari does not support the Web Serial API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/gettingstarted.md` around lines 8 - 10, Update the browser support documentation to reflect that Firefox 151+ now supports the Web Serial API. In the section stating that "Safari and Firefox don't support the Web Serial API," modify the text to specify that Firefox 151 and later versions are supported, while keeping Safari as unsupported. The supported browser list should now include Google Chrome, Microsoft Edge, Opera, and Firefox 151+ instead of excluding Firefox entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules/light/drivers/NetworkSendDriver.md`:
- Line 29: The phrase "is still deferred" on line 29 violates the documentation
guideline that prohibits forward-looking language in main docs. Replace the
forward-looking phrasing about E1.31 multicast in the NetworkSendDriver.md file
with present-tense language describing the current state (such as "is not
supported" or "is not currently implemented" instead of "is still deferred"),
while retaining the reference to the backlog entry as the planning artifact.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 595-602: The setsockopt call that enables SO_BROADCAST in the
UdpSocket::open() method doesn't check the return value, so the function returns
true even if socket configuration fails. Capture the return value of the
setsockopt call (where SO_BROADCAST is being set), check if it indicates failure
(typically a return value less than 0), and if so, return false or handle the
error appropriately to prevent the socket from being left in a partially
configured state. This ensures the function only returns true when the socket is
fully and successfully configured.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1075-1081: The setsockopt call with SO_BROADCAST flag in the
function around line 1080 is not checking for failures. Capture the return value
of the setsockopt(fd_, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on)) call and check
if it returns a non-zero value (indicating failure). If setsockopt fails, return
false to propagate the error to the caller instead of returning true and
allowing the function to report success when the broadcast socket option could
not be applied.
---
Outside diff comments:
In `@docs/install/install-orchestrator.js`:
- Around line 1093-1102: The onSuccess callback in the device configuration
block returns the board field regardless of whether applyDefaults is true or
false, which allows host pages to persist an unapplied model when applyDefaults
is false. Modify the onSuccess call to conditionally set the board field to an
empty string when applyDefaults is false, ensuring that only applied models are
returned as the device identity. Alternatively, if callers need access to the
original picker selection, add a separate selectedDeviceModel field while
setting board to empty string when the model was not applied.
---
Duplicate comments:
In `@docs/gettingstarted.md`:
- Around line 8-10: Update the browser support documentation to reflect that
Firefox 151+ now supports the Web Serial API. In the section stating that
"Safari and Firefox don't support the Web Serial API," modify the text to
specify that Firefox 151 and later versions are supported, while keeping Safari
as unsupported. The supported browser list should now include Google Chrome,
Microsoft Edge, Opera, and Firefox 151+ instead of excluding Firefox entirely.
🪄 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: cf1da9e8-e6c6-409c-a812-3f50a0ac5dd9
📒 Files selected for processing (12)
docs/backlog/backlog.mddocs/gettingstarted.mddocs/install/README.mddocs/install/deviceModels.jsondocs/install/index.htmldocs/install/install-orchestrator.jsdocs/moonmodules/light/drivers/NetworkSendDriver.mdsrc/light/drivers/NetworkSendDriver.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/ui/app.jssrc/ui/install-picker-boards.js
|
|
||
| - **Universe rule (both ends):** buffer offset = (universe − `universe_start`) × 510, and the sender emits from `universe_start` verbatim — no hidden 1-based adjustment for E1.31. Strict sACN gear reserves universe 0, so set `universe_start ≥ 1` on **both** ends when talking to it; the matching default of 0 on our own receiver keeps device↔device pairs aligned out of the box. | ||
| - **Unicast only.** E1.31 multicast (group 239.255.x.x) is deferred — the platform has no IGMP join yet; MoonLight ships unicast-only too. See the backlog entry. | ||
| - **Unicast or broadcast; not multicast.** The destination can be a single device (unicast) or the limited-broadcast address `255.255.255.255` (the default), which sprays the frame to every device on the LAN — the platform socket sets `SO_BROADCAST` so this works for all three protocols. The standard Art-Net convention is broadcast; a device↔device pair works out of the box with no IP typed. E1.31 multicast (group 239.255.x.x) is still deferred — the platform has no IGMP join yet; MoonLight ships without it too. See the backlog entry. |
There was a problem hiding this comment.
Remove forward-looking wording from main docs
Line 29 says E1.31 multicast “is still deferred,” which is forward-looking language in non-backlog docs. Rephrase to present-tense behavior only and keep backlog linkage as the planning artifact.
As per coding guidelines, “No forward-looking language ('roadmap', 'will be', 'planned', 'in the future', 'currently lacks', TODO, FIXME) in code and docs except docs/backlog/ and docs/history/.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/light/drivers/NetworkSendDriver.md` at line 29, The phrase
"is still deferred" on line 29 violates the documentation guideline that
prohibits forward-looking language in main docs. Replace the forward-looking
phrasing about E1.31 multicast in the NetworkSendDriver.md file with
present-tense language describing the current state (such as "is not supported"
or "is not currently implemented" instead of "is still deferred"), while
retaining the reference to the backlog entry as the planning artifact.
Source: Coding guidelines
Comment on lines
+595
to
+602
| if (fd_ < 0) return false; | ||
| // Allow sends to a broadcast address (e.g. 255.255.255.255 for an Art-Net / | ||
| // E1.31 spray to every device on the LAN). Without SO_BROADCAST the OS rejects | ||
| // such a send with EACCES; it has no effect on unicast/multicast sends. | ||
| const int on = 1; | ||
| ::setsockopt(sock(fd_), SOL_SOCKET, SO_BROADCAST, | ||
| reinterpret_cast<const char*>(&on), sizeof(on)); | ||
| return true; |
There was a problem hiding this comment.
Handle SO_BROADCAST failure instead of returning success
UdpSocket::open() returns true even if enabling broadcast fails (Line 600), which leaves the socket in a partially configured state and causes later broadcast sends to fail silently.
Suggested fix
bool UdpSocket::open() {
if (fd_ >= 0) return true;
fd_ = open_sock(AF_INET, SOCK_DGRAM, 0);
if (fd_ < 0) return false;
@@
const int on = 1;
- ::setsockopt(sock(fd_), SOL_SOCKET, SO_BROADCAST,
- reinterpret_cast<const char*>(&on), sizeof(on));
+ if (::setsockopt(sock(fd_), SOL_SOCKET, SO_BROADCAST,
+ reinterpret_cast<const char*>(&on), sizeof(on)) != 0) {
+ close_sock(fd_);
+ fd_ = -1;
+ return false;
+ }
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (fd_ < 0) return false; | |
| // Allow sends to a broadcast address (e.g. 255.255.255.255 for an Art-Net / | |
| // E1.31 spray to every device on the LAN). Without SO_BROADCAST the OS rejects | |
| // such a send with EACCES; it has no effect on unicast/multicast sends. | |
| const int on = 1; | |
| ::setsockopt(sock(fd_), SOL_SOCKET, SO_BROADCAST, | |
| reinterpret_cast<const char*>(&on), sizeof(on)); | |
| return true; | |
| bool UdpSocket::open() { | |
| if (fd_ >= 0) return true; | |
| fd_ = open_sock(AF_INET, SOCK_DGRAM, 0); | |
| if (fd_ < 0) return false; | |
| // Allow sends to a broadcast address (e.g. 255.255.255.255 for an Art-Net / | |
| // E1.31 spray to every device on the LAN). Without SO_BROADCAST the OS rejects | |
| // such a send with EACCES; it has no effect on unicast/multicast sends. | |
| const int on = 1; | |
| if (::setsockopt(sock(fd_), SOL_SOCKET, SO_BROADCAST, | |
| reinterpret_cast<const char*>(&on), sizeof(on)) != 0) { | |
| close_sock(fd_); | |
| fd_ = -1; | |
| return false; | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/platform_desktop.cpp` around lines 595 - 602, The
setsockopt call that enables SO_BROADCAST in the UdpSocket::open() method
doesn't check the return value, so the function returns true even if socket
configuration fails. Capture the return value of the setsockopt call (where
SO_BROADCAST is being set), check if it indicates failure (typically a return
value less than 0), and if so, return false or handle the error appropriately to
prevent the socket from being left in a partially configured state. This ensures
the function only returns true when the socket is fully and successfully
configured.
Comment on lines
+1075
to
+1081
| if (fd_ < 0) return false; | ||
| // Allow sends to a broadcast address (e.g. 255.255.255.255 for an Art-Net / | ||
| // E1.31 spray to every device on the LAN). Without SO_BROADCAST the stack | ||
| // rejects such a send; it has no effect on unicast/multicast sends. | ||
| const int on = 1; | ||
| setsockopt(fd_, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on)); | ||
| return true; |
There was a problem hiding this comment.
Propagate failure when broadcast socket option cannot be applied
At Line 1080, setsockopt(...SO_BROADCAST...) is unchecked. If it fails, open() still reports success and later broadcast output fails non-obviously.
Suggested fix
bool UdpSocket::open() {
if (fd_ >= 0) return true;
fd_ = socket(AF_INET, SOCK_DGRAM, 0);
if (fd_ < 0) return false;
@@
const int on = 1;
- setsockopt(fd_, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on));
+ if (setsockopt(fd_, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on)) != 0) {
+ lwip_close(fd_);
+ fd_ = -1;
+ return false;
+ }
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (fd_ < 0) return false; | |
| // Allow sends to a broadcast address (e.g. 255.255.255.255 for an Art-Net / | |
| // E1.31 spray to every device on the LAN). Without SO_BROADCAST the stack | |
| // rejects such a send; it has no effect on unicast/multicast sends. | |
| const int on = 1; | |
| setsockopt(fd_, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on)); | |
| return true; | |
| if (fd_ < 0) return false; | |
| // Allow sends to a broadcast address (e.g. 255.255.255.255 for an Art-Net / | |
| // E1.31 spray to every device on the LAN). Without SO_BROADCAST the stack | |
| // rejects such a send; it has no effect on unicast/multicast sends. | |
| const int on = 1; | |
| if (setsockopt(fd_, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on)) != 0) { | |
| lwip_close(fd_); | |
| fd_ = -1; | |
| return false; | |
| } | |
| return true; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 1075-1075: statement should be inside braces
(readability-braces-around-statements)
[warning] 1079-1079: variable name 'on' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 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 1075 - 1081, The
setsockopt call with SO_BROADCAST flag in the function around line 1080 is not
checking for failures. Capture the return value of the setsockopt(fd_,
SOL_SOCKET, SO_BROADCAST, &on, sizeof(on)) call and check if it returns a
non-zero value (indicating failure). If setsockopt fails, return false to
propagate the error to the caller instead of returning true and allowing the
function to report success when the broadcast socket option could not be
applied.
…dings
Folds in the pre-merge reviewer and CodeRabbit findings before the PR merges: the installer's direct HTTP inject path now honours replaceChildren (so the testbench device-model entries replace boot defaults instead of stacking), the re-flash-keeping-config path no longer persists an unapplied device model, plus comment/doc/indent tidies and a carried-forward naming lesson.
KPI: 16384lights | PC:365KB | tick:112/88/110/9/1/310/36/15/18/112/11us(FPS:8928/11363/9090/111111/1000000/3225/27777/66666/55555/8928/90909) | ESP32:1212KB | src:96(19452) | test:66(9833) | lizard:77w
UI:
- install-orchestrator.js: tryHttpInjectBoard now applies replaceChildren via a new clearDeviceChildren helper (enumerate the named container's children, DELETE each, then add the entry's) — the installer's reachable-device HTTP path now matches the device-UI ?deviceModel= path, so testbench entries that mark a container replaceChildren replace the boot defaults instead of stacking behind them. Fixed two stale comments (consumePendingDeviceModelParam, System/Network top-level).
- index.html: when "Apply device defaults" is unticked, addProvisionedDevice stores no board — a re-flash that keeps the device's config must not save (or offer to Inject) a device model that was never applied.
Light domain:
- RotateModifier.h: align a comment block's first line to the 12-space body indent (cosmetic).
Docs / CI:
- NetworkSendDriver.md: present-tense the E1.31-multicast note ("is not implemented", not "is still deferred"); the backlog entry remains the planning artifact.
- history/decisions.md: carry forward the deviceName (per-unit identity) vs deviceModel (product) vs board (bare PCB) split — when one noun answered three questions, the fix was to split into qualified terms and make the split visible in every layer.
Reviews:
- 👾 tryHttpInjectBoard duplicated the inject walk WITHOUT replaceChildren, so the testbench entries would stack effects via the installer HTTP path -> added clearDeviceChildren so the path honours replaceChildren (fixed).
- 👾 stale comments referencing the renamed consumePendingBoardParam / a Board-under-System unit -> updated (fixed).
- 👾 RotateModifier comment indent off by one space -> aligned (fixed).
- 👾 check_devices.py still prints "Board check"/"boards" wording -> deferred: the assets/boards/ image dir isn't renamed this branch, so renaming only the output strings would be a half-rename; revisit when the image dir moves.
- 🐇 onSuccess persisted board even when applyDefaults:false -> store "" so the saved entry doesn't claim an unapplied model (fixed).
- 🐇 NetworkSendDriver.md "is still deferred" forward-looking -> present-tense (fixed).
- 🐇 UdpSocket::open() doesn't check the SO_BROADCAST setsockopt return (x2 platforms) -> skipped: deliberately not propagated (the reviewer endorsed this) — a failed optional broadcast flag must not fail socket open for the unicast/multicast majority; a real broadcast failure surfaces at the broadcast send.
- 🐇 gettingstarted browser list should add "Firefox 151+" -> skipped: Firefox has not shipped Web Serial and that version is unverified/future-dated (same as the prior batch's skip).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… apply-defaults UX Makes the catalog inject survive a flaky device (the P4 right after flash) instead of abandoning setup on one hiccup, surfaces "applying device defaults" where the user actually sees it, and fills in the P4 testbench's grid + brightness. UI: - install-orchestrator.js: new deviceFetch helper retries a device request up to 3x with backoff on timeout / network error / 5xx (but not on a deterministic 4xx). tryHttpInjectBoard and clearDeviceChildren are now best-effort — a unit that still fails after retries is logged and skipped, so a single transient hiccup on a flaky device (the P4 over Ethernet, busy with link bring-up + per-add buildState right after flash) no longer abandons the rest of the inject. Returns false only if something was actually skipped, so the ?deviceModel= fallback still triggers. Was the root cause of the P4 coming up with an empty Layer and default grid. - index.html + install-orchestrator.js: "applying device defaults" is now both a visible modal status during the inject (fires on the eth/typed-IP path too, not just Improv) AND a durable note folded into the "Device is online!" popup (which stays up) — "Applied X defaults" / "X defaults will be applied when you open the device" / "Kept the device's existing config". The transient status alone flashed by too fast to read. Docs / CI: - deviceModels.json: testbench P4 gets Grid 8x8 (was missing → boot default 16x16) and Drivers.brightness 255 (the shared output default is a deliberately-low 20 for unknown supplies; the testbench has a known supply). Reviews: - 👾 (follow-up to the pre-merge reviewer) tryHttpInjectBoard's all-or-nothing abort was too fragile for a flaky device -> retry + best-effort, matching the device-UI handoff's contract (fixed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reshapes how a projectMM device identifies itself, on the network and to the tooling, then builds three device-networking + onboarding features on top of that identity. A device now has two clear identity facets —
deviceName(which individual unit, the network identity) anddeviceModel(which product/hardware) — each owned by SystemModule. Five commits; the identity work and the broadcast link are verified live on the ESP32-S3 and ESP32-P4.What landed
1.
deviceNameas the single network identity (fdcdcd1)deviceName(SystemModule) is the one source for the mDNS<name>.local, the SoftAP SSID, and the DHCP hostname — sanitised to a valid RFC-1123 hostname at the source (mm::sanitizeHostname), MAC-MM-XXXXfallback when empty, and it follows a live rename (mDNS re-registers within a tick).MM_DEVICE=<deviceName>.localon the boot-serial tick line so the web installer can offer a clickable.localsuccess link;readBootIpcaptures it alongsideMM_IP..locallink; the board-catalog/chip-detect code split into a Pages-only module (dependency-injected, not embedded in firmware).dragTsstamping), the editable-control guard hoisted, thepincontrol patches live.2. mDNS peer identity + green-dot fixes (
bc90a49)mm=1TXT record on its_http._tcpservice; a browsing peer reads it (and theinstance_name) to classify + name the peer without an HTTP scan. Promotion is gated on the base service being generic_http.ageSeclives), so an mDNS-fresh peer actually shows green; cached/ageSec render is mutually exclusive.3. Fold BoardModule into SystemModule; rename board → deviceModel (
ac37383)deviceModelcontrol +setDeviceModel(). Net subtraction.board→deviceModelacross code, the device catalog (boards.json→deviceModels.json,check_boards.py→check_devices.py, catalogtype:Boardblocks → top-leveltype:System/deviceModel), the wire/URL contracts (?deviceModel=, ImprovSET_DEVICE_MODEL— wire byte0xFEunchanged), MoonDeck's device-facing reads/pushes, and the docs.deviceNameis untouched; "board" survives only in its literal bare-PCB sense (on-board LED, board-soldered pins).4. Getting-started guide + testbench defaults +
replaceChildren(2804b47)docs/gettingstarted.md: a short, beginner-tone end-to-end install walkthrough (open the web installer → pick port → pick device → install → join network → open the device), one screenshot per step, links out for depth.replaceChildrenflag on a container unit (Layer/Layouts/Drivers): it DELETEs the container's boot-default children before adding the entry's, so a device's catalog effects replace the defaults instead of stacking behind them (a Layer renders only its first enabled effect). The testbench S3 entry uses it to come up as an 8×8 AudioSpectrum + RandomMap device.5. Broadcast Art-Net send + installer "Apply device defaults" + WiFi
.locallink (c95560e)UdpSocket::open()setsSO_BROADCAST(both platforms) andNetworkSendDriver'sipdefaults to255.255.255.255, so a sender reaches every receiver on the LAN with no IP typed. Verified live: the S3 sprays Art-Net broadcast and the P4 (receiver in its first layer) renders it./api/controlfan-out, and the?deviceModel=first-visit handoff) so re-flashing a configured device no longer silently re-injects defaults (replaceChildrenwould otherwise delete the user's effects). Auto-ticks with "Erase chip first", starts unticked otherwise; the board'stxPowerbrown-out cap still applies either way..locallink — the success screen builds<deviceName>.localfrom the name the ImprovGET_DEVICE_INFOalready reports, so the WiFi path gets the.locallink the Ethernet/already-online paths had, with no serial re-scrape. IP and.localnow render on two lines.6–7. Installer inject hardening + P4 testbench defaults + apply-defaults UX (
4d8587f,5d9d7f7)tryHttpInjectBoard) gainedreplaceChildrensupport (newclearDeviceChildren), so it matches the device-UI?deviceModel=path — testbench entries replace boot defaults instead of stacking.deviceFetchhelper retries a device request 3× with backoff on timeout / network error / 5xx (but not on a deterministic 4xx), andtryHttpInjectBoard/clearDeviceChildrenare now best-effort (log-and-continue, not all-or-nothing). This fixed the P4 coming up with an empty Layer + default grid: one transient hiccup right after flash (eth bring-up + per-add buildState) no longer abandons the rest of setup. Verified live on the P4.deviceModels.jsongains Grid 8×8 (was missing → 16×16 default) andDrivers.brightness255 (the shared output default is a deliberately-low 20 for unknown supplies).Verification
check_devices,check_firmwares, KPI. (Installer-JS/catalog-only commits run the cheap doc path — spec + catalog + JS parse.)?board=handoff) since fixed; the pre-merge passes returned mergeable / no MUST-FIX, and their SHOULD findings were folded in (the installer HTTP-inject path gainedreplaceChildrensupport and retry/best-effort robustness; stale comments fixed).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor