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

V201/missing component and type enums #300 #487

Merged

Conversation

Jared-Newell-Mobility
Copy link
Contributor

No description provided.

Comment on lines 1754 to 1758
OfflineQueuingSeverity = "OfflineQueuingSeverity"
MonitoringBase = "MonitoringBase"
MonitoringLevel = "MonitoringLevel"
ActiveMonitoringBase = "ActiveMonitoringBase"
ActiveMonitoringLevel = "ActiveMonitoringLevel"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These aren't snake case.

Copy link
Contributor Author

@Jared-Newell-Mobility Jared-Newell-Mobility Oct 13, 2023

Choose a reason for hiding this comment

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

Thanks - I've corrected this in fa72697

Comment on lines 1851 to 1861
ACPhaseSwitchingSupported = "ACPhaseSwitchingSupported"
Available = "Available"
Enabled = "Enabled"
Entries = "Entries"
ExternalControlSignalsEnabled = "ExternalControlSignalsEnabled"
LimitChangeSignificance = "LimitChangeSignificance"
NotifyChargingLimitWithSchedules = "NotifyChargingLimitWithSchedules"
PeriodsPerSchedule = "PeriodsPerSchedule"
Phases3to1 = "Phases3to1"
ProfileStackLevel = "ProfileStackLevel"
RateUnit = "RateUnit"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These aren't snake case.

Copy link
Contributor Author

@Jared-Newell-Mobility Jared-Newell-Mobility Oct 13, 2023

Choose a reason for hiding this comment

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

Thanks - I've corrected this in fa72697

@Jared-Newell-Mobility
Copy link
Contributor Author

@OrangeTux regarding #486 (review) - I used Appendices_CSV_v1.3 dm_components_vars.xlsx - this is referenced in OCPP 2.0.1 part 2 erratta v2.0 section 21.1 - where do you see the discrepancy?

Copy link
Collaborator

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

It's cumbersome task to add this enums, so thanks for the PR!

I didn't finish the review yet, since I found a few enums which are missing variants. That makes me wonder, based on which documents are the enums based? Are they based on the latest errata?

vehicle_id_sensor = "VehicleIdSensor"


class GenericVariableName(str, Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The .zip file containing the OCPP 2.0.1 contains Appendices_CSV_v1.3/variables.csv. This file contains defines 90 variables. This enum, however, contains 69 variants only. For example, AvailabilityState is missing. Can you add the missing variants?

enabled = "Enabled"
life_time = "LifeTime"
policy = "Policy"
storage = "Storage"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't find variant storage in OCPP-2.0.1_part2_appendices_v13.pdf. Is this variant a mistake, or have you seen it listed in another document?

disable_remote_authorization = "DisableRemoteAuthorization"


class CHAdeMOCtrlrVariableName(str, Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Section 3.1.4 of OCPP-2.0.1_part2_appendices_v13.pdf lists more variants. For example, VehicleStatus. Which document did you use to create this enum?

Variable names where the component type is CHAdeMOCtrlr
"""

selftest_active = "SelftestActive"
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/selftest_active/self_test_active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In OCPP 2.0.1 Part 2 Errata v2 - CHAdeMOCtrlr SelftestActive - Self-test is active or self-test is started by setting to
true. - It appears to be hyphenated that is why it is combined as selftest and not self test

@OrangeTux
Copy link
Collaborator

It seems that dm_components_vars.xlsx isn't matching the the written spec and the .csv files.

@Jared-Newell-Mobility
Copy link
Contributor Author

Ok thanks, so for this I will revisit it using the .csv files, document and raise this with the OCA also

@Jared-Newell-Mobility
Copy link
Contributor Author

@OrangeTux - ok, so after contacting the OCA, I have updated this to break the components into logical and physical components, I have also extended the docstrings to document where the variables were sourced from. I have generic variables both as complete (this is to align with the documents provided in appendices_CSV_v1.3.zip, dm_components_vars.csv) and split out for each component. The components have all variables listed in both OCPP 2.0.1 Part 2 Appendices and dm_components_vars.csv, so this results in additional variables.

Hopefully, this makes things a little more clear.

In addition, there is some variable names such as selftest_active_set = "SelftestActive(Set)" and count_height_in_chars = "Count[HeightInChars]" - I am not sure if this is the best way to represent these.

I didn't reply to the individual comments as they may no longer be relevant with the changes.

Looking forward to hear your thoughts.

# Conflicts:
#	CHANGELOG.md
#	ocpp/v201/enums.py
@Jared-Newell-Mobility Jared-Newell-Mobility merged commit b24fade into master Dec 22, 2023
5 checks passed
@Jared-Newell-Mobility Jared-Newell-Mobility deleted the v201/missing-component-and-type-enums-#300 branch December 22, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants