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

Flatten macro call structure #52

Closed
wants to merge 1 commit into from

Conversation

hellantos
Copy link

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.

Signed-off-by: Christoph Hellmann Santos <[email protected]>
@gavanderhoorn
Copy link

Similar to what I suggested over at StoglRobotics-forks/kuka_experimental#11 (comment).

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

@destogl
Copy link
Contributor

destogl commented Apr 17, 2023

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
Copy link

gavanderhoorn commented Apr 17, 2023

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 hellantos#1 as well) would largely avoid.

@fmauch
Copy link
Contributor

fmauch commented Apr 17, 2023

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
Copy link
Contributor

fmauch commented Apr 17, 2023

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
Copy link
Contributor

fmauch commented Dec 18, 2023

Closing this in favor of #114

@fmauch fmauch closed this Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants