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

Intelligently subscribe to topics according to their QoS profiles #355

Merged
merged 18 commits into from
Apr 8, 2020

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 7, 2020

Part of #125
Should probably go after #346

This PR handles subscribing to topics intelligently (across all rmw implementations), even if there are slightly different QoS profiles on offer. We still record offered profiles in full for completeness of information, but we do not test against their values for most policies because the different rmw implementations do not yet consistently report all values the same.

Only try to adapt to Reliability and Durability, because they are the only policies that actually affect delivery of messages. Since they each have two values, a "stricter" and "looser" value, we subscribe using the strict value if all publishers offer it, otherwise we fall back to the looser value.

History and Lifespan do not affect compatibility at all (history is purely local to both publishers and subscriptions, lifespan is local to publishers only and doesn't do anything on subscriptions), so we ignore them and use sensible defaults for rosbag2 use case.

Deadline and Liveliness don't affect delivery of messages, their purpose is to generate callbacks when the contract is broken. Since we do not record these callbacks, we can always use the loosest request and be compatible with all publishers on these policies.

This PR allows rosbag2 to now receive latched messages, and subscribe to Best Effort topics.

… just check that they are reported at all

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
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.

This LGTM. I'm not 100% satisfied with this approach because I feel that your ad-hoc logic could be drastically simplified by defining an order no QoS settings (possibly a partial order?). This order could just be defined for cases where QoS settings need to be reconciled like here, but I'm not sure if we can build such order. WDYT?

@emersonknapp
Copy link
Collaborator Author

I agree it feels a little ad hoc, but I think it solves the problem robustly for the Foxy feature set, in not very much code.

It should be possible to define an internal ordering for each individual QoS policy that ROS 2 exposes, which could provide a more programmatic way to construct an adapted QoS profile. Ideally it would be defined in or near the rmw layer, where the APIs around this behavior are defined. For now, given time constraints, I think we are better off defining this here, as is, rather than building the more general solution. The rmw API specification needs some refining to make sure that the assumptions we would be making in those utilities are obligated by the API contract - right now there are inconsistencies in reporting from the different rmw implementations that we are not in a good position to track down before the distro release.

Long way to say, yeah, I feel similarly, but I think this is the best move right now.

@emersonknapp
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

Some nits regarding the logging statements.
Does it make sense to move wait_for to rosbag2_test_common?

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/adaptive-subscribe branch from bc656be to c8f9967 Compare April 7, 2020 22:34
@emersonknapp
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Collaborator Author

@Karsten1987 the failing test on windows is record_end_to_end_test with a 60s timeout. My assessment is that the test is flaky on windows, the part of the test that waits is around metadata file creation, which is not touched here. There has been a backup on the windows CI executors all day, what are your thoughts on merging this?

@Karsten1987
Copy link
Collaborator

It'll take me a bit to review this, but feel free to go ahead and merge.
I don't think the test is flaky, it's legit broken given that it constantly fails.

@emersonknapp emersonknapp merged commit 619fa6f into master Apr 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/adaptive-subscribe branch April 8, 2020 00:10
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.

4 participants