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

Define mandatory field with checker rules? #750

Closed
ClemensLinnhoff opened this issue Dec 15, 2023 · 7 comments · Fixed by #778
Closed

Define mandatory field with checker rules? #750

ClemensLinnhoff opened this issue Dec 15, 2023 · 7 comments · Fixed by #778
Assignees
Labels
Question General questions about the standard, work-flow and code. ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB.

Comments

@ClemensLinnhoff
Copy link
Contributor

Describe the problem

As indicated by section 2.2.1 of the OSI documentation:

For the purpose of providing a complete interface, all existing fields should be set, unless not setting a field carries a specific meaning, as indicated in the accompanying comment.

The general case in the industry is however, that not all fields are set. It is completely up to the users, which fields to set in the interface.

Ask your question

Should some fields be marked as mandatory in any case? For example the version or the timestamp? If so, I propose to write this into the documentation of the respective field in machine readable form, so a checker can parse this as a rule, analog to rules like is_greater_than_or_equal_to.

@ClemensLinnhoff ClemensLinnhoff added the Question General questions about the standard, work-flow and code. label Dec 15, 2023
@PhRosenberger
Copy link
Contributor

It is definitely strange that "optional" fields are checked as "mandatory" by the validator. I like the proposal with machine readable additions to the fields for the validator. What are your thoughts @pmai and @jdsika ?

@jdsika
Copy link
Contributor

jdsika commented Jan 3, 2024

I mean someting like "version number" and "time stamp" are somewhat mandatory?

@jdsika jdsika added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jan 15, 2024
@jdsika
Copy link
Contributor

jdsika commented Jan 15, 2024

We need an initial draft for the logic of what is optional and what is not.
If radar then lidar not mandatory ?
What are the edge cases?
How to document this?

@PhRosenberger
Copy link
Contributor

I will take this for the next CCB and come back with some thoughts on this.

@PhRosenberger
Copy link
Contributor

We discussed this issue today and the CCB would encourage to start with a PR that works on the rules.yaml adding "is_set" and to start with the fields that are already marked or 100% clear to be mandatory.

@ClemensLinnhoff
Copy link
Contributor Author

We will create an example and then also adapt the mentioned section 2.2.1 in the documentation.

works on the rules.yaml adding "is_set" and to start with the fields that are already marked

It is kind of strange, that is_set is not in the rules.yml, as it is already used, e.g. in vehicle_attributes. And the osi-validation rules2yml script already parses the is_set rule somehow. I will look into it.

@ClemensLinnhoff
Copy link
Contributor Author

I created a Draft PR (see above) as a proposal. I mainly set version, timestamp and id fields as mandatory, as well as the main mounting positions.

This branch of osi-validation can handle the new input and only checks for "is_set" if it is actually a rule in the proto files.

@pmai pmai closed this as completed in #778 Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question General questions about the standard, work-flow and code. ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants