-
Notifications
You must be signed in to change notification settings - Fork 497
Backport: Implement action generic client (#2759) #3017
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
base: jazzy
Are you sure you want to change the base?
Backport: Implement action generic client (#2759) #3017
Conversation
Backport from rolling to jazzy. This adds GenericClient for rclcpp_action, which allows creating action clients without compile-time knowledge of the action type. New files added: - generic_client.hpp/cpp - generic_client_goal_handle.hpp/cpp - create_generic_client.hpp/cpp - client_base.hpp (wrapper for jazzy compatibility) - test_generic_client.cpp Note: On jazzy, ClientBase remains in client.hpp (not separated). client_base.hpp is a compatibility wrapper that includes client.hpp. Original PR: ros2#2759 Signed-off-by: Barry Xu <barry.xu@sony.com>
Add local implementation of get_action_typesupport_handle() which is not available in jazzy's rclcpp (only in rolling). Signed-off-by: Koki Shinjo <kshinjo@pfrobotics.jp>
6cb6160 to
875289f
Compare
|
@Barry-Xu-2018 could you help us out and review this? |
fujitatomoya
left a comment
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.
@kshinjo-pfr although this does not break ABI, it is unlikely to backport those new interfaces to the downstream branches. out of curiosity, what is your actual use case for this?
|
@fujitatomoya Since we plan to deploy this in our application running on the LTS version (jazzy), we would strongly appreciate it if you could consider backporting this, given that it maintains ABI compatibility. |
|
thanks! that makes sense. having this generic action client should be a part of it to bridge/proxy the protocol. but what is the plan to support generic action server? we do not have generic action server yet. are you planning to support it to rolling and also backport to jazzy without API/ABI breaking change? i think this needs to be implemented to achieve your requirement, correct? (not asking the commitment, but just curious about the development plan.) |
|
For our specific use case, we aim to trigger ROS Actions from a non-ROS environment. Therefore, the bridge node only needs to function as an Action Client to forward requests to the ROS network. We do not currently have a use case where the bridge acts as an Action Server to accept goals from ROS. So, to answer your question, we don't have a plan to implement the generic action server at this moment, as it is not required for this one-way integration. |
Barry-Xu-2018
left a comment
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.
Because the code is basically the same as rolling, I only reviewed at the parts that are different.
Overall, it looks good, I have only one minor suggestion.
| #include "action_msgs/msg/goal_info.hpp" | ||
| #include "action_msgs/msg/goal_status_array.hpp" | ||
|
|
||
| #include "rclcpp_action/client_base.hpp" |
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.
My thought is that there’s no need to create a separate client_base.hpp, because it serves the same purpose as using “Client.hpp” here. It’s sufficient to clearly explain the reasoning and limitations in this file.
It should also be emphasized that if you want to use both Client and GenericClient in the same file, you just need to include “generic_client.hpp”.
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.
Thank you for the review and suggestion.
You're right - I've removed client_base.hpp and changed generic_client.hpp to include client.hpp directly.
As per review feedback, there is no need to create a separate client_base.hpp since including client.hpp directly serves the same purpose. Signed-off-by: Koki Shinjo <kshinjo@pfrobotics.jp>
c040a5f to
0cd54d0
Compare
Description
Backport of #2759 from rolling to jazzy.
This adds
GenericClientforrclcpp_action, which allows creating action clients without compile-time knowledge of the action type.Is this user-facing behavior change?
Yes. Users can now create action clients dynamically without compile-time knowledge of the action type using
rclcpp_action::create_generic_client().Did you use Generative AI?
Yes. Claude Code (Claude Opus 4.5) was used to assist with:
get_action_typesupport_handle()functionclient_base.hppcompatibility wrapperAdditional Information
Test results:
colcon build --packages-select rclcpp_actionpassescolcon test --packages-select rclcpp_actionpasses (252 tests, 0 failures)Dependencies:
This backport is self-contained and does not require backporting #2750 (action typesupport helper) because
get_action_typesupport_handle()is implemented locally increate_generic_client.cpp.