COVESA / vehicle_signal_specification

Vehicle Signal Specification - standardized way to describe automotive data
Mozilla Public License 2.0
319 stars 164 forks source link

[VSS2] Position-related branches proposal #81

Closed klotzbenjamin closed 3 years ago

klotzbenjamin commented 5 years ago

The current VSS contains about 1100 signals in total, but only about 300 if we remove the position-related redundancy. The data model would also benefit from an udate of how positions are described, to make a clearer difference between what branches physically represent (composents, domains) and there position branch (Front, Left, Row1...).

Currently we have for instance Signal.Chassis.Axle.Row1.Wheel.Left.Tire.Pressure to describe the tire pressure on the front left tire. The metadata of this signal, and of its hosting branch is the same as in Signal.Chassis.Axle.Row1.Wheel.Right.Tire.Pressure, Signal.Chassis.Axle.Row2.Wheel.Left.Tire.Pressure, Signal.Chassis.Axle.Row2.Wheel.Right.Tire.Pressure

In addition, there is no fix rule about position modeling, and in the same example, there is the branch Wheel between 2 position branches.

Proposal We could dedicate a branch type to positions, and add a dedicated symbol (for instance "-") in a similar way as URI parameters. So for instace the previous example could become: Signal.Chassis.Axle-Row1.Wheel-Left.Tire.Pressure Where the branches Chassis and Wheel are parametrized with their position. Or even Signal.Chassis.Axle.Wheel.Tire.Pressure-Row1.Left Where the signal is parametrized with the position.

This could be a way of not only remove redundant concepts, but also allow access on multiple positions at once with a wildcard, as proposed in https://github.com/GENIVI/vehicle_signal_specification/issues/48

danielwilms commented 5 years ago

I would see it more as a tooling issue, if I understand it right. How I read it from your description, the main concern is redundancy.

I think redundancy in the representation of the final tree it's not a big issue. IMO we should make sure (e.g. by the compilers), that there is no redundancy in the spec file. Always if something is used in more than one place it has to be done with an include.

In addition, there is no fix rule about position modeling, and in the same example, there is the branch Wheel between 2 position branches.

I see that this is an issue. But it's again more of a tooling/rule question.

So for instace the previous example could become: Signal.Chassis.Axle-Row1.Wheel-Left.Tire.Pressure

The dash notation is in my opinion not intuitive and I don't understand the issue it tries to solve. The wild-card as in #48 is in the current tree structure possible as well.

danielwilms commented 5 years ago

what about this one @klotzbenjamin, @UlfBj? Any further opinions? Do you think it's needed? I'd close it otherwise.

klotzbenjamin commented 5 years ago

I think redundancy in the representation of the final tree it's not a big issue. IMO we should make sure (e.g. by the compilers), that there is no redundancy in the spec file. Always if something is used in more than one place it has to be done with an include.

I agree. That should be a first step for being a bit more consistent.

The dash notation is in my opinion not intuitive and I don't understand the issue it tries to solve.

It is not so much an issue in regard to the tree structure of VSS. We can always use include to import another file and that would reduce the size of the spec, while containing the same information. But this is not the point here.

The issue is mostly about clarifying what a branch is and how to use them. We have component or domains from a car as branches, as well as position. From a modeling perspective, this results first in an unclear definition of branch, and secondly in the specification of several position branches. E.g. Left in Wheel.Left is not the same as in Seats.Left.

I would suggest clarifying first the type of branch. It could be with a sub-type like "position branch". And secondly I would propose aligning all position branches in term of definition.

UlfBj commented 5 years ago

I am ok with the arguments by Daniel and Benjamin on this one.

I just want to try one more option here, and that would be to create two new key-value pairs to be included in the leave nodes in question: ‘attribute-row’ = x (x = [1..]) ‘attribute-handedness = y (y = [left, middle, right])

Then there would be no position information in the path, but in the respective nodes instead. Maybe a stupid idea, I do not know.

BR Ulf

magnusfeuer commented 5 years ago

Late to the party.

I am fairly agnostic on the structural solution we pick, and would be happy to support whatever we agree on.

However, Volvo Cars, BMW, and JLR, and possibly others, now have an interest in VSS, meaning that it is potentially being used in production projects. (I casn obviously not talk about how we do or do not use VSS in JLR's internal development.)

With that in mind, we need to be careful of drastic changes that would impact said projects. At the very least we need to look at versioning releases with the potential pain of back-porting and maintaining new entries to older releases.

I really don't know the best process for handling this, and would appreciate any input from other stakeholders on this.

Any ideas? @UlfBj / @klotzbenjamin

Regards,

/Magnus F.

klotzbenjamin commented 5 years ago

Then there would be no position information in the path, but in the respective nodes instead.

This raises another question: do we use VSS as a set of signal classes (with parameters in the node itself), or as the signals static metadata.

So far, VSS is meant to be used to directly map VSS signals to data sources, and for example the Row1.Right.Wheel has a one-to-one mapping with a signal from a car. There is no instance of this signal, simply a mapping.

If we want to make classes of signals instead, I do not see why we would bother with a tree structure at all, and graphs would be more consistent. This is basically the model that result when you reduce the redundancy of VSS concepts, like I did with VSSo.

With that in mind, we need to be careful of drastic changes that would impact said projects. At the very least we need to look at versioning releases with the potential pain of back-porting and maintaining new entries to older releases.

For this matter, we agree to create a "v1" branch so that applications and developers working on the current VSS can at least keep it, and we will try to ensure compatibilities.

danielwilms commented 5 years ago

With that in mind, we need to be careful of drastic changes that would impact said projects. At the very least we need to look at versioning releases with the potential pain of back-porting and maintaining new entries to older releases.

For this matter, we agree to create a "v1" branch so that applications and developers working on the current VSS can at least keep it, and we will try to ensure compatibilities.

Absolutely agreeing to this. Even though the structure is changing in a potential version 2, the nodes and sub branches should be easily backwards compatible, so that both can profit from newly mapped signals.

danielwilms commented 5 years ago

I had quite some discussions in the last weeks with various people, so that I think I understand better the need of change in the positioning. For the records:

That being said, I took one proposal and developed it a bit further. Currently, we have: Signal.Chassis.Axle.Row2.Wheel.Right.Tire.Pressure

I like the proposal https://github.com/GENIVI/vehicle_signal_specification/issues/81#issue-384773396 most, when putting the positioning info at the end, but with dot-notation: Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2.Right

Going forward from there here's a way on how to introduce this in the spec file with the goal of:

Let's look at the example above. We have two branches, which contain positioning information, namely Axis and Wheel. I think it would be beneficial to keep the information in the spec file close to the branch definition. Therefore I would like to add a new parameter to a branch:

I see two types of positioning information:

An adoption of the tooling would then help to create the definitions in every representation, like csv, yaml, etc. to achieve the combination of all branch positions attached to the leaves: Signal.Chassis.Axle.Wheel.Tire.Pressure Signal.Chassis.Axle.Wheel.Tire.Pressure.Row1 Signal.Chassis.Axle.Wheel.Tire.Pressure.Row1.LEFT Signal.Chassis.Axle.Wheel.Tire.Pressure.Row1.RIGHT Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2 Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2.LEFT Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2.RIGHT

Final question: What do you guys think? @UlfBj @klotzbenjamin @magnusfeuer @rtroncy

UlfBj commented 5 years ago

So the tool would then generate a tree like the following? image

tpanagos commented 5 years ago

JSON style accessor might look like:

where array accessors are embedded at the level to which they apply.

This has the advantage of supporting multiple levels of arrays (or maps) in an unambiguous way.

RESTFUL VERSION: Signal/Chassis/Axle/1/Wheel/RIGHT/Tire/Pressure

danielwilms commented 5 years ago

JSON style accessor might look like:

  • Signal.Chassis.Axle[1].Wheel[RIGHT].Tire.Pressure

where array accessors are embedded at the level to which they apply. This has the advantage of supporting multiple levels of arrays (or maps) in an unambiguous way.

RESTFUL VERSION: Signal/Chassis/Axle/1/Wheel/RIGHT/Tire/Pressure

@tpanagos: The issue isn't so obvious, if you request the pressure for one specific tire. But how would you like to address the pressure of all tires? We had the proposal like: 'RESTFUL VERSION: Signal/Chassis/Axle//Wheel//Tire/Pressure'

But I think this is not so nice. As well 4 requests in this case is not really practical, especially, if you want to get notified, when one value is changing. To get the entire subtree of axle would be as well overkill.

For the entire discussion please check out the minutes.

danielwilms commented 5 years ago

Hi @UlfBj,

sorry for the delay. I could imagine a tree being generated like this: screen shot 2019-02-12 at 14 54 41

For the description please check https://github.com/GENIVI/vehicle_signal_specification/issues/81#issuecomment-460622777

UlfBj commented 5 years ago

The new tree certainly has an advantage in terms of redundancy elimination. There are however some issues that needs to be sorted out, such as:

danielwilms commented 5 years ago
  • The Datatype property, and other properties, are no longer always found in the leave nodes. It will add complexity to an server impl, but my guess is it is manageable.

Leave nodes are either the datatype as before (single node) or a list/structure {"Row1":{"LEFT":,...}}

  • What node type will be assigned to the new (purple) nodes? Do the leave nodes contain any metadata?

Those are basically instances of pressure addressed by the position structure

  • How does the path expression look like that returns the pressure of all LEFT wheels?

This is not possible right now, but wasn't before either. Same question I got of a colleague ;)

Do not get me wrong, I think this will improve the tree structure, but all details must be clear.

Of course, that's why we discuss it here. Happy to get comments so quickly!

UlfBj commented 5 years ago

Just to exhaust all possibilities I want to propose one more solution. The Pressure node is the leaf node, in this case four of them, under the Tire node. Each Pressure node must, due to the pbranch nodes in its path, contain a property called "positionInstance". This property must as its value have one enum instance from the Axle pbranch, and one from the Wheel pbranch, for example "positionInstance : Row1, LEFT". The path expression "Vehicle.Chassis.Axle.Wheel.Tire.Pressure" would address all four Pressure nodes. To select any specific a query would have to be addded to the path, e.g. "Vehicle.Chassis.Axle.Wheel.Tire.Pressure?positionInstance=LEFT" would return all left wheel pressures, etc. This would not break the rule with leaf nodes containing Datatype and other metadata. It would also be more flexible in terms of search options (I believe).

UlfBj commented 5 years ago

This is not possible right now, but wasn't before either. I think it was - "Vehicle.Chassis.Axle.*.Wheel.LEFT.Tire.Pressure".

PeterWinzell commented 5 years ago

How would this translate for subscribe ?

Vehicle.Chassis.Axle.Wheel.Tire.Pressure?positionInstance=LEFT" would return all left wheel pressures, etc.

...

{"action": "subscribe", "path": "Vehicle.Chassis.Axle.Wheel.Tire.Pressure", "positionInstance":"{*,LEFT}, "requestId": ' id '}'

Or how would one express this for subscribe ? It seems that this does complicate both the server implementation and the client

UlfBj commented 5 years ago

What makes it different for subscribe compared to other actions? I cannot see how it complicates it more for the server, or client, than using wildcards for the same thing in the current VSS structure? Possibly the syntactic parsing of the query, but not otherwise. Strict rules needs to be defined for query syntax.

PeterWinzell commented 5 years ago

I was referring to the syntactic parsing - it is not as clean as wildcards in my view. One other question is how the positionInstance is defined ? LEFT and RIGHT would not suffice for example for seatbelts , but I guess we could cover this with numbers then ...or how would we make a generic positionInstance ?

UlfBj commented 5 years ago

As I have understood Daniels proposal on introducing pbranch nodes, the available enum position values are declared there. So it is just to declare what you need for the specific case, which would then set the scope for what can be found as positionInstance values. I agree that wildcards syntax is "cleaner", i.e. more simple and thus easier to parse/use. But if we want to eliminate redundancy in vspec and tree, I guess we have to leave it.

PeterWinzell commented 5 years ago

Ok.

danielwilms commented 5 years ago

The path expression "Vehicle.Chassis.Axle.Wheel.Tire.Pressure" would address all four Pressure nodes. To select any specific a query would have to be addded to the path, e.g. "Vehicle.Chassis.Axle.Wheel.Tire.Pressure?positionInstance=LEFT" would return all left wheel pressures, etc. This would not break the rule with leaf nodes containing Datatype and other metadata. It would also be more flexible in terms of search options (I believe).

I thought more about it. I think this can be an implementation detail of the usage of VSS in whatever protocol. GraphQL might have different ways of accessing it, then REST. IMHO, it might be important, that an ID is created for each node, as shown above:

Signal.Chassis.Axle.Wheel.Tire.Pressure Signal.Chassis.Axle.Wheel.Tire.Pressure.Row1 Signal.Chassis.Axle.Wheel.Tire.Pressure.Row1.LEFT Signal.Chassis.Axle.Wheel.Tire.Pressure.Row1.RIGHT Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2 Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2.LEFT Signal.Chassis.Axle.Wheel.Tire.Pressure.Row2.RIGHT

How it then get's accessed through the protocol is another question then.

UlfBj commented 5 years ago

I think we are getting there now:). My thinking has been leaning towards tree implementability, while you have been more on a logical level (maybe).
I agree that it is desirable to be possible to address each node separately, but I think we can get there by introducing the notion of "virtual position nodes", which then are represented in the tree realisation by the data in the "positionInstance" property in the (physical) leaf node (the property name is just a suggestion). It is then up to the server to map the ID/path to the correct node by reading the positionInstance value in leave nodes. Nothing would stop us from allowing LEFT wheel pressures to then be addressed by "Signal.Chassis.Axle.Wheel.Tire.Pressure.LEFT" if we would like to. The last two path segment are virtual, and their order in the path expression is loosely coupled to the (physical) tree structure (that a tree parser implementation has to deal with). It would also be a possibility to search for the LEFT wheel pressures using a query, if the above suggestion sounds too "rule breaking". I think it is important to have a solution that do not have built in search limitations, so one of the two above should at least be supported, in my view. So to summarize my current view: In the vspec files positioning is expressed through pbranches, as Daniel has suggested earlier in this thread. The Tools that transforms vspec to other formats then from the pbranch data creates positionInstance properties a number of leaf node instantiations given by the pbranch data. The server utilizing this tree will then use this positionInstance data when decoding the paths provided to it from clients as discussed above. I think we now have all in place - redundancy elimination in both vspec and tree realisations, ability to address separate position nodes, including search without asymmetrical limitations. I am ready to go with this:).

UlfBj commented 5 years ago

This is how the implementation of the tree would look like, as I see it from the comment above. The logical representation would be as shown in Daniels figure above, the "Proposal" tree. image

PeterWinzell commented 5 years ago

Ok, in the figure I see two trees. How are Chassis and Axle connected ? If I understand this correctly we are able to express position on vehicle "objects" which would match the actual "instance " of a particular vehicle model. Thus, eliminating redundancy. One concrete example would be a 7 seater SUV compared to a 4 seater sedan. I think it would be good to examplify this new approach...

danielwilms commented 5 years ago

This is how the implementation of the tree would look like, as I see it from the comment above. The logical representation would be as shown in Daniels figure above, the "Proposal" tree.

This looks good. One possible improvement. How about Tire - Pressure < - instanceOf - "Row1, LEFT"... This would have the advantage, to draw a line between the specification and instantiation. I will draw it tomorrow, when I'm back in the office.

One concrete example would be a 7 seater SUV compared to a 4 seater sedan. I think it would be good to examplify this new approach...

@PeterWinzell Sounds like a good use-case for checking the concept. Will write it down.

danielwilms commented 5 years ago

The following is the result of the seat tree, as it is after the change. The specification is then, as follows:

- Seat:
  type: pbranch
  description: All seats.
  position:
    - Row[1,2]
    - Pos[1,5]
#include SingleSeat.vspec Seat

#
# Seat attributes.
#
- DriverPosition:
  datatype: uint8
  type: attribute
  value: 0
  description: The position of the driver seat in row 1. (1-5) 

One issue which has to be tackled is, if you have row-specific elements, which you'd have to attach to the cabin then, with proper naming and positioning. The examples here, just were about the position count, which is already covered by the positioning information.

This example results then into the following tree structure, you find in the attached files:

The tools are almost there. Will push them ASAP, so that you guys can play around with them. Any questions @UlfBj, @PeterWinzell

UlfBj commented 5 years ago

As this is still an unresolved issue, I would like to add one more alternative solution to the positioning problem. The idea builds on introducing a new key-value property with the key name pathextension which defines the position similar to position in Daniels proposal in the thread above.However, instead of finding it in a pbranch node, it is found in the actual leaf node. If we take an example from the VSS tree, e.g. the eight door lock data points currently addressed by the eight paths: Vehicle.Cabin.Door.Row1.Left.IsLocked .. Vehicle.Cabin.Door.Row4.Right.IsLocked With this new proposal the YAML vspec definition of the Door and IsLocked nodes would look like below (IsOpen is also shown in the YAML example).

# Door signals and attributes
##
- Door:
  type: branch
  description: All doors, including windows and switches

#

Definition of a single door

#
- IsOpen:
  datatype: boolean
  type: actuator
      pathextension: Row[1,4], [Left, Right]
  description: Is door open or closed

- IsLocked:
  datatype: boolean
  type: actuator
      pathextension: Row[1,4], [Left, Right]
  description: Is door locked or unlocked. True = Locked. False = Unlocked.

The Door node is not changed, and the only change in the IsLocked (and IsOpen) node is the new “pathextension” property, which defines that this node can be addressed by the eight paths below. Vehicle.Cabin.Door.IsLocked.Row1.Left .. Vehicle.Cabin.Door.IsLocked.Row4.Right This new property is also found in the tree instantiation, the tools that transform from YAML to other formats just copies it into the IsLocked node. So the position redundancy is eliminated both in the YAML format, and in the format after tool transformation. This means that the server must interpret the pathextension property when it accesses this node, in order to match the path with the client request. If this processing is believed to be a problem, then the tool could be configured to write the pathextension value as a string containing all the complete path extensions, comma separated (Row1.Left, … ,Row4.Right). The search path “Vehicle.Cabin.Door.IsLocked” would match all eight position instances. The search path “Vehicle.Cabin.Door.IsLocked.Row1” would match the two Row1 instances. Search filtering could also be handled by query expressions. The proposal leads to optimal redundancy elimination at some small server processing cost, and it does not require any modification of the tools (except for a small modification of the c native tool).

klotzbenjamin commented 5 years ago

(from #48 ) Perhaps this question is rather obsolete now because of discussion had in #81 and ought to be closed? Or it needs to be updated since #81 clarifies tree structure and how to encode positional parameters, but it might not be documented (I think) how a Set operation shall function on a path query on to that new tree structure? (Is there a possibility/requirement to give multiple/different values for matching objects or only one?)

48 was one of the requirements that we wanted to ensure with this proposal. It is important that we meet it.

The idea builds on introducing a new key-value property with the key name pathextension which defines the position similar to position in Daniels proposal in the thread above.However, instead of finding it in a pbranch node, it is found in the actual leaf node.

This was one of the 2 initial proposals: either put position parameter in branch nodes or in leaf nodes. One of the reason for using pbranch and the first proposed solution is that position nodes are so far not consistently located close to leaves. For instance we have: - Axle.Row2.Wheel.Left: type: branch description: Row2 wheel left.

We could get rid of the position nodes and add a new entry in all the signals from Drivetrain.Axle.Wheel but I see 2 issues with this:

UlfBj commented 5 years ago

If a server contains Drivetrain.Axle.Wheel.Row1/2.Left/Right.Tire.Pressure/Temperature, if I want the pressure and temperature of the left wheel in the row1, can I express this as a single path (potentially with wildcard) in a query ?

With my proposal above, with a tree structure like: Drivetrain.Axle.Wheel.Tire.Pressure/Temperature where pathextension parameter in Pressure and Temperature nodes, the query would look like (I prefer a syntax where a wildcard represents one path segment, thus two wildcards): Drivetrain.Axle.Wheel.Tire...Left Regarding retrocompatibility I do not see that as high priority. And if you strive for it, you should be 100% compliant.

danielwilms commented 5 years ago

This new property is also found in the tree instantiation, the tools that transform from YAML to other formats just copies it into the IsLocked node. So the position redundancy is eliminated both in the YAML format, and in the format after tool transformation.

@UlfBj I see advantages of it, but what I like about the previous proposal more is that you define the position in the node, where it actually belongs to and to keep it logically together.

UlfBj commented 5 years ago

I see advantages of it, but what I like about the previous proposal more is that you define the position in the node, where it actually belongs to and to keep it logically together.

I would say that is what is proposed here. In the example I show above the position is defined in the IsOpen ans IsLocked leaf nodes in the vspec file, and this stays in the same nodes in the tree instantiation by the tools. So to make it clear, in my proposal the tools do not instantiate any "physical" position nodes, they exist "virtually" through the "pathextension" data in these nodes. The tree parser translates between the explicit position node data in the path expressions, and the "pathextension" data in the leaf nodes holding such data.

danielwilms commented 5 years ago

Ok, let's summarize:

Is wording and meaning ok @klotzbenjamin @UlfBj ?

UlfBj commented 5 years ago

Is wording and meaning ok @klotzbenjamin @UlfBj ?

It is fine with me.

klotzbenjamin commented 5 years ago

Is wording and meaning ok @klotzbenjamin @UlfBj ?

Same here.

UlfBj commented 4 years ago

The development of this position/extension mechanism seems to have stalled, which has led me to again think about it, and come up with a new proposal (sorry guys:). I believe my proposal should be reasonably simple to implement in the Tools, and it will also work together with the query mechanism I foresee in the W3C-Gen2 spec.

Rules for the extension model.

Example in YAML: #

Axle definition

#

#

Wheels on Axles

#

#

Tire

#

Example of existing branch expressions: Vehicle.Chassis.Axle.Row1.Wheel.Left.Tire.Pressure Vehicle.Chassis.Axle.Row1.Wheel.Left.Tire.PressureLow Vehicle.Chassis.Axle.Row1.Wheel.Left.Tire.Temperature

Same branch expressions after applying the extension model: Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row1.Left Vehicle.Chassis.Axle.Wheel.Tire.PressureLow.Row1.Left Vehicle.Chassis.Axle.Wheel.Tire.Temperature.Row1.Left

gunnarx commented 4 years ago

It's quite hard to just jump into this and understand the consequences of all the theoretical discussions. I therefore look to the examples for understanding:

Wheel: type: branch description: Brake signals for first row

Huh? There is a node which is named "Wheel" which has the purpose of showing Brake signals? And for only the first row? Maybe this is a typo. I just think that when it's a bit hard to wrap your head around a proposed concept, then the examples need to be very clear to not trip me up...

gunnarx commented 4 years ago

Spontaneously, not too happy with "extension-name"

In the example:

extension-name: "Row[1..2]"

This looks to me as a definition of an extension (or perhaps "extension point") and not only a definition of the name. Since it includes effectively the array size (or enum definition in the other format case), it is not only the name but also other information. Could the field then be called simply "extension:"?

Also, the number of levels seems like it could be derived from the spec (count the number of extension: entries)? I'm not 100% certain if the whole proposal "works" (still understanding it), but as I read it now I think "extension-levels" seems like redundant information that we skip?

gunnarx commented 4 years ago

Regarding the naming. What is the essence of extension? (I had to read through the history relatively quickly...) Would you say it is to talk about "instances" of a node definition? I'm thinking the word instance might be more familiar programming term. What we are then doing is dealing with a kind of addressing of instances? If you agree, some resulting proposals might be:

instance: "Row[1..4]" instance: "[Left, Right, Center]" or instance-address: "Row[1..4]" instance-address: "[Left, Right, Center]" or instance-specification: ...

UlfBj commented 4 years ago

The example is taken from the existing VSS tree, it is not my invented by me.

On Mon, Oct 28, 2019, 15:02 gunnarx notifications@github.com wrote:

It's quite hard to just jump into this and understand the consequences of all the theoretical discussions. I therefore look to the examples for understanding:

Wheel: type: branch description: Brake signals for first row

Huh? There is a node which is named "Wheel" which has the purpose of showing Brake signals? And for only the first row? Maybe this is a typo. I just think that when it's a bit hard to wrap your head around a proposed concept, then the examples need to be very clear to not trip me up...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GENIVI/vehicle_signal_specification/issues/81?email_source=notifications&email_token=AJERQQMYPF753JTIJ2L734LQQ3WI7A5CNFSM4GGVSHWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECM7JLA#issuecomment-546960556, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJERQQJU3I7HQ2XLRZ56U73QQ3WI7ANCNFSM4GGVSHWA .

UlfBj commented 4 years ago

The terminology could absolutely be improved. This is the "spec", so there is no other place to derive it from. However, the number of extension levels property can be claimed to be redundant as it can be derived from the number of extension-name lines. Maybe it should be scrapped? I claim "it works", else I would not have proposed it:). BR Ulf

On Mon, Oct 28, 2019, 15:08 gunnarx notifications@github.com wrote:

Spontaneously, not too happy with "extension-name"

In the example:

extension-name: "Row[1..2]"

This looks to me as a definition of an extension (or perhaps "extension point") and not only a definition of the name. Since it includes effectively the array size (or enum definition in the other format case), it is not only the name but also other information. Could the field then be called simply "extension:"?

Also, the number of levels seems like it could be derived from the spec (count the number of extension: entries)? I'm not 100% certain if the whole proposal "works" (still understanding it), but as I read it now I think "extension-levels" seems like redundant information that we skip?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GENIVI/vehicle_signal_specification/issues/81?email_source=notifications&email_token=AJERQQPTK73YNOLNRQD7ACDQQ3W4TA5CNFSM4GGVSHWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECM72RQ#issuecomment-546962758, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJERQQMQMQTOHCEH5LKYKQTQQ3W4TANCNFSM4GGVSHWA .

UlfBj commented 4 years ago

Instance could be a better terminology than extension. I like it.

On Mon, Oct 28, 2019, 15:12 gunnarx notifications@github.com wrote:

Regarding the naming. What is the essence of extension? (I had to read through the history relatively quickly...) Would you say it is to talk about "instances" of a node definition? I'm thinking the word instance might be more familiar programming term. What we are then doing is dealing with a kind of addressing of instances? If you agree, some resulting proposals might be:

instance: "Row[1..4]" instance: "[Left, Right, Center]" or instance-address: "Row[1..4]" instance-address: "[Left, Right, Center]" or instance-specification: ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GENIVI/vehicle_signal_specification/issues/81?email_source=notifications&email_token=AJERQQIUJZCRG3C6MCADVFTQQ3XOXA5CNFSM4GGVSHWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECNAKQA#issuecomment-546964800, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJERQQKGWOCNJPV4NUVI56LQQ3XOXANCNFSM4GGVSHWA .

gunnarx commented 4 years ago

Sidenote: Please try not to respond to these issues via email (and keeping a lot of quoted text in the response) because it adds so much noise and ugliness to the conversation.

(I wish GitHub did not even have this possibility!)

Basically, I think it is really hard to come to a consensus on some of these topics, so for that reason please keep things neat and try to be as clear as possible, and that is best done by replying in the Web interface only (as was done earlier in this ticket) and when necessary quoting only the relevant text.

UlfBj commented 4 years ago

In the existing tree the branches having Pressure as leaf node are: Vehicle.Chassis.Axle.Row1.Wheel.Left.Tire.Pressure Vehicle.Chassis.Axle.Row1.Wheel.Right.Tire.Pressure Vehicle.Chassis.Axle.Row2.Wheel.Left.Tire.Pressure Vehicle.Chassis.Axle.Row2.Wheel.Right.Tire.Pressure

In the proposed solution these branches will be: Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row1.Left Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row1.Right Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row2.Left Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row2.Right

where the last two nodes in the branches will be of node type "instance" (with Gunnars proposed terminology). These nodes will inherit properties from the Pressure node: datatype: uint8, type: sensor, unit: kpa, description: Tire pressure in kilo-Pascal (but these properties will not be copied into each node, the only properties in these nodes are name and type, where type MUST be instance). The branch node Tire contains the "instance-levels" (may not be needed), and "instance-address".

Just to be clear, in the YAML source the "instance" nodes are "virtual", existing only through the properties "instance-levels" and "instance-address" in the Tire node definition. The Tools will create the "physical" instance nodes in their output (and may not write the instance-properties into the Tire node as it is redundant there now?).

gunnarx commented 4 years ago

From what I can see we are using an associative mapping for extension/instance - YAML will not allow more than one with the same name.

I'm commenting under the assumption that we agree that we need to have multi-dimensional extension? It seems that way in the example (but more feedback on that later). For now, I would say let's design for it - there will probably be a need.

So, quoting the example a while back (even if we will switch to "instance" or other name later)

    Tire:
    type: branch
    extension-levels: 2
    extension-name: "Row[1..2]"
    extension-name: ["Left", Right"]
    description: Tire signals for wheels

This will not work since the mapping of extension-name is redefined

So we need to embed it into an array in that case, or find another node-tree structure where extension/instance are separated into different nodes and never multi-dimensional (But it is important that this stays simple)

(please help out, those of you who have thought a bit deeper on the structure, e.g. @danielwilms, @klotzbenjamin)

Valid YAML proposal. Switching now from extension-name to instance, or better instances:

Tire:
  type: branch
  instances: [ 
      "Row[1..2]",
      ["Left", Right"]
    ]

It is not as pretty but the above is valid YAML (here is a useful checker), and as we seem to agree now the "number of extension levels" need not be defined, since that is visible from the definition (simply the array length) Let's always avoid any redundant information, since it only causes more user error, and complexity in programming tools that need to check for inconsistency, and so on.

(Note that I'm just making incremental improvements here. I have still not wrapped my head around the whole thing fully to be confident that it works well in my mind. But once we sort out the details, then a bigger real-world example, or an actual PR, should give this confirmation)

gunnarx commented 4 years ago

Vehicle.Chassis.Axle.Row1.Wheel.Left.Tire.Pressure Vehicle.Chassis.Axle.Row1.Wheel.Right.Tire.Pressure Vehicle.Chassis.Axle.Row2.Wheel.Left.Tire.Pressure Vehicle.Chassis.Axle.Row2.Wheel.Right.Tire.Pressure

In the proposed solution these branches will be: Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row1.Left Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row1.Right Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row2.Left Vehicle.Chassis.Axle.Wheel.Tire.Pressure.Row2.Right

I think/hope we are separating the design of what's possible to encode in the VSS specification from the actual choice we make for the wheel pressure nodes in the actual VSS. I think we are, and that this is only a working example.

The reason is, personally I find the first one more logical. I still support to enable multi-dimensional instance/extension, but for this example I want to ask if there is a possibility to encode similar to the first structure somehow?

Or is this proposal forcing us to have extension points only at the leaf?

Essentially if the node structure could be encoding this reality in some way (without being too complex), because seen as a hierarchy this first one feels more logical.

Vehicle.Chassis.Axle.Row[1..2].Tire.[Left,Center,Right].Pressure

Any ideas?

Very off-topic side-note, but someone might note that there can be more than two axles, and the actual definition should probably prepare for that. There is no reason this standard cannot apply to heavy vehicles. Knowing that, let's keep going with the example

UlfBj commented 4 years ago

It is not as pretty but the above is valid YAML (here is a useful checker), and as we seem to agree now the "number of extension levels" need not be defined, since that is visible from the definition (simply the array length)

Logically this is the same, and as it is valid YAML, I find it to be a good solution.

But once we sort out the details, then a bigger real-world example, or an actual PR, should give this confirmation

As soon as we have the details sorted out, I am ready to create a PR containing an update of the VSS documentation, and an update of the c-native tool, which is my contribution to the Tool set. Updating of the other tools I then hope others take on.

I think we are, and that this is only a working example.

The example is coming from the existing VSS tree so unless we within this context also decides to update the existing tree structure relating to this, it is an actual example.

Or is this proposal forcing us to have extension points only at the leaf?

My proposal would only allow extensions at the leaf. Arguments for that restriction would be

Very off-topic side-note, but someone might note that there can be more than two axles, and the actual definition should probably prepare for that.

As I understand it, Row[1..X] is how the number of axles are declared. So heavy vehicles would be supported by this. Maybe the RowX nodes should be discarded, and should be declared as Axle[1..X] instead?

danielwilms commented 4 years ago

First of all: @UlfBj thanks for bringing it up again.

My proposal would only allow extensions at the leaf. Arguments for that restriction would be

  • Implementation simplicity. Both the tool implementation and the implementation of the server that will use this tree in serving clients.
  • URL compatability. In Gen2 the VSS paths will be explicit parts of the URLs, which e. g. will led to that wildcards cannot be used in the path (but in an appended query component). I would argue that we should not make it more complex than necessary, and hence restrict it to be allowed only at the leaf.

At the end instances are created by the leafs only. I like it to have it in the entire path, as it gives a clear mapping to what is multiplied. Also, it avoids duplication. Take Axle as an example. In case you have multiple leafs under it, you have to specify each and every time the amount of axles. The resulting tree meets IMHO your requirements.

As I understand it, Row[1..X] is how the number of axles are declared. So heavy vehicles would be supported by this. Maybe the RowX nodes should be discarded, and should be declared as Axle[1..X] instead?

you're right. As for the spec I would say the position has to be formed as [1,..,n] where n is the number of instances.

What do you think?

UlfBj commented 4 years ago

@danielwilms , I am not sure I understand your first comment. To clarify, in my example above where I introduced the "instances" properties (I prefer Gunnars terminology) in the Tire branch node, this node has three leaf nodes (Pressure, PressureLow, Temperature. Each of these leaf nodes would then be instantiated with the subtree declared in the Tire branch node. So the instance declaration is only specified once in the YAML source for all children of the branch. Therefore I still stand by my view above regarding restrictions on which branches can contain instance declarations: Only branches being direct parent of leaf nodes.

gunnarx commented 4 years ago

... would say the position has to be formed as [1,..,n] where n is the number of instances.

@danielwilms Just to clarify, by this you mean writing literally [1,..,7] for a possibility of the values 1,2,3,4,5,6,7? Or do you mean writing [1,2,3,4,5,6,7] ?

If the intention of this was to mimic a kind of array of instances I think [1..7] is a format more similar to what people are used to in programming languages. The other option is [7] but that does not clarify if the starting value is 0 or 1.