hid_get_input_report: Correct number of bytes_returned

[JoergAtGithub]

This fixes #229

[Youw]

And definitely breaks something else

[JoergAtGithub]

And definitely breaks something else

What does it break? Do you have an example of an HID device where the old code returned the correct number of bytes? Can you share the HID Report Descriptor of this device and the number of read bytes returned?

[Youw]

So I guess hid_get_feature_report has to be updated as well?

[Youw]

@todbot any chance you could take a look at this? I don't have any device that uses all of: feature, input and interrupt reports.

[Be-ing]

ping @todbot

[Youw]

While unable to verify it myself, I tried to check MS documentation for details on HidD_GetInputReport/IOCTL_HID_GET_INPUT_REPORT and it seems that the patch is correct, as per documentation.
Note, hid_get_input_report has been implemented based on hid_get_feature_reports current implementation.

What bothers me here, is that same applies for IOCTL_HID_GET_FEATURE/HidD_GetFeature and it seems, that similar update is required for hid_get_feature_report.

But: bytes_returned++; has been added explicitly by Alan, as a bugfix. Is it mean that Alan and Petr Stehlík been both wrong at that time?

@JoergAtGithub, any chance you can verify the behavior of hid_get_feature_report ?

[JoergAtGithub]

I tried hid_get_feature_report with my device, which has the following Feature Reports:

For ReportID 241 it reads 3 bytes
For ReportID 243 it reads 4 bytes

I expected 3 bytes (including the byte for the ReportID) in both cases.

[Youw]

Why do you expect reading 3 bytes, if report size for both of the reports is 8 as per descriptor?

[JoergAtGithub]

1 byte ReportID + ReportSize (8Bit) * ReportCount(2)

[Youw]

In such case we're at square 1 again, as without this patch you'd get 2 and 3 byts.

What about the content of the buffer? Is it updated as expected?
What happens if you pass a larger buffer (e.g. of 5 bytes) ?

[JoergAtGithub]

I patched hid_get_input_report, and tested as requested hid_get_feature_report. The later is not related to my patch.

[Youw]

I understad that. I'm just trying to get the full picture, as I'd expect an idential usage from two almost identical functions with pretty much identical API.

[Youw]

One more thing.
Has it been tested on Win10?

[JoergAtGithub]

Win7

[mcuee]

http://janaxelson.com/hidpage.htm
The above website has Windows host software and device side FW, which may be useful for tests.

[mcuee]

I will try to dig out my device with my test FW last time I used to test libusb Windows HID backend. It is a simple mod of Jan's Generic HID FW.
https://github.com/mcuee/picusb/tree/master/lvr_genhid_mod
https://github.com/mcuee/picusb/blob/master/lvr_genhid_mod/libusb1_lvrhid8.c

[mcuee]

So I just got my example up and running with libusb HID backend. I need to update my test code (INTERFACE to MYINTERFACE to avoid conflict) a bit.

First pair (char) is report ID (Out report ID and Input report ID), the results seems to be okay
Second pair and third pair (char) are the real data, OUT and IN data should be the same, so the results seem to be okay as well.
Last pair (char) is kind of my coding mistake (should be for (i=1;i<=PACKET_INT_LEN-1; i++) question[i]=0x30+i;) but it is also a good test. There should be no extra data on the IN data so the results (0) are still correct.

I will see if I can translate the code to hidapi and then test again.

/c/work/libusb/picusb/lvr_genhid_mod
$ ./libusb1_lvrhid8.exe
Successfully find the LVR Generic HID device
Successfully set usb configuration 1
Successfully claimed MYINTERFACE
Testing control transfer using loop back test of feature report
03, 04; 21, 21; 22, 22; 23, 00;
Testing control transfer using loop back test of input/output report
02, 01; 31, 31; 32, 32; 33, 00;
Testing interrupt transfer using loop back test of input/output report
02, 01; 41, 41; 42, 42; 43, 00;

[mcuee]

I think I can use hidapitester.

/c/work/libusb/hidapitester
$ ./hidapitester.exe --vidpid 0925 --list-detail
0925/7001: Lakeview Research - Generic HID
  vendorId:      0x0925
  productId:     0x7001
  usagePage:     0xFF00
  usage:         0x0001
  serial_number: (null)
  interface:     -1
  path: \\?\hid#vid_0925&pid_7001#e&21fff50e&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}

[mcuee]

From my test, it seems to me the input report is correct with hidapi.

 /c/work/libusb/hidapitester
$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input
Opening device, vid/pid: 0x0925/0x7001
Writing output report of 3-bytes...wrote 3 bytes:
 02 21 22
Reading 3-byte input report 0, 250 msec timeout...read 3 bytes:
 01 21 22
Closing device

[mcuee]

From my test, it seems to me there is an issue with feature report readback with hidapi. It says reading 4 bytes.

 /c/work/libusb/hidapitester
$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-feature 3,0x31,0x32 --read-feature 4
Opening device, vid/pid: 0x0925/0x7001
Writing 3-byte feature report...wrote 3 bytes:
 03 31 32
Reading 3-byte feature report, report_id 4...read 4 bytes:
 04 31 32
Closing device

[mcuee]

Report Descriptor of the device.

ROM struct{BYTE report[HID_RPT01_SIZE];}hid_rpt01={
 {
   0x06, 0x00, 0xFF,               // Usage page (Vendor Defined Page1)
   0x09, 0x01,                     // Usage (Vendor Usage 1)
   0xA1, 0x01,                     // Collection (Application)

   // Global items apply to all reports below

   0x15, 0x00,                     // Logical Minimum (0)
   0x26, 0xFF, 0x00,               // Logical Maximum (255)
   0x75, 0x08,                     // Report Size (8 bits)
   0x95, 0x02,                     // Report Count (2 fields)

   // Input report
   0x85, 0x01,                     // Report ID (1)
   0x09, 0x00,                     // Usage (Undefined)
   0x82, 0x02, 0x01,               // Input (Data,Var,Abs,Buf)

   // Output report
   0x85, 0x02,                     // Report ID (2)
   0x09, 0x00,                     // Usage (Undefined)
   0x92, 0x02, 0x01,               // Output (Data,Var,Abs,Buf)

   // Feature report
   0x85, 0x03,                     // Report ID (3)
   0x09, 0x00,                     // Usage (Undefined)
   0xB2, 0x02, 0x01,               // Feature (Data,Var,Abs,Buf)

   // Feature report
   0x85, 0x04,                     // Report ID (4)
   0x09, 0x00,                     // Usage (Undefined)
   0xB2, 0x02, 0x01,               // Feature (Data,Var,Abs,Buf)

   0xC0                            // end collection
 }

[mcuee]

#229

Unfortunately hidapitester does not test the correctness of hid_get_input_report().

[mcuee]

I believe the changes is correct based on my testing and analysis in #229. Similar changes need to be implemented for feature report as well.

[mcuee]

I believe the changes is correct based on my testing and analysis in #229. Similar changes need to be implemented for feature report as well.

The change is also confirmed by Tim Roberts' answer to my query in libusb-devel mailing list. Similar changes need to be implemented for feature report as well.

[mcuee]

@JoergAtGithub or @Youw Is it possible for you to create another pull request for feature report? I am poor at git myself and I do not want to touch the code for hidapi (or libusb as well).

diff --git a/windows/hid.c b/windows/hid.c
index f0013a5..74a05bc 100644
--- a/windows/hid.c
+++ b/windows/hid.c
@@ -876,11 +876,6 @@ int HID_API_EXPORT HID_API_CALL hid_get_feature_report(hid_device *dev, unsigned
                return -1;
        }

-       /* bytes_returned does not include the first byte which contains the
-          report ID. The data buffer actually contains one more byte than
-          bytes_returned. */
-       bytes_returned++;
-
        return bytes_returned;
 #endif
 }

[Youw]

Yes, I will.