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

Querying subs to query newly discovered publishers with publication caches #269

Merged
merged 12 commits into from
Aug 30, 2024

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Aug 26, 2024

Fix #263

This PR:

  • Renames Entity::keyexpr() to Entity::liveliness_keyexpr() to disambiguate between keyexpressions used for liveliness tokens and keyexpressions used for topic/service names.
  • Adds topic_keyexpr string to TopicInfo stored within Entity. This way, entities constructed within GraphCache are aware of they topic keyexpressions.
  • Adds an API to register callbacks to trigger when a Publisher with a PublicationCache is detected in the graph.
  • Updates rmw_create_subscription to register a callback as described above that will call ze_querying_subscriber_get over the keyexpression. This requires the zenoh session ID as the "selector" to uniquely identify which PublicationCache to query.

@Yadunund Yadunund force-pushed the yadu/query-new-pub-caches branch from d0f7f43 to 2313943 Compare August 26, 2024 06:50
@Yadunund
Copy link
Member Author

Yadunund commented Aug 26, 2024

This PR will likely remain in draft until we migrate to Zenoh 1.0.0. f4dc8ea set query_accept_replies to ZCU_REPLY_KEYEXPR_ANY but this option is passed at QueryingSubscriber creation and is used only for the initial query. It's not used for subsequent calls to ze_querying_subscriber_get which is what we need to fix the original problem.
The ability to pass this option to the ze_querying_subscriber_get() call that is triggered in the callback will only land in 1.0.0.

@Yadunund
Copy link
Member Author

I've temporarily merged changes from eclipse-zenoh/zenoh-c#620 and I can confirm this PR now fixes the original issue. Once upstream eclipse-zenoh/zenoh-c is updated, we can re-update the vendored zenoh-c commit hash before merging. In the meanwhile @clalancette do you mind reviewing the changes?

@Yadunund Yadunund force-pushed the yadu/query-new-pub-caches branch from 6ee5f03 to 91df705 Compare August 28, 2024 02:25
@Yadunund Yadunund marked this pull request as ready for review August 28, 2024 02:27
@Yadunund Yadunund requested a review from clalancette August 28, 2024 02:27
@Yadunund
Copy link
Member Author

The API in zenoh-c has been fixed and i've updated the commit hash accordingly. This PR is ready for review.

@Yadunund Yadunund force-pushed the yadu/query-new-pub-caches branch from 91df705 to 4188ea1 Compare August 28, 2024 02:37
@Yadunund
Copy link
Member Author

Hmm i've been unable to successfully run the nav2 demo with this PR. More specifically, the costmap does not show up in RViz and i'm unable to localize the robot. Digging into whether the problem is caused by the changes in the code here or perhaps a regression somewhere in zenoh-c between the old commit and the new one.

@Yadunund
Copy link
Member Author

Yadunund commented Aug 28, 2024

I've narrowed the problem to this line that this PR added

pub_cache_opts.queryable_prefix = z_loan(queryable_prefix);

If I comment out the line, running ros2 launch nav2_bringup tb3_simulation.launch.py headless:=False is able to successfully start up

image

Else I'm seeing

image

But the whole point of the bug fix is to include the queryable_prefix option to the publication cache. Perhaps some other options are not set correctly?

cc @JEnoch @imstevenpmwork @YuanYuYuan

@JEnoch
Copy link
Contributor

JEnoch commented Aug 28, 2024

@Yadunund this PR fixes this issue: #274

TL;DR: the QueryingSubscriber performs an initial query that must now be adapted following the addition of zid as a prefix in PublicationCache's queryable.
Detailed explanation in the PR.

…fix (#274)

* Adapt the QueryingSubscriber's initial query to the new queryable_prefix

* fix uncrustify issue

* Make choice of query_target explicit

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>
Co-authored-by: Yadunund <[email protected]>
@Yadunund
Copy link
Member Author

@Yadunund this PR fixes this issue: #274

TL;DR: the QueryingSubscriber performs an initial query that must now be adapted following the addition of zid as a prefix in PublicationCache's queryable. Detailed explanation in the PR.

Thanks for the fix! I've merged it into this PR. This is now ready for another round of review.

rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left one minor thing to improve. Once that is done, and @ahcorde 's comments are addressed, I think this will be good to go.

rmw_zenoh_cpp/src/detail/graph_cache.cpp Outdated Show resolved Hide resolved
Yadunund and others added 2 commits August 30, 2024 14:37
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund requested a review from ahcorde August 30, 2024 10:27
@ahcorde ahcorde requested a review from clalancette August 30, 2024 11:57
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for all of the changes here! Looks good to me.

@clalancette clalancette merged commit 60b72f0 into rolling Aug 30, 2024
8 checks passed
@clalancette clalancette deleted the yadu/query-new-pub-caches branch August 30, 2024 11:58
@Yadunund Yadunund mentioned this pull request Aug 31, 2024
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.

Subscribers sometimes not getting messages from topics using durability transient_local
5 participants