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

Add PID object PD limit #24567

Merged
merged 12 commits into from
Sep 26, 2023
Merged

Add PID object PD limit #24567

merged 12 commits into from
Sep 26, 2023

Conversation

lthall
Copy link
Contributor

@lthall lthall commented Aug 10, 2023

This feature lets the user limit the maximum PD output of the PID object. This is useful in large aircraft or systems where the actuators are slew limited.

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

The constrain is "invisible" because the pid_info object is not updated, this will make log review very difficult.

Its a little hard to say the best way to over come this. We could work out the ratio and decrease both P and D as logged. EG:

if (is_positive(_kpdmax)) {
    const float PD_scale = _kpdmax / MAX(fabsf(P_out + D_out), _kpdmax);
    P_out *= PD_scale;
    D_out *= PD_scale;
}

However doing it that way may still not make it clear as P and D will not "flat line" individually.

I think we would need that and a new limit flag. We can do a limit bitmask and in the existing uint8_t limit field so the log message size will not actually get bigger, although the pid_info struct will.

@xianglunkai
Copy link
Contributor

@lthall
I doubt the practical value of this change because based on my experience in autonomous driving, there are position, speed, and even acceleration limitations on the controller output, which is sufficient. Of course, the integration of individual saturation constraints is necessary. Perhaps my understanding is biased, thank you!

@lthall
Copy link
Contributor Author

lthall commented Aug 14, 2023

I think we would need that and a new limit flag. We can do a limit bitmask and in the existing uint8_t limit field so the log message size will not actually get bigger, although the pid_info struct will.

That sounds like the best option to me.......

@lthall I doubt the practical value of this change...

@xianglunkai this is not something that the majority of vehicles will use however it is useful for very particular problems. One of these problems is caused by slew limited actuators. Slew limited actuators have a small delay for small output changes and much larger delays for larger changes. Basically the frequency response changes with the magnitude of the signal. By adding a PD limit you can ensure the system doesn't move to the lower latency and unstable region.

Copy link
Contributor

@bnsgeyer bnsgeyer left a comment

Choose a reason for hiding this comment

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

LGTM

@tridge
Copy link
Contributor

tridge commented Aug 29, 2023

@lthall I've force pushed the change we discussed to remove the constructor changes

@lthall
Copy link
Contributor Author

lthall commented Sep 12, 2023

Thanks Pete!!! I like it.

@lthall
Copy link
Contributor Author

lthall commented Sep 19, 2023

Mark as advanced

AntennaTracker/Parameters.cpp Outdated Show resolved Hide resolved
@tridge tridge merged commit 550a860 into ArduPilot:master Sep 26, 2023
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants