Add QoS parameter to ros2 service call

[bastianhjaeger]

In case of service setups diverging from the default QoS on a service, a service may not be called by cli due to mismatching QoS profiles.
This PR should fix it.

[fujitatomoya]

besides adding test cases in ros2service.test.test_cli would be better.

[fujitatomoya]

@bastianhjaeger are you gonna work on this?

[bastianhjaeger]

@bastianhjaeger are you gonna work on this?

Yes. I am. I am more a c++ developer, so I am slow(er), but I am willing to work on this.

Regarding your comment

besides adding test cases in ros2service.test.test_cli would be better.
Can you elaborate what you mean in detail? What kind of test cases?

And my question to you here is still open:
#812 (comment)

In addition, I do not comprehend why the test fails here:
https://build.ros2.org/job/Hpr__ros2cli__ubuntu_jammy_amd64/20/testReport/ros2service.ros2service.test/test_cli/test_cli/
Maybe someone can elaborate this?

[fujitatomoya]

@bastianhjaeger thank you for the contribution, i will look into this.

[fujitatomoya]

@bastianhjaeger sorry to be late to get back to you, i think implementation looks good, but could be updated for future maintenance and flexibility. thanks

[bastianhjaeger]

@fujitatomoya
I actually just realized, that the merging of all the QoS arguments and its handling is not so desirable. For the ros2 topic echo you actually want the default to match current publishers QoS (reliability and durability). For ros2 topic pub it is different. This results in different descriptions in how arguments are handles, thus the arg description is different.

Moving all the QoS argument handling in one file would result in one file, but the file would contain something like

if "topic echo"
  // automatically adapt reliability/durability to current publisher
else if "topic pub"
  //...

and this is, in my opinion not desirable. My approach now would be to revert everything for the topic section and add a specific QoS arg handling for service call. What do you think?

[fujitatomoya]

cont to https://github.com/ros2/ros2cli/pull/812/files#r1202519155, entity name should be removed as well? (it says all for subscription only.)

Suggested change

	help='Quality of service preset profile to subscribe with (default: {})'
	help='Quality of service preset profile with (default: {})'

[fujitatomoya]

Sorry missed it 😅

Suggested change

	# Copyright 2017 Open Source Robotics Foundation, Inc.
	# Copyright 2023 Open Source Robotics Foundation, Inc.

[fujitatomoya]

match current

[fujitatomoya]

sorry accidentally closed, reopened.

[fujitatomoya]

@fujitatomoya I actually just realized, that the merging of all the QoS arguments and its handling is not so desirable. For the ros2 topic echo you actually want the default to match current publishers QoS (reliability and durability). For ros2 topic pub it is different. This results in different descriptions in how arguments are handles, thus the arg description is different.

Moving all the QoS argument handling in one file would result in one file, but the file would contain something like

if "topic echo"
  // automatically adapt reliability/durability to current publisher
else if "topic pub"
  //...

and this is, in my opinion not desirable. My approach now would be to revert everything for the topic section and add a specific QoS arg handling for service call. What do you think?

I am not clear on this. originally this PR is meant to introduce the QoS argument for ros2 service call.
but are you saying that we should introduce the new logic to see the most appropriate QoS setting for ros2 xxx xxx command individually? e.g ros2/rclcpp#1868

i might be mistaken, could you elaborate it?