Add hardcoded QoS for the /tf_static topic for parameter bridge#304
Add hardcoded QoS for the /tf_static topic for parameter bridge#304lFatality wants to merge 3 commits intoros2:masterfrom
Conversation
Signed-off-by: Fynn Boyer <fynn-boyer@web.de>
Signed-off-by: Fynn Boyer <fynn-boyer@web.de>
sloretz
left a comment
There was a problem hiding this comment.
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.
| topic_name.c_str(), type_name.c_str()); | ||
|
|
||
| auto ros2_publisher_qos = rclcpp::QoS(rclcpp::KeepLast(queue_size)); | ||
| if (topic_name == "/tf_static") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
|
||
| auto ros2_publisher_qos = rclcpp::QoS(rclcpp::KeepLast(queue_size)); | ||
| if (topic_name == "/tf_static") { | ||
| ros2_publisher_qos.keep_all(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
| bridge.ros1_type_name = ros1_type_name; | ||
| bridge.ros2_type_name = ros2_type_name; | ||
|
|
||
| bool ros1_pub_latching = (topic_name == "/tf_static"); |
There was a problem hiding this comment.
| 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.
| try { | ||
| bridge.bridge_handles = ros1_bridge::create_bridge_from_2_to_1( | ||
| ros2_node, ros1_node, | ||
| bridge.ros2_type_name, topic_name, 10, |
There was a problem hiding this comment.
| 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
|
From the description, I think the just merged #331 also does what you want. |
|
@LoyVanBeek Yes you're right, that solves the same problem. So I'll close this pull request. |
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 :)