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

Simplify overlay handling #360

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Conversation

erikbosch
Copy link
Collaborator

@erikbosch erikbosch commented May 29, 2024

This is an idea for simplifying overlays. If a node is found in overlay without type (e.g. branch, sensor) or datatype (e.g. uint8) it will try to do a look-up if a node with the same name already exist, and then use that type/datatype value.

There are limitations, like if you add something to Vehicle.Axle.Front.SomeSignal in an overlay (i.e. only for a single instance) it will not find source signal Vehicle.Axle.SomeSignal.

Many of the changes are just adding license headers. As I got failure on some changed files I also updated some files that were not changed.

Next step if this is merged is to adapt documentation in VSS repo. Possibly saying something like:

  • In most cases you do not need to specify datatype and type in overlay if you are modifying an existing signal.
  • Specifying datatype and type is needed if overlay addresses specific instances of a branch, like front left wheel.

@erikbosch erikbosch force-pushed the erik_overlay branch 4 times, most recently from 5dffe4b to afbf1ed Compare May 29, 2024 14:03
@nwesem
Copy link
Contributor

nwesem commented May 29, 2024

thx @erikbosch this is a great addition to implement issue #193

I tested it on a clean install of vss-tools and is working as expected on non-struct signals. I haven't looked into the mentioned limitations on structs yet

@erikbosch erikbosch marked this pull request as ready for review May 30, 2024 06:18
Datatype and type no longer needed if changing a node that already exists.
Note may not work for if overlay is specified for instantiated signals.

Signed-off-by: Erik Jaegervall <[email protected]>
# This file tries to use implicit type/datatype for already existing signals
# That does not work due to a limitation in current name lookup

A.B.Row1.Left.C:
Copy link
Collaborator Author

@erikbosch erikbosch May 30, 2024

Choose a reason for hiding this comment

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

This is an example where name lookup does not work as Row1.Left comes from an instance declaration. Lookup is currently NOT so smart that it realizes that it rather shall look for A.B.C in tree to find type/datatype.

As of now you get warnings/errors like:

WARNING  No old type found for A.B.Row1.Left.C
ERROR    No type specified for A.B.Row1.Left.C

@nwesem
Copy link
Contributor

nwesem commented Jun 4, 2024

@erikbosch I tested the structs and can confirm that I get the same warnings and errors. Now I was wondering what your planned next steps are on this topic?

@erikbosch
Copy link
Collaborator Author

I did not had time (or forgot) to mention the PRs in vss-tools yesterday, but as you have already looked at it I see no problems merging this one. I have no problems updating documentation, if you have time and interest in something for #193 feel free to try it out, or if you want to improve the limitations.

@erikbosch erikbosch merged commit 5e86a59 into COVESA:master Jun 7, 2024
5 checks passed
@nwesem
Copy link
Contributor

nwesem commented Jun 25, 2024

hi @erikbosch was there a specific reason why you didn't implement the limitations for structs? I would like to proceed with it to make #193 work. An idea how you would overcome the limitations would be a valuable first step for me as I haven't worked much with the code that deals with structs.

@erikbosch
Copy link
Collaborator Author

For structs (i.e. type files) I did not really did any tests so there I do not really know how much effort, if any, that is required.

For instances we can take the example in COVESA/vehicle_signal_specification#747

# Type and Datatype not needed
Vehicle.Speed:
  unit: m/s

# Type and Datatype needed as overlay affects a particular instance
Vehicle.Occupant.Row1.DriverSide.HeadPosition.Yaw:
  type: sensor
  datatype: float
  unit: mm

Here the problem with the second one is that when we read the overlay we have not yet analyzed/expanded instances, so you can easily find Vehicle.Occupant.HeadPosition.Yaw in tree but you would need a more advanced search algorithm to find Vehicle.Occupant.Row1.DriverSide.HeadPosition.Yaw as Row1 and DriverSide comes from instance information. Like finding Vehicle.Occupant and then check if Row1 refers to a real sub-branch or rather to an instance. This is a bit related to the discussion in #749,

@nwesem
Copy link
Contributor

nwesem commented Jul 16, 2024

hi @erikbosch, sry about this, I accidentally called instances structs in this whole conversation and just realized it now.. I was referring to instances the entire time, as structs are not being used (yet) as far as I know.
I'm currently trying to come up with a search algorithm to find the definition of a signal that was instantiated to link its datatype and/or type to it when it is being overwritten from an overlay..

@nwesem
Copy link
Contributor

nwesem commented Jul 18, 2024

hi @erikbosch would it be possible to have a short 15-30min call about finding the type (and datatype) for instances? I've been trying to find a way to search for them but as this data is cleaned from the model before rendering a tree, I can't find a slick solution to retrieve the type and datatype. I would really appreciate it!!

@erikbosch
Copy link
Collaborator Author

@nwesem - we could try to align on a slot using the COVESA slack channel, if that works for you

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

Successfully merging this pull request may close these issues.

2 participants