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

Improving unit documentation #717

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Conversation

erikbosch
Copy link
Collaborator

Based on discussion in #690, clarify that use of of alternative units is not a topic for VSS but rather APIs

Fixes #690

@erikbosch erikbosch marked this pull request as ready for review February 9, 2024 13:02
@erikbosch erikbosch added Scope:Minor A change that is not major and not trivial. Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting Status:Meeting Intended to be discussed at next VSS-project meeting labels Feb 13, 2024
@erikbosch
Copy link
Collaborator Author

MoM: Please review

@nickrbb
Copy link
Contributor

nickrbb commented Feb 13, 2024

Thanks for creating a PR to resolve Issues #690 in the way we concluded earlier. Just for clarification, do we want to indicate that the use of an alternative unit can require additions to the units file? For instance, miles per hour (mph) is a unit that some regions use for Vehicle.speed but this is undefined in units.yaml.

Also, please find below some very minor nits and suggested resolutions:

  1. "VSS does in itself not offer any syntax..." -> "VSS does not offer any syntax..."
  2. "The VSS project has for all units specified also specified a corresponding quantity,lime velocity for km/h." -> "The VSS project has specified for all units a corresponding quantity e.g. 'velocity' for 'km/h'."
  3. "The VSS list of units does however not specify conversion factors, ..." -> "The VSS list of units does not specify conversion factors, ..."

@erikbosch
Copy link
Collaborator Author

Thanks for the comments. I think it could make sense to add a section on what to do if you need or want a new unit like mph. So far our approach for the unit file has been quite flexible - we do not just include units actually used by signals in the VSS standard catalog, but also other units that "possibly" could be make sense in future additions to the standard catalog or in custom extensions. I will try to add something.

Signed-off-by: Erik Jaegervall <[email protected]>
@erikbosch
Copy link
Collaborator Author

Refactored a bit to address the comments of @nickrbb

@nickrbb
Copy link
Contributor

nickrbb commented Feb 14, 2024

Thanks @erikbosch for the updates, all looks good. I might create a new PR to add mph as a unit to the units file then, but as previously discussed and concluded, we will leave it to the API to determine its use in added or modified signals.

@erikbosch erikbosch removed the Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting label Feb 20, 2024
@erikbosch
Copy link
Collaborator Author

MoM: Merge

@erikbosch erikbosch merged commit acc665e into COVESA:master Feb 20, 2024
4 checks passed
@erikbosch erikbosch deleted the erik_unit_doc branch February 20, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope:Minor A change that is not major and not trivial. Status:Meeting Intended to be discussed at next VSS-project meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support of multiple measurement units in VSS signals
2 participants