-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat(nebula_ros): all sensors json schema support #155
Conversation
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
2004c58
to
39410ab
Compare
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
4516934
to
df8024b
Compare
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: vividf <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amadeuszsz @vividf Looking really good! Thank you for your work!
I have added a few suggestions here and there, mainly related to description texts and OT128's supported PTP settings. Once these are implemented the PR should be good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(some suggestions were incorrectly referencing the wrong line, deleted them above and added the corrected ones here)
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
PR Type
Related Links
Description
The PR adds json schema to nebula which is current way of handling param files in Autoware environment.
Old proposal aka inheritance. Due to our abstraction, we can't use
additionalProperties=false
. Sub schemas are pushed to the same parent keyros__parameters
, therefore they would block itself in case of not allowing extra parameters. WithadditionalProperties=true
, users can provide extra parameters to.param.yaml
but at the end they not affect runtime.New proposal aka namespaces.
additionalProperties=false
possible, user can't provide extra parameters. In addition, less maintenance - parameters are placed in namespaces and new sensors will not brake architecture. The final.json.schema
points specific parameters from sub schema files, it's possible to overwrite fields (e.g. if we need to userotation_speed
but it's required to define maximum).In general, schema considers only two numeric types:
number
andinteger
. For floating point number and type ofnumber
, the user can provideinteger
value as well. This will throw exception in node constructor. From schema level, we can't force user to use floating point number. We could provide extra method for declaring floating point number instead ofdeclare_parameter
. The idea would be to load parameter with dynamic typing, check the type, cast it todouble
and set it again.Review Procedure
.vscode/settings.json
, e.g.:Old proposal.
New proposal. All the parameters are generated automatically - no need to point abstractions.
Remarks
Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks