-
Notifications
You must be signed in to change notification settings - Fork 292
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
Add hardcoded QoS for the /tf_static topic for parameter bridge #304
Conversation
Signed-off-by: Fynn Boyer <fynn-boyer@web.de>
Signed-off-by: Fynn Boyer <fynn-boyer@web.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)