[Service Introspection] Support deserializing bytes type data for echoing

[deepanshubansal01]

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

This PR modifies the existing converter utility functions to support deserializing of bytes/bytesarray data type. This is useful for deserializing the service contents and echoing service contents to the user. Depends on ros2/ros2cli#732

Connects to: ros2/ros2#1285

Unrelated to Service Introspection REP:
The proposed changes can also be used to support deserializing of contents in ros2 topic echo by supporting an optional argument for --deserialize_msg_type, the utility can then be used to deserialize the bytes type to the user specified msg type.

Signed-off-by: deepanshu deepanshubansal01@gmail.com

[jacobperron]

This adds a new dependency on rclpy. I guess this doesn't create a dependency cycle, but IMO, it seems a bit strange to add a dependency on a client library to a rosidl package (it is usually the other way around).

I'm not sure I understand the point about deserializing values in ros2 topic echo. All values being echo'd are already deserialized, and the user can opt-in to echoing serialized data with the --raw option. Note, we decided to handle that case separately from the utility functions in rosidl_runtime_py:

https://github.com/ros2/ros2cli/blob/c7d3204aad16b6d9f10d2b2dd26a04d5d88f6b23/ros2topic/ros2topic/verb/echo.py#L304-L311

I think the case where a message field has a serialized message as the value is a pretty niche case and not worth the added complexity of handling here (or in ros2topic). For service introspection, I would just do the deserialization directly in ros2service.

[deepanshubansal01]

I see, make sense and adding a dependency on a client library to a rosidl package is indeed strange I guess, didn't thought about it. I will close this PR and iterate on the service echo PR to do deserialisation their itself.

I think one of the main reasons for using the existing conversion pipeline was that message_to_yaml and message_to_csv functions take care of the optional arguments such as --no-arr,--no-stretc, which we can piggy bank on for service echo but their should be a work around for this without touching the rosidl_runtime_py I guess.

Thanks for the review!

[jacobperron]

That's a good point. After looking at ros2/ros2cli#732, I see why you chose this solution.

Maybe we can still use the utilities in rosidl_runtime_py, but print out the deserialized request/response separately.

[sloretz]

This adds a new dependency on rclpy. I guess this doesn't create a dependency cycle, but IMO, it seems a bit strange to add a dependency on a client library to a rosidl package (it is usually the other way around).

From the name of this package I'm surprised it didn't create a dependency cycle, but I'm not super familiar with where this is used.

How about moving serialize_message and deserialize_message into rosidl_runtime_py? They seem like they fit with this package's description.

rosidl_runtime_py/package.xml

Line 6 in 092b6ed

<description>Runtime utilities for working with generated ROS interfaces in Python.</description>

[deepanshubansal01]

From the name of this package I'm surprised it didn't create a dependency cycle, but I'm not super familiar with where this is used.

yeah, we can do that makes sense. I checked further and seems like serialize_message and desrialize_message from rclpy are used mainly in rclpy itself and rosbag2 hence moving them to rosidl_runtime_py shouldn't be too painful.

One thing to note maybe is that a corresponding serialize_message and desrialize_message methods exists for rclcpp as well and maybe they are in rclpy for consistency. Not sure just wanted to point this out:)

@jacobperron any thoughts on this one

[sloretz]

One thing to note maybe is that a corresponding serialize_message and desrialize_message methods exists for rclcpp as well and maybe they are in rclpy for consistency.

I'd recommend leaving the API's in rclpy, but make them use the ones in rosidl_runtime_py. That avoids downstream breakage.

[deepanshubansal01]

One thing to note maybe is that a corresponding serialize_message and desrialize_message methods exists for rclcpp as well and maybe they are in rclpy for consistency.

I'd recommend leaving the API's in rclpy, but make them use the ones in rosidl_runtime_py. That avoids downstream breakage.

I see, makes sense. Thanks!