Added mission checksum message

[holmnikolaj]

fixes:
#1171

[hamishwillee]

@DonLakeFlyer @julianoes Don't know if you have been following discussion, but this is a mission item that includes a checksum to make it easier/faster to determine whether mission on vehicle matches GCS/API. See #1171 for broader discussion.

[julianoes]

I would probably prefer a separate MISSION_... message that fits with the rest of the mission protocol instead of a command.

[DonLakeFlyer]

I would just call this thing a unique id for the mission. If the GCS is in control of generating it and there is no spec as to how to do that then it can be done using whatever means is appropriate. It could be a database key or something like that in some cases. The only responsibility of the vehicle is to store all 7 params and send then back to the gcs on download.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[IamPete1]

Any update on this? It would solve the problem I was trying to solve with #1527 if it was to also include a enum for mission type, ie mission, fence, rally ect. Seems like that would be a simple addition and then this would be a useful feature, not sure why this has not been merged already?

[holmnikolaj]

It got stale because the solution we agreed upon was based on the, at the time, not yet implemented mavlink microservices protocol. I don't know the status of it but @julianoes was tasked to implement it.

[julianoes]

@holmnikolaj right. And then @ItsTimmy worked on the implementation and had a first proof of concept ready to review and test:
PX4/PX4-Autopilot#13565
mavlink/qgroundcontrol#8065

Unfortunately, it seemed like no one cared to move it forward from there and it was abandoned and eventually closed 😞.

[hamishwillee]

@IamPete1 @holmnikolaj Let's do an iteration and get this in. It has been a while, so let's summarise the thinking and confirm we're happy.

The proposal as I recall (please confirm):

• Mission includes a new MAV_CMD for the id.
• This gets set by GCS on mission upload. The method used to determine the UID value is entirely up to the GCS : TBD
• GCS can query current UID value using generic requestor method (UID emitted in a new message)
• There is a bit to indicate support for this feature.

Notes/Discussion:

• Should the MISSION_UID should also be emitted by the flight stack when the mission changes (for any reason). This gives interested systems the chance to know that a mission has changed sooner.
• The UID needs to change every time the mission changes - including partial updates. How do we expect this to be handled?
  • One suggestion is that if a system supports this mechanism and gets a mission without a UID, then internally the UID should be set by the to a UID that means "invalid" (by convention). Similarly if there was a partial update.
• The GCS can set whatever UID it likes. Perhaps we should make it clear that this shouldn't just be an iterated value - otherwise two systems are likely to clash.
• How should this work with fences and rally points? i.e. is the ID for the whole "plan" or should we define separate UIDs for all (my preference).

Originally we discussed having the UID as a hash, calculated by the ground station and not done as a mission item at all. Flight stack generates mission hash on every mission change and emits a new mission UID as part of the MISSION_ACK (also emits on request). Can anyone remember why we decided against?

[hamishwillee]

Further, under new governance approach, we're trying to only include PRs where there is going to be an actual flight stack implementation and commitment to get into another stack. So if we do this, is anyone committed to getting this into PX4/ArduPilot?

I'll add this to tonights DevCall. Come along if you wish to discuss it.

[LorenzMeier]

For effective implementation: I would propose to hash these elements and to apply the same logic as on the generator:

• frame
• command
• param1
• param2
• param3
• param4
• param5
• param6
• param7

[holmnikolaj]

@hamishwillee That summarizes it pretty well. As I recall we didn't agree on how to handle partial upload, but agreed that the proposed solution would not make it more difficult to support it in the future.

I will not be committed to the issue as we (Sky-Watch) opted for another solution we could implement right away.

[IamPete1]

I agree with Lorenz, it should be a hash of the whole command, and it should be possible to re-hash on the flight controller, then partial upload and scripting in AP will also work.

I think it should definitely be extended for fence and rally points.

From the AP side I can take on the implementation.

[hamishwillee]

Discussed in the devcall. We'd like this to use a hash for the id; which implies that the best place for this to be generated is the flight stack.

Proposal is then:

• Flight stack generates separate hash on any change to the mission, fence, or rally points.
  • Things to be part of that hash are (for all items in the mission, fence or rally point list) the frame, command and parameter values as per Added mission checksum message #1172 (comment)
• The generated hash is broadcast in a MISSION_UID following completion of an update to a on-vehicle mission - ie just after the ACK.
• The generated hash may also be emitted in response to MAV_CMD_REQUEST_MESSAGE
• Note that following this change no explicit mission item added to mission.

The a MISSION_UID would look something like (TBD):

  <message id="53" name="MISSION_UID">
    <description>Unique hash for the current mission, fence, or rally points definition. This is emitted by the flight stack after any change to a mission, fence, or rally point definition, typically after the` MISSION_ACK` is sent, and on request using MAV_CMD_REQUEST_MESSAGE.</description>
    <field type="uint8_t" name="plan_type" enum="MAV_MISSION_TYPE">Plan type for which the hash applies.</field>
    <field type="uint32_t" name="uid">Unique hash of items in specified plan_type.</field>
  </message>

Points for discussion/questions:

• Is this reasonable?
• Above I have assumed that hash result is uint32_t - is that reasonable - or should it be a 64 bit number?
• Is there a better name for this - ie would MISSION_HASH, or PLAN_HASH be better?
• What is hashing function should be used? SHA-1, MD5 etc etc? If we use one with greater hash length, how do we truncate to fit message?
• Should we also include the hash in the MISSION_ACK message from drone following upload? I kind of like this because for the GCS it confirms that the hash is for the plan it uploaded, whereas the UID emitted after the upload feels "separate". Thoughts?
  • Note, I say "also" because we'd still emit MISSION_UID so that other systems can be aware of it and query it when they want.

[holmnikolaj]

[hamishwillee]

@holmnikolaj Thanks for that.
The error shown is

./message_definitions/v1.0/common.xml needs formatting - run ./scripts/format_xml.sh ./message_definitions/v1.0/common.xml

Basically this means what it says. Can you please rebase then run ./scripts/format_xml.sh ./message_definitions/v1.0/common.xml (I do this on ubuntu). It is basically a linter for whitespace and other minor issues.

If this is a problem for you let me know and I'll push a PR to your branch with the required updates updates. Note though that probably won't be for a couple of weeks because I am out of office.

@amilcarlucas @IamPete1 Can you please do final sanity check of PR. I am going to push it in once there is a build that passes tests. Pete, note also that when you implement this would appreciate you letting me know if you run into any ambiguity at point of implementation.

[peterbarker]

Onboard scripted changes to missions (like @davidbuzz 's ) might lead to a lot of checksumming and network traffic.

[hamishwillee]

Onboard scripted changes to missions (like @davidbuzz 's ) might lead to a lot of checksumming and network traffic.

@peterbarker Yes. What do you suggest?

EDIT Perhaps we keep the protocol bit and a system can turn on and off the feature using a parameter if it doesn't choose to support it?

[holmnikolaj]

@hamishwillee It seems to pass now, and the script didn't make changes - perhaps it was the protocol bit part?

[davidbuzz]

I'd just drop a note here that i have operationally used AUTO and a dynamic mission (such as lua) that is changed repeatedly (a companion computer can also be used to repeatedly push a trivial 'next target' mission, i've done that pre-lua), as often as 1hz. The main value for us was being in AUTO was the pre-canned fully auto takeoff, fully auto landing, and the fact that when an aircraft gets to the end of its auto/mission it "rtls" without operator interaction, which for us brought it back, totally hands-off, still in auto, if for any reason the companion-computer (or lua) failed to keep the dynamic mission up-to-date. so although i suppose a lua script that changes it to guided-does-stuff-then-changes-back is probably very similar, for us, it was the 'fall off the end of the mission = rtl' feature that was an important failsafe for when either the CC or lua updating the mission stops for any reason.

[dagar]

Wouldn't this be helpful as extension fields to existing mission messages (MISSION_CURRENT, MISSION_COUNT, etc)?

http://mavlink.io/en/messages/common.html#MISSION_CURRENT
http://mavlink.io/en/messages/common.html#MISSION_COUNT

[hamishwillee]

Wouldn't this be helpful as extension fields to existing mission messages (MISSION_CURRENT, MISSION_COUNT, etc)?
MISSION_CURRENT, MISSION_COUNT

@dagar I was thinking is that it might be useful in MISSION_ACK. This would ensure that on completion of an upload the GCS gets the precise value of the hash that the drone has for this upload and doesn't have to calculate the hash if it does not want. The drone would still need to emit the hash separately though because no other systems will be looking at this. Thoughts?

For other messages

• If it is included in additional messages this is an increase in network consumption. What's the additional benefit and is it worth that cost (for each case)?
• MISSION_CURRENT is emitted every time the current mission number changes. I don't particularly see the benefit of this over GCS requesting when needed or getting a notification when it changes. I guess just that it gets emitted regularly but you'd still need the other emit cases.
• MISSION_COUNT is sent to kick off upload of mission to drone, and to tell GCS how many items will be sent in download from drone. Current thinking is that the drone hash is "canonical" so in the download from drone case it is fine, but during upload what should drone do with this info?

[hamishwillee]

Thanks @davidbuzz - that's what we needed to know. I completely "get" why you would use this approach; most of this can be done in other ways, but the end of script failsafe is pretty convenient.

@peterbarker So on this basis you're looking at an extra message being emitted at 1Hz. Perfectly acceptable in and of itself - but it might not end there; in theory every GCS on the network could then decide it needs to update the onboard mission. I guess we could provide guidance that a GCS should mark current mission as invalid, but only update when explicitly requested or after a timeout without changed hash messages?

Otherwise I'm open to proposals/suggestions for improvement but do not consider this "blocking".

PS If you want to chat about this offline I'm around. Ping me.

[peterbarker]

On Mon, 28 Dec 2020, Hamish Willee wrote: @peterbarker So on this basis you're looking at an extra message being emitted at 1Hz. Perfectly acceptable in and of itself - but it might not end there; in theory every GCS on the network could then decide it needs to update the onboard mission. I guess we could provide guidance that a GCS should mark current mission as invalid, but only update when explicitly requested or after a timeout without changed hash messages?

Probably removing the requirement that all updates to the mission be notified and leave it to the autopilot to determine when the hash should be sent would be a good idea.

[hamishwillee]

Probably removing the requirement that all updates to the mission be notified and leave it to the autopilot to determine when the hash should be sent would be a good idea.

Not sure. If a GCS doesn't know whether or not it will be pinged with updates by an autopilot presumably it will need to poll? That isn't good either.

Why not have autopilots have a parameter to enable/disable support entirely. That way anyone can check whether it is currently supported by an autopilot? Downside of course is that you're saying you can't have both hash and a companion doing regular updates. Maybe two bits "supports hash" and "supports update on change". Dunno

In theory we might also have a command from GCS to tell drone update on all changes or update when requested. But that is also problematic because the GCS doesn't know what the autopilot "needs".

[IamPete1]

I'm still planning on doing a AP implementation, have not had a chance to get to it yet tho. (@auturgy )

[hamishwillee]

This was discussed in the Devcall. Current policy is that we don't merge into common without a working implementation and at least intent to have in another implementation.

That said, the current design is considered acceptable, so you can take it and move forward (i.e. we will iterate based on experience with the prototype). OK by you @IamPete1 and @holmnikolaj ?

Note For example, if the requirement to send out hash on every update does swamp links, then we might make the requirement for auto emission "optional" (I'm kind of OK with that because even though it is desirable, the fact is any system could miss this update so no system can absolutely rely on it).

[holmnikolaj]

This was discussed in the Devcall. Current policy is that we don't merge into common without a working implementation and at least intent to have in another implementation.

That said, the current design is considered acceptable, so you can take it and move forward (i.e. we will iterate based on experience with the prototype). OK by you @IamPete1 and @holmnikolaj ?

Sure!

[julianoes]

@hamishwillee do you remember why this stalled? @dagar brought it up in todays QGC call.

When I look at the PR I would say that we merge it and try implementing it? Anyone against that?

[peterbarker]

@hamishwillee do you remember why this stalled? @dagar brought it up in todays QGC call.

When I look at the PR I would say that we merge it and try implementing it? Anyone against that?

common.xml shouldn't be a sandbox.

As this spec is written, the checksum ArduPilot returns won't be the same over time, even with no GCS interaction - at least while we're disarmed. ArduPilot considers its home location to be part of the mission, and returns it as mission item 0. We drift the home location until the vehicle is armed (exact behaviour varies by supported AP vehicle type).

item-0 could be excluded from the checksum by AP - but then a GCS will need to know to exclude it from its own checksum (and my nefarious plan to use a capability bit to indicate mission-slot-0 handling is still only a plan).

[julianoes]

common.xml shouldn't be a sandbox.

Right, I get that but I doubt we have done the plumbing work (aka px4.xml, or development.xml) to enable that sort of sandbox, right? In that case we have a chicken and egg problem where we're told we have to implement it first but at the same time, there is no way to do so. (One could of course generate the headers locally to start implementing it but it would not work once you try to push it and make a PR to actually proof that there is an implementation.)

Note that I'm out of the loop of the governance discussion, so I might be missing something.

item-0 could be excluded from the checksum by AP - but then a GCS will need to know to exclude it from its own checksum (and my nefarious plan to use a capability bit to indicate mission-slot-0 handling is still only a plan).

Ok, so that's the other thing that needs to be resolved first...

[hamishwillee]

@peterbarker @julianoes

This is waiting on an implementation before merging. Yes this is a pain for developers, and I hope we can move to having a development.xml soon that can be used instead to ease things going forward. @IamPete1 can you give us an update on your progress?

The issue with home is already addressed. The algorithm for calculating the has excludes the home location. The GCS knows that, because they looked at the documentation in the message in this PR :-)

PS We could in theory merge it because we have commitment from two flight stacks to create an implementation. But we're trying to do the right thing and at least have one good prototype.

[julianoes]

This is waiting on an implementation before merging.

I don't think we should change and enforce the process without having the tools in place.

[hamishwillee]

This is waiting on an implementation before merging.

I don't think we should change and enforce the process without having the tools in place.

I have no strong opinion, other than the last time something like this came up @LorenzMeier suggested that it isn't all that hard to branch mavlink for your changes while developing, which I took to mean we should modify the process. Certainly very strong push not to merge anything without a prototype.

[julianoes]

@hamishwillee that's what I wrote further above:

(One could of course generate the headers locally to start implementing it but it would not work once you try to push it and make a PR to actually proof that there is an implementation.)

[IamPete1]

@hamishwillee Still not got to this yet, but I have not forgotten about it.

[hamishwillee]

@julianoes Then let's discuss that in the dev call - added to agenda

[hamishwillee]

Superseded by #1611

@IamPete1 @holmnikolaj Once that goes in, implementations should be able to use the development.xml rather than common.xml (or ideally standard.xml) while prototyping.