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.

Sign up for GitHub

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

Jump to bottom

Parallel gripper controller #3246

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Parallel gripper controller #3246

wants to merge 6 commits into from

Conversation

MarqRazz
Copy link
Contributor

Description

This PR closes #3017 and adds the option to control grippers that use the ros2_control ParallelGripperCommand interface. I renamed all of the existing gripper controller files to include the name gripper_command and created new files for the parallel_gripper_command_controller.

To setup add the following to your moveit_controllers.yaml

right_arm_gripper_controller:
  type: ParallelGripperCommand
  joints:
     - right_arm_gripper_joint
  action_ns: gripper_cmd
  allow_stalling: true # optional if you want to pick grasp objects of unknown size and want the request to succeed when the gripper stalls while grasping the object
  max_effort: 1.0 # optional, max effort of the joint allowed while moving (Newtons or Newton-meters)
  max_velocity: 0.5 # optional, max velocity of the joint allowed while moving (radians or meters / second)
  default: true

When you start MoveIt you should see the following when adding the controllers

[move_group-4] [move_group.moveit.moveit.plugins.simple_controller_manager]: Added ParallelGripperCommand controller for right_arm_gripper_controller

parallel_gripper_command_controller

mikeferguson
mikeferguson reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

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

I'll take a closer look later today

moveit_plugins/moveit_simple_controller_manager/src/moveit_simple_controller_manager.cpp Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2025 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 9.47368% with 86 lines in your changes missing coverage. Please review.

Project coverage is 45.57%. Comparing base (415a4a2) to head (d23ee2d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ger/parallel_gripper_command_controller_handle.hpp 0.00% 56 Missing ⚠️
...r_manager/src/moveit_simple_controller_manager.cpp 32.00% 17 Missing ⚠️
...src/parallel_gripper_command_controller_plugin.cpp 0.00% 5 Missing ⚠️
...ller_manager/gripper_command_controller_handle.hpp 16.67% 5 Missing ⚠️
...nterface/src/gripper_command_controller_plugin.cpp 0.00% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3246      +/-   ##
==========================================
- Coverage   45.65%   45.57%   -0.07%     
==========================================
  Files         714      716       +2     
  Lines       62298    62374      +76     
  Branches     7532     7541       +9     
==========================================
- Hits        28437    28422      -15     
- Misses      33695    33785      +90     
- Partials      166      167       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JafarAbdi JafarAbdi self-assigned this Jan 15, 2025
@sea-bass sea-bass added the backport-jazzy Mergify label that triggers a PR backport to Jazzy label Jan 16, 2025
sea-bass
sea-bass reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@sea-bass sea-bass left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some minor comments.

The major comment is a totally new thing that needs to happen here, which is ensuring that this type of controller is included in the MoveIt Setup Assistant.

There's probably a few places in the moveit_setup_assistant/moveit_setup_controllers package to edit -- for example, this and this

Looking at the code, it seems MSA never added the effort_controllers/GripperActionController either, but... eh, probably not needed since that's also deprecated in Jazzy.

moveit_plugins/moveit_ros_control_interface/src/parallel_gripper_command_controller_plugin.cpp Outdated Show resolved Hide resolved
moveit_plugins/moveit_ros_control_interface/src/parallel_gripper_command_controller_plugin.cpp Show resolved Hide resolved
...ager/include/moveit_simple_controller_manager/parallel_gripper_command_controller_handle.hpp Outdated Show resolved Hide resolved
...ager/include/moveit_simple_controller_manager/parallel_gripper_command_controller_handle.hpp Outdated Show resolved Hide resolved
...ager/include/moveit_simple_controller_manager/parallel_gripper_command_controller_handle.hpp Outdated Show resolved Hide resolved
moveit_plugins/moveit_simple_controller_manager/src/moveit_simple_controller_manager.cpp Outdated Show resolved Hide resolved
moveit_plugins/moveit_simple_controller_manager/src/moveit_simple_controller_manager.cpp Outdated Show resolved Hide resolved
@MarqRazz
Copy link
Contributor Author

@sea-bass thanks for the detailed review, I appreciate the feedback!

I will try to get to the MSA stuff as soon as I can, but if you are okay to push this off to a different PR then I think this is good to go. I personally don't use the MSA because it makes messy configs.

sea-bass
sea-bass approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

You can do the MSA stuff after -- would definitely wait for @JafarAbdi and @mikeferguson to comment though, since they said they would.

JafarAbdi
JafarAbdi reviewed Jan 18, 2025
View reviewed changes
...ager/include/moveit_simple_controller_manager/parallel_gripper_command_controller_handle.hpp Outdated
/*
* The joints to command in the ParallelGripperCommand message
*/
std::set<std::string> command_joints_;
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to have it as a set? Looking at https://github.com/ros-controls/ros2_controllers/blob/master/parallel_gripper_controller/src/gripper_action_controller_parameters.yaml user can only specify one joint. It will simplify the rest of the code as well. Happy to help with refactoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I assumed because this used a JointState command it was able to support multi-dof grippers. I updated the code to only support one joint, please let me know if you like the updated changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JafarAbdi JafarAbdi JafarAbdi left review comments

@sea-bass sea-bass sea-bass approved these changes

@mikeferguson mikeferguson mikeferguson approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@JafarAbdi JafarAbdi

Labels
backport-jazzy Mergify label that triggers a PR backport to Jazzy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add missing ParallelGripperCommand controller type
5 participants
@MarqRazz @codecov-commenter @mikeferguson @sea-bass @JafarAbdi