Skip to content

Commit 69145c4

Browse files
Barry-Xu-2018Barry-Xu-2018
committed
Address review comments from William
Signed-off-by: Barry Xu <barry.xu@sony.com>
1 parent da86048 commit 69145c4

7 files changed

Lines changed: 708 additions & 684 deletions

File tree

‎rclcpp/include/rclcpp/client.hpp‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,29 @@ struct FutureAndRequestId
115115
/// Destructor.
116116
~FutureAndRequestId() = default;
117117
};
118+
119+
template<typename PendingRequestsT, typename AllocatorT = std::allocator<int64_t>>
120+
size_t
121+
prune_requests_older_than_impl(
122+
PendingRequestsT & pending_requests,
123+
std::mutex & pending_requests_mutex,
124+
std::chrono::time_point<std::chrono::system_clock> time_point,
125+
std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
126+
{
127+
std::lock_guard guard(pending_requests_mutex);
128+
auto old_size = pending_requests.size();
129+
for (auto it = pending_requests.begin(), last = pending_requests.end(); it != last; ) {
130+
if (it->second.first < time_point) {
131+
if (pruned_requests) {
132+
pruned_requests->push_back(it->first);
133+
}
134+
it = pending_requests.erase(it);
135+
} else {
136+
++it;
137+
}
138+
}
139+
return old_size - pending_requests.size();
140+
}
118141
} // namespace detail
119142

120143
namespace node_interfaces
@@ -771,19 +794,11 @@ class Client : public ClientBase
771794
std::chrono::time_point<std::chrono::system_clock> time_point,
772795
std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
773796
{
774-
std::lock_guard guard(pending_requests_mutex_);
775-
auto old_size = pending_requests_.size();
776-
for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
777-
if (it->second.first < time_point) {
778-
if (pruned_requests) {
779-
pruned_requests->push_back(it->first);
780-
}
781-
it = pending_requests_.erase(it);
782-
} else {
783-
++it;
784-
}
785-
}
786-
return old_size - pending_requests_.size();
797+
return detail::prune_requests_older_than_impl(
798+
pending_requests_,
799+
pending_requests_mutex_,
800+
time_point,
801+
pruned_requests);
787802
}
788803

789804
/// Configure client introspection.

‎rclcpp/include/rclcpp/create_generic_client.hpp‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@
1919
#include <string>
2020

2121
#include "rclcpp/generic_client.hpp"
22+
#include "rclcpp/node_interfaces/get_node_base_interface.hpp"
23+
#include "rclcpp/node_interfaces/get_node_graph_interface.hpp"
24+
#include "rclcpp/node_interfaces/get_node_services_interface.hpp"
2225
#include "rclcpp/node_interfaces/node_base_interface.hpp"
2326
#include "rclcpp/node_interfaces/node_graph_interface.hpp"
2427
#include "rclcpp/node_interfaces/node_services_interface.hpp"
@@ -51,6 +54,25 @@ create_generic_client(
5154
const rclcpp::QoS & qos = rclcpp::ServicesQoS(),
5255
rclcpp::CallbackGroup::SharedPtr group = nullptr);
5356

57+
template<typename NodeT>
58+
rclcpp::GenericClient::SharedPtr
59+
create_generic_client(
60+
NodeT node,
61+
const std::string & service_name,
62+
const std::string & service_type,
63+
const rclcpp::QoS & qos = rclcpp::ServicesQoS(),
64+
rclcpp::CallbackGroup::SharedPtr group = nullptr)
65+
{
66+
return create_generic_client(
67+
rclcpp::node_interfaces::get_node_base_interface(node),
68+
rclcpp::node_interfaces::get_node_graph_interface(node),
69+
rclcpp::node_interfaces::get_node_services_interface(node),
70+
service_name,
71+
service_type,
72+
qos,
73+
group
74+
);
75+
}
5476
} // namespace rclcpp
5577

5678
#endif // RCLCPP__CREATE_GENERIC_CLIENT_HPP_

‎rclcpp/include/rclcpp/generic_client.hpp‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -84,13 +84,16 @@ class GenericClient : public ClientBase
8484
rcl_client_options_t & client_options);
8585

8686
RCLCPP_PUBLIC
87-
SharedResponse create_response() override;
87+
SharedResponse
88+
create_response() override;
8889

8990
RCLCPP_PUBLIC
90-
std::shared_ptr<rmw_request_id_t> create_request_header() override;
91+
std::shared_ptr<rmw_request_id_t>
92+
create_request_header() override;
9193

9294
RCLCPP_PUBLIC
93-
void handle_response(
95+
void
96+
handle_response(
9497
std::shared_ptr<rmw_request_id_t> request_header,
9598
std::shared_ptr<void> response) override;
9699

@@ -123,7 +126,8 @@ class GenericClient : public ClientBase
123126
* \return a FutureAndRequestId instance.
124127
*/
125128
RCLCPP_PUBLIC
126-
FutureAndRequestId async_send_request(const Request request);
129+
FutureAndRequestId
130+
async_send_request(const Request request);
127131

128132
/// Clean all pending requests older than a time_point.
129133
/**
@@ -138,26 +142,21 @@ class GenericClient : public ClientBase
138142
std::chrono::time_point<std::chrono::system_clock> time_point,
139143
std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
140144
{
141-
std::lock_guard guard(pending_requests_mutex_);
142-
auto old_size = pending_requests_.size();
143-
for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
144-
if (it->second.first < time_point) {
145-
if (pruned_requests) {
146-
pruned_requests->push_back(it->first);
147-
}
148-
it = pending_requests_.erase(it);
149-
} else {
150-
++it;
151-
}
152-
}
153-
return old_size - pending_requests_.size();
145+
return detail::prune_requests_older_than_impl(
146+
pending_requests_,
147+
pending_requests_mutex_,
148+
time_point,
149+
pruned_requests);
154150
}
155151

156152
RCLCPP_PUBLIC
157-
size_t prune_pending_requests();
153+
size_t
154+
prune_pending_requests();
158155

159156
RCLCPP_PUBLIC
160-
bool remove_pending_request(int64_t request_id);
157+
bool
158+
remove_pending_request(
159+
int64_t request_id);
161160

162161
/// Take the next response for this client.
163162
/**
@@ -173,7 +172,8 @@ class GenericClient : public ClientBase
173172
* rcl function fail.
174173
*/
175174
RCLCPP_PUBLIC
176-
bool take_response(Response response_out, rmw_request_id_t & request_header_out)
175+
bool
176+
take_response(Response response_out, rmw_request_id_t & request_header_out)
177177
{
178178
return this->take_type_erased_response(response_out, request_header_out);
179179
}
@@ -183,10 +183,13 @@ class GenericClient : public ClientBase
183183
std::promise<SharedResponse>>; // Use variant for extension
184184

185185
int64_t
186-
async_send_request_impl(const Request request, CallbackInfoVariant value);
186+
async_send_request_impl(
187+
const Request request,
188+
CallbackInfoVariant value);
187189

188190
std::optional<CallbackInfoVariant>
189-
get_and_erase_pending_request(int64_t request_number);
191+
get_and_erase_pending_request(
192+
int64_t request_number);
190193

191194
RCLCPP_DISABLE_COPY(GenericClient)
192195

‎rclcpp/test/rclcpp/CMakeLists.txt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,17 @@ if(TARGET test_generic_client)
7474
${test_msgs_TARGETS}
7575
)
7676
endif()
77+
ament_add_gtest(test_client_common test_client_common.cpp)
78+
if(TARGET test_client_common)
79+
target_link_libraries(test_client_common ${PROJECT_NAME}
80+
mimick
81+
${rcl_interfaces_TARGETS}
82+
rmw::rmw
83+
rosidl_runtime_cpp::rosidl_runtime_cpp
84+
rosidl_typesupport_cpp::rosidl_typesupport_cpp
85+
${test_msgs_TARGETS}
86+
)
87+
endif()
7788
ament_add_gtest(test_create_subscription test_create_subscription.cpp)
7889
if(TARGET test_create_subscription)
7990
target_link_libraries(test_create_subscription ${PROJECT_NAME} ${test_msgs_TARGETS})

0 commit comments

Comments
 (0)