If timer canceled, rcl_timer_get_time_until_next_call returns TIMER_CANCELED

[mauropasse]

Return TIMER_CANCELED rcl_timer_get_time_until_next_call

Signed-off-by: Mauro Passerino mpasserino@irobot.com

[alsora]

CI including also ros2/rclcpp#1893

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@ivanpauno could you take a look at this? i think it should be fixed in rcl but rclpy or rclcpp.

[ivanpauno]

LGTM!

I'm not sure if there was a reason for the current behavior, @wjwwood @jacobperron to double check.

I guess this will need small changes in rclcpp/rclpy to handle the new return code, right?

[ivanpauno]

i think it should be fixed in rcl but rclpy or rclcpp.

Sorry I didn't get that, what do you mean? Do you mean to ignore this change and checking .is_cancelled() in rclpy/rclcpp?

[fujitatomoya]

I guess this will need small changes in rclcpp/rclpy to handle the new return code, right?

• rclcpp, TimerBase::time_until_trigger() throws an exception if it returns error. so no need to change.
• rclpy, Timer::time_until_next_call() throws an exception if it returns error. so no need to change.

Do you mean to ignore this change and checking .is_cancelled() in rclpy/rclcpp?

No, opposite. I am the one who suggested this fix in rcl.
So that we can close ros2/rclcpp#1893.

[fujitatomoya]

@mauropasse could you check the CI failure?

[jacobperron]

I'm not sure if there was a reason for the current behavior, @wjwwood @jacobperron to double check.

Looks okay to me.

[mauropasse]

Fixed test.

I guess this will need small changes in rclcpp/rclpy to handle the new return code, right?

@ivanpauno the new return code is handled in rclcpp in ros2/rclcpp#1893 and rclpy ros2/rclpy#907

rclcpp, TimerBase::time_until_trigger() throws an exception if it returns error. so no need to change.

@fujitatomoya we don't want to throw an exeption if timer is canceled and asking for next trigger time, that's why ros2/rclcpp#1893 and ros2/rclpy#907 are needed.
So currently, rcl sets TIMER_CANCELED and rclcpp gets this return code and returns std::chrono::nanoseconds::max() when calling TimerBase::time_until_trigger(). Or maybe your proposal is doing everything in rcl, i.e. return a max time in rcl_timer_get_time_until_next_call ?

[alsora]

New CI including also the rclcpp and rclpy PRs

• Linux
• Linux-aarch64
• Windows

[ivanpauno]

So currently, rcl sets TIMER_CANCELED and rclcpp gets this return code and returns std::chrono::nanoseconds::max() when calling TimerBase::time_until_trigger(). Or maybe your proposal is doing everything in rcl, i.e. return a max time in rcl_timer_get_time_until_next_call ?

If we want to always return std::chrono::nanoseconds::max() when the timer is cancelled, maybe it's simpler to do that directly in rcl.
Anyways, the PR LGTM!

[fujitatomoya]

@mauropasse

we don't want to throw an exeption if timer is canceled and asking for next trigger time, that's why ros2/rclcpp#1893 and ros2/rclpy#907 are needed.

sounds good to me. probably this is better instead of exception.

[alsora]

Another round of CI including the rclcpp and rclpy PRs:

• Linux
• Linux-aarch64
• Windows

[mauropasse]

CI is unstable due other reasons than changes in this PR (other PRs are showing the same unstable output).
Is there anything else required for this PR to be merged?

[fujitatomoya]

I think this would be better with rclpy? which i made here, ros2/rclpy#910

[mauropasse]

I think this would be better with rclpy? which i made here, ros2/rclpy#910

Yes that's better, thanks. I guess we're waiting for confirmation to change the API signature of
int64_t Timer::time_until_next_call() on your rclpy PR.