Skip to content

fix(tab5): align CSI input with ISP output - #48

Merged
vincentkoc merged 5 commits into
mainfrom
fix/tab5-csi-post-isp-format
Sep 12, 2026
Merged

fix(tab5): align CSI input with ISP output#48
vincentkoc merged 5 commits into
mainfrom
fix/tab5-csi-post-isp-format

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 9, 2026

Copy link
Copy Markdown
Member

Problem

The published esp_video 2.4.1 mapper selects RAW8 as CSI input and RGB565 as
CSI output when building with IDF below 6. The pinned SDK
362a1776ec212788fda95f75b733bfdde3a0c394 has backported the post-ISP CSI contract:
on early ESP32-P4 silicon, unequal CSI formats are rejected. A Tab5 revision-1.3
capture reached VIDIOC_STREAMON and failed with errno 3, the video component's
mapping of ESP_ERR_NOT_SUPPORTED, before a frame was captured.

Related upstream report: espressif/esp-video-components#98.
Its different board and sensor are not Tab5 hardware qualification.

Changes

  • Apply one explicit, pinned Tab5 component patch: preserve ISP RAW8 to RGB565,
    then use CSI RGB565 to RGB565. Preserve 16-bit output/DMA sizing, raw bypass,
    unsupported-format checks, and SDK guards.
  • For the explicit repair, require the exact SDK commit and CSI source hash, unchanged early-P4 profile,
    registry identity, full component/source hashes, and tracked patch hash.
    Refuse other SDKs without modifying them.
  • Apply only after successful configuration in the existing isolated CI path.
    After normal Ninja, verify the actual mapper compile input, current RISC-V
    object, and final component/source hashes. Do not rewrite component-manager
    integrity markers.
  • Add camera provenance without replacing existing SDK or SDIO records. The
    separate symbols artifact retains the same firmware manifest.
  • Report only allowlisted static camera/SDK refusal codes at the helper CLI
    boundary, with a fixed fallback for unknown errors. Do not print raw
    exceptions, paths, input values, or nested causes.
  • Run patch check and application from the enclosing Git worktree root with
    the validated component-relative --directory. Retain component-local
    application for a genuine standalone directory; refuse invalid, bare,
    broken, or out-of-root repository ownership.
  • For ordinary unpatched packaging, verify the resolved camera lock hash against
    both component-manager metadata and actual component bytes, with matching
    manifest version and safe managed paths. Do not impose the repair's fixed
    component version, mapper layout, SDK commit, or early-P4 profile.

This does not change SDK/dependency versions, chip-revision selections, sensor
configuration, allocation policy, existing SDK/SDIO patches, or application
capture/cleanup behavior. The packaging follow-up separates ordinary lock
integrity from the opt-in repair; all explicit repair pins, runtime patch
bytes, and workflow remain unchanged.

Packaging Review Follow-up

Head 27fc464a9667c70231cf5861f80888e2ca3fbe29 fixes ordinary packaging rejecting
a valid resolved component solely because it is not the repair's pinned version
or mapper layout. The actual packaging regression failed on the prior head and
passes after the separation. All 40 real component-manager helper/package tests
pass, including undeclared modifications, forged metadata, invalid lock identity,
safe paths, explicit repair rejection and additive provenance. Fresh P2 and
independent delta review found no remaining actionable issues. New-head CI,
attempt 1

passed all five builds. Historical hosted and physical evidence below remains
tied to the original exact source and artifact identities, not to a newly
flashed follow-up.

Validation

Prior completed local implementation checks:

  • 11 camera tests passed, including the complete upstream mapper: original
    post-ISP contract failure, patched pass, raw bypass, sibling outputs,
    unsupported formats, strict patch guards, and native-object verification.
  • 21 firmware packaging tests passed, including additive SDK/SDIO/camera
    provenance and post-build tampering refusal.
  • One additional revision-3 packaging regression failed before the review
    correction and passed afterward: the early-P4 guard applies only to the
    explicit camera patch, not to unchanged component packaging.
  • 12 existing SDK and 13 SDIO patch tests passed.
  • Workflow actionlint and staged git diff --check passed.
  • Tests used the real cached IDF component-manager 2.5.2 hashing API; no skips.

For the diagnostic-only follow-up, two new CLI regressions failed on the old
handler and passed after the change. They exercise real camera/SDK refusals,
exit 1, empty stdout, unchanged pre-apply bytes, and secret-canary exclusion.
A repeated unchanged mapper suite was interrupted locally and is not counted
as a fresh pass. Focused review, syntax, diff, and preservation checks passed.

The initial hosted run
at head 0116ddcd482125ef74d8be15d469cd8707d78c4c passed the four other jobs.
Tab5 stopped in the camera compatibility helper with an unclassified
ValueError, before Ninja and artifact upload. The diagnostic follow-up in
attempt 1
at head b3a7ea49c7c8bd67ce80fef7f825d3c5bf2ad93d stopped with
guard=source_profile, before artifact upload.

An exact local reproduction with the archived source and tracked Git-style
patch found that git apply from the nested component directory returned
success without changing the source. Standalone application worked. Running
from the owning repository root with the explicit component-relative directory
produced the expected patched hash in all three tested nested layouts. The
earlier synthetic fixture omitted diff --git headers and missed this behavior.
This is local exact-patch evidence consistent with the CI source-hash refusal,
not a retained source snapshot from the failed CI job.

The faithful Git-header regression failed before the invocation repair. All six
targeted tests passed afterward, covering nested and standalone application,
idempotence, preservation of unrelated caller edits and SDK/component bytes,
and refusal of invalid configuration, bare repositories, broken Git metadata,
and an out-of-root component. Fresh P2 and independent focused reviews found
no remaining issues in the frozen two-file delta.

The normal fix CI run, attempt 1
passed all five jobs at head 5e66310797e87d35684d8e61b9dba324c5b9cb66.
Tab5 executed 4 PSRAM RX, 13 SDIO, 18 camera, and 22 packaging tests successfully,
then completed native Ninja, post-build camera verification, firmware upload,
and separate symbols upload.

The downloaded Tab5 firmware artifact 10116769564 is 2,898,295 bytes and
matches GitHub's SHA256
b982e31985d551e4e222132b82634568c9997b54a986d62d6f2b91e74e914410.
All 11 internal checksums and the four images at 0x2000, 0x8000, 0x10000,
and 0x810000 were verified. Its manifest records compiled test merge
079791093d6dddb2eced111da3a3685ecb995fe1; the immutable Git object confirms
the expected base and PR-head parents.

Post-Ninja verification and the manifest record the expected patched mapper
source hash 99e47cbdbb6fc8d218ea17ac8ac4fd02c7b03aad89e06c48bbc1e5df4d4c3dfb
and current RISC-V object hash
26b129a2b1470ddd6aa657bd0509bd4e2c27a6630c7ec9659f852024eaa7ab88.
Existing SDK and SDIO patch records remain present and match the tracked patches.
The actual SDK is v5.5.5-648-g362a1776ec2-dirty, not pristine release 5.5.5;
the dirty state is explicitly qualified by the existing SPM/NVS patch records.
The flash bundle does not include mapper object bytes. Symbols were uploaded
but were not downloaded for this validation.

This exact 079791093d6dddb2eced111da3a3685ecb995fe1 image was flashed and
completed a bounded keyless boot/reconnect exercise. One camera.snap succeeded
on September 9, 2026 at 19:02:40.700 UTC, producing a 640x1136 JPEG of 48,905 bytes.
This exercises the previously failing VIDIOC_STREAMON path and establishes
one captured JPEG, not complete camera or media qualification.

The image had an edge artifact. A source-confirmed PPA quantized-geometry
mismatch is tracked separately in
#49; that correction is not part
of this image. Green tint, color/exposure quality, audible output
and complete media behavior remain unqualified.

Dependency Review

Dependency comparison with the previously flashed image found one resolver
drift: espressif/dl_fft 0.6.0 -> 0.7.0, permitted by unchanged >=0.6.0
constraints in ESP-SR and ESP-DL. The verified archive diff preserves the
existing C API/handle layouts and P4 assembly, but changes shared-table
allocation, locking/reference-counted lifetime, and C++ wrapper synchronization.
Review found no concrete defect or demonstrated cross-binary C++ ABI use
requiring a hold. Acceptance is limited to this exact artifact for bounded
keyless boot and one camera trial. Ambient AFE is active, so this is neither
a pure camera-only A/B comparison nor AFE equivalence proof. The bounded
physical results above do not qualify Talk or provider calls.

Landing

The predecessor #47 is merged.
Signed head 620a13e63168e7886f48ff4740726f1b9726f278 incorporates current
main with the reviewed workflow-context resolution: camera verification and
provenance remain intact, and both the build and manifest retain the immutable
Tab5 image selected in #57.
The complete tree matches the independently reviewed projection. No runtime
patch, SDK guard, component pin, or accepted packaging repair was changed.

All five native builds and CodeQL passed for this signed recovery head in
CI run 34704115708.
Accept the narrowly pinned repair and ordinary lock-based packaging under
the authorized merge-commit landing; retire the compatibility patch only
after its upstream replacement is qualified against the selected SDK.
Historical hardware observations above are not live qualification of this
cumulative tree. End-to-end voice, full camera quality and encrypted media
remain unqualified; no release or provider call is authorized by this landing.

@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: 🦐 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 9, 2026
@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

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

ClawSweeper review

What this changes

Aligns Tab5 camera input with the image processor’s output through a pinned component patch, guarded build tooling, firmware provenance, and regression coverage.

Merge readiness

Blocked before merge - 2 items remain

This repair remains necessary and has no remaining actionable findings. Prior packaging and integration concerns are resolved; the maintainer-authored PR remains a draft.

Priority: P2
Reviewed head: 620a13e63168e7886f48ff4740726f1b9726f278

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, guarded repair with relevant physical evidence and regression coverage; no remaining actionable defect was identified.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The retained maintainer report identifies a flashed Tab5 image whose patched video mapper reached a successful camera.snap JPEG through the formerly failing stream-start path. Subsequent tooling repairs and main integration preserve the runtime patch; this supports bounded recovery, not fresh cumulative-tree or complete media qualification.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The retained maintainer report identifies a flashed Tab5 image whose patched video mapper reached a successful camera.snap JPEG through the formerly failing stream-start path. Subsequent tooling repairs and main integration preserve the runtime patch; this supports bounded recovery, not fresh cumulative-tree or complete media qualification.
Evidence reviewed 10 items Review scope and policy: The origin identifies the target repository. No AGENTS.md or maintainer-notes directory was found. The complete local introduced change contains eight files; the checkout remained clean.
Verified integration: The test merge records the pinned main parent followed by the exact PR head. Both its tree and the checkout tree are 0c4c9cb8e766134a8dc1c7e6a02ea1cc7e497279; its main-relative changes are exactly the eight introduced files.
Prior findings resolved: Ordinary packaging validates the resolved component hash and manifest version without requiring the repair’s SDK, mapper layout, component version, or silicon profile. Patch application uses the discovered worktree root and a validated component-relative directory. Regression tests cover alternate component layouts, revision-3 packaging, nested application, and invalid repository ownership.
Findings None None.
Security None None.

How this fits together

Tab5 receives camera snapshot requests through the room-node firmware. Its video component configures the image processor and camera interface before captured pixels are transformed and encoded as JPEG.

flowchart LR
  A[Snapshot request] --> B[Tab5 capture owner]
  B --> C[Video format mapper]
  D[Pinned SDK and component checks] --> C
  C --> E[Image processor converts RAW to RGB565]
  E --> F[Camera interface passes RGB565]
  F --> G[Transform and JPEG response]
Loading

Before merge

  • Resolve merge risk (P1) - The physical capture proves historical stream-start recovery; the cumulative head has not been freshly flashed, and color/exposure quality and broader media behavior remain unqualified.
  • Complete next step (P2) - Mark the draft ready when the author is ready to proceed with the stated merge-commit landing.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Introduced line delta Production/build tooling +321; tests +724; workflow +9; docs +47 Most growth supports explicit dependency guards, provenance, and regression coverage around a small vendor mapper correction.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain the narrowly pinned compatibility repair and ordinary lock-based packaging, then retire the patch when an upstream component is qualified against the selected SDK.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Retain the narrowly pinned compatibility repair and ordinary lock-based packaging, then retire the patch when an upstream component is qualified against the selected SDK.

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

Yes: the board’s RGB565 request, pinned mapper’s RAW input selection, and SDK’s early-P4 rejection establish the failure path. This read-only review did not execute firmware or tests.

Is this the best way to solve the issue?

Yes: matching CSI input to the existing ISP output repairs the demonstrated contract mismatch while exact-profile guards constrain application and ordinary packaging remains independent of repair pins.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: Repairs a demonstrated camera failure limited to the selected early-P4 Tab5 SDK/component combination.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The retained maintainer report identifies a flashed Tab5 image whose patched video mapper reached a successful camera.snap JPEG through the formerly failing stream-start path. Subsequent tooling repairs and main integration preserve the runtime patch; this supports bounded recovery, not fresh cumulative-tree or complete media qualification.
  • proof: sufficient: Contributor real behavior proof is sufficient. The retained maintainer report identifies a flashed Tab5 image whose patched video mapper reached a successful camera.snap JPEG through the formerly failing stream-start path. Subsequent tooling repairs and main integration preserve the runtime patch; this supports bounded recovery, not fresh cumulative-tree or complete media qualification.

Evidence

What I checked:

  • Review scope and policy: The origin identifies the target repository. No AGENTS.md or maintainer-notes directory was found. The complete local introduced change contains eight files; the checkout remained clean. (620a13e63168)
  • Verified integration: The test merge records the pinned main parent followed by the exact PR head. Both its tree and the checkout tree are 0c4c9cb8e766134a8dc1c7e6a02ea1cc7e497279; its main-relative changes are exactly the eight introduced files. (1658377b5377)
  • Prior findings resolved: Ordinary packaging validates the resolved component hash and manifest version without requiring the repair’s SDK, mapper layout, component version, or silicon profile. Patch application uses the discovered worktree root and a validated component-relative directory. Regression tests cover alternate component layouts, revision-3 packaging, nested application, and invalid repository ownership. (scripts/tab5_camera_compat.py:164, 620a13e63168)
  • Integration preserves reviewed repair: GitHub’s comparison with the previous reviewed head shows the qualified image selection restored in both build input and packaging provenance. The helper and runtime patch have identical blob identities before and after integration. A local historical comparison failed to load missing objects; the read-only GitHub comparison supplied the relevant evidence. (.github/workflows/ci.yml:35, 620a13e63168)
  • Concrete dependency boundary: The patch directly modifies esp_video’s format mapper, while the helper verifies the exact ESP-IDF CSI source. The board requests RGB565 and invokes VIDIOC_STREAMON, establishing that these Espressif contracts govern the changed behavior. (patches/esp-video/tab5-csi-post-isp-format.patch:4, 620a13e63168)
  • Upstream mapper mismatch: The pinned mapper’s old-chip processing branch preserves ISP conversion to the requested format but selects the sensor format as CSI input under IDF below 6. The PR changes that input to the requested post-ISP format while retaining output and rejection behavior. (esp_video/src/device/esp_video_csi_format.c:506, 67a555a517b7)

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 (11 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-09T17:31:08.478Z sha 5e66310 :: blocked before merge. :: [P1] Keep repair-specific pins out of unpatched packaging
  • reviewed 2026-09-09T17:53:16.855Z sha 5e66310 :: blocked before merge. :: [P1] Keep repair-specific pins out of unpatched packaging
  • reviewed 2026-09-09T18:21:26.075Z sha 5e66310 :: blocked before merge. :: [P1] Keep repair-specific pins out of unpatched packaging
  • reviewed 2026-09-09T19:33:29.065Z sha 5e66310 :: blocked before merge. :: [P1] Keep repair-specific pins out of unpatched packaging
  • reviewed 2026-09-10T11:57:52.957Z sha 27fc464 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-10T12:09:09.435Z sha 27fc464 :: needs changes before merge. :: none
  • reviewed 2026-09-10T13:02:16.574Z sha 27fc464 :: needs changes before merge. :: none
  • reviewed 2026-09-12T16:01:51.486Z sha 27fc464 :: blocked before merge. :: none

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. label Sep 9, 2026
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. label Sep 9, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label 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 merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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 changed the base branch from diag/tab5-media-stages to main September 12, 2026 15:57
@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 16:16
@vincentkoc
vincentkoc merged commit cf600d8 into main Sep 12, 2026
13 of 14 checks passed
@vincentkoc
vincentkoc deleted the fix/tab5-csi-post-isp-format branch September 12, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. 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