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

Use QoS profile of existing publishers (if available) when creating new publishers #184

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

achim-k
Copy link
Collaborator

@achim-k achim-k commented Mar 8, 2023

Public-Facing Changes

  • Use QoS profile of existing publishers (if available) when creating new publishers

Description

So far Publishers created for client-published topics were using a default QoS profile. This could lead to issues with Subscribers not receiving messages when publishing to topics such as /rosout where usually a different QoS profile is used.

See also https://foxglove.slack.com/archives/C028UEY858S/p1678224259232239

@achim-k achim-k requested a review from jtbandes March 8, 2023 13:57
@achim-k achim-k merged commit e65d6f6 into main Mar 8, 2023
@achim-k achim-k deleted the achim/publisher_qos_profile branch March 8, 2023 19:01
achim-k added a commit that referenced this pull request Mar 31, 2023
### Public-Facing Changes
- Fix publishers being created with invalid QoS profile

### Description
Since #184 we use the QoS profile of an existing publisher when creating
a new publisher for a client advertised topic. In case the existing
publisher has been destroyed before (but is still registered, see #204),
the QoS profile is invalid and creating a publisher fails with the
message `'create_publisher() failed setting topic QoS, at
./src/publisher.cpp:226'`.

This PR mitigates the issue by taking the QoS profile of an existing
publisher from a different node (if there is any). The assumption is,
that publishers of other nodes are less likely to be destroyed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants