Repository navigation
Conversation
Global Size Summary (Total Flash delta in bytes)Built from 036cf7d rebased onto afeb9c4
|
2309125 to
8f640aa
Compare
8f640aa to
1664ee3
Compare
Previous review (2026-10-10)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 1664ee3. Two things need fixing before this can merge. The clang CI failures are all one line: libraries/AP_Mission/AP_Mission.cpp:1514 compares the float packet.param2 with the enumerator CAMERA_MODE_IMAGE_SURVEY, which clang rejects under -Werror=enum-conversion. The matching check in AP_Camera.cpp:524 already casts with float(CAMERA_MODE_IMAGE_SURVEY); the same cast here compiles cleanly with clang 21. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-1500d3a42eeb51ad59732e97da1aa22b07cf37676664f4b5a0e21ea2f80cadd2R1514 RC takeover of a leased gimbal does not work in the default mount mode. libraries/AP_Mount/AP_Mount_MAVLink.cpp:32 calls set_rctargeting_on_rcinput_change() and then returns while the lease is held, but that helper only calls set_mode() when the mount is not already in RC_TARGETING or RETRACT (libraries/AP_Mount/AP_Mount_Backend.cpp:981), and RC_TARGETING is the MNTx_DEFLT_MODE default. In SITL, with the lease taken in MAVLINK_TARGETING a stick move released it and targets resumed; taken in RC_TARGETING, stick moves were ignored and GIMBAL_MANAGER_STATUS kept showing the gimbal as owner for as long as it renewed. GCS commands, mode changes and the 3 s expiry release correctly. The lease path needs its own stick-movement check that does not depend on the current mode. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-255cebd65677a9b752f02d2d81935965af956253242578fe772d4ef352ce887aR32 Smaller points, none blocking. libraries/AP_Camera/AP_Camera_MAVLinkCamV2.cpp:113 assigns the uint32_t _sysid straight into target_system and sends with send_message(), where the other senders in the file use mavlink_msg_target_field(_sysid) and send_message_target(); with a camera on sysid 1000 the mode command went out addressed to 232 with no extended target, and the caller still got ACCEPTED. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-fcde52c2e367add12c6c072168596bcf33f0fd17e3da633f61e7cd4673ca62afR113 At libraries/AP_Camera/AP_Camera_MAVLinkCamV2.cpp:108 survey mode is refused unless the camera also advertises CAMERA_CAP_FLAGS_HAS_MODES; MAVLink defines the survey flag independently, so a stills-only survey camera advertising only CAMERA_CAP_FLAGS_HAS_IMAGE_SURVEY_MODE would be refused. I found no camera that does this, so it is a question rather than a bug. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-fcde52c2e367add12c6c072168596bcf33f0fd17e3da633f61e7cd4673ca62afR108 A mission item for MAV_CMD_SET_CAMERA_MODE with NaN in the reserved param3/param4 (the dialect defaults) is rejected by sanity_check_params() at libraries/AP_Mission/AP_Mission.cpp:1026 with INVALID_PARAM3/4; SET_CAMERA_ZOOM and SET_CAMERA_FOCUS behave the same way today, so this is consistent with its neighbours, but a nan_mask entry would be cheap. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-1500d3a42eeb51ad59732e97da1aa22b07cf37676664f4b5a0e21ea2f80cadd2R1513 The new set_camera_mode() declarations at libraries/AP_Camera/AP_Camera_Backend.h:84 and libraries/AP_Camera/AP_Camera_MAVLinkCamV2.h:52 landed under the existing zoom comment. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-b77efe434d7e45b7b6124581a62fc072003e56f813827f3a8f14fd81272c59bfR84 On tests: only accepting survey mode needs the SITL MT11 changes. The lease state machine (impersonating the gimbal as MountMAVLinkTargetRefresh does), image/video SET_CAMERA_MODE routing against the existing simulated camera, and the mission upload/download round trip can all be tested now, and a lease test in RC_TARGETING would have caught the takeover problem. The attitude-interval cap in the AP_Mount commit is a real fix: without it Plane at the default 50 Hz loop never discovers the gimbal. Not checked: hardware behaviour, survey mode actually being accepted by a camera that advertises it, and the linked AP_CameraGimbal and MAVProxy PRs. The one non-clang CI failure, QuadPlane TestLogDownload ('GPS status bits did not become good'), passed when run locally on this head; its cause is unconfirmed. |
| send_target_to_gimbal(); | ||
| } | ||
|
|
||
| void AP_Mount_MAVLink::release_device_control() |
There was a problem hiding this comment.
Really should have MountTargetType::SURVEY
|
seems really great As discussed on the dev call I think we should allow the pilot or user (via the GCS) to easily re-take control of the gimbal similar to what we do for ROI mode. |
Validate camera mode requests and route them to the selected backend using the full target system ID and the capability for the requested mode. Keep the native camera command reserved parameter zero after resolving the camera selector.
Store and round-trip the camera selector and validated mode so an AUTO mission can start or stop survey mode through the camera command handler. Accept NaNs in the reserved parameters and compare the floating-point mode against a float limit for Clang builds.
Add a SURVEY mount target type for a gimbal that holds a renewable control lease, allowing its survey controller to track ground targets while the autopilot sends no targets. The lease runs in MAVLink targeting, so RC movement, GCS targets and mode changes take over as they do for other targets. When the lease expires or another controller takes over, the previous mode and target resume, without a stale rate or an ROI cleared during the lease. A retracted mount refuses a lease, and a gimbal renewing a lease it no longer holds is denied.
Plane's default 50Hz loop cannot send AUTOPILOT_STATE_FOR_GIMBAL_DEVICE at the default 50Hz mount rate, so the interval request failed and the gimbal was never discovered. Clamp the initial request with cap_message_interval().
Allow ground stations to select survey mode through the autopilot camera interface using MAV_CMD_SET_CAMERA_MODE.
Exercise RC, GCS and mode takeover, lease renewal and expiry, camera mode capability checks and extended addressing, and mission mode round trips. Verify image and video mode changes with the existing MT11 simulator.
1664ee3 to
036cf7d
Compare
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 036cf7d. This replaces the 1664ee3 review: the PR was rebased since then. Both earlier blockers are fixed, and the new tests pass here. The clang error is gone. AP_Mission.cpp:1517 casts to float, and clang 21 with -Werror=enum-conversion accepts the file, while the old line still fails. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-1500d3a42eeb51ad59732e97da1aa22b07cf37676664f4b5a0e21ea2f80cadd2R1517 RC takeover now works from the default RC_TARGETING mode: taking the lease switches to MAVLINK_TARGETING with the new SURVEY target and resets the RC baseline. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-255cebd65677a9b752f02d2d81935965af956253242578fe772d4ef352ce887aR111 In a fresh SITL build, MountMAVLinkDeviceControl, MAVLinkCameraMode, CameraModeMission and MAVLinkCameraModeSITL all pass. The smaller points are addressed as well: the extended-target camera send, the independent survey capability flag, NaN reserved mission params, and the declaration comments. One question, not a blocker. After the pilot takes the gimbal back, the gimbal can send -2 straight away and get the lease again. The held stick then becomes the new baseline. https://github.com/ArduPilot/ardupilot/pull/34688/files#diff-255cebd65677a9b752f02d2d81935965af956253242578fe772d4ef352ce887aR103 In SITL the gimbal kept control for at least 2 s that way. This matches what ROI mode does when a GCS re-sends an ROI, and AP_CameraGimbal#43 pauses on takeover, so it is fine with that gimbal. Is relying on the gimbal to back off intended for third-party gimbals that adopt the lease? Not checked: final CI on this head (still pending when fetched, no failures yet), hardware, a real camera accepting survey mode, and MAVProxy#1772. |
6 participants
Summary
Supports putting a MAVLink camera into survey mode, and lets the camera's gimbal point itself while it surveys. Linked to this PR:
ArduPilot/AP_CameraGimbal#43
and this MAVProxy PR:
ArduPilot/MAVProxy#1772
Camera mode
MAV_CMD_SET_CAMERA_MODEis routed to AP_Camera (from a GCS or a mission item), with the camera selector in param1.CAMERA_CAP_FLAGS_HAS_IMAGE_SURVEY_MODEfor survey,CAMERA_CAP_FLAGS_HAS_MODESfor the others. A survey-only camera can always be returned to image mode.Gimbal control lease
MAV_CMD_DO_GIMBAL_MANAGER_CONFIGURE(param1 -2 to take it, -1 to renew) from its own sysid/compid.MountTargetType::SURVEY, and sends no targets while the lease is held. Attitude telemetry to the gimbal continues.MAV_RESULT_DENIED, so it knows control was taken back.Also
AUTOPILOT_STATE_FOR_GIMBAL_DEVICEinterval request is capped to what the link can send. Without this, Plane at its default 50Hz loop never discovered a MAVLink gimbal.Classification & Testing (check all that apply and add your own)
New Copter autotests:
MountMAVLinkDeviceControl: no targets during a lease; RC takeover in RC and MAVLink targeting, including gradual stick movement and a baseline taken when the lease began; GCS and mode-change takeover; denied renewal after takeover; retract refusing a lease; expiry restoring the previous angle target; an ROI cleared during a lease not resumed.MAVLinkCameraMode: capability checks, NaN reserved params and extended sysid addressing for forwarded mode commands.CameraModeMission: mission upload/download of camera mode items.MAVLinkCameraModeSITL: image and video mode changes on the existing simulated camera.The existing Mount, MountMAVLinkROI, MountMAVLinkTargetRefresh, MountMT11, MountMT11Telemetry, MountRCFailAngle, MountRCFailRate and MountPOIFromAuxFunction tests also pass.