Skip to content

Check if the value of the timer pointer is null and exchange by signed int64 #972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Check if the value of the timer pointer is null and exchange by signed int64 #972

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions rcl/src/rcl/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ typedef struct rcl_timer_impl_t
// The user supplied callback.
atomic_uintptr_t callback;
// This is a duration in nanoseconds.
atomic_uint_least64_t period;
atomic_int_least64_t period;
Copy link
Collaborator

@fujitatomoya fujitatomoya Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are addressing #967 on this PR too, but probably it would be nice to open different PR for this. and i think #967 (comment) makes more sense to me.

// This is a time in nanoseconds since an unspecified time.
atomic_int_least64_t last_call_time;
// This is a time in nanoseconds since an unspecified time.
Expand Down Expand Up @@ -92,7 +92,7 @@ void _rcl_timer_time_jump(
}
const int64_t last_call_time = rcutils_atomic_load_int64_t(&timer->impl->last_call_time);
const int64_t next_call_time = rcutils_atomic_load_int64_t(&timer->impl->next_call_time);
const int64_t period = rcutils_atomic_load_uint64_t(&timer->impl->period);
const int64_t period = rcutils_atomic_load_int64_t(&timer->impl->period);
if (RCL_ROS_TIME_ACTIVATED == time_jump->clock_change ||
RCL_ROS_TIME_DEACTIVATED == time_jump->clock_change)
{
Expand Down Expand Up @@ -233,6 +233,10 @@ rcl_ret_t
rcl_timer_clock(rcl_timer_t * timer, rcl_clock_t ** clock)
{
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
Comment on lines +236 to +239
Copy link
Collaborator

@fujitatomoya fujitatomoya Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about the following? so are other places.

Suggested change
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(timer->impl, RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_ARGUMENT_FOR_NULL(clock, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(timer->impl, RCL_RET_TIMER_INVALID);
*clock = timer->impl->clock;
Expand All @@ -244,6 +248,10 @@ rcl_timer_call(rcl_timer_t * timer)
{
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Calling timer");
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
if (rcutils_atomic_load_bool(&timer->impl->canceled)) {
RCL_SET_ERROR_MSG("timer is canceled");
return RCL_RET_TIMER_CANCELED;
Expand All @@ -263,7 +271,7 @@ rcl_timer_call(rcl_timer_t * timer)
(rcl_timer_callback_t)rcutils_atomic_load_uintptr_t(&timer->impl->callback);

int64_t next_call_time = rcutils_atomic_load_int64_t(&timer->impl->next_call_time);
int64_t period = rcutils_atomic_load_uint64_t(&timer->impl->period);
int64_t period = rcutils_atomic_load_int64_t(&timer->impl->period);
// always move the next call time by exactly period forward
// don't use now as the base to avoid extending each cycle by the time
// between the timer being ready and the callback being triggered
Expand Down Expand Up @@ -294,6 +302,10 @@ rcl_ret_t
rcl_timer_is_ready(const rcl_timer_t * timer, bool * is_ready)
{
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(is_ready, RCL_RET_INVALID_ARGUMENT);
int64_t time_until_next_call;
rcl_ret_t ret = rcl_timer_get_time_until_next_call(timer, &time_until_next_call);
Expand All @@ -308,6 +320,10 @@ rcl_ret_t
rcl_timer_get_time_until_next_call(const rcl_timer_t * timer, int64_t * time_until_next_call)
{
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(time_until_next_call, RCL_RET_INVALID_ARGUMENT);
rcl_time_point_value_t now;
rcl_ret_t ret = rcl_clock_get_now(timer->impl->clock, &now);
Expand All @@ -325,6 +341,10 @@ rcl_timer_get_time_since_last_call(
rcl_time_point_value_t * time_since_last_call)
{
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(time_since_last_call, RCL_RET_INVALID_ARGUMENT);
rcl_time_point_value_t now;
rcl_ret_t ret = rcl_clock_get_now(timer->impl->clock, &now);
Expand All @@ -340,19 +360,30 @@ rcl_ret_t
rcl_timer_get_period(const rcl_timer_t * timer, int64_t * period)
{
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(period, RCL_RET_INVALID_ARGUMENT);
*period = rcutils_atomic_load_uint64_t(&timer->impl->period);
*period = rcutils_atomic_load_int64_t(&timer->impl->period);
return RCL_RET_OK;
}

rcl_ret_t
rcl_timer_exchange_period(const rcl_timer_t * timer, int64_t new_period, int64_t * old_period)
{
RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_INVALID_ARGUMENT);
if (new_period < 0) {
return RCL_RET_INVALID_ARGUMENT;
}

RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(old_period, RCL_RET_INVALID_ARGUMENT);
*old_period = rcutils_atomic_exchange_uint64_t(&timer->impl->period, new_period);
*old_period = rcutils_atomic_exchange_int64_t(&timer->impl->period, new_period);
RCUTILS_LOG_DEBUG_NAMED(
ROS_PACKAGE_NAME, "Updated timer period from '%" PRIu64 "ns' to '%" PRIu64 "ns'",
*old_period, new_period);
Expand Down Expand Up @@ -394,6 +425,10 @@ rcl_ret_t
rcl_timer_is_canceled(const rcl_timer_t * timer, bool * is_canceled)
{
RCL_CHECK_ARGUMENT_FOR_NULL(timer, RCL_RET_INVALID_ARGUMENT);
if (timer->impl == NULL) {
RCL_SET_ERROR_MSG("timer is invalid");
return RCL_RET_INVALID_ARGUMENT;
}
RCL_CHECK_ARGUMENT_FOR_NULL(is_canceled, RCL_RET_INVALID_ARGUMENT);
*is_canceled = rcutils_atomic_load_bool(&timer->impl->canceled);
return RCL_RET_OK;
Expand All @@ -411,7 +446,7 @@ rcl_timer_reset(rcl_timer_t * timer)
if (now_ret != RCL_RET_OK) {
return now_ret; // rcl error state should already be set.
}
int64_t period = rcutils_atomic_load_uint64_t(&timer->impl->period);
int64_t period = rcutils_atomic_load_int64_t(&timer->impl->period);
rcutils_atomic_store(&timer->impl->next_call_time, now + period);
rcutils_atomic_store(&timer->impl->canceled, false);
rcl_ret_t ret = rcl_trigger_guard_condition(&timer->impl->guard_condition);
Expand Down