Skip to content

Improve slcan.py #1490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Feb 4, 2023
Merged

Improve slcan.py #1490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2c9e1e6
Improve slcan.py
mikisama Jan 17, 2023
f95ca7d
Merge branch 'hardbyte:develop' into develop
mikisama Jan 27, 2023
7fa9f5c
use `read_all` to read out serial port data.
mikisama Jan 27, 2023
c581cc9
fix when the `timeout` parameter is 0, it cannot enter the while loop.
mikisama Jan 27, 2023
8722a11
For performance reason, revert to using `read` to read serial data.
mikisama Jan 28, 2023
fa62a63
add `size=1` and renamed `new_data` to `new_byte`
mikisama Jan 29, 2023
73ce3dd
fix the issue of returning `None` before receiving
mikisama Jan 29, 2023
3eb80b9
Due to the simplification of the control flow,
mikisama Jan 29, 2023
2bbee49
Revert "Due to the simplification of the control flow,"
mikisama Jan 29, 2023
1635381
Simplify the control flow for finding the end of a SLCAN message.
mikisama Jan 29, 2023
42db106
improve the `timeout` handling of slcan.py
mikisama Jan 31, 2023
6f994cc
Merge branch 'hardbyte:develop' into develop
mikisama Jan 31, 2023
3c8a9e3
Change the plain format calls to f-strings.
mikisama Jan 31, 2023
ce3b9a4
using `bytearray.fromhex` to parse the frame's data.
mikisama Jan 31, 2023
2c22b4e
change `del self._buffer[:]` to `self._buffer.clear` since it's more …
mikisama Jan 31, 2023
09e1248
Simplify the timeout handling
mikisama Jan 31, 2023
e3a388b
fix the issue when the returned string is `None`.
mikisama Jan 31, 2023
370a2ff
using `pyserial.reset_input_buffer` to discard
mikisama Jan 31, 2023
68c9c71
fix failing PyPy test
mikisama Feb 1, 2023
cec01d7
fix failing PyPy test
mikisama Feb 1, 2023
6c3b13a
improve the comments
mikisama Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 57 additions & 90 deletions can/interfaces/slcan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand All @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the serial port timeout in the while loop slow down the RX performance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to ensure, that the function respects the timeout parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to ensure, that the function respects the timeout parameter?

  • This is the test code for testing the timeout parameter
import time

from can.interfaces.slcan import slcanBus
from can.message import Message

bus = slcanBus(channel="COM11", bitrate=20000, sleep_after_open=0)

tx_msg = Message(
    arbitration_id=0x3C,
    is_extended_id=False,
    is_remote_frame=False,
    dlc=8,
    data=bytes(
        [
            0x01,
            0x02,
            0x10,
            0x01,
            0xFF,
            0xFF,
            0xFF,
            0xFF,
        ]
    ),
)

rx_msg = Message(
    arbitration_id=0x3D,
    is_extended_id=False,
    is_remote_frame=True,
    dlc=8,
)

count = 5
timeout = 1

start_time = time.time()
for _ in range(count):
    bus.send(tx_msg)
    bus.send(rx_msg)
    msg = bus.recv(timeout)
    assert msg is None
    print(msg)
end_time = time.time()
spend_time = end_time - start_time
assert abs((timeout * count) - spend_time) < 0.1
print(f"{spend_time:.3f}")

bus.shutdown()
  • This is the result
    image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this proposed change I think there's still a scenario where the timeout is not enforced.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read_all sounds good to me.

    def _read(self, timeout: Optional[float]) -> Optional[str]:
        _timeout = serial.Timeout(timeout)

        with error_check("Could not read from serial device"):
            while not _timeout.expired():
                new_data = self.serialPortOrig.read_all()
                if new_data is not None:
                    self._buffer.extend(new_data)
                else:
                    time.sleep(0.001)
                    continue

                for terminator in (self._ERROR, self._OK):
                    if terminator in self._buffer:
                        i = self._buffer.index(terminator) + 1
                        string = self._buffer[:i].decode()
                        del self._buffer[:i]
                        return string

            return None

The serial port must be instantiated with timeout=0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what about when there is data in the buffer? What if there were an infinite amount of 'junk'?

@Tbruno25

def read(self, __size: int = ...) -> bytes: ...
def read_all(self):
    return self.read(self.in_waiting)
  1. If there is data in the buffer, the read method only copies it out. Only when the data bytes in the buffer are smaller than the parameter __size, the read method will blocking until the data bytes in the buffer is __size, then copy it out.
  2. Since the in_waiting parameter shows the current number of bytes in the buffer, blocking will not occur. (The time to copy data out is negligibly small)
  3. According to my test, the MAX receive buffer size in win10 is 16384, and ubuntu 20.04 is 4095. I'm not sure whether it can be changed. But I think there will not be infinite junk.
import serial
import time

with serial.Serial("COM11") as ser:
    # send a start command to make
    # the STM32 dev board start sending data.
    ser.write(b"start")

    while True:
        time.sleep(0.5)
        print(f"in_waiting {ser.in_waiting}")

image

image

Copy link
Contributor Author

@mikisama mikisama Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zariiii9003

Looks good. Just a few minor changes requested.

According to my test, the read_all method returns b'' if there is no data that can be read out.

So the if new_data is not None: should be changed to if new_data:, otherwise it will never do time.sleep, which will cause high CPU usage.

    def _read(self, timeout: Optional[float]) -> Optional[str]:
        _timeout = serial.Timeout(timeout)

        with error_check("Could not read from serial device"):
            while not _timeout.expired():
                new_data = self.serialPortOrig.read_all()
                if new_data:
                    self._buffer.extend(new_data)
                else:
                    time.sleep(0.001)
                    continue

                for terminator in (self._ERROR, self._OK):
                    if terminator in self._buffer:
                        i = self._buffer.index(terminator) + 1
                        string = self._buffer[:i].decode()
                        del self._buffer[:i]
                        return string

            return None

The serial port must be instantiated with timeout=0.

By the way, I did not initialize the timeout parameter of serialPortOrig at all and it defaults to None. It seems still works for me.

I'm curious. If you don't set the timeout to 0, what will happen to you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious. If you don't set the timeout to 0, what will happen to you?

Nothing, i don't have an interface to test this 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my test, the MAX receive buffer size in win10 is 16384, and ubuntu 20.04 is 4095. I'm not sure whether it can be changed. But I think there will not be infinite junk.

Interesting! And I agree -- I wasn't considering the physical limitations of the hardware/kernel. Good catch 🙂

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")
Expand All @@ -204,7 +197,7 @@ def _recv_internal(
canId = None
remote = False
extended = False
frame = []
data = None

string = self._read(timeout)

Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Copy link
Contributor Author

@mikisama mikisama Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-added the None checking.
Due to the unit test using the loop:// parameter, self._read will read back the content sent by self._write(cmd). It can not catch the None error.

image

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.
Expand All @@ -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
33 changes: 24 additions & 9 deletions test/test_slcan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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):
Expand Down