Timeouts

[albenik]

Read/Write timeouts implemented

[cmaglie]

Hi @albenik

first of all thank you for the very high quality patch, as usual. I've tested some of the functions and they seems to work as expected.

I have some questions about the interface though:

	// Sets interbytes timeout
	// Values:
	//   x <= 0: disable interbyte timeout
	//   x >  0: set iterbyte timeout ot x milliseconds
	SetInterbyteTimeout(msec int) error

	// SetReadTimeout sets whole packet read timeout.
	// Values:
	//   x < 0: wait forever / until requested number of bytes are received.
	//   x = 0: non-blocking mode, return immediately in any case, returning zero or more,
	//          up to the requested number of bytes.
	//          NOTE: SetReadTimeout(0) for windows resets interbyte timeout to MAXDWORD
	//   x > 0: set timeout to x milliseconds returns immediately when the requested number of bytes are available,
	//          otherwise wait until the timeout expires and return all bytes that were received until then.
	SetReadTimeout(msec int) error

	// SetWriteTimeout set whole packet write timeout
	// For possible values refer to the list for read timeout values above.
	SetWriteTimeout(msec int) error

in particular I'm wondering how we can simplify it, here some thoughts:

1. do we need timeout on Write? what is the current behaviour of Write, is it blocking when the buffer passed as argument is too big?

2. about Read, are all these different kinds of timeouts really needed? after merging this PR we will have 4 modes:

• blocking: returns only if the buffer is filled (SetReadTimeout(-1))
• non-blocking: returns immediately (SetReadTimeout(0))
• timeout-interbyte: returns msec after the last received char or when the buffer is filled (SetInterbyteTimeout(msec))
• timeout-classic: returns msec after the call to Read or when the buffer is filled (SetReadTimeout(msec))

I found this very convoluted and IMHO we lose the only timeout that is useful and has any sense with a serial port:

• timeout-until-first-char: returns msec after the call to Read or after the first character received

the current blocking Read implementation blocks until the first character is received, so it's like this latter case "timeout-until-first-char" but with the timeout set to infinity. If possible I'd like to see only this timeout implemented and available.

3. Why the Timeout error type? couldn't the Read just return (0, nil) to tell that nothing has been received?

I'm sorry for being picky on this one, but I'd like to hear your opinion before this gets merged and the API becomes written into stone. Getting the API right is probably the most difficult part of software design, so I think it's better to discuss a bit more now.

In my vision, I want this library to be useful and simple, by providing only the most useful functions. On the other hand the "serial port" is probably one of the most over-engineered piece of hardware ever invented and I'd like to avoid implementing (and maintaining!) all of the possible permutations of functionality that the OS provides...

[albenik]

First of all, I bought most of implementation from https://github.com/pyserial/pyserial. I consider it as mature, well-designed library. So actually ideas not my, but pyserial community, and i believe to them.

1. I using write timeouts in my projects. I've seen cases then write blocks forever without write timeout.
2. In my work, some of vending peripheral hardware have strict requirements for interbyte timeout too. Of course I can read byte-by-byte and implementing two custom timeouts for next byte and for whole package, but if such function implemented on the OS level it very strange ignoring that. Unfortunately design of many hardware is very convoluted too. I made this improvement for my real work needs not for fun.
3. My mistake. For windows it's implemented as you expect, but for unix I forgot to fix it. I will fix it soon.

[albenik]

Fixed

[cmaglie]

I'm doing some more in-depth tests, I started on linux.
It seems that the default setting for timeout is changed from blocking to non-blocking, this is the test that I'm running:

package main

import "fmt"
import "log"
import "go.bug.st/serial.v1"
import "time"

func main() {
        port, err := serial.Open("/dev/ttyACM0", &serial.Mode{})
        if err != nil {
                log.Fatal(err)
        }
        fmt.Println("main: Port opened")

        go func() {
                fmt.Println("goroutine: reading")
                buff := make([]byte, 1000)
                n, err := port.Read(buff)
                fmt.Printf("goroutine: reading completed n=%d err=%+v\n", n, err)
        }()

        time.Sleep(time.Second*1)

        fmt.Println("main: Closing port")
        if err := port.Close(); err != nil {
                fmt.Printf("main: close err=%+v\n", err)
        }
        fmt.Println("main: Port closed")

        time.Sleep(time.Second*5)
}

with the current v1 the result is:

$ go run test_close.go 
main: Port opened
goroutine: reading
main: Closing port
goroutine: reading completed n=0 err=Port has been closed
main: Port closed

in this case the Read() blocks and is interrupted after the Close().
With the current PR the result is:

$ go run test_close.go 
main: Port opened
goroutine: reading
goroutine: reading completed n=0 err=<nil>
main: Closing port
main: Port closed

in this case the Read() exits immediately (with n=0 and err=nil).

This is a non-backward compatible change that I can't merge.
May you change the default to be "blocking" again?

PS: the testsuite I'm working on is based on this kind of regression test.

[cmaglie]

In my work, some of vending peripheral hardware have strict requirements for interbyte timeout too. Of course I can read byte-by-byte and implementing two custom timeouts for next byte and for whole package, but if such function implemented on the OS level it very strange ignoring that. Unfortunately design of many hardware is very convoluted too. I made this improvement for my real work needs not for fun.

I'm sure you did because you needed it :-)

I was asking if this interface could be simplified in some way (by reducing the number of different kind of timeouts for example) because, IMHO, it makes easier to achieve a consistent behavior across different OS and reduce the risk of regressions (by reducing the surface of code touched by the PR).

Adding so many different types of timeouts to me seemed to make this goal harder but, I repeat, it's just my impression, since you get inspiration by pyserial it's probably easier than it looks.

[albenik]

@cmaglie Sorry for delay. I am under hard pressure with my primary job needs. So I can answer for question and fix bugs a bit later.

[albenik]

Hi @cmaglie, sorry for delay.

Bug with timeout under *nix fixed.

And I have a new read timeouts interface proposal:

1. SetReadTimeout(t int) error — Primary method. t < 0 blocking mode, t == 0 nowait mode, t > 0 timeout mode
2. SetReadTimeoutEx(t, i uint32) error — Used for fine tune timeouts with secondary argument of interbyte timeout.
3. SetFirstByteReadTimeout(t uint32) error — Backward compatible timeout, for waiting at least one byte.

At most cases just one of (1) or (3) simple methods enough. For complex cases (2) method can help.

Do you accept this proposal?

[dlisin]

Hello friends,

I am very interested in non-blocking read mode support.
Any chance to have this pr merged?

[cmaglie]

I've got some free time latlely, I'll try to take a look again to this PR this weekend.

(wow 2.5 years passed already from the last comment... time flies... 😞)

[cbrake]

For the short term if anyone needs timeouts, you might consider this package which can be used to wrap any io.ReadWriter:

https://pkg.go.dev/github.com/simpleiot/simpleiot/respreader?tab=doc

I've been using it for modem control (AT commands), and for RS485 protocols like modbus -- seems to be working well.

[dlisin]

@cmaglie Since this PR is quite old - I have prepared a simplified version of it, which allows to define read timeouts (see #82)

Can you please take a look, once you have a time

[cmaglie]

Superseded by #109