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

Refactor controller update to take const setpoint #1166

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

whoenig
Copy link
Contributor

@whoenig whoenig commented Dec 8, 2022

The update function of controllers should not alter the setpoint itself.
This could lead to unexpected behavior and makes the Python interface for
software-in-the-loop simulation less clean.

The change itself is simple, apart from the PID velocity controller.
Here, there is a small behavior change: when logging, the user-specified setpoint will be shown, not
the intermedate value altered by the controller.
However, this seems to be the more logical behavior and is consistent
with the behavior of all other controllers that compute intermediate
setpoints (e.g., position controller of mellinger
computes intermediate angles).

The update function of controllers should not alter the setpoint itself.
This could lead to unexpected behavior and makes the Python interface for
software-in-the-loop simulation less clean.

The change itself is simple, apart from the PID velocity controller.
Here, there is a small behavior change: when logging, the user-specified setpoint will be shown, not
the intermedate value altered by the controller.
However, this seems to be the more logical behavior and is consistent
with the behavior of all other controllers that compute intermediate
setpoints (e.g., position controller of mellinger
 computes intermediate angles).
@krichardsson
Copy link
Contributor

Looks good to me, I love const :-)
I think the setpoint is modified because that is the way the code was written a long time ago. Nice to get it fixed!

@krichardsson krichardsson merged commit c656b2c into bitcraze:master Dec 9, 2022
@krichardsson
Copy link
Contributor

Thanks @whoenig !

@whoenig whoenig deleted the refactor_ctrl_const_setpoint branch December 9, 2022 10:55
@krichardsson krichardsson added this to the 2022.12 milestone Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants