unsafe conv, fix exhaustive source

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.

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 <stelzo@steado.de>"]
 homepage = "https://github.com/stelzo/ros_pointcloud2"

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();

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 <stelzo@steado.de>"]
 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]

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<RPCL2Point<5>> 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),

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<String>),
     UnsupportedFieldCount,
     NumberConversion,
+    ExhaustedSource,
 }
 
 impl From<core::num::TryFromIntError> 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<N>,
     {
-        let point: RPCL2Point<N> = C::default().into();
-        debug_assert!(point.fields.len() == N);
-
         let field_names = ordered_field_names::<N, C>();
-        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<const N: usize> From<[PointData; N]> for RPCL2Point<N> {
 ///     }
 /// }
 ///
-/// 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<const N: usize> From<[PointData; N]> for RPCL2Point<N> {
 ///     }
 /// }
 /// ```
-pub trait PointConvertible<const N: usize>:
+/// # 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<const N: usize>:
     From<RPCL2Point<N>> + Into<RPCL2Point<N>> + Default + Sized
 {
     fn layout() -> LayoutDescription;

rpcl2/src/points.rs

@@ -160,7 +160,7 @@ impl From<PointXYZ> 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<PointXYZI> 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<PointXYZL> 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<PointXYZRGB> 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<PointXYZRGBA> 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<PointXYZRGBNormal> 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<PointXYZINormal> 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<PointXYZRGBL> 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<PointXYZNormal> for RPCL2Point<6> {
     }
 }
 
-impl PointConvertible<6> for PointXYZNormal {
+unsafe impl PointConvertible<6> for PointXYZNormal {
     fn layout() -> LayoutDescription {
         LayoutDescription::new(&[
             LayoutField::new("x", "f32", 4),

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<PointCloud2Msg, MsgConversionError> = 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),