ros2cli icon indicating copy to clipboard operation
ros2cli copied to clipboard

Published 20 hours ago • verified publisher icon ros2

Add ros2 service info

Open leeminju531 opened this issue 3 years ago • 1 comments

Related Issue https://github.com/ros2/ros2cli/issues/766

[DEPENDENCY] to track works related to this:

  • rmw_cyclonedds (https://github.com/ros2/rmw_cyclonedds/pull/427)
  • rmw_fastrtps (https://github.com/ros2/rmw_fastrtps/pull/641)
  • rmw_connextdds (https://github.com/ros2/rmw_connextdds/pull/93)
  • rmw (https://github.com/ros2/rmw/pull/334)
  • rmw_implementation (https://github.com/ros2/rmw_implementation/pull/208)
  • rcl (https://github.com/ros2/rcl/pull/1011)
  • rclpy (https://github.com/ros2/rclpy/pull/1024)

Anticipated output service_info

Signed-off-by: leeminju531 [email protected]

leeminju531 avatar Oct 08 '22 14:10 leeminju531

Running a full CI from (https://gist.github.com/mjcarroll/9e6d17c5e4b5b5ac9f9364365145277b)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

mjcarroll avatar Nov 09 '22 14:11 mjcarroll

@mjcarroll friendly ping, could you renew CI ?

leeminju531 avatar Dec 11 '22 17:12 leeminju531

@leeminju531 is this still draft? i am happy to review those PRs.

fujitatomoya avatar Dec 12 '22 00:12 fujitatomoya

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

mjcarroll avatar Dec 12 '22 14:12 mjcarroll

friendly ping @audrow , I think my tasks are completed, are there more steps? I'm waiting for merge :smiley::laughing:

leeminju531 avatar Dec 22 '22 15:12 leeminju531

@leeminju531 are you willing to implement this for https://github.com/ros2/rclcpp?

about https://github.com/ros2/rmw/pull/334#discussion_r1057986256, any thoughts? @audrow @gbiggs

fujitatomoya avatar Dec 28 '22 00:12 fujitatomoya

I think it would be useful to connect this to #703, which is my version of this feature.

From my perspective it's important to have a version of this new verb which can be backported to Humble, since I'm working with commercial and industrial clients who can only use LTS releases of ROS 2. If a feature doesn't make it into an LTS release then these users will not be able to benefit from it until at least May 2024 when J-Turtle is released.

It's a good approach to extend the fundamental rcl API to add more functionality. However, I expect that it will not be possible to backport this API change plus the dependent RMW implementation changes to Humble, since these are ABI-breaking changes that require recompiling these packages.

Since the files you've added to ros2service/verb are very similar to the ones I added in my version, I think we could do something like this:

  • Merge #703 and create a PR to backport it to Humble
  • Merge the upstream dependencies of this PR
  • Merge this PR to use your count_clients and count_services functions in the info verb.

@leeminju531 what do you think?

schornakj avatar Jan 02 '23 15:01 schornakj

@schornakj it sounds reasonable to me 😊

leeminju531 avatar Jan 03 '23 02:01 leeminju531

It looks like this PR is still pending. If any additional work is needed before merging, please feel free to let me know

leeminju531 avatar Mar 26 '23 17:03 leeminju531

@leeminju531 all related PRs look good to me except minor comments. i think it would be probably better to rebase all these PRs, and the we can start the CI.

fujitatomoya avatar Sep 10 '23 06:09 fujitatomoya

:ok_hand: All the things are addressed. @fujitatomoya

leeminju531 avatar Sep 12 '23 14:09 leeminju531

@leeminju531 thanks for rebasing and addressing comments.

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

fujitatomoya avatar Sep 12 '23 17:09 fujitatomoya

It looks like Windows is failing CI for some reason. @leeminju531 please take a look.

clalancette avatar Sep 13 '23 17:09 clalancette

failing https://github.com/leeminju531/rmw_implementation/blob/59c7868449b283b38370d2e6c25aa9f3f94096ce/test_rmw_implementation/test/test_graph_api.cpp#L993.

passing next client count though, https://github.com/leeminju531/rmw_implementation/blob/59c7868449b283b38370d2e6c25aa9f3f94096ce/test_rmw_implementation/test/test_graph_api.cpp#L1007

probably we need to add some sleep to wait the graph update?

fujitatomoya avatar Sep 13 '23 19:09 fujitatomoya

I think adding a brief sleep can solve this too. I believe the failure can occur not only in Windows but in any operating system. The test attempts to discover the topic right after creating participants, so the time to discover was faster than its period.

leeminju531 avatar Sep 14 '23 09:09 leeminju531

The sleep added in rmw_implementation. could you run CI again? @fujitatomoya

leeminju531 avatar Sep 14 '23 09:09 leeminju531

@clalancette can you check https://github.com/ros2/rmw_implementation/pull/208? once that is approved, i will start the CI again.

fujitatomoya avatar Sep 14 '23 17:09 fujitatomoya

@clalancette thanks for merging rmw packages. do you still need time for clients libraries including ros2cli? or i can start the CI based on the latest source.

fujitatomoya avatar Sep 29 '23 17:09 fujitatomoya

Thanks for merging all dependencies, Could someone run CI for this ?

Additionally, I've considered the '--verbose' option for details like topic info -v. However, it seems to require ABI breaking changes. Perhaps it needs something similar to rmw_get_clients_info_by_service like rmw_get_publishers_info_by_topic.

Nonetheless, it appears that information on which node owns the clients or services is possible here. In my experience, displaying this information seems to be quite useful.

leeminju531 avatar Oct 14 '23 07:10 leeminju531

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

fujitatomoya avatar Oct 15 '23 20:10 fujitatomoya