From 8d491cbc785a04df79642f48b48945ad67c1c930 Mon Sep 17 00:00:00 2001 From: ewowi Date: Wed, 29 Jul 2026 13:26:39 +0200 Subject: [PATCH] Run every LED driver on the desktop; report comments per declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host now links and runs all four LED drivers against emulated peripherals instead of declaring itself incapable, which makes ~2500 lines of driver body testable and visible to the analysis stack for the first time. clang-query gains a comments rule reporting doc coverage per declaration, and HueDriver is the pilot: 100% documented on every scope. KPI: 16384lights | Desktop:920KB | ESP32:1678KB | tick:7248us(FPS:137) | heap:8273KB | lizard:136w **Everything in the repo runs on the desktop build.** Code 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 — which is what the LED drivers were. The platform layer simply has no silicon behind the call, so where a peripheral is absent the host EMULATES it: lcdLanes/parlioLanes report 16, rmtTxChannels 4, hasLcdCam true, and the parallel buses are backed by heap memory rather than returning false. Recorded as a hard rule in architecture.md, with its limit — timing, wire protocol and pin state are NOT emulated, because faking them would let a self-test report on hardware it never touched. The three real backends already routed everything through platform.h and already had desktop stubs; only the constants said "not my chip". So this links the REAL drivers rather than a host-only stand-in — an earlier attempt built a parallel DesktopPeripheral and was discarded as duplicating what it was meant to test. **Core** - platform_config.h (desktop): non-zero lane counts + hasLcdCam. The capability is separate from the lane COUNT: conflating them made the host claim S3-only pin-expander support, so hasLcdCam is its own flag and the drivers key off it. - platform_desktop.cpp: one HostBus behind the i80/MoonI80/Parlio seams, and the RMT seam accepts symbols instead of refusing. The MoonI80 ring stays inert — a GDMA construct with no host equivalent, so the driver runs whole-frame here. - main.cpp links every driver via MM_LINKS_ALL_LED_DRIVERS, a capability macro in platform_config.h. Not an OS #ifdef: the boundary rule forbids those in main.cpp, and an #include cannot be gated by `if constexpr`. **Light domain** - HueDriver: the documentation pilot. ~60 declarations documented or promoted from `//`, four over-long comments compacted. The report found a real defect — a four-line /// block sat on hasCorrectionControls while describing defineDriverControls, so the published page showed one method's text under another's name. Public surface 40% -> 100%. **Scripts/MoonDeck** - clang-query gains a `comments` rule: doc coverage per declaration, split by scope x kind x visibility, sized in WORDS (a line is a formatting accident; a comment line carries a median of 13 words here). DOC DEVIATION is the signed % against 130/40/40-word ideals; DEV WORDS has no ideal because zero IS the ideal. `//` reaches the AST via a shadow copy of src/ under build/, where a leading `//` becomes `/// MMDEV:` — the marker survives into the comment text, so both kinds stay separable and the real tree is never touched. - The heap rule now names platform::alloc/allocInternal/allocExec/freeExec. Only `free` was listed, so HueDriver read as "8 frees, 0 allocations" — an implied leak that was not there. 80 -> 114 sites tree-wide. - check_module no longer forces --max-rows=0; one module's report is capped at 60 like every other table. **Tests** - The host-bus contract is one shared helper (test/unit/light/host_bus.h) rather than byte-identical cases per peripheral — three peripherals, one job. **Reviews** - Reviewer (Fable, 17 files): 8 findings, all fixed. The blocker invalidated numbers already published: `--extra-arg` APPENDS, so the compile database's own -I won and every transitive header resolved to the real, unmarked file. `//` counts were roughly half — 479 -> 950 declarations after switching to --extra-arg-before. A silent-zero guard now refuses to report when nothing in the tree carries a dev comment, which is what should have caught it. Also fixed: cross-TU merge overwrote where its docstring claimed max; two stub banners still said "no-op, driver idles" above code that allocates; a comment claimed colorCount_ can be -1 (it cannot); "4 is the classic ESP32's TX channel count" (it is 8; 4 is the S3's); lost bench knowledge about the bridge's 400 ms fade default, restored. Gates: 9 passed, 0 failed, 3 skipped. Desktop 0 warnings, three ESP32 firmwares 0 warnings and byte-identical, 876 unit tests, 19 scenarios. Not included: scenario JSON timing envelopes the runner rewrites per machine — measurement noise that would misrepresent performance in the diff. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +- docs/architecture.md | 13 + ...-comment size reporting via clang-query.md | 75 ++- docs/metrics/repo-health.json | 46 +- docs/metrics/repo-health.md | 28 +- moondeck/MoonDeck.md | 81 ++++ moondeck/check/check_clang_query.py | 437 +++++++++++++++++- moondeck/check/check_module.py | 13 +- moondeck/moondeck_config.json | 8 +- src/light/drivers/HueDriver.h | 315 +++++++------ src/light/drivers/MoonLedDriver.h | 2 +- src/light/drivers/MultiPinLedDriver.h | 2 +- src/main.cpp | 15 +- src/platform/desktop/platform_config.h | 48 +- src/platform/desktop/platform_desktop.cpp | 175 +++++-- src/platform/esp32/platform_config.h | 10 + test/unit/light/host_bus.h | 73 +++ test/unit/light/unit_MoonLedDriver.cpp | 6 +- test/unit/light/unit_MultiPinLedDriver.cpp | 17 +- test/unit/light/unit_ParlioLedDriver.cpp | 14 +- 20 files changed, 1114 insertions(+), 268 deletions(-) create mode 100644 test/unit/light/host_bus.h diff --git a/CLAUDE.md b/CLAUDE.md index 8835298f..4421b423 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. 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/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..54bb2adb 100644 --- a/docs/metrics/repo-health.json +++ b/docs/metrics/repo-health.json @@ -1,18 +1,18 @@ { - "commit": "4a2effa2", + "commit": "1af1be61", "flash": { - "esp32": 1684240, - "esp32p4-eth": 1497264, + "esp32": 1678368, + "esp32p4-eth": 1497168, "esp32p4-eth-wifi": 1793760, "esp32s3-n16r8": 1666592, "esp32s3-n8r8": 1666592, "esp32s31": 1919136, - "desktop": 878680 + "desktop": 942552 }, "perf": { "desktop": { - "tick_us": 127, - "fps": 7874 + "tick_us": 2741, + "fps": 364 }, "esp32": { "tick_us": 4164, @@ -21,11 +21,11 @@ }, "loc": { "core": 14827, - "light": 19515, - "platform": 11914, + "light": 19540, + "platform": 12054, "ui": 5811, - "test": 34447, - "moondeck": 18318 + "test": 34483, + "moondeck": 18766 }, "comments": { "core": { @@ -33,40 +33,40 @@ "ratio": 0.408 }, "light": { - "lines": 7457, - "ratio": 0.422 + "lines": 7482, + "ratio": 0.423 }, "platform": { - "lines": 3937, - "ratio": 0.366 + "lines": 3994, + "ratio": 0.367 }, "ui": { "lines": 1518, "ratio": 0.278 }, "test": { - "lines": 5898, + "lines": 5912, "ratio": 0.198 }, "moondeck": { - "lines": 2781, - "ratio": 0.174 + "lines": 2938, + "ratio": 0.18 } }, "tests": { - "cases": 970, + "cases": 974, "scenarios": 22 }, "docs": { "md_files": 169, - "md_lines": 22610, + "md_lines": 22534, "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": 2135, "over_threshold": 136, "worst_ccn": 93 } diff --git a/docs/metrics/repo-health.md b/docs/metrics/repo-health.md index d74198eb..6fc0e4ac 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 `1af1be61`. 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,8 +8,8 @@ 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 | +| desktop | 920 KB | +| esp32 | 1,639 KB | | esp32p4-eth | 1,462 KB | | esp32p4-eth-wifi | 1,752 KB | | esp32s3-n16r8 | 1,628 KB | @@ -20,7 +20,7 @@ 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 | 2,741 µs (+2,544 µs) ⚠ | 364 (−4,712) ⚠ | | esp32 | 4,164 µs | 240 | ## Code @@ -28,24 +28,24 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | 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 % | +| light | 19,540 (+1) ⚠ | 7,482 | 42.3 % | +| platform | 12,054 (−1) ✓ | 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,483 (−82) ✓ | 5,912 | 19.8 % (−0.1 %) ✓ | +| moondeck | 18,766 (+30) ⚠ | 2,938 | 18.0 % (+0.1 %) ⚠ | ## Tests | Kind | Count | |---|---:| -| unit cases | 970 | +| unit cases | 974 | | scenarios | 22 | ## Complexity | Metric | Value | |---|---:| -| functions | 2,129 (+1) ✓ | +| functions | 2,135 | | 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,534 (+2) ⚠ | | 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/moondeck/MoonDeck.md b/moondeck/MoonDeck.md index 6d77af12..b84c489e 100644 --- a/moondeck/MoonDeck.md +++ b/moondeck/MoonDeck.md @@ -315,6 +315,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 DECL NAME FILE:LINE + +1135% 1606 0 class MoonI80Peripheral MoonLedDriver.h:10 + +797% 1166 0 class HttpServerModule HttpServerModule.h:17 + -100% 0 61 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 `field` 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. diff --git a/moondeck/check/check_clang_query.py b/moondeck/check/check_clang_query.py index 1bcb55ea..796ae1a9 100644 --- a/moondeck/check/check_clang_query.py +++ b/moondeck/check/check_clang_query.py @@ -26,6 +26,13 @@ Not a violation — the driver layer allocates deliberately — but the hot path must not, and the count is the thing worth trending. +3. **Doc comment size.** `///` comments become the published module pages (moxydoc), and some + have grown past being read. Reported per scope — class, method, field — with NO threshold: + the scopes have different natural sizes (a class header IS the module spec; a method comment + is a sentence), so one cutoff would flag the best-documented modules as defects. The summary + line is what the thresholds get decided from. Plain `//` is not here: the lexer discards it, + so it never reaches the AST at all. + Usage: uv run moondeck/check/check_clang_query.py # every rule uv run moondeck/check/check_clang_query.py --rule arrays @@ -37,6 +44,7 @@ import json import os import re +import shutil import subprocess import sys from concurrent.futures import ThreadPoolExecutor @@ -115,6 +123,95 @@ # 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} + +# 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 +238,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 +281,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")))'), @@ -230,6 +359,57 @@ def _source_tus(build_dir): 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 +423,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 +605,223 @@ 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((l for l in block.split("\n") if l.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((l for l in lines if l.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:]: + 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 +845,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): @@ -541,12 +955,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_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/moondeck_config.json b/moondeck/moondeck_config.json index ac85676e..9e085312 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", @@ -277,7 +279,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/light/drivers/HueDriver.h b/src/light/drivers/HueDriver.h index 133bc53e..9d452148 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: a fixed array would reserve 768 B whether the bridge has 4 lights or 32 + /// (and cap at 32). One contiguous block is allocated to the ACTUAL count when the fetch runs + /// and freed in 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; + /// 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,7 @@ class HueDriver : public DriverBase { return len > 0 && body[len - 1] == '}'; } - // --- Learn the bridge's light ids (window index → hue id, in id order). + /// --- 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 +449,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 +466,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; @@ -480,10 +509,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 +532,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; @@ -540,11 +569,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\":["); @@ -563,16 +592,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 +611,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 +619,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 +631,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 +647,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 +692,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 +711,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 +725,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/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..42ab8ca6 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,15 @@ 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; + 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 +1384,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 +1412,25 @@ 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) { + 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/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>(); +}