Added support for executors #1
Conversation
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).
|
@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. |
|
+1 |
| /*** Populate class storage from stored weak node pointers and wait. ***/ | ||
|
|
||
| void | ||
| populate_all_handles(bool nonblocking) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Is the |
|
@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. |
| } | ||
|
|
||
| bool | ||
| ok() |
There was a problem hiding this comment.
Can we come up with a more descriptive name?
There was a problem hiding this comment.
I was just mirroring the ROS1 API here, but yeah I'm open to any name changes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The usage of |
|
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.
|
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. |
|
+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.
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 <[email protected]> * 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 <[email protected]> * Review feedback * Delete CHANGELOG.rst * Enable cppcheck * Remove all references to rosbag2/ros2bag Signed-off-by: Nikolai Morin <[email protected]> * Move rclpp_generic into rclcpp Signed-off-by: Nikolai Morin <[email protected]> * Rename namespace rclcpp_generic to rclcpp::generic Signed-off-by: Nikolai Morin <[email protected]> * Free 'create' functions instead of static functions in class Signed-off-by: Nikolai Morin <[email protected]> * Remove 'generic' subdirectory and namespace hierarchy Signed-off-by: Nikolai Morin <[email protected]> * Order includes according to style guide Signed-off-by: Nikolai Morin <[email protected]> * Remove extra README.md Signed-off-by: Nikolai Morin <[email protected]> * Also add brief to class docs Signed-off-by: Nikolai Morin <[email protected]> * Make ament_index_cpp a build_depend Signed-off-by: Nikolai Morin <[email protected]> * Add to rclcpp.hpp Signed-off-by: Nikolai Morin <[email protected]> * Remove memory_management, use rclcpp::SerializedMessage in GenericPublisher::publish Signed-off-by: Nikolai Morin <[email protected]> * Clean up the typesupport_helpers Signed-off-by: Nikolai Morin <[email protected]> * Use make_shared, add UnimplementedError Signed-off-by: Nikolai Morin <[email protected]> * Add more comments, make member variable private, remove unnecessary include Signed-off-by: Nikolai Morin <[email protected]> * Apply suggestions from code review Co-authored-by: William Woodall <[email protected]> Signed-off-by: Nikolai Morin <[email protected]> * Rename test Signed-off-by: Nikolai Morin <[email protected]> * Update copyright and remove ament_target_dependencies for test Signed-off-by: Nikolai Morin <[email protected]> * Accept PublisherOptions and SubscriptionOptions Signed-off-by: Nikolai Morin <[email protected]> * Remove target_include_directories Signed-off-by: Nikolai Morin <[email protected]> * Add explanatory comment to SubscriptionBase Signed-off-by: Nikolai Morin <[email protected]> * Use kSolibPrefix and kSolibExtension from rcpputils Signed-off-by: Nikolai Morin <[email protected]> * Fix downstream build failure by making ament_index_cpp a build_export_depend Signed-off-by: Nikolai Morin <[email protected]> * Use path_for_library(), fix documentation nitpicks Signed-off-by: Nikolai Morin <[email protected]> * Improve error handling in get_typesupport_handle Signed-off-by: Nikolai Morin <[email protected]> * Accept SubscriptionOptions in GenericSubscription Signed-off-by: Nikolai Morin <[email protected]> * Make use of PublisherOptions in GenericPublisher Signed-off-by: Nikolai Morin <[email protected]> * Document typesupport_helpers Signed-off-by: Nikolai Morin <[email protected]> * Improve documentation Signed-off-by: Nikolai Morin <[email protected]> * Use std::function instead of function pointer Co-authored-by: William Woodall <[email protected]> Signed-off-by: Nikolai Morin <[email protected]> * Minimize vertical whitespace Signed-off-by: Nikolai Morin <[email protected]> * Add TODO for callback with message info Signed-off-by: Nikolai Morin <[email protected]> * Link issue in TODO Signed-off-by: Nikolai Morin <[email protected]> * Add missing include for functional Signed-off-by: nnmm <[email protected]> * Fix compilation Signed-off-by: Jacob Perron <[email protected]> * Fix lint Signed-off-by: Jacob Perron <[email protected]> * Address review comments (#1) * fix redefinition of default template arguments Signed-off-by: Karsten Knese <[email protected]> * address review comments Signed-off-by: Karsten Knese <[email protected]> * rename test executable Signed-off-by: Karsten Knese <[email protected]> * add functionality to lifecycle nodes Signed-off-by: Karsten Knese <[email protected]> * Refactor typesupport helpers * Make extract_type_identifier function private * Remove unused extract_type_and_package function * Update unit tests Signed-off-by: Jacob Perron <[email protected]> * Remove note about ament from classes This comment only applies to the free functions. Signed-off-by: Jacob Perron <[email protected]> * Fix formatting Co-authored-by: Karsten Knese <[email protected]> * Fix warning Possible loss of data from double to rcutils_duration_value_t Signed-off-by: Jacob Perron <[email protected]> * Add missing visibility macros Signed-off-by: Jacob Perron <[email protected]> Co-authored-by: William Woodall <[email protected]> Co-authored-by: Jacob Perron <[email protected]> Co-authored-by: Karsten Knese <[email protected]>
Remove ones that aren't needed, and add in ones that are actually needed in the respective files. Signed-off-by: Chris Lalancette <[email protected]>
Remove ones that aren't needed, and add in ones that are actually needed in the respective files. Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Andrew Symington <[email protected]>
Signed-off-by: Andrew Symington <[email protected]>
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_likeexamples in ros2/examples#1.