position PID configuration parameters cleanup

[traversaro]

@andreadelprete is having trouble while tuning the position PID gains of the iCub .
Apparently is not clearly documented the meaning of the PID gains.
I am currently checking in the code, but we should document it somewhere.

[traversaro]

The PID schema used in iCub controlboard is documented in the iCub wiki [1] but apparently is really old. The schema currently used in the iCub appear to have changed a bit [2].

@barbalberto just in case, do you know where the new schema is defined/documented?

[1] : http://wiki.icub.org/wiki/ControlBoard_configuration_file#Pid
[2] : https://github.com/robotology/icub-main/blob/master/app/robots/iCubGenova01/hardware/motorControl/icub_left_leg.xml

[barbalberto]

ask to @randaz81 about pids, he likes adding new parameters from time to time :P

[traversaro]

Apparently we use just the first three parameters defined in the GAZEBO_PIDS [1], and their order is just:

1. Position Gain
2. Derivative Gain
3. Integral Gain

[1] :

gazebo-yarp-plugins/src/yarp_drivers/ControlBoardDriver.cpp

Line 362 in bc74f01

void GazeboYarpControlBoardDriver::prepareJointMsg(gazebo::msgs::JointCmd& j_cmd, const int joint_index, const double ref) //WORKS

Apparently we are not setting the integral limits, neither the output limit. This is consistent (if we assume that by default the integral limits are 0) with what @andreadelprete was observing, namely that the integral gain had no effect on the control. I guess that the default output limit value is not 0, otherwise the PID should not have an effect at all.

[francesco-romano]

@traversaro are you sure about the integral limit?
Do you remember if we pushed the fixes to gazebo about the PID?

[barbalberto]

from the conf .ini file I see, the maxInt e MaxOut are both set to 9999
In function setPIDsForGroup they are set with
pidValue.maxInt = pid.get(4).asDouble();
pidValue.maxOut = pid.get(5).asDouble();

[traversaro]

@francesco-romano The PID fixes are available in Gazebo 3.0, but not in Gazebo 2.2. We should document this to explain why it REALLY recommended to use a Gazebo version as recent as possible (i.e. the latest unless ROS forces you to use an old and bugged version of Gazebo). [1] [2]

@barbalberto Those parameters are loaded correctly, but then are never set in the gazebo PID. I'm fixing it.

[1] : https://bitbucket.org/osrf/gazebo/pull-request/1048/adding-tests-to-pull-request-1022/diff
[2] : https://bitbucket.org/osrf/gazebo/src/f27ed5d03ca32f31a87539ae22cebcc49dd9c354/test/regression?at=gazebo_3.0

[traversaro]

I tried to implement a fix in 4cf6b34 , fix_integral branch .

[traversaro]

The integral control is still not working, even with the fix in 4cf6b34 .

I'll try to further debug, using the gazebo JointController will probably complicate the debugging. Perhaps moving the PID computation in the plugin would simplify things, eliminating the use of protobuf middleware. This would also remove the problems related to the integral limit bug in Gazebo [1] . That bug is solved on 3.0, but people in the walkman project will need to use 2.2 for a long time.

[1] : https://bitbucket.org/osrf/gazebo/issue/1082/jointcontroller-does-not-handle-correctly

[traversaro]

Ok, the fix for integral gain in 4cf6b34 was working, the gain in the iCub was missing but I added it in robotology-legacy/icub-gazebo-legacy@302cfea .
While adding the fix I noticed that probably we are specifying in a undocumented way gains in radians-based units while everything else is in degree-based units, that we should fix.

[traversaro]

We have to fix this issue also for @hu-yue

[traversaro]

Interestingly, for some reason commit 4cf6b34 (that was fundamental to fix antiwindup terms and general limit of the controller) never made it to master...

[barbalberto]

How is the state of this issue? Can be closed?

[traversaro]

No. The pid format is still not documented, and additionally we should migrate to specifying them in degrees-based units, to be consistent with the rest of the interface.

[traversaro]

Furthermore, the velocity PID interfaces are not properly accounting for the unit of measurement mismatch.

[randaz81]

@traversaro As mentioned in #218, various changes were recently done to the PID parameters.
Is this issue still open?

[traversaro]

I think we can consider this issue as solved.