Add support for MAV_CMD_REQUEST_OPERATOR_CONTROL#33332
Conversation
Task #9 — takeover notification: when a takeover request is rejected (allow_takeover=0), broadcast MAV_CMD_REQUEST_OPERATOR_CONTROL on all active channels to notify the current owner so they can release or grant permission. Task #10 — heartbeat disconnect releases control: rate-limited check (1 Hz) in GCS::update_receive() releases operator control when the operator's heartbeat has been absent for GCS_OPERATOR_HEARTBEAT_TIMEOUT_MS (5 s). Uses the global sysid_mygcs_last_seen_time_ms() timestamp which is updated by any GCS in the operator range, so control is only released when ALL owners in a range disconnect simultaneously. Task #11 — gcs_secondary in CONTROL_STATUS: track secondary connected GCS sysids (sysids in the operator range that are not gcs_main) via heartbeats in a small per-GCS array. Populate gcs_secondary[] in CONTROL_STATUS from recently-seen secondaries so multi-owner range mode is correctly reflected. Autotest additions: notification forwarded to owner on takeover reject, heartbeat disconnect releases control after timeout, gcs_secondary populated when range operator is active. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ation send Restrict RC_CHANNELS_OVERRIDE and MANUAL_CONTROL to the primary operator (gcs_main) only. Secondaries can send state-changing MAVLink commands but not manual stick input, matching the spec: "only one GCS owner can control manual input of the vehicle". Fix operator control takeover notification: the original direct call to mavlink_msg_command_long_send() was silently dropped when the TX buffer was momentarily full (comm_send_lock sets chan_discard=true). Route the notification through the queued send_message()/try_send_message() path (MSG_OPERATOR_CONTROL_NOTIFICATION) so it goes out as soon as TX space is available, like all other deferred messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Georacer
left a comment
Georacer
left a comment
There was a problem hiding this comment.
This one's looking good, Peter! Thank you for your work!
I've left a couple of comments. I'll try to test it with MAVProxy next.
| if (now_ms - _operator_control_last_hb_check_ms >= 1000) { | ||
| _operator_control_last_hb_check_ms = now_ms; | ||
| if (_operator_control_sysid != 0) { | ||
| const uint32_t last_seen = sysid_mygcs_last_seen_time_ms(); | ||
| if (last_seen != 0 && | ||
| now_ms - last_seen > GCS_OPERATOR_HEARTBEAT_TIMEOUT_MS) { | ||
| set_operator_control(0, 0, false); |
There was a problem hiding this comment.
I read here:
"Every 1s, check _operator_control_sysid. If it's not 0 and I have ever seen any of my GCSs, then release the control.
That doesn't sound right. Am I reading this wrong?
I guess the intention is that if the controlling GCS goes away for more than 1s, then the operator control will be released?
And the sysid_is_gcs() logic will fall back to sysid ranges.
There was a problem hiding this comment.
You're right to be wary here, we flagged the same thing. The check keys on "any GCS in the range seen recently", so a secondary's heartbeats can mask the primary disconnecting, and a primary that's configured but never connects is never timed out (with takeover off, nobody can then take control). I think the fix is to track the primary's own last-seen separately from the range, so the timeout reflects the controller specifically. Happy to put that together if you and Peter agree on the direction.
| flags |= GCS_CONTROL_STATUS_FLAGS_TAKEOVER_ALLOWED; | ||
| } | ||
| uint8_t gcs_secondary[10] {}; | ||
| gcs().get_secondary_gcs(gcs_secondary); |
There was a problem hiding this comment.
The MAVLink spec part "It should only include IDs for connected GCS" must have been a major pain.
We're essentially maintaining this secondaries list just to report it, IIUC.
There was a problem hiding this comment.
Agreed it's a real cost. Now that the operator configures the secondary set GCS-side and it rides in the request range, ArduPilot already knows the authorized set without separately tracking who's been "seen", which is the only thing that bookkeeping buys is the spec's "only include connected GCS" wording. If that relaxed to "authorized range", AP could just report the range (the catch: gcs_secondary[10] caps at 10 while a range can be wider).
|
I gave this a test today, using the modified MAVProxy. I went through the 3 scenarios outlined in mavlink/mavlink#2158. They all seem to work... most of the time. Here are 3 successful .tlogs: I haven't actually verified that both GCSs have been logged in there. And some of the time I would get this. It would switch control and then immediately release it: Here are two failed .tlogs with that issue: |
|
Hi @peterbarker @Georacer, I've been testing this against my QGC side mavlink/qgroundcontrol#14560 (multi-GCS SITL, 4 × QGC + MAVProxy to relay traffic) and the core request / grant / takeover handshake seems solid end-to-end. I needed a few fixes on the ArduPilot side, opened as a PR against Peter's branch (peterbarker#46). The fixes: 1. Separate 2. Secondary GCS bypassed the takeover check ( 3. Notification lifecycle ( 4. Restrict release to the primary ( 5. Clamp request timeout ( Autotest ( Design questions (not addressed):
QGC already ACKs the takeover notification (COMMAND_ACK/ACCEPTED), so the full handshake completes against this branch with fix 3. |
|
@Georacer The "switch and immediately release" bug you hit might be fixed by the notification lifecycle fix (peterbarker@55117b5) Also, if you have cycles, would love your take on the open design questions from my comment above, especially the heartbeat timeout tracking the whole range instead of the primary specifically. That one's the most likely to bite us in real multi-GCS setups. @hamishwillee also said his thoughts on it in the last part of his comment ArduPilot/mavlink#503 (comment). |
|
I did some more, partial testing today:
I had not set any of
The control takeover between primary and secondary All 3 GCS can do the following:
Barring the new multi-operator control feature, I would expect this since I hadn't configured any SYSID ranges. But from a usability perspective, does this make sense? Other than that, only the primary GCS can issue joystick controls, as expected. Here are some logs from my testing: https://www.dropbox.com/scl/fo/m6aosis3eb8dre39j2t47/APxy7mjSnOChOygrPCqFwkM?rlkey=ycus3cc82zzmrctf22sc122pk&st=b7h0ho2d&dl=0 MAVProxy testing I'm still getting the flickering control status issue in MAVProxy, while running the Scenario 3 from mavlink/mavlink#2158. While running Scenario 2, I also got a ACK:Failed that I didn't expect. It's captured in I know this isn't exhaustive testing, let me know if you want me to immediately test something more specific. |
|
Regarding the question "What happens when our primary times out", my opinion is that for now it's okay to not do anything special, meaning that AP won't allow a primary change. If AP wants, we can change this implementation in another PR, so that timeouts are individually tracked and special action is taken. |
|
Thanks for the testing George. I think we need MAV_OPTIONS:GCS_SYSID_ENFORCE enabled. If a GCS is not on primary or secondary systemids it should not be able to send any command. Maybe with the parameter above enabled this works as expected. Please note also that you can "override" secondary GCSs to other than MAV_GCS_SYSID and MAV_GCS_SYSID_HI if from QGC you send a control request, and you have configured some secondary GCSs there. If the control is accepted, the new secondary GCSs will be the ones set in QGC, not the ones in AP parameters. |
It will probably behave correctly with
I'll test further the case where the |
Absolutely : "If a GCS is not on primary or secondary systemids it should not be able to send any command. Maybe with the parameter above enabled this works as expected." In range control the only special thing that the "operator in control" has (according to the spec) is ability to control sticks. But more generally, its up to the flight stack to decide the separation of powers in this mode.
This is of course something to agree with ardupilot. @Davidsastresas My thinking, as captured in the spec is that the modes are separate.
|
|
@hamishwillee Understood. In QGC-AP as it stands now we have a "hybrid" of both:
Question for Ardupilot devs: How should we differenciate between these 2 modes? aditional options in MAV_OPTIONS? |
I wasn't thinking this ^^^ (or at least I don't think so, based on how I interpet this). I think what you're saying here is that in exclusive mode AP ignores high and low parameter settings, but when you release control it reverts to whatever AP range parameters are. That's confusing to me. In exclusive mode, if you release exclusive control, then the first GCS that seizes control takes control in exclusive mode. I was thinking that the modes are distinct. In the operator in control mode though you're saying that you want to be the sole operator. So I was thinking you would have the command set the operator in control by setting both hi/lo parameters to the same value. You could even gate the modes like that - if syshi == sys_lo you are in the exclusive operator mode. If not, you're in the shared mode. Upshot, I'm a little unclear on what's going on. |
|
@hamishwillee apologies for the confusing back-and-forth on this, some of it was me muddying the water. I was thinking in a situation where we have several groups of primary-secondaries GCS. In this scenario, the only way to request control for the whole new primary-secondaries GCS group ( without changing AP parameters ) was the approach I did. But that was me being too creative and not re reading the message definition to make sure it was aligned with it. Sorry about it, I will fix it soon to be again within spec. Just to be sure we are on the same page:
In other words: the message picks who's in charge and which mode, and the flight stack owns who's allowed. Does that match how you intend it to work? If so we're aligned now and I'll implement against that; if I've mis-read any part, point me at it. Sorry about the confusion! |
|
The confusion is partially my fault. I thought I had removed MAV_CMD_REQUEST_OPERATOR_CONTROL.param5 for the SYS_HI.
Yes, though gcs_secondary might be plural.
Yes, in multi-operator control mode. The statement is true also in single-owner mode, except for this bit "The flight stack validates the requester against its configured owner set and rejects "
I don't think so (see above - we should remove param4). IMO it doesn't make sense to allow the command (at least as currently designed) to specify the ownership model: exclusive or shared control. To me that's something you set in the autopilot itself. I think it would be confusing, and possibly unsafe for a GCS to allow the sender of this message to decide "hey, we don't control the range of controlled GCS but we're going to turn them on now". Within the autopilot we don't care how that is done - I thought maybe using SYS_HI not equal to SYS_LO might be good way to indicate shared mode, but it could be an exclusive param.
I mention this because you might decide after prototyping that (say) using a range for the control status makes more sense than that array for the sysid in control. We changed it from an array because Tridge said that ArduPilot's implementation of a range should not constrain the design. The cost is that it is possilble to set a range of GCS in control that is smaller than the range we can advertise. It would be legitimate to say that is unacceptable - that we need a range, and that is what has fallen out of testing. This would mean an update of the spec. |
|
I think what you proposed makes sense @hamishwillee. The multi-operator setting is something that can completely live in the autopilot, and just be reported to the GCSs by CONTROL_STATUS. And to be honest, that simplifies the situation a lot. I think we can settle on that and test how it looks like. I will work on it in the next few days. |
4 participants
Summary
Adds support for development.xml messages (still under review) to pass control of the system from one GCS to another (or several)
Classification & Testing (check all that apply and add your own)
Description
Much discussion in various places, most notably in the mavlink PR.
There's a MAVProxy PR adding support
QGC has support in the codebase but (a) it is not enabled by default and (b) the message set that QGC compiles against by default has an older version of this message.