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] Support echo verb for ros2 service cli #732

Closed
wants to merge 20 commits into from

Conversation

deepanshubansal01
Copy link
Contributor

@deepanshubansal01 deepanshubansal01 commented Jul 5, 2022

Related to the proposed service introspection REP (ros-infrastructure/rep#360)

This PR implements the cli for echoing service events.
ros2 service echo /name_of_service

Depends on ros2/rosidl_runtime_py#17 for deserializing bytes type data and echoing contents to the user.

Connects to: ros2/ros2#1285

Signed-off-by: deepanshu [email protected]

@deepanshubansal01 deepanshubansal01 self-assigned this Jul 5, 2022
@deepanshubansal01 deepanshubansal01 marked this pull request as draft July 5, 2022 13:27
@deepanshubansal01 deepanshubansal01 changed the title ros2 service echo cli [Service Introspection] Support echo verb for ros2 service cli Jul 5, 2022
@ihasdapie
Copy link
Member

I gave this a run and it's great to see a working prototype going. I would like to see this tool behave in a way consistent to ros2 topic echo, i.e. wait until the service is avaliable instead of exiting if unavaliable. This way we can keep a terminal with ros2 service echo open instead of having to scramble to run it once the service starts.

@deepanshubansal01
Copy link
Contributor Author

deepanshubansal01 commented Jul 8, 2022

I gave this a run and it's great to see a working prototype going. I would like to see this tool behave in a way consistent to ros2 topic echo, i.e. wait until the service is avaliable instead of exiting if unavaliable. This way we can keep a terminal with ros2 service echo open instead of having to scramble to run it once the service starts.

I see, makes sense. I guess right now the ros2 service echo behaves similar to the way ros2 topic echo works, I might be wrong though. The ros2 topic echo also exits if the topic name does not exist. For instance if you do ros2 topic echo /test it will exit if /test topic is not their. The reason is it needs the topic to be present to determine the topic type. However, you can provide the topic type along with topic name, making the echo to not exit even if the topic is not present.

This being said we can also wait for the service to be present, I just made it similar to ros2 topic for now. In order to make it wait for service we can set blocking == True by default here or provide an additional optional argument.

def get_service_class(node, service, blocking=False, include_hidden_services=False):

The ros2 topic echo has blocking = False by default.

def get_msg_class(node, topic, blocking=False, include_hidden_topics=False):

if args.message_type is None:
message_type = get_msg_class(
node, args.topic_name, include_hidden_topics=True)



class EchoVerb(VerbExtension):
"""Echo a service."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you expand this docstring to provide a short explanation and an example ? It would be nice if we could write a simple test / self contained example for this feature.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should add a test to test_cli.py.

ros2service/setup.py Outdated Show resolved Hide resolved
ros2service/ros2service/api/__init__.py 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/verb/echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Outdated Show resolved Hide resolved
ros2service/ros2service/verb/echo.py Outdated Show resolved Hide resolved
@ihasdapie
Copy link
Member

@deepanshubansal01 I've pushed a commit which tidies up the code a bit and removes the serialization bit. I've also tided up the output.

Also, sorry for hijacking your branch. On a side note, if possible I'd like to close this PR and open up a new one so that we can have all of the PRs off a service_introspection branch which would make CI a little easier later.

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

@deepanshubansal01 I've pushed a commit which tidies up the code a bit and removes the serialization bit. I've also tided up the output.

Also, sorry for hijacking your branch. On a side note, if possible I'd like to close this PR and open up a new one so that we can have all of the PRs off a service_introspection branch which would make CI a little easier later.

Sounds good to me. You can open a new PR, this PR also contains some changes related to unit tests which we can ignore in the new PR since they were in progress. Unit tests can be added in the new PR though.

@ihasdapie
Copy link
Member

I opened #745. Closing in lieu of that PR.

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.

4 participants