DevicesModule + mDNS, 4 light modules + dynamic modifiers, pin controls, per-chip drivers, installer auto-detect

[ewowi]

Summary

A broad next-iteration batch on top of the runtime-Ethernet work in main: device discovery (mDNS + HTTP sweep), four new light modules plus the first dynamic modifiers, a standard Pin control type, per-chip LED-driver gating, two new performance scenarios with a three-board analysis, and the installer IP auto-detect this PR started as. Seven commits; all host gates green, all three ESP32 boards (classic / S3 / P4) build clean and were exercised live.

What landed

DevicesModule + discovery (core, domain-neutral)

• New DevicesModule: discovers other devices on the LAN (projectMM / WLED / generic), shows them in a generic List control, marks self. Two strategies merge into one list: a non-blocking mDNS browse every tick (devices advertise _http._tcp; cycles _http/_wled) and a boot-once HTTP subnet sweep (the blocking probe stays off the periodic path).
• Each device carries via (how found: scan/mdns/udp), ageSec (seconds since last seen, device-side), and a cached flag (restored-from-persistence, not yet re-seen live → "last seen: cached"). 24h timestamp age-out, strategy-agnostic.
• The list persists across reboot (instant boot list) via a new recursive mm::json reader (JsonUtil.h) + ListSource::restoreList; a generic core Sort.h (insertion sort) keeps it name-ordered.
• ESP32 now advertises _http._tcp, so projectMM devices find each other over mDNS.

Light modules + dynamic-modifier hook

• Effects: SineEffect (3D R/G/B sine), DistortionWavesEffect (WLED-style interfering sines) — integer sin8/cos8, no float.
• Layout: WheelLayout (spokes × ledsPerSpoke, integer trig).
• Modifiers: RandomMapModifier (1:1 random permutation, reshuffles on a bpm timer) and RotateModifier (stepped integer rotation) — the first dynamic modifiers, via a minimal MoonModule::loop() override ticked by Layer::loop() (only the first enabled modifier, matching rebuildLUT).
• architecture.md: light-domain rule — an effect renders a pattern, it does not transform geometry; mirror/tile/rotate/scroll/mask belong in a modifier.

Pin controls → a standard Pin type

• Every single-GPIO control (AudioModule wsPin/sdPin/sckPin, LED-driver clock/dc + loopback pins) moved from addUint16 to the standard addPin/int8_t Pin control. Unset sentinel is now -1 (was 0), fixing a latent bug where GPIO 0 was unusable; loopback now guards rxPin >= 0 (the old cast turned -1 into GPIO 255).

Per-chip LED-driver gating

• Each LED driver's #include + registration is gated on its SOC capability macro (RMT / LCD_CAM / Parlio), so a board's binary carries only the drivers its silicon can run (verified: the classic ELF has 0 LCD/Parlio symbols; its type picker offers only RmtLedDriver). Not the platform/OS #ifdef the boundary rule forbids — check_platform_boundary.py passes.

Performance scenarios + 3-board analysis

• New scenario_perf_light (fast, ≤64², driver-free) and scenario_perf_full (comprehensive: per-subsystem incremental cost, all-this-board drivers capped to 64 LEDs, light/heavy effect bracket to 16K, modifier sweep), replacing three overlapping scenarios (net −1765 scenario lines). The older mutate scenarios were made self-canvasing so chained live runs are order-independent.
• performance.md: three-board (classic / S3 / P4) incremental-cost tables. Key findings: render scales linearly (no fragmentation); the heavy effect is the 16K bottleneck (P4 57 / S3 20 / classic 16 FPS); the MultiplyModifier roughly halves heavy tick (¼ logical grid) at a LUT-memory cost; 16K Noise+Multiply runs on the no-PSRAM classic at 35 FPS render-only.

Installer IP auto-detection (the original PR scope)

• NetworkModule::currentIp(uint8_t[4]); main.cpp appends a MM_IP=<ip> token to the tick line (first-60s gated). The web installer's readBootIp() reads it post-flash and routes an already-online (Ethernet / saved-WiFi) device straight to the "Device is online!" success screen, auto-adding it. Board cards gained a boards.json details popup; install gated on a port being picked.
• IP octet seam: platform getters return uint8_t[4] end to end (ethGetIPv4/wifiStaGetIPv4); default_firmware removed (firmwares[0] by convention).

Verification

• Host gates: spec check, board catalog, platform boundary, desktop build (0 warnings), unit tests (420 cases), scenarios (all pass) — green.
• ESP32 builds: classic esp32, esp32s3-n16r8, esp32p4-eth all compile clean (-Werror).
• Hardware: both perf scenarios ran PASS on all three boards (classic 192.168.1.207, S3 .156, P4 .132); mDNS cross-discovery verified (S3 finds P4 via mdns); device-IP auto-detect verified on the P4 over Ethernet.
• Reviewer agent (pre-merge): whole-branch architectural review — clean, merge-ready, no must-fix.

🤖 Generated with Claude Code

[coderabbitai]

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.25% 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 is a concise, multi-faceted description of the primary changes: DevicesModule + mDNS discovery, 4 new light modules with dynamic modifiers, pin-type controls, per-chip driver gating, and installer auto-detection via IP boot-log reading.

_{✏️ 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 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: 3

Caution

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

⚠️ Outside diff range comments (1)

src/platform/esp32/platform_esp32_improv.cpp (1)

239-257: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard provision success on non-zero IPv4 before emitting the URL.

Line 253 formats and sends a success URL even though this path never verifies ip is non-zero. If wifiStaConnected() is true before a usable address is bound, this can emit http://0.0.0.0/ as a successful provisioning result.

Suggested fix

     uint8_t ip[4] = {};
     for (int i = 0; i < 300; i++) {  // 30 s @ 100 ms
         vTaskDelay(pdMS_TO_TICKS(100));
-        if (wifiStaConnected()) {
-            wifiStaGetIPv4(ip);
-            break;
-        }
+        if (!wifiStaConnected()) continue;
+        wifiStaGetIPv4(ip);
+        if (ip[0] || ip[1] || ip[2] || ip[3]) break;
     }
-    if (!wifiStaConnected()) {
+    if (!wifiStaConnected() || (!ip[0] && !ip[1] && !ip[2] && !ip[3])) {
         improvSetStatus("error: no IP after 30s");
         improvSendError(improv::ERROR_UNABLE_TO_CONNECT);
         return;
     }

As per coding guidelines, callers must treat 0.0.0.0 as “no IP yet” and avoid emitting/formatting 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 `@src/platform/esp32/platform_esp32_improv.cpp` around lines 239 - 257, The
code checks if wifiStaConnected() is true and retrieves the IP address via
wifiStaGetIPv4(ip), but does not verify that the retrieved IP address is
non-zero before formatting and sending it as a success URL. This can result in
emitting http://0.0.0.0/ as a successful provisioning result if the device
connects but hasn't yet been assigned a usable address. After the
wifiStaConnected() check at line 248, add validation to ensure the IP address in
the ip array is non-zero (for example, check that at least one octet is
non-zero, or that the full address is not 0.0.0.0). Only proceed with the
snprintf call that formats the URL and the subsequent improvSetStatus call if
this IP validation passes; otherwise, treat it as a failure case similar to the
timeout scenario.

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 `@scripts/check/check_boards.py`:
- Around line 88-100: The firmware validation in the check does not reject
whitespace-only or padded string values like " " or "esp32 ", which can cause
silent failures in exact firmware-key matching downstream. After the existing
check for non-empty strings (the condition `any(not f for f in fws)`), add
another validation to reject firmware entries that are whitespace-only or
contain leading/trailing whitespace. Use string methods like strip() to detect
such cases and append an appropriate error message to the errors list when such
values are found in the fws list.

In `@src/main.cpp`:
- Around line 340-348: The condition at line 340 uses a simple time comparison
(now < 60000) that re-activates after millis() wraps around on long uptimes,
causing MM_IP logging to restart. Replace the direct comparison with a wrap-safe
elapsed-time calculation by tracking a start time and comparing the elapsed time
(now minus start time) against 60000 milliseconds instead of comparing the
absolute now value against 60000. This ensures the MM_IP logging window only
occurs during the first 60 seconds regardless of timer wraparound.

In `@src/ui/install-picker.js`:
- Around line 611-619: The install button enablement is being unconditionally
re-enabled in a finally block after install attempts, which can bypass the port
requirement check. Instead of directly enabling the button, call the
applyInstallEnabled() gate function to properly recompute the button's
enablement state based on current state conditions including the hasPort()
check. This change needs to be applied at the finally block in the install
button's click handler (lines 611-619) and also at the additional location
(lines 647-648) where the same unconditional enablement occurs.

---

Outside diff comments:
In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 239-257: The code checks if wifiStaConnected() is true and
retrieves the IP address via wifiStaGetIPv4(ip), but does not verify that the
retrieved IP address is non-zero before formatting and sending it as a success
URL. This can result in emitting http://0.0.0.0/ as a successful provisioning
result if the device connects but hasn't yet been assigned a usable address.
After the wifiStaConnected() check at line 248, add validation to ensure the IP
address in the ip array is non-zero (for example, check that at least one octet
is non-zero, or that the full address is not 0.0.0.0). Only proceed with the
snprintf call that formats the URL and the subsequent improvSetStatus call if
this IP validation passes; otherwise, treat it as a failure case similar to the
timeout scenario.

🪄 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: e4e180cd-8143-496f-a759-8904ee1b2b20

📥 Commits

Reviewing files that changed from the base of the PR and between 4bd23ad and d87dac2.

📒 Files selected for processing (23)

• CLAUDE.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/coding-standards.md
• docs/install/README.md
• docs/install/boards.json
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/BoardModule.md
• docs/moonmodules/core/NetworkModule.md
• scripts/check/check_boards.py
• scripts/moondeck_ui/app.js
• src/core/NetworkModule.h
• src/light/effects/NetworkReceiveEffect.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/platform.h
• src/ui/install-picker.js
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/scenarios/light/scenario_Driver_mutation.json

[ewowi]

@coderabbitai, resume and review

[coderabbitai]

@ewowi Sure! Resuming automatic reviews and kicking off a review of PR #21 now.

✅ Action performed

Reviews resumed.

---

Review finished.

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, I did not saw reviews, so review 04413b0 and deliver a prompt for agents on your findings

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details of the specific commit
gh api repos/MoonModules/projectMM/commits/04413b0be7cb286fefbf06b4fe8971bf8128fbef --jq '{sha: .sha, message: .commit.message, files: [.files[] | {filename: .filename, status: .status, additions: .additions, deletions: .deletions, patch: .patch}]}'

Length of output: 28759

---

---

@ewowi — here's my review of commit 04413b0 with an agent-ready prompt at the bottom.

---

Review findings

✅ Good fixes in this commit

Location	What's fixed
src/main.cpp	millis() wraparound — now - bootMillis >= 60000 with a latch flag is correct unsigned arithmetic
src/platform/esp32/platform_esp32_improv.cpp	Improv no longer emits http://0.0.0.0/ — polls until a non-zero IP octet
scripts/check/check_boards.py	Whitespace-padded firmware keys rejected after the non-empty-string gate
src/ui/install-picker.js	finally block calls applyInstallEnabled() instead of unconditionally enabling Install

---

⚠️ Issues found

1 — applyHostname() silently drops DHCP if esp_netif_dhcpc_start fails (Medium)

File: src/platform/esp32/platform_esp32.cpp

esp_netif_dhcpc_stop(netif);   // ignored: ALREADY_STOPPED is benign, others are not
esp_netif_set_hostname(netif, hostname_);
esp_netif_dhcpc_start(netif);  // return value never checked

If esp_netif_dhcpc_start returns an error, the interface is left with a stopped DHCP client and never acquires an IP — silently. The stop error is documented as intentionally ignored (ALREADY_STOPPED), but the start error should at least be logged.

---

2 — CAP_MODULE map conflates Ethernet and WiFi (Medium)

File: docs/install/index.html

const CAP_MODULE = {
  Ethernet: t => t === "NetworkModule",
  WiFi:     t => t === "NetworkModule",
  ...
};

Both capabilities map to the same NetworkModule type check. A board that only has Ethernet wired (e.g. an Olimex with no WiFi antenna) shows WiFi as green/active, and vice-versa. The distinction needs a finer signal — either a separate module type, a capability flag on the module entry, or checking board-level supported[] membership against module presence.

---

3 — hostname_[] has no memory ordering guarantee across tasks (Low)

File: src/platform/esp32/platform_esp32.cpp

setHostname() writes hostname_[32] on the app task; applyHostname() reads it on the IDF system-event task. The write happens before ethInit()/wifiStaInit() in practice, so it's safe today, but there is no volatile, memory barrier, or mutex. A compiler may reorder the write past event-handler registration. At minimum a comment explaining the ordering contract — or a std::atomic_thread_fence — should guard this.

---

4 — hostName() fallback "mm" causes collisions (Low)

File: src/core/NetworkModule.h

const char* hostName() const {
    return (systemModule_ && systemModule_->deviceName()[0] != 0)
           ? systemModule_->deviceName() : "mm";
}

If deviceName() is empty, every device on the LAN registers both mDNS .local and DHCP option-12 as mm, causing immediate collision. The fallback should either be unique (e.g. a MAC-derived suffix) or generate a compile-time error/assert rather than silently use a generic name.

---

Agent prompt

You are an agent reviewing MoonModules/projectMM commit 04413b0 (PR `#21`).
Fix the following issues in priority order:

## Issue 1 — applyHostname: handle esp_netif_dhcpc_start failure
File: src/platform/esp32/platform_esp32.cpp
Function: static void applyHostname(esp_netif_t* netif)

After `esp_netif_dhcpc_start(netif)`, capture the return value and log a
warning via ESP_LOGW if it is not ESP_OK. Do NOT silently continue — the
interface will have no DHCP client and never acquire an IP. Suggested form:

    esp_err_t se = esp_netif_dhcpc_start(netif);
    if (se != ESP_OK)
        ESP_LOGW(NET_TAG, "dhcpc_start after set_hostname failed: %s", esp_err_to_name(se));

## Issue 2 — CAP_MODULE Ethernet/WiFi distinction
File: docs/install/index.html
Section: const CAP_MODULE = { ... }

Currently both "Ethernet" and "WiFi" keys use `t => t === "NetworkModule"`,
so any board with NetworkModule shows BOTH capabilities as green/active even
when only one interface is physically wired. Fix by distinguishing them using
the module controls already present in boards.json:

- A module is "active for Ethernet" when its type is "NetworkModule" AND
  it has a non-empty "ethType" in its controls (or a non-default ethType).
- A module is "active for WiFi" when its type is "NetworkModule" AND
  it does NOT have an ethType control (WiFi-only NetworkModule),
  OR check the board's "supported" array membership as a proxy.

Alternatively check the module-level controls for an eth-specific pin key
("ethTxPin", "ethRxPin", or "ethType" != "" / "None").

Adjust capActive(b, cap) to receive the module object (not just type) so
controls can be inspected.

## Issue 3 — hostname_ memory ordering
File: src/platform/esp32/platform_esp32.cpp
Near static char hostname_[32] = {};

Add a comment explicitly documenting the ordering contract:
"setHostname() is called from the app task before ethInit()/wifiStaInit()
is invoked, guaranteeing hostname_[] is written before any event-task
reader (applyHostname) can execute. Do not call setHostname() concurrently
with or after bring-up without adding a mutex."

Optionally add a static_assert or runtime assert that hostname_ is only
set while ethNetif_/staNetif_ are null (i.e. before bring-up), or use
portENTER_CRITICAL / std::atomic for true safety.

## Issue 4 — hostName() fallback uniqueness
File: src/core/NetworkModule.h
Function: const char* hostName() const

The fallback "mm" collides across devices. Replace with a unique default:
- Use a MAC-derived suffix (same pattern as deviceName's MM-XXXX default),
  or
- Assert/log-error that deviceName is never empty at this point and keep
  the fallback only as a last resort with a distinct prefix like "mm-unknown"
  to make collisions visible.

Note: do not change the public API of hostName(); fix is internal only.

[ewowi]

@coderabbitai, I did not saw reviews, so review 2ef57f3 and deliver a prompt for agents on your findings

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh api repos/MoonModules/projectMM/commits/2ef57f3f9ff6f8f8f3540eccee465ec260427c4e \
  --jq '{
    sha: .sha,
    message: .commit.message,
    files: [.files[] | {
      filename: .filename,
      status: .status,
      additions: .additions,
      deletions: .deletions,
      patch: .patch
    }]
  }'

Length of output: 95304

---

---

@ewowi — here's my review of commit 2ef57f3.

---

Review findings

✅ Strong additions in this commit

Area	What's good
DeviceIdentify.h	Pure, host-unit-testable classification logic — correctly factored out. The extractStringAfter two-step ("deviceName" → "value") fixes the type-field vs. value-field parsing bug
DevicesModule.h	1 KB probeBuf_ as a member (not stack-local) is the right call for the FreeRTOS stack
Desktop httpGet	Non-blocking connect + select() is the correct fix for the 75-second desktop hang on dead IPs
Control.h	ListSource interface is a clean adapter (no per-row alloc, data stays in the module's own array)
hostName() fix	MAC-derived fallback instead of the colliding "mm" literal
applyHostname fix	esp_netif_dhcpc_start failure now logged via ESP_LOGW
CAP_MODULE fix	Ethernet now requires a real ethType control — WiFi and Ethernet are properly separated

---

⚠️ Issues found

1 — probePort: classify without status-200 gate (Medium)

File: src/core/DevicesModule.h

int status = platform::httpGet(url, kProbeTimeoutMs, probeBuf_, sizeof(probeBuf_));
if (status == 0) return false;           // ← only rejects "no host"
DevType t = classifyDevice(probeBuf_, nullptr);
if (t == DevType::ProjectMM) { ... }     // ← classifies a 404/500 body

The WLED probe path already gates on status == 200:

if (platform::httpGet(...) == 200) { ...classifyDevice... }

The projectMM /api/state probe should do the same. A 302 redirect, a 404 "Not Found", or a custom error page that happens to contain "modules" in its body (e.g. a router's error page listing available modules) will be misclassified as a projectMM device.

Fix:

if (status != 200) {
    // Host alive but not serving /api/state — fall through to WLED/generic.
} else {
    DevType t = classifyDevice(probeBuf_, nullptr);
    if (t == DevType::ProjectMM) { upsert(ip, t, probeBuf_); return true; }
}

---

2 — Duplicated list-row DOM construction in app.js (Medium)

File: src/ui/app.js

createControl() and updateModuleControls() each contain ~30 lines of nearly-identical list-row DOM construction code. A future bug fix in one path is guaranteed to be missed in the other. Extract the shared logic into a helper:

// Build list entries into `container` from rows+details arrays.
// Returns the Set of currently-open summary texts (for preserving expand state).
function buildListEntries(container, rows, details, openSet) { ... }

Then createControl calls buildListEntries(list, rows, details, new Set()) and updateModuleControls calls buildListEntries(list, rows, details, open).

---

3 — kMaxMissed comment is off-by-one (Low)

File: src/core/DevicesModule.h

static constexpr uint8_t kMaxMissed = 2;   // drop after this many empty sweeps

Actual behaviour: upsert initialises missed=0; ageOut increments at end of sweep and drops when missed >= kMaxMissed. So a device is dropped after 3 missed sweeps, not 2. The comment should read // drop when missed >= this (i.e. after 3 consecutive unseen sweeps) or the constant should be renamed kDropMissedThreshold.

---

4 — Self row always shows "this device" instead of deviceName (Low)

File: src/core/DevicesModule.h

if (!d->name[0]) std::snprintf(d->name, sizeof(d->name), "this device");

The sweep intentionally skips probing self (correct, prevents a loopback race). However, DevicesModule already lives inside NetworkModule, which already knows hostName(). The self-row name could be populated from networkModule_ / a passed-in name pointer at upsertSelf() time — or DevicesModule could accept a const char* selfName in its constructor. As-is the device's own card in the Devices list always reads "this device" while the status page title, router, and mDNS all show MM-70BC.

---

Agent prompt

You are an agent reviewing MoonModules/projectMM commit 2ef57f3 (PR `#21`).
Fix the following issues in priority order:

## Issue 1 — probePort: gate classifyDevice on HTTP 200 (Medium)
File: src/core/DevicesModule.h
Function: bool probePort(const uint8_t ip[4], uint16_t port)

The /api/state probe classifies the body without checking whether the HTTP
status is 200. A 404/500 error body that happens to contain "modules" is
wrongly classified as a projectMM device. The WLED branch already gates on
== 200; apply the same guard to the /api/state branch:

    int status = platform::httpGet(url, kProbeTimeoutMs, probeBuf_, sizeof(probeBuf_));
    if (status == 0) return false;   // nothing on this port
    if (status == 200) {
        DevType t = classifyDevice(probeBuf_, nullptr);
        if (t == DevType::ProjectMM) { upsert(ip, t, probeBuf_); return true; }
    }
    // Fall through: alive host, not a projectMM → try WLED / classify generic.

Leave the WLED branch (already == 200) and the generic fallback unchanged.

## Issue 2 — app.js: extract duplicated list-row DOM construction (Medium)
File: src/ui/app.js

createControl() (case "list":) and updateModuleControls() (case "list":) each
contain ~30 lines of near-identical list-row DOM construction. Extract the
shared rows-into-container builder into a single helper function:

    /**
     * Rebuild list entries inside `container` from `rows`/`details`.
     * `openSet` is a Set<string> of summary texts whose detail panels
     * should start expanded (pass `new Set()` for initial creation).
     */
    function buildListEntries(container, rows, details, openSet) {
        container.replaceChildren();
        if (rows.length === 0) {
            const empty = document.createElement("div");
            empty.className = "list-empty";
            empty.textContent = "(none)";
            container.appendChild(empty);
            return;
        }
        rows.forEach((item, i) => {
            const entry = document.createElement("div");
            entry.className = "list-entry" + (item && item.self ? " list-self" : "");
            const summary = document.createElement("div");
            summary.className = "list-summary";
            summary.tabIndex = 0;
            summary.setAttribute("role", "button");
            summary.textContent = listSummaryText(item);
            const detailPanel = document.createElement("div");
            detailPanel.className = "list-detail";
            detailPanel.hidden = !openSet.has(summary.textContent);
            fillListDetail(detailPanel, details[i] ?? item);
            const toggle = () => { detailPanel.hidden = !detailPanel.hidden; };
            summary.addEventListener("click", toggle);
            summary.addEventListener("keydown", (e) => {
                if (e.key === "Enter" || e.key === " ") { e.preventDefault(); toggle(); }
            });
            entry.append(summary, detailPanel);
            container.appendChild(entry);
        });
    }

Replace the duplicated code in createControl "list": with:
    buildListEntries(list, rows, details, new Set());

Replace the duplicated code in updateModuleControls "list": with:
    buildListEntries(list, rows, details, open);
    (where `open` is the existing Set of open summary texts)

## Issue 3 — kMaxMissed comment is off-by-one (Low)
File: src/core/DevicesModule.h
Near: static constexpr uint8_t kMaxMissed = 2;

Update the comment to accurately describe the drop threshold. The current
comment "drop after this many empty sweeps" implies 2 sweeps, but the actual
behaviour drops after 3 consecutive unseen sweeps (missed is initialised to 0
by upsert, incremented by ageOut at end of each sweep, and dropped when
missed >= kMaxMissed). Change the comment to:

    static constexpr uint8_t kMaxMissed = 2;   // drop when missed >= this;
                                                // a device seen on sweep N is
                                                // dropped after sweeps N+1, N+2, N+3
                                                // without a re-sighting (3 misses).

Do NOT change the value or any logic — only the comment.

## Issue 4 — self row shows "this device" instead of deviceName (Low)
File: src/core/DevicesModule.h
Function: void upsertSelf(const uint8_t ip[4])

Add a mechanism to pass the device's own name into DevicesModule so the self
row in the list shows the same name as the status page / router / mDNS. The
simplest approach: add a private member `const char* selfName_ = nullptr;`
and a setter the wiring code can call before setup:

    void setSelfName(const char* name) { selfName_ = name; }

Then in upsertSelf, replace the hardcoded fallback:

    if (!d->name[0]) {
        const char* n = selfName_ && selfName_[0] ? selfName_ : "this device";
        std::snprintf(d->name, sizeof(d->name), "%s", n);
    }

In main.cpp, after creating devicesModule and before adding it to the
scheduler, wire the name:
    devicesModule->setSelfName(networkModule->hostName());
  or if networkModule exposes systemModule_:
    devicesModule->setSelfName(systemModule->deviceName());

The "this device" literal becomes the last-resort fallback only when no name
is available at all (same robustness contract as the rest of the module).

[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)

CLAUDE.md (1)

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

Name the actual default field.

entry[0] reads like the board object is indexed; the PR-wide contract is that firmwares[0] is the default firmware.

Proposed wording fix

-8. Board catalog, `check_boards.py`, fast (<1s), if `docs/install/boards.json` or `scripts/check/check_boards.py` changed. Validates the installer catalog: required fields, `firmwares` a non-empty list of non-empty strings (entry[0] is the default), every `image` resolves on disk, each `Board.board` control equals its entry `name`, module `type`s are factory-registered (or boot-wired singletons), `pins` controls live only on `*LedDriver` modules, and `supported` capabilities stay within the known vocabulary.
+8. Board catalog, `check_boards.py`, fast (<1s), if `docs/install/boards.json` or `scripts/check/check_boards.py` changed. Validates the installer catalog: required fields, `firmwares` a non-empty list of non-empty strings (`firmwares[0]` is the default), every `image` resolves on disk, each `Board.board` control equals its entry `name`, module `type`s are factory-registered (or boot-wired singletons), `pins` controls live only on `*LedDriver` modules, and `supported` capabilities stay within the known vocabulary.

🤖 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 114, The documentation in CLAUDE.md references "entry[0]"
as the default firmware, but this is unclear and doesn't reflect the actual
field name used in the PR contract. Replace "entry[0] is the default" with
"firmwares[0] is the default" to use the explicit field name that matches the
documented contract, making the reference clearer and more accurate.

🤖 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/core/DevicesModule.md`:
- Line 10: Update the documentation in DevicesModule.md to accurately reflect
the implemented behavior of the progress control and List persistence. The
progress control documentation should be corrected to indicate that it remains
present at all times rather than being shown only during sweeps, as the
implementation maintains the control and manages it through value changes.
Additionally, the List documentation should be updated to reflect that it
supports persisted restore via recursive JSON parsing rather than being
flat-parser only, matching the actual implementation behavior demonstrated in
the code and tests.

In `@src/core/Control.cpp`:
- Around line 293-300: The ControlType::List case handler calls
ListSource::restoreList() but ignores its return value, always returning
ApplyResult::Ok regardless of whether the restore succeeded. Capture the return
value from the restoreList() call on the ListSource pointer, check whether it
indicates success, and return an appropriate failure result if the restoration
fails (such as ApplyResult::InvalidOperation or similar). This ensures that
failures from parsing malformed persisted lists or invalid live writes are
properly propagated instead of being masked as successful operations.

In `@src/core/Control.h`:
- Around line 100-121: The `ListSource` struct creates a secondary
virtual-method hierarchy outside the `MoonModule` tree, violating the coding
guideline that the module tree should be the only class hierarchy. Replace the
`ListSource` interface with a non-inheriting callback/context table or
plain-old-data (POD) adapter pattern — convert the virtual methods
(listRowCount, writeListRow, writeListRowDetail, restoreList) into function
pointers or a callback struct that `DevicesModule` populates and passes, rather
than inheriting from. This keeps the virtual-dispatch hierarchy contained to the
module tree alone.
- Around line 248-251: In the addList() method, change the parameter from const
ListSource& source to ListSource& source to match the non-const nature of the
source being passed. Then remove the const_cast<ListSource*>(&source) and
replace it with just &source, since the source will no longer be const and no
casting will be needed. This eliminates the const-correctness violation and
undefined behavior from casting away constness of a method that mutates the
object.

In `@src/core/DevicesModule.h`:
- Around line 231-235: The scanProgress_ member variable is not being reset when
the network check fails (the "no network" status case at lines 231-235) or after
sweep completion (at lines 264-271), leaving stale progress values visible in
the UI. Add a reset of scanProgress_ to its idle state (typically 0) in both
locations: first, immediately after setting the "no network" status in the early
abort block where local address checks fail, and second, in the sweep completion
block at lines 264-271 where scanning concludes normally. This ensures the
progress indicator returns to an idle state in all code paths where scanning is
not active.
- Line 8: The DevicesModule.h header file includes "platform/platform.h" which
violates the core/platform separation principle—core-layer code must remain
platform-independent with no direct platform includes. Remove the include of
"platform/platform.h" from DevicesModule.h and refactor any platform-specific
API invocations in this file to use platform-agnostic abstractions or delegate
platform calls to a separate platform adaptation layer. This issue appears at
the include statement and also at additional locations within the file where
platform APIs are invoked (at lines 281-283 and 308), so ensure all direct
platform API calls throughout the file are either removed or replaced with
platform-independent equivalents.
- Around line 351-367: The upsertSelf method has an issue where the existing
entry path (when a device entry is already found via findByIp) does not update
critical fields like type and speaks, causing stale cached values to persist.
The type field is only set to DevType::ProjectMM inside the if (!d) block for
new entries. To fix this, move the type assignment and any speaks field updates
outside of the if (!d) conditional block so they apply to both newly created and
existing device entries, ensuring that when upsertSelf is called, all identity
fields are properly refreshed regardless of whether the entry already existed.

In `@src/core/JsonUtil.h`:
- Around line 233-235: The numeric tail-skip loop in the while condition at line
233 is too permissive and accepts malformed JSON numbers like `1-2` or `1e+`.
Replace the single overly-broad condition with validation logic that properly
enforces JSON number syntax: allow digits and a single decimal point before any
exponent marker, and only allow sign characters (+/-) immediately following an
exponent marker (e/E), which must be followed by at least one digit. This
ensures that invalid numeric sequences are rejected rather than silently
consumed, preventing corrupted payloads from being treated as valid JSON.
- Around line 397-406: The function template forEachListElement creates a
separate static JsonDoc instance for each distinct callback type, multiplying
.bss usage despite the intent to have one resident copy. Extract the JSON
parsing logic and the static JsonDoc variable into a separate non-template
helper function (with a name like parseJsonList or similar) that performs the
parse operation and returns the result. Then update forEachListElement to call
this helper function instead of doing the parsing inline. This way the static
variable is instantiated only once and shared across all template
instantiations, regardless of the callback type.

In `@src/main.cpp`:
- Around line 279-282: The inline comment for the Devices module in main.cpp
uses future-tense wording with "May be promoted to a top-level module later"
which violates the guideline of describing current system behavior in present
tense only. Remove or rewrite the speculative phrase about future promotion to
instead describe only the current state and structure of the Devices module,
keeping the comment focused on what it is now rather than what it might become.

In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 510-516: The snprintf call in this HTTP request building code has
a truncation vulnerability where reqLen can exceed the 256-byte buffer size. The
current validation only checks if reqLen is non-positive or if send fails, but
does not detect when snprintf truncates the buffer. Add a length validation
check before calling send that ensures reqLen does not exceed sizeof(req) (256
bytes), rejecting the request if the formatted HTTP header would be truncated
due to an excessively long path parameter. This prevents reading past the stack
buffer bounds when send is called with reqLen.

In `@src/platform/platform.h`:
- Around line 183-185: The comment describing httpGet in src/platform/platform.h
(lines 183-185) claims the scan runs off the render hot path, but this
contradicts the actual implementation documented in docs/backlog/backlog.md Line
25, which shows the DevicesModule scan is blocking on the render task. Update
the comment in the httpGet function documentation to accurately describe the
current blocking behavior on the render task and include information about the
bounded blocking budget, ensuring the comment reflects the actual scan path
implementation rather than the stated off-hot-path design.

In `@src/ui/app.js`:
- Around line 1363-1375: The summary button element lacks the aria-expanded
attribute needed to expose its expanded/collapsed state to assistive
technologies. Initialize the aria-expanded attribute on the summary element
after creating it, setting it to the opposite value of the initial
detailPanel.hidden state. Then, update the toggle function to also set
summary.setAttribute("aria-expanded", !detailPanel.hidden) whenever the detail
panel visibility changes, ensuring the aria-expanded state stays in sync with
the actual visibility state of the detailPanel.

In `@src/ui/style.css`:
- Line 482: The CSS rule for `.list-detail-val` uses the deprecated `word-break:
break-word` property which is no longer standards-compliant. Replace the
`word-break: break-word` declaration with `overflow-wrap: anywhere` to achieve
the same text-wrapping behavior using modern, non-deprecated CSS syntax.

In `@test/scenarios/core/scenario_DevicesModule_scan.json`:
- Around line 5-6: The current scenario_DevicesModule_scan.json is live-only,
network-dependent, and timing-sensitive, which violates the coding guideline
that tests should not depend on timing or network. Create a parallel
deterministic test scenario that exercises the same scan functionality (scan
restart, classification, upsert, aging, and list serialization) using canned
httpGet responses instead of real network calls, allowing it to run in the
desktop runner without flakiness. Keep the existing live-only scenario for
manual hardware verification. Apply this change to both the anchor location
(line 5-6) and sibling location (line 50-52) by ensuring each scenario is
properly configured for its respective test type (deterministic vs. live-only).

---

Outside diff comments:
In `@CLAUDE.md`:
- Line 114: The documentation in CLAUDE.md references "entry[0]" as the default
firmware, but this is unclear and doesn't reflect the actual field name used in
the PR contract. Replace "entry[0] is the default" with "firmwares[0] is the
default" to use the explicit field name that matches the documented contract,
making the reference clearer and more accurate.

🪄 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: c83cb0af-d427-4266-8ab1-60c622c594de

📥 Commits

Reviewing files that changed from the base of the PR and between 04413b0 and 42e66c3.

📒 Files selected for processing (28)

• CLAUDE.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/install/index.html
• docs/moonmodules/core/DevicesModule.md
• esp32/main/CMakeLists.txt
• src/core/Control.cpp
• src/core/Control.h
• src/core/DeviceIdentify.h
• src/core/DevicesModule.h
• src/core/FilesystemModule.h
• src/core/JsonUtil.h
• src/core/NetworkModule.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/esp32/platform_esp32_httpget.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/scenarios/core/scenario_DevicesModule_scan.json
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/unit/core/unit_Control_list.cpp
• test/unit/core/unit_DeviceIdentify.cpp
• test/unit/core/unit_JsonUtil_parse.cpp

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Avoid adding a second virtual-dispatch hierarchy.

ListSource introduces virtual methods outside the MoonModule tree, and DevicesModule now overrides that interface. Keep the generic list source as a non-inheriting callback/context table or another POD adapter so the module tree remains the only class hierarchy. As per coding guidelines, “The one deliberate class hierarchy is the module tree… Do not add inheritance elsewhere.”

🤖 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/Control.h` around lines 100 - 121, The `ListSource` struct creates a
secondary virtual-method hierarchy outside the `MoonModule` tree, violating the
coding guideline that the module tree should be the only class hierarchy.
Replace the `ListSource` interface with a non-inheriting callback/context table
or plain-old-data (POD) adapter pattern — convert the virtual methods
(listRowCount, writeListRow, writeListRowDetail, restoreList) into function
pointers or a callback struct that `DevicesModule` populates and passes, rather
than inheriting from. This keeps the virtual-dispatch hierarchy contained to the
module tree alone.

Source: Coding guidelines

[coderabbitai]

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

Keep DevicesModule core-layer code platform-independent.

src/core/DevicesModule.h directly includes and invokes platform APIs, which breaks the core/platform boundary and hard-couples this core module to platform implementations.

As per coding guidelines, src/core/** must be platform-independent and have no platform includes.

Also applies to: 281-283, 308-308

🤖 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` at line 8, The DevicesModule.h header file includes
"platform/platform.h" which violates the core/platform separation
principle—core-layer code must remain platform-independent with no direct
platform includes. Remove the include of "platform/platform.h" from
DevicesModule.h and refactor any platform-specific API invocations in this file
to use platform-agnostic abstractions or delegate platform calls to a separate
platform adaptation layer. This issue appears at the include statement and also
at additional locations within the file where platform APIs are invoked (at
lines 281-283 and 308), so ensure all direct platform API calls throughout the
file are either removed or replaced with platform-independent equivalents.

Source: Coding guidelines

[coderabbitai]

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

Make the scan coverage deterministic.

This scenario is live-only, depends on a real LAN, and asserts a timing-derived FPS percentage, so it is both skipped by the desktop runner and prone to network/load flake. Keep the hardware sweep as manual verification, but add a deterministic test path with canned httpGet responses that exercises scan restart, classification, upsert, aging, and list serialization without real network or timing. As per coding guidelines, test/** tests “should not depend on timing or network.”

Also applies to: 50-52

🤖 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/core/scenario_DevicesModule_scan.json` around lines 5 - 6, The
current scenario_DevicesModule_scan.json is live-only, network-dependent, and
timing-sensitive, which violates the coding guideline that tests should not
depend on timing or network. Create a parallel deterministic test scenario that
exercises the same scan functionality (scan restart, classification, upsert,
aging, and list serialization) using canned httpGet responses instead of real
network calls, allowing it to run in the desktop runner without flakiness. Keep
the existing live-only scenario for manual hardware verification. Apply this
change to both the anchor location (line 5-6) and sibling location (line 50-52)
by ensuring each scenario is properly configured for its respective test type
(deterministic vs. live-only).

Source: Coding guidelines

[coderabbitai]

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (3)

src/core/DevicesModule.h (2)

89-114: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix premature return in restoreList() so post-parse sorting executes.

At Line 89, return mm::json::forEachListElement(...) exits the function before Line 113, so restored entries never get sortByName().

Suggested fix

-        return mm::json::forEachListElement(json, key,
+        const bool ok = mm::json::forEachListElement(json, key,
             [&](const mm::json::JsonDoc& doc, const mm::json::JsonNode* el) {
                 if (deviceCount_ >= kMaxDevices) return;
                 if (mm::json::readBool(mm::json::member(doc, el, "self"))) return;  // skip persisted self
                 char ipStr[16] = {}, name[24] = {}, typeStr[12] = {};
                 mm::json::readString(mm::json::member(doc, el, "ip"), ipStr, sizeof(ipStr));
                 mm::json::readString(mm::json::member(doc, el, "name"), name, sizeof(name));
                 mm::json::readString(mm::json::member(doc, el, "type"), typeStr, sizeof(typeStr));
                 uint8_t octets[4];
                 if (!parseDottedQuad(ipStr, octets)) return;
                 Device& d = devices_[deviceCount_++];
                 std::memcpy(d.ip, octets, 4);
                 std::snprintf(d.name, sizeof(d.name), "%s", name);
                 d.type = (std::strcmp(typeStr, "projectMM") == 0) ? DevType::ProjectMM
                        : (std::strcmp(typeStr, "WLED") == 0)      ? DevType::Wled
                                                                   : DevType::Generic;
                 d.self = false;
                 d.speaks = ProtoHttp;
                 d.via = 0;        // no live sighting yet — via fills in when a strategy re-sees it
                 d.cached = true;  // restored, not re-confirmed live → UI shows "cached", not a time
                 // Stamp "now" so the cached entry gets a full kStaleMs grace window to
                 // re-announce before age-out drops it (a still-alive but slow device).
                 d.lastSeenMs = platform::millis();
             });
+        if (!ok) return false;
         sortByName();   // cached list shows alphabetically too, before the first sweep
         return true;

🤖 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 89 - 114, The restoreList() function
has a premature return statement that exits before the sortByName() call can
execute. Remove the return keyword from the mm::json::forEachListElement() call
so it simply executes the loop without early exit, allowing sortByName() on the
following line to sort the restored cached entries alphabetically, and then the
function returns true at the end.

---

327-336: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Advance scanProgress_ inside the sweep loop.

scanProgress_ is set at Line 313 and reset at Line 339, but never updated per host in Lines 327-336, so the progress bar does not reflect sweep progress.

Suggested fix

         for (uint8_t i = 0; i < kProbesPerTick && hostCursor_ <= 254; i++, hostCursor_++) {
+            scanProgress_ = static_cast<uint32_t>(hostCursor_);
             uint8_t ip[4] = {subnet_[0], subnet_[1], subnet_[2],
                              static_cast<uint8_t>(hostCursor_)};
             // Don't probe our own IP: upsertSelf already gave it the right identity
             // (projectMM, deviceName), and an HTTP request to ourselves mid-tick can
             // race the server / loopback and misclassify us as generic. Just keep 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 `@src/core/DevicesModule.h` around lines 327 - 336, The scanProgress_ variable
is initialized and reset but never incremented within the host scanning loop in
DevicesModule.h, causing the progress bar to not reflect sweep progress. Inside
the for loop that iterates through hosts (the loop containing the ipEq check and
probe call), increment scanProgress_ for each host processed to track the
scanning progress. This should be done for each iteration regardless of whether
the host is skipped via the continue statement or actually probed, so that the
progress bar advances consistently as the sweep progresses.

docs/moonmodules/core/DevicesModule.md (1)

25-30: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align probe table with the implemented HTTP status gating.

At Line 27 and Line 28, the table implies body-only matching; implementation classifies projectMM/WLED only when HTTP status is 200.

Suggested fix

-| `GET /api/state` | body contains `"modules"` | projectMM |
-| `GET /json/info` | body contains `WLED` | WLED |
+| `GET /api/state` | status `200` and body contains `"modules"` | projectMM |
+| `GET /json/info` | status `200` and body contains `WLED` | WLED |

🤖 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/DevicesModule.md` around lines 25 - 30, The probe
matching table in the Devices Module documentation currently describes the
projectMM and WLED detection criteria as body-only matching (checking if the
response body contains specific strings), but the actual implementation gates
these matches on HTTP status code 200 in addition to the body content check.
Update the Match column descriptions for the projectMM probe (GET /api/state)
and WLED probe (GET /json/info) rows to clarify that HTTP status 200 is a
required condition along with the body content match, ensuring the documentation
accurately reflects the implementation behavior.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 963-967: The code in the mdns_service_add block assumes any
failure from mdns_service_add means the service already exists, but the function
can fail for other reasons like OOM or invalid state. Instead of unconditionally
calling mdns_service_instance_name_set on failure, first check if the service
actually exists using mdns_service_exists before attempting to update the
instance name, or alternatively explicitly remove the service with
mdns_service_remove before adding it. This ensures the mDNS service is only
reported as started (the log message and mdnsRunning_ flag) when the service is
actually successfully registered or refreshed, not when it fails for unrelated
reasons.

In `@test/unit/core/unit_DevicesModule_ageout.cpp`:
- Around line 45-59: The test clock restoration at the end of `aPresentAfter()`
using `platform::setTestNowMs(0)` is not failure-safe. If either REQUIRE check
at line 53 or line 54 fails, the function will exit without executing the clock
restoration, leaving the test clock in a polluted state for subsequent tests.
Implement a failure-safe cleanup mechanism such as a scope guard object or RAII
wrapper that guarantees the call to `platform::setTestNowMs(0)` executes
regardless of whether REQUIRE assertions succeed or fail, ensuring the test
clock is always restored to its initial state before the function returns.

---

Outside diff comments:
In `@docs/moonmodules/core/DevicesModule.md`:
- Around line 25-30: The probe matching table in the Devices Module
documentation currently describes the projectMM and WLED detection criteria as
body-only matching (checking if the response body contains specific strings),
but the actual implementation gates these matches on HTTP status code 200 in
addition to the body content check. Update the Match column descriptions for the
projectMM probe (GET /api/state) and WLED probe (GET /json/info) rows to clarify
that HTTP status 200 is a required condition along with the body content match,
ensuring the documentation accurately reflects the implementation behavior.

In `@src/core/DevicesModule.h`:
- Around line 89-114: The restoreList() function has a premature return
statement that exits before the sortByName() call can execute. Remove the return
keyword from the mm::json::forEachListElement() call so it simply executes the
loop without early exit, allowing sortByName() on the following line to sort the
restored cached entries alphabetically, and then the function returns true at
the end.
- Around line 327-336: The scanProgress_ variable is initialized and reset but
never incremented within the host scanning loop in DevicesModule.h, causing the
progress bar to not reflect sweep progress. Inside the for loop that iterates
through hosts (the loop containing the ipEq check and probe call), increment
scanProgress_ for each host processed to track the scanning progress. This
should be done for each iteration regardless of whether the host is skipped via
the continue statement or actually probed, so that the progress bar advances
consistently as the sweep progresses.

🪄 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: 651eba90-e99b-4682-a331-173ecc39e1f7

📥 Commits

Reviewing files that changed from the base of the PR and between 42e66c3 and 6bb1a77.

📒 Files selected for processing (22)

• CLAUDE.md
• docs/backlog/backlog.md
• docs/moonmodules/core/DevicesModule.md
• src/core/Control.cpp
• src/core/Control.h
• src/core/DevicesModule.h
• src/core/JsonUtil.h
• src/core/NetworkModule.h
• src/core/Sort.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/scenarios/light/scenario_Driver_mutation.json
• test/unit/core/unit_Control_list.cpp
• test/unit/core/unit_DevicesModule_ageout.cpp
• test/unit/core/unit_Sort.cpp

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (3)

docs/moonmodules/light/drivers/RmtLedDriver.md (1)

28-29: ⚠️ Potential issue | 🟡 Minor

The documentation accurately reflects the current code, but the loopback pin controls diverge from the standard pin-control pattern.

loopbackTxPin and loopbackRxPin are correctly documented as uint16_t in the spec — they match the member variables in the header and are registered via controls_.addUint16(). However, the standard pin control pattern elsewhere (defined in Control.h::addPin()) uses int8_t storage and ControlType::Pin. The loopback pins intentionally use uint16_t and addUint16() instead, which is a deliberate design choice. If this divergence is intentional, add a comment in the header explaining why; if unintentional, unify them with the standard addPin() pattern and update the spec accordingly.

🤖 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/RmtLedDriver.md` around lines 28 - 29, The
loopback pin controls loopbackTxPin and loopbackRxPin intentionally diverge from
the standard pin control pattern by using uint16_t and addUint16() instead of
the standard int8_t and addPin() approach defined in Control.h. Clarify this
design decision by adding an explanatory comment in the header file where
loopbackTxPin and loopbackRxPin are registered, documenting why they use the
uint16_t pattern instead of adhering to the standard addPin() convention used
elsewhere in the codebase. Alternatively, if this divergence was unintentional,
refactor these two controls to use the standard addPin() pattern and update the
documentation to reflect int8_t as the type instead of uint16_t.

docs/moonmodules/core/DevicesModule.md (2)

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

Fix the progress range.

progress starts at 0 while idle, so 1..254 is off by one and misstates the control.

🐛 Suggested edit

-- `progress` — a progress bar of the sweep position (host 1..254). Always present (the WebSocket state push patches control *values* but not *structure*, so a hide-while-idle flag would not update live); at rest its value is 0 (empty bar).
+- `progress` — a progress bar of the sweep position (host 0..254). Always present (the WebSocket state push patches control *values* but not *structure*, so a hide-while-idle flag would not update live); at rest its value is 0 (empty bar).

🤖 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/DevicesModule.md` at line 10, The progress bar range
documentation for the DevicesModule states the sweep position is 1..254, but the
text clarifies that the value is 0 when idle, indicating the actual range should
be 0..254. Update the documented progress range from 1..254 to 0..254 to
accurately reflect the full range of values the progress bar can display.

---

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

Keep roadmap notes out of the shipped doc.

This page should stay in present tense; the backlog/planned-next notes belong in docs/backlog/ instead.

🧹 Suggested edit

- Moving the probe to its own task (the enabler for safe periodic sweeping + a UDP presence beacon) is in the backlog.
...
- A UDP beacon / ArtPoll listener is the planned next non-HTTP strategy.

As per coding guidelines: **/*.md uses present tense only, and roadmaps live only in docs/backlog/ or docs/history/.

Also applies to: 40-40

🤖 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/DevicesModule.md` at line 21, The DevicesModule.md
documentation contains a sentence describing future work ("Moving the probe to
its own task... is in the backlog") which violates the guideline that shipped
documentation should remain in present tense and roadmap/backlog items should
only exist in docs/backlog/ or docs/history/. Remove the sentence about moving
the probe to its own task and the backlog reference from the subnet sweep
description, keeping only the current-state technical details about how the
sweep operates. Apply the same fix to line 40 as mentioned in the comment.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/moonmodules/light/drivers/RmtLedDriver.md`:
- Around line 22-23: The RmtLedDriver.md file contains a future-work note about
dedicated core-1 driver tasks and core affinity that describes planned
improvements rather than current functionality. Remove the sentence beginning
with "A dedicated core-1 driver task..." from the end of the loop() description,
as module documentation should describe the system as it currently exists in
present tense. The deferred functionality is already documented in
docs/backlog/leddriver-deferred.md, so this future-work note does not belong in
the shipped module specification.

In `@docs/moonmodules/light/modifiers/RandomMapModifier.md`:
- Around line 5-6: Remove the word "first" from the description of
RandomMapModifier since it is no longer accurate. The sentence currently states
"This is the first **dynamic** modifier" but RotateModifier.md also introduces a
dynamic modifier in this PR. Update the sentence to simply describe
RandomMapModifier as a dynamic modifier without claiming it is the first one.

In `@src/light/modifiers/RotateModifier.h`:
- Around line 92-93: The comment describing the rotation timing in
RotateModifier.h incorrectly calculates the duration of one full turn. The
comment states the duration is (256*256/speed) ms, but the actual code uses step
= phaseNum_ >> 6, which divides by 64 rather than 256. Correct the comment to
reflect the actual timing: one full turn requires 256 steps times 64 units per
step, totaling 16384 units, which takes 16384/speed ms (or equivalently
256*64/speed ms) at speed S. This is 4 times shorter than what the current
comment states.

---

Outside diff comments:
In `@docs/moonmodules/core/DevicesModule.md`:
- Line 10: The progress bar range documentation for the DevicesModule states the
sweep position is 1..254, but the text clarifies that the value is 0 when idle,
indicating the actual range should be 0..254. Update the documented progress
range from 1..254 to 0..254 to accurately reflect the full range of values the
progress bar can display.
- Line 21: The DevicesModule.md documentation contains a sentence describing
future work ("Moving the probe to its own task... is in the backlog") which
violates the guideline that shipped documentation should remain in present tense
and roadmap/backlog items should only exist in docs/backlog/ or docs/history/.
Remove the sentence about moving the probe to its own task and the backlog
reference from the subnet sweep description, keeping only the current-state
technical details about how the sweep operates. Apply the same fix to line 40 as
mentioned in the comment.

In `@docs/moonmodules/light/drivers/RmtLedDriver.md`:
- Around line 28-29: The loopback pin controls loopbackTxPin and loopbackRxPin
intentionally diverge from the standard pin control pattern by using uint16_t
and addUint16() instead of the standard int8_t and addPin() approach defined in
Control.h. Clarify this design decision by adding an explanatory comment in the
header file where loopbackTxPin and loopbackRxPin are registered, documenting
why they use the uint16_t pattern instead of adhering to the standard addPin()
convention used elsewhere in the codebase. Alternatively, if this divergence was
unintentional, refactor these two controls to use the standard addPin() pattern
and update the documentation to reflect int8_t as the type instead of uint16_t.

🪄 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: 51e1636e-4d52-46c4-8473-374e959d57ae

📥 Commits

Reviewing files that changed from the base of the PR and between 6bb1a77 and 97bf3ca.

📒 Files selected for processing (42)

• CLAUDE.md
• docs/backlog/README.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/backlog/leddriver-deferred.md
• docs/backlog/leddriver-increment-1-plan.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/backlog/moonmodules_draft/.gitkeep
• docs/backlog/moonmodules_draft/core/ui.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/backlog/ui-deferred.md
• docs/history/README.md
• docs/history/decisions.md
• docs/history/repo-transfer.md
• docs/history/v1-inventory.md
• docs/moonmodules/core/DevicesModule.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/DistortionWavesEffect.md
• docs/moonmodules/light/effects/SineEffect.md
• docs/moonmodules/light/layouts/WheelLayout.md
• docs/moonmodules/light/modifiers/RandomMapModifier.md
• docs/moonmodules/light/modifiers/RotateModifier.md
• src/core/DevicesModule.h
• src/light/effects/DistortionWavesEffect.h
• src/light/effects/SineEffect.h
• src/light/layers/Layer.h
• src/light/layouts/WheelLayout.h
• src/light/modifiers/ModifierBase.h
• src/light/modifiers/RandomMapModifier.h
• src/light/modifiers/RotateModifier.h
• src/main.cpp
• src/platform/esp32/platform_esp32.cpp
• test/CMakeLists.txt
• test/unit/core/unit_DevicesModule_ageout.cpp
• test/unit/light/unit_DistortionWavesEffect.cpp
• test/unit/light/unit_RandomMapModifier.cpp
• test/unit/light/unit_RotateModifier.cpp
• test/unit/light/unit_SineEffect.cpp
• test/unit/light/unit_WheelLayout.cpp

💤 Files with no reviewable changes (10)

• docs/backlog/moonmodules_draft/light/effects/SineEffect.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/backlog/moonmodules_draft/light/modifiers/RotateModifier.md
• docs/history/repo-transfer.md
• docs/backlog/installer-3layer-plan.md
• docs/backlog/moonmodules_draft/light/layouts/WheelLayout.md
• docs/backlog/moonmodules_draft/light/effects/DistortionWavesEffect.md
• docs/backlog/leddriver-increment-1-plan.md
• docs/backlog/moonmodules_draft/core/ui.md
• docs/history/README.md

[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 (4)

docs/testing.md (1)

27-27: ⚠️ Potential issue | 🟡 Minor

Add subject to sentence fragment for clarity.

The phrase "May be empty or shorter than pins" lacks an explicit subject. Reword to either "It may be empty or shorter than pins" or rewrite as "When empty, the value must match pins by position" to improve clarity.

🤖 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/testing.md` at line 27, The sentence fragment "May be empty or shorter
than `pins`" lacks an explicit subject and needs to be reworded for clarity. Add
a subject to complete the sentence by either changing it to "It may be empty or
shorter than `pins`" or rewriting it as "When empty, the value must match `pins`
by position" to improve readability and grammatical correctness.

src/core/AudioModule.h (1)

58-61: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale pin-sentinel comments to match current behavior.

Line 58 still documents unset as 0, and Line 200 says GPIO 0 is invalid, but this code now uses -1 as unset and keeps GPIO 0 usable.

🛠 Proposed fix

-    // two read-only read-outs. The pins default to UNSET (0): the module is user-
+    // two read-only read-outs. The pins default to UNSET (-1): the module is user-
@@
-        // attempt an I2S init. A 0 GPIO is not a valid mic pin, and initialising
+        // attempt an I2S init. An unset pin (<0) is invalid; GPIO 0 is valid, and initialising

As per coding guidelines, comments should only be removed/changed when outdated or factually wrong; these are now factually wrong.

Also applies to: 200-201

🤖 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 58 - 61, Update the outdated comments in
AudioModule.h that document pin sentinel values and GPIO 0 constraints. The
comment describing the module behavior needs to be changed from stating pins
default to UNSET as 0 to correctly reflect that the code now uses -1 as the
unset sentinel value. Additionally, the comment around line 200 that says GPIO 0
is invalid should be updated to reflect that GPIO 0 is actually usable in the
current implementation. Replace the factually incorrect statements with accurate
descriptions of the current behavior where -1 represents unset and GPIO 0 is a
valid pin option.

Source: Coding guidelines

src/light/drivers/RmtLedDriver.h (2)

369-376: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard unset loopbackRxPin before hardware loopback call.

Line 375 converts the unset sentinel (-1) to 255, so loopback still executes with an invalid GPIO instead of short-circuiting the test path.

Proposed fix

         // The test reconfigures the first data pin as TX, so release ALL our TX
         // channels first — this also guarantees the test's RX channel can always
         // allocate RMT memory, even with every TX channel otherwise claimed;
         // reinit() restores them after.
+        if (loopbackRxPin < 0) {
+            clearFailBuf();
+            setStatus("loopback: set loopbackRxPin", Severity::Warning);
+            return;
+        }
         deinitAll();
         // TX override: when loopbackTxPin is set, transmit on it instead of the
         // first data pin, so the bench loopback runs on a dedicated jumper without
         // re-typing `pins`. Falls back to pins[0] when unset.
         const uint8_t txPin = loopbackTxPin >= 0
             ? static_cast<uint8_t>(loopbackTxPin)
             : static_cast<uint8_t>(pinList_[0]);
         const uint8_t rxPin = static_cast<uint8_t>(loopbackRxPin);

🤖 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/RmtLedDriver.h` around lines 369 - 376, The loopbackRxPin
is being unconditionally cast to uint8_t on line 375, which converts the unset
sentinel value of -1 to 255, resulting in an invalid GPIO pin being used for the
hardware loopback call. Add a guard condition before the
platform::RmtLoopbackResult initialization to check if loopbackRxPin is set (not
equal to -1). Only proceed with the loopback execution if loopbackRxPin has a
valid value, otherwise skip or short-circuit the test path similar to how
loopbackTxPin is conditionally handled with the ternary operator.

---

102-107: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the loopbackTxPin sentinel comment to match current behavior.

Line 106 still says 0 = transmit on pins[0], but the new logic treats -1 as unset and 0 as a valid explicit GPIO override.

Proposed fix

-        // flips the flag. txPin is the optional override (0 = transmit on pins[0]).
+        // flips the flag. txPin is the optional override
+        // (-1 = use pins[0], >=0 = force that GPIO).

As per coding guidelines, comments should be corrected when they become 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/drivers/RmtLedDriver.h` around lines 102 - 107, Update the comment
for txPin in the RmtLedDriver.h file (located near the loopbackTxPin control
addition) to reflect the current sentinel value behavior. The comment currently
states "0 = transmit on pins[0]" but the new logic treats -1 as the unset
sentinel value and 0 as a valid explicit GPIO override. Revise the comment to
accurately describe that -1 indicates an unset/default state and 0 is now a
valid explicit GPIO pin override, not the default behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@src/light/layers/Layer.h`:
- Around line 155-159: The loop iterating through children starting at line 155
ticks all enabled modifiers, but the rebuildLUT() method (lines 223-229) only
applies the first enabled modifier to the lookup table. This creates a semantic
mismatch where non-first modifiers can trigger rebuilds that never use their
mapping. Modify the loop that checks child(i)->role() and child(i)->enabled() to
only tick the first enabled modifier with ModuleRole::Modifier role, stopping
after processing that first one, so the ticking behavior aligns with how
rebuildLUT() actually applies modifiers.

In `@src/light/modifiers/RandomMapModifier.h`:
- Around line 96-97: Remove the `lyr->onBuildState()` call from the render loop
path in RandomMapModifier to avoid triggering full Layer rebuilds with memory
allocation/deallocation during the render tick. The `reshuffle()` call on the
preceding line is sufficient to invalidate the LUT for the next rebuild cycle
without forcing an immediate full rebuild from the modifier's `loop()` method,
which violates render-loop memory allocation guidelines. Simply delete the
onBuildState() line to defer the actual rebuild until a safer, non-render-path
context.

In `@src/main.cpp`:
- Around line 37-45: The preprocessor directives checking
CONFIG_SOC_RMT_SUPPORTED, CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED, and
CONFIG_SOC_PARLIO_SUPPORTED in the include sections are leaking
platform-specific branching logic into main.cpp, which violates the architecture
rules. Instead of using `#if` defined(CONFIG_SOC_...) blocks, define corresponding
compile-time boolean flags in platform_config.h (e.g., PLATFORM_SUPPORTS_RMT,
PLATFORM_SUPPORTS_LCD_LED_DRIVER, PLATFORM_SUPPORTS_PARLO_LED_DRIVER) and
replace the preprocessor conditional includes with if constexpr statements that
check these flags, ensuring all driver selection logic is abstracted through the
platform configuration system rather than scattered across main.cpp.

In `@test/unit/light/unit_RandomMapModifier.cpp`:
- Around line 74-119: Add a new test case after the existing tests to cover the
loop() method's timer-driven behavior. The test should create a
RandomMapModifier instance and call loop() with time values that cross a beat
boundary, testing both when bpm==0 (no reshuffling) and when bpm is set to a
positive value. Verify that when a beat boundary is crossed, the reshuffle
occurs and the mapping changes, while tracking that the internal phaseNum_ and
lastBeat_ state variables are properly updated. Also verify that calling loop()
with bpm==0 does not trigger a reshuffle even when time advances significantly,
confirming the timer logic correctly controls when dynamic remapping happens.

---

Outside diff comments:
In `@docs/testing.md`:
- Line 27: The sentence fragment "May be empty or shorter than `pins`" lacks an
explicit subject and needs to be reworded for clarity. Add a subject to complete
the sentence by either changing it to "It may be empty or shorter than `pins`"
or rewriting it as "When empty, the value must match `pins` by position" to
improve readability and grammatical correctness.

In `@src/core/AudioModule.h`:
- Around line 58-61: Update the outdated comments in AudioModule.h that document
pin sentinel values and GPIO 0 constraints. The comment describing the module
behavior needs to be changed from stating pins default to UNSET as 0 to
correctly reflect that the code now uses -1 as the unset sentinel value.
Additionally, the comment around line 200 that says GPIO 0 is invalid should be
updated to reflect that GPIO 0 is actually usable in the current implementation.
Replace the factually incorrect statements with accurate descriptions of the
current behavior where -1 represents unset and GPIO 0 is a valid pin option.

In `@src/light/drivers/RmtLedDriver.h`:
- Around line 369-376: The loopbackRxPin is being unconditionally cast to
uint8_t on line 375, which converts the unset sentinel value of -1 to 255,
resulting in an invalid GPIO pin being used for the hardware loopback call. Add
a guard condition before the platform::RmtLoopbackResult initialization to check
if loopbackRxPin is set (not equal to -1). Only proceed with the loopback
execution if loopbackRxPin has a valid value, otherwise skip or short-circuit
the test path similar to how loopbackTxPin is conditionally handled with the
ternary operator.
- Around line 102-107: Update the comment for txPin in the RmtLedDriver.h file
(located near the loopbackTxPin control addition) to reflect the current
sentinel value behavior. The comment currently states "0 = transmit on pins[0]"
but the new logic treats -1 as the unset sentinel value and 0 as a valid
explicit GPIO override. Revise the comment to accurately describe that -1
indicates an unset/default state and 0 is now a valid explicit GPIO pin
override, not the default 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: 9adc4583-a6aa-4a5a-aaf9-e7a45790a931

📥 Commits

Reviewing files that changed from the base of the PR and between 6bb1a77 and a708ef2.

📒 Files selected for processing (63)

• .gitignore
• CLAUDE.md
• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog.md
• docs/backlog/installer-3layer-plan.md
• docs/backlog/leddriver-deferred.md
• docs/backlog/leddriver-increment-1-plan.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/backlog/moonmodules_draft/.gitkeep
• docs/backlog/moonmodules_draft/core/ui.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/backlog/ui-deferred.md
• docs/history/README.md
• docs/history/decisions.md
• docs/history/repo-transfer.md
• docs/history/v1-inventory.md
• docs/moonmodules/core/AudioModule.md
• docs/moonmodules/core/DevicesModule.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/ParlioLedDriver.md
• docs/moonmodules/light/drivers/RmtLedDriver.md
• docs/moonmodules/light/effects/DistortionWavesEffect.md
• docs/moonmodules/light/effects/SineEffect.md
• docs/moonmodules/light/layouts/WheelLayout.md
• docs/moonmodules/light/modifiers/RandomMapModifier.md
• docs/moonmodules/light/modifiers/RotateModifier.md
• docs/performance.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• src/core/AudioModule.h
• src/core/DevicesModule.h
• src/light/drivers/LcdLedDriver.h
• src/light/drivers/ParallelLedDriver.h
• src/light/drivers/ParlioLedDriver.h
• src/light/drivers/RmtLedDriver.h
• src/light/effects/DistortionWavesEffect.h
• src/light/effects/SineEffect.h
• src/light/layers/Layer.h
• src/light/layouts/WheelLayout.h
• src/light/modifiers/ModifierBase.h
• src/light/modifiers/RandomMapModifier.h
• src/light/modifiers/RotateModifier.h
• src/main.cpp
• src/platform/esp32/platform_esp32.cpp
• test/CMakeLists.txt
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• test/scenarios/light/scenario_Layer_buildup.json
• test/scenarios/light/scenario_perf_full.json
• test/scenarios/light/scenario_perf_light.json
• test/unit/core/unit_DevicesModule_ageout.cpp
• test/unit/light/unit_AudioModule.cpp
• test/unit/light/unit_DistortionWavesEffect.cpp
• test/unit/light/unit_RandomMapModifier.cpp
• test/unit/light/unit_RotateModifier.cpp
• test/unit/light/unit_SineEffect.cpp
• test/unit/light/unit_WheelLayout.cpp

💤 Files with no reviewable changes (12)

• docs/backlog/installer-3layer-plan.md
• test/scenarios/light/scenario_Layer_buildup.json
• docs/backlog/moonmodules_draft/light/layouts/WheelLayout.md
• docs/backlog/moonmodules_draft/light/effects/SineEffect.md
• docs/backlog/leddriver-increment-2-plan.md
• docs/history/README.md
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• docs/backlog/moonmodules_draft/light/effects/DistortionWavesEffect.md
• docs/backlog/leddriver-increment-1-plan.md
• docs/backlog/moonmodules_draft/core/ui.md
• docs/history/repo-transfer.md
• docs/backlog/moonmodules_draft/light/modifiers/RotateModifier.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Ticking all modifiers conflicts with first-modifier LUT semantics.

Line 155 loops every enabled modifier, but rebuildLUT() (Lines 223-229) applies only the first enabled modifier. Non-first dynamic modifiers can still force rebuilds that never use their mapping.

Proposed fix (align loop semantics with rebuildLUT)

-        for (uint8_t i = 0; i < childCount(); i++) {
-            if (child(i)->role() != ModuleRole::Modifier) continue;
-            if (!child(i)->enabled()) continue;
-            static_cast<ModifierBase*>(child(i))->loop();
-        }
+        for (uint8_t i = 0; i < childCount(); i++) {
+            if (child(i)->role() != ModuleRole::Modifier) continue;
+            if (!child(i)->enabled()) continue;
+            static_cast<ModifierBase*>(child(i))->loop();
+            break;  // rebuildLUT() also uses first enabled modifier
+        }

📝 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

	for (uint8_t i = 0; i < childCount(); i++) {
	if (child(i)->role() != ModuleRole::Modifier) continue;
	if (!child(i)->enabled()) continue;
	static_cast<ModifierBase*>(child(i))->loop();
	}
	for (uint8_t i = 0; i < childCount(); i++) {
	if (child(i)->role() != ModuleRole::Modifier) continue;
	if (!child(i)->enabled()) continue;
	static_cast<ModifierBase*>(child(i))->loop();
	break; // rebuildLUT() also uses first enabled modifier
	}

🤖 Prompt for AI Agents

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

In `@src/light/layers/Layer.h` around lines 155 - 159, The loop iterating through
children starting at line 155 ticks all enabled modifiers, but the rebuildLUT()
method (lines 223-229) only applies the first enabled modifier to the lookup
table. This creates a semantic mismatch where non-first modifiers can trigger
rebuilds that never use their mapping. Modify the loop that checks
child(i)->role() and child(i)->enabled() to only tick the first enabled modifier
with ModuleRole::Modifier role, stopping after processing that first one, so the
ticking behavior aligns with how rebuildLUT() actually applies modifiers.

[coderabbitai]

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

Avoid full onBuildState() rebuilds from modifier loop().

Line 97 triggers a full Layer rebuild from the render tick path (Layer::loop() → ModifierBase::loop()), which can allocate/free LUT and buffer memory and introduce beat-time stalls.

As per coding guidelines, “In the render loop and anything it calls: no heap allocations … no blocking …”.

🤖 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 96 - 97, Remove the
`lyr->onBuildState()` call from the render loop path in RandomMapModifier to
avoid triggering full Layer rebuilds with memory allocation/deallocation during
the render tick. The `reshuffle()` call on the preceding line is sufficient to
invalidate the LUT for the next rebuild cycle without forcing an immediate full
rebuild from the modifier's `loop()` method, which violates render-loop memory
allocation guidelines. Simply delete the onBuildState() line to defer the actual
rebuild until a safer, non-render-path context.

Source: Coding guidelines

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Replace preprocessor SOC branching in main.cpp with platform_config-driven compile-time wiring.

These #if defined(CONFIG_SOC_...) blocks leak platform branching into src/main.cpp. This should be routed through platform_config.h flags with if constexpr (and, if needed, a small registry indirection) so platform selection stays aligned with repo architecture rules.

As per coding guidelines, "Use if constexpr on platform_config.h flags for compile-time platform branching; no #ifdef, platform-specific #include, or hardware API call outside src/platform/."

Also applies to: 104-112

🤖 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 37 - 45, The preprocessor directives checking
CONFIG_SOC_RMT_SUPPORTED, CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED, and
CONFIG_SOC_PARLIO_SUPPORTED in the include sections are leaking
platform-specific branching logic into main.cpp, which violates the architecture
rules. Instead of using `#if` defined(CONFIG_SOC_...) blocks, define corresponding
compile-time boolean flags in platform_config.h (e.g., PLATFORM_SUPPORTS_RMT,
PLATFORM_SUPPORTS_LCD_LED_DRIVER, PLATFORM_SUPPORTS_PARLO_LED_DRIVER) and
replace the preprocessor conditional includes with if constexpr statements that
check these flags, ensuring all driver selection logic is abstracted through the
platform configuration system rather than scattered across main.cpp.

Source: Coding guidelines

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Add direct coverage for the loop() timer path (bpm==0 and beat crossing).

Current cases validate reshuffle() and mapping properties, but not the runtime timer logic (phaseNum_/lastBeat_) that actually drives dynamic behavior.

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

🧰 Tools

🪛 Clang (14.0.6)

[warning] 75-75: multiple declarations in a single statement reduces readability

(readability-isolate-declaration)

---

[warning] 75-75: variable name 'a' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 75-75: variable name 'b' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 76-76: variable name 'da' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 77-77: variable name 'db' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 83-83: variable name 'm' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 84-84: multiple declarations in a single statement reduces readability

(readability-isolate-declaration)

---

[warning] 84-84: variable name 'w' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 84-84: variable name 'h' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 84-84: variable name 'd' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 85-85: variable name 'n' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 95-95: statement should be inside braces

(readability-braces-around-statements)

---

[warning] 100-100: variable name 'm' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 101-101: do not declare C-style arrays, use std::array<> instead

(modernize-avoid-c-arrays)

---

[warning] 108-108: function 'DOCTEST_ANON_FUNC_14' has cognitive complexity of 28 (threshold 25)

(readability-function-cognitive-complexity)

---

[note] 111-111: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 111-111: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 111-111: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 114-114: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 114-114: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 114-114: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 117-117: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 117-117: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 117-117: +3, including nesting penalty of 2, nesting level increased to 3

(clang)

---

[note] 117-117: +3, including nesting penalty of 2, nesting level increased to 3

(clang)

---

[note] 118-118: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 118-118: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 118-118: +3, including nesting penalty of 2, nesting level increased to 3

(clang)

---

[note] 118-118: +3, including nesting penalty of 2, nesting level increased to 3

(clang)

---

[warning] 109-109: variable name 'm' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 115-115: variable name 'n' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 118-118: statement should be inside braces

(readability-braces-around-statements)

🪛 Cppcheck (2.21.0)

[style] 110-110: The function 'rebuildControls' is never used.

(unusedFunction)

---

[style] 113-113: The function 'hasLUT' is never used.

(unusedFunction)

---

[style] 114-114: The function 'isPaged' is never used.

(unusedFunction)

---

[style] 115-115: The function 'logicalCount' is never used.

(unusedFunction)

---

[style] 116-116: The function 'destinationCount' is never used.

(unusedFunction)

---

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

(unusedFunction)

---

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

(unusedFunction)

---

[style] 82-82: The function 'DOCTEST_ANON_FUNC_27' is never used.

(unusedFunction)

---

[style] 82-82: The function 'DOCTEST_ANON_FUNC_42' is never used.

(unusedFunction)

---

[style] 99-99: The function 'DOCTEST_ANON_FUNC_32' is never used.

(unusedFunction)

---

[style] 99-99: The function 'DOCTEST_ANON_FUNC_47' is never used.

(unusedFunction)

---

[style] 99-99: The function 'DOCTEST_ANON_FUNC_5' is never used.

(unusedFunction)

---

[style] 108-108: The function 'DOCTEST_ANON_FUNC_37' is never used.

(unusedFunction)

---

[style] 108-108: The function 'DOCTEST_ANON_FUNC_52' is never used.

(unusedFunction)

---

[style] 108-108: The function 'DOCTEST_ANON_FUNC_6' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_RandomMapModifier.cpp` around lines 74 - 119, Add a new
test case after the existing tests to cover the loop() method's timer-driven
behavior. The test should create a RandomMapModifier instance and call loop()
with time values that cross a beat boundary, testing both when bpm==0 (no
reshuffling) and when bpm is set to a positive value. Verify that when a beat
boundary is crossed, the reshuffle occurs and the mapping changes, while
tracking that the internal phaseNum_ and lastBeat_ state variables are properly
updated. Also verify that calling loop() with bpm==0 does not trigger a
reshuffle even when time advances significantly, confirming the timer logic
correctly controls when dynamic remapping happens.

Source: Coding guidelines