MoonLive on power functions (and: Layers → Effects) - #63
Open
ewowi wants to merge 3 commits into
Open
Conversation
The three top-level light containers are now Layouts, Effects and Drivers — L.E.D. The old name sat one character from its own child (Layers holding Layers) and read as a near-twin of Layouts, which is the pair a newcomer actually has to tell apart. The tree keeps its shape: Effects -> Layers -> effects and modifiers. desktop 140us/7,142fps | esp32 4,164us/240fps | flash unchanged Light domain - Layers -> Effects: the class, its header, the registered type name, the card image, and Drivers::setLayers -> setEffects. - Layer, the child, is untouched. The two names differ by one character, so every replacement is word-boundary anchored rather than a token sweep — the distinction this rename draws is exactly what a blind replace would collapse. - src/light/layers/ keeps its name: it also holds Layer.h, Buffer.h, BlendMap.h and MappingLUT.h, which are all still layer things. Core - ControlModule's capture-role index is kEffectsRole, and its static_assert re-pinned to spell "Effects". It checked the old name character by character ([0]=='L', [1]=='a', [5]=='s'), so it would have failed the build. Tests - unit_Layers_container -> unit_Effects_container, scenario_Layers_composition -> scenario_Effects_composition, and the scenario key that binds Drivers to the container is "effects" on both sides. - No golden moved, which is the evidence this is a rename and nothing else. Docs/CI - MIGRATING.md carries the break. The type name IS the persisted filename and the preset capture key, so a device boots with an empty light tree and presets stop applying their effects. Per ADR-0013 that is documented, not migrated. Both halves were verified on a running desktop: the tree does come up empty, and renaming /.config/Layers.json to Effects.json restores it intact — so the shortcut in that entry is tested rather than assumed. - docs/history/ is left alone: it records what was true when written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request renames the light-pipeline container from ChangesEffects container rename
MoonLive scripted modifier
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Layer
participant MoonLiveModifier
participant MoonLiveCompiler
participant LightBuiltins
Layer->>MoonLiveModifier: prepare script and controls
MoonLiveModifier->>MoonLiveCompiler: compile expression
MoonLiveCompiler->>LightBuiltins: resolve setXYZ and print
Layer->>MoonLiveModifier: transform coordinate
MoonLiveModifier->>MoonLiveModifier: execute script and return output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture.md`:
- Line 33: Update both table-of-contents links for the “Effects and Layer”
section to use the correct Markdown anchor `#effects-and-layer` instead of
`#layers-and-layer`, including the reference near line 475.
In `@docs/backlog/system-modules.md`:
- Line 65: Rewrite the sentence around the “Services is to System” comparison so
it describes Services as the existing top-level container for user-added Audio
and IR modules, analogous to Effects containing user-added Layer children.
Remove the implication that Effects contains effect children directly or that
MQTT is a user-added Service; preserve MQTT as code-wired under Network.
In `@docs/gettingstarted.md`:
- Around line 272-279: Propagate the Layers-to-Effects terminology across all
specified documentation: in docs/gettingstarted.md lines 257-261 and 290-299 and
README.md line 23, use the pipeline name “Layouts → Effects → Drivers”; in
docs/moonmodules/core/control.md lines 51 and 53, change preset-role references
from “layer” to “Effects” or “effects”; in docs/moonmodules/light/supporting.md
lines 21-27, add and use the effects anchor, update the test link accordingly,
and retain layers only as an intentional compatibility alias.
In `@docs/moonmodules/core/services.md`:
- Line 7: Update the Services heading in the core services documentation from a
third-level heading to ## Services so it is correctly nested beneath # Core
services and provides the proper parent level for Audio and IR.
In `@docs/usecases/build-your-own-moonmodules.md`:
- Line 322: Update docs/usecases/build-your-own-moonmodules.md at line 322 to
describe the hierarchy as Layouts → Effects → Layer → Effect/Modifier → Drivers,
with Layouts, Effects, and Drivers as top-level sibling containers and Layer
nested under Effects. Update docs/usecases/home-automation.md at line 111 to
direct users to add the Hue driver in the top-level Drivers container, not under
a Layer.
- Line 322: Update the architecture document link description in the “The
architecture doc” bullet to reflect the hierarchy `Layouts → Effects → Layer →
Effect/Modifier → Drivers`, distinguishing the top-level Effects container from
its child Effect/Modifier stage. Keep the existing hot-path rules reference
unchanged.
In `@src/core/Scheduler.cpp`:
- Line 157: Update the nearby explanatory text to say “two Layer instances”
rather than “two Effects,” matching ModuleFactory::create("Layer") and the
duplicate Layer children under the Effects container; leave the uniqueness logic
unchanged.
In `@src/light/effects/EffectBase.h`:
- Line 69: Restore the MoonLight prior-art URL path in the comments by replacing
Effects/VirtualLayer.h with Layers/VirtualLayer.h in
src/light/effects/EffectBase.h lines 69-69, src/light/layers/Buffer.h lines
28-28, and src/light/layers/Layer.h lines 33-33; no other changes are needed.
In `@src/light/layers/Effects.h`:
- Around line 45-48: Update the comments around the role-filtered loop
propagation and the corresponding comment near line 103 to describe child
modules as Layer instances or having ModuleRole::Layer, not Effects. Keep
Effects reserved for the container terminology and align both comments with the
implementation.
- Line 16: Replace the broken MoonLight prior-art URL in the comment near the
Effects container in src/light/layers/Effects.h:16-16 with one verified
canonical reference, or remove the link. Apply the same consistent update to the
Drivers prior-art comment in src/light/drivers/Drivers.h:71-71.
In `@src/main.cpp`:
- Line 158: Add a one-time migration alongside
mm::ModuleFactory::registerType<mm::Effects> that rewrites persisted “Layers.*”
keys to “Effects.*” and “captures”:“Layers” values to “Effects” across settings
and preset payloads before loading them. Ensure existing data remains
applicable, and add coverage or documentation for the complete migration.
- Line 158: Update the documentation link in
mm::ModuleFactory::registerType<mm::Effects> from the stale `#layers` anchor to
light/supporting.md#effects, unless an explicit compatibility anchor is
intentionally retained at the Effects heading.
In `@test/scenario_runner.cpp`:
- Around line 316-323: Keep Effects/Layer terminology consistent across the
affected tests: in test/scenario_runner.cpp lines 316-323, describe child Layer
modules; update the specified descriptions in
test/unit/light/unit_Effects_container.cpp lines 29-32, 90-91, 117-119, and
141-145 to refer to child Layers, enabled Layer buffers, and Drivers compositing
Layers. In test/unit/light/unit_Effects_container.cpp lines 258-265, 280-287,
302-308, 322-329, 349-358, 404-408, 424-433, and 451, rename the local
mm::Effects variable from layers to effects without changing behavior.
In `@test/unit/light/unit_Drivers_container.cpp`:
- Line 11: Update the comment describing Layer::tick() and Effects::tick() to
replace “child Effects” with “child Layer modules,” accurately identifying
Effects as the top-level container and Layer as its child type.
🪄 Autofix
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 Plus
Run ID: ba437a56-c961-4f03-931a-3d9a52d331d3
⛔ Files ignored due to path filters (1)
docs/assets/light/Effects.pngis excluded by!**/*.png
📒 Files selected for processing (63)
docs/MIGRATING.mddocs/architecture.mddocs/backlog/power-functions-analysis-top-down.mddocs/backlog/system-modules.mddocs/coding-standards.mddocs/gettingstarted.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mddocs/moonmodules/core/services.mddocs/moonmodules/light/supporting.mddocs/usecases/build-your-own-moonmodules.mddocs/usecases/home-automation.mdsrc/core/ControlModule.hsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/Scheduler.cppsrc/core/Services.hsrc/light/drivers/Drivers.hsrc/light/effects/EffectBase.hsrc/light/layers/Buffer.hsrc/light/layers/Effects.hsrc/light/layers/Layer.hsrc/light/layers/MappingLUT.hsrc/light/layouts/Layouts.hsrc/main.cppsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Effects_composition.jsontest/scenarios/light/scenario_GridBlacks_blackpixel.jsontest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.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/scenarios/light/scenario_perf_light.jsontest/scenarios/light/scenario_peripheral_grid_sweep.jsontest/scenarios/light/scenario_peripheral_switch.jsontest/unit/core/unit_ControlModule.cpptest/unit/core/unit_FilesystemModule_subtree.cpptest/unit/core/unit_MoonModule_lifecycle.cpptest/unit/core/unit_MqttModule.cpptest/unit/core/unit_Scheduler_unique_names.cpptest/unit/core/unit_Services.cpptest/unit/light/golden_frame.htest/unit/light/unit_Canvas.cpptest/unit/light/unit_Drivers_container.cpptest/unit/light/unit_Drivers_rendersplit.cpptest/unit/light/unit_Effects_container.cpptest/unit/light/unit_Layer_live_modifier.cpptest/unit/light/unit_Layer_persistence.cpptest/unit/light/unit_Layer_phase_animation.cpptest/unit/light/unit_Layouts_container.cpptest/unit/light/unit_Layouts_toggle_cycle.cpptest/unit/light/unit_PreviewDriver.cpptest/unit/light/unit_RandomMapModifier.cpp
Comment on lines
+272
to
+279
| **Effects** — what plays on the lights. Add an **effect** (a moving pattern), stack | ||
| several to blend them, and reshape them with **modifiers** (mirror, rotate, and | ||
| more). Each effect has its own controls — speed, color mode, and so on — that you | ||
| tweak live. | ||
|
|
||
|  | ||
|  | ||
|
|
||
| > [Layers](moonmodules/light/supporting.md) · [Layer](moonmodules/light/supporting.md) | ||
| > [Effects](moonmodules/light/supporting.md) · [Layer](moonmodules/light/supporting.md) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Propagate the Layers → Effects rename through public documentation.
The visible rename is applied, but old container names remain in pipeline prose, preset-role prose, and anchors. Use Effects for the top-level container and Layer only for child modules.
docs/gettingstarted.md#L272-L279: update the pipeline references at Lines 257-261 and 290-299, andREADME.mdLine 23, to useLayouts → Effects → Drivers.docs/moonmodules/core/control.md#L38-L45: change the preset-role prose at Lines 51 and 53 fromlayertoEffectsoreffects.docs/moonmodules/light/supporting.md#L21-L27: add and use theeffectsanchor, update the test link, and retainlayersonly as an intentional compatibility alias.
📍 Affects 3 files
docs/gettingstarted.md#L272-L279(this comment)docs/moonmodules/core/control.md#L38-L45docs/moonmodules/light/supporting.md#L21-L27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/gettingstarted.md` around lines 272 - 279, Propagate the
Layers-to-Effects terminology across all specified documentation: in
docs/gettingstarted.md lines 257-261 and 290-299 and README.md line 23, use the
pipeline name “Layouts → Effects → Drivers”; in docs/moonmodules/core/control.md
lines 51 and 53, change preset-role references from “layer” to “Effects” or
“effects”; in docs/moonmodules/light/supporting.md lines 21-27, add and use the
effects anchor, update the test link accordingly, and retain layers only as an
intentional compatibility alias.
| /// **Prior art:** MoonLight's `PhysicalLayer` runs N `VirtualLayer`s and composites their buffers into the display channel — same idea, different shape: Drivers (not Layers) does the compositing here (https://github.com/ewowi/MoonLight/blob/main/src/MoonLight). | ||
| /// @card Layers.png | ||
| class Layers : public MoonModule { | ||
| /// **Prior art:** MoonLight's `PhysicalLayer` runs N `VirtualLayer`s and composites their buffers into the display channel — same idea, different shape: Drivers (not Effects) does the compositing here (https://github.com/ewowi/MoonLight/blob/main/src/MoonLight). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the duplicated broken prior-art links.
The same prior-art URL appears in both changed comments and returns 404. Replace both links with one verified canonical MoonLight reference, or remove both links. ()
- src/light/layers/Effects.h#L16-L16: update the
Effectscontainer prior-art link. - src/light/drivers/Drivers.h#L71-L71: update the
Driversprior-art link.
📍 Affects 2 files
src/light/layers/Effects.h#L16-L16(this comment)src/light/drivers/Drivers.h#L71-L71
🤖 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/light/layers/Effects.h` at line 16, Replace the broken MoonLight
prior-art URL in the comment near the Effects container in
src/light/layers/Effects.h:16-16 with one verified canonical reference, or
remove the link. Apply the same consistent update to the Drivers prior-art
comment in src/light/drivers/Drivers.h:71-71.
Source: MCP tools
| // Containers | ||
| mm::ModuleFactory::registerType<mm::Layouts>("Layouts", "light/supporting.md#layouts"); | ||
| mm::ModuleFactory::registerType<mm::Layers>("Layers", "light/supporting.md#layers"); | ||
| mm::ModuleFactory::registerType<mm::Effects>("Effects", "light/supporting.md#layers"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Migrate persisted keys as well as the filename.
Changing the registered type to "Effects" changes more than /.config/Layers.json to Effects.json. Existing persistence and preset payloads also contain old "Layers.*" keys and "captures":"Layers" values, while the updated fixtures require "Effects.*" and "captures":"Effects". Renaming only the file leaves settings unapplied and makes old presets non-applicable.
Add a one-time migration, or document and test a complete rewrite of the old keys and capture values before shipping.
🤖 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` at line 158, Add a one-time migration alongside
mm::ModuleFactory::registerType<mm::Effects> that rewrites persisted “Layers.*”
keys to “Effects.*” and “captures”:“Layers” values to “Effects” across settings
and preset payloads before loading them. Ensure existing data remains
applicable, and add coverage or documentation for the complete migration.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '^#+ .*(Effects|Layers)' docs/moonmodules/light/supporting.mdRepository: MoonModules/projectMM
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### File context"
sed -n '1,50p' docs/moonmodules/light/supporting.md
echo
echo "### Anchor usage"
rg -n 'supporting\.md#[A-Za-z0-9-]+' .Repository: MoonModules/projectMM
Length of output: 3263
Fix the Effects documentation anchor.
light/supporting.md has an old #layers anchor before the renamed Effects section and no matching #layers heading. registerType<mm::Effects>("Effects", "light/supporting.md#layers") points to the wrong section. Update it to light/supporting.md#effects, or keep an explicit compatibility anchor at the Effects heading.
🤖 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` at line 158, Update the documentation link in
mm::ModuleFactory::registerType<mm::Effects> from the stale `#layers` anchor to
light/supporting.md#effects, unless an explicit compatibility anchor is
intentionally retained at the Effects heading.
Comment on lines
+316
to
+323
| if (std::strcmp(type, "Effects") == 0) { | ||
| // Wire the container's Layouts (mirrors main.cpp's | ||
| // layersContainer->setLayouts). Layers re-propagates this to its | ||
| // child Layers at every prepareTree, so a Layer added later picks | ||
| // effectsContainer->setLayouts). Effects re-propagates this to its | ||
| // child Effects at every prepareTree, so a Layer added later picks | ||
| // it up — the self-healing path the device relies on. | ||
| if (props.has("layouts")) { | ||
| auto* layoutsModule = static_cast<mm::Layouts*>(modules[props["layouts"].str]); | ||
| if (layoutsModule) static_cast<mm::Layers*>(mod)->setLayouts(layoutsModule); | ||
| if (layoutsModule) static_cast<mm::Effects*>(mod)->setLayouts(layoutsModule); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep the Effects/Layer boundary consistent in test comments and names.
The runtime contract uses Effects as the grouping container and Layer as its child role. The changed test text and local names use Effects for child Layers or layers for an mm::Effects object.
test/scenario_runner.cpp#L316-L323: change “child Effects” to “child Layer modules”.test/unit/light/unit_Effects_container.cpp#L29-L32: describe composition across child Layers.test/unit/light/unit_Effects_container.cpp#L90-L91: rename “two child Effects” to “two child Layers”.test/unit/light/unit_Effects_container.cpp#L117-L119: refer to enabled Layer buffers.test/unit/light/unit_Effects_container.cpp#L141-L145: describe Drivers compositing Layers from the Effects container.test/unit/light/unit_Effects_container.cpp#L258-L265: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L280-L287: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L302-L308: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L322-L329: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L349-L358: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L404-L408: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L424-L433: rename the locallayersvariable toeffects.test/unit/light/unit_Effects_container.cpp#L451-L451: rename the locallayersvariable toeffects.
As per coding guidelines, use standard, recognizable names and make test descriptions state user-understandable behavior.
📍 Affects 2 files
test/scenario_runner.cpp#L316-L323(this comment)test/unit/light/unit_Effects_container.cpp#L29-L32test/unit/light/unit_Effects_container.cpp#L90-L91test/unit/light/unit_Effects_container.cpp#L117-L119test/unit/light/unit_Effects_container.cpp#L141-L145test/unit/light/unit_Effects_container.cpp#L258-L265test/unit/light/unit_Effects_container.cpp#L280-L287test/unit/light/unit_Effects_container.cpp#L302-L308test/unit/light/unit_Effects_container.cpp#L322-L329test/unit/light/unit_Effects_container.cpp#L349-L358test/unit/light/unit_Effects_container.cpp#L404-L408test/unit/light/unit_Effects_container.cpp#L424-L433test/unit/light/unit_Effects_container.cpp#L451-L451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/scenario_runner.cpp` around lines 316 - 323, Keep Effects/Layer
terminology consistent across the affected tests: in test/scenario_runner.cpp
lines 316-323, describe child Layer modules; update the specified descriptions
in test/unit/light/unit_Effects_container.cpp lines 29-32, 90-91, 117-119, and
141-145 to refer to child Layers, enabled Layer buffers, and Drivers compositing
Layers. In test/unit/light/unit_Effects_container.cpp lines 258-265, 280-287,
302-308, 322-329, 349-358, 404-408, 424-433, and 451, rename the local
mm::Effects variable from layers to effects without changing behavior.
Source: Coding guidelines
Scripts can now compute — `+`, `-`, `*` with parentheses and the usual precedence — and print a value to the serial log. On top of that, a modifier can be written as a script: the coordinate transform that decides where each light sits in the pattern, edited on a running device instead of compiled in and reflashed. desktop 133us/7,518fps | esp32 4,164us/240fps | flash: desktop 1,076KB (+2KB), esp32s3-n16r8 1,704KB (+4KB) Core - MoonLive gains arithmetic: precedence climbing over the existing Const/Add/Mul IR, so all three backends lower it unchanged. Subtraction is emitted as a + (b * -1): no supported ISA has a subtract, and Xtensa's add-immediate masks its operand to four bits, so negating the immediate there would silently produce a different constant. - Division and modulo are deliberately absent: both need a two-argument host call and Call is unary today. A script using them gets "unexpected character", which is the honest answer rather than a misleading parse error. Light domain - print(v) logs a value and returns it, so it wraps any sub-expression without changing the result. It is the only view inside a running script — one that compiles cleanly and renders wrong gives no other clue, which is exactly the case that cost this session its longest debugging detour. Capped at a short burst: the script runs once per light, and an uncapped print on a 16k wall would be 16k blocking serial writes per rebuild. - setXYZ(index, x, y, z) writes a POSITION through the same element store setRGB writes a colour through — three values at index * stride. The engine needed no change to host a second binding, which is the evidence its domain-neutrality is real rather than asserted. - MoonLiveModifier: scripts read x/y/z and width/height/depth as injected controls. A script needs the EXTENT to write a mirror at all; against a fixed 255 every light of a 16-wide grid lands out of bounds, the Layer drops it, and the fixture goes black with no error anywhere. The bug that made it render nothing, since it is the interesting one: prepare() asked the Layer for a mapping rebuild, the Layer's rebuild IS applyState(), and applyState() calls prepare(). The two called each other forever, the mapping was permanently mid-rebuild, and nothing ever drew. Every part was correct in isolation — the script compiled, the fold returned the right coordinates, every light was accepted — so only a probe inside the fold showed it: 782,296 rebuilds in six seconds where there should have been one. A rebuild is now requested only when the compiled source actually changed. Tests - The rebuild cycle: re-preparing an unchanged script must not ask again. - The black-screen case: the default script stays inside the grid it is given, at every coordinate. - print() transparency, arithmetic precedence, and the per-light cost (16,384 transforms in 31us, so the native call per light is free). Docs/CI - MoonLiveModifier.md, including the two limits worth knowing: a coordinate is a byte, and a script cannot resize the logical box. - Rename fallout from the external review: four upstream MoonLight URLs my sweep had rewritten to a path that does not exist, two broken doc anchors, the registerType help link, the README pipeline name, a heading level, and four comments saying Effects where they meant Layer. - The preset capture role is "effects" rather than "layer": it names the container a preset captures, not a module inside it. A breaking change, documented in MIGRATING.md. - system-modules.md no longer justifies a core-domain split by naming light-domain containers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/MIGRATING.md`:
- Around line 33-38: Update the migration documentation in docs/MIGRATING.md
(lines 33-38) to represent preset captures as the single string value the loader
reads, using "Layers" only for the file-edit migration path; update the
corresponding guidance in docs/moonmodules/core/control.md (lines 35-43)
consistently. Do not document captures as an array.
In `@docs/moonmodules/light/MoonLiveModifier.md`:
- Around line 41-47: Update the Limits section of MoonLiveModifier documentation
to state that values written by setXYZ are stored as bytes and therefore
truncate or wrap to the 0–255 range, including transformed outputs beyond that
range. Place this behavior alongside the existing input-coordinate limit
description and keep the documented input behavior unchanged.
In `@README.md`:
- Line 23: Update the pipeline description near the `layouts → layers with
effects + modifiers → drivers` text to name the top-level stage `Effects` and
describe its child layers separately, matching the terminology used in the
pluggable pipeline description while preserving the existing pipeline order.
In `@src/light/moonlive/MoonLiveBuiltins_light.h`:
- Around line 34-47: The print burst cap is process-lifetime rather than per-run
because mm_light_print uses a never-reset function-local static counter. In
src/light/moonlive/MoonLiveBuiltins_light.h:34-47, either reset the counter at
the start of each script run sequence or revise the surrounding comment to
accurately describe per-boot behavior; in
docs/moonmodules/light/MoonLiveModifier.md:39, update the documentation to match
the shipped behavior, explicitly replacing the per-run burst claim.
In `@src/light/moonlive/MoonLiveModifier.h`:
- Around line 138-151: Remove the unused public test accessors sourceForTest()
and boxForTest() from MoonLiveModifier; do not alter the release() or
setSource() behavior.
- Around line 165-175: Replace the redundant compiled_ buffer in the MoonLive
modifier state with a compact checksum of full_. Update the rebuild-detection
logic to compute and compare the checksum, refreshing it whenever the mapping is
rebuilt, while preserving the existing source-change behavior and using the
repository’s existing core/crc helper.
In `@test/unit/light/unit_MoonLiveModifier.cpp`:
- Around line 167-209: Remove the first duplicate rebuild-signal test case and
its preceding explanatory comment. Keep the second test case, “editing a script
asks the layer to rebuild its mapping,” including its rebuild-consumption checks
and final modifyLogical assertion.
- Around line 240-265: Update the test case “a script that computes a position
outside the grid does not black out the fixture” so it observes the modifier’s
mapped output rather than the prefilled layer buffer. Remove the direct buffer
fill/count assertion and inspect the mapping fold’s counts or destinations after
applying the layer, or render through a driver and count emitted lights; ensure
the assertion fails when no light is mapped.
- Around line 147-165: Update the test case “transforming a wall's worth of
lights stays within a rebuild's budget” to remove the timing-dependent CHECK on
elapsed microseconds and the associated wall-clock assertion. Preserve the loop
and MESSAGE measurement for reporting, and replace the timing-based validation
with a deterministic assertion that directly verifies the script is not compiled
once per modifyLogical call, using the existing observable API or state.
🪄 Autofix
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 Plus
Run ID: 6f4521ac-651a-4bef-9cee-475ff9d31d5e
📒 Files selected for processing (25)
README.mddocs/MIGRATING.mddocs/architecture.mddocs/backlog/system-modules.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mddocs/moonmodules/core/services.mddocs/moonmodules/light/MoonLiveModifier.mddocs/usecases/build-your-own-moonmodules.mddocs/usecases/home-automation.mdsrc/core/ControlModule.hsrc/core/moonlive/MoonLiveCompiler.cppsrc/light/drivers/Drivers.hsrc/light/layers/Effects.hsrc/light/layers/Layer.hsrc/light/moonlive/MoonLiveBuiltins_light.hsrc/light/moonlive/MoonLiveModifier.hsrc/main.cppsrc/ui/app.jstest/CMakeLists.txttest/unit/core/unit_ControlModule.cpptest/unit/light/unit_Drivers_container.cpptest/unit/light/unit_Effects_container.cpptest/unit/light/unit_MoonLiveModifier.cpp
Comment on lines
+33
to
+38
| | The saved light tree | The device looks for `/.config/Effects.json` and the old file is `Layers.json`, so the light tree boots empty | Re-add your Layer, effect and modifiers, then let it save | | ||
| | Presets that capture the look | A preset file records `"captures": ["Layers"]`, a key no module now answers to | Re-save each preset once the tree is rebuilt | | ||
|
|
||
| A preset also records the ROLE it covers, and that role is now named after the container rather than after a module inside it: `"layer"` becomes `"effects"`. A preset carrying the old role still loads, but shows no tint or emoji on its pad until it is re-saved — the UI has no `layer` role to colour it by. | ||
|
|
||
| Renaming the file on the device works if you would rather not rebuild by hand: `Layers.json` → `Effects.json`, and `"Layers"` → `"Effects"` inside each `/.config/presets/*.json`. Nothing else in either file changes. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect one persisted captures shape across implementation and documentation.
rg -n -C 8 \
'saveSubtreeTo|applySubtree|captures' \
--glob '*.{cpp,h,hpp,md,json}'Repository: MoonModules/projectMM
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching target names:"
git ls-files | rg '(^docs/MIGRATING\.md$|^docs/moonmodules/core/control\.md$|FilesystemModule\.md$|\.cpp$|\.h$|\.hpp$|\.json$)' | sed -n '1,200p'
echo
echo "Search presets/captures/persistence names without filtering by extension first:"
rg -n -C 5 'captures|saveSubtreeTo|applySubtree|Layers|Effects' . --glob '!**/.git/**' | sed -n '1,300p'Repository: MoonModules/projectMM
Length of output: 36383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target docs lines:"
sed -n '1,80p' docs/MIGRATING.md
echo
sed -n '1,70p' docs/moonmodules/core/control.md
echo
echo "Source files containing LightPresetsModule/preset/captures/saveSubtreeTo/applySubtree:"
rg -n -C 6 'LightPresetsModule|LightPresets|preset|presets|captures|saveSubtreeTo|applySubtree|Layers|Effects' \
src moondeck library.json espidf component.mk CMakeLists.txt -g '!**/build/**' -g '!**/.git/**' | sed -n '1,400p'
echo
echo "List LightPresetsModule candidate files:"
git ls-files | rg '(^src/light/drivers/LightPresetsModule\.(h|cpp)$|.*ControlModule.*|.*FilesystemModule.*)'Repository: MoonModules/projectMM
Length of output: 49643
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'class ControlModule|struct P|presets|captures|preset|apply\(|save\(' \
src/core/Control*.h src/core/Control*.cpp src/core/ControlModule.cpp || true
python3 - <<'PY'
import subprocess, re
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
for f in files:
if f.endswith(('.h','.hpp','.cpp','.md')):
text = open(f, encoding='utf-8', errors='surrogateescape').read()
for name in ['ControlModule','presetsRevision','presetCount','presetName','activePreset','applyPreset','capture','captures']:
if name in text:
lines = text.splitlines()
for i,l in enumerate(lines,1):
if name in l:
print(f"{f}:{i}: {l.strip()}")
PYRepository: MoonModules/projectMM
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ControlModule implementation excerpt:"
sed -n '1,240p' src/core/ControlModule.cpp
echo
sed -n '240,420p' src/core/ControlModule.cpp
echo
echo "Read-only semantic probe: parseString ignores array value."
python3 - <<'PY'
# Mirror the minimal parseString path from JsonUtil/ControlModule behavior without running repo code:
# parseString(head, "captures", p.captures) expects "captures":"Value"; when captures is ["Layers"]
# the value token is [, so the string parse does not produce "Layers".
import json
for value in ["Effects", '["Layers"]']:
data = {"captures": value}
print(value, "== parseString target ==")
try:
print(json.loads(data))
except Exception as e:
print("exception", *type(e).__name__, str(e))
PYRepository: MoonModules/projectMM
Length of output: 267
Keep the migration capture key as a string.
ControlModule persists preset captures as a single subtree string (for example "Effects"); "captures": ["Layers"] does not match the single-value capture the loader reads. The migration instruction should use "Layers" in the migrate-to example only if a file edit is the chosen path.
📍 Affects 2 files
docs/MIGRATING.md#L33-L38(this comment)docs/moonmodules/core/control.md#L35-L43
🤖 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/MIGRATING.md` around lines 33 - 38, Update the migration documentation
in docs/MIGRATING.md (lines 33-38) to represent preset captures as the single
string value the loader reads, using "Layers" only for the file-edit migration
path; update the corresponding guidance in docs/moonmodules/core/control.md
(lines 35-43) consistently. Do not document captures as an array.
Source: Coding guidelines
| 🧊 **Native 3D from the ground up**: 2D and 1D are just the cases where a dimension is size 1. Effects never pick a mode. | ||
|
|
||
| 🎛️ **Pluggable pipeline**: Layouts → Layers (effects + modifiers) → Drivers. Build it visually in the browser, and every change applies live (settings also persist to flash across power cycles). | ||
| 🎛️ **Pluggable pipeline**: Layouts → Effects (layers of effects + modifiers) → Drivers. Build it visually in the browser, and every change applies live (settings also persist to flash across power cycles). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the README pipeline terminology consistent.
Line 23 now uses Effects, but Line 106 still describes layouts → layers with effects + modifiers → drivers. The README presents two names for the same top-level stage. Update Line 106 to use Effects and distinguish its Layer children.
🤖 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 `@README.md` at line 23, Update the pipeline description near the `layouts →
layers with effects + modifiers → drivers` text to name the top-level stage
`Effects` and describe its child layers separately, matching the terminology
used in the pluggable pipeline description while preserving the existing
pipeline order.
Source: Coding guidelines
Comment on lines
+165
to
+175
| char source_[384] = "setXYZ(0, width - 1 - x, y, z);"; | ||
|
|
||
| // The user's source with the input declarations prepended — what actually gets compiled. | ||
| char full_[512] = {}; | ||
| // The source the CURRENT mapping was built from; a rebuild is needed only when it changes. | ||
| char compiled_[512] = {}; | ||
|
|
||
| char ctrlNames_[moonlive::kMaxCtrls][moonlive::kMaxControlName] = {}; | ||
|
|
||
| bool needsRebuild_ = false; // a recompile happened; the Layer's mapping is stale | ||
| Coord3D box_{0, 0, 0}; // the logical box, from modifyLogicalSize |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Consider a checksum instead of a second 512-byte copy.
Each instance holds source_ (384), full_ (512) and compiled_ (512), so about 1.4 KB of static RAM per modifier before ctrlNames_. compiled_ exists only to detect a source change. A CRC or FNV hash of full_ gives the same comparison for four bytes.
The repository already ships a CRC helper (unit/core/unit_crc.cpp implies core/crc), so no new dependency is needed.
♻️ Proposed change
- // The source the CURRENT mapping was built from; a rebuild is needed only when it changes.
- char compiled_[512] = {};
+ // A digest of the source the CURRENT mapping was built from; a rebuild is needed only when it
+ // changes. A digest rather than a copy: a second 512-byte buffer per modifier is real RAM on
+ // an ESP32.
+ uint32_t compiledHash_ = 0;- if (std::strcmp(full_, compiled_) != 0) {
- std::snprintf(compiled_, sizeof(compiled_), "%s", full_);
+ const uint32_t h = hashSource(full_);
+ if (h != compiledHash_) {
+ compiledHash_ = h;
needsRebuild_ = true;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/moonlive/MoonLiveModifier.h` around lines 165 - 175, Replace the
redundant compiled_ buffer in the MoonLive modifier state with a compact
checksum of full_. Update the rebuild-detection logic to compute and compare the
checksum, refreshing it whenever the mapping is rebuilt, while preserving the
existing source-change behavior and using the repository’s existing core/crc
helper.
A script's subtraction was correct only modulo 256 on two of the three targets. `a - b` compiles to `a + (b * -1)`, and both assemblers built that -1 through a 16-bit immediate, so the register held 65535. A stored colour byte came out right either way, which is why every test passed; anything reading the full value did not. An index computed by subtraction became ~65k, the element store's bounds guard rejected it, and the light silently never lit. desktop 133us/7,518fps | esp32 4,164us/240fps | flash unchanged Platform - arm64 emits `movn wD, #~imm16` for a negative constant; movz can only build a zero-extended one. - Xtensa uses the wide `movi` 12-bit SIGNED field (-2048..2047), the only encoding here that holds a negative. Verified byte for byte: -1 emits 0x32 0xaf 0xff, which the CPU reads back as -1. - RISC-V already sign-extended and is unchanged. Confirmed on the bench: the scripted mirror renders on the S3, which it could not have done with a wrapped -1 — every light would have folded outside the grid and the panel would be black. Light domain - A disabled-then-re-enabled scripted modifier was permanently inert: release() frees the compiled program but kept the source it was compiled from, so the next prepare saw "unchanged" and never asked for the rebuild that would apply it. release() now forgets it, which is the state a first compile starts from. - print()'s burst resets on every compile. It was a process-lifetime counter, so the tool that exists to explain a misbehaving script went silent after one burst — including in the middle of diagnosing this commit's own bug. Tests - Subtraction is pinned THROUGH print(), which returns the full 32-bit value. A byte comparison cannot see this class of bug at all: mod-256 congruence makes the stored byte correct whether or not the arithmetic is. - The wall-budget test asserts the compiled program is unchanged across 16,384 folds rather than a wall-clock bound, so it states the property (no per-call compile) instead of how busy the machine is. - The blackout test observes the folded COORDINATES. It filled the buffer through draw::fill and counted lit bytes, which writes every byte itself and so could not fail whatever the mapping did. Docs - MIGRATING: a preset records `"captures": "Layers"`, a single string; it was written as an array, which would send a file edit hunting for brackets that are not there. - MoonLiveModifier: a failed compile falls back to passing coordinates through (the doc claimed the previous mapping survives), and a position a script computes past 255 keeps its low byte. Reviews - Reviewer (Fable): H1 (this commit's subject), M1, M2 and L1-L5 addressed. - CodeRabbit: 5 fixed, 4 already done, 1 skipped — replacing the compared source with a checksum trades a collision-shaped false negative for nothing on a cold path that compares once per prepare. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/moonmodules/light/MoonLiveModifier.md (1)
49-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument script-declared controls.
The table lists only
source, butMoonLiveModifier::defineControls()also exposes each user-declared MoonLive control after compilation. Document how a script declares these controls and state that changing one rebuilds the mapping.As per coding guidelines: “Documentation must describe the system as it currently exists.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/light/MoonLiveModifier.md` around lines 49 - 55, Update the Controls section of MoonLiveModifier documentation to include the user-declared controls exposed by MoonLiveModifier::defineControls(). Describe how scripts declare these controls and state that changing a declared control rebuilds the mapping, alongside the existing source control behavior.Source: Coding guidelines
test/unit/light/unit_MoonLiveModifier.cpp (1)
151-174: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftUse an observable compile count.
dynamicBytes()isengine_.codeCap(). Recompiling the same source can produce the same executable capacity, so Line 174 still passes ifmodifyLogical()compiles on every call. Add a compile-count test seam or an executable-allocation counter and assert that the count remains one across the fold.🤖 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_MoonLiveModifier.cpp` around lines 151 - 174, Replace the dynamicBytes() comparison in the MoonLiveModifier test with an observable compilation count, exposed through an appropriate test seam or executable-allocation counter. Capture the count after m.prepare(), assert it reflects one compilation, and verify it remains unchanged after all modifyLogical() calls; do not rely on executable capacity as the recompilation indicator.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 `@src/light/moonlive/MoonLiveBuiltins_light.h`:
- Around line 47-52: Update mm_light_print and its printBudget diagnostic flow
so effect ticks perform only bounded, nonallocating queue writes and remain
noexcept, removing both std::printf calls from the tick path. Store the
formatted diagnostic records in a preallocated queue, then drain that queue from
a housekeeping path through the existing platform output seam, preserving the
print budget and burst-spent message behavior.
In `@src/platform/desktop/moonlive_asm_host.cpp`:
- Around line 48-59: Complete signed-immediate lowering in
HostAssembler::mulImm() so negative multipliers, including -1, are materialized
with signed semantics; update src/platform/desktop/moonlive_asm_host.cpp lines
48-59, while HostAssembler::movImm() is only context. In
src/platform/esp32/moonlive_asm_xtensa.cpp lines 58-71, prevent values below
-2048 from falling through to unsigned construction by materializing the full
signed value or rejecting the literal during compilation. Add backend tests
covering subtraction and a literal below -2048.
In `@test/unit/light/unit_MoonLiveModifier.cpp`:
- Around line 224-259: Update the test case name to distinguish the intentional
out-of-grid mapping from the default-script assertion, then add CHECK(inside ==
0) after the loop in “a script that computes a position outside the grid leaves
lights mapped” to assert every transformed coordinate is unmapped while
preserving the existing defInside check.
- Around line 290-308: Update the subtraction regression test around “a
subtraction produces the whole value, not just its low byte” to run against a
multi-light buffer and use the subtraction result as a bounds-checked element
index. Assert that the expected destination element is written, ensuring values
such as 99 and an incorrectly widened result cannot both pass through setXYZ’s
byte truncation.
---
Outside diff comments:
In `@docs/moonmodules/light/MoonLiveModifier.md`:
- Around line 49-55: Update the Controls section of MoonLiveModifier
documentation to include the user-declared controls exposed by
MoonLiveModifier::defineControls(). Describe how scripts declare these controls
and state that changing a declared control rebuilds the mapping, alongside the
existing source control behavior.
In `@test/unit/light/unit_MoonLiveModifier.cpp`:
- Around line 151-174: Replace the dynamicBytes() comparison in the
MoonLiveModifier test with an observable compilation count, exposed through an
appropriate test seam or executable-allocation counter. Capture the count after
m.prepare(), assert it reflects one compilation, and verify it remains unchanged
after all modifyLogical() calls; do not rely on executable capacity as the
recompilation indicator.
🪄 Autofix
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 Plus
Run ID: e555b367-e5cf-45b3-b0b5-f02211e7051e
📒 Files selected for processing (11)
docs/MIGRATING.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/light/MoonLiveModifier.mdsrc/light/moonlive/MoonLiveBuiltins_light.hsrc/light/moonlive/MoonLiveEffect.hsrc/light/moonlive/MoonLiveModifier.hsrc/platform/desktop/moonlive_asm_host.cppsrc/platform/esp32/moonlive_asm_xtensa.cpptest/unit/light/unit_Effects_container.cpptest/unit/light/unit_MoonLiveModifier.cpp
Comment on lines
+47
to
+52
| extern "C" inline uint32_t mm_light_print(uint32_t v) { | ||
| uint32_t& left = printBudget(); | ||
| if (left > 0) { | ||
| std::printf("[script] %u\n", static_cast<unsigned>(v)); | ||
| if (--left == 0) std::printf("[script] (burst spent; edit the script for a fresh one)\n"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Remove blocking output from the effect tick path.
MoonLiveEffect::tick() can invoke mm_light_print() through engine_.run(). Line 50 calls std::printf, which can block the render tick. Store bounded diagnostic records in a preallocated queue and drain them from a housekeeping path through a platform seam.
As per path instructions: “render/tick code is nonblocking and noexcept; avoid allocation, blocking I/O, delays, or network calls there.”
🤖 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/light/moonlive/MoonLiveBuiltins_light.h` around lines 47 - 52, Update
mm_light_print and its printBudget diagnostic flow so effect ticks perform only
bounded, nonallocating queue writes and remain noexcept, removing both
std::printf calls from the tick path. Store the formatted diagnostic records in
a preallocated queue, then drain that queue from a housekeeping path through the
existing platform output seam, preserving the print budget and burst-spent
message behavior.
Source: Path instructions
Comment on lines
+48
to
+59
| void HostAssembler::movImm(Reg d, int32_t imm) { | ||
| // movz builds a ZERO-extended 16-bit constant, so a negative immediate would land as its | ||
| // unsigned counterpart (-1 as 65535). The compiler emits Const(-1) to express subtraction — | ||
| // `a - b` is `a + (b * -1)` — and a wrapped -1 makes every subtraction correct only modulo 256. | ||
| // In a stored colour byte that is invisible; in a bounds-guarded index it silently drops the | ||
| // light, and in a host-call argument it is nonsense. movn is the negative form: it writes | ||
| // ~imm16, so movn #(~imm) materialises the true negative value. | ||
| if (imm < 0) { | ||
| emit32(0x12800000u | ((uint32_t(~imm) & 0xffff) << 5) | mr(d)); // movn wD, #~imm16 | ||
| return; | ||
| } | ||
| emit32(0x52800000u | ((uint32_t(imm) & 0xffff) << 5) | mr(d)); // movz wD, #imm16 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Complete signed immediate lowering in both assemblers.
The new signed paths do not cover all generated negative constants. The host multiplication path still loads -1 as 65535. The Xtensa fallback loads values below -2048 as unsigned 16-bit values.
src/platform/desktop/moonlive_asm_host.cpp#L48-L59: updateHostAssembler::mulImm()to materialize negative multipliers with signed semantics, including-1.src/platform/esp32/moonlive_asm_xtensa.cpp#L58-L71: do not fall through to unsigned construction for values below-2048; materialize the full signed value or reject the literal during compilation.- Add backend tests for subtraction and a literal below
-2048.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 48-48: The function 'movImm' is never used.
(unusedFunction)
📍 Affects 2 files
src/platform/desktop/moonlive_asm_host.cpp#L48-L59(this comment)src/platform/esp32/moonlive_asm_xtensa.cpp#L58-L71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/moonlive_asm_host.cpp` around lines 48 - 59, Complete
signed-immediate lowering in HostAssembler::mulImm() so negative multipliers,
including -1, are materialized with signed semantics; update
src/platform/desktop/moonlive_asm_host.cpp lines 48-59, while
HostAssembler::movImm() is only context. In
src/platform/esp32/moonlive_asm_xtensa.cpp lines 58-71, prevent values below
-2048 from falling through to unsigned construction by materializing the full
signed value or rejecting the literal during compilation. Add backend tests
covering subtraction and a literal below -2048.
Comment on lines
+224
to
+259
| TEST_CASE("a script that computes a position outside the grid leaves lights mapped") { | ||
| // The black-screen failure, at the level that can actually fail. Byte arithmetic wraps, so a | ||
| // script computing a position past the grid lands somewhere unintended — and if a coordinate | ||
| // ends up outside the logical box the Layer DISCARDS that light, which is how the fixture went | ||
| // dark with no error reported anywhere. | ||
| // | ||
| // The observation has to be the MAPPING. Filling the buffer through a Canvas and counting lit | ||
| // bytes cannot fail: draw::fill writes every byte itself, whatever the fold decided. | ||
| MoonLiveModifier m; | ||
| m.defineControls(); | ||
| m.setSource("setXYZ(0, x + 200, y, z);"); // deliberately off the end of a 16-wide grid | ||
| m.prepare(); | ||
| Coord3D box{16, 16, 1}; | ||
| m.modifyLogicalSize(box); | ||
|
|
||
| int inside = 0; | ||
| for (lengthType i = 0; i < 16; i++) { | ||
| Coord3D pos{i, 0, 0}; | ||
| m.modifyLogical(pos); | ||
| if (pos.x >= 0 && pos.x < 16) inside++; // what the Layer will keep | ||
| } | ||
| INFO("coordinates still inside a 16-wide grid: " << inside << " of 16"); | ||
| // Every light falling outside is precisely the blackout. The default script must keep them all. | ||
| MoonLiveModifier def; | ||
| def.defineControls(); | ||
| def.prepare(); | ||
| Coord3D defBox{16, 16, 1}; | ||
| def.modifyLogicalSize(defBox); | ||
| int defInside = 0; | ||
| for (lengthType i = 0; i < 16; i++) { | ||
| Coord3D pos{i, 0, 0}; | ||
| def.modifyLogical(pos); | ||
| if (pos.x >= 0 && pos.x < 16) defInside++; | ||
| } | ||
| CHECK(defInside == 16); // the shipped default never blacks a fixture out | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the deliberate out-of-grid result.
The test calculates inside for setXYZ(0, x + 200, y, z) but never checks it. Add CHECK(inside == 0) and rename the case to distinguish this intentional unmapped result from the default-script assertion at Line 258.
🤖 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_MoonLiveModifier.cpp` around lines 224 - 259, Update the
test case name to distinguish the intentional out-of-grid mapping from the
default-script assertion, then add CHECK(inside == 0) after the loop in “a
script that computes a position outside the grid leaves lights mapped” to assert
every transformed coordinate is unmapped while preserving the existing defInside
check.
Source: Path instructions
Comment on lines
+290
to
+308
| // Subtraction is emitted as `a + (b * -1)`, and -1 has to survive into the register. The assemblers | ||
| // materialise a constant with a 16-bit immediate, so a naive -1 becomes 65535 and every subtraction | ||
| // is right only MODULO 256 — invisible in a stored byte, and wrong everywhere the full value is | ||
| // used: a bounds-guarded index silently drops the light, and a value handed to a host call is | ||
| // nonsense. Byte-comparison tests cannot see this, so it is checked through print(), which returns | ||
| // the full 32-bit value. | ||
| TEST_CASE("a subtraction produces the whole value, not just its low byte") { | ||
| // `a - b` compiles to `a + (b * -1)`, so -1 has to reach the register intact. The assemblers | ||
| // build a constant from a 16-bit immediate, and a naive -1 lands as 65535 — which leaves every | ||
| // subtraction correct only MODULO 256. A stored colour byte cannot show that (the low byte is | ||
| // right either way), so this checks the value THROUGH print(), which returns the full 32 bits | ||
| // and is therefore the only observer that can fail. | ||
| // | ||
| // The consequences the byte hides: an index computed by subtraction becomes ~65k, the element | ||
| // store's bounds guard rejects it, and the light silently never lights; a subtraction handed to | ||
| // a host call (random16, print) gets a wrong argument. | ||
| CHECK(transform("setXYZ(0, print(width - 1 - x), y, z);", 0, 0, 0, 16, 16, 1).x == 15); | ||
| CHECK(transform("setXYZ(0, print(100 - 1), y, z);", 0, 0, 0, 255, 255, 1).x == 99); | ||
| CHECK(transform("setXYZ(0, print(5 - 5), y, z);", 0, 0, 0, 255, 255, 1).x == 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test subtraction through an untruncated observer.
Each print() result is immediately stored through setXYZ(), which keeps only its low byte. Both 99 and 65635 store as 99, so these checks pass when -1 is materialized as 65535. Execute the script against a multi-light buffer and use the subtraction result as a bounds-checked element index, then assert that the expected destination was written.
🤖 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_MoonLiveModifier.cpp` around lines 290 - 308, Update the
subtraction regression test around “a subtraction produces the whole value, not
just its low byte” to run against a multi-light buffer and use the subtraction
result as a bounds-checked element index. Assert that the expected destination
element is written, ensuring values such as 99 and an incorrectly widened result
cannot both pass through setXYZ’s byte truncation.
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.
Landed:
Layers→EffectsThe three top-level light containers are now Layouts, Effects, Drivers — L.E.D. The old name
sat one character from its own child (
LayersholdingLayers) and read as a near-twin ofLayouts, which is the pair a newcomer actually has to tell apart. The tree keeps its shape:Effects→Layers → effects and modifiers.Layer, the child, is untouched. The two names differ by one character, so every replacement isword-boundary anchored rather than a token sweep — a blind replace would collapse exactly the
distinction this rename draws.
src/light/layers/keeps its name too: it also holdsLayer.h,Buffer.h,BlendMap.handMappingLUT.h, which are all still layer things.One trap worth naming:
ControlModule's capture-role index carried astatic_assertthat spelledthe old name out character by character (
[0]=='L',[1]=='a',[5]=='s'). It is re-pinned to"Effects".No golden moved, which is the evidence this is a rename and nothing else.
Breaking change
The type name is the persisted filename and the preset capture key, so a device boots with an
empty light tree and presets stop applying their effects. Per
ADR-0013 that is
documented rather than migrated — see MIGRATING.md.
Both halves were verified on a running desktop: the tree does come up empty, and renaming
/.config/Layers.jsontoEffects.jsonrestores it intact, so the shortcut in that entry is testedadvice rather than a guess.
Planned: MoonLive on the power functions
The power-function library merged in #62 was built
top-down — the primitives first, then existing effects migrated onto them. This branch runs it the
other way: MoonLive composes those primitives at runtime, in orders nobody wrote by hand, which is a
much harder caller than an effect.
The approach is step by step, starting simple and extending, and the expectation is that it finds
real problems rather than confirming the design. #62 already showed the pattern twice —
wrap()silently changed its endpoint behaviour until an exhaustive diff caught it, and Fireworks carried two
framerate bugs that cancelled each other and so stayed invisible. Neither surfaced from writing the
power function; both surfaced from a caller exercising it. Expect the fixes to cluster in the
fixed-point edges: carries, endpoints, saturation.
Known
This branch predates
611ee892on main, which fixes an MSVC C4146 error inatan16, so the Windowsjob fails here until main is merged in.
Summary by CodeRabbit
New Features
Changes
Migration
LayerstoEffects; individualLayernames remain unchanged.