feat(parameters): parameter conversion - #688
Open
azerupi wants to merge 3 commits into
Open
Conversation
ParameterOptions was generic over the parameter type so that it could hold that type's own Range, and a private ParameterOptionsStorage existed alongside it to hold the erased ParameterRanges that the descriptor and every range check actually use. The two structs differed in that one field and nothing else, and each declaration path converted between them. A range describes the values a parameter may hold, which is a property of how the value is represented rather than of the Rust type it is read back as. Holding the erased form throughout collapses the two structs into one, and the three declaration paths stop converting at all. range() still takes the parameter type's own Range, so the bounds are still written in the units the field is read back in and a literal that does not fit is still a compile error. Only where the value is stored changes. ParameterOptions also stops being public, which it only ever was by accident. Its fields are private, no function takes or returns one, and its only uses are as a private field of the builder and of the stored parameter, so all a caller could do with the name was construct an empty one through Default and then find nothing that would accept it. Assisted-by: Claude:claude-opus-5 [Claude Code]
A parameter's conversion was reachable only through trait resolution. ParameterVariant supplied the kind and both directions, and every read, write and declaration went through the type. That shut out any type whose crate does not own the trait: std::time::Duration cannot implement ParameterVariant, and neither can a chrono or uom type, so configuring one meant a newtype in the caller's crate that existed for no other reason. Move the conversion into a value. ParameterConversion holds the kind and the two directions, a declaration fixes one, and every handle it produces carries it. ParameterConversion::of_variant assembles one from what a ParameterVariant already states, so a type that has an implementation needs nothing further and declares exactly as before. The trait keeps stating its kind and its conversions and does not also hand out an assembled conversion, so there is no second way to say the same thing and no way for the two to disagree. The orphan rules do not apply to values, so declare_parameter_with takes a foreign type directly. The conversion back from a parameter value is fallible, because that value arrives from a parameter file or from a remote SetParameters call and cannot be assumed representable. The constructors take the shapes std's own functions already have, so a duration in seconds is ParameterConversion::double(Duration::as_secs_f64, Duration::try_from_secs_f64) with nothing written by hand. get_with and set_with give the undeclared parameter API the same reach, so a parameter whose type has no implementation is readable and writable there too. The handles' Debug implementations lose their ParameterVariant bound, which was only ever needed for the conversion they now hold. The range setter splits in two. range() keeps resolving to the parameter type's own Range and moves to an impl block that still requires ParameterVariant, so bounds stay in the units the parameter is read back in and a literal that does not fit is still a compile error. stored_ranges() takes the erased form, in the terms the conversion stores the value in, for a parameter whose type has no Range to name. Two names because the blocks overlap and one name would be a duplicate definition. No existing test changed, which is what says the default conversion is the one the types already described for themselves. Assisted-by: Claude:claude-opus-5 [Claude Code]
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 in
the type a parameter was declared with. validate_parameter_setting only
compared kind discriminants and Parameters::set only compared kinds,
while every getter assumes the stored value converts back:
self.conversion.from_value(..).ok().unwrap()
So a remote SetParameters call could store a value that passes validation
and then panic the node on the next read. This is reachable today by
anyone implementing the public ParameterVariant trait for a type whose
conversion from ParameterValue is partial, and becomes unavoidable once
narrow integer and enum parameter types exist.
Run the declaration's conversion on both write paths, before the range
check and before the custom validate callback, whose type-erasure wrapper
depends on the value being convertible. The check is the conversion the
declaration already carries, so there is nothing extra for a type to
implement and no way for it to accept a value that a later read would
then reject. ParameterValueError::Invalid carries the reason the
conversion gave back to the caller, which the parameter service puts in
the response so the operator who made the call can see why the value was
refused.
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.
Problem
A parameter's conversion was defined only through trait resolution.
ParameterVariantsupplied the kind and conversion in both directions, and every read, write and declaration went through the type's conversion.But this has 2 problems:
ParameterVariantlives in rclrs and only rclrs can implement it for foreign types. This means that any type from the std or other crates that doesn't have aParameterVarianttrait implementation in rclrs can't be used by the user.std::time::Durationcould be stored in the parameter as secodns (f64) or milliseconds (i64) or even another representation. But you can only have one implementation ofParameterVariant.Both of those cases required the user to make newtypes and implement
ParameterVariantfor them.Solution
The conversion becomes a value that a declaration carries, rather than a trait implementation the type must have.
Nothing about the existing API changes.
declare_parameter::<T>(name)is nowdeclare_parameter_with(name, ParameterConversion::of_variant::<T>()), and a type with aParameterVariantimpl declares exactly as before.get_withandset_withgiveuse_undeclared_parameters()the same reach, so a parameter whose type has noParameterVariantis readable and writable there as well:Bug fix
ParameterKinddoes not identify a Rust type. Several types share one, and a value of the right kind can still be unrepresentable in the declared type, for example70000is a perfectly goodIntegerbut not au16.In current main the parameter write paths only compared kinds, while every getter assumes the stored value converts to the Rust type. This means that setting a bad parameter could succeed and make the node panic. It is even worse when you think a parameter can be changed externally to the process.
So right now in main, a remote
SetParameterscall could store a value that passed validation and then panicked the node on the next read. And while this can't happen if we use the standard ROS types as parameters it is reachable today by anyone implementing the publicParameterVarianttrait for a type whoseTryFromis partial. And this bug becomes unavoidable once narrow integers are used as parameters, which is a feature I want to add in a later PR.The declaration's conversion is the check that needs to pass, so in the setters we call this function in addition to the kind check:
ParameterValueError::Invalidcarries the reason the conversion gave, and the parameter service puts it in the response, so a user runningros2 param setsees why a value was refused.