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

Support basic QoS settings #91

Merged
merged 36 commits into from
Feb 7, 2024
Merged

Support basic QoS settings #91

merged 36 commits into from
Feb 7, 2024

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Jan 11, 2024

# TODO

  • Check whether PubCache can always be created or only if TRANSIENT_LOCAL qos
  • Set Zenoh congestion appropriately. (Default Drop except with RELIABLE and KEEP_ALL).
    - [ ] Call ze_querying_subscriber_get() when a TRANSIENT_LOCAL publisher is discovered with matching endpoints.
  • Update QoS compatibility checking methods.

This PR provides support for the following QoS settings:

  • Depth
  • History (KEEP_ALL andKEEP_LAST)
  • Durability (VOLATILE and TRANSIENT_LOCAL)
  • Reliability (RELIABLE and BEST_EFFORT)

It also updates the liveliness tokens to forward serialized qos information. Graph introspection methods are also updated.
Before testing, remember to restart any running zenoh routers and kill existing ros2 daemons.

Note: PublicationCache only works when time-stamping is enabled for peers in the zenoh configs.

Test below where publisher has durability VOLATILE buyt subsciption has durability TRANSIENT_LOCAL. The QoS properties including History, Durability, and Reliability should be correctly populated when

# terminal 1
ros2 topic pub /chatter std_msgs/msg/String '{data: 1}' --qos-reliability reliable --qos-durability transient_local --qos-history keep_last --qos-depth 7 --rate 0.001
# terminal 2
ros2 topic echo /chatter --qos-durability transient_local --qos-reliability reliable --qos-history keep_last --qos-depth 7 --no-lost-messages

# you should see the single message that was published printed in the terminal

Note: The --no-lost-messages flag is needed or else ros2topic strategy will try to create a subscription with event callback which will fail as the methods are not implemented. The catch block will create another subscription without the event but the message is not received in this case.

# terminal 3

ros2 topic info -v /chatter

Type: std_msgs/msg/String

Publisher count: 1

Node name: _ros2cli_716307
Node namespace: /
Topic type: std_msgs/msg/String
Topic type hash: INVALID
Endpoint type: PUBLISHER
GID: 00.00.00.00.00.00.00.00.00.00.00.00.00.00.00.00
QoS profile:
  Reliability: RELIABLE
  History (Depth): KEEP_LAST (7)
  Durability: TRANSIENT_LOCAL
  Lifespan: 0 nanoseconds
  Deadline: 0 nanoseconds
  Liveliness: AUTOMATIC
  Liveliness lease duration: 0 nanoseconds

Subscription count: 1

Node name: _ros2cli_716424
Node namespace: /
Topic type: std_msgs/msg/String
Topic type hash: INVALID
Endpoint type: SUBSCRIPTION
GID: 00.00.00.00.00.00.00.00.00.00.00.00.00.00.00.00
QoS profile:
  Reliability: RELIABLE
  History (Depth): KEEP_LAST (7)
  Durability: TRANSIENT_LOCAL
  Lifespan: 0 nanoseconds
  Deadline: 0 nanoseconds
  Liveliness: AUTOMATIC
  Liveliness lease duration: 0 nanoseconds


@Yadunund Yadunund force-pushed the yadu/qos-durability branch 3 times, most recently from c528258 to df8517a Compare January 12, 2024 02:58
@Yadunund Yadunund force-pushed the yadu/service_introspection branch from 5c6f26b to 26fca70 Compare January 12, 2024 13:19
@Yadunund Yadunund force-pushed the yadu/qos-durability branch 5 times, most recently from c2b749b to 4fe16af Compare January 16, 2024 08:36
@delete-merged-branch delete-merged-branch bot deleted the branch rolling January 17, 2024 15:56
@Yadunund Yadunund changed the base branch from yadu/service_introspection to rolling January 18, 2024 02:59
@Yadunund Yadunund force-pushed the yadu/qos-durability branch from 4fe16af to b7a27be Compare January 18, 2024 02:59
@Yadunund Yadunund marked this pull request as ready for review January 18, 2024 05:55
@Yadunund
Copy link
Member Author

With the qos methods implemented, actions also work**

# terminal 1
ros2 run action_tutorials_cpp fibonacci_action_server 
# terminal 2
ros2 run action_tutorials_cpp fibonacci_action_client 


[INFO] [1705560511.794785988] [fibonacci_action_client]: Sending goal
[INFO] [1705560511.794917049] [rmw_zenoh_cpp]: [rmw_send_request]
[INFO] [1705560511.796353630] [rmw_zenoh_cpp]: [rmw_take_response]
[INFO] [1705560511.796443000] [fibonacci_action_client]: Goal accepted by server, waiting for result
[INFO] [1705560511.796472320] [rmw_zenoh_cpp]: [rmw_send_request]
[INFO] [1705560511.796863030] [fibonacci_action_client]: Next number in sequence received: 0 1 1 
[INFO] [1705560512.797255312] [fibonacci_action_client]: Next number in sequence received: 0 1 1 2 
[INFO] [1705560513.797292703] [fibonacci_action_client]: Next number in sequence received: 0 1 1 2 3 
[INFO] [1705560514.797283484] [fibonacci_action_client]: Next number in sequence received: 0 1 1 2 3 5 
[INFO] [1705560515.797236973] [fibonacci_action_client]: Next number in sequence received: 0 1 1 2 3 5 8 
^C[INFO] [1705560516.232405866] [rclcpp]: signal_handler(signum=2)

** Killing the client process does not cancel the goal and the server will continue to publish feedback messages. This should be resolved with @clalancette's fixes to service/clients.

@Yadunund Yadunund requested a review from clalancette January 18, 2024 06:52
@Yadunund Yadunund force-pushed the yadu/qos-durability branch from b265745 to 762668b Compare January 18, 2024 16:45
@Yadunund Yadunund force-pushed the yadu/qos-durability branch from 44d7627 to 52d16cb Compare January 24, 2024 05:09
@Yadunund
Copy link
Member Author

Yadunund commented Jan 24, 2024

I did some testing of TRANSIENT_LOCAL vs. VOLATILE on DDS and on Zenoh. The one place where they differ is if the publisher is volatile, and the subscription is transient_local. With Fast-DDS, in that case no data is transmitted and we get an Incompatible QoS warning. With this PR in place, we don't get the incompatible QoS warning and data is transmitted. If possible, I think it would be best if we could keep the semantics the same (at least for now).

Thanks for doing that test. I'm not sure if I agree with that behavior. Imo the subscription should still continue receiving new messages from the publisher BUT with a warning printed that older messages will not be received. But having said that, I'm surprised the implementation here does not throw the same error since we fully reuse rmw_dds_common::qos_profile_check_compatibility and hence my comment here #91 (comment). 🤔 Looking into it.

After some digging, with Fast-DDS, the warning message is thrown because of qos incompatibility from the default_incompatibile_qos_callback. Since we don't support events yet, I think this isn't being flagged. But also since the zenoh transport can handle such qos mismatches, I don't think we will need to flag it? It's one of the reasons I left this todo in the impl here. I'm not sure if we should report incompatibility to match DDS behavior since zenoh transport can handle the differences.

@Yadunund Yadunund force-pushed the yadu/qos-durability branch from 09b301d to 6075f4d Compare January 25, 2024 07:35
@Yadunund Yadunund force-pushed the yadu/qos-durability branch from 6075f4d to a664ac2 Compare January 25, 2024 07:47
@Yadunund
Copy link
Member Author

Yadunund commented Jan 26, 2024

Went back and forth on whether to rely on a blocking vs non-blocking channel for making the liveliness query at startup.

I think we should stick to blocking for now (78ab5a1). Rationale is fewer threads needed (i think) and perhaps more accurate results when using CLI tools like ros2 topic list as the graph cache will be fully updated by the end of rmw_init.
It's better to rely on a non-blocking channel to avoid CLI tools waiting indefinitely when there are no other nodes in the graph.

After clarification from zettascale, we should stick to a blocking channel 2ad493d.

@Yadunund Yadunund force-pushed the yadu/qos-durability branch 3 times, most recently from 3234b66 to 4475bec Compare January 29, 2024 06:06
@Yadunund Yadunund force-pushed the yadu/qos-durability branch from 4475bec to 2ad493d Compare January 29, 2024 06:10
@Yadunund Yadunund force-pushed the yadu/qos-durability branch from ccdcdfb to 1298abd Compare January 31, 2024 03:28
@Yadunund
Copy link
Member Author

Yadunund commented Jan 31, 2024

I've set the SystemDefault depth to 1 via 1298abd.

The default depth is 1 in rmw_cyclonedds.

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 did another review pass. This is looking pretty good in general, just a few things to cleanup and followup on.

rmw_zenoh_cpp/CMakeLists.txt Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/liveliness_utils.cpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/liveliness_utils.cpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/liveliness_utils.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
Yadunund and others added 3 commits February 7, 2024 11:36
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
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 more relatively minor change. Once that is done, I think this is good to go in.

rmw_zenoh_cpp/src/detail/liveliness_utils.cpp Outdated Show resolved Hide resolved
Yadunund and others added 2 commits February 7, 2024 22:34
Signed-off-by: Chris Lalancette <[email protected]>
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 just pushed some minor cpplint fixes. With that, I'm happy with this, so once CI comes back green I'm going to go ahead and merge this.

@clalancette clalancette merged commit 075d0df into rolling Feb 7, 2024
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the yadu/qos-durability branch February 7, 2024 16:24
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.

2 participants