Skip to content

Conversation

@tahsinkose
Copy link

This PR adds two usage examples of MoveIt Servo with UR5 robot in the new ur5_moveit_tutorials package

@tahsinkose
Copy link
Author

@RobertWilbrandt @fmauch could you please have a look at this PR?

Copy link
Contributor

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

It would have been very nice to have this a few years ago. Personally I'd still like to see it merged after the changes get applied. I guess most of the community's efforts go towards ROS2 now, for better or worse...

Comment on lines 6 to 9
catkin_package(
)
include_directories(
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
catkin_package(
)
include_directories(
)
catkin_package()

<!-- Load and start the controllers listed in the 'controllers' arg. -->
<node name="ros_control_controller_spawner" pkg="controller_manager" type="spawner"
args="$(arg controllers)" output="screen" respawn="false" />
<group unless="$(eval arg('controllers') == '')">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any reason for group if there's just one node in the group? Suggest reverting.

Copy link
Author

Choose a reason for hiding this comment

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

I should actually have used if clause instead of this 😄

@tahsinkose tahsinkose force-pushed the tahsin/add-moveit-servo-tutorial branch from d0913a8 to 1e65d66 Compare November 2, 2024 13:54
@tahsinkose
Copy link
Author

tahsinkose commented Nov 2, 2024

@AndyZe addressed your review, PTAL 🙏🏼

It would have been very nice to have this a few years ago. Personally I'd still like to see it merged after the changes get applied. I guess most of the community's efforts go towards ROS2 now, for better or worse...

I can migrate them to ROS 2 when I finally switch to ROS 2 🙈

@tahsinkose
Copy link
Author

@AndyZe friendly ping 🙂

@AndyZe
Copy link
Contributor

AndyZe commented Nov 8, 2024

I don't have merge power here

@tahsinkose
Copy link
Author

I don't have merge power here

Who should we reach then? Do you know that @AndyZe?

@tahsinkose
Copy link
Author

@urfeex could you review this PR and possibly merge?

@urfeex
Copy link
Collaborator

urfeex commented Nov 3, 2025

I'm afraid, our resources towards ROS 1 are rather limited at the moment.

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.

3 participants