fw_att_control move to ModuleBase

[dagar]

No description provided.

[tstastny]

Cool, this does fix the subscription problem. But still does not fix the subtraction of the unsigned integers in L889.

E.g. see below a debug print (where "diff=airsp ts - time now"):
WARN [fw_att_control] airsp ts 15448699
WARN [fw_att_control] time now 15455263
WARN [fw_att_control] diff 18446744073709544825
WARN [fw_att_control] telapsed 7183
WARN [fw_att_control] valid? 0

However, using the hrt_elapsed_time you recommended in #8898 works.
WARN [fw_att_control] airsp ts 22961674
WARN [fw_att_control] time now 22969602
WARN [fw_att_control] diff 18446744073709543405
WARN [fw_att_control] telapsed 8560
WARN [fw_att_control] valid? 1

This amounts only to this change:

const bool airspeed_valid = PX4_ISFINITE(_sub_airspeed.get().indicated_airspeed_m_s) && (hrt_elapsed_time(&_sub_airspeed.get().timestamp) < 1e6);

@dagar can you do this? Then I think this PR is good to merge -- I just tested on hardware and it works out.

[dagar]

@tstastny I fixed the time check and also slipped in some trivial cleanup I've wanted to do for readability. I've tested on the bench and in SITL.

[dagar]

08b4d89 fixes attitude setpoint logging & display on flight review for stabilized mode.

[dagar]

Random mode testing https://logs.px4.io/plot_app?log=f3675edc-b8a8-45b6-aeb3-824ad4894936

[dagar]

@RomanBapst quick review? Nothing too interesting here should be different, I'm mainly trying to get this module organized.

[RomanBapst]

@dagar Sure, first thing in the morning.

[RomanBapst]

@dagar Something unrelated to your changes: Do we really need these checks here ?
Just wondering during which case we would not want to publish actuator outputs.

[RomanBapst]

@dagar Looks pretty good. We could a second pass and update some of the comments in the module.

[RomanBapst]

@dagar Any reason why the airspeed subscription is handled using the subscription class and not the others?

[acfloria]

I would say that within a module we should only use one type of subscription. So we either use the subscription class also for all other inputs of the fixed-wing attitude control module or use the same syntax for the airspeed as for the other subscribers.

[dagar]

In general I agree, but I'd like to strongly encourage moving away from the old C APIs for both params and orb. There's no type safety and most common usage throughout the code base doesn't check any errors.

[RomanBapst]

@dagar What does "final" mean?

[dagar]

That this class can't be derived from. In some cases it allows the compiler to devirtualize calls, but it's otherwise harmless.

[RomanBapst]

@dagar I guess the next thing would be a couple of flight tests in all possible modes?

[dagar]

@dagar Something unrelated to your changes: Do we really need these checks here ?Just wondering during which case we would not want to publish actuator outputs.

No, but I was going to do another pass later that continued splitting out the main loop into separate functions for attitude control and rate control (partially to enable rate setpoint subscriptions). We can continue removing all the extraneous checks as we go.

I was also tempted to completely pull out manual handling and finally create stick_mapper, but the rattitude mode is problematic.

[r0gelion]

@dagar we plan to go to the flying field tomorrow and test this PR if the weather allows it.

Regards.

[dannyfpv]

Perfect flight performance on this flights:
https://review.px4.io/plot_app?log=ff408fb9-e829-4db4-ab53-c0cbfb9764c6
https://review.px4.io/plot_app?log=3005515b-a35e-4cae-8dd2-2fa3176e0c8d

[dagar]

Thanks, looks good.

@dannyfpv @r0gelion You should reset FW_R_RMAX to default.

It's the reason why roll is spiking to max like this.

[dagar]

I made one minor change to fix the vehicle_attitude_setpoint publication (which is also wrong in master). https://logs.px4.io/plot_app?log=cee40f51-3e92-4700-ba6e-1bd2ad0de2da