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

added support for R900 #134

Open
wants to merge 1 commit into
base: melodic-devel
Choose a base branch
from

Conversation

fmauch
Copy link

@fmauch fmauch commented Jun 18, 2018

This MR contains meshes and URDF for the kr10r900 model. I'm not that much of a meshing pro, which is why the collision meshes might be further reduced. They currently are the convex hull of the real meshes.

@gavanderhoorn
Copy link
Member

Hi, thanks for the PR. Contributing a new model like this is always appreciated.

I'll add some high-level comments in a first review.

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Could I ask you to look at the following comments? Thanks.

  • please use the new features in xacro such as support for inline math and Python radians support. See pull/82/files for how that would change your files.
  • Don't forget to add --inorder to your load_...launch file.
  • Also remove the .py extension from xacro.py.
  • we'd like to keep things alphabetically sorted, so could I ask you to please re-order listings and explanatory text to list the R900 first?
  • I don't believe the robot_state_publisher has a use_gui parameter
  • please keep values for xmlns:xacro consistent and use the new url: http://wiki.ros.org/xacro
  • if you can, try and see whether the new variant you add has any meshes it shares with the R1100. Most of the times base_link, link_1, link_3, link_5 and link_6 can be shared between variants that only differ in reach.
  • please keep rviz enabled in test_...launch files. Those files are actually intended to launch RViz and allow you to inspect the URDF. Without RViz, that is rather difficult.

@BrettHemes
Copy link
Member

Also, FYI... I believe the meshes for the KR6r900 (existing) and KR10r900 are the same.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 18, 2018

If that is the case then they should probably be wholesale re-used.

Connects: #105.


Edit: if these robots are the same except for payload, then we may just skip adding this variant and declare the kr10r900 supported via the kr6r900. If joint limits are different, we might really want to look into parameterising these xacro macros using yaml files.

@fmauch
Copy link
Author

fmauch commented Jul 3, 2018

in fact, after applying the requested changes, the two xacro files are almost identical except for the kr6 one using the legacy DEG2RAD conversion. I'll adapt the kr6 one then and use the macro for the kr10, right?

@gavanderhoorn
Copy link
Member

Hm, yes, that does seem like a good way to do this.

Thanks for staying on top of this and iterating.

This does really connect to the discussion in #105.

@fmauch
Copy link
Author

fmauch commented Jul 3, 2018

now, there is only one macro left shared by the kr6r900 and the kr10r900.

@fmauch
Copy link
Author

fmauch commented Dec 13, 2018

Hi there, I had a quick look again and I think all from the list has been integrated into the MR. However, the CI pipeline fails as it is being executed on indigo, which doesn't have the radians capability, for example. @gavanderhoorn do you want to have another look?

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Dec 13, 2018

However, the CI pipeline fails as it is being executed on indigo, which doesn't have the radians capability, for example. @gavanderhoorn do you want to have another look?

Could it be that you missed this bullet in the review?

  • Don't forget to add --inorder to your load_...launch file.

That would be needed to enable Jade+ xacro functionality.


Edit: hm, strange. The launch file does add that, I'll need to check the CI result.

@fmauch
Copy link
Author

fmauch commented Dec 13, 2018

well, it is the kr6r900-file that fails. And that doesn't have the --inorder notation. I can change it there, as well.

@gavanderhoorn
Copy link
Member

well, it is the kr6r900-file that fails. And that doesn't have the --inorder notation. I can change it there, as well.

Ah. I missed that. Yes, that would be required.

@gavanderhoorn
Copy link
Member

Is your @fzi.de address not associated with your GH account? The commits are showing up unattributed to @fmauch.

@fmauch
Copy link
Author

fmauch commented Dec 13, 2018

Is your @fzi.de address not associated with your GH account? The commits are showing up unattributed to @fmauch.

I'll have to check that...

@gavanderhoorn
Copy link
Member

Is your @fzi.de address not associated with your GH account? The commits are showing up unattributed to @fmauch.

I'll have to check that...

Would you want to wait with merging this PR until after you've fixed this?

@fmauch
Copy link
Author

fmauch commented Dec 13, 2018 via email

@fmauch
Copy link
Author

fmauch commented Dec 13, 2018

Please merge it as is.

@simonschmeisser
Copy link
Collaborator

What's the status on this PR?

@simonschmeisser
Copy link
Collaborator

simonschmeisser commented Dec 11, 2019

Oh and do you know if there is a difference between KR10 R900-2 and KR10 R900 sixx? Besides color? I'm getting crazy with the Kuka naming schemes!


To answer my own question: KR agilus sixx and KR agilus-2 are different series so KR10 R900-2 and KR10 R900 sixx are not equivalent and I'll open a separate PR once I modeled this

@fmauch
Copy link
Author

fmauch commented Dec 12, 2019

I totally forgot about that one. I just rebased and squashed it. @gavanderhoorn I think, your requested changes are all in, right?

@simonschmeisser simonschmeisser changed the base branch from indigo-devel to melodic-devel September 23, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants