Add actuator control to offboard plugin

[irsdkv]

Implementation of set_actuator_control method due to #771 (review).

I think that this solution most corresponds to mavlink message specification compared first PR.

Also in protobuf3 "0" is the default for numeric types so this would hide the complexity.

The corresponding MAVSDK-Proto PR is here.

[irsdkv]

Hi @julianoes

I tried to implement your suggestion.
Also I tried to avoid unnecessary load on the FMU by passing the length of the array in MAVSDK API method and avoid to sent unnecessary message for Group 1.

If I misunderstand something I will be grateful if you will explain it.
This feature is important for us and also we want to implement really good solution for community :)

Thank you!

[julianoes]

@irsdkv what do you think about this suggestion? #771 (comment)

So the API would be:

std::array<float, 16> actuators = { 0.1, 0.2, 0.3, 0.4, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0 };

and if the last 8 entries are zero, we only send the message to control group 0, and we don't need to send the message to control group 1.

Would that work?

[irsdkv]

and if the last 8 entries are zero, we only send the message to control group 0, and we don't need to send the message to control group 1.

Unfortunately, due to px4 uORB subscription subsystem, if we need to fill all controls to zero - we should send all-zero message. If we don't do this then actuator_controls_1 subscribers will not receive zeroed controls and the actual control values will still remain the previously received (non-zero) values.

[julianoes]

Ok, what about NAN?

[irsdkv]

Ok, what about NAN?

Hm. I think that this can be a possible solution

[irsdkv]

Perhaps this description can be more understandable :)

[julianoes]

😄 what about:

Up to 16 actuator controls can be set. To ignore an output, set it to NaN.
The first 8 actuator controls internally map to control group 0, the latter 8 actuator controls map to control group 1.
Depending on what controls are set (instead of NaN) 1 or 2 MAVLink messages are actually sent.

Given that description is a bit awkward, would it be clearer, like that:

struct ActuatorControl {
    struct Group {
        float controls[8];
    };
    Group groups[2];
}

(Or is that almost what you had suggested in the beginning? Sorry for the long discussion)

[irsdkv]

All ok, I understand that you just want to construct a good solution :)

struct ActuatorControl {
    struct Group {
        float controls[8];
    };
    Group groups[2];
}

This is the possible solution, but In this case, in my assumption we need to change ActuatorControl structure in Proto too due to API "consistency". It can be like that:

message ControlGroup {
  repeated float controls = 1;
}
message ActuatorControl {
  repeated ControlGroup control_group = 1;
}

But this will be too comlpicated?

---

If you just don't want to have field like group_number in data types then I have this proposal:

struct ActuatorControl {
    struct Group {
        float controls[8];
    };
    union {
        Group groups[2];
        float controls[16];
    };
}

In this case current protobuf message can be used.
Also description of usage can be like that:

Up to 16 actuator controls can be set. To ignore an **output group**, set it to NaN. 
In case of **all** controls in the group is NaN than this group will not be sent (otherwise all NaN controls will sent as zero).
The first 8 actuator controls internally map to control group 0, the latter 8 actuator controls map to control group 1.
Depending on what controls are set (instead of NaN) 1 or 2 MAVLink messages are actually sent. 

What do you think about it?

[julianoes]

message ControlGroup {
 repeated float controls = 1;
}
message ActuatorControl {
 repeated ControlGroup control_group = 1;
}

But this will be too comlpicated?

I'd say that's fine.

[irsdkv]

Okay, I will implement it.
In this case the union is not required.

[irsdkv]

Done

[julianoes]

Thanks, mostly ok, I think.

[julianoes]

Great thanks for the continuous changes!

[irsdkv]

It's a pleasure. Thank you too 😄

[julianoes]

CI complains about std::isnan:

/home/user/DronecodeSDK/src/plugins/offboard/offboard.cpp:92:18: error: '__builtin_isnan' is not a member of 'std'
             if ((std::isnan(lhs.groups[i].controls[j]) && std::isnan(rhs.groups[i].controls[j])) ||
                  ^

Try adding #include <cmath> in offboard.cpp.

[julianoes]

Looks like the grpc service doesn't quite compile (anymore):
https://travis-ci.org/mavlink/MAVSDK/jobs/556753980#L7412

[irsdkv]

Looks like the grpc service doesn't quite compile (anymore):
https://travis-ci.org/mavlink/MAVSDK/jobs/556753980#L7412

My bad. Seems like just need to update proto submodule rev

[julianoes]

I think now you only need:

./tools/fix_style.sh .

[irsdkv]

Done

[JonasVautherin]

@irsdkv Thanks for all the work! We recently fixed the clang-format issues, that should be fine now. Sorry for the inconvenience there 😅.

Can you try fix_style again, from latest develop? Just want to make sure it works for you as well. If not I'll have to investigate.

[irsdkv]

I think all works ok :)

[JonasVautherin]

Hmm somehow it seems like some styling was still wrong. I pushed a fix, let's hope it works this time :-).

[irsdkv]

Hmm somehow it seems like some styling was still wrong. I pushed a fix, let's hope it works this time :-).

Oh, sorry. It was my changes. I did it because I thought it was more readable and similarly to other functions like this. I apologize for not telling you this.

P.S. Whew. Seems that this pull request has a hard fate :)

[JonasVautherin]

Oh, you're missing some docs (see Jenkins):

Parsing file /tmp/jenkins/workspace/mavlink_MAVSDK_PR-782/tools/docs/install/include/mavsdk/plugins/camera/came/tmp/jenkins/workspace/mavlink_MAVSDK_PR-782/tools/docs/install/include/mavsdk/plugins/offboard/offboard.h:151: warning: Compound mavsdk::Offboard::ActuatorControl::Group is not documented.

Please check doxygen warnings.

Good news is that this is the last CI test 😆

[JonasVautherin]

Argh, somehow having issues with Jenkins. Trying to fix that.

[julianoes]

Thanks again.