Fix QoS depth settings for clients/service ignored #340
Conversation
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
|
@ros-pull-request-builder retest this please |
There was a problem hiding this comment.
I know I suggested simply deleting these lines, and it does indeed prevent the QoS settings from being ignored. However, on consideration that's not the same as the defaults used for "regular" readers and writers also being appropriate for clients and services.
With this PR, providing a QoS profile with RMW_QOS_POLICY_HISTORY_SYSTEM_DEFAULT and RMW_QOS_POLICY_DEPTH_SYSTEM_DEFAULT will result in KEEP_LAST with depth 1. There doesn't seem to be any specification of what one should expect if QoS's are set to "default" in the ROS 2 QoS profile and therefore it can be considered the correct result given that the user is not entitled to any expectation. In all likelihood, this setting will result in a system that usually works fine — which, in my experience, is not good enough.
I do want to point out that it is, essentially, in line with rmw_qos_profile_services_default. Therefore, arguably, in ROS 2, KEEP_LAST for service invocation is considered the proper setting. In my view, this choice means an application should expect failure to receive a response without any indication, or the system (of which the application is part) be designed in such a way that the limited history depth cannot cause a problem.
I am approving this PR because I assume that I am just stating the obvious in this review comment and that default to KEEP_LAST with depth 1 is indeed the intent.
|
CI:
|
|
CI failure is unrelated. (see also ros2/rmw_fastrtps#564 (comment)) |
|
@clalancette @ivanpauno @eboasson we do not have access for this repository, could you do us a favor? |
|
@fujitatomoya sorry for waiting a bit. I was hoping to get a confirmation that this is really the intent despite my concerns about the wisdom of the defaults, but that's really not a good reason to delay this PR! |
|
@eboasson i understand it is always nice to check ❗ thanks for the support. |
to fix ros2/rclcpp#1785
Signed-off-by: Chen Lihui lihui.chen@sony.com