From 94991c720b7a09e569e803a3a5daa894e8616827 Mon Sep 17 00:00:00 2001 From: stelzo Date: Fri, 21 Jun 2024 13:22:35 +0200 Subject: [PATCH] unsafe conv, fix exhaustive source --- CHANGELOG.md | 7 ++++ rpcl2-derive/Cargo.toml | 2 +- rpcl2-derive/src/lib.rs | 2 +- rpcl2/Cargo.toml | 4 +-- rpcl2/examples/custom_enum_field_filter.rs | 4 +-- rpcl2/src/lib.rs | 39 ++++++++++++++-------- rpcl2/src/points.rs | 18 +++++----- rpcl2/tests/e2e_test.rs | 29 +++++++++++----- 8 files changed, 68 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e6b8a0..56335a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## v0.5.0-rc.3 -> v0.5.0 + +- `PointConvertible` trait is now `unsafe` since the offset is used for raw memory access, where safety can not be guaranteed by the compiler. +- std::error -> core::error for nightly clippy +- Fixes a bug when attempting to write larger types than available in the message. This now results in a `ExhaustedSource` error. +- Adds `repr(C)` to docs where custom conversions are explained to encourage best practices for raw type descriptions. + ## v0.5.0-rc.2 -> v0.5.0-rc.3 - Bump r2r to 0.9. diff --git a/rpcl2-derive/Cargo.toml b/rpcl2-derive/Cargo.toml index 6aa4b0d..fb4be31 100644 --- a/rpcl2-derive/Cargo.toml +++ b/rpcl2-derive/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "rpcl2-derive" description = "Derive macros for ros_pointcloud2 crate." -version = "0.2.0" +version = "0.3.0" edition = "2021" authors = ["Christopher Sieh "] homepage = "https://github.com/stelzo/ros_pointcloud2" diff --git a/rpcl2-derive/src/lib.rs b/rpcl2-derive/src/lib.rs index dfc2dc5..56f5906 100644 --- a/rpcl2-derive/src/lib.rs +++ b/rpcl2-derive/src/lib.rs @@ -109,7 +109,7 @@ pub fn ros_point_derive(input: TokenStream) -> TokenStream { let layout = layout_of_type(&name, &input.data); let expanded = quote! { - impl #impl_generics ::ros_pointcloud2::PointConvertible<#field_len_token> for #name #ty_generics #where_clause { + unsafe impl #impl_generics ::ros_pointcloud2::PointConvertible<#field_len_token> for #name #ty_generics #where_clause { fn layout() -> ::ros_pointcloud2::LayoutDescription { let mut last_field_end = 0; let mut fields = Vec::new(); diff --git a/rpcl2/Cargo.toml b/rpcl2/Cargo.toml index 99b1f05..1dd1bec 100644 --- a/rpcl2/Cargo.toml +++ b/rpcl2/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ros_pointcloud2" -version = "0.5.0-rc.3" +version = "0.5.0" edition = "2021" authors = ["Christopher Sieh "] description = "Customizable conversions for working with sensor_msgs/PointCloud2." @@ -36,7 +36,7 @@ rosrust = { version = "0.9.11", optional = true } r2r = { version = "0.9", optional = true } rayon = { version = "1", optional = true } nalgebra = { version = "0.32", optional = true, default-features = false } -rpcl2-derive = { version = "0.2", optional = true, path = "../rpcl2-derive" } +rpcl2-derive = { version = "0.3", optional = true, path = "../rpcl2-derive" } memoffset = { version = "0.9", optional = true } [dev-dependencies] diff --git a/rpcl2/examples/custom_enum_field_filter.rs b/rpcl2/examples/custom_enum_field_filter.rs index 14af4c3..9ffe5c6 100644 --- a/rpcl2/examples/custom_enum_field_filter.rs +++ b/rpcl2/examples/custom_enum_field_filter.rs @@ -20,7 +20,7 @@ enum Label { // Define a custom point with an enum. // This is normally not supported by PointCloud2 but we will explain the library how to handle it. #[derive(Debug, PartialEq, Clone, Default)] -#[repr(C)] +#[repr(C, align(4))] struct CustomPoint { x: f32, y: f32, @@ -91,7 +91,7 @@ impl From> for CustomPoint { } // C representation of the struct hardcoded without using the derive feature. -impl PointConvertible<5> for CustomPoint { +unsafe impl PointConvertible<5> for CustomPoint { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), diff --git a/rpcl2/src/lib.rs b/rpcl2/src/lib.rs index 107e5c2..8069664 100644 --- a/rpcl2/src/lib.rs +++ b/rpcl2/src/lib.rs @@ -67,6 +67,7 @@ //! ## Derive (recommended) //! ```ignore //! #[derive(Clone, Debug, PartialEq, Copy, Default, PointConvertible)] +//! #[repr(C, align(4))] //! pub struct MyPointXYZI { //! pub x: f32, //! pub y: f32, @@ -81,6 +82,7 @@ //! use ros_pointcloud2::prelude::*; //! //! #[derive(Clone, Debug, PartialEq, Copy, Default)] +//! #[repr(C, align(4))] //! pub struct MyPointXYZI { //! pub x: f32, //! pub y: f32, @@ -106,7 +108,7 @@ //! } //! } //! -//! impl PointConvertible<4> for MyPointXYZI { +//! unsafe impl PointConvertible<4> for MyPointXYZI { //! fn layout() -> LayoutDescription { //! LayoutDescription::new(&[ //! LayoutField::new("x", "f32", 4), @@ -169,6 +171,7 @@ pub enum MsgConversionError { FieldsNotFound(Vec), UnsupportedFieldCount, NumberConversion, + ExhaustedSource, } impl From for MsgConversionError { @@ -212,13 +215,18 @@ impl core::fmt::Display for MsgConversionError { MsgConversionError::NumberConversion => { write!(f, "The number is too large to be converted into a PointCloud2 supported datatype.") } + MsgConversionError::ExhaustedSource => { + write!( + f, + "The conversion requests more data from the source type than is available." + ) + } } } } -#[cfg(feature = "std")] -impl std::error::Error for MsgConversionError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { +impl core::error::Error for MsgConversionError { + fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { None } } @@ -487,13 +495,9 @@ impl PointCloud2Msg { where C: PointConvertible, { - let point: RPCL2Point = C::default().into(); - debug_assert!(point.fields.len() == N); - let field_names = ordered_field_names::(); - debug_assert!(field_names.len() == N); - let target_layout = KnownLayoutInfo::try_from(C::layout())?; + debug_assert!(field_names.len() <= target_layout.fields.len()); debug_assert!(self.fields.len() <= target_layout.fields.len()); @@ -506,8 +510,12 @@ impl PointCloud2Msg { size, count, } => { - let msg_f = &self.fields[field_counter]; - let f_translated = &field_names[field_counter]; + if field_counter >= self.fields.len() || field_counter >= field_names.len() { + return Err(MsgConversionError::ExhaustedSource); + } + + let msg_f = unsafe { self.fields.get_unchecked(field_counter) }; + let f_translated = unsafe { field_names.get_unchecked(field_counter) }; field_counter += 1; if msg_f.name != *f_translated @@ -533,7 +541,7 @@ impl PointCloud2Msg { }) } - /// Create a [`PointCloud2Msg`] from any iterable type. + /// Create a [`PointCloud2Msg`] from any iterable type that implements [`PointConvertible`]. /// /// # Example /// ``` @@ -897,7 +905,7 @@ impl From<[PointData; N]> for RPCL2Point { /// } /// } /// -/// impl PointConvertible<4> for MyPointXYZL { +/// unsafe impl PointConvertible<4> for MyPointXYZL { /// fn layout() -> LayoutDescription { /// LayoutDescription::new(&[ /// LayoutField::new("x", "f32", 4), @@ -909,7 +917,10 @@ impl From<[PointData; N]> for RPCL2Point { /// } /// } /// ``` -pub trait PointConvertible: +/// # Safety +/// The layout is used for raw memory interpretation, where safety can not be guaranteed by the compiler. +/// Take care when implementing the layout, especially in combination with `#[repr]` or use the `derive` feature when possible to prevent common errors. +pub unsafe trait PointConvertible: From> + Into> + Default + Sized { fn layout() -> LayoutDescription; diff --git a/rpcl2/src/points.rs b/rpcl2/src/points.rs index 11f93c8..11d026e 100644 --- a/rpcl2/src/points.rs +++ b/rpcl2/src/points.rs @@ -160,7 +160,7 @@ impl From for RPCL2Point<3> { } } -impl PointConvertible<3> for PointXYZ { +unsafe impl PointConvertible<3> for PointXYZ { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -221,7 +221,7 @@ impl From for RPCL2Point<4> { } } -impl PointConvertible<4> for PointXYZI { +unsafe impl PointConvertible<4> for PointXYZI { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -282,7 +282,7 @@ impl From for RPCL2Point<4> { } } -impl PointConvertible<4> for PointXYZL { +unsafe impl PointConvertible<4> for PointXYZL { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -360,7 +360,7 @@ impl From for RPCL2Point<4> { } } -impl PointConvertible<4> for PointXYZRGB { +unsafe impl PointConvertible<4> for PointXYZRGB { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -442,7 +442,7 @@ impl From for RPCL2Point<5> { } } -impl PointConvertible<5> for PointXYZRGBA { +unsafe impl PointConvertible<5> for PointXYZRGBA { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -546,7 +546,7 @@ impl From for RPCL2Point<7> { } } -impl PointConvertible<7> for PointXYZRGBNormal { +unsafe impl PointConvertible<7> for PointXYZRGBNormal { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -637,7 +637,7 @@ impl From for RPCL2Point<7> { } } -impl PointConvertible<7> for PointXYZINormal { +unsafe impl PointConvertible<7> for PointXYZINormal { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -728,7 +728,7 @@ impl From for RPCL2Point<5> { } } -impl PointConvertible<5> for PointXYZRGBL { +unsafe impl PointConvertible<5> for PointXYZRGBL { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -805,7 +805,7 @@ impl From for RPCL2Point<6> { } } -impl PointConvertible<6> for PointXYZNormal { +unsafe impl PointConvertible<6> for PointXYZNormal { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), diff --git a/rpcl2/tests/e2e_test.rs b/rpcl2/tests/e2e_test.rs index 170cc24..923f6db 100644 --- a/rpcl2/tests/e2e_test.rs +++ b/rpcl2/tests/e2e_test.rs @@ -56,6 +56,19 @@ fn write_cloud() { let msg = PointCloud2Msg::try_from_iter(cloud); assert!(msg.is_ok()); } +/* +#[test] +fn collect_vec() { + let cloud = vec![ + PointXYZ::new(0.0, 1.0, 5.0), + PointXYZ::new(1.0, 1.5, 5.0), + PointXYZ::new(1.3, 1.6, 5.7), + PointXYZ::new(f32::MAX, f32::MIN, f32::MAX), + ] + .into_iter(); + + let msg: Result = cloud.collect(); +}*/ #[test] fn write_cloud_from_vec() { @@ -131,7 +144,7 @@ fn conv_cloud_par_par_iter() { #[cfg(feature = "derive")] fn custom_xyz_f32() { #[derive(Debug, PartialEq, Clone, Default)] - #[repr(C)] + #[repr(C, align(4))] struct CustomPoint { x: f32, y: f32, @@ -154,7 +167,7 @@ fn custom_xyz_f32() { } } - impl PointConvertible<3> for CustomPoint { + unsafe impl PointConvertible<3> for CustomPoint { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -216,7 +229,7 @@ fn custom_xyzi_f32() { ]; #[derive(Debug, PartialEq, Clone, Default)] - #[repr(C)] + #[repr(C, align(4))] struct CustomPointXYZI { x: f32, y: f32, @@ -247,7 +260,7 @@ fn custom_xyzi_f32() { } } - impl PointConvertible<4> for CustomPointXYZI { + unsafe impl PointConvertible<4> for CustomPointXYZI { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -266,7 +279,7 @@ fn custom_xyzi_f32() { #[cfg(feature = "derive")] fn custom_rgba_f32() { #[derive(Debug, PartialEq, Clone, Default)] - #[repr(C)] + #[repr(C, align(4))] struct CustomPoint { x: f32, y: f32, @@ -306,7 +319,7 @@ fn custom_rgba_f32() { } } - impl PointConvertible<7> for CustomPoint { + unsafe impl PointConvertible<7> for CustomPoint { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4), @@ -557,7 +570,7 @@ fn write_xyzi_read_xyz_vec() { #[test] fn write_less_than_available() { #[derive(Debug, PartialEq, Clone, Default)] - #[repr(C)] + #[repr(C, align(4))] struct CustomPoint { x: f32, y: f32, @@ -582,7 +595,7 @@ fn write_less_than_available() { } } - impl PointConvertible<3> for CustomPoint { + unsafe impl PointConvertible<3> for CustomPoint { fn layout() -> LayoutDescription { LayoutDescription::new(&[ LayoutField::new("x", "f32", 4),