Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add COMMAND_CANCEL - message to cancel long running commands #1401

Merged
merged 10 commits into from
Jun 25, 2020

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Jun 11, 2020

@julianoes @auturgy This adds message to cancel a long running command, as suggested by @LorenzMeier

The message just takes a target sysid and compid, and the command being cancelled (not strictly needed if concurrency not supported, but a good check to make).

I don't have to implement this, so important you check the logic.

Essentially the sequence continues with progress updates until it completes - with one of acceptance, failure or cancellation. If the cancel message was sent, received and obeyed the flow would look like this.

If no cancellation or other completion ACK is received, the cancel request can be retried

If the operation has completed, the cancel message can be ignored.

Does this make sense?

  • Do we need a capability bit to indicate cancellation is supported? I guess "yes" because we can't assume handlers just check for "not in progress" to decide the sequence is done. Wish we had microservices.

message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
hamishwillee and others added 2 commits June 15, 2020 09:15
Co-authored-by: Julian Oes <julian@oes.ch>
@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Jun 14, 2020

@julianoes Thanks very much. Good point. I have accepted your suggestion as it makes sense - the sequence can only complete once, and with one result.

  1. How do we handle the case where no completion ACK received - ie say either a cancel, accepted or fail was sent but not received. If it hasn't actually completed we're OK because we can resend the cancel message - but more generally we're stuffed. Or to put it another way, what happens following our IN_PROGRESS timeout? We haven't specified this in the protocol?
    The easiest thing would be to perhaps just note "unknown problem" in UI and move back to idle.

  2. Do we use a capability bit to indicate this new option? Microservices versioning or a metadata to indicate this message supported would be better. I kind of think that we could ignore this for now - if the command isn't supported by a system nothing should change for the recipient. The sender of course will just keep resending the cancel and it will be ignored.

@hamishwillee
Copy link
Collaborator Author

@julianoes When you get a chance - new related question above: #1401 (comment)

Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's good.

@hamishwillee
Copy link
Collaborator Author

@julianoes I can merge this, but I'd quite like to resolve what we do if the final Cancel or Fail or Accepted ACK is missed?

If not, then can merge this and defer to the dev call.

@julianoes
Copy link
Collaborator

@hamishwillee sorry I missed that:

  1. I would say that's a timeout. In the same way that no answer after sending a command is a timeout.
  2. Yes, I think it's fine if cancel is ignored. There are not many (if any) commands yet that are long running and would need this.

@hamishwillee
Copy link
Collaborator Author

@julianoes

I would say that's a timeout. In the same way that no answer after sending a command is a timeout.

The difference is, if there is a timeout after sending a command you can re-request it.

So to be very clear, you're saying that if a component misses the final ACK (for any reason) it will timeout and return to idle (presumably notifying the user that the operation timed out).

The only other way I can see doing this is another message to request a progress update - the system then returns either the current progress or the last sent ACK.

Lets chat in the call.

@hamishwillee
Copy link
Collaborator Author

Merging the cancel message as WIP. Associated docs in mavlink/mavlink-devguide#243

@hamishwillee hamishwillee merged commit f693337 into master Jun 25, 2020
@hamishwillee hamishwillee deleted the hamishwillee-COMMAND_CANCEL branch June 25, 2020 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants