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

sveinse · 2024-07-10T09:56:59Z

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?

The text was updated successfully, but these errors were encountered:

erlend-aasland · 2024-07-10T10:44:12Z

IMO, the better thing to do would be to improve the documentation to put emphasis on always using Network.add_node and Network.create_node¹ 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.

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 · 2024-07-10T11:53:34Z

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 · 2024-08-07T07:29:26Z

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 · 2024-08-07T08:05:03Z

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

friederschueler added the discussion label Jul 10, 2024

sveinse mentioned this issue Jul 10, 2024

Annotate Network and check usage of it runtime #513

Open

erlend-aasland mentioned this issue Jul 10, 2024

Add mypy support and fixup project to give no errors #512

Open

acolomb linked a pull request Aug 7, 2024 that will close this issue

Add a dummy object to designate "uninitialized" networks (fixes #511) #525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Comments

Footnotes