Common - Opaque IDs for detecting plan changes

[hamishwillee]

This PR proposes a mechanism (for discussion) for determining whether part of an on-vehicle plan type (mission, fence, rally points) have changed, so that a GCS can upload the changed plan, or more importantly choose not to upload a plan that it already has.

This replaces the MISSION_CHECKSUM which is being removed in #2010.

Overview

Plan upload/download can consume quite a lot of bandwidth, especially for large missions.
Currently ground stations upload the vehicle plan every time they connect, even if they already have a matching plan. Further, if a second ground station or companion updates the plan on a vehicle, there is no reliable way for other ground stations to know that they have to update.

The ideal solution for this problem is for vehicles and ground stations to have a common way of calculating a representation of the plan (such as a checksum) and for the vehicle to publish its current representation. That way the ground station could always know if it has the current plan. Unfortunately the shared-representation approach is flawed because the vehicle will commonly not store a precise match to the GCS version - parameters that are invalid on the vehicle might not be stored, and other values might be stored as some kind of rounded value.

We're therefore limited to solutions where the ID representing a mission must be shared between a vehicle and GCS on upload and then published for all other vehicles. This allows a GCS that has uploaded a mission to know that it does not have to re-update, and for other GCS to know that they do.

This proposal does not require it to be a globally unique UUID, though that might be handy for systems that want to manage missions.

In theory this ID could just iterate for more than the likely number of missions uploaded between vehicle reboots (or several reboots?). It might be a checksum or UUID.

The id might be set by the Vehicle or a GCS. This PR currently defines a proposal for the first case, but both options are discussed here.

Vehicle supplies ID on upload (see PR)

Proposed solution :

• During upload of a mission, fence, or rally point to the vehicle the vehicle calculate and returns an opaque_id to the GCS in the final MISSION_ACK (the value is set to 0 on download to GCS). This is the current id for the particular plan type.
• Vehicle publishes the opaque id for the mission, fence and rally points in the MISSON_CURRENT
• During download from vehicle the opaque id is set in the MISSION_COUNT. Note that a round-tripped downloaded mission might not look the same as the original mission that was uploaded from a GCS, but it has the same on-vehicle characteristic.
• The id for each plan type is a unit32.

With this

• A GCS uploading a plan will have the matching value in MISSION_CURRENT - so no reupload
• Any other GCS will have a mismatch following upload of a new mission. After reuploading the mismatched plan-part it will match.
• An ArduPilot FTP based plan upload could work with this by setting the unique ID to the FTP file checksum. If they are all in the same file, they could all be set to the same value.

Open questions/concerns

• Should we just suggest the opaque ids iterate?

  • The only requirement is that a GCS seeing the emitted number knows that it doesn't match the current value.
  • So we might use a uInt_8 for this and iterate on each change.
  • The only problem case would be if a GCS had a mission and disconnected, then the vehicle had 256 ish new missions and end up with the same value. In this case the GCS would choose not to re-upload.
  • But you might well argue that uint_16 was sufficient.
  • In the case of an ArduPilot FTP this would still work - the FTP would have to trigger an iteration.
  • What is the argument "against".

• Do we need separate ids for each plan part?

  • The "for" argument is that if a rally point changes you can upload that separately without having to upload a mission.
  • I guess the "against" is that it means 3 uint_32 added to the MISSION_CURRENT rather than one.
  • Is it worth it? I think so - a mission might be hundreds of items.
  • And these ids could be smaller if we just iterate the number.

• Is uint32 the right size?

  • if we are allowing CRCs or various UID calculations, what's the best data size for this.

GCS supplies ID on upload.

An alternative is that

• GCS supplies the id of the current mission plan on upload, and gets it back on download in a field added to MISSION_COUNT (note, fewer fields, as you don't need it in both ACK and COUNT)
• Values emitted from MISSION_CURRENT
• Id has to be different from current mission. It couldn't reliably be iterated so would have to be calculated. It would therefore be better off being bigger - might be a UUID or a CHECKSUM.

With this

• A GCS uploading a plan will have the matching values in MISSION_CURRENT. Reupload not triggered
• Any other GCS will have a mismatch following upload of a new mission. After reuploading the mismatched plan-part it will match.
• An ArduPilot FTP based plan upload could again be required to use the checksum.

The main difference between the two approaches is that for the GCS you probably have to have a bigger ID because you can't reliably iterate (I don't think). The benefit is that the MISSION_COUNT changes are simpler, and occur early in the mission upload/download - so systems that for some reason don't get the ACK will still get the value.

[hamishwillee]

FYI @KonradRudin @peterbarker

[KonradRudin]

Thanks for the proposal @hamishwillee.
I prefer the solution where the vehicle supplies the ID. The vehicle should internally anyways keep track if the mission has been changed through some means which can be used as a counter/ID. Also it then is only dependent on the vehicle and not on all GCS connecting to the vehicle.
If the opaque Id should just be a counter or not depends if we want to have the ability for the GCS upon a first connect to know, if it already has the correct mission or needs to download it again. If we anyway download the mission on a first connect, a counter is sufficient.
I also prefer to have the IDs for all different mission items, since we do not want to download the complete mission. on rally point changes. Also this would only add additional 8 bytes (considering uint32) at 1 Hz in the MISSION_CURRENT which doesn't add much more load on the data link.

[hamishwillee]

Thanks @KonradRudin

If the opaque Id should just be a counter or not depends if we want to have the ability for the GCS upon a first connect to know, if it already has the correct mission or needs to download it again. If we anyway download the mission on a first connect, a counter is sufficient.

Thinking about this, the bigger issue about a counter is that it can reset, and possibly should if the mission is removed. That would mean that you'll far more often see a lower number than a high number and potentially have more chance of clashes.

I will add a note that if the mission is remove the associated id should be zero or max value.

Let's see what @peterbarker thinks about 8 extra bytes. If you're on an old 3DR radio, that might be a problem.

[peterbarker]

• the vehicle must supply the ID - the vehicle may itself programatically change the mission, for example
• a counter will work badly if the persistent state is restored from a backup somehow. For example, if one were to borrow a parameter to do it and paramters were restored, that would be bad.
• special care must be taken that your unique ID is not zero; in ArduPilot if the checksum turns out to be zero I emit UINT32_MAX instead.
• I have updated my Pull Request against against ArduPilot here: Implement new mission opaque ID protocol ArduPilot/ardupilot#20834

[hamishwillee]

@peterbarker @KonradRudin I have done minor update to description for readability (no semantic changes).

As discussed with both of you this reflects the original proposal above where:

• the id is calculated on upload to vehicle and returned in MISSION_ACK.
• On download it is returned in MISSION_COUNT. The ids for each part of the plan are streamed in mission count.
• All values are set to 0 if the feature is not supported, or if the plan-part is empty, or for invalid (e.g. in mission count on upload).

Need an implementation and validation this is OK before this can be merged, as it affects common.xml.

Do you guys have a plan/timeline for release so we can plan getting this in?

[peterbarker]

On Tue, 4 Jul 2023, Hamish Willee wrote: Do you guys have a plan/timeline for release so we can plan getting this in?

Sorry, this one is currently on an "as time permits" regime. My PR is failing all sorts of autotests right now, so there's still things to be sorted through.

[hamishwillee]

Thanks @peterbarker . Maybe @KonradRudin can get a testable version in faster :-)

[KonradRudin]

I will have time in the next few weeks to update and test on the PX4 side.

[hamishwillee]

I will have time in the next few weeks to update and test on the PX4 side.

Thank you. I assume you can fake the required mavlink, as though this had merged.

[hamishwillee]

@KonradRudin How's the work on this going?

[KonradRudin]

Hey @hamishwillee
The PX4 side is ready for review in PX4/PX4-Autopilot#21839 since some weeks.
But the ground station side will just be implemented soon so we could not test it yet end to end. But is sheduled to be implemented soon. So please hang on for just a bit.

[hamishwillee]

Thanks @KonradRudin - when you have a QGC PR can you please point me to it. It would be good for @peterbarker to test his changes at the same time so that we know we have two compatible implementations.

[KonradRudin]

Hey @hamishwillee we started to work on the implementation for the opaque ID again. We now have a working setup including PX4 and a groundstation. However, it is only for our own QGC version currently so sadly not shareable atm. At least you could check it out. How do we want to proceed?

[hamishwillee]

Hi Konrad, just back from flying so a bit Jetlagged. I guess logging or testing that it works as advertised? @julianoes is this something MAVSDK might be interested in providing user access to?

@auturgy Any thoughts?

[auturgy]

@peterbarker do you have an update?
@hamishwillee It's a bit hard to form an opinion based on an implementation that consists of an unreviewed draft autopilot PR and a closed gcs...

[hamishwillee]

@KonradRudin When will this version of AMC be published? (AMC is closed source, but that doesn't mean it can't be used to observe the behaviours of interest). When do you plan to un-draft the PR and get it reviewed? Is there any testing done and if so, how?

Our interest is not in merging stuff until we're sure that it will appear in flight stacks and is really common. Until your PR has seen some constructive criticism we can't proceed.

@auturgy Thanks for your point, I hadn't really looked at the PR - was more after "what is minimal testing do we need evidence of". The ArduPilot PR appears to be stuck - @peterbarker is keeping it updated but no obvious moves to merge ArduPilot/ardupilot#20834

[julianoes]

@julianoes is this something MAVSDK might be interested in providing user access to?

Sure, if someone wants to sponsor me, it'll happen sooner rather than later.

[KonradRudin]

@hamishwillee well i can't really get it out of the draft status as long as i need to point on a special branch on mavlink for it to work. But i can get it reviewed. But the order must be that this gets merged first before it can be merged in PX4.

[KonradRudin]

@hamishwillee so i discussed the issue and we can provide the binary file for our qgc. AS far as testing goes, apart form unit testing we did an integration test by connecting multiple GCS instances to PX4 in SITL mode and changed the mission on one GCS, then the other GCS automatically updates the mission and geofence as well.

[hamishwillee]

@hamishwillee well i can't really get it out of the draft status as long as i need to point on a special branch on mavlink for it to work. But i can get it reviewed. But the order must be that this gets merged first before it can be merged in PX4.

Of course!

Ping me when you have enough of a review where someone with merge rights says they would be willing to press the merge button if this was not dependent on the wrong branch and we can move forward. Then let's see about the binary.

[dagar]

I still think a mission checksum would have been better, but this is close enough for what I wanted to address in PX4.

[KonradRudin]

@hamishwillee I can share the link to google drive for the binary of our groundstation implementation. This can be tested then with the PX4 PR f you need more confidence.
https://drive.google.com/file/d/1k2adhfaey1z4C4TqvqRBDjcLtuLYa9wi/view?usp=sharing

[hamishwillee]

@KonradRudin Great. @auturgy has requested access to look at this.

After that, If you feel this has been tested enough I think after that we can merge because:

1. Daniel has said it can go in
2. The implementation doesn't rely on a particular algorithm for calculating the ID, which is the most likely problem area. So this should "work".

I still think a mission checksum would have been better, but this is close enough for what I wanted to address in PX4.

@dagar That wasn't going to work for ArduPilot, where the stored on-vehicle and off vehicle mission are different. There is nothing to stop PX4 using an opaque ID that is a checksum but MAVLink can't mandate that. But FWIW I would have much preferred that too.

@KonradRudin If we haven't heard from @auturgy by Tuesday next week and you feel due diligence has been done, please ping me so I can chase this up. My intent is for this to merge next week.

[KonradRudin]

Hey @hamishwillee can we merge now? So far i had one request for access which i granted for our GCS for testing.

[hamishwillee]

@KonradRudin Yes. Thanks for your patience.