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
|
@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