Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor FMU: move mixer handling into a library #12790

Merged
merged 8 commits into from
Aug 31, 2019

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Aug 23, 2019

Currently all actuator output drivers handle mixer loading, mixer parameters and actuator controls subscriptions with everything around that themselves, leading to duplicated code with minor differences.
This PR is a step towards improving that by pulling these parts out of fmu into a library that can be used by other modules.
It is a pure refactoring, except for:

  • removal of PX4FMU::write (9c84251). I don't see it being used anywhere.
  • mixer loading is now thread-safe (418021b)

Testing

I tested:

  • normal pwm on Pixhawk 4, including safety button
  • Pixracer, including an ESC calibration
  • oneshot on omnibus
  • mixer param changes

@Igor-Misic this serves as preparation for DShot.
I tend towards adding it as a new module (as opposed to integrate into fmu), as the fmu driver contains various things we won't need. On the other hand however, in the current form we would lose input capture pin configuration and the safety button.

@dagar one problem I found with the actuator_controls uORB scheduling is that you can end up your module not being scheduled at all. For example with an RC passthrough mixer and no RC connected. If the module wants to do other tasks as well we need to ensure it's run periodically even w/o uORB updates.

A careful review is appreciated.

@dagar
Copy link
Member

dagar commented Aug 23, 2019

@dagar one problem I found with the actuator_controls uORB scheduling is that you can end up your module not being scheduled at all. For example with an RC passthrough mixer and no RC connected. If the module wants to do other tasks as well we need to ensure it's run periodically even w/o uORB updates.

We should get those other tasks removed from fmu. #10119

perf_counter_t _control_latency_perf;

DEFINE_PARAMETERS(
(ParamInt<px4::params::MC_AIRMODE>) _param_mc_airmode, ///< multicopter air-mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think of a different way to handle this eventually. Seeing the "Multicopter Attitude Control" param group on a plane deeply bothers me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Maybe just move the param to a mixer group?

@dagar
Copy link
Member

dagar commented Aug 23, 2019

How do you see this working in cases like uavcan? I'm wondering if having the uORB subscription callbacks in mixer_module is going to be too restrictive for things like uavcan.

@bkueng
Copy link
Member Author

bkueng commented Aug 26, 2019

How do you see this working in cases like uavcan? I'm wondering if having the uORB subscription callbacks in mixer_module is going to be too restrictive for things like uavcan.

So far I see these options:

  • poll only on actuator groups 0 (or 1), and if not used/published run at a fixed rate
  • moving the subscription callbacks out of the library back into the module, but that again leads to more duplication.
  • Independently schedule MixingOutput by making it a separate work item. But this leads to more overhead and complexity (switching work queues for example), so I tend towards the first option.

@jlecoeur
Copy link
Contributor

I think this goes in the right direction, although I have not reviewed carefully.
I would tend towards the second option (i.e. to not impose callback or scheduling in the library). And later, remove the duplicated uses of this library by centralizing mixing in one module running on FMU.

@bkueng bkueng force-pushed the fmu_refactoring_output_module branch from 34cfa18 to db75c10 Compare August 27, 2019 15:10
@bkueng
Copy link
Member Author

bkueng commented Aug 27, 2019

Rebased.

I would tend towards the second option (i.e. to not impose callback or scheduling in the library). And later, remove the duplicated uses of this library by centralizing mixing in one module running on FMU.

I made it configurable now so the user of the library can decide.

@dagar
Copy link
Member

dagar commented Aug 27, 2019

  • Independently schedule MixingOutput by making it a separate work item. But this leads to more overhead and complexity (switching work queues for example), so I tend towards the first option.

One of the motivations for the PX4 WQ was to be able to afford to carve these things into separate little modules. Possibly splitting out mixing entirely from the output driver is something I'd like to consider down the road if we can wrap out heads around the current configuration mess.

@dagar
Copy link
Member

dagar commented Aug 27, 2019

@PX4/testflights could you please give this a try on one or more of the following flight controllers?

  • pixhawk 4 mini
  • cuav v5 nano
  • pixracer
  • nxphlite

Thanks!

@dagar dagar requested review from a team and jlecoeur August 27, 2019 15:51
@bkueng bkueng force-pushed the fmu_refactoring_output_module branch 2 times, most recently from f142dd7 to ab629a8 Compare August 27, 2019 18:26
@dannyfpv
Copy link

dannyfpv commented Aug 28, 2019

Tested on Pixracer v4 f-450

Modes Tested
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

log:
https://review.px4.io/plot_app?log=2f472b7e-9716-432f-849d-f429d9bc41f7

Tested on Pixhawk4 mini v5 f-450

Modes Tested
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

log:
https://review.px4.io/plot_app?log=bed1b711-4fa2-4379-aef2-6b66fe699d00

@jorge789
Copy link

Tested on NXP FMUK66 V3
Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoin activate RTL and see landing behaviour.

Notes:
No issues noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=63165b52-49d0-45af-9c80-451bdc0374cd

@jorge789
Copy link

jorge789 commented Aug 29, 2019

Tested on CUAV Nano V5:
Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Takeoff in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceeds to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint activate RTL and see landing behavior.

Notes:
The height of the vehicle and what QGC said did not coincide. In the mission, the height was 35 meters and the height of the actual vehicle was approximately 25 meters. The error was only visible in Altitude, stabilized and mission, in position the error could not be seen, position height seemed accurate.

Log:
https://review.px4.io/plot_app?log=6e6f1be3-5f78-4e7e-9281-777aa912329c

This should be a pure refactoring, no functional change.
Makes sure that the parameter is included for the builds that need it.
As there is nothing pwm-specific about it.
And make sure fmu calls MixingOutput::updateSubscriptions on startup even
if no mixer is loaded, so that it gets scheduled.
@bkueng bkueng force-pushed the fmu_refactoring_output_module branch from ab629a8 to c42c76b Compare August 30, 2019 06:14
@bkueng bkueng mentioned this pull request Aug 31, 2019
@dagar dagar merged commit f0ee0b5 into master Aug 31, 2019
@dagar dagar deleted the fmu_refactoring_output_module branch August 31, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants