Open jdacoello opened 4 months ago
I see this as a little bit related to #642 and the Left/Right vs DriverSide/PassengerSide discussion. If one see the part in square brackets as an instance labels, then one could possibly allow aliases, so that the two lines below may point to the same physical instance (for a LHD vehicle)
Vehicle.Cabin.Door[Row1.DriverSide].Window.Position
Vehicle.Cabin.Door[Row1.Left].Window.Position
If we would go this way we need to discuss how this should affect the released artifacts produced by VSS-tool (as can be seen in https://github.com/COVESA/vehicle_signal_specification/releases/tag/v4.2) Today most expand the tree. But if we change approach we should possibly not expand as today, as for example "Row1" should not be a branch itself.
MoM:
This will add complexity to the generation and parsing of paths. A less intrusive alternative would be to add this information as metadata in the tree, e.g. as a new node type, instance branch. Named ibranch maybe.
This will add complexity to the generation and parsing of paths. A less intrusive alternative would be to add this information as metadata in the tree, e.g. as a new node type, instance branch. Named ibranch maybe.
I think one need to separate different aspects, like:
Taking the VSS-tools JSON format as example, there we do not use expanded names but instead have a tree where currently Row1 is one branch and DriverSide a branch under Row1. With the proposal in this issue we could keep that design as is, possibly by adding a branch-type ibranch
or some other metadata to indicate that it comes from an instance. We could also flatten the instance hierarchy so that Row1.DriverSide
will be a branch/instance directly under Seat.
But in other outputs (like Yaml and CSV) we use expanded names and then just adding a metadata on the branch "line" likely does not give much information to those that look at individual signals.
I do not think the work in vss-tools to change according to any of the proposal from Daniel would be that big, but we need to consider each output, as maybe not all of them supports square brackets as part of names/identifiers. Some may need or want to keep the old dot notation for instances. And it could also be a variation point. Some programmatic APIs based on VSS already today use instances differently compared to other branches, with methods like myVehicle.getDoor(1,DRIVER_SIDE)
Bear in mind that the suggestion of square brackets [ ]
was only one possibility I mentioned because it is nice and clean to read. However it can be any other character we want (e.g., $...$
, (...)
, %...%
, -...-
, _..._
, etc.).
The principal point is to make an explicit distinction between the abstract entity that can be instantiated (e.g., a Window
) and its instances (e.g. the DriverWindow
, the PassengerWindow
, etc.).
I like the ideas written by @erikbosch of either:
ibranch
to indicate that it is an instance of the immediate parent branch, orRow1.DriverSide
as a whole branch/instance
to avoid unnecessary hops. In the end, the instance name is only a label that points to the specific instance. Most of the modelling value resides in the specification of the parent branch that can be instantiated and the associated properties (i.e., attributes, sensor, actuators).I think that the gain/pain it applies to sets of use cases should be considered. A major use case set for VISS are the telematics use cases. For these to have it explicitly expressed in the path I believe is more of a pain than a gain (but expressed in the node metadata would be ok). What would be major use cases where it provides a gain?
MoM: Daniel, create an example. How this should be documented in vspec.
Here are a couple of examples:
Vehicle.Cabin.Door.Row1.DriverSide.Window.Position
Vehicle.Cabin.Door:
type: branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
Vehicle.Cabin.Door.Window:
type: branch
Vehicle.Cabin.Door.Window.Position:
datatype: uint8
type: actuator
min: 0
max: 100
unit: percent
description: Item position. 0 = Start position 100 = End position.
comment: Relationship between Open/Close and Start/End position is item dependent.
Vehicle.Cabin.Door.Row1.PassengerSide.Window.Position:
comment: Relationship between Open/Close and Start/End position is item dependent.
datatype: uint8
description: Item position. 0 = Start position 100 = End position.
max: 100
min: 0
type: actuator
unit: percent
"Door": {
"children": {
"Row1": {
"children": {
"PassengerSide": {
"children": {
"Window": {
"children": {
"Position": {
"comment": "Relationship between Open/Close and Start/End position is item dependent.",
"datatype": "uint8",
"description": "Item position. 0 = Start position 100 = End position.",
"max": 100,
"min": 0,
"type": "actuator",
"unit": "percent"
},
},
"description": "Door window status. Start position for Window is Closed.",
"type": "branch"
}
etc...
Vehicle.Cabin.Door[i]:
type: instance_branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
Vehicle.Cabin.Door[i].Window:
type: branch
Vehicle.Cabin.Door[i].Window.Position:
datatype: uint8
type: actuator
min: 0
max: 100
unit: percent
description: Item position. 0 = Start position 100 = End position.
comment: Relationship between Open/Close and Start/End position is item dependent.
Vehicle.Cabin.Door[Row1.PassengerSide].Window.Position:
comment: Relationship between Open/Close and Start/End position is item dependent.
datatype: uint8
description: Item position. 0 = Start position 100 = End position.
max: 100
min: 0
type: actuator
unit: percent
"Door": {
"Row1.DriverSide": {
"Position": {
"comment": "Relationship between Open/Close and Start/End position is item dependent.",
"datatype": "uint8",
"description": "Item position. 0 = Start position 100 = End position.",
"max": 100,
"min": 0,
"type": "actuator",
"unit": "percent"
},
},
"description": "Door window status. Start position for Window is Closed.",
"type": "branch"
}
etc...
In case of instantiations in multiple branches, we might have something like:
Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Upper.Volume
In this example, both Door
and Speaker
are concepts with multiple instances.
Vehicle.Cabin.Door:
type: branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
Vehicle.Cabin.Door.Speaker:
type: branch
instances: ["Upper","Middle","Lower"]
Vehicle.Cabin.Door.Speaker.Volume:
type: actuator
datatype: int
min: 0
max: 100
description: The volume of a particular speaker that is embedded in the door.
Vehicle.Cabin.Door.Row1.PassengerSide.Speaker.Upper.Volume:
type: actuator
datatype: int
min: 0
max: 100
description: The volume of a particular speaker that is embedded in the door.
Vehicle.Cabin.Door[i]:
type: instance_branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
Vehicle.Cabin.Door[i].Speaker[i]:
type: instance_branch
instances: ["Upper","Middle","Lower"]
Vehicle.Cabin.Door[i].Speaker[i].Volume:
type: actuator
datatype: int
min: 0
max: 100
description: The volume of a particular speaker that is embedded in the door.
Vehicle.Cabin.Door[Row1.PassengerSide].Speaker[Upper].Volume:
type: actuator
datatype: int
min: 0
max: 100
description: The volume of a particular speaker that is embedded in the door.
The symbols or characters to use can be discussed and agreed upon. For example, the characters [I]
after the branch name indicates explicitly in the specification that the branch has instances. An alternative would be to simply introduce a new type instance_branch
. After exporting the specification in the desired format, the principal idea is to have the instance label together with the instantiated branch as one single element to avoid unnecessary hops.
MoM:
[i]
is excludedThe VISS WG does not support the proposed syntax changes to the paths.
It would be helpful for the discussion if some usecases that benefit from the proposal were presented.
It would be helpful for the discussion if some usecases that benefit from the proposal were presented.
One example is overlay handling if you want to change data for a specific instance. Today VSS-tools if it find A.B.C.D
in an overlay it must when it get to to C
first check if C
is a child of B
, then check if C
is an instance of B
. An identifier where it is obvious if C is a branch or an instance could simplify the design.
Similarly if you need to convert a full path to a programmatic API call. like A.B.C.D
-> myVehicle->getA()->getB("C")->getD()
It seems from the examples by @erikbosch that what this change provides is saving one or two rounds in an iteration loop.
An identifier where it is obvious if C is a branch or an instance could simplify the design.
If that identifier is the node type as mentioned above, then the cost to achieve the gain could be acceptable, but not if the solution is to change the path syntax.
My 2 cents, and I think the main aspects here are "output" (i.e. yaml/json) and vspec changes
Vehicle.Cabin.Door[Row1.PassengerSide].Speaker[Upper].Volume:
I think the square brackets are reasonable, I would also assume a branch to be marked "ibranch" (or otherwis tagged as instance branch)
As downstream user I am mainly looking from KUKSA perspective which currently (in core and API) is completely "instance-unanaware", I assume this is a bit similar to VISS(R). However I feel, I could still "parse" the bracket syntax and still just not expose instances to users.
It might be wise in any case to also allow the current format to be generated in exporters as well (I think it is easy to support), and the question would rather be if/when a change becomes the "default"
The given example was
Vehicle.Cabin.Door[i]:
type: instance_branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
Also since VISS brought it up: as I said "KUKSA-side" we are currently unaware of instances. However, it has always bothered me a bit that "instances" is a vspec thing, but that information is "lost" in our exports. The original argument I think was, that "instances" is jsut "syntactical sugar" to make writing vspecs easier, avoiding repetition. However, I see it mainly used where you do have "instances" of "some thing", and I do think this might be helpful for apps/APIs. If I have "Doors" and I do know I may have an arbitrary number of at least the same base door, I think this helps apps. It is not the same as "querying the subtree", because with subtrees, I don't know what to expect (i.e. in unaware Systems like KUKSA querying "Vehicle.Cabin." is fundamentally the same than "Vehicle.Cabin.Doors.", but from an application logic perspective the expectations are different. if I KNOW something is instantiated I expect a set of SIMILAR at some level) objects.
Also, I currently do not see a big effort using/converting the "new" format in instance-unaware systems, however I did not take part in the VISS WG discussion, so I may not see all complications
I thought a little about this issue during the weekend. I see (at least) two parts of the proposal. We can take the hypothetical example from Daniel above as starting point:
Vehicle.Cabin.Door:
type: branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
Vehicle.Cabin.Door.Speaker:
type: branch
instances: ["Upper","Middle","Lower"]
Vehicle.Cabin.Door.Speaker.Volume:
type: actuator
datatype: int
min: 0
max: 100
description: The volume of a particular speaker that is embedded in the door.
Today, if generating for example CSV we get a lot of branches like this:
"Vehicle.Cabin.Door","branch"
"Vehicle.Cabin.Door.Row1","branch"
"Vehicle.Cabin.Door.Row1.DriverSide","branch"
"Vehicle.Cabin.Door.Row1.DriverSide.Speaker","branch"
"Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Upper","branch"
"Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Volume","Actuator"
...and so on until ...
"Vehicle.Cabin.Door.Row2.PassengerSide.Speaker.Lower","branch"
"Vehicle.Cabin.Door.Row2.PassengerSide.Speaker.Lower.Volume","Actuator"
We have some objection on changing the "expanded path identifiers" to something like
Vehicle.Cabin.Door[Row1.PassengerSide].Speaker[Upper].Volume
so maybe we should put that discussion on hold for now and focus on the other part; the model representation and instance_branch
.
Daniel propose to use the term instance_branch
already in *.vspec like
Vehicle.Cabin.Door[i]:
type: instance_branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
But is it really needed there? Is it not more important to use it in the model after expansion to show that a branch comes from instantiation. In original *.vspec you still have the instances
field so the instance_branch
does not provide much value as it anyway can be deduced based on the instances attribute, or? But in expanded model it could serve a purpose. Taking the CSV example from above, one could think of something like:
"Vehicle.Cabin.Door","branch"
# "Vehicle.Cabin.Door.Row1","branch" - Deleted as we flatten the model
"Vehicle.Cabin.Door.Row1.DriverSide","instance_branch" # Indicate that this is not a "real" branch
"Vehicle.Cabin.Door.Row1.DriverSide.Speaker","branch"
"Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Upper","instance_branch"
"Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Volume","Actuator"
...
...and so on until ...
"Vehicle.Cabin.Door.Row2.PassengerSide.Speaker.Lower","instance_branch"
"Vehicle.Cabin.Door.Row2.PassengerSide.Speaker.Lower.Volume","Actuator"
And in JSON a tree structure like Vehicle->Cabin->Door->Row1.DriverSide->Speaker->Upper->Volume
.
This change (keywords and flattened structure) could be implemented separately from the "full path change". So far no-one has objected to the this part of the proposal. So is the way forward to give "ok" to the keyword/flattened-change, but do not change default full-path syntax. If the changed full path syntax would be needed for someone for some exporter (like CSV/Yaml) we could introduce it as an alternative output format, like --expanded-format square
About the keyword:
I agree with you @erikbosch . It makes more sense to have the keyword instance_branch
in the expanded tree. Even the [i]
is not needed. The specification itself (i.e., vspec
file) could remain the same as it is right now. Thus, the idea would be to simply introduce the new keyword instance_branch
that has to be used by the exporters. There are corner cases, however, where people prefer to specify individual instances manually in the vspec
and do not use the instances:
construct. So, I think that the keyword has to be supported in both the vspec
and the exports.
About the resulting path naming convention:
I still think that the form with the explicit instances defined with different characters (e.g., Vehicle.Cabin.Door(Row1.DriverSide).Speaker(Upper).Volume
instead of Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Upper.Volume
) should be the new default way for displaying the path. It looks cleaner as it has enough explicit membership information. I would propose to define this as the new default and to add in the tools an option to export only with dots .
for those who still want the current way.
So is the way forward to give "ok" to the keyword/flattened-change, but do not change default full-path syntax.
To inroduce the new keyword is fine. I question the value of the flattended-change. It removes a few branch nodes to the cost of added complexity. In the example above two branch nodes are removed. The typical array sizes are two or three, and the occasions with "two-dimensional" instantiations are few in the tree, so I cannot see that the overall node reduction balances the complexity cost. Keeping the Row1/Row2 nodes in the example, and changing them to "Instance_branch" type would be ok.
IF the flattening should also be part of this, then the dot (.) between Row1 and DriverSide should be replaced by another character, e. g. dash (-), to become Row1-DriverSide. Dash should then only be allowed in node names when used for this separation. A parser could then treat it like a single node name without any further analysis.
IF the flattening should also be part of this, then the dot (.) between Row1 and DriverSide should be replaced by another character, e. g. dash (-), to become Row1-DriverSide. Dash should then only be allowed in node names when used for this separation. A parser could then treat it like a single node name without any further analysis.
Well, if you are to generate full path from the tree structure and you are to keep the current dot-pattern you would need to translate it back to Row1.DriverSide
if you for instance still want an (instance) branch Vehicle.Cabin.Door.Row1.DriverSide
in for example your generated CSV file.
You previously stated
"Vehicle.Cabin.Door.Row1","branch" - Deleted as we flatten the model
The path Vehicle.Cabin.Door.Row1.DriverSide is then in the tree represented by three branch nodes: Vehicle, Cabin, Door, and one instance_branch node: Row1.DriverSide The convention up until now has been that there is a one-to-one relationship between "path segment names" (the names separated by dots) and nodes in the tree. If the instance_branch node is named Row1.DriverSide then this convention is broken. To keep this convention one solution could be to replace the dot in the instance_branch name.
Not doing that leads to a more complex parsing of path names. It also opens for unclarities, e.g. what does the path Vehicle.Cabin.Door.Row1 point to? If the advantage of this flattening is to save a few branch nodes in the tree it seems not to balance the cost of added complexity.
Daniel propose to use the term
instance_branch
already in *.vspec likeVehicle.Cabin.Door[i]: type: instance_branch instances: - Row[1,2] - ["DriverSide","PassengerSide"]
But is it really needed there? Is it not more important to use it in the model after expansion to show that a branch comes from instantiation. In original *.vspec you still have the
instances
field so theinstance_branch
does not provide much value as it anyway can be deduced based on the instances attribute, or? But in expanded model it could serve a purpose. Taking the CSV example from above, one could think of something like:"Vehicle.Cabin.Door","branch" # "Vehicle.Cabin.Door.Row1","branch" - Deleted as we flatten the model "Vehicle.Cabin.Door.Row1.DriverSide","instance_branch" # Indicate that this is not a "real" branch "Vehicle.Cabin.Door.Row1.DriverSide.Speaker","branch" "Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Upper","instance_branch" "Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Volume","Actuator" ... ...and so on until ... "Vehicle.Cabin.Door.Row2.PassengerSide.Speaker.Lower","instance_branch" "Vehicle.Cabin.Door.Row2.PassengerSide.Speaker.Lower.Volume","Actuator"
And in JSON a tree structure like
Vehicle->Cabin->Door->Row1.DriverSide->Speaker->Upper->Volume
.This change (keywords and flattened structure) could be implemented separately from the "full path change". So far no-one has objected to the this part of the proposal. So is the way forward to give "ok" to the keyword/flattened-change, but do not change default full-path syntax. If the changed full path syntax would be needed for someone for some exporter (like CSV/Yaml) we could introduce it as an alternative output format, like
--expanded-format square
I think the expanded version here is a bit confusing to me, I would have expected Vehicle.Cabin.Door
being the instance_branch
(or instantiated branch here), because this is the "thing" that is instantiated. Otherwise I "walk the tree" find instance_branch, and then need to backtrack one level to find what was instantiated
So this would in my opinion be less confusing when in the (flattened/expanded?) version somehting lie
Vehicle.Cabin.Door[Row1][DriverSide].Speaker[Upper].Volume
or
Vehicle.Cabin.Door[Row1.DriverSide].Speaker[Upper].Volume
would be supported. That seems more clear to me. (And I think there is no "harm" in still supporting the "old" flavor" of
Vehicle.Cabin.Door.Row1.DriverSide..Speaker.Upper.Volume
as well, maybe even without "marking" different branches at all.
Based on the previous comments, let me summarize the proposal as follows:
instance_branch
The expanded tree (not the vspec
itself) will use a keyword instance_branch
to indicate that a particular branch is an individual of a certain class.
# VSPEC
Vehicle.Cabin.Door:
type: branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
...
# EXPANDED TREE (USED IN EXPORTS)
Vehicle.Cabin.Door.Row1.DriverSide:
type: instance_branch
...
Vehicle.Cabin.Door.Row1.PassengerSide:
type: instance_branch
...
Vehicle.Cabin.Door.Row2.DriverSide:
type: instance_branch
...
Vehicle.Cabin.Door.Row1.PassengerSide:
type: instance_branch
...
As you can see, the introduction of the instance_branch
alone does not tell you the actual label of the instance you are dealing with. This can be problematic because the instance labels might consist of multiple concatenated strings with the dotted notation. So, once the tree is expanded, it is not straight forward to know what branch was actually instantiated. Thus, the introduction of special set of characters for instance labels was proposed:
# VSPEC
Vehicle.Cabin.Door:
type: branch
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
...
# EXPANDED TREE WITH SPECIAL CHARACTERS (USED IN EXPORTS)
Vehicle.Cabin.Door(Row1-DriverSide):
type: instance_branch
...
Vehicle.Cabin.Door(Row1-PassengerSide):
type: instance_branch
...
Vehicle.Cabin.Door(Row2-DriverSide):
type: instance_branch
...
Vehicle.Cabin.Door(Row2-PassengerSide):
type: instance_branch
...
Yet another idea would be to do include the label of the instance as a new field in the export. This might, however, imply the need for some rework in the UUID creation as the path would not be unique.
# EXPANDED TREE WITH `instance_label` FIELD (USED IN EXPORTS)
Vehicle.Cabin.Door:
type: instance_branch
instance_label: Row1-DriverSide
...
Vehicle.Cabin.Door:
type: instance_branch
instance_label: Row1-PassengerSide
...
Vehicle.Cabin.Door:
type: instance_branch
instance_label: Row2-DriverSide
...
Vehicle.Cabin.Door:
type: instance_branch
instance_label: Row2-PassengerSide
...
I think the information about instantiation should be represented as metadata in the nodes, and not being visible in the path expressions. I have still not heard any compelling arguments for it to be visible in the path names. A solution that would provide complete information about an instantiation would be to keep the original instances expression in the node also after expanding the tree.
instances:
- Row[1,2]
- ["DriverSide","PassengerSide"]
With this available in the tree there is no need to introduce either the instance_branch node type or modifying the paths.
MoM:
Just created the linked PR for the information keeping of what branches have been created from the instances
feature.
I am against creating a new node type such as instance_branch
because of:
Whether a branch has created from instances or explicitly written by the user should not be reflected in the type of the node since they will not differ.
I think the second part of this issue should be solved by e.g. an option in exporters. As soon as https://github.com/COVESA/vss-tools/pull/399 is merged, they have all the information they need to output some grouping based on instances
I think https://github.com/COVESA/vss-tools/pull/399 solves the entire issue, which with the referenced PR merged can be closed.
The current property naming convention can cause confusion when several branches exist, as it uses the same dotted notation for both branches and instances.
Simple scenario
For example, the property name
Vehicle.Cabin.Door.Row1.DriverSide.Window.Position
does not clearly differentiate between theRow1.DriverSide
being an instance of theDoor
feature of interest, versus a branch in the property hierarchy.To address this issue and improve the clarity of the property naming convention, the following two alternatives are proposed:
Vehicle.Cabin.Door[Row1.DriverSide].Window.Position
. In this approach, a set of distinct symbols, such as square brackets[ ]
, are used to enclose the instance information, making it visually distinct from the branch names. The specific symbols used (e.g.,[ ]
,{ }
,- -
etc.) can be determined based on team preference and overall consistency.Vehicle.Cabin.Door.Instance[Row1.DriverSide].Window.Position
In this approach, a separate identifier, such as the keywordInstance
, is used to explicitly indicate that the following information represents the instance of the feature of interestDoor
. The instance information is then enclosed in a set of distinct symbols, similar to the first solution.The goal of these proposals is to maintain the simplicity of the dot notation for branch names, while providing a clear visual distinction for the instance information. This will improve the overall readability and understanding of the property naming convention used in the codebase.
Complex scenario
What about cases when multiple branches are instantiated to specify the property of a feature of interest? For example, a vehicle can have multiple instances of a
Door
(e.g., Rows and sides), and each one can also have multiple instances ofSpeaker
(e.g., upper, middle, low, etc.). If we follow the current approach, we would end up with something like:Vehicle.Cabin.Door.Row1.DriverSide.Speaker.Upper.Volume
. With the proposed approaches, we could have something nicer and easier to handle like:Vehicle.Cabin.Door[Row1.DriverSide]Speaker[Upper].Volume
Vehicle.Cabin.Door.Instance[Row1.DriverSide].Speaker.Instance[Upper].Volume
Other examples can be, multiple buttons in a particular component, multiple lights in a particular position inside the cabin, multiple USB ports or button embedded in a seat, etc.
Any opinion or additional suggestion?