Orchestration reorg: applyState + hook rename + ScratchBuffer/ActiveInstance primitives; System/Services split; Tasks/Pins modules#43
Merged
Conversation
Adds TasksModule: a read-only System diagnostic that lists every FreeRTOS task and nests the projectMM modules running inside each, with per-module cost (us / class size / heap) and an optional per-task CPU%. It's the observability foundation for the coming core-affinity/task-relocation work — you can't optimise task placement you can't see. Ships alongside a set of design notes (System vs Services split, a Pins manager, app.js module-independence) that frame the next round of core modules; the notes are backlog/plan docs, no code follows from them yet. KPI: 16384lights | PC:666KB | tick:137/109/133/18/2/307/63/18/21/171/123/17/1/38us(FPS:7299..) | ESP32:1379KB | tick:3286us(FPS:304) | heap:8331KB Core - TasksModule: new header-only System module. One `tasks` List (rows = RTOS tasks) whose row-detail nests the modules in that task with cost, plus core0/core1 readouts and a "∑ modules Xus / tick Yus" closed cross-check. Refreshes on loop1s (off the hot path), reads the existing per-module self-report (loopTimeUs/classSize/dynamicBytes). Registered in main.cpp. - platform: new domain-neutral task seam — TaskInfo POD + taskSnapshot/currentTaskOnCore/renderTaskName, so no FreeRTOS type escapes src/platform/. Added setTestTaskSnapshot test seam for host-side testing. - platform/esp32: platform_esp32_tasks.cpp fills TaskInfo from uxTaskGetSystemState (gated on CONFIG_FREERTOS_USE_TRACE_FACILITY, no per-tick cost); CPU% behind the MM_TASK_CPU_STATS build flag (~5% tick, off by default). - platform/desktop: stubs return 0 (no RTOS) + honor the test seam. Scripts / MoonDeck - build_esp32.py: --task-cpu-stats flag wires the CPU-stats sdkconfig fragment + -DMM_TASK_CPU_STATS=1; the flag is added to the stale-feature-cache guard so toggling it forces a clean reconfigure. Tests - unit_TasksModule: 5 cases / 25 assertions — control set, empty-snapshot safety, injected-snapshot row rendering (CPU% shown/omitted), and the render-task detail nesting modules + the ∑/tick cross-check, via the desktop test seam. Docs / CI - services.md: Tasks section. system-modules.md + Plan-20260708/20260709: System-vs-Services split design (Services = user-added Audio/IR container modelled on Layers/Drivers; System modules fixed/wired-by-code; MQTT stays under Network; Devices stays fleet-scope under Network). pins-analysis + ui-extensibility-analysis: bottom-up field surveys for upcoming work. home-automation.md: HA integration screenshot + haDiscovery default wording. - esp32: sdkconfig.defaults gains USE_TRACE_FACILITY + VTASKLIST_INCLUDE_COREID; sdkconfig.defaults.task-cpu-stats is the opt-in CPU-stats fragment; main/CMakeLists.txt wires MM_TASK_CPU_STATS. Reviews - 👾 stale build-cache guard: build_esp32.py didn't invalidate on MM_TASK_CPU_STATS change — fixed (verified live: the ESP32 build this commit auto-cleaned a stale cache). - 👾 heap alloc in snapshot: taskSnapshot used malloc — fixed to a static TaskStatus_t buffer, dropped std::free/<cstdlib>. - 👾 renderTaskName comment overclaimed — corrected (only valid while single-tasked). - 👾 no host coverage of populated rows: added setTestTaskSnapshot seam + 2 test cases. - 👾 3 stale TasksModule comments + header-only exception note — fixed. - 👾 system-modules.md Services lists wrongly included MQTT/Devices; open-questions bullet listed MQTT as "moves to Services" — fixed to match the settled design (MQTT/Devices stay under Network). - 👾 backlog spec draft shipped with the module — deleted (its home is services.md + the .h, per Documentation model / Mandatory subtraction); fixed its one inbound plan link. - 👾 Plan-20260709: §4a falsely claimed Tasks is "idle until used" (it samples every loop1s) — corrected; §7 persistence migration must match the pre-rename `peripheral` wire value too — noted; §52 residual waffle removed. - 👾 home-automation.md haDiscovery step said "leave on the default" (it's off by default) — corrected to "turn on". Note: the CLAUDE.md pre-commit Python gate command lacks `--with wled`, so it can't collect test_wled_json_shape.py as written; the suite passes with that dep added (52 passed). Candidate CLAUDE.md fix, left for a separate change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new TasksModule diagnostic exposing FreeRTOS task and MoonModule nesting via a platform task-introspection API (desktop test stubs and ESP32 FreeRTOS implementation), with build/config wiring and unit tests. Also adds backlog/plan documentation for PinsModule, System/Services split, and UI extensibility, plus minor doc and test-data updates. ChangesTasksModule Feature
Backlog and Plan Documentation
Docs and Test Data Updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backlog/backlog-core.md`:
- Around line 207-211: The staged PinsModule scope is mixing the read-only first
increment with later conflict-enforcement authority. Update the PinsModule spec
language so phase 1 in the backlog.md entry clearly stays a read-only pin map
derived from ControlType::Pin controls, and move any “authority”/conflict-broker
wording to the later staged item; keep the references to PinsModule,
pin-uniqueness check, and the staged scope explicit so the implementation path
remains incremental.
In `@docs/backlog/system-modules.md`:
- Around line 50-56: The new ModuleRole::Service name is becoming overloaded
because it would apply both to user-managed Services children and to wired
Network children like MQTT/Devices. Update the split plan and role mapping so
only the Services container uses ModuleRole::Service, and keep the
Network-contained modules roleless or give them a distinct wired-child role;
adjust the roleName and acceptsChildRoles logic accordingly so the
container/role naming stays unambiguous.
In `@docs/history/plans/Plan-20260709` - Split System into System + Services.md:
- Around line 5-8: The opening summary in the plan is conflating user-added
services with fixed or code-wired modules, which makes the later split read
inconsistent. Update the intro around the plan’s main summary to clearly
separate Service Modules (user-added, add/delete, ModuleRole::Service, e.g.
Audio and IR) from System Modules/always-there infra (fixed, wired-by-code, no
add/delete, e.g. Tasks, I2cScan, Network, Firmware, Improv), using the existing
terms from the document so the rest of the sections align.
🪄 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: e8f7dbb5-ac27-43d1-8fd1-f53e2259b808
⛔ Files ignored due to path filters (2)
docs/assets/core/ha-integration.pngis excluded by!**/*.pngmoondeck/build/build_esp32.pyis excluded by!**/build/**
📒 Files selected for processing (20)
docs/backlog/backlog-core.mddocs/backlog/pins-analysis-bottom-up.mddocs/backlog/system-modules.mddocs/backlog/ui-extensibility-analysis-bottom-up.mddocs/history/plans/Plan-20260708 - TasksModule (nested task view).mddocs/history/plans/Plan-20260709 - Split System into System + Services.mddocs/moonmodules/core/services.mddocs/usecases/home-automation.mdesp32/main/CMakeLists.txtesp32/sdkconfig.defaultsesp32/sdkconfig.defaults.task-cpu-statssrc/core/TasksModule.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_tasks.cppsrc/platform/platform.htest/CMakeLists.txttest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/core/unit_TasksModule.cpp
Comment on lines
+50
to
+56
| Container = **Services**, and `ModuleRole::Peripheral` is **renamed to `ModuleRole::Service`** so there is one concept, one word (the container name and the role name agree). Chosen on the merits, not on what already exists: | ||
|
|
||
| - **"Service" is the more accurate word.** The set is mostly *network/software bridges* (MQTT, Devices, Hue) with some hardware (Audio, IR). "Peripheral" is a hardware term (a thing attached at the periphery) — stretching it to cover a network integration is a flaw the current code already carries, not a reason to keep it. "Service" = an ongoing capability the device provides or consumes, which is exactly what these are, and the word HA / systemd / Android use for the same idea. | ||
| - **The rename cost is not an argument to avoid it.** "We already have `Peripheral`" is a precedent-defense the principles reject ([*don't store derivable, no precedent-defense*](../../CLAUDE.md#principles); best solution over technical-debt-carried-forward). The rename is mechanical (role enum + `roleName` + `acceptsChildRoles` + docs) and one-time. | ||
| - **The "services" overload (WiFi/HTTP are also 'services')** is weaker than it looks: those are *always-there infrastructure the device runs on*, not *user-added capabilities* — they stay in System. The line "user-added optional capability (Services)" vs "always-there infrastructure (System)" is clear. | ||
|
|
||
| So: **Services** container, **`ModuleRole::Service`** role, and the split plan carries the peripheral→service rename. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep Service reserved for Services children.
If MQTT/Devices stay under Network, renaming their probed role to service makes the new role mean two different things: a user-managed Services child and a wired Network child. That breaks the "container name and role name agree" rationale and will also make the later split plan's "update their probed role to service" wording ambiguous. Either keep those modules roleless or introduce a separate wired-child classification.
🧰 Tools
🪛 LanguageTool
[style] ~52-~52: Consider an alternative for the overused word “exactly”.
Context: ...e device provides or consumes, which is exactly what these are, and the word HA / syste...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/backlog/system-modules.md` around lines 50 - 56, The new
ModuleRole::Service name is becoming overloaded because it would apply both to
user-managed Services children and to wired Network children like MQTT/Devices.
Update the split plan and role mapping so only the Services container uses
ModuleRole::Service, and keep the Network-contained modules roleless or give
them a distinct wired-child role; adjust the roleName and acceptsChildRoles
logic accordingly so the container/role naming stays unambiguous.
Draws a clean line between the device's fixed infrastructure and its user-added capabilities. System (identity, network, provisioning, firmware, and the inspection tools Tasks/I2cScan) is now fixed and wired-by-code — you don't add or delete it. A new Services container — the core-domain twin of the light pipeline's Layers/Drivers — holds the user-added capability modules (Audio, IR), which you add and remove at runtime. The old ModuleRole::Peripheral (which conflated both, and which Tasks borrowed only to render a delete button) becomes ModuleRole::Service, carried only by the Services container's children. Audio/IR modules are renamed AudioService/IrService to match. No config reboot, no back-compat shims — projectMM is new, so this is the best design now.
KPI: ESP32 tick:3415us(FPS:292) | heap:8331KB | src:180(36043)
Core
- MoonModule: ModuleRole::Peripheral → Service (enum, roleName, all consumers); ModuleFactory::displayNameFor strips the "Service" suffix (AudioService → Audio) — removing the old Peripheral special case, so suffix-stripping is now uniform across every role.
- Services: new thin top-level container (src/core/Services.h) — one acceptsChildRoles("service") override, no controls/loop/state, modelled on Layers/Layouts/Drivers.
- SystemModule: acceptsChildRoles → "" (accepts no user-added children; the override is deleted, inheriting the base default). Class + lifecycle-chain comments rewritten from "user-added Peripherals" to "wired-by-code System modules".
- TasksModule + I2cScanModule: now fixed wired-by-code children of System (created in main.cpp with markWiredByCode), role override dropped (→ Generic) so no container offers a delete affordance; their /// prose rewritten from the borrowed-Peripheral stopgap to "fixed System module".
- FileManagerModule: dropped its incidental Peripheral role (→ Generic).
- AudioService/IrService (renamed from AudioModule/IrModule): role → Service; /// prose reframed as "a child of the Services container".
- main.cpp: register Services + wire Tasks/I2cScan as System children; every registerType docPath repointed to the correct split page (system.md for fixed infra, services.md for Audio/IR).
Light domain
- The audio effects and the WLED-sync path follow the AudioModule → AudioService rename (AudioService::latestFrame()), a pure identifier change.
UI
- app.js: ROLE_EMOJI peripheral → service (🛰️); refreshed a stale "System hosts peripherals" comment. No logic change — the 1:1 role→container invariant holds (System→"", Services→"service", fixed modules→Generic).
Scripts / MoonDeck
- screenshot_modules.py: Audio/IR renamed + reparented to the Services nav root, Services added to the capture list; moondeck.py + build_esp32.py comment refresh.
- mkdocs_hooks.py: core/system.md added to the catalog-page set so it renders as a table, not raw prose.
- check_devices.py: unchanged — Services is factory-registered, so parent_id:"Services" already resolves (no BOOT_WIRED_TYPES entry needed).
Tests
- unit_Services: new — pins that Services accepts "service" children and System accepts none, and that Services is a controls-free grouping node.
- unit_SystemModule + unit_TasksModule: the "accepts peripheral"/"is a Peripheral" cases flip to "accepts none"/"fixed Generic". AudioService/IrService test renames; config-ops.test.mjs asserts the S3 entry now clears Services (not System). scenario_Audio_mutation adds the Services fixture and reparents the mic; perf scenarios target the mic by its runtime name "Audio" (was the dead id "AudioService").
Catalog
- deviceModels.json: Audio/IR parent_id System → Services; I2cScan entries drop parent_id (now boot-wired, so they become pure sda/scl control-sets); Audio/Ir type strings → AudioService/IrService.
- Olimex Gateway image: replaced olimex-esp32-gateway-rev-g.jpg with olimex-esp32-gateway.png, compressed 1.01MB → 215KB.
Docs / CI
- Split docs/moonmodules/core/services.md → core/system.md (fixed infra) + core/services.md (Audio/IR); mkdocs nav + all cross-references updated.
- architecture.md § Peripherals rewritten as § Services (role Service, Services container, AudioService exemplar); the role-emoji table, coding-standards.md, and building.md follow.
- Swept the retired-Peripheral-role prose and stale AudioModule/IrModule class references out of every present-tense + forward-looking doc (backlog GyroDriver→GyroService plan, audio-reactive specs, reference pages, README); fixed a pre-existing broken README moxygen link. docs/history/** left as the backward-looking record; on-chip-"peripheral" hardware usage left intact.
- Renamed asset PNGs AudioModule.png/IrModule.png → AudioService.png/IrService.png.
Reviews
- 🐇 backlog-core PinsModule: phase-1 scope reworded to read-only-first, "conflict authority" deferred to the later staged increment.
- 🐇 system-modules Service-role overload: clarified the role applies ONLY to the Services container's children; MQTT/Devices stay roleless (Generic) under Network.
- 🐇 Plan-20260709 intro: separated user-added Services (Audio/IR) from fixed/always-there infra (Tasks/I2cScan/Network/…) up front.
- 👾 H1 architecture.md § Peripherals stale — rewritten as § Services + emoji table + coding-standards/building cross-refs.
- 👾 H2 core/system.md missing from the docs table hook — added.
- 👾 H3 SystemModule orphaned /// + stale class/chain prose — folded + rewritten.
- 👾 H4 AudioService /// still said "User-added Peripheral" — reframed as Service.
- 👾 M1 screenshot pipeline used the old types/parentage — renamed + reparented to Services.
- 👾 M2 web-installer/README taught parent_id:"System" for Audio — fixed to Services.
- 👾 M3 perf scenarios targeted the dead id "AudioService" (runtime name is "Audio") so the audio-cost measurement silently no-op'd — fixed to "Audio".
- 👾 M4 IrService cited I2cScan as the not-auto-wired exemplar (now boot-wired) — dropped.
- 👾 Low: comment/label leftovers (old test filenames, "TWO AudioModules", "Core › Services › MQTT" breadcrumbs now pointing at system.md), plan BOOT_WIRED note reconciled — all fixed.
- 👾 Deferred (accepted): docs/assets/core/Services.png needs a live hardware screenshot capture; the @card reference matches the sibling-container convention and mkdocs doesn't break without it. Capture on the next screenshot run.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/platform.h (1)
52-59: 🩺 Stability & Availability | 🔵 TrivialConfirmed:
uxTaskGetSystemStatesuspends the scheduler — the design already restricts it toloop1s(), good.Cross-checked against FreeRTOS's official docs: this API is documented as intended for debugging only, "as its use results in the scheduler remaining suspended for an extended period." The header's own comment already documents and mitigates this by keeping the call off
loop()/loop20ms(). Just flagging this as an explicit constraint any future caller oftaskSnapshot()must keep respecting — it must never move onto the per-frame hot 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/platform/platform.h` around lines 52 - 59, The `taskSnapshot()`/TasksModule path already notes that `uxTaskGetSystemState` suspends the scheduler, but the constraint should be made explicit and preserved so future callers do not move it onto the per-frame hot path. Update the `platform.h` RTOS introspection comment around `taskSnapshot()`/`loop1s()` to clearly state that this API must remain off `loop()` and `loop20ms()`, and keep any usage confined to the once-per-second path.
🤖 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/coding-standards.md`:
- Line 125: Update the terminology in the coding standards entry so it does not
conflate the new [Services] container with fixed infrastructure modules. In the
MoonModule description, adjust the wording around the module categories to refer
to Network/System/FileSystem as System modules rather than “core services,”
while keeping the distinction between UI and supporting modules intact. Use the
existing “Everything is a MoonModule” section and the [Services] link as the
anchor when revising the sentence.
In `@docs/history/plans/Plan-20260708` - TasksModule (nested task view).md:
- Line 7: Update the TasksModule plan record so all doc-home references match
the shipped location: the link text and the Decisions/Files/verification entries
should consistently point to the same `system.md` Tasks section rather than
`services.md`. Review the references in the plan document around the TasksModule
summary, the “Doc home” decision, the Files section, and the verification note,
and align them with the existing `TasksModule.h` and `core/system.md#tasks`
symbols.
In `@docs/history/plans/Plan-20260709` - Split System into System + Services.md:
- Around line 66-69: The persistence migration section has conflicting behavior:
it says Audio/IR configs are dropped with no migration, but the execution notes
describe reparenting the saved subtree into Services before load. Update this
paragraph to choose one consistent approach and align it with the rest of the
plan, referencing FilesystemModule::migrateRenamedConfigs() and the
System/Services parent mapping so the migration story is unambiguous.
In `@docs/moonmodules/core/system.md`:
- Around line 1-7: The markdown heading hierarchy in the core system doc skips a
level from the top-level title to the module section. Update the headings in the
system document so the structure is sequential, using an h2 between the existing
“Core system” title and the “System” section, or lowering the module headings to
h2 consistently. Keep the change localized to the headings around “Core system”
and “System” so markdownlint no longer reports an h1 → h3 skip.
---
Outside diff comments:
In `@src/platform/platform.h`:
- Around line 52-59: The `taskSnapshot()`/TasksModule path already notes that
`uxTaskGetSystemState` suspends the scheduler, but the constraint should be made
explicit and preserved so future callers do not move it onto the per-frame hot
path. Update the `platform.h` RTOS introspection comment around
`taskSnapshot()`/`loop1s()` to clearly state that this API must remain off
`loop()` and `loop20ms()`, and keep any usage confined to the once-per-second
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43c11e2f-e79d-467d-b906-b99ccca8b666
⛔ Files ignored due to path filters (4)
docs/assets/core/AudioService.pngis excluded by!**/*.pngdocs/assets/core/IrService.pngis excluded by!**/*.pngdocs/assets/deviceModels/olimex-esp32-gateway-rev-g.jpgis excluded by!**/*.jpgdocs/assets/deviceModels/olimex-esp32-gateway.pngis excluded by!**/*.png
📒 Files selected for processing (84)
README.mddocs/architecture.mddocs/backlog/audio-dsp-roadmap.mddocs/backlog/backlog-core.mddocs/backlog/backlog-light.mddocs/backlog/backlog-mixed.mddocs/backlog/livescripts-analysis-top-down.mddocs/backlog/moonlight-fidelity-tensions.mddocs/backlog/system-modules.mddocs/building.mddocs/coding-standards.mddocs/gettingstarted.mddocs/history/plans/Plan-20260708 - TasksModule (nested task view).mddocs/history/plans/Plan-20260709 - Split System into System + Services.mddocs/moonmodules/core/services.mddocs/moonmodules/core/system.mddocs/moonmodules/light/effects.mddocs/performance.mddocs/reference/esp32-s31-coreboard.mddocs/usecases/home-automation.mdmkdocs.ymlmoondeck/docs/mkdocs_hooks.pymoondeck/docs/screenshot_modules.pymoondeck/moondeck.pysrc/core/AudioFrame.hsrc/core/AudioService.hsrc/core/DevicesModule.hsrc/core/FileManagerModule.hsrc/core/HttpServerModule.cppsrc/core/I2cScanModule.hsrc/core/IrService.hsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/MqttModule.hsrc/core/Scheduler.hsrc/core/Services.hsrc/core/SystemModule.hsrc/core/TasksModule.hsrc/light/Palette.hsrc/light/WLEDAudioSyncPacket.hsrc/light/drivers/Drivers.hsrc/light/drivers/HueDriver.hsrc/light/effects/AudioSpectrumEffect.hsrc/light/effects/AudioVolumeEffect.hsrc/light/effects/BlurzEffect.hsrc/light/effects/FreqMatrixEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PaintBrushEffect.hsrc/main.cppsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32_es8311.cppsrc/platform/esp32/platform_esp32_i2s.cppsrc/platform/esp32/platform_esp32_ir.cppsrc/platform/platform.hsrc/ui/app.jstest/CMakeLists.txttest/js/config-ops.test.mjstest/scenario_runner.cpptest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/core/unit_AudioService_sync.cpptest/unit/core/unit_IrService.cpptest/unit/core/unit_MqttModule.cpptest/unit/core/unit_Services.cpptest/unit/core/unit_SystemModule.cpptest/unit/core/unit_TasksModule.cpptest/unit/light/unit_AudioBands.cpptest/unit/light/unit_AudioLevel.cpptest/unit/light/unit_AudioService.cpptest/unit/light/unit_BlurzEffect.cpptest/unit/light/unit_FreqMatrixEffect.cpptest/unit/light/unit_FreqSawsEffect.cpptest/unit/light/unit_GEQ3DEffect.cpptest/unit/light/unit_GEQEffect.cpptest/unit/light/unit_NoiseMeterEffect.cpptest/unit/light/unit_PaintBrushEffect.cppweb-installer/README.mdweb-installer/deviceModels.json
💤 Files with no reviewable changes (1)
- src/core/FileManagerModule.h
…A, testbench catalog Ships pins top-down increment #1: a read-only System module that walks the live module tree and shows which module owns each GPIO, for what role — the device's pin ownership map, keyed by physical GPIO like a Device Manager. Also fixes the firmware OTA push abort on MB-scale images, finalizes the Parlio loopback fix and the MM-testbench catalog, and processes the Reviewer findings. KPI: tick:131/91/121/15/2/281/59/17/21/160/120/17/1/71us (PC) · 4352us/FPS229 (ESP32) Core - PinsModule: new fixed, wired-by-code System child (Tasks/I2cScan pattern). Walks the tree, collects every ControlType::Pin (>=0) plus each LED-driver "pins" CSV, exposes a GPIO-keyed read-only list — owner + name-derived role (sckPin→BCLK, pins→LED lane N, sda/scl→I²C, ethMdcGpio→MDC, …). A GPIO claimed twice stays visible (both owners in the row detail) — read-only surfacing, not enforcement. Reads live on loop1s, holds no state; owner AND role are copied per-claim so a claim survives its module being deleted. Inverts MoonLight's central ModuleIO: each module owns its pins, this only observes. Hardware-tested on P4 + S3. - HttpServerModule: firmware push-OTA gets its own 5-min whole-request ceiling (kFirmwareUploadHardMs) instead of the file path's 60s anti-slowloris cap, which aborted MB-scale firmware uploads mid-flash (~87%). The idle guard and file-upload bounds are unchanged. Verified end-to-end on S3. - NetworkModule: ethPhyAddr is a PHY MDIO address (0..31), not a GPIO — bind it with addUint8 instead of addPin, so the pin map doesn't report it as a false GPIO claim. - platform_esp32_rmt: the loopback continuity log is kept as a permanent bench HAL diagnostic (reframed from a TEMP comment) — it pinned the MHC-WLED P4 shield loopback. Light domain - ParallelLedDriver: cap the loopback self-test frame to kLoopbackTestLights=256 so a large operational grid can't exceed the P4 Parlio single-transfer bit limit or the RMT-RX capture buffer. Verified PASS on MM-P4 (32/33, 8×8 and 128×128), S3 RMT (13/12), Olimex RMT (33/34). - CarLightsLayout / TorontoBarGourdsLayout: attribute the original authors (Eric Marciniak; troyhacks). UI / Installer - deviceModels.json + web-installer: MM-testbench catalog (P4/S3/olimex) — audio line-in, LED + loopback pins, images; renamed "projectMM testbench X" → "MM testbench X". Tests - unit_PinsModule: 7 cases — collects set pins + skips -1; parses the pins CSV into per-lane claims; GPIO-ordered rows; a double-claim shows both owners in detail; child modules walked depth-first; a claim survives its owner being destroyed (no use-after-free); an out-of-range CSV pin is skipped, not wrapped. - config-ops.test.mjs: "MM testbench S3" rename. Docs - pins-analysis-top-down (new, PO-approved) + bottom-up (added the MoonLight ModuleIO field survey); Plan-20260709 PinsModule saved; system.md Pins card + #pins anchor. - layouts.md: author column for the two attributed layouts. Reviews - 🐇 5 CodeRabbit findings processed (earlier this cycle). - 👾 Reviewer: FIXED a use-after-free in PinsModule (a claim now copies its owner name, not a pointer into module storage a delete frees — the UI serializes right after a delete, so it was a live read); FIXED ethPhyAddr addPin→addUint8 (a phantom GPIO claim the map surfaced); FIXED a broken markdown table, a CSV pin-range wrap, and a duplicated comment. Accepted (not changed): "MM testbench classic olimex" naming (touches the untracked bench registry; "classic" is intentional). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.cpp (1)
445-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
servicesModuleto the scheduler-ordering comment.The comment at lines 445–452 explains the rationale for every module's position in the
addModulesequence but omitsservicesModule(added at line 471, betweennetworkModuleandlayouts). A future developer reading the comment alone would not know whereServicesfits or why.📝 Proposed comment update
// each; Improv only exists to feed Network credentials, and Mqtt only bridges once the -// network is up), light pipeline (Layouts → Layers → Drivers), then HTTP. The Scheduler -// walks roots in this order each tick; child propagation happens inside each root. +// network is up), services (user-added capability container — Audio/IR, same explicit-add +// model as effects/drivers), light pipeline (Layouts → Layers → Drivers), then HTTP. The +// Scheduler walks roots in this order each tick; child propagation happens inside each root.🤖 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/main.cpp` around lines 445 - 471, Update the scheduler-ordering comment around the addModule sequence in main.cpp to explicitly mention servicesModule and why it is placed after networkModule and before the light pipeline/layouts entry. Keep the explanation aligned with the existing rationale in the scheduler.addModule block so the comment fully matches the actual module registration order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 569-576: The firmware upload hard cap in uploadPull was increased
to 5 minutes, which widens the synchronous render freeze on the hot
Scheduler::tick/loop20ms path. Review the kFirmwareUploadHardMs change in
HttpServerModule and either restore a tighter ceiling or move the
firmware-upload handling toward the existing non-blocking/chunked pattern used
by drainPreviewSend so the render loop is not blocked for minutes. Keep the
upload path aligned with the hot-path no-blocking guideline, and apply the same
adjustment to the related timeout logic referenced by the surrounding
firmware-upload comments.
In `@src/core/PinsModule.h`:
- Around line 7-15: Move the shared pin-list parsing primitive out of the light
layer and into core: `PinsModule.h` should include `core/PinList.h` instead of
`light/drivers/PinList.h`, and `parsePinList` should live in the core
reusable-utilities area alongside symbols like `Sort.h`. Update `RmtLedDriver.h`
and `ParallelLedDriver.h` to include the new core header so the dependency
direction stays from light/domain to core, not the other way around.
---
Outside diff comments:
In `@src/main.cpp`:
- Around line 445-471: Update the scheduler-ordering comment around the
addModule sequence in main.cpp to explicitly mention servicesModule and why it
is placed after networkModule and before the light pipeline/layouts entry. Keep
the explanation aligned with the existing rationale in the scheduler.addModule
block so the comment fully matches the actual module registration order.
🪄 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: 01f7a9aa-b8c7-4188-ab68-df9810caafcf
⛔ Files ignored due to path filters (3)
docs/assets/deviceModels/esp32-olimex-testbench.jpgis excluded by!**/*.jpgdocs/assets/deviceModels/esp32-p4-testbench.jpgis excluded by!**/*.jpgdocs/assets/deviceModels/esp32-s3-testbench.jpgis excluded by!**/*.jpg
📒 Files selected for processing (22)
docs/backlog/backlog-core.mddocs/backlog/pins-analysis-bottom-up.mddocs/backlog/pins-analysis-top-down.mddocs/coding-standards.mddocs/history/plans/Plan-20260708 - TasksModule (nested task view).mddocs/history/plans/Plan-20260709 - PinsModule (ownership map, read-only).mddocs/history/plans/Plan-20260709 - Split System into System + Services.mddocs/moonmodules/core/system.mddocs/moonmodules/light/layouts.mdsrc/core/HttpServerModule.cppsrc/core/NetworkModule.hsrc/core/PinsModule.hsrc/light/drivers/ParallelLedDriver.hsrc/light/layouts/CarLightsLayout.hsrc/light/layouts/TorontoBarGourdsLayout.hsrc/main.cppsrc/platform/esp32/platform_esp32_rmt.cpptest/CMakeLists.txttest/js/config-ops.test.mjstest/unit/core/unit_PinsModule.cppweb-installer/README.mdweb-installer/deviceModels.json
Comment on lines
+569
to
+576
| // A firmware image is MB-scale (1.5+ MB), not the KB-scale of a config file, and pushing it over | ||
| // weak WiFi can legitimately take minutes — well past kUploadHardMs, which sized the whole-request | ||
| // cap for a 256 KB file. So the firmware-upload path uses its own far larger ceiling: the idle guard | ||
| // (kUploadIdleMs, reset on every read) is the real anti-stall, and the whole-request cap only has to | ||
| // bound a wedged transfer. 5 min covers any firmware over any LAN link with wide margin. The | ||
| // render-freeze the file path's tight cap also bounds is a non-issue here — a firmware upload reboots | ||
| // the device the moment it finishes, so the freeze is terminal, not a lingering degradation. | ||
| constexpr uint32_t kFirmwareUploadHardMs = 300000; // 5 min absolute ceiling for a firmware push |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Firmware-upload path now blocks the render tick for up to 5 minutes instead of 1.
This raises the whole-request ceiling used inside uploadPull, which the surrounding comments confirm runs synchronously on loop20ms() inside Scheduler::tick — so a stalling firmware push can now freeze LED rendering for up to 5 minutes (vs ~1 minute before). The fix is well-justified (real "aborted at 87%" bug) and the idle timeout still bounds true stalls, but this materially widens the worst-case render-freeze window on the hot-path tick. Given the file's own comments flag the "zero-freeze fix" (chunked, non-blocking drain like drainPreviewSend) as backlogged, it may be worth prioritizing that for the firmware path specifically, since 5 minutes of frozen LEDs is a much more noticeable regression than 1 minute if a transfer stalls near (but not past) the idle timeout repeatedly.
As per coding guidelines, "Keep hot-path code allocation-free and non-blocking: no heap allocations, no blocking calls... in the render loop and anything it calls."
Also applies to: 671-675
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/HttpServerModule.cpp` around lines 569 - 576, The firmware upload
hard cap in uploadPull was increased to 5 minutes, which widens the synchronous
render freeze on the hot Scheduler::tick/loop20ms path. Review the
kFirmwareUploadHardMs change in HttpServerModule and either restore a tighter
ceiling or move the firmware-upload handling toward the existing
non-blocking/chunked pattern used by drainPreviewSend so the render loop is not
blocked for minutes. Keep the upload path aligned with the hot-path no-blocking
guideline, and apply the same adjustment to the related timeout logic referenced
by the surrounding firmware-upload comments.
Source: Coding guidelines
…releases-pins Extends the read-only pin ownership map: it now flags a claim on an unsafe pin (reserved / boot strap / input-only) and a double-claim, shows each pin's live direction/level/drive, and drops a disabled module's pins. Also fixes an I2cScan pin-default collision, splits the pin-list parser into core, processes CodeRabbit findings, and records a user lead on the P4-WiFi hosted-teardown issue. Hardware-verified across classic-ESP32, S3, and P4. KPI: tick:121/92/121/15/2/280/58/17/21/162/116/17/1/36us (PC) · 3990us/FPS250 (ESP32) Core - PinsModule: increment 2 (severity flag — a claim on a reserved flash/PSRAM/USB pin or a conflict flags error, a driven role on a boot strap or input-only pin flags warn; strap/reserved data from gpio-usage.md via the platform layer, PSRAM-conditional pins flagged only when PSRAM is present at runtime); increment 3 (a double-claimed GPIO soft-flags both rows red, surface-not-enforce); increment 4 (live dir/level/drive columns from the pad, shown as HAL info). Expanding a flagged row names the reason. A disabled module's pins drop from the map (release-on-disable intent). Owner/role/reason copied per-claim so the snapshot survives a delete. - platform gpioCapability + gpioLiveState: new domain-neutral seams (esp32 uses the IDF's GPIO_IS_VALID_* / rtc_gpio / gpio_get_io_config / gpio_get_level, plus a per-chip strap/reserved table; PSRAM detected via heap_caps to avoid a component-dependency; desktop stubs report all-safe with test-injection seams). - PinList: parsePinList moved to core/PinList.h (domain-neutral primitive both a driver's pins control and PinsModule use), so the dependency runs light→core; the light-domain assignCounts stays in light/drivers/PinList.h. - I2cScanModule: pins default to -1 (unused) so an idle scanner claims no GPIO on a board without I2C; it now respects the enabled toggle (off releases its pins in the map); statusBuf_ sized to fit the format. - NetworkModule: ethPhyAddr is a MDIO address, not a GPIO — bind with addUint8 so the map doesn't report it as a false claim. - HttpServerModule: firmware push-OTA whole-request ceiling set to 3 min (kFirmwareUploadHardMs), tight enough to bound the synchronous render freeze while covering a real MB-scale upload; the idle guard is the anti-stall, the non-blocking drain-per-tick is the proper fix (backlogged). UI - app.js / style.css: generic rowSeverityClass — a `severity` field on any list row colours its edge (red/yellow), mirroring the existing `*Sec` age convention; no pins-specific UI. Tests - unit_PinsModule: severity (reserved/strap/input-only, precedence), conflict soft-flag + strap→error promotion, disabled-releases-pins, dir column (out/in/both/off), and the direction-mismatch-is-info-not-a-warning case (dropped after hardware showed false positives on idle I2C / loopback / external clock pins). Docs / CI - system.md: Pins card documents severity, live columns, PSRAM-aware flagging, reason-on-expand; gpio-usage.md notes the S3 22-25 gap; three increment plans saved (shipped); backlog-core records a user lead that P4-WiFi STA failover likely fails on the hosted esp_wifi_deinit/re-init cycle (avoid the teardown on hosted targets); the shipped "direct binary upload OTA" backlog item pruned. - CLAUDE.md: the product owner solely controls commit/merge timing and bundling; agents never ask. Reviews - 🐇 CodeRabbit (3): FIXED OTA freeze (tightened + comment names the sync-freeze and the backlogged non-blocking fix); FIXED PinList core-boundary (split parsePinList to core); FIXED main.cpp module-ordering comment (added servicesModule rationale). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai, review cdc3d37 and share your findings in copy pastable text for agents |
Turning a module off (or booting with it persisted off) now frees the hardware it holds — the LED peripheral, the I²S mic, a socket — so its GPIOs are genuinely reusable by another module, not just idle. This fixes a real P4 bug: a disabled RmtLed sharing GPIO 20 with an enabled ParlioLed was grabbing the pin at boot and leaving the strip dark until a manual toggle. Also documents the MHC-WLED ESP32-P4 shield's terminal map.
tick:132/92/120/15/2/280/59/17/21/164/116/17/1/35us(FPS:7575/10869/8333/66666/500000/3571/16949/58823/47619/6097/8620/58823/1000000/28571) | tick:4289us(FPS:233)
Core:
- AudioService: onEnabled routes to setup/teardown so disable frees the I²S mic + sync socket and vacates the mic election; setup() and onBuildState() gate their acquire on enabled() so a disabled service never grabs the mic or wins the active_ seat (at boot or on a rebuild sweep); ~AudioService vacates active_ so a bare destruction can't leave the static dangling.
- DevicesModule: added teardown() (close the presence socket, reset the bound flag, vacate active_ — also fixes a pre-existing socket leak on removal), onEnabled routing, a setup() enabled-gate so a disabled instance doesn't claim the active_ seat at boot, and ~DevicesModule to vacate active_ on destruction.
- IrService: onEnabled(false) releases the RMT-RX channel via the new platform::irStop() seam; re-enable reopens it lazily on the next read.
- PinsModule: comment clarifies the freed-pin display is truthful now that disable actually releases.
Light domain:
- RmtLedDriver / ParallelLedDriver (Parlio/LCD): onEnabled releases the peripheral + DMA buffer on disable and re-acquires on enable; setup(), onBuildState(), onCorrectionChanged(), and the buffer setters all gate their acquire on enabled() so a disabled driver never re-grabs a shared GPIO — the fix for the ParlioLed-dark-at-boot regression.
- NetworkSendDriver: onEnabled closes/reopens the UDP socket; setup() gates the socket open on enabled(); teardown() now chains to DriverBase::teardown() (was leaking its status string).
- DriverBase: added releaseOnDisable(on) — the opt-in setup()/teardown() router each resource-holding driver calls from its own onEnabled (not a base override, because HueDriver's teardown is not a clean inverse of its setup).
- NetworkReceiveEffect: onEnabled routes to setup/teardown (already gated its build sweep on enabled()).
Core (platform):
- platform.h / esp32 / desktop: added the irStop() seam (esp32 calls the existing closeChannel(); desktop no-op).
Tests:
- unit_RmtLedDriver_lifecycle: a disabled driver's setup() and onBuildState() must not acquire (pins the boot bug — fails without the gate); the round-trip test now runs onBuildState() after disable and asserts the buffer stays freed.
- unit_AudioService: a disabled AudioService must not win the mic election at boot.
- unit_DevicesModule_discovery: a disabled DevicesModule must not claim the active_ seat at boot.
Docs / CI:
- reference: new MHC-WLED ESP32-P4 shield hardware page (terminal map + graphical pinout SVG) and gpio-usage cross-reference; the shield routes every terminal through level shifters / RS-485 transceivers / protected inputs (no bare GPIO), which is why a direct loopback jumper doesn't apply.
- backlog: RS-485 / DMX-512 wired-output future extension (backlog-light); "centralize the enabled-gate for setup/onBuildState in the Scheduler" (backlog-core) — the per-module gates above are the tested interim; the core fix (mirror the loop gate) is a follow-up needing its own plan.
- lessons.md: the release-on-disable arc, including the boot-time setup() gate (the path that actually reached the strip) and the dangling-active_ trap.
- CLAUDE.md: sharpened "Complexity lives in core" — when core already owns a mechanism for one path, extend it to the sibling path instead of pasting the guard per module.
- pins-analysis / plan: pin manager marked shipped; the disabling-releases-resources plan recorded as shipped.
KPI Details:
Desktop:
Lights: 16,384
Binary: 666 KB
tick: 120us, 91us, 120us, 15us, 2us, 281us, 59us, 17us, 20us, 161us, 117us, 16us, 1us, 35us (FPS: 8333, 10989, 8333, 66666, 500000, 3558, 16949, 58823, 50000, 6211, 8547, 62500, 1000000, 28571) (per scenario)
20 scenario(s), 20 passed, 0 failed
Platform boundary: PASS
Specs: 85 modules, 85 ok
ESP32 (P4):
Image: 1,381,698 bytes (67% partition free)
tick: 4289us (FPS: 233) heap free: 8524431
Code:
183 source files (36750 lines)
125 test files (18898 lines)
136 specs, 20 scenarios
Lizard: 133 warnings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/backlog/backlog-core.md (1)
217-228: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale backlog entry — PinsModule has already shipped.
This entry frames PinsModule as a not-yet-built "spec + /plan item," staging phases 1-4. But
src/core/PinsModule.hin this same PR already implements the read-only ownership map,gpio-usage.md-backed severity grading, and multi-claim conflict soft-flagging (phases 1-3) — anddocs/backlog/pins-analysis-top-down.mdindependently confirms "Increments#1–#4 landed." Trim this section to only the genuinely-remaining piece (the optional strict reject-on-add mode), per the "delete a backlog item when it ships" rule.As per path instructions, "Use backlog files only for forward-looking work; delete a backlog item when it ships."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/backlog-core.md` around lines 217 - 228, This backlog entry is stale because PinsModule already exists and covers the read-only map, gpio-usage severity grading, and soft conflict flagging; update the PinsModule section to remove the shipped phase-1-to-3 spec language and keep only the remaining forward-looking work. In the backlog text around PinsModule, trim references to it being “not yet built,” “spec + /plan,” and the staged authority roadmap, and align the entry with the existing `PinsModule` implementation and `pins-analysis-top-down.md` confirmation. Keep only the optional strict reject-on-add mode or any truly unfinished follow-up, per the backlog rule to delete items when shipped.docs/backlog/pins-analysis-top-down.md (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShipped design study belongs in
docs/history/, notdocs/backlog/.The doc now states the pin manager is "shipped and complete" and is "kept as the design record" — that's exactly the backward-looking design-intent record
docs/history/plans/(orlessons.md) exists for. Leaving it indocs/backlog/conflicts with the rule to delete a backlog item once it ships. Consider moving this file (or its remaining content) todocs/history/plans/alongside the siblingPlan-20260709...(shipped).md, keeping only the one open remainder (optional strict reject-on-add mode) in backlog-core.md.As per path instructions, "Use backlog files only for forward-looking work; delete a backlog item when it ships, and keep the backlog in present tense only where forward-looking content is appropriate."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/pins-analysis-top-down.md` around lines 1 - 6, This backlog doc is now describing shipped work, so it no longer belongs in the forward-looking backlog. Move the `pins-analysis-top-down` content to the appropriate history location used by the shipped pin-manager records, and leave backlog-only docs focused on the remaining open item; if this file remains, strip it down to just the still-pending strict reject-on-add work and keep the references to the shipped status in the history copy.src/core/AudioService.h (1)
463-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment contradicts the actual "first-wins" election.
This says "the last one to setup() wins," but
setup()claims the seat onlyif (active_ == nullptr)(first-wins) — matching the correct description a few lines above atlatestFrame(): "The FIRST live module claims the seat insetup()". Update this comment to match.📝 Proposed fix
- // The mic that latestFrame() hands to effects. One in practice; the last one - // to setup() wins, teardown() clears it. inline so the header stays standalone. + // The mic that latestFrame() hands to effects. One in practice; the first one + // to setup() wins, teardown() clears it. inline so the header stays standalone.🤖 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/AudioService.h` around lines 463 - 465, Update the stale comment for AudioService::active_ so it matches the actual first-wins behavior enforced by setup(); the current text says “the last one to setup() wins,” but setup() only claims active_ when it is nullptr. Keep the wording consistent with the nearby latestFrame() comment that says the first live module claims the seat, and make sure the comment reflects teardown() clearing the active_ pointer.src/core/IrService.h (1)
51-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a
teardown()override to release the IR RX channel on removal.Module deletion paths call
teardown()beforedeleteTree(), soonEnabled(false)does not cover this case. Without a teardown hook here, deleting an enabledIrServicecan leave the IR peripheral and GPIO claimed until reboot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/IrService.h` around lines 51 - 53, Add a teardown() override on IrService to explicitly release the IR RX channel during module removal. The current onEnabled(false) path is not sufficient for deletion flows, so update IrService to declare and implement teardown() and free the peripheral/GPIO resources there, using the existing IrService and onEnabled logic as reference for the cleanup that must also happen before deleteTree().Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/light/drivers/NetworkSendDriver.h`:
- Around line 92-97: The UDP sender state is not fully reset during teardown, so
re-enabling can skip reconnecting the new socket and leave sends silently
failing. Update NetworkSendDriver::teardown() to clear the cached destination
state (lastConnectedIp_ and lastConnectedProtocol_) along with closing socket_,
so setup() and connectIfDestChanged() will force a fresh connect after
re-enable. Keep the fix localized to the
teardown()/setup()/connectIfDestChanged() flow in NetworkSendDriver.
---
Outside diff comments:
In `@docs/backlog/backlog-core.md`:
- Around line 217-228: This backlog entry is stale because PinsModule already
exists and covers the read-only map, gpio-usage severity grading, and soft
conflict flagging; update the PinsModule section to remove the shipped
phase-1-to-3 spec language and keep only the remaining forward-looking work. In
the backlog text around PinsModule, trim references to it being “not yet built,”
“spec + /plan,” and the staged authority roadmap, and align the entry with the
existing `PinsModule` implementation and `pins-analysis-top-down.md`
confirmation. Keep only the optional strict reject-on-add mode or any truly
unfinished follow-up, per the backlog rule to delete items when shipped.
In `@docs/backlog/pins-analysis-top-down.md`:
- Around line 1-6: This backlog doc is now describing shipped work, so it no
longer belongs in the forward-looking backlog. Move the `pins-analysis-top-down`
content to the appropriate history location used by the shipped pin-manager
records, and leave backlog-only docs focused on the remaining open item; if this
file remains, strip it down to just the still-pending strict reject-on-add work
and keep the references to the shipped status in the history copy.
In `@src/core/AudioService.h`:
- Around line 463-465: Update the stale comment for AudioService::active_ so it
matches the actual first-wins behavior enforced by setup(); the current text
says “the last one to setup() wins,” but setup() only claims active_ when it is
nullptr. Keep the wording consistent with the nearby latestFrame() comment that
says the first live module claims the seat, and make sure the comment reflects
teardown() clearing the active_ pointer.
In `@src/core/IrService.h`:
- Around line 51-53: Add a teardown() override on IrService to explicitly
release the IR RX channel during module removal. The current onEnabled(false)
path is not sufficient for deletion flows, so update IrService to declare and
implement teardown() and free the peripheral/GPIO resources there, using the
existing IrService and onEnabled logic as reference for the cleanup that must
also happen before deleteTree().
🪄 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: c5ab9417-402e-48c2-86e3-0c0f4036bfd4
⛔ Files ignored due to path filters (1)
docs/assets/reference/mhc-wled-esp32-p4-shield-pinout.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
CLAUDE.mddocs/backlog/backlog-core.mddocs/backlog/backlog-light.mddocs/backlog/pins-analysis-top-down.mddocs/history/lessons.mddocs/history/plans/Plan-20260709 - Disabling releases resources (onEnabled per module) (shipped).mddocs/reference/gpio-usage.mddocs/reference/mhc-wled-esp32-p4-shield.mdsrc/core/AudioService.hsrc/core/DevicesModule.hsrc/core/IrService.hsrc/core/PinsModule.hsrc/light/drivers/DriverBase.hsrc/light/drivers/NetworkSendDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/RmtLedDriver.hsrc/light/effects/NetworkReceiveEffect.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_ir.cppsrc/platform/platform.htest/unit/core/unit_DevicesModule_discovery.cpptest/unit/light/unit_AudioService.cpptest/unit/light/unit_RmtLedDriver_lifecycle.cpp
Core now owns the enable/disable resource decision in one place: a new applyState() router calls onBuildState() (build) on an effectively-enabled module and teardown() (release) otherwise, recursing the tree. Catalog modules go back to plain build methods with no enabled() checks, and disabling any container (a Layer, Drivers, Services) now cleanly releases its whole subtree's memory and hardware — a disabled Layer frees its effects' heap, a disabled Drivers frees its drivers' GPIOs. This replaces the ~24 per-module enabled() guards the earlier disable-releases-resources work scattered across setup/onBuildState/setters.
tick:127/91/120/16/4/282/60/17/21/180/126/19/1/38us(FPS:7874/10989/8333/62500/250000/3546/16666/58823/47619/5555/7936/52631/1000000/26315) | tick:3642us(FPS:274)
Core:
- MoonModule: added applyState() — the single resource-lifecycle router (effectivelyEnabled() ? onBuildState() : teardown(), then recurse), and effectivelyEnabled() which walks parent pointers so a disabled parent releases its subtree (a respectsEnabled()==false ancestor stays neutral). onBuildState() is now a pure leaf build (no child recursion, no enabled() check); the tree recursion lives in applyState(). Doc contracts updated: onBuildState = build, teardown = release, onEnabled = genuine edge-triggers only.
- Scheduler: the boot Phase-4 sweep and buildState() call applyState() instead of onBuildState(), so the enabled decision + recursion route through the one primitive.
- HttpServerModule: the add/replace module paths call applyState() (build-if-enabled) instead of onBuildState().
- AudioService / DevicesModule / IrService: dropped their onEnabled overrides and setup() gates; onBuildState() is pure acquire (mic election, active_ seat, learned-code parse), teardown() is the release (IrService gained a teardown() calling platform::irStop()). Destructors still vacate the active_ static.
- PinsModule: the ownership map now gates on effectivelyEnabled() (was enabled()) so a child of a disabled parent shows its pins FREED, matching the applyState cascade — a hardware-found bug (disabling Drivers left ParlioLed's GPIO 20 falsely claimed in the map).
Light domain:
- RmtLedDriver / ParallelLedDriver (Parlio/LCD) / NetworkSendDriver: onBuildState() is now a pure build (parse + reinit / socket open); the release is teardown(), which applyState() routes to on disable. Dropped the onEnabled overrides and the boot setup() gates. NetworkSendDriver::teardown() also resets the cached destination so a re-enable forces a fresh connect.
- DriverBase: removed releaseOnDisable() — no longer called now that applyState() owns the build-or-teardown routing.
- effects (Fire, GEQ, GameOfLife, Tetrix, Particles, Wave, StarField, StarSky, BouncingBalls, Solid, NetworkReceive): onBuildState() drops its self enabled() gate — pure build, release via teardown() the router calls. The ~15 MoonModule::onBuildState() recurse tail-calls removed (recursion is in applyState now).
- Layer / Layers / Layouts / Drivers: onBuildState() is a leaf that prepares its own state (LUT, buffer, composite); applyState() then recurses to children so they build against the parent's fresh state — the parent-prepares-then-children-build order is preserved.
Tests:
- unit_Layers_container: new cascade test — disabling a parent Layer releases its effect's heap and re-enabling re-acquires + renders (proves the LUT rebuilds before the effect builds against it).
- unit_PinsModule: new test — a child of a disabled parent shows its pins freed (pins the map-honesty fix; fails without it).
- unit_RmtLedDriver_lifecycle / unit_AudioService / unit_DevicesModule_discovery: lifecycle tests migrated to drive applyState() and assert the new contract; ~50 test files' onBuildState() sweep-calls updated to applyState().
Docs / CI:
- CLAUDE.md: sharpened "Complexity lives in core" — the applyState router is the canonical example of lifting a cross-cutting rule into core instead of pasting it per module.
- lessons.md: the release-on-disable arc distilled to the shipped design (one router, not per-module gates) + the cold-boot repro and the teardown-isn't-always-setup's-inverse gotcha.
- backlog-core.md: trimmed the shipped PinsModule "coordinates GPIO" entry to just the remaining optional reject-on-add increment.
- Plan-20260709 - Unify lifecycle (shipped): the design record.
Reviews:
- 🐇 NetworkSendDriver teardown left the cached destination set, so a re-enable skipped reconnecting the new socket — fixed (reset lastConnectedIp_/protocol_ on teardown).
- 🐇 AudioService active_ comment said "last one to setup() wins" — corrected to first-wins (matches the code).
- 🐇 IrService needed an explicit teardown() for the deletion path — added (already covered by the applyState refactor's teardown()).
- 🐇 backlog PinsModule + pins-analysis described shipped work — trimmed the backlog entry to the open item; pins-analysis kept as the shipped design record with its status banner.
KPI Details:
Desktop:
Lights: 16,384
Binary: 666 KB
tick: 120us, 91us, 121us, 15us, 2us, 283us, 60us, 17us, 21us, 162us, 117us, 16us, 1us, 36us (per scenario)
20 scenario(s), 20 passed, 0 failed
Platform boundary: PASS
Specs: 85 modules, 85 ok
ESP32 (P4):
Image: 1,381,202 bytes (67% partition free)
tick: 3642us (FPS: 274) heap free: 8522799
Code:
183 source files (36747 lines)
125 test files (18987 lines)
136 specs, 20 scenarios
Lizard: 133 warnings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/unit/light/unit_Layer_sparse_mapping.cpp (1)
23-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale comment referencing
onBuildState.Line 23 comment says "run onBuildState" but the code now calls
applyState(). Per coding guidelines, comments should use present-tense descriptions and not narrate removed or former behavior.✏️ Proposed fix
-// Build a Layer over a single layout, run onBuildState, return it wired. +// Build a Layer over a single layout, run applyState, return it wired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/light/unit_Layer_sparse_mapping.cpp` around lines 23 - 33, Update the comment above LayerRig to describe wiring a Layer over a single layout and applying its state, replacing the stale “run onBuildState” reference with the current applyState() behavior.Source: Coding guidelines
test/unit/light/unit_Layer_modifier_chain.cpp (1)
21-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale comment to reflect
applyState().The comment says "run onBuildState" but the code now calls
group.applyState()/layer.applyState(). As per coding guidelines, outdated comments should be updated.📝 Proposed fix
-// Build a dense w×h grid Layer with the given modifiers added in order, run onBuildState. +// Build a dense w×h grid Layer with the given modifiers added in order, run applyState.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/light/unit_Layer_modifier_chain.cpp` at line 21, Update the comment describing the dense grid layer helper to say it runs applyState() rather than onBuildState, matching the current group.applyState() and layer.applyState() calls.test/unit/light/unit_RmtLedDriver_lifecycle.cpp (1)
22-24: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale doc comment: still describes
onBuildState, but the call is nowapplyState().
wire()'s comment says "then run onBuildState (the sizing hook)" while the code callsd.applyState(). As per coding guidelines, "Use present-tense descriptions; do not narrate removed or former behavior."✏️ Proposed fix
-// Wire a driver up to a source buffer + correction the way the Drivers container -// does, then run onBuildState (the sizing hook). Returns nothing; the caller -// inspects the driver. +// Wire a driver up to a source buffer + correction the way the Drivers container +// does, then run applyState() (the sizing/build-or-release hook). Returns nothing; +// the caller inspects the driver.Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/light/unit_RmtLedDriver_lifecycle.cpp` around lines 22 - 24, Update the documentation comment for the wire() helper to describe its current behavior: wiring the driver to the source buffer and correction, then calling applyState(). Remove the stale onBuildState reference and avoid describing former behavior.Source: Coding guidelines
src/core/AudioService.h (1)
250-260: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc:
latestFrame()still says the seat is claimed insetup().The election now happens in
onBuildState()(setup() is a no-op per Line 228), but this comment still reads "The FIRST live module claims the seat insetup()". Update the reference toonBuildState()to match the relocated logic.As per coding guidelines,
**/*.{md,markdown,cpp,h,hpp,cc,cxx}: "Use present-tense descriptions; do not narrate removed or former behavior."📝 Proposed fix
- /// frame, so an effect added before/without a mic still reads valid silence - /// instead of null. The FIRST live module claims the seat in `setup()`, vacates - /// it in `teardown()`, and any running module re-claims an empty seat in + /// frame, so an effect added before/without a mic still reads valid silence + /// instead of null. The FIRST live module claims the seat in `onBuildState()`, + /// vacates it in `teardown()`, and any running module re-claims an empty seat in /// `loop()` — so a device with two mics reads the first consistently, and🤖 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/AudioService.h` around lines 250 - 260, Update the latestFrame() documentation to state that the first live module claims the seat in onBuildState(), replacing the stale setup() reference while preserving the rest of the election behavior description.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/history/plans/Plan-20260709` - Disabling releases resources (onEnabled
per module) (shipped).md:
- Line 1: Mark the plan title as “(attempted, abandoned)” instead of
“(shipped)”, and add a link to the succeeding shipped plan. Ensure the document
reflects that the proposed per-module onEnabled() routing was abandoned in favor
of centralized lifecycle handling through MoonModule::applyState().
In `@docs/history/plans/Plan-20260709` - Unify lifecycle - onBuildState is the
sole enabled-gate (shipped).md:
- Around line 1-80: The shipped plan documents an outdated lifecycle design and
must acknowledge the implemented applyState() routing. Update the plan’s
Decisions, design, file/scheduler descriptions, and scope language to state that
applyState() centrally routes enabled modules to onBuildState() and disabled
modules to teardown(), replacing direct onBuildState() calls; explain this
evolution from the original build-empty-when-disabled proposal and remove or
qualify claims that onBuildState is the sole gate, no third hook exists, and
Scheduler behavior is unchanged.
In `@src/core/MoonModule.h`:
- Around line 199-207: Correct the onBuildState() documentation to match
applyState(): state that onBuildState() is invoked only when the module is
effectively enabled, while the disabled path calls teardown() to release buffers
and peripherals. Remove or revise claims that onBuildState() handles
release-on-disable, and preserve the accurate lifecycle guidance in the
surrounding documentation.
- Around line 143-151: Update the onEnabled() documentation in MoonModule.h to
state that resources are built in onBuildState() when effectively enabled and
released in teardown() when disabled, rather than claiming both operations occur
in onBuildState(). Clarify that teardown() handles the full resource-release
sweep, including disabled child modules.
In `@test/unit/light/unit_Layouts_toggle_cycle.cpp`:
- Around line 38-44: Update the comment above rebuildAndTick to describe the
current applyState() calls instead of referencing onBuildState(), ensuring it
accurately reflects the function’s behavior.
In `@test/unit/light/unit_TextEffect.cpp`:
- Line 91: Replace the unbounded std::strcpy calls initializing tiny.text.text_
and zero.text.text_ with a bounded copy that specifies the destination capacity
and preserves null termination, using the appropriate buffer-size symbol or
sizeof expression.
---
Outside diff comments:
In `@src/core/AudioService.h`:
- Around line 250-260: Update the latestFrame() documentation to state that the
first live module claims the seat in onBuildState(), replacing the stale setup()
reference while preserving the rest of the election behavior description.
In `@test/unit/light/unit_Layer_modifier_chain.cpp`:
- Line 21: Update the comment describing the dense grid layer helper to say it
runs applyState() rather than onBuildState, matching the current
group.applyState() and layer.applyState() calls.
In `@test/unit/light/unit_Layer_sparse_mapping.cpp`:
- Around line 23-33: Update the comment above LayerRig to describe wiring a
Layer over a single layout and applying its state, replacing the stale “run
onBuildState” reference with the current applyState() behavior.
In `@test/unit/light/unit_RmtLedDriver_lifecycle.cpp`:
- Around line 22-24: Update the documentation comment for the wire() helper to
describe its current behavior: wiring the driver to the source buffer and
correction, then calling applyState(). Remove the stale onBuildState reference
and avoid describing former behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d8d057f-42c3-42f0-aecc-79258505f84a
⛔ Files ignored due to path filters (1)
docs/assets/reference/mhc-wled-esp32-p4-shield-pinout.svgis excluded by!**/*.svg
📒 Files selected for processing (96)
CLAUDE.mddocs/backlog/backlog-core.mddocs/backlog/backlog-light.mddocs/backlog/pins-analysis-top-down.mddocs/history/lessons.mddocs/history/plans/Plan-20260709 - Disabling releases resources (onEnabled per module) (shipped).mddocs/history/plans/Plan-20260709 - Unify lifecycle - onBuildState is the sole enabled-gate (shipped).mddocs/reference/gpio-usage.mddocs/reference/mhc-wled-esp32-p4-shield.mdsrc/core/AudioService.hsrc/core/DevicesModule.hsrc/core/HttpServerModule.cppsrc/core/IrService.hsrc/core/MoonModule.hsrc/core/PinsModule.hsrc/core/Scheduler.cppsrc/light/drivers/Drivers.hsrc/light/drivers/NetworkSendDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/PreviewDriver.hsrc/light/drivers/RmtLedDriver.hsrc/light/effects/BlurzEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/FireEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/NetworkReceiveEffect.hsrc/light/effects/ParticlesEffect.hsrc/light/effects/SolidEffect.hsrc/light/effects/StarFieldEffect.hsrc/light/effects/StarSkyEffect.hsrc/light/effects/TetrixEffect.hsrc/light/effects/WaveEffect.hsrc/light/layers/Layer.hsrc/light/layers/Layers.hsrc/light/layouts/Layouts.hsrc/light/moonlive/MoonLiveEffect.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_ir.cppsrc/platform/platform.htest/unit/core/unit_AudioService_sync.cpptest/unit/core/unit_DevicesModule_discovery.cpptest/unit/core/unit_MoonModule_replacechild.cpptest/unit/core/unit_PinsModule.cpptest/unit/light/unit_AudioService.cpptest/unit/light/unit_BlurzEffect.cpptest/unit/light/unit_BouncingBallsEffect.cpptest/unit/light/unit_DemoReelEffect.cpptest/unit/light/unit_DistortionWavesEffect.cpptest/unit/light/unit_Drivers_firstOutputRgb.cpptest/unit/light/unit_FireEffect.cpptest/unit/light/unit_FixedRectangleEffect.cpptest/unit/light/unit_FreqMatrixEffect.cpptest/unit/light/unit_FreqSawsEffect.cpptest/unit/light/unit_GEQ3DEffect.cpptest/unit/light/unit_GEQEffect.cpptest/unit/light/unit_Layer_extrude.cpptest/unit/light/unit_Layer_live_modifier.cpptest/unit/light/unit_Layer_modifier_chain.cpptest/unit/light/unit_Layer_persistence.cpptest/unit/light/unit_Layer_phase_animation.cpptest/unit/light/unit_Layer_sparse_mapping.cpptest/unit/light/unit_Layer_zero_grid.cpptest/unit/light/unit_Layers_container.cpptest/unit/light/unit_Layouts_toggle_cycle.cpptest/unit/light/unit_LcdLedDriver.cpptest/unit/light/unit_LissajousEffect.cpptest/unit/light/unit_MetaballsEffect.cpptest/unit/light/unit_NetworkReceiveEffect.cpptest/unit/light/unit_NetworkReceiveEffect_protocols.cpptest/unit/light/unit_NetworkSendDriver_no_alloc_in_loop.cpptest/unit/light/unit_Noise2DEffect.cpptest/unit/light/unit_NoiseEffect.cpptest/unit/light/unit_NoiseMeterEffect.cpptest/unit/light/unit_PaintBrushEffect.cpptest/unit/light/unit_ParlioLedDriver.cpptest/unit/light/unit_ParticlesEffect.cpptest/unit/light/unit_PlasmaEffect.cpptest/unit/light/unit_PraxisEffect.cpptest/unit/light/unit_PreviewDriver.cpptest/unit/light/unit_RainbowEffect.cpptest/unit/light/unit_RandomEffect.cpptest/unit/light/unit_RandomMapModifier.cpptest/unit/light/unit_RmtLedDriver_lifecycle.cpptest/unit/light/unit_RmtLedDriver_pins.cpptest/unit/light/unit_RubiksCubeEffect.cpptest/unit/light/unit_SineEffect.cpptest/unit/light/unit_SolidEffect.cpptest/unit/light/unit_SphereMoveEffect.cpptest/unit/light/unit_StarFieldEffect.cpptest/unit/light/unit_StarSkyEffect.cpptest/unit/light/unit_TetrixEffect.cpptest/unit/light/unit_TextEffect.cpptest/unit/light/unit_effects_render.cpp
💤 Files with no reviewable changes (7)
- src/light/drivers/PreviewDriver.h
- src/light/layers/Layers.h
- src/light/effects/FreqSawsEffect.h
- src/light/effects/BlurzEffect.h
- src/light/layouts/Layouts.h
- src/light/moonlive/MoonLiveEffect.h
- src/light/drivers/Drivers.h
| @@ -0,0 +1,53 @@ | |||
| # Plan — Disabling a module releases its resources (onEnabled per module) | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark this superseded plan as abandoned.
This plan specifies per-module onEnabled() routing, while the delivered lifecycle design centralizes the decision in MoonModule::applyState(). Label it (attempted, abandoned) and link to the succeeding shipped plan.
Proposed fix
-# Plan — Disabling a module releases its resources (onEnabled per module)
+# Plan — Disabling a module releases its resources (onEnabled per module) (attempted, abandoned)As per coding guidelines, “retain plans and mark completed outcomes with (shipped) or (attempted, abandoned).”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plans/Plan-20260709` - Disabling releases resources (onEnabled
per module) (shipped).md at line 1, Mark the plan title as “(attempted,
abandoned)” instead of “(shipped)”, and add a link to the succeeding shipped
plan. Ensure the document reflects that the proposed per-module onEnabled()
routing was abandoned in favor of centralized lifecycle handling through
MoonModule::applyState().
Source: Coding guidelines
"Desktop" is the better, industry-standard word for the Windows/macOS/Linux family (Flutter/Electron/Tauri all group them as "desktop"), and it's already the codebase's own convention (src/platform/desktop/, architecture.md). "pc" was the outlier. This renames it everywhere — the scenario perf-target keys, the MoonDeck tab, docs, and code comments. Raspberry Pi stays its own target (an SBC, not a desktop); local variables named pc, the "PC-relative" assembler term, and colloquial "Mac or PC" examples are left alone.
tick:122/94/119/15/2/278/61/18/21/167/120/18/1/38us(FPS:8196/10638/8403/66666/500000/3597/16393/55555/47619/5988/8333/55555/1000000/26315) | tick:3709us(FPS:269)
Core:
- platform_desktop.cpp / DevicesModule.h / ModifierBase.h: comment-only — "PC" → "desktop" where it named the platform (the ESP32 binary is byte-identical).
Tests:
- scenario_runner.cpp: hostTarget() returns desktop-macos / desktop-windows / desktop-linux / desktop-unknown; the tolerance-default branch is isDesktop (was isPc) keyed on the "desktop-" prefix.
- test/scenarios/*.json: 153 contract/observed keys renamed pc-macos → desktop-macos etc. across 16 scenarios — a pure key rename in lockstep with the runner (verified: symmetric diff, no tick/heap value drift).
Scripts / MoonDeck:
- run_scenario.py / run_live_scenario.py: _host_target() / _detect_target() emit desktop-<os>; the live runner's tolerance branch is is_desktop.
- run_network_live.py / run_network_roundtrip.py: the host machine is "desktop" (desktop→device→desktop round-trip).
- collect_kpi.py: the KPI one-liner label is "Desktop:" (was "PC:").
- generate_test_docs.py / screenshot_modules.py / update_module_docs.py / history_report.py: "PC"/"PC tab" prose → "Desktop".
UI:
- moondeck_ui: the MoonDeck tab is "Desktop" — button label, data-tab="desktop", id="tab-desktop", scripts-desktop grid, app.js grid key, and the persisted "tab" default all move together.
- moondeck_config.json / moondeck.py: the persisted tab state + defaults are "desktop".
- Renamed the screenshot asset moondeck_pc.png → moondeck_desktop.png (git mv) and every live reference; the frozen v1.0.0 release-notes URL is left pointing at the tagged file.
Docs / CI:
- testing.md / performance.md / building.md: contract keys and prose use desktop-<os>; the perf-contract section reads "desktop-*".
- MoonDeck.md / README.md: the tab is "Desktop"; the network-latency prose is desktop→device→desktop.
- CLAUDE.md: the KPI/cross-platform-smoke gates read "desktop + ESP32".
- lessons.md / v2-inventory.md / backlog leddriver survey: "PC" → "desktop" where it named the platform.
- scenario-tests.md regenerated (desktop-* keys).
KPI Details:
Desktop:
Lights: 16,384
Binary: 666 KB
tick: 116us, 93us, 119us, 16us, 2us, 273us, 58us, 17us, 20us, 156us, 113us, 16us, 1us, 34us (per scenario)
20 scenario(s), 20 passed, 0 failed
Platform boundary: PASS
Specs: 85 modules, 85 ok
ESP32 (P4):
Image: 1,381,202 bytes (67% partition free)
tick: 3709us (FPS: 269) heap free: 8520995
Code:
183 source files (36747 lines)
125 test files (18987 lines)
136 specs, 20 scenarios
Lizard: 133 warnings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A hands-on guide for writing effects (and later layouts, modifiers, drivers), written for beginners and school use: it teaches the core-orchestrates lifecycle we sharpened (prepare/tick via onBuildState/loop, release via teardown), with a real rainbow example. Alongside it, a single umbrella header per module kind so an author writes one #include instead of guessing the set. Also lands the review-findings fixes on the last two lifecycle commits and two design plans (a scratch-buffer primitive and the module hook rename).
tick:131/93/122/16/2/287/62/18/22/166/119/17/1/36us(FPS:7633/10752/8196/62500/500000/3484/16129/55555/45454/6024/8403/58823/1000000/27777) | tick:3624us(FPS:275)
Core:
- MoonModule: corrected the onBuildState / onEnabled doc comments to match the shipped applyState router (onBuildState is a pure build; the disabled path routes to teardown for release — the earlier text still said onBuildState both builds and releases).
- AudioService: corrected the latestFrame / loop comments — the mic seat is claimed in onBuildState now, not setup.
Light domain:
- Effect.h / Layout.h / Modifier.h / Driver.h: new umbrella headers. Each pulls in its base class plus that kind's common helpers (effects: Layer + draw + Palette + math8 + color), so a module includes one file. The umbrella is a separate thin header (not the base) because EffectBase forward-declares Layer while Layer.h defines EffectBase's accessors — folding Layer into EffectBase would be circular; the umbrella depends on both, neither base does.
- RainbowEffect: migrated to `#include "light/effects/Effect.h"` as the proof the umbrella works (the rest migrate later, folded into the hook-rename sweep so each file is touched once).
Tests:
- unit_TextEffect: the text_ writes use bounded snprintf(sizeof) instead of strcpy.
- unit_RmtLedDriver_lifecycle / unit_Layer_modifier_chain / unit_Layer_sparse_mapping / unit_Layouts_toggle_cycle: helper comments say applyState() (the actual call), not the stale onBuildState.
Docs / CI:
- usecases/build-your-own-moonmodules.md: the new guide. Teaches MoonModule as the universal building block, the hot-path framing, the two-then-four hooks, a real effect, when you need memory (onBuildState/teardown), a tour of the other kinds (drivers as the advanced topic), a testing section, the /// vs // documentation convention, and a classroom path. Written present-tense / describe-what-you-do throughout.
- coding-standards.md: the umbrella-header convention (one include per kind; new modules use it, existing ones migrate as a sweep) + the /// vs // rule already documented there.
- Plan-20260710 - Scratch buffer helper: a core ScratchBuffer<T> primitive (type-erased base + thin typed facade to avoid flash bloat) that removes the alloc/free/release/destructor/setDynamicBytes/static_cast/null-guard boilerplate from ~11 memory-holding effects.
- Plan-20260710 - Rename module hooks to prepare-tick-release (decided): an industry-standard survey (JUCE/Unity/Unreal/Vue/Angular + ESP-IDF/FreeRTOS) landing on the decision to rename onBuildControls->defineControls, onBuildState->prepare, loop->tick, teardown->release, onUpdate->onControlChanged; setup/onEnabled/applyState keep their names.
- Plan-20260709 (Disabling releases resources): relabeled (shipped -> shipped, superseded), noting the per-module onEnabled design shipped then was superseded by applyState.
- Plan-20260709 (Unify lifecycle): added an "as implemented" note that the design evolved to the applyState router; fixed the retired-study dead links.
Reviews:
- 🐇 MoonModule onBuildState / onEnabled docs claimed release happens in onBuildState — corrected to teardown via applyState.
- 🐇 AudioService latestFrame comment said setup() claims the mic seat — corrected to onBuildState.
- 🐇 unit_TextEffect used unbounded strcpy — switched to snprintf(sizeof).
- 🐇 four test-helper comments referenced onBuildState but call applyState — corrected.
- 🐇 the "Disabling releases resources" plan was labeled (shipped) but its design was superseded — relabeled (shipped, superseded) with a successor link. Reviewer suggested "(attempted, abandoned)"; declined — it did ship and run in production before being superseded, so "superseded" is accurate and "abandoned" is not.
KPI Details:
Desktop:
Lights: 16,384
Binary: 666 KB
tick: 121us, 91us, 121us, 15us, 2us, 280us, 59us, 17us, 21us, 160us, 137us, 23us, 1us, 35us (per scenario)
20 scenario(s), 20 passed, 0 failed
Platform boundary: PASS
Specs: 89 modules, 89 ok
ESP32 (P4):
Image: 1,381,202 bytes (67% partition free)
tick: 3624us (FPS: 275) heap free: 8522831
Code:
187 source files (36828 lines)
125 test files (18988 lines)
136 specs, 20 scenarios
Lizard: 133 warnings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Renames the MoonModule lifecycle hooks from their Arduino-era names to the industry-standard prepare/tick/release vocabulary (JUCE prepareToPlay/processBlock/releaseResources is the direct precedent), after an internet survey. A bare imperative verb now means work the module does (prepare/tick/release/defineControls); an on-prefix means a notification it observes (onEnabled/onControlChanged). Behaviour is unchanged; the one wire-format effect is the /api JSON field loopTimeUs becoming tickTimeUs. KPI: 16384lights | Desktop:666KB | tick:119/91/121/15/2/278/60/17/21/164/121/17/1/35us(FPS:8403/10989/8264/66666/500000/3597/16666/58823/47619/6097/8264/58823/1000000/28571) | ESP32:1386KB | tick:3639us(FPS:274) | heap:8324KB | src:187(36828) | test:125(18988) | lizard:133w Core: - MoonModule: renamed the lifecycle hooks loop/loop20ms/loop1s -> tick/tick20ms/tick1s, teardown -> release, onBuildState -> prepare, onBuildControls -> defineControls, onUpdate -> onControlChanged; kept setup/onEnabled/applyState. Private member loopTimeUs_ -> tickTimeUs_. - Scheduler: buildState -> prepareTree, teardown -> release, controlChangeTriggersBuildState -> controlChangeTriggersPrepare across the router and call sites. - HttpServerModule: /api/state and /api/system now emit the JSON field tickTimeUs (was loopTimeUs) — the one external-facing wire change; every in-repo consumer (app.js, live-scenario runner, baseline) moved with it. - All core modules (Audio/Ir services, Network, Mqtt, Filesystem, Firmware, Devices, Tasks, Pins, System, FileManager, MoonLive) migrated to the new hook names. Light domain: - Every effect, layout, modifier, driver, and Layer/Layers/Drivers container migrated to the new hook names. - SolidEffect/StarFieldEffect/StarSkyEffect/GameOfLifeEffect: their private release() helper (buffer-free) renamed to freeBuffers() first, so it no longer collides with the teardown->release hook rename. UI: - app.js: reads mod.tickTimeUs (was loopTimeUs). preview3d.js's own local animation loop() left as-is (not a hook). Tests: - Renamed every hook reference in unit-test bodies, TEST_CASE/SUBCASE display names, and scenario-JSON step descriptions; regenerated docs/tests/*.md from the updated sources. Docs / CI: - architecture.md, coding-standards.md, testing.md, performance.md, the core catalog pages, CLAUDE.md, gen_api.py, and 4 ADRs (identifier-only sync, decision prose unchanged) all point at the new hook names. - build-your-own-moonmodules guide: hook table + examples updated; added a beginner aside for tick(); fixed the memory example (public release() frees directly, dropped the redundant destructor and the duplicate private helper the rename had collided into). - S31 CoreBoard: loopback pins moved to Tx=48/Rx=47 (the two stacked pins of J2 column 7, jumperable with one cap); reference doc gained the full 2-row J2 silkscreen table + a bench-unconfirmed free-GPIO list (4, 36-49 skipping 41) and a GPIO4-for-LED note; deviceModels.json + catalog validated. Compressed the CoreBoard photo (6MB PNG -> 201KB JPG). - Added the active-instance-election primitive plan. Reviews: - 👾 Reviewer (Fable): tutorial SparkleEffect had a duplicate release() after the sweep — fixed by inlining the free and dropping the destructor. Fixed a falsified MoonLight prior-art citation (kept its real hook name onUpdate). Synced CLAUDE.md + the write-behaviour-tests workflow + unswept TEST_CASE names + comment stragglers. Corrected the S31 free-GPIO list that contradicted its own J2 table. Flagged the loopTimeUs->tickTimeUs API field change (noted above). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ella sweep Adds a core ScratchBuffer<T> primitive so a memory-holding effect declares one member and sizes it in prepare() — the primitive owns the alloc/free, the free-on-disable, the memory reporting, and the cast, once in core instead of hand-rolled per effect. Migrates all 11 such effects onto it, and sweeps every effect/modifier/layout/concrete-driver to include only its per-kind umbrella header. Net ESP32 flash shrinks ~1.4 KB: collapsing the duplicated alloc/free/release logic into one shared base outweighs the new primitive. KPI: 16384lights | Desktop:666KB | tick:125/93/122/15/2/280/60/17/21/160/117/17/1/35us(FPS:8000/10752/8196/66666/500000/3571/16666/58823/47619/6250/8547/58823/1000000/28571) | src:189(36606) | test:126(19125) | lizard:132w ESP32: image 1532560 bytes (−1.4 KB vs pre-change baseline). Live tick/FPS omitted — the configured KPI port was not the current bench port this session; the three boards (P4/S3/S31) were flashed and confirmed running separately. Core: - ScratchBuffer.h/.cpp: new primitive. A non-template ScratchBufferBase (in .cpp) owns the platform alloc/free, the resizeBytes body (realloc only on byte-count change, zero-fill, free on 0), the owner reference, and an intrusive free-list node; a thin template ScratchBuffer<T> façade forwards inline (the one static_cast lives in data()). Move/copy deleted — it is an owned member tied to its module. Base/façade split keeps flash flat: the heavy body compiles once, a new T adds ~0. - MoonModule: added addDynamicBytes(delta) + the scratchBuffers_ intrusive-list head + register/deregister, all private with `friend ScratchBufferBase` so a module can't mix them with setDynamicBytes. release() now frees every registered buffer (resizeBytes(0)) then recurses to children; documented that release() frees ALL resources (a driver's GPIO pins, a service's I²S mic, sockets — via each module's own override — AND buffers via the base), and that an override freeing hardware while holding a buffer must chain to MoonModule::release(). Light domain: - 11 effects migrated to ScratchBuffer, each dropping its raw pointer, count mirror, release()/free-helper/destructor, setDynamicBytes call, and static_cast: Fire, GEQ, Wave (keeps the same-bytes-different-geometry clear), Solid, Particles, Tetrix (ScratchBuffer<Tetris>), BouncingBalls (<Ball>), StarField (<Star>), StarSky (4 buffers), GameOfLife (3 buffers + test seam), NetworkReceive (keeps its release() override for sockets, chains to the base for the buffer). - Effect.h umbrella made the effect author's full standard library: EffectBase + render context + draw/palette/math/noise/color/crc/ScratchBuffer/AudioService/AudioFrame + cstring/cmath. Every effect now includes only Effect.h (plus genuinely-external headers: packet formats, fonts, ModuleFactory, platform). Modifier.h / Layout.h / Driver.h similarly enriched with their common tails; every modifier, layout, and concrete driver swept to its single umbrella. Zero firmware-byte cost (unused declarations emit no code). Tests: - unit_ScratchBuffer.cpp: pins resize grow/shrink/free, zero-fill, the dynamicBytes delta, multi-buffer summation, release()-frees-all, and destructor deregistration. Docs / CI: - build-your-own-moonmodules guide: the "When you need memory" section rewritten to the ScratchBuffer idiom (declare a member, one line in prepare(), done) — no release(), no setDynamicBytes, no cast; includes the multi-buffer case. The hook table's release() row updated (the core frees your buffers; override only for a non-buffer resource). - CMakeLists.txt + esp32/main/CMakeLists.txt: build ScratchBuffer.cpp. Reviews: - 👾 Reviewer (Fable): verified the primitive's memory/lifetime (destruct ordering safe, delta accounting exact, no double-free), audited all 15 release() overriders for the chain invariant (drivers + AudioService correct as leaf/no-buffer modules), and confirmed the multi-buffer delta sums match the old setDynamicBytes values and the umbrella has no include cycle or global-scope initializer. Three findings, all fixed: GEQ no longer re-zeroed peaks_ on a same-width rebuild (restored the explicit clear); GameOfLife's allocateForTest test-seam could inherit stale cells on a same-dims re-call (clear unconditionally); the ScratchBuffer registration API was public despite a private contract (moved to private + friend). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… housekeeping
Closes the orchestration reorg with the ActiveInstance<T> election primitive (the sibling of ScratchBuffer, for singleton-election state), renames the bespoke controlChangeTriggersPrepare hook to the industry-standard affectsPrepare (WPF's AffectsMeasure family, already cited as the precedent in our own docs), teaches the control-change hooks in the effect-author guide, and does a round of history/plans + bench-doc housekeeping.
KPI: 16384lights | Desktop:666KB | tick:117/89/116/14/2/273/58/17/20/155/115/16/1/36us(FPS:8547/11235/8620/71428/500000/3663/17241/58823/50000/6451/8695/62500/1000000/27777) | ESP32:1384KB | src:190(36671) | test:127(19228) | lizard:132w
Core:
- ActiveInstance.h: new header-only primitive for the "one active instance" election — a per-type static seat, owner-tied by reference (ActiveInstance<T> seat_{*this}), RAII-vacated. claim() is idempotent claim-if-empty, so the same call serves both the initial claim and a survivor's tick-reclaim; the destructor vacates (the dangling-static guard); copy/move deleted. Mirrors the ScratchBuffer owner-tie pattern without sharing its release-only registry (election claims in prepare + reclaims in tick, a different lifecycle).
- AudioService: replaced the four hand-written active_ moves (claim-if-empty, vacate-if-mine, destructor guard, tick self-elect) + the static active_ with an ActiveInstance<AudioService> micSeat_ member; latestFrame() reads the primitive's active().
- DevicesModule: same, with ActiveInstance<DevicesModule> seat_; unified its unconditional claim onto claim-if-empty (safe — it is a boot-wired singleton, so the semantics are identical). active() delegates to the primitive.
- MoonModule + Scheduler + all Layout/Modifier/Driver/MoonLive overriders: renamed controlChangeTriggersPrepare -> affectsPrepare. The old name was a bespoke sentence (xTriggersY); the new one is the recognizable WPF AffectsMeasure/AffectsArrange family — a per-property flag meaning "changing this invalidates and re-runs the pass" — which architecture.md and ADR-0011 already named as the precedent. Shorter (28 -> 14 chars), and honest to the pattern we claim.
Tests:
- unit_ActiveInstance.cpp: pins claim-first-wins, second-claimant-doesn't-displace, vacate-only-if-mine, survivor-reclaims-via-claim, destructor-vacates (the dangling-static regression), per-type seats. The existing AudioService two-mic election and DevicesModule seat tests pass unchanged — they assert the behaviour the primitive preserves.
Docs / CI:
- build-your-own-moonmodules guide: added a "Reacting to a control change" section teaching the control-change split — onControlChanged (a cheap per-edit reaction) and affectsPrepare (return true when a change reshapes derived state → the core re-runs prepare() off the hot path), with the real GameOfLife pattern.
- architecture.md, testing.md, ADR-0011: affectsPrepare rename synced.
- esp32-s31-coreboard.md + history/lessons.md: recorded the J5 finding — J5 is the S31's power/enable jumper; with it removed the board half-powers (the USB bridge enumerates but the MCU is dead-silent on UART), which cost an hour of chasing a cable that wasn't the problem. Also recorded the S31 loopback validation (RmtLed + ParlioLed PASS; LcdLed is S3-only).
- history/plans: marked 32 shipped plans that had never received their (shipped) marker (including the two 2026-07-10 primitives and the rename plan whose stale (decided) meant "approach chosen", not "shipped"); consolidated the four Pins increments into one flat (shipped) record preserving each phase's design-intent arc.
- backlog-mixed.md: the intermittent-RMT-LED-pause item marked RESOLVED — the strongest suspect (WiFi modem power-save) was fixed in 7d0cfaf, and a bench re-verification across all four boards (S3/classic on WiFi, S31/P4 on Ethernet) showed zero pauses over cumulative minutes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ewowi
changed the title
Collaborator
Author
|
@coderabbitai, we are about to merge this PR, do a review over all commits and place the findings in copy pastable text for agents |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== MoonModule.h relevant section ==="
fd -a MoonModule.h src/core | xargs -I{} sed -n '1,400p' {}
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>
</br>Length of output: 26788 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== TasksModule.h ==="
fd -a TasksModule.h src | xargs -I{} cat -n {}
echo "=== PinsModule.h ==="
fd -a PinsModule.h src | xargs -I{} cat -n {}Length of output: 35038 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== platform_esp32_tasks.cpp ==="
fd -a platform_esp32_tasks.cpp src | xargs -I{} cat -n {}
echo "=== platform_esp32_gpio.cpp ==="
fd -a platform_esp32_gpio.cpp src | xargs -I{} cat -n {}Length of output: 13769
[resolve] |
… rule Merge-prep for PR #43: fixes the Reviewer + CodeRabbit findings from the whole-branch pre-merge review (including the two traps the orchestration reorg created for itself), and formalises "a catalog module includes only its umbrella" as a coding-standards rule by folding the last common standard headers into the umbrellas. KPI: ESP32 image 1532356 bytes (still ~1.5 KB below the pre-reorg baseline — the maximal umbrella is byte-free; unused declarations emit no code). Core: - DriverBase: release() now chains to MoonModule::release() — it dead-ended one level early, so no LED driver (all of which chain to DriverBase::release()) ran the base's ScratchBuffer-free loop or child recursion. No leak today (no driver holds a buffer), but the first one that adds one would have leaked despite following the convention. The trap the ScratchBuffer commit created. - AudioService, HttpServerModule: release() overrides now chain to MoonModule::release() too (uniform override-and-chain; harmless today, both leaf/no-buffer). - PinsModule: the pin-map's active-gate was a third predicate (!respectsEnabled() || effectivelyEnabled()) that diverged from the applyState router (which gates on effectivelyEnabled() alone) — in the respectsEnabled-false-under-disabled-ancestor corner the map would show a pin held that the router freed. Now uses plain effectivelyEnabled(), and the comment (which claimed Tasks/Pins override respectsEnabled — they don't) is corrected. Light domain: - Drivers: migrated its hand-rolled `active_` summary seat to ActiveInstance<Drivers> — the third instance of the election pattern (the ActiveInstance commit migrated two and walked past this one). It had no destructor guard, the exact dangling-static bug the primitive exists to prevent. Docs / CI: - coding-standards: rewrote the umbrella bullet into a firm rule — "a catalog module includes ONLY its umbrella." The umbrella is the module author's standard library (base + domain helpers + ScratchBuffer + audio + the stdlib headers the bodies use); a second include is permitted only for a genuinely-external helper (packet format, font, factory, platform) with a justification comment. A common header two-plus modules reach for moves INTO the umbrella instead. Folded <cstdint>/<array> into Effect.h and <algorithm> into Driver.h; dropped the now-redundant per-file stdlib includes across effects + drivers so every catalog module leads with one include. - backlog-core: two Reviewer findings backlogged with the core fix named — heap-allocate the registerType<T> boot stack-probe (lift the per-module stack-overflow lesson into core), and migrate HueDriver's name buffers to ScratchBuffer (now unblocked by the DriverBase chain fix). Reviews: - 👾 Reviewer (Fable, whole branch): reorg is architecturally coherent (the four primitives share one owner-tie idiom; the applyState router genuinely centralizes the enabled-gate). 8 findings — 6 fixed here (the release-chain traps, the Drivers seat, the PinsModule gate/comment, two non-chaining overrides, a stale doc line, include nits), 2 backlogged (registerType probe, HueDriver buffers) with the core fix named. - 🐇 CodeRabbit (PR #43): "ready to merge from a correctness standpoint" — independently verified the lifecycle refactor is applied consistently across effects/drivers/layers with thorough test coverage; only two low-priority nits (zero-length capability arrays would trip -Wpedantic if ever enabled; the diagnostic-view kMaxTasks/kMaxClaims caps silently truncate on a board that exceeds them) — both noted, no action. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This branch grew well beyond its original scope (TasksModule + System/Services) into a module-orchestration reorg plus the modules and bench work that surrounded it. Twelve commits, landing four connected threads.
1. The orchestration reorg (the through-line)
Core now owns the module lifecycle end-to-end, and two patterns that were hand-copied across many modules were lifted into core primitives:
applyState()is the sole enabled-gate. A single core router callsprepare()(build) on an effectively-enabled node andrelease()(release) otherwise, recursing the tree — so a catalog module'sprepare()contains noenabled()check, and disabling any container cleanly releases its whole subtree's memory and hardware. This replaced ~24 per-moduleenabled()guards.prepareToPlay/processBlock/releaseResourcesis the direct precedent):loop→tick,teardown→release,onBuildState→prepare,onBuildControls→defineControls,onUpdate→onControlChanged. Keptsetup/onEnabled/applyState. Naming rule: a bare verb = work the module does; anon…prefix = a notification it observes.ScratchBuffer<T>— a core primitive that replaces the alloc/free/free-on-disable/report-memory/cast boilerplate in the ~11 memory-holding effects. Each effect now declares one member (ScratchBuffer<uint8_t> heat_{*this}) and sizes it inprepare(); the primitive owns the rest via an owner-tied RAII member. ESP32 flash shrank ~1.4 KB (the base/façade split eliminates more duplication than the primitive adds).ActiveInstance<T>— the sibling primitive for the "one active instance" singleton election, replacing the hand-writtenactive_ == thisbookkeeping in AudioService and DevicesModule.controlChangeTriggersPrepare→affectsPrepare— bespokexTriggersY→ the recognizable WPFAffectsMeasurefamily (already cited as the precedent in architecture.md + ADR-0011).2. New modules + the System/Services split
Peripheralrole becameService; fixed inspection modules (Tasks, I2cScan) are now wired-by-code System children, and user-added capability modules (Audio, IR) live in a new Services container — the core-domain twin of the light pipeline's Layers/Drivers.3. Docs + author experience
onControlChanged+affectsPrepare), and the ScratchBuffer memory idiom.Effect.h/Layout.h/Modifier.h/Driver.h) — every effect/modifier/layout/concrete-driver now includes only its umbrella (the author's standard library), byte-free at the firmware level.4. Platform + bench
pc→desktopeverywhere (Raspberry Pi stays separate asrpi).lessons.md+ the S31 reference: the S31 J5 power/enable jumper (removing it half-powers the board — the USB bridge enumerates but the MCU is dead-silent on UART), and the S31 loopback validation (RmtLed + ParlioLed PASS; LcdLed is S3-only).Verification
-Wall -Wextra -Werror, ctest, scenarios, spec check, platform boundary, ESP32 build, KPI).tickTimeUs), ScratchBuffer, and ActiveInstance run on the P4 (eth), S3 (WiFi), S31 (eth), and classic Olimex (WiFi); all render, and the S31's RmtLed + ParlioLed loopback self-tests PASS on real silicon.Notes
/apiJSON fieldloopTimeUsbecametickTimeUs(with the hook rename). Every in-repo consumer moved with it; an external dashboard polling the old field would need updating.docs/history/plans/Plan-20260710 - …records andlessons.md; ADRs 0001/0003/0005/0011 had their hook-name identifiers synced.🤖 Generated with Claude Code