Implement generic client #2358

Barry-Xu-2018 · 2023-11-06T08:56:06Z

This PR depended on #2209.

This interface is added for playing service in rosbag2.

ros2/rosbag2#1481 depended on this PR.

fujitatomoya · 2023-12-04T20:14:53Z

ros2/rosbag2#1480, and ros2/rosbag2#1481 depended on this PR.

@Barry-Xu-2018 this one too, we should close this?

fujitatomoya · 2023-12-04T23:53:38Z

Ah sorry, this one is required to play service, right? is this still in draft or i can start the review?

Barry-Xu-2018 · 2023-12-05T07:26:52Z

Ah sorry, this one is required to play service, right? is this still in draft or i can start the review?

Yes.
This PR was based on #2209 and 2209 isn't merged.
So current changed files of this PR include changed file for 2209.
After 2209 is merged, the changes made in this PR can be displayed more clearly.
So I set draft for this PR.

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2023-12-12T12:44:19Z

Now #2209 has been merged.
So do rebase and ready to review.

Barry-Xu-2018 · 2023-12-12T12:58:19Z

Build failure is because used rcl codes are old.

20:43:25 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/logger.cpp:84:9: error: ‘RCL_RET_NOT_FOUND’ was not declared in this scope; did you mean ‘RCUTILS_RET_NOT_FOUND’?
20:43:25    84 |     if (RCL_RET_NOT_FOUND == rcl_ret) {
20:43:25       |         ^~~~~~~~~~~~~~~~~
20:43:25       |         RCUTILS_RET_NOT_FOUND

fujitatomoya

overall lgtm, several comments.

rclcpp/include/rclcpp/create_generic_client.hpp

rclcpp/include/rclcpp/node.hpp

rclcpp/test/rclcpp/test_generic_client.cpp

fujitatomoya · 2023-12-12T20:56:03Z

@iuhilnehc-ynos can you do another review on this?

Signed-off-by: Barry Xu <[email protected]>

iuhilnehc-ynos

some minor comments.

rclcpp/include/rclcpp/create_generic_client.hpp

rclcpp/include/rclcpp/generic_client.hpp

rclcpp/include/rclcpp/node.hpp

rclcpp/src/rclcpp/generic_client.cpp

Barry-Xu-2018 · 2023-12-22T09:13:23Z

@iuhilnehc-ynos

Thank you for your comments. I will handle them.

Signed-off-by: Barry Xu <[email protected]>

fujitatomoya

lgtm, one comment needs to be resolved.

rclcpp/include/rclcpp/generic_client.hpp

fujitatomoya · 2024-01-04T22:42:32Z

CI:

Linux
Linux-aarch64
Windows

Barry-Xu-2018 · 2024-01-05T02:05:29Z

lgtm, one comment needs to be resolved.

Sorry, I missed it. I will fix it.

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2024-01-05T02:13:05Z

After checking log, the CI failure for Linux is related to CI system.
About the CI failure for Window, I will investigate it.

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2024-01-05T03:28:55Z

@fujitatomoya Please help re-run the CI.

fujitatomoya · 2024-01-05T03:45:58Z

CI:

Linux
Linux-aarch64
Windows

Barry-Xu-2018 · 2024-01-05T07:22:54Z

For Linux-aarch64, the failure case 'ros2topic.ros2topic.test.test_cli.test_cli' is unrelated to this patch.

wjwwood

I had a few requested changes and questions.

One of the bigger issues I have with the implementation (as opposed to the concept) is that we're copy-pasting a lot of code from the client class for this new generic class, and especially for the tests it's a lot. I think the goal for the tests should be to (as often as possible) have the same tests run over both the normal and generic client classes and where it doesn't make sense it should be easy to see which tests don't apply to the generic client and why.

wjwwood · 2024-01-06T00:17:14Z

rclcpp/include/rclcpp/generic_client.hpp

+  RCLCPP_PUBLIC
+  void handle_response(
+    std::shared_ptr<rmw_request_id_t> request_header,
+    std::shared_ptr<void> response) override;


nitpick: if you need to multi-line the function prototypes, I think it's cleaner to put the return code on its own line first, then the arguments, and if you do that for one function, it's helpful for readability (again in my opinion) to put them all on their own line even for functions that are short. This squarely falls into developer discretion, but I wanted to point it out. To be clear, I'm suggesting:

Suggested change

RCLCPP_PUBLIC

void handle_response(

std::shared_ptr<rmw_request_id_t> request_header,

std::shared_ptr<void> response) override;

RCLCPP_PUBLIC

void

handle_response(

std::shared_ptr<rmw_request_id_t> request_header,

std::shared_ptr<void> response) override;

I will update other declarations to use this coding style.

rclcpp/include/rclcpp/create_generic_client.hpp

wjwwood · 2024-01-06T00:22:05Z

rclcpp/include/rclcpp/generic_client.hpp

+  /// Clean all pending requests older than a time_point.
+  /**
+   * \param[in] time_point Requests that were sent before this point are going to be removed.
+   * \param[inout] pruned_requests Removed requests id will be pushed to the vector
+   *  if a pointer is provided.
+   * \return number of pending requests that were removed.
+   */
+  template<typename AllocatorT = std::allocator<int64_t>>
+  size_t
+  prune_requests_older_than(
+    std::chrono::time_point<std::chrono::system_clock> time_point,
+    std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
+  {
+    std::lock_guard guard(pending_requests_mutex_);
+    auto old_size = pending_requests_.size();
+    for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
+      if (it->second.first < time_point) {
+        if (pruned_requests) {
+          pruned_requests->push_back(it->first);
+        }
+        it = pending_requests_.erase(it);
+      } else {
+          it;
+      }
+    }
+    return old_size - pending_requests_.size();
+  }


Copy and pasting large chunks of logic like this one (pulled from

rclcpp/rclcpp/include/rclcpp/client.hpp

Lines 761 to 787 in 126d517

/// Clean all pending requests older than a time_point.

/**

* \param[in] time_point Requests that were sent before this point are going to be removed.

* \param[inout] pruned_requests Removed requests id will be pushed to the vector

* if a pointer is provided.

* \return number of pending requests that were removed.

*/

template<typename AllocatorT = std::allocator<int64_t>>

size_t

prune_requests_older_than(

std::chrono::time_point<std::chrono::system_clock> time_point,

std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)

{

std::lock_guard guard(pending_requests_mutex_);

auto old_size = pending_requests_.size();

for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {

if (it->second.first < time_point) {

if (pruned_requests) {

pruned_requests->push_back(it->first);

}

it = pending_requests_.erase(it);

} else {

it;

}

}

return old_size - pending_requests_.size();

}

) is really bad imo. If we discover and fix a bug in one we may fail to update the other. It would be better to factor these into common functions, so that it is more obvious that this logic is used in two places.

@wjwwood

"Currently, pending_requests_ is defined in GenericClient class and Client class. The operations on pending_requests_ involve duplicated code.
There are two ways to avoid duplicated codes:

Move pending_requests_ and the corresponding mutex lock into the client base class. Since pending_requests_ contains different contents, the BaseClient class must be changed to a template class. This change would be quite significant.

Use macros to include the same code and place the macros in client.hpp."

I am inclined towards 2.
I would like to hear your opinion.

Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.

Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.

Yes. Using function is okay.
pending_requests_ must be passed, and the type of pending_requests_ is different for Client and GenericClient. So the function have to be a template function.

wjwwood · 2024-01-06T00:42:11Z

rclcpp/test/rclcpp/test_generic_client.cpp

+/*
+   Testing on_new_response callbacks.
+ */
+TEST_F(TestGenericClient, on_new_response_callback) {
+  auto client_node = std::make_shared<rclcpp::Node>("test_client_node", "ns");
+  auto server_node = std::make_shared<rclcpp::Node>("test_server_node", "ns");
+
+  rclcpp::ServicesQoS client_qos;
+  client_qos.keep_last(3);
+  auto client =
+    client_node->create_generic_client("test_service", "test_msgs/srv/Empty", client_qos);
+  std::atomic<size_t> server_requests_count {0};
+  auto server_callback = [&server_requests_count](
+    const test_msgs::srv::Empty::Request::SharedPtr,
+    test_msgs::srv::Empty::Response::SharedPtr) {server_requests_count  ;};
+  auto server = server_node->create_service<test_msgs::srv::Empty>(
+    "test_service", server_callback, client_qos);
+  auto request = std::make_shared<test_msgs::srv::Empty::Request>();
+
+  std::atomic<size_t> c1 {0};
+  auto increase_c1_cb = [&c1](size_t count_msgs) {c1  = count_msgs;};
+  client->set_on_new_response_callback(increase_c1_cb);
+
+  client->async_send_request(request.get());
+  auto start = std::chrono::steady_clock::now();
+  while (server_requests_count == 0 &&
+    (std::chrono::steady_clock::now() - start) < 10s)
+  {
+    rclcpp::spin_some(server_node);
+  }
+
+  ASSERT_EQ(server_requests_count, 1u);
+
+  start = std::chrono::steady_clock::now();
+  do {
+    std::this_thread::sleep_for(100ms);
+  } while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);
+
+  EXPECT_EQ(c1.load(), 1u);
+
+  std::atomic<size_t> c2 {0};
+  auto increase_c2_cb = [&c2](size_t count_msgs) {c2  = count_msgs;};
+  client->set_on_new_response_callback(increase_c2_cb);
+
+  client->async_send_request(request.get());
+  start = std::chrono::steady_clock::now();
+  while (server_requests_count == 1 &&
+    (std::chrono::steady_clock::now() - start) < 10s)
+  {
+    rclcpp::spin_some(server_node);
+  }
+
+  ASSERT_EQ(server_requests_count, 2u);
+
+  start = std::chrono::steady_clock::now();
+  do {
+    std::this_thread::sleep_for(100ms);
+  } while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);
+
+  EXPECT_EQ(c1.load(), 1u);
+  EXPECT_EQ(c2.load(), 1u);
+
+  client->clear_on_new_response_callback();
+
+  client->async_send_request(request.get());
+  client->async_send_request(request.get());
+  client->async_send_request(request.get());
+  start = std::chrono::steady_clock::now();
+  while (server_requests_count < 5 &&
+    (std::chrono::steady_clock::now() - start) < 10s)
+  {
+    rclcpp::spin_some(server_node);
+  }
+
+  ASSERT_EQ(server_requests_count, 5u);
+
+  std::atomic<size_t> c3 {0};
+  auto increase_c3_cb = [&c3](size_t count_msgs) {c3  = count_msgs;};
+  client->set_on_new_response_callback(increase_c3_cb);
+
+  start = std::chrono::steady_clock::now();
+  do {
+    std::this_thread::sleep_for(100ms);
+  } while (c3 < 3 && std::chrono::steady_clock::now() - start < 10s);
+
+  EXPECT_EQ(c1.load(), 1u);
+  EXPECT_EQ(c2.load(), 1u);
+  EXPECT_EQ(c3.load(), 3u);
+
+  std::function<void(size_t)> invalid_cb = nullptr;
+  EXPECT_THROW(client->set_on_new_response_callback(invalid_cb), std::invalid_argument);
+}


Similar comment here, let's not copy-paste tests (this is from

rclcpp/rclcpp/test/rclcpp/test_client.cpp

Lines 413 to 503 in 126d517

/*

Testing on_new_response callbacks.

*/

TEST_F(TestClient, on_new_response_callback) {

auto client_node = std::make_shared<rclcpp::Node>("client_node", "ns");

auto server_node = std::make_shared<rclcpp::Node>("server_node", "ns");

rclcpp::ServicesQoS client_qos;

client_qos.keep_last(3);

auto client = client_node->create_client<test_msgs::srv::Empty>("test_service", client_qos);

std::atomic<size_t> server_requests_count {0};

auto server_callback = [&server_requests_count](

const test_msgs::srv::Empty::Request::SharedPtr,

test_msgs::srv::Empty::Response::SharedPtr) {server_requests_count ;};

auto server = server_node->create_service<test_msgs::srv::Empty>(

"test_service", server_callback, client_qos);

auto request = std::make_shared<test_msgs::srv::Empty::Request>();

std::atomic<size_t> c1 {0};

auto increase_c1_cb = [&c1](size_t count_msgs) {c1 = count_msgs;};

client->set_on_new_response_callback(increase_c1_cb);

client->async_send_request(request);

auto start = std::chrono::steady_clock::now();

while (server_requests_count == 0 &&

(std::chrono::steady_clock::now() - start) < 10s)

{

rclcpp::spin_some(server_node);

}

ASSERT_EQ(server_requests_count, 1u);

start = std::chrono::steady_clock::now();

do {

std::this_thread::sleep_for(100ms);

} while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);

EXPECT_EQ(c1.load(), 1u);

std::atomic<size_t> c2 {0};

auto increase_c2_cb = [&c2](size_t count_msgs) {c2 = count_msgs;};

client->set_on_new_response_callback(increase_c2_cb);

client->async_send_request(request);

start = std::chrono::steady_clock::now();

while (server_requests_count == 1 &&

(std::chrono::steady_clock::now() - start) < 10s)

{

rclcpp::spin_some(server_node);

}

ASSERT_EQ(server_requests_count, 2u);

start = std::chrono::steady_clock::now();

do {

std::this_thread::sleep_for(100ms);

} while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);

EXPECT_EQ(c1.load(), 1u);

EXPECT_EQ(c2.load(), 1u);

client->clear_on_new_response_callback();

client->async_send_request(request);

client->async_send_request(request);

client->async_send_request(request);

start = std::chrono::steady_clock::now();

while (server_requests_count < 5 &&

(std::chrono::steady_clock::now() - start) < 10s)

{

rclcpp::spin_some(server_node);

}

ASSERT_EQ(server_requests_count, 5u);

std::atomic<size_t> c3 {0};

auto increase_c3_cb = [&c3](size_t count_msgs) {c3 = count_msgs;};

client->set_on_new_response_callback(increase_c3_cb);

start = std::chrono::steady_clock::now();

do {

std::this_thread::sleep_for(100ms);

} while (c3 < 3 && std::chrono::steady_clock::now() - start < 10s);

EXPECT_EQ(c1.load(), 1u);

EXPECT_EQ(c2.load(), 1u);

EXPECT_EQ(c3.load(), 3u);

std::function<void(size_t)> invalid_cb = nullptr;

EXPECT_THROW(client->set_on_new_response_callback(invalid_cb), std::invalid_argument);

}

and it's not just this test, it's most of this file) that are hugely complex, instead please refactor them to have the same code run over Generic and regular Clients.

rclcpp/include/rclcpp/create_generic_client.hpp

Barry-Xu-2018 · 2024-01-09T02:32:07Z

@wjwwood
Thank you for your advice.

One of the bigger issues I have with the implementation (as opposed to the concept) is that we're copy-pasting a lot of code from the client class for this new generic class, and especially for the tests it's a lot. I think the goal for the tests should be to (as often as possible) have the same tests run over both the normal and generic client classes and where it doesn't make sense it should be easy to see which tests don't apply to the generic client and why.

Yes. I will optimize test codes.

wjwwood · 2024-01-09T15:24:35Z

Thanks, you can have a look at some of the executor tests for examples of tests that run over several different classes by parameterization of the test.

wjwwood · 2024-01-09T15:25:47Z

rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp

Lines 121 to 146 in 6f6b5f2

    
           // TYPED_TEST_SUITE is deprecated as of gtest 1.9, use TYPED_TEST_SUITE when gtest dependency 
        
           // is updated. 
        
           TYPED_TEST_SUITE(TestExecutors, ExecutorTypes, ExecutorTypeNames); 
        
           // StaticSingleThreadedExecutor is not included in these tests for now, due to: 
        
           // https://github.com/ros2/rclcpp/issues/1219 
        
           using StandardExecutors = 
        
             ::testing::Types< 
        
             rclcpp::executors::SingleThreadedExecutor, 
        
             rclcpp::executors::MultiThreadedExecutor, 
        
             rclcpp::experimental::executors::EventsExecutor>; 
        
           TYPED_TEST_SUITE(TestExecutorsStable, StandardExecutors, ExecutorTypeNames); 
        
           // Make sure that executors detach from nodes when destructing 
        
           TYPED_TEST(TestExecutors, detachOnDestruction) 
        
           { 
        
             using ExecutorType = TypeParam; 
        
             { 
        
               ExecutorType executor; 
        
               executor.add_node(this->node); 
        
             } 
        
             { 
        
               ExecutorType executor; 
        
               EXPECT_NO_THROW(executor.add_node(this->node)); 
        
             } 
        
           }

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2024-01-12T03:20:30Z

@wjwwood

I submitted changes 69145c4.

Move common tests for Client and GenericClient to test_client_common.cpp.
Add new interface create_generic_client with NodeT parameter. Implement generic client #2358 (comment)
Use function for common function Implement generic client #2358 (comment)

fujitatomoya · 2024-03-01T21:47:53Z

me and @iuhilnehc-ynos 's comments are resolved. i will start the Full CI.

Linux
Linux-aarch64
Windows

fujitatomoya · 2024-03-01T21:48:11Z

@wjwwood

I submitted changes 69145c4.

Move common tests for Client and GenericClient to test_client_common.cpp.

Add new interface create_generic_client with NodeT parameter. Implement generic client #2358 (comment)

Use function for common function Implement generic client #2358 (comment)

@wjwwood can you review your comments?

Barry-Xu-2018 · 2024-03-04T02:31:01Z

Linux

Linux-aarch64

Windows

I checked failed cases.
The reason for the test failure is unrelated to this PR.

wjwwood

lgtm with passing CI

Thank you for iterating and removing a lot of the duplicate code! It looks much better now.

rclcpp/include/rclcpp/create_generic_client.hpp

Signed-off-by: Barry Xu <[email protected]>

Signed-off-by: methylDragon <[email protected]> Fix a format-security warning when building with clang (ros2#2171) In particular, you should never have a "bare" string in a printf-like call; that could potentially access uninitialized memory. Instead, make sure to format the string with %s. Signed-off-by: Chris Lalancette <[email protected]> Add missing stdexcept include (ros2#2186) Signed-off-by: Øystein Sture <[email protected]> Fix race condition in events-executor (ros2#2177) The initial implementation of the events-executor contained a bug where the executor would end up in an inconsistent state and stop processing interrupt/shutdown notifications. Manually adding a node to the executor results in a) producing a notify waitable event and b) refreshing the executor collections. The inconsistent state would happen if the event was processed before the collections were finished to be refreshed: the executor would pick up the event but be unable to process it. This would leave the `notify_waitable_event_pushed_` flag to true, preventing additional notify waitable events to be pushed. The behavior is observable only under heavy load. Signed-off-by: Alberto Soragna <[email protected]> Changelog Signed-off-by: Yadunund <[email protected]> 21.1.1 Declare rclcpp callbacks before the rcl entities (ros2#2024) This is to ensure callbacks are destroyed last on entities destruction, avoiding the gap in time in which rmw entities hold a reference to a destroyed function. Signed-off-by: Mauro Passerino <[email protected]> Co-authored-by: Mauro Passerino <[email protected]> add mutex to protect events_executor current entity collection (ros2#2187) * add mutex to protect events_executor current entity collection and unit-test Signed-off-by: Alberto Soragna <[email protected]> * be more precise with mutex locks; make stress test less stressfull Signed-off-by: Alberto Soragna <[email protected]> * fix uncrustify error Signed-off-by: Alberto Soragna <[email protected]> --------- Signed-off-by: Alberto Soragna <[email protected]> Feature/available capacity of ipm (ros2#2173) * added available_capacity to get the lowest number of free capacity for intra-process communication for a publisher Signed-off-by: Joshua Hampp <[email protected]> * added unit tests for available_capacity Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> * fixed typos in comments Signed-off-by: Joshua Hampp <[email protected]> * Updated warning Co-authored-by: Alberto Soragna <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> * returning 0 if ipm is disabled in lowest_available_ipm_capacity Signed-off-by: Joshua Hampp <[email protected]> * return 0 if no subscribers are present in lowest_available_capacity Signed-off-by: Joshua Hampp <[email protected]> * updated unit test Signed-off-by: Joshua Hampp <[email protected]> * update unit test Signed-off-by: Joshua Hampp <[email protected]> * moved available_capacity to a lambda function to be able to handle subscriptions which went out of scope Signed-off-by: Joshua Hampp <[email protected]> * updated unit test to check subscriptions which went out of scope Signed-off-by: Joshua Hampp <[email protected]> --------- Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> Co-authored-by: Joshua Hampp <[email protected]> Co-authored-by: Joshua Hampp <[email protected]> Co-authored-by: Alberto Soragna <[email protected]> remove nolint since ament_cpplint updated for the c 17 header (ros2#2198) Signed-off-by: Chen Lihui <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 21.2.0 Use TRACETOOLS_ prefix for tracepoint-related macros (ros2#2162) Signed-off-by: Christophe Bedard <[email protected]> Remove flaky stressAddRemoveNode test (ros2#2206) Signed-off-by: Michael Carroll <[email protected]> Fix up misspellings of "receive". (ros2#2208) Signed-off-by: Chris Lalancette <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 21.3.0 Enable callback group tests for connextdds (ros2#2182) * Enable callback group tests for connextdds * Enable executors and event executor tests for connextdds * Enable qos events tests for connextdds * Less flaky qos_event tests Signed-off-by: Christopher Wecht <[email protected]> Modifies timers API to select autostart state (ros2#2005) * Modifies timers API to select autostart state * Removes unnecessary variables * Adds autostart documentation and expands some timer test Signed-off-by: Voldivh <[email protected]> warning: comparison of integer expressions of different signedness (ros2#2219) ros2#2167 (comment) Signed-off-by: Tomoya Fujita <[email protected]> Revamp the test_subscription.cpp tests. (ros2#2227) The original motiviation to do this was a crash during teardown when using a newer version of gtest. But while I was in here, I did a small overall cleanup, including: 1. Moving code closer to where it is actually used. 2. Getting rid of unused 'using' statements. 3. Adding in missing includes. 4. Properly tearing down and recreating the rclcpp context during test teardown (this fixed the actual bug). 5. Making class members private where possible. 6. Renaming class methods to our usual conventions. Signed-off-by: Chris Lalancette <[email protected]> Move always_false_v to detail namespace (ros2#2232) Since this is a common idiom, especially under this name, we should define the `always_false_v` template within a namespace to avoid conflict with other libraries and user code. This could either be `rclcpp::detail` if it's intended only for internal use or just `rclcpp` if it's intended as a public helper. In this PR, I've initially chosen the former. Signed-off-by: Nathan Wiebe Neufeldt <[email protected]> Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (ros2#2224) * TypeDescriptions interface with readonly param configuration * Add parameter descriptor, to make read only * example of spinning in thread for get_type_description service * Add a basic test for the new interface * Fix tests with new parameter * Add comments about builtin parameters Signed-off-by: Emerson Knapp <[email protected]> Signed-off-by: William Woodall <[email protected]> Switch lifecycle to use the RCLCPP macros. (ros2#2233) This ensures that they'll go out to /rosout and the disk. Signed-off-by: Chris Lalancette <[email protected]> Implement get_node_type_descriptions_interface for lifecyclenode and add smoke test for it (ros2#2237) Signed-off-by: Emerson Knapp <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 22.0.0 Stop using constref signature of benchmark DoNotOptimize. (ros2#2238) * Stop using constref signature of benchmark DoNotOptimize. Newer versions of google benchmark (1.8.2 in my case) warn that the compiler may optimize away the DoNotOptimize calls when using the constref version. Get away from that here by explicitly *not* calling the constref version, casting where necessary. Signed-off-by: Chris Lalancette <[email protected]> Instrument loaned message publication code path (ros2#2240) Signed-off-by: Christophe Bedard <[email protected]> associated clocks should be protected by mutex. (ros2#2255) Signed-off-by: Tomoya Fujita <[email protected]> Change associated clocks storage to unordered_set (ros2#2257) Signed-off-by: Luca Della Vedova <[email protected]> Adding Missing Group Exceptions (ros2#2256) * Adding Missing Group Exceptions Signed-off-by: CursedRock17 <[email protected]> Signed-off-by: Chris Lalancette <[email protected]> Co-authored-by: Tomoya Fujita <[email protected]> Add spin_all shortcut (ros2#2246) Signed-off-by: Tony Najjar <[email protected]> Remove an unused variable from the events executor tests. (ros2#2270) Signed-off-by: Chris Lalancette <[email protected]> Add a pimpl inside rclcpp::Node for future distro backports (ros2#2228) * Add a pimpl inside rclcpp::Node for future distro backports Signed-off-by: Emerson Knapp <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Adding Custom Unknown Type Error (ros2#2272) Signed-off-by: CursedRock17 <[email protected]> Co-authored-by: Christophe Bedard <[email protected]> Do not crash Executor when send_response fails due to client failure. (ros2#2276) * Do not crash Executor when send_response fails due to client failure. Related to ros2/ros2#1253 It is not sane that a faulty client can crash our service Executor, as discussed in the referred issue, if the client is not setup properly, send_response may return RCL_RET_TIMEOUT, we should not throw an error in this case. Signed-off-by: Zang MingJie <[email protected]> * Update rclcpp/include/rclcpp/service.hpp Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Zang MingJie <[email protected]> * address review comments. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Zang MingJie <[email protected]> Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Zang MingJie <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 22.1.0 doc fix: call `canceled` only after goal state is in canceling. (ros2#2266) Signed-off-by: Tomoya Fujita <[email protected]> Fix a typo in a comment. (ros2#2283) Signed-off-by: Chris Lalancette <[email protected]> Revamp list_parameters to be more efficient and easier to read. (ros2#2282) 1. Use constref for the loop variable. 2. Do more work outside of the loop. 3. Skip doing unnecessary work where we can inside the loop. With this in place, I measured about a 7% performance improvement over the previous implementation. Signed-off-by: Chris Lalancette <[email protected]> Add rcl_logging_interface as an explicit dependency. (ros2#2284) It is depended on by rclcpp/src/rclcpp/logger.cpp, but the dependency was not explicitly declared (it was being inherited from rcl, I believe). Fix that here. Signed-off-by: Chris Lalancette <[email protected]> add logger level service to lifecycle node. (ros2#2277) Signed-off-by: Tomoya Fujita <[email protected]> Remove unnecessary lambda captures in the tests. (ros2#2289) * Remove unnecessary lambda captures in the tests. This was pointed out by compiling with clang. Signed-off-by: Chris Lalancette <[email protected]> Correct the position of a comment. (ros2#2290) Signed-off-by: Jiaqi Li <[email protected]> Make Rate to select the clock to work with (ros2#2123) * Make Rate to select the clock to work with Add ROSRate respective with ROS time * Make GenericRate class to be deprecated * Adjust test cases for new rates is_steady() to be deprecated Signed-off-by: Alexey Merzlyakov <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Fix C 20 allocator construct deprecation (ros2#2292) Signed-off-by: Guilherme Rodrigues <[email protected]> Topic correct typeadapter deduction (ros2#2294) * fix TypeAdapter deduction Signed-off-by: Chen Lihui <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 22.2.0 Cleanup flaky timers_manager tests. (ros2#2299) * Cleanup flaky timers_manager tests. The timers_manager tests were relying too heavily on specific timings; this caused them to be flaky on the buildfarm, particularly on Windows. Here, we increase the timeouts, and remove one test which just relies too heavily on specific timeouts. This should make this test much less flaky on the buildfarm. Signed-off-by: Chris Lalancette <[email protected]> fix(ClientGoalHandle): Made mutex recursive to prevent deadlocks (ros2#2267) * fix(ClientGoalHandle): Made mutex recursive to prevent deadlocks This prevents deadlocks in cases, were e.g. get_status() would be called on the handle in a callback of the handle. * test(rclcpp_action): Added test for deadlocks during access of a goal handle This test checks, if the code deadlocks, if methods on the goal handle are called from the callbacks. Signed-off-by: Janosch Machowinski <[email protected]> Co-authored-by: Tomoya Fujita <[email protected]> Update API docs links in package READMEs (ros2#2302) Signed-off-by: Christophe Bedard <[email protected]> Fix the return type of Rate::period. (ros2#2301) In a recent commit (bc43577), we reworked how the Rate class worked so it could be used with ROS time as well. Unfortunately, we also accidentally broke the API of it by changing the return type of Rate::period to a Duration instead of a std::chrono::nanoseconds . Put this back to a std::chrono::nanoseconds; if we want to change it to a Duration, we'll have to add a new method and deprecate this one. Signed-off-by: Chris Lalancette <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 23.0.0 fix the depth to relative in list_parameters (ros2#2300) * fix the depth to relative in list_parameters Signed-off-by: leeminju531 <[email protected]> Decouple rosout publisher init from node init. (ros2#2174) Signed-off-by: Tomoya Fujita <[email protected]> Documentation for list_parameters (ros2#2315) * list_parameters documentation Signed-off-by: CursedRock17 <[email protected]> Removing Old Connext Tests (ros2#2313) * Removing Old Connext Tests Signed-off-by: CursedRock17 <[email protected]> Update SignalHandler get_global_signal_handler to avoid complex types in static memory (ros2#2316) * Update SignalHandler get_global_signal_handler to avoid complex types in static memory This was flagged by msan as a problem. There's a description of why this is a potential problem here: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Signed-off-by: Tully Foote <[email protected]> Co-authored-by: William Woodall <william [email protected]> Add locking to protect the TimeSource::NodeState::node_base_ (ros2#2320) We need this because it is possible for one thread to be handling the on_parameter_event callback while another one is detaching the node. This lock will protect that from happening. Signed-off-by: Chris Lalancette <[email protected]> Add missing header required by the rclcpp::NodeOptions type (ros2#2324) Signed-off-by: Ignacio Vizzo <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 23.1.0 Adding API to copy all parameters from one node to another (ros2#2304) Signed-off-by: stevemacenski <[email protected]> remove invalid sized allocation test for SerializedMessage. (ros2#2330) Signed-off-by: Tomoya Fujita <[email protected]> add clients & services count (ros2#2072) * add clients & services count * add count clients,services tests Signed-off-by: leeminju531 <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 23.2.0 Fix rclcpp_lifecycle inclusion on Windows. (ros2#2331) The comment in the commit explains this clearly, but on Windows ERROR is a macro. The reuse of it, even as an enum, causes compilation errors on downstream users. Push the macro and undefine it so downstream consumers can freely include it. Signed-off-by: Chris Lalancette <[email protected]> Remove useless ROSRate class (ros2#2326) Signed-off-by: Alexey Merzlyakov <[email protected]> Fixes pointed out by the clang analyzer. (ros2#2339) 1. Remove the default Logger copy constructor without copy assignment (rule of three -> rule of zero). 2. Remove an unnecessary capture in a lambda. 3. Mark a variable unused. 4. Mark a method as override. Signed-off-by: Chris Lalancette <[email protected]> address rate related flaky tests. (ros2#2329) Signed-off-by: Tomoya Fujita <[email protected]> Adjust rclcpp usage of type description service (ros2#2344) Signed-off-by: Michael Carroll <[email protected]> Add missing 'enable_rosout' comments (ros2#2345) Signed-off-by: Jiaqi Li <[email protected]> Use message_info in SubscriptionTopicStatistics instead of typed message (ros2#2337) * Use message_info in SubscriptionTopicStatistics instead of typed message - Untemplatize the rclcpp::topic_statistics::SubscriptionTopicStatistics class. Now we will be using message_info instead of typed deserialized messages in the handle_message callbacks. * Fix test_receive_stats_include_window_reset by using publisher emulator - Emulate publishing messages by directly calling rclcpp::Subscription::handle_message(msg_shared_ptr, message_info) and settling up needed message_info.source_timestamp Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Tomoya Fujita <[email protected]> Disable the loaned messages inside the executor. (ros2#2335) * Disable the loaned messages inside the executor. They are currently unsafe to use; see the comment in the commit for more information. Signed-off-by: Chris Lalancette <[email protected]> Add a custom deleter when constructing rcl_service_t (ros2#2351) * Add a custom deleter when constructing rcl_service_t In the type description service construction, we were previously passing the shared_ptr to the rcl_service_t with the assumption that rclcpp::Service would do the clean up. This was an incorrect assumption, and so I have added a custom deleter to fini the service and delete when the shared_ptr is cleaned up. Signed-off-by: Michael Carroll <[email protected]> Serialized Messages with Topic Statistics (ros2#2274) Signed-off-by: CursedRock17 <[email protected]> rclcpp::Time::max() clock type support. (ros2#2352) Signed-off-by: Tomoya Fujita <[email protected]> Updates to not use std::move in some places. (ros2#2353) gcc 13.1.1 complains that these uses inhibit copy elision. Signed-off-by: Chris Lalancette <[email protected]> fix (signal_handler.hpp): spelling (ros2#2356) Signed-off-by: Zard-C <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 24.0.0 fix(rclcpp_components): increase the service queue sizes in component_container (ros2#2363) * fix(rclcpp_components): increase the service queue sizes in component_container Signed-off-by: M. Fatih Cırıt <[email protected]> Support users holding onto shared pointers in the message memory pool (ros2#2336) * Support users holding onto shared pointers in the message memory pool Before this commit, the MessageMemoryPool would actually reuse messages in the pool, even if the user had taken additional shared_ptr copies. This commit fixes things so that we properly handle that situation. In particular, we allocate memory during class initialization, and delete it during destruction. We then run the constructor when we hand the pointer out, and the destructor (only) when we return it to the pool. This keeps things consistent. We also add in locks, since in a multi-threaded scenario we need to protect against multiple threads accessing the pool at the same time. With this in place, things work as expected when users hold shared_ptr copies. We also add in a test for this situation. One note about performance: this update preserves the "no-allocations-at-runtime" aspect of the MessagePool. However, there are some tradeoffs with CPU time here, particularly with very large message pools. This could probably be optimized further to do less work when trying to add items back to the free_list, but I view that as a further enhancement. Signed-off-by: Chris Lalancette <[email protected]> Fix data race in EventHandlerBase (ros2#2349) Both the EventHandler and its associated pubs/subs share the same underlying rmw event listener. When a pub/sub is destroyed, the listener is destroyed. There is a data race when the ~EventHandlerBase wants to access the listener after it has been destroyed. The EventHandler stores a shared_ptr of its associated pub/sub. But since we were clearing the listener event callbacks on the base class destructor ~EventHandlerBase, the pub/sub was already destroyed, which means the rmw event listener was also destroyed, thus causing a segfault when trying to obtain it to clear the callbacks. Clearing the callbacks on ~EventHandler instead of ~EventHandlerBase avoids the race, since the pub/sub are still valid. Signed-off-by: Mauro Passerino <[email protected]> aligh with rcl that a rosout publisher of a node might not exist (ros2#2357) * aligh with rcl * keep same behavior with rclpy 1. to not throw exception durning rcl_logging_rosout_remove_sublogger 2. reset error message for RCL_RET_NOT_FOUND * test for a node with rosout disabled Signed-off-by: Chen Lihui <[email protected]> feat(rclcpp_components): support events executor in node main template (ros2#2366) Signed-off-by: wep21 <[email protected]> Switch to target_link_libraries. (ros2#2374) That way we can hide more of the implementation by using the PRIVATE keyword. Signed-off-by: Chris Lalancette <[email protected]> Adding QoS to subscription options (ros2#2323) * Adding QoS to subscription options Signed-off-by: CursedRock17 <[email protected]> make type support helper supported for service (ros2#2209) * make type support helper supported for service and action as well Signed-off-by: Chen Lihui <[email protected]> * not to use template and only add the necessary service type currently Signed-off-by: Chen Lihui <[email protected]> * update comment Signed-off-by: Chen Lihui <[email protected]> * add deprecated cycle for `get_typesupport_handle` Signed-off-by: Chen Lihui <[email protected]> --------- Signed-off-by: Chen Lihui <[email protected]> Updated GenericSubscription to AnySubscriptionCallback (ros2#1928) * added rclcpp::SerializedMessage support for AnySubscriptionCallback Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> * using AnySubscription callback for generic subscriptiion Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> * updated tests Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> * Remove comment Signed-off-by: Joshua Hampp <[email protected]> --------- Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]> Co-authored-by: Joshua Hampp <[email protected]> Co-authored-by: Jacob Perron <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 25.0.0 Increase timeout for rclcpp_lifecycle to 360 (ros2#2395) * Increase timeout for rclcpp_lifecycle to 360 Signed-off-by: Jorge Perez <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Stop storing the context in the guard condition. (ros2#2400) * Stop storing the context in the guard condition. This was creating a circular reference between GuardCondition and Context, so that Context would never be cleaned up. Since we never really need the GuardCondition to know about its own Context, remove that part of the circular reference. While we are in here, we also change the get_context() lambda to a straight weak_ptr; there is no reason for the indirection since the context for the guard condition cannot change at runtime. We also remove the deprecated version of the get_notify_guard_condition(). That's because there is no way to properly implement it in the new scheme, and it seems to be unused outside of rclcpp. Finally, we add in a test that guarantees the use_count is what we expect when inside and leaving a scope, ensuring that contexts will properly be destroyed. Signed-off-by: Chris Lalancette <[email protected]> Add transient local durability support to publisher and subscriptions when using intra-process communication (ros2#2303) * Add intra process transient local durability support to publisher and subscription Signed-off-by: Jeffery Hsu <[email protected]> * Remove durability_is_transient_local_ from publisher_base Signed-off-by: Jeffery Hsu <[email protected]> * Design changes that move most transient local publish functionalities out of intra process manager into intra process manager Signed-off-by: Jeffery Hsu <[email protected]> * Move transient local publish to a separate function Signed-off-by: Jeffery Hsu <[email protected]> * Remove publisher buffer weak ptr from intra process manager when it associated publisher is removed. Signed-off-by: Jeffery Hsu <[email protected]> * Remove incorrectly placed RCLCPP_PUBLIC Signed-off-by: Jeffery Hsu <[email protected]> * Add missing RCLCPP_PUBLIC Signed-off-by: Jeffery Hsu <[email protected]> * Expand RingBufferImplementation beyond shared_ptr and unique_ptr Signed-off-by: Jeffery Hsu <[email protected]> * Comment and format fix Signed-off-by: Jeffery Hsu <[email protected]> --------- Signed-off-by: Jeffery Hsu <[email protected]> Increase the cppcheck timeout to 600 seconds. (ros2#2409) Signed-off-by: Chris Lalancette <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 26.0.0 Make sure to mark RingBuffer methods as 'override'. (ros2#2410) This gets rid of a warning when building under clang. Signed-off-by: Chris Lalancette <[email protected]> Removed deprecated header (ros2#2413) Signed-off-by: Alejandro Hernández Cordero <[email protected]> [events executor] - Fix Behavior with Timer Cancel (ros2#2375) * fix Signed-off-by: Matt Condino <[email protected]> * add timer cancel tests Signed-off-by: Matt Condino <[email protected]> * cleanup header include Signed-off-by: Matt Condino <[email protected]> * reverting change to timer_greater function Signed-off-by: Gus Brigantino <[email protected]> * use std::optional, and handle edgecase of 1 cancelled timer Signed-off-by: Matt Condino <[email protected]> * clean up run_timers func Signed-off-by: Gus Brigantino <[email protected]> * some fixes and added tests for cancel then reset of timers. Signed-off-by: Matt Condino <[email protected]> * refactor and clean up. remove cancelled timer tracking. Signed-off-by: Matt Condino <[email protected]> * remove unused method for size() Signed-off-by: Matt Condino <[email protected]> * linting Signed-off-by: Matt Condino <[email protected]> * relax timing constraints in tests Signed-off-by: Matt Condino <[email protected]> * further relax timing constraints to ensure windows tests are not flaky. Signed-off-by: Matt Condino <[email protected]> * use sim clock for tests, pub clock at .25 realtime rate. Signed-off-by: Matt Condino <[email protected]> --------- Signed-off-by: Matt Condino <[email protected]> Signed-off-by: Gus Brigantino <[email protected]> Co-authored-by: Gus Brigantino <[email protected]> Split test_executors up into smaller chunks. (ros2#2421) The original reason is that on Windows Debug, we were failing to compile because the compilation unit was too large. But also this file was way too large (1200 lines), so it makes sense to split it up. Signed-off-by: Chris Lalancette <[email protected]> Changelog. Signed-off-by: Chris Lalancette <[email protected]> 27.0.0 feat: add/minus for msg::Time and rclcpp::Duration (ros2#2419) * feat: add/minus for msg::Time and rclcpp::Duration Signed-off-by: HuaTsai <[email protected]> crash on no class found (ros2#2415) * crash on no class found * error on no class found instead of no callback groups Signed-off-by: Adam Aposhian <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Update quality declaration documents (ros2#2427) Signed-off-by: Christophe Bedard <[email protected]> Set hints to find the python version we actually want. (ros2#2426) The comment in the commit explains the reasoning behind it. Signed-off-by: Chris Lalancette <[email protected]> fix doxygen syntax for NodeInterfaces (ros2#2428) Signed-off-by: Jonas Otto <[email protected]> Remove the set_deprecated signatures in any_subscription_callback. (ros2#2431) These have been deprecated since April 2021, so it is safe to remove them now. Signed-off-by: Chris Lalancette <[email protected]> Add EXECUTOR docs (ros2#2440) Signed-off-by: Ruddick Lawrence <679360 [email protected]> Various cleanups to deal with uncrustify 0.78. (ros2#2439) These should also work with uncrustify 0.72. Signed-off-by: Chris Lalancette <[email protected]> Rule of five: implement move operators (ros2#2425) Signed-off-by: Tim Clephas <[email protected]> Implement generic client (ros2#2358) * Implement generic client Signed-off-by: Barry Xu <[email protected]> * Fix the incorrect parameter declaration Signed-off-by: Barry Xu <[email protected]> * Deleted copy constructor and assignment for FutureAndRequestId Signed-off-by: Barry Xu <[email protected]> * Update codes after rebase Signed-off-by: Barry Xu <[email protected]> * Address review comments Signed-off-by: Barry Xu <[email protected]> * Address review comments from iuhilnehc-ynos Signed-off-by: Barry Xu <[email protected]> * Correct an error in a description Signed-off-by: Barry Xu <[email protected]> * Fix window build errors Signed-off-by: Barry Xu <[email protected]> * Address review comments from William Signed-off-by: Barry Xu <[email protected]> * Add doc strings to create_generic_client Signed-off-by: Barry Xu <[email protected]> --------- Signed-off-by: Barry Xu <[email protected]> Fix TypeAdapted publishing with large messages. (ros2#2443) Mostly by ensuring we aren't attempting to store large messages on the stack. Also add in tests. I verified that before these changes, the tests failed, while after them they succeed. Signed-off-by: Chris Lalancette <[email protected]> Modify rclcpp_action::GoalUUID hashing algorithm (ros2#2441) * Add unit tests for hashing rclcpp_action::GoalUUID's * Use the FNV-1a hash algorithm for Goal UUID Signed-off-by: Mauro Passerino <[email protected]> relax the test simulation rate for timer canceling tests. (ros2#2453) Signed-off-by: Tomoya Fujita <[email protected]> Revert "relax the test simulation rate for timer canceling tests. (ros2#2453)" (ros2#2456) This reverts commit 1c350d0. Signed-off-by: Tomoya Fujita <[email protected]> enable simulation clock for timer canceling test. (ros2#2458) * enable simulation clock for timer canceling test. Signed-off-by: Tomoya Fujita <[email protected]> * move MainExecutorTypes to test_executors_timer_cancel_behavior.cpp. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Tomoya Fujita <[email protected]> refactor and improve the parameterized spin_some tests for executors (ros2#2460) * refactor and improve the spin_some parameterized tests for executors Signed-off-by: William Woodall <[email protected]> * disable spin_some_max_duration for the StaticSingleThreadedExecutor and EventsExecutor Signed-off-by: William Woodall <[email protected]> * fixup and clarify the docstring for Executor::spin_some() Signed-off-by: William Woodall <[email protected]> * style Signed-off-by: William Woodall <[email protected]> * review comments Signed-off-by: William Woodall <[email protected]> --------- Signed-off-by: William Woodall <[email protected]> fix spin_some_max_duration unit-test for events-executor (ros2#2465) Signed-off-by: Alberto Soragna <[email protected]> Do not generate the exception when action service response timeout. (ros2#2464) * Do not generate the exception when action service response timeout. Signed-off-by: Tomoya Fujita <[email protected]> * address review comment. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Tomoya Fujita <[email protected]> Changelog. Signed-off-by: Marco A. Gutierrez <[email protected]> 28.0.0 Signed-off-by: Marco A. Gutierrez <[email protected]> fix flakiness in TestTimersManager unit-test (ros2#2468) the previous version of the test was relying on the assumption that a timer with 1ms period gets called at least 6 times if the main thread waits 15ms. this is true most of the times, but it's not guaranteed, especially when running the test on windows CI servers. the new version of the test makes no assumptions on how much time it takes for the timers manager to invoke the timers, but rather focuses on ensuring that they are called the right amount of times, which is what's important for the purpose of the test Signed-off-by: Alberto Soragna <[email protected]> Utilize rclcpp::WaitSet as part of the executors (ros2#2142) * Deprecate callback_group call taking context Signed-off-by: Michael Carroll <[email protected]> * Add base executor objects that can be used by implementors Signed-off-by: Michael Carroll <[email protected]> * Template common operations Signed-off-by: Michael Carroll <[email protected]> * Address reviewer feedback: * Add callback to EntitiesCollector constructor * Make function to check automatically added callback groups take a list Signed-off-by: Michael Carroll <[email protected]> * Lint Signed-off-by: Michael Carroll <[email protected]> * Address reviewer feedback and fix templates Signed-off-by: Michael Carroll <[email protected]> * Lint and docs Signed-off-by: Michael Carroll <[email protected]> * Make executor own the notify waitable Signed-off-by: Michael Carroll <[email protected]> * Add pending queue to collector, remove from waitable Also change node's get_guard_condition to return shared_ptr Signed-off-by: Michael Carroll <[email protected]> * Change interrupt guard condition to shared_ptr Check if guard condition is valid before adding it to the waitable Signed-off-by: Michael Carroll <[email protected]> * Lint and docs Signed-off-by: Michael Carroll <[email protected]> * Utilize rclcpp::WaitSet as part of the executors Signed-off-by: Michael Carroll <[email protected]> * Don't exchange atomic twice Signed-off-by: Michael Carroll <[email protected]> * Fix add_node and add more tests Signed-off-by: Michael Carroll <[email protected]> * Make get_notify_guard_condition follow API tick-tock Signed-off-by: Michael Carroll <[email protected]> * Improve callback group tick-tocking Signed-off-by: Michael Carroll <[email protected]> * Don't lock twice Signed-off-by: Michael Carroll <[email protected]> * Address reviewer feedback Signed-off-by: Michael Carroll <[email protected]> * Add thread safety annotations and make locks consistent Signed-off-by: Michael Carroll <[email protected]> * @wip Signed-off-by: Michael Carroll <[email protected]> * Reset callback groups for multithreaded executor Signed-off-by: Michael Carroll <[email protected]> * Avoid many small function calls when building executables Signed-off-by: Michael Carroll <[email protected]> * Re-trigger guard condition if buffer has data Signed-off-by: Michael Carroll <[email protected]> * Address reviewer feedback Signed-off-by: Michael Carroll <[email protected]> * Trace points Signed-off-by: Michael Carroll <[email protected]> * Remove tracepoints Signed-off-by: Michael Carroll <[email protected]> * Reducing diff Signed-off-by: Michael Carroll <[email protected]> * Reduce diff Signed-off-by: Michael Carroll <[email protected]> * Uncrustify Signed-off-by: Michael Carroll <[email protected]> * Restore tests Signed-off-by: Michael Carroll <[email protected]> * Back to weak_ptr and reduce test time Signed-off-by: Michael Carroll <[email protected]> * reduce diff and lint Signed-off-by: Michael Carroll <[email protected]> * Restore static single threaded tests that weren't working before Signed-off-by: Michael Carroll <[email protected]> * Restore more tests Signed-off-by: Michael Carroll <[email protected]> * Fix multithreaded test Signed-off-by: Michael Carroll <[email protected]> * Fix assert Signed-off-by: Michael Carroll <[email protected]> * Fix constructor test Signed-off-by: Michael Carroll <[email protected]> * Change ready_executables signature back Signed-off-by: Michael Carroll <[email protected]> * Don't enforce removing callback groups before nodes Signed-off-by: Michael Carroll <[email protected]> * Remove the "add_valid_node" API Signed-off-by: Michael Carroll <[email protected]> * Only notify if the trigger condition is valid Signed-off-by: Michael Carroll <[email protected]> * Only trigger if valid and needed Signed-off-by: Michael Carroll <[email protected]> * Fix spin_some/spin_all implementation Signed-off-by: Michael Carroll <[email protected]> * Restore single threaded executor Signed-off-by: Michael Carroll <[email protected]> * Picking ABI-incompatible executor changes Signed-off-by: Michael Carroll <[email protected]> * Add PIMPL Signed-off-by: Michael Carroll <[email protected]> * Additional waitset prune Signed-off-by: Michael Carroll <[email protected]> * Fix bad merge Signed-off-by: Michael Carroll <[email protected]> * Expand test timeout Signed-off-by: Michael Carroll <[email protected]> * Introduce method to clear expired entities from a collection Signed-off-by: Michael Carroll <[email protected]> * Make sure to call remove_expired_entities(). Signed-off-by: Chris Lalancette <[email protected]> * Prune queued work when callback group is removed Signed-off-by: Michael Carroll <[email protected]> * Prune subscriptions from dynamic storage Signed-off-by: Michael Carroll <[email protected]> * Styles fixes. Signed-off-by: Chris Lalancette <[email protected]> * Re-trigger guard conditions Signed-off-by: Michael Carroll <[email protected]> * Condense to just use watiable.take_data Signed-off-by: Michael Carroll <[email protected]> * Lint Signed-off-by: Michael Carroll <[email protected]> * Address reviewer comments (nits) Signed-off-by: Michael Carroll <[email protected]> * Lock mutex when copying Signed-off-by: Michael Carroll <[email protected]> * Refactors to static single threaded based on reviewers Signed-off-by: Michael Carroll <[email protected]> * More small refactoring Signed-off-by: Michael Carroll <[email protected]> * Lint Signed-off-by: Michael Carroll <[email protected]> * Lint Signed-off-by: Michael Carroll <[email protected]> * Add ready executable accessors to WaitResult Signed-off-by: Michael Carroll <[email protected]> * Make use of accessors from wait_set Signed-off-by: Michael Carroll <[email protected]> * Fix tests Signed-off-by: Michael Carroll <[email protected]> * Fix more tests Signed-off-by: Michael Carroll <[email protected]> * Tidy up single threaded executor implementation Signed-off-by: Michael Carroll <[email protected]> * Don't null out timer, rely on call Signed-off-by: Michael Carroll <[email protected]> * change how timers are checked from wait result in executors Signed-off-by: William Woodall <[email protected]> * peak -> peek Signed-off-by: William Woodall <[email protected]> * fix bug in next_waitable logic Signed-off-by: William Woodall <[email protected]> * fix bug in StaticSTE that broke the add callback groups to executor tests Signed-off-by: William Woodall <[email protected]> * style Signed-off-by: William Woodall <[email protected]> --------- Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: Chris Lalancette <[email protected]> Signed-off-by: William Woodall <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Co-authored-by: William Woodall <[email protected]> update rclcpp::Waitable API to use references and const (ros2#2467) Signed-off-by: William Woodall <[email protected]> Add tracepoint for generic publisher/subscriber (ros2#2448) Signed-off-by: h-suzuki <[email protected]>

* Implement generic client Signed-off-by: Barry Xu <[email protected]> * Fix the incorrect parameter declaration Signed-off-by: Barry Xu <[email protected]> * Deleted copy constructor and assignment for FutureAndRequestId Signed-off-by: Barry Xu <[email protected]> * Update codes after rebase Signed-off-by: Barry Xu <[email protected]> * Address review comments Signed-off-by: Barry Xu <[email protected]> * Address review comments from iuhilnehc-ynos Signed-off-by: Barry Xu <[email protected]> * Correct an error in a description Signed-off-by: Barry Xu <[email protected]> * Fix window build errors Signed-off-by: Barry Xu <[email protected]> * Address review comments from William Signed-off-by: Barry Xu <[email protected]> * Add doc strings to create_generic_client Signed-off-by: Barry Xu <[email protected]> --------- Signed-off-by: Barry Xu <[email protected]> (cherry picked from commit 3df73f0)

Barry-Xu-2018 mentioned this pull request Nov 6, 2023

make type support helper supported for service #2209

Merged

Barry-Xu-2018 force-pushed the review/topic-generic-client-support branch from cac70b0 to 26628fe Compare December 6, 2023 07:20

Barry-Xu-2018 added 4 commits December 12, 2023 20:25

Implement generic client

beeadfc

Signed-off-by: Barry Xu <[email protected]>

Fix the incorrect parameter declaration

a1fca1a

Signed-off-by: Barry Xu <[email protected]>

Deleted copy constructor and assignment for FutureAndRequestId

5906087

Signed-off-by: Barry Xu <[email protected]>

Update codes after rebase

8ef18fe

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 force-pushed the review/topic-generic-client-support branch from 26628fe to 8ef18fe Compare December 12, 2023 12:41

Barry-Xu-2018 marked this pull request as ready for review December 12, 2023 12:44

Barry-Xu-2018 requested review from ivanpauno, hidmic and wjwwood as code owners December 12, 2023 12:44

Barry-Xu-2018 changed the title ~~[WIP] Implement generic client~~ Implement generic client Dec 12, 2023

fujitatomoya self-requested a review December 12, 2023 18:51

fujitatomoya self-assigned this Dec 12, 2023

fujitatomoya added the enhancement New feature or request label Dec 12, 2023

fujitatomoya reviewed Dec 12, 2023

View reviewed changes

Address review comments

f3933cf

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 mentioned this pull request Dec 22, 2023

Support service 2/2 --- rosbag2 service play ros2/rosbag2#1481

Merged

iuhilnehc-ynos reviewed Dec 22, 2023

View reviewed changes

Address review comments from iuhilnehc-ynos

8540fe6

Signed-off-by: Barry Xu <[email protected]>

fujitatomoya approved these changes Jan 4, 2024

View reviewed changes

rclcpp/include/rclcpp/generic_client.hpp Outdated Show resolved Hide resolved

Correct an error in a description

a73f5ed

Signed-off-by: Barry Xu <[email protected]>

Fix window build errors

da86048

Signed-off-by: Barry Xu <[email protected]>

wjwwood requested changes Jan 6, 2024

View reviewed changes

Address review comments from William

69145c4

Signed-off-by: Barry Xu <[email protected]>

wjwwood approved these changes Mar 4, 2024

View reviewed changes

rclcpp/include/rclcpp/create_generic_client.hpp Show resolved Hide resolved

Add doc strings to create_generic_client

6f0cec6

Signed-off-by: Barry Xu <[email protected]>

fujitatomoya merged commit 3df73f0 into ros2:rolling Mar 5, 2024
2 of 3 checks passed

fujitatomoya mentioned this pull request Apr 5, 2024

Service Type Adaptations #2285

Open

fujitatomoya mentioned this pull request Apr 18, 2024

Action Introspection and Record/Play Support ros2/ros2#1543

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement generic client #2358

Implement generic client #2358

Barry-Xu-2018 commented Nov 6, 2023 •

edited

Loading

fujitatomoya commented Dec 4, 2023

fujitatomoya commented Dec 4, 2023

Barry-Xu-2018 commented Dec 5, 2023

Barry-Xu-2018 commented Dec 12, 2023

Barry-Xu-2018 commented Dec 12, 2023

fujitatomoya left a comment

fujitatomoya commented Dec 12, 2023

iuhilnehc-ynos left a comment

Barry-Xu-2018 commented Dec 22, 2023

fujitatomoya left a comment

fujitatomoya commented Jan 4, 2024

Barry-Xu-2018 commented Jan 5, 2024

Barry-Xu-2018 commented Jan 5, 2024

Barry-Xu-2018 commented Jan 5, 2024

fujitatomoya commented Jan 5, 2024

Barry-Xu-2018 commented Jan 5, 2024

wjwwood left a comment

wjwwood Jan 6, 2024

Barry-Xu-2018 Jan 9, 2024

wjwwood Jan 6, 2024

Barry-Xu-2018 Jan 9, 2024

wjwwood Jan 9, 2024

Barry-Xu-2018 Jan 10, 2024

wjwwood Jan 6, 2024

Barry-Xu-2018 commented Jan 9, 2024

wjwwood commented Jan 9, 2024

wjwwood commented Jan 9, 2024

Barry-Xu-2018 commented Jan 12, 2024

fujitatomoya commented Mar 1, 2024

fujitatomoya commented Mar 1, 2024

Barry-Xu-2018 commented Mar 4, 2024 •

edited

Loading

wjwwood left a comment

	/// Clean all pending requests older than a time_point.
	/**
	* \param[in] time_point Requests that were sent before this point are going to be removed.
	* \param[inout] pruned_requests Removed requests id will be pushed to the vector
	* if a pointer is provided.
	* \return number of pending requests that were removed.
	*/
	template<typename AllocatorT = std::allocator<int64_t>>
	size_t
	prune_requests_older_than(
	std::chrono::time_point<std::chrono::system_clock> time_point,
	std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
	{
	std::lock_guard guard(pending_requests_mutex_);
	auto old_size = pending_requests_.size();
	for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
	if (it->second.first < time_point) {
	if (pruned_requests) {
	pruned_requests->push_back(it->first);
	}
	it = pending_requests_.erase(it);
	} else {
	it;
	}
	}
	return old_size - pending_requests_.size();
	}

	/*
	Testing on_new_response callbacks.
	*/
	TEST_F(TestClient, on_new_response_callback) {
	auto client_node = std::make_shared<rclcpp::Node>("client_node", "ns");
	auto server_node = std::make_shared<rclcpp::Node>("server_node", "ns");

	rclcpp::ServicesQoS client_qos;
	client_qos.keep_last(3);
	auto client = client_node->create_client<test_msgs::srv::Empty>("test_service", client_qos);
	std::atomic<size_t> server_requests_count {0};
	auto server_callback = [&server_requests_count](
	const test_msgs::srv::Empty::Request::SharedPtr,
	test_msgs::srv::Empty::Response::SharedPtr) {server_requests_count ;};
	auto server = server_node->create_service<test_msgs::srv::Empty>(
	"test_service", server_callback, client_qos);
	auto request = std::make_shared<test_msgs::srv::Empty::Request>();

	std::atomic<size_t> c1 {0};
	auto increase_c1_cb = [&c1](size_t count_msgs) {c1 = count_msgs;};
	client->set_on_new_response_callback(increase_c1_cb);

	client->async_send_request(request);
	auto start = std::chrono::steady_clock::now();
	while (server_requests_count == 0 &&
	(std::chrono::steady_clock::now() - start) < 10s)
	{
	rclcpp::spin_some(server_node);
	}

	ASSERT_EQ(server_requests_count, 1u);

	start = std::chrono::steady_clock::now();
	do {
	std::this_thread::sleep_for(100ms);
	} while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);

	EXPECT_EQ(c1.load(), 1u);

	std::atomic<size_t> c2 {0};
	auto increase_c2_cb = [&c2](size_t count_msgs) {c2 = count_msgs;};
	client->set_on_new_response_callback(increase_c2_cb);

	client->async_send_request(request);
	start = std::chrono::steady_clock::now();
	while (server_requests_count == 1 &&
	(std::chrono::steady_clock::now() - start) < 10s)
	{
	rclcpp::spin_some(server_node);
	}

	ASSERT_EQ(server_requests_count, 2u);

	start = std::chrono::steady_clock::now();
	do {
	std::this_thread::sleep_for(100ms);
	} while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);

	EXPECT_EQ(c1.load(), 1u);
	EXPECT_EQ(c2.load(), 1u);

	client->clear_on_new_response_callback();

	client->async_send_request(request);
	client->async_send_request(request);
	client->async_send_request(request);
	start = std::chrono::steady_clock::now();
	while (server_requests_count < 5 &&
	(std::chrono::steady_clock::now() - start) < 10s)
	{
	rclcpp::spin_some(server_node);
	}

	ASSERT_EQ(server_requests_count, 5u);

	std::atomic<size_t> c3 {0};
	auto increase_c3_cb = [&c3](size_t count_msgs) {c3 = count_msgs;};
	client->set_on_new_response_callback(increase_c3_cb);

	start = std::chrono::steady_clock::now();
	do {
	std::this_thread::sleep_for(100ms);
	} while (c3 < 3 && std::chrono::steady_clock::now() - start < 10s);

	EXPECT_EQ(c1.load(), 1u);
	EXPECT_EQ(c2.load(), 1u);
	EXPECT_EQ(c3.load(), 3u);

	std::function<void(size_t)> invalid_cb = nullptr;
	EXPECT_THROW(client->set_on_new_response_callback(invalid_cb), std::invalid_argument);
	}

Implement generic client #2358

Implement generic client #2358

Conversation

Barry-Xu-2018 commented Nov 6, 2023 • edited Loading

fujitatomoya commented Dec 4, 2023

fujitatomoya commented Dec 4, 2023

Barry-Xu-2018 commented Dec 5, 2023

Barry-Xu-2018 commented Dec 12, 2023

Barry-Xu-2018 commented Dec 12, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Dec 12, 2023

iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Dec 22, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jan 4, 2024

Barry-Xu-2018 commented Jan 5, 2024

Barry-Xu-2018 commented Jan 5, 2024

Barry-Xu-2018 commented Jan 5, 2024

fujitatomoya commented Jan 5, 2024

Barry-Xu-2018 commented Jan 5, 2024

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Jan 6, 2024

Choose a reason for hiding this comment

Barry-Xu-2018 Jan 9, 2024

Choose a reason for hiding this comment

wjwwood Jan 6, 2024

Choose a reason for hiding this comment

Barry-Xu-2018 Jan 9, 2024

Choose a reason for hiding this comment

wjwwood Jan 9, 2024

Choose a reason for hiding this comment

Barry-Xu-2018 Jan 10, 2024

Choose a reason for hiding this comment

wjwwood Jan 6, 2024

Choose a reason for hiding this comment

Barry-Xu-2018 commented Jan 9, 2024

wjwwood commented Jan 9, 2024

wjwwood commented Jan 9, 2024

Barry-Xu-2018 commented Jan 12, 2024

fujitatomoya commented Mar 1, 2024

fujitatomoya commented Mar 1, 2024

Barry-Xu-2018 commented Mar 4, 2024 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Nov 6, 2023 •

edited

Loading

Barry-Xu-2018 commented Mar 4, 2024 •

edited

Loading