Convert serialize/deserialize to pybind11 by sloretz · Pull Request #712 · ros2/rclpy

sloretz · 2021-03-11T18:07:59Z

part of #665

sloretz · 2021-03-11T18:09:24Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-03-11T18:53:20Z

Rebased on master

cottsay

Needs some fixes for error handling.

cottsay · 2021-03-16T00:27:50Z

rclpy/src/rclpy/serialization.cpp

+  if (RMW_RET_OK != rmw_ret) {
+    throw RCLError("Failed to serialize ROS message");
+  }


This isn't the right way to handle RMW errors. There are explicit functions defined for it, but they're all just typedefs to rcutils error handling. It would be OK with me if you just switched this to rcutils error handling, but I think the more correct way to do it would be to introduce an RMWError exception class like you did for RCUtilsError.

Fully agreed.

I'll update this to use RMWError from #709 once that's merged.

Uses RMWError in 5204de1

cottsay · 2021-03-16T00:32:03Z

rclpy/src/rclpy/serialization.cpp

+  auto message_deleter = [&](void * ptr) {destroy_ros_message(ptr);};
+  auto deserialized_ros_msg = std::unique_ptr<void, decltype(message_deleter)>(
+    deserialized_ros_msg_c, message_deleter);


I think that in this case, you can pass destroy_ros_message directly as the deleter. Up to you.

Suggested change

auto message_deleter = [&](void * ptr) {destroy_ros_message(ptr);};

auto deserialized_ros_msg = std::unique_ptr<void, decltype(message_deleter)>(

deserialized_ros_msg_c, message_deleter);

auto deserialized_ros_msg = std::unique_ptr<void, decltype(destroy_ros_message)>(

deserialized_ros_msg_c, destroy_ros_message);

Removed unnecessary lambda in 801d7db

cottsay · 2021-03-16T00:32:29Z

rclpy/src/rclpy/serialization.cpp

+  rmw_ret_t rmw_ret = rmw_deserialize(&serialized_msg, ts, deserialized_ros_msg.get());
+
+  if (RMW_RET_OK != rmw_ret) {
+    throw RCLError("failed to deserialize ROS message");


Again with the RMW error handling.

Uses RMWError in 5204de1

hidmic · 2021-03-16T16:57:09Z

rclpy/src/rclpy/serialization.cpp

+
+extern "C"
+{
+#include "rclpy_common/common.h"


@sloretz this one's curious, why the extern "C" block? Doesn't rclpy_common/common.h include one already?

It didn't have extern "C". I added it in b51f10f

hidmic · 2021-03-16T16:58:35Z

rclpy/src/rclpy/serialization.cpp

+  {
+    rcutils_ret_t ret = rmw_serialized_message_fini(&rcl_msg_);
+    if (RCUTILS_RET_OK != ret) {
+      fprintf(


@sloretz FYI there's RCUTILS_SAFE_FWRITE_TO_STDERR too.

Uses rcutils stderr macro in 2c8745c

hidmic · 2021-03-16T17:03:17Z

rclpy/src/rclpy/serialization.hpp

+
+/// Deserialize a ROS message
+/**
+ * Raises RCLError on serialization failure


@sloretz nit:

Suggested change

* Raises RCLError on serialization failure

* Raises RCLError on deserialization failure

deserialization in b174caa

hidmic · 2021-03-16T17:04:08Z

rclpy/src/rclpy/serialization.hpp

+/// Serialize a ROS message
+/**
+ * Raises RCLError on serialization failure
+ * Raises TypeError if an argument has the wrong type


@sloretz meta: isn't this true for any Python code, native or interpreted?

Hmm maybe it's not true in this case since the args are both py::object 😅 . It's true for any method we bind with a specific type using pybind11, so probably it can be assumed rather than documented. I removed it in 3fa151c.

hidmic · 2021-03-16T17:05:36Z

rclpy/src/rclpy/serialization.cpp

+  }
+  serialized_msg.buffer_capacity = length;
+  serialized_msg.buffer_length = length;
+  serialized_msg.buffer = reinterpret_cast<uint8_t *>(serialized_buffer);


@sloretz nit: why not using serialized_msg members in place instead of having these serialized_buffer and length temporary variables?

Unsigned/signed conversion - speaking of which I added a check that length is greater than 0 before casting in 9efb9cc.

hidmic · 2021-03-16T17:06:15Z

rclpy/src/rclpy/serialization.cpp

+  serialized_msg.buffer_length = length;
+  serialized_msg.buffer = reinterpret_cast<uint8_t *>(serialized_buffer);
+
+  destroy_ros_message_signature * destroy_ros_message = NULL;


@sloretz nit:

Suggested change

destroy_ros_message_signature * destroy_ros_message = NULL;

destroy_ros_message_signature * destroy_ros_message = nullptr;

NULL -> nullptr in 0b5b9b1

hidmic · 2021-03-16T17:08:03Z

rclpy/src/rclpy/serialization.cpp

+  }
+
+  void
+  init(rcutils_allocator_t allocator)


@sloretz why a two-stage construction? Why not merging SerializedMessage::init with the constructor?

I think I had it as two-stage originally so I could reuse it in the deserialize method, but ended up not going with it to avoid a copy. I merged init and the constructor in d2d3c29

hidmic · 2021-03-16T17:13:45Z

rclpy/src/rclpy/serialization.cpp

+
+namespace rclpy
+{
+class SerializedMessage


@sloretz meta: perhaps a struct is more adequate?

Made it a struct in 5fc0e07

hidmic · 2021-03-16T17:14:36Z

rclpy/src/rclpy/serialization.cpp

+    }
+  }
+
+  rcl_serialized_message_t rcl_msg_;


@sloretz meta: given it's a public member, shall we drop the trailing underscore?

I didn't find anything in the style guide about trailing underscore's only being for private member variables, but it says structs don't have them. I converted it to a struct and removed the trailing underscore in 5fc0e07

sloretz · 2021-03-17T22:49:40Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Feedback addressed

sloretz · 2021-03-18T16:41:24Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz requested review from azeey and cottsay March 11, 2021 18:07

sloretz mentioned this pull request Mar 11, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

sloretz force-pushed the pybind11_serialize branch from 41b3514 to ab53412 Compare March 11, 2021 18:53

cottsay previously requested changes Mar 16, 2021

View reviewed changes

hidmic reviewed Mar 16, 2021

View reviewed changes

hidmic mentioned this pull request Mar 16, 2021

Convert init/shutdown to pybind11 #715

Merged

sloretz force-pushed the pybind11_serialize branch from ab53412 to 801d7db Compare March 17, 2021 22:03

sloretz requested review from cottsay and hidmic March 17, 2021 23:30

sloretz added 11 commits March 18, 2021 08:57

Convert serialize/deserialize to pybind11

1f77c6e

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Single stage construction of SerializedMessage

7c82680

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

SerializedMessage is now a struct

66bc229

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

NULL -> nullptr

152cde8

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

serialization -> deserialization

cee5e80

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Use RMWError

c485fa7

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Types and exception when converting bytes

593cdef

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Remove redundant redundant lambda

fef98ea

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Fix exceptions documentation

307dc9a

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

rcutils stderr writing

c274d4c

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Move extern "C" to common.h

787d3a6

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the pybind11_serialize branch from b51f10f to 787d3a6 Compare March 18, 2021 15:57

hidmic approved these changes Mar 18, 2021

View reviewed changes

sloretz merged commit 84e467b into master Mar 18, 2021

delete-merged-branch bot deleted the pybind11_serialize branch March 18, 2021 20:29

clalancette mentioned this pull request Mar 19, 2021

Convert names_and_types graph APIs to pybind11 #717

Merged

iuhilnehc-ynos mentioned this pull request Feb 18, 2022

[Foxy] Memory leak in rclpy.serialization.serialize_message() #897

Closed

	* Raises RCLError on serialization failure
	* Raises RCLError on deserialization failure

	destroy_ros_message_signature * destroy_ros_message = NULL;
	destroy_ros_message_signature * destroy_ros_message = nullptr;

Conversation

sloretz commented Mar 11, 2021

Uh oh!

sloretz commented Mar 11, 2021

Uh oh!

sloretz commented Mar 11, 2021

Uh oh!

cottsay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Mar 17, 2021

Uh oh!

sloretz commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants