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.

Sign up for GitHub

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

Jump to bottom

Left/Right Instance Discussion #718

Closed
wants to merge 1 commit into from
Closed

Left/Right Instance Discussion #718

wants to merge 1 commit into from

Conversation

erikbosch
Copy link
Collaborator

This is a follow up proposal to the items listed below, showing a potential solution:

  • Duplicating instances os we have both Left/Right and DriverSide/PassengerSide
  • Leaving mapping to implementation

Concerning implementation, I see several alternatives.

  • If an OEM exclusively use Left/Right or Driver/Passenger references, they can in overlay remove the labels they do not need, or just not provide values for the unused "dimension"
  • If both are used it can either be managed within the server by some config, or you have a LHD/RHD-specific mapper that manage mapping where needed.

Would a change like in this PR be an acceptable solution?

#695
#639
#642

@erikbosch
Left/Right Instance
78a7df4 
Signed-off-by: Erik Jaegervall <[email protected]>
@erikbosch erikbosch marked this pull request as ready for review February 12, 2024 08:54
@erikbosch erikbosch added Scope:Major A change that either significantly adds new functionality, or affects backward compatibility. Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting Status:Meeting Intended to be discussed at next VSS-project meeting labels Feb 13, 2024
@erikbosch
Copy link
Collaborator Author

erikbosch commented Feb 13, 2024 •
edited
Loading

MoM:

  • Please review/discuss, will need longer discussion if this is how we want things to be
  • Daniel: Instances cannot be normative, focus should be mostly on entities, feature of interest, only suggesting a set of instances. But current expanded representation is a bit problematic as it mix entity and instance.
  • Erik: Maybe discuss the topic at AMM

@erikbosch erikbosch added Status:DoNotMerge PR shall not be merged now, maybe later, maybe after rework and removed Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting Status:Meeting Intended to be discussed at next VSS-project meeting labels Feb 19, 2024
@erikbosch erikbosch marked this pull request as draft February 19, 2024 12:23
@erikbosch
Copy link
Collaborator Author

Putting it on hold/draft for now, in the sense that there seems to be more to discuss

ppb2020
ppb2020 reviewed Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@ppb2020 ppb2020 left a comment

Choose a reason for hiding this comment

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

Some thoughts on this topic.

docs-gen/content/rule_set/instances.md
by a configuration file.


As an example is shown below for doors. In VSS four instances of doors are specified for each row,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Review phrasing. Not clear what is meant by "As an example is shown below."

"In VSS" -> "In VSS,"

docs-gen/content/rule_set/instances.md

As an example is shown below for doors. In VSS four instances of doors are specified for each row,
but there are at most two physical doors on each side.
`DriverSide`and `PassengerSide`can be considered as aliases for `Left`and `Right`, or vice versa.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can -> can

as aliases -> aliases

and -> and

docs-gen/content/rule_set/instances.md

<!-- Image source in docs-gen/image_source/instance_tree.puml -->
![Example tree](/vehicle_signal_specification/images/instance_tree.png)

When expanded this corresponds to:

If expanded (for instance by [VSS-tools](https://github.com/COVESA/vss-tools)) this corresponds to:
Copy link
Collaborator

Choose a reason for hiding this comment

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

) -> ),

docs-gen/content/rule_set/instances.md
interpreted by the tools. As an example is shown below for doors:
interpreted by the tools.

The instance names defined in VSS can be seen as labels
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this makes sense, but I would like to spend some more time thinking this through as to how this would be implemented in practice.

I immediately see two approaches:

  1. Use an overlay to (effectively) delete the entries that are not implemented (that is, replace ["DriverSide", "PassengerSide", "Left", "Right"] by ["Left", "Right"] or ["DriverSide", "PassengerSide"]).
  2. Support them as aliases.

For 1., there are portability issues (an app written to look for DriverSide would not find it on vehicles that use a ["Left", "Right"] definition, and thus would need to be adapted). There is also the argument that if we expect an overlay to be used here, then going with one or the other of ["Left", "Right"] or ["DriverSide", "PassengerSide"] makes some minimal portability guarantee (at least for vehicles that use the definition as it is).

For 2., any code that reports the value for this signal would need to know that it needs to report (or store) this value twice and not just once as it normally does for other signals. Another issue is that

As well, this approach embeds the notion of left-hand vs. right-hand driver side in a bit of an awkward way. Thinking out loud, would it be better for this notion of driver side not to be embedded in this signal, but carried independently, leaving this signal to only have ["Left", "Right"] as instances, with the application relaying on the independent driver position information to cheese between the two instances?

Lastly, where does this leave us with vehicles that do not have a driver side? That is, where the driver is in the middle?

@erikbosch erikbosch mentioned this pull request Apr 8, 2024
Merged
@erikbosch
Copy link
Collaborator Author

Closing this one for now, may reopen later

@erikbosch erikbosch closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppb2020 ppb2020 ppb2020 left review comments

Assignees
No one assigned
Labels
Scope:Major A change that either significantly adds new functionality, or affects backward compatibility. Status:DoNotMerge PR shall not be merged now, maybe later, maybe after rework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erikbosch @ppb2020