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

[control-allocation] parameterize yaw margin #22776

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

somebody-once-told-me
Copy link
Contributor

Solved Problem

There is a hard-coded magic-number yaw margin parameter in ControlAllocationDesaturation. Users might not be aware that the flight controller will give up a maximum of 15% of collective thrust for yaw when saturated.

Solution

This parameterizes and documents the yaw margin value.

Alternatives

We could also simply centralize and document the yaw margin as a static constexpr class member, instead of a user-facing parameter. This would require new builds when modifying the yaw margin value.

Two potential problems with the current change:

  1. Addition of a public member getter variable only for unit testing (possible bad style).
  2. Possible problems from usage of parameter in control loop?

Test coverage

Updated the ControlAllocationDesaturationTest to utilize the new parameter.

Copy link
Member

@bresch bresch left a comment

Choose a reason for hiding this comment

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

Looks good to me.
However, should we maybe set that one as a percentage?
if yes, you can add the tag * @unit % just above * @min 0.0, set the default to 15, max to 100 (or maybe a lower safe value? e.g.: 30%) and so on.

BTW, could you remove the diff in platforms/nuttx/NuttX/nuttx ?

@somebody-once-told-me somebody-once-told-me marked this pull request as ready for review March 9, 2024 23:53
bresch
bresch previously approved these changes Mar 10, 2024
@somebody-once-told-me
Copy link
Contributor Author

@MaEtUgR is there anything else i can do to help with this PR?

MaEtUgR
MaEtUgR previously approved these changes Mar 25, 2024
Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

The pr looks correct yes.
@somebody-once-told-me It'd be really valuable to know for what use/problem case you want to have this configurable compared to having a different value the "default". I assume most people will neither understand nor touch this parameter and hence it might be good to solve the problem you have either in a different way or by documenting what use case this configuration helps with.

Related to this I'm working on something I call limited airmode to essentially replace minimum thrust with a configurable authority for airmode: #22706 Maybe your issue can be solved with this as well.

src/lib/mixer_module/params.c Show resolved Hide resolved
@somebody-once-told-me
Copy link
Contributor Author

@MaEtUgR Thanks for the review! I do not quite understand your PR yet, but I would definitely defer to that if this PR is merely a subset of yours, functionally. This PR is very specific and my use case is to simply set it to 0 (do not sacrifice altitude (e.g. drop) for yaw authority). If you feel strongly this is not going to provide any value apart from your change, I can close this and wait for yours. Do you happen to have an approximate ETA?

@somebody-once-told-me somebody-once-told-me changed the base branch from main to release/1.15 November 19, 2024 23:25
@somebody-once-told-me somebody-once-told-me changed the base branch from release/1.15 to main November 19, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

Successfully merging this pull request may close these issues.

4 participants