thegridelectric / gridworks-protocol

Message protocol for GridWorks SCADA
MIT License
0 stars 0 forks source link

Jm/channel #346

Closed anschweitzer closed 1 month ago

anschweitzer commented 1 month ago

PR created for ongoing review

anschweitzer commented 1 month ago

A general comment.

  1. I think we should put code like:

    def model_dump(self, **kwargs: dict[str, Any]) -> dict:
        d = super().model_dump(**kwargs)
        d["TelemetryName"] = self.TelemetryName.value
        return d
    
    @classmethod
    def type_name_value(cls) -> str:
        return "data.channel.gt"

    into a base class rather than using code generation to inject them into each class. Here's an example:

class Gt(BaseModel):
    TypeName: Literal["component.gt"] = "component.gt"
    Version: Literal["000"] = "000"

    model_config = ConfigDict(use_enum_values=True)

    @classmethod
    @cached_property
    def model_type_name(cls) -> str:
        return cls.model_config["TypeName"]

Then all the message classes, ComponentGt and CacGt inherit from this. (Confirm model_type_name does what we want). (Also, I thought I had put use_enum_values in, but I don't see it and I'm not sure why the tests are passing on dev.

  1. I don't think we should override model_dump(). I think it should be doing what we want, and if not we should investigatem and in general i don't think it's the recommended approach until you prove to yourself you need it.
anschweitzer commented 1 month ago

More general comment:

  1. I think we should investigate use of config options arbitrary_types_allowed=True and extra="allow". I'm not sure we need any of them except in special cases (e.g. AnyComponent and AnyEvent knows they represent something unrecognized but with a recognized base, so they need extra="allow" to avoid losing fields)?
anschweitzer commented 1 month ago

Additional specific comments:

  1. I don't see why hardware_layout.py would break things. I'll follow up with you later.
  2. Can we add a variant of Batched readings which doesn't have FSM info in it, just as a message? My sense was that the FSM stuff wasn't fully worked out yet, or maybe I don't recall that correctly.
  3. If we did add Batched readings, A simpler structure might be:
    class ChannelReadings(DataChannelGt):
      ChannelId: UUID4Str
      ValueList: List[ReallyAnInt]
      ScadaReadTimeUnixMsList: List[UTCMilliseconds]
      TypeName: Literal["channel.readings"] = "channel.readings"
      Version: Literal["000"] = "000"

Then you could remove DataChannelList from BatchedReadings and you wouldn't have to keep track of indices.

  1. I think we can use StrictInt for ReallyAnInt if we want to.
jessicamillar commented 1 month ago

I took your suggestions re strict int, ChannelReadings and the reduced BatchedReadings. See this pr