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

Fix Panda inertials, add SRDF #147

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

wxmerkt
Copy link
Contributor

@wxmerkt wxmerkt commented Dec 12, 2022

Fixes #122

wxmerkt and others added 3 commits December 12, 2022 16:18
… Gazebo URDF

Created the Gazebo URDF from upstream franka_description package and
then manually merged the inertials and new TCP frame into our packaged
URDF. Should fix Gepetto#122

Note: There are two types of URDFs in the upstream repository. One also
has simplified, larger collision shapes. Since we possibly want to
simulate using the URDFs here, I kept the configuration using the convex
meshes rather than primitive shapes.
Copy link
Member

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

You need to prepend example-robot-data/robots/ to mesh paths.

Also, I think this is a good thing to update this inertia, but I'm afraid this might break benchmarks or other reproducible tests relying on the current values. But I guess we'll only have to advertise this correctly in release notes.

Copy link
Collaborator

@cmastalli cmastalli left a comment

Choose a reason for hiding this comment

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

Thank @wxmerkt for taking care of this issue.
I have left some comments about the PR.

robots/panda_description/urdf/panda.urdf Outdated Show resolved Hide resolved
robots/panda_description/urdf/panda.urdf Show resolved Hide resolved
robots/panda_description/urdf/panda.urdf Show resolved Hide resolved
@wxmerkt
Copy link
Contributor Author

wxmerkt commented Dec 12, 2022

Also, I think this is a good thing to update this inertia, but I'm afraid this might break benchmarks or other reproducible tests relying on the current values. But I guess we'll only have to advertise this correctly in release notes.

I don't think we need to worry about this: We haven't used it in benchmarks yet to my knowledge - and couldn't due to the zero inertials - i.e. that was the reason behind #122 and the original issue upstream in Crocoddyl about failed optimisation (due to zero inertials).

Copy link
Collaborator

@cmastalli cmastalli left a comment

Choose a reason for hiding this comment

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

I don't have any further concern.

Copy link
Member

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

You're right, we couldn't, it's perfect for me then :)

@cmastalli cmastalli merged commit aaa2584 into Gepetto:devel Dec 12, 2022
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.

3 participants