-
Notifications
You must be signed in to change notification settings - Fork 136
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
MissionProtocol: Partial upload/download #157
Conversation
@@ -115,6 +115,9 @@ This section explains the main operations defined by the protocol. | |||
|
|||
The diagram below shows the communication sequence to upload a mission to a drone (assuming all operations succeed). | |||
|
|||
> **Note** Mission update must be robust! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, this is PX4 but not ArduPilot implementation for full mission upload. I have assumed the same for partial update. Will clarify for ArduPilot in the implemetation section.
ArduPilot doesn't implement that message at all that I can find. If you think about it the GCS should be able to just request whatever it wants (ArduPilot is fine with this anyways) in which case this isn't needed. If we ever had proper transactions exposed over MAVLink then this message would have utility, but till then I think it's mostly a noop/not worth implementing anywhere.
ArduPilot won't do this as missions aren't transactional there (nor is there a proper MAVLink way to tell if they are/aren't/who is coordinating a transaction). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The download seems confusing because functionally I can't tell what the aircraft is supposed to do with the start/end messages.
ab25c8f
to
15a1a2e
Compare
@LorenzMeier @WickedShell Can we change the implied sequence for partial mission download which is documented here - which is used by neither ArduPilot or PX4? What I'd like is that the GCS can query for any mission item if it is not in another operation (e.g. idle), and get either the item or an ACK with an error. My reasoning is that
Note, this PR is nearly ready for re-review following integration of feedback from @WickedShell - but I would like to resolve this issue first. |
Protocol implementations must also support [MISSION_ITEM](../messages/common.md#MISSION_ITEM) and [MISSION_REQUEST](../messages/common.md#MISSION_REQUEST) in the same way. | ||
|
||
|
||
### Canceling Operations {#cancel} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LorenzMeier @WickedShell This is a new section. It is irrelevant to ArduPilot, which does not allow cancellation or treat errors as unrecoverable.
There will also need to be fixes to PX4/QGC to allow mission cancellation during uploading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs review. Specifically, the assumption is that a mission is cancelled by responding to an ACK with MAV_MISSION_OPERATION_CANCELLED - and that is reasonable. But
- you could argue that a separate message is required for this to enable cancelling at any point - not just after receiving a message.
- If the ack with cancellation does not arrive then the request will be re-sent. The cancelling system will now be in idle getting an out of sequence message, so will respond with a different error. I think that is OK/good response - ie no need to cache the ack until you're sure the other system got it.
MissionProtocol:Undo Ardupilot implemetnation section Mission:Fix timeout direction for partial upload Mission:Fix timeouts and sequence errors in docs Mission-further updates further fixups typos
8bb7c99
to
4eba9b0
Compare
@LorenzMeier This is updated as we discussed so should now be ready to merge - you OK with that? Specifically it now states that partial updates must be robust in the same way as full updates are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly an improvement over the previous spec, merging.
Thanks @LorenzMeier . This was definitely a doc improvement. FYI It was not merged because it had info about partial mission upload/download that was unresolved when I wrote this. That since has been agree in our discussion to not be part of the current service specification. I removed those bits of this in #231 |
This updates the sections for partial upload/download of missions. PX4 does not implement this so what I have done is inferred sensible operation from the normal upload/download and reviewed ArduPilot implementation.
Would be good if I can get some confirmation that the proposal is sensible even though it might not perfectly match what ArduPilot does @WickedShell, @auturgy . Rendered version is here: https://hamishwillee.gitbooks.io/ham_mavdevguide/content/v/mission_ap/en/services/mission.html#upload_partial
What I have said is
MISSION_WRITE_PARTIAL_LIST
instead of aMISSION_COUNT