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

Refactoring charge plug signals and clarifying enum naming recommendations #388

Closed
wants to merge 1 commit into from

Conversation

erikbosch
Copy link
Collaborator

@erikbosch erikbosch commented Dec 20, 2021

Fixes #376

Merging the two existing signals to a common signal. Aligned with EvConnectorType in https://android.googlesource.com/platform/hardware/interfaces/+/master/automotive/vehicle/2.0/types.hal - it seems to be quite exhaustive and also include some Tesla-specific connectors.

As of today we are are a bit inconsistent on enum symbols (string literals) in VSS concerning style. Here using a pattern with underscore and upper case letters, but I am open to using a different pattern. As long as we do not use whitespace I am happy. As the signals involved in this change also are used as examples I took the liberty to also extend and change the documentation accordingly.

See https://www.evexpert.eu/eshop1/knowledge-center/connector-types-for-ev-charging-around-the-world
for general information on connector types

@erikbosch erikbosch force-pushed the erik_chargeport branch 2 times, most recently from 34489ca to 14e0e9a Compare December 21, 2021 13:03
@erikbosch erikbosch changed the title Refactoring charge plug signals Refactoring charge plug signals and clarifying enum naming recommendations Dec 21, 2021
default: UNKNOWN
enum: [UNKNOWN, ALT_1, ALT_2]
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general I think this applies to the 'allowedValues' change as well. Just still not sure if we should recommend the use 'UNKNOWN'.

I'd prefer a recommendation for the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it would be good if VSS at least states a recommended approach for allowed, especially concerning undefined/unknown. I can see two major tracks:

  • Recommend that first allowed value for strings always shall be UNKNOWN. Or e.g. UNDEFINED if we consider that a better choice. At least for actuators UNDEFINED might be a better option as it could be used to state that you do not care, for instance if someone would like to use something like VSS to specify rental car preferences.
  • Recommend that UNKNOWN (or similar) generally not shall be used, unless there are good reasons, e.g. compatibility with values from an existing standard/protocol

@danielwilms
Copy link
Collaborator

@erikbosch, it would be great, if we could separate the documentation proposal from the actual changes.

My proposal: let's get #381 merged, create a PR for the docs on top of that one and then do the actual changes. What do you think?

@erikbosch
Copy link
Collaborator Author

erikbosch commented Jan 7, 2022

@danielwilms - your suggestion makes sense. I will convert this one to a draft and adapt it when #381 and related changes are merged

@erikbosch
Copy link
Collaborator Author

Closing this one, will replace the charge plug signal renaming with a different PR

@erikbosch erikbosch closed this Jan 26, 2022
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.

ChargingPort / ChargePlugType signals
2 participants