search

ros2 / ros2cli

ROS 2 command line interface tools
Apache License 2.0
172 stars 161 forks source link

Add QoS parameter to `ros2 service call` #812

Open bastianhjaeger opened 1 year ago

bastianhjaeger commented 1 year ago

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 commented 1 year ago

@bastianhjaeger are you gonna work on this?

bastianhjaeger commented 1 year ago

@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: https://github.com/ros2/ros2cli/pull/812#discussion_r1145895479

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 commented 1 year ago

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

fujitatomoya commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

match current

fujitatomoya commented 1 year ago

sorry accidentally closed, reopened.

fujitatomoya commented 1 year ago

@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 https://github.com/ros2/rclcpp/issues/1868

i might be mistaken, could you elaborate it?