Skip to content

fix(tab5): align camera RGB order with JPEG encoder - #56

Merged
vincentkoc merged 2 commits into
mainfrom
fix/tab5-camera-rgb-order
Sep 13, 2026
Merged

fix(tab5): align camera RGB order with JPEG encoder#56
vincentkoc merged 2 commits into
mainfrom
fix/tab5-camera-rgb-order

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 11, 2026

Copy link
Copy Markdown
Member

Problem

The Tab5 camera passes PPA RGB888 output directly to esp_new_jpeg. The pinned
PPA pixel layout is B,G,R in memory, while the encoder's RGB888 input contract
is R,G,B. Without a channel swap, red and blue reach the encoder reversed.

Changes

  • Enable PPA's existing input RGB channel-swap option at the camera transform.
  • Keep ISP, white balance, rotation, quantized geometry, allocation, ownership,
    quality policy, SDK and dependencies unchanged in this PR's unique delta.
  • Extend the existing actual-source fixture through camera_snap,
    transform_camera_frame, and the JPEG process boundary with independent
    red, green, blue and gray vectors.
  • Document the byte-order boundary without claiming a fix for observed color
    cast or physical color accuracy.

The pinned SDK contracts are in
color_types.h
and
ppa.h.
The reviewed encoder contract is espressif/esp_new_jpeg 1.0.2.

Validation

  • Original production source compiled against the new fixture, then failed
    specifically on reversed red and blue at JPEG ingress; green and gray passed.
  • All nine focused camera test methods passed again after current-main
    integration, with ASan/UBSan and warnings-as-errors. Coverage includes the
    existing geometry, warm-up, lifetime and failure-cleanup cases.
  • The original semantic delta passed P2 autoreview and independent source
    review. The ancestry-only integration preserves that exact five-file delta.
  • Diff whitespace and privacy checks passed.
  • All five native jobs passed on the integrated head:
    https://github.com/openclaw/esp-openclaw-node/actions/runs/34754017538.
  • All three CodeQL analyses passed:
    https://github.com/openclaw/esp-openclaw-node/actions/runs/34754016230.
    No extra workflow was dispatched.
  • The Tab5 firmware archive was downloaded once and verified against its
    GitHub SHA256 and byte count. All 11 checksum records, ten manifest file
    records and four offset-mapped images passed verification. The image ranges
    do not overlap NVS, PHY initialization or storage partitions.

Current-Main Integration

Originally stacked on #55,
this PR now targets main. Signed merge commit
e517b141cdd80ec3f979aefcfe4ca02a129d2f25 preserves the original signed head
4f87df7d5b381b4d9f3b1a79364caff2c86dca11 and incorporates main
f3f7719a9d2d75763c1645dcd758b61189c8e872.

The candidate tree a369f7a33734ca2d59e60205753b7d597fedcb13 exactly matches
current main plus the unchanged reviewed patch. GitHub's synthetic merge
8f63e61a4f9fb0adf7f6dfac83dd44d5b9df79c6 has the same tree and the expected
main/head parents.

The archive identifies compiled source
8f63e61a4f9fb0adf7f6dfac83dd44d5b9df79c6, app version 8f63e61, and the
qualified ESP-IDF image with the existing tracked patches. Firmware artifact
10316388299 is 2,899,804 bytes, SHA256
815a8f1ed89d6381a69a4c548c8577d793cd0e3d26fd6d01f1832cacf3999848.
Symbols are retained in the same run but were not downloaded.

Qualification

On September 13, 2026, candidate
8f63e61a4f9fb0adf7f6dfac83dd44d5b9df79c6 was flashed to an M5Stack Tab5:
all four programmed images were verified, and a clean ESP32-P4 v1.3 boot
reached app_main. Both node and operator roles reconnected using saved
sessions. device.info confirmed the Tab5 and app version 8f63e61; one
camera.snap RPC succeeded and returned a JPEG that decoded successfully.
The bounded run ended with its owned resources closed.

The inspected image was dark and nearly uniform, without an identifiable
scene. This proves a working capture/encode/RPC path, not optical RGB
calibration or usable scene quality. Physical color accuracy, exposure,
white balance and orientation remain unqualified. The cause of the dark
frame and previously observed color cast remains unknown. Voice qualification
is still pending; no successful voice exchange is claimed.

The owner has explicitly approved landing this narrow, source-contract-backed
channel-order correction with those optical limitations, releasing the draft
hold. The production delta remains one option and its explanatory comment;
the PPA/JPEG boundaries in the color fixtures are modeled, not physical color
measurements. No orientation, exposure or white-balance repair is included.
Landing is by merge commit to preserve the original signed series. No release,
OTA, secure-provisioning or other-device work is included.

@clawsweeper

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

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 13, 2026, 7:32 AM ET / 11:32 UTC (Revision 4).

ClawSweeper review

What this changes

The Tab5 camera swaps red and blue before JPEG encoding, adds four color-vector regression checks, and documents the correction’s limits.

Merge readiness

Blocked before merge - 3 items remain

The correction remains absent from current main, and no actionable patch defect was found. Keep this maintainer-authored PR open while its explicit physical-qualification hold remains in force.

Priority: P2
Reviewed head: e517b141cdd80ec3f979aefcfe4ca02a129d2f25
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, contract-backed correction with useful regression coverage and no blocking code findings; the owner’s landing hold is separate from patch quality.
Proof confidence 🌊 off-meta tidepool Not applicable: The ordinary contributor-proof gate is exempt for this MEMBER-authored PR, and no authority boundary changes. The actual snapshot/transform fixture models accelerator and encoder behavior; native CI and artifact checks do not prove physical JPEG colors. The author’s separate qualification hold remains active.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The ordinary contributor-proof gate is exempt for this MEMBER-authored PR, and no authority boundary changes. The actual snapshot/transform fixture models accelerator and encoder behavior; native CI and artifact checks do not prove physical JPEG colors. The author’s separate qualification hold remains active.
Evidence reviewed 8 items Introduced production change: The verified main-to-head delta changes only the accelerator’s rgb_swap option and its comment in production; geometry, encoding policy, privacy checks, and dependencies remain unchanged.
Still necessary on main: Current main configures RGB565-to-RGB888 conversion without rgb_swap and passes that buffer to the RGB888 encoder. The GitHub releases endpoint returned no releases; no shipped fix was established.
Pinned accelerator contract: The PR explicitly relies on this SDK revision. Its accelerator configuration documents rgb_swap as input RGB channel swapping; color_types.h defines RGB888 components in B,G,R memory order.
Findings None None.
Security None None.

How this fits together

The Tab5 camera handles snapshot requests from OpenClaw by capturing sensor frames, transforming them with Espressif’s pixel-processing accelerator, and encoding JPEG images. This change aligns the accelerator’s output bytes with the encoder’s input format.

flowchart LR
  A[Snapshot request] --> B[Capture indicator and camera lease]
  B --> C[RGB565 sensor frame]
  C --> D[Scale and rotate with channel swap]
  D --> E[RGB888 JPEG encoder]
  E --> F[Snapshot response]
Loading

Decision needed

Question Recommendation
Should source landing remain blocked on physical Tab5 color qualification, or may this byte-order correction land separately? Retain the qualification hold: Keep the PR draft until the owner records acceptable physical channel-order evidence and releases the hold.

Why: The author explicitly retains a physical-qualification hold; source review cannot release that owner-imposed landing condition.

Before merge

  • Resolve merge risk (P1) - Physical accelerator output and decoded JPEG channel order remain unqualified; the supplied regression fixture models both boundaries.
  • Complete next step (P2) - Resolve the explicit physical-qualification hold with the camera owner and record the accepted landing criteria before making this PR ready.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +2/-0; tests +125/-14; docs +5/-0 The production growth is justified by one existing SDK option and its explanatory comment.
Color regression vectors 4 vectors added Independent red, green, blue, and gray expectations check encoder ingress through the actual transform owner.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain the narrow channel-order correction and resolve the owner’s qualification criteria without expanding its claims to exposure, white balance, or full camera qualification.
  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 narrow channel-order correction and resolve the owner’s qualification criteria without expanding its claims to exposure, white balance, or full camera qualification.

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

Yes, at the source-contract boundary: main passes B,G,R accelerator output to an encoder requiring R,G,B. The contributor reports a failing-before/passing-after fixture; physical capture was not reproduced.

Is this the best way to solve the issue?

Yes. Enabling the accelerator’s existing channel-swap option repairs the byte-order mismatch without adding a conversion pass or changing camera policy.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: This is a bounded Tab5 snapshot color-order correction without evidence of an urgent wider regression.
  • 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 ordinary contributor-proof gate is exempt for this MEMBER-authored PR, and no authority boundary changes. The actual snapshot/transform fixture models accelerator and encoder behavior; native CI and artifact checks do not prove physical JPEG colors. The author’s separate qualification hold remains active.

Evidence

What I checked:

  • Introduced production change: The verified main-to-head delta changes only the accelerator’s rgb_swap option and its comment in production; geometry, encoding policy, privacy checks, and dependencies remain unchanged. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:1026, e517b141cdd8)
  • Still necessary on main: Current main configures RGB565-to-RGB888 conversion without rgb_swap and passes that buffer to the RGB888 encoder. The GitHub releases endpoint returned no releases; no shipped fix was established. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:1024, f3f7719a9d2d)
  • Pinned accelerator contract: The PR explicitly relies on this SDK revision. Its accelerator configuration documents rgb_swap as input RGB channel swapping; color_types.h defines RGB888 components in B,G,R memory order. (components/esp_driver_ppa/include/driver/ppa.h:180, 362a1776ec21)
  • Encoder byte-order contract: The component manifest at this revision identifies esp_new_jpeg version 1.0.2 and this repository as its source. Its RGB888 enum explicitly requires red, green, blue components in increasing memory addresses. (esp_new_jpeg/include/esp_jpeg_common.h:48, 95289ee8beba)
  • Regression coverage and limits: The fixture now includes the actual transform and snapshot owners, checks red/green/blue/gray bytes at encoder ingress, and retains cleanup assertions. Accelerator conversion and JPEG encoding remain stubs; these checks do not establish physical image quality. Tests were inspected, not executed during this read-only review. (scripts/tests/fixtures/test_tab5_camera_capture.c:376, e517b141cdd8)
  • Explicit landing hold and review continuity: The complete supplied PR body says no hardware was flashed or captured and requests retaining draft status until physical qualification and final landing gates are resolved. The previous completed review covered the same head and had no findings. Its historical qualification concern remains supported by the current body, independently of that prior review. (e517b141cdd8)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Record the physical channel-order qualification result, or explicitly approve source-only landing and release the hold.

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-11T07:13:53.803Z sha 4f87df7 :: blocked before merge. :: none
  • reviewed 2026-09-12T17:11:05.068Z sha 4f87df7 :: blocked before merge. :: none
  • reviewed 2026-09-13T11:20:53.320Z sha e517b14 :: blocked before merge. :: none

Base automatically changed from fix/room-diagnostics-header-stdio to main September 12, 2026 17:07
* commit 'f3f7719a9d2d75763c1645dcd758b61189c8e872':
  fix(ci): pin Tab5 to qualified ESP-IDF image
  fix(tab5): retain DMA buffers when flash encryption is active
  feat(tab5): add a static room-node home (#39)
  fix(talk): enforce safe SDK logging without global changes
  fix(ci): verify unpatched camera components against their lock
  fix(ui): retain setup guidance on the node home
  ci: publish traceable example firmware bundles (#37)
  ci: require explicit verified component releases (#38)
  docs: clarify source setup and hardware qualification
@vincentkoc
vincentkoc marked this pull request as ready for review September 13, 2026 13:30
@vincentkoc
vincentkoc merged commit 7b8b32d into main Sep 13, 2026
13 of 14 checks passed
@vincentkoc
vincentkoc deleted the fix/tab5-camera-rgb-order branch September 13, 2026 13:31
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. 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