next-iteration: SystemModule/Network, persistence, UI rewrite, eth-only build, side-nav#2
Merged
Conversation
…ggle
KPI: 16384lights | PC:131KB | tick:50us(FPS:20000) | ESP32:775KB | tick:66ms(FPS:15) | src:33(3839) | test:12(1399) | lizard:8w
SystemModule:
- deviceName (MM-XXXX from MAC), uptime, fps, tickTimeUs, heap/psram progress bars
- Firmware vs partition progress bar, flash chip size, version, build date, chip, sdk
- Version auto-generated from library.json at build time (single source of truth)
- deviceName shown in HTML title and top bar
NetworkModule (ESP32):
- Ethernet → WiFi STA → WiFi AP priority cascade with automatic fallback
- ethLinkUp() for fast cable detection (3s), 15s DHCP timeout
- esp_wifi_deinit() on AP/STA stop to fully release WiFi resources
- mDNS toggle (conditional start/stop via syncMdns helper)
- Dynamic controls: DHCP/Static addressing dropdown shows/hides IP fields
- AP at 4.3.2.1, open, deviceName as SSID
- Credential injection via REST API
New control types:
- ReadOnly (display-only text), Select (dropdown with options), Progress (bar with value/total)
- UI renders all three with live WebSocket updates
- Select change triggers dynamic onBuildControls rebuild
Module enabled toggle:
- Every MoonModule has enabled property, Scheduler skips disabled modules
- UI shows checkbox in card header, settable via POST /api/control
Platform additions:
- getMacAddress, totalHeap, totalInternalHeap, chipModel, sdkVersion
- firmwareSize, firmwarePartition, flashChipSize, filesystemUsed/Total
- ethInit/ethLinkUp/ethConnected, wifiStaInit/Stop, wifiApInit/Stop, mdnsInit/Stop
- All stubs on desktop, full ESP-IDF implementations on ESP32
Other:
- Renamed mmv3 → projectMM throughout codebase
- Pre-commit checklist expanded to 10 steps (live scenario analysis + doc check)
- managed_components/ and dependencies.lock added to .gitignore
- mDNS via ESP-IDF component manager (idf_component.yml)
KPI Details:
Desktop:
Lights: 16,384
Binary: 131 KB
56 tests, 270 assertions, all passed
tick: 50us (FPS: 20000)
6 scenarios, all passed
Platform boundary: PASS
Specs: 10 modules, 10 ok
ESP32:
Image: 900,093 bytes (14% partition free)
Flash (code+data): 775 KB
DRAM: 41,156 / 180,736 (139,580 free)
tick: 66ms (FPS: 15) — stable, no spikes
Free heap: 128KB
Code:
33 source files (3839 lines)
12 test files (1399 lines)
20 specs, 6 scenarios
Lizard: 8 warnings
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames build target to projectMM, adds generated version header, introduces FilesystemModule persistence, System/Network modules, platform filesystem/networking APIs (desktop/ESP32), updates HTTP server and UI, revises tests/scenarios/scripts, and refreshes documentation. ChangesEnd-to-end runtime, platform, UI, and build integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
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)
49-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate device name on initial
/api/stateload too.
updateDeviceName()is only called from WebSocket messages, so title/top-bar can be stale until the first push.Suggested fix
state = await resp.json(); + updateDeviceName(); renderNav();🤖 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 49 - 56, The device name is only updated on WebSocket messages, so modify init() to call updateDeviceName() after loading state from /api/state so the title/top-bar reflect the current device immediately; specifically, after state = await resp.json() and before renderNav()/selectModule(...) invoke updateDeviceName(state) (or call updateDeviceName() if it reads from the global state) so the UI updates on initial load as well.
🤖 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 `@CMakeLists.txt`:
- Around line 19-23: The custom command that generates
${CMAKE_SOURCE_DIR}/src/core/version.h via add_custom_command currently only
lists ${CMAKE_SOURCE_DIR}/library.json as a DEPENDS target, so changes to the
generator script scripts/build/generate_version.py won't trigger regeneration;
update the add_custom_command for OUTPUT ${CMAKE_SOURCE_DIR}/src/core/version.h
(the add_custom_command invocation) to include
${CMAKE_SOURCE_DIR}/scripts/build/generate_version.py in the DEPENDS list so
edits to generate_version.py will re-run the command and produce an up-to-date
version.h.
In `@docs/moonmodules/core/NetworkModule.md`:
- Around line 15-16: The doc claims behaviors (10s AP shutdown delay with UI
message and heap-transition orchestration) that are not implemented; either
update docs to match current implementation or implement those behaviors in
NetworkModule.h. If updating docs: change phrasing to present tense and remove
the AP shutdown delay and heap-transition orchestration sections (lines
referencing "AP shutdown delay", "AP shutting down, switch to your local
network", and the heap transition steps). If implementing: add code in
NetworkModule.h to start a 10‑second timer when onStaConnected/onSTAConnected
fires, emit a UI message "AP shutting down, switch to your local network", then
call the existing AP teardown path after the timer and implement the heap
transition orchestration functions referenced in the doc so the documented
sequence matches the code.
- Around line 110-127: The "Prior art" section (the "Prior art" header and its
items: projectMM v1 entries
`Network.h`/`WifiSta.h`/`WifiAp.h`/`DeviceDiscovery.h`, MoonLight entries, and
ESP-IDF entries like `esp_wifi.h`/`mdns.h`/`esp_netif.h`/`esp_event.h`) should
be removed from the module spec and relocated to the appropriate historical/plan
docs (e.g., docs/history/ or docs/plan.md) so the NetworkModule spec remains
implementation-focused; delete the "Prior art" block from NetworkModule.md and
add the same content to the chosen history/plan file with a short heading
indicating prior art/context.
In `@docs/moonmodules/core/SystemModule.md`:
- Around line 61-69: Remove the "Prior art" section from SystemModule.md (the
"## Prior art" header and the "### projectMM v1" / "### MoonLight" bullets)
since it is historical; either delete it entirely or relocate its content into
docs/history/ per the docs policy, and update any cross-references if other docs
pointed to this section.
- Around line 7-13: The spec list is out of sync with the shipped module:
replace the outdated controls (freeHeap, freeInternal, maxBlock) with the
current dynamic controls used by the implementation — namely read-only "uptime",
read-only "fps", read-only "tickTimeUs" (use the platform API naming that the
codebase currently calls on Scheduler), and progress controls "heap" and "psram"
that report current/total; also update any ESP-IDF API name in the doc to match
the actual function used in the codebase (the Scheduler-related call and the
ESP-IDF heap API the module uses), and apply the same updates for the duplicate
section at lines 50-54.
In `@docs/testing.md`:
- Around line 175-181: Update the mdns-toggle scenario documentation to use
present tense and accurately reflect the scenario JSON: state that the
mdns-toggle scenario measures FPS only (not tick time), remove the claim about
"5KB heap difference" since there are no heap bounds or assertions in the
scenario, and change the verification text to reflect the configured min_pct:80
(i.e., allow up to 20% FPS drop) rather than claiming "zero FPS impact";
reference the mdns-toggle scenario and the min_pct field when making these
edits.
In `@esp32/main/CMakeLists.txt`:
- Around line 25-29: The add_custom_command that generates
${VERSION_DIR}/version.h currently lists only
${COMPONENT_DIR}/../../library.json in DEPENDS; include the generator script
(${COMPONENT_DIR}/../../scripts/build/generate_version.py) in the DEPENDS for
the add_custom_command so CMake will regenerate version.h when the
generate_version.py logic changes (the command invoking the script is the
python3 ${COMPONENT_DIR}/../../scripts/build/generate_version.py entry).
In `@esp32/main/idf_component.yml`:
- Around line 1-3: The repo is ignoring dependencies.lock which prevents
reproducible ESP-IDF Component Manager installs; generate and commit a
dependencies.lock that pins the exact component versions referenced by
idf_component.yml and stop ignoring it in .gitignore (remove dependencies.lock
from the ignore rules), e.g., run the Component Manager lock command to produce
dependencies.lock, add and commit that file alongside the existing
idf_component.yml so builds use locked versions.
In `@src/core/Control.h`:
- Around line 58-75: Replace the raw pointer+length APIs addText, addReadOnly,
and addSelect with std::span-based signatures: change addText(const char* name,
char* var, uint8_t bufSize) -> addText(const char* name, std::span<char> buf),
addReadOnly(const char* name, std::span<const char> buf), and addSelect(const
char* name, uint8_t& var, const char* const* options, uint8_t optionCount) ->
addSelect(const char* name, uint8_t& var, std::span<const char* const> options)
(or std::span<const char*>), include <span>, and update the Control storage and
initialization in these methods to store/encode the span (adjust Control struct
fields to hold span or a pointer+size pair typed appropriately) so
controls_[count_++] = { ... } assigns the span-backed fields; then update all
call sites in SystemModule.h, NetworkModule.h, and ArtNetSendDriver.h to pass
std::span (e.g., std::span{buffer, size} or array-to-span) to the new
addText/addReadOnly/addSelect overloads.
In `@src/core/HttpServerModule.h`:
- Around line 379-385: The current control handler allows disabling the HTTP
server module via the "enabled" control; prevent remote shutdown by refusing to
set enabled=false for the HttpServerModule itself. Modify the branch handling
controlName == "enabled" to detect when the target is this module (e.g., compare
pointer identity or use dynamic_cast<HttpServerModule*>(target)), and if so,
read the requested value with parseJsonBool(body, "value") and if it's false
return an error response (e.g., sendResponse(conn, 403 or 400, ...) with a clear
message) without calling target->setEnabled or scheduler_->rebuild; allow
setting enabled=true for this module as before and leave behavior for other
modules unchanged, using the existing parseJsonBool, target->setEnabled,
scheduler_->rebuild, and sendResponse symbols.
- Around line 417-420: The Select case writes any parsed integer into the
control without range checking; update the ControlType::Select branch (around
parseJsonInt and c.ptr) to validate the parsed int against the control's allowed
bounds before casting/writing: obtain the control's min/max (e.g., c.min/c.max
or c.minValue/c.maxValue depending on your struct), if v is outside [min,max]
return an error response (HTTP 400 / bad request) or reject the update instead
of writing, and only then cast to uint8_t and assign to
*static_cast<uint8_t*>(c.ptr).
In `@src/core/NetworkModule.h`:
- Around line 227-233: The code sets mdnsRunning_ to true unconditionally after
calling platform::mdnsInit(), which prevents retries on failure; change the
logic in the block that checks shouldRun (using mdnsEnabled_, state_,
State::ConnectedEth/ConnectedSta) so that you call platform::mdnsInit(devName)
and only set mdnsRunning_ = true if platform::mdnsInit returns success (or
doesn't throw) — otherwise log or handle the failure and leave mdnsRunning_
false so future iterations can retry; update the branch that currently sets
mdnsRunning_ to refer to the result of platform::mdnsInit and keep references to
devName and systemModule_->deviceName() as-is.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 274-275: wifiStaInit() creates staNetif_ but wifiStaStop() never
destroys it and wifiApInit() creates a local apNetif pointer that is not
retained or destroyed, causing leaked esp-netif objects; update wifiStaStop() to
call esp_netif_destroy_default_wifi(staNetif_) (or
esp_netif_destroy_default_wifi() if used globally) and clear staNetif_
afterwards, change wifiApInit() to store apNetif in a member (e.g., apNetif_) so
wifiApStop() can call esp_netif_destroy_default_wifi(apNetif_) and clear it, and
ensure both wifiStaStop() and wifiApStop() are called before reinitializing to
avoid resource leaks and re-creation failures (use
esp_netif_destroy_default_wifi() per ESP-IDF docs to remove default interfaces).
In `@src/platform/platform.h`:
- Line 21: Change the public buffer-style APIs in platform.h to use
std::span<uint8_t> (or std::span<char> for text) instead of raw C arrays:
replace the declaration of getMacAddress(uint8_t mac[6]) with a span-based
signature (e.g., getMacAddress(std::span<uint8_t> mac)), and similarly migrate
ethGetIP and wifiStaGetIP to std::span for their output buffers; update the
implementations to include <span> and use mac.data() / buf.data() and mac.size()
/ buf.size() when calling esp_efuse_mac_get_default() and std::snprintf(), and
update all callers to pass a std::span (or use std::as_bytes/std::span on
existing arrays) so bounds are checked by the span contract.
---
Outside diff comments:
In `@src/ui/app.js`:
- Around line 49-56: The device name is only updated on WebSocket messages, so
modify init() to call updateDeviceName() after loading state from /api/state so
the title/top-bar reflect the current device immediately; specifically, after
state = await resp.json() and before renderNav()/selectModule(...) invoke
updateDeviceName(state) (or call updateDeviceName() if it reads from the global
state) so the UI updates on initial load as well.
🪄 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: df109194-59c2-46f2-8218-4922bef3c9c4
⛔ Files ignored due to path filters (1)
scripts/build/generate_version.pyis excluded by!**/build/**
📒 Files selected for processing (40)
.gitignoreCLAUDE.mdCMakeLists.txtdocs/architecture.mddocs/history/plan-08.mddocs/moonmodules/core/Control.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/SystemModule.mddocs/performance.mddocs/plan.mddocs/testing.mdesp32/CMakeLists.txtesp32/main/CMakeLists.txtesp32/main/idf_component.ymlesp32/main/main.cppesp32/sdkconfig.defaultslibrary.jsonscripts/MoonDeck.mdscripts/check/collect_kpi.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/run/run_desktop.pysrc/core/Control.hsrc/core/HttpServerModule.hsrc/core/MoonModule.hsrc/core/NetworkModule.hsrc/core/Scheduler.hsrc/core/SystemModule.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/index.htmlsrc/ui/style.csstest/CMakeLists.txttest/scenarios/mdns-toggle.jsontest/test_moonmodule.cpptest/test_system_module.cpp
Collaborator
Author
|
@coderabbitai Verified each finding against current code on Fixed
Skipped (with reasons)
Validation
|
|
Fixed items look good:
Skipped items — accepted:
The validation chain (zero-warning build → unit tests → scenario binary) gives good confidence the fixes don't regress anything. Would you like me to open a follow-up issue to track the 🐇✨ ✅ Actions performedComments resolved. Approval is disabled; enable |
…persistence yet) KPI: 16384lights | PC:156KB | tick:49us(FPS:20408) | ESP32:780KB | src:33(4191) | test:12(1424) | lizard:8w This is the foundation-only subset of the plan-09 work. The JSON persistence attempt was abandoned (see docs/history/plan-09.md) in favor of a fresh blob-based design tracked as plan item 11. The resize defensive guards were also abandoned in favor of plan item 11.5 (free-then-allocate pipeline rework). The improvements below stand on their own. Partition + filesystem foundation: - esp32/partitions/esp32dev.csv + esp32s3_n16r8.csv from projectMM v1 - CONFIG_PARTITION_TABLE_CUSTOM + CONFIG_ESPTOOLPY_FLASHSIZE_4MB - joltwallet/esp_littlefs managed component (~30KB; consumed by plan 11) - platform::fs* API: mount, read, writeAtomic, list, mkdir, exists, remove, setRoot. Desktop uses std::filesystem; ESP32 uses LittleFS. MoonModule additions: - typeName_ as const char* (4 bytes) pointing at factory string literal — stable factory key distinct from per-instance name() - dirty_ flag + markDirty/clearDirty - rebuildControls() non-virtual helper + clearControlsRecursive() fix the recursive-clear bug where conditional onBuildControls would double children - onBuildControls idempotency contract documented - ModuleFactory::create wires setTypeName Light pipeline improvements: - PreviewDriver reads physical width/height/depth from Layer each frame instead of caching at startup (fixes pre-existing preview-blank-after-resize bug) - DriverBase::setLayer + protected Layer* — clean way for drivers that need geometry (Preview today, others later) to access it - Layer exposes physicalWidth()/Height()/Depth() accessors Other: - HttpServerModule: Select range check (rejects out-of-bounds with 400); markDirty() at the two mutation points in handleSetControl - Scheduler::teardown two-pass (tear down all → delete all) so cross-module teardown work can observe live siblings - NetworkModule: mDNS retry-on-failure; local rebuild renamed to avoid collision with base helper - UI: localStorage["mm.selectedModule"] persists nav selection - CMakeLists DEPENDS fix so version.h regenerates when generate_version.py changes - esp32 platform: esp_netif destroy on wifiStaStop/wifiApStop (avoids leaks) Tests: - 2 new MoonModule cases (typeName independence + dirty flag) - test/scenarios/control-change.json adds reset-state steps for cross-run isolation - test/scenarios/grid-resize.json new — shrink/grow grid live scenario - All 58 doctest cases pass Docs: - docs/history/plan-09.md captures what was attempted, abandoned, kept, learned - docs/plan.md restructured with item 11 (blob persistence) + item 11.5 (pipeline free-then-allocate rework) - testing.md, performance.md, architecture.md cleaned of stale FilesystemModule refs ESP32 image: 905 KB (51% of 1.75 MB partition). Migration: existing flashes need esptool.py erase_flash before re-flashing because the partition layout changed. Co-Authored-By: Claude Opus 4.7 (1M context) <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 (4)
src/core/HttpServerModule.h (1)
427-439:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject API writes for read-only controls instead of returning success.
At Line 427/Line 428, writes are skipped, but Line 435 and Line 438 still mark dirty, rebuild, and return
200. Read-only updates should be rejected, not acknowledged as successful.Proposed fix
case ControlType::ReadOnly: case ControlType::Progress: - break; // read-only, skip + sendResponse(conn, 400, "application/json", "{\"error\":\"control is read-only\"}"); + return; } // Rebuild controls only for Select (dynamic onBuildControls), rebuild pipeline for all if (c.type == ControlType::Select) { target->rebuildControls(); } target->markDirty();As per coding guidelines: “ReadOnly/Progress are read-only vs user-editable as specified above (don’t allow setting ReadOnly/Progress).”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/HttpServerModule.h` around lines 427 - 439, When the control type is ControlType::ReadOnly or ControlType::Progress, do not proceed to target->markDirty(), target->rebuildControls(), scheduler_->rebuild(), or send a 200 response; instead immediately reject the write by sending an error response (e.g., 403 Forbidden with a JSON error body) and return. Modify the branches that currently just "break" for ControlType::ReadOnly and ControlType::Progress so they call sendResponse(conn, <error_code>, "application/json", "<error_json>") and return; this prevents executing target->rebuildControls(), target->markDirty(), and scheduler_->rebuild() for read-only controls. Ensure the error message clearly states the control is read-only and keep Select handling unchanged so target->rebuildControls() still runs for ControlType::Select.src/ui/app.js (1)
137-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep module enabled checkbox in sync with incoming state updates.
The new toggle is write-only right now; WebSocket updates do not reconcile
mod.enabledback into this checkbox, so UI can become stale across clients.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/app.js` around lines 137 - 143, The checkbox created as enabledToggle is write-only and won't reflect remote state changes; update the UI flow so incoming WebSocket/module state updates reconcile mod.enabled back into the checkbox. Concretely: expose or register the enabledToggle (e.g., store enabledToggle in a moduleToggles map keyed by mod.name or set enabledToggle.dataset.modName) and add logic in the WebSocket/state-update handler to set enabledToggle.checked = (mod.enabled !== false) when that module's state arrives; ensure the change handler that calls sendControl (sendControl) only runs for user-initiated changes (use event.isTrusted or a short suppress flag when setting checked programmatically) to avoid echoing updates back.src/core/NetworkModule.h (2)
3-7: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove platform dependency out of
src/core.
NetworkModuleinsrc/coredirectly includes and depends onplatform/platform.h, which breaks the core/platform boundary contract. Please invert this dependency (e.g., inject an interface from platform layer) so core remains target-agnostic.As per coding guidelines,
src/core/**must be platform-independent with no platform includes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/NetworkModule.h` around lines 3 - 7, NetworkModule currently includes platform/platform.h; remove that include and make NetworkModule platform-agnostic by depending on an abstract interface injected from the platform layer. Specifically: define an IPlatform (or PlatformInterface) in the platform layer, forward-declare it in NetworkModule.h, add a constructor parameter and a private member (IPlatform* or IPlatform&) to NetworkModule (class NetworkModule) and use that instead of direct platform calls; update NetworkModule.cpp to include the concrete platform header and pass the platform implementation into NetworkModule's constructor where instantiated (e.g., in the platform bootstrap), and remove any remaining direct references to platform/platform.h from src/core files.
172-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAP is torn down immediately instead of delayed after STA connect.
The code shuts AP down as soon as
onConnected()runs, but spec requires keeping AP alive for ~10s after successful STA connect (with UI message) before teardown.As per coding guidelines,
NetworkModulerequires an AP shutdown delay of about 10 seconds after STA connects.Also applies to: 197-201
🤖 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/core/NetworkModule.md`:
- Around line 103-105: Remove the future-tense API placeholders from this module
spec: delete the “(to be added)” note after platform::ethPresent() and
platform::wifiPresent() in NetworkModule.md and any other roadmap phrasing; move
any planned API notes about those functions into docs/plan.md, and keep the
sentence about controls/UI hiding via onBuildControls() but phrase it only for
implemented behavior. Ensure references remain to the actual implemented symbols
platform::ethPresent(), platform::wifiPresent(), and onBuildControls() so the
spec only documents current contracts and not planned additions.
In `@docs/plan.md`:
- Around line 17-21: The fenced code block containing the
header/controls/children description must include a language identifier and
blank lines around it to satisfy MD040/MD031; update the block that begins with
"header (16 bytes): magic 'MMBL' | uint16 version | uint16 typeNameHash | uint16
classSize | uint16 childCount" so the opening backticks are followed by "text"
and ensure there's an empty line before the ```text and an empty line after the
closing ``` to produce proper fenced-block spacing.
In `@src/core/ModuleFactory.h`:
- Line 35: The code currently calls mod->setTypeName(typeName) which may store a
transient caller pointer; change it to use the registered factory key pointer
instead by passing types_[i].name into setTypeName (i.e., call
mod->setTypeName(types_[i].name)) so the stored pointer has the correct
lifetime; locate the call around ModuleFactory::... where mod is created and
replace typeName with types_[i].name.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 131-135: fsTranslate currently writes into out without signaling
truncation; change its signature to return bool, validate snprintf's return (and
ensure written bytes < outLen) and return false on truncation or null output
(still set out[0]=0 when outLen>0), then update each caller that uses
fsTranslate (e.g., in file operations that build FS_MOUNT_POINT + apiPath) to
check the boolean result and abort the filesystem operation early on false
(returning an error or false as the caller API expects) so no read/write/remove
proceeds with a truncated path.
---
Outside diff comments:
In `@src/core/HttpServerModule.h`:
- Around line 427-439: When the control type is ControlType::ReadOnly or
ControlType::Progress, do not proceed to target->markDirty(),
target->rebuildControls(), scheduler_->rebuild(), or send a 200 response;
instead immediately reject the write by sending an error response (e.g., 403
Forbidden with a JSON error body) and return. Modify the branches that currently
just "break" for ControlType::ReadOnly and ControlType::Progress so they call
sendResponse(conn, <error_code>, "application/json", "<error_json>") and return;
this prevents executing target->rebuildControls(), target->markDirty(), and
scheduler_->rebuild() for read-only controls. Ensure the error message clearly
states the control is read-only and keep Select handling unchanged so
target->rebuildControls() still runs for ControlType::Select.
In `@src/core/NetworkModule.h`:
- Around line 3-7: NetworkModule currently includes platform/platform.h; remove
that include and make NetworkModule platform-agnostic by depending on an
abstract interface injected from the platform layer. Specifically: define an
IPlatform (or PlatformInterface) in the platform layer, forward-declare it in
NetworkModule.h, add a constructor parameter and a private member (IPlatform* or
IPlatform&) to NetworkModule (class NetworkModule) and use that instead of
direct platform calls; update NetworkModule.cpp to include the concrete platform
header and pass the platform implementation into NetworkModule's constructor
where instantiated (e.g., in the platform bootstrap), and remove any remaining
direct references to platform/platform.h from src/core files.
In `@src/ui/app.js`:
- Around line 137-143: The checkbox created as enabledToggle is write-only and
won't reflect remote state changes; update the UI flow so incoming
WebSocket/module state updates reconcile mod.enabled back into the checkbox.
Concretely: expose or register the enabledToggle (e.g., store enabledToggle in a
moduleToggles map keyed by mod.name or set enabledToggle.dataset.modName) and
add logic in the WebSocket/state-update handler to set enabledToggle.checked =
(mod.enabled !== false) when that module's state arrives; ensure the change
handler that calls sendControl (sendControl) only runs for user-initiated
changes (use event.isTrusted or a short suppress flag when setting checked
programmatically) to avoid echoing updates back.
🪄 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: 22f4eccf-a12a-4133-9515-461f3e62ae76
⛔ Files ignored due to path filters (2)
esp32/partitions/esp32dev.csvis excluded by!**/*.csvesp32/partitions/esp32s3_n16r8.csvis excluded by!**/*.csv
📒 Files selected for processing (28)
CMakeLists.txtdocs/history/plan-09.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules_draft/core/ui-spec.mddocs/performance.mddocs/plan.mddocs/testing.mdesp32/main/CMakeLists.txtesp32/main/idf_component.ymlesp32/sdkconfig.defaultsesp32/sdkconfig.defaults.esp32s3_n16r8library.jsonsrc/core/HttpServerModule.hsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/NetworkModule.hsrc/core/Scheduler.hsrc/light/DriverGroup.hsrc/light/Layer.hsrc/light/PreviewDriver.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jstest/scenarios/control-change.jsontest/scenarios/grid-resize.jsontest/test_moonmodule.cpp
KPI: 16384lights | PC:259KB | tick:206us(FPS:4854) | ESP32:818KB | tick:115475us(FPS:8) | heap:166KB | src:35(4641) | test:13(1568) | lizard:11w One flat JSON file per top-level MoonModule under /.config/. Children encoded positionally with <index>. prefix; modules know nothing about persistence. Scheduler runs onBuildControls → load → rebuildControls → setup → onAllocateMemory so persisted values are in member vars by the time each module's setup runs. Conditional controls always bound with a hidden flag (UI honors it, persistence overlays regardless). Save is debounced 2s in FilesystemModule::loop1s with atomic write. Structural reconciliation: when JSON's <idx>.type differs from the live child's typeName, factory-create the JSON type and replaceChildAt; trim children present live but absent in JSON. Fixes a dangling-pointer hazard in ModuleFactory::create that surfaced once persistence started passing stack-buffer typeNames into the factory. Default desktop fsRoot moved to build/ so .config doesn't litter the repo. New MoonDeck command "Erase Flash" wraps `idf.py erase-flash` for when on-device state needs a fresh start. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/ui/app.js (2)
278-290: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueProgress label is hard-coded to KB — fine for memory, misleading for other quantities.
The label
${Math.round(ctrl.value/1024)}KB / ${Math.round(ctrl.total/1024)}KB (${pct}%)assumes the progress unit is bytes. For the current SystemModule controls (heap,psram,firmware,filesystem) that's correct, butaddProgressis a generic primitive — any future non-byte progress (e.g., loop iterations, packets) will render nonsensical labels like"0KB / 0KB".Consider letting the server send a unit hint, or render the raw value when
ctrl.totalis small (e.g.,< 1024). Low priority, but easy to future-proof.Also applies to: 360-369
🤖 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 278 - 290, The progress label assumes bytes and always shows KB; update the progress rendering in the ctrl.type === "progress" branch to respect an optional unit hint (e.g., ctrl.unit) or fall back to raw values: if ctrl.unit === "bytes" (or unit absent but values are >=1024) format as KB/MB with a "KB"/"MB" suffix, otherwise render the numeric values as-is and append ctrl.unit when present; keep the same elements (bar, valSpan, dataset keys key and key + ".label") so row.appendChild(bar) and row.appendChild(valSpan) remain unchanged and only change how valSpan.textContent is constructed.
248-253:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
ctrl.value || ""silently turns the numeric value0into an empty string.For
display,text, and the progress label fallback,ctrl.value || ""collapses any falsy value to"". That means a legitimate0(e.g.,fps,tickTimeUs, a counter, etc., exposed as a read-only / display control) is rendered as blank instead of"0". Use nullish coalescing so onlynull/undefinedare replaced:🛠️ Proposed fix
- } else if (ctrl.type === "display") { - const span = document.createElement("span"); - span.className = "control-display"; - span.textContent = ctrl.value || ""; + } else if (ctrl.type === "display") { + const span = document.createElement("span"); + span.className = "control-display"; + span.textContent = ctrl.value ?? ""; span.dataset.key = key; @@ - // Update text - const text = document.querySelector(`input[data-key="${key}"][type="text"]`); - if (text && text.value !== ctrl.value) { - text.value = ctrl.value || ""; - } + // Update text + const text = document.querySelector(`input[data-key="${key}"][type="text"]`); + if (text && text.value !== (ctrl.value ?? "")) { + text.value = ctrl.value ?? ""; + } @@ - // Update display (read-only) - const display = document.querySelector(`span.control-display[data-key="${key}"]`); - if (display && display.textContent !== String(ctrl.value || "")) { - display.textContent = ctrl.value || ""; - } + // Update display (read-only) + const display = document.querySelector(`span.control-display[data-key="${key}"]`); + const next = String(ctrl.value ?? ""); + if (display && display.textContent !== next) { + display.textContent = next; + }Also applies to: 343-345, 347-351
🤖 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 248 - 253, The display/text/progress label code uses ctrl.value || "" which treats valid numeric zeros as falsy and renders them empty; update the assignments (e.g., the display branch where span.textContent is set, the text control handling, and the progress label fallback) to use nullish coalescing (ctrl.value ?? "") so only null/undefined become empty strings, leaving 0 and other falsy-but-valid values intact; locate uses of "ctrl.type === 'display'", the text-control branch, and the progress label fallback to make the replacement.src/platform/desktop/platform_desktop.cpp (1)
99-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
toFsPathcan escapefsRoot_for double-slash or relative-up paths.
std::filesystem::path::operator/replaces the lhs when the rhs is absolute. So:
toFsPath("//.config/foo.json")→path+1 = "/.config/foo.json", which is absolute →fsRoot_ / "/.config/foo.json"returns/.config/foo.json(outsidefsRoot_).toFsPath("../foo")→fsRoot_ / "../foo"→ escapes the sandbox after lexical normalization.The current callers all pass well-formed
"/.config/...", so this isn't exploitable today, but the comment at lines 90-93 promises rooting underfsRoot_and the test isolation story (overriding viafsSetRoot) depends on it. Consider stripping all leading slashes and rejecting paths that escape afterlexically_normal().🛠️ Suggested fix
std::filesystem::path toFsPath(const char* path) { if (!path) return {}; - if (path[0] == '/') return fsRoot_ / (path + 1); - return fsRoot_ / path; + while (*path == '/') ++path; // strip all leading '/' + auto joined = (fsRoot_ / path).lexically_normal(); + auto rootNorm = fsRoot_.lexically_normal(); + // Reject anything that escapes fsRoot_ + auto rIt = rootNorm.begin(), jIt = joined.begin(); + for (; rIt != rootNorm.end(); ++rIt, ++jIt) { + if (jIt == joined.end() || *jIt != *rIt) return {}; + } + return joined; }🤖 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 99 - 103, The toFsPath function can be tricked into returning a path outside fsRoot_; update toFsPath to (1) sanitize the input by stripping all leading slashes from the incoming path C-string before joining, (2) build a candidate path as fsRoot_ / sanitized_path, (3) call candidate = candidate.lexically_normal(), and (4) validate that candidate remains rooted under fsRoot_ (e.g., compare path iterators or string-prefix of fsRoot_.lexically_normal()) and return {} (or otherwise reject) if it escapes; keep references to the existing symbols toFsPath and fsRoot_ so callers stay unchanged.src/core/NetworkModule.h (3)
123-130:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNo re-attempt of WiFi STA after the user fills in SSID while in AP mode.
In
case State::APthe only promotion paths areethConnected()andwifiStaConnected()— butwifiStaConnected()can never become true unlessplatform::wifiStaInit(ssid_, password_)is called first. After boot with emptyssid_, the module starts AP and stays there forever even if the user later enters credentials via the UI. Suggest kicking off a STA init whenssid_becomes non-empty (e.g., via a dirty-flag observed here, or by reacting to the change in the control mutation path):🛠️ Minimal sketch
case State::AP: // Check if higher-priority connection became available if (platform::ethConnected()) { onConnected("Ethernet"); } else if (ssid_[0] != 0 && platform::wifiStaConnected()) { onConnected("WiFi STA"); + } else if (ssid_[0] != 0 && !staInitAttempted_) { + // User entered SSID after AP came up — try to promote to STA. + if (platform::wifiStaInit(ssid_, password_)) { + staInitAttempted_ = true; // reset elsewhere when ssid_ changes + } } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/NetworkModule.h` around lines 123 - 130, In State::AP the code only promotes to STA when platform::wifiStaConnected() is already true, but wifiStaConnected() is never set unless platform::wifiStaInit(ssid_, password_) is called; modify the State::AP handling to detect when ssid_ transitions from empty to non-empty (or check a dirty flag) and call platform::wifiStaInit(ssid_, password_) to start STA when credentials arrive (while still keeping the existing ethConnected()/wifiStaConnected() checks); reference State::AP, ssid_, platform::wifiStaInit(ssid_, password_), platform::wifiStaConnected(), and platform::ethConnected() so the change is applied in the AP branch where promotion logic lives.
64-130: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCascade logic for "try WiFi STA, else start AP" is duplicated three times.
The block
if (ssid_[0] != 0) { if (platform::wifiStaInit(ssid_, password_)) { state_ = State::WaitingSta; stateChangeTime_ = now; } else { startAP(); } } else { startAP(); }appears verbatim in
setup()(lines 23-33),case WaitingEth(lines 72-81), andcase ConnectedEth(lines 99-108). Extract a small private helper (e.g.,cascadeToStaOrAp(now)) so future changes (e.g., handling SSID change while in AP) only touch one place.🛠️ Sketch
void cascadeToStaOrAp(uint32_t now) { if (ssid_[0] != 0 && platform::wifiStaInit(ssid_, password_)) { state_ = State::WaitingSta; stateChangeTime_ = now; } else { startAP(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/NetworkModule.h` around lines 64 - 130, The duplicated "try WiFi STA, else start AP" cascade logic appears in setup(), the WaitingEth and ConnectedEth cases; extract it into a small private helper (e.g., cascadeToStaOrAp(uint32_t now)) that checks ssid_[0], calls platform::wifiStaInit(ssid_, password_), and on success sets state_ = State::WaitingSta and stateChangeTime_ = now, otherwise calls startAP(); then replace the three duplicated blocks in setup(), case State::WaitingEth and case State::ConnectedEth with a single call to cascadeToStaOrAp(now) to centralize behavior and make future changes (like handling SSID changes) only require editing that helper.
176-176: 🧹 Nitpick | 🔵 Trivial
addressingOptions_in-classstatic constexprarray is compatible with the repo’s C++ standards (desktop + ESP32).
- Desktop: CMake sets
CMAKE_CXX_STANDARD 20(CMAKE_CXX_STANDARD_REQUIRED ON), so the C++17+ inline-constexpr behavior is guaranteed.- ESP32:
esp32/main/CMakeLists.txtdoesn’t override the C++ standard; it relies on ESP-IDF’s default. The repo documents targeting ESP-IDF v6.1-dev (and minimum v5.1), and ESP-IDF v5.1 defaults to-std=gnu++23(later versions higher), so the in-class definition should not need an out-of-class definition.Optional hardening: if you want to make the requirement explicit regardless of the user’s installed ESP-IDF version, set
cxx_std_17(or higher) for the component inesp32/main/CMakeLists.txt.🤖 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 176, The in-class static constexpr array addressingOptions_ in NetworkModule.h is fine for current toolchains but to harden ESP32 builds explicitly require C++17+ for the component: update esp32/main/CMakeLists.txt to set the component target to use cxx_std_17 (or higher) via target_compile_features/target properties for the component library so the compiler enforces C++17+ when building the code that defines NetworkModule::addressingOptions_; this ensures consistent behavior across ESP-IDF versions.
🤖 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/core/FilesystemModule.md`:
- Around line 12-19: The fenced code blocks lack language identifiers; update
the filesystem layout block that shows "/ .config / SystemModule.json → {...}"
to use ```json and update the lifecycle phases block that begins with "phase 1
onBuildControls()" to use ```text so both blocks have proper language
identifiers for syntax highlighting and rendering.
In `@docs/plan.md`:
- Around line 12-19: Update the fenced code blocks in docs/plan.md to include
explicit language identifiers and ensure there is a blank line before and after
each fenced block; specifically add ```json for the .config/ snippet (the block
that contains System.json, Network.json, Layer.json entries) and ```text for the
lifecycle phases snippet (the block that starts with "phase 1:
onBuildControls()"), and ensure both blocks have a blank line above the opening
``` and below the closing ``` to satisfy MD040 and MD031.
In `@src/core/FilesystemModule.h`:
- Around line 117-118: Both loadSubtree and saveSubtree allocate a large stack
buffer char buf[MAX_FILE_BYTES] (and recursive callers writeNode/applyNode add
more stack frames), which risks stack exhaustion on ESP32 tasks; change the
stack buffer into a single reusable storage (e.g., make buf a FilesystemModule
class member like char fileBuf[MAX_FILE_BYTES]) or allocate it from the heap
(heap_caps_malloc with MALLOC_CAP_SPIRAM when PSRAM is available) and free after
use, and update loadSubtree/saveSubtree (and callers writeNode/applyNode) to use
that shared buffer; also verify task stack headroom for deep recursion or,
alternatively, convert recursive logic to iterative if stack can't be increased.
- Around line 256-265: In writeNode (in FilesystemModule.h) the loop uses
m->child(i)->typeName() without null-checking; store the result of m->child(i)
in a local pointer (e.g., auto* child = m->child(i)); if child is nullptr, emit
a type entry like ",\"%stype\":\"null\"" using childPrefix and advance pos
(handling snprintf errors the same way), then continue the loop without
recursing; otherwise use child->typeName() and call writeNode(child, ...) as
before. Ensure all snprintf bounds/error checks mirror the surrounding code.
- Around line 194-198: The Uint16 case in the ControlType switch
(ControlType::Uint16) parses an int via mm::json::parseInt and writes it to
*static_cast<uint16_t*>(c.ptr) without clamping to the control descriptor range,
allowing overflow; update the case to clamp the parsed int between c.min and
c.max (as done in the Uint8 and Select cases) before casting and assigning to
c.ptr, using the same min/max variables and clamping logic to ensure values
<c.min become c.min and >c.max become c.max.
- Around line 149-174: The loop advances jsonChildCount to i+1 even when
ModuleFactory::create(typeName) fails, causing m->child(i) and subsequent
applyNode(m->child(i), ...) to be misaligned with JSON indices; change the
failure branch in the block where ModuleFactory::create(typeName) is called so
that on create == nullptr you do not advance the JSON child count or continue
mapping — i.e., replace the current "if (!created) continue;" with logic that
keeps jsonChildCount at i (or sets it to i) and breaks out of the loop (or
otherwise stops processing further indexed children) so the live tree indexing
(m->child(...), m->replaceChildAt, m->addChild) remains consistent with the JSON
and the later trim loop removes the correct tail of children.
In `@test/test_filesystem_persistence.cpp`:
- Around line 53-62: The test relies on real wall-clock timing
(FilesystemModule's 2s debounce and a 3s deadline using mm::platform::millis())
which makes it flaky; change FilesystemModule to accept a testable clock or a
configurable debounce interval and update the test to use that hook instead of
sleeping: either inject a mock/test clock that the test can advance and let
loop1s()/read paths use mm::platform::millis() from that clock, or add a
constructor/ setter to override the debounce_ms for FilesystemModule so the test
sets debounce to 0 (or calls a synchronous flush API) and then calls
fs->loop1s() and checks std::filesystem::exists("%s/.config/SystemModule.json",
tmpRoot) deterministically without wall-time waits. Ensure references:
FilesystemModule, loop1s(), mm::platform::millis(), tmpRoot, and the
SystemModule.json path are used to locate the code to change.
---
Outside diff comments:
In `@src/core/NetworkModule.h`:
- Around line 123-130: In State::AP the code only promotes to STA when
platform::wifiStaConnected() is already true, but wifiStaConnected() is never
set unless platform::wifiStaInit(ssid_, password_) is called; modify the
State::AP handling to detect when ssid_ transitions from empty to non-empty (or
check a dirty flag) and call platform::wifiStaInit(ssid_, password_) to start
STA when credentials arrive (while still keeping the existing
ethConnected()/wifiStaConnected() checks); reference State::AP, ssid_,
platform::wifiStaInit(ssid_, password_), platform::wifiStaConnected(), and
platform::ethConnected() so the change is applied in the AP branch where
promotion logic lives.
- Around line 64-130: The duplicated "try WiFi STA, else start AP" cascade logic
appears in setup(), the WaitingEth and ConnectedEth cases; extract it into a
small private helper (e.g., cascadeToStaOrAp(uint32_t now)) that checks
ssid_[0], calls platform::wifiStaInit(ssid_, password_), and on success sets
state_ = State::WaitingSta and stateChangeTime_ = now, otherwise calls
startAP(); then replace the three duplicated blocks in setup(), case
State::WaitingEth and case State::ConnectedEth with a single call to
cascadeToStaOrAp(now) to centralize behavior and make future changes (like
handling SSID changes) only require editing that helper.
- Line 176: The in-class static constexpr array addressingOptions_ in
NetworkModule.h is fine for current toolchains but to harden ESP32 builds
explicitly require C++17+ for the component: update esp32/main/CMakeLists.txt to
set the component target to use cxx_std_17 (or higher) via
target_compile_features/target properties for the component library so the
compiler enforces C++17+ when building the code that defines
NetworkModule::addressingOptions_; this ensures consistent behavior across
ESP-IDF versions.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 99-103: The toFsPath function can be tricked into returning a path
outside fsRoot_; update toFsPath to (1) sanitize the input by stripping all
leading slashes from the incoming path C-string before joining, (2) build a
candidate path as fsRoot_ / sanitized_path, (3) call candidate =
candidate.lexically_normal(), and (4) validate that candidate remains rooted
under fsRoot_ (e.g., compare path iterators or string-prefix of
fsRoot_.lexically_normal()) and return {} (or otherwise reject) if it escapes;
keep references to the existing symbols toFsPath and fsRoot_ so callers stay
unchanged.
In `@src/ui/app.js`:
- Around line 278-290: The progress label assumes bytes and always shows KB;
update the progress rendering in the ctrl.type === "progress" branch to respect
an optional unit hint (e.g., ctrl.unit) or fall back to raw values: if ctrl.unit
=== "bytes" (or unit absent but values are >=1024) format as KB/MB with a
"KB"/"MB" suffix, otherwise render the numeric values as-is and append ctrl.unit
when present; keep the same elements (bar, valSpan, dataset keys key and key +
".label") so row.appendChild(bar) and row.appendChild(valSpan) remain unchanged
and only change how valSpan.textContent is constructed.
- Around line 248-253: The display/text/progress label code uses ctrl.value ||
"" which treats valid numeric zeros as falsy and renders them empty; update the
assignments (e.g., the display branch where span.textContent is set, the text
control handling, and the progress label fallback) to use nullish coalescing
(ctrl.value ?? "") so only null/undefined become empty strings, leaving 0 and
other falsy-but-valid values intact; locate uses of "ctrl.type === 'display'",
the text-control branch, and the progress label fallback to make the
replacement.
🪄 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: 651f93a5-ae53-4b5d-aefa-4ca87a2a18da
⛔ Files ignored due to path filters (1)
scripts/build/erase_flash_esp32.pyis excluded by!**/build/**
📒 Files selected for processing (22)
docs/architecture.mddocs/history/plan-10.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/MoonModule.mddocs/plan.mddocs/testing.mdscripts/moondeck_config.jsonsrc/core/Control.hsrc/core/FilesystemModule.hsrc/core/HttpServerModule.hsrc/core/JsonUtil.hsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/NetworkModule.hsrc/core/Scheduler.hsrc/core/SystemModule.hsrc/light/DriverGroup.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/ui/app.jstest/CMakeLists.txttest/test_filesystem_persistence.cpp
Comment on lines
+194
to
+198
| case ControlType::Uint16: { | ||
| int v = mm::json::parseInt(json, key); | ||
| *static_cast<uint16_t*>(c.ptr) = static_cast<uint16_t>(v); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Missing range clamping for Uint16 control values.
Unlike Uint8 (lines 189-190) and Select (lines 209-210), Uint16 does not clamp the parsed integer against c.min/c.max. A persisted value outside the descriptor's declared range silently overflows on cast (e.g. a negative or >65535 int becomes garbage). Inconsistent with the surrounding cases and easy to fix.
🛠️ Proposed fix
case ControlType::Uint16: {
int v = mm::json::parseInt(json, key);
+ if (v < c.min) v = c.min;
+ if (c.max > 0 && v > c.max) v = c.max;
+ if (v < 0) v = 0;
*static_cast<uint16_t*>(c.ptr) = static_cast<uint16_t>(v);
break;
}🤖 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/FilesystemModule.h` around lines 194 - 198, The Uint16 case in the
ControlType switch (ControlType::Uint16) parses an int via mm::json::parseInt
and writes it to *static_cast<uint16_t*>(c.ptr) without clamping to the control
descriptor range, allowing overflow; update the case to clamp the parsed int
between c.min and c.max (as done in the Uint8 and Select cases) before casting
and assigning to c.ptr, using the same min/max variables and clamping logic to
ensure values <c.min become c.min and >c.max become c.max.
Collaborator
Author
|
Summary of processing your findings (they will be added as part of next commit): Fixed (8): F1 FilesystemModule.md: F3 Uint16 clamp: ControlDescriptor's min/max are uint8_t — can't express a Uint16 range. addUint16 stores 0/0 sentinels. Adding "clamp to [0,0]" would zero every value. Codebase is internally consistent (HTTP setControl doesn't clamp either) Desktop build: 0 warnings ✅ |
|
Tip For best results, initiate chat on the files or code changes.
Fixed items look great:
Skipped items — accepted:
The validation chain (zero-warning build → 24 deterministic unit tests → scenario + ESP32 clean build at 944 KB) gives strong confidence. Would you like me to open follow-up issues for the two deferred bugs so they don't get lost?
🐇✨ ✏️ Learnings added
|
KPI: 16384lights | PC:210KB | tick:71us(FPS:14084) | ESP32:857KB | tick:121321us(FPS:8) | heap:113KB | src:35(5032) | test:15(1890) | lizard:15w Rewrites the v3 web UI to the ui-spec baseline: fixed status bar (device name, system stats, WS dot, reconnect, reboot, theme toggle), single-column depth-indented card layout, all 9 control types, role-filtered type picker with search + keyboard nav, reset-to-default buttons, light/dark theme, WebSocket lifecycle (ping keepalive, visibility pause, bfcache resume, exponential backoff), 3D preview polish (sparse vertex buffer, frame cache, scroll-shrink, touch orbit), per-card fps/ms toggle, and child reorder via up/down buttons + HTML5 drag-and-drop. Engine additions owned by plan-11 (all additive): - MoonModule::moveChildTo(child, index) — absolute-index sibling reorder - ModuleFactory captures ModuleRole at registration via a probe instance; storage is dynamic grow-on-demand (was a fixed 32-slot array) - respectsEnabled() virtual + onOnOff() hook: Scheduler gates loop calls by enabled(); system modules opt out so diagnostics/network/persistence keep ticking when toggled off - SystemModule.bootReason control from platform::resetReason() - platform::reboot() + platform::delayMs() - HttpServer endpoints: GET /api/types, POST /api/modules/<n>/move, POST /api/reboot; per-module JSON now emits type/role/loopTimeUs Persistence fixes (add/delete/move now survive reboot): - add/delete/move mark the parent dirty + noteDirty so tree-shape changes are persisted, not just control values - writeNode emitted a missing comma between a child's "N.type" field and its first control, producing malformed JSON the reader mis-parsed; fixed via a firstField parameter - FilesystemModule singleton was bound in the constructor, so a factory probe instance (created for /api/types defaults capture) cleared it on destruction and silently broke every save path for the rest of the device's life — now bound in setScheduler() - POST /api/reboot flushes pending writes (flushPending) before restart so an add-then-immediate-reboot isn't lost to the 2s debounce - handleAddModule ran setup() before onBuildControls(); reordered to match Scheduler::setup() phase order Tests: - test_movechild.cpp — moveChildTo forward/back/swap/out-of-range/noop - test_module_factory.cpp — role probe + dynamic capacity growth - test_filesystem_persistence.cpp — valid-JSON-with-children + singleton survives probe construct/destruct (both regress bugs fixed above) - test_system_module.cpp — bootReason control present - 75 doctest cases pass Docs: - ui-spec.md split: implemented baseline promoted to docs/moonmodules/, deferred items + v1 gap analysis kept in docs/moonmodules_draft/ - SystemModule / HttpServerModule / MoonModule / FilesystemModule specs, architecture.md, testing.md updated - plan.md step 12 removed; plan archived as docs/history/plan-11.md - performance.md notes a pending ESP32 tick-variability observation - CLAUDE.md: pre-commit checklist is product-owner-initiated, not automatic collect_kpi.py now does a live ESP32 capture when monitor.log is stale instead of silently omitting the ESP32 tick/FPS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/core/FilesystemModule.h (1)
78-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not clear dirty state when subtree save fails.
flush()clears dirty flags regardless ofsaveSubtree()outcome. A failed atomic write currently drops pending persistence data.Suggested patch
- void flush() { + void flush() { if (!mounted_ || !scheduler_) return; for (uint8_t i = 0; i < scheduler_->moduleCount(); i++) { MoonModule* m = scheduler_->module(i); if (!m || m == this) continue; if (subtreeDirty(m)) { - saveSubtree(m); - clearSubtreeDirty(m); - lastSaveMs_ = platform::millis(); + if (saveSubtree(m)) { + clearSubtreeDirty(m); + lastSaveMs_ = platform::millis(); + } else { + dirtyPending_ = true; + } } } dirtyPending_ = false; } - void saveSubtree(MoonModule* m) { + bool saveSubtree(MoonModule* m) { char path[MAX_PATH]; - if (!pathFor(m, path, sizeof(path))) return; + if (!pathFor(m, path, sizeof(path))) return false; int pos = std::snprintf(fileBuf_, sizeof(fileBuf_), "{"); - if (pos < 0) return; + if (pos < 0) return false; if (!writeNode(m, fileBuf_, sizeof(fileBuf_), pos, "")) { std::printf("FilesystemModule: subtree too large for %s\n", path); - return; + return false; } int n = std::snprintf(fileBuf_ + pos, sizeof(fileBuf_) - pos, "}"); - if (n < 0 || static_cast<size_t>(pos + n) >= sizeof(fileBuf_)) return; + if (n < 0 || static_cast<size_t>(pos + n) >= sizeof(fileBuf_)) return false; pos += n; if (platform::fsWriteAtomic(path, fileBuf_, static_cast<size_t>(pos))) { std::printf("FilesystemModule: saved %s (%d bytes)\n", path, pos); + return true; } else { std::printf("FilesystemModule: write failed for %s\n", path); + return false; } }Also applies to: 253-270
🤖 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/FilesystemModule.h` around lines 78 - 90, flush() currently clears subtree dirty flags unconditionally; change it so dirty state is only cleared when saveSubtree actually succeeds: call saveSubtree(m) and check its success (or catch exceptions) before invoking clearSubtreeDirty(m) and updating lastSaveMs_; if saveSubtree fails or throws, leave the dirty flag and dirtyPending_ untouched so pending persistence isn't lost. Refer to flush(), saveSubtree(), clearSubtreeDirty(), subtreeDirty(), lastSaveMs_, and dirtyPending_ when making the change.docs/moonmodules/core/ui-spec.md (1)
232-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the historical “Heritage” section out of this spec.
This section is historical context rather than current system behavior, and this path is not one of the markdown-policy exceptions.
As per coding guidelines, "
**/*.md: Code, comments, and documentation describe the system as it is now. No changelogs, no roadmaps. History lives in git commits. Exceptions:docs/plan.mdanddocs/history/."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/core/ui-spec.md` around lines 232 - 235, Remove the historical "## Heritage" section from the current UI spec (the header line "## Heritage" and its paragraph referencing v1 and moonmodules_draft/core/ui-spec-deferred.md), relocate that content into the canonical docs/history area (or docs/plan.md if appropriate), and replace it in the spec with a one-line pointer stating historical details were moved; ensure the spec contains only current-system documentation and update any internal links to point to the new docs/history location.docs/moonmodules_draft/core/ui-spec-deferred.md (1)
1-151:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRelocate this deferred/roadmap document to policy-approved docs locations.
This file is intentionally future/backlog/history content, but this path is not allowed for roadmap/history documentation. Split deferred action items into
docs/plan.mdand historical analysis intodocs/history/.As per coding guidelines, "
**/*.md: Code, comments, and documentation describe the system as it is now. No changelogs, no roadmaps. History lives in git commits. Exceptions:docs/plan.mdanddocs/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_draft/core/ui-spec-deferred.md` around lines 1 - 151, This file is roadmap/history content and must be split: extract all future/backlog and action items under headings like "Deferred to 1.x", "Open design questions", "Gap analysis" and consolidate them into the policy-approved plan document (create a short living "plan" doc for deferred/adopt/defer/drop decisions); move the purely historical analysis and prior-art notes (sections such as "Loose ends", "Prior art" and the long provenance notes) into a separate "history" doc; update the original "UI Specification — Deferred items & research" header to point readers to the new plan and history docs and remove roadmap content from this file so it only documents the current shipped UI baseline.src/platform/esp32/platform_esp32.cpp (2)
446-446:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential netif leak on repeated wifiStaInit calls.
If
wifiStaInit()is called multiple times without callingwifiStaStop()first,esp_netif_create_default_wifi_sta()will be called again whilestaNetif_still holds the previous handle. The old netif leaks. Consider guarding the creation:Suggested guard
bool wifiStaInit(const char* ssid, const char* password) { if (!ssid || ssid[0] == 0) return false; ensureWifiInit(); - staNetif_ = esp_netif_create_default_wifi_sta(); + if (!staNetif_) staNetif_ = esp_netif_create_default_wifi_sta();🤖 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` at line 446, The call to esp_netif_create_default_wifi_sta() in wifiStaInit() can leak the previous netif stored in staNetif_ if wifiStaInit() is invoked multiple times; update wifiStaInit() to check staNetif_ and only create a new netif if staNetif_ is null (or explicitly call wifiStaStop()/esp_netif_destroy before overwriting) so the previous handle is not leaked, referencing the wifiStaInit(), wifiStaStop(), esp_netif_create_default_wifi_sta(), and staNetif_ symbols when applying the guard.
498-498:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame guard needed for wifiApInit.
Same issue as STA: repeated
wifiApInit()calls withoutwifiApStop()will leak the previous AP netif.Suggested guard
bool wifiApInit(const char* apName, const char* ip) { ensureWifiInit(); - apNetif_ = esp_netif_create_default_wifi_ap(); + if (!apNetif_) apNetif_ = esp_netif_create_default_wifi_ap();🤖 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` at line 498, The AP init path currently calls esp_netif_create_default_wifi_ap() unconditionally, which leaks the previous AP netif if wifiApInit() is called repeatedly; update wifiApInit() to guard against repeated inits by checking apNetif_ (the existing AP netif handle) and either return early if already initialized or call wifiApStop() to teardown the existing apNetif_ before creating a new one with esp_netif_create_default_wifi_ap(); apply the same guard pattern used for the STA code to ensure apNetif_ is properly cleaned up and reused.
♻️ Duplicate comments (2)
docs/moonmodules/core/NetworkModule.md (2)
103-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove future-tense language from module spec.
Lines 103-105 mix current behavior with planned features. "Deferred — see docs/plan.md" violates the present-tense-only guideline for module specs. Either describe only the current implementation or move planned features to plan.md.
Based on coding guidelines: "
**/*.md: Code, comments, and documentation describe the system as it is now. No changelogs, no roadmaps."📝 Proposed fix
-Today the cascade tries each interface unconditionally and relies on the platform init calls to fail fast when the hardware isn't present: `platform::ethInit()` returns false on boards without a PHY, and the WiFi STA/AP init paths return false on chips without WiFi. The `onBuildControls()` pass binds the full control set on every device — one firmware variant handles all combinations. +The cascade tries each interface unconditionally and relies on the platform init calls to fail fast when the hardware isn't present: `platform::ethInit()` returns false on boards without a PHY, and the WiFi STA/AP init paths return false on chips without WiFi. The `onBuildControls()` pass binds the full control set on every device — one firmware variant handles all combinations. -Surfacing hardware presence to the UI (so cards for absent interfaces hide rather than show as "no link" / "no IP") is deferred — see `docs/plan.md`. +(Move the "surfacing hardware presence to UI" feature to docs/plan.md if not yet implemented)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/core/NetworkModule.md` around lines 103 - 105, Update the NetworkModule.md passage to use present-tense and describe only current behavior: remove the deferred/plan language and the "see docs/plan.md" reference; keep the factual statements about how the cascade tries each interface, that platform::ethInit() returns false on boards without a PHY, and that the WiFi STA/AP init paths return false on chips without WiFi, and note that onBuildControls() binds the full control set on every device — do not mention future work or plans in this spec (move any planned UI/hardware-presence work into docs/plan.md instead).
120-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove "Prior art" section to docs/history/.
Module specs should focus on current implementation. Historical context and reference material belong in
docs/history/per project guidelines.Based on coding guidelines: "Code, comments, and documentation describe the system as it is now. No changelogs, no roadmaps. History lives in git commits. Exceptions:
docs/plan.md(what to build next) anddocs/history/(lessons, plans, inventories)."📝 Suggested action
Remove the "Prior art" section (lines 120-137) from NetworkModule.md and add it to an appropriate file in
docs/history/(e.g.,docs/history/decisions.mdunder a "Network implementation references" subsection).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/core/NetworkModule.md` around lines 120 - 137, Remove the "Prior art" subsection from NetworkModule.md (the block starting with "## Prior art" and the listed bullets) and create or append the same content into a new entry under docs/history/decisions.md (or another file in docs/history/) as a "Network implementation references" subsection so historical context is kept in docs/history while NetworkModule.md describes only the current implementation.
🤖 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/history/decisions.md`:
- Line 460: Insert a single blank line immediately before the "### Cycle-1
assessment (May 2026)" heading so the Markdown heading is separated from the
previous paragraph or content; modify the block containing the heading text
("### Cycle-1 assessment (May 2026)") to ensure there is an empty newline above
it.
In `@docs/history/plan-11.md`:
- Line 9: The link string
"../../Developer/GitHub/ewowi/projectMM-v3/docs/moonmodules_draft/core/ui-spec.md"
in docs/history/plan-11.md is an absolute/local filesystem path; replace that
token with the repository-relative path "../moonmodules_draft/core/ui-spec.md"
so the markdown link points correctly from docs/history/plan-11.md to
docs/moonmodules_draft/core/ui-spec.md.
- Around line 128-132: The code fence containing CSS variables (e.g., --bg-0,
--bg-1, --fg, --fg-muted, --accent, --accent-soft, --card-bg-0, --card-bg-1,
--card-bg-2, --border, --green, --red, --yellow) should be updated to include a
language identifier; change the opening backticks from ``` to ```css so the
block is ```css ... ``` to enable proper syntax highlighting for the CSS
variables listing.
In `@docs/moonmodules/core/ui-spec.md`:
- Around line 139-152: Update the Markdown fenced code blocks in the UI spec to
include a language identifier (e.g., replace ``` with ```text) so the API and
localStorage examples conform to MD040; locate the code fences around the API
block (the GET/POST list) and the localStorage key list (the block around
mm_selectedRoot, mm_theme, mm_timing_mode) and change their opening fences to
```text for proper linting.
In `@docs/performance.md`:
- Around line 67-70: The note under the "Pending: ESP32 tick variability
(plan-11)" heading is a planned-investigation comment, not current measured
behavior; remove this block from docs/performance.md and relocate it into the
project's plan/history documentation (e.g., the central plan or history docs).
Edit the "Pending: ESP32 tick variability (plan-11)" section in
docs/performance.md to retain only measured results and move the investigative
TODO text into the plan/history doc so it no longer appears in the live
performance report.
In `@scripts/check/collect_kpi.py`:
- Around line 133-140: The current logic only triggers _live_capture(log) when
monitor.log is stale, so a fresh but unparseable file yields no KPI; change the
flow to call _extract_esp32_tick(log, kpi) first and check its boolean return,
and if it returns False then attempt _live_capture(log) and re-run
_extract_esp32_tick to populate kpi. Locate the block using ESP32_DIR /
"monitor.log" and the functions _extract_esp32_tick and _live_capture and update
control flow to fall back to live capture when parsing fails (not just when the
file is stale).
In `@src/core/FilesystemModule.h`:
- Around line 146-148: The code passes fileBuf_ as a C-string to applyNode() but
never null-terminates it after platform::fsRead; modify the read handling so
after int n = platform::fsRead(path, fileBuf_, sizeof(fileBuf_)); you check n >
0, then ensure fileBuf_ is NUL-terminated (e.g. set fileBuf_[n <
(int)sizeof(fileBuf_) ? n : (int)sizeof(fileBuf_) - 1] = '\0') before calling
applyNode(m, fileBuf_, ""); this prevents reading past the valid bytes when
applyNode parses the buffer.
In `@src/core/HttpServerModule.h`:
- Around line 762-763: The code currently marks the moved child dirty
(mod->markDirty()) but the move only changes the parent's children ordering;
change this to mark the parent dirty instead: retrieve the parent of mod (ensure
parent != nullptr) and call parent->markDirty(), then call
FilesystemModule::noteDirty() as before; mirror the behavior used in
handleDeleteModule for consistency.
In `@src/ui/app.js`:
- Around line 424-429: The rolesAcceptedBy function currently compares
parentMod.name instead of its type; update the comparisons to use parentMod.type
(e.g., change parentMod.name === "Layer" to parentMod.type === "Layer", and
similarly for "DriverGroup" and "LayoutGroup") so the function matches the same
type-based logic used in acceptsChildren and returns the same role arrays.
- Around line 662-666: fmtProgressLabel currently always formats values as KB
which is incorrect for controls that represent percentages or counts; update
fmtProgressLabel to inspect a unit property on the ctrl (e.g., ctrl.unit) and
branch formatting accordingly: if unit is "bytes" (or "bytes" variants) convert
v/t to KB/MB as appropriate, if unit is "percent" format as a percentage (e.g.,
v/t*100 or v with '%' depending on meaning), otherwise render a generic "v / t
[unit]" or "v / t" fallback; keep function name fmtProgressLabel and inputs
ctrl.value/ctrl.total and ensure graceful fallback when ctrl.unit is missing.
- Around line 418-422: acceptsChildren currently compares mod.name to literal
strings which is fragile; change it to use a stable module identifier (e.g.,
mod.type or mod.role) or an explicit property from the server (e.g.,
mod.allowedChildRoles) instead of mod.name. Update the acceptsChildren function
to return true when mod.type (or mod.role) matches the intended kinds (e.g.,
"Layer", "DriverGroup", "LayoutGroup") or when mod.allowedChildRoles indicates
it can contain children, and replace all mod.name checks with that property so
renaming or multiple instances won't break behavior.
In `@src/ui/index.html`:
- Around line 16-18: The header buttons default to type="submit" and should
explicitly be non-submitting: update the three button elements with ids
ws-reconnect, reboot-btn, and theme-toggle to include type="button" (i.e., set
the type attribute to "button" on each) to prevent accidental form submission
and silence the HTMLHint warning.
In `@test/test_system_module.cpp`:
- Around line 17-20: The test currently asserts CHECK(sys.controls().count() >=
6) which can hide regressions; change this to assert the exact expected number
of controls for desktop behavior (replace the >= 6 check with
CHECK(sys.controls().count() == N) using the correct expected count) so the test
fails if any control is missing or removed—locate the assertion that calls
sys.controls().count() in test_system_module.cpp and update it to the precise
expected value for desktop (use the actual integer for N based on expected
controls: uptime, fps, tickTimeUs, maxBlock, deviceName, chip, sdk, bootReason).
---
Outside diff comments:
In `@docs/moonmodules_draft/core/ui-spec-deferred.md`:
- Around line 1-151: This file is roadmap/history content and must be split:
extract all future/backlog and action items under headings like "Deferred to
1.x", "Open design questions", "Gap analysis" and consolidate them into the
policy-approved plan document (create a short living "plan" doc for
deferred/adopt/defer/drop decisions); move the purely historical analysis and
prior-art notes (sections such as "Loose ends", "Prior art" and the long
provenance notes) into a separate "history" doc; update the original "UI
Specification — Deferred items & research" header to point readers to the new
plan and history docs and remove roadmap content from this file so it only
documents the current shipped UI baseline.
In `@docs/moonmodules/core/ui-spec.md`:
- Around line 232-235: Remove the historical "## Heritage" section from the
current UI spec (the header line "## Heritage" and its paragraph referencing v1
and moonmodules_draft/core/ui-spec-deferred.md), relocate that content into the
canonical docs/history area (or docs/plan.md if appropriate), and replace it in
the spec with a one-line pointer stating historical details were moved; ensure
the spec contains only current-system documentation and update any internal
links to point to the new docs/history location.
In `@src/core/FilesystemModule.h`:
- Around line 78-90: flush() currently clears subtree dirty flags
unconditionally; change it so dirty state is only cleared when saveSubtree
actually succeeds: call saveSubtree(m) and check its success (or catch
exceptions) before invoking clearSubtreeDirty(m) and updating lastSaveMs_; if
saveSubtree fails or throws, leave the dirty flag and dirtyPending_ untouched so
pending persistence isn't lost. Refer to flush(), saveSubtree(),
clearSubtreeDirty(), subtreeDirty(), lastSaveMs_, and dirtyPending_ when making
the change.
In `@src/platform/esp32/platform_esp32.cpp`:
- Line 446: The call to esp_netif_create_default_wifi_sta() in wifiStaInit() can
leak the previous netif stored in staNetif_ if wifiStaInit() is invoked multiple
times; update wifiStaInit() to check staNetif_ and only create a new netif if
staNetif_ is null (or explicitly call wifiStaStop()/esp_netif_destroy before
overwriting) so the previous handle is not leaked, referencing the
wifiStaInit(), wifiStaStop(), esp_netif_create_default_wifi_sta(), and staNetif_
symbols when applying the guard.
- Line 498: The AP init path currently calls esp_netif_create_default_wifi_ap()
unconditionally, which leaks the previous AP netif if wifiApInit() is called
repeatedly; update wifiApInit() to guard against repeated inits by checking
apNetif_ (the existing AP netif handle) and either return early if already
initialized or call wifiApStop() to teardown the existing apNetif_ before
creating a new one with esp_netif_create_default_wifi_ap(); apply the same guard
pattern used for the STA code to ensure apNetif_ is properly cleaned up and
reused.
---
Duplicate comments:
In `@docs/moonmodules/core/NetworkModule.md`:
- Around line 103-105: Update the NetworkModule.md passage to use present-tense
and describe only current behavior: remove the deferred/plan language and the
"see docs/plan.md" reference; keep the factual statements about how the cascade
tries each interface, that platform::ethInit() returns false on boards without a
PHY, and that the WiFi STA/AP init paths return false on chips without WiFi, and
note that onBuildControls() binds the full control set on every device — do not
mention future work or plans in this spec (move any planned UI/hardware-presence
work into docs/plan.md instead).
- Around line 120-137: Remove the "Prior art" subsection from NetworkModule.md
(the block starting with "## Prior art" and the listed bullets) and create or
append the same content into a new entry under docs/history/decisions.md (or
another file in docs/history/) as a "Network implementation references"
subsection so historical context is kept in docs/history while NetworkModule.md
describes only the current implementation.
🪄 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: 03ac7a05-84e2-4a1a-95bf-e937731db753
📒 Files selected for processing (36)
CLAUDE.mddocs/architecture.mddocs/history/decisions.mddocs/history/plan-11.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/SystemModule.mddocs/moonmodules/core/ui-spec.mddocs/moonmodules_draft/core/ui-spec-deferred.mddocs/moonmodules_draft/core/ui-spec.mddocs/performance.mddocs/plan.mddocs/testing.mdscripts/check/collect_kpi.pysrc/core/FilesystemModule.hsrc/core/HttpServerModule.hsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/NetworkModule.hsrc/core/Scheduler.hsrc/core/SystemModule.hsrc/light/DriverGroup.hsrc/light/Layer.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/index.htmlsrc/ui/style.csstest/CMakeLists.txttest/test_filesystem_persistence.cpptest/test_module_factory.cpptest/test_movechild.cpptest/test_system_module.cpp
💤 Files with no reviewable changes (1)
- docs/moonmodules_draft/core/ui-spec.md
| ### setName must copy, not store pointer | ||
| - HTTP module creation stored a pointer to a stack-local buffer. After the function returned, the name was garbage. Fixed by making name_ a char[24] buffer with memcpy in setName(). | ||
|
|
||
| ### Cycle-1 assessment (May 2026) |
There was a problem hiding this comment.
Add blank line before heading.
Markdown best practice requires a blank line before headings.
📝 Proposed fix
- `classSize` set via `register_type<T>()`.
+
### Cycle-1 assessment (May 2026)📝 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
| ### Cycle-1 assessment (May 2026) | |
| - `classSize` set via `register_type<T>()`. | |
| ### Cycle-1 assessment (May 2026) |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 460-460: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/decisions.md` at line 460, Insert a single blank line
immediately before the "### Cycle-1 assessment (May 2026)" heading so the
Markdown heading is separated from the previous paragraph or content; modify the
block containing the heading text ("### Cycle-1 assessment (May 2026)") to
ensure there is an empty newline above it.
Comment on lines
+128
to
+132
| ``` | ||
| --bg-0, --bg-1, --fg, --fg-muted, --accent, --accent-soft, | ||
| --card-bg-0, --card-bg-1, --card-bg-2 (depth-based backgrounds), | ||
| --border, --green (connected/ok), --red (error/crashed), --yellow (warn) | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to code fence.
CSS variables block should be marked with ```css for proper syntax highlighting.
📝 Proposed fix
**Layer 1 — variables.** Define `:root` CSS variables for the palette:
-```
+```css
--bg-0, --bg-1, --fg, --fg-muted, --accent, --accent-soft,🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 128-128: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 132-132: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plan-11.md` around lines 128 - 132, The code fence containing
CSS variables (e.g., --bg-0, --bg-1, --fg, --fg-muted, --accent, --accent-soft,
--card-bg-0, --card-bg-1, --card-bg-2, --border, --green, --red, --yellow)
should be updated to include a language identifier; change the opening backticks
from ``` to ```css so the block is ```css ... ``` to enable proper syntax
highlighting for the CSS variables listing.
Comment on lines
+139
to
+152
| ``` | ||
| GET /api/state full module tree state — initial load + post-mutation refresh | ||
| each module entry includes name, type, role, enabled, loopTimeUs, controls[] | ||
| GET /api/types {types:[{name, role, defaults}]} — for the type picker | ||
| defaults map is captured from a fresh probe instance per type | ||
| GET /api/system fps, tickTimeUs, freeHeap, freeInternal, maxBlock, uptime | ||
| POST /api/control {module, control, value} — set a control value | ||
| POST /api/modules {type, parent_id?} — create | ||
| POST /api/modules/<n>/move {to: N} — reorder to absolute index N within parent | ||
| strict-suffix match: /movex returns 404 | ||
| triggers Scheduler::rebuild() so LUT-affecting reorders rebuild | ||
| DELETE /api/modules/<name> | ||
| POST /api/reboot calls platform::reboot() — esp_restart() on ESP32, std::exit(0) on desktop | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
The API and localStorage fences are missing language tags (MD040), which breaks lint consistency.
Suggested patch
-```
+```text
GET /api/state full module tree state — initial load + post-mutation refresh
...
POST /api/reboot calls platform::reboot() — esp_restart() on ESP32, std::exit(0) on desktop- +text
mm_selectedRoot id of currently-selected root module (string)
mm_theme "dark" | "light" (default: "dark")
mm_timing_mode "fps" | "ms" (default: "fps")
Also applies to: 209-213
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/core/ui-spec.md` around lines 139 - 152, Update the Markdown
fenced code blocks in the UI spec to include a language identifier (e.g.,
replace ``` with ```text) so the API and localStorage examples conform to MD040;
locate the code fences around the API block (the GET/POST list) and the
localStorage key list (the block around mm_selectedRoot, mm_theme,
mm_timing_mode) and change their opening fences to ```text for proper linting.
Comment on lines
+662
to
666
| function fmtProgressLabel(ctrl) { | ||
| const v = Number(ctrl.value) || 0; | ||
| const t = Number(ctrl.total) || 0; | ||
| return Math.round(v / 1024) + "KB / " + Math.round(t / 1024) + "KB"; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Progress label always displays KB, but not all progress controls represent bytes.
fmtProgressLabel hardcodes KB formatting, but progress controls could represent percentages, counts, or other units. Consider using the control's unit property if available, or making this formatter more generic.
🤖 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 662 - 666, fmtProgressLabel currently always
formats values as KB which is incorrect for controls that represent percentages
or counts; update fmtProgressLabel to inspect a unit property on the ctrl (e.g.,
ctrl.unit) and branch formatting accordingly: if unit is "bytes" (or "bytes"
variants) convert v/t to KB/MB as appropriate, if unit is "percent" format as a
percentage (e.g., v/t*100 or v with '%' depending on meaning), otherwise render
a generic "v / t [unit]" or "v / t" fallback; keep function name
fmtProgressLabel and inputs ctrl.value/ctrl.total and ensure graceful fallback
when ctrl.unit is missing.
KPI: 16384lights | PC:210KB | tick:51us(FPS:19607) | ESP32:857KB | tick:101211us(FPS:9) | heap:118KB | src:35(5054) | test:15(1893) | lizard:15w Follow-up to plan-11 (92a558b): bug fixes from code review plus a documentation cleanup pass. Code fixes: - FilesystemModule: NUL-terminate the read buffer after platform::fsRead before applyNode parses it as a C-string; saveSubtree now returns bool and flush() clears a subtree's dirty flag only on a successful write, keeping dirtyPending_ set so loop1s retries on failure. - HttpServerModule: the move handler marks the parent dirty (the move changes the parent's child ordering), matching the add/delete handlers. - platform_esp32: wifiStaInit/wifiApInit tear down a pre-existing netif before creating a new one — guards a leak when the cascade re-inits an interface. The guard runs before ensureWifiInit() because wifiStaStop/ wifiApStop deinit the WiFi driver. - app.js: acceptsChildren / rolesAcceptedBy key on mod.type (stable factory key) instead of mod.name (editable per instance). - index.html: type="button" on the three header buttons. - test_system_module: assert the exact desktop control count (== 11) instead of >= 6, so a removed/renamed control fails the test. - collect_kpi.py: live ESP32 capture also triggers when monitor.log parses to no tick line, not only when the file is stale. Documentation: - moonmodules/ specs describe current behavior only — cross-repo v1/v2/MoonLight mentions moved out of prose into the per-spec "Prior art" sections (kept as a temporary cherry-pick record). - architecture.md "The Problem" rewritten domain-neutral (the core is a modular runtime, not a light system); the LED/DMX framing moved to architecture-light.md, along with the "Rebuild Propagation" section. - README.md leads with the core/domain split. - ui-spec.md renamed to ui.md (docs/moonmodules/core/ and the deferred companion in docs/moonmodules_draft/core/); all cross-links updated. - .coderabbit.yaml: auto_review disabled — reviews are now requested on demand to fit CodeRabbit's hourly budget. ESP32 tick variability (plan-11, tracked in plan.md) measured again at ~100ms / 9 FPS — within the documented swing; performance.md range widened to 6-9 FPS accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/history/plan-11.md (1)
128-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifier to CSS code fence.
The CSS variables block should be marked with
```cssfor proper syntax highlighting.📝 Proposed fix
**Layer 1 — variables.** Define `:root` CSS variables for the palette: -``` +```css --bg-0, --bg-1, --fg, --fg-muted, --accent, --accent-soft,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/history/plan-11.md` around lines 128 - 132, The code block containing the CSS variables (--bg-0, --bg-1, --fg, --fg-muted, --accent, --accent-soft, --card-bg-0, --card-bg-1, --card-bg-2, --border, --green, --red, --yellow) should be changed from a plain triple-backtick fence to a CSS-marked fence by adding the language identifier (```css) at the start of the block so the variables receive proper syntax highlighting; locate the code fence surrounding that variables list and prefix it with "css" after the opening backticks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/history/plan-11.md`:
- Around line 128-132: The code block containing the CSS variables (--bg-0,
--bg-1, --fg, --fg-muted, --accent, --accent-soft, --card-bg-0, --card-bg-1,
--card-bg-2, --border, --green, --red, --yellow) should be changed from a plain
triple-backtick fence to a CSS-marked fence by adding the language identifier
(```css) at the start of the block so the variables receive proper syntax
highlighting; locate the code fence surrounding that variables list and prefix
it with "css" after the opening backticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aac6bfcd-2be1-4954-bd9d-7be4a6384de4
📒 Files selected for processing (31)
.coderabbit.yamlREADME.mddocs/architecture-light.mddocs/architecture.mddocs/history/plan-11.mddocs/moonmodules/core/Control.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/core/Scheduler.mddocs/moonmodules/core/ui.mddocs/moonmodules/light/Buffer.mddocs/moonmodules/light/EffectBase.mddocs/moonmodules/light/Layer.mddocs/moonmodules/light/LayoutGroup.mddocs/moonmodules/light/LightConfig.mddocs/moonmodules/light/MappingLUT.mddocs/moonmodules/light/drivers/ArtNetSendDriver.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/effects/RainbowEffect.mddocs/moonmodules/light/layouts/GridLayout.mddocs/moonmodules_draft/core/ui.mddocs/performance.mddocs/plan.mdscripts/check/collect_kpi.pysrc/core/FilesystemModule.hsrc/core/HttpServerModule.hsrc/platform/esp32/platform_esp32.cppsrc/ui/app.jssrc/ui/index.htmltest/test_system_module.cpp
💤 Files with no reviewable changes (3)
- docs/moonmodules/core/Scheduler.md
- docs/moonmodules/light/layouts/GridLayout.md
- docs/moonmodules/light/drivers/ArtNetSendDriver.md
Collaborator
Author
|
@coderabbitai, are these all your review findings: #2 (review) ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull up all the reviews on this PR to give you an accurate answer. 🏁 Script executed: Length of output: 11545 Let me also check the actual reviews list via the REST API to confirm whether that review ID exists. 🏁 Script executed: Length of output: 1163
The actual findings were spread across 4 earlier reviews, each triggered by new commits pushed to
38 inline comments total across 4 reviews. The triage you documented in the comment chain (the two "Fixed/Skipped" summaries) covers findings from reviews 1 and 3–4. Review 2 (4335053864) contained the |
Collaborator
Author
|
@coderabbitai, regarding
I processed these, see: #2 (comment) So is it correct for commit 4ca59c1 you only have 1 finding:
? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the exact comments tied to that commit. 🏁 Script executed: Length of output: 9122
The GitHub API is reporting all 24 inline comments from all four reviews with that commit hash — this happens because GitHub resolves comment Looking specifically at review
Cross-referencing against your two triage passes:
🐇 🔍 |
…ntrols
KPI: 16384lights | PC:211KB | tick:95/70/116/102us(FPS:10526/14285/8620/9803) | ESP32:471KB | tick:51101us(FPS:19) | heap:124KB | src:35(5298) | test:16(2082) | lizard:17w
Ethernet-only ESP32 build profile: build_esp32.py --profile default|eth-only.
WiFi is compiled out via EXCLUDE_COMPONENTS + an MM_NO_WIFI define; a hasWiFi
compile-time constant gates if constexpr branches in NetworkModule. The eth-only
image is 542KB vs 989KB default (-45%), confirming WiFi is genuinely absent.
FPS-swing fix: the render tick collapsed (~38ms -> ~100ms) when a browser
connected because HttpServerModule pushed a ~49KB preview WebSocket frame with a
blocking write that spun vTaskDelay until lwIP's small send buffer drained. The
preview broadcast now uses a non-blocking scatter-gather write
(TcpConnection::writeChunks) and PreviewDriver downsamples the payload (adaptive
stride, <=1849 voxels) so the frame fits the send buffer in one write. The
render task never blocks on a slow browser; frames are simply dropped.
ArtNet send-cost optimization: connected UDP socket (skips per-packet route/ARP
lookup) plus lwIP core locking. ArtNet per-tick cost ~50,000 -> ~27,700us.
Combined with the FPS fix, the steady ESP32 tick is ~51ms / 19 FPS with a
browser connected (was ~69ms / 14 FPS).
PreviewDriver detail (1/2/3) and decompress controls: detail sets the downsample
voxel budget; decompress is a client-side block-replicate UI hint. PreviewFrame
carries the original grid dimensions so the browser can reconstruct.
Tests: test_preview_driver.cpp (6 unit tests covering detail strides, original
dimensions, send-buffer budget, channel-agnostic copy); preview-detail.json live
scenario; a formula-based min_fps_led_product throughput floor wired into the
scenario runner, run_live_scenario.py, collect_kpi.py and base-pipeline.json --
compared against measured tick time, not lossy derived FPS.
Scenario-runner crash fix: scenario_runner.cpp stack-allocated modules but
Scheduler::teardown() now deletes them; switched to ModuleFactory::create()
(heap-allocated), matching production main.cpp.
CLAUDE.md gains pre-commit step 11 (advisory permission review).
Pre-commit: desktop build clean (-Werror), 81 unit tests + 8 in-process
scenarios pass, platform boundary + spec check PASS, both ESP32 profiles build
clean, reviewer agent PASS (findings applied). ESP32 flashed and live-measured
at 19 FPS. Live-scenario suite has known-accepted failures: control-change /
grid-resize expect a device tree with modules named Mirror/Noise (the board has
MirrorModifier/RainbowEffect), and a few min_pct misses are documented ESP32
run-to-run tick variance, not regressions -- see docs/performance.md.
KPI details:
Desktop: 16,384 lights, 211KB binary, 81 tests pass, 8 scenarios pass
ESP32 (eth-only): 542,457 byte image (70% partition free), 471KB flash,
tick 51,101us / 19 FPS, 127KB heap free
Code: 35 source files (5,298 lines), 16 test files (2,082 lines), 17 lizard
warnings (broadcastPreviewFrame is the only new high-complexity function)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/moonmodules/core/ui.md (1)
13-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreview binary frame contract is documented with an outdated header.
This spec still states the old 3-dimension header (
[0x02][w16][h16][d16]...) in multiple places. The current contract includes downsampled and original dimensions, so this doc currently describes the wrong wire format.Suggested doc patch
-- WebSocket at `/ws` pushes full-tree state JSON and binary preview frames (`[0x02][w16][h16][d16][RGB…]`). +- WebSocket at `/ws` pushes full-tree state JSON and binary preview frames (`[0x02][dw16][dh16][dd16][ow16][oh16][od16][RGB…]`). -- Server pushes binary preview frames: `[0x02][w16][h16][d16][R G B …]` — 3D voxel grid, little-endian widths +- Server pushes binary preview frames: `[0x02][dw16][dh16][dd16][ow16][oh16][od16][R G B …]` — downsampled + original grid dimensions (all little-endian) -- Frame format: `[0x02][w16][h16][d16][R G B …]` — width/height/depth in 16-bit little-endian, then RGB triples in `(z, y, x)` order +- Frame format: `[0x02][dw16][dh16][dd16][ow16][oh16][od16][R G B …]` — downsampled + original dimensions in 16-bit little-endian, then RGB triples in `(z, y, x)` orderAlso applies to: 132-133, 182-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/core/ui.md` around lines 13 - 14, Update the documented preview binary frame header for the WebSocket at `/ws`: replace the outdated 3-dimension header notation `[0x02][w16][h16][d16]...` with the current contract that includes both downsampled and original dimensions (e.g. `[0x02][w_ds16][h_ds16][w16][h16][d16][RGB…]`) and apply the same replacement to the other occurrences of the old header in the document (the other spots describing preview binary frames). Ensure the text clearly names the downsampled (w_ds,h_ds) and original (w,h) fields and that examples/diagrams reflect the new ordering and sizes.
♻️ Duplicate comments (1)
src/ui/app.js (1)
1041-1043:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the stable preview type here too.
mod.nameis instance-editable, so renaming the Preview card disables this lookup even though the module kind has not changed. Match only onmod.typein this path.🤖 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 1041 - 1043, The lookup currently matches on m.type or m.name which breaks if the module instance is renamed; update the condition in the block that checks preview modules so it matches only on m.type (e.g., replace the check (m.type === "PreviewDriver" || m.name === "Preview") with a check that both allowed preview kinds use m.type, e.g., m.type === "PreviewDriver" || m.type === "Preview"), then keep the rest (finding the "decompress" control on m.controls and setting found = !!c.value) unchanged.
🤖 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/performance.md`:
- Around line 58-60: The documentation contains conflicting FPS numbers for the
same optimized Ethernet/browser-connected benchmark (e.g., the "Total tick |
FPS: 19" line versus later mentions of 13 FPS and 20 FPS and the HttpServer
(~44,000 → ~850 µs) and ArtNet (~50,000 → ~27,700 µs) improvements); update this
section and the other affected spots (around lines cited) to present a single,
consistent, clearly-scoped measurement set or explicitly label each figure with
its test condition (e.g., "steady tick with browser connected, optimizations
A+B", "only HttpServer fix", "only ArtNet fix"); ensure the sentence referencing
the combined effect explicitly states which test produced ~51 ms / 19 FPS and
remove or relabel the contradictory 13 and 20 FPS statements so all claims about
"Total tick", "HttpServer", and "ArtNet" are consistent and unambiguous.
In `@docs/testing.md`:
- Around line 241-243: The docs currently contradict whether preview-detail
enforces an absolute throughput floor; update the preview-detail checklist
wording so it consistently states: preview-detail uses a relative bound for
per-step assertions (fps.min_pct) but the absolute floor min_fps_led_product is
also enforced by the base-pipeline/preview-detail via tick-time budgeting and is
validated by the settled-reading check in collect_kpi.py --commit; mention that
decompress is client-side and does not affect render tick and that live-only
runs are skipped by the in-process runner to keep behavior clear.
In `@scripts/check/collect_kpi.py`:
- Around line 360-364: The throughput gate is using desktop.get("lights") (a
buffer size) instead of the live device light count; replace lights =
desktop.get("lights") with a live count lookup from the esp32 state (e.g.,
lights = esp32.get("lights")) and keep the current fallback to desktop if the
esp32 value is missing, then compute max_tick using that live lights variable
with MIN_ESP32_FPS_LED_PRODUCT and compare esp32_tick > max_tick as before
(referencing esp32_tick, lights, max_tick, esp32, desktop,
MIN_ESP32_FPS_LED_PRODUCT).
- Around line 149-155: The code currently calls _extract_esp32_tick(log, kpi)
even when stale is True and a refresh via _live_capture(log) failed, which
allows using an old monitor.log; change the control flow so _extract_esp32_tick
is only invoked against a fresh capture or a non-stale file: call
_live_capture(log) and capture its boolean result, and only call
_extract_esp32_tick(log, kpi) when stale is False or when the captured result is
True (i.e., replace the unconditional second _extract_esp32_tick call with a
conditional that checks the success of _live_capture), referencing the variables
and functions stale, log, _live_capture, _extract_esp32_tick and kpi.
In `@scripts/scenario/run_live_scenario.py`:
- Around line 180-195: Validate the bounds["fps"]["min_fps_led_product"] value
before using it in the division: check that product is a positive number (>0)
and handle non-numeric inputs; if invalid, skip the throughput check (or mark as
failed) with a clear warning instead of performing the division that computes
max_tick, otherwise compute max_tick as currently done and compare with tick_us.
Update the logic around the product variable and the max_tick calculation in the
block that calls count_lights(client) so the division never runs with product <=
0 or non-numeric values.
In `@src/core/FilesystemModule.h`:
- Around line 262-281: The save now can succeed even when writeNode produced
malformed JSON (unescaped " or \ in Text), so modify writeNode (the function
called as writeNode(m, fileBuf_, sizeof(fileBuf_), pos, "")) to escape string
contents when serializing Text controls (replace " with \" and \ with \\),
ensure escaping accounts for buffer capacity and return false on overflow or any
failure; this guarantees fileBuf_ contains valid JSON before saveSubtree calls
platform::fsWriteAtomic(path, fileBuf_, ...), so saveSubtree's existing checks
will treat malformed serialization as a failed save.
In `@src/core/HttpServerModule.h`:
- Around line 930-932: Update the outdated comment above the chunks[]
declaration: replace the claim that previewFrame_->data is a zero-copy pointer
into DriverGroup with an accurate note that PreviewDriver::loop() now points
previewFrame_->data at the driver's own downsample buffer (i.e., it is
driver-owned, not a DriverGroup zero-copy slice), and clarify that the payload
chunk refers to that driver-owned buffer; update or remove any wording that
implies zero-copy into DriverGroup and mention PreviewDriver::loop() and
DriverGroup to document intent.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 661-679: TcpConnection::writeChunks must report how many bytes
were actually consumed on partial lwip_writev so callers can resume correctly;
change the API to return or populate bytesWritten (e.g., add an out parameter
size_t* bytesWritten or return a struct { WriteResult result; size_t
bytesWritten; }) and update writeChunks(…) to loop/adjust iovec on partial
writes: call lwip_writev(fd_, iov + iov_offset, iov_count), on n>0 subtract n
from the leading iov entries (advance iov_base and reduce iov_len), accumulate
bytesWritten, and continue until all bytes sent (return Complete) or lwip_writev
returns EAGAIN/EWOULDBLOCK (return WouldBlock with bytesWritten set) or error
(return Error and bytesWritten set). Ensure you update all call sites that use
writeChunks and reference symbols: WriteChunk, MAX_WRITE_CHUNKS, lwip_writev,
and WriteResult.
In `@src/platform/platform.h`:
- Around line 83-86: Replace raw pointer+length APIs with std::span to avoid
exposing pointer/len contracts: change sendTo(const uint8_t* data, size_t len)
to sendTo(std::span<const uint8_t> data), update the WriteChunk struct to hold
std::span<const uint8_t> data; size_t len should be removed, and change
writeChunks(const WriteChunk* chunks, int count) to writeChunks(std::span<const
WriteChunk> chunks). Also add `#include` <span> and adjust any callers to
construct std::span from buffers/arrays accordingly.
In `@test/test_preview_driver.cpp`:
- Around line 51-53: The helper produceFrame() currently uses
mm::platform::delayMs(1000 / driver.fps + 5) which makes tests rely on
wall-clock timing; change tests to drive frames deterministically by removing
the delay and using a test seam on PreviewDriver: either call a new test-only
method (e.g., PreviewDriver::forceLoop()/advanceFrame()) or inject a
deterministic Clock/Now source into PreviewDriver and call
driver.loopWithTimestamp(...) with a simulated timestamp; update the test to
call driver.loop() via that seam instead of sleeping so frame rate limiting in
PreviewDriver is bypassed for tests.
---
Outside diff comments:
In `@docs/moonmodules/core/ui.md`:
- Around line 13-14: Update the documented preview binary frame header for the
WebSocket at `/ws`: replace the outdated 3-dimension header notation
`[0x02][w16][h16][d16]...` with the current contract that includes both
downsampled and original dimensions (e.g.
`[0x02][w_ds16][h_ds16][w16][h16][d16][RGB…]`) and apply the same replacement to
the other occurrences of the old header in the document (the other spots
describing preview binary frames). Ensure the text clearly names the downsampled
(w_ds,h_ds) and original (w,h) fields and that examples/diagrams reflect the new
ordering and sizes.
---
Duplicate comments:
In `@src/ui/app.js`:
- Around line 1041-1043: The lookup currently matches on m.type or m.name which
breaks if the module instance is renamed; update the condition in the block that
checks preview modules so it matches only on m.type (e.g., replace the check
(m.type === "PreviewDriver" || m.name === "Preview") with a check that both
allowed preview kinds use m.type, e.g., m.type === "PreviewDriver" || m.type ===
"Preview"), then keep the rest (finding the "decompress" control on m.controls
and setting found = !!c.value) unchanged.
🪄 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: bd76f408-094a-4fb0-beb9-31f61bdc53ad
⛔ Files ignored due to path filters (2)
scripts/build/build_esp32.pyis excluded by!**/build/**scripts/build/build_esp32_ethonly.pyis excluded by!**/build/**
📒 Files selected for processing (53)
.coderabbit.yamlCLAUDE.mdREADME.mddocs/architecture-light.mddocs/architecture.mddocs/history/plan-11.mddocs/moonmodules/core/Control.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/Scheduler.mddocs/moonmodules/core/ui.mddocs/moonmodules/light/Buffer.mddocs/moonmodules/light/EffectBase.mddocs/moonmodules/light/Layer.mddocs/moonmodules/light/LayoutGroup.mddocs/moonmodules/light/LightConfig.mddocs/moonmodules/light/MappingLUT.mddocs/moonmodules/light/drivers/ArtNetSendDriver.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/effects/RainbowEffect.mddocs/moonmodules/light/layouts/GridLayout.mddocs/moonmodules_draft/core/ui.mddocs/performance.mddocs/plan.mddocs/testing.mdesp32/main/CMakeLists.txtesp32/sdkconfig.defaultsscripts/MoonDeck.mdscripts/check/collect_kpi.pyscripts/moondeck_config.jsonscripts/scenario/run_live_scenario.pysrc/core/FilesystemModule.hsrc/core/HttpServerModule.hsrc/core/NetworkModule.hsrc/core/PreviewFrame.hsrc/light/ArtNetSendDriver.hsrc/light/PreviewDriver.hsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/index.htmltest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/base-pipeline.jsontest/scenarios/preview-detail.jsontest/test_preview_driver.cpptest/test_system_module.cpp
💤 Files with no reviewable changes (2)
- docs/moonmodules/light/layouts/GridLayout.md
- docs/moonmodules/core/Scheduler.md
Comment on lines
+360
to
+364
| esp32_tick = esp32.get("tick_us") | ||
| lights = desktop.get("lights") | ||
| if esp32_tick is not None and lights: | ||
| max_tick = round(lights * 1_000_000 / MIN_ESP32_FPS_LED_PRODUCT) | ||
| if esp32_tick > max_tick: |
There was a problem hiding this comment.
Scale the throughput gate with the live light count, not desktop output.
esp32["tick_us"] comes from the current live device state, but desktop["lights"] is just the largest in-process scenario buffer. If those layouts differ, this gate can false-pass or false-fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check/collect_kpi.py` around lines 360 - 364, The throughput gate is
using desktop.get("lights") (a buffer size) instead of the live device light
count; replace lights = desktop.get("lights") with a live count lookup from the
esp32 state (e.g., lights = esp32.get("lights")) and keep the current fallback
to desktop if the esp32 value is missing, then compute max_tick using that live
lights variable with MIN_ESP32_FPS_LED_PRODUCT and compare esp32_tick > max_tick
as before (referencing esp32_tick, lights, max_tick, esp32, desktop,
MIN_ESP32_FPS_LED_PRODUCT).
Comment on lines
+661
to
+679
| WriteResult TcpConnection::writeChunks(const WriteChunk* chunks, int count) { | ||
| if (fd_ < 0) return WriteResult::Error; | ||
| if (count < 1 || count > MAX_WRITE_CHUNKS) return WriteResult::Error; | ||
| struct iovec iov[MAX_WRITE_CHUNKS]; | ||
| size_t total = 0; | ||
| for (int i = 0; i < count; i++) { | ||
| iov[i].iov_base = const_cast<uint8_t*>(chunks[i].data); | ||
| iov[i].iov_len = chunks[i].len; | ||
| total += chunks[i].len; | ||
| } | ||
| // Single non-blocking writev — the socket is already O_NONBLOCK. | ||
| ssize_t n = lwip_writev(fd_, iov, count); | ||
| if (n < 0) { | ||
| return (errno == EAGAIN || errno == EWOULDBLOCK) | ||
| ? WriteResult::WouldBlock : WriteResult::Error; | ||
| } | ||
| if (n == 0) return WriteResult::WouldBlock; | ||
| if (static_cast<size_t>(n) == total) return WriteResult::Complete; | ||
| return WriteResult::Partial; |
There was a problem hiding this comment.
Return consumed bytes for partial writev sends.
A TCP writev can legally send only a prefix. Returning only WriteResult::Partial gives the caller no way to resume at the correct offset, so any retry has to resend bytes that already reached the peer and can corrupt the stream. Either finish the send loop inside writeChunks() or extend the API to report bytesWritten.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 661-661: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 661-661: method 'writeChunks' can be made const
(readability-make-member-function-const)
[warning] 662-662: statement should be inside braces
(readability-braces-around-statements)
[warning] 663-663: statement should be inside braces
(readability-braces-around-statements)
[warning] 664-664: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 665-665: variable 'total' is not initialized
(cppcoreguidelines-init-variables)
[warning] 672-672: variable 'n' is not initialized
(cppcoreguidelines-init-variables)
[warning] 672-672: variable name 'n' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 677-677: statement should be inside braces
(readability-braces-around-statements)
[warning] 678-678: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.20.0)
[style] 661-661: The function 'writeChunks' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/esp32/platform_esp32.cpp` around lines 661 - 679,
TcpConnection::writeChunks must report how many bytes were actually consumed on
partial lwip_writev so callers can resume correctly; change the API to return or
populate bytesWritten (e.g., add an out parameter size_t* bytesWritten or return
a struct { WriteResult result; size_t bytesWritten; }) and update writeChunks(…)
to loop/adjust iovec on partial writes: call lwip_writev(fd_, iov + iov_offset,
iov_count), on n>0 subtract n from the leading iov entries (advance iov_base and
reduce iov_len), accumulate bytesWritten, and continue until all bytes sent
(return Complete) or lwip_writev returns EAGAIN/EWOULDBLOCK (return WouldBlock
with bytesWritten set) or error (return Error and bytesWritten set). Ensure you
update all call sites that use writeChunks and reference symbols: WriteChunk,
MAX_WRITE_CHUNKS, lwip_writev, and WriteResult.
Comment on lines
+83
to
+86
| // Bind a fixed destination so each sendTo() skips the per-packet address | ||
| // parse + route lookup. Returns false on a bad IP. | ||
| bool connect(const char* ip, uint16_t port); | ||
| bool sendTo(const uint8_t* data, size_t len); // uses the connect()ed destination |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking configured C++ standard:"
fd -a 'CMakeLists.txt' . | xargs rg -n 'CXX_STANDARD|cxx_std_20|cxx_std_23' || true
echo
echo "Searching for existing std::span usage:"
rg -n 'std::span' src test || trueRepository: ewowi/projectMM-v3
Length of output: 446
Avoid raw pointer+length contracts in mm::platform public API (src/platform/platform.h lines 83-86, 93-127)
sendTo(const uint8_t*, size_t), WriteChunk { const uint8_t* data; size_t len; }, and writeChunks(const WriteChunk*, int) expand the platform boundary with pointer+length APIs; switch these signatures to std::span (and ensure <span> is included) to align with the repo’s C++20 + existing std::span usage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/platform.h` around lines 83 - 86, Replace raw pointer+length
APIs with std::span to avoid exposing pointer/len contracts: change sendTo(const
uint8_t* data, size_t len) to sendTo(std::span<const uint8_t> data), update the
WriteChunk struct to hold std::span<const uint8_t> data; size_t len should be
removed, and change writeChunks(const WriteChunk* chunks, int count) to
writeChunks(std::span<const WriteChunk> chunks). Also add `#include` <span> and
adjust any callers to construct std::span from buffers/arrays accordingly.
KPI: 16384lights | PC:227KB | tick:50/51/100/99us(FPS:20000/19607/10000/10101) | ESP32:485KB | tick:43247us(FPS:23) | heap:115KB | src:35(5397) | test:16(2080) | lizard:18w
Side-navigation UI (plan-12): a hamburger-toggled left column lists the root
modules, one root visible at a time, selection persisted. A footer carries the
copyright and social links. A MoonLight logo (downscaled to 64x64, embedded as
a 4th hex array, served at /moonlight-logo.png) appears in the header and as the
browser favicon. On narrow screens the nav becomes a slide-in drawer.
Password control: new ControlType::Password. NetworkModule's WiFi password uses
it. /api/state serializes the password XOR-obfuscated + base64-encoded rather
than in plaintext — a first line of defence (the XOR key is shared with app.js,
so it is trivially reversible by design), enough that the secret is not plainly
readable in a raw API response. The UI decodes it so hold-to-peek still reveals
the stored value; FilesystemModule persists it.
NetworkModule WiFi->Ethernet cascade fix: State::ConnectedSta now promotes to
Ethernet when a cable comes up (it previously only watched for WiFi dropping),
matching the priority cascade the spec already documented.
JSON string escaping: control values containing " or \ produced malformed JSON
in /api/state and the persisted config. FilesystemModule and HttpServerModule
now escape string values; JsonUtil::parseString un-escapes on read.
Unnecessary-abstraction cleanup: removed HttpServerModule's parseJsonString/
Int/Bool no-op wrappers (call sites use mm::json::* directly) and
NetworkModule::rebuildLocalControlsAndPipeline (a one-line wrapper whose name
contradicted its own behaviour). CLAUDE.md's Reviewer role sharpened to name the
no-op-wrapper pattern explicitly, plus a stale test command fixed and the
duplicated Reviewer definition collapsed to one authoritative copy.
CodeRabbit review fixes: collect_kpi.py no longer trusts a stale monitor.log
when a live capture fails; run_live_scenario.py validates min_fps_led_product;
test_preview_driver.cpp uses a PreviewDriver::renderFrame() test seam instead of
wall-clock delayMs; performance.md / testing.md / ui.md consistency fixes.
MoonDeck: device discover/refresh sped up (~3s -> ~0.6s) via a wide
ThreadPoolExecutor and a shorter probe timeout; Refresh button restyled to
match Discover.
Docs: plan-12 added; UI/Control/NetworkModule specs updated; the v1 repo URL
renamed github.com/ewowi/projectMM -> projectMM-v1 across the docs.
Pre-commit: desktop build clean (-Werror), 81 unit tests + 8 in-process
scenarios pass, platform boundary + spec check PASS, both ESP32 profiles build
clean (default 1004KB, eth-only 558KB). ESP32 flashed (eth-only) and live-
measured on Ethernet at ~43ms / 23 FPS. Reviewer agent PASS (one must-fix — a
stale password comment — applied). Live-scenario suite has known-accepted
failures: control-change / grid-resize expect modules named Mirror/Noise (the
device tree has MirrorModifier/RainbowEffect) and occasional min_pct misses are
documented ESP32 run-to-run variance, not regressions.
KPI details:
Desktop: 16,384 lights, 227KB binary, 81 tests pass, 8 scenarios pass
ESP32 (eth-only): 557,729 byte image (70% partition free), 485KB flash,
tick 43,247us / 23 FPS (Ethernet, no browser), 115KB heap free
Code: 35 source files (5,397 lines), 16 test files (2,080 lines), 18 lizard
warnings (json::parseString gained un-escape handling; writeControls grew)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ewowi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/JsonUtil.h (1)
17-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
std::spaninstead of pointer+length in parser API.
parseString(..., char* out, size_t maxLen)introduces a new pointer+length interface; this should be a span-based parameter per repo rule.As per coding guidelines: "Use
std::spanover pointer + length parameters".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/JsonUtil.h` at line 17, The parseString function signature and implementation should be changed to accept a std::span<char> for the output buffer instead of separate (char* out, size_t maxLen): update the signature inline void parseString(const char* json, const char* key, std::span<char> out), add `#include` <span>, and adjust the body to use out.size() for bounds checks, write at most out.size()-1 bytes then null-terminate (or handle empty span safely), and update all call sites to pass std::span (e.g., std::span(buf, bufLen) or std::as_writable_bytes) so pointer+length usage is removed throughout; keep behavior identical otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules_draft/light/effects/ArtNetReceiveEffect.md`:
- Around line 25-26: Insert a blank line immediately after the heading
"projectMM v1 — ArtNetInModule" (the Markdown heading at the start of the
section describing ArtNetInModule) so the paragraph that begins with "v1
ArtNetInModule..." is separated by an empty line, satisfying MD022 and ensuring
proper Markdown heading spacing.
In `@docs/moonmodules_draft/light/effects/DistortionWavesEffect.md`:
- Around line 25-26: Add a blank line after the heading "projectMM v1 —
DistortionWaves2DEffect" so the heading is followed by an empty line before the
paragraph "Two interfering sine waves..." to satisfy MD022; update the Markdown
near the DistortionWaves2DEffect heading in DistortionWavesEffect.md
accordingly.
In `@docs/moonmodules_draft/light/effects/GameOfLifeEffect.md`:
- Around line 26-27: Add a single blank line immediately after the "###
projectMM v1 — GameOfLifeEffect
([source](https://github.com/ewowi/projectMM-v1/blob/54b50bc/src/modules/effects/GameOfLifeEffect.h))"
heading so the section is separated from the following paragraph (this fixes
markdownlint rule MD022); ensure only one empty line is inserted and no other
content is changed.
In `@docs/moonmodules_draft/light/effects/LinesEffect.md`:
- Around line 11-12: The heading "### projectMM v1 — LinesEffect (
[source](https://github.com/ewowi/projectMM-v1/blob/54b50bc/src/modules/effects/LinesEffect.h))"
violates MD022 (missing required blank line); fix by inserting a single blank
line immediately after that heading line so the following paragraph ("v1
LinesEffect (commit 54b50bc). Simple rhythmic line pattern. Good for
music-reactive setups.") is separated from the heading.
In `@docs/moonmodules_draft/light/effects/SineEffect.md`:
- Around line 24-25: Add a blank line immediately after the heading "###
projectMM v1 — SineEffect
([source](https://github.com/ewowi/projectMM-v1/blob/54b50bc/src/modules/effects/SineEffect.h))"
in SineEffect.md so the heading is separated from the following paragraph (this
satisfies markdownlint MD022); locate the heading string and insert one empty
line right after it.
In `@docs/moonmodules/core/Control.md`:
- Around line 64-65: The two lines conflict on when persisted control values are
applied; standardize to: Load persisted values at setup before
onBuildControls(), and then apply those pending values during onBuildControls().
Update the text so both the "Load" description and the "When" description
reference the same model (Load at setup before onBuildControls(); persisted
values are applied in onBuildControls(); save on control change is still
debounced).
In `@docs/moonmodules/core/HttpServerModule.md`:
- Around line 53-54: Add a single blank line after the heading "### projectMM v1
— HttpServer + WsServer
([source](https://github.com/ewowi/projectMM-v1/blob/54b50bc/src/core/HttpServer.h))"
so the following paragraph ("HTTP via cpp-httplib...") is separated from the
heading (fixes MD022); ensure there is exactly one empty line between the
heading and the paragraph.
In `@docs/moonmodules/core/MoonModule.md`:
- Around line 101-105: Add a blank line after each Markdown subheading
"projectMM v1 — StatefulModule" and "projectMM v2 — MoonModule" so the list
items following those headings are separated (this fixes MD022); update the
lines containing those headings in MoonModule.md to insert one empty line before
the subsequent list item entries under each heading.
In `@docs/moonmodules/core/Scheduler.md`:
- Around line 31-32: The markdown heading "### projectMM v1 — Scheduler
([source](https://github.com/ewowi/projectMM-v1/blob/54b50bc/src/core/Scheduler.h))"
must be followed by a single blank line to satisfy MD022; update the
Scheduler.md content by inserting one empty line immediately after that heading
so the subsequent paragraph/listing ("- Time-sliced dispatch: setup, loop,
loop20ms, loop1s for all modules. Single-threaded.") is separated from the
heading.
In `@docs/moonmodules/light/Buffer.md`:
- Around line 42-43: Add a single blank line immediately after the markdown
heading "### projectMM v1 — Channel" so there is an empty line between that
heading and the following paragraph; this fixes the MD022 lint error by ensuring
the heading is separated from the paragraph content.
In `@docs/moonmodules/light/DriverGroup.md`:
- Around line 31-32: The markdown heading "### projectMM v1 — DriverLayer
([source](https://github.com/ewowi/projectMM-v1/blob/54b50bc/src/modules/layers/DriverLayer.h))"
is missing a blank line after it (MD022); add a single empty line between that
heading and the following paragraph "Container for driver modules. Receives
pixel data from EffectsLayer." so the heading is separated from the body text.
In `@docs/moonmodules/light/drivers/ArtNetSendDriver.md`:
- Around line 40-41: Add a blank line after the Markdown heading "projectMM v1 —
ArtNetOutModule" to satisfy MD022; edit the section in ArtNetSendDriver.md where
the heading appears and insert an empty line before the following paragraph that
begins "Controls: universe_start (slider 0-255), ip (text). Platform UDP
abstraction." so the heading and its description are separated.
In `@docs/moonmodules/light/drivers/PreviewDriver.md`:
- Around line 43-47: The two subheadings "### MoonLight — PhysicalLayer +
WebSocket" and "### projectMM v1 — PreviewModule" are not followed by a blank
line; add a single empty line after each of these heading lines so that each
heading is separated from the subsequent paragraph text (fix the MD022 linting
issue).
In `@docs/moonmodules/light/effects/NoiseEffect.md`:
- Around line 36-37: The heading "projectMM v1 — NoiseEffect2D" is not separated
from the following paragraph; add a single blank line between this heading and
the paragraph (fixing MD022) so the heading at "projectMM v1 — NoiseEffect2D" is
followed by one empty line before the "Hash-based value noise..." paragraph.
In `@docs/moonmodules/light/LightConfig.md`:
- Around line 35-36: The heading "### projectMM v1 — RGB" in LightConfig.md
violates MD022 (missing blank line); fix by inserting a single empty line
directly after the heading so the following paragraph ("Plain 3-byte RGB
struct...") is separated; update the file docs/moonmodules/light/LightConfig.md
by adding that blank line immediately after the "projectMM v1 — RGB" heading.
In `@docs/performance.md`:
- Around line 75-76: The docs currently claim that collect_kpi.py --commit
enforces the absolute min_fps_led_product floor using a "settled median", but
the script actually gates on a single parsed tick sample; update the sentence(s)
referring to --commit and "settled median" to state that the --commit gate
evaluates a single parsed tick sample (so it can flag an unlucky slow sample)
and retain the guidance to re-run live-scenario failures before treating a 1-FPS
miss (references: collect_kpi.py --commit, min_fps_led_product, min_pct); keep
the note that the absolute floor is enforced as a hard gate but clarify it uses
the single-sample parsing behavior rather than a median.
In `@scripts/check/collect_kpi.py`:
- Around line 360-377: The ESP32 hard-throughput gate (the block that reads
esp32_tick, computes max_tick from lights and MIN_ESP32_FPS_LED_PRODUCT and
calls sys.exit(1) on failure) is currently applied for normal report runs;
restrict it to only fail the process when running in commit mode by wrapping the
failure check in the commit flag guard (e.g., if args.commit or a similar
commit_mode variable): compute esp32_tick, lights and max_tick as now, but if
esp32_tick > max_tick then either print the same FAIL message and only
sys.exit(1) when commit mode is true; in non-commit mode print a non-fatal
warning instead so interactive KPI collection does not abort.
In `@scripts/scenario/run_live_scenario.py`:
- Around line 69-70: The int conversions of w, h, d in the light-count
accumulation can raise ValueError on malformed control values; update the block
that updates total (the code using variables w, h, d and total) to wrap the
int(...) calls in a try/except (catch ValueError and TypeError), skip (continue)
or treat the triple as zero on error, and emit a warning/log (using the existing
logger) identifying the bad values and the context so the scenario run continues
instead of aborting. Ensure you only change the block that multiplies int(w) *
int(h) * int(d) and preserve existing control-flow and total accumulation
semantics when values are valid.
In `@src/light/ArtNetSendDriver.h`:
- Around line 23-28: The code binds the destination once in setup() with
socket_.connect(ip, ARTNET_PORT) but never reconnects if the ip control changes,
so runtime control updates still send to the old address; change the driver to
detect ip changes and re-establish the connected socket before sending (e.g.,
store the last_connected_ip, compare it to the current ip in the send path or in
the ip control setter, and if different close/reopen or call socket_.connect(ip,
ARTNET_PORT) again), updating the logic surrounding socket_.connect and the send
path (the code that currently sends via the connected socket_ at the send
location) and ensure any reconnection is done safely with respect to threading
and socket state.
In `@src/ui/index.html`:
- Around line 19-21: The three icon-only header buttons (elements with
id="ws-reconnect", id="reboot-btn", and id="theme-toggle") rely on title for
identification which is not a reliable accessible name; add explicit aria-label
attributes to each button (e.g., aria-label="Reconnect WebSocket",
aria-label="Reboot device", aria-label="Toggle light/dark theme") so screen
readers receive a clear name while keeping the existing title and visual icon
unchanged.
In `@test/scenario_runner.cpp`:
- Around line 186-197: The code currently assumes IDs map to the expected types
and uses static_cast (e.g., static_cast<mm::Layer*>(modules[props["layer"].str])
and static_cast<mm::DriverGroup*>(mod)) which can produce UB if the ID points to
a different module type; change these lookups to a type-safe resolver: implement
and use a helper like getModuleAs<T>(modules, id) that checks modules.count(id),
obtains modules[id], and performs a runtime-checked cast (dynamic_cast or an
explicit type-tag check) returning nullptr on mismatch, then only call
setLayoutGroup, setChannelsPerLight, setLayer, etc., when the returned pointer
is non-null; replace all occurrences around the Layer/DriverGroup handling (and
the other occurrences at 324-327) to use this helper so you never static_cast
blindly.
---
Outside diff comments:
In `@src/core/JsonUtil.h`:
- Line 17: The parseString function signature and implementation should be
changed to accept a std::span<char> for the output buffer instead of separate
(char* out, size_t maxLen): update the signature inline void parseString(const
char* json, const char* key, std::span<char> out), add `#include` <span>, and
adjust the body to use out.size() for bounds checks, write at most out.size()-1
bytes then null-terminate (or handle empty span safely), and update all call
sites to pass std::span (e.g., std::span(buf, bufLen) or std::as_writable_bytes)
so pointer+length usage is removed throughout; keep behavior identical
otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 695b4dab-b62b-4f90-89ec-eb6c79034f80
⛔ Files ignored due to path filters (3)
scripts/build/build_esp32.pyis excluded by!**/build/**scripts/build/build_esp32_ethonly.pyis excluded by!**/build/**src/ui/moonlight-logo.pngis excluded by!**/*.png
📒 Files selected for processing (67)
.coderabbit.yamlCLAUDE.mdREADME.mddocs/architecture-light.mddocs/architecture.mddocs/history/plan-11.mddocs/history/plan-12.mddocs/moonmodules/core/Control.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/Scheduler.mddocs/moonmodules/core/ui.mddocs/moonmodules/light/Buffer.mddocs/moonmodules/light/DriverGroup.mddocs/moonmodules/light/EffectBase.mddocs/moonmodules/light/Layer.mddocs/moonmodules/light/LayoutGroup.mddocs/moonmodules/light/LightConfig.mddocs/moonmodules/light/MappingLUT.mddocs/moonmodules/light/drivers/ArtNetSendDriver.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/effects/RainbowEffect.mddocs/moonmodules/light/layouts/GridLayout.mddocs/moonmodules_draft/core/ui.mddocs/moonmodules_draft/light/effects/ArtNetReceiveEffect.mddocs/moonmodules_draft/light/effects/DistortionWavesEffect.mddocs/moonmodules_draft/light/effects/GameOfLifeEffect.mddocs/moonmodules_draft/light/effects/LinesEffect.mddocs/moonmodules_draft/light/effects/RipplesEffect.mddocs/moonmodules_draft/light/effects/SineEffect.mddocs/performance.mddocs/plan.mddocs/testing.mdesp32/main/CMakeLists.txtesp32/sdkconfig.defaultsscripts/MoonDeck.mdscripts/check/collect_kpi.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/style.cssscripts/scenario/run_live_scenario.pysrc/core/Control.hsrc/core/FilesystemModule.hsrc/core/HttpServerModule.hsrc/core/JsonUtil.hsrc/core/NetworkModule.hsrc/core/PreviewFrame.hsrc/light/ArtNetSendDriver.hsrc/light/PreviewDriver.hsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/embed_ui.cmakesrc/ui/index.htmlsrc/ui/style.csstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/base-pipeline.jsontest/scenarios/preview-detail.jsontest/test_preview_driver.cpptest/test_system_module.cpp
Comment on lines
+186
to
+197
| auto* layer = static_cast<mm::Layer*>(mod); | ||
| if (props.has("layoutGroup")) { | ||
| auto* lg = static_cast<mm::LayoutGroup*>(modules[props["layoutGroup"].str]); | ||
| if (lg) layer.setLayoutGroup(lg); | ||
| if (lg) layer->setLayoutGroup(lg); | ||
| } | ||
| if (props.has("channelsPerLight")) { | ||
| layer.setChannelsPerLight(static_cast<uint8_t>(props["channelsPerLight"].num)); | ||
| layer->setChannelsPerLight(static_cast<uint8_t>(props["channelsPerLight"].num)); | ||
| } | ||
| } else if (std::strcmp(type, "DriverGroup") == 0) { | ||
| if (props.has("layer")) { | ||
| auto* l = static_cast<mm::Layer*>(modules[props["layer"].str]); | ||
| if (l) driverGroup.setLayer(l); | ||
| if (l) static_cast<mm::DriverGroup*>(mod)->setLayer(l); |
There was a problem hiding this comment.
Use type-safe module resolution instead of ID-coupled static_cast.
These casts trust IDs and expected types from scenario JSON. A mismatched ID/type can cause UB, and fixed-key lookup ("Layer", "DriverGroup") can skip checks even when a valid layer exists under a different ID.
Suggested patch (type-based lookup helper)
+static mm::MoonModule* findModuleByType(const std::map<std::string, mm::MoonModule*>& modules,
+ const char* typeName) {
+ for (const auto& [_, mod] : modules) {
+ if (mod && mod->typeName() && std::strcmp(mod->typeName(), typeName) == 0) return mod;
+ }
+ return nullptr;
+}
...
- auto* layer = static_cast<mm::Layer*>(ctx.modules.count("Layer")
- ? ctx.modules["Layer"] : nullptr);
- auto* driverGroup = static_cast<mm::DriverGroup*>(ctx.modules.count("DriverGroup")
- ? ctx.modules["DriverGroup"] : nullptr);
+ auto* layer = static_cast<mm::Layer*>(findModuleByType(ctx.modules, "Layer"));
+ auto* driverGroup = static_cast<mm::DriverGroup*>(findModuleByType(ctx.modules, "DriverGroup"));Also applies to: 324-327
🧰 Tools
🪛 Clang (14.0.6)
[warning] 186-186: variable 'layer' is not initialized
(cppcoreguidelines-init-variables)
[warning] 188-188: variable 'lg' is not initialized
(cppcoreguidelines-init-variables)
[warning] 188-188: variable name 'lg' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 196-196: variable 'l' is not initialized
(cppcoreguidelines-init-variables)
[warning] 196-196: variable name 'l' is too short, expected at least 3 characters
(readability-identifier-length)
🪛 Cppcheck (2.20.0)
[style] 191-191: The function 'loopTimeUs' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/scenario_runner.cpp` around lines 186 - 197, The code currently assumes
IDs map to the expected types and uses static_cast (e.g.,
static_cast<mm::Layer*>(modules[props["layer"].str]) and
static_cast<mm::DriverGroup*>(mod)) which can produce UB if the ID points to a
different module type; change these lookups to a type-safe resolver: implement
and use a helper like getModuleAs<T>(modules, id) that checks modules.count(id),
obtains modules[id], and performs a runtime-checked cast (dynamic_cast or an
explicit type-tag check) returning nullptr on mismatch, then only call
setLayoutGroup, setChannelsPerLight, setLayer, etc., when the returned pointer
is non-null; replace all occurrences around the Layer/DriverGroup handling (and
the other occurrences at 324-327) to use this helper so you never static_cast
blindly.
CodeRabbit review fixes for commit 707f6ba: - MD022: insert the required blank line after every spec-doc heading (61 violations project-wide, not just the flagged subset, so the files stay consistent) - ArtNetSendDriver: re-bind the connected UDP socket when the `ip` control changes at runtime — it was bound once in setup(), so a UI ip change silently kept sending to the old address until reboot - collect_kpi.py: the ESP32 throughput gate now aborts only in --commit mode; a plain interactive report warns instead of exiting non-zero - run_live_scenario.py: guard the light-count int() math against non-numeric control values (warn + skip instead of crashing) - index.html: add aria-label to the icon-only header buttons for screen readers (title alone is not a reliable accessible name) - Control.md: clarify the load-at-setup / apply-in-onBuildControls persistence model so the two lines no longer read as conflicting - performance.md: correct the collect_kpi --commit gate description — it parses a single tick sample, not a "settled median" Skipped (with reason): the std::span suggestion for JsonUtil::parseString (pure-style churn on a just-reviewed API) and a dynamic_cast resolver in scenario_runner.cpp (defensive code for malformed test-authored JSON — the static_cast is keyed on a strcmp type-match that already succeeded). Branch-merge prep (CLAUDE.md branch-merge procedure): plans 08-12 reviewed and moved to docs/history/archive/; lessons from the next-iteration branch distilled into docs/history/decisions.md. Reduced pre-commit (docs + scripts + small C++ fix — no render-loop change): desktop build clean, 81 unit tests + 8 scenarios pass, platform boundary + spec check PASS. ESP32 build/flash/KPI skipped — nothing here affects ESP32 render-loop timing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai, here some review feedbacks: let me know if we are fine to merge Fixed (8 findings)
Fixed (still-valid issues)
Fixed (9 findings)
|
|
Tip For best results, initiate chat on the files or code changes.
All three rounds of findings have been cleanly triaged:
The JSON string escaping fix (round 3 All skips are correctly reasoned against CLAUDE.md policy (specs-before-code, no speculative fields, frozen history docs, cosmetic-only refactors). Would you like me to:
🐇 ✨ ✏️ Learnings added
|
This was referenced May 24, 2026
ewowi
added a commit
that referenced
this pull request
May 25, 2026
…acklog
- .github/workflows/release.yml: pass tag via step-level env: TAG instead
of direct ${{ inputs.tag || github.ref_name }} template interpolation in
the run line. Same value, same behaviour; eliminates the static-analyser
template-injection warning (CodeRabbit #2). Mirrors the $REPO pattern
the release job already uses.
- src/ui/app.js: rewrite the SystemModule-lookup comment in present tense.
CLAUDE.md is explicit about no forward-looking phrasing ('future spec ever
drops type from the wire shape'). Code is unchanged — m.type lookup with
m.name fallback as a belt-and-braces safety net (CodeRabbit #16).
- docs/plan.md: backlog entry for std::span migration on the new platform
API surface (CodeRabbit #15). Not RC2 scope — touches every caller; worth
doing alongside the next platform-API expansion (Windows sockets / POST
/api/firmware streaming).
The remaining 18 CodeRabbit findings on PR #7 are either already addressed
in fac6a00 / 44068e5 (review was running against earlier commits and the
fix-pack landed in those) or false positives where 'platform/platform.h' is
the legitimate boundary API a core module imports — not platform-specific
code in a core file.
Pre-commit gates: desktop build clean. No new src/ logic changed (one
JS comment + one workflow var pattern + one docs entry); no test impact.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 27, 2026
ewowi
added a commit
that referenced
this pull request
Jun 3, 2026
Moondeck.json restructures from a flat device list + single port into named
networks, each holding its own devices, last-used serial port, and WiFi
credentials. Header network bar auto-selects from host's current subnet on
load; manual override pins. Improv WiFi reads the active network's creds —
wifi_credentials.json retired. Existing moondeck.json migrates in place.
KPI: n/a (no functional src/ changes — only comments in scenario_runner.cpp)
Tests
- scenario_runner.cpp: KEEP IN SYNC comment pointing at run_live_scenario.py tolerance defaults.
- unit_ArtNetSendDriver_no_alloc_in_loop.cpp: present-tense ("would need" → "needs", "grow happens" → "grow runs"). [CodeRabbit]
- unit_FilesystemModule_persistence.cpp: present-tense ("would have caught" → "catches"). [CodeRabbit]
- scenario_PreviewDriver_detail.json: present-tense descriptions ("must still hit" → "still hits", "must not affect" → "does not affect"). [CodeRabbit]
- All scenario JSONs: routine observed-range widenings from gate sweeps.
Scripts / MoonDeck
- moondeck.py: load_state() detects legacy flat shape and runs _migrate_to_networks (buckets by /24 subnet, largest bucket "Home"); save_state() strips volatile per-device fields including conditionally board when it equals the deduced value; new _active_network / _auto_select_network / _deduce_board helpers; /api/state GET auto-selects on host subnet, /api/discover attributes found devices to matching network, /api/refresh refreshes single network's devices; flash-event breadcrumb still links last_port. NAMING COLLISION breadcrumb added to _deduce_board docstring pointing at docs/plan.md. KEEP IN SYNC note between _deduce_board and the JS board picker list. contextlib.suppress for three inline try/except sites. [CodeRabbit #5]
- moondeck_ui/app.js: getActiveNetwork() helper; renderNetworkBar() + setupNetworkBar() build the dropdown, Rename/Add buttons, WiFi panel; applyNetworkBarVisibility() hides the bar on PC tab; every state.devices / state.port site re-routed through the active network; cross-ref comment at the hardware-board picker pointing at moondeck.py::_deduce_board.
- moondeck_ui/index.html: network-bar div above the per-tab content in the sidebar.
- moondeck_ui/style.css: .network-bar + .network-wifi styling; .device-board picker layout fix.
- build/host_wifi.py: primary credentials source is the active network's wifi block in moondeck.json; OS auto-detect fallback stays.
- check/collect_kpi.py: 18 FPS banner → derived from MIN_ESP32_FPS_LED_PRODUCT (currently 10). [CodeRabbit #3]
- scenario/run_live_scenario.py: max_alloc_block 0 fails when contract demands >0 (was silently passing). [CodeRabbit #6]; KEEP IN SYNC comment about tolerance defaults mirroring scenario_runner.cpp.
- scenario/_observed.py: shared widen-only range update for observed.<target> blocks (factored from both runners).
- docs/screenshot_modules.py: --extras-only flag + _ExtrasOnlyDone sentinel; lets MoonDeck UI re-shoots run without projectMM.
- build/flash_esp32.py: writes scripts/.last_flash.json breadcrumb on success; MoonDeck consumes it to attribute last_port.
Deleted
- scripts/build/wifi_credentials.example.json — the live source moved into moondeck.json's network records.
- .gitignore entry for scripts/build/wifi_credentials.json.
Docs
- testing.md: new Live tab screenshot; observed-range model wording.
- building.md: MoonDeck PC tab + ESP32 tab screenshots placed in their sections.
- MoonDeck.md: Network bar UI Features bullet; improv_provision section updated for the new active-network credentials flow.
- docs/plan.md: "Board vs firmware separation, runtime board presets (multi-commit, started)" roadmap.
- docs/tests/unit-tests.md + scenario-tests.md: regenerated (esp32 row now included in PreviewDriver tables from stale-doc fix). [CodeRabbit #1]
- assets/screenshots/moondeck_{pc,esp32,live}.png: re-captured with the network bar.
Reviews
- 🐰 CodeRabbit: 6 fixed (#1 esp32 row from stale doc → regen; #2 unit-test tense; #3 18 FPS → 10 FPS; #5 contextlib.suppress; #6 max_alloc_block 0 fails; #7 scenario descriptions present-tense). 1 skipped: #4 (show tolerated contract values in perf table) — conflates contract (promise) with tolerance (measurement noise); reviewer agent agreed.
- 👾 Reviewer (Opus, branch diff): ship with changes; 3 architectural calls accepted (sync comments for tolerance defaults, board naming-collision breadcrumb, hardware-board catalog cross-ref) + 1 minor refactor (FPS helper docstring extended to name shared core).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
10 tasks
ewowi
added a commit
that referenced
this pull request
Jun 4, 2026
Adds the LOLIN S3 N16R8 board entry to the catalog and the full chain
of fixes it needed: a writable WiFi-TX-power cap to prevent the
on-module LDO brown-out, USB-Serial-JTAG support in the Improv
listener so the device responds on native-USB S3 boards, a CORS
preflight handler so cross-origin POSTs from the web installer
actually reach `/api/control`, and a generic per-board controls
fan-out across MoonDeck and the installer paths so future per-board
fields land without code changes. Web installer also gains a
Monitor button + retry affordance, and `maxBlock` now reports
internal-RAM block (useful) instead of PSRAM block (always ~8MB).
KPI: tick:153831us(FPS:6) on esp32s3-n16r8 with txPower capped to
8 dBm (LOLIN brown-out fix). Slower than full-power ArtNet because
8 dBm cuts radio TX margin — known hardware trade-off, not a code
regression.
Core:
- NetworkModule: added writable `txPowerSetting` (0..21 dBm, 0=no
override) re-applied on change via syncTxPower(). Setting back to 0
actively pushes 80 quarter-dBm to undo any prior cap (no IDF
"reset to default" API exists; sticky-cap was the previous behavior).
- BoardModule: ASCII-printable validation rationale documented at
setBoard() (JSON encoding, UI rendering, C-string round-trip).
- HttpServerModule: added OPTIONS preflight handler returning 204 +
CORS headers. Root cause of every "cross-origin POST silently
blocked" symptom — the orchestrator's HTTP inject from preview now
lands properly. Also documented path-agnostic preflight choice.
- SystemModule: maxBlock display switched to maxInternalAllocBlock;
added comment explaining how PSRAM is detected (heap-size derivation,
not a flag).
Platform:
- platform.h / platform_esp32.cpp / platform_desktop.cpp: new
`maxInternalAllocBlock()` API alongside existing `maxAllocBlock`.
Internal-only block is the memory-pressure KPI; the all-memory
variant reports ~8 MB on PSRAM boards and tells you nothing.
Layer::canAllocate keeps the all-memory call (light buffers may
live in PSRAM); every diagnostic site (SystemModule, main.cpp tick
log, HttpServerModule /api/state, scenario_runner) now uses the
internal-only variant.
- platform.h / platform_esp32.cpp / platform_desktop.cpp: new
`wifiSetTxPower(int8_t quarterDbm)` setter wrapping
esp_wifi_set_max_tx_power. Clamps into ESP-IDF's 8..84 range.
- platform_esp32_improv.cpp: USB-Serial-JTAG read+write path added
alongside UART0 (guarded by SOC_USB_SERIAL_JTAG_SUPPORTED). LOLIN
S3 N16R8 and other native-USB S3 boards expose USB-C through
USB-Serial-JTAG, not UART0 — without this the Improv task was deaf
to host writes on those boards. Both transports drained
symmetrically each loop with a single 10 ms yield when both are
empty; ESP_LOGW for driver-install warnings + compound
`"listening (uart unavailable)"` status so partial-transport
failures stay visible.
UI:
- src/ui/install-picker.js: added `installRowExtras` slot so the
installer page can slot the Erase-chip checkbox between firmware
dropdown and Install button without the on-device OTA UI inheriting
installer-specific affordances.
- src/ui/app.js: sendControl() now logs non-ok HTTP + network errors
to console.warn (no retry — design intent for the single-shot
`?board=` consume path).
Scripts / MoonDeck:
- MoonDeck `_push_board_to_device` now fans out the full
`controls.<Module>.<control>` block from boards.json on every push
(was Board.board only). Falls back to bare-board for catalog-
missing names; no-op on empty board. Generic — any future per-board
control in boards.json lands via MoonDeck without code changes.
- docs/install/boards.json: new "LOLIN S3 N16R8" entry injecting
`Network.txPowerSetting: 8` (8 dBm — well below the ~13 dBm
threshold ESPHome documents for the brown-out symptom).
- docs/install/index.html: rich installer-UI changes — Monitor
button + serial-monitor modal (Reset/Clear/Close, autoscroll,
UTF-8 decode, RTS-pulse reset), Retry button on the "Improv not
detected" dialog with stale-port probe + 250 ms post-close wait
for SDK lock release, lazy-render WiFi-creds form (avoids macOS
iCloud Passwords prompt on page load), Skip-creds path exits
cleanly via onSuccess({url:""}) rather than failing in provision(),
HTTP injection runs on the Improv-success path too (was needsIp-
only — fixes the "txPower not applied after Improv install"
symptom), 2 s → 3 s post-flash port-reopen wait (boot race against
Improv task init on S3), monitor button auto-disabled during
install (Web Serial port-mutex enforcement), pendingBoard fallback
now covers Improv-success HTTP fail.
- docs/install/install-orchestrator.js: stale-prePickedPort probe
with fallback to requestPort() in both start() and eraseOnly();
isTransientImprovError helper extracted next to isImprovNotDetected
so SDK error-string drift has one update site; uiShowNeedsIpRetrying
contract reconciled with actual behavior.
- docs/install/devices.js: pendingBoard explicitly cleared on
successful inject (was leaking across re-installs); erase confirm
text rewritten to reflect the orchestrator-driven flow (EWT
branding removed).
Docs / CI:
- docs/install/README.md, scripts/MoonDeck.md: replaced "ESP Web
Tools installer page" branding with the orchestrator-driven
description (present tense, no backward-looking change-log talk).
- docs/moonmodules/core/BoardModule.md: MoonDeck section updated to
reflect full controls fan-out; "Pre-WiFi limitation" paragraph
trimmed to a one-line pointer; full timing-constraint rationale
moved to docs/history/decisions.md as the lesson worth carrying
forward.
- docs/moonmodules/core/NetworkModule.md: txPowerSetting bullet
added (trimmed to wire contract + intended use).
- docs/history/decisions.md: new "Board-injection pipeline timing
constraint" entry — names the controls that wouldn't tolerate the
HTTP-after-WiFi fan-out (country code, antenna selector, pre-
association TX-power) and the two escape hatches (new vendor RPC,
sdkconfig bake). Warns against extending SET_BOARD's wire format.
Tests:
- test/scenario_runner.cpp: switched to maxInternalAllocBlock for
the regression KPI; updated rationale to explain why (PSRAM
boards previously masked internal-heap fragmentation).
- test/unit/core/unit_BoardModule.cpp: added c.readonly assertion
on the board control — regression guard against accidentally
making the BoardModule.board field user-editable.
Reviews:
- 👾 Reviewer (on-demand pre-commit): 0 blockers, 10 should-fix, 12
nits — all 22 addressed in this commit. Highlights: #1
syncTxPower=0 now actively lifts the cap (was sticky until reboot);
#2 pendingBoard fallback covers the Improv-success HTTP-fail path
the LOLIN feature most needs to defend; #4 Monitor button now
actually disabled during install (matched the documented mutex);
#5 Improv driver-install warnings now logged via ESP_LOGW and
carried in compound status (were silently overwritten by
"listening"); #7 JTAG TX timeout 50 ms → 0 (truly non-blocking);
#8 UART/JTAG polling made symmetric. The remaining items are
comment trims, sentinel-vs-tagged-union docstring clarifications,
and the 32-vs-34-quarter-dBm example fix in platform.h.
- 🐇 CodeRabbit (prior round): 10 findings processed earlier in the
branch; 2 rejected with reason (HttpServerModule single-line `if
(c.readonly)` brace fix — no clang-tidy config in the repo, and
the codebase has many single-line ifs; consumePendingBoardParam
retry-token — contradicts the documented "no retry" single-shot
design).
KPI Details:
Desktop:
Lights: 16,384
Binary: 375 KB
[doctest] test cases: 207 | 207 passed | 0 failed | 0 skipped
tick: 352us, 99us, 304us, 100us, 99us, 100us, 45us, 35us, 35us
(FPS: 2840, 10101, 3289, 10000, 10101, 10000, 22222, 28571, 28571)
=== 10 scenario(s), 10 passed, 0 failed ===
Platform boundary: PASS
Specs: 26 modules, 26 ok, 0 missing, 0 outdated
ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm):
Image: 1,307,517 bytes (69% partition free)
Flash (code+data): 1171 KB
tick: 153831us (FPS: 6)
free heap: 8354995 maxBlock (internal): 163840 (160 KB)
ArtNetSend: ~105 ms/tick (LOLIN at 8 dBm — known trade-off)
Code:
62 source files (10491 lines)
38 test files (5410 lines)
39 specs, 10 scenarios
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 4, 2026
Four CodeRabbit findings folded in (three with leaner shapes than proposed), one deferred with reason. Most user-visible: syncTxPower() now runs immediately after every successful wifiStaInit / wifiApInit, so the LOLIN brown-out cap lands BEFORE the first association burst rather than up to 1 s after — closes the window the cap exists to defend. Also: Improv replies now go only on the transport that asked, the needs-ip dialog catches whitespace-only input via the browser's own validation tooltip, and the BoardModule.h ASCII-validation asymmetry between SET_BOARD-over-Improv (validates) and HTTP /api/control (doesn't) is documented explicitly. Plus an unrelated docs addition: the NanoDLA v1.3 logic analyzer is named as the project's reference fx2lafw device with wiring + capture instructions for measuring ESP32 WS2812 output. KPI: tick:138506us(FPS:7) on esp32s3-n16r8 at txPower=8 dBm. Within noise of last commit's 141ms/FPS:7; the syncTxPower-on-init change doesn't measurably affect the tick. Core: - NetworkModule: syncTxPower() called immediately after each of the 5 wifiStaInit / wifiApInit success sites (setWifiCredentials, setup, the two ethernet-cascade-to-WiFi paths in loop1s, and startAP). The comment at setWifiCredentials's call is the canonical one; the rest point back to it. Closes the up-to-1 s gap between WiFi-up and the next loop1s tick during which the radio was at full power. - BoardModule: documented the HTTP-write-bypasses-ASCII-validation asymmetry on setBoard()'s comment. The HTTP path uses the generic Text-control write at applyControlValue (Control.cpp) which does no printable-ASCII check. Acceptable today because HTTP-write callers (MoonDeck, web installer's HTTP inject) source values from boards.json (project-controlled). If the threat model grows, the right fix is a per-control validator hook on ControlDescriptor — named in the comment for the next reader. Platform: - platform_esp32_improv.cpp: per-transport reply routing. New static `g_replySource` set at the top of improvFeedByte's FrameReady branch and read inside improvSend; replies go ONLY on the transport that received the request rather than broadcast to both. The Improv task is single-threaded so a single static is sufficient; resets to ImprovSource::Both after dispatch so any unsolicited send (status broadcasts) keeps the prior shape. Lighter than threading an enum through every signature — same correctness outcome. UI: - docs/install/index.html: uiWaitForIp's submit handler now rejects whitespace-only input via setCustomValidity + reportValidity. Surfaces the browser's existing field-validation tooltip on the same field rather than letting normalizeDeviceUrl() return empty and the orchestrator silently re-prompt. The `required` attribute catches empty submits but not " " — trim first, then flag. Docs / CI: - docs/moonmodules/light/leddriver-analysis-top-down.md: named the NanoDLA v1.3 as the project's reference fx2lafw-class logic analyzer (replacing the generic "$10-15 fx2lafw clone" mention in the TL;DR and recommended-suite table) and added a new §5.3.1 "Wiring the NanoDLA v1.3 to an ESP32 and capturing one frame" — common-ground / signal-tap / power-isolation / strand-power wiring, voltage-level reasoning (3.3→3.3 direct, no shifter needed), sample-rate math for typical strands, copy-pasteable sigrok-cli capture + decode sequence, and common gotchas in the order they'll appear. Tests: - test/scenarios/light/scenario_PreviewDriver_detail.json: drift- tracking observed[pc-macos].tick_us[0] updated from 297→296. Normal CI noise persisted by scripts/scenario/run_scenario.py per the docs/testing.md drift-visibility contract. Reviews: - 🐇 CodeRabbit round 2: 4 findings; 3 fixed, 1 deferred. (a) #1 empty-IP validation: chose host-side setCustomValidity instead of proposed onValidationError callback (smaller, equivalent UX, no new API surface). (b) Outside #improv-routing: chose static- g_replySource instead of proposed signature-threading (same correctness, ~30 fewer touched call sites). (c) #4 syncTxPower- after-wifi-init: implemented as-proposed. (d) #2 unified validation between HTTP and Improv board-name paths: deferred — both clean fixes are architectural (per-control validator hook on ControlDescriptor, or per-module HTTP dispatch exception); the asymmetry is documented with the right fix-shape named, to be picked up when the threat model warrants it. KPI Details: Desktop: Lights: 16,384 Binary: 375 KB [doctest] test cases: 207 | 207 passed | 0 failed | 0 skipped tick: 354us, 100us, 303us, 99us, 99us, 98us, 45us, 35us, 34us (FPS: 2824, 10000, 3300, 10101, 10101, 10204, 22222, 28571, 29411) === 10 scenario(s), 10 passed, 0 failed === Platform boundary: PASS Specs: 26 modules, 26 ok, 0 missing, 0 outdated ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm): Image: 1,307,661 bytes (69% partition free) Flash (code+data): 1171 KB tick: 138506us (FPS: 7) free heap: 8358927 maxBlock (internal): 163840 (160 KB) ArtNetSend: ~93 ms/tick (LOLIN at 8 dBm — known trade-off) Code: 62 source files (10592 lines) 38 test files (5410 lines) 39 specs, 10 scenarios Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 5, 2026
Merged
ewowi
added a commit
that referenced
this pull request
Jun 12, 2026
…bit review Renames the audio module from MicModule to AudioModule (it does audio acquisition plus level/FFT analysis, and will gain line-in / USB sources beyond the I²S mic, so the name now reflects the module, not one source). Makes the primary-source references in the docs clickable (datasheets, Espressif API pages, vendor sites) per the spec-and-test principle. Processes the CodeRabbit review on the prior commit: six findings fixed, six skipped with a stated reason. KPI: 16384lights | PC:340KB | tick:121/89/119/122/56/2/37/21/117/342us(FPS:8264/11235/8403/8196/17857/500000/27027/47619/8547/2923) | tick:21177us(FPS:47) | heap:33066KB | src:87(16753) | test:53(8518) | lizard:68w Core: - MicModule.h → AudioModule.h (git mv, history preserved): class MicModule → AudioModule, registration string "MicModule" → "AudioModule", the latestFrame() static, the main.cpp wiring. The mic-source platform seam (hasI2sMic, audioMicRead, AudioMicHandle) is deliberately NOT renamed: it correctly names today's actual source; it broadens when line-in lands (concrete-first). Verified live on the S3: AudioModule registers and runs (AudioModule:523us in the tick breakdown), no boot-loop. Light domain: - AudioLevel.h: clamp the DcBlocker IIR output to the int32 range before narrowing (a settling transient could push the float past int32 bounds, which is UB on the cast). 🐇 #1. - AudioSpectrumEffect.h: fix a height-gradient off-by-one — the gradient divided by (h-1) but the spectrum spans specH rows; when the bottom row is the level meter (specH == h-1) the top spectrum row never reached full red. Now divides by (specH-1). 🐇 #4. - AudioVolumeEffect.h: defensively zero any per-light channels beyond RGB (buffers are RGB/cpl=3 today; this keeps the write correct for any cpl and leaves no stale bytes). 🐇 #3. Scripts / build: - idf_component.yml: pin esp_wifi_remote ~1.6.1 and esp_hosted ~2.12.9 (the P4-NANO-validated versions) instead of "*", so the P4 build can't silently drift to a new minor; also corrected the stale "esp_hosted bring-up prelude" comment (that prelude was removed). 🐇 #2. Verified the pins resolve and the P4 builds + boots. Docs / CI: - AudioModule.md (was MicModule.md): retitled; opening reframed as "acquires an audio source ... named for what it does, not one source" (present-tense, no claim line-in exists yet). Added primary-source links: INMP441 datasheet, esp-dsp, ESP-IDF I2S API, Hann window. First fully em-dash-free module spec. - RmtLedDriver.md / LcdLedDriver.md / ParlioLedDriver.md: linked WS2812B datasheet, ESP-IDF RMT v2 / esp_lcd / Parlio API pages, the v6.0 migration guide (legacy-RMT removal), ESP32-P4 product page. (Caught + fixed an agent-guessed Parlio URL that 404'd; all links verified live.) - SystemModule.md / building.md: linked ESP32-C6, esp_hosted, esp_wifi_remote, Waveshare ESP32-P4-NANO. - decisions.md: reworded the "designed fresh" audio lesson so it no longer reads as "don't look at prior art at all" (which fought the study-with-respect principle); now "reference proven behaviour, don't trace structure," with the flat-mic / no-correction-table call as the example. 🐇 #6. - AudioVolumeEffect.md: em-dashes removed (touched alongside the effect). 🐇 #7 (partial). - AudioSpectrumEffect.md / AudioFrame.h / test files: AudioModule rename propagated (@module annotations, references). Reviews — 🐇 CodeRabbit, prior commit (b79d54f): - Fixed #1 (DcBlocker int32 clamp), #2 (component version pins), #3 (AudioVolumeEffect extra-channel clear), #4 (AudioSpectrumEffect specH off-by-one), #6 (decisions.md prior-art wording), #7 (AudioVolumeEffect.md em-dashes). - Skipped #5 (architecture.md Buffer*/Correction* "pin at wiring time"): describes the identity-mapping fast path where the pointer is genuinely stable; the suggestion proposes a different architecture, not a doc fix; paragraph untouched this branch. - Skipped #7 (README.md / backlog.md bulk em-dash sweep): the rule is "as files are touched, not a single sweep"; a ~176-dash reformat is its own task. - Skipped #8 (AudioModule stale frame): already handled — latestFrame() returns the static silent frame when active_ is null (teardown nulls it); reinit keeps the same active mic and refreshes within one loop. - Skipped #9 (AudioModule platform boundary): not a violation — the boundary check passes and the neutral platform.h facade is included by ~10 core modules; the rule forbids platform-specific code outside src/platform/, not the abstract interface. An IAudioSource indirection is exactly the bespoke abstraction the architecture avoids. - Skipped #10 (desktop improv signature): decl and definition match exactly; the desktop build passes, which proves it. - Skipped #11/#12 (platform_esp32.cpp fcntl error-handling / include): pre-existing code not changed this branch; expanding into untouched code violates minimal-change scope. Hardware verified this commit: S3 boots clean at 225 FPS with AudioModule live; P4 boots clean at 60 FPS on Ethernet with the pinned components resolved and wifiCoproc reporting "not detected" (the known C6-slave-firmware blocker, unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 12, 2026
… fixes Processes the Event-2 (pre-merge) Reviewer pass over the branch. The headline is a structural deduplication of the three LED drivers: the parallel WS2812 drivers (S3 LCD_CAM i80, P4 Parlio) were ~250 of ~370 lines byte-for-byte identical, now one shared CRTP base. Also fixes four stale doc/comment items, makes the AudioModule enabled-toggle actually stop its FFT cost, and removes two dead platform flags. No control names or output behaviour change; the LED hot path is unchanged (measured zero overhead, see below). KPI: 16384lights | PC:340KB | tick:116/95/118/115/56/1/37/20/113/332us(FPS:8620/10526/8474/8695/17857/1000000/27027/50000/8849/3012) | ESP32:1178KB | tick:17341us(FPS:57) | heap:33068KB | src:88(16543) | test:53(8518) | lizard:65w Light domain (the dedup, per the No-duplication rule): - ParallelLedDriver.h (new): a CRTP base ParallelLedDriver<Derived> holding the shared body of the parallel drivers — the pins/ledsPerPin/loopback controls, parseConfig, the per-ROW fused correct+encode loop(), reinit/deinit, the loopback self-test. Binding is CRTP (static polymorphism), NOT a virtual second hierarchy: the base calls derived()->busX() resolved at compile time, no vtable, no per-light indirection — so it stays inside the data-over-objects / hot-path rules and "the one deliberate class hierarchy is the module tree" rule. - LcdLedDriver.h: 378 → 92 lines, now a derived shell supplying only the i80-specific pieces (clockPin/dcPin, the exactly-8-pins rule via kExactLaneCount, the platform::lcdWs2812* calls). - ParlioLedDriver.h: 362 → 82 lines, the simpler shell (no clock/dc, kExactLaneCount=false, kClockHz). - Drivers.h (DriverBase): absorbs the status-string lifecycle (configErr_/failBuf_, setConfigErr/clearConfigErr/failBufEnsure/clearFailBuf) that was triplicated verbatim across all three drivers (RMT too). - RmtLedDriver.h: loses its status-lifecycle copy (now inherited); symbol-per-light model stays standalone (genuinely different from the parallel drivers — concrete-first boundary). Core: - AudioModule.h: removed respectsEnabled()=false so the Scheduler honours `enabled` — disabling the module now skips loop() entirely, stopping the FFT (the real per-tick cost). Verified on hardware: 524us enabled -> 0us disabled. The FFT is never gated while enabled (it is the capability audio effects consume, not an optional cost). - platform_config.h (esp32 + desktop): removed the dead isEsp32/isEsp32S3 family flags (no users anywhere); kept isEsp32P4 (drives ethPins + hasWifiCoprocessor). Desktop now mirrors with isEsp32P4=false. Docs / CI: - architecture.md: § Firmware vs board now lists the two P4 firmwares (esp32p4-eth, esp32p4-eth-wifi); § Drivers updated for the LCD_CAM/Parlio drivers and the four-driver shared Correction pointer. (👾 MUST-FIX 1, 2) - sdkconfig.defaults.esp32p4-eth-wifi: corrected the comment that described an esp_hosted bring-up prelude the code deliberately does NOT do (it was removed; esp_hosted self-inits at boot). Following the stale comment would reintroduce the bench-fixed SDIO teardown. (👾 MUST-FIX 3) - platform_esp32_lcd.cpp: the header + #else/#endif comments now name the correct SOC_LCDCAM_I80_LCD_SUPPORTED macro (they said SOC_LCD_I80_SUPPORTED — the exact macro confusion that caused the classic-ESP32 boot loop). (👾 MUST-FIX 4) - check_specs.py: skip CRTP template bases (template<...> class X : public DriverBase) — shared infrastructure, not a registered module; the concrete derived classes still carry the docs. - backlog.md: the LED-driver duplication item updated (driver-logic dedup landed via the CRTP base; the platform loopback capture+verify extraction remains as a follow-up). Reviews — 👾 Reviewer (Fable 5, over git diff main...HEAD; verdict "merge with noted fixes"): - Fixed all 4 MUST-FIX (stale docs/comments, above). - SHOULD-CONSIDER #1/#2 (Lcd/Parlio driver-logic + status duplication): FIXED via the CRTP base + DriverBase status lifecycle (chose to do it now per the No-duplication rule rather than backlog). The remaining platform loopback capture+verify duplication is the tracked follow-up. - SHOULD-CONSIDER #4 (AudioModule enabled toggle couldn't stop the FFT): FIXED (above); the FFT itself is must-have, only the disabled-still-runs gap was the bug. - SHOULD-CONSIDER #5 (unused isEsp32/isEsp32S3 flags): FIXED (removed). - SHOULD-CONSIDER #3 (audio headers in src/light/ giving a core->light arrow): accepted as-is — documented producer/consumer placement, boundary check passes. Verification: 56 driver/encoder unit tests pass, desktop build zero-warning, spec + platform-boundary clean. All three ESP32 targets (esp32, esp32s3-n16r8, esp32p4-eth-wifi) build clean under -Werror, including both CRTP instantiations. On hardware: S3 LcdLedDriver loopback bit-perfect over a 13->12 jumper (1536/1536 symbols captured, 0-bits 15..15 / 1-bits 30..30 ticks, zero jitter); P4 ParlioLedDriver renders the S3's audio spectrum live over the DDP path. Hot-path A/B (same S3, same config): pre-dedup ~3421-3442us/frame vs CRTP ~3425-3436us/frame — statistically identical, zero overhead (CRTP is static dispatch; the per-light loop is unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 14, 2026
…rd catalog Two infrastructure wins plus a catalog round. (1) The firmware list, previously hand-copied six times (one copy wrong), is now generated from the FIRMWARES dict into docs/install/firmwares.json and read by CI, MoonDeck, and the docs — fixing a bug where MoonDeck couldn't offer the P4 variants. (2) The byte-identical release assets are shared: ota-data once globally, partition-table once per flash-size group, instead of one copy per firmware — verified end-to-end in a real browser flash. The board catalog gains capability chips, real LED pins from MoonLight's pin database, two new MHC/ABC boards, and 89%-smaller images. Scripts / MoonDeck: - generate_firmwares.py (new): projects build_esp32's FIRMWARES (now with a per-entry `ships` flag) into docs/install/firmwares.json. check_firmwares.py (new) guards the committed file against drift. - generate_manifest.py: emit shared asset names — `shared-ota-data.bin` (global) and `partition-table-<size>.bin` (per flash-size group, key read from flasher_args' flash_settings.flash_size); app + bootloader stay per-firmware. - moondeck.py / moondeck_config.json: read the shipping firmware list from firmwares.json instead of a hardcoded list that was missing both P4 variants (the bug fix). Added the check_firmwares entry to the check group. - preview_installer.py: stage the shared asset names so local flash-ready preview matches release; added the json import. Docs / CI: - release.yml: a `firmwares` job emits the shipping list; build-esp32's matrix reads it via fromJSON; both manifest loops select `.ships` from firmwares.json (three hand-copied lists gone). Stage step writes shared asset names; publish + Pages-download globs include them. - boards.json: added supported/planned capability arrays (short labels) to all boards; LED pins from MoonLight ModuleIO.h (Serg ×2, MHC ×2, Yves, Cube, QuinLED family); new boards MHC V5.7 PRO image+url and ABC-WLED ESP32-P4 shield; LightCrafter16/SE16 kept pinless with a "16-ch I2S (not yet)" planned note (their 16 outputs exceed RMT/LCD lane counts). - board images: compressed 7.3 MB -> 0.8 MB (89% smaller) — capped at 600px, JPEG q80; the three photo-PNGs converted to JPEG; Dig-2-Go re-fetched uncropped; small images that re-encode inflated were restored to originals. New vendor images (MHC, ABC P4, Serg UniShield) hosted locally, not hotlinked. - install-alt/index.html: capability chips — supported green, planned orange (colour, not text); show all chips with short labels (dropped the +N overflow); short tooltips; keyboard-accessible cards + search aria-label. - building.md: firmware-variant table replaced with a pointer to the FIRMWARES dict + firmwares.json (no duplicated list). - check_boards.py: capability vocab updated for the shortened "Audio" label. - CLAUDE.md: commit gate 9 (check_firmwares). - installer-3layer-plan.md: marked #2 (firmwares.json) and #8 (asset dedup) done; added §10 — the detected-chip-vs-boards.json-chip granularity mismatch (onDetect returns raw silicon on the web path, so the flash guard false-warns; lean toward normalising silicon→family at compare time rather than adding per-SKU precision to boards.json). Reviews: - 🐇 CodeRabbit (prior round, folded in earlier this session): a11y on the board cards + search input, decisions.md list numbering, preview_installer uv-run, LcdLedDriver tense, install-picker txPower bounds — all fixed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 15, 2026
Merged
ewowi
added a commit
that referenced
this pull request
Jun 21, 2026
Follow-up to the "Improv = REST over serial" work on this branch: processes the CodeRabbit review of the prior commit and adds the host-side + device-side test coverage that makes config-push over serial provable without hardware. The Improv frame format (built three times — device C++, Python, installer JS) and the device's APPLY_OP chunk reassembly + sequence guard now have automated tests; the installer's frame builders were extracted into a shared, dependency-free module so the JS is testable in node at all. Also restores 18 historical plans under the corrected Plan-YYYYMMDD (ISO-8601) naming. KPI: 16384lights | PC:365KB | tick:111/87/110/9/1/312/37/15/19/111/11us(FPS:9009/11494/9090/111111/1000000/3205/27027/66666/52631/9009/90909) | ESP32:1249KB | tick:5155us(FPS:193) | heap:8335KB | src:97(19754) | test:68(10118) | lizard:77w (ESP32 tick 5155us is a fresh live capture from the S3 running the full testbench config — Grid + AudioSpectrum + RandomMap + RmtLed + NetworkSend broadcasting Art-Net. Higher than the old idle-ish 3.6ms because NetworkSend is active; this change touches no render-path code, so the tick is unchanged for a like-for-like config.) Core: - ImprovOpReassembler (new): extracted the APPLY_OP chunk-reassembly + out-of-order/duplicate sequence guard out of the ESP32 handler into a pure, header-only core state machine (the ImprovFrame.h precedent — algorithm in core, UART in platform). Returns a typed Continue/Ready/Error. This makes the heart of "REST over serial" desktop-unit-testable and shrinks the platform handler (removed two g_improv fields). - HttpServerModule: split OpResult::NotFound into ModuleNotFound + ControlNotFound so the HTTP /api/control handler reports the two distinct 404 bodies again (CodeRabbit #1); added OpResult::AlreadyExists so an idempotent add reports {"ok":true,"note":"already exists"} again (#5); documented the serial "parent" vs HTTP "parent_id" key difference at the applyOp site (#6). - ImprovProvisioningModule: a non-Ok APPLY_OP result is now surfaced — logged over serial and parked in provision_status — so a silently-misconfigured device is visible on a monitor and via /api/state instead of looking like a clean install (#2). Ok/AlreadyExists are both treated as success. Platform: - platform_esp32_improv.cpp: improvHandleApplyOp now delegates chunk merge + seq guard to mm::ImprovOpReassembler, keeping only the serial I/O (busy guard, ack/error sends). No behaviour change; the logic is now the unit-tested core path. UI: - improv-frame.js (new): the pure Improv frame builders (buildImprovFrame, encodeApplyOpFrames, the command IDs) extracted from install-orchestrator.js so node:test can import them without the orchestrator's top-level unpkg imports. The orchestrator imports them back — no behaviour change. - install-orchestrator.js: APPLY_OP inter-op pacing 30ms -> 120ms to cover the worst-case single-buffer consume window with headroom (CodeRabbit #3; the closed-loop read-back-ack upgrade is backlogged, ops are idempotent). Tests: - unit_ImprovOpReassembler (new, 12 cases): in-order multi-chunk reassembly + NUL-termination, duplicate-chunk rejection, out-of-order/skipped-seq rejection, overflow at the buffer-minus-NUL boundary, exact-fit boundary, mid-stream seq-0 reset, empty final chunk, recovery after every error. - test/js/improv-frame.test.mjs + test/python/test_improv_frame.py (new): assert the SAME golden frame (buildImprovFrame(0x03,[0x01]) == 49 4d 50 52 4f 56 01 03 01 01 e3) so the device C++, Python, and JS frame builders provably agree; the JS suite also pins APPLY_OP chunking (seq/last, the 125-byte boundary). Encode (JS) + reassemble (C++) prove APPLY_OP end to end without hardware. - unit_HttpServerModule_apply: updated for the ModuleNotFound/ControlNotFound/AlreadyExists split. Scripts / CI: - .github/workflows/test.yml (new): PR-triggered pytest (test/python) + node --test (test/js) — the first PR-level test gate (none existed). Python tests carry deps in a PEP-723 inline block (repo convention, no central pyproject). Docs / CI: - testing.md: new "Host-side tests (Python + JS)" tier with a per-test inventory of exactly what the Python/JS frame suites pin, plus the C++ reassembler coverage. test/python + test/js added to the file layout. - CLAUDE.md: added commit gate 10 (host-side Python/JS unit tests); corrected the plan naming convention to Plan-YYYYMMDD (ISO-8601, sorts chronologically) and clarified plans are product-owner reference (agents write, don't auto-read). - ImprovProvisioningModule.md: documented the serial-vs-HTTP parent key difference. - docs/history/plans: restored 18 historical plans (plan-01..18, deleted in an old merge) under Plan-YYYYMMDD names; renamed the Improv-as-REST plan to the corrected ISO date. - backlog: removed the shipped "board injection / general data injector" item (it IS APPLY_OP now); added the closed-loop-pacing follow-up; renamed stale boards.json -> deviceModels.json references. Reviews: - 🐇 #1 module-vs-control 404 specificity — fixed (OpResult split). - 🐇 #2 dropped APPLY_OP result — fixed (logged + provision_status). - 🐇 #3 open-loop 30ms pacing — fixed (bumped to 120ms; ack-loop backlogged). - 🐇 #4 perf-bound "regression" — accepted: the cited tick_us are observed.* telemetry (0 bounds, 21 observed), which only widens on a slow run; no render-path change, not a regression. - 🐇 #5 dropped "already exists" note — fixed (AlreadyExists OpResult). - 🐇 #6 parent vs parent_id — fixed (documented at code site + spec). - 🐇 #7 NUL-guard off-by-one — no action (CodeRabbit confirmed correct). - 🐇 #8 plan date format — fixed (YYYYDDMM -> ISO YYYYMMDD, all plans renamed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 22, 2026
Merged
This was referenced Jun 30, 2026
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
The
next-iterationbranch — 7 commits, plans 08-12. Builds the device-management layer, persistence, the generic web UI, the Ethernet-only build, and the side-navigation UI.Plan-08 — SystemModule + NetworkModule
Plan-09 — Filesystem foundation (persistence attempt partly abandoned; foundations kept)
typeName_,dirty_,rebuildControls())Plan-10 — Control-list-driven JSON persistence
Plan-11 — Web UI rewrite to the ui.md spec baseline
/api/types,/api/modules/<n>/move,/api/rebootEth-only build + FPS-swing fix + ArtNet optimization
build_esp32.py --profile eth-only— WiFi compiled out (~558 KB vs ~1 MB)detail/decompresscontrolsPlan-12 — Side navigation + logo
/api/stateserializes it XOR-obfuscated + base64 (not plaintext)Test plan
-Werror)Notes
Not the 1.0 release —
docs/plan.mditem 13 (README quick-start) is still open. This is a branch-consolidation merge.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements