feat(room-node): add live diagnostics menu - #21
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed August 8, 2026, 3:15 AM ET / 07:15 UTC. ClawSweeper reviewWhat this changesThe PR adds a local room-node diagnostics overlay, USB REPL controls, audio-path meters, and a Talk-serialized production-path speaker test. Merge readinessKeep open for maintainer product-direction review: this is a coherent, hardware-proven room-node capability, but it adds a large always-bundled diagnostics surface rather than repairing established behavior. Priority: P2 Review scores
Verification
How this fits togetherRoom-node firmware connects local microphone/AFE capture, Talk playback, the display UI, and codec hardware. The new overlay reads owner-recorded runtime snapshots and gives a nearby operator local evidence about those paths. flowchart LR
A[Microphone and codec input] --> B[AFE and capture processing]
C[Talk playback] --> D[Renderer and codec output]
B --> E[Diagnostics snapshots]
D --> E
F[Status screen or USB REPL] --> G[Diagnostics overlay]
E --> G
G --> H[Local operator evidence]
Decision needed
Why: The implementation is coherent and physically demonstrated, but it introduces a substantial always-shipped UI and runtime surface that current main does not define. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: If maintainers want on-device diagnostics as a core room-node capability, merge this shared-owner approach after accepting the cross-board availability risk and scheduling a Waveshare device smoke test. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug report. The contributor supplied a high-confidence exact-head physical Tab5 run for the newly added behavior. Is this the best way to solve the issue? Unclear pending product direction: the shared snapshot design avoids board-specific debug screens, but maintainers must choose whether this large local diagnostics surface belongs in core firmware. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 1cb1a17eb28c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
Physical-device proof for reviewed head The transcript below is captured from the primary USB Serial/JTAG console. ANSI cursor traffic, Wi-Fi identity/signal, addresses, authentication details, room audio, and camera imagery are omitted. No firmware lines are synthesized. The camera invocation during the open modal returned a The 500 ms production-path tone was also recorded with a separate room microphone and analyzed locally. The recording remains private; this is the exact spectral-analysis output: Together this proves the after-fix path on hardware: asynchronous modal construction without reset, live MIC/AFE movement, 25/25/25 tone delivery with zero renderer errors, analog approximately-1 kHz speaker output, camera coexistence, and tone rejection while Talk owns media. All five exact-head CI build jobs are also green. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
Maintainer decision: sponsor the shared diagnostics interface as supported room-node functionality. Local observability is part of the room-node operator contract: when Talk is silent, the person beside the node needs an on-device answer for microphone, AFE, playback, renderer, codec-path, ownership, and runtime state. The shared owner-recorded snapshots and single modal are the canonical implementation; board-specific debug screens or a remote gateway command would be a worse ownership boundary. I accept the exact-head Tab5 physical proof plus all five green target builds as sufficient for this change. The Waveshare 410x502 physical gesture/layout smoke test remains a worthwhile follow-up when that hardware is available, but is not a blocker for the shared implementation: the existing Waveshare firmware builds successfully on this head, retains its animated status path, and the modal/input code is common. No code changed after the clean source review, exact-head CI, and sufficient physical proof. Proceeding to merge PR #21; issue #20 should close through the PR linkage. |
1 participant
What Problem This Solves
When Talk connects but the room is silent, the firmware previously exposed no local evidence showing whether audio reached the microphone, AFE, decoded playback, renderer, or codec path. Operators had to infer health from connection state and logs.
Closes #20
Why This Change Was Made
This adds one shared diagnostics owner to the room runtime rather than board-specific debug screens or log parsing.
lv_layer_top(); the underlying screen and state remain intact.av_render -> render tap -> I2S renderer -> esp_codec_dev -> ES8388.renderer accepted; software cannot prove analog speaker output.diagnostics open|close|tone|status. There is no new remote node command or gateway allowlist surface.The modal is not a third screen or a new room UI state. Cached scalar snapshots are updated by their owners under small locks; the LVGL timer performs no I²C, filesystem, JSON, command, or network work.
User Impact
A person standing at the room node can now answer:
On non-animated boards such as Tab5, the status screen shows
Hold for diagnosticsso the gesture is discoverable.Evidence
Automated
git diff --check-Werror: PASSPhysical Tab5
160x280JPEG), proving shared screen/privacy coexistence.25/25/25 requested/enqueued/renderer accepted, with zero renderer errors and RX/SPK meter86/100.MIC 11/100,AFE 7/100) with fresh timestamps.busyand enqueued zero frames; Talk then stopped cleanly.UI geometry proof
No room-camera screenshot or audio artifact is uploaded. The physical modal contains:
Diagnosticsheader and largeClosebutton.Play test tonebutton.The layout was exercised on the connected 1280x720 ST7123 Tab5. Waveshare remains build-proven; its 410x502 touch layout needs the next available physical-device smoke test.
Size
Production C/H: approximately +1,474 net lines. Tests: +28. Docs/build metadata: +27. The growth is the requested capability and its owner-recorded telemetry, not duplicated board policy.