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.

Sign up for GitHub

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

Jump to bottom

Allow GenericPublisher / GenericSubscription to take a QoS profile #337

Merged
merged 2 commits into from Mar 31, 2020
Merged

Allow GenericPublisher / GenericSubscription to take a QoS profile #337

merged 2 commits into from Mar 31, 2020

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Mar 30, 2020 •
edited

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:

@emersonknapp emersonknapp force-pushed the emersonknapp/generic-pubsub-qos branch from 72f78ff to 7b9ef99 Compare March 30, 2020 23:08
thomas-moulard
thomas-moulard reviewed Mar 30, 2020
View reviewed changes
Copy link
Contributor

@thomas-moulard thomas-moulard left a 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/CMakeLists.txt Show resolved Hide resolved
rosbag2_transport/CMakeLists.txt Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/generic_publisher.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/generic_subscription.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_rosbag2_node.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_rosbag2_node.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_rosbag2_node.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_rosbag2_node.cpp Show resolved Hide resolved
zmichaels11
zmichaels11 reviewed Mar 30, 2020
View reviewed changes
Copy link
Contributor

@zmichaels11 zmichaels11 left a comment •
edited

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
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_rosbag2_node.cpp Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_rosbag2_node.cpp Outdated Show resolved Hide resolved
@emersonknapp
Add QoS to the GenericSubscription / GenericPublisher APIs
8affccf 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/generic-pubsub-qos branch from 79f12e3 to 8affccf Compare March 31, 2020 02:13
@emersonknapp
Make uncrustify happy again
30e5d1a 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@Karsten1987 Karsten1987 mentioned this pull request Mar 31, 2020
Closed
21 tasks
thomas-moulard
thomas-moulard approved these changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

💯

@emersonknapp emersonknapp merged commit c1302c1 into ros2:master Mar 31, 2020
emersonknapp added a commit that referenced this pull request Mar 31, 2020
@emersonknapp
Revert "Allow GenericPublisher / GenericSubscription to take a QoS pr…
b809014 
…ofile (#337)"

This reverts commit c1302c1.
@emersonknapp emersonknapp mentioned this pull request Mar 31, 2020
Merged
emersonknapp added a commit that referenced this pull request Mar 31, 2020
@emersonknapp
Revert "Allow GenericPublisher / GenericSubscription to take a QoS pr…
bcfc4b2 
…ofile (#337)"

This reverts commit c1302c1.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
emersonknapp added a commit that referenced this pull request Mar 31, 2020
@emersonknapp
Revert "Allow GenericPublisher / GenericSubscription to take a QoS pr…
9a0d3a6 
…ofile (#337)" (#339)

This reverts commit c1302c1.
@emersonknapp emersonknapp mentioned this pull request Mar 31, 2020
Merged
emersonknapp added a commit that referenced this pull request Mar 31, 2020
@emersonknapp
Allow GenericPublisher / GenericSubscription to take a QoS profile (r…
d15dd97 
…e-do #337) (#340)

* Add QoS to the GenericSubscription / GenericPublisher APIs

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

@mm318 mm318 Awaiting requested review from mm318

@piraka9011 piraka9011 Awaiting requested review from piraka9011

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@emersonknapp @thomas-moulard @Karsten1987 @mm318 @zmichaels11 @piraka9011