Modularize Attitude Control #11308
Modularize Attitude Control #11308
Conversation
MaEtUgR
force-pushed
the
modular-attitude-control
branch
from
53926b2 to
a35b36a
Compare
|
I made a quick and dirty test setup with gtest inspired by DronecodeSDK testing and the first even so trivial corner case unit test I write fails: PS: No worries about the failing test, I analyzed the cause and it only happens if you set roll and pitch gain both to zero. I'll fix it. |
|
I'd be interested in seeing the unit test integration. The one thing I really wanted to figure out is unit testing we can also run on the actual embedded hardware. So far I've failed with gtest (platform and c++ issues), but I think Catch2 might be workable. https://github.com/catchorg/Catch2 |
|
@dagar I looked into Catch2 yesterday. For the basic tests it's more simple to include buuut on the long run I fear it might strike us back because it has far less customizability than gtest and also e.g. no built in mocking. It could end up with fast setup for embedded and a lot more work later. |
|
Devcall: Consent unit tests are necessary and valuable and should be integrated in the code base such that tests can be added with really low effort. Major concern we also need unit testing on hardware. I start the discussion about that with @dagar anyone who wants to join please write. I'll should probably create an issue for that whole topic. |
|
@dagar My private hacking branch is here: master...MaEtUgR:gtest but I can not yet encourage anyone to look at it otherwise I would have pushed it here. I found this interesting about running gtest on hardware: https://stackoverflow.com/questions/38427998/google-test-on-bare-metal-stm32-mcu |
MaEtUgR
force-pushed
the
modular-attitude-control
branch
3 times, most recently
from
0774449 to
1a022f3
Compare
MaEtUgR
changed the title
MaEtUgR
changed the title
MaEtUgR
force-pushed
the
modular-attitude-control
branch
2 times, most recently
from
beff235 to
453ce04
Compare
MaEtUgR
changed the title
|
@dagar ok, now that part is done and I'll follow up with the unit test pr. |
to modularize and simplify unit testing
MaEtUgR
force-pushed
the
modular-attitude-control
branch
from
2bcfa47 to
16cb44b
Compare
- Remove unnecessary in between rate limits member vectors. - Only switch the yaw rate limit in auto modes, other values stay the same anyways. - Fill gain vectors with parameters in one line.
MaEtUgR
force-pushed
the
modular-attitude-control
branch
from
16cb44b to
4556184
Compare
Put adaption into a method because it needs to be called when the control mode or the parameter changes.
|
Is this good to go? |
|
Devcall: No objections for merging even before the release since it's just refactoring to enable unit testing. But we need hardware tests to make sure there is no problem with CPU and Memory on the microcontroller. So we add it to the flight test teams list and if possible do a HITL test. @PX4/testflights A small basic multicopter test on different flight control hardware covers this. |
|
Tested on Pixhawk v2 Cube; No issues were noted, good flight, in general, tested in Position, Altitude, Stabilized, Mission and RTL. https://review.px4.io/plot_app?log=7dccc216-aa4b-4d6e-9793-a2172d9461c1 |
|
Tested on Pixhawk 4 v5; no issues were noted, good flight in general. |
|
Thanks team for all the testing!! Mergeable @dagar ? |
| @@ -58,6 +58,8 @@ | |||
| #include <uORB/topics/vehicle_land_detected.h> | |||
| #include <uORB/topics/landing_gear.h> | |||
|
|
|||
| #include <AttitudeControl.hpp> | |||
There was a problem hiding this comment.
Within a module, I somewhat prefer relative includes, which makes it easier to see where the file is and reduces potential for conflicts:
| #include <AttitudeControl.hpp> | |
| #include "AttitudeControl/AttitudeControl.hpp" |
There was a problem hiding this comment.
Thanks for the quick reviewing! I totally see your point if the class is closely tied to the module and lives within its CMake target. But the whole point of this pr is to modularize the controller into a reusable self-contained library (https://github.com/PX4/Firmware/pull/11308/files#diff-66b6bc5cf0e74d3ebb07ebb30d50ade2R34) that is included and linked by CMake via the separate AttitudeControl library target (https://github.com/PX4/Firmware/pull/11308/files#diff-59b5eaaeffc3071a322da5225c7e3706R48). This makes it file location independent as long as the subdirectory is recursively added (https://github.com/PX4/Firmware/pull/11308/files#diff-59b5eaaeffc3071a322da5225c7e3706R34). If the file location is a problem we can already move it to the libraries directory but I thought I leave it in the mc_att_control folder for now since this is the first step only refactoring the existing functionality. I wanted to separate this step from introduction of testing, extending and reusing to make overseeable prs which are easy to review. (Testing draft here: #11573)
| @@ -145,6 +147,10 @@ class MulticopterAttitudeControl : public ModuleBase<MulticopterAttitudeControl> | |||
| */ | |||
| matrix::Vector3f pid_attenuations(float tpa_breakpoint, float tpa_rate); | |||
|
|
|||
| /** lower yawspeed limit in auto modes because we expect yaw steps */ | |||
| void adapt_auto_yaw_rate_limit(); | |||
There was a problem hiding this comment.
That's okay for now, but this changes the closed-loop response.
Let's take the extreme case where the user tightly tuned the yaw P gain to its maximum and is just stable; changing the saturation in auto mode will lead to yaw oscillations or even instability.
I made some modifications in the FlightTask to limit the rate of change of yaw setpoint such that we don't need to do that nasty thing here, I'll try to upstream that really soon.
There was a problem hiding this comment.
Good point, I totally agree. We definitely need to change that.
- This is refactoring only
- I made the function such that the logic only gets called in the event mode or parameter changes and not in every loop iteration, see review comment https://github.com/PX4/Firmware/pull/11308/files#discussion_r261836721
- I changed it to only switch the yaw rate limit because the rest was just confusing overhead
- The attitude controller currently can produce wobbles in the yaw movement when commanding big steps depending on the tuning gins which currently change the yaw weight (https://github.com/PX4/Firmware/pull/11308/files#diff-c7e26e8b748c35ed8718b27314fa807bR51). This stayed as it is because I'd like to have testing in place first (Enable convenient unit testing using googletest on POSIX #11573) and then change it according to my findings while experimenting with the prototyping script https://github.com/Auterion/Flight_Control_Prototyping_Scripts/tree/2eef448fb589a1c694e0717b5835644acf8d69dd/quaternion_attitude_control
|
Thanks for the reviews. As I wrote this is the refactoring step only. I'll follow up with testing and changes to the behavior as soon as this is merged and I consider it safe now. |
Describe problem solved by the proposed pull request
Like discussed with @dagar separating core control calculations from organizational logic like task management with clear interfaces is desired for code readability, simple automatic unit testing, reusability (e.g. fixed wing attitude) and replaceability.
So I'm starting here with the example for the attitude control logic which I would know very well how to unit test because I did a lot of testing with hardcoded values before pushing it here: #8003 I had to get rid of all my test because there was no simple way to contribute them as well. My first goal is to enable them.
Describe your preferred solution
All attitude control calculations are separated into the
AttitudeControlclass which can then have its unit test set that allows for automatic evaluation. This class should also be more readable due to clear members and interfaces, is hence easier to extend and improve further and could also be reused for fixed wing attitude control.