Repository navigation
Use canopen with externally provided bus and notifier #556
Description
Activity
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.buswe 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:- Why can there be only a single
Notifierinstance? Is there something special about your use-case that requires this? Or is it a design choice in python-can? - When we allow passing in a custom notifier object, we have a timing conflict. The
.busmight 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
.notifierattribute. But I'd first like to clarify the first question, where maybe we can still assume the.notifierto be managed by the class itself, without external intervention.- Why can there be only a single
can.Busandcan.Notifierare 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=Noneand 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 05So this is the long rationale for why a system running multiple can protocols must share
can.Busandcan.Notifier. Unfortunately, because I agree, having clean interfaces is the big want.FYI for inspiration: It was urgently needed in the async port, so I've implemented the following:
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 listenersSO, 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)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.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 CANopenYour 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
Let's look at this for 3.0.0
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.notifierattribute externally between__init__()andconnect(). 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 inNotifier.__init__()) is better suited to happen inconnect()IMHO, unless the notifier was already created externally.
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 thecan.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:To use your own:
Option2
Extend arguments to
connect():This allows the usage:
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
Networkshould be extended: