ros-industrial / staubli_experimental

Experimental packages for Staubli manipulators within ROS-Industrial (http://wiki.ros.org/staubli_experimental)
Apache License 2.0
26 stars 27 forks source link

Tx60 inclusion #19

Closed nilsmelchert closed 6 years ago

nilsmelchert commented 6 years ago

Requesting to add the packages staubli_tx60_support and staubli_tx60_gazebo the the staubli_experimental repository.

nilsmelchert commented 6 years ago

does link_3.stl contain two links in one file?

link_3.stl looks fine to me for both versions of the robots. It looks a bit different from the TX90 series. Or did I miss out on something?

gavanderhoorn commented 6 years ago

@nilsmelchert wrote:

@gavanderhoorn:

does link_3.stl contain two links in one file?

link_3.stl looks fine to me for both versions of the robots. It looks a bit different from the TX90 series. Or did I miss out on something?

You're right. The 'strange' shape confused me.

gavanderhoorn commented 6 years ago

Your last commit does not appear to be made using a client that had your registered email address configured. That is fine, but will mean that the commit won't be attributed to your account.

nilsmelchert commented 6 years ago

I included every change that was requested. Please have a closer look at the new frames in the urdf files. There is only one translation per joint. In my opinion, the frames look kind of strange, but regarding the official datasheet, it seems to be the only way to fullfill the desired criteria of only one translation per joint. Thanks for your work!

nilsmelchert commented 6 years ago

Your last commit does not appear to be made using a client that had your registered email address configured. That is fine, but will mean that the commit won't be attributed to your account.

How can I change that?

gavanderhoorn commented 6 years ago

You can either register your other email address with github (as a second one for your current account), or you could do something like:

git commit --amend --author "Your Name <correct@email.address>" --no-edit

and then a git push $name_of_remote tx60_inclusion --force.

That will overwrite your last commit to the tx60_inclusion branch.

Be sure to check that everything is ok before you do that though. You can do that with git log -n1 or gitk.

nilsmelchert commented 6 years ago

Your last commit does not appear to be made using a client that had your registered email address configured. That is fine, but will mean that the commit won't be attributed to your account.

I did a small commit to update my email address. Thanks for the review.

gavanderhoorn commented 6 years ago

I've added a few more commits to tidy up some things.

I've reverted the changes to the mesh and joint origins that you made in 87f728d: looking at the datasheet you linked allowing the small y translation for joint_2 makes more sense than to have the entire link_2 to link_4 subtree run 'outside' the robot.

I've also removed your email address from the author tag in the manifest of tx60_gazebo (in 6ef9dab): it's more consistent with tx60_support this way. If you want to have your email address in both that's fine with me. Just be prepared to receive emails from the buildfarm if there are ever any issues with any builds.

gavanderhoorn commented 6 years ago

Other than that this is quite excellent work @nilsmelchert. Thanks for the PR and the contribution.

gavanderhoorn commented 6 years ago

One thing I noticed is that the xacros claim that link_6 weighs only 8 grams, which seems a bit on the low side, but we've added a disclaimer to the package manifest, so users should be aware.

nilsmelchert commented 6 years ago

I've also removed your email address from the author tag in the manifest of tx60_gazebo

That's fine with me.

nilsmelchert commented 6 years ago

One thing I noticed is that the xacros claim that link_6 weighs only 8 grams, which seems a bit on the low side, but we've added a disclaimer to the package manifest, so users should be aware.

That is because of the assumption of a homogeneously distributed density with was made in the stable_tx90_support package. The weights and inertias are definitely still not accurate. But as soon as I get the sytem identification done, I will contribute my results.

nilsmelchert commented 6 years ago

Other than that this is quite excellent work @nilsmelchert. Thanks for the PR and the contribution.

Very welcome. I am still new to ROS and even git. But I am sure that this was not my last contribution.

nilsmelchert commented 6 years ago

I've added a few more commits to tidy up some things.

So do I have to make some more changes on my PR? How are you able to make commits when you are not owner of the repository? Let me know, if I can do something to finish the addition of the tx60 series. I mean if you reverted the changes, the collision models also have to be adjusted again. I could definitely do that, if that still needs to be done.

And the last question: When will my PR be merged into the staubli_experimental package?

gavanderhoorn commented 6 years ago

@nilsmelchert wrote:

I've added a few more commits to tidy up some things.

So do I have to make some more changes on my PR?

No.

How are you able to make commits?

Because by default, PRs on github have allow edits from maintainers enabled.

I mean if you reverted the changes, the collision models also have to be adjusted again. I could definitely do that, if that still needs to be done.

I updated the collision meshes as well.

gavanderhoorn commented 6 years ago

I was just waiting for Travis to complete.

nilsmelchert commented 6 years ago

Ok so I guess my work is done here.

Thanks a lot for your help and reviews. I learned a lot and I am also very thankful for people like you @gavanderhoorn , who make my research so much easier.

gavanderhoorn commented 6 years ago

Thanks again @nilsmelchert :+1:

gavanderhoorn commented 6 years ago

No problem.

I look forward to the results of your system identification.

gavanderhoorn commented 6 years ago

Note that I did a squash and merge when merging your PR. It may look like those commits have not been merged, and that would be correct: to avoid all the fixup commits to become part of the history of this repository I've squashed them into a single one (e3a983c) and merged that.

Your tx60_inclusion branch will still show all those individual commits, but indigo-devel should not (if you update it from this repository).

nilsmelchert commented 6 years ago

I've squashed them into a single one (e3a983c) and merged that.

Ok good. One more thing I learned about git ;-)