bufReader read_until not handled correctly on Windows

[craigkai]

Using the following code:

 let mut reader = BufReader::new(connection);
 let mut my_str = vec![];

if let Err(error) = reader.read_until(0xA, &mut my_str) {....}

Where I am reading some data from a serial connection. On Linux & Mac I see the read_until function executing quite fast, but on Windows (11) I see the serial connection's timeout (500ms) is what is ending the read_until command -- Which does then proceed to do the correct thing after with the correct value in the buffer. It is just comparatively slow.

An example of the output I am seeing:

Successfully sent write to serial: CONFIG
Successfully read from serial "0\n"
Successfully sent write to serial: ILLUMDAY
Successfully read from serial "10\n"
Successfully sent write to serial: ILLUMNIGHT
Successfully read from serial "25\n"
Successfully sent write to serial: ILLUMMODE
Successfully read from serial "0\n"
Successfully sent write to serial: ECHO
Successfully read from serial "0\n"
Successfully sent write to serial: ANIM
Successfully read from serial "0\n"

Is this some kind of bug with Windows or am I missing something obvious? Thanks for any input!

[mlsvrts]

@craigkai Windows COMMTIMEOUTS are extremely cursed, and I'm nearly certain they're at fault here.

On Windows, if you try to read 'n' bytes from a serial port no data will be returned until**:

1. A timeout has occurred
2. Your buffer has been filled

In theory, you could set your read timeout to be 1ms to get only kind of bad latency, but in practice this just doesn't work and you're looking at a minimum timeout of 10+ milliseconds. So. What can you do?

1. Read one byte at time, and just loop until you read your terminator. You can pretty much do this by using a BufReader with capacity of 1.

2. Know how much data your responses will contain, and don't try to read_until until at least 'n' bytes are available. This results in a lot of syscalls while you busy-poll the available bytes, but lower latency. You could achieve this a few ways: a. Looking up the response length base on passed command from a table b. Adding some fixed-length header to your response that contains the message length c. Probably some other ways...

3. Number 1, but use async so that you don't need to busy wait for each byte. (This is actually how PuTTy waits for data). You can make this work with mio-serial and tokio-serial, but not without hacking NamedPipe to have a 1-byte buffer

**Actually, when I told you this I was lying; Windows only behaves like this for certain configurations of COMMTIMEOUTS. Feel free to refer to this table (credit: Carsten Andrich)

Denomination	termios	COMMTIMEOUTS	POSIX behavior	Windows behavior
Polling read	MIN = 0 and TIME = 0	ReadIntervalTimeout = MAXDWORD, ReadTotalTimeoutMultiplier = 0, and ReadTotalTimeoutConstant = 0	Always returns immediately	Like POSIX
Blocking read	MIN = 1 and TIME = 0	ReadIntervalTimeout = 0, ReadTotalTimeoutMultiplier = 0, and ReadTotalTimeoutConstant = 0	Returns when at least 1 byte is available	Returns only when buffer is full
Blocking read	MIN = 1 and TIME = 0	ReadIntervalTimeout = 1, ReadTotalTimeoutMultiplier = 0, and ReadTotalTimeoutConstant = 0	Returns when at least 1 byte is available	Like POSIX, but presumably extra delay
Read with timeout	MIN = 0 and TIME > 0	ReadIntervalTimeout = MAXDWORD, ReadTotalTimeoutMultiplier = MAXDWORD, and ReadTotalTimeoutConstant > 0	Returns when at least 1 byte is available or timeout occurs	Similar to POSIX, but first ReadFile() returns only 1 byte and second ReadFile() returns the remainder
Read with timeout	MIN = 0 and TIME > 0	ReadIntervalTimeout = 1, ReadTotalTimeoutMultiplier = 0, and ReadTotalTimeoutConstant > 0	Returns when at least 1 byte is available or timeout occurs	Like POSIX, but presumably extra delay

[mlsvrts]

Fast BufReader example (loopback port):

use std::io::{BufReader, BufRead};

fn main() {
    let mut port = serialport::new("COM3", 9600)
        .timeout(std::time::Duration::from_millis(500))
        .open()
        .expect("failed to open port");

    let reader = port.try_clone()
        .expect("failed to clone port");

    port.write_all(&[0xDE, 0xAD, 0xBE, 0xEF, 0x00])
        .expect("failed to write to port");

    // Create the slow bufreader
    let mut buf = vec![];
    let mut b_reader = BufReader::with_capacity(100, reader);

    let now = std::time::Instant::now();
    if let Err(e) = b_reader.read_until(0x00, &mut buf) {
        println!("Slow reader error: {:?}", e);
    }

    println!("slow read time: {}ms", now.elapsed().as_millis());

    // Fast bufreader
    port.write_all(&[0xDE, 0xAD, 0xBE, 0xEF, 0x00])
        .expect("failed to write to port");
    
    let mut b_reader = BufReader::with_capacity(1, b_reader.into_inner());

    let now = std::time::Instant::now();
    if let Err(e) = b_reader.read_until(0x00, &mut buf) {
        println!("Fast reader error: {:?}", e);
    }

    println!("fast read time: {}ms", now.elapsed().as_millis());
}

Which gives me:

slow read time: 515ms
fast read time: 19ms

[craigkai]

This has been so helpful, thank you so much for all the assistance!!

[DanielJoyce]

To me this perhaps implies we need to refactor things so this is more apparent to the user. Windows behavior is apparently commonly counterintuitive.

That or the default windows serial port config needs to be changed to better mirror POSIX behavior and this information added to the docs.

[larsch]

I have suggested a change to use the POSIX-like blocking read (with early return on ANY data) and timeout in #79. This fixes the general case of of read blocking until the buffer is full. It only makes it slightly more CPU intensive as it will return as much as in the FIFO any time the read call is invoked, but for serial port speeds, this is irrelevant.

This change is necessary for any serial port protocol that uses request/response scheme where the size of packets is unknown in advance, or when using mio-seriel/tokio-serial where you can't tell the underlying library how much data you want to read.

[jerrywrice]

I've just started using serialport-rs crate, but I also require the POSIX-like (new) blocking read() behavior that larsch mentions in his post. I would hope this could be incorporated as a new Windows specific feature that a developer can explicitly enable (opt into). Ideally this could be added as a non-breaking change, by defining additional functions or methods to activate it. Thus it would only be 'new' user code that activates the feature.