ziutek / ftdi

Go binding for libFTDI
Other
25 stars 14 forks source link

Device.Read can return 0,nil #16

Open rasky opened 3 years ago

rasky commented 3 years ago

Documentation of io.Reader says:

Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

It looks like Device.Read is a wrapper on ftdi_read_data: https://github.com/ziutek/ftdi/blob/f54995ade86ebc5fa08d67f6fb3e3ac99f69c66c/device.go#L441 which is documented that it can return 0 if no data is available on USB.

This BTW breaks io.ReadFull but maybe this is more a problem on Go side, given that returning 0,nil is "discouraged" but not "forbidden". Anyway, is there any way to more strictly adhere to io.Reader and have Device.Read automatically block until at least a byte is available?

ziutek commented 3 years ago

Hmm...

Generally this package is intended to be a thin wrapper over libftdi so you can read the doc of ftdi_read_data and expect the same behavior from Device.Read.

Even if we want to block if no data can be read from USB I wander how do it efficiently if the ftdi_read_data returns immediately...

rasky commented 3 years ago

Generally this package is intended to be a thin wrapper over libftdi so you can read the doc of ftdi_read_data and expect the same behavior from Device.Read.

If the function was called (*Device).ReadData I would have not objected. (*Device).Read implements io.Reader so you're not only providing a thin wrapper: you're actually promising to adhere to a specific protocol, which all Go users know and that is composable with many utilities with expect a specific behavior.

Even if we want to block if no data can be read from USB I wander how do it efficiently if the ftdi_read_data returns immediately...

I don't know libftdi well. I was hoping that one of the existing function allowed to either block, or wait for some timeout. If that's not the case, then I agree there's nothing we can do.

ziutek commented 3 years ago

You are right that user can rely on Read to implement io.Reader.

I've just read the ftdi_read_data source. It calls libusb_bulk_transfer with default timeout 5000 ms. So if it returns after 5 s we can simply return 0, ErrTimeout. I haven't tested this case myself, but if I understand you correctly the ftdi_read_data can return 0 immediately, so that's not the case.

Internally the ftdi_read_data reads two FTDI status bytes followed by data bytes. The status bytes are removed from returned data. It seems it returns 0 and not timeout error if it has read only these two status bytes. Maybe the subsequent call will wait more but probably no more than the duration set by Device.SetLatencyTimer that is 255 ms max.

If you have any FTxxx device on hand please test it with something like this:

d.SetLatencyTimer(ms)
for {
    t1 := time.Now()
    n, err := d.Read(buf)
    t2 := time.Now()
    fmt.Println(n, err, t2.Sub(t1))
}