mavlink
QGC is using deeprecated mavlink message SET_MODE
QGC is using the deprecated message SET_MODE. Reference: code
This should be fixed moving the code to use MAV_CMD_DO_SET_MODE.
This is necessary, since the mavlink spec does not have a ACK for the SET_MODE message, breaking QGC actual code.
I agree that we should move to MAV_CMD_DO_SET_MODE for current systems as SET_MODE is deprecated. But might not be possible to remove if we still are supporting legacy systems that use it.
But what QGC code would be broken by using SET_MODE? Any implementation that relies on it isn't expecting an ACK.
The problem @hamishwillee is that some flight controllers think that is correct to ACK such command, and that results in unexpected behaviour if you follow the mavlink spec.
E.g: Ardupilot is sending a wrong value for command in COMMAND_ACK. Reference: code Sending 11 (set_mode) as MAV_CMD enum value, that does not exist. This command is deprecated for more than 5 years, I believe that is already time to remove it.
Your but is against QGC, and QGC itself isn't doing anything wrong :-) AFAIK it isn't waiting on any ACK. Unless it can't handle the ACK, in which case IMO the problem is that QGC is not robust enough. IMO the root cause, not the messenger.
Fundamentally I agree though - I'd happily drop support for this in QGC provided it can still talk to all the versions of the flight stack it wants to.
breaking QGC actual code.
There isn't anything broken in QGC with respect to setting flight modes. QGC code knows a flight mode changed this way isn't acked and can fail and handles appropriately in the right places. Changing to MAV_CMD_DO_SET_MODE is theoretically possible but I'm not super interested in going through the hassle of verifying which firmwares supports this or not and in what versions. If the support for the vehicle telling a groundstation what commands it supports finally happens then I can see changing this. But still super low priority.
Sounds fair to me. @patrickelectric Perhaps we should leave this open for now as we're working on knowing what messages are supported in what versions.
The problem @hamishwillee is that some flight controllers think that is correct to ACK such command, and that results in unexpected behaviour if you follow the mavlink spec.
Well, while ArduPilot does send that ACK, the programmers certainly don't believe that it is correct to ACK a SET_MODE message. Absolutely not. We simply do so so as not to break things unnecessarily. Those that (bizarrely) expect the ACK get it. Those that don't ignore it.
Personally, I'd love to kill that compatibility code off right now, as @patrickelectric 's PR does, but I don't know what it will break. It might be that the team is comfortable breaking anything that relies on broken-ish behaviour - I've already marked @patrickelectric 's PR as a DevCall topic to see if we are.
E.g: Ardupilot is sending a wrong value for command in COMMAND_ACK. Reference: code Sending 11 (set_mode) as MAV_CMD enum value, that does not exist. This command is deprecated for more than 5 years, I believe that is already time to remove it.
It would be great to eliminate SET_MODE, no question. But that requires the GCSs to stop sending it, which pymavlink (i.e. MAVProxy and dronekit-python) only did earlier this week. I wasn't aware that QGC still sent the old message through. My quick scan of MissionPlanner says that it still uses the deprecated message. My guess is that the (non-QGC) Android apps probably all use the message rather than the command, too. So. Much. Breakage. We'd need to do something like get the majors patched, wait a while, start to send a warning when receiving a SET_MODE, wait a while, remove SET_MODE.
@peterbarker Thanks very much for your clarifications.
FWIW IMO that is broken, not broken-ish, and I would have no qualms notifying a GCS to fix and breaking them if they don't. Can you let me know what was decided by the dev team?
Re removing deprecated SET_MODE agree 100%. From a MAVLink perspective this has been deprecated for ages, and I would in theory feel fine making a release that simply deletes the message. A standard has to be able to do that in order to move forward.
The problem is that we can't in good concience start doing that until we have a much better formal release and notification process. Right now no one really believes we will do a break, so this would be pretty bad.
There is a bit of a chain to fix this problem, but the first link is merging one of the PRs on nested dialect inclusion (probably Ollies, after cleanup). Let's have a call sometime and work on that?
I appreciate this is an old ticket and support was added for MAV_CMD_DO_SET_MODE here; https://github.com/mavlink/qgroundcontrol/commit/231f300e7f51bcbd7f59e5902b05c7c375732d74
HOWEVER, it seems very specific to APM and does not follow the Mavlink spec and so does not work with PX4 or other flight controllers that do follow the spec.
Anyone have any thoughts on this? Happy to create a PR to resolve in which setFlightMode returns custom_mode and sub_mode instead of just custom_mode which sometimes has the two combined.
HOWEVER, it seems very specific to APM and does not follow the Mavlink spec and so does not work with PX4 or other flight controllers that do follow the spec.
Yes, it is very specific to APM - that's why the "supports" callback exists.
If you need to change the callback to include the submode, that's cool - we can just ignore that in the "APM"-specific code.
Anyone have any thoughts on this? Happy to create a PR to resolve in which
setFlightModereturnscustom_modeandsub_modeinstead of justcustom_modewhich sometimes has the two combined.
I thought PX4 Autopilot was moving away from custom-submode....
At some stage QGC has to deal with the fact that autopilots have different modes - after all, it needs to know what mode/submode numbers to pass in!
I'm aware of the work around having a set of common modes, but this is really an area where autopilots will have differences.
Was the lack of passing in submode the only non-spec thing you were concerned about?
@peterbarker Thank you for you quick response and sorry for my very late reply
I have managed to worked around the issue on the drone side, however I still believe if MAV_CMD_DO_SET_MODE_is_supported is set then it should support the Mavlink standard which uses 3 parameters (base_mode, main_mode, sub_mode), and allow the firmware plugin manipulate all 3.
I thought PX4 Autopilot was moving away from custom-submode....
I did not know this, although I am aware of the addition of ROS2 support to allow more customisation with regard to modes.
I have managed to worked around the issue on the drone side, however I still believe if
MAV_CMD_DO_SET_MODE_is_supportedis set then it should support the Mavlink standard which uses 3 parameters (base_mode, main_mode, sub_mode), and allow the firmware plugin manipulate all 3.
... so MAV_CMD_DO_SET_MODE has been around since 2012. Until 2015 it only supported "custom mode", in 2015 someone added the submode. The documentation on the message is quite explicit about these numbers being stack-specific.
Suggesting that ArduPilot's use of "custom mode" goes contrary to the "Mavlink standard" seems a little odd given the lengthy history of the simple "custom mode" presence in the standard.
Note that ArduPilot makes an effort to fill in the "base mode" flags in the heartbeat message, based on which flightmode you're currently in. They're generally useless, except for the safety and custom-mode bits.
Having base-mode in a MAV_CMD_DO_SET_MODE doesn't really make sense; we have a perfectly good command for manipulating the safety switch, and custom-mode is kind of implied. The other bits are pointless if you are specifying a custom mode.
There's already facility in the message for a submode - but there isn't actually one in-scope in the method I modified when adding that support! Nor is there facility in the old SET_MODE message to convey a submode.
I thought PX4 Autopilot was moving away from custom-submode.... I did not know this, although I am aware of the addition of ROS2 support to allow more customisation with regard to modes.
... I just checked, and I can't see PX4-AutoPilot actually consuming MAV_CMD_DO_SET_MODE via mavlink; they seem to synthesize it for internal use somehow. Further, the handling of the old SET_MODE message split the 32-bit custom_mode field into several parts including a mode and a submode - which means that when support is added for MAV_CMD_DO_SET_MODE there's going to be some terminology conflicts...
You may look at the recent "common mode" work that people have been working on, seeking to provide a list of modes which have roughly the same characteristics across different autopilots. I confess to not being particularly familiar with that work yet.
Generally for offboard support it's "go to offboard mode ("GUIDED" in ArduPilot parlance), handle relevant positition/attitude-target commands".