diff --git a/.clang-tidy b/.clang-tidy index d8e0b50c..552c02f0 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -94,15 +94,17 @@ # readability-function-cognitive-complexity / *-function-size — complexity is lizard's job; # it produces the per-commit number repo-health.json trends. # bugprone-branch-clone — fires on deliberate parallel switch arms. -# clang-analyzer-security.ArrayBound — analyses the macOS SDK's headers, not our code. +# clang-analyzer-security.ArrayBound — analyzes the macOS SDK's headers, not our code. +# clang-diagnostic-function-effects — the hot-path check, owned by check_nonblocking.py +# and the clang-hotpath card, which splits findings by tick tier and marks what is NEW +# against docs/metrics/hotpath-baseline.txt. Leaving it on here double-reports the same +# hot-path findings with none of that context, and buries clang-tidy's own under them. # # Everything else disabled below is a style opinion we do not hold; enabling any of them # would gate a rewrite rather than a fix. # -# NOT GATED YET. `WarningsAsErrors` stays empty until the remaining 47 clang-analyzer findings -# are triaged (backlog: "clang-tidy: triage the 47 clang-analyzer findings"). Switching it to -# `'*'` is the ratchet that stops a zero decaying, and it is one line — but it fails the gate -# today, so it lands with that triage rather than before it. +# NOT A GATE, BY DESIGN. `WarningsAsErrors` stays empty — the reasoning lives once, in +# docs/testing.md § Static analysis, and applies to every analyser here. --- Checks: >- *, @@ -197,7 +199,8 @@ Checks: >- -google-readability-function-size, -google-build-using-namespace, -google-runtime-int, - -clang-analyzer-security.ArrayBound + -clang-analyzer-security.ArrayBound, + -clang-diagnostic-function-effects WarningsAsErrors: '' HeaderFilterRegex: 'src/(core|light)/' FormatStyle: none diff --git a/.github/codeql-config.yml b/.github/codeql-config.yml new file mode 100644 index 00000000..8338e746 --- /dev/null +++ b/.github/codeql-config.yml @@ -0,0 +1,11 @@ +name: "projectMM CodeQL config" + +# Code we did not write and will not change is not a finding we can act on. doctest.h is the +# vendored single-header test framework (test/doctest.h) and produced the only critical alert +# in the tree that we do not own — excluding it here is the standard mechanism, rather than +# dismissing the same alert by hand after every scan. +# +# This is the ONLY vendored source in the repo; everything else under src/ and test/ is ours. +# Add to this list only for genuinely third-party code, never to quiet a finding in our own. +paths-ignore: + - test/doctest.h diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index ece55a4c..915ae777 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -1,37 +1,35 @@ name: CodeQL -# POC (docs/history/plans/Plan-20260727): does CodeQL's stock C/C++ suite find anything -# real in this firmware? The question that motivated it: we parse six network packet -# formats (ArtNet, DDP, E1.31, WLED audio sync, MQTT, WLED) plus HTTP, doing ~22 memcpy -# operations on data arriving from the LAN, on a device with no MMU and no process -# isolation — and we run NO security analysis at all today. Nothing else in our toolchain -# looks for that class of bug: the compiler checks effects, lizard counts branches, our -# Python checks read text. +# Layer 4 of the analysis stack (docs/testing.md § Static analysis) — the only one that sees +# whole-program taint. It earns its place on one question: we parse six network packet formats +# (ArtNet, DDP, E1.31, WLED audio sync, MQTT, WLED) plus HTTP, doing ~22 memcpy operations on +# data arriving from the LAN, on a device with no MMU and no process isolation. Nothing else in +# the stack looks for that class of bug — the compiler checks effects, lizard counts branches, +# the Python checks read text. # -# `build-mode: none` (GA Oct 2025) scans C/C++ WITHOUT building it, inferring compile flags -# per file. That matters here because our real firmware build is an ESP-IDF cross-compile -# for Xtensa/RISC-V that a runner would have to reproduce; build-free skips that entirely. -# The trade is some extraction noise on macro/template-dense headers — acceptable for a POC -# whose question is "is there anything here", not "prove there is nothing". +# It has paid for itself: 3 critical `localtime` findings in main_desktop.cpp (shared static, +# not thread-safe), fixed via a portable isoTimestamp helper. The six packet parsers produced +# no taint-flow findings, which is positive evidence nothing else in the stack could give. # -# Free on public repos, so the cost is wall-clock only. Scheduled weekly + on demand rather -# than per-push: this is a sweep, not a gate, until the POC says otherwise. +# `build-mode: none` scans C/C++ WITHOUT building it, inferring compile flags per file. That +# matters here because the real firmware build is an ESP-IDF cross-compile for Xtensa/RISC-V a +# runner would have to reproduce. The trade is some extraction noise on macro/template-dense +# headers. # -# If this finds nothing actionable after a few runs, that is a RESULT — record it in the -# scorecard and delete this file. +# Free on public repos, so the cost is wall-clock only. on: - # Push-triggered on the POC branch, deliberately. + # NOT A GATE: no `pull_request` trigger, so it cannot block a merge. The reasoning is + # docs/testing.md § Static analysis, which every analyser here follows. The Security tab + # keeps the open/fixed alert lifecycle, which is the baselining we would otherwise build. # - # `workflow_dispatch` alone would NOT work here: GitHub only offers the "Run workflow" - # button for workflows that exist on the DEFAULT branch, and this one lives on - # next-iteration until the POC is judged. A push trigger is the only way to get a first - # result without merging an unevaluated workflow into main. - # - # This is still not a gate: it does not run on pull_request, so it cannot block a merge. - # It runs, we read the findings, and we decide. + # Both branches are listed on purpose. A workflow whose trigger names a branch nobody is + # pushing to runs never, and reports nothing while looking healthy — this job sat idle for + # two commits when the working branch was called `next` instead of `next-iteration` + # (docs/history/lessons.md). `main` keeps it firing once this branch is merged and retired. push: branches: + - main - next-iteration paths: # Only when C/C++ or the workflow itself changes — a docs commit has nothing new to @@ -70,6 +68,8 @@ jobs: # question above, and the quality ones tell us whether CodeQL sees anything our # existing checks miss. Narrow it later if the noise is not worth it. queries: security-and-quality + # Excludes vendored code (test/doctest.h) — see the file for why. + config-file: ./.github/codeql-config.yml - name: Perform CodeQL analysis uses: github/codeql-action/analyze@v3 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f77c199a..9ac425f9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -80,7 +80,11 @@ jobs: strategy: fail-fast: false # a TSan race and an ASan UAF are independent signals — report both matrix: - kind: [address, thread] + # `realtime` is the runtime half of the hot-path check: MM_NONBLOCKING marks the tick + # methods, -Wfunction-effects proves what it can at compile time, and RTSan catches what + # it cannot — allocation or blocking reached through virtual dispatch or a function + # pointer. Same attribute drives both, so this lane costs one matrix entry. + kind: [address, thread, realtime] steps: - uses: actions/checkout@v4 with: @@ -88,12 +92,34 @@ jobs: # The CMake configure needs uv on PATH — it resolves UV_EXECUTABLE for the UI-embed step # (CLAUDE.md § Use uv for every Python invocation), so a build without it fails at configure. - uses: astral-sh/setup-uv@v3 + # RTSan needs Clang 20+; ubuntu-latest ships Clang 18, which rejects + # `-fsanitize=realtime` at the compiler-probe stage. Install a new enough one for that + # lane only — ASan/TSan run on the runner's default compiler. + - name: install clang for RealtimeSanitizer + if: matrix.kind == 'realtime' + run: | + wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key | sudo tee /etc/apt/trusted.gpg.d/llvm.asc + sudo add-apt-repository -y "deb http://apt.llvm.org/$(lsb_release -cs)/ llvm-toolchain-$(lsb_release -cs)-20 main" + sudo apt-get install -y clang-20 - name: build + run unit tests under ${{ matrix.kind }}sanitizer run: | - cmake -S . -B build/san -DCMAKE_BUILD_TYPE=Debug \ + # RTSan is clang-only — GCC rejects `-fsanitize=realtime` outright — and the attribute + # it keys off is [[clang::nonblocking]], which GCC ignores anyway. Passed as a CMake + # variable on this lane only, rather than exporting CXX: an empty CXX="" on the other + # lanes is not the same as unset, and some probes read it as a broken compiler path. + compiler="" + if [ "${{ matrix.kind }}" = "realtime" ]; then + compiler="-DCMAKE_CXX_COMPILER=clang++-20" + fi + cmake -S . -B build/san -DCMAKE_BUILD_TYPE=Debug $compiler \ -DCMAKE_CXX_FLAGS="-fsanitize=${{ matrix.kind }} -fno-omit-frame-pointer -g" \ -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=${{ matrix.kind }}" cmake --build build/san --target mm_tests -j"$(nproc)" # Leak detection is a different concern from the races/UAF this gate hunts, and it fires on # third-party static init; keep the signal on what we're actually looking for. - ASAN_OPTIONS=detect_leaks=0 TSAN_OPTIONS=halt_on_error=1 ./build/san/test/mm_tests + # + # RTSan does NOT halt: the render path's known blocking calls are frozen in + # docs/metrics/hotpath-baseline.txt and backlogged as architecture work, so halting would + # fail this lane on every run. It reports; the log is the signal. + ASAN_OPTIONS=detect_leaks=0 TSAN_OPTIONS=halt_on_error=1 \ + RTSAN_OPTIONS=halt_on_error=0 ./build/san/test/mm_tests diff --git a/CLAUDE.md b/CLAUDE.md index d332d00a..ac267221 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,6 +63,8 @@ Git only with the PO in the loop: staging, committing, and pushing happen only w On "run pre-commit": `uv run moondeck/event/precommit.py`. It runs every gate whose trigger the change matches and reports PASS / FAIL / SKIP / MANUAL. Then wait for an explicit "commit now". +**"commit now" applies to the diff the PO just reviewed, and any later edit cancels it.** The PO reviews every line before committing (§ Roles), so the go-ahead is scoped to the files as they stood when it was given. Change one afterwards — a review finding, a CI fix, a doc touch-up — and the order is void: say what changed and wait for a fresh "commit now". This holds however small the change and however clearly an earlier instruction seems to cover it ("we commit in one go" says how *many* commits, not *when*). + Commit message: title ≤ 72 characters, imperative. Then a 1–3 sentence end-user TL;DR (no file lists). Then the performance one-liner, measured for every supported target by running `collect_kpi.py --commit` with a board attached. Then change sections as bullets: **Core**, **Light domain**, **UI**, **Scripts/MoonDeck**, **Tests**, **Docs/CI**, **Reviews** (🐇 external / 👾 Reviewer, one bullet per finding: flagged → done/accepted/deferred + why). Core and Light domain are the preferred default categories (a core-module test → Core; a script fix touching a light driver → Light domain). No hard wraps inside a part. Full performance block at the bottom. **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". diff --git a/CMakeLists.txt b/CMakeLists.txt index 7c026207..8c0f9f65 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -42,7 +42,28 @@ if(MSVC) # Static MSVC runtime so the Windows binary doesn't need vcredist. set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>") else() - add_compile_options(-Wall -Wextra -Werror) + # Tier zero on top of -Wall -Wextra: five warnings that catch real defects the base set + # misses. -Wdouble-promotion is the one that earns its place here specifically — the Xtensa + # has no FPU, so an accidental float->double promotion is a silent softfloat call in the + # render path, and it enforces the integer-math rule the coding standards ask for. + # (GCC/Clang only; the MSVC branch above uses /W4 /WX, which has no direct equivalents.) + add_compile_options(-Wall -Wextra -Werror + -Wshadow -Wnon-virtual-dtor -Wdouble-promotion + -Wimplicit-fallthrough -Wnull-dereference) + + # Hot-path discipline, enforced by the compiler. MM_NONBLOCKING marks tick/tick20ms/tick1s + # (platform.h); -Wfunction-effects then checks TRANSITIVELY that nothing they reach + # allocates or blocks, through the whole call graph. Clang 20+ only; the flag does not + # exist on GCC, so the ESP32 build gets no hot-path check — src/platform/esp32/ is + # analysed only where the desktop build reaches it (docs/testing.md § Static analysis). + if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 20) + # -Wno-error is PERMANENT, not a stepping stone: this is a report, not a gate. The + # findings are real and frozen in docs/metrics/hotpath-baseline.txt as architecture + # work; failing the build on them would block every other gate, and a new one may be + # legitimate (a driver that must wait on hardware). The clang-hotpath card reports + # what is NEW; a human judges it. + add_compile_options(-Wfunction-effects -Wno-error=function-effects) + endif() endif() # `uv` is the project's Python launcher (see CLAUDE.md / moondeck/MoonDeck.md). diff --git a/docs/backlog/backlog-core.md b/docs/backlog/backlog-core.md index baf32fb4..167c128a 100644 --- a/docs/backlog/backlog-core.md +++ b/docs/backlog/backlog-core.md @@ -341,23 +341,115 @@ Fix options: (a) make every live mutate scenario clear+rebuild its own canvas (c ## Housekeeping -### clang-tidy: triage the 47 clang-analyzer findings, then gate +### Hot path: move blocking work off the render callbacks (architecture) + +`-Wfunction-effects` proves the render path really does block — these are not annotation gaps, +they are synchronous I/O, allocation and lifecycle work reachable from `tick()`. Each needs the +work moved to a worker or made resumable, so each is a design change rather than a lint fix. +Confirmed by external review (CodeRabbit, PR #56). + +**`tick()` — every frame, the sharp ones:** +- `PreviewDriver::tick` — `sendFrame()` writes a socket synchronously; `buildAndSendCoordTable()` + resizes `keptIdx_`. Currently suppressed at the site with the reason. +- `ParallelLedDriver::tick` — `tickSync()`/`tickRing()` reach `busWaitIfBusy()`, which spins for + the DMA peripheral. Deliberate (the driver owns the bus for the frame) but blocking. Suppressed. +- `Drivers::tick` — joins/stops the render-split worker synchronously on the timed-out recovery + path. Wants asynchronous handling that still preserves buffer ownership. +- `HueDriver::tick` / `tick1s` — synchronous HTTP (`pushOneChangedLight`, `pollPairing`, + `fetchLights`, `fetchGroups`) plus allocation. Wants a queue to a worker. +- `Layer::tick` — calls `applyState()` on a modifier rebuild, which runs prepare/release and + resizes ScratchBuffers. Wants the rebuild deferred to the scheduler's non-render prepare path, + keeping today's coalescing of `consumeNeedsRebuild()`. +- `DemoReelEffect::tick` — `advance()`/`swapTo()` create, delete and `applyState()` child effects + from the render callback. Wants a pending-switch flag consumed off-tick. +- `NetworkSendDriver::tick` — sends inline; wants to enqueue. +- `RmtLedDriver::tick` — waits on hardware completion and reset; wants polling or offload. +- `AudioService::tick` — UDP `sendTo`/`recvFrom` every frame (`syncSend`, `syncReceive`, + `syncEnsureSocket`). + +**`tick20ms()` / `tick1s()` — milder, same shape:** +- `HttpServerModule::tick20ms` — inline `handleConnection` transport work, so the 100 ms budget + cannot actually bound it. Wants resumable connection handling. `tick1s` writes WLED state + frames inline; wants the existing resumable sender. +- `MqttModule::tick1s` — synchronous DNS + discovery-buffer allocation. +- `NetworkModule::tick1s` — WiFi/AP lifecycle transitions and tree rebuilds. +- `FilesystemModule::tick1s` — the debounced `flush()` does filesystem work; the poll itself is + fine, the save wants a worker. +- `FileManagerModule::tick1s` — `platform::filesystemUsed()`; wants a cached value. +- `SystemModule::tick1s` — the P4 `coprocessorWifi()` path calls + `esp_hosted_get_coprocessor_fwversion()` synchronously; wants a cached snapshot. +- `ImprovProvisioningModule::tick` — runs the queued APPLY_OP inline; wants a cold task to + execute and publish only the result. +- `Scheduler::tick` — dispatches all of the above, so its own contract is only as good as theirs. + +**Explicitly NOT in scope:** the float-math findings (`BouncingBallsEffect`, `RipplesEffect`, +`SphereMoveEffect`). Review suggested fixed-point, but the float trajectory IS the ported +MoonLight behaviour and fidelity is deliberate. If the FPU-less Xtensa cost is real, that is a +profiling question first, not a lint fix. + +`-Wfunction-effects` reports these but never fails a build, and +`docs/metrics/hotpath-baseline.txt` freezes the known set so a NEW blocking call stands out +in the report. The pre-commit gate runs the same check incrementally. + +### ESP32 clang/LLVM toolchain — extend the clang checks to src/platform/esp32/ + +Espressif ships an xtensa LLVM (their fork), but the installed `esp-clangd` package contains +**only `clangd`** — no `clang++` driver — so a clang analysis pass over ESP32 sources needs their +full LLVM installed separately. + +What it would buy: `-Wfunction-effects` (and clang-tidy, clang-query) over `src/platform/esp32/` +— 20 translation units, the only code the desktop build excludes. Everything else, including the +LED drivers, already compiles on desktop and is already checked. + +What it costs: a second ~1 GB toolchain, maintained purely for analysis — the firmware would +still be built by GCC, so the analysing compiler is not the shipping compiler. That is a real +"one rule, one owner" tension, and the reason this is a decision rather than an obvious yes. + +Worth revisiting when either the coverage gap bites (a hot-path bug traced to the platform layer +that the desktop check could not see) or Espressif's LLVM becomes the default toolchain. + +### lizard: 94 functions are measured under a mis-parsed name (baseline can't pin them) + +lizard's C++ parser loses the function name on certain bodies and falls back to the first keyword +or cast it meets inside, so `mm::SolidEffect::tick` is reported as `mm::SolidEffect::static_cast` +and `mm::NetworkModule::tick1s` as `mm::NetworkModule::switch`. Measured across `src/`: **94 of +2404** functions carry such a name (`if` 47, `for` 41, `static_cast` 5, `switch` 1), of which **10** +are over threshold and therefore reach the report. + +The consequence is the part that matters. `whitelizard.txt` matches by NAME, so those entries pin +nothing: **35 of 162** baseline lines name a function lizard no longer produces. Each is a function +whose complexity is now unmeasured against the baseline — real growth in it would either surface as +a spurious "NEW violation" under the fallback name, or not surface at all. The check currently +reports `FAIL — 9 NEW` on an unmodified tree for exactly this reason, which trains the reader to +ignore the number. + +It is not a threshold problem and not fixable by re-baselining: re-running `--baseline` just freezes +today's fallback names, which shift again the moment a line moves inside the body. Real options, in +order of preference: key the baseline on `file:startline`-anchored identity or lizard's `long_name` +instead of `name`; pre-process so the parser keeps the name; or replace lizard's C++ front end with +a clang-AST-based complexity pass (`check_clang_query.py` already has the AST machinery, so the +metric could move there and drop the dependency entirely). Sizeable enough for its own `/plan`. + +### clang-tidy: triage the remaining 30 findings `.clang-tidy` runs `*` minus a documented disable list and reaches zero on everything except the -path-sensitive `clang-analyzer-*` family: **47 findings**, led by `core.UndefinedBinaryOperatorResult` -(11), `bugprone-unchecked-string-to-number-conversion` (9), `security.insecureAPI.strcpy` (5), -`cplusplus.NewDeleteLeaks` (4) and `core.NonNullParamChecker` (4). `NewDeleteLeaks` and -`cplusplus.Move` are the two worth reading first — the analyzer traces real paths, so a finding is -a claim about an execution, not a style opinion. +path-sensitive `clang-analyzer-*` family: **30 findings**, led by +`bugprone-unchecked-string-to-number-conversion` (9), `security.insecureAPI.strcpy` (5), +`core.NonNullParamChecker` (4), `deadcode.DeadStores` (3) and `unix.cstring.NullArg` (3). Roughly +half sit in test code (`strcpy` into fixed local buffers, a deliberate divide-by-zero probe). + +The nine `unchecked-string-to-number-conversion` sites were read individually and are all safe: +HueDriver guards on `if (id > 0)`, JsonUtil documents 0-means-absent, MqttModule uses a `v = -1` +sentinel and clamps. They stay reported rather than suppressed — a report shows the real situation, +and a `NOLINT` would hide a genuine class of bug for the next person who writes one of these. These surfaced late for an instructive reason: the report parser's check-name pattern rejected the `,-warnings-as-errors` suffix clang-tidy appends when `WarningsAsErrors` is set, so **every finding was silently dropped** the moment the ratchet was switched on. Fixed; recorded here because it is the sixth silent-zero this tooling has produced, and each looked like a clean tree. -Once triaged (fix / `NOLINT` with a reason / disable with a measured one), set -`WarningsAsErrors: '*'`. That one line is the ratchet that stops a zero decaying back into noise; -it is deliberately not set today because it would fail the gate on the 47. +`WarningsAsErrors` stays empty: clang-tidy reports, it does not gate +([testing.md § Static analysis](../testing.md#static-analysis)). ### Heap-allocate the `registerType` boot probe (lift a per-module lesson into core) diff --git a/docs/coding-standards.md b/docs/coding-standards.md index 7242e0f1..55e0a96f 100644 --- a/docs/coding-standards.md +++ b/docs/coding-standards.md @@ -138,7 +138,7 @@ Two things worth knowing: ## Static checks - **Platform boundary** (`moondeck/check/check_platform_boundary.py`) — scans all files outside `src/platform/` for `#ifdef` / `#if defined` with platform macros and `#include` of platform-specific headers (`esp_*`, `freertos/*`, `driver/*`, `SDL.h`, `wiringPi.h`, …). Fails if any are found. The platform boundary rule itself: [architecture.md § Platform abstraction](architecture.md#platform-abstraction). -- **Hot path lint** (`moondeck/check/check_hotpath.py`) — reads the body of every `tick()` / `tick20ms()` / `tick1s()` under `src/` and flags the allocation (`new`, `malloc`, `push_back`, `std::string`, `make_unique`, `make_shared`) and blocking (`delay`, `sleep`, `mutex.lock()`) it can see. A lint, not a proof: it cannot see what a callee allocates, so a clean run means "nothing visible in the render path's own source". A justified exception is marked `// hot-path-ok: ` at the line, so the reason lives at the site. The hot path rule itself: [architecture.md § Hot path discipline](architecture.md#hot-path-discipline). A "no blocking in the hot path" audit must sweep *every* syscall the path can reach (connect, DNS, read, *and* write), not just the loudest one: a single-threaded loop that services I/O must make no blocking call at all (a socket timeout is not a fix, it is the size of the freeze; non-blocking + poll is the only safe shape). Fixing one blocker while an equally-blocking sibling survives is a partial fix that reads as complete. +- **Hot path check** (`moondeck/check/check_nonblocking.py`) — `MoonModule::tick/tick20ms/tick1s` carry `MM_NONBLOCKING`, and Clang 20+ verifies under `-Wfunction-effects` that nothing they reach allocates or blocks — **transitively**, through the whole call graph. It reports; it does not fail a build: a new blocking call may be legitimate (a driver that must wait for hardware), so the finding is stated and the product owner judges it. `docs/metrics/hotpath-baseline.txt` freezes the known set so a new one stands out. The hot path rule itself: [architecture.md § Hot path discipline](architecture.md#hot-path-discipline). A "no blocking in the hot path" audit must sweep *every* syscall the path can reach (connect, DNS, read, *and* write), not just the loudest one: a single-threaded loop that services I/O must make no blocking call at all (a socket timeout is not a fix, it is the size of the freeze; non-blocking + poll is the only safe shape). Fixing one blocker while an equally-blocking sibling survives is a partial fix that reads as complete. - **Code formatting** — `clang-format` with a project `.clang-format` file. Applied in CI; code that doesn't match fails the check. Run locally via editor integration or `clang-format -i`. ## When checks run diff --git a/docs/history/lessons.md b/docs/history/lessons.md index 15a67bff..1c060845 100644 --- a/docs/history/lessons.md +++ b/docs/history/lessons.md @@ -59,6 +59,7 @@ The plan-18 branch landed plans 17 + 18 + six unplanned follow-ups (19, 19.1, 20 - **Desktop's `platform_desktop.cpp` is correctly asymmetric with ESP32's**, its OTA/Improv/FS sections are 6-line stubs, so per-subsystem files would be all overhead. Symmetry across platforms is a heuristic, not a rule. - **Nightly builds live in their own workflow** (`nightly.yml` tags `nightly-YYYY-MM-DD`, `release.yml` builds via tag push): zero duplication of the build matrix. The skip-on-no-change check costs ~2s on quiet days. - **`workflow_dispatch` reads the workflow YAML from the default branch, not the dispatched branch.** Cost a CI cycle on RC2: dispatched against `plan-18` with a tag invalid against `main`'s older `release.yml`. `inputs.tag` arrives correctly but the consuming logic is whatever main has. +- **`/code-scanning/alerts` returns `0` for a non-default branch unless you pass `?ref=refs/heads/`** — and that zero reads exactly like a clean tree. Same family as the analyser silent-zeros in [testing.md § Verify a zero before believing it](../testing.md#verify-a-zero-before-believing-it): confirm a scan actually ran before reporting what it found. Related: a workflow whose `push:` trigger names a branch the branch is no longer CALLED never runs at all — the CodeQL job sat idle while the branch was `next` instead of `next-iteration`, and nothing reported an error. - **The reviewer agent's PR-merge job is architectural drift across N commits, not line-level bugs** (CodeRabbit's job). Two agents, two scopes. - **A 13-commit branch is the upper end of what one merge should carry**, the merge train is heavy and the reviewer's job gets harder as commits stack. Aim for "ship 3-4 plans, merge, start the next branch." This one worked because the plans were mostly independent. - **MoonModule's asymmetric lifecycle propagation was historical, not principled.** `setup`/`teardown`/`onBuildControls`/`onAllocateMemory` propagated to children, but `loop`/`loop20ms`/`loop1s` defaulted to empty no-ops, so every container duplicated a 5-line per-child block. A shared `tickChildren` helper (gated by `!respectsEnabled() || enabled()`, per-child timing accumulated) closed it; leaf modules pay one predicted-not-taken branch. Desktop tick stayed 55-160 µs. The parked Plan-21 move became four lines. diff --git a/docs/history/plans/Plan-20260727 - Static analysis and repo health tooling.md b/docs/history/plans/Plan-20260727 - Static analysis and repo health tooling.md deleted file mode 100644 index 63ac2f23..00000000 --- a/docs/history/plans/Plan-20260727 - Static analysis and repo health tooling.md +++ /dev/null @@ -1,367 +0,0 @@ -# Plan: static analysis + repo-health tooling - -**Date:** 2026-07-27 · **Status:** in progress — clang-tidy, lizard and clang-query landed (steps 2, 5, 8). Open: warnings tier-zero (1), `[[clang::nonblocking]]` (3), RTSan (4), CodeQL housekeeping (6) - -Temporary document: this text becomes the PR description and the file is deleted once the -plan is realized (CLAUDE.md § Branch). - -## The question - -*Is lizard the right tool, and how do we monitor the repo against our own principles?* - -Answered by researching what serious C++ projects actually **configure** — LLVM, Chromium, -ClickHouse, SerenityOS, Godot, ESPHome, Zephyr, ESP-IDF — rather than by running each tool -once and scoring the output. That distinction matters, and getting it wrong the first time is -what produced the earlier draft of this plan (see *What we got wrong* at the end). - -## The stack - -Five layers, cheapest and most immediate first. Each catches something the layer above cannot. - -| # | Layer | Catches | Where | -|---|---|---|---| -| 0 | **Compiler warnings** | shadowing, double promotion, fallthrough, null deref | Every build, all targets | -| 1 | **clang-tidy** (curated) | bug patterns, performance, portability | Editor as you type **+** CI | -| 2 | **Sanitizers** (ASan+UBSan, TSan) | real memory/threading faults at run time | Desktop CI lanes | -| 3 | **RTSan + `[[clang::nonblocking]]`** | allocation/blocking in the render loop, **transitively** | Compile time + run time | -| 4 | **CodeQL** (stock suite) | untrusted input, whole-program taint, use-after-free | CI, Security tab | - -Alongside them, three tools that measure or extend rather than analyse: - -| Tool | Job | Status | -|---|---|---| -| **lizard** | Complexity *number* per commit, for the trend | Keep, baselined | -| **Our Python checks** | Cross-artifact contracts nothing else can see | Keep as-is | -| **clang-query** | Home for the next bespoke rule | Shipped — 3 rules in `check_clang_query.py` | - -**Our Python checks** stay untouched: ~0.4 s for all four, and they cover contracts whose -other half is a Markdown page, a JSON catalog or a built binary — not a C++ question, so no -analyser can replace them. - -**clang-query** is the designated home for the *next* bespoke rule: the stack enforces the -rules we have, this is how we add the ones we invent. Matchers are plain text in the repo, -minutes to write, no plugin to build and no LLVM ABI coupling — which is why it beats a -compiled clang-tidy check (LLVM-version-locked, and **clangd will not load one**, so a custom -check would never appear in the editor). Same compilation database as layer 1, so adopting it -later costs nothing now. Off until a rule needs it: a tool running zero rules is overhead. - -**First named candidate: large static array declarations** (`char buf[512]` and friends), which -bloat RAM on a 180 KB-heap device and which nothing else in the stack reports. PROBED and it -works — `varDecl(hasType(constantArrayType()))` matches, and `set output dump` yields -`file:line`, the name, and the full type (`char[80]`), so the byte size parses straight out. - -Two things that probe established, both of which shape the eventual script: - -- **There is no size-threshold matcher.** `hasSize(64)` is exact-match only; `sizeGreaterThan` - does not exist. So the matcher takes ALL constant arrays and the "> N bytes" filter, the - our-files-only filter, and the dedupe (template instantiations repeat a declaration) happen - in Python — the same shape as `check_lizard.py`, which already parses a tool's raw output - and applies our policy on top. -- **It needs the same `-isysroot` fix as clang-tidy.** Without it, `Control.cpp` reported 5 - matches; with it, 14. Identical silent-under-report trap to the one recorded below — a wrong - answer, not an error. Any clang-query script must reuse `check_clang_tidy._toolchain_args()`. - -Unprobed: whether stack arrays, class members and statics are worth separating (they are -different problems — a 512-byte local is a stack-depth risk, a 512-byte member is per-instance -RAM), and where the threshold should sit. Both are policy questions for when the rule lands. - -### Is lizard the right tool? Yes — as a counter, not an analyser - -The question this plan opened with, answered directly. - -**Keep it.** It is actively maintained (1.23.0, June 2026), runs in ~1 s, and it does one -thing the rest of the stack does not: it produces a **number per commit** that we can trend. -clang-tidy tells you a function is too complex *today*; lizard tells you whether the codebase -is getting worse *over time*. Those are different jobs, and `repo-health.json` needs the -second. - -**But only as a counter.** Its own README warns it is a fuzzy tokenizer, not a parser — no -macro expansion, confused by heavy templates. It can never express an architectural rule, so -it is not a platform to build on. - -**Its 162 warnings are a threshold artifact, not a verdict:** 161 come from `CCN>10` alone, -and 80% of 2,185 functions sit at CCN ≤ 5. The real tail is 4 functions above CCN 50. A metric -that can never reach zero is a poor gate, which is what baselining fixes. - -**Its VS Code extension is dead** (v1.0.1, Oct 2022) — do not build on it. The live in-editor -equivalent is clang-tidy's `readability-function-*` through clangd, which is layer 1. - -**Overlap with clang-tidy, settled:** both can measure complexity, so they do not both gate. -**lizard owns the metric and the trend**; clang-tidy's complexity checks stay off. One number, -one owner. - -**Deleted:** `check_hotpath.py` (170 lines) once layer 3 is green — the compiler subsumes it -transitively, which a regex never could. - -**Declined:** cppcheck (modest unique yield next to a clean clang-tidy), SonarCloud (a second -findings home; no custom C++ rules at any tier), custom CodeQL queries (the one rule we named -is owned by layer 3), Semgrep (C++ GA is paywalled), Aikido (AppSec aggregator, wrong -category), PVS-Studio / CodeScene / MISRA suites (built for certification, not for us), -`-Weverything` and GCC `-fanalyzer` (Clang's and GCC's own docs advise against, respectively). - -### One rule, one owner - -| Rule | Enforced by | -|---|---| -| No allocation/blocking in the render path | `[[clang::nonblocking]]` + RTSan | -| No platform code outside `src/platform/` | `check_platform_boundary.py` | -| Specs match the code | `check_specs.py` | -| Catalog matches the modules | `check_devices.py` | -| Untrusted input is memory-safe | CodeQL | -| Bug patterns / performance | clang-tidy | -| Complexity does not grow | lizard (baselined) | -| Size/LOC/docs do not grow silently | `repo_health.py` | - -## How clang-tidy gets configured - -The centrepiece, and the part the first attempt botched. Real projects use one of three -shapes; ours is **archetype B — enable families, disable individually with a stated reason** -(the [SerenityOS](https://github.com/SerenityOS/serenity/blob/master/.clang-tidy) shape). - -Nobody serious enables `cppcoreguidelines-*` or `hicpp-*` wholesale: mostly aliases plus -bounds/cast rules that firmware register access must violate. ClickHouse disables the family -as *"impractical… also slow"*; ESPHome — a large ESP32 C++ codebase, our closest peer — does -the same and still runs `WarningsAsErrors: '*'`. - -Starting config, **derived bottom-up from ESPHome's** (their `.clang-tidy` is `*` minus 175 -checks with `WarningsAsErrors: '*'` — battle-tested on a large ESP32 C++ codebase, the closest -peer we have), then tuned top-down against our own tree. The full file is written at -implementation time; the shape is `*` minus ~78 disables, `HeaderFilterRegex: 'src/(core|light)/'`. - -**Tuning it was iterative, and the numbers show why the first attempt failed:** - -| Config | Unique findings in our code | -|---|---:| -| My original shotgun (`bugprone-*,performance-*,concurrency-*`) | 384, of which 66% one noisy check | -| `*` + ESPHome's family disables only | **6,073** | -| + their style-check disables | 3,949 | -| + the `cert-*` family (`cert-err33-c` alone was 3,678 — every `snprintf`) | **131** | - -**131 real findings** — a tractable, mostly-actionable list. Three checks ESPHome disables that -I had wrongly called useful in the first evaluation: `performance-enum-size`, -`bugprone-narrowing-conversions`, `bugprone-easily-swappable-parameters`. They run the same -class of memory-constrained device and still reject all three. - -**Triage outcome: 125 → 47.** Every finding was read against the actual code rather than -trusted. The remaining 47 are all `clang-analyzer-*` — the path-sensitive family — and they -were invisible until `WarningsAsErrors` was switched on: the report parser rejected the -`,-warnings-as-errors` suffix clang-tidy then appends and dropped every finding. So the -"0" this section originally claimed was partly a parser bug, which is the sixth silent-zero -this exercise produced. Backlogged: *clang-tidy: triage the 47 clang-analyzer findings*. -The split, which is the number worth remembering for the next tool evaluation: - -| Disposition | Count | Examples | -|---|---:|---| -| Real defects, fixed | 2 | `std::forward` inside a loop (below); a duplicated `TEST_CASE` + its duplicate include | -| Genuine improvements, applied | ~20 | `localtime`→`localtime_r`, `std::numbers::pi`, `scoped_lock`, `ranges::any_of`, two accidentally-private overrides, a throwing test-rig destructor | -| Deliberate convention → check disabled with a measured reason | ~80 | see the disable table in `.clang-tidy` | -| Deliberate at one site → `NOLINT` with a reason | 12 | Bresenham's assign-and-test; asserting moved-from state *is* the test | - -**The real bug it found** is in `HttpServerModule.cpp`: `visitModuleLeaves(mod, std::forward(fn))` -called inside a `for` loop, at two levels. Forwarding moves the callable into the first module, -so every later sibling receives a moved-from object. It survived because the callables in use -happen to be cheap-to-copy lambdas, which is precisely the kind of latent, works-by-luck defect -a reviewer skims past. - -**The disabled checks are the more interesting result.** Four families were wrong on *every* -occurrence, each because it collides with a deliberate convention: `bugprone-signed-char-misuse` -(12/12 — and its suggested fix would turn the `-1` unset-pin sentinel into GPIO 255, a real bug), -`performance-no-int-to-ptr` (9/9, the `uintptr_t` tagged-pointer field), `bugprone-infinite-loop` -(28/28, `uint8_t` counters), `bugprone-implicit-widening-of-multiplication-result` (47 findings, -0 reachable — margin ~87,000×). A check that is wrong every time trains you to ignore its family, -which costs more than it catches; the reasoning for each is recorded in `.clang-tidy` so the next -reader does not re-litigate it. - -A sample false positive for contrast: `WledPacket.h:80` "memcpy result is not -null-terminated" — the buffer is pre-zeroed by a `memset`, as the adjacent comment says. That -one gets a `NOLINTNEXTLINE` with the reason, which is the intended workflow. - -`bugprone-infinite-loop` **stays enabled** despite being 26/28 false on our `uint8_t` counters -in the first run: the FPs are a known LLVM issue with fixes still landing, and an FP there -sometimes reveals a genuinely missing `volatile`. Each gets a `NOLINTNEXTLINE` with a reason. - -Rejected checks stay listed in the config with their reason, so they are never re-litigated — -[the Chromium pattern](https://github.com/chromium/chromium/blob/main/.clang-tidy). - -**A structural catch for our layout:** clang-tidy picks its config from the *translation -unit's main file*, so a `src/light/.clang-tidy` would **not** govern our header-only light -modules — they are compiled as part of some `.cpp` elsewhere. Per-directory strictness on the -core/light split therefore does not work as one might assume. The working shape is one root -config plus a relaxed `test/.clang-tidy` (`InheritParentConfig: true`), with -`HeaderFilterRegex` covering the headers. - -## Implementation - -Each step is independent and revertible. **✅ done · ◻ not started · ◐ partly done.** - -1. ◻ **Warnings tier-zero** — add `-Wshadow -Wnon-virtual-dtor -Wdouble-promotion - -Wimplicit-fallthrough -Wnull-dereference` to the existing `-Wall -Wextra -Werror`. - `-Wdouble-promotion` catches accidental `double` math: real cost on Xtensa, and it enforces - the integer-math rule. Trial `-Wconversion` separately — expect to reject it for `light/`. -2. ✅ **Baseline lizard** — DONE. `docs/metrics/whitelizard.txt` freezes today's 162 and - `check_lizard.py` fails only on new violations (verified both ways: a probe function makes it - exit 1, removing it returns green). Deliberately NOT at the repo root — that is lizard's - default whitelist path, and a baseline that applies itself silently zeroed the KPI's - complexity count. The KPI and repo-health now record the RAW number via shared code; the - baseline is applied only by the gate. `repo-health.json` gained a `complexity` block, so the - trend the plan asked for actually exists now. -3. ◻ **`[[clang::nonblocking]]`** on `MoonModule::tick/tick20ms/tick1s` (three lines; the - attribute is inherited by overrides, so ~90 modules are covered) + `-Wfunction-effects` on - the desktop build. Then delete `check_hotpath.py`. -4. ◻ **RealtimeSanitizer** — add `realtime` to the sanitizer matrix in `test.yml`. Only - meaningful after step 3, since it keys off the same attributes. -5. ◐ **clang-tidy** — config landed, clangd wired, and the tree triaged from 125 findings to - **0** (see the triage table above). What remains is the ratchet: `WarningsAsErrors` is still - `''`, and cannot go to `'*'` until the 47 clang-analyzer findings are triaged — switching it - on today fails the gate. One line, and it is what stops a zero decaying, so it lands WITH that - triage. Original text: land the config above, wire **clangd first** (editor-only, gates nothing, - and it filters slow checks automatically). Then one full run per family: real bug → fix; - FP → `NOLINTNEXTLINE` with a reason; loud-and-useless → the disable list with a comment. - When a family is clean it joins `WarningsAsErrors`. Target end state is **zero baseline** — - at 50k LOC that is reachable, which is why we skip CodeChecker and diff-only CI entirely. -6. ◻ **CodeQL housekeeping** — exclude vendored code (`test/doctest.h` produced the only - critical we do not own) and decide whether it gates PRs or stays a weekly sweep. -7. ◐ **Triage the 4 real CodeQL findings** — the 3 `localtime` criticals are DONE (a portable - `isoTimestamp` helper in `main_desktop.cpp`, `localtime_r`/`localtime_s` behind the file's - existing `_WIN32` branch); clang-tidy's `concurrency-mt-unsafe` flagged the identical three, - so two independent tools agreeing was the signal to fix rather than suppress. Remaining: file - modes `0666` → `0600` where it matters (desktop only; meaningless on LittleFS). - -8. ✅ **clang-query — the bespoke-rule report. DONE.** `check_clang_query.py`, MoonDeck card - "AST Rules". Shipped with two rules and the frame for more. Measured on this tree: 290 - RAM-costing arrays over 10 bytes (193 local, 97 member) and 78 heap allocation sites. - Thresholds settled from the real numbers, not in advance — excluding constexpr/static - storage took the array list from >1000 to 362, which is what made a 10-byte default usable. - Two traps hit while building it, both silent-zero: a bare top-level `anyOf()` of `Stmt` - matchers is ambiguous and matches NOTHING while exiting 0 (needs a `stmt(...)` wrapper), and - the guard against that must key on more than `"error:"` — clang-query says "Input value has - unresolved overloaded type". Original design: One MoonDeck entry, - `check_clang_query.py`, holding a GROWING LIST of rules we invent — not one script per rule. - Each rule is a matcher plus a Python predicate, and the report prints a section per rule, so - rule two costs a list entry rather than a new script, a new card and a new help page. - - **First rule: large array declarations** (`char buf[512]`), the RAM-bloat question nothing - else in the stack answers. Probed and working (§ clang-query above): match all - `constantArrayType()` declarations, then filter in Python — there is no size-threshold - matcher, so `> N bytes`, our-files-only, and the dedupe of repeated template instantiations - are ours to do. Reuse `check_clang_tidy._toolchain_args()`: without `-isysroot` the same - silent under-report appears (5 matches instead of 14). - - **PO's categories, each probed against the real tree:** - - | Ask | Verdict | - |---|---| - | Stack vs heap vs member | ✅ Separable. `varDecl` = locals/statics, `fieldDecl` = members (my first probe used only `varDecl` and silently missed EVERY class member). `hasStaticStorageDuration()` and `isConstexpr()` split flash-resident tables from real RAM. | - | Fixed number vs named constant | ❌ **Not recoverable.** The AST folds `kMaxLanes` to `[16]` before we see it — `busPinBuf_` reads as `uint16_t[16]` with no trace of the spelling. Source text has it (730 literal-sized vs 105 `kConstant`-sized) but only via regex, which is the fragile approach this whole plan moved away from. Recommend dropping. | - | Heap allocs and frees | ✅ Works. `cxxNewExpr()` / `cxxDeleteExpr()` match, as does `callExpr(callee(functionDecl(hasAnyName("malloc","calloc","realloc","free","heap_caps_malloc",…))))` with exact locations. Needs the our-files filter — a raw `cxxNewExpr()` run returns standard-library internals like `new _Codecvt`. | - | 10-byte threshold | ⚠️ **Measured, and it is too low as a flat rule.** Three sampled files alone hold 245 array declarations: 212 are >10 bytes, 106 >64, 25 >256. Extrapolated across `src/`, >10 bytes is over a thousand findings. | - | Nothing fixed, all dynamic | Agreed as the principle — but see below on what the 10-64 byte band actually contains. | - - **Why a flat 10-byte threshold would misfire.** Sampling the 10-64 byte band shows it is - almost entirely small fixed string buffers and constant lookup tables, and several are the - minimalism principle already applied rather than violated: - - `MoonModule::name_[16]` — its own comment records it was shrunk FROM `char[24]` to save - 8 bytes per module (~240 bytes across a typical tree). Flagging it reports a past win as a - problem. - - `kRGB[3]` / `kGRB[3]` / `kBuiltins[]` — `static constexpr`, so they live in FLASH and cost - no RAM at all. 16 of 17 array decls in `LightPresetsModule.h` are static-storage. - - So the useful report is not "arrays over N bytes" but **arrays that cost RAM**, which the - probed matchers can express: exclude `isConstexpr()` / `hasStaticStorageDuration()`, then - report locals (stack-depth risk), members (per-instance RAM × instance count) and mutable - statics (unconditional RAM) as three separate lists. At that point a threshold near 10 bytes - is plausible, because the flash-resident noise is already gone. **Measure before fixing it.** - - Still open, and to be decided on the first real run rather than now: the exact threshold per - category, and whether the count needs `check_lizard.py`'s baseline shape or is small enough - to be a plain report. - - The rule after that is unassigned on purpose — the point of the step is the *frame*, so the - next rule is a matcher and a threshold rather than a project. - -## Evidence - -### CodeQL — run, and it delivered - -CodeQL 2.26.1, `build-mode: none`: ~3 min, **179 rules, 867 results**, no build required. - -- **3 × critical** `localtime` in `main_desktop.cpp` — real (shared static, not thread-safe), - desktop-only. FIXED via a portable `isoTimestamp` helper; clang-tidy independently flagged the - same three as `concurrency-mt-unsafe`. -- **1 × critical** use-after-free in `test/doctest.h` — vendored; exclude it. -- **5 × high** files created mode `0666` — meaningless on LittleFS, minor hardening on desktop. -- **1 × high** `Control.cpp:168`, `uint8_t o < c.max` where `max` is `int32_t` — benign today - (`addSelect` takes a `uint8_t` count) but a latent trap if that signature widens. - -**The six network packet parsers produced no taint-flow findings** — positive evidence about -~22 `memcpy` calls on LAN data that nothing else in the stack could have given. - -### clang-query — probed as the bespoke-rule engine - -The one part of the first evaluation worth keeping, because it tested *authoring a rule* -rather than reading default output. - -A matcher took minutes and no build: -`match callExpr(callee(functionDecl(hasName("malloc"))), hasAncestor(functionDecl(matchesName("tick.*"))))` -→ 0 matches, a true clean result (broad control matchers returned 42 and 245, proving the -machinery works rather than silently passing). It reaches the **245 control registrations** a -future "a conditional control is registered below the control it depends on" rule would need. - -Class of rule reachable: per-function AST and lexical position — **not** whole-program -reachability. That limit is fine, because the one rule needing reachability (allocation -reachable from `tick()`) is owned by layer 3. - -### `[[clang::nonblocking]]` — verified locally - -Clang 22 caught a `push_back` two levels deep through a helper, tracing the chain into libc++. -The attribute is **inherited by overrides**, so one annotation on the base covers every module. - -### Two gotchas worth keeping - -- **The compilation database's compiler must match the tool's.** A database generated by Apple - Clang while analysing under Homebrew LLVM produced `'cstdint' file not found` on every file - and silently blocked clang-query entirely. -- **`workflow_dispatch` only offers a "Run workflow" button on the default branch**, so a POC - workflow on a feature branch needs a `push:` trigger. And `/code-scanning/alerts` returns - `0` for a non-default branch unless you pass `?ref=refs/heads/` — that zero does not - mean "clean". - -## What we got wrong - -Recorded because the mistake is instructive, and because the first draft of this plan -**declined clang-tidy on bad evidence**. - -The first evaluation ran each tool *once, unconfigured*, and scored the raw output. For -clang-tidy that meant enabling `bugprone-*,performance-*,concurrency-*` — a shotgun — then -counting the noise that choice produced and calling it a tool defect. 66% of the findings came -from `bugprone-easily-swappable-parameters`, **a check that SerenityOS, ClickHouse, ESPHome, -Godot and the Zephyr template all disable by name**. Turning it off is standard practice. - -The comparison was also structurally unfair: CodeQL was given a written workflow with a -curated query set, clang-tidy got one command line, and the two outputs were then compared as -though that were like-for-like. - -The lesson, and the reason this plan now leads with configuration: **a static-analysis tool's -default output is not its verdict.** Evaluating one without a curated check list measures the -evaluator's config, not the tool. - -### The silent-failure modes, all of which read as "clean" - -Five separate defects in the tooling each produced a plausible-looking report rather than an -error. This is the part worth carrying forward, because every one of them would have let a -green check certify an unanalysed tree: - -| Defect | What it looked like | -|---|---| -| `#` inside a YAML `>-` folded scalar is not a comment | Every disable after the first comment was folded into the check string; `abseil-*` stayed on → 12,181 findings | -| `run-clang-tidy` shells out to `clang-tidy` **by name** | Not on PATH → exits 0 having analysed nothing → "0 findings" | -| `-extra-arg VALUE` (space form) is silently ignored | Only `-extra-arg=VALUE` works → 0 findings | -| Same trap in `-checks` | `--check X` filtered nothing; the filter had never worked | -| Compilation database recorded a *different* compiler | `'cstdint' file not found` on 129/129 files; unparsed files are never analysed, so the findings were debris | - -The defence now in `check_clang_tidy.py`: it refuses to report when more than ten files fail to -compile, because "most files errored" is a broken run, not a result. The general rule — **verify -a zero before believing it.** After reaching 0 findings, running a deliberately-disabled check -(`--check readability-magic-numbers`) returned 3,307, which is what proves the pipeline is -actually reading the code. A zero with no control is indistinguishable from a tool that ran -nothing. diff --git a/docs/metrics/hotpath-baseline.txt b/docs/metrics/hotpath-baseline.txt new file mode 100644 index 00000000..0271e175 --- /dev/null +++ b/docs/metrics/hotpath-baseline.txt @@ -0,0 +1,118 @@ +# Hot-path baseline — the blocking calls reachable from tick/tick20ms/tick1s today. +# +# Generated by `uv run moondeck/check/check_nonblocking.py --baseline`. Format is +# `\t`; `#` starts a comment. Keyed on file+callee, NOT line, so an entry +# survives edits above it. +# +# A line here means KNOWN AND BACKLOGGED, not acceptable — see backlog-core.md +# 'move blocking work off the render callbacks'. The list only shrinks: move the work +# off the render path, delete the line. Adding a line means admitting a new blocking +# call on the render path, which is what this exists to prevent. + +src/core/AudioService.h (static local variable) +src/core/AudioService.h mm::AudioService::syncEnsureSocket +src/core/AudioService.h mm::AudioService::syncReceive +src/core/AudioService.h mm::AudioService::syncSend +src/core/AudioService.h mm::platform::audioFft +src/core/AudioService.h mm::platform::audioMicRead +src/core/AudioService.h snprintf +src/core/DevicesModule.h mm::DevicesModule::ageOut +src/core/DevicesModule.h mm::DevicesModule::broadcastPresence +src/core/DevicesModule.h mm::DevicesModule::ensureListener +src/core/DevicesModule.h mm::DevicesModule::localIp +src/core/DevicesModule.h mm::DevicesModule::mergePacket +src/core/DevicesModule.h mm::DevicesModule::upsertSelf +src/core/DevicesModule.h mm::platform::UdpSocket::recvFrom +src/core/FileManagerModule.cpp mm::platform::filesystemUsed +src/core/FilesystemModule.cpp mm::FilesystemModule::flush +src/core/FilesystemModule.cpp mm::FilesystemModule::updateLastSavedStr +src/core/FirmwareUpdateModule.h mm::MoonModule::rebuildControls +src/core/FirmwareUpdateModule.h snprintf +src/core/HttpServerModule.cpp (static local variable) +src/core/HttpServerModule.cpp mm::HttpServerModule::drainPreviewSend +src/core/HttpServerModule.cpp mm::HttpServerModule::handleConnection +src/core/HttpServerModule.cpp mm::HttpServerModule::pollWledStateFromWebSockets +src/core/HttpServerModule.cpp mm::HttpServerModule::pushStateToWebSockets +src/core/HttpServerModule.cpp mm::platform::TcpServer::accept +src/core/ImprovProvisioningModule.h mm::HttpServerModule::applyOp +src/core/ImprovProvisioningModule.h mm::NetworkModule::setTxPowerSetting +src/core/ImprovProvisioningModule.h mm::NetworkModule::setWifiCredentials +src/core/ImprovProvisioningModule.h printf +src/core/ImprovProvisioningModule.h snprintf +src/core/IrService.h mm::IrService::processCode +src/core/IrService.h mm::platform::irRead +src/core/MqttModule.cpp mm::MqttModule::resetConnection +src/core/MqttModule.cpp mm::MqttModule::sendConnectPacket +src/core/MqttModule.cpp mm::MqttModule::serviceConnected +src/core/MqttModule.cpp mm::MqttModule::startConnect +src/core/MqttModule.cpp mm::platform::TcpConnection::connectPoll +src/core/MqttModule.cpp mm::platform::networkReady +src/core/NetworkModule.h mm::NetworkModule::applyStaticIfConfigured +src/core/NetworkModule.h mm::NetworkModule::onConnected +src/core/NetworkModule.h mm::NetworkModule::startAP +src/core/NetworkModule.h mm::NetworkModule::syncAddressingLive +src/core/NetworkModule.h mm::NetworkModule::syncEthLive +src/core/NetworkModule.h mm::NetworkModule::syncMdns +src/core/NetworkModule.h mm::NetworkModule::syncTxPower +src/core/NetworkModule.h mm::NetworkModule::updateMetrics +src/core/NetworkModule.h mm::NetworkModule::updateStatusIP +src/core/NetworkModule.h mm::NetworkModule::writeEthDegradedStatus +src/core/NetworkModule.h mm::platform::mdnsStop +src/core/NetworkModule.h mm::platform::wifiApClientCount +src/core/NetworkModule.h mm::platform::wifiStaInit +src/core/NetworkModule.h mm::platform::wifiStaStop +src/core/NetworkModule.h printf +src/core/NetworkModule.h snprintf +src/core/PinsModule.h mm::PinsModule::PinListSource::refresh +src/core/SystemModule.h mm::Scheduler::elapsed +src/core/SystemModule.h mm::platform::coprocessorWifi +src/core/SystemModule.h mm::platform::freeHeap +src/core/SystemModule.h mm::platform::freeInternalHeap +src/core/SystemModule.h mm::platform::getMacAddress +src/core/SystemModule.h mm::platform::maxInternalAllocBlock +src/core/SystemModule.h snprintf +src/core/TasksModule.h mm::TasksModule::TaskListSource::refresh +src/core/TasksModule.h mm::platform::currentTaskOnCore +src/light/drivers/Drivers.h mm::Drivers::quiesceEncode +src/light/drivers/Drivers.h mm::Drivers::stopEncodeTask +src/light/drivers/Drivers.h mm::platform::notifyTask +src/light/drivers/Drivers.h snprintf +src/light/drivers/HueDriver.h mm::HueDriver::fetchGroups +src/light/drivers/HueDriver.h mm::HueDriver::fetchLights +src/light/drivers/HueDriver.h mm::HueDriver::pollPairing +src/light/drivers/HueDriver.h mm::HueDriver::pushOneChangedLight +src/light/drivers/HueDriver.h mm::HueDriver::reportBridge +src/light/drivers/MoonLedDriver.h mm::platform::moonI80Ws2812IsRing +src/light/drivers/NetworkSendDriver.h mm::platform::UdpSocket::sendToAddr +src/light/drivers/ParallelLedDriver.h mm::LedPeripheral::busBuffer +src/light/drivers/ParallelLedDriver.h mm::LedPeripheral::busCapacity +src/light/drivers/ParallelLedDriver.h mm::LedPeripheral::busLastTransmitUs +src/light/drivers/ParallelLedDriver.h mm::LedPeripheral::refreshBusKpi +src/light/drivers/ParallelLedDriver.h mm::ParallelLedDriver::tickAsync +src/light/drivers/ParallelLedDriver.h mm::ParallelLedDriver::tickRing +src/light/drivers/ParallelLedDriver.h mm::ParallelLedDriver::tickSync +src/light/drivers/ParallelLedDriver.h snprintf +src/light/drivers/PreviewDriver.h (static local variable) +src/light/drivers/PreviewDriver.h mm::BinaryBroadcaster::bufferedSendIdle +src/light/drivers/PreviewDriver.h mm::BinaryBroadcaster::clientGeneration +src/light/drivers/PreviewDriver.h mm::PreviewDriver::buildAndSendCoordTable +src/light/drivers/PreviewDriver.h mm::PreviewDriver::sendFrame +src/light/drivers/RmtLedDriver.h mm::platform::delayUs +src/light/drivers/RmtLedDriver.h mm::platform::rmtWs2812Transmit +src/light/drivers/RmtLedDriver.h mm::platform::rmtWs2812Wait +src/light/effects/DemoReelEffect.h mm::DemoReelEffect::advance +src/light/effects/NetworkReceiveEffect.h mm::NetworkReceiveEffect::noteReceiving +src/light/effects/NetworkReceiveEffect.h mm::NetworkReceiveEffect::replyToPoll +src/light/effects/NetworkReceiveEffect.h mm::platform::UdpSocket::recvFrom +src/light/effects/RubiksCubeEffect.h (static local variable) +src/light/effects/RubiksCubeEffect.h mm::RubiksCubeEffect::init +src/light/layers/Layer.h mm::EffectBase::dimensions +src/light/layers/Layer.h mm::Layer::applyLivePass +src/light/layers/Layer.h mm::ModifierBase::consumeNeedsRebuild +src/light/layers/Layer.h mm::MoonModule::applyState +src/light/moonlive/MoonLiveEffect.h mm::moonlive::MoonLive::run +src/platform/desktop/platform_desktop.cpp inet_pton +src/platform/desktop/platform_desktop.cpp mm::platform::hostIp +test/unit/light/unit_Drivers_rendersplit.cpp (static local variable) +test/unit/light/unit_Drivers_rendersplit.cpp std::condition_variable::notify_all +test/unit/light/unit_Drivers_rendersplit.cpp std::condition_variable::wait_for, (lambda at test/unit/light/unit_Drivers_rendersplit.cpp:64:38)> diff --git a/docs/metrics/repo-health.json b/docs/metrics/repo-health.json index c54fcd7a..4936cdd8 100644 --- a/docs/metrics/repo-health.json +++ b/docs/metrics/repo-health.json @@ -1,56 +1,56 @@ { - "commit": "df9fbca6", + "commit": "4a2effa2", "flash": { "esp32": 1684240, "esp32p4-eth": 1497264, "esp32p4-eth-wifi": 1793760, - "esp32s3-n16r8": 1667104, + "esp32s3-n16r8": 1666592, "esp32s3-n8r8": 1666592, "esp32s31": 1919136, - "desktop": 1413464 + "desktop": 878680 }, "perf": { "desktop": { - "tick_us": 128, - "fps": 7812 + "tick_us": 127, + "fps": 7874 }, "esp32": { - "tick_us": 4218, - "fps": 237 + "tick_us": 4164, + "fps": 240 } }, "loc": { - "core": 14819, - "light": 19511, - "platform": 11842, + "core": 14827, + "light": 19515, + "platform": 11914, "ui": 5811, - "test": 34420, - "moondeck": 18055 + "test": 34447, + "moondeck": 18318 }, "comments": { "core": { - "lines": 5541, + "lines": 5549, "ratio": 0.408 }, "light": { - "lines": 7433, - "ratio": 0.421 + "lines": 7457, + "ratio": 0.422 }, "platform": { - "lines": 3892, - "ratio": 0.364 + "lines": 3937, + "ratio": 0.366 }, "ui": { "lines": 1518, "ratio": 0.278 }, "test": { - "lines": 5874, - "ratio": 0.197 + "lines": 5898, + "ratio": 0.198 }, "moondeck": { - "lines": 2719, - "ratio": 0.173 + "lines": 2781, + "ratio": 0.174 } }, "tests": { @@ -59,15 +59,15 @@ }, "docs": { "md_files": 169, - "md_lines": 22540, + "md_lines": 22610, "plans_files": 89, - "backlog_lines": 3021, + "backlog_lines": 3114, "lessons_lines": 378, "claude_md_lines": 126 }, "complexity": { - "functions": 2186, - "over_threshold": 162, + "functions": 2129, + "over_threshold": 136, "worst_ccn": 93 } } diff --git a/docs/metrics/repo-health.md b/docs/metrics/repo-health.md index 540af343..d74198eb 100644 --- a/docs/metrics/repo-health.md +++ b/docs/metrics/repo-health.md @@ -1,6 +1,6 @@ # Repo health -Measured at `df9fbca6`. Generated by [`moondeck/check/repo_health.py`](../../moondeck/check/repo_health.py) on every KPI-gate run — **do not edit by hand**. +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**. 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,7 +8,7 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | Target | Flash | |---|---:| -| desktop | 1,380 KB | +| desktop | 858 KB (+0 KB) ⚠ | | esp32 | 1,645 KB | | esp32p4-eth | 1,462 KB | | esp32p4-eth-wifi | 1,752 KB | @@ -20,19 +20,19 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | Target | Tick | FPS | |---|---:|---:| -| desktop | 128 µs (−2 µs) ✓ | 7,812 (+120) ✓ | -| esp32 | 4,218 µs | 237 | +| desktop | 127 µs | 7,874 | +| esp32 | 4,164 µs | 240 | ## Code | Area | Lines | Comments | Comment share | |---|---:|---:|---:| -| core | 14,819 | 5,541 | 40.8 % | -| light | 19,511 | 7,433 | 42.1 % | -| platform | 11,842 | 3,892 | 36.4 % | +| core | 14,827 | 5,549 | 40.8 % | +| light | 19,515 | 7,457 | 42.2 % | +| platform | 11,914 (+24) ⚠ | 3,937 | 36.6 % | | ui | 5,811 | 1,518 | 27.8 % | -| test | 34,420 | 5,874 | 19.7 % | -| moondeck | 18,055 | 2,719 | 17.3 % | +| test | 34,447 | 5,898 | 19.8 % | +| moondeck | 18,318 | 2,781 | 17.4 % | ## Tests @@ -45,8 +45,8 @@ Current state only; the trend is this file's git history (`git log -p docs/metri | Metric | Value | |---|---:| -| functions | 2,186 | -| over threshold | 162 | +| functions | 2,129 (+1) ✓ | +| over threshold | 136 | | worst CCN | 93 | ## Documentation @@ -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,540 | +| markdown lines | 22,610 (+18) ⚠ | | plan files | 89 | -| backlog lines | 3,021 | +| backlog lines | 3,114 | | lessons lines | 378 | | CLAUDE.md lines | 126 | diff --git a/docs/metrics/whitelizard.txt b/docs/metrics/whitelizard.txt index 1e284483..eb243845 100644 --- a/docs/metrics/whitelizard.txt +++ b/docs/metrics/whitelizard.txt @@ -40,7 +40,7 @@ src/core/MqttModule.cpp:mm::MqttModule::routePublish # CCN 25, NLOC 61 src/core/HttpServerModule.cpp:mm::HttpServerModule::parseFilePath # CCN 25, NLOC 33 src/main.cpp:mm_main # CCN 24, NLOC 148 src/light/layers/Layer.h:mm::Layer::buildFoldedLUT # CCN 24, NLOC 68 -src/light/effects/FreqSawsEffect.h:mm::FreqSawsEffect::tick # CCN 24, NLOC 62 +src/light/effects/FreqSawsEffect.h:mm::FreqSawsEffect::for # CCN 13, NLOC 36 (was 24/62; lizard cannot name this tick and falls back to an inner token) src/light/draw.h:mm::draw::line # CCN 24, NLOC 43 src/light/drivers/Drivers.h:mm::Drivers::prepare # CCN 24, NLOC 36 src/light/drivers/ParallelLedDriver.h:mm::ParallelLedDriver::runLoopbackSelfTest # CCN 23, NLOC 65 diff --git a/docs/testing.md b/docs/testing.md index 8877740a..10fd270e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -27,6 +27,71 @@ Three test categories, each with a clear purpose: Per-module timing, memory, and sizeof measurements per platform live in [performance.md](performance.md). +## Static analysis + +Tests pin behaviour that runs; static analysis catches what never gets exercised. Five layers, +cheapest first — each catches something the layer above cannot: + +| # | Layer | Catches | Where | +|---|---|---|---| +| 0 | Compiler warnings | shadowing, double promotion, fallthrough, null deref | Every build, all targets | +| 1 | [clang-tidy](../.clang-tidy) | bug patterns, performance, portability | Editor (clangd) + MoonDeck card | +| 2 | Sanitizers (ASan, TSan) | real memory/threading faults at run time | Desktop CI lanes | +| 3 | RTSan + `[[clang::nonblocking]]` | allocation/blocking in the render path, **transitively** | Compile time + CI | +| 4 | [CodeQL](../.github/codeql-config.yml) | untrusted input, whole-program taint, use-after-free | CI, Security tab | + +Alongside them: **lizard** counts complexity per commit for the trend (a fuzzy tokenizer, not a +parser — it can never express an architectural rule), **clang-query** is the home for bespoke +AST rules we invent, and the Python checks in `moondeck/check/` cover contracts whose other half +is a Markdown page, a JSON catalog or a built binary. Every one has a MoonDeck card +([MoonDeck.md](../moondeck/MoonDeck.md)). + +**Every one of these is a report, not a gate.** They state what they find; a consumer decides +what to do about it. `WarningsAsErrors` is empty, CodeQL never runs on `pull_request`, and the +hot-path check never fails the event. A gate nobody can satisfy gets disabled rather than +obeyed, and it pushes people to suppress a finding under time pressure — which is the opposite +of why the tool is there. The exception is layer 0: compiler warnings ARE `-Werror`, because +they are few, actionable, and fixed at the moment they appear. + +**One rule, one owner.** Each rule is enforced in exactly one place, so a finding has one home +and cannot drift between two tools that half-agree: + +| Rule | Enforced by | +|---|---| +| No allocation/blocking in the render path | `[[clang::nonblocking]]` + RTSan | +| No platform code outside `src/platform/` | `check_platform_boundary.py` | +| Specs match the code | `check_specs.py` | +| Catalog matches the modules | `check_devices.py` | +| Untrusted input is memory-safe | CodeQL | +| Bug patterns / performance | clang-tidy | +| Complexity does not grow | lizard (baselined) | +| Size/LOC/docs do not grow silently | `repo_health.py` | + +### Verify a zero before believing it + +An analyser reporting "0 findings" is indistinguishable from one that read nothing, and every +silent-failure mode below produced a plausible clean report rather than an error: + +| Defect | What it looked like | +|---|---| +| `#` inside a YAML `>-` folded scalar is not a comment | Every disable after the first was folded into the check string; `abseil-*` stayed on → 12,181 findings | +| `run-clang-tidy` shells out to `clang-tidy` by name | Not on PATH → exits 0 having analysed nothing → "0 findings" | +| `-extra-arg VALUE` (space form) is silently ignored | Only `-extra-arg=VALUE` works → 0 findings | +| Same trap in `-checks` | The filter had never worked | +| Compilation database records a different compiler than the tool runs | `'cstdint' file not found` on 129/129 files; unparsed files are never analysed | +| Missing `-isysroot` | Under-report, not an error: 5 matches where there were 14 | +| A baseline keyed on a name the tool no longer emits | Pins nothing while looking green (see backlog-core.md § lizard) | + +So: **run a control check that MUST fire.** After reaching 0, enabling a deliberately-disabled +check (`--check readability-magic-numbers`) returned 3,307 — that is what proves the pipeline +reads the code. `check_clang_tidy.py` also refuses to report when more than ten files fail to +compile, because "most files errored" is a broken run, not a result. + +A tool's *default* output is not its verdict, either. Evaluating an analyser without a curated +check list measures the configuration, not the tool: an unconfigured clang-tidy run here gave +6,073 findings, two thirds of them from one check that every comparable project disables by +name. The curated config takes it to 30. + ## Standards Two principles drive every standard below: diff --git a/moondeck/MoonDeck.md b/moondeck/MoonDeck.md index 513b7213..6d77af12 100644 --- a/moondeck/MoonDeck.md +++ b/moondeck/MoonDeck.md @@ -9,6 +9,7 @@ Below: the UI behaviours common to every card, described once, then one section ## UI Features - **Status dots** on each card: grey (not run), orange (running), green (exit 0), red (exit non-zero). +- **Last-run log** — the **📄** button replays that script's last run in the log pane. It appears **only on cards that have actually run** (and shows up the moment a first run finishes, no reload needed), so its absence is informative too: a card with no 📄 is one nobody has used yet. Every run is teed to `build/moondeck-logs/.log` as it streams (not buffered to the end, so a run you Stop still leaves what it printed), which answers "what did this do last time" after a page reload or a switch to another card — the case a live-only stream cannot. One file per script, overwritten each run: a last-run record, not a history. Gitignored, being derived state. - **Run/Stop toggle** for long-running scripts (Run desktop, Monitor ESP32). - **Duration hint** — every card shows how long it takes: ⚡ about a second, ⏱️ a few seconds up to ~30, 🐌 more than 30 seconds (a build, a flash, a gate list, clang-tidy). All three are shown rather than only the extremes, so a blank badge reads as "nobody set a speed on this card" instead of being confused with medium. Set per script as `"speed": "instant" | "medium" | "slow"` in `moondeck_config.json`. This is a *label*, not a timeout — nothing enforces it, so a script that grows slower needs its flag updated by hand. Separate from `long_running`, which controls the Run/Stop toggle rather than expected duration. - **Group headers** in the sidebar (setup, build, flash, run, test, check, scenario). @@ -104,17 +105,6 @@ uv run moondeck/check/check_platform_boundary.py Scans all source files outside `src/platform/` for forbidden includes and platform `#ifdef`s. -### check_hotpath - -Flag allocation or blocking written directly in a render-path method. - -```bash -uv run moondeck/check/check_hotpath.py # report findings -uv run moondeck/check/check_hotpath.py --list # list the methods it scans -``` - -Reads the body of every `tick()` / `tick20ms()` / `tick1s()` under `src/` and reports the banned constructs it can see: `new` / `malloc` / `push_back` / `std::string` / `make_unique` / `make_shared` (allocation) and `delay` / `sleep` / `mutex.lock()` (blocking). A **lint, not a proof** — it cannot see what a callee allocates, so a clean run means "nothing visible in the render path's own source". A justified exception is marked `// hot-path-ok: ` at the line, so the reason lives at the site. - ### check_esp32_built Check that a firmware binary exists and is newer than every source that feeds it. @@ -198,6 +188,15 @@ Two properties worth knowing. Measurements read **tracked files only** (`git ls- Static-analysis tools, run **manually**: they take minutes rather than seconds, so they are not in the commit/merge gate lists yet. +**A report shows the real situation.** Array usage, hot-path blocking, complexity — the number +is only worth reading if nothing was hidden to make it smaller. A finding is *fixed*, or it is +*shown with its reason*; it is never suppressed to tidy the output. `ParallelLedDriver::tick` +and `PreviewDriver::tick` genuinely block, so they appear in clang-hotpath every run — hiding +the two worst offenders would have made the report worthless while making the count look better. +The only suppression that earns its place is one where the tool is wrong about our code (e.g. +libc++ not annotating `steady_clock::now`, which does not block), and it carries that reason at +the site. + ### check_module Every static-analysis tool, on ONE module — the repo-wide reports turned around. @@ -244,11 +243,9 @@ finding grouped by file. No report file to open: a run this slow should answer o and the old `build/clang-tidy-report.md` was gitignored anyway, so it existed only to be read once. -**Verify a zero before believing it.** A tool that analysed nothing also reports zero, and this -one has four documented ways to fail silently (the reasons are in the script). The control is a -check you know must fire: `--check readability-magic-numbers` returns thousands. If that returns -zero too, the run is broken, not the tree. The script also refuses to report when more than ten -files fail to compile, because "most files errored" is a broken run and not a result. +**Verify a zero before believing it** ([testing.md](../docs/testing.md#verify-a-zero-before-believing-it) +covers why and lists the known silent-failure modes). This script's own guard: it refuses to +report when more than ten files fail to compile. ### check_clang_query @@ -322,6 +319,53 @@ Takes ~50s cold (a few seconds once the compilation database is warm). clang-que 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. +### check_nonblocking + +What the render path calls that can block or allocate — checked by the compiler. + +```bash +uv run moondeck/check/check_nonblocking.py # summary by callee, then every site +uv run moondeck/check/check_nonblocking.py --module AudioService +``` + +`MoonModule::tick/tick20ms/tick1s` carry `MM_NONBLOCKING` ([platform.h](../src/platform/platform.h)), +and Clang 20+ verifies under `-Wfunction-effects` that nothing they reach allocates or blocks — +**transitively**, through the whole call graph ([coding-standards.md § Static checks](../docs/coding-standards.md#static-checks) owns the rule). + +The attribute is inherited by overrides, so three annotations cover every module's tick. It also +sits in `tickChildren`'s **member-pointer type** — without that, the indirect call through `fn` +is a hole the check cannot reason about, and passing an unannotated method now fails to compile. + +Reports unique **sites**: a header included by N translation units emits the same warning N +times, so a raw build prints ~1350 lines for ~180 real findings. + +**Split by tick tier**, because the same blocking call costs roughly two orders of magnitude +more in one than another: `tick()` runs every frame, `tick20ms()` fifty times a second, `tick1s()` once. +Pooling them hides which findings actually matter. `OTHER` is a site whose enclosing method could +not be resolved from source. + +| Column | | +|---|---| +| **CALLS** | the function that blocks — or `(static local variable)`, a violation with no callee: a static local needs a guard variable and a one-time lock on first use | +| **IN** | the method the call sits in, which is what places it in a tier. Clang names the call and the callee but *not* their enclosing function, so this is read back from the source | +| **WHY IT BLOCKS** | clang's own root cause, e.g. `calls mm::platform::UdpSocket::sendTo`. `—` means a leaf the compiler could not look inside (external or unannotated) | +| **FILE:LINE** | where to go | + +**Desktop-only, and that loses nothing.** On GCC `MM_NONBLOCKING` expands to `noexcept` — the +exception contract still holds; only the clang attribute and the warning are absent. The ESP32 toolchain +has neither the attribute nor the warning, and builds with `-Werror`, so a bare attribute there +is a build break. Every tick method compiles on desktop — modules, effects, and the **LED +drivers** — so the render path itself is covered. + +The gap is real but narrow: `src/platform/esp32/` has no tick methods (it is free functions the +tick path calls into), and while a call INTO one of them is reported at the call site, the +function's own body is never analyzed. A platform function that blocks internally without +carrying `MM_NONBLOCKING` is invisible. Closing that needs an xtensa clang — backlogged as +"ESP32 clang/LLVM toolchain" in backlog-core.md. + +Not a gate yet: `-Wno-error=function-effects` keeps the build green while the findings are +triaged. Each is a judgement — fix it, annotate the callee, or accept it with a scoped reason. + ### check_lizard Complexity gate: fail on **new** over-complex functions, not the ones already there. diff --git a/moondeck/check/check_clang_query.py b/moondeck/check/check_clang_query.py index 2bc3f715..1bcb55ea 100644 --- a/moondeck/check/check_clang_query.py +++ b/moondeck/check/check_clang_query.py @@ -115,10 +115,22 @@ # The type a `new` produces: `CXXNewExpr 0x... <...> 'Foo *' array ...`. _NEW_TYPE = re.compile(r"CXXNewExpr 0x\w+ <[^>]*> '(?P[^']+?) ?\*'") +# 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: +# +# 1. There is NO size-threshold matcher. `hasSize(N)` is exact-match and `sizeGreaterThan` +# does not exist, so every "> N bytes" decision happens in Python below, not in the AST. +# 2. `varDecl` alone silently misses EVERY class member — it needs `fieldDecl` beside it. +# The first version of this rule had only `varDecl` and reported a plausible, wrong list. +# 3. "Fixed number vs named constant" is NOT RECOVERABLE. The AST folds `kMaxLanes` to `[16]` +# before a matcher sees it: `busPinBuf_` reads as `uint16_t[16]` with no trace of the +# spelling. Only regex over source text could tell them apart, which is the fragile +# approach this whole script exists to replace. Do not try again. RULES = { "arrays": { "title": "RAM-costing fixed arrays", "output": "dump", + # constexpr and static-storage arrays are excluded: they live in flash and cost no RAM. "matcher": ("namedDecl(anyOf(" "varDecl(hasType(constantArrayType()), " "unless(anyOf(isConstexpr(), hasStaticStorageDuration()))), " diff --git a/moondeck/check/check_clang_tidy.py b/moondeck/check/check_clang_tidy.py index dd60599d..03d00e85 100644 --- a/moondeck/check/check_clang_tidy.py +++ b/moondeck/check/check_clang_tidy.py @@ -29,6 +29,9 @@ ROOT = Path(__file__).resolve().parent.parent.parent +# Third-party code that lives inside our tree. Findings here are upstream's to fix. +VENDORED = ("test/doctest.h",) + # A diagnostic line: path:line:col: severity: message [check-name] # # The trailing bracket can hold MORE than the check name: with WarningsAsErrors set, clang-tidy @@ -137,7 +140,10 @@ def run(build_dir, check_filter=None, tu_regex=None): d = m.groupdict() d["check"] = d["check"].split(",")[0] # drop the `,-warnings-as-errors` suffix # Our code only: a TU drags in SDK and vendored headers we neither own nor fix. - if not d["file"].startswith(("src/", "test/")): + # test/doctest.h is vendored too — it sits under test/ but is upstream's single-header + # release, so its findings (4 NewDeleteLeaks + a garbage-value read) are not ours to act + # on and would never reach zero. + if not d["file"].startswith(("src/", "test/")) or d["file"] in VENDORED: continue # A header analysed via N translation units yields N identical diagnostics. key = (d["file"], d["line"], d["col"], d["check"]) diff --git a/moondeck/check/check_hotpath.py b/moondeck/check/check_hotpath.py deleted file mode 100644 index da33809b..00000000 --- a/moondeck/check/check_hotpath.py +++ /dev/null @@ -1,170 +0,0 @@ -#!/usr/bin/env python3 -"""Flag hot-path discipline violations: allocation and blocking in the render path. - -The rule (CLAUDE.md § Principles, Minimalism; architecture.md § Hot path discipline): in -the render loop and everything it calls there is no heap allocation and no blocking. A -violation does not fail the build — it produces a frame hitch or, on a long-running ESP32, -fragmentation that degrades over hours. That makes it exactly the kind of defect a review -misses and a user reports as "it stutters after a day". - -This is a LINT, not a proof. It reads the text of the functions that make up the render -path and reports the banned constructs it can see: - - allocation new / malloc / push_back / std::string / make_unique / make_shared - blocking delay / sleep / mutex.lock (try_lock is the sanctioned form) - -What it cannot see: a call into a helper that allocates, a container that grows behind an -innocent-looking method, allocation inside a template instantiated elsewhere. So a clean -run means "no violation is visible in the render path's own source", not "the hot path is -allocation-free". Treat a finding as a question to answer, not an automatic bug. - -Usage: - uv run moondeck/check/check_hotpath.py # report findings, exit 1 if any - uv run moondeck/check/check_hotpath.py --list # list the scanned functions and exit -""" - -import argparse -import re -import sys -from pathlib import Path - -ROOT = Path(__file__).resolve().parent.parent.parent -SRC = ROOT / "src" - -# The methods that ARE the render path. `tick()` is the render loop itself; the periodic -# ticks share the same thread between two frames, so a heavy or allocating one steals from -# the render budget just as directly (architecture.md § Hot path discipline, sub-hot path). -HOT_METHODS = ("tick", "tick20ms", "tick1s") - -# One pattern per banned construct, with the reason the reader needs at the point of the -# finding — a bare "push_back found" teaches nothing. -BANNED = [ - (re.compile(r'\bnew\s+[A-Za-z_]'), "heap allocation (`new`)", - "allocate in setup()/prepare() and reuse the buffer"), - (re.compile(r'\bmalloc\s*\('), "heap allocation (`malloc`)", - "allocate in setup()/prepare() and reuse the buffer"), - (re.compile(r'\.push_back\s*\('), "heap allocation (`push_back` may grow)", - "pre-size the container in prepare(), or use a fixed array"), - (re.compile(r'\bstd::string\b'), "heap allocation (`std::string`)", - "use a fixed char buffer; std::string allocates on construction and on append"), - (re.compile(r'\bmake_unique\s*<|\bmake_shared\s*<'), "heap allocation (smart-pointer factory)", - "allocate in setup()/prepare()"), - (re.compile(r'\bdelay\s*\(|\bdelayMs\s*\('), "blocking (`delay`)", - "gate on millis() instead; blocking the render task shows as a visible glitch"), - (re.compile(r'\bsleep\s*\(|sleep_for\s*\('), "blocking (`sleep`)", - "gate on millis() instead"), - (re.compile(r'\.lock\s*\(\s*\)'), "blocking (`mutex.lock`)", - "use try_lock and skip the work this tick"), -] - -# A line carrying this marker is a deliberate, explained exception. The rule is the same -# one the codebase uses for a -Wno- suppression: the escape hatch exists, but it has to -# state its reason at the site, so a reviewer sees the justification rather than silence. -ALLOW_MARKER = "hot-path-ok:" - -# The desktop platform's own run loop is not the device's render path: it is host-side -# glue, free to allocate and block. (Only `src/` is scanned, so the test tree needs no -# entry here.) -SKIP_PARTS = {"platform/desktop"} - - -def hot_path_bodies(path, text): - """Yield (method_name, start_line, body_text) for each hot method defined in `text`. - - Brace-matched from the method's opening `{`, so a nested block or a lambda inside the - body stays part of it. Deliberately simple: this is a lint over well-formed project - source, not a C++ parser. - """ - for method in HOT_METHODS: - # `void tick() override {` / `void tick1s() {` — the definition, not a call. - for m in re.finditer(rf'\b(?:void|bool|int)\s+{method}\s*\([^)]*\)[^;{{]*\{{', text): - start = m.end() - 1 - depth, i = 0, start - while i < len(text): - if text[i] == '{': - depth += 1 - elif text[i] == '}': - depth -= 1 - if depth == 0: - break - i += 1 - body = text[start:i] - yield method, text[:start].count('\n') + 1, body - - -def scan_file(path): - findings = [] - rel = path.relative_to(ROOT).as_posix() - if any(part in rel for part in SKIP_PARTS): - return findings - - text = path.read_text(encoding="utf-8", errors="replace") - for method, method_line, body in hot_path_bodies(path, text): - for offset, line in enumerate(body.splitlines()): - stripped = line.strip() - # Skip comments and deliberate, explained exceptions. - if stripped.startswith("//") or stripped.startswith("*"): - continue - if ALLOW_MARKER in line: - continue - # Match against CODE only. A trailing comment ("// fade dead on new game") - # otherwise reads as an allocation — prose is full of the banned words, and a - # lint that cries wolf on comments gets muted, which costs the real findings. - code = line.split("//", 1)[0] - if not code.strip(): - continue - for pattern, what, fix in BANNED: - if pattern.search(code): - findings.append({ - "file": rel, - "line": method_line + offset, - "method": method, - "what": what, - "fix": fix, - "code": stripped[:100], - }) - break # one finding per line is enough to send the reader there - return findings - - -def main(): - parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) - parser.add_argument("--list", action="store_true", - help="List the hot-path methods that would be scanned, then exit.") - args = parser.parse_args() - - files = sorted(SRC.rglob("*.h")) + sorted(SRC.rglob("*.cpp")) - - if args.list: - for path in files: - rel = path.relative_to(ROOT).as_posix() - if any(part in rel for part in SKIP_PARTS): - continue - text = path.read_text(encoding="utf-8", errors="replace") - for method, line, _ in hot_path_bodies(path, text): - print(f"{rel}:{line} {method}()") - return 0 - - findings = [] - for path in files: - findings.extend(scan_file(path)) - - if not findings: - print("Hot-path check passed: no allocation or blocking visible in the render path.") - return 0 - - print(f"Hot-path findings ({len(findings)}):\n") - for f in findings: - print(f" {f['file']}:{f['line']} in {f['method']}()") - print(f" {f['what']}") - print(f" {f['code']}") - print(f" -> {f['fix']}\n") - - print(f"A finding is a question, not a verdict: this lint reads only the method's own " - f"source.\nIf a line is a justified exception, mark it `// {ALLOW_MARKER} ` " - f"so the reason lives at the site.") - return 1 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/moondeck/check/check_nonblocking.py b/moondeck/check/check_nonblocking.py new file mode 100644 index 00000000..44422867 --- /dev/null +++ b/moondeck/check/check_nonblocking.py @@ -0,0 +1,297 @@ +#!/usr/bin/env python3 +"""Hot-path discipline, checked by the compiler: what the render path calls that can block. + +`MoonModule::tick/tick20ms/tick1s` carry `MM_NONBLOCKING` (platform.h). Clang 20+ then verifies +under `-Wfunction-effects` that nothing they reach allocates or blocks — TRANSITIVELY, through +the whole call graph — the half a regex over the tick body cannot see, being blind to what its +callees do. + +Reports unique SITES. A header included by N translation units yields N copies of the same +warning, so a raw build prints ~1350 lines for ~175 real findings; deduplicating on +(file, line) is most of this script's value. + +Not a gate. `-Wno-error=function-effects` in CMakeLists keeps the build green while these are +triaged; each finding is a judgement — fix it, annotate the callee, or accept it with a scoped +reason — and the answer differs per site. + +Usage: + uv run moondeck/check/check_nonblocking.py # summary by callee, then every site + uv run moondeck/check/check_nonblocking.py --module AudioService +""" + +import argparse +import collections +import re +import subprocess +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent.parent + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import check_clang_query # noqa: E402 — the module→files resolver, one owner +import check_clang_tidy # noqa: E402 — build-dir resolution, one owner + +# `path:line:col: warning: [-Wfunction-effects]` +_WARN = re.compile(r"^(?P\S+?):(?P\d+):(?P\d+): warning: " + r"(?P.*?) \[-Wfunction-effects\]") +_CALLEE = re.compile(r"non-'nonblocking' function '(?P[^']+)'") + +# Clang follows each warning with a `note:` saying WHY the callee is not nonblocking — either it +# reaches another blocking function, or it has a construct that rules it out (a static local +# needs a guard variable and a lock). That note is the difference between "this line is flagged" +# and "here is what to fix", so it becomes the WHY column. +_NOTE = re.compile(r"note: function cannot be inferred 'nonblocking' because it " + r"(?:calls non-'nonblocking' function '(?P[^']+)'|(?P.+))") + +MAX_ROWS = 40 + +# Shown when clang gave no root cause (a leaf it could not look inside). Named rather than +# inlined: a backslash escape inside an f-string EXPRESSION is a syntax error before 3.12. +NO_CAUSE = "\u2014" + +# The known set, frozen — a READING AID, not a gate. Every finding stays in the report (a report +# shows the real situation); the baseline only marks which ones are NEW since it was taken, so +# ~50 known entries do not bury the one that appeared this week. +# +# Deliberately does not fail a build. A new blocking call may be entirely legitimate — a driver +# that genuinely must wait for hardware — and a hard failure would push someone to suppress it +# under time pressure, which is precisely what this report exists to prevent. The tools report; +# the human judges. +# +# Keyed on (file, callee) rather than line number, so the entry survives edits above it. Losing +# the line means two calls to the same function in one file collapse to one entry — acceptable: +# the question this answers is "is this a NEW kind of blocking call", not "how many". +BASELINE = ROOT / "docs" / "metrics" / "hotpath-baseline.txt" + + +def read_baseline(): + """The frozen (file, callee) pairs. Absent file = nothing frozen, everything is new.""" + if not BASELINE.exists(): + return set() + out = set() + for line in BASELINE.read_text(encoding="utf-8").splitlines(): + line = line.split("#")[0].strip() + if "\t" in line: + f, callee = line.split("\t", 1) + out.add((f.strip(), callee.strip())) + return out + + +def write_baseline(rows): + lines = [ + "# Hot-path baseline — the blocking calls reachable from tick/tick20ms/tick1s today.", + "#", + "# Generated by `uv run moondeck/check/check_nonblocking.py --baseline`. Format is", + "# `\\t`; `#` starts a comment. Keyed on file+callee, NOT line, so an entry", + "# survives edits above it.", + "#", + "# A line here means KNOWN AND BACKLOGGED, not acceptable — see backlog-core.md", + "# 'move blocking work off the render callbacks'. The list only shrinks: move the work", + "# off the render path, delete the line. Adding a line means admitting a new blocking", + "# call on the render path, which is what this exists to prevent.", + "", + ] + for r in sorted({(x["file"], x["callee"]) for x in rows}): + lines.append(f"{r[0]}\t{r[1]}") + BASELINE.write_text("\n".join(lines) + "\n", encoding="utf-8") + +# The enclosing method of a call site. Clang names the call and the callee but NOT the function +# they sit in, and that is the column that says which tick tier is affected — tick() every frame +# vs tick1s() once a second is an order of magnitude difference in what a blocking call costs. +# Walked backwards from the site to the nearest definition at class-member indent. +# Excludes control-flow keywords, which otherwise match `if (...) {` and report `IN: if`. +# Handles both in-class definitions and out-of-line `void Foo::tick() {`. +_KEYWORDS = ("if", "for", "while", "switch", "catch", "else", "do", "return") +_DEF = re.compile(r"^(?P\s*)(?:[\w:<>,&*\s]+?\s)?(?:(?P\w+)::)?(?P\w+)\s*" + r"\([^;]*\)\s*(?:const\s*)?(?:MM_NONBLOCKING\s*)?(?:override\s*)?" + r"(?:noexcept\s*)?\{") + +# Which tick tier a function belongs to, once resolved through the enclosing method. +TIERS = ("tick", "tick20ms", "tick1s") + + +def enclosing_function(rel_path, line, _cache={}): + """The method a call site sits in, or "" when it cannot be determined.""" + src = _cache.get(rel_path) + if src is None: + f = ROOT / rel_path + src = _cache[rel_path] = (f.read_text(encoding="utf-8", errors="replace").split("\n") + if f.exists() else []) + for i in range(min(line, len(src)) - 1, -1, -1): + m = _DEF.match(src[i]) + if m and len(m.group("indent")) <= 4 and m.group("name") not in _KEYWORDS: + return m.group("name") + return "" + + +def build_output(build_dir, clean=True): + """A full rebuild's warnings, or None if the build failed. + + `--clean-first` because an incremental build only recompiles what changed, and a cached TU + prints nothing — which would read as "no findings". Likewise a FAILED build produces partial + output: returning it would report a small number as if the tree were nearly clean, the same + silent-zero trap the other checks guard against. + """ + cmd = ["cmake", "--build", str(build_dir)] + (["--clean-first"] if clean else []) + proc = subprocess.run(cmd, cwd=ROOT, capture_output=True, text=True, check=False) + out = proc.stdout + proc.stderr + if proc.returncode != 0: + errors = [line for line in out.splitlines() if ": error:" in line] + print(f"Build FAILED (exit {proc.returncode}) — findings would be incomplete.", + file=sys.stderr) + for error_line in errors[:5]: + print(f" {error_line.replace(str(ROOT) + '/', '')}", file=sys.stderr) + return None + + # FAIL CLOSED. -Wfunction-effects exists only on Clang 20+; CMake silently omits the flag + # on anything else, and this script would then report "0 findings" from a build that never + # ran the check — indistinguishable from a clean tree. A zero is only trustworthy if the + # warning is actually enabled, so say so instead of reporting a comfortable number. + if clean and "[-Wfunction-effects]" not in out: + print("No -Wfunction-effects diagnostics in the build output.\n" + "That means either the tree really is clean, or the compiler does not support the\n" + "warning (it needs Clang 20+; CMake omits it silently otherwise). Check with:\n" + f" grep -n 'Wfunction-effects' {(ROOT / 'CMakeLists.txt').relative_to(ROOT)}\n" + " cmake --build --clean-first 2>&1 | grep -c function-effects", + file=sys.stderr) + return None + return out + + +def collect(out): + """Unique findings keyed on (file, line, col), each with the root cause clang gives. + + A warning line names the call; the `note:` line that follows names why that callee blocks. + Both are needed to act on a finding, so they are carried together. + """ + rows, last = {}, None + for line in out.splitlines(): + clean = line.replace(str(ROOT) + "/", "") + m = _WARN.match(clean) + if m: + f = m["file"] + last = None + if f.startswith(("src/", "test/")): + c = _CALLEE.search(m["msg"]) + key = (f, int(m["line"]), int(m["col"])) + # Two violation kinds: a call to something that blocks, or a construct that + # blocks by itself (a static local needs a guard variable and a one-time lock). + # The second has no callee, so name the construct instead of truncating the + # warning text into the CALLS column. + rows.setdefault(key, { + "file": f, "line": int(m["line"]), + "callee": c["name"] if c else "(static local variable)", + "why": "" if c else "guard variable + one-time lock on first use", + "fn": enclosing_function(f, int(m["line"])), + }) + last = key + continue + # The note belongs to the warning just above it. + if last: + n = _NOTE.search(clean) + if n and not rows[last]["why"]: + rows[last]["why"] = (f"calls {n['via']}" if n["via"] + else n["reason"].strip().rstrip(".")) + return sorted(rows.values(), key=lambda r: (r["file"], r["line"])) + + +def main(): + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("--module", help="Only findings in this module's source files.") + ap.add_argument("--baseline", action="store_true", + help="Rewrite the frozen set from today's findings.") + ap.add_argument("--incremental", action="store_true", + help="Skip the clean rebuild — only files that changed are recompiled, so " + "this reports on THOSE. For a gate that asks 'did this change add a " + "blocking call', not for the full picture.") + args = ap.parse_args() + + build_dir = check_clang_tidy._host_build_dir() + if not (build_dir / "CMakeCache.txt").exists(): + print(f"No build in {build_dir.relative_to(ROOT)} — run " + f"`uv run moondeck/build/build_desktop.py` first.", file=sys.stderr) + return 2 + + out = build_output(build_dir, clean=not args.incremental) + if out is None: + return 2 + rows = collect(out) + + if args.baseline: + write_baseline(rows) + print(f"Baseline written: {len({(r['file'], r['callee']) for r in rows})} " + f"(file, callee) pairs frozen in {BASELINE.relative_to(ROOT)}") + return 0 + + # Mark what is new since the baseline. Not a filter — everything is still shown. + # Existence, not emptiness: an EMPTY baseline is a deliberate "nothing is known-good", so + # every finding is new. Only a MISSING file means "no baseline taken yet". + has_baseline = BASELINE.exists() + known = read_baseline() + for r in rows: + r["new"] = has_baseline and (r["file"], r["callee"]) not in known + + if args.module: + only = check_clang_query.module_files(args.module) + if not only: + print(f"No source files for module '{args.module}'.", file=sys.stderr) + return 2 + print(f"Filtered to {args.module}: {', '.join(only)}\n") + rows = [r for r in rows if r["file"] in only] + + n_new = sum(1 for r in rows if r.get("new")) + print(f"{len(rows)} call(s) from the render path that can block or allocate.") + if has_baseline: + print(f"{n_new} NEW since the baseline ({len(known)} known, backlogged as architecture " + f"work)." if n_new else + f"None new since the baseline — all {len(known)} are known and backlogged.") + else: + print("No baseline yet: run with --baseline to freeze today's set, so a future run can " + "show what is new.") + if not rows: + print("\nNone. \u2713 (If that seems wrong, confirm the build actually recompiled \u2014 a " + "cached TU prints no warnings.)") + return 0 + + # Split by tick tier: tick() runs every frame, tick1s() once a second, so the same blocking + # call costs roughly two orders of magnitude more in one than the other (50/s vs 1/s). + # Pooling them hides that. + def tier_of(r): + return r["fn"] if r["fn"] in TIERS else "other" + + def table(title, subset, note): + if not subset: + return + callee_w = min(max(len(r["callee"]) for r in subset), 36) + fn_w = min(max(len(r["fn"] or "?") for r in subset), 18) + why_w = min(max((len(r["why"]) for r in subset), default=0), 44) or 1 + loc_w = min(max(len(f"{r['file']}:{r['line']}") for r in subset), 44) + + def clip(s, w): + return s if len(s) <= w else s[: w - 1] + "\u2026" + + print(f"\n{title} \u2014 {len(subset)} ({note})") + print(f" {'CALLS':<{callee_w}} {'IN':<{fn_w}} {'WHY IT BLOCKS':<{why_w}} FILE:LINE") + print(f" {'-' * callee_w} {'-' * fn_w} {'-' * why_w} {'-' * loc_w}") + for r in subset[:MAX_ROWS]: + why = r["why"] or NO_CAUSE + mark = "NEW " if r.get("new") else " " + print(f" {mark}{clip(r['callee'], callee_w):<{callee_w}} " + f"{clip(r['fn'] or '?', fn_w):<{fn_w}} " + f"{clip(why, why_w):<{why_w}} " + f"{r['file']}:{r['line']}") + if len(subset) > MAX_ROWS: + print(f" \u2026 {len(subset) - MAX_ROWS} more. Use --module to scope.") + + for tier, note in (("tick", "every frame \u2014 the hot path"), + ("tick20ms", "50x a second"), + ("tick1s", "once a second \u2014 sub-hot path"), + ("other", "reached from a tick, tier not resolved")): + table(tier + "()" if tier != "other" else "OTHER", + [r for r in rows if tier_of(r) == tier], note) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/moondeck/check/repo_health.py b/moondeck/check/repo_health.py index 96f70197..ae1db237 100644 --- a/moondeck/check/repo_health.py +++ b/moondeck/check/repo_health.py @@ -154,7 +154,7 @@ def measure_tests(): def measure_complexity(): - """Complexity, the number lizard owns (plan § one rule, one owner). + """Complexity, the number lizard owns (docs/testing.md § Static analysis). Deliberately the RAW count, not the baselined one: the gate (check_lizard.py) subtracts whitelizard.txt so it fails only on new violations, but the TREND has to see the whole diff --git a/moondeck/event/_gates.py b/moondeck/event/_gates.py index b5144daa..7209b64e 100644 --- a/moondeck/event/_gates.py +++ b/moondeck/event/_gates.py @@ -136,9 +136,14 @@ def when(*prefixes, exclude=()): when(*COMPILES_DESKTOP, "test/scenarios/")), Gate("platform boundary", UV + ["moondeck/check/check_platform_boundary.py"], when("src/", exclude=("src/platform/",))), - # A lint, not a proof: it sees allocation/blocking written directly in a hot - # method, not what a callee does. A finding is a question to answer. - Gate("hot-path discipline", UV + ["moondeck/check/check_hotpath.py"], + # Reports what the compiler proved about THIS change: -Wfunction-effects checks the + # render path transitively, and `--incremental` restricts the rebuild to what the commit + # touched, so the gate answers "did this add a blocking call" in ~1s rather than + # re-reporting the whole 107-entry baseline. It never fails the event — a new blocking + # call may be legitimate (a driver that must wait for hardware), so this states the + # finding and the product owner judges it. Full picture: the clang-hotpath card. + Gate("hot-path discipline", + UV + ["moondeck/check/check_nonblocking.py", "--incremental"], when("src/")), ] diff --git a/moondeck/moondeck.py b/moondeck/moondeck.py index 4e1ac195..31175e50 100644 --- a/moondeck/moondeck.py +++ b/moondeck/moondeck.py @@ -10,11 +10,23 @@ import tempfile import threading from contextlib import suppress +from datetime import datetime from pathlib import Path PORT = 8420 ROOT = Path(__file__).resolve().parent.parent SCRIPTS_DIR = Path(__file__).resolve().parent + +# Last run's output per script, so a card can answer "what did this do last time" after the +# stream is gone — a page reload, a different tab, or simply coming back later. Under build/ +# because it is derived state, not repo state (build/ is gitignored). One file per script id, +# overwritten each run: this is a "last run" record, not a history. +LOG_DIR = ROOT / "build" / "moondeck-logs" +# Cap per log. An ESP32 build or a docs screenshot run can print megabytes, and this is a +# convenience record, not an archive. The TAIL is what matters (the result, the exit code), +# so once the cap is hit the writer stops and says so — truncating the end would throw away +# the part you came for. +LOG_MAX_BYTES = 1_000_000 UI_DIR = SCRIPTS_DIR / "moondeck_ui" ASSETS_DIR = ROOT / "docs" / "assets" STATE_FILE = SCRIPTS_DIR / "moondeck.json" @@ -1299,7 +1311,14 @@ def _read_body(self) -> bytes: def do_GET(self): if self.path == "/api/scripts": - self._send_json({"scripts": SCRIPTS, "firmwares": FIRMWARES}) + # Which scripts have a stored last run, so the UI shows the 📄 button only where + # there is something to show. The absence is informative too: a card with no 📄 is + # one nobody has run yet. + have_logs = set() + if LOG_DIR.exists(): + have_logs = {f.stem for f in LOG_DIR.glob("*.log")} + scripts = [{**s, "hasLog": s["id"] in have_logs} for s in SCRIPTS] + self._send_json({"scripts": scripts, "firmwares": FIRMWARES}) elif self.path == "/api/ports": # Enrich each port with chip family + specific board (levels 2/3), @@ -1356,6 +1375,9 @@ def do_GET(self): script_id = self.path.split("/")[-1] self._handle_stream(script_id) + elif self.path.startswith("/api/log/"): + self._serve_log(self.path.split("/")[-1]) + elif self.path.startswith("/api/help"): self._serve_help() @@ -1777,6 +1799,19 @@ def _handle_stream(self, script_id: str): # master read raises OSError (EIO) once the child exits and closes the slave; treat that as # EOF, same as the pipe's b"" sentinel. stream = getattr(proc, "_mm_read_stream", None) or proc.stdout + + # Tee to disk as we stream, rather than buffering and writing at the end: a long run + # killed with Stop (or a crashed server) still leaves everything it printed up to + # that point, which is exactly when you want the log. + log = None + log_bytes = 0 + with suppress(OSError): + LOG_DIR.mkdir(parents=True, exist_ok=True) + log = (LOG_DIR / f"{script_id}.log").open("w", encoding="utf-8") + # Timezone-aware: a bare local timestamp is ambiguous when the log is read from + # another machine or after a DST change. + log.write(f"# {script_id} — {datetime.now().astimezone().isoformat(timespec='seconds')}\n") + try: while True: try: @@ -1786,11 +1821,28 @@ def _handle_stream(self, script_id: str): if not line: break # pipe EOF text = line.decode("utf-8", errors="replace").rstrip("\r\n") + if log: + # Every write is best-effort: a full disk or a removed build/ must not kill + # the stream the user is watching. + try: + if log_bytes < LOG_MAX_BYTES: + log_bytes += log.write(text + "\n") + log.flush() + if log_bytes >= LOG_MAX_BYTES: + log.write(f"\n[log truncated at {LOG_MAX_BYTES} bytes]\n") + log.flush() + except OSError: + with suppress(OSError): + log.close() + log = None self.wfile.write(f"data: {json.dumps(text)}\n\n".encode()) self.wfile.flush() proc.wait() exit_msg = f"[exit code: {proc.returncode}]" + if log: + with suppress(OSError): + log.write(exit_msg + "\n") # always recorded, even past the cap self.wfile.write(f"data: {json.dumps(exit_msg)}\n\n".encode()) done_data = json.dumps({"exitCode": proc.returncode}) self.wfile.write(f"event: done\ndata: {done_data}\n\n".encode()) @@ -1798,6 +1850,9 @@ def _handle_stream(self, script_id: str): except (BrokenPipeError, ConnectionResetError): pass finally: + if log: + with suppress(OSError): + log.close() # Close the pty master fd (Unix) so the kernel reclaims it; the pipe closes with proc. master_fd = getattr(proc, "_mm_master_fd", None) if master_fd is not None: @@ -2021,6 +2076,29 @@ def _serve_scenario_steps(self): self.end_headers() self.wfile.write(data_bytes) + def _serve_log(self, script_id: str) -> None: + """The last run's output for one script, as plain text. + + 404 when a script has not run since the server had a log dir — the UI treats that as + "no previous run" rather than an error. Reads at most LOG_MAX_BYTES: the writer caps + too, but a log from an older build could predate that cap. + """ + import re as _re + if not _re.fullmatch(r"[A-Za-z0-9_-]+", script_id or ""): + self.send_error(400, "bad script id") + return + path = LOG_DIR / f"{script_id}.log" + if not path.exists(): + self.send_error(404, "no log for this script yet") + return + with path.open("rb") as f: + body = f.read(LOG_MAX_BYTES) + self.send_response(200) + self.send_header("Content-Type", "text/plain; charset=utf-8") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + def _serve_help(self): """Serve MoonDeck.md as styled HTML with deep-link anchor support.""" md_path = SCRIPTS_DIR / "MoonDeck.md" diff --git a/moondeck/moondeck_config.json b/moondeck/moondeck_config.json index c1a9796e..ac85676e 100644 --- a/moondeck/moondeck_config.json +++ b/moondeck/moondeck_config.json @@ -134,15 +134,6 @@ "help": "history_report", "script": "report/history_report.py" }, - { - "id": "check_hotpath", - "tab": "desktop", - "group": "check", - "label": "Hot Path", - "speed": "instant", - "help": "check_hotpath", - "script": "check/check_hotpath.py" - }, { "id": "check_esp32_built", "tab": "esp32", @@ -209,6 +200,15 @@ "help": "check_clang_query", "script": "check/check_clang_query.py" }, + { + "id": "check_nonblocking", + "tab": "desktop", + "group": "tools", + "label": "clang-hotpath", + "speed": "slow", + "help": "check_nonblocking", + "script": "check/check_nonblocking.py" + }, { "id": "check_lizard", "tab": "desktop", diff --git a/moondeck/moondeck_ui/app.js b/moondeck/moondeck_ui/app.js index 460834ea..180a4461 100644 --- a/moondeck/moondeck_ui/app.js +++ b/moondeck/moondeck_ui/app.js @@ -240,6 +240,29 @@ const SPEED_BADGE = { slow: { icon: "🐌", title: "Slow — takes more than 30 seconds" }, }; +// Replay a script's stored last run into the log pane. Shared by the button rendered at load +// and the one created when a first run completes. +let logLoadToken = 0; + +async function showLastRun(script) { + switchPane("log"); + // Clear BEFORE the fetch, and token the request: clicking two cards quickly must not let + // the slower response paint over the log you actually asked for last. + const token = ++logLoadToken; + logEl.textContent = ""; + appendLog(`— loading last run for ${script.label} —`); + try { + const r = await fetch(`/api/log/${script.id}`); + if (token !== logLoadToken) return; // superseded by a later click + logEl.textContent = ""; + appendLog(r.ok ? await r.text() : `— no stored run for ${script.label} —`); + } catch (err) { + if (token !== logLoadToken) return; + logEl.textContent = ""; + appendLog(`— could not read log: ${err} —`); + } +} + function speedBadge(speed) { const b = SPEED_BADGE[speed]; return b ? `${b.icon}` : ""; @@ -377,6 +400,7 @@ function renderScripts() { ${script.label} ${speedBadge(script.speed)} + ${script.hasLog ? `` : ""} @@ -489,6 +513,13 @@ function renderScripts() { runScript(script, e.target); }); + // Its own button rather than a click on the status dot: the dot is a status + // INDICATOR, and making it secretly clickable gave a new reader no way to guess the + // feature existed. The server tees every stream to build/moondeck-logs/.log, so + // this answers "what did this do last time" after a page reload or a switch to + // another card — the case a live-only stream cannot. + card.querySelector(".log-btn")?.addEventListener("click", () => showLastRun(script)); + target.appendChild(card); } } @@ -600,6 +631,22 @@ async function runScriptOnce(script, btn, extraParams) { // instead of waiting up to 5s for the poll, so the button // flips back to "Stop" without a visible blink. if (script.long_running) updateRunningState(); + // A first run just created this script's log, so the 📄 appears without + // needing a page reload. + if (!script.hasLog) { + script.hasLog = true; + const row = document.querySelector( + `.status-dot[data-id="${script.id}"]`)?.parentElement; + if (row && !row.querySelector(".log-btn")) { + const b = document.createElement("button"); + b.className = "log-btn"; + b.title = "Show this script's last run"; + b.setAttribute("aria-label", "Show this script's last run"); + b.textContent = "📄"; + b.addEventListener("click", () => showLastRun(script)); + row.insertBefore(b, row.querySelector(".help-btn")); + } + } resolve(data.exitCode === 0); } catch { resetBtn(1); diff --git a/moondeck/moondeck_ui/style.css b/moondeck/moondeck_ui/style.css index cf19ef2b..9a5b498d 100644 --- a/moondeck/moondeck_ui/style.css +++ b/moondeck/moondeck_ui/style.css @@ -196,6 +196,20 @@ select { cursor: default; } +/* Replays this script's last run. Same shape as the `?` next to it — an emoji alone would be + ambiguous, but a button with a tooltip in a row that already has one reads as a control. */ +.script-card .log-btn { + background: transparent; + border: none; + cursor: pointer; + font-size: 11px; + line-height: 1; + padding: 1px 4px; + border-radius: 3px; + opacity: 0.6; +} +.script-card .log-btn:hover { opacity: 1; background: #0f3460; } + .script-card .help-btn { background: transparent; border: none; diff --git a/src/core/AudioService.h b/src/core/AudioService.h index 2b588076..35eb05cb 100644 --- a/src/core/AudioService.h +++ b/src/core/AudioService.h @@ -89,7 +89,7 @@ class AudioService : public MoonModule { static constexpr size_t kBlock = 512; static constexpr size_t kMag = kBlock / 2; ///< real-FFT magnitude bins - ModuleRole role() const override { return ModuleRole::Service; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Service; } /// Unlike a zero-cost diagnostic peripheral, this module pays a @@ -301,13 +301,17 @@ class AudioService : public MoonModule { /// `tick()` — so a device with two mics reads the first consistently, and /// removing the active one lets a survivor take over. Add/remove in any order /// leaves a coherent answer (the robustness rule). - static const AudioFrame* latestFrame() { - static const AudioFrame kSilence{}; + static const AudioFrame* latestFrame() MM_NONBLOCKING { + // constexpr, not a function-local static: a static needs a guard variable and a + // one-time lock on first use, which is a blocking operation on the render path — and + // this is called from EIGHT audio-reactive effects' tick(). constexpr is initialized at + // compile time, so there is no guard and no lock. + static constexpr AudioFrame kSilence{}; AudioService* a = ActiveInstance::active(); return a ? &a->frame_ : &kSilence; } - void tick() override { + void tick() MM_NONBLOCKING override { // Self-elect as the active mic if the seat is empty. prepare() gives it to the first live // module and release() vacates it, but removing the active module while a second one is still running // would otherwise leave the seat empty (effects go silent). A running module re-claiming an @@ -441,7 +445,7 @@ class AudioService : public MoonModule { if (frame_.level > levelPeak_) levelPeak_ = static_cast(frame_.level); } - void tick1s() override { + void tick1s() MM_NONBLOCKING override { std::snprintf(levelStr_, sizeof(levelStr_), "%u", static_cast(levelPeak_)); std::snprintf(peakStr_, sizeof(peakStr_), "%u Hz", static_cast(frame_.peakHz)); levelPeak_ = 0; // reset for the next window diff --git a/src/core/DevicesModule.h b/src/core/DevicesModule.h index b29e20f1..2ed4445c 100644 --- a/src/core/DevicesModule.h +++ b/src/core/DevicesModule.h @@ -253,7 +253,7 @@ class DevicesModule : public MoonModule, public ListSource { /// broadcast our own presence on a slow cadence, and age out devices unheard for /// kStaleMs. The drain is non-blocking (recvFrom returns -1 when nothing pending), so it /// never stalls the tick — the hot-path-safe replacement for the old mDNS query. - void tick1s() override { + void tick1s() MM_NONBLOCKING override { MoonModule::tick1s(); uint8_t local[4] = {}; localIp(local); @@ -286,7 +286,7 @@ class DevicesModule : public MoonModule, public ListSource { ageOut(local); } - ModuleRole role() const override { return ModuleRole::Generic; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Generic; } /// Test seam: feed a synthetic presence datagram through the real classify→upsert /// pipeline, exactly as the live recvFrom loop does. The desktop unit/scenario tests diff --git a/src/core/FileManagerModule.cpp b/src/core/FileManagerModule.cpp index f5ba7a23..45438cb9 100644 --- a/src/core/FileManagerModule.cpp +++ b/src/core/FileManagerModule.cpp @@ -41,7 +41,7 @@ void FileManagerModule::defineControls() { MoonModule::defineControls(); } -void FileManagerModule::tick1s() { +void FileManagerModule::tick1s() MM_NONBLOCKING { if (totalBytes_ > 0) usedBytes_ = static_cast(platform::filesystemUsed()); } diff --git a/src/core/FileManagerModule.h b/src/core/FileManagerModule.h index b703b1ee..2cf45f61 100644 --- a/src/core/FileManagerModule.h +++ b/src/core/FileManagerModule.h @@ -45,7 +45,7 @@ class FileManagerModule : public MoonModule { public: void defineControls() override; void setup() override; - void tick1s() override; + void tick1s() MM_NONBLOCKING override; private: bool showHidden_ = false; // reveal dot-prefixed entries (forwarded to /api/dir by the UI) diff --git a/src/core/FilesystemModule.cpp b/src/core/FilesystemModule.cpp index 01fc1f8f..c54ae562 100644 --- a/src/core/FilesystemModule.cpp +++ b/src/core/FilesystemModule.cpp @@ -47,7 +47,7 @@ void FilesystemModule::setup() { // The filesystem-usage gauge likewise lives on FileManagerModule (that's where filesystem state // is topical). -void FilesystemModule::tick1s() { +void FilesystemModule::tick1s() MM_NONBLOCKING { if (!mounted_ || !scheduler_) return; updateLastSavedStr(); if (!dirtyPending_) return; diff --git a/src/core/FilesystemModule.h b/src/core/FilesystemModule.h index bb58c129..a4c80f19 100644 --- a/src/core/FilesystemModule.h +++ b/src/core/FilesystemModule.h @@ -96,7 +96,7 @@ class FilesystemModule : public MoonModule { /// Persistence must keep flushing dirty subtrees regardless of the `enabled` toggle — /// otherwise the user could lose changes by accidentally disabling this module via /// the UI before the 2s debounce expires. - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } /// Non-UI: a pure persistence engine with no controls — not shown as a card (its "last saved" /// status is displayed by FileManagerModule). See MoonModule::appearsInUi. @@ -104,7 +104,7 @@ class FilesystemModule : public MoonModule { void setScheduler(Scheduler* s); void setup() override; - void tick1s() override; + void tick1s() MM_NONBLOCKING override; /// The engine's live "last saved" buffer — "never" before the first save, else "5m ago". This /// is the persistence engine's status; FileManagerModule binds its read-only "lastSaved" control diff --git a/src/core/FirmwareUpdateModule.h b/src/core/FirmwareUpdateModule.h index ab398d4f..4e31be9c 100644 --- a/src/core/FirmwareUpdateModule.h +++ b/src/core/FirmwareUpdateModule.h @@ -101,7 +101,7 @@ class FirmwareUpdateModule : public MoonModule { /// Diagnostics keep ticking regardless of the user toggle; matches /// SystemModule + NetworkModule. The user can't easily re-enable a /// disabled diagnostic module without it being visible. - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } void setup() override { // Copy the file-scope globals into the bound buffers on boot so the @@ -152,7 +152,7 @@ class FirmwareUpdateModule : public MoonModule { controls_.addProgress("update_pct", bytesRead_, totalSnap_); } - void tick1s() override { + void tick1s() MM_NONBLOCKING override { // Poll the OTA task's progress + status. No locks: the writer is // a single task, reads are atomic at this granularity, and a torn // read shows as a brief mid-update glimpse — visually harmless. diff --git a/src/core/HttpServerModule.cpp b/src/core/HttpServerModule.cpp index 4e0ba017..b5e1371e 100644 --- a/src/core/HttpServerModule.cpp +++ b/src/core/HttpServerModule.cpp @@ -64,7 +64,7 @@ void HttpServerModule::release() { MoonModule::release(); // chain: uniform override-and-chain (no buffers/children today, but the convention holds) } -void HttpServerModule::tick20ms() { +void HttpServerModule::tick20ms() MM_NONBLOCKING { // Drain the in-flight resumable preview frame on the TRANSPORT-poll cadence (20 ms), NOT the // per-render-tick tick(): pushing frame bytes to the socket must not be charged to the LED // render hot path. The render tick stays free of preview work; the preview frame rate is @@ -108,7 +108,7 @@ void HttpServerModule::tick20ms() { } } -void HttpServerModule::tick1s() { +void HttpServerModule::tick1s() MM_NONBLOCKING { pushStateToWebSockets(); } diff --git a/src/core/HttpServerModule.h b/src/core/HttpServerModule.h index 2442ad35..00099144 100644 --- a/src/core/HttpServerModule.h +++ b/src/core/HttpServerModule.h @@ -163,7 +163,7 @@ class HttpServerModule : public MoonModule, public BinaryBroadcaster { /// Keep running even when "disabled" via the UI — otherwise the user has no way /// to re-enable themselves through the same UI. - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } /// Non-UI: this IS the server that renders /api/state — it doesn't list itself as a card. /// The "not a UI module" opt-out (shared with FilesystemModule), read by the state serializer's @@ -173,8 +173,8 @@ class HttpServerModule : public MoonModule, public BinaryBroadcaster { void defineControls() override; void setup() override; void release() override; - void tick20ms() override; - void tick1s() override; + void tick20ms() MM_NONBLOCKING override; + void tick1s() MM_NONBLOCKING override; // ----------------------------------------------------------------------- // Transport-free apply-core — "the REST API, callable in-process" diff --git a/src/core/ImprovProvisioningModule.h b/src/core/ImprovProvisioningModule.h index d29f4785..2bc715cc 100644 --- a/src/core/ImprovProvisioningModule.h +++ b/src/core/ImprovProvisioningModule.h @@ -101,7 +101,7 @@ class ImprovProvisioningModule : public MoonModule { void setHttpServerModule(HttpServerModule* h) { httpServerModule_ = h; } /// Diagnostics keep ticking; matches FirmwareUpdateModule / SystemModule. - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } /// Apparatus, not swappable content — provisioning is a fixed device service. /// Not deletable (matches Board / Preview); can still be disabled. @@ -139,7 +139,7 @@ class ImprovProvisioningModule : public MoonModule { /// writes are visible before we read them). The TX-power cap is applied first on purpose (see /// the inline note), and the deviceModel arrives like any other catalog default via the /// APPLY_OP poll below rather than here. - void tick1s() override { + void tick1s() MM_NONBLOCKING override { // Vendor SET_TX_POWER RPC — handled BEFORE the credentials on purpose: // when an installer sends the cap and the credentials back-to-back, // both flags can land within one tick, and the cap must be persisted @@ -174,7 +174,7 @@ class ImprovProvisioningModule : public MoonModule { /// mutation off the serial task — the same discipline the credentials/deviceModel paths /// follow. A failed op can't travel back on the already-spent frame ack, so it's surfaced /// over serial + in provision_status rather than looking like a clean install. - void tick() override { + void tick() MM_NONBLOCKING override { if (pendingOpReady_.load(std::memory_order_acquire) && httpServerModule_) { // The Improv task already acked frame RECEIPT; the op is APPLIED here. A // failed op (UnknownType, OutOfRange, a not-found target) can't travel back diff --git a/src/core/IrService.h b/src/core/IrService.h index 645afd11..30ba4095 100644 --- a/src/core/IrService.h +++ b/src/core/IrService.h @@ -50,7 +50,7 @@ namespace mm { /// @card IrService.png class IrService : public MoonModule { public: - ModuleRole role() const override { return ModuleRole::Service; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Service; } void defineControls() override { controls_.addPin("pin", pin_); @@ -97,7 +97,7 @@ class IrService : public MoonModule { } } - void tick() override { + void tick() MM_NONBLOCKING override { if (pin_ < 0) return; uint32_t code = 0; if (platform::irRead(static_cast(pin_), code)) processCode(code); diff --git a/src/core/MoonModule.h b/src/core/MoonModule.h index 06ceb9d0..c06dc30c 100644 --- a/src/core/MoonModule.h +++ b/src/core/MoonModule.h @@ -118,9 +118,9 @@ class MoonModule { /// reverse-iterates children; override and chain late so the parent shuts down its /// own state first. virtual void setup() { for (uint8_t i = 0; i < childCount_; i++) children_[i]->setup(); } - virtual void tick() { tickChildren(&MoonModule::tick); } - virtual void tick20ms() { tickChildren(&MoonModule::tick20ms); } - virtual void tick1s() { tickChildren(&MoonModule::tick1s); } + virtual void tick() MM_NONBLOCKING { tickChildren(&MoonModule::tick); } + virtual void tick20ms() MM_NONBLOCKING { tickChildren(&MoonModule::tick20ms); } + virtual void tick1s() MM_NONBLOCKING { tickChildren(&MoonModule::tick1s); } virtual void release() { // release() frees ALL of a module's held resources on disable, not only buffers: a driver's // GPIO/RMT/Parlio pins, a service's I²S mic, an effect's sockets are freed by that module's @@ -359,7 +359,7 @@ class MoonModule { const char* typeName() const { return typeName_; } void setTypeName(const char* tn) { typeName_ = tn ? tn : ""; } - bool enabled() const { return enabled_; } + bool enabled() const MM_NONBLOCKING { return enabled_; } void setEnabled(bool e) { if (enabled_ == e) return; enabled_ = e; @@ -370,7 +370,7 @@ class MoonModule { /// Default true — disabled modules don't have their loop fns called. Override to /// return false for system modules that must keep running regardless (HttpServer, /// Network, Filesystem) so the user can re-enable other modules through them. - virtual bool respectsEnabled() const { return true; } + virtual bool respectsEnabled() const MM_NONBLOCKING { return true; } /// True unless this module — or an ancestor that respects the enabled flag — is disabled. /// The single predicate the resource-lifecycle gate keys off: `prepare()` acquires @@ -450,7 +450,7 @@ class MoonModule { } /// Role for type identification (no RTTI needed). - virtual ModuleRole role() const { return ModuleRole::Generic; } + virtual ModuleRole role() const MM_NONBLOCKING { return ModuleRole::Generic; } /// Curated emoji tags for the module picker's chip filter — extras beyond the /// role chip (which the UI derives from role() on its own). A short string of @@ -648,7 +648,7 @@ class MoonModule { /// Per-module timing: parents time children, Scheduler times top-level modules. /// tickTimeUs() is the average microseconds per tick over the last 1-second window. uint32_t tickTimeUs() const { return tickTimeUs_; } - void addAccumUs(uint32_t us) { accumUs_ += us; } + void addAccumUs(uint32_t us) MM_NONBLOCKING { accumUs_ += us; } /// Called by Scheduler every ~1 second. Averages the accumulated tick time and recurses /// into children. @@ -676,8 +676,12 @@ class MoonModule { /// worker while the rest tick on the render core) must not re-implement the gate and the timing /// per side — that rule is core's, not the module's (CLAUDE.md § Complexity lives in core). enum class RoleFilter : uint8_t { All, Only, Except }; - void tickChildren(void (MoonModule::*fn)(), RoleFilter filter = RoleFilter::All, - ModuleRole role = ModuleRole::Generic) { + // The attribute is part of the POINTER TYPE, not decoration: without it clang cannot know + // that `fn` only ever holds one of the three annotated ticks, and the indirect call becomes + // the one hole in the transitive check. With it, passing an unannotated method here is a + // compile error at the call site. + void tickChildren(void (MoonModule::*fn)() MM_NONBLOCKING, RoleFilter filter = RoleFilter::All, + ModuleRole role = ModuleRole::Generic) MM_NONBLOCKING { for (uint8_t i = 0; i < childCount_; i++) { MoonModule* c = children_[i]; if (filter == RoleFilter::Only && c->role() != role) continue; diff --git a/src/core/MqttModule.cpp b/src/core/MqttModule.cpp index 0cce537b..93e0c002 100644 --- a/src/core/MqttModule.cpp +++ b/src/core/MqttModule.cpp @@ -411,7 +411,7 @@ void MqttModule::resetConnection(const char* status) { setStatusLine(status); } -void MqttModule::tick1s() { +void MqttModule::tick1s() MM_NONBLOCKING { if constexpr (!platform::hasNetwork) { MoonModule::tick1s(); return; } if (!enabled() || broker_[0] == '\0') { diff --git a/src/core/MqttModule.h b/src/core/MqttModule.h index 2b66a701..e7df4620 100644 --- a/src/core/MqttModule.h +++ b/src/core/MqttModule.h @@ -80,7 +80,7 @@ class MqttModule : public MoonModule { void defineControls() override; void onControlChanged(const char* controlName) override; // a broker/port/cred change re-homes the socket void onEnabled(bool enabled) override; // enable/disable → connect / clean DISCONNECT - void tick1s() override; + void tick1s() MM_NONBLOCKING override; /// Feed inbound bytes as if they arrived from the broker socket — the entry the host unit tests /// drive (there's no live broker in ctest). Mirrors IrService::injectCodeForTest. diff --git a/src/core/NetworkModule.h b/src/core/NetworkModule.h index 2632965a..4b8e6019 100644 --- a/src/core/NetworkModule.h +++ b/src/core/NetworkModule.h @@ -201,7 +201,7 @@ class NetworkModule : public MoonModule { /// Networking is infrastructure — keep the cascade ticking even when the user /// toggled "enabled" off, otherwise the device would silently drop off the LAN /// and become unreachable. - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } void setup() override { // Push the DHCP hostname (option 12) before any bring-up so the device shows @@ -377,7 +377,7 @@ class NetworkModule : public MoonModule { // Chain to base is at the top of this method — see comment there. } - void tick1s() override { + void tick1s() MM_NONBLOCKING override { uint32_t now = platform::millis(); uint32_t elapsed = now - stateChangeTime_; diff --git a/src/core/PinsModule.h b/src/core/PinsModule.h index 5f0dc1ce..4085d42e 100644 --- a/src/core/PinsModule.h +++ b/src/core/PinsModule.h @@ -58,7 +58,7 @@ class PinsModule : public MoonModule { /// owner name + role into its own storage, so the rows serialize safely even if the owning module is /// deleted between refreshes — the snapshot holds no pointers into module memory. The next refresh /// rebuilds from the live tree, so a deleted module's claim drops within a second. - void tick1s() override { + void tick1s() MM_NONBLOCKING override { MoonModule::tick1s(); pins_.refresh(); } diff --git a/src/core/Scheduler.cpp b/src/core/Scheduler.cpp index aea629cb..4e88fcb8 100644 --- a/src/core/Scheduler.cpp +++ b/src/core/Scheduler.cpp @@ -63,7 +63,7 @@ void Scheduler::setup() { lastTimingUpdate_ = platform::millis(); } -void Scheduler::tick() { +void Scheduler::tick() MM_NONBLOCKING { uint32_t now = platform::millis(); uint32_t tickStart = platform::micros(); diff --git a/src/core/Scheduler.h b/src/core/Scheduler.h index 9773a6c1..98917d44 100644 --- a/src/core/Scheduler.h +++ b/src/core/Scheduler.h @@ -62,7 +62,7 @@ class Scheduler { void addModule(MoonModule* mod); void setup(); - void tick(); + void tick() MM_NONBLOCKING; void release(); uint32_t elapsed() const; diff --git a/src/core/SystemModule.h b/src/core/SystemModule.h index 67af2737..f11a06cf 100644 --- a/src/core/SystemModule.h +++ b/src/core/SystemModule.h @@ -79,7 +79,7 @@ class SystemModule : public MoonModule { /// Diagnostics keep ticking regardless — disabling System hides uptime/heap/fps /// from the UI for no good reason, and the user can't easily re-enable. - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } /// Accepts no user-added children — System is fixed infrastructure. Its child /// modules (Tasks, I2cScan, …) are wired by code in main.cpp and marked @@ -229,7 +229,7 @@ class SystemModule : public MoonModule { MoonModule::defineControls(); } - void tick1s() override { + void tick1s() MM_NONBLOCKING override { // deviceName is the single network identity (mDNS .local, SoftAP SSID, // DHCP hostname all derive from it), so it must stay a valid hostname whatever // the user typed or persistence restored. Coerce it here each tick — idempotent diff --git a/src/core/TasksModule.h b/src/core/TasksModule.h index e3b38a33..ff3ff966 100644 --- a/src/core/TasksModule.h +++ b/src/core/TasksModule.h @@ -60,7 +60,7 @@ class TasksModule : public MoonModule { /// Refresh the RTOS snapshot + the current-per-core names once a second (off the hot path). The /// module cost table needs no refresh — it reads the live tree on each serialize. - void tick1s() override { + void tick1s() MM_NONBLOCKING override { MoonModule::tick1s(); tasks_.refresh(); platform::currentTaskOnCore(0, core0_, sizeof(core0_)); diff --git a/src/light/drivers/DriverBase.h b/src/light/drivers/DriverBase.h index ef3e1f9a..973fd495 100644 --- a/src/light/drivers/DriverBase.h +++ b/src/light/drivers/DriverBase.h @@ -48,7 +48,7 @@ class DriverBase : public MoonModule { /// release() then deleteTree; removeChild() quiesces). A stack-declared driver in a test must be /// declared BEFORE its Drivers, so reverse-declaration order destroys the container (and stops its /// worker) first. TSan enforces this. - ModuleRole role() const override { return ModuleRole::Driver; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Driver; } virtual void setSourceBuffer(Buffer* buf) = 0; /// The hardware peripheral block this driver drives, for the parallel-driver claim guard (two live diff --git a/src/light/drivers/Drivers.h b/src/light/drivers/Drivers.h index 4968308f..4dd54e29 100644 --- a/src/light/drivers/Drivers.h +++ b/src/light/drivers/Drivers.h @@ -247,7 +247,7 @@ class Drivers : public MoonModule { /// single frame's — a lone sample lands wherever tick1s happens to fall and reads ~0 even when the /// core is idling most frames. The peak is the number the Step 2b (ping-pong buffer) decision /// wants: how much time core 0 gives up at worst. A dash when the split isn't running. - void tick1s() override { + void tick1s() MM_NONBLOCKING override { if (renderSplitActive_) std::snprintf(renderWaitStr_, sizeof(renderWaitStr_), "%u µs", static_cast(renderWaitPeakUs_)); else std::snprintf(renderWaitStr_, sizeof(renderWaitStr_), "—"); @@ -382,7 +382,7 @@ class Drivers : public MoonModule { return true; } - void tick() override { + void tick() MM_NONBLOCKING override { // Split active: core 1 is encoding the PREVIOUS frame from outputBuffer_. Wait for it to // finish before overwriting the shared buffer (the boundary). The stall is timed — it's the // Step 2b trigger metric: ~0 when render ≈ encode (heavy effect), large when render ≪ encode. diff --git a/src/light/drivers/HueDriver.h b/src/light/drivers/HueDriver.h index 02f5d396..133bc53e 100644 --- a/src/light/drivers/HueDriver.h +++ b/src/light/drivers/HueDriver.h @@ -105,7 +105,7 @@ class HueDriver : public DriverBase { /// round-trip would stall the single-thread render loop (the "never block the loop" rule, /// lessons.md). One PUT every kPutIntervalMs, round-robined across the lights; pairing + the /// bridge announce ride the slow 1 Hz tick. - void tick() override { + void tick() MM_NONBLOCKING override { if (pairTicksLeft_ > 0) return; // pairing owns the bridge during its window if (!appKey[0] || !haveBridge() || lightCount_ == 0) return; const uint32_t now = platform::millis(); @@ -117,7 +117,7 @@ class HueDriver : public DriverBase { /// The 1 Hz tick handles the non-render-critical, slower bridge work: the pairing poll, the /// one-shot light + group fetch, and the periodic DevicesModule announce. Each is at most one /// bridge call per second — acceptable on a 1 Hz tick, and never in the per-frame tick(). - void tick1s() override { + void tick1s() MM_NONBLOCKING override { if (pairTicksLeft_ > 0) { pollPairing(); DriverBase::tick1s(); return; } if (!appKey[0] || !haveBridge()) { DriverBase::tick1s(); return; } if (!sawLights_) { fetchLights(); DriverBase::tick1s(); return; } diff --git a/src/light/drivers/LedPeripheral.h b/src/light/drivers/LedPeripheral.h index 1558d693..33c469e0 100644 --- a/src/light/drivers/LedPeripheral.h +++ b/src/light/drivers/LedPeripheral.h @@ -47,7 +47,7 @@ class LedPeripheral { // --- Static descriptors: per-peripheral constants the orchestrator reads through the interface --- /// Parallel lanes this peripheral's silicon provides on the current chip (0 = not this chip). - virtual uint8_t lanesAvailable() const = 0; + virtual uint8_t lanesAvailable() const MM_NONBLOCKING = 0; /// Can this peripheral host the 74HCT595 pin expander? (Needs a DMA that reaches PSRAM.) virtual bool supportsPinExpander() const = 0; /// Can this peripheral run the async double-buffer (a second whole-frame buffer, encode overlaps @@ -95,7 +95,7 @@ class LedPeripheral { /// Send one frame on the ring (prime + arm + ISR refill). False if the ring isn't up. virtual bool busTransmitRing() { return false; } /// Is the live bus a streaming ring? - virtual bool busIsRing() const { return false; } + virtual bool busIsRing() const MM_NONBLOCKING { return false; } /// Should reinit build a ring for the current config instead of the whole-frame path? virtual bool wantsRing() const { return false; } /// The ring's active-mode label for the status line, or nullptr for the whole-frame path. diff --git a/src/light/drivers/LightPresetsModule.h b/src/light/drivers/LightPresetsModule.h index e2eeb21f..4a6494f6 100644 --- a/src/light/drivers/LightPresetsModule.h +++ b/src/light/drivers/LightPresetsModule.h @@ -47,7 +47,7 @@ namespace mm { /// @card lightpresets.png class LightPresetsModule : public MoonModule, public ListSource { public: - ModuleRole role() const override { return ModuleRole::Generic; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Generic; } // A boot-wired singleton (exactly one, added under Drivers at boot): not user-deletable, the same // as the boot-wired PreviewDriver. Drivers accepts only `driver` children, so a deleted preset diff --git a/src/light/drivers/MoonLedDriver.h b/src/light/drivers/MoonLedDriver.h index fb930db3..1ff8a2dd 100644 --- a/src/light/drivers/MoonLedDriver.h +++ b/src/light/drivers/MoonLedDriver.h @@ -256,7 +256,7 @@ class MoonI80Peripheral : public LedPeripheral { /// LCD_CAM lanes on this chip (0 = none, and then the orchestrator's guards make the driver inert). /// Unlike the sibling this does NOT add `i2sLanes`: the classic ESP32's i80 is the I2S peripheral, /// which this backend does not implement. - uint8_t lanesAvailable() const override { return platform::lcdLanes; } + uint8_t lanesAvailable() const MM_NONBLOCKING override { return platform::lcdLanes; } /// The i80 bus width is 8 or 16 — a hardware fact (`lcd_ll_set_data_wire_width` takes nothing else). /// The PIN count stays free: configure only the pins that drive something and the orchestrator rounds /// the bus up around them, parking the spare lanes on WR (which the peripheral already drives, and @@ -547,7 +547,7 @@ class MoonI80Peripheral : public LedPeripheral { } /// Did the bus actually come up as a ring? The orchestrator routes tick() on this, so it reports what /// the platform BUILT, not what was asked for — a ring that would not fit falls back to whole-frame. - bool busIsRing() const override { return platform::moonI80Ws2812IsRing(bus_); } + bool busIsRing() const MM_NONBLOCKING override { return platform::moonI80Ws2812IsRing(bus_); } /// The platform's `MoonI80EncodeFn` seam: a plain function pointer (there is no virtual hook for it), /// so this static trampoline recovers `this` from `user` and encodes one slice into the ring buffer the diff --git a/src/light/drivers/MultiPinLedDriver.h b/src/light/drivers/MultiPinLedDriver.h index 45e8c99e..58a61ee7 100644 --- a/src/light/drivers/MultiPinLedDriver.h +++ b/src/light/drivers/MultiPinLedDriver.h @@ -94,7 +94,7 @@ class I80Peripheral : public LedPeripheral { /// inert-on-wrong-chip guards key off it. Reads whichever backend the silicon has — /// `lcdLanes` (LCD_CAM, S3/P4/S31) or `i2sLanes` (I2S-i80, classic ESP32) — which are mutually /// exclusive per chip (at most one is non-zero), so the sum picks the right one. - uint8_t lanesAvailable() const override { return platform::lcdLanes + platform::i2sLanes; } + uint8_t lanesAvailable() const MM_NONBLOCKING override { return platform::lcdLanes + platform::i2sLanes; } bool powerOfTwoBus() const override { return true; } // the BUS rounds to 8/16; the pin count is free /// Whole-frame DMA byte budget. On the classic ESP32 the i80 is the I2S peripheral: its DMA is diff --git a/src/light/drivers/NetworkSendDriver.h b/src/light/drivers/NetworkSendDriver.h index c48ffce3..ade6cd6d 100644 --- a/src/light/drivers/NetworkSendDriver.h +++ b/src/light/drivers/NetworkSendDriver.h @@ -205,7 +205,7 @@ class NetworkSendDriver : public DriverBase { /// Rate-limit to `fps`, apply this driver's correction into corrected_ (passthrough if it /// emits no channels), then chunk the window slice into protocol packets and send inline. - void tick() override { + void tick() MM_NONBLOCKING override { if (!sourceBuffer_ || !sourceBuffer_->data()) return; // No destination → idle. An unconfigured driver does nothing; it never falls back to diff --git a/src/light/drivers/ParallelLedDriver.h b/src/light/drivers/ParallelLedDriver.h index 8bcfdee9..e7702f82 100644 --- a/src/light/drivers/ParallelLedDriver.h +++ b/src/light/drivers/ParallelLedDriver.h @@ -549,7 +549,14 @@ class ParallelLedDriver : public DriverBase { /// buffer + correction. (The double-buffer defaults ON — it overlaps the blocking wire wait and /// lifted the P4 whole-board rate 48→76 fps; OFF is the sound-reactive 0-latency opt-out and pays /// for exactly one buffer — see the doubleBuffer control + docs/history/lessons.md.) - void tick() override { + // REPORTED AS BLOCKING, deliberately: tickSync()/tickRing() reach busWaitIfBusy(), which + // waits for the DMA transfer to finish (bounded by waitBudgetMs, and self-limiting via + // deadFrames_). The wait is by design — the driver owns the bus for the frame, and encoding + // over a live transfer corrupts output — but it IS blocking, so clang-hotpath lists it. + // Not suppressed: a report of what blocks the render path is worthless if the two worst + // offenders are hidden from it. Moving the wait off the render path is backlogged + // (backlog-core: hot path). + void tick() MM_NONBLOCKING override { if (!peripheral_ || peripheral_->lanesAvailable() == 0) return; // no backend / inert off this chip // Loopback mode owns the peripheral EXCLUSIVELY. While it is on, the render loop must not // transmit — the loopback tears the bus down, drives its own private frame, and rebuilds it. @@ -689,7 +696,7 @@ class ParallelLedDriver : public DriverBase { /// wire time and the fps ceiling it implies (1e6 / frameTime). This is the pure WS2812 output floor — /// the render loop can never beat it, so it's the target the multicore work drives the system tick /// toward, and it tracks an overclocked slot rate directly. "—" until the first transfer completes. - void tick1s() override { + void tick1s() MM_NONBLOCKING override { if (!peripheral_) return; const uint32_t us = peripheral_->busLastTransmitUs(); if (us == 0) std::snprintf(frameTimeStr_, sizeof(frameTimeStr_), "—"); diff --git a/src/light/drivers/ParlioLedDriver.h b/src/light/drivers/ParlioLedDriver.h index 0f2c4d1d..b7faf72f 100644 --- a/src/light/drivers/ParlioLedDriver.h +++ b/src/light/drivers/ParlioLedDriver.h @@ -33,7 +33,7 @@ class ParlioPeripheral : public LedPeripheral { /// The number of Parlio lanes this chip provides (0 = not this chip); the orchestrator's /// inert-on-wrong-chip guards key off it. - uint8_t lanesAvailable() const override { return platform::parlioLanes; } + uint8_t lanesAvailable() const MM_NONBLOCKING override { return platform::parlioLanes; } bool powerOfTwoBus() const override { return false; } // 1..16 lanes all valid // Parlio builds a 1-lane private unit for the loopback, so the loopback frame stays 8-bit // regardless of the operational bus width (unlike i80, which needs a full-width bus). diff --git a/src/light/drivers/PreviewDriver.h b/src/light/drivers/PreviewDriver.h index c4472382..e9255df8 100644 --- a/src/light/drivers/PreviewDriver.h +++ b/src/light/drivers/PreviewDriver.h @@ -129,7 +129,10 @@ class PreviewDriver : public DriverBase { /// The frame rate self-limits to what the link sustains (sheds rate first, then /// spatial resolution via adaptive downscale), so a large grid never stalls the /// loop or tears — it always delivers a complete frame. - void tick() override { + // REPORTED AS BLOCKING, deliberately: sendFrame() writes to a socket and + // buildAndSendCoordTable() resizes keptIdx_. Both are real and both are on the render path, + // so clang-hotpath lists them rather than hiding them. Backlogged (backlog-core: hot path). + void tick() MM_NONBLOCKING override { if (fps == 0) return; uint32_t now = platform::millis(); uint32_t interval = 1000 / fps; diff --git a/src/light/drivers/RmtLedDriver.h b/src/light/drivers/RmtLedDriver.h index d973dc4d..b12e6d89 100644 --- a/src/light/drivers/RmtLedDriver.h +++ b/src/light/drivers/RmtLedDriver.h @@ -231,7 +231,7 @@ class RmtLedDriver : public DriverBase { /// over this driver's window, then start every pin's transmit before waiting on /// any, so the tick costs the longest strand rather than the sum. Inert off RMT /// chips and idle until inited with a source buffer + correction. - void tick() override { + void tick() MM_NONBLOCKING override { if constexpr (platform::rmtTxChannels == 0) return; // inert off RMT chips if (!inited_ || !sourceBuffer_ || !sourceBuffer_->data()) return; @@ -357,7 +357,7 @@ class RmtLedDriver : public DriverBase { // Convert a ns duration to RMT ticks using the resolution the platform // granted. Falls back to the requested clock when not inited (host/desktop). - uint16_t nsToTicks(uint32_t ns) const { + uint16_t nsToTicks(uint32_t ns) const MM_NONBLOCKING { uint32_t hz = inited_ ? platform::rmtWs2812Resolution(rmt_[0]) : kResolutionHz; if (hz == 0) hz = kResolutionHz; return static_cast((static_cast(ns) * hz) / 1'000'000'000ull); diff --git a/src/light/effects/AudioSpectrumEffect.h b/src/light/effects/AudioSpectrumEffect.h index 59b527b4..007aea22 100644 --- a/src/light/effects/AudioSpectrumEffect.h +++ b/src/light/effects/AudioSpectrumEffect.h @@ -34,7 +34,7 @@ class AudioSpectrumEffect : public EffectBase { controls_.addSelect("colorMode", colorMode, kColorOptions, 2); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); const lengthType w = width(); const lengthType h = height(); diff --git a/src/light/effects/AudioVolumeEffect.h b/src/light/effects/AudioVolumeEffect.h index 28b20465..76109ae1 100644 --- a/src/light/effects/AudioVolumeEffect.h +++ b/src/light/effects/AudioVolumeEffect.h @@ -21,7 +21,7 @@ class AudioVolumeEffect : public EffectBase { controls_.addUint8("brightness", brightness, 1, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); const lengthType w = width(); const lengthType h = height(); diff --git a/src/light/effects/BlurzEffect.h b/src/light/effects/BlurzEffect.h index 5087fe12..27966abe 100644 --- a/src/light/effects/BlurzEffect.h +++ b/src/light/effects/BlurzEffect.h @@ -53,10 +53,9 @@ class BlurzEffect : public EffectBase { scanPos_ = 0; } - void tick() override { + void tick() MM_NONBLOCKING override { const int cols = width(); const int rows = height(); - if (cols <= 0 || rows <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(cols), static_cast(rows), depthDim()}; diff --git a/src/light/effects/BouncingBallsEffect.h b/src/light/effects/BouncingBallsEffect.h index 45b9961f..957936e0 100644 --- a/src/light/effects/BouncingBallsEffect.h +++ b/src/light/effects/BouncingBallsEffect.h @@ -46,12 +46,11 @@ class BouncingBallsEffect : public EffectBase { balls_.resize(cols * maxNumBalls); } - void tick() override { + void tick() MM_NONBLOCKING override { if (!balls_) return; const int cols = width(); const int rows = height(); - if (cols <= 0 || rows <= 0) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(cols), static_cast(rows), depthDim()}; diff --git a/src/light/effects/DemoReelEffect.h b/src/light/effects/DemoReelEffect.h index 103a785b..e156adf5 100644 --- a/src/light/effects/DemoReelEffect.h +++ b/src/light/effects/DemoReelEffect.h @@ -53,7 +53,7 @@ class DemoReelEffect : public EffectBase { swapTo(cursor_ < eligibleCount_ ? cursor_ : 0); } - void tick() override { + void tick() MM_NONBLOCKING override { if (eligibleCount_ == 0 || !current_) return; // Advance on the interval. elapsed() is the Layer's monotonic ms clock (same source every diff --git a/src/light/effects/DistortionWavesEffect.h b/src/light/effects/DistortionWavesEffect.h index 10188202..18166194 100644 --- a/src/light/effects/DistortionWavesEffect.h +++ b/src/light/effects/DistortionWavesEffect.h @@ -33,7 +33,7 @@ class DistortionWavesEffect : public EffectBase { controls_.addUint8("speed", speed, 0, 100); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); const lengthType w = width(); const lengthType h = height(); diff --git a/src/light/effects/EffectBase.h b/src/light/effects/EffectBase.h index 47882be4..171ad1bf 100644 --- a/src/light/effects/EffectBase.h +++ b/src/light/effects/EffectBase.h @@ -65,7 +65,7 @@ class Layer; // forward declaration (defined in light/layers/Layer.h, included a /// https://github.com/ewowi/MoonLight/blob/main/src/MoonLight/Layers/VirtualLayer.h). class EffectBase : public MoonModule { public: - ModuleRole role() const override { return ModuleRole::Effect; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Effect; } /// Which axes the effect *iterates* — a claim, not a guarantee about the layer. /// diff --git a/src/light/effects/FireEffect.h b/src/light/effects/FireEffect.h index ba02db6e..131ba58e 100644 --- a/src/light/effects/FireEffect.h +++ b/src/light/effects/FireEffect.h @@ -34,7 +34,7 @@ class FireEffect : public EffectBase { heat_.resize(static_cast(width()) * height()); } - void tick() override { + void tick() MM_NONBLOCKING override { if (!heat_) return; uint8_t* buf = buffer(); diff --git a/src/light/effects/FixedRectangleEffect.h b/src/light/effects/FixedRectangleEffect.h index 0cc2bbb5..d7159f84 100644 --- a/src/light/effects/FixedRectangleEffect.h +++ b/src/light/effects/FixedRectangleEffect.h @@ -57,11 +57,10 @@ class FixedRectangleEffect : public EffectBase { controls_.addBool("alternateWhite", alternateWhite); } - void tick() override { + void tick() MM_NONBLOCKING override { const int w = width(); const int h = height(); const int d = depth(); - if (w <= 0 || h <= 0) return; Buffer& buf = layer()->buffer(); const uint8_t cpl = channelsPerLight(); diff --git a/src/light/effects/FreqMatrixEffect.h b/src/light/effects/FreqMatrixEffect.h index 39eb89ef..360c11cf 100644 --- a/src/light/effects/FreqMatrixEffect.h +++ b/src/light/effects/FreqMatrixEffect.h @@ -59,11 +59,10 @@ class FreqMatrixEffect : public EffectBase { controls_.addBool("audioSpeed", audioSpeed); } - void tick() override { + void tick() MM_NONBLOCKING override { // D1: read the live grid each frame; the scroll runs down the x=0 column, length = height(). const int cols = width(); const int len = height(); - if (cols <= 0 || len <= 0) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(cols), static_cast(len), depthDim()}; diff --git a/src/light/effects/FreqSawsEffect.h b/src/light/effects/FreqSawsEffect.h index ba2dea80..925aa193 100644 --- a/src/light/effects/FreqSawsEffect.h +++ b/src/light/effects/FreqSawsEffect.h @@ -64,10 +64,9 @@ class FreqSawsEffect : public EffectBase { clearState(); } - void tick() override { + void tick() MM_NONBLOCKING override { const int sizeX = width(); const int sizeY = height(); - if (sizeX <= 0 || sizeY <= 0 || channelsPerLight() < 3) return; const AudioFrame* f = AudioService::latestFrame(); if (!f) return; // null-safe (latestFrame returns silence, never null, but guard regardless) diff --git a/src/light/effects/GEQ3DEffect.h b/src/light/effects/GEQ3DEffect.h index 4bf77087..0556a322 100644 --- a/src/light/effects/GEQ3DEffect.h +++ b/src/light/effects/GEQ3DEffect.h @@ -46,12 +46,11 @@ class GEQ3DEffect : public EffectBase { controls_.addBool("borders", borders); } - void tick() override { + void tick() MM_NONBLOCKING override { if (numBands == 0) return; const int cols = width(); const int rows = height(); - if (cols <= 0 || rows <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(cols), static_cast(rows), depthDim()}; diff --git a/src/light/effects/GEQEffect.h b/src/light/effects/GEQEffect.h index 03a51e8d..6fbb4e66 100644 --- a/src/light/effects/GEQEffect.h +++ b/src/light/effects/GEQEffect.h @@ -61,10 +61,9 @@ class GEQEffect : public EffectBase { rippleCounter_ = 0; } - void tick() override { + void tick() MM_NONBLOCKING override { const int cols = width(); const int rows = height(); - if (cols <= 0 || rows <= 0 || channelsPerLight() < 3) return; if (!peaks_) return; // build hasn't allocated yet (e.g. disabled) — nothing to draw const AudioFrame* f = AudioService::latestFrame(); diff --git a/src/light/effects/GameOfLifeEffect.h b/src/light/effects/GameOfLifeEffect.h index 3ae8eb6c..007e54e7 100644 --- a/src/light/effects/GameOfLifeEffect.h +++ b/src/light/effects/GameOfLifeEffect.h @@ -149,7 +149,7 @@ class GameOfLifeEffect : public EffectBase { bool birthForTest(uint8_t n) const { return birthNumbers_[n]; } bool surviveForTest(uint8_t n) const { return surviveNumbers_[n]; } - void tick() override { + void tick() MM_NONBLOCKING override { if (!cells_ || !future_ || !colors_ || cellCount_ == 0) return; const lengthType w = width(), h = height(), d = depth(); const uint8_t cpl = channelsPerLight(); diff --git a/src/light/effects/LavaLampEffect.h b/src/light/effects/LavaLampEffect.h index 3dfd0bce..b4cced24 100644 --- a/src/light/effects/LavaLampEffect.h +++ b/src/light/effects/LavaLampEffect.h @@ -28,12 +28,18 @@ class LavaLampEffect : public EffectBase { controls_.addUint8("intensity", intensity, 1, 255); } - void tick() override { + // Class scope, not function-local: -Wfunction-effects flags ANY static local in a + // nonblocking function, including a constexpr that needs no guard variable. Same + // storage and value here, and these are per-effect constants anyway. + static constexpr uint8_t SPEED_MUL[NUM_BLOBS] = { 1, 2, 1 }; + static constexpr uint8_t PHASE_X[NUM_BLOBS] = { 0, 80, 160 }; + static constexpr uint8_t PHASE_Y[NUM_BLOBS] = { 64, 200, 100 }; + + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); lengthType w = width(); lengthType h = height(); uint8_t cpl = channelsPerLight(); - if (w <= 0 || h <= 0) return; uint32_t now = elapsed(); uint32_t dt = now - lastElapsed_; @@ -46,9 +52,6 @@ class LavaLampEffect : public EffectBase { int16_t bx[NUM_BLOBS] = {}; int16_t by[NUM_BLOBS] = {}; - static constexpr uint8_t SPEED_MUL[NUM_BLOBS] = { 1, 2, 1 }; - static constexpr uint8_t PHASE_X[NUM_BLOBS] = { 0, 80, 160 }; - static constexpr uint8_t PHASE_Y[NUM_BLOBS] = { 64, 200, 100 }; for (uint8_t b = 0; b < NUM_BLOBS; b++) { uint8_t tb = static_cast(t * SPEED_MUL[b]); bx[b] = static_cast((sin8(static_cast(tb + PHASE_X[b])) * w) >> 8); diff --git a/src/light/effects/LinesEffect.h b/src/light/effects/LinesEffect.h index 0ea919aa..3078af03 100644 --- a/src/light/effects/LinesEffect.h +++ b/src/light/effects/LinesEffect.h @@ -42,7 +42,7 @@ class LinesEffect : public EffectBase { controls_.setHidden(controls_.count() - 1, !dots); // panel dots only } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); const lengthType w = width(); const lengthType h = height(); diff --git a/src/light/effects/LissajousEffect.h b/src/light/effects/LissajousEffect.h index e9a8100a..376d6444 100644 --- a/src/light/effects/LissajousEffect.h +++ b/src/light/effects/LissajousEffect.h @@ -37,10 +37,9 @@ class LissajousEffect : public EffectBase { controls_.addUint8("speed", speed, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { const int w = width(); const int h = height(); - if (w <= 0 || h <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(w), static_cast(h), depthDim()}; diff --git a/src/light/effects/MetaballsEffect.h b/src/light/effects/MetaballsEffect.h index d69dae0c..9fdf752e 100644 --- a/src/light/effects/MetaballsEffect.h +++ b/src/light/effects/MetaballsEffect.h @@ -27,12 +27,18 @@ class MetaballsEffect : public EffectBase { controls_.addUint8("hue_shift", hue_shift, 0, 255); } - void tick() override { + // Class scope, not function-local: -Wfunction-effects flags ANY static local in a + // nonblocking function, including a constexpr that needs no guard variable. Same + // storage and value here, and these are per-effect constants anyway. + static constexpr uint8_t SPEED_MUL[MAX_BALLS] = { 1, 2, 3, 1, 2, 3, 1, 2 }; + static constexpr uint8_t PHASE_X[MAX_BALLS] = { 0, 30, 60, 120, 160, 200, 90, 220 }; + static constexpr uint8_t PHASE_Y[MAX_BALLS] = { 64, 94, 124, 184, 16, 210, 150, 40 }; + + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); lengthType w = width(); lengthType h = height(); uint8_t cpl = channelsPerLight(); - uint32_t now = elapsed(); uint32_t dt = now - lastElapsed_; lastElapsed_ = now; @@ -47,9 +53,6 @@ class MetaballsEffect : public EffectBase { const uint8_t n = count < MAX_BALLS ? count : MAX_BALLS; int16_t bx[MAX_BALLS]; int16_t by[MAX_BALLS]; - static constexpr uint8_t SPEED_MUL[MAX_BALLS] = { 1, 2, 3, 1, 2, 3, 1, 2 }; - static constexpr uint8_t PHASE_X[MAX_BALLS] = { 0, 30, 60, 120, 160, 200, 90, 220 }; - static constexpr uint8_t PHASE_Y[MAX_BALLS] = { 64, 94, 124, 184, 16, 210, 150, 40 }; for (uint8_t b = 0; b < n; b++) { uint8_t tb = static_cast(t * SPEED_MUL[b]); bx[b] = static_cast((sin8(static_cast(tb + PHASE_X[b])) * w) >> 8); diff --git a/src/light/effects/NetworkReceiveEffect.h b/src/light/effects/NetworkReceiveEffect.h index dd927845..3cc940e6 100644 --- a/src/light/effects/NetworkReceiveEffect.h +++ b/src/light/effects/NetworkReceiveEffect.h @@ -85,7 +85,7 @@ class NetworkReceiveEffect : public EffectBase { staging_.resize(static_cast(nrOfLights()) * channelsPerLight()); } - void tick() override { + void tick() MM_NONBLOCKING override { if (!staging_) return; // Bounded non-blocking drain per socket: 128 packets ≈ one full ArtNet // frame for ~21k RGB lights; a flood costs at most 3×128 recvfrom calls diff --git a/src/light/effects/Noise2DEffect.h b/src/light/effects/Noise2DEffect.h index 46c63cc9..7b7aac74 100644 --- a/src/light/effects/Noise2DEffect.h +++ b/src/light/effects/Noise2DEffect.h @@ -36,10 +36,9 @@ class Noise2DEffect : public EffectBase { controls_.addUint8("scale", scale, 2, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { const int cols = width(); const int rows = height(); - if (cols <= 0 || rows <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(cols), static_cast(rows), depthDim()}; diff --git a/src/light/effects/NoiseEffect.h b/src/light/effects/NoiseEffect.h index 9de559d4..d5b3650c 100644 --- a/src/light/effects/NoiseEffect.h +++ b/src/light/effects/NoiseEffect.h @@ -20,7 +20,7 @@ class NoiseEffect : public EffectBase { controls_.addUint8("bpm", bpm, 1, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); lengthType w = width(); lengthType h = height(); diff --git a/src/light/effects/NoiseMeterEffect.h b/src/light/effects/NoiseMeterEffect.h index e94c0114..28b22362 100644 --- a/src/light/effects/NoiseMeterEffect.h +++ b/src/light/effects/NoiseMeterEffect.h @@ -35,11 +35,10 @@ class NoiseMeterEffect : public EffectBase { controls_.addUint8("width", width, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { const int sizeX = width_(); const int sizeY = height(); const int sizeZ = depthDim(); - if (sizeX <= 0 || sizeY <= 0 || channelsPerLight() < 3) return; const AudioFrame* f = AudioService::latestFrame(); if (!f) return; // null-safe (latestFrame returns silence, never null, but guard regardless) diff --git a/src/light/effects/PaintBrushEffect.h b/src/light/effects/PaintBrushEffect.h index 04b3ba06..4309b756 100644 --- a/src/light/effects/PaintBrushEffect.h +++ b/src/light/effects/PaintBrushEffect.h @@ -39,7 +39,7 @@ class PaintBrushEffect : public EffectBase { controls_.addBool("phase_chaos", phase_chaos); } - void tick() override { + void tick() MM_NONBLOCKING override { const lengthType cols = width(), rows = height(), depth = this->depth(); const uint8_t cpl = channelsPerLight(); if (cols == 0 || rows == 0 || cpl < 3) return; // 0×0×0 and short-channel guard diff --git a/src/light/effects/ParticlesEffect.h b/src/light/effects/ParticlesEffect.h index 5e6fbb1c..640dfde0 100644 --- a/src/light/effects/ParticlesEffect.h +++ b/src/light/effects/ParticlesEffect.h @@ -33,10 +33,14 @@ class ParticlesEffect : public EffectBase { // layers — avoids allocating depth× more heap than needed. resize() reallocs (zero-filled) // only when the byte count changes, frees on 0, and keeps dynamicBytes current. trail_.resize(static_cast(width()) * height() * channelsPerLight()); + // `trail_` IS the degenerate-grid gate on this path. Layer::tick's gate covers tick() + // only, and prepare() runs outside it — so a zero grid is stopped here by resize(0) + // freeing the buffer, not by that gate. Without this, initParticles seeds every + // particle from `(rand8() * w) >> 4` with w == 0. if (trail_) initParticles(); } - void tick() override { + void tick() MM_NONBLOCKING override { if (!trail_) return; lengthType w = width(); @@ -100,7 +104,6 @@ class ParticlesEffect : public EffectBase { void initParticles() { lengthType w = width(); lengthType h = height(); - if (w <= 0 || h <= 0) return; if (initialized_) return; for (uint8_t i = 0; i < MAX_PARTICLES; i++) { particles_[i].x = static_cast((static_cast(rand8()) * w) >> 4); diff --git a/src/light/effects/PlasmaEffect.h b/src/light/effects/PlasmaEffect.h index bed658fa..57b878be 100644 --- a/src/light/effects/PlasmaEffect.h +++ b/src/light/effects/PlasmaEffect.h @@ -27,7 +27,7 @@ class PlasmaEffect : public EffectBase { controls_.addUint8("hue_shift", hue_shift, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); lengthType w = width(); lengthType h = height(); diff --git a/src/light/effects/PraxisEffect.h b/src/light/effects/PraxisEffect.h index 68d08153..d0212dd6 100644 --- a/src/light/effects/PraxisEffect.h +++ b/src/light/effects/PraxisEffect.h @@ -50,10 +50,9 @@ class PraxisEffect : public EffectBase { controls_.addUint8("microMutatorMax", microMutatorMax, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { const int w = width(); const int h = height(); - if (w <= 0 || h <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(w), static_cast(h), depthDim()}; diff --git a/src/light/effects/RainbowEffect.h b/src/light/effects/RainbowEffect.h index 92f4314c..ffa80e4a 100644 --- a/src/light/effects/RainbowEffect.h +++ b/src/light/effects/RainbowEffect.h @@ -20,7 +20,7 @@ class RainbowEffect : public EffectBase { controls_.addUint8("speed", speed, 1, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { // D2 effect — writes only z=0; Layer::extrude duplicates across z. uint8_t* buf = buffer(); lengthType w = width(); diff --git a/src/light/effects/RandomEffect.h b/src/light/effects/RandomEffect.h index 42a52f32..51dcf6ee 100644 --- a/src/light/effects/RandomEffect.h +++ b/src/light/effects/RandomEffect.h @@ -30,7 +30,7 @@ class RandomEffect : public EffectBase { controls_.addUint8("fade", fade, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { Buffer& buf = layer()->buffer(); const nrOfLightsType n = nrOfLights(); const uint8_t cpl = buf.channelsPerLight(); diff --git a/src/light/effects/RingsEffect.h b/src/light/effects/RingsEffect.h index 5a83daea..9c4ea3a2 100644 --- a/src/light/effects/RingsEffect.h +++ b/src/light/effects/RingsEffect.h @@ -34,12 +34,11 @@ class RingsEffect : public EffectBase { controls_.addUint8("hue_shift", hue_shift, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); lengthType w = width(); lengthType h = height(); uint8_t cpl = channelsPerLight(); - if (w <= 0 || h <= 0) return; // Visible radius limit (octagonal distance to far corner) uint8_t maxR = dist8(static_cast(w), static_cast(h)); diff --git a/src/light/effects/RipplesEffect.h b/src/light/effects/RipplesEffect.h index c4499735..a59c3ccf 100644 --- a/src/light/effects/RipplesEffect.h +++ b/src/light/effects/RipplesEffect.h @@ -34,13 +34,12 @@ class RipplesEffect : public EffectBase { controls_.addUint8("interval", interval, 1, 254); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); const lengthType w = width(); const lengthType h = height(); const lengthType d = depth(); const uint8_t cpl = channelsPerLight(); - if (w <= 0 || h <= 0 || d <= 0) return; // Clear: every column lights at most one y, so the rest must be black. std::memset(buf, 0, static_cast(nrOfLights()) * cpl); diff --git a/src/light/effects/RubiksCubeEffect.h b/src/light/effects/RubiksCubeEffect.h index adb32f5c..b4b00a05 100644 --- a/src/light/effects/RubiksCubeEffect.h +++ b/src/light/effects/RubiksCubeEffect.h @@ -61,9 +61,8 @@ class RubiksCubeEffect : public EffectBase { doInit_ = true; } - void tick() override { + void tick() MM_NONBLOCKING override { const lengthType w = width(), h = height(), d = depth(); - if (w <= 0 || h <= 0 || d <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{w, h, d}; @@ -110,8 +109,8 @@ class RubiksCubeEffect : public EffectBase { using Face = std::array, MAX_SIZE>; Face front, back, left, right, top, bottom; - void init(uint8_t cubeSize) { - SIZE = cubeSize; + void init(uint8_t order) { + SIZE = order; for (int i = 0; i < MAX_SIZE; i++) for (int j = 0; j < MAX_SIZE; j++) { front[i][j] = 0; back[i][j] = 1; left[i][j] = 2; diff --git a/src/light/effects/SineEffect.h b/src/light/effects/SineEffect.h index a1c78817..fe9fbd9b 100644 --- a/src/light/effects/SineEffect.h +++ b/src/light/effects/SineEffect.h @@ -33,7 +33,7 @@ class SineEffect : public EffectBase { controls_.addUint8("bpm", bpm, 1, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); const lengthType w = width(); const lengthType h = height(); diff --git a/src/light/effects/SolidEffect.h b/src/light/effects/SolidEffect.h index 91f916d3..14a2e8c1 100644 --- a/src/light/effects/SolidEffect.h +++ b/src/light/effects/SolidEffect.h @@ -61,11 +61,10 @@ class SolidEffect : public EffectBase { validIndices_.resize(256); } - void tick() override { + void tick() MM_NONBLOCKING override { const int w = width(); const int h = height(); const int d = depth(); - if (w <= 0 || h <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const lengthType dz = d > 0 ? static_cast(d) : 1; diff --git a/src/light/effects/SphereMoveEffect.h b/src/light/effects/SphereMoveEffect.h index fe545750..d746681b 100644 --- a/src/light/effects/SphereMoveEffect.h +++ b/src/light/effects/SphereMoveEffect.h @@ -33,11 +33,10 @@ class SphereMoveEffect : public EffectBase { controls_.addUint8("speed", speed, 0, 99); } - void tick() override { + void tick() MM_NONBLOCKING override { const int w = width(); const int h = height(); const int d = depth(); - if (w <= 0 || h <= 0 || d <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(w), static_cast(h), static_cast(d)}; diff --git a/src/light/effects/SpiralEffect.h b/src/light/effects/SpiralEffect.h index 1ffa0a47..7e7f1d44 100644 --- a/src/light/effects/SpiralEffect.h +++ b/src/light/effects/SpiralEffect.h @@ -23,7 +23,7 @@ class SpiralEffect : public EffectBase { controls_.addUint8("hue_shift", hue_shift, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); lengthType w = width(); lengthType h = height(); diff --git a/src/light/effects/StarFieldEffect.h b/src/light/effects/StarFieldEffect.h index eb0ed9ac..7651ffa1 100644 --- a/src/light/effects/StarFieldEffect.h +++ b/src/light/effects/StarFieldEffect.h @@ -66,12 +66,11 @@ class StarFieldEffect : public EffectBase { } } - void tick() override { + void tick() MM_NONBLOCKING override { if (!stars_) return; const lengthType w = width(); const lengthType h = height(); - if (w <= 0 || h <= 0 || channelsPerLight() < 3) return; // Throttle: pause when speed==0, else advance at most once per 1000/speed ms. if (speed == 0) return; diff --git a/src/light/effects/StarSkyEffect.h b/src/light/effects/StarSkyEffect.h index 2627f064..3abbcffa 100644 --- a/src/light/effects/StarSkyEffect.h +++ b/src/light/effects/StarSkyEffect.h @@ -64,10 +64,9 @@ class StarSkyEffect : public EffectBase { } } - void tick() override { + void tick() MM_NONBLOCKING override { if (!indexes_ || !fadeDir_ || !brightness_ || !colors_ || nbStars_ == 0) return; const lengthType w = width(), h = height(), d = depthDim(); - if (w <= 0 || h <= 0 || channelsPerLight() < 3) return; const nrOfLightsType count = nrOfLights(); if (count == 0) return; diff --git a/src/light/effects/TetrixEffect.h b/src/light/effects/TetrixEffect.h index 8c04d717..15323448 100644 --- a/src/light/effects/TetrixEffect.h +++ b/src/light/effects/TetrixEffect.h @@ -66,12 +66,11 @@ class TetrixEffect : public EffectBase { } } - void tick() override { + void tick() MM_NONBLOCKING override { if (!drops_) return; const lengthType w = width(); const lengthType h = height(); - if (w <= 0 || h <= 0 || channelsPerLight() < 1) return; Buffer& buf = layer()->buffer(); const Coord3D dims{w, h, depthDim()}; diff --git a/src/light/effects/TextEffect.h b/src/light/effects/TextEffect.h index 9bfd763b..b0263387 100644 --- a/src/light/effects/TextEffect.h +++ b/src/light/effects/TextEffect.h @@ -42,10 +42,9 @@ class TextEffect : public EffectBase { controls_.addUint8("hue", hue, 0, 255); } - void tick() override { + void tick() MM_NONBLOCKING override { const int w = width(); const int h = height(); - if (w <= 0 || h <= 0 || channelsPerLight() < 3) return; Buffer& buf = layer()->buffer(); const Coord3D dims{static_cast(w), static_cast(h), depthDim()}; diff --git a/src/light/effects/WaveEffect.h b/src/light/effects/WaveEffect.h index e83ac967..1fd33b02 100644 --- a/src/light/effects/WaveEffect.h +++ b/src/light/effects/WaveEffect.h @@ -60,7 +60,7 @@ class WaveEffect : public EffectBase { trailW_ = w; trailH_ = h; trailCpl_ = cpl; } - void tick() override { + void tick() MM_NONBLOCKING override { if (!trail_) return; const lengthType w = width(); const lengthType h = height(); diff --git a/src/light/layers/Layer.h b/src/light/layers/Layer.h index c06a3341..6ee641e3 100644 --- a/src/light/layers/Layer.h +++ b/src/light/layers/Layer.h @@ -34,7 +34,7 @@ namespace mm { /// @card Layer.png class Layer : public MoonModule { public: - ModuleRole role() const override { return ModuleRole::Layer; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Layer; } const char* acceptsChildRoles() const override { return "effect,modifier"; } ~Layer() override { if (liveScratch_) platform::free(liveScratch_); } @@ -145,7 +145,7 @@ class Layer : public MoonModule { // applyState() recurses to the effects next — they allocate against the LUT/buffer just built. } - void tick() override { + void tick() MM_NONBLOCKING override { // Scheduler already gates the Layer itself by enabled() via respectsEnabled(). // We still gate per-effect-child explicitly because Layer iterates its own // children rather than going through the Scheduler. @@ -162,7 +162,16 @@ class Layer : public MoonModule { // fading effects on one layer cost ONE buffer pass (the gentlest amount wins, preserving the // most light / longest trail) instead of each effect fading the whole shared buffer itself. if (fadeBy_ > 0) { draw::fade(buffer_, fadeBy_); fadeBy_ = 0; } - for (uint8_t i = 0; i < childCount(); i++) { + // A degenerate grid has nothing to draw. This is orchestration — the Layer owns the + // decision to run the effect pass at all, the same way it owns the enabled/role gates + // below — so it is checked ONCE here rather than repeated as a guard clause in every + // effect's tick(). Effects may assume width/height/depth are all >= 1. + // + // It gates only the EFFECT pass, not the whole tick: the modifier pass below advances + // per-frame state (a beat-driven RandomMap) that must keep running so the chain is in + // the right phase when the grid comes back. + const bool hasGrid = width_ > 0 && height_ > 0 && depth_ > 0; + for (uint8_t i = 0; hasGrid && i < childCount(); i++) { if (child(i)->role() != ModuleRole::Effect) continue; if (!child(i)->enabled()) continue; auto* eff = static_cast(child(i)); diff --git a/src/light/layers/Layers.h b/src/light/layers/Layers.h index 72eb1ed4..4b2032a2 100644 --- a/src/light/layers/Layers.h +++ b/src/light/layers/Layers.h @@ -48,7 +48,7 @@ class Layers : public MoonModule { /// ticking it through Layers would run its loop at the wrong tree /// depth (an Effect that should be ticked inside a Layer). Matches /// the role-filter precedent in setLayouts / activeLayer above. - void tick() override { + void tick() MM_NONBLOCKING override { for (uint8_t i = 0; i < childCount(); i++) { MoonModule* c = child(i); if (!c || c->role() != ModuleRole::Layer) continue; diff --git a/src/light/layouts/LayoutBase.h b/src/light/layouts/LayoutBase.h index fab12eb2..13af97dd 100644 --- a/src/light/layouts/LayoutBase.h +++ b/src/light/layouts/LayoutBase.h @@ -65,7 +65,7 @@ struct CoordSink { /// control change triggers the pipeline-wide rebuild. class LayoutBase : public MoonModule { public: - ModuleRole role() const override { return ModuleRole::Layout; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Layout; } virtual nrOfLightsType lightCount() const = 0; virtual void forEachCoord(const CoordSink& sink) const = 0; diff --git a/src/light/layouts/Rings241Layout.h b/src/light/layouts/Rings241Layout.h index 1efcbe79..77b401e7 100644 --- a/src/light/layouts/Rings241Layout.h +++ b/src/light/layouts/Rings241Layout.h @@ -52,7 +52,7 @@ class Rings241Layout : public LayoutBase { // Shared centre — MoonLight: leftMargin = 1.1 * getRadius(60), assigned to // a uint8_t (implicit truncation), stored as ringCenter's integer x/y, then // scaled per LED: x = scale * ringCenter.x. - const uint8_t leftMargin = static_cast(1.1 * getRadius(60)); + const uint8_t leftMargin = static_cast(1.1f * getRadius(60)); nrOfLightsType idx = 0; // Rings emitted smallest-to-largest, the same order MoonLight calls diff --git a/src/light/modifiers/ModifierBase.h b/src/light/modifiers/ModifierBase.h index 1d1173c4..d01ad134 100644 --- a/src/light/modifiers/ModifierBase.h +++ b/src/light/modifiers/ModifierBase.h @@ -36,7 +36,7 @@ namespace mm { /// **Prior art:** the two building blocks are the textbook image-warping pattern — bake a coordinate transform into a precomputed spatial LUT, and build that table by BACKWARD mapping (walk the destinations, find each one's source) so no output pixel is ever left unmapped (https://towardsdatascience.com/forward-and-backward-mapping-for-computer-vision-833436e2472/). What is specific here — and credited to MoonLight — is collapsing a whole *chain* of discrete pixel folds into ONE index table: a desktop node graph (TouchDesigner, shader graphs) gives each node its own frame buffer; an ESP32 can't spare a buffer per modifier, so the chain is folded into a single LUT and the hot path stays one gather. MoonLight's `modifySize` / `modifyPosition` / `modifyXYZ` map to our `modifyLogicalSize` / `modifyLogical` / `modifyLive`, written fresh against our `MappingLUT` (https://github.com/ewowi/MoonLight/blob/main/src/MoonLight/Nodes/Modifiers/M_MoonLight.h). class ModifierBase : public MoonModule { public: - ModuleRole role() const override { return ModuleRole::Modifier; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Modifier; } /// A modifier control change alters the mapping, so the owning Layer must rebuild /// it — the pipeline-wide rebuild path. See MoonModule::onControlChanged. diff --git a/src/light/modifiers/RandomMapModifier.h b/src/light/modifiers/RandomMapModifier.h index 0db3eb73..f9955509 100644 --- a/src/light/modifiers/RandomMapModifier.h +++ b/src/light/modifiers/RandomMapModifier.h @@ -73,7 +73,7 @@ class RandomMapModifier : public ModifierBase { // generation (so the next rebuild reshuffles) and trigger the Layer's rebuild. // bpm 0 freezes. Layer::tick() invokes this per enabled modifier child; it sets a // dirty flag the Layer coalesces into one rebuild (see Layer::tick()). - void tick() override { + void tick() MM_NONBLOCKING override { const uint32_t now = platform::millis(); if (lastElapsed_ == 0) lastElapsed_ = now; // first tick: no dt jump const uint32_t dt = now - lastElapsed_; diff --git a/src/light/modifiers/RotateModifier.h b/src/light/modifiers/RotateModifier.h index 81cd1129..8ed408d7 100644 --- a/src/light/modifiers/RotateModifier.h +++ b/src/light/modifiers/RotateModifier.h @@ -73,7 +73,7 @@ class RotateModifier : public ModifierBase { // the new angle on the next frame. The angle is uint8 turn units (256 = a turn); // dt·speed accumulates so a sub-ms frame isn't lost (the integer-accumulator idiom // the effects use). Layer::tick() invokes this per enabled modifier child. - void tick() override { + void tick() MM_NONBLOCKING override { const uint32_t now = platform::millis(); if (lastElapsed_ == 0) lastElapsed_ = now; const uint32_t dt = now - lastElapsed_; diff --git a/src/light/moonlive/MoonLiveEffect.h b/src/light/moonlive/MoonLiveEffect.h index 869c366b..0280ee95 100644 --- a/src/light/moonlive/MoonLiveEffect.h +++ b/src/light/moonlive/MoonLiveEffect.h @@ -73,7 +73,7 @@ class MoonLiveEffect : public EffectBase { setDynamicBytes(engine_.ok() ? engine_.codeCap() : 0); } - void tick() override { + void tick() MM_NONBLOCKING override { // The native emitter stores R,G,B at offsets +0/+1/+2 with channelsPerLight() only as the // stride (moonlive_lower_*: addr = index * cpl, then 3 writes). A 0/1/2-channel layer would // let the last light's +1/+2 write run past the buffer, so a sub-RGB layout renders dark. diff --git a/src/platform/desktop/platform_desktop.cpp b/src/platform/desktop/platform_desktop.cpp index 78aeacb2..16fa7c25 100644 --- a/src/platform/desktop/platform_desktop.cpp +++ b/src/platform/desktop/platform_desktop.cpp @@ -105,7 +105,19 @@ static std::atomic testNowMs{0}; void setTestNowMs(uint32_t ms) { testNowMs.store(ms, std::memory_order_relaxed); } -uint32_t millis() { +// steady_clock::now() is a vDSO clock_gettime read — no allocation, no lock, no syscall on +// any platform we build for. libc++ does not annotate it, so -Wfunction-effects has to assume +// the worst; this is the standard-library gap, not ours. Scoped to the two clock readers, and +// desktop-only (the ESP32 millis/micros call esp_timer_get_time directly). +// #ifdef-guarded: `#pragma clang ...` is an UNKNOWN PRAGMA to GCC, and the CI sanitizer lanes +// build with GCC + -Werror, so an unguarded clang pragma fails the build there. (`#pragma GCC` +// would be understood by both, but -Wfunction-effects is a clang-only warning, so the pragma +// must be clang-only too.) +#ifdef __clang__ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wfunction-effects" +#endif +uint32_t millis() MM_NONBLOCKING { uint32_t override_ = testNowMs.load(std::memory_order_relaxed); if (override_) return override_; auto now = std::chrono::steady_clock::now(); @@ -114,12 +126,15 @@ uint32_t millis() { ); } -uint32_t micros() { +uint32_t micros() MM_NONBLOCKING { auto now = std::chrono::steady_clock::now(); return static_cast( std::chrono::duration_cast(now - startTime).count() ); } +#ifdef __clang__ +#pragma clang diagnostic pop +#endif void* alloc(size_t bytes) { return std::malloc(bytes); @@ -510,12 +525,36 @@ int fsRead(const char* path, char* buf, size_t maxLen) { return static_cast(n); } +// Open a temp file for atomic-write, owner-only (0600) where the OS has file modes. +// +// std::fopen creates with 0666 & ~umask, so on a typical umask 022 the file lands world-readable +// — and these are /.config/*.json, which hold WiFi PSKs and MQTT passwords. Nothing here is +// multi-user on ESP32 (LittleFS has no modes at all, so the platform layer's ESP32 half is +// unaffected), but the desktop build runs on real machines with real other users. +// +// POSIX gets O_CREAT|O_EXCL with an explicit 0600 — EXCL because a pre-existing temp file is +// either a crashed run's leftover or someone else's, and inheriting its mode would defeat the +// point. Windows has no mode_t; its files inherit the parent directory's ACL, which is the +// platform's own answer to the same question, so it keeps plain fopen. +static FILE* openTempOwnerOnly(const char* path) { +#ifdef _WIN32 + return std::fopen(path, "wb"); +#else + ::unlink(path); // clear a leftover so O_EXCL cannot fail on our own temp + const int fd = ::open(path, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, 0600); + if (fd < 0) return nullptr; + FILE* f = ::fdopen(fd, "wb"); + if (!f) ::close(fd); // fdopen failure leaves the descriptor ours to release + return f; +#endif +} + bool fsWriteAtomic(const char* path, const char* data, size_t len) { auto target = toFsPath(path); auto tmp = target; tmp += ".tmp"; - FILE* f = std::fopen(tmp.string().c_str(), "wb"); + FILE* f = openTempOwnerOnly(tmp.string().c_str()); if (!f) return false; size_t written = std::fwrite(data, 1, len, f); if (written != len) { @@ -567,7 +606,7 @@ bool fsWriteStream(const char* path, FsWriteSrc src, void* user) { auto tmp = target; tmp += ".tmp"; - FILE* f = std::fopen(tmp.string().c_str(), "wb"); + FILE* f = openTempOwnerOnly(tmp.string().c_str()); if (!f) return false; // Pull chunks from the source and write each straight through — fixed buffer, any file size. // `abort` set by the source (a short/timed-out upload) means the data is incomplete → discard. @@ -638,9 +677,9 @@ size_t filesystemTotal() { void setEthConfig(const EthPinConfig&) {} // no eth on desktop; ethInit stubs false void ethStop() {} // no eth on desktop bool ethInit() { return false; } -bool ethLinkUp() { return false; } -bool ethConnected() { return false; } -void ethGetIPv4(uint8_t out[4]) { +bool ethLinkUp() MM_NONBLOCKING { return false; } +bool ethConnected() MM_NONBLOCKING { return false; } +void ethGetIPv4(uint8_t out[4]) MM_NONBLOCKING { // Desktop has no real interface state, but DevicesModule needs the host's LAN // IP to scan from (otherwise a desktop projectMM instance reports "no network" and // never sweeps). hostIp() resolves it via the outbound-route trick; report it @@ -668,7 +707,7 @@ void setTestWifiStaAvailable(bool available) { testWifiStaAvailable.store(availa bool wifiStaInit(const char* /*ssid*/, const char* /*password*/) { return testWifiStaAvailable.load(std::memory_order_relaxed); } -bool wifiStaConnected() { return false; } +bool wifiStaConnected() MM_NONBLOCKING { return false; } void wifiStaGetIPv4(uint8_t out[4]) { out[0] = out[1] = out[2] = out[3] = 0; } // Addressing is OS-managed on desktop; the static/DHCP setters are inert (no netif to reconfigure). // The per-interface apply counter is the observable a host test pins the static-addressing path on. @@ -1182,7 +1221,7 @@ bool rmtWs2812Init(RmtWs2812Handle& /*h*/, uint8_t /*gpio*/, uint32_t /*resoluti bool /*invert*/) { return false; } -uint32_t rmtWs2812Resolution(const RmtWs2812Handle& /*h*/) { return 0; } +uint32_t rmtWs2812Resolution(const RmtWs2812Handle& /*h*/) MM_NONBLOCKING { return 0; } bool rmtWs2812Transmit(RmtWs2812Handle& /*h*/, const uint32_t* /*symbols*/, size_t /*symbolCount*/) { return false; diff --git a/src/platform/esp32/platform_esp32.cpp b/src/platform/esp32/platform_esp32.cpp index 6eae7073..46c9af6b 100644 --- a/src/platform/esp32/platform_esp32.cpp +++ b/src/platform/esp32/platform_esp32.cpp @@ -90,13 +90,13 @@ void setTestNowMs(uint32_t ms) { testNowMs.store(ms, std::memory_order_relaxed); // Host-test hook (see platform.h); no ESP32 test drives a bind failure, so it is inert here. void setTestBindFails(bool) {} -uint32_t millis() { +uint32_t millis() MM_NONBLOCKING { uint32_t override_ = testNowMs.load(std::memory_order_relaxed); if (override_) return override_; return static_cast(esp_timer_get_time() / 1000); } -uint32_t micros() { +uint32_t micros() MM_NONBLOCKING { return static_cast(esp_timer_get_time()); } @@ -401,10 +401,16 @@ size_t flashChipSize() { static const char* NET_TAG = "mm_net"; -// Connection state tracked by event handlers +// Connection state tracked by event handlers. +// +// ATOMIC because these cross threads: the IDF event loop writes them, the render task reads +// them through ethLinkUp()/ethConnected() every tick. A plain bool there is a data race — benign +// in practice on this target, but undefined behaviour the compiler may fold or reorder, and +// TSan reports it. Relaxed ordering is enough: each flag is an independent state signal, nothing +// else is published through them. #ifndef MM_NO_ETH -static bool ethLinkUp_ = false; -static bool ethConnected_ = false; +static std::atomic ethLinkUp_{false}; +static std::atomic ethConnected_{false}; // Static-addressing state for Ethernet, so the CONNECTED handler restores the static IP on a // re-plug instead of letting IDF's per-link-up DHCP-client restart pull a lease. Set by // netSetStaticIPv4(Eth) (which stores the octets), cleared by netSetDhcp(Eth). @@ -474,13 +480,16 @@ static void applyHostname(esp_netif_t* netif) { } #ifndef MM_NO_WIFI -// WiFi-only state — absent in the Ethernet-only build. -static bool wifiStaConnected_ = false; +// WiFi-only state — absent in the Ethernet-only build. Atomic for the same reason as the eth +// pair: written by the IDF event loop, read from the render task. +static std::atomic wifiStaConnected_{false}; static bool wifiApActive_ = false; // L2 association state, distinct from wifiStaConnected_ (which means "has an IP"): true between // WIFI_EVENT_STA_CONNECTED and _DISCONNECTED. A static STA is reachable once associated (no DHCP // round), so this is the signal the static apply keys off — see netSetStaticIPv4(Sta). -static bool wifiStaAssociated_ = false; +// Atomic for the same reason as wifiStaConnected_ above: written by the IDF event handler, +// read by netSetStaticIPv4() on the caller's thread. +static std::atomic wifiStaAssociated_{false}; // Static-addressing state for WiFi STA, mirroring the eth pair. `wifiStaConnected_` normally means // "has a DHCP IP" (set on GOT_IP), which never fires on a DHCP-less network — so for a static STA // the address is pinned at L2 association (WIFI_EVENT_STA_CONNECTED) and connected is marked there. @@ -510,7 +519,7 @@ static void ethEventHandler(void* /*arg*/, esp_event_base_t base, if (base == ETH_EVENT) { if (id == ETHERNET_EVENT_CONNECTED) { ESP_LOGI(NET_TAG, "Ethernet link up"); - ethLinkUp_ = true; + ethLinkUp_.store(true, std::memory_order_relaxed); if (ethStatic_.load(std::memory_order_acquire)) { // Static mode: do NOT let the DHCP client restart on this link-up (applyHostname // would) — that is what made a re-plugged cable grab a DHCP lease instead of the @@ -528,16 +537,16 @@ static void ethEventHandler(void* /*arg*/, esp_event_base_t base, applyHostname(ethNetif_); } } else if (id == ETHERNET_EVENT_DISCONNECTED) { - ethLinkUp_ = false; + ethLinkUp_.store(false, std::memory_order_relaxed); ESP_LOGI(NET_TAG, "Ethernet link down"); - ethConnected_ = false; + ethConnected_.store(false, std::memory_order_relaxed); } else if (id == ETHERNET_EVENT_START) { ESP_LOGI(NET_TAG, "Ethernet started"); } } else if (base == IP_EVENT && id == IP_EVENT_ETH_GOT_IP) { auto* event = static_cast(data); ESP_LOGI(NET_TAG, "Ethernet got IP: " IPSTR, IP2STR(&event->ip_info.ip)); - ethConnected_ = true; + ethConnected_.store(true, std::memory_order_relaxed); } } @@ -860,8 +869,8 @@ void ethStop() { #ifdef MM_ETH_W5500 if (ethSpiActive_) { spi_bus_free(SPI2_HOST); ethSpiActive_ = false; } #endif - ethLinkUp_ = false; - ethConnected_ = false; + ethLinkUp_.store(false, std::memory_order_relaxed); + ethConnected_.store(false, std::memory_order_relaxed); } bool ethInit() { @@ -885,15 +894,15 @@ bool ethInit() { } } -bool ethLinkUp() { - return ethLinkUp_; +bool ethLinkUp() MM_NONBLOCKING { + return ethLinkUp_.load(std::memory_order_relaxed); } -bool ethConnected() { - return ethConnected_; +bool ethConnected() MM_NONBLOCKING { + return ethConnected_.load(std::memory_order_relaxed); } -void ethGetIPv4(uint8_t out[4]) { +void ethGetIPv4(uint8_t out[4]) MM_NONBLOCKING { netifIPv4(ethNetif_, out); } @@ -904,9 +913,9 @@ void ethGetIPv4(uint8_t out[4]) { void setEthConfig(const EthPinConfig&) {} void ethStop() {} bool ethInit() { return false; } -bool ethLinkUp() { return false; } -bool ethConnected() { return false; } -void ethGetIPv4(uint8_t out[4]) { out[0] = out[1] = out[2] = out[3] = 0; } +bool ethLinkUp() MM_NONBLOCKING { return false; } +bool ethConnected() MM_NONBLOCKING { return false; } +void ethGetIPv4(uint8_t out[4]) MM_NONBLOCKING { out[0] = out[1] = out[2] = out[3] = 0; } #endif // MM_NO_ETH @@ -931,13 +940,13 @@ static void wifiEventHandler(void* /*arg*/, esp_event_base_t base, // mark connected — a DHCP-less network never fires GOT_IP, so waiting for it would strand // a static STA. Mirrors the eth CONNECTED handler's ethStatic_ re-pin. DHCP mode is a // no-op here (the DHCP client runs and GOT_IP sets wifiStaConnected_ as before). - wifiStaAssociated_ = true; + wifiStaAssociated_.store(true, std::memory_order_relaxed); if (staStatic_.load(std::memory_order_acquire)) { netSetStaticIPv4(NetIface::Sta, staStaticIp_, staStaticGw_, staStaticMask_, staStaticDns_); } } else if (id == WIFI_EVENT_STA_DISCONNECTED) { - wifiStaConnected_ = false; - wifiStaAssociated_ = false; + wifiStaConnected_.store(false, std::memory_order_relaxed); + wifiStaAssociated_.store(false, std::memory_order_relaxed); // **The reconnect must be explicit — IDF does not do it for us.** Without this // esp_wifi_connect(), a dropped association is permanent: the device keeps rendering but // is unreachable until it is power-cycled, which for a controller in a ceiling is a real @@ -974,7 +983,7 @@ static void wifiEventHandler(void* /*arg*/, esp_event_base_t base, } else if (base == IP_EVENT && id == IP_EVENT_STA_GOT_IP) { auto* event = static_cast(data); ESP_LOGI(NET_TAG, "WiFi STA got IP: " IPSTR, IP2STR(&event->ip_info.ip)); - wifiStaConnected_ = true; + wifiStaConnected_.store(true, std::memory_order_relaxed); } } @@ -1095,8 +1104,8 @@ bool wifiStaInit(const char* ssid, const char* password) { return true; } -bool wifiStaConnected() { - return wifiStaConnected_; +bool wifiStaConnected() MM_NONBLOCKING { + return wifiStaConnected_.load(std::memory_order_relaxed); } void wifiStaGetIPv4(uint8_t out[4]) { @@ -1121,14 +1130,17 @@ void wifiStaStop() { esp_netif_destroy_default_wifi(staNetif_); staNetif_ = nullptr; } - wifiStaConnected_ = false; + wifiStaConnected_.store(false, std::memory_order_relaxed); + // Association state must clear with the interface: a later netSetStaticIPv4(Sta) keys off + // this flag, and a stale `true` from a torn-down STA would apply a static IP to nothing. + wifiStaAssociated_.store(false, std::memory_order_relaxed); wifiInitDone_ = false; wifiStaStopping_.store(false, std::memory_order_relaxed); // a later wifiStaInit() reconnects normally ESP_LOGI(NET_TAG, "WiFi STA stopped + deinit"); } int wifiStaRssi() { - if (!wifiStaConnected_) return 0; + if (!wifiStaConnected_.load(std::memory_order_relaxed)) return 0; wifi_ap_record_t info{}; if (esp_wifi_sta_get_ap_info(&info) != ESP_OK) return 0; return info.rssi; @@ -1136,13 +1148,13 @@ int wifiStaRssi() { void wifiStaBssid(uint8_t out[6]) { std::memset(out, 0, 6); - if (!wifiStaConnected_) return; + if (!wifiStaConnected_.load(std::memory_order_relaxed)) return; wifi_ap_record_t info{}; if (esp_wifi_sta_get_ap_info(&info) == ESP_OK) std::memcpy(out, info.bssid, 6); } int wifiStaChannel() { - if (!wifiStaConnected_) return 0; + if (!wifiStaConnected_.load(std::memory_order_relaxed)) return 0; wifi_ap_record_t info{}; if (esp_wifi_sta_get_ap_info(&info) != ESP_OK) return 0; return info.primary; @@ -1256,7 +1268,7 @@ bool wifiSetTxPower(int8_t quarterDbm) { // With hasWiFi==false the calls are not code-generated, so --gc-sections drops // these stubs from the final image. bool wifiStaInit(const char* /*ssid*/, const char* /*password*/) { return false; } -bool wifiStaConnected() { return false; } +bool wifiStaConnected() MM_NONBLOCKING { return false; } void wifiStaGetIPv4(uint8_t out[4]) { out[0] = out[1] = out[2] = out[3] = 0; } void wifiStaStop() {} int wifiStaRssi() { return 0; } @@ -1345,7 +1357,7 @@ void netSetStaticIPv4(NetIface iface, const uint8_t ip[4], const uint8_t gw[4], // Mark connected only if the link is actually up — else a static apply racing a cable pull // would leave ethConnected() true on a dead link (the state machine would sit in ConnectedEth // instead of cascading). On a genuine link-up the CONNECTED handler re-applies + sets it. - if (ethLinkUp_) ethConnected_ = true; + if (ethLinkUp_.load(std::memory_order_relaxed)) ethConnected_.store(true, std::memory_order_relaxed); } #endif #ifndef MM_NO_WIFI @@ -1360,7 +1372,7 @@ void netSetStaticIPv4(NetIface iface, const uint8_t ip[4], const uint8_t gw[4], // — the very case static addressing exists for. So mark connected here (the IP is applied); // WIFI_EVENT_STA_CONNECTED re-applies on a reconnect. Only when the STA is actually // associated, so a static apply while the radio is down doesn't fake a connection. - if (wifiStaAssociated_) wifiStaConnected_ = true; + if (wifiStaAssociated_.load(std::memory_order_relaxed)) wifiStaConnected_.store(true, std::memory_order_relaxed); } #endif ESP_LOGI(NET_TAG, "Static IPv4 set on %s: %u.%u.%u.%u", @@ -1375,7 +1387,7 @@ void netSetDhcp(NetIface iface) { #ifndef MM_NO_ETH if (iface == NetIface::Eth) { ethStatic_.store(false, std::memory_order_release); // link-up handler goes back to the DHCP hostname path - ethConnected_ = false; // static forced this true; drop it so the state machine re-evaluates + ethConnected_.store(false, std::memory_order_relaxed); // static forced this true; drop it so the state machine re-evaluates // (GOT_IP re-sets it on a lease). Else a Static→DHCP toggle on a // network that can't lease wedges in ConnectedEth at 0.0.0.0. } @@ -1383,7 +1395,7 @@ void netSetDhcp(NetIface iface) { #ifndef MM_NO_WIFI if (iface == NetIface::Sta) { staStatic_.store(false, std::memory_order_release); // stop re-pinning static on the next association - wifiStaConnected_ = false; // static forced this true; GOT_IP re-sets it once DHCP leases + wifiStaConnected_.store(false, std::memory_order_relaxed); // static forced this true; GOT_IP re-sets it once DHCP leases } #endif esp_netif_dhcpc_start(netif); // ignore ESP_ERR_ESP_NETIF_DHCP_ALREADY_STARTED diff --git a/src/platform/esp32/platform_esp32_rmt.cpp b/src/platform/esp32/platform_esp32_rmt.cpp index 37b60d71..c5d3923a 100644 --- a/src/platform/esp32/platform_esp32_rmt.cpp +++ b/src/platform/esp32/platform_esp32_rmt.cpp @@ -88,7 +88,7 @@ bool rmtWs2812Init(RmtWs2812Handle& h, uint8_t gpio, uint32_t resolutionHz, bool return true; } -uint32_t rmtWs2812Resolution(const RmtWs2812Handle& h) { +uint32_t rmtWs2812Resolution(const RmtWs2812Handle& h) MM_NONBLOCKING { auto* st = static_cast(h.impl); return st ? st->resolutionHz : 0; } diff --git a/src/platform/platform.h b/src/platform/platform.h index f68dbdd8..f95d76cd 100644 --- a/src/platform/platform.h +++ b/src/platform/platform.h @@ -5,10 +5,31 @@ #include #include "platform_config.h" // hasOta / hasPsram / … — flags this header's contract refers to +// The render path must not allocate or block (architecture.md § Hot path discipline). Clang 20+ +// checks that TRANSITIVELY under -Wfunction-effects: the attribute is inherited by overrides, so +// marking the three tick methods here covers every module's tick and everything it calls, which a +// regex over source text can never do — it sees a tick body, not what its callees reach. +// +// On GCC it expands to `noexcept` alone — that toolchain has neither the attribute nor the warning +// (so the effect check is desktop-only), but the exception contract still holds. It builds with +// -Werror, so a bare [[clang::nonblocking]] there is a build break (-Wattributes). Same shape as +// MM_PRINTF_FORMAT in JsonSink.h. That makes this a DESKTOP-side check, which loses nothing: every +// tick method — modules, effects, and the LED drivers — compiles on desktop. src/platform/esp32/ +// has no tick methods at all; it is free functions the tick path calls INTO, and those are checked +// through their call sites. +#if defined(__clang__) && __clang_major__ >= 20 + // noexcept is part of the contract, not decoration: clang warns + // (-Wperf-constraint-implies-noexcept) if a nonblocking function can throw, because + // unwinding allocates. Folded in here so the two never drift apart. + #define MM_NONBLOCKING noexcept [[clang::nonblocking]] +#else + #define MM_NONBLOCKING noexcept +#endif + namespace mm::platform { -uint32_t millis(); -uint32_t micros(); +uint32_t millis() MM_NONBLOCKING; +uint32_t micros() MM_NONBLOCKING; // Test-only override: when set to non-zero, millis() returns this value instead // of reading the platform clock. Production code never calls this; tests use it @@ -317,16 +338,16 @@ bool ethInit(); // the live reconfigure path (used for W5500/SPI, which tears down cleanly; RMII // keeps apply-on-next-init). Safe to call when nothing is running. Desktop: no-op. void ethStop(); -bool ethLinkUp(); // PHY link detected (cable plugged, fast check) -bool ethConnected(); // IP assigned (DHCP complete) +bool ethLinkUp() MM_NONBLOCKING; // PHY link detected (cable plugged, fast check) +bool ethConnected() MM_NONBLOCKING; // IP assigned (DHCP complete) // Current IP as raw octets — out[0..3]. All-zero (0.0.0.0) means "no IP yet". // Octets, not a string: the IP's canonical form is uint8_t[4] (matching the // static-IP controls and formatDottedQuad); callers that need text format at // their own boundary, callers that need bytes (ArtNet) use them directly. -void ethGetIPv4(uint8_t out[4]); +void ethGetIPv4(uint8_t out[4]) MM_NONBLOCKING; bool wifiStaInit(const char* ssid, const char* password); -bool wifiStaConnected(); +bool wifiStaConnected() MM_NONBLOCKING; void wifiStaGetIPv4(uint8_t out[4]); // see ethGetIPv4 — same octet contract void wifiStaStop(); @@ -625,7 +646,7 @@ bool rmtWs2812Init(RmtWs2812Handle& h, uint8_t gpio, uint32_t resolutionHz, bool // The tick resolution the platform actually granted (may differ from requested). // The driver converts its ns timings to ticks with this. 0 if not initialised. -uint32_t rmtWs2812Resolution(const RmtWs2812Handle& h); +uint32_t rmtWs2812Resolution(const RmtWs2812Handle& h) MM_NONBLOCKING; // Start transmitting `symbolCount` pre-encoded WS2812 RMT symbols and return // immediately — channels started back-to-back clock out concurrently. Pair with diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 33c1e16d..3e61d10d 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -134,6 +134,15 @@ add_custom_target(effect_sweep_gen ALL target_include_directories(mm_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}/generated) target_link_libraries(mm_tests PRIVATE mm_core mm_platform) + +# doctest::Approx takes a double, so every CHECK against a float literal promotes — inside +# doctest.h itself, which is vendored. -Wdouble-promotion stays -Werror for the firmware +# (the Xtensa has no FPU, so a stray promotion is a silent softfloat call in the render +# path); it just cannot also police a third-party header. Test-target only, and only the +# error, not the warning. Surfaced by the RTSan lane: clang diagnoses this where GCC does not. +if(NOT MSVC) + target_compile_options(mm_tests PRIVATE -Wno-error=double-promotion) +endif() add_dependencies(mm_tests effect_sweep_gen) add_test(NAME unit_tests COMMAND mm_tests) diff --git a/test/scenarios/light/scenario_peripheral_grid_sweep.json b/test/scenarios/light/scenario_peripheral_grid_sweep.json index c9f0d988..9f2ba908 100644 --- a/test/scenarios/light/scenario_peripheral_grid_sweep.json +++ b/test/scenarios/light/scenario_peripheral_grid_sweep.json @@ -156,7 +156,7 @@ "desktop-macos": { "tick_us": [ 4, - 14 + 21 ], "free_heap": [ 0, @@ -234,7 +234,7 @@ "desktop-macos": { "tick_us": [ 17, - 48 + 52 ], "free_heap": [ 0, @@ -390,7 +390,7 @@ "desktop-macos": { "tick_us": [ 273, - 1036 + 1115 ], "free_heap": [ 0, @@ -900,7 +900,7 @@ "desktop-macos": { "tick_us": [ 17, - 81 + 94 ], "free_heap": [ 0, @@ -1155,7 +1155,7 @@ "desktop-macos": { "tick_us": [ 4, - 13 + 19 ], "free_heap": [ 0, @@ -1310,7 +1310,7 @@ }, "desktop-macos": { "tick_us": [ - 69, + 68, 245 ], "free_heap": [ @@ -1389,7 +1389,7 @@ "desktop-macos": { "tick_us": [ 273, - 948 + 1357 ], "free_heap": [ 0, diff --git a/test/scenarios/light/scenario_peripheral_switch.json b/test/scenarios/light/scenario_peripheral_switch.json index c4859440..8b26beef 100644 --- a/test/scenarios/light/scenario_peripheral_switch.json +++ b/test/scenarios/light/scenario_peripheral_switch.json @@ -164,7 +164,7 @@ "desktop-macos": { "tick_us": [ 4, - 15 + 25 ], "free_heap": [ 0, @@ -344,7 +344,7 @@ "desktop-macos": { "tick_us": [ 4, - 13 + 21 ], "free_heap": [ 0, @@ -433,7 +433,7 @@ "desktop-macos": { "tick_us": [ 4, - 14 + 20 ], "free_heap": [ 0, @@ -523,7 +523,7 @@ "desktop-macos": { "tick_us": [ 4, - 17 + 30 ], "free_heap": [ 0, @@ -629,7 +629,7 @@ "desktop-macos": { "tick_us": [ 4, - 13 + 15 ], "free_heap": [ 0, diff --git a/test/unit/core/unit_Buffer.cpp b/test/unit/core/unit_Buffer.cpp index 579ab62f..4eb75507 100644 --- a/test/unit/core/unit_Buffer.cpp +++ b/test/unit/core/unit_Buffer.cpp @@ -39,7 +39,7 @@ TEST_CASE("Buffer move constructor") { CHECK(b.channelsPerLight() == 3); // Asserting the moved-from state IS the test: Buffer's move must leave the source empty, // not merely valid-but-unspecified. - // NOLINTNEXTLINE(bugprone-use-after-move) + // NOLINTNEXTLINE(bugprone-use-after-move,clang-analyzer-cplusplus.Move) CHECK(a.data() == nullptr); CHECK(a.count() == 0); } @@ -56,7 +56,7 @@ TEST_CASE("Buffer move assignment") { CHECK(b.count() == 100); // Asserting the moved-from state IS the test: Buffer's move must leave the source empty, // not merely valid-but-unspecified. - // NOLINTNEXTLINE(bugprone-use-after-move) + // NOLINTNEXTLINE(bugprone-use-after-move,clang-analyzer-cplusplus.Move) CHECK(a.data() == nullptr); } diff --git a/test/unit/core/unit_ModuleFactory.cpp b/test/unit/core/unit_ModuleFactory.cpp index 8e768a93..a74da953 100644 --- a/test/unit/core/unit_ModuleFactory.cpp +++ b/test/unit/core/unit_ModuleFactory.cpp @@ -10,19 +10,19 @@ namespace { // Real domain types are excluded so this test stays a unit test of the factory itself. class EffectStub : public mm::MoonModule { public: - mm::ModuleRole role() const override { return mm::ModuleRole::Effect; } + mm::ModuleRole role() const MM_NONBLOCKING override { return mm::ModuleRole::Effect; } }; class ModifierStub : public mm::MoonModule { public: - mm::ModuleRole role() const override { return mm::ModuleRole::Modifier; } + mm::ModuleRole role() const MM_NONBLOCKING override { return mm::ModuleRole::Modifier; } }; class DriverStub : public mm::MoonModule { public: - mm::ModuleRole role() const override { return mm::ModuleRole::Driver; } + mm::ModuleRole role() const MM_NONBLOCKING override { return mm::ModuleRole::Driver; } }; class LayoutStub : public mm::MoonModule { public: - mm::ModuleRole role() const override { return mm::ModuleRole::Layout; } + mm::ModuleRole role() const MM_NONBLOCKING override { return mm::ModuleRole::Layout; } }; class GenericStub : public mm::MoonModule { // No override — inherits ModuleRole::Generic. diff --git a/test/unit/core/unit_MoonModule_lifecycle.cpp b/test/unit/core/unit_MoonModule_lifecycle.cpp index 0b7d641d..187505ac 100644 --- a/test/unit/core/unit_MoonModule_lifecycle.cpp +++ b/test/unit/core/unit_MoonModule_lifecycle.cpp @@ -27,15 +27,15 @@ class Counting : public mm::MoonModule { uint32_t tick20msCalls = 0; uint32_t tick1sCalls = 0; - void tick() override { + void tick() MM_NONBLOCKING override { loopCalls++; mm::MoonModule::tick(); // chain so this counter doubles as a parent too } - void tick20ms() override { + void tick20ms() MM_NONBLOCKING override { tick20msCalls++; mm::MoonModule::tick20ms(); } - void tick1s() override { + void tick1s() MM_NONBLOCKING override { tick1sCalls++; mm::MoonModule::tick1s(); } @@ -45,7 +45,7 @@ class Counting : public mm::MoonModule { // NetworkModule / SystemModule / FirmwareUpdateModule today. class AlwaysOn : public Counting { public: - bool respectsEnabled() const override { return false; } + bool respectsEnabled() const MM_NONBLOCKING override { return false; } }; } // namespace diff --git a/test/unit/core/unit_NetworkModule.cpp b/test/unit/core/unit_NetworkModule.cpp index 8ecebafc..a2a1b9eb 100644 --- a/test/unit/core/unit_NetworkModule.cpp +++ b/test/unit/core/unit_NetworkModule.cpp @@ -98,7 +98,11 @@ TEST_CASE("NetworkModule mode control reflects current state") { // parseDottedQuad (in Control.h) is the validator on every IPv4 write, // over both the HTTP API and persistence. Pin the contract. TEST_CASE("parseDottedQuad accepts valid dotted-quads and rejects junk") { - uint8_t out[4]; + // Zero-initialized: the analyzer cannot see that parseDottedQuad fills every octet on + // success, so it reads the CHECKs below as comparing garbage. Cheaper to state the + // starting value than to argue about it, and a failing parse then compares against a + // known 0 rather than whatever was on the stack. + uint8_t out[4] = {}; CHECK(mm::parseDottedQuad("0.0.0.0", out)); CHECK((out[0] == 0 && out[1] == 0 && out[2] == 0 && out[3] == 0)); diff --git a/test/unit/core/unit_Services.cpp b/test/unit/core/unit_Services.cpp index 4ae904ed..7e183825 100644 --- a/test/unit/core/unit_Services.cpp +++ b/test/unit/core/unit_Services.cpp @@ -18,7 +18,7 @@ using namespace mm; namespace { // A stand-in Service-role child (what Audio/IR are): the container accepts it by role. struct FakeService : MoonModule { - ModuleRole role() const override { return ModuleRole::Service; } + ModuleRole role() const MM_NONBLOCKING override { return ModuleRole::Service; } }; } // namespace diff --git a/test/unit/core/unit_SystemModule.cpp b/test/unit/core/unit_SystemModule.cpp index 9e140c69..5600cc24 100644 --- a/test/unit/core/unit_SystemModule.cpp +++ b/test/unit/core/unit_SystemModule.cpp @@ -14,8 +14,8 @@ class CountingChild : public mm::MoonModule { public: uint32_t setupCalls = 0, tick20msCalls = 0, tick1sCalls = 0; void setup() override { setupCalls++; } - void tick20ms() override { tick20msCalls++; } - void tick1s() override { tick1sCalls++; } + void tick20ms() MM_NONBLOCKING override { tick20msCalls++; } + void tick1s() MM_NONBLOCKING override { tick1sCalls++; } }; } // namespace diff --git a/test/unit/light/unit_Drivers_container.cpp b/test/unit/light/unit_Drivers_container.cpp index a527d4f3..446d7e8f 100644 --- a/test/unit/light/unit_Drivers_container.cpp +++ b/test/unit/light/unit_Drivers_container.cpp @@ -22,7 +22,7 @@ namespace { class CountingDriver : public mm::DriverBase { public: void setSourceBuffer(mm::Buffer*) override {} - void tick() override { loopCalls++; } + void tick() MM_NONBLOCKING override { loopCalls++; } int loopCalls = 0; }; @@ -34,7 +34,7 @@ class CountingDriver : public mm::DriverBase { class CorrectionCapturingDriver : public mm::DriverBase { public: void setSourceBuffer(mm::Buffer*) override {} - void tick() override {} + void tick() MM_NONBLOCKING override {} void defineDriverControls() override { defineCorrectionControls(); } }; @@ -44,7 +44,7 @@ class CorrectionCapturingDriver : public mm::DriverBase { class RebuildTrackingDriver : public mm::DriverBase { public: void setSourceBuffer(mm::Buffer*) override {} - void tick() override {} + void tick() MM_NONBLOCKING override {} void defineDriverControls() override { defineCorrectionControls(); } void prepare() override { prepareCalls++; } void onCorrectionChanged() override { correctionCalls++; } diff --git a/test/unit/light/unit_Drivers_rendersplit.cpp b/test/unit/light/unit_Drivers_rendersplit.cpp index 4af9be6d..e2c18fdb 100644 --- a/test/unit/light/unit_Drivers_rendersplit.cpp +++ b/test/unit/light/unit_Drivers_rendersplit.cpp @@ -32,7 +32,7 @@ namespace { class MockDriver : public mm::DriverBase { public: void setSourceBuffer(mm::Buffer* b) override { src_ = b; } - void tick() override { + void tick() MM_NONBLOCKING override { ticks.fetch_add(1); if (src_ && src_->data() && src_->count() > 0 && src_->data()[0] == 0xEE) sawTear.store(true); @@ -50,7 +50,12 @@ class MockDriver : public mm::DriverBase { class SlowDriver : public mm::DriverBase { public: void setSourceBuffer(mm::Buffer*) override {} - void tick() override { + // DELIBERATELY blocking, despite MM_NONBLOCKING: the mutex and timed wait ARE the test. + // They hold the use-after-free window open on demand, which is the only way to observe + // the race this file pins. The annotation is inherited from MoonModule::tick and cannot + // be dropped without breaking the override, so clang-hotpath will report these lines — + // that report is correct and expected here. + void tick() MM_NONBLOCKING override { { std::unique_lock lk(m); inTick = true; diff --git a/test/unit/light/unit_Effects_gridsweep.cpp b/test/unit/light/unit_Effects_gridsweep.cpp index a0197751..4930d3ed 100644 --- a/test/unit/light/unit_Effects_gridsweep.cpp +++ b/test/unit/light/unit_Effects_gridsweep.cpp @@ -8,6 +8,16 @@ // grid to 0x0x0 (every layout child disabled), and a 1-wide or 1-deep grid is what a // single strand or a flat panel actually is. // +// Layer::tick owns the degenerate-grid gate: it skips the effect pass when any axis is 0, +// so effects may assume width/height/depth >= 1 and carry no such guard themselves. The +// 0x0x0 row therefore pins the LAYER's behaviour, not each effect's: that a folded-away +// grid produces a clean no-op and a zero-byte buffer rather than a crash. Effect bodies +// are covered by the three non-degenerate rows, where they do run. +// +// Do NOT "improve" this by calling fx->tick() directly on the zero grid. That asserts a +// contract no effect makes, and it fails: GEQ3DEffect divides by width() (imap over a +// zero range) and takes SIGFPE. The single Layer gate is what makes that safe. +// // Per-effect tests pin this one effect at a time, which means a NEW effect is covered only // if its author remembers to write that case. This sweep asks the factory for every // registered effect instead, so the floor is enforced for effects that do not exist yet: diff --git a/test/unit/light/unit_Layer_extrude.cpp b/test/unit/light/unit_Layer_extrude.cpp index f38e328d..22c863d5 100644 --- a/test/unit/light/unit_Layer_extrude.cpp +++ b/test/unit/light/unit_Layer_extrude.cpp @@ -67,7 +67,7 @@ TEST_CASE("D2 effect on 3D grid: z-slices are identical (Layer::extrude)") { class D1StubEffect : public mm::EffectBase { public: mm::Dim dimensions() const override { return mm::Dim::D1; } - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); mm::lengthType w = width(); mm::lengthType h = height(); diff --git a/test/unit/light/unit_Layer_live_modifier.cpp b/test/unit/light/unit_Layer_live_modifier.cpp index c740d2f5..27ac2d75 100644 --- a/test/unit/light/unit_Layer_live_modifier.cpp +++ b/test/unit/light/unit_Layer_live_modifier.cpp @@ -29,7 +29,7 @@ namespace { // the live pass, not the effect animating itself. class GradientEffect : public mm::EffectBase { public: - void tick() override { + void tick() MM_NONBLOCKING override { uint8_t* buf = buffer(); if (!buf) return; const mm::lengthType w = width(), h = height(); diff --git a/test/unit/light/unit_Layer_persistence.cpp b/test/unit/light/unit_Layer_persistence.cpp index f6219c80..f7d73f05 100644 --- a/test/unit/light/unit_Layer_persistence.cpp +++ b/test/unit/light/unit_Layer_persistence.cpp @@ -23,7 +23,7 @@ struct WriteOnceEffect : mm::EffectBase { int calls = 0; const char* tags() const override { return ""; } mm::Dim dimensions() const override { return mm::Dim::D3; } - void tick() override { + void tick() MM_NONBLOCKING override { if (calls++ == 0) { mm::Buffer& b = layer()->buffer(); mm::Coord3D dims{width(), height(), depth()}; @@ -38,7 +38,7 @@ struct FadeOnlyEffect : mm::EffectBase { uint8_t amt = 0; const char* tags() const override { return ""; } mm::Dim dimensions() const override { return mm::Dim::D3; } - void tick() override { layer()->fadeToBlackBy(amt); } + void tick() MM_NONBLOCKING override { layer()->fadeToBlackBy(amt); } }; struct Scene { @@ -113,7 +113,7 @@ TEST_CASE("Layer: collected fade resets after it is consumed") { int n = 0; const char* tags() const override { return ""; } mm::Dim dimensions() const override { return mm::Dim::D3; } - void tick() override { if (n++ == 0) layer()->fadeToBlackBy(128); } + void tick() MM_NONBLOCKING override { if (n++ == 0) layer()->fadeToBlackBy(128); } } oneFade; s.layer.addChild(&once); s.layer.addChild(&oneFade); diff --git a/test/unit/light/unit_Layouts_toggle_cycle.cpp b/test/unit/light/unit_Layouts_toggle_cycle.cpp index 810cb55c..98f2a20e 100644 --- a/test/unit/light/unit_Layouts_toggle_cycle.cpp +++ b/test/unit/light/unit_Layouts_toggle_cycle.cpp @@ -17,7 +17,7 @@ namespace { class CaptureDriver : public mm::DriverBase { public: void setSourceBuffer(mm::Buffer* buf) override { source_ = buf; } - void tick() override { + void tick() MM_NONBLOCKING override { if (source_ && source_->data()) { lastBytes = source_->bytes(); lastNonZero = false; diff --git a/test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp b/test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp index aded09db..cda4b0af 100644 --- a/test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp +++ b/test/unit/light/unit_ParallelLedDriver_doublebuffer.cpp @@ -45,7 +45,7 @@ class MockPeripheral : public mm::LedPeripheral { std::vector calls; // transmit/wait order, in call sequence // --- LedPeripheral hooks (mock, host-only) --- - uint8_t lanesAvailable() const override { return 8; } // pretend this chip has lanes + uint8_t lanesAvailable() const MM_NONBLOCKING override { return 8; } // pretend this chip has lanes bool powerOfTwoBus() const override { return false; } bool loopbackFullWidth() const override { return false; } // The mock bus is memory, not a peripheral, so it can host the 74HCT595 expander — which is what diff --git a/test/unit/light/unit_ParallelLedDriver_pinexpander.cpp b/test/unit/light/unit_ParallelLedDriver_pinexpander.cpp index 8d7349b3..8f95b411 100644 --- a/test/unit/light/unit_ParallelLedDriver_pinexpander.cpp +++ b/test/unit/light/unit_ParallelLedDriver_pinexpander.cpp @@ -28,7 +28,7 @@ using mm::nrOfLightsType; // (powerOfTwoBus true — the expander only ever runs on an i80-shaped bus). class MockPeripheral : public mm::LedPeripheral { public: - uint8_t lanesAvailable() const override { return 8; } // 8 data lines, like an 8-bit bus + uint8_t lanesAvailable() const MM_NONBLOCKING override { return 8; } // 8 data lines, like an 8-bit bus bool powerOfTwoBus() const override { return true; } // the BUS rounds to 8/16 whatever the pin count bool loopbackFullWidth() const override { return false; } bool supportsPinExpander() const override { return true; } // memory bus: the expander is allowed diff --git a/test/unit/light/unit_ParallelLedDriver_ring.cpp b/test/unit/light/unit_ParallelLedDriver_ring.cpp index 8649b384..024ebc04 100644 --- a/test/unit/light/unit_ParallelLedDriver_ring.cpp +++ b/test/unit/light/unit_ParallelLedDriver_ring.cpp @@ -53,7 +53,7 @@ class MockRingDriver; // forward decl — the peripheral's ring hooks call bac // MoonI80Peripheral::ringEncodeTrampoline does. class MockRingPeripheral : public mm::LedPeripheral { public: - uint8_t lanesAvailable() const override { return 8; } // 8 data lines (an 8-bit bus) + uint8_t lanesAvailable() const MM_NONBLOCKING override { return 8; } // 8 data lines (an 8-bit bus) bool powerOfTwoBus() const override { return true; } bool loopbackFullWidth() const override { return false; } bool supportsPinExpander() const override { return true; } @@ -103,7 +103,7 @@ class MockRingPeripheral : public mm::LedPeripheral { for (auto& f : needsPrefill_) f = true; return true; } - bool busIsRing() const override { return ringActive_; } + bool busIsRing() const MM_NONBLOCKING override { return ringActive_; } bool busTransmitRing() override { return true; } // the wire itself is the platform's; the encode is what we test // Replay one frame through the ring exactly as the platform does: prime the first min(N, needed) diff --git a/test/unit/light/unit_ParallelLedDriver_swap.cpp b/test/unit/light/unit_ParallelLedDriver_swap.cpp index 4875ea7c..63cebee4 100644 --- a/test/unit/light/unit_ParallelLedDriver_swap.cpp +++ b/test/unit/light/unit_ParallelLedDriver_swap.cpp @@ -60,7 +60,7 @@ struct SwapMock : mm::LedPeripheral { if (owner_) { attachedDtors++; g_hookFiredBeforeLastDtor = (g_hookFires > 0); } } - uint8_t lanesAvailable() const override { return 8; } + uint8_t lanesAvailable() const MM_NONBLOCKING override { return 8; } bool powerOfTwoBus() const override { return true; } bool loopbackFullWidth() const override { return false; } bool supportsPinExpander() const override { return false; } diff --git a/test/unit/light/unit_PreviewDriver.cpp b/test/unit/light/unit_PreviewDriver.cpp index 0014e635..1014fc1a 100644 --- a/test/unit/light/unit_PreviewDriver.cpp +++ b/test/unit/light/unit_PreviewDriver.cpp @@ -26,6 +26,13 @@ namespace { // WS header (begin is given the PAYLOAD length, so what's pushed is exactly the payload), and // classifies by first byte at end. dropCoord/acceptNext make endBinaryFrame report a client that // didn't get the whole frame (false) to drive the coord-pending retry + adaptive-downscale paths. +// -Wnon-virtual-dtor: BinaryBroadcaster's own destructor is protected and non-virtual on +// purpose ("not owned through this interface"), so no code can delete through a base pointer. +// This double is a stack local in every test, never owned polymorphically — and it cannot copy +// the base's protected-destructor trick, because that would forbid the stack construction the +// tests rely on. Scoped to this one type. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wnon-virtual-dtor" struct CaptureBroadcaster : mm::BinaryBroadcaster { int coordMsgs = 0, frameMsgs = 0; std::vector lastCoord, lastFrame; @@ -96,6 +103,7 @@ struct CaptureBroadcaster : mm::BinaryBroadcaster { mutable bool active_ = false; // a buffered send is in flight mutable int remaining_ = 0; // bufferedSendIdle polls left before it goes idle }; +#pragma GCC diagnostic pop // Wire PreviewDriver under Drivers, over a Layer + single layout, with a // CaptureBroadcaster — the full real path (sparse driver buffer + layout coords).