i_clamp use between ros parameters and dynamic_reconfigure in pid class is inconsistent

[skohlbr]

We just had a lengthy debugging session because we uploaded "i_clamp_min" and "i_clamp_max" to the parameter server (after using dynamic_reconfigure with these params for a while and also looking up the dynamic_reconfigure config, seeing these two parameters). Strangely, the parameters somehow always defaulted to 0 when running our controllers, no matter what value we set them to.

Turns out, on startup, only a "i_clamp" parameter is read from the parameter server. This was not set in our setup, so it defaulted to 0, which triggered setting "i_clamp_min" and "i_clamp_max" symmetrically (to -0.0/0.0) on the parameter server, too.

Would it make sense to also check for the existence of "i_clamp_min" and "i_clamp_max" on the parameter first on init? Appears this wouldn´t break existing code, but allow using the parameters without running into this annoyance.

[davetcoleman]

I'm the author of the dynamic_reconfigure PID functionality. I haven't run into that issue but also I've never really fully understood the i_clam functionality. You are probably right in identifying that bug - do you have the commit that fixes it so that I can look at it and understand a little better your issue?

[adolfo-rt]

@ipa-fxm, thanks for proposing a fix.

This is what I take from reading the code:

• The C++ API allows to specify min and max integrator clamping values.
• The Pid::init(const ros::NodeHandle &node, const bool quiet) method reads the i_clamp parameter, which then sets the main/max clamp values to -i_clamp, i_clamp, respectively. fix issue #28: i_clamp/i_clamp_min/i_clamp_max reconfigure inconsistency #29 improves the situation by allowing to separately specify the min and max values.
• The dynamic_reconfigure update looks for i_clamp_min and i_clamp_max, but not i_clamp. This could stay as-is.

What needs to be checked now that #29 has been proposed is whether dynamic reconfigure updates of the min and max clamping values are getting picked up correctly.

[fmessmer]

I guess changing i_clamp_min and i_clamp_max via dynamic_reconfigure worked correctly before - and still should. Changing values in dynamic_reconfigure also updates the params on the parameter server correctly.
Thus, the i_clamp in a yaml can be considered a convenience parameter for setting both i_clamp_min and i_clamp_maxto equal magnitude.

[fmessmer]

There is just one more "inconsistency" where I don't know whether it was meant to be like this:
In the yaml the parameters are called p, i, d
where in dynamic reconfigure they are called p_gain, i_gain, d_gain

Thus, having reconfigure running we end up with something like this:

rosparam get /arm_controller/gains/arm_1_joint
{d: 0.0, d_gain: 220.0, i: 0.0, i_clamp: 0.0, i_clamp_max: 260.0, i_clamp_min: -380.0,
  i_gain: 0.0, p: 10, p_gain: 10.0}

Where the p, i, d were initially set from the yaml and p_gain, i_gain, d_gain are used by reconfigure, where p, i, d (and i_clamp) are not updated by reconfigure.

Was there a reason for this or should this be harmonized?

[davetcoleman]

Thanks for the PR!

[davetcoleman]

@ipa-fxm its called i_gain because of this issue I ran into:
http://answers.ros.org/question/46724/auto-generated-dynamic_reconfigure-headers-fail-to-compile-depending-on-field-names/

See my dev note:
#1 (comment)

[fmessmer]

Thanks @davetcoleman for clarification.
The issue you mentioned seames to be fixed since dynamic_reconfigure v1.5.36 according to the answer to the thread you mentioned.

So, shall we further harmonize or would renaming the reconfigure *_gains break something in the backwards compatibility?

[davetcoleman]

Dynamic reconfigure is only used by end-users manually, right? You don't really use it in the API I believe, in which case I don't see how it would hurt to change it for Indigo, or at latest ROS-J.

+1 changing it in Indigo. We'd just need to update the documentation on the gazebosim.org website

[adolfo-rt]

Dynamic reconfigure is only used by end-users manually, right?

Almost. When you launch a node that has a dynamic reconfigure server, it tries to initialize the (dynamic) parameter values from the parameter server, otherwise it takes the defaults from the cfg file. So, setups which override the cfg defaults with rosparam settings would break.

On the other hand, I expect most people that define gains via rosparam to use the old fashioned p, i, d keys, which have been around for many ROS distros. I don't really expect there to be anyone setting p_gain and not p on the parameter server (has been around since hydro only), but it's possible.

How about asking on the SIG if someone adheres to the above unlikely case, and if nobody comes out, we can consider fixing this in indigo.

[davetcoleman]

@ipa-fxm can you ask on the sig? I don't have time to get too involved in this, I just wanted to help with my past experience since i was original creator

[fmessmer]

I decided to do the harmonization in a new PR (see #30).
There dynamic_reconfigure also uses p, i and d instead of p_gain, i_gain and d_gain. Thus, changing parameters in dynamic_reconfigure does not add additional parameters to the parameter server but updates the ones being set via rosparam.

rosparam get /arm_controller/gains/elbow_joint
{d: 100.0, i: 500.0, i_clamp: 100.0, i_clamp_max: 100.0, i_clamp_min: -590.0, p: 79000.0}

As to the use-case @adolfo-rt mentioned:
To me it seems that all controllers in ros_controllers initialize their Pid's with a NodeHandle. There, they fail if no p is set. Thus, it's not possible to initialize Pid without setting p on the parameter server.
Furthermore, it seems that the default values for dynamic_reconfigure (from the cfg) are actually never used because they always are overwritten by the rosparam values (or - if those are not found - by the default values of nh.param().
In that case, I'd say #30 is just beautifying and minimal invasive.

Or am I missing something?

[adolfo-rt]

You have a point here, which only means good news :D

[davetcoleman]

+1 makes sense

[dcconner]

Can close this with recent pull of #30