Add support for multi-GCS operation #12410
Conversation
Useful to test several instances of QGC on the same machine, to test multi GCS setups
|
@DonLakeFlyer I applied your latest comments, thanks for the review! |
| if (commandLong.target_system != MAVLinkProtocol::instance()->getSystemId()) { | ||
| return; | ||
| } | ||
| if (commandLong.command == MAV_CMD(32100)) { // MAV_CMD_REQUEST_OPERATOR_CONTROL |
There was a problem hiding this comment.
Why the magic number here?
There was a problem hiding this comment.
For some reason it would not pick it otherwise. I suspect in the new mavlink implementation for some reason we are not getting the ids on development.xml.
I spent some time tracing it but couldn't find the cause and I needed to move on.
If this works nicely we will move it to common soon probably, that's why I really didn't thought much about it. Makes sense?
There was a problem hiding this comment.
This pull missing a mavlink version update? Hence development.xml is using an old version which doesn't include these new commands. You should just need up update the mavlink commit.
You can see it here:
| safeRequestTimeoutSecs = SettingsManager::instance()->flyViewSettings()->requestControlTimeout()->cookedDefaultValue().toInt(); | ||
| } | ||
| sendMavCommand(_defaultComponentId, | ||
| MAV_CMD(32100), // MAV_CMD_REQUEST_OPERATOR_CONTROL |
There was a problem hiding this comment.
same #12410 (comment)
| } | ||
| sendMavCommand(_defaultComponentId, | ||
| MAV_CMD(32100), // MAV_CMD_REQUEST_OPERATOR_CONTROL | ||
| false, // Don't show errors, as per Mavlink control protocol Autopilot will report result failed prior to forwarding the request to the GCS in control. |
There was a problem hiding this comment.
What happens if there is no response from the vehicle because the message never makes it to the vehicle.
There was a problem hiding this comment.
We will never know if it is that of something went wrong on the request.
I guess such situations could be dealt with in the future if we ever iterate this "sub-protocol". For awareness @hamishwillee, when sending REQUEST_OPERATOR_CONTROL I am telling QGC to not report if command failed, because if we are asking other GCS for control the command will returned failed as per protocol and that will show an annoying popup in QGC telling the command failed, hence Don's comment. What do you think?
There was a problem hiding this comment.
No response to a mavlink command is different than getting a failure return. The problem is you are using the generic version of sendMavCommand. You should be using sendMavCommandWithHandler which allows you to create custom handling of errors. This allows you to distinguish between a failure response and no response at all with MavCmdResultFailureNoResponseToCommand. That way in the case of no response you can tell the user that the vehicle did not respond to the request.
|
The other top level problem with this is that the toolbar indicator dropdown doesn't really match the new style of ui. That's fine for now. I can come in after this is merged in and clean it up. |
|
@DonLakeFlyer thanks for the review. I addressed most of your requests, but some others required clarification, so I left the corresponding threads opened. About UI and style, sorry about it, this was thought for 4.4 initially. I personally don't think it is a huge deal, because this popup, together with the gimbal one, are not quite the same as the other top toolbar indicators. The rest of indicators usually report telemetry, so the logic of the new UI makes total sense, but I am not sure I would change this one. I am open to it though, so we can come back to this in the future if that is fine, to not hold the PR because of it? |
No need to update UI in this pull. Once this goes through I'll take a look at it later on to see if anything needs updating. |
There was a problem hiding this comment.
Thanks for the screenshare, it looks great!
One thing that I find a bit confusing is that you have to do two actions: first request, and then secondly confirm. Is that really required?
I would have expected to get control immediately if I requested it and it gets approved.
Hi Julian, thanks for the review. I am not sure what exactly you mean, but:
I imagine you were referring to the second scenario. This is something @hamishwillee and I discussed back at the time, and we ended up with that procedure. I think the reasoning was that the "requesting GCS" does not know exactly when the GCS in control will grant control, so it could grant control when the "requesting GCS" is not expecting it. So this procedure of first allowing takeover, so "requesting GCS" can then get control instantly with a click made more sense, was more predictable and safer for everybody. |
|
@DonLakeFlyer I spared some time today to look at your pending requests. About mavlink and magic numbersI updated mavlink to the latest version, tried to rebuild, but it still fails if I use MAV_CMD_REQUEST_OPERATOR_CONTROL instead of the magic number MAV_CMD(32100). I used to be familiarized with how mavlink is integrated in QGC, but it has changed so much, and each time I look at it has changed again, so I am sorry but I don't have the time to troubleshoot why it isn't picking the commands in development.xml. I would not mind doing it if it was a stable version, but I would hate to spend the time for it to change again next time I look at it. About handlers #12410 (comment)Great point, and I agree that is the neat way of doing it. However I don't think I will have time until late April to spend time on this again. Do you think that should prevent the PR getting in? It is fine if you do, I will fix it in April. Thank you very much for your review. |
|
Another solution would be to leave this PR as draft, or close it, and do a PR against Stable QGC. |
That doesn't make sense since development.xml stuff is debug build only. Also 5.0 will be out in a couple months. |
Create an Issue for this, assign it to yourself. And then I'm fine with it going in like this. |
|
@DonLakeFlyer good point about stable, makes sense. I created the issue #12552 Thanks! |
|
I'll take a stab at the mavlink version upgrade... |
It's pretty easy, it's just here. Not sure if you need a clean full build if you change that: |
|
And then you need to |
|
No wait, it was working now it's not.. still looking... |
It's broken upstream. I'll fix that, and bump the mavlink in that pull. |
|
So once my fix goes through. You'll need to rebase. Then make sure to |
|
Please note I did a mavlink update right as you described earlier today, see commit b9e0dea, right before #12410 (comment) and it didn´t work for me. About the QGC_DAILY_BUILD part, sure! I will be a couple of weeks away so maybe I can not service it on the next few days, but I will be back on April. Thanks Don! |
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 thread instead. Mavlink docs will be updated shortly, when we confirm all of this looks good.
On this PR, the implementation is as follows:
New top toolbar indicator
This indicator will be populated if the active vehicle is sending the new CONTROL_STATUS message. On this icon we can grasp:
Note this icon will have an animation effect whenever the control status changes, whether it is because of a change in takeover allowed, or because of a change in system in control.
This way, we have some variants:
On this case this GCS (252) is in control, as we see the the sysId label green, and also the aircraft icon and the segment joining label and aircraft.
On this case, this GCS (252) is not in control ( label is white, not green ), and also the aircraft icon and segment joining is white, indicating automatic takeover is not allowed ( we need to ask to the GCS in control first )
On this case it is similar to the above situation, only automatic takeover is allowed ( aircraft icon in green ) so we don't need to request permission to the GCS in control, we can adquire control directly.
Expanded menu after clicking top toolbar indicator
After clicking the top toolbar icon, we see an expanded panel. This panel changes depending on the particular control situation. On this panel, we can:
Control request procedure when takeover is allowed
In the most simple case where we are not in control and takeover is allowed, just clicking "Acquire control" will grant us control of the vehicle.
Note that on this situation, we can choose before acquiring control if we want to allow automatic takeover or not, on the tickbox. If we want to change this after being in control it is possible too, see previous screenshot.
Control request procedure when takeover is not allowed
On this situation, we will have the following panel:
Clicking "send request" will send a control request to the GCS in control, with a timeout specified in "Request timeout (sec)", in this case 7 seconds. This timeout will be sent as a parameter in the command, to syncronize progress indicators in both GCS.
We can not sent another request until that timeout expires. This way, after clicking "send request", we will see the following:
And in the current GCS in control, a popup will appear indicating the request, with a syncronized countdown:
Clicking "Ignore" will just discard the popup, with no consequences, and if we click "allow takeover" the requestor GCS will be able to adquire control directly, as in the previous section "Control request procedure when takeover is allowed". However, we will see the following popup appear:
We have a timeout of 10 seconds after clicking "allow takeover" above. If after those 10 seconds the requestor GCS didn't take control, automatic takeover will be set to disabled again. This will happen regardless of discarding ( clicking Ignore ) on the panel above. This is done as a security measure so operators don't forget they accepted a takeover that was never fully completed.
Aditional comments
A couple of new command line arguments were added, in order to make testing easier:
Using both arguments it is handier to test this in SITL
Testing status
This has been tested in SITL using the following Ardupilot branch ArduPilot/ardupilot#29252
Here is a short video showing this testing:
multi-GCS-demo-2025-02-08_17.12.46.mp4
For awareness @hamishwillee @rmackay9. Also @julianoes might want to take a look at it too, for Px4 support.
Thanks to https://harrisaerial.com and https://www.lincesystems.com for sponsoring this PR, and to @hamishwillee for his wise and valuable support, experience and patience during all the iterations that preceded this work.