Add 10 stateless and stateful effects

[ewowi]

Summary

Ten new effects for the light pipeline — the effects-only half of the more-effects work.

• Stateless (zero-heap): Plasma, PlasmaPalette, Metaballs, Checkerboard, Spiral, Ripples, GlowParticles, LavaLamp
• Stateful (own dynamic buffer, freed when disabled): Fire (heat grid), Particles (trail buffer)

Each effect ships a module spec in docs/moonmodules/light/effects/ and a doctest case. color.h gains palette helpers shared by the palette-driven effects. main.cpp registers all ten via ModuleFactory; the startup pipeline keeps the single default Noise effect.

Effects use the existing MoonModule::enabled() system control — no parallel enabled field. Fire/Particles free their buffer when disabled.

Not included — dynamic effect management

The /api/types + /api/modules/<n>/move endpoints and the add-effect picker / reorder UI are intentionally excluded from this PR. They land separately as their own change.

Test plan

• Desktop build clean (-Werror)
• 104 unit-test cases pass (23 new effect cases)
• 8 in-process scenarios pass
• Platform boundary + spec check (20/20) PASS
• CodeRabbit review

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR extends the LED effect pipeline by adding 10 new visual effects (Plasma, Fire, Metaballs, Particles, Spiral, Checkerboard, Lava Lamp, Glow Particles, Ripples, PlasmaPalette), introducing integer-only math utilities for fast rendering, enabling dynamic effect reordering/addition/deletion via HTTP APIs and UI controls, and updating project documentation.

Changes

Effect Pipeline Expansion

Layer / File(s)	Summary
Integer Math Utilities & Base Effect Controls src/core/color.h, src/light/EffectBase.h, src/light/Layer.h, src/light/NoiseEffect.h, src/light/RainbowEffect.h	New sin8_lut, sin8, cos8, atan2_8, dist8 constexpr functions in color.h for fast integer math. EffectBase adds public enabled boolean with control registration. Layer::loop() reverses child iteration for correct top-to-bottom draw order. NoiseEffect and RainbowEffect call EffectBase::onBuildControls() to inherit base controls.
Stateless Effects: Plasma, Spiral, Checkerboard, Lava Lamp, PlasmaPalette src/light/PlasmaEffect.h, src/light/PlasmaPaletteEffect.h, src/light/SpiralEffect.h, src/light/CheckerboardEffect.h, src/light/LavaLampEffect.h	Five effects with no persistent buffers: PlasmaEffect (four-sine waves with HSV output), PlasmaPaletteEffect (plasma with fire/ocean palette lookup), SpiralEffect (rotating spiral using atan2_8 and dist8), CheckerboardEffect (animated checkerboard with two HSV hues), and LavaLampEffect (three slow blobs with fixed palette). Each maintains internal phase state and renders via elapsed-time-based animation.
Fire & Metaballs Effects src/light/FireEffect.h, src/light/MetaballsEffect.h	FireEffect allocates a heat buffer (sized to LED count), simulates per-frame cooling/upward diffusion, injects random sparks at the bottom, and renders heat-to-RGB via an internal palette with optional hue shift. MetaballsEffect computes four moving ball centers using sin8 with per-ball phases, renders per-pixel via inverse-distance field summation, and outputs HSV-to-RGB.
Particle & Glow Particle Effects src/light/ParticlesEffect.h, src/light/GlowParticlesEffect.h	ParticlesEffect maintains a persistent per-pixel trail buffer, lazily initializes 64 fixed-point particles with RNG, advances particles with boundary-bounce physics, fades trail each frame, and copies final trail to layer buffer. GlowParticlesEffect manages 8 particles with an inverse-squared metaball field per pixel for a soft-glow effect.
Ripples Effect src/light/RipplesEffect.h	Maintains 8 ripple instances with centers, radii, and hues; advances radii over time and respawns ripples at random positions when they exceed visible bounds; accumulates per-ripple contributions per pixel with thickness falloff and age-based fade.
Module Reordering API src/core/MoonModule.h, src/core/HttpServerModule.h	MoonModule adds moveChild(child, delta) to swap child pointers in children_ array. HttpServerModule adds GET /api/types endpoint (returns JSON array of registered module types) and POST /api/modules/{Name}/move handler (validates and applies reordering via parent->moveChild()).
UI Effect Type Management src/ui/app.js, src/ui/style.css	app.js fetches /api/types alongside /api/state on load, stores available effect types. renderCards() detects layer containers, inserts "Add effect" picker card, and renders effect children with move/delete buttons. New helpers (createAddEffectCard, addModule, deleteModule, moveModule) wire UI to HTTP endpoints. style.css adds flex layout to card headers, plus .card-actions, .card-btn, .add-card styling.
Effect Registration & Defaults src/main.cpp, src/light/ArtNetSendDriver.h, src/light/PreviewDriver.h	main.cpp includes all 10 new effect headers and registers them with ModuleFactory; changes default initial effect from NoiseEffect to PlasmaEffect. ArtNetSendDriver and PreviewDriver increase fps control maximum from 60/120 to 255.
Effect Documentation & Tests docs/moonmodules/light/*.md, docs/testing.md, test/CMakeLists.txt, test/test_*.cpp	Markdown documentation for all 10 effects, plus EffectBase.md and testing.md subsections. Five test files validate non-zero RGB output, spatial variation, and buffer allocation/freeing for stateful effects (Fire, Particles, Plasma, Metaballs) and stateless effects (Checkerboard, Spiral, PlasmaPalette, GlowParticles, RipplesEffect, LavaLamp).
Project Status Assessment docs/history/assessment-2026-05.md	Objective assessment covering pipeline completeness, missing items, completion estimates, technical strengths/debt, dimension scoring, historical comparison, and prioritization.

---

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

---

Possibly related PRs

• ewowi/projectMM-v3#1: Foundational pipeline PR that introduced the HTTP server and MoonModule architecture; this PR builds directly on those APIs by extending HttpServerModule with effect-type discovery and move endpoints, and leveraging MoonModule for child reordering.

---

🐇 Ten effects bloom bright,
With swirling sine and blazing heat,
Math packed tight, pixels light,
A dancing show—now move and delete! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.35% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: adding 10 new effects to the system.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/more-effects

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ewowi]

@coderabbitai, regarding

Rate limit exceeded

it says wait 56 min. but how much is it now? As I need a review from you

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check recent comments on the PR for rate limit info
gh pr view 3 --repo ewowi/projectMM-v3 --comments --json comments | jq '.comments[-5:] | .[] | {author: .author.login, createdAt: .createdAt, body: .body[0:300]}'

Length of output: 595

---

@ewowi The rate limit was triggered at 13:44 UTC with a 56-minute cooldown, meaning it expires at approximately 14:40 UTC. When you posted your question at 14:22 UTC, there were roughly ~17 minutes remaining.

The window should be clear (or very close to it) by now — I'll kick off the review for you! 🐇

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[ewowi]

@coderabbitai, please review this pr and provide a prompt for all review comments with AI agents

[coderabbitai]

@ewowi Sure! I'll kick off a full review of this PR right away. Once the review is complete, each comment will include an AI agent prompt that you can use to implement the suggested fix directly. 🐇

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 15

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/light/NoiseEffect.h (1)

10-15: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the scale range comment to match the control bounds.

Line 10 says 1-32, but Line 15 now exposes 1-255.

Suggested fix

-    uint8_t scale = 4;  // spatial frequency (1-32)
+    uint8_t scale = 4;  // spatial frequency (1-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/NoiseEffect.h` around lines 10 - 15, The comment for the uint8_t
field "scale" is out of sync with the control bounds: update the comment on the
"scale" member (declared as uint8_t scale = 4) to reflect the actual range used
by controls_.addUint8 in onBuildControls (1-255) so the inline documentation
matches the control limits; locate the scale declaration and the onBuildControls
method to change the comment from "spatial frequency (1-32)" to "spatial
frequency (1-255)" (or a wording that indicates the 1–255 range).

🤖 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/assessment-2026-05.md`:
- Line 26: Update the assessment to match this PR’s post-merge state (or clearly
label it as a pre-PR baseline): replace the Effects entry that currently lists
only "Rainbow, Noise" with the full set added by layers 2–5 (Plasma, Metaballs,
Fire, Particles, Checkerboard, Spiral, PlasmaPalette, Ripples, GlowParticles,
LavaLamp), mark that RipplesEffect is implemented (layer 5), change the "UI type
picker + dynamic effect switching" missing note to present since layer 7
implements the UI and wiring for /api/types, and remove/adjust the claim that
GET /api/types does not exist because HttpServerModule (layer 6) adds that
endpoint; ensure references to RipplesEffect, HttpServerModule, GET /api/types,
and the layer numbers are updated or add a “pre-PR baseline” header so readers
know the snapshot’s intent.

In `@src/core/color.h`:
- Around line 65-78: atan2_8 produces wrong quadrant offsets: fix by computing
the 4 quadrant base (0,64,128,192) from the signs of x and y (use (x<0) and
(y<0)), keep the octant swap logic but make offset represent 0 or 32 (45°
octant), compute the intra-octant step b in 0..31 (use y*32/x, not *64/x) and
return quadrant_base + offset + b; update variables r, offset, and b in atan2_8
accordingly so final value maps 0..255 with +X≈0, +Y≈64, -X≈128, -Y≈192.

In `@src/core/HttpServerModule.h`:
- Around line 458-463: The loop emitting the /api/types JSON array uses the loop
index to decide comma placement but skips null names, which can produce a
leading comma; change the emission logic in the loop that iterates
ModuleFactory::typeCount() / calls ModuleFactory::typeName(i) to track a bool
first = true (outside the loop), and when emitting each non-null name write a
comma only if first is false, then set first = false after emitting; keep using
jsonBuf_, pos and JSON_BUF_SIZE and the existing snprintf/bounds checks when
appending.
- Around line 146-155: The current check using std::strstr(path + 13, "/move")
matches any occurrence of "/move" and can accept malformed paths; change the
condition to require "/move" to be an exact suffix (or followed only by
query/fragment) before calling handleMoveModule. Specifically, find the existing
strstr usage and replace it with a check that locates the "/move" token (e.g.,
via std::strstr or std::strcmp on the found pointer) and then assert that the
characters after "/move" are only end-of-string, '?' or '#' (reject
"/move/extra" etc.), then proceed to extract nameBuf and call
handleMoveModule(nameBuf, body).

In `@src/light/FireEffect.h`:
- Line 5: Remove the direct platform dependency from FireEffect.h by deleting
`#include` "platform/platform.h" and any direct calls to platform allocation APIs
inside the FireEffect class; instead inject or call an abstract allocator
interface (e.g., IMemoryAllocator) via FireEffect’s constructor or an initialize
method and replace platformAlloc/platformFree usages with
IMemoryAllocator::allocate/ free (or equivalent) calls; keep only the allocator
interface declaration visible to src/light (reference symbols: class FireEffect,
FireEffect::FireEffect(...), FireEffect::initialize(...), and any
platformAlloc/platformFree calls) and move the concrete platform include and
implementation of the allocator into the platform-specific module so FireEffect
only depends on the allocator abstraction.

In `@src/light/GlowParticlesEffect.h`:
- Around line 37-54: The first-frame dt can be huge because lastElapsed_ is
initialized to 0; change the delta calculation so that if lastElapsed_ is 0 (or
otherwise indicates “no prior sample”) you treat dt as 0 instead of
now-lastElapsed_. In practice, call elapsed() into now, then compute dt =
(lastElapsed_ == 0 ? 0 : now - lastElapsed_), then set lastElapsed_ = now; this
prevents the huge initial sx/sy updates in the particle update loop that touches
particles_ and uses p.vx/p.vy, speed, and dt.

In `@src/light/LavaLampEffect.h`:
- Around line 49-63: The radius is currently used as an absolute pixel value
when computing r2, causing saturation on small panels; adjust radius before
computing r2 by scaling it to the panel size (e.g., multiply radius by a factor
derived from w and h such as min(w,h)/reference_size or normalize to [0,1] then
multiply by min(w,h)) so that r2 = (scaledRadius * scaledRadius) uses the
panel-aware value; update the calculation that sets r2 (and any use of radius in
the inner loop that influences d2/field/scaled) so that NUM_BLOBS, bx/by,
intensity and palette_ logic remain unchanged but operate on the normalized
radius to restore spatial variation.

In `@src/light/ParticlesEffect.h`:
- Around line 70-95: The particle motion is frame-rate dependent because sx/sy
are computed as (p.vx * speed) >> 6 with no time factor; update ParticlesEffect
to use elapsed milliseconds (dt) like GlowParticlesEffect::loop(): add/maintain
a lastElapsed_ timestamp, compute dt = elapsed - lastElapsed_, multiply the
velocity term by dt (so displacement = p.vx * speed * dt * scaling_factor rather
than just shifting), use that to compute sx/sy and update p.x/p.y, and update
lastElapsed_ after the loop so the slider and motion are resolution-independent;
reference symbols: sx, sy, p.vx, speed, p.x/p.y, particles_, lastElapsed_, and
loop().

In `@src/ui/app.js`:
- Around line 230-252: addModule, deleteModule and moveModule currently ignore
fetch responses and errors so UI refreshes even when the server operation fails;
update each function to await the fetch response, check response.ok (and parse
error body when not ok), throw or handle errors (e.g., log/display) and only
call refreshAndRender if the request succeeded, and wrap the fetch in try/catch
to handle network errors and prevent silent failures.

In `@test/test_fire.cpp`:
- Around line 72-74: Replace the direct effect-level manipulation (setting
fire.enabled = false and calling fire.onAllocateMemory()) with the layer-level
flow: locate the Layer that owns the fire effect and call its
disable/setEnabled(false) API so the Layer handles reallocation, then trigger
the Layer's allocation/reallocation entry point (e.g., Layer::onAllocateMemory
or Layer::reallocateMemory) and assert fire.dynamicBytes() == 0; do not call
fire.onAllocateMemory() directly. Ensure you reference the fire.enabled and
fire.onAllocateMemory() occurrences to remove them and use the Layer methods
instead.

In `@test/test_metaballs.cpp`:
- Around line 51-55: The test dereferences the raw pointer returned by
layer.buffer().data() (variable data) without checking for null; add a null
precondition by inserting a REQUIRE(data != nullptr) immediately after obtaining
data (before the uint8_t r0 = ... line) so a failed allocation is reported by
doctest instead of crashing the test; ensure the check references the same
expression (data) used later in the comparisons.

In `@test/test_particles.cpp`:
- Around line 68-70: The test currently disables particles and calls
particles.onAllocateMemory() directly, which bypasses the Layer-level
disable/reallocation contract; instead, after setting particles.enabled = false
you should invoke the owning Layer's reallocation API (i.e., trigger the Layer
reallocation flow that the effect contract uses rather than calling
particles.onAllocateMemory() directly) so the Layer performs the proper
disable/reallocate behavior, then assert particles.dynamicBytes() == 0; replace
the direct onAllocateMemory() call with the Layer reallocation call that owns
this particle set.

In `@test/test_plasma.cpp`:
- Around line 52-56: The test directly indexes layer.buffer().data() and can
crash if the buffer is null or too small; before accessing data[0] and
data[lastIdx+2] add REQUIRE checks that the buffer pointer is non-null and that
layer.buffer().size() (or equivalent size accessor on layer.buffer()) is greater
than lastIdx+2 so the accesses are safe, then proceed with the existing uint8_t
r0/g0/b0, r1/g1/b1 and CHECK; reference the local variables data, lastIdx and
the CHECK assertion when adding these precondition REQUIREs.

In `@test/test_stateless_effects.cpp`:
- Around line 58-74: The tests generated by STATELESS_EFFECT_TEST must assert
the stateless contract by checking effect.dynamicBytes() == 0; add an assertion
after allocation/loop in both generated TEST_CASEs so that for each mm::EFFECT
created (inside Ctx ctx(...); mm::EFFECT effect; ctx.layer.addChild(&effect);
ctx.layer.onAllocateMemory(); ctx.layer.loop();) you call
CHECK(effect.dynamicBytes() == 0) to ensure heap-free behavior; update the macro
STATELESS_EFFECT_TEST (and the same pattern in the other tests covering lines
76-118) to include this assertion immediately after onAllocateMemory()/loop().
- Around line 92-94: The test currently only compares a single channel (data[0]
!= data[4 * 3]) which is brittle; change it to assert the full RGB triplet
differs by comparing the three color bytes for the two pixels retrieved from
ctx.layer.buffer().data() (the local variable data). Replace the single-channel
check with a full-pixel comparison (e.g., use std::memcmp(&data[0], &data[4 *
3], 3) != 0 or explicitly check data[0..2] vs data[4*3..4*3+2]) so the test
verifies RGB-level spatial variation as specified.

---

Outside diff comments:
In `@src/light/NoiseEffect.h`:
- Around line 10-15: The comment for the uint8_t field "scale" is out of sync
with the control bounds: update the comment on the "scale" member (declared as
uint8_t scale = 4) to reflect the actual range used by controls_.addUint8 in
onBuildControls (1-255) so the inline documentation matches the control limits;
locate the scale declaration and the onBuildControls method to change the
comment from "spatial frequency (1-32)" to "spatial frequency (1-255)" (or a
wording that indicates the 1–255 range).

🪄 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: 8327ded8-cca9-4eed-ab2f-eed2ca9b1261

📥 Commits

Reviewing files that changed from the base of the PR and between 2a116af and 9e28ef8.

📒 Files selected for processing (41)

• docs/history/assessment-2026-05.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/effects/CheckerboardEffect.md
• docs/moonmodules/light/effects/FireEffect.md
• docs/moonmodules/light/effects/GlowParticlesEffect.md
• docs/moonmodules/light/effects/LavaLampEffect.md
• docs/moonmodules/light/effects/MetaballsEffect.md
• docs/moonmodules/light/effects/ParticlesEffect.md
• docs/moonmodules/light/effects/PlasmaEffect.md
• docs/moonmodules/light/effects/PlasmaPaletteEffect.md
• docs/moonmodules/light/effects/RipplesEffect.md
• docs/moonmodules/light/effects/SpiralEffect.md
• docs/testing.md
• src/core/HttpServerModule.h
• src/core/MoonModule.h
• src/core/color.h
• src/light/ArtNetSendDriver.h
• src/light/CheckerboardEffect.h
• src/light/EffectBase.h
• src/light/FireEffect.h
• src/light/GlowParticlesEffect.h
• src/light/LavaLampEffect.h
• src/light/Layer.h
• src/light/MetaballsEffect.h
• src/light/NoiseEffect.h
• src/light/ParticlesEffect.h
• src/light/PlasmaEffect.h
• src/light/PlasmaPaletteEffect.h
• src/light/PreviewDriver.h
• src/light/RainbowEffect.h
• src/light/RipplesEffect.h
• src/light/SpiralEffect.h
• src/main.cpp
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/test_fire.cpp
• test/test_metaballs.cpp
• test/test_particles.cpp
• test/test_plasma.cpp
• test/test_stateless_effects.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Assessment is inconsistent with this PR's changes.

This assessment document describes features as missing that are actually being delivered in this PR:

• Line 26 lists only "Rainbow, Noise" effects, but this PR adds 10 new effects (Plasma, Metaballs, Fire, Particles, Checkerboard, Spiral, PlasmaPalette, Ripples, GlowParticles, LavaLamp) per layers 2–5.
• Line 44 claims "UI type picker + dynamic effect switching" is missing, but layer 7 implements exactly this (UI fetches /api/types, adds picker card, wire add/delete/move actions).
• Line 47 lists "Ripples" as missing, but RipplesEffect is added in layer 5.
• Line 102 states "/api/types does not exist yet," but layer 6 adds GET /api/types in HttpServerModule.

Since this document is part of the PR and dated the same month, readers will assume it reflects the post-merge state, making these claims misleading.

Recommendation: Either update the assessment to reflect the state after this PR, or clearly label it as a "pre-PR baseline" snapshot for historical reference.

Also applies to: 44-44, 47-47, 102-102

🤖 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/assessment-2026-05.md` at line 26, Update the assessment to
match this PR’s post-merge state (or clearly label it as a pre-PR baseline):
replace the Effects entry that currently lists only "Rainbow, Noise" with the
full set added by layers 2–5 (Plasma, Metaballs, Fire, Particles, Checkerboard,
Spiral, PlasmaPalette, Ripples, GlowParticles, LavaLamp), mark that
RipplesEffect is implemented (layer 5), change the "UI type picker + dynamic
effect switching" missing note to present since layer 7 implements the UI and
wiring for /api/types, and remove/adjust the claim that GET /api/types does not
exist because HttpServerModule (layer 6) adds that endpoint; ensure references
to RipplesEffect, HttpServerModule, GET /api/types, and the layer numbers are
updated or add a “pre-PR baseline” header so readers know the snapshot’s intent.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

atan2_8 currently maps key directions to wrong angles.

Line 66-78 does not produce a correct 0..255 full-circle mapping (e.g., (y=1,x=0) should be ~64, (y=0,x=-1) should be ~128). This will skew angle-based effects.

Suggested fix

-constexpr uint8_t atan2_8(int16_t y, int16_t x) {
-    uint8_t r = 0;
-    if (y < 0) { y = static_cast<int16_t>(-y); r = 0x80; }
-    if (x < 0) { x = static_cast<int16_t>(-x); r = static_cast<uint8_t>(r | 0x40); }
-    uint8_t offset = (x > y) ? 0 : 32;
-    if (x < y) {
-        int16_t t = y;
-        y = x;
-        x = t;
-    }
-    uint8_t b = (x == 0) ? 0 : static_cast<uint8_t>((static_cast<uint16_t>(y) * 64) / static_cast<uint16_t>(x));
-    return static_cast<uint8_t>(r + offset + b);
-}
+constexpr uint8_t atan2_8(int16_t y, int16_t x) {
+    if (x == 0 && y == 0) return 0;
+
+    const uint16_t ax = static_cast<uint16_t>(
+        x < 0 ? -static_cast<int32_t>(x) : static_cast<int32_t>(x));
+    const uint16_t ay = static_cast<uint16_t>(
+        y < 0 ? -static_cast<int32_t>(y) : static_cast<int32_t>(y));
+
+    uint8_t base;
+    if (ax >= ay) {
+        base = static_cast<uint8_t>((static_cast<uint32_t>(ay) * 32u) / (ax == 0 ? 1u : ax)); // 0..32
+    } else {
+        base = static_cast<uint8_t>(64u - (static_cast<uint32_t>(ax) * 32u) / ay); // 32..64
+    }
+
+    if (x >= 0) {
+        return (y >= 0) ? base : static_cast<uint8_t>(256u - base);
+    }
+    return (y >= 0) ? static_cast<uint8_t>(128u - base) : static_cast<uint8_t>(128u + base);
+}

🤖 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/color.h` around lines 65 - 78, atan2_8 produces wrong quadrant
offsets: fix by computing the 4 quadrant base (0,64,128,192) from the signs of x
and y (use (x<0) and (y<0)), keep the octant swap logic but make offset
represent 0 or 32 (45° octant), compute the intra-octant step b in 0..31 (use
y*32/x, not *64/x) and return quadrant_base + offset + b; update variables r,
offset, and b in atan2_8 accordingly so final value maps 0..255 with +X≈0,
+Y≈64, -X≈128, -Y≈192.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Tighten /move route matching to an exact suffix.

Current matching accepts any path containing "/move" after /api/modules/, so malformed paths can still trigger moves.

Suggested fix

-            } else if (std::strncmp(path, "/api/modules/", 13) == 0 &&
-                       std::strstr(path + 13, "/move") != nullptr && body) {
+            } else if (std::strncmp(path, "/api/modules/", 13) == 0 && body) {
+                const char* suffix = "/move";
+                const char* tail = path + std::strlen(path) - std::strlen(suffix);
+                if (tail >= path + 13 && std::strcmp(tail, suffix) == 0) {
                 // POST /api/modules/Name/move with body {"delta": -1|1}
                 char nameBuf[32] = {};
-                const char* slash = std::strstr(path + 13, "/move");
+                const char* slash = tail;
                 size_t nameLen = static_cast<size_t>(slash - (path + 13));
                 if (nameLen >= sizeof(nameBuf)) nameLen = sizeof(nameBuf) - 1;
                 std::memcpy(nameBuf, path + 13, nameLen);
                 nameBuf[nameLen] = 0;
                 handleMoveModule(conn, nameBuf, body);
+                } else {
+                    sendResponse(conn, 404, "text/plain", "Not found");
+                }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/HttpServerModule.h` around lines 146 - 155, The current check using
std::strstr(path + 13, "/move") matches any occurrence of "/move" and can accept
malformed paths; change the condition to require "/move" to be an exact suffix
(or followed only by query/fragment) before calling handleMoveModule.
Specifically, find the existing strstr usage and replace it with a check that
locates the "/move" token (e.g., via std::strstr or std::strcmp on the found
pointer) and then assert that the characters after "/move" are only
end-of-string, '?' or '#' (reject "/move/extra" etc.), then proceed to extract
nameBuf and call handleMoveModule(nameBuf, body).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use a first flag when emitting /api/types array entries.

Comma placement is tied to index, but null names are skipped. That can produce invalid JSON with a leading comma.

Suggested fix

     void serveTypes(platform::TcpConnection& conn) {
         int pos = std::snprintf(jsonBuf_, JSON_BUF_SIZE, "{\"types\":[");
+        bool first = true;
         for (uint8_t i = 0; i < ModuleFactory::typeCount(); i++) {
             const char* name = ModuleFactory::typeName(i);
             if (!name) continue;
             int n = std::snprintf(jsonBuf_ + pos, JSON_BUF_SIZE - pos,
-                "%s\"%s\"", (i == 0 ? "" : ","), name);
+                "%s\"%s\"", first ? "" : ",", name);
             if (n > 0 && static_cast<size_t>(pos + n) < JSON_BUF_SIZE) pos += n;
+            first = false;
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/HttpServerModule.h` around lines 458 - 463, The loop emitting the
/api/types JSON array uses the loop index to decide comma placement but skips
null names, which can produce a leading comma; change the emission logic in the
loop that iterates ModuleFactory::typeCount() / calls ModuleFactory::typeName(i)
to track a bool first = true (outside the loop), and when emitting each non-null
name write a comma only if first is false, then set first = false after
emitting; keep using jsonBuf_, pos and JSON_BUF_SIZE and the existing
snprintf/bounds checks when appending.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Keep platform-specific includes and API calls out of src/light.

This effect directly includes platform headers and calls platform allocation APIs from the light layer. Please route memory allocation through a non-platform abstraction owned outside src/light so platform coupling remains confined to src/platform/.

As per coding guidelines, src/**/*.{h,hpp,cpp,cc}: All #ifdef, platform-specific #include``s, and hardware API calls must live exclusively in src/platform/`.

Also applies to: 29-30, 114-114

🤖 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/FireEffect.h` at line 5, Remove the direct platform dependency from
FireEffect.h by deleting `#include` "platform/platform.h" and any direct calls to
platform allocation APIs inside the FireEffect class; instead inject or call an
abstract allocator interface (e.g., IMemoryAllocator) via FireEffect’s
constructor or an initialize method and replace platformAlloc/platformFree
usages with IMemoryAllocator::allocate/ free (or equivalent) calls; keep only
the allocator interface declaration visible to src/light (reference symbols:
class FireEffect, FireEffect::FireEffect(...), FireEffect::initialize(...), and
any platformAlloc/platformFree calls) and move the concrete platform include and
implementation of the allocator into the platform-specific module so FireEffect
only depends on the allocator abstraction.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a null precondition before dereferencing the buffer pointer.

Line 52 dereferences data without guarding for allocation failure. Add a REQUIRE(data != nullptr) so failures report cleanly instead of crashing.

Proposed fix

     auto* data = layer.buffer().data();
+    REQUIRE(data != nullptr);
     uint8_t r0 = data[0], g0 = data[1], b0 = data[2];

As per coding guidelines, "Unit and integration tests using doctest. Verify tests cover edge cases and match the specifications in docs/moonmodules/."

🧰 Tools

🪛 Cppcheck (2.20.0)

[style] 54-54: The function 'addText' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_metaballs.cpp` around lines 51 - 55, The test dereferences the raw
pointer returned by layer.buffer().data() (variable data) without checking for
null; add a null precondition by inserting a REQUIRE(data != nullptr)
immediately after obtaining data (before the uint8_t r0 = ... line) so a failed
allocation is reported by doctest instead of crashing the test; ensure the check
references the same expression (data) used later in the comparisons.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate disable memory release via Layer reallocation flow.

Line 69 calls particles.onAllocateMemory() directly, which skips the layer-level disable/reallocation behavior required by the effect contract.

Proposed fix

     particles.enabled = false;
-    particles.onAllocateMemory();
+    layer.onAllocateMemory();
     CHECK(particles.dynamicBytes() == 0);

As per coding guidelines, "Unit and integration tests using doctest. Verify tests cover edge cases and match the specifications in docs/moonmodules/."

🧰 Tools

🪛 Cppcheck (2.20.0)

[style] 69-69: The function 'removeChild' is never used.

(unusedFunction)

---

[style] 70-70: The function 'physicalLightCount' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_particles.cpp` around lines 68 - 70, The test currently disables
particles and calls particles.onAllocateMemory() directly, which bypasses the
Layer-level disable/reallocation contract; instead, after setting
particles.enabled = false you should invoke the owning Layer's reallocation API
(i.e., trigger the Layer reallocation flow that the effect contract uses rather
than calling particles.onAllocateMemory() directly) so the Layer performs the
proper disable/reallocate behavior, then assert particles.dynamicBytes() == 0;
replace the direct onAllocateMemory() call with the Layer reallocation call that
owns this particle set.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard buffer pointer/size before direct indexing.

This test can crash instead of failing cleanly if allocation/config regresses; add REQUIRE checks before data[0]/data[lastIdx] access.

Proposed fix

     layer.onAllocateMemory();
     layer.loop();

-    auto* data = layer.buffer().data();
+    auto& buf = layer.buffer();
+    REQUIRE(buf.data() != nullptr);
+    REQUIRE(buf.bytes() >= (16u * 16u * 3u));
+    auto* data = buf.data();
     uint8_t r0 = data[0], g0 = data[1], b0 = data[2];
     size_t lastIdx = (16 * 16 - 1) * 3;
     uint8_t r1 = data[lastIdx], g1 = data[lastIdx + 1], b1 = data[lastIdx + 2];

As per coding guidelines, test/** requires edge-case-aware tests; crashing on null/short buffers bypasses that.

🧰 Tools

🪛 Cppcheck (2.20.0)

[style] 54-54: The function 'addText' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_plasma.cpp` around lines 52 - 56, The test directly indexes
layer.buffer().data() and can crash if the buffer is null or too small; before
accessing data[0] and data[lastIdx+2] add REQUIRE checks that the buffer pointer
is non-null and that layer.buffer().size() (or equivalent size accessor on
layer.buffer()) is greater than lastIdx+2 so the accesses are safe, then proceed
with the existing uint8_t r0/g0/b0, r1/g1/b1 and CHECK; reference the local
variables data, lastIdx and the CHECK assertion when adding these precondition
REQUIREs.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add dynamicBytes()==0 assertions for stateless effects.

These tests currently miss a key documented contract (heap-free/stateless memory behavior). Please assert effect.dynamicBytes() == 0 after allocation/loop for each stateless effect case.

Proposed pattern

 `#define` STATELESS_EFFECT_TEST(EFFECT) \
     TEST_CASE(`#EFFECT` " writes non-zero RGB") { \
         Ctx ctx(16, 16); \
         mm::EFFECT effect; \
         ctx.layer.addChild(&effect); \
         ctx.layer.onAllocateMemory(); \
         ctx.layer.loop(); \
+        CHECK(effect.dynamicBytes() == 0); \
         CHECK(ctx.hasNonZero()); \
     } \

As per coding guidelines, tests in test/** must match docs/moonmodules/ specifications, and those effect docs explicitly require heap-free / dynamicBytes() = 0 behavior.

Also applies to: 76-118

🧰 Tools

🪛 Clang (14.0.6)

[warning] 58-58: function-like macro 'STATELESS_EFFECT_TEST' used; consider a 'constexpr' template function

(cppcoreguidelines-macro-usage)

🪛 Cppcheck (2.20.0)

[style] 69-69: The function 'removeChild' is never used.

(unusedFunction)

---

[style] 63-63: The function 'cos8' is never used.

(unusedFunction)

---

[style] 61-61: The function 'lut' is never used.

(unusedFunction)

---

[style] 70-70: The function 'physicalLightCount' is never used.

(unusedFunction)

---

[style] 74-74: The function 'lutSkipped' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_stateless_effects.cpp` around lines 58 - 74, The tests generated by
STATELESS_EFFECT_TEST must assert the stateless contract by checking
effect.dynamicBytes() == 0; add an assertion after allocation/loop in both
generated TEST_CASEs so that for each mm::EFFECT created (inside Ctx ctx(...);
mm::EFFECT effect; ctx.layer.addChild(&effect); ctx.layer.onAllocateMemory();
ctx.layer.loop();) you call CHECK(effect.dynamicBytes() == 0) to ensure
heap-free behavior; update the macro STATELESS_EFFECT_TEST (and the same pattern
in the other tests covering lines 76-118) to include this assertion immediately
after onAllocateMemory()/loop().

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use full RGB-triplet comparison for checkerboard variation.

Comparing only one channel is brittle; assert full-pixel difference to match the spec intent.

Proposed fix

     ctx.layer.onAllocateMemory();
     ctx.layer.loop();
     auto* data = ctx.layer.buffer().data();
-    CHECK(data[0] != data[4 * 3]);
+    const size_t a = 0;
+    const size_t b = 4 * 3;
+    CHECK(data[a] != data[b] || data[a + 1] != data[b + 1] || data[a + 2] != data[b + 2]);

As per coding guidelines, tests under test/** should match specs in docs/moonmodules/, which define spatial variation at color/pixel level.

🧰 Tools

🪛 Cppcheck (2.20.0)

[style] 94-94: The function 'forEachDestination' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_stateless_effects.cpp` around lines 92 - 94, The test currently
only compares a single channel (data[0] != data[4 * 3]) which is brittle; change
it to assert the full RGB triplet differs by comparing the three color bytes for
the two pixels retrieved from ctx.layer.buffer().data() (the local variable
data). Replace the single-channel check with a full-pixel comparison (e.g., use
std::memcmp(&data[0], &data[4 * 3], 3) != 0 or explicitly check data[0..2] vs
data[4*3..4*3+2]) so the test verifies RGB-level spatial variation as specified.