-
Notifications
You must be signed in to change notification settings - Fork 106
Add three new anti-windup techniques and a Saturation feature #298
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
base: ros2-master
Are you sure you want to change the base?
Conversation
This pull request is in conflict. Could you fix it @ViktorCVS? |
d0feb10
to
bd7c8f0
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for this thorough PR, but it will need some time to properly review it
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ros2-master #298 +/- ##
===============================================
+ Coverage 78.17% 80.58% +2.41%
===============================================
Files 30 30
Lines 1338 1700 +362
Branches 87 99 +12
===============================================
+ Hits 1046 1370 +324
- Misses 245 276 +31
- Partials 47 54 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
@ViktorCVS could you please resolve the current conflicts? |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the nice work including tests etc. Please fix the pre-commit errors, and only some minor comments in the code
Please add a deprecation notice to the code, as well as a warning on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
We get closer to the finish line, but some of our comments haven't been addressed yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are failing now, can you have a look please?
5513ff0
to
55f3505
Compare
The i_bounds and u_bounds conditions were combined using an 'or' operator. I have updated them to use separate if else statements.
Delete comments related to dynamic reconfigure and ROS, and update several other comments.
The i_bounds and u_bounds conditions were combined using an 'or' operator. I have updated them to use separate if else statements.
This commit adds a helper function for comparing values to zero, improving the readability of the code.
Refactor antiwindup strategy variable implementation replacing string usage with a dedicated enum improve type safety.
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. |
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? |
I merged them already in, now the diff view looks fixed again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
if (is_zero(gains.trk_tc_) && !is_zero(gains.d_gain_)) | ||
{ | ||
// Default value for tracking time constant for back calculation technique | ||
gains.trk_tc_ = std::sqrt(gains.d_gain_ / gains.i_gain_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure? in [1] the definition of the derivative action is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. in [1] p.39 eq. 3.3, he defines
Aplicadas a Sistemas de Controle/Comparative Study of Anti-Windup Strategies Applied to Control Systems) that list the equation this way.
else if (is_zero(gains.trk_tc_) && is_zero(gains.d_gain_)) | ||
{ | ||
// Default value for tracking time constant for back calculation technique | ||
gains.trk_tc_ = gains.p_gain_ / gains.i_gain_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in [1] they claim:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in [1] p.39 eq. 3.4,
} | ||
else if (gains.antiwindup_strat_ == AntiwindupStrategy::CONDITIONAL_INTEGRATION) | ||
{ | ||
if (!(cmd_unsat_ != cmd_ && error * cmd_unsat_ > 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should not compare if two double are equal, let's compare the difference against eps?
gains.antiwindup_strat_ == AntiwindupStrategy::CONDITIONING_TECHNIQUE && | ||
!is_zero(gains.i_gain_)) | ||
{ | ||
i_term_ += dt_s * gains.i_gain_ * (error + 1 / gains.trk_tc_ * (cmd_ - cmd_unsat_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have access to [2], but in [1] the conditioning technique is described differently (Fig 3.4).
Is there a real benefit in adding all of the three? Or should we skip the CONDITIONING_TECHNIQUE as it is the least known/documented one? I think the other two leave enough choices and tuning possibilities for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified the material from source [2], but I have researched the CONDITIONING_TECHNIQUE and noticed some discrepancies between sources. In some of them, BACK_CALCULATION and CONDITIONING_TECHNIQUE can have the same effect if we tune Tt in a specific way. Therefore, I agree to remove CONDITIONING_TECHNIQUE. I’ll do this as soon as possible. I’ll also replace [2] with an equivalent but accessible article.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great thx, let's push this to be merged soon. documentation can be added afterwards, because we don't need to release it
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
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:
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
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.
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.
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.
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%).
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.