Skip to content
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

Jump to bottom

Tweak windows COMMTIMEOUTS settings #13

Closed
wants to merge 1 commit into from
Closed

Tweak windows COMMTIMEOUTS settings #13

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 16 additions & 5 deletions src/sys/windows/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use winapi::shared::minwindef::{BOOL, HKEY};
use winapi::shared::winerror;
use winapi::um::{commapi, fileapi, handleapi, ioapiset, minwinbase, synchapi, winbase, winnt, winreg};

const DEFAULT_TIMEOUT: u32 = 10;
Copy link
Owner

Choose a reason for hiding this comment

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

As a default timeout, I think 10ms is too short. Normally read/writes can block for much longer before timing out. Maybe even a few seconds would be good.

The value of 10 ms was the timeout between receiving character data, where it could be sensible.

Copy link
Author

Choose a reason for hiding this comment

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

This time out, at least in POSIX land is how we wait for data to be available, or block on the serial port waiting to write some data. Usually its basically 0 for serial ports. Usually you try and read/write data immediately without a timeout, and the system tells you how many bytes you actually read/wrote.

I know windows has its own weird windows-isms, so I don't know.

Copy link
Author

Choose a reason for hiding this comment

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

Theoretically I guess windows might provide support for filling a larger buffer within a timeout window timeframe, but the purpose of this PR was to try and approximate the simpler POSIX style serial port handling, and we could then expose a windows-only api.

Again, should dig around in the PySerial sources for windows. Hmm

Copy link
Author

Choose a reason for hiding this comment

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

I think a lot of people doing serial on windows get confused because many other packages on windows ( pyserial, arduino ) basically configure the windows serial port handling to be more like POSIX.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm also in favor of emulating POSIX behaviour, but I still think 10ms is very short. On Linux, the default timeout for a virtual terminal seems to be infinite (or at least longer than my patience).

Note that the timeout only triggers if no data is available. If data is available, it will still return without waiting for the timeout. That's why I think 10ms is too short as a default timeout.


pub struct SerialPort {
pub file: std::fs::File,
}
Expand Down Expand Up @@ -37,8 +39,17 @@ impl SerialPort {
unsafe {
let mut timeouts: winbase::COMMTIMEOUTS = std::mem::zeroed();
check_bool(commapi::GetCommTimeouts(file.as_raw_handle(), &mut timeouts))?;
timeouts.ReadIntervalTimeout = 10;
check_bool(commapi::SetCommTimeouts(file.as_raw_handle(), &mut timeouts))?;
// How to set the COMM timeouts so the behavior is a bit more sane
// https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-commtimeouts#remarks
// TODO: Expose these as configuration options on Windows
timeouts.ReadIntervalTimeout = std::u32::MAX;
timeouts.ReadTotalTimeoutMultiplier = std::u32::MAX;
// Default timeout is 10ms
timeouts.ReadTotalTimeoutConstant = DEFAULT_TIMEOUT;
// Set write timeouts
timeouts.WriteTotalTimeoutMultiplier = std::u32::MAX;
timeouts.WriteTotalTimeoutConstant = DEFAULT_TIMEOUT;
Comment on lines +50 to +51
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
timeouts.WriteTotalTimeoutMultiplier = std::u32::MAX;
timeouts.WriteTotalTimeoutConstant = DEFAULT_TIMEOUT;
timeouts.WriteTotalTimeoutMultiplier = 0;
timeouts.WriteTotalTimeoutConstant = DEFAULT_TIMEOUT;

DWORDMAX is not a special value for the write timeouts.

Copy link
Author

@DanielJoyce DanielJoyce Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This from PySerial win32 seems to show some other behavior

        # Set Windows timeout values
        # timeouts is a tuple with the following items:
        # (ReadIntervalTimeout,ReadTotalTimeoutMultiplier,
        #  ReadTotalTimeoutConstant,WriteTotalTimeoutMultiplier,
        #  WriteTotalTimeoutConstant)
        timeouts = win32.COMMTIMEOUTS()
        if self._timeout is None:
            pass  # default of all zeros is OK
        elif self._timeout == 0:
            timeouts.ReadIntervalTimeout = win32.MAXDWORD
        else:
            timeouts.ReadTotalTimeoutConstant = max(int(self._timeout * 1000), 1)
        if self._timeout != 0 and self._inter_byte_timeout is not None:
            timeouts.ReadIntervalTimeout = max(int(self._inter_byte_timeout * 1000), 1)

        if self._write_timeout is None:
            pass
        elif self._write_timeout == 0:
            timeouts.WriteTotalTimeoutConstant = win32.MAXDWORD
        else:
            timeouts.WriteTotalTimeoutConstant = max(int(self._write_timeout * 1000), 1)
        win32.SetCommTimeouts(self._port_handle, ctypes.byref(timeouts))

Copy link
Author

Choose a reason for hiding this comment

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

        elif self._write_timeout == 0:
            timeouts.WriteTotalTimeoutConstant = win32.MAXDWORD

Copy link
Owner

Choose a reason for hiding this comment

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

Thats an attempt to make the timeout as long as possible. It's not a special value.

Also, I'm suggesting to change the WriteTotalTimeoutInterval to 0, not WriteTotalTimeoutConstant.

timeouts.check_bool(commapi::SetCommTimeouts(file.as_raw_handle(), &mut timeouts))?;
}
Ok(Self::from_file(file))
}
Expand Down Expand Up @@ -66,8 +77,8 @@ impl SerialPort {
unsafe {
let mut timeouts = std::mem::zeroed();
check_bool(commapi::GetCommTimeouts(self.file.as_raw_handle(), &mut timeouts))?;
timeouts.ReadIntervalTimeout = 0;
timeouts.ReadTotalTimeoutMultiplier = 0;
timeouts.ReadIntervalTimeout = std::u32::MAX;
timeouts.ReadTotalTimeoutMultiplier = std::u32::MAX;
timeouts.ReadTotalTimeoutConstant = timeout.as_millis().try_into().unwrap_or(u32::MAX);
check_bool(commapi::SetCommTimeouts(self.file.as_raw_handle(), &mut timeouts))
}
Expand All @@ -85,7 +96,7 @@ impl SerialPort {
unsafe {
let mut timeouts = std::mem::zeroed();
check_bool(commapi::GetCommTimeouts(self.file.as_raw_handle(), &mut timeouts))?;
timeouts.WriteTotalTimeoutMultiplier = 0;
timeouts.WriteTotalTimeoutMultiplier = std::u32::MAX;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
timeouts.WriteTotalTimeoutMultiplier = std::u32::MAX;
timeouts.WriteTotalTimeoutMultiplier = 0;

timeouts.WriteTotalTimeoutConstant = timeout.as_millis().try_into().unwrap_or(u32::MAX);
check_bool(commapi::SetCommTimeouts(self.file.as_raw_handle(), &mut timeouts))
}
Expand Down
Loading