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

Support ROS parameter descriptions #68

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

wentasah
Copy link
Contributor

With this change, adding doc comments to fields of structures used with #[derive(RosParams)] results in those comments to be used as parameter description.

See r2r/examples/parameters_derive.rs for how to use and test this feature.

The advantage of this implementation is that it should be backward compatible with all current users of r2r crate. However, it has also some disadvantages:

  • There is no API for setting parameter description without using RosParams trait. Older apps should either be ported to use RosParams or they cannot benefit from this.
  • The implementation could be more efficient, if the compatibility is broken. Currently, we have two HashMaps - one for ParameterValues and another for ParameterDescriptors. It might be better to change Node::params to a HashMap whose values contain both ParameterValue and ParameterDescriptior. This way, descriptors would be also available to applications, which might (or might not) be useful.

@m-dahl How stable is r2r API? Would it be acceptable/welcomed to change parameters API as outlined above, say for version 0.8?

…rvices

With this, the parameters are shown in Rqt or Foxglove Studio.
@m-dahl
Copy link
Collaborator

m-dahl commented Sep 27, 2023

This is a neat feature, thanks!

I think it's better to make the API changes now and then bump to 0.8 rather than trying to be backwards compatible. You can make the changes in this PR and when you are done I can merge it and make a new release.

@wentasah
Copy link
Contributor Author

Ok, but I'll get to this not earlier than next week.

The code is already merged, so renaming makes little sense.
With this change, adding doc comments to fields of structures used
with `#[derive(RosParams)]` results in those comments being used as
parameter description.

See r2r/examples/parameters_derive.rs for how to use and test this
feature.

*BREAKING CHANGE*

This commit changes r2r public API. Previously Node::params contained
HashMap<String, ParameterValue>, now it contains HashMap<String, Parameter>.

If you previously used the ParameterValue from this HashMap, now you
can get the same by using the .value field of the Parameter structure.
@wentasah
Copy link
Contributor Author

wentasah commented Oct 2, 2023

This is the version with updated API. I think, it could be reviewed. As described in the commit message, the only change in public API is a different type of Node::params.

I was thinking whether to change API of parameter event stream or not and decided not to change it. If one needs, the parameter description (struct Parameter) can be obtained from Node::params. And if it is not needed in the stream handler, we save time needed for copying of that structure.

@m-dahl
Copy link
Collaborator

m-dahl commented Oct 5, 2023

Looks good. 👍 on the well written descriptions. I will make a release shortly.

@m-dahl m-dahl merged commit 00d7a3d into sequenceplanner:master Oct 5, 2023
4 checks passed
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