Skip to content

Behavior and failure for uninitialized network #562

Closed

Description

@sveinse

https://github.com/christiansandberg/canopen/blob/5382f245d8ede61b113c6a648cb99b199667f0cd/canopen/node/local.py#L51-L59

Calling LocalNode.remove_network() or RemoteNode.remove_network() on an already closed network will fail, due to self.network no longer have the .unsubscribe() method (which is expected). Depending on what behavior is wanted, there lacks some functionality with regards to the logic of uninitialized LocalNode. Related to #525

Option 1 - multiple calls to .remove_network() should be ok

If it should be acceptable to call .remove_network() multiple times without failing -> then these methods should support that the network is uninitialized

    def remove_network(self) -> None:
        if self.network is canopen.network._UNINITIALIZED_NETWORK:
            return
        # Rest of the function....

Option 2 - multiple calls to .remove_network() should fail

If a repeated call to .remove_network() should fail or fail on uninitialized network, I think we're missing a function to test if the LocalNode object is uninitialized.

LocalNode and friends it sets the network uninitialized on its __init__() and then later initialize it by .associate_network(). This means that there are code paths through the usage of LocalNode where a cleanup code might need to call .remove_network() conditionally depending on the state of its association with a network.

There is no current way to test if a LocalNode object is associated without using the private canopen.network._UNINITIALIZED_NETWORK object. Moreso, _UNINITIALIZED_NETWORK is inherited from Network so a negative isinstance(node.network, Network) does not help to determine if its initialized.

To prevent access to the private object, I suggest that the best approach would be to add a new .has_network() method to the LocalNode and RemoteNode implementation.

    def has_network(self):
       """Return True if the node has an associated network."""
       return self.network is not canopen.network._UNINITIALIZED_NETWORK

Activity

  1. acolomb commented on Feb 25, 2025

    @acolomb
    Member

    Thanks for pointing this out. I prefer the first option, it does not make much sense to fail on that call.

  2. sveinse commented on Apr 26, 2025

    @sveinse
    CollaboratorAuthor

    While working on this, it was uncovered that calling associate_network() multiple times is not safe either. The Network.subscribe() method happily appends multiple callbacks to its lists. This is addressed in the PR.

  3. sveinse commented on Apr 26, 2025

    @sveinse
    CollaboratorAuthor

    FYI I just discovered that we already have the a method for testing that a node is associated with a network or not:

    def has_network(self) -> bool:
    """Check whether the node has been associated to a network."""
    return not isinstance(self.network, canopen.network._UninitializedNetwork)

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions