hid_get_input_report: Correct number of bytes_returned

JoergAtGithub commented

2021-01-16 03:30:44 -06:00

(Migrated from github.com)

This fixes https://github.com/libusb/hidapi/issues/229

Youw commented

2021-01-27 04:19:01 -06:00

(Migrated from github.com)

And definitely breaks something else

JoergAtGithub commented

2021-01-27 14:50:37 -06:00

(Migrated from github.com)

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?

> 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?

👍 1

Youw commented

2021-01-27 17:37:36 -06:00

(Migrated from github.com)

So I guess hid_get_feature_report has to be updated as well?

So I guess `hid_get_feature_report` has to be updated as well?

Youw commented

2021-01-27 17:38:24 -06:00

(Migrated from github.com)

@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 commented

2021-02-17 16:51:26 -06:00

(Migrated from github.com)

ping @todbot

Youw commented

2021-03-07 06:15:40 -06:00

(Migrated from github.com)

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 ?

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_report`s 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](https://github.com/libusb/hidapi/commit/54eb31dc16dcc67d0b689ed947bc53a038608c0e) 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 commented

2021-03-20 04:48:47 -05:00

(Migrated from github.com)

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.

I tried hid_get_feature_report with my device, which has the following Feature Reports: ![grafik](https://user-images.githubusercontent.com/64457745/111865775-4826b600-8969-11eb-964d-b37bc653c607.png) 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 commented

2021-03-20 09:39:36 -05:00

(Migrated from github.com)

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

JoergAtGithub commented

2021-03-20 09:41:31 -05:00

(Migrated from github.com)

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

Youw commented

2021-03-20 09:48:40 -05:00

(Migrated from github.com)

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) ?

In such case we're at square 1 again, as without [this patch](https://github.com/libusb/hidapi/commit/54eb31dc16dcc67d0b689ed947bc53a038608c0e) 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 commented

2021-03-20 09:51:26 -05:00

(Migrated from github.com)

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

Youw commented

2021-03-20 09:53:18 -05:00

(Migrated from github.com)

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 commented

2021-03-20 09:56:05 -05:00

(Migrated from github.com)

One more thing.
Has it been tested on Win10?

One more thing. Has it been tested on Win10?

JoergAtGithub commented

2021-03-20 09:56:40 -05:00

(Migrated from github.com)

Win7

mcuee commented

2021-06-13 07:44:23 -05:00

(Migrated from github.com)

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

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

mcuee commented

2021-06-13 07:48:57 -05:00

(Migrated from github.com)

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

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

👍 1

mcuee commented

2021-06-14 01:35:20 -05:00

(Migrated from github.com)

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;

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 commented

2021-06-14 01:38:09 -05:00

(Migrated from github.com)

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}

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 commented

2021-06-14 02:00:09 -05:00

(Migrated from github.com)

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

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 commented

2021-06-14 02:03:57 -05:00

(Migrated from github.com)

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

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 commented

2021-06-14 02:06:10 -05:00

(Migrated from github.com)

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
 }

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 commented

2021-06-14 07:33:17 -05:00

(Migrated from github.com)

https://github.com/libusb/hidapi/issues/229

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

https://github.com/libusb/hidapi/issues/229 Unfortunately hidapitester does not test the correctness of hid_get_input_report().

mcuee commented

2021-06-15 20:41:55 -05:00

(Migrated from github.com)

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.

> 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 commented

2021-06-17 22:09:35 -05:00

(Migrated from github.com)

@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
 }

@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 commented

2021-06-18 05:18:34 -05:00

(Migrated from github.com)

Yes, I will.

👍 1

Rows
Columns

hid_get_input_report: Correct number of bytes_returned #232