UniversalRobots / Universal_Robots_ROS2_Description

ROS2 URDF description for Universal Robots
BSD 3-Clause "New" or "Revised" License
110 stars 107 forks source link

Flatten macro call structure #52

Closed hellantos closed 9 months ago

hellantos commented 1 year ago

Problem Description

The xacro structure of this description package does not allow to use the robot model without the ros2_control model. The call structure is:

ur.urdf.xacro -> xacro:ur_robot-> xacro:ur_ros2_control

This breaks with the common practice in other packages (e.g. abb_ros2 or franka_ros2).

robot.urdf.xacro
   -> xacro:robot
   -> xacro:robot_ros2_control

We have a few applications where we really don't wnat the ros2_control part in the model and would like to be able to instantiate a plain urdf model. This is impossible with the current structure. Also this confuses tools such as moveit_setup_assistant.

Problem Solution

Move to the common practice of flat macro call hierarchy and instantiate the xacro:ur_ros2_control seperately from xacro:ur_robot.

ur.urdf.xacro
   -> xacro:read_model_data
   -> xacro:ur_robot
   -> xacro:ur_ros2_control

This PR contains a POC which instantiates ros2_control directly in ur.urdf.xacro. Did only test with fake hardware for now.

gavanderhoorn commented 1 year ago

Similar to what I suggested over at https://github.com/StoglRobotics-forks/kuka_experimental/pull/11#discussion_r1114516528.

You could go even further and refactor the ros2_control dependency out into a separate package.

destogl commented 1 year ago

Agree, but I would not go as far as abstracting ros2_control out of it. I don't there is sufficient added value for added complexity.

For the KUKA it made sense because it is a new port.

gavanderhoorn commented 1 year ago

For the KUKA it made sense because it is a new port.

just calling it out: I would say it makes sense in general, as due the way ros2_control works, users must make changes to the actual .xacro files to make changes to their ros2_control configuration. There is no way around it.

See the various PRs against this repository and the ROS 1 version for multi-robot support fi.

As an example: changes to the base .xacro files to able to spawn two instances seems like an indication of a violation of separation of concerns, something the approach suggested by @ipa-cmh and myself (and it seems @danzimmerman in https://github.com/ipa-cmh/Universal_Robots_ROS2_Description/pull/1 as well) would largely avoid.

fmauch commented 1 year ago

I indeed so a point in the concerns raised here. At least for Rolling I don't see any problem in factoring this into an own package. @RobertWilbrandt what do you think?

fmauch commented 1 year ago

I've added a playground of splitting out the ros2_control part in #59, but I don't like it so far. I'd like to keep as much as possible inside the ur_macro.xacro, since the idea of the ur.urdf.xacro was basically to not be more than specifying parameters and actually creating an instance of the robot macro itself into the world. Doing it the way as proposed in this PR bloats up the ur.urdf.xacro file when keeping everything here, but leads to a lot of code duplication when splitting things into ros2_control, gazebo, etc. packages.

fmauch commented 9 months ago

Closing this in favor of #114