From 56262ea403b2803ec82e3eaf1130a9fe45fab9f1 Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Fri, 6 Sep 2024 08:56:51 +0200 Subject: [PATCH 1/9] Add autostart option (kwarg) to BusABC.send_periodic() to fix issue #1848 --- can/broadcastmanager.py | 4 +++- can/bus.py | 11 ++++++++++- can/interfaces/ixxat/canlib.py | 3 ++- can/interfaces/ixxat/canlib_vcinpl.py | 2 ++ can/interfaces/ixxat/canlib_vcinpl2.py | 2 ++ can/interfaces/socketcan/socketcan.py | 7 ++++++- 6 files changed, 25 insertions(+), 4 deletions(-) diff --git a/can/broadcastmanager.py b/can/broadcastmanager.py index 0fa94c82c..bc65d3a47 100644 --- a/can/broadcastmanager.py +++ b/can/broadcastmanager.py @@ -276,6 +276,7 @@ def __init__( period: float, duration: Optional[float] = None, on_error: Optional[Callable[[Exception], bool]] = None, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> None: """Transmits `messages` with a `period` seconds for `duration` seconds on a `bus`. @@ -324,7 +325,8 @@ def __init__( stacklevel=1, ) - self.start() + if autostart: + self.start() def stop(self) -> None: self.stopped = True diff --git a/can/bus.py b/can/bus.py index 954c78c5f..7ddb439bc 100644 --- a/can/bus.py +++ b/can/bus.py @@ -215,6 +215,7 @@ def send_periodic( period: float, duration: Optional[float] = None, store_task: bool = True, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> can.broadcastmanager.CyclicSendTaskABC: """Start sending messages at a given period on this bus. @@ -237,6 +238,9 @@ def send_periodic( :param store_task: If True (the default) the task will be attached to this Bus instance. Disable to instead manage tasks manually. + :param autostart: + If True (the default) the sending task will immediately start after creation. + Otherwise, the task has to be started by calling start() on it. :param modifier_callback: Function which should be used to modify each message's data before sending. The callback modifies the :attr:`~can.Message.data` of the @@ -272,7 +276,7 @@ def send_periodic( # Create a backend specific task; will be patched to a _SelfRemovingCyclicTask later task = cast( _SelfRemovingCyclicTask, - self._send_periodic_internal(msgs, period, duration, modifier_callback), + self._send_periodic_internal(msgs, period, duration, autostart, modifier_callback), ) # we wrap the task's stop method to also remove it from the Bus's list of tasks periodic_tasks = self._periodic_tasks @@ -299,6 +303,7 @@ def _send_periodic_internal( msgs: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> can.broadcastmanager.CyclicSendTaskABC: """Default implementation of periodic message sending using threading. @@ -312,6 +317,9 @@ def _send_periodic_internal( :param duration: The duration between sending each message at the given rate. If no duration is provided, the task will continue indefinitely. + :param autostart: + If True (the default) the sending task will immediately start after creation. + Otherwise, the task has to be started by calling start() on it. :return: A started task instance. Note the task can be stopped (and depending on the backend modified) by calling the @@ -328,6 +336,7 @@ def _send_periodic_internal( messages=msgs, period=period, duration=duration, + autostart=autostart, modifier_callback=modifier_callback, ) return task diff --git a/can/interfaces/ixxat/canlib.py b/can/interfaces/ixxat/canlib.py index 330ccdcd9..a1693aeed 100644 --- a/can/interfaces/ixxat/canlib.py +++ b/can/interfaces/ixxat/canlib.py @@ -155,10 +155,11 @@ def _send_periodic_internal( msgs: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> CyclicSendTaskABC: return self.bus._send_periodic_internal( - msgs, period, duration, modifier_callback + msgs, period, duration, autostart, modifier_callback ) def shutdown(self) -> None: diff --git a/can/interfaces/ixxat/canlib_vcinpl.py b/can/interfaces/ixxat/canlib_vcinpl.py index 709780ceb..c370bf5ac 100644 --- a/can/interfaces/ixxat/canlib_vcinpl.py +++ b/can/interfaces/ixxat/canlib_vcinpl.py @@ -793,6 +793,7 @@ def _send_periodic_internal( msgs: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> CyclicSendTaskABC: """Send a message using built-in cyclic transmit list functionality.""" @@ -821,6 +822,7 @@ def _send_periodic_internal( msgs=msgs, period=period, duration=duration, + autostart=autostart, modifier_callback=modifier_callback, ) diff --git a/can/interfaces/ixxat/canlib_vcinpl2.py b/can/interfaces/ixxat/canlib_vcinpl2.py index 18b3a1e57..edb872781 100644 --- a/can/interfaces/ixxat/canlib_vcinpl2.py +++ b/can/interfaces/ixxat/canlib_vcinpl2.py @@ -937,6 +937,7 @@ def _send_periodic_internal( msgs: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> CyclicSendTaskABC: """Send a message using built-in cyclic transmit list functionality.""" @@ -967,6 +968,7 @@ def _send_periodic_internal( msgs=msgs, period=period, duration=duration, + autostart=autostart, modifier_callback=modifier_callback, ) diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py index 03bd8c3f8..10a726f0e 100644 --- a/can/interfaces/socketcan/socketcan.py +++ b/can/interfaces/socketcan/socketcan.py @@ -813,6 +813,7 @@ def _send_periodic_internal( msgs: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, + autostart: bool = True, modifier_callback: Optional[Callable[[Message], None]] = None, ) -> can.broadcastmanager.CyclicSendTaskABC: """Start sending messages at a given period on this bus. @@ -823,13 +824,16 @@ def _send_periodic_internal( :class:`CyclicSendTask` within BCM provides flexibility to schedule CAN messages sending with the same CAN ID, but different CAN data. - :param messages: + :param msgs: The message(s) to be sent periodically. :param period: The rate in seconds at which to send the messages. :param duration: Approximate duration in seconds to continue sending messages. If no duration is provided, the task will continue indefinitely. + :param autostart: + If True (the default) the sending task will immediately start after creation. + Otherwise, the task has to be started by calling start() on it. :raises ValueError: If task identifier passed to :class:`CyclicSendTask` can't be used @@ -868,6 +872,7 @@ def _send_periodic_internal( msgs=msgs, period=period, duration=duration, + autostart=autostart, modifier_callback=modifier_callback, ) From efb0f942531b1330b824147996060034cb274ce8 Mon Sep 17 00:00:00 2001 From: Tomas Bures Date: Fri, 6 Sep 2024 13:05:45 +0200 Subject: [PATCH 2/9] Fix for #1849 (PCAN fails when PCAN_ERROR_ILLDATA is read via ReadFD) (#1850) * When there is an invalid frame on CAN bus (in our case CAN FD), PCAN first reports result PCAN_ERROR_ILLDATA and then it send the error frame. If the PCAN_ERROR_ILLDATA is not ignored, python-can throws an exception. This fix add the ignore on the PCAN_ERROR_ILLDATA. * Fix for ruff error `can/interfaces/pcan/pcan.py:5:1: I001 [*] Import block is un-sorted or un-formatted` Added comment explaining why to ignore the PCAN_ERROR_ILLDATA. --- can/interfaces/pcan/pcan.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/can/interfaces/pcan/pcan.py b/can/interfaces/pcan/pcan.py index 52dbb49b0..d0372a83c 100644 --- a/can/interfaces/pcan/pcan.py +++ b/can/interfaces/pcan/pcan.py @@ -43,6 +43,7 @@ PCAN_DICT_STATUS, PCAN_ERROR_BUSHEAVY, PCAN_ERROR_BUSLIGHT, + PCAN_ERROR_ILLDATA, PCAN_ERROR_OK, PCAN_ERROR_QRCVEMPTY, PCAN_FD_PARAMETER_LIST, @@ -555,6 +556,12 @@ def _recv_internal( elif result & (PCAN_ERROR_BUSLIGHT | PCAN_ERROR_BUSHEAVY): log.warning(self._get_formatted_error(result)) + elif result == PCAN_ERROR_ILLDATA: + # When there is an invalid frame on CAN bus (in our case CAN FD), PCAN first reports result PCAN_ERROR_ILLDATA + # and then it sends the error frame. If the PCAN_ERROR_ILLDATA is not ignored, python-can throws an exception. + # So we ignore any PCAN_ERROR_ILLDATA results here. + pass + else: raise PcanCanOperationError(self._get_formatted_error(result)) From 9360118db820eca5cf8db6c3175511084c6bed85 Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Mon, 9 Sep 2024 13:35:09 +0200 Subject: [PATCH 3/9] Format with black to pass checks --- can/bus.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/can/bus.py b/can/bus.py index 7ddb439bc..ad6355631 100644 --- a/can/bus.py +++ b/can/bus.py @@ -276,7 +276,9 @@ def send_periodic( # Create a backend specific task; will be patched to a _SelfRemovingCyclicTask later task = cast( _SelfRemovingCyclicTask, - self._send_periodic_internal(msgs, period, duration, autostart, modifier_callback), + self._send_periodic_internal( + msgs, period, duration, autostart, modifier_callback + ), ) # we wrap the task's stop method to also remove it from the Bus's list of tasks periodic_tasks = self._periodic_tasks From 4f410bccf335cd1a2a94f84999a45ac2ef0fa900 Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Wed, 18 Sep 2024 09:36:18 +0200 Subject: [PATCH 4/9] Do not ignore autostart parameter for Bus.send_periodic() on IXXAT devices --- can/interfaces/ixxat/canlib_vcinpl.py | 20 +++++++++++++++++--- can/interfaces/ixxat/canlib_vcinpl2.py | 20 +++++++++++++++++--- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/can/interfaces/ixxat/canlib_vcinpl.py b/can/interfaces/ixxat/canlib_vcinpl.py index c370bf5ac..0579d0942 100644 --- a/can/interfaces/ixxat/canlib_vcinpl.py +++ b/can/interfaces/ixxat/canlib_vcinpl.py @@ -808,7 +808,12 @@ def _send_periodic_internal( self._scheduler_resolution = caps.dwClockFreq / caps.dwCmsDivisor _canlib.canSchedulerActivate(self._scheduler, constants.TRUE) return CyclicSendTask( - self._scheduler, msgs, period, duration, self._scheduler_resolution + self._scheduler, + msgs, + period, + duration, + self._scheduler_resolution, + autostart=autostart, ) # fallback to thread based cyclic task @@ -865,7 +870,15 @@ def state(self) -> BusState: class CyclicSendTask(LimitedDurationCyclicSendTaskABC, RestartableCyclicTaskABC): """A message in the cyclic transmit list.""" - def __init__(self, scheduler, msgs, period, duration, resolution): + def __init__( + self, + scheduler, + msgs, + period, + duration, + resolution, + autostart: bool = True, + ): super().__init__(msgs, period, duration) if len(self.messages) != 1: raise ValueError( @@ -885,7 +898,8 @@ def __init__(self, scheduler, msgs, period, duration, resolution): self._msg.uMsgInfo.Bits.dlc = self.messages[0].dlc for i, b in enumerate(self.messages[0].data): self._msg.abData[i] = b - self.start() + if autostart: + self.start() def start(self): """Start transmitting message (add to list if needed).""" diff --git a/can/interfaces/ixxat/canlib_vcinpl2.py b/can/interfaces/ixxat/canlib_vcinpl2.py index edb872781..5872f76b9 100644 --- a/can/interfaces/ixxat/canlib_vcinpl2.py +++ b/can/interfaces/ixxat/canlib_vcinpl2.py @@ -954,7 +954,12 @@ def _send_periodic_internal( ) # TODO: confirm _canlib.canSchedulerActivate(self._scheduler, constants.TRUE) return CyclicSendTask( - self._scheduler, msgs, period, duration, self._scheduler_resolution + self._scheduler, + msgs, + period, + duration, + self._scheduler_resolution, + autostart=autostart, ) # fallback to thread based cyclic task @@ -985,7 +990,15 @@ def shutdown(self): class CyclicSendTask(LimitedDurationCyclicSendTaskABC, RestartableCyclicTaskABC): """A message in the cyclic transmit list.""" - def __init__(self, scheduler, msgs, period, duration, resolution): + def __init__( + self, + scheduler, + msgs, + period, + duration, + resolution, + autostart: bool = True, + ): super().__init__(msgs, period, duration) if len(self.messages) != 1: raise ValueError( @@ -1005,7 +1018,8 @@ def __init__(self, scheduler, msgs, period, duration, resolution): self._msg.uMsgInfo.Bits.dlc = self.messages[0].dlc for i, b in enumerate(self.messages[0].data): self._msg.abData[i] = b - self.start() + if autostart: + self.start() def start(self): """Start transmitting message (add to list if needed).""" From 83280cda54b13af3b22f5880d69585537233d34d Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Wed, 18 Sep 2024 09:38:27 +0200 Subject: [PATCH 5/9] Do not ignore autostart parameter for Bis.send_periodic() on socketcan devices --- can/interfaces/socketcan/socketcan.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py index 10a726f0e..0bf801794 100644 --- a/can/interfaces/socketcan/socketcan.py +++ b/can/interfaces/socketcan/socketcan.py @@ -327,6 +327,7 @@ def __init__( messages: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, + autostart: bool = False, ) -> None: """Construct and :meth:`~start` a task. @@ -351,6 +352,9 @@ def __init__( self.task_id = task_id self._tx_setup(self.messages) + if autostart: + self.start() + def _tx_setup( self, messages: Sequence[Message], raise_if_task_exists: bool = True ) -> None: @@ -858,7 +862,9 @@ def _send_periodic_internal( msgs_channel = str(msgs[0].channel) if msgs[0].channel else None bcm_socket = self._get_bcm_socket(msgs_channel or self.channel) task_id = self._get_next_task_id() - task = CyclicSendTask(bcm_socket, task_id, msgs, period, duration) + task = CyclicSendTask( + bcm_socket, task_id, msgs, period, duration, autostart=autostart + ) return task # fallback to thread based cyclic task From 310d275b765fe7e554c2e4f7b3fb819f32c3fa91 Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Wed, 18 Sep 2024 14:22:36 +0200 Subject: [PATCH 6/9] Fix double start socketcan periodic --- can/interfaces/socketcan/socketcan.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py index 0bf801794..45e722c95 100644 --- a/can/interfaces/socketcan/socketcan.py +++ b/can/interfaces/socketcan/socketcan.py @@ -327,7 +327,7 @@ def __init__( messages: Union[Sequence[Message], Message], period: float, duration: Optional[float] = None, - autostart: bool = False, + autostart: bool = True, ) -> None: """Construct and :meth:`~start` a task. @@ -350,13 +350,13 @@ def __init__( self.bcm_socket = bcm_socket self.task_id = task_id - self._tx_setup(self.messages) - - if autostart: - self.start() + self._tx_setup(self.messages, send=autostart) def _tx_setup( - self, messages: Sequence[Message], raise_if_task_exists: bool = True + self, + messages: Sequence[Message], + raise_if_task_exists: bool = True, + send: bool = True, ) -> None: # Create a low level packed frame to pass to the kernel body = bytearray() @@ -380,7 +380,9 @@ def _tx_setup( for message in messages: body += build_can_frame(message) log.debug("Sending BCM command") - send_bcm(self.bcm_socket, header + body) + + if send: + send_bcm(self.bcm_socket, header + body) def _check_bcm_task(self) -> None: # Do a TX_READ on a task ID, and check if we get EINVAL. If so, From 1a7f3a19d7bf973ccec58fd0035de275ac2110be Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Mon, 7 Oct 2024 11:47:10 +0200 Subject: [PATCH 7/9] Fix link methods in docstring for start() methods of the tasks can.broadcastmanager.CyclicTask.start --- can/bus.py | 6 ++++-- can/interfaces/socketcan/socketcan.py | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/can/bus.py b/can/bus.py index ad6355631..678adc4fe 100644 --- a/can/bus.py +++ b/can/bus.py @@ -240,7 +240,8 @@ def send_periodic( Disable to instead manage tasks manually. :param autostart: If True (the default) the sending task will immediately start after creation. - Otherwise, the task has to be started by calling start() on it. + Otherwise, the task has to be started by calling the tasks :meth:`~can.broadcastmanager.CyclicTask.start` + method on it. :param modifier_callback: Function which should be used to modify each message's data before sending. The callback modifies the :attr:`~can.Message.data` of the @@ -321,7 +322,8 @@ def _send_periodic_internal( no duration is provided, the task will continue indefinitely. :param autostart: If True (the default) the sending task will immediately start after creation. - Otherwise, the task has to be started by calling start() on it. + Otherwise, the task has to be started by calling the tasks :meth:`~can.broadcastmanager.CyclicTask.start` + method on it. :return: A started task instance. Note the task can be stopped (and depending on the backend modified) by calling the diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py index 45e722c95..32f9d8ad1 100644 --- a/can/interfaces/socketcan/socketcan.py +++ b/can/interfaces/socketcan/socketcan.py @@ -839,7 +839,8 @@ def _send_periodic_internal( no duration is provided, the task will continue indefinitely. :param autostart: If True (the default) the sending task will immediately start after creation. - Otherwise, the task has to be started by calling start() on it. + Otherwise, the task has to be started by calling the tasks :meth:`~can.broadcastmanager.CyclicTask.start` + method on it. :raises ValueError: If task identifier passed to :class:`CyclicSendTask` can't be used From 9b11310f01c54b4f9d43991d69066a12a5098a81 Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Mon, 7 Oct 2024 12:16:48 +0200 Subject: [PATCH 8/9] Change the behaviour of autostart parameter in socketcan implementation of CyclicSendTask to not call _tx_setup() method instead of adding a parameter to it. --- can/interfaces/socketcan/socketcan.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py index 32f9d8ad1..c87439e8e 100644 --- a/can/interfaces/socketcan/socketcan.py +++ b/can/interfaces/socketcan/socketcan.py @@ -350,13 +350,13 @@ def __init__( self.bcm_socket = bcm_socket self.task_id = task_id - self._tx_setup(self.messages, send=autostart) + if autostart: + self._tx_setup(self.messages) def _tx_setup( self, messages: Sequence[Message], raise_if_task_exists: bool = True, - send: bool = True, ) -> None: # Create a low level packed frame to pass to the kernel body = bytearray() @@ -380,9 +380,7 @@ def _tx_setup( for message in messages: body += build_can_frame(message) log.debug("Sending BCM command") - - if send: - send_bcm(self.bcm_socket, header + body) + send_bcm(self.bcm_socket, header + body) def _check_bcm_task(self) -> None: # Do a TX_READ on a task ID, and check if we get EINVAL. If so, From 1925d03d9127c308215aacac7a102c443155b9fe Mon Sep 17 00:00:00 2001 From: Sebastian Wolf Date: Mon, 7 Oct 2024 21:36:29 +0200 Subject: [PATCH 9/9] Fix code style (max 100 chars per line) Fix wrong docstring reference. --- can/bus.py | 8 ++++---- can/interfaces/socketcan/socketcan.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/can/bus.py b/can/bus.py index 678adc4fe..a12808ab6 100644 --- a/can/bus.py +++ b/can/bus.py @@ -240,8 +240,8 @@ def send_periodic( Disable to instead manage tasks manually. :param autostart: If True (the default) the sending task will immediately start after creation. - Otherwise, the task has to be started by calling the tasks :meth:`~can.broadcastmanager.CyclicTask.start` - method on it. + Otherwise, the task has to be started by calling the + tasks :meth:`~can.RestartableCyclicTaskABC.start` method on it. :param modifier_callback: Function which should be used to modify each message's data before sending. The callback modifies the :attr:`~can.Message.data` of the @@ -322,8 +322,8 @@ def _send_periodic_internal( no duration is provided, the task will continue indefinitely. :param autostart: If True (the default) the sending task will immediately start after creation. - Otherwise, the task has to be started by calling the tasks :meth:`~can.broadcastmanager.CyclicTask.start` - method on it. + Otherwise, the task has to be started by calling the + tasks :meth:`~can.RestartableCyclicTaskABC.start` method on it. :return: A started task instance. Note the task can be stopped (and depending on the backend modified) by calling the diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py index c87439e8e..40da0d094 100644 --- a/can/interfaces/socketcan/socketcan.py +++ b/can/interfaces/socketcan/socketcan.py @@ -837,8 +837,8 @@ def _send_periodic_internal( no duration is provided, the task will continue indefinitely. :param autostart: If True (the default) the sending task will immediately start after creation. - Otherwise, the task has to be started by calling the tasks :meth:`~can.broadcastmanager.CyclicTask.start` - method on it. + Otherwise, the task has to be started by calling the + tasks :meth:`~can.RestartableCyclicTaskABC.start` method on it. :raises ValueError: If task identifier passed to :class:`CyclicSendTask` can't be used