From 50ee2eeb599cbcdca4875117be1fe6383a951abb Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 20 Apr 2020 17:57:00 -0300 Subject: [PATCH 1/5] Save allocator when setting uninitialized clock Signed-off-by: Jorge Perez --- rcl/src/rcl/time.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/rcl/src/rcl/time.c b/rcl/src/rcl/time.c index a050a008c..2d2de99ed 100644 --- a/rcl/src/rcl/time.c +++ b/rcl/src/rcl/time.c @@ -48,13 +48,14 @@ rcl_get_system_time(void * data, rcl_time_point_value_t * current_time) // Internal method for zeroing values on init, assumes clock is valid static void -rcl_init_generic_clock(rcl_clock_t * clock) +rcl_init_generic_clock(rcl_clock_t * clock, rcl_allocator_t * allocator) { clock->type = RCL_CLOCK_UNINITIALIZED; clock->jump_callbacks = NULL; clock->num_jump_callbacks = 0u; clock->get_now = NULL; clock->data = NULL; + clock->allocator = *allocator; } // The function used to get the current ros time. @@ -91,7 +92,7 @@ rcl_clock_init( switch (clock_type) { case RCL_CLOCK_UNINITIALIZED: RCL_CHECK_ARGUMENT_FOR_NULL(clock, RCL_RET_INVALID_ARGUMENT); - rcl_init_generic_clock(clock); + rcl_init_generic_clock(clock, allocator); return RCL_RET_OK; case RCL_ROS_TIME: return rcl_ros_clock_init(clock, allocator); @@ -144,7 +145,7 @@ rcl_ros_clock_init( { RCL_CHECK_ARGUMENT_FOR_NULL(clock, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(allocator, RCL_RET_INVALID_ARGUMENT); - rcl_init_generic_clock(clock); + rcl_init_generic_clock(clock, allocator); clock->data = allocator->allocate(sizeof(rcl_ros_clock_storage_t), allocator->state); if (NULL == clock->data) { RCL_SET_ERROR_MSG("allocating memory failed"); @@ -186,7 +187,7 @@ rcl_steady_clock_init( { RCL_CHECK_ARGUMENT_FOR_NULL(clock, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(allocator, RCL_RET_INVALID_ARGUMENT); - rcl_init_generic_clock(clock); + rcl_init_generic_clock(clock, allocator); clock->get_now = rcl_get_steady_time; clock->type = RCL_STEADY_TIME; clock->allocator = *allocator; @@ -213,7 +214,7 @@ rcl_system_clock_init( { RCL_CHECK_ARGUMENT_FOR_NULL(clock, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(allocator, RCL_RET_INVALID_ARGUMENT); - rcl_init_generic_clock(clock); + rcl_init_generic_clock(clock, allocator); clock->get_now = rcl_get_system_time; clock->type = RCL_SYSTEM_TIME; clock->allocator = *allocator; From e1597c07a355b542231fbe96db556530cf3ade73 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 20 Apr 2020 18:02:28 -0300 Subject: [PATCH 2/5] Add tests for clock types init/fini Signed-off-by: Jorge Perez --- rcl/test/rcl/test_time.cpp | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/rcl/test/rcl/test_time.cpp b/rcl/test/rcl/test_time.cpp index 0a25ae282..d2c0e7fe5 100644 --- a/rcl/test/rcl/test_time.cpp +++ b/rcl/test/rcl/test_time.cpp @@ -263,11 +263,22 @@ TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), specific_clock_instantiation) { rcl_allocator_t allocator = rcl_get_default_allocator(); { rcl_clock_t uninitialized_clock; - rcl_ret_t ret = rcl_clock_init( - RCL_CLOCK_UNINITIALIZED, &uninitialized_clock, &allocator); + rcl_ret_t ret = rcl_clock_init(RCL_CLOCK_UNINITIALIZED, &uninitialized_clock, &allocator); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; EXPECT_EQ(uninitialized_clock.type, RCL_CLOCK_UNINITIALIZED) << "Expected time source of type RCL_CLOCK_UNINITIALIZED"; + ret = rcl_clock_fini(&uninitialized_clock); + EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT) << rcl_get_error_string().str; + rcl_reset_error(); + EXPECT_EQ( + rcl_ros_clock_fini(&uninitialized_clock), RCL_RET_ERROR) << rcl_get_error_string().str; + rcl_reset_error(); + EXPECT_EQ( + rcl_steady_clock_fini(&uninitialized_clock), RCL_RET_ERROR) << rcl_get_error_string().str; + rcl_reset_error(); + EXPECT_EQ( + rcl_system_clock_fini(&uninitialized_clock), RCL_RET_ERROR) << rcl_get_error_string().str; + rcl_reset_error(); } { rcl_clock_t ros_clock; @@ -296,6 +307,12 @@ TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), specific_clock_instantiation) { ret = rcl_clock_fini(&steady_clock); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; } + { + rcl_clock_t fail_clock; + rcl_clock_type_t undefined_type = (rcl_clock_type_t) 130; + rcl_ret_t ret = rcl_clock_init(undefined_type, &fail_clock, &allocator); + EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT) << rcl_get_error_string().str; + } } TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), rcl_time_difference) { From 69ec74934ecd4a3282e1b377d694fbad55123cae Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Tue, 21 Apr 2020 11:19:47 -0300 Subject: [PATCH 3/5] Remove extra calls to set clock allocator Signed-off-by: Jorge Perez --- rcl/src/rcl/time.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/rcl/src/rcl/time.c b/rcl/src/rcl/time.c index 2d2de99ed..3b9174356 100644 --- a/rcl/src/rcl/time.c +++ b/rcl/src/rcl/time.c @@ -158,7 +158,6 @@ rcl_ros_clock_init( storage->active = false; clock->get_now = rcl_get_ros_time; clock->type = RCL_ROS_TIME; - clock->allocator = *allocator; return RCL_RET_OK; } @@ -190,7 +189,6 @@ rcl_steady_clock_init( rcl_init_generic_clock(clock, allocator); clock->get_now = rcl_get_steady_time; clock->type = RCL_STEADY_TIME; - clock->allocator = *allocator; return RCL_RET_OK; } @@ -217,7 +215,6 @@ rcl_system_clock_init( rcl_init_generic_clock(clock, allocator); clock->get_now = rcl_get_system_time; clock->type = RCL_SYSTEM_TIME; - clock->allocator = *allocator; return RCL_RET_OK; } From ea90b9010d6c98c0aa64eeb17d8e9be739fa6b68 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Tue, 21 Apr 2020 11:30:37 -0300 Subject: [PATCH 4/5] Remove tests not related to this PR Signed-off-by: Jorge Perez --- rcl/test/rcl/test_time.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/rcl/test/rcl/test_time.cpp b/rcl/test/rcl/test_time.cpp index d2c0e7fe5..3a03bf840 100644 --- a/rcl/test/rcl/test_time.cpp +++ b/rcl/test/rcl/test_time.cpp @@ -270,15 +270,6 @@ TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), specific_clock_instantiation) { ret = rcl_clock_fini(&uninitialized_clock); EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT) << rcl_get_error_string().str; rcl_reset_error(); - EXPECT_EQ( - rcl_ros_clock_fini(&uninitialized_clock), RCL_RET_ERROR) << rcl_get_error_string().str; - rcl_reset_error(); - EXPECT_EQ( - rcl_steady_clock_fini(&uninitialized_clock), RCL_RET_ERROR) << rcl_get_error_string().str; - rcl_reset_error(); - EXPECT_EQ( - rcl_system_clock_fini(&uninitialized_clock), RCL_RET_ERROR) << rcl_get_error_string().str; - rcl_reset_error(); } { rcl_clock_t ros_clock; @@ -307,12 +298,6 @@ TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), specific_clock_instantiation) { ret = rcl_clock_fini(&steady_clock); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; } - { - rcl_clock_t fail_clock; - rcl_clock_type_t undefined_type = (rcl_clock_type_t) 130; - rcl_ret_t ret = rcl_clock_init(undefined_type, &fail_clock, &allocator); - EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT) << rcl_get_error_string().str; - } } TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), rcl_time_difference) { From ce8d867187741775b3bb22da355bb16acbf3d42a Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Tue, 21 Apr 2020 12:44:51 -0300 Subject: [PATCH 5/5] Replace test to make it clearer what is tested in the PR Signed-off-by: Jorge Perez --- rcl/test/rcl/test_time.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rcl/test/rcl/test_time.cpp b/rcl/test/rcl/test_time.cpp index 3a03bf840..2b552ea68 100644 --- a/rcl/test/rcl/test_time.cpp +++ b/rcl/test/rcl/test_time.cpp @@ -267,9 +267,7 @@ TEST(CLASSNAME(rcl_time, RMW_IMPLEMENTATION), specific_clock_instantiation) { EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; EXPECT_EQ(uninitialized_clock.type, RCL_CLOCK_UNINITIALIZED) << "Expected time source of type RCL_CLOCK_UNINITIALIZED"; - ret = rcl_clock_fini(&uninitialized_clock); - EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT) << rcl_get_error_string().str; - rcl_reset_error(); + EXPECT_TRUE(rcutils_allocator_is_valid(&(uninitialized_clock.allocator))); } { rcl_clock_t ros_clock;