-
Notifications
You must be signed in to change notification settings - Fork 253
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
Allow GenericPublisher / GenericSubscription to take a QoS profile #337
Allow GenericPublisher / GenericSubscription to take a QoS profile #337
Conversation
72f78ff
to
7b9ef99
Compare
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.
Let's remove the sleep from the tests, those won't work reliably.
rosbag2_transport/src/rosbag2_transport/generic_subscription.cpp
Outdated
Show resolved
Hide resolved
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.
Left a few comments on your code.
rosbag2_transport/src/rosbag2_transport/generic_subscription.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Emerson Knapp <[email protected]>
79f12e3
to
8affccf
Compare
Signed-off-by: Emerson Knapp <[email protected]>
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.
💯
…ofile (#337)" This reverts commit c1302c1. Signed-off-by: Emerson Knapp <[email protected]>
…e-do #337) (#340) * Add QoS to the GenericSubscription / GenericPublisher APIs Signed-off-by: Emerson Knapp <[email protected]>
Part of #125
Part of https://github.com/ros-tooling/aws-roadmap/issues/216
This PR adds QoS profiles to the GenericSubscription / GenericPublisher internal API, so that they do not always use the default. It continues to set the default explicitly, so behavior is not yet changed.
Note on testing: I validated that both added tests fail before the rest of the change is introduced. The reason I added the timeout is that the failing case waits the full 60 second rostest timeout otherwise, which is very overkill in all cases, for passing 2 messages.
The followups after this change will be:
create_generic_subscription
for recording behavior.create_generic_publisher
for playback behavior