Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macro for deriving ROS parameters code #65

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

wentasah
Copy link
Contributor

@wentasah wentasah commented Sep 14, 2023

This is an initial attempt to create a derive macro, which would make dealing with ROS parameters easier. Currently, it works for my simple use case, but it's far from perfect and there is several TODOs and FIXMEs in the code. I'm posting it here if @m-dahl or somebody else wants to comment on it. Please, don't look at individual commits (which are a mess), just the final diff should make sense. I'll clean up the commit history before eventual merge.

How to use this feature is best visible in the parameters_derive.rs example.

Before merging this, I think the following should be clarified/fixed:

  • What should be the name of the derived trait? Currently, it's RosParams.
  • Updating of parameters is currently hooked into the event stream. This should probably be changed to happen directly in set_params_future handler to report errors to the clients if the parameter was not defined or the value has incorrect type.
  • Error handling/reporting should be completed/extended.
  • Currently, parameters are "read only". It's possible to change the value of the parameter, but the value is not propagated outside of the node via rcl_interfaces/srv/GetParameters.
  • Sane conversion of ParameterValue to underlying type. Currently, we use as, which has wrapping semantics, i.e. 255 as i8 == -1.
  • Support String parameters.

@wentasah wentasah force-pushed the param-derive branch 2 times, most recently from 305e6b3 to d89f40a Compare September 15, 2023 14:35
@wentasah wentasah changed the title WiP: Macro for deriving ROS parameters code Macro for deriving ROS parameters code Sep 15, 2023
@wentasah
Copy link
Contributor Author

I think this is now close to be merged (or at least reviewed). As written above, I'll clean up commit history before merging and write useful commit messages.

@wentasah wentasah marked this pull request as ready for review September 15, 2023 14:56
@m-dahl
Copy link
Collaborator

m-dahl commented Sep 18, 2023

Hi,

interesting. I haven't had a chance to look at it yet but I hope to find some time later this week. Would you mind expanding a little on the motivation (e.g. show how it reduces boiler plate or something) similar. Thanks!

With this, declaring and handling node parameters becomes easy. One
just needs to define a structure(s) containing the parameters such as:

    #[derive(RosParams, Default, Debug)]
    struct Params {
        par1: f64,
        par2: i32,
        str: String,
    }

And then instantiate and register it with:

    let params = Arc::new(Mutex::new(Params::default()));
    let (paramater_handler, _) = node.make_derived_parameter_handler(params.clone())?;

This will add three parameters `par1`, `par2` and `str` to the node.
Their type will be `Double`, `Integer` and `String` respectively.
Other Rust types such as `f32` or differently sized integers, e.g.
`u16` are also supported and registered as appropriate ROS parameter
types.

After spawning the handler, e.g.:

    spawner.spawn_local(paramater_handler)?;

changing a parameter with external ROS tools (e.g. `ros2 param set`)
will result in changing the appropriate field in the `Params`
structure. Type conversion is handled automatically. For example,
setting an `i8` field (represented as `Integer` ROS parameter) will
succeed if the value is in range -128 to 127 and fail with appropriate
error message for other values.

The other direction also works: Changing a value in the `Params`
structure will be visible outside of the Node via the `get_parameters`
service.

It is also possible to organize the parameters as several nested
structures with parameters. Then, parameter names of different nesting
levels will be separated by `.`. For example `nested.par3`. See the
full example in `parameters_derive.rs`.
@wentasah
Copy link
Contributor Author

I squashed most of the commits and added the following to the commit message of the first commit. Hopefully, this is what you're looking for.


With this, declaring and handling node parameters becomes easy. One just needs to define a structure(s) containing the parameters such as:

#[derive(RosParams, Default, Debug)]
struct Params {
    par1: f64,
    par2: i32,
    str: String,
}

And then instantiate and register it with:

let params = Arc::new(Mutex::new(Params::default()));
let (paramater_handler, _) = node.make_derived_parameter_handler(params.clone())?;

This will add three parameters par1, par2 and str to the node. Their type will be Double, Integer and String respectively. Other Rust types such as f32 or differently sized integers, e.g. u16 are also supported and registered as appropriate ROS parameter types.

After spawning the handler, e.g.:

spawner.spawn_local(paramater_handler)?;

changing a parameter with external ROS tools (e.g. ros2 param set) will result in changing the appropriate field in the Params structure. Type conversion is handled automatically. For example, setting an i8 field (represented as Integer ROS parameter) will succeed if the value is in range -128 to 127 and fail with appropriate error message for other values.

The other direction also works: Changing a value in the Params structure will be visible outside of the Node via the get_parameters service.

It is also possible to organize the parameters as several nested structures with parameters. Then, parameter names of different nesting levels will be separated by .. For example nested.par3. See the full example in parameters_derive.rs.

@m-dahl
Copy link
Collaborator

m-dahl commented Sep 22, 2023

Thanks that description was what I wanted. It will be convenient to have and I think the code looks good and it really only touches the parameter handling, so I will just merge it to master. RosParams is fine imo.

Hoping to get the windows support in also before I make a new release but don't hesitate to open an issue about that if it takes too long.

Thanks!

@m-dahl m-dahl merged commit 98550c7 into sequenceplanner:master Sep 22, 2023
4 checks passed
@wentasah
Copy link
Contributor Author

That was quick :-) I'm also working on support of describe_parameters service (see d78bac8) and the ability to extract parameter descriptions from doc comments. I think I'll have this ready today or tomorrow, so you may want to wait for this too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants