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

Feature request: support strings in the list of suffixes in an instance definition #375

Closed
slawr opened this issue Nov 29, 2021 · 6 comments

Comments

@slawr
Copy link
Contributor

slawr commented Nov 29, 2021

The VSS v2.2 data model rules support instances using numerical suffixes.
For example:

Door:
  type: branch
  instances:
    - Row[1,4]

producing:

Row1.
Row2.
Row3.
Row4.    

The request is to extend the rules to allow a list of strings to be used as the suffix.
For example:

Door:
  type: branch
  instances:
    - Front["Left", "Right"]
    - Rear["Left", "Right"]

would result in:

Door.FrontLeft.
Door.FrontRight.
Door.RearLeft.
Door.RearRight.
@slawr slawr changed the title Feature request: support strings in the list of suffixes in an instance block Feature request: support strings in the list of suffixes in an instance definition Nov 29, 2021
@erikbosch
Copy link
Collaborator

For the sake of completeness, we already support enums like this:

Door:
  type: branch
  instances:
    - ["Front","Rear"]
    - ["Left","Right"]
  description: All doors, including windows and switches

Will give you signals like:

Vehicle.Cabin.Door.Front.Left.Window.ChildLock
Vehicle.Cabin.Door.Front.Right.Window.ChildLock
Vehicle.Cabin.Door.Rear.Left.Window.ChildLock
Vehicle.Cabin.Door.Rear.Right.Window.ChildLock

But we do not support prefix in combination with enum, like this:

Door:
  type: branch
  instances:
    - ["Front", "Rear"]
    - Pos["Left", "Right"]

I assume it should be quite easy to change tooling so that the prefix (if any) is considered and correctly handled.

@danielwilms
Copy link
Collaborator

Personally, I like the notation, we have now, like

instances:
    - ["Front","Rear"]
    - ["Left","Right"]

more than the prefix from the first comment. But definitely open for discussion. As I see it now, there isn't anything, we don't support with the current solution, right?

@danielwilms danielwilms added the v3 label Mar 29, 2022
@slawr
Copy link
Contributor Author

slawr commented Mar 29, 2022

Personally, I like the notation, we have now, like

instances:
    - ["Front","Rear"]
    - ["Left","Right"]

more than the prefix from the first comment. But definitely open for discussion. As I see it now, there isn't anything, we don't support with the current solution, right?

On the specific point about the door notation then agreed or perhaps put more clearly my example just happened to use doors it was not intended to be a proposal for change in the standard catalogue.

[Update: reading your comment again Daniel perhaps you meant less the standard catalogue and more the prefixes used to separate left/right and front/rear. If that is what you mean I see your point in terms of style. In the general sense of text prefixes I think the question still stands]

I found the issue in a corner case of using vspec to define an existing data model to be interfaced to VSS. It can be seen here (see comment about lack of air bags in rear seat) : https://github.com/slawr/vss-otaku/blob/0f4c7b8f7f14d64a5f5cbfc1a1e66048a00ee5d9/converters/r-sim2vss/model/renesas-sim-data-model.vspec#L364

My example was purely to illustrate the problem. It was fairly easily worked around. I simply described the nodes in long hand rather than use the instance feature. As importantly I do believe in the KISS strategy for VSS and so I do not think you should start bending VSS to all possible data models.

So for this issue I think we can consider purely the "instance building" aspect of the request for VSS and the standard catalog. In those terms I assume there are two aspects, 1. the vspec rules and 2. (perhaps larger in terms of effort) vss-tools changes to implement.

From @erikbosch input it seems the vss-tool changes may not be onerous but of course there is still a cost benefit aspect to any change. I am not personally especially opinionated on whether it must be implemented. More that I noticed the gap in the instance handling and reported it for wider discussion.

@erikbosch
Copy link
Collaborator

Concerning the lack of Airbag for rear seat in your instantiation we have briefly touched the subject of "pruning" the signal tree in e.g. #252 to remove nodes/branches that will not exist in a deployment. A typical example concerns seats where we typically define 2 rows and 3 positions, but many cars does not have 3 positions/seats in the first row, and then tree pruning could be used to remove "Seat.Row1.Pos3".

But as of today we have no proposed way to remove individual signals for individual instances in a deployment, like keeping the airbag branch only for seats in the front row, more than first expanding the tree and then manually removing signals based on what each seat support. But possibly the prune-concept could be extended to cover this as well.

@slawr
Copy link
Contributor Author

slawr commented Mar 30, 2022

Thanks for the heads up Erik. To be honest I was thinking later last night that I probably shouldn't have mentioned the airbag in my recent comment as it was in fact a separate issue and that the github comment could do with some editing for simplicity. Dangers of updates at the end of a day. Anyway I will try to revisit it. The gist of it is really just something like "should tooling be able to join two string parts to make a single instance naming".

@erikbosch
Copy link
Collaborator

I am doing some cleanup closing all issues created 2021 or earlier where there have been no activity 2023 or later. If you still consider this issue relevant feel free to reopen it add and add a comment on how you would like to see progress on this one.

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

No branches or pull requests

3 participants