feat(parameters): add a derive based API inspired from clap - #692
Open
azerupi wants to merge 4 commits into
Open
feat(parameters): add a derive based API inspired from clap#692azerupi wants to merge 4 commits into
azerupi wants to merge 4 commits into
Conversation
A node with more than a handful of parameters had to declare each one in
its own builder chain, restate its type, and keep the resulting handle
somewhere. Nothing tied the group together, and nothing related the shape
of the code to the shape of the parameter file configuring it.
A parameter set is a struct of plain Rust values whose fields are the
parameters. This adds the traits that describe one:
* ParameterSet, the group itself, with the namespace it declares under
and how to declare it.
* ParameterSetHandles, the live handles for a set, and the snapshot that
reads them back as plain values.
* DeclareField, which is what decides what a field of a set actually is.
A single parameter, an optional one, or a whole nested group is
determined by which implementation applies to the field's type, so a
nested set needs no annotation to be recognised as one.
* FieldSpec, the options for one field.
DeclareField is the extension point, and it is public: implement it, or
call declare_parameter_field! for a ParameterVariant of your own, and the
type can be a field of any set. Option<T> is implemented once for every
ParameterVariant rather than per type, because the orphan rules would not
let a downstream crate implement a trait from rclrs for Option<its own
type>.
Three ways to declare a set on a node, differing only in who owns the
declarations: declare_parameters returns handles that undeclare on drop,
retain_parameters has the node keep them and returns shared handles that
can be read at any time, and load_parameters is the latter plus a
snapshot, for a static configuration read once at startup.
Errors name the parameter that failed, 'limits.max_force' rather than just
"no value was available", which matters when a set declares thirty of
them. ParameterSetError converts into RclrsError so that `?` works in the
functions that return one.
The derive macro that makes these traits worth using follows in the next
commit.
Co-authored-by: balthasarschuess <balthasarschuess@users.noreply.github.com>
Co-authored-by: BCSol <BCSol@users.noreply.github.com>
Assisted-by: Claude:claude-opus-5 [Claude Code]
Implements the traits from the previous commit from a struct of plain Rust
values, so that a parameter set is written as the configuration it
describes rather than as a series of builder chains.
The macro emits the same shape of code for every field:
field: <FieldType as DeclareField<Mode>>::declare(node, &name, spec)?
so it never has to work out what kind of parameter a field is. Whether a
field is a single parameter, an optional one or a whole nested set of
parameters is decided by which DeclareField implementation applies to its
type. That is what lets a nested set be written with no annotation at all,
and lets a type defined outside rclrs be used as a parameter without the
macro knowing anything about it.
The macro does inspect field types, but only for diagnostics. Deciding by
trait resolution means a mistake is otherwise reported as an unsatisfied
trait bound, which is not something to hand a user, so the macro
recognises the types and attribute combinations most likely to be reached
for by mistake and says what to use instead, at the span of the field:
u64, usize, Duration, &str, Vec<SomeStruct>, a map of plain values, nested
Option, read_only combined with Option, flatten on a single parameter, a
range on an array, an exclusive range, and several more.
Those messages are tested by calling the expansion directly and asserting
on what it reports, rather than by compiling generated code and comparing
rustc's rendering of it, so the tests pin the wording the macro is
responsible for and do not break when a toolchain changes how it renders
a trait error.
rclrs depends on the new crate the way it already depends on
rosidl_runtime_rs: by version, resolved to the installed copy by
colcon-ros-cargo, with the dependency declared in package.xml so that
colcon knows to build it first and knows which prefix to write a [patch]
entry for. A path dependency would be simpler to read, but cargo
ament-build copies Cargo.toml into the install prefix verbatim, so a
relative path stops resolving there and the installed rclrs becomes
unusable to anything that depends on it.
For the same reason the manifest spells its metadata out rather than
inheriting it from [workspace.package]: the installed copy has no
workspace root to inherit from, and cargo refuses to parse it.
That leaves the crate outside the cargo workspace, so that building it
does not have to resolve rclrs's dependency on a version of itself that is
not published yet. colcon does not descend into a non-member directory of
a workspace, hence the additional-packages entry that puts it back in
view.
Co-authored-by: balthasarschuess <balthasarschuess@users.noreply.github.com>
Co-authored-by: BCSol <BCSol@users.noreply.github.com>
Assisted-by: Claude:claude-opus-5 [Claude Code]
A field of a parameter set had to be a type implementing ParameterVariant,
which the orphan rules put out of reach for any type belonging to another
crate. A duration had to be a newtype that existed only to carry its unit,
and a chrono or uom type could not be a field at all.
`#[param(convert = ...)]` gives the representation at the declaration
instead, so the field's type can be anything the conversion knows how to
store:
#[param(convert = seconds(), default = Duration::from_millis(500))]
timeout: Duration,
Such a field cannot go through DeclareField, because no implementation of
it for a foreign type could exist, so the derive names the handle type
directly and calls declare_converted, declare_converted_optional or
declare_converted_read_only with the conversion alongside the spec. The
range comes in erased for the same reason: there is no type to name a
Range on, so the bounds are in the units the conversion stores.
The mandatory, optional and read-only forms all work, and so do defaults,
ranges, overrides from a parameter file, and the values struct, which
holds the field's own type rather than anything from rclrs.
Duration's rejection message now points at convert rather than at the
wrapper types, and the wrappers leave the built-in type lists, since a
declaration says which unit it means without them.
Assisted-by: Claude:claude-opus-5 [Claude Code]
The derive took a range apart and reassembled it, which meant deciding for itself what a range may be. It rejected an exclusive range outright, so `range = 0..10` on an i64 field was an error in a set while the same range is accepted by the builder API. Pass the range through where the macro has no use for the bounds, and let the builder's own conversions decide. Every range the builder accepts now works in a set too, so what a range may be is stated in one place rather than two. A step is part of no Rust range, so it is added to the converted range rather than built into it, which leaves how the range is written none of its business. The macro still assembles the range for a converted field, whose spec holds the erased form that no Rust range converts into directly. That is the only case that still requires an inclusive or open range, and it keeps the message explaining why. Assisted-by: Claude:claude-opus-5 [Claude Code]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The code changes in this PR have been assisted by Claude Code but I have reviewed and iterated on the code.
This is a big one. The goal is to bring a derive based API for parameters inspired from clap to reduce boiler plate. This API is completely optional, you can still use the builder to declare all your parameters.
Problem
A node with more than a handful of parameters declares each one in its own builder chain, restates its type, and keeps the resulting handle somewhere. Nothing ties the group together, and nothing relates the shape of the code to the shape of the parameter file that configures it.
The namespace
limits.is a string typed by hand. Reading the whole configuration means reading each handle. Passing the configuration to the code that consumes it means passing rclrs types around, so that code cannot be tested without a node.Solution
Describe the parameters as a struct and derive the declaration, the way
clapderives an argument parser from a struct rather than a builder chain.and the struct now has the same shape as the file that configures it:
The doc comment becomes the parameter's description, so
ros2 param describereports it and the two cannot drift.The type of the field allows us to derive the type of parameter (except for read-only).
TOption<T>T: ParameterSetTwith#[param(read_only)]The macro does not guess based on heuristics or pattern matching, it emits the same code for every field and lets trait resolution pick the right
DeclareFieldimplementation. A nested set needs no annotation becauseParameterSettypes implement that trait.Declaring parameter sets
I have added 3 ways to declare a parameter set on the node. They differ in who holds the declarations:
load_parametersis the simplest one. The node will keep the parameter declarations alive but you only get the values at startup and don't have a way to read the values again. This is really a shortcut for static configurations.retain_parametersis similar except you get a clone of the parameter handles. With the handles you can get updated values later, you can setupon_changecallbacks etc. When you drop the handles the node will retain the parameter declarations alive andon_changecallbacks will continue to fire.declare_parametersgets you the parameter handles but the node doesn't retain them for you. So if you drop the handles the parameters are undeclared.Errors name the parameter
When you have errors, they name the field on which the error occurs so that the user can now exactly what to fix.
Custom conversions
The conversion machinery introduced in a prior PR can also be used to allow the use of types that you don't own.
#[param(convert = ...)]gives the representation at the declaration, so the field's type can be one whose crate could never implement a trait from rclrs:High-level design
The derive is deliberately thin. Almost all of the design lives in four traits in
rclrs, and the macro's job is to write out the same shape of code for every field and let the type system decide what each one means.Two structs per set
A set is two types: the values struct you write, holding plain Rust, and a handles struct the derive generates alongside it, holding the live parameter handles.
The split allows the application code to work with pure Rust types without any rclrs types. This makes it very easy to unit test, etc.
snapshot()is the function that allows to read all the parameter values from the handles into the struct.The four traits
ParameterSetDeclareFieldimplementation.ParameterSetHandlesDeclareFieldDeclareFlattenedParameterSetstructs that indicate they may be flattened into their parent's namespaceDeclareFieldis really the key to this design. Instead of trying to guess / pattern match in the macro what each field is (which I tried in an earlier version), the derive emits the exact same line for each field independently of what the field is:Nothing in that line says whether the field is a single parameter, an optional one, or a whole nested group. Which implementation applies to
FieldTypedecides.f64resolves to a mandatory parameter,Option<f64>to an optional one, and a type that derivesParameterSetresolves to a nested group, because the derive also emitsDeclareFieldfor the set itself.That is why a nested set needs no annotation, and why a type defined outside rclrs can be a field without the macro knowing anything about it.
Diagnostics
So far, the implementation of the macro as described above is fairly simple. However, since it is all trait based and trait error messages are not the nicest. I have made a particular effort to have good diagnostics for common errors. And this is probably where most of the complexity of the derive macro comes from.
known_types.rsrecognises a handful of likely mistakes,u64,Duration,&str, aVecof the wrong thing, so the macro can report them at the field's span:Anything it does not recognise is passed straight through to trait resolution. It's a bit of a double-edged sword because it makes the design very flexible and robust at the expense of clear and intuitive error messages. I have tried my best to fill as many gaps as I could.
The one exception
#[param(convert = ...)]cannot go throughDeclareField, because a conversion is a runtime value and no implementation can be selected by one. For those fields the derive names the handle type itself and callsdeclare_converteddirectly. It is the only place the macro has to know what kind of parameter it is emitting, and it is the price of letting a field be a type whose crate could never implement a trait from rclrs.