-
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
Intelligently subscribe to topics according to their QoS profiles #355
Conversation
… just check that they are reported at all Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
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.
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?
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 Long way to say, yeah, I feel similarly, but I think this is the best move right now. |
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.
Some nits regarding the logging statements.
Does it make sense to move wait_for
to rosbag2_test_common
?
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
bc656be
to
c8f9967
Compare
@Karsten1987 the failing test on windows is |
It'll take me a bit to review this, but feel free to go ahead and merge. |
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.