diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index 8fdccad7..c0baf758 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -42,11 +42,11 @@ use crate::{ IntoActionClientOptions, IntoActionServerOptions, IntoAsyncServiceCallback, IntoAsyncSubscriptionCallback, IntoNodeServiceCallback, IntoNodeSubscriptionCallback, IntoNodeTimerOneshotCallback, IntoNodeTimerRepeatingCallback, IntoTimerOptions, LogParams, - Logger, MessageInfo, ParameterBuilder, ParameterInterface, ParameterVariant, Parameters, - Promise, Publisher, PublisherOptions, PublisherState, RclrsError, RequestedGoal, Service, - ServiceOptions, ServiceState, Subscription, SubscriptionOptions, SubscriptionState, - TerminatedGoal, TimeSource, Timer, TimerState, ToLogParams, Worker, WorkerOptions, WorkerState, - ENTITY_LIFECYCLE_MUTEX, + Logger, MessageInfo, ParameterBuilder, ParameterConversion, ParameterInterface, + ParameterVariant, Parameters, Promise, Publisher, PublisherOptions, PublisherState, RclrsError, + RequestedGoal, Service, ServiceOptions, ServiceState, Subscription, SubscriptionOptions, + SubscriptionState, TerminatedGoal, TimeSource, Timer, TimerState, ToLogParams, Worker, + WorkerOptions, WorkerState, ENTITY_LIFECYCLE_MUTEX, }; /// A processing unit that can communicate with other nodes. See the API of @@ -1420,6 +1420,47 @@ impl NodeState { self.parameter.declare(name.into()) } + /// Declares a parameter of a type that does not implement [`ParameterVariant`], or one that + /// does but should be represented differently here. + /// + /// [`Self::declare_parameter`] is this called with the conversion the type describes for + /// itself. Nothing after the call differs: the builder, the handles, the range checks and the + /// parameter services do not care where the conversion came from. + /// + /// # Example + /// ``` + /// # use rclrs::*; + /// # use std::time::Duration; + /// let executor = Context::default().create_basic_executor(); + /// let node = executor.create_node("drive_controller")?; + /// + /// // `Duration` belongs to `std` and cannot implement a trait from rclrs, but saying how it + /// // is represented is enough. + /// let timeout = node + /// .declare_parameter_with( + /// "timeout", + /// ParameterConversion::double(Duration::as_secs_f64, Duration::try_from_secs_f64), + /// ) + /// .default(Duration::from_millis(500)) + /// .mandatory()?; + /// + /// assert_eq!(timeout.get(), Duration::from_millis(500)); + /// # Ok::<(), RclrsError>(()) + /// ``` + pub fn declare_parameter_with<'a, T: 'static>( + &'a self, + name: impl Into>, + conversion: ParameterConversion, + ) -> ParameterBuilder<'a, T> { + self.parameter.declare_with(name.into(), conversion) + } + + /// 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 27e8930e..35cf50e1 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -1,6 +1,9 @@ +mod conversion; mod override_map; mod range; mod service; +pub use conversion::*; + mod value; pub(crate) use override_map::*; @@ -16,7 +19,6 @@ use crate::{ use std::{ collections::{btree_map::Entry, BTreeMap}, fmt::Debug, - marker::PhantomData, sync::{Arc, Mutex, RwLock, Weak}, }; use tokio::sync::watch; @@ -41,33 +43,20 @@ use tokio::sync::watch; // * Explicit API for access to undeclared parameters by having a // `node.use_undeclared_parameters()` API that allows access to all parameters. -#[derive(Clone, Debug)] -struct ParameterOptionsStorage { - description: Arc, - constraints: Arc, - ranges: ParameterRanges, -} - -impl From> for ParameterOptionsStorage { - fn from(opts: ParameterOptions) -> Self { - Self { - description: opts.description, - constraints: opts.constraints, - ranges: opts.ranges.into(), - } - } -} - /// Options that can be attached to a parameter, such as description, ranges. /// Some of this data will be used to populate the ParameterDescriptor +/// +/// The range is held in the erased [`ParameterRanges`] form that the descriptor and every range +/// check use, because a range constrains the stored value rather than the Rust type it is read +/// back as. #[derive(Clone, Debug)] -pub struct ParameterOptions { +pub(crate) struct ParameterOptions { description: Arc, constraints: Arc, - ranges: T::Range, + ranges: ParameterRanges, } -impl Default for ParameterOptions { +impl Default for ParameterOptions { fn default() -> Self { Self { description: Arc::from(""), @@ -87,18 +76,21 @@ enum DeclaredValue { /// Builder used to declare a parameter. Obtain this by calling /// [`crate::NodeState::declare_parameter`]. #[must_use] -pub struct ParameterBuilder<'a, T: ParameterVariant> { +pub struct ParameterBuilder<'a, T> { name: Arc, default_value: Option, + /// How the value is represented as a parameter value. Chosen at declaration and carried by + /// every handle the declaration hands out. + conversion: ParameterConversion, ignore_override: bool, discard_mismatching_prior_value: bool, discriminator: DiscriminatorFunction<'a, T>, - options: ParameterOptions, + options: ParameterOptions, interface: &'a ParameterInterface, validate: Option Result<(), String> + Send + Sync>>, } -impl<'a, T: ParameterVariant> ParameterBuilder<'a, T> { +impl<'a, T: 'static> ParameterBuilder<'a, T> { /// Sets the default value for the parameter. The parameter value will be /// initialized to this if no command line override was given for this /// parameter and if the parameter also had no value prior to being @@ -147,18 +139,13 @@ impl<'a, T: ParameterVariant> ParameterBuilder<'a, T> { self } - /// Sets the range for the parameter. - /// - /// Takes the parameter type's own range, which for a numeric parameter is a - /// [`ParameterRange`], or anything that converts into one. Every standard Rust range does, so - /// a bound can be written the way it is anywhere else in the language: `a..=b`, `a..`, `..=b`, - /// `a..b` and `..` are all accepted. + /// Sets the bounds on the value as it is stored, in the terms the conversion stores it in. /// - /// A ROS 2 range is inclusive of both ends, so an exclusive Rust range converts by naming the - /// value just below its end. Writing a [`ParameterRange`] out is what a `step` needs, since no - /// Rust range carries one. - pub fn range(mut self, range: impl Into) -> Self { - self.options.ranges = range.into(); + /// [`Self::range`] is the one to reach for when the parameter's type describes a range of its + /// own. This is for a parameter declared with a [`ParameterConversion`], whose type need not + /// implement [`ParameterVariant`] and so has no `Range` to name. + pub fn stored_ranges(mut self, ranges: ParameterRanges) -> Self { + self.options.ranges = ranges; self } @@ -221,6 +208,28 @@ impl<'a, T: ParameterVariant> ParameterBuilder<'a, T> { } } +impl<'a, T: ParameterVariant> ParameterBuilder<'a, T> { + /// Sets the range for the parameter, in the terms of its own type. + /// + /// Takes the parameter type's own range, which for a numeric parameter is a + /// [`ParameterRange`], or anything that converts into one. Every standard Rust range does, so + /// a bound can be written the way it is anywhere else in the language: `a..=b`, `a..`, `..=b`, + /// `a..b` and `..` are all accepted. + /// + /// A ROS 2 range is inclusive of both ends, so an exclusive Rust range converts by naming the + /// value just below its end. Writing a [`ParameterRange`] out is what a `step` needs, since no + /// Rust range carries one. + /// + /// Only available where the type describes a range of its own, which is what keeps the bounds + /// in the units the parameter is read back in and makes a literal that does not fit a compile + /// error. A parameter declared with a [`ParameterConversion`] has no `Range` to name and uses + /// [`Self::stored_ranges`] instead. + pub fn range(self, range: impl Into) -> Self { + let range: T::Range = range.into(); + self.stored_ranges(range.into()) + } +} + impl ParameterBuilder<'_, Arc<[T]>> where Arc<[T]>: ParameterVariant, @@ -271,9 +280,18 @@ pub struct AvailableValues<'a, T> { /// discriminator function to [`ParameterBuilder::discriminate()`]. pub fn default_initial_value_discriminator( available: AvailableValues, +) -> Option { + discriminate_by_preference(available, &ParameterConversion::::of_variant()) +} + +/// The body of [`default_initial_value_discriminator`], for a parameter whose conversion is not +/// the one its type describes for itself and so cannot be reached through [`ParameterVariant`]. +fn discriminate_by_preference( + available: AvailableValues, + conversion: &ParameterConversion, ) -> Option { if let Some(prior) = available.prior_value { - if available.ranges.in_range(&prior.clone().into()) { + if available.ranges.in_range(&conversion.to_value(&prior)) { return Some(prior); } } @@ -287,25 +305,26 @@ 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`. -fn wrap_validate_callback( +/// 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>, + conversion: ParameterConversion, ) -> ValidateCallback { Arc::new(move |pv: &ParameterValue| { - let typed: T = pv.clone().try_into().ok().expect( + let typed: T = conversion.from_value(pv.clone()).expect( "type mismatch in validate callback wrapper — parameter type is fixed at declaration", ); callback(&typed) }) } -impl TryFrom> for OptionalParameter { +impl TryFrom> for OptionalParameter { type Error = DeclarationError; fn try_from(builder: ParameterBuilder) -> Result { - let ranges = builder.options.ranges.clone().into(); + let ranges = builder.options.ranges.clone(); let initial_value = builder.interface.get_declaration_initial_value::( &builder.name, builder.default_value, @@ -313,6 +332,7 @@ impl TryFrom> for OptionalParameter builder.discard_mismatching_prior_value, builder.discriminator, &ranges, + &builder.conversion, )?; // Run the validate callback on the initial value (if both exist) @@ -325,20 +345,26 @@ impl TryFrom> for OptionalParameter let type_erased_validate = builder .validate .as_ref() - .map(|cb| wrap_validate_callback::(Arc::clone(cb))); + .map(|cb| wrap_validate_callback::(Arc::clone(cb), builder.conversion.clone())); - let value = Arc::new(RwLock::new(initial_value.map(|v| v.into()))); + let value = Arc::new(RwLock::new( + initial_value.map(|v| builder.conversion.to_value(&v)), + )); // The change_tx is used to notify async subscribers of changes to the parameter value. let (change_tx, _) = watch::channel(()); 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: builder.conversion.kind(), + options: builder.options, + type_check: type_check_of(builder.conversion.clone()), + validate: type_erased_validate, + on_change: None, + change_tx: Some(change_tx.clone()), + }, ); Ok(OptionalParameter { name: builder.name, @@ -347,7 +373,7 @@ impl TryFrom> for OptionalParameter map: Arc::downgrade(&builder.interface.parameter_map), change_tx, validate: builder.validate, - _marker: Default::default(), + conversion: builder.conversion, }) } } @@ -355,17 +381,18 @@ impl TryFrom> for OptionalParameter /// A parameter that must have a value /// This struct has ownership of the declared parameter. Additional parameter declaration will fail /// while this struct exists and the parameter will be undeclared when it is dropped. -pub struct MandatoryParameter { +pub struct MandatoryParameter { name: Arc, value: Arc>, ranges: ParameterRanges, map: Weak>, change_tx: watch::Sender<()>, validate: Option Result<(), String> + Send + Sync>>, - _marker: PhantomData, + /// The conversion the parameter was declared with. + conversion: ParameterConversion, } -impl Debug for MandatoryParameter { +impl Debug for MandatoryParameter { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("MandatoryParameter") .field("name", &self.name) @@ -375,7 +402,7 @@ impl Debug for MandatoryParameter { } } -impl Drop for MandatoryParameter { +impl Drop for MandatoryParameter { fn drop(&mut self) { // Clear the entry from the parameter map if let Some(map) = self.map.upgrade() { @@ -385,11 +412,11 @@ impl Drop for MandatoryParameter { } } -impl TryFrom> for MandatoryParameter { +impl TryFrom> for MandatoryParameter { type Error = DeclarationError; fn try_from(builder: ParameterBuilder) -> Result { - let ranges = builder.options.ranges.clone().into(); + let ranges = builder.options.ranges.clone(); let initial_value = builder.interface.get_declaration_initial_value::( &builder.name, builder.default_value, @@ -397,6 +424,7 @@ impl TryFrom> for MandatoryParamete builder.discard_mismatching_prior_value, builder.discriminator, &ranges, + &builder.conversion, )?; let Some(initial_value) = initial_value else { return Err(DeclarationError::NoValueAvailable); @@ -412,20 +440,24 @@ impl TryFrom> for MandatoryParamete let type_erased_validate = builder .validate .as_ref() - .map(|cb| wrap_validate_callback::(Arc::clone(cb))); + .map(|cb| wrap_validate_callback::(Arc::clone(cb), builder.conversion.clone())); - let value = Arc::new(RwLock::new(initial_value.into())); + let value = Arc::new(RwLock::new(builder.conversion.to_value(&initial_value))); // The change_tx is used to notify async subscribers of changes to the parameter value. let (change_tx, _) = watch::channel(()); 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: builder.conversion.kind(), + options: builder.options, + type_check: type_check_of(builder.conversion.clone()), + validate: type_erased_validate, + on_change: None, + change_tx: Some(change_tx.clone()), + }, ); Ok(MandatoryParameter { name: builder.name, @@ -434,7 +466,7 @@ impl TryFrom> for MandatoryParamete map: Arc::downgrade(&builder.interface.parameter_map), change_tx, validate: builder.validate, - _marker: Default::default(), + conversion: builder.conversion, }) } } @@ -442,17 +474,18 @@ impl TryFrom> for MandatoryParamete /// A parameter that might not have a value, represented by `Option`. /// This struct has ownership of the declared parameter. Additional parameter declaration will fail /// while this struct exists and the parameter will be undeclared when it is dropped. -pub struct OptionalParameter { +pub struct OptionalParameter { name: Arc, value: Arc>>, ranges: ParameterRanges, map: Weak>, change_tx: watch::Sender<()>, validate: Option Result<(), String> + Send + Sync>>, - _marker: PhantomData, + /// The conversion the parameter was declared with. + conversion: ParameterConversion, } -impl Debug for OptionalParameter { +impl Debug for OptionalParameter { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("OptionalParameter") .field("name", &self.name) @@ -462,7 +495,7 @@ impl Debug for OptionalParameter { } } -impl Drop for OptionalParameter { +impl Drop for OptionalParameter { fn drop(&mut self) { // Clear the entry from the parameter map if let Some(map) = self.map.upgrade() { @@ -475,14 +508,15 @@ impl Drop for OptionalParameter { /// A parameter that must have a value and cannot be written to /// This struct has ownership of the declared parameter. Additional parameter declaration will fail /// while this struct exists and the parameter will be undeclared when it is dropped. -pub struct ReadOnlyParameter { +pub struct ReadOnlyParameter { name: Arc, value: ParameterValue, map: Weak>, - _marker: PhantomData, + /// The conversion the parameter was declared with. + conversion: ParameterConversion, } -impl Debug for ReadOnlyParameter { +impl Debug for ReadOnlyParameter { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ReadOnlyParameter") .field("name", &self.name) @@ -491,7 +525,7 @@ impl Debug for ReadOnlyParameter { } } -impl Drop for ReadOnlyParameter { +impl Drop for ReadOnlyParameter { fn drop(&mut self) { // Clear the entry from the parameter map if let Some(map) = self.map.upgrade() { @@ -501,11 +535,11 @@ impl Drop for ReadOnlyParameter { } } -impl TryFrom> for ReadOnlyParameter { +impl TryFrom> for ReadOnlyParameter { type Error = DeclarationError; fn try_from(builder: ParameterBuilder) -> Result { - let ranges = builder.options.ranges.clone().into(); + let ranges = builder.options.ranges.clone(); let initial_value = builder.interface.get_declaration_initial_value::( &builder.name, builder.default_value, @@ -513,6 +547,7 @@ impl TryFrom> for ReadOnlyParameter builder.discard_mismatching_prior_value, builder.discriminator, &ranges, + &builder.conversion, )?; let Some(initial_value) = initial_value else { return Err(DeclarationError::NoValueAvailable); @@ -525,20 +560,26 @@ impl TryFrom> for ReadOnlyParameter validate(&initial_value).map_err(DeclarationError::InitialValueRejected)?; } - let value = initial_value.into(); + let value = builder.conversion.to_value(&initial_value); 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: builder.conversion.kind(), + options: builder.options, + type_check: type_check_of(builder.conversion.clone()), + // 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, value, map: Arc::downgrade(&builder.interface.parameter_map), - _marker: Default::default(), + conversion: builder.conversion, }) } } @@ -546,10 +587,29 @@ 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 at declaration time so that the parameter map can enforce the declared Rust type +/// without knowing what it is. The declaration's own [`ParameterConversion`] *is* the check, so +/// there is nothing extra for a type to implement and no way for the check to accept a value that +/// a later read would then reject. +fn type_check_of(conversion: ParameterConversion) -> ValidateCallback { + Arc::new(move |value: &ParameterValue| conversion.from_value(value.clone()).map(|_| ())) +} + struct DeclaredStorage { value: DeclaredValue, kind: ParameterKind, - options: ParameterOptionsStorage, + options: ParameterOptions, + /// 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 the declared + /// type. See [`type_check_of`]. + type_check: ValidateCallback, validate: Option, on_change: Option, change_tx: Option>, @@ -561,6 +621,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", &"..") @@ -631,6 +692,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()); } @@ -710,10 +780,13 @@ impl ParameterMap { } } -impl MandatoryParameter { +impl MandatoryParameter { /// Returns a clone of the most recent value of the parameter. pub fn get(&self) -> T { - self.value.read().unwrap().clone().try_into().ok().unwrap() + self.conversion + .from_value(self.value.read().unwrap().clone()) + .ok() + .unwrap() } /// Sets the parameter value. @@ -721,7 +794,7 @@ impl MandatoryParameter { /// Returns [`ParameterValueError::ValidationFailed`] if the validate callback rejects the value. pub fn set>(&self, value: U) -> Result<(), ParameterValueError> { let typed_value: T = value.into(); - let value: ParameterValue = typed_value.clone().into(); + let value = self.conversion.to_value(&typed_value); if !self.ranges.in_range(&value) { return Err(ParameterValueError::OutOfRange); } @@ -755,10 +828,11 @@ impl MandatoryParameter { let Some(map) = self.map.upgrade() else { return; }; + let conversion = self.conversion.clone(); let type_erased: OnChangeCallback = Arc::new(move |opt_pv: Option<&ParameterValue>| { // MandatoryParameter always has a value, so we always expect Some if let Some(pv) = opt_pv { - let typed: T = pv.clone().try_into().ok() + let typed: T = conversion.from_value(pv.clone()) .expect("type mismatch in on_change callback wrapper — parameter type is fixed at declaration"); callback(&typed); } @@ -778,28 +852,34 @@ impl MandatoryParameter { /// Multiple subscriptions can be created from the same parameter. pub fn subscribe(&self) -> ParameterSubscription { let value = Arc::clone(&self.value); + let conversion = self.conversion.clone(); ParameterSubscription { rx: self.change_tx.subscribe(), - get_value: Arc::new(move || value.read().unwrap().clone().try_into().ok().unwrap()), + get_value: Arc::new(move || { + conversion + .from_value(value.read().unwrap().clone()) + .ok() + .unwrap() + }), } } } -impl ReadOnlyParameter { +impl ReadOnlyParameter { /// Returns a clone of the most recent value of the parameter. pub fn get(&self) -> T { - self.value.clone().try_into().ok().unwrap() + self.conversion.from_value(self.value.clone()).ok().unwrap() } } -impl OptionalParameter { +impl OptionalParameter { /// Returns a clone of the most recent value of the parameter. pub fn get(&self) -> Option { self.value .read() .unwrap() .clone() - .map(|p| p.try_into().ok().unwrap()) + .map(|p| self.conversion.from_value(p).ok().unwrap()) } /// Assigns a value to the optional parameter, setting it to `Some(value)`. @@ -807,7 +887,7 @@ impl OptionalParameter { /// Returns [`ParameterValueError::ValidationFailed`] if the validate callback rejects the value. pub fn set>(&self, value: U) -> Result<(), ParameterValueError> { let typed_value: T = value.into(); - let value: ParameterValue = typed_value.clone().into(); + let value = self.conversion.to_value(&typed_value); if !self.ranges.in_range(&value) { return Err(ParameterValueError::OutOfRange); } @@ -859,10 +939,11 @@ impl OptionalParameter { let Some(map) = self.map.upgrade() else { return; }; + let conversion = self.conversion.clone(); let type_erased: OnChangeCallback = Arc::new(move |opt_pv: Option<&ParameterValue>| { match opt_pv { Some(pv) => { - let typed: T = pv.clone().try_into().ok() + let typed: T = conversion.from_value(pv.clone()) .expect("type mismatch in on_change callback wrapper — parameter type is fixed at declaration"); callback(Some(&typed)); } @@ -884,6 +965,7 @@ impl OptionalParameter { /// Multiple subscriptions can be created from the same parameter. pub fn subscribe(&self) -> ParameterSubscription> { let value = Arc::clone(&self.value); + let conversion = self.conversion.clone(); ParameterSubscription { rx: self.change_tx.subscribe(), get_value: Arc::new(move || { @@ -891,7 +973,7 @@ impl OptionalParameter { .read() .unwrap() .clone() - .map(|p| p.try_into().ok().unwrap()) + .map(|p| conversion.from_value(p).ok().unwrap()) }), } } @@ -955,6 +1037,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. @@ -966,6 +1052,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}"), } @@ -1023,18 +1113,31 @@ impl Parameters<'_> { /// /// Returns `Some(T)` if a parameter of the requested type exists, `None` otherwise. pub fn get(&self, name: &str) -> Option { + self.get_with(name, &ParameterConversion::::of_variant()) + } + + /// Tries to read a parameter, converting it with `conversion` rather than through the type. + /// + /// A parameter declared with a conversion of its own is reachable only this way, since its + /// type need not implement [`ParameterVariant`] at all. The conversion given here does not + /// have to be the one the parameter was declared with: this reads whatever is stored and + /// interprets it, which is the same freedom [`Self::get`] has over a type. + pub fn get_with( + &self, + name: &str, + conversion: &ParameterConversion, + ) -> Option { let storage = &self.interface.parameter_map.lock().unwrap().storage; let storage = storage.get(name)?; - match storage { + let value = match storage { ParameterStorage::Declared(storage) => match &storage.value { - DeclaredValue::Mandatory(p) => p.read().unwrap().clone().try_into().ok(), - DeclaredValue::Optional(p) => { - p.read().unwrap().clone().and_then(|p| p.try_into().ok()) - } - DeclaredValue::ReadOnly(p) => p.clone().try_into().ok(), + DeclaredValue::Mandatory(p) => p.read().unwrap().clone(), + DeclaredValue::Optional(p) => p.read().unwrap().clone()?, + DeclaredValue::ReadOnly(p) => p.clone(), }, - ParameterStorage::Undeclared(value) => value.clone().try_into().ok(), - } + ParameterStorage::Undeclared(value) => value.clone(), + }; + conversion.from_value(value).ok() } /// Tries to set a parameter with the requested value. @@ -1043,6 +1146,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. @@ -1051,6 +1157,20 @@ impl Parameters<'_> { &self, name: impl Into>, value: T, + ) -> Result<(), ParameterValueError> { + self.set_with(name, value, &ParameterConversion::::of_variant()) + } + + /// Tries to set a parameter, converting the value with `conversion` rather than through the + /// type. + /// + /// The counterpart of [`Self::get_with`], and the only way to write a parameter whose type + /// does not implement [`ParameterVariant`]. + pub fn set_with( + &self, + name: impl Into>, + value: T, + conversion: &ParameterConversion, ) -> Result<(), ParameterValueError> { let mut map = self.interface.parameter_map.lock().unwrap(); let name: Arc = name.into(); @@ -1061,8 +1181,12 @@ impl Parameters<'_> { // Undeclared parameters are dynamic by default match entry.get_mut() { ParameterStorage::Declared(param) => { - if T::kind() == param.kind { - let value = value.into(); + if conversion.kind() == param.kind { + let value = conversion.to_value(&value); + // This conversion is the caller's, which is not necessarily the one + // the parameter was declared with. The two only have to agree on 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); } @@ -1088,12 +1212,12 @@ impl Parameters<'_> { } } ParameterStorage::Undeclared(param) => { - *param = value.into(); + *param = conversion.to_value(&value); } } } Entry::Vacant(entry) => { - entry.insert(ParameterStorage::Undeclared(value.into())); + entry.insert(ParameterStorage::Undeclared(conversion.to_value(&value))); } } // Release the map lock before invoking on_change @@ -1134,12 +1258,26 @@ impl ParameterInterface { &'a self, name: Arc, ) -> ParameterBuilder<'a, T> { + self.declare_with(name, ParameterConversion::::of_variant()) + } + + pub(crate) fn declare_with<'a, T: 'static>( + &'a self, + name: Arc, + conversion: ParameterConversion, + ) -> ParameterBuilder<'a, T> { + // The default discriminator range-checks the prior value, which it can only do through + // the conversion, so it captures one rather than reaching for the type's own. + let for_discriminator = conversion.clone(); ParameterBuilder { name, default_value: None, ignore_override: false, discard_mismatching_prior_value: false, - discriminator: Box::new(default_initial_value_discriminator::), + discriminator: Box::new(move |available| { + discriminate_by_preference(available, &for_discriminator) + }), + conversion, options: Default::default(), interface: self, validate: None, @@ -1152,7 +1290,7 @@ impl ParameterInterface { Ok(()) } - fn get_declaration_initial_value<'a, T: ParameterVariant + 'a>( + fn get_declaration_initial_value<'a, T: 'static>( &self, name: &str, default_value: Option, @@ -1160,37 +1298,39 @@ impl ParameterInterface { discard_mismatching_prior: bool, discriminator: DiscriminatorFunction, ranges: &ParameterRanges, + conversion: &ParameterConversion, ) -> Result, DeclarationError> { ranges.validate()?; let override_value: Option = if ignore_override { None } else if let Some(override_value) = self.override_map.get(name).cloned() { Some( - override_value - .try_into() + conversion + .from_value(override_value) .map_err(|_| DeclarationError::OverrideValueTypeMismatch)?, ) } else { None }; - let prior_value = - if let Some(prior_value) = self.parameter_map.lock().unwrap().storage.get(name) { - match prior_value { - ParameterStorage::Declared(_) => return Err(DeclarationError::AlreadyDeclared), - ParameterStorage::Undeclared(param) => match param.clone().try_into() { - Ok(prior) => Some(prior), - Err(_) => { - if !discard_mismatching_prior { - return Err(DeclarationError::PriorValueTypeMismatch); - } - None + let prior_value = if let Some(prior_value) = + self.parameter_map.lock().unwrap().storage.get(name) + { + match prior_value { + ParameterStorage::Declared(_) => return Err(DeclarationError::AlreadyDeclared), + ParameterStorage::Undeclared(param) => match conversion.from_value(param.clone()) { + Ok(prior) => Some(prior), + Err(_) => { + if !discard_mismatching_prior { + return Err(DeclarationError::PriorValueTypeMismatch); } - }, - } - } else { - None - }; + None + } + }, + } + } else { + None + }; let selection = discriminator(AvailableValues { default_value, @@ -1199,33 +1339,19 @@ impl ParameterInterface { ranges, }); if let Some(initial_value) = &selection { - if !ranges.in_range(&initial_value.clone().into()) { + if !ranges.in_range(&conversion.to_value(initial_value)) { return Err(DeclarationError::InitialValueOutOfRange); } } 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) { @@ -2239,4 +2365,149 @@ mod tests { param.set(75).unwrap(); assert_eq!(sub.get(), 75); } + + /// 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)] + 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 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/conversion.rs b/rclrs/src/parameter/conversion.rs new file mode 100644 index 00000000..ab620251 --- /dev/null +++ b/rclrs/src/parameter/conversion.rs @@ -0,0 +1,262 @@ +//! How a Rust type is represented as a ROS 2 parameter value. + +use std::{fmt::Display, sync::Arc}; + +use super::{ParameterKind, ParameterValue, ParameterVariant}; + +/// How a Rust type is represented as a ROS 2 parameter value. +/// +/// A parameter's conversion is fixed when it is declared and carried with it from then on. It is +/// a value rather than a trait implementation, and the orphan rules do not apply to values, so a +/// crate can declare a parameter of a type it does not own. `std::time::Duration` and the types +/// of any other dependency are all reachable this way. +/// +/// Every [`ParameterVariant`] describes one, assembled by [`Self::of_variant`], so this is needed +/// only to use a type that has no implementation, or to choose a representation other than the one +/// a type declares for itself. +/// +/// The conversion back from a [`ParameterValue`] is fallible, because a parameter value arrives +/// from a parameter file or from a remote `SetParameters` call and cannot be trusted to be +/// representable. A conversion that genuinely cannot fail can return `Ok` unconditionally. +/// +/// # Example +/// +/// A duration stored as a number of seconds. The functions `std` already provides have the shapes +/// this needs, so there is nothing to write by hand. +/// +/// ``` +/// # use rclrs::*; +/// # use std::time::Duration; +/// let seconds = ParameterConversion::double(Duration::as_secs_f64, Duration::try_from_secs_f64); +/// +/// let executor = Context::default().create_basic_executor(); +/// let node = executor.create_node("drive_controller")?; +/// let timeout = node +/// .declare_parameter_with("timeout", seconds) +/// .default(Duration::from_millis(500)) +/// .mandatory()?; +/// +/// assert_eq!(timeout.get(), Duration::from_millis(500)); +/// # Ok::<(), Box>(()) +/// ``` +pub struct ParameterConversion { + kind: ParameterKind, + to_value: Arc ParameterValue + Send + Sync>, + from_value: Arc Result + Send + Sync>, +} + +// Written out rather than derived, because deriving would ask for `T: Clone` and a conversion is +// clonable whatever it converts. +impl Clone for ParameterConversion { + fn clone(&self) -> Self { + Self { + kind: self.kind, + to_value: Arc::clone(&self.to_value), + from_value: Arc::clone(&self.from_value), + } + } +} + +impl std::fmt::Debug for ParameterConversion { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ParameterConversion") + .field("kind", &self.kind) + .finish_non_exhaustive() + } +} + +/// Builds the body of a constructor for one ROS 2 parameter type. +/// +/// Each one is the same shape: wrap the value on the way out, and on the way in check that the +/// value is of the expected kind before handing it to the caller's function. The mismatch case is +/// what stops a conversion from ever seeing a value it was not written for. +macro_rules! conversion_constructor { + ($name:ident, $wire:ty, $variant:ident, $described:literal) => { + #[doc = concat!("A `T` represented as ", $described, ".")] + /// + /// `to` is how a value of the type becomes the stored value, and `from` how it is read + /// back. `from` may fail, and its error is reported as the reason the value was refused. + /// The error type has to outlive the conversion, which holds `from` for as long as the + /// parameter is declared. + pub fn $name( + to: fn(&T) -> $wire, + from: fn($wire) -> Result, + ) -> ParameterConversion { + ParameterConversion { + kind: ParameterKind::$variant, + to_value: Arc::new(move |value| ParameterValue::$variant(to(value))), + from_value: Arc::new(move |value| match value { + ParameterValue::$variant(wire) => from(wire).map_err(|err| err.to_string()), + other => Err(format!("expected {}, got {:?}", $described, other.kind())), + }), + } + } + }; +} + +impl ParameterConversion { + conversion_constructor!(boolean, bool, Bool, "a boolean"); + conversion_constructor!(integer, i64, Integer, "an integer"); + conversion_constructor!(double, f64, Double, "a double"); + conversion_constructor!(string, Arc, String, "a string"); + conversion_constructor!(byte_array, Arc<[u8]>, ByteArray, "a byte array"); + conversion_constructor!( + boolean_array, + Arc<[bool]>, + BoolArray, + "an array of booleans" + ); + conversion_constructor!( + integer_array, + Arc<[i64]>, + IntegerArray, + "an array of integers" + ); + conversion_constructor!(double_array, Arc<[f64]>, DoubleArray, "an array of doubles"); + conversion_constructor!( + string_array, + Arc<[Arc]>, + StringArray, + "an array of strings" + ); + + /// The ROS 2 parameter type a value of this conversion is stored as. + pub fn kind(&self) -> ParameterKind { + self.kind + } + + /// Converts a value into the parameter value that represents it. + pub fn to_value(&self, value: &T) -> ParameterValue { + (self.to_value)(value) + } + + /// Converts a stored parameter value back, reporting why if it cannot. + pub fn from_value(&self, value: ParameterValue) -> Result { + (self.from_value)(value) + } +} + +impl ParameterConversion { + /// The conversion a [`ParameterVariant`] describes for itself. + /// + /// Assembled from the type's kind, its constraints and its own `Into` and `TryFrom`, so the + /// trait states those once and there is no second way to say the same thing. Every parameter + /// declared by naming its type alone uses this. + pub fn of_variant() -> Self { + Self { + kind: T::kind(), + to_value: Arc::new(|value: &T| value.clone().into()), + from_value: Arc::new(|value| T::try_from(value).map_err(|err| err.to_string())), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + Context, CreateBasicExecutor, DeclarationError, ParameterRange, ParameterValueError, + }; + use std::time::Duration; + + /// Conversion for `std::time::Duration` stored as a number of seconds + fn seconds() -> ParameterConversion { + ParameterConversion::double(Duration::as_secs_f64, Duration::try_from_secs_f64) + } + + fn node(name: &str) -> crate::Node { + Context::default() + .create_basic_executor() + .create_node(name) + .unwrap() + } + + /// `Duration` belongs to `std` and so can never implement `ParameterVariant`. Saying how it + /// is represented is enough to declare it, to read and write it as itself through a handle, + /// and to reach it through the undeclared API, which has no type to dispatch on either. + #[test] + fn test_a_foreign_type_can_be_a_parameter() { + let node = node("foreign_type"); + let timeout = node + .declare_parameter_with("timeout", seconds()) + .default(Duration::from_millis(500)) + .mandatory() + .unwrap(); + + assert_eq!(timeout.get(), Duration::from_millis(500)); + timeout.set(Duration::from_secs(2)).unwrap(); + assert_eq!(timeout.get(), Duration::from_secs(2)); + + let undeclared = node.use_undeclared_parameters(); + // What is stored is the representation, which is what a parameter file would contain and + // what `ros2 param get` reports. + assert_eq!(undeclared.get::("timeout"), Some(2.0)); + + // Given the conversion, the undeclared API reads and writes it as a `Duration` too. + assert_eq!( + undeclared.get_with("timeout", &seconds()), + Some(Duration::from_secs(2)) + ); + undeclared + .set_with("timeout", Duration::from_secs(3), &seconds()) + .unwrap(); + assert_eq!(timeout.get(), Duration::from_secs(3)); + } + + /// The conversion decides the range's units too, since the range constrains the stored value. + #[test] + fn test_a_range_applies_to_the_stored_value() { + let node = node("foreign_range"); + let timeout = node + .declare_parameter_with("timeout", seconds()) + .default(Duration::from_secs(1)) + // `Duration` has no `ParameterVariant` impl and so no `Range` of its own to name. + // The bounds are written in what the conversion stores, which here is seconds. + .stored_ranges( + ParameterRange { + lower: Some(0.0), + upper: Some(5.0), + step: None, + } + .into(), + ) + .mandatory() + .unwrap(); + + assert!(timeout.set(Duration::from_secs(4)).is_ok()); + assert!(matches!( + timeout.set(Duration::from_secs(6)), + Err(ParameterValueError::OutOfRange) + )); + } + + /// A value already set before the declaration has to pass through the conversion too. One + /// that cannot fails the declaration rather than waiting to panic on the first read, which is + /// what a negative number of seconds would otherwise do. + #[test] + fn test_a_prior_value_the_conversion_refuses() { + let node = node("foreign_refused"); + node.use_undeclared_parameters() + .set("timeout", -1.0) + .unwrap(); + + let err = node + .declare_parameter_with("timeout", seconds()) + .default(Duration::from_secs(1)) + .mandatory() + .err() + .unwrap(); + assert_eq!(err, DeclarationError::PriorValueTypeMismatch); + } + + /// A type that does implement `ParameterVariant` keeps working through the conversion it + /// describes for itself, which is what `declare_parameter` uses. + #[test] + fn test_a_variant_supplies_its_own_conversion() { + let conversion = ParameterConversion::::of_variant(); + assert_eq!(conversion.kind(), ParameterKind::Double); + assert_eq!(conversion.to_value(&1.5), ParameterValue::Double(1.5)); + assert_eq!(conversion.from_value(ParameterValue::Double(1.5)), Ok(1.5)); + assert!(conversion.from_value(ParameterValue::Bool(true)).is_err()); + } +} diff --git a/rclrs/src/parameter/value.rs b/rclrs/src/parameter/value.rs index e646b30e..ed4cf808 100644 --- a/rclrs/src/parameter/value.rs +++ b/rclrs/src/parameter/value.rs @@ -57,7 +57,7 @@ pub enum ParameterValue { /// Describes the parameter's type. Similar to `ParameterValue` but also includes a `Dynamic` /// variant for dynamic parameters. -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq)] pub enum ParameterKind { /// A boolean parameter. Bool, @@ -137,7 +137,7 @@ 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;