Paged MappingLUT + blendMap; Peripheral role + SystemModule child management

[ewowi]

Summary

Two commits on next-iteration:

1. be505bb — Paged MappingLUT + blendMap overwrite path; docs reorg — fixes the two no-PSRAM-ESP32 blockers (mirror LUT degrading at 128×128, and the effect-cost regression it caused) by paging the mapping table's destinations so it fits a fragmented heap, and speeds the per-tick map by skipping the additive blend where each light is written once. Also reorganises forward-looking docs into docs/backlog/ and migrates the present-tense docs.
2. 2631d7a — Peripheral module role + SystemModule child management — adds a domain-neutral ModuleRole::Peripheral for hardware/network bridges (sensors, actuators) the user adds/removes at runtime under SystemModule. The core infrastructure a gyro/IMU, mic, relay, or Home Assistant module attaches to. Plus a CodeRabbit review round.

---

1. Paged MappingLUT + blendMap (be505bb)

The blockers (no-PSRAM ESP32, 128×128): a mirror LUT needs ~64 KB of destinations, but the largest contiguous free block on the Olimex is ~18 KB — a fragmentation cliff that degraded the modifier to 1:1 (mirror invisible) and made the effect render the full grid (~4× cost).

Fix — three-tier destinations allocation: single contiguous block if it fits (PSRAM + small grids — unchanged hot path), else page into fixed 8 KB (4096-entry, power-of-two) blocks when total heap allows, else degrade to 1:1 + warning. forEachDestination keeps the flat loop for the single-block case and switches pages at each 4096 boundary otherwise — paged_ is one branch-predicted check per blend, not per destination.

blendMap overwrite fast-path: MappingLUT::overwrites() (default true — every current layout/modifier writes each light once) → plain copy instead of additive read-add-clamp (~4×). Additive blend stays available (setOverwrites(false)) for future multi-layer compositing; both paths tested.

ArtNet investigation (reverted): found that lwIP blocks UDP TX below the socket API — neither O_NONBLOCK nor MSG_DONTWAIT sheds it (verified on hardware, same ~35 ms, zero drops). Reverted; the real fix (async send task) is recorded in backlog as PSRAM-only.

Docs: plan.md → backlog/backlog.md, moonmodules_draft/ → backlog/moonmodules_draft/; present-tense migration (facts pulled into specs, forward/backward pointers removed).

2. Peripheral role + SystemModule child management (2631d7a)

A peripheral is a domain-neutral MoonModule (ModuleRole::Peripheral) that bridges to hardware/network independently of the light pipeline. The defining line: consumes the light output buffer → driver; otherwise → peripheral (a DMX sender is a driver despite a UART transport). Read-vs-write is a per-module decision, not a role split — one role spans readers, writers, and both.

Peripherals are user-add/deletable children of SystemModule (the firmware is identical whether or not the device has the peripheral soldered on). Reuses the existing generic child add/replace/delete + persistence machinery. Lifecycle fix: SystemModule's overridden setup() and loop1s() now chain to MoonModule:: base so a peripheral child actually inits and polls (closing the latent "container children never tick" gap from decisions.md). BoardModule::userEditable() → false so the code-wired board identity can't be deleted from the now-editable child list.

Architecture documented (§ Peripherals, with the concrete sensor→effect data-exchange wiring). The Pi-sensor backlog entry updated to reflect the role now exists.

CodeRabbit round: BlendMap comment, a sparse-clear regression test, esp32 testMaxBlock made atomic + min(real,cap), NetworkModule.md/CLAUDE.md doc fixes. One finding declined (CLAUDE.md gate-list renumber — the 1-N-across-subheaders numbering is intentional).

Pre-merge review

• 👾 Reviewer (Opus, whole branch): APPROVE-WITH-NITS — domain boundary clean, core-grows-slower bar passed (Peripheral role justified with a stated roster), hot path clean, paged-LUT correctness verified, docs reorg consistent. Findings addressed in-branch: carry-forward lesson recorded in decisions.md, two comment nits fixed.
• 🐇 CodeRabbit round processed (see commit 2).

Test plan

• Desktop build clean (-Werror, zero warnings)
• ctest / mm_tests — 229 cases, 5017 assertions pass
• mm_scenarios — 10/10 pass · boundary PASS · spec check 28/28
• ESP32 builds — esp32, esp32-eth, esp32-eth-wifi, esp32s3-n16r8 all clean
• KPI — tick:24707us(FPS:40) on esp32-eth (Olimex); LUT builds clean at maxBlock ~19 KB (no skip), mirror applied
• Hardware-verified: paged LUT allocates where a single block can't fit; mirror applied; effect on the quadrant

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2c72d2ff-06cf-4dac-a221-889ca88667d4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% 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 summarizes the two main changes: paged MappingLUT with blendMap overwrite optimization and the introduction of Peripheral role with SystemModule child management.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch next-iteration

---

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.}

[coderabbitai]

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (2)

src/light/layers/BlendMap.h (1)

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

Update the function header to match the new overwrite fast path.

Lines 10-12 still describe blendMap() as additive-only, but the default behavior is now overwrite/copy when lut.overwrites() is true. Leaving that comment stale misdocuments the API right above the hot-path implementation.

✏️ Suggested update

-// Reads from logical buffer (src), writes to physical buffer (dst) via LUT.
-// Additive blending with clamping (for future multi-layer support).
+// Reads from logical buffer (src), writes to physical buffer (dst) via LUT.
+// Default behavior is overwrite/copy when each physical destination is unique;
+// additive clamped blending is used only when `lut.overwrites()` is false.

As per coding guidelines for **/*.{cpp,h,ts,tsx,js}, comments document intent and context, so factually wrong comments should be corrected.

Also applies to: 23-39

🤖 Prompt for AI Agents

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

In `@src/light/layers/BlendMap.h` around lines 10 - 12, Update the comment for the
blendMap function to reflect the new fast-path behavior: note that when
lut.overwrites() is true the function performs an overwrite/copy from src to dst
(fast path), and when it is false it performs additive blending with clamping
for multi-layer support; update any similar stale comments in the same block
(lines covering the rest of the function header and description) so the comment
correctly references MappingLUT::overwrites() and both behaviors (overwrite vs
additive) are documented.

Source: Coding guidelines

test/unit/light/unit_BlendMap.cpp (1)

75-167: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add a sparse overwrite test that starts from dirty dst.

The new overwrite path depends on dst.clear() in src/light/layers/BlendMap.h Lines 18-20, but every added case here either fills all physical slots or writes into a fresh buffer. A regression removing that clear would still pass these tests while reintroducing stale pixels on sparse layouts.

🧪 Suggested test

+TEST_CASE("blendMap overwrite path clears unmapped physical cells") {
+    mm::Buffer src, dst;
+    src.allocate(1, 3);
+    dst.allocate(3, 3);
+    std::memset(dst.data(), 0x7F, dst.bytes());
+
+    src.data()[0] = 10;
+    src.data()[1] = 20;
+    src.data()[2] = 30;
+
+    mm::MappingLUT lut;
+    REQUIRE(lut.build(1, 1));
+    mm::nrOfLightsType map[] = {1};
+    lut.setMapping(0, map, 1);
+    lut.finalize();
+
+    mm::blendMap(src, dst, lut, 3);
+
+    CHECK(dst.data()[0] == 0);
+    CHECK(dst.data()[1] == 0);
+    CHECK(dst.data()[2] == 0);
+    CHECK(dst.data()[3] == 10);
+    CHECK(dst.data()[4] == 20);
+    CHECK(dst.data()[5] == 30);
+    CHECK(dst.data()[6] == 0);
+    CHECK(dst.data()[7] == 0);
+    CHECK(dst.data()[8] == 0);
+}

As per coding guidelines for test/**, tests should cover edge cases and match the specifications in docs/moonmodules/.

🤖 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_BlendMap.cpp` around lines 75 - 167, Add a new unit test
that verifies the overwrite (overwrites=true) path clears destination pixels for
sparse mappings by starting dst filled with non-zero "dirty" values, applying a
MappingLUT that only writes to a subset of physical lights, calling
mm::blendMap(src, dst, lut, channels) and asserting that (a) written physical
pixels contain the expected last-writer values and (b) untouched physical pixels
were cleared (e.g., zero); reference mm::Buffer, mm::MappingLUT, and
mm::blendMap and ensure the test exercises the sparse layout so it would fail if
dst.clear() in src/light/layers/BlendMap.h were removed.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@CLAUDE.md`:
- Line 176: The fenced code block opening fence currently lacks a language tag;
update the opening fence (the triple-backtick block) to include a language
identifier such as "text" (e.g., change ``` to ```text) so markdownlint rule
MD040 is satisfied and the block is properly tagged.
- Line 167: The numbered list item labeled "7. **Principles audit** — sweep
`docs/`..." uses the wrong prefix and triggers markdownlint MD029; change the
numeric prefix from "7." to "3." so the ordered list sequence is correct (update
the line containing "Principles audit" in CLAUDE.md to start with "3."),
ensuring the list remains valid and passes MD029.

In `@docs/moonmodules/core/NetworkModule.md`:
- Line 109: Change the ambiguous statement about onBuildControls() to specify it
binds only the full set of controls applicable to the current build/target and
detected hardware (e.g., “full applicable set per build target”), and update the
contract so onBuildControls() binds controls only for interfaces that will be
initialized/supported (use platform::ethInit() and the WiFi STA/AP init return
values as the determiners); also note that absent-interface cards still expose
"no link"/"no IP" since there is no hardware-presence signal to the UI, and that
ssid/password and other WiFi controls must be omitted when WiFi init fails or
the build target is Ethernet-only.

In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 106-111: The ESP32 backend currently uses a non-atomic global
testMaxBlock and entirely replaces the real value in maxAllocBlock(); change
testMaxBlock to an atomic<size_t> (like the desktop backend), update
setTestMaxAllocBlock to store into that atomic, and modify maxAllocBlock() to
read the real heap_caps_get_largest_free_block(MALLOC_CAP_8BIT) then return the
minimum of the real value and the atomic cap (treat 0 as no cap) to avoid races
and preserve the “cap” contract documented in platform.h.

---

Outside diff comments:
In `@src/light/layers/BlendMap.h`:
- Around line 10-12: Update the comment for the blendMap function to reflect the
new fast-path behavior: note that when lut.overwrites() is true the function
performs an overwrite/copy from src to dst (fast path), and when it is false it
performs additive blending with clamping for multi-layer support; update any
similar stale comments in the same block (lines covering the rest of the
function header and description) so the comment correctly references
MappingLUT::overwrites() and both behaviors (overwrite vs additive) are
documented.

In `@test/unit/light/unit_BlendMap.cpp`:
- Around line 75-167: Add a new unit test that verifies the overwrite
(overwrites=true) path clears destination pixels for sparse mappings by starting
dst filled with non-zero "dirty" values, applying a MappingLUT that only writes
to a subset of physical lights, calling mm::blendMap(src, dst, lut, channels)
and asserting that (a) written physical pixels contain the expected last-writer
values and (b) untouched physical pixels were cleared (e.g., zero); reference
mm::Buffer, mm::MappingLUT, and mm::blendMap and ensure the test exercises the
sparse layout so it would fail if dst.clear() in src/light/layers/BlendMap.h
were removed.

🪄 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: f7fa1aaf-8d7e-4089-aa72-a1fe54bd1b20

📥 Commits

Reviewing files that changed from the base of the PR and between a3d4611 and be505bb.

📒 Files selected for processing (35)

• .github/workflows/release.yml
• CLAUDE.md
• README.md
• docs/backlog/backlog.md
• docs/backlog/moonmodules_draft/.gitkeep
• docs/backlog/moonmodules_draft/core/ui.md
• docs/backlog/moonmodules_draft/light/effects/ArtNetReceiveEffect.md
• docs/backlog/moonmodules_draft/light/effects/DistortionWavesEffect.md
• docs/backlog/moonmodules_draft/light/effects/SineEffect.md
• docs/backlog/moonmodules_draft/light/layouts/WheelLayout.md
• docs/backlog/moonmodules_draft/light/modifiers/RotateModifier.md
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/core/ui.md
• docs/moonmodules/light/Layers.md
• docs/moonmodules/light/MappingLUT.md
• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/moonmodules/light/leddriver-analysis-bottom-up.md
• docs/moonmodules_draft/light/effects/LinesEffect.md
• docs/moonmodules_draft/light/effects/RipplesEffect.md
• docs/performance.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/check/collect_kpi.py
• src/core/BoardModule.h
• src/light/drivers/Drivers.h
• src/light/layers/BlendMap.h
• src/light/layers/Layer.h
• src/light/layers/MappingLUT.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• test/scenarios/light/scenario_Layouts_mutation.json
• test/unit/core/unit_MappingLUT.cpp
• test/unit/light/unit_BlendMap.cpp

💤 Files with no reviewable changes (3)

• docs/moonmodules_draft/light/effects/LinesEffect.md
• docs/moonmodules/core/ui.md
• docs/moonmodules_draft/light/effects/RipplesEffect.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix ordered-list numbering to satisfy markdownlint (MD029).

Line 167 should use the next numeric prefix in sequence (3.), not 7., to avoid list-prefix lint warnings.

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 167-167: Ordered list item prefix
Expected: 3; Actual: 7; Style: 1/2/3

(MD029, ol-prefix)

🤖 Prompt for AI Agents

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

In `@CLAUDE.md` at line 167, The numbered list item labeled "7. **Principles
audit** — sweep `docs/`..." uses the wrong prefix and triggers markdownlint
MD029; change the numeric prefix from "7." to "3." so the ordered list sequence
is correct (update the line containing "Principles audit" in CLAUDE.md to start
with "3."), ensuring the list remains valid and passes MD029.

Source: Linters/SAST tools

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/architecture.md`:
- Around line 195-200: The paragraph that reads “Automatic detection — probing a
bus and adding the matching peripheral module — is a future convenience on top;
the manual path is the foundation.” uses future-tense and should be rewritten to
present-tense to comply with docs present-tense rules: change that sentence to
something like “Automatic detection — probing a bus and adding the matching
peripheral module — is out of scope here; the manual path is the foundation.”
Ensure any other forward-looking phrasing in the same paragraph (e.g., “a future
`platform::uart*`”) is likewise converted to present-tense or
scoped/out-of-scope phrasing, keeping the rest of the section (mentions of
SystemModule, Peripheral role, file conventions, loop20ms/loop1s,
check_specs.py) unchanged.

In `@docs/moonmodules/core/NetworkModule.md`:
- Line 109: The docs claim onBuildControls() always binds deviceName and mode
but the Controls section lacks deviceName (it's provided by SystemModule);
update the NetworkModule.md to remove deviceName from the module's own controls
or explicitly state that deviceName is read/provided by SystemModule, and keep
mode listed as bound by onBuildControls(); ensure the Controls section and the
narrative mention the same set of controls (reference onBuildControls(),
deviceName, mode, and SystemModule) so the spec is unambiguous for implementers.

🪄 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: f8ce799f-3cc6-431e-be6b-a7dfe9632858

📥 Commits

Reviewing files that changed from the base of the PR and between be505bb and 2631d7a.

📒 Files selected for processing (15)

• CLAUDE.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/NetworkModule.md
• src/core/BoardModule.h
• src/core/ModuleFactory.h
• src/core/MoonModule.h
• src/core/SystemModule.h
• src/light/layers/BlendMap.h
• src/platform/esp32/platform_esp32.cpp
• src/ui/app.js
• test/unit/core/unit_BoardModule.cpp
• test/unit/core/unit_SystemModule.cpp
• test/unit/light/unit_BlendMap.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep architecture text strictly present-tense in non-backlog docs.

Line 196 uses forward-looking phrasing (“future convenience”), which conflicts with the present-tense-only rule for docs/ outside backlog/history. Reword this as current-state contract text (e.g., “Automatic detection is out of scope here; the manual path is the foundation.”).

As per coding guidelines: “docs/!(backlog|history)/**/*.md: All code and documentation must use present tense only; no changelogs, roadmaps, or future-looking language”.

🧰 Tools

🪛 LanguageTool

[style] ~195-~195: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...stemModule**. The firmware is identical whether or not the device has the peripheral wired, so...

(WHETHER)

🤖 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 195 - 200, The paragraph that reads
“Automatic detection — probing a bus and adding the matching peripheral module —
is a future convenience on top; the manual path is the foundation.” uses
future-tense and should be rewritten to present-tense to comply with docs
present-tense rules: change that sentence to something like “Automatic detection
— probing a bus and adding the matching peripheral module — is out of scope
here; the manual path is the foundation.” Ensure any other forward-looking
phrasing in the same paragraph (e.g., “a future `platform::uart*`”) is likewise
converted to present-tense or scoped/out-of-scope phrasing, keeping the rest of
the section (mentions of SystemModule, Peripheral role, file conventions,
loop20ms/loop1s, check_specs.py) unchanged.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Resolve control-contract mismatch (deviceName vs listed controls).

This line says onBuildControls() always binds deviceName and mode, but this module’s Controls section does not define a deviceName control (it is described as read from SystemModule). Please align wording to avoid an ambiguous spec contract.

Based on learnings: “Module specs must be sufficient to implement from before writing code…”.

🤖 Prompt for AI Agents

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

In `@docs/moonmodules/core/NetworkModule.md` at line 109, The docs claim
onBuildControls() always binds deviceName and mode but the Controls section
lacks deviceName (it's provided by SystemModule); update the NetworkModule.md to
remove deviceName from the module's own controls or explicitly state that
deviceName is read/provided by SystemModule, and keep mode listed as bound by
onBuildControls(); ensure the Controls section and the narrative mention the
same set of controls (reference onBuildControls(), deviceName, mode, and
SystemModule) so the spec is unambiguous for implementers.

Sources: Coding guidelines, Learnings