Porting rosapi package. #388

jubeira · 2019-02-22T20:28:04Z

This PR focuses on porting rosapi package to ROS2.

See ros2#2 for the current overall progress.

- Porting service servers and auxiliary functions to ROS2. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

- Using absolute node names. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira · 2019-02-28T21:25:11Z

@dirk-thomas this should be ready for an initial review. Some notes to consider:

There are two services that haven't been ported: ServiceHost and SearchParam.
- ServiceHost should return the hostname of the node providing a given service. This is not available in rclpy. For topics for example, only the amount of publishers / subscribers can be retrieved in contrast with rostopic info in ROS1, where the hostname is also exposed. I've taken a quick look and adding this would mean adjusting the code all the way down to rmw layer to upstream that information.
- SearchParam relied on rospy.search_param. Porting it would require bringing in that functionality to ROS2 and maybe adapting it a bit. It shouldn't be as hard as ServiceHost; it's just not straightforward and I'm not sure whether this is needed or not for a start.
For services related with parameters:
- I created a different node to call services within service callbacks without entering a deadlock. There's a note explaining that it should be a temporary workaround.
- I added node_name as a parameter for service requests, as parameters are now associated with nodes in contrast to ROS1. This breaks the API, but for a start it seemed the most logical thing to do. Another possibility would be to guess the node name and the parameter name from a single string considering the node names can be fetched before, but that is a bit more complex.
Adding test cases would be a good idea.

That being said, my initial suggestion would be to proceed if possible without dealing with those two remaining services. Of course, comments and suggestions are welcome!

rosapi/scripts/rosapi_node

rosapi/src/rosapi/glob_helper.py

dirk-thomas · 2019-03-19T18:24:15Z

rosapi/src/rosapi/objectutils.py

@@ -95,7 +95,7 @@ def get_service_response_typedef_recursive(servicetype):

 def _get_typedef(instance):
    """ Gets a typedef dict for the specified instance """
-    if instance is None or not hasattr(instance, "__slots__") or not hasattr(instance, "_slot_types"):
+    if instance is None or not hasattr(instance, "__slots__") or not hasattr(instance, "_fields_and_field_types"):


Just a heads up: _fields_and_field_types is likely going to be replaced on master soon, see ros2/rosidl_python#33.

Thanks for the heads up; I'll keep an eye on that PR and update this code once it's merged.

It seems that in the end it was not replaced (see rosidl_generator_py/_msg.py.em).
It should then be fine to keep this as is.

rosapi/src/rosapi/objectutils.py

rosapi/src/rosapi/params.py

rosbridge_library/src/rosbridge_library/internal/ros_loader.py

dirk-thomas · 2019-03-19T19:45:34Z

There are two services that haven't been ported:

What are those two services being used? So how severe is it to not port them?

jubeira · 2019-03-19T20:29:19Z

Thanks for the comments @dirk-thomas! I'll be addressing them soon.

What are those two services being used? So how severe is it to not port them?

These services aren't being used in rosbridge itself; I've searched the codebase for a client for those services and I found none.
I also made some tests with rvizweb by adding some print statements for each service handler and then using the controls provided by rvizweb (pointcloud, image, depthcloud, tf, etc). The only service that was used was GetParam, the others were not.

In other words, a missing service might affect client code using rosbridge, not rosbridge itself. Not porting those two services in particular doesn't seem to have a very high impact at least in principle.

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

- Assuming that message packages are already in the path. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira · 2019-04-12T20:20:53Z

@dirk-thomas comments were addressed; this one is pending until the referenced PR is merged.

jubeira · 2019-06-06T13:55:30Z

@dirk-thomas I went through the comments once again to double check; I think this is ready to be merged to ros2 branch already.
I'll be waiting for approval or further comments. Thanks in advance!

gonzodepedro

LGTM

Porting rosapi package. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

travipross · 2020-08-14T04:31:07Z

rosapi/src/rosapi/params.py

        d = loads(value)
    except ValueError:
        raise Exception("Due to the type flexibility of the ROS parameter server, the value argument to set_param must be a JSON-formatted string.")
+
+    node_name = get_absolute_node_name(node_name)
    with param_server_lock:
-        rospy.set_param(name, d)
-
-
-def get_param(name, default, params_glob):
+        _set_param(node_name, name, value)


@jubeira Sorry to dig up this old PR, but I'm having some trouble with a client that calls on this API and I think I've isolated the issue to this spot. It seems as though when value is a JSON-encoded string, the later call to get_parameter_value() sets the ParameterValue.string_value attribute with double-quotes, meaning it's impossible to set a string parameter via this method without double quotes being applied.

If the client sends the value without JSON encoding, the above ValueError is thrown and set_param() fails altogether.

I think inside the try block, there could be a line value = d if isinstance(d, str) else value.

This would ensure that a valid string object is later passed to get_parameter_value() rather than a JSON-encoded one, but otherwise behaviour would stay the same. Would this potentially work?

Hi @travipross!
Honestly it's been a while so I'm not sure what the actual consequences of that change could be. Your proposal makes sense but I can't be 100% sure without trying the code.
I'd suggest you to fork the repo, apply the change and check if it works for you. If it does, please feel free to contribute the change with a PR explaining what you did.
Thanks!

Thanks, opened one here: #521

Juan Ignacio Ubeira added 3 commits February 22, 2019 17:24

Porting rosapi package.

f9dcf6b

- Porting service servers and auxiliary functions to ROS2. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Adding missing param services.

9d6fe34

- Using absolute node names. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Adjusting globs, documentation and nits.

e6449e8

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

dirk-thomas reviewed Mar 19, 2019

View reviewed changes

Juan Ignacio Ubeira added 5 commits April 1, 2019 16:48

Refactoring globs' names.

9665be8

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Adjusting exception handling in proxy.

a5b4fec

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Addressing nits and indentation issues.

9cebbc7

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Adding missing get_service_response_details.

afcd59c

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Removing load_manifest from ros_loader.

63df0a1

- Assuming that message packages are already in the path. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

dirk-thomas mentioned this pull request May 16, 2019

ROS 2 Dashing Diademata ros2/ros2#607

Closed

20 tasks

jubeira changed the title ~~[WIP] Porting rosapi package.~~ Porting rosapi package. Jun 6, 2019

gonzodepedro approved these changes Jun 25, 2019

View reviewed changes

jubeira merged commit dfcf70f into RobotWebTools:ros2_ament_cmake Jun 25, 2019

jubeira deleted the ros2_port/rosapi_package branch June 25, 2019 20:23

jubeira pushed a commit that referenced this pull request Aug 22, 2019

Porting rosapi package. (#388)

d4eb55c

Porting rosapi package. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira pushed a commit that referenced this pull request Aug 22, 2019

Porting rosapi package. (#388)

9869364

Porting rosapi package. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira pushed a commit that referenced this pull request Aug 22, 2019

Porting rosapi package. (#388)

43467ea

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

travipross reviewed Aug 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting rosapi package. #388

Porting rosapi package. #388

jubeira commented Feb 22, 2019

jubeira commented Feb 28, 2019

dirk-thomas Mar 19, 2019

jubeira Apr 9, 2019

jubeira Jun 6, 2019

dirk-thomas commented Mar 19, 2019

jubeira commented Mar 19, 2019

jubeira commented Apr 12, 2019

jubeira commented Jun 6, 2019

gonzodepedro left a comment

travipross Aug 14, 2020

jubeira Aug 14, 2020

travipross Aug 19, 2020

Porting rosapi package. #388

Porting rosapi package. #388

Conversation

jubeira commented Feb 22, 2019

jubeira commented Feb 28, 2019

dirk-thomas Mar 19, 2019

Choose a reason for hiding this comment

jubeira Apr 9, 2019

Choose a reason for hiding this comment

jubeira Jun 6, 2019

Choose a reason for hiding this comment

dirk-thomas commented Mar 19, 2019

jubeira commented Mar 19, 2019

jubeira commented Apr 12, 2019

jubeira commented Jun 6, 2019

gonzodepedro left a comment

Choose a reason for hiding this comment

travipross Aug 14, 2020

Choose a reason for hiding this comment

jubeira Aug 14, 2020

Choose a reason for hiding this comment

travipross Aug 19, 2020

Choose a reason for hiding this comment