Download mission capability #145

julianoes · 2017-10-30T19:47:43Z

This changes the API from send_mission_async() to upload_mission_async() and adds download_mission_async.

This allows missions created with DroneCore to be downloaded from the vehicle. However, DroneCore only supports a subset of the mavlink mission protocol and will fail with the Result::UNSUPPORTED for anything it does not know.

coveralls · 2017-10-30T21:50:36Z

Coverage remained the same at 55.131% when pulling d17aebd on download-mission into 09053c3 on develop.

hamishwillee · 2017-10-31T02:18:16Z

plugins/mission/mission.h

+     *
+     * The mission items are downloaded from a drone.
+     *
+     * @param mission_items reference to vector of mission items.


Capitalise R in reference

hamishwillee · 2017-10-31T02:18:41Z

plugins/mission/mission.h

+     * The mission items are downloaded from a drone.
+     *
+     * @param mission_items reference to vector of mission items.
+     * @param callback callback to receive result of this request


Capitisalise C in "Callback to ..."

hamishwillee · 2017-10-31T02:18:55Z

plugins/mission/mission.h


    /**
     * @brief Starts the mission (asynchronous).
     *
-     * Note that the mission must be uplaoded to the vehicle using `send_mission_async()` before
+     * Note that the mission must be uplaoded to the vehicle using `upload_mission_async()` before
     * this method is called.
     *
     * @param callback callback to receive result of this request


Capitalise sentence

hamishwillee · 2017-10-31T02:19:20Z

plugins/mission/mission.h

@@ -49,21 +51,37 @@ class Mission
    typedef std::function<void(Result)> result_callback_t;

    /**
-     * @brief Sends a vector of mission items to the device (asynchronous).
+     * @brief Uploads a vector of mission items to the device (asynchronous).
     *
     * The mission items are uploaded to a drone. Once uploaded the mission can be started and
     * executed even if a connection is lost.
     *
     * @param mission_items reference to vector of mission items.
     * @param callback callback to receive result of this request


Capitalize C in sentence. Add full stop.

hamishwillee · 2017-10-31T02:24:21Z

Can we have

an integration test attempts to download a complicated mission.
Example or example idea for a download, modify, upload example

hamishwillee · 2017-10-31T02:43:33Z

So to be very clear, you can't download a "partial mission". It either fails and returns nothing or succeeds. Correct?

So is the assumption that a user will create mission in QGC (say) and that DroneCore will allow you to download it if it is valid? Are we expecting people to then manipulate the mission in DroneCore and reupload? How useful is this?

I have created linked issue to update the mission example and guide docs when this goes in.

I still think that as a user I'd sometime like to bypass DC altogether and upload/download raw mavlink mission files. Then I could support any mission I liked. Not as "robust" but an easy way to get dronecore working with any vehicle.

This looks fine to me, but leave to @mrpollo to do proper C++ review.

julianoes · 2017-10-31T12:48:55Z

Can we have

an integration test attempts to download a complicated mission.

Example or example idea for a download, modify, upload example

I have added an integration test that uploads and then downloads the mission and verifies it.
I don't think that is needed. It seems quite complicated and not a common use case. We don't need examples for all functionality in my opinion.

julianoes · 2017-10-31T12:54:43Z

So to be very clear, you can't download a "partial mission". It either fails and returns nothing or succeeds. Correct?

Correct. I don't want bugs or corner cases because something gets omitted or changed.

So is the assumption that a user will create mission in QGC (say) and that DroneCore will allow you to download it if it is valid? Are we expecting people to then manipulate the mission in DroneCore and reupload? How useful is this?

No, that's not really the use case. This is to download DroneCore missions or super simple QGC missions if really needed.

I have created linked issue to update the mission example and guide docs when this goes in.

Thanks.

I still think that as a user I'd sometime like to bypass DC altogether and upload/download raw mavlink mission files. Then I could support any mission I liked. Not as "robust" but an easy way to get dronecore working with any vehicle.

The point of DroneCore is to give you a thoroughly tested API. Once more functionality is tested and implemented easy to use, then we can add that. If somebody wants to write its own mission, they can just write their own mission plugin or use something else altogether. I know this is very opinionated but I see people struggling with the mavlink API every day and if we just replicate that with DroneCore we are no better but only added another layer.

This looks fine to me, but leave to @mrpollo to do proper C++ review.

Yes please.

coveralls · 2017-10-31T13:08:33Z

Coverage remained the same at 55.131% when pulling f6d5247 on download-mission into 09053c3 on develop.

hamishwillee · 2017-11-01T23:45:44Z

@julianoes

So yes to almost everything. The "no" is I still don't see why we are bothering. Consider

Example or example idea for a download, modify, upload example

I have added an integration test that uploads and then downloads the mission and verifies it.
I don't think that is needed. It seems quite complicated and not a common use case. We don't need examples for all functionality in my opinion.

So is the assumption that a user will create mission in QGC (say) and that DroneCore will allow you to download it if it is valid? Are we expecting people to then manipulate the mission in DroneCore and reupload? How useful is this?

No, that's not really the use case. This is to download DroneCore missions or super simple QGC missions if really needed.

Yes, but if you're not going to manipulate them, why would you bother? If its a DroneCore mission you already created it in your app, so why bother downloading it from the vehicle? Just seems a bit pointless.

Perhaps doesn't matter for this PR, but for the docs it is nice to be able to explain why functionality exists.

julianoes · 2017-11-02T12:19:39Z

Yes, but if you're not going to manipulate them, why would you bother? If its a DroneCore mission you already created it in your app, so why bother downloading it from the vehicle? Just seems a bit pointless.

Good point. Because it is on my feature todo list 😄.

One reason could be that you plan a mission using a desktop application and upload it to the drone. Later when you view the telemetry using an app on your phone you can download the mission, so you can follow the progress.

mrpollo · 2017-11-06T19:50:23Z

@hamishwillee here's the list of supported commands this PR introduces

MAV_CMD_NAV_WAYPOINT
MAV_CMD_DO_MOUNT_CONTROL
MAV_CMD_IMAGE_START_CAPTURE
	START_PHOTO_INTERVAL
	TAKE_PHOTO
MAV_CMD_IMAGE_STOP_CAPTURE
MAV_CMD_VIDEO_START_CAPTURE
MAV_CMD_VIDEO_STOP_CAPTURE
MAV_CMD_DO_CHANGE_SPEED

from assemble_mission_items found in mission_impl.cpp

hamishwillee · 2017-11-06T23:50:59Z

Thanks @mrpollo - #145 (comment) not above very useful. I won't do update in my docs tree until this has been merged into develop.

@julianoes What other ones are supported - I should add them all.

julianoes · 2017-11-07T21:09:14Z

@julianoes What other ones are supported - I should add them all.

It's just these, I think.

hamishwillee · 2017-11-07T22:43:47Z

Thanks. Any ETA on this going into develop?

It seems more intuitive to "upload" a mission than to "send" it. Also, once the functionality to download a mission, it would be awkward to call it "receive" a mission.

And don't forget to set activity.

When we implement mission item download we need to have a unique mapping from mavlink messages to mission items, otherwise we can't compare uploaded to downloaded mission items. Previously, we omitted gimbal and speed settings if the previous mission item had the same setting. However, this would mean that we don't have a unique mapping when downloading a mission item. For instance if we have two mission items with speed 5 m/s, we would only send one mission item and omit the speed setting in the second mission item. When downloading the mavlink items for this, we get NAN for the speed of the second mission item because we don't if it was actually set or omitted when set. By always setting the speed, we have a bit more overhead but it makes it explicit and we don't lose information when doing multiple transfers. To save mission item "space" it is always possible to set the speed to NAN which means no mavlink item is sent for it.

This should make the mission module somewhat thread-safe.

This adds the feature to download mission items from a vehicle. This currently fails if any command or waypoint is downloaded that is not implemented. Implemented means what can be uploaded.

julianoes · 2017-11-08T18:52:16Z

@hamishwillee rebased, if CI goes through we can merge it.

coveralls · 2017-11-08T19:48:29Z

Coverage remained the same at 17.571% when pulling af5007d on download-mission into 670b15f on develop.

hamishwillee · 2017-11-08T21:55:38Z

Looks like some checks were not successful :-(

coveralls · 2017-11-10T21:30:13Z

Coverage remained the same at 17.571% when pulling 7085d97 on download-mission into 670b15f on develop.

julianoes requested review from mrpollo and hamishwillee October 30, 2017 19:47

hamishwillee mentioned this pull request Oct 31, 2017

Update mission docs when 145 merged mavlink/MAVSDK-docs#54

Closed

hamishwillee reviewed Oct 31, 2017

View reviewed changes

julianoes force-pushed the download-mission branch from d17aebd to f6d5247 Compare October 31, 2017 12:54

mrpollo approved these changes Nov 6, 2017

View reviewed changes

julianoes added 9 commits November 8, 2017 13:50

mission: change send_mission to upload_mission

710e5ca

It seems more intuitive to "upload" a mission than to "send" it. Also, once the functionality to download a mission, it would be awkward to call it "receive" a mission.

integration_tests: the mission API was renamed

16247d0

example: adapt after mission API changed

bc5e11a

grpc: adapted after mission API changed

d1bc5a9

mission: various cleanup

b684be5

mission: add missing return in error case

9072384

mission: set activity before calling callback

3e6ba4e

And don't forget to set activity.

mission: add a mutex to lock everything

1390c0c

This should make the mission module somewhat thread-safe.

julianoes added 3 commits November 8, 2017 13:50

mission: add mission item download

8704697

This adds the feature to download mission items from a vehicle. This currently fails if any command or waypoint is downloaded that is not implemented. Implemented means what can be uploaded.

mission: add another missing return

f4b847d

mission: capitalize, add period.

af5007d

julianoes force-pushed the download-mission branch from f6d5247 to af5007d Compare November 8, 2017 18:51

mission: fix double promotion error

7085d97

julianoes merged commit 54890a2 into develop Nov 10, 2017

julianoes deleted the download-mission branch November 10, 2017 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download mission capability #145

Download mission capability #145

julianoes commented Oct 30, 2017

coveralls commented Oct 30, 2017

hamishwillee Oct 31, 2017

hamishwillee Oct 31, 2017

hamishwillee Oct 31, 2017

hamishwillee Oct 31, 2017

hamishwillee commented Oct 31, 2017

hamishwillee commented Oct 31, 2017 •

edited

Loading

julianoes commented Oct 31, 2017

julianoes commented Oct 31, 2017

coveralls commented Oct 31, 2017

hamishwillee commented Nov 1, 2017 •

edited

Loading

julianoes commented Nov 2, 2017

mrpollo commented Nov 6, 2017 •

edited

Loading

hamishwillee commented Nov 6, 2017 •

edited

Loading

julianoes commented Nov 7, 2017

hamishwillee commented Nov 7, 2017

julianoes commented Nov 8, 2017

coveralls commented Nov 8, 2017

hamishwillee commented Nov 8, 2017

coveralls commented Nov 10, 2017

Download mission capability #145

Download mission capability #145

Conversation

julianoes commented Oct 30, 2017

coveralls commented Oct 30, 2017

hamishwillee Oct 31, 2017

Choose a reason for hiding this comment

hamishwillee Oct 31, 2017

Choose a reason for hiding this comment

hamishwillee Oct 31, 2017

Choose a reason for hiding this comment

hamishwillee Oct 31, 2017

Choose a reason for hiding this comment

hamishwillee commented Oct 31, 2017

hamishwillee commented Oct 31, 2017 • edited Loading

julianoes commented Oct 31, 2017

julianoes commented Oct 31, 2017

coveralls commented Oct 31, 2017

hamishwillee commented Nov 1, 2017 • edited Loading

julianoes commented Nov 2, 2017

mrpollo commented Nov 6, 2017 • edited Loading

hamishwillee commented Nov 6, 2017 • edited Loading

julianoes commented Nov 7, 2017

hamishwillee commented Nov 7, 2017

julianoes commented Nov 8, 2017

coveralls commented Nov 8, 2017

hamishwillee commented Nov 8, 2017

coveralls commented Nov 10, 2017

hamishwillee commented Oct 31, 2017 •

edited

Loading

hamishwillee commented Nov 1, 2017 •

edited

Loading

mrpollo commented Nov 6, 2017 •

edited

Loading

hamishwillee commented Nov 6, 2017 •

edited

Loading