MAV_RESULT: Clarify exactly what the possible results mean

[hamishwillee]

This clarifies the meaning of possible results (MAV_RESULT) of a command (in a COMMAND_ACK) to remove ambiguity on which should be used in what circumstance.

[hamishwillee]

These are my take (very slight variation) on suggestions from @olliw42.

@auturgy @amilcarlucas @julianoes Would appreciate your review.

Does anyone know the intention for MAV_RESULT_IN_PROGRESS, which is marked as WIP? Obviously it means "command is supported and execution has started but is not finished. But when would you actually use it?

The only real use I see is a more explicit error value than MAV_RESULT_TEMPORARILY_REJECTED. Ie MAV_RESULT_TEMPORARILY_REJECTED indicates the state machine is busy, while MAV_RESULT_IN_PROGRESS indicates that it is busy on this particular command. I guess you'd use it on something like a long running file transfer so you could ping for completion. Any thoughts? Anyone actually used it?

[olliw42]

great.

pl note that the distinction between MAV_RESULT_TEMPORARILY_REJECTED and MAV_RESULT_FAILED is not really "exactly" clarified

there are currently in fact these three results, MAV_RESULT_TEMPORARILY_REJECTED , MAV_RESULT_FAILED , and MAV_RESULT_IN_PROGRESS, which are very very similar, since they all three indicate "command is supported" and "Retrying later may work".

I would see these distinction:

MAV_RESULT_TEMPORARILY_REJECTED: the implementation can't currently process it, but please retry

MAV_RESULT_FAILED: the implementation can't process it because of some nearly fatal error. Retrying later might work, but most likely won't (since fatal reason most likely won't have been resolved)

MAV_RESULT_IN_PROGRESS: the implementation is already processing the very same command and is in the course of achieve it's goal, so it actually doesn't make sense to retry, please don't retry.

so a long running file upload would IMHO lead to a MAV_RESULT_TEMPORARILY_REJECTED not MAV_RESULT_IN_PROGRESS (since a 2nd command would try to upload a next chunk, not the same chunk).
An example of a MAV_RESULT_IN_PROGRESS would be a command there a 2nd incoming command would not lead to a different consequence and essentially would be dropped. E.g. retracting a gimbal or some landing gear or anything which takes longer to process. It would be triggered by a first command but if a 2nd command would come in while it's retracting the 2nd command would essentially be ignored since it's already retracting and the intended consequence of this 2nd command will be achieved.

IMHO

[hamishwillee]

The distinction is that for MAV_RESULT_TEMPORARILY_REJECTED we won't try to execute it right now while for MAV_RESULT_FAILED we thought we could execute it but it failed for some unexpected reasons. You're right that the thing to capture is that there is no point trying to retry the MAV_RESULT_FAILED unless it is fixed. I think that is covered.

Currently I believe that MAV_RESULT_TEMPORARILY_REJECTED is used inconsistently. For example I know that it is returned as a pre-arm failure (ie not ready to arm) on PX4. But arguably you might also return it if sending a takeoff when already flying - but in this case the command is ignored (ACK is MAV_RESULT_ACCEPTED).

While that is a sensible definition of MAV_RESULT_IN_PROGRESS:

• it is WIP with no documentation, so we don't know the intention (we're guessing)
• Right now instead cases where you might use this return MAV_RESULT_ACCEPTED - e.g. takeoff just ignores the command. I don't see much real benefit in using MAV_RESULT_IN_PROGRESS

[olliw42]

if it is WIP with no documentation so we don't know the intention and are guessing, then there shouldn't be some half-cooked documentation there making us guessing and if we may want to use it ... that is, I then would strongly pledge to just write WIP and nothing else !

I agree that IN_RESULT is not really needed and I guess world will continue to spin without it

I'm btw convinced that you will find several places and cases there the ack wasn't used as now outlined ... a consistent interpretation of the result was just missing .... so, there will be "consequences", but IMHO this just demonstrates why a spec should want to be a spec and a spec must be a spec

[julianoes]

Currently I believe that MAV_RESULT_TEMPORARILY_REJECTED is used inconsistently. For example I know that it is returned as a pre-arm failure (ie not ready to arm) on PX4.

Exactly. To me the question is what we should be using for an arming failure because of the fact that e.g. we don't have GPS signal. Should it be MAV_RESULT_TEMPORARILY_REJECTED because we might have better GPS signal later, or MAV_RESULT_DENIED because given this environment it's not going to happen. Or asked differently, how "static" does something need to be to response with MAV_RESULT_DENIED?

MAV_RESULT_IN_PROGRESS Any thoughts? Anyone actually used it?

I have added it and used it, so I have some thoughts:
This result is needed for commands with actions that take "longer" to complete. And one might not know if the completion is going to be successful or not.
Answering a command immediately with MAV_RESULT_IN_PROGRESS has two purposes:

1. Prevent retransmissions of a command due to a timeout.
   E.g. if a camera knows that switching from video to photo mode takes 1-3 seconds, it can immediately answer the command and the ground station can stop worrying if the command has arrived, and therefore won't send a retry after typically 0.5 seconds.
2. The COMMAND_ACK message has a field progress to indicate progress to a user.
   E.g. if a camera knows that switching from video to photo mode takes 1-3 seconds it would set it to NaN because it doesn't really know which can be displayed as a spinning wheel. For other operations which take a time that can roughly be estimated, multiple COMMAND_ACK messages can be sent indicating the progress every time. This allows the ground station to show a progress bar.

After the long running action has finally completed a COMMAND_ACK with the end result is still required, so typically MAV_RESULT_ACCEPTED or MAV_RESULT_FAILED.

[olliw42]

I think the conflict in case of the arming comes from a gross misuse of the concept

IMHO it cannot be the purpose of the ack to report back command specific states, it only - and therefore IMHO should only - report back command generic states, i.e. info about the fate of the command but not about it's outcome

the correct handling should be as it is done in so many other cases, namely the command should be acked PLUS trigger some sort of feedback message which carries the specific info
e.g. MAV_CMD_REQUEST_PROTOCOL_VERSION -> COMMAND_ACK + PROTOCOL_VERSION, and so on

[olliw42]

so the distinction between MAV_RESULT_TEMPORARILY_REJECTED and MAV_RESULT_IN_PROGRESS is that the sender in the first case needs to decide itself if it wants to take action and retry by actively sending a new command, while in the 2nd case the sender can expect to see more ACKs coming in, reporting the progress, but without having to actively ask for the progress by sending new commands. Right?

This makes sense IMHO.

While the descriptions have improved they IMHO still don't sufficiently accurate make clear the course of events in the different cases.

[hamishwillee]

Thanks guys. I've updated the MAV_RESULT_IN_PROGRESS case for English.

Note, the reason I found MAV_RESULT_IN_PROGRESS confusing is that the sequences show only a single ACK - https://mavlink.io/en/services/command.html. So there an update is required for this case. Of course it makes sense, but it wasn't clear that it is "supported".

• Update command protocol to show in-progress case

Exactly. To me the question is what we should be using for an arming failure because of the fact that e.g. we don't have GPS signal. Should it be MAV_RESULT_TEMPORARILY_REJECTED because we might have better GPS signal later, or MAV_RESULT_DENIED because given this environment it's not going to happen. Or asked differently, how "static" does something need to be to response with MAV_RESULT_DENIED?

DENIED means "command supported, invalid params", I think you mean MAV_RESULT_FAILED.

My take on this is that MAV_RESULT_TEMPORARILY_REJECTED is returned if a system knows in advance that it can't execute is - e.g. state machine is not ready, while you use MAV_RESULT_FAILED for requests that should work considering the known vehicle state - e.g. request to LOG that fails due to broken SD card.

On this basis prearm failure is MAV_RESULT_TEMPORARILY_REJECTED - because both the system making the request and the requested system do know in advance whether a command to arm will succeed.

@olliw42 I am sure you will still find this ambiguous. How would you word them to remove that ambiguity, based on the current wording (i.e. don't start from scratch).

[olliw42]

I like the introduction of the notation "valid/invalid". This makes it clear what the conditions for the various responses are.

I'm not totally happy with the notion of "will work/will not work". I couldn't come up with a better wording, sorry, but let me describe what I mean. E.g. "Retrying later may work": TEMPORARILY_REJECTED is in fact exactly the response for informing the sender that it should retry later! It is not just a "may" and it is not about "next time it will work", but it is "can't do now, retry later". Maybe changing the position of text might do it:

MAV_RESULT_TEMPORARILY_REJECTED: "Command is valid, but cannot be executed at this time. Retry later. This usually indicates that the system is not ready to process the command (i.e. a state machine is busy)."

As regards MAV_RESULT_FAILED: I guess I would try to be somewhat more pointed. Maybe:

MAV_RESULT_FAILED: "Command is valid, but execution failed. The issue must be fixed before retrying the command. This typically indicates an environmental issue such as attempting to write a file when out of memory.

As regards MAV_RESULT_IN_PROGRESS: As much as I see it, the sole justification of this response is to inform the sender that there WILL be a sequence of further acks! If the system knows at time of sending the ack that it will have completed the task on the typical timescale of the MAVLink communication it should send an ACCEPTED. So the minimal possible result is a IN_PROGRESS followed by a ACCEPTED. That is, "may be followed" is inaccurate. So, maybe something like

MAV_RESULT_IN_PROGRESS: "This will be followed by further progress updates, i.e. the component will send further COMMAND_ACK messages with result MAV_RESULT_IN_PROGRESS (at a rate decided by the implementation), and must terminate with sending a COMMAND_ACK message with result MAV_RESULT_ACCEPTED. The progress field can be used to indicate the progress in percent (0 - 100), or should be set to 255 if not known. There is no need for the sender to retry the command, but if done during execution, the component will also return MAV_RESULT_IN_PROGRESS with an update progress."

[julianoes]

MAV_RESULT_IN_PROGRESS: "This will be followed by further progress updates, i.e. the component will send further COMMAND_ACK messages with result MAV_RESULT_IN_PROGRESS (at a rate decided by the implementation), and must terminate with sending a COMMAND_ACK message with result MAV_RESULT_ACCEPTED. The progress field can be used to indicate the progress in percent (0 - 100), or should be set to 255 if not known.

It can also be followed by something else than MAV_RESULT_ACCEPTED because it might still fail even after making progress.

The progress part is documented in the message doc:

... the progress percentage or 255 if unknown the progress when result is MAV_RESULT_IN_PROGRESS

[olliw42]

It can also be followed by something else than MAV_RESULT_ACCEPTED because it might still fail even after making progress.

ah, that's of course right, thx for correcting this ... I guess what I tried to make more clear was that the key point of the IN_PROGRESS is that there WILL be further acks coming, and not just "may" be coming, which distinguishes it from the other acks.

The progress part is documented in the message doc:

yes, I have seen this and that's there I got the values from. It just seemed to me that it would be useful and help understanding the nature of the IN_PROGRESS if this woudl be mentioned here. But technically you are of course right that it's defined there.

[hamishwillee]

Thanks. I've updated in line with feedback. Didn't go to full detail on the .progress field, but mentioned it - as you say, useful. We can go into more detail in command protocol docs.

@julianoes I'm still not happy with the definition of failed vs rejected.
Currently:

• Failed is for command that the systems thinks will work, but you try them and something stops them working.
• Temporary rejection is definitely reasonable when you know that a command can't work right but you expect this to change without you doing anything. So this would occur if you tried to arm when you know that you don't have a GPS fix.

The question is what do you do for the case where you know the message can't succeed for an environmental issue you do know about. For example prearm failing due to sensors not calibrated. In this case the issue isn't going to fix itself. It isn't temporary.

We have some options:

• We use temporary reject for this case - ie we reject all problems we can determine from our telemetry.
• We try use failed indicating the environmental issues that are both known (uncalibrated sensor) and unknown (oom)
• We add an extra state for this kind of rejection

I think "Failed" is correct for this. So

• Failed is for valid command that the system knows cannot execute due to a non-transient issue. or which it believes can execute but due to some unexpected error.
• Temporary rejection is for known issues that are expected to resolve themselves - state machine busy, GPS lock not achieved.

Is that reasonable? Is it implementable? Does that help the user?

[olliw42]

IMHO:

Temporary rejection is for known issues that are expected to resolve themselves with time

that is, it's IMHO the right ack if it is expected that the issue goes away with just waiting (during the wait time of course all sorts of things can happen, like the universe explodes, we can't capture also this, hence "is expected" in here)

anything else, which is a valid message but couldn't be executed is FAILED (i.e. I would define failed as the logical negative to temporarily rejected)

your example of a prearm failing due to sensors not calibrated is IMHO failed iff this will need e.g. some user intervention, but should be temporarily rejected if it is done e.g. by some EKF thing and this thing just needs a bit more time to settle. So, the answer would be implementation specific. But the implementation would know.

I guess I've said this before, for something as state-full as prearm and arm it's IMHO outside of the scope of COMMAND_ACK to provide a detailed account of the source of issue. If that is what is wanted then there should be a complementing (PRE)ARM_ERROR message which gives a detailed error code.

[hamishwillee]

@olliw42 Thanks, that was my take on it too.

I guess I've said this before, for something as state-full as prearm and arm it's IMHO outside of the scope of COMMAND_ACK to provide a detailed account of the source of issue. If that is what is wanted then there should be a complementing (PRE)ARM_ERROR message which gives a detailed error code.

Yes, and you're right. The need for a way to monitor prearm reasons has been discussed many times. Proved difficult to get agreement on what the message should contain. Whether or not we do that, I think we can provide helpful advice here.

[julianoes]

that is, it's IMHO the right ack if it is expected that the issue goes away with just waiting (during the wait time of course all sorts of things can happen, like the universe explodes, we can't capture also this, hence "is expected" in here)

I like that as a definition. So this means that GPS might come back when the universe explodes, right?

[olliw42]

So this means that GPS might come back when the universe explodes, right?

I'm afraid I lack experience in this matter to provide an answer (LOL)

[hamishwillee]

Self merging. I am much happier with these definitions. Even if they are not perfect, they are definitely much more useful than previously, and they are consistent and "followable". FYI @auturgy

Thanks guys for your advice and help.