Adding callback groups in executor (#1218)

* Initial version of callback-group-based Executor. Signed-off-by: Ralph Lange <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * removed RealTimeClass Signed-off-by: Pedro Pena <[email protected]> * can add multiple cbgs and check if callback is owned by another exec before adding Signed-off-by: Pedro Pena <[email protected]> * cbg var for option to add to executor Signed-off-by: Pedro Pena <[email protected]> * getter for callback groups in executor Signed-off-by: Pedro Pena <[email protected]> * test Signed-off-by: Pedro Pena <[email protected]> * add many nodes and callback groups together Signed-off-by: Pedro Pena <[email protected]> * test for map of callback groups and nodes Signed-off-by: Pedro Pena <[email protected]> * added a test for map and callback group duplication Signed-off-by: Pedro Pena <[email protected]> * add cbg that are not assign and allow to do so, only iterate through groups in maps Signed-off-by: Pedro Pena <[email protected]> * memory strat should only add handles that belong to it Signed-off-by: Pedro Pena <[email protected]> * fixed executor deconstructor seg fault bug Signed-off-by: Pedro Pena <[email protected]> * fixed remove node and guard condition bug Signed-off-by: Pedro Pena <[email protected]> * fixed uncrustify Signed-off-by: Pedro Pena <[email protected]> * cpplint Signed-off-by: Pedro Pena <[email protected]> * remove line break and add static executor in cmakelist Signed-off-by: Pedro Pena <[email protected]> * enabled static executor and added add callback group feature Signed-off-by: Pedro Pena <[email protected]> * fixed test_allocator_memory_strategy Signed-off-by: Pedro Pena <[email protected]> * test allocator Signed-off-by: Pedro Pena <[email protected]> * test mem strat with cbg feat Signed-off-by: Pedro Pena <[email protected]> * remove cbg in static executor Signed-off-by: Pedro Pena <[email protected]> * adapted guard conditions Signed-off-by: Pedro Pena <[email protected]> * collector deconstructor and remove cbg when remove node in static Signed-off-by: Pedro Pena <[email protected]> * fixed invalid group ptr seg fault introduced in wait for work Signed-off-by: Pedro Pena <[email protected]> * passes the test allocator mem strat Signed-off-by: Pedro Pena <[email protected]> * added weak node check in memory strategy; passes brawner unit tests Signed-off-by: Pedro Pena <[email protected]> * uncrustify for tests Signed-off-by: Pedro Pena <[email protected]> * lint and uncrustify Signed-off-by: Pedro Pena <[email protected]> * exposed allowable state at the node level and added unit tests Signed-off-by: Pedro Pena <[email protected]> * unit test to add one node mult executors Signed-off-by: Pedro Pena <[email protected]> * frixed allow executor reset bug Signed-off-by: Pedro Pena <[email protected]> * code block for callback group and executor Signed-off-by: Pedro Pena <[email protected]> * add code block for add/remove cbg Signed-off-by: Pedro Pena <[email protected]> * add comments for add/remove callback group Signed-off-by: Pedro Pena <[email protected]> * changed from atomic to const Signed-off-by: Pedro Pena <[email protected]> * fixed test different cbgs for nodes Signed-off-by: Pedro Pena <[email protected]> * lint Signed-off-by: Pedro Pena <[email protected]> * added disabled nodes in services and map Signed-off-by: Pedro Pena <[email protected]> * changed var name to suggestion Signed-off-by: Pedro Pena <[email protected]> * comment for callback group constructor Signed-off-by: Pedro Pena <[email protected]> * header ordering Signed-off-by: Pedro Pena <[email protected]> * Update rclcpp/include/rclcpp/executor.hpp Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * removed const ref and made protected Signed-off-by: Pedro Pena <[email protected]> * removing internals in comments Signed-off-by: Pedro Pena <[email protected]> * Apply suggestions from code review general fixes Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * remove white space Signed-off-by: Pedro Pena <[email protected]> * Apply suggestions from code review general fix Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * fix comments Signed-off-by: Pedro Pena <[email protected]> * Update rclcpp/include/rclcpp/executor.hpp Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * fix comments Signed-off-by: Pedro Pena <[email protected]> * fix comments Signed-off-by: Pedro Pena <[email protected]> * general fixes Signed-off-by: Pedro Pena <[email protected]> * clang tidy and llvm deprecation and overriden fixes Signed-off-by: Pedro Pena <[email protected]> * made typedtests Signed-off-by: Pedro Pena <[email protected]> * add has callback method for static executor Signed-off-by: Pedro Pena <[email protected]> * removed map function and added comment about remove callback group Signed-off-by: Pedro Pena <[email protected]> * adding two different data structures for add_node and add_callback_group Signed-off-by: Pedro Pena <[email protected]> * nitpick changes to documentation Signed-off-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * move implementation out of header Signed-off-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * use const & Signed-off-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * splitting add node and add cbg in static executro Signed-off-by: Pedro Pena <[email protected]> * get cbgs for static executor and collector Signed-off-by: Pedro Pena <[email protected]> * add weak nodes for nodes Signed-off-by: Pedro Pena <[email protected]> * get next ready executable with two maps Signed-off-by: Pedro Pena <[email protected]> * passes tests Signed-off-by: Pedro Pena <[email protected]> * Apply suggestions from code review Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * fixed has node function Signed-off-by: Pedro Pena <[email protected]> * fixed collect entities Signed-off-by: Pedro Pena <[email protected]> * added unit tests for removal and added 3rd data struct Signed-off-by: Pedro Pena <[email protected]> * eliminated cbs vector Signed-off-by: Pedro Pena <[email protected]> * reusing same functions and added comments Signed-off-by: Pedro Pena <[email protected]> * documentation, more exceptions, and name changes Signed-off-by: Pedro Pena <[email protected]> * Apply suggestions from code review changes for review Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * fixed deconstructor, first remove cbgs, then nodes Signed-off-by: Pedro Pena <[email protected]> * Apply suggestions from code review Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * fixed remove node issue Signed-off-by: Pedro Pena <[email protected]> * throw an exception in remove node of collector Signed-off-by: Pedro Pena <[email protected]> * Update rclcpp/include/rclcpp/executor.hpp Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> * Update rclcpp/include/rclcpp/executor.hpp Co-authored-by: William Woodall <[email protected]> Signed-off-by: Pedro Pena <[email protected]> Co-authored-by: Ralph Lange <[email protected]> Co-authored-by: William Woodall <[email protected]>
ros2 · Sep 2, 2020 · 01d6f52 · 01d6f52
1 parent 633e115
commit 01d6f52
Show file tree

Hide file tree

Showing 21 changed files with 2,080 additions and 644 deletions.
diff --git a/rclcpp/include/rclcpp/callback_group.hpp b/rclcpp/include/rclcpp/callback_group.hpp
@@ -56,8 +56,44 @@ class CallbackGroup
 public:
   RCLCPP_SMART_PTR_DEFINITIONS(CallbackGroup)
 
+  /// Constructor for CallbackGroup.
+  /**
+   * Callback Groups have a type, either 'Mutually Exclusive' or 'Reentrant'
+   * and when creating one the type must be specified.
+   *
+   * Callbacks in Reentrant Callback Groups must be able to:
+   *   - run at the same time as themselves (reentrant)
+   *   - run at the same time as other callbacks in their group
+   *   - run at the same time as other callbacks in other groups
+   *
+   * Callbacks in Mutually Exclusive Callback Groups:
+   *   - will not be run multiple times simultaneously (non-reentrant)
+   *   - will not be run at the same time as other callbacks in their group
+   *   - but must run at the same time as callbacks in other groups
+   *
+   * Additiionally, callback groups have a property which determines whether or
+   * not they are added to an executor with their associated node automatically.
+   * When creating a callback group the automatically_add_to_executor_with_node
+   * argument determines this behavior, and if true it will cause the newly
+   * created callback group to be added to an executor with the node when the
+   * Executor::add_node method is used.
+   * If false, this callback group will not be added automatically and would
+   * have to be added to an executor manually using the
+   * Executor::add_callback_group method.
+   *
+   * Whether the node was added to the executor before creating the callback
+   * group, or after, is irrelevant; the callback group will be automatically
+   * added to the executor in either case.
+   *
+   * \param[in] group_type They type of the callback group.
+   * \param[in] automatically_add_to_executor_with_node a boolean which
+   *   determines whether a callback group is automatically added to an executor
+   *   with the node with which it is associated.
+   */
   RCLCPP_PUBLIC
-  explicit CallbackGroup(CallbackGroupType group_type);
+  explicit CallbackGroup(
+    CallbackGroupType group_type,
+    bool automatically_add_to_executor_with_node = true);
 
   template<typename Function>
   rclcpp::SubscriptionBase::SharedPtr
@@ -102,6 +138,31 @@ class CallbackGroup
   const CallbackGroupType &
   type() const;
 
+  /// Return a reference to the 'associated with executor' atomic boolean.
+  /**
+   * When a callback group is added to an executor this boolean is checked
+   * to ensure it has not already been added to another executor.
+   * If it has not been, then this boolean is set to true to indicate it is
+   * now associated with an executor.
+   *
+   * When the callback group is removed from the executor, this atomic boolean
+   * is set back to false.
+   *
+   * \return the 'associated with executor' atomic boolean
+   */
+  RCLCPP_PUBLIC
+  std::atomic_bool &
+  get_associated_with_executor_atomic();
+
+  /// Return true if this callback group should be automatically added to an executor by the node.
+  /**
+   * \return boolean true if this callback group should be automatically added
+   *   to an executor when the associated node is added, otherwise false.
+   */
+  RCLCPP_PUBLIC
+  bool
+  automatically_add_to_executor_with_node() const;
+
 protected:
   RCLCPP_DISABLE_COPY(CallbackGroup)
 
@@ -136,12 +197,14 @@ class CallbackGroup
   CallbackGroupType type_;
   // Mutex to protect the subsequent vectors of pointers.
   mutable std::mutex mutex_;
+  std::atomic_bool associated_with_executor_;
   std::vector<rclcpp::SubscriptionBase::WeakPtr> subscription_ptrs_;
   std::vector<rclcpp::TimerBase::WeakPtr> timer_ptrs_;
   std::vector<rclcpp::ServiceBase::WeakPtr> service_ptrs_;
   std::vector<rclcpp::ClientBase::WeakPtr> client_ptrs_;
   std::vector<rclcpp::Waitable::WeakPtr> waitable_ptrs_;
   std::atomic_bool can_be_taken_from_;
+  const bool automatically_add_to_executor_with_node_;
 
 private:
   template<typename TypeT, typename Function>

diff --git a/rclcpp/include/rclcpp/executor.hpp b/rclcpp/include/rclcpp/executor.hpp
@@ -21,6 +21,7 @@
 #include <cstdlib>
 #include <iostream>
 #include <list>
+#include <map>
 #include <memory>
 #include <mutex>
 #include <string>
@@ -42,6 +43,10 @@
 namespace rclcpp
 {
 
+typedef std::map<rclcpp::CallbackGroup::WeakPtr,
+    rclcpp::node_interfaces::NodeBaseInterface::WeakPtr,
+    std::owner_less<rclcpp::CallbackGroup::WeakPtr>> WeakCallbackGroupsToNodesMap;
+
 // Forward declaration is used in convenience method signature.
 class Node;
 
@@ -76,13 +81,118 @@ class Executor
   virtual void
   spin() = 0;
 
+  /// Add a callback group to an executor.
+  /**
+   * An executor can have zero or more callback groups which provide work during `spin` functions.
+   * When an executor attempts to add a callback group, the executor checks to see if it is already
+   * associated with another executor, and if it has been, then an exception is thrown.
+   * Otherwise, the callback group is added to the executor.
+   *
+   * Adding a callback group with this method does not associate its node with this executor
+   * in any way
+   *
+   * \param[in] group_ptr a shared ptr that points to a callback group
+   * \param[in] node_ptr a shared pointer that points to a node base interface
+   * \param[in] notify True to trigger the interrupt guard condition during this function. If
+   * the executor is blocked at the rmw layer while waiting for work and it is notified that a new
+   * callback group was added, it will wake up.
+   * \throw std::runtime_error if the callback group is associated to an executor
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  add_callback_group(
+    rclcpp::CallbackGroup::SharedPtr group_ptr,
+    rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr,
+    bool notify = true);
+
+  /// Get callback groups that belong to executor.
+  /**
+   * This function returns a vector of weak pointers that point to callback groups that were
+   * associated with the executor.
+   * The callback groups associated with this executor may have been added with
+   * `add_callback_group`, or added when a node was added to the executor with `add_node`, or
+   * automatically added when it created by a node already associated with this executor and the
+   * automatically_add_to_executor_with_node parameter was true.
+   *
+   * \return a vector of weak pointers that point to callback groups that are associated with
+   * the executor
+   */
+  RCLCPP_PUBLIC
+  virtual std::vector<rclcpp::CallbackGroup::WeakPtr>
+  get_all_callback_groups();
+
+  /// Get callback groups that belong to executor.
+  /**
+   * This function returns a vector of weak pointers that point to callback groups that were
+   * associated with the executor.
+   * The callback groups associated with this executor have been added with
+   * `add_callback_group`.
+   *
+   * \return a vector of weak pointers that point to callback groups that are associated with
+   * the executor
+   */
+  RCLCPP_PUBLIC
+  virtual std::vector<rclcpp::CallbackGroup::WeakPtr>
+  get_manually_added_callback_groups();
+
+  /// Get callback groups that belong to executor.
+  /**
+   * This function returns a vector of weak pointers that point to callback groups that were
+   * added from a node that is associated with the executor.
+   * The callback groups are added when a node is added to the executor with `add_node`, or
+   * automatically if they are created in the future by that node and have the
+   * automatically_add_to_executor_with_node argument set to true.
+   *
+   * \return a vector of weak pointers that point to callback groups from a node associated with
+   * the executor
+   */
+  RCLCPP_PUBLIC
+  virtual std::vector<rclcpp::CallbackGroup::WeakPtr>
+  get_automatically_added_callback_groups_from_nodes();
+
+  /// Remove a callback group from the executor.
+  /**
+   * The callback group is removed from and disassociated with the executor.
+   * If the callback group removed was the last callback group from the node
+   * that is associated with the executor, the interrupt guard condition
+   * is triggered and node's guard condition is removed from the executor.
+   *
+   * This function only removes a callback group that was manually added with
+   * rclcpp::Executor::add_callback_group.
+   * To remove callback groups that were added from a node using
+   * rclcpp::Executor::add_node, use rclcpp::Executor::remove_node instead.
+   *
+   * \param[in] group_ptr Shared pointer to the callback group to be added.
+   * \param[in] notify True to trigger the interrupt guard condition during this function. If
+   * the executor is blocked at the rmw layer while waiting for work and it is notified that a
+   * callback group was removed, it will wake up.
+   * \throw std::runtime_error if node is deleted before callback group
+   * \throw std::runtime_error if the callback group is not associated with the executor
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  remove_callback_group(
+    rclcpp::CallbackGroup::SharedPtr group_ptr,
+    bool notify = true);
+
   /// Add a node to the executor.
   /**
-   * An executor can have zero or more nodes which provide work during `spin` functions.
+   * Nodes have associated callback groups, and this method adds any of those callback groups
+   * to this executor which have their automatically_add_to_executor_with_node parameter true.
+   * The node is also associated with the executor so that future callback groups which are
+   * created on the node with the automatically_add_to_executor_with_node parameter set to true
+   * are also automatically associated with this executor.
+   *
+   * Callback groups with the automatically_add_to_executor_with_node parameter set to false must
+   * be manually added to an executor using the rclcpp::Executor::add_callback_group method.
+   *
+   * If a node is already associated with an executor, this method throws an exception.
+   *
    * \param[in] node_ptr Shared pointer to the node to be added.
    * \param[in] notify True to trigger the interrupt guard condition during this function. If
    * the executor is blocked at the rmw layer while waiting for work and it is notified that a new
    * node was added, it will wake up.
+   * \throw std::runtime_error if a node is already associated to an executor
    */
   RCLCPP_PUBLIC
   virtual void
@@ -98,10 +208,19 @@ class Executor
 
   /// Remove a node from the executor.
   /**
+   * Any callback groups automatically added when this node was added with
+   * rclcpp::Executor::add_node are automatically removed, and the node is no longer associated
+   * with this executor.
+   *
+   * This also means that future callback groups created by the given node are no longer
+   * automatically added to this executor.
+   *
    * \param[in] node_ptr Shared pointer to the node to remove.
    * \param[in] notify True to trigger the interrupt guard condition and wake up the executor.
    * This is useful if the last node was removed from the executor while the executor was blocked
    * waiting for work in another thread, because otherwise the executor would never be notified.
+   * \throw std::runtime_error if the node is not associated with an executor.
+   * \throw std::runtime_error if the node is not associated with this executor.
    */
   RCLCPP_PUBLIC
   virtual void
@@ -327,22 +446,79 @@ class Executor
 
   RCLCPP_PUBLIC
   rclcpp::node_interfaces::NodeBaseInterface::SharedPtr
-  get_node_by_group(rclcpp::CallbackGroup::SharedPtr group);
+  get_node_by_group(
+    WeakCallbackGroupsToNodesMap weak_groups_to_nodes,
+    rclcpp::CallbackGroup::SharedPtr group);
+
+  /// Return true if the node has been added to this executor.
+  /**
+   * \param[in] node_ptr a shared pointer that points to a node base interface
+   * \return true if the node is associated with the executor, otherwise false
+   */
+  RCLCPP_PUBLIC
+  bool
+  has_node(
+    const rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr,
+    WeakCallbackGroupsToNodesMap weak_groups_to_nodes) const;
 
   RCLCPP_PUBLIC
   rclcpp::CallbackGroup::SharedPtr
   get_group_by_timer(rclcpp::TimerBase::SharedPtr timer);
 
+  /// Add a callback group to an executor
+  /**
+   * \see rclcpp::Executor::add_callback_group
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  add_callback_group_to_map(
+    rclcpp::CallbackGroup::SharedPtr group_ptr,
+    rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr,
+    WeakCallbackGroupsToNodesMap & weak_groups_to_nodes,
+    bool notify = true);
+
+  /// Remove a callback group from the executor.
+  /**
+   * \see rclcpp::Executor::remove_callback_group
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  remove_callback_group_from_map(
+    rclcpp::CallbackGroup::SharedPtr group_ptr,
+    WeakCallbackGroupsToNodesMap & weak_groups_to_nodes,
+    bool notify = true);
+
   RCLCPP_PUBLIC
   bool
   get_next_ready_executable(AnyExecutable & any_executable);
 
+  RCLCPP_PUBLIC
+  bool
+  get_next_ready_executable_from_map(
+    AnyExecutable & any_executable,
+    WeakCallbackGroupsToNodesMap weak_groups_to_nodes);
+
   RCLCPP_PUBLIC
   bool
   get_next_executable(
     AnyExecutable & any_executable,
     std::chrono::nanoseconds timeout = std::chrono::nanoseconds(-1));
 
+  /// Add all callback groups that can be automatically added from associated nodes.
+  /**
+   * The executor, before collecting entities, verifies if any callback group from
+   * nodes associated with the executor, which is not already associated to an executor,
+   * can be automatically added to this executor.
+   * This takes care of any callback group that has been added to a node but not explicitly added
+   * to the executor.
+   * It is important to note that in order for the callback groups to be automatically added to an
+   * executor through this function, the node of the callback groups needs to have been added
+   * through the `add_node` method.
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  add_callback_groups_from_nodes_associated_to_executor();
+
   /// Spinning state, used to prevent multi threaded calls to spin and to cancel blocking spins.
   std::atomic_bool spinning;
 
@@ -367,8 +543,25 @@ class Executor
   void
   spin_once_impl(std::chrono::nanoseconds timeout);
 
+  typedef std::map<rclcpp::node_interfaces::NodeBaseInterface::WeakPtr,
+      const rcl_guard_condition_t *,
+      std::owner_less<rclcpp::node_interfaces::NodeBaseInterface::WeakPtr>>
+    WeakNodesToGuardConditionsMap;
+
+  /// maps nodes to guard conditions
+  WeakNodesToGuardConditionsMap weak_nodes_to_guard_conditions_;
+
+  /// maps callback groups associated to nodes
+  WeakCallbackGroupsToNodesMap weak_groups_associated_with_executor_to_nodes_;
+
+  /// maps callback groups to nodes associated with executor
+  WeakCallbackGroupsToNodesMap weak_groups_to_nodes_associated_with_executor_;
+
+  /// maps all callback groups to nodes
+  WeakCallbackGroupsToNodesMap weak_groups_to_nodes_;
+
+  /// nodes that are associated with the executor
   std::list<rclcpp::node_interfaces::NodeBaseInterface::WeakPtr> weak_nodes_;
-  std::list<const rcl_guard_condition_t *> guard_conditions_;
 };
 
 namespace executor