-
Notifications
You must be signed in to change notification settings - Fork 112
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
Update dynamic properties #195
Update dynamic properties #195
Conversation
9b63c35
to
5f7fd4d
Compare
5f7fd4d
to
789e258
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a first glimpse this looks good, thank you very much. Did you test those values with a simulator, e.g. Gazebo?
It could be nice if e.g. @VinDp have time to test them in GZ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased this locally to rolling and manually tested a ur3, ur3e, ur5, ur5e, ur10, ur10e, ur16e, ur20, ue30 with gz sim. I can confirm that the robot doesn't explode there and moves as expected.
The inertia look a little funny, especially the orientation of the visualizations of the shoulder and wrist1, wrist2. I am not an expert on simulation / dynamics calculation. Does this look correct to you?
Also, please rebase this branch onto the rolling
branch and change the PR's base to rolling
. It should be a straightforward backport that we can trigger automatically as soon as it is merged to rolling.
Content-wise I only reviewed the ur10, but the comments there should probably be valid for all models. Please also have a look at the masses as we've just been updating them in #187. For example, this PR currently proposes to change the ur20's wrist_3 mass to the value of the ur30. While I am writing this it seems strange to me, that those should be different. Is that information on https://www.universal-robots.com/articles/ur/application-installation/dh-parameters-for-calculations-of-kinematics-and-dynamics/ correct? Looking at the config files on URSim it indeed seems to be the value you used in this PR. One of them is probably wrong.
@fmauch Thank you for testing these in Gazebo sim for me. Very appreciated. I'm not sure how to change to rolling. I was just thinking that we could cherry-pick or merge from humble from rolling. Perhaps, you can guide me once everything is reviewed and accepted. |
I tried that locally and I could simply rebase your branch on rolling without any further modifications. As written, we introduce changes on rolling and then backport to all distros where we want to make that change available, in this case all 4 distributions. I'm happy to assist / do the rebase once everything is done (which I guess it is by now, so I'll take another look) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good to me, thank you very much for your efforts. As discussed earlier I will rebase this onto rolling and backport it.
y: 0.0 # model.y | ||
z: -0.0293 # model.z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The online documentation says different about this, as well. Your values match the values in URSim, though.
590f711
to
e9efbdd
Compare
* Remove unused dh_parameters * Rename prop__wrist_2_cog to prop_wrist_2_cog for consistency * Update mass and cog and use full inertia matrix. * Update max_efforts (torque limits) * Remove code and values that are no longer needed. --------- Co-authored-by: Rune Søe-Knudsen <[email protected]> (cherry picked from commit 60e6974)
* Remove unused dh_parameters * Rename prop__wrist_2_cog to prop_wrist_2_cog for consistency * Update mass and cog and use full inertia matrix. * Update max_efforts (torque limits) * Remove code and values that are no longer needed. --------- Co-authored-by: Rune Søe-Knudsen <[email protected]> (cherry picked from commit 60e6974)
* Remove unused dh_parameters * Rename prop__wrist_2_cog to prop_wrist_2_cog for consistency * Update mass and cog and use full inertia matrix. * Update max_efforts (torque limits) * Remove code and values that are no longer needed. --------- Co-authored-by: Rune Søe-Knudsen <[email protected]>
* Remove unused dh_parameters * Rename prop__wrist_2_cog to prop_wrist_2_cog for consistency * Update mass and cog and use full inertia matrix. * Update max_efforts (torque limits) * Remove code and values that are no longer needed. --------- Co-authored-by: Rune Søe-Knudsen <[email protected]>
Update some dynamic properties of the robots to reflect the most recent values. Note that the inertia matrices of UR3, UR3e, UR5, and UR5e are generated by the cylindrical inertia, since most of the values provided by UR are zeros and could potentially cause simulation to blow up. Those quantities may be updated later when appropriate values are found.