fix(tab5): align CSI input with ISP output - #48
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 12, 2026, 12:10 PM ET / 16:10 UTC (Revision 12). ClawSweeper reviewWhat this changesAligns 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 Review scores
Verification
How this fits togetherTab5 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]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest 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. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (11 earlier review cycles; latest 8 shown)
|
Problem
The published
esp_video2.4.1 mapper selects RAW8 as CSI input and RGB565 asCSI output when building with IDF below 6. The pinned SDK
362a1776ec212788fda95f75b733bfdde3a0c394has backported the post-ISP CSI contract:on early ESP32-P4 silicon, unequal CSI formats are rejected. A Tab5 revision-1.3
capture reached
VIDIOC_STREAMONand failed with errno 3, the video component'smapping 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
then use CSI RGB565 to RGB565. Preserve 16-bit output/DMA sizing, raw bypass,
unsupported-format checks, and SDK guards.
registry identity, full component/source hashes, and tracked patch hash.
Refuse other SDKs without modifying them.
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.
separate symbols artifact retains the same firmware manifest.
boundary, with a fixed fallback for unknown errors. Do not print raw
exceptions, paths, input values, or nested causes.
the validated component-relative
--directory. Retain component-localapplication for a genuine standalone directory; refuse invalid, bare,
broken, or out-of-root repository ownership.
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
27fc464a9667c70231cf5861f80888e2ca3fbe29fixes ordinary packaging rejectinga 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:
post-ISP contract failure, patched pass, raw bypass, sibling outputs,
unsupported formats, strict patch guards, and native-object verification.
provenance and post-build tampering refusal.
correction and passed afterward: the early-P4 guard applies only to the
explicit camera patch, not to unchanged component packaging.
actionlintand stagedgit diff --checkpassed.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
0116ddcd482125ef74d8be15d469cd8707d78c4cpassed the four other jobs.Tab5 stopped in the camera compatibility helper with an unclassified
ValueError, before Ninja and artifact upload. The diagnostic follow-up inattempt 1
at head
b3a7ea49c7c8bd67ce80fef7f825d3c5bf2ad93dstopped withguard=source_profile, before artifact upload.An exact local reproduction with the archived source and tracked Git-style
patch found that
git applyfrom the nested component directory returnedsuccess 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 --githeaders 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
10116769564is 2,898,295 bytes andmatches GitHub's SHA256
b982e31985d551e4e222132b82634568c9997b54a986d62d6f2b91e74e914410.All 11 internal checksums and the four images at
0x2000,0x8000,0x10000,and
0x810000were verified. Its manifest records compiled test merge079791093d6dddb2eced111da3a3685ecb995fe1; the immutable Git object confirmsthe expected base and PR-head parents.
Post-Ninja verification and the manifest record the expected patched mapper
source hash
99e47cbdbb6fc8d218ea17ac8ac4fd02c7b03aad89e06c48bbc1e5df4d4c3dfband 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
079791093d6dddb2eced111da3a3685ecb995fe1image was flashed andcompleted a bounded keyless boot/reconnect exercise. One
camera.snapsucceededon September 9, 2026 at 19:02:40.700 UTC, producing a 640x1136 JPEG of 48,905 bytes.
This exercises the previously failing
VIDIOC_STREAMONpath and establishesone 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.0constraints 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
620a13e63168e7886f48ff4740726f1b9726f278incorporates currentmain 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.