search

COVESA / vss-tools

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

Static id generator #305

Closed erikbosch closed 9 months ago

erikbosch commented 10 months ago

Only for comparison

nwesem commented 10 months ago

Hi everyone, if you want to run or test this. Please scroll down to see the latest state

erikbosch commented 10 months ago

Some thoughts - it would be good to have some documentation that describes common use cases, like:

Another thought - based on the discussion in the VSS meeting - is if the documentation briefly should describe the use cases it intends to solve. I see some "rough" scenarios being discussed:

The Blackberry use-case seems to focus more on the first alternative

adobekan commented 10 months ago
  • Thin name-based APIs, like KUKSA gRPC or VISS where access is by name and one client may use the old name, and another use the new name when doing set/get/subscribe. Then the server must know that these two names mean the same thing

Where do we store that information so implementation can know how to handle the data?

  • Thin id-based APIs. If the client (or producer) converts name to "static id" before sending to server then server only needs to know static id, name does not matter.

I think that this is quite similar. It is just matter of where do you keep mapping details.

I see two open points.

  1. How do we generate UID, hash or just sequence value? I would prefer short version of 24-bits, max 32-bits and anyhow keep the information in the generated file. Hash value is also fine as long as implementation is checking for collision and validating existing IDs.
  2. Where do we keep the mappings details of version changes? I prefer separated file outside of VSS that is not related to specific implementation.
erikbosch commented 10 months ago

There seems to be a limitation - if you both add a new signal and change name, then the checks seems to bail out. The same actually seems to happen if you just add a new signal.

Original:

Wheelbase:
  datatype: uint16
  type: attribute
  default: 0
  unit: mm
  description: Overall wheelbase, in mm.

#
# Axle definition
#
Axle:
  instances:
    - Row[1,2]
  type: branch
  datatype: uint8
  description: Axle signals

Changed to:

# Adding a new signal, all fields differ

NewSignal:
  datatype: float
  type: sensor
  unit: inch
  description: Neues Signal.

# Changing to "Wheelbaze", all other things equal
Wheelbaze:
  datatype: uint16
  type: attribute
  default: 0
  unit: mm
  description: Overall wheelbase, in mm.

# No change to axle
Axle:
  instances:
    - Row[1,2]
  type: branch
  datatype: uint8
  description: Axle signals

Gives the following output:

WARNING  [Validation] NAME CHANGE: The name of the node in the current vspec was changed from 'Vehicle.Chassis.Wheelbase' to 'Vehicle.Chassis.NewSignal'. This is a non-breaking change if the static UID stays the same. Continuing...
WARNING  DESCRIPTION MISMATCH: The description of Vehicle.Chassis.NewSignal has changed from 
       Validation: 'Overall wheelbase, in mm.' to 
       Current vspec: 'Neues Signal.'
WARNING  [Validation] UNIT MISMATCH: Units of 'Vehicle.Chassis.NewSignal' in unit: 'inch' in the current specification and the validation file 'Vehicle.Chassis.NewSignal' in unit: 'mm' don't match which causes a breaking change, so we need to assign a new id!
INFO     Assigned new ID '0x0004F263' for Vehicle.Chassis.NewSignal
WARNING  DATATYPE MISMATCH also, but new id for 'Vehicle.Chassis.NewSignal' has already been assigned
WARNING  [Validation] NAME CHANGE: The name of the node in the current vspec was changed from 'Vehicle.Chassis.Axle' to 'Vehicle.Chassis.Wheelbaze'. This is a non-breaking change if the static UID stays the same. Continuing...
WARNING  DESCRIPTION MISMATCH: The description of Vehicle.Chassis.Wheelbaze has changed from 
       Validation: 'Axle signals' to 
       Current vspec: 'Overall wheelbase, in mm.'
WARNING  [Validation] UID MISMATCH: IDs don't match. Current tree's node 'Vehicle.Chassis.Axle' has static UID '0x0003FC63' and validation tree's node 'Vehicle.Chassis.Axle' has static UID '0x0003FB63'!
WARNING  What would you like to do?
1) Assign new ID
2) Overwrite ID with validation ID
3) Skip

Also removing a signal seems to make the check bail out. Then you get warnings on all subsequent signals.

WARNING  [Validation] UID MISMATCH: IDs don't match. Current tree's node 'Vehicle.Chassis.Axle' has static UID '0x0003FA63' and validation tree's node 'Vehicle.Chassis.Axle' has static UID '0x0003FB63'!
nwesem commented 10 months ago

Hi @erikbosch thanks a lot for your comments, they were a great help! I am currently working on a hashed pipeline, that we can compare to the current sequential numbering pipeline. I pushed it to a different branch until I'm done with the implementation. I will let you know when it's ready

nwesem commented 10 months ago

Hi @adobekan and @erikbosch after some discussion I finished implementing a first draft of the hashing for the static UIDs. We decided to expect the user to write a new attribute called fka (formerly known as) in case the user wants to keep the same UID (semantic name change or path change). So as an example if we have a node called OldName and we update it to NewName we would extend the vspec like this:

A.B.NewName:
  datatype: uint32
  type: sensor
  unit: mm
  description: A.B.NewName's old name is 'OldName'. And its even older name is 'OlderName'.
  fka: ['A.B.OldName', 'A.B.OlderName']

fka is a list of strings so it can be renamed more than once.

I used the test.vspec in the tests to verify my implementation. It would be very interesting to hear your opinion on the current state. If you like this approach I would go ahead and add full functionality to this. Currently we are checking for breaking changes in name, unit, datatype and type, so if you change one of these you should receive a breaking change warning log.

For non-breaking changes we currently only track the description which also triggers a corresponding log but it doesn't affect the current UID.

Regarding semantic name changes I added a temp_test.vspec in the test directory of the current implementation, so you can follow these commands to trigger a SEMANTIC NAME CHANGE.

First generate the UID file (which will also be the validation later) with temp_test.vpsec which contains the node A.B.OldName.

./vspec2id.py tests/vspec/test_static_uids/test_vspecs/temp_test.vspec ../output_hash_test.vspec

and then validate using the generated ID file and the test.vspec that contains the renamed node A.B.NewName

./vspec2id.py tests/vspec/test_static_uids/test_vspecs/test.vspec ../output_hash_test_v2.vspec --validate-static-uid ../output_hash_test.vspec

You should now see the normal logs from vspec2x plus a few more warnings in particular we expect the SEMANTIC NAME CHANGE warning here plus the ones mentioned below. You can also now change more things in the test.vspec to trigger the other cases.

If you have any other questions just let me know! Thank you

nwesem commented 9 months ago

UPDATE: I added the remaining attributes that cause breaking changes, so you should now see multiple WARNING logs when running the id generator and validator as described above. You would expect the following warnings:

...
INFO     Now validating nodes, static UIDs, types, units and description with file '../output_hash_test.vspec'
WARNING  DEPRECATION MSG CHANGE: Deprecation message for 'A.String' was 'This is the legacy for test deprecation.' in validation but now is 'This is test deprecation, let's say it used to be called Str instead String.'.
WARNING  [Validation] SEMANTIC NAME CHANGE or PATH CHANGE for 'A.B.NewName', it used to be 'A.B.OldName'.
WARNING  [Validation] BREAKING CHANGE: There was a breaking change for 'A.B.IsLeaf' which means its name, unit, datatype, type, enum or min/max has changed.
WARNING  [Validation] BREAKING CHANGE: There was a breaking change for 'A.B.Min' which means its name, unit, datatype, type, enum or min/max has changed.
WARNING  [Validation] BREAKING CHANGE: There was a breaking change for 'A.B.Max' which means its name, unit, datatype, type, enum or min/max has changed.
...
erikbosch commented 9 months ago

Meeting notes:

Adnan informed:

Erik: Please review, wants to make a merge decision next week to be able to handle 4.1 pre-release and release before Christmas.

nwesem commented 9 months ago

Hi everyone, the latest changes have not been merged to this branch. I was waiting on input to merge my changes to here. Before reviewing please give me at least until tomorrow morning to merge it. Thank you

nwesem commented 9 months ago

Merged the most recent changes to this branch now, last thing that we are missing is a NON-BREAKING CHANGE log for adding and removing attributes which I will add to the implementation asap. All comments above have been updated to use the latest state of the command line arguments, please also refer to the update documentation here https://github.com/adobekan/vss-tools/blob/static_id_generator/docs/vspec2id.md

erikbosch commented 9 months ago

@adobekan @nwesem - there are many commits in this PR, so I think a squash before merging makes sense. Do you want to do the squash or should I do it when I merge it?

nwesem commented 9 months ago

@adobekan @nwesem - there are many commits in this PR, so I think a squash before merging makes sense. Do you want to do the squash or should I do it when I merge it?

@erikbosch yes definitely, it makes sense to squash all of this! It would be great if you do that on the merge. Thx

erikbosch commented 9 months ago

Considered good enough for now, some additional tests will be performed as part of 4.1 release preparations

erikbosch commented 9 months ago

Merged and squashed through #313 Closing this one