support services run with multiple threads by iuhilnehc-ynos · Pull Request #315 · ros2/ros1_bridge

iuhilnehc-ynos · 2021-05-24T09:56:02Z

Fixes #314
Signed-off-by: Chen Lihui lihui.chen@sony.com

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

…ubsciption Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos · 2021-05-25T10:13:34Z

This patch is to:

make ros2 client call ros1 service via ros1_bridge at the same time if ros1 service is supported with multiple threads (Multi-threaded Spinning).

https://github.com/iuhilnehc-ynos/ros_tutorials/blob/44666088677028f4edeb6246b9f791c489dbb653/roscpp_tutorials/add_two_ints_server/add_two_ints_server.cpp#L57-L59

running ros2 run examples_rclcpp_minimal_client client_main& ros2 run examples_rclcpp_minimal_client client_main to expect this service can serve multiple requests at the same time.

[ INFO] [1621936604.891178820]: request: x=41, y=1
[ INFO] [1621936604.891784270]:   sending back response: [42]
[ INFO] [1621936604.892629346]: request: x=41, y=1
[ INFO] [1621936604.892643574]:   sending back response: [42]
              ^^^^^^^^^//\   the timestamp of two requests are almost same.

without this patch, the log as following,

[ INFO] [1621933196.546494184]: request: x=41, y=1
[ INFO] [1621933196.546513489]:   sending back response: [42]
[ INFO] [1621933198.556169433]: request: x=41, y=1
              ^^^^^^^^^^^^^^^^^^^^^/\   2 sec after to process the next request
[ INFO] [1621933198.556201921]:   sending back response: [42]

make ros1 client call ros2 service via ros1_bridge at the same time if ros2 service is supported with multiple threads(MultiThreadExecutor and a callback with reentrant).

https://github.com/iuhilnehc-ynos/ros2-examples/blob/0acb1b892f318ec7898b812d10765f538c93ebc7/rclcpp/services/minimal_service/main.cpp#L44-L46

running rosservice call /add_two_ints '{a: 3, b: 4}'& rosservice call /add_two_ints '{a: 3, b: 4}' expect to get the service log as following

[INFO] [1621936929.275491886] [minimal_service]: request: 3 + 4
[INFO] [1621936929.275632310] [minimal_service]: request: 3 + 4

without this patch, the log as following,

[INFO] [1621937408.339715576] [minimal_service]: request: 3 + 4
[INFO] [1621937410.340892268] [minimal_service]: request: 3 + 4

Notice that supporting the service with running multiple threads is for not only the one service(a custom callback group with reentrant set by user) but also multiple different services.

Could somebody help to review this PR?

and resotre the original main loop logic Signed-off-by: Chen Lihui <lihui.chen@sony.com>

to fix 'Not all nodes were finished before finishing the context' since ros2 PublisherBase passed into ros1 subscriber as parameter which might be freed after ros2 context. Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos · 2021-05-31T07:40:44Z

The failure about The following signatures were invalid: EXPKEYSIG F42ED6FBAB17C654 Open Robotics <info@osrfoundation.org> was mentioned on https://answers.ros.org/question/379190/apt-update-signatures-were-invalid-f42ed6fbab17c654/, I'll run retest this please after the CI is fixed.

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fujitatomoya · 2021-06-07T23:56:53Z

include/ros1_bridge/factory.hpp

+  auto group = node_base->get_default_callback_group();
+  if (topic_name.empty()) {
+    // create a callback group with Reentrant for creating ros2 clients and services
+    static auto s_shared_callbackgroup =


do we need this static memory? how about returning ros2_node->create_callback_group(rclcpp::CallbackGroupType::Reentrant) instead?

I want to reuse only one callbackgroup for all services and clients.

include/ros1_bridge/factory.hpp

fujitatomoya · 2021-06-08T00:16:08Z

include/ros1_bridge/factory.hpp

+  }
+
+  typedef std::map<std::string, rclcpp::CallbackGroup::SharedPtr> TopicCallbackGroupMap;
+  static TopicCallbackGroupMap s_topic_callbackgroups;


instead of introducing static auto s_shared_callbackgroup, can we have everything in this map including rclcpp::CallbackGroup::SharedPtr for service and client?

Using a specific string as a key is difficult and seems a little ugly, that's the reason why I introduce another static variable. If we can make an agreement, I'll update it. To use shared-callbackgroup as a key, which is not a valid topic name due to the character -, after that, it will not conflict with other topic names. what do you think?

Maybe I can just use the empty string as the key

fujitatomoya · 2021-06-08T00:18:58Z

src/dynamic_bridge.cpp


  // ROS 1 asynchronous spinner
-  ros::AsyncSpinner async_spinner(1);
+  ros::AsyncSpinner async_spinner(0, &ros1_callback_queue);


this will use the all cores in the system to spin. i think using Single or Multi threaded executor is dependent on the use case. for example, someone just bridge the certain topic only, in that case we do not need Multi threaded executor.

i think at least we would want to provide the option for user, then user can decide what they need.

OK, I'll introduce an option, and also append an argument (bool multithread = false) to some methods to check if it's necessary to create a custom callbackgroup or not.

fujitatomoya · 2021-06-08T00:19:06Z

src/dynamic_bridge.cpp


  // ROS 2 spinning loop
-  rclcpp::executors::SingleThreadedExecutor executor;
+  rclcpp::executors::MultiThreadedExecutor executor;


Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fujitatomoya

overall this looks good to me. but i guess we need to discuss on the changes in community.

fujitatomoya · 2021-06-08T12:39:49Z

src/dynamic_bridge.cpp

  bool bridge_all_topics = get_flag_option(args, "--bridge-all-topics");
  bridge_all_1to2_topics = bridge_all_topics || get_flag_option(args, "--bridge-all-1to2-topics");
  bridge_all_2to1_topics = bridge_all_topics || get_flag_option(args, "--bridge-all-2to1-topics");
+  multi_threads = get_flag_option(args, "--multi-threads");


option either all core will be used or single thread would be okay, but how about option to choose number of threads? 0 means all core threads, and if more threads are specified than platform possesses then print warning to use all core threads.

I think this needs to be discussed.

Yeah, maybe some guys want to set two different values for the threads count of ros1 and ros2 spinner.

fujitatomoya · 2021-08-25T02:17:23Z

@clalancette @sloretz friendly ping.

fujitatomoya · 2022-12-07T18:07:33Z

@gbiggs you might want to take a look, this fixes #314

joelbudu · 2023-03-23T18:38:58Z

Any chance in this getting merged? @iuhilnehc-ynos @sloretz @clalancette

iuhilnehc-ynos · 2023-03-24T03:25:13Z

@joelbudu

I am sorry about not figuring out an elegant fix for this. The answer is no for this PR, at least.

meetgandhi-dev · 2023-06-02T09:45:00Z

src/dynamic_bridge.cpp

  // ROS 1 asynchronous spinner
-  ros::AsyncSpinner async_spinner(1);
-  async_spinner.start();
+  std::unique_ptr<ros::AsyncSpinner> async_spinner = nullptr;


May be we create one more thread here and use ros::MultiThreadedSpinner?

Chen Lihui added 4 commits May 24, 2021 17:53

support services run with multiple threads

ba32b22

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fix unscrutify

16c85a6

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

use only one custom callback with reentrant for service, client and s…

f980aba

…ubsciption Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fix ros2 callback of ros1_bridge running with multiple threads

50ba40f

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos marked this pull request as ready for review May 25, 2021 10:13

Chen Lihui added 2 commits May 27, 2021 18:11

ensure that the message data of each topic is received in order

bb062af

and resotre the original main loop logic Signed-off-by: Chen Lihui <lihui.chen@sony.com>

use custom callbackqueue instead of ros1 global callbackqueue

597a205

to fix 'Not all nodes were finished before finishing the context' since ros2 PublisherBase passed into ros1 subscriber as parameter which might be freed after ros2 context. Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Chen Lihui added 2 commits May 31, 2021 16:39

adjust creating callbackgroup location and rename the variable

23d065f

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

update new tag version with v0.2 for ros-tooling/setup-ros

f96ce59

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fujitatomoya reviewed Jun 8, 2021

View reviewed changes

fujitatomoya added the enhancement New feature or request label Jun 8, 2021

Chen Lihui and others added 2 commits June 8, 2021 15:24

add option and refactor some code based on review

879dfb8

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com>

update test for --multi-threads option

0d41087

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos requested a review from fujitatomoya June 8, 2021 08:40

fujitatomoya approved these changes Jun 8, 2021

View reviewed changes

clalancette assigned sloretz Jul 1, 2021

msmcconnell mentioned this pull request Mar 3, 2022

Combined branch usdot-fhwa-stol/ros1_bridge#1

Merged

meetgandhi-dev reviewed Jun 2, 2023

View reviewed changes

Conversation

iuhilnehc-ynos commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iuhilnehc-ynos commented May 25, 2021

Uh oh!

iuhilnehc-ynos commented May 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fujitatomoya commented Aug 25, 2021

Uh oh!

fujitatomoya commented Dec 7, 2022

Uh oh!

joelbudu commented Mar 23, 2023

Uh oh!

iuhilnehc-ynos commented Mar 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iuhilnehc-ynos commented May 24, 2021 •

edited

Loading