Share one toolbox: power functions, particles, shaders — and presets - #62
Merged
Conversation
Save the device's state as a named preset and bring it back with one click. A preset is a file, so it can be uploaded, downloaded and shared; each one records which parts of the setup it carries, so a look saved on one board applies to a board with different hardware. The presets sit on an 8x8 pad grid with encoders above and faders below, laid out like a Mackie control desk so a MIDI surface maps onto it later without a translation layer. Flash esp32s3-n16r8 1,651 KB (+22 KB), esp32s31 1,887 KB (+7 KB), desktop 960 KB (+36 KB); desktop tick 132 us (+5 us). Core: - ControlModule: a new top-level module, peer of Layouts/Layers/Drivers rather than a child of Services, since it reaches across them. Hosts presets now and external control (MIDI, IR) later. - FilesystemModule gains two seams: saveSubtreeTo writes a subtree into a caller's sink, applySubtree puts one back onto a LIVE tree. saveSubtree now calls the former, so there is exactly one serializer. - applySubtree guards on the prefix being present: without it applyNode reads "no children in JSON" as "delete every child", so a truncated preset would wipe the live look. - ListSource::persistsList: a list whose rows are re-derived at setup is no longer written to flash. The preset list was serialized on every save and discarded on load, since nothing restores it. - Preset names are validated as printable ASCII without / \ or . -- the name becomes a file name, and ESP32's fsTranslate does no path normalization (desktop's does), so an unguarded name could escape the preset folder on device via delete, rename or save. - Control.h: fader/encoder/faderTarget descriptor flags and the pad-grid ListSource hooks, all presentation-only and domain-neutral. Light domain: - Unchanged. ControlModule resolves subtrees generically through typeName(), so core carries no light-specific knowledge. UI: - Pad grid, rotary encoders and faders share one column track, so the three banks line up and still follow the pane as it is resized. - Pads are tinted by the roles they carry (layout/layer/driver/service), mixing hues when a preset carries several. Applying a preset claims only the roles it carries, so a layout preset and a layer preset stay lit at once. - Fixed: the seven-segment readouts and knob dials never redrew on a WebSocket patch (which fires neither input nor change), and were built before the input had a value or bounds. One redrawRangeDecorations call now owns the seam. - Power-on demo sweep, marked as one removable block plus a single call site. View-only: it never sends a value to the device. Tests: - 21 ControlModule tests, 7 FilesystemModule subtree tests. - Mutation-tested: removing the per-role rule fails 3 assertions, removing the path-traversal guard fails 7. - No scenario test for the preset round trip: the scenario runner has no op that can apply a preset (it speaks /api/control, applying needs /api/list/). Extending the runner is separate work. Docs/CI: - docs/moonmodules/core/control.md: catalog card plus the rules no header owns (what a preset carries, one-active-preset-per-role, applying is a rebuild). Reviews: - Path traversal via preset name -> fixed, validator on the control so every write path runs it; pinned and mutation-tested. - Preset list persisted then discarded -> fixed at the core seam (ListSource::persistsList) rather than locally, so Pins/Tasks can use it. - Save wrote the file without its slot then moved it, a second whole-folder rewrite that could displace an unrelated preset -> fixed by writing the slot up front. Exposed a real bug: an unaimed save landed on pad 1; added kNoSlot. - Duplicated fader binding -> driveFader now parses faderTarget, so the popup and the action cannot disagree. - Stale `order` naming in three comments -> renamed to `slot`. - "Apply runs on the HTTP thread, not the render tick" -> not applied. HttpServerModule::tick20ms is MM_NONBLOCKING and drains inside Scheduler::tick, so the docstring is correct. - 192-byte header read, insertion-sort struct copies -> deferred to the dynamic presets rework, along with kMaxPresets 36->64 (accepted by the PO for now). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds file-backed ChangesControlModule and platform updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 23
🤖 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/Control.h`:
- Around line 398-409: Update addText and addTextArea to use designated
initializers for the Control descriptors, explicitly naming the relevant members
such as var, name, type, bufSize, and validate. Remove the positional
false/nullptr values and rely on default initialization for unused aggregate
fields, while preserving each method’s existing behavior.
In `@src/core/ControlModule.h`:
- Around line 522-534: Update applyPreset around applySubtree and prepareTree to
persist the applied preset by marking each mutated module dirty and calling
FilesystemModule::noteDirty(). After the batch completes, trigger the existing
MoonModule schema-changed signal/hook so HttpServerModule performs
requestFullResync(), without coupling ControlModule directly to
HttpServerModule; preserve the current status reporting and return behavior.
- Around line 63-69: Update the grid-rendering comment above kGridCols,
kGridRows, and kMaxPresets to describe all kMaxPresets cells, or otherwise
reference kGridCols * kGridRows instead of the stale literal 36.
- Around line 109-110: Make the “slot” control non-persistable so saveSlot_
remains transient and kNoSlot is never written to or restored from flash, using
the existing transient-control mechanism. In savePreset, treat any value outside
the valid preset range as kNoSlot before selecting the target slot, preserving
assignFreeSlots for no-pad selections and preventing invalid API values from
becoming a real slot.
- Around line 537-555: Update renamePreset to detect whether the destination
preset already exists before calling fsWriteAtomic, and reject the operation
with the existing collision-reporting behavior instead of overwriting it.
Preserve the current rename flow for unused destination names, and follow the
collision handling principle already used by moveListRow.
- Around line 345-357: Update ControlModule::onEntry to skip preset files whose
stem length is at least sizeof(p.name), rather than truncating the name into
p.name. Only create a preset row when the complete filename stem fits,
preserving pathFor compatibility for all discovered entries.
- Around line 397-427: Update the slot persistence flow so a reorder writes only
presets whose slot values changed, rather than calling writeSlots for every
preset. In moveListRow, identify the moved preset and swapped occupant, then
persist each affected preset individually using the existing serialization and
atomic-write behavior; leave unchanged preset files untouched. Refactor
writeSlots or extract a single-preset helper as needed, preserving slot metadata
rewriting and cleanup.
In `@src/core/FilesystemModule.cpp`:
- Around line 350-357: The namespaced branch of FilesystemModule::saveSubtreeTo
must pass firstField=true to writeNode, matching the bare branch, because
savePreset already emits the sole separator before each subtree. In
src/core/ControlModule.h lines 467-475, retain the existing sink.append(",")
separator and add a strict JSON parsing test for saved preset output; no
separator change is needed there.
In `@src/ui/app.js`:
- Around line 2316-2338: Update the knob drag handlers around the pointerdown
listener so the existing up teardown also runs for pointercancel and
lostpointercapture. Ensure all termination paths remove the pointermove
listener, clear knob-turning, release capture when applicable, unregister the
end listeners, and dispatch the final change event only once.
- Around line 1505-1535: Extract the duplicated target-popup construction and
contextmenu/long-press listener setup from the encoder and fader branches into a
shared helper near this control-building logic. Have the helper accept the input
and control name/target context, then call it from both branches while
preserving the existing popup text and event behavior.
- Around line 2436-2459: Update the empty-cell creation logic in the
fixed/item-null branch to use a button element instead of a div, preserving its
existing drop-target behavior and styling. Add a primary click handler plus
keyboard activation for Enter and Space that call openPadEditor with the same
moduleName, ctrlName, null item, and slot index; retain context-menu and
long-press behavior as appropriate.
- Around line 2121-2141: Update the popup teardown around close, away, and the
document listeners so close() removes the popup and detaches both mousedown and
keydown listeners, matching away’s cleanup behavior. Ensure openPadEditor’s
save, overwrite, and delete paths use this shared teardown without leaving
listeners attached.
- Around line 2487-2493: The action payloads used by the pad click handler and
generic list button in src/ui/app.js (lines 2487-2493 and 2705-2720) must match
the corresponding test expectations in test/unit/core/unit_ControlModule.cpp.
Update both UI handlers and the tests to use the same payload, using "{}" if
that is the intended activate/apply body, while preserving the existing
listSetField flow.
- Around line 689-747: Add an early prefers-reduced-motion check at the
beginning of startSurfaceDemo, before checking or mutating surfaceDemoShownFor
or starting the animation, using the existing matchMedia browser API to return
immediately when reduced motion is requested. Preserve the current sweep
behavior for users who do not request reduced motion.
In `@src/ui/style.css`:
- Around line 940-947: Fix the Stylelint declaration-empty-line-before errors by
inserting a blank line before the padding declaration in .list-pad and before
the background declaration in .list-pad-active. Preserve all existing CSS values
and formatting otherwise.
- Around line 852-857: Update the .encoder-input styling to expose focus
feedback on its associated knob, using the existing :has() pattern because the
input follows the knob in the DOM. Add a visible focus-ring rule that activates
when the hidden encoder input is focused, while preserving its focusability and
current layout behavior.
- Around line 1665-1668: Consolidate the cursor declarations for the .knob
selector with its existing rules, removing the later duplicate cursor: grab
declaration so the intended ns-resize cursor is preserved. Keep the
.knob.knob-turning grabbing state, and group the drag/hover cursor styles with
the other .knob rules.
- Around line 1034-1055: Ensure the empty-cell hover styling in
.list-pad-empty:hover overrides the later generic .list-pad:hover rule by moving
it after the generic rule or increasing its specificity to
.list-pad.list-pad-empty:hover; preserve the quieter empty-cell background and
border colors.
In `@test/scenarios/light/scenario_peripheral_switch.json`:
- Line 257: Resolve the unsupported performance claim for the measure-i80-double
step by rerunning it alongside measure-i80-single on identical targets and runs,
then either add the appropriate supported performance or relative-bound
assertion or update the description near the measure-i80 scenario to state only
the non-freeze/normal-tick regression guard. Ensure the recorded values and
expectation are consistent.
In `@test/unit/core/unit_ControlModule.cpp`:
- Around line 37-62: Both fixtures derive temporary roots from
mm::platform::millis() and lack cleanup. In
test/unit/core/unit_ControlModule.cpp:37-62, update Device to use a monotonic
counter for unique roots and add a destructor that deletes the module tree and
removes the root directory; apply the same changes to Tree in
test/unit/core/unit_FilesystemModule_subtree.cpp:40-62, or use a shared helper
for both fixtures.
- Around line 73-82: Update the comment above setText to state that it sets the
named control's text value only, without claiming that it fires the change hook;
preserve the implementation and note that hook invocation is handled separately
by the tests.
- Around line 214-236: Update the fixture in “ControlModule skips a capture this
build does not have” so the capture names a module type that no build registers,
rather than “Drivers,” which the fixture/device provides. Keep the existing
assertions and valid Layers subtree, ensuring applyPreset reaches the
missing-module !m branch while still applying NoiseEffect and reporting the
skipped capture.
In `@test/unit/core/unit_FilesystemModule_subtree.cpp`:
- Around line 189-194: Update the applySubtree calls in
unit_FilesystemModule_subtree.cpp, including the cases at lines 121, 142, 158,
175, 189, 194, and 219, to assert their returned bool according to whether each
body is expected to be accepted or rejected. Preserve the existing tree-state
assertions while making corrupt and empty-body cases explicitly verify the
rejection result, following the corresponding ControlModule test.
🪄 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 Plus
Run ID: a5f03ff1-34f5-469d-9dd5-0c0ae42db909
⛔ Files ignored due to path filters (1)
docs/assets/core/ControlModule.pngis excluded by!**/*.png
📒 Files selected for processing (28)
docs/history/plans/Plan-20260731 - ControlModule and presets.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmkdocs.ymlsrc/core/Control.cppsrc/core/Control.hsrc/core/ControlModule.hsrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/main.cppsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_GridBlacks_blackpixel.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/scenarios/light/scenario_peripheral_grid_sweep.jsontest/scenarios/light/scenario_peripheral_switch.jsontest/unit/core/unit_ControlModule.cpptest/unit/core/unit_FilesystemModule_subtree.cpp
| "tick_us": [ | ||
| 4, | ||
| 15 | ||
| 31 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)scenario_peripheral_switch\.json$|test/scenarios|docs/moonmodules' || true
echo
echo "== file stats =="
wc -l test/scenarios/light/scenario_peripheral_switch.json
echo
echo "== relevant lines =="
sed -n '220,275p' test/scenarios/light/scenario_peripheral_switch.json | cat -n -v
echo
echo "== lines around 150-180 =="
sed -n '150,180p' test/scenarios/light/scenario_peripheral_switch.json | cat -n -v
echo
echo "== timing references =="
rg -n "tick_us|double-buffer|single|baseline|peripheral_switch\\)|observed\\.|at" test/scenarios/light/scenario_peripheral_switch.jsonRepository: MoonModules/projectMM
Length of output: 8932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all measure steps names =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path("test/scenarios/light/scenario_peripheral_switch.json")
tree = json.loads(p.read_text())
for i, step in enumerate(tree.get("steps", []), 1):
if step.get("op") == "measure" and "name" in step:
print(f"{i}: {step['name']}: {step.get('description', '')[:120]}")
PY
echo
echo "== measure-i80-single and measure-i80-double observed values =="
python3 - <<'PY'
import json
p = Path("test/scenarios/light/scenario_peripheral_switch.json")
tree = json.loads(p.read_text())
for idx, step in enumerate(tree.get("steps", []), 1):
if step.get("op") == "measure" and step.get("name") in ("measure-i80-single", "measure-i80-double"):
print(f"step {idx}: {step['name']}")
for platform, obs in step.get("observed", {}).items():
ticks.obs = obs.get("tick_us")
print(f" {platform}: tick_us={obs.get('tick_us')}, at={obs.get('at')}")
PY
echo
echo "== scenario runner handling for measure expectations =="
rg -n '"description": "i80 with double-buffer: the encode overlaps the wire, so the tick should be at or below the single-buffer baseline\."|tick should be at or below|baseline|measure": true|observed|run scenario|scenario runner|scenarios' test scenario test/scenarios --glob '*.ts' --glob '*.js' --glob '*.json' --glob '*.md' | head -200
echo
echo "== nearby moonmodules docs =="
sed -n '1,120p' docs/moonmodules/core/system.md | cat -n -v || trueRepository: MoonModules/projectMM
Length of output: 956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== measure-i80-single and measure-i80-double observed values =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path("test/scenarios/light/scenario_peripheral_switch.json")
tree = json.loads(p.read_text())
for idx, step in enumerate(tree.get("steps", []), 1):
if step.get("op") == "measure" and step.get("name") in ("measure-i80-single", "measure-i80-double"):
print(f"step {idx}: {step['name']}")
print(f" observed tick_us:")
for platform, obs in step.get("observed", {}).items():
print(f" {platform}: {obs.get('tick_us')}, at={obs.get('at')}")
PY
echo
echo "== benchmark invariant wording and nearby tests =="
rg -n "i80 with double-buffer: the encode overlaps the wire|tick should be at or below the single-buffer baseline|measure-i80-double|measure-i80-single" test docs --glob '*.ts' --glob '*.js' --glob '*.json' --glob '*.md' || true
echo
echo "== scenario runner handling for measure expectations =="
rg -n "scenario_peripheral_switch|measure-i80-double|baseline|tick should be at or below|run scenario|scenarios" scenario test test/scenarios docs --glob '*.ts' --glob '*.js' --glob '*.json' --glob '*.md' || trueRepository: MoonModules/projectMM
Length of output: 50377
Resolve the i80 double-buffer performance claim before publication.
measure-i80-double currently records 8,925 µs for esp32s3-n16r8 and 31 µs for desktop-macos, which are above the measure-i80-single baselines in this JSON. Rerun both steps on the same target and run; if this bound is required, add a supported performance contract or relative bound. If not, update the description at line 231 to match the non-freeze/normal-tick regression guard.
🤖 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_peripheral_switch.json` at line 257, Resolve
the unsupported performance claim for the measure-i80-double step by rerunning
it alongside measure-i80-single on identical targets and runs, then either add
the appropriate supported performance or relative-bound assertion or update the
description near the measure-i80 scenario to state only the
non-freeze/normal-tick regression guard. Ensure the recorded values and
expectation are consistent.
Sources: Coding guidelines, Learnings
A preset now captures exactly one thing: a look, or a geometry, or a hardware
setup, or a service configuration. Never a combination. Looks also reach Home
Assistant, where they appear in its own preset dropdown and can be applied from
the UI, a voice assistant or an automation.
Flash esp32s3-n16r8 1,656 KB (+6 KB), desktop 976 KB (+17 KB). Desktop tick reads
337 us but the sample was taken with an HTTP client attached; ControlModule has
no tick method and the hot-path gate passes.
Core:
- ControlModule: the four capture toggles become one `captures` Select, so the 16
representable combinations become 4 and the invalid ones are unrepresentable.
Defaults to Layers. Applying claims one role and leaves the other three, so a
layout preset and a look stay lit together.
- A preset file naming several subtrees (written by the previous build) is listed
but refused with a reason, so it can be seen and deleted rather than silently
vanishing.
- ControlModule stamps a revision whenever the preset set changes, exposed as the
WLED shim's `info.fs.pmt`. Home Assistant caches the preset list and re-fetches
only when that value moves; a constant left HA showing the list it read at setup
forever.
- Preset names are validated (printable ASCII, no / \ or .) — the name becomes a
file name, and ESP32's fsTranslate does no path normalization, so an unguarded
name could escape the preset folder via save, delete or rename.
- applySubtree marks the tree dirty: an applied preset rendered correctly and was
then lost on reboot, because the boot loader restored the config the apply never
updated.
- saveSubtreeTo passed firstField=false for a namespaced subtree while the caller
also emitted a separator, so every preset carrying more than one capture was
written as invalid JSON (",,"). Our own first-match reader tolerated it; a real
parser would not.
- ListSource::persistsList: a list whose rows are re-derived at setup is no longer
written to flash. The preset list was serialized on every save and discarded on
load.
- A preset filename longer than the name buffer was truncated, so pathFor then
addressed a different file — reachable by uploading through the File Manager.
Renaming onto an existing preset overwrote it and deleted the source.
- Save writes its slot into the file rather than fixing it up afterwards, which
removed a second whole-folder rewrite that could displace an unrelated preset.
Light domain:
- Drivers: `multicore` and `renderWait` are expert-only. Tuning knobs, not
settings.
UI:
- The capture checkboxes become a radio group; pad tint is a single role hue.
- Popup teardown detached only on click-away, so every save/delete leaked a
mousedown+keydown pair. A pointercancel left the knob turning after the gesture
ended. Empty pads were divs, so a keyboard user could not create a preset.
- The demo sweep respects prefers-reduced-motion and runs 1s rather than 3s.
Scripts/MoonDeck:
- run_desktop.py takes --port. Home Assistant's WLED integration connects on port
80 only (its host field rejects a port), so testing that path on desktop needs
`sudo uv run moondeck/run/run_desktop.py --port 80`.
- run_desktop.py picked the first executable path that existed, which served a
build a day older than `cmake --build build` produces; it now picks the newest.
Tests:
- 28 ControlModule tests. Three mutation-tested this session: the per-role rule,
the path-traversal guard, and the presets revision stamp.
- No scenario test for the preset round trip: the scenario runner speaks
/api/control only, and applying a preset needs /api/list/.
Docs/CI:
- control.md covers one-role presets and both Home Assistant paths, including
that the WLED integration is HA's native preset support while MQTT publishes the
same looks as effects (HA has no MQTT preset concept).
Reviews:
- CodeRabbit, 24 findings: fixed the invalid-JSON separator, the lost-on-reboot
apply, the filename truncation, the rename collision, the persisted-then-
discarded list, the slot clamp, the popup and pointer leaks, the keyboard
accessibility, four CSS ordering/duplication issues, and the designated
initializers. Declined one: the activate payload already matches (the UI passes
the value, the tests pass the body). Deferred the 192-byte header read and the
insertion-sort copies to the dynamic-presets rework.
- One finding exposed a vacuous test: "skips a capture this build does not have"
named a module the fixture provides, so it never reached the missing-module
branch.
SKIPPED GATE: "ESP32 firmware up to date" fails — no boards connected this
session, so the ESP32-affecting changes (HttpServerModule, MqttModule,
ControlModule, Drivers) are verified on desktop only. Needs a hardware check
before merge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/core/ControlModule.h (2)
500-534: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA new save can land on an already-occupied pad, putting two presets on one grid cell.
savePreset()writessaveSlot_into the new preset's file (Line 521) without checking whether another preset already holds that slot.moveListRowexplicitly swaps to avoid this class of collision (Line 337: "swap rather than overwrite"), butsavePreset()has no equivalent guard.This is reachable without any UI action:
slot,name,captures, andsaveare ordinary hidden controls, and the class doc states they are settable "from the popup, the API and persistence." A client that setsslotto an occupied value and then triggerssavecreates a second file claiming the same grid cell.assignFreeSlots()only reassigns presets withhasSlot == false(Line 393), so it does not detect or resolve two presets that both already declare the sameslot.🐛 Proposed fix — refuse a save onto an occupied slot held by a different preset
+ if (saveSlot_ < kMaxPresets) { + for (uint8_t i = 0; i < presetCount_; i++) { + if (presets_[i].slot == saveSlot_ && std::strcmp(presets_[i].name, name_) != 0) { + setStatusf(Severity::Warning, "slot %u is occupied by %s", + static_cast<unsigned>(saveSlot_), presets_[i].name); + return; + } + } + } if (captureRole_ >= kCaptureCount) { setStatusf(Severity::Warning, "choose what to capture"); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 500 - 534, Update savePreset() to check whether saveSlot_ is already assigned to a different existing preset before writing the new file. If the slot is occupied, refuse the save and report an appropriate status instead of persisting a duplicate slot; preserve the current behavior for unassigned slots and slots that are not selected.
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
kCaptureCountagainst drifting fromkCapturable/kCaptureRole.
kCaptureCountis a hand-maintained constant separate from thekCapturableandkCaptureRolearray literals. If either array ever grows without updatingkCaptureCount, every loop bounded bykCaptureCount(role lookup, capture serialization,writeListRow's role list) silently ignores the extra entries instead of failing to compile. The file already uses astatic_assertfor this exact class of risk at Lines 81-82 (kLayersRole).♻️ Proposed fix
static constexpr uint8_t kCaptureCount = 4; + static_assert(sizeof(kCapturable) / sizeof(kCapturable[0]) == kCaptureCount && + sizeof(kCaptureRole) / sizeof(kCaptureRole[0]) == kCaptureCount, + "kCaptureCount must match kCapturable/kCaptureRole length");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 73 - 78, Replace the hand-maintained kCaptureCount value with a compile-time size derived from kCapturable, and add a static_assert alongside the existing kLayersRole check to verify kCapturable and kCaptureRole have equal lengths. Keep the resulting count usable by the existing loops and role lookup code.src/core/FilesystemModule.cpp (2)
350-368: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the block comment to match the split between
saveSubtreeToandsaveSubtree.Lines 351-352 state "Returns true only when the file was written," but
saveSubtreeTo(Line 356) never touches a file — it writes into the caller'sJsonSinkand returnsfalseonly on an allocation failure (Line 366). The file-write contract belongs tosaveSubtree(Line 369). Leaving the two comments merged risks a future reader assumingsaveSubtreeTo's return value reflects a completed write.📝 Proposed fix
// ---- Save ---- -// Returns true only when the file was written. On failure (path/overflow/write -// error) the caller must keep the subtree dirty so the change isn't lost. // Serialize a subtree into a caller's sink. The write half of saveSubtree, split out so a caller // storing the bytes elsewhere (a named preset file) produces the SAME format the loader reads, // rather than a second serializer that could drift from this one. See the header. +// Returns false only on an allocation failure (sink.overflowed()); this function never touches a file. bool FilesystemModule::saveSubtreeTo(MoonModule* m, JsonSink& sink, const char* prefix) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/FilesystemModule.cpp` around lines 350 - 368, Update the comment immediately before saveSubtreeTo to describe serializing into the caller-provided JsonSink and returning false only when the sink overflows; move the file-written/dirty-subtree contract to the saveSubtree comment near that method. Keep the existing format and loader-compatibility documentation attached to saveSubtreeTo.
196-223: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersistence is now fixed; the WS resync gap from the same past comment remains open.
applySubtreenow callsm->markDirty()andnoteDirty()(Lines 220-221), so an applied preset survives a reboot. This resolves the persistence half of the earlier "An applied preset is never persisted" finding.The other half of that same finding is not addressed here.
applyNode(called at Line 209) creates, replaces, and removes children to match the JSON — a structural mutation of the live tree. The past comment noted that every other structural mutator inHttpServerModule(applyAddModule,handleDeleteModule,handleReplaceModule,handleMoveModule) ends by callingrequestFullResync()so connected WS clients do not patch against a stale leaf-hash baseline.applySubtreeperforms the same class of mutation but has no equivalent signal here, and none ofControlModule.h's callers (applyPreset) add one either.#!/bin/bash # Confirm whether applySubtree's structural changes reach HttpServerModule's resync signal. set -euo pipefail rg -n 'requestFullResync|setSchemaChangedHook|onSchemaChanged' -C4 src/core🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/FilesystemModule.cpp` around lines 196 - 223, Update the applySubtree flow in FilesystemModule::applySubtree to notify HttpServerModule after applyNode performs structural subtree changes, using the existing requestFullResync or schema-change hook mechanism rather than adding a separate signaling path. Ensure preset callers such as ControlModule::applyPreset result in a full WS resync while preserving the existing markDirty and noteDirty persistence behavior.
♻️ Duplicate comments (2)
src/core/ControlModule.h (2)
464-494: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winA single pad drag still rewrites every preset file.
moveListRow(Line 338) still callswriteSlots(), which loops over allpresetCount_presets and performs a read, a heap allocation, and anfsWriteAtomicfor each one. This code is unchanged from the prior review round: only the moved preset and the swapped occupant actually changed slot, so a full-grid drag still costs up to 64 file rewrites (128 flash operations viafsWriteAtomic's temp-file-and-rename) on a cold path that already blocks the render tick.♻️ Proposed fix — write only the presets whose slot changed
- void writeSlots() { - for (uint8_t i = 0; i < presetCount_; i++) { - char path[128]; - pathFor(presets_[i].name, path, sizeof(path)); + void writeSlot(const Preset& p) { + char path[128]; + pathFor(p.name, path, sizeof(path)); const long size = platform::fsSize(path); - if (size <= 0) continue; + if (size <= 0) return; char* body = static_cast<char*>(platform::alloc(static_cast<size_t>(size) + 1)); - if (!body) continue; + if (!body) return; const int n = platform::fsRead(path, body, static_cast<size_t>(size) + 1); if (n > 0) { body[n] = '\0'; JsonSink sink; - sink.appendf("{\"slot\":%u,", static_cast<unsigned>(presets_[i].slot)); + sink.appendf("{\"slot\":%u,", static_cast<unsigned>(p.slot)); // … unchanged … } platform::free(body); - } }Then in
moveListRow, replace the whole-folder rewrite:const uint8_t from = moving->slot; moving->slot = to; if (occupant) occupant->slot = from; // swap rather than overwrite - writeSlots(); + writeSlot(*moving); + if (occupant) writeSlot(*occupant); sortBySlot();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 464 - 494, Update the moveListRow flow and writeSlots implementation so a reorder persists only presets whose slot value changed, rather than rewriting every preset file. Track the moved preset and swapped occupant, then invoke the existing file-writing logic only for those affected presets while preserving slot metadata cleanup and atomic writes.
598-610: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA rename can still overwrite an existing zero-byte preset file.
The collision check uses
platform::fsSize(dst) > 0(Line 607). If a preset file atdstexists but is empty (0 bytes), this check does not detect it as "already exists," and the subsequent write silently replaces it. The intent stated in the adjacent comment ("Never overwrite another preset") calls for detecting existence, not just non-empty content.🐛 Proposed fix
- if (platform::fsSize(dst) > 0) { + if (platform::fsSize(dst) >= 0) { setStatusf(Severity::Warning, "%s already exists", to); return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 598 - 610, Update the collision check in renamePreset to detect whether the destination preset file exists, including zero-byte files, instead of relying on platform::fsSize(dst) > 0. Preserve the existing warning status and early return for any existing destination, so renaming never overwrites another preset.
🤖 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 `@moondeck/run/run_desktop.py`:
- Around line 45-52: Update the root-build candidate list in the executable
selection logic to include the Windows-suffixed path ROOT / "build" /
"projectMM.exe" alongside the existing unsuffixed candidate, so Windows builds
are considered when selecting the freshest executable.
In `@src/core/HttpServerModule.cpp`:
- Around line 1495-1497: Add a monotonically increasing preset-set revision to
ControlModule, incrementing it after every successful preset-set mutation,
including saves, deletes, and renames. Update the pmt assignment in the
HttpServerModule handler to report this revision instead of presetsModifiedS(),
while preserving the nonzero behavior. Add a regression test covering two
mutations performed within the same second.
In `@src/core/MqttModule.cpp`:
- Around line 77-90: Expose a monotonic preset revision from ControlModule and
increment it whenever presets are saved, renamed, or deleted, including the
existing rescan flow. In MqttModule::tick1s(), retain the last observed revision
and, when discovery is enabled, connected, and the revision changes, invoke
publishDiscovery(true) so buffers and retained discovery are refreshed. Add
coverage for live preset save, rename, and delete updates.
- Around line 826-834: Update publishState(false) to include a currentLook()
signature in its change-detection state alongside lastOn_, lastBri_, and
lastPalette_, so look-only changes publish updated ha/state effects. Capture the
look signature before the early-return check, and update it only after all MQTT
state publishes succeed; add a regression test applying two look-only presets
while Drivers values remain unchanged.
- Around line 186-204: Move the effect-list scratch storage out of
discoveryPayload_ and into non-overlapping storage such as discoveryBuf_ before
the final snprintf. Update the fxScratch capacity and writeHaEffectList call
accordingly, while keeping buildMqttPublish’s use of discoveryBuf_ safe by
ensuring the temporary effect data is consumed before that call.
In `@src/platform/desktop/main_desktop.cpp`:
- Around line 75-92: Update the argument parsing in main to use strtol’s end
pointer and reject any non-numeric trailing characters, while preserving
validation for ports outside 1..65535 and missing values. Reject unknown
arguments with an error instead of ignoring them, and add regression coverage
for valid, missing, non-numeric, trailing-character, and out-of-range --port
values.
In `@test/unit/core/unit_ControlModule.cpp`:
- Around line 806-826: Update the test case “ControlModule stamps a new revision
whenever the preset set changes” to replace both delayMs(1100) calls with
deterministic mm::platform::setTestNowMs() advances before each save/delete
expectation. Restore the test clock with setTestNowMs(0) after the case,
including on failure if the test framework supports cleanup.
---
Outside diff comments:
In `@src/core/ControlModule.h`:
- Around line 500-534: Update savePreset() to check whether saveSlot_ is already
assigned to a different existing preset before writing the new file. If the slot
is occupied, refuse the save and report an appropriate status instead of
persisting a duplicate slot; preserve the current behavior for unassigned slots
and slots that are not selected.
- Around line 73-78: Replace the hand-maintained kCaptureCount value with a
compile-time size derived from kCapturable, and add a static_assert alongside
the existing kLayersRole check to verify kCapturable and kCaptureRole have equal
lengths. Keep the resulting count usable by the existing loops and role lookup
code.
In `@src/core/FilesystemModule.cpp`:
- Around line 350-368: Update the comment immediately before saveSubtreeTo to
describe serializing into the caller-provided JsonSink and returning false only
when the sink overflows; move the file-written/dirty-subtree contract to the
saveSubtree comment near that method. Keep the existing format and
loader-compatibility documentation attached to saveSubtreeTo.
- Around line 196-223: Update the applySubtree flow in
FilesystemModule::applySubtree to notify HttpServerModule after applyNode
performs structural subtree changes, using the existing requestFullResync or
schema-change hook mechanism rather than adding a separate signaling path.
Ensure preset callers such as ControlModule::applyPreset result in a full WS
resync while preserving the existing markDirty and noteDirty persistence
behavior.
---
Duplicate comments:
In `@src/core/ControlModule.h`:
- Around line 464-494: Update the moveListRow flow and writeSlots implementation
so a reorder persists only presets whose slot value changed, rather than
rewriting every preset file. Track the moved preset and swapped occupant, then
invoke the existing file-writing logic only for those affected presets while
preserving slot metadata cleanup and atomic writes.
- Around line 598-610: Update the collision check in renamePreset to detect
whether the destination preset file exists, including zero-byte files, instead
of relying on platform::fsSize(dst) > 0. Preserve the existing warning status
and early return for any existing destination, so renaming never overwrites
another preset.
🪄 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 Plus
Run ID: 71613d6f-210b-42bc-a616-20576761a37a
📒 Files selected for processing (18)
docs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmoondeck/run/run_desktop.pysrc/core/Control.hsrc/core/ControlModule.hsrc/core/FilesystemModule.cppsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/light/drivers/Drivers.hsrc/main.cppsrc/platform/desktop/main_desktop.cppsrc/ui/app.jssrc/ui/style.csstest/unit/core/unit_ControlModule.cpptest/unit/core/unit_FilesystemModule_subtree.cpp
Comment on lines
+1495
to
+1497
| unsigned pmt = 1; | ||
| if (auto* control = static_cast<ControlModule*>(findModuleByName("Control"))) | ||
| pmt = static_cast<unsigned>(control->presetsModifiedS()) + 1; // +1: never report 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a monotonic preset revision for pmt.
ControlModule::rescan() stores platform::millis() / 1000u. Two saves, deletes, or renames in the same second produce the same value. Home Assistant then keeps its previous /presets.json result.
Add a monotonically increasing preset-set revision in ControlModule. Increment it after each successful preset-set mutation. Report that revision here instead of the second-resolution timestamp. Add a regression test with two mutations in one second.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/HttpServerModule.cpp` around lines 1495 - 1497, Add a monotonically
increasing preset-set revision to ControlModule, incrementing it after every
successful preset-set mutation, including saves, deletes, and renames. Update
the pmt assignment in the HttpServerModule handler to report this revision
instead of presetsModifiedS(), while preserving the nonzero behavior. Add a
regression test covering two mutations performed within the same second.
Home Assistant now sees a preset saved, renamed or deleted while it is connected, instead of keeping the list it read at setup. Preset pads refuse to overwrite each other, and a preset applied from HA reaches every open browser. Separately, the first power-function groundwork lands: a 16-bit math tier and a golden-frame harness that pins what the effects render today, so the coming migration can prove it changes nothing. Flash desktop 982 KB (+6 KB); desktop tick 125 us (the 337 us in the previous commit was sampled with an HTTP client attached, not a regression). Core: - ControlModule reports a monotonic preset revision instead of a seconds-resolution stamp: two changes inside one second were indistinguishable, so a consumer caching on it missed the second one. Drives both the WLED shim's info.fs.pmt and MQTT's re-announce. - MqttModule re-announces discovery when that revision moves, so a mid-session preset reaches Home Assistant without a reconnect; and the ha/state change gate now includes the applied look, which alters neither on, brightness nor palette and so never published. - The HA effect-list scratch moved out of discoveryPayload_: it sat at a fixed offset inside the buffer snprintf was writing, and the fixed prefix can grow past that offset and trample the list mid-format. - Saving a different preset onto an occupied pad is refused with the holder's name; saving over the same name is unchanged. Rename now treats a zero-byte destination as a collision (fsSize returns 0 for an existing empty file, -1 for a missing one). - A reorder rewrites only the presets whose slot changed, not every file. - applySubtree fires the existing schema-changed hook, so a preset applied with no HTTP request in flight (HA over MQTT or the WLED shim) still reaches open browsers. - ModuleFactory::registerType is idempotent by name. It never deduped, so each test fixture construction re-registered its types until the uint8_t capacity saturated and every later registration in the run failed. - New core/math16.h: the 16-bit contract tier for the power functions -- sin16/cos16, map32, and BeatPhase (the BPM accumulator nine effects hand-roll, which freezes when the frame time rounds to zero). sin16 uses a 130-byte quarter-wave table: interpolating the existing 8-bit LUT was implemented first and rejected on measurement at 1.1% error, worse than the 0.69% it was meant to beat; the table measures 0.031%. Light domain: - Drivers: multicore and renderWait are expert-only. Tuning knobs, not settings. Scripts/MoonDeck: - run_desktop.py takes --port (Home Assistant's WLED integration hardcodes port 80 and its host field rejects a port, so testing that path on desktop needs `sudo ... --port 80`), and now picks the newest executable rather than the first path that exists -- it was serving a build a day older than `cmake --build build` produces. - The desktop binary rejects a non-numeric or trailing-garbage --port and unknown arguments, instead of silently running on the default. Tests: - Golden-frame harness: renders an effect at a fixed clock and hashes the frame, so "renders exactly the same" is proved rather than asserted. Ten baselines captured from the current code and verified reproducible across runs; hashes, not frame blobs, so repo size stays flat. - unit_math16: sin16 smoothness between LUT entries (the property large fixtures need), the 0.5% accuracy bound, map32's fencepost, and BeatPhase under sub-millisecond frames and the millis wrap. - New MQTT rig covering live preset save and look-only state publishes. Docs/CI: - Power-function analysis, bottom-up and top-down: the primitive catalog with its prior art, and the build spec (homes, types, migration order, resource accounting, eleven showcase effects). A canon survey found one structural gap -- no way to read the framebuffer as a texture at a transformed coordinate, which is about a third of the classic effect canon. - architecture.md drops "concrete first, abstract later" (removed from CLAUDE.md earlier); the four backlog files citing it now stand on their own rationale. - ADRs are documented as immutable except the status line, so a superseded decision gets a dated pointer instead of the convention being folklore. Reviews: - CodeRabbit, 11 findings: fixed the scratch-buffer overlap, the revision resolution, the MQTT re-announce and state gate, the slot and rename collisions, the per-preset slot write, the resync hook, --port validation and the Windows path. The delayMs-based revision test was replaced by a counter, which made the suggested test-clock fix unnecessary. - Skipped: regression tests for --port parsing -- main() is not linkable into the unit binary, and the parsing is ten lines validated by inspection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/core/HttpServerModule.cpp (1)
1557-1567: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccept standalone
pscommands on the WLED WebSocket path.
applyWledStatenow supportsps, butpollWledStateFromWebSockets()calls it only when the frame containsonorbri. A WLED client that sends{"ps":N}alone drops the preset request.Include
psin the WebSocket ingress predicate. Add a regression test for a masked WebSocket frame that contains onlyps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/HttpServerModule.cpp` around lines 1557 - 1567, The WebSocket ingress predicate in pollWledStateFromWebSockets must invoke applyWledState for frames containing only ps, not just on or bri. Extend that predicate to recognize ps and add a regression test covering a masked WebSocket frame with a standalone ps command.src/core/ControlModule.h (3)
639-644: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not report a rename after source deletion fails.
platform::fsRemove(src)is ignored. If it fails afterfsWriteAtomic(dst, ...)succeeds, both preset files remain but this method returnstrue. The HTTP list operation then reports success for a rename that created a copy.Check the source removal result. If it fails, remove the new destination when possible and return failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 639 - 644, Update the rename flow around the source-removal call in ControlModule so it checks the result of platform::fsRemove(src) before setting ok to true. If source deletion fails after fsWriteAtomic succeeds, attempt to remove the newly created destination when possible, keep the operation failed, and preserve the existing cleanup and rescan behavior.
382-391: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRefresh the preset list after File Manager changes.
The preset grid is rebuilt only at setup and after ControlModule operations. A preset uploaded through the File Manager does not appear until another ControlModule operation or a reboot. This conflicts with the documented upload behavior.
src/core/ControlModule.h#L382-L391: add a core-neutral filesystem-change notification or an explicit live refresh path that callsrescan()after preset-folder uploads, deletes, and renames.docs/moonmodules/core/control.md#L27-L29: retain this statement only after the live refresh behavior exists. Otherwise document the required refresh step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 382 - 391, The preset list must refresh immediately after File Manager uploads, deletes, and renames. Add a core-neutral filesystem-change notification or explicit live refresh path connected to ControlModule::rescan() for changes in the preset folder; update docs/moonmodules/core/control.md lines 27-29 to retain the documented behavior only if live refresh is implemented, otherwise document the required manual refresh step.
278-287: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep active-role state consistent with preset file mutations.
current_stores preset names. Deleting or renaming an active preset leaves its old name active. MQTT can then publish an effect that no longer exists, and WLED cannot resolve the active preset slot.
src/core/ControlModule.h#L278-L287: after a successful delete, clear everycurrent_entry equal to the deleted name.src/core/ControlModule.h#L619-L645: after a successful rename, replace everycurrent_entry equal tofromwithto.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ControlModule.h` around lines 278 - 287, Keep active-role state synchronized with preset mutations in ControlModule: in deleteListRow, after a successful fsRemove, clear every current_ entry matching the deleted preset name; in the rename flow around lines 619-645, after a successful rename, replace every current_ entry matching from with to. Apply the required changes at both listed sites in src/core/ControlModule.h (278-287 and 619-645).moondeck/run/run_desktop.py (1)
49-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict executable candidates to the current host.
The resolver now selects the newest path from both
.exeand unsuffixed candidates. If a stale artifact from another host remains inbuild, the launcher can select an incompatible binary and fail to start or run the wrong build. Filter candidates by the host-specific suffix before callingmax(...).Proposed fix
- existing = [c for c in candidates if c.exists()] + suffix = ".exe" if sys.platform == "win32" else "" + existing = [ + c for c in candidates + if c.suffix == suffix and c.is_file() + ]🤖 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 `@moondeck/run/run_desktop.py` around lines 49 - 53, Update the executable candidate selection in the resolver to retain only paths matching the current host’s executable suffix before evaluating existence and calling max. Preserve the newest-existing-candidate behavior while excluding incompatible .exe or unsuffixed artifacts from other hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backlog/power-functions-analysis-bottom-up.md`:
- Line 3: Update the introductory paragraph in the power-functions analysis
document to remove the stale “to be written” wording and state that the top-down
companion already exists as the implementation specification, preserving the
surrounding description and link context.
- Line 138: Clarify the Stage 2 scope for fillTriangle across the candidate
table, gather-gap entry, filled-polygons cut line, and VectorBallsEffect
showcase reference. If triangles are included, list fillTriangle explicitly and
distinguish it from deferred general polygon fill; otherwise remove the showcase
reference and keep the deferred-scope statements consistent.
- Line 16: Reconcile the documented scope counts in the TL;DR, synthesis, and
repeated summary: update the stale “~30-function” and “eight families”
references to consistently reflect the defined ~34 functions across families
1–9, including Projection. Ensure all affected statements use the same totals.
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Line 44: Update the particle API example’s code fence in the documentation to
specify the cpp language tag, changing the untyped opening fence to a cpp-tagged
fence so Markdownlint MD040 passes.
In `@src/core/ControlModule.h`:
- Around line 341-345: Update the reorder method surrounding writeSlot(*moving)
and writeSlot(*occupant) so writeSlot returns whether each file write persisted
successfully. Only increment presetsRevision_ and report success after every
affected write succeeds; on failure, restore the original in-memory slot
assignments and use a recoverable two-file swap strategy that avoids duplicate
slot claims after a partial write.
In `@src/core/math16.h`:
- Around line 82-88: Update map32 to widen operands before subtracting, avoiding
int32_t overflow, and replace the intermediate int64_t multiplication with an
overflow-safe multiply-divide approach that handles full-width 32-bit input and
output spans. Preserve clamping and zero-span behavior, and add regression
coverage for INT32_MIN/INT32_MAX combinations across input and output ranges;
invalid or unrepresentable cases must degrade visibly rather than crash.
- Around line 52-88: Mark the `sin16` and `map32` function declarations as
`constexpr` so their existing integer-only implementations can be evaluated at
compile time under C++20. Leave `cos16` unchanged since it already delegates to
`sin16`.
In `@test/unit/core/unit_math16.cpp`:
- Around line 13-14: Update unit_math16.cpp to include <algorithm> for std::max
and replace the implementation-defined M_PI usage with the portable C++20
std::numbers::pi_v<double> from <numbers> (or an equivalent local constexpr
value), including the repeated usage around the referenced lines.
In `@test/unit/core/unit_MqttModule.cpp`:
- Around line 311-333: Update PresetRig’s destructor to reset the platform
filesystem-root override before or while tearing down the fixture, then remove
root_. Ensure later tests no longer retain platform::fsSetRoot(root_) after
PresetRig ends.
---
Outside diff comments:
In `@moondeck/run/run_desktop.py`:
- Around line 49-53: Update the executable candidate selection in the resolver
to retain only paths matching the current host’s executable suffix before
evaluating existence and calling max. Preserve the newest-existing-candidate
behavior while excluding incompatible .exe or unsuffixed artifacts from other
hosts.
In `@src/core/ControlModule.h`:
- Around line 639-644: Update the rename flow around the source-removal call in
ControlModule so it checks the result of platform::fsRemove(src) before setting
ok to true. If source deletion fails after fsWriteAtomic succeeds, attempt to
remove the newly created destination when possible, keep the operation failed,
and preserve the existing cleanup and rescan behavior.
- Around line 382-391: The preset list must refresh immediately after File
Manager uploads, deletes, and renames. Add a core-neutral filesystem-change
notification or explicit live refresh path connected to ControlModule::rescan()
for changes in the preset folder; update docs/moonmodules/core/control.md lines
27-29 to retain the documented behavior only if live refresh is implemented,
otherwise document the required manual refresh step.
- Around line 278-287: Keep active-role state synchronized with preset mutations
in ControlModule: in deleteListRow, after a successful fsRemove, clear every
current_ entry matching the deleted preset name; in the rename flow around lines
619-645, after a successful rename, replace every current_ entry matching from
with to. Apply the required changes at both listed sites in
src/core/ControlModule.h (278-287 and 619-645).
In `@src/core/HttpServerModule.cpp`:
- Around line 1557-1567: The WebSocket ingress predicate in
pollWledStateFromWebSockets must invoke applyWledState for frames containing
only ps, not just on or bri. Extend that predicate to recognize ps and add a
regression test covering a masked WebSocket frame with a standalone ps command.
🪄 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: 22131fa5-8b3c-4797-8ac3-85d896892434
📒 Files selected for processing (28)
CLAUDE.mddocs/adr/README.mddocs/architecture.mddocs/backlog/backlog-core.mddocs/backlog/power-functions-analysis-bottom-up.mddocs/backlog/power-functions-analysis-top-down.mddocs/backlog/rename-to-moonlight.mddocs/backlog/system-modules.mddocs/backlog/ui-extensibility-analysis-bottom-up.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmoondeck/run/run_desktop.pysrc/core/ControlModule.hsrc/core/FilesystemModule.cppsrc/core/HttpServerModule.cppsrc/core/ModuleFactory.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/core/math16.hsrc/platform/desktop/main_desktop.cpptest/CMakeLists.txttest/scenarios/light/scenario_peripheral_grid_sweep.jsontest/unit/core/unit_ControlModule.cpptest/unit/core/unit_MqttModule.cpptest/unit/core/unit_math16.cpptest/unit/light/golden_frame.htest/unit/light/unit_Effects_golden.cpp
| |---|---|---|---| | ||
| | 1 | **Frame ops** | `fill` *(have)*, `fade` *(have)*, `blur` *(have — already dimension-generic)*, `scroll(axis, delta, wrap)` | WLED #6/#8/#9; FreqMatrix's hand-rolled shift | | ||
| | 2 | **Pixel ops** | `pixel`/`get`/`addPixel`/`blendPixel` *(have)*, **`splat(fx, fy, c)`** — the Wu sub-pixel writer, 12.4 or 16.16 coords | WLED-PS renderer; ParticlesEffect's private 12.4 math; "modern motion" on coarse matrices | | ||
| | 3 | **Geometry** | `line` *(have)*, `lineAA` (Wu 1991), `circle`/`fillCircle` (midpoint), `rect`/`fillRect`/`bar` (the audio-meter staple), `text` *(have)*; **SDF trio** `sdCircle/sdBox/sdSegment` + `smin` + coverage-AA | 4 effects hand-roll bars; SDFs subsume metaballs/glow/outline | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clarify whether fillTriangle is in scope.
The candidate table at Line 138 omits fillTriangle, the gather gap adds it at Line 162, and Line 172 places filled polygons below the cut. The companion top-down spec uses fillTriangle in VectorBallsEffect at Line 102. If triangles are in Stage 2, list them in the candidate set and distinguish them from deferred general polygon fill. Otherwise, remove the top-down showcase reference.
Also applies to: 162-162, 172-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/backlog/power-functions-analysis-bottom-up.md` at line 138, Clarify the
Stage 2 scope for fillTriangle across the candidate table, gather-gap entry,
filled-polygons cut line, and VectorBallsEffect showcase reference. If triangles
are included, list fillTriangle explicitly and distinguish it from deferred
general polygon fill; otherwise remove the showcase reference and keep the
deferred-scope statements consistent.
Comment on lines
+82
to
+88
| inline int32_t map32(int32_t v, int32_t inLo, int32_t inHi, int32_t outLo, int32_t outHi) { | ||
| if (inHi == inLo) return outLo; // zero span: no meaningful ratio | ||
| if (inHi > inLo) { if (v <= inLo) return outLo; if (v >= inHi) return outHi; } | ||
| else { if (v >= inLo) return outLo; if (v <= inHi) return outHi; } | ||
| const int64_t num = static_cast<int64_t>(v - inLo) * (outHi - outLo); | ||
| return static_cast<int32_t>(outLo + num / (inHi - inLo)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent signed overflow in map32.
Line 86 subtracts int32_t values before the cast to int64_t. For example, v == INT32_MAX and inLo == INT32_MIN invokes signed overflow.
Widen operands before subtraction. Also use a safe multiply-divide implementation for full-width ranges, because two valid 32-bit spans can exceed int64_t when multiplied. Add INT32_MIN and INT32_MAX regression cases.
As per path instructions, “For any input, order, or size, degrade visibly rather than crash.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/math16.h` around lines 82 - 88, Update map32 to widen operands
before subtracting, avoiding int32_t overflow, and replace the intermediate
int64_t multiplication with an overflow-safe multiply-divide approach that
handles full-width 32-bit input and output spans. Preserve clamping and
zero-span behavior, and add regression coverage for INT32_MIN/INT32_MAX
combinations across input and output ranges; invalid or unrepresentable cases
must degrade visibly rather than crash.
Source: Path instructions
…uard Ten effects stop hand-rolling the same two things. Nine copies of a BPM accumulator become one shared BeatPhase, five copies of an integer range map become one map32, and a new golden-frame test proves the rewrites render the same frames as before. Five effects also stop starting at a random point in their animation depending on how long the device had been running. Flash esp32s3-n16r8 1,664 KB (+2 KB), desktop 982 KB (+0 KB); desktop tick 132 us (+7 us, within the scenario contracts' margin). Core: - core/math16.h: the 16-bit tier the power-function contract is written in. sin16/cos16 (a 130-byte quarter-wave table plus interpolation: 0.031% error against 0.69% for FastLED's classic sin16 -- an 8-bit-table variant was built first and rejected on measurement at 1.1%), map32, and BeatPhase. - map32 widens every operand before subtracting: a full-width int32 range (INT32_MIN..INT32_MAX) overflowed the span, which would misplace pixels silently rather than crash. sin16 and map32 are constexpr. - draw::Canvas binds a buffer to the dimensions that address it, so the two can no longer disagree, and applies the depth guard that sixteen effects each carry a private copy of. Passed BY VALUE deliberately: measured 62 instructions in a per-pixel fill loop against 67 for today's separate arguments and 69 for a const reference, which forces the extents out of registers. - ControlModule: a deleted preset no longer keeps its pad lit, and a renamed one follows its new name (the active-role slots track presets by name). A failed source removal during rename now rolls back instead of leaving the preset visible twice, and a reorder is all-or-nothing. - The WLED WebSocket path accepts a frame carrying only `ps`: choosing a preset worked over HTTP and did nothing over the socket. Light domain: - All nine BPM accumulators now use BeatPhase; five imap copies now use map32. Two effects (Noise, DistortionWaves) exercise the scaled forms -- one reads a single accumulator at two scales, which is why phase() takes the scale at the read rather than baking it in. - StarSky migrated to Canvas as the pilot, its private depthDim() deleted. Tests: - Golden-frame harness with 11 baselines. It renders 200 frames, not 8: at a typical default speed a short render moves nothing by a whole pixel, so the first version passed even with an effect's phase perturbed 7x. Found by mutation-testing the harness itself. - Five goldens moved, all for one reason: those effects added `now * bpm` on their first tick, so their startup phase depended on device uptime. Wave and Noise ALREADY had that guard and their goldens did NOT move -- two control cases proving a moved hash means "this effect gained the guard", not "the migration drifted". - unit_Canvas (9 tests incl. byte-for-byte equivalence with the legacy form) and unit_math16 (smoothness, accuracy bound, full-width ranges, the millis wrap). Docs/CI: - The power-function documents gain: a dimension audit (five 2D-primary primitives named with their generalisation paths), a determinism section for the planned supersync (pure functions of position/time/seed; BeatPhase already qualifies, the PRNG stream does not), the measured Canvas trade-off, resource accounting, and what the migration has and has not extracted so far. - Nine July friend-repo digests, including a new one: hpwit/new-parser is ESPLiveScript2, a from-scratch rewrite whose stated goal is a verifiable compiler (host builds, QEMU running the actual compiled bytes). Our livescripts analysis is flagged superseded-upstream because it reads v1. - Backlogged: a filesystem-change notification, so a preset uploaded through the File Manager appears without waiting for the next rescan. Reviews: - CodeRabbit, 11 findings: the map32 overflow, the three ControlModule state bugs above, the WebSocket ps gate, constexpr, host-suffix filtering in run_desktop.py, portable pi in the tests, and the MQTT rig restoring the global filesystem root. Skipped: --port parsing tests (main() is not linkable into the unit binary). - LavaLamp and Metaballs saturate their field to full brightness at their defaults, so their frames barely vary and their goldens cannot detect a phase error -- found by mutation, recorded in the harness header, and left for the effect-tuning pass rather than changed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/backlog/power-functions-analysis-top-down.md (3)
172-175: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExpand scenario-test requirements.
Lines 172-175 require unit tests for every power function but add only one scenario for the particle kernel. Add scenario coverage for the other user-visible families, or document an approved scope exception before implementation.
As per coding guidelines, “Every behavior must be covered by meaningful unit and scenario tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/power-functions-analysis-top-down.md` around lines 172 - 175, Expand the scenario-test requirement in the documentation beyond the single particle-effects scenario to cover each other user-visible power-function family. If any family is intentionally excluded, document an explicitly approved scope exception before implementation, while retaining meaningful unit and scenario coverage for all included behaviors.Source: Coding guidelines
106-106: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftResolve the per-pixel floating-point exception.
Line 106 permits a float raymarch loop in
RaymarchEffectand claims ESP32 support. Line 40 says per-light floating point is not allowed. Keep the effect desktop-only, convert the hot loop to the fixed-point contract, or record an approved small-fixture exception with measured limits. Line 121 repeats the same support claim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/power-functions-analysis-top-down.md` at line 106, Resolve the contradiction between RaymarchEffect’s ESP32 support claim and the floating-point raymarch loop: make the effect desktop-only, convert its hot loop to the project’s fixed-point contract, or document an approved small-fixture exception with measured limits. Update both the RaymarchEffect entry and the repeated support claim at line 121 so they consistently reflect the chosen behavior.
9-9: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the
Canvasmutability contract consistent.Line 9 defines
draw::Canvas{buf, dims, cpl}withEffectBase::canvas()returningconst Canvas&, while line 24 still specifiesCanvas&as the first argument. Choose one signature for the spec and update the other. If the API keeps a constCanvas&while the underlying buffer is still writable, document that rule explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/power-functions-analysis-top-down.md` at line 9, Make the Canvas mutability contract consistent throughout the specification: align the signature described around the existing Canvas argument with EffectBase::canvas() returning const Canvas&, or update both references to the selected alternative. If retaining const Canvas&, explicitly state that the buffer remains writable through the Canvas API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Around line 141-143: Define a shared supersync time origin in the document,
including the epoch or absolute timestamp and the quantization used by
BeatPhase, hashInt, and stateful-kernel reseeding. Update the claims around
BeatPhase and the decision at the referenced conclusion so cross-device
agreement requires this shared origin, not merely matching local elapsed-time
state.
In `@docs/history/hpwit-I2SClocklessVirtualLedDriver.md`:
- Line 11: Align the branch scope in the audit description with the documented
branches: update the file description to include dev and optomize, or remove
those branches from the audit statement on line 11. Keep the listed branches
identical in both places.
In `@docs/history/MoonModules-WLED-MM.md`:
- Line 13: Update the audit statement on the line containing the issue searches
so it either adds a reproducible updated/comment-activity query covering the
same date range, or removes the unsupported “commented on” claim while
preserving the created and closed results.
In `@docs/history/PlummersSoftwareLLC-NightDriverStrip.md`:
- Line 17: Update the file introduction’s release scope statement to reference
v2.0.0 and v2.0.1 as the latest June 2026 releases, replacing the outdated
v1.3.0-only January reference. Keep the surrounding July auditability details
unchanged.
In `@docs/moonmodules/core/control.md`:
- Line 27: Correct the rescan description to match ControlModule::moveListRow:
remove reorder from the operations that trigger a rescan, unless the
implementation is changed to call rescan() after a successful reorder. Keep the
documentation in present tense.
In `@moondeck/run/run_desktop.py`:
- Around line 54-57: Update the candidate selection around existing so mtime
lookup tolerates candidates disappearing after exists() succeeds: collect valid
(path, mtime) pairs while catching FileNotFoundError from c.stat(), then select
the newest pair with max and preserve the normal missing-executable behavior
when none remain.
In `@src/light/effects/GEQ3DEffect.h`:
- Line 81: In the rendering method containing the NUM_BANDS calculation, return
immediately when cols or rows is non-positive before computing NUM_BANDS or
performing palette/geometry work. Add regression coverage for both zero-width
and zero-height layers, ensuring each degrades without crashing.
---
Outside diff comments:
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Around line 172-175: Expand the scenario-test requirement in the documentation
beyond the single particle-effects scenario to cover each other user-visible
power-function family. If any family is intentionally excluded, document an
explicitly approved scope exception before implementation, while retaining
meaningful unit and scenario coverage for all included behaviors.
- Line 106: Resolve the contradiction between RaymarchEffect’s ESP32 support
claim and the floating-point raymarch loop: make the effect desktop-only,
convert its hot loop to the project’s fixed-point contract, or document an
approved small-fixture exception with measured limits. Update both the
RaymarchEffect entry and the repeated support claim at line 121 so they
consistently reflect the chosen behavior.
- Line 9: Make the Canvas mutability contract consistent throughout the
specification: align the signature described around the existing Canvas argument
with EffectBase::canvas() returning const Canvas&, or update both references to
the selected alternative. If retaining const Canvas&, explicitly state that the
buffer remains writable through the Canvas API.
🪄 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: e60bad45-5f52-4789-91d2-689e78e9198f
📒 Files selected for processing (46)
docs/backlog/backlog-core.mddocs/backlog/livescripts-analysis-bottom-up.mddocs/backlog/power-functions-analysis-bottom-up.mddocs/backlog/power-functions-analysis-top-down.mddocs/history/FastLED-FastLED.mddocs/history/MoonModules-WLED-MM.mddocs/history/PlummersSoftwareLLC-NightDriverStrip.mddocs/history/README.mddocs/history/hpwit-ESPLiveScript.mddocs/history/hpwit-I2SClocklessLedDriver.mddocs/history/hpwit-I2SClocklessVirtualLedDriver.mddocs/history/hpwit-new-parser.mddocs/history/troyhacks-WLED.mddocs/history/wled-WLED.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmoondeck/run/run_desktop.pysrc/core/ControlModule.hsrc/core/HttpServerModule.cppsrc/core/math16.hsrc/light/draw.hsrc/light/effects/DistortionWavesEffect.hsrc/light/effects/EffectBase.hsrc/light/effects/FreqMatrixEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/LavaLampEffect.hsrc/light/effects/MetaballsEffect.hsrc/light/effects/NoiseEffect.hsrc/light/effects/PlasmaEffect.hsrc/light/effects/SineEffect.hsrc/light/effects/SpiralEffect.hsrc/light/effects/StarFieldEffect.hsrc/light/effects/StarSkyEffect.hsrc/light/effects/WaveEffect.hsrc/light/layers/Layer.htest/CMakeLists.txttest/scenarios/light/scenario_peripheral_grid_sweep.jsontest/unit/core/unit_ControlModule.cpptest/unit/core/unit_MqttModule.cpptest/unit/core/unit_math16.cpptest/unit/light/golden_frame.htest/unit/light/unit_Canvas.cpptest/unit/light/unit_Effects_golden.cpp
Comment on lines
+141
to
+143
| - **Time, never frame count.** `BeatPhase` already satisfies this — it integrates `elapsed()`, so a device that drops frames still arrives at the same phase. This is the property that makes the nine-accumulator migration *more* than tidying: each hand-rolled copy also added `now * bpm` on its first tick, so its phase depended on device uptime and two devices could never agree. That is removed by construction (verified: it is the sole cause of the one golden that moved). | ||
| - **Position-addressable randomness beside the stream.** `Random8` advances per *call*, so a device that renders one extra frame — or a different light count — desynchronizes permanently and never recovers. `hashInt(x, y, t, seed)` (identified in the canon survey as the dissolve-transition primitive) is the supersync form: ask "what is this pixel's random value" rather than "what is next in the stream". Both ship; the hash form is the default for anything a synced effect uses, the stream stays for effects that are legitimately local. | ||
| - **Stateful kernels declare a resync point.** Particles, ripple, fire and CA carry evolving state that cannot be recomputed from time alone; a lost or late device cannot silently drift. Each exposes a deterministic re-seed from (time, seed) so a joining device can be placed into the same state — the same "keyframe" idea lockstep networking uses. Their *inputs* (emitters, forces) stay pure so only the state needs syncing, not the physics. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define a shared time origin for supersync.
Line 141 says that BeatPhase integrates local elapsed() and gives two devices the same phase. Local elapsed time only gives repeatability to instances with the same start state. Devices that start at different times can have different phases at the same wall-clock time. Define a shared epoch, absolute timestamp, or phase seed. Also define time quantization for hashInt and stateful re-seeding. Update the decision at Line 186 to match.
Also applies to: 186-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/backlog/power-functions-analysis-top-down.md` around lines 141 - 143,
Define a shared supersync time origin in the document, including the epoch or
absolute timestamp and the quantization used by BeatPhase, hashInt, and
stateful-kernel reseeding. Update the claims around BeatPhase and the decision
at the referenced conclusion so cross-device agreement requires this shared
origin, not merely matching local elapsed-time state.
| // horizon is a Y row used as the vanishing point's y; clamp the 0..255 control to the grid. | ||
| const int hzn = horizon < rows ? horizon : rows - 1; | ||
| const int split = imap(projector, 0, cols, 0, NUM_BANDS - 1); | ||
| const int split = map32(projector, 0, cols, 0, NUM_BANDS - 1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return before rendering a zero-sized grid.
If cols is zero, NUM_BANDS becomes zero. The later palette and geometry calculations divide by NUM_BANDS, which can trap.
Add if (cols <= 0 || rows <= 0) return; before calculating NUM_BANDS. Add a regression test for zero-width and zero-height layers.
As per coding guidelines, inputs “of any value, order, or size must degrade visibly rather than crash.”
🤖 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/effects/GEQ3DEffect.h` at line 81, In the rendering method
containing the NUM_BANDS calculation, return immediately when cols or rows is
non-positive before computing NUM_BANDS or performing palette/geometry work. Add
regression coverage for both zero-width and zero-height layers, ensuring each
degrades without crashing.
Source: Coding guidelines
Every effect now takes its drawing surface as one value instead of assembling a buffer and a set of dimensions itself, and the checks about whether a frame should run at all moved to the Layer that owns that decision. Two effects that refused to draw on mono or two-channel fixtures now render on them. Flash esp32s3-n16r8 1,665 KB (+0 KB), desktop 998 KB (+16 KB). Desktop tick reads 147 us in the metrics file, measured while the test instance was still running; 125 us with it stopped, which is flat against the previous commit. Core: - draw.h gains Canvas forms of line, fade, fill, blur, blendPixel, addPixel, glyph, text and offsetOf, so an effect never has to fall back to the older (Buffer&, dims) pair mid-migration. - Layer::tick() returns before running any child when an extent is zero or the buffer holds no lights. That decision belongs to the Layer, and having it in one place is what let 13 copies of it come out of the effects. - Layer::setChannelsPerLight rejects zero. A light with no channels would allocate a zero-byte buffer and give every effect a stride of zero; the invariant is now enforced where the value enters rather than defended at each use. Light domain: - 23 effects migrated to Canvas: the three-line preamble is one line, and the private depthDim() helper is gone from all of them (21 preambles and 16 copies down to the single definition each). - WaveEffect wrote three bytes per light unconditionally, so on a one-channel buffer it wrote two bytes past each light into its neighbours -- 62 of 64 pixels on an 8x8 grid. It now writes per channel, as draw::pixel does. - PaintBrushEffect and FixedRectangleEffect returned early below three channels and drew nothing. Both draw through channel-aware primitives already, so the guards only blanked the fixture. Tests: - Golden baselines 11 -> 22, capturing the effects that had none. DemoReel's was removed: it hosts whatever the global factory registry contains, so its frame depends on test order rather than on its own code. - New sweep: every effect at 1, 2, 3, 4 and 8 channels. Nothing covered channel count before, which is why the WaveEffect overrun survived this long. - No golden moved in this commit: all 23 migrations are byte-identical. Docs/CI: - architecture.md § Robustness rules rewritten: the Layer decides whether a frame runs, the effect decides what it paints, effects render at every channel count, and the test for which side a check belongs to. - The power-function spec gains the Canvas const contract (const protects the surface, not the pixels), a bounded per-light float exception for the desktop raymarch showcase, and a per-family scenario rule. - July digests for the eight friend repos, plus a ninth: hpwit/new-parser is ESPLiveScript2, a from-scratch rewrite built around a verifiable compiler. Our livescripts analysis is flagged as reading v1. Reviews: - CodeRabbit round 3, 7 findings: the Canvas mutability contradiction, the float rule contradiction, scenario coverage, a stat race in run_desktop.py, a rescan claim in control.md that moveListRow does not make, and three digest inconsistencies. The GEQ3D zero-dimension finding led to the Layer guard rather than the per-effect check it suggested. Verified on the running desktop device (48 minutes, no restart): all 15 effect families swapped live, the empty-grid guard exercised by shrinking the grid to zero width mid-render, a preset saved and restored through the Home Assistant ps path, and four mutate scenarios driven against it. Not verified on hardware. 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)
src/light/effects/SolidEffect.h (1)
87-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake palette-spread writes channel-aware.
Lines 87-94 write three bytes for every light even when
cplis 1 or 2. On a one-channel buffer, the writes spill into following lights. On a two-channel buffer, the third byte overwrites the next light's red channel.Limit the writes to
min(cpl, 3), or call the channel-awaredraw::pixelprimitive. Add regression coverage for palette mode with one- and two-channel buffers.Proposed fix
uint8_t* data = cv.data; const size_t bytes = cv.bytes; for (nrOfLightsType i = 0; i < nLights; i++) { const uint8_t idx = static_cast<uint8_t>(mapI(static_cast<int>(i), 0, static_cast<int>(nLights), 0, 256)); const RGB c = colorFromPalette(pal, idx, brightness); const size_t off = static_cast<size_t>(i) * cpl; - if (off + 3 > bytes) break; - data[off + 0] = c.r; data[off + 1] = c.g; data[off + 2] = c.b; + const uint8_t write = cpl < 3 ? cpl : 3; + if (off + write > bytes) break; + if (write >= 1) data[off + 0] = c.r; + if (write >= 2) data[off + 1] = c.g; + if (write >= 3) data[off + 2] = c.b; }As per path instructions,
src/light/**effects must use configurable channel counts and render at every channel count.🤖 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/effects/SolidEffect.h` around lines 87 - 94, Update the palette rendering loop in SolidEffect to write only min(cpl, 3) color channels per light, preventing writes beyond each pixel’s configured channel count; preserve RGB ordering for available channels and ensure palette mode renders correctly for one-, two-, and three-channel buffers. Add regression coverage for the one- and two-channel cases.Source: Path instructions
docs/moonmodules/core/control.md (1)
25-59: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd an end-to-end preset-apply scenario.
The PR excludes scenario coverage for preset save and restore. This leaves the file format and structural restore path without an end-to-end regression test.
Add a deterministic scenario-runner apply action. Cover a successful round trip and a truncated-file rejection that leaves the live tree unchanged.
As per coding guidelines, “Every behavior must be covered by meaningful unit and scenario tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/core/control.md` around lines 25 - 59, Add deterministic scenario-runner coverage for preset save and restore, including an apply action that verifies a successful round trip and rejects a truncated preset while preserving the existing live tree. Use the preset persistence and structural restore flow described by saveSubtreeTo and applySubtree, and assert both outcomes end to end.Source: Coding guidelines
♻️ Duplicate comments (1)
docs/backlog/power-functions-analysis-top-down.md (1)
141-149: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine a shared time origin and state-reconstruction contract for supersync.
Line 145 relies on local
elapsed()time. Devices that start at different times can therefore produce different phases at the same wall-clock time. Lines 146-147 also leave hash-time quantization and stateful-kernel input history undefined. CurrentBeatPhasereceives local elapsed time, not a shared epoch.Define a shared timestamp or epoch, its quantization, and either deterministic input replay or an explicit keyframe for stateful kernels. Update the decision at Line 192 to match.
Proposed specification update
-Each exposes a deterministic re-seed from (time, seed) so a joining device can be placed into the same state. +Each exposes a deterministic re-seed from a shared timestamp, seed, and a defined input-history or keyframe contract so a joining device can be placed into the same state.This repeats the unresolved supersync finding from the previous review.
Also applies to: 192-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/power-functions-analysis-top-down.md` around lines 141 - 149, Update the supersync specification around the “Time, never frame count” and stateful-kernel rules to define a shared timestamp or epoch, its quantization for hash-based randomness, and how stateful kernels reconstruct history through deterministic input replay or an explicit keyframe. Clarify that BeatPhase and other time-based effects use the shared origin rather than local elapsed() time, then revise the decision at “Line 192” to reflect this contract.
🤖 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 432-442: Rewrite the “Effects run at every grid size” and
Layer-gating guidance to match Layer::tick(): skip the effect pass when the grid
is empty, while preserving modifier execution when only depth_ is zero. Remove
the claim that every effect must handle 0×0×0 and clarify that empty-grid checks
belong to Layer::tick(), not individual effects; retain effect-owned guards for
resources, controls, timing, and producer input.
In `@src/light/draw.h`:
- Around line 372-417: Update the Canvas overload line(const Canvas&, Coord3D,
Coord3D, RGB, uint8_t) to use the same Bresenham error-carry loop and tie
handling as the existing Buffer line implementation, rather than maintaining a
separate rasterization loop. Preserve the Canvas pixel writer and shorten
behavior while ensuring cases such as (0,0,0) to (2,1,0) produce the same pixel
sequence as Buffer.
- Around line 363-370: Update the Y-axis call in inline blur to traverse all w
columns within each z-slice, matching the existing Buffer implementation. Adjust
the blurAxis arguments so the line count and outer stride process w Y lines per
slice, while preserving the existing X- and Z-axis behavior.
In `@src/light/layers/Layer.h`:
- Around line 156-163: Remove the early return in the Layer tick flow and
compute a hasGrid condition including width_, height_, and buffer_.count(); use
it to skip only the effect loop and applyLivePass(), while keeping
ModifierBase::tick() reachable so modifiers continue advancing. Add a
deterministic regression test covering modifier updates during a zero-size frame
and preserve live application of all settings for any size.
---
Outside diff comments:
In `@docs/moonmodules/core/control.md`:
- Around line 25-59: Add deterministic scenario-runner coverage for preset save
and restore, including an apply action that verifies a successful round trip and
rejects a truncated preset while preserving the existing live tree. Use the
preset persistence and structural restore flow described by saveSubtreeTo and
applySubtree, and assert both outcomes end to end.
In `@src/light/effects/SolidEffect.h`:
- Around line 87-94: Update the palette rendering loop in SolidEffect to write
only min(cpl, 3) color channels per light, preventing writes beyond each pixel’s
configured channel count; preserve RGB ordering for available channels and
ensure palette mode renders correctly for one-, two-, and three-channel buffers.
Add regression coverage for the one- and two-channel cases.
---
Duplicate comments:
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Around line 141-149: Update the supersync specification around the “Time,
never frame count” and stateful-kernel rules to define a shared timestamp or
epoch, its quantization for hash-based randomness, and how stateful kernels
reconstruct history through deterministic input replay or an explicit keyframe.
Clarify that BeatPhase and other time-based effects use the shared origin rather
than local elapsed() time, then revise the decision at “Line 192” to reflect
this contract.
🪄 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: db5e69e8-d670-483d-8b3a-0e823879ac88
📒 Files selected for processing (44)
docs/architecture.mddocs/backlog/power-functions-analysis-top-down.mddocs/history/MoonModules-WLED-MM.mddocs/history/PlummersSoftwareLLC-NightDriverStrip.mddocs/history/hpwit-I2SClocklessVirtualLedDriver.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmoondeck/run/run_desktop.pysrc/light/draw.hsrc/light/effects/AudioSpectrumEffect.hsrc/light/effects/BlurzEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/FixedRectangleEffect.hsrc/light/effects/FreqMatrixEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/LinesEffect.hsrc/light/effects/LissajousEffect.hsrc/light/effects/Noise2DEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PaintBrushEffect.hsrc/light/effects/PraxisEffect.hsrc/light/effects/RandomEffect.hsrc/light/effects/RubiksCubeEffect.hsrc/light/effects/SolidEffect.hsrc/light/effects/SphereMoveEffect.hsrc/light/effects/StarFieldEffect.hsrc/light/effects/StarSkyEffect.hsrc/light/effects/TetrixEffect.hsrc/light/effects/TextEffect.hsrc/light/effects/WaveEffect.hsrc/light/layers/Layer.htest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/core/scenario_MqttModule_haDiscovery_toggle.jsontest/scenarios/core/scenario_NetworkModule_mdns_toggle.jsontest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_modifier_swap.jsontest/unit/light/unit_Effects_golden.cpptest/unit/light/unit_Effects_gridsweep.cpp
💤 Files with no reviewable changes (2)
- src/light/effects/StarSkyEffect.h
- src/light/effects/AudioSpectrumEffect.h
Effects now share one toolbox instead of each hand-rolling its own drawing, field and motion math. Bars, scrolling, circles, signed distance fields, noise composition and polar addressing all live in one place, and six new effects show what the toolbox makes possible. Rings and Spiral gain a true radius, so they no longer stall short of the edge on a panel wider than 255 lights. KPI: 16384lights | Desktop:1037KB | tick:130/105/4/6/130/292/21/2/292/71/19/24/291/130/23/6/46/4us | ESP32:1551KB | src:207(51484) | test:154(29667) | lizard:148w Core: - math16 gains the polar pair (atan16, dist16), the kaleidoscope fold, three Penner easings, smoothFollow, peakHold and hashInt. atan16 uses a 66-byte octant table after a fitted polynomial measured 9.6 degrees of error at the fold; the table measures 0.015. - noise.h gains the composition layer: fbm8, turbulence8 and warp8 (domain warping), each built over the existing inoise8 rather than a second field. Light domain: - draw.h gains bar/rect/fillRect, scroll, circle/fillCircle, lineAA, the SDF family (sdCircle/sdBox/sdSegment, smin, coverage), splat, the gather pair (sampleWrap, combineMax) and the shared blob field. - bar takes a colour callback because every real call site varies colour ALONG the bar; the flat RGB overload is what a MoonLive script will reach. Measured identical to the hand-rolled loop (55 instructions, ratio 0.98-1.01). - LavaLamp and Metaballs converge on the shared blob field, GEQ and AudioSpectrum on bar, FreqMatrix on scroll — all pixel-identical, pinned by goldens. AudioSpectrum loses a private setRGB that re-implemented draw::pixel. - Spiral and Rings move to 16-bit polar. This CHANGES their look and fixes a real bug: dist8 saturates at 255, so Rings' radius limit stopped growing and the rings stalled short of the edge on a large panel. Goldens re-baselined. - New: PolarNoise, WaterRipple, Tunnel, Echo, Dissolve, Spectrum. Each proves a different part of the toolbox — Echo shows that feedback is three lines once the grid can be sampled as a texture, Dissolve that position-addressed randomness needs no per-pixel state. - WaterRipple: three bugs found by the product owner on hardware. Brightness was normalised against the peak while the mean magnitude is 8% of it, so the surface rendered dark and single-hued; drops were timed per frame, so the rate scaled with the framerate; and the wave loop skipped the border, leaving a dead one-pixel frame around the fixture. Now scaled against a typical ripple, timed in milliseconds with a speed control, and stepped with reflecting boundaries so every pixel moves. Tests: - New suites for splat, SDF, bar, scroll, circle/lineAA, fields and the polar pair; goldens for the six new effects. - A modifier keeps ticking while the grid is empty (mutation-tested: the early return CodeRabbit warned about makes it fail 0 == 5). - A per-channel write never spills into the next light — the overrun class that hid in SolidEffect and WaveEffect. Docs/CI: - docs/moonmodules/light/power-functions.md is new: every power function, what it does, and its callers, generated by reading the call sites. Shared by effects, modifiers and MoonLive, which is why it is its own page. - The modifier column is almost entirely empty, and that is the architecture: an effect decides what a pixel looks like, a modifier decides where a pixel comes from. Exactly one modifier uses a power function. Reviews: - 🐇 CodeRabbit, 4 findings: the Canvas blur y-pass looped z wrongly, line was duplicated between the Buffer and Canvas forms, SolidEffect wrote 3 channels unconditionally, and Layer::tick returned before the modifier pass. All fixed; the architecture.md rule was corrected to describe what ships. - 👾 Reviewer, 2 real bugs: kaleido put a one-unit discontinuity at every seam (wedge - within maps 0 past the end), and smin overflowed int32 once the blend radius passed ~131000 sub-units, returning a dip of 9464 where the correct value is 75000. Both fixed and pinned. The Reviewer's prescribed kaleido fix was wrong — adding the wedge base back made the seams worse — so only the off-by-one was taken. - Not extracted: a WeaveModifier was built to generalise FreqSaws' invert, then reverted. The control exists for columns mapped onto RINGS, and measured against WheelLayout a spoke spans many grid columns, so a column flip cannot make wheels counter-rotate. It would have carried the name of an effect it does not achieve. Verified on the ESP32-S3 testbench: all seven showcase effects present, WaterRipple at 1327us against a 2137us tick at 467 fps, and the border fix confirmed on the panel. Desktop-verified for the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/moonmodules/core/control.md (1)
57-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe a single-subtree restore.
Line 59 conflicts with Lines 33-47. A preset captures exactly one subtree, and legacy multi-subtree files are refused. Replace “Every captured subtree is applied” with singular wording and remove the per-capture comparison.
As per coding guidelines, “Documentation must describe the system as it currently exists.” As per path instructions, “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/core/control.md` around lines 57 - 59, Update the preset restore description near prepareTree() to state that the single captured subtree is applied, then prepareTree() runs once at the end. Remove the reference to applying every captured subtree and any per-capture comparison.Sources: Coding guidelines, Path instructions
src/light/effects/FixedRectangleEffect.h (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe Canvas migration left
depthDim()comments behind in two effects. Each file deleted its privatedepthDim()helper but kept the comment that described it, so bothprivate:sections now carry text about a zero-depth dims guard thatcanvas()performs.
src/light/effects/FixedRectangleEffect.h#L108-L110: delete the two-line comment belowMINiand keepMINi.src/light/effects/PraxisEffect.h#L91-L92: delete the comment and the now-emptyprivate:label.🤖 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/effects/FixedRectangleEffect.h` around lines 108 - 110, Remove the obsolete two-line depthDim() comment beneath MINi in src/light/effects/FixedRectangleEffect.h:108-110, keeping MINi unchanged. In src/light/effects/PraxisEffect.h:91-92, remove the obsolete comment and the now-empty private: label.src/main.cpp (1)
357-361: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
ControlModuleto the root module tree.The boot-created
controlModuleis injected intoMqttModuleand added toSchedulerbeforesetup(), but it is not inserted as a child of any root module. Use one existing root module’saddChild()so the tree ownership matches the scheduler lifetime and MQTT does not hold a separate dangling reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 357 - 361, Add the boot-created controlModule to an existing root module using that module’s addChild() before setup(), while preserving its injection into MqttModule and Scheduler. Ensure the root tree owns the same ControlModule instance so its lifetime matches the scheduler and MQTT references it safely.
♻️ Duplicate comments (1)
docs/architecture.md (1)
432-440: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the empty-grid statement with
Layer::tick().Line 432 says an effect
tick()handles0×0×0. Lines 434-440 state thatLayer::tick()skips the effect pass before an effect runs. State that effects support all non-empty grid shapes, whileLayer::tick()owns empty-grid skipping.As per coding guidelines,
docs/**/*.mdmust 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/architecture.md` around lines 432 - 440, Update the “Effects run at every grid size” section to state that effects support every non-empty grid shape, while `Layer::tick()` skips effect execution for empty extents. Keep the existing explanation of modifier execution and effect-owned checks consistent with this responsibility split.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/math16.h`:
- Around line 263-265: Update smoothFollow so every nonzero rate makes progress
toward target, using a signed step rounded toward the target; ensure rate 255
reaches target immediately and preserve current behavior for rate 0. Add
endpoint tests covering rates 1 and 255, including both upward and downward
movement.
- Around line 300-315: Add direct unit tests for kaleido covering identity when
segments is 0 or 1, mirrored output in alternating wedges, and seam behavior for
non-divisor counts such as 3 and 255. Assert both boundary and representative
within-wedge values, including that folded results remain within the wedge and
do not exhibit a one-unit seam discontinuity.
- Around line 192-220: Mark the visible atan16 and dist16 helper functions
constexpr, ensuring every operation and dependency, including atan16_octant, is
usable in constant evaluation. Add representative static_assert checks for key
angle and distance results to provide compile-time coverage of the core math
behavior.
- Around line 192-220: Update dist16 to compute each coordinate’s squared
magnitude in uint64_t, safely handling INT32_MIN, and saturate the combined
distance square to UINT32_MAX before calling isqrt. Add regression coverage for
INT32_MIN axis, INT32_MAX axis, and diagonal inputs.
In `@src/core/noise.h`:
- Around line 152-158: Update warp8 so coordinate displacement is performed
entirely in uint32_t modulo arithmetic, avoiding the signed int32_t conversions
when adding dx and dy before calling fbm8. Preserve the existing displacement
calculation and add coverage for inputs near INT32_MAX and UINT32_MAX to verify
wrapping behavior remains valid.
In `@src/light/draw.h`:
- Around line 980-995: In src/light/draw.h lines 980-995, update smin so h is
calculated and clamped as int64_t, then narrowed to int32_t only for the mixed
and bump terms. In src/light/draw.h lines 1002-1008, update coverage so both
(edge - d) * 255 and 2 * edge are evaluated as int64_t before division, with
clamping performed on the widened result.
- Around line 1002-1008: Widen the intermediate arithmetic in coverage() so both
the numerator and denominator are computed in a sufficiently wide integer type
before division. Preserve the existing clamping and [255, 0] mapping while
preventing overflow in (edge - d) * 255 and 2 * edge for large caller-supplied
edge values.
- Around line 516-546: Update the wrapping branch in the strided rotation logic
to preserve all cpl bytes when saving and restoring the final cell, rather than
limiting the scratch buffer to four channels. Use the existing project-wide
channel bound or define kMaxChannelsPerLight beside the draw constants, and
ensure the implementation safely handles the configured channel count without
truncation or overflow.
- Around line 332-341: Replace the Canvas-overload header comment with an
accurate description: Canvas overloads generally contain independent
implementations of the corresponding Buffer-based primitives and do not
construct a Buffer view, so those pairs may drift. Identify line as the sole
exception, since its overloads share the detail::walkLine implementation.
- Around line 718-733: Rename the local variable near in the lineAA drawing loop
to a non-conflicting identifier such as weightNear, and update both scale8 calls
that use it. Leave the anti-aliased weighting behavior unchanged.
In `@src/light/effects/DissolveEffect.h`:
- Around line 63-64: Update the hue interpolation around hueA, hueB, and the
calculation at line 86 to interpolate using the fixed 40-step delta rather than
subtracting the truncated uint8_t values. Preserve uint8_t wraparound by
applying the cast after computing the interpolated hue, so palette transitions
always advance 40 steps across index wrap.
In `@src/light/effects/EchoEffect.h`:
- Around line 113-119: Update the history write guard in the frame-copy loop to
compare i + 2 against history_.bytes() instead of the locally derived bytes
value, ensuring writes are bounded by the actual allocated history buffer.
- Around line 82-88: Widen the intermediate rotation arithmetic in the sampling
code to 64-bit so the products and sums in the rx and ry calculations cannot
overflow on large canvases. Keep the existing fixed-point shifts and subsequent
scale calculations unchanged, while ensuring both px*cosA/py*sinA combinations
are evaluated using int64_t before conversion back to the coordinate type.
In `@src/light/effects/LinesEffect.h`:
- Line 55: Update the buffer initialization guard near the null check in
LinesEffect so it rejects non-positive lengthType dimensions and zero channel
counts before calculating the memset size; preserve the existing early-return
behavior for invalid inputs and add a regression test covering negative
dimensions.
In `@src/light/effects/RingsEffect.h`:
- Around line 43-48: Remove the 8-bit radius ceiling in
src/light/effects/RingsEffect.h lines 43-48 by keeping maxR and the per-ripple
radius_ in a sufficiently wide type based on dist16, preserving far-corner
values above 255. At lines 82-83, retain the wider per-pixel distance when
computing diff so distances above 255 are not truncated. Add a regression test
covering a far-corner radius greater than 255.
In `@src/light/effects/SolidEffect.h`:
- Around line 86-100: Update the palette-writing loop in SolidEffect’s case 1 to
use cv.cpl consistently for the per-light stride and channel-count bound,
matching RandomEffect’s flat-index addressing. Retain the existing write-limit
logic and overrun guard while removing reliance on the separately read cpl value
for these calculations.
In `@src/light/effects/SpectrumEffect.h`:
- Around line 57-62: Update SpectrumEffect::tick() to return immediately when
width() or height() is zero, in addition to the existing levels_ and peaks_
checks. Place the grid-size guard before drawing or calculating bar coordinates,
preserving normal rendering for positive dimensions.
In `@src/light/effects/WaterRippleEffect.h`:
- Around line 129-133: Clamp the computed wave step in the update logic before
assigning it to lastField()[i]. After applying damping to next, constrain the
int32_t value to the representable int16_t range, preserving normal values while
preventing overflow wraparound for large interfering drops.
In `@src/light/layers/Layer.h`:
- Line 156: Guard the applyLivePass() call in Layer’s layout-processing flow
with both hasGrid and hasLive_, so live modifiers are not applied when
buffer_.count() is zero. Add a regression test in unit_Layer_zero_grid.cpp
covering a live modifier on an empty layout and confirming it completes without
entering the remapping pass.
- Around line 78-82: Add a focused unit test for Layer::setChannelsPerLight that
records the initial valid channelsPerLight() value, calls
setChannelsPerLight(0), and verifies the value remains unchanged. Use the
existing Layer test fixture and assertion conventions.
In `@test/scenarios/core/scenario_MoonModule_control_change.json`:
- Line 120: Align each changed tick_us[0] timing sample with its corresponding
observation date by updating at[0] rather than at[1] in all referenced blocks,
or revise both at entries to the actual measurement dates; preserve positional
pairing between timing and date arrays.
In `@test/unit/core/unit_fields.cpp`:
- Around line 37-44: Replace the tautological v <= 255 assertion in the fbm8
test with a normalization check: compute the minimum and maximum samples across
the requested octaves and verify the fBm result remains within that range. Keep
the existing x and oct sampling coverage and captures.
In `@test/unit/light/unit_Effects_golden.cpp`:
- Around line 74-102: Replace the implementation-type labels in the new SUBCASE
entries with user-understandable descriptions of each effect’s expected
rendering behavior, including the 16×16 fixed-cadence context where relevant.
Keep each effect instance, golden::renderHash call, and expected hash unchanged;
update only the descriptive labels across the affected subcases.
In `@test/unit/light/unit_Effects_gridsweep.cpp`:
- Around line 92-105: Update the channel-count sweep helper and its caller to
wrap the backing storage with sentinels and verify them after each effect run,
including 1- and 2-channel fixtures. Assert that guard bytes and per-light
channel boundaries remain unchanged for every cpl, while retaining the existing
wrote detection; add meaningful unit/scenario coverage for the new corruption
checks.
In `@test/unit/light/unit_Splat.cpp`:
- Around line 148-161: Update the test case “a per-channel write never spills
into the next light” to assert light 1’s second channel when cpl >= 2, verifying
the expected green value at the corresponding buffer offset while retaining the
existing red and untouched-light checks.
- Around line 58-61: Rename the local variables near and far in the test case
containing the Splat coverage checks to Windows-safe, descriptive names, and
update all corresponding CHECK expressions.
---
Outside diff comments:
In `@docs/moonmodules/core/control.md`:
- Around line 57-59: Update the preset restore description near prepareTree() to
state that the single captured subtree is applied, then prepareTree() runs once
at the end. Remove the reference to applying every captured subtree and any
per-capture comparison.
In `@src/light/effects/FixedRectangleEffect.h`:
- Around line 108-110: Remove the obsolete two-line depthDim() comment beneath
MINi in src/light/effects/FixedRectangleEffect.h:108-110, keeping MINi
unchanged. In src/light/effects/PraxisEffect.h:91-92, remove the obsolete
comment and the now-empty private: label.
In `@src/main.cpp`:
- Around line 357-361: Add the boot-created controlModule to an existing root
module using that module’s addChild() before setup(), while preserving its
injection into MqttModule and Scheduler. Ensure the root tree owns the same
ControlModule instance so its lifetime matches the scheduler and MQTT references
it safely.
---
Duplicate comments:
In `@docs/architecture.md`:
- Around line 432-440: Update the “Effects run at every grid size” section to
state that effects support every non-empty grid shape, while `Layer::tick()`
skips effect execution for empty extents. Keep the existing explanation of
modifier execution and effect-owned checks consistent with this responsibility
split.
🪄 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: 4e419df3-40e4-422a-89db-c47b6efb3d05
📒 Files selected for processing (74)
docs/architecture.mddocs/assets/extra.cssdocs/backlog/power-functions-analysis-bottom-up.mddocs/backlog/power-functions-analysis-top-down.mddocs/history/MoonModules-WLED-MM.mddocs/history/PlummersSoftwareLLC-NightDriverStrip.mddocs/history/hpwit-I2SClocklessVirtualLedDriver.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mddocs/moonmodules/light/MoonLiveEffect.mddocs/moonmodules/light/effects.mddocs/moonmodules/light/modifiers.mddocs/moonmodules/light/power-functions.mdmkdocs.ymlmoondeck/run/run_desktop.pysrc/core/math16.hsrc/core/noise.hsrc/light/draw.hsrc/light/effects/AudioSpectrumEffect.hsrc/light/effects/BlurzEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/DissolveEffect.hsrc/light/effects/EchoEffect.hsrc/light/effects/FixedRectangleEffect.hsrc/light/effects/FreqMatrixEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/LavaLampEffect.hsrc/light/effects/LinesEffect.hsrc/light/effects/LissajousEffect.hsrc/light/effects/MetaballsEffect.hsrc/light/effects/Noise2DEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PaintBrushEffect.hsrc/light/effects/PolarNoiseEffect.hsrc/light/effects/PraxisEffect.hsrc/light/effects/RandomEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/RubiksCubeEffect.hsrc/light/effects/SdfShapesEffect.hsrc/light/effects/SolidEffect.hsrc/light/effects/SpectrumEffect.hsrc/light/effects/SphereMoveEffect.hsrc/light/effects/SpiralEffect.hsrc/light/effects/StarFieldEffect.hsrc/light/effects/StarSkyEffect.hsrc/light/effects/TetrixEffect.hsrc/light/effects/TextEffect.hsrc/light/effects/TunnelEffect.hsrc/light/effects/WaterRippleEffect.hsrc/light/effects/WaveEffect.hsrc/light/layers/Layer.hsrc/main.cpptest/CMakeLists.txttest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/core/scenario_MqttModule_haDiscovery_toggle.jsontest/scenarios/core/scenario_NetworkModule_mdns_toggle.jsontest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_modifier_swap.jsontest/unit/core/unit_fields.cpptest/unit/core/unit_math16.cpptest/unit/light/unit_Bar.cpptest/unit/light/unit_Circle.cpptest/unit/light/unit_Effects_golden.cpptest/unit/light/unit_Effects_gridsweep.cpptest/unit/light/unit_Layer_zero_grid.cpptest/unit/light/unit_Scroll.cpptest/unit/light/unit_Sdf.cpptest/unit/light/unit_Splat.cpp
💤 Files with no reviewable changes (1)
- src/light/effects/StarSkyEffect.h
Comment on lines
+192
to
+220
| inline angle16 atan16(int32_t y, int32_t x) { | ||
| if (x == 0 && y == 0) return 0; // the centre has no direction | ||
| // Fold into the first octant, remembering which one, then interpolate the arctangent there. | ||
| int32_t ax = x < 0 ? -x : x; | ||
| int32_t ay = y < 0 ? -y : y; | ||
| const bool swap = ay > ax; | ||
| if (swap) { const int32_t t = ax; ax = ay; ay = t; } | ||
| // ratio = ay/ax in 0..65535; within one octant arctan is near-linear, so a linear read with a | ||
| // small cubic correction is well inside a pixel of error at any grid size we drive. | ||
| const uint32_t ratio = static_cast<uint32_t>((static_cast<uint64_t>(ay) << 16) / (ax ? ax : 1)); | ||
| // Table lookup with linear interpolation, the same shape as sin16 above and for the same reason: | ||
| // a fitted polynomial was tried first and measured 9.6 degrees of error at the octant boundary, | ||
| // where a 66-byte table is exact at every entry and closes precisely at 8192. | ||
| const uint16_t idx = static_cast<uint16_t>(ratio >> 11); // 0..32 | ||
| const uint16_t frac = static_cast<uint16_t>((ratio >> 3) & 0xFF); | ||
| const int32_t lo = atan16_octant[idx]; | ||
| const int32_t hi = atan16_octant[idx < 32 ? idx + 1 : 32]; | ||
| uint32_t oct = static_cast<uint32_t>(lo + (((hi - lo) * frac) >> 8)); | ||
| uint16_t a = swap ? static_cast<uint16_t>(16384 - oct) : static_cast<uint16_t>(oct); | ||
| if (x < 0) a = static_cast<uint16_t>(32768 - a); // reflect into quadrant 2/3 | ||
| if (y < 0) a = static_cast<uint16_t>(65536 - a); // and below the axis | ||
| return static_cast<angle16>(a); | ||
| } | ||
|
|
||
| /// True Euclidean distance from the origin to (dx, dy) — a real radius, not the octagon `dist8` | ||
| /// approximates, and it does not saturate at 255. | ||
| inline uint32_t dist16(int32_t dx, int32_t dy) { | ||
| const int64_t d2 = static_cast<int64_t>(dx) * dx + static_cast<int64_t>(dy) * dy; | ||
| return isqrt(static_cast<uint32_t>(d2 > 0xFFFFFFFFLL ? 0xFFFFFFFFLL : d2)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make atan16 and dist16 constexpr, with optional static_assert coverage.
atan16 and dist16 are pure integer-only helpers used by core math, and atan16_octant is constexpr lookup data. Mark both helpers constexpr, and add representative compile-time checks to preserve the core color/math requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/math16.h` around lines 192 - 220, Mark the visible atan16 and dist16
helper functions constexpr, ensuring every operation and dependency, including
atan16_octant, is usable in constant evaluation. Add representative
static_assert checks for key angle and distance results to provide compile-time
coverage of the core math behavior.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate math16.h =="
fd -a 'math16\.h$' . || true
echo
echo "== file excerpt =="
if [ -f src/core/math16.h ]; then
sed -n '1,260p' src/core/math16.h | cat -n
fi
echo
echo "== relevant definitions/usages =="
rg -n "angle16|atan16|dist16|isqrt|atan16_octant|dist8" src/core math src 2>/dev/null || true
echo
echo "== git diff stat/name-status if available =="
git diff --stat HEAD~1..HEAD 2>/dev/null || true
git diff -- src/core/math16.h 2>/dev/null | sed -n '1,160p' || true
echo
echo "== read-only C unsigned/saturation behavior probe =="
python3 - <<'PY'
from math import atan2, degrees, isqrt
# Model signed 32-bit inputs. The concern is whether negative-x/y and max squared sum
# fit in the algorithm state; C behavior is out-of-range signed arithmetic.
values = [
(0, -2147483648),
(-2147483648, 0),
(-2147483648, -2147483648),
]
for xv, yv in values:
ox, oy = xv, yv
# Proposed unsigned magnitude state.
mex = xv if xv >= 0 else (-xv) % (2**32)
mey = yv if yv >= 0 else (-yv) % (2**32)
print(f"x={xv} y={yv} first_octant {mex},{mey} ratio fits 0..65535? {me <= 0xFFFFFFFF for me in (mex,mey)}")
# Squared distance state: 31-bit inputs => 98... fits 64-bit; 32-bit inputs => need 64-bit.
PYRepository: MoonModules/projectMM
Length of output: 27763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== unit_math16 excerpt =="
sed -n '1,240p' test/unit/core/unit_math16.cpp | cat -n
echo
echo "== C overflow behavior probe (read-only arithmetic) =="
python3 - <<'PY'
import math
INT32_MIN = -2**31
INT32_MAX = 2**31 - 1
def neg(x):
return -(x % (2**32)) if x == INT32_MIN else -x
def atan16(x, y):
if x == 0 and y == 0:
return 0
ax = neg(x)
ay = neg(y)
swap = ay > ax
if swap:
ax, ay = ay, ax
ratio = (ay << 16) // ax if ax else (ay << 16)
idx = ratio >> 11
frac = (ratio >> 3) & 0xFF
atan16_octant = [
0, 326, 651, 975, 1297, 1617, 1933, 2246,
2555, 2860, 3159, 3453, 3742, 4025, 4302, 4572,
4836, 5094, 5344, 5589, 5826, 6058, 6282, 6500,
6712, 6917, 7117, 7310, 7498, 7679, 7856, 8026,
8192
]
lo = atan16_octant[idx]
hi = atan16_octant[idx + 1 if idx < 32 else 32]
oct = lo + ((hi - lo) * frac) >> 8
a = 16384 - oct if swap else oct
if x < 0:
a = 32768 - a
if y < 0:
a = 65536 - a
return a & 0xFFFF
for x, y in [(0, INT32_MIN), (INT32_MIN, 0), (INT32_MIN, INT32_MIN), (-1000000000, INT32_MIN)]:
expected_degrees = degrees(math.atan2(y, x))
expected_ccw = (expected_degrees / 360) * 65536
if expected_ccw < 0:
expected_ccw += 65536
print(f"x={x} y={y} expected_ccw={expected_ccw:.4f} current={atan16(x,y)} ratio={(neg(y) << 16) // (neg(x) if neg(x) else 1) if not (y==0) else (neg(x) << 16) if x else 0}")
print("INT32_MAX squares:", INT32_MAX * INT32_MAX, "sum:", INT32_MAX * INT32_MAX + INT32_MAX * INT32_MAX)
print("UINT64_MAX:", 2**64 - 1)
PYRepository: MoonModules/projectMM
Length of output: 13174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== C overflow behavior probe (read-only arithmetic, fixed) =="
python3 - <<'PY'
from math import degrees
INT32_MIN = -2**31
INT32_MAX = 2**31 - 1
def neg(x):
return -(x % (2**32)) if x == INT32_MIN else -x
def atan16(x, y):
if x == 0 and y == 0:
return 0
ax = neg(x)
ay = neg(y)
swap = ay > ax
if swap:
ax, ay = ay, ax
ratio = (ay << 16) // ax if ax else (ay << 16)
idx = ratio >> 11
frac = (ratio >> 3) & 0xFF
atan16_octant = [
0, 326, 651, 975, 1297, 1617, 1933, 2246,
2555, 2860, 3159, 3453, 3742, 4025, 4302, 4572,
4836, 5094, 5344, 5589, 5826, 6058, 6282, 6500,
6712, 6917, 7117, 7310, 7498, 7679, 7856, 8026,
8192
]
lo = atan16_octant[idx]
hi = atan16_octant[idx + 1 if idx < 32 else 32]
oct = lo + ((hi - lo) * frac) >> 8
a = 16384 - oct if swap else oct
if x < 0:
a = 32768 - a
if y < 0:
a = 65536 - a
return a & 0xFFFF
for x, y in [(0, INT32_MIN), (INT32_MIN, 0), (INT32_MIN, INT32_MIN), (-1000000000, INT32_MIN)]:
expected_degrees = degrees(degrees if False else (0 if True else (degrees if False else (0 if True else 0))))[:0] # no-op
expected_degrees = math.degrees(math.atan2(y, x))
PYRepository: MoonModules/projectMM
Length of output: 342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== C overflow behavior probe (read-only arithmetic, fixed) =="
python3 - <<'PY'
import math
INT32_MIN = -2**31
INT32_MAX = 2**31 - 1
def neg(x):
return -(x % (2**32)) if x == INT32_MIN else -x
def atan16(x, y):
if x == 0 and y == 0:
return 0
ax = neg(x)
ay = neg(y)
swap = ay > ax
if swap:
ax, ay = ay, ax
ratio = (ay << 16) // ax if ax else (ay << 16)
idx = ratio >> 11
frac = (ratio >> 3) & 0xFF
atan16_octant = [
0, 326, 651, 975, 1297, 1617, 1933, 2246,
2555, 2860, 3159, 3453, 3742, 4025, 4302, 4572,
4836, 5094, 5344, 5589, 5826, 6058, 6282, 6500,
6712, 6917, 7117, 7310, 7498, 7679, 7856, 8026,
8192
]
lo = atan16_octant[idx]
hi = atan16_octant[idx + 1 if idx < 32 else 32]
oct = lo + ((hi - lo) * frac) >> 8
a = 16384 - oct if swap else oct
if x < 0:
a = 32768 - a
if y < 0:
a = 65536 - a
return a & 0xFFFF
for x, y in [(0, INT32_MIN), (INT32_MIN, 0), (INT32_MIN, INT32_MIN), (-1000000000, INT32_MIN)]:
expected_degrees = math.degrees(math.atan2(float(y), float(x)))
expected_ccw = (expected_degrees / 360) * 65536
if expected_ccw < 0:
expected_ccw += 65536
print(f"x={x} y={y} expected_ccw={expected_ccw:.4f} current={atan16(x,y)} ratio={(neg(neg(y)) << 16) // (neg(neg(x)) if neg(neg(x)) else 1)}")
print("INT32_MAX squares:", INT32_MAX * INT32_MAX)
print("INT32_MIN magnitude sum of squares:", (2**31) * (2**31) + (2**31) * (2**31))
print("INT64_MAX:", 2**63 - 1)
print("UINT64_MAX:", 2**64 - 1)
PY
echo
echo "== inspect INT32_MIN regression area if any =="
rg -n "INT32_MIN|int32.*min|INT_MIN|atan16|dist16" test/unit/core/unit_math16.cpp src/core/math16.hRepository: MoonModules/projectMM
Length of output: 368
Compute dist16 through a 64-bit distance square.
Casting dx and dy to int64_t before squaring still lets INT32_MAX pairs exceed int64_t range before the sum, so dist16 can overflow and then silently wrap or saturate. Compute both squared components in uint64_t, saturate the sum to UINT32_MAX, then call isqrt. Add regression cases for INT32_MIN axes, INT32_MAX axes, and the diagonal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/math16.h` around lines 192 - 220, Update dist16 to compute each
coordinate’s squared magnitude in uint64_t, safely handling INT32_MIN, and
saturate the combined distance square to UINT32_MAX before calling isqrt. Add
regression coverage for INT32_MIN axis, INT32_MAX axis, and diagonal inputs.
Source: Path instructions
Comment on lines
+300
to
+315
| inline angle16 kaleido(angle16 a, uint8_t segments) { | ||
| if (segments < 2) return a; // one segment is the identity | ||
| const uint32_t wedge = 65536u / segments; | ||
| uint32_t within = a % wedge; // position inside this wedge | ||
| const uint32_t index = a / wedge; | ||
| // Reflect alternate wedges and return the FOLDED coordinate — deliberately one wedge wide, not | ||
| // the original angle. That is what a kaleidoscope is: every wedge maps onto the same range, so a | ||
| // field sampled through it repeats n times around the circle, and mirroring every other wedge is | ||
| // what makes the seams join rather than showing a hard edge. A caller that wants the full turn | ||
| // simply does not fold. | ||
| // | ||
| // The `- 1` matters: `wedge - within` maps 0 to `wedge`, one past the end, which puts a | ||
| // one-unit discontinuity at every seam. | ||
| if (index & 1) within = wedge - 1 - within; | ||
| return static_cast<angle16>(within); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add direct kaleido unit coverage.
This new helper has no test coverage. Add tests for identity at segments 0 and 1, alternating-wedge mirroring, and seam behavior for non-divisor segment counts such as 3 and 255.
As per coding guidelines, “Pin every new behavior with meaningful unit and scenario tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/math16.h` around lines 300 - 315, Add direct unit tests for kaleido
covering identity when segments is 0 or 1, mirrored output in alternating
wedges, and seam behavior for non-divisor counts such as 3 and 255. Assert both
boundary and representative within-wedge values, including that folded results
remain within the wedge and do not exhibit a one-unit seam discontinuity.
Source: Coding guidelines
Comment on lines
+152
to
+158
| inline uint8_t warp8(uint32_t x, uint32_t y, uint16_t strength, uint8_t octaves = 1) { | ||
| // Offset the two probe fields so the x and y displacements are independent rather than equal | ||
| // (sampling the same field twice would displace everything along one diagonal). | ||
| const int32_t dx = (static_cast<int32_t>(inoise8(x, y)) - 128) * strength / 128; | ||
| const int32_t dy = (static_cast<int32_t>(inoise8(x + 0x9E37u, y + 0x7C15u)) - 128) * strength / 128; | ||
| return fbm8(static_cast<uint32_t>(static_cast<int32_t>(x) + dx), | ||
| static_cast<uint32_t>(static_cast<int32_t>(y) + dy), octaves); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '147,160p' src/core/noise.h
rg -n -C 3 '\bwarp8\s*\(' --glob '*.{cpp,h,hpp}' .Repository: MoonModules/projectMM
Length of output: 3916
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== relevant noise.h declarations =="
sed -n '1,170p' src/core/noise.h
echo
echo "== behavior probe for the signed wrap pattern =="
python3 - <<'PY'
import ctypes
def signed_wrap(value):
return ctypes.c_int32(value).value
def add_signed_wrap(x_signed, dx_signed):
s = x_signed + dx_signed
# model C 32-bit signed overflow: convert to two's-complement range, then to uint32_t
return ctypes.c_uint32(s).value
for x in [0, 0x7fffffff, 0xffffffff]:
for dx in [-2000, -1, 0, 1, 2000]:
actual = add_signed_wrap(signed_wrap(x), dx)
expected = (x + dx) & 0xffffffff
if actual != expected:
print(f"x={x:`#x`}, dx={dx:`#x`}, signed_add_result={actual:`#8x`}, unsigned_modulo_result={expected:`#8x`}")
PYRepository: MoonModules/projectMM
Length of output: 8622
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import struct
def as_int32_bits(v):
return struct.unpack("i", struct.pack("I", v & 0xFFFFFFFF))[0]
def add_modulo(x, dx):
return (x + dx) & 0xFFFFFFFF
def add_signed_with_wrap(x, dx):
s = as_int32_bits(x) + dx
return struct.unpack("I", struct.pack("i", s))[0]
tests = [(0, -1), (0xFFFFFFFF, -1), (0x0FFFFFFF, 1), (0x70301800, 0x70305870)]
for x, dx in tests:
m = add_modulo(x, dx)
sm = add_signed_with_wrap(x, dx)
if m != sm:
print(f"x={x:`#010x`}, dx={dx:`#010x`}: modulo={m:`#010x`}, signed-wrapped={sm:`#010x`}, match={m==sm}")
else:
print(f"x={x:`#010x`}, dx={dx:`#010x`}: match")
PYRepository: MoonModules/projectMM
Length of output: 459
🌐 Web query:
GCC C signed integer overflow undefined behavior overflow wrap-around
💡 Result:
In the C programming language, signed integer overflow is classified as undefined behavior (UB) [1][2][3]. This means the C standard does not mandate a specific outcome, and compilers—including GCC—are permitted to assume that signed integer overflow never occurs [1][4][5]. When compiling with optimizations (such as -O2 or -O3), GCC utilizes this assumption to perform aggressive optimizations [4][5]. Because the compiler assumes overflow will not happen, it may: 1. Optimize away code paths or checks that would only be reached if an overflow occurred [6][7][5]. 2. Assume algebraic properties that would be invalidated by overflow (e.g., assuming i + 1 > i is always true) [6][4]. 3. Generate machine code that produces inconsistent or unexpected results, as the compiler is not obligated to produce code that "wraps around" or performs any specific action when overflow occurs [1][5]. While signed integer overflow is undefined behavior in standard C, you can alter GCC's behavior using specific compiler flags: - -fwrapv: This flag instructs GCC to treat signed integer overflow as having well-defined "two's complement wrap-around" behavior, effectively disabling the optimizations that rely on the assumption that overflow cannot occur [1][4]. - -ftrapv: This flag instructs GCC to generate a trap (typically causing an abnormal program exit) when a signed integer overflow occurs [8][1][2]. - -fsanitize=undefined: This option enables the UndefinedBehaviorSanitizer (UBSan), which can be used during testing to detect signed integer overflows at runtime [8]. For security and correctness, it is generally recommended to prevent overflow by checking operands before performing arithmetic, rather than relying on compiler-specific behaviors or flags [1][7]. Methods for safe arithmetic (such as those provided by built-in compiler functions like builtin_add_overflow or manual checks) should be used to ensure programs behave predictably [8][7].
Citations:
- 1: https://cmu-sei.github.io/secure-coding-standards/sei-cert-c-coding-standard/rules/integers-int/int32-c/
- 2: https://en.cppreference.com/c/language/operator_arithmetic
- 3: https://en.cppreference.com/c/language/behavior
- 4: https://stackoverflow.com/questions/12729110/strange-integer-behavior-with-gcc-o2
- 5: https://users.cs.utah.edu/~regehr/papers/tosem15.pdf
- 6: http://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html
- 7: https://stackoverflow.com/questions/25151602/will-gcc-skip-this-check-for-signed-integer-overflow
- 8: http://www.pixelbeat.org/programming/gcc/integer_overflow.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== wrap8/displacement tests =="
sed -n '85,112p' test/unit/core/unit_fields.cpp
rg -n -C 2 'INT32_MAX|UINT32_MAX|UINT32_C|0xFFFF|warp8\(.*9000|warp8\(.*2000' test src --glob '*.{cpp,h,hpp}' || true
echo
echo "== deterministic bit-level probe for unsigned-displacement semantics =="
python3 - <<'PY'
import ctypes
def c_uint32(x): return ctypes.c_uint32(x & 0xFFFFFFFF).value
def c_int32(x): return ctypes.c_int32(x & 0xFFFFFFFF).value
def inoise8(x, y): 128 if (x * y * 31) % (1 << 32) < (1 << 30) else 127 # placeholder invariant: same field always same result for same inputs
def fbm8(x, y, octaves): 128 + (x + y) % 256 # placeholder behavior only for regression case ordering
def warp_current(x, y, strength):
dx = c_int32((c_int32(inoise8(x, y)) - 128) * strength / 128)
dy = c_int32((c_int32(inoise8(x + 0x9E37, y + 0x7C15)) - 128) * strength / 128)
return fbm8(c_uint32(c_int32(x + dx)), c_uint32(c_int32(y + dy)), 1)
def warp_unsigned(x, y, strength):
dx = c_uint32(c_int32((c_int32(inoise8(x, y)) - 128) * strength / 128))
dy = c_uint32(c_int32((c_int32(inoise8(x + 0x9E37, y + 0x7C15)) - 128) * strength / 128))
return fbm8(c_uint32(x + dx), c_uint32(y + dy), 1)
samples = [0, 0xFFFFFFFF]
strengths = [0, 128, 6000]
x = samples[1]; y = samples[0]; strength = strengths[2]
# Show displacement direction at the boundary; same-field probe produces same result here.
print(f"boundary probe x=INT32_MAX+1, y=0: current warped={warp_current(x, y, strength)}, unsigned warped={warp_unsigned(x, y, strength)}")
PYRepository: MoonModules/projectMM
Length of output: 11846
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== core arithmetic includes =="
sed -n '1,35p' src/core/noise.h
sed -n '1,30p' src/core/math16.h
echo
echo "== deterministic signed-overflow model probe =="
python3 - <<'PY'
def as_int32(v):
return ctypes.c_int32(v).value if 'ctypes' in globals() else None
class C32:
`@staticmethod`
def u(v):
return (v & 0xFFFFFFFF)
`@staticmethod`
def i(v):
h = v & 0xFFFFFFFF
return h if h < (1 << 31) else h - (1 << 32)
def fbm8(x, y, octaves):
# deterministic placeholder for ordering purposes only.
return (5 * x + 7 * y + 1) % 256
def warp_current(x, y, strength):
inoise8_at = lambda xx, yy: (xx * yy * 251 + 1) % 256
dx = C32.i(C32.i(inoise8_at(x, y)) - 128) * strength // 128
dy = C32.i(C32.i(inoise8_at(x + 0x9E37, y + 0x7C15)) - 128) * strength // 128
sx = C32.i(x) + dx
sy = C32.i(y) + dy
return fbm8(C32.u(sx), C32.u(sy), 1)
def warp_unsigned(x, y, strength):
inoise8_at = lambda xx, yy: (xx * yy * 251 + 1) % 256
inoise8_at_9 = lambda xx, yy: (xx * yy * 199 + 1) % 256
dx = (C32.i(C32.i(inoise8_at(x, y)) - 128) * strength // 128) & 0xFFFFFFFF
dy = (C32.i(C32.i(inoise8_at_9(x + 0x9E37, y + 0x7C15)) - 128) * strength // 128) & 0xFFFFFFFF
return fbm8(C32.u(x + dx), C32.u(y + dy), 1)
for (x, y) in [(0xFFFFFFFFu, 0), (0x70000000u, 0), (0x70000000u, 0xFFFFFFFFu)]:
for strength in [128, 6000]:
# signed current may overflow; the model uses two's-complement wrap only for comparison.
# Add real signed-overflow coverage so callers do not rely on machine code wrap.
print(f"warp8(x={x:`#010x`}, y={y:`#010x`}, strength={strength}): current_model={warp_current(x,y,strength):0#4x}, unsigned={warp_unsigned(x,y,strength):0#4x}")
PYRepository: MoonModules/projectMM
Length of output: 3821
Keep warped coordinate arithmetic unsigned.
warp8 maps full-range fixed-point coordinates to int32_t before applying the displacement, so fbm8(...) can receive overflowing signed arithmetic for valid high coordinate values. Keep the coordinate offsets in uint32_t modulo space and add coverage for near-INT32_MAX / UINT32_MAX inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/noise.h` around lines 152 - 158, Update warp8 so coordinate
displacement is performed entirely in uint32_t modulo arithmetic, avoiding the
signed int32_t conversions when adding dx and dy before calling fbm8. Preserve
the existing displacement calculation and add coverage for inputs near INT32_MAX
and UINT32_MAX to verify wrapping behavior remains valid.
Source: Coding guidelines
Comment on lines
+92
to
+105
| // Did the effect actually put light in the buffer? "No crash" is not enough — but neither is | ||
| // "wrote something": an effect that assumes RGB on a 1-channel buffer writes two bytes PAST | ||
| // each light into its neighbours, which stays in bounds and looks like output while silently | ||
| // corrupting the frame. The caller checks that separately via a canary (see the sweep). | ||
| bool wrote = false; | ||
| for (size_t i = 0; i < layer.buffer().bytes(); i++) | ||
| if (layer.buffer().data()[i]) { wrote = true; break; } | ||
|
|
||
| // release() returns every buffer in the tree (it recurses to children); the caller | ||
| // then destroys the effect. The Layer is a local about to go out of scope, so there | ||
| // is no detach to do — and a removeChild() here would run a structural mutation over | ||
| // a just-released tree. | ||
| layer.release(); | ||
| return wrote; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the channel-count test detect out-of-bounds writes.
The test only scans bytes inside the allocated buffer. On a 1- or 2-channel fixture, an RGB write can overwrite the next light while remaining inside that allocation. The test then reports wrote == true and passes.
The comment says a canary is checked, but the changed helper and sweep do not create or verify one. Add guard bytes around the backing storage, or use a bounded test buffer with sentinels. Assert that all guards and channel boundaries remain unchanged for every cpl.
As per coding guidelines, **/*.{cpp,h,hpp,c,ino,py} must pin every new behavior with meaningful unit and scenario tests.
Also applies to: 110-130
🤖 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_Effects_gridsweep.cpp` around lines 92 - 105, Update the
channel-count sweep helper and its caller to wrap the backing storage with
sentinels and verify them after each effect run, including 1- and 2-channel
fixtures. Assert that guard bytes and per-light channel boundaries remain
unchanged for every cpl, while retaining the existing wrote detection; add
meaningful unit/scenario coverage for the new corruption checks.
Source: Coding guidelines
ewowi
changed the title
Effects that behave like matter or like a shader now compose one shared kernel instead of re-deriving the physics. Four new effects show what that buys: fireworks that decide their own apex, a ballpit that piles up, an endless Truchet weave, and a raymarched 3D scene. Everything time-varying now runs at the same speed on every target, from an ESP32 at 470 fps to a desktop at 5000. KPI: 16384lights | Desktop:1037KB | tick:132/106/4/5/133/300/21/3/300/73/18/22/297/133/23/5/46/5us | ESP32:1567KB | src:214(53114) | test:157(30763) | lizard:151w Core: - math16 gains isqrt64. dist16 was wrong far inside normal range: two coordinates of 70000 square-and-sum past UINT32_MAX, so the 32-bit root returned 65535 for a distance of 70000. - sin16/cos16 now return SIGNED -32767..32767, FastLED's lib8tion contract. The unsigned form broke any ported snippet silently, offset by half scale; eleven of our own call sites were already subtracting 32768 to undo it. - smoothFollow could fall but not climb: the shift truncated toward zero, so a small rate moved 100->99 and left 0 at 0. - noise.h states plainly that inoise8 is VALUE noise under FastLED's gradient-noise name, so a ported effect looking different is explicable. Light domain: - particles.h: an SoA pool over caller-owned buffers. Forces (gravity, force, forceSmall, drag, attract), semi-implicit Euler, walls (bounce, wrap, killOutside), collisions, three emitters, and FrameTime. ttl is 16-bit because a byte capped life at ~4.25s, and particles carry an optional size so a pool draws blobs rather than points. - shader.h: the GLSL vocabulary in fixed point — mix, fract, step, smoothstep, clamp, length, rotate, uv, repeat, mirror, the SDF operators, cosPalette — with a shader::each runner. Runs on every target. - raymarch.h: sphere tracing, 3D SDFs, gradient normals, a camera. Compiled only where the SoC declares a hardware FPU, so the float exception is a whole-header gate rather than a rule weakened in place. - opSubtract now follows Quilez's operand order in both 2D and 3D: the op* names are borrowed from his catalog, so the reverse order silently inverted any scene transcribed from Shadertoy. - New: Fireworks, Ballpit, Truchet, Raymarch. Framerate independence, now a system rule (architecture.md): - Everything time-varying is driven by elapsed time, never frame count. A faster device renders the same motion MORE SMOOTHLY, not more motion, so quantising to a fixed step and skipping frames is the wrong fix. - A permanent audit renders all 50 effects at 60 and 1200 fps over the same simulated span. It found four pre-existing failures: BouncingBalls (11.7x, an integer division truncating to zero so every ball froze), Tetrix (3.1x), Lissajous (2.4x), Echo (1.6x). All fixed; three goldens re-baselined. Tests: - Suites for the particle kernel, the shader vocabulary, and the framerate audit. Mutation-tested where a passing test proves little: halving the integrator step, and reintroducing the frame counter, both make them fail. Docs: - power-functions.md gains particle and shader sections, each opening with what the area covers rather than only listing functions. Reviews: - CodeRabbit, 15 findings: 10 fixed (kaleido's one-unit seam discontinuity, smin overflowing int32 past a blend radius of ~131000, Rings' 8-bit radius ceiling stalling ripples on a wide panel, WaterRipple wrapping int16 and inverting the surface, Dissolve's hue delta collapsing at the palette wrap, the Layer live pass, Echo's history bound, a scroll scratch truncating wide fixtures, two Windows-reserved identifiers). 5 skipped: coverage does not overflow (measured), ControlModule is deliberately top-level, and two asked for grid guards inside effects that Layer::tick already gates. - Industry-standardness audit (Fable): the two contract breaks above, plus the three particle gaps closed here. 16-bit noise is the one accepted remaining gap and lands next. Verified on an ESP32-S3: Raymarch 1.64ms at 4096 lights (409 fps), Truchet 3.0ms at 1024 lights (242 fps). Flash grew 16KB for three kernels and four effects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/light/effects/TetrixEffect.h (1)
176-177: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDelete the stale reference to the removed depth accessor.
The change removed
depthDim(). The first clause of this comment still explains why that accessor needed a>0guard. Keep only the*Controlnaming note.🧹 Proposed change
- // The grid depth accessor needs the >0 guard for the dims z extent; the width/oneColor control - // members are named *Control so they don't shadow the inherited width()/depth() accessors. + // The width/oneColor control members are named *Control so they don't shadow the inherited + // width()/depth() accessors.🤖 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/effects/TetrixEffect.h` around lines 176 - 177, Remove the stale clause referencing the removed depthDim() accessor from the nearby comment, preserving only the note that width/oneColor members use the *Control suffix to avoid shadowing inherited width()/depth() accessors.test/unit/core/unit_math16.cpp (1)
32-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStore the
sin16samples in a signed type.
sin16now returnsint16_t. Line 34 stores the four samples inuint16_t. The four sampled angles all sit in the first quadrant, so the values are positive and the assertions still hold today. If the angles change to sample the falling half, the negative results convert to large unsigned values and thea < b < c < dchain reports the opposite of the intended property.Use
int16_tso the test matches the declared signed contract.💚 Proposed fix
- const uint16_t a = sin16(0x1000), b = sin16(0x1040), c = sin16(0x1080), d = sin16(0x10C0); + const int16_t a = sin16(0x1000), b = sin16(0x1040), c = sin16(0x1080), d = sin16(0x10C0);🤖 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/core/unit_math16.cpp` around lines 32 - 39, Change the sample variables a, b, c, and d in the sin16 smoothness test to int16_t, matching sin16’s signed return type and preserving correct ordering for negative results.src/light/effects/RingsEffect.h (1)
43-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDelete the contradictory sentence in the comment.
Lines 45-46 state "Clamped to a byte because the per-ripple radius state is 8-bit." Lines 47-48 state "Kept WIDE, not clamped to a byte." The first sentence describes the previous implementation and is now wrong. The code keeps
maxRasuint16_t.Keep only the description of the current behavior.
As per coding guidelines, "Documentation must describe the system as it currently is."
📝 Proposed fix
// Visible radius limit: a TRUE distance to the far corner (dist16), where the 8-bit form // approximated an octagon and saturated at 255 — so on a panel wider than ~255 lights the - // limit stopped growing and the rings stalled short of the edge. Clamped to a byte because - // the per-ripple radius state is 8-bit. - // Kept WIDE, not clamped to a byte: on a panel whose far corner is more than 255 lights - // away the ceiling stopped growing, so every ripple died before reaching the edge. + // limit stopped growing and the rings stalled short of the edge. Kept WIDE, not clamped to + // a byte: on a panel whose far corner is more than 255 lights away the ceiling stopped + // growing, so every ripple died before reaching the edge.🤖 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/effects/RingsEffect.h` around lines 43 - 50, Remove the outdated sentence in the comment above maxR32 that says the radius is clamped to a byte because per-ripple state is 8-bit. Keep the description that maxR remains wide and is capped at uint16_t’s range, matching the current uint16_t maxR implementation.Source: Coding guidelines
src/core/math16.h (1)
92-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
map32can still overflowint64_tin the product.The operands are now widened before subtraction, which fixes the span computation. The comment on Line 94 states the product of two 32-bit spans fits in
int64, but that is not true for two full-width spans.inSpanandoutSpaneach reach2^32 - 1, andv - inLoreaches2^32 - 2whenvis not clamped.map32(INT32_MAX - 1, INT32_MIN, INT32_MAX, INT32_MIN, INT32_MAX)therefore computes about1.8e19, which exceedsINT64_MAX.Use a multiply-divide that splits the numerator, for example compute
(v - inLo) / inSpanand the remainder term separately. Add a regression case for a full-width input range mapped to a full-width output range withvstrictly inside the range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/math16.h` around lines 92 - 98, Update map32 to avoid multiplying the full-width offset by outSpan in int64_t: split the numerator into quotient and remainder terms, then perform multiply-divide steps whose intermediates remain within range while preserving the mapping result. Correct the nearby overflow comment and add a regression case covering strictly interior v with full-width input and output ranges.
🤖 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 455: Update the architecture documentation around particles::FrameTime to
describe 256 fixed-point units as one reference frame determined by the
constructor’s referenceHz parameter, noting that 60 Hz is only the default.
Remove the unconditional claim that 256 equals one 60 Hz frame while preserving
the explanation of sub-millisecond remainder handling.
- Around line 432-434: Remove the duplicated Layer::tick() empty-grid/live-pass
gate explanation on the second paragraph and reference the single existing
statement instead. Update the stale “39 effects” count to 51, matching the
*Effect classes inheriting from EffectBase.
In `@docs/moonmodules/core/control.md`:
- Around line 57-59: Move preset subtree restoration out of Scheduler::tick() so
applySubtree(), applyNode(), lifecycle construction, and prepareTree() execute
through a queued structural transaction outside the render callback. Keep
rendering quiesced only around the minimal tree-swap section, update the
documentation to describe the non-blocking behavior, and add a deterministic
test proving restore work is not invoked from Layer::tick() or another render
callback.
In `@docs/moonmodules/light/power-functions.md`:
- Line 205: Change the “Raymarching — one technique inside a shader” heading
from level four to level three so it follows the surrounding section hierarchy
and satisfies markdownlint MD001.
- Around line 172-183: Wrap each of the three new tables—Particles, Shaders, and
Raymarching—in its own <div class="mm-pf" markdown="1"> … </div> container,
matching the existing table structure elsewhere on the page and preserving all
table content unchanged.
In `@src/core/math16.h`:
- Around line 168-176: Update isqrt64 to avoid overflow in the initial Newton
iteration for UINT64_MAX, ensuring the divisor r never becomes zero and
preserving correct convergence for x values 1, 2, and 3. Add regression coverage
for isqrt64(UINT64_MAX) and these small inputs.
In `@src/light/effects/BallpitEffect.h`:
- Around line 74-82: Update the fill logic around filled_ so it detects changes
to the current balls value and regenerates the pool instead of permanently
skipping once filled_. Track the previously used count or otherwise gate
spawning on the current balls value, and ensure the balls control change invokes
prepare() or resets filled_ so subsequent count changes take effect.
In `@src/light/effects/EchoEffect.h`:
- Line 84: Remove the redundant decayNow local copy in the relevant effect
method and update the uses around lines 114-116 to reference decay directly,
preserving the existing behavior.
- Line 101: Enclose the entire nested loop guarded by if (feedbackDue) in
braces, preserving the existing loop body and control flow.
- Around line 74-84: Fix the pacing gate in the effect update logic by adding
persistent accumulated frame-scale state next to time_ and adding each
time_.advance(elapsed()) result to it; consume one FrameTime::kOne unit per
feedback pass, retaining any remainder for subsequent frames so rates above 60
FPS still trigger feedback and history capture. Add a framerate test covering 60
FPS and 1200 FPS that asserts both render a trail.
In `@src/light/effects/FireworksEffect.h`:
- Around line 57-73: Add an allocation-failure else branch in prepare() for
src/light/effects/FireworksEffect.h:57-73 and
src/light/effects/BallpitEffect.h:51-64 that resets pool_ to an empty
particles::Pool, ensuring valid() is false after failed resize operations. Add a
regression test that forces scratch allocation failure and verifies tick()
performs no writes.
- Around line 127-135: Make the launch decision time-based rather than dependent
on frame_ increments: add a launchAcc_ member, accumulate the advance() scale,
and consume one reference-frame interval before evaluating the hash-based launch
condition. Keep the existing shell initialization in the launch block, and add a
framerate test comparing burst counts over one simulated second at 60 FPS and
1200 FPS.
In `@src/light/effects/LissajousEffect.h`:
- Line 4: Move the domain-neutral FrameTime definition from light/particles.h
into src/core/ alongside BeatPhase, then update LissajousEffect and all other
users to include the new core header directly. Remove the old particles.h
definition and preserve FrameTime’s existing behavior and API so light modules
no longer depend on the particle kernel for timing.
- Around line 85-89: Remove the stale depthDim/z-extent comment near trailTime_
in LissajousEffect. Verify EffectBase declares virtual prepare(), then update
LissajousEffect::prepare() to override it and call trailTime_.reset() so
restarts discard the elapsed timing gap.
In `@src/light/effects/RingsEffect.h`:
- Around line 84-89: Remove the int16_t narrowing cast from the
negative-difference assignment in the diff calculation near dist16 and
radius_[i]. Preserve the int32_t type throughout the absolute-difference
computation so large values are not truncated before subsequent thickness
comparisons.
In `@src/light/effects/SdfShapesEffect.h`:
- Line 109: Update the sine range documentation near the sin16 call in
SdfShapesEffect to state the signed range -32767..32767, and replace the
outdated “32768 is its zero point” wording with documentation consistent with
the current signed sin16 contract. Leave the existing calculation unchanged.
In `@src/light/effects/TruchetEffect.h`:
- Around line 127-130: Update floorDiv to explicitly document its
positive-divisor precondition and signal invalid negative divisors, or revise
the implementation to correctly compute floor division for both divisor signs.
Preserve the existing b == 0 behavior and ensure future callers cannot silently
receive an incorrect result for b < 0.
In `@src/light/particles.h`:
- Around line 338-350: Update wrap() to replace the x[i] and y[i] while-loop
reductions with constant-time modulo normalization, preserving
negative-coordinate handling and the existing wrapX/wrapY and positive-dimension
guards. Confirm whether positions equal to w or h must remain at that endpoint
or map to zero, then preserve the required endpoint behavior and add a focused
test if needed.
- Around line 417-431: Update collide to compute r2 and the dx/dy squared
distance in int64_t, preventing overflow for large draw::pos_t radii while
preserving the existing collision checks. Also ensure the resulting d2 value is
handled safely by the subsequent square-root calculation, using a
64-bit-compatible path or an explicit clamp before any uint32_t-only API.
In `@src/light/raymarch.h`:
- Line 28: Remove the platform-specific compilation decision from the light
layer around the MM_HEAVY_COMPUTE conditional in raymarch.h. Move target
selection into the platform layer or platform-selected build sources, then
expose a platform-neutral light-domain interface to this header while preserving
the selected implementation behavior.
In `@src/light/shader.h`:
- Around line 127-131: Remove the stale first documentation line above
opSubtract, leaving only the description that matches the implemented max(-a, b)
operand order and preserves the Shadertoy transcription guidance.
In `@src/platform/esp32/platform_config.h`:
- Around line 209-227: Update hasHeavyCompute and MM_HEAVY_COMPUTE in
platform_config.h so the heavy per-pixel code gate enables Raymarch only on the
intended supported targets (S3/P4 and desktop), excluding classic ESP32 despite
SOC_CPU_HAS_FPU being true. Keep the runtime budget logic separate from this
compile-time gate, and revise the Raymarch documentation in
docs/moonmodules/light/effects.md lines 383-397 plus the nearby implementation
comments to describe the same supported-target policy consistently.
In `@test/unit/light/unit_Effects_framerate.cpp`:
- Around line 34-66: Extend the framerate tests around meanLit to verify
time-aligned visual output or effect state for each audited motion and feedback
effect, comparing results at equivalent simulated elapsed times across FPS
values. Keep meanLit as supplemental activity coverage, but do not use it as the
sole assertion; use the existing effect instances and deterministic
platform::setTestNowMs timeline to detect frame-rate-dependent motion.
In `@test/unit/light/unit_Effects_golden.cpp`:
- Around line 97-104: Update the header comment in the golden effects test to
document that the BouncingBallsEffect, LissajousEffect, and TetrixEffect hash
changes are intentional and result from the 60-versus-1200 fps
framerate-independence audit, while preserving the existing BeatPhase update
entries.
In `@test/unit/light/unit_Layer_zero_grid.cpp`:
- Around line 120-135: Update the empty-grid test “a live modifier is skipped on
an empty grid without crashing” to use a modifier whose hasModifyLive() returns
true instead of CountingModifier. Preserve the existing tick-count assertion and
empty-layout setup so the test specifically exercises the live REMAP gate.
In `@test/unit/light/unit_Particles.cpp`:
- Around line 276-282: Update the assertions in the “an attractor does not fling
a particle sitting on top of it” test to compare the magnitude of each velocity
component against the finite upper bound, constraining both positive and
negative blowups while preserving the existing threshold.
In `@test/unit/light/unit_Scroll.cpp`:
- Around line 188-194: Extend the test case “a strided wrap conserves every
light, whatever the rotation” to assert the final light positions after the
three scroll operations, not just preservation of s.total(). Verify the expected
one-position net rotation on the five-element axis while retaining the existing
conservation check.
In `@test/unit/light/unit_Shader.cpp`:
- Around line 32-38: Add negative-input assertions to the fract test case,
covering negative fractional and whole 16.16 values and verifying they wrap into
the expected 0..65535 fractional range. Keep the existing non-negative cases and
ensure the tests distinguish unsigned masking behavior from signed remainder
behavior.
---
Outside diff comments:
In `@src/core/math16.h`:
- Around line 92-98: Update map32 to avoid multiplying the full-width offset by
outSpan in int64_t: split the numerator into quotient and remainder terms, then
perform multiply-divide steps whose intermediates remain within range while
preserving the mapping result. Correct the nearby overflow comment and add a
regression case covering strictly interior v with full-width input and output
ranges.
In `@src/light/effects/RingsEffect.h`:
- Around line 43-50: Remove the outdated sentence in the comment above maxR32
that says the radius is clamped to a byte because per-ripple state is 8-bit.
Keep the description that maxR remains wide and is capped at uint16_t’s range,
matching the current uint16_t maxR implementation.
In `@src/light/effects/TetrixEffect.h`:
- Around line 176-177: Remove the stale clause referencing the removed
depthDim() accessor from the nearby comment, preserving only the note that
width/oneColor members use the *Control suffix to avoid shadowing inherited
width()/depth() accessors.
In `@test/unit/core/unit_math16.cpp`:
- Around line 32-39: Change the sample variables a, b, c, and d in the sin16
smoothness test to int16_t, matching sin16’s signed return type and preserving
correct ordering for negative results.
🪄 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: f6557248-c08e-4ed0-8ab8-39f11dd01cde
📒 Files selected for processing (43)
docs/architecture.mddocs/backlog/power-functions-analysis-top-down.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mddocs/moonmodules/light/effects.mddocs/moonmodules/light/power-functions.mdsrc/core/AudioLevel.hsrc/core/math16.hsrc/core/noise.hsrc/light/Palette.hsrc/light/draw.hsrc/light/effects/BallpitEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DissolveEffect.hsrc/light/effects/EchoEffect.hsrc/light/effects/FireworksEffect.hsrc/light/effects/LinesEffect.hsrc/light/effects/LissajousEffect.hsrc/light/effects/RaymarchEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/SdfShapesEffect.hsrc/light/effects/SpectrumEffect.hsrc/light/effects/TetrixEffect.hsrc/light/effects/TruchetEffect.hsrc/light/effects/WaterRippleEffect.hsrc/light/layers/Layer.hsrc/light/particles.hsrc/light/raymarch.hsrc/light/shader.hsrc/main.cppsrc/platform/desktop/platform_config.hsrc/platform/esp32/platform_config.htest/CMakeLists.txttest/unit/core/unit_fields.cpptest/unit/core/unit_math16.cpptest/unit/light/unit_Effects_framerate.cpptest/unit/light/unit_Effects_golden.cpptest/unit/light/unit_Layer_zero_grid.cpptest/unit/light/unit_Particles.cpptest/unit/light/unit_Scroll.cpptest/unit/light/unit_Shader.cpptest/unit/light/unit_Splat.cpp
Comment on lines
+57
to
+59
| Applying a preset creates, replaces and destroys modules to match what the file describes — it is a restore, not a value overlay: a preset carrying more than the device has adds it, and one describing less removes what it omits. | ||
|
|
||
| Structural mutation quiesces the render worker, and mutations run inline on the render tick, so a large restore stalls rendering for its duration. The captured subtree is applied and `prepareTree()` runs once at the end. Presets are a cold-path feature; the tick path is untouched. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/core --items all --type function,method --match 'applySubtree|prepareTree|restore|preset'
rg -n -C5 'applySubtree|prepareTree|quiesce|preset|restore' src/core src/light test
rg -n -C5 '\bvoid tick\s*\(|MM_NONBLOCKING|applySubtree|prepareTree' src/core src/lightRepository: MoonModules/projectMM
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs lines =="
sed -n '50,70p' docs/moonmodules/core/control.md
echo "== focused searches =="
rg -n -C4 'applySubtree|prepareTree\s*\(|void tick\(|tick\(\)|MM_NONBLOCKING|render tick|render tick|render tick|render.' src/core/light src/light test docs --glob '!**/unit_LightPresetsModule.cpp' --glob '!**/scenarios/**' | head -n 300
echo "== candidate files =="
git ls-files src | rg '(Light|Layer|Scheduler|Renderer|render|tick)\.(h|cpp)$|presets|subtree|Module'Repository: MoonModules/projectMM
Length of output: 44082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find relevant files =="
git ls-files | rg '(Scheduler|Renderer|render|tick|presets|control)\.(h|cpp)$|src/core|src/light' | head -n 200
echo "== focused applySubtree names =="
rg -n -C3 'applySubtree|prepareTree\s*\(|void tick\s*\(|tick\s*\(\)' src test docs --glob '*.cpp' --glob '*.h' --glob '*.md' | head -n 400
echo "== control module outline =="
fd -a 'Scheduler|Renderer|Render|Presets|Control' src/core src/light docs 2>/dev/null | head -n 100Repository: MoonModules/projectMM
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FilesystemModule outline =="
ast-grep outline src/core/FilesystemModule.h src/core/FilesystemModule.cpp --match 'applySubtree' --view expanded || true
echo "== applySubtree implementation =="
rg -n -C12 'bool FilesystemModule::applySubtree|applySubtree\(' src/core/FilesystemModule.cpp src/core/FilesystemModule.h | head -n 400
echo "== Scheduler.cpp tick / prepareTree =="
rg -n -C6 'void Scheduler::tick|prepareTree|render|tick\(|tick\s*\(' src/core/Scheduler.cpp src/core/Scheduler.h | head -n 400
echo "== FilesystemModule class and includes =="
sed -n '1,220p' src/core/FilesystemModule.h
sed -n '1,260p' src/core/FilesystemModule.cppRepository: MoonModules/projectMM
Length of output: 39700
Move preset subtree restoration off the render tick.
applySubtree() calls applyNode() and builds runtime lifecycle, and the caller keeps it on Scheduler::tick() before prepareTree(). This means module creation, replacement, and teardown plus prepareTree() can block rendering during a large preset restore; the docs should not document stalling rendering as acceptable behavior.
Queue the structural transaction outside rendering. Quiesce rendering only for the minimal tree-swap section. Add a deterministic test that proves restore work does not run from Layer::tick() or another render callback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/core/control.md` around lines 57 - 59, Move preset subtree
restoration out of Scheduler::tick() so applySubtree(), applyNode(), lifecycle
construction, and prepareTree() execute through a queued structural transaction
outside the render callback. Keep rendering quiesced only around the minimal
tree-swap section, update the documentation to describe the non-blocking
behavior, and add a deterministic test proving restore work is not invoked from
Layer::tick() or another render callback.
Source: Path instructions
Effects now behave the same whatever framerate the hardware renders at: the same settings give the same motion per second on a 30 fps wall and a 5,000 fps desktop, with the faster device simply drawing it more smoothly. Adds a particle kernel and a shader tier to the power-function library, with five new effects built on them. desktop 140us/7,142fps | esp32 4,164us/240fps | flash: desktop 1,075KB (+1KB), esp32 1,704KB, esp32s3-n16r8 1,699KB (+3KB) Core - math16: isqrt64 no longer overflows its first Newton step, which returned 0 for UINT64_MAX and would have read as zero separation once collide() started using it. Seeded from a power-of-two bound instead. - noise: 16-bit tier (inoise16 1/2/3D, fbm16). lerp16 widens its product to int64 (the 32-bit form was signed overflow on roughly a quarter of samples), and fbm16 sums octaves at full width rather than shifting each down by 8, which had made the output 8-bit wearing a 16-bit type: 195 distinct values over 20,000 samples, now 15,118. Light domain - particles.h: the kernel. SoA pool over caller-owned buffers, forces, semi-implicit Euler, walls, collisions, emitters, and FrameTime as the shared elapsed-time scale. - shader.h: the GLSL vocabulary in fixed point, plus project/depthFade. - raymarch.h: sphere tracing, gated on SOC_CPU_HAS_FPU so small fixtures get it too, not desktop only. - collide() computes distance in int64 end-to-end, including the root, so a large contact radius no longer truncates. - wrap() reduces by modulo instead of per-span loops: constant time whatever the overshoot, and behaviour-identical to the loops it replaces, including the asymmetric endpoints (a multiple lands on span from above, on 0 from below). - Echo, Lissajous, Ballpit, Particles and StarField moved onto elapsed time. Echo spends every whole reference frame a tick covers rather than one, so below 60 fps the trail compounds at 60 Hz instead of at the render rate; it was measured running 1 feedback pass per second at 240 fps instead of 60. Particles carries its sub-step remainder, so a slow speed still moves at high fps rather than truncating to a standstill. - New effects: VectorBalls, plus the particle and shader showcases. Tests - unit_Effects_framerate.cpp audits all 51 effects at 60 vs 1200 fps, with targeted cases for the two mechanisms the aggregate cannot see. - Coverage for isqrt64 across the full 64-bit range, the 16-bit noise properties, wrap endpoints, scroll positions, fract on negatives, and the attractor's magnitude bound. - The empty-grid live-modifier test now uses a modifier that returns true from hasModifyLive(), so it exercises the gate it names. - Echo, BouncingBalls, Lissajous and Tetrix goldens recaptured: what they draw on a given frame changed, the motion per second did not. Docs/CI - power-functions.md gains the particles and shader sections; caller lists record Particles. - architecture.md states the tick-rate rule and drops a duplicated paragraph. - The migration rule lives with the library rather than on the effect catalog. - Backlog: worley, and the remaining 16-bit noise tuning. Reviews - CodeRabbit, 15 findings: fixed the substantive ones; skipped the two grid guards, since Layer::tick already gates on hasGrid and a guard in an effect is both dead code and an orchestration violation. - Reviewer (Fable), 10 findings + 4 nits: 8 fixed. Skipped map32's overflow, as it needs both spans near 2^32 and every call site maps extents or bytes; documented the bound instead. Skipped moving FrameTime to core and abstracting raymarch behind a platform interface: both are real, both are refactors across every caller rather than review fixes. Known, unfixed: Fireworks still counts frames for its launch roll. It has two framerate bugs that currently cancel (frame-counted launches, and a fadeToBlackBy floor that erases the trail faster on a fast device); fixing one exposes the other, and fixing both changes how the effect looks, so it gets its own commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/light/effects/RingsEffect.h (1)
82-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse 32-bit coordinate deltas.
int16_t dxandint16_t dytruncate a coordinate gap above 32767 beforedist16receives it. A panel wider than 32767 lights can therefore render distant pixels as near the ring centre.Convert both operands to
int32_tbefore subtracting. Add a regression test with a ring centre and pixel separated by more than 32767 lights.Proposed fix
- int16_t dx = static_cast<int16_t>(x - cx_[i]); - int16_t dy = static_cast<int16_t>(y - cy_[i]); + const int32_t dx = static_cast<int32_t>(x) - static_cast<int32_t>(cx_[i]); + const int32_t dy = static_cast<int32_t>(y) - static_cast<int32_t>(cy_[i]);🤖 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/effects/RingsEffect.h` around lines 82 - 89, Update the coordinate deltas in the ring-rendering loop around dist16 to compute x - cx_[i] and y - cy_[i] as int32_t, avoiding int16_t narrowing before distance calculation. Add a regression test using a ring centre and pixel separated by more than 32767 lights, verifying the distant pixel is not treated as near the ring centre.Source: Coding guidelines
test/unit/light/unit_Effects_framerate.cpp (2)
71-105: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAssert time-aligned state, not only lit-pixel averages.
meanLitcombines source, trail, and shell pixels. An Echo feedback gate that runs too rarely can still pass because the source renders every frame. The Fireworks assertion is one-sided:fast == 0still satisfiesfast < slow * 2. Compare deterministic frames or effect state at equal elapsed times, and assert both lower and upper bounds for the launch and trail behavior.Also applies to: 111-132
🤖 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_Effects_framerate.cpp` around lines 71 - 105, Replace the meanLit-only checks in the framerate test and the related Fireworks assertions with deterministic frame or effect-state comparisons at equal elapsed times. Ensure the assertions independently enforce both lower and upper bounds for launch and trail behavior, including the fast == 0 case, so source pixels cannot mask an under-updating Echo feedback gate.Source: Coding guidelines
18-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse fractional milliseconds for high-FPS time steps.
meanLit(..., 1200, ...)computes1000 / fpsin integer milliseconds, so each 1200 FPS frame advances time by0 ms. This can freeze elapsed time instead of sampling the intended interval. Accumulate fractional milliseconds and advanceplatform::setTestNowMsby the same effective interval for each rate.🤖 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_Effects_framerate.cpp` around lines 18 - 69, Update meanLit to accumulate elapsed time using fractional milliseconds rather than truncating 1000 / fps per frame, ensuring high-FPS cases such as 1200 advance simulated time correctly. Use the accumulated effective interval when calling platform::setTestNowMs while preserving the existing start offset and cleanup behavior.docs/backlog/power-functions-analysis-top-down.md (1)
68-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a rounding-safe expression for the bounce scaling comment.
The existing particle kernel uses
scaleSigned(vx[i], e, 256); the-(v*e)>>8comment here does not match that because the shift does not round toward zero like the canonical helper. Use the same fixed-point helper name here or express rounding explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backlog/power-functions-analysis-top-down.md` at line 68, Update the bounce scaling comment near pool.bounce to match the canonical scaleSigned(v, e, 256) behavior, or explicitly describe rounding toward zero; do not retain the unrounded -(v*e)>>8 expression.src/core/math16.h (2)
140-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winVerify the long-running phase result against a full 128-bit reference.
num_ * scaleexceedsuint64_tcapacity whennum_is larger thanUINT64_MAX / 65536; C++ unsigned multiplication then discards the upper 64 bits before the/ 60000u. Use an upper-bound test that cannot be duplicated by the wrapping result and proves the lower 32-bit phase is still correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/math16.h` around lines 140 - 142, Update the const phase(uint32_t scale) method to avoid relying on the overflowing uint64_t product when num_ exceeds UINT64_MAX / 65536. Add an upper-bound test using widened arithmetic or equivalent quotient/remainder reasoning that cannot be reproduced by the wrapped multiplication, and verify the resulting lower 32-bit phase against a full 128-bit reference while preserving the existing result for non-overflowing inputs.Source: Coding guidelines
231-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not negate
INT32_MINinatan16.Lines 234-235 execute
-xor-yasint32_t.atan16(INT32_MIN, ...)has signed overflow undefined behavior. Use widened magnitude arithmetic, likedist16, and keep results in a wider unsigned type for the fold.Add
INT32_MINaxis and diagonal regression cases foratan16.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/math16.h` around lines 231 - 252, Update atan16’s magnitude-folding logic to widen x and y before taking absolute values, avoiding signed overflow for INT32_MIN and retaining magnitudes in a sufficiently wide unsigned type through ratio calculation. Preserve the existing octant interpolation and quadrant/sign mapping, and add regression coverage for INT32_MIN on both axes and the diagonal.Source: Coding guidelines
src/light/effects/TetrixEffect.h (1)
183-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
fallTime_inprepare().
prepare()does not callfallTime_.reset(), so a re-prepare followed bytick()consumes the elapsed idle interval and can advance start-roll state past its stall cap while the column positions are still in start-delay. AddfallTime_.reset()inprepare()and add a re-prepare after an elapsed-gap case.🤖 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/effects/TetrixEffect.h` around lines 183 - 184, Update TetrixEffect::prepare() to call fallTime_.reset() whenever the effect is prepared, preventing elapsed idle time from carrying into the next tick. Add coverage for re-preparing after an elapsed gap, verifying the subsequent tick preserves the start-delay stall behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Line 29: Update the MoonLive requirements prose to use “built-in table”
consistently, including the referenced occurrences around the additional
locations, replacing “builtin table” or “Builtin table” without changing the
surrounding requirements.
- Around line 28-31: Reconcile the migration-status claims across the summary,
§5, and convergence sections using one verified source of truth. Update phase
completion, current-work labels, and individual effect statuses—including
particles, StarField, BouncingBalls, StarSky, and Tetrix—so they consistently
reflect the same verified state; do not claim phases ④ or ⑤ are complete while
their listed work remains unresolved.
- Around line 23-25: Document the Canvas API migration in docs/MIGRATING.md:
describe replacing the draw::pixel/get/line/blur/fade/fill and related (Buffer&,
Coord3D dims) overloads with the draw::Canvas forms, and identify affected
callers. Until the Buffer overloads are removed, explicitly mark them as
internal.
In `@docs/moonmodules/light/effects.md`:
- Around line 366-378: Update the VectorBalls performance sentence to remove the
unconditional “3 µs a frame” claim, or qualify it with the benchmark’s hardware,
panel dimensions, and relevant control values including size. Keep the existing
point-count comparison and showcase context intact.
In `@src/core/noise.h`:
- Around line 67-75: Update lerp16 to use a 65535 denominator so t == 65535
returns b exactly while preserving the documented interpolation range. Verify
inoise16 maintains continuity between the final fractional coordinate of one
cell and the next cell boundary, and add meaningful unit and scenario tests
covering the endpoint and adjacent-cell continuity.
In `@src/light/effects/EchoEffect.h`:
- Around line 111-115: The feedback calculation in the feedbackDue path only
compounds keep, so transforms represented by a and s are applied once regardless
of elapsed passes. Update the transform composition alongside the keep loop to
apply rotation and scale for every pass, preserving single-pass behavior; add a
golden regression comparing 30 fps and 60 fps output.
- Around line 74-87: Correct FrameTime so FrameTime{60} accumulates dt *
referenceHz against a 1000 ms denominator while preserving fractional remainder,
and add a unit test verifying exactly 60 * FrameTime::kOne units after one
second from initialization. In src/light/effects/EchoEffect.h:74-87, retain
fractional feedbackAcc_ pacing; in src/light/effects/BallpitEffect.h:91-102, use
the corrected shared FrameTime scale and add framerate scenarios for both
effects. Update the reference-rate contract documentation in
docs/architecture.md:455 and docs/moonmodules/light/power-functions.md:190.
In `@src/light/effects/ParticlesEffect.h`:
- Line 127: Guard the trail copy at the call site around memcpy in
ParticlesEffect so it returns without copying when the current output buffer is
smaller than trail_.bytes(), preventing writes during a live shrink before
prepare() runs. Preserve the existing partial-plane copy behavior for 3D layers,
and add a regression test covering resize-before-prepare.
In `@src/light/effects/StarFieldEffect.h`:
- Around line 105-111: The projection-to-pixel conversion currently multiplies
16.16 coordinates in int32_t, allowing overflow before bounds checks. In
src/light/effects/StarFieldEffect.h lines 105-111, compute sx and sy using
int64_t, validate their bounds, then narrow to int; apply the same change to px
and py in src/light/effects/VectorBallsEffect.h lines 100-103. Use a shared
helper if both effects retain this conversion.
In `@src/light/effects/TruchetEffect.h`:
- Around line 125-131: Update floorDiv to avoid negating the dividend: compute
the quotient and remainder, then decrement the quotient only for a negative
dividend with a nonzero remainder, while retaining the b <= 0 behavior. Add a
regression test covering INT32_MIN with a positive tile size and verify the
mathematically floored result.
In `@src/light/particles.h`:
- Around line 451-458: Widen the coordinate differences at their calculation
site, using int64_t operands so subtraction cannot overflow before the d2
computation in the particle collision response. Update the broad-phase
absolute-distance checks to use a non-overflowing widened type, then retain the
existing squared-distance and response logic. Add a regression test covering
particles positioned near opposite draw::pos_t limits and verify the system
degrades visibly without crashing.
In `@src/light/shader.h`:
- Around line 200-205: Update project to compute each projected coordinate in a
wide integer type, validate the quotient against INT32_MIN and INT32_MAX before
assigning to outX or outY, and return false when either coordinate is out of
range. Preserve the existing z <= 0 rejection and successful int32 assignments,
and add boundary tests covering both limits and overflow cases.
In `@test/unit/core/unit_noise.cpp`:
- Around line 72-89: Replace the doctest::Approx comparisons in the two lerp16
worst-case checks with direct equality against the exact expected integer values
49151 and 16383. Keep the surrounding interpolation and monotonicity checks
unchanged.
In `@test/unit/light/unit_Particles.cpp`:
- Around line 280-283: Update the velocity assertions for t.vx[0] and t.vy[0] to
avoid std::abs on signed values; widen each value to int64_t before comparing
against the threshold, or use explicit lower- and upper-bound checks while
preserving the intended magnitude limit.
---
Outside diff comments:
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Line 68: Update the bounce scaling comment near pool.bounce to match the
canonical scaleSigned(v, e, 256) behavior, or explicitly describe rounding
toward zero; do not retain the unrounded -(v*e)>>8 expression.
In `@src/core/math16.h`:
- Around line 140-142: Update the const phase(uint32_t scale) method to avoid
relying on the overflowing uint64_t product when num_ exceeds UINT64_MAX /
65536. Add an upper-bound test using widened arithmetic or equivalent
quotient/remainder reasoning that cannot be reproduced by the wrapped
multiplication, and verify the resulting lower 32-bit phase against a full
128-bit reference while preserving the existing result for non-overflowing
inputs.
- Around line 231-252: Update atan16’s magnitude-folding logic to widen x and y
before taking absolute values, avoiding signed overflow for INT32_MIN and
retaining magnitudes in a sufficiently wide unsigned type through ratio
calculation. Preserve the existing octant interpolation and quadrant/sign
mapping, and add regression coverage for INT32_MIN on both axes and the
diagonal.
In `@src/light/effects/RingsEffect.h`:
- Around line 82-89: Update the coordinate deltas in the ring-rendering loop
around dist16 to compute x - cx_[i] and y - cy_[i] as int32_t, avoiding int16_t
narrowing before distance calculation. Add a regression test using a ring centre
and pixel separated by more than 32767 lights, verifying the distant pixel is
not treated as near the ring centre.
In `@src/light/effects/TetrixEffect.h`:
- Around line 183-184: Update TetrixEffect::prepare() to call fallTime_.reset()
whenever the effect is prepared, preventing elapsed idle time from carrying into
the next tick. Add coverage for re-preparing after an elapsed gap, verifying the
subsequent tick preserves the start-delay stall behavior.
In `@test/unit/light/unit_Effects_framerate.cpp`:
- Around line 71-105: Replace the meanLit-only checks in the framerate test and
the related Fireworks assertions with deterministic frame or effect-state
comparisons at equal elapsed times. Ensure the assertions independently enforce
both lower and upper bounds for launch and trail behavior, including the fast ==
0 case, so source pixels cannot mask an under-updating Echo feedback gate.
- Around line 18-69: Update meanLit to accumulate elapsed time using fractional
milliseconds rather than truncating 1000 / fps per frame, ensuring high-FPS
cases such as 1200 advance simulated time correctly. Use the accumulated
effective interval when calling platform::setTestNowMs while preserving the
existing start offset and cleanup behavior.
🪄 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: c29808f4-27fc-43da-add3-02033dff1cb7
📒 Files selected for processing (31)
docs/architecture.mddocs/backlog/backlog-light.mddocs/backlog/power-functions-analysis-top-down.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/light/effects.mddocs/moonmodules/light/power-functions.mdsrc/core/math16.hsrc/core/noise.hsrc/light/effects/BallpitEffect.hsrc/light/effects/EchoEffect.hsrc/light/effects/LissajousEffect.hsrc/light/effects/ParticlesEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/SdfShapesEffect.hsrc/light/effects/StarFieldEffect.hsrc/light/effects/TetrixEffect.hsrc/light/effects/TruchetEffect.hsrc/light/effects/VectorBallsEffect.hsrc/light/particles.hsrc/light/shader.hsrc/main.cpptest/unit/core/unit_math16.cpptest/unit/core/unit_noise.cpptest/unit/light/unit_Effects_framerate.cpptest/unit/light/unit_Effects_golden.cpptest/unit/light/unit_Layer_zero_grid.cpptest/unit/light/unit_Particles.cpptest/unit/light/unit_ParticlesEffect.cpptest/unit/light/unit_Scroll.cpptest/unit/light/unit_Shader.cpp
Comment on lines
+23
to
+25
| - ✅ **The 22-effect boilerplate dies with one struct:** `draw::Canvas{buf, dims, cpl}`, returned by `EffectBase::canvas()`. Taken as `const Canvas&` (measured: a non-const reference costs ~3% more instructions in a tight per-pixel loop, because the extents become memory re-loads the compiler cannot hoist past a possible alias with the buffer; passing dims by value avoids that today). The gain is **correctness, not speed**: buffer and dims are currently two independent arguments nothing checks for agreement, and the pairing becomes unrepresentable-if-wrong — plus the 16 `depthDim()` copies are deleted rather than centralised, and `splat`/SDF-coverage/projection get a home for their context instead of adding loose parameters at every call site. The existing `(Buffer&, dims)` overloads remain during migration and their removal is **mandatory, not aspirational** — a permanent two-API window is worse than either option alone. | ||
|
|
||
| **Const means the surface, not the pixels.** Every draw call takes `const Canvas&` yet writes to the buffer: the Canvas is a *descriptor* (a raw pointer plus extents), so const protects the description — nothing can retarget a call to a different buffer or silently change the extents mid-frame — while the pixels behind the pointer stay writable. This is the same shape as a `std::span` passed by const reference. Stated because a call that mutates through a `const&` is otherwise a surprise **(proposal)**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'MIGRATING.md' .
rg -n -C3 'Buffer.*dims|draw::Canvas|Canvas' src docs testRepository: MoonModules/projectMM
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- MIGRATING.md ---\n'
cat -n docs/MIGRATING.md
printf '\n--- draw.h public Canvas/Buffer section ---\n'
sed -n '1,180p' src/light/draw.h | cat -n
printf '\n--- draw.h Canvas primitives around Buffer compat section ---\n'
sed -n '333,410p' src/light/draw.h | cat -n
printf '\n--- callers of old overload names excluding draw.h definitions ---\n'
python3 - <<'PY'
import re, pathlib
for path in pathlib.Path('.').rglob('*'):
if '.git' in path.parts or not path.is_file(): continue
s = path.read_text(errors='ignore')
if path.name == 'draw.h':
continue
# Match simple call patterns only enough to detect old 4-arg pixel/get/blur/etc style usage
for m in re.finditer(r'\b(draw::|)(pixel|get|blendPixel|addPixel|offsetOf|line|blur|fill|fillRect|rect|circles?|text|glyph)\s*\([^)]*\)', s):
line_no = s.count('\n', 0, m.start()) + 1
text = m.group(0)
if 'Buffer&' in text or re.search(r'\([A-Za-z_][A-Za-z0-9_&\[\] ,]*,\s*Coord3D\s+dims\b', text):
continue
if 'Canvas' in text or f'::pixel(cv,' in text or 'draw::pixel(cv' in text or '.pixel(cv,' in text:
continue
# Keep lines mentioning draw overloads with likely Buffer/dims call shape
if any(op in text for op in ['draw::', '::pixel(', '::get(', '::blur(', '::line(']):
print(f'{path}:{line_no}: {text[:220]}')
PYRepository: MoonModules/projectMM
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- MIGRATING.md ---'
cat -n docs/MIGRATING.md
echo
echo '--- draw.h public Canvas/Buffer section ---'
sed -n '1,180p' src/light/draw.h | cat -n
echo
echo '--- draw.h Canvas primitives around Buffer compat section ---'
sed -n '333,410p' src/light/draw.h | cat -n
echo
echo '--- legacy Buffer+Coord3D draw callers in non-header source/tests (patterned scan) ---'
python3 - <<'PY'
import pathlib, re
def is_header(path):
return path.suffix in {'.h', '.hpp'}
def likely_legacy_call(line):
calls = [
'draw::pixel(',
'draw::get(',
'draw::blur(',
'draw::line(',
'draw::fill(',
'draw::rect(',
'draw::circle(',
'draw::text(',
'draw::glyph(',
'pixel(buf,',
'get(buf,',
'blur(buf,',
'line(buf,',
]
lower = line.lower()
for c in calls:
if c.lower() in lower:
return True
if 'Coord3D' in line and any(x in line for x in ['draw::pixel', '.pixel(', 'pixel(']) and 'Canvas' not in line:
return True
return False
for path in sorted(pathlib.Path('.').rglob('*')):
if '.git' in path.parts or not path.is_file():
continue
text = path.read_text(errors='ignore')
for i, line in enumerate(text.splitlines(), 1):
if likely_legacy_call(line):
print(f'{path}:{i}: {line.strip()[:220]}')
PYRepository: MoonModules/projectMM
Length of output: 41268
Log the Canvas API break in docs/MIGRATING.md.
src/light/draw.h exposes both draw::pixel/get/line/blur/fade/fill... Canvas forms and the matching (Buffer&, Coord3D dims) forms, and docs/MIGRATING.md does not record the upcoming removal of the Buffer forms. Until those are removed, record the Canvas replacement and affected callers or explicitly mark those old forms as internal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/backlog/power-functions-analysis-top-down.md` around lines 23 - 25,
Document the Canvas API migration in docs/MIGRATING.md: describe replacing the
draw::pixel/get/line/blur/fade/fill and related (Buffer&, Coord3D dims)
overloads with the draw::Canvas forms, and identify affected callers. Until the
Buffer overloads are removed, explicitly mark them as internal.
Source: Coding guidelines
Comment on lines
+28
to
+31
| - ✅ **Migration order is by leverage, cheapest risk first:** *(phases ① ② ④ ⑤ done; ③'s kernel is built and the five convergences are 🔨 open)* ① `beatPhase` + `map16` + `Canvas` (mechanical, pixel-identical, kills the three biggest hand-roll counts) → ② geometry + bars (4 audio effects) → ③ `splat` + `particles`, converging the five particle-shaped effects (bench-judged, the PS-replaces-twin decision) → ④ fields + polar (LavaLamp/Metaballs/Rings/Spiral) → ⑤ hidden-modifier extraction as encountered (FreqSaws `invert` first). Each pixel-identical claim is pinned by a **golden-frame test** (fixed seed, fixed time, byte-compare) — a new, small test harness capability. | ||
| - 🔨 **MoonLive exposure is stage 3 and states only its requirements here:** a builtin table of ≥ 64 entries, typed multi-arg host calls (up to 6 args + return), the symbols `x/y/z/w/h/d/time` (already threaded to the runtime, unexposed), and a per-frame entry point alongside the per-pixel one — the bottom-up's feasibility math says scripts *compose* kernels per frame; they do not interpret per pixel on large surfaces. The calling convention itself belongs to the livescripts engine work. | ||
| - ✅ 📖 **Measured on hardware (ESP32-S3, 240 MHz, 128×128, 2026-08-06)** *(the per-effect numbers now live in [effects.md](../moonmodules/light/effects.md) beside each effect)* — the theoretical budget below was an upper bound; these are the real numbers, and they reframe it. Today's *existing* effects already cost **305–692 cycles/pixel** and run at **21–48 fps** on a 128×128 panel, so "292 cycles/pixel at 50 fps" describes a frame rate this fixture size does not reach in the first place, with or without power functions. What the budget genuinely constrains is *added* cost per pixel, and the measured SDF forms are small against that: `sdBox` ≈ 6, squared-distance `sdCircle` ≈ 14, and the full `isqrt` form ≈ 108 cycles/pixel (desktop instruction counts; the ESP32 divide penalty makes the last one worse, the first two barely move). A squared-form SDF plus `smin` plus a palette lookup is a fraction of what Plasma already spends. **Design consequence:** the squared forms are the default path and the sqrt form is opt-in for true distance (outline width, linear glow). | ||
| - ✅ **Budgets are stated per family and gated:** the render loop's ceiling stays the bottom-up's 293 cycles/pixel at 128×128@50; the particle budget is ~40 cycles/particle/frame (2048 particles ≈ 0.34 ms at 240 MHz); every function gets a host micro-benchmark and the migrations ride the existing `collect_kpi` gate. Zero static RAM for everything unused (`check_footprint`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
doc='docs/backlog/power-functions-analysis-top-down.md'
rg -n -C2 'converg|showcases built|TruchetEffect|Canvas preambles|depthDim' "$doc"
rg -n '\bdepthDim\s*\(' src/light/effects
rg -n 'Canvas' src/light/effectsRepository: MoonModules/projectMM
Length of output: 20760
Reconcile the migration status before release.
The top migration summary says phases ① and ② are done, but §5 still lists only Foundations as ✅ and Marks particles as the current work with five convergences open. The convergence section says Particles and StarField are converged but also keeps BouncingBalls as a work item and marks StarSky/Tetrix as NOT converged, contradicting “phases ① ② ④ ⑤ done/③ open”. Use one verified status source for these claims. Also applies to lines 86-115, 129-137, 238-245.
🧰 Tools
🪛 LanguageTool
[grammar] ~29-~29: Ensure spelling is correct
Context: ... states only its requirements here:** a builtin table of ≥ 64 entries, typed multi-arg ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/backlog/power-functions-analysis-top-down.md` around lines 28 - 31,
Reconcile the migration-status claims across the summary, §5, and convergence
sections using one verified source of truth. Update phase completion,
current-work labels, and individual effect statuses—including particles,
StarField, BouncingBalls, StarSky, and Tetrix—so they consistently reflect the
same verified state; do not claim phases ④ or ⑤ are complete while their listed
work remains unresolved.
Source: Coding guidelines
Comment on lines
+67
to
+75
| /// Linear interpolate a→b by t/65535. | ||
| constexpr uint16_t lerp16(uint16_t a, uint16_t b, uint16_t t) { | ||
| const int32_t delta = static_cast<int32_t>(b) - static_cast<int32_t>(a); | ||
| // 64-bit intermediate: `delta * t` reaches 4.29e9 against an INT32_MAX of 2.15e9, so the 32-bit | ||
| // form was signed overflow (undefined behaviour) on roughly a quarter of samples. It happened to | ||
| // produce the right low bits on wrap-around hardware, which is what kept it invisible. | ||
| return static_cast<uint16_t>(static_cast<int32_t>(a) | ||
| + static_cast<int32_t>((static_cast<int64_t>(delta) * t) >> 16)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make lerp16 reach its documented endpoint.
At t == 65535, the right shift divides by 65536, so lerp16(a, b, 65535) stops short of b when a != b. inoise16 then has a one-unit discontinuity between the last fractional coordinate in a cell and the next cell boundary.
Use a denominator of 65535, or define the fraction as 0..65536 and update every caller. Add endpoint and adjacent-cell continuity tests.
As per coding guidelines, “Every behavior must be covered by meaningful unit and scenario tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/noise.h` around lines 67 - 75, Update lerp16 to use a 65535
denominator so t == 65535 returns b exactly while preserving the documented
interpolation range. Verify inoise16 maintains continuity between the final
fractional coordinate of one cell and the next cell boundary, and add meaningful
unit and scenario tests covering the endpoint and adjacent-cell continuity.
Source: Coding guidelines
Comment on lines
+451
to
+458
| // 64-bit: a large contact radius squares past int32 (dx and dy are sub-pixel). | ||
| const int64_t d2 = static_cast<int64_t>(dx) * dx + static_cast<int64_t>(dy) * dy; | ||
| if (d2 > r2 || d2 == 0) continue; // not touching, or exactly coincident | ||
|
|
||
| // Elastic response along the line of centres, equal masses: the pair swaps the | ||
| // component of velocity that points at the other particle and keeps the tangential | ||
| // part. Scaled by `e` so contacts can lose energy. | ||
| const int32_t d = static_cast<int32_t>(isqrt64(static_cast<uint64_t>(d2))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Widen the coordinate difference before squaring.
dx and dy are calculated before Line 452 promotes them to int64_t. If two positions are near opposite draw::pos_t limits, the subtraction overflows first. Signed overflow is undefined. The 64-bit multiplication cannot recover the lost value.
Compute the deltas in int64_t. Perform broad-phase absolute-distance checks in a non-overflowing type before calculating d2. Add a regression test with extreme coordinates.
As per coding guidelines, “For any input, order, or size, degrade visibly rather than crash.”
🤖 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/particles.h` around lines 451 - 458, Widen the coordinate
differences at their calculation site, using int64_t operands so subtraction
cannot overflow before the d2 computation in the particle collision response.
Update the broad-phase absolute-distance checks to use a non-overflowing widened
type, then retain the existing squared-distance and response logic. Add a
regression test covering particles positioned near opposite draw::pos_t limits
and verify the system degrades visibly without crashing.
Source: Coding guidelines
Effects calibrated against 60 fps ran about 4% fast, because the shared frame timer derived its reference period as 1000/60 and got 16 ms — a 62.5 Hz clock. Separately, two effects painted only the pixels they lit, so they inherited whatever was on screen: their own trails as permanent ghosts, and the previous effect's whole picture as background. desktop 140us/7,142fps | esp32 4,164us/240fps | flash: desktop 1,075KB, esp32 1,704KB, esp32s3-n16r8 1,699KB Core - FrameTime carries the undivided numerator and divides late, the rule BeatPhase already follows. One unit is 1000/(256*60) = 0.065 ms, so a remainder held in whole milliseconds cannot represent it: the truncated time was discarded, and because the amount discarded differs by render rate (11 units a frame at 60 fps, under 1 at 200) it was itself a framerate dependency. A simulated second now measures exactly 60 reference frames at 60 fps. - atan16 folds through unsigned magnitudes. Negating INT32_MIN has no int32 representation, so the previous fold was undefined behaviour at one input per axis — reachable from any unclamped coordinate difference. Confirmed by UBSan, which now runs clean over the range. Light domain - SdfShapes and Tetrix write their background instead of skipping it. The Layer does not clear between frames (ADR-0003), so an effect that only writes lit pixels shows the previous effect's frame around its own. Tetrix predates this branch. - shader::project rejects a projection that does not fit int32 rather than truncating it, which wrapped geometry off one edge of the panel to the other. - Truchet's floorDiv adjusts the quotient rather than negating the dividend, and Rings keeps its coordinate deltas in int32 — the same INT32_MIN class as atan16. - Fireworks launches at launchRate 1-3. Its own control minimum is 1, and the rate divided by 4 truncated those to zero, so the three slowest settings the UI offers never launched anything. - BouncingBalls carries its trail fade instead of rounding up to 1, which at high fps applied many times the intended decay. - VectorBalls uses shader::rotate rather than its own copy of it. Tests - A new sweep ticks all 51 effects against a DIRTY buffer, which is what a device hands an effect on every frame after the first. Golden tests render into a zeroed buffer, so the pixels an effect never writes are black by luck and hash correctly — this is the shape of bug they cannot see. It found the Tetrix case on its first run. DemoReel and NetworkReceive are exempt with reasons: one delegates its frame to a child, the other blocks in recvfrom. - Coverage for the exact reference rate, atan16 at INT32_MIN, and project at the limits of its range. - Seven goldens recaptured: the reference-rate fix changes what every FrameTime user draws on a given frame, and makes the motion per second correct rather than merely consistent. - BouncingBalls is recorded at a 1.40 framerate band with the reason in the test, rather than widening the band for all 51. Its motion is time-driven and correct; its trail rendering is what drifts, and it is the effect still to move onto the particle kernel. Docs/CI - architecture.md states why the frame timer divides late. - lessons.md: four lessons from this branch — verifying a ported function's sign against upstream, asserting on a distribution rather than eyeballing it, compensating bugs that cancel, and proving a "no-op" primitive rewrite by exhaustive comparison. Reviews - Reviewer (Fable) over the branch diff, 4 fixed. Skipped: moving FrameTime to core and the Buffer-form draw API removal (both real, both refactors across every caller); the uncalled power-function surface (the planned MoonLive builtin set); em-dash prose sweep (opportunistic per coding-standards). - CodeRabbit, 8 fixed. Skipped: lerp16's 65535 denominator, since t is a fraction of 65536 and changing it would bias every interpolation to correct one endpoint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/unit/light/unit_Effects_gridsweep.cpp`:
- Around line 201-216: Update the test around the initial layer.tick() sequence
to inspect the buffer immediately after the first tick and require that no stale
pixels remain. Retain the later tick loop only if it verifies a distinct
animation behavior, and make each assertion or test description clearly state
the user-visible behavior being validated.
In `@test/unit/light/unit_Particles.cpp`:
- Around line 661-667: Update the frame-advance loop in the particle timing test
to include the sample at f == fps, ensuring it always covers the full 1000 ms
interval. Replace the broad refFrames bounds with a fixed-point-sized tolerance
around the expected 60 reference frames, and make the test description state the
user-visible timing behavior.
🪄 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: 4f0c8f4b-657e-4fda-81d7-49835e114a88
📒 Files selected for processing (21)
docs/architecture.mddocs/backlog/power-functions-analysis-top-down.mddocs/history/lessons.mddocs/moonmodules/light/effects.mdsrc/core/math16.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/FireworksEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/SdfShapesEffect.hsrc/light/effects/TetrixEffect.hsrc/light/effects/TruchetEffect.hsrc/light/effects/VectorBallsEffect.hsrc/light/particles.hsrc/light/shader.htest/unit/core/unit_math16.cpptest/unit/core/unit_noise.cpptest/unit/light/unit_Effects_framerate.cpptest/unit/light/unit_Effects_golden.cpptest/unit/light/unit_Effects_gridsweep.cpptest/unit/light/unit_Particles.cpptest/unit/light/unit_Shader.cpp
Comment on lines
+201
to
+216
| for (int f = 0; f < 90; f++) { | ||
| mm::platform::setTestNowMs(100000u + static_cast<uint32_t>(f) * 16u); | ||
| layer.tick(); | ||
| } | ||
| mm::platform::setTestNowMs(0); | ||
|
|
||
| int untouched = 0; | ||
| for (mm::nrOfLightsType i = 0; i < lights; i++) | ||
| if (buf[i * 3] == kStale && buf[i * 3 + 1] == kStale && buf[i * 3 + 2] == kStale) | ||
| untouched++; | ||
|
|
||
| INFO("effect: " << effectName); | ||
| CAPTURE(untouched); | ||
| // A few stale pixels are possible where an effect legitimately paints a static subset; | ||
| // a whole inherited frame is not. Half the grid is the line between the two. | ||
| CHECK(untouched < static_cast<int>(lights) / 2); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Validate the first dirty-buffer frame.
This test checks the buffer only after 90 ticks. An effect can expose stale pixels on its first frame and overwrite them later, then pass this test.
Tick once after initializing the stale buffer. Require that no stale pixels remain before later ticks run. Keep later ticks only when they test a separate animation behavior.
Based on learnings, “Every behavior must be covered by meaningful unit and scenario tests whose descriptions state user-understandable functional behavior.”
🤖 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_Effects_gridsweep.cpp` around lines 201 - 216, Update
the test around the initial layer.tick() sequence to inspect the buffer
immediately after the first tick and require that no stale pixels remain. Retain
the later tick loop only if it verifies a distinct animation behavior, and make
each assertion or test description clearly state the user-visible behavior being
validated.
Sources: Path instructions, Learnings
Comment on lines
+661
to
+667
| for (int f = 0; f < fps; f++) | ||
| total += t.advance(static_cast<uint32_t>(static_cast<uint64_t>(f) * 1000 / fps)); | ||
| // 60 reference frames of 256 units. The band absorbs the sub-millisecond carry at the end | ||
| // of the second; it does not absorb a wrong reference period, which is a flat 4%. | ||
| const double refFrames = static_cast<double>(total) / particles::FrameTime::kOne; | ||
| CHECK(refFrames > 58.0); | ||
| CHECK(refFrames < 61.0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the 1000 ms timestamp.
The loop stops at fps - 1. At 30 FPS, its last sample is 966 ms. The test therefore does not measure one second, and the covered duration changes with fps.
Include f == fps and compare the total against 60 reference frames with a fixed-point-sized tolerance.
Based on learnings, “Every behavior must be covered by meaningful unit and scenario tests whose descriptions state user-understandable functional behavior.”
🤖 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_Particles.cpp` around lines 661 - 667, Update the
frame-advance loop in the particle timing test to include the sample at f ==
fps, ensuring it always covers the full 1000 ms interval. Replace the broad
refFrames bounds with a fixed-point-sized tolerance around the expected 60
reference frames, and make the test description state the user-visible timing
behavior.
Sources: Path instructions, Learnings
Fixes a test that passed locally and failed on CI: one effect changed the global palette while the sweep was running, so every effect ticked after it painted with a colour it never chose. desktop 140us/7,142fps | esp32 4,164us/240fps Tests - The channel-count sweep resets the active palette per effect. DemoReelEffect reassigns that global when its randomPalette control fires, and the sweep ticks it, so every later effect inherited DemoReel's pick. The probe can only read the RED channel at cpl=1, and 66 of 256 palette entries have red == 0 — so WaveEffect drew a real wave that the probe could not see, and reported "drew nothing". Isolated it passed every time; in suite order it failed every time. Now 0 failures over 5 full-suite runs. - The dirty-buffer audit samples the FIRST frame as well as the settled one: the first frame is what a user sees at the moment they switch effects, and a check only after a long run lets an effect slowly paint over the old picture and still pass. - The frame-time test covers the full 0..1000 ms interval (it stopped one frame short, measuring slightly under a second and hiding a small rate error in the shortfall) and its band is one reference frame rather than two. Docs - Backlog: clearing the grid on an effect's FIRST frame. The stricter audit above found fourteen effects that still show the previous effect's picture right after a switch — audio-reactive ones with no input yet, and simulations that seed on tick one. Most predate this branch, and each needs its own judgement about whether to clear, fade or seed differently, so the audit asserts the settled frame and captures the first-frame count rather than blocking on it. - Scenario observation bounds re-recorded from a live desktop run. Reviews - CodeRabbit, 2 findings, both fixed (the first-frame assertion and the frame-time interval). Its first-frame finding is what surfaced the fourteen effects above. Verified before merge: 19 headless scenarios pass; 20 of 22 live scenarios pass against the running desktop build. The two live failures are pre-existing runner limitations, not code: reset-ethType 404s because the eth controls exist only on ESP32 builds (the scenario's own description says the desktop runner skips it), and MULT is declared in a fixture block the live runner does not apply. This branch changes nothing under moondeck/scenario/, and both scenarios are identical on main. Co-Authored-By: Claude Opus 5 (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.
Effects used to hand-roll their own drawing, math and motion: each one carried its own line
walker, its own sine, its own fade, its own idea of how fast a frame is. This branch replaces
that with one shared toolbox, and migrates the existing effects onto it.
Two things fall out. Effects get shorter and more capable — anti-aliasing, sub-pixel
positioning and signed distance fields arrive everywhere at once rather than per effect. And
motion becomes a property of time rather than of hardware: the same settings look the same on
a 30 fps wall and a 5,000 fps desktop, with the faster device drawing it more smoothly.
What landed
Presets and a control surface.
ControlModulesaves and restores any part of the moduletree as a named JSON file. A
Layers-only preset is hardware-portable; addingDriversmakesit a device snapshot carrying pin maps. Presets publish to Home Assistant as a select.
The power-function library (
docs/moonmodules/light/power-functions.mdlists everyfunction with its callers):
Canvas, lines, bars, rects, circles, scroll, splat, anti-aliased linessin16/cos16,atan16,dist16,map32, easings,BeatPhase,hashIntfbm,turbulence,warp, blobssmin, and free anti-aliasing fromcoverage()collisions, emitters
Twelve new effects (40 → 52), each a showcase for one part of the toolbox: SdfShapes,
PolarNoise, WaterRipple, Tunnel, Echo, Dissolve, Spectrum, Fireworks, Ballpit, Truchet,
Raymarch, VectorBalls.
Framerate independence as a system rule (
architecture.md). Everything that changes overtime is driven by elapsed time, never by frame count. Quantising to a fixed 60 Hz and skipping
frames is explicitly the wrong fix, since it discards the smoothness the extra frames buy.
unit_Effects_framerate.cppaudits all 52 effects at 60 vs 1200 fps.Bugs this found and fixed
Several were live before the branch, and none were visible from reading the code:
sin16/cos16returned unsigned where FastLED master and WLED main both return signed. Aported effect that writes
sin16(x) + 32768is offset by half scale with no error anywhere.isqrt64returned 0 forUINT64_MAX— its first Newton step overflowed. Oncecollide()used it, that would have read as zero separation.
lerp16overflowed int32 on roughly a quarter of samples (undefined behaviour), andfbm16shifted each octave down by 8 before summing, making its output 8-bit in a 16-bit type: 195
distinct values over 20,000 samples, now 15,118.
speedon a fast device, where the per-frame steptruncated to zero.
Performance
Desktop tick measured three runs each, side by side against a
mainworktree: the rangesoverlap and there is no regression. The repo-health "+13 µs ⚠" marker compares two single
samples of a noisy metric. Flash grows by the new effects and the library.
Raymarching measures 96 cycles/pixel on an S3 — below the 292 budgeted for a full 128×128
wall — which is why it is gated on
SOC_CPU_HAS_FPUrather than restricted to desktop.Known and deferred
currently cancel: frame-counted launches, and a
fadeToBlackByfloor that erases the trailfaster on a fast device. Fixing one exposes the other, and fixing both changes how the
effect looks, so it gets its own commit.
which is a rework rather than a migration.
fbm16,warp16/turbulence16) are backlogged, each to land with the first effect thatneeds it.
Review
CodeRabbit and two Reviewer passes. Findings fixed, except: the two grid guards in effects
(
Layer::tickalready gates onhasGrid, so a guard there is dead code and an orchestrationviolation),
map32's split numerator (needs both spans near 2^32; no call site does), andmoving
FrameTimeto core / abstracting raymarch behind a platform interface — both real,both refactors across every caller rather than review fixes.
Summary by CodeRabbit
New Features
Bug Fixes