diff --git a/CHANGELOG.md b/CHANGELOG.md index aeac997..f23bc4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Internal integral type Time changed from u32 to i32 representation to better align with ROS1 +- Conversions between ROS Time and Duration to std::time::Time and std::time::Duration switched to TryFrom as they can be fallible. + ## 0.11.1 ### Added diff --git a/roslibrust_codegen/src/integral_types.rs b/roslibrust_codegen/src/integral_types.rs index 736c419..9a98400 100644 --- a/roslibrust_codegen/src/integral_types.rs +++ b/roslibrust_codegen/src/integral_types.rs @@ -1,8 +1,17 @@ +use simple_error::{bail, SimpleError}; + use crate::RosMessageType; /// Matches the integral ros1 type time, with extensions for ease of use /// NOTE: in ROS1 "Time" is not a message in and of itself and std_msgs/Time should be used. /// However, in ROS2 "Time" is a message and part of builtin_interfaces/Time. +// Okay some complexities lurk here that I really don't like +// In ROS1 time is i32 secs and i32 nsecs +// In ROS2 time is i32 secs and u32 nsecs +// How many nsecs are there in a sec? +1e9 which will fit inside of either. +// But ROS really doesn't declare what is valid for nsecs larger than 1e9, how should that be handled? +// How should negative nsecs work anyway? +// https://docs.ros2.org/foxy/api/builtin_interfaces/msg/Time.html #[derive(:: serde :: Deserialize, :: serde :: Serialize, Debug, Default, Clone, PartialEq)] pub struct Time { // Note: rosbridge appears to accept secs and nsecs in for time without issue? @@ -10,22 +19,55 @@ pub struct Time { // This alias is required for ros2 where field has been renamed #[serde(alias = "sec")] - pub secs: u32, + pub secs: i32, // This alias is required for ros2 where field has been renamed #[serde(alias = "nanosec")] - pub nsecs: u32, + pub nsecs: i32, } -impl From for Time { - fn from(val: std::time::SystemTime) -> Self { - let delta = val - .duration_since(std::time::UNIX_EPOCH) - .expect("Failed to convert system time into unix epoch"); - let downcast_secs = u32::try_from(delta.as_secs()).expect("Failed to convert system time to ROS representation, seconds term overflows u32 likely"); - Time { +/// Provide a standard conversion between ROS time and std::time::SystemTime +impl TryFrom for Time { + type Error = SimpleError; + fn try_from(val: std::time::SystemTime) -> Result { + let delta = match val.duration_since(std::time::UNIX_EPOCH) { + Ok(delta) => delta, + Err(e) => bail!("Failed to convert system time into unix epoch: {}", e), + }; + // TODO our current method doesn't try to handel negative times + // It is unclear from ROS documentation how these would be generated or how they should be handled + // For now adopting a strict conversion policy of only converting when it makes clear logical sense + let downcast_secs = match i32::try_from(delta.as_secs()) { + Ok(val) => val, + Err(e) => bail!("Failed to convert seconds to i32: {e:?}"), + }; + let downcast_nanos = match i32::try_from(delta.subsec_nanos()) { + Ok(val) => val, + Err(e) => bail!("Failed to convert nanoseconds to i32: {e:?}"), + }; + Ok(Time { secs: downcast_secs, - nsecs: delta.subsec_nanos(), - } + nsecs: downcast_nanos, + }) + } +} + +/// Provide a standard conversion between ROS time and std::time::SystemTime +impl TryFrom