feat(application-schema): support parameter files for components and controllers#463
feat(application-schema): support parameter files for components and controllers#463
Conversation
domire8
left a comment
There was a problem hiding this comment.
Looks good overall, you can add a changelog entry too
Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>
eeberhard
left a comment
There was a problem hiding this comment.
I would prefer to see a common parameters_file subschema that can be used in both places. It would reduce the risk of a typo or maintenence error in the regex between the components and controllers, and in any case one common subschema will make the generated type easier to work with in the implementation.
Add a new file common/parameters_file.schema.json for this subschema
In components and hardware.schema.json then "$ref": "common/parameters_file.schema.json"
In application.schema.json, under $defs:
"parameters_file": {
"$ref": "common/parameters_file.schema.json"
},The last step helps to ensure that the bundled schema uses clean a top-level ref - otherwise it will define the first occasion inline (for example, under components) and then the ref from hardware would actually point to components (instead of them both referencing the "same" property)
| }, | ||
| "parameters_file": { | ||
| "title": "Component Parameters File", | ||
| "description": "An address pointing to a YAML or JSON file containing component parameters", |
There was a problem hiding this comment.
I think somewhere we will need to define 1: the schema of the parameters file, 2: the priority order of parameter resolution, and 3: the meanings of the various path specifiers.
For 1, is there a URL to a ROS standard (or at least an example) we can link to?
For 2, the order is: component default parameters from implementation - overridden by -> parameters file - overridden by -> individual parameters from the parameters property
For 3, file:// is an absolute path in the container, data:// is a path relative to the default mounted data folder in the container, and package:// is a path relative to a ROS 2 package installed in the container.
But mostly, can we include all of this information in the description of the property in a concise way? It seems too much. So I think it would be good to clearly define these points in a public issue or docs page and link to that instead.
For now I would suggest the following:
| "description": "An address pointing to a YAML or JSON file containing component parameters", | |
| "description": "A path to a YAML or JSON parameters file that is used set parameter values on load. This is evaluated before any values in the `parameters` property are applied.", |
There was a problem hiding this comment.
Applied the suggestion, not resolving the conversation to keep the rest of your comment visible.
There was a problem hiding this comment.
I think somewhere we will need to define 1: the schema of the parameters file, 2: the priority order of parameter resolution, and 3: the meanings of the various path specifiers.
For 1, is there a URL to a ROS standard (or at least an example) we can link to?
For 2, the order is: component default parameters from implementation - overridden by -> parameters file - overridden by -> individual parameters from the
parameterspropertyFor 3,
file://is an absolute path in the container,data://is a path relative to the default mounted data folder in the container, andpackage://is a path relative to a ROS 2 package installed in the container.But mostly, can we include all of this information in the description of the property in a concise way? It seems too much. So I think it would be good to clearly define these points in a public issue or docs page and link to that instead.
Should I make an issue out of this before closing?
The mistakes I made above a case and point. I was thinking about it too, thanks for the suggestion. |
eeberhard
left a comment
There was a problem hiding this comment.
Good to go, it won't be released until the next version change but we can already use this for testing!
Description
This PR solves the issue by adding the relative argument in components and controllers.
Review guidelines
Estimated Time of Review: 5 minutes
Checklist before merging: