Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added optional TimerInfo to timer callback #2343

Merged
merged 2 commits into from Apr 12, 2024
Merged

Added optional TimerInfo to timer callback #2343

merged 2 commits into from Apr 12, 2024

Conversation

jmachowinski
Copy link
Contributor

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
Copy link
Contributor Author

@clalancette : ping

sloretz
sloretz reviewed Nov 16, 2023
View reviewed changes
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this pointer be the actual type instead of void?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@sloretz sloretz requested a review from alsora November 16, 2023 21:12
@jmachowinski
Copy link
Contributor Author

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

fujitatomoya
fujitatomoya reviewed Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

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

besides, test would be ideal.

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
Copy link
Contributor Author

@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 jmachowinski force-pushed the timer_info_rolling branch 2 times, most recently from 0b4ec71 to 88d1b0e Compare February 20, 2024 13:06
@jmachowinski
Copy link
Contributor Author

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

@clalancette
Copy link
Contributor

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
Copy link
Contributor Author

jmachowinski commented Feb 20, 2024 •
edited

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
Copy link
Member

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
Copy link
Contributor Author

jmachowinski commented Feb 22, 2024 •
edited

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,

@jmachowinski jmachowinski mentioned this pull request Mar 22, 2024
Merged
@mjcarroll
Copy link
Member

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

@fujitatomoya
Copy link
Collaborator

CI:

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

@jmachowinski
Copy link
Contributor Author

reworked patch after merge of #2142

@fujitatomoya
Copy link
Collaborator

CI (without ros2/examples#375)

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

@clalancette
Copy link
Contributor

I'm going to suggest we pause on these types of changes for the moment. We have regressions right now that we need to track down, possibly related to either ros2/rcl#1135 or #2142.

See * Linux Build Status

@fujitatomoya fujitatomoya mentioned this pull request Mar 29, 2024
Open
@fujitatomoya
Copy link
Collaborator

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

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

CI:

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

@fujitatomoya
Copy link
Collaborator

CI:

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

@atsogias
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>
fix: Make passing of timer data thread safe
1b3fe24 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski
Copy link
Contributor Author

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

@mjcarroll
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@mjcarroll mjcarroll merged commit 634cb87 into ros2:rolling Apr 12, 2024
2 of 3 checks passed
@wjwwood wjwwood mentioned this pull request Apr 12, 2024
Merged
@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2024

Had a follow up to this one: #2501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll left review comments

@sloretz sloretz sloretz left review comments

@alsora alsora alsora left review comments

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees

@mjcarroll mjcarroll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jmachowinski @clalancette @mjcarroll @fujitatomoya @wjwwood @sloretz @alsora @atsogias