From 5342dd087a95b816cfbf8f474c9c1dbabff85292 Mon Sep 17 00:00:00 2001 From: Mathieu David Date: Sat, 25 Jul 2026 16:29:06 +0200 Subject: [PATCH] feat(parameters): declare a parameter set per entry of a map A driver often manages a set of devices whose names only the integrator knows: the sensors on a robot, the motors on an arm. That was not expressible. Declaring sensors.front_lidar.rate requires code that already knows the string "front_lidar", and there was no way to find it out: overrides live in a private map on the parameter interface and are never inserted into the parameter map, so use_undeclared_parameters cannot see a parameter the node has not declared. The choice was to hard-code the device names or to stop using ROS 2 parameters for those values, giving up descriptors, ranges, validation, ros2 param list and the parameter services. A map field declares a parameter set for each of its entries, with the entry names recovered from the parameters the node was configured with: #[derive(ParameterSet, Debug)] struct SensorHub { sensors: BTreeMap, } Every leaf is an ordinary parameter, so sensors.front_lidar.rate has the description and range SensorConfig gives it and can be watched for changes like any other. Combined with an enum set, the entries do not have to be the same kind of thing. The entries are fixed when the map is declared, since that is when the parameters are declared. A name that turns up later names a parameter that does not exist. A default value contributes entries too, so a node can have built-in ones that a parameter file adds to or overrides -- which is what a default supplied for a whole set taking precedence over a field's own default is for. Assisted-by: Claude:claude-opus-5 [Claude Code] --- rclrs-macros/src/parameter_set/codegen.rs | 4 +- .../src/parameter_set/expand_tests.rs | 19 +- rclrs-macros/src/parameter_set/known_types.rs | 30 +- rclrs/src/node.rs | 1 - rclrs/src/parameter.rs | 32 +- rclrs/src/parameter/map_tests.rs | 324 ++++++++++++++++++ rclrs/src/parameter/set.rs | 66 +++- 7 files changed, 463 insertions(+), 13 deletions(-) create mode 100644 rclrs/src/parameter/map_tests.rs 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. ///