From b574b1ac4e395feaf2d7ab33248ad50dde4d09ac Mon Sep 17 00:00:00 2001 From: Voldivh Date: Wed, 10 Aug 2022 18:06:48 -0300 Subject: [PATCH 1/4] Modifies timers API to select autostart state Signed-off-by: Voldivh --- rclcpp/include/rclcpp/create_timer.hpp | 17 ++++++++++------ rclcpp/include/rclcpp/node.hpp | 4 +++- rclcpp/include/rclcpp/node_impl.hpp | 6 ++++-- rclcpp/include/rclcpp/timer.hpp | 18 ++++++++++++----- rclcpp/src/rclcpp/timer.cpp | 9 +++++---- rclcpp/test/rclcpp/test_create_timer.cpp | 25 ++++++++++++++++++++++++ rclcpp/test/rclcpp/test_timer.cpp | 24 +++++++++++++++++++++++ 7 files changed, 85 insertions(+), 18 deletions(-) diff --git a/rclcpp/include/rclcpp/create_timer.hpp b/rclcpp/include/rclcpp/create_timer.hpp index 345f43fcbb..9bfe34ae01 100644 --- a/rclcpp/include/rclcpp/create_timer.hpp +++ b/rclcpp/include/rclcpp/create_timer.hpp @@ -39,13 +39,15 @@ create_timer( rclcpp::Clock::SharedPtr clock, rclcpp::Duration period, CallbackT && callback, - rclcpp::CallbackGroup::SharedPtr group = nullptr) + rclcpp::CallbackGroup::SharedPtr group = nullptr, + bool autostart = true) { auto timer = rclcpp::GenericTimer::make_shared( clock, period.to_chrono(), std::forward(callback), - node_base->get_context()); + node_base->get_context(), + autostart); node_timers->add_timer(timer, group); return timer; @@ -59,7 +61,8 @@ create_timer( rclcpp::Clock::SharedPtr clock, rclcpp::Duration period, CallbackT && callback, - rclcpp::CallbackGroup::SharedPtr group = nullptr) + rclcpp::CallbackGroup::SharedPtr group = nullptr, + bool autostart = true) { return create_timer( rclcpp::node_interfaces::get_node_base_interface(node), @@ -67,7 +70,8 @@ create_timer( clock, period, std::forward(callback), - group); + group, + autostart); } /// Convenience method to create a timer with node resources. @@ -92,7 +96,8 @@ create_wall_timer( CallbackT callback, rclcpp::CallbackGroup::SharedPtr group, node_interfaces::NodeBaseInterface * node_base, - node_interfaces::NodeTimersInterface * node_timers) + node_interfaces::NodeTimersInterface * node_timers, + bool autostart = true) { if (node_base == nullptr) { throw std::invalid_argument{"input node_base cannot be null"}; @@ -133,7 +138,7 @@ create_wall_timer( } auto timer = rclcpp::WallTimer::make_shared( - period_ns, std::move(callback), node_base->get_context()); + period_ns, std::move(callback), node_base->get_context(), autostart); node_timers->add_timer(timer, group); return timer; } diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index bd5d39f6bb..9e4b745f43 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -232,13 +232,15 @@ class Node : public std::enable_shared_from_this * \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. + * \param[in] autostart The state of the clock on initialization. */ template typename rclcpp::WallTimer::SharedPtr create_wall_timer( std::chrono::duration period, CallbackT callback, - rclcpp::CallbackGroup::SharedPtr group = nullptr); + rclcpp::CallbackGroup::SharedPtr group = nullptr, + bool autostart = true); /// Create and return a Client. /** diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp index 5b427b5b25..f3c8c96ab7 100644 --- a/rclcpp/include/rclcpp/node_impl.hpp +++ b/rclcpp/include/rclcpp/node_impl.hpp @@ -110,14 +110,16 @@ typename rclcpp::WallTimer::SharedPtr Node::create_wall_timer( std::chrono::duration period, CallbackT callback, - rclcpp::CallbackGroup::SharedPtr group) + rclcpp::CallbackGroup::SharedPtr group, + bool autostart) { return rclcpp::create_wall_timer( period, std::move(callback), group, this->node_base_.get(), - this->node_timers_.get()); + this->node_timers_.get(), + autostart); } template diff --git a/rclcpp/include/rclcpp/timer.hpp b/rclcpp/include/rclcpp/timer.hpp index e31c9fe4bf..68ce48ddda 100644 --- a/rclcpp/include/rclcpp/timer.hpp +++ b/rclcpp/include/rclcpp/timer.hpp @@ -53,12 +53,14 @@ class TimerBase * \param clock A clock to use for time and sleeping * \param period The interval at which the timer fires * \param context node context + * \param autostart timer state on initialization */ RCLCPP_PUBLIC explicit TimerBase( Clock::SharedPtr clock, std::chrono::nanoseconds period, - rclcpp::Context::SharedPtr context); + rclcpp::Context::SharedPtr context, + bool autostart = true); /// TimerBase destructor RCLCPP_PUBLIC @@ -152,8 +154,11 @@ class TimerBase protected: Clock::SharedPtr clock_; std::shared_ptr timer_handle_; + rclcpp::Context::SharedPtr context_; + std::chrono::nanoseconds period_; std::atomic in_use_by_wait_set_{false}; + bool autostart_{false}; }; @@ -179,12 +184,13 @@ class GenericTimer : public TimerBase * \param[in] period The interval at which the timer fires. * \param[in] callback User-specified callback function. * \param[in] context custom context to be used. + * \param autostart timer state on initialization */ explicit GenericTimer( Clock::SharedPtr clock, std::chrono::nanoseconds period, FunctorT && callback, - rclcpp::Context::SharedPtr context + rclcpp::Context::SharedPtr context, bool autostart = true ) - : TimerBase(clock, period, context), callback_(std::forward(callback)) + : TimerBase(clock, period, context, autostart), callback_(std::forward(callback)) { TRACEPOINT( rclcpp_timer_callback_added, @@ -287,13 +293,15 @@ class WallTimer : public GenericTimer * \param period The interval at which the timer fires * \param callback The callback function to execute every interval * \param context node context + * \param autostart timer state on initialization */ WallTimer( std::chrono::nanoseconds period, FunctorT && callback, - rclcpp::Context::SharedPtr context) + rclcpp::Context::SharedPtr context, + bool autostart = true) : GenericTimer( - std::make_shared(RCL_STEADY_TIME), period, std::move(callback), context) + std::make_shared(RCL_STEADY_TIME), period, std::move(callback), context, autostart) {} protected: diff --git a/rclcpp/src/rclcpp/timer.cpp b/rclcpp/src/rclcpp/timer.cpp index 788cdf8dce..a5c43b324a 100644 --- a/rclcpp/src/rclcpp/timer.cpp +++ b/rclcpp/src/rclcpp/timer.cpp @@ -29,8 +29,9 @@ using rclcpp::TimerBase; TimerBase::TimerBase( rclcpp::Clock::SharedPtr clock, std::chrono::nanoseconds period, - rclcpp::Context::SharedPtr context) -: clock_(clock), timer_handle_(nullptr) + rclcpp::Context::SharedPtr context, + bool autostart) +: clock_(clock), timer_handle_(nullptr), context_(context), period_(period), autostart_(autostart) { if (nullptr == context) { context = rclcpp::contexts::get_global_default_context(); @@ -62,8 +63,8 @@ TimerBase::TimerBase( { std::lock_guard clock_guard(clock_->get_clock_mutex()); rcl_ret_t ret = rcl_timer_init( - timer_handle_.get(), clock_handle, rcl_context.get(), period.count(), nullptr, - rcl_get_default_allocator()); + timer_handle_.get(), clock_handle, rcl_context.get(), period.count(), + nullptr, rcl_get_default_allocator(), autostart_); if (ret != RCL_RET_OK) { rclcpp::exceptions::throw_from_rcl_error(ret, "Couldn't initialize rcl timer handle"); } diff --git a/rclcpp/test/rclcpp/test_create_timer.cpp b/rclcpp/test/rclcpp/test_create_timer.cpp index 13c3564544..b35d3f8049 100644 --- a/rclcpp/test/rclcpp/test_create_timer.cpp +++ b/rclcpp/test/rclcpp/test_create_timer.cpp @@ -133,3 +133,28 @@ TEST(TestCreateTimer, timer_function_pointer) rclcpp::shutdown(); } + +TEST(TestCreateTimer, timer_without_autostart) +{ + rclcpp::init(0, nullptr); + auto node = std::make_shared("test_create_timer_node"); + + rclcpp::TimerBase::SharedPtr timer; + timer = rclcpp::create_timer( + node, + node->get_clock(), + rclcpp::Duration(0ms), + []() {}, + nullptr, + false); + + ASSERT_TRUE(timer->is_canceled()); + + timer->reset(); + + ASSERT_FALSE(timer->is_canceled()); + + timer->cancel(); + + rclcpp::shutdown(); +} diff --git a/rclcpp/test/rclcpp/test_timer.cpp b/rclcpp/test/rclcpp/test_timer.cpp index 7a6599dfe4..a744ea1def 100644 --- a/rclcpp/test/rclcpp/test_timer.cpp +++ b/rclcpp/test/rclcpp/test_timer.cpp @@ -59,6 +59,20 @@ class TestTimer : public ::testing::Test ); EXPECT_TRUE(timer->is_steady()); + timer_without_autostart = test_node->create_wall_timer( + 100ms, + [this]() -> void + { + this->has_timer_run.store(true); + + if (this->cancel_timer.load()) { + this->timer->cancel(); + } + // prevent any tests running timer from blocking + this->executor->cancel(); + }, nullptr, false); + EXPECT_TRUE(timer_without_autostart->is_steady()); + executor->add_node(test_node); // don't start spinning, let the test dictate when } @@ -78,6 +92,7 @@ class TestTimer : public ::testing::Test std::atomic cancel_timer; rclcpp::Node::SharedPtr test_node; std::shared_ptr timer; + std::shared_ptr timer_without_autostart; std::shared_ptr executor; }; @@ -283,3 +298,12 @@ TEST_F(TestTimer, test_failures_with_exceptions) std::runtime_error("Timer could not get time until next call: error not set")); } } + +/// Simple test of a timer without autostart +TEST_F(TestTimer, test_timer_without_autostart) +{ + EXPECT_TRUE(timer_without_autostart->is_canceled()); + // Reset to change start timer + timer_without_autostart->reset(); + EXPECT_FALSE(timer_without_autostart->is_canceled()); +} From f97543992659bc88d54c2c7153d4601461f7794f Mon Sep 17 00:00:00 2001 From: Voldivh Date: Fri, 26 Aug 2022 10:26:24 -0500 Subject: [PATCH 2/4] Removes unnecessary variables Signed-off-by: Voldivh --- rclcpp/include/rclcpp/timer.hpp | 3 --- rclcpp/src/rclcpp/timer.cpp | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/rclcpp/include/rclcpp/timer.hpp b/rclcpp/include/rclcpp/timer.hpp index 68ce48ddda..5cb453c1bd 100644 --- a/rclcpp/include/rclcpp/timer.hpp +++ b/rclcpp/include/rclcpp/timer.hpp @@ -154,11 +154,8 @@ class TimerBase protected: Clock::SharedPtr clock_; std::shared_ptr timer_handle_; - rclcpp::Context::SharedPtr context_; - std::chrono::nanoseconds period_; std::atomic in_use_by_wait_set_{false}; - bool autostart_{false}; }; diff --git a/rclcpp/src/rclcpp/timer.cpp b/rclcpp/src/rclcpp/timer.cpp index a5c43b324a..190ad4dae0 100644 --- a/rclcpp/src/rclcpp/timer.cpp +++ b/rclcpp/src/rclcpp/timer.cpp @@ -31,7 +31,7 @@ TimerBase::TimerBase( std::chrono::nanoseconds period, rclcpp::Context::SharedPtr context, bool autostart) -: clock_(clock), timer_handle_(nullptr), context_(context), period_(period), autostart_(autostart) +: clock_(clock), timer_handle_(nullptr) { if (nullptr == context) { context = rclcpp::contexts::get_global_default_context(); @@ -64,7 +64,7 @@ TimerBase::TimerBase( std::lock_guard clock_guard(clock_->get_clock_mutex()); rcl_ret_t ret = rcl_timer_init( timer_handle_.get(), clock_handle, rcl_context.get(), period.count(), - nullptr, rcl_get_default_allocator(), autostart_); + nullptr, rcl_get_default_allocator(), autostart); if (ret != RCL_RET_OK) { rclcpp::exceptions::throw_from_rcl_error(ret, "Couldn't initialize rcl timer handle"); } From c2426887c3ac88e1108a1184cb5dc1c13442af4e Mon Sep 17 00:00:00 2001 From: Voldivh Date: Tue, 30 Aug 2022 08:13:31 -0500 Subject: [PATCH 3/4] Adds autostart documentation and expands some timer test Signed-off-by: Voldivh --- rclcpp/include/rclcpp/create_timer.hpp | 1 + rclcpp/test/rclcpp/test_create_timer.cpp | 7 ++++--- rclcpp/test/rclcpp/test_timer.cpp | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/rclcpp/include/rclcpp/create_timer.hpp b/rclcpp/include/rclcpp/create_timer.hpp index 9bfe34ae01..19618b4bc8 100644 --- a/rclcpp/include/rclcpp/create_timer.hpp +++ b/rclcpp/include/rclcpp/create_timer.hpp @@ -85,6 +85,7 @@ create_timer( * \param group * \param node_base * \param node_timers + * \param autostart defines if the timer should start it's countdown on initialization or not. * \return * \throws std::invalid argument if either node_base or node_timers * are null, or period is negative or too large diff --git a/rclcpp/test/rclcpp/test_create_timer.cpp b/rclcpp/test/rclcpp/test_create_timer.cpp index b35d3f8049..78866836d3 100644 --- a/rclcpp/test/rclcpp/test_create_timer.cpp +++ b/rclcpp/test/rclcpp/test_create_timer.cpp @@ -148,11 +148,12 @@ TEST(TestCreateTimer, timer_without_autostart) nullptr, false); - ASSERT_TRUE(timer->is_canceled()); + EXPECT_TRUE(timer->is_canceled()); + EXPECT_EQ(timer->time_until_trigger().count(), std::chrono::nanoseconds::max().count()); timer->reset(); - - ASSERT_FALSE(timer->is_canceled()); + EXPECT_LE(timer->time_until_trigger().count(), std::chrono::nanoseconds::max().count()); + EXPECT_FALSE(timer->is_canceled()); timer->cancel(); diff --git a/rclcpp/test/rclcpp/test_timer.cpp b/rclcpp/test/rclcpp/test_timer.cpp index a744ea1def..fe04216e19 100644 --- a/rclcpp/test/rclcpp/test_timer.cpp +++ b/rclcpp/test/rclcpp/test_timer.cpp @@ -303,7 +303,13 @@ TEST_F(TestTimer, test_failures_with_exceptions) TEST_F(TestTimer, test_timer_without_autostart) { EXPECT_TRUE(timer_without_autostart->is_canceled()); + EXPECT_EQ( + timer_without_autostart->time_until_trigger().count(), + std::chrono::nanoseconds::max().count()); // Reset to change start timer timer_without_autostart->reset(); + EXPECT_LE( + timer_without_autostart->time_until_trigger().count(), + std::chrono::nanoseconds::max().count()); EXPECT_FALSE(timer_without_autostart->is_canceled()); } From 2ef04786849a269b03fca27cbdb0da615dea89d8 Mon Sep 17 00:00:00 2001 From: Voldivh Date: Thu, 15 Jun 2023 17:29:30 -0500 Subject: [PATCH 4/4] Adds a little documentation on how to start the timer if autostart is false Signed-off-by: Voldivh --- rclcpp/include/rclcpp/timer.hpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rclcpp/include/rclcpp/timer.hpp b/rclcpp/include/rclcpp/timer.hpp index 4fa3be6e53..6060d8bd78 100644 --- a/rclcpp/include/rclcpp/timer.hpp +++ b/rclcpp/include/rclcpp/timer.hpp @@ -54,6 +54,9 @@ class TimerBase * \param period The interval at which the timer fires * \param context node context * \param autostart timer state on initialization + * + * In order to activate a timer that is not started on initialization, + * user should call the reset() method. */ RCLCPP_PUBLIC explicit TimerBase(