Long running commands - receiving same command is request for progress update

[hamishwillee]

This came up here: mavlink/qgroundcontrol#9904 (comment)

Essentially QGC is polling for progress updates rather than waiting for them from AutoTune. That's a reasonable approach, and it makes sense that the protocol should act in this way - so this explicitly specifies the implied behaviour.

Note, it is a bit scary, in that if you poll like this, you could kick off another autotune if you do it too late.

[peterbarker]

The fact the same command can kick off the freshly-cancelled command is a real concern, particularly for multi-link vehicles with slower links. I don't think the response should be IN_PROGRESS at all - I think BUSY or whatever would be a better choice and perhaps allow polling the progress in another manner - MAV_CMD_POLL_PROGRESS?

[hamishwillee]

@julianoes What do you think - see #405 (review).

I think it is a real concern. Note that polling wasn't even considered by us - I added this because it is what QGC is doing with PX4 autotune and to me it made sense. They are also using the progress field and disarmed state to find when the tuning is complete rather than any final ACK.

Peter and I also talked about this:

If another command is received while handling a command (long running or otherwise) the new command should be rejected with MAV_RESULT_TEMPORARILY_REJECTED. What this means is that to restart an operation (i.e. with new parameters) it must first be cancelled.

He see's this as overly restrictive - why can't a flight stack handle multiple commands if it wants? My first reaction was sure - if a GCS can't handle multiple responses it should send multiple requests, and if a flight stack can't handle multiple requests it can still choose to reject the request with MAV_RESULT_TEMPORARILY_REJECTED.

My main concern is that we haven't thought enough about slow links and what could happen if multiple commands are floating around.

@peterbarker there is another possible option we "slightly explored" in mavlink/mavlink#1635 . This essentially replaces the idea of long running commands with a completely separate mechanism for streaming command progress. The idea being that for something like AUTOTUNE you'd do what you have always done and complete the request immediately.

But a system that understood the COMMAND_PROGRESS it could output where you're at, and you could poll for this as well. A bit like the events interface but focused on the use case.

There was more discussion to be had, and we got distracted. But there is some merit in theory to this, in particular that it makes it easier to update everything to use the new approach.

[julianoes]

My intuition is that if you send the command again when it is already running you get a BUSY result. The progress should be emitted, not polled for, in my opinion. That's how it was designed and ought to be implemented.

If there is a good argument for poling, then I'm ok with a change but the fact that it is implemented incorrectly is not a reason to change the spec, in my mind.

[hamishwillee]

@julianoes @peterbarker Agreed, so I am going to close this PR.

@julianoes Generally, what do you think about Peter's concern in #405 (comment) about only allowing one long running command at a time.

Because the acks include command ids it feels like they can handle multiple commands at a time, albeit of different types, and the only limit would be whether the implementation allows it. So I think Peter is right and this is unnecessary.

I don't think we need a mechanism for polling, but that would require a use case.

For the AUTOTUNE thingy I think we should push for correct implementation of long running protocol. We should also try to get a new command in for this - the old one is not great/makes assumptions, so if we use it we are breaking things that have build a different implementation on those assumptions. The only way to avoid that would be for ArduPilot to switch to a long running command too (which is not reasonable to ask)

[julianoes]

about only allowing one long running command at a time.

I would think only one at a time is possible, yes.

[hamishwillee]

@julianoes

about only allowing one long running command at a time.

I would think only one at a time is possible, yes.

Why? i.e. are you thinking about the protocol or a specific implementation of the flight stack. I "think" the protocol can handle multiple long running commands.

I am not sure PX4 can with the design of the command loop, but it can still respond sensibly. That's OK as long as we say that some flight stacks may not be able to handle multiple outstanding requests.

[julianoes]

Ok, multiple different long running commands would be possible from a protocol view. If this is implemented is another question, you're right. What I meant is that multiple "same" commands are not possible, and wouldn't make much sense.