GCS_MAVLink: Remove wrong usage of COMMAND_ACK message

[patrickelectric]

SET_MODE message does not exist inside the MAV_CMD enum
as described in the mavlink specification for COMMAND_ACK.
The system that is using SET_MODE to communicate with the
vehicle should rely on HEARTBEAT message to detect if
the mode was set correctly.

Signed-off-by: Patrick José Pereira [email protected]

[peterbarker]

I preserved the current insane behaviour in a refactor - thus that comment.

Have you checked apmplanner2 it may special-case this - I don't know? I know that it interprets command-acks.

I do know this will be a significant behaviour change for MAVProxy users as they won't get the feedback on success/failure they currently do - unless they have the console open. dronekit-python users likewise - it may break scripts.

I opened ArduPilot/pymavlink#411 several weeks ago as a move towards what you're doing in this PR :-)

Better by far to actually use the actual MAV_CMD_DO_SET_MODE - you get a legitimate ACK from that, and it can provide you a hint as to what's gone wrong and how permanent a thing it is. I believe APMplanner2 will actually interpret that CMD_ACK, too. That's no an infallible system; I really wish there was an opaque identifier in these commands.

[patrickelectric]

Have you checked apmplanner2 it may special-case this - I don't know? I know that it interprets command-acks.

I have only checked with QGC team, I did a small discussion here about the usage of the deprecated command SET_MODE and move to the non deprecated alternative MAV_CMD_DO_SET_MODE.

If someone is using ACK_COMMAND to acknowledge SET_MODE, they are breaking the mavlink specification.
If ArduPilot continues to acknowledge SET_MODE, it enforces the break of the specification and the wrong usage of it, resulting in software that can not work with vehicles or any other system that follows the mavlink specification.

[magicrub]

This effects GCSs. Has this been tested on MissionPlanner or QGC? @DonLakeFlyer @meee1

[rmackay9]

We should test this against MP before merging.

[meee1]

MP has never used this ack.

[magicrub]

What about generating an immediate heartbeat on successful mode change?

[DonLakeFlyer]

QGC doesn't use it

[peterbarker]

On Sun, 7 Jun 2020, Tom Pittenger wrote: What about generating an immediate heartbeat on successful mode change?

ArduPilot does that.

[tridge]

for GCS that have been correctly updated to use the command this makes no difference
for GCS that have not been updated they need the ack
Note that pymavlink was only changed to avoid use of set_mode() quite recently
MissionPlanner still uses set_mode() and should change to use the command
qgroundcontrol still uses set_mode()
Until the GCS implementations all use the command instead of set_mode the ack/nack in the tlog is useful, even if the GCS doesn't display the ack/nack

[peterbarker]

It sounds like this old, bad send-ack code is going to be with us for a very long time.

@patrickelectric did you want to keep this PR open for the next 3 years until everyone's moved away from the message to the command, or shall we close this and redo it in the future?

[peterbarker]

Nudge @patrickelectric

[patrickelectric]

We could close it, no problem.
I'll add an event in my calendar to update this PR in 3 years from now 😆

[patrickelectric]

Less than 1 year to follow the MAVLink standard correctly.

[patrickelectric]

After 4 years, @peterbarker I believe that is time to merge it.

[patrickelectric]

Thanks everybody!