control_toolbox icon indicating copy to clipboard operation
control_toolbox copied to clipboard

Add three new anti-windup techniques and a Saturation feature

Open ViktorCVS opened this issue 8 months ago • 3 comments

Overview

This PR adds three new anti-windup techniques: back‑calculation, the conditioning technique, and conditional integration. It also adds a saturation feature for the PID output. New parameters have been introduced, and additional overloads have been implemented to ensure compatibility.

What was added/changed in this PR

  • Added three new anti-windup techniques
    • back-calculation
    • conditioning technique
    • conditional integration
  • Added saturation feature to PID output

About compatibility

The packages compile correctly and have passed the pre‑commit and colcon tests (packages with dependencies continue to show the same number of failures before and after my modifications). If the new parameters are not used, the package retains its old behavior.

About the older anti-windup technique

My plan, either by the end of this PR or in a subsequent one, is to completely remove the older anti‑windup technique that has been used so far. This method, which is a form of conditional integration, has several disadvantages:

  • If set incorrectly, it may cause a steady‑state error.
  • If set incorrectly, it may not affect the system at all.
  • Even if it is set between the steady‑state error limit and the value beyond which it has no effect, it is still difficult to find a configuration that improves the system as effectively as the other techniques.

Additionally, regardless of whether the 'antiwindup' parameter is set to true or false, the anti-windup technique is applied (using the same method with a different approach), so the user does not have the option to disable it.

About unit tests

I've added 10 new unit tests for the new features and updated the existing ones to accommodate the new parameters.

Related PR's

  • ros-controls/ros2_controllers#1585
  • ros-controls/control_msgs#178

Important notes

These three techniques are common anti‑windup strategies used to mitigate the windup effect and are widely employed in control applications: back‑calculation [1], the conditioning technique [1,2], and conditional integration [1,3].

The default values for the tracking time constant are defined in [3,4] for back‑calculation and in [1] for the conditioning technique.

Both back‑calculation and the conditioning technique use forward Euler discretization; this may change before merging this PR.

Graphs

I tested it on ros2_control_demos to better illustrate this feature and test it on simulation to valide the equations. The tests were conducted using a modified version of Example 1: RRBot, which uses a PID controller instead of the default forward position controller. It was tested on Docker, Ubuntu Noble, and Jazzy.

PID values: p = 4.0, i = 25.0, d = 0.5; u_max = 13, u_min = -13; and the tracking time constant was left at its default value.

unsat+sat+back

The standard response with a settling time (ts) of 5.2 seconds, the response affected by saturation, resulting in a settling time (ts_sat) of 8.6 seconds (+65.4% increase) and the response using the back-calculation technique, which improves performance with a settling time (ts_back) of 4.1 seconds (–21.2% decrease), even lower than the standard response.

back+ct+cin back+ct+cin2

Those figures compares three anti-windup methods applied to the step response, a zoomed-in view of the step response is provided here to clearly distinguish between the three anti-windup strategies. They are all very similar due to the system and PID values, but they may vary significantly between applications.

cs_unsat+sat+back

The standard control output, the control output affected by saturation, with a recovery time from saturation of 6.8s and the control output using the back-calculation technique, with a recovery time from saturation of 2s (-70.6%).

cs_back+ct+cin cs_back+ct+cin2

Those figures compares three control outputs using anti-windup methods, a zoomed-in view of the control output is provided here to clearly distinguish between the three anti-windup strategies. They are all very similar due to the system and PID values, but they may vary significantly between applications.

All the equations have been validated with these simulations, providing a feature with three techniques to address windup.

Final notes

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

References

[1] VISIOLI, A. Pratical PID Control. London: Springer-Verlag London Limited, 2006. 476 p. [2] VRANCIC, D. Some Aspects and Design of Anti-Windup and Conditioned Transfer. Thesis (Master in Electrical Engineering) — University of Ljubljana, Faculty of Electrical Engeneering, 1995. [3] BOHN, C.; ATHERTON, D. An analysis package comparing pid anti-windup strategies. IEEE Control Systems Magazine, p. 34–40, 1995. [4] ASTRöM, K.; HäGGLUND, T. PID Controllers: Theory, Design and Tuning. ISA Press. Research Triangle Park, USA: Springer-Verlag London Limited, 1995. 343 p.

ViktorCVS avatar Mar 11 '25 22:03 ViktorCVS

This pull request is in conflict. Could you fix it @ViktorCVS?

mergify[bot] avatar Mar 12 '25 10:03 mergify[bot]

@christophfroehlich, it appears that no reviewers have been assigned to this PR. Could you please help with that? If you have time, I'd appreciate it if you could also take a look at the changes.

ViktorCVS avatar Mar 13 '25 18:03 ViktorCVS

Codecov Report

Attention: Patch coverage is 78.42640% with 85 lines in your changes missing coverage. Please review.

Project coverage is 77.53%. Comparing base (516eccd) to head (257fb57). Report is 1 commits behind head on ros2-master.

Files with missing lines Patch % Lines
control_toolbox/src/pid_ros.cpp 55.76% 41 Missing and 5 partials :warning:
control_toolbox/src/pid.cpp 58.97% 30 Missing and 2 partials :warning:
control_toolbox/include/control_toolbox/pid.hpp 84.44% 6 Missing and 1 partial :warning:
Additional details and impacted files
@@               Coverage Diff               @@
##           ros2-master     #298      +/-   ##
===============================================
- Coverage        78.17%   77.53%   -0.65%     
===============================================
  Files               30       30              
  Lines             1338     1696     +358     
  Branches            87      101      +14     
===============================================
+ Hits              1046     1315     +269     
- Misses             245      327      +82     
- Partials            47       54       +7     
Flag Coverage Δ
unittests 77.53% <78.42%> (-0.65%) :arrow_down:

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 95.00% <100.00%> (ø)
control_toolbox/test/pid_tests.cpp 100.00% <100.00%> (ø)
control_toolbox/include/control_toolbox/pid.hpp 82.35% <84.44%> (+4.57%) :arrow_up:
control_toolbox/src/pid.cpp 71.24% <58.97%> (-19.77%) :arrow_down:
control_toolbox/src/pid_ros.cpp 65.93% <55.76%> (-7.96%) :arrow_down:
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Mar 14 '25 22:03 codecov-commenter

@ViktorCVS could you please resolve the current conflicts?

bmagyar avatar Apr 24 '25 10:04 bmagyar

@ViktorCVS could you please resolve the current conflicts?

done

ViktorCVS avatar Apr 24 '25 17:04 ViktorCVS

My plan, either by the end of this PR or in a subsequent one, is to completely remove the older anti‑windup technique that has been used so far. This method, which is a form of conditional integration, has several disadvantages:

Please add a deprecation notice to the code, as well as a warning on std::cout if "none" is configured, for example "xxx is deprecated. This option will be removed by the ROS 2 Kilted Kaiju release."

christophfroehlich avatar Apr 26 '25 12:04 christophfroehlich

My plan, either by the end of this PR or in a subsequent one, is to completely remove the older anti‑windup technique that has been used so far. This method, which is a form of conditional integration, has several disadvantages:

Please add a deprecation notice to the code, as well as a warning on std::cout if "none" is configured, for example "xxx is deprecated. This option will be removed by the ROS 2 Kilted Kaiju release."

Solve it in commits 0f76355 and 00e0a45

ViktorCVS avatar May 05 '25 16:05 ViktorCVS

For the future: Please don't force push to PRs because it makes it harder for reviewers to check the changes since the last review ;) The history does not have to be linear, because we squash that anyways.

christophfroehlich avatar May 13 '25 13:05 christophfroehlich

For the future: Please don't force push to PRs because it makes it harder for reviewers to check the changes since the last review ;) The history does not have to be linear, because we squash that anyways.

Sorry about that. I tried pushing without force, but Git wouldn’t accept it. I’ll try other methods from now on.

ViktorCVS avatar May 13 '25 16:05 ViktorCVS

why you rebased and force-pushed again? the diff is strange now because the last commit on master is included here?

christophfroehlich avatar May 27 '25 17:05 christophfroehlich

why you rebased and force-pushed again? the diff is strange now because the last commit on master is included here?

There’s a warning that this branch is out of date with the base branch, so I pulled the latest changes and pushed. This time I didn’t use a force push, so it’s strange that you’re still seeing this message.

Whenever the master branch is updated, I can’t push without force. Since you asked me to avoid force pushes, I merged master into my branch before pushing.

ViktorCVS avatar May 28 '25 15:05 ViktorCVS

alright, I see what I can do to fix it and do the final review round ;)

christophfroehlich avatar May 28 '25 16:05 christophfroehlich

alright, I see what I can do to fix it and do the final review round ;)

Thanks! There are new updates in the master branch. Should I wait before merging them?

ViktorCVS avatar May 28 '25 16:05 ViktorCVS

I merged them already in, now the diff view looks fixed again.

christophfroehlich avatar May 28 '25 17:05 christophfroehlich

I merged them already in, now the diff view looks fixed again.

Can you take a look again, please? My Git didn't work without push with force, so I push the next commit with /f.

ViktorCVS avatar May 30 '25 02:05 ViktorCVS

The API and structure looks good now, thanks! I cross-checked the equations with the cited literature and have some questions, see my comments.

I recently added documentation integrated in control.ros.org. Could you please summarize your changes in

  • the release notes. You can use the style from the other repos.
  • and add necessary migration steps here. What will be the default after the deprecation cleanup? conditional integration?

Not necessarily in this PR, but could you please summarize the PID class in structured text format and add it here?

I have a question about the migration steps: this PR was designed so that it doesn’t break existing implementations, but the next PR will actually remove the deprecated technique. Should I list the migration steps now?

ViktorCVS avatar May 30 '25 02:05 ViktorCVS

Can you take a look again, please? My Git didn't work without push with force, so I push the next commit with /f.

Why? Have you pulled the changes (after my merge via Github UI?). It maybe won't let you pull if you have uncommited changes, either commit them and do a git pull --rebase or stash them before the pull.

I have a question about the migration steps: this PR was designed so that it doesn’t break existing implementations, but the next PR will actually remove the deprecated technique. Should I list the migration steps now?

Let's add the migration notes before we remove it. We will release the changes, wait for the next sync, and then we can remove it. So users have one release cycle to see the deprecation warnings before any behavior changes are applied.

christophfroehlich avatar May 30 '25 08:05 christophfroehlich

Oh, I see that the downstream PidController tests fail now. Can you have a look please and update ros-controls/ros2_controllers#1585?

Fixed

ViktorCVS avatar May 30 '25 13:05 ViktorCVS

Fixed

Thanks. I see now that antiwindup parameter is not used any more if a strategy other than NONE is configured. Shouldn't we add this flag there also?

christophfroehlich avatar May 30 '25 15:05 christophfroehlich

@ViktorCVS Very good work

I've only one question, what will the bool antiwindup parameter be used for from now on?

Thanks! Until I remove it, it will still maintain the old behavior. This PR just adds more options. I’ll open a follow-up PR soon to remove it.

ViktorCVS avatar May 30 '25 16:05 ViktorCVS

Fixed

Thanks. I see now that antiwindup parameter is not used any more if a strategy other than NONE is configured. Shouldn't we add this flag there also?

In ros2_controllers? I agree. I’ll update the anti-windup parameter description accordingly.

ViktorCVS avatar May 30 '25 16:05 ViktorCVS

Thanks again!

Could you please submit two follow-up PRs:

  • One which updates the documentation, basically the summary of your initial message here. But you don't need to document the old behavior, just the result after the deprecation cleanup. Here you can add the migration notes.
  • Another one cleaning up the deprecated code. You can draft it for now, and we will merge after the next sync to kilted/rolling.

Should I wait for the merge?

ViktorCVS avatar May 30 '25 16:05 ViktorCVS

@ViktorCVS I came to the conlusion with Sai that we merge this and discuss about the saturation in a follow-up PR. Could you please prepare the documentation update first? The deprecation removal has more time, at least until we release the package.

christophfroehlich avatar Jun 06 '25 09:06 christophfroehlich

@ViktorCVS Do you maybe have time and like to open a PR cleaning all the legacy and deprecated behavior now?

christophfroehlich avatar Jul 23 '25 13:07 christophfroehlich