Kawasaki-Robotics / khi_robot

ROS KHI robot meta-package
http://wiki.ros.org/khi_robot
BSD 3-Clause "New" or "Revised" License
55 stars 28 forks source link

Robot xacros not macros #1

Closed gavanderhoorn closed 5 years ago

gavanderhoorn commented 5 years ago

Thank you very much for releasing ROS support for Kawasaki robots :bowing_man: :+1:

While looking at the rs_description package, I noticed that the .urdf.xacro files do not include any xacro:macros of the robots (this one for the RS007 fi).

This makes it difficult to use the robot model in a larger scene, as it cannot be easily composed with other xacros.

Would you consider merging a Pull Request that transforms the .urdf.xacros into xacro:macros? If the macros are then stored in a separate file, the .urdf.xacro could xacro:include those and then call the macro (and thereby instantiate the model).

The public API of rs_description would not change, but by including the file that contains the xacro:macro, using the Kawasaki robot models in a larger work cell would become possible.

gavanderhoorn commented 5 years ago

As an example for discussion: https://github.com/Kawasaki-Robotics/khi_robot/compare/master...gavanderhoorn:add_xacro_macros.

I've kept the world link as part of the top-level xacro (ie: rs007l.urdf.xacro), so as to not collide with other .xacros that may already have a world link (making composition impossible again).


Edit: and please find some additional suggestions for changes to the .xacro of the RS 007L here: https://github.com/gavanderhoorn/khi_robot/compare/add_xacro_macros...gavanderhoorn:suggested_changes.

These are mostly small convenience changes that make it easier to use the robot model macro in other xacros (avoid naming conflicts for instance) and to make better use of some Jade+ xacro features (such as radians(..)).

I'll submit a PR with these changes at a later time.

matsui-hiro commented 5 years ago

Thank you for reporting the issue. As you say, it is difficult to model multiple robots with the same cell if they are not xacro: macro. I will adopt your plan.

gavanderhoorn commented 5 years ago

2 was merged, so this has been addressed.

A follow-up PR to make the xacros really composable will be submitted.