350 mission backend #369

JonasVautherin · 2018-04-23T08:47:05Z

Add UploadMission and StartMission to the mission backend.

Connects to #350.

julianoes · 2018-04-23T13:49:18Z

backend/test/mission_service_impl_test.cpp

 {
-    // TODO
+    auto upload_handle = uploadMissionAndSaveParams(nullptr, nullptr);


julianoes · 2018-04-23T13:50:47Z

backend/test/mission_service_impl_test.cpp

 {
-    // TODO
+    auto upload_handle = uploadMissionAndSaveParams(nullptr, nullptr);
+    _result_callback(dc::Mission::Result::UNKNOWN);


Why would the result be UNKNOWN? I think the result should only be unknown if something in the enum translation went wrong. If it is an actual error, it should be ERROR or some specific error code, right?

Or is it that the error is unknown because with nullptr the callback has not actually been called?

It is just that I need to synchronize the call in the service (see here and here). So the upload call is blocked until the callback is actually called (this assumes that the callback is always called, eventually).

The value of it in this unit test has not importance. It just needs to trigger. Maybe I should make it a variable called ARBITRARY_RESULT, to make this clear!

One question is also whether it would make sense to make a sync upload_mission(...) function in the core libs as well, so that I could get rid of this future here.

Ok, yer UNKNOWN is totally fine then.

One question is also whether it would make sense to make a sync upload_mission(...) function in the core libs as well, so that I could get rid of this future here.

So, is the easier way to use sync calls in the grpc backend? If yes, then it probably makes sense to have sync calls by default.

julianoes · 2018-04-23T13:54:16Z

backend/test/mission_service_impl_test.cpp

+    auto request = generateUploadRequest(mission_items);
+    auto upload_handle = uploadMissionAndSaveParams(request, response);
+
+    _result_callback(GetParam().second);


Help me please. What's GetParam() here?

That's about the parameterized tests. You see it is parameterized because it is a TEST_P. And the parameters are instantiated here. So GetParam().second will get the second value of each of the pairs initialized here (i.e. the test is run once for each pair).

ah, got it, thank you!

julianoes · 2018-04-23T13:56:46Z

backend/test/mission_service_impl_test.cpp

+    auto mission_item = std::make_shared<dc::MissionItem>();
+    mission_item->set_position(41.848695, 75.132751);
+    mission_item->set_relative_altitude(50.4);
+    mission_item->set_speed(8.3);


Some of the fields are actually floats and so it should be 8.3f, however, I wonder if we shouldn't just make everything double in DroneCore, especially in the public interface since DroneCore is not run on embedded platforms generally and everything is becoming 64 bits these days.

Oh, is the compiler here generating a double with value 8.3, and then implicitly casting it to a float? I will fix that.

I am not familiar with performance issues on embedded platforms, so I'll follow you on that (but dronecore could be run onboard the drone, right?). For me it's not a big problem to have floats. We will just need to stick to our decisions as it will be define in the proto files :-).

Yes, but the onboard computers are likely arm64 these days, so moving to a double only interface might make sense.

julianoes · 2018-04-23T14:00:19Z

plugins/mission/mission_item.cpp

+bool operator==(const MissionItem &lhs, const MissionItem &rhs)
+{
+    return (lhs.get_latitude_deg() == rhs.get_latitude_deg()
+            || isnan(lhs.get_latitude_deg()) && isnan(rhs.get_latitude_deg()))


I think it would be better to make it explicit what the operator order is.

Oh, right. macOS is warning about that, and apparently I did not fix it on the right computer. I'll change it!

Haha, it's a warning. The operator precedence is clear in theory but it's nice for the reader to make it explicit in my opinion.

julianoes · 2018-04-23T14:00:36Z

plugins/mission/mission_item.cpp

+           << "\tloiter_time: " << mission_item.get_loiter_time_s() << std::endl
+           << "\tcamera_action: " << mission_item.get_camera_action() << std::endl
+           << "\tcamera_photo_interval: " << mission_item.get_camera_photo_interval_s()
+           << std::endl << "]" << std::endl;


julianoes · 2018-04-23T14:01:35Z

plugins/mission/mission_item.cpp

+        case MissionItem::CameraAction::STOP_VIDEO:
+            return str << "CameraAction::STOP_VIDEO";
+        case MissionItem::CameraAction::NONE:
+            return str << "CameraAction::NONE";


We should probably do the same kind of thing instead of the result_str() methods.

julianoes · 2018-04-23T14:02:22Z

plugins/mission/mocks/mission_mock.h

@@ -8,13 +8,14 @@
 namespace dronecore {
 namespace testing {

-typedef std::function<void(Mission::Result)> result_callback_t;
+typedef std::function<void(Mission::Result)> mission_result_callback_t;


Can't you get this typedef from somwhere else? Or is it specific to testing?

You're right. I did not want to take Mission::result_callback_t, but for wrong reasons :-).

julianoes

Looks good to merge whenever you're done with the changes.

Effectively fixing the type of the relative altitude field in MissionItem (was double, needs float).

* Add an ARBITRARY_RESULT to tests to make it clear that the result does not matter * Make comparison operators priority clear by adding parentheses * Use result_callback_t from Mission instead or redefining it * Fix some floats assignments ("f" suffix was forgotten)

JonasVautherin added the grpc label Apr 23, 2018

JonasVautherin self-assigned this Apr 23, 2018

JonasVautherin requested review from julianoes and shakthi-prashanth-m April 23, 2018 08:47

JonasVautherin added the in progress label Apr 23, 2018

JonasVautherin force-pushed the 350-mission-backend branch 2 times, most recently from c61d8e0 to 6e922bf Compare April 23, 2018 13:51

julianoes reviewed Apr 23, 2018

View reviewed changes

julianoes approved these changes Apr 23, 2018

View reviewed changes

JonasVautherin added 6 commits April 23, 2018 18:00

backend: add missing getters for gimbal in mission item

484d0f7

backend: add UploadMission command in mission service

e98f20a

backend: add StartMission command in mission service

72c0f30

backend: add main to unit tests runner

6d250bd

backend: update proto submodule

3ecf449

Effectively fixing the type of the relative altitude field in MissionItem (was double, needs float).

JonasVautherin force-pushed the 350-mission-backend branch from 144af79 to 9b677cf Compare April 23, 2018 16:00

JonasVautherin merged commit c84df75 into develop Apr 23, 2018

JonasVautherin deleted the 350-mission-backend branch April 23, 2018 17:17

JonasVautherin removed the in progress label Apr 23, 2018

mrpollo mentioned this pull request Jun 5, 2018

v1.0.0 Changelog scratchpad #417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

350 mission backend #369

350 mission backend #369

JonasVautherin commented Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

JonasVautherin Apr 23, 2018

JonasVautherin Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

JonasVautherin Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

JonasVautherin Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

JonasVautherin Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

julianoes Apr 23, 2018

JonasVautherin Apr 23, 2018

julianoes left a comment

350 mission backend #369

350 mission backend #369

Conversation

JonasVautherin commented Apr 23, 2018

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

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

julianoes left a comment

Choose a reason for hiding this comment