rcl
rcl copied to clipboard
ros2
Modifies timers API to select autostart state
Signed-off-by: Voldivh [email protected]
This PR addresses the issue. Basically it gives the timer initialization a variable that enables or disables the autostart on initialization. The variable autostart is defaulted to True in order to avoid any breaks in current use of the API.
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 https://github.com/ros2/rclcpp/pull/2006#discussion_r973768620, 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 https://github.com/ros2/rcl/commit/c43c7abb0a254f4b60c42c4c2993917eb08cb0ec?
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
- https://github.com/ros2/rcl/pull/1004
- https://github.com/ros2/rclcpp/pull/2005
- https://github.com/ros2/rclpy/pull/999
- https://github.com/ros2/system_tests/pull/508
Hey @fujitatomoya, I believe the CI build failed because of the branch for https://github.com/ros2/system_tests/pull/508 was outdated. I already sync it with the latest changes from rolling, I think it should fix it.
@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 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: ros2/rcl#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.
- https://github.com/ros2/rcl/pull/1004
- https://github.com/ros2/rclcpp/pull/2005
- https://github.com/ros2/rclpy/pull/999
- https://github.com/ros2/system_tests/pull/508
CC: @emersonknapp @Voldivh