-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This should be updated based on my previous comment |
||||||
bridge.ros1_type_name, topic_name, 10); | ||||||
bridge.ros1_type_name, topic_name, 10, | ||||||
ros1_pub_latching); | ||||||
} catch (std::runtime_error & e) { | ||||||
fprintf( | ||||||
stderr, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,9 +86,18 @@ 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)); | ||
bool ros1_publisher_latching = false; | ||
if (topic_name == "/tf_static") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I share your concern about the |
||
ros1_publisher_latching = true; | ||
ros2_publisher_qos.keep_all(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the bridge's ROS 2 subscriber would need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi there, 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 I agree that the ROS1 publisher would need to use |
||
ros2_publisher_qos.transient_local(); | ||
} | ||
|
||
try { | ||
ros1_bridge::BridgeHandles handles = ros1_bridge::create_bidirectional_bridge( | ||
ros1_node, ros2_node, "", type_name, topic_name, queue_size); | ||
ros1_node, ros2_node, "", type_name, topic_name, queue_size, ros1_publisher_latching, | ||
ros2_publisher_qos); | ||
all_handles.push_back(handles); | ||
} catch (std::runtime_error & e) { | ||
fprintf( | ||
|
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 there should also be ros2_subscriber_qos set here to
transient_local
(don't know if we needkeep_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.