Add support for multi-GCS operation #29252

Davidsastresas · 2025-02-08T16:00:00Z

This PR adds support for the recently added multi-GCS mavlink subprotocol mavlink/mavlink#2158. This allows graceful change in control ownership on a system with multiple GCS. Please note this subprotocol does not attempt to cover security, it assumes all the operators are in contact between them and they work in a collaborative manner.

For more information about the protocol itself, please read that mavlink thread instead, and also please take a look at the corresponding QGroundControl PR here mavlink/qgroundcontrol#12410. I am also attaching a short video showing how this was tested in SITL:

multi-GCS-demo-2025-02-08_17.12.46.mp4

How the implementation works

GCS UI is populated based on CONTROL_STATUS being received. For single GCS operations, it is probably desirable to not show such UI, and not send that message at all. For that reason I left the evaluation to send this message or not at initialization, based on SYSID_ENFORCE parameter. If set to 0 at boot up, this message won't be sent at all, and no UI will be populated.
When the current GCS in control releases control, it only sets takeover allowed to true, but does not change sysid_mygcs. Mavlink protocol says we should use 0, no one in control, but maybe this is too hardcore at the moment, until we are sure all of this makes total sense. So the current solution of setting takeover allowed to true from my point of view is a balance. Let me know your thoughts.
As it is now it will assume all the GCS are connected to the same link. Is it worth implementing something so this can work between different links? for example a GCS connected on serialport x to a 900MHz radio telemetry and another GCS possibly connected to a different serial port to a serial to ethernet converter and a digital Ip link.
On link loss, when we stop seing heartbeats from the GCS in control, we set the allow override, so other GCS can take over automatically.
Right now, when a GCS that is not our SYSID_MYGCS sends a command it just ignores the command, so the GCS ack mechanism times out and we see a "no response" message. When we were working on this at the mavlink level, @hamishwillee suggested the use of a new MAV_RESULT_PERMISSION_DENIED, but in the end we ditched it due to the extra complexity. Maybe we should come back to this later if this concept works, to make more user friendly when a GCS not in control is exchanging messages with the vehicle.
In any case, I don't think this one in particular is too bad at the moment.
As we are basing this on sysid_enforce, when a GCS is not in control it will have trouble to download the mission and parameters. Maybe we should allow parameters and mission to be sent to GCS that are not in control?

Notes

We need this mavlink changes ArduPilot/mavlink#382 for this PR to build

Pending work to do

This PR only adds support for Copter. If everything looks good we should port it to the other kinds of vehicles too.
Make this work between different mavlink channels?
Allow parameters and mission messages to be sent to a GSC that is not in control?

For awareness @rmackay9

timtuxworth · 2025-02-09T19:11:55Z

As it is now it will assume all the GCS are connected to the same link. Is it worth implementing something so this can work between different links? for example a GCS connected on serialport x to a 900MHz radio telemetry and another GCS possibly connected to a different serial port to a serial to ethernet converter and a digital Ip link.

Absolutely - I do this all the time

hamishwillee · 2025-02-11T23:48:04Z

When the current GCS in control releases control, it only sets takeover allowed to true, but does not change sysid_mygcs. Mavlink protocol says we should use 0, no one in control, but maybe this is too hardcore at the moment, until we are sure all of this makes total sense. So the current solution of setting takeover allowed to true from my point of view is a balance. Let me know your thoughts.

No, you need to change CONTROL_STATUS.sysid_in_control to zero in this case.

The reason is that this information isn't just for other GCS. It is for components of the system to know what the system in charge is. If you don't set this to 0 then they will refuse commands from anything other than the GCS that used to be in control.

hamishwillee · 2025-02-11T23:56:34Z

As we are basing this on sysid_enforce, when a GCS is not in control it will have trouble to download the mission and parameters. Maybe we should allow parameters and mission to be sent to GCS that are not in control?

FWIW My intent with this protocol was that parameter and mission data from a controlled system should be available to all GCS, in the same way as telemetry. However a controlled GCS should only accept a mission upload from its controlling GCS.

The protocol wording is something like "a controlled component/system should only accept commands and command-like messages from its controller" so we should perhaps clarify that broadly speaking telemetry and metadata are expected to be available to all GCS.

Davidsastresas · 2025-02-13T14:08:58Z

Thanks for the comments @hamishwillee. I would like to get the Ok from some of the AP dev team before proceeding with the changes.

@rmackay9, could you take a look at this one? Thanks!

peterbarker

What GCS infrastructure supports this as it currently stands?

I will create a MAVProxy module at some stage to do this of course.

peterbarker · 2025-03-10T22:54:03Z

ArduCopter/GCS_MAVLink_Copter.cpp

 }
+void GCS_MAVLINK_Copter::set_sysid_my_gcs(uint8_t sysid) const
+{
+    copter.g.sysid_my_gcs.set_and_save(sysid);


We do not do this - definitely not "save" and not "set".

We've learnt over time that changing the vehicle parameters based on random other mavlink messages leads to a lot of pain. The most obvious of these were the "set-speed" mavlink messages!

Alternative here would probably make SYSID_MYGCS a reboot-required parameter and initialise a little object at boot-time.

peterbarker · 2025-03-10T22:54:54Z

ArduCopter/GCS_MAVLink_Copter.cpp

 {
    return copter.g.sysid_my_gcs;
 }
+void GCS_MAVLINK_Copter::set_sysid_my_gcs(uint8_t sysid) const


This absolutely doesn't belong in Copter. If you're finding it convenient to work only in the Copter directory, fine - but itreally should be up in libvraries/GCS_MAVLink

peterbarker · 2025-03-10T22:56:42Z

ArduCopter/GCS_MAVLink_Copter.cpp

 {
    return copter.g2.sysid_enforce;
 }
+bool GCS_MAVLINK_Copter::control_takeover_allowed() const


This implementation might benefit from a little sub-object, in the same way we have for Params and MissionitemProtocol

peterbarker · 2025-03-10T22:59:04Z

ArduCopter/GCS_MAVLink_Copter.cpp

+}
+void GCS_MAVLINK_Copter::set_control_takeover_allowed(bool takeoverAllowed) const
+{
+    return copter.g2.control_takeover_allowed.set_and_save(takeoverAllowed);


What's your reasoning for persisting things?

peterbarker · 2025-03-10T23:00:35Z

libraries/GCS_MAVLink/GCS_Common.cpp

+        // We evaluate SYSID_ENFORCE parameter to send or not CONTROL_STATUS. This message is what
+        // populates GCS UI for multi GCS control
+        if (sysid_enforce()) {
+            set_mavlink_message_id_interval(MAVLINK_MSG_ID_CONTROL_STATUS, 5000);


Definite no-no. Should be in the streams.

I did this one to not send this message unless really required. I thought it would be pointless to share it with other streams, as for the case of single GCS operations it would be a small but nonetheless waste of bandwidth, as it would not be used at all.

peterbarker · 2025-03-11T02:17:32Z

libraries/GCS_MAVLink/GCS_Common.cpp

+            return MAV_RESULT_FAILED;
+        }
+    // Release control
+    } else {


Pointless else

peterbarker · 2025-03-11T02:18:38Z

libraries/GCS_MAVLink/GCS_Common.cpp

+            send_control_status();
+            return MAV_RESULT_ACCEPTED;
+        // A release control command should always be sent from the GCS in control. Return failed otherwise
+        } else {


Pointless else, and sysid check should be inverted

peterbarker · 2025-03-11T02:21:56Z

libraries/GCS_MAVLink/GCS_Common.cpp

+    // Allow REQUEST_OPERATOR_CONTROL packets. This is needed for a GCS not in control
+    // to communicate with the vehicle, so it can forward the request to GCS in control.
+    if (is_control_request_packet(msg)) {


Why does the vehicle need to be in the middle of this negotiation?

I'm not saying it's wrong.

When we thought about this protocol we thought about this paradigm, and some reasons I remember:

We thought the vehicle should have the ultimate decision to proceed or not on such negotiations. Imagine in the future we add a layer of security to this, and it relies on some kind of key that all parties need to know. And even on current implementation we have certain control over it, for example disabling SYSID_ENFORCE parameter to disable the protocol completely, including GCS UI.

In the case of mesh radios I guess it would be good to talk GCS - GCS, but if these mesh radios don't work as they are supposed to, or if we are using non mesh radios, we rely on Autopilot mavlink router to make sure the messages arrive to the correct GCS. The initial idea was to handover control between 2 operators in mountain environment, so GCSs would not be having direct contact between them, only through vehicle.

peterbarker · 2025-03-11T02:23:24Z

libraries/GCS_MAVLink/GCS_Common.cpp

 }
 #endif  // AP_MAVLINK_MSG_RELAY_STATUS_ENABLED

+void GCS_MAVLINK::send_control_status() const


Everything needs to be behind a feature define

Noted. would MAVLINK_MULTIGCS_PROTOCOL_ENABLED work?

peterbarker · 2025-03-11T02:23:57Z

libraries/GCS_MAVLink/GCS_Common.cpp

    return false;
 }

+bool GCS_MAVLINK::is_control_request_packet(const mavlink_message_t &msg) const


This is an awfully expensive function to be doing on every incoming packet!

I know it isn't ideal but:

It is only awfully expensive for command long and int. For the rest of the messages it only checks msgid, similar to what we are doing above with RADIO_STATUS messages.

What other approach could you think about to do this? We are in a situation where we are ignoring all messages from other GCS, but we really want control request commands to get in. Maybe using a different msgid for these messages we want to let in, so it is less expensive to check for them? We actually talked about this when we were building the protocol and after going back and forth several times we decided to stick with regular commands instead of specific new messages.

Davidsastresas · 2025-03-11T10:00:03Z

Thanks very much for the review Peter.

What GCS infrastructure supports this as it currently stands?
This PR was tested using this QGC master PR mavlink/qgroundcontrol#12410. As soon as this is confirmed I will port it to QGC stable too.

About my motivation to persisting data
My idea was to integrate this as seamlessly as possible on the current AP mechanism of SYSID_MYGCS and SYSID_ENFORCE, and that is how it made sense for me. I wasn't aware setting parameters that way had been discarded, I had on my mind we did it eventually for such speed messages, etc. My bad, sorry about it.

Next steps
Before starting to work on your suggestions, I think we need to clarify:

What do we do about the awfully expensive message checks? Add support for multi-GCS operation #29252 (comment)
About persisting data. Should we leave SYSID_MYGCS and SYSID_ENFORCE as they are now, and instead maybe make a new parameter to enable this, or rely on a #define to build the feature? If the feature is enabled and we always boot without any GCS in control, and we rely on a GCS asking control, it would be possible for this feature to work without persisting data. However in this case we could have trouble on initial handshake GCS-AP if SYSID_ENFORCE is set and our GCS id don't match.
Should we allow parameter download ( and maybe mission download too? ) for GCS not in control? I think all current GCS implementations out there will misbehave trying to connect a vehicle not answering such requests. In the collaborative environment this is meant to work on, it would be good that all GCS are synced on this front too.

I think the rest of the points are clear to me. I will be out a couple of weeks but later in April I will be available again to work on this.

Thanks!

rmackay9 · 2025-04-28T23:32:42Z

Hi @Davidsastresas,

Thanks for this, could you rebase on master? Also if possible if you could address PeterB's review comments?

We're happy with the overall plan though

Davidsastresas · 2025-05-06T11:39:43Z

@rmackay9 thanks for the answer. I will rebase and address the comments Peter made that are clear to me.

However, I need some guidance/answers to complete all of them, I pointed them on my latest answer. If anybody could give me a hint it would be much appreciated, otherwise I have voids on the plan to complete this PR for it to be suitable for merging. Thanks.

Next steps
Before starting to work on your suggestions, I think we need to clarify:

What do we do about the awfully expensive message checks? Add support for multi-GCS operation #29252 (comment)

About persisting data. Should we leave SYSID_MYGCS and SYSID_ENFORCE as they are now, and instead maybe make a new parameter to enable this, or rely on a #define to build the feature? If the feature is enabled and we always boot without any GCS in control, and we rely on a GCS asking control, it would be possible for this feature to work without persisting data. However in this case we could have trouble on initial handshake GCS-AP if SYSID_ENFORCE is set and our GCS id don't match.

Should we allow parameter download ( and maybe mission download too? ) for GCS not in control? I think all current GCS implementations out there will misbehave trying to connect a vehicle not answering such requests. In the collaborative environment this is meant to work on, it would be good that all GCS are synced on this front too.

hamishwillee · 2025-05-06T21:34:25Z

Should we allow parameter download ( and maybe mission download too? ) for GCS not in control? I think all current GCS implementations out there will misbehave trying to connect a vehicle not answering such requests. In the collaborative environment this is meant to work on, it would be good that all GCS are synced on this front too.

FWIW Yes. The intent is to control what can command the vehicle, not reserve its telemetry or hide its mission. Generally you would not allow upload or setting of parameters.

Note the allowed/denied should be managed by the autopilot, but a GCS aware of this feature should also not even try.

peterbarker · 2025-07-13T00:47:27Z

So now we have #29252 which seeks to allow for multiple system IDs to be "mygcs".

peterbarker · 2025-07-13T00:59:26Z

libraries/GCS_MAVLink/GCS_Common.cpp

+    if (msg.msgid == MAVLINK_MSG_ID_COMMAND_LONG) {
+        mavlink_command_long_t command;
+        mavlink_msg_command_long_decode(&msg, &command);
+        if (command.command == MAV_CMD_REQUEST_OPERATOR_CONTROL) {
+            result = true;
+        }
+    } 


Suggested change

if (msg.msgid == MAVLINK_MSG_ID_COMMAND_LONG) {

mavlink_command_long_t command;

mavlink_msg_command_long_decode(&msg, &command);

if (command.command == MAV_CMD_REQUEST_OPERATOR_CONTROL) {

result = true;

}

}

peterbarker · 2025-07-13T01:11:20Z

... oh, should have mentioned this before, but definitely need a timeout in here so if you haven't seen your GCS for a while something else can take control

peterbarker · 2025-07-30T08:08:41Z

Hit an interesting thing where we want multiple GCSs to be considered "mygcs" - but do not want more than one to be able to provide RC inputs to the vehicle.

Davidsastresas · 2025-07-30T10:46:23Z

Thanks for the feedback Peter. Would you mind moving the converstation here mavlink/mavlink#2313 (comment) Thanks!

Davidsastresas added 4 commits February 6, 2025 13:50

GCS_MAVLINK: add support for mavlink multi-GCS protocol

18151cc

Copter: add support for mavlink multi-GCS protocol

08258ac

GCS_Common.cpp: fix PERMISSION_DENIED no longer used

b5d1c91

GCS_Common.cpp: resend param 4 for REQUEST_OPERATOR_CONTROL

40ce68c

Davidsastresas marked this pull request as draft February 8, 2025 16:23

Georacer added DevCallEU and removed DevCallEU labels Feb 21, 2025

peterbarker requested changes Mar 11, 2025

View reviewed changes

Hwurzburg added the WikiNeeded needs wiki update label Mar 11, 2025

rmackay9 added the DevCallTopic label Apr 24, 2025

CraigElder removed the DevCallTopic label Apr 28, 2025

peterbarker reviewed Jul 13, 2025

View reviewed changes

peterbarker mentioned this pull request Jul 13, 2025

GCS_MAVLink: added MAV_GCS_SYSID_HI #30627

Merged

Davidsastresas mentioned this pull request Jul 30, 2025

GCS Operator control updates mavlink/mavlink#2313

Open

peterbarker mentioned this pull request Aug 4, 2025

GCS_MAVLink: remove restriction on accepting RC_CHANNELS_OVERRIDE from my-gcs #30767

Open

Add support for multi-GCS operation #29252

Are you sure you want to change the base?

Add support for multi-GCS operation #29252

Conversation

Davidsastresas commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How the implementation works

Notes

Pending work to do

Uh oh!

timtuxworth commented Feb 9, 2025

Uh oh!

hamishwillee commented Feb 11, 2025

Uh oh!

hamishwillee commented Feb 11, 2025

Uh oh!

Davidsastresas commented Feb 13, 2025

Uh oh!

peterbarker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davidsastresas commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmackay9 commented Apr 28, 2025

Uh oh!

Davidsastresas commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamishwillee commented May 6, 2025

Uh oh!

peterbarker commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbarker commented Jul 13, 2025

Uh oh!

peterbarker commented Jul 30, 2025

Uh oh!

Davidsastresas commented Jul 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Davidsastresas commented Feb 8, 2025 •

edited

Loading

Davidsastresas commented Mar 11, 2025 •

edited

Loading

Davidsastresas commented May 6, 2025 •

edited

Loading