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

Replace old PID library with a new unit tested C++ class #23931

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Nov 12, 2024

Solved Problem

When doing heli rpm and boat control I found that there is a PID library in PX4 which is pretty tedious to use.

Solution

I wrote a new one with a C++ interface and unit tests.

Changelog Entry

Replace old PID library with a new unit tested C++ class

Test coverage

I successfully used this library for heli rpm control and boat heading control but without a derivative term. PI control only.
For a more practical derivative implementation it'd need a filter on the derivative and possibly one that considers the change of error not just feedback.

Note that the PID speed control instance in RoverPositionControl supplied a vehicle acceleration but configured the old library with PID_MODE_DERIVATIV_CALC which made it calculate a numerical derivative and ignore the provided acceleration all along:
aef5225#diff-0e5af3f25fa84fecddb10c4607acaf89f788f58553835199b217b5d230dfde14R142

FYI @TomasTwardzik

@MaEtUgR MaEtUgR self-assigned this Nov 12, 2024
@MaEtUgR MaEtUgR changed the title Replace ols PID library with a new unit tested C++ class Replace old PID library with a new unit tested C++ class Nov 13, 2024
Copy link
Contributor

@chfriedrich98 chfriedrich98 left a comment

Choose a reason for hiding this comment

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

LGTM, seems to be a lot easier to implement then the old library.
I just have one fix on the implementation in the ackermann module (which is due to the fact that I had initially implemented the old PID library wrong) and a suggestion on handling the Output/Integral limits.

src/lib/pid/PID.cpp Show resolved Hide resolved
src/lib/pid/PID.cpp Show resolved Hide resolved
Copy link

github-actions bot commented Nov 19, 2024

FLASH Analysis

px4_fmu-v5x
    FILE SIZE        VM SIZE    
--------------  -------------- 
[ = ]       0  [ = ]       0    TOTAL

px4_fmu-v6x
    FILE SIZE        VM SIZE    
--------------  -------------- 
[ = ]       0  [ = ]       0    TOTAL

Updated: 2024-11-26T13:10:55

@chfriedrich98 chfriedrich98 self-requested a review November 25, 2024 08:08
chfriedrich98
chfriedrich98 previously approved these changes Nov 25, 2024
Copy link
Contributor

@chfriedrich98 chfriedrich98 left a comment

Choose a reason for hiding this comment

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

These changes fix the unit tests, but I also noticed that the integral is delayed in the output by one update step. Is this on purpose?

src/lib/pid/PID.cpp Outdated Show resolved Hide resolved
src/lib/pid/PIDTest.cpp Show resolved Hide resolved
MaEtUgR and others added 2 commits November 26, 2024 14:01
Co-authored-by: chfriedrich98 <125505139+chfriedrich98@users.noreply.github.com>
…teration

Co-authored-by: chfriedrich98 <125505139+chfriedrich98@users.noreply.github.com>
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 26, 2024

Thanks @chfriedrich98 for following up! I didn't even realize the tests were failing 🙈 I accepted both your suggestions but changed them as in having a separate function for the derivative since we'll likely add filtering to it, similar to https://github.com/PX4/PX4-Autopilot/blob/e194a52907083783f7e67ff93504016ca1275ce8/src/lib/mathlib/math/filter/FilteredDerivative.hpp and having the test verify the reset to zero and the first integral update.

@chfriedrich98 chfriedrich98 self-requested a review November 26, 2024 13:38
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 26, 2024

MAVROS fails on every new pr 😬

@MaEtUgR MaEtUgR merged commit a280d67 into main Nov 26, 2024
59 of 62 checks passed
@MaEtUgR MaEtUgR deleted the pid-class branch November 26, 2024 15:13
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