Update services examples #317

threeal · 2021-07-03T14:46:04Z

Separate rclcpp's minimal client and minimal service examples each into lambda, member function, and not composable.

Signed-off-by: Alfi Maulana <alfi.maulana.f@gmail.com>

ralph-lange

Thank you for improving and extending the ROS 2 examples. I deem this to be important work!

I found an issue in case that the client node is canceled/stopped (e.g., by ctrl+c) while waiting for the service.

ralph-lange · 2021-12-14T12:38:01Z

rclcpp/services/minimal_client/lambda.cpp

@@ -0,0 +1,67 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


You may want to update the copyright information, at least with the year 2021.

Suggested change

// Copyright 2016 Open Source Robotics Foundation, Inc.

// Copyright 2024 Open Source Robotics Foundation, Inc.

Happy new year 🆕 can be applied to other new files.

ralph-lange · 2021-12-14T12:45:26Z

rclcpp/services/minimal_client/lambda.cpp

+    client_ = this->create_client<AddTwoInts>("add_two_ints");
+    while (!client_->wait_for_service(1s)) {
+      if (!rclcpp::ok()) {
+        RCLCPP_ERROR(this->get_logger(), "client interrupted while waiting for service to appear.");


To prevent an infinite loop, add return (which is valid in a ctor) to leave the loop, cf. the return 1; in not_composable.cpp. However, note that the spin(node) in the main function must not be called if rclcpp::ok() is already false since this will cause an the given context is not valid error. Probably it is better to move the waiting for the service into a one-shot timer as in the examples_rclcpp_minimal_action_client example.

i think @threeal tries to keep the current behavior here, i am okay with current behavior for this example so that user does not need to issue service 1st.

ralph-lange · 2021-12-14T12:46:03Z

rclcpp/services/minimal_client/member_function.cpp

+    client_ = this->create_client<AddTwoInts>("add_two_ints");
+    while (!client_->wait_for_service(1s)) {
+      if (!rclcpp::ok()) {
+        RCLCPP_ERROR(this->get_logger(), "client interrupted while waiting for service to appear.");


See my above comment about leaving the loop by a return.

ralph-lange · 2021-12-14T12:53:56Z

rclcpp/services/minimal_client/not_composable.cpp


 int main(int argc, char * argv[])
 {
  rclcpp::init(argc, argv);
  auto node = rclcpp::Node::make_shared("minimal_client");
  auto client = node->create_client<AddTwoInts>("add_two_ints");
-  while (!client->wait_for_service(std::chrono::seconds(1))) {
+  while (!client->wait_for_service(1s)) {
    if (!rclcpp::ok()) {
      RCLCPP_ERROR(node->get_logger(), "client interrupted while waiting for service to appear.");


Suggested change

RCLCPP_ERROR(node->get_logger(), "client interrupted while waiting for service to appear.");

RCLCPP_ERROR(rclcpp::get_logger("rclcpp"), "client interrupted while waiting for service to appear.");

With the node's logger, the publisher for rosout will occasionally raise a publisher's context is invalid exception on this line.

ralph-lange · 2021-12-14T13:03:23Z

rclcpp/services/minimal_client/lambda.cpp

+    client_ = this->create_client<AddTwoInts>("add_two_ints");
+    while (!client_->wait_for_service(1s)) {
+      if (!rclcpp::ok()) {
+        RCLCPP_ERROR(this->get_logger(), "client interrupted while waiting for service to appear.");


Once the return issue is solved, it is probably also necessary to change the logger to rclcpp::get_logger("rclcpp") as in not_composable.cpp.

Why? this log is not obviously for rclcpp? so getting logger from this(Node) is fine? (saying current code looks fine.)

ralph-lange · 2021-12-14T13:03:33Z

rclcpp/services/minimal_client/member_function.cpp

+    client_ = this->create_client<AddTwoInts>("add_two_ints");
+    while (!client_->wait_for_service(1s)) {
+      if (!rclcpp::ok()) {
+        RCLCPP_ERROR(this->get_logger(), "client interrupted while waiting for service to appear.");


Once the return issue is solved, it is probably also necessary to change the logger to rclcpp::get_logger("rclcpp") as in not_composable.cpp.

fujitatomoya · 2024-01-02T05:30:25Z

@threeal this's been 3 years. still working on this? if you are, could you please rebase this?

threeal · 2024-01-02T14:21:15Z

I'm sorry, i almost forgot i have this PR, i'll try to work on this when i have time.

fujitatomoya

@threeal a couple of comments. thank you for the contribution.

fujitatomoya · 2024-01-02T19:21:02Z

rclcpp/services/minimal_client/lambda.cpp

@@ -0,0 +1,67 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


Suggested change

// Copyright 2016 Open Source Robotics Foundation, Inc.

// Copyright 2024 Open Source Robotics Foundation, Inc.

Happy new year 🆕 can be applied to other new files.

fujitatomoya · 2024-01-02T19:26:50Z

rclcpp/services/minimal_client/lambda.cpp

+    client_ = this->create_client<AddTwoInts>("add_two_ints");
+    while (!client_->wait_for_service(1s)) {
+      if (!rclcpp::ok()) {
+        RCLCPP_ERROR(this->get_logger(), "client interrupted while waiting for service to appear.");


i think @threeal tries to keep the current behavior here, i am okay with current behavior for this example so that user does not need to issue service 1st.

fujitatomoya · 2024-01-02T19:28:26Z

rclcpp/services/minimal_client/lambda.cpp

+    client_ = this->create_client<AddTwoInts>("add_two_ints");
+    while (!client_->wait_for_service(1s)) {
+      if (!rclcpp::ok()) {
+        RCLCPP_ERROR(this->get_logger(), "client interrupted while waiting for service to appear.");


Why? this log is not obviously for rclcpp? so getting logger from this(Node) is fine? (saying current code looks fine.)

threeal added 5 commits July 3, 2021 21:46

Change services main example into not composable

5de6121

Signed-off-by: Alfi Maulana <alfi.maulana.f@gmail.com>

Add a service member function example

fcc2137

Signed-off-by: Alfi Maulana <alfi.maulana.f@gmail.com>

Add a client member function example

32cef87

Signed-off-by: Alfi Maulana <alfi.maulana.f@gmail.com>

Add a service lambda example

45817c7

Signed-off-by: Alfi Maulana <alfi.maulana.f@gmail.com>

Add a client lambda example and remove unused header

9d79427

Signed-off-by: Alfi Maulana <alfi.maulana.f@gmail.com>

threeal force-pushed the update-services-examples branch from f6173ae to 9d79427 Compare July 3, 2021 14:46

ralph-lange suggested changes Dec 14, 2021

View reviewed changes

clalancette assigned adityapande-1995 Feb 3, 2022

audrow changed the base branch from master to rolling June 28, 2022 14:18

tylerjw mentioned this pull request Dec 9, 2023

Migrate std::bind calls to lambda expressions ros2/ros2_documentation#4041

Merged

fujitatomoya requested changes Jan 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update services examples #317

Update services examples #317

threeal commented Jul 3, 2021

ralph-lange left a comment

ralph-lange Dec 14, 2021

fujitatomoya Jan 2, 2024

ralph-lange Dec 14, 2021

fujitatomoya Jan 2, 2024

ralph-lange Dec 14, 2021

ralph-lange Dec 14, 2021

ralph-lange Dec 14, 2021

fujitatomoya Jan 2, 2024

ralph-lange Dec 14, 2021

fujitatomoya commented Jan 2, 2024

threeal commented Jan 2, 2024 •

edited

Loading

fujitatomoya left a comment

fujitatomoya Jan 2, 2024

fujitatomoya Jan 2, 2024

fujitatomoya Jan 2, 2024

		@@ -0,0 +1,67 @@
		// Copyright 2016 Open Source Robotics Foundation, Inc.

	// Copyright 2016 Open Source Robotics Foundation, Inc.
	// Copyright 2024 Open Source Robotics Foundation, Inc.

	RCLCPP_ERROR(node->get_logger(), "client interrupted while waiting for service to appear.");
	RCLCPP_ERROR(rclcpp::get_logger("rclcpp"), "client interrupted while waiting for service to appear.");

Update services examples #317

Are you sure you want to change the base?

Update services examples #317

Conversation

threeal commented Jul 3, 2021

ralph-lange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya commented Jan 2, 2024

threeal commented Jan 2, 2024 • edited Loading

fujitatomoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threeal commented Jan 2, 2024 •

edited

Loading