Skip to content
This repository has been archived by the owner on Oct 7, 2021. It is now read-only.

rmw_get_service_names_and_types should support other RMW's service types #239

Closed
dhood opened this issue Jun 29, 2018 · 3 comments · Fixed by #240
Closed

rmw_get_service_names_and_types should support other RMW's service types #239

dhood opened this issue Jun 29, 2018 · 3 comments · Fixed by #240
Assignees
Labels
bug Something isn't working

Comments

@dhood
Copy link
Member

dhood commented Jun 29, 2018

The implementation of rmw_get_service_names_and_types in rmw_opensplice_cpp was changed in #234 with support for services using opensplice typesupport in mind.

We mentioned in that PR that it is always guaranteed that Sample_ is in the type name, which is indeed true for services using opensplice typesupport.

However, if, for example, the ros2 daemon is using rmw_opensplice_cpp, it may need to also report services that use different typesupports, e.g. those produced by an rmw_fastrtps_cpp node.

The following currently fails (testing on xenial from-source; originally uncovered by @nuclearsandwich testing on arm debs):

$ ros2 daemon stop
$ RMW_IMPLEMENTATION=rmw_opensplice_cpp ros2 daemon start
$ RMW_IMPLEMENTATION=rmw_fastrtps_cpp ros2 run lifecycle lifecycle_talker 
$ ros2 service list
Traceback (most recent call last):
  File "/home/dhood/ros2_bouncy/install/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli', 'console_scripts', 'ros2')()
  File "/home/dhood/ros2_bouncy/build/ros2cli/ros2cli/cli.py", line 69, in main
    rc = extension.main(parser=parser, args=args)
  File "/home/dhood/ros2_bouncy/build/ros2service/ros2service/command/service.py", line 43, in main
    return extension.main(args=args)
  File "/home/dhood/ros2_bouncy/build/ros2service/ros2service/verb/list.py", line 40, in main
    include_hidden_services=args.include_hidden_services)
  File "/home/dhood/ros2_bouncy/build/ros2service/ros2service/api/__init__.py", line 21, in get_service_names_and_types
    service_names_and_types = node.get_service_names_and_types()
  File "/usr/lib/python3.5/xmlrpc/client.py", line 1092, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python3.5/xmlrpc/client.py", line 1432, in __request
    verbose=self.__verbose
  File "/usr/lib/python3.5/xmlrpc/client.py", line 1134, in request
    return self.single_request(host, handler, request_body, verbose)
  File "/usr/lib/python3.5/xmlrpc/client.py", line 1150, in single_request
    return self.parse_response(resp)
  File "/usr/lib/python3.5/xmlrpc/client.py", line 1322, in parse_response
    return u.close()
  File "/usr/lib/python3.5/xmlrpc/client.py", line 655, in close
    raise Fault(**self._stack[0])
xmlrpc.client.Fault: <Fault 1: "<class 'RuntimeError'>:Failed to get_service_names_and_types: failed to convert DDS type name to ROS service type name: '/Sample_' not found in type: 'lifecycle_msgs/ChangeState', at /home/dhood/ros2_bouncy/src/ros2/rmw_opensplice/rmw_opensplice_cpp/src/rmw_service_names_and_types.cpp:129">
@dhood dhood added the bug Something isn't working label Jun 29, 2018
@mikaelarguedas
Copy link
Member

Indeed that's a bug

Can be trivially fixed by reverting f16af60 that was not assuming that the /Sample_ prefix was always present.

From the original PR, approach is somewhat fragile but should work for all ROS cases as users are not allowed to use / in packages / interface names.

We should document that it may not work for pure DDS services that could include that string in their type but is not a blocker apply the fix.

@dhood
Copy link
Member Author

dhood commented Jun 29, 2018

From the original PR, approach is somewhat fragile but should work for all ROS cases as users are not allowed to use / in packages / interface names.

I don't know that I follow your reasoning here. When I was originally investigating ros2/ros2cli#106, the service types were getting reported as e.g. lifecycle_msgs/Sample_GetState. So, isn't it the case that a ROS type of say Sample_Number in any package will match the string, since the type is joined with the package name?

Is that name allowed? This design document says, on the topic of .msg and .srv definitions:

Both file names must use an upper camel case name and only consist of alphanumeric characters.

From looking at rosidl_parser I am not seeing anywhere that we enforce that service names cannot have an underscore in them. So today it seems that my_package/Sample_Number is allowed, so it's possible that this will still catch ROS users' service types.

@dhood
Copy link
Member Author

dhood commented Jun 29, 2018

as @mikaelarguedas suspected, srvs with underscores do end up failing to match a regex. That happens here, for something like example_interfaces/Add_Two_Ints.srv

So, it does seem safe to remove Sample_ in the type if it's been found, because user-defined ROS service types containing Sample_ won't make it that far.

I'll remove the printing of the error message.

@dhood dhood self-assigned this Jun 29, 2018
@dhood dhood added the in progress Actively being worked on (Kanban column) label Jun 29, 2018
@dhood dhood removed the in progress Actively being worked on (Kanban column) label Jun 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants