feat(reolink): detect a motorised lens instead of assuming static cameras have none

[joseg20]

What

ReolinkCamera refuses zoom and focus commands whenever cam_type == "static". The guard appears three times — in start_zoom_focus, set_manual_focus and focus_finder, the last returning a hardcoded 720.

cam_type describes how a camera is mounted — whether it pans and tilts — which says nothing about its optics. Reolink bullets sit fixed on their mast and still ship a motorised varifocal lens.

This PR asks the camera instead of inferring from the type.

Why it matters

In our Chilean deployment every camera registered as static is either an RLC-811A or a P430, both with a motorised 5× lens. We surveyed 14 of them across four stations and all report a zoom position. Four are already sitting at a non-zero zoom (7, 28, 1 and 1), set by hand through the Reolink app, because the platform cannot drive them.

Worse than being blocked, the command fails silently. start_zoom_focus returns None without raising, so POST /control/zoom/{ip}/{level} answers 200 OK and nothing happens. The operator has no way to tell the command was dropped.

Note that the endpoint already gets this right: routes_control.py guards on hasattr(cam, "start_zoom_focus"), a capability check. Only the adapter falls back to the camera type.

How

GetZoomFocus reports a zoom position only on models that can drive the lens, so the answer comes from the device and holds for any model, present or future.

The probe is cached: a lens cannot grow a motor at runtime, and zoom commands are frequent enough that an extra request each time would be wasteful.

A camera that cannot be reached is treated as fixed-lens rather than raising, so a network blip degrades the feature instead of taking the call down.

Verified on hardware

Tested against a production RLC-811A registered as static:

1. initial state        zoom=0 focus=336
2. StartZoomFocus pos=5  http=200 code=0
3. after the command    zoom=5 focus=1882   -> lens moved
4. restoring to 0        http=200 code=0
5. final state          zoom=0 focus=449    -> restored

A capture taken afterwards is sharp and correctly framed.

One thing worth knowing before exposing the control to operators: driving the zoom makes the camera refocus. Focus read 336 before, 1882 at zoom 5, and settled at 449 once back at 0. That is expected behaviour for a varifocal lens, not a side effect of this change.

Compatibility

Fixed-lens cameras keep their current behaviour. They report no zoom position, so the commands stay unsent, exactly as before. PTZ cameras are unaffected.

Tests

Six unit tests in pyro_camera_api/tests/test_reolink_lens.py cover a static camera with a varifocal lens, a fixed-lens one, an unreachable camera, the caching, and both branches of start_zoom_focus. The existing suite passes unchanged (16 tests total).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ab092a0b1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Do not cache an inconclusive lens probe

When the first GetZoomFocus probe receives an HTTP error or a nonzero Reolink response, get_focus_level() returns None, which this expression converts to and permanently caches as False. For an existing PTZ camera experiencing a transient API failure, every subsequent zoom and manual-focus command will therefore skip its StartZoomFocus request until the service restarts, even after the camera recovers; only cache False when a successful response actually establishes that zoom is unavailable.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Remove the API's static-camera autofocus gate

For the static varifocal cameras this new branch is meant to support, POST /focus/focus_finder still rejects the request before calling this method: routes_focus.py lines 121-122 unconditionally return 400 whenever cam_type == "static". Consequently the changed capability check cannot enable focus optimization through the camera service; the route needs to use the lens capability rather than the mounting type as well.

Useful? React with 👍 / 👎.

[joseg20]

Thanks — both findings were real and are fixed in 365cbd7.

P1, inconclusive probe cached as False. Correct, and the worse case is the one you describe: a PTZ camera hitting one transient error would have lost zoom and manual focus for the rest of the process, with no way back short of a restart. get_focus_level() returning None now skips the command without caching, so the next call probes again. Only a successful response settles the answer either way. Added a test that a None probe is not remembered.

P2, the autofocus route. Also correct: routes_focus.py rejected static cameras before the adapter was ever reached, so the capability check could not take effect through the camera service. The route now asks the camera the same way. It uses getattr(cam, "has_motorised_lens", None) and allows the call when the adapter cannot answer, so adapters that do not implement the probe keep their current behaviour rather than being locked out.

While re-checking I also found that my lint suppression used # ruff: ignore[...], which only exists in ruff 0.16. Against the version this repo pins (>=0.14,<0.15) it is an unknown directive and would have failed CI. Reverted to # noqa: S106 and re-ran with 0.14.14: ruff check, ruff format --check and the 17 tests all pass.

[MateoLostanlen]

HI @joseg20

Thanks for this, the capability probe is clearly the right approach and the hardware validation helps a lot. Three points before merge:

1. Cameras that reject GetZoomFocus would be re-probed on every command

get_focus_level() returns None in two very different situations: the camera is unreachable, and the camera answered with a non-zero error code. The probe treats both as inconclusive and never caches. That is fine for a network blip, but a fixed-lens model that rejects the command outright (instead of just omitting zoom.pos like the RLC-811A) would pay an extra HTTP request plus a warning log on every single zoom/focus call, forever. Could you treat a well-formed error reply as a real answer (cache False) and only leave transport failures uncached?

2. The silent 200 is still there on the control route

The PR description rightly points out that POST /control/zoom/{ip}/{level} answers 200 while the command is dropped. After this change that is still the case for a genuinely fixed-lens camera: the route only checks hasattr(cam, "start_zoom_focus"), so it returns "Zoom set to X" while the adapter silently returns None. Same for /focus/manual. Since you added the capability guard to /focus/focus_finder, the same check on those two routes would actually close the operator-visibility gap you describe. Fine as a follow-up PR too, but let's decide where it lands.

3. focus_finder can wipe a hand-set zoom, and #391 reworks this area anyway

With the route guard lifted, running /focus/focus_finder on a static varifocal camera whose focus_position is still None starts with start_zoom_focus(0), which would erase the zoom levels your operators set by hand (the 7, 28, 1 and 1 from the description). Also note that #391, which we plan to merge soon, rewrites focus_finder and the route guard (supports_focus_search(), automatic calibration at patrol startup), so this part of the PR conflicts with it directly. I would suggest scoping this PR to the adapter change (the probe plus the three guards) and rebasing the routes_focus.py part on top of #391 once it lands, making sure calibration preserves the zoom on these cameras.

[joseg20]

All three landed in 2ddcaa9. Thanks — point 3 in particular was a regression I had introduced and missed.

1. Rejections are now settled answers. You were right that the probe conflated two different things. It no longer goes through get_focus_level(), which flattens everything to None, but reads the reply itself:

Outcome	Cached
Request raises (transport)	no, retried
HTTP != 200	no, retried
HTTP 200, non-zero Reolink code	yes, False — the camera answered
HTTP 200, code 0	yes, on whether zoom.pos is present

I kept HTTP errors uncached: a 500 is the device tripping over itself rather than a statement about its optics, and caching that would reintroduce the stranding problem from your first review. Tests cover the rejection being probed once, and both the transport and HTTP-error paths being retried.

3. Dropped from this PR. You are right that lifting the route guard would let the search run on a static varifocal camera with focus_position unset, and that path opens with start_zoom_focus(0). That would wipe the hand-set zoom levels I cited as evidence in the description — the very state this change exists to respect. routes_focus.py is back to untouched, so the path stays unreachable and there is nothing to conflict with #391.

2. Closed on the zoom route only. /control/zoom lives in routes_control.py, which #391 does not touch, so the guard goes there now: hasattr only proves the adapter has the method, so without it the route answered 200 for a command the adapter had dropped — the silent failure this PR set out to remove. /focus/manual and /focus/focus_finder are both in routes_focus.py, so they belong to the follow-up, where all the focus routes can be made consistent at once on top of #391.

One thing worth flagging: #391 also changes reolink.py by +74/-52, so it will conflict with the adapter change here regardless of the route scoping. Whichever lands first, I am happy to rebase — just say which order suits you.

33 tests pass, ruff check and ruff format --check clean against the pinned 0.14.14.

[MateoLostanlen]

Thanks, the three points are addressed. Two things need changing before merge, both on paths that were not covered by the earlier rounds.

1. stop_stream now wipes the hand-set zoom.

routes_stream.py:172 does a best-effort cam.start_zoom_focus(position=0) after every stream stop. On a static camera that used to be a no-op. With the probe in place it is a real command:

stop_stream on a static varifocal camera sends: ['GetZoomFocus', 'StartZoomFocus']

So on the Chilean cameras sitting at zoom 7, 28, 1 and 1, stopping a stream silently resets the detection framing to 0. That is the same regression you caught in point 3 of the last round, on a caller that was not grepped. Either the reset is intended and should restore the pre-stream zoom rather than 0, or this path should stay neutral for cameras whose zoom was set outside the platform. Your call which, but the current behaviour destroys the exact state this PR exists to respect.

2. The probe can raise, which the PR says it does not.

The try only wraps requests.post. The parsing sits outside it:

non-JSON body          -> RAISED: ValueError Expecting value
code 0, no ZoomFocus   -> RAISED: KeyError 'ZoomFocus'

The exception propagates out of has_motorised_lens() and out of start_zoom_focus(). On /control/zoom the guard runs before the try, so a camera answering 200 with a body you did not expect gives a 500 instead of the 400 the guard is there to produce. "A camera that cannot be reached is treated as fixed-lens rather than raising" only holds for the transport failure today. Moving response.json() and the indexing inside the try, or reading through .get(), covers it.

Two smaller things, not blocking.

The focus_finder guard is unreachable: routes_focus.py:127 still rejects static cameras, so no caller can get there. It is also the one line #391 rewrites. Dropping it removes almost all of the reolink.py conflict you flagged, since the rest of #391 touches focus_finder and _measure_sharpness rather than the zoom guards.

And on the caching table: Reolink answers HTTP 200 with a non-zero code for auth failures too, not only for an unsupported command.

probe with {"code":1,"error":{"rspCode":-7,"detail":"login failed"}} -> False
then a valid reply with zoom.pos=3                                   -> False

A PTZ camera that fails one login loses zoom and manual focus until the process restarts, which is the stranding Codex raised, through a different door. Since you have the hardware, could you check what an RLC-811A actually returns for a bad password? If the unsupported-command rspCode is distinct, keying the cache on that instead of on code != 0 settles it.

[MateoLostanlen]

Hi @joseg20

Thanks — not caching a rejected probe is the right call. Two points from the last round are still open, plus one small one worth closing while you are in here.

1. stop_stream still resets the zoom to 0 — routes_stream.py:148

To answer the obvious objection first: resetting after a stream is the right intent, it is the target value that is wrong. On a PTZ camera the platform owns the zoom and 0 is the detection framing. On the Chilean static varifocals it is not: the 7, 28, 1 and 1 were set by hand through the Reolink app precisely because the platform could not drive the lens, so that hand-set value is their detection framing. Before this PR the call was a no-op there, so nothing was lost. Now it is a real command, and stopping a stream leaves those cameras at a framing nobody chose.

Restoring the pre-stream zoom keeps your intent and costs very little: read get_focus_level()["zoom"] in start_stream before the pipeline goes up, stash it alongside the stream state, and pass that value instead of 0 in stop_stream. Fall back to 0 if the read fails. On a PTZ camera sitting at 0 the behaviour is identical to today.

A stored per-camera "detection zoom" is the real answer once operators can drive the lens from the platform, but that is a follow-up, not this PR.

2. The probe can still raise — reolink.py:95 and :99

response.json() and payload[0]["value"]["ZoomFocus"] sit outside the try. A camera answering 200 with a non-JSON body raises ValueError; one answering code: 0 without a ZoomFocus block raises KeyError. Both propagate out of has_motorised_lens() and out of start_zoom_focus(). On /control/zoom the guard runs before the route's own try, so you get a 500 where the guard exists to produce a 400. Widening the try over the parse, or reading through .get(), closes it.

3. Drop the focus_finder guard — reolink.py:356

routes_focus.py:121 still rejects static cameras, so no caller can reach it. Removing it also takes almost all of the reolink.py conflict with #391 with it, since the rest of #391 lands on focus_finder and _measure_sharpness.

On the cache — not blocking. Never caching a rejection is the safe choice and I am fine with it: a login failure can no longer strand a camera. One consequence to handle: the logger.warning at reolink.py:97 now fires on every zoom and focus command for a camera that genuinely rejects GetZoomFocus, which will fill the log. Warn once per camera, then drop to debug. If you do get the RLC-811A's rspCode for a bad password and it is distinct from the unsupported-command one, keying the cache on that is a nicer answer, but not worth blocking on.

CI. Both red jobs are stale and unrelated: the docker apt-get break and the test_engine.py failures were on develop when this last ran on 23 Aug. develop is green again at 2465419, so a rebase should clear them.