Skip to content

Conversation

ViktorCVS
Copy link
Contributor

@ViktorCVS ViktorCVS commented Sep 16, 2025

Overview

Adds a configurable derivative filter. It also introduces separate discretization method selectors for the I and D terms. Forward Euler remains the default to preserve existing behavior.

What was added/changed in this PR

  • Added a first-order derivative filter (tf) applied to the D term.
  • Introduced per-term discretization selectors: i_method and d_method (e.g., "forward_euler", "backward_euler", "trapezoidal"), with "forward_euler" as the default.
  • Implemented discrete D-term updates for the supported methods, including filtered forms; falls back to unfiltered behavior when tf == 0.
  • Added internal state (d_term_last_, d_error_last_, i_term_last_, aw_term_last_) to support trapezoidal (Tustin) and filtered updates.

About tests

The packages compile correctly and have passed the pre‑commit and colcon tests. New tests are being added based on formula values to compare with those from compute_command.

Related PR's

Attachments

Full derivation of the discrete-time PID used in this PR. The document shows the step-by-step development of the equations.

ros2_control_new_discretization_feature.pdf

Final notes

I'm very open to any recommendations to improve this code.

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 92.44936% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.98%. Comparing base (6a28f23) to head (2b7a909).

Files with missing lines Patch % Lines
control_toolbox/include/control_toolbox/pid.hpp 54.16% 19 Missing and 3 partials ⚠️
control_toolbox/src/pid_ros.cpp 68.42% 10 Missing and 2 partials ⚠️
control_toolbox/src/pid.cpp 91.66% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           ros2-master     #471      +/-   ##
===============================================
+ Coverage        78.90%   80.98%   +2.08%     
===============================================
  Files               29       29              
  Lines             1896     2351     +455     
  Branches           119      128       +9     
===============================================
+ Hits              1496     1904     +408     
- Misses             332      378      +46     
- Partials            68       69       +1     
Flag Coverage Δ
unittests 80.98% <92.44%> (+2.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ontrol_toolbox/include/control_toolbox/pid_ros.hpp 100.00% <ø> (ø)
control_toolbox/test/pid_ros_parameters_tests.cpp 100.00% <100.00%> (ø)
control_toolbox/test/pid_ros_publisher_tests.cpp 94.85% <100.00%> (+0.40%) ⬆️
control_toolbox/test/pid_tests.cpp 100.00% <100.00%> (ø)
control_toolbox/src/pid.cpp 81.15% <91.66%> (-2.59%) ⬇️
control_toolbox/src/pid_ros.cpp 67.65% <68.42%> (-0.22%) ⬇️
control_toolbox/include/control_toolbox/pid.hpp 57.72% <54.16%> (-3.26%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ViktorCVS
Copy link
Contributor Author

Hi, @christophfroehlich! I still need to fix the existing tests and add new ones. However, the equations are quite large, and the compute_command function has grown a lot. Do you think it’s acceptable (with better organization), or should something be changed?

@christophfroehlich
Copy link
Contributor

Hi, @christophfroehlich! I still need to fix the existing tests and add new ones. However, the equations are quite large, and the compute_command function has grown a lot. Do you think it’s acceptable (with better organization), or should something be changed?

IMHO it is acceptable to go further in this direction, thanks!

@christophfroehlich christophfroehlich linked an issue Sep 17, 2025 that may be closed by this pull request
double u_max_ = std::numeric_limits<double>::infinity(); /**< Maximum allowable output. */
double u_min_ = -std::numeric_limits<double>::infinity(); /**< Minimum allowable output. */
AntiWindupStrategy antiwindup_strat_; /**< Anti-windup strategy. */
std::string i_method_ = "forward_euler"; /**< Integration method. */
Copy link
Contributor

Choose a reason for hiding this comment

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

why have you chosen a std::string here? Wouldn't a simple enum be more lightweight here? Only thing we loose will be the human readable value in the print method without adding additional logic there. alternative would be a struct like the andiwindup strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I change it to struct in commit 2b7a909

gains.d_gain_ = parameter.get_value<double>();
changed = true;
}
else if (param_name == param_prefix_ + "derivative_filter_time")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you have not added the integration/derivative method here on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for now I don’t think anyone would modify the discretization methods during runtime.

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.

Add option to PID for Forward/Backward integration
3 participants