MoonLight effect migration + buffer-persistence render model, driver-doc consolidation, module attribution#32
Merged
Merged
Conversation
Three additions plus an architecture fix. HueDriver drives Philips Hue lights as a projectMM output — the bulbs become colour pixels of an effect (paired over the Hue v1 HTTP API, listed in the Devices module), with the master brightness/colour-order correction and a smooth cadence-matched fade. WaveEffect ports MoonLight's travelling-waveform effect (six shapes) fresh onto our EffectBase. And a correctness fix surfaced while bench-testing the wave: 1D effects now iterate along Y (a vertical column expanded across X), matching MoonLight's verified convention and making a 1D output the natural first column of its 2D form.
PC worst-scenario tick:474us; ESP32 tick:903us(FPS:1107).
Core
- platform::httpRequest: new synchronous outbound LAN HTTP GET/PUT/POST seam (plain HTTP, no TLS — the Hue v1 API allows it), reading the response straight into the caller's buffer so a multi-KB body fits and a fire-and-forget PUT (null body) still executes. Desktop + ESP32 over raw sockets, off the render path.
- DevicesModule: lists a Hue bridge a driver has connected to via a new static upsertHueBridge(ip, name, colourCount) entry reached through DevicesModule::active() — the AudioModule::latestFrame() static-seam shape, so the light-domain driver registers the bridge without a structural core↔light dependency. The row carries the bridge's colour-light count (for sizing a layout) and round-trips through persistence. DevType gains Hue.
- Layer::extrude: 1D effects now run along Y — a D1 effect writes the (x=0) column down Y and extrude duplicates it across X (then Z), so 1D expands into 2D by adding columns (the 1D output is the first column of its 2D form). Was the reverse (X-row extruded across Y). Verified against MoonLight's effect source; 0 D1 effects existed, so no effect behaviour changed.
Light domain
- HueDriver: Hue lights as a colour effect output. Pairs (link-button poll), fetches the bridge's lights, and drives only the colour-capable + reachable ones — each window pixel → one bulb, RGB→HSV (textbook integer) → {on,bri,hue,sat}. Rate-paced by a millis() gate in loop() (one bounded PUT per interval — never blocks the single render loop, and keeps TCP connection churn bounded), with a transitiontime matched to each light's refresh so colours glide. Applies the shared Correction (brightness LUT + channel order) like the physical drivers. Exposes a colourLights count and lists the bridge in Devices.
- WaveEffect: a waveform (sawtooth / triangle / sine / square / sin3 / noise) that travels across the grid leaving a fading trail — MoonLight's Wave behaviour reproduced on EffectBase + our integer primitives (sin8, scale8), the colour routed through a single palette-ready seam (hsvToRgb today). D2; follows the 1D-runs-along-Y layout convention.
UI / wiring
- main.cpp: register HueDriver and WaveEffect in the factory.
Tests
- unit_HueDriver: RGB→HSV mapping, the {on,bri,hue,sat} body, the changed-only diff, and the colour-capable+reachable light filter against a real bridge /lights shape.
- unit_DevicesModule_hue: the Hue bridge row + its persistence round trip.
- unit_WaveEffect: each of the six waveform shapes maps phase→y correctly and stays in bounds at any grid size.
- unit_Layer_extrude: the D1 stub now writes the x=0 column and asserts it spreads across X (the flipped convention).
Docs / CI
- architecture.md § Dimensionality: corrected the D1 definition and added the unified expand rule (1D→2D adds columns across X, 2D→3D adds slices across Z) with the reasoning, and that a 1D output is laid out 1×N.
- HueDriver.md, WaveEffect.md: new specs (with the three Hue screenshots); DevicesModule.md: the out-of-band Hue-bridge listing; backlog-core.md: the pair-once-in-DevicesModule + mDNS-discovery follow-ups; EffectBase.h / Layer.h comments updated to the 1D-along-Y convention.
- Plan-20260630 - HueDriver: the saved design plan.
Reviews
- 🌊 WaveEffect ports MoonLight's Wave (Ewoud Wijma) — behaviour reproduced, written fresh against our architecture, credited as prior art (no code/structure traced).
- 1D-along-Y convention reverse-engineered + verified against MoonLight's own effect source (RingRandomFlow, AudioRings, FreqMatrix, NoiseMeter, PacMan, PopCorn all iterate Y).
Co-Authored-By: Claude Opus 4.8 (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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backlog/backlog-core.md`:
- Line 28: The backlog note uses the wrong contract name for Hue bridge light
counts; update the wording around HueDriver and DevicesModule so it refers to
the bridge’s colour-light count, not a dimmable-light count. Keep the existing
references to HueDriver and DevicesModule, but align the description with the
`colourCount_`/`colour` semantics that are actually persisted and used.
In `@docs/history/plans/Plan-20260630` - HueDriver (Hue lights as an effect
output).md:
- Line 1: The plan entry in docs/history/plans is still unresolved, so update
the document title to include a trailing outcome marker indicating it is closed,
such as “(shipped)”. Use the existing plan filename and heading for
“Plan-20260630 - HueDriver (Hue lights as an effect output)” and rename/mark it
consistently so the resolved status is obvious to readers.
In `@docs/moonmodules/light/effects/WaveEffect.md`:
- Around line 23-27: The WaveEffect spec uses future-facing and
absence-narration wording around color handling and prior art, which should be
rewritten in present tense. Update the descriptions in WaveEffect.md near the
`waveColor(index)` seam and the prior-art paragraph to state only the current
implementation and relationship, avoiding phrases like “today,” “when palettes
land,” and “palettes aren't in projectMM yet.”
In `@src/core/DevicesModule.h`:
- Around line 131-160: The core seam is leaking a light-domain concept into
DevicesModule by hard-coding Hue-specific bridge registration and colour-count
metadata. Rename or generalize upsertHueBridge and Device::colourCount into a
domain-neutral external-device registration/capability shape inside
DevicesModule, then update the persistence/sorting path to use that neutral
metadata so the core/light split stays recoverable for future foreign devices.
- Around line 106-111: Clamp the persisted colour value before narrowing it in
the device-loading path, because `mm::json::readInt(...)` can return negative or
oversized values that wrap when assigned to `d.colourCount`. Update the
`DevicesModule` deserialization logic around the `d.colourCount` assignment to
bound the parsed value to the valid range used by `HueDriver::parseLights()`
(0–127) before casting to `uint8_t`, so corrupt or edited device entries cannot
restore invalid counts.
In `@src/light/drivers/HueDriver.h`:
- Around line 67-79: The HueDriver::loop() path still performs blocking bridge
PUTs via pushOneChangedLight(), which can stall the render loop even with the
millis() gate. Refactor HueDriver so loop() only schedules or marks changed
lights, and move the actual platform::httpRequest()-based PUT work into a
non-render worker/tick or a non-blocking transport seam; keep the throttled,
changed-only behavior and ensure pushOneChangedLight() is no longer reachable
from the render path.
- Around line 57-65: The HueDriver configuration update path currently leaves
cached bridge/light state alive when bridgeIp or appKey changes, so the driver
keeps using stale discovery and push data. Update HueDriver::onUpdate to detect
changes to those controls and clear the learned light/cache state used by loop1s
and loop, including lightCount_, hueId_, sent_, and colourCount_, so fetchLights
runs again and the new bridge configuration takes effect immediately. Keep the
reset scoped to bridge configuration changes and preserve the existing pairing
behavior for the pair control.
In `@src/light/effects/WaveEffect.h`:
- Around line 14-16: Rewrite the WaveEffect header comments in present tense so
they describe the current implementation only, using the existing WaveEffect and
hsvToRgb seam as the focus. Remove roadmap/absence phrasing like “no palette
dependency yet,” “today,” and “when palettes land,” and move any historical or
future-facing notes into backlog/history docs instead of the header comments.
- Around line 78-82: The phase accumulation in WaveEffect is using
millisecond-based elapsed() values, which causes sub-millisecond frames to be
truncated and the first frame to jump from uptime. Update the timing logic in
WaveEffect::update (or the method that advances phase_) to use
platform::micros() instead of elapsed(), and initialize/reset lastElapsed_ on
first use so the accumulator starts from the current timestamp rather than zero.
Keep the phase_ math in the same accumulator path and ensure the timestamp
bookkeeping is updated consistently wherever the wave phase is advanced.
In `@src/light/layers/Layer.h`:
- Around line 231-245: The current handling in Layer and effectDim incorrectly
hardens Dim::D1 inside the light pipeline, which conflicts with the src/light/**
rule that all effects must be at least 2D. Remove the D1 expansion path from
this logic and keep single-strip behavior modeled as a 1×N×1 Dim::D2 surface
instead, so the framework continues to treat D1 as unsupported. Update the
Layer.h dimensionality expansion flow so Dim::D2 remains the supported path for
strip/Hue semantics and avoid changing the core D1 rules.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 514-525: The request write path in platform_desktop.cpp currently
treats a short return from send(sock(fd), req, n, 0) as failure, which can
truncate HTTP requests under backpressure. Update the send logic in the HTTP
request builder path around the req buffer to loop until all n bytes are
written, retrying on positive partial sends and only failing on zero or negative
returns. Keep the fix localized to the send handling that follows the
snprintf-built request.
- Around line 495-512: The connect path in platform_desktop.cpp only applies
SO_RCVTIMEO/SO_SNDTIMEO and still lets connect() block past timeoutMs during the
TCP handshake. Update the socket setup in this block around the connect/recv
timeout logic so connect is also bounded by timeoutMs, using the existing
fd/socket helpers and the same timeoutMs parameter to keep the Hue
pairing/update flow within the deadline.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1135-1146: The HTTP request send path in platform_esp32.cpp
currently calls ::send() once and fails on any short write, which can leave a
partial request on the wire. Update the request-writing logic in the helper that
builds req so it loops until all n bytes are transmitted, retrying on short
writes and handling EAGAIN/EWOULDBLOCK as retryable conditions before giving up.
- Around line 1123-1133: The ESP32 socket setup in the connect path is still
using SO_RCVTIMEO and SO_SNDTIMEO, which do not bound the TCP handshake for
::connect; update this logic in the connection routine to use a non-blocking
connect with an explicit timeout wait (or equivalent bounded connect flow) so
unreachable hosts cannot stall the caller. Keep the change localized around the
connect sequence after sockaddr_in is populated, and ensure the timeout is
enforced before returning success from ::connect.
In `@test/unit/core/unit_DevicesModule_hue.cpp`:
- Around line 40-50: The DevicesModule restore test only covers a valid
persisted Hue bridge, so add doctest cases around restoreList/writeListRow to
cover malformed persisted colour values as well. Extend the existing
DevicesModule test or add nearby cases to verify that the restore path clamps
invalid colour inputs such as negative and overflow values, while still
restoring the Hue row correctly through writeListRow and the listed JSON fields.
Use the existing restoreList, listRowCount, and writeListRow symbols to keep the
persistence contract covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50652141-825b-4643-83e6-bd2563ea4d7b
⛔ Files ignored due to path filters (3)
docs/assets/screenshots/Hue device disco.pngis excluded by!**/*.pngdocs/assets/screenshots/Hue driver.pngis excluded by!**/*.pngdocs/assets/screenshots/Hue friendly effect.pngis excluded by!**/*.png
📒 Files selected for processing (22)
docs/architecture.mddocs/backlog/backlog-core.mddocs/history/plans/Plan-20260630 - HueDriver (Hue lights as an effect output).mddocs/moonmodules/core/DevicesModule.mddocs/moonmodules/light/drivers/HueDriver.mddocs/moonmodules/light/effects/WaveEffect.mdsrc/core/DeviceIdentify.hsrc/core/DevicesModule.hsrc/light/drivers/HueDriver.hsrc/light/effects/EffectBase.hsrc/light/effects/WaveEffect.hsrc/light/layers/Layer.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/CMakeLists.txttest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_DevicesModule_hue.cpptest/unit/light/unit_HueDriver.cpptest/unit/light/unit_Layer_extrude.cpptest/unit/light/unit_WaveEffect.cpp
| @@ -0,0 +1,83 @@ | |||
| # Plan — HueDriver: Philips Hue lights as a projectMM effect output | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark this resolved plan with an outcome.
This PR is landing the feature, but the history-plan convention requires resolved entries in docs/history/plans/ to carry a trailing outcome such as (shipped). Please rename/update this plan now so readers can tell it is closed. As per path instructions, "Approved feature plans must be saved in docs/history/plans/ with the Plan-YYYYMMDD - <title>.md naming pattern, then marked with a trailing outcome such as (shipped) or (attempted, abandoned) when resolved."
Also applies to: 76-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plans/Plan-20260630` - HueDriver (Hue lights as an effect
output).md at line 1, The plan entry in docs/history/plans is still unresolved,
so update the document title to include a trailing outcome marker indicating it
is closed, such as “(shipped)”. Use the existing plan filename and heading for
“Plan-20260630 - HueDriver (Hue lights as an effect output)” and rename/mark it
consistently so the resolved status is obvious to readers.
Source: Path instructions
Comment on lines
+131
to
+160
| // The boot DevicesModule (exactly one exists). A foreign-bridge driver in the light domain | ||
| // (HueDriver) registers a discovered bridge through this without a compile-time dependency | ||
| // on DevicesModule's address — the same static-seam shape as AudioModule::latestFrame(). | ||
| static DevicesModule* active() { return active_; } | ||
|
|
||
| // Register a Hue bridge a HueDriver has connected to. Unlike upsertDevice (driven by a UDP | ||
| // presence packet), a bridge is discovered out-of-band — the driver already holds its IP + | ||
| // app key — so this is the explicit entry point for that. Idempotent: updates the name + | ||
| // colour count of the existing row, or inserts one. `colour` is how many of the bridge's | ||
| // lights are colour-capable, the figure for sizing a layout. Persisted like any device row. | ||
| void upsertHueBridge(const uint8_t ip[4], const char* name, uint8_t colour) { | ||
| Device* d = findByIp(ip); | ||
| bool persistChanged = false; | ||
| if (!d) { | ||
| if (deviceCount_ >= kMaxDevices) return; // bounded; silently cap | ||
| d = &devices_[deviceCount_++]; | ||
| std::memcpy(d->ip, ip, 4); | ||
| persistChanged = true; | ||
| } | ||
| if (d->type != DevType::Hue) { d->type = DevType::Hue; persistChanged = true; } | ||
| if (name && name[0] && std::strcmp(d->name, name) != 0) { | ||
| std::snprintf(d->name, sizeof(d->name), "%s", name); | ||
| persistChanged = true; | ||
| } | ||
| if (!d->name[0]) { formatDottedQuad(d->name, ip); persistChanged = true; } | ||
| if (d->colourCount != colour) { d->colourCount = colour; persistChanged = true; } | ||
| d->lastSeenMs = platform::millis(); // transient — keeps the bridge from ageing out | ||
| d->cached = false; | ||
| if (persistChanged) { sortByName(); refreshStatus(); } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep this core seam domain-neutral.
upsertHueBridge() and Device::colourCount hard-code a light-domain concept into src/core/DevicesModule.h. A neutral external-device registration shape plus generic capability metadata would keep the core/light split recoverable when the next foreign output arrives. As per coding guidelines, Use domain-neutral naming when core and light concerns must mix, so the code can be separated later if needed.
Also applies to: 232-238
🤖 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/DevicesModule.h` around lines 131 - 160, The core seam is leaking a
light-domain concept into DevicesModule by hard-coding Hue-specific bridge
registration and colour-count metadata. Rename or generalize upsertHueBridge and
Device::colourCount into a domain-neutral external-device
registration/capability shape inside DevicesModule, then update the
persistence/sorting path to use that neutral metadata so the core/light split
stays recoverable for future foreign devices.
Source: Coding guidelines
Three threads land together. CodeRabbit's review of the HueDriver/WaveEffect work is processed (real fixes + skips with reasons). The docs/assets tree is reorganised flat-by-type (effects/modifiers/layouts/drivers) so it matches src and stops being a 63-file flat folder. And the MoonLight effect/modifier/layout migration is staged as a multi-stage plan plus its decided foundations (palette design, folder structure, tags legend, effect inventory) — design only, no migration code yet.
PC worst-scenario tick:448us; ESP32 tick:907us(FPS:1102).
Core
- DevicesModule: clamp a persisted Hue colour-count to 0–127 before narrowing, so a corrupt/hand-edited device entry can't wrap into a bogus value on restore.
- platform httpRequest (desktop + esp32): send the request in a loop until all bytes are out (a blocking send can return short under backpressure); and bound the TCP connect by timeoutMs via a non-blocking connect + select, so an unreachable host can't stall the caller past the deadline (re-added the desktop make_blocking helper the connect path needs).
Light domain
- HueDriver: clear the learned light list + push cache when bridgeIp or appKey changes, so the driver re-fetches against the new bridge instead of using stale discovery data.
- light_types.h / WaveEffect: comments corrected to the 1D-runs-along-Y extrude convention and present tense.
Scripts / MoonDeck
- screenshot_modules.py / update_module_docs.py: write/read module assets under the new docs/assets/{core,light/{effects,modifiers,layouts,drivers},ui}/ structure (an asset_dir_for(type) map) instead of the flat screenshots/ folder.
Tests
- unit_DevicesModule_hue: a case pinning that a corrupt persisted colour value (negative / overflow) clamps to 0–127 and the Hue row still restores.
Docs / CI
- assets: moved all 63 screenshots/gifs into docs/assets/{core, light/{effects,modifiers,layouts,drivers}, ui}/ by module type; repointed every doc image reference (82 local refs verified resolving), leaving the v1.0.0 tag-pinned URLs on their historical path.
- architecture.md: added the canonical tag-emoji legend (MoonLight basis — role/dim/origin/creator/audio) and corrected the 1D-along-Y extrude wording.
- README: credited FastLED as the prior art behind the recognisable colour/animation primitive names + the gradient-palette model (names carried, implementation ours, no FastLED dependency).
- Migration plan (docs/history/plans): the multi-stage MoonLight migration map; linked bidirectionally with the rename plan's effect-parity gate.
- References: moonlight-effect-inventory.md (47 effects by origin) and moonlight-palettes-data.md (~54 gradient palettes + the decided palette design: a 16-entry CRGBPalette16-model lookup, global active palette on Drivers via Palettes::active(), MoonLive-authored dynamic palettes as a MoonLivePalette type, hard-swap now / crossfade backlogged).
- folder-structure-proposal.md: the domain/type folder decision (library is a tag + a per-library doc-page split, not a folder).
- backlog: LightsControl + light-presets items (the global-control hub MoonLight has; pragmatic interim on Drivers); the HueDriver plan marked (shipped).
Reviews
- 🐇 Persisted Hue colour-count can wrap when narrowed — fixed (clamp + test).
- 🐇 HueDriver keeps stale bridge/light state on a bridgeIp/appKey change — fixed (cache reset).
- 🐇 platform send() can truncate under backpressure — fixed (send loop, both platforms).
- 🐇 connect() not bounded by timeoutMs — fixed (non-blocking connect + select, both platforms).
- 🐇 Backlog/spec wording (dimmable→colour, WaveEffect present tense, HueDriver plan (shipped)) — fixed.
- 🐇 Remove the Dim::D1 extrude path ("effects must be 2D") — SKIPPED: D1 is a supported, documented dimension deliberately added this branch (1D-runs-along-Y, verified against MoonLight's own 1D effects); no such rule exists. Fixed the one genuinely stale D1 comment instead.
- 🐇 WaveEffect should use micros() for phase + init lastElapsed_ — SKIPPED: elapsed()/millis with lastElapsed_=0 is the project-wide pattern (10+ effects); changing one effect breaks that consistency and the first-frame effect is nil on a wrapping uint8 phase.
- 🐇 Generalise upsertHueBridge/colourCount to a domain-neutral external-device shape — SKIPPED (concrete-first): one consumer today; DevType + the active() static seam are already neutral; generalise when a second foreign device exists.
- 🐇 Move the HueDriver PUT off the render loop to a worker thread — SKIPPED: the millis-gated single bounded PUT is the deliberate design (built + bench-debugged with the product owner); a worker thread is deferred infrastructure, rationale in the code comment.
Co-Authored-By: Claude Opus 4.8 (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 (6)
src/light/effects/WaveEffect.h (2)
83-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove remaining forward-looking phrasing in comments.
Line 83 still says "a palette substitute until palettes land" and lines 121–122 say "today a hue sweep via hsvToRgb; when palettes land this becomes…". Both are absence-narration that violates the present-tense rule. Describe the current
hsvToRgbseam without referencing future palette work.As per coding guidelines, "Avoid forward-looking or absence-narration phrasing; describe what exists now rather than what was removed or what will happen later, except in backlog, history, and decision-contrast contexts."
Also applies to: 121-123
🤖 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/WaveEffect.h` at line 83, Remove the forward-looking wording in the comments around WaveEffect’s `waveColor`/`hsvToRgb` path. Update the comment near the slow colour cycle and the `hsvToRgb` seam to describe only the current behavior in present tense, avoiding references to “until palettes land” or what this will become later. Keep the explanation focused on how `hsvToRgb` is used today and where the seam exists in `WaveEffect`.Source: Coding guidelines
78-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeed
lastElapsed_on the first tick.lastElapsed_starts at 0, so the firstnow - lastElapsed_uses the whole uptime and jumps the wave to an arbitrary phase; use the same first-frame bootstrap pattern as the other time-gated effects, and remove the “sub-ms” claim from the comment.🤖 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/WaveEffect.h` around lines 78 - 84, The first update in WaveEffect::WaveEffect/WaveEffect::render is using lastElapsed_ before it has been initialized from the current elapsed() time, so the initial phase jumps by uptime instead of starting cleanly. Add the same first-tick bootstrap pattern used by the other time-based effects: initialize lastElapsed_ on the first call and only advance phase_ after that, using the existing elapsed() and phase_ logic. Also update the nearby comment to remove the “sub-ms dt” claim and describe the first-tick behavior accurately.Source: Coding guidelines
docs/history/plans/Plan-20260630 - HueDriver (Hue lights as an effect output) (shipped).md (1)
17-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the shipped plan to match the shipped scope.
This plan is marked shipped, but it still describes colour and DevicesModule bridge listing as follow-ups, and says
loop()does nothing. The supplied implementation now sends hue/sat/bri, reports the bridge toDevicesModule, and calls the PUT path fromloop(), so the history entry should record the actual outcome.Also applies to: 46-48, 78-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/history/plans/Plan-20260630` - HueDriver (Hue lights as an effect output) (shipped).md around lines 17 - 20, The shipped plan still describes an earlier scope, so update the `HueDriver` history entry to match the implemented behavior. Revise the bullets that mention only on/off and brightness, the DevicesModule bridge listing, and `loop()` doing nothing so they reflect the actual shipped outcome: `HueDriver` now sends hue/sat/bri, exposes the bridge through `DevicesModule`, and drives the PUT path from `loop()`. Keep the entry aligned with the final implementation and remove any “follow-up” wording that no longer applies.src/light/drivers/HueDriver.h (1)
205-211: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset the push cache on pairing success.
A successful re-pair changes
appKeybut leavessent_/lastRgb_from the previous bridge session. AfterfetchLights()repopulates IDs, unchanged colours can be skipped and never pushed to the newly paired bridge. ReuseresetLightCache()here instead of only clearinglightCount_.Proposed fix
if (key[0]) { + resetLightCache(); std::snprintf(appKey, sizeof(appKey), "%s", key); pairTicksLeft_ = 0; - lightCount_ = 0; // re-fetch the light list with the new key refreshStatus(); markDirty(); // persist the new app keyAlso applies to: 222-229
🤖 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/drivers/HueDriver.h` around lines 205 - 211, The pairing-success path in HueDriver leaves the push cache stale after updating the app key, so unchanged colors may be skipped for the new bridge session. In the success branch that handles a non-empty key, replace the ad hoc cache reset with resetLightCache() so sent_ and lastRgb_ are cleared along with the light list state, then continue with refreshStatus(), markDirty(), and FilesystemModule::noteDirty() as before.src/platform/desktop/platform_desktop.cpp (1)
534-545: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse one deadline across connect/send/recv.
platform.hsayshttpRequest()blocks up totimeoutMs, but this path can spend that budget inselect(), then again in eachsend()andrecv()call. Pass only the remaining time into each 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/platform/desktop/platform_desktop.cpp` around lines 534 - 545, In httpRequest() in platform_desktop.cpp, the timeout budget is being reused independently across connect/select, send, and recv, which can exceed the intended total deadline. Update the flow so a single deadline is tracked from entry and each step computes and uses the remaining time before calling select(), send(), and recv(). Make the change in the timeout setup around the socket operations, keeping the existing platform-specific setsockopt behavior but basing each call on the remaining milliseconds rather than the original timeoutMs.src/core/DevicesModule.h (1)
159-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh status when a cached Hue row becomes live.
If a restored bridge is reported again with the same persisted
nameandcolour,persistChangedstays false,d->cachedflips tofalse, and Line 162 skipsrefreshStatus(). The card can stay stuck at"(cached)"even though the row is live again.Proposed fix
- d->lastSeenMs = platform::millis(); // transient — keeps the bridge from ageing out - d->cached = false; - if (persistChanged) { sortByName(); refreshStatus(); } + d->lastSeenMs = platform::millis(); // transient — keeps the bridge from ageing out + const bool statusChanged = d->cached; + d->cached = false; + if (persistChanged) sortByName(); + if (persistChanged || statusChanged) refreshStatus();🤖 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/DevicesModule.h` around lines 159 - 162, The live-bridge update path in DevicesModule::addOrUpdateDevice (where d->cached is cleared and persistChanged controls sortByName()/refreshStatus()) skips refreshing status when a cached Hue row becomes active again without any persisted field changes. Update the logic so refreshStatus() runs whenever d->cached transitions from true to false, even if persistChanged remains false, while keeping sortByName() tied to actual persisted changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture.md`:
- Line 539: Update the “Tag emoji legend” heading in the architecture document
so it uses an h2 instead of an h4, since the heading hierarchy in this section
should step down from the existing top-level heading without skipping levels.
Locate the markdown heading text “Tag emoji legend” and change its heading level
to match the expected sequence from the surrounding section structure.
- Around line 543-551: The emoji legend is out of sync with the UI mapping in
src/ui/app.js, so update the affected legend entries to match the current role
and origin labels. Adjust the Role row to use the UI’s actual symbols for
layout, driver, peripheral, and include the additional layer and generic
categories, and update the Origin/library row to replace the old WLED symbol
with the one used in the app plus the FastLED entry. Keep the surrounding
explanation about role/dim coming from ROLE_EMOJI and tags() consistent with the
new legend.
In `@docs/backlog/folder-structure-proposal.md`:
- Line 24: The fenced tree-diagram blocks in the markdown are missing a language
tag and trigger markdownlint MD040. Update the three fenced blocks in the
folder-structure proposal document to use the text language specifier while
keeping the ASCII tree content unchanged; use the existing fenced block
locations as the guide and ensure all affected fences are consistently tagged.
In `@docs/moonmodules/light/drivers/HueDriver.md`:
- Line 3: The architecture link in HueDriver.md points to the wrong target, so
update the markdown link in the driver description to reference the project’s
architecture doc at the correct relative path. Fix the link text in the sentence
mentioning “[driver]” and keep the rest of the HueDriver explanation unchanged.
In `@scripts/docs/screenshot_modules.py`:
- Around line 78-83: `asset_dir_for()` is using inline one-line `if ...: return
...` branches that trigger Ruff E701; rewrite the `asset_dir_for` conditional
chain into standard multi-line `if` blocks with each `return` on its own line,
keeping the same matching logic for `Effect`, `Modifier`, `Layout`, `Driver`,
and the plural names branch.
In `@scripts/docs/update_module_docs.py`:
- Around line 35-40: The asset_dir_for helper uses multiple one-line if
statements with inline returns, which triggers Ruff E701 in
update_module_docs.py. Rewrite the conditional branches in asset_dir_for to use
standard multi-line if blocks with explicit return statements for each type_name
suffix check and the plural-name fallback, keeping the same logic while removing
the inline statement style.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1148-1153: The timeout handling in the ESP32 request path is split
between select() and SO_RCVTIMEO/SO_SNDTIMEO, which can exceed the intended
overall budget. Update the request flow in platform_esp32.cpp so the
send/receive operations in the relevant request method use a single absolute
deadline derived from timeoutMs, and have the code enforce that same deadline
across both phases instead of applying separate per-phase socket timeouts.
---
Outside diff comments:
In `@docs/history/plans/Plan-20260630` - HueDriver (Hue lights as an effect
output) (shipped).md:
- Around line 17-20: The shipped plan still describes an earlier scope, so
update the `HueDriver` history entry to match the implemented behavior. Revise
the bullets that mention only on/off and brightness, the DevicesModule bridge
listing, and `loop()` doing nothing so they reflect the actual shipped outcome:
`HueDriver` now sends hue/sat/bri, exposes the bridge through `DevicesModule`,
and drives the PUT path from `loop()`. Keep the entry aligned with the final
implementation and remove any “follow-up” wording that no longer applies.
In `@src/core/DevicesModule.h`:
- Around line 159-162: The live-bridge update path in
DevicesModule::addOrUpdateDevice (where d->cached is cleared and persistChanged
controls sortByName()/refreshStatus()) skips refreshing status when a cached Hue
row becomes active again without any persisted field changes. Update the logic
so refreshStatus() runs whenever d->cached transitions from true to false, even
if persistChanged remains false, while keeping sortByName() tied to actual
persisted changes.
In `@src/light/drivers/HueDriver.h`:
- Around line 205-211: The pairing-success path in HueDriver leaves the push
cache stale after updating the app key, so unchanged colors may be skipped for
the new bridge session. In the success branch that handles a non-empty key,
replace the ad hoc cache reset with resetLightCache() so sent_ and lastRgb_ are
cleared along with the light list state, then continue with refreshStatus(),
markDirty(), and FilesystemModule::noteDirty() as before.
In `@src/light/effects/WaveEffect.h`:
- Line 83: Remove the forward-looking wording in the comments around
WaveEffect’s `waveColor`/`hsvToRgb` path. Update the comment near the slow
colour cycle and the `hsvToRgb` seam to describe only the current behavior in
present tense, avoiding references to “until palettes land” or what this will
become later. Keep the explanation focused on how `hsvToRgb` is used today and
where the seam exists in `WaveEffect`.
- Around line 78-84: The first update in
WaveEffect::WaveEffect/WaveEffect::render is using lastElapsed_ before it has
been initialized from the current elapsed() time, so the initial phase jumps by
uptime instead of starting cleanly. Add the same first-tick bootstrap pattern
used by the other time-based effects: initialize lastElapsed_ on the first call
and only advance phase_ after that, using the existing elapsed() and phase_
logic. Also update the nearby comment to remove the “sub-ms dt” claim and
describe the first-tick behavior accurately.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 534-545: In httpRequest() in platform_desktop.cpp, the timeout
budget is being reused independently across connect/select, send, and recv,
which can exceed the intended total deadline. Update the flow so a single
deadline is tracked from entry and each step computes and uses the remaining
time before calling select(), send(), and recv(). Make the change in the timeout
setup around the socket operations, keeping the existing platform-specific
setsockopt behavior but basing each call on the remaining milliseconds rather
than the original timeoutMs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65623acc-1de5-46af-9bc8-7d5b36130d39
⛔ Files ignored due to path filters (63)
docs/assets/core/Devices module.pngis excluded by!**/*.pngdocs/assets/core/FilesystemModule.pngis excluded by!**/*.pngdocs/assets/core/FirmwareUpdateModule.pngis excluded by!**/*.pngdocs/assets/core/Hue device disco.pngis excluded by!**/*.pngdocs/assets/core/Layers.pngis excluded by!**/*.pngdocs/assets/core/Layouts.pngis excluded by!**/*.pngdocs/assets/core/NetworkModule.pngis excluded by!**/*.pngdocs/assets/core/SystemModule.pngis excluded by!**/*.pngdocs/assets/core/WLED Native discovers projectMM.jpegis excluded by!**/*.jpegdocs/assets/core/Wled discovers projectMM.pngis excluded by!**/*.pngdocs/assets/light/drivers/Drivers.pngis excluded by!**/*.pngdocs/assets/light/drivers/Hue driver.pngis excluded by!**/*.pngdocs/assets/light/drivers/Hue friendly effect.pngis excluded by!**/*.pngdocs/assets/light/drivers/NetworkSendDriver.pngis excluded by!**/*.pngdocs/assets/light/drivers/PreviewDriver.pngis excluded by!**/*.pngdocs/assets/light/effects/CheckerboardEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/CheckerboardEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/FireEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/FireEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/GameOfLifeEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/GameOfLifeEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/GlowParticlesEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/GlowParticlesEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/LavaLampEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/LavaLampEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/LinesEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/LinesEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/MetaballsEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/MetaballsEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/NoiseEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/NoiseEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/ParticlesEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/ParticlesEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/PlasmaEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/PlasmaEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/PlasmaPaletteEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/PlasmaPaletteEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/RainbowEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/RainbowEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/RingsEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/RingsEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/RipplesEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/RipplesEffect.pngis excluded by!**/*.pngdocs/assets/light/effects/SpiralEffect.gifis excluded by!**/*.gifdocs/assets/light/effects/SpiralEffect.pngis excluded by!**/*.pngdocs/assets/light/layouts/GridLayout.pngis excluded by!**/*.pngdocs/assets/light/modifiers/CheckerboardModifier.gifis excluded by!**/*.gifdocs/assets/light/modifiers/CheckerboardModifier.pngis excluded by!**/*.pngdocs/assets/light/modifiers/MirrorModifier.gifis excluded by!**/*.gifdocs/assets/light/modifiers/MirrorModifier.pngis excluded by!**/*.pngdocs/assets/light/modifiers/MultiplyModifier.gifis excluded by!**/*.gifdocs/assets/light/modifiers/MultiplyModifier.pngis excluded by!**/*.pngdocs/assets/ui/installer-board-picker-collapsed.pngis excluded by!**/*.pngdocs/assets/ui/installer-board-picker-expanded.pngis excluded by!**/*.pngdocs/assets/ui/installer.pngis excluded by!**/*.pngdocs/assets/ui/installer2.pngis excluded by!**/*.pngdocs/assets/ui/installer3.pngis excluded by!**/*.pngdocs/assets/ui/moondeck_esp32.pngis excluded by!**/*.pngdocs/assets/ui/moondeck_live.pngis excluded by!**/*.pngdocs/assets/ui/moondeck_pc.pngis excluded by!**/*.pngdocs/assets/ui/ui_light.pngis excluded by!**/*.pngdocs/assets/ui/ui_overview.pngis excluded by!**/*.pngdocs/assets/ui/ui_theme.gifis excluded by!**/*.gif
📒 Files selected for processing (53)
README.mddocs/architecture.mddocs/backlog/backlog-core.mddocs/backlog/backlog-mixed.mddocs/backlog/folder-structure-proposal.mddocs/backlog/moonlight-effect-inventory.mddocs/backlog/moonlight-palettes-data.mddocs/backlog/rename-to-moonlight.mddocs/building.mddocs/history/plans/Plan-20260630 - HueDriver (Hue lights as an effect output) (shipped).mddocs/history/plans/Plan-20260630 - MoonLight migration (multi-stage).mddocs/install/README.mddocs/moonmodules/core/DevicesModule.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/FirmwareUpdateModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/SystemModule.mddocs/moonmodules/light/Drivers.mddocs/moonmodules/light/Layers.mddocs/moonmodules/light/Layouts.mddocs/moonmodules/light/drivers/HueDriver.mddocs/moonmodules/light/drivers/NetworkSendDriver.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/effects/CheckerboardEffect.mddocs/moonmodules/light/effects/FireEffect.mddocs/moonmodules/light/effects/GameOfLifeEffect.mddocs/moonmodules/light/effects/GlowParticlesEffect.mddocs/moonmodules/light/effects/LavaLampEffect.mddocs/moonmodules/light/effects/LinesEffect.mddocs/moonmodules/light/effects/MetaballsEffect.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/effects/ParticlesEffect.mddocs/moonmodules/light/effects/PlasmaEffect.mddocs/moonmodules/light/effects/PlasmaPaletteEffect.mddocs/moonmodules/light/effects/RainbowEffect.mddocs/moonmodules/light/effects/RingsEffect.mddocs/moonmodules/light/effects/RipplesEffect.mddocs/moonmodules/light/effects/SpiralEffect.mddocs/moonmodules/light/effects/WaveEffect.mddocs/moonmodules/light/layouts/GridLayout.mddocs/moonmodules/light/modifiers/CheckerboardModifier.mddocs/moonmodules/light/modifiers/MultiplyModifier.mddocs/testing.mdscripts/MoonDeck.mdscripts/docs/screenshot_modules.pyscripts/docs/update_module_docs.pysrc/core/DevicesModule.hsrc/light/drivers/HueDriver.hsrc/light/effects/WaveEffect.hsrc/light/light_types.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cpptest/unit/core/unit_DevicesModule_hue.cpp
Adds a global colour palette (selectable on Drivers, with a gradient-swatch dropdown) that all index-coloured effects now read; curates the effect set (removes four, tunes others); and consolidates the per-module effect/modifier/layout docs into one compact-row page per type. Net −669 lines.
KPI: tick:119/92/121/15/2/280/59/17/20/161/118/17/1/35us(FPS:8403/10869/8264/66666/500000/3571/16949/58823/50000/6211/8474/58823/1000000/28571) | ESP32:1274KB | tick:1236us(FPS:809) | heap:151KB
Core:
- Control: added ControlType::Palette + addPalette() with a PaletteOptionsFn seam, so the light domain supplies each option's {name,colors} swatch and core stays palette-agnostic (mirrors the Select shape for value read/parse).
- DevicesModule: refreshStatus() now also runs when a cached device row transitions back to active with no persisted field change (a re-announced bridge no longer stays greyed-out); sortByName() stays tied to real persisted changes. (🐇)
- platform_desktop / platform_esp32: httpRequest now tracks one absolute deadline across connect/send/recv instead of giving each phase a fresh timeoutMs (total could reach ~3× the budget). (🐇)
Light domain:
- Palette: new 16-entry CRGBPalette16-style model with colorFromPalette()/fromGradient(), built-in palettes, and a global active palette read via Palettes::active() (the AudioModule::latestFrame() static-seam pattern); default is Rainbow so effects render visibly before any selection. paletteOptions() emits the dropdown swatches.
- Drivers: added the `palette` control (addPalette); selecting a palette recolours every palette-driven effect live via onUpdate.
- Effects: converted ~14 effects to colorFromPalette(active, index[, brightness]) (Rainbow, Noise, Rings, Spiral, DistortionWaves, Metaballs, Plasma, GameOfLife→removed, Wave, Particles via Glow merge, AudioVolume, AudioSpectrum per-band); Fire/LavaLamp keep their identity colour, Sine/NetworkReceive/Lines have no single index. Lines stays an R/G/B axis test pattern.
- Effect curation: removed PlasmaPaletteEffect (subset of Plasma), GlowParticlesEffect (identical render to Metaballs — Metaballs gained a `count` control), CheckerboardEffect, GameOfLifeEffect (to be replaced). Plasma default scale 16→48 (bigger, calmer blobs); Fire spark count scales with width so a wide grid lights its whole base; Rings calmed (count 4→2, speed 60→30).
- WaveEffect: first-tick bootstrap so the wave starts at phase 0 instead of jumping by uptime; present-tense comments (palette has landed). (🐇)
- HueDriver: pairing-success path uses resetLightCache() so sent_/lastRgb_ clear and the new bridge session re-sends all lights. (🐇)
UI:
- app.js: palette control renders a custom gradient-swatch dropdown (native <option> can't show gradients, the MoonLight limitation) — a trigger + styled rows, outside-click/Esc dismiss, live WS patch; module-picker now cursors to the current type on replace and scrolls it into view.
- style.css: palette-control / palette-list / palette-item styling.
Scripts / MoonDeck:
- check_specs.py: page-scoped contract — effects/modifiers/leaf-layouts map to one shared per-type page by type-suffix (MoonLive excluded); every control name must appear on its page.
- moondeck.py: _render_cell passes our <img width>/<a id> table-cell HTML through (resolving the gif src to /api/doc-asset/) so the compact pages render previews + row anchors.
- screenshot_modules.py / update_module_docs.py: asset_dir_for E701 multi-line rewrite. (🐇)
Tests:
- Removed the four deleted effects' registrations/tests; renamed the shared effect-render test to unit_effects_render.cpp (Checkerboard→Spiral as the generic D2 subject); swapped scenarios + zero-grid/extrude/phase tests to surviving effects; added MetaballsEffect count to the unit coverage path.
Docs / CI:
- Consolidated ~29 per-module effect/modifier/layout .md into docs/moonmodules/light/{effects,modifiers,layouts}/<type>.md — MoonLight-style compact rows (Name+tags · Preview gif · Description · Controls · Tests), grouped into library sections (MoonLight/WLED/FastLED/projectMM-native); per-library file split kept as future growth. main.cpp registers each module's page#anchor. Drivers + containers stay per-file.
- architecture.md: Tag-emoji-legend h4→h2 and role/origin rows synced to app.js ROLE_EMOJI. (🐇)
- folder-structure-proposal.md: fenced trees tagged `text` (MD040). (🐇)
- HueDriver.md: architecture link path fixed. (🐇)
- README/FastLED attribution and the two saved plans (palette stage 1, doc consolidation).
- 🐇 plan-archive: declined CodeRabbit's ask to rewrite the shipped HueDriver plan's "Decisions locked" to match what shipped — plans are the intent record, kept as written; the (shipped) marker already signals it landed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/light/drivers/HueDriver.h (3)
89-93: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse
sawLights_as the discovery sentinel.A successful
/lightsfetch with zero eligible bulbs leaveslightCount_ == 0, so this path will issue the full GET every second forever. Gate the one-shot fetch on!sawLights_instead;resetLightCache()already flips that flag whenbridgeIporappKeychanges.Suggested fix
void loop1s() override { if (pairTicksLeft_ > 0) { pollPairing(); DriverBase::loop1s(); return; } if (!appKey[0] || !haveBridge()) { DriverBase::loop1s(); return; } - if (lightCount_ == 0) { fetchLights(); DriverBase::loop1s(); return; } + if (!sawLights_) { fetchLights(); DriverBase::loop1s(); return; } if (++reportTick_ >= kReportEverySec) { reportTick_ = 0; reportBridge(); } DriverBase::loop1s(); }🤖 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/drivers/HueDriver.h` around lines 89 - 93, `HueDriver::loop1s` is using `lightCount_ == 0` as the discovery trigger, which causes `fetchLights()` to run every second even after a successful empty `/lights` result. Update the one-shot discovery gate to use `!sawLights_` instead, and keep the existing `resetLightCache()` behavior in `HueDriver` as the mechanism that clears this sentinel when `bridgeIp` or `appKey` changes.
325-333: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly mark a light as sent after a successful PUT.
httpRequest()'s return code is ignored here, butlastRgb_andsent_are still updated. A timeout or error response will then suppress retries for unchanged frames, so that bulb can stay stale until its color changes again.Suggested fix
- platform::httpRequest("PUT", host, 80, path, body, kHttpTimeoutMs, nullptr, 0); - lastRgb_[i][0] = rgb[0]; lastRgb_[i][1] = rgb[1]; lastRgb_[i][2] = rgb[2]; - sent_[i] = true; - pushCursor_ = static_cast<uint8_t>((i + 1) % n); // resume after this one next time + const int st = platform::httpRequest("PUT", host, 80, path, body, kHttpTimeoutMs, nullptr, 0); + pushCursor_ = static_cast<uint8_t>((i + 1) % n); // keep the round-robin moving + if (st == 200) { + lastRgb_[i][0] = rgb[0]; lastRgb_[i][1] = rgb[1]; lastRgb_[i][2] = rgb[2]; + sent_[i] = true; + } return; // ONE PUT — done🤖 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/drivers/HueDriver.h` around lines 325 - 333, Update the Hue send path in HueDriver::push so a light is only marked as sent after a successful PUT. Right now the result of platform::httpRequest() is ignored, but lastRgb_, sent_, and pushCursor_ are still updated immediately after the request; capture the return value from httpRequest(), check for success before mutating those fields, and leave the light eligible for retry on failure or timeout. Keep the change localized to the diffAndFormat() block that builds the host/path and sends the state update.
313-314: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch
transitiontimeto the actual driven window.The round-robin only iterates over
n = min(lightCount_, winLen), buttransitionDeciseconds()uses the full discoveredlightCount_. With a partial driver window, fades are computed as if the skipped lights were still in the rotation, so the bulbs glide much more slowly than the real refresh cadence.Suggested fix
- bool wouldPushForTest(uint8_t idx, uint8_t r, uint8_t g, uint8_t b, char* outBody, size_t cap) { - if (!diffAndFormat(idx, r, g, b, outBody, cap)) return false; + bool wouldPushForTest(uint8_t idx, uint8_t r, uint8_t g, uint8_t b, char* outBody, size_t cap) { + if (!diffAndFormat(idx, 1, r, g, b, outBody, cap)) return false; if (idx < kMaxLights) { lastRgb_[idx][0] = r; lastRgb_[idx][1] = g; lastRgb_[idx][2] = b; sent_[idx] = true; @@ - if (diffAndFormat(i, rgb[0], rgb[1], rgb[2], body, sizeof(body))) { + if (diffAndFormat(i, n, rgb[0], rgb[1], rgb[2], body, sizeof(body))) { @@ - bool diffAndFormat(uint8_t idx, uint8_t r, uint8_t g, uint8_t b, char* out, size_t cap) { + bool diffAndFormat(uint8_t idx, uint8_t activeLights, + uint8_t r, uint8_t g, uint8_t b, char* out, size_t cap) { if (idx >= kMaxLights) return false; if (sent_[idx] && lastRgb_[idx][0] == r && lastRgb_[idx][1] == g && lastRgb_[idx][2] == b) return false; // unchanged — skip - const uint8_t tt = transitionDeciseconds(); + const uint8_t tt = transitionDeciseconds(activeLights); @@ - uint8_t transitionDeciseconds() const { - const uint32_t intervalMs = static_cast<uint32_t>(lightCount_ ? lightCount_ : 1) * kPutIntervalMs; + static uint8_t transitionDeciseconds(uint8_t activeLights) { + const uint32_t intervalMs = static_cast<uint32_t>(activeLights ? activeLights : 1) * kPutIntervalMs; const uint32_t ds = intervalMs / 100; return static_cast<uint8_t>(ds < 1 ? 1 : (ds > 30 ? 30 : ds)); }Also applies to: 350-370
🤖 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/drivers/HueDriver.h` around lines 313 - 314, `transitionDeciseconds()` is using the full discovered `lightCount_` even when the round-robin in `HueDriver` only drives `n = min(lightCount_, winLen)` lights, which skews fade timing for partial windows. Update the transition-time calculation and the call sites in the affected `HueDriver` update path so the window size used for cadence matches the actual driven set (`n`), not the total discovered count, and keep the logic consistent wherever the round-robin window is computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/history/plans/Plan-20260630` - Stage 1 palette.md:
- Line 1: The plan file name is missing the required resolved outcome marker.
Update the Stage 1 palette plan in docs/history/plans/ to include the trailing
“(shipped)” status in its filename, following the existing Plan-YYYYMMDD -
<title>.md convention used for approved plans.
In `@docs/moonmodules/light/Drivers.md`:
- Line 29: The Drivers documentation still uses forward-looking phrasing in the
`palette` entry; update the wording to describe only the current implementation.
In the `palette` row, remove the “a future LightsControl module absorbs it”
language and rephrase the sentence around Drivers owning it as a global render
parameter alongside `brightness` and `lightPreset`, keeping the rest of the
`Palettes::active()` / `colorFromPalette(index)` description unchanged.
In `@docs/moonmodules/light/effects/effects.md`:
- Line 16: The LavaLamp entry in effects.md has an incorrect tests anchor link;
update the tests reference in the LavaLamp row so it points to the
LavaLampEffect anchor instead of the SpiralEffect anchor. Locate the Markdown
table entry for LavaLamp and fix the link target in the existing tests
hyperlink.
- Line 19: The Ripples entry in the effects documentation has the wrong tests
anchor, so the link points to SpiralEffect instead of RipplesEffect. Update the
tests reference in the Ripples row to use the correct anchor target for Ripples,
and verify the surrounding markdown table entry remains consistent with the
existing effect names and links.
- Line 18: The Rings entry in the effects documentation has an incorrect tests
anchor link. Update the markdown link in the “Rings” row so the tests reference
points to the Rings test section instead of the Spiral section, using the
existing effects table entry and its `tests` link target as the place to fix it.
In `@scripts/moondeck.py`:
- Around line 1642-1662: The `_render_cell` helper has multi-statement lines
that violate the lint rule, so split them into separate statements. In the
nested `_img` and `_stash` functions, break out each assignment and return into
its own line while keeping the existing logic unchanged, so the code in
`moondeck.py` passes the Python lint gate.
In `@src/core/Control.h`:
- Around line 129-147: The core control API is leaking a light-specific concept
through ControlType::Palette and PaletteOptionsFn in Control.h, so rename the
control type and callback to a domain-neutral preview/swatch-select concept
while keeping the same behavior. Update the enum entry, the PaletteOptionsFn
alias, and any descriptor or metadata paths that reference these symbols so core
remains agnostic and the light domain still supplies the option data through the
renamed hook.
In `@src/light/effects/FireEffect.h`:
- Around line 89-95: The spark x-position selection in FireEffect::tick still
uses rand8(), which caps placement to 256 buckets and leaves columns unreachable
on wide grids. Update the sx generation to use a wider random source or combine
multiple random bytes before scaling by w, so the column selection in the spark
seeding loop covers the full grid width correctly at any size.
In `@src/light/Palette.h`:
- Around line 64-66: Reword the header comments in Palette.h to describe the
current behavior in present tense instead of future-facing wording. Update the
comment near the seam effects path to state what the blending logic does now,
and remove phrases like “future” or “substitutes” that imply upcoming changes.
Apply the same present-tense rewrite to the related comment block around the
other referenced lines, keeping the intent tied to the current contract of the
palette blending logic.
- Around line 39-50: In sampleGradient(), add handling for positions before the
first stop so the first stop color is returned when pos is less than the initial
stop position; currently the pos - p0 subtraction can underflow when the first
stop in the stops array is above 0. Update the early bounds checks around the
existing last-stop clamp to cover both ends, keeping the logic in sampleGradient
and the lerpRGB path unchanged for in-range positions.
In `@src/main.cpp`:
- Around line 84-110: The ModuleFactory registrations in main need a
restore-time compatibility path for removed effect types, because persisted
module trees may still reference those old type names. Add temporary legacy
aliases or a type-remap layer in the restore/loading flow so the existing
mm::ModuleFactory::registerType entries still resolve old saved configs to the
new modules. Use the affected registrations in main and the module tree restore
code path as the place to hook the remap before shipping.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 499-507: The shared deadline logic in the timeout helper is not
rollover-safe because `deadline` is computed as a raw `millis() + timeoutMs`
value and later compared with `now >= deadline`, which breaks near 32-bit wrap.
Update the `remainingMs` lambda in `platform_desktop.cpp` to use wrap-safe
unsigned subtraction/elapsed-time handling based on `millis()`, so the deadline
budget works correctly across rollover while still flooring to 1 ms.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1119-1127: The shared deadline logic in the ESP32 request path is
not rollover-safe because `millis()` is combined into a single absolute
`deadline` and later compared with `now >= deadline`, which fails across 32-bit
wraparound. Update the deadline handling in the request timeout code near
`remainingMs()` so it uses wraparound-safe elapsed-time arithmetic from
`millis()` and `timeoutMs` instead of a plain absolute timestamp comparison,
keeping the same shared budget behavior for connect/send/recv.
In `@src/ui/app.js`:
- Around line 1176-1269: The palette branch is missing the same default-reset
affordance used by other persisted controls because it never calls
appendResetButton(). In the palette case inside the control-rendering switch,
add the reset button wiring for the palette control using the existing
appendResetButton pattern so it can restore ctrl.default and refresh the
displayed swatch/name via the palette trigger logic.
In `@test/unit/light/unit_Layer_extrude.cpp`:
- Around line 259-260: The doctest name is still referring to Checkerboard even
though this test exercises mm::SpiralEffect, so update the TEST_CASE title to
match the actual effect under test. Keep the description aligned with the
extrusion behavior being verified, and ensure any generated test reports/docs
will now reference SpiralEffect instead of Checkerboard.
In `@test/unit/light/unit_Layer_phase_animation.cpp`:
- Around line 75-76: The animation-slot test currently targets SpiralEffect, but
this case should exercise the new WaveEffect instead; update the relevant
TEST_CASE in unit_Layer_phase_animation.cpp to use WaveEffect with
animates_over_ms so the time-driven loop/trail behavior is covered. Keep
SpiralEffect coverage where it already exists, and make sure the doctest name
and assertion reference WaveEffect so the regression check matches the new
effect.
---
Outside diff comments:
In `@src/light/drivers/HueDriver.h`:
- Around line 89-93: `HueDriver::loop1s` is using `lightCount_ == 0` as the
discovery trigger, which causes `fetchLights()` to run every second even after a
successful empty `/lights` result. Update the one-shot discovery gate to use
`!sawLights_` instead, and keep the existing `resetLightCache()` behavior in
`HueDriver` as the mechanism that clears this sentinel when `bridgeIp` or
`appKey` changes.
- Around line 325-333: Update the Hue send path in HueDriver::push so a light is
only marked as sent after a successful PUT. Right now the result of
platform::httpRequest() is ignored, but lastRgb_, sent_, and pushCursor_ are
still updated immediately after the request; capture the return value from
httpRequest(), check for success before mutating those fields, and leave the
light eligible for retry on failure or timeout. Keep the change localized to the
diffAndFormat() block that builds the host/path and sends the state update.
- Around line 313-314: `transitionDeciseconds()` is using the full discovered
`lightCount_` even when the round-robin in `HueDriver` only drives `n =
min(lightCount_, winLen)` lights, which skews fade timing for partial windows.
Update the transition-time calculation and the call sites in the affected
`HueDriver` update path so the window size used for cadence matches the actual
driven set (`n`), not the total discovered count, and keep the logic consistent
wherever the round-robin window is computed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 368ed30f-6988-4419-8bce-45bc7177f908
📒 Files selected for processing (89)
docs/architecture.mddocs/backlog/folder-structure-proposal.mddocs/history/plans/Plan-20260630 - Doc consolidation - per-type compact pages (effects-modifiers-layouts).mddocs/history/plans/Plan-20260630 - Stage 1 palette.mddocs/moonmodules/core/AudioModule.mddocs/moonmodules/light/Drivers.mddocs/moonmodules/light/ModifierBase.mddocs/moonmodules/light/drivers/HueDriver.mddocs/moonmodules/light/effects/AudioSpectrumEffect.mddocs/moonmodules/light/effects/AudioVolumeEffect.mddocs/moonmodules/light/effects/CheckerboardEffect.mddocs/moonmodules/light/effects/DistortionWavesEffect.mddocs/moonmodules/light/effects/FireEffect.mddocs/moonmodules/light/effects/GameOfLifeEffect.mddocs/moonmodules/light/effects/GlowParticlesEffect.mddocs/moonmodules/light/effects/LavaLampEffect.mddocs/moonmodules/light/effects/LinesEffect.mddocs/moonmodules/light/effects/MetaballsEffect.mddocs/moonmodules/light/effects/NetworkReceiveEffect.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/effects/ParticlesEffect.mddocs/moonmodules/light/effects/PlasmaEffect.mddocs/moonmodules/light/effects/PlasmaPaletteEffect.mddocs/moonmodules/light/effects/RainbowEffect.mddocs/moonmodules/light/effects/RingsEffect.mddocs/moonmodules/light/effects/RipplesEffect.mddocs/moonmodules/light/effects/SineEffect.mddocs/moonmodules/light/effects/SpiralEffect.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/GridLayout.mddocs/moonmodules/light/layouts/SphereLayout.mddocs/moonmodules/light/layouts/WheelLayout.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/modifiers/CheckerboardModifier.mddocs/moonmodules/light/modifiers/MultiplyModifier.mddocs/moonmodules/light/modifiers/RandomMapModifier.mddocs/moonmodules/light/modifiers/RegionModifier.mddocs/moonmodules/light/modifiers/RotateModifier.mddocs/moonmodules/light/modifiers/modifiers.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/check/check_specs.pyscripts/docs/screenshot_modules.pyscripts/docs/update_module_docs.pyscripts/moondeck.pysrc/core/Control.cppsrc/core/Control.hsrc/core/DevicesModule.hsrc/light/Palette.hsrc/light/drivers/Drivers.hsrc/light/drivers/HueDriver.hsrc/light/effects/AudioSpectrumEffect.hsrc/light/effects/AudioVolumeEffect.hsrc/light/effects/CheckerboardEffect.hsrc/light/effects/DistortionWavesEffect.hsrc/light/effects/FireEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/GlowParticlesEffect.hsrc/light/effects/MetaballsEffect.hsrc/light/effects/NoiseEffect.hsrc/light/effects/PlasmaEffect.hsrc/light/effects/PlasmaPaletteEffect.hsrc/light/effects/RainbowEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/SpiralEffect.hsrc/light/effects/WaveEffect.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/light/unit_GameOfLifeEffect.cpptest/unit/light/unit_Layer_extrude.cpptest/unit/light/unit_Layer_phase_animation.cpptest/unit/light/unit_Layer_zero_grid.cpptest/unit/light/unit_Layers_container.cpptest/unit/light/unit_Palette.cpptest/unit/light/unit_effects_render.cpp
💤 Files with no reviewable changes (34)
- docs/moonmodules/light/layouts/SphereLayout.md
- docs/moonmodules/light/effects/PlasmaPaletteEffect.md
- docs/moonmodules/light/modifiers/RandomMapModifier.md
- docs/moonmodules/light/effects/DistortionWavesEffect.md
- docs/moonmodules/light/effects/AudioVolumeEffect.md
- docs/moonmodules/light/modifiers/RotateModifier.md
- docs/moonmodules/light/effects/NoiseEffect.md
- docs/moonmodules/light/layouts/WheelLayout.md
- docs/moonmodules/light/effects/AudioSpectrumEffect.md
- docs/moonmodules/light/effects/CheckerboardEffect.md
- docs/moonmodules/light/effects/PlasmaEffect.md
- docs/moonmodules/light/modifiers/MultiplyModifier.md
- docs/moonmodules/light/effects/GameOfLifeEffect.md
- docs/moonmodules/light/effects/LinesEffect.md
- docs/moonmodules/light/effects/RainbowEffect.md
- src/light/effects/GameOfLifeEffect.h
- docs/moonmodules/light/effects/SineEffect.md
- docs/moonmodules/light/modifiers/CheckerboardModifier.md
- docs/moonmodules/light/effects/NetworkReceiveEffect.md
- src/light/effects/CheckerboardEffect.h
- test/unit/light/unit_GameOfLifeEffect.cpp
- docs/moonmodules/light/effects/RipplesEffect.md
- docs/moonmodules/light/effects/MetaballsEffect.md
- docs/moonmodules/light/modifiers/RegionModifier.md
- docs/moonmodules/light/effects/RingsEffect.md
- docs/moonmodules/light/layouts/GridLayout.md
- docs/moonmodules/light/effects/FireEffect.md
- docs/moonmodules/light/effects/SpiralEffect.md
- src/light/effects/PlasmaPaletteEffect.h
- src/light/effects/GlowParticlesEffect.h
- docs/moonmodules/light/effects/GlowParticlesEffect.md
- docs/moonmodules/light/effects/ParticlesEffect.md
- docs/moonmodules/light/effects/LavaLampEffect.md
- test/scenario_runner.cpp
| @@ -0,0 +1,40 @@ | |||
| # Plan — Stage 1 (palette) of the MoonLight migration | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the resolved outcome marker to the filename.
The coding guidelines require approved feature plans in docs/history/plans/ to carry a trailing outcome such as (shipped) or (attempted, abandoned) when resolved. Since this plan's implementation is part of the shipping PR, rename to include (shipped).
As per coding guidelines, approved feature plans must be saved with the Plan-YYYYMMDD - <title>.md naming pattern, then marked with a trailing outcome such as (shipped) or (attempted, abandoned) when resolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plans/Plan-20260630` - Stage 1 palette.md at line 1, The plan
file name is missing the required resolved outcome marker. Update the Stage 1
palette plan in docs/history/plans/ to include the trailing “(shipped)” status
in its filename, following the existing Plan-YYYYMMDD - <title>.md convention
used for approved plans.
Source: Coding guidelines
| | <a id="spiral"></a>**Spiral** 💫🦅 | <img src="../../../assets/light/effects/SpiralEffect.gif" width="300"> | 2D | Rotating spiral from angle + distance (`atan2_8`/`dist8`). | `bpm`, `twist`, `hue_shift` | [tests](../../../tests/unit-tests.md#spiraleffect) | | ||
| | <a id="distortionwaves"></a>**DistortionWaves** 💫 | — | 2D | Two interfering sine waves beat against each other into a moiré colour field. | `freq_x`, `freq_y`, `speed` | [tests](../../../tests/unit-tests.md#distortionwaveseffect) | | ||
| | <a id="metaballs"></a>**Metaballs** 💫🦅 | <img src="../../../assets/light/effects/MetaballsEffect.gif" width="300"> | 2D | `count` blobs orbit via integer sin/cos; metaball field per pixel — bright HSV merge/split. | `bpm`, `radius`, `count`, `hue_shift` | [tests](../../../tests/unit-tests.md#metaballseffect) | | ||
| | <a id="lavalamp"></a>**LavaLamp** 💫🦅 | <img src="../../../assets/light/effects/LavaLampEffect.gif" width="300"> | 2D | Three slow blobs through a black→red→orange→yellow→white ramp — atmospheric lava look. | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#spiraleffect) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect test link anchor.
The tests link for LavaLamp points to #spiraleffect instead of #lavalampeffect.
- | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#spiraleffect) |
+ | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#lavalampeffect) |📝 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
| | <a id="lavalamp"></a>**LavaLamp** 💫🦅 | <img src="../../../assets/light/effects/LavaLampEffect.gif" width="300"> | 2D | Three slow blobs through a black→red→orange→yellow→white ramp — atmospheric lava look. | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#spiraleffect) | | |
| | <a id="lavalamp"></a>**LavaLamp** 💫🦅 | <img src="../../../assets/light/effects/LavaLampEffect.gif" width="300"> | 2D | Three slow blobs through a black→red→orange→yellow→white ramp — atmospheric lava look. | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#lavalampeffect) | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 16-16: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/light/effects/effects.md` at line 16, The LavaLamp entry in
effects.md has an incorrect tests anchor link; update the tests reference in the
LavaLamp row so it points to the LavaLampEffect anchor instead of the
SpiralEffect anchor. Locate the Markdown table entry for LavaLamp and fix the
link target in the existing tests hyperlink.
| | <a id="metaballs"></a>**Metaballs** 💫🦅 | <img src="../../../assets/light/effects/MetaballsEffect.gif" width="300"> | 2D | `count` blobs orbit via integer sin/cos; metaball field per pixel — bright HSV merge/split. | `bpm`, `radius`, `count`, `hue_shift` | [tests](../../../tests/unit-tests.md#metaballseffect) | | ||
| | <a id="lavalamp"></a>**LavaLamp** 💫🦅 | <img src="../../../assets/light/effects/LavaLampEffect.gif" width="300"> | 2D | Three slow blobs through a black→red→orange→yellow→white ramp — atmospheric lava look. | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#spiraleffect) | | ||
| | <a id="particles"></a>**Particles** 💫🦅 | <img src="../../../assets/light/effects/ParticlesEffect.gif" width="300"> | 2D | A swarm of drifting particles with persistent fading trails. | `count`, `speed`, `fade`, `hue_shift` | [tests](../../../tests/unit-tests.md#particleseffect) | | ||
| | <a id="rings"></a>**Rings** 💫🦅 | <img src="../../../assets/light/effects/RingsEffect.gif" width="300"> | 2D | Expanding concentric rings from random centres, additive overlap (calm defaults). | `count`, `speed`, `thickness`, `hue_shift` | [tests](../../../tests/unit-tests.md#spiraleffect) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect test link anchor.
The tests link for Rings points to #spiraleffect instead of #ringseffect.
- | `count`, `speed`, `thickness`, `hue_shift` | [tests](../../../tests/unit-tests.md#spiraleffect) |
+ | `count`, `speed`, `thickness`, `hue_shift` | [tests](../../../tests/unit-tests.md#ringseffect) |📝 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
| | <a id="rings"></a>**Rings** 💫🦅 | <img src="../../../assets/light/effects/RingsEffect.gif" width="300"> | 2D | Expanding concentric rings from random centres, additive overlap (calm defaults). | `count`, `speed`, `thickness`, `hue_shift` | [tests](../../../tests/unit-tests.md#spiraleffect) | | |
| | <a id="rings"></a>**Rings** 💫🦅 | <img src="../../../assets/light/effects/RingsEffect.gif" width="300"> | 2D | Expanding concentric rings from random centres, additive overlap (calm defaults). | `count`, `speed`, `thickness`, `hue_shift` | [tests](../../../tests/unit-tests.md#ringseffect) | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 18-18: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/light/effects/effects.md` at line 18, The Rings entry in the
effects documentation has an incorrect tests anchor link. Update the markdown
link in the “Rings” row so the tests reference points to the Rings test section
instead of the Spiral section, using the existing effects table entry and its
`tests` link target as the place to fix it.
| | <a id="lavalamp"></a>**LavaLamp** 💫🦅 | <img src="../../../assets/light/effects/LavaLampEffect.gif" width="300"> | 2D | Three slow blobs through a black→red→orange→yellow→white ramp — atmospheric lava look. | `bpm`, `radius`, `intensity` | [tests](../../../tests/unit-tests.md#spiraleffect) | | ||
| | <a id="particles"></a>**Particles** 💫🦅 | <img src="../../../assets/light/effects/ParticlesEffect.gif" width="300"> | 2D | A swarm of drifting particles with persistent fading trails. | `count`, `speed`, `fade`, `hue_shift` | [tests](../../../tests/unit-tests.md#particleseffect) | | ||
| | <a id="rings"></a>**Rings** 💫🦅 | <img src="../../../assets/light/effects/RingsEffect.gif" width="300"> | 2D | Expanding concentric rings from random centres, additive overlap (calm defaults). | `count`, `speed`, `thickness`, `hue_shift` | [tests](../../../tests/unit-tests.md#spiraleffect) | | ||
| | <a id="ripples"></a>**Ripples** 💫🟦🦅 | <img src="../../../assets/light/effects/RipplesEffect.gif" width="300"> | 3D | Distance-from-centre sets a per-column wave phase; the lit surface ripples like water. | `speed`, `interval` | [tests](../../../tests/unit-tests.md#spiraleffect) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect test link anchor.
The tests link for Ripples points to #spiraleffect instead of #rippleseffect.
- | `speed`, `interval` | [tests](../../../tests/unit-tests.md#spiraleffect) |
+ | `speed`, `interval` | [tests](../../../tests/unit-tests.md#rippleseffect) |📝 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
| | <a id="ripples"></a>**Ripples** 💫🟦🦅 | <img src="../../../assets/light/effects/RipplesEffect.gif" width="300"> | 3D | Distance-from-centre sets a per-column wave phase; the lit surface ripples like water. | `speed`, `interval` | [tests](../../../tests/unit-tests.md#spiraleffect) | | |
| | <a id="ripples"></a>**Ripples** 💫🟦🦅 | <img src="../../../assets/light/effects/RipplesEffect.gif" width="300"> | 3D | Distance-from-centre sets a per-column wave phase; the lit surface ripples like water. | `speed`, `interval` | [tests](../../../tests/unit-tests.md#rippleseffect) | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/light/effects/effects.md` at line 19, The Ripples entry in
the effects documentation has the wrong tests anchor, so the link points to
SpiralEffect instead of RipplesEffect. Update the tests reference in the Ripples
row to use the correct anchor target for Ripples, and verify the surrounding
markdown table entry remains consistent with the existing effect names and
links.
Comment on lines
259
to
260
| // SpiralEffect (D2) on a 3D layer: extrude copies z=0 to every z>0 (stateless D2 contract). | ||
| TEST_CASE("D2 effect on 3D layer: Checkerboard extruded across z") { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the doctest case to match the effect under test.
This test now instantiates mm::SpiralEffect, but the title still says Checkerboard. Failed assertions and generated test docs will report the wrong effect.
Proposed fix
-TEST_CASE("D2 effect on 3D layer: Checkerboard extruded across z") {
+TEST_CASE("D2 effect on 3D layer: SpiralEffect extruded across z") {📝 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
| // SpiralEffect (D2) on a 3D layer: extrude copies z=0 to every z>0 (stateless D2 contract). | |
| TEST_CASE("D2 effect on 3D layer: Checkerboard extruded across z") { | |
| // SpiralEffect (D2) on a 3D layer: extrude copies z=0 to every z>0 (stateless D2 contract). | |
| TEST_CASE("D2 effect on 3D layer: SpiralEffect extruded across z") { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/light/unit_Layer_extrude.cpp` around lines 259 - 260, The doctest
name is still referring to Checkerboard even though this test exercises
mm::SpiralEffect, so update the TEST_CASE title to match the actual effect under
test. Keep the description aligned with the extrusion behavior being verified,
and ensure any generated test reports/docs will now reference SpiralEffect
instead of Checkerboard.
Comment on lines
+75
to
+76
| TEST_CASE("SpiralEffect animates over a 100ms gap") { | ||
| CHECK(animates_over_ms<mm::SpiralEffect>(100)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use this animation slot for WaveEffect instead.
SpiralEffect is already covered elsewhere in this file, while WaveEffect is the new effect in this PR and has its own time-driven loop/trail behavior. Repointing this case to WaveEffect would give the new surface a much more valuable regression check.
As per path instructions, "test/**: Unit and integration tests using doctest. Verify tests cover edge cases and match the specifications in docs/moonmodules/. Tests should not depend on timing or network."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/light/unit_Layer_phase_animation.cpp` around lines 75 - 76, The
animation-slot test currently targets SpiralEffect, but this case should
exercise the new WaveEffect instead; update the relevant TEST_CASE in
unit_Layer_phase_animation.cpp to use WaveEffect with animates_over_ms so the
time-driven loop/trail behavior is covered. Keep SpiralEffect coverage where it
already exists, and make sure the doctest name and assertion reference
WaveEffect so the regression check matches the new effect.
Source: Path instructions
…ixes Adds the shared FastLED-style primitive library (math8/noise/draw + blend) and migrates the three MoonModules effects (GameOfLife, GEQ3D, PaintBrush) onto it. Also lands the CodeRabbit review pass, a pre-existing P4 boot-crash fix (HueDriver's 8 KB inline buffer overflowed the main-task stack during type registration), and a dynamic Hue /lights read buffer. KPI: tick:160/125/160/20/2/370/79/23/28/218/155/22/1/45us(FPS:6250/8000/6250/50000/500000/2702/12658/43478/35714/4587/6451/45454/1000000/22222) | ESP32 tick:3577us(FPS:279) | heap:31542KB Core: - math8.h (new): the lib8tion surface — sin8/cos8/triwave8, beat8/beatsin8/beatsin16, qadd8/qsub8/nscale8, a seedable Random8 PRNG, and atan2_8/dist8 (moved from color.h). Integer-only, LUT-backed, FastLED credited. - color.h: slimmed to RGB/hsvToRgb/scale8 (color only); trig/geometry moved to math8.h. - noise.h (new): inoise8 1D/2D/3D value noise, promoted from NoiseEffect's own hash so every effect shares one field generator. - crc.h (new): CRC-16/CCITT-FALSE — a grid fingerprint for Game of Life stasis detection. - DevicesModule: refreshStatus() also runs when a cached device row transitions back to active with no persisted change (a re-announced bridge no longer stays greyed-out). (🐇) - platform_desktop / platform_esp32: httpRequest tracks one shared, wrap-safe deadline across connect/send/recv instead of a fresh timeoutMs per phase (total could reach ~3× the budget). (🐇) Light domain: - GameOfLifeEffect (new, 2D/3D): full faithful port of MoonLight's GoL — 7 rulesets + custom B#/S# parser, 8/26-neighbour with toroidal wrap, palette-coloured cells, dead-cell blur trail, color-by-age, CRC16 stasis detection → R-pentomino respawn. Grid state is heap/PSRAM (bit-packed cells + per-cell colour), allocated in onBuildState, freed in teardown — never an inline member. - GEQ3DEffect (new, 2D, audio): perspective EQ bars with a sweeping projector vanishing point, on draw::line + AudioFrame. - PaintBrushEffect (new, 3D, audio): beat-driven lines whose 3D endpoints oscillate (beatsin8), length gated by band magnitude, over a fading field. - NoiseEffect: rewired to the shared inoise8 (behaviour-preserving); its inline hash/lerp/noise helpers removed. - draw.h (new): draw::pixel / draw::line — bounds-clipped, integer 3D Bresenham, works 1D→3D on the Buffer. Geometry lives once. - Palette.h: added blend(RGB,RGB,amt) and fadeToBlackBy next to colorFromPalette (FastLED colorutils shape). - HueDriver: moved the 8 KB /lights read buffer off the object — it was an inline member, so sizeof(HueDriver) overflowed the 8 KB main-task stack when registerType<HueDriver> built a throwaway probe, bootlooping the P4 (Stack protection fault); fetchLights now sizes the buffer DYNAMICALLY (grow-and-retry from 2 KB, truncation detected by the body's shape since httpRequest strips headers) on heap/PSRAM, freed after. Also: loop1s discovery gates on !sawLights_; push marks a light sent only on a successful PUT; transition fade time uses the driven window size, not the full discovered count. (🐇) - WaveEffect: first-tick bootstrap so the wave starts at phase 0 instead of jumping by uptime; present-tense comments. (🐇) - effects: trig users re-include core/math8.h after the color.h split. - Effect curation tuning: Fire spark x uses a 16-bit random so wide grids (width to 512) light their whole base; Palette sampleGradient guards a first-stop>0 underflow. (🐇) UI: - app.js: palette control gains the default-reset button like every other persisted control. (🐇) Scripts / MoonDeck: - moondeck.py: _render_cell passes our <img>/<a id> table-cell HTML through (resolving the gif src) so the compact effect pages render previews + row anchors; split the E702 multi-statement lines. (🐇) - check_specs.py: page-scoped contract (effects/modifiers/leaf-layouts map to one shared per-type page by suffix). - screenshot_modules.py / update_module_docs.py: asset_dir_for E701 multi-line rewrite. (🐇) Tests: - New: unit_math8, unit_noise, unit_draw (primitives), unit_crc (CCITT vector), unit_GameOfLifeEffect (Conway block stable, blinker period-2, lone cell dies, B/S parser, HighLife). The three new effects pinned in the 0×0×0 robustness test. Full suite ~11998 assertions. Docs / CI: - effects.md: new MoonModules section (GameOfLife/GEQ3D/PaintBrush rows + anchors + source links). - architecture.md: tag-emoji legend h4→h2 and role/origin rows synced to app.js. (🐇) - folder-structure-proposal.md fenced trees tagged text (MD040); HueDriver.md architecture link path fixed. (🐇) - Plans: primitive library, E_MoonModules batch, doc consolidation (Stage 1/3 of the MoonLight migration). Reviews: - 🐇 Declined CodeRabbit's ask to rewrite the shipped HueDriver plan's "Decisions locked" — plans are the intent record, kept as written. Skipped 3 effects.md test-anchor edits (the suggested anchors don't exist; they correctly point to the @module section). Declined renaming ControlType::Palette/colorFromPalette to domain-neutral — recognisable FastLED names are intentional. Declined legacy aliases for removed effect types — restore already degrades gracefully. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/light/drivers/HueDriver.h (1)
41-41: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement the age ramp behind
colorByAge.
colorByAgepromises age-based coloring, but live cells always render asRGB{0, 255, 0}and survivor age is never tracked. Store/reset/increment an age value for cells when this mode is enabled and map it green→red, or remove/rename the control. This also removes the forward-looking “future refinement” comment. As per coding guidelines, “Avoid forward-looking or absence-narration phrasing.”Also applies to: 236-240, 287-289
🤖 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/drivers/HueDriver.h` at line 41, The `colorByAge` control in `HueDriver` is misleading because cell age is never tracked and rendering still hardcodes `RGB{0, 255, 0}`. Update the cell lifecycle in the driver to store, reset, and increment an age value when `colorByAge` is enabled, and use that age to compute a green-to-red color ramp in the live-cell rendering path. If age-based coloring is not intended, remove or rename the `colorByAge` control and eliminate the forward-looking “future refinement” comment so the behavior matches the public API.Source: Coding guidelines
♻️ Duplicate comments (4)
src/main.cpp (1)
87-116: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemoved effect types still lack a restore-time migration path.
This change again drops factory registrations for
PlasmaPaletteEffect,GlowParticlesEffect, andCheckerboardEffect. Persisted module trees referencing those old type names will fail to restore after upgrade. This was already raised previously without a visible resolution — add legacy aliases or a restore-time type remap before shipping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 87 - 116, The module registrations in main() still remove support for old persisted effect type names, so restore will fail for saved trees using them. Add legacy compatibility by registering aliases or remapping the deprecated types PlasmaPaletteEffect, GlowParticlesEffect, and CheckerboardEffect during restore before ModuleFactory lookup, using the existing registerType() / factory restore path in src/main.cpp or the restore-time loader.src/light/drivers/HueDriver.h (1)
77-83: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove Hue PUTs out of the render loop.
Line 83 calls
pushOneChangedLight(), which reaches the blockingplatform::httpRequest()at Line 367. The 500 ms gate reduces frequency but a slow bridge can still stall a render tick for the HTTP timeout. Queue the changed light inloop()and perform the PUT from a non-render tick/worker, or make the transport non-blocking. As per coding guidelines, “In the render loop and anything it calls, avoid heap allocations, blocking calls, mutex blocking, andstd::string.”Also applies to: 367-367
🤖 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/drivers/HueDriver.h` around lines 77 - 83, Move the Hue PUT work out of HueDriver::loop(), since loop() currently calls pushOneChangedLight() and can still block the render tick via platform::httpRequest() inside the PUT path. Update HueDriver so loop() only detects and queues a pending changed light, then have a non-render worker/tick drain that queue and perform the network request, or refactor the transport to be non-blocking. Keep HueDriver::loop() and pushOneChangedLight() as the key symbols to relocate the blocking I/O away from the render path.Source: Coding guidelines
src/platform/esp32/platform_esp32.cpp (1)
1161-1169: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce the deadline across send and recv, not just connect.
smsis captured once and applied to bothSO_SNDTIMEOandSO_RCVTIMEO; a slow send can consume that budget, thenrecv()still gets the full same timeout. RecomputeremainingMs()before each blocking wait, or keep the socket non-blocking and gate send/recv withselect()against the same absolute budget. As per coding guidelines,src/**/*.cpp: “No UI action or API-call sequence may crash or wedge a running device.”Also applies to: 1184-1202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/esp32/platform_esp32.cpp` around lines 1161 - 1169, The timeout handling in the ESP32 socket flow only snapshots remainingMs() once and reuses it for both SO_SNDTIMEO and SO_RCVTIMEO, so send() and recv() are not independently bounded by the shared deadline. Update the socket logic in platform_esp32.cpp around the blocking send/recv sequence to recompute remainingMs() immediately before each blocking wait, or switch the socket path to non-blocking and use select() against the same absolute deadline. Make sure the fix covers the send/receive block identified by the timeout setup near the existing remainingMs() call and the later recv path in the same request flow.Source: Coding guidelines
src/platform/desktop/platform_desktop.cpp (1)
547-560: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce the shared timeout across send and recv.
remainingMs()is sampled once intosms, then bothSO_SNDTIMEOandSO_RCVTIMEOreceive that same value. A slow send can spend the budget, and the receive loop can still block for another fullsms. Recompute before each phase/wait or switch send/recv to deadline-driven non-blocking I/O.Also applies to: 576-596
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/desktop/platform_desktop.cpp` around lines 547 - 560, The shared deadline is only sampled once in the send/receive path, so `remainingMs()` must be re-read before each blocking phase in the request handling logic that sets `SO_SNDTIMEO` and `SO_RCVTIMEO`. Update the code around the socket send/recv flow in `platform_desktop.cpp` so the send timeout and receive timeout each use the current remaining budget, or convert the send/recv loop to deadline-based non-blocking I/O, ensuring the combined request path stays within one shared timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/history/plans/Plan-20260630` - Stage 1 primitive library (math8 + noise
+ draw + blend).md:
- Line 1: Mark the plan as resolved by adding a trailing outcome suffix to the
title (for example, “(shipped)”) and update the body so the PRNG description
matches the actual shipped API in math8.h. Specifically, revise the section
describing random generation to refer to the Random8 class and its next8(),
next16(), and below() methods rather than free functions named random8/random16,
keeping the plan text aligned with the implemented symbols.
In `@docs/history/plans/Plan-20260630` - Stage 3 E_MoonModules batch (GameOfLife +
GEQ3D + PaintBrush).md:
- Line 1: The plan entry is resolved but still lacks the required outcome
marker. Update the plan’s title/filename in docs/history/plans to include a
trailing status such as “(shipped)” so it matches the approved plan naming rule;
keep the existing plan name content intact and apply the marker consistently in
the document title.
In `@src/light/effects/DistortionWavesEffect.h`:
- Line 5: The include comment in DistortionWavesEffect.h is stale because it
still references hsvToRgb even though the effect now uses colorFromPalette.
Update the comment next to the math8.h include so it accurately reflects the
current dependency and remove the outdated color.h/hsvToRgb reference; keep the
change scoped to the comment, not the include itself.
In `@src/light/effects/GameOfLifeEffect.h`:
- Line 41: The GameOfLifeEffect colorByAge option is currently only a flag and
the render path still uses a fixed green, so wire age tracking into the cell
state updates and use that age when coloring. Update the GameOfLifeEffect
update/survivor logic to increment age for surviving cells and initialize
newborns appropriately, then change the render code to map age from green toward
red using the existing colorByAge setting; if that state cannot be tracked yet,
remove the control/comment from GameOfLifeEffect instead.
- Line 174: Guard the ruleset selection in GameOfLifeEffect before accessing
kRulesetOptions, since ruleset is mutable persisted state and can point past the
preset array. Update the logic around the existing ruleset/customRule choice so
invalid values (for example, values at or beyond kRulesetCount) fall back safely
to Conway or customRule instead of indexing kRulesetOptions directly. Keep the
fix localized to the ruleset lookup in GameOfLifeEffect.
In `@src/light/effects/GEQ3DEffect.h`:
- Around line 90-94: The top-surface perspective line in GEQ3DEffect::draw()
collapses when projector_ is left of the bar because the start point is always
anchored at x0 while only the end point switches between x0 and x1. Update the
line endpoints so both sides of the projector sweep use the correct near edge
based on projector_ (in the same logic that computes px and ty), preserving the
diagonal perspective instead of degenerating into a vertical segment.
---
Outside diff comments:
In `@src/light/drivers/HueDriver.h`:
- Line 41: The `colorByAge` control in `HueDriver` is misleading because cell
age is never tracked and rendering still hardcodes `RGB{0, 255, 0}`. Update the
cell lifecycle in the driver to store, reset, and increment an age value when
`colorByAge` is enabled, and use that age to compute a green-to-red color ramp
in the live-cell rendering path. If age-based coloring is not intended, remove
or rename the `colorByAge` control and eliminate the forward-looking “future
refinement” comment so the behavior matches the public API.
---
Duplicate comments:
In `@src/light/drivers/HueDriver.h`:
- Around line 77-83: Move the Hue PUT work out of HueDriver::loop(), since
loop() currently calls pushOneChangedLight() and can still block the render tick
via platform::httpRequest() inside the PUT path. Update HueDriver so loop() only
detects and queues a pending changed light, then have a non-render worker/tick
drain that queue and perform the network request, or refactor the transport to
be non-blocking. Keep HueDriver::loop() and pushOneChangedLight() as the key
symbols to relocate the blocking I/O away from the render path.
In `@src/main.cpp`:
- Around line 87-116: The module registrations in main() still remove support
for old persisted effect type names, so restore will fail for saved trees using
them. Add legacy compatibility by registering aliases or remapping the
deprecated types PlasmaPaletteEffect, GlowParticlesEffect, and
CheckerboardEffect during restore before ModuleFactory lookup, using the
existing registerType() / factory restore path in src/main.cpp or the
restore-time loader.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 547-560: The shared deadline is only sampled once in the
send/receive path, so `remainingMs()` must be re-read before each blocking phase
in the request handling logic that sets `SO_SNDTIMEO` and `SO_RCVTIMEO`. Update
the code around the socket send/recv flow in `platform_desktop.cpp` so the send
timeout and receive timeout each use the current remaining budget, or convert
the send/recv loop to deadline-based non-blocking I/O, ensuring the combined
request path stays within one shared timeout.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1161-1169: The timeout handling in the ESP32 socket flow only
snapshots remainingMs() once and reuses it for both SO_SNDTIMEO and SO_RCVTIMEO,
so send() and recv() are not independently bounded by the shared deadline.
Update the socket logic in platform_esp32.cpp around the blocking send/recv
sequence to recompute remainingMs() immediately before each blocking wait, or
switch the socket path to non-blocking and use select() against the same
absolute deadline. Make sure the fix covers the send/receive block identified by
the timeout setup near the existing remainingMs() call and the later recv path
in the same request flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c217887-5d24-4766-bfe5-7ca9848fa7a7
📒 Files selected for processing (45)
docs/history/plans/Plan-20260630 - MoonLight migration (multi-stage).mddocs/history/plans/Plan-20260630 - Stage 1 palette (shipped).mddocs/history/plans/Plan-20260630 - Stage 1 primitive library (math8 + noise + draw + blend).mddocs/history/plans/Plan-20260630 - Stage 3 E_MoonModules batch (GameOfLife + GEQ3D + PaintBrush).mddocs/moonmodules/light/Drivers.mddocs/moonmodules/light/effects/effects.mdscripts/moondeck.pysrc/core/color.hsrc/core/crc.hsrc/core/math8.hsrc/core/noise.hsrc/light/Palette.hsrc/light/draw.hsrc/light/drivers/HueDriver.hsrc/light/effects/DistortionWavesEffect.hsrc/light/effects/FireEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/LavaLampEffect.hsrc/light/effects/MetaballsEffect.hsrc/light/effects/NoiseEffect.hsrc/light/effects/PaintBrushEffect.hsrc/light/effects/PlasmaEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/SineEffect.hsrc/light/effects/SpiralEffect.hsrc/light/effects/WaveEffect.hsrc/light/layouts/WheelLayout.hsrc/light/modifiers/RotateModifier.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/ui/app.jstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_crc.cpptest/unit/core/unit_math8.cpptest/unit/core/unit_noise.cpptest/unit/light/unit_GameOfLifeEffect.cpptest/unit/light/unit_HueDriver.cpptest/unit/light/unit_Layer_extrude.cpptest/unit/light/unit_Layer_phase_animation.cpptest/unit/light/unit_Layer_zero_grid.cpptest/unit/light/unit_draw.cpp
💤 Files with no reviewable changes (1)
- docs/history/plans/Plan-20260630 - Stage 1 palette (shipped).md
| @@ -0,0 +1,55 @@ | |||
| # Plan — Stage 3 E_MoonModules batch (GameOfLife + GEQ3D + PaintBrush) | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Plan looks resolved but has no outcome marker.
All three effects plus the supporting core/crc.h/tests this plan calls for are present in this PR's file set, so the plan appears shipped. Per the path instruction, mark it with a trailing outcome (e.g. (shipped)) in the title/filename now that it's resolved.
As per path instructions, "Approved feature plans must be saved in docs/history/plans/ with the Plan-YYYYMMDD - <title>.md naming pattern, then marked with a trailing outcome such as (shipped) or (attempted, abandoned) when resolved."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plans/Plan-20260630` - Stage 3 E_MoonModules batch (GameOfLife +
GEQ3D + PaintBrush).md at line 1, The plan entry is resolved but still lacks the
required outcome marker. Update the plan’s title/filename in docs/history/plans
to include a trailing status such as “(shipped)” so it matches the approved plan
naming rule; keep the existing plan name content intact and apply the marker
consistently in the document title.
Source: Path instructions
| uint8_t density = 30; // initial live fraction, 10..90 % | ||
| uint8_t mutation = 5; // % chance a newborn gets a fresh random colour instead of inheriting | ||
| bool wrap = true; // toroidal edges | ||
| bool colorByAge = false; // green newborn → red as it ages, instead of palette colour |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make colorByAge actually age cells.
The control says cells color by age, but newborn/survivor state never records age and render always uses a constant green. Track age for survivors and map it green→red, or remove the control/comment until that behavior exists. As per coding guidelines, “Avoid forward-looking or absence-narration phrasing.”
Also applies to: 236-240, 287-289
🤖 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/GameOfLifeEffect.h` at line 41, The GameOfLifeEffect
colorByAge option is currently only a flag and the render path still uses a
fixed green, so wire age tracking into the cell state updates and use that age
when coloring. Update the GameOfLifeEffect update/survivor logic to increment
age for surviving cells and initialize newborns appropriately, then change the
render code to map age from green toward red using the existing colorByAge
setting; if that state cannot be tracked yet, remove the control/comment from
GameOfLifeEffect instead.
Source: Coding guidelines
Re-ports the three E_MoonModules effects (Game of Life, GEQ3D, PaintBrush) from MoonLight with exact behavioural fidelity, on a new shared 3D-aware primitive library (beat/noise/draw/blend), and adds room+light selection to the Hue driver so users can drive a subset of their bridge's bulbs. The effect docs are reshaped so each module's controls sit next to its description. KPI: tick:3592us(FPS:278) Core - math8.h: extended to the FastLED 5-arg beatsin8/beatsin16 (bpm, ms, low, high, timebase, phase), beat8(timebase), map8, and Random8::below(min,max) — the timing/oscillator surface the migrated effects need. Light domain - draw.h: unified 3D-aware primitives — one line() (1D→3D) with a shorten parameter (MoonLight's depth line-shortening), plus pixel/get/blendPixel/fade/fill; replaces the per-dimension draw helpers. - GameOfLifeEffect / GEQ3DEffect / PaintBrushEffect: full faithful rewrites against the new primitives. GoL keeps the heap grid state, neighbour-colour inheritance, green→red aging, CRC stasis detection and R-pentomino/glider respawn; GEQ3D restores the perspective lines toward the projector; PaintBrush restores the partial-line swept-curve mechanism (the dropped length/shorten param was why its lines drew straight). - HueDriver: added room/light dropdowns (extracted from the bridge /groups, type==Room) that narrow which colour lights the driver drives — both default to All, picking a room repopulates the light list. Moved the per-light/per-room name buffers off the resident struct onto the heap (allocated to the bridge on fetch, freed on teardown/cache-reset): sizeof(HueDriver) drops ~1.15KB and an unconfigured driver pays nothing. Folded the duplicate hueStatus + colourLights controls into the single status line, now showing the driven-of-total count (paired, 3-4 lights when filtered). - DistortionWavesEffect: corrected a stale comment (colorFromPalette/sin8, not hsvToRgb). UI - app.js: the select update-case now re-syncs its <option> list when the served options differ from what's rendered, so async-populated dropdowns (the Hue room/light lists) show their full set instead of only the default. Scripts / MoonDeck - moondeck.py: card cells pass <img>/<a id>/<br> through, and a standalone <img> line resolves to /api/doc-asset/ — so the reshaped doc heading-blocks render in MoonDeck. Tests - unit_HueDriver: added a room/light filter test (All=4, room=2, light=1) and a status-format test (paired, 4 lights vs paired, 2-4 lights). New unit_math8 / unit_noise / unit_draw / unit_crc / unit_GameOfLifeEffect pin the primitives + the GoL automaton (Conway block stable, blinker period-2). Docs / CI - effects.md / modifiers.md / layouts.md: reshaped to per-module heading blocks (anchor + heading + preview + description + controls, then a Tests link below the controls) so each module's controls sit next to its description. - HueDriver.md: documented the room/light controls, the /groups wire contract, and the folded status line. - plans: marked the Stage-1 primitive-library and Stage-3 E_MoonModules-batch plans (shipped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules/light/drivers/HueDriver.md`:
- Around line 25-27: Persist the Hue selection by stable IDs instead of the
current dropdown index in HueDriver’s room/light handling. Update the logic that
reads/writes the `room` and `light` settings so the saved value is the bridge
`group`/`light` ID, and only derive the displayed dropdown index from the
current `/api/state` option list when rendering. Keep the `All` case as the
default sentinel and make sure the filter/window mapping still uses the selected
IDs to resolve the driven lights.
In `@docs/moonmodules/light/effects/effects.md`:
- Line 13: Add descriptive alt text to each effect preview image in effects.md
so the markdownlint alt-text rule passes and the docs remain accessible. Update
every relevant img tag in the effect sections (including the one for
RainbowEffect and the other referenced preview images) to include a concise,
meaningful alt attribute that describes the effect shown, keeping the existing
image sources and sizing intact.
- Around line 3-4: Update the opening organization paragraph in effects.md to
describe the current structure only: explain that effects are grouped by origin
into sections and that each effect block includes its preview, behavior, and
control descriptions together. Remove the forward-looking “future work”
phrasing, and keep the folder-structure decision link only as supporting
context. Refer to the existing effects overview text and the
tags()/Layer/modifiers/driver explanation when revising the wording.
In `@docs/moonmodules/light/layouts/layouts.md`:
- Around line 3-4: Update the introductory paragraph in the Layouts docs to
point `[driver]` at a concrete `.md` page that the MoonDeck viewer can rewrite,
and keep the wording contract-focused. In the layouts overview text, remove
roadmap/meta phrasing like “future work” and “Preview gifs land…” and instead
describe the current Layouts/Layer contract and control meanings using the
existing symbols such as `Layouts`, `Layer`, and the layout types mentioned
there.
In `@docs/moonmodules/light/modifiers/modifiers.md`:
- Line 11: Add alt text to the preview images in the markdown file so the
`<img>` tags pass markdownlint and remain accessible to screen readers. Update
the image tag(s) used for the modifier previews, including the one referenced by
the diff and the other matching image instance mentioned in the review, by
adding a descriptive alt attribute that identifies the preview content.
- Around line 3-4: The opening overview in modifiers.md uses forward-looking
“future work” language for the file split, which should be replaced with a
description of the current layout. Update the introductory text near the main
modifiers overview to explain how modifiers are organized today, while keeping
the folder-structure decision link only as background context; preserve the rest
of the existing concepts around ModifierBase, Layer, and tags() without
mentioning what will happen later.
In `@docs/tests/unit-tests.md`:
- Around line 924-928: Update the draw test docs to use the exported helper name
introduced by src/light/draw.h, replacing the old drawPixel reference with
mm::draw::pixel() in the draw section so the documented test maps directly to
the current API; keep the rest of the unit_draw.cpp description unchanged.
In `@scripts/moondeck.py`:
- Line 1671: The lambda in the re.sub call is ignoring the regex match, so
rename the unused parameter in the re.sub callback to _ in the code around the
_stash helper usage. Keep the behavior the same, just update the lambda
signature so Ruff no longer reports an unused argument in this substitution.
In `@src/light/drivers/HueDriver.h`:
- Around line 353-364: The cache reset in resetLightCache() frees the name
buffers used by addSelect-backed dropdowns, but the borrowed
roomOptions_/lightOptions_ pointers are left stale. After freeNameBuffers(),
immediately rebuild or rebind the dropdown option arrays so roomOptions_ and
lightOptions_ point to valid storage before rebuildDriven() runs, using the
resetLightCache(), freeNameBuffers(), and addSelect-related state as the place
to fix it.
In `@src/light/effects/GameOfLifeEffect.h`:
- Around line 381-411: The 3D neighbor handling in GameOfLifeEffect currently
uses `neighbors` directly to index `birthNumbers_` and `surviveNumbers_`, but in
the `d > 1` path that count can exceed the 9-entry table size. Update the logic
around the `neighbors`-based birth/survival checks so the 3D case never indexes
those B/S tables out of bounds, using the existing `GameOfLifeEffect` control
flow and the `birthNumbers_`/`surviveNumbers_` lookups to clamp, remap, or
otherwise guard the index before access.
- Around line 332-356: The placePentomino() path updates future_ and draws the
injected cells, but it never assigns a corresponding entry in colors_, so newly
placed live cells lose their color state and break later neighbor-color
inheritance. In the placement loop inside GameOfLifeEffect::placePentomino, set
colors_[i2] for every cell that is marked alive, using the same palette-derived
color (or the appropriate colorByAge value) that is used for drawing. Keep the
color assignment aligned with the existing setBit(future_, i2, true) logic so
injected pentomino cells are fully initialized.
In `@src/light/effects/GEQ3DEffect.h`:
- Around line 96-99: The bar layout in GEQ3DEffect is using cols / NUM_BANDS,
which truncates to zero on narrow grids and collapses positions like linex,
pPos, pPos1, and the border edges. Update the band placement logic in the
affected GEQ3DEffect rendering code paths to compute each band’s start/end from
remapped boundaries or scaled coordinates instead of a shared integer width, so
the math stays nonzero across all grid sizes and tick rates. Apply the same fix
consistently wherever the same width calculation appears in GEQ3DEffect.
- Around line 34-50: The sweep in GEQ3DEffect is frame-throttled via speed, so
it varies with loop() rate instead of elapsed time and does not follow the BPM
control rule. Replace the counter-based stepping with time-based motion driven
by elapsed milliseconds in the effect’s update/render path, and expose the
control as BPM rather than a raw frame speed. Update
GEQ3DEffect::onBuildControls and the logic that advances the projector sweep so
movement is consistent across tick rates and grid sizes, including fast devices
and degenerate 0×0×0 cases.
In `@test/unit/light/unit_HueDriver.cpp`:
- Around line 112-115: The paired-mode setup in the HueDriver status test should
not depend on the current size of HueDriver::appKey; replace the unbounded
std::strcpy use in the TEST_CASE with a size-independent non-empty marker so
status() still sees a paired key without risking UB if the field layout changes.
Keep the setup minimal by writing a single character plus terminator or using a
bounded copy approach, and preserve the test’s intent of exercising
HueDriver::status().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fce8b49c-84f4-4707-a37e-4a0cbface004
📒 Files selected for processing (18)
docs/history/plans/Plan-20260630 - Stage 1 primitive library (math8 + noise + draw + blend) (shipped).mddocs/history/plans/Plan-20260630 - Stage 3 E_MoonModules batch (GameOfLife + GEQ3D + PaintBrush) (shipped).mddocs/moonmodules/light/drivers/HueDriver.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/modifiers/modifiers.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/moondeck.pysrc/core/math8.hsrc/light/draw.hsrc/light/drivers/HueDriver.hsrc/light/effects/DistortionWavesEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/PaintBrushEffect.hsrc/ui/app.jstest/unit/light/unit_HueDriver.cpp
💤 Files with no reviewable changes (1)
- docs/history/plans/Plan-20260630 - Stage 3 E_MoonModules batch (GameOfLife + GEQ3D + PaintBrush) (shipped).md
Comment on lines
+25
to
+27
| - `room` / `light` — two **dropdowns** that narrow *which* colour lights the driver drives. Both default to `All`. Pick a `room` (extracted from the bridge's `/groups`, `type == "Room"`) to drive only that room's colour lights; the `light` dropdown then lists just that room's lights, so picking one drives a single bulb. `room = All, light = All` drives every colour light (the default). The dropdowns store the selected **index** (Hue's room/light *order* — stable on a settled bridge; `All` is always index 0 so the common case never shifts); the option *names* are regenerated from the bridge each time the page reads `/api/state`, they are not persisted. | ||
| - `start` / `count` — the window of the shared buffer this driver drives ([the standard driver window](NetworkSendDriver.md); `count` 0 = to the end of the buffer). Window index *i* maps to the *i*-th **driven** light — i.e. the *i*-th light of the current `room`/`light` filter, not the raw bridge list. | ||
| The module's generic **status** line carries the driver state: `unpaired`, `pairing: press the bridge button`, `pairing timed out`, or — once paired — `paired, M lights` (M = colour-capable, reachable lights on the bridge), shown as `paired, N-M lights` when the `room`/`light` filter narrows the driven set to N of M. Size a grid layout (N×1×1) to the driven count to map every driven bulb to a pixel. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist Hue room/light IDs, not dropdown indices.
Saving the selection as a live-list index means a bridge reorder, room change, or added/removed bulb can silently retarget the driver to a different room/light on the next refresh. Persist the Hue group/light IDs and derive the UI index from the current option list 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 `@docs/moonmodules/light/drivers/HueDriver.md` around lines 25 - 27, Persist
the Hue selection by stable IDs instead of the current dropdown index in
HueDriver’s room/light handling. Update the logic that reads/writes the `room`
and `light` settings so the saved value is the bridge `group`/`light` ID, and
only derive the displayed dropdown index from the current `/api/state` option
list when rendering. Keep the `All` case as the default sentinel and make sure
the filter/window mapping still uses the selected IDs to resolve the driven
lights.
Comment on lines
+3
to
+4
| Every layout, one block each: what it does and what each control means — together. A layout maps light indices to physical `(x, y, z)` positions — it defines the *shape* an [effect](../effects/effects.md) draws onto and a [driver](../drivers/) sends out. The [Layouts](../Layouts.md) container holds one or more layout children and composes them into one coordinate space; a [Layer](../Layer.md) renders over that combined space. The per-library file split is future work — see the [folder-structure decision](../../../backlog/folder-structure-proposal.md). Preview gifs land with the [MoonLight migration](../../../history/plans/Plan-20260630%20-%20MoonLight%20migration%20(multi-stage).md) (MoonLight documents matching layout nodes — Panel ≈ Grid, Wheel — at <https://moonmodules.org/MoonLight/moonlight/layouts/>; Sphere is projectMM-native). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Link this to a concrete docs page and keep the intro contract-focused.
[driver](../drivers/) won't resolve in MoonDeck's doc viewer because the renderer only rewrites relative links that end in .md, so this currently sends readers to a dead directory URL. The same paragraph also drifts into roadmap/meta text (future work, Preview gifs land…) instead of documenting the current contract.
As per coding guidelines, "Avoid forward-looking or absence-narration phrasing; describe what exists now rather than what was removed or what will happen later." As per path instructions, "docs/moonmodules/*.md must be end-user/API-integrator documentation that explains contracts, cross-domain wiring, prior art, and at least one mention of every control name, without repeating facts already stated in the corresponding header."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/light/layouts/layouts.md` around lines 3 - 4, Update the
introductory paragraph in the Layouts docs to point `[driver]` at a concrete
`.md` page that the MoonDeck viewer can rewrite, and keep the wording
contract-focused. In the layouts overview text, remove roadmap/meta phrasing
like “future work” and “Preview gifs land…” and instead describe the current
Layouts/Layer contract and control meanings using the existing symbols such as
`Layouts`, `Layer`, and the layout types mentioned there.
Sources: Coding guidelines, Path instructions
Brings the bulk of MoonLight's effect library into projectMM: 19 faithfully-ported effects across four themed batches (simple 2D/3D, particle/physics, audio-reactive) plus a DemoReel that cycles every registered effect, on a 60-palette set (the full MoonLight gradient collection) and a new unified blur primitive. Effects, modifiers and layouts now list alphabetically in the picker, the code, and the docs. Also folds in the CodeRabbit review fixes from the previous PR.
KPI: tick:3529us(FPS:283)
Core
- math8/color: no change to the surface; effects consume the existing beatsin/inoise/scale8 primitives.
Light domain
- Palette: expanded the built-in set from 10 to 60 — the full MoonLight palettes.h gradient collection (reformatted into our {pos,R,G,B} stop layout), plus removed the dead parallel kNames array (kBuiltins[].name is the single source).
- draw: added a unified draw::blur(buf, dims, amt) working 1D→3D — the canonical FastLED blur1d carryover-seep, run on the raw bytes (stride walk, no per-pixel getRGB/setRGB/Coord3D) so it's the fastest form while byte-identical to MoonLight's blur1d/blur2d output.
- effects (batch 3a, 9): Solid, StarSky, SphereMove, StarField, Praxis, FixedRectangle, Random, Lissajous, RubiksCube — faithful ports of the MoonLight/WLED originals.
- effects (batch 3b/3c/3d, 10): Noise2D, BouncingBalls, Tetrix, PacMan, Ant, and the audio-reactive FreqSaws, Blurz, FreqMatrix, GEQ, NoiseMeter — behaviour reproduced from the source, own implementation on EffectBase + the shared primitives.
- DemoReelEffect: a projectMM-native effect that hosts one other registered effect at a time and auto-advances on a timer (sequential or shuffle), reusing the create→build→teardown lifecycle a Layer runs — plays the whole library hands-free without compositing (that stays the Layer stack's job).
- HueDriver: resetLightCache now rebuilds the room/light option arrays after freeing the name buffers, so the addSelect-borrowed pointers can't dangle into freed storage.
- GameOfLifeEffect: clamp the 3D neighbour count before indexing the 9-entry B/S rule tables (a 3×3×3 neighbourhood yields up to 26, which read out of bounds); and set colors_ for injected pentomino cells so neighbour-colour inheritance sees them as live.
UI
- app.js: the module picker sorts its choices alphabetically by display name (localeCompare), so effects/modifiers/layouts/drivers list in order regardless of registration sequence.
Scripts / MoonDeck
- moondeck.py: renamed an unused re.sub lambda parameter to _ (Ruff unused-arg).
Tests
- unit_draw: pin draw::blur against an inline carryover-seep reference on a 1D row, plus 2D/3D spread and degenerate-size safety.
- unit_DemoReelEffect: the reel enumerates the registry, hosts+renders each effect, skips itself, and survives the per-swap create/teardown/delete churn (the robustness path).
- unit_GameOfLifeEffect: a dense 3D neighbourhood (3×3×3 cube, centre has 26 neighbours) steps without an out-of-bounds B/S table read.
- unit_HueDriver: a size-independent non-empty appKey marker instead of an unbounded strcpy.
Docs / CI
- effects.md / modifiers.md / layouts.md: blocks reordered alphabetically within each library section; added the 20 new effect blocks + the DemoReel block; present-tense intros and img alt-text (CodeRabbit).
- moonlight-fidelity-tensions.md: new log of every place strict MoonLight fidelity collides with a projectMM principle (GEQ3D frame-sweep vs BPM, cols/NUM_BANDS on narrow grids, audio volume-scale, the reconstructed-logic bench cross-checks) for product-owner decisions.
- moonlight-palettes-data.md: deleted — the captured gradient data now lives in Palette.h (shipped).
- unit-tests.md: draw section uses mm::draw::pixel(); added DemoReel test notes.
Reviews
- 🐇 HueDriver resetLightCache stale option pointers — fixed (rebuild options after free).
- 🐇 GameOfLife 3D neighbours index B/S tables out of bounds — fixed (clamp <9) + regression test.
- 🐇 GameOfLife placePentomino didn't set colors_ — fixed.
- 🐇 unit_HueDriver unbounded strcpy — fixed (size-independent marker).
- 🐇 moondeck.py unused lambda arg — fixed.
- 🐇 effects/modifiers/layouts alt-text + present-tense intros; unit-tests drawPixel name — fixed.
- 🐇 GEQ3D cols/NUM_BANDS narrow-grid collapse + frame-throttled sweep — accepted as faithful to MoonLight (would diverge from the effect users know); logged in moonlight-fidelity-tensions.md for decision.
- 🐇 HueDriver persist room/light by stable ID not index — deferred: a control-mechanism redesign, not a bug (index approach was the chosen minimal design); flagged for product owner.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/light/drivers/HueDriver.h (1)
371-380: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
bodyLooksComplete()still accepts some truncated bridge responses.A
/lightsor/groupsbody clipped exactly after a complete entry in a larger object still ends in}. In that case this heuristic treats the response as whole, and discovery silently drops the remaining lights/rooms. This needs a transport-level truncation signal (for example bytes-read/full-buffer information fromplatform::httpRequest()), not a suffix check on the parsed body alone.Also applies to: 395-405, 487-494
🤖 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/drivers/HueDriver.h` around lines 371 - 380, The truncation check in bodyLooksComplete() is too weak because a clipped /lights or /groups response can still end with a closing brace, so discovery may stop early and miss remaining entries. Replace this suffix-based heuristic with a transport-level completeness signal from platform::httpRequest() (such as bytes-read versus buffer capacity or a full-buffer/truncated flag), and thread that result through fetchLights, the groups fetch path, and any other callers using bodyLooksComplete() so they can reliably detect incomplete reads.docs/tests/scenario-tests.md (1)
771-776: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the documented observed window to July 1, 2026.
This section no longer matches
test/scenarios/light/scenario_perf_full.json, which now records the secondpc-macossample formeasure-heavy-128on2026-07-01.Suggested fix
-- `pc-macos`: observed 2026-06-17 → 2026-06-30 +- `pc-macos`: observed 2026-06-17 → 2026-07-01🤖 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/tests/scenario-tests.md` around lines 771 - 776, The documented observed window for the pc-macos scenario is stale and should be updated to include the new July 1, 2026 sample recorded in test/scenarios/light/scenario_perf_full.json. Update the observed date range in the scenario performance documentation alongside the pc-macos entry so it matches the latest measure-heavy-128 data, keeping the esp32, esp32p4-eth, and esp32s3-n16r8 entries unchanged.scripts/moondeck.py (1)
1642-1657: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
altwhen re-emitting raw<img>tags.The new docs now carry alt text, but both raw-image render paths drop it from the generated HTML, so MoonDeck still serves every preview image without accessible text.
Suggested fix
def _img(m): src_ = m.group("src") width = m.group("w") style = m.group("style") + alt_m = re.search(r'\balt="([^"]*)"', m.group(0)) + alt_attr = f' alt="{html_mod.escape(alt_m.group(1))}"' if alt_m else "" if not src_.startswith(("http://", "https://", "/")): abs_src = (md_path.parent / src_).resolve() try: src_ = str(abs_src.relative_to(ROOT.resolve())) except ValueError: pass src_ = f"/api/doc-asset/{src_}" wattr = f' width="{width}"' if width else "" # Preserve an author-set width style (the cross-renderer size lever) and append our # margin so the preview isn't flush against the cell edges. style = (style + ";" if style else "") + "margin:4px 0" - return f'<img src="{src_}"{wattr} style="{style}">' + return f'<img src="{src_}"{wattr}{alt_attr} style="{style}">' ... img_m = re.fullmatch(r'<img src="(?P<src>[^"]+)"(?:\s+width="(?P<w>\d+)")?[^>]*>', stripped_check) if img_m: src_ = img_m.group("src") + alt_m = re.search(r'\balt="([^"]*)"', stripped_check) + alt_attr = f' alt="{html_mod.escape(alt_m.group(1))}"' if alt_m else "" if not src_.startswith(("http://", "https://", "/")): abs_src = (md_path.parent / src_).resolve() try: src_ = f"/api/doc-asset/{abs_src.relative_to(ROOT.resolve())}" except ValueError: pass wattr = f' width="{img_m.group("w")}"' if img_m.group("w") else "" - lines.append(f'<img src="{src_}"{wattr} style="margin:4px 0">') + lines.append(f'<img src="{src_}"{wattr}{alt_attr} style="margin:4px 0">') continueAlso applies to: 1667-1672, 1811-1821
🤖 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 `@scripts/moondeck.py` around lines 1642 - 1657, The raw <img> re-emission path in _img is dropping the alt attribute, so preserve m.group("alt") when rebuilding the HTML and include it in the returned img tag. Apply the same fix to the other raw-image render helpers mentioned in the comment so all image preview paths keep alt text consistently. Use the existing _img-style reconstruction logic as the anchor point and add alt preservation without changing the current src/width/style handling.
♻️ Duplicate comments (3)
src/light/effects/GEQ3DEffect.h (2)
96-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid
cols / NUM_BANDSfor band geometry.When
cols < NUM_BANDS, integer division makes each band width zero, collapsinglinex,pPos,pPos1, and border endpoints. Compute each band’s start/end from scaled boundaries instead.As per path instructions,
src/light/**: “Effects must run correctly at every grid size and tick rate, including 0×0×0, and must not truncate animation math to zero on fast devices.”Suggested direction
- const int linex = i * (cols / NUM_BANDS); + const int linex = imap(i, 0, NUM_BANDS, 0, cols); + const int nextX = imap(i + 1, 0, NUM_BANDS, 0, cols); + if (nextX <= linex) continue;Also applies to: 124-127, 151-155, 174-186
🤖 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` around lines 96 - 103, The band geometry in GEQ3DEffect is still derived from cols / NUM_BANDS, which can truncate to zero when the grid is smaller than the band count and collapse positions like linex, pPos, and border endpoints. Update the geometry calculations in the affected GEQ3DEffect drawing logic to derive each band’s start/end from scaled boundaries or proportional ranges instead of integer division, so the bar layout stays valid for any cols value.Source: Path instructions
34-42: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the projector sweep time-based and expose it as BPM.
speedis still a frame throttle viacounter_++, so projector motion changes withloop()frequency. Drive the sweep from elapsed time/BPM instead.As per path instructions,
src/light/**: “Speed controls use BPM (uint8_t).” and “Effects must run correctly at every grid size and tick rate.”Also applies to: 67-72
🤖 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` around lines 34 - 42, The projector sweep in GEQ3DEffect is still driven by frame throttling through speed/counter_++, so it changes with loop frequency instead of elapsed time. Update the GEQ3DEffect update logic to derive sweep progress from time/BPM and keep it stable across different tick rates and grid sizes, and adjust onBuildControls to expose the setting as BPM rather than a frame-based speed control. Make sure the state used by the sweep/render path in GEQ3DEffect stays synchronized with the new time-based BPM behavior.Source: Path instructions
src/light/effects/GameOfLifeEffect.h (1)
332-360: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep generated color indices non-zero.
Line 332 and Lines 425-426 use
rng_.next8(), which can return0even thoughcolors_[i] == 0is treated as the dead/no-color marker. Use the same non-zero range asstartNewGame()for injected and mutated cells.Proposed fix
- const uint8_t colorIndex = rng_.next8(); + const uint8_t colorIndex = rng_.below(1, 255); @@ - uint8_t colorIndex = (colorCount > 0) ? nColors[rng_.below(colorCount)] : rng_.next8(); - if (rng_.below(100) < mutation) colorIndex = rng_.next8(); + uint8_t colorIndex = (colorCount > 0) ? nColors[rng_.below(colorCount)] : rng_.below(1, 255); + if (rng_.below(100) < mutation) colorIndex = rng_.below(1, 255);Also applies to: 425-427
🤖 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/GameOfLifeEffect.h` around lines 332 - 360, The injected and mutated cell color generation in GameOfLifeEffect can produce 0 via rng_.next8(), which conflicts with colors_[i] == 0 being the dead/no-color marker. Update the colorIndex assignment in the injection path and the mutation path to use the same non-zero palette range as startNewGame(), and keep the existing storage/inheritance logic in place so newly created cells never get a zero color marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules/light/effects/effects.md`:
- Line 3: The link in the Effects overview is pointing to a directory path
instead of a Markdown page, so update the reference in effects.md where the
drivers are mentioned to use a concrete .md target. Use the existing Drivers.md
link already referenced nearby (or another specific driver page) so MoonDeck can
rewrite it correctly; keep the rest of the overview text unchanged.
In `@src/core/math8.h`:
- Around line 85-91: map8() currently scales by rangeEnd - rangeStart, which
makes the upper bound unreachable and breaks one-step spans. Update map8() to
include rangeEnd in the mapping for the rangeStart <= rangeEnd case, and keep
the function’s behavior consistent for callers such as GEQ3DEffect that rely on
a height of 1 being possible. Verify the logic in map8() and any helper it
depends on so small ranges do not collapse to zero.
In `@src/light/drivers/HueDriver.h`:
- Around line 231-238: The discovery flow in HueDriver’s name-buffer setup is
marking lights/groups as complete even when allocation fails, which prevents
retries. Update ensureNameBuffers() to return success/failure instead of
silently leaving lightNames_ and roomNames_ null, and have parseLights() and
parseGroups() only set sawLights_ / sawGroups_ when the required buffers were
allocated. If allocation is unavailable, leave discovery incomplete so the
bridge can retry on the next pass.
- Around line 525-535: Replace the unchecked atoi-based id parsing in the bridge
JSON parsers with a validated strtoul-style helper that checks the end pointer
and range before accepting an id. Update the parsing logic in parseGroups(),
roomMaskFor(), and the matching path in parseLights() so malformed bridge
responses are rejected instead of silently producing bad ids or mis-mapped
rooms/lights. Keep the existing commit/pending object flow intact, but only
assign pendingId after the parsed value has been fully validated.
In `@src/light/effects/AntEffect.h`:
- Around line 86-87: The D1 render path in AntEffect is still using the old
row-based convention for the strip, so update the 1D extrusion logic to source
pixels from the x=0 column and advance along Y instead of writing across X.
Apply the same coordinate/dimension correction in the duplicated D1 handling
blocks in AntEffect so all 1D effects use the source column consistently,
including when the grid is empty.
- Around line 18-20: The collision mode in AntEffect is ignoring the exposed
passBy control whenever gatherFood is enabled, so the passBy=false behavior
never takes effect under default settings. Update the logic in AntEffect so the
passByEff decision honors passBy (or explicitly change the documented
contract/comment if gatherFood must override it), and make sure the behavior in
the ant collision handling paths matches that choice.
In `@src/light/effects/BlurzEffect.h`:
- Around line 88-100: The reconstruction comments in BlurzEffect should be
rewritten to describe the current implementation contract in present tense
instead of narrating missing source or reconstruction history. Update the
comments around the frequency mapping and geqScanner logic in BlurzEffect so
they explain what the code does now, using the existing symbols f->peakHz,
kMaxFreqLog10, kLoLog10, and geqScanner, and remove any “RECONSTRUCTED” or
“missing upstream source” phrasing.
In `@src/light/effects/DemoReelEffect.h`:
- Around line 30-33: DemoReelEffect is bypassing the normal Layer extrusion path
by calling current_->loop() directly while declaring Dim::D3, which causes
hosted D1/D2 effects to lose their intended extrusion. Update DemoReelEffect to
render the child through the same Layer-based loop/extrude flow, or explicitly
extrude the child using its dimensions() after loop(); use the
DemoReelEffect::dimensions and current_->loop() path as the anchor points, and
make the same fix in the related child-rendering sections noted by the comment
so all hosted effects work across grid sizes.
In `@src/light/effects/FixedRectangleEffect.h`:
- Around line 85-93: The RGBW write logic in FixedRectangleEffect still applies
the white channel to every cell, which makes the non-white tiles pick up white
when alternateWhite is enabled. Update the per-cell handling in
FixedRectangleEffect so the 4th-channel write is gated by the same alternate
branch used for the white/colour decision, and explicitly clear the white
channel on coloured tiles when the buffer may preserve old data. Use the
existing symbols alternateWhite, alternate, draw::pixel, and draw::offsetOf to
keep the RGB and white-channel paths aligned.
In `@src/light/effects/FreqSawsEffect.h`:
- Around line 88-145: The per-band state in FreqSawsEffect is being updated
inside the x loop, so bandSpeed, bandPhase, lastBpm, and phaseOffset advance
multiple times per frame for columns that map to the same band. Move the
physics/update logic out of the per-column loop and compute each band's
speed/phase and resulting Y position once per tick, then have the drawing loop
reuse that cached snapshot for every x. Use the existing FreqSawsEffect band
arrays and the method-specific logic to locate the refactor point.
In `@src/light/effects/GameOfLifeEffect.h`:
- Around line 386-407: In GameOfLifeEffect’s neighbor scan, the dead-cell color
sampling can collect more than the 9 slots available in nColors, but the later
random pick via rng_.below(colorCount) may still use the full count and read out
of bounds. Update the accumulation logic in the 3D neighbor loop to cap the
number of stored colors to the size of nColors, and make the selection path in
the same GameOfLifeEffect code use the capped sample count before indexing
nColors.
In `@src/light/effects/GEQEffect.h`:
- Around line 63-66: The peak history in GEQEffect is being stored as uint8_t,
which truncates barHeight for tall panels and breaks rendering above 255 rows.
Update GEQEffect’s peak storage and related reads/writes (including the
initialization and update logic around peaks_, peakCount_, and barHeight in
GEQEffect.h) to use a wider type that can represent the full row range, and keep
any zero-init/reset behavior consistent across all affected peak-handling sites.
In `@src/light/effects/LissajousEffect.h`:
- Around line 66-73: In LissajousEffect’s point mapping, the degenerate-axis
fallback is wrong for 1-cell-wide or 1-cell-high grids because lx/ly become 1
instead of 0 and get clipped. Update the coordinate calculation in the effect’s
draw loop so the size-1 case maps to coordinate 0, while still preserving the
existing 0×0×0-safe behavior. Use the LissajousEffect render logic and the lx/ly
computation near draw::pixel as the target for the fix.
In `@src/light/effects/NoiseMeterEffect.h`:
- Around line 87-90: The comment in NoiseMeterEffect should be rewritten to
describe the current behavior only, removing the unresolved “RECONSTRUCTED” and
“PO to cross-check” language. Update the note near the noise-field scrolling
logic to explain that aux0 is advanced by two slow, slightly detuned oscillators
so the field weaves instead of scrolling at a constant rate, and keep the
wording grounded in what mode_noisemeter() does now.
In `@src/light/effects/PacManEffect.h`:
- Around line 78-79: The Pac-Man D1 write path in PacManEffect is using the
wrong axis convention for pixels. Update the buffer indexing in the effect’s
write loop so it writes source points as {0, idx, 0} and uses dimensions {1, n,
1} instead of {idx, 0, 0} with {n, 1, 1}; make the same change in the related
block referenced by the comment so the effect behaves correctly across all grid
sizes and tick rates, including 0×0×0.
In `@src/light/effects/PaintBrushEffect.h`:
- Around line 72-81: The PaintBrushEffect coordinate generation is truncating
grid bounds to uint8_t in the beatsin8 calls, which breaks larger grids. Update
the x1/x2/y1/y2 and z1/z2 logic in PaintBrushEffect to avoid passing cols - 1,
rows - 1, and depth - 1 as 8-bit values; instead generate the oscillator output
in 0..255 and scale it to the full lengthType range so the effect works
correctly for any grid size.
- Around line 65-68: The index mapping in PaintBrushEffect is off by one: the
loop iterates over 0..numLines-1 but the map() calls in the band/gradient
selection still use numLines as the upper bound, which prevents the last
band/gradient entry from ever being reached. Update the mapping logic in
PaintBrushEffect to use numLines - 1 as the input high bound for both the band
index and gradient mapping paths, including the related code at the other
occurrence noted in the diff.
In `@src/light/effects/RubiksCubeEffect.h`:
- Around line 201-216: drawCube() is doing floating-point multiply/round work
inside the hot voxel render loop, so replace the per-voxel projection math with
fixed-point or integer scaling. Use the existing drawCube/voxel loop and the
sizeX/sizeY/sizeZ setup to precompute integer step values once, then derive nX,
nY, and nZ with integer arithmetic instead of scaleX/scaleY/scaleZ and roundI
calls. Apply the same change to the related code in the later drawCube section
as well.
In `@src/light/effects/SolidEffect.h`:
- Around line 86-174: The white channel handling in SolidEffect::render is
inconsistent because only the colorMode 0 branch calls writeWhite, so RGBW
buffers can keep stale W data when switching to palette/band modes or when white
is set to 0. Update SolidEffect::render so every mode either explicitly clears W
when it should be off or computes and writes the white channel consistently,
using the existing writeWhite helper and the same brightness/white policy as
mode 0.
In `@src/light/effects/SphereMoveEffect.h`:
- Around line 54-59: The SphereMoveEffect oscillator is truncating animation
math because time_interval in SphereMoveEffect.h is computed with integer
division in the time_interval initialization. Update the calculation in the
SphereMoveEffect effect logic to keep ms and (100 - speed) in floating-point
before dividing by 6.4f, preserving smooth phase progression at low speeds. Use
the existing SphereMoveEffect and time_interval symbols to locate the fix and
ensure no intermediate integer division remains.
In `@src/light/effects/StarFieldEffect.h`:
- Around line 38-40: The StarFieldEffect trail fade logic is inverted relative
to the documented contract on the blur field, since draw::fade treats larger
values as a stronger clear. Update the logic in StarFieldEffect (including the
blur handling used in the effect update/draw path and the related initialization
path) so that higher blur values produce longer streaks, by converting the
stored blur amount to the correct fade strength before calling draw::fade. Keep
the behavior consistent wherever blur is applied, including the spots referenced
in the effect implementation.
In `@src/light/effects/StarSkyEffect.h`:
- Around line 53-57: StarSkyEffect is truncating light indices on PSRAM builds
because indexes_ is stored as uint16_t* while nrOfLightsType can exceed 65,535.
Update the StarSkyEffect allocation and indexing logic to use a wider index type
consistently (including indexes_, respawn paths, and any helpers that read/write
star targets) so large installations are addressed correctly. Make sure the
selection logic in the StarSkyEffect update/respawn code no longer relies on
rng_.next16() for the upper bound when count can exceed 16 bits.
In `@src/light/effects/TetrixEffect.h`:
- Around line 67-72: The deadline logic in TetrixEffect still compares raw
millisecond timestamps directly, which breaks across 32-bit wraparound. Update
the checks around platform::millis(), drops_[i].step, and any later d.step
comparison in the TetrixEffect update/fade flow to use wrap-safe
elapsed/deadline arithmetic instead of direct greater-than timestamp
comparisons. Ensure the same wrap-safe pattern is applied wherever the effect
waits on step values so a wrapped counter cannot leave a column stuck for an
entire cycle.
In `@test/unit/light/unit_DemoReelEffect.cpp`:
- Around line 57-67: The DemoReelEffect unit test only checks s.anyNonZero()
after the full loop, so it can pass from pixels left over from earlier
iterations. In unit_DemoReelEffect.cpp, update the per-hosted-effect loop around
reel.advanceForTest(), reel.currentTypeForTest(), and s.layer.loop() so the
buffer is cleared before each render and the non-zero assertion happens
immediately after each loop() call. Keep the existing checks for
currentTypeForTest() and the DemoReelEffect type, but validate rendering on
every hosted effect rather than only at the end.
In `@test/unit/light/unit_draw.cpp`:
- Around line 108-166: Add a doctest covering `draw::line(..., shorten)` in the
existing `unit_draw.cpp` suite, since the new `shorten` branch in `draw::line`
is currently untested. Create a small, deterministic case that exercises
`shorten` values 0, 1, 128, and 255 and asserts the endpoints are pulled back as
expected. Use the `draw::line` and `draw::pixel` helpers to verify the rendered
positions/lengths rather than timing-based checks. Keep it consistent with the
nearby `draw::blur` tests and focus on the rounding behavior of the shorten
logic.
---
Outside diff comments:
In `@docs/tests/scenario-tests.md`:
- Around line 771-776: The documented observed window for the pc-macos scenario
is stale and should be updated to include the new July 1, 2026 sample recorded
in test/scenarios/light/scenario_perf_full.json. Update the observed date range
in the scenario performance documentation alongside the pc-macos entry so it
matches the latest measure-heavy-128 data, keeping the esp32, esp32p4-eth, and
esp32s3-n16r8 entries unchanged.
In `@scripts/moondeck.py`:
- Around line 1642-1657: The raw <img> re-emission path in _img is dropping the
alt attribute, so preserve m.group("alt") when rebuilding the HTML and include
it in the returned img tag. Apply the same fix to the other raw-image render
helpers mentioned in the comment so all image preview paths keep alt text
consistently. Use the existing _img-style reconstruction logic as the anchor
point and add alt preservation without changing the current src/width/style
handling.
In `@src/light/drivers/HueDriver.h`:
- Around line 371-380: The truncation check in bodyLooksComplete() is too weak
because a clipped /lights or /groups response can still end with a closing
brace, so discovery may stop early and miss remaining entries. Replace this
suffix-based heuristic with a transport-level completeness signal from
platform::httpRequest() (such as bytes-read versus buffer capacity or a
full-buffer/truncated flag), and thread that result through fetchLights, the
groups fetch path, and any other callers using bodyLooksComplete() so they can
reliably detect incomplete reads.
---
Duplicate comments:
In `@src/light/effects/GameOfLifeEffect.h`:
- Around line 332-360: The injected and mutated cell color generation in
GameOfLifeEffect can produce 0 via rng_.next8(), which conflicts with colors_[i]
== 0 being the dead/no-color marker. Update the colorIndex assignment in the
injection path and the mutation path to use the same non-zero palette range as
startNewGame(), and keep the existing storage/inheritance logic in place so
newly created cells never get a zero color marker.
In `@src/light/effects/GEQ3DEffect.h`:
- Around line 96-103: The band geometry in GEQ3DEffect is still derived from
cols / NUM_BANDS, which can truncate to zero when the grid is smaller than the
band count and collapse positions like linex, pPos, and border endpoints. Update
the geometry calculations in the affected GEQ3DEffect drawing logic to derive
each band’s start/end from scaled boundaries or proportional ranges instead of
integer division, so the bar layout stays valid for any cols value.
- Around line 34-42: The projector sweep in GEQ3DEffect is still driven by frame
throttling through speed/counter_++, so it changes with loop frequency instead
of elapsed time. Update the GEQ3DEffect update logic to derive sweep progress
from time/BPM and keep it stable across different tick rates and grid sizes, and
adjust onBuildControls to expose the setting as BPM rather than a frame-based
speed control. Make sure the state used by the sweep/render path in GEQ3DEffect
stays synchronized with the new time-based BPM behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d41cecd-e22d-4458-a21f-4f353abc62c2
📒 Files selected for processing (54)
docs/backlog/moonlight-fidelity-tensions.mddocs/history/plans/Plan-20260630 - Stage 1 primitive library (math8 + noise + draw + blend) (shipped).mddocs/history/plans/Plan-20260630 - Stage 3 E_MoonModules batch (GameOfLife + GEQ3D + PaintBrush) (shipped).mddocs/moonmodules/light/drivers/HueDriver.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/modifiers/modifiers.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/moondeck.pysrc/core/math8.hsrc/light/Palette.hsrc/light/draw.hsrc/light/drivers/Drivers.hsrc/light/drivers/HueDriver.hsrc/light/effects/AntEffect.hsrc/light/effects/BlurzEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/DistortionWavesEffect.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/LissajousEffect.hsrc/light/effects/Noise2DEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PacManEffect.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/main.cppsrc/ui/app.jstest/CMakeLists.txttest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/light/unit_DemoReelEffect.cpptest/unit/light/unit_GameOfLifeEffect.cpptest/unit/light/unit_HueDriver.cpptest/unit/light/unit_draw.cpp
💤 Files with no reviewable changes (1)
- docs/history/plans/Plan-20260630 - Stage 3 E_MoonModules batch (GameOfLife + GEQ3D + PaintBrush) (shipped).md
Comment on lines
+231
to
+238
| void ensureNameBuffers() { | ||
| if (!lightNames_) lightNames_ = static_cast<char*>(platform::alloc(static_cast<size_t>(kMaxLights) * kNameLen)); | ||
| if (!roomNames_) roomNames_ = static_cast<char*>(platform::alloc(static_cast<size_t>(kMaxRooms) * kNameLen)); | ||
| } | ||
| void freeNameBuffers() { | ||
| platform::free(lightNames_); lightNames_ = nullptr; | ||
| platform::free(roomNames_); roomNames_ = nullptr; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep discovery retryable when the name buffers cannot be allocated.
parseLights() and parseGroups() mark discovery as complete even when ensureNameBuffers() leaves lightNames_/roomNames_ null. A transient allocation failure then collapses the bridge to zero lights/rooms and suppresses all future retries until another config reset. Make the buffer allocation path return success/failure and leave sawLights_ / sawGroups_ false when the required storage is unavailable.
Also applies to: 443-477, 509-537
🤖 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/drivers/HueDriver.h` around lines 231 - 238, The discovery flow in
HueDriver’s name-buffer setup is marking lights/groups as complete even when
allocation fails, which prevents retries. Update ensureNameBuffers() to return
success/failure instead of silently leaving lightNames_ and roomNames_ null, and
have parseLights() and parseGroups() only set sawLights_ / sawGroups_ when the
required buffers were allocated. If allocation is unavailable, leave discovery
incomplete so the bridge can retry on the next pass.
Comment on lines
+525
to
+535
| const char* q = std::strchr(p, '"'); | ||
| if (!q) break; | ||
| int id = std::atoi(q + 1); | ||
| const char* close = std::strchr(q + 1, '"'); | ||
| if (id > 0 && close && close[1] == ':') { | ||
| commit(q); // the PREVIOUS group's object ends here | ||
| pendingId = id; | ||
| pendingStart = close + 1; | ||
| } | ||
| p = close ? close + 1 : q + 1; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file first, then read the relevant ranges.
ast-grep outline src/light/drivers/HueDriver.h --view expanded || true
echo '--- lines 500-570 ---'
sed -n '500,570p' src/light/drivers/HueDriver.h | cat -nRepository: MoonModules/projectMM
Length of output: 4488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding declarations and the second atoi site.
grep -nE 'hueId_|parseGroups|roomMaskFor|std::atoi|strtoul|strtol|kMaxLights|kMaxRooms|pendingId' src/light/drivers/HueDriver.h | cat -n
echo '--- lines 1-220 ---'
sed -n '1,220p' src/light/drivers/HueDriver.h | cat -n
echo '--- lines 540-565 ---'
sed -n '540,565p' src/light/drivers/HueDriver.h | cat -nRepository: MoonModules/projectMM
Length of output: 22909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -nE 'hueId_|parseGroups|roomMaskFor|std::atoi|strtoul|strtol|kMaxLights|kMaxRooms|pendingId' src/light/drivers/HueDriver.h | cat -n
echo '--- lines 1-220 ---'
sed -n '1,220p' src/light/drivers/HueDriver.h | cat -n
echo '--- lines 540-565 ---'
sed -n '540,565p' src/light/drivers/HueDriver.h | cat -nRepository: MoonModules/projectMM
Length of output: 22909
Replace atoi() with checked parsing in the bridge JSON parsers. parseGroups(), roomMaskFor(), and the same pattern in parseLights() accept malformed ids silently today; a bad bridge response can drop or mis-map rooms/lights. A small strtoul() helper with end-pointer and range checks would reject bad data cleanly.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 526-526: Avoid atoi/atol/atoll/atof: they perform no error detection, returning 0 on non-numeric input and invoking undefined behavior on out-of-range values. This rule flags every use of these functions regardless of data provenance. Convert strings with strtol/strtoul/strtod and check errno (and the endptr) so malformed or overflowing input is rejected.
Context: atoi(q + 1)
Note: [CWE-20] Improper Input Validation.
(atoi-no-error-detection-c)
🤖 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/drivers/HueDriver.h` around lines 525 - 535, Replace the unchecked
atoi-based id parsing in the bridge JSON parsers with a validated strtoul-style
helper that checks the end pointer and range before accepting an id. Update the
parsing logic in parseGroups(), roomMaskFor(), and the matching path in
parseLights() so malformed bridge responses are rejected instead of silently
producing bad ids or mis-mapped rooms/lights. Keep the existing commit/pending
object flow intact, but only assign pendingId after the parsed value has been
fully validated.
Source: Linters/SAST tools
Comment on lines
+18
to
+20
| // With `passBy` off, ants that meet collide elastically (the heavier-speed one reverses), resolved | ||
| // with a closed-form collision-time solve so two ants exchange momentum at the exact crossing moment | ||
| // rather than passing through each other. `antSize` widens each ant to a short body; `blur` softens |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make passBy effective when food gathering is enabled.
Line 93 forces passByEff true whenever gatherFood is true, so the exposed passBy=false collision mode does nothing under the default settings. Either honor passBy here or change the control/comment contract.
Proposed fix if collisions should follow the control
- const bool passByEff = passBy || gatherFood;
+ const bool passByEff = passBy;Also applies to: 93-93
🤖 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/AntEffect.h` around lines 18 - 20, The collision mode in
AntEffect is ignoring the exposed passBy control whenever gatherFood is enabled,
so the passBy=false behavior never takes effect under default settings. Update
the logic in AntEffect so the passByEff decision honors passBy (or explicitly
change the documented contract/comment if gatherFood must override it), and make
sure the behavior in the ant collision handling paths matches that choice.
Comment on lines
+38
to
+40
| uint8_t speed = 20; // advance rate (0..30); 0 = paused. Throttle is 1000/speed ms. | ||
| uint8_t numStars = 16; // active stars (1..255) | ||
| uint8_t blur = 128; // per-frame fade-to-black amount (0..255); higher = longer streaks |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make blur match the documented streak direction.
draw::fade(..., 255) is used as a full clear elsewhere, so passing blur directly makes higher values shorten trails, contrary to Line 40’s “higher = longer streaks” contract.
Proposed fix
- uint8_t blur = 128; // per-frame fade-to-black amount (0..255); higher = longer streaks
+ uint8_t blur = 128; // streak persistence: 0 = clear quickly, 255 = longest streaks
@@
- draw::fade(buf, blur);
+ draw::fade(buf, static_cast<uint8_t>(255 - blur));Also applies to: 94-95
🤖 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/StarFieldEffect.h` around lines 38 - 40, The
StarFieldEffect trail fade logic is inverted relative to the documented contract
on the blur field, since draw::fade treats larger values as a stronger clear.
Update the logic in StarFieldEffect (including the blur handling used in the
effect update/draw path and the related initialization path) so that higher blur
values produce longer streaks, by converting the stored blur amount to the
correct fade strength before calling draw::fade. Keep the behavior consistent
wherever blur is applied, including the spots referenced in the effect
implementation.
Continues the MoonLight migration: adds the modifier and layout libraries (6 modifiers, 12 layouts), a bitmap-font text renderer with a TextEffect, and grows DemoReel into a hands-free showcase that cycles every effect with a random palette and an on-screen name label. Also folds in the CodeRabbit review of the previous effect PR — 14 effect fixes, several of which are deliberate improvements over MoonLight, registered in a new log. Ant and PacMan (1D) are removed; a real 2D/3D PacMan is backlogged. KPI: tick:3536us(FPS:282) Core - math8: map8 now reaches its range top (lo + in*(hi-lo)/255, FastLED's documented map8), so a one-step span like a 1-row audio bar is reachable instead of collapsing to 0. Light domain - draw: added the glyph/text bitmap-font blitter (draw::glyph reads columns MSB-first — leftmost pixel is bit 7 — so text isn't left-right mirrored; draw::text lays out multi-line strings, newline wraps down a font height). - fonts: two flash-resident console fonts (4x6, 6x8) carried from the public raster-fonts set; more are pure data to add later. - TextEffect: renders a multi-line string (the shared text-area control) in a selectable font — static top-left by default, an optional horizontal marquee, palette-coloured; default text "projectMM". - Modifiers (6, MoonLight): Mirror, Transpose, Circle, Block, Pinwheel, RippleXZ — coordinate transforms on ModifierBase (modifyLogicalSize + modifyLogical), the 1D→2D/3D expanders (Circle/Block) among them. - Layouts (12, MoonLight): SingleRow, SingleColumn, Panel, Panels, Cube, Human Sized Cube, Tubes, Ring, Rings 241, Spiral, Car Lights, Toronto Bar Gourds — emit their LED positions through LayoutBase (lightCount + forEachCoord). - DemoReelEffect: added randomPalette (a fresh palette each cycle) and showName (overlay the playing effect's name, top-left) — both default on, making the reel a showcase/test tool for the whole library. - MoonLiveEffect: default script is now random pixels (an always-visible starting example) instead of solid blue. - Effect fixes (CodeRabbit): FreqSaws integrates each band once per frame (grid-width-independent speed); SphereMove uses float time (smooth motion); Lissajous maps a size-1 axis to 0 (draws on thin grids); PaintBrush spans the full grid on large panels; FixedRectangle clears the W channel on coloured RGBW tiles; GameOfLife guards the 3D neighbour count against the 9-entry B/S tables and gives injected/mutated cells a non-zero colour; GEQ/StarSky widen index/peak types for large installations; Tetrix uses wrap-safe deadlines; RubiksCube keeps its projection (perf note in code). UI - app.js: (from the prior batch) the module picker sorts alphabetically — the 18 new modules slot in by name. Scripts / MoonDeck - moondeck.py: the raw-<img> card renderer preserves the alt attribute (markdownlint + accessibility). Tests - unit_draw: pin the glyph orientation (the 'L' probe — bar on the left, foot at the bottom — catches a mirror/flip regression), draw::line shorten rounding, and draw::blur vs a reference. - unit_TextEffect: static render, newline wrap, and scroll safety at any grid size. - unit_math8: map8 reaches both range ends including one-step spans. - unit_DemoReelEffect: clears the buffer between hosted renders so the render check measures each host. - unit_GameOfLifeEffect: a dense 3D neighbourhood steps without an out-of-bounds B/S read. Docs / CI - effects.md / modifiers.md / layouts.md: blocks for the new modules (alphabetical within each library section); WLED-origin effect previews cross-linked from WLED-Utils (scottrbailey) with credit; StarSky / Car Lights / Toronto Bar Gourds previews moved from the image pool into assets and wired. - moonlight-improvements.md (new): registers each deliberate divergence from MoonLight that improves an effect (map8 bar heights, FreqSaws timing, SphereMove smoothness, Lissajous thin-grid, PaintBrush coverage, FixedRectangle RGBW), so improvements are decisions on record. - backlog-light.md: a real 2D/3D PacMan, and a fix for DemoReel extruding hosted 1D/2D effects (the first attempt crashed and was reverted). - moonlight_images: drained the layouts folder — controls-panel screenshots deleted, render gifs moved to assets. Reviews - 🐇 map8 upper bound unreachable — fixed (FastLED-documented form) + test; logged as an improvement. - 🐇 FreqSaws per-column band physics; SphereMove integer time; Lissajous size-1; PaintBrush uint8 bounds + off-by-one; FixedRectangle RGBW white; GameOfLife 3D OOB + zero-colour marker; GEQ/StarSky index widths; Tetrix wrap — all fixed; visible ones registered in moonlight-improvements.md. - 🐇 DemoReel extrude hosted lower-D effects — attempted, crashed the suite (heap/vtable smash), reverted + backlogged with root-cause direction. - 🐇 moondeck raw-img alt text; effects.md driver link → concrete page — fixed. - 🐇 GEQ3D cols/NUM_BANDS narrow-grid + frame-throttled sweep — accepted as faithful to MoonLight; in moonlight-fidelity-tensions.md. - 🐇 HueDriver persist-by-ID — deferred (control-mechanism redesign, not a bug). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
scripts/moondeck.py (1)
1815-1828: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve
alton standalone raw images too.Lines 1818-1828 rebuild a raw
<img>line with onlysrcandwidth, so any authoredalttext disappears._render_cell()now preservesaltfor table-cell images, so the two raw-image paths already behave differently.Proposed fix
- img_m = re.fullmatch(r'<img src="(?P<src>[^"]+)"(?:\s+width="(?P<w>\d+)")?[^>]*>', stripped_check) + img_m = re.fullmatch( + r'<img src="(?P<src>[^"]+)"(?:\s+width="(?P<w>\d+)")?' + r'(?:\s+alt="(?P<alt>[^"]*)")?[^>]*>', + stripped_check, + ) if img_m: src_ = img_m.group("src") @@ wattr = f' width="{img_m.group("w")}"' if img_m.group("w") else "" - lines.append(f'<img src="{src_}"{wattr} style="margin:4px 0">') + altattr = f' alt="{html_mod.escape(img_m.group("alt"))}"' if img_m.group("alt") else "" + lines.append(f'<img src="{src_}"{wattr}{altattr} style="margin:4px 0">') continue🤖 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 `@scripts/moondeck.py` around lines 1815 - 1828, The standalone raw image handling in moondeck.py drops authored alt text when rebuilding an <img> line. Update the raw <img> path near the img_m parsing logic to capture and preserve the optional alt attribute the same way _render_cell() does for table-cell images, and include it when appending the reconstructed tag along with src and width.docs/tests/unit-tests.md (1)
933-944: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe draw test index is missing the new
shortenand glyph regressions.
test/unit/light/unit_draw.cppnow pins bothdraw::line(..., shorten)rounding anddraw::glyph()orientation, but this section still only documents pixel/line/blur coverage.Proposed change
- A line running off the grid clips: it draws the on-grid part and stops, no crash. +- `draw::line(..., shorten)` pulls the far endpoint back toward the start with the expected WLEDMM rounding. - draw::blur on a 1D row matches the canonical carryover-seep reference byte-for-byte (same behaviour as FastLED blur1d / MoonLight blurRows), and is symmetric around a centred bright pixel. - blur runs separably on every axis with extent>1: a 2D blur spreads a centre pixel to all four orthogonal neighbours; a 3D blur reaches the z neighbours too. And it never writes out of bounds. +- `draw::glyph()` blits fonts upright and un-mirrored.🤖 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/tests/unit-tests.md` around lines 933 - 944, Update the draw test index entry to include the newly added regression coverage for `draw::line(..., shorten)` rounding and `draw::glyph()` orientation, since `test/unit/light/unit_draw.cpp` now pins both behaviors. Keep the existing pixel/line/blur bullets, and add brief notes for the `shorten` line-case and glyph orientation checks so the documentation matches the current `unit_draw.cpp` coverage.docs/tests/scenario-tests.md (1)
575-635: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThese three
pc-macosFPS rows no longer matchscenario_perf_full.json.
measure-lcdnow storestick_us: [0, 12], andmeasure-parlio/measure-light-16store[0, 9], so the documented ranges should be83,333-—and111,111-—, not142,857-—.🤖 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/tests/scenario-tests.md` around lines 575 - 635, The pc-macos FPS values in the three measure scenario tables are out of sync with the stored tick_us ranges from scenario_perf_full.json. Update the `measure-lcd`, `measure-parlio`, and `measure-light-16` entries in this doc so the documented pc-macos FPS range matches the current tick data, and verify the `Performance` tables for those scenarios reflect the correct computed FPS values.docs/backlog/moonlight-improvements.md (1)
1-33: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove this shipped decision log out of
docs/backlog/.This reads like durable product/architecture documentation for behavior that is shipping now, so keeping it under
docs/backlog/turns a temporary draft area into a permanent registry.As per coding guidelines,
docs/backlog/**/*.md: "Backlog items are temporary working drafts: delete them when the work ships, and do not treatdocs/backlog/as append-only."🤖 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/moonlight-improvements.md` around lines 1 - 33, This shipped decision log should not remain under the backlog area now that the behavior is approved and durable. Move the content from moonlight-improvements.md out of docs/backlog/ into the appropriate permanent documentation location, and remove the backlog copy so docs/backlog/ stays reserved for temporary drafts only.Source: Coding guidelines
src/light/draw.h (1)
199-207: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
draw::fill()against zero-channel buffers.With
channelsPerLight() == 0,off += cplnever advances, so any caller wedges in this loop.Proposed change
inline void fill(Buffer& buf, RGB c) { const uint8_t cpl = buf.channelsPerLight(); + if (cpl == 0) return; uint8_t* d = buf.data(); const size_t n = buf.bytes(); for (size_t off = 0; off + cpl <= n; off += cpl) { if (cpl >= 1) d[off + 0] = c.r; if (cpl >= 2) d[off + 1] = c.g;🤖 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 199 - 207, Guard draw::fill() against zero-channel buffers by checking the result of Buffer::channelsPerLight() before entering the write loop. In fill(Buffer& buf, RGB c), return early or otherwise skip iteration when cpl is 0 so the off += cpl loop cannot stall, and keep the existing per-channel writes unchanged for valid channel counts.
♻️ Duplicate comments (2)
src/light/effects/SolidEffect.h (1)
92-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScale the white channel by
brightnesstoo.Lines 92-93 now clear stale W correctly, but mode 0 still writes raw
white. On RGBW layers,brightness=0therefore leaves the white channel lit even though the RGB channels were scaled to black.Proposed fix
- if (cpl >= 4) writeWhite(buf, nLights, cpl, white); + if (cpl >= 4) + writeWhite(buf, nLights, cpl, + static_cast<uint8_t>(white * brightness / 255));🤖 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 92 - 93, The white channel in SolidEffect is still written unscaled in mode 0, so `brightness` does not affect RGBW output consistently. Update the `SolidEffect` write path that handles `white` (the `writeWhite` call guarded by `cpl >= 4`) to apply the same `brightness` scaling used for the RGB channels before writing, so a zero-brightness frame clears W as well. Use the existing `white`/`brightness` handling in `SolidEffect` as the source of truth and keep the stale-W clearing behavior intact.test/unit/light/unit_DemoReelEffect.cpp (1)
63-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis still doesn't prove each hosted effect rendered.
Line 74 only proves that at least one iteration produced pixels somewhere in the run. In this fixture the reel is cycling known-rendering hosts, so a single dead host can still slip through. As per path instructions,
test/**: “Verify tests cover edge cases and match the specifications in docs/moonmodules/.”Proposed fix
- int rendered = 0; for (int step = 0; step < n * 2 + 3; step++) { reel.advanceForTest(); const char* cur = reel.currentTypeForTest(); REQUIRE(cur != nullptr); CHECK(std::strcmp(cur, "DemoReelEffect") != 0); draw::fill(s.layer.buffer(), {0, 0, 0}); // clear so the count measures THIS host's output s.layer.loop(); // render the newly-hosted effect — must not crash - if (s.anyNonZero()) rendered++; + CHECK(s.anyNonZero()); } - CHECK(rendered > 0); // over the run, hosted effects render into the freshly-cleared buffer🤖 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_DemoReelEffect.cpp` around lines 63 - 74, The current test in unit_DemoReelEffect only asserts that at least one hosted effect rendered, so a dead host can still pass unnoticed. Update the test around eligibleCountForTest, advanceForTest, currentTypeForTest, and s.layer.loop so it verifies rendering for each hosted effect (or each expected host transition) instead of a single aggregate non-zero count. Use per-iteration assertions or track which hosts rendered at least once, and fail if any expected hosted effect never produces pixels.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules/light/effects/effects.md`:
- Line 7: Update the effect preview prose in effects.md to use present-tense
source attribution only. In the affected preview image comments, keep the
WLED-Utils credit and clearly state that the gif is the current preview source,
but remove forward-looking language like “as an interim illustration,” “are
replaced,” and “once the effect is bench-verified.” Apply the same wording
pattern consistently across the referenced effect entries so the descriptions
read as current-state provenance, not planned future changes.
In `@docs/moonmodules/light/layouts/layouts.md`:
- Line 30: The heading text uses an unhyphenated compound modifier, so update
the “Human Sized Cube” title in the layouts documentation to “Human-Sized Cube”
to match standard compound-adjective usage. Locate the heading text itself and
change only the wording; no other surrounding content needs to be modified.
In `@docs/moonmodules/light/moonlive/MoonLiveEffect.md`:
- Line 17: Update the MoonLiveEffect documentation’s `source` description so it
matches actual behavior: the current example using `random16(65535)` is not
reliably selecting a valid live light on typical grids, so the effect is often
dark. In the `MoonLiveEffect.md` text, either change the example/wording to
describe a best-effort random write that may be dropped by bounds checks, or
adjust the example to use the live light range explicitly so it truly describes
one random visible light per tick.
In `@src/light/draw.h`:
- Around line 229-241: draw::text currently returns the width accumulated in cx,
which becomes the last rendered line’s width after any newline instead of the
documented first-line width. Update text(Buffer&, Coord3D, const fonts::Font&,
const char*, lengthType, lengthType, RGB) to track the first line’s end position
separately from the multiline cursor state, and return that first-line width
while keeping cx/cy resets for rendering subsequent lines.
In `@src/light/effects/FixedRectangleEffect.h`:
- Around line 88-98: The RGB write in the FixedRectangleEffect tile path is
incorrectly skipped when anyColor is false, which prevents white tiles from
being drawn and can leave stale RGB data behind. Update the tile rendering logic
in the effect’s pixel loop so the draw::pixel call always runs for every tile,
using the white-tile color when isWhiteTile is true, and keep the RGBW-specific
white-channel handling under the cpl >= 4 check only.
In `@src/light/effects/FreqSawsEffect.h`:
- Around line 69-71: Seed the effect’s timing state during
FreqSawsEffect::onBuildState so rebuilds start from a valid baseline instead of
uptime zero. After clearState() and before MoonModule::onBuildState(),
initialize lastTime to the current time source used by the effect so BandPhases
computes a near-zero delta on the first update after rebuild. Keep the fix
localized to FreqSawsEffect and preserve the existing rebuild flow.
In `@src/light/effects/PaintBrushEffect.h`:
- Around line 70-72: The PaintBrushEffect frame handling still dereferences the
result of AudioModule::latestFrame() without checking it first. In
PaintBrushEffect::update (right after the f assignment and before any access to
f->bands[0] or f->bands[bin]), add the same null guard pattern used by
BlurzEffect so the method returns early when no audio frame is available.
In `@src/light/fonts.h`:
- Line 16: Add compile-time size checks for the font data referenced by
Font::rows so each blob is validated against the documented 95*height layout
before build time. Update the font blob declarations in fonts.h and the related
font tables to enforce that their row arrays have exactly 95 * height bytes,
using compile-time assertions tied to the height field for each Font instance.
This should be applied to all font blobs in the file, including the additional
ones called out by the review, so any missing or extra row fails compilation
instead of causing an out-of-bounds read in the text renderer.
In `@src/light/layouts/CubeLayout.h`:
- Around line 68-74: The CubeLayout::lightCount() implementation is still
computing volume with 32-bit multiplication after only casting width, so
negative restored dimensions and large direct-set values can overflow before the
clamp. Update the calculation to use non-negative 64-bit factors for width,
height, and depth, then clamp the 64-bit product to nrOfLightsType::max() before
casting back, keeping the fix localized to CubeLayout::lightCount().
In `@src/light/layouts/PanelsLayout.h`:
- Around line 72-80: Clamp the signed members used by PanelsLayout::lightCount()
before multiplying: horizontalPanels, verticalPanels, panelWidth, and
panelHeight should be validated at the start of lightCount() so any negative
persisted/API values are treated as zero instead of being cast to large unsigned
values. Update the panelCount/perPanel calculation in lightCount() to use the
clamped values, keeping the saturation logic intact, so the result matches
forEachCoord() behavior when dimensions are invalid.
In `@src/light/layouts/SpiralLayout.h`:
- Around line 61-75: Guard the angular step calculation in SpiralLayout so a
zero bottomRadius cannot produce a zero ledsPerRound and a divide-by-zero in the
radians computation. In the SpiralLayout loop, clamp the effective bottom radius
to a non-negative value and add a degenerate-case path for bottomRadius == 0
that assigns stable coordinates without using the angular formula. Keep the fix
localized around bottomCircumference, ledsPerRound, and the radians calculation.
In `@src/light/layouts/TubesLayout.h`:
- Around line 28-38: Rewrite the comments in TubesLayout::onBuildControls() to
describe the current control contract only: update the reversed field comment so
it states the boolean meaning directly without referring to “odd”, and replace
the reconstruction/upstream-metadata narration with brief range descriptions for
nrOfTubes, ledsPerTube, and tubeDistance. Keep the same controls and ranges, but
make the comments factual and present-tense, using the symbols reversed,
nrOfTubes, ledsPerTube, tubeDistance, and onBuildControls() as anchors.
In `@src/light/modifiers/BlockModifier.h`:
- Around line 40-59: The logical ring sizing in BlockModifier is using the box
extents as if they were coordinates, which inflates the computed strip and
breaks edge cases like 0×0×0. Update the distance math in the BlockModifier
sizing path to derive from the last valid coordinate (size minus one / inclusive
max index) before applying the ring formula, then keep the final axis growth
step so the result stays consistent with MoonLight’s modifyPosition behavior.
Use the existing modifierSize_ and size handling in this block to ensure 1×1
stays unit-sized, larger boxes do not gain a phantom outer ring, and zero-sized
inputs remain empty.
In `@src/light/modifiers/CircleModifier.h`:
- Around line 44-56: The logical radius calculation in
CircleModifier::modifyLogical is using the box size itself as the input to
fold(), which makes the far-cell distance one too large and turns empty boxes
into 1×1×1 after the increments. Update the logic to derive the folded corner
from the far cell position, not the raw size value, and keep the post-fold
growth consistent so the transform matches the intended logical radius for all
grid sizes, including 0×0×0.
In `@src/light/modifiers/MirrorModifier.h`:
- Around line 14-17: The reflection comment in MirrorModifier.h is factually
wrong for odd widths and should be corrected, not removed. Update the
explanation near the mirrored extent logic to match the actual mapping
implemented by the mirror calculation in MirrorModifier, making clear that
`half*2 - 1 - pos` sends the far edge to logical 0 only for even extents, while
odd extents map the far edge to 1 (for example, size 5: 4 -> 1). Keep the
comment aligned with the mirror behavior and the `half` / `pos` reflection
formula.
In `@src/light/modifiers/RippleXZModifier.h`:
- Around line 9-12: Update the axis description in RippleXZModifier’s
documentation to match the new 1D convention after the Y-axis change. In the
comment near the shrink/towardsX and towardsZ explanation, revise the wording so
collapsing X is described as an effect that varies along Y and is broadcast
across X, instead of saying it runs along X. Keep the existing axis-collapse
explanation consistent with the RippleXZModifier behavior and terminology.
In `@test/unit/light/unit_TextEffect.cpp`:
- Around line 75-85: Add the missing zero-sized grid coverage to
TextEffect:scroll animates and is safe at any size so the test matches its
comment and spec. Extend the existing TEST_CASE in unit_TextEffect.cpp to
construct a 0×0 Scene, enable scroll on Scene::text, populate text as needed,
then call Scene::layer.onBuildState() and Scene::layer.loop() to verify it
remains a safe no-op without crashing. Keep the existing 16×8 and 1×1 checks
intact and ensure the new 0×0 case is explicitly exercised in the same test.
---
Outside diff comments:
In `@docs/backlog/moonlight-improvements.md`:
- Around line 1-33: This shipped decision log should not remain under the
backlog area now that the behavior is approved and durable. Move the content
from moonlight-improvements.md out of docs/backlog/ into the appropriate
permanent documentation location, and remove the backlog copy so docs/backlog/
stays reserved for temporary drafts only.
In `@docs/tests/scenario-tests.md`:
- Around line 575-635: The pc-macos FPS values in the three measure scenario
tables are out of sync with the stored tick_us ranges from
scenario_perf_full.json. Update the `measure-lcd`, `measure-parlio`, and
`measure-light-16` entries in this doc so the documented pc-macos FPS range
matches the current tick data, and verify the `Performance` tables for those
scenarios reflect the correct computed FPS values.
In `@docs/tests/unit-tests.md`:
- Around line 933-944: Update the draw test index entry to include the newly
added regression coverage for `draw::line(..., shorten)` rounding and
`draw::glyph()` orientation, since `test/unit/light/unit_draw.cpp` now pins both
behaviors. Keep the existing pixel/line/blur bullets, and add brief notes for
the `shorten` line-case and glyph orientation checks so the documentation
matches the current `unit_draw.cpp` coverage.
In `@scripts/moondeck.py`:
- Around line 1815-1828: The standalone raw image handling in moondeck.py drops
authored alt text when rebuilding an <img> line. Update the raw <img> path near
the img_m parsing logic to capture and preserve the optional alt attribute the
same way _render_cell() does for table-cell images, and include it when
appending the reconstructed tag along with src and width.
In `@src/light/draw.h`:
- Around line 199-207: Guard draw::fill() against zero-channel buffers by
checking the result of Buffer::channelsPerLight() before entering the write
loop. In fill(Buffer& buf, RGB c), return early or otherwise skip iteration when
cpl is 0 so the off += cpl loop cannot stall, and keep the existing per-channel
writes unchanged for valid channel counts.
---
Duplicate comments:
In `@src/light/effects/SolidEffect.h`:
- Around line 92-93: The white channel in SolidEffect is still written unscaled
in mode 0, so `brightness` does not affect RGBW output consistently. Update the
`SolidEffect` write path that handles `white` (the `writeWhite` call guarded by
`cpl >= 4`) to apply the same `brightness` scaling used for the RGB channels
before writing, so a zero-brightness frame clears W as well. Use the existing
`white`/`brightness` handling in `SolidEffect` as the source of truth and keep
the stale-W clearing behavior intact.
In `@test/unit/light/unit_DemoReelEffect.cpp`:
- Around line 63-74: The current test in unit_DemoReelEffect only asserts that
at least one hosted effect rendered, so a dead host can still pass unnoticed.
Update the test around eligibleCountForTest, advanceForTest, currentTypeForTest,
and s.layer.loop so it verifies rendering for each hosted effect (or each
expected host transition) instead of a single aggregate non-zero count. Use
per-iteration assertions or track which hosts rendered at least once, and fail
if any expected hosted effect never produces pixels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b80dc59f-c4ab-4a2c-84c1-ac0c7a5bff8c
⛔ Files ignored due to path filters (24)
docs/assets/light/effects/StarSkyEffect.gifis excluded by!**/*.gifdocs/assets/light/layouts/CarLightsLayout.gifis excluded by!**/*.gifdocs/assets/light/layouts/TorontoBarGourdsLayout.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/drivers/Art-Net-In.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/drivers/ArtNetInControls.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/drivers/ArtNetOutControls.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/drivers/FastLED-Audio.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/drivers/FastLED-Driver.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/drivers/IMUDriverControls.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/drivers/IRDriver.jpegis excluded by!**/*.jpegdocs/backlog/moonlight_images/moonlight/drivers/MPU-6050.jpgis excluded by!**/*.jpgdocs/backlog/moonlight_images/moonlight/drivers/irdrivercontrols.pngis excluded by!**/*.pngdocs/backlog/moonlight_images/moonlight/effects/E_Ball2D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Cosmic3D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Geq2D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Hello1D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Noise2D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Noise3D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Orbit2D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Oscillate2D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Random1D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Sweep2D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/E_Vu1D.gifis excluded by!**/*.gifdocs/backlog/moonlight_images/moonlight/effects/layers.gifis excluded by!**/*.gif
📒 Files selected for processing (60)
docs/backlog/backlog-light.mddocs/backlog/moonlight-improvements.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/modifiers/modifiers.mddocs/moonmodules/light/moonlive/MoonLiveEffect.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/moondeck.pysrc/core/math8.hsrc/light/draw.hsrc/light/effects/BlurzEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/FixedRectangleEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/GameOfLifeEffect.hsrc/light/effects/LissajousEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PaintBrushEffect.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/fonts.hsrc/light/layouts/CarLightsLayout.hsrc/light/layouts/CubeLayout.hsrc/light/layouts/HumanSizedCubeLayout.hsrc/light/layouts/PanelLayout.hsrc/light/layouts/PanelsLayout.hsrc/light/layouts/RingLayout.hsrc/light/layouts/Rings241Layout.hsrc/light/layouts/SingleColumnLayout.hsrc/light/layouts/SingleRowLayout.hsrc/light/layouts/SpiralLayout.hsrc/light/layouts/TorontoBarGourdsLayout.hsrc/light/layouts/TubesLayout.hsrc/light/modifiers/BlockModifier.hsrc/light/modifiers/CircleModifier.hsrc/light/modifiers/MirrorModifier.hsrc/light/modifiers/PinwheelModifier.hsrc/light/modifiers/RippleXZModifier.hsrc/light/modifiers/TransposeModifier.hsrc/light/moonlive/MoonLiveEffect.hsrc/main.cpptest/CMakeLists.txttest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_MoonLiveEffect_livescript.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_math8.cpptest/unit/light/unit_DemoReelEffect.cpptest/unit/light/unit_TextEffect.cpptest/unit/light/unit_draw.cpp
Comment on lines
+40
to
+59
| // RECONSTRUCTED: MoonLight calls `modifyPosition(layer->size)` here to fold | ||
| // the box itself into block-distance space, then does `size.x++/y++/z++`. | ||
| // We inline that same computation over `size` (our modifyLogical is const and | ||
| // reads the stash we are still writing, so we can't call it here). The math is | ||
| // identical: the box becomes {0, max(|dx|,|dy|), 0}, then each axis grows by 1. | ||
| const int centerX = (modifierSize_.x + 1) / 2 - 1; | ||
| const int centerY = (modifierSize_.y + 1) / 2 - 1; | ||
| const int dx = std::abs(static_cast<int>(size.x) - centerX); | ||
| const int dy = std::abs(static_cast<int>(size.y) - centerY); | ||
| const int distance = std::max(dx, dy); | ||
|
|
||
| size.x = 0; | ||
| size.y = static_cast<lengthType>(distance); | ||
| size.z = 0; | ||
|
|
||
| // Grow each axis by one so the top ring index is inclusive (MoonLight's | ||
| // size.x++/y++/z++). Result: {1, distance + 1, 1}. | ||
| size.x++; | ||
| size.y++; | ||
| size.z++; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Size the logical ring strip from the last valid coordinate, not the box extents.
size is a count box here, but Lines 45-49 treat it like a coordinate. That adds a phantom outer ring: a 4×4 box becomes height 4 instead of 3, 1×1 becomes 2, and 0×0 turns into a non-empty logical box.
Suggested fix
void modifyLogicalSize(Coord3D& size) override {
// Stash the incoming (physical) box so the const per-light fold can read the
// centre from it. This is MoonLight's `modifierSize = layer->size`.
modifierSize_ = size;
+
+ if (size.x <= 0 || size.y <= 0) {
+ size = {0, 0, 0};
+ return;
+ }
// RECONSTRUCTED: MoonLight calls `modifyPosition(layer->size)` here to fold
// the box itself into block-distance space, then does `size.x++/y++/z++`.
// We inline that same computation over `size` (our modifyLogical is const and
// reads the stash we are still writing, so we can't call it here). The math is
// identical: the box becomes {0, max(|dx|,|dy|), 0}, then each axis grows by 1.
const int centerX = (modifierSize_.x + 1) / 2 - 1;
const int centerY = (modifierSize_.y + 1) / 2 - 1;
- const int dx = std::abs(static_cast<int>(size.x) - centerX);
- const int dy = std::abs(static_cast<int>(size.y) - centerY);
+ const int dx = std::abs((static_cast<int>(size.x) - 1) - centerX);
+ const int dy = std::abs((static_cast<int>(size.y) - 1) - centerY);
const int distance = std::max(dx, dy);
- size.x = 0;
- size.y = static_cast<lengthType>(distance);
- size.z = 0;
-
- // Grow each axis by one so the top ring index is inclusive (MoonLight's
- // size.x++/y++/z++). Result: {1, distance + 1, 1}.
- size.x++;
- size.y++;
- size.z++;
+ size.x = 1;
+ size.y = static_cast<lengthType>(distance + 1);
+ size.z = 1;
}As per coding guidelines, “Effects must run correctly at every grid size and tick rate, including 0×0×0, and must not truncate animation math to zero on fast devices.”
📝 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
| // RECONSTRUCTED: MoonLight calls `modifyPosition(layer->size)` here to fold | |
| // the box itself into block-distance space, then does `size.x++/y++/z++`. | |
| // We inline that same computation over `size` (our modifyLogical is const and | |
| // reads the stash we are still writing, so we can't call it here). The math is | |
| // identical: the box becomes {0, max(|dx|,|dy|), 0}, then each axis grows by 1. | |
| const int centerX = (modifierSize_.x + 1) / 2 - 1; | |
| const int centerY = (modifierSize_.y + 1) / 2 - 1; | |
| const int dx = std::abs(static_cast<int>(size.x) - centerX); | |
| const int dy = std::abs(static_cast<int>(size.y) - centerY); | |
| const int distance = std::max(dx, dy); | |
| size.x = 0; | |
| size.y = static_cast<lengthType>(distance); | |
| size.z = 0; | |
| // Grow each axis by one so the top ring index is inclusive (MoonLight's | |
| // size.x++/y++/z++). Result: {1, distance + 1, 1}. | |
| size.x++; | |
| size.y++; | |
| size.z++; | |
| // RECONSTRUCTED: MoonLight calls `modifyPosition(layer->size)` here to fold | |
| // the box itself into block-distance space, then does `size.x++/y++/z++`. | |
| // We inline that same computation over `size` (our modifyLogical is const and | |
| // reads the stash we are still writing, so we can't call it here). The math is | |
| // identical: the box becomes {0, max(|dx|,|dy|), 0}, then each axis grows by 1. | |
| const int centerX = (modifierSize_.x + 1) / 2 - 1; | |
| const int centerY = (modifierSize_.y + 1) / 2 - 1; | |
| if (size.x <= 0 || size.y <= 0) { | |
| size = {0, 0, 0}; | |
| return; | |
| } | |
| const int dx = std::abs((static_cast<int>(size.x) - 1) - centerX); | |
| const int dy = std::abs((static_cast<int>(size.y) - 1) - centerY); | |
| const int distance = std::max(dx, dy); | |
| size.x = 1; | |
| size.y = static_cast<lengthType>(distance + 1); | |
| size.z = 1; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/modifiers/BlockModifier.h` around lines 40 - 59, The logical ring
sizing in BlockModifier is using the box extents as if they were coordinates,
which inflates the computed strip and breaks edge cases like 0×0×0. Update the
distance math in the BlockModifier sizing path to derive from the last valid
coordinate (size minus one / inclusive max index) before applying the ring
formula, then keep the final axis growth step so the result stays consistent
with MoonLight’s modifyPosition behavior. Use the existing modifierSize_ and
size handling in this block to ensure 1×1 stays unit-sized, larger boxes do not
gain a phantom outer ring, and zero-sized inputs remain empty.
Source: Coding guidelines
Comment on lines
+44
to
+56
| // Reshape the logical box exactly as MoonLight does: run the box's own far | ||
| // corner through the SAME distance fold (yielding (0, cornerDistance, 0)), | ||
| // then grow every axis by one. modifyLogical is const, so fold a local copy | ||
| // and copy the result back into `size` — same net transform as MoonLight | ||
| // calling modifyPosition(layer->size). // RECONSTRUCTED (fold-order equivalence) | ||
| Coord3D corner = size; | ||
| fold(corner); | ||
| size = corner; | ||
|
|
||
| // Change the size to be one bigger in each dimension. | ||
| size.x++; | ||
| size.y++; | ||
| size.z++; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compute the logical radius from the far cell, not from the box size.
corner = size feeds the element count into fold(), so the logical radius is one cell too large on every non-empty box, and an empty box becomes 1×1×1 after the final increments.
Suggested fix
void modifyLogicalSize(Coord3D& size) override {
// Stash the incoming box so the per-light fold (const, no box arg) can read
// the centre from it — the MoonLight `modifierSize` pattern.
modifierSize_ = size;
+
+ if (size.x <= 0 || size.y <= 0) {
+ size = {0, 0, 0};
+ return;
+ }
// Reshape the logical box exactly as MoonLight does: run the box's own far
// corner through the SAME distance fold (yielding (0, cornerDistance, 0)),
// then grow every axis by one. modifyLogical is const, so fold a local copy
// and copy the result back into `size` — same net transform as MoonLight
// calling modifyPosition(layer->size). // RECONSTRUCTED (fold-order equivalence)
- Coord3D corner = size;
+ Coord3D corner{
+ static_cast<lengthType>(size.x - 1),
+ static_cast<lengthType>(size.y - 1),
+ static_cast<lengthType>(size.z > 0 ? size.z - 1 : 0),
+ };
fold(corner);
size = corner;
// Change the size to be one bigger in each dimension.
size.x++;As per coding guidelines, “Effects must run correctly at every grid size and tick rate, including 0×0×0, and must not truncate animation math to zero on fast devices.”
🤖 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/modifiers/CircleModifier.h` around lines 44 - 56, The logical
radius calculation in CircleModifier::modifyLogical is using the box size itself
as the input to fold(), which makes the far-cell distance one too large and
turns empty boxes into 1×1×1 after the increments. Update the logic to derive
the folded corner from the far cell position, not the raw size value, and keep
the post-fold growth consistent so the transform matches the intended logical
radius for all grid sizes, including 0×0×0.
Source: Coding guidelines
Comment on lines
+9
to
+12
| // physical box. With `shrink` on, `towardsX` flattens the box's X extent to 1 and | ||
| // folds every physical X onto x=0 (a 1D effect running along X is painted once and | ||
| // broadcast across all X); `towardsZ` does the same for the Z extent (each XY plane | ||
| // shared across depth). It is the static "collapse an axis" fold: the logical box |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the axis description to the new 1D convention.
After this PR’s Y-axis convention change, collapsing X leaves the effect varying along Y and broadcasting across X. The current wording still describes the old X-running 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 `@src/light/modifiers/RippleXZModifier.h` around lines 9 - 12, Update the axis
description in RippleXZModifier’s documentation to match the new 1D convention
after the Y-axis change. In the comment near the shrink/towardsX and towardsZ
explanation, revise the wording so collapsing X is described as an effect that
varies along Y and is broadcast across X, instead of saying it runs along X.
Keep the existing axis-collapse explanation consistent with the RippleXZModifier
behavior and terminology.
Adopt the FastLED/WLED/MoonLight buffer-persistence model — the Layer no longer clears each frame, so trail and scroll effects finally work — and add a MoonLight-style collected per-frame fade. Fixes a batch of effect regressions found on the P4/S3 test panels (FreqMatrix/NoiseMeter expansion, Blurz visibility, Fire/LavaLamp palette, Plasma/Rainbow/DistortionWaves timing) plus a two-mic dangling-pointer bug and a DemoReel crash on non-EffectBase effect-role types. Also processes a CodeRabbit review batch.
KPI: 16384lights | PC:465KB | tick:364/237/1/341/45/6/800/180/51/58/487/401/49/3/1/92us(FPS:2747/4219/1000000/2932/22222/166666/1250/5555/19607/17241/2053/2493/20408/333333/1000000/10869) | ESP32:1339KB | tick:27585us(FPS:36) | heap:31521KB | src:166(30902) | test:90(13339) | lizard:113w
Core:
- Layer: removed the per-frame buffer_.clear() — the buffer now persists frame-to-frame (FastLED/WLED/MoonLight model), so fade-trail, scroll, and read-prior (Game-of-Life) effects keep their history; one clear on onBuildState() means a freshly (re)built effect still starts black.
- Layer: added fadeToBlackBy(amt) — effects register a fade, the Layer collects the MIN across them (gentlest fade / longest trail wins) and applies ONE buffer pass per frame (MoonLight's VirtualLayer model) instead of each effect fading the shared buffer itself.
- AudioModule: fixed the two-mic active_ dangling pointer — first live mic wins the seat, teardown vacates it, and any running module re-elects itself in loop(), so removing the active mic while a second one runs no longer drops every audio effect to silence.
- AudioModule: added `simulate` (music/sweep, on-silence/always) — synthesizes an AudioFrame so audio effects light up on a mic-less board and audio behaviour is testable deterministically; feeds the same 1 s peak-hold as the mic path so the level display tracks it.
- AudioFrame: added levelSmoothed (EMA of raw level) — WLED's volume/volumeSmth pair; effects read the one matching their intent.
Light domain:
- DemoReel: extrudes its hosted D1/D2 child (via ModuleFactory::typeDim, RTTI-free) so a hosted NoiseMeter/FreqMatrix fans across the grid; fixed a SIGBUS from downcasting a non-EffectBase effect-role type (the test EffectStub) — the factory-probed dim is used instead of a per-frame static_cast.
- NoiseMeter: D3→D1 (writes the x=0 column, extrude fans x and z) — dropped the hand-rolled per-row X/Z broadcast the framework already does.
- AudioSpectrum: D3→D2 (writes the z=0 slice) — dropped the redundant per-z broadcast; keeps its genuine 2D X gradient.
- FreqMatrix / FreqSaws / GEQ / GEQ3D / Blurz / NoiseMeter / StarField / StarSky / SphereMove / Lissajous / BouncingBalls / PaintBrush / Random / FixedRectangle: switched draw::fade(buf,amt) → layer()->fadeToBlackBy(amt) (the collected once-per-frame fade).
- Blurz: dot radius now scales with the fixture (a proportional glowing blob at any grid size), re-stamped on top of the blur (WLED addRGB) so the core stays bright, and fadeRate corrected 16→48 to match MoonLight (the low value muddied it into a faint wash).
- Fire: renders through the active palette (heat = palette index) instead of a hardcoded ramp; a fully cold cell stays black so the "sky" above the flame isn't tinted by a non-Lava palette's index-0 colour. Dropped the hue_shift control.
- LavaLamp: renders through the active palette; deleted its hardcoded 768-byte palette table.
- Plasma: dropped the `* width` phase term that made it scroll width-times too fast on a wide panel — speed is now grid-size-independent.
- Rainbow: default speed 60→20 BPM (a whole rainbow per second read too fast).
- Praxis: added a `speed` control scaling the hue scroll (default calmer) — leaves MoonLight's mutator defaults intact.
- DistortionWaves: derive ty from the raw phase accumulator, not the wrapped uint8 t, so it no longer jumps ~76 every ~1 s when t wrapped.
- RubiksCube: drawCube self-clears (writes only surface voxels; without a wipe old poses smeared as the cube turns).
- SolidEffect: scale the W channel by brightness like RGB in colour mode 0.
- FixedRectangle: default box 1×1 → 15×15×15 at origin (visible on a 16K-light panel; clamps to the grid on small fixtures); always writes RGB so white tiles render.
- PaintBrush: null-guard latestFrame() before dereferencing bands[].
- MoonLive: default script index random16(65535)→random16(256) (65535 was off-grid ~99.6% of ticks → near-black).
UI:
- draw.h: added draw::addPixel (saturating add — WLED addRGB); fixed draw::fill infinite-loop guard on cpl==0 and draw::text returning the first-line width; glyph column read stays MSB-first.
- fonts.h: compile-time static_asserts pin each font blob at 95 glyphs × height.
Scripts / MoonDeck:
- moondeck.py: standalone <img> render preserves the alt attribute.
Tests:
- unit_Layer_persistence (new): pins buffer persistence, the MIN-collect fade, once-per-frame decay, and reset-after-consume.
- unit_AudioModule: two-mic first-wins + survivor re-election regression.
- unit_TextEffect: added a 0×0 scroll case; unit_RainbowEffect: assert a lit palette colour (not channel==255, which was a stale hsvToRgb assumption); regenerated test docs (draw shorten/glyph bullets).
Docs / CI:
- architecture.md: new "Buffer persistence" section (states our model as fact, industry note as a parenthetical); decisions.md: rewrote the clear-every-frame lesson to the persistence model; AudioModule.md (simulate), effects.md (Fire palette hint), MoonLiveEffect.md, layouts.md ("Human-Sized Cube"), TubesLayout/MirrorModifier present-tense + odd-extent comment fixes.
- Skipped (CodeRabbit, verified not applicable): Cube/Panels/Spiral layout overflow/div-zero (control-bounded ≥1), FreqSaws lastTime (state zeroed), RippleXZ comment (matches code), DemoReel test (already per-host), scenario-tests.md (current). moonlight-improvements.md move deferred (PO call).
Reviews:
- 🐇 draw::fill cpl==0 guard, draw::text first-line width, FixedRectangle white tiles, PaintBrush null-guard, SolidEffect W×brightness, fonts.h static_asserts, MoonLive default index, moondeck.py alt, unit_TextEffect 0×0 — fixed.
- 🐇 Cube/Panels/Spiral/FreqSaws/RippleXZ/DemoReel-test/scenario-doc findings — verified not valid against current code, skipped with reason.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/light/draw.h (1)
146-149: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the blur-axis comment to match the new 1D convention.
On a
1×N×1layer the x-pass is a no-op and the y-pass does the work, so “a 1D layer blurs along x” is no longer accurate. As per coding guidelines,**/*.{h,hpp,hh,cpp,cxx,cc,md}: “Do not remove comments unless they are outdated or factually wrong.”🤖 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 146 - 149, The blur-axis comment in draw::draw.h is outdated for the new 1D convention: a 1×N×1 layer should be described as blurring on the y-pass, not “along x.” Update the comment near the box blur/separable seep description to reflect the current axis behavior for 1D, while keeping the rest of the 1D→3D explanation consistent with the draw::line-style primitive wording.Source: Coding guidelines
docs/architecture.md (1)
389-395: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSeparate the D1 effect contract from the layout contract.
Saying every one-dimensional physical output is
1 × N, “notN × 1,” conflicts with the documented layout vocabulary that includes both Single Column and Single Row. The D1 rule here is about how lower-dimensional effects are extruded, not a blanket ban on row-oriented 1D layouts. As per path instructions,docs/moonmodules/light/layouts/layouts.md: “Single Column/Single Row define 1D along Y/X respectively, with Z treated as 0.”🤖 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 389 - 395, Separate the D1 extrusion rule from the physical layout definition in the architecture doc: the current wording in the D1/D2 section and “Why 1D runs along Y” conflates effect expansion with layout orientation. Update the text around the D1 contract to state only that lower-dimensional effects expand across the next axis, and move any 1D layout orientation language to the layouts documentation so it aligns with the Single Column/Single Row definitions in layouts.md. Preserve the existing symbols and terminology used in the D1/D2 and expand-rule explanation, but remove the blanket statement that all 1D outputs are only 1 × N.Source: Path instructions
src/light/effects/SolidEffect.h (1)
123-126: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winReplace
sqrtfwith an integer square root in the render loop.This branch runs inside
loop()and does three floating-point square roots per frame. Use integer RMS math here. As per coding guidelines, “In the render loop and anything it calls, avoid heap allocations, blocking calls, mutex blocking, andstd::string; prefer integer math and contiguous buffers.”Proposed direction
- avg.r = static_cast<uint8_t>(sqrtf(static_cast<float>(sumR) / n)); - avg.g = static_cast<uint8_t>(sqrtf(static_cast<float>(sumG) / n)); - avg.b = static_cast<uint8_t>(sqrtf(static_cast<float>(sumB) / n)); + avg.r = static_cast<uint8_t>(isqrt(sumR / static_cast<uint32_t>(n))); + avg.g = static_cast<uint8_t>(isqrt(sumG / static_cast<uint32_t>(n))); + avg.b = static_cast<uint8_t>(isqrt(sumB / static_cast<uint32_t>(n)));Add a small integer helper near
mapI():static uint16_t isqrt(uint32_t v) { uint32_t bit = 1u << 30; while (bit > v) bit >>= 2; uint32_t res = 0; while (bit) { if (v >= res + bit) { v -= res + bit; res = (res >> 1) + bit; } else { res >>= 1; } bit >>= 2; } return static_cast<uint16_t>(res); }🤖 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 123 - 126, The averaging path in SolidEffect.h still uses three floating-point sqrtf calls inside the render loop, which should be replaced with integer math. Update the SolidEffect::render/loop branch that computes avg.r, avg.g, and avg.b to use a small integer square-root helper (for example, a new isqrt near mapI()) and compute the RMS values with integer arithmetic instead of float conversion. Keep the change localized to the SolidEffect code path and preserve the existing behavior for the n > 0 case.Source: Coding guidelines
src/light/effects/StarFieldEffect.h (1)
111-128: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftUse fixed-point star projection in the hot loop.
The per-star path does floating-point divide/multiply and float depth updates each rendered frame. Store star coordinates/depth as fixed-point integers and project with integer division. As per coding guidelines, “In the render loop and anything it calls, avoid heap allocations, blocking calls, mutex blocking, and
std::string; prefer integer math and contiguous buffers.”Also applies to: 139-140
🤖 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/StarFieldEffect.h` around lines 111 - 128, The star rendering hot path in StarFieldEffect should avoid floating-point projection and depth updates; use fixed-point integer storage and integer division in the per-star loop instead of the current float-based math around the star projection and brightness calculation. Update the logic in the StarFieldEffect render/update path that computes sx, sy, and depth-dependent brightness so it stays entirely integer-based, keeping the code in the hot loop allocation-free and math-friendly as required.Source: Coding guidelines
src/light/effects/DemoReelEffect.h (1)
143-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the current child until the replacement is live.
swapTo()tears downcurrent_beforeModuleFactory::create()and child init can fail. When that happens duringadvance(), the same tick still reachescurrent_->loop()and crashes on a null child. Only commitcursor_/current_after the new module is successfully created and initialized.Suggested fix
void swapTo(uint8_t which) { - destroyCurrent(); if (which >= eligibleCount_) return; - cursor_ = which; const char* typeName = ModuleFactory::typeName(eligible_[which]); MoonModule* mod = ModuleFactory::create(typeName); if (!mod) return; @@ mod->setup(); mod->onBuildState(); + destroyCurrent(); + cursor_ = which; current_ = mod;Also applies to: 154-159
🤖 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/DemoReelEffect.h` around lines 143 - 149, The swapTo() flow in DemoReelEffect tears down the existing child before ModuleFactory::create() and initialization succeed, leaving current_ null if creation fails and causing advance() to crash on the same tick. Update swapTo() so the old child is kept alive until the new MoonModule is successfully created and initialized, then atomically commit cursor_ and current_ only after success. Use swapTo(), current_, cursor_, and ModuleFactory::create() as the key touchpoints, and apply the same lifetime/commit ordering fix anywhere else in the effect that performs the replacement path.test/unit/light/unit_RainbowEffect.cpp (1)
40-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPin the active palette before asserting
(0,0)is non-black.
Palettes::active()is global mutable state. This test currently assumes palette index0is lit, but at the initial phase(0,0)maps to index0, so an earlier test that leaves a black-at-zero palette makes this order-dependent. Set a known palette here beforelayer.loop()or assert a property that does not depend on the shared palette.Proposed fix
mm::RainbowEffect rainbow; layer.addChild(&rainbow); layer.onBuildState(); + mm::Palettes::setActive(0); // Rainbow: hue 0 is known non-black layer.loop();🤖 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_RainbowEffect.cpp` around lines 40 - 65, The RainbowEffect test is relying on shared global palette state via Palettes::active(), which makes the `(0,0)` non-black assertion order-dependent. In `TEST_CASE("RainbowEffect pixel 0,0 produces valid RGB")`, pin a known lit palette before calling `layer.loop()` (or otherwise isolate the test from global palette mutations) so `RainbowEffect` always runs against a predictable palette and the `data[0..2]` check is stable.test/scenarios/light/scenario_Layer_base_pipeline.json (1)
75-80: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRenegotiate the
pc-macostiming contract here.
test/scenario_runner.cpptreatstick_usas a ceiling withmax(20%, 200us)slack onpc-*. With the contract still at50usand the new sample at243us, this step now passes by only7us. That is effectively a flaky desktop contract. Please raise the contract or add an explicittolerance_usthat reflects the current variance.Also applies to: 83-99
🤖 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_Layer_base_pipeline.json` around lines 75 - 80, The `pc-macos` timing contract in the scenario file is too tight for the current sample and is only barely passing under `test/scenario_runner.cpp`’s `tick_us` ceiling logic. Update the `contract` entry for `pc-macos` in `scenario_Layer_base_pipeline` by either raising `tick_us` to a value that matches the observed variance or adding an explicit `tolerance_us` that aligns with the desktop slack rules. Keep the change scoped to the `contract` block so the runner’s comparison against `tick_us` and any tolerance handling remains consistent.docs/tests/scenario-tests.md (1)
1019-1023: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the
scenario_Layers_compositionFPS row.
test/scenarios/light/scenario_Layers_composition.jsonnow recordspc-macos.tick_usas[52, 870], so this table's lower FPS bound should be about1,149, not1,361. Right now the doc and the scenario source disagree.Suggested fix
-| `pc-macos` | — / 1,361-19,231 | — / unlimited | — / unlimited | +| `pc-macos` | — / 1,149-19,231 | — / unlimited | — / unlimited |🤖 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/tests/scenario-tests.md` around lines 1019 - 1023, The FPS range for the `scenario_Layers_composition` documentation is stale and no longer matches the scenario data. Update the `pc-macos` FPS lower bound in the scenario tests table to reflect the current `tick_us` values from `scenario_Layers_composition.json`, and keep the documented range aligned with the source data. Use the existing `scenario_Layers_composition` entry in the scenario tests markdown to locate and adjust the row.scripts/moondeck.py (1)
1673-1678: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake raw
<img>parsing attribute-order agnostic. Both regexes requiresrc→width→alt(andstylein the table-cell path), so valid tags like<img src="..." alt="..." width="160">will render as escaped text. Matching the tag first and extracting attributes independently avoids that fragility.🤖 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 `@scripts/moondeck.py` around lines 1673 - 1678, The raw image handling in moondeck’s HTML sanitizing path is too strict about attribute order, causing valid <img> tags to be missed. Update the <img> parsing logic around the regexes that call _stash(_img(m)) so it matches the tag independently of attribute order and extracts src, width, alt, and style without assuming a fixed sequence. Make the change in both the general image handling and the table-cell image path so _img can render tags like <img src="..." alt="..." width="160"> correctly.
🤖 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 374-383: The persistence description is broader than the actual
Layer behavior, since onBuildState() clears the buffer during a rebuild instead
of preserving it across effect reconfiguration. Update the
Layer::loop()/Layer::fadeToBlackBy documentation in docs/architecture.md to
state that persistence holds between frames, but not across rebuilds/resizes,
and make the text consistent with the buffer reset behavior in Layer.h.
In `@docs/tests/unit-tests.md`:
- Around line 950-953: Rewrite the affected unit-test descriptions in
present-tense contract language within the docs/tests/unit-tests.md section,
especially the entries describing shorten, draw::blur, blur, and glyph
orientation. Replace history/absence phrasing like “previously untested” and
“regression here” with direct guarantees stated via the relevant symbols and
behaviors now covered by the tests, keeping the wording focused on current
expected behavior rather than past gaps or future prevention.
In `@src/core/AudioModule.h`:
- Around line 124-130: The active_ ownership in AudioModule is claimed too early
in setup(), before reinit() confirms the module can actually publish frames.
Update setup() so active_ is assigned only after successful initialization and
publish readiness, using the existing setup(), reinit(), inited_, and active_
flow. Also adjust the self-election logic in loop()/tick path so if this
instance currently owns active_ but is no longer inited_ and simulate == 0, it
clears active_ and allows another working AudioModule to claim the seat on its
next iteration.
In `@src/light/effects/BlurzEffect.h`:
- Around line 127-131: The radius calculation in BlurzEffect currently violates
the documented “≤32 grid stays one pixel” contract because the r assignment
using cols/rows divided by 32 yields 1 at exactly 32. Update the logic in
BlurzEffect so the edge case matches the comment, keeping r at 0 for any grid up
to 32 and only increasing it for larger sizes; use the existing r computation
site and surrounding blur/blob drawing code to preserve the intended visual
behavior.
- Around line 38-40: The comment in BlurzEffect.h should be rewritten in present
tense to describe the current default behavior rather than narrating the old 16
value or source drift. Update the note near the blur/fade defaults in the
BlurzEffect context so it states what fadeRate 48 and blur 127 do now, without
contrastive or historical phrasing, keeping the wording aligned with the coding
guideline.
In `@src/light/effects/PlasmaEffect.h`:
- Around line 40-44: The plasma phase update in PlasmaEffect is truncating
fractional progress to zero on fast loops, so the animation can stall. Change
the phase accumulation logic so PlasmaEffect keeps the full numerator across
frames instead of doing the division in the per-tick increment; use the existing
phase_ state as an accumulator in the phase update path and only convert to
byte-sized wave phases when sampling the effect. Make the fix in the
PlasmaEffect time-step math where phase_ is advanced, preserving sub-unit
progress before dividing by 60000.
In `@src/light/effects/PraxisEffect.h`:
- Line 76: The hue phase calculation in PraxisEffect is overflowing in 32-bit
before the division, causing palette jumps after long uptimes. Update the
hue-scroll math in the huebase computation to promote the multiplication to
64-bit by casting now before multiplying by speed, while keeping the rest of the
effect logic unchanged so the scroll remains monotonic.
In `@test/unit/light/unit_Layer_persistence.cpp`:
- Around line 58-127: Add a regression test covering the rebuild-clear path in
Layer::onBuildState(). Create a case that writes a pixel into the Layer buffer,
call onBuildState() again to simulate rebuild, and assert the buffer is cleared
to black afterward. Use the existing Scene and Layer setup patterns from the
persistence tests so the new check directly exercises buffer_.clear() and
protects the rebuild contract.
---
Outside diff comments:
In `@docs/architecture.md`:
- Around line 389-395: Separate the D1 extrusion rule from the physical layout
definition in the architecture doc: the current wording in the D1/D2 section and
“Why 1D runs along Y” conflates effect expansion with layout orientation. Update
the text around the D1 contract to state only that lower-dimensional effects
expand across the next axis, and move any 1D layout orientation language to the
layouts documentation so it aligns with the Single Column/Single Row definitions
in layouts.md. Preserve the existing symbols and terminology used in the D1/D2
and expand-rule explanation, but remove the blanket statement that all 1D
outputs are only 1 × N.
In `@docs/tests/scenario-tests.md`:
- Around line 1019-1023: The FPS range for the `scenario_Layers_composition`
documentation is stale and no longer matches the scenario data. Update the
`pc-macos` FPS lower bound in the scenario tests table to reflect the current
`tick_us` values from `scenario_Layers_composition.json`, and keep the
documented range aligned with the source data. Use the existing
`scenario_Layers_composition` entry in the scenario tests markdown to locate and
adjust the row.
In `@scripts/moondeck.py`:
- Around line 1673-1678: The raw image handling in moondeck’s HTML sanitizing
path is too strict about attribute order, causing valid <img> tags to be missed.
Update the <img> parsing logic around the regexes that call _stash(_img(m)) so
it matches the tag independently of attribute order and extracts src, width,
alt, and style without assuming a fixed sequence. Make the change in both the
general image handling and the table-cell image path so _img can render tags
like <img src="..." alt="..." width="160"> correctly.
In `@src/light/draw.h`:
- Around line 146-149: The blur-axis comment in draw::draw.h is outdated for the
new 1D convention: a 1×N×1 layer should be described as blurring on the y-pass,
not “along x.” Update the comment near the box blur/separable seep description
to reflect the current axis behavior for 1D, while keeping the rest of the 1D→3D
explanation consistent with the draw::line-style primitive wording.
In `@src/light/effects/DemoReelEffect.h`:
- Around line 143-149: The swapTo() flow in DemoReelEffect tears down the
existing child before ModuleFactory::create() and initialization succeed,
leaving current_ null if creation fails and causing advance() to crash on the
same tick. Update swapTo() so the old child is kept alive until the new
MoonModule is successfully created and initialized, then atomically commit
cursor_ and current_ only after success. Use swapTo(), current_, cursor_, and
ModuleFactory::create() as the key touchpoints, and apply the same
lifetime/commit ordering fix anywhere else in the effect that performs the
replacement path.
In `@src/light/effects/SolidEffect.h`:
- Around line 123-126: The averaging path in SolidEffect.h still uses three
floating-point sqrtf calls inside the render loop, which should be replaced with
integer math. Update the SolidEffect::render/loop branch that computes avg.r,
avg.g, and avg.b to use a small integer square-root helper (for example, a new
isqrt near mapI()) and compute the RMS values with integer arithmetic instead of
float conversion. Keep the change localized to the SolidEffect code path and
preserve the existing behavior for the n > 0 case.
In `@src/light/effects/StarFieldEffect.h`:
- Around line 111-128: The star rendering hot path in StarFieldEffect should
avoid floating-point projection and depth updates; use fixed-point integer
storage and integer division in the per-star loop instead of the current
float-based math around the star projection and brightness calculation. Update
the logic in the StarFieldEffect render/update path that computes sx, sy, and
depth-dependent brightness so it stays entirely integer-based, keeping the code
in the hot loop allocation-free and math-friendly as required.
In `@test/scenarios/light/scenario_Layer_base_pipeline.json`:
- Around line 75-80: The `pc-macos` timing contract in the scenario file is too
tight for the current sample and is only barely passing under
`test/scenario_runner.cpp`’s `tick_us` ceiling logic. Update the `contract`
entry for `pc-macos` in `scenario_Layer_base_pipeline` by either raising
`tick_us` to a value that matches the observed variance or adding an explicit
`tolerance_us` that aligns with the desktop slack rules. Keep the change scoped
to the `contract` block so the runner’s comparison against `tick_us` and any
tolerance handling remains consistent.
In `@test/unit/light/unit_RainbowEffect.cpp`:
- Around line 40-65: The RainbowEffect test is relying on shared global palette
state via Palettes::active(), which makes the `(0,0)` non-black assertion
order-dependent. In `TEST_CASE("RainbowEffect pixel 0,0 produces valid RGB")`,
pin a known lit palette before calling `layer.loop()` (or otherwise isolate the
test from global palette mutations) so `RainbowEffect` always runs against a
predictable palette and the `data[0..2]` check is stable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88ad8ff6-4274-4fdc-a8ce-2534980e6048
📒 Files selected for processing (55)
docs/architecture.mddocs/backlog/moonlight-fidelity-tensions.mddocs/backlog/moonlight-improvements.mddocs/history/decisions.mddocs/moonmodules/core/AudioModule.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/moonlive/MoonLiveEffect.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/moondeck.pysrc/core/AudioFrame.hsrc/core/AudioModule.hsrc/light/draw.hsrc/light/effects/AudioSpectrumEffect.hsrc/light/effects/AudioVolumeEffect.hsrc/light/effects/BlurzEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/DistortionWavesEffect.hsrc/light/effects/FireEffect.hsrc/light/effects/FixedRectangleEffect.hsrc/light/effects/FreqMatrixEffect.hsrc/light/effects/FreqSawsEffect.hsrc/light/effects/GEQ3DEffect.hsrc/light/effects/GEQEffect.hsrc/light/effects/LavaLampEffect.hsrc/light/effects/LissajousEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PaintBrushEffect.hsrc/light/effects/PlasmaEffect.hsrc/light/effects/PraxisEffect.hsrc/light/effects/RainbowEffect.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/fonts.hsrc/light/layers/Layer.hsrc/light/layouts/TubesLayout.hsrc/light/modifiers/MirrorModifier.hsrc/light/moonlive/MoonLiveEffect.htest/CMakeLists.txttest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/light/unit_AudioModule.cpptest/unit/light/unit_Layer_persistence.cpptest/unit/light/unit_RainbowEffect.cpptest/unit/light/unit_TextEffect.cpp
Comment on lines
+124
to
+130
| void setup() override { | ||
| if (active_ == nullptr) active_ = this; // first live mic wins; a 2nd mic is captured but not read | ||
| reinit(); | ||
| } | ||
| void teardown() override { | ||
| deinit(); | ||
| if (active_ == this) active_ = nullptr; // effects fall back to silence | ||
| if (active_ == this) active_ = nullptr; // vacate; a surviving module re-elects itself in loop() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only claim active_ after this module can publish frames.
Line 125 assigns active_ before reinit() can fail or leave the module idle with unset pins. That stale owner blocks a later working AudioModule because Line 157 only self-elects when active_ == nullptr, so effects keep reading silence from the first module.
Suggested direction
void setup() override {
- if (active_ == nullptr) active_ = this; // first live mic wins; a 2nd mic is captured but not read
reinit();
+ if (active_ == nullptr && (inited_ || simulate != 0)) active_ = this;
}Also clear the seat when this instance owns it but no longer has inited_ and simulate == 0, so another live module can claim it on its next tick.
Also applies to: 151-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 `@src/core/AudioModule.h` around lines 124 - 130, The active_ ownership in
AudioModule is claimed too early in setup(), before reinit() confirms the module
can actually publish frames. Update setup() so active_ is assigned only after
successful initialization and publish readiness, using the existing setup(),
reinit(), inited_, and active_ flow. Also adjust the self-election logic in
loop()/tick path so if this instance currently owns active_ but is no longer
inited_ and simulate == 0, it clears active_ and allows another working
AudioModule to claim the seat on its next iteration.
Credit every effect, modifier, and layout with its original author and source (from MoonLight) in both the code and the docs, consolidate the six per-driver doc pages into one drivers.md the way effects.md works, and process a CodeRabbit review batch (fast-board plasma stall, praxis overflow, blurz radius edge, present-tense docs, order-independent img parsing, plus new regression tests). KPI: 16384lights | PC:465KB | tick:256/191/1/250/40/4/533/120/45/37/314/248/42/2/68us(FPS:3906/5235/1000000/4000/25000/250000/1876/8333/22222/27027/3184/4032/23809/500000/14705) | tick:5116us(FPS:195) | heap:31522KB | src:166(30973) | test:90(13362) | lizard:113w Core: - AudioModule: reverted a CodeRabbit suggestion to gate the active_ seat on inited_ — a claimed-but-uninited module publishes valid silence (the documented latestFrame() contract, pinned by the two-mic test), so the first-wins model is correct; the change would have broken the desktop/no-mic case. Light domain: - Attribution: every effect/modifier/layout .h carries an `// Author: <name(s)> — <MoonLight source URL>` line in its header comment (Andrew Tuline / WLED-SR, @softhack007, limpkin, ldirko & blazoncek, @Brandon502 / WildCats08, MONSOONO / @Flavourdynamics, @troyhacks, Ewoud Wijma, Mark Kriegsman / FastLED, netmindz; project-level credit where MoonLight names no individual; "projectMM original" for native effects). - PlasmaEffect: phase_ accumulates the raw numerator (dt·bpm·256) and the /60000 divide runs only at the sampling point — dividing per tick truncated sub-unit progress to zero on a fast board (short dt), stalling the animation. - PraxisEffect: promote now·speed to 64-bit before the divide in huebase — it overflowed uint32 after a few hours' uptime and made the hue jump. - BlurzEffect: dot radius stays 0 for a grid up to 32 (was 1 at exactly 32, contradicting the "≤32 = one pixel" contract); present-tense default comment. - draw.h: box-blur comment corrected — a 1×N 1D layer blurs along Y (its only axis with extent>1), not X. UI: - (docs only this cycle — see Docs / CI) Scripts / MoonDeck: - moondeck.py: <img> parsing is now attribute-order-independent (a shared _img_attrs helper for the table-cell path + an inline extractor for the standalone-line path), so `<img src alt width>` in any order keeps every attribute instead of silently dropping the out-of-order ones. - check_specs.py: added "Driver" to CONSOLIDATED_PAGES so the six leaf *Driver control names route to the new drivers.md (the Drivers container and the ParallelLedDriver CRTP base are excluded). Tests: - unit_Layer_persistence: added a rebuild-clear regression — a lit pixel must not survive Layer::onBuildState() (persistence holds between frames, not across rebuilds). - unit_RainbowEffect: pin Palettes::setActive(0) before the non-black assertion so it no longer depends on prior tests' global palette state. - unit_draw: present-tense comments (dropped "previously untested" / "regression here"); regenerated unit-tests.md and scenario-tests.md. Docs / CI: - drivers.md (new): the six drivers as one page like effects.md — description + controls + Source + a link to each driver's detail page; grouped LED output / Network / Smart light / Preview. - The six detail pages (Rmt/Lcd/Parlio/NetworkSend/Hue/Preview) keep their rich reference material (wire contracts, buffer slicing, memory sizing, the RMT flicker-troubleshooting playbook, loopback self-tests, the P4 pin budget, per-protocol chunking, Hue HTTP contract, Preview binary protocol) with a backlink header; the moved description+controls are dropped. - Attribution: every effect/modifier/layout .md entry gains an `Origin: <project> · by <author> · via [MoonLight] · source [<Name>.h]` line under its controls; the effects Source section completed to all 39. - architecture.md: persistence section states it holds between frames, not across rebuilds (onBuildState clears); trimmed the D1 paragraph's layout-orientation over-reach. Reviews: - 🐇 Plasma fast-board stall (accumulator fix), Praxis 32-bit overflow (64-bit), Blurz radius ≤32 edge, draw.h 1D-blur comment, architecture.md persistence-across-rebuild + D1 wording, unit_draw present-tense, unit_Layer_persistence rebuild-clear test, unit_RainbowEffect palette pin, moondeck.py order-independent <img> — fixed. - 🐇 AudioModule active_ ownership (silence-frame contract; would break desktop no-mic), SolidEffect sqrtf (3/frame cold path, not per-light), StarField float (≤255 stars off the per-pixel path, fidelity-locked), DemoReel swapTo null-after-failed-create (loop()'s !current_ guard already prevents the crash), scenario_Layer_base_pipeline.json tight contract (the 200us pc- tolerance floor already absorbs it; loosening hides regressions) — verified not valid against current code, skipped with reason. Co-Authored-By: Claude Opus 4.8 (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 (11)
docs/architecture.md (1)
554-564: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRewrite this legend in affirmative terms.
This block still defines the native-origin case by absence (
none,not repeated,never duplicates), which conflicts with the markdown wording guideline. Describe the current contract positively instead of centering on what is missing.As per coding guidelines, "Avoid forward-looking or absence-narration phrasing; describe what exists now rather than what was removed or what will happen later, except in backlog, history, and decision-contrast contexts."
✏️ Suggested rewrite
-A module's chips come from three sources, rendered identically on the card and the type picker: a **role** chip (UI-derived from `role`), a **dimensional** chip (UI-derived from `dim`), and the curated **`tags()`** string (a flash literal the module returns; the UI splits it into grapheme clusters, one chip each). Role and dim are *not* repeated in `tags()` — only the categories below are. The `ROLE_EMOJI` / `DIM_EMOJI` maps in `app.js` are the single source of truth for the UI-derived chips; the legend takes [MoonLight](https://github.com/MoonModules/MoonLight)'s set as the canonical basis: +A module's chips come from three sources, rendered identically on the card and the type picker: a **role** chip (UI-derived from `role`), a **dimensional** chip (UI-derived from `dim`), and the curated **`tags()`** string (a flash literal the module returns; the UI splits it into grapheme clusters, one chip each). `tags()` carries the categories below, while the UI adds the role and dimensional chips from `ROLE_EMOJI` / `DIM_EMOJI` in `app.js`; the legend uses [MoonLight](https://github.com/MoonModules/MoonLight)'s set as the canonical basis: @@ -| **Origin / library** (`tags()`) | 💫 MoonLight · 🐙 WLED · ⚡️ FastLED · *(projectMM-native: none — the default)* | which library the module came from; the [migration](../backlog/moonlight-effect-inventory.md) files docs by this, the emoji filters by it | +| **Origin / library** (`tags()`) | 💫 MoonLight · 🐙 WLED · ⚡️ FastLED · *(untagged origin = projectMM-native)* | which library the module came from; the [migration](../backlog/moonlight-effect-inventory.md) inventory uses this marker, and the emoji filters use it | @@ -`tags()` carries **only** origin + creator + audio (+ any genuinely module-specific marker); a module can carry several (e.g. `💫🦅` = MoonLight origin, a named creator). Role and dim are added by the UI, so a module never duplicates them in its string. When migrating, set each module's `tags()` from this legend so the chip set is consistent across the library. +`tags()` carries origin + creator + audio (+ any genuinely module-specific marker); a module can carry several (e.g. `💫🦅` = MoonLight origin, a named creator). The UI adds the role and dimensional chips separately, so `tags()` stays focused on module metadata. When migrating, set each module's `tags()` from this legend so the chip set stays consistent across the library.🤖 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 554 - 564, The legend in this section should be rewritten to describe the current chip sources and categories in affirmative terms instead of defining anything by absence. Update the prose around tags(), ROLE_EMOJI, and DIM_EMOJI so it states that modules present UI-derived role and dimensional chips plus tagged origin/creator/audio markers, and describe the projectMM-native origin as the default origin label in the legend rather than “none” or “not repeated.” Keep the meaning intact while making the wording positive and explicit throughout the legend and the migration guidance.Source: Coding guidelines
src/light/effects/FireEffect.h (1)
106-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDon’t describe Lava as the default palette here.
This path now renders through
*Palettes::active(), and the suppliedsrc/light/Palette.h:213-241context initializes that active palette from builtin 0 (rainbow), not Lava. Reword this to say Lava gives the classic fire look instead of calling it the default unless another supplied caller pins Lava first.✏️ Suggested fix
- // is the palette index, so the flame takes the palette's low→high gradient. The Lava - // palette (black→red→orange→yellow→white) gives the classic fire look and is the intended - // default; any palette works (an Ocean/Forest palette makes a blue/green "fire"). + // is the palette index, so the flame takes the palette's low→high gradient. The Lava + // palette (black→red→orange→yellow→white) gives the classic fire look; any palette + // works (an Ocean/Forest palette makes a blue/green "fire").As per coding guidelines, "Do not remove comments unless they are outdated or factually wrong."
🤖 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/FireEffect.h` around lines 106 - 109, Update the comment in FireEffect to stop calling Lava the default palette, since the rendering path uses Palettes::active() and the active palette is initialized from builtin 0 rather than Lava. Reword the FireEffect heat-to-RGB note to say that Lava produces the classic fire look, while other palettes can also be used, and keep the rest of the explanatory comment intact.Source: Coding guidelines
src/light/effects/WaveEffect.h (1)
52-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the trail on shape changes, not just byte-count changes.
Line 56 only compares
neededagainsttrailBytes_. A rebuild from one geometry/channel layout to another with the same total byte count will keep the old trail contents and reinterpret them with the new stride, so the first frames render stale pixels until they fade out. Trackwidth(),height(), andchannelsPerLight()separately, or clear the trail whenever any of them changes.As per path instructions,
src/light/**: "Effects must run correctly at every grid size and tick rate, including 0×0×0..."Proposed fix
void onBuildState() override { - const uint8_t cpl = channelsPerLight(); - const size_t needed = static_cast<size_t>(width()) * height() * cpl; + const lengthType w = width(); + const lengthType h = height(); + const uint8_t cpl = channelsPerLight(); + const size_t needed = static_cast<size_t>(w) * h * cpl; + const bool shapeChanged = (w != trailW_) || (h != trailH_) || (cpl != trailCpl_); if (enabled() && needed > 0) { - if (needed != trailBytes_) { + if (needed != trailBytes_ || shapeChanged) { releaseTrail(); trail_ = static_cast<uint8_t*>(platform::alloc(needed)); - if (trail_) { std::memset(trail_, 0, needed); trailBytes_ = needed; } + if (trail_) { + std::memset(trail_, 0, needed); + trailBytes_ = needed; + trailW_ = w; + trailH_ = h; + trailCpl_ = cpl; + } } } else { releaseTrail(); } setDynamicBytes(trailBytes_); @@ uint8_t* trail_ = nullptr; size_t trailBytes_ = 0; + lengthType trailW_ = 0; + lengthType trailH_ = 0; + uint8_t trailCpl_ = 0; @@ void releaseTrail() { if (trail_) { platform::free(trail_); trail_ = nullptr; } trailBytes_ = 0; + trailW_ = 0; + trailH_ = 0; + trailCpl_ = 0; }Also applies to: 122-126, 179-182
🤖 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/WaveEffect.h` around lines 52 - 64, The WaveEffect trail allocation in onBuildState() only checks trailBytes_ against the new needed size, so a rebuild with the same byte count but different width(), height(), or channelsPerLight() reuses stale trail contents. Update WaveEffect to track the last geometry/channel layout separately (or explicitly clear/reinitialize the trail whenever any of those inputs change) before recomputing trailBytes_ and calling setDynamicBytes().Source: Path instructions
src/light/effects/NoiseEffect.h (1)
32-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve fractional phase between ticks.
phase_ += static_cast<uint64_t>(dt) * bpm * w * 64 / 60000truncates each frame's fractional motion. On fast ticks / small grids this evaluates to0repeatedly, so the noise field can stop moving until a slower frame arrives. Carry the remainder in fixed-point instead of dividing away the sub-tick progress each loop. As per coding guidelines, "Effects must run correctly at every grid size and tick rate, including 0×0×0, and must not truncate animation math to zero on fast devices."Suggested fix
- phase_ += static_cast<uint64_t>(dt) * bpm * w * 64 / 60000; + phaseRemainder_ += static_cast<uint64_t>(dt) * bpm * w * 64; + phase_ += phaseRemainder_ / 60000u; + phaseRemainder_ %= 60000u;uint64_t phaseRemainder_ = 0;🤖 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/NoiseEffect.h` around lines 32 - 38, The phase update in NoiseEffect is dropping sub-tick motion because the fixed-point increment is divided each loop, so fast ticks and small grids can produce repeated zero advances. Update the phase tracking in NoiseEffect to preserve fractional progress between calls by keeping a persistent remainder field alongside phase_ and folding the leftover bits into the next tick instead of truncating them away. Use the existing elapsed(), lastElapsed_, and phase_ logic as the entry point for the fix, and ensure the animation still advances correctly at every grid size and tick rate.Source: Coding guidelines
src/light/effects/GameOfLifeEffect.h (1)
376-385: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the next-generation population for reset/respawn decisions.
aliveCount/deadCountare counted fromcells_before evolution, but the repetition and density-floor checks run aftercells_has already been replaced withfuture_. If a step kills the last live cells, extinction is missed for that generation and the effect spends an extra tick in the empty state before reset/respawn. Base those checks on the post-step live count instead.Also applies to: 459-476
🤖 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/GameOfLifeEffect.h` around lines 376 - 385, Use the post-evolution population when deciding reset/respawn behavior in GameOfLifeEffect, since the current aliveCount/deadCount are computed from cells_ before the swap to future_. Update the logic around the evolution step in the GameOfLifeEffect update path so the repetition and density-floor checks evaluate the next-generation live count after cells_ has been replaced, using the existing aliveCount/deadCount variables or a new post-step count in the same area.src/light/modifiers/PinwheelModifier.h (2)
95-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake negative
swirlnegate the shear term instead of mirroring the wheel.
swirlis documented as a radius-based angle shear, butif (swirl < 0) angle = 360 - angleflips the base angle before the radius term is applied. That makesswirl = -na different mapping than “the opposite shear of+n”, especially near the centre where the shear should vanish.Suggested fix
- const int swirlFactor = - swirl == 0 ? 0 : static_cast<int>(std::hypot(static_cast<float>(dy), static_cast<float>(dx)) * std::abs(swirl)); + const int swirlDelta = + swirl == 0 ? 0 : static_cast<int>(std::hypot(static_cast<float>(dy), static_cast<float>(dx)) * swirl); @@ - if (swirl < 0) angle = 360 - angle; // Reverse swirl - - int value = angle + swirlFactor + (zTwist * pos.z); - value = static_cast<int>(value / petalWidth_); - value %= petals; + int value = angle + swirlDelta + (zTwist * pos.z); + value = ((value % 360) + 360) % 360; + value = static_cast<int>(value / petalWidth_) % petals;🤖 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/modifiers/PinwheelModifier.h` around lines 95 - 106, The negative-swirl handling in PinwheelModifier::compute should negate the radius-based shear instead of mirroring the base angle. Remove the `if (swirl < 0) angle = 360 - angle` behavior and apply the sign of `swirl` to the `swirlFactor` term in the angle calculation so `+n` and `-n` become opposite shears in the same mapping. Keep the fix localized to the angle/shear logic around `swirlFactor`, `angle`, and `value`.
63-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute the 1D petal index onto
y, notx.
modifyLogicalSize()reshapes 1D inputs to1×petals×1, butmodifyLogical()still writes the petal bin topos.x. After the fold, Layer's bounds check drops every mapped coord withvalue > 0, so only the first petal survives under the new D1-on-Y convention.Suggested fix
- pos.x = static_cast<lengthType>(value); - pos.y = 0; - // 2D+ branch (see modifyLogicalSize): the radius becomes the along-petal y. - if (modifierSize_.y > 1) { - pos.y = static_cast<lengthType>(std::sqrt(static_cast<float>(dx * dx + dy * dy))); - } + if (modifierSize_.y > 1) { + pos.x = static_cast<lengthType>(value); + pos.y = static_cast<lengthType>(std::sqrt(static_cast<float>(dx * dx + dy * dy))); + } else { + pos.x = 0; + pos.y = static_cast<lengthType>(value); + } pos.z = 0;Also applies to: 110-116
🤖 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/modifiers/PinwheelModifier.h` around lines 63 - 74, The 1D petal index is being written to pos.x in PinwheelModifier::modifyLogical(), but modifyLogicalSize() now folds 1D inputs into 1×petals×1, so the mapped coordinate must use y to match the new convention. Update the petal bin assignment in modifyLogical() so the 1D path routes the index onto y while keeping x fixed, and ensure the existing size reshaping logic in modifyLogicalSize() remains consistent with that axis choice.src/light/modifiers/RandomMapModifier.h (1)
51-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard this permutation path against boxes larger than
nrOfLightsType.
boxCountand the flattenedidxboth narrow tonrOfLightsType, so large boxes alias unrelated coordinates before permutation. This reintroduces the wraparound thatGridLayoutalready avoids withuint32_t: for example, a128×128×128cube hits flattened index65536at{x=0,y=0,z=4}. Once that wraps, the advertised “true 1:1 permutation” degenerates into many-to-one remapping.Suggested fix
- const nrOfLightsType boxCount = - static_cast<nrOfLightsType>(box_.x) * static_cast<nrOfLightsType>(box_.y) * - static_cast<nrOfLightsType>(box_.z); - ensurePermutation(boxCount); + const uint64_t boxCount = + static_cast<uint64_t>(box_.x) * static_cast<uint64_t>(box_.y) * + static_cast<uint64_t>(box_.z); + if (boxCount == 0 || boxCount > std::numeric_limits<nrOfLightsType>::max()) { + return true; // safe identity fallback until wider permutation indices exist + } + ensurePermutation(static_cast<nrOfLightsType>(boxCount)); @@ - const nrOfLightsType idx = - static_cast<nrOfLightsType>(pos.z) * static_cast<nrOfLightsType>(w) * static_cast<nrOfLightsType>(h) + - static_cast<nrOfLightsType>(pos.y) * static_cast<nrOfLightsType>(w) + - static_cast<nrOfLightsType>(pos.x); + const uint64_t idx = + static_cast<uint64_t>(pos.z) * static_cast<uint64_t>(w) * static_cast<uint64_t>(h) + + static_cast<uint64_t>(pos.y) * static_cast<uint64_t>(w) + + static_cast<uint64_t>(pos.x); + if (idx > std::numeric_limits<nrOfLightsType>::max()) { + return true; // same safe fallback + }Also applies to: 109-115
🤖 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/modifiers/RandomMapModifier.h` around lines 51 - 68, The permutation logic in RandomMapModifier is narrowing boxCount and the flattened idx to nrOfLightsType, which causes wraparound and many-to-one remapping for boxes larger than that type can represent. Update the permutation path in RandomMapModifier::ensurePermutation and the idx/mapped handling to use a wider integer type or explicitly reject/skip permutation when the box dimensions exceed nrOfLightsType’s safe range, so large grids cannot alias before the permutation is applied.src/light/layouts/TubesLayout.h (1)
41-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
lightCount()aligned with the emit loop for invalid restored values.Line 43 casts signed
nrOfTubes/ledsPerTubetouint32_tbefore validating them. If persisted state restores-1,lightCount()clamps tonrOfLightsType::max()whileforEachCoord()emits nothing because Line 53 fails immediately. That breaks the layout contract and shifts later children inLayouts::forEachCoord().Proposed fix
nrOfLightsType lightCount() const override { - uint32_t n = static_cast<uint32_t>(nrOfTubes) * static_cast<uint32_t>(ledsPerTube); + if (nrOfTubes <= 0 || ledsPerTube <= 0) return 0; + const uint32_t n = + static_cast<uint32_t>(nrOfTubes) * static_cast<uint32_t>(ledsPerTube); constexpr uint32_t kMax = std::numeric_limits<nrOfLightsType>::max(); return static_cast<nrOfLightsType>(n > kMax ? kMax : n); }🤖 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/layouts/TubesLayout.h` around lines 41 - 46, The `TubesLayout::lightCount()` implementation is out of sync with `forEachCoord()` for invalid restored values because it multiplies after casting signed fields to `uint32_t`, which turns negative values into huge counts instead of rejecting them. Update `lightCount()` to validate `nrOfTubes` and `ledsPerTube` with the same non-negative checks used by `forEachCoord()` before computing the total, and return zero or otherwise an empty count for invalid state so `Layouts::forEachCoord()` stays aligned with the actual emitted coordinates.docs/moonmodules/light/modifiers/modifiers.md (1)
3-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe the live-pass gate as
hasModifyLive().
ModifierBasemakes the per-frame path opt-in viahasModifyLive(), not by overridingmodifyLive()alone. This line currently documents the wrong contract for anyone implementing a dynamic modifier. As per path instructions, "docs/moonmodules/*.mdmust be end-user/API-integrator documentation that explains contracts, cross-domain wiring, prior art, and at least one mention of every control name, without repeating facts already stated in the corresponding header."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/light/modifiers/modifiers.md` at line 3, The modifier docs currently describe the live-pass opt-in contract incorrectly, so update the `ModifierBase`/dynamic modifier explanation to name `hasModifyLive()` as the gate for per-frame behavior rather than implying `modifyLive()` alone is sufficient. Keep the surrounding `ModifierBase`, `modifyLive`, and dynamic/static split guidance aligned with the existing modifier overview so readers understand that a modifier must explicitly opt into the live pass.Source: Path instructions
scripts/check/check_specs.py (1)
68-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the exact-stem fallback when the shared page is missing.
return page if page.exists() else Nonestops the lookup early, so a suffix-backed module loses its spec even when a per-module page still exists.♻️ Proposed fix
- if name.endswith(suffix): - return page if page.exists() else None + if name.endswith(suffix): + if page.exists(): + return page + break🤖 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 `@scripts/check/check_specs.py` around lines 68 - 88, The fallback in find_spec is too aggressive for suffix-backed modules: when a consolidated page is absent, it returns None before checking the per-module spec. Update find_spec so the CONSOLIDATED_PAGES lookup only returns the shared page when it exists, and otherwise continues into the existing SPECS.rglob("*.md") exact-stem search. Keep the MoonLive exception and the stem-matching behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/moonmodules/light/drivers/drivers.md`:
- Line 20: Hyphenate the compound adjective in the `loopbackFrame` documentation
text by changing “back to back” to “back-to-back” so the wording is consistent
with the style check. Update the sentence in the `drivers.md` entry for
`loopbackFrame` only; no other content needs to change.
In `@docs/moonmodules/light/drivers/LcdLedDriver.md`:
- Line 3: The LcdLedDriver.md rewrite removed the driver’s own control list, so
restore at least one mention of each control name on this module page while
keeping the link to drivers.md for the fuller overview. Update the
LcdLedDriver.md content so it still names the controls alongside the contract
details, using the LcdLedDriver page itself as the place where integrators can
find every control name without duplicating the header’s already-stated facts.
In `@scripts/moondeck.py`:
- Around line 1822-1840: The standalone image handling in the
`_render_cell`/`stripped_check` branch only matches a bare `<img ...>` tag, so
lines that include a trailing HTML comment fall through and render as escaped
text. Update the `<img>` fast-path in `scripts/moondeck.py` to also accept an
optional trailing comment after the tag, while keeping the existing attribute
parsing and `lines.append(...)` rendering behavior unchanged.
In `@src/light/effects/BlurzEffect.h`:
- Around line 26-29: The comment in BlurzEffect.h overstates that silence always
means all bands are 0, but AudioModule::latestFrame() can produce nonzero bands
in simulate modes. Update the wording near BlurzEffect so it matches the actual
AudioModule simulation behavior, explicitly limiting the silence/fade-to-black
contract to cases where simulation is disabled or no publisher is active.
---
Outside diff comments:
In `@docs/architecture.md`:
- Around line 554-564: The legend in this section should be rewritten to
describe the current chip sources and categories in affirmative terms instead of
defining anything by absence. Update the prose around tags(), ROLE_EMOJI, and
DIM_EMOJI so it states that modules present UI-derived role and dimensional
chips plus tagged origin/creator/audio markers, and describe the
projectMM-native origin as the default origin label in the legend rather than
“none” or “not repeated.” Keep the meaning intact while making the wording
positive and explicit throughout the legend and the migration guidance.
In `@docs/moonmodules/light/modifiers/modifiers.md`:
- Line 3: The modifier docs currently describe the live-pass opt-in contract
incorrectly, so update the `ModifierBase`/dynamic modifier explanation to name
`hasModifyLive()` as the gate for per-frame behavior rather than implying
`modifyLive()` alone is sufficient. Keep the surrounding `ModifierBase`,
`modifyLive`, and dynamic/static split guidance aligned with the existing
modifier overview so readers understand that a modifier must explicitly opt into
the live pass.
In `@scripts/check/check_specs.py`:
- Around line 68-88: The fallback in find_spec is too aggressive for
suffix-backed modules: when a consolidated page is absent, it returns None
before checking the per-module spec. Update find_spec so the CONSOLIDATED_PAGES
lookup only returns the shared page when it exists, and otherwise continues into
the existing SPECS.rglob("*.md") exact-stem search. Keep the MoonLive exception
and the stem-matching behavior intact.
In `@src/light/effects/FireEffect.h`:
- Around line 106-109: Update the comment in FireEffect to stop calling Lava the
default palette, since the rendering path uses Palettes::active() and the active
palette is initialized from builtin 0 rather than Lava. Reword the FireEffect
heat-to-RGB note to say that Lava produces the classic fire look, while other
palettes can also be used, and keep the rest of the explanatory comment intact.
In `@src/light/effects/GameOfLifeEffect.h`:
- Around line 376-385: Use the post-evolution population when deciding
reset/respawn behavior in GameOfLifeEffect, since the current
aliveCount/deadCount are computed from cells_ before the swap to future_. Update
the logic around the evolution step in the GameOfLifeEffect update path so the
repetition and density-floor checks evaluate the next-generation live count
after cells_ has been replaced, using the existing aliveCount/deadCount
variables or a new post-step count in the same area.
In `@src/light/effects/NoiseEffect.h`:
- Around line 32-38: The phase update in NoiseEffect is dropping sub-tick motion
because the fixed-point increment is divided each loop, so fast ticks and small
grids can produce repeated zero advances. Update the phase tracking in
NoiseEffect to preserve fractional progress between calls by keeping a
persistent remainder field alongside phase_ and folding the leftover bits into
the next tick instead of truncating them away. Use the existing elapsed(),
lastElapsed_, and phase_ logic as the entry point for the fix, and ensure the
animation still advances correctly at every grid size and tick rate.
In `@src/light/effects/WaveEffect.h`:
- Around line 52-64: The WaveEffect trail allocation in onBuildState() only
checks trailBytes_ against the new needed size, so a rebuild with the same byte
count but different width(), height(), or channelsPerLight() reuses stale trail
contents. Update WaveEffect to track the last geometry/channel layout separately
(or explicitly clear/reinitialize the trail whenever any of those inputs change)
before recomputing trailBytes_ and calling setDynamicBytes().
In `@src/light/layouts/TubesLayout.h`:
- Around line 41-46: The `TubesLayout::lightCount()` implementation is out of
sync with `forEachCoord()` for invalid restored values because it multiplies
after casting signed fields to `uint32_t`, which turns negative values into huge
counts instead of rejecting them. Update `lightCount()` to validate `nrOfTubes`
and `ledsPerTube` with the same non-negative checks used by `forEachCoord()`
before computing the total, and return zero or otherwise an empty count for
invalid state so `Layouts::forEachCoord()` stays aligned with the actual emitted
coordinates.
In `@src/light/modifiers/PinwheelModifier.h`:
- Around line 95-106: The negative-swirl handling in PinwheelModifier::compute
should negate the radius-based shear instead of mirroring the base angle. Remove
the `if (swirl < 0) angle = 360 - angle` behavior and apply the sign of `swirl`
to the `swirlFactor` term in the angle calculation so `+n` and `-n` become
opposite shears in the same mapping. Keep the fix localized to the angle/shear
logic around `swirlFactor`, `angle`, and `value`.
- Around line 63-74: The 1D petal index is being written to pos.x in
PinwheelModifier::modifyLogical(), but modifyLogicalSize() now folds 1D inputs
into 1×petals×1, so the mapped coordinate must use y to match the new
convention. Update the petal bin assignment in modifyLogical() so the 1D path
routes the index onto y while keeping x fixed, and ensure the existing size
reshaping logic in modifyLogicalSize() remains consistent with that axis choice.
In `@src/light/modifiers/RandomMapModifier.h`:
- Around line 51-68: The permutation logic in RandomMapModifier is narrowing
boxCount and the flattened idx to nrOfLightsType, which causes wraparound and
many-to-one remapping for boxes larger than that type can represent. Update the
permutation path in RandomMapModifier::ensurePermutation and the idx/mapped
handling to use a wider integer type or explicitly reject/skip permutation when
the box dimensions exceed nrOfLightsType’s safe range, so large grids cannot
alias before the permutation is applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9452618a-81c7-41f7-b09c-0b2d9d55b770
📒 Files selected for processing (85)
docs/architecture.mddocs/moonmodules/light/drivers/HueDriver.mddocs/moonmodules/light/drivers/LcdLedDriver.mddocs/moonmodules/light/drivers/NetworkSendDriver.mddocs/moonmodules/light/drivers/ParlioLedDriver.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/drivers/RmtLedDriver.mddocs/moonmodules/light/drivers/drivers.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/modifiers/modifiers.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdscripts/check/check_specs.pyscripts/moondeck.pysrc/core/AudioModule.hsrc/light/draw.hsrc/light/effects/AudioSpectrumEffect.hsrc/light/effects/AudioVolumeEffect.hsrc/light/effects/BlurzEffect.hsrc/light/effects/BouncingBallsEffect.hsrc/light/effects/DemoReelEffect.hsrc/light/effects/DistortionWavesEffect.hsrc/light/effects/FireEffect.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/NetworkReceiveEffect.hsrc/light/effects/Noise2DEffect.hsrc/light/effects/NoiseEffect.hsrc/light/effects/NoiseMeterEffect.hsrc/light/effects/PaintBrushEffect.hsrc/light/effects/ParticlesEffect.hsrc/light/effects/PlasmaEffect.hsrc/light/effects/PraxisEffect.hsrc/light/effects/RainbowEffect.hsrc/light/effects/RandomEffect.hsrc/light/effects/RingsEffect.hsrc/light/effects/RipplesEffect.hsrc/light/effects/RubiksCubeEffect.hsrc/light/effects/SineEffect.hsrc/light/effects/SolidEffect.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/WaveEffect.hsrc/light/layouts/CarLightsLayout.hsrc/light/layouts/CubeLayout.hsrc/light/layouts/GridLayout.hsrc/light/layouts/HumanSizedCubeLayout.hsrc/light/layouts/PanelLayout.hsrc/light/layouts/PanelsLayout.hsrc/light/layouts/RingLayout.hsrc/light/layouts/Rings241Layout.hsrc/light/layouts/SingleColumnLayout.hsrc/light/layouts/SingleRowLayout.hsrc/light/layouts/SphereLayout.hsrc/light/layouts/SpiralLayout.hsrc/light/layouts/TorontoBarGourdsLayout.hsrc/light/layouts/TubesLayout.hsrc/light/layouts/WheelLayout.hsrc/light/modifiers/BlockModifier.hsrc/light/modifiers/CheckerboardModifier.hsrc/light/modifiers/CircleModifier.hsrc/light/modifiers/MirrorModifier.hsrc/light/modifiers/MultiplyModifier.hsrc/light/modifiers/PinwheelModifier.hsrc/light/modifiers/RandomMapModifier.hsrc/light/modifiers/RegionModifier.hsrc/light/modifiers/RippleXZModifier.hsrc/light/modifiers/RotateModifier.hsrc/light/modifiers/TransposeModifier.htest/unit/light/unit_Layer_persistence.cpptest/unit/light/unit_RainbowEffect.cpptest/unit/light/unit_draw.cpp
| # LCD LED Driver | ||
|
|
||
| Parallel [WS2812B](https://cdn-shop.adafruit.com/datasheets/WS2812B.pdf) output on the **ESP32-S3** over the [LCD_CAM](https://docs.espressif.com/projects/esp-idf/en/stable/esp32s3/api-reference/peripherals/lcd/index.html) peripheral: up to **8 strands clock out simultaneously**, one GPIO per strand, all fed by a single autonomous DMA transfer. The S3's scale path — where the [RMT driver](RmtLedDriver.md) tops out at 4 channels on the S3, this drives 8 lanes for the wall time of one. Reads the Drivers container's buffer, applies the shared [Correction](Correction.md) per light, and bit-transposes corrected bytes across the lanes. | ||
| Overview and controls: [drivers.md § LCD LED](drivers.md#lcdled). This page carries the reference detail a control list can't — 3-slot-per-bit wire contract, buffer slicing, DMA memory sizing, and cross-domain wiring. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep each control name on the module page.
This rewrite drops the control list from LcdLedDriver.md, so the per-driver page no longer mentions the driver's own controls. Linking back to drivers.md is useful, but this page still needs at least one mention of each control name. As per path instructions, "docs/moonmodules/*.md must be end-user/API-integrator documentation that explains contracts, cross-domain wiring, prior art, and at least one mention of every control name, without repeating facts already stated in the corresponding header."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/moonmodules/light/drivers/LcdLedDriver.md` at line 3, The
LcdLedDriver.md rewrite removed the driver’s own control list, so restore at
least one mention of each control name on this module page while keeping the
link to drivers.md for the fuller overview. Update the LcdLedDriver.md content
so it still names the controls alongside the contract details, using the
LcdLedDriver page itself as the place where integrators can find every control
name without duplicating the header’s already-stated facts.
Source: Path instructions
…e digests
Move the filesystem-usage bar to the module that owns the filesystem, delink a dead Correction.md reference, remove the redundant sdkDate control (the build date already lives on the firmware card), process a CodeRabbit review batch (fast-board effect stalls, a Pinwheel 1D bug, integer/geometry guards, doc wording), and add the June 2026 friend-repo activity digests.
KPI: 16384lights | PC:465KB | tick:335/231/1/340/45/6/816/174/50/56/485/338/48/3/1/98us(FPS:2985/4329/1000000/2941/22222/166666/1225/5747/20000/17857/2061/2958/20833/333333/1000000/10204) | tick:5007us(FPS:199) | heap:31526KB | src:166(30979) | test:91(13423) | lizard:113w
Core:
- FilesystemModule: owns the `filesystem` usage bar now (bytes used / total, refreshed in loop1s) — moved off SystemModule, the module that actually owns the filesystem. Bound only when the platform reports a real partition.
- SystemModule: dropped the `filesystem` bar (moved) and the `sdkDate` control (it showed the firmware build date, not an SDK release date — and the build date is already on the firmware card, so it was a duplicate). Kept `sdk` (the IDF version string) and `flash` (chip flash size).
- platform: removed `platform::sdkDate()` (ESP32 + desktop) and its declaration, and the now-unused `esp_app_desc.h` include.
Light domain:
- PinwheelModifier: 1D petal index now routes onto Y (the {1, petals, 1} reshape puts petals on Y) — it was written to x, so on a 1D layer every petal but 0 fell outside the 1-wide logical box and the pinwheel collapsed to a single petal. Pinned by a new unit_PinwheelModifier test.
- NoiseEffect: phase accumulates the raw numerator and divides only at sample time — dividing per tick truncated sub-unit progress to zero on a fast board / small grid, stalling the field (same fix class as PlasmaEffect).
- WaveEffect: clears the trail buffer when the grid geometry changes but the byte count stays equal (e.g. 8×16 → 16×8) — reusing the old-layout bytes smeared stale pixels for a few frames.
- TubesLayout: lightCount() rejects negative restored dimensions (return 0) instead of casting to uint32_t and reporting a phantom huge count, so it agrees with forEachCoord().
- FireEffect: comment no longer calls Lava "the intended default" (the render uses the active palette, which inits from builtin 0) — Lava gives the classic look, any palette works.
- BlurzEffect: comment scopes the silence→fade-to-black claim to "simulation off or no publisher" (simulate modes publish nonzero bands).
UI:
- (docs only this cycle — see Docs / CI)
Scripts / MoonDeck:
- moondeck.py: the standalone-<img> fast path accepts an optional trailing HTML comment after the tag (a `<img …> <!-- note -->` line rendered as escaped text before).
- check_specs.py: find_spec falls through to the per-module stem search when a consolidated page is absent (it returned None early); routing for the Driver-suffix consolidation is unchanged.
Tests:
- unit_PinwheelModifier (new): pins the 2D (petals-on-x) and 1D (petals-on-y) reshapes and that a 1D sweep maps across more than one petal (the regression).
Docs / CI:
- drivers.md / RmtLedDriver.md: the dead `[Correction](Correction.md)` links (Correction is a struct owned by Drivers, not a module) now point at `Drivers.md#output-correction`, where its controls are already documented. drivers.md `back to back` → `back-to-back`.
- FilesystemModule.md / SystemModule.md: the `filesystem` bar doc moved with the control; `sdkDate` line removed.
- architecture.md: persistence section notes it holds between frames but not across rebuilds (onBuildState clears); D1 expand-rule paragraph trimmed of its layout-orientation over-reach; the tag/role/dim legend states projectMM-native as the default origin affirmatively instead of "none".
- history/README.md: the reusable digest prompt gains a step to investigate the month's GitHub issues (not just commits), the auditability line now cites the issue query too, and the cross-repo-trends window extends to June 2026 (with NightDriverStrip 2.0's runtime-drivers/new-UI folded into the UI-as-consumer thread).
- history digests: added `## June 2026` to all 8 friend repos — FastLED (split at 3.10.4: RGBW rework, LPC/Teensy driver bring-up), NightDriverStrip (split at v2.0.0: new web UI + installer, runtime-selectable drivers), WLED (post-16.0 fixes, no split — release cut off a separate branch), WLED-MM (S3 Matrix board, reboot/palette fixes); troyhacks + the three hpwit repos had no June activity (noted with the ranges checked).
Reviews:
- 🐇 NoiseEffect phase stall, WaveEffect stale-trail-on-geometry-change, TubesLayout negative lightCount, FireEffect "default palette" wording, BlurzEffect silence wording, Pinwheel 1D petal-axis (+ regression test), drivers.md hyphen, moondeck.py trailing-comment <img>, check_specs.py find_spec fallthrough, architecture.md persistence/D1/legend wording — fixed.
- 🐇 GameOfLifeEffect pre-swap alive count (intended MoonLight-fidelity behaviour), RandomMapModifier nrOfLightsType narrowing (the pipeline-wide light-index type, not a RandomMap bug), PinwheelModifier negative-swirl 360-angle (MoonLight-exact — negating would diverge), SolidEffect sqrtf (3/frame cold path, not per-light), StarField float (bounded stars off the per-pixel path, fidelity-locked), LcdLedDriver control re-add (already documented on drivers.md via the consolidation), modifiers.md live-pass wording (already correct) — verified not valid against current code, skipped with reason.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
changed the title
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 (5)
src/light/effects/WaveEffect.h (1)
159-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
inoise8primitive instead of a private hash/valueNoise.
valueNoise/hashreimplement 1D value noise with its own mixing constants, duplicating whatcore/noise.h'sinoise8(added in this same PR) already provides. Callinginoise8(static_cast<uint32_t>(phase) << 8)for theNoisewaveform case would drop this private implementation and keep noise generation consolidated in one place, matching the intent of centralizing math8/noise primitives.♻️ Proposed refactor
- default: v = valueNoise(phase); break; // Noise (type 5) + default: v = inoise8(static_cast<uint32_t>(phase) << 8); break; // Noise (type 5)- // 1-D value noise from a small integer hash (the NoiseEffect hash, reduced to one axis) — a - // smoothly-varying pseudo-random byte, so the "Noise" wave jitters without being pure static. - static uint8_t valueNoise(uint8_t x) { - uint32_t hi = x >> 4, lo = x & 0x0F; // cell + fractional position - uint8_t a = hash(hi), b = hash(hi + 1); - return static_cast<uint8_t>(a + ((static_cast<int>(b) - a) * lo) / 16); // lerp between cells - } - static uint8_t hash(uint32_t n) { - n = (n << 13) ^ n; - return static_cast<uint8_t>((n * (n * n * 15731u + 789221u) + 1376312589u) >> 24); - }🤖 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/WaveEffect.h` around lines 159 - 176, The Noise waveform in WaveEffect.h currently duplicates 1-D value noise via the private valueNoise/hash helpers. Replace that path with the shared inoise8 primitive from core/noise.h, using the phase value in the Noise case inside the WaveEffect logic, and remove the redundant valueNoise and hash implementations so noise generation is centralized.scripts/moondeck.py (2)
1817-1845: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStandalone
<img>handler doesn't escapesrc/width/altat all, unlike the sibling_render_cell._img().
_render_cell's_img()at least escapesalt; this handler interpolatessrc_,w_, andalt_straight from the regex match into the output f-string with no escaping, so a stray"in any of them breaks the attribute and can inject arbitrary markup into the rendered page.🛡️ Suggested fix
- w_ = _attr("width") - wattr = f' width="{w_}"' if w_ else "" - alt_ = _attr("alt") - aattr = f' alt="{alt_}"' if alt_ is not None else "" - lines.append(f'<img src="{src_}"{wattr}{aattr} style="margin:4px 0">') + w_ = _attr("width") + wattr = f' width="{html_mod.escape(w_)}"' if w_ else "" + alt_ = _attr("alt") + aattr = f' alt="{html_mod.escape(alt_)}"' if alt_ is not None else "" + lines.append(f'<img src="{html_mod.escape(src_)}"{wattr}{aattr} style="margin:4px 0">')🤖 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 `@scripts/moondeck.py` around lines 1817 - 1845, The standalone <img> handling in moondeck.py interpolates src_, w_, and alt_ directly into the output HTML, which can break attributes or inject markup. Update this branch to escape attribute values before building the img tag, following the same approach used by _render_cell._img() so the standalone path treats src/width/alt safely and consistently.
1636-1687: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_img()escapesaltbut notsrc/width/style.
altattris passed throughhtml_mod.escape, butsrc_,width, andstyleare interpolated into the<img ...>output verbatim. A doc author'sstyle="..."value (or a path segment) containing a"would break out of the attribute and corrupt the rendered HTML.🛡️ Suggested fix
- wattr = f' width="{width}"' if width else "" + wattr = f' width="{html_mod.escape(width)}"' if width else "" altattr = f' alt="{html_mod.escape(alt_)}"' if alt_ else "" style = (style + ";" if style else "") + "margin:4px 0" - return f'<img src="{src_}"{wattr}{altattr} style="{style}">' + return f'<img src="{html_mod.escape(src_)}"{wattr}{altattr} style="{html_mod.escape(style)}">'🤖 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 `@scripts/moondeck.py` around lines 1636 - 1687, The _img() helper in render_cell is interpolating src_, width, and style directly into the generated <img> tag while only alt_ is escaped. Update the _img() path inside _render_cell so every attribute value that is emitted into HTML is escaped or safely quoted before formatting the tag, especially style and width (and any src value that may contain special characters), keeping the existing altattr handling and overall raw-<img> behavior unchanged.scripts/check/check_specs.py (1)
53-67: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComment scope no longer matches the code's behavior.
The comment says "the leaf layouts (Grid/Sphere/Wheel) document themselves as one compact-row page," but
CONSOLIDATED_PAGES["Layout"]suffix-matches every module ending inLayout— including the newCarLightsLayout,CubeLayout,PanelLayout,RingLayout, etc. added in this same PR, which also get routed to the sharedlayouts.md. The comment describes a narrower/stale scope than what actually happens.✏️ Suggested comment update
-# Effects, modifiers, and the leaf layouts (Grid/Sphere/Wheel) document themselves as one -# compact-row page per type rather than a file per module (docs consolidation — see the -# folder-structure decision). A module of one of these types is documented on its shared page; +# Effects, modifiers, and layouts document themselves as one compact-row page per type rather +# than a file per module (docs consolidation — see the folder-structure decision). A module of +# one of these types is documented on its shared page;🤖 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 `@scripts/check/check_specs.py` around lines 53 - 67, Update the explanatory comment near CONSOLIDATED_PAGES in check_specs.py so it matches the actual suffix-based behavior of discover_modules and the shared layouts.md routing. The current wording is stale because Layout modules like CarLightsLayout, CubeLayout, PanelLayout, and RingLayout also consolidate, not just the older leaf layouts. Keep the note focused on the true rule used by CONSOLIDATED_PAGES and its type-name suffix matching.docs/moonmodules/light/drivers/drivers.md (1)
79-79: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDecode
%20before resolving doc assets.docs/assets/light/drivers/Hue driver.pngis a literal-space filename, but this markdown points toHue%20driver.png, so MoonDeck turns it into/api/doc-asset/.../Hue%20driver.pngand the image 404s in the local View pane. Rename the asset to a no-space name or URL-decodesrcbefore serving it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/light/drivers/drivers.md` at line 79, The markdown image reference in drivers.md is using a URL-encoded space, which causes the doc asset resolver to request the wrong path and 404. Update the HueDriver image source handling so MoonDeck resolves doc assets with decoded src values before serving them, or rename the asset to avoid spaces; check the markdown link and the doc-asset serving path together to ensure they match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/history/FastLED-FastLED.md`:
- Around line 5-38: The June history sections use plain “New:” and “Fixed:”
labels, which are inconsistent with the rest of the digest. Update the headings
in this document to match the existing style by using the same bold label format
as other months, and apply the same change to all matching New/Fixed occurrences
in the June 2026 sections.
In `@docs/history/hpwit-ESPLiveScript.md`:
- Around line 9-13: Update the June 2026 entry in hpwit-ESPLiveScript to include
version-branch activity alongside main/issues/PRs/releases, since the digest is
meant to cover active version branches. Use the existing month-summary block and
its checked-data section to either explicitly state that all version branches
were quiet in June 2026 or summarize any work found on those branches, keeping
the wording aligned with the summary’s scope.
In `@docs/history/README.md`:
- Around line 68-73: The issue sweep in docs/history/README.md can overcount
because gh api repos/<owner>/<NAME>/issues returns pull requests too. Update the
guidance in the “investigate the issues” step to explicitly filter out PRs (for
example by checking for the pull_request field or otherwise excluding them)
before summarizing issues, so the issue query matches only real issues.
In `@src/light/effects/NoiseEffect.h`:
- Around line 32-40: Seed lastElapsed_ on the first update in NoiseEffect so the
initial dt does not include full device uptime and the noise phase starts at t =
0. Add the same one-time guard used in WaveEffect (the started_/first-tick
pattern) around the elapsed()/dt accumulation in the NoiseEffect update path,
then initialize lastElapsed_ from the current elapsed value before applying
phase_ += dt * bpm * w * 64.
---
Outside diff comments:
In `@docs/moonmodules/light/drivers/drivers.md`:
- Line 79: The markdown image reference in drivers.md is using a URL-encoded
space, which causes the doc asset resolver to request the wrong path and 404.
Update the HueDriver image source handling so MoonDeck resolves doc assets with
decoded src values before serving them, or rename the asset to avoid spaces;
check the markdown link and the doc-asset serving path together to ensure they
match.
In `@scripts/check/check_specs.py`:
- Around line 53-67: Update the explanatory comment near CONSOLIDATED_PAGES in
check_specs.py so it matches the actual suffix-based behavior of
discover_modules and the shared layouts.md routing. The current wording is stale
because Layout modules like CarLightsLayout, CubeLayout, PanelLayout, and
RingLayout also consolidate, not just the older leaf layouts. Keep the note
focused on the true rule used by CONSOLIDATED_PAGES and its type-name suffix
matching.
In `@scripts/moondeck.py`:
- Around line 1817-1845: The standalone <img> handling in moondeck.py
interpolates src_, w_, and alt_ directly into the output HTML, which can break
attributes or inject markup. Update this branch to escape attribute values
before building the img tag, following the same approach used by
_render_cell._img() so the standalone path treats src/width/alt safely and
consistently.
- Around line 1636-1687: The _img() helper in render_cell is interpolating src_,
width, and style directly into the generated <img> tag while only alt_ is
escaped. Update the _img() path inside _render_cell so every attribute value
that is emitted into HTML is escaped or safely quoted before formatting the tag,
especially style and width (and any src value that may contain special
characters), keeping the existing altattr handling and overall raw-<img>
behavior unchanged.
In `@src/light/effects/WaveEffect.h`:
- Around line 159-176: The Noise waveform in WaveEffect.h currently duplicates
1-D value noise via the private valueNoise/hash helpers. Replace that path with
the shared inoise8 primitive from core/noise.h, using the phase value in the
Noise case inside the WaveEffect logic, and remove the redundant valueNoise and
hash implementations so noise generation is centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29f8d82c-a433-4f3e-ab27-38773035c5b2
📒 Files selected for processing (30)
docs/architecture.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/troyhacks-WLED.mddocs/history/wled-WLED.mddocs/moonmodules/core/FilesystemModule.mddocs/moonmodules/core/SystemModule.mddocs/moonmodules/light/drivers/RmtLedDriver.mddocs/moonmodules/light/drivers/drivers.mdscripts/check/check_specs.pyscripts/moondeck.pysrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/SystemModule.hsrc/light/effects/BlurzEffect.hsrc/light/effects/FireEffect.hsrc/light/effects/NoiseEffect.hsrc/light/effects/WaveEffect.hsrc/light/layouts/TubesLayout.hsrc/light/modifiers/PinwheelModifier.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/CMakeLists.txttest/unit/light/unit_PinwheelModifier.cpp
💤 Files with no reviewable changes (4)
- docs/moonmodules/core/SystemModule.md
- src/platform/platform.h
- src/platform/esp32/platform_esp32.cpp
- src/platform/desktop/platform_desktop.cpp
Comment on lines
+5
to
+38
| ## June 2026 (up to 3.10.4) | ||
|
|
||
| Released **3.10.4** (2026-06-16), cut from `master`. | ||
|
|
||
| New: | ||
| - STM32: Arduino UNO Q board support. | ||
| - New NXP LPC8xx family drivers land (LPC804 PLU, LPC845 bit-bang + PWM/DMA-to-GPIO, LPC11xx) — early bring-up, bench-validated. | ||
| - Unified `fl::Watchdog` API with real hardware implementations across platforms (ESP32, Teensy 4, AVR, Apollo3, RP2040, STM32) plus a non-allocating reset/crash-classification helper. | ||
| - Wave simulation: opt-in 9-point isotropic Laplacian (smoother 2D waves), exposed as a UI toggle in several example sketches. | ||
| - ScreenMap gains a v2 schema (auto-detected), and a new `.fled` container format for video/screenmaps that `FxSdCard` can load. | ||
| - FFT now auto-detects ESP-DSP on ESP32 (no opt-in macro needed). | ||
|
|
||
| Fixed: | ||
| - RGBW colorimetric path reworked: native LED gamut + D65 as the default source, improved strict/boosted solvers, corrected dual- and 3-channel solving. | ||
| - ESP32: reliable streaming for SPI strips over ~680 LEDs; ESP32-C6 routes Serial to HWCDC with non-blocking writes and rejects USB-serial pins for LED output. | ||
| - Teensy Audio selection fixed on low-memory boards; nRF52 now honors configured SPI data rates. | ||
| - LuminescentGrand example: corrected serpentine column wiring/orientation. | ||
|
|
||
| ## June 2026 (post-3.10.4) | ||
|
|
||
| New: | ||
| - Teensy 4.x LED driver bring-up: ObjectFLED and FlexIO parallel output engines plus a new LPUART-based WS2812 driver (inverted-TX + eDMA), and FlexPWM-based RX capture. | ||
| - New WS2812-style RX capture path on classic ESP32 (RMT4) and LPC845 (SCT+DMA). | ||
| - ARM Cortex-M DSP-extension SIMD backend wired up for Teensy 3.x/4.x (faster scale/blend on those chips). | ||
|
|
||
| Fixed: | ||
| - SM16824E chipset timing corrected to match the datasheet. | ||
| - RGBW gamut configuration is now kept per strip. | ||
| - Fixed a memory leak in the chunked `fl::deque` container. | ||
|
|
||
| Note: the bulk of June's ~481 commits were internal (LPC bring-up scaffolding, the AutoResearch hardware-test harness, a Python→Rust C++ linter migration, RPC/JSON size-and-speed tuning, and test-file splits) and are not user-visible. Notable issue traffic was dominated by the Teensy driver and LPC845 bring-up trackers; a user-reported "multi strip problem" (#3340) was triaged and closed, and a deque refactor briefly broke a macOS-arm64 audio unit test (#3286, fixed same month). | ||
|
|
||
| _Auditability: 481 commits with author-date in 2026-06-01..2026-06-30 on `master` (first-parent/merged view); split at the 3.10.4 release (published 2026-06-16, an ancestor of `master`). Issues reviewed via `search/issues` for `created:2026-06-01..2026-06-30` and `closed:2026-06-01..2026-06-30` (~50 each); user-facing ones folded in above, the remainder were internal bring-up/CI/linter trackers._ | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Heading style inconsistent with the rest of the file.
Every other month uses bold **New** / **Fixed** labels; these two new June sections use plain New: / Fixed:, which won't render as visually distinct headers like the rest of the digest.
✏️ Suggested fix
-New:
+**New**
- STM32: Arduino UNO Q board support.(and similarly for the other three New:/Fixed: occurrences in this section)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/FastLED-FastLED.md` around lines 5 - 38, The June history
sections use plain “New:” and “Fixed:” labels, which are inconsistent with the
rest of the digest. Update the headings in this document to match the existing
style by using the same bold label format as other months, and apply the same
change to all matching New/Fixed occurrences in the June 2026 sections.
Comment on lines
+9
to
+13
| ## June 2026 | ||
|
|
||
| No user-facing activity: no merged commits on `main` and no notable issues in June 2026. (Latest commit on `main` predates the window — June 2025.) | ||
|
|
||
| _Checked: commits on `main` author-dated 2026-06-01..2026-06-30 (0 commits); issues created 2026-06-01..2026-06-30, closed 2026-06-01..2026-06-30, and updated 2026-06-01..2026-06-30 (0 each); PRs created in-window (0); no versioned release published in June 2026._ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target history file with line numbers
sed -n '1,120p' docs/history/hpwit-ESPLiveScript.md | cat -n
# Find nearby history files that may share the same wording/scope pattern
fd -a 'hpwit-*.md' docs/history
# Search for the key phrases used in these summaries
rg -n "user-facing activity|version branches|main predates the window|latest commit on `main`" docs/history docs -g '*.md'Repository: MoonModules/projectMM
Length of output: 5450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the history README for the summary rules and branch-scope language
sed -n '1,120p' docs/history/README.md | cat -n
# Find where this history note is generated or referenced
rg -n "hpwit-ESPLiveScript|version branches|main is quiet|This file gets a new" README.md docs -g '*.md'Repository: MoonModules/projectMM
Length of output: 14423
Include the version-branch activity in June 2026. docs/history/hpwit-ESPLiveScript.md:9-13 says this digest reads across the active version branches, but the month summary only checks main, issues, PRs, and releases. If June was quiet everywhere, say that explicitly; otherwise fold the version-branch work into the summary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/hpwit-ESPLiveScript.md` around lines 9 - 13, Update the June
2026 entry in hpwit-ESPLiveScript to include version-branch activity alongside
main/issues/PRs/releases, since the digest is meant to cover active version
branches. Use the existing month-summary block and its checked-data section to
either explicitly state that all version branches were quiet in June 2026 or
summarize any work found on those branches, keeping the wording aligned with the
summary’s scope.
Comment on lines
+68
to
+73
| > 1b. **Also investigate the issues over that month** (`gh api repos/<owner>/<NAME>/issues --paginate` with `--jq` on `created_at`/`closed_at`, or the GitHub search API `is:issue created:<YYYY-MM>` / `closed:<YYYY-MM>`). The commit log shows what shipped; the issues show what users *hit* and what the maintainers are prioritising — the two together are the real activity picture. Skim: notable bugs opened (recurring pain points, hardware quirks), fixes closed that map to a commit, and any heavily-discussed feature request or design thread. Fold the user-facing ones into the summary below (a widely-reported bug that got fixed, a feature the community is pushing for); an issue with no user-facing outcome yet is still worth a one-line "watching:" note if it signals a direction. Don't list every issue — surface the few that matter, the same bar as the commit summary. | ||
| > 2. **Split a month at any release boundary — but only if the release was cut from the branch you're summarising.** If a *versioned* release was published mid-month (check `git for-each-ref refs/tags` / the GitHub releases API; ignore rolling tags like `nightly` and prereleases), AND the tag is an ancestor of the digest branch (`git merge-base --is-ancestor <tag> <branch>`), split that month at the release date into `## <Month Year> (up to v<X>)` / `## <Month Year> (post-v<X>)`. If the tag is NOT an ancestor (the project cuts releases from a separate release branch — e.g. upstream WLED tags off `0_15`/release branches, not `main`), do NOT split: keep the month whole and just note which release shipped that month as context, since the trunk you're summarising feeds future releases rather than being the release line. Whole months with no in-branch release stay one section. | ||
| > 3. Write an **end-user-readable** summary: what changed that a *user of the library* would notice or care about — new features, new hardware/platform support, notable fixes, breaking changes. Skip internal refactors, CI, test-only, and dependency bumps unless they affect users. | ||
| > 4. Format as **short bullet points**, each one line, plainest language, minimal jargon. Group only if there's a natural split (e.g. "New" / "Fixed"); otherwise a flat list. | ||
| > 5. Add it as a `## <MONTH YEAR>` section to `docs/history/<NAME>.md`, newest month on top. Don't editorialise or compare to projectMM — just report what they shipped. | ||
| > 6. State the commit range / count summarised so the digest is auditable. | ||
| > 6. State the commit range / count **and the issue query** summarised so the digest is auditable. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers
sed -n '55,85p' docs/history/README.md | cat -n
# Inspect whether the repository documents GitHub issues API usage elsewhere
rg -n "gh api repos/<owner>/<NAME>/issues|pull_request|select\\(has\\(\"pull_request\"\\) \\| not\\)" docs/history README.md . || trueRepository: MoonModules/projectMM
Length of output: 6179
🌐 Web query:
GitHub REST API issues endpoint includes pull requests documentation pull_request field
💡 Result:
Yes, the GitHub REST API "Issues" endpoints include a pull_request field in the response [1][2]. Because GitHub's REST API treats every pull request as an issue (though not every issue is a pull request), the "Issues" endpoints may return both issues and pull requests [1][3]. You can programmatically identify if an issue object is actually a pull request by checking for the presence of the pull_request key [1][4]. Important technical considerations include: * Identity: The id returned for a pull request by an "Issues" endpoint is the issue ID, not the pull request ID [1][2]. To obtain the specific pull request ID, you must use the "List pull requests" endpoint [1][5]. * Compatibility: Pull requests are technically a type of issue, so actions such as managing assignees, labels, and milestones are handled via the "Issues" API endpoints rather than the "Pull Requests" endpoints [6]. * Event Objects: This pattern extends to other endpoints, such as Issue Events and Timeline Events, where the pull_request property will exist within the issue object if the event relates to a pull request [4].
Citations:
- 1: https://docs.github.com/en/rest/issues/issues
- 2: https://docs.github.com/enterprise-server@3.8/rest/issues/issues
- 3: [Schema Inaccuracy] Fetching Issues, redirects to Pull Requests instead github/rest-api-description#3506
- 4: https://docs.github.com/en/rest/using-the-rest-api/issue-event-types
- 5: https://docs.github.com/enterprise-server@2.21/rest/reference/issues
- 6: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2026-03-10
Exclude pull requests from the issue sweep.
gh api repos/<owner>/<NAME>/issues --paginate includes PRs, so this step will overcount unless it filters them out.
♻️ Proposed fix
-> 1b. **Also investigate the issues over that month** (`gh api repos/<owner>/<NAME>/issues --paginate` with `--jq` on `created_at`/`closed_at`, or the GitHub search API `is:issue created:<YYYY-MM>` / `closed:<YYYY-MM>`).
+> 1b. **Also investigate the issues over that month** (`gh api repos/<owner>/<NAME>/issues --paginate` with `--jq` and `select(has("pull_request") | not)` to drop PRs, or the GitHub search API `is:issue created:<YYYY-MM>` / `closed:<YYYY-MM>`).📝 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
| > 1b. **Also investigate the issues over that month** (`gh api repos/<owner>/<NAME>/issues --paginate` with `--jq` on `created_at`/`closed_at`, or the GitHub search API `is:issue created:<YYYY-MM>` / `closed:<YYYY-MM>`). The commit log shows what shipped; the issues show what users *hit* and what the maintainers are prioritising — the two together are the real activity picture. Skim: notable bugs opened (recurring pain points, hardware quirks), fixes closed that map to a commit, and any heavily-discussed feature request or design thread. Fold the user-facing ones into the summary below (a widely-reported bug that got fixed, a feature the community is pushing for); an issue with no user-facing outcome yet is still worth a one-line "watching:" note if it signals a direction. Don't list every issue — surface the few that matter, the same bar as the commit summary. | |
| > 2. **Split a month at any release boundary — but only if the release was cut from the branch you're summarising.** If a *versioned* release was published mid-month (check `git for-each-ref refs/tags` / the GitHub releases API; ignore rolling tags like `nightly` and prereleases), AND the tag is an ancestor of the digest branch (`git merge-base --is-ancestor <tag> <branch>`), split that month at the release date into `## <Month Year> (up to v<X>)` / `## <Month Year> (post-v<X>)`. If the tag is NOT an ancestor (the project cuts releases from a separate release branch — e.g. upstream WLED tags off `0_15`/release branches, not `main`), do NOT split: keep the month whole and just note which release shipped that month as context, since the trunk you're summarising feeds future releases rather than being the release line. Whole months with no in-branch release stay one section. | |
| > 3. Write an **end-user-readable** summary: what changed that a *user of the library* would notice or care about — new features, new hardware/platform support, notable fixes, breaking changes. Skip internal refactors, CI, test-only, and dependency bumps unless they affect users. | |
| > 4. Format as **short bullet points**, each one line, plainest language, minimal jargon. Group only if there's a natural split (e.g. "New" / "Fixed"); otherwise a flat list. | |
| > 5. Add it as a `## <MONTH YEAR>` section to `docs/history/<NAME>.md`, newest month on top. Don't editorialise or compare to projectMM — just report what they shipped. | |
| > 6. State the commit range / count summarised so the digest is auditable. | |
| > 6. State the commit range / count **and the issue query** summarised so the digest is auditable. | |
| > 1b. **Also investigate the issues over that month** (`gh api repos/<owner>/<NAME>/issues --paginate` with `--jq` and `select(has("pull_request") | not)` to drop PRs, or the GitHub search API `is:issue created:<YYYY-MM>` / `closed:<YYYY-MM>`). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/README.md` around lines 68 - 73, The issue sweep in
docs/history/README.md can overcount because gh api repos/<owner>/<NAME>/issues
returns pull requests too. Update the guidance in the “investigate the issues”
step to explicitly filter out PRs (for example by checking for the pull_request
field or otherwise excluding them) before summarizing issues, so the issue query
matches only real issues.
Comment on lines
+32
to
+40
| // Accumulate phase incrementally — changing BPM doesn't cause a jump. phase_ holds the RAW | ||
| // numerator (dt·bpm·w·64) and the /60000 divide runs only at the sampling point below; dividing | ||
| // per tick would truncate sub-unit progress to zero on a fast board (short dt) or small grid, | ||
| // stalling the field. Factor 64 tuned so 60 BPM at 128-wide gives smooth motion. | ||
| uint32_t now = elapsed(); | ||
| uint32_t dt = now - lastElapsed_; | ||
| lastElapsed_ = now; | ||
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64 / 60000; | ||
| uint32_t t = static_cast<uint32_t>(phase_); | ||
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64; | ||
| uint32_t t = static_cast<uint32_t>(phase_ / 60000); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Seed lastElapsed_ on first tick to avoid a startup phase jump.
lastElapsed_ starts at 0, so the first call computes dt = elapsed() - 0, i.e. the full device uptime at the moment this effect starts. That gets multiplied into phase_ (dt * bpm * w * 64), causing the noise field to jump to an arbitrary offset on its very first frame instead of starting at t = 0. WaveEffect.h in this same PR fixes the identical pattern with a started_ guard — apply the same fix here for consistency.
As per coding guidelines, "Effects must run correctly at every grid size and tick rate, including 0×0×0, and must not truncate animation math to zero on fast devices."
🐛 Proposed fix
private:
uint64_t phase_ = 0;
uint32_t lastElapsed_ = 0;
+ bool started_ = false; uint32_t now = elapsed();
- uint32_t dt = now - lastElapsed_;
+ if (!started_) { lastElapsed_ = now; started_ = true; }
+ uint32_t dt = now - lastElapsed_;
lastElapsed_ = now;
phase_ += static_cast<uint64_t>(dt) * bpm * w * 64;
uint32_t t = static_cast<uint32_t>(phase_ / 60000);📝 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
| // Accumulate phase incrementally — changing BPM doesn't cause a jump. phase_ holds the RAW | |
| // numerator (dt·bpm·w·64) and the /60000 divide runs only at the sampling point below; dividing | |
| // per tick would truncate sub-unit progress to zero on a fast board (short dt) or small grid, | |
| // stalling the field. Factor 64 tuned so 60 BPM at 128-wide gives smooth motion. | |
| uint32_t now = elapsed(); | |
| uint32_t dt = now - lastElapsed_; | |
| lastElapsed_ = now; | |
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64 / 60000; | |
| uint32_t t = static_cast<uint32_t>(phase_); | |
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64; | |
| uint32_t t = static_cast<uint32_t>(phase_ / 60000); | |
| private: | |
| uint64_t phase_ = 0; | |
| uint32_t lastElapsed_ = 0; | |
| bool started_ = false; |
Suggested change
| // Accumulate phase incrementally — changing BPM doesn't cause a jump. phase_ holds the RAW | |
| // numerator (dt·bpm·w·64) and the /60000 divide runs only at the sampling point below; dividing | |
| // per tick would truncate sub-unit progress to zero on a fast board (short dt) or small grid, | |
| // stalling the field. Factor 64 tuned so 60 BPM at 128-wide gives smooth motion. | |
| uint32_t now = elapsed(); | |
| uint32_t dt = now - lastElapsed_; | |
| lastElapsed_ = now; | |
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64 / 60000; | |
| uint32_t t = static_cast<uint32_t>(phase_); | |
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64; | |
| uint32_t t = static_cast<uint32_t>(phase_ / 60000); | |
| // Accumulate phase incrementally — changing BPM doesn't cause a jump. phase_ holds the RAW | |
| // numerator (dt·bpm·w·64) and the /60000 divide runs only at the sampling point below; dividing | |
| // per tick would truncate sub-unit progress to zero on a fast board (short dt) or small grid, | |
| // stalling the field. Factor 64 tuned so 60 BPM at 128-wide gives smooth motion. | |
| uint32_t now = elapsed(); | |
| if (!started_) { lastElapsed_ = now; started_ = true; } | |
| uint32_t dt = now - lastElapsed_; | |
| lastElapsed_ = now; | |
| phase_ += static_cast<uint64_t>(dt) * bpm * w * 64; | |
| uint32_t t = static_cast<uint32_t>(phase_ / 60000); |
🤖 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/NoiseEffect.h` around lines 32 - 40, Seed lastElapsed_ on
the first update in NoiseEffect so the initial dt does not include full device
uptime and the noise phase starts at t = 0. Add the same one-time guard used in
WaveEffect (the started_/first-tick pattern) around the elapsed()/dt
accumulation in the NoiseEffect update path, then initialize lastElapsed_ from
the current elapsed value before applying phase_ += dt * bpm * w * 64.
Source: Coding guidelines
…bbit batch Fold the PR-merge gate-list outputs and a CodeRabbit review round into one commit: carry-forward lessons to decisions.md, a render-loop note in performance.md, three effects moved off private LCGs onto the shared mm::Random8 (the Reviewer's must-fix), WaveEffect's noise onto the shared inoise8, a NoiseEffect first-frame guard, and moondeck/check_specs/digest fixes. KPI: 16384lights | PC:465KB | tick:485/251/728/52/6/785/175/49/60/471/344/48/3/1/102us(FPS:2061/3984/1373/19230/166666/1273/5714/20408/16666/2123/2906/20833/333333/1000000/9803) | tick:5343us(FPS:187) | heap:31526KB | src:166(30962) | test:91(13423) | lizard:113w Light domain: - FireEffect / ParticlesEffect / RingsEffect: replaced each effect's private LCG (`rngState_` + a hand-rolled `rand8()`) with a shared `mm::Random8` member — the primitive this cycle added for exactly this, so the three copies are the duplication the Reviewer flagged. `rand8()` becomes a one-line adapter over `rng_.next8()`, call sites unchanged. - WaveEffect: the "Noise" waveform uses the shared `inoise8` (1-arg value noise from core/noise.h); dropped the private `valueNoise`/`hash` helpers that reimplemented it. - NoiseEffect: seed `lastElapsed_` on the first tick (the `started_` guard WaveEffect uses) so the field starts at phase 0 instead of jumping by the whole device uptime. Scripts / MoonDeck: - moondeck.py: `_serve_doc_asset` URL-decodes the request path (a doc image with a space is `Hue%20driver.png` in the URL — without decoding the file lookup sought a literal `%20` and 404'd). Both `<img>` render paths now escape every attribute value (src/width/style/alt), not just alt. - check_specs.py: corrected the stale `CONSOLIDATED_PAGES` comment — every `*Layout`/`*Driver` module consolidates by type-name suffix (CarLights/Cube/Panel/Ring/…), not a hand-picked leaf subset. Docs / CI: - decisions.md: two carried-forward lessons — a static "current instance" pointer needs self-election in loop(), not just claim/vacate (the two-mic AudioModule case); and an Effect-role module is not guaranteed to be an EffectBase (the DemoReel per-frame downcast SIGBUS — use the factory's probed dim, not a cross-tree static_cast under -fno-rtti). - performance.md: a render-loop-model note — the buffer persists frame-to-frame (no per-frame clear) and the collected fade applies one pass per frame; the scenario `observed` blocks were re-measured against this model. - history digests: bold **New**/**Fixed**/**Watching** labels in the FastLED / NightDriverStrip / WLED June sections (matching the other months); ESPLiveScript June now states all ~30 version branches were quiet (verified), not just `main`. - history/README.md digest prompt: the issue-sweep step filters out pull requests (the REST /issues endpoint returns PRs too) via `select(.pull_request == null)` or the `is:issue` search API. Reviews: - 👾 Reviewer (PR-merge, whole branch diff): MERGE-WITH-FIXES — the one must-fix (FireEffect + ParticlesEffect private LCGs → mm::Random8) fixed, plus RingsEffect swept for consistency. Accepted-with-reason: ParticlesEffect keeps its 512-byte inline `particles_[64]` (well under the P4 stack-probe bootloop threshold; sizeof MoonModule=72 verified on all boards). - 🐇 NoiseEffect first-frame dt, WaveEffect inoise8 dedup, moondeck.py %20 doc-asset + attr escaping, check_specs.py stale comment, FastLED/NightDriver/WLED digest labels, ESPLiveScript version-branch coverage, README issue-sweep PR filter — fixed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full MoonLight-migration cycle (10 commits) plus the render-loop rework it drove. Originally opened as "Add HueDriver + WaveEffect"; the branch grew to cover the effect library, a buffer-persistence model change, driver-doc consolidation, and per-module attribution. Every commit was gated and carries a KPI line; CodeRabbit batches were processed along the way.
Effect / light domain
EffectBase+ shared primitives (not traced from source). 60 palettes, a demo-reel showcase, text/font draw primitives.draw::primitive library (line/glyph/fade/blur/blendPixel/addPixel).Render-loop model (the significant architecture change)
Layer::loop()no longer clears it (the FastLED/WLED/MoonLight convention). Effects own their background: full-grid overwrites, trail effects fade, sparse effects self-clear. This fixed ~13 effects whose fade/scroll/read-prior behaviour a per-frame clear had silently broken (FreqMatrix lit only one row,fadecontrols did nothing).Layer::fadeToBlackByMINs the requested amounts and applies one buffer pass per frame (MoonLight'sVirtualLayermodel), so N fading effects on a layer cost one pass, not N.Layer::extrudefans a D1/D2 effect across the unused axes.Core
AudioFrame(16 bands + raw/smoothed level + peak); asimulatecontrol (synth music/sweep) so audio effects run mic-less; two-micactive_re-election.sdkDatecontrol removed (build date already on the firmware card).Docs / drivers
.hand the.md.Tests / gates
unit_Layer_persistence(persistence + collected-fade MIN + reset),unit_PinwheelModifier(1D petal-axis), two-mic AudioModule re-election, plus updated extrude/zero-grid/render coverage.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation