search

ottowayi / pycomm3

A Python Ethernet/IP library for communicating with Allen-Bradley PLCs.
MIT License
377 stars 88 forks source link

[FEATURE] - IdentityObject type alias #264

Closed jamesbraza closed 1 year ago

jamesbraza commented 1 year ago

Type of Feature Request

Feature Description

I think it would be cool to add a type alias:

IdentityObject: TypeAlias = dict[str, Any]

Then CIPDriver could use this like so:

class CIPDriver:

    @classmethod
    def discover(cls) -> List[IdentityObject]:
        ...
    @classmethod
    def list_identity(cls, path) -> Optional[IdentityObject]:
        ...

For what it's worth, a feature like this could have prevented the bug in https://github.com/ottowayi/pycomm3/issues/262.

Possible Solution

Additional context

ottowayi commented 1 year ago

Yeah I think the type hinting needs to be improved and this is one way to do it. And if you want to submit a PR for it I'll accept it, but I wouldn't spend a lot of time on it. I'm working on rewriting the library entirely and one of the things I'm changing is making new datatypes for all of the CIP objects, like turning structs from plain dicts to dataclasses. I've been updating the type hints as I go, so I wouldn't want you to waste a bunch of time on something that will get replaced anyway.

jamesbraza commented 1 year ago

Okay I gotchu, I will not make a PR to add the type alias then.

I also feel a type alias for the Identity Object makes it a special case, when it shouldn't be. So I am not convinced this feature request is reasonable. Imo it's best to have one obvious way to do things.

Aside

Since you said you're doing a major rev, I figured I would share these two thoughts.

At my employer, we wanted to use pycomm3 about one year ago, but some motor required implicit messaging so we couldn't. We ended up wrapping some DLL, and next thing we know, .NET had crept into things. It was a total mess! It would be good if pycomm3 supported implicit messaging in the future, as that would make pycomm3 a no-brainer/one-stop-shop for Python + EIP.

One other thing is we don't use any PLCs. We have a multithreaded Python process in a VM or local linux/Windows machine. So when you re-write, maybe make sure you're supporting non-PLC users. I see in CIPDriver's internals it uses the word "PLC" a lot, maybe just put "controller" or something so it's generalized.

ottowayi commented 1 year ago

I'll close this issue for now since I'm already working on it.

So for implicit messaging, I've looked into it a bit, but haven't really done any work to support it. I've been rewriting the protocol implementation from the ground up, part of the reason why is to make it easier to support other connection types. I'm working on making separate cip and enip connections where the cip connection would use the enip one as a transport connection. This way one could make an implicit enip connection to use with the cip one. Then the drivers become simpler too, they just provide features built on the protocol services instead of also implementing the protocol too. And it would allow the connections to be used outside of a driver too.

And yeah, the CIP driver is not nearly generic enough. It basically was cut and paste of methods from the Logix driver, so there's still some PLC bias. I plan on making the connections as close to the protocol as possible and keep things pretty generic, then use the drivers to provide to specific device features.