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

Changes from 3 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
47 changes: 16 additions & 31 deletions can/interfaces/slcan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,40 +150,25 @@ 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
while not _timeout.expired():
new_data = self.serialPortOrig.read_all()
if new_data:
self._buffer.extend(new_data)
else:
time_left = timeout - (time.time() - start)
if time_left > 0:
continue
else:
return None

# 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
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

def flush(self) -> None:
del self._buffer[:]
Expand Down