CC: @ivanpauno @wjwwood

having start and stop capabilities for the timer.

How would they work exactly?
Those sound like reset and cancel to me, only with different names.
The period of the timer can also be modified.
With that and the possibility of starting a timer "stopped" as this PR is adding, all the start/stop capabilities that I can imagine of a timer are supported.

for example, period is 30s timer. after 15s, it can call stop, then left time should be kept. if it starts the timer, 1st period will be 15s and after that 30s period. this is the difference from reset.

Ah sorry, you already explained my question.
I think that adding support for that is fine, but I don't think it's related to the functionality added here.
Naming of the new function(s) to achieve that is going to be confusing though ...

Taking into account the proposal of extending the capabilities of timers and some concerns on this PR regarding the use of the reset method. I wanted to show a simple flowchart on how this autostart feature is working at the moment.

Basically I'm giving a choice and using current 'states' of the timer to achieve the feature. I do think that in order to start a timer you have to reset it, sounds weird (semantically). However, my concern is that implementing a start() method would imply to basically code the same reset() method or call that method inside a new one (I'm not sure on what would be preferred on this case). Keeping that in mind and assuming we are going to add that start() method, we could differentiate it from the reset function by extending it in order to do the following:

That gives the possibility to reset the timer without it being autostarted and the start method is just a way to "uncancel" the timer so it goes to an ongoing state. Let me know what you guys think about this @fujitatomoya @ivanpauno

I believe that the latter one makes more sense. (related to ros2/rclcpp#2006 (comment), adding start / stop would be useful to support these features.)

@ros2/team friendly ping.

overall lgtm, sorry to be late to get back to this.

@Voldivh are you still working on this? there are conflicts what needs to be addressed.

Hey @fujitatomoya I stopped working on this some time ago. However, I believe it only requires a little final push to get this and the follow up PR's in. I believe I can get back on this later this week. Give me a couple of days to recall the implementation I was doing and solve the conflicts we currently have.

@Voldivh thanks for taking care of this, there is build failure https://build.ros2.org/job/Rpr__rcl__ubuntu_jammy_amd64/350/console

@Voldivh thanks for taking care of this, there is build failure https://build.ros2.org/job/Rpr__rcl__ubuntu_jammy_amd64/350/console

Yeah I noticed, however, I believe the error is coming from this commit made into rolling. I'm not sure if I should modify something here.

@christophebedard Rpr job is supposed to fail because of c43c7ab?

Yeah, it will fail until tracetools is released and available in the testing repo.

@Voldivh never mind the rpr job failure, i wll start the CI to verify.

CI:

• Linux
• Linux-aarch64
• Windows

with

• Modifies timers API to select autostart state #1004
• Modifies timers API to select autostart state rclcpp#2005
• Changes ros_timer_init for ros_timer_init2 rclpy#999
• Changes ros_timer_init for ros_timer_init2 system_tests#508

Hey @fujitatomoya, I believe the CI build failed because of the branch for ros2/system_tests#508 was outdated. I already sync it with the latest changes from rolling, I think it should fix it.

@Voldivh thanks for checking!

CI: 🤞

• Linux
• Linux-aarch64
• Windows

@fujitatomoya well that didn't do the trick :c. I'm not sure on what to do next, this is the error we are getting https://ci.ros2.org/job/ci_linux/18944/console#:~:text=12%3A18%3A46%20%3D%3D%3E,with%20return%20code%20%271%27

@fujitatomoya well that didn't do the trick :c. I'm not sure on what to do next, this is the error we are getting https://ci.ros2.org/job/ci_linux/18944/console#:~:text=12%3A18%3A46%20%3D%3D%3E,with%20return%20code%20%271%27

There's no package called system_tests, it is called test_rclcpp.

There's no package called system_tests, it is called test_rclcpp.

That makes a lot of sense, is that something that is defined when running the CI or do I have to modify something?

@Voldivh @clalancette Ah, sorry 😓 i will restart the CI.

CI:

• Linux
• Linux-aarch64
• Windows

@Voldivh i think we need to rebase rclcpp, please check https://ci.ros2.org/job/ci_linux/18949/console.

@Voldivh i think we need to rebase rclcpp, please check https://ci.ros2.org/job/ci_linux/18949/console.

I agree, it should be fine now after this commit.

Pulls: #1004, ros2/rclcpp#2005, ros2/system_tests#508, ros2/rclpy#999
Gist: https://gist.githubusercontent.com/emersonknapp/f9353e435d78a1ff796a38b74eaf0547/raw/2a9e6c7645332dcc2c4a6d0fb6f7b66e6b428bb7/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action rclcpp rclpy test_rclcpp
TEST args: --packages-above rcl rcl_action rclcpp rclpy test_rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12240

• Linux
• Linux-aarch64
• Windows

• Linux
• Linux-aarch64
• Windows

I think we had the same issue as before but now with rclpy. I already updated the branch. All branches from the different PR's should be up to date now with the latest updates of rolling.

Rebuilds build #12240
Running as SYSTEM
Building on the built-in node in workspace /var/lib/jenkins/jobs/ci_launcher/workspace

• Linux
• Linux-aarch64
• Windows

@clalancette can you review and merge these PRs? they need to be go in at the same time, but i do not have merge permission for system_tests.

• Modifies timers API to select autostart state #1004
• Modifies timers API to select autostart state rclcpp#2005
• Changes ros_timer_init for ros_timer_init2 rclpy#999
• Changes ros_timer_init for ros_timer_init2 system_tests#508

CC: @emersonknapp @Voldivh

Final? CI:

• Linux
• Linux-aarch64
• Windows