add QoS Profile/Depth support to Node. #1376
Conversation
Signed-off-by: Tomoya Fujita <[email protected]>
fujitatomoya
requested review from
sloretz and
adityapande-1995
as code owners
| :param rosout_qos_profile: A QoSProfile or a history depth to apply to rosout publisher. | ||
| In the case that a history depth is provided, the QoS history is set to KEEP_LAST, | ||
| the QoS history depth is set to the value of the parameter, | ||
| and all other QoS settings are set to their default values. |
There was a problem hiding this comment.
One concern: changing QoS for rosout publisher would generate incompatible QoS setting with concerned subscribers. this possibly break the ROS 2 user application, this will not happen in downstream users, but we would want to add more comment for this?
|
@ahcorde @clalancette please take a look if you have time. |
There was a problem hiding this comment.
flake8 is failing https://build.ros2.org/job/Rpr__rclpy__ubuntu_noble_amd64/400/
Signed-off-by: Tomoya Fujita <[email protected]>
|
Pulls: #1376
|
| def test_create_node_disable_rosout(self): | ||
| node_name = 'create_node_test_disable_rosout' | ||
| namespace = '/ns' | ||
| node = rclpy.create_node( | ||
| node_name, namespace=namespace, context=self.context, enable_rosout=False) | ||
| node.destroy_node() | ||
|
|
||
| def test_create_node_rosout_qos_profile(self): | ||
| node_name = 'create_node_test_rosout_rosout_qos_profile' | ||
| namespace = '/ns' | ||
| node = rclpy.create_node( | ||
| node_name, namespace=namespace, context=self.context, enable_rosout=True, | ||
| rosout_qos_profile=qos_profile_sensor_data) | ||
| node.destroy_node() | ||
|
|
||
| def test_create_node_rosout_qos_depth(self): | ||
| node_name = 'create_node_test_rosout_rosout_qos_depth' | ||
| namespace = '/ns' | ||
| node = rclpy.create_node( | ||
| node_name, namespace=namespace, context=self.context, enable_rosout=True, | ||
| rosout_qos_profile=10) | ||
| node.destroy_node() |
There was a problem hiding this comment.
I'm not a huge fan of tests that don't assert anything. I guess these do test that this isn't throwing an exception, but they aren't testing that what we asked for actually happened. Could we assert that rosout exists (or doesn't exist), and that it has the QoS profile we provided?
| @@ -214,6 +217,7 @@ def create_node( | |||
| namespace: Optional[str] = None, | |||
| use_global_arguments: bool = True, | |||
| enable_rosout: bool = True, | |||
| rosout_qos_profile: Optional[Union[QoSProfile, int]] = qos_profile_system_default, | |||
There was a problem hiding this comment.
I don't think this is the correct "default" profile for the rosout publisher. As far as I can tell, the default here is actually https://github.com/ros2/rcl/blob/d9daca7c0af7ecdcd103ece30bb7c64bc919ef24/rcl/include/rcl/logging_rosout.h#L37-L48 . So I think we should define this to be the same by default.
|
@clalancette thanks for the review and information. i agree with your comments, i will try to address them. |
closes #1368