diff --git a/.github/codeql-config.yml b/.github/codeql-config.yml index 8338e746..f6e6201e 100644 --- a/.github/codeql-config.yml +++ b/.github/codeql-config.yml @@ -1,11 +1,15 @@ name: "projectMM CodeQL config" # Code we did not write and will not change is not a finding we can act on. doctest.h is the -# vendored single-header test framework (test/doctest.h) and produced the only critical alert -# in the tree that we do not own — excluding it here is the standard mechanism, rather than -# dismissing the same alert by hand after every scan. +# vendored single-header test framework (test/doctest.h) and the only third-party source in the +# repo; everything else under src/ and test/ is ours. Add to this list only for genuinely +# third-party code, never to quiet a finding in our own. # -# This is the ONLY vendored source in the repo; everything else under src/ and test/ is ours. -# Add to this list only for genuinely third-party code, never to quiet a finding in our own. +# NB it does NOT suppress findings inside doctest.h. For C/C++ `paths-ignore` filters which files +# are ANALYZED, not which are extracted through an `#include`: 143 test files include this header, +# so its code reaches the database via them and alerts still land on it (measured — the critical +# `cpp/use-after-free` at doctest.h:3705 stayed open across scans after this entry was added, and +# was dismissed in the Security tab instead). Kept because it still drops the header as a +# standalone analysis target; the dismissal is what actually silences it. paths-ignore: - test/doctest.h diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 7ff61b90..a7f4491f 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -14,7 +14,9 @@ name: CodeQL # `build-mode: none` scans C/C++ WITHOUT building it, inferring compile flags per file. That # matters here because the real firmware build is an ESP-IDF cross-compile for Xtensa/RISC-V a # runner would have to reproduce. The trade is some extraction noise on macro/template-dense -# headers. +# headers — and, more sharply, that what it analyzes is the DESKTOP configuration: anything whose +# truth depends on a target typedef (`nrOfLightsType` is uint32_t here, uint16_t on a no-PSRAM +# ESP32) is judged against the wrong build. Read findings of that shape with the target in mind. # # Free on public repos, so the cost is wall-clock only. @@ -32,10 +34,14 @@ on: - main - next-iteration paths: - # Only when C/C++ or the workflow itself changes — a docs commit has nothing new to - # analyse and would just burn a runner. + # Only when C/C++ or the analysis setup itself changes — a docs commit has nothing new to + # analyze and would just burn a runner. Both setup files are listed: a change to the query + # set or the ignore list changes what a scan FINDS, so it has to trigger one. Listing only + # the workflow left an edit to codeql-config.yml silently unscanned until something under + # src/ happened to change. - 'src/**' - '.github/workflows/codeql.yml' + - '.github/codeql-config.yml' workflow_dispatch: schedule: # Monday 04:00 UTC. Weekly is enough for a codebase this size, and keeps the signal @@ -64,10 +70,21 @@ jobs: with: languages: c-cpp build-mode: none - # security-and-quality is the widest stock set: the security queries answer the - # question above, and the quality ones tell us whether CodeQL sees anything our - # existing checks miss. Narrow it later if the noise is not worth it. - queries: security-and-quality + # security-extended, NOT security-and-quality. The quality pack was included to answer + # one question — does CodeQL see anything our existing checks miss — and after two runs + # the answer is no: its findings land in territory that already has an owner (unused + # variables and commented-out code are clang-tidy's, long/trivial switches are lizard's), + # which is the one-rule-one-owner rule this stack is built on. + # + # It was also actively misleading here. `build-mode: none` analyzes the DESKTOP + # configuration only, so 7 `cpp/constant-comparison` alerts flagged the layouts' overflow + # clamps as always-false — correct for that build, wrong for the target: `nrOfLightsType` + # is uint16_t on a no-PSRAM ESP32, where those guards are load-bearing. Build-dependent + # dead code is exactly what the quality pack hunts and exactly what it cannot judge here. + # + # The security queries are untouched, and they are the reason CodeQL has a slot at all: + # whole-program taint across the six packet parsers on a device with no MMU. + queries: security-extended # Excludes vendored code (test/doctest.h) — see the file for why. config-file: ./.github/codeql-config.yml diff --git a/CLAUDE.md b/CLAUDE.md index 8835298f..3ad3c378 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,7 +33,7 @@ fix, keeping main clean) — creating one silently moves work somewhere the PO i 1. **Pick.** One module/effect/driver/capability — the product owner picks what to build next. 2. **Spec.** Specs before code: the module spec and the UI spec sufficient to implement from (a draft may sit in the backlog until it ships); when in doubt, ask. -3. **Plan.** Plan mode before every feature; save the approved plan to `docs/history/plans/` as `Plan-YYYYMMDD - .md` — a temporary document: it ends up as the PR description and the file is deleted once the plan is realized; the merged PR is the design record. For a restructure ("make it simpler/cleaner"): enumerate 2–4 end states, name what each gains and loses, pick the leanest that solves the actual problem; propose as a question, implement only what's picked; surface follow-ups before starting so it's one coherent refactor. +3. **Plan.** Plan mode before every feature; save the approved plan to `docs/history/plans/` as `Plan-YYYYMMDD - <title>.md` — a temporary document: it ends up as the PR description and the file is deleted once the plan is realized; the merged PR is the design record. **Deleting a plan is the product owner's call — never the agent's.** "The code is written" is not "the plan is realized": a plan is realized when its *verification* is done too, including the judgement steps (thresholds tuned, results read together, the bench check). Ask; do not infer it from a green build. For a restructure ("make it simpler/cleaner"): enumerate 2–4 end states, name what each gains and loses, pick the leanest that solves the actual problem; propose as a question, implement only what's picked; surface follow-ups before starting so it's one coherent refactor. ### Build @@ -74,6 +74,8 @@ Commit message: title ≤ 72 characters, imperative. Then a 1–3 sentence end-u **Reviewer at commit-time:** run the Reviewer on the staged diff when the commit is large (roughly ten files or more across areas) or on PO request — start it first so the other checks run in parallel; findings fixed or accepted-with-reason before "commit now". +**Handling review findings** — from the Reviewer, CodeRabbit, or a human: *verify each finding against current code; fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.* A reviewer reads a snapshot and can be wrong or already out of date, so a finding is a claim to check, not an instruction to apply. Work through **every** finding, lowest severity first — a nit is a one-line fix while attention is cheap, and leaving the small ones for later means they are never done. Rising to the serious findings last also means the cheap context is already loaded. + ### Merge The PO pushes the branch; external review runs on the PR; findings are processed on the branch. On "run pre-merge": `uv run moondeck/event/premerge.py`, which re-runs the mechanical checks over the whole branch diff and lists the judgment gates it cannot decide. @@ -126,7 +128,7 @@ Published at [moonmodules.org/projectMM](https://moonmodules.org/projectMM/); so - [backlog/](https://moonmodules.org/projectMM/backlog/index.html) — forward-looking to-build lists (core / light / mixed) - [adr/](https://moonmodules.org/projectMM/adr/index.html) — immutable architecture decision records (Nygard format) - [history/](https://moonmodules.org/projectMM/history/index.html) — lessons, prior-project inventories, friend-repo digests -- [moonmodules/](docs/moonmodules/) — module catalog pages + generated technical pages +- [moonmodules/](https://github.com/MoonModules/projectMM/tree/main/docs/moonmodules) — module catalog pages + generated technical pages Docs describe the system as it is; git is the history; specs precede implementation. **Documentation model**: [coding-standards.md § Documentation model](docs/coding-standards.md#documentation-model). diff --git a/docs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.md b/docs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.md index d372ed5f..2d52e05b 100644 --- a/docs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.md +++ b/docs/adr/0016-one-parallel-led-driver-runtime-peripheral-strategy.md @@ -40,4 +40,4 @@ The alternatives weighed and rejected: **keep CRTP + one registered wrapper** (s The migration cost is documented, not coded (per [ADR-0013](0013-no-migration-code-robust-persistence-plus-documented-breaks.md)): a field device's persisted `MoonLedDriver`/`MultiPinLedDriver`/`ParlioLedDriver` type no longer resolves, so the module drops on boot and the user re-adds a Parallel LED driver and picks the peripheral — a `MIGRATING.md` entry covers it, and the web-installer catalog names the new type so a fresh install is correct. -The design intent and staged plan are the [consolidation plan](../history/plans/); this ADR is the decision record. +The design intent and staged plan are the [consolidation plan](../history/plans/README.md); this ADR is the decision record. diff --git a/docs/architecture.md b/docs/architecture.md index 413b306e..13065aaf 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -236,6 +236,19 @@ Abstractions are added when a concrete implementation needs them, not pre-design **Platform boundary (hard rule).** All `#ifdef`, `#if defined`, platform-specific `#include`s, and hardware API calls live exclusively in `src/platform/`. Everything outside `src/platform/` compiles on every target without modification. Compile-time platform branching uses `if constexpr` on `platform_config.h` flags, never a preprocessor `#ifdef`. The boundary is enforced by [`moondeck/check/check_platform_boundary.py`](../moondeck/check/check_platform_boundary.py), a commit gate (see [CLAUDE.md § The Process](../CLAUDE.md#the-process)). +**The desktop build runs everything (hard rule).** Every module, effect and driver in the repo +links and runs on the host — the platform layer simply has no silicon behind the call. Where a +peripheral is absent the host *emulates* it rather than declaring itself incapable: the parallel +WS2812 buses are backed by heap buffers, `lcdLanes` / `parlioLanes` / `rmtTxChannels` report a +real chip's counts, and `hasLcdCam` is true. Code excluded from the host binary is code that +cannot be unit-tested, cannot be seen by any AST-based check, and only ever runs where it is +hardest to debug — which is exactly what the LED drivers were until they were linked here. + +A capability flag therefore answers *"can this build exercise the path?"*, not *"is this real +hardware?"*. Where a flag must mean the latter (`hasLcdCam` gating the pin expander), that is a +deliberate, commented exception. Timing, wire protocol and pin state are NOT emulated: they need +silicon, and faking them would let a self-test report on hardware it never touched. + ## Firmware vs deviceModel vs board Three distinct things, kept distinct in the vocabulary: diff --git a/docs/history/README.md b/docs/history/README.md index af2b13e9..1a307bda 100644 --- a/docs/history/README.md +++ b/docs/history/README.md @@ -33,7 +33,7 @@ One-time surveys of earlier projects, used to decide what to harvest into projec ### The plan archive -[`plans/`](plans/) holds 89 approved feature plans from before plans became temporary. Under the current rule ([CLAUDE.md § Branch](../../CLAUDE.md#branch)) a plan's text goes into its PR description and the file is deleted once the plan ships, so nothing new is added here. These files predate that: they follow the older kept-forever convention, with the outcome marked in the filename (`… (shipped).md`, `… (attempted, abandoned).md`, unmarked = never finished). Reference only, and a candidate for the same subtraction the rest of `history/` gets — the merged PRs are the permanent record of what these describe. +[`plans/`](plans/README.md) holds 89 approved feature plans from before plans became temporary. Under the current rule ([CLAUDE.md § Branch](../../CLAUDE.md#branch)) a plan's text goes into its PR description and the file is deleted once the plan ships, so nothing new is added here. These files predate that: they follow the older kept-forever convention, with the outcome marked in the filename (`… (shipped).md`, `… (attempted, abandoned).md`, unmarked = never finished). Reference only, and a candidate for the same subtraction the rest of `history/` gets — the merged PRs are the permanent record of what these describe. ### Our own lessons diff --git a/docs/history/lessons.md b/docs/history/lessons.md index 1c060845..35df5c89 100644 --- a/docs/history/lessons.md +++ b/docs/history/lessons.md @@ -172,7 +172,7 @@ The first audio-reactive capability: `AudioModule` (a SystemModule Peripheral) r - **Effects reach the producer via `AudioModule::latestFrame()` (a static accessor), not a boot setter**, because an audio effect can be added through the UI after boot and a setter only wired the boot instance. The active mic registers in `setup()`, clears in `teardown()`, returns a static silent frame when there's no mic. - **Two hardware-only bugs, now pinned:** a missing `registerType<AudioModule>` made `create(...)->markWiredByCode()` deref null and boot-loop; the I²S read blocked the tick ~7.7 ms at a 20 ms timeout (fixed to non-blocking + a cross-tick sample accumulator, dropping to ~400 µs). The INMP441 is on the **left** I²S slot here (right reads empty), the first bench suspect when level floors with sound present. - **Shipped the manual core; the adaptive conditioner was prototyped and removed.** Auto noise-floor + AGC + smoothing needs tuning in a *quiet* room; the dev environment (a campground van with strong varying low-frequency rumble) was the adversarial worst case that kept it from settling. Land the manual core, treat adaptive auto-tuning as its own increment, tune it where the noise floor is real-quiet. Also: `level` is overall RMS (independent of the FFT), don't derive it from the bands or it stops fluctuating with volume. -- **Designed fresh from the datasheet + textbook DSP, not a prior project.** The datasheet made even a behaviour-reference unnecessary: a flat ±3 dB mic has no per-frequency error to correct, so the hand-tuned band-correction table years of prior-project work produced was the wrong tool; the textbook defaults sufficed. The DSP choices and *why* live in [AudioModule.md](../moonmodules/core/moxygen/AudioModule.md). +- **Designed fresh from the datasheet + textbook DSP, not a prior project.** The datasheet made even a behaviour-reference unnecessary: a flat ±3 dB mic has no per-frequency error to correct, so the hand-tuned band-correction table years of prior-project work produced was the wrong tool; the textbook defaults sufficed. The DSP choices and *why* live in [AudioService.md](../moonmodules/core/moxygen/AudioService.md). ## ESP32-P4 round 3, WiFi via the C6: the abstraction the earlier round feared wasn't needed diff --git a/docs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.md b/docs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.md index ac6033af..9bd21226 100644 --- a/docs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.md +++ b/docs/history/plans/Plan-20260722 - Release 4 scope - effect breadth + rename runway.md @@ -24,7 +24,7 @@ Then the next migration batch on top. This is the R4 headline: it unblocks the r ## Two quick wins — scoped and ready - **Active-instance election primitive.** ([Plan-20260710 - Active-instance election primitive](Plan-20260710%20-%20Active-instance%20election%20primitive.md).) A core `ActiveInstance<T>` that removes duplicated singleton-election bookkeeping from `AudioService` + `DevicesModule` (both had real dangling-static bugs). Textbook *Complexity-lives-in-core* subtraction; small; in flight. -- **CodeRabbit #29 boundary findings (4).** ([backlog-core § MoonLive core/platform layering](../../backlog/backlog-core.md#moonlive-coreplatform-layering--jit-sdkconfig-scoping-coderabbit-29-4-findings).) MoonLive core-includes-platform + compiled-into-`mm_core`, W^X disabled in the board default, a scenario riding timing + network. Real, already scoped; good hygiene to close before a named release. +- **CodeRabbit #29 boundary findings (4).** ([backlog-core § MoonLive core/platform layering](../../backlog/backlog-core.md#moonlive-coreplatform-layering-jit-sdkconfig-scoping-coderabbit-29-4-findings).) MoonLive core-includes-platform + compiled-into-`mm_core`, W^X disabled in the board default, a scenario riding timing + network. Real, already scoped; good hygiene to close before a named release. ## The RS-485 / DMX-512 opportunity (candidate, larger) diff --git a/docs/history/plans/Plan-20260728 - Doc-comment size reporting via clang-query.md b/docs/history/plans/Plan-20260728 - Doc-comment size reporting via clang-query.md index 9e5fb793..940176b7 100644 --- a/docs/history/plans/Plan-20260728 - Doc-comment size reporting via clang-query.md +++ b/docs/history/plans/Plan-20260728 - Doc-comment size reporting via clang-query.md @@ -111,17 +111,70 @@ lizard covers 94% of it already. ## Verification -1. `uv run moondeck/check/check_clang_query.py --rule comments` — table appears, split by scope. -2. **Control check that must fire** (a zero is indistinguishable from a tool that read nothing): - the report must contain `MoonLedDriver.h` at ~9731 chars / 129 lines and `HttpServerModule.h` - at ~8085 / 101. If the longest known comments are absent, the run is broken, not the tree. -3. Cross-check against the source-text count in this plan: **619 blocks** (class 114 / method 400 - / field 63 / other 42). A materially lower AST count means the matcher is missing a scope — - the `varDecl`-without-`fieldDecl` mistake that silently dropped every class member from the - array rule. -4. Run it through the **MoonDeck card**, not the terminal, so the PO's 📄 log is the real number. -5. `cmake --build build` + `ctest` unaffected (no `src/` change); `check_specs.py` clean. -6. Then read the output together and set the per-scope thresholds. +1. ✅ The rule runs and prints a per-scope table. +2. ✅ **Control check fired.** `HttpServerModule.h` at 7968/101 and `MoonI80Peripheral` at + 9334/129 are both in the report. Four parsing bugs were caught by exactly this step: every + NAME read as `col`; the largest comment in the tree dropped by a fixed 80-line lookahead; + ~45 one-line `///` blocks missed because a same-line span prints `<col:23, col:71>` with no + line number; and 365 of 474 rows mis-attributed until the matcher bound the declaration. +3. ✅ Cross-checked. **629 found** (class 124 / method 400 / field 105) against 642 in source — + the remainder is 4 blocks in `ImprovFrame.h` plus enum/typedef scopes outside the matcher. +4. ✅ Run through the MoonDeck card; the log reads 629. +5. ✅ 876 unit tests, 19 scenarios, specs clean, three ESP32 firmwares byte-identical. +6. ◻ **OPEN — the reason this plan is not yet realized.** Read the output together and set the + per-scope thresholds. The report deliberately ships with none. + +### Where the thresholds landed + +The report went through three shapes on PO direction, each discarded for a measured reason: + +1. **Per comment, by characters** — the biggest single comments. Covered only `///`, so it saw + 24% of the tree's comment lines, and could not tell "one huge comment" from "a file of many". +2. **Per file, by line ratio** — hid the outlier inside an average (a 16-line comment in a 27-line + header is fine), and the PO's "2× the line count" budget could not apply: `/// lines ÷ total + lines` is capped at 1.0 by construction, so 2.0 would flag nothing. +3. **Per declaration, by words** — what shipped. A line is a formatting accident; words are what a + reader absorbs. Measured: a comment line carries a median of **13 words** in both kinds, so the + PO's line yardstick (class 10, method/attribute 3) converts to **130 / 40 / 40 words**. + +`DOC DEVIATION` is the signed % against those ideals. `DEV WORDS` is a raw count with no ideal, +because zero IS the ideal there — measuring deviation from "one line" made the best case (no +developer note at all) read as -100%, i.e. worst. Rows sort by |deviation| so both failure modes — +bloated and missing — surface together. + +A `VIS` column separates public from private: doxygen publishes only public members, so an +undocumented public method is an API gap while a private one is a maintenance note. Both are +reported; a bloated comment is bloat either way. + +**No cutoff is enforced.** The ideals are a ruler — `MoonI80Peripheral`'s header may be right at +ten times the ideal, because it IS the driver's spec. + +## Grew out of this plan: the host runs every driver + +Not in the original scope, added on PO direction while implementing. The plan predicted a +permanent coverage limit — 184 `///` blocks in `src/light/drivers/` that the desktop AST could +never see, because the drivers were `#if defined(CONFIG_SOC_*)`-gated in `main.cpp`. The PO's +question ("shouldn't we instead run all existing drivers on the desktop?") turned out to be +right, and the premise wrong: those headers have zero direct ESP includes, already route +everything through `platform.h`, and already had desktop stubs. Only the *constants* said "not +my chip". + +So the host now **emulates** the peripherals rather than declaring itself incapable: +`lcdLanes`/`parlioLanes` 16, `rmtTxChannels` 4, `hasLcdCam` true, and the platform seams back the +buses with heap memory instead of returning `false`/`nullptr`. `ParallelLedDriver` runs on macOS +against all three real backends, switchable live. + +Recorded as a hard rule in [architecture.md § Platform abstraction](../../architecture.md). Its +limit is deliberate: timing, wire protocol and pin state are NOT emulated, because faking them +would let a self-test report on hardware it never touched. + +Coverage side effects, all from the same change: doc comments 454 → 629, RAM-costing arrays +362 → 390, heap allocation sites 63 → 80. Three ESP32 firmwares stayed byte-identical. + +**Follow-up the PO raised, not yet planned:** a host LED-strip emulator that decodes the encoded +buffer back to RGB — `busLoopback` is already the "read back what the wire carried" seam, with a +fully specified `RmtLoopbackResult`. That would catch encode bugs `PreviewDriver` structurally +cannot, since it shows the *layer* buffer rather than the wire. ## Deliberately not in this plan diff --git a/docs/metrics/repo-health.json b/docs/metrics/repo-health.json index 4936cdd8..2d8d2e14 100644 --- a/docs/metrics/repo-health.json +++ b/docs/metrics/repo-health.json @@ -1,18 +1,18 @@ { - "commit": "4a2effa2", + "commit": "8d491cbc", "flash": { - "esp32": 1684240, - "esp32p4-eth": 1497264, + "esp32": 1678368, + "esp32p4-eth": 1498160, "esp32p4-eth-wifi": 1793760, - "esp32s3-n16r8": 1666592, + "esp32s3-n16r8": 1668336, "esp32s3-n8r8": 1666592, "esp32s31": 1919136, - "desktop": 878680 + "desktop": 942648 }, "perf": { "desktop": { - "tick_us": 127, - "fps": 7874 + "tick_us": 126, + "fps": 7936 }, "esp32": { "tick_us": 4164, @@ -20,53 +20,53 @@ } }, "loc": { - "core": 14827, - "light": 19515, - "platform": 11914, + "core": 14857, + "light": 19571, + "platform": 12056, "ui": 5811, - "test": 34447, - "moondeck": 18318 + "test": 34719, + "moondeck": 19477 }, "comments": { "core": { - "lines": 5549, + "lines": 5566, "ratio": 0.408 }, "light": { - "lines": 7457, - "ratio": 0.422 + "lines": 7508, + "ratio": 0.423 }, "platform": { - "lines": 3937, - "ratio": 0.366 + "lines": 3994, + "ratio": 0.367 }, "ui": { "lines": 1518, "ratio": 0.278 }, "test": { - "lines": 5898, - "ratio": 0.198 + "lines": 5966, + "ratio": 0.199 }, "moondeck": { - "lines": 2781, - "ratio": 0.174 + "lines": 3097, + "ratio": 0.182 } }, "tests": { - "cases": 970, + "cases": 982, "scenarios": 22 }, "docs": { "md_files": 169, - "md_lines": 22610, + "md_lines": 22536, "plans_files": 89, - "backlog_lines": 3114, - "lessons_lines": 378, - "claude_md_lines": 126 + "backlog_lines": 3113, + "lessons_lines": 379, + "claude_md_lines": 135 }, "complexity": { - "functions": 2129, + "functions": 2137, "over_threshold": 136, "worst_ccn": 93 } diff --git a/docs/metrics/repo-health.md b/docs/metrics/repo-health.md index d74198eb..74b19f41 100644 --- a/docs/metrics/repo-health.md +++ b/docs/metrics/repo-health.md @@ -1,6 +1,6 @@ # Repo health -Measured at `4a2effa2`. Generated by [`moondeck/check/repo_health.py`](../../moondeck/check/repo_health.py) on every KPI-gate run — **do not edit by hand**. +Measured at `8d491cbc`. Generated by [`moondeck/check/repo_health.py`](../../moondeck/check/repo_health.py) on every KPI-gate run — **do not edit by hand**. Current state only; the trend is this file's git history (`git log -p docs/metrics/repo-health.md`). Nothing here fails a build: the numbers make growth visible, the judgment stays human. @@ -8,11 +8,11 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | Target | Flash | |---|---:| -| desktop | 858 KB (+0 KB) ⚠ | -| esp32 | 1,645 KB | -| esp32p4-eth | 1,462 KB | +| desktop | 921 KB | +| esp32 | 1,639 KB | +| esp32p4-eth | 1,463 KB | | esp32p4-eth-wifi | 1,752 KB | -| esp32s3-n16r8 | 1,628 KB | +| esp32s3-n16r8 | 1,629 KB (+2 KB) ⚠ | | esp32s3-n8r8 | 1,628 KB | | esp32s31 | 1,874 KB | @@ -20,32 +20,32 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | Target | Tick | FPS | |---|---:|---:| -| desktop | 127 µs | 7,874 | +| desktop | 126 µs (−14 µs) ✓ | 7,936 (+794) ✓ | | esp32 | 4,164 µs | 240 | ## Code | Area | Lines | Comments | Comment share | |---|---:|---:|---:| -| core | 14,827 | 5,549 | 40.8 % | -| light | 19,515 | 7,457 | 42.2 % | -| platform | 11,914 (+24) ⚠ | 3,937 | 36.6 % | +| core | 14,857 | 5,566 | 40.8 % | +| light | 19,571 | 7,508 | 42.3 % | +| platform | 12,056 | 3,994 | 36.7 % | | ui | 5,811 | 1,518 | 27.8 % | -| test | 34,447 | 5,898 | 19.8 % | -| moondeck | 18,318 | 2,781 | 17.4 % | +| test | 34,719 | 5,966 | 19.9 % | +| moondeck | 19,477 | 3,097 | 18.2 % | ## Tests | Kind | Count | |---|---:| -| unit cases | 970 | +| unit cases | 982 | | scenarios | 22 | ## Complexity | Metric | Value | |---|---:| -| functions | 2,129 (+1) ✓ | +| functions | 2,137 | | over threshold | 136 | | worst CCN | 93 | @@ -54,9 +54,9 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | Metric | Value | |---|---:| | markdown files | 169 | -| markdown lines | 22,610 (+18) ⚠ | +| markdown lines | 22,536 | | plan files | 89 | -| backlog lines | 3,114 | -| lessons lines | 378 | -| CLAUDE.md lines | 126 | +| backlog lines | 3,113 | +| lessons lines | 379 | +| CLAUDE.md lines | 135 | diff --git a/docs/moonmodules/light/layouts.md b/docs/moonmodules/light/layouts.md index d429cddc..783825af 100644 --- a/docs/moonmodules/light/layouts.md +++ b/docs/moonmodules/light/layouts.md @@ -1,5 +1,7 @@ # Layouts +![layouts controls](../../assets/core/Layouts.png) + Every layout, one block each: what it does and what each control means — together. A layout maps light indices to physical `(x, y, z)` positions — it defines the *shape* an [effect](effects.md) draws onto and a [driver](drivers.md) sends out. The [Layouts](moxygen/Layouts.md) container holds one or more layout children and composes them into one coordinate space; a [Layer](moxygen/Layer.md) renders over that combined space. (For how this page maps to the source/asset folders, see the [folder-structure decision](../../adr/0015-library-is-a-tag-not-a-folder.md).) ## MoonLight layouts diff --git a/moondeck/MoonDeck.md b/moondeck/MoonDeck.md index 6d77af12..26d0edd2 100644 --- a/moondeck/MoonDeck.md +++ b/moondeck/MoonDeck.md @@ -10,6 +10,7 @@ Below: the UI behaviours common to every card, described once, then one section - **Status dots** on each card: grey (not run), orange (running), green (exit 0), red (exit non-zero). - **Last-run log** — the **📄** button replays that script's last run in the log pane. It appears **only on cards that have actually run** (and shows up the moment a first run finishes, no reload needed), so its absence is informative too: a card with no 📄 is one nobody has used yet. Every run is teed to `build/moondeck-logs/<id>.log` as it streams (not buffered to the end, so a run you Stop still leaves what it printed), which answers "what did this do last time" after a page reload or a switch to another card — the case a live-only stream cannot. One file per script, overwritten each run: a last-run record, not a history. Gitignored, being derived state. +- **Run count** — every run ends `[exit code: 0] [run #7]`, counting that script's runs in `build/moondeck-logs/run-counts.json`. Two identical reports are otherwise indistinguishable, so the number answers "did this actually re-run since the fix, or am I reading the same output again?". Derived like the logs: deleting `build/` resets it, and a missing or corrupt file costs the count, never the run. - **Run/Stop toggle** for long-running scripts (Run desktop, Monitor ESP32). - **Duration hint** — every card shows how long it takes: ⚡ about a second, ⏱️ a few seconds up to ~30, 🐌 more than 30 seconds (a build, a flash, a gate list, clang-tidy). All three are shown rather than only the extremes, so a blank badge reads as "nobody set a speed on this card" instead of being confused with medium. Set per script as `"speed": "instant" | "medium" | "slow"` in `moondeck_config.json`. This is a *label*, not a timeout — nothing enforces it, so a script that grows slower needs its flag updated by hand. Separate from `long_running`, which controls the Run/Stop toggle rather than expected duration. - **Group headers** in the sidebar (setup, build, flash, run, test, check, scenario). @@ -201,6 +202,22 @@ the site. Every static-analysis tool, on ONE module — the repo-wide reports turned around. +**The stack, and why it is five tools.** Each answers a question the others structurally cannot, +which is what keeps them from being redundant: + +| | sees | answers | +|---|---|---| +| **clang-tidy** | one statement, with full type information | is this line wrong? | +| **clang-query** | the AST, via matchers we write | does this codebase's own rule hold? | +| **`-Wfunction-effects`** | the whole call graph, from the compiler | can the render path block? | +| **lizard** | tokens, no types | is this function getting more complex over time? | +| **CodeQL** | a queryable database of the program | can LAN bytes reach a `memcpy`? | + +One rule, one owner: where two tools could report the same thing, one is switched off (lizard owns +complexity, so clang-tidy's `readability-function-*` checks stay disabled). The individual cards +run each of these across the tree; this card inverts that — everything at once, scoped to the +module you are actually working on. + ```bash uv run moondeck/check/check_module.py --module Control uv run moondeck/check/check_module.py --module Layer --skip clang-tidy @@ -215,12 +232,12 @@ disagree about a finding. Each tool also accepts `--module` on its own if you wa **Module is not the same as a translation unit.** A TU is one `.cpp` plus everything it includes — the unit the compiler processes, and there are 15 under `src/`. A module is one class with its `.h` and optional `.cpp`, and there are ~90. Most are **header-only**, so they have no -TU of their own: `ParallelLedDriver.h` is analysed through whichever `.cpp` includes it. That is -why `--module` filters the findings rather than the file list — scoping by TU would analyse +TU of their own: `ParallelLedDriver.h` is analyzed through whichever `.cpp` includes it. That is +why `--module` filters the findings rather than the file list — scoping by TU would analyze nothing for a header-only module and report a confident, wrong zero. It still scopes the *parse*, by resolving which TUs actually reach the module's files — -following `#include` edges transitively, so a header-only module is analysed through the one or +following `#include` edges transitively, so a header-only module is analyzed through the one or two `.cpp` files that include it rather than all 15. `BouncingBallsEffect` is reached only by `main.cpp`: **8s for all three tools, down from over four minutes**. The run prints which TUs it picked, so the scope is visible rather than assumed. @@ -229,6 +246,20 @@ picked, so the scope is visible rather than assumed. Run clang-tidy over the whole tree and write a Markdown report. +**What clang-tidy is.** LLVM's linter for C++. It compiles each file for real — same parser as the +compiler, so it sees types, overloads and template instantiations — then matches a library of +named checks against the resulting AST (605 available in the LLVM 22 we run). That is the +difference from a text linter: it knows +`std::atoi(s)` is a call to the C library, not a word that looks like one. Checks come in +families (`bugprone-*`, `performance-*`, `readability-*`, `cert-*`), each independently +switchable, and many carry a machine-applicable fix. + +**What it does for us.** It is the per-statement layer: the bug that lives inside one function and +needs type information to see. `bugprone-unchecked-string-to-number-conversion` is the shape — +`atoi` cannot report a failure, which is invisible to grep and uninteresting to a complexity +counter. Enabled checks live in [.clang-tidy](../.clang-tidy) at the repo root, so the editor and +this report agree. + ```bash uv run moondeck/check/check_clang_tidy.py # full run, report to stdout uv run moondeck/check/check_clang_tidy.py --check bugprone-infinite-loop # one check, for triage @@ -251,6 +282,20 @@ report when more than ten files fail to compile. Our own AST rules — the checks we invent, that no off-the-shelf tool reports. +**What clang-query is.** An interactive REPL over Clang's AST, shipped with LLVM. You write a +matcher in the same domain-specific language clang-tidy checks are built from — +`cxxRecordDecl(hasName("Foo"))`, `callExpr(hasAncestor(ifStmt()))` — and it prints or dumps every +node that matches. It has no opinions and no built-in checks: it answers structural questions +about the code, and what counts as a finding is left to the caller. + +**What it does for us.** It is how a project-specific rule gets written without building a +clang-tidy plugin. A plugin is a compiled C++ target with its own build; a matcher is one line of +text, editable in minutes. So the rules here are ours by definition — how much RAM the fixed +arrays cost, where the heap is touched, whether every class carries a `///` — questions no +off-the-shelf linter asks because they are about *this* codebase's constraints. The matchers +match broadly and the filtering happens in Python, because the matcher language has no notion of +"bigger than 64 bytes". + ```bash uv run moondeck/check/check_clang_query.py # every rule uv run moondeck/check/check_clang_query.py --rule=arrays # one rule @@ -289,8 +334,9 @@ prints everything, and `--min-bytes N` restores a size cutoff — both CLI-only, one button and the default plus the "N more not shown" line already answer the question from the browser. -The per-module card runs with no cap: you scoped to one module precisely to see all of its -findings, and `HttpServerModule` alone has 71 arrays. +The per-module card keeps the 60-row cap, on every table including arrays: scoping to one module +narrows WHICH findings, not how many fit on a screen — `HttpServerModule` alone reports 212 +declarations. Truncation is always announced, so the tail is one `--max-rows 0` away. **Rule `scratch` — `ScratchBuffer` members.** `ScratchBuffer` *is* the project's heap manager, so a module that uses one allocates without any `new` or `malloc` appearing in its own source — @@ -315,6 +361,87 @@ Member methods named `free()` are excluded. We have three (`MoonLive`, `Buffer`, and matching on the name alone counted every call to them as heap deallocation — 15 false positives pointing at the wrong lines. +**Rule `comments` — which declarations are documented, and with which kind.** The project's rule +is that every class, method and attribute carries a short, readable `///` (that is what moxydoc +publishes), while `//` developer notes belong in the code lines rather than stacked on a +declaration. The report gives the matrix: + +``` + DECL DOC DEV NONE %DOC + class 53 116 25 27% + method 415 651 1141 18% + attribute 133 183 923 10% + + 950 declarations carry only a `//` where the rule asks for `///`; 2089 carry no comment at all. + Public surface: 503 of 2558 documented (19%) — the part doxygen publishes. +``` + +Three columns because there are three states, each with a different fix: `///` is documented; +`//` documents it in the wrong kind (promote it, or move it into the code lines); `NONE` has no +comment at all (write one). `%DOC` is the share carrying a real doc comment. + +Then every declaration, ranked by how far it sits from the ideal: + +``` + DOC DEVIATION DOC WORDS DEV WORDS VIS DECL NAME FILE:LINE + +1135% 1606 0 pub class MoonI80Peripheral MoonLedDriver.h:10 + +797% 1166 0 pub class HttpServerModule HttpServerModule.h:17 + -100% 0 61 priv method driversOn Scheduler.h:138 +``` + +**Sizes are WORDS, not lines.** A line is a formatting accident — the same paragraph is 5 lines at +100 columns and 9 at 60 — while words are what a reader absorbs. Measured here, a comment line +carries a median of **13 words** in both kinds, so a line-based yardstick of class 10 / method 3 / +attribute 3 converts to the ideals below. + +| Column | Meaning | +|---|---| +| `DOC DEVIATION` | Signed % difference from the ideal doc size — **class 130 words, method and attribute 40**. `0%` is ideal, `-100%` is absent, and over-documenting is unbounded. Not a "ratio": a ratio is a bare quotient (2.3×), this is a deviation from a target. | +| `DOC WORDS` | Raw `///` word count — the number the deviation is derived from, so it sits beside it. | +| `DEV WORDS` | Raw `//` word count. **No deviation column**, because zero is the ideal: measuring against "one line" made the best case (no note at all) read as `-100%`, i.e. worst. | +| `VIS` | `pub` or `priv`. Doxygen publishes only public members, so an undocumented public method is an **API gap** while a private one is a maintenance note. Both are reported — a bloated comment is bloat either way, and clangd's hover shows both kinds to whoever maintains the code — but the reader can weigh them. | + +The `Public surface: N of M documented` line is the number the doc site reflects. It differs +sharply from the overall figure: `HueDriver` reads 40% public against 13% overall, because 52 of +its 77 declarations are private. + +Rows sort by **|DOC DEVIATION|**, so both failure modes surface together: a bloated header and a +declaration with no comment at all are equally wrong in opposite directions, and ranking on the +signed value would bury one of them. + +The ideals are a ruler, not a gate. `MoonI80Peripheral`'s 1606 words may be exactly right — they +ARE the driver's spec. The ratio says how far from typical something sits; a human decides. + +**How `//` becomes visible to the AST.** Clang's lexer discards `//` — only `///` and `/** */` +become AST nodes, because the compiler consumes those itself. So the rule parses a SHADOW COPY of +`src/` under `build/comment-shadow/`, where every leading `//` has been rewritten to +`/// MMDEV: …`. The marker survives into the comment text, which is what keeps the two kinds +apart; `src/` is never touched and the reported paths are mapped back. + +The marker must be plain text — an `@`-prefixed one is parsed as a doc *command* and stripped, +which silently merges the two kinds again. `-Wdocumentation` would fabricate warnings on +rewritten dev notes (`// @param wrong` becomes a real diagnostic), but clang-query never enables +it, so that risk does not apply here. + +Not reported, because none of them is a declaration anyone documents: **forward declarations** +(`class JsonSink;` — no body, and the real class is reported from the header that defines it), +**lambdas** (`unless(isLambda())` on the record and `unless(ofClass(isLambda()))` on the method +— a lambda written inside a function body is code, not a documented declaration, and its +synthesised closure class plus call operator would otherwise report as 53 undocumented +"methods"), `implicit` closure classes, `invalid` declarations that only parse in the TU that +owns them, and `= default` / `= delete` members. + +A **real** `operator()` on a named functor class is still reported — the exclusion asks the AST +whether the enclosing record is a lambda, rather than testing the method's name, so writing +`struct Compare { bool operator()(…) const; }` tomorrow does not silently drop it. + +Not reported: function PARAMETERS. C++ cannot attach a doc comment to one — 1054 probed, zero +with a comment — they are documented via `@param` inside the method's own comment, and this tree +uses `@param` 8 times, all in a `.js` file. Class ATTRIBUTES are the per-member scope that does +exist, and they are the `attribute` row above. Declarations clang marks `implicit` (a lambda's +closure class) or `invalid` (a parse that only succeeds in the TU that owns it) are skipped: +both are anonymous, so their only "name" is the literal word `definition`. + Takes ~50s cold (a few seconds once the compilation database is warm). clang-query has no parallel runner of its own and costs ~44s per translation unit, so this runs the 15 `src/` TUs across cores; serial would be ~11 minutes. @@ -323,6 +450,19 @@ across cores; serial would be ~11 minutes. What the render path calls that can block or allocate — checked by the compiler. +**What `-Wfunction-effects` is.** Not a separate tool: a Clang 20+ warning implementing the C++ +*function effects* proposal (P2698). Annotate a function `[[clang::nonblocking]]` and the compiler +verifies, **transitively through the whole call graph**, that nothing it reaches allocates, locks, +throws, or otherwise blocks. It is a compiler feature, so it sees exactly what the compiler sees — +including through virtual dispatch and member-pointer calls, which is where a hand-written check +gives up. + +**What it does for us.** The render tick has a hard real-time budget, and the failure mode is a +`malloc` four calls deep in something that looks harmless. Nothing else in the stack can answer +that: clang-tidy checks one statement, lizard counts branches, the Python checks read text. This +script's own job is deduplication and framing — a raw build prints ~1350 warning lines for ~175 +unique sites, because a header is recompiled once per translation unit that includes it. + ```bash uv run moondeck/check/check_nonblocking.py # summary by callee, then every site uv run moondeck/check/check_nonblocking.py --module AudioService @@ -346,11 +486,25 @@ not be resolved from source. | Column | | |---|---| +| **COND** | the branch guarding the call: `—` none (runs every time its tick does), `if`, `loop`, `switch`, `?:`, `&&`, and `ret` for a call reached only past an `if (…) return;` above it. `·rate` marks a rate limiter (`if (++n >= kEvery)`, `if (now - last < kIntervalMs) return;`) or a once-only latch (`if (!inited_)`). `?` means clang-query was unavailable, which is *unknown*, not unguarded | | **CALLS** | the function that blocks — or `(static local variable)`, a violation with no callee: a static local needs a guard variable and a one-time lock on first use | | **IN** | the method the call sits in, which is what places it in a tier. Clang names the call and the callee but *not* their enclosing function, so this is read back from the source | | **WHY IT BLOCKS** | clang's own root cause, e.g. `calls mm::platform::UdpSocket::sendTo`. `—` means a leaf the compiler could not look inside (external or unannotated) | | **FILE:LINE** | where to go | +**Sorted unconditional-first** within each tier, then by file and line. A call that runs every +time its tick does costs more than the same call behind a branch, and ties keep source order so +findings in one file stay together and the list diffs cleanly between runs. + +**Two engines, each doing what it is good at.** The compiler finds the blocking calls, because +`-Wfunction-effects` is transitive and a matcher would have to rebuild the call graph to match it. +clang-query then annotates each site with its guard — a purely local AST question — joined on +`file:line`. Measured ~1s per TU, against a report whose cost is the clean rebuild. + +**Guarded is not rare.** `if (enabled_)` is conditional and true every frame; only the `·rate` +hint separates those, and it reads the condition's *spelling*, so it is a lead rather than a +verdict. COND answers "is this reached every tick", never "is this acceptable". + **Desktop-only, and that loses nothing.** On GCC `MM_NONBLOCKING` expands to `noexcept` — the exception contract still holds; only the clang attribute and the warning are absent. The ESP32 toolchain has neither the attribute nor the warning, and builds with `-Werror`, so a bare attribute there @@ -364,12 +518,84 @@ carrying `MM_NONBLOCKING` is invisible. Closing that needs an xtensa clang — b "ESP32 clang/LLVM toolchain" in backlog-core.md. Not a gate yet: `-Wno-error=function-effects` keeps the build green while the findings are -triaged. Each is a judgement — fix it, annotate the callee, or accept it with a scoped reason. +triaged. Each is a judgment — fix it, annotate the callee, or accept it with a scoped reason. + +### check_codeql + +CodeQL's open alerts, read from GitHub — the Security tab as a card. + +**What CodeQL is.** GitHub's semantic analysis engine. It compiles the codebase into a *relational +database* of every declaration, expression and control-flow edge, then runs queries written in QL, +a logic language, against it. Because it is a database rather than a pass over one file, a query +can follow data **across function and file boundaries** — the standard packs trace values from a +`source` (something attacker-controlled) to a `sink` (somewhere dangerous) and report the path. +That is *taint tracking*, and it is what nothing else in this stack can do. + +**What it does for us.** One question: we parse six network packet formats (ArtNet, DDP, E1.31, +WLED audio sync, MQTT, WLED) plus HTTP, doing ~22 `memcpy` operations on bytes arriving from the +LAN, on a device with **no MMU and no process isolation** — a bad length check is not a crash, it +is arbitrary memory. CodeQL is the only layer that can trace a length field from the wire to the +copy. It has already paid for itself once (3 thread-unsafe `localtime` findings, fixed), and its +current answer on the packet parsers is *clean* — which is positive evidence, not silence. + +```bash +uv run moondeck/check/check_codeql.py # open alerts, worst first +uv run moondeck/check/check_codeql.py --state fixed # what has been resolved +uv run moondeck/check/check_codeql.py --all # every state, including dismissed +uv run moondeck/check/check_codeql.py --module HueDriver +``` + +CodeQL runs in CI ([codeql.yml](../.github/workflows/codeql.yml)), not locally: it is the one +layer of the stack that sees whole-program taint, which is what the six network packet parsers +justify. Its findings then live behind the Security tab — a report nobody opens. + +**Fetches, does not scan.** The CodeQL CLI would need a ~1 GB install and minutes per run to +recompute what CI already has, and the alert lifecycle (open / fixed / dismissed, tracked across +runs) is the valuable part — baselining we would otherwise build. The trade is stated in every +run's output: alerts describe the last **analyzed pushed commit**, so local edits are not in them. + +Two severity scales are merged into one ordering, because the question is "what matters most", not +"which query pack found it": `security_severity_level` on the security queries +(critical/high/medium/low) and `severity` on the quality ones (error/warning/note/recommendation). + +**Split into `src/` and everything else**, because the two are read differently: a finding in +shipping code reaches a device, one in a test does not. Measured at 855 open alerts, **783 sat in +`test/`** — pooling them buried the three `high` findings in `src/` under doctest noise. The +severity tally prints before either table so the counts survive row truncation. + +**All states** (`--all`) adds the `fixed` and `dismissed` alerts to the open ones — the lifecycle +GitHub tracks and the reason this fetches rather than rescans. It answers "did that finding go +away, or did someone dismiss it?", so the tables gain a STATE column and a per-state tally +whenever more than one state is present. On a repo with nothing fixed or dismissed yet it reads +the same as the default, which is correct, not a broken flag. Each state is queried **explicitly**: +omitting `state` does not mean "any" — the endpoint defaults to `open`. + +`--module` scopes to one module's files from the command line. It is deliberately NOT wired to the +tools-group module dropdown: that selector is promoted above the cards as soon as two cards in a +group declare `needs_module`, which moves it out of *All Tools on Module* where it belongs. + +**Exit 2 when the answer is unknown** — no `gh`, not authenticated, code scanning disabled — with +the reason on stderr. An empty list and an unreachable API look identical in a table, and only one +of them means the code is clean. ### check_lizard Complexity gate: fail on **new** over-complex functions, not the ones already there. +**What lizard is.** A small language-agnostic complexity counter (Python, ~20 languages). It does +not parse C++ properly — it tokenizes, counts branch keywords, and reports cyclomatic complexity +(CCN), line count (NLOC), parameter count and token count per function. That shallowness is the +point: no build, no compile database, no toolchain, so it runs anywhere in about a second. + +**What it does for us.** It owns ONE number — how complex a function is — and it is the only tool +here that produces a per-commit trend rather than a verdict. clang-tidy can tell you a function is +complex today; only a series tells you the codebase is drifting, which is what +[repo-health](../docs/metrics/repo-health.json) and `collect_kpi` plot. Its own +`readability-function-*` checks stay off in clang-tidy for exactly that reason (one rule, one +owner). The tokenizer's cost is real: on template- and macro-dense C++ it reports a mangled +function name (`SolidEffect::static_cast<lengthType>` for a method called `tick`), and since the +baseline matches on `file:name`, such an entry pins nothing — see the note below. + ```bash uv run moondeck/check/check_lizard.py # report NEW violations, exit 1 if any uv run moondeck/check/check_lizard.py --all # every violation, baseline ignored diff --git a/moondeck/check/check_clang_query.py b/moondeck/check/check_clang_query.py index 1bcb55ea..af9c897f 100644 --- a/moondeck/check/check_clang_query.py +++ b/moondeck/check/check_clang_query.py @@ -26,6 +26,16 @@ Not a violation — the driver layer allocates deliberately — but the hot path must not, and the count is the thing worth trending. +3. **Comments per declaration.** Which classes, methods and attributes are documented, and with + which kind: `///` is what moxydoc publishes, while a `//` stacked on a declaration is usually a + thought that belongs in the code below it. Sized in WORDS against per-scope ideals, with the + deviation reported rather than a pass/fail — a class header IS the module spec, so its length + is a judgment, not a defect. + + Clang's lexer discards `//`, so it reaches the AST only via the shadow tree below: a copy of + src/ where a leading `//` becomes `/// MMDEV:`. That marker is what keeps the two kinds apart + in the DEV column; the real source is never touched. + Usage: uv run moondeck/check/check_clang_query.py # every rule uv run moondeck/check/check_clang_query.py --rule arrays @@ -37,6 +47,7 @@ import json import os import re +import shutil import subprocess import sys from concurrent.futures import ThreadPoolExecutor @@ -115,6 +126,109 @@ # The type a `new` produces: `CXXNewExpr 0x... <...> 'Foo *' array ...`. _NEW_TYPE = re.compile(r"CXXNewExpr 0x\w+ <[^>]*> '(?P<type>[^']+?) ?\*'") +# A doc comment's span. The AST dump elides whatever is unchanged from the previous node, so the +# same construct prints three ways and all three must parse: +# <line:272:4, line:298:76> multi-line +# <line:309:8, col:61> multi-column, one line +# <col:23, col:71> ENTIRELY on the current line — no line number at all +# The third form is why ~45 one-line `///` comments (every effect, layout and modifier header) +# were silently missing from the first version: a regex demanding a digit pair never matched +# them. When a `line:` is absent the line is inherited from the enclosing context, which is +# tracked while walking. +_FULLCOMMENT = re.compile( + r"FullComment 0x\w+ <(?:(?P<path>[^:>]*):(?P<pstart>\d+):\d+|line:(?P<start>\d+):\d+|col:\d+)" + r"(?:, (?:[^:>]*:(?P<pend>\d+):\d+|line:(?P<end>\d+):\d+|col:\d+))?>") + +# The text of one comment line: `TextComment 0x... <col:4, col:29> Text=" A doc comment."`. +_TEXTCOMMENT = re.compile(r"TextComment 0x\w+ <[^>]*> Text=\"(?P<text>.*)\"") + +# Any node that is part of a doc comment's subtree rather than the declaration below it. +# `Comment` alone would also swallow the FullComment of the NEXT comment, so the wrappers are +# named explicitly and FullComment is deliberately absent — reaching one means this comment ended. +_COMMENT_NODE = re.compile( + r"\b(?:ParagraphComment|BlockCommandComment|ParamCommandComment|TParamCommandComment" + r"|InlineCommandComment|VerbatimBlockComment|VerbatimBlockLineComment|VerbatimLineComment" + r"|HTMLStartTagComment|HTMLEndTagComment)\b") + +# The declaration a comment is attached to — the next decl node after the comment subtree. +# Clang dumps a doc comment as the first child of the declaration it documents, so "the next +# decl below this FullComment" is its owner. Validated on a real TU: 206 of 207 comments +# resolved, split 17 class / 155 method / 28 field / 6 constructor. +# The name is the LAST identifier before the type/qualifier tail, not the first token after the +# source range — a non-greedy `\b(\w+)\b` there captures `col` out of `<line:299:1, col:7> col:7 +# implicit referenced class ControlList`, which is how the first version reported every row as +# "col". Anchoring on the trailing keyword (`class`/`struct`/`union`) or the quoted type is what +# makes it the declaration's own name. +_OWNER = re.compile( + r"(?P<kind>CXXRecordDecl|CXXMethodDecl|CXXConstructorDecl|CXXDestructorDecl" + r"|CXXConversionDecl|FieldDecl" + r"|FunctionDecl|VarDecl|EnumDecl|EnumConstantDecl|TypedefDecl|TypeAliasDecl) 0x\w+[^<]*" + r"<(?:[^:>]*:)?(?:line:)?(?P<line>\d+):\d+[^>]*>" + # A record's name follows its keyword and may be trailed by `definition`; a `[^\n]*?` before + # the keyword would let the tail win, so the trailing token is matched explicitly. Without + # this, every `class Foo … definition` line reported its name as "definition". + r"(?:[^\n]*?\b(?:class|struct|union|enum)\s+(?P<cname>\w+)(?:\s+definition)?\s*$" + r"|[^\n]*?(?P<fname>~?(?:operator\s*\S+|\w+))\s+')", re.M) + +# A DIRECT child of the block head: exactly one tree connector before the node name. A deeper +# node (`| |-`, `| `-`) belongs to a nested declaration, which clang-query emits as its own +# bound block — so depth is what stops a class absorbing every comment its members carry. +_TOP_CHILD = re.compile(r"^[|`]-") + +# A record declaration head, for the forward-declaration test above. Only records can be +# forward-declared; a method or attribute always has its definition where it is declared. +_RECORD_HEAD = re.compile(r"^(?:CXXRecordDecl|ClassTemplateDecl) ") + +# The file a dumped node belongs to. The AST dump prints an absolute path only when it CHANGES, +# then `line:N` for subsequent nodes in the same file — so the current file has to be tracked +# as the dump is walked, not read off each node. +_PATH_HINT = re.compile(r"<(?P<path>/[^:>]*\.(?:h|cpp|hpp|cc)):(?P<line>\d+):") + +# `<line:N:C>` with no path — same file, new line. Feeds the line that a `col:`-only span inherits. +_LINE_HINT = re.compile(r"<line:(?P<line>\d+):\d+") + +# What each owner kind counts as, for the per-scope split. A constructor is a method; an enum or +# a typedef is neither class nor method, so it reports as `other` rather than being dropped — +# a scope nobody expected is a finding, not a reason to hide the row. +_SCOPE = { + "CXXRecordDecl": "class", "EnumDecl": "class", + "EnumConstantDecl": "attribute", + "CXXMethodDecl": "method", "CXXConstructorDecl": "method", + "CXXDestructorDecl": "method", "CXXConversionDecl": "method", "FunctionDecl": "method", + "FieldDecl": "attribute", "VarDecl": "attribute", +} + +# What "documented well" looks like, in WORDS, per declaration kind. +# +# Words rather than lines: a line is a formatting accident — the same paragraph is 5 lines at 100 +# columns and 9 at 60 — while the word count is what a reader actually absorbs. Measured on this +# tree, a comment line carries a median of 13 words in BOTH kinds, so a line-based yardstick of +# class 10 / method 3 / attribute 3 converts to 130 and 40. +# +# A class carries the spec: what it is for, how it fits, what a caller must know. A method or an +# attribute is one idea. This is a ruler, not a gate — MoonI80Peripheral's header may be exactly +# right at ten times the ideal, because it IS the driver's spec. +_IDEAL_DOC_WORDS = {"class": 130, "method": 40, "attribute": 40, "other": 40} + +# `@moreinfo` ends the part being measured. The directive splits a class comment in two (see +# gen_api.py): everything before it is the lead description, rendered above the attribute/method +# lists, and everything after is deep-dive reference relocated BELOW them under `## More info`. +# +# Only the lead is measured, because the ideal asks "how much must a reader take in before the +# member lists" — and reference material deliberately parked at the bottom of the page is not that. +# Counting the whole block punished the very structure the directive exists to encourage: +# NetworkSendDriver read +407% as one blob, and +48% once its 469 More-info words were separated +# from its 193-word lead. +# +# Matched on the AST node, not the source text: clang parses the directive into +# `InlineCommandComment ... Name="moreinfo"`, so the split point is stated rather than guessed at. +_MOREINFO_NODE = re.compile(r'InlineCommandComment.*Name="moreinfo"') + +# The `//` side deliberately has NO ideal, because zero IS the ideal: a developer note stacked on +# a declaration is usually a thought that belongs in the code below it. Measuring it as +# deviation-from-one-line made the best case (no note at all) read as -100%, i.e. worst — so DEV +# is reported as a raw word count. Zero reads as zero. + # Three constraints this matcher set was built around. Recorded here because two are NEGATIVE # results — the kind that gets re-attempted by whoever forgets they were already tried: # @@ -141,6 +255,32 @@ "output": "dump", "matcher": ('fieldDecl(hasType(cxxRecordDecl(hasName("ScratchBuffer"))))'), }, + "comments": { + "title": "Comments per declaration (scope x kind)", + "output": "dump", + # `shadow: True` parses a copy of src/ where every leading `//` became `/// MMDEV: …`, + # so ONE pass reports both kinds against the declaration each documents. Without it this + # rule sees only `///` — 24% of the tree's comment lines. + "shadow": True, + # The scopes that can actually carry a comment: class/struct, method, and class ATTRIBUTE. + # parmVarDecl is deliberately absent — a C++ parameter cannot hold a doc comment (1054 + # probed, zero with one), it is documented via `@param` in the method's own comment, and + # this tree uses `@param` 8 times, all in a .js file. Reporting 1054 permanent violations + # would bury every other row. + # `unless(isLambda())` on the record, and `unless(ofClass(isLambda()))` on the method: + # a lambda written INSIDE a function body is code, not a documented declaration, and + # its compiler-synthesised closure class + call operator would otherwise be reported + # as 53 undocumented "methods". The AST knows the difference — this asks it, rather + # than testing the name, so a REAL `operator()` on a named functor class is still + # reported. + "matcher": ("namedDecl(anyOf(cxxRecordDecl(unless(isLambda())), " + "cxxMethodDecl(unless(ofClass(cxxRecordDecl(isLambda())))), " + "cxxConstructorDecl(), cxxDestructorDecl(), cxxConversionDecl(), " + "fieldDecl(), enumDecl(), " + "enumConstantDecl(), " + "functionDecl(unless(cxxMethodDecl(ofClass(cxxRecordDecl(isLambda())))))), " + 'unless(isExpansionInSystemHeader())).bind("d")'), + }, "heap": { "title": "Heap allocation sites", # `dump`, not `diag`: diag gives a location with no expression and print gives an @@ -158,7 +298,13 @@ # says which code allocates rather than only which line. clang-query then emits two # blocks per match ("fn" then "root"), which is why collect_heap parses per-match. "matcher": ("stmt(anyOf(cxxNewExpr(), cxxDeleteExpr(), callExpr(callee(functionDecl(" + # The project's OWN allocator belongs here, not just libc's: platform::alloc + # is how 35 sites in src/ acquire memory, and listing only `free` made the + # table read as "8 frees, 0 allocations" on HueDriver — an implied leak that + # was not there. allocInternal pairs with the ordinary free() (platform.h:63); + # allocExec/freeExec are their own pair. 'hasAnyName("malloc","calloc","realloc","free","strdup",' + '"alloc","allocInternal","allocExec","freeExec",' '"heap_caps_malloc","heap_caps_calloc","heap_caps_realloc","heap_caps_free",' '"ps_malloc","ps_calloc"), unless(cxxMethodDecl()))))), ' 'hasAncestor(functionDecl().bind("fn")))'), @@ -180,7 +326,7 @@ def module_files(module): def including_tus(files, build_dir): - """The translation units that reach `files` — the TUs worth parsing to analyse them. + """The translation units that reach `files` — the TUs worth parsing to analyze them. A header is not a TU: it is compiled through whichever .cpp includes it. Parsing every TU to reach one header costs minutes (263s here) when a single TU usually suffices (18s), so this @@ -217,19 +363,70 @@ def including_tus(files, build_dir): frontier = nxt hits = [tu for tu in tus if Path(tu).name in reached] - return hits or tus # no edge found -> analyse everything rather than nothing + return hits or tus # no edge found -> analyze everything rather than nothing def _source_tus(build_dir): - """The src/ translation units to analyse. + """The src/ translation units to analyze. Only src/: test/ TUs would double the runtime to report on code that never ships, and a - header is analysed through whichever .cpp includes it (hence the dedupe below). + header is analyzed through whichever .cpp includes it (hence the dedupe below). """ db = json.loads((build_dir / "compile_commands.json").read_text(encoding="utf-8")) return sorted({e["file"] for e in db if "/src/" in e["file"].replace("\\", "/")}) +# The marker a `//` comment carries once it has been rewritten into a doc comment for parsing. +# Plain text on purpose: an `@`-prefixed marker is parsed as a doc COMMAND and stripped from the +# text, so `@MMDEV` vanishes and the two kinds become indistinguishable again. This survives. +_DEV_MARK = "MMDEV:" + +# Leading `//` only — never `///` (already a doc comment) and never a trailing `x = 1; // note` +# (it documents nothing, and rewriting it would change the code line). +_LEADING_DEV = re.compile(r"^([ \t]*)//(?!/)( ?)", re.M) + + +def _shadow_tree(build_dir): + """A copy of `src/` where every leading `//` is a MARKED doc comment. + + Clang's lexer DISCARDS `//`: only `///` and `/** */` become AST nodes, because the compiler + consumes those itself. That leaves 76% of this tree's comment lines invisible to any matcher. + Rewriting them into `/// MMDEV: …` makes them parse, and the marker keeps them separable from + real doc comments — so one AST pass can report BOTH kinds against the declaration each one + documents, which is the whole point (a `//` on a class is a different finding from a `///`). + + The rewrite happens in a SHADOW COPY under the build dir; `src/` is never touched. That is the + line between this and a bad idea: the report describes the real tree, and the only thing the + marker changes is which lexer bucket a comment lands in. + + Two things probed and settled, recorded so they are not re-derived: + - `-Wdocumentation` WOULD fabricate warnings on rewritten dev notes (a `// @param wrong` + becomes a real diagnostic). It does not matter here: clang-query never enables it. + - Function PARAMETERS cannot carry a doc comment in C++ at all — 1054 ParmVarDecls in one TU, + zero with a comment, and `@param` appears 8 times in the tree (all in a .js file). Class + ATTRIBUTES (FieldDecl) are the per-member scope that does exist, and they are reported. + """ + shadow = build_dir / "comment-shadow" + # Reused within a run: the comments rule and the visibility pass both need it, and rebuilding + # (rmtree + copytree + rewrite of all of src/) twice per run bought nothing — the source + # cannot change mid-run. A stale tree from a PREVIOUS run would be wrong, so the marker file + # records which source state it was built from. + stamp = shadow / ".built-from" + newest = max((f.stat().st_mtime for f in (ROOT / "src").rglob("*") if f.is_file()), default=0) + if stamp.exists() and stamp.read_text(encoding="utf-8").strip() == str(newest): + return shadow + if shadow.exists(): + shutil.rmtree(shadow) + shutil.copytree(ROOT / "src", shadow / "src") + for f in list((shadow / "src").rglob("*.h")) + list((shadow / "src").rglob("*.cpp")): + text = f.read_text(encoding="utf-8", errors="replace") + marked = _LEADING_DEV.sub(rf"\1/// {_DEV_MARK}\2", text) + if marked != text: + f.write_text(marked, encoding="utf-8") + stamp.write_text(str(newest), encoding="utf-8") + return shadow + + def _run_rule(rule, tus, build_dir, tool): """Run one matcher over every TU, in parallel. @@ -243,12 +440,29 @@ def _run_rule(rule, tus, build_dir, tool): cmd = [tool, "-p", str(build_dir), "-f", str(qfile)] cmd += [f"--extra-arg={a}" for a in check_clang_tidy._toolchain_args()] + # The comments rule parses the shadow tree instead, so `//` is visible. The include path is + # prepended so a TU's `#include "core/Foo.h"` resolves to the rewritten header, not the real + # one — without it only the .cpp itself would be marked and every header would read as clean. + shadow = None + if rule.get("shadow"): + shadow = _shadow_tree(build_dir) + # `--extra-arg-before`, NOT `--extra-arg`: the latter APPENDS, so the compile + # database's own `-I…/src` was searched first and every transitive include resolved + # to the REAL, unmarked header. Only first-level quoted includes hit the shadow, so + # `//` counts were a systematic undercount — a silent zero for most shared headers. + cmd += [f"--extra-arg-before=-I{shadow / 'src'}"] + tus = [str(shadow / Path(tu).relative_to(ROOT)) for tu in tus] + def one(tu): p = subprocess.run(cmd + [tu], cwd=ROOT, capture_output=True, text=True) return p.stdout + p.stderr with ThreadPoolExecutor(max_workers=max(1, (os.cpu_count() or 4) - 2)) as ex: - return "".join(ex.map(one, tus)) + out = "".join(ex.map(one, tus)) + if shadow: + # Paths in the dump point into the shadow; map them back so a row is clickable. + out = out.replace(str(shadow / "src"), str(ROOT / "src")) + return out def _rel(path): @@ -408,6 +622,227 @@ def render_scratch(rows, max_rows=0): return L + note +# Access is NOT in the AST dump's declaration line — clang prints `private` only as a separate +# AccessSpecDecl node, which a per-block parse never sees. So visibility comes from a second, +# cheap matcher pass listing the non-public declarations by file:line, and rows are tagged +# against that set. +# +# Why report private members at all, rather than filtering them out: doxygen never publishes +# them, so the "it should reach the module page" argument does not apply — but a bloated comment +# is still bloat, and clangd's hover shows it to whoever maintains the code. The VIS column lets +# a reader weigh a finding (a public method with no `///` is an API gap; a private one is a +# maintenance note) without dropping half the tree from the report. +_NONPUBLIC_MATCHER = ("namedDecl(anyOf(cxxRecordDecl(), cxxMethodDecl(), cxxConstructorDecl(), " + "cxxDestructorDecl(), cxxConversionDecl(), fieldDecl(), enumDecl(), " + "enumConstantDecl(), functionDecl()), " + "anyOf(isPrivate(), isProtected()), " + 'unless(isExpansionInSystemHeader())).bind("d")') + + +def collect_nonpublic(out): + """The (file, line) of every private/protected declaration in the dump.""" + seen = set() + cur_path = None + for block in out.split('Binding for "d":')[1:]: + head = next((ln for ln in block.split("\n") if ln.strip()), "") + hint = _PATH_HINT.search(head) + if hint: + cur_path = hint["path"] + rel = _rel(cur_path) if cur_path else None + owner = _OWNER.search(head) + if rel and owner: + seen.add((rel, int(owner["line"]))) + return seen + + +def collect_comments(out): + """Every declaration, with the WORDS of `///` and `//` attached to it. + + Parsed per BOUND MATCH: `.bind("d")` makes clang-query emit one block per matched declaration, + headed by that declaration, so the owner is stated rather than inferred. Inferring it + positionally fails two ways — a class match dumps its whole subtree (so a member's comment + hangs off the class), and a trailing `///<` attaches to the declaration BEFORE it. + + Only a DIRECT child of the block head is this declaration's own comment; a deeper FullComment + belongs to a nested declaration, which has its own block. + + Deduplicated by (file, declaration line), and sizes are a MAX rather than a sum: a header + parsed by several TUs yields the same comment once per TU, and adding them reported a + 129-line class comment as 258. + """ + rows = {} + cur_path = None + for block in out.split('Binding for "d":')[1:]: + lines = block.split("\n") + head = next((ln for ln in lines if ln.strip()), "") + + hint = _PATH_HINT.search(head) + if hint: + cur_path = hint["path"] + rel = _rel(cur_path) if cur_path else None + owner = _OWNER.search(head) + if not rel or not owner: + continue + # `implicit` — a lambda's closure class or a compiler-injected self-reference: anonymous, + # so the dump's only "name" is the literal word "definition". + # `invalid` — a declaration clang could not fully parse in THIS TU (it parses fine in the + # TU that owns it). Same anonymous shape, and it would overwrite the good row. + if " implicit " in head or " invalid " in head: + continue + # A record with no body is a FORWARD DECLARATION (`class JsonSink;`) — there is nothing + # to document, and the real class is reported from the header that defines it. Clang + # marks a defined record with a trailing `definition`; without it, the two `class Foo;` + # lines at the top of every module header showed up as undocumented classes. + if _RECORD_HEAD.match(head) and not head.rstrip().endswith("definition"): + continue + + + # Both kinds accumulate: a `///` spec and a `//` note can sit on the same method, and + # keeping only whichever the dump listed first would hide half of it. A declaration with + # neither still earns a row — "undocumented" is the other half of the question. + seen = {"///": 0, "//": 0} + first_line = None + for k, line in enumerate(lines): + if "FullComment" not in line or not _TOP_CHILD.match(line): + continue + fc = _FULLCOMMENT.search(line) + if not fc: + continue + # Three span forms, and the PATH one is not optional: the dump prints a full path + # whenever the comment is the first node from a new file, which is exactly the case + # for a declaration whose doc comment opens a header. Missing it reported two + # correctly-documented methods as having no comment at all. + begin = int(fc["pstart"] or fc["start"] or owner["line"]) + body = "" + for sub in lines[k + 1:]: + # `@moreinfo` ends the LEAD description; everything after it is relocated below + # the member lists at render time and is not what the ideal measures. + if _MOREINFO_NODE.search(sub): + break + tc = _TEXTCOMMENT.search(sub) + if tc: + body += tc["text"] + " " + continue + # The subtree is not flat — ParagraphComment and friends wrap the text — so the + # scan ends only at a node that is not part of a comment at all. + if not _COMMENT_NODE.search(sub): + break + kind = "//" if _DEV_MARK in body else "///" + # The marker is not prose: strip it before counting, or every rewritten `//` block + # would read one word longer per line than it is on disk. + words = len(body.replace(_DEV_MARK, " ").split()) + seen[kind] = max(seen[kind], words) + if first_line is None: + first_line = begin + + # `= default` / `= delete` have no body to document — reporting them as undocumented is + # noise. They still count when they DO carry a comment. + if not seen["///"] and not seen["//"]: + if " default" in head or " delete" in head: + continue + + scope = _SCOPE.get(owner["kind"], "other") + ideal = _IDEAL_DOC_WORDS.get(scope, _IDEAL_DOC_WORDS["other"]) + # MAX across TUs, not last-writer-wins. A header is parsed once per TU that includes it, + # and a plain assignment let a later TU that saw fewer words overwrite an earlier one that + # saw more — the docstring claimed max while the code overwrote. + key = (rel, int(owner["line"])) + prev = rows.get(key) + if prev: + seen["///"] = max(seen["///"], prev["doc_words"]) + seen["//"] = max(seen["//"], prev["dev_words"]) + rows[key] = { + "file": rel, "line": first_line or int(owner["line"]), + # The DECLARATION's line, distinct from `line` (where its comment starts) — the + # visibility pass keys on the declaration, so the two must not be conflated. + "decl_line": int(owner["line"]), + "doc_words": seen["///"], "dev_words": seen["//"], + "scope": scope, + # DOC is a target, so the deviation is a signed percentage: 0% ideal, -100% absent, + # no ceiling. DEV has no target — zero IS the ideal — so `dev_words` above is + # reported raw, with no deviation column at all. + "doc_deviation": 100 * (seen["///"] - ideal) / ideal, + "name": owner["cname"] or owner["fname"] or "?", + } + # Absolute deviation: a bloated header and a missing comment are both wrong, in opposite + # directions, and ranking by |deviation| surfaces the two together rather than burying one. + return sorted(rows.values(), key=lambda r: -abs(r["doc_deviation"])) + + +def render_comments(rows, max_rows=0): + """The DECL x kind matrix, then every declaration ranked by how far it sits from the ideal. + + The project's rule is that every class, method and attribute carries a short readable `///` + — that is what moxydoc publishes — while `//` developer notes belong in the code lines rather + than stacked on a declaration. Three states, three different fixes, so the matrix shows which + way each kind of declaration is leaning. + + Sizes are WORDS, not lines: a line is a formatting accident (the same paragraph is 5 lines at + 100 columns and 9 at 60) while words are what a reader absorbs. Measured here, a comment line + carries a median of 13 words in both kinds. + + DOC DEVIATION is the signed % difference from `_IDEAL_DOC_WORDS`; DEV is an absolute + count because zero is the + ideal there, and a ratio would report the best case (no note at all) as -100%. + """ + L = [f"{len(rows)} found."] + if not rows: + return L + + scopes = ("class", "method", "attribute", "other") + blank = {"///": 0, "//": 0, "none": 0} + counts = {s: dict(blank) for s in scopes} + for r in rows: + c = counts.setdefault(r["scope"], dict(blank)) + if r["doc_words"]: + c["///"] += 1 + elif r["dev_words"]: + c["//"] += 1 + else: + c["none"] += 1 + + ideals = " · ".join(f"{s} {_IDEAL_DOC_WORDS[s]}" for s in ("class", "method", "attribute")) + # Column names match the detail table below, so a reader carries one vocabulary down the + # report rather than mapping "DOC ///" onto "DOC WORDS" halfway through. + L += ["", f" {'DECL':<10} {'DOC':>7} {'DEV':>7} {'NONE':>7} {'%DOC':>5}" + f" (ideal doc words: {ideals}; ideal dev words: 0)", + f" {'-' * 10} {'-' * 7} {'-' * 7} {'-' * 7} {'-' * 5}"] + for s in scopes: + c = counts.get(s, blank) + total = c["///"] + c["//"] + c["none"] + if total: + L.append(f" {s:<10} {c['///']:>7} {c['//']:>7} {c['none']:>7} " + f"{100 * c['///'] // total:>4}%") + + dev = sum(1 for r in rows if not r["doc_words"] and r["dev_words"]) + none = sum(1 for r in rows if not r["doc_words"] and not r["dev_words"]) + L += ["", f" {dev} declarations carry only a `//` where the rule asks for `///`; " + f"{none} carry no comment at all."] + + name_w = min(max(len(r["name"]) for r in rows), 30) + # The public subset, called out because it is the part that SHIPS as documentation: doxygen + # publishes only public members, so an undocumented public method is an API gap while a + # private one is a maintenance note. Both are reported; the split says which is which. + pub = [r for r in rows if r.get("vis") == "pub"] + if pub: + pub_doc = sum(1 for r in pub if r["doc_words"]) + L += [f" Public surface: {pub_doc} of {len(pub)} documented " + f"({100 * pub_doc // len(pub)}%) — the part doxygen publishes."] + + # DOC DEVIATION sits beside DOC WORDS: the percentage and the number it is derived from + # belong together, and DEV WORDS is a separate measure. "Deviation", not "ratio" — a ratio is + # a bare quotient (2.3x), this is a signed percentage difference from a target. + L += ["", f" {'DOC DEVIATION':>13} {'DOC WORDS':>9} {'DEV WORDS':>9} {'VIS':<4} " + f"{'DECL':<9} {'NAME':<{name_w}} FILE:LINE", + f" {'-' * 13} {'-' * 9} {'-' * 9} {'-' * 4} {'-' * 9} {'-' * name_w} {'-' * 30}"] + shown, note = _truncate(rows, max_rows) + for r in shown: + L.append(f" {r['doc_deviation']:>+12.0f}% {r['doc_words']:>9} {r['dev_words']:>9} " + f"{r.get('vis', '?'):<4} {r['scope']:<9} {r['name'][:name_w]:<{name_w}} " + f"{r['file']}:{r['line']}") + return L + note + + def render_arrays(rows, min_bytes, max_rows=0): over = f" over {min_bytes} bytes" if min_bytes else "" L = [f"{len(rows)} found{over}."] @@ -431,7 +866,7 @@ def render_arrays(rows, min_bytes, max_rows=0): # Which side of the lifecycle a site is on. `realloc` acquires (it can move and grow), so it # sits with the allocations; a bare `free`/`delete` releases. -_RELEASING = ("free()", "delete", "delete[]", "heap_caps_free()") +_RELEASING = ("free()", "freeExec()", "delete", "delete[]", "heap_caps_free()") def render_heap(rows, max_rows=0): @@ -450,7 +885,7 @@ def render_heap(rows, max_rows=0): def table(title, subset): if not subset: - return [f"", f"{title}: none."] + return ["", f"{title}: none."] kinds = collections.Counter(r["what"] for r in subset) what_w = min(max(len(r["what"]) for r in subset), 12) tgt_w = min(max((len(r["target"]) for r in subset), default=0), 24) or 1 @@ -495,7 +930,7 @@ def main(): f"run `uv run moondeck/build/build_desktop.py` first.", file=sys.stderr) return 2 - # A module's HEADER is not a translation unit — it is analysed through whichever .cpp + # A module's HEADER is not a translation unit — it is analyzed through whichever .cpp # includes it. So --module narrows the REPORT, not the TU list; narrowing the TUs would # miss every header-only module, which is most of the light domain. only = None @@ -530,8 +965,8 @@ def main(): # do not all say "error:" — an ambiguous top-level anyOf() reports "Input value has # unresolved overloaded type" — so key on "no matches at all", which is never a real # result for these rules on this codebase. - bad = [l for l in out.splitlines() - if "error: " in l or "unresolved overloaded type" in l or "not found" in l] + bad = [ln for ln in out.splitlines() + if "error: " in ln or "unresolved overloaded type" in ln or "not found" in ln] if bad and "binds here" not in out and "Match #" not in out: print(f"[{name}] clang-query rejected the matcher: {bad[0].strip()}", file=sys.stderr) return 2 @@ -541,12 +976,31 @@ def main(): rows = collect_arrays(out, args.min_bytes) elif name == "scratch": rows = collect_scratch(out) + elif name == "comments": + rows = collect_comments(out) + # A zero here is indistinguishable from a shadow tree that never got included: the + # first version appended `--extra-arg`, so every transitive header resolved to the + # REAL unmarked file and `//` counts silently halved. If NOTHING carries a dev + # comment, the rewrite did not reach the headers — say so instead of reporting it. + if rows and not any(r["dev_words"] for r in rows): + print("[comments] no `//` comment found anywhere — the shadow tree did not reach " + "the headers (include order?). Refusing to report a false zero.", + file=sys.stderr) + return 2 + # Second pass for visibility — see collect_nonpublic. Cheap next to the main run: + # same TUs, a matcher with no comment traversal. + nonpub = collect_nonpublic( + _run_rule({"output": "dump", "matcher": _NONPUBLIC_MATCHER, "shadow": True}, + tus, build_dir, tool)) + for r in rows: + r["vis"] = "priv" if (r["file"], r["decl_line"]) in nonpub else "pub" else: rows = collect_heap(out) if only: rows = [r for r in rows if r["file"] in only] body = (render_arrays(rows, args.min_bytes, args.max_rows) if name == "arrays" else render_scratch(rows, args.max_rows) if name == "scratch" + else render_comments(rows, args.max_rows) if name == "comments" else render_heap(rows, args.max_rows)) print("\n".join(body)) print() diff --git a/moondeck/check/check_codeql.py b/moondeck/check/check_codeql.py new file mode 100644 index 00000000..8bc432c5 --- /dev/null +++ b/moondeck/check/check_codeql.py @@ -0,0 +1,251 @@ +#!/usr/bin/env python3 +"""CodeQL's open alerts, read from GitHub — the Security tab as a card. + +CodeQL runs in CI (.github/workflows/codeql.yml), not locally: it is the one layer of the +analysis stack that sees whole-program taint, and it earns that slot on the six network packet +formats we parse. But its findings live behind the Security tab, which means they are invisible +while working — a report nobody opens is a report nobody reads. + +This FETCHES rather than scans. The CodeQL CLI would need a ~1 GB install and minutes per run to +reproduce what CI already computed, and the alert lifecycle (open / fixed / dismissed, tracked +across runs) is the part worth having — that is baselining we would otherwise build ourselves. +The trade is honest and stated in the output: this shows the last ANALYZED commit, so local edits +and unpushed commits are not in it. + +Not a gate, like the rest of the stack (docs/testing.md § Static analysis): it reports, the human +judges. Exit is 0 whenever the fetch succeeded, whatever the findings — and non-zero only when the +answer is unknown (no `gh`, not authenticated, no network), because "I could not look" must never +render as "nothing found". + +Usage: + uv run moondeck/check/check_codeql.py # open alerts, worst first + uv run moondeck/check/check_codeql.py --state fixed # what has been resolved + uv run moondeck/check/check_codeql.py --all # every state, including dismissed + uv run moondeck/check/check_codeql.py --module HueDriver +""" + +import argparse +import json +import subprocess +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent.parent + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import check_clang_query # noqa: E402 — the module→files resolver, one owner + +# Worst first. CodeQL reports two different severity scales: `security_severity_level` on the +# security queries (critical/high/medium/low) and `severity` on the quality ones +# (error/warning/note/recommendation). They are merged into one ordering here, because a reader +# wants "what matters most", not two tables to compare by eye. +_ORDER = {"critical": 0, "high": 1, "error": 2, "medium": 3, + "warning": 4, "low": 5, "note": 6, "recommendation": 7} + +MAX_ROWS = 60 + + +def _repo(): + """`owner/name` for the current checkout, or None when it cannot be determined.""" + # `check=False` does not cover the binary being ABSENT — that raises FileNotFoundError before + # any exit code exists, which is the commonest cold-start failure and would print a traceback + # instead of the install guidance the caller has ready. + try: + p = subprocess.run(["gh", "repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner"], + cwd=ROOT, capture_output=True, text=True, check=False) + except OSError: + return None + return p.stdout.strip() or None + + +def fetch(state): + """Alerts for the repo, or `(None, reason)` when the answer could not be obtained. + + A failure is returned rather than raised so the caller can say WHY the list is empty. An + empty list and an unreachable API look identical in a table, and only one of them means + the code is clean. + """ + repo = _repo() + if not repo: + return None, ("`gh` could not identify the repository. Install the GitHub CLI and run " + "`gh auth login`, or check that this is a GitHub checkout.") + # `state` must be passed EXPLICITLY for every query. Omitting it does not mean "any state" — + # the endpoint defaults to `open`, so an "all states" run that simply dropped the parameter + # returned the open alerts and called them all of them. Verified against the API: with no + # `state`, every returned alert has `"state": "open"`. + states = ("open", "fixed", "dismissed") if state == "all" else (state,) + out = [] + for st in states: + query = f"per_page=100&state={st}" + try: + p = subprocess.run(["gh", "api", "--paginate", + f"repos/{repo}/code-scanning/alerts?{query}"], + cwd=ROOT, capture_output=True, text=True, check=False) + except OSError as exc: + return None, (f"Could not run `gh` ({exc}). Install the GitHub CLI and run " + f"`gh auth login`.") + if p.returncode != 0: + err = (p.stderr or "").strip().splitlines() + detail = err[-1] if err else f"exit {p.returncode}" + # 404 is the common one and does not mean "clean": code scanning may be off, or the + # token may lack the security_events scope. + return None, (f"Could not read the alerts ({detail}). Code scanning may be disabled " + f"for {repo}, or `gh auth` may lack the `security_events` scope.") + # --paginate concatenates one JSON array per page; load them all. + dec = json.JSONDecoder() + text = p.stdout.strip() + i = 0 + while i < len(text): + try: + val, end = dec.raw_decode(text, i) + except ValueError: + break + if isinstance(val, list): + out.extend(val) + i = end + while i < len(text) and text[i] in " \t\r\n": + i += 1 + return out, None + + +def collect(alerts, only=None): + """One row per alert: severity, rule, where, and the message that says what is wrong.""" + rows = [] + for a in alerts: + rule = a.get("rule") or {} + inst = a.get("most_recent_instance") or {} + loc = inst.get("location") or {} + path = loc.get("path") or "?" + if only is not None and path not in only: + continue + sev = rule.get("security_severity_level") or rule.get("severity") or "note" + rows.append({ + "sev": sev.lower(), + "rule": rule.get("id") or "?", + "file": path, + "line": loc.get("start_line") or 0, + "msg": " ".join((inst.get("message") or {}).get("text", "").split()), + "state": a.get("state") or "?", + "ref": (inst.get("ref") or "").replace("refs/heads/", ""), + "url": a.get("html_url") or "", + }) + # Severity, then file/line so a re-run diffs cleanly and one file's findings stay together. + return sorted(rows, key=lambda r: (_ORDER.get(r["sev"], 99), r["file"], r["line"])) + + +def _table(rows, title, show_state=False): + """One severity-ordered table. Widths come from the data, capped, like the other reports. + + `show_state` adds the STATE column, which only earns its width when the listing mixes states + (an `--all` run) — on a single-state run the header already said which one. + """ + # Never narrower than the header itself, or the dashes stop lining up under it (a `note`-only + # table gave a 4-wide rule under an 8-wide "SEVERITY"). + sev_w = max(min(max(len(r["sev"]) for r in rows), 8), len("SEVERITY")) + rule_w = max(min(max(len(r["rule"]) for r in rows), 34), len("RULE")) + loc_w = max(min(max(len(f"{r['file']}:{r['line']}") for r in rows), 46), len("FILE:LINE")) + st_w = max(min(max(len(r["state"]) for r in rows), 9), len("STATE")) if show_state else 0 + + def clip(s, w): + return s if len(s) <= w else s[: w - 1] + "…" + + st_head = f"{'STATE':<{st_w}} " if show_state else "" + st_rule = f"{'-' * st_w} " if show_state else "" + L = ["", f"{title} — {len(rows)}", + f" {st_head}{'SEVERITY':<{sev_w}} {'RULE':<{rule_w}} {'FILE:LINE':<{loc_w}} WHAT", + f" {st_rule}{'-' * sev_w} {'-' * rule_w} {'-' * loc_w} {'-' * 40}"] + for r in rows[:MAX_ROWS]: + loc = f"{r['file']}:{r['line']}" + st = f"{clip(r['state'], st_w):<{st_w}} " if show_state else "" + L.append(f" {st}{clip(r['sev'], sev_w):<{sev_w}} {clip(r['rule'], rule_w):<{rule_w}} " + f"{clip(loc, loc_w):<{loc_w}} {clip(r['msg'], 60)}") + if len(rows) > MAX_ROWS: + L.append(f" … {len(rows) - MAX_ROWS} more (severity order, so these are the least " + f"severe). Use --module <name> to scope.") + return L + + +def render(rows, state, refs, scoped=False): + """The counts first, then shipping code, then tests. + + Split because the two are read differently: a finding in `src/` ships to a device, one in + `test/` does not. Measured here, 783 of 855 alerts are in test files — pooling them buries + the three `high` findings in shipping code under a wall of doctest noise. + + The severity breakdown is printed BEFORE any table, so the numbers survive truncation: a + reader must be able to see "3 high" even when the table showed only the first 60 rows. + """ + L = [] + if not rows: + L.append(f"No {state} alerts. ✓") + if scoped: + L.append(" (Scoped to a module — run without one to see the whole repo.)") + L.append(" (CodeQL analyzes PUSHED commits — local edits are not included.)") + return L + + branches = ", ".join(sorted(refs)) if refs else "?" + L.append(f"{len(rows)} {state} alert(s) — analyzed on {branches}, not on your working tree.") + + src = [r for r in rows if r["file"].startswith("src/")] + other = [r for r in rows if not r["file"].startswith("src/")] + + def tally(subset): + c = {} + for r in subset: + c[r["sev"]] = c.get(r["sev"], 0) + 1 + return " · ".join(f"{k} {v}" for k, v in + sorted(c.items(), key=lambda kv: _ORDER.get(kv[0], 99))) or "none" + + L.append(f" shipping code (src/): {tally(src)}") + L.append(f" tests + vendored: {tally(other)}") + + # With more than one state in the list, say which — a `fixed` row rendered like an open one + # reads as a live finding. Absent on a single-state run, where the header already said it. + states = {} + for r in rows: + states[r["state"]] = states.get(r["state"], 0) + 1 + if len(states) > 1: + L.append(" by state: " + + " · ".join(f"{k} {v}" for k, v in sorted(states.items()))) + + mixed = len(states) > 1 + if src: + L += _table(src, "src/ — ships to a device", mixed) + if other: + L += _table(other, "test/ + vendored — does not ship", mixed) + return L + + +def main(): + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("--state", default="open", choices=("open", "fixed", "dismissed"), + help="Alert state to list (default: open).") + ap.add_argument("--all", action="store_true", help="Every state, not just one.") + ap.add_argument("--module", help="Only alerts in this module's source files.") + args = ap.parse_args() + + state = "all" if args.all else args.state + alerts, err = fetch(state) + if alerts is None: + # FAIL LOUD. A tool that could not look must not print a comfortable zero. + print(err, file=sys.stderr) + return 2 + + only = None + if args.module: + only = check_clang_query.module_files(args.module) + if not only: + print(f"No source files for module '{args.module}'.", file=sys.stderr) + return 2 + print(f"Filtered to {args.module}: {', '.join(only)}\n") + + rows = collect(alerts, only) + # The branch comes from the alerts themselves; when none matched a scope there is nothing to + # read it from, so fall back to every alert's ref rather than printing "?". + refs = {r["ref"] for r in (rows or collect(alerts)) if r["ref"]} + print("\n".join(render(rows, state, refs, scoped=only is not None))) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/moondeck/check/check_module.py b/moondeck/check/check_module.py index 8c42e19c..145c7d08 100644 --- a/moondeck/check/check_module.py +++ b/moondeck/check/check_module.py @@ -36,13 +36,16 @@ sys.path.insert(0, str(HERE)) import check_clang_query # noqa: E402 — the module→files resolver, one owner -# `--all` on lizard and `--max-rows=0` on clang-query: the repo-wide defaults exist to keep a -# 362-row sweep readable, but you scoped to ONE module precisely to see all of its findings. -# Truncating here would hide the tail that scoping was meant to expose (HttpServerModule alone -# has 71 arrays). +# `--all` on lizard ignores the baseline: you scoped to ONE module precisely to see all of its +# findings, not just the ones newer than the freeze. +# +# clang-query keeps its 60-row cap, unlike lizard. It used to run `--max-rows=0` on the same +# reasoning, but the comments rule made a single module's report unreadable — HttpServerModule +# alone prints 212 declarations, and a wall of rows is skimmed rather than read. The cap is per +# TABLE and always announces what it dropped, so the tail is one `--max-rows=0` away. TOOLS = [ ("clang-tidy", ["check_clang_tidy.py"]), - ("clang-query", ["check_clang_query.py", "--max-rows=0"]), + ("clang-query", ["check_clang_query.py"]), ("lizard", ["check_lizard.py", "--all"]), ] diff --git a/moondeck/check/check_nonblocking.py b/moondeck/check/check_nonblocking.py index 44422867..bf108976 100644 --- a/moondeck/check/check_nonblocking.py +++ b/moondeck/check/check_nonblocking.py @@ -10,8 +10,14 @@ warning, so a raw build prints ~1350 lines for ~175 real findings; deduplicating on (file, line) is most of this script's value. +Two engines, each doing what it is good at. The compiler finds the blocking calls, because +`-Wfunction-effects` is transitive and a matcher would have to rebuild the call graph to match +it. clang-query then annotates each site with the branch that guards it (the COND column) — +a purely local AST question. Sites that run unconditionally sort first: they cost every tick, +where a guarded one may not run at all. + Not a gate. `-Wno-error=function-effects` in CMakeLists keeps the build green while these are -triaged; each finding is a judgement — fix it, annotate the callee, or accept it with a scoped +triaged; each finding is a judgment — fix it, annotate the callee, or accept it with a scoped reason — and the answer differs per site. Usage: @@ -20,7 +26,6 @@ """ import argparse -import collections import re import subprocess import sys @@ -110,9 +115,285 @@ def write_baseline(rows): # Which tick tier a function belongs to, once resolved through the enclosing method. TIERS = ("tick", "tick20ms", "tick1s") +# ---------------------------------------------------------------- guard form (COND) +# +# Whether a flagged call actually runs every time its tick does. `-Wfunction-effects` answers +# "can this block", never "is it reached" — so a report without this column ranks a call behind +# `if (++n >= 50)` (fires twice a second) level with one that runs unconditionally every frame. +# +# Asked of clang-query rather than scanned from the text, because the branch forms in this tree +# defeat a text scan: `if constexpr` (AudioService.h — a text scan reads it as a plain `if`), +# conditions spanning lines, and macros. The ancestor of a call is exactly the kind of LOCAL AST +# fact clang-query is for. Detection stays with the compiler: -Wfunction-effects is TRANSITIVE +# through the call graph, which a matcher would have to rebuild by hand. +# +# GUARDED IS NOT RARE. `if (enabled_)` is conditional and true every frame; only the rate hint +# below separates those, and it is a heuristic — read it as a lead, not a verdict. -def enclosing_function(rel_path, line, _cache={}): - """The method a call site sits in, or "" when it cannot be determined.""" +# One matcher per branch kind, each naming the guard from the OUTSIDE in (`ifStmt(hasDescendant( +# callExpr))`) rather than from the call outwards. `hasAncestor(...).bind()` is rejected outright +# — "Matcher does not support binding" — so the kind cannot be read back from a combined query; +# running them separately is what makes the answer unambiguous. +# +# Cheap enough to do this way: measured ~1s per TU (a narrow matcher, unlike the comments rule's +# ~44s whole-subtree dump), so seven passes over the TUs holding findings add seconds to a report +# whose cost is dominated by the clean rebuild. +# +# Ordered most-specific first: the first form to claim a site keeps it, so a call in a loop body +# reads as `loop` even when an `if` also encloses it — the loop says more about how often it runs. +# `forEachDescendant`, NOT `hasDescendant`: the latter binds only the FIRST matching descendant +# per guard, so a branch containing several calls reported one and left the rest reading as +# unconditional. Measured on the `for` at DevicesModule.h:274 — `hasDescendant` bound only +# mergePacket (:277) while recvFrom (:275) in the same loop came back blank. +# +# Each matcher names the GUARDED REGION, never the whole statement. A call in an `if`'s CONDITION +# runs every time the `if` is reached — `if (!listener_.open()) return;` (DevicesModule.h:385) is +# an unconditional call to a blocking function — so matching the bare `ifStmt` labelled it `if` +# and sorted it below the unconditional rows, i.e. wrong in the reassuring direction. Same for a +# loop's condition, a `switch`'s subject, and the LHS of `&&`/`||`, which is always evaluated. +_GUARD_MATCHERS = ( + ("loop", "forStmt(hasBody(forEachDescendant(callExpr().bind(\"c\"))))"), + ("loop", "whileStmt(hasBody(forEachDescendant(callExpr().bind(\"c\"))))"), + ("loop", "cxxForRangeStmt(hasBody(forEachDescendant(callExpr().bind(\"c\"))))"), + ("if", "ifStmt(anyOf(hasThen(forEachDescendant(callExpr().bind(\"c\")))," + " hasElse(forEachDescendant(callExpr().bind(\"c\")))))"), + # `switchCase`, not `switchStmt`: the latter has no `hasBody` (clang-query rejects it), and + # matching the whole statement would claim calls in the SUBJECT expression, which always runs. + # `switchCase` covers `case` and `default` alike — the bodies, which is the guarded region. + ("switch", "switchCase(forEachDescendant(callExpr().bind(\"c\")))"), + # Only the branches, not the condition. + ("?:", "conditionalOperator(anyOf(" + " hasTrueExpression(forEachDescendant(callExpr().bind(\"c\")))," + " hasFalseExpression(forEachDescendant(callExpr().bind(\"c\")))))"), + # `a && blocking()` — the RHS runs only if the left side allows it; the LHS always runs. + ("&&", "binaryOperator(hasAnyOperatorName(\"&&\", \"||\")," + " hasRHS(forEachDescendant(callExpr().bind(\"c\"))))"), +) + +# A guarded early exit: `if (...) return;` / `continue` / `break`. These guard everything BELOW +# them in the function, which no ancestor matcher can see — the call is genuinely unnested, so +# every matcher above reports nothing and the site reads as "runs every time". +# +# That was actively wrong, not merely incomplete. HueDriver::tick calls pushOneChangedLight +# unnested (HueDriver.h:112) but four early returns above it — including a `now - lastPutMs_ < +# kPutIntervalMs` rate limit — mean it runs at most once per interval, not once per frame. +# +# `returnStmt` ONLY. `continue`/`break` leave the enclosing LOOP, not the function, so one above +# a call — in a drain loop that has already closed, say — guards nothing about that call, and +# counting it labelled an unconditional site as guarded. A call genuinely inside the loop body is +# the `loop` matcher's to claim. +_EXIT_MATCHER = ("functionDecl(forEachDescendant(" + " returnStmt(hasAncestor(ifStmt())).bind(\"c\")))") + +# The bound call's own header line in a dump, whose start position is the join key onto a +# warning: `CallExpr 0x7a0ee05e8 </abs/path/File.h:369:11, col:25> 'int'`. Three location spellings +# appear — a full path, a bare `line:369:11` when the file is unchanged from the previous node, +# and `col:11` when the line is too — so file and line are both optional and inherited when absent. +_CALL_LOC = re.compile(r"^\w+ 0x\w+ <(?:(?P<file>(?!line:|col:)[^:<>]+):)?" + r"(?:line:(?P<line>\d+)|(?P<full>\d+)|col:\d+)") + +# A rate-limiting guard: a counter compared against a constant or a `k`-prefixed limit, the +# `if (++broadcastTick_ >= kBroadcastEverySec)` shape. Also a once-only latch (`if (!inited_)`), +# which runs its body exactly once for the life of the process. +# +# `> 0` is deliberately NOT a rate limit: `if (pairTicksLeft_ > 0)` is a plain non-empty test, and +# accepting any integer literal marked four unrelated guards in HueDriver::tick1s as throttled. +# A threshold is a named `k` constant or a literal of 2 or more. +_LIMIT = r"(?:k[A-Z]\w*|[2-9]\d*|\d{2,})" +_RATE = re.compile(rf"(\+\+|--)\s*\w+\s*(>=|>|==)|" # ++n >= limit + rf"\w+\s*(>=|>)\s*{_LIMIT}\b|" # n >= kLimit / n >= 50 + rf"[-+]\s*\w+\s*(<|<=)\s*{_LIMIT}\b|" # now - last < kIntervalMs + rf"%\s*{_LIMIT}\s*==") # n % kEvery == 0 +# +# The latch word must END the identifier (`inited_`, `sawLights`, `isOpen`) rather than sit inside +# it: a trailing `\w*` let `open` match `online`, so `if (!online) return;` — a plain connectivity +# guard — marked DevicesModule's ageOut call as rate-limited. +_LATCH_WORD = r"(?:inited|ready|open|opened|started|done|valid)" +_LATCH = re.compile(rf"!\s*\w*{_LATCH_WORD}_?\b|" + rf"\w*{_LATCH_WORD}_?\s*==\s*false") + + +def _rate_on_line(rel_path, line, _cache={}): + """True when THIS one line spells a rate limiter or a once-only latch — no surrounding scan. + + Used for early exits, where the condition and the `return` share a line and any wider window + would read a neighbouring block that does not guard the call at all. + """ + src = _cache.get(rel_path) + if src is None: + f = ROOT / rel_path + src = _cache[rel_path] = (f.read_text(encoding="utf-8", errors="replace").split("\n") + if f.exists() else []) + if not 0 < line <= len(src): + return False + return bool(_RATE.search(src[line - 1]) or _LATCH.search(src[line - 1])) + + +def _matcher_rejected(out): + """True when clang-query refused the matcher rather than finding nothing. + + The two are indistinguishable by exit code — a rejected matcher still exits 0 with no matches. + + Matched on clang-query's OWN diagnostic shapes, anchored to the start of the line. A substring + search for `error: ` finds one in the dumped source too: `snprintf(…, "error: apply failed")` + is a string literal in a matched node, and treating that as a rejection blanked the whole COND + column for a matcher that had in fact produced 255 matches. + """ + # A rejection is reported as `<line>:<col>: <complaint>` against the QUERY text, before any + # matching happens, and never alongside a match. So: complaint present AND nothing matched. + rejected = ("Input value has unresolved overloaded type", + "Error parsing argument", + "Error building matcher", + "Matcher does not support binding", + "Invalid matcher") + if "Match #" in out: + return False # it ran and produced matches — not a rejection + return any(phrase in out for phrase in rejected) + + +def guard_forms(rows, build_dir, tool): + """`(file, line) -> guard form` for every flagged site, from the AST. + + Runs one clang-query pass per branch kind over only the TUs that actually contain findings, + then joins on the call's start position — the same key the warnings carry. + + Returns None when the answer could not be obtained — no clang-query, or a matcher it refused. + The caller then renders the column as `?`, because a tool that could not look must never read + as "nothing is guarded", which is the comfortable answer and the wrong one. + """ + if not tool: + return None + # Every TU that includes a file with findings. Headers have no TU of their own — they are + # analyzed through whichever .cpp includes them — so the set is resolved, not guessed. + tus = check_clang_query.including_tus({r["file"] for r in rows}, build_dir) + if not tus: + return None + + def run(form, matcher): + """Bound sites for one matcher, or None when clang-query could not answer.""" + try: + out = check_clang_query._run_rule( + {"output": "dump", "matcher": matcher}, tus, build_dir, tool) + except Exception as exc: + print(f"clang-query failed on the {form} matcher ({exc}); COND cannot be reported.", + file=sys.stderr) + return None + # A matcher clang-query cannot resolve yields ZERO matches and exit 0 — indistinguishable + # from "no site is guarded". Same detection as check_clang_query.main; without it a single + # matcher-syntax drift silently blanks the column and every row reads as unconditional. + if _matcher_rejected(out): + print(f"clang-query rejected the {form} matcher; COND cannot be reported.", + file=sys.stderr) + return None + return list(_bound_sites(out)) + + found = {} + for form, matcher in _GUARD_MATCHERS: + sites = run(form, matcher) + if sites is None: + return None + for rel, line in sites: + found.setdefault((rel, line), form) + + # Early exits last: only a site that no enclosing branch claimed can be guarded this way, + # and being INSIDE an `if` says more about how often a call runs than sitting after one. + sites = run("ret", _EXIT_MATCHER) + if sites is None: + return None + exits = {} + for rel, line in sites: + exits.setdefault(rel, set()).add(line) + for r in rows: + if (r["file"], r["line"]) in found: + continue + # An exit guards this call only if it sits ABOVE it in the SAME function, so the search + # is bounded by the enclosing method's own first line. + start = function_start(r["file"], r["line"]) + if not start: + continue + above = [ln for ln in exits.get(r["file"], ()) if start < ln < r["line"]] + if above: + # Test the EXIT lines themselves, not a window around them: an early exit's condition + # is on its own line (`if (now - last < kInterval) return;`), and walking up from it + # would pick up whatever unrelated block sits above — which marked ageOut (:286) as + # rate-limited from a `++n >= k` block that had already closed at :284. + found[(r["file"], r["line"])] = ("ret·rate" if any( + _rate_on_line(r["file"], ln) for ln in above) else "ret") + return found + + +def _bound_sites(out): + """`(repo-relative file, line)` for every node bound as "c" in a clang-query dump. + + `dump`, not `print`: print emits the SOURCE TEXT of a match with no location, and the join + onto a warning needs file:line. The bound node's own header line carries it. + + The file is tracked across blocks because a dump states a path once and then spells later + positions `line:274:9` against it. Two traps that cost real coverage here: + - it must hold the RAW path, not `_rel()` of it. `_rel` returns None outside src/, so + filtering at assignment made one SDK match blank the file for every `line:`-only match + after it — silently dropping our own sites (DevicesModule.h:275 read as unconditional + while sitting inside a `for`). + - it must reset per TU. `_run_rule` concatenates every TU's output, so a path carried across + the seam labels one TU's matches with another's file. + """ + for chunk in out.split("Match #1:"): + cur = None + for block in chunk.split('Binding for "c":')[1:]: + head = next((ln for ln in block.split("\n") if ln.strip()), "") + m = _CALL_LOC.search(head) + if not m: + continue + if m["file"]: + cur = m["file"] + # A bare `col:` form inherits BOTH file and line from the enclosing context, so it + # carries no line of its own and is skipped — guessing one would join the guard onto + # the wrong warning, worse than leaving the column blank. + line = m["line"] or m["full"] + rel = check_clang_query._rel(cur) if cur else None + if rel and line: + yield rel, int(line) + + +def rate_hint(rel_path, line, _cache={}): + """True when the guard above `line` looks like a rate limiter or a once-only latch. + + Read from the source text, not the AST: this asks what the condition MEANS, and a counter + compared to a constant is a spelling question, not a structural one. Heuristic by nature — + it can miss a rate limiter written another way, so it only ever ADDS a hint to a site the + AST already called conditional. + """ + src = _cache.get(rel_path) + if src is None: + f = ROOT / rel_path + src = _cache[rel_path] = (f.read_text(encoding="utf-8", errors="replace").split("\n") + if f.exists() else []) + if not 0 < line <= len(src): + return False + # Walk UP out of the call's own block, stopping at the line that opened it — the guard. + # `if (++n >= k) {` / `n = 0;` / `send();` is a common shape, so a fixed one-line window misses + # the throttle (measured: DevicesModule.h:281 guards the call at :283, two lines down). + # + # Bounded by INDENTATION rather than a line count: only lines indented at least as far as the + # call belong to its block, so the scan cannot wander into a sibling guard above. A fixed wider + # window did exactly that — HueDriver::tick1s line 123's `++reportTick_ >= kReportEverySec` + # marked the four unrelated guards above it as rate-limited. + indent = len(src[line - 1]) - len(src[line - 1].lstrip()) + scan = [line - 1] + for i in range(line - 2, max(-1, line - 12), -1): + if not src[i].strip(): + continue + scan.append(i) + if len(src[i]) - len(src[i].lstrip()) < indent: + break # this line opened the block — the guard itself; stop here + for i in scan: + if i >= 0 and (_RATE.search(src[i]) or _LATCH.search(src[i])): + return True + return False + + +def _enclosing_def(rel_path, line, _cache={}): + """`(name, 1-based line)` of the definition a call site sits in, or `("", 0)`.""" src = _cache.get(rel_path) if src is None: f = ROOT / rel_path @@ -121,8 +402,20 @@ def enclosing_function(rel_path, line, _cache={}): for i in range(min(line, len(src)) - 1, -1, -1): m = _DEF.match(src[i]) if m and len(m.group("indent")) <= 4 and m.group("name") not in _KEYWORDS: - return m.group("name") - return "" + return m.group("name"), i + 1 + return "", 0 + + +def enclosing_function(rel_path, line): + """The method a call site sits in, or "" when it cannot be determined.""" + return _enclosing_def(rel_path, line)[0] + + +def function_start(rel_path, line): + """The line the enclosing definition opens on, or 0 — the lower bound when asking whether an + early exit sits above a call. Without it, a `return` from the function ABOVE this one would + read as guarding a call it cannot reach.""" + return _enclosing_def(rel_path, line)[1] def build_output(build_dir, clean=True): @@ -240,6 +533,26 @@ def main(): print(f"Filtered to {args.module}: {', '.join(only)}\n") rows = [r for r in rows if r["file"] in only] + # Annotate each site with the branch that guards it. Separate pass, separate tool: the + # compiler says WHAT can block, the AST says WHETHER it is reached every tick. + tool = check_clang_tidy._find_tool("clang-query") + guards = guard_forms(rows, build_dir, tool) + for r in rows: + if guards is None: + # Unknown, NOT unconditional. A missing or refused tool is not an answer, and `—` + # would read as one. + r["cond"] = "?" + else: + form = guards.get((r["file"], r["line"]), "") + # `ret` is already final — guard_forms tested the exit lines themselves and appended + # `·rate` if one of THEM throttles. Re-testing here would scan upward from the call + # instead and pick up any unrelated block above it (measured: DevicesModule's ageOut + # read `ret·rate` from a `++n >= k` block that had already closed two lines earlier). + if not form or form.startswith("ret") or "·" in form: + r["cond"] = form or NO_CAUSE + else: + r["cond"] = f"{form}·rate" if rate_hint(r["file"], r["line"]) else form + n_new = sum(1 for r in rows if r.get("new")) print(f"{len(rows)} call(s) from the render path that can block or allocate.") if has_baseline: @@ -263,6 +576,14 @@ def tier_of(r): def table(title, subset, note): if not subset: return + # Unconditional first: a call that runs every time its tick does is strictly worse than + # the same call behind a branch. (file, line) breaks ties, so the order stays stable + # between runs and findings in one file stay together — that is how they get fixed. + subset = sorted(subset, key=lambda r: (r["cond"] not in (NO_CAUSE, "?"), + r["file"], r["line"])) + # Never narrower than the header: a tier where every row is `—` is width 1, and the dashes + # then stop lining up under "COND". + cond_w = max(min(max(len(r["cond"]) for r in subset), 9), len("COND")) callee_w = min(max(len(r["callee"]) for r in subset), 36) fn_w = min(max(len(r["fn"] or "?") for r in subset), 18) why_w = min(max((len(r["why"]) for r in subset), default=0), 44) or 1 @@ -272,18 +593,32 @@ def clip(s, w): return s if len(s) <= w else s[: w - 1] + "\u2026" print(f"\n{title} \u2014 {len(subset)} ({note})") - print(f" {'CALLS':<{callee_w}} {'IN':<{fn_w}} {'WHY IT BLOCKS':<{why_w}} FILE:LINE") - print(f" {'-' * callee_w} {'-' * fn_w} {'-' * why_w} {'-' * loc_w}") + print(f" {'COND':<{cond_w}} {'CALLS':<{callee_w}} {'IN':<{fn_w}} " + f"{'WHY IT BLOCKS':<{why_w}} FILE:LINE") + print(f" {'-' * cond_w} {'-' * callee_w} {'-' * fn_w} " + f"{'-' * why_w} {'-' * loc_w}") for r in subset[:MAX_ROWS]: why = r["why"] or NO_CAUSE mark = "NEW " if r.get("new") else " " - print(f" {mark}{clip(r['callee'], callee_w):<{callee_w}} " + print(f" {mark}{clip(r['cond'], cond_w):<{cond_w}} " + f"{clip(r['callee'], callee_w):<{callee_w}} " f"{clip(r['fn'] or '?', fn_w):<{fn_w}} " f"{clip(why, why_w):<{why_w}} " f"{r['file']}:{r['line']}") if len(subset) > MAX_ROWS: print(f" \u2026 {len(subset) - MAX_ROWS} more. Use --module <name> to scope.") + if guards is not None: + print(f"\nCOND = the branch guarding the call ({NO_CAUSE} = none, runs every time). " + f"`\u00b7rate` marks a\nrate limiter or once-only latch. Guarded is not rare: " + f"`if (enabled_)` still runs every frame.") + elif not tool: + print("\nCOND = ? \u2014 clang-query not found, so no site could be checked for a guard. " + "Install\nLLVM (brew install llvm) for the column. Unknown is not unconditional.") + else: + print("\nCOND = ? \u2014 the guard query did not complete (reason above), so no site could " + "be\nchecked. Unknown is not unconditional.") + for tier, note in (("tick", "every frame \u2014 the hot path"), ("tick20ms", "50x a second"), ("tick1s", "once a second \u2014 sub-hot path"), diff --git a/moondeck/docs/mkdocs_hooks.py b/moondeck/docs/mkdocs_hooks.py index e668dea6..e60e0165 100644 --- a/moondeck/docs/mkdocs_hooks.py +++ b/moondeck/docs/mkdocs_hooks.py @@ -15,6 +15,7 @@ /install/ so the local preview mirrors production's single-origin layout. """ +import logging import os import re from pathlib import Path @@ -53,7 +54,8 @@ # Repo top-level dirs/files a doc may link into but the site doesn't host. _OUT_OF_DOCS = ("src/", "moondeck/", "test/", "esp32/", "web-installer/", - ".github/", "CLAUDE.md", "README.md", "library.json", "CMakeLists.txt") + ".github/", "CLAUDE.md", "README.md", "library.json", "CMakeLists.txt", + ".clang-tidy", ".clangd") # A markdown link into a repo file the site doesn't host. Two authored shapes, both # common in the transient history/plans + backlog notes: @@ -247,7 +249,7 @@ def _render_catalog_table(markdown: str) -> str: passes through verbatim, so a table only ever contains genuine module blocks (a `##` heading closes the current table). Source .md stays authored as prose.""" lines = markdown.split("\n") - details_names = {_DETAILS_RE.match(l).group("name") for l in lines if _DETAILS_RE.match(l)} + details_names = {_DETAILS_RE.match(ln).group("name") for ln in lines if _DETAILS_RE.match(ln)} out = [] rows = [] # accumulated table rows for the current run @@ -368,6 +370,13 @@ def _add(src_uri: str, content: str): # ride along untouched. _DOCS_PREFIXED_HREF_RE = re.compile(r'href="docs/([^"#]+?)(\.md)?(#[^"]*)?"') +# The same borrowed text also links to files OUTSIDE docs/ (`moondeck/MoonDeck.md`), which +# have no page on the site at all. `_rewrite_out_of_docs_links` already sends those to the +# GitHub blob, but it runs at markdown stage where this page is still an unexpanded +# `--8<--` directive — so the embedded ones need the same treatment here. +_OUT_OF_DOCS_HREF_RE = re.compile( + rf'href="({"|".join(re.escape(p) for p in _OUT_OF_DOCS)})([^"#]*)(#[^"]*)?"') + # The page's first `<h1 ...>text</h1>`, split so the tag and its attributes (the id the # table of contents anchors on) survive and only the text is replaced. _FIRST_H1_RE = re.compile(r"(<h1[^>]*>)(.*?)(</h1>)", re.S) @@ -393,7 +402,32 @@ def _rebase_repo_root_doc_links(html): def _fix(m): path, _, anchor = m.group(1), m.group(2), m.group(3) or "" return f'href="{path}.html{anchor}"' if _ else f'href="{path}{anchor}"' - return _DOCS_PREFIXED_HREF_RE.sub(_fix, html) + html = _DOCS_PREFIXED_HREF_RE.sub(_fix, html) + return _OUT_OF_DOCS_HREF_RE.sub( + lambda m: f'href="{_BLOB_BASE}/{m.group(1)}{m.group(2)}{m.group(3) or ""}"', html) + + +class _MuteRebasedLinkWarnings(logging.Filter): + """Drop the link warnings for the page whose links this hook rebases after validation. + + `principles-and-process.md` embeds CLAUDE.md, whose links (`docs/architecture.md`, + `moondeck/MoonDeck.md`) are correct where that file is actually read — the repo root and + GitHub — and `_rebase_repo_root_doc_links` turns them into working site links. But + validation is a markdown treeprocessor inside `Page.render()`, so it judges the + pre-rebase text and reports 13 broken links the built site does not have. + + Filtering the log is the narrow fix. `validation.links.not_found: ignore` in mkdocs.yml + would hide genuinely missing pages across every other doc, and MkDocs has no per-page + validation setting; the `inclusion` levels that do suppress warnings (`DRAFT` and below) + also drop the page from the built site. + """ + + def filter(self, record): + msg = record.getMessage() + return not any(f"Doc file '{p}' contains a link" in msg for p in _EMBEDS_REPO_ROOT_FILE) + + +logging.getLogger("mkdocs.structure.pages").addFilter(_MuteRebasedLinkWarnings()) def on_page_content(html, page, config, files): diff --git a/moondeck/docs/update_module_docs.py b/moondeck/docs/update_module_docs.py index a672ed78..9f176d0c 100644 --- a/moondeck/docs/update_module_docs.py +++ b/moondeck/docs/update_module_docs.py @@ -74,6 +74,26 @@ def type_name_from_md(md_file: Path) -> str: return md_file.stem +def resolve_asset(path: Path) -> Path | None: + """`path` as it is ACTUALLY spelled on disk, or None when no such file exists. + + `Path.exists()` answers through the filesystem, and macOS's is case-insensitive: it reports + True for `assets/core/layouts.png` when the file is `Layouts.png`. The link written from that + path then resolves locally and 404s on the case-sensitive Linux runner that builds the + published site — a failure that cannot reproduce on the machine that produced it. + + So the parent directory is listed and matched case-insensitively, and the REAL name is + returned. Catalog pages are the case that needs it: `layouts.md` has a lowercase stem while + the screenshot follows the `<TypeName>.png` convention. + """ + if path.exists() and path.name in {p.name for p in path.parent.iterdir()}: + return path # exact match, including case + if not path.parent.is_dir(): + return None + lowered = path.name.lower() + return next((p for p in sorted(path.parent.iterdir()) if p.name.lower() == lowered), None) + + def insert_extra_shot(doc_path: Path, filename: str, anchor: str, dry_run: bool) -> bool: """Insert an image reference for filename into doc_path after anchor. @@ -205,14 +225,16 @@ def main() -> int: for md_file in sorted(DOCS_DIR.rglob("*.md")): type_name = type_name_from_md(md_file) - png = asset_dir_for(type_name) / f"{type_name}.png" - gif = asset_dir_for(type_name) / f"{type_name}.gif" + # Resolved through the directory listing, not Path.exists(), so the link carries the name + # as it is really spelled — see resolve_asset. + png = resolve_asset(asset_dir_for(type_name) / f"{type_name}.png") + gif = resolve_asset(asset_dir_for(type_name) / f"{type_name}.gif") - if not png.exists(): + if png is None: skipped_no_screenshot.append(md_file.relative_to(ROOT)) continue - changed = insert_assets(md_file, png, gif if gif.exists() else None, args.dry_run) + changed = insert_assets(md_file, png, gif, args.dry_run) if changed: rel = md_file.relative_to(ROOT) verb = "would update" if args.dry_run else "updated" diff --git a/moondeck/moondeck.py b/moondeck/moondeck.py index 31175e50..01050562 100644 --- a/moondeck/moondeck.py +++ b/moondeck/moondeck.py @@ -9,6 +9,7 @@ import sys import tempfile import threading +import time from contextlib import suppress from datetime import datetime from pathlib import Path @@ -27,6 +28,13 @@ # so once the cap is hit the writer stops and says so — truncating the end would throw away # the part you came for. LOG_MAX_BYTES = 1_000_000 +# How many times each script has been run, so a card's footer can say "run #7" — the question +# "have I actually re-run this since the fix, or am I reading a stale number?" is otherwise +# unanswerable from a report that looks identical either way. Alongside the logs and equally +# derived: deleting build/ resets the counts, which is the right blast radius for a convenience +# record. One small JSON for every script rather than a file each — it is read and rewritten +# once per run, and a dict of ints stays tiny. +RUNS_FILE = LOG_DIR / "run-counts.json" UI_DIR = SCRIPTS_DIR / "moondeck_ui" ASSETS_DIR = ROOT / "docs" / "assets" STATE_FILE = SCRIPTS_DIR / "moondeck.json" @@ -104,6 +112,43 @@ def load_scripts(): SCRIPTS = _scripts_data["scripts"] FIRMWARES = _load_firmwares() + +def _duration(seconds): + """A run's wall time, read at a glance: `4.2s`, `1m12s`, `1h04m`. + + Seconds alone stop being readable somewhere around a minute — `312s` has to be divided in + your head — and a clean rebuild here runs into the minutes. + """ + s = int(seconds) + if s < 60: + return f"{seconds:.1f}s" + if s < 3600: + return f"{s // 60}m{s % 60:02d}s" + return f"{s // 3600}h{(s % 3600) // 60:02d}m" + + +def bump_run_count(script_id): + """Record one more run of `script_id` and return the new total. + + Best-effort throughout: this is a footer on a report, so a missing or corrupt counts file + must never fail the run that produced the report. A read error starts from zero rather than + raising, and a write error is dropped — the count is the only thing lost. + """ + counts = {} + with suppress(OSError, ValueError): + loaded = json.loads(RUNS_FILE.read_text(encoding="utf-8")) + if isinstance(loaded, dict): + counts = loaded + # A hand-edited or half-written file can hold anything; a bad value restarts the count for + # that script rather than raising in the middle of reporting a successful run. + prev = counts.get(script_id, 0) + n = (prev if isinstance(prev, int) else 0) + 1 + counts[script_id] = n + with suppress(OSError): + RUNS_FILE.parent.mkdir(parents=True, exist_ok=True) + RUNS_FILE.write_text(json.dumps(counts, indent=1, sort_keys=True), encoding="utf-8") + return n + # --------------------------------------------------------------------------- # Device discovery # --------------------------------------------------------------------------- @@ -255,8 +300,6 @@ def _push_device(ip: str, model: str) -> bool: """ if not model: return True # nothing to push; not a failure - import urllib.request - import urllib.error # Look up the catalog entry. DEVICE_MODELS is loaded at module init; we don't # re-read deviceModels.json per push so a tight discover-refresh cycle # doesn't hammer the disk. If the user edits deviceModels.json, restart @@ -1800,6 +1843,10 @@ def _handle_stream(self, script_id: str): # EOF, same as the pipe's b"" sentinel. stream = getattr(proc, "_mm_read_stream", None) or proc.stdout + # Wall clock, from the first read to the child's exit. Monotonic, so an NTP step or a DST + # change mid-run cannot report a negative or wildly wrong duration. + started = time.monotonic() + # Tee to disk as we stream, rather than buffering and writing at the end: a long run # killed with Stop (or a crashed server) still leaves everything it printed up to # that point, which is exactly when you want the log. @@ -1839,7 +1886,8 @@ def _handle_stream(self, script_id: str): self.wfile.flush() proc.wait() - exit_msg = f"[exit code: {proc.returncode}]" + exit_msg = (f"[exit code: {proc.returncode}] [{_duration(time.monotonic() - started)}]" + f" [run #{bump_run_count(script_id)}]") if log: with suppress(OSError): log.write(exit_msg + "\n") # always recorded, even past the cap diff --git a/moondeck/moondeck_config.json b/moondeck/moondeck_config.json index ac85676e..736cceae 100644 --- a/moondeck/moondeck_config.json +++ b/moondeck/moondeck_config.json @@ -17,7 +17,9 @@ "speed": "slow", "help": "compile_tests", "script": "build/build_desktop.py", - "args": ["--tests"] + "args": [ + "--tests" + ] }, { "id": "test_desktop", @@ -71,6 +73,78 @@ "long_running": true, "process_name": "preview_installer.py" }, + { + "id": "check_module", + "tab": "desktop", + "group": "tools", + "label": "All Tools on Module", + "speed": "medium", + "help": "check_module", + "script": "check/check_module.py", + "needs_module": true + }, + { + "id": "check_clang_tidy", + "tab": "desktop", + "group": "tools", + "label": "clang-tidy", + "speed": "slow", + "help": "check_clang_tidy", + "script": "check/check_clang_tidy.py", + "long_running": true + }, + { + "id": "check_clang_query", + "tab": "desktop", + "group": "tools", + "label": "clang-query", + "speed": "medium", + "help": "check_clang_query", + "script": "check/check_clang_query.py" + }, + { + "id": "check_nonblocking", + "tab": "desktop", + "group": "tools", + "label": "clang-hotpath", + "speed": "slow", + "help": "check_nonblocking", + "script": "check/check_nonblocking.py" + }, + { + "id": "check_lizard", + "tab": "desktop", + "group": "tools", + "label": "Lizard Complexity", + "speed": "medium", + "help": "check_lizard", + "script": "check/check_lizard.py", + "flags": [ + { + "id": "all", + "label": "All", + "arg": "--all", + "default": false + } + ] + }, + { + "id": "check_codeql", + "tab": "desktop", + "group": "tools", + "label": "CodeQL (CI alerts)", + "speed": "instant", + "help": "check_codeql", + "script": "check/check_codeql.py", + "flags": [ + { + "id": "all", + "label": "All states", + "arg": "--all", + "default": false + } + ] + }, { "id": "check_specs", "tab": "desktop", @@ -89,6 +163,16 @@ "help": "check_platform_boundary", "script": "check/check_platform_boundary.py" }, + { + "id": "check_esp32_built", + "tab": "esp32", + "group": "build", + "label": "Firmware Up To Date", + "speed": "instant", + "help": "check_esp32_built", + "script": "check/check_esp32_built.py", + "needs_firmware": true + }, { "id": "check_devices", "tab": "desktop", @@ -134,16 +218,6 @@ "help": "history_report", "script": "report/history_report.py" }, - { - "id": "check_esp32_built", - "tab": "esp32", - "group": "build", - "label": "Firmware Up To Date", - "speed": "instant", - "help": "check_esp32_built", - "script": "check/check_esp32_built.py", - "needs_firmware": true - }, { "id": "event_precommit", "tab": "desktop", @@ -171,61 +245,6 @@ "help": "event_prerelease", "script": "event/prerelease.py" }, - { - "id": "check_module", - "tab": "desktop", - "group": "tools", - "label": "All Tools on Module", - "speed": "medium", - "help": "check_module", - "script": "check/check_module.py", - "needs_module": true - }, - { - "id": "check_clang_tidy", - "tab": "desktop", - "group": "tools", - "label": "clang-tidy", - "speed": "slow", - "help": "check_clang_tidy", - "script": "check/check_clang_tidy.py", - "long_running": true - }, - { - "id": "check_clang_query", - "tab": "desktop", - "group": "tools", - "label": "clang-query", - "speed": "medium", - "help": "check_clang_query", - "script": "check/check_clang_query.py" - }, - { - "id": "check_nonblocking", - "tab": "desktop", - "group": "tools", - "label": "clang-hotpath", - "speed": "slow", - "help": "check_nonblocking", - "script": "check/check_nonblocking.py" - }, - { - "id": "check_lizard", - "tab": "desktop", - "group": "tools", - "label": "Lizard Complexity", - "speed": "medium", - "help": "check_lizard", - "script": "check/check_lizard.py", - "flags": [ - { - "id": "all", - "label": "All", - "arg": "--all", - "default": false - } - ] - }, { "id": "install_playwright", "tab": "desktop", @@ -277,7 +296,9 @@ "speed": "slow", "help": "build_docs", "script": "docs/build_docs.py", - "args": ["--serve"], + "args": [ + "--serve" + ], "long_running": true, "process_name": "mkdocs" }, diff --git a/src/core/HttpServerModule.cpp b/src/core/HttpServerModule.cpp index b5e1371e..26e931ca 100644 --- a/src/core/HttpServerModule.cpp +++ b/src/core/HttpServerModule.cpp @@ -1499,7 +1499,7 @@ void HttpServerModule::applyWledState(const char* body) { const char* segStart = std::strstr(body, "\"seg\":"); const char* palStart = segStart ? std::strstr(segStart, "\"pal\":") : nullptr; if (palStart) { - int pal = std::atoi(palStart + 6); + int pal = mm::json::parseIntStr(palStart + 6); if (pal < 0) pal = 0; if (pal >= mm::palettes::kCount) pal = mm::palettes::kCount - 1; char valueJson[24]; diff --git a/src/core/JsonUtil.h b/src/core/JsonUtil.h index de9ad328..746be022 100644 --- a/src/core/JsonUtil.h +++ b/src/core/JsonUtil.h @@ -19,6 +19,8 @@ // Any malformed / truncated input fails cleanly (parse() returns false, accessors // return safe defaults) and never reads OOB. Off the hot path (boot load, control writes). +#include <cerrno> // ERANGE — parseIntStr's overflow signal on 32-bit `long` +#include <climits> // INT_MIN/INT_MAX — parseIntStr's range check #include <cstdint> #include <cstdio> #include <cstdlib> @@ -109,6 +111,34 @@ inline bool hasKey(const char* json, const char* key) { return std::strstr(json, search) != nullptr; } +/// The integer `s` starts with, or `fallback` when it does not start with one or the value does +/// not fit in `int`. The one string→int conversion these readers use. +/// +/// `strtol`, not `atoi`: atoi cannot report a failure, so "no digits at all" and a genuine `"0"` +/// are indistinguishable, and a value too large for `long` is undefined behavior rather than a +/// detectable error. Callers that then narrow the result (a `uint16_t` id, a `uint8_t` percent) +/// would silently store a DIFFERENT valid number — `"65537"` becoming 1. Out-of-range is reported +/// as the fallback here, which is the only place it can still be seen. +/// +/// Overflow needs BOTH checks, because they cover different targets. `long` is 64-bit on the +/// desktop, so a huge value lands inside `long` and only the INT_MAX compare rejects it; `long` is +/// 32-bit on ESP32 and Windows, where `LONG_MAX == INT_MAX` makes that compare dead code and +/// strtol's own saturation-to-LONG_MAX plus `ERANGE` is the only signal. Testing one alone passes +/// on the desktop and silently returns INT_MAX on the target this exists to protect. +/// +/// Trailing text is deliberately allowed: these values are read out of a JSON body, so digits are +/// followed by `,` or `}`. Only the LEADING characters decide. +inline int parseIntStr(const char* s, int fallback = 0) { + if (!s) return fallback; + char* end = nullptr; + errno = 0; // strtol only ever SETS it on error + const long v = std::strtol(s, &end, 10); + if (end == s) return fallback; // no digits — not a number at all + if (errno == ERANGE) return fallback; // saturated: outside `long` + if (v < INT_MIN || v > INT_MAX) return fallback; // fits `long`, would not survive `int` + return static_cast<int>(v); +} + inline int parseInt(const char* json, const char* key) { if (!json || !key) return 0; char search[kSearchLen]; @@ -119,7 +149,7 @@ inline int parseInt(const char* json, const char* key) { start = std::strstr(json, search); } if (!start) return 0; - return std::atoi(start + std::strlen(search)); + return parseIntStr(start + std::strlen(search)); } inline bool parseBool(const char* json, const char* key) { diff --git a/src/core/MqttModule.cpp b/src/core/MqttModule.cpp index 93e0c002..b590992a 100644 --- a/src/core/MqttModule.cpp +++ b/src/core/MqttModule.cpp @@ -660,7 +660,7 @@ void MqttModule::routePublish(const char* topic, const uint8_t* payload, size_t setControlValue("on", on ? "{\"value\":true}" : "{\"value\":false}"); } else if (std::strcmp(suffix, "brightness/set") == 0) { // mqttthing sends 0..100; rescale to 0..255. - int pct = std::atoi(value); + int pct = mm::json::parseIntStr(value); if (pct < 0) pct = 0; if (pct > 100) pct = 100; const int bri = (pct * 255) / 100; diff --git a/src/light/drivers/HueDriver.h b/src/light/drivers/HueDriver.h index 133bc53e..fe3111a0 100644 --- a/src/light/drivers/HueDriver.h +++ b/src/light/drivers/HueDriver.h @@ -14,13 +14,11 @@ namespace mm { /// driver reads its window of the shared buffer and pushes each light's color to the bridge. Same /// shape as NetworkSendDriver (read a window, send it out), but over the Hue v1 HTTP API not UDP. /// -/// It's HTTP, not a wire protocol (`GET /api/<key>/lights`, `PUT .../lights/<id>/state`), so the -/// rate is bounded by connection churn — each PUT opens a fresh TCP connection (the bridge speaks -/// `Connection: close`), and tick() does at most one PUT every `kPutIntervalMs` (see there) — giving -/// smooth ambient color, not real-time. The shared output Correction applies as on the LED/network -/// drivers, so the brightness slider and color-order preset reach the Hue lights too (brightness -/// 0 → light off). Only color-capable, reachable lights are driven (see `parseLights`); the `room` -/// and `light` dropdowns aim the effect at a subset (see `rebuildDriven`). +/// It's HTTP, not a wire protocol, so the rate is bounded by connection churn — each PUT opens a +/// fresh TCP connection (the bridge speaks `Connection: close`), giving smooth ambient color, not +/// real-time (see `kPutIntervalMs`). The shared output Correction applies as on the LED/network +/// drivers, so brightness and color-order reach the Hue lights too. Only color-capable, reachable +/// lights are driven (`parseLights`); the `room` / `light` dropdowns narrow that (`rebuildDriven`). /// /// **Wire contract (Hue v1 API, plain HTTP, no TLS — bench-confirmed on a BSB002 bridge, API 1.77):** /// - Pair — `POST http://<bridgeIp>/api` `{"devicetype":"projectMM#device"}`; before the link @@ -47,12 +45,12 @@ class HueDriver : public DriverBase { /// The Hue username/app key — filled by the Pair button, then persisted. char appKey[48] = {}; + /// Hue converts to HSV, RGB-fixed, so there is no correction UI to show. + bool hasCorrectionControls() const override { return false; } + /// Register the controls: bridge IP, the persisted app key, the Pair link-button, the room + /// light filter dropdowns (both default to index 0 = "All", rebuilt in place from the parsed /// bridge data on every control change), the shared window, then refresh the status line. - /// Hue converts to HSV, RGB-fixed, no correction UI. - bool hasCorrectionControls() const override { return false; } - void defineDriverControls() override { controls_.addIPv4("bridgeIp", bridgeIp); controls_.addText("appKey", appKey, sizeof(appKey)); // persisted credential @@ -134,10 +132,10 @@ class HueDriver : public DriverBase { DriverBase::release(); } - // Test seam: drive the changed-light diff + PUT formatting without a live bridge — feed a - // light's RGB and get back whether it would PUT + the body it would send. Records the push - // (like pushChangedLights does) so a follow-up call with the same RGB exercises the - // unchanged-skip path. + /// Test seam: drive the changed-light diff + PUT formatting without a live bridge — feed a + /// light's RGB and get back whether it would PUT + the body it would send. Records the push + /// (like pushChangedLights does) so a follow-up call with the same RGB exercises the + /// unchanged-skip path. bool wouldPushForTest(uint8_t idx, uint8_t r, uint8_t g, uint8_t b, char* outBody, size_t cap) { if (!diffAndFormat(idx, r, g, b, outBody, cap)) return false; if (idx < kMaxLights) { @@ -147,33 +145,41 @@ class HueDriver : public DriverBase { return true; } - // Test seam: parse a real /lights JSON body through fetchLights' color-light extractor. + /// Test seam: parse a real /lights JSON body through fetchLights' color-light extractor. void parseLightsForTest(const char* json) { parseLights(json); rebuildDriven(); } - uint8_t lightCountForTest() const { return lightCount_; } // kept color+reachable lights + /// Count of kept color+reachable lights. + uint8_t lightCountForTest() const { return lightCount_; } + /// The bridge light id at window index `i`, or 0 when out of range. uint16_t hueIdForTest(uint8_t i) const { return i < kMaxLights ? hueId_[i] : 0; } + /// The same count as the read-only control / bridge field; 0 before any fetch. int8_t colorCountForTest() const { return colorCount_; } - // Test seam: parse a real /groups JSON body through fetchGroups' Room extractor. Call - // parseLightsForTest FIRST — room membership resolves against the known color lights (hueId_), - // exactly as production order guarantees (fetchGroups runs only after fetchLights). + /// Test seam: parse a real /groups JSON body through fetchGroups' Room extractor. Call + /// parseLightsForTest FIRST — room membership resolves against the known color lights (hueId_), + /// exactly as production order guarantees (fetchGroups runs only after fetchLights). void parseGroupsForTest(const char* json) { parseGroups(json); rebuildDriven(); } - uint8_t roomCountForTest() const { return roomCount_; } // kept Rooms (type=="Room") + /// Count of kept Rooms (bridge groups with type=="Room"). + uint8_t roomCountForTest() const { return roomCount_; } - // Test seams for the room→light filtering. setRoomForTest/setLightForTest mirror what a UI - // Select change does (write the index, then re-derive the driven subset); drivenCountForTest / - // drivenIdForTest report the filtered set pushOneChangedLight walks. + /// Test seams for the room→light filtering: mirror what a UI Select change does — write the + /// index, then re-derive the driven subset. + /// Select room `r` and re-derive the driven subset, as the UI dropdown does. void setRoomForTest(uint8_t r) { room_ = r; if (light_ >= lightOptionCount_) light_ = 0; rebuildDriven(); refreshStatus(); } + /// Select light `l` within the chosen room and re-derive the driven subset. void setLightForTest(uint8_t l) { light_ = l; rebuildDriven(); refreshStatus(); } + /// Recompute the status line without waiting for a tick. void refreshStatusForTest() { refreshStatus(); } + /// How many lights survive the room+light filter — the set pushOneChangedLight walks. uint8_t drivenCountForTest() const { return drivenLightCount_; } + /// The bridge light id at filtered index `i`, or 0 when out of range. uint16_t drivenIdForTest(uint8_t i) const { return i < drivenLightCount_ ? hueId_[drivenIdx_[i]] : 0; } - // Test seam for the RGB→HSV mapping (no bridge needed). + /// Test seam for the RGB→HSV mapping (no bridge needed). static void rgbToHsvForTest(uint8_t r, uint8_t g, uint8_t b, uint16_t& h, uint8_t& s, uint8_t& v) { rgbToHsv(r, g, b, h, s, v); } - // Test seam: the truncation signal fetchLights grows against (a complete /lights body ends '}'). + /// Test seam: the truncation signal fetchLights grows against (a complete /lights body ends '}'). static bool bodyLooksCompleteForTest(const char* body) { return bodyLooksComplete(body); } private: @@ -204,66 +210,89 @@ class HueDriver : public DriverBase { static constexpr uint32_t kHttpTimeoutMs = 200; static constexpr uint32_t kSlowTimeoutMs = 400; + /// The shared layer buffer this driver reads its window from; null until setSourceBuffer. Buffer* sourceBuffer_ = nullptr; - // Per-light Hue id + the last RGB we pushed (the changed-only filter). hueId maps a window - // index → the bridge's light id, learned from GET /api/<key>/lights. + /// Window index → the bridge's light id, learned from GET /api/<key>/lights. Holds ONLY + /// color-capable lights (the bridge's "Extended color light"s) — a dimmable-only white or an + /// on/off plug is skipped, so every window pixel maps to a bulb that can show the full color. uint16_t hueId_[kMaxLights] = {}; + /// The last RGB pushed per light — what the changed-only filter compares against. uint8_t lastRgb_[kMaxLights][3] = {}; - bool sent_[kMaxLights] = {}; // have we pushed this light at least once - // hueId_ holds ONLY color-capable lights (the bridge's "Extended color light"s) — a - // dimmable-only white or an on/off plug is skipped, so every window pixel maps to a bulb - // that can show the effect's full color. lightCount_ is that filtered count. - uint8_t lightCount_ = 0; // number of color-capable lights - int8_t colorCount_ = 0; // same, as the read-only control / bridge field - bool sawLights_ = false; // fetchLights ran → the list is trustworthy - // Friendly names for the dropdowns. Heap, NOT inline: a fixed [kMaxLights][kNameLen] array - // would reserve 768 B whether the bridge has 4 lights or 32 (and cap at 32). Instead one - // contiguous block of (count × kNameLen) is allocated to the ACTUAL light/room count when the - // fetch runs, and freed in release()/release — so memory scales to the real bridge and - // sizeof(HueDriver) stays small (the lightsBuf_ stack-overflow lesson, applied to the names). - char* lightNames_ = nullptr; // kMaxLights × kNameLen; lightNameAt(i) indexes it - char* roomNames_ = nullptr; // kMaxRooms × kNameLen + /// Whether this light has been pushed at least once (the first send is never "unchanged"). + bool sent_[kMaxLights] = {}; + /// How many color-capable lights survived the filter — the length of hueId_. + uint8_t lightCount_ = 0; + /// The same count as the read-only control / bridge field. + int8_t colorCount_ = 0; + /// fetchLights has run, so the list is trustworthy. + bool sawLights_ = false; + /// Friendly light names for the dropdown — `kMaxLights × kNameLen`, indexed by lightNameAt(). + /// Heap, NOT inline: the block is allocated lazily on first parse and freed in release(), so + /// an unconfigured driver pays nothing and sizeof(HueDriver) stays small (the lightsBuf_ + /// stack-overflow lesson, applied to the names). The capacity is the kMax bound, not the live + /// count — the parser fills it incrementally and the count is not known until it finishes. + char* lightNames_ = nullptr; + /// Friendly room names, same shape — `kMaxRooms × kNameLen`, indexed by roomNameAt(). + char* roomNames_ = nullptr; + /// Pointer to light `i`'s name inside the lightNames_ block, or null before it is allocated. char* lightNameAt(uint8_t i) { return lightNames_ ? lightNames_ + static_cast<size_t>(i) * kNameLen : nullptr; } + /// Pointer to room `i`'s name inside the roomNames_ block, or null before it is allocated. char* roomNameAt(uint8_t i) { return roomNames_ ? roomNames_ + static_cast<size_t>(i) * kNameLen : nullptr; } - // Allocate the two name blocks lazily on first parse (so an unconfigured driver pays nothing), - // and free them on release / cache reset (so a removed-then-readded bridge starts clean). The - // blocks are sized to the kMax bound, not the live count, because the parser fills them - // incrementally and the count isn't known until it finishes — keeping the names off the - // resident sizeof(HueDriver) is the win (the lightsBuf_ stack-probe lesson), not per-byte fit. + /// Allocate the two name blocks lazily on first parse (so an unconfigured driver pays nothing), + /// and free them on release / cache reset (so a removed-then-readded bridge starts clean). The + /// blocks are sized to the kMax bound, not the live count, because the parser fills them + /// incrementally and the count isn't known until it finishes — keeping the names off the + /// resident sizeof(HueDriver) is the win (the lightsBuf_ stack-probe lesson), not per-byte fit. void ensureNameBuffers() { if (!lightNames_) lightNames_ = static_cast<char*>(platform::alloc(static_cast<size_t>(kMaxLights) * kNameLen)); if (!roomNames_) roomNames_ = static_cast<char*>(platform::alloc(static_cast<size_t>(kMaxRooms) * kNameLen)); } + /// Release both name blocks and null the pointers — a re-add re-fetches and re-allocates. void freeNameBuffers() { platform::free(lightNames_); lightNames_ = nullptr; platform::free(roomNames_); roomNames_ = nullptr; } - // --- Rooms (GET /api/<key>/groups, type=="Room"): name + a color-light membership bitmask. - uint32_t roomMask_[kMaxRooms] = {}; // bit i set ⇔ this Room references color light hueId_[i] - uint8_t roomCount_ = 0; // number of Rooms kept - bool sawGroups_ = false; // fetchGroups ran → the room list is trustworthy - - // --- Filter selection (Select indices, persisted as uint8) and the derived driven subset. - uint8_t room_ = 0; // 0 = "All", else roomName_[room_-1] - uint8_t light_ = 0; // 0 = "All", else the n-th light of the current option list - uint8_t drivenIdx_[kMaxLights] = {}; // color-light array-indices actually driven (after filter) - uint8_t drivenLightCount_ = 0; // size of drivenIdx_ — what pushOneChangedLight walks - - // --- Stable option pointer arrays for the two Selects. addSelect borrows the pointer; these - // live for the driver's lifetime and are refilled in place (pointing into the *Name_ buffers) - // by buildRoomOptions / buildLightOptions on every defineControls. "All" is always index 0. + /// Rooms from GET /api/<key>/groups (type=="Room"): bit i set ⇔ this Room references color + /// light hueId_[i]. + uint32_t roomMask_[kMaxRooms] = {}; + /// Number of Rooms kept. + uint8_t roomCount_ = 0; + /// fetchGroups has run, so the room list is trustworthy. + bool sawGroups_ = false; + + /// Room filter (a Select index, persisted as uint8): 0 = "All", else roomName_[room_-1]. + uint8_t room_ = 0; + /// Light filter: 0 = "All", else the n-th light of the current option list. + uint8_t light_ = 0; + /// Color-light array indices actually driven, after the room+light filter. + uint8_t drivenIdx_[kMaxLights] = {}; + /// Size of drivenIdx_ — the set pushOneChangedLight walks. + uint8_t drivenLightCount_ = 0; + + /// Stable option pointers for the room Select. addSelect BORROWS the pointer, so these live + /// for the driver's lifetime and are refilled in place (pointing into the *Names_ buffers) by + /// buildRoomOptions on every defineControls. "All" is always index 0. const char* roomOptions_[kMaxRooms + 1] = {}; + /// How many entries of roomOptions_ are live. uint8_t roomOptionCount_ = 1; + /// The same borrowed-pointer arrangement for the light Select, refilled by buildLightOptions. const char* lightOptions_[kMaxLights + 1] = {}; + /// How many entries of lightOptions_ are live. uint8_t lightOptionCount_ = 1; - uint8_t pushCursor_ = 0; // round-robin position across the lights - uint8_t drivenCount_ = 0; // lights driven this pass (n); fade-time basis - uint32_t lastPutMs_ = 0; // millis() of the last PUT — the tick() rate gate + /// Round-robin position across the lights — one bounded PUT per tick, not a whole sweep. + uint8_t pushCursor_ = 0; + /// Lights driven this pass (n), the basis for the fade time. + uint8_t drivenCount_ = 0; + /// millis() of the last PUT — the rate gate tick() checks before doing any work. + uint32_t lastPutMs_ = 0; + /// Remaining 1 Hz ticks to keep polling for the link-button press; 0 = not pairing. int pairTicksLeft_ = 0; - uint16_t reportTick_ = 0; // counts tick1s ticks toward kReportEverySec + /// Counts tick1s ticks toward kReportEverySec, for the periodic DevicesModule announce. + uint16_t reportTick_ = 0; + /// The status line shown on the driver's card; refreshStatus() rewrites it in place. char statusBuf_[40] = "unpaired"; // /lights read buffer is sized dynamically in fetchLights (grow-and-retry): a small first try // covers a typical home; it doubles up to the cap only when a bigger bridge's response fills it. @@ -272,10 +301,11 @@ class HueDriver : public DriverBase { static constexpr size_t kLightsBufInitial = 2048; static constexpr size_t kLightsBufMax = 16384; + /// True once a bridge IP has been set — any non-zero octet counts. bool haveBridge() const { return bridgeIp[0] || bridgeIp[1] || bridgeIp[2] || bridgeIp[3]; } - // Does the JSON span [begin, end) contain `key` (e.g. "\"hue\"") — used to read a light's - // capabilities off its state block (a color light has "hue"; the bridge omits it otherwise). + /// Does the JSON span [begin, end) contain `key` (e.g. "\"hue\"") — used to read a light's + /// capabilities off its state block (a color light has "hue"; the bridge omits it otherwise). static bool containsKey(const char* begin, const char* end, const char* key) { const size_t kl = std::strlen(key); for (const char* s = begin; s + kl <= end; s++) @@ -283,11 +313,11 @@ class HueDriver : public DriverBase { return false; } - // Read a `"<key>":"<value>"` string from WITHIN a JSON object span [begin, end) — the - // span-bounded analogue of containsKey, used to grab one light's / one room's "name" without - // matching the first "name" elsewhere in the bridge's big response. Copies the raw value up to - // its closing quote (the bridge's names carry no escapes worth decoding) into out[cap], NUL- - // terminated; leaves out empty if the key isn't in the span. + /// Read a `"<key>":"<value>"` string from WITHIN a JSON object span [begin, end) — the + /// span-bounded analogue of containsKey, used to grab one light's / one room's "name" without + /// matching the first "name" elsewhere in the bridge's big response. Copies the raw value up to + /// its closing quote (the bridge's names carry no escapes worth decoding) into out[cap], NUL- + /// terminated; leaves out empty if the key isn't in the span. static void parseStringIn(const char* begin, const char* end, const char* key, char* out, size_t cap) { if (cap == 0) return; out[0] = 0; @@ -304,14 +334,15 @@ class HueDriver : public DriverBase { } } + /// Format bridgeIp as a dotted quad into `out`, for the HTTP host argument. void bridgeStr(char out[16]) const { std::snprintf(out, 16, "%u.%u.%u.%u", bridgeIp[0], bridgeIp[1], bridgeIp[2], bridgeIp[3]); } - // The single status line, folding what were three separate controls (status / hueStatus / - // colorLights). Shows the pairing state and the light count as driven-of-total: "paired, - // 3-4 lights" = the room/light filter narrowed 4 color lights to 3 driven. When nothing is - // filtered (driven == total) it collapses to the plain count, "paired, 4 lights". + /// Rebuild the single status line, folding what were three separate controls (status / + /// hueStatus / colorLights). Shows the pairing state and the light count as driven-of-total: + /// "paired, 3-4 lights" = the room/light filter narrowed 4 color lights to 3 driven. When + /// nothing is filtered (driven == total) it collapses to the plain count, "paired, 4 lights". void refreshStatus() { if (!appKey[0]) std::snprintf(statusBuf_, sizeof(statusBuf_), "unpaired"); else if (!lightCount_) std::snprintf(statusBuf_, sizeof(statusBuf_), "paired"); @@ -321,7 +352,8 @@ class HueDriver : public DriverBase { setStatus(statusBuf_); } - // --- Pairing: POST /api {"devicetype":"projectMM#<name>"} until the user presses the button. + /// One pairing attempt: POST /api {"devicetype":"projectMM#<name>"} and, if the user has + /// pressed the bridge's link button, keep the app key it returns. void pollPairing() { if (!haveBridge()) { pairTicksLeft_ = 0; std::snprintf(statusBuf_, sizeof(statusBuf_), "set bridge IP first"); setStatus(statusBuf_); return; } char host[16]; bridgeStr(host); @@ -353,7 +385,7 @@ class HueDriver : public DriverBase { } } - // Drop the learned light list + room list + push cache so tick1s re-fetches (bridge/key change). + /// Drop the learned light list + room list + push cache so tick1s re-fetches (bridge/key change). void resetLightCache() { lightCount_ = 0; colorCount_ = 0; @@ -371,10 +403,10 @@ class HueDriver : public DriverBase { rebuildDriven(); // empty caches → empty driven set, until the re-fetch repopulates them } - // A complete /lights response is a JSON object: its last non-whitespace char is '}'. A read cut - // short by a too-small buffer ends mid-content, so this is the truncation signal fetchLights - // grows against. (Not a full JSON validator — the bridge's well-formed body is the contract; - // this only distinguishes "whole" from "cut off".) + /// A complete /lights response is a JSON object: its last non-whitespace char is '}'. A read cut + /// short by a too-small buffer ends mid-content, so this is the truncation signal fetchLights + /// grows against. (Not a full JSON validator — the bridge's well-formed body is the contract; + /// this only distinguishes "whole" from "cut off".) static bool bodyLooksComplete(const char* body) { size_t len = std::strlen(body); while (len > 0 && (body[len - 1] == '\n' || body[len - 1] == '\r' @@ -382,7 +414,16 @@ class HueDriver : public DriverBase { return len > 0 && body[len - 1] == '}'; } - // --- Learn the bridge's light ids (window index → hue id, in id order). + /// The bridge id a quoted JSON key opens with (`"7":{…}` → 7), or 0 when `s` does not start + /// with a positive integer that fits `hueId_` — the value every caller already treats as + /// "not an id". The range bound is the point: an id too large would otherwise narrow into a + /// DIFFERENT valid light (`"65537"` → light 1). + static uint16_t parseId(const char* s) { + const int v = json::parseIntStr(s); + return (v > 0 && v <= 0xFFFF) ? static_cast<uint16_t>(v) : 0; + } + + /// --- Learn the bridge's light ids (window index → hue id, in id order). void fetchLights() { char host[16]; bridgeStr(host); char path[80]; std::snprintf(path, sizeof(path), "/api/%s/lights", appKey); @@ -417,10 +458,10 @@ class HueDriver : public DriverBase { } } - // List the bridge in DevicesModule (so it shows alongside discovered WLED/projectMM peers, - // carrying its dimmable-light count for layout sizing). The bridge isn't a UDP-presence - // device, so it's registered explicitly through the static seam — no compile-time core↔light - // dependency beyond the same DevicesModule::active() shape AudioService::latestFrame() uses. + /// List the bridge in DevicesModule (so it shows alongside discovered WLED/projectMM peers, + /// carrying its dimmable-light count for layout sizing). The bridge isn't a UDP-presence + /// device, so it's registered explicitly through the static seam — no compile-time core↔light + /// dependency beyond the same DevicesModule::active() shape AudioService::latestFrame() uses. void reportBridge() { auto* dev = DevicesModule::active(); if (!dev || !haveBridge()) return; @@ -434,14 +475,11 @@ class HueDriver : public DriverBase { dev->upsertHueBridge(bridgeIp, name, static_cast<uint8_t>(colorCount_)); } - // Extract the COLOR-capable, REACHABLE light ids from a /lights JSON body: - // {"1":{…},"5":{…},…}. A color light's object carries a "hue" field in its state; a - // dimmable-only white or an on/off plug does not. A light that's powered off / out of mesh - // reports "reachable":false. We keep only lights that are BOTH color-capable and reachable - // — those are the ones an effect can actually animate right now — so the window maps every - // pixel to a live color bulb. The bridge response (~8 KB / hundreds of fields) exceeds the - // recursive JSON reader's node arena, so this is a lightweight forward scan: spot each - // top-level id key, then keep it iff its object span (up to the next id key) has both. + /// Extract the COLOR-capable, REACHABLE light ids from a /lights body ({"1":{…},"5":{…},…}), + /// so the window maps every pixel to a bulb an effect can animate right now. Color lights + /// carry a "hue" field; a dimmable white or on/off plug does not, and an unpowered light + /// reports "reachable":false. A forward scan, not the recursive JSON reader — the ~8 KB + /// response exceeds its node arena. void parseLights(const char* resp) { ensureNameBuffers(); lightCount_ = 0; @@ -464,7 +502,7 @@ class HueDriver : public DriverBase { while (true) { const char* q = std::strchr(p, '"'); // next key open-quote if (!q) break; - int id = std::atoi(q + 1); // light id is a quoted integer key + int id = parseId(q + 1); // light id is a quoted integer key const char* close = std::strchr(q + 1, '"'); // A top-level light-id key: a quoted positive integer followed by ':'. if (id > 0 && close && close[1] == ':') { @@ -480,10 +518,10 @@ class HueDriver : public DriverBase { rebuildDriven(); // the color-light set changed → re-derive the filtered driven subset } - // --- Learn the bridge's Rooms (GET /api/<key>/groups). Same dynamic grow-and-retry read as - // fetchLights — the /groups body grows with the room+zone count, so size the heap buffer up - // until the response parses whole. fetchGroups runs at 1 Hz, off the render loop, after - // fetchLights (gated by sawGroups_), so this alloc/refetch is never hot-path. + /// --- Learn the bridge's Rooms (GET /api/<key>/groups). Same dynamic grow-and-retry read as + /// fetchLights — the /groups body grows with the room+zone count, so size the heap buffer up + /// until the response parses whole. fetchGroups runs at 1 Hz, off the render loop, after + /// fetchLights (gated by sawGroups_), so this alloc/refetch is never hot-path. void fetchGroups() { char host[16]; bridgeStr(host); char path[80]; std::snprintf(path, sizeof(path), "/api/%s/groups", appKey); @@ -503,11 +541,11 @@ class HueDriver : public DriverBase { } } - // Extract the Rooms from a /groups JSON body: {"1":{"name":"Living","lights":["3","5"], - // "type":"Room",…},…}. Keep only type=="Room" (drop Zones, LightGroups, Entertainment); for - // each, store its name and the light ids its "lights" array references. Same lightweight - // forward scan as parseLights (the response exceeds the recursive reader's node arena): spot - // each top-level id key, then read the object span up to the next id key. + /// Extract the Rooms from a /groups JSON body: {"1":{"name":"Living","lights":["3","5"], + /// "type":"Room",…},…}. Keep only type=="Room" (drop Zones, LightGroups, Entertainment); for + /// each, store its name and the light ids its "lights" array references. Same lightweight + /// forward scan as parseLights (the response exceeds the recursive reader's node arena): spot + /// each top-level id key, then read the object span up to the next id key. void parseGroups(const char* resp) { ensureNameBuffers(); roomCount_ = 0; @@ -527,7 +565,7 @@ class HueDriver : public DriverBase { while (true) { const char* q = std::strchr(p, '"'); if (!q) break; - int id = std::atoi(q + 1); + int id = parseId(q + 1); const char* close = std::strchr(q + 1, '"'); if (id > 0 && close && close[1] == ':') { commit(q); // the PREVIOUS group's object ends here @@ -540,11 +578,11 @@ class HueDriver : public DriverBase { sawGroups_ = true; } - // Resolve a Room's "lights":["3","5",…] array (within [begin, end)) to a color-light - // membership bitmask: for each listed bridge id, set bit i if it equals a kept color light - // hueId_[i]. Ids the Room lists that aren't color-capable (a white bulb, a plug) simply don't - // match and are dropped. Scans from the "lights" key to the array's ']' so a later array - // (e.g. a Zone's "lights" in a wider scan) can't bleed in. + /// Resolve a Room's "lights":["3","5",…] array (within [begin, end)) to a color-light + /// membership bitmask: for each listed bridge id, set bit i if it equals a kept color light + /// hueId_[i]. Ids the Room lists that aren't color-capable (a white bulb, a plug) simply don't + /// match and are dropped. Scans from the "lights" key to the array's ']' so a later array + /// (e.g. a Zone's "lights" in a wider scan) can't bleed in. uint32_t roomMaskFor(const char* begin, const char* end) const { const char* s = begin; const size_t kl = std::strlen("\"lights\":["); @@ -552,7 +590,7 @@ class HueDriver : public DriverBase { uint32_t mask = 0; for (const char* q = s; q < end && *q != ']'; ) { if (*q == '"') { - const int id = std::atoi(q + 1); + const int id = parseId(q + 1); for (uint8_t i = 0; i < lightCount_; i++) // map the id to its color-light bit if (hueId_[i] == id) { mask |= (1u << i); break; } const char* c = std::strchr(q + 1, '"'); // skip to the value's closing quote @@ -563,16 +601,13 @@ class HueDriver : public DriverBase { return mask; } - // The color-light array-indices (into hueId_ / lightName_) that the CURRENT room selection - // exposes: room_==0 ("All") → every color light, in order; else only the color lights whose - // id appears in that Room's member list. Writes up to kMaxLights indices into `out`, returns - // the count. The single source of truth both the light-dropdown options and the driven set - // derive from, so the dropdown and the driven subset can never disagree. - // `out` is a reference-to-array, not a bare pointer: the bound then lives in the TYPE, so the - // compiler checks it (a wrong-sized caller is a compile error) instead of trusting the comment. - // With a bare uint8_t* the callee cannot see the caller's size at all, and GCC must assume the - // worst — it warned that these writes could run past the end (-Wstringop-overflow). lightCount_ - // is itself capped at kMaxLights when the lights are parsed, so n never exceeds the array. + /// The color-light indices (into hueId_ / lightNames_) the CURRENT room selection exposes: + /// room_==0 ("All") → every color light in order, else only those in that Room's member list. + /// Writes up to kMaxLights indices into `out` and returns the count. The single source of + /// truth for both the light-dropdown options and the driven set, so the two cannot disagree. + /// + /// `out` is a reference-to-array so the bound lives in the TYPE and the compiler checks it — + /// with a bare `uint8_t*` GCC cannot see the caller's size and warns (-Wstringop-overflow). uint8_t roomColorLights(uint8_t (&out)[kMaxLights]) const { uint8_t n = 0; if (room_ == 0 || room_ > roomCount_) { // "All" (or a stale index) → every color light @@ -585,7 +620,7 @@ class HueDriver : public DriverBase { return n; } - // Rebuild the room dropdown options: {"All", room0, room1, …}, pointing into roomName_. + /// Rebuild the room dropdown options: {"All", room0, room1, …}, pointing into roomName_. void buildRoomOptions() { roomOptions_[0] = "All"; uint8_t n = 1; @@ -593,9 +628,9 @@ class HueDriver : public DriverBase { roomOptionCount_ = n; } - // Rebuild the light dropdown options: {"All", <names of the current room's color lights>}, - // pointing into lightName_. The option count tracks the current room, so the light index - // selects within that narrowed list (index 0 = "All", index k = the k-th listed light). + /// Rebuild the light dropdown options: {"All", <names of the current room's color lights>}, + /// pointing into lightName_. The option count tracks the current room, so the light index + /// selects within that narrowed list (index 0 = "All", index k = the k-th listed light). void buildLightOptions() { lightOptions_[0] = "All"; uint8_t idx[kMaxLights]; @@ -605,10 +640,10 @@ class HueDriver : public DriverBase { lightOptionCount_ = n; } - // Derive drivenIdx_ from the current room+light filter — the subset pushOneChangedLight walks. - // room=All & light=All → every color light (the original behaviour, unchanged). - // room=X → that room's color lights. - // light=Y → just that one light (the Y-th of the current room's list). + /// Derive drivenIdx_ from the current room+light filter — the subset pushOneChangedLight walks. + /// room=All & light=All → every color light (the original behaviour, unchanged). + /// room=X → that room's color lights. + /// light=Y → just that one light (the Y-th of the current room's list). void rebuildDriven() { drivenLightCount_ = 0; uint8_t idx[kMaxLights]; @@ -621,10 +656,10 @@ class HueDriver : public DriverBase { if (pushCursor_ >= drivenLightCount_) pushCursor_ = 0; } - // Push AT MOST ONE changed light per call (the tick() gate already limited the rate). The - // round-robin cursor walks every light over successive calls, so each gets its turn; we - // advance the cursor whether or not this light changed, scanning at most one full lap so an - // all-unchanged frame costs no PUT and returns fast (no blocking I/O on the render loop). + /// Push AT MOST ONE changed light per call (the tick() gate already limited the rate). The + /// round-robin cursor walks every light over successive calls, so each gets its turn; we + /// advance the cursor whether or not this light changed, scanning at most one full lap so an + /// all-unchanged frame costs no PUT and returns fast (no blocking I/O on the render loop). void pushOneChangedLight() { if (!sourceBuffer_ || !sourceBuffer_->data()) return; nrOfLightsType winStart, winLen; @@ -666,17 +701,12 @@ class HueDriver : public DriverBase { // No light changed this lap — nothing to send. Cursor stays put. } - // The changed-only diff + the Hue state body. Returns true (and fills `out`) when light - // `idx`'s RGB differs from the last push (or was never sent). Every driven light is color- - // capable (parseLights keeps only those), so the body carries the full color: on/off, plus - // bri (value) + hue + sat from a textbook RGB→HSV — so a color effect actually animates. - // "transitiontime" is the bridge's built-in fade — the smoothing knob. Set to roughly the - // per-light update interval (a light updates every kPutIntervalMs × lightCount), so the bulb - // glides from its current color to the next instead of snapping. The bridge's default is - // 400 ms (too long for our cadence — it smears and looks frozen); we compute a value matched - // to the actual rate so transitions are smooth but keep up. transitiontime is in deciseconds - // (×100 ms). The Hue standard API tops out ~10 cmd/s — true real-time needs the Entertainment - // API; this is smooth ambient color, the standard API's sweet spot. + /// The changed-only diff + the Hue state body. Returns true (and fills `out`) when light + /// `idx`'s RGB differs from the last push (or was never sent). Every driven light is + /// color-capable (parseLights keeps only those), so the body carries on/off plus bri + hue + + /// sat from a textbook RGB→HSV. `transitiontime` is the bridge's fade: its 400 ms default is + /// too long for our cadence — it smears and looks frozen — so transitionDeciseconds() sizes it + /// to the actual refresh rate. bool diffAndFormat(uint8_t idx, uint8_t r, uint8_t g, uint8_t b, char* out, size_t cap) { if (idx >= kMaxLights) return false; if (sent_[idx] && lastRgb_[idx][0] == r && lastRgb_[idx][1] == g && lastRgb_[idx][2] == b) @@ -690,10 +720,10 @@ class HueDriver : public DriverBase { return true; } - // Fade time matched to how often THIS light is refreshed: with n lights round-robined one - // per kPutIntervalMs, each light's turn comes every (n × kPutIntervalMs) ms. Convert to - // deciseconds and clamp to ≥1 (0 = snap) so the fade lasts about until the next update — - // continuous glide, no visible steps. + /// Fade time matched to how often THIS light is refreshed: with n lights round-robined one + /// per kPutIntervalMs, each light's turn comes every (n × kPutIntervalMs) ms. Convert to + /// deciseconds and clamp to ≥1 (0 = snap) so the fade lasts about until the next update — + /// continuous glide, no visible steps. uint8_t transitionDeciseconds() const { // Use the count actually driven this pass (n = min(lightCount_, window)), not the full // discovered lightCount_ — a partial window refreshes each of its lights sooner, so a @@ -704,8 +734,8 @@ class HueDriver : public DriverBase { return static_cast<uint8_t>(ds < 1 ? 1 : (ds > 30 ? 30 : ds)); } - // Textbook RGB→HSV mapped to Hue's ranges: hue 0..65535 (Hue's 16-bit wheel), sat 0..254, - // val(=bri) 0..254. Integer math, no float — the standard max/min/chroma formulation. + /// Textbook RGB→HSV mapped to Hue's ranges: hue 0..65535 (Hue's 16-bit wheel), sat 0..254, + /// val(=bri) 0..254. Integer math, no float — the standard max/min/chroma formulation. static void rgbToHsv(uint8_t r, uint8_t g, uint8_t b, uint16_t& hueOut, uint8_t& satOut, uint8_t& valOut) { const uint8_t mx = r > g ? (r > b ? r : b) : (g > b ? g : b); const uint8_t mn = r < g ? (r < b ? r : b) : (g < b ? g : b); diff --git a/src/light/drivers/MoonLedDriver.h b/src/light/drivers/MoonLedDriver.h index 1ff8a2dd..40395e58 100644 --- a/src/light/drivers/MoonLedDriver.h +++ b/src/light/drivers/MoonLedDriver.h @@ -274,7 +274,7 @@ class MoonI80Peripheral : public LedPeripheral { const char* initFailMsg() const override { return "MoonI80 bus init failed — check pins / memory"; } /// The expander needs a backend that can stream its ×8 frame; LCD_CAM is it, and this backend is /// LCD_CAM-only, so the answer is simply "wherever this backend runs at all". - bool supportsPinExpander() const override { return platform::lcdLanes > 0; } + bool supportsPinExpander() const override { return platform::hasLcdCam; } /// No async double-buffer on this backend — the own-GDMA two-buffer completion handshake races and /// wedges the bus (see busInit). Single-buffer only; the ring is where MoonI80's speed lives. diff --git a/src/light/drivers/MultiPinLedDriver.h b/src/light/drivers/MultiPinLedDriver.h index 58a61ee7..c86083c5 100644 --- a/src/light/drivers/MultiPinLedDriver.h +++ b/src/light/drivers/MultiPinLedDriver.h @@ -210,7 +210,7 @@ class I80Peripheral : public LedPeripheral { /// the flag on `lcdLanes` (non-zero only on the LCD_CAM chips, S3/P4/S31) makes the refusal a /// compile-time property of the silicon rather than a runtime surprise, and the orchestrator then /// reports it as a config error instead of letting the bus die at init with "check pins / memory". - bool supportsPinExpander() const override { return platform::lcdLanes > 0; } + bool supportsPinExpander() const override { return platform::hasLcdCam; } /// The bus pin list comes from the orchestrator: in shift mode it appends the latch to the data pins /// (the latch is a bus lane), so the peripheral drives it. busClockMultiplier() tells the platform diff --git a/src/light/drivers/NetworkSendDriver.h b/src/light/drivers/NetworkSendDriver.h index ade6cd6d..a7363ee0 100644 --- a/src/light/drivers/NetworkSendDriver.h +++ b/src/light/drivers/NetworkSendDriver.h @@ -17,12 +17,25 @@ namespace mm { /// studied, not copied). Byte layouts live in ArtNetPacket.h / E131Packet.h / DdpPacket.h, shared /// with the receiver so the two sides cannot drift. /// -/// **Addressing: unicast to a LIST of receivers, and why that is the default.** +/// **Addressing:** one driver feeds N receivers (`ips`), each taking a contiguous run of the window +/// (`lightsPerIp`, the same broadcasting idiom as an LED driver's `ledsPerPin`) and each addressed +/// only by itself. So a wall of tubes is one driver, not N — the driver-level twin of one LED driver +/// fanning out to N GPIO lanes. Unicast is the default, not an option among equals; broadcast +/// survives only as legacy compatibility. /// -/// One driver feeds N receivers (`ips`), each taking a contiguous run of the window (`lightsPerIp`, -/// the same broadcasting idiom as an LED driver's `ledsPerPin`) and each addressed only by itself. -/// So a wall of tubes is one driver, not N — the driver-level twin of one LED driver fanning out to -/// N GPIO lanes. +/// **Synchronous send:** the whole frame goes out inline in tick() (~35 ms Ethernet / ~90 ms WiFi at +/// 128×128 ArtNet; DDP less). A decoupling send task is a PSRAM-gated backlog item. Added per board +/// via the catalog like the LED drivers; applies the same shared Correction, so network and wired +/// outputs show identical colors. +/// +/// The deep dives are under *More info*, below the attribute/method lists: +/// @xref{why-unicast-is-the-default|why unicast is the default}, +/// @xref{liveness-and-what-is-not-here|liveness, multicast and E1.31 framing}. +/// @card NetworkSendDriver.png +/// +/// @moreinfo +/// +/// ## Why unicast is the default /// /// The Art-Net 4 spec leaves no room here: *"ArtDmx packets must be unicast to subscribers of the /// specific universe contained in the ArtDmx packet… There are no conditions in which broadcast is @@ -50,20 +63,16 @@ namespace mm { /// costs the sender the same and costs every other receiver nothing. Mirroring is the one case where /// a broadcast address is genuinely the better tool. /// -/// **Liveness.** UDP is fire-and-forget, so a dead receiver is invisible to the sender: the send -/// loop simply tolerates it (a failed send drops that packet and moves on, so one dark tube cannot -/// stall the others) rather than pretending to detect it. Real detection is ArtPoll/ArtPollReply -/// discovery — the spec's own mechanism, and the next increment (backlog). +/// ## Liveness, and what is not here +/// +/// UDP is fire-and-forget, so a dead receiver is invisible to the sender: the send loop simply +/// tolerates it (a failed send drops that packet and moves on, so one dark tube cannot stall the +/// others) rather than pretending to detect it. Real detection is ArtPoll/ArtPollReply discovery — +/// the spec's own mechanism, and the next increment (backlog). /// /// NOT multicast (no IGMP join yet — backlogged; it is sACN's native mode and the best answer on a /// switch with IGMP snooping, but degrades to a flood without it). E1.31 framing: CID stable per /// device (from the MAC), source name `projectMM`, priority 100, one frame-level sequence per frame. -/// -/// **Synchronous send:** the whole frame goes out inline in tick() (~35 ms Ethernet / ~90 ms WiFi at -/// 128×128 ArtNet; DDP less). A decoupling send task is a PSRAM-gated backlog item. Added per board -/// via the catalog like the LED drivers; applies the same shared Correction, so network and wired -/// outputs show identical colors. -/// @card NetworkSendDriver.png class NetworkSendDriver : public DriverBase { public: /// DMX-over-network fixtures (ArtNet / E1.31 / DDP) are RGB by convention — the @@ -82,6 +91,9 @@ class NetworkSendDriver : public DriverBase { /// destinations, so the driver idles (never falls back to broadcasting — see the class note). /// Capped at a wall of tubes, not a subnet scan. static constexpr uint8_t kMaxDestinations = 32; + /// Receiver addresses, comma-separated (`192.168.1.10,192.168.1.11`). Each takes a contiguous + /// run of the window per `lightsPerIp`, so the list order is the fan-out order. A broadcast + /// address works but is not the default — see the unicast rationale above. char ips[64] = {}; /// Lights per destination — the same idiom as an LED driver's `ledsPerPin`, and literally the /// same helper (`assignCounts`): blank = even split of the window, one number = that many each, @@ -328,9 +340,9 @@ class NetworkSendDriver : public DriverBase { // light/ArtNetPacket.h, light/E131Packet.h and light/DdpPacket.h, shared // with NetworkReceiveEffect — each wire format exists in exactly one place. - // Test-only accessor for the correction-applied buffer. Lets the unit - // tests pin the no-allocation-in-loop contract (size set in prepare - // / onCorrectionChanged, never in loop). Not part of any runtime API. + /// Test-only accessor for the correction-applied buffer, letting the unit tests pin the + /// no-allocation-in-loop contract (sized in prepare / onCorrectionChanged, never in the send + /// loop). Not part of any runtime API. const Buffer& correctedBuffer() const { return corrected_; } /// The destination table prepare() derived: how many receivers, each one's address, and how many @@ -342,24 +354,34 @@ class NetworkSendDriver : public DriverBase { nrOfLightsType lightsAt(uint8_t i) const { return destCounts_[i]; } private: + /// The send socket, opened once in setup() and reused for every destination. platform::UdpSocket socket_; + /// The shared frame this driver reads its window from; borrowed, not owned. Buffer* sourceBuffer_ = nullptr; - Buffer corrected_; // owned: source bytes after brightness/order/white + /// Owned: source bytes after brightness/order/white. Sized off the hot path (resizeCorrected). + Buffer corrected_; + /// Art-Net/E1.31 per-frame sequence counter; wraps at 255, which both protocols expect. uint8_t sequence_ = 0; + /// millis() of the last frame sent — the `fps` rate limiter's reference point. uint32_t lastSendTime_ = 0; - uint8_t cid_[E131_CID_LENGTH] = {}; // E1.31 component id, built once in setup() - // Destinations + each one's slice of the window, both derived in prepare() (never in tick()). + /// E1.31 component id, built once in setup() from the MAC so it is stable per device. + uint8_t cid_[E131_CID_LENGTH] = {}; + /// Destination addresses, derived in prepare() (never in tick()). uint8_t dest_[kMaxDestinations][4] = {}; + /// Each destination's slice of the window, index-aligned with `dest_`. nrOfLightsType destCounts_[kMaxDestinations] = {}; + /// How many entries of `dest_` / `destCounts_` are live. uint8_t nDest_ = 0; + /// The UDP port for a protocol index — each wire format has its own registered port. static uint16_t protocolPort(uint8_t p) { return p == 1 ? E131_PORT : p == 2 ? DDP_PORT : ARTNET_PORT; } - // Called off the hot path (prepare, onCorrectionChanged, setSourceBuffer) to make - // sure corrected_ is sized for the current source + this driver's correction. Skips - // when no source is wired yet, or when the existing allocation already fits. + /// Size `corrected_` for the current source and this driver's correction. Called only off the + /// hot path (prepare, onCorrectionChanged, setSourceBuffer) — that placement IS the + /// no-allocation-in-the-send-loop contract. Skips when no source is wired yet, or when the + /// existing allocation already fits. void resizeCorrected() { if (!sourceBuffer_) return; // Size for the window slice this sender actually transmits, not the whole diff --git a/src/main.cpp b/src/main.cpp index 4fad5140..54b8ada2 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -87,19 +87,22 @@ // (ESP_PLATFORM / CONFIG_IDF_TARGET_* / __APPLE__ / …) — check_platform_boundary.py // passes them, by design. The driver bodies themselves keep all hardware behind // the platform seam; this gate only decides which driver headers are present. -#if defined(CONFIG_SOC_RMT_SUPPORTED) +// `|| MM_LINKS_ALL_LED_DRIVERS`: the desktop build links every driver — the rule and its reasons +// live in architecture.md § Platform abstraction. On a real chip the CONFIG_SOC_* gate is +// unchanged, so no board links a driver its silicon cannot run. +#if defined(CONFIG_SOC_RMT_SUPPORTED) || MM_LINKS_ALL_LED_DRIVERS #include "light/drivers/RmtLedDriver.h" #endif // The parallel-WS2812 driver + its peripheral backends. Each backend header self-registers its factory // into ParallelLedDriver's peripheral registry (gated by the chip's CONFIG_SOC_*), so including the ones // this silicon supports is what populates the `peripheral` control's options. -#if defined(CONFIG_SOC_LCD_I80_SUPPORTED) +#if defined(CONFIG_SOC_LCD_I80_SUPPORTED) || MM_LINKS_ALL_LED_DRIVERS #include "light/drivers/MultiPinLedDriver.h" // esp_lcd i80 backend (I80Peripheral) #endif -#if defined(CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED) +#if defined(CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED) || MM_LINKS_ALL_LED_DRIVERS #include "light/drivers/MoonLedDriver.h" // MoonI80 own-GDMA backend (MoonI80Peripheral) #endif -#if defined(CONFIG_SOC_PARLIO_SUPPORTED) +#if defined(CONFIG_SOC_PARLIO_SUPPORTED) || MM_LINKS_ALL_LED_DRIVERS #include "light/drivers/ParlioLedDriver.h" // Parlio backend (ParlioPeripheral) #endif #include "core/HttpServerModule.h" @@ -220,7 +223,7 @@ static void registerModuleTypes() { // Register only the LED drivers this chip's silicon can run (see the gated // includes above) — keeps the type picker honest (no MultiPinLedDriver offered on a // chip without an i80 bus) and the binary lean. -#if defined(CONFIG_SOC_RMT_SUPPORTED) +#if defined(CONFIG_SOC_RMT_SUPPORTED) || MM_LINKS_ALL_LED_DRIVERS mm::ModuleFactory::registerType<mm::RmtLedDriver>("RmtLedDriver", "light/drivers.md#rmtled"); #endif // ParallelLedDriver — ONE driver for the parallel-WS2812 output, whatever the DMA peripheral. The @@ -228,7 +231,7 @@ static void registerModuleTypes() { // peripheral registry when their header is included above (gated by the same CONFIG_SOC_* below), so // the `peripheral` control offers exactly the ones this chip links. Registered once, on any chip that // links at least one parallel backend. -#if defined(CONFIG_SOC_LCD_I80_SUPPORTED) || defined(CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED) || defined(CONFIG_SOC_PARLIO_SUPPORTED) +#if defined(CONFIG_SOC_LCD_I80_SUPPORTED) || defined(CONFIG_SOC_LCDCAM_I80_LCD_SUPPORTED) || defined(CONFIG_SOC_PARLIO_SUPPORTED) || MM_LINKS_ALL_LED_DRIVERS mm::ModuleFactory::registerType<mm::ParallelLedDriver>("ParallelLedDriver", "light/drivers.md#parallelled"); #endif mm::ModuleFactory::registerType<mm::HttpServerModule>("HttpServerModule", "core/system.md"); diff --git a/src/platform/desktop/platform_config.h b/src/platform/desktop/platform_config.h index e15b7eca..8e02cc3a 100644 --- a/src/platform/desktop/platform_config.h +++ b/src/platform/desktop/platform_config.h @@ -17,17 +17,35 @@ constexpr bool hasPsram = true; // (the general isEsp32/isEsp32S3 family flags had no users and were removed). constexpr bool isEsp32P4 = false; -// No RMT peripheral — the RMT LED driver guards on this and is inert on desktop. -constexpr uint8_t rmtTxChannels = 0; - -// No LCD_CAM peripheral — the LCD LED driver guards on this and is inert too. -constexpr uint8_t lcdLanes = 0; - -// No Parlio peripheral — the Parlio LED driver guards on this and is inert too. -constexpr uint8_t parlioLanes = 0; - -// No I2S-i80 peripheral — MultiPinLedDriver's lanesAvailable() reads lcdLanes + i2sLanes; -// both 0 on desktop, so the driver is inert (host tests exercise only its parse/slice math). +// RMT channels the host reports. Non-zero for the same reason as the parallel lane counts: the +// desktop build emulates the peripheral so RmtLedDriver actually RUNS here, rather than guarding +// itself off and leaving its encode + channel-assignment logic testable only on hardware. 4 matches +// the S3 / P4 / S31 TX channel count (the classic ESP32 has 8), so the host exercises the tighter +// of the two real constraints. +constexpr uint8_t rmtTxChannels = 4; + +// Lane counts the parallel backends report on desktop. NOT zero, deliberately: everything in the +// repo runs on the desktop build — the platform layer just has no hardware behind the call. A +// zero here makes every backend's lanesAvailable() report "not my silicon", so ParallelLedDriver +// idles and its ~2500-line body never executes off-device: not runnable, not unit-testable, and +// invisible to every AST-based check. +// +// 16 is the widest real rig (LightCrafter 16), so the host exercises the same lane-splitting and +// bus-rounding arithmetic the hardware does rather than a degenerate 1-lane path. The bus behind +// them is a heap buffer (platform_desktop.cpp § Parallel-WS2812 buses): the driver encodes real +// WS2812 bit patterns into real memory, and only the DMA hand-off is absent. +constexpr uint8_t lcdLanes = 16; + +// hasLcdCam — TRUE on the host, like the lane counts above: the desktop build emulates the +// peripheral rather than declaring itself incapable. Saying false here would leave the pin-expander +// path (a real feature with real config validation) unreachable off-device, which is the same gap +// the zero lane counts used to create. There is no LCD_CAM silicon; there is a memory bus that +// behaves like one. +constexpr bool hasLcdCam = true; +constexpr uint8_t parlioLanes = 16; + +// MultiPinLedDriver's lanesAvailable() reads lcdLanes + i2sLanes, so this stays 0 — otherwise the +// i80 backend would claim 32 lanes, which no real chip offers. constexpr uint8_t i2sLanes = 0; // No I2S microphone — AudioService guards on this and is inert on desktop. The @@ -98,3 +116,11 @@ constexpr bool hasImprov = false; #else #define MM_MOONLIVE_HAS_HOST_JIT 0 #endif + +// MM_LINKS_ALL_LED_DRIVERS — 1 where the build links every LED driver regardless of silicon. +// The desktop host does: the repo's rule is that everything runs there, with the platform layer +// simply having no hardware behind the call. A driver excluded from the host binary cannot be +// unit-tested, cannot be seen by any AST-based check, and only ever runs where it is hardest to +// debug. A #define (not constexpr) because it gates `#include`s in main.cpp, which `if constexpr` +// cannot do — and it lives here, not in core, per the platform-boundary rule. +#define MM_LINKS_ALL_LED_DRIVERS 1 diff --git a/src/platform/desktop/platform_desktop.cpp b/src/platform/desktop/platform_desktop.cpp index c0cf2ebf..593ec496 100644 --- a/src/platform/desktop/platform_desktop.cpp +++ b/src/platform/desktop/platform_desktop.cpp @@ -9,6 +9,7 @@ #include <cstring> #include <filesystem> #include <string> +#include <vector> // HostBus frame buffers — the memory-backed parallel bus #include <thread> #include <mutex> #include <condition_variable> @@ -1219,21 +1220,44 @@ void TcpServer::close() { } // --------------------------------------------------------------------------- -// RMT WS2812 — no-op stubs. Desktop has no RMT peripheral; the driver guards -// every call with `if constexpr (platform::rmtTxChannels == 0)` (0 here), so -// these exist only to satisfy the linker and are never reached at runtime. +// RMT WS2812 on the host: accepted and counted, not refused. +// +// Same rule as the parallel buses above (architecture.md § Platform abstraction). Refusing here +// made RmtLedDriver inert off device, so nothing in it could be tested on a host. +// +// RMT is symbol-based rather than buffer-based, so there is nothing to hand back: the driver owns +// the symbol array and this seam only has to accept it. The resolution is echoed so the driver's +// timing arithmetic (which divides by it) works on real numbers instead of zero. // --------------------------------------------------------------------------- -bool rmtWs2812Init(RmtWs2812Handle& /*h*/, uint8_t /*gpio*/, uint32_t /*resolutionHz*/, +namespace { +struct HostRmt { uint32_t resolutionHz = 0; }; +HostRmt* hostRmt(void*& impl) { + if (!impl) impl = new HostRmt(); + return static_cast<HostRmt*>(impl); +} +} // namespace + +bool rmtWs2812Init(RmtWs2812Handle& h, uint8_t /*gpio*/, uint32_t resolutionHz, bool /*invert*/) { - return false; + // A zero resolution would make the driver divide by zero when it converts nanoseconds to + // ticks — refuse it here rather than hand back a channel that cannot be used. + if (resolutionHz == 0) return false; + hostRmt(h.impl)->resolutionHz = resolutionHz; + return true; } -uint32_t rmtWs2812Resolution(const RmtWs2812Handle& /*h*/) MM_NONBLOCKING { return 0; } -bool rmtWs2812Transmit(RmtWs2812Handle& /*h*/, const uint32_t* /*symbols*/, - size_t /*symbolCount*/) { - return false; +uint32_t rmtWs2812Resolution(const RmtWs2812Handle& h) MM_NONBLOCKING { + return h.impl ? static_cast<HostRmt*>(h.impl)->resolutionHz : 0; +} +bool rmtWs2812Transmit(RmtWs2812Handle& h, const uint32_t* symbols, + size_t symbolCount) { + if (!h.impl || !symbols || symbolCount == 0) return false; + return true; } void rmtWs2812Wait(RmtWs2812Handle& /*h*/, uint32_t /*timeoutMs*/) {} -void rmtWs2812Deinit(RmtWs2812Handle& /*h*/) {} +void rmtWs2812Deinit(RmtWs2812Handle& h) { + delete static_cast<HostRmt*>(h.impl); + h.impl = nullptr; +} size_t rmtWs2812RxCapture(uint8_t /*gpio*/, uint32_t /*resolutionHz*/, uint32_t* /*outSymbols*/, size_t /*maxSymbols*/, uint32_t /*timeoutMs*/) { @@ -1253,22 +1277,80 @@ RmtLoopbackResult ws2812LoopbackRide(uint16_t /*rxGpio*/, const uint8_t* /*sent* } // --------------------------------------------------------------------------- -// LCD_CAM WS2812 — no-op stubs. Desktop has no i80 peripheral; the LCD LED -// driver guards every call with `if constexpr (platform::lcdLanes == 0)` -// (0 here), so these exist only to satisfy the linker. +// Parallel-WS2812 buses on desktop: REAL MEMORY, no silicon. +// +// The repo's rule is that everything runs on the desktop build — the platform layer simply has +// no hardware behind the call. These used to return false/nullptr, which made every parallel +// backend report failure, so ParallelLedDriver's ~2500-line body never executed off-device: not +// runnable, not unit-testable, and invisible to every AST-based check. +// +// So the bus is implemented against a heap buffer. `init` allocates and zeroes, `Buffer` hands +// back writable memory, `Transmit` records the byte count, `Wait` returns immediately. Everything +// ABOVE the seam is then the same code that runs on hardware — the driver encodes real WS2812 bit +// patterns into a real buffer — and only the DMA hand-off is absent. +// +// What is deliberately NOT modelled: timing, wire protocol, pin state, and loopback capture. +// Those need silicon, and faking them would make the driver's self-test lie about hardware it +// never touched. // --------------------------------------------------------------------------- -bool i80Ws2812Init(I80Ws2812Handle& /*h*/, const uint16_t* /*dataPins*/, +namespace { + +/// One memory-backed parallel bus. Shared by the i80, MoonI80 and Parlio seams below — they are +/// three DMA peripherals for the same job, and off-device the job is "hold a frame". +struct HostBus { + std::vector<uint8_t> buf[2]; + size_t capacity = 0; + + bool init(size_t bytes, bool wantSecond) { + if (bytes == 0) return false; + capacity = bytes; + buf[0].assign(bytes, 0); + if (wantSecond) buf[1].assign(bytes, 0); + else buf[1].clear(); + return true; + } + uint8_t* buffer(uint8_t i) { + if (i > 1 || buf[i].empty()) return nullptr; + return buf[i].data(); + } + bool transmit(uint8_t i, size_t bytes) { + if (i > 1 || buf[i].empty() || bytes > capacity) return false; + return true; + } +}; + +HostBus* hostBus(void*& impl) { + if (!impl) impl = new HostBus(); + return static_cast<HostBus*>(impl); +} +void freeHostBus(void*& impl) { + delete static_cast<HostBus*>(impl); + impl = nullptr; +} + +} // namespace + +bool i80Ws2812Init(I80Ws2812Handle& h, const uint16_t* /*dataPins*/, uint8_t /*laneCount*/, uint16_t /*wrGpio*/, uint16_t /*dcGpio*/, - size_t /*bufferBytes*/, bool /*wantSecondBuffer*/, + size_t bufferBytes, bool wantSecondBuffer, uint8_t /*clockMultiplier*/) { - return false; + if (bufferBytes == 0) return false; // refuse before allocating, as the RMT seam does + return hostBus(h.impl)->init(bufferBytes, wantSecondBuffer); +} +uint8_t* i80Ws2812Buffer(const I80Ws2812Handle& h, uint8_t buffer) { + return h.impl ? static_cast<HostBus*>(h.impl)->buffer(buffer) : nullptr; +} +size_t i80Ws2812BufferCapacity(const I80Ws2812Handle& h) { + return h.impl ? static_cast<HostBus*>(h.impl)->capacity : 0; } -uint8_t* i80Ws2812Buffer(const I80Ws2812Handle& /*h*/, uint8_t /*buffer*/) { return nullptr; } -size_t i80Ws2812BufferCapacity(const I80Ws2812Handle& /*h*/) { return 0; } -bool i80Ws2812Transmit(I80Ws2812Handle& /*h*/, uint8_t /*buffer*/, size_t /*bytes*/) { return false; } +bool i80Ws2812Transmit(I80Ws2812Handle& h, uint8_t buffer, size_t bytes) { + return h.impl && static_cast<HostBus*>(h.impl)->transmit(buffer, bytes); +} +// True, not false: the driver reads a false as "the previous frame never completed" and holds +// the next one back, which would stall the render path on a bus that is never busy. bool i80Ws2812Wait(I80Ws2812Handle& /*h*/, uint8_t /*buffer*/, uint32_t /*timeoutMs*/) { return true; } uint32_t i80Ws2812LastTransmitUs(const I80Ws2812Handle& /*h*/) { return 0; } -void i80Ws2812Deinit(I80Ws2812Handle& /*h*/) {} +void i80Ws2812Deinit(I80Ws2812Handle& h) { freeHostBus(h.impl); } RmtLoopbackResult i80Ws2812Loopback(const uint16_t* /*dataPins*/, uint8_t /*laneCount*/, uint16_t /*wrGpio*/, uint16_t /*dcGpio*/, uint16_t /*rxGpio*/, const uint8_t* /*frame*/, @@ -1277,14 +1359,16 @@ RmtLoopbackResult i80Ws2812Loopback(const uint16_t* /*dataPins*/, uint8_t /*lane return {}; // not supported off the S3 } -// MoonI80 (our own LCD_CAM DMA driver, ADR-0014) — no-op stubs, same as the esp_lcd-backed family -// above. Desktop has no LCD_CAM, so the driver instantiates (lanesAvailable() == 0) and idles, which -// is what lets its config/validation half be tested on the host. -bool moonI80Ws2812Init(MoonI80Ws2812Handle& /*h*/, const uint16_t* /*dataPins*/, +// MoonI80 (our own LCD_CAM DMA driver, ADR-0014) — the same memory-backed bus as the esp_lcd +// family above. The RING path stays inert: it is a GDMA construct with no host equivalent, so a +// driver that would stream on device runs whole-frame here (busInitRing returns false and the +// orchestrator falls back, exactly as its contract specifies). +bool moonI80Ws2812Init(MoonI80Ws2812Handle& h, const uint16_t* /*dataPins*/, uint8_t /*laneCount*/, uint16_t /*wrGpio*/, - size_t /*bufferBytes*/, bool /*wantSecondBuffer*/, + size_t bufferBytes, bool wantSecondBuffer, uint8_t /*clockMultiplier*/) { - return false; + if (bufferBytes == 0) return false; // refuse before allocating, as the other seams do + return hostBus(h.impl)->init(bufferBytes, wantSecondBuffer); } // Ring mode is a GDMA construct with no host equivalent — inert here, bench-verified on the S3, exactly // like the whole-frame path above. A driver that would pick the ring on device stays whole-frame on host. @@ -1301,13 +1385,19 @@ void moonI80Ws2812PrimeRange(MoonI80Ws2812Handle& /*h*/, uint8_t /*bufLo*/, uint bool moonI80Ws2812ArmRing(MoonI80Ws2812Handle& /*h*/) { return false; } bool moonI80Ws2812IsRing(const MoonI80Ws2812Handle& /*h*/) { return false; } bool moonI80Ws2812InternalFits(size_t /*bytes*/) { return false; } -uint8_t* moonI80Ws2812Buffer(const MoonI80Ws2812Handle& /*h*/, uint8_t /*buffer*/) { return nullptr; } -size_t moonI80Ws2812BufferCapacity(const MoonI80Ws2812Handle& /*h*/) { return 0; } -bool moonI80Ws2812Transmit(MoonI80Ws2812Handle& /*h*/, uint8_t /*buffer*/, size_t /*bytes*/) { return false; } +uint8_t* moonI80Ws2812Buffer(const MoonI80Ws2812Handle& h, uint8_t buffer) { + return h.impl ? static_cast<HostBus*>(h.impl)->buffer(buffer) : nullptr; +} +size_t moonI80Ws2812BufferCapacity(const MoonI80Ws2812Handle& h) { + return h.impl ? static_cast<HostBus*>(h.impl)->capacity : 0; +} +bool moonI80Ws2812Transmit(MoonI80Ws2812Handle& h, uint8_t buffer, size_t bytes) { + return h.impl && static_cast<HostBus*>(h.impl)->transmit(buffer, bytes); +} bool moonI80Ws2812Wait(MoonI80Ws2812Handle& /*h*/, uint8_t /*buffer*/, uint32_t /*timeoutMs*/) { return true; } uint32_t moonI80Ws2812LastTransmitUs(const MoonI80Ws2812Handle& /*h*/) { return 0; } MoonI80RingStats moonI80Ws2812RingStats(const MoonI80Ws2812Handle& /*h*/) { return {}; } -void moonI80Ws2812Deinit(MoonI80Ws2812Handle& /*h*/) {} +void moonI80Ws2812Deinit(MoonI80Ws2812Handle& h) { freeHostBus(h.impl); } RmtLoopbackResult moonI80Ws2812Loopback(const uint16_t* /*dataPins*/, uint8_t /*laneCount*/, uint16_t /*wrGpio*/, uint16_t /*rxGpio*/, const uint8_t* /*frame*/, @@ -1323,19 +1413,26 @@ RmtLoopbackResult moonI80Ws2812LoopbackRide(uint16_t /*rxGpio*/, const uint8_t* return {}; // not supported off LCD_CAM } -// Parlio WS2812 — no-op stubs. Desktop has no Parlio peripheral; the driver -// idles (parlioLanes == 0). Sizing/slicing is host-pinned by the driver tests. -bool parlioWs2812Init(ParlioWs2812Handle& /*h*/, const uint16_t* /*dataPins*/, - uint8_t /*laneCount*/, uint32_t /*pclkHz*/, size_t /*bufferBytes*/, - bool /*wantSecondBuffer*/) { - return false; +// Parlio WS2812 — the same memory-backed bus. No Parlio silicon here, but the driver runs and +// its sizing/slicing is host-pinned by the driver tests. +bool parlioWs2812Init(ParlioWs2812Handle& h, const uint16_t* /*dataPins*/, + uint8_t /*laneCount*/, uint32_t /*pclkHz*/, size_t bufferBytes, + bool wantSecondBuffer) { + if (bufferBytes == 0) return false; // refuse before allocating, as the other seams do + return hostBus(h.impl)->init(bufferBytes, wantSecondBuffer); +} +uint8_t* parlioWs2812Buffer(const ParlioWs2812Handle& h, uint8_t buffer) { + return h.impl ? static_cast<HostBus*>(h.impl)->buffer(buffer) : nullptr; +} +size_t parlioWs2812BufferCapacity(const ParlioWs2812Handle& h) { + return h.impl ? static_cast<HostBus*>(h.impl)->capacity : 0; +} +bool parlioWs2812Transmit(ParlioWs2812Handle& h, uint8_t buffer, size_t bytes) { + return h.impl && static_cast<HostBus*>(h.impl)->transmit(buffer, bytes); } -uint8_t* parlioWs2812Buffer(const ParlioWs2812Handle& /*h*/, uint8_t /*buffer*/) { return nullptr; } -size_t parlioWs2812BufferCapacity(const ParlioWs2812Handle& /*h*/) { return 0; } -bool parlioWs2812Transmit(ParlioWs2812Handle& /*h*/, uint8_t /*buffer*/, size_t /*bytes*/) { return false; } bool parlioWs2812Wait(ParlioWs2812Handle& /*h*/, uint8_t /*buffer*/, uint32_t /*timeoutMs*/) { return true; } uint32_t parlioWs2812LastTransmitUs(const ParlioWs2812Handle& /*h*/) { return 0; } -void parlioWs2812Deinit(ParlioWs2812Handle& /*h*/) {} +void parlioWs2812Deinit(ParlioWs2812Handle& h) { freeHostBus(h.impl); } RmtLoopbackResult parlioWs2812Loopback(const uint16_t* /*dataPins*/, uint8_t /*laneCount*/, uint16_t /*rxGpio*/, const uint8_t* /*frame*/, size_t /*frameBytes*/, size_t /*dataBytes*/, diff --git a/src/platform/esp32/platform_config.h b/src/platform/esp32/platform_config.h index 2f2d4511..de60cbc4 100644 --- a/src/platform/esp32/platform_config.h +++ b/src/platform/esp32/platform_config.h @@ -98,6 +98,11 @@ constexpr uint8_t lcdLanes = 16; constexpr uint8_t lcdLanes = 0; #endif +// hasLcdCam — is this LCD_CAM silicon (S3/P4/S31)? Separate from the lane COUNT because the host +// sets a non-zero count so the parallel driver RUNS there against a memory bus, while having no +// LCD_CAM at all. On a real chip the two coincide; the pin expander keys off the capability. +constexpr bool hasLcdCam = (lcdLanes > 0); + // Parallel WS2812 lanes over the Parlio (Parallel IO) TX peripheral — the // ESP32-P4's scale path. The unit does 16 data lines; the driver derives the bus // width (8 or 16) from the pin count. SOC-derived like the others, so a future @@ -304,3 +309,8 @@ constexpr bool hasImprov = true; // in src/platform/esp32/moonlive_emit.cpp), validated by the live hardware run, not host tests. // Kept in platform_config.h so the core header stays free of architecture #ifs. #define MM_MOONLIVE_HAS_HOST_JIT 0 + +// MM_LINKS_ALL_LED_DRIVERS — 0 on ESP32: a board links only the drivers its silicon can run, so +// the type picker stays honest and the binary lean. See the desktop config for why the host is +// the other way round. +#define MM_LINKS_ALL_LED_DRIVERS 0 diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3e61d10d..88d27114 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -31,6 +31,7 @@ add_executable(mm_tests unit/core/unit_ImprovFrame.cpp unit/core/unit_ImprovOpReassembler.cpp unit/core/unit_JsonUtil_parse.cpp + unit/core/unit_JsonUtil_parseint.cpp unit/core/unit_JsonSink_detach.cpp unit/core/unit_MappingLUT.cpp unit/core/unit_ModuleFactory.cpp @@ -80,6 +81,7 @@ add_executable(mm_tests unit/light/unit_WaveEffect.cpp unit/light/unit_FireEffect.cpp unit/light/unit_GridLayout.cpp + unit/light/unit_SingleColumnLayout.cpp unit/light/unit_SphereLayout.cpp unit/light/unit_WheelLayout.cpp unit/light/unit_Layer_extrude.cpp diff --git a/test/scenarios/light/scenario_peripheral_grid_sweep.json b/test/scenarios/light/scenario_peripheral_grid_sweep.json index 9f2ba908..6cf1a220 100644 --- a/test/scenarios/light/scenario_peripheral_grid_sweep.json +++ b/test/scenarios/light/scenario_peripheral_grid_sweep.json @@ -1055,7 +1055,7 @@ }, "desktop-macos": { "tick_us": [ - 273, + 271, 945 ], "free_heap": [ @@ -1068,7 +1068,7 @@ ], "at": [ "2026-07-26", - "2026-07-28" + "2026-07-30" ] } } diff --git a/test/unit/core/unit_JsonUtil_parseint.cpp b/test/unit/core/unit_JsonUtil_parseint.cpp new file mode 100644 index 00000000..60f32ae1 --- /dev/null +++ b/test/unit/core/unit_JsonUtil_parseint.cpp @@ -0,0 +1,55 @@ +// @module JsonUtil + +// Pins the string→int conversion the flat readers share (mm::json::parseIntStr, and parseInt +// through it). The interesting half is what a NON-number does: every caller treats the result as +// a value to clamp or compare, so "not a number" has to arrive as the fallback rather than as a +// plausible-looking integer. + +#include "doctest.h" +#include "core/JsonUtil.h" + +#include <climits> + +using namespace mm; + +TEST_CASE("a JSON integer value is read up to the character that ends it") { + // Digits run until the JSON punctuation that follows them — the reader is handed a pointer + // into the middle of a body, not a clean NUL-terminated number. + CHECK(json::parseIntStr("7,\"next\":1") == 7); + CHECK(json::parseIntStr("42}") == 42); + CHECK(json::parseIntStr("-5,") == -5); + CHECK(json::parseIntStr("0}") == 0); +} + +TEST_CASE("text that does not start with a number reads as the fallback, not as zero-by-accident") { + // The distinction atoi cannot make: it returns 0 for both "0" and "abc". A caller that treats + // 0 as "absent" needs the two to be separable, so the fallback is explicit. + CHECK(json::parseIntStr("abc") == 0); + CHECK(json::parseIntStr("") == 0); + CHECK(json::parseIntStr(nullptr) == 0); + CHECK(json::parseIntStr("abc", -1) == -1); + CHECK(json::parseIntStr("", -1) == -1); + // A real zero still reads as zero — the fallback must not swallow the valid value. + CHECK(json::parseIntStr("0", -1) == 0); +} + +TEST_CASE("a value too large to represent reads as the fallback instead of wrapping") { + // The case that made this worth sharing: atoi on an out-of-range value is undefined + // behavior, and a caller narrowing the result would store a DIFFERENT valid number — + // a Hue id of "65537" becoming light 1. Out-of-range must be visible, not silently wrapped. + CHECK(json::parseIntStr("99999999999999999999") == 0); + CHECK(json::parseIntStr("99999999999999999999", -1) == -1); + CHECK(json::parseIntStr("-99999999999999999999", -1) == -1); + // The boundaries themselves still convert. + CHECK(json::parseIntStr("2147483647") == INT_MAX); + CHECK(json::parseIntStr("-2147483648") == INT_MIN); +} + +TEST_CASE("parseInt reads a key's integer value, and absent keys read as zero") { + CHECK(json::parseInt("{\"a\":1,\"b\":22}", "b") == 22); + CHECK(json::parseInt("{\"a\": 7}", "a") == 7); // space after the colon (json.dumps) + CHECK(json::parseInt("{\"a\":1}", "missing") == 0); + CHECK(json::parseInt(nullptr, "a") == 0); + // A non-numeric value for a present key is not a number: 0, same as absent. + CHECK(json::parseInt("{\"a\":\"text\"}", "a") == 0); +} diff --git a/test/unit/light/host_bus.h b/test/unit/light/host_bus.h new file mode 100644 index 00000000..2322460f --- /dev/null +++ b/test/unit/light/host_bus.h @@ -0,0 +1,73 @@ +#pragma once + +// The host-bus contract, shared by every parallel peripheral's test file. +// +// `i80`, `MoonI80` and `Parlio` are three DMA peripherals for one job, and off-device that job is +// "hold a frame": the desktop platform backs all three with the same heap buffer +// (platform_desktop.cpp § Parallel-WS2812 buses). So the assertions are identical per peripheral, +// and writing them out per file produced two byte-identical ~50-line test cases differing only in +// a type name. Same shape as test/unit/core/conditional_controls.h — one behaviour, one home, +// called with the type under test. + +#include "doctest.h" +#include "light/drivers/ParallelLedDriver.h" + +#include <cstdint> + +namespace mm::test { + +/// Pin the memory-backed bus contract for one peripheral type: allocation, that the encode path +/// can really write and read back, that an over-capacity transmit is refused rather than +/// truncated, and that busWait reports success. +template <typename Peripheral> +inline void checkHostBusAllocates() { + mm::ParallelLedDriver drv; + Peripheral peripheral; + peripheral.attach(&drv); // busInit reads the pin list through the owner + drv.setPeripheralForTest(&peripheral); // borrowed, not owned — no delete of a local + + REQUIRE(peripheral.busInit(768, /*wantSecondBuffer=*/false)); + CHECK(peripheral.busCapacity() == 768); + + uint8_t* buf = peripheral.busBuffer(0); + REQUIRE(buf != nullptr); + // Writable and reads back: the difference between a bus that carries the frame and one that + // silently discards it. + buf[0] = 0xA5; + buf[767] = 0x5A; + CHECK(buf[0] == 0xA5); + CHECK(buf[767] == 0x5A); + + CHECK(peripheral.busTransmit(0, 768)); + // Truncating instead of refusing would look like a good frame while dropping every light past + // the end of the buffer. + CHECK_FALSE(peripheral.busTransmit(0, 4096)); + // Must be true: the driver reads a false as "the previous frame never completed" and holds the + // next one back, stalling a bus that is never actually busy. + CHECK(peripheral.busWait(0, 10)); + + peripheral.busDeinit(); + CHECK(peripheral.busBuffer(0) == nullptr); +} + +/// Double-buffering lets the driver encode one frame while the other is in flight, so the two must +/// be distinct allocations — aliasing them would tear every frame. +template <typename Peripheral> +inline void checkHostBusDoubleBuffer() { + mm::ParallelLedDriver drv; + Peripheral peripheral; + peripheral.attach(&drv); + drv.setPeripheralForTest(&peripheral); + + REQUIRE(peripheral.busInit(256, /*wantSecondBuffer=*/true)); + REQUIRE(peripheral.busBuffer(0) != nullptr); + REQUIRE(peripheral.busBuffer(1) != nullptr); + CHECK(peripheral.busBuffer(0) != peripheral.busBuffer(1)); + + // Re-initialising single-buffered releases the second, rather than leaving a stale span a + // later transmit could read from. + REQUIRE(peripheral.busInit(256, /*wantSecondBuffer=*/false)); + CHECK(peripheral.busBuffer(1) == nullptr); +} + +} // namespace mm::test diff --git a/test/unit/light/unit_MoonLedDriver.cpp b/test/unit/light/unit_MoonLedDriver.cpp index ebe672e2..2ff47a83 100644 --- a/test/unit/light/unit_MoonLedDriver.cpp +++ b/test/unit/light/unit_MoonLedDriver.cpp @@ -69,8 +69,10 @@ void wire(mm::ParallelLedDriver& d, mm::MoonI80Peripheral& peripheral, mm::Buffe TEST_CASE("MoonLedDriver is LCD_CAM-only — it does not claim the classic ESP32's I2S i80") { mm::MoonI80Peripheral peripheral; CHECK(peripheral.lanesAvailable() == mm::platform::lcdLanes); - // The expander needs LCD_CAM, which is exactly where this driver runs — so the two agree. - CHECK(peripheral.supportsPinExpander() == (mm::platform::lcdLanes > 0)); + // The expander needs LCD_CAM silicon — `hasLcdCam`, not `lcdLanes > 0`. The two used to be + // the same test, until the host set a non-zero lane count so the driver would RUN there + // against a memory bus; a lane count is "how wide", the capability is "which peripheral". + CHECK(peripheral.supportsPinExpander() == mm::platform::hasLcdCam); } // The i80 BUS is 8 or 16 bits wide whatever the pin count, so the base rounds it up (powerOfTwoBus()) diff --git a/test/unit/light/unit_MultiPinLedDriver.cpp b/test/unit/light/unit_MultiPinLedDriver.cpp index 6a170d57..6c64a275 100644 --- a/test/unit/light/unit_MultiPinLedDriver.cpp +++ b/test/unit/light/unit_MultiPinLedDriver.cpp @@ -2,6 +2,7 @@ // @also Drivers, Correction #include "doctest.h" +#include "host_bus.h" #include "light/drivers/Correction.h" #include "correction_presets.h" #include "light/drivers/MultiPinLedDriver.h" @@ -416,7 +417,10 @@ TEST_CASE("MultiPinLedDriver loopbackTxPin tracks the loopbackTest toggle") { // driver's peripheral_ (which is protected). TEST_CASE("MultiPinLedDriver hides pinExpander where the chip can't host it") { mm::I80Peripheral peripheral; - CHECK_FALSE(peripheral.supportsPinExpander()); // desktop lcdLanes==0 → unsupported + // Tied to the capability flag, not to a hard-coded value: the host now EMULATES LCD_CAM (so + // the expander path is reachable off-device), and a classic ESP32 still reports false. The + // control's visibility must track whatever the target says, which is what this pins. + CHECK(peripheral.supportsPinExpander() == mm::platform::hasLcdCam); mm::ParallelLedDriver d; d.setPeripheralForTest(&peripheral); @@ -432,3 +436,14 @@ TEST_CASE("MultiPinLedDriver hides pinExpander where the chip can't host it") { } CHECK(found); // still BOUND (a saved value survives), just not shown } + +// The host bus is REAL MEMORY, not a refusal: `busInit` used to return false on desktop, so +// every bus assertion was unreachable off-device and the driver's encode path only ever ran +// on hardware. The contract is identical for all three peripherals, so it lives in one place. +TEST_CASE("MultiPinLedDriver allocates a real host bus the driver can encode into") { + mm::test::checkHostBusAllocates<mm::I80Peripheral>(); +} + +TEST_CASE("MultiPinLedDriver gives the host bus two distinct buffers when asked") { + mm::test::checkHostBusDoubleBuffer<mm::I80Peripheral>(); +} diff --git a/test/unit/light/unit_ParlioLedDriver.cpp b/test/unit/light/unit_ParlioLedDriver.cpp index c1e36f2d..acc6afdc 100644 --- a/test/unit/light/unit_ParlioLedDriver.cpp +++ b/test/unit/light/unit_ParlioLedDriver.cpp @@ -2,6 +2,7 @@ // @also Drivers, Correction #include "doctest.h" +#include "host_bus.h" #include "light/drivers/Correction.h" #include "correction_presets.h" #include "light/drivers/ParlioLedDriver.h" @@ -195,7 +196,7 @@ TEST_CASE("ParlioLedDriver frame grows on RGBW preset") { // latch pad. So the max lights/lane is ~ (65535 − 864) / (channels × 24): **897 for RGB (3ch)**, ~673 // for RGBW (4ch), ~538 for RGBCCT (5ch) — wider fixtures fit fewer lights per one-shot transfer. This // pins the boundary in host-visible frameBytes terms for the RGB and RGBW cases. The reject itself is -// hardware-only (busInit is a desktop no-op), verified on the P4 (LEDs burn at 8×896 RGB/lane; the +// hardware-only (the host bus allocates but enforces no Parlio transfer ceiling), verified on the P4 (LEDs burn at 8×896 RGB/lane; the // driver reports a status error above the ceiling). Catches the ceiling shifting if the encoding // changes. Mirrors the platform constant. TEST_CASE("ParlioLedDriver frame at the Parlio single-transfer ceiling (byte limit, channel-relative)") { @@ -363,3 +364,14 @@ TEST_CASE("ParlioLedDriver loopbackTxPin tracks the loopbackTest toggle") { }; mm::test::checkConditionalControl(d, "loopbackTxPin", setTest, /*visibleWhenTrue=*/true); } + +// The host bus is REAL MEMORY, not a refusal: `busInit` used to return false on desktop, so +// every bus assertion was unreachable off-device and the driver's encode path only ever ran +// on hardware. The contract is identical for all three peripherals, so it lives in one place. +TEST_CASE("ParlioLedDriver allocates a real host bus the driver can encode into") { + mm::test::checkHostBusAllocates<mm::ParlioPeripheral>(); +} + +TEST_CASE("ParlioLedDriver gives the host bus two distinct buffers when asked") { + mm::test::checkHostBusDoubleBuffer<mm::ParlioPeripheral>(); +} diff --git a/test/unit/light/unit_SingleColumnLayout.cpp b/test/unit/light/unit_SingleColumnLayout.cpp new file mode 100644 index 00000000..30c00dd1 --- /dev/null +++ b/test/unit/light/unit_SingleColumnLayout.cpp @@ -0,0 +1,108 @@ +// @module SingleColumnLayout +// @also GridLayout + +// Pins the vertical-column layout: index order, spatial offset, and reversed wiring. +// +// The cross-check against GridLayout is the load-bearing one. A 1-wide, N-high grid and an N-high +// column describe the SAME strip, so the two must emit identical coordinates; anything else means +// one of them is wrong, and only a direct comparison catches a layout that is self-consistently +// wrong. GridLayout had a test and this had none, which is why the pair is asserted here rather +// than each in isolation. + +#include "doctest.h" +#include "light/layouts/GridLayout.h" +#include "light/layouts/SingleColumnLayout.h" + +#include <vector> + +namespace { + +struct CoordEntry { + mm::nrOfLightsType idx; + mm::lengthType x, y, z; +}; + +void collectCoord(void* ctx, mm::nrOfLightsType idx, mm::lengthType x, mm::lengthType y, mm::lengthType z) { + static_cast<std::vector<CoordEntry>*>(ctx)->push_back({idx, x, y, z}); +} + +std::vector<CoordEntry> coordsOf(const mm::LayoutBase& layout) { + std::vector<CoordEntry> out; + layout.forEachCoord(mm::CoordSink{collectCoord, nullptr, &out}); + return out; +} + +} // namespace + +// Indices are contiguous 0..N-1 and every light sits at the configured x — a gap or a repeat here +// is a light the driver would never write, so the strip would have a permanently dark pixel. +TEST_CASE("SingleColumnLayout of height 10 emits ten consecutively indexed lights") { + mm::SingleColumnLayout column; + column.height = 10; + + CHECK(column.lightCount() == 10); + + const auto coords = coordsOf(column); + REQUIRE(coords.size() == 10); + for (mm::nrOfLightsType i = 0; i < 10; i++) { + CHECK(coords[i].idx == i); // contiguous, no holes + CHECK(coords[i].x == 0); // the default X position + CHECK(coords[i].y == static_cast<mm::lengthType>(i)); + CHECK(coords[i].z == 0); + } +} + +// The two layouts must agree: a 1×10×1 grid and a 10-high column are the same physical strip, so +// they produce identical coordinates in identical order or one of them is wrong. +TEST_CASE("A 10-high column emits the same coordinates as a 1x10x1 grid") { + mm::SingleColumnLayout column; + column.height = 10; + + mm::GridLayout grid; + grid.width = 1; + grid.height = 10; + grid.depth = 1; + + CHECK(column.lightCount() == grid.lightCount()); + + const auto a = coordsOf(column); + const auto b = coordsOf(grid); + REQUIRE(a.size() == b.size()); + for (size_t i = 0; i < a.size(); i++) { + CHECK(a[i].idx == b[i].idx); + CHECK(a[i].x == b[i].x); + CHECK(a[i].y == b[i].y); + CHECK(a[i].z == b[i].z); + } +} + +// `starting Y` offsets the coordinates but NOT the indices: the driver writes light 0 first +// whatever the column's position in space, so an offset that shifted indices would leave the +// first `start_y` lights of the strip unwritten. +TEST_CASE("starting Y moves the column in space without renumbering its lights") { + mm::SingleColumnLayout column; + column.start_y = 5; + column.height = 4; + + const auto coords = coordsOf(column); + REQUIRE(coords.size() == 4); + CHECK(coords[0].idx == 0); + CHECK(coords[0].y == 5); + CHECK(coords[3].idx == 3); + CHECK(coords[3].y == 8); +} + +// Reversed wiring flips which end of the strip is light 0 — the y values run high to low while +// the indices still start at 0 and stay contiguous. +TEST_CASE("reversed order walks the column from the far end while keeping indices contiguous") { + mm::SingleColumnLayout column; + column.height = 4; + column.reversed_order = true; + + const auto coords = coordsOf(column); + REQUIRE(coords.size() == 4); + CHECK(coords[0].idx == 0); + CHECK(coords[0].y == 3); + CHECK(coords[3].idx == 3); + CHECK(coords[3].y == 0); +}