Add ControlModule: presets on a control surface - #62
Open
ewowi wants to merge 6 commits into
Open
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:
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
Comment on lines
+156
to
+163
| // | ||
| // A degenerate grid (any extent 0, so no lights) is gated HERE, once, for every effect: | ||
| // there is nothing to render into, and geometry derived from a zero extent is meaningless | ||
| // (a band split or horizon computed from 0 columns goes negative). Orchestration is the | ||
| // Layer's job — an effect must never carry its own "is my grid empty" check, or the rule | ||
| // ends up re-implemented 39 times and drifts. See architecture.md § Effects. | ||
| if (width_ <= 0 || height_ <= 0 || buffer_.count() == 0) return; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep modifier updates reachable when the grid is empty.
Line 162 returns before ModifierBase::tick() runs. This freezes beat-driven modifier state while a layout has zero width, zero height, or no buffer entries. The later comments explicitly require modifiers to keep advancing during this interval.
Remove the early return. Compute hasGrid with the buffer-count check, gate only the effect loop and applyLivePass(), and keep the modifier loop reachable. Add a deterministic regression test for a modifier across a zero-size frame.
As per coding guidelines, the system must accept any size and apply every setting live.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/layers/Layer.h` around lines 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.
Source: Coding guidelines
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
+263
to
+265
| constexpr uint8_t smoothFollow(uint8_t current, uint8_t target, uint8_t rate) { | ||
| const int32_t delta = static_cast<int32_t>(target) - current; | ||
| return static_cast<uint8_t>(current + ((delta * rate) >> 8)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make every nonzero follow rate progress.
At rate == 255, smoothFollow(0, 255, 255) returns 254, then stalls. Small nonzero rates also stall when the remaining delta truncates to zero. Use a signed, rounded-toward-target step so every nonzero rate converges and the maximum rate reaches the target immediately. Add endpoint tests for rates 1 and 255.
As per path instructions, “Every behavior must handle any input, order, and size without crashing; degrade visibly instead.”
🤖 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 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.
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
+332
to
+341
| // ---- Canvas overloads -------------------------------------------------------------------------- | ||
| // The Canvas forms of the primitives above. Each forwards to the (Buffer&, dims) implementation | ||
| // rather than duplicating its logic — one algorithm, two call shapes — so the pair cannot drift | ||
| // while the migration is in progress. When the last caller of the older form is gone, these become | ||
| // the implementations and the pair collapses to one (§ the subtraction pass in the top-down spec). | ||
| // | ||
| // `Canvas` carries a raw pointer rather than a Buffer&, so these rebuild the light Buffer view the | ||
| // legacy signatures expect. That view is a non-owning wrapper: constructing it copies three fields | ||
| // and allocates nothing. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Correct the Canvas-overload header comment.
Lines 333-340 state that each Canvas form "forwards to the (Buffer&, dims) implementation rather than duplicating its logic" and that the Canvas forms "rebuild the light Buffer view the legacy signatures expect". The code does neither. fade (343), fill (349), blur (373), glyph (740) and text (754) each carry a full copy of the Buffer algorithm and never construct a Buffer.
Only line uses a shared implementation, through detail::walkLine. Describe the actual arrangement so a maintainer knows which pairs can drift.
♻️ Proposed comment update
// ---- Canvas overloads --------------------------------------------------------------------------
-// The Canvas forms of the primitives above. Each forwards to the (Buffer&, dims) implementation
-// rather than duplicating its logic — one algorithm, two call shapes — so the pair cannot drift
-// while the migration is in progress. When the last caller of the older form is gone, these become
-// the implementations and the pair collapses to one (§ the subtraction pass in the top-down spec).
-//
-// `Canvas` carries a raw pointer rather than a Buffer&, so these rebuild the light Buffer view the
-// legacy signatures expect. That view is a non-owning wrapper: constructing it copies three fields
-// and allocates nothing.
+// The Canvas forms of the primitives above. `Canvas` carries a raw pointer rather than a Buffer&,
+// so these cannot call the legacy signatures directly.
+//
+// `line` shares one error-carry loop with the Buffer form through `detail::walkLine`. The other
+// pairs (fade, fill, blur, glyph, text) currently hold a COPY of the Buffer algorithm and must be
+// changed together until the last caller of the older form is gone, at which point these become
+// the implementations and each pair collapses to one (§ the subtraction pass in the top-down spec).As per coding guidelines: "each fact has one authoritative home rather than duplicated copies."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ---- Canvas overloads -------------------------------------------------------------------------- | |
| // The Canvas forms of the primitives above. Each forwards to the (Buffer&, dims) implementation | |
| // rather than duplicating its logic — one algorithm, two call shapes — so the pair cannot drift | |
| // while the migration is in progress. When the last caller of the older form is gone, these become | |
| // the implementations and the pair collapses to one (§ the subtraction pass in the top-down spec). | |
| // | |
| // `Canvas` carries a raw pointer rather than a Buffer&, so these rebuild the light Buffer view the | |
| // legacy signatures expect. That view is a non-owning wrapper: constructing it copies three fields | |
| // and allocates nothing. | |
| // ---- Canvas overloads -------------------------------------------------------------------------- | |
| // The Canvas forms of the primitives above. `Canvas` carries a raw pointer rather than a Buffer&, | |
| // so these cannot call the legacy signatures directly. | |
| // | |
| // `line` shares one error-carry loop with the Buffer form through `detail::walkLine`. The other | |
| // pairs (fade, fill, blur, glyph, text) currently hold a COPY of the Buffer algorithm and must be | |
| // changed together until the last caller of the older form is gone, at which point these become | |
| // the implementations and each pair collapses to one (§ the subtraction pass in the top-down spec). |
🤖 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/draw.h` around lines 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.
Source: Coding guidelines
Comment on lines
+37
to
+44
| TEST_CASE("fbm stays inside the 0..255 field range") { | ||
| for (uint32_t x = 0; x < 3000; x += 137) | ||
| for (uint8_t oct = 1; oct <= 5; oct++) { | ||
| const uint8_t v = fbm8(x, x / 2, oct); | ||
| CHECK(v <= 255); // the type guarantees the ceiling; this pins the normalisation | ||
| CAPTURE(x); CAPTURE(oct); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the normalization property instead of the return type.
Line 41 always passes because v is uint8_t. A broken implementation that overflows before narrowing also passes this test.
Compare the fBm result with the minimum and maximum octave samples. A normalized weighted average must remain within that range.
Proposed fix
const uint8_t v = fbm8(x, x / 2, oct);
- CHECK(v <= 255); // the type guarantees the ceiling; this pins the normalisation
+ uint8_t low = 255;
+ uint8_t high = 0;
+ uint32_t sampleX = x;
+ uint32_t sampleY = x / 2;
+ for (uint8_t octave = 0; octave < oct; ++octave) {
+ const uint8_t sample = inoise8(sampleX, sampleY);
+ low = std::min(low, sample);
+ high = std::max(high, sample);
+ sampleX <<= 1;
+ sampleY <<= 1;
+ }
+ CHECK(v >= low);
+ CHECK(v <= high);
CAPTURE(x); CAPTURE(oct);As per coding guidelines, “Pin every new behavior with meaningful unit and scenario tests.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TEST_CASE("fbm stays inside the 0..255 field range") { | |
| for (uint32_t x = 0; x < 3000; x += 137) | |
| for (uint8_t oct = 1; oct <= 5; oct++) { | |
| const uint8_t v = fbm8(x, x / 2, oct); | |
| CHECK(v <= 255); // the type guarantees the ceiling; this pins the normalisation | |
| CAPTURE(x); CAPTURE(oct); | |
| } | |
| } | |
| TEST_CASE("fbm stays inside the 0..255 field range") { | |
| for (uint32_t x = 0; x < 3000; x += 137) | |
| for (uint8_t oct = 1; oct <= 5; oct++) { | |
| const uint8_t v = fbm8(x, x / 2, oct); | |
| uint8_t low = 255; | |
| uint8_t high = 0; | |
| uint32_t sampleX = x; | |
| uint32_t sampleY = x / 2; | |
| for (uint8_t octave = 0; octave < oct; ++octave) { | |
| const uint8_t sample = inoise8(sampleX, sampleY); | |
| low = std::min(low, sample); | |
| high = std::max(high, sample); | |
| sampleX <<= 1; | |
| sampleY <<= 1; | |
| } | |
| CHECK(v >= low); | |
| CHECK(v <= high); | |
| CAPTURE(x); CAPTURE(oct); | |
| } | |
| } |
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 43-43: The function 'cos8' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/core/unit_fields.cpp` around lines 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.
Source: Coding guidelines
Comment on lines
+74
to
+102
| SUBCASE("SdfShapesEffect") { SdfShapesEffect e; golden::checkGolden("SdfShapesEffect", golden::renderHash(e, 16, 16, 1), 0xbcfb74b4836606a3ull); } | ||
| SUBCASE("PolarNoiseEffect") { PolarNoiseEffect e; golden::checkGolden("PolarNoiseEffect", golden::renderHash(e, 16, 16, 1), 0x5e888644938f8851ull); } | ||
| SUBCASE("WaterRippleEffect") { WaterRippleEffect e; golden::checkGolden("WaterRippleEffect", golden::renderHash(e, 16, 16, 1), 0xa11f9c4f27cba8d5ull); } | ||
| SUBCASE("TunnelEffect") { TunnelEffect e; golden::checkGolden("TunnelEffect", golden::renderHash(e, 16, 16, 1), 0xa2f6752d82436fc1ull); } | ||
| SUBCASE("EchoEffect") { EchoEffect e; golden::checkGolden("EchoEffect", golden::renderHash(e, 16, 16, 1), 0x27141166d74860c0ull); } | ||
| SUBCASE("DissolveEffect") { DissolveEffect e; golden::checkGolden("DissolveEffect", golden::renderHash(e, 16, 16, 1), 0xeb7810ca874152bcull); } | ||
| SUBCASE("SineEffect") { SineEffect e; golden::checkGolden("SineEffect", golden::renderHash(e, 16, 16, 1), 0xe96c6fd2da1b264bull); } | ||
| SUBCASE("PlasmaEffect") { PlasmaEffect e; golden::checkGolden("PlasmaEffect", golden::renderHash(e, 16, 16, 1), 0xfe821e9102099b93ull); } | ||
| SUBCASE("NoiseEffect") { NoiseEffect e; golden::checkGolden("NoiseEffect", golden::renderHash(e, 16, 16, 1), 0xdeb42f569f324cebull); } | ||
| SUBCASE("DistortionWavesEffect") { DistortionWavesEffect e; golden::checkGolden("DistortionWavesEffect", golden::renderHash(e, 16, 16, 1), 0xe4cd8111e8159133ull); } | ||
| SUBCASE("LavaLampEffect") { LavaLampEffect e; golden::checkGolden("LavaLampEffect", golden::renderHash(e, 16, 16, 1), 0x3c312e8a75b9ac83ull); } | ||
| SUBCASE("MetaballsEffect") { MetaballsEffect e; golden::checkGolden("MetaballsEffect", golden::renderHash(e, 16, 16, 1), 0x96a26bf931ad8341ull); } | ||
| SUBCASE("SpiralEffect") { SpiralEffect e; golden::checkGolden("SpiralEffect", golden::renderHash(e, 16, 16, 1), 0xfb0fb3b138dde70full); } | ||
| SUBCASE("RingsEffect") { RingsEffect e; golden::checkGolden("RingsEffect", golden::renderHash(e, 16, 16, 1), 0xf3b1ea7162afcf6bull); } | ||
| SUBCASE("WaveEffect") { WaveEffect e; golden::checkGolden("WaveEffect", golden::renderHash(e, 16, 16, 1), 0xa1150376dd23bea1ull); } | ||
| SUBCASE("StarSkyEffect") { StarSkyEffect e; golden::checkGolden("StarSkyEffect", golden::renderHash(e, 16, 16, 1), 0xa7ff8aab806be9ffull); } | ||
| SUBCASE("RainbowEffect") { RainbowEffect e; golden::checkGolden("RainbowEffect", golden::renderHash(e, 16, 16, 1), 0x75a2b1be1db07979ull); } | ||
| SUBCASE("BouncingBallsEffect") { BouncingBallsEffect e; golden::checkGolden("BouncingBallsEffect", golden::renderHash(e, 16, 16, 1), 0xbfc9de4aabc3c3b2ull); } | ||
| SUBCASE("FixedRectangleEffect") { FixedRectangleEffect e; golden::checkGolden("FixedRectangleEffect", golden::renderHash(e, 16, 16, 1), 0x22b828f908e9ce1cull); } | ||
| SUBCASE("LissajousEffect") { LissajousEffect e; golden::checkGolden("LissajousEffect", golden::renderHash(e, 16, 16, 1), 0x6f680693a1a90d78ull); } | ||
| SUBCASE("Noise2DEffect") { Noise2DEffect e; golden::checkGolden("Noise2DEffect", golden::renderHash(e, 16, 16, 1), 0xefbc5485de148631ull); } | ||
| SUBCASE("PraxisEffect") { PraxisEffect e; golden::checkGolden("PraxisEffect", golden::renderHash(e, 16, 16, 1), 0x0420f0404b3f12c5ull); } | ||
| SUBCASE("SolidEffect") { SolidEffect e; golden::checkGolden("SolidEffect", golden::renderHash(e, 16, 16, 1), 0x56711c1cf0c8ae83ull); } | ||
| SUBCASE("SphereMoveEffect") { SphereMoveEffect e; golden::checkGolden("SphereMoveEffect", golden::renderHash(e, 16, 16, 1), 0xb3f3d7c75fe49fdbull); } | ||
| SUBCASE("TetrixEffect") { TetrixEffect e; golden::checkGolden("TetrixEffect", golden::renderHash(e, 16, 16, 1), 0x048d66b3ecf2b377ull); } | ||
| SUBCASE("TextEffect") { TextEffect e; golden::checkGolden("TextEffect", golden::renderHash(e, 16, 16, 1), 0xc7c4faf87d12c099ull); } | ||
| SUBCASE("GameOfLifeEffect") { GameOfLifeEffect e; golden::checkGolden("GameOfLifeEffect", golden::renderHash(e, 16, 16, 1), 0xb2fb46cdf32ddd8bull); } | ||
| SUBCASE("RubiksCubeEffect") { RubiksCubeEffect e; golden::checkGolden("RubiksCubeEffect", golden::renderHash(e, 16, 16, 1), 0xecd4da66adc09f5dull); } | ||
| SUBCASE("StarFieldEffect") { StarFieldEffect e; golden::checkGolden("StarFieldEffect", golden::renderHash(e, 16, 16, 1), 0xeaea6687bd3e4676ull); } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use functional descriptions for the new subcases.
The new SUBCASE labels only identify implementation types. Change them to state the expected behavior, such as “SpiralEffect keeps its 16×16 frame stable at fixed cadence.”
This makes a failed subcase understandable without reading the test body.
As per coding guidelines, “test descriptions must 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_golden.cpp` around lines 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.
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
Comment on lines
+58
to
+61
| const uint8_t near = s.at(2, 4), far = s.at(3, 4); | ||
| CHECK(near > far); | ||
| CHECK(far > 0); | ||
| CHECK(near > 2 * far); // 3:1 by coverage, allowing for rounding |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rename the near and far locals.
near and far are macros in the Windows SDK headers (windef.h). The scenario contracts record a desktop-windows target, so this file can be compiled there. If any translation unit in the include chain pulls in <windows.h>, these declarations stop compiling. Use names that cannot collide.
♻️ Proposed rename
- const uint8_t near = s.at(2, 4), far = s.at(3, 4);
- CHECK(near > far);
- CHECK(far > 0);
- CHECK(near > 2 * far); // 3:1 by coverage, allowing for rounding
+ const uint8_t nearPx = s.at(2, 4), farPx = s.at(3, 4);
+ CHECK(nearPx > farPx);
+ CHECK(farPx > 0);
+ CHECK(nearPx > 2 * farPx); // 3:1 by coverage, allowing for rounding📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const uint8_t near = s.at(2, 4), far = s.at(3, 4); | |
| CHECK(near > far); | |
| CHECK(far > 0); | |
| CHECK(near > 2 * far); // 3:1 by coverage, allowing for rounding | |
| const uint8_t nearPx = s.at(2, 4), farPx = s.at(3, 4); | |
| CHECK(nearPx > farPx); | |
| CHECK(farPx > 0); | |
| CHECK(nearPx > 2 * farPx); // 3:1 by coverage, allowing for rounding |
🤖 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_Splat.cpp` around lines 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.
Comment on lines
+148
to
+161
| TEST_CASE("a per-channel write never spills into the next light") { | ||
| for (uint8_t cpl : {uint8_t{1}, uint8_t{2}}) { | ||
| CAPTURE(cpl); | ||
| Surface s(4, 1, 1, cpl); | ||
| // Mark every byte, then write one light through the primitive under test. | ||
| for (size_t i = 0; i < s.buf.bytes(); i++) s.buf.data()[i] = 0x5A; | ||
| draw::pixel(s.cv, {1, 0, 0}, RGB{10, 20, 30}); | ||
|
|
||
| // Light 1 took the colour it could hold... | ||
| CHECK(s.buf.data()[1 * cpl] == 10); | ||
| // ...and light 2 is untouched. | ||
| CHECK(s.buf.data()[2 * cpl] == 0x5A); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Check every channel the light holds.
For cpl == 2 light 1 owns bytes 2 and 3. The test asserts only on byte 1 * cpl, so a primitive that wrote the correct red but a wrong green would still pass. Assert the second channel when cpl >= 2.
💚 Proposed assertion
// Light 1 took the colour it could hold...
CHECK(s.buf.data()[1 * cpl] == 10);
+ if (cpl >= 2) CHECK(s.buf.data()[1 * cpl + 1] == 20);
// ...and light 2 is untouched.
CHECK(s.buf.data()[2 * cpl] == 0x5A);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TEST_CASE("a per-channel write never spills into the next light") { | |
| for (uint8_t cpl : {uint8_t{1}, uint8_t{2}}) { | |
| CAPTURE(cpl); | |
| Surface s(4, 1, 1, cpl); | |
| // Mark every byte, then write one light through the primitive under test. | |
| for (size_t i = 0; i < s.buf.bytes(); i++) s.buf.data()[i] = 0x5A; | |
| draw::pixel(s.cv, {1, 0, 0}, RGB{10, 20, 30}); | |
| // Light 1 took the colour it could hold... | |
| CHECK(s.buf.data()[1 * cpl] == 10); | |
| // ...and light 2 is untouched. | |
| CHECK(s.buf.data()[2 * cpl] == 0x5A); | |
| } | |
| } | |
| TEST_CASE("a per-channel write never spills into the next light") { | |
| for (uint8_t cpl : {uint8_t{1}, uint8_t{2}}) { | |
| CAPTURE(cpl); | |
| Surface s(4, 1, 1, cpl); | |
| // Mark every byte, then write one light through the primitive under test. | |
| for (size_t i = 0; i < s.buf.bytes(); i++) s.buf.data()[i] = 0x5A; | |
| draw::pixel(s.cv, {1, 0, 0}, RGB{10, 20, 30}); | |
| // Light 1 took the colour it could hold... | |
| CHECK(s.buf.data()[1 * cpl] == 10); | |
| if (cpl >= 2) CHECK(s.buf.data()[1 * cpl + 1] == 20); | |
| // ...and light 2 is untouched. | |
| CHECK(s.buf.data()[2 * cpl] == 0x5A); | |
| } | |
| } |
🤖 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_Splat.cpp` around lines 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.
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.
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 completely different hardware. The presets sit on an 8x8 pad grid with rotary encoders above and faders below, laid out like a Mackie control desk (X-Touch, QCon Pro G2) so a MIDI surface maps onto it later without a translation layer.
Why a core module
ControlModuleis a top-level module, a peer of Layouts/Layers/Drivers rather than a child of Services, because it reaches across the top-level modules and cannot sit inside one. Presets are its first capability; external control (MIDI, IR, a hardware panel) is what it exists to host next.MoonLight solved presets inside
ModuleLightsControl, effects-only. This generalises it: a preset can carry any part of the tree, and the file records which.What a preset carries
{ "slot": 12, "captures": "Layouts,Layers", "Layouts.enabled": true, "Layouts.0.type": "GridLayout", "Layouts.0.width": 128, "Layers.enabled": true, "Layers.0.type": "Layer", "Layers.0.0.type": "NoiseEffect" }Each captured subtree is exactly the bytes the persistence engine already writes, namespaced under a
<TypeName>.prefix. Save and restore reuse the engine that already reconciles a tree against JSON rather than a second serializer that could drift from it.Layersalone is a portable look. AddingDriversmakes it a device snapshot carrying pin maps. Because the file records the set, applying a preset is never a surprise.One active preset per role
Each capturable subtree holds a role: layout, layer, driver, service. Applying a preset claims every role it carries and leaves the others alone, so a layout preset and a layer preset are both lit at once, and a new layer preset replaces only the layer.
Pads are tinted by their roles, mixing hues when a preset carries several. This is why mixed presets need no special case: a mixed preset owns several roles rather than being a different kind of pad, and one rule both colours it and decides when it is superseded.
Core changes
FilesystemModule::saveSubtreeTo/applySubtree— two new seams.saveSubtreenow calls the former, so there is exactly one serializer.applySubtreeguards on the prefix being present: without itapplyNodereads "no children in JSON" as "delete every child", so a truncated preset file would wipe the live look. Pinned by two tests.ListSource::persistsList— a list whose rows are re-derived at setup is no longer written to flash. The preset list was being serialized on every save and discarded on load. Added at the core seam so Pins/Tasks can use it too./,\or.. The name becomes a file name, and ESP32'sfsTranslatedoes no path normalization (the desktop one does), so an unguarded name could escape the preset folder on device through save, delete or rename.UI
The pad grid, encoders and faders share one column track, so the three banks line up and still follow the pane as it is resized.
Two real bugs fixed along the way:
inputnorchange), and were built before the input had a value or bounds. OneredrawRangeDecorationscall now owns that seam, so any decorated control added later stays in sync.Testing
Gap, stated plainly: no scenario test for the preset round trip. The scenario runner has no op that can apply a preset — it speaks
/api/control, and applying a preset needs/api/list/. Extending the runner is separate work, so the round trip is covered by unit tests only.Review
Findings fixed: path traversal via preset name; the persisted-then-discarded list; a redundant whole-folder rewrite on save that could displace an unrelated preset (fixing it exposed a real bug where an unaimed save landed on pad 1); a duplicated fader binding; stale
ordernaming in three comments.One finding not applied: "applying a preset runs on the HTTP thread, not the render tick".
HttpServerModule::tick20msisMM_NONBLOCKINGand drains synchronously insideScheduler::tick, so the docstring as written is correct.Deferred to the dynamic-presets rework: the 192-byte header read, the insertion-sort struct copies, and
kMaxPresets36 -> 64 (accepted for now).Not in this PR
Playlists, apply-on-boot, and the external-control bindings the encoders and faders 2-8 are waiting for. Each gets its own plan.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes