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

Refactoring unit file #669

Merged
merged 1 commit into from
Nov 23, 2023
Merged

Refactoring unit file #669

merged 1 commit into from
Nov 23, 2023

Conversation

erikbosch
Copy link
Collaborator

@erikbosch erikbosch commented Oct 13, 2023

This is a draft for discussion only, partially inspired by #667, partially inspired by discussions at the AMM.

At the AMM we discussed if signals shall have a unit (like km/h) or just a domain/dimension (like speed). With the latter approach it would be possible to send around values with different units, as long as the unit-information is included the receiver can convert it to whatever the user wants. On the other hand, in some APIs/solutions like VISS and KUKSA gRPC unit information is never explicitly included when you get an observation, you just get a value which is assumed to be in the unit defined for that signal

Based on that I come up with some ideas:

  • Should we possibly specify in the VSS unit file how different units like km/h and m/s relate to each other? Even if it might be easy to understand for a human how to convert between them it might be more difficult for a computer. One way could be to say the we use km/h as default and all other speed units must specify how you they relate to km/h. That would allow for VSS-based solutions to offer solutions where you can get the signal value in whatever VSS unit you want, like get('Vehicle.Speed', 'm/s')
  • For time it is infeasible to specify how to convert between iso8061 and unix timestamp in the unit file itself, so there one may need to specify that custom (hardcoded) conversion methods must be implemented
  • What fields/terms do we actually need for units? Shall we possibly remove "label", it is not used and does not seem to provide much value if we have both name and description.
  • Shall we possibly specify supported datatypes for each unit? If we do so the tooling could give an error or warning if you use another datatype. Like combining iso8061 with a numeric type, or a unix timestamp with string or a small integer.

I would like to know your opinion on if this seems to be reasonable direction for improving/extending the unit file. If we agree on that then we can start looking into details. That CI will fail for this PR is expected - as of today "label" is a required property, so if we agree to make it optional we must do a minor change in vss-tools.

Note: I think there are a lot of things that could be discussed. Like shall we keep the term "domain", or would it be better to call it "dimension" or "quantity" (term used in the NIST document). Similarly would "time" + "duration" be better than "point in time" + "time".

spec/units.yaml Outdated
# Multiply this value by 1000 to get default unit (meters)
multiplier: 1000
# List of allowed datatypes. Numeric is short-cut for all int/uint types
allowed-datatypes: ['numeric']
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is to give a warning or error if someone defines a VSS signal that has the unit mm but where the datatype is string, boolean or some struct type. Array types based on any numeric type should be accepted.

@erikbosch erikbosch added Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting Status:Review Please review/discuss contents labels Oct 16, 2023
@adobekan
Copy link
Collaborator

here is nice ref document.
https://www.govinfo.gov/content/pkg/GOVPUB-C13-f10c2ff9e7af2091314396a2d53213e4/pdf/GOVPUB-C13-f10c2ff9e7af2091314396a2d53213e4.pdf

spec/units.yaml Outdated
psi:
description: Pressure measured in pounds per square inch
domain: pressure
multiplier: 6894.7572931783
Copy link
Collaborator Author

@erikbosch erikbosch Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be 6894.757 if we would follow the recommendation to use 7 significant digits as in the document referenced by Adnan

@Kostadin-Ivanov
Copy link
Contributor

This multiplier conversion sounds reasonable, but in what direction would be the conversion?

Lets say we have a measured speed in km/h, then what multiplier would we set and for that kind of conversion?
Km/h to m/s (meters per second) or km/h to miles/h, or km/r to some other type of unit?

On my opinion, would make more sense if the unit is defined as set of available units in the VSS. Then, whichever sender is, it can declare the unit in which it sends its corresponding measurement / value, including the corresponding value, and the receiver would know both key details of received information: the unit type and its value. From there, the receiver can make any relevant validations, conversions etc.

Otherwise it might be tricky to define what kind of a multiplier would be correct, unless there is additional information like set of multipliers for each supported conversion. As mentioned above - from: km/h, to: meters/second, miles/hour, miles/second etc.

@erikbosch
Copy link
Collaborator Author

So my idea (for now), it is that the domain-definition defines the default unit, like this

domains:
  speed:
    description: Change in distance per time unit
    default-unit: km/h

Then in the unit declarations you declare relationship to the default unit:

units:
  km/h:
    description: Speed measured in kilometers per hours
    domain: speed
    # As km/h is default we do not need to specify conversion, default (1:1) suffice
  m/s:
    description: Speed measured in meters per second
    domain: speed
    # To convert m/s to km/h you need to multiply with 36 and divide by 10 (or just multiply with 3.6)
    multiplier: 36
    denominator: 10
  mph:
    description: Speed measured in miles per hour
    domain: speed
    multiplier: 1.609344

Defining a default unit serves to purposes:

  • It shows the preferred unit for the standard VSS catalog
  • It acts as a reference for conversion, so if you would need to convert from mph (miles per hour) to m/s you could first convert mph to km/h, and then as a second step km/h to m/s. I.e. no need to specify all possible conversions, which easily could be complex for domains with many units, like length. Like in the example above, to convert 5 mph to m/s you would need to calculate 5 * 1.609344 / (36/10) = 2.2 m/s

I think having a file that specifies conversions could be useful both for the scenario when every observation contains both value and unit, and for the scenario where an observation only contains a value (like KUKSA and VISS).

@SebastianSchildt
Copy link
Collaborator

I am not sure, our unit files should define scaling at all. Where would you end? km/h, mph, m/s and suddenly somebody needs knots/hour. This seems to be quite out of scope for COVESA/VSS

Wouldn't it be enough to define a dimension and a default unit? With dimension a system that understand dimensions, can use whatever unit they support, and a Default unit can be used by systems who do not understand quantities, and is considered the VSS default representation (likely in a VSS based data loop, that goes all the way from a zone/vehicle computer to a or several clouds there will be transport and systems in between that need to rely on a specific unit)

@erikbosch
Copy link
Collaborator Author

I am not sure, our unit files should define scaling at all. Where would you end? km/h, mph, m/s and suddenly somebody needs knots/hour. This seems to be quite out of scope for COVESA/VSS

Our tooling can support multiple unit files, so if someone needs knots/hour they can put it in their own unit file if we do not see it as useful for the VSS standard unit file. We have by the way a lot of units today that is not used in the standard catalog, so regardless of whether we define scaling or not we must decide on which units to include in the "standard" units.yaml. So far we have been quite relaxed - as long as we think that a unit may be relevant for a vehicle we have allowed it, even if not used in the standard catalog.

Concerning only using dimension and default unit - we have today many signals that use the domain/dimension "distance" but very different units, like TraveledDistance (km), Length (mm) and Axle.WheelDiameter (inch). Theoretically all could be represented by meter, but that would be a quite big difference and then likely we would need to make sure that all use float as datatype. Also, especially for wheel/tire-related data metric values are uncommon also in Europe. You rarely see someone bragging about their 48 cm wheels, but sometimes about their 19 inch wheels. By that reason I think it makes sense to keep the possibility for signals to use a different unit than the default unit for that dimension.

Defining scale could as I see it serve two purposes:

  • It could be useful for VSS-tooling/implementation that wants to support conversion between units, they do not need to create their own table of conversion factors. Others like VISS and KUKSA do not need to care.
  • It would make the standard VSS unit definitions less ambiguous. As an example, we have today PS (horsepower) as unit, and the name PS may indicate that it is metric horsepower that we are talking about. That would be less ambiguous if we state that conversion factor is 735.49875 (and not 745.69987 like for Mechanical horsepower)

@Kostadin-Ivanov
Copy link
Contributor

I see that using such appendices with unit conversion and other guides could be useful, but I agree with Sebastian that the VSS, being a standard should only define default unit and provide list with, let's say other allowed units.

This way each VSS user / application should be able to easy validate such signals and will make sure to be VSS compliant etc.

With regards to messages and data transport between different VSS compliant edges (applications), it would be sufficient that sender to declare its unit + value, and the receiver would either read it as it is (in case both use same unit), or convert it to its own unit for the corresponding signal (attribute/measurement).

Like I said, having unit conversions rules as a reference guides will be very handy for both user reads and tooling / implementation, but not critical since every user can have their own way to read (convert) the data depending on their use case.

@SebastianSchildt
Copy link
Collaborator

Concerning only using dimension and default unit - we have today many signals that use the domain/dimension "distance" but very different units, like TraveledDistance (km), Length (mm) and Axle.WheelDiameter (inch). Theoretically all could be represented by meter, but that would be a quite big difference and then likely we would need to make sure that all use float as datatype. Also, especially for wheel/tire-related data metric values are uncommon also in Europe. You rarely see someone bragging about their 48 cm wheels, but sometimes about their 19 inch wheels. By that reason I think it makes sense to keep the possibility for signals to use a different unit than the default unit for that dimension

I don't disagree, I just think dimension and _(default)unit are a property of a signal, but there is no need for a unit file to "list all combinations". So today, Vehicle.Speed is defined as "km/h", and a dimension you might only learn indirectly (i.e. our own tooling ignores it), what I would propose, - in case we do want to make this explicit - that every unit we have in the unit file needs a dimension, and that this can exported in vss-tools as well.

We might think about whether later just defining a dimension is enough, and a unit not needed, but I feel today for most practical purposes/tech stacks, you would like to have a (default) unit as well.

@erikbosch
Copy link
Collaborator Author

erikbosch commented Oct 17, 2023

Some points to discuss tonight:

  • Shall we add iso8601 as unit - so far no one objects
  • Shall we remove label - so far no one objects
  • Shall we define default unit and scaling for each domain - different opinions
  • Shall we define supported datatypes - so far no one objects

Meeting notes:

  • Daniel: If you want users to use a specific unit then you shall specify, otherwise better to use just domain
  • Daniel: There are other ontologies that specify conversion, no need to have it here
  • Daniel: Concerning label, would like to have abbreviation and full name
  • Sebastian: Units are important
  • Daniel: We could possibly rename "unit" to "default_unit"
  • Daniel: VSS to define units useful as default for signals, then users can add other as needed

@SebastianSchildt
Copy link
Collaborator

SebastianSchildt commented Oct 17, 2023

Some general information on "dimensions" and how even they might need to be combined:

https://www.me.psu.edu/cimbala/Learning/General/units.htm
https://www.sciencetopia.net/physics/dimensional-formula-of-speed

@Kostadin-Ivanov
Copy link
Contributor

  • Daniel: We could possibly rename "unit" to "default_unit"

  • Daniel: VSS to define units useful as default for signals, then users can add other as needed

If there will be a default unit, should VSS then specify the set of allowed units for corresponding measurement so user can pick one of these and don't mess up with some other - invalid unit?

For example: to mix speed units (km/h, mph etc) with acceleration (m/s*s) ones?

@erikbosch
Copy link
Collaborator Author

  • Daniel: We could possibly rename "unit" to "default_unit"
  • Daniel: VSS to define units useful as default for signals, then users can add other as needed

If there will be a default unit, should VSS then specify the set of allowed units for corresponding measurement so user can pick one of these and don't mess up with some other - invalid unit?

For example: to mix speed units (km/h, mph etc) with acceleration (m/s*s) ones?

That is in one way at least implicitly defined today in https://github.com/COVESA/vehicle_signal_specification/blob/master/spec/units.yaml

Our signal Vehicle.Speed is today defined with the unit km/h. In the unit file one can find that it is defined with domain speed, so a downstream project using VSS (and supporting units) could then verify that Vehicle.Speed is sent/received/set using either km/h or m/s.

  km/h:
    label: kilometer per hour
    description: Speed measured in kilometers per hours
    domain: speed
  m/s:
    label: meters per second
    description: Speed measured in meters per second
    domain: speed

@erikbosch erikbosch force-pushed the erik_units branch 2 times, most recently from 90dbae8 to 1f9446b Compare October 18, 2023 13:55
@erikbosch
Copy link
Collaborator Author

I updated the PR to reflect discussions in the VSS meeting. I also changed approach, documenting a possible new syntax in the documentation. It may be easier to read at https://github.com/boschglobal/vehicle_signal_specification/blob/erik_units/docs-gen/content/rule_set/data_entry/data_unit_types.md

There are no longer any changes in the units.yaml file, instead I just highlight some problems or discussion topics I see. So if reviewing, focus on the markdown file.

@erikbosch
Copy link
Collaborator Author

We have an old idea in #588 - should unit be mandatory? I.e. require unit: none to explicitly state that no unit is used?

@Kostadin-Ivanov
Copy link
Contributor

Kostadin-Ivanov commented Oct 26, 2023

I guess that units are mainly for the leaf nodes (signals, properties, attributes etc.). Branches or standalone trees, might not need to have a unit value.
Unless there is some other scenario / use case?

@erikbosch
Copy link
Collaborator Author

I guess that units are mainly for the leaf nodes (signals, properties, attributes etc.). Branches or standalone trees, might not need to have a unit value. Unless there is some other scenario / use case?

Yes, today we only list unit as optional keyword/member for attribute/sensor/actuator.

https://covesa.github.io/vehicle_signal_specification/rule_set/branches/
https://covesa.github.io/vehicle_signal_specification/rule_set/data_entry/sensor_actuator/
https://covesa.github.io/vehicle_signal_specification/rule_set/data_entry/attributes/

@Kostadin-Ivanov
Copy link
Contributor

Thank you for the quick response, Erik.

Ah, now, after I read your previous comment:

We have an old idea in #588 - should unit be mandatory? I.e. require unit: none to explicitly state that no unit is used?

I got its point. Namely, to have "unit: none" on each branch / node in the VSS which does not have unit.

Now on the real question :)
Since vsstree.py in vss-tools is handling the defaulting of not provided entry for branch unit to none, maybe it would be good to keep it like this, so to keep VSS less wordy.

I mean having a mechanism to handle default values in VSS nodes fields would keep the VSS with just right kind of information, which is required to be presented.
Otherwise, we might end up with many "defaulted" fields under each described branch, signal etc., which will just add more, less significant, content to the specification. Especially to indicate some none fields, which might not be that relevant to the described branch / node / signal.

@erikbosch erikbosch mentioned this pull request Oct 26, 2023
@doganulus
Copy link

Before seeing this pull request, I have opened this discussion: #680

Seems related.

@erikbosch
Copy link
Collaborator Author

Before seeing this pull request, I have opened this discussion: #680

Seems related.

They surely are. Current status in the VSS-project discussions (feel free to join the Tuesday meeting if you like) is that no-one seems to oppose "improving" the unit file. There do however seems to be some different views on whether the unit file shall define conversion factors or not.

And as you note in your issue, units derived from SI-units is not sufficient, like if we intend to give semantic meaning also to string values, like being able to specify iso8061 as unit if the content shall be a string encoded according to ISO 8061.

@nickrbb
Copy link
Contributor

nickrbb commented Oct 31, 2023

I think it's good to be re-looking at this aspect. However, I wonder if we should perhaps be looking to be more flexible when it comes to units in VSS. Right now, we define a "default" unit, which is mostly (although not always) in metric. However, there is a large proportion of the world that doesn't use metric (or whatever default is specified) and so the default unit will not suit them. Instead, why don't we extend VSS signal definitions to include measurement units as part of the signal path? For example, for the vehicle.speed signal, we would extend it to include those units specified in the speed domain e.g. vehicle.speed.mph, vehicle.speed.kmh. This way, a VSS "client" can request the unit it prefers and the VSS "server" can take care of any needed conversion and provide the requested signal in the requested unit. If no unit is provided, then maybe the default unit can be used.

To a certain extent (i.e. by making the unit now explicit in signals), it also takes care of the case where a VSS server implementation provides signals in a different unit to that specified as the default in VSS (which is something not prohibited by VSS) but clients don't know this without prior knowledge of the system they are running on.

@Kostadin-Ivanov
Copy link
Contributor

With current VSS implementation we have:

Vehicle.Speed.datatype
Vehicle.Speed.type
Vehicle.Speed.unit
Vehicle.Speed.description

which is enough sufficient to build an aspect model for the corresponding signal / measurement.

If the implementation, goes with:
Vehicle.Speed.mph
Vehicle.Speed.kmh
and discards the Vehicle.Speed.unit, then what will happen if we need to handle: meter per second (ms) or foot per second (fps)?
Will there be required to declare:
Vehicle.Speed.### for each supported unit type?

On my opinion, having default metric units makes the VSS definition consistent.
The units.yamls file provides the option to the user to load VSS with imperial or other types of units, for corresponding domain. In this case Vehicle.Speed.
Then, VSS user can build their aspect models with clearly defined units so their models' user will have to conform with the declared interface / contract.

@nickrbb
Copy link
Contributor

nickrbb commented Oct 31, 2023

With current VSS implementation we have:

Vehicle.Speed.datatype
Vehicle.Speed.type
Vehicle.Speed.unit
Vehicle.Speed.description

You seem to be mixing paradigms here. We have "vehicle.speed" as the signal name/path. Datatype, Type, Unit, and Description are attributes or metadata for the signal.

which is enough sufficient to build an aspect model for the corresponding signal / measurement.

Agreed, but that's not the issue I was highlighting.

If the implementation, goes with: Vehicle.Speed.mph Vehicle.Speed.kmh and discards the Vehicle.Speed.unit, then what will happen if we need to handle: meter per second (ms) or foot per second (fps)? Will there be required to declare: Vehicle.Speed.### for each supported unit type?

Not entirely sure I follow you, but there would be a need to specify all allowed units for each signal. But is this any different to adding allowed units into the units.yaml file as we do today? Also, we don't have to be explicit and add them to the signal name, we could just allow any unit to be appended to a signal name that is specified in the associated domain as specified in units.yaml.

On my opinion, having default metric units makes the VSS definition consistent.

Metric is not always the default (see Vehicle.Chassis.Axle.Row1.WheelDiameter, Vehicle.Chassis.Axle.Row1.WheelWidth), and as I previously stated, the default unit we specify in VSS isn't always the default used in certain parts of the real world.

The units.yamls file provides the option to the user to load VSS with imperial or other types of units, for corresponding domain. In this case Vehicle.Speed. Then, VSS user can build their aspect models with clearly defined units so their models' user will have to conform with the declared interface / contract.

Right, but you're assuming here that VSS users "know" what unit is provided in a response to a request for a signal. My point was that in applications written to be cross-platform/system-independent, how do they know the unit and/or how can they request a unit that they will understand and can process?

@Kostadin-Ivanov
Copy link
Contributor

Kostadin-Ivanov commented Nov 1, 2023

Hello Nick,
thank you for the detailed response.

With regards to:

You seem to be mixing paradigms here. We have "vehicle.speed" as the signal name/path. Datatype, Type, Unit, and Description are attributes or metadata for the signal.
I understand that datatype, type, unit and description are attributes of the Vehicle.Speed signal. I was just following the suggested paths: Vehicle.Speed.mph and Vehicle.Speed.kmh, which at first reading, I understood that they are supposed to hold a value of the measured speed in mph and kmh respectively.

I agree on the note that there should be a list with allowed units for the speed domain, as defined in the units.yaml file, which then should be included in the generated VSSNode object, when the VSS tree is expanded (parsed) for further use.

In relation to the default units - being either metrics or imperial - I'd say that in order to keep consistent specification, VSS should be defined in either of these default units systems. When user expands them, it will be up to the user to switch to other than the default units system.
Personally, not that I am used to or I am the expert to debate which system is better, but the metric one seems to be the better default candidate to me. These irregular relations like 12 inches for a foot, 3 feet for a yard are not my fitting in my calculator :)
Anyway, these are just my thoughts and as I said I am not the expert to say which system is better.

And, on the last topic about:

Right, but you're assuming here that VSS users "know" what unit is provided in a response to a request for a signal. My point was that in applications written to be cross-platform/system-independent, how do they know the unit and/or how can they request a unit that they will understand and can process?
Sorry that I've missed one of "The Four Agreements", which would perfectly fit in many different areas, including software engineering. Namely: "don't make assumptions" :)
I checked some of the APIs which we generate, using VSS, and Speed was simply provided as a number without any notice of the related unit, which is definitely a miss in the API as you mentioned.

As per above, maybe it would worth to re-discuss design and modeling of such VSS signals / nodes, where unit type is kind of "a must to be included in the API".

Maybe if the Speed or similar signal is provided in the API as:

Speed: {
   value: 50.5,
   unit: "kmh"
}

then users and providers can easy declare the system (metric, imperial) in which they require / provide corresponding signal.
In order to achieve this, I guess that the Vehicle.Speed might not just declared as a signal, but rather as a branch, which has an object of Speed with related unit etc.
This here is just my guessing so I am not 100% if this would be best way to describe a signal like Speed it in VSS as an object with value and unit, which then can result in an API entity like the one above.

One more thing in addition to the allowed units. I think that if we want to keep VSS platform / system independent, we might want to add and some constraints like: min / max values.
In the case of Speed, I am not sure if we can have negative speed, but lets say that some (super) cars could go up to 400 kmh but if we want to cover and other types of vehicles like airplanes or super-speed trains, which can be way faster, or not that fast trucks, which could probably go up to 150 at most, a feasible min/max values could allow to build APIs which can fit not only specific car models but also other types of vehicles.

@erikbosch
Copy link
Collaborator Author

I refactored the deprecation part to make it more similar to the corresponding keyword for signals where we propose to suggest information like

deprecation: V2.1 moved to Vehicle.CurrentLocation

... then it would be quite easy in code to have something like:

if self.unit.deprecation != "":
  print "Warning: Signal {self.name} use deprecated unit {self.unit}: {self.unit.deprecation}"

@doganulus
Copy link

This may be a stylistic choice, but what about two separate quantities.yml and units.yml without top-level quantities and units keys?

This means:

  1. Flatter file structure
  2. Saving one-level access when the file is loaded into a variable.

Unless we want one config.yml to rule them all.

@erikbosch
Copy link
Collaborator Author

This may be a stylistic choice, but what about two separate quantities.yml and units.yml without top-level quantities and units keys?

This means:

1. Flatter file structure

2. Saving one-level access when the file is loaded into a variable.

Unless we want one config.yml to rule them all.

Works fine for me.

@nickrbb
Copy link
Contributor

nickrbb commented Nov 7, 2023

I certainly don't want to hold up this PR, but I think it would be good to continue the discussion on the possible enhancement of specifying unit types using some kind of branch/sensor hybrid solution. Should I start a new Issue?

@erikbosch
Copy link
Collaborator Author

See also #680

@erikbosch
Copy link
Collaborator Author

I certainly don't want to hold up this PR, but I think it would be good to continue the discussion on the possible enhancement of specifying unit types using some kind of branch/sensor hybrid solution. Should I start a new Issue?

Feel free to create a new issue, then we can see if it fits here or somewhere else

erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 9, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 9, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 9, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 9, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
@erikbosch erikbosch changed the title FOR DISCUSSION: Refactoring unit file Refactoring unit file Nov 9, 2023
@erikbosch
Copy link
Collaborator Author

Based on the discussion so far I have tried to come up with something "doable" and I have refactored the PR accordingly. It now consist of:

  • A slightly changed format for the unit file including refactoring of current file
  • A format for domain file including an initial version
  • Introduction of iso8601 as unit
  • Some minor change of domain names.

The changes would require changes in tools, I have a draft at COVESA/vss-tools#307 which can handle both the old and new unit format. It does not add any new functionality like consuming the domain file. So that this PR fails in CI right now is expected, if we "approve" the new syntax we shall first merge the vss-tools PR, then update submodule link.

My suggestion when you start reviewing is to focus on structural parts, actual definition of "domains" is not that important now, that we can improve later. It is more important to agree on that we keep the term "domain" and that it can cover all types of "domains" that we have as of today.

@erikbosch erikbosch marked this pull request as ready for review November 9, 2023 09:03
@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Please review, may be merged next week if no comments
  • Sebastian: I prefer "quantity" rather than "domain"

Copy link
Collaborator

@ppb2020 ppb2020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few suggested textual changes.

docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
docs-gen/content/rule_set/data_entry/data_unit_types.md Outdated Show resolved Hide resolved
spec/domains.yaml Outdated Show resolved Hide resolved
@ppb2020
Copy link
Collaborator

ppb2020 commented Nov 14, 2023

As to 'quantity' vs. 'domain', I'd go with a standard usage of the term. For 'quantity', I found:

https://www.iso.org/obp/ui/#iso:std:iso:80000:-1:ed-1:v1:en
https://en.wikipedia.org/wiki/International_System_of_Quantities

Hence, I think quantity would be the correct term to use here.

erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 15, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 15, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

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

PR updated to use "quantity" instead of "domain". The companion PR COVESA/vss-tools#307 updated as well. There are some CI check failures at the moment, partially caused by the dependency to vss-tools. They will be fixed before merging.

erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Nov 21, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

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

Meeting notes: Merge after fixing conflicts and updating submodule reference

erikbosch added a commit to COVESA/vss-tools that referenced this pull request Nov 22, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
Signed-off-by: Erik Jaegervall <[email protected]>
@erikbosch erikbosch merged commit 9934db9 into COVESA:master Nov 23, 2023
5 checks passed
erikbosch added a commit to COVESA/vss-tools that referenced this pull request Dec 1, 2023
This refactors unit handling so that it keeps supporting old format
but also supports format in
COVESA/vehicle_signal_specification#669.

It only focus on keeping existing functionality.
No functionality for parsing and verifying domains added.

Signed-off-by: Erik Jaegervall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Review Please review/discuss contents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants