COVESA / vehicle_signal_specification

Vehicle Signal Specification - standardized way to describe automotive data
Mozilla Public License 2.0
307 stars 157 forks source link

Improving unit documentation #717

Closed erikbosch closed 4 months ago

erikbosch commented 4 months ago

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

Fixes #690

erikbosch commented 4 months ago

MoM: Please review

nickrbb commented 4 months ago

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 commented 4 months ago

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.

erikbosch commented 4 months ago

Refactored a bit to address the comments of @nickrbb

nickrbb commented 4 months ago

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 commented 4 months ago

MoM: Merge