agent: Get/SetCameraControls — tune a local camera's exposure/gain (and make it stick) - #1818
Open
chrisdok43 wants to merge 10 commits into
Open
agent: Get/SetCameraControls — tune a local camera's exposure/gain (and make it stick)#1818chrisdok43 wants to merge 10 commits into
chrisdok43 wants to merge 10 commits into
Conversation
…, ...) The agent owns each camera's /dev/videoN node -- StreamVideo multiplexes one capture to every subscriber -- so it is the only place a V4L2 control can be set to stick. A setting made by anyone else is lost when the capture pipeline reopens the device, and on a device the app reads through the agent (CAMERA_SOURCE=wendy-agent://) there is no other writer at all. Motivating case: a scene the camera's auto-exposure gets wrong -- a flame or lamp that blows out to white -- where forcing manual exposure keeps the highlight from clipping so a fire model can see it. Measured on an Arducam UVC cam: dropping exposure_time_absolute took a candle flame from 47% clipped-white (a fire model scored 0) to a clean orange flame it scored 0.52 on. - Proto: GetCameraControls (value + range) and SetCameraControls (set, and optionally persist) on WendyVideoService; local USB/CSI cameras only, network cameras rejected. - Server: reuse the existing VIDIOC_S_EXT_CTRLS primitive (generalised to take the control class/which), add G_EXT_CTRLS and QUERYCTRL; a JSON store re-applies desired controls on every capture (re)start and across agent restarts, so a stream reconnect or reboot does not silently revert. - CLI: `wendy device camera controls <id>` and `wendy device camera set-control <id> name=value ...`. Controls are reordered so a mode control (auto_exposure=Manual) lands before the control it gates (exposure_time_absolute), which the driver marks inactive until then. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
AI Security ReviewNote Automated security review from Claude. Apply, adapt, silence with Input coverage: 12/12 changed files; 97,001/97,001 bytes reviewed; diff SHA-256 Claude found security review findings for this PR.
|
EBro912
approved these changes
Aug 27, 2026
Joannis
added
risk: mid
Joannis
requested changes
Aug 28, 2026
Joannis
left a comment
Joannis
left a comment
Member
There was a problem hiding this comment.
This is not an acceptable UX. I'm not going to guess arguments
`tunableControls` listed 14 CIDs, and a name outside it was refused as an "unknown control". That makes the feature only as good as the cameras somebody owned when they wrote the list. On a Logitech C920 it covered 12 of the 17 controls the camera actually has, silently omitting **zoom, pan, tilt, focus and auto-focus** -- every geometry control, so the service could tune the picture but not aim it. That gap is not cosmetic. Measuring a candle against the fire model on a wendy-console box: backing the camera off took detection from 98.4% of frames to 52.4%, and 3x zoom brought it back to 86.3% -- a bigger recovery than any control on the list produces. Manual exposure, which IS on the list, moved mean confidence +0.095 and the hit rate not at all. The most useful control was the one that could not be set. So enumerate instead. V4L2_CTRL_FLAG_NEXT_CTRL OR'd into the id makes QUERYCTRL return the next control the DRIVER has; walking from 0 yields the camera's whole set, vendor-private controls included. The driver's label is normalised to the spelling v4l2-ctl prints -- "Zoom, Absolute" -> `zoom_absolute` -- because that is what someone reading their camera's documentation will type. Names are then resolved per camera, so any camera attached to any device is tunable with no code change here. Verified on the device, not just in tests: the C920 went 12 -> 17 controls, `set-control zoom_absolute=250` applied and read back 250 where the same command was previously refused, and a control the camera genuinely lacks (`iso_sensitivity`) still reports cleanly instead of failing the request. Details worth keeping: * **Falls back to the static table** when the driver does not implement NEXT_CTRL. A first probe returning nothing is indistinguishable from "this camera has no controls", so a real camera would otherwise enumerate empty. * **Two loop guards**: stop after 256 controls, and stop if the driver returns the id it was given. A driver that ignores the flag would otherwise spin inside the agent forever. * Class headings (V4L2_CTRL_TYPE_CTRL_CLASS) and driver-disabled controls are skipped; the 32-byte name field is read without assuming a terminator. * The refusal text changes from "unknown control" to "this camera has no control by that name", because with enumeration the first is simply wrong: the control may be perfectly well known and merely absent here. * `controlIndexFor` joins `applyLocalControls`/`queryLocalControls` as a seam, so the tests still need no real /dev/videoN. `tunableControls` survives only as that fallback probe list, and says so. Tests: replaced TestControlCID -- it pinned a table that is no longer the source of truth -- with the normalisation cases against real driver spellings and an unterminated-name bounds check. The re-apply test now asserts CIDs are non-zero and distinct (what "resolved against the camera" can promise) rather than matching the table.
…iscover agent: ask the camera which controls it has, instead of a table
…ents
Running either command bare printed a cobra arity error and nothing else:
$ wendy device camera set-control
✗ requires at least 2 arg(s), only received 0
$ wendy device camera controls
✗ accepts 1 arg(s), received 0
That says how many arguments are missing and never what they are, so the only
way forward is to guess -- which is the review comment on #1818, verbatim: "I'm
not going to guess arguments."
Both arguments are things the tool can look up. So it does:
* `controls` with no id, and `set-control` with no arguments, list the cameras
this device actually has (id, name, path) -- the id IS the missing argument.
* `set-control <id>` with no pairs lists that camera's settable controls with
their current value and range, and prints an example using one of them.
Neither is a usage dump: the answer to a missing argument is the set of values
it could take, read off the hardware in front of you. Since #1837 the control
list comes from the camera, so what is printed is what that camera supports --
zoom and focus on a C920, whatever a different sensor offers.
Verified against a C920 over the tunnel: bare `controls` prints the one camera;
`set-control 0` prints its 14 settable controls, ranges included, and the
example line.
Args validation is relaxed to MaximumNArgs(1) and ArbitraryArgs respectively,
because the bare invocation is now a legitimate way to ask what the command
wants. Wrong-shaped pairs are still rejected by parseControlAssignments, and a
non-numeric id by parseCameraID, so nothing that used to be an error stops being
one -- only the empty case changes, from a refusal into an answer.
There was no way to undo one. `--no-persist` only declines to store the write in
front of it; the store itself had Load/merge/save/get and no remove, so once a
control was persisted it was re-asserted on every pipeline reopen forever, and
the only way out was editing /var/lib/wendy/camera-controls.json by hand.
That is a trap rather than an inconvenience, because a wrong camera setting does
not fail loudly -- it quietly costs a vision model its detections. Forcing a long
exposure so a flame blows out to white is a durable, self-healing denial of
detection that nothing in the CLI could reverse.
`CameraControl.reset` (field 8, Set only) now means: put this control back to the
driver's own default and stop persisting it. The default comes from QUERYCTRL,
so it is the camera's answer -- no caller has to know what "default" means for a
control nobody documented. The CLI exposes it as:
wendy device camera set-control 0 --reset auto_exposure,exposure_time_absolute
wendy device camera set-control 0 --reset-all
Two things this got wrong first, both found by running it against a C920:
* **The persist block re-stored what reset had just removed**, because `applied`
contains the reset controls too. Persisting now skips them -- pinning a control
to the default is still pinning, which is what reset undoes.
* **Forgetting has to be unconditional.** It was scoped to controls whose write
succeeded, so `--reset-all` left `exposure_time_absolute` in the store: the
driver reports it inactive while auto_exposure is on, the write fails, and the
stale value would be re-applied on every reopen with no way to clear it.
Reset makes two promises -- stop persisting this, and put it back -- and the
first has to hold even when the second cannot. For the same reason `--reset-all`
covers every control the camera reports, not only the currently settable ones.
Verified end to end on a C920 over the tunnel: `brightness=200` persisted and
appeared in the store; `--reset brightness` returned it to 128 (the driver's
default) and removed that entry while leaving the others; `--reset-all` then
emptied the store, including the inactive control that the first attempt stripped.
Proto regeneration touched only this file: the generator bumps a protoc version
header across all 69 generated files, so the rest were reverted and the header
pinned, leaving a diff that is just the new field.
The MCP server registers tools by hand, one file per area, and there was no camera file. The only mention of a camera anywhere in it was a description string in hardware_capabilities -- which reports that a camera exists and nothing else. So adding RPCs to the agent gave the CLI two commands and gave an agent nothing: camera_list, camera_controls and camera_set_control did not exist because nothing derives them from the proto. Three tools, following the existing pattern: * `camera_list` -- read-only. The ids the other two take. * `camera_controls` -- read-only. Name, value, range, driver default and whether the control is settable right now. Since the list is enumerated from the camera, this is that hardware's real control set rather than a fixed one. * `camera_set_control` -- mutating, idempotent, not destructive: it changes how the camera captures and that is durable, but nothing is removed and reset/reset_all undo it. Takes `controls` as name=value pairs, plus `reset`, `reset_all` and `persist`. This is the loop the tools exist for: look at the picture, notice the exposure is wrong for the scene, fix it, put it back. Every step needs the device, and until now an agent could do none of them -- which matters because a camera whose auto-exposure blows a flame out to white silently costs a vision model its detections, and the fix is one control away. `reset_all` asks the camera for its controls rather than keeping a list, and resets every one it reports, not only the currently settable ones -- a control gated inactive by a mode (exposure_time_absolute while auto_exposure is on) is exactly the one that would otherwise stay pinned in the store. Per-control `detail` is passed through on failure: "this camera has no control by that name" is actionable, a bare false is not. Verified by running the server and listing tools: 37 total, the three camera ones present, `camera_set_control` correctly annotated readOnly=false destructive=false, and device_id required on both that and camera_controls.
cli+mcp: make camera controls discoverable, reversible, and usable by an agent
81 commits behind, and one conflict: the GENERATED go/proto/gen/agentpb/wendy_agent_v1_video_service.pb.go. Both sides had edited the video service -- main added ROS 2 cameras (`topic = 12`, VIDEO_TRANSPORT_ROS2) and this branch added the camera-control RPCs and CameraControl.reset. The .proto itself merged cleanly; only its generated output collided, which is not a disagreement about anything, just two sets of codegen. Resolved by regenerating from the merged .proto rather than hand-editing a DO NOT EDIT file, then restoring the other generated files the script rewrites so the diff stays to the one that actually changed. Verified the result carries BOTH sides: Topic and VIDEO_TRANSPORT_ROS2 from main, GetReset_ and the control messages from here. go build ./... clean; go vet clean on services, mcp and commands; the services and mcp test packages pass; the agent still cross-builds for linux/arm64.
github-actions
Bot
added
risk: high
It was written 0o644 in a 0o755 directory, so any local user could read it and, depending on the directory, replace it. The contents are not secrets, but they are device configuration that decides how a camera captures — and where that picture feeds a detector, someone who can rewrite the exposure can blind it without going near the detector. The values are also re-applied on every capture reopen, so a single write persists. 0o700/0o600, matching ipcam's credential store in the same package rather than inventing a number. Pinned by a test, since a permission is the kind of thing that regresses silently. Raised as a LOW finding by the automated security review on this PR.
Contributor
Author
|
@Joannis — the UX point is addressed, in #1837 and #1838, both already merged into this branch. Arguments are no longer guessed. Running either command bare now lists what it needs instead of printing an arity error: No id lists the cameras; an id with no pairs lists that camera's controls. The answer to a missing argument is the set of values it can take, read off the hardware. The control list now comes from the camera. It was a hardcoded table of 14 CIDs, which silently omitted anything not on it — zoom, pan, tilt and focus among them. It now enumerates from the driver, so any camera exposes what it actually supports (12 → 17 on a C920). Settings are reversible. Also exposed as MCP tools ( On the automated security review: 1 (MEDIUM, no authorization on the new RPCs) — addressed by the interceptor the finding asks about. The video service is registered on the 2 (LOW, world-readable store) — fixed in 3 (LOW, re-applied on every capture restart) — the re-apply is the feature: a setting that reverts on a stream reconnect is not a setting. The "no way to undo it" half is no longer true — Mergeable, checks green. Ready for another look. |
Contributor
Swift E2E ReviewNo Swift E2E review issues were generated for this run. Report artifact: swift-e2e-tests.gh33372600213.run.0001 |
Joannis
requested changes
Aug 31, 2026
Comment on lines
+120
to
+132
| int32 minimum = 3; // inclusive; Get only | ||
| int32 maximum = 4; // inclusive; Get only | ||
| int32 step = 5; // Get only | ||
| int32 default_value = 6; // Get only | ||
| bool settable = 7; // Get only: false when the driver reports it read-only or disabled right now | ||
| // Set only: put this control back to the driver's own default and stop | ||
| // persisting it, ignoring `value`. Without this a persisted control cannot | ||
| // be taken back -- the store only ever grows, and a setting that degrades a | ||
| // scene (a wrong exposure quietly costs a vision model its detections) is | ||
| // re-applied on every reopen with no way to undo it short of editing the | ||
| // store by hand. The default comes from the driver (QUERYCTRL), so it is | ||
| // the camera's answer rather than a value the caller has to know. | ||
| bool reset = 8; |
Member
There was a problem hiding this comment.
No get/set only properties. Let's make a separate message for reading and writing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
GetCameraControlsandSetCameraControlsto the agent'sWendyVideoService, with CLI verbs:They let an operator read and set a local camera's tunable V4L2 controls — exposure, gain, white balance, backlight compensation, and friends.
Why
The agent owns each camera's
/dev/videoNnode —StreamVideomultiplexes one capture to every subscriber — so it is the only process that can set a V4L2 control to stick. A control set by anyone else is lost the moment the capture pipeline reopens the device, and on a device whose app reads through the agent (CAMERA_SOURCE=wendy-agent://…) there is no other writer at all.Motivating case: a scene the camera's auto-exposure gets wrong — a flame or lamp that blows out to white. Measured on an Arducam UVC cam, dropping
exposure_time_absolutetook a candle flame from 47% clipped-to-white (a fire model scored 0) to a clean orange flame the same model scored 0.52 on.What's in it
GetCameraControls(value + range) andSetCameraControls(set + optional persist) onWendyVideoService. Local USB/CSI cameras only; network cameras rejected.camera_controls.go): reuses the existingVIDIOC_S_EXT_CTRLSprimitive (generalised to take the controlwhich), addsG_EXT_CTRLSandQUERYCTRL. A JSON store at/var/lib/wendy/camera-controls.jsonre-applies desired controls on every capture (re)start (hook inrunProducer) and across agent restarts — so a stream reconnect or reboot does not revert to the firmware default. Controls are ordered so a mode control (auto_exposure=Manual) lands before the control it gates (exposure_time_absolute, which the driver marks inactive until then).controls(list) andset-control(set;--no-persistfor a one-shot).Testing
gofmt -l,go vet,go build ./..., and the new unit tests all pass. The device-touching ioctl path is behind seams so the RPC logic is unit-tested without a camera; it was validated end-to-end on an Arducam viav4l2-ctlbeforehand.Note
Deploying this to a device that relies on raw-thermal radiometry also needs #1817 (
feat/raw-thermal-frames); that combining is out of scope for this PR, which targetsmainon its own.🤖 Generated with Claude Code