-
Notifications
You must be signed in to change notification settings - Fork 339
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
feature/141, OCPP 1.6 Security White Paper Ed 2 #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR. I've some suggestion for improvements.
I don't like the suffix Type
to all enums. although it's according to the specification. So I'm not sure yet if we should keep it or remove it. I'll come back to you regarding this topic.
I've made the following changes:
|
ocpp/v16/enums.py
Outdated
class ExtendedTriggerMessageStatus(str, Enum): | ||
""" | ||
TriggerMessageStatusEnumType is used by: ExtendedTriggerMessage.conf | ||
""" | ||
|
||
accepted = "Accepted" | ||
rejected = "Rejected" | ||
not_implemented = "NotImplemented" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already exists as TriggerMessageStatus, so I dont think we need to repeat it with a different name. Also in the whitepaper is defined as "TriggerMessageStatusEnumType" and I think we should stick with the whitepaper terms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point as values are the same there's no point duplicating this type.
ocpp/v16/enums.py
Outdated
class ExtendedMessageTrigger(str, Enum): | ||
""" | ||
MessageTriggerEnumType is used by: ExtendedTriggerMessage.req | ||
""" | ||
|
||
boot_notification = "BootNotification" | ||
log_status_notification = "LogStatusNotification" | ||
firmware_status_notification = "FirmwareStatusNotification" | ||
heartbeat = "Heartbeat" | ||
meter_values = "MeterValues" | ||
sign_charge_point_certificate = "SignChargePointCertificate" | ||
status_notification = "StatusNotification" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need to create a new Status for the Extended Trigger. We can just add the extra fields that are added by the whitepaper and add a comment mentioning it. This will then be in accordance with the Whitepaper which also doesnt change the name of the enum "TriggerMessageStatusEnumType"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make that change also.
I will also sort payload classes in call.py and call_result.py based on communication direction as are previous payloads defined. Also enums.py is using alphabetical sorting so I'll follow that as well. |
I made suggested/requested changed (hopefully all this time) and also rebased against master. |
… security for OCPP 1.6-J specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, to me ;)
@OrangeTux @tropxy When are you planning to create next release that will include this PR? |
@OrangeTux @tropxy Is there something still regarding this PR that you would like to see changed? When are you planning to include this in release? |
As I said last time, for me this PR seems complete. There are some lack of new lines in the schema files, but that is not a blocker for me at least. @OrangeTux needs to take an action here and check if he confirms that the PR is ready to merge. Without his feedback, your PR cant be merged |
Improved Security for OCPP 1.6-J specification (https://www.openchargealliance.org/news/enhanced-security-for-ocpp-16/) adds few messages backported from 2.0 to 1.6. Some messages have been renamed to prevent conflict with original 1.6 messages. Also some schemas have modified content. Open Charge Alliance hasn't released official backported schema files.
Changes to mobilityhouse/ocpp:
This PR is a fix for #141.