Added support for executors #1

wjwwood · 2014-08-27T07:35:52Z

This pull request replaces the current Node.h with a larger interface which includes executors, groups, timers, and some other common utilities which are required to implement the ros1_like examples in ros2/examples#1.

also check to make sure given groups are in node

It differs from this_thread::sleep_for because it will exit on SIGINT (ctrl-c).

wjwwood · 2014-08-30T00:58:09Z

@dirk-thomas @tfoote @vmayoral

There are some rough edges on this still, but I would like to get it merged so we prevent further divergence from the master branch.

tfoote · 2014-09-02T07:23:53Z

+1

dirk-thomas · 2014-09-02T16:53:45Z

rclcpp/include/rclcpp/executor.hpp

+/*** Populate class storage from stored weak node pointers and wait. ***/
+
+  void
+  populate_all_handles(bool nonblocking)


This function calls wait internally which I wouldn't have expected based on the name.

It also allocates/deallocates the memory every time. Isn't that kind of a bottleneck especially when doing this with a higher frequency?

Yeah, it sort of evolved into the wait role, we can rename/reorganize. I guess it means populate_all_handles no matter what (wait or otherwise).

Can we avoid the memory reallocation for every cycle somehow? It would be great if it would only do so when the items of the wait set actually change.

I suppose you could reduce the number of times it happens by not deallocating it, but checking the size each time and only free'ing and re-allocing when the size changes. But that wouldn't eliminate the need to alloc here.

TODO's added in 35b51c5

dirk-thomas · 2014-09-02T17:13:28Z

Is the Context use anywhere / for anything yet?

wjwwood · 2014-09-02T17:15:27Z

@dirk-thomas No the Context is empty right now. I haven't figured out what its role is yet. I just reserved the API space for it when I thought I was about to use it, but I ended up not going down that rabbit hole yet.

dirk-thomas · 2014-09-02T17:17:23Z

rclcpp/include/rclcpp/utilities.hpp

+}
+
+bool
+ok()


Can we come up with a more descriptive name?

I was just mirroring the ROS1 API here, but yeah I'm open to any name changes.

One idea: is_shutting_down

is_shutting_down isn't that nice in my opinion because it implies an intermediate state between shutdown and running which may or may not be the case when called, rospy uses rospy.is_shutdown and you have to loop on the negative, i.e. while (!rclcpp::is_shutdown()). We could do while (rclcpp::not_shutdown) or while (rclcpp::is_not_shutdown), both of which read a little better.

I changed the name to wait_for_work because it really only gets called when wait will be called: f77e6d7

I also changed the way spin_node_some works and therefore needed to add a remove_node function and I also fixed up the add_node to make better checks and to interrupt wait when nodes are added/removed.

dirk-thomas · 2014-09-02T17:33:18Z

The usage of friend seems to oppose the goal of dog fooding. It makes it difficult for users to extend / replace the code with custom stuff. Isn't there a better way for the API?

wjwwood · 2014-09-02T18:44:46Z

The use of friend here was to prevent the user from trying to create instances of classes like Subscriber and Publisher, while allowing the Node class to do so. Another way would be to have friend free functions, which act as factories for Subscribers and Publishers. Yet another way would be to pass the node into the constructors of Subscribers and Publishers.

This allowed me to unify populate_all_handles and populate_all_handles_with_node into a single function wait_for_work. This means that spin_node_some now adds, then removes a node to the executor for calling wait_for_work. That implies I also added the remove_node function and while I was at it I put some more robust checking and wait interruption into add_node.

wjwwood · 2014-09-03T01:30:56Z

Ok, I think this pull request is good to go, there are still issues and todo's but I think we should merge it and ticket them out separately.

dirk-thomas · 2014-09-03T03:21:30Z

+1

Specifically we must always use sa_sigaction and then when chaining to the original signal handler we must determine if it is a sa_sigaction or a sa_handler type function and call it accordingly.

Added support for executors

Address code review feedback

#1452) * Copying files from rosbag2 The generic_* files are from rosbag2_transport typesupport_helpers incl. test is from rosbag2_cpp memory_management.hpp is from rosbag2_test_common test_pubsub.cpp was renamed from test_rosbag2_node.cpp from rosbag2_transport Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Rebrand into rclcpp_generic Add package.xml, CMakeLists.txt, Doxyfile, README.md and CHANGELOG.rst Rename namespaces Make GenericPublisher and GenericSubscription self-contained by storing shared library New create() methods that return shared pointers Add docstrings Include only what is needed Make linters & tests pass Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Review feedback * Delete CHANGELOG.rst * Enable cppcheck * Remove all references to rosbag2/ros2bag Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Move rclpp_generic into rclcpp Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Rename namespace rclcpp_generic to rclcpp::generic Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Free 'create' functions instead of static functions in class Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Remove 'generic' subdirectory and namespace hierarchy Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Order includes according to style guide Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Remove extra README.md Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Also add brief to class docs Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Make ament_index_cpp a build_depend Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Add to rclcpp.hpp Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Remove memory_management, use rclcpp::SerializedMessage in GenericPublisher::publish Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Clean up the typesupport_helpers Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Use make_shared, add UnimplementedError Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Add more comments, make member variable private, remove unnecessary include Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Apply suggestions from code review Co-authored-by: William Woodall <william+github@osrfoundation.org> Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Rename test Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Update copyright and remove ament_target_dependencies for test Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Accept PublisherOptions and SubscriptionOptions Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Remove target_include_directories Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Add explanatory comment to SubscriptionBase Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Use kSolibPrefix and kSolibExtension from rcpputils Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Fix downstream build failure by making ament_index_cpp a build_export_depend Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Use path_for_library(), fix documentation nitpicks Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Improve error handling in get_typesupport_handle Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Accept SubscriptionOptions in GenericSubscription Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Make use of PublisherOptions in GenericPublisher Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Document typesupport_helpers Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Improve documentation Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Use std::function instead of function pointer Co-authored-by: William Woodall <william+github@osrfoundation.org> Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Minimize vertical whitespace Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Add TODO for callback with message info Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Link issue in TODO Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai> * Add missing include for functional Signed-off-by: nnmm <nnmmgit@gmail.com> * Fix compilation Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix lint Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Address review comments (#1) * fix redefinition of default template arguments Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com> * address review comments Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com> * rename test executable Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com> * add functionality to lifecycle nodes Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com> * Refactor typesupport helpers * Make extract_type_identifier function private * Remove unused extract_type_and_package function * Update unit tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove note about ament from classes This comment only applies to the free functions. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix formatting Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com> * Fix warning Possible loss of data from double to rcutils_duration_value_t Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add missing visibility macros Signed-off-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: William Woodall <william+github@osrfoundation.org> Co-authored-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>

Remove ones that aren't needed, and add in ones that are actually needed in the respective files. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Andrew Symington <andrew.c.symington@gmail.com>

major refactor of rclcpp to support executors

b6e1a26

wjwwood mentioned this pull request Aug 27, 2014

Adding examples which use executors ros2/examples#1

Merged

wjwwood added 9 commits August 27, 2014 00:54

WIP changes to support multithreaded executor and timers

222a053

remove vestigial SingleThreadedExecutor header

029008b

add access control to groups using atomic_bool

b5b8451

refactor of executors for mutually exclusive cb's

b91dd6e

added convenience debugging macro

f4c97ec

modify factories in node to use groups

b78164e

also check to make sure given groups are in node

modify rates to allow ctrl-c to interrupt sleep

680e536

added global rclcpp::sleep_for function

55487d4

It differs from this_thread::sleep_for because it will exit on SIGINT (ctrl-c).

refactor timers to use rates

7b8b318

dirk-thomas reviewed Sep 2, 2014
View reviewed changes

dirk-thomas and others added 3 commits September 2, 2014 10:36

spelling

d39ca12

STE: remove vestigial overlaid storage for nodes

e4dc536

MTE: store number of threads and provide accessor

6f0e49d

wjwwood added 5 commits September 2, 2014 11:45

executor: make destructor virtual

cba6ee2

add some TODO's in the code to track known issues

35b51c5

executor: correctly mark some functions virtual

0f9351a

add sigint singal handler chaining, err checking

d474d0e

wjwwood added 2 commits September 3, 2014 17:47

fix compiler warning

7820b53

changed signal handling to consider sigaction

7d8e60d

Specifically we must always use sa_sigaction and then when chaining to the original signal handler we must determine if it is a sa_sigaction or a sa_handler type function and call it accordingly.

wjwwood added a commit that referenced this pull request Sep 4, 2014

Merge pull request #1 from ros2/executors

bc7b340

Added support for executors

wjwwood merged commit bc7b340 into master Sep 4, 2014

wjwwood deleted the executors branch September 4, 2014 00:51

charlotteheggem mentioned this pull request May 28, 2020

Rclcpp clock problems #1144

Closed

hidmic mentioned this pull request Jun 26, 2020

Fix pub/sub count API tests. #1203

Merged

BaiCai1990 mentioned this pull request Jan 23, 2021

rclcpp::spin_some(node) Occasionally crash. #1524

Closed

nuclearsandwich pushed a commit that referenced this pull request Mar 5, 2021

Merge pull request #1 from mjeronimo/mjeronimo/address-review-feedback

ab4ff32

Address code review feedback

clalancette added a commit to clalancette/rclcpp that referenced this pull request Dec 7, 2021

Cleanup includes. (ros2#1)

e757d84

Remove ones that aren't needed, and add in ones that are actually needed in the respective files. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

nightduck referenced this pull request in nightduck/rclcpp Dec 16, 2021

Heap-of-heaps attempt #1 (segfaults)

ff3aba1

clalancette added a commit to clalancette/rclcpp that referenced this pull request Dec 21, 2021

Cleanup includes. (ros2#1)

f29664e

Remove ones that aren't needed, and add in ones that are actually needed in the respective files. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

nightduck referenced this pull request in nightduck/rclcpp Apr 8, 2022

Heap-of-heaps attempt #1 (segfaults)

8d3fd55

nightduck referenced this pull request in nightduck/rclcpp Apr 8, 2022

Heap-of-heaps attempt #1 (segfaults)

dbbca27

nightduck referenced this pull request in nightduck/rclcpp Apr 8, 2022

Heap-of-heaps attempt #1 (segfaults)

4e5d719

asymingt added a commit to asymingt/rclcpp that referenced this pull request Aug 2, 2022

CI nondeterminism test ros2#1

58cc424

Signed-off-by: Andrew Symington <andrew.c.symington@gmail.com>

ivanpauno pushed a commit to asymingt/rclcpp that referenced this pull request Aug 26, 2022

CI nondeterminism test ros2#1

e3fffe2

Signed-off-by: Andrew Symington <andrew.c.symington@gmail.com>

firesurfer mentioned this pull request May 15, 2024

fix: Fixed race condition in action server between is_ready and take"… #2531

Merged

zxf101826 mentioned this pull request Jun 24, 2024

Segment fault occurred when node was added to executor if the callback group variable created was local #2445

Open

+              }
+              bool
+              ok()

Added support for executors #1

Added support for executors #1

Uh oh!

Conversation

wjwwood commented Aug 27, 2014

Uh oh!

wjwwood commented Aug 30, 2014

Uh oh!

tfoote commented Sep 2, 2014

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!

dirk-thomas commented Sep 2, 2014

Uh oh!

wjwwood commented Sep 2, 2014

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!

dirk-thomas commented Sep 2, 2014

Uh oh!

wjwwood commented Sep 2, 2014

Uh oh!

wjwwood commented Sep 3, 2014

Uh oh!

dirk-thomas commented Sep 3, 2014

Uh oh!

Uh oh!