vpelletier / python-libusb1

Python ctype-based wrapper around libusb1
GNU Lesser General Public License v2.1
168 stars 65 forks source link

No partial data when timeout occured in bulk reads #42

Closed gotzl closed 5 years ago

gotzl commented 5 years ago

When performing bulk reads from the device by using e.g. USBDeviceHandle.bulkRead(0x86, 0x1000, timeout=1000) and the timeout is hit, a 'libusb1.USBError' is raised and the partially transferred data is lost. I would've expected that bulkRead (or _bulkTransfer which actually calls the underlying libusb1) ignores the timeout error and yields the partial data.

My (hacky) workaround looks like this:

    def _bulkTransfer(self, endpoint, data, length, timeout):
        transferred = c_int()
        try: mayRaiseUSBError(libusb1.libusb_bulk_transfer(
            self.__handle, endpoint, data, length, byref(transferred), timeout,
        ))
        except USBError as e:
            if e.value != libusb1.LIBUSB_ERROR_TIMEOUT: raise e
        return transferred.value
vpelletier commented 5 years ago

Indeed, with C the number of transferred bytes would be available to caller, while here it is kept local and lost on exception.

I am tempted to special-case the timeout exception to hold this value, which would mean python-libusb1 caller code would become something like:

try:
  transferred = handle.bulkRead(...)
except usb1.USBErrorTimeout, exc:
  transferred = exc.transferred
  # timeout-specific handling, if any

Which means the exception has to be specially handled in _bulkTransfer, just a bit differently.

How does this look to you ?

gotzl commented 5 years ago

This looks very good. Having both the exception and the partial data is preferable. Thanks!

vpelletier commented 5 years ago

I just pushed an implementation to master, would you mind giving it a try ?

Also, I botched my example, as the exception on Read variants needs to hold the receive buffer (truncated to the number of received bytes, as would have been the returned value), the new received property. It is the Write variants which only get a transferred property.

gotzl commented 5 years ago

Your implementation works perfectly for me (although I've only tested bulkRead). Thanks for the fast response!

vpelletier commented 5 years ago

Thanks for testing. This change is now released as part of version 1.7 .