Complete the camera parameter coverage, add VGA/2K/4K and 60 fps - #217
Draft
johannesschrimpf wants to merge 16 commits into
Draft
Complete the camera parameter coverage, add VGA/2K/4K and 60 fps#217johannesschrimpf wants to merge 16 commits into
johannesschrimpf wants to merge 16 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor/getter-setter-api #217 +/- ##
==============================================================
+ Coverage 77.88% 80.64% +2.75%
==============================================================
Files 11 11
Lines 1723 1808 +85
==============================================================
+ Hits 1342 1458 +116
+ Misses 381 350 -31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates the SDK’s camera configuration surface to align with the underlying protocol by expanding supported camera resolutions (VGA/2K/4K) and adding streaming protocol access, along with tests to validate the new behavior.
Changes:
- Extend
Camera.resolutionto support 480 (VGA), 1440 (2K), and 2160 (4K) mappings in addition to existing values. - Add
Camera.streaming_protocolgetter/setter behavior and corresponding tests. - Add/expand unit tests for
resolutionandstreaming_protocolproperties.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
blueye/sdk/camera.py |
Extends resolution mapping and adds streaming protocol property logic. |
tests/test_camera.py |
Adds parametrized tests for new resolution values and streaming protocol getter/setter validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
johannesschrimpf
force-pushed
the
streaming
branch
from
June 9, 2026 12:16
ddfa68d to
8ffcbaa
Compare
johannesschrimpf
changed the title
Comment on lines
1005
to
1018
| self._update_camera_parameters() | ||
| if self._camera_parameters.resolution == blueye.protocol.Resolution.RESOLUTION_HD_720P: | ||
| if self._camera_parameters.resolution == blueye.protocol.Resolution.RESOLUTION_VGA_480P: | ||
| return 480 | ||
| elif self._camera_parameters.resolution == blueye.protocol.Resolution.RESOLUTION_HD_720P: | ||
| return 720 | ||
| elif ( | ||
| self._camera_parameters.resolution == blueye.protocol.Resolution.RESOLUTION_FULLHD_1080P | ||
| ): | ||
| return 1080 | ||
| elif self._camera_parameters.resolution == blueye.protocol.Resolution.RESOLUTION_QHD_2K: | ||
| return 1440 | ||
| elif self._camera_parameters.resolution == blueye.protocol.Resolution.RESOLUTION_UHD_4K: | ||
| return 2160 | ||
|
|
A reusable data descriptor that warns with DeprecationWarning and delegates to the new getter/setter methods. Being a data descriptor, it also makes a typo'd assignment to a read-only shim raise AttributeError instead of silently creating a stray attribute. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
state_of_charge -> get_state_of_charge(), with a deprecated_property shim kept for backward compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
surge/sway/heave/yaw, boost/slow become get_/set_ methods; the auto_* control modes become is_*_active()/enable_* methods. Internal callers now read the private setpoint dicts directly so the SDK never warns on itself. current_thruster_setpoints keeps its dedicated 'do not set directly' error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Config.water_density and Drone.lights become get_/set_ methods; read-only telemetry (depth, pose, altitude, error_flags, active_video_streams, water_temperature, dive_time, connected_clients, client_in_control) become get_* methods. take_control now calls get_client_in_control() internally to avoid self-warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tilt, Overlay and Camera properties become explicit methods: booleans use is_*()/enable_*, values use get_/set_, is_recording -> is_recording_active()/set_recording(). All validation, version gating and unit conversion logic is preserved in the new methods, with deprecated_property shims kept for compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Gripper.grip_velocity and rotation_velocity become get_/set_ methods, with deprecated_property shims kept for backward compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update existing tests to the new getter/setter API so the suite runs warning-free, and add tests/test_deprecations.py: a parametrized check that every old property still works, warns, and delegates, and that read-only shims raise AttributeError on assignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update print_status.py and gamepad_controller.py to use the new getter/setter methods instead of the deprecated properties. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update quick_start, configuration, from-the-CLI and peripherals docs to demonstrate the new getter/setter methods. migrating-to-v2.md is left as historical content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add 480 (VGA), 1440 (2K) and 2160 (4K) to the camera resolution property, using the enum members that actually exist in the protocol (RESOLUTION_VGA_480P, RESOLUTION_QHD_2K, RESOLUTION_UHD_4K). Add tests for the resolution mapping in both directions and for the existing streaming_protocol property, which was previously untested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
blueye.protocol 3.3.0 exposes more of CameraParameters than the SDK did: * Add 60 fps to the framerate property. The Ultra supports it at 1440p and below, p2_drone and gst_rtsp_record already map and cap it. * Add the Ultra image parameters (brightness, contrast, saturation, gamma, sharpness, backlight_compensation, denoise and the eHDR options), the ISO gain, and the streaming MTU size. All of them were already handled by the drone but had no SDK property. * Deprecate the resolution property. Drones running Blunux 4.4 or newer take the resolution from stream_resolution/recording_resolution and ignore this field when setting parameters, so assigning to it silently does nothing. The integration test xfails on those drones, and a stream_resolution round trip takes its place. * Raise instead of returning None when the drone reports a resolution or frame rate the SDK does not recognize, and map both through lookup tables rather than if/elif chains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
recording_bitrate had no test coverage at all - no getter, no setter and no version gate - despite being the property our bench integration suite leans on hardest. Add those three, following the shapes of the existing parameter tests. Add the two cache contracts callers actually depend on: * Getters refresh from the drone rather than answering from the cache, so a read-back is trustworthy after the drone resolves an automatic request to a concrete value. * A setter on a camera that has not talked to the drone yet fetches the current parameters first, so the first assignment after construction cannot send a default-constructed struct and zero resolution, framerate and codec. Deliberately not covered: that a setter transmits the whole cached struct and carries unrelated fields along with it. That behaviour is what #221 is for, so pinning it here would prejudge the discussion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the integration tests against an X3 Ultra on Blunux 5.1.0 left three camera tests failing for reasons that have nothing to do with the SDK: - bitrate: the Ultra applies the stream bitrate but always reports it as 0. Measured on the wire, requesting 2 Mbit/s gives 1.98 Mbit/s and 12 Mbit/s gives 11.88 Mbit/s, so only the readback is missing. The drone fills h264_bitrate from the camera control node, and the Ultra encodes H264 in the RTSP server rather than in the camera. - hue: only available on Pioneer/Pro/X1/X3, the Ultra camera has no hue control. - framerate: the Ultra only applies 25 fps to the recording pipeline, and the reported frame rate comes from the stream pipeline, which stays at 30. Add a drone_model fixture and use it to xfail the bitrate readback and skip the two unsupported parameters, so a drone-connected run is green on both families. Cover what the Ultra does support instead: 60 fps at 1080p, which the drone caps to 30 above 1440p, and sweep every resolution this branch adds across both stream_resolution and recording_resolution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
johannesschrimpf
force-pushed
the
streaming
branch
from
August 20, 2026 13:51
0655ee3 to
7d319b9
Compare
johannesschrimpf
changed the title
johannesschrimpf
changed the base branch from
master
to
refactor/getter-setter-api
August 20, 2026 13:51
conftest has had a mocked_telemetry_client fixture since "Fix mocks for new
clients", but mocked_drone never requested it, so every unit test built a real
TelemetryClient. That opens a ZMQ subscriber against the drone IP, and with a
drone on the same network it fills the state within a few hundred milliseconds:
t=0.00s msgs_in_state= 0 ControlModeTel=False is_weather_vaning_active()=None
t=0.25s msgs_in_state= 8 ControlModeTel=True is_weather_vaning_active()=False
The tests asserting that a getter returns None when no telemetry has been
received then fail, and because it is a race between the subscriber thread and
the assertion, a different one fails on each run. CI never saw it - there is no
drone there - but a full run at a desk with a drone on it failed every time.
Wire the fixture into mocked_drone, and give the mock a real dict for _state
plus a get() that looks up in it, so the 28 tests that seed telemetry with
_telemetry_watcher._state[SomeTel] keep working. get() reads the attribute on
every call rather than closing over the dict, because several tests rebind
_state to a fresh dict to simulate telemetry going away.
Also drops the full suite from 43s to 18s, since it no longer opens a socket
and starts a subscriber thread per test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sindrehan
force-pushed
the
refactor/getter-setter-api
branch
from
August 20, 2026 14:10
530a8db to
21cf664
Compare
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
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.
Description
Completes the SDK's coverage of
CameraParametersagainst blueye.protocol 3.3.0, and adds theresolutions and frame rate the Ultra supports.
Built on top of #215, so everything here is expressed as getter/setter methods rather than
properties. Please merge that one first.
get_resolution/set_resolutionnow cover 480 (VGA), 1440 (2K) and 2160 (4K)alongside 720 and 1080, using the enum members that actually exist in the protocol
(
RESOLUTION_VGA_480P,RESOLUTION_QHD_2K,RESOLUTION_UHD_4K).get_framerate/set_framerate. The Ultra supports it at 1440p and below;p2_droneandgst_rtsp_recordalready map and cap it.brightness,contrast,saturation,gamma,sharpness,backlight_compensation,denoiseand the eHDR options (is_ehdr_enabled/enable_ehdr,get/set_ehdr_exposure_min_number,get/set_ehdr_exposure_max_number), plus the ISOgainand the streamingmtu_size. All of them were already handled by the drone but had noSDK accessor. The Blunux version requirements are enforced in the setters and in
configure().get_resolution/set_resolutionare deprecated. Drones running Blunux 4.4 or newer takethe resolution from
stream_resolution/recording_resolutionand ignore this field whensetting parameters, so setting it silently does nothing. Both now raise a
DeprecationWarningpointing at the stream/recording resolution methods. Note this is deprecation on top of the
Camera.resolutionproperty shim from Refactor to a getter/setter API #215 — reading the old property warns twice, once forthe property and once for the concept.
get_resolutionandget_framerateused to returnNonewhen thedrone reported something the SDK did not recognise; they now raise
RuntimeError. Both mapthrough lookup tables instead of if/elif chains.
Testing
Unit tests cover the resolution and frame rate mapping in both directions, the new parameters'
getters/setters and their version gating, the
streaming_protocolaccessor (previouslyuntested), and two contracts that were implicit before: a getter always re-reads from the drone
rather than answering from the cache, and a setter on a camera that has not talked to the drone
yet fetches the current parameters before sending.
The drone-connected tests are now guarded by a
drone_modelfixture, because running themagainst an X3 Ultra on Blunux 5.1.0 left three tests failing for reasons unrelated to the SDK:
bitrate— the Ultra applies the stream bitrate but always reports it as 0. Measured on thewire, requesting 2 Mbit/s gives 1.98 Mbit/s and 12 Mbit/s gives 11.88 Mbit/s, so only the
readback is missing. The drone fills
h264_bitratefrom the camera control node, and the Ultraencodes H264 in the RTSP server rather than in the camera. Marked xfail.
hue— only available on Pioneer/Pro/X1/X3, the Ultra camera has no hue control. Skipped.framerate— the Ultra only applies 25 fps to the recording pipeline, and the reported framerate comes from the stream pipeline, which stays at 30. Skipped, with
test_camera_framerate_60_fpscovering the frame rate the Ultra does support instead.A drone-connected run is green on both families (verified against an X3 Ultra on Blunux 5.1.0:
18 passed, 4 skipped, 2 xfailed). The integration tests also sweep every resolution this PR adds
across both
stream_resolutionandrecording_resolution.Also carries an unrelated test-fixture fix that a drone-connected run turned up.
mocked_dronenever requested the
mocked_telemetry_clientfixture, so every unit test built a realTelemetryClientand subscribed to the drone over ZMQ. With a drone on the same network thatfills the telemetry state within ~250 ms, so the "returns None when no telemetry has been
received" tests failed at random - a different one each run. CI never saw it because there is no
drone there. The fixture is now wired in, which also drops the suite from 43s to 18s. The same
bug is on master, so it may be worth cherry-picking there rather than waiting for this to merge.
Checklist before merging