Add QGC Mission import capability #235

shakthi-prashanth-m · 2018-01-23T10:45:37Z

This patch

shakthi-prashanth-m · 2018-01-23T11:06:00Z

So many errors reported in json11! We better ignore them.

julianoes

This is certainly going in the right direction, I think!

julianoes · 2018-01-23T11:07:11Z

CMakeLists.txt

@@ -99,6 +99,7 @@ include(autogenerate_plugin_container.cmake)
 include_directories(
    include
    libs/include
+    libs/json11


put libs/json11 to the bottom of this list and put a SYSTEM in front of it. This way the warnings are ignored.

julianoes · 2018-01-23T11:07:56Z

plugins/mission/mission.cpp

+                    LogErr() << "Mission: Unsupported mission command: " << command;
+                    return Mission::Result::UNSUPPORTED_MISSION_CMD;
+            }
+            std::cout << "Cmd: " << command << " Lat: " << lat_deg << " Lon: " << lon_deg << " Alt: " <<


Can you use LogDebug() please?

Yeah, I will. Thanks.

julianoes · 2018-01-23T11:08:59Z

plugins/mission/mission.cpp

+            }
+            std::cout << "Cmd: " << command << " Lat: " << lat_deg << " Lon: " << lon_deg << " Alt: " <<
+                      rel_alt_deg <<
+                      " Speed " << speed_m_s << " Is fly thru: " << is_fly_through <<


Does this print Is fly through: 1 ? Or true and false?

You could do: << (is_fly_through ? "yes" : "no") <<

Forgot! I will add.

julianoes · 2018-01-23T11:10:47Z

plugins/mission/mission.cpp

+    }
+
+    // Compose mission items
+    Mission::Result result = [&items](const Json & mission_json) -> Mission::Result {


I feel this function is too big. It would be better to split it up into logic that reads the file and a parse function.

Even I felt the same. But we need to make them static again! Also we need to add them to mission.h. C-style is to make them static functions. But lets not use C-style. Isn't it ?

Or we can have whole implementation in MissionImpl class. And lets invoke them via Mission class. Does it make sense ?

Yes, move it to MissionImpl and invoke in Mission.

julianoes · 2018-01-23T16:59:13Z

plugins/mission/mission_item.h

@@ -87,6 +88,13 @@ class MissionItem
        NONE /**< @brief No action. */
    };

+    /**
+     * @brief Converts CamerAction to English strings.


CamerAction -> CameraAction.

julianoes · 2018-01-23T16:59:32Z

plugins/mission/mission_impl.cpp

+                gimbal_yaw_deg = params[2];
+                break;
+            case MAV_CMD_NAV_WAYPOINT:
+                is_fly_through = (params[0] == 0.0) ? true : false;


no break on purpose?

Yes. Because, param 5,6,7 of both NAV_WAYPOINT & NAV_TAKEOFF require Lat, Long and Alt respectively.

julianoes · 2018-01-23T17:00:12Z

plugins/mission/mission_impl.cpp

+                speed_m_s = params[1];
+                break;
+            case MAV_CMD_DO_MOUNT_CONTROL:
+                // Possible bug in QGroundcontrol. It stores -ve values for pitch in deg.


pitch down is - because it's all in the NED frame.

Oh! Thanks @julianoes for the clarification.

julianoes

@shakthi-prashanth-m this is looking good, however, such a piece of code really should have a unit test in my opinion.

I suggest you add an example json file and read it in and check the values. This way, when QGC changes the format, we can just update the test file and rerun it and make sure we're still compatible.

julianoes · 2018-01-24T09:01:36Z

plugins/mission/mission_impl.cpp

+                gimbal_yaw_deg = params[2];
+                break;
+            case MAV_CMD_NAV_WAYPOINT:
+                is_fly_through = (params[0] == 0.0) ? true : false;


Missing break, right?

:) Like I said here, its intentional. Param 5,6,7 of both NAV_WAYPOINT & NAV_TAKEOFF need Lat, Long and Alt respectively, so combined them.

julianoes · 2018-01-24T09:02:31Z

plugins/mission/mission_impl.cpp

+
+        // Add mission item to the list
+        mission_items.push_back([&]() -> std::shared_ptr<MissionItem> const {
+            std::shared_ptr<MissionItem> new_item(new MissionItem());


Instead of std::shared_ptr<MissionItem> new_item(new MissionItem()); you can do auto new_item = std::make_shared<MissionItem>();.

@julianoes I agree with you. How about adding integration_tests/mission_qgc.cpp

It should be a unit test in my opinion.

OK, you mean as part of integration_tests/mission.cpp ?

No, not an integration test, a unit test.
https://github.com/dronecore/DroneCore/blob/develop/CMakeLists.txt#L221-L230

Ok! Thanks. I'll add it.

julianoes · 2018-01-24T09:13:25Z

plugins/mission/mission_impl.cpp

+                LogWarn() << "Mission: Unsupported mission command: " << command << ". Ignoring.";
+                break;
+        }
+        // Maybe removed


Maybe what? 😄

I meant that we can remove debug log if its annoying for larger mission. :)

I understand but it's a funny comment.

JonasVautherin · 2018-01-25T07:22:47Z

CMakeLists.txt

@@ -104,6 +104,7 @@ include_directories(
    ${CMAKE_CURRENT_BINARY_DIR}/include
    ${CMAKE_CURRENT_BINARY_DIR}/core
    SYSTEM ${additional_includes}
+    SYSTEM libs/json11


When #188 gets merged, I guess json11 can become a dependency of the mission plugin only, right?

Yes, right.

hamishwillee

The new functionality is good.
This does need a unit test.
This replaces the example showing normal use of the API with use of the QGC plan. The implication is that you don't want to show people how to use the API, you only expect them to use the QGC plan.

IMO you either show both cases or you have a separate example for the qgc plan import.
Personally I would revert to the previous example (but with your excellent documentation header) and either have a standalone example, or possibly just point users to the unit test - that should be enough information.

If this is part of the example code, I'd supply a dummy plan. This will make it easier for users to play with.

shakthi-prashanth-m · 2018-01-30T06:44:34Z

@hamishwillee Thanks for your review comments.

This does need a unit test.

Yes, next commit will have it.

This replaces the example showing normal use of the API with use of the QGC plan. The implication is that you don't want to show people how to use the API, you only expect them to use the QGC plan.
IMO you either show both cases or you have a separate example for the qgc plan import.

Good observation. But we've example usage of it in integration tests. So, we don't need to show it in example.

Personally I would revert to the previous example (but with your excellent documentation header) and either have a standalone example, or possibly just point users to the unit test - that should be enough information.

Thanks. Sure, I would prefer to show Fly mission example with QGC import mission feature as it is user-friendly.

If this is part of the example code, I'd supply a dummy plan. This will make it easier for users to play with.

Plan for example code ? Please share.

hamishwillee · 2018-01-30T07:00:27Z

@shakthi-prashanth-m

Good observation. But we've example usage of it in integration tests. So, we don't need to show it in example.

Well you're going to have examples of everything in the integration tests or unit tests, so you might argue why have any examples at all. See below ....

Personally I would revert to the previous example (but with your excellent documentation header) and either have a standalone example, or possibly just point users to the unit test - that should be enough information.

Thanks. Sure, I would prefer to show Fly mission example with QGC import mission feature as it is user-friendly.

Example code should be designed to show off the most commonly used parts of the API or the tricky bits of the API. Essentially I think I can very easily work out how to use the QGC import function by inspection, but using the main API is more challenging.

If you really think that 90% of users will be working off a QGC plan then maybe, but I doubt that is the case.

I don't mind if you have both :-)

Maybe @julianoes can tie break.

If this is part of the example code, I'd supply a dummy plan. This will make it easier for users to play with.

Plan for example code ? Please share.

What I mean is that for this example the instructions would have to say "go create a plan in QGC, download it, then run our example code using this command line ...". I am proposing that you provide a pre-created QGC plan (ie not a "dummy plan", sorry for wording) so that the instructions can be something like "You can test this by using the following command line".

shakthi-prashanth-m · 2018-01-30T07:39:29Z

OK, I understand your point @hamishwillee .
I would like to summarize next steps for this PR as below:

Add unit test for import QGC mission feature which tests with a sample QGC plan.
Lets keep the existing Fly mission example as it demonstrates core Mission APIs (improving documentation header).
Add Fly QGC mission example to demonstrate popular QGC plan.

@julianoes @hamishwillee please share your opinion.

Thanks

shakthi-prashanth-m · 2018-02-04T03:56:39Z

@julianoes @hamishwillee @JonasVautherin Kindly review. Thanks.

shakthi-prashanth-m · 2018-02-04T16:19:18Z

[----------] 1 test from QGCMissionImport
[ RUN      ] QGCMissionImport.ValidateQGCMissonItems
[       OK ] QGCMissionImport.ValidateQGCMissonItems (1 ms)
[----------] 1 test from QGCMissionImport (1 ms total)

JonasVautherin · 2018-02-04T23:24:06Z

.gitmodules

@@ -13,3 +13,6 @@
 [submodule "grpc/server/proto"]
 	path = grpc/server/proto
 	url = https://github.com/dronecore/DroneCore-Proto.git
+[submodule "plugins/mission/libs/json11"]


Nice to have it inside the plugin :-).

How about putting it in third_party instead of libs? I started using third_party because that's what they use in gRPC and many of its dependencies (maybe that's a Google convention, Idk), and I find it clearer.

Nice to have it inside the plugin :-)

Yes, I placed it inside mission as it is dependency of the plugin alone.

How about putting it in third_party instead of libs? I started using third_party because that's what they use in gRPC and many of its dependencies (maybe that's a Google convention, Idk), and I find it clearer.

I see. Yeah AFAIK we have a choice: external or third_party. I'm not particular about it, but I see that external is popular. What do you think ?

I don't have a strong opinion on this but we should be consistent if possible. If it changes, then let me know because this is used in the docs.

I like third_party, firstly because there is no possible confusion for me, and also because that's the one I see in grpc, protobuf, boringssl, that are our dependencies already.

shakthi-prashanth-m · 2018-02-05T04:17:31Z

Travis CI says below error while performing Docker build for Ubuntu & Fedora.

This job ran on our Trusty environment,
which has been updated on Tuesday, December 12th, 2017.
Read all about this on our blog and take note that you can
add group: deprecated-2017Q4 in your .travis.yml file to use the previous image version.

Rest are successful. Can we ignore this ?

JonasVautherin · 2018-02-05T06:39:53Z

Travis fails on Ubuntu and Fedora because of a style error (see last lines of the log). Regarding the update warning, IMO it is not so important for us, so we can ignore it.

shakthi-prashanth-m · 2018-02-05T06:50:26Z

@JonasVautherin Yeah, its strange; when I ran this locally, there are no such style errors reported.

JonasVautherin · 2018-02-05T06:54:08Z

Does the ./fix_style.sh work locally? Maybe you have to run it manually :-).

shakthi-prashanth-m · 2018-02-05T06:56:12Z

Yes. I ran make fix_style which in-turn invokes fix_style.sh. But none reported.

julianoes

Nice, looks fairly correct. Does it run as part of the make run_unit_tests?

See various inline comments.

julianoes · 2018-02-05T08:55:44Z

core/CMakeLists.txt

@@ -107,6 +107,7 @@ if(NOT IOS AND NOT ANDROID)
    # SYSTEM because we don't want warnings for gtest headers.
    include_directories(SYSTEM libs/gtest/googletest/include)
    include_directories(SYSTEM libs/gtest/googlemock/include)
+	include_directories(${CMAKE_SOURCE_DIR}/core)


indentation

julianoes · 2018-02-05T08:55:56Z

core/CMakeLists.txt

@@ -117,7 +118,9 @@ if(NOT IOS AND NOT ANDROID)
        timeout_handler_test.cpp
        call_every_handler_test.cpp
        curl_test.cpp
+		# FIXME: Below line is NOT capturing plugin unit tests sources. So adding manually.


indentation, make sure to switch to spaces.

julianoes · 2018-02-05T08:57:00Z

example/fly_qgc_mission/fly_qgc_mission.cpp

+using namespace std::this_thread; // for sleep_for()
+
+// FIXME: Using relative path for QGC mission plan. What's the right way ?
+const std::string DEFAULT_QGC_MISSION_PLAN = "../../../plugins/mission/qgroundcontrol_sample.plan";


I would add a sample in the example folder.

julianoes · 2018-02-05T09:08:45Z

plugins/mission/mission_impl.cpp

+#endif
+
+        // Add mission item to the list
+        mission_items.push_back([&]() -> std::shared_ptr<MissionItem> const {


What bugs me a little bit is that the importer does not combine the mavlink mission items together into dronecore mission items.

So, for the optimal case, a location + speed command + gimbal command would optimally end up in the same dronecore mission item. Basically, all commands following a location should be cominbed with the location.

julianoes · 2018-02-05T09:09:13Z

plugins/mission/mission_impl.cpp

+            case MAV_CMD_NAV_WAYPOINT:
+                is_fly_through = (params[0] == 0.0) ? true : false;
+            // NO break here, because params 4, 5 & 6 of both
+            // of these commands are Lat, Lon & Alt respectively.


Add // FALLTHROUGH for GCC 7.

shakthi-prashanth-m · 2018-02-05T10:04:03Z

@julianoes Yes, it runs as part of make run_unit_tests

Previous name is too long and unnecessary. Hence renamed to keep API name simple and effective.

Rename `Mission::import_QGC_mission()` to `Mission::import_qgroundcontrol_mission()` to maintain snake_case style.

1. Moved building unit tests to main CMakeLists.txt. 2. Renamed libs to "third_party". 3. Methods used to import mission are renamed for clarity. 4. Enum for AirSpeed, GroundSpeed are explicity intialized for protability.

Move to plugins/mission/third_party/json11

As per https://git-scm.com/docs/gitmodules#_description, "name" is the name of the submodule. Hence this commit does make sure each submodule entry abides by it.

Corrected name of submodules: mavlink and dronecore-proto.

This fixes Windows build error.

Its not necessary anymore as it is done in dronecore lib itself.

Moved QGC sample path to plugins/mission and used absolute path in unit test. Absolute path is composed to work on Windows as well.

For Debug configuration, we need to copy debug versions of DLL.

QGC sample plan is moved to "plugins/mission". Removed "-g" flag in building Fly QGC example.

CI fails to find debug version of DLLs and hence reverting changes to copy release versions of DLLs even in debug configuration.

julianoes · 2018-02-15T14:27:37Z

plugins/mission/mission_import_qgc_test.cpp

@@ -11,6 +11,13 @@
 #include "global_include.h"
 #include "log.h"

+// To locate QGroundControl plan file during Unit test.
+#if defined(WIN32) || defined(_WIN32) || defined(__WIN32)


@shakthi-prashanth-m you can just use #ifdef WINDOWS it's what we already have in the build system.

julianoes · 2018-02-15T14:32:15Z

example/fly_qgc_mission/fly_qgc_mission.cpp

-    const std::string DEFAULT_QGC_MISSION_PLAN = "../qgroundcontrol_sample.plan";
-    std::string qgc_plan(DEFAULT_QGC_MISSION_PLAN);
+    // Locate path of QGC Sample plan
+    std::string qgc_plan = "../../../plugins/mission/qgroundcontrol_sample.plan";


Eventually, I would duplicate the .plan file but for now that's ok.

Used `WINDOWS` macro for consistency.

shakthi-prashanth-m · 2018-02-15T16:08:20Z

@julianoes now ready to be merged unless @hamishwillee requests for changes.

julianoes · 2018-02-15T16:09:29Z

Thanks @shakthi-prashanth-m for the continuous fixing and improvements 😄.

hamishwillee · 2018-02-15T22:11:58Z

Congratulations on getting this in @shakthi-prashanth-m ! I'll *try to get associated docs updates in next week: mavlink/MAVSDK-docs#99

shakthi-prashanth-m · 2018-02-16T08:29:39Z

Congratulations on getting this in @shakthi-prashanth-m ! I'll *try to get associated docs updates in next week: mavlink/MAVSDK-docs#99

Thanks @hamishwillee .
I would like to thank @julianoes and @JonasVautherin for the review and valuable suggestions! 👍

telemetry: remove level health check

shakthi-prashanth-m requested review from julianoes and hamishwillee January 23, 2018 10:45

JonasVautherin added the in progress label Jan 23, 2018

julianoes requested changes Jan 23, 2018

View reviewed changes

julianoes reviewed Jan 23, 2018

View reviewed changes

julianoes requested changes Jan 24, 2018

View reviewed changes

JonasVautherin reviewed Jan 25, 2018

View reviewed changes

hamishwillee requested changes Jan 29, 2018

View reviewed changes

hamishwillee mentioned this pull request Jan 30, 2018

Improve Fly mission example #232

Closed

shakthi-prashanth-m changed the title ~~Add Mission plugin capability to import mission from QGC plan~~ [WIP] Add Mission plugin capability to import mission from QGC plan Jan 30, 2018

shakthi-prashanth-m force-pushed the import-qgc-plan-mission branch 2 times, most recently from bed6160 to db1474f Compare February 4, 2018 03:52

shakthi-prashanth-m changed the title ~~[WIP] Add Mission plugin capability to import mission from QGC plan~~ Add Mission plugin capability to import mission from QGC plan Feb 4, 2018

shakthi-prashanth-m force-pushed the import-qgc-plan-mission branch from 2524158 to 095063a Compare February 4, 2018 15:52

shakthi-prashanth-m force-pushed the import-qgc-plan-mission branch from e84797e to 6466535 Compare February 4, 2018 17:58

JonasVautherin reviewed Feb 4, 2018

View reviewed changes

shakthi-prashanth-m force-pushed the import-qgc-plan-mission branch from 15c9e75 to f3054df Compare February 5, 2018 07:17

julianoes reviewed Feb 5, 2018

View reviewed changes

Shakthi Prashanth M and others added 15 commits February 15, 2018 20:02

Rename QGC importer method to import_QGC_mission()

0ffb5e3

Previous name is too long and unnecessary. Hence renamed to keep API name simple and effective.

QGC Mission Importer: Rename importer method

830ca9a

Rename `Mission::import_QGC_mission()` to `Mission::import_qgroundcontrol_mission()` to maintain snake_case style.

QGC Mission reporter: Fix review comments

2621d89

1. Moved building unit tests to main CMakeLists.txt. 2. Renamed libs to "third_party". 3. Methods used to import mission are renamed for clarity. 4. Enum for AirSpeed, GroundSpeed are explicity intialized for protability.

QGC Mission Importer: Remove old json11 path

dec1e1e

Move to plugins/mission/third_party/json11

build_system: fix json submodule

4977b50

build_system: some cleanup with unit tests

fac1743

Correct name of submodules in .gitmodules

7cc6f34

As per https://git-scm.com/docs/gitmodules#_description, "name" is the name of the submodule. Hence this commit does make sure each submodule entry abides by it.

.gitmodules: Correct name of submoduels

fc3d6c1

Corrected name of submodules: mavlink and dronecore-proto.

cmake/unit_tests: Add cURL include dir

53fe582

This fixes Windows build error.

Follow Me Example : Remove linking pthread

a827e58

Its not necessary anymore as it is done in dronecore lib itself.

appveyor: fix unit test for mission import test

4b70726

QGC Mission Importer: Use absolute path for QGC sample plan

45ab3b4

Moved QGC sample path to plugins/mission and used absolute path in unit test. Absolute path is composed to work on Windows as well.

Appveyor: Copy debug DLLs for mission and json11

d0b9140

For Debug configuration, we need to copy debug versions of DLL.

Fly QGC Mission example: Update sample plan path

c0b620d

QGC sample plan is moved to "plugins/mission". Removed "-g" flag in building Fly QGC example.

Appveyor: Revert copying debug version of DLLs

2313a3e

CI fails to find debug version of DLLs and hence reverting changes to copy release versions of DLLs even in debug configuration.

julianoes reviewed Feb 15, 2018

View reviewed changes

QGC Mission Importer: Use WINDOWS flag instead of WIN32

84c73a7

Used `WINDOWS` macro for consistency.

shakthi-prashanth-m force-pushed the import-qgc-plan-mission branch from 04b1f61 to 84c73a7 Compare February 15, 2018 14:43

julianoes merged commit f7354e3 into develop Feb 15, 2018

julianoes deleted the import-qgc-plan-mission branch February 15, 2018 16:09

julianoes removed the in progress label Feb 15, 2018

hamishwillee mentioned this pull request Feb 15, 2018

Add new docs updates for QGC Mission import mavlink/MAVSDK-docs#99

Closed

shakthi-prashanth-m mentioned this pull request Feb 16, 2018

Rename camera_action_delay_s to loiter_time_s in MissionItem class #249

Closed

hamishwillee mentioned this pull request Feb 16, 2018

[WIP] Update docs following addition of QGC plan import mavlink/MAVSDK-docs#100

Merged

3 tasks

mrpollo mentioned this pull request Jun 5, 2018

v1.0.0 Changelog scratchpad #417

Closed

dlech pushed a commit to dlech/MAVSDK that referenced this pull request Jan 14, 2022

Merge pull request mavlink#235 from mavlink/pr-telemetry-no-level

57a4a80

telemetry: remove level health check

Add QGC Mission import capability #235

Add QGC Mission import capability #235

Conversation

shakthi-prashanth-m commented Jan 23, 2018 • edited Loading

shakthi-prashanth-m commented Jan 23, 2018

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

shakthi-prashanth-m Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

shakthi-prashanth-m Jan 23, 2018 • 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

shakthi-prashanth-m Jan 23, 2018 • edited Loading

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

shakthi-prashanth-m Jan 24, 2018 • 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

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

hamishwillee left a comment • edited Loading

Choose a reason for hiding this comment

shakthi-prashanth-m commented Jan 30, 2018

hamishwillee commented Jan 30, 2018

shakthi-prashanth-m commented Jan 30, 2018 • edited Loading

shakthi-prashanth-m commented Feb 4, 2018

shakthi-prashanth-m commented Feb 4, 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

shakthi-prashanth-m commented Feb 5, 2018

JonasVautherin commented Feb 5, 2018

shakthi-prashanth-m commented Feb 5, 2018

JonasVautherin commented Feb 5, 2018

shakthi-prashanth-m commented Feb 5, 2018

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

shakthi-prashanth-m commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakthi-prashanth-m commented Feb 15, 2018

julianoes commented Feb 15, 2018

hamishwillee commented Feb 15, 2018

shakthi-prashanth-m commented Feb 16, 2018

shakthi-prashanth-m commented Jan 23, 2018 •

edited

Loading

shakthi-prashanth-m Jan 23, 2018 •

edited

Loading

shakthi-prashanth-m Jan 23, 2018 •

edited

Loading

shakthi-prashanth-m Jan 23, 2018 •

edited

Loading

shakthi-prashanth-m Jan 24, 2018 •

edited

Loading

hamishwillee left a comment •

edited

Loading

shakthi-prashanth-m commented Jan 30, 2018 •

edited

Loading