Derive macro for StructuredParameters

[BCSol]

Proposal for structured parameters derive macro (#496)

Changes:

• added trait StructuredParameters
• added derive macro in new rclrs_proc_macros
• added poc some tests
• install test_msgs and example_messages with apt, without the cargo build without colon would fail to link rosidl message (I'd be happy about some help there)

TODO:

• proper documentation of traits
• default values should be made possible
• more tests
• add generic declare macro to NodeState/Node

@mxgrey before I continue here, I'd be happy about some feedback, whether this is what you had in mind or how we could improve upon this POC.

[esteve]

@balthasarschuess @BCSol thanks for the PR, it doesn't seem necessary to have a separate crate only for the macros, can you move that into rclrs? Thanks.

[balthasarschuess]

@balthasarschuess @BCSol thanks for the PR, it doesn't seem necessary to have a separate crate only for the macros, can you move that into rclrs? Thanks.

Hey @esteve :)
thanks for having a look. I think this is not possible as proc-macros require the crate to be marked as proc macro and cannot be used inside the crate they are defined in (https://doc.rust-lang.org/reference/procedural-macros.html#r-macro.proc.def)
I think it's better to have the proc macro tests inside rclrs.

I might be mistaken, but I could not find any resource claiming that this restriction has been lifted.
In case can you point me towards the resource.

[balthasarschuess]

Colcon autodiscovers proc-macros through the workspace members, but fails to build it on windows, as the package.xml is missing and it doesn't know how to build the package
However, the build without the package.xml appears to work on linux.
I think this is a bug in colcon-ros-cargo/colcon-cargo, as the package should not be listed as ros package in the first place. As it clearly is not a ros package due to the lack of the package.xml

[balthasarschuess]

@esteve I finally made the windows build work, can you have another look 🙏

[azerupi]

Hey @balthasarschuess @BCSol,
I was reviewing this PR because I'm excited about this feature but as I was looking through the code and trying to understand some design decisions I was thinking about slightly different approaches that could be taken and I ended up doing some exploration with AI help which resulted in #612

I apologize, my goal wasn't to dismiss the work that was done in this PR, it was just easier for me to come up with an alternative proposal than to dump a bunch of review comments here.

Feel free to take anything from #612 and incorporate it into this PR if you want. If however the decision is to move forward with #612, then I'll happily add both of you as co-authors on the commits as the work was based on the initial ideas in this PR.

[azerupi]

The three implementations of this trait seem to be almost identical except for the last calls to mandatory(), read_only() and optional(). Can the common code be extracted into a single function that gets called in all three impls?

[balthasarschuess]

Not really unless we generate them directly as in #612

[azerupi]

Why do we have to add these two impls?

[balthasarschuess]

The constraint is a consequence of having the same Trait for both nodes and leafs, where the declare signature contains parameters.
The default value requires this bound, but there is not a 1:1 correspondence between a ParameterVariant and a "parameter container".

That's why the DefaultForbidden type was added so that we can implement/derive the trait and keep the information, that there cannot be a default value.

[balthasarschuess]

Maybe it's possible to remove the ParameterVariant constraint from the type parameter for the trait.
Then these two implementations could be removed.

[balthasarschuess]

Hey @azerupi

I am really happy about your feedback!
I think your solution is more elegant in terms of that it avoids the somewhat unsound type implementation. It would be interesting to evaluate, which version yields better compiler errors. My version was lacking in this regard a bit.

The major reason why I chose this implementation over directly generating the leaf implementations, was that I wanted users to give the option to implement the Trait for their custom type.

In our use case we would like to support struct values, which would be internally declared as ReadOnlyParameter<Arc<str>> but are directly deserialized with serde_json to a more complex data structure. As ros parameters don't support more complex map types or conditional parsing yet.

I would be nice if something like that worked without intermediate structs

#[derive(serde::Deserialize)]
struct Config {
   param: String,
   param2: Map<String, String>,
}

node.declare_parameters::<Config>()

This all begs the question if we should directly support serde deserializable as ParameterType.

[balthasarschuess]

The constraint is a consequence of having the same Trait for both nodes and leafs, where the declare signature contains parameters.
The default value requires this bound, but there is not a 1:1 correspondence between a ParameterVariant and a "parameter container".

That's why the DefaultForbidden type was added so that we can implement/derive the trait and keep the information, that there cannot be a default value.

[balthasarschuess]

Not really unless we generate them directly as in #612

[azerupi]

@balthasarschuess that's an interesting requirement!

I think this feature is somewhat orthogonal to the derive macro so let's talk about what the builder API would look like. When you say without intermediate structs, you mean without a wrapper type like a hypothetical API like below for example?

#[derive(Clone, Serialize, Deserialize, Default)]
struct SensorConfig {
    rate: f64,
    filters: HashMap<String, String>,
}

let sensors: MandatoryParameter<JsonParameter<SensorConfig>> = node
    .declare_parameter("sensors")
    .default(JsonParameter(SensorConfig::default()))
    .description("Sensor configuration as JSON")
    .mandatory()?;

 // Read the value
let config: SensorConfig = sensors.get();
println!("rate: {}", config.rate);

[balthasarschuess]

@azerupi Sorry my previous example was drafted up with little time.
I think the example was a bit misleading.
It's more about supporting arbitrary value parsers from Arc<str> parameters.

So what I'd like to be able to achieve is:

use serde::{Serialize, Deserialize};

#[derive(StructuredParameters, Debug)]
struct SimpleStructuredParameters {
  _mandatory: rclrs::MandatoryParameter<f64>,
  _optional: rclrs::OptionalParameter<f64>,
  _readonly: rclrs::ReadOnlyParameter<f64>,
}

#[derive(Serialize, Deserialize, Debug)]
struct MyJsonValue {
   values: Map<u32, u32>
}

#[derive(StructuredParameters, Debug)]
struct MyComplexParameters {
   params_0 :  SimpleStructuredParameters,
   params_1: MyJsonValue
}

impl   StructuredParametersMeta<Arc<str>> for serde::Deserialize {

    fn declare_structured_(
        node: &NodeState,
        name: &str,
        default: Option<T>,
        description: impl Into<std::sync::Arc<str>>,
        constraints: impl Into<std::sync::Arc<str>>,
        ignore_override: bool,
        discard_mismatching_prior_value: bool,
        discriminate: Option<Box<dyn FnOnce(crate::AvailableValues<T>) -> Option<T>>>,
        range: Option<<T as crate::ParameterVariant>::Range>,
    ) -> core::result::Result<Self, crate::DeclarationError>{
    // implement declaration for serde serializable values.
    // probably with something like read only -> or in the future also with mutable
    
    unimplemented!()


}

fn() -{
  let params : MyComplexParameters = node.delcare_parameters("param.key").ok();

}

}    
    

The idea is, that the user can extend the leaf declaration with arbitrary parsing logic -> serde yaml, serde json, some weird DSL, without having to:

• parse the value on each access
• or maintaining parallel struct architectures to collapse the MandatoryParameter<Arc<st>> into the target type MyJsonValue