Improve slcan.py (#1490)

* Improve slcan.py

1. Improve receiving performance
2. Fix an issue that the first read may blocking even if the `timeout`
   parameter is not `None`.

* use `read_all` to read out serial port data.

* fix when the `timeout` parameter is 0, it cannot enter the while loop.

* For performance reason, revert to using `read` to read serial data.

* add `size=1` and renamed `new_data` to `new_byte`
to make the code easier to understand and read.

* fix the issue of returning `None` before receiving
the full SLCAN message.

* Due to the simplification of the control flow,
the class variable `self._buffer` can be replaced
by the local variable `buffer`.

* Revert "Due to the simplification of the control flow,"

This reverts commit 3eb80b9.

* Simplify the control flow for finding the end of a SLCAN message.

* improve the `timeout` handling of slcan.py

* Change the plain format calls to f-strings.

* using `bytearray.fromhex` to parse the frame's data.

* change `del self._buffer[:]` to `self._buffer.clear` since it's more readable.

* Simplify the timeout handling

* fix the issue when the returned string is `None`.

* using `pyserial.reset_input_buffer` to discard
the data in the input buffer.

* fix failing PyPy test

* fix failing PyPy test

* improve the comments

Author: mikisama

can/interfaces/slcan.py

@@ -64,6 +64,7 @@ def __init__(
         btr: Optional[str] = None,
         sleep_after_open: float = _SLEEP_AFTER_SERIAL_OPEN,
         rtscts: bool = False,
+        timeout: float = 0.001,
         **kwargs: Any,
     ) -> None:
         """
@@ -82,7 +83,8 @@ def __init__(
             Time to wait in seconds after opening serial connection
         :param rtscts:
             turn hardware handshake (RTS/CTS) on and off
-
+        :param timeout:
+            Timeout for the serial or usb device in seconds (default 0.001)
         :raise ValueError: if both ``bitrate`` and ``btr`` are set or the channel is invalid
         :raise CanInterfaceNotImplementedError: if the serial module is missing
         :raise CanInitializationError: if the underlying serial connection could not be established
@@ -98,7 +100,10 @@ def __init__(
 
         with error_check(exception_type=CanInitializationError):
             self.serialPortOrig = serial.serial_for_url(
-                channel, baudrate=ttyBaudrate, rtscts=rtscts
+                channel,
+                baudrate=ttyBaudrate,
+                rtscts=rtscts,
+                timeout=timeout,
             )
 
         self._buffer = bytearray()
@@ -150,46 +155,34 @@ def _write(self, string: str) -> None:
             self.serialPortOrig.flush()
 
     def _read(self, timeout: Optional[float]) -> Optional[str]:
+        _timeout = serial.Timeout(timeout)
 
         with error_check("Could not read from serial device"):
-            # first read what is already in receive buffer
-            while self.serialPortOrig.in_waiting:
-                self._buffer += self.serialPortOrig.read()
-            # if we still don't have a complete message, do a blocking read
-            start = time.time()
-            time_left = timeout
-            while not (
-                ord(self._OK) in self._buffer or ord(self._ERROR) in self._buffer
-            ):
-                self.serialPortOrig.timeout = time_left
-                byte = self.serialPortOrig.read()
-                if byte:
-                    self._buffer += byte
-                # if timeout is None, try indefinitely
-                if timeout is None:
-                    continue
-                # try next one only if there still is time, and with
-                # reduced timeout
-                else:
-                    time_left = timeout - (time.time() - start)
-                    if time_left > 0:
-                        continue
+            while True:
+                # Due to accessing `serialPortOrig.in_waiting` too often will reduce the performance.
+                # We read the `serialPortOrig.in_waiting` only once here.
+                in_waiting = self.serialPortOrig.in_waiting
+                for _ in range(max(1, in_waiting)):
+                    new_byte = self.serialPortOrig.read(size=1)
+                    if new_byte:
+                        self._buffer.extend(new_byte)
                     else:
-                        return None
+                        break
+
+                    if new_byte in (self._ERROR, self._OK):
+                        string = self._buffer.decode()
+                        self._buffer.clear()
+                        return string
+
+                if _timeout.expired():
+                    break
 
-        # return first message
-        for i in range(len(self._buffer)):
-            if self._buffer[i] == ord(self._OK) or self._buffer[i] == ord(self._ERROR):
-                string = self._buffer[: i + 1].decode()
-                del self._buffer[: i + 1]
-                break
-        return string
+            return None
 
     def flush(self) -> None:
-        del self._buffer[:]
+        self._buffer.clear()
         with error_check("Could not flush"):
-            while self.serialPortOrig.in_waiting:
-                self.serialPortOrig.read()
+            self.serialPortOrig.reset_input_buffer()
 
     def open(self) -> None:
         self._write("O")
@@ -204,7 +197,7 @@ def _recv_internal(
         canId = None
         remote = False
         extended = False
-        frame = []
+        data = None
 
         string = self._read(timeout)
 
@@ -215,14 +208,12 @@ def _recv_internal(
             canId = int(string[1:9], 16)
             dlc = int(string[9])
             extended = True
-            for i in range(0, dlc):
-                frame.append(int(string[10 + i * 2 : 12 + i * 2], 16))
+            data = bytearray.fromhex(string[10 : 10 + dlc * 2])
         elif string[0] == "t":
             # normal frame
             canId = int(string[1:4], 16)
             dlc = int(string[4])
-            for i in range(0, dlc):
-                frame.append(int(string[5 + i * 2 : 7 + i * 2], 16))
+            data = bytearray.fromhex(string[5 : 5 + dlc * 2])
         elif string[0] == "r":
             # remote frame
             canId = int(string[1:4], 16)
@@ -242,7 +233,7 @@ def _recv_internal(
                 timestamp=time.time(),  # Better than nothing...
                 is_remote_frame=remote,
                 dlc=dlc,
-                data=frame,
+                data=data,
             )
             return msg, False
         return None, False
@@ -252,15 +243,15 @@ def send(self, msg: Message, timeout: Optional[float] = None) -> None:
             self.serialPortOrig.write_timeout = timeout
         if msg.is_remote_frame:
             if msg.is_extended_id:
-                sendStr = "R%08X%d" % (msg.arbitration_id, msg.dlc)
+                sendStr = f"R{msg.arbitration_id:08X}{msg.dlc:d}"
             else:
-                sendStr = "r%03X%d" % (msg.arbitration_id, msg.dlc)
+                sendStr = f"r{msg.arbitration_id:03X}{msg.dlc:d}"
         else:
             if msg.is_extended_id:
-                sendStr = "T%08X%d" % (msg.arbitration_id, msg.dlc)
+                sendStr = f"T{msg.arbitration_id:08X}{msg.dlc:d}"
             else:
-                sendStr = "t%03X%d" % (msg.arbitration_id, msg.dlc)
-            sendStr += "".join(["%02X" % b for b in msg.data])
+                sendStr = f"t{msg.arbitration_id:03X}{msg.dlc:d}"
+            sendStr += msg.data.hex().upper()
         self._write(sendStr)
 
     def shutdown(self) -> None:
@@ -295,29 +286,17 @@ def get_version(
         cmd = "V"
         self._write(cmd)
 
-        start = time.time()
-        time_left = timeout
-        while True:
-            string = self._read(time_left)
-
-            if not string:
-                pass
-            elif string[0] == cmd and len(string) == 6:
-                # convert ASCII coded version
-                hw_version = int(string[1:3])
-                sw_version = int(string[3:5])
-                return hw_version, sw_version
-            # if timeout is None, try indefinitely
-            if timeout is None:
-                continue
-            # try next one only if there still is time, and with
-            # reduced timeout
-            else:
-                time_left = timeout - (time.time() - start)
-                if time_left > 0:
-                    continue
-                else:
-                    return None, None
+        string = self._read(timeout)
+
+        if not string:
+            pass
+        elif string[0] == cmd and len(string) == 6:
+            # convert ASCII coded version
+            hw_version = int(string[1:3])
+            sw_version = int(string[3:5])
+            return hw_version, sw_version
+
+        return None, None
 
     def get_serial_number(self, timeout: Optional[float]) -> Optional[str]:
         """Get serial number of the slcan interface.
@@ -331,24 +310,12 @@ def get_serial_number(self, timeout: Optional[float]) -> Optional[str]:
         cmd = "N"
         self._write(cmd)
 
-        start = time.time()
-        time_left = timeout
-        while True:
-            string = self._read(time_left)
-
-            if not string:
-                pass
-            elif string[0] == cmd and len(string) == 6:
-                serial_number = string[1:-1]
-                return serial_number
-            # if timeout is None, try indefinitely
-            if timeout is None:
-                continue
-            # try next one only if there still is time, and with
-            # reduced timeout
-            else:
-                time_left = timeout - (time.time() - start)
-                if time_left > 0:
-                    continue
-                else:
-                    return None
+        string = self._read(timeout)
+
+        if not string:
+            pass
+        elif string[0] == cmd and len(string) == 6:
+            serial_number = string[1:-1]
+            return serial_number
+
+        return None

test/test_slcan.py

@@ -2,11 +2,26 @@
 
 import unittest
 import can
+from .config import IS_PYPY
+
+
+"""
+Mentioned in #1010 & #1490
+
+> PyPy works best with pure Python applications. Whenever you use a C extension module, 
+> it runs much slower than in CPython. The reason is that PyPy can't optimize C extension modules since they're not fully supported. 
+> In addition, PyPy has to emulate reference counting for that part of the code, making it even slower.
+
+https://realpython.com/pypy-faster-python/#it-doesnt-work-well-with-c-extensions
+"""
+TIMEOUT = 0.5 if IS_PYPY else 0.001  # 0.001 is the default set in slcanBus
 
 
 class slcanTestCase(unittest.TestCase):
     def setUp(self):
-        self.bus = can.Bus("loop://", interface="slcan", sleep_after_open=0)
+        self.bus = can.Bus(
+            "loop://", interface="slcan", sleep_after_open=0, timeout=TIMEOUT
+        )
         self.serial = self.bus.serialPortOrig
         self.serial.read(self.serial.in_waiting)
 
@@ -15,7 +30,7 @@ def tearDown(self):
 
     def test_recv_extended(self):
         self.serial.write(b"T12ABCDEF2AA55\r")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNotNone(msg)
         self.assertEqual(msg.arbitration_id, 0x12ABCDEF)
         self.assertEqual(msg.is_extended_id, True)
@@ -33,7 +48,7 @@ def test_send_extended(self):
 
     def test_recv_standard(self):
         self.serial.write(b"t4563112233\r")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNotNone(msg)
         self.assertEqual(msg.arbitration_id, 0x456)
         self.assertEqual(msg.is_extended_id, False)
@@ -51,7 +66,7 @@ def test_send_standard(self):
 
     def test_recv_standard_remote(self):
         self.serial.write(b"r1238\r")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNotNone(msg)
         self.assertEqual(msg.arbitration_id, 0x123)
         self.assertEqual(msg.is_extended_id, False)
@@ -68,7 +83,7 @@ def test_send_standard_remote(self):
 
     def test_recv_extended_remote(self):
         self.serial.write(b"R12ABCDEF6\r")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNotNone(msg)
         self.assertEqual(msg.arbitration_id, 0x12ABCDEF)
         self.assertEqual(msg.is_extended_id, True)
@@ -85,23 +100,23 @@ def test_send_extended_remote(self):
 
     def test_partial_recv(self):
         self.serial.write(b"T12ABCDEF")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNone(msg)
 
         self.serial.write(b"2AA55\rT12")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNotNone(msg)
         self.assertEqual(msg.arbitration_id, 0x12ABCDEF)
         self.assertEqual(msg.is_extended_id, True)
         self.assertEqual(msg.is_remote_frame, False)
         self.assertEqual(msg.dlc, 2)
         self.assertSequenceEqual(msg.data, [0xAA, 0x55])
 
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNone(msg)
 
         self.serial.write(b"ABCDEF2AA55\r")
-        msg = self.bus.recv(0)
+        msg = self.bus.recv(TIMEOUT)
         self.assertIsNotNone(msg)
 
     def test_version(self):