[Pid] Save i_term instead of error integral
#294
Conversation
github-actions
bot
requested review from
DasRoteSkelett,
destogl,
LukasMacha97,
mcbed,
rosterloh and
saikishor
christophfroehlich
force-pushed
the
fix/i_term
branch
from
76a4ff7 to
8176afc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ros2-master #294 +/- ##
============================================
Coverage 75.62% 75.62%
============================================
Files 24 24
Lines 1161 1157 -4
Branches 87 86 -1
============================================
- Hits 878 875 -3
Misses 236 236
+ Partials 47 46 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
LGTM! The documentation though should be updated for the antiwindup parameter of the pid_controller. As the documentation currently states that i_term_ is always limited regardless of what the value of the antiwindup parameter is, which was the case before this PR.
| } | ||
|
|
||
| // Calculate derivative contribution to command | ||
| d_term = gains.d_gain_ * d_error_; | ||
|
|
||
| // Compute the command | ||
| cmd_ = p_term + i_term + d_term; | ||
| // Limit i_term so that the limit is meaningful in the output | ||
| cmd_ = p_term + std::clamp(i_term_, gains.i_min_, gains.i_max_) + d_term; |
There was a problem hiding this comment.
Not sure why i_term is being clamped again here when it was clamped above when antiwindup is true. Shouldn't it only be clamped when antiwindup is true
There was a problem hiding this comment.
I left that to keep the current behavior, it was clamped even with antiwindup being true.
from the docstring:
antiwindup Antiwindup functionality. When set to true, limits
the integral error to prevent windup; otherwise, constrains the
integral contribution to the control output. i_max and
i_min are applied in both scenarios.
this was already questioned in #276 and I didn't want to change this behavior in this PR.
There was a problem hiding this comment.
Ok. I understand and it shouldn't be changed in this PR. The only minor comment I would make is if antiwindup is false, the iterm is not clamped but the use of it is clamped when calculating output. That is slightly different behavior than before where the iterm was always clamped. Practically, I don't think it makes a difference.
I definitely think the antiwindup behavior needs to be fixed and I guess it will be addressed in #276. With this PR and the earlier behavior, the antiwindup flag, in practical terms, doesn't change anything. Antiwindup is always being implemented regardless of the flag.
| @@ -284,7 +284,7 @@ void PidROS::publish_pid_state(double cmd, double error, rclcpp::Duration dt) | |||
| rt_state_pub_->msg_.error = error; | |||
| rt_state_pub_->msg_.error_dot = d_error; | |||
| rt_state_pub_->msg_.p_error = p_error; | |||
| rt_state_pub_->msg_.i_error = i_error; | |||
| rt_state_pub_->msg_.i_error = i_term; | |||
There was a problem hiding this comment.
I think you said you will change the message in a different PR since this is really weighted i-error.
| } | ||
|
|
||
| // Calculate derivative contribution to command | ||
| d_term = gains.d_gain_ * d_error_; | ||
|
|
||
| // Compute the command | ||
| cmd_ = p_term + i_term + d_term; | ||
| // Limit i_term so that the limit is meaningful in the output | ||
| cmd_ = p_term + std::clamp(i_term_, gains.i_min_, gains.i_max_) + d_term; |
There was a problem hiding this comment.
Ok. I understand and it shouldn't be changed in this PR. The only minor comment I would make is if antiwindup is false, the iterm is not clamped but the use of it is clamped when calculating output. That is slightly different behavior than before where the iterm was always clamped. Practically, I don't think it makes a difference.
I definitely think the antiwindup behavior needs to be fixed and I guess it will be addressed in #276. With this PR and the earlier behavior, the antiwindup flag, in practical terms, doesn't change anything. Antiwindup is always being implemented regardless of the flag.
The internal state should be the i_term to avoid jumps in the output while tuning the integral gain. Reverts #33
With constant gains, this PR does not change the current behavior of output calculation. Only the field in the published message and the result of
get_current_pid_errorswill be changed.I got confused of the message definition, because it has some duplicates:
ros-controls/control_msgs#173
If yes, let's fix that in a new PR.