Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ros2 service info #771

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Add ros2 service info #771

merged 4 commits into from
Oct 16, 2023

Conversation

leeminju531
Copy link
Contributor

@leeminju531 leeminju531 commented Oct 8, 2022

@mjcarroll
Copy link
Member

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

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

@leeminju531
Copy link
Contributor Author

@mjcarroll friendly ping, could you renew CI ?

@fujitatomoya
Copy link
Collaborator

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

@leeminju531 leeminju531 marked this pull request as ready for review December 12, 2022 02:15
@mjcarroll
Copy link
Member

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

Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me with green CI.

It looks like there may be some code-style errors still. I added suggestions for adding new lines at the end.

ros2cli/test/test_daemon.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/info.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/info.py Outdated Show resolved Hide resolved
@leeminju531
Copy link
Contributor Author

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

@fujitatomoya
Copy link
Collaborator

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

about ros2/rmw#334 (comment), any thoughts? @audrow @gbiggs

@schornakj
Copy link

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 Add ros2 service info command #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?

@leeminju531
Copy link
Contributor Author

@schornakj it sounds reasonable to me 😊

@leeminju531
Copy link
Contributor Author

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

ros2service/ros2service/verb/info.py Show resolved Hide resolved
ros2service/ros2service/verb/info.py Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

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

@leeminju531
Copy link
Contributor Author

👌 All the things are addressed. @fujitatomoya

@fujitatomoya
Copy link
Collaborator

@leeminju531 thanks for rebasing and addressing comments.

CI:

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

@clalancette
Copy link
Contributor

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

@leeminju531
Copy link
Contributor Author

leeminju531 commented Sep 14, 2023

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
Copy link
Contributor Author

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

@fujitatomoya
Copy link
Collaborator

@clalancette can you check ros2/rmw_implementation#208? once that is approved, i will start the CI again.

@fujitatomoya
Copy link
Collaborator

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

Signed-off-by: leeminju531 <[email protected]>
Signed-off-by: leeminju531 <[email protected]>
Signed-off-by: leeminju531 <[email protected]>
Signed-off-by: leeminju531 <[email protected]>
@leeminju531
Copy link
Contributor Author

leeminju531 commented Oct 14, 2023

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.

@fujitatomoya
Copy link
Collaborator

CI:

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

@fujitatomoya fujitatomoya merged commit 41b5073 into ros2:rolling Oct 16, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants