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 MoveIt Servo Tutorial #688

Open
wants to merge 10 commits into
base: noetic-devel
Choose a base branch
from

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 🙈

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.

2 participants