diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index 8fdccad7..dee1ffff 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -1420,6 +1420,12 @@ impl NodeState { self.parameter.declare(name.into()) } + /// Access to this node's parameter interface, for the parameter implementation itself. + #[cfg(test)] + pub(crate) fn parameter_interface(&self) -> &ParameterInterface { + &self.parameter + } + /// Enables usage of undeclared parameters for this node. /// /// Returns a [`Parameters`] struct that can be used to get and set all parameters. diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs index 1ff3415a..5b5b0828 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -1,11 +1,13 @@ mod override_map; mod range; mod service; +mod std_types; mod value; pub(crate) use override_map::*; pub use range::*; use service::*; +pub use std_types::*; pub use value::*; use ros_env::rcl_interfaces::msg::rmw::{ParameterType, ParameterValue as RmwParameterValue}; @@ -50,9 +52,17 @@ struct ParameterOptionsStorage { impl From> for ParameterOptionsStorage { fn from(opts: ParameterOptions) -> Self { + // A declaration that says nothing about constraints inherits whatever the parameter's + // type has to say about itself, so that introspection is useful without every + // declaration site restating the type's own rules. + let constraints = if opts.constraints.is_empty() { + T::type_constraints().unwrap_or(opts.constraints) + } else { + opts.constraints + }; Self { description: opts.description, - constraints: opts.constraints, + constraints, ranges: opts.ranges.into(), } } @@ -278,9 +288,9 @@ type DiscriminatorFunction<'a, T> = Box) -> Option /// Wraps a typed validate callback into a type-erased one that operates on `ParameterValue`. /// -/// The `expect` here is safe: this callback is only invoked from -/// `validate_parameter_setting` which checks the type discriminant first, -/// and from `Parameters::set()` which checks `T::kind() == param.kind`. +/// The `expect` here is safe: this callback is only invoked from `validate_parameter_setting` +/// and `Parameters::set()`. Both of them run the declaration's `type_check`, which is this same +/// conversion, before reaching it. fn wrap_validate_callback( callback: Arc Result<(), String> + Send + Sync>, ) -> ValidateCallback { @@ -325,11 +335,15 @@ impl TryFrom> for OptionalParameter builder.interface.store_parameter( builder.name.clone(), - T::kind(), - DeclaredValue::Optional(value.clone()), - builder.options.into(), - type_erased_validate, - Some(change_tx.clone()), + DeclaredStorage { + value: DeclaredValue::Optional(value.clone()), + kind: T::kind(), + options: builder.options.into(), + type_check: type_check_of::, + validate: type_erased_validate, + on_change: None, + change_tx: Some(change_tx.clone()), + }, ); Ok(OptionalParameter { name: builder.name, @@ -412,11 +426,15 @@ impl TryFrom> for MandatoryParamete builder.interface.store_parameter( builder.name.clone(), - T::kind(), - DeclaredValue::Mandatory(value.clone()), - builder.options.into(), - type_erased_validate, - Some(change_tx.clone()), + DeclaredStorage { + value: DeclaredValue::Mandatory(value.clone()), + kind: T::kind(), + options: builder.options.into(), + type_check: type_check_of::, + validate: type_erased_validate, + on_change: None, + change_tx: Some(change_tx.clone()), + }, ); Ok(MandatoryParameter { name: builder.name, @@ -519,11 +537,17 @@ impl TryFrom> for ReadOnlyParameter let value = initial_value.into(); builder.interface.store_parameter( builder.name.clone(), - T::kind(), - DeclaredValue::ReadOnly(value.clone()), - builder.options.into(), - None, - None, + DeclaredStorage { + value: DeclaredValue::ReadOnly(value.clone()), + kind: T::kind(), + options: builder.options.into(), + type_check: type_check_of::, + // A read-only parameter never changes, so it needs neither the validate + // callback nor a change notification channel. + validate: None, + on_change: None, + change_tx: None, + }, ); Ok(ReadOnlyParameter { name: builder.name, @@ -537,10 +561,31 @@ impl TryFrom> for ReadOnlyParameter type ValidateCallback = Arc Result<(), String> + Send + Sync>; type OnChangeCallback = Arc) + Send + Sync>; +/// Checks that a value is usable for a parameter declared as `T`. +/// +/// A [`ParameterKind`] does not uniquely identify a Rust type: several types can share one, and a +/// value of the right kind can still be unrepresentable (an integer that does not fit a [`u16`], a +/// string that is not a known enum variant). Parameters are declared with a fixed type, so such a +/// value has to be rejected when it arrives, or reading the parameter back would panic. +/// +/// Captured as a function pointer at declaration time so that the parameter map can enforce the +/// declared Rust type without knowing what it is. The conversion the type already has to provide +/// *is* the check, so there is nothing for a type to implement beyond [`TryFrom`], and no way for +/// it to accept a value that a later read would then reject. +fn type_check_of(value: &ParameterValue) -> Result<(), String> { + T::try_from(value.clone()) + .map(|_| ()) + .map_err(|err| err.to_string()) +} + struct DeclaredStorage { value: DeclaredValue, kind: ParameterKind, options: ParameterOptionsStorage, + /// Enforces the Rust type the parameter was declared with. The [`kind`](Self::kind) alone + /// is not enough: a value can have the right kind and still be unrepresentable in `T`. + /// See [`type_check_of`]. + type_check: fn(&ParameterValue) -> Result<(), String>, validate: Option, on_change: Option, change_tx: Option>, @@ -552,6 +597,7 @@ impl Debug for DeclaredStorage { .field("value", &self.value) .field("kind", &self.kind) .field("options", &self.options) + .field("type_check", &"..") .field("validate", &self.validate.as_ref().map(|_| "..")) .field("on_change", &self.on_change.as_ref().map(|_| "..")) .field("change_tx", &"..") @@ -622,6 +668,15 @@ impl ParameterMap { == std::mem::discriminant(&value.kind()) || matches!(storage.kind, ParameterKind::Dynamic) { + // The kind matching is not sufficient. The parameter was declared with a + // concrete Rust type, and a value of the right kind can still be + // unrepresentable in it. Reject those here rather than letting a later + // read of the parameter fail. + if let Err(reason) = (storage.type_check)(&value) { + return Err(format!( + "Parameter value is not valid for this parameter's type: {reason}" + )); + } if !storage.options.ranges.in_range(&value) { return Err("Parameter value is out of range".into()); } @@ -946,6 +1001,10 @@ pub enum ParameterValueError { OutOfRange, /// Parameter was stored in a static type and an operation on a different type was attempted. TypeMismatch, + /// The value had the right [`ParameterKind`] for this parameter but was not valid for the + /// Rust type it was declared with, e.g. an integer that does not fit the declared integer + /// type or a string that is not a known enum variant. + Invalid(String), /// A write on a read-only parameter was attempted. ReadOnly, /// A custom validation callback rejected the value. @@ -957,6 +1016,10 @@ impl std::fmt::Display for ParameterValueError { match self { ParameterValueError::OutOfRange => write!(f, "parameter value was out of the parameter's range"), ParameterValueError::TypeMismatch => write!(f, "parameter was stored in a static type and an operation on a different type was attempted"), + // The reason is a whole sentence written by the type's own conversion, which reports + // this variant as its error. Callers that go on to add their own context, such as the + // parameter service, would repeat a prefix added here. + ParameterValueError::Invalid(reason) => write!(f, "{reason}"), ParameterValueError::ReadOnly => write!(f, "a write on a read-only parameter was attempted"), ParameterValueError::ValidationFailed(reason) => write!(f, "custom validation rejected the value: {reason}"), } @@ -1034,6 +1097,9 @@ impl Parameters<'_> { /// * `Ok(())` if setting was successful. /// * [`Err(ParameterValueError::TypeMismatch)`] if the type of the requested value is different /// from the parameter's type. + /// * [`Err(ParameterValueError::Invalid)`] if the requested value shares its + /// [`ParameterKind`] with the parameter's type but cannot be represented in it, e.g. + /// setting an `i64` of `70000` on a parameter declared as [`u16`]. /// * [`Err(ParameterValueError::OutOfRange)`] if the requested value is out of the parameter's /// range. /// * [`Err(ParameterValueError::ReadOnly)`] if the parameter is read only. @@ -1054,6 +1120,10 @@ impl Parameters<'_> { ParameterStorage::Declared(param) => { if T::kind() == param.kind { let value = value.into(); + // `T` here is the type of the value being set, which is not + // necessarily the type the parameter was declared with. The two only + // have to share a kind, so enforce the declared type as well. + (param.type_check)(&value).map_err(ParameterValueError::Invalid)?; if !param.options.ranges.in_range(&value) { return Err(ParameterValueError::OutOfRange); } @@ -1197,26 +1267,12 @@ impl ParameterInterface { Ok(selection) } - fn store_parameter( - &self, - name: Arc, - kind: ParameterKind, - value: DeclaredValue, - options: ParameterOptionsStorage, - validate: Option, - change_tx: Option>, - ) { - self.parameter_map.lock().unwrap().storage.insert( - name, - ParameterStorage::Declared(DeclaredStorage { - options, - value, - kind, - validate, - on_change: None, - change_tx, - }), - ); + fn store_parameter(&self, name: Arc, storage: DeclaredStorage) { + self.parameter_map + .lock() + .unwrap() + .storage + .insert(name, ParameterStorage::Declared(storage)); } pub(crate) fn allow_undeclared(&self) { @@ -1224,6 +1280,84 @@ impl ParameterInterface { } } +/// Shared support for the parameter tests in this module and its children. +#[cfg(test)] +pub(crate) mod test_support { + use super::*; + use crate::Node; + use ros_env::rcl_interfaces::{ + msg::rmw::ParameterDescriptor, srv::rmw::DescribeParameters_Request, + }; + use rosidl_runtime_rs::{seq, Sequence}; + + /// The descriptor a node reports for one of its parameters, as `ros2 param describe` gets it. + pub(crate) fn parameter_descriptor(node: &Node, name: &str) -> ParameterDescriptor { + let map = node.parameter_interface().parameter_map.lock().unwrap(); + let response = crate::parameter::service::describe_parameters( + DescribeParameters_Request { + names: seq![name.into()], + }, + &map, + ); + response + .descriptors + .into_iter() + .next() + .expect("a descriptor is returned for every requested name") + } + + /// A string-backed parameter type, of the kind a user can define today by implementing the + /// public [`ParameterVariant`] trait. Its conversion from [`ParameterValue`] is *partial*: + /// a value can be a `String`, and so pass any check based on [`ParameterKind`], and still + /// not be a valid `Switch`. + #[derive(Clone, Debug, PartialEq)] + pub(crate) enum Switch { + On, + Off, + } + + impl From for ParameterValue { + fn from(value: Switch) -> Self { + ParameterValue::String( + match value { + Switch::On => "on", + Switch::Off => "off", + } + .into(), + ) + } + } + + impl TryFrom for Switch { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::String(s) => match s.as_ref() { + "on" => Ok(Switch::On), + "off" => Ok(Switch::Off), + other => Err(ParameterValueError::Invalid(format!( + "unknown Switch '{other}', expected one of: on, off" + ))), + }, + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + impl ParameterVariant for Switch { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::String + } + + fn type_constraints() -> Option> { + Some("one of: on, off".into()) + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -2230,4 +2364,104 @@ mod tests { param.set(75).unwrap(); assert_eq!(sub.get(), 75); } + + use super::test_support::Switch; + + fn rmw_string(value: &str) -> RmwParameterValue { + RmwParameterValue { + type_: ParameterType::PARAMETER_STRING, + string_value: value.into(), + ..Default::default() + } + } + + #[test] + fn test_service_path_rejects_value_of_right_kind_but_wrong_type() { + let node = Context::default() + .create_basic_executor() + .create_node(&format!("param_test_node_{}", line!())) + .unwrap(); + let param: MandatoryParameter = node + .declare_parameter("switch") + .default(Switch::On) + .mandatory() + .unwrap(); + + let map = node.parameter_interface().parameter_map.lock().unwrap(); + + // A valid value is accepted. + assert!(map + .validate_parameter_setting("switch", rmw_string("off")) + .is_ok()); + + // "banana" is a perfectly good string, so the parameter kind matches. It is not a + // Switch, though, and accepting it would make the next `get()` panic. + // + // Asserted in full because this string is what an operator sees come back from + // `ros2 param set`, and because the reason travels through a `Display` that used to + // prepend a duplicate of the context added here. + let err = map + .validate_parameter_setting("switch", rmw_string("banana")) + .unwrap_err(); + assert_eq!( + err, + "Parameter value is not valid for this parameter's type: \ + unknown Switch 'banana', expected one of: on, off" + ); + + // A genuine kind mismatch still reports as one. + let err = map + .validate_parameter_setting( + "switch", + RmwParameterValue { + type_: ParameterType::PARAMETER_INTEGER, + integer_value: 42, + ..Default::default() + }, + ) + .unwrap_err(); + assert!(err.contains("different type"), "unexpected reason: {err}"); + + drop(map); + + // Nothing was applied, and reading the parameter still works. + assert_eq!(param.get(), Switch::On); + } + + #[test] + fn test_undeclared_set_rejects_value_of_right_kind_but_wrong_type() { + let node = Context::default() + .create_basic_executor() + .create_node(&format!("param_test_node_{}", line!())) + .unwrap(); + let param: MandatoryParameter = node + .declare_parameter("switch") + .default(Switch::On) + .mandatory() + .unwrap(); + + // `Parameters::set` only requires the value's kind to match the parameter's, so this + // `Arc` is accepted as far as the kind check goes even though the parameter was + // declared as a `Switch`. + let err = node + .use_undeclared_parameters() + .set::>("switch", "banana".into()) + .unwrap_err(); + assert!( + matches!(err, ParameterValueError::Invalid(_)), + "expected Invalid, got {err:?}" + ); + // The reason reaches the caller once, not wrapped in a restatement of itself. + assert_eq!( + err.to_string(), + "unknown Switch 'banana', expected one of: on, off" + ); + assert_eq!(param.get(), Switch::On); + + // A value that is valid for the declared type goes through. + node.use_undeclared_parameters() + .set::>("switch", "off".into()) + .unwrap(); + assert_eq!(param.get(), Switch::Off); + } } diff --git a/rclrs/src/parameter/service.rs b/rclrs/src/parameter/service.rs index 79ea85e9..9b7c46f7 100644 --- a/rclrs/src/parameter/service.rs +++ b/rclrs/src/parameter/service.rs @@ -35,7 +35,7 @@ pub struct ParameterService { set_parameters_atomically_service: Service, } -fn describe_parameters( +pub(crate) fn describe_parameters( req: DescribeParameters_Request, map: &ParameterMap, ) -> DescribeParameters_Response { @@ -833,6 +833,121 @@ mod tests { Ok(()) } + /// A remote caller cannot put a value into a parameter that the type it was declared with + /// cannot hold, even when the value's ROS 2 type is the right one. Were it able to, the + /// node would panic the next time it read the parameter. + #[test] + fn test_set_parameters_service_enforces_the_declared_rust_type() -> Result<(), RclrsError> { + let mut executor = Context::default().create_basic_executor(); + let node = executor + .create_node(NodeOptions::new("node").namespace("narrow")) + .unwrap(); + let port: MandatoryParameter = node + .declare_parameter("port") + .default(8080) + .mandatory() + .unwrap(); + + let client_node = executor + .create_node(NodeOptions::new("client").namespace("narrow")) + .unwrap(); + let set_client = + client_node.create_client::("/narrow/node/set_parameters")?; + + let set_client_inner = Arc::clone(&set_client); + let ready = client_node + .notify_on_graph_change_with_period(Duration::from_millis(1), move || { + set_client_inner.service_is_ready().unwrap() + }); + executor + .spin(SpinOptions::default().until_promise_resolved(ready)) + .first_error()?; + + let integer = |value: i64| RmwParameter { + name: "port".into(), + value: RmwParameterValue { + type_: ParameterType::PARAMETER_INTEGER, + integer_value: value, + ..Default::default() + }, + }; + let request = SetParameters_Request { + // 70000 is a valid ROS 2 integer, so the kind matches, but it is not a u16. + parameters: seq![integer(70000), integer(-1), integer(9000)], + }; + + let callback_ran = Arc::new(AtomicBool::new(false)); + let callback_ran_inner = Arc::clone(&callback_ran); + let promise = set_client + .call_then(&request, move |response: SetParameters_Response| { + assert_eq!(response.results.len(), 3); + assert!(!response.results[0].successful); + // Asserted in full: this is the text `ros2 param set` prints back at whoever + // made the call, so it is part of the interface rather than a detail. + let reason = response.results[0].reason.to_string(); + assert_eq!( + reason, + "Parameter value is not valid for this parameter's type: \ + 70000 is out of range for u16, which accepts 0..=65535" + ); + assert!(!response.results[1].successful, "negative port"); + assert!(response.results[2].successful, "9000 is a valid u16"); + callback_ran_inner.store(true, Ordering::Release); + }) + .unwrap(); + + executor + .spin(SpinOptions::default().until_promise_resolved(promise)) + .first_error()?; + assert!(callback_ran.load(Ordering::Acquire)); + + // Only the valid value was applied, and reading the parameter does not panic. + assert_eq!(port.get(), 9000); + Ok(()) + } + + /// A parameter's own type can describe what it accepts, so that `ros2 param describe` + /// reports it without the declaration having to restate it, and without that restatement + /// drifting from the type as it gains variants. + #[test] + fn test_descriptor_constraints_come_from_the_parameter_type() { + use crate::parameter::test_support::{parameter_descriptor, Switch}; + + let node = Context::default() + .create_basic_executor() + .create_node("describe_type_constraints") + .unwrap(); + + // No `.constraints()` on either declaration: the type supplies the text. + let _inherited: MandatoryParameter = node + .declare_parameter("inherited") + .default(Switch::On) + .mandatory() + .unwrap(); + // An explicit constraint still wins, for rules specific to this declaration. + let _explicit: MandatoryParameter = node + .declare_parameter("explicit") + .default(Switch::On) + .constraints("must be off on tuesdays") + .mandatory() + .unwrap(); + // Types that say nothing about themselves are unaffected. + let _plain: MandatoryParameter = node + .declare_parameter("plain") + .default(1) + .mandatory() + .unwrap(); + + let constraints = |name| { + parameter_descriptor(&node, name) + .additional_constraints + .to_string() + }; + assert_eq!(constraints("inherited"), "one of: on, off"); + assert_eq!(constraints("explicit"), "must be off on tuesdays"); + assert_eq!(constraints("plain"), ""); + } + #[test] fn test_describe_get_types_parameters_service() -> Result<(), RclrsError> { let (mut executor, _test, client_node) = construct_test_nodes("describe"); diff --git a/rclrs/src/parameter/std_types.rs b/rclrs/src/parameter/std_types.rs new file mode 100644 index 00000000..053a672d --- /dev/null +++ b/rclrs/src/parameter/std_types.rs @@ -0,0 +1,765 @@ +//! [`ParameterVariant`] implementations for common Rust types. +//! +//! ROS 2 has nine parameter types, and [`value`](super::value) implements [`ParameterVariant`] +//! for the Rust type that represents each of them most directly, such as `i64`, `f64`, +//! `Arc` and `Arc<[i64]>`. Those are the right types for the parameter machinery, but they +//! are not always the types an application would otherwise use. +//! +//! This module widens the set of types a parameter can be declared with, so that a `Vec` +//! or a `u16` can be used where the ROS 2 representation happens to be a string array or an +//! integer. Conversions in this direction can be partial: not every `i64` fits a `u16`. Values +//! that do not fit are rejected by the type's [`TryFrom`] before they are ever stored, so a +//! parameter declared as `u16` can never hold something that is not one. +//! +//! Every scalar type here has a `Vec` form, which uses whichever ROS 2 array type holds the +//! scalar's representation. A `Vec` is therefore an integer array whose elements are each +//! checked against the range of a `u16`, and the rejection message says which element was at +//! fault. The one exception is `Vec`, which is a ROS 2 *byte* array rather than an integer +//! array, since that is what ROS 2 has a byte array for. +//! +//! # Unsupported integer types +//! +//! `u64`, `usize`, `i128` and `u128` are deliberately absent. A parameter's value has to be +//! representable in ROS 2, whose integer type is `i64`, and every way of handling a value above +//! `i64::MAX` silently produces a number the application did not ask for, whether by saturating, +//! by wrapping, or by panicking inside an infallible `From`. Use `i64`, or `u32` when the value +//! must be unsigned and small. + +use std::{ops::Deref, path::PathBuf, sync::Arc, time::Duration}; + +use crate::{ + parameter::{ParameterRange, ParameterRanges}, + ParameterKind, ParameterValue, ParameterValueError, ParameterVariant, +}; + +/// Implements the three traits needed to declare a parameter as `$t`, where `$t` maps onto the +/// ROS 2 integer type but holds a narrower range of values. +macro_rules! impl_narrow_integer { + ($($t:ty),* $(,)?) => { $( + impl From<$t> for ParameterValue { + fn from(value: $t) -> Self { + ParameterValue::Integer(value.into()) + } + } + + impl TryFrom for $t { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Integer(v) => <$t>::try_from(v).map_err(|_| { + ParameterValueError::Invalid(format!( + "{v} is out of range for {}, which accepts {}..={}", + stringify!($t), + <$t>::MIN, + <$t>::MAX, + )) + }), + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + // Ranges are written in the field's own type, so `range = 1024..=65535` on a `u16` + // means what it appears to. `ParameterRanges` stores the ROS 2 representation. + // + // A bound the declaration leaves open is still bounded by the type, so it is filled in + // from the type's own limits. That puts the narrowing into the descriptor's + // `IntegerRange`, where `ros2 param describe` and rqt_reconfigure can read it, instead + // of leaving them to discover it by having a value rejected. + impl From> for ParameterRanges { + fn from(range: ParameterRange<$t>) -> Self { + ParameterRange:: { + lower: Some(range.lower.map_or(i64::from(<$t>::MIN), i64::from)), + upper: Some(range.upper.map_or(i64::from(<$t>::MAX), i64::from)), + step: range.step.map(i64::from), + } + .into() + } + } + + impl ParameterVariant for $t { + type Range = ParameterRange<$t>; + + fn kind() -> ParameterKind { + ParameterKind::Integer + } + + fn type_constraints() -> Option> { + Some(format!("{}..={}", <$t>::MIN, <$t>::MAX).into()) + } + } + )* }; +} + +impl_narrow_integer!(i8, i16, i32, u8, u16, u32); + +impl From for ParameterValue { + fn from(value: f32) -> Self { + ParameterValue::Double(value.into()) + } +} + +impl TryFrom for f32 { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Double(v) => { + let narrowed = v as f32; + // Narrowing is lossy in the mantissa, which is inherent to asking for an `f32`, + // but a finite value must not silently become an infinity. + if narrowed.is_finite() || !v.is_finite() { + Ok(narrowed) + } else { + Err(ParameterValueError::Invalid(format!( + "{v} is out of range for f32" + ))) + } + } + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl From> for ParameterRanges { + fn from(range: ParameterRange) -> Self { + ParameterRange:: { + lower: range.lower.map(f64::from), + upper: range.upper.map(f64::from), + step: range.step.map(f64::from), + } + .into() + } +} + +impl ParameterVariant for f32 { + type Range = ParameterRange; + + fn kind() -> ParameterKind { + ParameterKind::Double + } +} + +impl From for ParameterValue { + fn from(value: String) -> Self { + ParameterValue::String(value.into()) + } +} + +impl TryFrom for String { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::String(v) => Ok(v.to_string()), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for String { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::String + } +} + +/// Note that a path which is not valid UTF-8 cannot be represented as a ROS 2 string and is +/// converted lossily, since ROS 2 parameter strings are UTF-8. +impl From for ParameterValue { + fn from(value: PathBuf) -> Self { + ParameterValue::String(value.to_string_lossy().as_ref().into()) + } +} + +impl TryFrom for PathBuf { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::String(v) => Ok(PathBuf::from(v.as_ref())), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for PathBuf { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::String + } +} + +/// Implements the traits needed to declare a parameter as `Vec<$item>`, for the array kinds +/// whose ROS 2 representation is `Arc<[$item]>`. +macro_rules! impl_vec_of { + ($(($item:ty, $variant:ident, $kind:ident)),* $(,)?) => { $( + impl From> for ParameterValue { + fn from(value: Vec<$item>) -> Self { + ParameterValue::$variant(value.into()) + } + } + + impl TryFrom for Vec<$item> { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::$variant(v) => Ok(v.to_vec()), + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + impl ParameterVariant for Vec<$item> { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::$kind + } + } + )* }; +} + +impl_vec_of!( + (u8, ByteArray, ByteArray), + (bool, BoolArray, BoolArray), + (i64, IntegerArray, IntegerArray), + (f64, DoubleArray, DoubleArray), +); + +/// Reports which element of an array failed to convert, since "out of range for u16" is not much +/// help on its own when the array has forty entries. +fn element_error(index: usize, error: ParameterValueError) -> ParameterValueError { + let reason = match error { + ParameterValueError::Invalid(reason) => reason, + other => other.to_string(), + }; + ParameterValueError::Invalid(format!("element {index}: {reason}")) +} + +/// Implements the traits needed to declare a parameter as `Vec<$item>`, for an item type whose +/// ROS 2 representation is `$scalar` and which therefore lives in the `$array` array type. +/// +/// Reading an element back goes through the item type's own [`TryFrom`], so whatever an element of +/// that type may be is decided in exactly one place, and the error says which element was at fault. +macro_rules! impl_vec_of_convertible { + ($(($item:ty, $array:ident, $scalar:ident, $kind:ident, $to_element:expr)),* $(,)?) => { $( + impl From> for ParameterValue { + fn from(value: Vec<$item>) -> Self { + ParameterValue::$array(value.into_iter().map($to_element).collect()) + } + } + + impl TryFrom for Vec<$item> { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::$array(items) => items + .iter() + .enumerate() + .map(|(index, item)| { + <$item>::try_from(ParameterValue::$scalar(item.clone())) + .map_err(|error| element_error(index, error)) + }) + .collect(), + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + impl ParameterVariant for Vec<$item> { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::$kind + } + + fn type_constraints() -> Option> { + <$item as ParameterVariant>::type_constraints() + .map(|constraints| format!("every element {constraints}").into()) + } + } + )* }; +} + +// `Vec` is deliberately absent: ROS 2 has a byte array type, so a sequence of bytes is that +// rather than an integer array. It is implemented above, alongside the other array types that need +// no element conversion. +impl_vec_of_convertible!( + (i8, IntegerArray, Integer, IntegerArray, i64::from), + (i16, IntegerArray, Integer, IntegerArray, i64::from), + (i32, IntegerArray, Integer, IntegerArray, i64::from), + (u16, IntegerArray, Integer, IntegerArray, i64::from), + (u32, IntegerArray, Integer, IntegerArray, i64::from), + (f32, DoubleArray, Double, DoubleArray, f64::from), + ( + PathBuf, + StringArray, + String, + StringArray, + |path: PathBuf| { Arc::from(path.to_string_lossy().as_ref()) } + ), + ( + DurationSecs, + DoubleArray, + Double, + DoubleArray, + |value: DurationSecs| { value.0.as_secs_f64() } + ), + ( + DurationMillis, + IntegerArray, + Integer, + IntegerArray, + |value: DurationMillis| { whole_millis(value.0) } + ), +); + +impl From> for ParameterValue { + fn from(value: Vec) -> Self { + ParameterValue::StringArray(value.into_iter().map(Arc::from).collect()) + } +} + +impl TryFrom for Vec { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::StringArray(v) => Ok(v.iter().map(|s| s.to_string()).collect()), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for Vec { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::StringArray + } +} + +/// A [`Duration`] parameter expressed in seconds, as a ROS 2 double. +/// +/// [`Duration`] itself is not a parameter type, because the unit it would be stored in is not +/// something to leave implicit in a robotics configuration. Choose the unit at the point of +/// declaration by using this or [`DurationMillis`]. Both dereference to [`Duration`], and the +/// unit is reported in the parameter descriptor's constraints. +/// +/// Ranges are expressed in seconds, e.g. `range = 0.0..=5.0`. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct DurationSecs(pub Duration); + +/// A [`Duration`] parameter expressed in whole milliseconds, as a ROS 2 integer. +/// +/// See [`DurationSecs`] for why the unit is part of the type. Sub-millisecond precision is lost. +/// Ranges are expressed in milliseconds, e.g. `range = 0..=5000`. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct DurationMillis(pub Duration); + +macro_rules! impl_duration_wrapper { + ($($t:ident),* $(,)?) => { $( + impl Deref for $t { + type Target = Duration; + + fn deref(&self) -> &Duration { + &self.0 + } + } + + impl From for $t { + fn from(value: Duration) -> Self { + Self(value) + } + } + + impl From<$t> for Duration { + fn from(value: $t) -> Self { + value.0 + } + } + )* }; +} + +impl_duration_wrapper!(DurationSecs, DurationMillis); + +impl From for ParameterValue { + fn from(value: DurationSecs) -> Self { + ParameterValue::Double(value.0.as_secs_f64()) + } +} + +impl TryFrom for DurationSecs { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Double(v) => { + Duration::try_from_secs_f64(v) + .map(DurationSecs) + .map_err(|_| { + ParameterValueError::Invalid(format!( + "{v} is not a valid duration in seconds; must be finite and not negative" + )) + }) + } + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for DurationSecs { + type Range = ParameterRange; + + fn kind() -> ParameterKind { + ParameterKind::Double + } + + fn type_constraints() -> Option> { + Some("a duration in seconds".into()) + } +} + +/// The whole milliseconds of a duration, as a ROS 2 integer. +fn whole_millis(value: Duration) -> i64 { + let millis = value.as_millis(); + debug_assert!( + millis <= i64::MAX as u128, + "duration of {millis} ms cannot be represented as a ROS 2 integer parameter", + ); + i64::try_from(millis).unwrap_or(i64::MAX) +} + +impl From for ParameterValue { + fn from(value: DurationMillis) -> Self { + ParameterValue::Integer(whole_millis(value.0)) + } +} + +impl TryFrom for DurationMillis { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Integer(v) => u64::try_from(v) + .map(|millis| DurationMillis(Duration::from_millis(millis))) + .map_err(|_| { + ParameterValueError::Invalid(format!( + "{v} is not a valid duration in milliseconds; must not be negative" + )) + }), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for DurationMillis { + type Range = ParameterRange; + + fn kind() -> ParameterKind { + ParameterKind::Integer + } + + fn type_constraints() -> Option> { + Some("a duration in milliseconds".into()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::*; + + /// Asserts that a value survives a round trip through `ParameterValue`. + fn round_trip(value: T) + where + T: ParameterVariant + PartialEq + std::fmt::Debug, + { + let stored: ParameterValue = value.clone().into(); + let Ok(recovered) = T::try_from(stored) else { + panic!("{value:?} did not convert back from its ParameterValue"); + }; + assert_eq!(recovered, value); + } + + #[test] + fn test_round_trips() { + round_trip(String::from("hello")); + round_trip(PathBuf::from("/dev/ttyUSB0")); + + round_trip(vec![String::from("a"), String::from("b")]); + round_trip(vec![PathBuf::from("/dev/a"), PathBuf::from("/dev/b")]); + round_trip(vec![1i64, 2, 3]); + round_trip(vec![1.5f64, 2.5]); + round_trip(vec![true, false]); + round_trip(vec![1u8, 2, 3]); + round_trip(vec![-5i8, 5]); + round_trip(vec![-5000i16, 5000]); + round_trip(vec![-70000i32, 70000]); + round_trip(vec![0u16, 65535]); + round_trip(vec![0u32, 4_000_000_000]); + round_trip(vec![1.5f32, -2.5]); + round_trip(vec![DurationSecs(Duration::from_millis(250))]); + round_trip(vec![DurationMillis(Duration::from_millis(30))]); + + round_trip(1.5f32); + round_trip(-5i8); + round_trip(-5000i16); + round_trip(-70000i32); + round_trip(200u8); + round_trip(65535u16); + round_trip(4_000_000_000u32); + + round_trip(DurationSecs(Duration::from_millis(1500))); + round_trip(DurationMillis(Duration::from_millis(1500))); + } + + #[test] + fn test_narrow_integers_reject_out_of_range_values() { + // The kind is right and the value is a perfectly good i64, but it is not a u16. + let err = u16::try_from(ParameterValue::Integer(70000)).unwrap_err(); + assert!( + matches!(&err, ParameterValueError::Invalid(reason) + if reason.contains("70000") && reason.contains("0..=65535")), + "unhelpful reason: {err}" + ); + + assert!(u16::try_from(ParameterValue::Integer(-1)).is_err()); + assert!(i8::try_from(ParameterValue::Integer(128)).is_err()); + assert!(u8::try_from(ParameterValue::Integer(256)).is_err()); + assert!(i32::try_from(ParameterValue::Integer(i64::MAX)).is_err()); + + // Boundaries are accepted. + assert_eq!( + u16::try_from(ParameterValue::Integer(65535)).unwrap(), + 65535 + ); + assert_eq!(i8::try_from(ParameterValue::Integer(-128)).unwrap(), -128); + } + + #[test] + fn test_f32_rejects_values_it_cannot_represent() { + assert!(f32::try_from(ParameterValue::Double(1e300)).is_err()); + assert!(f32::try_from(ParameterValue::Double(-1e300)).is_err()); + + // Non-finite values pass through unchanged rather than being rejected as out of range. + assert!(f32::try_from(ParameterValue::Double(f64::INFINITY)) + .unwrap() + .is_infinite()); + assert!(f32::try_from(ParameterValue::Double(f64::NAN)) + .unwrap() + .is_nan()); + } + + #[test] + fn test_durations_reject_invalid_values() { + assert!(DurationSecs::try_from(ParameterValue::Double(-1.0)).is_err()); + assert!(DurationSecs::try_from(ParameterValue::Double(f64::NAN)).is_err()); + assert!(DurationMillis::try_from(ParameterValue::Integer(-1)).is_err()); + } + + /// An array of a narrower type holds the same values as the scalar type does, and says which + /// element is at fault when one of them does not fit. + #[test] + fn test_arrays_check_every_element() { + let too_big = ParameterValue::IntegerArray(vec![1, 2, 70000].into()); + let err = Vec::::try_from(too_big).unwrap_err(); + assert!( + matches!(&err, ParameterValueError::Invalid(reason) + if reason.contains("element 2") && reason.contains("0..=65535")), + "the reason should name the element: {err}" + ); + + assert!(Vec::::try_from(ParameterValue::IntegerArray(vec![-1].into())).is_err()); + assert!(Vec::::try_from(ParameterValue::DoubleArray(vec![1e300].into())).is_err()); + assert!( + Vec::::try_from(ParameterValue::IntegerArray(vec![-1].into())).is_err() + ); + + // An array of the wrong ROS 2 type is a different problem from a bad element. + assert!(matches!( + Vec::::try_from(ParameterValue::DoubleArray(vec![1.0].into())), + Err(ParameterValueError::TypeMismatch) + )); + } + + /// A sequence of bytes is a ROS 2 byte array, not an integer array, which is why `Vec` is + /// the one array type that does not follow its scalar. + #[test] + fn test_byte_arrays_are_not_integer_arrays() { + assert_eq!(Vec::::kind(), ParameterKind::ByteArray); + assert_eq!(Vec::::kind(), ParameterKind::IntegerArray); + assert_eq!(u8::kind(), ParameterKind::Integer); + } + + #[test] + fn test_arrays_report_the_constraints_of_their_elements() { + assert_eq!( + Vec::::type_constraints().as_deref(), + Some("every element 0..=65535") + ); + assert_eq!( + Vec::::type_constraints().as_deref(), + Some("every element a duration in milliseconds") + ); + // An element type with nothing to say about itself leaves the array with nothing either. + assert_eq!(Vec::::type_constraints(), None); + } + + #[test] + fn test_declaring_parameters_with_ergonomic_types() { + let node = Context::default() + .create_basic_executor() + .create_node("std_types") + .unwrap(); + + let name: MandatoryParameter = node + .declare_parameter("name") + .default("robot".to_string()) + .mandatory() + .unwrap(); + assert_eq!(name.get(), "robot"); + + let wheels: MandatoryParameter> = node + .declare_parameter("wheels") + .default(vec!["left".to_string(), "right".to_string()]) + .mandatory() + .unwrap(); + assert_eq!(wheels.get(), vec!["left", "right"]); + + let device: MandatoryParameter = node + .declare_parameter("device") + .default(PathBuf::from("/dev/ttyUSB0")) + .mandatory() + .unwrap(); + assert_eq!(device.get(), PathBuf::from("/dev/ttyUSB0")); + + let timeout: MandatoryParameter = node + .declare_parameter("timeout") + .default(DurationSecs(Duration::from_millis(2500))) + .mandatory() + .unwrap(); + assert_eq!(timeout.get().as_millis(), 2500); + + let ports: MandatoryParameter> = node + .declare_parameter("ports") + .default(vec![8080, 9090]) + .mandatory() + .unwrap(); + assert_eq!(ports.get(), vec![8080, 9090]); + } + + /// A `Vec` parameter cannot be made to hold an integer array that is not one, even by a + /// caller going through the untyped interface with a well-formed array of `i64`. + #[test] + fn test_narrow_arrays_cannot_be_corrupted() { + let node = Context::default() + .create_basic_executor() + .create_node("std_types_arrays") + .unwrap(); + + let ports: MandatoryParameter> = node + .declare_parameter("ports") + .default(vec![8080u16]) + .mandatory() + .unwrap(); + + let err = node + .use_undeclared_parameters() + .set::>("ports", vec![1, 70000]) + .unwrap_err(); + assert!(matches!(err, ParameterValueError::Invalid(_)), "{err}"); + assert_eq!(ports.get(), vec![8080]); + } + + /// Ranges on a narrow integer parameter are expressed in that integer's own type, and are + /// enforced against the ROS 2 representation. + #[test] + fn test_narrow_integer_ranges() { + let node = Context::default() + .create_basic_executor() + .create_node("std_types_ranges") + .unwrap(); + + let port: MandatoryParameter = node + .declare_parameter("port") + .default(8080) + .range(ParameterRange { + lower: Some(1024), + upper: Some(49151), + step: None, + }) + .mandatory() + .unwrap(); + + assert_eq!(port.get(), 8080); + assert!(port.set(80u16).is_err(), "below the range"); + assert!(port.set(50000u16).is_err(), "above the range"); + port.set(9000u16).unwrap(); + assert_eq!(port.get(), 9000); + } + + /// The narrowing is reported as the descriptor's `IntegerRange`, which is what rqt and + /// `ros2 param describe` read, and not only as free-text constraints. + #[test] + fn test_narrow_integers_report_their_range_in_the_descriptor() { + use crate::parameter::test_support::parameter_descriptor; + + let node = Context::default() + .create_basic_executor() + .create_node("std_types_descriptor") + .unwrap(); + + // No range on the declaration: the type is the only thing that bounds the value. + let _implied: MandatoryParameter = node + .declare_parameter("implied") + .default(8080) + .mandatory() + .unwrap(); + // A half-open range keeps its declared bound and takes the type's for the other end. + let _half_open: MandatoryParameter = node + .declare_parameter("half_open") + .default(8080) + .range(ParameterRange { + lower: Some(1024), + upper: None, + step: None, + }) + .mandatory() + .unwrap(); + let _signed: MandatoryParameter = node + .declare_parameter("signed") + .default(0) + .mandatory() + .unwrap(); + + let bounds = |name| { + let descriptor = parameter_descriptor(&node, name); + let range = descriptor + .integer_range + .first() + .expect("a narrow integer is always bounded, so it always has a range"); + (range.from_value, range.to_value) + }; + assert_eq!(bounds("implied"), (0, 65535)); + assert_eq!(bounds("half_open"), (1024, 65535)); + assert_eq!(bounds("signed"), (-128, 127)); + + // An `i64` is bounded by nothing narrower than the ROS 2 type, so it still reports no + // range unless the declaration asked for one. + let _plain: MandatoryParameter = node + .declare_parameter("plain") + .default(0) + .mandatory() + .unwrap(); + assert!(parameter_descriptor(&node, "plain") + .integer_range + .is_empty()); + } +} diff --git a/rclrs/src/parameter/value.rs b/rclrs/src/parameter/value.rs index e646b30e..32218c1a 100644 --- a/rclrs/src/parameter/value.rs +++ b/rclrs/src/parameter/value.rs @@ -137,13 +137,26 @@ impl From]>> for ParameterValue { /// A trait that describes a value that can be converted into a parameter. pub trait ParameterVariant: - Into + Clone + TryFrom + 'static + Into + Clone + TryFrom + 'static { /// The type used to describe the range of this parameter. type Range: Into + Default + Clone; /// Returns the `ParameterKind` of the implemented type. fn kind() -> ParameterKind; + + /// Human-readable constraints that are inherent to this type, such as the set of variants + /// a string-backed enum accepts. + /// + /// Used for the parameter descriptor's `additional_constraints` field when the declaration + /// does not set its own with [`ParameterBuilder::constraints`], so that introspection + /// through `ros2 param describe` can report what a value of this type may be without every + /// declaration site having to restate it. + /// + /// [`ParameterBuilder::constraints`]: crate::ParameterBuilder::constraints + fn type_constraints() -> Option> { + None + } } impl TryFrom for bool {