Add new members for PID controller parameters#1585
Add new members for PID controller parameters#1585christophfroehlich merged 14 commits intoros-controls:masterfrom ViktorCVS:master
Conversation
github-actions
bot
requested review from
LukasMacha97,
arne48,
destogl,
mamueluth,
mcbed,
progtologist and
saikishor
|
I set this as draft until we approved the control_toolbox PR. |
Adds new members to PID controller parameters to support saturation and three new anti-windup features.
Replace the `subset_of` validator with `one_of` because `antiwindup_strategy` is a single string, not a string_array.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1585 +/- ##
=======================================
Coverage 86.23% 86.23%
=======================================
Files 123 123
Lines 11903 11903
Branches 994 994
=======================================
Hits 10264 10264
Misses 1337 1337
Partials 302 302
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
I know that this is not only related to this PR here, but I realize now that the saturation parameter is somehow superfluous: What do you think about initializing u_clamp with inf here, which will be std::numeric_limits<double>::infinity() and the clamping in the base class automatically deactivated? I don't see the benefit of adding an additional boolean parameter..
|
Sai is working on this in ros-controls/control_toolbox#390 |
The saturation parameter has been removed, and the anti-windup parameter, along with i_clamp_max and i_clamp_min—has been marked as deprecated. Additionally, the default saturation bounds have been updated to positive and negative infinity.
saikishor
left a comment
saikishor
left a comment
There was a problem hiding this comment.
I suggested some changes based on the upcoming changes of
ros-controls/control_toolbox#400
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
Another update. I'll open a similar PR with changes to JTC soon.
Nice! There are something more I can change in this PR? |
|
Could you please add release_notes similar to #1759? Then everything is fine I think |
Simplified the PID controller section in doc/release_notes.rst by adding support for u_clamp_max and u_clamp_min output limits, deprecating the old antiwindup flag and i_clamp_max/i_clamp_min in favor of a more flexible antiwindup_strategy and introducing tracking_time_constant and error_deadband.
After merging this PR, I’ll open a new one in control_toolbox to remove the old anti-windup technique, right? |
|
I had to fix some pre-commit errors, and added explicit migration notes.
Let us fix ros-controls/control_toolbox#156 first, then we remove all deprecations at once. |
christophfroehlich
merged commit aebe6b1
into
ros-controls:master
(cherry picked from commit aebe6b1) # Conflicts: # doc/migration.rst # doc/release_notes.rst
4 participants
Add new members to the PID controller parameters to support saturation and three new anti-windup features.
These changes support modifications made to the PID controller in the control_toolbox package. For more details, see ros-controls/control_toolbox#298.
If this PR is approved, it should be merged together with the main PR in the control_toolbox package referenced above.