-
Notifications
You must be signed in to change notification settings - Fork 340
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
Adding Io gripper controller #1439
base: master
Are you sure you want to change the base?
Adding Io gripper controller #1439
Conversation
This PR depends on this PR |
io_gripper_controller/README.md
Outdated
# IO Gripper Controller | ||
|
||
The IO Gripper Controller is provides implementation to control the gripper using IOs. It provides functionalities like open, close and reconfigure which can be used either though action server or service server and also publishes `joint_states` of gripper and also `dynamic_interfaces` for all command and state interfaces. | ||
|
||
## Description of controller's interfaces | ||
|
||
- `joint_states` [`sensor_msgs::msg::JointState`]: Publishes the state of gripper joint and configuration joint | ||
- `dynamic_interfaces` [`control_msgs::msg::DynamicInterfaceValues`]: Publishes all command and state interface of the IOs and sensors of gripper. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go into the doc folder in the rst format. For an example please check the joint_trajectory_controller
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also write docs extensively about the internal state machine and also internal functionalities and external interface. As template, use the docs from another controller and adjust the content.
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
io_gripper_controller/include/io_gripper_controller/visibility_control.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First set of comments. The second one comes tomorrow.
io_gripper_controller/README.md
Outdated
# IO Gripper Controller | ||
|
||
The IO Gripper Controller is provides implementation to control the gripper using IOs. It provides functionalities like open, close and reconfigure which can be used either though action server or service server and also publishes `joint_states` of gripper and also `dynamic_interfaces` for all command and state interfaces. | ||
|
||
## Description of controller's interfaces | ||
|
||
- `joint_states` [`sensor_msgs::msg::JointState`]: Publishes the state of gripper joint and configuration joint | ||
- `dynamic_interfaces` [`control_msgs::msg::DynamicInterfaceValues`]: Publishes all command and state interface of the IOs and sensors of gripper. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also write docs extensively about the internal state machine and also internal functionalities and external interface. As template, use the docs from another controller and adjust the content.
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
- removed the template from license - added one variable per line - documented the enums - updated the doc folder as per `joint_trajectory_controller`
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
"~/gripper_close", close_service_callback, rmw_qos_profile_services_hist_keep_all, | ||
close_service_callback_group_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"~/gripper_close", close_service_callback, rmw_qos_profile_services_hist_keep_all, | |
close_service_callback_group_); | |
"~/gripper_close", close_service_callback, qos_services, close_service_callback_group_); |
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
io_gripper_controller/include/io_gripper_controller/io_gripper_controller.hpp
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
void IOGripperController::handle_reconfigure_state_transition(const reconfigure_state_type & state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal here. Can we do this general check only when activating the controller, i.e., on_activate
because later when we're switching states we know already which states do we expect, and then we check only those.
The main idea is to avoid so many iterations in every control step is this is not even necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also please add tests with different initial states of the gripper? To make sure that we recognize it correctly.
} | ||
} | ||
|
||
void IOGripperController::handle_gripper_state_transition_open(const gripper_state_type & state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find generally this function somewhat inconsistent with the closing state checks. Can we unify this a bit. Especially the part when setting low and high states. I would except there more or less copy-paste code, or better yet calling subfunctions as we constantly do the same.
} | ||
} | ||
|
||
controller_interface::CallbackReturn IOGripperController::check_parameters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the tests according to these checks of the parameters I have changed below. We should be able to work with parameter size 0.
closed_state_values_ = params_.close.state.possible_closed_states_map.at(closed_state_name_); | ||
|
||
for (const auto & high_val : closed_state_values_.set_after_command_high) | ||
{ | ||
setResult = find_and_set_command(high_val, 1.0); | ||
if (!setResult) | ||
{ | ||
RCLCPP_ERROR( | ||
get_node()->get_logger(), "Failed to set the command state for %s", high_val.c_str()); | ||
} | ||
} | ||
|
||
for (const auto & low_val : closed_state_values_.set_after_command_low) | ||
{ | ||
setResult = find_and_set_command(low_val, 0.0); | ||
if (!setResult) | ||
{ | ||
RCLCPP_ERROR( | ||
get_node()->get_logger(), "Failed to set the command state for %s", low_val.c_str()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code has two parts that are 98% identical. This should go into a function. Also this should be updated for all before/after, or if even possible setting commands in both function. We can reduce at least 50 lines of code doing that.
This Pull Request introduces the IO Gripper Controller, an implementation for controlling a gripper using IOs. The controller supports functionalities such as open, close, and reconfigure, which can be triggered either through an Action Server or a Service Server. Additionally, the controller publishes the gripper’s state via joint_states and provides dynamic_interfaces to expose all command and state interfaces.
Features
Interfaces
Published Topics
joint_states
[sensor_msgs::msg::JointState
]:dynamic_interfaces
[control_msgs::msg::DynamicInterfaceValues
]:This addition enables better modular control of robotic grippers, paving the way for seamless integration in complex robotic systems.
Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:
To send us a pull request, please:
colcon test
andpre-commit run
(requires you to install pre-commit bypip3 install pre-commit
)