Add hardcoded QoS for the /tf_static topic for parameter bridge #304

lFatality · 2020-12-27T22:29:56Z

Hello everyone,

the hardcoded QoS (keep all, transient local) for the /tf_static topic that were introduced to the dynamic bridge are quite useful and I think they should be added to the parameter bridge as well. This pull request introduces an API to create bidirectional bridges with adjustable QoS for the 1->2 publisher. This API is then used in the parameter bridge to change the QoS for the /tf_static topic.

Cheers
lFatality

P.S.: This is my very first pull request to an open source project so let me know if I miss some steps or something like that :)

Signed-off-by: Fynn Boyer <fynn-boyer@web.de>

sloretz

Thanks for the PR!

I can see how it's an improvement, though I'm hesitant to merge something that overrides the QoS settings based on hardcoded topic name. I'm not sure what the alternatives are.

sloretz · 2021-01-14T18:14:32Z

src/parameter_bridge.cpp

@@ -86,9 +86,15 @@ int main(int argc, char * argv[])
        "with ROS 2 type '%s'\n",
        topic_name.c_str(), type_name.c_str());

+      auto ros2_publisher_qos = rclcpp::QoS(rclcpp::KeepLast(queue_size));
+      if (topic_name == "/tf_static") {


I see how this is an improvement, thought it seems a little fragile. It won't set the QoS settings if the /tf_static has been remapped.

I share your concern about the tf_static being hardcoded but currently I also do not have a better solution. Maybe this could be solved with the approaches that were discussed in the issues that mentioned this PR?

sloretz · 2021-01-14T18:15:34Z

src/parameter_bridge.cpp

@@ -86,9 +86,15 @@ int main(int argc, char * argv[])
        "with ROS 2 type '%s'\n",
        topic_name.c_str(), type_name.c_str());

+      auto ros2_publisher_qos = rclcpp::QoS(rclcpp::KeepLast(queue_size));
+      if (topic_name == "/tf_static") {
+        ros2_publisher_qos.keep_all();


I don't think this should set keep_all. Upstream uses a queue depth of 1 for broadcasters, or 100 for listeners. It seems reasonable to keep using KeepLast(queue_size).

https://github.com/ros2/geometry2/blob/4a6526b3e861f22639460750cd197b59ac1a100b/tf2_ros/include/tf2_ros/qos.hpp#L56

I think the bridge's ROS 2 subscriber would need to use transient_local() too, otherwise I think it won't get any old samples. Similarly the ROS 1 publisher would need to use latching.

Hi there,
the settings were adopted from those used in #282 so they are equal to the dynamic bridge.
I assumed the history policy was set to keep_all as the messages on the tf_static topic are often only published once as far as I know so any late-joining subscriber would need to get all the messages transported on the topic. But I could be wrong here. If I change the setting, should I also change it in the dynamic bridge?

If I understand the QoS documentation (https://docs.ros.org/en/foxy/Concepts/About-Quality-of-Service-Settings.html) correctly I think changing the ROS2 subscriber to use transient_local would have the effect that the sub would only accept messages from publishers that are publishing with a transient_local policy, too. It would ignore volatile publishers. So in order for the sub to receive old samples, I don't think it needs to use transient_local. Instead the ROS2 publishers that publish the static TFs would need to be transient_local. Correct me if I'm mistaken here, the QoS settings are still pretty new to me.

I agree that the ROS1 publisher would need to use latching. This is also not done in the dynamic bridge at the moment. I added a commit that activates ROS1 pub latching if the topic to bridge is tf_static (for both the dynamic & static bridge).

Signed-off-by: Fynn Boyer <fynn-boyer@web.de>

mechwiz · 2021-12-01T18:29:43Z

src/dynamic_bridge.cpp

@@ -254,11 +254,14 @@ void update_bridge(
    bridge.ros1_type_name = ros1_type_name;
    bridge.ros2_type_name = ros2_type_name;

+    bool ros1_pub_latching = (topic_name == "/tf_static");


Suggested change

bool ros1_pub_latching = (topic_name == "/tf_static");

auto ros2_subscriber_qos = rclcpp::QoS(rclcpp::KeepLast(10));

bool ros1_pub_latching = false;

if (topic_name == "/tf_static") {

ros1_pub_latching = true;

ros2_subscriber_qos.keep_all();

ros2_subscriber_qos.transient_local();

}

I think there should also be ros2_subscriber_qos set here to transient_local (don't know if we need keep_all but put it there for now) since the ros2 subscriber might not come alive until after the /tf_static topic has been puslihed to and so will miss the message. This fixes it.

mechwiz · 2021-12-01T18:31:37Z

src/dynamic_bridge.cpp

@@ -254,11 +254,14 @@ void update_bridge(
    bridge.ros1_type_name = ros1_type_name;
    bridge.ros2_type_name = ros2_type_name;

+    bool ros1_pub_latching = (topic_name == "/tf_static");
+
    try {
      bridge.bridge_handles = ros1_bridge::create_bridge_from_2_to_1(
        ros2_node, ros1_node,
        bridge.ros2_type_name, topic_name, 10,


Suggested change

bridge.ros2_type_name, topic_name, 10,

bridge.ros2_type_name, topic_name, ros2_subscriber_qos,

This should be updated based on my previous comment

LoyVanBeek · 2022-07-01T09:45:50Z

From the description, I think the just merged #331 also does what you want.

lFatality · 2022-07-09T16:29:39Z

@LoyVanBeek Yes you're right, that solves the same problem. So I'll close this pull request.

lFatality added 2 commits December 27, 2020 23:48

Add API to create bidirectional bridge with rclcpp QoS

ca6f59b

Signed-off-by: Fynn Boyer <fynn-boyer@web.de>

Add hardcoded QoS for /tf_static topic in parameter bridge

da90da9

Signed-off-by: Fynn Boyer <fynn-boyer@web.de>

lFatality force-pushed the master branch from e9b2a50 to da90da9 Compare December 27, 2020 22:49

sloretz reviewed Jan 14, 2021

View reviewed changes

clalancette assigned sloretz Jan 14, 2021

clalancette mentioned this pull request Jan 14, 2021

Move the QoS detection code to a more central place ros2/rosbag2#601

Open

jacobperron mentioned this pull request Mar 9, 2021

Get most compatibility QoS policies given QoS for multiple endpoints ros2/rmw#304

Closed

Let ROS1 pub be latching if bridging /tf_static topic

1fc58b9

Signed-off-by: Fynn Boyer <fynn-boyer@web.de>

mechwiz reviewed Dec 1, 2021

View reviewed changes

lFatality closed this Jul 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hardcoded QoS for the /tf_static topic for parameter bridge #304

Add hardcoded QoS for the /tf_static topic for parameter bridge #304

lFatality commented Dec 27, 2020

sloretz left a comment

sloretz Jan 14, 2021

lFatality Mar 27, 2021

sloretz Jan 14, 2021

sloretz Jan 14, 2021

lFatality Mar 27, 2021

mechwiz Dec 1, 2021 •

edited

Loading

mechwiz Dec 1, 2021 •

edited

Loading

LoyVanBeek commented Jul 1, 2022

lFatality commented Jul 9, 2022

-    bool ros1_pub_latching = (topic_name == "/tf_static");
+    auto ros2_subscriber_qos = rclcpp::QoS(rclcpp::KeepLast(10));
+    bool ros1_pub_latching = false;
+    if (topic_name == "/tf_static") {
+      ros1_pub_latching = true;
+      ros2_subscriber_qos.keep_all();
+      ros2_subscriber_qos.transient_local();
+    }

	bridge.ros2_type_name, topic_name, 10,
	bridge.ros2_type_name, topic_name, ros2_subscriber_qos,

Add hardcoded QoS for the /tf_static topic for parameter bridge #304

Add hardcoded QoS for the /tf_static topic for parameter bridge #304

Conversation

lFatality commented Dec 27, 2020

sloretz left a comment

Choose a reason for hiding this comment

sloretz Jan 14, 2021

Choose a reason for hiding this comment

lFatality Mar 27, 2021

Choose a reason for hiding this comment

sloretz Jan 14, 2021

Choose a reason for hiding this comment

sloretz Jan 14, 2021

Choose a reason for hiding this comment

lFatality Mar 27, 2021

Choose a reason for hiding this comment

mechwiz Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

mechwiz Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

LoyVanBeek commented Jul 1, 2022

lFatality commented Jul 9, 2022

mechwiz Dec 1, 2021 •

edited

Loading

mechwiz Dec 1, 2021 •

edited

Loading