Added SetAttitudeTarget message to the off board implementation #621

salauddinaliahmed · 2018-12-09T21:33:04Z

Added a new message in the offboard plugin implementation.
Added an integration test example showcasing roll, pitch, yaw and thrust values. (offboard_attitude.cpp)

Please review and let me know incase it needs further improvements.
Thank you!

JonasVautherin

Thanks a lot for the contribution! I reviewed mainly the style, I'll wait for a comment from @julianoes regarding the actual feature.

BTW, all the .DS_Store files should be removed from this PR.

cmake/compiler_flags.cmake

plugins/offboard/include/plugins/offboard/offboard.h

plugins/offboard/offboard.cpp

plugins/offboard/offboard_impl.cpp

plugins/offboard/offboard_impl.h

plugins/offboard/offboard_impl.cpp

julianoes

Awesome, thank you! This looks correct and very clean. I would change the naming a bit because we might also add attitude control later, so we should call this attitude rate, and we don't need to use the confusing "target" naming from MAVLink.

julianoes · 2018-12-10T09:24:17Z

plugins/offboard/include/plugins/offboard/offboard.h

+     * @brief Type for Attitude rate commands in body coordinates (roll, pitch, yaw  angular
+     * rate and Thrust).
+     */
+    struct SetAttitude {


I would call this AttitudeRate.

julianoes · 2018-12-10T09:25:29Z

plugins/offboard/include/plugins/offboard/offboard.h

+        float roll_deg_s;
+        float pitch_deg_s;
+        float yaw_deg_s;
+        float thrust_value;


We we'll need comments for every field here because it's the public header file. This is used for the auto-generated reference docs.

julianoes · 2018-12-10T09:25:47Z

plugins/offboard/include/plugins/offboard/offboard.h

@@ -153,6 +164,13 @@ class Offboard : public PluginBase {
     */
    void set_velocity_body(VelocityBodyYawspeed velocity_body_yawspeed);

+    /**
+     * @brief Set the attitude target in terms of pitch, roll and yaw angular rate along with thrust in percentage.


attitude -> attitude rate

julianoes · 2018-12-10T09:26:26Z

plugins/offboard/include/plugins/offboard/offboard.h

+     *
+     * @param set_attitude_body pitch, roll and yaw angular rate along with Thrust in percentage `struct`.
+     */
+    void set_attitude_target(SetAttitude set_attitude);


set_attitude_target -> set_attitude_rate I'd say.

julianoes · 2018-12-10T09:28:52Z

plugins/offboard/offboard_impl.cpp

+        body_roll_rate, 
+        body_pitch_rate, 
+        body_yaw_rate, 
+        thrust);


Looks all correct.

julianoes · 2018-12-10T09:29:34Z

plugins/offboard/offboard_impl.h

@@ -46,10 +48,11 @@ class OffboardImpl : public PluginImplBase {
    void stop_sending_setpoints();

    mutable std::mutex _mutex{};
-    enum class Mode { NOT_ACTIVE, VELOCITY_NED, VELOCITY_BODY } _mode = Mode::NOT_ACTIVE;
+    enum class Mode { NOT_ACTIVE, VELOCITY_NED, VELOCITY_BODY, ATTITUDE_TARGET} _mode = Mode::NOT_ACTIVE;


I would call it ATTITUDE_RATE because there will also be an ATTITUDE.

julianoes · 2018-12-10T15:19:00Z

integration_tests/offboard_attitude.cpp

+    offboard->set_attitude_rate({0.0f, 0.0f, 0.0f, 0.6f});
+    std::this_thread::sleep_for(std::chrono::seconds(2));
+    // Roll for 45 degrees.
+    std::cout << "Rolling for -45 degrees" << std::endl;


As discussed, open-loop control like this won't work properly. We need to close the loop.

JonasVautherin

Thanks for all the changes! Looking good! There is still one .DS_Store to remove, and then I'll be fine with it 😊

JonasVautherin

Thanks for the changes, LGTM! Now you only need to convince @julianoes ;-)

salauddinaliahmed · 2018-12-11T17:30:17Z

I have added a closing loop logic to my integration test example (offboard_attitude.cpp) as discussed.

julianoes · 2018-12-11T18:00:05Z

integration_tests/offboard_attitude.cpp

+    double thrust_control = 0.1 * (double)(10.5f - telemetry->position().relative_altitude_m);
+    if (thrust_control < 0.1)
+    {
+        thrust_control = 0.35;


Are you sure this is correct? This means thrust_control will jump from 0.1 to 0.35 when < 0.1?

That is to prevent against negative thrust. Because, the quad would then free fall.
I am trying to stay at an altitude of <=10 meters.
If I go beyond 10mts due to any action, then the thrust becomes negative and the props stop. Causing a free fall. (0.3 was like a sweet spot which helps in lowering the altitude)

julianoes · 2018-12-11T18:01:59Z

integration_tests/offboard_attitude.cpp

+         {
+             auto yaw_bal = close_loop(0.0f,0.0f,0.0f,telemetry);
+             offboard->set_attitude_rate({yaw_bal.roll, yaw_bal.pitch, yaw_bal.yaw, yaw_bal.thrust});
+             std::this_thread::sleep_for(std::chrono::milliseconds(10));


It would be nicer to use the callback for new attitude updates instead of sleeping here but we can do that in a future pull request.

Sure, I will work on that later (mostly by next week)

julianoes · 2018-12-13T09:13:45Z

@salauddinaliahmed ok I've tested this and it seems to work, or at least do something 😄.

Now I suggest you do the following and then we can merge this:

Merge with latest develop:

git fetch origin
git merge origin/develop

Fix style issues:
```
make fix_style
```

Fix the discontinuance in throttle, I think this would make sense:

    if (thrust_control < 0.1) {
        thrust_control = 0.1;
    }

julianoes · 2018-12-13T19:56:43Z

I messed up here with force-pushing, but the changes are now in #627.

Added SetAttitudeTarget message to the off board implementation (same as #621)

JonasVautherin requested changes Dec 9, 2018

View reviewed changes

JonasVautherin reviewed Dec 9, 2018

View reviewed changes

plugins/offboard/offboard_impl.cpp Outdated Show resolved Hide resolved

julianoes requested changes Dec 10, 2018

View reviewed changes

julianoes previously approved these changes Dec 10, 2018

View reviewed changes

salauddinaliahmed dismissed julianoes’s stale review via a121fd6 December 10, 2018 14:49

julianoes requested changes Dec 10, 2018

View reviewed changes

JonasVautherin requested changes Dec 10, 2018

View reviewed changes

JonasVautherin previously approved these changes Dec 11, 2018

View reviewed changes

salauddinaliahmed dismissed JonasVautherin’s stale review via 4e8e78d December 11, 2018 17:25

julianoes reviewed Dec 11, 2018

View reviewed changes

julianoes closed this Dec 13, 2018

julianoes mentioned this pull request Dec 13, 2018

Added SetAttitudeTarget message to the off board implementation (same as #621) #627

Merged

julianoes added a commit that referenced this pull request Dec 14, 2018

Merge pull request #627 from Dronecode/pr_621

00d0ea2

Added SetAttitudeTarget message to the off board implementation (same as #621)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SetAttitudeTarget message to the off board implementation #621

Added SetAttitudeTarget message to the off board implementation #621

salauddinaliahmed commented Dec 9, 2018

JonasVautherin left a comment

julianoes left a comment

julianoes Dec 10, 2018

julianoes Dec 10, 2018

julianoes Dec 10, 2018

julianoes Dec 10, 2018

julianoes Dec 10, 2018

julianoes Dec 10, 2018

julianoes Dec 10, 2018

JonasVautherin left a comment

JonasVautherin left a comment

salauddinaliahmed commented Dec 11, 2018

julianoes Dec 11, 2018

salauddinaliahmed Dec 11, 2018 •

edited

Loading

julianoes Dec 11, 2018

salauddinaliahmed Dec 11, 2018

julianoes commented Dec 13, 2018 •

edited

Loading

julianoes commented Dec 13, 2018

Added SetAttitudeTarget message to the off board implementation #621

Added SetAttitudeTarget message to the off board implementation #621

Conversation

salauddinaliahmed commented Dec 9, 2018

JonasVautherin left a 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

JonasVautherin left a comment

Choose a reason for hiding this comment

JonasVautherin left a comment

Choose a reason for hiding this comment

salauddinaliahmed commented Dec 11, 2018

Choose a reason for hiding this comment

salauddinaliahmed Dec 11, 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 commented Dec 13, 2018 • edited Loading

julianoes commented Dec 13, 2018

salauddinaliahmed Dec 11, 2018 •

edited

Loading

julianoes commented Dec 13, 2018 •

edited

Loading