diff --git a/rclcpp/include/rclcpp/create_timer.hpp b/rclcpp/include/rclcpp/create_timer.hpp index 5225f0fde0..345f43fcbb 100644 --- a/rclcpp/include/rclcpp/create_timer.hpp +++ b/rclcpp/include/rclcpp/create_timer.hpp @@ -23,63 +23,12 @@ #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 -std::chrono::nanoseconds -safe_cast_to_period_in_ns(std::chrono::duration period) -{ - if (period < std::chrono::duration::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(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 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>( - 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(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 @@ -92,13 +41,14 @@ create_timer( CallbackT && callback, rclcpp::CallbackGroup::SharedPtr group = nullptr) { - return create_timer( - node_base.get(), - node_timers.get(), + auto timer = rclcpp::GenericTimer::make_shared( clock, period.to_chrono(), std::forward(callback), - group); + node_base->get_context()); + + node_timers->add_timer(timer, group); + return timer; } /// Create a timer with a given clock @@ -112,99 +62,82 @@ 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.to_chrono(), + period, std::forward(callback), - group, - rclcpp::node_interfaces::get_node_base_interface(node).get(), - rclcpp::node_interfaces::get_node_timers_interface(node).get()); + group); } -/// Convenience method to create a general timer with node resources. +/// Convenience method to create a 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 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 + * \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 */ template -typename rclcpp::GenericTimer::SharedPtr -create_timer( - rclcpp::Clock::SharedPtr clock, +typename rclcpp::WallTimer::SharedPtr +create_wall_timer( std::chrono::duration 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"}; } - const std::chrono::nanoseconds period_ns = detail::safe_cast_to_period_in_ns(period); + if (period < std::chrono::duration::zero()) { + throw std::invalid_argument{"timer period cannot be negative"}; + } - // Add a new generic timer. - auto timer = rclcpp::GenericTimer::make_shared( - std::move(clock), period_ns, std::move(callback), node_base->get_context()); - node_timers->add_timer(timer, group); - return timer; -} + // 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(1); -/// 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 rclcpp::WallTimer::SharedPtr -create_wall_timer( - std::chrono::duration 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"}; + // 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 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>( + maximum_safe_cast_ns); + if (period > ns_max_as_double) { + throw std::invalid_argument{ + "timer period must be less than std::chrono::nanoseconds::max()"}; } - if (node_timers == nullptr) { - throw std::invalid_argument{"input node_timers cannot be null"}; + const auto period_ns = std::chrono::duration_cast(period); + if (period_ns < std::chrono::nanoseconds::zero()) { + throw std::runtime_error{ + "Casting timer period to nanoseconds resulted in integer overflow."}; } - const std::chrono::nanoseconds period_ns = detail::safe_cast_to_period_in_ns(period); - - // Add a new wall timer. auto timer = rclcpp::WallTimer::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_ diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index e514137b51..35162de03b 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -227,7 +227,7 @@ class Node : public std::enable_shared_from_this ) ); - /// Create a wall timer that uses the wall clock to drive the callback. + /// Create a timer. /** * \param[in] period Time interval between triggers of the callback. * \param[in] callback User-defined callback function. @@ -240,19 +240,6 @@ class Node : public std::enable_shared_from_this 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 rclcpp::GenericTimer::SharedPtr - create_timer( - std::chrono::duration period, - CallbackT callback, - rclcpp::CallbackGroup::SharedPtr group = nullptr); - /// Create and return a Client. /** * \param[in] service_name The topic to service on. diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp index d6b090d4d1..5b427b5b25 100644 --- a/rclcpp/include/rclcpp/node_impl.hpp +++ b/rclcpp/include/rclcpp/node_impl.hpp @@ -120,22 +120,6 @@ Node::create_wall_timer( this->node_timers_.get()); } -template -typename rclcpp::GenericTimer::SharedPtr -Node::create_timer( - std::chrono::duration 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 Client::SharedPtr Node::create_client( diff --git a/rclcpp/test/rclcpp/test_create_timer.cpp b/rclcpp/test/rclcpp/test_create_timer.cpp index f253af4838..13c3564544 100644 --- a/rclcpp/test/rclcpp/test_create_timer.cpp +++ b/rclcpp/test/rclcpp/test_create_timer.cpp @@ -62,7 +62,7 @@ TEST(TestCreateTimer, call_with_node_wrapper_compiles) rclcpp::shutdown(); } -TEST(TestCreateWallTimer, call_wall_timer_with_bad_arguments) +TEST(TestCreateTimer, call_wall_timer_with_bad_arguments) { rclcpp::init(0, nullptr); NodeWrapper node("test_create_wall_timers_with_bad_arguments"); @@ -117,66 +117,6 @@ TEST(TestCreateWallTimer, 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) diff --git a/rclcpp/test/rclcpp/test_time_source.cpp b/rclcpp/test/rclcpp/test_time_source.cpp index a5aad9a498..2bf89a5b09 100644 --- a/rclcpp/test/rclcpp/test_time_source.cpp +++ b/rclcpp/test/rclcpp/test_time_source.cpp @@ -81,7 +81,7 @@ void spin_until_time( executor.spin_once(10ms); - if (clock->now().nanoseconds() >= end_time.count()) { + if (clock->now().nanoseconds() == end_time.count()) { return; } } @@ -108,7 +108,7 @@ void spin_until_ros_time_updated( executor.add_node(node); auto start = std::chrono::system_clock::now(); - while (std::chrono::system_clock::now() < (start + 2s)) { + while (std::chrono::system_clock::now() < (start + 1s)) { if (!rclcpp::ok()) { break; // Break for ctrl-c } @@ -630,8 +630,7 @@ class SimClockPublisherNode : public rclcpp::Node { public: SimClockPublisherNode() - : rclcpp::Node("sim_clock_publisher_node"), - pub_time_(0, 0) + : rclcpp::Node("sim_clock_publisher_node") { // Create a clock publisher clock_pub_ = this->create_publisher( @@ -646,6 +645,10 @@ class SimClockPublisherNode : public rclcpp::Node &SimClockPublisherNode::timer_callback, this) ); + + // Init clock msg to zero + clock_msg_.clock.sec = 0; + clock_msg_.clock.nanosec = 0; } ~SimClockPublisherNode() @@ -668,15 +671,13 @@ class SimClockPublisherNode : public rclcpp::Node private: void timer_callback() { - // Increment the time, update the clock msg and publish it - pub_time_ += rclcpp::Duration(0, 1000000); - clock_msg_.clock = pub_time_; + // Increment clock msg and publish it + clock_msg_.clock.nanosec += 1000000; clock_pub_->publish(clock_msg_); } rclcpp::Publisher::SharedPtr clock_pub_; rclcpp::TimerBase::SharedPtr pub_timer_; - rclcpp::Time pub_time_; rosgraph_msgs::msg::Clock clock_msg_; std::thread node_thread_; rclcpp::executors::SingleThreadedExecutor node_executor; @@ -694,7 +695,7 @@ class ClockThreadTestingNode : public rclcpp::Node this->set_parameter(rclcpp::Parameter("use_sim_time", true)); // Create a 100ms timer - timer_ = this->create_timer( + timer_ = this->create_wall_timer( std::chrono::milliseconds(100), std::bind( &ClockThreadTestingNode::timer_callback, @@ -734,33 +735,29 @@ class ClockThreadTestingNode : public rclcpp::Node bool is_callback_frozen_ = true; }; -// TODO(ivanpauno): This test was using a wall timer, when it was supposed to use sim time. -// It was also using `use_clock_tread = false`, when it was supposed to be `true`. -// Fixing the test to work as originally intended makes it super flaky. -// Disabling it until the test is fixed. -// TEST_F(TestTimeSource, check_sim_time_updated_in_callback_if_use_clock_thread) { -// // Test if clock time of a node with -// // parameter use_sim_time = true and option use_clock_thread = true -// // is updated while node is not spinning -// // (in a timer callback) - -// // Create a "sim time" publisher and spin it -// SimClockPublisherNode pub_node; -// pub_node.SpinNode(); - -// // Spin node for 2 seconds -// ClockThreadTestingNode clock_thread_testing_node; -// auto steady_clock = rclcpp::Clock(RCL_STEADY_TIME); -// auto start_time = steady_clock.now(); -// while (rclcpp::ok() && -// (steady_clock.now() - start_time).seconds() < 2.0) -// { -// rclcpp::spin_some(clock_thread_testing_node.get_node_base_interface()); -// } - -// // Node should have get out of timer callback -// ASSERT_FALSE(clock_thread_testing_node.GetIsCallbackFrozen()); -// } +TEST_F(TestTimeSource, check_sim_time_updated_in_callback_if_use_clock_thread) { + // Test if clock time of a node with + // parameter use_sim_time = true and option use_clock_thread = true + // is updated while node is not spinning + // (in a timer callback) + + // Create a "sim time" publisher and spin it + SimClockPublisherNode pub_node; + pub_node.SpinNode(); + + // Spin node for 2 seconds + ClockThreadTestingNode clock_thread_testing_node; + auto steady_clock = rclcpp::Clock(RCL_STEADY_TIME); + auto start_time = steady_clock.now(); + while (rclcpp::ok() && + (steady_clock.now() - start_time).seconds() < 2.0) + { + rclcpp::spin_some(clock_thread_testing_node.get_node_base_interface()); + } + + // Node should have get out of timer callback + ASSERT_FALSE(clock_thread_testing_node.GetIsCallbackFrozen()); +} TEST_F(TestTimeSource, clock_sleep_until_with_ros_time_basic) { SimClockPublisherNode pub_node; diff --git a/rclcpp/test/rclcpp/test_timer.cpp b/rclcpp/test/rclcpp/test_timer.cpp index 59a1218519..7a6599dfe4 100644 --- a/rclcpp/test/rclcpp/test_timer.cpp +++ b/rclcpp/test/rclcpp/test_timer.cpp @@ -30,15 +30,8 @@ using namespace std::chrono_literals; -/// We want to test everything for both the wall and generic timer. -enum class TimerType -{ - WALL_TIMER, - GENERIC_TIMER, -}; - /// Timer testing bring up and teardown -class TestTimer : public ::testing::TestWithParam +class TestTimer : public ::testing::Test { protected: void SetUp() override @@ -51,7 +44,10 @@ class TestTimer : public ::testing::TestWithParam test_node = std::make_shared("test_timer_node"); - auto timer_callback = [this]() -> void { + timer = test_node->create_wall_timer( + 100ms, + [this]() -> void + { this->has_timer_run.store(true); if (this->cancel_timer.load()) { @@ -59,20 +55,10 @@ class TestTimer : public ::testing::TestWithParam } // prevent any tests running timer from blocking this->executor->cancel(); - }; - - // Store the timer type for use in TEST_P declarations. - timer_type = GetParam(); - switch (timer_type) { - case TimerType::WALL_TIMER: - timer = test_node->create_wall_timer(100ms, timer_callback); - EXPECT_TRUE(timer->is_steady()); - break; - case TimerType::GENERIC_TIMER: - timer = test_node->create_timer(100ms, timer_callback); - EXPECT_FALSE(timer->is_steady()); - break; - } + } + ); + EXPECT_TRUE(timer->is_steady()); + executor->add_node(test_node); // don't start spinning, let the test dictate when } @@ -86,7 +72,6 @@ class TestTimer : public ::testing::TestWithParam } // set to true if the timer callback executed, false otherwise - TimerType timer_type; std::atomic has_timer_run; // flag used to cancel the timer in the timer callback. If true cancel the timer, otherwise // cancel the executor (preventing any tests from blocking) @@ -106,7 +91,7 @@ void test_initial_conditions( } /// Simple test -TEST_P(TestTimer, test_simple_cancel) +TEST_F(TestTimer, test_simple_cancel) { // expect clean state, don't run otherwise test_initial_conditions(timer, has_timer_run); @@ -119,7 +104,7 @@ TEST_P(TestTimer, test_simple_cancel) } /// Test state when using reset -TEST_P(TestTimer, test_is_canceled_reset) +TEST_F(TestTimer, test_is_canceled_reset) { // expect clean state, don't run otherwise test_initial_conditions(timer, has_timer_run); @@ -144,7 +129,7 @@ TEST_P(TestTimer, test_is_canceled_reset) } /// Run and check state, cancel the executor -TEST_P(TestTimer, test_run_cancel_executor) +TEST_F(TestTimer, test_run_cancel_executor) { // expect clean state, don't run otherwise test_initial_conditions(timer, has_timer_run); @@ -161,7 +146,7 @@ TEST_P(TestTimer, test_run_cancel_executor) } /// Run and check state, cancel the timer -TEST_P(TestTimer, test_run_cancel_timer) +TEST_F(TestTimer, test_run_cancel_timer) { // expect clean state, don't run otherwise test_initial_conditions(timer, has_timer_run); @@ -174,7 +159,7 @@ TEST_P(TestTimer, test_run_cancel_timer) EXPECT_TRUE(timer->is_canceled()); } -TEST_P(TestTimer, test_bad_arguments) { +TEST_F(TestTimer, test_bad_arguments) { auto node_base = rclcpp::node_interfaces::get_node_base_interface(test_node); auto context = node_base->get_context(); @@ -213,19 +198,13 @@ TEST_P(TestTimer, test_bad_arguments) { rclcpp::exceptions::RCLError); } -TEST_P(TestTimer, callback_with_timer) { +TEST_F(TestTimer, callback_with_timer) { rclcpp::TimerBase * timer_ptr = nullptr; - auto timer_callback = [&timer_ptr](rclcpp::TimerBase & timer) { + timer = test_node->create_wall_timer( + std::chrono::milliseconds(1), + [&timer_ptr](rclcpp::TimerBase & timer) { timer_ptr = &timer; - }; - switch (timer_type) { - case TimerType::WALL_TIMER: - timer = test_node->create_wall_timer(1ms, timer_callback); - break; - case TimerType::GENERIC_TIMER: - timer = test_node->create_timer(1ms, timer_callback); - break; - } + }); auto start = std::chrono::steady_clock::now(); while (nullptr == timer_ptr && (std::chrono::steady_clock::now() - start) < std::chrono::milliseconds(100)) @@ -237,19 +216,13 @@ TEST_P(TestTimer, callback_with_timer) { EXPECT_FALSE(timer_ptr->is_ready()); } -TEST_P(TestTimer, callback_with_period_zero) { +TEST_F(TestTimer, callback_with_period_zero) { rclcpp::TimerBase * timer_ptr = nullptr; - auto timer_callback = [&timer_ptr](rclcpp::TimerBase & timer) { + timer = test_node->create_wall_timer( + std::chrono::milliseconds(0), + [&timer_ptr](rclcpp::TimerBase & timer) { timer_ptr = &timer; - }; - switch (timer_type) { - case TimerType::WALL_TIMER: - timer = test_node->create_wall_timer(0ms, timer_callback); - break; - case TimerType::GENERIC_TIMER: - timer = test_node->create_timer(0ms, timer_callback); - break; - } + }); auto start = std::chrono::steady_clock::now(); while (nullptr == timer_ptr && (std::chrono::steady_clock::now() - start) < std::chrono::milliseconds(100)) @@ -262,7 +235,7 @@ TEST_P(TestTimer, callback_with_period_zero) { } /// Test internal failures using mocks -TEST_P(TestTimer, test_failures_with_exceptions) +TEST_F(TestTimer, test_failures_with_exceptions) { // expect clean state, don't run otherwise test_initial_conditions(timer, has_timer_run); @@ -272,16 +245,8 @@ TEST_P(TestTimer, test_failures_with_exceptions) auto mock = mocking_utils::inject_on_return("lib:rclcpp", rcl_timer_fini, RCL_RET_ERROR); EXPECT_NO_THROW( { - switch (timer_type) { - case TimerType::WALL_TIMER: - timer_to_test_destructor = - test_node->create_wall_timer(std::chrono::milliseconds(0), [](void) {}); - break; - case TimerType::GENERIC_TIMER: - timer_to_test_destructor = - test_node->create_timer(std::chrono::milliseconds(0), [](void) {}); - break; - } + timer_to_test_destructor = + test_node->create_wall_timer(std::chrono::milliseconds(0), [](void) {}); timer_to_test_destructor.reset(); }); } @@ -318,19 +283,3 @@ TEST_P(TestTimer, test_failures_with_exceptions) std::runtime_error("Timer could not get time until next call: error not set")); } } - -INSTANTIATE_TEST_SUITE_P( - PerTimerType, TestTimer, - ::testing::Values(TimerType::WALL_TIMER, TimerType::GENERIC_TIMER), - [](const ::testing::TestParamInfo & info) -> std::string { - switch (info.param) { - case TimerType::WALL_TIMER: - return std::string("wall_timer"); - case TimerType::GENERIC_TIMER: - return std::string("generic_timer"); - default: - break; - } - return std::string("unknown"); - } -); diff --git a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp index 5e06876018..84c62b9e6a 100644 --- a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp +++ b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp @@ -242,7 +242,7 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface, ) ); - /// Create a timer that uses the wall clock to drive the callback. + /// Create a timer. /** * \param[in] period Time interval between triggers of the callback. * \param[in] callback User-defined callback function. @@ -255,19 +255,6 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface, 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 rclcpp::GenericTimer::SharedPtr - create_timer( - std::chrono::duration period, - CallbackT callback, - rclcpp::CallbackGroup::SharedPtr group = nullptr); - /// Create and return a Client. /** * \sa rclcpp::Node::create_client diff --git a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp index e2a5696b92..88d981e051 100644 --- a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp +++ b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp @@ -90,28 +90,11 @@ LifecycleNode::create_wall_timer( CallbackT callback, rclcpp::CallbackGroup::SharedPtr group) { - return rclcpp::create_wall_timer( - period, - std::move(callback), - group, - this->node_base_.get(), - this->node_timers_.get()); -} - -template -typename rclcpp::GenericTimer::SharedPtr -LifecycleNode::create_timer( - std::chrono::duration 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()); + auto timer = rclcpp::WallTimer::make_shared( + std::chrono::duration_cast(period), + std::move(callback), this->node_base_->get_context()); + node_timers_->add_timer(timer, group); + return timer; } template diff --git a/rclcpp_lifecycle/test/test_lifecycle_publisher.cpp b/rclcpp_lifecycle/test/test_lifecycle_publisher.cpp index d10544ef1f..2a5d08f728 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_publisher.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_publisher.cpp @@ -42,17 +42,10 @@ class TestDefaultStateMachine : public ::testing::Test } }; -/// We want to test everything for both the wall and generic timer. -enum class TimerType -{ - WALL_TIMER, - GENERIC_TIMER, -}; - class EmptyLifecycleNode : public rclcpp_lifecycle::LifecycleNode { public: - explicit EmptyLifecycleNode(const std::string & node_name, const TimerType & timer_type) + explicit EmptyLifecycleNode(const std::string & node_name) : rclcpp_lifecycle::LifecycleNode(node_name) { rclcpp::PublisherOptionsWithAllocator> options; @@ -62,20 +55,8 @@ class EmptyLifecycleNode : public rclcpp_lifecycle::LifecycleNode add_managed_entity(publisher_); // For coverage this is being added here - switch (timer_type) { - case TimerType::WALL_TIMER: - { - auto timer = create_wall_timer(std::chrono::seconds(1), []() {}); - add_timer_handle(timer); - break; - } - case TimerType::GENERIC_TIMER: - { - auto timer = create_timer(std::chrono::seconds(1), []() {}); - add_timer_handle(timer); - break; - } - } + auto timer = create_wall_timer(std::chrono::seconds(1), []() {}); + add_timer_handle(timer); } std::shared_ptr> publisher() @@ -87,13 +68,13 @@ class EmptyLifecycleNode : public rclcpp_lifecycle::LifecycleNode std::shared_ptr> publisher_; }; -class TestLifecyclePublisher : public ::testing::TestWithParam +class TestLifecyclePublisher : public ::testing::Test { public: void SetUp() { rclcpp::init(0, nullptr); - node_ = std::make_shared("node", GetParam()); + node_ = std::make_shared("node"); } void TearDown() @@ -105,7 +86,7 @@ class TestLifecyclePublisher : public ::testing::TestWithParam std::shared_ptr node_; }; -TEST_P(TestLifecyclePublisher, publish_managed_by_node) { +TEST_F(TestLifecyclePublisher, publish_managed_by_node) { // transition via LifecycleNode auto success = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS; auto reset_key = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::ERROR; @@ -144,7 +125,7 @@ TEST_P(TestLifecyclePublisher, publish_managed_by_node) { } } -TEST_P(TestLifecyclePublisher, publish) { +TEST_F(TestLifecyclePublisher, publish) { // transition via LifecyclePublisher node_->publisher()->on_deactivate(); EXPECT_FALSE(node_->publisher()->is_activated()); @@ -167,19 +148,3 @@ TEST_P(TestLifecyclePublisher, publish) { EXPECT_NO_THROW(node_->publisher()->publish(std::move(msg_ptr))); } } - -INSTANTIATE_TEST_SUITE_P( - PerTimerType, TestLifecyclePublisher, - ::testing::Values(TimerType::WALL_TIMER, TimerType::GENERIC_TIMER), - [](const ::testing::TestParamInfo & info) -> std::string { - switch (info.param) { - case TimerType::WALL_TIMER: - return std::string("wall_timer"); - case TimerType::GENERIC_TIMER: - return std::string("generic_timer"); - default: - break; - } - return std::string("unknown"); - } -);