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.

Sign up for GitHub

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

Jump to bottom

UUIDs instead of id database #102

Closed
danielwilms opened this issue Apr 23, 2019 · 13 comments
Closed

UUIDs instead of id database #102

danielwilms opened this issue Apr 23, 2019 · 13 comments

Comments

@danielwilms
Copy link
Collaborator

As the process of generating the id database is not so transparent and fail proof, I would suggest to replace them with uuids, to have them truly unique and having a fixed length. To do so and not having an arbitrary string length as input the namespace system of uuid v5 could be used:

  • the root node gets a fixed uuid
  • every uuid of a sub-node gets is calculated by the root node uuid (namespace) and the name of the node

That can be done recursively to the leafs. I think it offers some advantages over the old system as mentioned above. Any opinion on this?
@magnusfeuer
Copy link
Contributor

magnusfeuer commented Apr 25, 2019 •
edited
Loading

Just so I get it:

  1. Root UUID would be something like 491eb4c4-6772-11e9-9709-679fd5b85dd5

  2. Engine UUIID would be generated through:
    $ uuid -v5 491eb4c4-6772-11e9-9709-679fd5b85dd5 Engine
    68a75eb6-8be0-56c9-86f0-e567beb37fc1
    [Result is used as input in step 3.]

  3. Engine.RPM would be generated through:
    $ uuid -v5 68a75eb6-8be0-56c9-86f0-e567beb37fc1 Engine.RPM
    df1faf92-7d3f-590f-a206-778de3d6eb81

I really like the idea of a root UUID, which may even be different for each release, thus allowing a program to verify that they have loaded the correct version of a VSS spec.
Taking this a step further, a vendor can create a new root UUID for a vendor-internal release that consists of the GENIVI spec + internal and proprietary extensions.

That said, I have two hesitations about UUIDs:

  1. We already have a way to use a string to uniquely identify a signal, which is the full path name.
  2. UUIDs are a lot heavier to hash on in a program when mapping UUID to an internal struct or object.

That said (again), I would be happy to update the tooling to support this, and retire the original int-based ID file.

My $0.02.

@danielwilms
Copy link
Collaborator Author

We already have a way to use a string to uniquely identify a signal, which is the full path name.

Yes, absolutely agreed. I got sometimes the complaint, that strings with varying length might not be so easy to handle in certain environments and that a fixed size unique identifier would help. I didn't want to replace the pathname as a primary identifier but just add another option.

@magnusfeuer
Copy link
Contributor

Sounds good to me.
Do you want me to open a branch and implement it?

@danielwilms
Copy link
Collaborator Author

@magnusfeuer, sorry I've overseen your comment. That would be fantastic!

@magnusfeuer
Copy link
Contributor

Will do. Give me a week or so.
Do you want me to retire the integer Signal IDs?

@magnusfeuer
Copy link
Contributor

@danielwilms Starting on this now. I will remove Signal IDs since they can be implicitly created on both pub and sub side by:

  1. Sort all signals by their name or UUID.
  2. Use the position of the signal in the sorted list as a signal index.

magnusfeuer pushed a commit that referenced this issue May 11, 2019
Initial run replacing signal ID with signal UUID as per #102
b306085
@magnusfeuer
Copy link
Contributor

@danielwilms
https://github.com/GENIVI/vehicle_signal_specification/tree/magnusfeuer-issue-102

The generated CSV, fidl, and JSON files looks ok at a cursory glance, with UUID values having replaced integer IDs everywhere.

I cannot speak for the cnative format since I haven't read up on that particular subsystem.
@UlfBj - Can you have a look at cnative on this branch to make sure it does what it is supposed to?

I will also add a vspec2hdr script that generates a C header file with all the signal specs.

I may also add a vspec2py that generates python code with the signal spec as wll.

@UlfBj
Copy link
Contributor

UlfBj commented May 11, 2019 via email

@magnusfeuer
Copy link
Contributor

Added vspec2c.py that generates hdr and C file that encodes the entire signal spec and provides access functions.

Demo code that exercises the auto-generated code:
https://github.com/GENIVI/vehicle_signal_specification/blob/magnusfeuer-issue-102/tools/vspec2c_demo/demo.c

At this point I am ready to merge into master
@UlfBj @danielwilms - Are you ok with a Merge request? We will lose ID and get UUID.

@UlfBj
Copy link
Contributor

UlfBj commented May 16, 2019

Are you ok with a Merge request? We will lose ID and get UUID.
It is fine with me.

@magnusfeuer
Copy link
Contributor

Merged to master.
I've tested the generated C/Hdr code extensively through an internal project, and it seems solid.
Please ping me if you have issues and I will fix.

@UlfBj
Copy link
Contributor

UlfBj commented May 20, 2019

I cannot speak for the cnative format since I haven't read up on that particular subsystem.
@UlfBj - Can you have a look at cnative on this branch to make sure it does what it is supposed to?

This is now fixed in the PR#108 "C-native tool updated with UUID support. "

@magnusfeuer
Copy link
Contributor

Thank you, @UlfBj .

Merged.

erikbosch added a commit to erikbosch/vehicle_signal_specification that referenced this issue Oct 26, 2021
@erikbosch
Removing byteBuffer
869c589 
Considered to be a duplicate of uint8[]
Has previously not been supported by tooling so removing
it from documentation shall not cause any backwards compatibility issues.

See COVESA#102 and COVESA#103
@erikbosch erikbosch mentioned this issue Oct 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@magnusfeuer @danielwilms @UlfBj