Add support for rosidl::Buffer in rosidl C path for rclpy#943
Add support for rosidl::Buffer in rosidl C path for rclpy#943sloretz merged 10 commits intonative_buffer/2-rosidl_cppfrom
Conversation
| return py::isinstance<PyBuffer>(obj); | ||
| }, "Check if the given object is an rosidl_buffer.Buffer"); | ||
|
|
||
| m.def("_get_buffer_ptr", [](PyBuffer & buf) -> uintptr_t { |
There was a problem hiding this comment.
@nvcyc meta: how would one reach to the underlying memory safely if we type erase like this?
There was a problem hiding this comment.
_get_buffer_ptr is strictly internal plumbing. It's mainly used by the generated Python-to-C conversion code (_msg_support.c.em) during publish to hand the rosidl::Buffer<uint8_t>* back to the C sequence struct (where it is plain C code that can't easily handle typed rosidl::Buffer<uint8_t>*). We don't expect end users to call it.
There was a problem hiding this comment.
Yeah, I see now. We are basically using the C message as a vehicle for a type erased, heap allocated rosidl::Buffer. On publish you need to borrow. On callback (ie. on take), you need to transfer ownership from C to Python.
I can't say I love it but I understand it.
0f9fd11 to
986d123
Compare
e0d2f52 to
9d7675f
Compare
| { | ||
| if (buffer_->get_backend_type() == "cpu") { | ||
| return py::bytes( | ||
| reinterpret_cast<const char *>(buffer_->data()), |
There was a problem hiding this comment.
@nvcyc meta: I missed this the first time. Copying the entire buffer isn't great. Returning a memoryview or implemeting the array protocol for CPU buffers may be more appropriate.
There was a problem hiding this comment.
to_bytes() exists mainly for supporting rosidl_runtime_py (message_to_ordereddict, message_to_yaml) to serialize buffer data to dicts, YAML, and CSV.
For users to interact with a buffer, it should either be through
- backend vendor provided APIs
- converting to the CPU equivalent type in rclpy, which is
array.arrayvia to_arrary() if desired
| return py::isinstance<PyBuffer>(obj); | ||
| }, "Check if the given object is an rosidl_buffer.Buffer"); | ||
|
|
||
| m.def("_get_buffer_ptr", [](PyBuffer & buf) -> uintptr_t { |
There was a problem hiding this comment.
Yeah, I see now. We are basically using the C message as a vehicle for a type erased, heap allocated rosidl::Buffer. On publish you need to borrow. On callback (ie. on take), you need to transfer ownership from C to Python.
I can't say I love it but I understand it.
|
I would love to see some type stubs for the python Buffer class! |
fce6039 to
6375777
Compare
3085e51 to
f06ac85
Compare
b4e6596 to
4dcb08e
Compare
f06ac85 to
20ba671
Compare
4dcb08e to
897f0bc
Compare
20ba671 to
4623bdf
Compare
0a33311 to
9f1a637
Compare
4623bdf to
d04fd63
Compare
9f1a637 to
4a1fce2
Compare
d04fd63 to
863e827
Compare
4a1fce2 to
3a47ecd
Compare
863e827 to
c3f83ab
Compare
3a47ecd to
2ac2f2a
Compare
|
Does this CI also cover 942? |
|
Pulls: #943, ros2/rosidl_typesupport_fastrtps#144, ros2/rosidl_runtime_py#39, ros2/rosidl_python#250 |
|
Pulls: #943, ros2/rosidl_typesupport_fastrtps#144, ros2/rosidl_runtime_py#39, ros2/rosidl_python#250, ros2/rosidl_core#14, ros2/rmw_cyclonedds#575 |
wjwwood
left a comment
There was a problem hiding this comment.
With the other comments addressed, this lgtm
|
Pulls: #943, ros2/rosidl_typesupport_fastrtps#144, ros2/rosidl_runtime_py#39, ros2/rosidl_python#250, ros2/rosidl_core#14, ros2/rmw_cyclonedds#575 |
|
Pulls: #943, ros2/rosidl_typesupport_fastrtps#144, ros2/rosidl_runtime_py#39, ros2/rosidl_python#250, ros2/rosidl_core#14, ros2/rmw_cyclonedds#575 |
|
Triggered with custom ros2.repos that contain all PRs: https://raw.githubusercontent.com/nvcyc/ros2/refs/heads/rolling-native-buffer-prs/ros2.repos Started by user CY Chen |
2bd85fd to
3a8edd9
Compare
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
Signed-off-by: CY Chen <cyc@nvidia.com>
6d21fcd to
22ffbb9
Compare
|
Pulls: #943, ros2/rosidl_typesupport_fastrtps#144, ros2/rosidl_runtime_py#39, ros2/rosidl_python#250, ros2/rosidl_core#14, ros2/rmw_fastrtps#867 |
|
Windows CI minus zenoh is green, and all 6 PRs are aproved, so I'm merging them |
* Update rosidl cpp path to emit rosidl::Buffer for uint8[] type Signed-off-by: CY Chen <cyc@nvidia.com> * Remove unused has_buffer_fields_ field Signed-off-by: CY Chen <cyc@nvidia.com> * Add support for rosidl::Buffer in rosidl C path for rclpy (#943) * Add support for rosidl::Buffer in rosidl C path for rclpy Signed-off-by: CY Chen <cyc@nvidia.com> * Add rosidl_buffer_py to rosidl_runtime_packages group Signed-off-by: CY Chen <cyc@nvidia.com> * Remove unused has_buffer_fields_ field from introspection C Signed-off-by: CY Chen <cyc@nvidia.com> * Simplify rosidl_buffer Python bindings Signed-off-by: CY Chen <cyc@nvidia.com> * Add owns_rosidl_buffer in sequence struct to handle buffer lifecycle Signed-off-by: CY Chen <cyc@nvidia.com> * Add Python type stub for Buffer class Signed-off-by: CY Chen <cyc@nvidia.com> * Remove unused test assertion for introspection Signed-off-by: CY Chen <cyc@nvidia.com> * Add comments for the limitation of _take_buffer_from_ptr Signed-off-by: CY Chen <cyc@nvidia.com> * Revert adding a line in msg__functions.c.em Signed-off-by: CY Chen <cyc@nvidia.com> * Update rosidl_buffer/c_helpers.h to use the right comment style Signed-off-by: CY Chen <cyc@nvidia.com> --------- Signed-off-by: CY Chen <cyc@nvidia.com> --------- Signed-off-by: CY Chen <cyc@nvidia.com>
Description
This pull request makes the rosidl C layer Buffer-aware and adds Python bindings for
rosidl::Buffer, enabling rclpy message types to work with vendor-backed buffer memory.This pull request consists of the following key changes:
rosidl_runtime_c: Addsis_rosidl_bufferfield (bool) in each primitive sequence struct (e.g.,rosidl_runtime_c__uint8__Sequence). When true,datapoints to anrosidl::Buffer<T>*.rosidl_generator_c: For messages withuint8[]fields, the generated fini logic (msg__functions.c.em) checksmsg->field.is_rosidl_bufferand clears the sequence fields without calling normal sequence fini, avoiding double-free of a borrowedrosidl::Buffer*used by the Python C extension.rosidl_typesupport_introspection_c: The goal for the changes in C introspection is to keep it working with CPU-based path. The introspection accessors throw exceptions for non-CPU backends.rosidl_buffer_py(new): Python bindings (pybind11) forrosidl::Buffer.Is this user-facing behavior change?
This pull request does not change existing rclpy behavior. The
is_rosidl_bufferflag defaults to false, so all existing C sequence code paths are unchanged. Therosidl_buffer_py.Bufferclass is new and only used when vendor-backed (non-CPU) buffers are received by rclpy subscribers. CPU-based data continues to be delivered asarray.array.Did you use Generative AI?
Yes. Claude (claude-4.6-opus) via Cursor was used to assist with creating an initial prototype version of the changes contained in this PR.
Additional Information
This PR is part of the broader ROS 2 native buffer feature introduced in this post.
It depends on PRs #941, #942 and ros2/rosidl_typesupport_fastrtps#144 (core buffer types, C++ rosidl changes, and C++ serialization).