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

Fixed multiple subscriber on transient_local topic #723

Merged
merged 2 commits into from
Apr 22, 2022
Merged

Fixed multiple subscriber on transient_local topic #723

merged 2 commits into from
Apr 22, 2022

Conversation

p0rys
Copy link
Contributor

@p0rys p0rys commented Mar 12, 2022

Public API Changes
None

Description
Problem: Only the first subscription to a transient_local topic does receive the "latched" data.

The MultiSubscriber class creates a new subscription to initially receive the "latched" data for each new client. After this subscription returns with a message the client is shifted over to the "real" subscription and the other one is destroyed. Because this intermittent subscribers does not use the same QoS settings as the real one it will never receive latched data from transient_local topics except there is send new data from the publisher.

Fix: This PR adds the QoS settings as a global variable to the MultiSubscriber class and uses it when creating intermittent subscribers for new clients. With the correct settings these will directly get back with the latched data.

The transient_local_publisher test is also changed to subscribe three times to cover this case.

@p0rys
Copy link
Contributor Author

p0rys commented Mar 22, 2022

Rolling CI see #725

@defunctzombie
Copy link
Contributor

@mvollrath @MatthijsBurgh does this change looks reasonable to you? It looks 👍 to me.

Copy link
Contributor

@MatthijsBurgh MatthijsBurgh left a comment

Choose a reason for hiding this comment

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

I am not familiar with ROS2. So I can only judge the syntax, which looks fine.

@jtbandes jtbandes merged commit b4b996b into RobotWebTools:ros2 Apr 22, 2022
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

4 participants