Porting rosbridge library (capabilities and internal) to ROS2. #419

jubeira · 2019-07-16T19:56:25Z

With this patch, all the code from the entrypoint (Websocket server) up to the internal libraries should be ROS2-compatible, and no references to rclpy or ROS1 code should remain.

Details

All the ROS1 code that involves publishers, subscribers and services now need a rclpy.node.Node handle to work.
RosbridgeWebsocketNode in rosbridge_websocket module was converted to a ROS2 node because it's near the entry point of the application. A reference to it shall be used to generate publishers, subscribers and service clients where necessary.
Besides routing the node handle to all the points of the program where it was required (the deepest point from the entrypoint is the internal package), there were a few features from ROS1 that changed a bit, so workarounds had to be found:
- In publishers module inside internal, a ROS1 SubscriberListener was used to prevent late-joining subscribers from missing messages. This is now achieved using transient local QoS with a lifespan.
- At the same time, there was support for latched topics. This now applies for publishers and subscribers, so transient local is the QoS of choice for subscribers as well.
- In subscribers module, additional subscriber callbacks were used to send latched messages to new (web) subscribers using only one ROS subscriber. Since this is not available in ROS2 (at least not that I know of), a workaround was implemented and tested in a minimal example.
In message_conversion, the parameters don't seem to be used in the original code. For a first iteration, using defaults should work.

jubeira · 2019-07-16T19:57:32Z

@ivanpauno I can't add you as a reviewer, but would you please take an initial look?

/cc @dirk-thomas

ivanpauno

I left a bunch of comments. It's looking good.

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

ivanpauno · 2019-07-17T17:35:00Z

rosbridge_library/src/rosbridge_library/internal/publishers.py

        """ Register a publisher on the specified topic.

        Keyword arguments:
        topic    -- the name of the topic to register the publisher to
+        node_handle -- Handle to a rclpy node to create the publisher.
        msg_type -- (optional) the type to register the publisher as.  If not
        provided, an attempt will be made to infer the topic type
        latch    -- (optional) if a client requested this publisher to be latched,


latch is not actually the keyword name. queue_size is undocumented.
That patch could be send to master.

rosbridge_library/src/rosbridge_library/internal/publishers.py

rosbridge_server/scripts/rosbridge_websocket.py

ivanpauno · 2019-07-17T21:22:32Z

rosbridge_server/scripts/rosbridge_websocket.py

+            for element in params_glob[1:-1].split(',')
+            if len(element.strip().strip("'")) > 0]
+
+        if "--port" in sys.argv:


I would consider start using argparse.

Maybe, that can be done in the original code too.

Yes, this seems to be like a manually implemented argparse. I'd say it's out of scope of this PR.

- Connecting to main entrypoint where necessary to get node handles. - Updating to Dashing API. - Initial E2E tests passing for topics, services and parameters. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira · 2019-08-24T22:48:21Z

This was rebased against develop; the latest changes were ported to ROS2 as well.

Initial E2E tests are passing for topics, services and parameters using standard roslibjs. This is ready for another review.

ivanpauno

I left some comments and questions, otherwise LGTM.

How will this be tested? Will this repo be added to ros2 build farm or will it be tested in another place?

ivanpauno · 2019-08-26T14:21:04Z

rosapi/srv/DeleteParam.srv

@@ -1,3 +1,2 @@
-string node_name
 string name
 ---


Unrelated, but add new line before EOF (develop should be corrected too).

ivanpauno · 2019-08-26T14:21:30Z

rosapi/srv/GetParamNames.srv

@@ -1,3 +1,2 @@
-string node_name
 ---
 string[] names


ivanpauno · 2019-08-26T14:21:38Z

rosapi/srv/HasParam.srv

@@ -1,4 +1,3 @@
-string node_name
 string name
 ---
 bool exists


ivanpauno · 2019-08-26T14:21:44Z

rosapi/srv/SetParam.srv

@@ -1,4 +1,3 @@
-string node_name
 string name
 string value
 ---


ivanpauno · 2019-08-26T14:34:21Z

rosapi/scripts/rosapi_node

        self.globs = self.get_globs()
        self.register_services()

    # Initialises the ROS node
    def register_services(self):
        proxy.init(self)
-        params.init()
+        params.init(self.NAME)


Node name could have been remapped, use self.get_name, probably combined with self.get_namespace for getting the full name (get_fully_qualified_name method should be added to rclpy Node).

rclcpp Node class has a get_fully_qualified_name method

+1.
The code in the node already needs that internally:
https://github.com/ros2/rclpy/blob/0.7.5/rclpy/rclpy/node.py#L682-L686; a helper method could be a good idea.

ivanpauno · 2019-08-26T14:52:38Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

@@ -51,7 +51,7 @@
    "int":     ["int8", "byte", "uint8", "char",
                "int16", "uint16", "int32", "uint32",
                "int64", "uint64", "float32", "float64"],
-    "float":   ["float32", "float64"],
+    "float":   ["float32", "float64", "double"],


Why has double been added?

That's the type you get in a ROS2 message when analyzing slots and types.
E.g. for a geometry_msgs/Vector3:

__slots__ = [ '_x', '_y', '_z', ] _fields_and_field_types = { 'x': 'double', 'y': 'double', 'z': 'double', }

ivanpauno · 2019-08-26T15:06:14Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

@@ -165,7 +198,7 @@ def _from_list_inst(inst, rostype):
        return []

    # Remove the list indicators from the rostype
-    rostype = list_braces.sub("", rostype)
+    rostype = re.search(list_tokens, rostype).group(1)

    # Shortcut for primitives
    if rostype in ros_primitive_types and not rostype in ["float32", "float64"]:


ivanpauno · 2019-08-26T15:11:41Z

rosbridge_library/src/rosbridge_library/internal/ros_loader.py

@@ -199,6 +199,8 @@ def _splittype(typestring):
    splits = [x for x in typestring.split("/") if x]
    if len(splits) == 2:
        return splits
+    elif len(splits) == 3:
+        return (splits[0], splits[2])


return [splits[0], splits[2]
for consistency, always return a list.

ivanpauno · 2019-08-26T15:27:38Z

rosbridge_msgs/package.xml

+  <buildtool_depend>rosidl_default_generators</buildtool_depend>
+
+  <exec_depend>rclpy</exec_depend>
+  <exec_depend>rcl_interfaces</exec_depend>


I think there is a build_depend on builtin_interfaces and there is no dependency on rcl_interfaces, right?

ivanpauno · 2019-08-26T15:33:09Z

rosbridge_server/src/rosbridge_server/client_mananger.py

 from rosbridge_msgs.msg import ConnectedClient, ConnectedClients
 from std_msgs.msg import Int32


 class ClientManager:
-    def __init__(self):
+    def __init__(self, node_handle):
+        qos = QoSProfile(depth=10,


Consider depth=1. I think that in ROS 1 with a latched topic and depth=10, you only got the last message in a late subscriber.
But in ROS 2 transient local durability with depth=10 will result in late subscribers receiving the last 10 messages.

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

jubeira · 2019-09-02T17:34:41Z

@gonzodepedro would you please take one more look? Thanks!

gonzodepedro

LGTM

jubeira · 2019-09-09T17:07:01Z

Thanks @gonzodepedro !

jubeira self-assigned this Jul 16, 2019

ivanpauno reviewed Jul 17, 2019

View reviewed changes

wjwwood mentioned this pull request Jul 23, 2019

ROS 2 Eloquent Elusor ros2/ros2#734

Closed

34 tasks

jubeira force-pushed the ros2_ament_cmake branch from dfcf70f to d4eb55c Compare August 22, 2019 18:39

jubeira changed the base branch from ros2_ament_cmake to ros2 August 22, 2019 18:41

jubeira force-pushed the ros2 branch from 9869364 to 43467ea Compare August 22, 2019 19:19

Porting rosbridge library (capabilities and internal) to ROS2.

184b462

- Connecting to main entrypoint where necessary to get node handles. - Updating to Dashing API. - Initial E2E tests passing for topics, services and parameters. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira force-pushed the ros2_port/rosbridge_library_package branch from 9161521 to 184b462 Compare August 24, 2019 22:44

ivanpauno reviewed Aug 26, 2019

View reviewed changes

Addressing comments.

02deb34

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

gonzodepedro approved these changes Sep 6, 2019

View reviewed changes

jubeira merged commit 8dea92f into RobotWebTools:ros2 Sep 9, 2019

jubeira deleted the ros2_port/rosbridge_library_package branch September 9, 2019 17:06

jtbandes mentioned this pull request Aug 19, 2021

Fix wildcard imports and unused variables #609

Merged

kenji-miyake mentioned this pull request Feb 12, 2022

Fix time behavior #691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting rosbridge library (capabilities and internal) to ROS2. #419

Porting rosbridge library (capabilities and internal) to ROS2. #419

jubeira commented Jul 16, 2019 •

edited

Loading

jubeira commented Jul 16, 2019

ivanpauno left a comment

ivanpauno Jul 17, 2019

ivanpauno Jul 17, 2019

ivanpauno Jul 17, 2019

jubeira Jul 24, 2019

jubeira commented Aug 24, 2019

ivanpauno left a comment

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

jubeira Aug 27, 2019

ivanpauno Aug 26, 2019

jubeira Aug 27, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

ivanpauno Aug 26, 2019

jubeira commented Sep 2, 2019

gonzodepedro left a comment

jubeira commented Sep 9, 2019

Porting rosbridge library (capabilities and internal) to ROS2. #419

Porting rosbridge library (capabilities and internal) to ROS2. #419

Conversation

jubeira commented Jul 16, 2019 • edited Loading

jubeira commented Jul 16, 2019

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira commented Aug 24, 2019

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira commented Sep 2, 2019

gonzodepedro left a comment

Choose a reason for hiding this comment

jubeira commented Sep 9, 2019

jubeira commented Jul 16, 2019 •

edited

Loading