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 Kuka KR 16 R1610 cybertech support #146

Open
wants to merge 11 commits into
base: melodic-devel
Choose a base branch
from

Conversation

simonschmeisser
Copy link
Collaborator

This adds support for the Kuka cybertech KR16 R1610 model. It does not have that much in common with the Kuka KR16-2 already supported. Therefore I added completely separate packages.

In the technical specification [1] it is referred to as "kr16 r1610" ie omitting the "cybertech" part. I added it to the package name anyway as it helps in distinguishing the product lines. We will test this one on real hardware starting next week so it should not be merged yet. I would however be grateful about naming advice and therefore open the PR already.

https://www.kuka.com/-/media/kuka-downloads/imported/48ec812b1b2947898ac2598aff70abc0/spez_kr_cybertech_en.pdf

@simonschmeisser
Copy link
Collaborator Author

Just a small update: this package has been used and tested extensible now.

The ikfast plugin was created on kinetic and seems to create problems here on indigo (C++98vsC++11?)

@simonschmeisser
Copy link
Collaborator Author

@gavanderhoorn @BrettHemes I would be really grateful for some feedback on naming. We have a current request from a client for the new Kuka Ionic KR70 R2100 and same as here I'm wondering if it should be kuka_cybertech_kr16_support or an addition to kuka_kr16_support or similarily for the Ionic kuka_ionic_kr70_support vs kuka_kr70_support

@BrettHemes
Copy link
Member

Not really sure here, but we haven't used the informal series names anywhere else in the package names (e.g. Agilus). They are, I believe, mentioned in the manifests. I think it comes down to preference. I would vote for consistency, however.

@simonschmeisser simonschmeisser changed the base branch from indigo-devel to melodic-devel September 29, 2021 20:24
@simonschmeisser simonschmeisser changed the title Add Kuka cybertech KR16 R1610 support Add Kuka KR 16 R1610 cybertech support Sep 30, 2021
@simonschmeisser
Copy link
Collaborator Author

This now follows the naming standards of having the variant last. I think the cybertech should not be omitted as there is also a KR 16 R1610-2 (PR upcoming ...)

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Sep 30, 2021

CMakeLists.txt and the package.xml appear to be in a kuka_cybertech_kr16_r1610_support directory according to github?

And roslaunch_test.xml as well.

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.

High-level comment (which I've also placed in-line): I'd like us to consider adding an underscore before the family modifier, so we'd get kuka_kr16r1610_cybertech.

I've left some other in-line comments.

@@ -9,6 +9,7 @@ catkin_package()
if (CATKIN_ENABLE_TESTING)
find_package(roslaunch REQUIRED)
roslaunch_add_file_check(test/roslaunch_test.xml)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please also rename this test?

kuka_kr16_support/package.xml Outdated Show resolved Hide resolved
kuka_kr16_support/package.xml Outdated Show resolved Hide resolved
kuka_kr16_support/urdf/kr16r1610cybertech_macro.xacro Outdated Show resolved Hide resolved
kuka_kr16_support/urdf/kr16r1610cybertech_macro.xacro Outdated Show resolved Hide resolved
kuka_kr16_support/urdf/kr16r1610cybertech_macro.xacro Outdated Show resolved Hide resolved
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.

3 participants