Revert "Revert "Add a create_timer method to Node and LifecycleNode classes (#1975)" (#2009)

rclcpp/include/rclcpp/create_timer.hpp

@@ -23,12 +23,63 @@
 
 #include "rclcpp/duration.hpp"
 #include "rclcpp/node_interfaces/get_node_base_interface.hpp"
+#include "rclcpp/node_interfaces/get_node_clock_interface.hpp"
 #include "rclcpp/node_interfaces/get_node_timers_interface.hpp"
 #include "rclcpp/node_interfaces/node_base_interface.hpp"
+#include "rclcpp/node_interfaces/node_clock_interface.hpp"
 #include "rclcpp/node_interfaces/node_timers_interface.hpp"
 
 namespace rclcpp
 {
+namespace detail
+{
+/// Perform a safe cast to a timer period in nanoseconds
+/**
+ *
+ * \tparam DurationRepT
+ * \tparam DurationT
+ * \param period period to execute callback. This duration must be 0 <= period < nanoseconds::max()
+ * \return period, expressed as chrono::duration::nanoseconds
+ * \throws std::invalid_argument if period is negative or too large
+ */
+template<typename DurationRepT, typename DurationT>
+std::chrono::nanoseconds
+safe_cast_to_period_in_ns(std::chrono::duration<DurationRepT, DurationT> period)
+{
+  if (period < std::chrono::duration<DurationRepT, DurationT>::zero()) {
+    throw std::invalid_argument{"timer period cannot be negative"};
+  }
+
+  // Casting to a double representation might lose precision and allow the check below to succeed
+  // but the actual cast to nanoseconds fail. Using 1 DurationT worth of nanoseconds less than max.
+  constexpr auto maximum_safe_cast_ns =
+    std::chrono::nanoseconds::max() - std::chrono::duration<DurationRepT, DurationT>(1);
+
+  // If period is greater than nanoseconds::max(), the duration_cast to nanoseconds will overflow
+  // a signed integer, which is undefined behavior. Checking whether any std::chrono::duration is
+  // greater than nanoseconds::max() is a difficult general problem. This is a more conservative
+  // version of Howard Hinnant's (the <chrono> guy>) response here:
+  // https://stackoverflow.com/a/44637334/2089061
+  // However, this doesn't solve the issue for all possible duration types of period.
+  // Follow-up issue: https://github.com/ros2/rclcpp/issues/1177
+  constexpr auto ns_max_as_double =
+    std::chrono::duration_cast<std::chrono::duration<double, std::chrono::nanoseconds::period>>(
+    maximum_safe_cast_ns);
+  if (period > ns_max_as_double) {
+    throw std::invalid_argument{
+            "timer period must be less than std::chrono::nanoseconds::max()"};
+  }
+
+  const auto period_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(period);
+  if (period_ns < std::chrono::nanoseconds::zero()) {
+    throw std::runtime_error{
+            "Casting timer period to nanoseconds resulted in integer overflow."};
+  }
+
+  return period_ns;
+}
+}  // namespace detail
+
 /// Create a timer with a given clock
 /// \internal
 template<typename CallbackT>
@@ -41,14 +92,13 @@ create_timer(
   CallbackT && callback,
   rclcpp::CallbackGroup::SharedPtr group = nullptr)
 {
-  auto timer = rclcpp::GenericTimer<CallbackT>::make_shared(
+  return create_timer(
     clock,
     period.to_chrono<std::chrono::nanoseconds>(),
     std::forward<CallbackT>(callback),
-    node_base->get_context());
-
-  node_timers->add_timer(timer, group);
-  return timer;
+    group,
+    node_base.get(),
+    node_timers.get());
 }
 
 /// Create a timer with a given clock
@@ -62,82 +112,99 @@ create_timer(
   rclcpp::CallbackGroup::SharedPtr group = nullptr)
 {
   return create_timer(
-    rclcpp::node_interfaces::get_node_base_interface(node),
-    rclcpp::node_interfaces::get_node_timers_interface(node),
     clock,
-    period,
+    period.to_chrono<std::chrono::nanoseconds>(),
     std::forward<CallbackT>(callback),
-    group);
+    group,
+    rclcpp::node_interfaces::get_node_base_interface(node).get(),
+    rclcpp::node_interfaces::get_node_timers_interface(node).get());
 }
 
-/// Convenience method to create a timer with node resources.
+/// Convenience method to create a general timer with node resources.
 /**
  *
  * \tparam DurationRepT
  * \tparam DurationT
  * \tparam CallbackT
+ * \param clock clock to be used
  * \param period period to execute callback. This duration must be 0 <= period < nanoseconds::max()
  * \param callback callback to execute via the timer period
- * \param group
- * \param node_base
- * \param node_timers
- * \return
- * \throws std::invalid argument if either node_base or node_timers
- * are null, or period is negative or too large
+ * \param group callback group
+ * \param node_base node base interface
+ * \param node_timers node timer interface
+ * \return shared pointer to a generic timer
+ * \throws std::invalid_argument if either clock, node_base or node_timers
+ * are nullptr, or period is negative or too large
  */
 template<typename DurationRepT, typename DurationT, typename CallbackT>
-typename rclcpp::WallTimer<CallbackT>::SharedPtr
-create_wall_timer(
+typename rclcpp::GenericTimer<CallbackT>::SharedPtr
+create_timer(
+  rclcpp::Clock::SharedPtr clock,
   std::chrono::duration<DurationRepT, DurationT> period,
   CallbackT callback,
   rclcpp::CallbackGroup::SharedPtr group,
   node_interfaces::NodeBaseInterface * node_base,
   node_interfaces::NodeTimersInterface * node_timers)
 {
+  if (clock == nullptr) {
+    throw std::invalid_argument{"clock cannot be null"};
+  }
   if (node_base == nullptr) {
     throw std::invalid_argument{"input node_base cannot be null"};
   }
-
   if (node_timers == nullptr) {
     throw std::invalid_argument{"input node_timers cannot be null"};
   }
 
-  if (period < std::chrono::duration<DurationRepT, DurationT>::zero()) {
-    throw std::invalid_argument{"timer period cannot be negative"};
-  }
+  const std::chrono::nanoseconds period_ns = detail::safe_cast_to_period_in_ns(period);
 
-  // Casting to a double representation might lose precision and allow the check below to succeed
-  // but the actual cast to nanoseconds fail. Using 1 DurationT worth of nanoseconds less than max.
-  constexpr auto maximum_safe_cast_ns =
-    std::chrono::nanoseconds::max() - std::chrono::duration<DurationRepT, DurationT>(1);
+  // Add a new generic timer.
+  auto timer = rclcpp::GenericTimer<CallbackT>::make_shared(
+    std::move(clock), period_ns, std::move(callback), node_base->get_context());
+  node_timers->add_timer(timer, group);
+  return timer;
+}
 
-  // If period is greater than nanoseconds::max(), the duration_cast to nanoseconds will overflow
-  // a signed integer, which is undefined behavior. Checking whether any std::chrono::duration is
-  // greater than nanoseconds::max() is a difficult general problem. This is a more conservative
-  // version of Howard Hinnant's (the <chrono> guy>) response here:
-  // https://stackoverflow.com/a/44637334/2089061
-  // However, this doesn't solve the issue for all possible duration types of period.
-  // Follow-up issue: https://github.com/ros2/rclcpp/issues/1177
-  constexpr auto ns_max_as_double =
-    std::chrono::duration_cast<std::chrono::duration<double, std::chrono::nanoseconds::period>>(
-    maximum_safe_cast_ns);
-  if (period > ns_max_as_double) {
-    throw std::invalid_argument{
-            "timer period must be less than std::chrono::nanoseconds::max()"};
+/// Convenience method to create a wall timer with node resources.
+/**
+ *
+ * \tparam DurationRepT
+ * \tparam DurationT
+ * \tparam CallbackT
+ * \param period period to execute callback. This duration must be 0 <= period < nanoseconds::max()
+ * \param callback callback to execute via the timer period
+ * \param group callback group
+ * \param node_base node base interface
+ * \param node_timers node timer interface
+ * \return shared pointer to a wall timer
+ * \throws std::invalid_argument if either node_base or node_timers
+ * are null, or period is negative or too large
+ */
+template<typename DurationRepT, typename DurationT, typename CallbackT>
+typename rclcpp::WallTimer<CallbackT>::SharedPtr
+create_wall_timer(
+  std::chrono::duration<DurationRepT, DurationT> period,
+  CallbackT callback,
+  rclcpp::CallbackGroup::SharedPtr group,
+  node_interfaces::NodeBaseInterface * node_base,
+  node_interfaces::NodeTimersInterface * node_timers)
+{
+  if (node_base == nullptr) {
+    throw std::invalid_argument{"input node_base cannot be null"};
   }
 
-  const auto period_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(period);
-  if (period_ns < std::chrono::nanoseconds::zero()) {
-    throw std::runtime_error{
-            "Casting timer period to nanoseconds resulted in integer overflow."};
+  if (node_timers == nullptr) {
+    throw std::invalid_argument{"input node_timers cannot be null"};
   }
 
+  const std::chrono::nanoseconds period_ns = detail::safe_cast_to_period_in_ns(period);
+
+  // Add a new wall timer.
   auto timer = rclcpp::WallTimer<CallbackT>::make_shared(
     period_ns, std::move(callback), node_base->get_context());
   node_timers->add_timer(timer, group);
   return timer;
 }
-
 }  // namespace rclcpp
 
 #endif  // RCLCPP__CREATE_TIMER_HPP_

rclcpp/include/rclcpp/node.hpp

@@ -227,7 +227,7 @@ class Node : public std::enable_shared_from_this<Node>
     )
   );
 
-  /// Create a timer.
+  /// Create a wall timer that uses the wall clock to drive the callback.
   /**
    * \param[in] period Time interval between triggers of the callback.
    * \param[in] callback User-defined callback function.
@@ -240,6 +240,19 @@ class Node : public std::enable_shared_from_this<Node>
     CallbackT callback,
     rclcpp::CallbackGroup::SharedPtr group = nullptr);
 
+  /// Create a timer that uses the node clock to drive the callback.
+  /**
+   * \param[in] period Time interval between triggers of the callback.
+   * \param[in] callback User-defined callback function.
+   * \param[in] group Callback group to execute this timer's callback in.
+   */
+  template<typename DurationRepT = int64_t, typename DurationT = std::milli, typename CallbackT>
+  typename rclcpp::GenericTimer<CallbackT>::SharedPtr
+  create_timer(
+    std::chrono::duration<DurationRepT, DurationT> period,
+    CallbackT callback,
+    rclcpp::CallbackGroup::SharedPtr group = nullptr);
+
   /// Create and return a Client.
   /**
    * \param[in] service_name The topic to service on.

rclcpp/include/rclcpp/node_impl.hpp

@@ -120,6 +120,22 @@ Node::create_wall_timer(
     this->node_timers_.get());
 }
 
+template<typename DurationRepT, typename DurationT, typename CallbackT>
+typename rclcpp::GenericTimer<CallbackT>::SharedPtr
+Node::create_timer(
+  std::chrono::duration<DurationRepT, DurationT> period,
+  CallbackT callback,
+  rclcpp::CallbackGroup::SharedPtr group)
+{
+  return rclcpp::create_timer(
+    this->get_clock(),
+    period,
+    std::move(callback),
+    group,
+    this->node_base_.get(),
+    this->node_timers_.get());
+}
+
 template<typename ServiceT>
 typename Client<ServiceT>::SharedPtr
 Node::create_client(

rclcpp/test/rclcpp/test_create_timer.cpp

@@ -62,7 +62,7 @@ TEST(TestCreateTimer, call_with_node_wrapper_compiles)
   rclcpp::shutdown();
 }
 
-TEST(TestCreateTimer, call_wall_timer_with_bad_arguments)
+TEST(TestCreateWallTimer, call_wall_timer_with_bad_arguments)
 {
   rclcpp::init(0, nullptr);
   NodeWrapper node("test_create_wall_timers_with_bad_arguments");
@@ -117,6 +117,66 @@ TEST(TestCreateTimer, call_wall_timer_with_bad_arguments)
   rclcpp::shutdown();
 }
 
+TEST(TestCreateTimer, call_timer_with_bad_arguments)
+{
+  rclcpp::init(0, nullptr);
+  NodeWrapper node("test_create_timers_with_bad_arguments");
+  auto callback = []() {};
+  rclcpp::CallbackGroup::SharedPtr group = nullptr;
+  auto node_interface =
+    rclcpp::node_interfaces::get_node_base_interface(node).get();
+  auto timers_interface =
+    rclcpp::node_interfaces::get_node_timers_interface(node).get();
+
+  auto clock = node.get_node_clock_interface()->get_clock();
+
+  // Negative period
+  EXPECT_THROW(
+    rclcpp::create_timer(
+      clock, -1ms, callback, group, node_interface, timers_interface),
+    std::invalid_argument);
+
+  // Very negative period
+  constexpr auto nanoseconds_min = std::chrono::nanoseconds::min();
+  EXPECT_THROW(
+    rclcpp::create_timer(
+      clock, nanoseconds_min, callback, group, node_interface, timers_interface),
+    std::invalid_argument);
+
+  // Period must be less than nanoseconds::max()
+  constexpr auto nanoseconds_max = std::chrono::nanoseconds::min();
+  EXPECT_THROW(
+    rclcpp::create_timer(
+      clock, nanoseconds_max, callback, group, node_interface, timers_interface),
+    std::invalid_argument);
+
+  EXPECT_NO_THROW(
+    rclcpp::create_timer(
+      clock, nanoseconds_max - 1us, callback, group, node_interface, timers_interface));
+
+  EXPECT_NO_THROW(
+    rclcpp::create_timer(clock, 0ms, callback, group, node_interface, timers_interface));
+
+  // Period must be less than nanoseconds::max()
+  constexpr auto hours_max = std::chrono::hours::max();
+  EXPECT_THROW(
+    rclcpp::create_timer(
+      clock, hours_max, callback, group, node_interface, timers_interface),
+    std::invalid_argument);
+
+  // node_interface is null
+  EXPECT_THROW(
+    rclcpp::create_timer(clock, 1ms, callback, group, nullptr, timers_interface),
+    std::invalid_argument);
+
+  // timers_interface is null
+  EXPECT_THROW(
+    rclcpp::create_timer(clock, 1ms, callback, group, node_interface, nullptr),
+    std::invalid_argument);
+
+  rclcpp::shutdown();
+}
+
 static void test_timer_callback(void) {}
 
 TEST(TestCreateTimer, timer_function_pointer)