-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
FW Rate Controller: allow to enable/disable yaw axis in Acro #21566
Conversation
Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
…axes in Acro Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should generally work but I have the following open questions:
- Wasn't the discussion about enabling/disabling just the integrator on the yaw axis or is this something else?
- I'd either make the rate controller aware that an axis is disabled and invoke it the same way or have the fixed-wing controller handle it all and then not put the enum in the rate controller. The build currently fails because of the mixture.
- I see some possible low-hanging readability improvements:
- isn't torque stick input just a feed-forward?
- _airspeed scaling doesn't need to be multiplied to feed-forward and control output separately?
- couldn't more of it be calculated in a vector?
Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
The original discussion was indeed only about the integrator, but I never liked it as the P and D part of the controller are just as mis-placed in a Acro Yaw controller as the I.
True, I now opted for handling it in the FW rate controller and not in the rate controller library. If there is a benefit for enabling setting each axis on/off also for other cases (eg helicopter) then we could push the logic into the library.
Please elaborate, I don't quite get you!
On the PID it's applied squared, on the FF not.
Yeah probably, though now with the extra logic per axis it would get a bit messy again, no? Beside that I've addressed your points @MaEtUgR, please have a look again! |
…itch Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
As discussed in the fixed-wing call I reverted part of my changes again, and now only allow to disable/enable the yaw axis in Fixed-wing Acro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do simplifications later 👍
* RateControl: rename setGains to setPidGains to be more precise Signed-off-by: Silvan Fuhrer <silvan@auterion.com> * FW Rate controller: only allow to disable Yaw in Acro, not roll and pitch Signed-off-by: Silvan Fuhrer <silvan@auterion.com> --------- Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
* RateControl: rename setGains to setPidGains to be more precise Signed-off-by: Silvan Fuhrer <silvan@auterion.com> * FW Rate controller: only allow to disable Yaw in Acro, not roll and pitch Signed-off-by: Silvan Fuhrer <silvan@auterion.com> --------- Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
Solved Problem
Alternative to #20486 to disable yaw in Acro by default, but keep the option to enable it.
Solution
Add param FW_ACRO_AXS_CTRL with bitmask for roll/pitch/yaw. If the corresponding bit is not set, then do not take output of rate controller but set torque_setpoint manually with manual_control_setpoint. Reset the integrator of the disabled axes.
Changelog Entry
For release notes:
Alternatives
#20486 (only reset integrator, but keep P, D and FF action).
Test coverage
SITL tested.