Offboard example #116

shakthi-prashanth-m · 2017-10-13T13:38:22Z

Adding Offboard velocity control app.

Offboard velocity control is demonstrated using

Local NED
Body NED

Offboard velocity control is demonstrated using * Local NED * Body NED

julianoes

I made a couple of comments but I think this is a very nice example to have. Thanks!

julianoes · 2017-10-17T13:13:54Z

.gitignore

@@ -3,3 +3,8 @@
 **/install
 **/logs
 .vscode
+DroneCore.config


Can you make a comment where these are from?

ok, I'll add it. It comes from Qt Creator

julianoes · 2017-10-17T13:15:24Z

example/offboard_velocity/offboard_velocity.cpp

@@ -0,0 +1,229 @@
+//
+// Example that demonstrates Offboard velocity control using LOCAL_NED & BODY_NED


What about?
Example that demonstrates offboard velocity control in local NED and body coordinates

julianoes · 2017-10-17T13:16:47Z

example/offboard_velocity/offboard_velocity.cpp

+
+void wait_msec(int msec)
+{
+    std::this_thread::sleep_for(std::chrono::milliseconds(msec));


You could alternatively do this:

using std::this_thread::sleep_for; using std::chrono::milliseconds; sleep_for(milliseconds(100));

julianoes · 2017-10-17T13:16:57Z

example/offboard_velocity/offboard_velocity.cpp

+
+void wait_sec(int sec)
+{
+    std::this_thread::sleep_for(std::chrono::seconds(sec));


julianoes · 2017-10-17T13:18:48Z

example/offboard_velocity/offboard_velocity.cpp

+
+    // Wait for device to connect via heartbeat.
+    wait_sec(2);
+    Device &device = dc.device();


You can now use is_connected(), see https://github.com/dronecore/DroneCore/blob/develop/include/dronecore.h#L161.

julianoes · 2017-10-17T13:19:35Z

example/offboard_velocity/offboard_velocity.cpp

+            const float one_cycle = 2.0f * (float)M_PI;
+            const unsigned steps = (unsigned)(one_cycle / step_size);
+
+            std::cout << "[" << offb_mode << "]" << " Go right & oscilate" << std::endl;


oscilate -> oscillate

julianoes · 2017-10-17T13:21:38Z

example/offboard_velocity/offboard_velocity.cpp

+    wait_sec(5);
+
+    //////////////////////////////////////////////////////////////////////////////
+    if (offb_mode == "local-ned") {


What about having two functions for the two if cases. That would make it easier to read and not require /////....

It would certainly allow for more modular code

I tend to think that the two frame cases are separate, and users will tend to copy the code for the frame they are interested in using. Furthermore, what if PX4 adds additional frame support? So I would tend to have completely separate code blocks - and even only use functions if it aids the explanation.

mrpollo · 2017-10-18T01:54:42Z

@shakthi-prashanth-m can you please rebase this and push to upstream? I think Travis needs a little push

mrpollo · 2017-10-18T02:08:26Z

example/offboard_velocity/offboard_velocity.cpp

+    DroneCore dc;
+    std::string offb_mode = (argc > 1) ? argv[1] : "";
+
+    if (argc == 1 || (offb_mode != "local-ned" && offb_mode != "body-ned")) {


can we have this default to local? this would simplify the main routine, and perhaps use "body" instead of "body-ned" for the argument, just to simplify this

ok.. Do you mean when the example is launched with no arguments, it should start offboard with local NED coordinates mode ?

As of now, we're demonstrating both NED & Body co-ordinates. So, removed command-line handling.

mrpollo · 2017-10-18T02:09:04Z

example/offboard_velocity/offboard_velocity.cpp

+
+    if (argc == 1 || (offb_mode != "local-ned" && offb_mode != "body-ned")) {
+        std::cerr << "Usage: offboard <coordinate-frame>\n" <<
+                  "coordinate-frame: \"local-ned\", or \"body-ned\""


if we default to local then this would explain that we are using local by default and explain how to enable body ned

We need to report usage of the example anyway even if we default to local. So, I thought we shall leave this to user.

IMO code should show both modes at work - not an either/or choice determined by user. That way you can show off the nuances of using each approach.

@hamishwillee that is fair enough..

mrpollo · 2017-10-18T02:12:00Z

example/offboard_velocity/offboard_velocity.cpp

+    wait_sec(2);
+    Device &device = dc.device();
+
+    while (!device.telemetry().health_all_ok()) {


what about health_all_ok_async?

although this would force you to split your code even further

@mrpollo, thanks. I will rebase with changes suggested and push new changes may be tomorrow.

Disagree - this synchronous is better.

@mrpollo The code can't progress until health is achieved so if you were to do this aysnc style you'd end up using promises to effectively create a synchronous solution. The only reason to use async here would be if the code was a library potentially running in a context that could block other code (e.g. UI). There are very few cases with single-vehicle drones where using the async approach makes - because we're only every waiting on one instruction, and instructions are mostly daisychained.
It might be worth using async to manage multiple vehicles, or if we can find a case where we need code to run in parallel.

@hamishwillee: I agree. I feel its fine to use sync API. There is no vital task to perform until device.telemetry().health_all_ok() return true. @mrpollo is that ok ?

mrpollo · 2017-10-18T02:13:02Z

example/offboard_velocity/offboard_velocity.cpp

+    }
+
+    std::cout << "In Air" << std::endl;
+    wait_sec(5);


you could use [in_air_async](https://github.com/dronecore/DroneCore/blob/develop/plugins/telemetry/telemetry_impl.h#L65) instead of waiting

Generally I don't see much use for the async APIs for managing a single vehicle.

In any case though we might use this as an opportunity to show off some "comprehensive" takeoff code, as per this PR

Close app if vehicle not calibrated

Print what we're waiting on (maybe)

Check that arming occurred before calling takeoff

Check that we reached takeoff height rather than using timer. This is a bit better than in_air() because you actually know you have completed the takeoff.

@mrpollo, this is basically to let drone reach takeoff altitude before we start issuing OFFBOARD commands. I think it should be fine to sleep for 5 seconds, similar to the way done in takeoff land example.

mrpollo · 2017-10-18T02:13:46Z

example/offboard_velocity/offboard_velocity.cpp

+    wait_sec(5);
+
+    //////////////////////////////////////////////////////////////////////////////
+    if (offb_mode == "local-ned") {


It would certainly allow for more modular code

mrpollo · 2017-10-18T02:18:10Z

example/offboard_velocity/offboard_velocity.cpp

+        std::cout << "[" << offb_mode << "]" << " Go UP 2 m/s, Turn clock-wise 180 deg" << std::endl;
+        for (unsigned i = 0; i < 400; ++i) {
+            device.offboard().set_velocity_ned({0.0f, 0.0f, -2.0f, 180.0f});
+            wait_msec(10);


@julianoes I loathe waiting using timers to catch up with movement it can get unpredictable and unstable, this is something I always wanted to fix in DroneKit, we should come up with a way to either block for N time by an argument on set_velocity or find a way to have a callback

This isn't something you should worry on this PR @shakthi-prashanth-m, I'm just venting here, I would like to break this pattern in UAV development, we should be able to be more precise

Don't know what you mean by "catch up with movement".

Generally for velocity control examples using timers makes sense because they are something like "travel this far for X time". If we wanted to control on distance we'd use the position variants of the functions. If we do want to control on distance anyway, then we might add telemetry to measure that distance and block on meeting targets.

Offboard velocity control is demonstrated using * Local NED co-ordinates * Body co-ordinates

…th-m/DroneCore into offboard_example

coveralls · 2017-10-19T16:47:10Z

Coverage remained the same at 48.37% when pulling 6ea8c21 on shakthi-prashanth-m:offboard_example into f96c303 on dronecore:develop.

hamishwillee · 2017-10-19T22:40:16Z

example/offboard_velocity/offboard_velocity.cpp

+    if (result != Action::Result::SUCCESS) {
+        std::cerr << ERROR_CONSOLE_TEXT << message << Action::result_str(
+                      result) << NORMAL_CONSOLE_TEXT << std::endl;
+        exit(EXIT_FAILURE);


Just for me, where is EXIT_FAILURE defined?

hamishwillee · 2017-10-19T22:51:32Z

example/offboard_velocity/offboard_velocity.cpp

+    offboard_log(offb_mode, " Let yaw settle...");
+    for (unsigned i = 0; i < 100; ++i) {
+        device.offboard().set_velocity_ned({0.0f, 0.0f, 0.0f, 90.0f});
+        sleep_for(milliseconds(10));


@julianoes Are we expecting the API client to maintain the resend rate? Not saying we shouldn't, just that we should think about whether there is any way to make this easier for developers.
I don't think it matters too much for velocity control, but definitely for position control it would be much better if the developer could send a message "move to position" and not have to control resends.

Perhaps I am overthinking this.

I agree that we might want to handle that internally.

hamishwillee · 2017-10-19T22:52:48Z

@shakthi-prashanth-m Great job!

hamishwillee · 2017-10-19T23:26:43Z

@shakthi-prashanth-m Would be good to add the calibration checks (at least) before the health check.

shakthi-prashanth-m · 2017-10-20T14:07:05Z

@hamishwillee Thanks!
Will look into adding callbacks. Thanks for the suggestion.

shakthi-prashanth-m requested a review from julianoes October 13, 2017 13:38

shakthi-prashanth-m mentioned this pull request Oct 13, 2017

Offboard mode can lead to crash #84

Closed

Shakthi Prashanth M added 2 commits October 17, 2017 17:57

example: Added Offboard velocity control app.

e7725b8

Offboard velocity control is demonstrated using * Local NED * Body NED

example: Added Offboard velocity control app.

44a5c8c

Offboard velocity control is demonstrated using * Local NED * Body NED

julianoes requested changes Oct 17, 2017

View reviewed changes

mrpollo reviewed Oct 18, 2017

View reviewed changes

Shakthi Prashanth M added 5 commits October 19, 2017 19:34

example: Added Offboard velocity control app.

f13f02c

Offboard velocity control is demonstrated using * Local NED co-ordinates * Body co-ordinates

Merge branch 'offboard_example' of https://github.com/shakthi-prashan…

133b936

…th-m/DroneCore into offboard_example

Merge branch 'develop' into offboard_example

368a625

Merge branch 'offboard_example' of https://github.com/shakthi-prashan…

f9632fc

…th-m/DroneCore into offboard_example

Merge branch 'offboard_example' of https://github.com/shakthi-prashan…

6ea8c21

…th-m/DroneCore into offboard_example

julianoes approved these changes Oct 19, 2017

View reviewed changes

hamishwillee reviewed Oct 19, 2017

View reviewed changes

julianoes merged commit ce714f5 into mavlink:develop Oct 20, 2017

		@@ -0,0 +1,229 @@
		//
		// Example that demonstrates Offboard velocity control using LOCAL_NED & BODY_NED

Offboard example #116

Offboard example #116

Conversation

shakthi-prashanth-m commented Oct 13, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrpollo commented Oct 18, 2017

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

coveralls commented Oct 19, 2017

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 commented Oct 19, 2017

hamishwillee commented Oct 19, 2017

shakthi-prashanth-m commented Oct 20, 2017