[VSS2] Position-related branches proposal #81
Comments
|
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.
I see that this is an issue. But it's again more of a tooling/rule question.
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. |
|
what about this one @klotzbenjamin, @UlfBj? Any further opinions? Do you think it's needed? I'd close it otherwise. |
I agree. That should be a first step for being a bit more consistent.
It is not so much an issue in regard to the tree structure of VSS. We can always use The issue is mostly about clarifying what a 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. |
|
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
|
|
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. |
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 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.
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. |
|
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: I like the proposal #81 (comment) most, when putting the positioning info at the end, but with dot-notation: 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
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: Final question: What do you guys think? @UlfBj @klotzbenjamin @magnusfeuer @rtroncy |
|
So the tool would then generate a tree like the following? |
|
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 |
@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: 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. |
|
Hi @UlfBj, sorry for the delay. I could imagine a tree being generated like this: For the description please check #81 (comment) |
|
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:
|
Leave nodes are either the datatype as before (single node) or a list/structure {"Row1":{"LEFT":,...}}
Those are basically instances of pressure addressed by the position structure
This is not possible right now, but wasn't before either. Same question I got of a colleague ;)
Of course, that's why we discuss it here. Happy to get comments so quickly! |
|
Just to exhaust all possibilities I want to propose one more solution. |
|
|
How would this translate for subscribe ?
... {"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 |
|
What makes it different for subscribe compared to other actions? |
|
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 ? |
|
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. |
|
Ok. |
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:
How it then get's accessed through the protocol is another question then. |
|
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). |
|
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. |
|
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... |
This looks good. One possible improvement. How about
@PeterWinzell Sounds like a good use-case for checking the concept. Will write it down. |
danielwilms
added this to In progress
in vehicle_signal_specification prepare version 2
|
Hey @UlfBj
(I'm stripping out the case of "instance":"[1..2]", since there is less controversy for this case I think).
Is my interpretation of you proposals correct?
Here, I pledge ignorance. Where is this rule stated?
I understand this design rationale. But then, IMHO, you need to name your instances differently to remind what they are replacing while in @gunnarx's proposal you don't have to. This means you need to provide additional guidelines or best practices on how instances should be named.
I'm on the side of @gunnarx here. For the ones who really try to understand the value of each other's proposal, I can tell you that we much prefer clarity than saving x number of lines in lengthy comments. I appreciate though that this is a matter of style. |
From my view: Yes.
Your ignorance may be excused by that there seems not to be a normative statement expressing it.
This is a good point, I agree. It does not strike me as a particularly hard task to resolve. One suggestion from the top of my head could be to specify that the name should be "Placeholder:A/B" where A and B are the names from the instance declaration. |
I think we do. But I really don't have the time to doublecheck if you correctly restated my position again. You can't convince people by repeating the same things until they give up. I already provided an answer to the full example before. Actually, I asked you to not restate the differences once again. Maybe it helped someone else, I don't know, but I really feel they were better described earlier.
That is your way of writing it. In my opinion the parent is already written in the YAML. And I suppose the instances appear, yes, that's the instantiation process. You did not like my description "disappearing", so I don't like yours where you call them "appearing". Shall we leave it at that?
I also sort of felt these rules seem fairly arbitrary since we are talking about a major design change to VSS here. I still feel it is better than the alternatives. In my proposal, either the new parent node (Wheel in our example) needs to transformed to a branch once when it is instantiated, or perhaps it does not matter that much? I think I'm just more focused on the YAML definition (where no rules are broken in my proposal), and on the ability to address the end result. If you want to be formal about it, then sure we can say that the parent becomes a branch in the instance process. It's not that important to me.
Its name disappears, that's for sure. But you feel the node does not disappear. It is all matter of definition for me, so let's leave it. (on writing guidelines)
Is it roughly comparable to writing the following line to resolve the supposed problem with my proposal: "When a node is instantiated in a way that creates new children, a branch is inserted and the instances become its children"
Oh here we go again... ;) And why do we not have a design rule that we don't put node names in the YAML tree that are never part of the resulting addressing path? The rule that your proposal would violate? OF COURSE you should try to avoid breaking design rules. You should also make sure those rules are there for a reason, and also be willing to compromise between different conflicting choices. I don't see this helping the discussion. It would be enough that you specify what you find to be illogical and then leave it there.
First, I was talking about the effects in both mine and your proposal. Primarily yours, since we had not seen it (for leaf instances). I was also talking about including it in the full example which we use for the comparison. You correctly described what my proposal would yield, but not how your proposal might address it. That is why I think it is better if we have a real example for this.
I still don't understand because I feel you keep avoiding the question and repeating other details we already know. HOW would you make several instances of a sensor in your proposal, or can you please show on a real example a different way forward? I imagine you propose that the designer needs to create a branch above the sensor in order to enable instantiation on that branch instead of the sensor itself. But it's not helpful that I try to interpret and make things up about your proposal. I will probably get it wrong. You should provide it so that it becomes correct. A real example would help us understand this. Also I don't know why you ignored my idea that we might deal with if instances (of sensor) are allowed or not after deciding on how the instantiation on branch nodes. Maybe see if we can add it to your proposal. It was really intended as a way to help your proposal, and help find some new compromise ways forward. Otherwise it seems to me that your proposal would fail immediately because of inability to have instances on things like sensors? But there are surely ways around, so why not look at them? |
I'm not sure exactly how to do that in a good way but how about this then. becomes or "instance_definition" or whatever. or some symbol: Of course it could be just a guideline to use the same generic name all the time, while the actual rule is that it could be anything. Since the name "disappears" (sorry, I don't know a better word) anyway we could reconsider all of these options - maybe then think outside the box and consider other formats of defining instances. I know we are eager to close this, but we should take the time to get it right. Of course I'm opening this mostly because of how much I dislike the addition of a node name that is not used in the result. This is illogical to me. So I'm wondering if there are compromises. Maybe there is a completely different and better way to write instance definitions. |
|
Thanks @gunnarx for proposing a possible convention for naming placeholder node that aim to be instantiated, e.g.
Now, a naive question, that I think is relevant for the 2 proposals on the table: will we necessarily always instantiate those nodes? I mean, what if you want to write something for both wheels? At some point, much earlier in the discussion, we were talking about the use of wildcards, is it related? |
|
|| HOW would you make several instances of a sensor in your proposal With the risk of being told to continue repeating, I point to comment #81 (comment)
@gunnarx proposal of using a reserved word for nodes in YAML that will be expanded is a good idea, which I already have adopted;) If we by this now are on the same page, I think we should try to move on. @rtroncy I do not get your question, please elaborate a bit more. |
OK, understood and this is as expected, but it is an example of where leaves (The leaf is "Heating", or earlier in your text "E") gets duplicated because of the branch having instances. My example, as given also before, is rather one where the actual leaf needs instances. Just building on your example here, you might have had two Heating sensors on each mirror. So two on the Left and two on the Right. You cannot instantiate Heating in your proposal, so it will then require the designer to add another branch to achieve it, like this: yielding: It's possible, but I wanted to find consensus with you that this becomes the required behavior. |
My understanding is that all combinations will be instantiated (as we saw in the full example). I have a slight different thinking when it comes to "generating" nodes but I have just accepted it for the sake of the conversation so far. In my view, a server might never actually print out a list of all combinations, but might just directly implement based on the YAML spec the ability to address each node using the path we have shown and called "generated" result. Do they "exist" as objects. Yes I would suppose so. I just don't put emphasis on the generated one being the official tree (as opposed to the original YAML file including its instances). Maybe they will be printed in a flat list or CSV just for clarity. But I would prefer to never see a YAML file generated to include them - I much prefer there is basically only one YAML file in the system at any time, which defines the database once. Those are probably very minor distinctions, I don't know. Nonetheless, I think listing all the combinations that become "generated" helps our discussion.
Don't understand. We already have examples of instances that end up on both wheels, simply because the wheels have instances. (All branches below the instantiation become duplicated automatically, as seen in the full example).
I'd say it is worth discussing. I know the group has from time to time said remove the simple wildcard (I here am talking about W3C VISS/v2/Gen2, so to be strict the protocol discussion is distinct from VSS itself, but still important to keep in mind). So I mean addressing a path like A.B.C.*.E. I've always thought such wildcards are useful to keep, actually. |
|
I think the example below solves your scenario also? "instance":["Left", "Right"] in first instances This example shows that the idea with the reserved word "instances" may need some further development. |
What? Yuck. :) So you would write this in the YAML?
Please write out both nodes, what the YAML looks like. Until now I didn't expect ever seeing "instances.instances", so I think the full example is needed (You could edit above).
Fine. But just like the original plan where instances were pushed to the leaf, this tree does not look like it is mirroring the "real" hierarchy -- would much prefer to have it named Heating.Primary
I still have my own original proposal as choice #1, but regarding the modification of yours I'm much more partial to this alternative actually: The question on wildcards is interesting too. Perhaps even makes sense? |
|
Also a tip is to use the triple back-tick or ```a few words``` to get : Helps to separate code examples from text... |
I think wildcard is actually a better choice than "instances" as placeholder node name. |
First attempt on how the tooling could look like with the positions as instances. Dot notation might not be the best form. But as discussion item for now to conclude COVESA#81.
|
Hi all,
Unfortunately Github can't do sequences of PRs properly, or at least I don't know how. So please just consider the second commit of the second PR. |
That's great. I think we might then be getting towards an acceptable compromise. I still prefer my proposal but think we can reach a compromise here. My favorite wildcard (until convinced otherwise) if following "your proposal" is then to use underscore, as shown earlier. A.B.C._ :
I don't understand this example. I think we require the full path in the YAML? I just copied this from some YAML documentation (not the actual spec, it is harder to read) :
... so you can't redefine the value of the key to something else and expect to keep both of them, and also you can't depend on the order they are defined. |
@danielwilms GitHub will compare your branch used for the PR against the "base branch" the PR is sent to, which in the case of #130 and #131 is GENIVI:master. If your second branch (named second_pr_positioning) included only the tooling commit on top of master then only that would be shown. It however includes both commits - you can see it in the list here Possibly you created your second branch starting from the first, and then just added the new change. |
I agree with @gunnarx. I would see the instances as part of the yaml branch or leaf.
I would prefer the instances at the end, too. as shown in the csv result of #131. This leads to an understandable and readable base tree with a minimal number of concepts, which have to be understood. Further the path to the leaf is clear. The instances are added after the leaf in the definition. Further instances can change, without effecting the base tree.
In my opinion the definition of how to address the instances with e.g. wildcards shouldn't be part of VSS, but the protocol serving the tree. It might be different in e.g. GraphQL vs REST. I would keep that discussion out of context here. |
First attempt on how the tooling could look like with the positions as instances. Dot notation might not be the best form. But as discussion item for now to conclude COVESA#81.
First attempt on how the tooling could look like with the positions as instances. Dot notation might not be the best form. But as discussion item for now to conclude COVESA#81.
First attempt on how the tooling could look like with the positions as instances. Dot notation might not be the best form. But as discussion item for now to conclude #81.
|
All. Do we need to move the instance support, currently only implemented in the CSV generator, into If that is the case, are we ok with the current YAML structure and its corresponding CSV output structure that is about to be replicated for all other generated formats, or do we want to change anything? |
|
Whatever the result of instantiation and include statements is, it should naturally be same for all generators. I am not convinced that we are all fully agreed yet but the CSV has sort of been the place to evaluate the results. I see no problem in moving the code to a common place even if we discuss a fee more tweaks of the result later. |
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.Pressureto 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.PressureIn 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.PressureWhere the branches Chassis and Wheel are parametrized with their position.
Or even
Signal.Chassis.Axle.Wheel.Tire.Pressure-Row1.LeftWhere 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 #48
The text was updated successfully, but these errors were encountered: