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

Add option to keep instance information #293

Merged
merged 1 commit into from Sep 20, 2023

Conversation

erikbosch
Copy link
Collaborator

@erikbosch erikbosch commented Jun 9, 2023

As of today all our tools expand instance information, resulting in that you do not know (for sure) if Row1 represents an instance or is a real branch. In some cases, like for example if you want to create an object oriented API/SDK this might be a disadvantage as you may want to offer methods like:

is_my_door_open = my_vehicle.getDoor(ROW1, DRIVER_SIDE).is_open()

... rather than having Row1 and DriverSide as objects. Having a JSON file where no transformation/expansion has been performed make it easier to generate the API/SDK as wanted. Under the hood the API/SDK must still create the old expanded path if it is to communicate with an implementation that rely on trditional expanded paths (like VISS and KUKSA.val)

The implementation in this PR add functionality for Yaml, JSON and CSV exporters. For others a warning is given if --no-expand is used. See test cases for examples.

The included *.json file on top level is only for reference and will not be part of final commit. It shows resulting JSON if using latest VSS and the --no-expand option.

@erikbosch erikbosch marked this pull request as draft June 9, 2023 13:09
@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Erik presented the ideas.
  • Please review/discuss

Expanded-info.md Outdated
...gives an extra output `expanded:true` for branches that has been expanded.

I.e. branches like `Vehicle.Cabin.Seat.Row2.PassengerSide` will have `expanded:true`, while regular branches like
`Vehicle.Cabin` and `Vehicle.Cabin.Seat` will not have any `expanded` attribute.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disadvantages would be:

  • The detection of instances is possible, in principle. But it requires extra effort.
  • The detection might face some limits, when it comes to instances that contain attributes or signals valid for that single instance (or more precisely: a limited set of instances), only
  • Digital.Auto is using a json-based VSS catalogue which is "not aware" of instances. There, the "expanded" attribute would need to be maintained manually, which probably leads to additional error cases.

Expanded-info.md Outdated
## Alternative 1B: Keep original instance-information (not implemented)

Instead of `expanded:true` we could keep the original expansion information like below, or format it in some other way.
But that require a bit more work, as today the original expansion information is modfiied during expansion

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This then would be a "mixed" solution?
Means, expanded instances plus original instance information?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, some form of mixed solution.

Expanded-info.md Outdated
"PassengerSide"
]
],
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this would be our (Eclipse Velocitas') favorite solution, because with that the generated files (json, ...) become semantically identical (except file structure) to the original vspec.

I also tested this with our code generator and it works fine.

@BjoernAtBosch
Copy link

We (Velocitas) also need to discuss with Digital.Auto, what they think/prefer.

@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Sebastian: How would overlays work, adding a signal for a specific instance
  • Please review

@erikbosch
Copy link
Collaborator Author

Added test examples on how this works with overlays. In short, using our overlay mechanism to add signals to expanded paths works, but then the consumer of the output need to do the logic understanding that Vehicle.Door.Row1.Pos1.NewSignal actually refer to the signal Vehicle.Door.NewSignal in the door instance Row1.Pos1.

That is likely Ok for now, but perhaps as part ongoing VSS discussions (this one, ontologies, ...) we possibly need to refactor how we define and use instances, like discussed in COVESA/vehicle_signal_specification#642.

@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Erik: Proposal - document limitation "individual instances in overlays" and the merge
  • ok

@erikbosch erikbosch added Status:Rework Committer must refactor or address comments Status:Approved labels Sep 20, 2023
Signed-off-by: Erik Jaegervall <erik.jaegervall@se.bosch.com>
@erikbosch erikbosch removed the Status:Rework Committer must refactor or address comments label Sep 20, 2023
@erikbosch erikbosch merged commit 51bf94e into COVESA:master Sep 20, 2023
4 checks passed
erikbosch added a commit to boschglobal/vss-tools that referenced this pull request Sep 20, 2023
Signed-off-by: Erik Jaegervall <erik.jaegervall@se.bosch.com>
erikbosch added a commit that referenced this pull request Sep 20, 2023
Signed-off-by: Erik Jaegervall <erik.jaegervall@se.bosch.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants