Composable modifiers: chain the modifier stack (physical→logical fold)#29
Open
ewowi wants to merge 6 commits into
Open
Composable modifiers: chain the modifier stack (physical→logical fold)#29ewowi wants to merge 6 commits into
ewowi wants to merge 6 commits into
Conversation
Multiple modifiers per Layer now compose as a chain (Region → Multiply → Rotate), each a coordinate fold applied in child order. The Layer's mapping build is inverted from virtual→physical fan-out to physical→logical fold (MoonLight's proven model, written fresh), which makes chaining fall out, deletes the fan-out overflow risk, and leaves the render hot path untouched. RegionModifier gains off-screen windows so an effect can slide partly or fully out of view without rescaling. Also folds in the white-theme preview-background fix. S3 live: tick:3482us(FPS:287). PC: tick:120/87/118/9/2/323/36/15/18/56/115/11/19us(FPS:8333/11494/8474/111111/500000/3095/27777/66666/55555/17857/8695/90909/52631). Classic ESP32 tick from a stale monitor log — no classic board attached this session (only the S3 was connected); esp32 + esp32s3-n16r8 firmware both build clean. Light domain: - ModifierBase: new fold interface (modifyLogicalSize / bool modifyLogical / modifyLive + hasModifyLive + consumeNeedsRebuild), replacing the single-modifier logicalDimensions/mapToPhysical/maxMultiplier - Layer: rebuildLUT inverted to a physical→logical counting-sort CSR build folding the whole enabled-modifier chain; each modifier folds in its OWN stage's box (stashed, MoonLight-style, no per-stage array); per-frame live pass for dynamic modifiers, gated on hasLive_ (a static-only chain pays nothing); coalesced one-rebuild-per-frame; deleted buildBoxToDriver/buildSparseIdentityLUT/fan-out-ceiling - MultiplyModifier/CheckerboardModifier/RandomMapModifier: rewritten as folds - RegionModifier: fold + reject; off-screen windows (start/end unclamped, full-window logical size — move without rescale, fully-off renders dark) - RotateModifier: dynamic modifyLive per-frame remap via an explicit integer 2×2 rotation matrix (the codebase's transform-matrix reference); no more step-gated rebuild - light_types.h: add Coord3D (per-axis operators) UI: - preview3d/app.js: read --bg-0 from <body> (the theme override) not <html>, so the preview canvas matches the light theme; redraw on theme toggle Tests: - unit_Coord3D; unit_Layer_modifier_chain (compose, A∘B≠B∘A, disabled-skip, per-stage regression); unit_Layer_live_modifier (live remap, pay-for-what-you-use gate, disabled gate, two-dynamic coalescing); off-screen Region cases; all 5 modifier tests rewritten to the fold interface - scenario_runner: register Region/Rotate/RandomMap (the chain scenario was silently skipping them); scenario_modifier_chain exercises Region+Multiply+Checkerboard+live Rotate Docs: - ModifierBase.md (fold contract + prior art); architecture.md (chain build, drop stale "multi-layer composition" WIP example — it shipped); modifier specs to the fold model; RegionModifier.md off-screen windows; decisions.md inversion lesson; backlog: delete shipped composed-modifiers item, update chain-viz premise; Plan-20260626 saved Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (44)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/light/unit_RandomMapModifier.cpp (1)
121-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winObserve Layer state here, not
RandomMapModifierstate.
mappingChangesOverMs()comparesmapAll(mod, ...)before and after the timed run, so the post-beat read can rebuild the permutation throughmodifyLogical*even ifLayer::loop()never consumes the rebuild request. If this test is meant to cover the layer-driven rebuild path described in the comment, comparelayer.lut()or rendered output before/after instead.As per path instructions,
test/**: “Verify tests cover edge cases and match the specifications in docs/moonmodules/.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/light/unit_RandomMapModifier.cpp` around lines 121 - 143, The test is asserting on RandomMapModifier state instead of the Layer-driven behavior it is meant to cover. Update mappingChangesOverMs() to observe the Layer after Layer::loop() runs, using layer.lut() or equivalent rendered output rather than mapAll(mod, ...), so the check verifies whether the layer consumes the modifier’s rebuild request and rebuilds the mapping as expected.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture.md`:
- Around line 356-358: The architecture doc’s later ## Modifiers section still
describes the old transformCoord()/transformLights() API, which conflicts with
the new fold-based modifier chain contract. Update that section to match the
current ModifierBase model by describing the three hooks, the
physical-to-logical build in Layer::rebuildLUT, and the live pass behavior, and
remove any references to the removed API so the document is consistent with the
current implementation.
- Line 525: Rephrase the 🚧 marker definition in present-tense terms so it
describes what the marker denotes now, not what is missing or will happen later.
Update the wording in the architecture docs around the marker explanation to
distinguish it from genuinely open questions using current-state language, and
keep the distinction clear in the same section where the marker is defined.
In `@docs/backlog/backlog-mixed.md`:
- Line 9: The backlog note still describes the removed 1:N fan-out model, so
update the text to match the current physical→logical fold behavior. In the
affected entry, revise the MultiplyModifier cost description to say the
destinations array is bounded by physical light count and that each physical
light contributes at most one destination, using the existing
MultiplyModifier/scenario_perf_full wording as the anchor. Remove or rewrite any
“fan-out mapping LUT” phrasing so it no longer implies the old build-time
fan-out mechanism.
In `@docs/history/plans/Plan-20260626` - Composable modifiers (chain the whole
stack).md:
- Around line 69-75: Update the plan text to match the current ModifierBase API:
remove the discarded modifyLogical(pos, phys, logical) contract and describe the
landed modifyLogical(Coord3D&) shape instead. Also revise the
stateless/phys-logical wording so it reflects the actual fold implementation and
cached stage state used by modifiers like MultiplyModifier (for example tile_),
and keep the references aligned with ModifierBase.h and the modifier folding
behavior.
In `@docs/moonmodules/light/ModifierBase.md`:
- Around line 7-13: Update the ModifierBase documentation so the `modifyLogical`
hook matches the current API exposed by `ModifierBase.h`: document `bool
modifyLogical(Coord3D& pos) const` instead of the older three-argument form.
Keep the surrounding explanation focused on the actual contract for
`modifyLogical`, and adjust any text in this page that still describes `phys`
and `logical` parameters so the integrator-facing docs stay aligned with the
class interface.
In `@docs/moonmodules/light/modifiers/RegionModifier.md`:
- Around line 24-27: The off-screen window description in RegionModifier.md uses
forward-looking wording in the last sentence, which should be rewritten in
present tense to describe the current contract. Update the prose around the
“Off-screen windows (move, don’t rescale)” section so it says the fixed-span
window already lets an effect move across and off the panel without distortion,
and keep the wording consistent with the existing RegionModifier behavior
description.
In `@docs/moonmodules/light/modifiers/RotateModifier.md`:
- Around line 3-17: The RotateModifier.md page repeats the header’s
implementation walkthrough instead of staying at the contract level. Trim the
detailed matrix/backward-mapping explanation from the RotateModifier section and
keep only the user-facing behavior tied to RotateModifier, modifyLive, and loop:
the speed control, that rotation happens in the Layer live pass, and that
out-of-bounds sources render dark, while preserving prior art and the required
control name mention.
In `@src/light/layers/Layer.h`:
- Around line 287-294: The child scan in Layer::build/chain construction is
stopping too early because the `childCount()` loop is guarded by `chainLen <
kMaxChain`, which prevents later modifiers from being visited at all. Update the
loop so it always iterates through every child, keep the `ModuleRole::Modifier`
and `enabled()` checks, and only apply the `kMaxChain` limit when appending to
`chain[chainLen++]`; also preserve the `hasLive_` update from
`ModifierBase::hasModifyLive()` for any live modifier encountered after the
static chain is full.
- Line 21: Move liveScratch_ allocation, resizing, and freeing out of the hot
render path so applyLivePass() only snapshots and remaps. Update Layer’s cold
build path, such as rebuildLUT(), to own the liveScratch_ lifecycle and keep
loop()/render-path code free of heap churn; ensure the Layer destructor only
performs final cleanup. Refer to the Layer class and
applyLivePass()/rebuildLUT() when relocating this management.
In `@src/light/modifiers/MultiplyModifier.h`:
- Around line 46-85: The remainder of non-divisible axes is being incorrectly
wrapped by MultiplyModifier, causing leftover edge pixels to duplicate instead
of being dropped. Update MultiplyModifier’s modifyLogicalSize/modifyLogical flow
so that when an axis extent is not evenly divisible by the effective multiplier,
the partial strip is rejected before foldAxis is applied, rather than mapping it
back through tile_. Keep the existing behavior for divisible extents and mirror
handling in modifyLogical(), and make sure the change is localized around eff(),
foldAxis(), and the tile_ logic.
In `@src/light/modifiers/RandomMapModifier.h`:
- Around line 69-97: `RandomMapModifier` is still triggering a full Layer
rebuild on `bpm` updates because
`ModifierBase::controlChangeTriggersBuildState()` falls back to the default
behavior. Override this in `RandomMapModifier` so the `bpm` control returns
false and only affects future `loop()` timing, while keeping the existing
`loop()`/`consumeNeedsRebuild()` beat-driven rebuild path for actual permutation
changes.
- Around line 39-42: The comment block around
RandomMapModifier::modifyLogicalSize is contradictory because it says there is
no override even though the override exists in the same class. Update the
surrounding comments in RandomMapModifier.h to remove the stale “no
modifyLogicalSize override” wording and keep only the explanation that
modifyLogicalSize stores the logical box in box_ for permutation use, so the
fold contract reads consistently.
In `@src/light/modifiers/RotateModifier.h`:
- Around line 35-42: The RotateModifier still inherits the default
control-change rebuild path, so changing speed unnecessarily triggers
Layer::onBuildState() even though rotation now happens only in modifyLive().
Update RotateModifier to override controlChangeTriggersBuildState() and exclude
speed from rebuild-triggering controls, keeping speed as a live-only parameter
while preserving the existing onBuildControls() and modifyLive() behavior.
In `@test/scenarios/light/scenario_modifier_chain.json`:
- Around line 89-94: This scenario is still tied to host timing and network
behavior through the tick_us baselines and the NetworkSendDriver module. Replace
the NetworkSendDriver-based fixture in scenario_modifier_chain.json with an
in-process sink path, and update the assertions to validate only the
modifier-chain structure and expected module wiring. Keep the scenario hermetic
by avoiding any timing-sensitive baselines or network-path dependencies in the
affected sections.
In `@test/unit/light/unit_Layer_live_modifier.cpp`:
- Around line 148-163: The current `destCount()` assertion only checks the total
number of LUT edges and does not prove the mapping is still a true permutation
or that a rebuild happened after the beat boundary. Update this test around
`layer.lut()`, `forEachDestination`, and the `layer.loop()` tick sequence to
capture the before/after destination mapping or per-destination hit counts, then
assert every logical cell maps exactly once and that the post-beat mapping
differs appropriately when the coalesced rebuild should occur.
In `@test/unit/light/unit_Layer_modifier_chain.cpp`:
- Around line 94-100: The LUT fingerprint in the test helper is flattening all
destinations into one vector, which can hide differences in per-logical-cell
grouping. Update the fingerprint lambda on mm::Layer::lut() to encode each
logical cell boundary explicitly (for example by separating cells in the
collected data or otherwise preserving per-cell chunks) so comparisons detect
mappings that have the same flattened sequence but different cell grouping.
---
Outside diff comments:
In `@test/unit/light/unit_RandomMapModifier.cpp`:
- Around line 121-143: The test is asserting on RandomMapModifier state instead
of the Layer-driven behavior it is meant to cover. Update mappingChangesOverMs()
to observe the Layer after Layer::loop() runs, using layer.lut() or equivalent
rendered output rather than mapAll(mod, ...), so the check verifies whether the
layer consumes the modifier’s rebuild request and rebuilds the mapping as
expected.
🪄 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: b8fdaf53-6273-45da-9113-19b330d4d459
📒 Files selected for processing (32)
docs/architecture.mddocs/backlog/README.mddocs/backlog/backlog-core.mddocs/backlog/backlog-mixed.mddocs/history/decisions.mddocs/history/plans/Plan-20260626 - Composable modifiers (chain the whole stack).mddocs/moonmodules/light/ModifierBase.mddocs/moonmodules/light/modifiers/CheckerboardModifier.mddocs/moonmodules/light/modifiers/MultiplyModifier.mddocs/moonmodules/light/modifiers/RegionModifier.mddocs/moonmodules/light/modifiers/RotateModifier.mdsrc/light/layers/Layer.hsrc/light/light_types.hsrc/light/modifiers/CheckerboardModifier.hsrc/light/modifiers/ModifierBase.hsrc/light/modifiers/MultiplyModifier.hsrc/light/modifiers/RandomMapModifier.hsrc/light/modifiers/RegionModifier.hsrc/light/modifiers/RotateModifier.hsrc/ui/app.jssrc/ui/preview3d.jstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/light/scenario_modifier_chain.jsontest/unit/light/unit_CheckerboardModifier.cpptest/unit/light/unit_Coord3D.cpptest/unit/light/unit_Layer_live_modifier.cpptest/unit/light/unit_Layer_modifier_chain.cpptest/unit/light/unit_MultiplyModifier.cpptest/unit/light/unit_RandomMapModifier.cpptest/unit/light/unit_RegionModifier.cpptest/unit/light/unit_RotateModifier.cpp
Comment on lines
+69
to
+75
| ### `src/light/modifiers/ModifierBase.h` — interface inversion | ||
| - Replace `logicalDimensions()` + `mapToPhysical()` + `maxMultiplier()` with: | ||
| - `virtual void modifyLogicalSize(Coord3D& size) const {}` (default: no resize) | ||
| - `virtual bool modifyLogical(Coord3D& pos, const Coord3D& phys, const Coord3D& logical) const { return true; }` (default: pass-through) | ||
| - `virtual void modifyLive(Coord3D& pos, const Coord3D& logical) const {}` (default: no per-frame work; presence detected so absence = zero cost) | ||
| - Keep `dimensions()` (the 📏/🟦/🧊 chip) and `controlChangeTriggersBuildState` (already `true`). | ||
| - `phys`/`logical` passed in (not stashed on the modifier) so modifiers stay stateless and the two-pass build can't desync a cached size. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This plan still documents the discarded modifyLogical(pos, phys, logical) contract.
These bullets point readers at an API that no longer exists. The landed contract in src/light/modifiers/ModifierBase.h is modifyLogical(Coord3D&), and the fold implementation now relies on per-modifier cached stage state (tile_ in MultiplyModifier) rather than the stateless phys/logical parameters described here.
🤖 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-20260626` - Composable modifiers (chain the whole
stack).md around lines 69 - 75, Update the plan text to match the current
ModifierBase API: remove the discarded modifyLogical(pos, phys, logical)
contract and describe the landed modifyLogical(Coord3D&) shape instead. Also
revise the stateless/phys-logical wording so it reflects the actual fold
implementation and cached stage state used by modifiers like MultiplyModifier
(for example tile_), and keep the references aligned with ModifierBase.h and the
modifier folding behavior.
Comment on lines
+89
to
+94
| "name": "fix-artnet", | ||
| "op": "add_module", | ||
| "id": "ArtNet", | ||
| "type": "NetworkSendDriver", | ||
| "parent_id": "Drivers" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep this scenario off timing- and network-sensitive paths.
The tick_us baselines make this fixture host-performance dependent, and routing it through NetworkSendDriver pulls network-path behavior into a test whose purpose is modifier-chain composition. Use an in-process sink and assert structural results instead so the scenario stays hermetic.
As per path instructions, test/**: “Tests should not depend on timing or network.”
Also applies to: 101-118, 130-147, 157-174
🤖 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/scenarios/light/scenario_modifier_chain.json` around lines 89 - 94, This
scenario is still tied to host timing and network behavior through the tick_us
baselines and the NetworkSendDriver module. Replace the NetworkSendDriver-based
fixture in scenario_modifier_chain.json with an in-process sink path, and update
the assertions to validate only the modifier-chain structure and expected module
wiring. Keep the scenario hermetic by avoiding any timing-sensitive baselines or
network-path dependencies in the affected sections.
Source: Path instructions
Addresses the CodeRabbit pass on PR #29 (composable modifiers): fixes a Multiply edge-duplication bug on non-divisible grids, moves the live-pass snapshot allocation off the render path, stops needless LUT rebuilds on live-only control edits, strengthens two modifier tests, and corrects the modifier docs to the shipped fold contract. Also folds in the kMaxChain removal (no predefined maxes) and the getting-started board→device wording. Hardware-verified on the classic Olimex ESP32 and S3; 16K perf unchanged. KPI: 16384lights | PC:384KB | tick:116/93/118/9/1/323/36/16/18/55/115/11/19us(FPS:8620/10752/8474/111111/1000000/3095/27777/62500/55555/18181/8695/90909/52631) | ESP32:1231KB | src:98(20354) | test:73(11451) | lizard:77w (ESP32 16K heavy-Noise live this session: classic 62,316us/16 FPS, S3 51,959us/~20 FPS — matches committed performance.md; KPI script's hardcoded port missed the live capture, values read from the perf_full write-back.) Core: - Layer: removed the fixed kMaxChain cap and chain[] array — the size-fold and counting-sort passes iterate the dynamic child list directly, so a trailing live modifier after many statics still sets hasLive_ (no predefined maxes). 🐇/👾 (both flagged the same cap) - Layer: moved liveScratch_ (re)allocation out of the render path into a cold ensureLiveScratch() called from onBuildState; the per-frame live pass now only memcpys, never allocates, and the scratch is freed when no modifier is live (no longer pinned). 🐇 Light domain: - MultiplyModifier: a non-divisible extent (e.g. 5 wide, multiply 2) leaves a leftover edge strip the tiles don't cover — modifyLogical now rejects those pixels instead of wrapping them, which had duplicated the edge. Stashes the covered extent per axis. 🐇 - RotateModifier: controlChangeTriggersBuildState=false — speed is applied live in modifyLive, so a speed edit no longer triggers a full LUT rebuild. 🐇 - RandomMapModifier: controlChangeTriggersBuildState=false — bpm only changes future reshuffle timing; removed a stale comment that denied the modifyLogicalSize override. 🐇 Tests: - unit_MultiplyModifier: regression for the leftover-edge drop on a non-divisible extent. 🐇 - unit_Layer_live_modifier: the coalesced-rebuild test now snapshots the full per-cell mapping, asserts it stays a bijection, and that a beat actually changes it (was only counting edges). 🐇 - unit_Layer_modifier_chain: the order-sensitivity fingerprint now encodes per-cell boundaries, so two mappings that flatten the same but group differently compare unequal. 🐇 Docs / CI: - architecture.md: rewrote the § Modifiers section to the static/live fold contract (dropped the never-existent transformCoord/transformLights); made the 🚧-marker definition present-tense. 🐇 - ModifierBase.md: corrected modifyLogical to the 1-arg bool signature (modifiers stash their own box). 🐇 - RegionModifier.md / RotateModifier.md: off-screen and rotation descriptions to present tense / contract level (matrix detail stays in the header). 🐇 - backlog-mixed.md: removed the self-contradictory "1:N fan-out LUT" framing — the cost is the destinations array, bounded by light count. 🐇 - gettingstarted.md: board→device wording (a board is a bare PCB; the assembled product is a device). - install/README.md: trimmed the stale "renders only its first enabled effect" rendering digression from the install doc. - 🐇 scenario_modifier_chain timing/network: accepted as-is — the scenario has no timing assertions (only informational observed drift), and NetworkSendDriver is the established pattern (11/13 light scenarios, incl. the sibling swap scenario); a one-off hermetic scenario would diverge from convention. - 🐇 Plan-20260626 stale 3-arg contract: accepted — saved plans are frozen design-intent records (CLAUDE.md keeps plans; divergence from what shipped is expected), not rewritten to match the code.
… fix
Two features. (1) MoonLive: the first vertical slice of projectMM's live-script engine — author an effect as text, compile it to native machine code on the device, run it in the render tick. A scripted effect's `source` control ("fill(r,g,b);") is lexed/parsed/codegen'd to Xtensa on-device and rendered; editing it recompiles live. Proven on desktop (arm64/x86-64), ESP32-S3 (LX7) and classic ESP32 (LX6). (2) Installer: "apply device defaults" now lands the full config without erasing the flash — it clears every container it re-populates and adds Grid/Layer explicitly instead of assuming boot defaults, so a non-erased device gets the same tree an erase+flash produces.
KPI: 16384lights | PC:402KB | tick:218/88/117/9/1/319/36/16/19/56/117/11/34us(FPS:4587/11363/8547/111111/1000000/3134/27777/62500/52631/17857/8547/90909/29411) | ESP32:1234KB | src:106(21130) | test:75(11674) | lizard:79w
(MoonLive ran live this session on the S3 (~230 fps with the scripted effect) and classic (~1960 fps); the KPI script's hardcoded serial port missed the live ESP32 tick capture, so those figures are from the on-device API, not collect_kpi.)
Core:
- platform: new executable-memory seam — allocExec/freeExec/writeExec. ESP32 allocates IRAM (MALLOC_CAP_EXEC) and writeExec does the 32-bit-aligned store + esp_cache_msync I-cache sync; desktop mmaps a PROT_EXEC page (macOS arm64 needs MAP_JIT + a pthread_jit_write_protect_np toggle — the W^X wall). This is what a JIT needs: write-then-execute memory.
- MoonLive (src/core/moonlive/): the domain-neutral engine — compile(source)/run(buf,n,cpl,t)/free(); holds an exec block, calls it through a function pointer. Includes only cstdint, the compiler/emitter seams, and the platform seam — never EffectBase/Buffer.
- MoonLiveCompiler: a hand-written recursive-descent lexer + parser + codegen for the one-statement grammar fill(r,g,b);. Pure, deterministic, knows the language never an ISA; codegen drives the per-ISA emitter so the parser adds no codegen of its own (golden-bytes equivalence).
- moonlive_emit (per-ISA backend, behind the platform line): hand-encoded fill + animated-fill routines — Xtensa in src/platform/esp32/, host arm64/x86-64 in src/platform/desktop/. Byte arrays copied verbatim from the assembler output (a hand-transcribed Xtensa byte caused a StoreProhibited crash mid-spike; regenerated from the raw blob, comment added).
Light domain:
- MoonLiveEffect (src/light/moonlive/): the thin EffectBase binding — carries the `source` text control, compiles it in onBuildState, runs the engine over buffer() each loop(), frees the exec block in teardown. controlChangeTriggersBuildState gates on "source" so an edit recompiles live; a parse error surfaces via the MoonModule status field (Severity::Error) and the layer goes dark — robust, no reboot.
UI / Installer:
- config-ops.js (new): planConfigOps(entry) — pure, exported, the ordered APPLY_OP plan (clearChildren pre-pass for every re-populated container that isn't itself added fresh, then add, then set). install-orchestrator.js uses it; the macOS https-CDN imports kept it un-importable in node, so the pure planner lives in its own module.
- deviceModels.json: S3 + P4 testbench entries add Grid (parent Layouts) and Layer (parent Layers) explicitly and drop the now-obsolete replaceChildren on Layer — the entry is self-contained, so the no-erase path rebuilds Layouts/Layers from any prior state (the bug: drivers reappeared but layouts/layers didn't, because the entry assumed boot-default Grid/Layer).
Tests:
- unit_moonlive_fill: emit/allocExec/run path on the desktop host backend — fill + animated-from-t, zero-lights, recompile, free, allocExec round-trip.
- unit_moonlive_compiler: golden-bytes equivalence (parsed fill == hand-emitted bytes), whitespace tolerance, 11 parser-error diagnostics (no crash on malformed input), live source recompile.
- config-ops.test.mjs: every add-parent cleared unless added fresh, clears precede adds precede sets, dedup, malformed-input robustness, the S3 entry's add/clear set.
Docs / CI:
- MoonLiveEffect.md (new spec): the engine/compiler/emitter/binding pieces, the source control, the allocExec contract, the golden-bytes property, ESPLiveScript/ARTI-FX/MoonLight prior art.
- ImprovProvisioningModule.md: clearChildren pre-pass now covers every add-parent, not just replaceChildren containers — and why (the AlreadyExists-skip on a non-erased device).
- sdkconfig.defaults.esp32s3-n16r8: disable ESP_SYSTEM_MEMPROT + enable HEAP_HAS_EXEC_HEAP so MALLOC_CAP_EXEC exists (the standard ESP32-JIT config; safety for scripts is the staged bounds/watchdog, not the hardware W^X wall).
- Plan-20260626 - MoonLive Stage 0 (native codegen spike).md: the approved plan record.
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakeLists.txt`:
- Around line 58-62: The root core target is pulling compiled MoonLive sources
into mm_core and linking it to mm_platform, which violates the header-only core
boundary. Remove MoonLive.cpp and MoonLiveCompiler.cpp from mm_core and move
their platform-dependent implementation behind mm_platform (or another platform
target), keeping mm_core as an INTERFACE-only surface. Use the mm_core target
setup and the MoonLive symbols to locate the affected build wiring and preserve
the declared layering contract.
In `@docs/history/plans/Plan-20260626` - MoonLive Stage 0 (native codegen
spike).md:
- Around line 1-3: This plan is being resolved in the current PR, so the
history-plan filename should be updated to include a trailing outcome suffix.
Rename the plan document to follow the resolved convention used by
docs/history/plans entries, such as appending “(shipped)” or the appropriate
resolved outcome to the existing Plan-20260626 title, and keep the content
unchanged unless the outcome needs to reflect a different final status.
In `@docs/install/config-ops.js`:
- Around line 23-39: The clear-prepass in config-ops.js is using looser
validation than the add pass, so malformed modules can trigger a clearChildren
without a matching add. Update sendConfigOverSerial’s planning logic to use the
same validity check for both addedIds and clearParents, ideally by factoring a
shared helper for “valid addable module” and reusing it in both loops. Ensure
the helper gates modules before adding parent_id to clearParents and before
recording ids in addedIds.
In `@docs/moonmodules/light/moonlive/MoonLiveEffect.md`:
- Around line 3-5: Rewrite the MoonLiveEffect documentation to describe only the
shipped behavior and current integration points, removing roadmap or
absence-based phrasing such as “forward-looking analysis,” “grows rule by rule,”
“no IR yet,” and “clean library later.” Update the overview around MoonLive’s
current contract, the persisted script-source control, the lexer/parser/codegen
pipeline, and the existing native render-tick execution path, using the
document’s existing symbols and concepts rather than future-state language.
Preserve the current stage description only as a factual status line and keep
the focus on what the module does now across domains.
In `@esp32/sdkconfig.defaults.esp32s3-n16r8`:
- Around line 16-24: The board default sdkconfig currently turns off memory
protection and enables the executable heap globally, which should not apply to
every build. Remove the MoonLive/JIT-specific CONFIG_ESP_SYSTEM_MEMPROT_FEATURE
and CONFIG_HEAP_HAS_EXEC_HEAP settings from the default profile, and move them
into a dedicated opt-in overlay or explicit build profile tied to the
MoonLive/native codegen path so only that configuration gets executable memory.
In `@src/core/moonlive/moonlive_emit.h`:
- Around line 6-17: The comments in moonlive_emit.h are written in stage-history
and future-looking terms (“no IR yet”, “Stage 1b”, “Stage 2 replaces it”), so
rewrite the header comment block to describe only the current emitter contract
and backend behavior. Update the documentation near the MoonLive backend seam
and FillFn description so it states what exists now, using the symbols MoonLive,
FillFn, and the platform::allocExec/writeExec flow as anchors, and remove any
references to absent features or planned replacements.
In `@src/core/moonlive/MoonLive.cpp`:
- Around line 7-10: Rewrite the implementation comment near kCodeCap in
MoonLive.cpp to describe the current cap and emitter behavior only, without any
future-stage or “later” narration. Keep the note focused on the present
constraint that the fixed arena size is sufficient for Stage-1 fill loops and
that the emitter returns the actual length, using current-state wording rather
than mentioning what will come later.
- Line 3: `MoonLive.cpp` is violating the core layer boundary by including
`platform/platform.h` and invoking platform exec-memory APIs from `src/core`.
Refactor the placement/freeing logic behind a core-neutral abstraction that is
injected into `MoonLive` (or move the runtime placement layer out of `src/core`
entirely), and keep `MoonLive` using only platform-independent interfaces;
update the affected placement and cleanup code paths so no platform-specific
include or direct executable-memory call remains in `src/core`.
In `@src/core/moonlive/MoonLive.h`:
- Around line 7-17: Revise the MoonLive.h header comments to describe only the
current public engine contract, not future compiler stages or what the engine
does not include. Update the top-of-file narrative around MoonLive and its
compile/run flow, and the neutrality note tied to the
MoonLiveEngine/MoonLiveEffect boundary, so it states present behavior and
responsibilities only while keeping the API symbols clear for maintainers.
- Around line 45-48: Guard MoonLive::run against invalid output buffers before
invoking fn_ or anim_ by checking that buf is non-null when nLights is nonzero
and that cpl is at least 3, since the native routines write channels +0/+1/+2.
If the buffer shape is invalid or the grid is empty, return early without
calling the callback so the render path remains safe for all grid sizes.
- Around line 20-22: MoonLive currently owns a heap-backed exec block via free()
in ~MoonLive(), so the implicit copy constructor and copy assignment would
duplicate ownership and cause double-free bugs. Update the MoonLive class
definition to make it non-copyable by deleting the copy constructor and copy
assignment operator, alongside the existing destructor/free() ownership pattern.
In `@src/core/moonlive/MoonLiveCompiler.cpp`:
- Around line 8-11: The comments in MoonLiveCompiler should be rewritten to
describe the current lexer/parser/codegen behavior without stage-based or
absence-based narration. Update the affected comment blocks near the lexer,
parser, and codegen sections to state what the recursive-descent front-end,
token flow, AST handling, and emitted output do now, and remove phrases like
“Stage-2,” “No token table,” “a real tree arrives,” and “No IR yet.”
- Around line 116-127: Reject a null output buffer in compileSource before
calling emitFill: after parse succeeds, check out and return a diagnostic if it
is nullptr so code generation never writes through an invalid pointer. Update
the compileSource path in MoonLiveCompiler.cpp to handle the missing buffer case
alongside the existing no-source and parse-error checks, using the existing
symbols compileSource, parse, and emitFill.
In `@src/core/moonlive/MoonLiveCompiler.h`:
- Around line 6-20: The header comment in MoonLiveCompiler.h should be rewritten
into present-tense contract language for the existing fill(r, g, b); compiler
path, removing roadmap and absence narration such as “yet,” “from here,” and “is
introduced when.” Update the block near the MoonLive front-end description so it
states only what the compiler currently does: tokenize, parse, and emit native
code for the single fill statement via the backend-neutral emitFill flow. Keep
the emphasis on current behavior and API neutrality, and avoid any
future-looking or “no X yet” phrasing.
In `@src/light/moonlive/MoonLiveEffect.h`:
- Around line 54-55: The MoonLiveEffect::loop path passes channelsPerLight()
straight into engine_.run, but the native emitters always write channel offsets
0, 1, and 2, so short buffers can be overrun. Update MoonLiveEffect::loop (or
the call chain into engine_.run) to guard against layouts with fewer than 3
channels per light, and skip or clamp MoonLive rendering when the buffer cannot
safely accommodate RGB writes. Use the existing buffer(), nrOfLights(),
channelsPerLight(), and elapsed() call site to locate the fix.
- Around line 12-16: Update the comment in MoonLiveEffect.h to describe the
current behavior instead of the staged rollout: the `source` text control
already exists and is compiled by the `MoonLiveEffect` path, so remove the “no
script source yet” and “arrive in later stages” wording. Keep the comment
focused on what the effect currently does in the
emit/allocExec/call/write-buffer flow and align it with the actual `source`
control and compilation logic referenced elsewhere in the class.
In `@src/platform/desktop/moonlive_emit.cpp`:
- Around line 5-16: Rewrite the MoonLive desktop backend comments in
moonlive_emit.cpp to describe the current backend contract in present tense,
replacing future-looking or stage-based phrasing like “eventual home”,
“Stage-1a”, and “Stage 1b” with what the backend does now. Update the
descriptive block near the top and the other referenced comment locations so
they explain the desktop backend, the in-process execution path, and the fill
routine as current behavior, using symbols like MoonLive desktop backend, fill,
and the template patching notes to keep the context clear.
- Around line 87-141: Selecting the x86-64 path in emitFill and emitAnimatedFill
by __x86_64__ alone is too broad because the current kX64 and kX64Anim blobs
assume SysV argument registers and will mis-handle Windows x64. Update the ABI
dispatch around these functions to either add a Windows x64-specific template
for the rcx/rdx/r8/r9 calling convention or explicitly exclude _WIN32 from this
branch until that template exists, keeping the existing SysV blob only for the
SysV x86-64 ABI.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 27-30: Add a platform-specific Windows path in writeExec so _WIN32
does not fall through to __builtin___clear_cache, which MSVC lacks. Update the
executable-memory handling in platform_desktop.cpp alongside the existing
sys/mman.h and pthread/JIT branches, using FlushInstructionCache for Windows
while preserving the current cache flush behavior on non-Windows platforms.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 115-153: Round executable allocations up to a full word so
writeExec() never reads or writes past the buffer when len is not 4-byte
aligned. Update allocExec/freeExec behavior in platform_esp32.cpp so callers
using allocExec(len) still get enough padded space for the d[words] remainder
path and the esp_cache_msync length, and keep the fix aligned with the public
contract in platform.h. Add a regression test for a non-4-byte executable length
to cover the allocExec/writeExec path and verify no overrun occurs.
In `@test/js/config-ops.test.mjs`:
- Around line 54-59: The ordering assertion in the config ops test incorrectly
fails for plans that contain only clearChildren operations. Update the check
around the lastIndex/firstIndex logic so the assertion only runs when there is
at least one non-clearChildren op, while still validating that any clearChildren
in mixed plans occur before add/set operations. Use the existing helpers
lastIndex and firstIndex in config-ops.test.mjs to locate the condition and
guard it appropriately.
In `@test/unit/core/unit_moonlive_fill.cpp`:
- Around line 31-45: The current MoonLive test only covers 3-channel buffers, so
add a reproducer for 1- and 2-channel layers and verify safe behavior in the
runtime path. Update the MoonLiveEffect-to-engine call path so
`MoonLiveEffect::run`/`engine_.run(...)` does not blindly write RGB bytes into
buffers with `channelsPerLight() < 3`; instead guard or clamp writes so sub-RGB
buffers are left in a valid state without out-of-bounds access. Use the existing
`MoonLiveEffect` and `MoonLive::run` entry points to locate the fix, and back it
with a unit test in `unit_moonlive_fill.cpp` that proves the buffer remains
in-bounds for cpl 1/2.
🪄 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: 1a7d7d72-3177-441f-bd5f-b5e4d1256eaf
📒 Files selected for processing (26)
CMakeLists.txtdocs/history/plans/Plan-20260626 - MoonLive Stage 0 (native codegen spike).mddocs/install/config-ops.jsdocs/install/deviceModels.jsondocs/install/install-orchestrator.jsdocs/moonmodules/core/ImprovProvisioningModule.mddocs/moonmodules/light/moonlive/MoonLiveEffect.mdesp32/main/CMakeLists.txtesp32/sdkconfig.defaults.esp32s3-n16r8src/core/moonlive/MoonLive.cppsrc/core/moonlive/MoonLive.hsrc/core/moonlive/MoonLiveCompiler.cppsrc/core/moonlive/MoonLiveCompiler.hsrc/core/moonlive/moonlive_emit.hsrc/light/moonlive/MoonLiveEffect.hsrc/main.cppsrc/platform/desktop/moonlive_emit.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/moonlive_emit.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/CMakeLists.txttest/js/config-ops.test.mjstest/scenario_runner.cpptest/unit/core/unit_moonlive_compiler.cpptest/unit/core/unit_moonlive_fill.cpp
Comment on lines
+58
to
62
| src/core/moonlive/MoonLive.cpp | ||
| src/core/moonlive/MoonLiveCompiler.cpp | ||
| ) | ||
| target_include_directories(mm_core PUBLIC src/) | ||
| target_link_libraries(mm_core PUBLIC mm_platform) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep compiled MoonLive core out of mm_core.
Adding MoonLive.cpp/MoonLiveCompiler.cpp to the root core target and linking mm_core to mm_platform breaks the declared build/layering contract. Move the compiled/platform-dependent lifecycle behind the platform target or refactor this surface to preserve the header-only core boundary.
As per path instructions, root CMakeLists.txt says “Core library is INTERFACE (header-only). Only platform .cpp files need compilation.”
🤖 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 `@CMakeLists.txt` around lines 58 - 62, The root core target is pulling
compiled MoonLive sources into mm_core and linking it to mm_platform, which
violates the header-only core boundary. Remove MoonLive.cpp and
MoonLiveCompiler.cpp from mm_core and move their platform-dependent
implementation behind mm_platform (or another platform target), keeping mm_core
as an INTERFACE-only surface. Use the mm_core target setup and the MoonLive
symbols to locate the affected build wiring and preserve the declared layering
contract.
Source: Path instructions
Comment on lines
+1
to
+3
| # Plan — MoonLive Stage 0: native-codegen load-bearing spike | ||
|
|
||
| > Approved plan record (CLAUDE.md *Plan before implementing*). Implements the first, smallest step of [livescripts-analysis-top-down.md](../../backlog/livescripts-analysis-top-down.md) — its Stage 0 "load-bearing spike", split one notch finer so the single novel hardware risk is isolated and proven before any compiler front-end is written. S3-only, bare-minimum assembler, near-zero language. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark this resolved plan with an outcome suffix.
This plan is landing in the current PR, so the history-plan convention wants the filename to end with a resolved outcome such as (shipped). As per path instructions, docs/history/plans/*.md: "Approved feature plans must be saved in docs/history/plans/ with the Plan-YYYYMMDD - <title>.md naming pattern, then marked with a trailing outcome such as (shipped) or (attempted, abandoned) when resolved."
🤖 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-20260626` - MoonLive Stage 0 (native codegen
spike).md around lines 1 - 3, This plan is being resolved in the current PR, so
the history-plan filename should be updated to include a trailing outcome
suffix. Rename the plan document to follow the resolved convention used by
docs/history/plans entries, such as appending “(shipped)” or the appropriate
resolved outcome to the existing Plan-20260626 title, and keep the content
unchanged unless the outcome needs to reflect a different final status.
Source: Path instructions
Comment on lines
+16
to
+24
| # MoonLive native codegen needs an executable heap (allocExec → MALLOC_CAP_EXEC IRAM). | ||
| # MALLOC_CAP_EXEC is gated behind CONFIG_HEAP_HAS_EXEC_HEAP, which IDF disables whenever | ||
| # memory protection (PMP/PMS W^X) is on — the same write-XOR-execute rule we handle on | ||
| # macOS. A JIT fundamentally needs writable-then-executable memory, so disable memprot | ||
| # (which enables the exec heap). This is the standard ESP32-JIT configuration (ESPLiveScript | ||
| # runs the same way); the safety story for scripted code is the staged bounds/watchdog | ||
| # checks, not the hardware W^X wall. | ||
| CONFIG_ESP_SYSTEM_MEMPROT_FEATURE=n | ||
| CONFIG_HEAP_HAS_EXEC_HEAP=y |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Don’t disable W^X in the board default profile.
Lines 23-24 disable ESP32-S3 memory protection and enable executable heap for every build using this default, even when no MoonLive effect is installed. Keep this in a dedicated MoonLive/JIT opt-in sdkconfig overlay or gate MoonLive behind an explicit build profile.
🤖 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 `@esp32/sdkconfig.defaults.esp32s3-n16r8` around lines 16 - 24, The board
default sdkconfig currently turns off memory protection and enables the
executable heap globally, which should not apply to every build. Remove the
MoonLive/JIT-specific CONFIG_ESP_SYSTEM_MEMPROT_FEATURE and
CONFIG_HEAP_HAS_EXEC_HEAP settings from the default profile, and move them into
a dedicated opt-in overlay or explicit build profile tied to the MoonLive/native
codegen path so only that configuration gets executable memory.
Extends the MoonLive live-script engine to a third ISA and hardens it. The scripted fill(r,g,b) effect now compiles to native code on the ESP32-P4 (RISC-V) as well as Xtensa (classic/S3) and the desktop host — adding the P4 touched only the per-ISA emitter and its sdkconfig, proving the engine/front-end/binding seam decouples. Also processes the first CodeRabbit review: buffer-shape guards so a sub-RGB or empty layout can't overrun, a non-copyable engine, word-padded exec allocation, a shared installer add/clear validity check, Windows-ABI guards, and present-tense docs. A new live scenario exercises the whole wired-module lifecycle and runs in-process plus live on PC/S3/P4. KPI: 16384lights | PC:403KB | tick:132/91/120/9/1/625/36/15/18/59/125/13/35us(FPS:7575/10989/8333/111111/1000000/1600/27777/66666/55555/16949/8000/76923/28571) | ESP32:1234KB | src:106(21215) | test:75(11688) | lizard:79w (MoonLive live this session via the new scenario: S3 ~250 FPS, P4 ~90 FPS, heap steady across broken-script + remove/re-add; the KPI script's hardcoded serial port missed the live ESP32 tick capture, values from the device API.) Core: - platform: allocExec rounds the request up to a 4-byte word — writeExec's final partial-word store and the esp_cache_msync length both round up, so the block must hold the whole word (🐇). writeExec now does a two-step cache flush on ESP32 — data-cache writeback (C2M) then I-cache invalidate — because the P4's exec region is cache-backed and a single TYPE_INST sync left freshly-written code in the data cache (illegal-instruction crash); correct on Xtensa and RISC-V. - MoonLive: run() guards a null buffer / zero lights / cpl<3 so the RGB-writing routines can't overrun a sub-RGB or empty layout (the every-grid-size hard rule) (🐇). Engine made non-copyable (owns a heap exec block) (🐇). compileSource rejects a null/empty output buffer (🐇). Light domain: - MoonLiveEffect: unchanged behaviour; comments rewritten present-tense (🐇). UI / Installer: - config-ops.js: factored a shared isAddable() helper used by both the clear pre-pass and the add pass, so a malformed module can't trigger a clearChildren without a matching add (🐇). Tests: - unit_moonlive_fill: cpl 1/2 + null-buffer no-op test (pins the overrun guard) (🐇). - scenario_MoonLiveEffect_livescript (new): the scripted effect as a wired MoonModule — add, live source recompile, broken-script degrade (no crash, status diagnostic), recover, grid 1x1 + back, remove, re-add. Runs in-process each commit and live over REST on PC/S3/P4. - config-ops.test.mjs: guarded the ordering assertion against clears-only plans (🐇). Platform / Build: - esp32/moonlive_emit.cpp: added the RISC-V (P4) backend — a #elif defined(__riscv) branch with the fill + animated routines, copied verbatim from the assembler. Adding the ISA changed nothing in the engine, front-end, or binding (the Stage 0.5 seam-proof). - desktop/moonlive_emit.cpp: the x86-64 SysV blob is gated && !defined(_WIN32) (the Windows x64 ABI differs); #error covers Win64 until a template exists (🐇). - desktop/platform_desktop.cpp: writeExec uses FlushInstructionCache on _WIN32 (MSVC lacks __builtin___clear_cache) (🐇). - sdkconfig.defaults.esp32p4-eth: enable the executable heap (disable memprot) for the P4's JIT, same as the S3 — scoped to the testbench board, not global. Docs: - MoonLiveEffect.md: present-tense rewrite; documents the source control, the per-ISA backends incl P4, and the new live scenario (🐇). - Plan-20260626 … (native codegen spike): marked (shipped). - 🐇 mm_core "header-only/INTERFACE" boundary, core-includes-platform, sdkconfig "global" overlay: accepted as-is — coding-standards.md makes core .h+.cpp that calls the platform seam, and the exec-heap config is already per-board, not global; the suggested changes would diverge from the documented conventions.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/scenarios/light/scenario_perf_light.json (1)
546-558: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSync
measure-heavy-64withdocs/tests/scenario-tests.md.
docs/tests/scenario-tests.mdstill documentsmeasure-heavy-64forpc-macosas2,924-16,129FPS observed through2026-06-21. Updating this fixture to50µs and2026-06-26moves it outside that documented range, so the published scenario contract is now stale unless the docs are updated in the same PR.🤖 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/scenarios/light/scenario_perf_light.json` around lines 546 - 558, The `measure-heavy-64` fixture in `scenario_perf_light.json` has been updated past the currently documented `pc-macos` contract in `docs/tests/scenario-tests.md`, so align both together. Either keep the fixture values consistent with the documented `2,924-16,129` / `2026-06-21` expectations or update the docs to match the new `50` µs and `2026-06-26` data, and make sure the `measure-heavy-64` scenario entry stays synchronized across the fixture and documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules/light/moonlive/MoonLiveEffect.md`:
- Line 32: Update the MoonLiveEffect documentation wording so it matches the
actual broken-script behavior: when MoonLive::compile(source) fails, the
previous compiled code is freed, MoonLiveEffect reports the parse error, and the
effect renders dark instead of continuing to run old code. Adjust the
description in MoonLiveEffect.md to reference MoonLive::compile and
MoonLiveEffect explicitly, and remove the claim that prior code keeps running.
In `@src/core/moonlive/moonlive_emit.h`:
- Around line 27-31: The emitter entry points currently assume a valid output
buffer and can crash on a null `out` pointer instead of failing cleanly. Update
the backend implementations of `emitFill` and `emitAnimatedFill` to treat `out
== nullptr` as a failure case and return 0 before any `memcpy` or byte emission,
matching the existing graceful behavior in `MoonLiveCompiler::compileSource`.
Add a regression test covering direct calls with a null output buffer so this
contract stays enforced.
In `@src/platform/esp32/moonlive_emit.cpp`:
- Around line 14-17: The file header comment in the ESP32 moonlive emit
implementation is written as historical/forward-looking narration instead of
current-state contract text. Rewrite the top block in moonlive_emit.cpp, near
the ISA compile-time split, to describe only the present Xtensa/RISC-V backend
selection and runtime call path, and remove references like “Adding the P4...”
and “Stage 0.5”. Keep the wording focused on what moonlive_emit and the
compile-time __XTENSA__/__riscv branching currently do.
In `@test/scenarios/light/scenario_MoonLiveEffect_livescript.json`:
- Around line 91-94: The benchmark metadata in the MoonLiveEffect scenario
contains future-dated observed.at entries, so update the affected observed
blocks to use the actual measurement date or remove the future endpoint until
those runs exist. Check the repeated observed.at arrays throughout
scenario_MoonLiveEffect_livescript.json and replace the 2026-06-27 values
consistently.
- Line 13: The scenario description is out of sync with the actual broken-script
behavior in MoonLiveEffect. Update the overview text so it matches the engine
flow and the BROKEN-script step: on a failed recompile, the previous routine is
freed and the effect renders dark instead of continuing to run the old code.
Keep the wording aligned with the live end-to-end scenario intent and the
existing MoonLiveEffect/BROKEN script behavior described elsewhere in the
scenario.
---
Outside diff comments:
In `@test/scenarios/light/scenario_perf_light.json`:
- Around line 546-558: The `measure-heavy-64` fixture in
`scenario_perf_light.json` has been updated past the currently documented
`pc-macos` contract in `docs/tests/scenario-tests.md`, so align both together.
Either keep the fixture values consistent with the documented `2,924-16,129` /
`2026-06-21` expectations or update the docs to match the new `50` µs and
`2026-06-26` data, and make sure the `measure-heavy-64` scenario entry stays
synchronized across the fixture and documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c44e1055-07ff-4486-b714-b34c8d1f8423
📒 Files selected for processing (19)
docs/history/plans/Plan-20260626 - MoonLive Stage 0 (native codegen spike) (shipped).mddocs/install/config-ops.jsdocs/moonmodules/light/moonlive/MoonLiveEffect.mdesp32/sdkconfig.defaults.esp32p4-ethsrc/core/moonlive/MoonLive.cppsrc/core/moonlive/MoonLive.hsrc/core/moonlive/MoonLiveCompiler.cppsrc/core/moonlive/MoonLiveCompiler.hsrc/core/moonlive/moonlive_emit.hsrc/light/moonlive/MoonLiveEffect.hsrc/platform/desktop/moonlive_emit.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/moonlive_emit.cppsrc/platform/esp32/platform_esp32.cpptest/js/config-ops.test.mjstest/scenarios/light/scenario_MoonLiveEffect_livescript.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/core/unit_moonlive_fill.cpp
💤 Files with no reviewable changes (1)
- docs/history/plans/Plan-20260626 - MoonLive Stage 0 (native codegen spike) (shipped).md
Comment on lines
+91
to
+94
| "at": [ | ||
| "2026-06-26", | ||
| "2026-06-27" | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace future-dated benchmark metadata.
These observed.at entries include 2026-06-27, but today is June 26, 2026. Use the actual run date or remove the future endpoint until those measurements exist.
Also applies to: 109-112, 127-130, 156-159, 174-177, 192-195, 221-224, 239-242, 257-260, 286-289, 304-307, 322-325, 351-354, 369-372, 387-390, 416-419, 435-437, 453-455, 480-482, 498-500, 516-518, 545-547, 563-565, 581-583
🤖 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/scenarios/light/scenario_MoonLiveEffect_livescript.json` around lines 91
- 94, The benchmark metadata in the MoonLiveEffect scenario contains
future-dated observed.at entries, so update the affected observed blocks to use
the actual measurement date or remove the future endpoint until those runs
exist. Check the repeated observed.at arrays throughout
scenario_MoonLiveEffect_livescript.json and replace the 2026-06-27 values
consistently.
MoonLive scripts now take expression arguments — any argument can be a nested call like random16(256), so setRGB(random16(256), random16(256), random16(256), random16(256)) works. The functions (setRGB, fill, random16) are no longer baked into the compiler; the light domain registers them in a host builtin table, keeping the engine core LED-free. Adds a third device backend (RISC-V / ESP32-P4) and a register-reuse pass so multi-call statements fit the small device register file. Verified live on PC, S3 (Xtensa), and P4 (RISC-V). KPI: 16384lights | tick:143/99/132/13/2/364/40/17/25/67/133/12/38us(FPS:6993/10101/7575/76923/500000/2747/25000/58823/40000/14925/7518/83333/26315) | tick:6957us(FPS:143) | heap:8334KB | src:118(22357) | test:76(11833) | lizard:82w Core: - MoonLive: compile(source, table) resolves call names against an injected BuiltinTable; the core owns the grammar plus a generic Call/Inline mechanism and no LED vocabulary. Code arena sized once for the heaviest realistic single statement (four-arg all-calls on the bulkiest ISA) rather than grown per script. - MoonLiveCompiler: recursive-descent expression parser (expr := number | call), every argument lowered to a vreg; a free-stack vreg allocator recycles each dead argument temp the moment its call consumes it, so a chain of calls peaks at a handful of registers instead of growing without bound — what makes the multi-call statement fit the 12-register device file. - MoonLiveIr: a neutral typed IR (Const/Add/AddImm/Mul/Loop/Bounds/Call/Inline) replacing the RGB-specific Store op; Call carries a host fn pointer, Inline a neutral opcode tag (WriteRGB/FillRGB) the backend stores inline (no per-pixel call on the hot path). - MoonLiveBuiltins: the neutral host-binding seam (BuiltinTable of name -> Call | Inline descriptor). Light domain: - MoonLiveBuiltins_light: the only home for the LED vocabulary — registers setRGB/fill (Inline) and random16 (Call); a different host writes its own table and the core is unchanged. - MoonLiveEffect: passes the light builtin table to compile(); source text buffer raised to 128 chars so a four-call statement is not truncated. Core / platform: - RISC-V assembler + lowering (ESP32-P4): replaces the previous stub with a real RV32 backend (named instructions, label back-patching, standard call ABI with caller-saved register preservation); every encoding verified against riscv32-esp-elf disassembly before flashing. - Xtensa and host (desktop) assemblers + lowerings carried onto the neutral IR; the stub lowerToBytes removed. Tests: - unit_moonlive_compiler: expression grammar (random16 in any/every slot, uint16 range), the multi-call vreg-reuse budget case, and the domain-neutral property (empty table -> core knows no functions; a host can register an arbitrary name). - unit_moonlive_ir: behavioral golden — compiled fill vs the hand-encoded reference render an identical buffer — plus setRGB single-pixel write and the runtime bounds guard. Docs: - MoonLiveEffect.md: rewritten to the builtin-table model (expression grammar, Call/Inline descriptors, prior art ESPLiveScript/ARTI). - decisions.md: the build-around-expressions-not-statement-shapes lesson, and the RISC-V bring-up + vreg-reuse lesson (a third ISA proves the IR seam is real; the smallest register file is where host-vs-device diverges). - two approved plans saved to docs/history/plans/ (the host-bound-functions plan marked shipped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/moonlive/MoonLiveBuiltins.h`:
- Around line 25-32: The core opcode names in InlineOp are too light-specific;
rename WriteRGB and FillRGB in MoonLiveBuiltins to domain-neutral storage
primitives, then update any consumers so src/light maps setRGB/fill onto those
neutral tags. Keep the semantics the same in the enum and its operand comments,
but remove host-vocabulary coupling from src/core by using generic names and
adjusting the light-layer translation points accordingly.
- Around line 52-63: The builtin registry in MoonLiveBuiltins::add and
MoonLiveBuiltins::find allows a builtin with a null name to be stored, and
find() then dereferences items[i].name unconditionally. Update add() to reject
or skip entries with a null b.name, or add a defensive null check in find()
before reading the name field, so lookups on the Builtin array cannot
null-deref.
- Around line 36-42: Use a typed function-pointer alias for host callables
instead of storing them as const void*; update Builtin::fn and the matching
IrInst::callFn field to use the same alias so the builtin table and IR remain
type-safe and portable across desktop and ESP32. Locate the change in Builtin
and the IR instruction definitions, then update any construction, assignment,
and call sites that read or write these fields to use the new typed
function-pointer representation consistently.
In `@src/core/moonlive/MoonLiveCompiler.cpp`:
- Around line 9-10: The lexer section header in MoonLiveCompiler should describe
the present contract instead of change history; update the comment above Tok in
the lexer area to use current-state wording like a plain “Lexer” section header,
and remove the “unchanged shape” phrasing so the comment reflects what exists
now. Keep the Tok enum itself as-is and only revise the surrounding comment in
the lexer block.
- Around line 133-150: The MoonLiveCompiler::compile call handling only forwards
args[0] into IrOp::Call, so multi-argument BuiltinKind::Call invocations lose
the rest of their operands. Update the BuiltinKind::Call path to either reject
fn->argc > 1 up front in this block or extend the IR/backend contract so all
arguments are preserved through ir.push and consumed by the call emission logic;
make sure both the result and statement-call branches in
MoonLiveCompiler::compile are covered consistently.
- Around line 77-80: The alloc() path in MoonLiveCompiler currently aliases the
last virtual register when nextTemp reaches kMaxVRegs, which can hide register
exhaustion and produce incorrect IR. Update alloc() to detect exhaustion instead
of returning VReg(kMaxVRegs - 1), and propagate a compile error from the
MoonLiveCompiler temp-register allocation flow so callers fail cleanly when too
many temps are needed. Make sure the fix uses the existing alloc() and
nextTemp/freeCount bookkeeping rather than introducing silent reuse.
In `@src/core/moonlive/MoonLiveIr.h`:
- Around line 61-65: `IrProgram::push()` currently enforces only the opcode
count and can still admit instructions that reference vregs beyond `kMaxVRegs`.
Update this method to validate the instruction’s destination and any source
virtual registers against the fixed register budget before storing it, and
return false when any operand exceeds the limit so invalid programs are rejected
early. Use the existing `kMaxIrOps`, `kMaxVRegs`, `IrInst`, and `vregsUsed`
logic in `MoonLiveIr.h` to keep the check consistent with the current IR
bookkeeping.
In `@src/platform/esp32/moonlive_asm_riscv.cpp`:
- Line 63: Handle wide constants in the RISC-V constant path by updating
RiscvAssembler::movImm and its callers so IrOp::Const values above the 12-bit
addi range are materialized correctly instead of being emitted directly as a
single addi. Add a proper multi-instruction load-immediate sequence for larger
constants, and make the Const lowering in the RISC-V backend choose that path
for values like 65535 rather than relying on movImm alone.
In `@src/platform/esp32/moonlive_asm_xtensa.cpp`:
- Around line 46-49: The Xtensa backend’s XtensaAssembler::movImm only emits the
low 8 bits of the immediate, so constants coming from IrOp::Const in
MoonLiveCompiler can wrap when they exceed 255. Update the lowering path around
XtensaAssembler::movImm to either emit a wider constant-loading sequence for
larger values or add an explicit range check and fallback before calling movImm.
Keep the fix localized to the Const-to-assembly path so values up to 65535 are
handled correctly without truncation.
In `@test/unit/core/unit_moonlive_compiler.cpp`:
- Around line 75-86: The malformed-input coverage in compileSource should
explicitly assert that each bad program fails, because the current loop only
checks for a non-empty error message after a failure and would miss accidental
successful compilation. In unit_moonlive_compiler.cpp, split the valid and
invalid moonlive::compileSource cases in compileSource rejects malformed
programs with a diagnostic, never crashes, keep the existing success assertions
for valid inputs elsewhere, and for each entry in the bad set use
CHECK_FALSE(r.ok) before validating r.error so the parser contract is actually
enforced.
🪄 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: 72d263a3-8599-4893-938c-640a5df55d21
📒 Files selected for processing (39)
CMakeLists.txtdocs/history/decisions.mddocs/history/plans/Plan-20260627 - MoonLive Stage 3 (IR seam + assembler, second statement).mddocs/history/plans/Plan-20260627 - MoonLive expressions + host-bound functions (domain-neutral core) (shipped).mddocs/moonmodules/light/moonlive/MoonLiveEffect.mddocs/tests/scenario-tests.mdesp32/main/CMakeLists.txtsrc/core/moonlive/MoonLive.cppsrc/core/moonlive/MoonLive.hsrc/core/moonlive/MoonLiveBuiltins.hsrc/core/moonlive/MoonLiveCompiler.cppsrc/core/moonlive/MoonLiveCompiler.hsrc/core/moonlive/MoonLiveIr.hsrc/core/moonlive/moonlive_emit.hsrc/light/moonlive/MoonLiveBuiltins_light.hsrc/light/moonlive/MoonLiveEffect.hsrc/platform/desktop/moonlive_asm_host.cppsrc/platform/desktop/moonlive_asm_host.hsrc/platform/desktop/moonlive_emit.cppsrc/platform/desktop/moonlive_lower_host.cppsrc/platform/esp32/moonlive_asm_riscv.cppsrc/platform/esp32/moonlive_asm_riscv.hsrc/platform/esp32/moonlive_asm_xtensa.cppsrc/platform/esp32/moonlive_asm_xtensa.hsrc/platform/esp32/moonlive_emit.cppsrc/platform/esp32/moonlive_lower_riscv.cppsrc/platform/esp32/moonlive_lower_xtensa.cpptest/CMakeLists.txttest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MoonLiveEffect_livescript.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_moonlive_compiler.cpptest/unit/core/unit_moonlive_fill.cpptest/unit/core/unit_moonlive_ir.cpp
Addresses a CodeRabbit review of the MoonLive engine (two real device-codegen bugs plus hardening), adds a reusable multi-line textarea control used by the MoonLive script source, gives the status bar and side nav a floating rounded-panel look, and refreshes the README screenshots with a light/dark theme GIF. Device animation correctness verified by disassembling the compiled Xtensa output with the real toolchain (channel order and the full-uint16 constant path both confirmed). KPI: 16384lights | tick:126/89/118/9/1/326/36/15/18/55/118/11/34us(FPS:7936/11235/8474/111111/1000000/3067/27777/66666/55555/18181/8474/90909/29411) | ESP32:916KB | tick:12724us(FPS:78) | heap:8327KB | src:118(22415) | test:76(11842) | lizard:82w Core: - Control: new TextArea control type (addTextArea) — identical char-buffer storage, parse, and persist path to Text, differing only in the UI type string so the front-end renders a resizable multi-line textarea. - MoonLiveCompiler: alloc() now fails the compile on register exhaustion instead of silently aliasing the last vreg (which produced wrong IR); all IR appends route through an emit() helper that fails on a full program; a multi-argument Call-kind builtin is rejected up front rather than dropping args[1..]; lexer section comment made present-tense. - MoonLiveIr: IrProgram::push validates every operand vreg against the register budget and rejects out-of-range programs at the seam; IrInst::callFn uses the new typed HostCallFn alias. - MoonLiveBuiltins: typed HostCallFn function-pointer alias replaces the bare const void*; add() rejects a null builtin name (would null-deref in find()); the neutral inline opcodes renamed WriteRGB/FillRGB to the domain-neutral StoreElem/FillElems. Light domain: - MoonLiveBuiltins_light: registers random16 with the typed pointer (no reinterpret_cast); StoreElem/FillElems tag names. - MoonLiveEffect: source control uses addTextArea; buffer is 128 chars so a four-call statement is not truncated. Core / platform: - Xtensa assembler: movImm now materialises the full uint16 (movi hi8; slli 8; movi lo8; add) instead of emitting only the low byte — a Const above 255 (e.g. random16(65535)) no longer truncates to 255. - RISC-V assembler: movImm materialises wide constants with lui+addi (the hi/lo split) instead of a single sign-extended addi that capped at 2047. - RISC-V lowering: calls the host's typed function pointer directly like the other backends; removes a duplicate per-TU random16 LCG copy. - Xtensa/host lowering: cast the typed callFn to the assembler's address argument at the call site. UI: - app.js: render + live-sync + reset-equality for the textarea control type; EDITABLE_CONTROL_TYPES includes textarea. - style.css: textarea control (monospace, vertical resize, compact one-row default, solid corner grip); status bar and side nav float as rounded panels inset from the window edges (matching the module cards), with the mobile nav drawer kept flush-left. Docs / CI: - MoonLiveEffect.md: source is a multi-line textarea control. - README: hero is the animated light/dark theme GIF. - screenshots: ui_light.png (light theme, 131KB) and ui_overview.png (dark theme, 104KB) compressed from ~600KB; ui_theme.gif (269KB) alternates the two themes with per-frame palettes for accurate dark colours. Reviews: - 🐇 Xtensa/RISC-V movImm truncation: fixed (real bugs — verified 65535 -> 0xffff by toolchain disassembly). - 🐇 alloc() register-exhaustion aliasing: fixed (fails cleanly). - 🐇 IrProgram::push vreg validation: added. - 🐇 BuiltinTable::add null-name null-deref: guarded. - 🐇 typed function-pointer alias for fn/callFn: applied (also removed a duplicate RISC-V random16). - 🐇 InlineOp WriteRGB/FillRGB domain-specific names: renamed to neutral StoreElem/FillElems. - 🐇 multi-argument Call dropping args: rejected up front. - 🐇 lexer "unchanged shape" comment: made present-tense. - 🐇 malformed-input test only checked on failure: each invalid case now asserts CHECK_FALSE(r.ok). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Multiple modifiers per Layer now compose as a chain —
Region → Multiply → Rotate, each a coordinate fold applied in child order. The Layer's mapping build is inverted from virtual→physical fan-out to physical→logical fold (MoonLight's proven model, written fresh against ourMappingLUT), which makes chaining fall out for free, deletes the fan-out-overflow risk entirely, and leaves the render hot path untouched.RegionModifieralso gains off-screen windows so an effect can slide partly or fully out of view without rescaling. The white-theme preview-background fix is folded in.Hardware-verified live on the ESP32-S3 (3-deep chains compose, fold correctly, survive arbitrary add/remove/disable, off-screen windows slide without rescale).
Light domain
modifyLogicalSize/bool modifyLogical/modifyLive+hasModifyLive+consumeNeedsRebuild), replacing the single-modifierlogicalDimensions/mapToPhysical/maxMultiplier.rebuildLUTinverted to a physical→logical counting-sort CSR build folding the whole enabled-modifier chain. Each modifier folds in its own stage's box (stashed MoonLight-style, no per-stage array). A per-frame live pass for dynamic modifiers is gated onhasLive_— a static-only chain pays nothing (pay-for-what-you-use). Dynamic rebuilds coalesce to one per frame. DeletedbuildBoxToDriver/buildSparseIdentityLUT/ the fan-out ceiling.modifyLiveper-frame remap via an explicit integer 2×2 rotation matrix (the codebase's transform-matrix reference); no more step-gated rebuild.Coord3Dwith per-axis operators.UI
preview3d.js/app.js— read--bg-0from<body>(where the theme override lives) instead of<html>, so the preview canvas matches the light theme; redraw on theme toggle.Tests
unit_Coord3D;unit_Layer_modifier_chain(compose, A∘B ≠ B∘A, disabled-skip, per-stage regression);unit_Layer_live_modifier(live remap, pay-for-what-you-use gate, disabled gate, two-dynamic coalescing); off-screen Region cases; all 5 modifier tests rewritten to the fold interface.scenario_runner: register Region/Rotate/RandomMap — the chain scenario was silently skipping them;scenario_modifier_chainnow exercises Region + Multiply + Checkerboard + a live Rotate end-to-end.esp32+esp32s3-n16r8firmware build clean.Docs
ModifierBase.md(fold contract + prior art).architecture.mdupdated to the chain build (and dropped the stale "multi-layer composition" 🚧 example — it shipped). Modifier specs to the fold model;RegionModifier.mdoff-screen windows.decisions.mdinversion lesson. Backlog: deleted the shipped composed-modifiers item, updated the chain-viz premise. Plan saved todocs/history/plans/.KPI
S3 live:
tick:3482us(FPS:287). Classic ESP32 tick is from a stale monitor log — no classic board was attached this session (only the S3 was connected).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation