Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TIAGo and TIAGo++ Robot from PAL Robotics to menagerie #20

Closed

Conversation

saikishor
Copy link
Contributor

This PR adds the requested models as in the issue: #19. The models are generated inspiring from the existing models in the repository.

Please feel free to suggest any suggestions to improve the quality of this PR.

Have a great day,

Best Regards,
Sai Kishor Kothakota

@google-cla
Copy link

google-cla bot commented Aug 7, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@saran-t
Copy link
Member

saran-t commented Sep 20, 2023

Hey, sorry I dropped the ball on this. We're going to import this for internal reviewing now.

@saikishor
Copy link
Contributor Author

Hello @saran-t,

Thank you so much. Looking forward to it.

Have a great day,

Best Regards,
Sai Kishor Kothakota

@saran-t
Copy link
Member

saran-t commented Sep 20, 2023

@saikishor I can't find any license file in the upstream repository. We can't accept assets into this repository without an explicit OSS license from the vendor.

@saikishor
Copy link
Contributor Author

@saikishor I can't find any license file in the upstream repository. We can't accept assets into this repository without an explicit OSS license from the vendor.

Thanks for getting back to me. I'm a part of @pal-robotics team. We usually define the licences directly in the package.xml as per ROS standards. You can find it here : https://github.com/pal-robotics/tiago_robot/blob/humble-devel/tiago_description/package.xml#L12.

@saran-t
Copy link
Member

saran-t commented Sep 20, 2023

Would you be willing to include a copy of https://www.apache.org/licenses/LICENSE-2.0.txt verbatim as a file called LICENSE in the root of your repository?

Alternatively we can also take an email from a company director to confirm that @pal-robotics licenses these assets under Apache-2.0.

@saikishor
Copy link
Contributor Author

saikishor commented Sep 20, 2023

Would you be willing to include a copy of https://www.apache.org/licenses/LICENSE-2.0.txt verbatim as a file called LICENSE in the root of your repository?

@saran-t Sure, I think it can be done. Can I do it tomorrow in morning, and ping you back once it is done?, it's late already here in Spain. I hope it's not a problem.

In case of an email, to what email can we send the confirmation?. I'll make sure atleast one of it is done by tomorrow.

I'm sorry for the inconvenience caused.

Thank you,

Best Regards,
Sai

@saran-t
Copy link
Member

saran-t commented Sep 20, 2023

Yeah no problem, I'll get the review started under the assumption that LICENSE is added.

@saikishor
Copy link
Contributor Author

Thanks for that🙌🏽🙌🏽

@saran-t
Copy link
Member

saran-t commented Sep 20, 2023

The GitHub Actions build is failing which suggests that the physics blows up. We'll take a look at this at some point but perhaps you should also take a look at the failure.

@saikishor
Copy link
Contributor Author

@saran-t Thanks for reporting the issue. Where can I find these scripts to test the physics?. On my machine, upon launching it seems stable.

@saran-t
Copy link
Member

saran-t commented Sep 21, 2023

This is the basic test that we run.

@saran-t
Copy link
Member

saran-t commented Sep 21, 2023

One obvious issue is that you have three actuators attached to each joint: <position>, <velocity>, and <motor>. These are all applying torques concurrently.

If you want to provide three control options for your robot models, you should have a base XML file without any actuator. Then create one XML for each actuation type, <include> the base XML, then add actuators there.

Additionally, your kp for position controllers are extremely high. Have these been identified against the real hardware at all?

@saikishor
Copy link
Contributor Author

Hello @saran-t,

We have updated the licenses on our repo: TIAGo and TIAGo++ robot models. I've attached the links for you to look over.

Regarding the issue with the actuators, sorry we weren't aware of this. We can refactor it to have one for each control mode. Regarding the Kp, unfortunately, those were not identified w.r.t to the real hardware, however, we tuned them until the robot showed some stable behavior in the simulation. That's how we have done also for our Gazebo models. Do you have any suggestions in this regard? We really appreciate your feedback on this.

Thank you,

Best Regards,
Sai Kishor Kothakota

@saikishor
Copy link
Contributor Author

Hello @saran-t,

I've updated the structure so that there is one scene file per control mode. I ran the model_test.py and now it is passing without issues. Thanks for the guidance.

Running tests under Python 3.8.10: /usr/bin/python
[ RUN      ] ModelsTest.test_compiles_and_steps0 (PosixPath('shadow_hand/scene_left.xml'))
[       OK ] ModelsTest.test_compiles_and_steps0 (PosixPath('shadow_hand/scene_left.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps1 (PosixPath('shadow_hand/scene_right.xml'))
[       OK ] ModelsTest.test_compiles_and_steps1 (PosixPath('shadow_hand/scene_right.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps10 (PosixPath('wonik_allegro/scene_right.xml'))
[       OK ] ModelsTest.test_compiles_and_steps10 (PosixPath('wonik_allegro/scene_right.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps11 (PosixPath('rethink_robotics_sawyer/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps11 (PosixPath('rethink_robotics_sawyer/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps12 (PosixPath('robotiq_2f85/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps12 (PosixPath('robotiq_2f85/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps13 (PosixPath('pal_tiago_dual/scene_motor.xml'))
[       OK ] ModelsTest.test_compiles_and_steps13 (PosixPath('pal_tiago_dual/scene_motor.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps14 (PosixPath('pal_tiago_dual/scene_velocity.xml'))
[       OK ] ModelsTest.test_compiles_and_steps14 (PosixPath('pal_tiago_dual/scene_velocity.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps15 (PosixPath('pal_tiago_dual/scene_position.xml'))
[       OK ] ModelsTest.test_compiles_and_steps15 (PosixPath('pal_tiago_dual/scene_position.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps16 (PosixPath('unitree_go1/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps16 (PosixPath('unitree_go1/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps17 (PosixPath('agility_cassie/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps17 (PosixPath('agility_cassie/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps18 (PosixPath('robotis_op3/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps18 (PosixPath('robotis_op3/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps19 (PosixPath('anybotics_anymal_b/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps19 (PosixPath('anybotics_anymal_b/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps2 (PosixPath('pal_tiago/scene_motor.xml'))
[       OK ] ModelsTest.test_compiles_and_steps2 (PosixPath('pal_tiago/scene_motor.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps20 (PosixPath('anybotics_anymal_c/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps20 (PosixPath('anybotics_anymal_c/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps3 (PosixPath('pal_tiago/scene_velocity.xml'))
[       OK ] ModelsTest.test_compiles_and_steps3 (PosixPath('pal_tiago/scene_velocity.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps4 (PosixPath('pal_tiago/scene_position.xml'))
[       OK ] ModelsTest.test_compiles_and_steps4 (PosixPath('pal_tiago/scene_position.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps5 (PosixPath('franka_emika_panda/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps5 (PosixPath('franka_emika_panda/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps6 (PosixPath('unitree_a1/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps6 (PosixPath('unitree_a1/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps7 (PosixPath('universal_robots_ur5e/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps7 (PosixPath('universal_robots_ur5e/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps8 (PosixPath('kuka_iiwa_14/scene.xml'))
[       OK ] ModelsTest.test_compiles_and_steps8 (PosixPath('kuka_iiwa_14/scene.xml'))
[ RUN      ] ModelsTest.test_compiles_and_steps9 (PosixPath('wonik_allegro/scene_left.xml'))
[       OK ] ModelsTest.test_compiles_and_steps9 (PosixPath('wonik_allegro/scene_left.xml'))
----------------------------------------------------------------------
Ran 21 tests in 8.311s

OK

Regarding the failing CLA check, it seems like our IT department has disabled the email of my intern who was working on this few months back. Is that a problem?

Thank you,

Best Regards,
Sai Kishor Kothakota

@saikishor
Copy link
Contributor Author

Hello @saran-t!

We believe the PR is good for review now. I have fixed all the things that are requested by you.

Thank you,

Best Regards,
Sai Kishor

@kevinzakka
Copy link
Collaborator

Hi @saikishor, I'll be taking over the review from here. Will go over this as soon as I can and leave some comments.

@saikishor
Copy link
Contributor Author

Hello @kevinzakka!

Thank you for letting me know.

Best Regards,
Sai Kishor


<compiler angle="radian" meshdir="./assets/" autolimits="true"/>

<option timestep="0.001" integrator="implicit" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to implicitfast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<option timestep="0.001" integrator="implicit" />

<asset>
<mesh name="sick_tim551" file="sensors/sick_tim551.stl"/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get rid of all the name attributes below. MuJoCo will automatically extract and use the filename from the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 31 to 48
<default class="main">
<geom contype="0" conaffinity="1" group="1"/>
<default class="arm">
<geom contype="1" conaffinity="1"/>
</default>
</default>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the XML file more compact, we should really be using the default classes more. Can you take a look at some of the XML files in the other Menagerie repos to see how we make use of defaults? Specifically, geoms, joints, actuators, etc. can be given defaults to significantly reduce the worldbody definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @kevinzakka!

This might take some time, I've fixed most of your other comments, I'll try to get this done soon. Please let me know if you have more comments.

Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinzakka I've also applied the default class changes to reduce the definitions you mentioned. I think the PR is ready for another review.

</default>

<worldbody>
<body name="base_link" pos="0 0 0">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove pos="0 0 0", it is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<worldbody>
<body name="base_link" pos="0 0 0">
<joint name="reference" type="free"/>
<geom pos="0 0 0.0985" type="mesh" density="0" rgba="1 1 1 1" mesh="base"/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move all the rgba definitions into materials and define them in <assets>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<body name="suspension_right_link" pos="0 0 0.0985">
<inertial pos="0 0 -0.02" mass="10" diaginertia="1 1 1"/>
<joint name="suspension_right_joint" pos="0 0 0" axis="0 0 1" type="slide" range="-0.005 0.005" damping="100"/>
<body name="wheel_right_link" pos="0 -0.2022 0" quat="0.707107 -0.707107 0 0">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MuJoCo will normalize quaternions for you. Therefore, for quats like .7 -0.7 0 0, you can more cleanly write it as 1 -1 0 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<inertial pos="-0.000173894 0.000176395 0.0817355"
quat="0.997803 -0.00177536 -0.00128747 0.0662101" mass="1.00276"
diaginertia="0.0025841 0.0022568 0.000746434"/>
<joint name="arm_7_joint" pos="0 0 0" axis="0 0 1" range="-2.0944 2.0944" damping="1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all the pos="0 0 0" in joint definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kevinzakka
Copy link
Collaborator

Hi @saikishor, I left some review comments.

@kevinzakka
Copy link
Collaborator

Hi @saikishor, we should try to get this submitted again. Let me know when I should review.

@saikishor
Copy link
Contributor Author

Hi @saikishor, we should try to get this submitted again. Let me know when I should review.

Hello @kevinzakka!

Thanks for responding. We are currently working on the changes. Sorry for the delay. My colleague @thomaspeyrucain is working on it. We will get back to you, once we have it ready. I will mark it as a draft until then.

Thank you

@saikishor saikishor marked this pull request as draft August 2, 2024 12:05
@saikishor saikishor marked this pull request as ready for review September 4, 2024 10:33
@saikishor
Copy link
Contributor Author

Hello @kevinzakka!

This PR is also ready for review now! We have addressed most of the comments.

Thank you!

Copy link
Collaborator

@kevinzakka kevinzakka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @saikishor, we're working on merging the other models and hope to have everything merged by end of week!

@saikishor
Copy link
Contributor Author

Thank @saikishor, we're working on merging the other models and hope to have everything merged by end of week!

Amazing! Thank you so much for all the help! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants