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

Change default QoSProfile for pub #653

Merged
merged 6 commits into from
Sep 1, 2021
Merged

Conversation

gonzodepedro
Copy link
Contributor

Signed-off-by: Gonzalo de Pedro gonzalo@depedro.com.ar

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
@gonzodepedro
Copy link
Contributor Author

This PR uses ACTION_STATUS_DEFAULT which sets "Reliable" reliability and "Transient-local" durability.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

It seems a bit awkward to choose the "action status" preset. How about we don't set a default QoS profile for the --qos-profile CLI arg? We can instead use some custom QoS profile, unless the user explicitly passes the --qos-profile option.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

just a quick question, is there any requirement or actual problems to reset the default?

@jacobperron
Copy link
Member

just a quick question, is there any requirement or actual problems to reset the default?

While addressing #594 for ros2 topic echo, we also reviewed what QoS was being used for ros2 topic pub. Most users won't notice QoS matching issues with the "system default" QoS profile, though it does not guarantee that QoS will always match since it is system dependent. Also, chances are the default durability setting is volatile, which won't match transient_local subscriptions. By making ros2 topic pub reliable and transient_local by default, we provide the most flexible QoS.

As an aside, "system default" was chosen arbitrarily: #245

@fujitatomoya Does that answer your question?

@fujitatomoya
Copy link
Collaborator

@jacobperron

it does. thanks for the explanation.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM, I have a few minor comments

ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Gonzalo de Pedro added 2 commits August 30, 2021 15:18
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM with green CI.

I would recommend using --packages-up-to ros2topic for building and --packages-select ros2topic for testing.

ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
@gonzodepedro
Copy link
Contributor Author

CI:

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

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

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