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

Add read_until_timeout #17

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

Add read_until_timeout #17

wants to merge 1 commit into from

Conversation

RossSmyth
Copy link

A method that reads as many bytes as possible until either the size specified is reached, or a timeout occurs. Helpful for quick testing of serial protocols.

A method that reads as many bytes as possible until either the size specified is reached, or a timeout occurs. Helpful for quick testing of serial protocols.
@RossSmyth
Copy link
Author

I already had a working implementation of what I described in #7 so I just cleaned it up a bit.

@de-vri-es
Copy link
Owner

I like the idea of easier utility functions.

I'm thinking maybe the SerialPort shouldn't return Err(TimedOut), but just Ok(0) when a timeout occurs. Then read_to_end() just works, and the documentation of Read seems to allow it.

@RossSmyth
Copy link
Author

It just depends on what the API should mean, as something like what read_to_end() is now could probably be useful. As currently it is "read to the end of the OS's internal serial port buffer or wait until there is something in the buffer" while this API is "read to the end of a serial transmission"

/// assert_eq!(read_to_timeout(&mut port, None), );
/// assert_eq!(port, [0x48, 0x49, 0x20, 0x46, 0x45, 0x52, 0x52, 0x49, 0x53]);
/// ```
pub fn read_until_timeout(&self, size: Option<NonZeroUsize>) -> std::io::Result<Vec<u8>> {

Choose a reason for hiding this comment

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

Instead of this

pub fn read_until_timeout(&self, size: Option<NonZeroUsize>) -> std::io::Result<Vec<u8>> {

You could do this

pub fn read_until_timeout(&self, buffer: &mut [u8]) -> usize{

the function would then read into the buffer slice and return the number of bytes successfully into the buffer. If a timeout occured, then obviously the return value will < buff.len()

this aligns with how many other apis work as well, and is also nostd ( embedded ) friendly as the buffer could just be a slice into a static allocated array

Copy link

@DanielJoyce DanielJoyce Apr 10, 2024

Choose a reason for hiding this comment

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

Also, the timeout bounds is kinda common for all of these methods, so does not really need to be explained.

pub fn read_into(...)

Copy link
Author

Choose a reason for hiding this comment

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

My primary use is an API that doesn't really care about the size and more about the timeout, which is why I named it such.

You do showcase something interesting which is that this API is in essence very similar to read_exact. Just it special cases timeout as being Ok, which you can do manually with something like

match port.read_exact(&mut buf) {
    Ok(_) => (),
    Err(err) if err.kind() == std::io::ErrorKind::TimedOut => (),
    Err(err) => return Err(err),
}

Though this does rely on the implementation detail of this library which is that the buffer is written to in the loop.

Copy link
Owner

@de-vri-es de-vri-es Apr 15, 2024

Choose a reason for hiding this comment

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

read_exact using a loop is something you can rely on. There is no other way, since the contract of the underlying read syscall is that it can return less than the requested bytes. At-least on Unix, and we try to get that behavior on Windows too.

I'm still considering if this should be fixed by a timeout returned Ok(0) instead of an error or not.

If we do, read_exact will still fail with UnexpectedEof (instead of TimedOut). But read_to_end() will return Ok.

From the standard library documentation of read():

If the return value of this method is Ok(n), then implementations must guarantee that 0 <= n <= buf.len(). A nonzero n value indicates that the buffer buf has been filled in with n bytes of data from this source. If n is 0, then it can indicate one of two scenarios:

  1. This reader has reached its “end of file” and will likely no longer be able to produce bytes. Note that this does not mean that the reader will always no longer be able to produce bytes. As an example, on Linux, this method will call the recv syscall for a TcpStream, where returning zero indicates the connection was shut down correctly. While for File, it is possible to reach the end of file and get zero as result, but if more data is appended to the file, future calls to read will return more data.
  2. The buffer specified was 0 bytes in length.

So the question is: should we mimick the guarantees of a TcpStream or a File? Conceptually, a serial port is more similar to a TcpStream than a File, so that kinda hints to keeping the timeout as an error instead of Ok(0).

But then a convenience function that reads until a timeout (and returns Ok) is certainly useful.

@DanielJoyce
Copy link

Could also call it "read_until_deadline", since we expect the timeout to occur, don't return the timeout error case.

@DanielJoyce
Copy link

One tricky bit is avoiding skipping/dropping bytes because of interrupting the read, etc.

Read a small buffer in a loop, document the tolerance / deadline behavior, etc.

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.

3 participants