feat(reolink): detect a motorised lens instead of assuming static cameras have none - #396
feat(reolink): detect a motorised lens instead of assuming static cameras have none#396joseg20 wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| """ | ||
| if self._has_motorised_lens is None: | ||
| try: | ||
| self._has_motorised_lens = (self.get_focus_level() or {}).get("zoom") is not None |
There was a problem hiding this comment.
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 👍 / 👎.
| if not self.has_motorised_lens(): | ||
| return 720 |
There was a problem hiding this comment.
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 👍 / 👎.
|
Thanks — both findings were real and are fixed in 365cbd7. P1, inconclusive probe cached as P2, the autofocus route. Also correct: While re-checking I also found that my lint suppression used |
…eras have none cam_type describes how a camera is mounted, not what optics it carries. A "static" camera is one that does not pan or tilt, which says nothing about zoom: Reolink bullets such as the RLC-811A or the P430 sit fixed on their mast and still ship a motorised varifocal lens. Ask the camera instead of inferring. 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. The probe is cached, since a lens cannot grow a motor at runtime and zoom commands are frequent. A camera that cannot be reached is treated as fixed-lens rather than raising, so a network blip degrades the feature instead of failing the call. Fixed-lens cameras keep their current behaviour: they report no zoom position, so the commands stay unsent exactly as before. PTZ cameras are unaffected.
…route Two problems raised in review, both real. A failed GetZoomFocus request was being cached as "no motorised lens". One transient error would have stranded a PTZ camera without zoom or manual focus for the rest of the process, long after the camera recovered. An inconclusive probe now skips the command without caching, so the next call tries again. The autofocus route rejected static cameras before ever reaching the adapter, so the capability check could not take effect through the camera service. It now asks the camera for its lens the same way, and falls back to allowing the call on adapters that cannot answer.
365cbd7 to
5192fe0
Compare
|
HI @joseg20 Thanks for this, the capability probe is clearly the right approach and the hardware validation helps a lot. Three points before merge:
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?
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.
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. |
… routes Three points from review. The probe conflated a camera that could not be reached with one that answered by rejecting the command: both returned None from get_focus_level() and neither was cached. A fixed-lens model that rejects GetZoomFocus outright would have paid an HTTP request and a warning on every zoom and focus call for the life of the process. The probe now reads the reply itself: a well-formed non-zero code is the camera settling the question and is cached, while transport failures and HTTP errors stay uncached and are retried. The capability guard moves from the autofocus route to the zoom route. Lifting it on /focus/focus_finder would let the search run on a static varifocal camera whose focus_position is unset, and that path opens with start_zoom_focus(0) — wiping zoom levels set by hand in the field, which is exactly the state this change exists to respect. routes_focus.py is left untouched, both for that reason and because pyronear#391 rewrites it. /control/zoom keeps the guard: hasattr only proves the adapter has the method, so without it the route answered 200 for a command the adapter dropped, which is the silent failure this PR set out to remove.
|
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
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 2. Closed on the zoom route only. One thing worth flagging: #391 also changes 33 tests pass, |
2 participants
What
ReolinkCamerarefuses zoom and focus commands whenevercam_type == "static". The guard appears three times — instart_zoom_focus,set_manual_focusandfocus_finder, the last returning a hardcoded720.cam_typedescribes 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
staticis 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_focusreturnsNonewithout raising, soPOST /control/zoom/{ip}/{level}answers200 OKand nothing happens. The operator has no way to tell the command was dropped.Note that the endpoint already gets this right:
routes_control.pyguards onhasattr(cam, "start_zoom_focus"), a capability check. Only the adapter falls back to the camera type.How
GetZoomFocusreports 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: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.pycover a static camera with a varifocal lens, a fixed-lens one, an unreachable camera, the caching, and both branches ofstart_zoom_focus. The existing suite passes unchanged (16 tests total).