diff --git a/rclrs-macros/src/parameter_set/codegen.rs b/rclrs-macros/src/parameter_set/codegen.rs index 5403dfcd..294ad073 100644 --- a/rclrs-macros/src/parameter_set/codegen.rs +++ b/rclrs-macros/src/parameter_set/codegen.rs @@ -164,8 +164,8 @@ fn handle_field_doc(field: &Field) -> String { /// The two are kept apart because they do not carry the same authority. See [`default_value`]. #[derive(Clone, Copy)] pub(crate) enum DefaultSource { - /// The value passed in for this one instance of the set, from a parent field's `default` or - /// from the caller of `declare_parameters`. + /// The value passed in for this one instance of the set, from a parent field's `default`, + /// from the caller of `declare_parameters`, or from one entry of a map. Supplied, /// The set's own `#[parameters(default = ...)]`. Own, diff --git a/rclrs-macros/src/parameter_set/expand_tests.rs b/rclrs-macros/src/parameter_set/expand_tests.rs index 410159ad..108d5f0d 100644 --- a/rclrs-macros/src/parameter_set/expand_tests.rs +++ b/rclrs-macros/src/parameter_set/expand_tests.rs @@ -129,13 +129,28 @@ fn test_rejects_sequences_of_things_ros_has_no_array_type_for() { } #[test] -fn test_rejects_maps() { +fn test_rejects_a_map_of_plain_values() { rejected_with( "struct C { extra: HashMap }", - &["no map parameter type"], + &["no map parameter type", "#[derive(ParameterSet)]"], ); } +#[test] +fn test_rejects_a_map_whose_keys_are_not_names() { + rejected_with( + "struct C { sensors: HashMap }", + &["have to be `String`"], + ); +} + +/// A map of parameter sets is how entries named by whoever configures the node are declared. +#[test] +fn test_accepts_a_map_of_parameter_sets() { + accepted("struct C { sensors: HashMap }"); + accepted("struct C { sensors: BTreeMap }"); +} + #[test] fn test_rejects_nested_option() { rejected_with( diff --git a/rclrs-macros/src/parameter_set/known_types.rs b/rclrs-macros/src/parameter_set/known_types.rs index 7aeab6d5..ee0f8b88 100644 --- a/rclrs-macros/src/parameter_set/known_types.rs +++ b/rclrs-macros/src/parameter_set/known_types.rs @@ -227,12 +227,30 @@ pub(crate) fn shape_of(ty: &Type) -> TypeShape { } } - // A map is a recognised mistake rather than an unrecognised type, since no `DeclareField` - // implementation could ever make one work. - if key.starts_with("HashMap<") || key.starts_with("BTreeMap<") { - return TypeShape::Rejected(format!( - "`{key}` cannot be a ROS 2 parameter: ROS 2 has no map parameter type" - )); + // A map is a parameter set per entry, with the entry names coming from the parameters the + // node was configured with. The values therefore have to be sets, and the keys have to be + // the names of those entries. + if let Some(arguments) = key + .strip_prefix("HashMap<") + .or_else(|| key.strip_prefix("BTreeMap<")) + .and_then(|k| k.strip_suffix('>')) + { + let (map_key, map_value) = arguments.split_once(',').unwrap_or((arguments, "")); + + if map_key != "String" { + return TypeShape::Rejected(format!( + "the keys of a parameter map are the names its entries are declared under, so \ + they have to be `String`, not `{map_key}`" + )); + } + + if NUMERIC_LEAVES.contains(&map_value) || OTHER_LEAVES.contains(&map_value) { + return TypeShape::Rejected(format!( + "`{key}` cannot be a ROS 2 parameter: ROS 2 has no map parameter type. A map \ + field declares a parameter set for each of its entries, so its values have to \ + be types with `#[derive(ParameterSet)]`" + )); + } } // `Option` is an optional parameter when `T` is a parameter type. `Option` of anything diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index ddd428e6..e6102464 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -1464,7 +1464,6 @@ impl NodeState { } /// Access to this node's parameter interface, for the parameter implementation itself. - #[cfg(test)] pub(crate) fn parameter_interface(&self) -> &ParameterInterface { &self.parameter } diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs index 3e0d3dc7..15dbb9b6 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -20,7 +20,7 @@ use crate::{ call_string_getter_with_rcl_node, rcl_bindings::*, Node, RclrsError, ENTITY_LIFECYCLE_MUTEX, }; use std::{ - collections::{btree_map::Entry, BTreeMap}, + collections::{btree_map::Entry, BTreeMap, BTreeSet}, fmt::Debug, sync::{Arc, Mutex, RwLock, Weak}, }; @@ -1383,6 +1383,32 @@ impl ParameterInterface { .insert(name, ParameterStorage::Declared(storage)); } + /// The distinct names that appear directly under `prefix` in the parameter overrides. + /// + /// For overrides `sensors.front.rate` and `sensors.rear.rate`, the names under `sensors` are + /// `front` and `rear`. This is how a map field, whose entries are named by whoever configures + /// the node, finds out what those names are. ROS 2 has no map parameter type, and overrides + /// are not otherwise visible until the parameter they name is declared. + pub(crate) fn override_names_under(&self, prefix: &str) -> BTreeSet { + let scope = if prefix.is_empty() { + String::new() + } else { + format!("{prefix}.") + }; + + self.override_map + .range(scope.clone()..) + .take_while(|(name, _)| name.starts_with(&scope)) + .filter_map(|(name, _)| { + let rest = &name[scope.len()..]; + // Only a name with something after it is a namespace, and so an entry of the map. + // `sensors: 3` alongside `sensors.front.rate` is a different parameter entirely. + let (entry, _) = rest.split_once('.')?; + (!entry.is_empty()).then(|| entry.to_string()) + }) + .collect() + } + pub(crate) fn allow_undeclared(&self) { self.parameter_map.lock().unwrap().allow_undeclared = true; } @@ -1392,6 +1418,10 @@ impl ParameterInterface { #[path = "parameter/enum_set_tests.rs"] mod enum_set_tests; +#[cfg(test)] +#[path = "parameter/map_tests.rs"] +mod map_tests; + #[cfg(test)] #[path = "parameter/variant_tests.rs"] mod variant_tests; diff --git a/rclrs/src/parameter/map_tests.rs b/rclrs/src/parameter/map_tests.rs new file mode 100644 index 00000000..befa96e0 --- /dev/null +++ b/rclrs/src/parameter/map_tests.rs @@ -0,0 +1,324 @@ +//! Tests for map fields: parameter sets whose entries are named by whoever configures the node. + +use std::collections::{BTreeMap, HashMap}; + +use crate::{parameter::test_support::node_with_parameter_overrides as node_with_params, *}; + +/// Settings for one sensor. +#[derive(ParameterSet, Debug, PartialEq, Clone)] +struct SensorConfig { + /// Publish rate in Hz. + #[param(default = 30, range = 1..=100)] + rate: i64, + /// Frame this sensor reports in. + #[param(default = "base_link")] + frame_id: String, +} + +/// A node that manages however many sensors it is configured with. +#[derive(ParameterSet, Debug, PartialEq)] +struct SensorHub { + /// One entry per sensor, named in the parameter file. + sensors: BTreeMap, +} + +/// The names of the entries cannot be known when the node is written, and are not visible +/// anywhere until the parameters they name are declared. They come from the overrides. +#[test] +fn test_entry_names_come_from_the_parameter_file() { + let (node, _file) = node_with_params( + "sensor_hub", + r#" +/sensor_hub: + ros__parameters: + sensors: + front_lidar: + rate: 40 + rear_lidar: + rate: 10 + frame_id: rear_mount +"#, + ); + + let config: SensorHub = node.load_parameters().unwrap(); + assert_eq!( + config.sensors.keys().collect::>(), + vec!["front_lidar", "rear_lidar"] + ); + assert_eq!( + config.sensors["front_lidar"], + SensorConfig { + rate: 40, + // Not in the file, so the field's own default. + frame_id: "base_link".to_string(), + } + ); + assert_eq!( + config.sensors["rear_lidar"], + SensorConfig { + rate: 10, + frame_id: "rear_mount".to_string(), + } + ); +} + +/// Every entry's fields are ordinary parameters: named, described, ranged and live. +#[test] +fn test_the_entries_are_ordinary_parameters() { + let (node, _file) = node_with_params( + "sensor_hub_params", + r#" +/sensor_hub_params: + ros__parameters: + sensors: + front_lidar: + rate: 40 +"#, + ); + + let params = node.declare_parameters::().unwrap(); + let front = ¶ms.sensors["front_lidar"]; + assert_eq!(front.rate.get(), 40); + front.rate.set(50).unwrap(); + assert!(front.rate.set(500).is_err(), "the range still applies"); + + assert_eq!( + node.use_undeclared_parameters() + .get::("sensors.front_lidar.rate"), + Some(50) + ); +} + +#[test] +fn test_no_entries_is_an_empty_map_rather_than_an_error() { + let node = Context::default() + .create_basic_executor() + .create_node("sensor_hub_empty") + .unwrap(); + + let config: SensorHub = node.load_parameters().unwrap(); + assert!(config.sensors.is_empty()); +} + +/// A default supplies entries of its own, so a node can have built-in ones. +#[derive(ParameterSet, Debug, PartialEq)] +#[parameters(default = Self::default())] +struct DefaultedHub { + sensors: BTreeMap, +} + +impl Default for DefaultedHub { + fn default() -> Self { + let mut sensors = BTreeMap::new(); + sensors.insert( + "builtin".to_string(), + SensorConfig { + rate: 5, + frame_id: "builtin_frame".to_string(), + }, + ); + Self { sensors } + } +} + +#[test] +fn test_a_default_supplies_entries() { + let node = Context::default() + .create_basic_executor() + .create_node("hub_defaulted") + .unwrap(); + + let config: DefaultedHub = node.load_parameters().unwrap(); + assert_eq!(config.sensors.len(), 1); + assert_eq!(config.sensors["builtin"].rate, 5); +} + +#[test] +fn test_a_parameter_file_adds_to_and_overrides_the_default_entries() { + let (node, _file) = node_with_params( + "hub_defaulted_yaml", + r#" +/hub_defaulted_yaml: + ros__parameters: + sensors: + builtin: + rate: 50 + extra: + rate: 20 +"#, + ); + + let config: DefaultedHub = node.load_parameters().unwrap(); + assert_eq!(config.sensors.len(), 2); + // The file wins for an entry that exists in both... + assert_eq!(config.sensors["builtin"].rate, 50); + // ...but the default still supplies the fields the file does not mention. + assert_eq!(config.sensors["builtin"].frame_id, "builtin_frame"); + // And a new entry appears. + assert_eq!(config.sensors["extra"].rate, 20); + assert_eq!(config.sensors["extra"].frame_id, "base_link"); +} + +/// An error inside an entry names the entry, since that is what the caller has to go and look at. +#[derive(ParameterSet)] +struct RequiredHub { + sensors: BTreeMap, +} + +#[derive(ParameterSet)] +struct RequiredSensor { + /// No default: the file has to supply it. + rate: i64, +} + +#[test] +fn test_an_error_inside_an_entry_names_the_entry() { + let (node, _file) = node_with_params( + "hub_required", + r#" +/hub_required: + ros__parameters: + sensors: + front_lidar: + frame_id: x +"#, + ); + + let err = node.declare_parameters::().err().unwrap(); + assert_eq!(err.name, "sensors.front_lidar.rate"); + assert_eq!(err.source, DeclarationError::NoValueAvailable); +} + +/// A `HashMap` works the same way, and only the iteration order differs. +#[derive(ParameterSet, Debug, PartialEq)] +struct HashHub { + sensors: HashMap, +} + +#[test] +fn test_a_hash_map_field() { + let (node, _file) = node_with_params( + "hash_hub", + "/hash_hub:\n ros__parameters:\n sensors:\n a:\n rate: 1\n", + ); + + let config: HashHub = node.load_parameters().unwrap(); + assert_eq!(config.sensors["a"].rate, 1); +} + +/// The names of the entries are the first path segment under the map's own name. Anything that +/// merely starts with the same characters is a different parameter, and a name with nothing under +/// it is not an entry at all. +#[test] +fn test_which_override_names_count_as_entries() { + let (node, _file) = node_with_params( + "hub_names", + r#" +/hub_names: + ros__parameters: + sensors: + front_lidar: + rate: 40 + rear_lidar: + nested: + deeper: 1 + sensors_extra: + rate: 1 + unrelated: 2 +"#, + ); + + let names = node + .parameter_interface() + .override_names_under("sensors") + .into_iter() + .collect::>(); + // `sensors_extra` shares a prefix but is not under `sensors`, and a name is reported once + // however deeply nested the parameters beneath it are. + assert_eq!(names, vec!["front_lidar", "rear_lidar"]); + + // A name with no parameters under it has no entries of its own. + assert!(node + .parameter_interface() + .override_names_under("unrelated") + .is_empty()); + assert!(node + .parameter_interface() + .override_names_under("nonexistent") + .is_empty()); +} + +// --------------------------------------------------------------------------------------------- +// Entries that are not all the same shape +// --------------------------------------------------------------------------------------------- + +/// Configuration for one device, whatever kind it is. +#[derive(ParameterSet, Debug, PartialEq)] +#[parameters(rename_all = "snake_case")] +enum DeviceConfig { + /// A 2D scanning lidar. + Lidar { + /// Scan rate in Hz. + #[param(default = 30)] + rate: i64, + }, + /// A camera. + Camera { + /// Frame width in pixels. + #[param(default = 1920)] + width: u32, + /// Frame height in pixels. + #[param(default = 1080)] + height: u32, + }, +} + +/// A node that manages however many devices it is configured with, of whatever kinds. +#[derive(ParameterSet, Debug, PartialEq)] +struct DeviceHub { + /// One entry per device. + devices: BTreeMap, +} + +/// Maps and enum sets compose: the entries are named by the parameter file *and* each one +/// declares only the parameters its own kind needs. +#[test] +fn test_entries_can_have_different_shapes() { + let (node, _file) = node_with_params( + "device_hub", + r#" +/device_hub: + ros__parameters: + devices: + front_lidar: + type: lidar + rate: 40 + main_camera: + type: camera + width: 640 +"#, + ); + + let config: DeviceHub = node.load_parameters().unwrap(); + assert_eq!( + config.devices["front_lidar"], + DeviceConfig::Lidar { rate: 40 } + ); + assert_eq!( + config.devices["main_camera"], + DeviceConfig::Camera { + width: 640, + height: 1080, + } + ); + + // Each entry declared only what its own kind needs. + let undeclared = node.use_undeclared_parameters(); + assert_eq!(undeclared.get::("devices.front_lidar.rate"), Some(40)); + assert_eq!(undeclared.get::("devices.front_lidar.width"), None); + assert_eq!( + undeclared.get::("devices.main_camera.width"), + Some(640) + ); + assert_eq!(undeclared.get::("devices.main_camera.rate"), None); +} diff --git a/rclrs/src/parameter/set.rs b/rclrs/src/parameter/set.rs index 604eeff7..e2d939fd 100644 --- a/rclrs/src/parameter/set.rs +++ b/rclrs/src/parameter/set.rs @@ -20,7 +20,12 @@ //! [`NodeState::retain_parameters`]: crate::NodeState::retain_parameters //! [`NodeState::load_parameters`]: crate::NodeState::load_parameters -use std::{fmt::Debug, path::PathBuf, sync::Arc}; +use std::{ + collections::{BTreeMap, HashMap}, + fmt::Debug, + path::PathBuf, + sync::Arc, +}; use crate::{ AvailableValues, DeclarationError, NodeState, ParameterBuilder, ParameterValue, @@ -448,6 +453,65 @@ pub fn declare_converted_read_only( .map_err(|e| ParameterSetError::new(name, e)) } +/// A map field is a parameter set per entry, with the entry names taken from the parameters the +/// node was configured with. +/// +/// ROS 2 has no map parameter type, and no way to ask what parameters were provided before +/// declaring them, so the names are recovered from the node's parameter overrides: for +/// `sensors.front.rate` and `sensors.rear.rate`, a `sensors` map declares an entry named `front` +/// and one named `rear`, each a full [`ParameterSet`] in its own right. +/// +/// Points worth knowing: +/// +/// * The set of entries is fixed when the map is declared. A name that appears later, over +/// `SetParameters`, refers to a parameter that was never declared and is rejected like any +/// other undeclared parameter. +/// * A default value contributes its entries too, so a map can have built-in entries that a +/// parameter file adds to or overrides. +/// * No overrides and no default means an empty map, not an error. +macro_rules! declare_parameter_map { + ($($map:ident),* $(,)?) => { $( + impl DeclareField for $map { + type Value = Self; + type Handle = $map; + type Range = (); + + fn declare( + node: &NodeState, + name: &str, + spec: FieldSpec, + ) -> Result { + let mut defaults = spec.default.unwrap_or_default(); + // Declared in name order, so that which entry fails first does not depend on the + // iteration order of a hash map. + let mut entries = node.parameter_interface().override_names_under(name); + entries.extend(defaults.keys().cloned()); + + let mut handles = $map::new(); + for entry in entries { + let default = defaults.remove(&entry); + let prefix = join_parameter_name(name, &entry); + handles.insert(entry, S::declare(node, &prefix, default)?); + } + Ok(handles) + } + + fn snapshot(handle: &Self::Handle) -> Self { + handle + .iter() + .map(|(entry, handles)| (entry.clone(), handles.snapshot())) + .collect() + } + + fn into_default(self) -> Option { + Some(self) + } + } + )* }; +} + +declare_parameter_map!(BTreeMap, HashMap); + /// Implements [`DeclareField`] for a [`ParameterVariant`], so that it can be used as the type of /// a [`ParameterSet`] field. ///