Add constants for configuration key names

[lbbrhzn]

trafficstars

Pleae add constants for configuration key names and measurands. i.e:

heartbeat_interval = "HeartbeatInterval"

current_import = "Current.Import"

This prevents errors due to typing mistakes in the strings.

[tropxy]

I am afraid you have overlooked this section, mate: https://github.com/mobilityhouse/ocpp/blob/a6f57307d2a8a1c8f85f802742b5855ccf48756d/ocpp/v16/enums.py#L436

[lbbrhzn]

Ah, great, so the measurands are there. What about the configuration keys?

[tropxy]

Those are missing, you are right. Our types are acquired directly from the schemas and those Enums do not appear in the schemas, so they are not automatically added.

But, there are just a few configuration keys and would be relatively simple to add them at least for ocpp1.6. What do you think about opening a small PR and contribute to our repo? We would really appreciate it

[lbbrhzn]

i added pull request #231

[lbbrhzn]

How about field names of a request or confirmation payload? I would like to do something like:

@on(Action.BootNotification)
def on_boot_notification(self, **kwargs):
   device_registry.async_get_or_create(
      model = kwargs.get(BootNotification.conf.charge_point_model,None),
      manufacturer = kwargs.get(BootNotification.conf.charge_point_vendor, None),
  )

instead of:

@on(Action.BootNotification)
def on_boot_notification(self, **kwargs):
   device_registry.async_get_or_create(
      model = kwargs.get("charge_point_model",None),
      manufacturer = kwargs.get("charge_point_vendor", None),
  )

[OrangeTux]

I see the issue you're having. It's not very ergonomic having to use strings to look up data in kwargs. However, I'm not sure if I like your suggestion.

What do you think of a solution like this:

@on(Action.BootNotification)
def on_boot_notification(self, **kwargs):
   payload = call.BootNotificationPayload.from_dict(kwargs):
   device_registry.async_get_or_create(
      model = payload.charge_point_model,
      manufacturer = payload.charge_point_vendor,
  )

Or this slight variant on the above solution:

def on_boot_notification(self, **kwargs):
   payload = payload_from_dict(call.BootNotificationPayload, kwargs):
   device_registry.async_get_or_create(
      model = payload.charge_point_model,
      manufacturer = payload.charge_point_vendor,
  )

[lbbrhzn]

It would be great if a lint tool would be able to detect a typo. e.g. payload.charge_pont_model. It looks like the first variant could work that way, but the second variant not. Correct?

[lbbrhzn]

Also, if the charge_point_model is not in the payload, will payload.charge_point_model default to None?

[lbbrhzn]

Wonder if it would be an option to do this:

@on(Action.BootNotification)
def on_boot_notification(payload:BootNotificationPayload):
    device_registry.async_get_or_create(
        model = payload.charge_point_model,
        manufacturer = payload.charge_point_vendor,
    )

[tropxy]

I was actually thinking about this topic of handling the payloads of the data structure and I came across, recently, with this lib called pydantic, which includes a lot of interesting features. I also checked the footprint of the code base and it looks quite small. https://github.com/samuelcolvin/pydantic

For example, for a complex scenario like the Device Model of OCPP 2.0.1, this lib can actually be useful. Here is an example on how to use it to construct the SetVariablesRequest:

from pydantic import BaseModel as PydanticBaseModel, Field, constr
from typing import Dict, Any, List, Optional



class BaseModel(PydanticBaseModel):
    class Config: 
        """Allow input by alias or field name."""
        allow_population_by_field_name = True

    # this overrides the dict method, avoiding us to write by_alias=True
    # all the time
    def dict(
        self,
        *,
        by_alias: bool = True,
        exclude_none: bool = True,
        **kwargs,
    ) -> Dict[str, Any]:
        return super().dict(by_alias=by_alias,
                            exclude_none=exclude_none,
                            **kwargs)


class CustomDataType(BaseModel):
     """Generic type that allows vendor specific data into the messages."""

     vendor_id: constr(max_length=255) = Field(..., alias="vendorId")



class ComponentType(BaseModel):
     """Component base.

     A physical or logical component.

     According with OCPP 2.0.1: Part2 - Specification
     Section: Messages, Datatypes & Enumerations
     2. Datatypes
     2.16. ComponentType
     """

     name: str = Field(
         ..., max_length=50
     )

     class Config:  # noqa: WPS431
         """Internal config so we can use classes from Mongo directly."""
         allow_population_by_field_name = True
         orm_mode = True
         validate_assignment = True


class VariableType(BaseModel):
     """VariableType.

     According with OCPP 2.0.1: Part2 - Specification
     Section: Messages, Datatypes & Enumerations
     2. Datatypes
     2.53. VariableType

     """

     name: str = Field(
         ..., max_length=50,
     )


class SetVariableDataType(BaseModel):
     """SetVariableDataType.

     According with OCPP 2.0.1: Part2 - Specification
     Section: Messages, Datatypes & Enumerations
     2. Datatypes
     2.44. SetVariableDataType

     """

     custom_data: Optional[CustomDataType] = Field(None, alias="customData")
     attr_type: Optional[str] = Field(None, alias="attributeType")
     attr_value: str = Field(..., alias="attributeValue", max_length=1000)
     component: ComponentType
     variable: VariableType


class SetVariablesRequest(BaseModel):
     """SetVariablesRequest.

     This contains the field definition of the SetVariablesRequest PDU sent by
     the CSMS to the Charging Station.

     According with OCPP 2.0.1: Part2 - Specification
     Section: Messages, Datatypes & Enumerations
     1. Messages
     1.57. SetVariables
     1.57.1. SetVariablesRequest

     """

     custom_data: Optional[CustomDataType] = Field(None, alias="customData")
     set_variable_data: List[SetVariableDataType] = Field(
         ..., alias="setVariableData", min_items=1,
     )


var_list = [SetVariableDataType(attr_type="Target", attr_value="10", component=ComponentType(name="TxCtrlr"), variable=VariableType(name="EvConnectionTimeout"))]

set_req = SetVariablesRequest(set_variable_data=var_list)

In [24]: set_req.dict()
Out[24]:
{'setVariableData': [{'attributeType': 'Target',
   'attributeValue': '10',
   'component': {'name': 'TxCtrlr'},
   'variable': {'name': 'EvConnectionTimeout'}}]}

Pydantic can also be used to perform validation and includes a json parser as well. It can also be used to generate schemas.

This would extend our dataclasses and allow to detail, correctly, each Message payload, instead of having a generic description like, for exmaple, SetChargingProfile:

@dataclass
class SetChargingProfilePayload:
    connector_id: int
    cs_charging_profiles: Dict

Whose cs_charging_profiles is only defined as a dict, but without checking the docs, we dont know what that dict must contain. This way, we can address each field...

[lbbrhzn]

@tropxy, that looks quite useful! Would it be possible to generate the pedantic models straight from the specification? For instance, using pdf parser

[tropxy]

yup, I believe, it is. but for this case we still use the schemas right?

Or are you referring to the configuration keys?

Colleagues of mine have used this parser: https://github.com/euske/pdfminer

[OrangeTux]

It seems that the above discussion tries to address several different issues:

1. Avoid having to use strings to look up keys in the kwargs dict of request handlers. This is error prone and not very ergonomic.

I like the suggestion of @lbbrhzn:

@on(Action.BootNotification)
def on_boot_notification(payload:BootNotificationPayload):
    device_registry.async_get_or_create(
        model = payload.charge_point_model,
        manufacturer = payload.charge_point_vendor,
    )

We can keep supporting the current way of implementing request. Along side we could implement this method. I think the proposed solution makes it easier to figure out what data is passed to a handler.

2. Introduce types to model complex objects like CsChargingProfiles. Currently users have to model these objects using dictionaries. It's unclear how this dictionary have to look like. Creating such dict is prone to error and not very ergonomic.

I totally agree we can improve on this.

3. Implement a stricter validation of OCPP messages.

I'm in doubt about this. We created this project with correctness in mind. Adding stricter validation improves correctness and that is why I like this idea. But I think we also should keep backwards compatibility in mind. I don't think we should force this stricter validation on our users. At least not too fast. We should make stricter validation at least optional for a while and raise some warning before we enforce it.

That being said, I'm not a fan of @tropxy 's proposal. It's API is very verbose and it changes the API of this project. I would like to explore an implementation that complies with the current API.

[tropxy]

I do agree, it is a rather verbose solution. But for super nested messages like SetVariablesRequest or SetChargingProfile, if you want to have a flexible constructor of those messages, where you can address each nested field, I am not sure if you can do it without being verbose

[Jared-Newell-Mobility]

Considering the original purpose for this issue has been resolved, and that the discussion around the following enhancements has gone stale, I'll close this for now