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

Driver/steering position issues #582

Merged
merged 1 commit into from
May 3, 2023

Conversation

nickrbb
Copy link
Contributor

@nickrbb nickrbb commented Apr 20, 2023

This PR addresses two identified issues:

  1. Vehicle.Cabin.DriverPosition values can be 'LEFT', 'MIDDLE', 'RIGHT'. However, Vehicle.Chassis.SteeringWheel.Position can only be 'FRONT_LEFT', 'FRONT_RIGHT', meaning that there is a possibility that the steering position and driver position cannot be set the same when the driver/steering position is in the middle.
  • Suggested fix: Remove Vehicle.Chassis.SteeringWheel.Position entirely, since this is an overlap with Vehicle.Cabin.DriverPosition.
  • Cleanest solution, especially since in VSS v4.0 we don’t have to worry about backwards incompatibility.
  • Note: There are other leaf nodes under the Vehicle.Chassis.SteeringWheel branch that should of course remain
  1. Vehicle.Cabin.DriverPosition default value is ‘LEFT’. However, it is unclear why this needs a default value since other signals that are restricted to Allowed Values do not have a Default Value specified. Furthermore, specifying a default of ‘LEFT’ increases the chances of right-hand drive vehicles being misconfigured (e.g. because the implementer forgot to actually assign it a specific value), which could affect up to 54 of 195 UN recognised countries (see https://en.wikipedia.org/wiki/Left-_and_right-hand_traffic#Worldwide_distribution_by_country). In a worse case scenario, such a misconfiguration could wrongly infer to national authorities that the vehicle is illegal in that country due to some countries prohibiting registration of vehicles with steering wheels on the “wrong side” of the vehicle (see https://en.wikipedia.org/wiki/Left-_and_right-hand_traffic#Legality_of_wrong-hand-drive_vehicles_by_country).
  • Suggested fix: Remove default value of Vehicle.Cabin.DriverPosition.
  • This then gives better assurance of correct vehicle configuration by forcing implementers to actually set this value in their deployments rather than (possibly inadvertently) relying on a default value which will be wrong in at least 28% of UN recognised countries, as well as in vehicles whose steering is located in the middle (e.g. some sports cars, motorbikes/mopeds, etc). A blank/null value is easily caught in testing and therefore can be rectified early.

@erikbosch
Copy link
Collaborator

I triggered the build - needed for first time contributors.

DCO-check fails, all commits shall include a sign-off, see https://www.covesa.global/contribute

So before being merged the commits shall be updated to include a sign-off. They can also be squashed first if you like. Apart from that I think the PR is OK and could be included for VSS 4.0 if no-one has any objections so it can be improved at latest May 2nd.

@nickrbb
Copy link
Contributor Author

nickrbb commented Apr 20, 2023

Happy to merge the commits and add a sign-off. Is there some easy way to do the merging of the commits here in the web interface, or do I need to do them locally in the branch of my fork and then push the updates to this PR?

@nickrbb
Copy link
Contributor Author

nickrbb commented Apr 21, 2023

OK, I've now squashed the two commits and added the sign-off in the commit message. Let me know anything else I need to do.

…sis.SteeringWheel.Position

Also modified comment to Vehicle.Cabin.DriverPosition to remove text about default value.

Signed-off-by: Nick Russell <nrussell@blackberry.com>
Copy link
Collaborator

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

Both changes make sense to me: I think e sort of agreed that default should not be used in standard catalogue anyway, as this is more deployment specific information.

Also, I agree SteeringWheel Position is redundant. (We might want to model something later about the position of the steering column on cars where you can higher/lower the steering wheel, but this is not what the original point meant to capture)

@erikbosch
Copy link
Collaborator

Meeting note: merge

@erikbosch erikbosch merged commit 61ad838 into COVESA:master May 3, 2023
3 checks passed
@nickrbb nickrbb deleted the DriverSteeringPos branch June 27, 2023 09:36
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.

None yet

3 participants