Implement MaxEffortGripperActionController for Dynamixel grippers#30
Merged
Conversation
- bug 1: stop calling RealtimeBuffer::writeFromNonRT(nullptr) from the RT update loop (it takes a mutex). Replace with an RT-only velocity_cached_valid_ flag; the watchdog clears it without touching the buffer. - bug 2: previous_rt_goal_ is touched from both RT (clear_active_goal via check_for_success) and non-RT (cancel/preempt/accept/deactivate). Wrap access in store/exchange helpers using std::atomic_store / std::atomic_exchange on the shared_ptr to eliminate the data race. - bug 3: in accepted_callback, flush_previous_goal_if_any() now runs BEFORE rt_active_goal_ is replaced. Otherwise an interleaved RT check_for_success could stash the new goal into previous_rt_goal_ and the subsequent flush would discard the old goal's terminal state. - bug 4: drop the redundant last_movement_time_ assignment in accepted_callback. The RT update path is now the sole writer, removing a data race on rclcpp::Time. - cleanup: drop the dead new_message branch from the velocity helper and rename it to continue_velocity_integration_if_within_watchdog().
There was a problem hiding this comment.
Pull request overview
Adds a new terminal (non-chainable) ros2_control controller that implements control_msgs/action/GripperCommand while preserving and commanding the max_effort field (intended for Dynamixel current/torque limiting), plus topic-based position/velocity teleop inputs and is_grasped publishing.
Changes:
- Introduce
MaxEffortGripperActionControllerwith action + topic inputs, joint-limit clamping, stall detection, andis_graspedpublisher. - Add generated parameter library, pluginlib registration, and new dependencies (
control_msgs,rclcpp_action) to build/package metadata. - Add comprehensive unit tests (including action lifecycle tests) and a minimal URDF fixture; document the new controller in the README.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| hector_ros_controllers/src/max_effort_gripper_action_controller.cpp | New controller implementation (action server, topic inputs, arbitration, stall + grasp detection). |
| hector_ros_controllers/include/max_effort_gripper_action_controller/max_effort_gripper_action_controller.hpp | Public header defining controller API and internal state. |
| hector_ros_controllers/params/max_effort_gripper_action_controller_parameters.yaml | New generate_parameter_library schema for controller parameters. |
| hector_ros_controllers/hector_ros_controller_plugin.xml | Exposes the new controller as a pluginlib controller. |
| hector_ros_controllers/CMakeLists.txt | Builds/links the controller + param lib, wires new gmock test, adds deps. |
| hector_ros_controllers/package.xml | Declares new runtime dependencies for action + messages. |
| hector_ros_controllers/test/test_max_effort_gripper_action_controller.cpp | New unit tests covering arbitration, watchdog, goal lifecycle, and regressions. |
| hector_ros_controllers/test/config/test_gripper.urdf | URDF fixture used to validate joint-limit parsing/clamping. |
| README.md | Documentation for the new controller, interfaces, parameters, and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Migrate rt_active_goal_ to atomic shared_ptr (matching previous_rt_goal_); RT clear/preempt paths no longer take the RealtimeBuffer mutex. - Reject non-finite goals in goal_callback with REJECT instead of accept-then-abort; drop the NaN branch in accepted_callback. - Delete dead pre_alloc_result_ resets in accepted_callback / on_activate (check_for_success rewrites both fields anyway, and the reset could mutate a previous goal's still-held result). - Generalise goal-slot atomic helpers (store/load/exchange_goal_slot) for both slots. - Fix joint param YAML description to include the effort state interface.
There was a problem hiding this comment.
Pull request overview
Adds a new ROS 2 ros2_control controller plugin to drive single-joint grippers while honoring GripperCommand.max_effort (effort/current limit), targeting Dynamixel “current-based position” mode. This fills the gap where the upstream gripper_controllers/GripperActionController ignores max_effort in position mode.
Changes:
- Introduces
MaxEffortGripperActionControllerplugin with action + topic inputs, last-writer-wins arbitration, stall detection, and~/is_graspedpublishing. - Adds generated parameter definitions + build/package wiring (CMake, package.xml, plugin XML).
- Adds a comprehensive gmock-based test suite and a minimal URDF fixture; updates README with usage docs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
hector_ros_controllers/src/max_effort_gripper_action_controller.cpp |
Implements the new gripper controller logic (interfaces, action server, topic inputs, arbitration, watchdogs, grasp detection). |
hector_ros_controllers/include/max_effort_gripper_action_controller/max_effort_gripper_action_controller.hpp |
Declares the controller class, RT buffers, goal-handle slots, and parameters. |
hector_ros_controllers/params/max_effort_gripper_action_controller_parameters.yaml |
Defines parameters for generate_parameter_library (limits, timeouts, thresholds, rates). |
hector_ros_controllers/hector_ros_controller_plugin.xml |
Exports the new controller as a pluginlib class. |
hector_ros_controllers/CMakeLists.txt |
Builds/installs the new source + parameter library; adds the new gmock test target. |
hector_ros_controllers/package.xml |
Adds missing runtime dependencies for action + messages (control_msgs, rclcpp_action). |
hector_ros_controllers/test/test_max_effort_gripper_action_controller.cpp |
Adds extensive unit tests for arbitration, watchdog behavior, grasp detection, and action lifecycle edge cases. |
hector_ros_controllers/test/config/test_gripper.urdf |
Provides URDF joint limits fixture for tests. |
README.md |
Documents the new controller’s features, interfaces, parameters, configuration, and example usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- README: reason in the result -> reason is logged (GripperCommand Result has no reason field). - Add #include <tuple> for std::ignore (was relying on transitive). - Soften the RT-safe claim in preempt_active_goal — lock-free, but formats a log line and allocates a Result like the other throttled paths in the file. - cancel_callback returns REJECT when the handle is not the active goal (was always ACCEPT). - Trim verbose multi-line comments throughout the cpp and hpp.
…face optional
Rename MaxEffortGripperActionController to GripperPositionEffortController.
The class additionally accepts position and velocity topics, so ActionController
was misleading; the new name describes what it commands (position + effort).
Add effort_command_interface parameter ('required' | 'disabled', default 'required').
In 'disabled' mode the controller does not claim or write the effort command
interface, enabling use in simulation environments (e.g. Gazebo) that expose
only a position command interface. The effort *state* interface remains required
so is_grasped detection works in both modes. An empty/invalid value fails
on_init via one_of<> validation.
Also drop a stale ament_add_gmock(test_velocity_to_position_command_controller)
block that was reintroduced as a merge-conflict artifact in 9491b95; the test
file was renamed to test_sync_group_velocity_to_position_controller in e163a69.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a single-joint gripper action controller that simultaneously
commands position and a max-effort (current/torque) limit, designed for
Dynamixel actuators in the current-based position control mode, where the
upstream gripper_controllers/GripperActionController drops the
GripperCommand max_effort field on the floor.
Features:
timeout, clamped to URDF joint limits
counter; topic input preempts active action goals
joint velocity and effort with configurable dwell-cycle latching
effort state interfaces; activation fails loudly if any are missing
the hardware write