Use canopen with externally provided bus and notifier

[sveinse]

I have a use-case where canopen have to live next to another protocol (using only extendedids). The (physical) can bus interface cannot have multiple instances and the same can.Bus() interface. It and and the can.Notifier() instance must be shared with both protocols. Network.connect() makes it a bit cumbersome to define bus and notifier from the outside.

https://github.com/christiansandberg/canopen/blob/ae71853be7fb42870bb5763066b0c3c50d015669/canopen/network.py#L83-L112

There are several things that can be done here to make it more consistent. I'm interested in hearing what opinions there might be for how to improve this:

Option 1

Be able to support a provided notifier, similar to what's done with bus:

# L111 in network.py
if self.notifier is None:
    self.notifier = can.Notifier(self.bus, [], self.NOTIFIER_CYCLE, **kwargs_notifier)
for listener in self.listeners:
    self.notifier.add_listener(listener)

To use your own:

network = canopen.Network()
network.bus = mybus
network.notifier = mynotifier
network.connect()

Option2

Extend arguments to connect():

bus = kwargs.pop("bus", None)
notifier = kwargs.pop("notifier", None)
if self.bus is None:
    self.bus = bus or can.Bus(*args, *kwargs)
logger.info("Connected to '%s'", self.bus.channel_info)
if self.notifier is None:
    self.notifier = notifier or can.Notifier(self.bus, [], self.NOTIFIER_CYCLE)
for listener in self.listeners:
    self.notifier.add_listener(listener)

This allows the usage:

network = canopen.Network()
network.connect(bus=mybus, notifier=mynotifier)

PS! Network.disconnect() doesn't really play nice with externally provided bus and notifier either and should be looked at.

* EDIT *

I think also the init of Network should be extended:

def __init__(self, bus: Optional[can.BusABC] = None, notifier: Optional[can.Notifier] = None):

[acolomb]

Still haven't quite wrapped my head around what approach makes most sense here. I do see the possible need to provide your existing implementation / instance for the bus. Not sure what the requirements are for the notifier, and why that should be required to be a single instance.

In general, I think having a clearly defined point when these things are assigned, to be important. For the self.bus we currently allow passing it during construction. Or it can be overridden by direct attribute access, but before .connect() is first called. That method lazily initializes a missing bus instance at that time.

Providing bus and notifier as keyword arguments to .connect() opens up a third way to manipulate these attributes. I'd like to avoid proliferating these, to keep the code focused on a recommended usage pattern. So these are the main questions here, IMHO:

1. Why can there be only a single Notifier instance? Is there something special about your use-case that requires this? Or is it a design choice in python-can?
2. When we allow passing in a custom notifier object, we have a timing conflict. The .bus might be initialized as late as .connect(), therefore we cannot always produce a working notifier before that (such as in __init__()). But preferably both attributes should be provided in conjunction, that would mean adding an optional notifier argument to the constructor.

In my opinion, the direct attribute access (Option 1) makes more sense, for the reasons outlined above. Thus the only needed change would be to avoid overwriting a preset .notifier attribute. But I'd first like to clarify the first question, where maybe we can still assume the .notifier to be managed by the class itself, without external intervention.

[sveinse]

can.Bus and can.Notifier are resources that cannot have multiple instances unfortunately. The bus is often not shareable because many CAN adapters doesn't allow multiple connections to it. The notifier is a bit more subtle, but the reception running in a thread is done by the notifier, not the bus. This means that with two notifiers there will be two threads. Due to how the the bus reception is done (see https://github.com/hardbyte/python-can/blob/main/can/bus.py#L110), only one will get the message.

The following code demonstrates that it doen't work with multiple notifiers. If they where truly independent every message received on the bus should be delivered to each of the independent listener. But it itsn't. Only the first in line gets the message, which can be seen on the output. And for the record, this example being async has no impact. Set loop=None and the same result is given.

class MyListener(can.Listener):

    def __init__(self):
        print(f"Listener {id(self)} created")

    def on_message_received(self, msg):
        print(f"{id(self)} Message received: {msg}")

async def main():

    loop = asyncio.get_event_loop()

    bus = can.Bus(interface='pcan', bitrate=1000000)
    listen1 = MyListener()
    notifier1 = can.Notifier(bus, [listen1], loop=asyncio.get_event_loop())

    listen2 = MyListener()
    notifier2 = can.Notifier(bus, [listen2], loop=asyncio.get_event_loop())

    await asyncio.sleep(3600)

Which produce

22:29:04.983  DEBUG  [can]  can config: {'bitrate': 1000000, 'interface': 'pcan', 'channel': None}
Listener 2545276566032 created
Listener 2545289674832 created
2545276566032 Message received: Timestamp: 1738531784.332148    ID:      702    S Rx                DL:  1    05
2545276566032 Message received: Timestamp: 1738531784.332206    ID:      702    S Rx                DL:  1    05
2545289674832 Message received: Timestamp: 1738531787.845614    ID:      702    S Rx                DL:  1    05
2545289674832 Message received: Timestamp: 1738531792.842708    ID:      702    S Rx                DL:  1    05
2545289674832 Message received: Timestamp: 1738531797.839500    ID:      702    S Rx                DL:  1    05

So this is the long rationale for why a system running multiple can protocols must share can.Bus and can.Notifier. Unfortunately, because I agree, having clean interfaces is the big want.

[sveinse]

FYI for inspiration: It was urgently needed in the async port, so I've implemented the following:

https://github.com/sveinse/canopen-asyncio/blob/46f9b4a01997dc099fb85789f3e7534f3ab7b634/canopen/network.py#L33-L48

https://github.com/sveinse/canopen-asyncio/blob/46f9b4a01997dc099fb85789f3e7534f3ab7b634/canopen/network.py#L121-L128

[ericbaril72]

On the same can Bus, I use a mix of CANopen, J1939 and raw-can.

I do not use the "connect" or "disconnect" methods ( also used in the J1939 library ).
Have a look at the can.Notifier class... it handles a "List" of listeners

SO, your "network bus" needs to be added or removed to the can.listeners list rather then "connected"/"disconnected".

when the can interface thread have a message available: it will call each "listerners" ( canopen / j1939 /rawcan ) you added
def _on_message_received(self, msg: Message) -> None:
for callback in self.listeners:
res = callback(msg)

[sveinse]

This is precisely the main point of this issue. canopen is creating a new notifier from its Network.connect() and sets up its own listeners. In the system I'm working with, a bus, a notifier and listeners for another protocol is already in place, so when the canopen network is created, it should (be able to) use the provided ones, not create new.

Edit:

Network.connect() is the only method available that will setup the necessary canopen listeners to the notifier. Canopen needs this to work.

[ericbaril72]

I understand your point of view and I agree, it would be awesome !
BUT I am sure the library started from the goal of using CANopen

Your in-depth understanding of the underlaying issue makes me believe you would like to propose a Pull-request and wanted to know if Christian or some other hardcore user of this library would be willing to take over the maintenance of this new "Layer".

Do I understand you well ? I saw from your repos you are getting your hands dirty on an asyncio spin-off ...

If yes, I would be more then interested in using your "Branch". Possibly testing it and providing feedback( Pull-request ) to address some of the issues that will arise. I am not as well versed on multi-threading BUT I always end-up having to adjust the libraries I use.

MY TARGET USE-CASE: Bus sniffing/decoding & emulating.

In the mean time, I replaced the CANopen.network Listener with my own ...
when the incoming can mesage is extended, I notify J1939.ecu, otherwise, i notify canopen.network.

Looking forward for a sveineBus interface layer

[sveinse]

Let's look at this for 3.0.0

[acolomb]

Revisiting this, I think it can be handled pretty easily without API changes.

We keep it in Network.connect(), where the notifier is currently created unconditionally. For a custom notifier, set the .notifier attribute externally between __init__() and connect(). If already set to not-None, we use the pre-provided notifier and add our listeners to it. If it doesn't use the same bus instance, we add the bus as well.

Sounds about right? I can make a PR with that proposal if you think it's sensible.

Note that extending the API is not preferable in this case IMHO, because it is a very niche use-case and we can get away with much lower complexity by delaying it until after the __init__() method. Starting a thread (which might happen in Notifier.__init__()) is better suited to happen in connect() IMHO, unless the notifier was already created externally.