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

Bugfix: Recorder discovery does not restart after being stopped #1894

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

oysstu
Copy link

@oysstu oysstu commented Jan 9, 2025

Repeated calls to start_discovery should not start additional topics discovery tasks without stop_discovery calls.

This was originally reported in #1893, as discovery task would not be spawned if a previous stop_discovery call was made. This only works currently because the atomic is initialized to false, thus making the "correct" choice on the first call after construction.

Repeated calls to start_discovery should not start additional topics discovery tasks without stop_discovery calls

Signed-off-by: Øystein Sture <[email protected]>
@oysstu
Copy link
Author

oysstu commented Jan 9, 2025

The can_record_again_after_stop is failing because it records one more message than expected. If discovery now works between the record calls, it makes sense that it records something else as well, e.g., rosout?

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@oysstu thank you for PR and attempt to fix a wrong behavior.
The naming is hard! It seems the root cause of the issue is in unclear naming fo the variable stop_discovery_.
The logic would be more readable and much easy to understand if stop_discovery_ would be renamed to the discovery_runing_.

Could you please do such rename in the code and change conditions accordingly?

@MichaelOrlov MichaelOrlov changed the title Recorder: invert start discovery condition (fixes #1893) Bugfix: Recorder discovery does not restart after being stopped Jan 10, 2025
@oysstu
Copy link
Author

oysstu commented Jan 10, 2025

I've made the change, that should clarify the logic a lot. Any thoughts on the failing tests?

Edit: seemed to pass tests this time

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.

2 participants