PID implementation inside mujoco-py

[bayesian]

Expose 5 new parameters for full PID with clamp/smoothing.

test plans:

1. verified the correctness, when client do not specify new parameters, PID behaves like P
2. for mujoco_shadow_hand test, with global I/D settings, position errors after 100 steps become smaller.
3. unit tests all with inverted pendulum

[MillionIntegrals]

Few comments on this PR.

[MillionIntegrals]

I don't think we should override that manually for the actuators here. That should be specified in the XML file.

[MillionIntegrals]

I also would like it to be opt-in, but on the XML level - then we can have multiple hands, one with PID Controller, one without. We can have even these two hand in the same simulation. Or we can have kuka arm with P controllers and a shadow hand with PID controllers. The more things we allow to configure via XML the better for flexibility.

[MillionIntegrals]

I know it's not that important, but why not use gainprm for this purpose?

[MillionIntegrals]

I also see this kind of as a good thing. If we keep the kp at the same position, just by changing the gaintype from user to position, we can change between the P and PID controller with the same P value. Pretty cool, isn't it?

[arthurpetron]

No! They are that way for a reason!

…

On Fri, Oct 4 2019 at 15:47, Jonas Schneider < ***@***.*** > wrote: ***@***.**** commented on this pull request. Few comments on this PR. In mujoco_py/mjpid.pyx ( #462 (comment) ) : > + return fmax(-corrective_effort_limit, fmin(corrective_effort_limit, f)) + + +def set_pid_control(m, d): + global mjcb_act_gain + global mjcb_act_bias + + if m.nuserdata < m.nu * NUM_USER_DATA_PER_ACT: + raise Exception('nuserdata is not set large enough to store PID internal states') + + for i in range(m.nuserdata): + d.userdata[i] = 0.0 + + for i in range(m.nu): + m.actuator_gaintype[i] = const.GAIN_USER + m.actuator_biastype[i] = const.BIAS_USER I don't think we should override that manually for the actuators here. That should be specified in the XML file. In mujoco_py/mjpid.pyx ( #462 (comment) ) : > + global mjcb_act_gain + global mjcb_act_bias + + if m.nuserdata < m.nu * NUM_USER_DATA_PER_ACT: + raise Exception('nuserdata is not set large enough to store PID internal states') + + for i in range(m.nuserdata): + d.userdata[i] = 0.0 + + for i in range(m.nu): + m.actuator_gaintype[i] = const.GAIN_USER + m.actuator_biastype[i] = const.BIAS_USER + + # if user does not set, will chose default settings. + # for kp, it tries to use m.actuator_gainprm[i][0] + if m.actuator_user[i][PROPORTIONAL_GAIN] <= 0.0: I know it's not that important, but why not use gainprm for this purpose? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub ( #462?email_source=notifications&email_token=AI2BVUG7OIRU6EGRC6MHYR3QM7BXRA5CNFSM4I5UQIR2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCG7UHXQ#pullrequestreview-297747422 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AI2BVUCT3PUHR3Y3SE55HZTQM7BXRANCNFSM4I5UQIRQ ).

[bayesian]

will update soon with Author's comments

[MillionIntegrals]

I approve this change, but let's make sure @arthurpetron is also happy

[arthurpetron]

By DERIVATIVE_TIME_CONSTANT do you actually mean DERIVATIVE_TIME_CONSTANT_ID? Same comment for lines 44 - 50 and lines 20 - 25 and lines 29 - 32. What is NGAIN? Is it also an ID?

[bayesian]

DERIVATIVE_TIME_CONSTANT is the index for the field, NGAIN is the total number of parameters

[bayesian]

updated to IDX_

[arthurpetron]

Please label ID's as IDs to prevent future confusion. Otherwise looks good. Thanks!!

[jborbik]

Thanks for a great PR!

May I ask you if it is working fine with shadow robot hand? I was trying to use this PID control, but the finger control does not work anymore, the fingers don't move a bit.

I tried also the P control equivalent case based on the commited pid_test, but the fingers are still immobile. If the shadow robot should work I will open a separate issue.

[bayesian]

yes, it works for us on our shadow hand simulation and policy training, it is also backward compatible. you need to change xml and call set_pid_control, like in the test_pid.py in the PR.

[jborbik]

Thank you for your quick answer! But are you always setting up both gaintype and biastype to "user"? If I define gaintype="user" the fingers stop any movement.

Edit: would it be possible to share an example of the shadow robot dexterous hand model which uses PID so that I can find where my mistake lay?

Edit2: I have created corresponding issue with the prepared repository for problem reproduction.

[bayesian]

you have to set biastype="user" and gaintype="user", as our pid implementation use user-defined callback: mjcb_act_gain and mjcb_act_bias to implement full PID control.

you can read more about mujoco actuator model here: http://www.mujoco.org/book/XMLreference.html#actuator

in our shadow robot model, it has something like:

    <general gaintype="user" biastype="user" class="asset_class" ctrlrange="0.0 1.5708" forcerange="-0.2 0.2" joint="FFJ2" gainprm="1.0 5.0 0.2 0.05 0.1 0.0" name="A_FFJ2" user="2002"/>

[brinij]

Hi,
I would also like to have shadow hand mujoco simulation with PID controller.
My only question now is: are you using the same hand model that is in gym environments: robotics/assets/hand/shared.xml or some other? Because the joint names are not quite the same and I am wondering weather this PID parameters will work on other model as well?
Thank you in advance!

[bayesian]

you are free to choose the PID settings, the example settings are from one of my experiments. our hand model used for sim2real is a bit different from gym version.