Repository navigation
Support for different control modes with mujoco by defining PIDs - #5
Conversation
eholum-nasa
left a comment
eholum-nasa
left a comment
There was a problem hiding this comment.
Sorry for the delay here, @saikishor! The changes look great. Thanks for your efforts there.
Just the one, very, very nit comment but IMO we should ship this and keep moving forward.
| last_joint_state.is_position_control_enabled = true; | ||
| last_joint_state.position_command = | ||
| should_override_start_position ? mj_data_->ctrl[joint_state.mj_actuator_id] : last_joint_state.position; | ||
| if (last_joint_state.actuator_type == ActuatorType::VELOCITY || |
There was a problem hiding this comment.
Thanks! The comments and the table in the README are also super helpful here.
| ros__parameters: | ||
| pid_gains: | ||
| position: | ||
| joint1: |
There was a problem hiding this comment.
I think we could probably tweak these to work in a more stable fashion but I'm also not sure what I was thinking when I made these links 27 kg each...
Fun to play with these though!
There was a problem hiding this comment.
I think we might as well update these to real values as a part of this PR so it is reasonable to test? Bc at the moment, it is hard to verify functionality with those values
There was a problem hiding this comment.
Sure, we can do it. Give us sometime
There was a problem hiding this comment.
Thanks! I think mainly just so that the default gains work well in test space. To get something relatively reasonable I was ending up with some pretty strange gains 0.o
There was a problem hiding this comment.
Oh blah. Yeah I was thinking we should consolidate the example models into a single file and maybe xacro out the driver specifics. When we do that we should not have 27 kg links and pick something a little more reasonable that is easier to tune with "normal" gains.
Thoughts?
There was a problem hiding this comment.
Done. I have some more reasonable gains.
There was a problem hiding this comment.
Thanks, @saikishor! I just tried it and things look great, though we might have to add i_clamp values to accommodate this... silly timing...
ros-controls/control_toolbox#515
Which looks like it just got released and picked up locally... Here's the config that worked for me:
/**:
ros__parameters:
pid_gains:
position:
joint1:
p: 200.0
i: 0.005
d: 50.0
u_clamp_max: 100.0
u_clamp_min: -100.0
i_clamp_max: 10.0
i_clamp_min: -10.0
joint2:
p: 200.0
i: 0.005
d: 50.0
u_clamp_max: 100.0
u_clamp_min: -100.0
i_clamp_max: 10.0
i_clamp_min: -10.0
pid_gains:
velocity:
joint1:
p: 100.0
i: 0.001
d: 5.0
u_clamp_max: 100.0
u_clamp_min: -100.0
i_clamp_max: 10.0
i_clamp_min: -10.0
joint2:
p: 100.0
i: 0.001
d: 5.0
u_clamp_max: 100.0
u_clamp_min: -100.0
i_clamp_max: 10.0
i_clamp_min: -10.0
There was a problem hiding this comment.
Oh whoops... mixing work and non-work accounts again...
There was a problem hiding this comment.
hahaha. Thanks. I just updated the gains
eholum-nasa
left a comment
eholum-nasa
left a comment
There was a problem hiding this comment.
Oh sorry I was going to apply and merge that line fix but don't have access... if you modify it I think this is GTG!
Co-authored-by: Erik Holum <erik.holum@nasa.gov>
| joint.name.c_str()); | ||
| } | ||
| } | ||
| else if (last_joint_state.actuator_type == ActuatorType::POSITION) |
There was a problem hiding this comment.
Should we somehow warn the user if they specified PID gains, but the position control actuator is actually used instead? I think power users would probably understand the implication of mixing and matching mujoco actuator types with pid actuator types, but I think relatively newer users might just assume that adding their own pid gains and including that file in the ros2 control tag would mean that those would be the default behavior.
There was a problem hiding this comment.
No, it is not the default behaviour. I've added a table in the README to explain the same
There was a problem hiding this comment.
If you use effort and then PIDs of the position and velocity, you should be able to switch between them at runtime
There was a problem hiding this comment.
Yeah, I guess that is fair. I think maybe just adding a comment that the mujoco actuator of a specific type will be preferred if both that mujoco actuator and the pid of that type is specified.
|
@ndunkelb-nasa I've addressed your comments |
eholum-nasa
left a comment
eholum-nasa
left a comment
There was a problem hiding this comment.
with the right account!
Thanks, I think it looks good! |
…-controls#5) --------- Co-authored-by: Ortisa Poci <ortisa.poci@studenti.unipd.it>
Hello!
We added a new feature here. If you define the PIDs via an argument, then you can utilize different control modes. For instance, with a standard MuJoCo actuator, after defining gains of position and velocity PIDs, you can control the robot in position, velocity or effort.
We also added an example test case, so you can try it out
Kudos to @Ortisa
Similar to NASA-JSC-Robotics/mujoco_ros2_simulation#38