rclpy raw subscriptions

[josephduchesne]

Note: This implementation is incomplete and at the moment this PR exists primarily for getting feedback.
This is being done so that ros2cli#132 can implement "ros2 topic bw".

• Initial prototype
• Correct memory allocation implementation
• Add tests
• Add example to demos/demo_nodes_py

The general idea is to allow creating a raw subscription like so:

class ListenerRaw(Node):

    def __init__(self):
        super().__init__('listener')
        self.sub = self.create_subscription(String, 'chatter', self.chatter_callback, raw=True)

    def chatter_callback(self, msg):
        self.get_logger().info('I heard: [%s]' % msg)

This will call rcl_take_serialized_message rather than rcl_take internally, and return a python byte array representing the serialised message.

[INFO] [listener]: I heard: [b'\x00\x01\x00\x00\x05\x00\x00\x00Test\x00\x00\x00\x00']
(For the message "Test").

[josephduchesne]

I've updated the implementation to use rmw's allocator (I believe) properly. Let me know if this is a reasonable way to go about this and I'll round it out with tests and an example.

[clalancette]

The idea here looks good to me. This needs a rebase, and as you mentioned tests and examples, but I think we can go ahead with this.

[clalancette]

Marking this as "Request changes" until it is rebased and comments are fixed.

[josephduchesne]

Thanks for the review. I'll have a chance to work on the rebase, requested changes and tests this weekend.

[josephduchesne]

I think this PR is done now. I don't believe that this functionality requires a demo node, although I can happily add one. It's fairly esoteric and demo nodes are aimed at beginners.

Unit tests pass locally for the package, however the build farm test is failing. I think it's due to the build farm compiling against bouncy debs (specifically bouncy rcl), when this is built against and branched off of the latest master branch. Let me know if you believe that is my fault and I'll dig further to fix it.

[Karsten1987]

It generally looks good to me. One remark I'd have though is that the test is maybe not necessarily complete here. Just because the received message has a non-zero amount of bytes, doesn't necessarily mean the message is complete and correct.
In your opinion, how much effort would it be to expose the rcl_serialize and rcl_deserialize functions to this PR? This would allow to instantiate the message being published, serialize it and byte-wise compare the received message.

[Karsten1987]

Does this really test that the incoming message is complete and correct?

[josephduchesne]

It only partially tests the incoming message.

That said, it seems unlikely that it's incorrect given that the rclpy code does not manipulate the array. If a non-zero array is passed up, it means that rcl_take_serialized_message returned bad data, and it should have its own tests. I understand the desire for completeness though, and if we do expose deserialization to rclpy we can make this test more robust.

[Karsten1987]

@josephduchesne thanks for all the work done here.
I ran a first CI build on that:

• Linux
• Linux-aarch64
• macOS
• Windows

In the meantime, I am actually in favor of having a demo similar to the C++ version of it so that the demo packages don't diverge too much. I don't consider them only aiming at beginners, but also to literally demonstrate the capabilities of ROS 2 and their respective APIs.

[josephduchesne]

In your opinion, how much effort would it be to expose the rcl_serialize and rcl_deserialize functions to this PR? This would allow to instantiate the message being published, serialize it and byte-wise compare the received message.

I poked around the rcl code and it appears that this doesn't exist in rcl. The functions you are thinking of are probably rmw_serialize and rmw_deserialize. That raises some design questions.

How would these look:

rmw_ret_t
rmw_deserialize(
  const rmw_serialized_message_t * serialized_message,
  const rosidl_message_type_support_t * type_support,
void * ros_message)

string_message = rclpy.deserialize(input_bytes, std_msgs.msg.String)

rmw_ret_t
rmw_serialize(
  const void * ros_message,
  const rosidl_message_type_support_t * type_support,
  rmw_serialized_message_t * serialized_message)

output_bytes = rclpy.serialize(std_msgs_string_object)

Should I go ahead with this, or is this too much of a diversion from this original ticket?

It's not prohibitively complex, but I'm not sure the use case justifies the effort at the moment.

This would be required if someone wanted to implement something like ros1's rosbag (ros2bag doesn't use the rmw serialization system AFIK, and for good reasons- although it appears to me to result in an inevitable performance hit due to the abstraction). Other than that I don't think it provides much value.

[Karsten1987]

Should I go ahead with this, or is this too much of a diversion from this original ticket?
It's not prohibitively complex, but I'm not sure the use case justifies the effort at the moment.

I believe it makes sense to add this here, e.g. once you've received binary data, eventually the binary data should be converted to ROS messages again. This use case is also helpful for the tests of this PR.
We would appreciate if you spend a little bit more time on this, but it's entirely up to you :)

In case, the proposed python API for the rmw_serialize and rmw_deserialize look good to me. You could maybe stay consistent with the rclpy API and swap the arguments similar like here:

self.pub = self.create_publisher(String, 'chatter')

https://github.com/ros2/demos/blob/master/demo_nodes_py/demo_nodes_py/topics/talker.py#L26

With that the proposed API:

string_message = rclpy.deserialize(std_msgs.msg.String, input_bytes)
binary_message = rclpy.serialize(std_msgs.msg.String, message)

Let us know if you want to work a little bit more on this. Otherwise, we could go ahead and merge this and ticket the remaining tasks.

[dirk-thomas]

rclpy.serialize(std_msgs.msg.String, message)

For rclpy.serialize the first argument isn't necessary since it is implicitly available as type(message).

[josephduchesne]

Unfortunately, I don't think I'll have time in the next few weeks to work on further extending rclpy, and it would be nice to unblock the rostopic bw work that I originally wrote this for. If it's possible for this to be merged as is or with minimal changes, that would be nice.

I can try to do the rclpy.deserialize/rclpy.serialize work around the holidays, but that's probably about as much work as this PR by itself, again.

Concerns handled, clearing review for others to review.

[josephduchesne]

@Karsten1987 can this be merged?

[Karsten1987]

The current branch does not compile with the latest master version and has to be rebased as far as I can tell.
Then further, there was a recent change in the crystal API which takes the serialized messages with a buffer of type uint8_t and not as char *. This change has to be addressed here as well.

[josephduchesne]

Latest master branch merged in, compiled and tested in a clean source install of ros2 crystal (with the minor fixes performed). Also updated the sister request in ros2/demos#287 in the same way although it worked without changes after merging.

[Karsten1987]

Thanks for iterating over this.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

one little comment, but lgtm.

[Karsten1987]

should it return NULL in this case?

[josephduchesne]

It now returns NULL after decremented the python_bytes refcount in this error condition. If this is an adequate solution, I think this might finally be merge-able :)

[Karsten1987]

Thanks for iterating over this.