-
Notifications
You must be signed in to change notification settings - Fork 212
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
Autodetect QoS #806
base: rolling
Are you sure you want to change the base?
Autodetect QoS #806
Conversation
Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>
Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>
Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>
Seems to work! I tested this briefly from import rclpy
from rclpy.node import Node
from rclpy.qos import DurabilityPolicy, HistoryPolicy, LivelinessPolicy, HistoryPolicy, QoSProfile, qos_profile_sensor_data
from sensor_msgs.msg import LaserScan
rclpy.init()
node = Node('ipython')
rclpy.spin_once(node, timeout_sec=0.01)
node.create_publisher(LaserScan, 'laser', qos_profile_sensor_data)
weird_qos = QoSProfile(history=HistoryPolicy.KEEP_ALL, durability=DurabilityPolicy.TRANSIENT_LOCAL)
node.create_publisher(LaserScan, 'weird_laser', weird_qos)
rclpy.spin_once(node, timeout_sec=0.01) and indeed the weird QoS settings are automatically set in RViz if you add the topics Looks good to me! |
Dear @ivanpauno @clalancette Any comment about this PR? Do you think it is useful/valid? Any improvement that you want I do. Thanks!! |
// Check if there is any Subscriber with the same QoS as set | ||
bool any_publisher_qos_compatible = false; | ||
for (const auto & publisher : publishers) { | ||
if (qos_profile == publisher.qos_profile()) { |
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.
The profile doesn't have to be the same in order to be compatible.
We should check for compatibility instead.
There's no rclcpp wrapper, so you can either use the rmw function directly or add one.
if (!any_publisher_qos_compatible) { | ||
qos_profile = publishers[0].qos_profile(); | ||
qos_profile_property_->setQoSProfile(qos_profile); | ||
} |
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.
This is not great, because e.g. if you pick RELIABLE
reliability and there was another published that used BEST_EFFORT
, you're going to only match the first one.
Whereas if you picked BEST_EFFORT
for the subscription, that was going to match both publishers.
In practice, having more than one publisher in the same topic with different reliability is not a good idea, and when that's found the best thing to do is warn the user.
Anyways if we need to pick one default in those cases, it should be the one that matches the most cases.
For durability, if we find publishers with different settings we should pick VOLATILE, which matches all publisher.
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 you can find an example of this logic in ros2/ros2cli#653
Dear RViz2 developers,
This PR contains a way to determine the topic QoS when subscribing to a topic.
I would appreciate the opinion of @LoyVanBeek and @zmk5, because we were talking about this in this thread and maybe they want to contribute to this PR.
I hope it helps!!
Signed-off-by: Francisco Martín Rico fmrico@gmail.com