-
Notifications
You must be signed in to change notification settings - Fork 417
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
[timer] Fix malfunction when reset cancelled timer #1013
Conversation
Signed-off-by: Donghee Ye <donghee.ye@samsung.com>
@DongheeYe Thanks for opening the PR.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment #1013 (comment).
@ivanpauno Thank to comments. I'd considered to use the rcl timer guard condition, either. (here). However, I thought, the purpose of current rcl timer guard condition is to wake up of waitset when waiting timer is expired. In this case, cancelled timer guard condition is not added to the node waitset rcl_wait. So, additional guard condition is needed to wake up node waitset to add timer waitset again like. I will try to make a rcl patch that add additional guard condition to rcl timer and trigger when rcl_timer_reset. |
I think you can change the logic to make sure is always added, instead of adding a new guard condition. |
Interrupting all wait set associated with the context is overkill. It should only interrupt the wait set/executor of which the timer is associated. Pub/sub accomplish this by being lazily removed when the executor notices that the publisher or subscription have gone out of scope (it stores them as weak pointers), but they don't have a "reset" like function. I think the best thing to do would be to get the node's node_base_interface and trigger the guard condition there, like rclcpp/rclcpp/src/rclcpp/node_interfaces/node_timers.cpp Lines 42 to 46 in 96ebf59
You could add a method to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment for how it should change.
Closed in favor of ros2/rcl#589 (I added the reviewers here by error, the intention was to add them to the rcl PR). |
This comment is still valid. Can you open a PR proposing that? |
* Increase test timeout and decrease msgs sent Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com> * Add debug instrumentation for `test_play_services` - Increased service_call_timeout_ up to 3 seconds. - Split `toggle_paused` on two separate tests `is_paused` and `toggle_paused` - Add output of the function name and line number in case of failure. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Restore msgs_to_publish Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com> * Fix for windows build Add redefinition for __PRETTY_FUNCTION__ to __FUNCSIG__ if it is not defined and not gcc compiler. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Donghee Ye donghee.ye@samsung.com
Related to #1012
To reset wait_set after reset a cancelled timer, there is needed node handle or context handle. When constructing a timer, there are context handle parameter. To trigger guard condition to reset wait_set, save context handle in constructor and trigger when reset timer.