Assume publisher qos depth of 1 if the middleware reports the qos history as unknown #239
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…epth Some RMWs do not retrieve history information of the publisher endpoint in which case the history depth is 0. We use a lower limit of 1 here, so that the history depth is at least equal to the number of publishers. This covers the case where there are multiple transient_local publishers with a depth of 1 (e.g. multiple tf_static transform broadcasters). See also #238 and #208
achim-k
changed the title
jtbandes
reviewed
Jul 5, 2023
| // Set up text message handler to resolve the promise when the topic is advertised | ||
| foxglove::Client<websocketpp::config::asio_client> wsClient; | ||
| auto wsClient = std::make_shared<foxglove::Client<websocketpp::config::asio_client>>(); |
There was a problem hiding this comment.
Why does this need to be a shared_ptr? Would unique_ptr work? I don't see the ownership being actually shared anywhere?
There was a problem hiding this comment.
You are right, will see what I can do. Can just pass a raw pointer to waitForChannel then
jtbandes
reviewed
Jul 5, 2023
| @@ -118,6 +112,30 @@ std::future<Service> waitForService(std::shared_ptr<ClientInterface> client, | |||
| return future; | |||
| } | |||
|
|
|||
| std::future<Channel> waitForChannel(std::shared_ptr<ClientInterface> client, | |||
| const std::string& topicName) { | |||
| auto promise = std::make_shared<std::promise<Channel>>(); | |||
There was a problem hiding this comment.
Why shared_ptr? The std::promise example code seems to allocate a promise on the stack and still move it into a thread or lambda.
There was a problem hiding this comment.
I remember that John and I had some issues with that but don't remember exactly why. Will see what I can do once I created a new release
jtbandes
approved these changes
Jul 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Public-Facing Changes
Assume publisher qos depth of 1 if the middleware reports the qos history as unknown
Description
Some RMWs do not report history information of a publisher endpoint in which case the history depth is reported as 0. We internally use this depth to calculate an appropriate depth for the subscription, which uses the
KeepLast(depth)history policy.In case a publisher's history depth is reported as 0, we assume a depth of 1 so that the final history depth is at least equal to the number of publishers. This covers the case where there are multiple transient_local publishers with a depth of 1 (e.g. multiple tf_static transform broadcasters). Before this PR, a user would have to manually specify a
min_qos_depthof N when having N tf_static broadcasters.This PR also increases the max qos depth default value, 10 seemed very low. I'm not sure anymore why we added an upper limit in the first place. Additionally, a warning is logged when the upper limit is reached.
Fixes #238
See also #208