Computation of i_term_

[carlosjoserg]

Before creating any pull request, I'd like to raise this for discussion.

Lines 282 and 315 in pid.cpp, explicitly:

i_term_ = i_term_ + gains.i_gain_ * dt.toSec() * p_error_;

Shouldn't this term be:

i_term_ = gains.i_gain_*( i_term_ +  dt.toSec() * p_error_ );

Note that, for instance, setting gains.i_gain_ to zero online in the current implementation does not remove the integrated i_term_ so far, and it won't until you reset the controller.

/cc @manuelbonilla

[adolfo-rt]

Lines 282 and 315 in pid.cpp, explicitly:

i_term_ = i_term_ + gains.i_gain_ * dt.toSec() * p_error_;

What this is implementing is that gain changes will only affect the samples after which it was changed. I don't know if this is what was originally intended or not, but I get the feeling that the original implementation assumed constant gains, where this was not a problem. The dynamic_reconfigure interface was added much later.

Shouldn't this term be:

i_term_ = gains.i_gain_*( i_term_ +  dt.toSec() * p_error_ );

Not quite, as i_term_ already contains the multiplicative contribution of the integral gain.

It would seem reasonable to expect that if you modify the integral gain (and in particular set it to zero), it should affect the whole integration window, so how about:

• Compute separately the error integral, and cache it in a new class member p_error_int_

p_error_int_ += dt.toSec() * p_error_;

• Compute the integral term as

i_term_ = gains.i_gain_* p_error_int_;

• p_error_int_ would get reset to zero when the PID is reset.

Patches and tests welcome :-)

[carlosjoserg]

You are right, my mistake, I was only thinking in the reset functionality and forgot about the integration scheme!

The change does not modify the control law, it is only useful for tunning and changing the gains dynamically properly, I believe. However, I'm not sure what happens with performance when adjusting the gains online, so I will test and come back with an answer or a patch.

Cheers

[adolfo-rt]

The change does not modify the control law, it is only useful for tunning and changing the gains dynamically properly, I believe.

Correct, but now that there is the dynamic_reconfigure interface, it is an important improvement. Thanks for catching it.

[forrestv]

The original way of doing actually made much more sense, since it prevented the output from being discontinuous when the integral gain is changed. For example, if the integrator has converged to what the output has to be to maintain zero steady state error and then the integrator gain is doubled, all the sudden the output will be doubled(!). It makes more sense to integrate the current gain * current error (the original behavior), which avoids this. See https://en.wikipedia.org/wiki/PID_controller#Bumpless_Operation for a similar solution.

Yes, this does imply that changing to a zero integrator gain will result in a constant, unchanging output offset until the controller is reset, but that's better than sudden, potentially large jumps in the output. In addition, if your integrator has actually converged to something nonzero, it means that you probably do need an integrator term in your controller and you shouldn't be setting the I gain to zero anyway!

[carlosjoserg]

I'm not an expert on the topic, so thanks for the comment, since it made me go and investigate more on PID design.

I found this PhD thesis, and this summary taken from that thesis and other papers. All diagrams make sense to me, and the chosen structure varies according to the application, such as the anti-windup opt requested in #34 for a quadrotor control.

As far as I understand, the bumpless operation (or transfer, which is the most-used term in the literature and also by matlab), is meant to be used when transitioning between Manual and Automatic control. Don't know if there are too many use-cases in (autonomous) robotics, but still valid as an argument for a generic PID controller implementation. However, in the Wikipedia link you provide, it says literally A partial implementation... to refer to something like the original way of doing it.

Based on this, my suggestion would be to keep the new way, which makes get and reset of the integral error easier and safer, and implement a proper bumpless opt by means of a tracking mode, such as the ones in diagram 11, 12 or 13 from the summary I provided above, or as described in the matlab documentation.