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

[service introspection] ros2 service echo #745

Merged
merged 5 commits into from
May 22, 2023

Conversation

ihasdapie
Copy link
Member

@ihasdapie ihasdapie commented Aug 10, 2022

Implementation for proposed REP2012 ros-infrastructure/rep#360

This PR implements ros2 service echo

Related PR: ros2/ros2#1285

Depends on ros2/rclpy#1089

@clalancette
Copy link
Contributor

I've reworked this to be a lot simpler now. It seems to now work fairly well; the only missing thing here is tests, which I'll do another time.

@jacobperron jacobperron removed their assignment May 9, 2023
@jacobperron
Copy link
Member

@clalancette I've added a test checking that the tool can see all four types of events and content (c6fb6f3). It's not super thorough, but a good smoke test.

@jacobperron jacobperron marked this pull request as ready for review May 12, 2023 08:29
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me now, so I'm going to go ahead with CI on it. I'd prefer another review (maybe from @jacobperron ) before merging, since I did substantial work here.

@clalancette
Copy link
Contributor

CI:

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

ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Show resolved Hide resolved
Copy link

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

A few suggestions.

ros2service/test/test_echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py Outdated Show resolved Hide resolved
deepanshubansal01 and others added 5 commits May 19, 2023 20:40
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Launch a test fixture with an introspectable client repeatedly sending requests to an introspectable service.
The test checks that we can see all four expected service events sequentially using the new 'echo' verb.

Signed-off-by: Jacob Perron <jacobmperron@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@clalancette
Copy link
Contributor

CI:

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

@fujitatomoya fujitatomoya merged commit 8523728 into rolling May 22, 2023
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the service_introspection branch May 22, 2023 00:22
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.

None yet

6 participants