karalabe / hid

Gopher Interface Devices (USB HID)
Other
266 stars 132 forks source link

implement hidapi::read_timeout #11

Closed toudi closed 9 months ago

toudi commented 6 years ago

I had a piece of code using your hid library. Everything was working great, but I was struggling with timeouts. Initially, when I realized that your library did not bound to the underlying read_timeout, I did a similar thing to:

func (h *HIDDevice) readTimeout(b []byte, timeout int) (int, error) {
    var err error
    var num_read int
    // this channel is for notifying about succesful read
    finish := make(chan bool, 1)
    // close it up when this function ends
    defer close(finish)

    go func() {
        num_read, err = mydevice.Read(b)
        if !timeoutOccured {
            // this means that device is not closed and we can proceed to pass
            // the data
            finish <- true
        } else {
            log.Error("readTimeout timed out before the reading thread managed to return response")
        }
    }()

    // the select statement waits until one of cases happens. In our case:
    // either the Read() call would return some result, or a timeout would
    // occur
    select {
    case <-finish:
        return num_read, err
    case <-time.After(time.Duration(timeout) * time.Millisecond):
        timeoutOccured = true
        log.Errorf("Timeout occured during Read call")
        return 0, errors.New("Timeout occured during Read call")
    }
}

however, this led to several problems:

  1. the underlying read() call would actually return the data (post timeout) and I haven't figured out a clean way to deal with this scenario. I could have introduced an underlying buffer and then, the next call to readTimeout would have check this buffer, and return the data instead of repeating read() however this just seemed awkward approach
  2. without this buffer, the N-th read() call wouldn't get recordered, and thus the N+1'th send() would receive the data from N'th read()
  3. something else which I may have forgotten at this point, probably to do with goroutines synchronisation

Of course, I could have also implemented this timeout thing wrong.

All this led me to believe that I was trying to kick open the door which was already opened for me - namely - hidapi already implements read_timeout call so if hid would bind to it I could use it.

this PR does just that - it binds to read_timeout call. Thanks to this, my code simply uses this function as a blocking one (no more of goroutines / select calls). I would be very happy if you could point out if this is a right or wrong way to go, basically I am still learning golang so there is a good chance I messed something up.

thanks in advance, and once again, thank you for the wonderful binding!

bpedman commented 5 years ago

This is old but I also have a need for read with timeout and did kind of the same thing. Could we get this merged?

As for the implementation, technically you could just do

// Refactor existing Read function to take a timeout and rename it
func (dev *Device) ReadTimout(b []byte, timeout int) (int, error) {
 ... existing read implementation but just use C.hid_read_timeout
}

// Make Read function call ReadTimeout with timeout value of 0
func (dev *Device) Read(b []byte) (int, error) {
    return dev.ReadTimout(b, 0)
}

but not a big deal

toudi commented 5 years ago

@gballet do you want me to extract everything to a separate function and always call hid_read_timeout as @bpedman suggested ? I think that's a nice way of not repeating the code, but it's your call

bpedman commented 5 years ago

Forgot to mention I went ahead and implemented that variant a while ago, mainly so I could pull from a branch I controlled, but they are basically the same so I don't care which one is used, see #22

toudi commented 3 years ago

is there a chance that we could merge this or #22 ? I'm using a fork of mine in the program that I use but I think it could be beneficial to merge the changes upstream :)

holiman commented 9 months ago

Let's merge https://github.com/karalabe/hid/pull/22, it's a bit simpler. Thanks!