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

Network instance is optional in many classes, which requires lot of checking #511

Closed
sveinse opened this issue Jul 10, 2024 · 4 comments · Fixed by #525
Closed

Network instance is optional in many classes, which requires lot of checking #511

sveinse opened this issue Jul 10, 2024 · 4 comments · Fixed by #525

Comments

@sveinse
Copy link
Collaborator

sveinse commented Jul 10, 2024

With reference to #509 - lots of classes depend on an instance of class Network and have it as an attribute in order to communicate with the CAN bus. Examples are SdoBase(), PdoBase(), NmtBase() and others. Their constructor does not require a Network argument so the formal type of the attribute is network: Optional[Network] = None.

These attributes are set when a RemoteNode() is created and the reference to the network is injected from RemoteNode.associate_network().

The effect of this is that we'll need a lot of checks all over the code to ensure network is properly set. E.g. as here from class EmcyProducer:

    def send(self, code: int, register: int = 0, data: bytes = b""):
        payload = EMCY_STRUCT.pack(code, register, data)
        if self.network is None:   # This is needed to not fail the send_message line
            raise RuntimeError("A Network is required")
        self.network.send_message(self.cob_id, payload)

Is is a requirement to be able to create these class instances without a functional Network? We could make a default Network-like object that will emulate network operations if we need it to work without an actual CAN bus network.

What do you think we should do here?

@erlend-aasland
Copy link
Contributor

IMO, the better thing to do would be to improve the documentation to put emphasis on always using Network.add_node and Network.create_node1 to create nodes.

BTW, are there use-cases for subclassing the *Base classes you mention?

We could make a default Network-like object that will emulate network operations if we need it to work without an actual CAN bus network.

I would instead prefer keeping the status quo.

Footnotes

  1. It is slightly confusing that add_node and create_node are used for creating remote and local nodes respectively. Something like create_remote_node and create_local_node would be clearer.

@sveinse
Copy link
Collaborator Author

sveinse commented Jul 10, 2024

I would instead prefer keeping the status quo.

I've added all the runtime checks in PR #513 to ensure the None case of Optional[Network] is properly handled. We could avoid all that if Network weren't Optional. But in many ways, I agree because having as it is today requires less intrusive changes to the codebase.

@acolomb
Copy link
Collaborator

acolomb commented Aug 7, 2024

Picking up my comment on #513... After some prototyping, I think Protocol will not really help us here. We could still implement a canopen.network._DummyNetwork class that basically throws targeted exceptions when any methods are actually used. That allows proper static type checking, while punting off error messages to run-time. Which is okay, because it depends on the correct run-time usage of the API (add to a network before using the node object) and a static checker will never be able to judge that. The type annotation on the attribute would be simply Network then, initialized with the dummy object whose class inherits from Network but overrides the constructor to not initialize all the attributes.

Would that be a feasible solution? It would require minimal changes only and not touch the "hot" code paths.

@acolomb
Copy link
Collaborator

acolomb commented Aug 7, 2024

See #525 for a small proof of concept. I will extend that one to cover more instances when the approach is deemed acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants