Support Mission Download In Mission Raw Server #2463

jonathanreeves · 2024-12-09T05:17:48Z

Mission Download Support

Overview

MAVSDK has two classes for managing mission transfers:

MavlinkMissionTransferClient used for initiating mission transfers (used primarily on the ground side)
MavlinkMissionTransferServer used for handling mission transfer requests (used primarily on the air vehicle)

MavlinkMissionTransferClient has support both for sending missions as well as requesting missions from a server and handling the response. MavlinkMissionTransferServer currently only handles receiving missions, but does not respond to mission download requests (air to ground). The MissionRawServer service uses MavlinkMissionTransferServer to handle mission transfer requests, and thus MissionRawServer is currently "unidirectional".

This pull request implements the required functionality in MavlinkMissionTransferServer to handle mission download requests (air to ground), and adds the necessary hooks in MissionRawServer.

Guides For Review

Files changed will be commented with relevant guides. At a high level the changes are as follows:

src/mavsdk/core/mavlink_mission_transfer_server.h/.cpp: added SendOutgoingMission work item class and send_outgoing_items_async() method for handling outgoing transfers. Added associated unit tests
src/mavsdk/plugins/mission_raw_server/include/plugins/mission_raw_server/mission_raw_server.h: added API for mission download result callback
src/mavsdk/plugins/mission_raw_server/mission_raw_server_impl.h/.cpp: Added listener for MAVLINK MISSION_REQUEST_LIST and machinery to queue an outgoing mission transfer in MavlinkMissionTransferServer.

Proof of function

Before this change, running QGroundControl against the MAVSDK MissionRawServer would result in the following error on launch:

The QGC log shows the same failed queries

PlanManagerLog: "Retrying T:Mission REQUEST_LIST retry Count" 1
PlanManagerLog: "_requestList T:Mission _planType:_retryCount" 0 1
...
PlanManagerLog: "Retrying T:Mission REQUEST_LIST retry Count" 2
PlanManagerLog: "_requestList T:Mission _planType:_retryCount" 0 2
...
PlanManagerLog: "Retrying T:Mission REQUEST_LIST retry Count" 6
PlanManagerLog: "_requestList T:Mission _planType:_retryCount" 0 6
...

Here's a video showing a vehicle loaded with a valid mission, followed by closing QGC and re-launching:

QGC-failed-reload-mission-4.mp4

And here's doing the same thing after applying this change. Note that you can close QGC and re-launch, and the same mission re-populates in the UI without error

QGC-reload-mission-4.mp4

Opportunities For Improvement

MavlinkMissionTransferClient and MavlinkMissionTransferServer currently have a great deal of duplicated code. This was true before this change, but I haven't done anything to address opportunistic refactoring. In this future this could probably be collapsed into a single multipurpose mission transfer class.

jonathanreeves · 2024-12-09T05:20:12Z

@julianoes @JonasVautherin my apologies, I know this is a much bigger change than the last one, and will for sure take a bit more to review. I think this functionality is worth having in mainline though, I hope you agree. If you want to discuss any of this in real time please don't hesitate to reach out. I'd be happy to set up a call to walk through the changes, or whatever else you might find useful.

jonathanreeves · 2024-12-09T05:23:13Z

src/mavsdk/core/mavlink_mission_transfer_server.cpp

@@ -238,7 +271,6 @@ void MavlinkMissionTransferServer::ReceiveIncomingMission::process_mission_count

    _timeout_handler.refresh(_cookie);
    _next_sequence = 0;
-    _step = Step::RequestItem;


This appeared to be unused (probably copy pasta), removed.

jonathanreeves · 2024-12-09T05:24:27Z

src/mavsdk/core/mavlink_mission_transfer_server.cpp

@@ -301,4 +333,363 @@ void MavlinkMissionTransferServer::ReceiveIncomingMission::callback_and_reset(Re
    _done = true;
 }

+MavlinkMissionTransferServer::SendOutgoingMission::SendOutgoingMission(


The vast majority of this block of changes was copied from MavlinkMissionTransferClient::UploadWorkItem with minor adjustments.

jonathanreeves · 2024-12-09T05:26:05Z

src/mavsdk/core/mavlink_mission_transfer_server.cpp

+        }
+    }
+
+    if (_items.size() > 0) {


Note that this is different from the Mission Transfer Client, specifically we do allow send operations when the mission count is 0, in other words the ground station can request a mission download, and get an answer which is an empty mission.

I think that makes sense, so this check could be removed, right?

Actually I think you want to keep this specific check. This is doing an integrity check on non-empty missions to ensure that only one mission item is marked as "current". The concern is the if(fum_currents != 1) check below on line 394 will fail if the mission count is 0, so the point is that we only do these checks if the mission is non-empty. Does that make sense?

I'm sorry I just realized the spirit of your question is more to the point: why are we validating and erroring out on a mission that's being requested for download? Presumably the mission was already validated on upload to the vehicle. I agree this should be removed.

Remove

jonathanreeves · 2024-12-09T05:26:23Z

src/mavsdk/core/mavlink_mission_transfer_server.h

@@ -130,11 +130,6 @@ class MavlinkMissionTransferServer {
        void process_timeout();
        void callback_and_reset(Result result);

-        enum class Step {


Again, unused: removed

jonathanreeves · 2024-12-09T05:29:14Z

src/mavsdk/core/mavlink_mission_transfer_server_test.cpp

The diff of this file makes the changes look bigger than they are due to some slight re-organization I did. There were a bunch of utility functions scattered throughout the file, most of them were not static. The same functions are also declared in mavlink_mission_transfer_server_client, and when I copied some of those methods back here, the linker complained about multiple definition. It seemed cleaner to me that all of these methods should be static class methods, which gives them a protected namespace.

Sure, thanks!

jonathanreeves · 2024-12-09T05:30:04Z

src/mavsdk/core/mavlink_mission_transfer_server_test.cpp

@@ -413,3 +492,877 @@ TEST_F(MavlinkMissionTransferServerTest, ReceiveIncomingMissionCanBeCancelled)
    mmt.do_work();
    EXPECT_TRUE(mmt.is_idle());
 }
+
+TEST_F(MavlinkMissionTransferServerTest, SendOutgoingMissionEmptyMission)


Most of these tests were copied from mavlink_mission_transfer_client_test.cpp, except for this one, which I wrote to explicitly test the ability to query empty missions.

jonathanreeves · 2024-12-09T05:31:30Z

src/mavsdk/plugins/mission_raw_server/include/plugins/mission_raw_server/mission_raw_server.h

+     * @brief Subscribe to the result of outgoing mission download requests (air to ground)
+     */
+    OutgoingMissionResultHandle
+    subscribe_outgoing_mission_result(const OutgoingMissionResultCallback& callback);


I added the ability to get notifications when a mission download request completes (with status) because it seemed harmless. In practice I'm not sure how useful this will actually be, but I left it here, happy to take it out if you think it's cluttering the API.

JonasVautherin

Wow that's a pretty nice PR, with comments and videos! Thanks a lot for the work!

One thing that needs to be done is to update the proto API (this one).

I don't think it will have a big impact on your code though: it will automatically generate the mission_raw_server.h/cpp files (but you have minimal changes there). The benefit is that it will also generate the mavsdk_server part of it, and then it will propagate to the other languages automatically 😊.

Does that make sense?

jonathanreeves · 2024-12-09T14:10:53Z

Wow that's a pretty nice PR, with comments and videos! Thanks a lot for the work!

One thing that needs to be done is to update the proto API (this one).

I don't think it will have a big impact on your code though: it will automatically generate the mission_raw_server.h/cpp files (but you have minimal changes there). The benefit is that it will also generate the mavsdk_server part of it, and then it will propagate to the other languages automatically 😊.

Does that make sense?

It does, thanks for the link! Just noticing there's a failed CI check associated with it too. Still learning my way around the workflows here, but no problem, I'll get it updated today. Thanks for the quick response!

JonasVautherin · 2024-12-09T14:14:47Z

there's a failed CI check associated with it too

That's the style check. You should try running the script fix_style.sh 👍

jonathanreeves · 2024-12-09T17:40:47Z

there's a failed CI check associated with it too

That's the style check. You should try running the script fix_style.sh 👍

jonathanreeves · 2024-12-09T17:42:37Z

Sorry fat fingered the close/re-open. @JonasVautherin just noting the Proto update is here:
mavlink/MAVSDK-Proto#357

Obviously will need some coordination to marshal all changes through, don't really want to update the submodule reference here to a local fork.

jonathanreeves · 2024-12-09T21:33:30Z

Ok @JonasVautherin, sorry for the churn here, a bit of a journey to sort out the right/intended way to do this and get everything in order. I think everything should be good to go now, but please let me know. MAVSDK-Proto PR is open per link above. I'm assuming we'll need that to merge before I can update the submodule here and get the proto CI check to pass. Let me know if I missed anything here.

jonathanreeves · 2024-12-10T17:25:06Z

Just noting that the proto changes are also dependent on this PR merging:
#2386

In other words my proto PR is based on changes associated with that branch which have already merged. I tested this branch by manually patching the proto, but if you actually update the submodule and run ./tools/generate_from_protos.sh, it will break the build since none of the camera changes are there.

jonathanreeves · 2024-12-10T17:46:22Z

.gitmodules

@@ -1,3 +1,3 @@
 [submodule "mavsdk-proto"]
 	path = proto
-	url = https://github.com/mavlink/MAVSDK-Proto.git
+	url = https://github.com/jonathanreeves/MAVSDK-Proto.git


Temporary change to keep this whole feature branch consistent and let the proto CI checks pass. This should be reverted back before merge.

jonathanreeves · 2024-12-10T17:49:10Z

proto

Currently pointing to branch revert-pr-352-multiple-cameras of fork [email protected]:jonathanreeves/MAVSDK-Proto.git. The branch was created by taking the submodule ref currently on MAVSDK main (which currently does not include changes for multiple cameras), and adding the changes for the mission_raw_server API.

Yes that's the way! Then when it's all approved we merge the proto PR, update this one and merge it. It's a bit cumbersome, but it works 😊

Yeah fundamentally not that different from my own/my company's workflows, I was just reluctant to change the .gitmodules file (normally I'd just create a branch directly but no permissions to do that obviously). In the end really not a big deal, thanks for the confirmation on the approach!

julianoes

Just a few small confusions from my side. But I think this is looking great, thanks for that!

julianoes · 2024-12-11T00:51:32Z

src/mavsdk/core/mavlink_mission_transfer_server.cpp

+        }
+    }
+
+    if (_items.size() > 0) {


I think that makes sense, so this check could be removed, right?

julianoes · 2024-12-11T00:52:37Z

src/mavsdk/core/mavlink_mission_transfer_server_test.cpp

Sure, thanks!

julianoes · 2024-12-11T00:58:11Z

src/mavsdk/core/mavlink_mission_transfer_server.cpp

+    if (!_int_messages_supported) {
+        if (callback) {
+            LogErr() << "Int messages are not supported.";
+            callback(Result::IntMessagesNotSupported);
+        }
+        return {};


Does this one make sense? I'm trying to wrap my head around the message support. This would only really make sense for a client that sees that the server does not support INT messages.

In any case, the non-int messages have been deprecated anyway.

Not a blocker, just something that caught my attention.

It's a good point, admittedly I copied this from MavlinkMissionTransferClient::upload_items_async() and scanned over it but didn't think about this super carefully. Reading through the server code _int_messages_supported is initialized to true, and never changed. the set_int_messages_supported() method has no actual implementation in MavlinkMissionTransferServer, which means nobody is calling it, and all of it can be removed. I think I would propose removing this check along with all references to _int_message_supported in this class. Thoughts?

julianoes · 2024-12-11T00:58:47Z

src/mavsdk/plugins/mission_raw_server/mission_raw_server_impl.cpp

 MissionRawServer::MissionPlan MissionRawServerImpl::incoming_mission() const
 {
-    // FIXME: this doesn't look right.
+    // TO-DO


While you're at it 😄 ...

Nevermind, we should just make that one ASYNC only, I think.

Ha yeah I actually did consider adding support for this as part of the PR but current_item_changed() and clear_all() both have the same issue, I figured all three should be done at the same time and I had already let this balloon enough. I agree it might make sense to just make all of them ASYNC though, if you want blocking behavior on the app side you can always set up promises for them. Would you like me to do that? It's easy enough to just update the proto and regen.

Exactly. I can do it in a follow up PR if you don't beat me to it.

No worries, I can roll it in here along with all the other changes discussed tomorrow morning US/Pacific time

jonathanreeves · 2024-12-12T23:09:23Z

proto

Note: this is now pointing to a feature branch which started from 01a546d, which is the current submodule ref for MAVSDK main. I couldn't branch from MAVSDK-Proto main because it contains changes that break MAVSDK. I'm not sure how you want to handle this, but my recommendation would be to make a release branch of MAVSDK-Proto based off of 01a546d, and I can merge these changes there.

jonathanreeves · 2024-12-12T23:12:10Z

@julianoes @JonasVautherin Review items have been addressed, please feel free to re-review. Relevant raw server API elements have been made strictly async as well, but you'll need to tell me what you want to do about the proto submodule.

julianoes

Looks good. Can you make the PR for the proto change?

julianoes · 2024-12-13T02:40:15Z

Ah, I found it, and merged it. So now you can reference it here and push again.

jonathanreeves · 2024-12-13T04:30:12Z

Ah, I found it, and merged it. So now you can reference it here and push again.

Hey @julianoes, unless I've missed something, I don't think I can do that. Currently MAVSDK won't build against MAVSDK-Proto main. I made a MAVSDK-Proto PR that illustrates why that is, you can view it here:
mavlink/MAVSDK-Proto#359

You don't necessarily have to merge that PR, but updating this PR to use something based on MAVSDK-Proto main is going to fail the proto regeneration step.

jonathanreeves · 2024-12-13T04:37:50Z

I updated the submodule to point to the PR I just opened in proto:
mavlink/MAVSDK-Proto#359

I can confirm that this works with the code changes I have here and would maintain consistency with the "mains" of these two projects. Without the camera revert we would be required to merge this first, which seems like it might be a while:
#2386

julianoes · 2024-12-18T02:31:52Z

I've merged the proto PR mavlink/MAVSDK-Proto#359 and rebased this PR and force pushed. If CI agrees, I can merge it soon.

jonathanreeves · 2024-12-18T15:35:15Z

Looks good @julianoes, thanks for helping to get this through! Just noting the submodule is still pointing to my local fork, gonna fix that and push again.

sonarqubecloud · 2024-12-18T15:49:57Z

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
41.7% Duplication on New Code

See analysis details on SonarQube Cloud

julianoes · 2024-12-18T21:26:39Z

Good catch! Thanks.

jonathanreeves commented Dec 9, 2024

View reviewed changes

JonasVautherin reviewed Dec 9, 2024

View reviewed changes

jonathanreeves mentioned this pull request Dec 9, 2024

Added outgoing mission response subscription API mavlink/MAVSDK-Proto#356

Closed

jonathanreeves closed this Dec 9, 2024

jonathanreeves reopened this Dec 9, 2024

jonathanreeves mentioned this pull request Dec 9, 2024

API for receiving status notifications on outgoing mission requests mavlink/MAVSDK-Proto#357

Merged

jonathanreeves commented Dec 10, 2024

View reviewed changes

julianoes reviewed Dec 11, 2024

View reviewed changes

jonathanreeves commented Dec 12, 2024

View reviewed changes

julianoes reviewed Dec 13, 2024

View reviewed changes

Jon Reeves added 2 commits December 18, 2024 15:28

Add mission download support (air to ground) in mission raw server

675fdcc

Removed unused private variable

4a2c089

julianoes force-pushed the main branch from 73ce654 to 4a2c089 Compare December 18, 2024 02:30

Point proto back to mavlink root

293d9ad

julianoes merged commit c7b7bd4 into mavlink:main Dec 18, 2024
40 checks passed

Support Mission Download In Mission Raw Server #2463

Support Mission Download In Mission Raw Server #2463

Conversation

jonathanreeves commented Dec 9, 2024 • edited Loading

Mission Download Support

Overview

Guides For Review

Proof of function

Opportunities For Improvement

jonathanreeves commented Dec 9, 2024

Choose a reason for hiding this comment

jonathanreeves Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanreeves Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

jonathanreeves Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasVautherin left a comment

Choose a reason for hiding this comment

jonathanreeves commented Dec 9, 2024

JonasVautherin commented Dec 9, 2024

jonathanreeves commented Dec 9, 2024

jonathanreeves commented Dec 9, 2024 • edited Loading

jonathanreeves commented Dec 9, 2024

jonathanreeves commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanreeves commented Dec 12, 2024

julianoes left a comment

Choose a reason for hiding this comment

julianoes commented Dec 13, 2024

jonathanreeves commented Dec 13, 2024

jonathanreeves commented Dec 13, 2024

julianoes commented Dec 18, 2024

jonathanreeves commented Dec 18, 2024

sonarqubecloud bot commented Dec 18, 2024

Quality Gate passed

julianoes commented Dec 18, 2024

jonathanreeves commented Dec 9, 2024 •

edited

Loading

jonathanreeves Dec 9, 2024 •

edited

Loading

jonathanreeves Dec 11, 2024 •

edited

Loading

jonathanreeves Dec 11, 2024 •

edited

Loading

jonathanreeves commented Dec 9, 2024 •

edited

Loading