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

Remove assumptions and checks around QoS policies that don't affect compatibility #350

Closed

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 3, 2020

Part of #125
Depends on ros2/rclcpp#1049

Some of the policies in QoS profiles don't affect compatibility, only local behavior, so it's not worth comparing those values. The different RMW implementations report these values inconsistently currently, so though it is useful to record everything, it is not meaningful to test for specific values.

@thomas-moulard
Copy link
Contributor

I think keeping a trace of how things have been recorded is valuable for debugging later. If we have this feature already, I find it a bit counter-productive to throw this data away.

Example use case: "rosbag2 has troubles recording everything, and is dropping messages? oh yeah the history depth was set to 1 erroneously, let's change this behavior". I'm not sure if we already have a rosbag2 info like in ros1, but that's something we could pretty print there.

I understand that some RMW implementations may not be "there yet" in terms of reporting data correctly, but we should continue pushing them to get better, instead of trying to workaround their quirks, unless there is a clear loss for our customers.

I think what's missing maybe from your PR message, is why you're doing this? What problem are you fixing here for users?

@emersonknapp emersonknapp changed the title Only serialize QoS policies that affect compatibility Remove testing assumptions about QoS policies that don affect compatibility Apr 3, 2020
@emersonknapp emersonknapp changed the title Remove testing assumptions about QoS policies that don affect compatibility Remove testing assumptions about QoS policies that don't affect compatibility Apr 3, 2020
@emersonknapp emersonknapp force-pushed the emersonknapp/yaml-qos-only-compatibility-policies branch from e75ce3e to aa7ea23 Compare April 3, 2020 20:44
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 3, 2020

@thomas-moulard yeah I think you're right. My main problem had to do with checking for equality. The real solution here is not to check for exact values, but continue recording them as yes, they are potentially-useful information. I've rewritten the PR to reflect this new approach.

… 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]>
@emersonknapp emersonknapp force-pushed the emersonknapp/yaml-qos-only-compatibility-policies branch from 61e892c to 7bdab8a Compare April 4, 2020 00:15
@emersonknapp emersonknapp changed the title Remove testing assumptions about QoS policies that don't affect compatibility Remove assumptions and checks around QoS policies that don't affect compatibility Apr 4, 2020
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 6, 2020

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

@emersonknapp
Copy link
Collaborator Author

I am closing this PR in favor of a more complete approach that will cause better behavior on average (with a more accurate branch name)

@emersonknapp emersonknapp deleted the emersonknapp/yaml-qos-only-compatibility-policies branch April 20, 2020 21:46
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