Skip to content

fix(tab5): bound camera dequeue waits - #53

Merged
vincentkoc merged 9 commits into
mainfrom
fix/tab5-camera-dequeue-timeout
Sep 12, 2026
Merged

fix(tab5): bound camera dequeue waits#53
vincentkoc merged 9 commits into
mainfrom
fix/tab5-camera-dequeue-timeout

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 10, 2026

Copy link
Copy Markdown
Member

Problem

Tab5 capture leaves ESP Video's ready-buffer wait at its default
portMAX_DELAY. The existing delayMs deadline is checked only after a
successful dequeue, so it does not bound a missing frame.

A recent camera attempt produced dqbuf_begin without later completion
evidence. That observation does not establish where it stalled; this change
addresses the independently verified unbounded ready-buffer wait and adds
stage timing without claiming an incident root cause.

Stacked on #52.

Changes

  • Set a two-second per-dequeue timeout on every open through the existing
    VIDIOC_S_DQBUF_TIMEOUT ioctl. Fail closed through the existing capture
    cleanup path if configuration fails.
  • Preserve warm-up/requeue semantics, capture formats, geometry, allocation
    policy, privacy indicator, and camera/Talk ownership. A missing frame fails
    without retry, including during warm-up.
  • Add at most ten fixed-field INFO records for first dequeue return, selected
    capture, transform, encode, cleanup, and release. No per-frame trace or
    request identity; a region returning is not proof of success or sensor
    power-off.
  • Extend the existing actual-source capture/handler fixture and document the
    finite wait and remaining blocking boundaries.

Validation

  • Original source compiles with the final fixture, then fails both intended
    regressions: the missing-frame wait remains infinite, and timeout setup is
    unchecked.
  • Eight focused test methods pass with ASan/UBSan and
    -Wall -Wextra -Werror. Coverage includes setup failure, missing frames,
    partial cleanup, repeated open, zero/ten-second virtual warm-up, starvation,
    bounded stage records, transform/encode failures, and actual-handler release.
    Existing transform geometry tests also pass.
  • Actionlint and git diff --check pass. No workflow change.
  • Fresh scoped P2 autoreview and independent source-delta review found no
    actionable P0-P2 findings on the frozen four-file change.
  • All five native CI jobs passed on head
    33748b8c5b43db4f1c494677e75313f07c217969:
    https://github.com/openclaw/esp-openclaw-node/actions/runs/34519194614.
  • The verified Tab5 firmware was compiled from synthetic merge
    14f511933af6fceaa0923b4a3457283eb4de36f5 (app version 14f5119).
    Its parents and tree match the reviewed base/head. Artifact 10169253664
    matches GitHub's SHA256
    16f4d1d1f8e9bd7f8a323fe9c7e2c334470288888c087ad852e4343b94c9dc97;
    all 11 internal checksums, 10 manifest file entries, and four offset-mapped
    images passed verification. The images do not overlap NVS, PHY, or storage.
  • SDK 362a1776ec212788fda95f75b733bfdde3a0c394 with the existing tracked
    patches, component patch provenance, generated configuration, resolved
    dependency lock, and partition table match the previously verified PR52
    bundle. The post-Ninja camera-source verifier and strong-symbol link checks
    passed in CI. These checks do not establish reproducible builds or new
    hardware qualification.

Bounded Physical Observations

  • The exact 14f5119 image was flashed with four writes and four independent
    readbacks passing. Runtime identity and device.info confirmed this image.
  • In the first keyless camera trial, all ten pipeline records were observed.
    First dequeue returned in 41 ms; selected capture was reported at 508 ms,
    transform at 119 ms, encode at 482 ms, cleanup at 2 ms, and release at 15 ms.
    These records show the instrumented regions returned, not that a valid
    JPEG reached the caller or that sensor power-off was verified.
  • The camera RPC failed with DISCONNECTED after 10,816 ms. Same-node
    connection reconciliation was recorded immediately before the failure;
    host cleanup sent SIGTERM afterward. This ordering does not establish the
    disconnect's cause. No JPEG was delivered, and Canvas was not dispatched.
    No assertion, panic, or additional reset was observed beyond the single
    controlled boot.
  • A second metadata-diagnostic attempt stopped at the test harness's
    initial-drain-not-empty guard, before Gateway startup or any RPC. It did
    not reach the camera. This was a harness preflight stop, not an additional
    firmware fault or camera qualification result.

Limits

Two seconds is a policy allowance, not a measured maximum first-frame
latency. Setup, post-wait preprocessing, PPA, JPEG, logging, and cleanup can
still block; this is not a whole-command or Gateway timeout guarantee.

No dependency, SDK patch, format, allocator, retry, reset, or Gateway change.
Host fixtures do not prove actual sensor timing, PPA/JPEG output, teardown,
or media quality. The bounded physical observations above do not establish
successful camera delivery, image quality, or a resolved disconnect cause.
Voice remains inconclusive; no successful voice qualification is claimed.
Further physical qualification remains separate from source landing.

Maintainer Disposition

Accept the two-second per-dequeue policy and its existing failure cleanup:
a camera that needs longer for a frame will now return the existing unavailable
result, including during warm-up. Keeping an indefinitely blocked dequeue is
not the selected default. This accepts that compatibility tradeoff; it does
not claim fresh/upgraded-device latency coverage or physical missing-frame
recovery. The actual-owner tests and exact-head native CI support the bounded
implementation, while the limitations above remain open.

Land after #52 with a merge
commit and verified cumulative tree. Preserve accepted predecessor repairs.
No release, whole-request timing guarantee or successful voice qualification
is implied.

@clawsweeper

clawsweeper Bot commented Sep 10, 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 10, 2026
@clawsweeper

clawsweeper Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 12, 2026, 12:13 PM ET / 16:13 UTC (Revision 4).

ClawSweeper review

What this changes

The branch bounds Tab5 camera frame waits and adds pipeline timing coverage, alongside stacked camera compatibility, image geometry, display-memory, allocation-diagnostic, and firmware-packaging changes.

Merge readiness

Blocked before merge - 3 items remain

The bounded wait remains absent from main, and no blocking code defect was found. The maintainer’s explicit acceptance resolves the previous cutoff-policy concern; this member-authored contribution remains useful.

Priority: P2
Reviewed head: 33748b8c5b43db4f1c494677e75313f07c217969

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused timeout repair with useful regression coverage and explicit qualification limits; stack integration remains a workflow blocker.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof. Its reported historical Tab5 run exercised capture through release and all ten timing records, but did not demonstrate missing-frame recovery or JPEG delivery; the maintainer explicitly accepts those limits for source landing.
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. Its reported historical Tab5 run exercised capture through release and all ten timing records, but did not demonstrate missing-frame recovery or JPEG delivery; the maintainer explicitly accepts those limits for source landing.
Evidence reviewed 9 items Policy and repository boundary: The origin identifies the target repository. Neither the checkout nor the fetched-main tree contains AGENTS.md or maintainer notes.
Current-main necessity: Main opens the camera without configuring a dequeue timeout and checks delayMs only after VIDIOC_DQBUF returns. The releases endpoint returned no releases; no shipped fixing release was established.
Introduced timeout and cleanup: The timeout is configured after every successful open. Configuration and dequeue failures preserve errno before logging, use existing cleanup, and return the existing unavailable result. The exact head commit changes four files; the merge-base delta includes 22 files from the predecessor stack.
Findings None None.
Security None None.

How this fits together

The Tab5 board adapter handles camera requests from the OpenClaw Gateway, acquires the camera and privacy indicator, captures a frame through ESP Video, and produces a JPEG response. The changed wait sits between starting the camera stream and transforming the selected frame.

flowchart TD
  A[Gateway camera request] --> B[Acquire camera and indicator]
  B --> C[Configure two-second frame wait]
  C --> D{Frame ready}
  D -->|Yes| E[Transform and encode JPEG]
  D -->|Timeout or error| F[Cleanup and release camera]
  E --> F
  F --> G[JPEG response or existing error]
Loading

Before merge

  • Resolve merge risk (P2) - A camera needing more than two seconds for a frame now returns UNAVAILABLE, including during warm-up; the maintainer accepts this change, but fresh/upgraded-device latency and physical timeout recovery remain unqualified.
  • Resolve merge risk (P1) - The current stack conflicts with main, and historical firmware observations do not validate the eventual conflict-resolved cumulative tree.
  • Complete next step (P2) - Land fix(core): serialize runtime NVS diagnostics through stdio #52 first, then resolve any remaining conflicts and verify the cumulative tree and native CI before the requested merge-commit landing.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Cumulative LOC delta Production/tooling +508 net; tests +1716 net; documentation +135 net The growth includes the predecessor stack’s documented hardware compatibility and diagnostic work, beyond the final timeout commit.
Review scope 22 cumulative files; 4 files in the final commit Landing must preserve the predecessor repairs as well as the focused camera wait change.

Merge-risk options

Maintainer options:

  1. Honor the accepted timeout policy (recommended)
    Land the two-second cutoff with its documented qualification limits and existing unavailable response, as explicitly accepted in the maintainer disposition.

Technical review

Best possible solution:

Preserve the accepted finite frame wait and existing cleanup semantics in a verified cumulative landing tree, keeping physical qualification and whole-request timing claims separate.

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

Yes, at source level: main leaves ESP Video’s ready-buffer timeout infinite, so a missing frame prevents the later delayMs check from running. No physical failing reproduction was executed in this review.

Is this the best way to solve the issue?

Yes. The existing dependency ioctl is the narrow solution, and failure follows the existing cleanup path; the maintainer has explicitly accepted the cutoff rather than requiring additional configuration.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: This is a bounded Tab5 camera reliability improvement with a limited device-specific blast radius.
  • merge-risk: 🚨 compatibility: The accepted cutoff changes captures taking over two seconds from waiting indefinitely to returning UNAVAILABLE.
  • 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. Its reported historical Tab5 run exercised capture through release and all ten timing records, but did not demonstrate missing-frame recovery or JPEG delivery; the maintainer explicitly accepts those limits for source landing.

Evidence

What I checked:

  • Policy and repository boundary: The origin identifies the target repository. Neither the checkout nor the fetched-main tree contains AGENTS.md or maintainer notes. (6b63951f0ac5)
  • Current-main necessity: Main opens the camera without configuring a dequeue timeout and checks delayMs only after VIDIOC_DQBUF returns. The releases endpoint returned no releases; no shipped fixing release was established. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:862, 6b63951f0ac5)
  • Introduced timeout and cleanup: The timeout is configured after every successful open. Configuration and dequeue failures preserve errno before logging, use existing cleanup, and return the existing unavailable result. The exact head commit changes four files; the merge-base delta includes 22 files from the predecessor stack. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:759, 33748b8c5b43)
  • Authoritative dependency contract: The target directly includes esp_video_ioctl.h and invokes VIDIOC_S_DQBUF_TIMEOUT. At the pinned dependency revision, open initializes dqbuf_timeout_ticks to portMAX_DELAY; the setter converts timeval to ticks, and dequeue passes those ticks to the ready semaphore, which returns ESP_ERR_TIMEOUT on expiry. (esp_video/src/esp_video.c:2290, 67a555a517b7)
  • Stacked CSI compatibility contract: The target’s camera patch and verifier explicitly depend on this SDK revision. Its CSI driver rejects differing source and destination formats on pre-revision-3 ESP32-P4 hardware, consistent with preserving ISP conversion followed by CSI passthrough. (components/esp_driver_cam/csi/src/esp_cam_ctlr_csi.c:687, 362a1776ec21)
  • Regression coverage inspected: The fixture extracts the production capture and request handlers. Assertions cover timeout setup refusal, missing frames, warm-up starvation, cleanup and camera release, preserved error codes, and bounded timing records. These are SDK-boundary stubs, not physical timeout recovery. Tests were inspected but not executed during this read-only review. (scripts/tests/fixtures/test_tab5_camera_capture.c:372, 33748b8c5b43)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: 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 (3 earlier review cycles)
  • reviewed 2026-09-10T19:16:51.474Z sha 33748b8 :: blocked before merge. :: none
  • reviewed 2026-09-10T19:57:23.086Z sha 33748b8 :: blocked before merge. :: none
  • reviewed 2026-09-10T21:19:28.449Z sha 33748b8 :: blocked before merge. :: none

@vincentkoc
vincentkoc changed the base branch from fix/runtime-nvs-diagnostic-stdio to main September 12, 2026 16:09
@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 16:09
@vincentkoc
vincentkoc merged commit a18d447 into main Sep 12, 2026
14 of 16 checks passed
@vincentkoc
vincentkoc deleted the fix/tab5-camera-dequeue-timeout branch September 12, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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