Skip to content

fix: Expose timers used by rclcpp::Waitables#2699

Merged
ahcorde merged 3 commits intoros2:rollingfrom
cellumation:expose_timers_of_waitables
Jan 30, 2025
Merged

fix: Expose timers used by rclcpp::Waitables#2699
ahcorde merged 3 commits intoros2:rollingfrom
cellumation:expose_timers_of_waitables

Conversation

@jmachowinski
Copy link
Copy Markdown
Collaborator

@jmachowinski jmachowinski commented Dec 6, 2024

This change was needed, as an events based executors needs to be aware of the timers, in order generate the timer ready events.

@alsora @wjwwood This PR is the followup to the discussion two weeks back in the client workgroup.

@jmachowinski jmachowinski mentioned this pull request Dec 6, 2024
Merged
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Dec 6, 2024

Added a fix for the expire timer not working for action servers.
Note, this is an alternative to #2661 for rolling.

@jmachowinski jmachowinski force-pushed the expose_timers_of_waitables branch from 335fbe4 to c172276 Compare December 6, 2024 13:39
@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Dec 6, 2024

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-6th-december-2024-8am-pt/40954/2

alsora
alsora approved these changes Dec 10, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

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

The changes look good.
Would you mind adding a couple of unit-tests that exercise the new method?
It would be ideal to have:

  • a test for the action server to verify that it returns 1 timer
  • a test with the events executor where we verify that the timer is correctly handled

@jmachowinski jmachowinski force-pushed the expose_timers_of_waitables branch from f6651f9 to 35694c8 Compare December 11, 2024 16:40
@jmachowinski jmachowinski marked this pull request as ready for review January 17, 2025 12:53
@jmachowinski jmachowinski force-pushed the expose_timers_of_waitables branch from 35694c8 to f0ee613 Compare January 17, 2025 13:18
mjcarroll
mjcarroll approved these changes Jan 27, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Changes LGTM with green CI.

@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Jan 28, 2025

Pulls: #2699
Gist: https://gist.githubusercontent.com/ahcorde/e1b81cfe9cc4cb47caeba7602ece63fc/raw/5045048b2525ca7c7ace02cd94188f9b42e41d9a/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15097

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Jan 28, 2025

@ahcorde this commit depends on ros2/rcl#1201, so we need a custom gist.

@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Jan 28, 2025

Pulls: #2699, ros2/rcl#1201
Gist: https://gist.githubusercontent.com/ahcorde/a4917dcff0940203284bcd592f2ecb96/raw/dfc2a9450c7b234ee278414773300dc944688bdf/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rcl --packages-above-and-dependencies rclcpp rcl
TEST args: --packages-above rclcpp rcl --packages-above rclcpp rcl
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15100

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski jmachowinski mentioned this pull request Jan 28, 2025
Merged
@jmachowinski jmachowinski force-pushed the expose_timers_of_waitables branch from f0ee613 to 8e984ec Compare January 28, 2025 10:18
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Jan 28, 2025

Pulls: #2699, ros2/system_tests#556, ros2/rcl#1201
Gist: https://gist.githubusercontent.com/jmachowinski/711b3e342bed5cd8c16b1b4cffd6c361/raw/22af95b0a7ea9789dfda5280fe2b51d84965a9de/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15101

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski jmachowinski force-pushed the expose_timers_of_waitables branch from 8e984ec to f924997 Compare January 28, 2025 16:21
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Jan 28, 2025

Pulls: #2699, ros2/system_tests#556, ros2/rcl#1201
Gist: https://gist.githubusercontent.com/jmachowinski/7608e9da50fcb66b359b3a37b7a301b4/raw/22af95b0a7ea9789dfda5280fe2b51d84965a9de/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15102

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Janosch Machowinski added 3 commits January 29, 2025 10:57
fix: Expose timers used by rclcpp::Waitables
37654e8 
This change was needed, as an events based executors needs to be
aware of the timers, in order generate the timer ready events.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
fix(events_executor): Register Waitable timers at TimersManager
933480a 
This should fix a bug, were the internal expire timer of
the action server was not triggered.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
test: Added test case, if timer within waitable works as expected
2747640 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski jmachowinski force-pushed the expose_timers_of_waitables branch from f924997 to 2747640 Compare January 29, 2025 09:58
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Jan 29, 2025 •
edited
Loading

Pulls: #2699, ros2/system_tests#556, ros2/rcl#1201
Gist: https://gist.githubusercontent.com/jmachowinski/d0c0fd4939d4c2a8515d3e7c6039c009/raw/22af95b0a7ea9789dfda5280fe2b51d84965a9de/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15105

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Jan 29, 2025

CI checks out.
This must be merged after ros2/rcl#1201 and together with ros2/system_tests#556

@ahcorde ahcorde merged commit 6069c3d into ros2:rolling Jan 30, 2025
This was referenced Jan 30, 2025
Closed
Merged
@Crola1702 Crola1702 mentioned this pull request Jan 31, 2025
Closed
@Crola1702
Copy link
Copy Markdown
Contributor

Crola1702 commented Jan 31, 2025

Hey! @jmachowinski @ahcorde, this PR introduced an issue in Rolling CI: #2731

@skyegalaxy skyegalaxy mentioned this pull request Jan 16, 2026
Merged
@skyegalaxy skyegalaxy mentioned this pull request Jan 16, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@ahcorde ahcorde ahcorde approved these changes

@alsora alsora alsora approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jmachowinski @ros-discourse @ahcorde @Crola1702 @mjcarroll @alsora