-
Notifications
You must be signed in to change notification settings - Fork 171
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
Conversation
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'] |
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.
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.
spec/units.yaml
Outdated
psi: | ||
description: Pressure measured in pounds per square inch | ||
domain: pressure | ||
multiplier: 6894.7572931783 |
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.
Would be 6894.757 if we would follow the recommendation to use 7 significant digits as in the document referenced by Adnan
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? 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. |
So my idea (for now), it is that the domain-definition defines the default unit, like this
Then in the unit declarations you declare relationship to the default unit:
Defining a default unit serves to purposes:
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). |
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) |
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" 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:
|
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. |
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. |
Some points to discuss tonight:
Meeting notes:
|
Some general information on "dimensions" and how even they might need to be combined: https://www.me.psu.edu/cimbala/Learning/General/units.htm |
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
|
90dbae8
to
1f9446b
Compare
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 |
We have an old idea in #588 - should unit be mandatory? I.e. require |
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. |
Yes, today we only list https://covesa.github.io/vehicle_signal_specification/rule_set/branches/ |
Thank you for the quick response, Erik. Ah, now, after I read your previous comment:
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 :) 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. |
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 |
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. |
With current VSS implementation we have: Vehicle.Speed.datatype which is enough sufficient to build an aspect model for the corresponding signal / measurement. If the implementation, goes with: On my opinion, having default metric units makes the VSS definition consistent. |
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.
Agreed, but that's not the issue I was highlighting.
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.
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.
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? |
Hello Nick, With regards to:
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. And, on the last topic about:
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:
then users and providers can easy declare the system (metric, imperial) in which they require / provide corresponding signal. 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. |
I refactored the deprecation part to make it more similar to the corresponding keyword for signals where we propose to suggest information like
... then it would be quite easy in code to have something like:
|
This may be a stylistic choice, but what about two separate This means:
Unless we want one |
Works fine for me. |
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? |
See also #680 |
Feel free to create a new issue, then we can see if it fits here or somewhere else |
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]>
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]>
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]>
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]>
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:
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. |
Meeting notes:
|
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.
Looks good. A few suggested textual changes.
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 Hence, I think quantity would be the correct term to use here. |
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]>
ffb9029
to
628ca18
Compare
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]>
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. |
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]>
Meeting notes: Merge after fixing conflicts and updating submodule reference |
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]>
628ca18
to
e9fbab9
Compare
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]>
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:
km/h
andm/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, likeget('Vehicle.Speed', 'm/s')
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".