From 43f8163b9d3421912c85b9a7d367996ff406a44e Mon Sep 17 00:00:00 2001 From: Eloy Briceno <51831786+Voldivh@users.noreply.github.com> Date: Wed, 21 Jun 2023 09:46:58 -0500 Subject: [PATCH] Modifies timers API to select autostart state (#1004) * Modifies timers API to select autostart state * Add test for the initial state of the timer * Marks as deprecated the API rcl_timer_init Signed-off-by: Voldivh Signed-off-by: Chris Lalancette --- rcl/include/rcl/timer.h | 22 +++- rcl/src/rcl/timer.c | 17 ++- rcl/test/rcl/test_timer.cpp | 142 +++++++++++++++------- rcl/test/rcl/test_wait.cpp | 14 ++- rcl_action/src/rcl_action/action_server.c | 4 +- 5 files changed, 142 insertions(+), 57 deletions(-) diff --git a/rcl/include/rcl/timer.h b/rcl/include/rcl/timer.h index e0a31f2b0..c2f7bc1fb 100644 --- a/rcl/include/rcl/timer.h +++ b/rcl/include/rcl/timer.h @@ -31,6 +31,7 @@ extern "C" #include "rcl/macros.h" #include "rcl/time.h" #include "rcl/types.h" +#include "rcutils/logging_macros.h" #include "rmw/rmw.h" typedef struct rcl_timer_impl_s rcl_timer_impl_t; @@ -125,8 +126,8 @@ rcl_get_zero_initialized_timer(void); * // ... error handling * * rcl_timer_t timer = rcl_get_zero_initialized_timer(); - * ret = rcl_timer_init( - * &timer, &clock, context, RCL_MS_TO_NS(100), my_timer_callback, allocator); + * ret = rcl_timer_init2( + * &timer, &clock, context, RCL_MS_TO_NS(100), my_timer_callback, allocator, true); * // ... error handling, use the timer with a wait set, or poll it manually, then cleanup * ret = rcl_timer_fini(&timer); * // ... error handling @@ -151,6 +152,7 @@ rcl_get_zero_initialized_timer(void); * \param[in] period the duration between calls to the callback in nanoseconds * \param[in] callback the user defined function to be called every period * \param[in] allocator the allocator to use for allocations + * \param[in] autostart the state of the timer at initialization * \return #RCL_RET_OK if the timer was initialized successfully, or * \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or * \return #RCL_RET_ALREADY_INIT if the timer was already initialized, or @@ -160,6 +162,22 @@ rcl_get_zero_initialized_timer(void); RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t +rcl_timer_init2( + rcl_timer_t * timer, + rcl_clock_t * clock, + rcl_context_t * context, + int64_t period, + const rcl_timer_callback_t callback, + rcl_allocator_t allocator, + bool autostart); + +/** + * \deprecated `rcl_timer_init` implementation was removed. + * Refer to `rcl_timer_init2`. + */ +RCL_PUBLIC +RCUTILS_DEPRECATED_WITH_MSG("Call rcl_timer_init2 instead") +rcl_ret_t rcl_timer_init( rcl_timer_t * timer, rcl_clock_t * clock, diff --git a/rcl/src/rcl/timer.c b/rcl/src/rcl/timer.c index 93398b98e..23b768d25 100644 --- a/rcl/src/rcl/timer.c +++ b/rcl/src/rcl/timer.c @@ -134,6 +134,21 @@ rcl_timer_init( int64_t period, const rcl_timer_callback_t callback, rcl_allocator_t allocator) +{ + return rcl_timer_init2( + timer, clock, context, period, callback, + allocator, true); +} + +rcl_ret_t +rcl_timer_init2( + rcl_timer_t * timer, + rcl_clock_t * clock, + rcl_context_t * context, + int64_t period, + const rcl_timer_callback_t callback, + rcl_allocator_t allocator, + bool autostart) { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT); @@ -182,7 +197,7 @@ rcl_timer_init( atomic_init(&impl.time_credit, 0); atomic_init(&impl.last_call_time, now); atomic_init(&impl.next_call_time, now + period); - atomic_init(&impl.canceled, false); + atomic_init(&impl.canceled, !autostart); impl.allocator = allocator; // Empty init on reset callback data diff --git a/rcl/test/rcl/test_timer.cpp b/rcl/test/rcl/test_timer.cpp index ec36259c4..636e3aa2d 100644 --- a/rcl/test/rcl/test_timer.cpp +++ b/rcl/test/rcl/test_timer.cpp @@ -112,9 +112,9 @@ class TestPreInitTimer : public TestTimerFixture RCL_RET_OK, rcl_clock_init(RCL_ROS_TIME, &clock, &allocator)) << rcl_get_error_string().str; - ret = rcl_timer_init( + ret = rcl_timer_init2( &timer, &clock, this->context_ptr, RCL_S_TO_NS(1), timer_callback_test, - rcl_get_default_allocator()); + rcl_get_default_allocator(), true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } @@ -133,29 +133,29 @@ TEST_F(TestTimerFixture, test_timer_init_with_invalid_arguments) { ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - nullptr, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, allocator); + ret = rcl_timer_init2( + nullptr, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, allocator, true); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); rcl_reset_error(); - ret = rcl_timer_init( - &timer, nullptr, this->context_ptr, RCL_MS_TO_NS(50), nullptr, allocator); + ret = rcl_timer_init2( + &timer, nullptr, this->context_ptr, RCL_MS_TO_NS(50), nullptr, allocator, true); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); rcl_reset_error(); - ret = rcl_timer_init( - &timer, &clock, nullptr, RCL_MS_TO_NS(50), nullptr, allocator); + ret = rcl_timer_init2( + &timer, &clock, nullptr, RCL_MS_TO_NS(50), nullptr, allocator, true); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); rcl_reset_error(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, -1, nullptr, allocator); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, -1, nullptr, allocator, true); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); rcl_reset_error(); rcl_allocator_t invalid_allocator = rcutils_get_zero_initialized_allocator(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, invalid_allocator); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, invalid_allocator, true); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); rcl_reset_error(); } @@ -167,8 +167,8 @@ TEST_F(TestTimerFixture, test_timer_with_invalid_clock) { ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, 0, nullptr, allocator); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, 0, nullptr, allocator, true); EXPECT_EQ(RCL_RET_ERROR, ret); rcl_reset_error(); @@ -180,8 +180,8 @@ TEST_F(TestTimerFixture, test_timer_with_invalid_clock) { EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; }); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, 0, nullptr, allocator); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, 0, nullptr, allocator, true); ASSERT_EQ(RCL_RET_OK, ret); OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -233,12 +233,14 @@ TEST_F(TestTimerFixture, test_two_timers) { rcl_timer_t timer = rcl_get_zero_initialized_timer(); rcl_timer_t timer2 = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - ret = rcl_timer_init( - &timer2, &clock, this->context_ptr, RCL_MS_TO_NS(1000), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer2, &clock, this->context_ptr, RCL_MS_TO_NS(1000), nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set(); @@ -301,12 +303,14 @@ TEST_F(TestTimerFixture, test_two_timers_ready_before_timeout) { rcl_timer_t timer2 = rcl_get_zero_initialized_timer(); // Keep the first timer period low enough so that rcl_wait() doesn't timeout too early. - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, RCL_MS_TO_NS(10), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_MS_TO_NS(10), nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - ret = rcl_timer_init( - &timer2, &clock, this->context_ptr, RCL_MS_TO_NS(1000), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer2, &clock, this->context_ptr, RCL_MS_TO_NS(1000), nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set(); @@ -367,8 +371,9 @@ TEST_F(TestTimerFixture, test_timer_not_ready) { rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, RCL_MS_TO_NS(1000), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_MS_TO_NS(1000), nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set(); @@ -415,8 +420,9 @@ TEST_F(TestTimerFixture, test_timer_overrun) { }); rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, RCL_MS_TO_NS(200), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_MS_TO_NS(200), nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -470,8 +476,9 @@ TEST_F(TestTimerFixture, test_timer_with_zero_period) { }); rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, 0, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, 0, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -492,6 +499,43 @@ TEST_F(TestTimerFixture, test_timer_with_zero_period) { EXPECT_EQ(RCL_RET_OK, rcl_timer_call(&timer)) << rcl_get_error_string().str; } +TEST_F(TestTimerFixture, test_timer_init_state) { + rcl_ret_t ret; + + rcl_clock_t clock; + rcl_allocator_t allocator = rcl_get_default_allocator(); + ret = rcl_clock_init(RCL_STEADY_TIME, &clock, &allocator); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + rcl_timer_t timer = rcl_get_zero_initialized_timer(); + + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_S_TO_NS(1), nullptr, rcl_get_default_allocator(), + false); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + bool is_canceled = false; + ret = rcl_timer_is_canceled(&timer, &is_canceled); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + EXPECT_TRUE(is_canceled); + + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_S_TO_NS(1), nullptr, rcl_get_default_allocator(), + true); + ASSERT_EQ(RCL_RET_ALREADY_INIT, ret) << rcl_get_error_string().str; + + timer = rcl_get_zero_initialized_timer(); + + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_S_TO_NS(1), nullptr, rcl_get_default_allocator(), + true); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_timer_is_canceled(&timer, &is_canceled); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + EXPECT_FALSE(is_canceled); +} + TEST_F(TestTimerFixture, test_canceled_timer) { rcl_ret_t ret; @@ -502,8 +546,9 @@ TEST_F(TestTimerFixture, test_canceled_timer) { rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, 500, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, 500, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; ret = rcl_timer_cancel(&timer); @@ -561,8 +606,9 @@ TEST_F(TestTimerFixture, test_rostime_time_until_next_call) { ASSERT_EQ(RCL_RET_OK, rcl_enable_ros_time_override(&clock)) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -600,8 +646,9 @@ TEST_F(TestTimerFixture, test_system_time_to_ros_time) { }); rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -643,8 +690,9 @@ TEST_F(TestTimerFixture, test_ros_time_to_system_time) { ASSERT_EQ(RCL_RET_OK, rcl_enable_ros_time_override(&clock)) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -689,8 +737,9 @@ TEST_F(TestTimerFixture, test_ros_time_backwards_jump) { ASSERT_EQ(RCL_RET_OK, rcl_enable_ros_time_override(&clock)) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, sec_5, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -733,8 +782,9 @@ TEST_F(TestTimerFixture, test_ros_time_wakes_wait) { ASSERT_EQ(RCL_RET_OK, rcl_enable_ros_time_override(&clock)) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, sec_1, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, sec_1, nullptr, rcl_get_default_allocator(), + true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -912,15 +962,15 @@ TEST_F(TestPreInitTimer, test_invalid_init_fini) { rcl_timer_t timer_fail = rcl_get_zero_initialized_timer(); EXPECT_EQ( - RCL_RET_ALREADY_INIT, rcl_timer_init( + RCL_RET_ALREADY_INIT, rcl_timer_init2( &timer, &clock, this->context_ptr, 500, nullptr, - rcl_get_default_allocator())) << rcl_get_error_string().str; + rcl_get_default_allocator(), true)) << rcl_get_error_string().str; rcl_reset_error(); ASSERT_EQ( - RCL_RET_BAD_ALLOC, rcl_timer_init( + RCL_RET_BAD_ALLOC, rcl_timer_init2( &timer_fail, &clock, this->context_ptr, RCL_S_TO_NS(1), timer_callback_test, - bad_allocator)) << rcl_get_error_string().str; + bad_allocator, true)) << rcl_get_error_string().str; rcl_reset_error(); EXPECT_EQ(RCL_RET_OK, rcl_timer_fini(nullptr)) << rcl_get_error_string().str; diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 11fe07c7a..daadfbf66 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -188,8 +188,9 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), negative_timeout) { ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, RCL_MS_TO_NS(10), nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, RCL_MS_TO_NS(10), nullptr, rcl_get_default_allocator(), + true); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -305,8 +306,9 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), zero_timeout_overrun_t ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_timer_t timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( - &timer, &clock, this->context_ptr, 0, nullptr, rcl_get_default_allocator()); + ret = rcl_timer_init2( + &timer, &clock, this->context_ptr, 0, nullptr, rcl_get_default_allocator(), + true); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { @@ -362,9 +364,9 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), canceled_timer) { ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_timer_t canceled_timer = rcl_get_zero_initialized_timer(); - ret = rcl_timer_init( + ret = rcl_timer_init2( &canceled_timer, &clock, this->context_ptr, - RCL_MS_TO_NS(1), nullptr, rcl_get_default_allocator()); + RCL_MS_TO_NS(1), nullptr, rcl_get_default_allocator(), true); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index 19987f521..2320c9286 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -174,9 +174,9 @@ rcl_action_server_init( action_server->impl->clock = clock; // Initialize Timer - ret = rcl_timer_init( + ret = rcl_timer_init2( &action_server->impl->expire_timer, action_server->impl->clock, node->context, - options->result_timeout.nanoseconds, NULL, allocator); + options->result_timeout.nanoseconds, NULL, allocator, true); if (RCL_RET_OK != ret) { goto fail; }