-
Notifications
You must be signed in to change notification settings - Fork 431
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
Support intra-process communication between Clients and Services #1847
base: rolling
Are you sure you want to change the base?
Support intra-process communication between Clients and Services #1847
Conversation
17a8555
to
78ea732
Compare
rclcpp/include/rclcpp/client.hpp
Outdated
/// Implementation detail. | ||
RCLCPP_PUBLIC | ||
void | ||
setup_intra_process( |
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 probably be protected (and also the using
definition above)
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.
Done in 0d17a8e
@@ -676,7 +711,7 @@ class Client : public ClientBase | |||
PromiseWithRequest promise; | |||
auto shared_future = promise.get_future().share(); | |||
auto req_id = async_send_request_impl( | |||
*request, | |||
request, |
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.
why not std::move
as in the other calls above?
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.
Not moving here because needs to be valid for later use in line 718
rclcpp/include/rclcpp/client.hpp
Outdated
|
||
private: | ||
using ClientIntraProcessT = rclcpp::experimental::ClientIntraProcess<ServiceT>; | ||
std::shared_ptr<ClientIntraProcessT> client_intra_process_; |
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.
to match subscriptions implementation, we should have that client_intra_process_
is a protected member in client_base.hpp
.
Its type will be ClientIntraProcessBase
and the using
definition can be moved to the create_intra_process_client
The same applies to servers.
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.
Done in bec9017
bool | ||
resolve_use_intra_process(const OptionsT & options, const NodeBaseT & node_base) | ||
resolve_use_intra_process(const IntraProcessSetting & ipc_setting, const NodeBaseT & node_base) |
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.
IntraProcessSetting
is an enum, we can avoid passing that as a reference as it's likely to be a pessimization (and may cause confusion between the enum value and the memory address)
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.
Done in d19d5c2
// Not to be used in this class. Todo: review base class to avoid this. | ||
bool use_take_shared_method() const override | ||
{ | ||
throw std::runtime_error( |
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.
do we really need to throw here?
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'm just returning false now: 08290a8
auto & typed_response = data_ptr->first; | ||
auto & value = data_ptr->second; | ||
|
||
if (std::holds_alternative<Promise>(value)) { |
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.
it would be nice if we could avoid duplicating this logic between here and client.hpp
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2022-04-21/25293/1 |
bec9017
to
b6245f2
Compare
Hi, can we get a review here? |
Hi, anyone available for a review? |
Friendly ping for a review |
98aad33
to
abf4e71
Compare
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
abf4e71
to
0a98fda
Compare
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
d45b7ea
to
b995963
Compare
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
@wjwwood @clalancette @audrow @hidmic @ivanpauno can we get a review? |
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
These changes extend the intra-process capabilities to support intra-process client/services communication, so no need to go through the DDS when sending client requests / services responses, when they belong to the same process.
Missing: Add unit tests.
Design: https://github.com/mauropasse/design/blob/mauro/gh-pages-ipc-clients-services/articles/intraprocess_communication.md
@clalancette
@alsora