Improved channel handling #332
Conversation
Make Notifier support multiple buses Add support for channels in more formats, interfaces, and loggers
| @@ -197,8 +198,12 @@ def on_message_received(self, msg): | |||
| if msg.is_extended_id: | |||
| arb_id += 'x' | |||
|
|
|||
| # Many interfaces start channel numbering at 0 which is invalid | |||
| channel = msg.channel + 1 if isinstance(msg.channel, int) else self.channel | |||
| channel = channel2int(msg.channel) | |||
There was a problem hiding this comment.
Should this always be turned into an int? Because SocketCAN's vcan0/can2 are hard to transform into a int. Same applies for BLF as well. Or is that required by the file format? Because then, it should be noted in the code and/or the class docs, because users might expect that Message 1 -> Writer -> Reader -> Message 2 results in Message 1 == Message 2. That should be added somewhere as a warning/note or whatever.
There was a problem hiding this comment.
Unfortunately the ASC format requires integers (same as BLF). So the channel read back may be different than the one put in. 😞 I still think it's better than not being able to tell the channels apart at all.
There was a problem hiding this comment.
Yep, you are right. Who designs such short-sighted protocols ... ? Well anyways, we should note that somewhere in the docs at least.
can/notifier.py
Outdated
| self._running = False | ||
| timeout = self.timeout + 0.1 if self.timeout is not None else 5 | ||
| for reader in self._readers: | ||
| reader.join(timeout) |
There was a problem hiding this comment.
maybe add a timeout parameter to the method. Additionally, it might be surprising that the timeout counts for each bus. So for five channels and a timeout of one seconds, this method could block 5 seconds although many would expect it to block 1 second.
can/util.py
Outdated
| def channel2int(channel): | ||
| """Try to convert the channel to an integer. | ||
|
|
||
| :param str channel: |
There was a problem hiding this comment.
Maybe we should support arbitrary Python objects here, because the virtual and other future interfaces might use anything as an channel identifier. We could just check for type character string, None, and integral number, and simply use channel = str(channel) or channel = repr(channel) for the rest.
can/notifier.py
Outdated
| :param bus: The :ref:`bus` to listen too. | ||
| :param listeners: An iterable of :class:`~can.Listener`s | ||
| :param timeout: An optional maximum number of seconds to wait for any message. | ||
| :param bus: The :ref:`bus` or list of buses to listen to. |
There was a problem hiding this comment.
I would say any iterable should do.
There was a problem hiding this comment.
It seems to be difficult to determine if a variable is iterable or not in an easy way. I'm not sure when one would need an iterable instead of a list.
| @@ -13,57 +13,62 @@ | |||
|
|
|||
| class Notifier(object): | |||
There was a problem hiding this comment.
We should probably add a test for this class that includes multiple buses & listeners.
Make Notifier support multiple buses
Add support for channels in more formats, interfaces, and loggers