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

cancel timer before letting caller use the node to avoid spurious wakeups for consumers #115

Merged
merged 2 commits into from
Jun 26, 2018

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Jun 26, 2018

@dirk-thomas Is this timer intended to stay alive and be used by consumers?

Otherwise I can update this PR to destroy the timer before exiting the init function

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Jun 26, 2018
@wjwwood wjwwood changed the title cancel timer before letting caller use the node to avoid spurious wak… cancel timer before letting caller use the node to avoid spurious wakeups for consumers Jun 26, 2018
@dirk-thomas
Copy link
Member

I don't think the timer is used anywhere. It could be made "internal" to the constructor and be destroyed immediately.

@mikaelarguedas
Copy link
Member Author

Sounds good updated in cbe14dd

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 26, 2018
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

lgtm

(Since the ticket doesn't mention any reproducible example I didn't try the patch.)

@mikaelarguedas
Copy link
Member Author

It can be reproduced by adding a print in the timer callback.

         def timer_callback():
             nonlocal timeout_reached
+            print('timer_cb')
             timeout_reached = True

In one terminal run: ros2 run demo_nodes_cpp talker
In the second one run: ros2 topic echo /chatter std_msgs/String

Behavior without the patch

$ ros2 topic echo /chatter std_msgs/String
timer_cb
timer_cb
timer_cb
timer_cb
timer_cb
timer_cb
timer_cb
timer_cb
data: 'Hello World: 1'

timer_cb
timer_cb
data: 'Hello World: 2'

timer_cb
timer_cb
data: 'Hello World: 3'

timer_cb
timer_cb
data: 'Hello World: 4'

timer_cb
timer_cb

Behavior with the patch

$ ros2 topic echo /chatter std_msgs/String
timer_cb
data: 'Hello World: 1'

data: 'Hello World: 2'

data: 'Hello World: 3'

data: 'Hello World: 4'

@mikaelarguedas mikaelarguedas merged commit e5f67d0 into master Jun 26, 2018
@mikaelarguedas mikaelarguedas deleted the cancel_timer_after_init branch June 26, 2018 03:19
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Jun 26, 2018
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
fixes ros2#115

Signed-off-by: William Woodall <william@osrfoundation.org>
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.

None yet

2 participants