Run every LED driver on the desktop; report doc coverage per declaration - #59
Open
ewowi wants to merge 1 commit into
Open
Run every LED driver on the desktop; report doc coverage per declaration#59ewowi wants to merge 1 commit into
ewowi wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // 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") { |
| // 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") { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@moondeck/check/check_clang_query.py`:
- Line 630: Rename the comprehension loop variable l to ln in both occurrences
of the head assignment, including the corresponding expression references, so
the code remains behaviorally identical and passes Ruff E741.
- Around line 786-788: Update the four report-header list literals in the
surrounding output-building code, including the sites near lines 786, 798, 808,
and 814, to wrap adjacent f-string expressions in explicit parentheses before
placing each resulting string in the list. Preserve the existing formatting and
row structure while eliminating implicit string concatenation.
- Around line 29-35: Update the module docstring in check_clang_query.py to
remove or correct the claim that plain // comments never reach the AST,
reflecting that the comments rule reports them through the shadow tree and DEV
column. Keep the surrounding documentation of /// scope reporting and thresholds
unchanged.
In `@moondeck/check/check_module.py`:
- Around line 42-48: Update the relevant paragraph in MoonDeck.md to document
clang-query’s default 60-row cap for every table, including arrays, and explain
that truncated output is announced; remove the outdated no-cap claim and the
71-array rationale while keeping the per-module card behavior accurate.
In `@moondeck/MoonDeck.md`:
- Line 323: Tag both newly added fenced sample-output blocks in MoonDeck.md with
the text language identifier, including the blocks containing “DECL DOC DEV NONE
%DOC” and “DOC DEVIATION DOC WORDS DEV WORDS DECL NAME FILE:LINE”.
- Around line 340-344: Update the sample detail table in MoonDeck.md to include
the VIS column between DEV WORDS and DECL, matching the documented
render_comments output and the VIS description near line 356.
- Around line 392-394: Update the documentation text in MoonDeck.md to refer to
the per-member scope as “attribute” instead of “field,” matching the matrix and
the _SCOPE/render_comments labels. Do not change the surrounding explanation
about function parameters.
In `@src/light/drivers/HueDriver.h`:
- Around line 230-246: Correct the comments above lightNames_ and roomNames_ to
state that ensureNameBuffers() allocates fixed-capacity blocks sized to
kMaxLights × kNameLen and kMaxRooms × kNameLen, rather than the actual bridge
counts. Remove the inaccurate claims about allocation to the actual count and
memory scaling to the live bridge size, while preserving the explanation that
the buffers are heap-allocated to keep sizeof(HueDriver) small.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 1362-1370: Guard moonI80Ws2812Init and parlioWs2812Init against
zero bufferBytes before calling hostBus(h.impl), returning false immediately for
zero-size initialization. Preserve the existing hostBus initialization path for
nonzero buffer sizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1723b3c0-52c9-4dbe-82bf-f17c240fa833
📒 Files selected for processing (20)
CLAUDE.mddocs/architecture.mddocs/history/plans/Plan-20260728 - Doc-comment size reporting via clang-query.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mdmoondeck/MoonDeck.mdmoondeck/check/check_clang_query.pymoondeck/check/check_module.pymoondeck/moondeck_config.jsonsrc/light/drivers/HueDriver.hsrc/light/drivers/MoonLedDriver.hsrc/light/drivers/MultiPinLedDriver.hsrc/main.cppsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.htest/unit/light/host_bus.htest/unit/light/unit_MoonLedDriver.cpptest/unit/light/unit_MultiPinLedDriver.cpptest/unit/light/unit_ParlioLedDriver.cpp
Comment on lines
+29
to
+35
| 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. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Module docstring now contradicts the shipped rule.
Lines 33-34 state plain // "never reaches the AST at all", but the comments rule reports a DEV column precisely because the shadow tree makes // parse. The docstring is the first thing a reader (and --help, via __doc__.splitlines()[0]'s neighbours) sees.
📝 Proposed fix
-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.
+3. **Doc comment size.** `///` comments become the published module pages (moxydoc), and some
+ have grown past being read. Reported per scope — class, method, attribute — 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 reported
+ too: the lexer discards it, so the rule parses a shadow copy of `src/` where every leading
+ `//` has been rewritten to `/// MMDEV: …` (see `_shadow_tree`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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. | |
| 3. **Doc comment size.** `///` comments become the published module pages (moxydoc), and some | |
| have grown past being read. Reported per scope — class, method, attribute — 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 reported | |
| too: the lexer discards it, so the rule parses a shadow copy of `src/` where every leading | |
| `//` has been rewritten to `/// MMDEV: …` (see `_shadow_tree`). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/check/check_clang_query.py` around lines 29 - 35, Update the module
docstring in check_clang_query.py to remove or correct the claim that plain //
comments never reach the AST, reflecting that the comments rule reports them
through the shadow tree and DEV column. Keep the surrounding documentation of
/// scope reporting and thresholds unchanged.
| 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()), "") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename the loop variable l (E741).
Ruff flags both comprehensions as errors; ln reads the same and keeps the file lint-clean.
♻️ Proposed fix
- head = next((l for l in block.split("\n") if l.strip()), "")
+ head = next((ln for ln in block.split("\n") if ln.strip()), "")- head = next((l for l in lines if l.strip()), "")
+ head = next((ln for ln in lines if ln.strip()), "")Also applies to: 660-660
🧰 Tools
🪛 Ruff (0.16.0)
[error] 630-630: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/check/check_clang_query.py` at line 630, Rename the comprehension
loop variable l to ln in both occurrences of the head assignment, including the
corresponding expression references, so the code remains behaviorally identical
and passes Ruff E741.
Source: Linters/SAST tools
Comment on lines
+786
to
+788
| 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}"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Parenthesize the implicit string concatenations inside these list literals (ISC004).
Each of these L += [...] literals relies on adjacent f-strings concatenating across lines; a stray comma silently turns one row into two. Wrapping in parentheses makes the intent explicit and clears the linter.
♻️ Proposed fix (pattern, apply to all four sites)
- L += ["", f" {'DECL':<10} {'DOC':>7} {'DEV':>7} {'NONE':>7} {'%DOC':>5}"
- f" (ideal doc words: {ideals}; ideal dev words: 0)",
+ 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}"]Also applies to: 798-799, 808-809, 814-815
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 786-787: Unparenthesized implicit string concatenation in collection
Did you forget a comma?
(ISC004)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/check/check_clang_query.py` around lines 786 - 788, Update the four
report-header list literals in the surrounding output-building code, including
the sites near lines 786, 798, 808, and 814, to wrap adjacent f-string
expressions in explicit parentheses before placing each resulting string in the
list. Preserve the existing formatting and row structure while eliminating
implicit string concatenation.
Source: Linters/SAST tools
Comment on lines
+42
to
+48
| # 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"]), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dropping --max-rows=0 leaves moondeck/MoonDeck.md stating the opposite.
moondeck/MoonDeck.md (lines 292-293) still says the per-module card runs with no cap and cites HttpServerModule's 71 arrays as the reason — with the default 60-row cap that list is now truncated too, not only the comments table. Update that paragraph alongside this change so the card's documented behaviour matches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/check/check_module.py` around lines 42 - 48, Update the relevant
paragraph in MoonDeck.md to document clang-query’s default 60-row cap for every
table, including arrays, and explain that truncated output is announced; remove
the outdated no-cap claim and the 71-array rationale while keeping the
per-module card behavior accurate.
| publishes), while `//` developer notes belong in the code lines rather than stacked on a | ||
| declaration. The report gives the matrix: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Tag the two new fenced blocks with a language (MD040).
Both sample-output blocks are unlabelled; text keeps the linter quiet without adding syntax highlighting noise.
♻️ Proposed fix
-```
+```text
DECL DOC DEV NONE %DOC-```
+```text
DOC DEVIATION DOC WORDS DEV WORDS DECL NAME FILE:LINEAlso applies to: 339-339
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 323-323: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/MoonDeck.md` at line 323, Tag both newly added fenced sample-output
blocks in MoonDeck.md with the text language identifier, including the blocks
containing “DECL DOC DEV NONE %DOC” and “DOC DEVIATION DOC WORDS DEV WORDS DECL
NAME FILE:LINE”.
Source: Linters/SAST tools
Comment on lines
+340
to
+344
| 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 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Sample detail table is missing the VIS column it documents.
render_comments prints DOC DEVIATION | DOC WORDS | DEV WORDS | VIS | DECL | NAME | FILE:LINE, and line 356 documents VIS, but the example output omits it — a reader matching the doc against real output will think the column is unimplemented.
📝 Proposed fix
- 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
+ DOC DEVIATION DOC WORDS DEV WORDS VIS DECL NAME FILE:LINE
+ +1135% 1606 0 pub class MoonI80Peripheral MoonLedDriver.h:10
+ +797% 1166 0 pub class HttpServerModule HttpServerModule.h:17
+ -100% 0 61 priv method driversOn Scheduler.h:138📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 | |
| ``` | |
| DOC DEVIATION DOC WORDS DEV WORDS VIS DECL NAME FILE:LINE | |
| 1135% 1606 0 pub class MoonI80Peripheral MoonLedDriver.h:10 | |
| 797% 1166 0 pub class HttpServerModule HttpServerModule.h:17 | |
| -100% 0 61 priv method driversOn Scheduler.h:138 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/MoonDeck.md` around lines 340 - 344, Update the sample detail table
in MoonDeck.md to include the VIS column between DEV WORDS and DECL, matching
the documented render_comments output and the VIS description near line 356.
Comment on lines
+392
to
+394
| 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 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
field row does not exist in the report — it is attribute.
The matrix at line 327 and _SCOPE/render_comments both label the per-member scope attribute; sending the reader to "the field row" points at a row that is never printed.
📝 Proposed fix
-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.
+uses `@param` 8 times, all in a `.js` file. Class ATTRIBUTES are the per-member scope that does
+exist, and they are the `attribute` row above.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 | |
| 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 do | |
| exist, and they are the `attribute` row above. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/MoonDeck.md` around lines 392 - 394, Update the documentation text
in MoonDeck.md to refer to the per-member scope as “attribute” instead of
“field,” matching the matrix and the _SCOPE/render_comments labels. Do not
change the surrounding explanation about function parameters.
Comment on lines
+230
to
+246
| /// 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the name-buffer allocation documentation.
Lines 230-234 say the blocks are allocated for the actual bridge count, but ensureNameBuffers() always allocates kMaxLights * kNameLen and kMaxRooms * kNameLen. Lines 242-246 already describe the real fixed-capacity behavior.
Proposed fix
-/// (and cap at 32). One contiguous block is allocated to the ACTUAL count when the fetch runs
+/// (and cap at 32). Fixed-capacity contiguous blocks are allocated lazily when the fetch runs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// 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. | |
| /// 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). Fixed-capacity contiguous blocks are allocated lazily 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(). | |
| /// 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* 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. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/drivers/HueDriver.h` around lines 230 - 246, Correct the comments
above lightNames_ and roomNames_ to state that ensureNameBuffers() allocates
fixed-capacity blocks sized to kMaxLights × kNameLen and kMaxRooms × kNameLen,
rather than the actual bridge counts. Remove the inaccurate claims about
allocation to the actual count and memory scaling to the live bridge size, while
preserving the explanation that the buffers are heap-allocated to keep
sizeof(HueDriver) small.
Comment on lines
+1362
to
+1370
| // 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); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
moonI80Ws2812Init/parlioWs2812Init allocate HostBus even when refusing.
Unlike rmtWs2812Init (line 1244) and i80Ws2812Init (line 1337), which both refuse a zero-size init before calling their allocation helper, these two call hostBus(h.impl) unconditionally — hostBus() will new a HostBus even though init() then returns false for bytes==0. If the caller never calls the matching Deinit on a failed init (plausible, since failure usually means "never initialized"), that HostBus object leaks.
🔧 Proposed fix
bool moonI80Ws2812Init(MoonI80Ws2812Handle& h, const uint16_t* /*dataPins*/,
uint8_t /*laneCount*/, uint16_t /*wrGpio*/,
size_t bufferBytes, bool wantSecondBuffer,
uint8_t /*clockMultiplier*/) {
+ if (bufferBytes == 0) return false; // refuse before allocating, as the RMT seam does
return hostBus(h.impl)->init(bufferBytes, wantSecondBuffer);
} bool parlioWs2812Init(ParlioWs2812Handle& h, const uint16_t* /*dataPins*/,
uint8_t /*laneCount*/, uint32_t /*pclkHz*/, size_t bufferBytes,
bool wantSecondBuffer) {
+ if (bufferBytes == 0) return false; // refuse before allocating, as the RMT seam does
return hostBus(h.impl)->init(bufferBytes, wantSecondBuffer);
}Also applies to: 1415-1433
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/platform_desktop.cpp` around lines 1362 - 1370, Guard
moonI80Ws2812Init and parlioWs2812Init against zero bufferBytes before calling
hostBus(h.impl), returning false immediately for zero-size initialization.
Preserve the existing hostBus initialization path for nonzero buffer sizes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this lands
Everything in the repo now runs on the desktop build. The host links and runs all four LED drivers against emulated peripherals instead of declaring itself incapable. 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 exactly what
ParallelLedDriver's ~2500-line body was.The platform layer simply has no silicon behind the call, so where a peripheral is absent the host emulates it:
lcdLanes/parlioLanesreport 16,rmtTxChannels4,hasLcdCamtrue, and the parallel buses are backed by heap memory rather than returningfalse. Recorded as a hard rule in architecture.md, with its limit stated — timing, wire protocol and pin state are not emulated, because faking them would let a self-test report on hardware it never touched.Worth noting how this landed: the three real backends already routed everything through
platform.hand already had desktop stubs. Only the constants said "not my chip". An earlier attempt built a parallelDesktopPeripheralstand-in and was discarded — it would have duplicated the ~2500 lines it was meant to exercise.A comments rule for clang-query. Doc coverage per declaration, split by scope × kind × visibility, sized in words (a line is a formatting accident; a comment line carries a median of 13 words here).
DOC DEVIATIONis the signed % against 130/40/40-word ideals;DEV WORDShas no ideal because zero is the ideal there.//reaches the AST via a shadow copy ofsrc/underbuild/, where a leading//becomes/// MMDEV:— the marker survives into the comment text, so both kinds stay separable and the real tree is never touched. Clang's lexer otherwise discards//, which is 76% of this tree's comment lines.HueDriver as the pilot: 40% → 100% documented on the public surface. The report earned its keep immediately by finding a real defect — a four-line
///block sat onhasCorrectionControlswhile describingdefineDriverControls, so the published module page showed one method's text under another's name.Also
platform::alloc/allocInternal/allocExec/freeExec. Onlyfreewas listed, so HueDriver read as "8 frees, 0 allocations" — an implied leak that was not there. 80 → 114 sites tree-wide.check_moduleno longer forces--max-rows=0; one module's report is capped at 60 like every other table.Reviewed
The Reviewer (Fable) raised 8 findings, all fixed. The blocker invalidated numbers already published in the docs:
--extra-argappends to the compiler command line, so the compilation database's own-I…/srcwon the include search and every transitive header resolved to the real, unmarked file.//counts were roughly half — 479 → 950 declarations after switching to--extra-arg-before.That is the "a zero is indistinguishable from a tool that read nothing" trap the tooling exists to prevent, and the rule shipped without a control for it. There is one now: if nothing in the tree carries a dev comment, the rule exits 2 rather than reporting a false zero.
Four of the eight were comments contradicting their own code — in a branch about comment accuracy: two stub banners still reading "no-op stubs, the driver idles" above code that allocates;
/// … or -1 before any fetchon a field that is never -1; and "4 is the classic ESP32's TX channel count" (it is 8 — 4 is the S3's).Verification
20 files, +1114/−268. Pre-commit gates: 9 passed, 0 failed, 3 skipped. Desktop 0 warnings, 876 unit tests, 19 scenarios, platform boundary and spec checks clean.
Three ESP32 firmwares byte-identical — 1678243 / 1666467 / 1496740 — so nothing leaked into the device binaries.
Not included: nine scenario JSON timing envelopes the runner rewrites per machine. Measurement noise that would misrepresent performance in the diff.
Known and open
-ast-dump=jsonis the standard construct to switch to.performance.mdis unchanged — its numbers are hardware-measured.🤖 Generated with Claude Code
Summary by CodeRabbit