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

Discrepancy in returned number of read bytes in hid_get_feature_report() #182

Open
joysfera opened this issue Aug 17, 2014 · 6 comments
Open

Comments

@joysfera
Copy link

There's a problematic discrepancy between libusb's and windows implementation of hid_get_feature_report() for report ID = 0. The libusb's implementation contains a special code for zero report ID that adds 1 to number of returned bytes. However, the windows code does not do that. Thus the same code calling HIDAPI gets different number of read bytes when it's run on Linux than when it's run on Windows.

The API documentation says that "this function returns the number of bytes read". There's no note of extra byte for Report ID, contrary to the input parameter 'length'. Thus I'd say the libusb's implementation is wrong with adding 1 to the number of returned bytes.

Also, technically, the USB device sent only X bytes and the driver read only X bytes, not X+1. So I'd be inclined to removing the extra code from libusb's implementation. I realize that the returned number of read bytes is then different than the value sent in the 'length' parameter but that shouldn't matter as the length is mainly the size of the allocated buffer, not the number of bytes that are to be read.

I also realize that the buffer contains one more byte than what is returned as the number of read bytes. Hmmm. That's not consistent with result of the same call with non-zero report ID, right?

Anyway, please go ahead and fix it either way, it just needs to be consistent on all supported platforms so that the code calling HIDAPI does not have to have special code for each platform. Thank you!

@signal11
Copy link
Owner

There's a problematic discrepancy between libusb's and windows implementation of hid_get_feature_report() for report ID = 0. The libusb's implementation contains a special code for zero report ID that adds 1 to number of returned bytes. However, the windows code does not do that.

Windows does it for you. Windows always uses a report id in its API (including that DeviceIoControl call), regardless of whether there's one on the bus. On the bus, there's no report ID if numbered reports are not being used, but there is if numbered reports are being used. In the API there's a report ID every time.

The API documentation says that "this function returns the number of bytes read". There's no note of extra byte for Report ID, contrary to the input parameter 'length'.

It's more complicated than that. It's an extra byte only when not using numbered reports.

Also, technically, the USB device sent only X bytes and the driver read only X bytes, not X+1. So I'd be inclined to removing the extra code from libusb's implementation.

Well that's not going to happen. It needs to be consistent across platforms, and I believe it is. Maybe the documentation should be updated to reflect the extra byte.

I also realize that the buffer contains one more byte than what is returned as the number of read bytes. Hmmm. That's not consistent with result of the same call with non-zero report ID, right?

That's right. On the bus, there's no report ID at all transferred if you're not using numbered reports. HIDAPI is different. There is always a report number for these functions.

Anyway, please go ahead and fix it either way, it just needs to be consistent on all supported platforms so that the code calling HIDAPI does not have to have special code for each platform. Thank you!

I believe it already is, and you haven't shown me an example where it is not. Do you have some code and a device which shows different values for this on different platforms? What device are you using anyway that has feature reports. It's fairly rare in commercial custom HID devices.

Alan.

@joysfera
Copy link
Author

Alan Ott pí¹e v Ne 17. 08. 2014 v 16:04 -0700:

    There's a problematic discrepancy between libusb's and windows
    implementation of hid_get_feature_report() for report ID = 0.
    The libusb's implementation contains a special code for zero
    report ID that adds 1 to number of returned bytes. However,
    the windows code does not do that. 

Windows does it for you.

It does not. Windows 7/8.1 returns 40 (in my case) which is the count
defined in descriptor and the amount of data actually returned by the
device. Linux (thanks to the extra code in HIDAPI) returns 41. That's
the discrepancy.

    Also, technically, the USB device sent only X bytes and the
    driver read only X bytes, not X+1. So I'd be inclined to
    removing the extra code from libusb's implementation.

Well that's not going to happen. It needs to be consistent across
platforms, and I believe it is.

If it was consistent I wouldn't bother creating this issue. If you, like
me, want it indeed consistent then please add the same extra code to the
Windows part of HIDAPI so it would also return 41.

I can go ahead and create a patch and test it for you if you wish.

Do you have some code and a device which shows different values for
this on different platforms?

yes, of course. As I said above, I had a code working well under Linux
that was counting on 41 returned bytes but it failed under Windows.

What device are you using anyway that has feature reports.

it's my own device, created using V-USB.

It's fairly rare in commercial custom HID devices.

I've been searching for the simplest descriptor that would allow me
passing data via USB and happened to find a simple descriptor with the
feature report that took me only two days to get working under
MS-Windows - with the exception of the number of returned bytes, that's
the last thing to get solved.

Thanks,

Petr

@signal11
Copy link
Owner

You're right. Windows doesn't do what I thought it did. Fixed on the Windows side:
pushed 54eb31d

Also fixed documentation in hidapi.h to reflect what we discussed.

Thanks!

@kristofferkoch
Copy link

Hi. I'm just reviewing hidapi to check the current state. Should this issue be closed?

@joysfera
Copy link
Author

yes

@mirh
Copy link

mirh commented Jan 12, 2017

No, it broke #192

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

No branches or pull requests

4 participants