feat: introduce the advanced publisher and subscriber #368

YuanYuYuan · 2024-12-31T11:18:30Z

With this PR, we replace the QueryingSubscriber and PublicationCache (+ a normal publisher)
with the AdvancedSubscriber and AdvancedPublisher.

NOTE: A deadlock is found when undeclaring advanced subscribers. It will be fixed once eclipse-zenoh/zenoh#1685 is merged and synced with zenoh-c.

YuanYuYuan · 2025-01-07T07:58:06Z

Update: The deadlock fix has been merged. Mark this PR as ready for review.

YuanYuYuan · 2025-01-08T09:32:53Z

Test Failure

ros2.repos

rclcpp_action
- 1 - test_client
rclc_parameter
- 1 - rclc_parameter_test
test_rclcpp.log
- 74 - test_n_nodes__rmw_zenoh_cpp (Failed) <--- Flaky

No new failure is found.

YuanYuYuan · 2025-01-08T15:04:04Z

Update: bump up zenoh-cpp version to include the memory leak fix eclipse-zenoh/zenoh-cpp#363.

YuanYuYuan · 2025-01-10T01:42:19Z

Will rebase once #405 is merged.

Yadunund

Please see feedback inline.
I have the following additional questions

It looks like this PR does not fix [Test Failure] rclcpp_action / test_client #326. But from my experience, it changes the behavior from deterministically failing to failing sometimes. ie, the test is now flaky. Do you observe the same?
With AdvancedSubscribers do we no longer need to call Get() when we detect an AdvancedPublisher with publication cache? This was needed to fix Subscribers sometimes not getting messages from topics using durability transient_local #263.

zenoh_cpp_vendor/CMakeLists.txt

rmw_zenoh_cpp/src/detail/graph_cache.hpp

rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp

rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp

YuanYuYuan · 2025-01-13T08:46:04Z

Please see feedback inline. I have the following additional questions

1. It looks like this PR does not fix [[Test Failure] rclcpp_action / test_client #326](https://github.com/ros2/rmw_zenoh/issues/326). But from my experience, it changes the behavior from deterministically failing to failing sometimes. ie, the test is now flaky. Do you observe the same?

2. With `AdvancedSubscribers` do we no longer need to call `Get()`  when we detect an `AdvancedPublisher` with publication cache? This was needed to fix [Subscribers sometimes not getting messages from topics using durability transient_local #263](https://github.com/ros2/rmw_zenoh/issues/263).

Here’s an improved version of your paragraph with clearer phrasing and a more professional tone:

This PR does not resolve the issue reported in [Test Failure] rclcpp_action / test_client #326. Based on the investigation, the root cause lies in the inappropriate synchronization assumptions in the test. We have also observed similar failures (slightly more often) when using the previous QueryingSubscriber implementation. To address this issue, we can either make the local publisher-to-subscriber action a blocking call or modify the test itself.
We expect the issue will be resolved. So no longer need the hack 😄

YuanYuYuan · 2025-01-13T08:48:28Z

Redid the test with the repos. No new failures were found.

rclcpp_action/test_client
rclc_parameter/rclc_parameter_test

rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp

YuanYuYuan · 2025-01-22T08:49:52Z

Now we set the advanced pub/sub to map the ROS2 QoS according to the following table.

AdvPub

Option	ROS2 QoS
publisher_detection	Durability::TransientLocal
cache	Durability::TransientLocal or Reliable
cache->max_samples	Depth
sample_miss_detection	Reliable
sample_miss_detection->heartbeat	Reliable
Z_RELIABILITY_RELIABLE	Reliable
Z_CONGESTION_CONTROL_BLOCK	Reliable & History::KeepALL

AdvSub

Option	ROS2 QoS
subscriber_detection	Durability::TransientLocal
sample_miss_detection	Durability::TransientLocal
history->detect_late_publishers	Durability::TransientLocal
history->max_samples	Depth
recovery	Reliable
recovery->last_sample_miss_detection = heartbeat	Reliable

Tested against Yadunund/ros2@87d1477. No regression was found.

Yadunund · 2025-01-23T22:55:13Z

We discussed this today and we agreed to hold merging this PR until we test the changes here on more complex applications to gauge whether the constants adopted are appropriate.

Yadunund · 2025-02-06T21:30:22Z

We discussed this again today and agreed to convert this to a draft PR until eclipse-zenoh/zenoh#1753 is merged and zenoh version here is updated. Along with this change, we will also be disabling recovery and sample missed options as it does not fit the typical ROS use case.

YuanYuYuan · 2025-02-07T07:45:22Z

The recovery option has been removed in 0e4b99d

JEnoch · 2025-02-12T09:53:03Z

I created #457 to address the possible usage of recovery and sample_miss_detection options.
Those options are only required to benefit end-to-end reliability.
They are not required to implement TRANSIENT_LOCAL QoS which is the goal of this PR, as a replacement of the deprecated QueryingSubscriber and Publication Cache.

As soon as #456 (that includes eclipse-zenoh/zenoh#1753) is merged, I think this PR is good to go.

Yadunund

Tested with latest zenoh bump and I can confirm that rclcpp_action/test_client passes 🎉

However, I'm concerned with one change which i've flagged below.

rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp

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

Yadunund

LGTM! Thanks for iterating on this PR to find the right fix! We can finally close out #326

Yadunund · 2025-02-18T19:46:14Z

@Mergifyio backport jazzy humble

mergify · 2025-02-18T19:46:45Z

backport jazzy humble

✅ Backports have been created

#469 feat: introduce the advanced publisher and subscriber (backport #368) has been created for branch jazzy but encountered conflicts
#470 feat: introduce the advanced publisher and subscriber (backport #368) has been created for branch humble but encountered conflicts

* feat: introduce the advanced publisher/subscriber * chore: ament_uncrustify and ament_cpplint * trigger CI * refactor: remove the unneeded querying callbacks * fix: reorder the advanced pub/sub options to match the ROS2 QoS * chore: add tmp * chore: apply the QoS mapping * style: ament_cpplint + ament_uncrustify * trigger CI * refactor: remove the recovery option as it exceeds typical ROS use cases * chore: remove the QoS for end-to-end reliability * style: ament_uncrustify * Cleanup Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Co-authored-by: Yadunund <[email protected]> (cherry picked from commit 77561d8) # Conflicts: # rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp

…#469) * feat: introduce the advanced publisher and subscriber (#368) * feat: introduce the advanced publisher/subscriber * chore: ament_uncrustify and ament_cpplint * trigger CI * refactor: remove the unneeded querying callbacks * fix: reorder the advanced pub/sub options to match the ROS2 QoS * chore: add tmp * chore: apply the QoS mapping * style: ament_cpplint + ament_uncrustify * trigger CI * refactor: remove the recovery option as it exceeds typical ROS use cases * chore: remove the QoS for end-to-end reliability * style: ament_uncrustify * Cleanup Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Co-authored-by: Yadunund <[email protected]> (cherry picked from commit 77561d8) # Conflicts: # rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp * Fixed merge Signed-off-by: Alejandro Hernandez Cordero <[email protected]> --------- Signed-off-by: Alejandro Hernandez Cordero <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> Co-authored-by: Alejandro Hernandez Cordero <[email protected]>

YuanYuYuan mentioned this pull request Dec 31, 2024

[Test Failure] rclcpp_action / test_client #326

Closed

clalancette marked this pull request as draft January 2, 2025 16:09

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from e0c490f to 1f9357f Compare January 7, 2025 07:57

YuanYuYuan marked this pull request as ready for review January 7, 2025 07:58

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from 08b0906 to b76ee4c Compare January 11, 2025 17:05

Yadunund requested changes Jan 11, 2025

View reviewed changes

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from b76ee4c to 9cc776b Compare January 13, 2025 08:28

Yadunund reviewed Jan 14, 2025

View reviewed changes

rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp Show resolved Hide resolved

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from 9cc776b to 9f72ee8 Compare January 22, 2025 08:37

Yadunund self-assigned this Feb 6, 2025

Yadunund marked this pull request as draft February 6, 2025 21:28

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from c199a39 to 60042bf Compare February 7, 2025 07:40

Yadunund mentioned this pull request Feb 7, 2025

qos :2:,10:,:,:,, not found in for topic type rcl_interfaces::srv::dds_::SetParametersAtomically_. Report this. #455

Closed

This was referenced Feb 11, 2025

Bump Zenoh to commit id 3bbf6af (1.2.1 + few commits) #456

Merged

Make end-to-end reliability configurable #457

Open

This was referenced Feb 13, 2025

Bump Zenoh to commit id 3bbf6af (1.2.1 + few commits) (backport #456) #461

Merged

Bump Zenoh to commit id 3bbf6af (1.2.1 + few commits) (backport #456) #462

Merged

Yadunund marked this pull request as ready for review February 13, 2025 16:45

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from 0e4b99d to df169c8 Compare February 13, 2025 16:46

Yadunund requested changes Feb 14, 2025

View reviewed changes

rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp Outdated Show resolved Hide resolved

YuanYuYuan added 11 commits February 17, 2025 15:02

feat: introduce the advanced publisher/subscriber

726546d

chore: ament_uncrustify and ament_cpplint

1f58b3e

trigger CI

73898be

refactor: remove the unneeded querying callbacks

1265a9a

fix: reorder the advanced pub/sub options to match the ROS2 QoS

8957900

chore: add tmp

b0bdcb6

chore: apply the QoS mapping

ced3319

style: ament_cpplint + ament_uncrustify

1fc8c69

trigger CI

5dc1df8

refactor: remove the recovery option as it exceeds typical ROS use cases

61a34e4

chore: remove the QoS for end-to-end reliability

6068096

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from ab8a7aa to 6068096 Compare February 17, 2025 07:04

YuanYuYuan and others added 2 commits February 17, 2025 15:06

style: ament_uncrustify

b0ebb95

Cleanup

b1b2dd8

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

Yadunund force-pushed the feat/advanced-pub-sub branch from 15d522a to b1b2dd8 Compare February 18, 2025 19:25

Yadunund approved these changes Feb 18, 2025

View reviewed changes

Yadunund merged commit 77561d8 into ros2:rolling Feb 18, 2025
5 checks passed

This was referenced Feb 18, 2025

feat: introduce the advanced publisher and subscriber (backport #368) #469

Merged

feat: introduce the advanced publisher and subscriber (backport #368) #470

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce the advanced publisher and subscriber #368

feat: introduce the advanced publisher and subscriber #368

YuanYuYuan commented Dec 31, 2024

YuanYuYuan commented Jan 7, 2025

YuanYuYuan commented Jan 8, 2025 •

edited

Loading

YuanYuYuan commented Jan 8, 2025

YuanYuYuan commented Jan 10, 2025

Yadunund left a comment

YuanYuYuan commented Jan 13, 2025 •

edited

Loading

YuanYuYuan commented Jan 13, 2025 •

edited

Loading

YuanYuYuan commented Jan 22, 2025 •

edited

Loading

Yadunund commented Jan 23, 2025

Yadunund commented Feb 6, 2025

YuanYuYuan commented Feb 7, 2025

JEnoch commented Feb 12, 2025

Yadunund left a comment

Yadunund left a comment

Yadunund commented Feb 18, 2025

mergify bot commented Feb 18, 2025 •

edited

Loading

feat: introduce the advanced publisher and subscriber #368

feat: introduce the advanced publisher and subscriber #368

Conversation

YuanYuYuan commented Dec 31, 2024

YuanYuYuan commented Jan 7, 2025

YuanYuYuan commented Jan 8, 2025 • edited Loading

Test Failure

YuanYuYuan commented Jan 8, 2025

YuanYuYuan commented Jan 10, 2025

Yadunund left a comment

Choose a reason for hiding this comment

YuanYuYuan commented Jan 13, 2025 • edited Loading

YuanYuYuan commented Jan 13, 2025 • edited Loading

YuanYuYuan commented Jan 22, 2025 • edited Loading

Yadunund commented Jan 23, 2025

Yadunund commented Feb 6, 2025

YuanYuYuan commented Feb 7, 2025

JEnoch commented Feb 12, 2025

Yadunund left a comment

Choose a reason for hiding this comment

Yadunund left a comment

Choose a reason for hiding this comment

Yadunund commented Feb 18, 2025

mergify bot commented Feb 18, 2025 • edited Loading

✅ Backports have been created

YuanYuYuan commented Jan 8, 2025 •

edited

Loading

YuanYuYuan commented Jan 13, 2025 •

edited

Loading

YuanYuYuan commented Jan 13, 2025 •

edited

Loading

YuanYuYuan commented Jan 22, 2025 •

edited

Loading

mergify bot commented Feb 18, 2025 •

edited

Loading