Skip to content

chore: add bounded Tab5 media stage diagnostics - #47

Merged
vincentkoc merged 3 commits into
mainfrom
diag/tab5-media-stages
Sep 12, 2026
Merged

vincentkoc merged 3 commits into
mainfrom
diag/tab5-media-stages

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 9, 2026

Copy link
Copy Markdown
Member

Problem

Tab5 camera capture currently collapses several BSP/V4L2 failures into one unavailable result, while Talk setup lacks enough stage evidence to distinguish signaling, HTTP, peer startup and teardown. The physical media failure causes remain unproven; this is diagnostic instrumentation, not a camera or RTC repair.

Stacked on #46 (fix/tab5-sdio-rx-psram). This PR does not duplicate or change that branch's SDK/component patches or allocator policy.

Changes

  • Capture immediate syscall errno separately from BSP and validation failures, retain existing numeric format/length details, and emit one before-first-dequeue marker plus one terminal camera failure record.
  • Record fixed Talk callback, descriptor, HTTP and answer stages, preserving the first observed local failure and distinguishing absent/canceled callbacks from executed callbacks.
  • Record existing room-generation startup, peer and teardown boundaries with cumulative audio-counter snapshots. These counters are not network-delivery or audible-output proof.
  • Suppress the pinned SDK's outbound-SDP INFO logging before negotiation without changing global logging or raising a quieter tag level. Use the per-tag setter only where supported, retain effective-level readback, and require compile-time INFO exclusion in static mode. Refuse via existing teardown if the policy is unsafe.
  • Extend actual-owner host fixtures and route the camera regression through the existing CI job.

The log-policy refusal is the only intentional runtime behavior change. Public ABI, SDK/configuration, media ownership, cleanup, capture policy, timeouts, retries, callback return handling and existing ignored HTTP setter returns remain unchanged. Records contain fixed stages, numeric results and flags, not SDP, URLs, credentials, identities or remote error text. Room results retain native stage semantics: timer pdPASS=1 is success, and peer enum values are not error codes.

Logging Review Follow-up

Head e0ca61f313316a2d1c6bd6bc16fd1b18a879b364 fixes the no-tag setter changing global verbosity and preserves already quieter policies. It also rejects static logging with a WARN default but an INFO compiled maximum: the getter alone is not evidence that INFO cannot be emitted. No SDK or configuration change is required.

The actual-owner no-tag regression failed before the repair, and 48 lifecycle cases passed across the initial logging modes. The additional static-mode regression failed on the prior guard and passes with the correction; all four focused tagged, no-tag, static-unsafe and static-quiet cases pass with ASan/UBSan. Existing teardown, no-negotiation refusal and global-level preservation are checked. Fresh scoped and independent delta reviews are clear. New-head CI, attempt 1, passed all five builds. The earlier hosted and physical evidence below remains tied to its original source/image, not a new live test of this repair.

Validation

  • 15 Talk Unity host tests passed under ASan/UBSan.
  • 24 threaded HTTP/callback/ownership cases passed under ASan/UBSan and TSan; positional/designated public-ABI checks passed.
  • 39 room lifecycle cases plus queue/boundary checks passed under ASan/UBSan.
  • 31 actual camera capture/cleanup scenarios passed under ASan/UBSan. The baseline lacks the required diagnostic records and fails the same checks.
  • The pre-readback owner fails the ignored-log-setter privacy boundary; the new guard prevents WebRTC open and preserves local cleanup.
  • actionlint, diff whitespace checks and the public-data scrub passed. Test dependencies use the pinned SDK/WebRTC/Unity sources and registry-verified source/header bytes.

The Mach-O-only ROM fixture adaptation changes no firmware memory placement. All five jobs passed in CI run 34318348917, attempt 1 for PR head 32697940a996a742390407142828e6397b80fe24. Tab5 firmware was compiled from test merge 9ec6242a08f97c6c450ef4d3df48302c77aad7b7; its parents match this head and the PR46 base, and its tree matches this head.

The downloaded Tab5 firmware archive matches the GitHub artifact SHA256. All 11 file checksums and four flash-image offsets were verified; SDK/component patch metadata, configuration and the partition table match PR46, and the flash ranges exclude NVS.

Bounded Hardware Proof

  • On September 9, 2026 at 09:30:23 UTC, the verified 9ec6242a08f97c6c450ef4d3df48302c77aad7b7 image was flashed to M5Stack Tab5. All four writes and four independent readbacks passed. NVS, PHY, storage, C6 firmware and eFuses were preserved; no full-device erase or force override was used.
  • The same image, without reflashing, completed 226.103 seconds of continuous keyless diagnostic capture with the expected boot and one controlled reset. Phase A was stable for 64.796 seconds, followed by one Gateway restart.
  • Both roles connected in the post-restart phase B window of 49.717 seconds, with 40.127 seconds of measured stability. This is not a full 60-second post-restart qualification.
  • The capture contained eight valid NVS records and no observed assertion, panic, SDIO, NVS or parser fault. An earlier 13.157-second attempt was unqualified because of a 213-second pre-execution orchestration gap, not a demonstrated firmware defect; the launch orchestration was corrected without changing firmware.

Independent review accepted the bounded keyless evidence for one diagnostic Talk attempt, not production qualification. The subsequent bounded trial stopped before talk.start dispatch because the test verifier rejected a connect-diagnostic record. No assertion or SDIO fault was observed, and no RTC or media result was obtained. This rejection does not establish a PR47 firmware defect. No OpenAI/provider request was verified for that trial; its actual provider request count remains unknown. These observations do not establish camera, RTC or audible-output qualification.

Maintainer Disposition

On September 12, 2026, accept the intentional fail-closed logging policy and
the reviewed default-profile builds plus actual-owner mode regressions.
Custom no-tag dynamic INFO builds and static builds that compile INFO or
higher must use a safe logging configuration before Talk can start; preserving
potential SDP disclosure is not an acceptable compatibility fallback.
Native fresh-build and upgrade qualification of every custom logging mode
remains a limitation, not a claimed test pass. The accepted source repair
preserves global verbosity and existing teardown.

Land this bounded diagnostics/privacy change with a merge commit and retain
its signed source history. This disposition does not assert successful
end-to-end voice or full media qualification and does not authorize a release.

@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 9, 2026
@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 12, 2026, 11:50 AM ET / 15:50 UTC (Revision 8).

ClawSweeper review

What this changes

Adds bounded camera and Talk diagnostics, prevents unsafe SDK logging during Talk setup, and carries the stacked SDIO PSRAM allocation repair with packaging and regression coverage.

Merge readiness

Blocked before merge - 2 items remain

The diagnostics remain useful and absent from main. The prior logging defect is repaired, and the recorded maintainer disposition accepts the compatibility tradeoffs; no new blocking defect was found.

Priority: P2
Reviewed head: e0ca61f313316a2d1c6bd6bc16fd1b18a879b364

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A useful, well-scoped implementation with focused regression coverage, the prior defect repaired, and explicitly accepted qualification limits.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof, and the diff does not materially change authorization authority. Reported earlier-image boot/reconnect observations do not prove camera or Talk stages or the final logging repair; owner-level regressions and the explicit maintainer acceptance support this bounded change.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof, and the diff does not materially change authorization authority. Reported earlier-image boot/reconnect observations do not prove camera or Talk stages or the final logging repair; owner-level regressions and the explicit maintainer acceptance support this bounded change.
Evidence reviewed 10 items Policy and checkout: No root or nested AGENTS.md or maintainer-notes directory was found in the checkout or fetched main tree. The final working-tree status was clean.
Verified introduction boundary: The merge-base-to-head diff contains 27 files, including the inherited SDIO allocation commit. The raw test-merge object has exactly the pinned main parent followed by the pinned PR head, matching the host-provided merge-result evidence; base-only release and UI changes are not attributed to this PR.
Prior finding resolved: The current guard avoids the setter for no-tag logging, preserves quieter tag levels, checks effective verbosity, and requires static builds to exclude INFO. An initial local historical comparison could not load a missing blob; the exact commit patch was subsequently inspected through GitHub.
Findings None None.
Security None None.

How this fits together

The room-node firmware handles Gateway camera commands and voice calls through board capture, WebRTC signaling, and local audio resources. These changes expose fixed diagnostic stages and control logging before negotiation; the included SDIO repair changes where receive buffers are allocated.

flowchart TD
  A[Gateway commands and wake events] --> B[Room-node media controller]
  B --> C[Camera capture]
  B --> D{Safe SDK logging?}
  D -->|Yes| E[WebRTC signaling and audio]
  D -->|No| F[Local teardown]
  C --> G[Bounded diagnostic records]
  E --> G
  F --> G
  H[SDIO receive buffers in PSRAM] --> E
Loading

Before merge

  • Resolve merge risk (P1) - Existing custom no-tag dynamic INFO builds and static builds compiling INFO or higher will refuse Talk until logging is configured safely. The maintainer accepts this behavior; native fresh-build and upgrade qualification across every custom mode remains incomplete.
  • Resolve merge risk (P2) - The inherited SDIO repair moves eligible receive buffers to PSRAM without an internal-DMA fallback; exhaustion can still reach the retained assertion. Representative receive-pressure and exhaustion qualification remain accepted follow-ups.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Firmware C +219 lines; tests and harnesses +992; build/patch tooling +89; docs +75 The growth supports fixed diagnostic records, privacy enforcement, and the inherited allocation repair, with most added lines devoted to regression coverage.

Merge-risk options

Maintainer options:

  1. Honor the recorded bounded acceptance (recommended)
    Land under the explicit logging and PSRAM tradeoffs already accepted by the maintainer, retaining the documented hardware qualification limits.
  2. Wait for broader device qualification
    Pause landing if native custom-logging upgrades and receive-pressure qualification are desired before accepting the documented limits.

Technical review

Best possible solution:

Retain the bounded diagnostics and accepted privacy guard with the pinned allocation contract, preserve signed history through the requested merge commit, and keep release qualification separate.

Do we have a high-confidence way to reproduce the issue?

Not applicable to the primary diagnostic change; the underlying physical camera and RTC failures are not claimed as reproduced or repaired here. Source inspection confirms the prior logging defect is addressed.

Is this the best way to solve the issue?

Yes. Fixed-field instrumentation preserves existing media ownership and callback behavior, and the maintainer explicitly accepts refusal instead of allowing potentially private SDP logging.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against dc5c8917467a.

Labels

Label changes:

  • add merge-risk: 🚨 availability: The included receive-buffer placement changes the allocation arena, while PSRAM exhaustion and receive-pressure behavior remain incompletely qualified.

Label justifications:

  • P2: This is a bounded diagnostic and firmware reliability improvement without a demonstrated urgent regression.
  • merge-risk: 🚨 compatibility: The accepted logging policy makes previously usable unsafe custom logging profiles refuse Talk.
  • merge-risk: 🚨 availability: The included receive-buffer placement changes the allocation arena, while PSRAM exhaustion and receive-pressure behavior remain incompletely qualified.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof, and the diff does not materially change authorization authority. Reported earlier-image boot/reconnect observations do not prove camera or Talk stages or the final logging repair; owner-level regressions and the explicit maintainer acceptance support this bounded change.

Evidence

What I checked:

  • Policy and checkout: No root or nested AGENTS.md or maintainer-notes directory was found in the checkout or fetched main tree. The final working-tree status was clean. (dc5c8917467a)
  • Verified introduction boundary: The merge-base-to-head diff contains 27 files, including the inherited SDIO allocation commit. The raw test-merge object has exactly the pinned main parent followed by the pinned PR head, matching the host-provided merge-result evidence; base-only release and UI changes are not attributed to this PR. (0ef0e5bf988a)
  • Prior finding resolved: The current guard avoids the setter for no-tag logging, preserves quieter tag levels, checks effective verbosity, and requires static builds to exclude INFO. An initial local historical comparison could not load a missing blob; the exact commit patch was subsequently inspected through GitHub. (components/esp-openclaw-room-node/esp_openclaw_room_node.c:544, e0ca61f31331)
  • Logging dependency contract: The target guard directly depends on ESP-IDF logging semantics. At the pinned SDK revision, the no-tag setter changes the default level, while static no-tag logging does not dynamically filter emitted levels; these contracts support both parts of the repaired guard. (components/log/src/log_level/tag_log_level/tag_log_level.c:87, 362a1776ec21)
  • SDK privacy boundary: The target's .gitmodules verifies this dependency's repository, and its pinned WebRTC source emits outgoing SDP at INFO under the webrtc tag before passing the message to signaling. This directly supports suppressing that tag before negotiation. (components/esp_webrtc/src/esp_webrtc.c:278, 4135993ac537)
  • Inherited allocation dependency contract: The introduced SDIO patch directly calls esp_dma_capable_malloc. The pinned implementation supports explicit SPIRAM capabilities and combines requested DMA alignment with cache alignment; the separately inspected SDMMC transaction path performs alignment checks and cache synchronization. (components/esp_hw_support/dma/esp_dma_utils.c:162, 362a1776ec21)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (7 earlier review cycles)
  • reviewed 2026-09-09T06:19:48.617Z sha 3269794 :: blocked before merge. :: none
  • reviewed 2026-09-09T06:33:25.193Z sha 3269794 :: blocked before merge. :: none
  • reviewed 2026-09-09T11:09:47.355Z sha 3269794 :: blocked before merge. :: none
  • reviewed 2026-09-09T11:29:58.040Z sha 3269794 :: blocked before merge. :: [P2] Prevent SDP suppression from lowering the global log level
  • reviewed 2026-09-10T12:44:06.543Z sha e0ca61f :: blocked before merge. :: none
  • reviewed 2026-09-10T13:02:56.781Z sha e0ca61f :: blocked before merge. :: none
  • reviewed 2026-09-12T15:27:45.772Z sha e0ca61f :: blocked before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 9, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 10, 2026
@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 15:23
@vincentkoc
vincentkoc changed the base branch from fix/tab5-sdio-rx-psram to main September 12, 2026 15:46
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Sep 12, 2026
@vincentkoc
vincentkoc merged commit 6b63951 into main Sep 12, 2026
13 of 14 checks passed
@vincentkoc
vincentkoc deleted the diag/tab5-media-stages branch September 12, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant