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.

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

Added optional TimerInfo to timer callback #2343

Merged
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

@@ -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

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_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

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

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_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

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,

@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

@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

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

wjwwood commented Apr 12, 2024

Had a follow up to this one: #2501

apojomovsky pushed a commit to apojomovsky/rclcpp that referenced this pull request May 16, 2024
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
Copy link

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

apojomovsky pushed a commit to apojomovsky/rclcpp that referenced this pull request Jun 17, 2024
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>
apojomovsky pushed a commit to apojomovsky/rclcpp that referenced this pull request Jun 17, 2024
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>
apojomovsky pushed a commit to apojomovsky/rclcpp that referenced this pull request Jun 21, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants