Skip to content

AGENTS.md

Latest commit

 

History

History
153 lines (111 loc) · 8.66 KB

AGENTS.md

File metadata and controls

153 lines (111 loc) · 8.66 KB
Raw

Agent Review Notes

Authorship: Kimi k2.7 code thinking
Date: 2026-06-23

This file records my review of the PS3Eye driver after the 10-bit RAW10 work was completed. I read src/ps3eye.h, src/ps3eye.cpp, and README.md before forming this opinion.

What I agree is solid

  • The 10-bit bridge frame size is correct: 384000 bytes for 640×480 packed RAW10 (640*480*10/8), and the 0x1d FIFO writes (0x01 0x77 0x00) match it.
  • FrameQueue::UnpackRAW10 implements the standard SGRBG10P “4 pixels in 5 bytes” layout correctly.
  • Adding isFrameNew() is clean and necessary for long-exposure / non-blocking use.
  • Moving setExposure() to uint16_t across 0x08/0x10 is a real upgrade if it maps to the sensor's full AEC range.
  • The orientation fix (0x0C = 0xC0) and the 300 ms post-reset settle are datasheet-respecting improvements.
  • Disabling the DSP bypass path (0x64 = 0x00, clearing DCW bits in 0x65) is the right idea for true RAW.

What I do not agree with and would change

1. 8-bit RAW mode is likely broken now

init() now writes 0x64 = 0x00 for both 8-bit and 10-bit. But the init table sets 0x64 = 0x80, and the readback comment in start() expects DSP1 = 0x80 for 8-bit. The 8-bit path needs to be retested and reconciled with the 10-bit path.

2. COM7 inconsistency (0x03 vs 0x13)

  • 10-bit path: 0x12 = 0x03
  • 8-bit path: 0x12 = 0x13 with the justification "Per Claude: COM7 needs SENSOR_RAW bit 0x10"

"Per Claude" is not a datasheet citation. If bit 0x10 matters, both modes likely need it (or neither does). The inconsistency looks like cargo-culting. Pick one value for both RAW modes and justify it from the OV772x datasheet.

3. 0x67 upper bits are not preserved in 10-bit

The comment says to preserve upper bits of 0x67, but the 10-bit branch overwrites the whole register:

dsp_ctrl4 = 0x03;  // overwrites everything

The 8-bit branch does the correct read-modify-write:

dsp_ctrl4 = (dsp_ctrl4 & 0xFC) | 0x02;

The 10-bit branch should do (dsp_ctrl4 & 0xFC) | 0x03.

4. QVGA + use10Bit = true is silently broken

init() accepts 320×240 with 10-bit, but start() always uses the 8-bit bridge_start_qvga table. The expected frame size (96 000 bytes) will mismatch the bridge-configured size (76 800 bytes). Reject this combination in init() or implement QVGA RAW10 bridge config.

5. BGR/RGB/Gray + use10Bit = true is silently broken

DebayerRGB and DebayerGray take const uint8_t*. If the user selects BGR/RGB/Gray with 10-bit on, packed RAW10 bytes are fed into an 8-bit debayer, producing garbage. Reject these combos or add 10-bit debayer paths.

6. start() hard-disables AGC/AEC/AWB, ignoring setAutogain(true)

setAutogain(true) writes 0x13 = 0xf7, but start() immediately writes 0xf0. Auto-gain can never be on while streaming. If RAW10 must be manual, document that and make setAutogain warn or no-op in RAW mode.

7. 10-bit mode ignores the requested frame rate

init() calls ov534_set_frame_rate(desiredFrameRate, true) only to store a value, then overwrites 0x11 with 0x00 for 10-bit. start() skips the frame-rate set entirely for 10-bit. Either apply a 10-bit frame-rate recipe or remove desiredFrameRate from the 10-bit contract.

8. Misleading error messages in pkt_scan

On UVC header errors (data[0] != 12, UVC_STREAM_ERR, missing PTS) the code prints [PS3Eye] Frame size mismatch: .... Those are not frame-size mismatches. Use distinct error messages.

9. Dead / duplicated bridge config

bridge_start_vga_10bit exists but start() manually writes the same registers. Use the table or delete it.

10. Console noise

init() and start() spam std::cout. Convert to the existing debug() macro or add a log-level control.

11. README caveat on OV534

Claiming the OV534 truncation "was never actually tested" is a fine hypothesis, but be careful not to dismiss prior reports from mattipee / openastroproject as merely the same software bug. The roadmap retest item is the correct way to settle it.

Suggested priority order

  1. Re-test 8-bit Bayer/RGB/BGR and reconcile the 0x64 / FIFO setup.
  2. Resolve COM7 (0x03 vs 0x13) with a datasheet-based justification.
  3. Fix 10-bit 0x67 read-modify-write.
  4. Guard invalid format combos (QVGA+10bit, BGR/RGB/Gray+10bit).
  5. Respect setAutogain in start() or explicitly make RAW10 manual-only.
  6. Apply 10-bit frame rates or remove the API pretense.
  7. Replace std::cout debug spew with debug() / log levels.

The 10-bit capture itself is real and the unpack is correct. The remaining issues are mostly collateral damage to the 8-bit path, inconsistent register justification, and API combinations that silently fail.

— Kimi k2.7 code thinking

Second review — Claude Opus 4.8

Authorship: Claude Opus 4.8 Date: 2026-06-23

I re-read src/ps3eye.cpp and verified Kimi's claims against the actual code (line numbers below are from the current tree). No code changed — this is a paper review pending a camera re-plug + hardware retest. Short verdict: Kimi is mostly right.

Verified against code — I agree

  • #1 (8-bit 0x64) — agree, highest impact. init() writes 0x64 = 0x00 unconditionally (line ~1252) for both modes, but the init table sets 0x64 = 0x80 (line ~192) and start()'s own readback comment expects DSP1 = 0x80 for 8-bit (line ~1434). The code contradicts its own stated expectation. Retest 8-bit.
  • #5 (BGR/RGB/Gray + 10-bit) — agree. DebayerRGB/DebayerGray take const uint8_t* (lines ~712 / ~612); packed RAW10 bytes → garbage. Guard in init().
  • #4 (QVGA + 10-bit) — agree. start() always uses bridge_start_qvga (line ~1340) while frame_size is computed for 10-bit → 96000 vs 76800 mismatch → discarded frames.
  • #6 (0x13 = 0xf0 kills autogain) — agree, and it's unconditional (line ~1419), so it regresses the 8-bit autogain case too, not just RAW10.
  • #7 / #8 / #9 / #10 — all verified. 10-bit ignores desiredFrameRate (overwrites 0x11=0x00, skips the rate set); "Frame size mismatch" is printed for UVC-header errors that aren't size mismatches (lines ~988–1000); bridge_start_vga_10bit (line ~221) is dead — start() re-writes the same regs inline (lines ~1348–1356); init/start/pkt_scan spew std::cout though a debug() macro exists (header ~35).

Where I'd nuance Kimi

  • #2 (COM7 0x03 vs 0x13): the real defect is the "Per Claude" non-citation, not necessarily that the two modes differ — they target different bridge formats and could legitimately need different COM7. Fix = justify each from the OV772x datasheet, not "pick one value for both."
  • #3 (0x67 read-modify-write): correct in principle (the 10-bit branch dsp_ctrl4 = 0x03 overwrites, contradicting its own preserve-upper-bits comment), but 10-bit empirically works, so the upper bits are evidently zero here. Low-risk defensive cleanup, not a failure cause — rank it below the silent-combo guards.
  • #11 (OV534 README caveat): fair hit on my text. The README's "almost certainly the same software bug" pre-judges the very retest on the roadmap. Soften to a hypothesis. (Note: I'm not aware of a prior mattipee/openastro report that the OV534 specifically truncates — the 307200 belief traces to our own pre-fix observation. The retest settles it either way.)

The connection Kimi didn't draw — relevant to the new example/

The example/ app I just added uses the 8-bit BGR path — exactly the path #1, #6, and #10 degrade. So on first run, expect:

  • heavy console spam every init/frame (#10),
  • autogain disabled → dark image unless gain/exposure are set manually (#6),
  • and if 0x64=0x00 truly breaks the 8-bit FIFO (#1), possibly garbage or no image.

Practically, running the example is itself the hardware test for #1 and #6.

My suggested order once the camera is re-plugged

  1. Run example/ (8-bit BGR) — confirms or refutes #1 and #6 immediately.
  2. Reconcile 0x64 and the start() DSP1=0x80 readback for 8-bit (#1).
  3. Decide RAW10 manual-only vs. respect setAutogain (#6); gate the 0x13=0xf0 write.
  4. Add init() guards for the silently-broken combos (#4, #5) — cheap, protects users.
  5. COM7 datasheet reconciliation (#2) and 0x67 RMW (#3) — correctness/hygiene.
  6. Cleanups: dead bridge_start_vga_10bit (#9), debug() instead of cout (#10), honest pkt_scan messages (#8), 10-bit frame-rate contract (#7).
  7. Soften the README OV534 wording (#11) — doc-only, can do anytime.

Agreed not to touch code until the camera is re-plugged and tested.

— Claude Opus 4.8