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

APM_Control: AP_RollContoroller: move rate limit #11237

Merged
merged 1 commit into from
May 6, 2019

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented May 1, 2019

this moves the rate limiting from _get_rate_out up to get_servo_out. This brings it inline with the pitch controller and removes the rate limit with using get_rate_out in acro mode.

fix for: #9192

@IamPete1 IamPete1 changed the title APM_Control: AP_RollContorller: move rate limit APM_Control: AP_RollContoroller: move rate limit May 1, 2019
@WickedShell
Copy link
Contributor

I'm not a control guy, but doesn't this open you up to large spikes being driven into the integrator that can take quite awhile to remove later?

@IamPete1
Copy link
Member Author

IamPete1 commented May 5, 2019

I'm not sure I follow, functionality should be the same, except when get_rate_out is used directly in acro mode. I that case the max rate is set by the acro parameter. There were complaints that acro mode was still limited by the max rate and the acro rate param was ignored.

@WickedShell
Copy link
Contributor

This is removing the limit on the error calculation, which is used to update the integrator. This means a single large roll change (say large roll left to large roll right) can create a very large error that gets put into the integrator, and doesn't have an effect on the actual vehicle control, as the P or FF term may be sufficient to saturate it). As I said though I'm not a controls person, so I may be misunderstanding the problem, or the behavior I've described is actually good.

@IamPete1
Copy link
Member Author

IamPete1 commented May 5, 2019

it moves the limit from _get_rate_out to get_servo_out all modes except acro use get_servo_out so should be unaffected.

@WickedShell
Copy link
Contributor

... I hadn't opened quite enough context on get_servo_out and had forgotten that it is just piping to get_rate_out sorry, my objections make no sense :)

@tridge
Copy link
Contributor

tridge commented May 6, 2019

I'm not convinced that disabling the rate limiter in ACRO mode is the right thing to do. I would support having a way to set the acro rate limit separately though. Maybe we should have an optional parameter which is a rate limit for get_rate_out()?

@tridge
Copy link
Contributor

tridge commented May 6, 2019

hmm, I guess the acro rate is already a parameter ...
ok, I'm convinced!

@tridge tridge merged commit 505e1d8 into ArduPilot:master May 6, 2019
@IamPete1 IamPete1 deleted the acro_rate_fix branch May 6, 2019 09:42
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.

None yet

3 participants