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.

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

Set Windows timeouts to enforce non-blocking read #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

larsch
Copy link

@larsch larsch commented Feb 1, 2023

This is the equivalent of the PR serialport/serialport-rs#79. I would actually recommend completely removing the COMMTIMEOUTS override in mio-serial when/if the patch is accepted on serialport-rs, as this will solve the issue of read not returning available data until buffer is full or timeout. However, without this patch, the fix in serialport-rs does nothing.

This behavior is critical for and request/response protocol. I see 27-30 millisecond minimum latency on reads for data that takes <1ms to transfer, which is unacceptable.

Original change description:

Settings ReadIntervalTimeout and ReadTotalTimeoutMultiplier to MAXDWORD in the COMMTIMEOUTS structure on Windows makes Windows behave as normally expected; a read call to the serial port will return immediately if there is any data available, or if a single byte arrives in the buffer. If no data arrives, it will timeout out after the timeout specified in ReadTotalTimeoutConstant.

This fixes issues where reading from a serial port would take longer than desired when less data arrives that there is room for in the buffer passed to the read function.

See also: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-commtimeouts

Settings ReadIntervalTimeout and ReadTotalTimeoutMultiplier to MAXDWORD
in the COMMTIMEOUTS structure on Windows makes Windows behave as
normally expected; a read call to the serial port will return
immediately if there is any data available, or if a single byte arrives
in the buffer. If no data arrives, it will timeout out after the timeout
specified in ReadTotalTimeoutConstant.

This fixes issues where reading from a serial port would take longer
than desired when less data arrives that there is room for in the buffer
passed to the read function.

See also: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-commtimeouts
@estokes
Copy link
Collaborator

estokes commented Feb 8, 2023

Ping me when this is accepted in serialport and I'll give it a detailed look

@barafael
Copy link
Contributor

@estokes this has now been released with serialport-rs 4.4 :)

@vadixidav
Copy link

@larsch I may have encountered an issue with this as I have been testing it. It seems that you get the EOF condition somewhat unexpectedly with the serial port, causing read_exact to terminate with ErrorKind::UnexpectedEof. If you get 0 bytes back upon termination, you can't give 0 bytes to the caller, because the rules for read are that returning 0 bytes indicates an EOF condition. Instead, it needs to ignore the fact that 0 bytes were returned unless the serial port is truly "EOF", whatever that means for a serial port.

I might work up a fix for this if I can.

@vadixidav
Copy link

vadixidav commented Jul 9, 2024

Just based on my rudimentary understanding, which is not yet complete, I think that we need to detect somewhere (PollEvented or NamedPipeClient in tokio?) whether this 0 bytes occurred due to this timeout we set or whether the real end of file was reached. In the meantime I will be adding a hack somewhere to ignore this end of file condition for the serial port.

This might also not block this PR since this is a change forced upon tokio-serial or tokio rather than this crate, potentially.

parasyte added a commit to parasyte/mio-serial that referenced this pull request Jul 22, 2024
- Removes bad timeout overrides
- See:
  - berkowski#38
  - berkowski#47
  - serialport/serialport-rs#79
  - serialport/serialport-rs#73 (comment)
- Cleans up unused imports
@estokes
Copy link
Collaborator

estokes commented Dec 31, 2024

Given that the fix is accepted in serialport, would you like to amend this PR to completely remove the override?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants