Implement mission checksum

[MatejFranceskin]

Describe problem solved by this pull request
Ground control station should be able to compare its flight plan with the plan stored in the vehicle to determine if download/upload of mission plan is necessary.

Describe your solution
Implement calculation of mission checksum as defined in https://mavlink.io/en/messages/development.html#MISSION_CHECKSUM
Ground control station can request it with MAV_CMD_REQUEST_MESSAGE and compare its missions with those that are stored on the vehicle.

[julianoes]

Nice! Just please check all dm_lock and dm_unlock, and consider using a union instead of casting.

[dagar]

Generally looks good, but I was wondering why not have this in mavlink_mission? Then we could calculate the crc for a mission upon receipt, and the crc could be broadcast as part of ORB_ID(mission). https://github.com/PX4/PX4-Autopilot/blob/master/msg/mission.msg

Then instead of a new MISSION_CHECKSUM stream it can simply be a method in mavlink_mission alongside mission_count.

PX4-Autopilot/src/modules/mavlink/mavlink_mission.cpp

Line 292 in 8f6fd5f

MavlinkMissionManager::send_mission_count(uint8_t sysid, uint8_t compid, uint16_t count, MAV_MISSION_TYPE mission_type)

Later I'd also like to make it a more integral part of verifying the mission in dataman before usage.

[MatejFranceskin]

Generally looks good, but I was wondering why not have this in mavlink_mission? Then we could calculate the crc for a mission upon receipt, and the crc could be broadcast as part of ORB_ID(mission). https://github.com/PX4/PX4-Autopilot/blob/master/msg/mission.msg
@dagar I am slightly lost - how can I handle all 4 checksum types this way? (MISSION, FENCE, RALLY & ALL)

[ThomasDebrunner]

Gave it a shot to refactor to @dagar 's comments. The checksums are now computed upon receiving from mavlink and combined when requesting checksum for all. That's all fine and pretty clean.
What is now far less clean, is that I need to hijack the handling of the MAV_CMD_REQUEST_MESSAGE to re-direct to the _mission_manager instead of handling it by streams. I see three options:

• Make a mavlink stream for the checksum messages (basically like Matej original PR)
• Do not use MAV_CMD_REQUEST_MESSAGE to request the checksum (change in MAVLINK)
• Keep the "hack" I addd here

@dagar @julianoes

[julianoes]

What about using development.xml for all boards?

diff --git a/src/modules/mavlink/CMakeLists.txt b/src/modules/mavlink/CMakeLists.txt
index f57186d52d..de750922af 100644
--- a/src/modules/mavlink/CMakeLists.txt
+++ b/src/modules/mavlink/CMakeLists.txt
@@ -33,11 +33,6 @@
 
 set(MAVLINK_DIALECT "development") # standard, development, etc
 
-# force mavlink dialect to standard if flash constrained
-if(px4_constrained_flash_build)
-       set(MAVLINK_DIALECT "standard")
-endif()
-
 set(MAVLINK_GIT_DIR "${CMAKE_CURRENT_LIST_DIR}/mavlink")
 set(MAVLINK_LIBRARY_DIR "${CMAKE_BINARY_DIR}/mavlink")
 file(RELATIVE_PATH MAVLINK_GIT_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${MAVLINK_GIT_DIR})

[ThomasDebrunner]

@julianoes I think that was how it used to be before the mavlink auto-generation, right?

Added a commit for that

[julianoes]

@julianoes I think that was how it used to be before the mavlink auto-generation, right?

We did not have the development.xml dialect before auto-generation, so not really.

[DanielePettenuzzo]

@dagar Can you take a look at this one when you have time? We are already using this branch and everything works for us so far.

[ThomasDebrunner]

Just rebased that to master and tested, @julianoes I think that should be good to go

[junwoo091400]

Any update on this? This seems like a cool feature we can have without much more effort, as the PR is almost ready (just need merge conflict resolve)

[KonradRudin]

I have rebased the PR on top of main.
Additional small changes:

• Make sure the crc is calculated with the mavlink endianness
• The mission checksum is broadcasted at a frequency of 1 Hz.

[hamishwillee]

The mission checksum is broadcasted at a frequency of 1 Hz.

FWIW the spec says

This message must be broadcast with the appropriate checksum following any change to a mission, geofence or rally point definition ... (and on request)

There is no requirement for it to be streamed. Though there is nothing to stop you doing so, and streaming at 1Hz would mean that you comply.

[hamishwillee]

@KonradRudin ArduPilot tried to implement this and found it "conceptually flawed". I've been waiting on retesting of proposed changes, which is why this is still in development.xml.

The idea is that the autopilot generates the checksum and broadcasts it. The GCS can generate the same checksum and therefore knows that nothing has changed. However this is flawed in general, because of the vagaries of floating point implementations mean that the checksums might not match even if calculated in the same way. The problem is even worse for ArduPilot because they only store the data fields that they use, so the mission on the vehicle does not match the mission on the GCS even in the best case.

The counter proposal is that vehicle calculates a UID for its mission and passes that to the GCS as part of the MISSION_ACK. This value is then added to MISSION_CURRENT and streamed. That way the GCS always knows that it has the correct UID. We dropped the idea of separate ids for fence, plan, rally points, though we could add if the tradeoffs are considered reasonable.

The full discussion and counter proposal is here: ArduPilot/ardupilot#20834 (comment)

I am very sorry that not notifying of this discussion has potentially cost you work.

Thoughts?

[KonradRudin]

Hey @hamishwillee Thanks for mentioning this.
Is there any proposal of this on the mavlink repo as well? I haven't found anything about it. Generally, it does not matter for us if it is a checksum or a UID, we just need a mechanism to check on QGC if the mission is still up to date (and i have found the MISSION_CHECKSUM message to be the most useful instead of defining something new). Also since we are looking into Multi GCS connections, we need a way to to synchronize the mission, geofence, and rally points, thus the simple solution here was to simple stream it at a low frequency. We for sure would need the ability to stream the checksum/UID for all those three, since we have no other way of knowing that those have changed from another GCS.

[hamishwillee]

@KonradRudin no, other discussion than on ArduPilot. It's a flaw in our MAVLInk process that stuff in development is kind of "open to all". The theory being that only one group are working on it .

So my take on this is that the UID can be a checksum, and it can be streamed. That's all good. IN fact the MISSION_CURRENT is streamed anyway. So the proposed change here is that we add extensions to MSSON_CURRRENT rather than having a stand alone message, and we add a change to the mission ack on upload as well.
Other than that nothing much would need to change for you.

You can PR a change, or ping me on Wednesday and I can do it.

[KonradRudin]

Here is the PR for the new Mavlink proposal:
mavlink/mavlink#2012

[KonradRudin]

I would propose to close this Pr in favor of #21839. Reasoning: The Mavlink support for MISSION_CHECKSUM is deprecated as seen in mavlink/mavlink#2010