search

COVESA / vss-tools

Software for working with VSS (https://github.com/COVESA/vehicle_signal_specification)
Mozilla Public License 2.0
55 stars 55 forks source link

Core Rework ⚠️ BREAKING CHANGES #382

Closed sschleemilch closed 1 month ago

sschleemilch commented 2 months ago

Fixes: #380 Fixes: #390 Fixes the issue in discussion of #360 (removed the test in test_overlay_on_instance)

Rewrote the core logic using Pydantic.

[!TIP] Overlay nodes do not need any attributes anymore except the ones you want to overwrite That is also valid for overwriting instances. Example: 

Vehicle:
  type: branch
  description: Vehicle

Vehicle.Door:
  type: branch
  description: Door
  instances:
  - Row[1,2]

Vehicle.Door.Row1:
  description: Very special Door

[!WARNING] This breaks the backwards compatibility to older vss versions and therefore the test has been marked to be skipped. Reason is that we handle unit and quantity files more strict and because of that, vss had to be patched (https://github.com/COVESA/vehicle_signal_specification/pull/758). Old release are decided not to be patched

Points of interest / Questions / Code TODOs

Added

Todo

⚠️ Behavior changes to be discussed -> Change is ok 🆗

Extending instances implicitly

Take this config

Vehicle.Door:
  type: branch
  description: Door
  instances:
  - Row[1,2]

Vehicle.Door.Row3:
  type: branch
  description: Got another door

Vehicle.Door.Id:
  type: branch
  description: Door ID

Although there are only Row1 and Row2 created the user wants to add another one implicitly. However that is some weird magic since it is unlogical to distinguish the behavior from what is displayed in the config: Adding something to all instances. Just because the name of Row3 matches the instance expansions we should not rely on that imo.

So, the outcome of the config as is would be in this implementation:

Vehicle
└── Door
    ├── Row1
    │   ├── Row3
    │   └── Id
    └── Row2
        ├── Row3
        └── Id

That is obviously not what the config intended but in my opinion the config is incorrect. When changing instances to Row[1,3] we get the correct result:

Vehicle
└── Door
    ├── Row1
    │   └── Id
    ├── Row2
    │   └── Id
    └── Row3
        └── Id

Validation improvements

Cross checking of allowed-datatypes

Taking this example config:

kWh:
  definition: Energy consumption measured in kilowatt hours
  unit: kilowatt hours
  quantity: work
  allowed-datatypes: ['numerics']

Is detected as a misconfiguration since numerics does not exist but passes the current vss-tools model: image

Cross checking of quantity being defined

Pretty similar config but using a quantity that is not defined:

kWh:
  definition: Energy consumption measured in kilowatt hours
  unit: kilowatt hours
  quantity: workie
  allowed-datatypes: ['numeric']

Is detected by tool but not by vss-tools: image

Strict units yaml structure

Protecting against wrong structure in units.yaml and quantities.yaml such as using description instead of definition, units instead of unit We actually have already violations in the current vss:

diff --git a/spec/units.yaml b/spec/units.yaml
index 5f32a026..ee0c9ab3 100644
--- a/spec/units.yaml
+++ b/spec/units.yaml
@@ -58,7 +58,7 @@ cm/s^2:
 # Volume

 ml:
-  description: Volume measured in milliliters
+  definition: Volume measured in milliliters
   unit: milliliter
   quantity: volume
   allowed-datatypes: ['numeric']
@@ -138,7 +138,7 @@ g:
   allowed-datatypes: ['numeric']
 kg:
   definition: Mass measured in kilograms
-  label: kilogram
+  unit: kilogram
   quantity: mass
   allowed-datatypes: ['numeric']
 lbs:
@@ -205,7 +205,7 @@ weeks:
   allowed-datatypes: ['numeric']
 months:
   definition: Duration measured in months
-  units: months
+  unit: months
   quantity: duration
   allowed-datatypes: ['numeric']
 years:

Cross checking arraysize to datatype being an array

Example config

Vehicle:
    type: branch
    description: vehicle

Vehicle.Test:
    type: sensor
    datatype: uint8
    arraysize: 3
    description: foo

Is detected by the tool (note that datatype is not an array) but vss-tools has no problem with it: image

Similar: Also checks that arraysize matches the entries of default: image

Checking default against datatype

Example:

Vehicle:
    type: branch
    description: vehicle

Vehicle.Test:
    type: sensor
    datatype: uint8[]
    description: foo
    default: 3

Is detected but is not currently: image

Checking allowed entries matching datatype

Similar to the use case before, Config:

Vehicle.Test:
    type: sensor
    datatype: uint8[]
    arraysize: 3
    description: foo
    allowed: [1,2,'foo']

Note that foo is not an uint8 and should not be accepted. Is detected: image

It also checks ranges e.g. -3 will violate uint8.

Checking min/max set together with allowed

Config:

Vehicle.Test:
    type: sensor
    datatype: uint8
    description: foo
    min: 3
    max: 5
    allowed: [2,3]

Result: image

Checking of default values against allowed

Config:

Vehicle.Test:
    type: sensor
    datatype: uint8
    description: foo
    allowed: [1,2,3]
    default: 4

Result: image

Checking of datatype vs allowed_values in specified unit

Config:

Vehicle.Speed:
    type: sensor
    datatype: string
    unit: kWh

Result: image

erikbosch commented 2 months ago

MoM:

erikbosch commented 2 months ago

MoM:

Questions

We are never exporting the arraysize attribute. On purpose?

S Schildt: Not on purpose

vss2binary -> export_node -> validate what is it?

@UlfBj to be the one to answer.

Should type tree properties be compliant to name-style and should follow the same --strict/--aborts name-style behavior?

sschleemilch commented 2 months ago

MoM:

Questions

We are never exporting the arraysize attribute. On purpose?

S Schildt: Not on purpose

vss2binary -> export_node -> validate what is it?

@UlfBj to be the one to answer.

  • Ulf: Checked by VISS server. Says what access control you have.
  • Niclas: Written/Read as a string
  • S Schildt: Will need to provide it
  • Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

Should type tree properties be compliant to name-style and should follow the same --strict/--aborts name-style behavior?

  • E: Does anyone used struct concept
  • Adnan will check with S Schleemilch what it should be
  • Please review
  • Possible merge decision next week

From my perspective, it is done

UlfBj commented 2 months ago

Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

This information is already linked to: More information can be found in: VISS Access Control.

sschleemilch commented 2 months ago

Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

This information is already linked to: More information can be found in: VISS Access Control.

I am wondering why it is not a specified internal attribute if it is present in the code and explicitly mentioned?

SebastianSchildt commented 1 month ago

Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

This information is already linked to: More information can be found in: VISS Access Control.

I am wondering why it is not a specified internal attribute if it is present in the code and explicitly mentioned?

Because it is "only" used in VISS (and thus the binary exporter, which currently is also only for the VISSR reference implementation), and not considered part of VSS (i.e. expected to be supported by all VSS tooling). That does not mean it is "not good", it is just similar to e.g. DBC mapping keys (currently afaik only used in KUKSA) or some specific deployment keys BMW might use in internal tooling.

I think, what would be a nice addition for the future, in case a specific tooling "requires" or understands special keys (like the binary exporter), if it could programmatically via an API whitelist those, instead of forcing the user to whitelist them on the command line.

SebastianSchildt commented 1 month ago

Meeting 20th: Looking good,

What about the breaking changes?

MoM: Merge