Added optional TimerInfo to timer callback #2343

jmachowinski · 2023-10-27T11:21:24Z

This allows us to get the correct time of the timer callback as node->now() can
return a later timestamp than 'expected' due to race conditions.

This commit depends on ros2/rcl#1113

jmachowinski · 2023-10-27T11:21:49Z

@clalancette : ping

rclcpp/include/rclcpp/timer.hpp

sloretz · 2023-11-16T17:56:47Z

rclcpp/src/rclcpp/executor.cpp

@@ -709,9 +709,9 @@ Executor::execute_subscription(rclcpp::SubscriptionBase::SharedPtr subscription)
 }

 void
-Executor::execute_timer(rclcpp::TimerBase::SharedPtr timer)
+Executor::execute_timer(rclcpp::TimerBase::SharedPtr timer, const std::shared_ptr<void> & dataPtr)


Can this pointer be the actual type instead of void?

The return from take_data() is void as it's the standard in the rest of the code. Therefore this pointer is also void.

jmachowinski · 2024-01-29T08:25:39Z

@wjwwood @mjcarroll @fujitatomoya Ping, I would like to see this merged for jazzy

fujitatomoya

@jmachowinski thank you for the PR, i had a few comments.

besides, test would be ideal.

rclcpp/include/rclcpp/timer.hpp

rclcpp/src/rclcpp/executors/static_single_threaded_executor.cpp

rclcpp_action/src/client.cpp

rclcpp/include/rclcpp/timer.hpp

jmachowinski · 2024-02-20T09:33:51Z

@fujitatomoya Thanks for the review, I'll update the PR accordingly.

besides, test would be ideal.

Hm, I'm not really sure what to test here, the only thing I can think of would be that the info contains somewhat expected values.

jmachowinski · 2024-02-20T13:07:41Z

@fujitatomoya Added a simple test. And updated the commits.

clalancette · 2024-02-20T13:08:17Z

I haven't looked in this in too much detail, but I have to say that the use of (effectively) a void * pointer here is extremely suspicious. It doesn't give us any type checking at all. Can you give us more detail on exactly what this change is fixing?

jmachowinski · 2024-02-20T13:40:38Z

I haven't looked in this in too much detail, but I have to say that the use of (effectively) a void * pointer here is extremely suspicious. It doesn't give us any type checking at all. Can you give us more detail on exactly what this change is fixing?

See #2343 (comment) for the concrete example, of what we are trying to fix here.

The pointer returned by take_data is void, as you need to assign it to the AnyExecutable here

rclcpp/rclcpp/include/rclcpp/any_executable.hpp

Line 50 in 10252e9

std::shared_ptr<void> data;

in order to pass it on inside of the Executor.

This is pretty much the standard in this area of the code, see

rclcpp/rclcpp/include/rclcpp/waitable.hpp

Lines 158 to 161 in 10252e9

    
           RCLCPP_PUBLIC 
        
           virtual 
        
           std::shared_ptr<void> 
        
           take_data() = 0;

for reference.

mjcarroll · 2024-02-20T14:44:15Z

I haven't looked in this in too much detail, but I have to say that the use of (effectively) a void * pointer here is extremely suspicious. It doesn't give us any type checking at all.

This is standard for the executor/waitable model. The waitable is supposed to return a data handle that can be associated when the actual execute call happens. This is because a single waitable can, by design, have multiple ready executables internally

jmachowinski · 2024-02-22T08:21:35Z

This is standard for the executor/waitable model. The waitable is supposed to return a data handle that can be associated when the actual execute call happens. This is because a single waitable can, by design, have multiple ready executables internally

@mjcarroll Do you think the approach I took is viable, or should I look into something more typesafe ?
Note, this would mean, that we need to replace data in AnyExecutable by a std::variant,

rclcpp/include/rclcpp/timer.hpp

rclcpp/test/rclcpp/test_timer.cpp

mjcarroll · 2024-03-25T18:37:25Z

Linux
Linux-aarch64
Windows

rclcpp/include/rclcpp/timer.hpp

fujitatomoya · 2024-03-28T22:46:33Z

CI:

Linux
Linux-aarch64
Windows

jmachowinski · 2024-03-29T15:03:01Z

reworked patch after merge of #2142

fujitatomoya · 2024-03-29T17:16:39Z

CI (without ros2/examples#375)

Linux
Linux-aarch64
Windows

fujitatomoya · 2024-04-05T05:42:28Z

@jmachowinski probably we want to rebase this before restarting CI since some fixes are merged in a week?

fujitatomoya · 2024-04-05T15:57:49Z

CI:

Linux
Linux-aarch64
Windows

fujitatomoya · 2024-04-05T16:15:06Z

CI:

Linux
Linux-aarch64
Windows

…dd interface for a callback with TimerInfo argument Signed-off-by: Alexis Tsogias <a.tsogias@cellumation.com>

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski · 2024-04-11T09:52:04Z

Can I somehow tell the auto CI, that this PR and ros2/examples#375 belong together, and I should build using both ?

mjcarroll · 2024-04-11T13:21:55Z

The Rpr job isn't aware of cross-repo changes, but we can submit any arbitrary ros2.repos file to the other CI. Is it just those two branches that need to be done together?

jmachowinski · 2024-04-11T13:39:32Z

The Rpr job isn't aware of cross-repo changes, but we can submit any arbitrary ros2.repos file to the other CI. Is it just those two branches that need to be done together?

Yes, this and examples.

mjcarroll · 2024-04-11T13:42:12Z

Linux
Linux-aarch64
Windows

wjwwood · 2024-04-12T12:52:47Z

Had a follow up to this one: #2501

Signed-off-by: Alexis Tsogias <a.tsogias@cellumation.com> Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Alexis Tsogias <a.tsogias@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>

ros-discourse · 2024-05-23T14:29:56Z

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

https://discourse.ros.org/t/ros-2-jazzy-jalisco-released/37862/1

Signed-off-by: Alexis Tsogias <a.tsogias@cellumation.com> Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Alexis Tsogias <a.tsogias@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski requested review from ivanpauno, hidmic and wjwwood as code owners October 27, 2023 11:21

jmachowinski commented Oct 27, 2023

View reviewed changes

rclcpp/include/rclcpp/timer.hpp Outdated Show resolved Hide resolved

sloretz reviewed Nov 16, 2023

View reviewed changes

sloretz requested a review from alsora November 16, 2023 21:12

fujitatomoya reviewed Feb 19, 2024

View reviewed changes

rclcpp/include/rclcpp/timer.hpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/executors/static_single_threaded_executor.cpp Outdated Show resolved Hide resolved

rclcpp_action/src/client.cpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/timer.hpp Show resolved Hide resolved

jmachowinski force-pushed the timer_info_rolling branch 2 times, most recently from 0b4ec71 to 88d1b0e Compare February 20, 2024 13:06

mjcarroll reviewed Feb 22, 2024

View reviewed changes

rclcpp/include/rclcpp/timer.hpp Outdated Show resolved Hide resolved

wjwwood assigned mjcarroll Feb 29, 2024

jmachowinski mentioned this pull request Mar 22, 2024

fix: Fixed compilation after API change of TimerBase::execute ros2/examples#375

Merged

fujitatomoya reviewed Mar 25, 2024

View reviewed changes

rclcpp/test/rclcpp/test_timer.cpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the timer_info_rolling branch from 88d1b0e to 30a95af Compare March 25, 2024 10:35

fujitatomoya approved these changes Mar 25, 2024

View reviewed changes

alsora reviewed Mar 26, 2024

View reviewed changes

rclcpp/include/rclcpp/timer.hpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the timer_info_rolling branch from 30a95af to 5616a9c Compare March 26, 2024 10:52

jmachowinski force-pushed the timer_info_rolling branch from 5616a9c to 2bfee89 Compare March 29, 2024 15:01

jmachowinski force-pushed the timer_info_rolling branch 3 times, most recently from 0840828 to e920e72 Compare April 5, 2024 15:15

jmachowinski force-pushed the timer_info_rolling branch from e920e72 to fa754fe Compare April 5, 2024 16:12

Use rcl_timer_call_with_info instead if rcl_timer_call in Timer and a…

ca129f5

…dd interface for a callback with TimerInfo argument Signed-off-by: Alexis Tsogias <a.tsogias@cellumation.com>

jmachowinski force-pushed the timer_info_rolling branch from fa754fe to 3328d38 Compare April 10, 2024 18:30

fix: Make passing of timer data thread safe

1b3fe24

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski force-pushed the timer_info_rolling branch from 3328d38 to 1b3fe24 Compare April 10, 2024 18:38

mjcarroll merged commit 634cb87 into ros2:rolling Apr 12, 2024
2 of 3 checks passed

wjwwood mentioned this pull request Apr 12, 2024

fixup var names to snake case #2501

Merged

fujitatomoya mentioned this pull request May 14, 2024

add extended timer information of rcl/rclcpp for jazzy release note. ros2/ros2_documentation#4455

Merged

apojomovsky mentioned this pull request May 16, 2024

[Draft] Cherry pick changes from rcl#1113 irobot-ros/rcl#32

Draft

tonynajjar mentioned this pull request May 23, 2024

Warn if Timer frequency is not met ros2/rclpy#1126

Closed

fujitatomoya mentioned this pull request Jun 2, 2024

Add TimerInfo to timer callback. ros2/rclpy#1292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added optional TimerInfo to timer callback #2343

Added optional TimerInfo to timer callback #2343

jmachowinski commented Oct 27, 2023

jmachowinski commented Oct 27, 2023

sloretz Nov 16, 2023

jmachowinski Nov 16, 2023

jmachowinski commented Jan 29, 2024

fujitatomoya left a comment

jmachowinski commented Feb 20, 2024

jmachowinski commented Feb 20, 2024

clalancette commented Feb 20, 2024

jmachowinski commented Feb 20, 2024 •

edited

Loading

mjcarroll commented Feb 20, 2024

jmachowinski commented Feb 22, 2024 •

edited

Loading

mjcarroll commented Mar 25, 2024

fujitatomoya commented Mar 28, 2024

jmachowinski commented Mar 29, 2024

fujitatomoya commented Mar 29, 2024

fujitatomoya commented Apr 5, 2024

fujitatomoya commented Apr 5, 2024

fujitatomoya commented Apr 5, 2024

jmachowinski commented Apr 11, 2024

mjcarroll commented Apr 11, 2024

jmachowinski commented Apr 11, 2024

mjcarroll commented Apr 11, 2024

wjwwood commented Apr 12, 2024

ros-discourse commented May 23, 2024

Added optional TimerInfo to timer callback #2343

Added optional TimerInfo to timer callback #2343

Conversation

jmachowinski commented Oct 27, 2023

jmachowinski commented Oct 27, 2023

sloretz Nov 16, 2023

Choose a reason for hiding this comment

jmachowinski Nov 16, 2023

Choose a reason for hiding this comment

jmachowinski commented Jan 29, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

jmachowinski commented Feb 20, 2024

jmachowinski commented Feb 20, 2024

clalancette commented Feb 20, 2024

jmachowinski commented Feb 20, 2024 • edited Loading

mjcarroll commented Feb 20, 2024

jmachowinski commented Feb 22, 2024 • edited Loading

mjcarroll commented Mar 25, 2024

fujitatomoya commented Mar 28, 2024

jmachowinski commented Mar 29, 2024

fujitatomoya commented Mar 29, 2024

fujitatomoya commented Apr 5, 2024

fujitatomoya commented Apr 5, 2024

fujitatomoya commented Apr 5, 2024

jmachowinski commented Apr 11, 2024

mjcarroll commented Apr 11, 2024

jmachowinski commented Apr 11, 2024

mjcarroll commented Apr 11, 2024

wjwwood commented Apr 12, 2024

ros-discourse commented May 23, 2024

jmachowinski commented Feb 20, 2024 •

edited

Loading

jmachowinski commented Feb 22, 2024 •

edited

Loading