Conversation
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
|
Pulls: #2617
|
ahcorde
left a comment
There was a problem hiding this comment.
Windows is not compiling
13:38:54 C:\ci\ws\src\Barry-Xu-2018\rclcpp\rclcpp\include\rclcpp/create_generic_service.hpp(48,1): error C2491: 'rclcpp::create_generic_service': definition of dllimport function not allowed [C:\ci\ws\build\rclcpp\test\rclcpp\test_wait_set.vcxproj]
13:38:54 C:\ci\ws\src\Barry-Xu-2018\rclcpp\rclcpp\include\rclcpp/create_generic_service.hpp(87,1): error C2491: 'rclcpp::create_generic_service': definition of dllimport function not allowed [C:\ci\ws\build\rclcpp\test\rclcpp\test_wait_set.vcxproj]Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
|
Re-run CI on Windows
|
Signed-off-by: Barry Xu <barry.xu@sony.com>
|
Re-run CI on Windows after fixing compiling error Re-run CI on Windows
|
|
|
@fujitatomoya, you self assigned this PR for review, do you have something to add ? |
|
yes, i am going to take a look at this in today. |
fujitatomoya
left a comment
There was a problem hiding this comment.
@Barry-Xu-2018 ltgm but i have a few comments. could you check thek?
| }); | ||
| *service_handle_.get() = rcl_get_zero_initialized_service(); | ||
|
|
||
| rcl_ret_t ret = rcl_service_init( |
There was a problem hiding this comment.
Not directly related to this PR but future refactoring. I think rcl_service_init and rcl_client_init can be called in ServiceBase and ClientBase to avoid redundant code, that actually aligns with Publisher and Subscription class implementation.
There was a problem hiding this comment.
Yes.
I will consider optimizing this part of the code. If there are no issues, I will create a PR for the client code.
| } catch (std::runtime_error & err) { | ||
| RCLCPP_ERROR( | ||
| rclcpp::get_node_logger(node_handle_.get()).get_child("rclcpp"), | ||
| "Invalid service type: %s", | ||
| err.what()); | ||
| throw rclcpp::exceptions::InvalidServiceTypeError(err.what()); | ||
| } |
There was a problem hiding this comment.
Dose this need to be added to the constructor of GenericClient?
There was a problem hiding this comment.
Yes. I think so.
I will create a PR for GenericClient.
| using SharedRequest = std::shared_ptr<void>; | ||
| using SharedResponse = std::shared_ptr<void>; | ||
|
|
||
| GenericServiceCallback() |
There was a problem hiding this comment.
Instead of having this class, could we extend the AnyServiceCallback and have it as a member variable under GenericService class? this does not block the PR but can be future refactoring?
There was a problem hiding this comment.
I haven't figured out how to extend AnyServiceCallback for use in GenericService yet.
GenericServiceCallback should consider how to merge with AnyServiceCallback to reduce redundant code.
|
Implement generic service.