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 iiwa support package and 14kg r820 version #14

Merged
merged 1 commit into from
Jul 7, 2015

Conversation

Levi-Armstrong
Copy link
Member

No description provided.

@@ -0,0 +1,3 @@
<launch>
<param name="robot_description" command="$(find xacro)/xacro.py '$(find kuka_lbr_iiwa_support)/urdf/lbr_iiwa_14_r820.xacro'" />
Copy link
Member

Choose a reason for hiding this comment

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

Should only be two spaces.

@gavanderhoorn
Copy link
Member

Do we want to include the .blend files in the support pkg? As they are in the meshes sub dir, it will increase the size of the pkg by almost 10MB. Same for the original directory.

@gavanderhoorn
Copy link
Member

Do you know what convention KUKA uses when referring to their joints/links for IIWA's? The KUKA KR 120 R2500 PRO in the kr120 support pkg uses joint_a1.

@Levi-Armstrong
Copy link
Member Author

Do you know what convention KUKA uses when referring to their joints/links for IIWA's? The KUKA KR 120 R2500 PRO in the kr120 support pkg uses joint_a1.

In the documentation they refer to the axes and joints by A1 (J1) to A7 (J7) so I think joint_1 should be correct.

@Levi-Armstrong
Copy link
Member Author

Do we want to include the .blend files in the support pkg? As they are in the meshes sub dir, it will increase the size of the pkg by almost 10MB. Same for the original directory.

I included the blend files because they are sued to create the .dae files and also noticed that they are include with the UR robots. I don't think it is absolutely necessary to keep but if there is changes that need to be made to the shading and color keeping it saves a little work. I think it would be good to at least keep them until it get moved to the released repository when one is created.

@gavanderhoorn
Copy link
Member

All files in the meshes dir have the executable bit set, please remove. The launch/test_...launch file also.

@gavanderhoorn
Copy link
Member

Do we want to include the .blend files in the support pkg? As they are in the meshes sub dir, it will increase the size of the pkg by almost 10MB. Same for the original directory.

I included the blend files because they are sued to create the .dae files and also noticed that they are include with the UR robots. I don't think it is absolutely necessary to keep but if there is changes that need to be made to the shading and color keeping it saves a little work. I think it would be good to at least keep them until it get moved to the released repository when one is created.

We can keep them, but lets treat them like source artefacts (ie: don't install them). To avoid installing / distributing them, we should either change the install(..) statements in the CMakeLists.txt to not install the entire meshes dir, or move the original and blender dirs to somewhere outside meshes. Perhaps something like meshes_originals or meshes_srcs.

The meshes dir is 29MB right now, just for this single model.

@gavanderhoorn
Copy link
Member

Do you know what convention KUKA uses when referring to their joints/links for IIWA's? The KUKA KR 120 R2500 PRO in the kr120 support pkg uses joint_a1.

In the documentation they refer to the axes and joints by A1 (J1) to A7 (J7) so I think joint_1 should be correct.

:). The question was: the KR120 urdf I linked uses joint_aN, exactly because KUKA uses A1, A2 etc. See the discussion here.

This is all in experimental pkgs, so we should perhaps decide (again) whether we want to follow the mfg's naming or always use joint_N (with N an integer), regardless of what the mfg does.

Following mfg naming may be more intuitive for users used to that. However, standardising joint/link (and thus TF frame) names could be beneficial.

The various motoman support pkgs do use Motoman's naming scheme.

<p>
Joint limits and max joint velocities are based on the information in
the KUKA data sheets online.
http://www.kuka-robotics.com/res/sps/e6c77545-9030-49b1-93f5-4d17c92173aa_Spez_LBR_iiwa_en.pdf
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a link. Something like <a href="http://www.kuka-robotics.com/..">data sheets</a>.

Or if the datasheet actually has a document nr, use that for the link text.

@gavanderhoorn
Copy link
Member

One more thing: collision models do not appear to be convex hulls. I guess that is because of the geometry of the links? Would it make sense to decimate the stls a bit anyway? base_link fi has ~9k vertices and ~20k faces, the convex hull has ~450 and ~900 respectively. That is a large difference.

@Levi-Armstrong
Copy link
Member Author

@gavanderhoorn, I believe I have made all the requested changes except the collision mesh decimation. I currently need the fidelity of the collision model for my application, but I can decimate the collision geometry and add a parameter in the urdf xacro to not include the collision tag to easily switch between using the coarse collision geometry and using the fine visual model for collision if that is acceptable?

@Levi-Armstrong
Copy link
Member Author

@gavanderhoorn, Will this work?

@gavanderhoorn
Copy link
Member

Sorry @Levi-Armstrong, I'm currently travelling, so I couldn't respond properly. Thought I'd wait till I was back in Delft to give you a proper answer.

I just wanted to say that decimation does not necessarily equal 'convex hull'. I'd be surprised if the meshes you included couldn't be decimated quite a bit before they started losing fidelity.

Personally I'm not really a fan of adding extra parameters to the xacros, as they are somewhat hidden if you don't know what to look for. If you absolutely need all faces in the meshes, then let's not fuss about 9MB. I just figured it might be a small bit of extra work that would get the pkg size down significantly.

Note that I haven't had a chance to look at your last commit (the github diff does show that all files have their execute bit set again ;)).

@Levi-Armstrong
Copy link
Member Author

Sorry, I am using a VM and it appears anytime I bring a file from window to the VM it has the execute bit set. Once you get back from travel let me know your thoughts on the latest commit. It adds the parameter to include the collision geometry tag or exclude it so it will use the visual mesh. Based on your comment it may be better revert the last commit and it should be good.

@gavanderhoorn
Copy link
Member

@Levi-Armstrong wrote:

Sorry, I am using a VM and it appears anytime I bring a file from window to the VM it has the execute bit set.

no problem :) I figured you were using Windows.

It adds the parameter to include the collision geometry tag or exclude it so it will use the visual mesh. Based on your comment it may be better revert the last commit and it should be good.

I'd remove the additional parameter, yes. It isn't really visible (so may become confusing) and not having a collision element isn't / wasn't really recommended anyway. There is / was also some funky business with using Collada meshes for collision geometry (which iiuc you'd be doing if you set coarse to false) so I wouldn't be too comfortable with that either.

@Levi-Armstrong
Copy link
Member Author

@gavanderhoorn, Ok, I removed the latest commit so I think all requested changes have been made.

@gavanderhoorn
Copy link
Member

Your install logic in CMakeLists.txt can be greatly simplified:

foreach(dir config launch meshes/lbr_iiwa_14_r820/collision meshes/lbr_iiwa_14_r820/visual urdf) 
  install(DIRECTORY ${dir}/
    DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}/${dir})
endforeach()

I admire the complexity of your file(GLOB ..) and foreach, but they are not necessary :).

</node>
<node name="robot_state_publisher" pkg="robot_state_publisher" type="robot_state_publisher" />
<node name="rviz" pkg="rviz" type="rviz" args="-d $(find industrial_robot_client)/config/robot_state_visualize.rviz" required="true" />
</launch>
Copy link
Member

Choose a reason for hiding this comment

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

On my local clone, this file appears to have Windows line-endings. Could you fix that?

Copy link
Member

Choose a reason for hiding this comment

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

There's also extra whitespace after the <param name="use_gui" value="true" /> line.

@gavanderhoorn
Copy link
Member

If you fix those two last issues, we can merge this.

@gavanderhoorn
Copy link
Member

O if you could squash your commits into a single "Adding support for LBR IIWA 14 R280." that would be nice.

Correct style and spacing

Rename mesh subfolder

Changed permissions and install files

Fix requested changes

Decimate collision meshes
@Levi-Armstrong
Copy link
Member Author

@gavanderhoorn, I made the additional changes and when ahead and decimating the collision meshes. Also thank you for the install code, I figured there was an easier way but I have very little experience with cmake.

@gavanderhoorn
Copy link
Member

@Levi-Armstrong: I'll merge this. Thanks for tolerating my many requests.

gavanderhoorn added a commit that referenced this pull request Jul 7, 2015
Add Kuka iiwa support package and 14kg r820 version
@gavanderhoorn gavanderhoorn merged commit 62256c3 into ros-industrial:hydro-devel Jul 7, 2015
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.

2 participants