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

Branch names are not unique #790

Open
jdacoello opened this issue Nov 26, 2024 · 5 comments
Open

Branch names are not unique #790

jdacoello opened this issue Nov 26, 2024 · 5 comments

Comments

@jdacoello
Copy link
Contributor

During the refactoring of the GraphQL exporter, I noticed there are repeated names for branches in the specification.

Branch name only

For example, if we look at the branch name without the context path (i.e., Window vs. Vehicle.Cabin.Door.Window), we have the following duplicates:

branch_name      count
Brake            3
Identifier       3
Shade            2
Backrest         2
Lumbar           2
SideBolster      2
Headrest         2
Occupant         2
Seating          2
EngineCoolant    2
                                                            name
fqn                                                             
Vehicle.Cabin.Seat.Backrest                             Backrest
Vehicle.Cabin.Seat.Switch.Backrest                      Backrest
Vehicle.Body.Lights.Brake                                  Brake
Vehicle.Chassis.Brake                                      Brake
Vehicle.Chassis.Axle.Wheel.Brake                           Brake
Vehicle.Powertrain.CombustionEngine.EngineCoolant  EngineCoolant
Vehicle.Powertrain.ElectricMotor.EngineCoolant     EngineCoolant
Vehicle.Cabin.Seat.Headrest                             Headrest
Vehicle.Cabin.Seat.Switch.Headrest                      Headrest
Vehicle.Cabin.Seat.Occupant.Identifier                Identifier
Vehicle.Driver.Identifier                             Identifier
Vehicle.Occupant.Identifier                           Identifier
Vehicle.Cabin.Seat.Backrest.Lumbar                        Lumbar
Vehicle.Cabin.Seat.Switch.Backrest.Lumbar                 Lumbar
Vehicle.Occupant                                        Occupant
Vehicle.Cabin.Seat.Occupant                             Occupant
Vehicle.Cabin.Seat.Seating                               Seating
Vehicle.Cabin.Seat.Switch.Seating                        Seating
Vehicle.Cabin.Sunroof.Shade                                Shade
Vehicle.Cabin.Door.Shade                                   Shade
Vehicle.Cabin.Seat.Backrest.SideBolster              SideBolster
Vehicle.Cabin.Seat.Switch.Backrest.SideBolster       SideBolster

Immediate parent name + Branch name

If we consider the combination of the {inmmediate_parent_name}.{branch_name} (e.g., Door.Window instead of the full Vehicle.Cabin.Door.Window), we still have duplicate entries:

combined                count
Backrest.Lumbar         2
Backrest.SideBolster    2
Occupant.Identifier     2
                                                            combined
fqn                                                                 
Vehicle.Cabin.Seat.Backrest.Lumbar                   Backrest.Lumbar
Vehicle.Cabin.Seat.Switch.Backrest.Lumbar            Backrest.Lumbar
Vehicle.Cabin.Seat.Backrest.SideBolster         Backrest.SideBolster
Vehicle.Cabin.Seat.Switch.Backrest.SideBolster  Backrest.SideBolster
Vehicle.Cabin.Seat.Occupant.Identifier           Occupant.Identifier
Vehicle.Occupant.Identifier                      Occupant.Identifier

Proposal

I think it would be a smart move to correct that in the specification. More concretely:

  • If duplicate branches convey the exact same intended meaning, then merge them.
  • if duplicate branches have different intended meaning, then rename them to be more specific and to distinguish them from the others.
  • Add a rule that makes it mandatory for branches to have a unique name.

As you can see, there are not too many duplicates, and the proposed change is feasible.
We can start by fixing the uniqueness in the combination {inmmediate_parent_name}.{branch_name}. Once this is resolved, we can address the uniqueness of the branch name alone.

One big advantage I see is that we can decouple the dependencies of the nodes from the whole fqn path and the concepts become easier to maintain in the long run as: FeatureOfInterest and Property. For example, modeling the window position, will be about modeling the property position of the feature of interest Window.

For example, in GraphQL, types, enums, scalars are unique within the schema. This uniqueness makes it really easy to restructure the hierarchies and define mappings to other languages.

@erikbosch
Copy link
Collaborator

MoM:

  • Daniel presented the issue
  • Sebastion Schleemilch: Could use struct in some cases
  • Erik: Historically we have not requested individual segments to be unique, only full path
  • Please review, continued discussion in next meeting

@erikbosch
Copy link
Collaborator

The Shade example is quite interesting, today we have two similar instances in VSS:

In Cabin.vspec

Sunroof.Shade:
  type: branch
  description: Sun roof shade status. Open = Retracted, Closed = Deployed.
               Start position for Sunroof.Shade is Open/Retracted.

# Include shade specification also used by side and rear window.
#include ../include/MovableItem.vspec Sunroof.Shade

In SingleDoor.vspec


Shade:
  type: branch
  description: Side window shade. Open = Retracted, Closed = Deployed.
               Start position for Shade is Open/Retracted.

# Include the standard shade definition used by sunroof
# and rear shade.

#include ../include/MovableItem.vspec Shade

This shows a common approach in VSS today - we use #include to reuse content, but not for defining a type. The MovableItem.vspec defines just three signals.

With our dot-notation I am not sure if it makes sense to require that each segment must be reused, you may for example be interested in reusing a status sub-branch name here and there, like Vehicle.Gearbox.Status, Vehicle.CoolingSystem.Status and so on. Requiring all segments to be globally unique could mean that quite complex names would be needed like Vehicle.CoolingSystem.CoolingSystemStatus , and could give problems if you get signals from different sources/overlays. It would also require a quite big refactoring of the current VSS tree.

But would it possibly be an idea to separate the member/attribute name, and the type/class name?

One option could be to have reusable branch type/class definitions, in the Shade example one could let both of them refer to the same ShadeData class/type.

A.B.C.Sunroof.Shade:
  type: branch
  datatype: ShadeData

A.B.Shade:
  type: branch
  datatype: ShadeData

ShadeData: # This name must be globally unique (as it is on top level)
  type: class

ShadeData.IsOpen:
  datatype: boolean
  type: actuator

That would allow for example GraphQL to use ShadeData as type name without any need for duplication. At the same time no changes would necessarily be visibly in a traditional JSON/Yaml/csv representation of VSS, i.e. you could still use the same full paths to access signals in e.g. VISSR.

Another option could be to allow specification of a unique name in *.vspec files

Sunroof.Shade:
  type: branch
  unique_branch_name: SunShade

With that we could add unique names in VSS standard catalog where needed, to assure that there are unique names for all branches, at the same time we do not need to rename any nodes.

I think that we at the next meeting should discuss if we can align on if VSS branch segement names must be unique or not, and depending on outcome discuss how to proceed from that.

@jdacoello
Copy link
Contributor Author

I would avoid introducing yet another metadata to the language, such as unique_branch_name . I suggest to address the problem at its root by guaranteeing unique names for the branches.

Dealing directly with the {branch_name} would be too much at once. Probably, we can start by solving the uniqueness for the combination of {inmmediate_parent_name}.{branch_name}.

There are 3 cases in the current specification:

Backrest.Lumbar

Vehicle.Cabin.Seat.Backrest.Lumbar

Seat.INSTANCE.Backrest.Lumbar:
  type: branch
  description: Adjustable lumbar support mechanisms in seats allow the user to change the seat back shape.

Seat.INSTANCE.Backrest.Lumbar.Support:
  datatype: float
  type: actuator
  unit: percent
  min: 0
  max: 100
  description: Lumbar support (in/out position). 0 = Innermost position. 100 = Outermost position.

Seat.INSTANCE.Backrest.Lumbar.Height:
  datatype: uint8
  type: actuator
  min: 0
  unit: mm
  description: Height of lumbar support. Position is relative within available movable range of the lumbar support.
               0 = Lowermost position supported.

Vehicle.Cabin.Seat.Switch.Backrest.Lumbar

Seat.INSTANCE.Switch.Backrest.Lumbar:
  type: branch
  description: Switches for SingleSeat.Backrest.Lumbar.

Seat.INSTANCE.Switch.Backrest.Lumbar.IsMoreSupportEngaged:
  datatype: boolean
  type: actuator
  description: Is switch for more lumbar support engaged (SingleSeat.Backrest.Lumbar.Support).

Seat.INSTANCE.Switch.Backrest.Lumbar.IsLessSupportEngaged:
  datatype: boolean
  type: actuator
  description: Is switch for less lumbar support engaged (SingleSeat.Backrest.Lumbar.Support).

Seat.INSTANCE.Switch.Backrest.Lumbar.IsUpEngaged:
  datatype: boolean
  type: actuator
  description: Lumbar up switch engaged (SingleSeat.Backrest.Lumbar.Support).

Seat.INSTANCE.Switch.Backrest.Lumbar.IsDownEngaged:
  datatype: boolean
  type: actuator
  description: Lumbar down switch engaged (SingleSeat.Backrest.Lumbar.Support).

Backrest.SideBolster

Vehicle.Cabin.Seat.Backrest.SideBolster

Seat.INSTANCE.Backrest.SideBolster:
  type: branch
  description: Backrest side bolster (lumbar side support) settings.

Seat.INSTANCE.Backrest.SideBolster.Support:
  datatype: float
  type: actuator
  unit: percent
  min: 0
  max: 100
  description: Side bolster support. 0 = Minimum support (widest side bolster setting).
               100 = Maximum support.

Vehicle.Cabin.Seat.Switch.Backrest.SideBolster

Seat.INSTANCE.Switch.Backrest.SideBolster:
  type: branch
  description: Switches for SingleSeat.Backrest.SideBolster.

Seat.INSTANCE.Switch.Backrest.SideBolster.IsMoreSupportEngaged:
  datatype: boolean
  type: actuator
  description: Is switch for more side bolster support engaged (SingleSeat.Backrest.SideBolster.Support).

Seat.INSTANCE.Switch.Backrest.SideBolster.IsLessSupportEngaged:
  datatype: boolean
  type: actuator
  description: Is switch for less side bolster support engaged (SingleSeat.Backrest.SideBolster.Support).

Occupant.Identifier

Vehicle.Cabin.Seat.Occupant.Identifier

Vehicle.Cabin.Seat.INSTANCE.Occupant:
  description: Occupant data.
  type: branch

Vehicle.Cabin.Seat.INSTANCE.Occupant.Identifier:
  deprecation: v5.0 - use data from Vehicle.Occupant.*.*.Identifier.
  description: Identifier attributes based on OAuth 2.0.
  type: branch

Vehicle.Cabin.Seat.INSTANCE.Occupant.Identifier.Issuer:
  datatype: string
  deprecation: v5.0 - use data from Vehicle.Occupant.*.*.Identifier.
  description: Unique Issuer for the authentication of the occupant e.g. https://accounts.funcorp.com.
  type: sensor

Vehicle.Cabin.Seat.INSTANCE.Occupant.Identifier.Subject:
  datatype: string
  deprecation: v5.0 - use data from Vehicle.Occupant.*.*.Identifier.
  description: Subject for the authentication of the occupant e.g. UserID 7331677.
  type: sensor

Vehicle.Occupant.Identifier

Vehicle.Occupant.INSTANCE.Identifier:
  description: Identifier attributes based on OAuth 2.0.
  type: branch

Vehicle.Occupant.INSTANCE.Identifier.Issuer:
  datatype: string
  description: Unique Issuer for the authentication of the occupant e.g. https://accounts.funcorp.com.
  type: sensor

Vehicle.Occupant.INSTANCE.Identifier.Subject:
  datatype: string
  description: Subject for the authentication of the occupant e.g. UserID 7331677.
  type: sensor

@erikbosch
Copy link
Collaborator

From a semantic meaning I have no problem renaming

Possibly From:

Seat.INSTANCE.Backrest.Lumbar.Support
Seat.INSTANCE.Switch.Backrest.Lumbar.IsMoreSupportEngaged
Seat.INSTANCE.Switch.Backrest.Lumbar.IsLessSupportEngaged

... to for instance ...

Seat.INSTANCE.Backrest.Lumbar.Support
Seat.INSTANCE.Backrest.Lumbar.IsMoreSupportEngaged
Seat.INSTANCE.Backrest.Lumbar.IsLessSupportEngaged

Or even more drastic if we do not think the logical unit Backrest makes much sense, and that there theoretically in the future may be additional Lumbar somewhere:

Seat.INSTANCE.BackrestLumbar.Support
Seat.INSTANCE.BackrestLumbar.IsMoreSupportEngaged
Seat.INSTANCE.BackrestLumbar.IsLessSupportEngaged

But changing like this would change the names used in our traditional release artifacts used by tools like Kuksa Databroker and VISSR. And that I think is the part we must align - is that considered as acceptable by VSS users?

@erikbosch
Copy link
Collaborator

MoM:

  • Erik: Please continue review
  • Daniel: What VSS shall list is a set of "Feature of Interest", with a defined set of properties. If we make sure that the "feature Of Interest" are unique maintenance is easier. Uniqueness on properties not needed
  • Daniel: Do we want uniqueness for branch-name as required, or branch+parent (gives fewer conflicts)
  • Sebastian Schildt: Start with branch+parent, as some names might be the same for convention.
  • Erik: We need to consider overlays which may come from different organizations, and the same segment name may exist in both

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants