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

feat: Matching OCPP 2.0.1 payload types to OCPP 2.0.1 json schema #680

Merged

Conversation

ajmirsky
Copy link
Contributor

@ajmirsky ajmirsky commented Nov 22, 2024

This brings @malsyned #562 up-to-date with base branch and fixes linting errors.

Justification

Payload types now match the json schema for OCPP2.0.1 that are also included in this library. allows for static type checking of payloads.

Breaking changes

To verify that there are no breaking changes, the unit test included in #684 is used to prevent regression with the nested model datatypes.

from ocpp.v201.call import BootNotification

def process_boot_notification(payload: BootNotification):
    vendor_name = payload.charging_station['vendor_name']


payload =  json.loads('{"charging_station": {"vendor_name": "OC", "model": "OC-X", "modem": {"iccid": "1234567890", "imsi": "1234567890"}, "serial_number": "123456", "firmware_version": "1.0.0"}, "reason": "PowerUp", "custom_data": null}')

boot_notification_payload = BootNotification(**payload)
type(boot_notification_payload.charging_station)  # <class 'dict'>

process_boot_notification(boot_notification_payload)

To hydrate with nested datatypes, use json-dataclasses

@ajmirsky
Copy link
Contributor Author

ajmirsky commented Nov 26, 2024

Included same unit test for v2.0.1 datatypes and enums as #684 to make sure that nested model type changes don't alter functionality.

@jainmohit2001
Copy link
Collaborator

@on(Action.TransactionEvent)
def on_transaction_event(self, transaction_info: dict, *args, **kwargs):
    transaction_id = transaction_info.get("transaction_id", None)
    # I can access all the data inside transaction_info using the `.get` method.
    meter_value = kwargs.get("meter_value", [])
    # I can access meter values using the same .get method.

My question is, whether the following will work after the changes in dataclasses?

@on(Action.TransactionEvent)
def on_transaction_event(self, transaction_info: TransactionEvent, *args, **kwargs):
    transaction_id = transaction_info.get("transaction_id", None)
    meter_value = kwargs.get("meter_value", [])

@jainmohit2001
Copy link
Collaborator

NVM. The question is resolved.
Upon carefully inspecting the _handle_call function in charge_point.py, it's clear that the data passed to @on handlers is simply dictionary. We don't convert the incoming data to data classes here.

@ajmirsky
Copy link
Contributor Author

@jainmohit2001 yup. the type is just a hint in python; it won't cast the object to a new type. so as long as you interact with the transaction_info as if it were a dict, then you're ok.

The change you're describing can be made now, without the dataclass changes in this pull request, since TransactionEvent already exists.

@on(Action.TransactionEvent)
def on_transaction_event(self, transaction_info: TransactionEvent, *args, **kwargs):
    transaction_id = transaction_info.get("transaction_id", None)
    meter_value = kwargs.get("meter_value", [])

Before or after this pull request, a static type checker or IDE should show a warning. With the type hint of TransactionEvent, the checkers will expect you to access transaction_info not with get but by accessing the attribute directly: transaction_info.transaction_id. But, as you point out, charge_point.py still sends as a dict.

@ajmirsky
Copy link
Contributor Author

ajmirsky commented Dec 6, 2024

@jainmohit2001 now that your question has been answered, will this PR be reviewed/approved for inclusion in the next release?

@ajmirsky ajmirsky mentioned this pull request Dec 8, 2024
@jainmohit2001 jainmohit2001 changed the title Matching OCPP 2.0.1 payload types to OCPP 2.0.1 json schema feat: Matching OCPP 2.0.1 payload types to OCPP 2.0.1 json schema Dec 9, 2024
…calEnergeStorageVariableName, ConnectedEVVariableName, StandardizedVariableName, InvalidMessageSeq, Bytes
@ajmirsky
Copy link
Contributor Author

Standardized units of measure

To maintain consistency with the OCPP nomenclature, we need to make the following changes:

  • Rename UnitOfMeasureType to StandardizedUnitsofMeasureEnumType

There is a UnitOfMeasureType which is a datatype (2.49). This datatype has a field unit which is of type StandardizedUnitsofMeasureEnumType (based on Appendix 2 of OCPP 2.0.1 Part 2).

  • Re order the declaration of fields and match them with OCPP docs

everything, i believe, matches the json schema; which comes directly from OCPP. so I think we're ok if we just follow the ordering of the schema

  • Fix values for entries:

    • b: Bytes -> B

changed

StatusInfoReasonType

  • Fix the values for the following entries:

    • invalid_message_sequence: InvalidMessageSequence -> InvalidMessageSeq

changed

GenericVariableName

  • We should rename GenericVariableName to StandardizedVariableName

changed

  • Add entry for ISO15118EvseId right after Interval

added

TxCtrlrVariableName

  • Additional entry present for ChargingTime

removed

ChargingStationVariableName

  • Additional entry present for Identity

removed

ConnectedEVVariableName

  • A lot of missing values from 3.2.13. ConnectedEV and additional entries which are not part of official OCPP list of variable names.

updated

LocalEnergyStorageVariableName

  • Additional entry present for Capacity

removed

@ajmirsky
Copy link
Contributor Author

@jainmohit2001 thanks for the careful review and double check of things. please let me know if I've missed anything else.

@jainmohit2001
Copy link
Collaborator

Thanks for updating the PR quickly. I'll be reviewing the whole PR once again soon.

Copy link
Collaborator

@jainmohit2001 jainmohit2001 left a comment

Choose a reason for hiding this comment

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

Added my review for enums and datatypes.
Will take up call and call_result next.

ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jainmohit2001 jainmohit2001 left a comment

Choose a reason for hiding this comment

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

Completed review for call and call_result

ocpp/v201/call_result.py Outdated Show resolved Hide resolved
ocpp/v201/call_result.py Outdated Show resolved Hide resolved
@jainmohit2001
Copy link
Collaborator

@ajmirsky You have included the tests for data types in this PR and in #684 as well. I think we can close #684 and keep the tests in this PR itself.

@jainmohit2001
Copy link
Collaborator

Hi @ajmirsky, please look at my comments whenever you get the time. I think we are good to go for this PR apart from the comments I have mentioned.

ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
ocpp/v201/enums.py Outdated Show resolved Hide resolved
@ajmirsky
Copy link
Contributor Author

Hi @ajmirsky, please look at my comments whenever you get the time. I think we are good to go for this PR apart from the comments I have mentioned.

Apologies for the delay. They should now be resolved.

@ajmirsky
Copy link
Contributor Author

@ajmirsky You have included the tests for data types in this PR and in #684 as well. I think we can close #684 and keep the tests in this PR itself.

closed #684

@ajmirsky
Copy link
Contributor Author

@jainmohit2001 since we're removing the deprecated enums, should we also be removing the deprecated call and call_result Payloads? Or is that scheduled for a future release?

@jainmohit2001
Copy link
Collaborator

@jainmohit2001 since we're removing the deprecated enums, should we also be removing the deprecated call and call_result Payloads? Or is that scheduled for a future release?

A part of that is being done in #694

@jainmohit2001
Copy link
Collaborator

@proelke, have a look at the comments in this PR and share your thoughts. I have gone through the changes thoroughly and we are good for merge.

@proelke
Copy link
Collaborator

proelke commented Dec 18, 2024

My only feedback would be the use of importing the module and not each class. This diverges from how it is done for the 1.6 code. I'm personally more of a fan how it's done in the 1.6 code base vs what this PR introduces but they should be consistent.

@jainmohit2001
Copy link
Collaborator

My only feedback would be the use of importing the module and not each class. This diverges from how it is done for the 1.6 code. I'm personally more of a fan how it's done in the 1.6 code base vs what this PR introduces but they should be consistent.

Considering the fact that OCPP 2.0.1 have a large number of datatypes and enums, importing each class instead of the whole module would only bring long import lines. I agree that we should be consistent in imports. We can create a separate PR for that and update the OCPP 1.6 imports if that works.

@proelke proelke merged commit 23c8e8d into mobilityhouse:master Dec 18, 2024
8 checks passed
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.

5 participants