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

hid_get_input_report returns one byte more than hid_read, also how to deal with hid_get_feature_report #229

Closed
JoergAtGithub opened this issue Jan 3, 2021 · 30 comments · Fixed by #232
Labels
bug Something isn't working Windows Related to Windows backend

Comments

@JoergAtGithub
Copy link
Contributor

hid_get_input_report returns one byte more than hid_read

If I read the same report using hid_get_input_report and than with hid_read, the return value is I get different number of byes read as return value.

This is for the case of a single input report (I reported a maybe related issue about for the case of two input reports here #210 )

This occurs on Windows 7 with HIDAPI 0.10.0

@Youw Youw added the Windows Related to Windows backend label Jan 5, 2021
@JoergAtGithub
Copy link
Contributor Author

JoergAtGithub commented Jan 7, 2021

The code of hid_get_input_report contains:
/* 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++;

I compared this with the implementation in Google Chrome (WebHID backend), and they read the data using the same API, but do not increment the byte count:
https://chromium.googlesource.com/chromium/src/+/cebcd836b83e1d48db1000b2a5878aa90bc7e88b/services/device/hid/hid_connection_win.cc

@jonasmalacofilho
Copy link
Contributor

jonasmalacofilho commented Mar 31, 2021

(Arriving here from a reference in another issue...)

That is the documented behavior of hid_get_input_report when report IDs are not used, which could be the case here, since there is a single report.

hidapi/hidapi/hidapi.h

Lines 370 to 376 in 6a01f3b

/** @brief Get a input report from a HID device.
Set the first byte of @p data[] to the Report ID of the
report to be read. Make sure to allow space for this
extra byte in @p data[]. Upon return, the first byte will
still contain the Report ID, and the report data will
start in data[1].

hid_read, indeed, follows different semantics.

hidapi/hidapi/hidapi.h

Lines 271 to 275 in 6a01f3b

/** @brief Read an Input report from a HID device.
Input reports are returned
to the host through the INTERRUPT IN endpoint. The first byte will
contain the Report number if the device uses numbered reports.


Addendum

That is at least a reasonable interpretation of the documentation for hid_get_input_report. However, the same text appears in hid_get_feature_report which, in turn, seems to have conflicting implementations:

  • the libusb implementation appears to follow the interpretation above;
  • but the hidraw implementation simply wraps HIDIOCGFEATURE, which is documented to have a different behavior: "For devices which do not use numbered reports, the report data will begin at the first byte of the returned buffer."

@mcuee
Copy link
Member

mcuee commented Jun 13, 2021

http://janaxelson.com/hidpage.htm
Jan Axelson's page may be of some interests. Including FW codes to be used for test device.

@mcuee mcuee changed the title hid_get_input_report returns one byte more than hid_read hid_get_input_report returns one byte more than hid_read, also how to deal with hid_get_feature_report Jun 13, 2021
@mcuee
Copy link
Member

mcuee commented Jun 13, 2021

Historical info about feature report.
https://marc.info/?l=libusb-devel&m=128378733122118&w=2

https://github.com/libusb/hidapi/blob/master/windows/hid.c#L56
#define IOCTL_HID_GET_FEATURE                         HID_OUT_CTL_CODE(100)
#define IOCTL_HID_GET_INPUT_REPORT              HID_OUT_CTL_CODE(104)

List: libusb-devel
Subject: Re: [Libusb-devel] Returned HID feature report size on windows
From: Alan Ott <alan () signal11 ! us>
Date: 2010-09-06 15:35:11
Message-ID: 4C850A2F.5090509 () signal11 ! us
[Download RAW message or body]

On 09/04/2010 10:18 PM, Xiaofan Chen wrote:

ReadFile() is generic and should be able to be used for
input/output/feature reports.

...

You may want to get some hints from Alan Ott's code. Now
he is using ReadFile for Feature Report.
http://github.com/signal11/hidapi/blob/master/windows/hid.cpp

ReadFile() will only give input reports which came back on the
Interrupt-IN endpoint. To get a feature report and also its length, you
have to use the DeviceIoControl() call instead of HidD_GetFeature(). The
problem is, the definitions of the IOCTLs are only in the DDK, and
they're not even in the same place as the hid.dll headers. Further,
there are two sets of IOCTLs, one for kernel-space, and one for
user-space, which do many of the same things.

I was eventually able to figure it out, and if you look here:
http://github.com/signal11/hidapi/blob/master/windows/hid.cpp

at the hid_get_feature_report() function, you'll see what works.

... skipped ...

@mcuee
Copy link
Member

mcuee commented Jun 13, 2021

Ref from Microsoft, which is said not to be so clear.
https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/obtaining-hid-reports

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

Please check my test result also.
#232

Test FW and libusb code
https://github.com/mcuee/picusb/tree/master/lvr_genhid_mod

hidapitester results: it seems input report is correct but feature report has an issue as it reads back 4 bytes.


$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input 1
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

$ ./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
Copy link
Member

mcuee commented Jun 14, 2021

I just purposefully send one more byte and here is the test result. No issues with the input report and feature report.

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

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

@Youw
Copy link
Member

Youw commented Jun 14, 2021

If I understand your results correctly, hid_get_input_report actually works fine, but hid_get_feature_report doesn't.
This makes #232 invalid.
UPD: comments below.


@JoergAtGithub something I've failed to ask you earlier:
Does you device uses numbered reports?

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

I am using hidapitester.
https://github.com/todbot/hidapitester
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
Copy link
Member

mcuee commented Jun 14, 2021

Form the code of hidapitester.c, it is using hid_read_timeout() for input report and hid_get_feature_report() for feature report read.
https://github.com/todbot/hidapitester/blob/master/hidapitester.c

So based on my test result hid_read_timeout() is correct for input report. hid_get_feature_report() is a bit wrong for feature report in terms of the bytes reading. hidapitester does not test the correctness of hid_get_input_report().

@Youw
Copy link
Member

Youw commented Jun 14, 2021

hidapitester does not test the correctness of hid_get_input_report().

That's unfortunate. Do you have other way to test it? (maybe modify hidapitester?)

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

I will see what I can do. I need to brush up my C skills (very low anyway).

Interesting under Linux it seems to me hid_get_feature_report() is correct. I am using Raspberry Pi 400 running Ubuntu Linux 64bit.

hidapitester on  master ❯ sudo ./hidapitester --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 3 bytes:
 04 31 32
Closing device

However I have issues with hid_read() under Linux.

hidapitester on  master ❯ sudo ./hidapitester --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input 1
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 0 bytes:
Closing device

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

No issues under macOS with my Mac Mini M1.


hidapitester on  master [?] ❯ ./hidapitester --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 3 bytes:
 04 31 32
Closing device
hidapitester on  master [?] ❯ ./hidapitester --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input 1
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

@Youw
Copy link
Member

Youw commented Jun 14, 2021

Yeah those are all different implementations, and the issue seem to be only with Win backend.

As for Linux read - hidraw or libusb? In both cases you shouldn't have any issues, this seem weird.

@Youw
Copy link
Member

Youw commented Jun 14, 2021

I will see what I can do. I need to brush up my C skills (very low anyway).

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

For the purpose if this test, I think the easiest way would be to replace all hid_get_feature_report with hid_get_input_report in hidapitester

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

I will test under Windows later, but my quick fix does not produce good result under macOS.


diff --git a/hidapitester.c b/hidapitester.c
index b4ae3d9..a5f116f 100644
--- a/hidapitester.c
+++ b/hidapitester.c
@@ -363,8 +363,7 @@ int main(int argc, char* argv[])
                 msg("wrote %d bytes:\n", res);
                 if(!msg_quiet) { printbuf(buf,buflen, print_base, print_width); }
             }
-            else if( cmd == CMD_READ_INPUT ||
-                     cmd == CMD_READ_INPUT_FOREVER ) {
+            else if( cmd == CMD_READ_INPUT_FOREVER ) {
 
                 if( !dev ) {
                     msg("Error on read: no device opened.\n"); break;
@@ -388,6 +387,23 @@ int main(int argc, char* argv[])
                     }
                 } while( cmd == CMD_READ_INPUT_FOREVER );
             }
+            else if( cmd == CMD_READ_INPUT ) {
+
+                if( !dev ) {
+                    msg("Error on read: no device opened.\n"); break;
+                }
+                if( !buflen) {
+                    msg("Error on read: buffer length is 0. Use --len to specify.\n");
+                    break;
+                }
+                uint8_t report_id = (optarg) ? strtol(optarg,NULL,10) : 0;
+                buf[0] = report_id;
+                msg("Reading %d-byte input report, report_id %d...",buflen, report_id);
+                res = hid_get_input_report(dev, buf, buflen);
+                msg("read %d bytes:\n",res);
+                printbuf(buf, buflen, print_base, print_width);
+            }
+            
             else if( cmd == CMD_READ_FEATURE ) {
 
                 if( !dev ) {

hidapitester on  master [!?] ❯ ./hidapitester --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input 1
Opening device, vid/pid: 0x0925/0x7001
Writing output report of 3-bytes...wrote 3 bytes:
 02 21 22
Reading 3-byte input report, report_id 0...read -1 bytes:
 00 00 00
Closing device
hidapitester on  master [!?] ❯ ./hidapitester --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input-forever 1
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
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...^C

@Youw
Copy link
Member

Youw commented Jun 14, 2021

report_id 0

This means you're not using numbered reports, but your device does use them (numbered reports). Report ID need to be explicitly specified, and be non-zero.

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

Yes I already specified input report ID as 1 as the parameter for hidapitester. So looks like there are other problems somewhere.

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

Looks like my quick fix is not correct. I got the same results under Windows.


$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input 1
Opening device, vid/pid: 0x0925/0x7001
Writing output report of 3-bytes...wrote 3 bytes:
 02 21 22
Reading 3-byte input report, report_id 0...read -1 bytes:
 00 00 00
Closing device

$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input-forever 1
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
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...read 0 bytes:
Reading 3-byte input report 0, 250 msec timeout...

$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-feature 3,0x21,0x22 --read-feature 4
Opening device, vid/pid: 0x0925/0x7001
Writing 3-byte feature report...wrote 3 bytes:
 03 21 22
Reading 3-byte feature report, report_id 4...read 4 bytes:
 04 21 22
Closing device


@Youw
Copy link
Member

Youw commented Jun 14, 2021

Does you device/FW support sending syncronous input reports?
I know that with the devices I use, if a report can be hid_read (basically the device sends me interruptions on a specific events), I cannot get_input_report it - only those that are specifically designed/implemented that way.

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

The device supports reading input report using either interrupt transfer or control transfer. I have tested with libusb Windows HID backend.

I got the same results using hidpytoy, it also use hid_read for input reports.
https://github.com/todbot/hidpytoy/blob/master/src/main/python/main.py

And I got the same results with feature report that one extra byte is read using hidpytoy and hidapitester, so there is an issue with Windows hid_get_feature_report. I will not be surprised that hid_get_input_report has the same issue.

@Youw
Copy link
Member

Youw commented Jun 14, 2021

Yeah, no surprise 1, 2 lol.

I guess I've trusted (and didn't check so far) the original commit too much.

@mcuee
Copy link
Member

mcuee commented Jun 14, 2021

FYI on the device FW and libusb test program. For this paticular device FW, libusb HID backend seems to work. But there are other cases it does not work. You can see this report ID is causing lots of problem for Windows HID related codes, due to the unclear documentation from Microsoft.
libusb/libusb#902

My picusb repo:
https://github.com/mcuee/picusb/tree/master/lvr_genhid_mod

I need to have a minor fix to build under MSYS2 MinGW-w64.

$ git diff
diff --git a/lvr_genhid_mod/libusb1_lvrhid8.c b/lvr_genhid_mod/libusb1_lvrhid8.c
index 6616c4b..ddd1372 100644
--- a/lvr_genhid_mod/libusb1_lvrhid8.c
+++ b/lvr_genhid_mod/libusb1_lvrhid8.c
@@ -25,7 +25,7 @@

 const static int PACKET_CTRL_LEN=3;
 const static int PACKET_INT_LEN=3;
-const static int INTERFACE=0;
+const static int MYINTERFACE=0;
 const static int ENDPOINT_INT_IN=0x81; /* endpoint 0x81 address for IN */
 const static int ENDPOINT_INT_OUT=0x01; /* endpoint 1 address for OUT */
 const static int TIMEOUT=5000; /* timeout in ms */
@@ -166,7 +166,7 @@ int main(void)
                goto out;
        }
        printf("Successfully set usb configuration 1\n");
-       r = libusb_claim_interface(devh, 0);
+       r = libusb_claim_interface(devh, MYINTERFACE);
        if (r < 0) {
                fprintf(stderr, "libusb_claim_interface error %d\n", r);
                goto out;

$ cd lvr_genhid_mod/

$ ./libusb1_lvrhid8.exe
Successfully find the LVR Generic HID device
Successfully set usb configuration 1
Successfully claimed interface
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
Copy link
Member

mcuee commented Jun 15, 2021

Yeah, no surprise 1, 2 lol.

I guess I've trusted (and didn't check so far) the original commit too much.

My anlaysis below and I believe hidapi current codes are wrong. I have asked in the libusb-devel mailing list to confirm.

The following article is from the Windows Hardware Developer site.

https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/obtaining-hid-reports

This topic discusses the obtaining of HID input reports or HID feature reports,
by user-mode applications using ReadFile or the HidD_GetXxx routines.

However, an application should only use the HidD_GetXxx routines to obtain the current state of a device. 
If an application attempts to use HidD_GetInputReport to continuously obtain input reports, the reports
can be lost. In addition, some devices might not support HidD_GetInputReport, and will become 
unresponsive if this routine is used.

Using IOCTL_HID_GET_Xxx Requests
A driver can use the following I/O requests to obtain the most current input and feature reports from a 
HID collection:
IOCTL_HID_GET_INPUT_REPORT Returns an input report from a HID collection (Windows XP and later versions).
IOCTL_HID_GET_FEATURE Returns a feature report from a HID collection.

So you can see that HIDAPI is using ReadFile for the fetching of HID reports continuously with the
hid_read API. And then it does not use HidD_GetXxx routines to obtain the most current input report
and feature report, rather it uses IOCTLs to get the input report and feature report.

https://github.com/libusb/hidapi/blob/master/windows/hid.c#L56
#define IOCTL_HID_GET_FEATURE HID_OUT_CTL_CODE(100)
#define IOCTL_HID_GET_INPUT_REPORT HID_OUT_CTL_CODE(104)

Historical reasons why Alan Ott chose to use DeviceIoControl() call and not HidD_GetXxx.
#229

So it seems to me that the Microsoft documentation may be a bit confusing to say the driver can use
the two IOCTLs. Usually driver means kernel space in this context even though there are user mode
drivers as well.

Now the question is the return data of the two IOCTLs (output buffer). From what I read, I believe
HIDAPI codes are wrong in both cases, as Microsoft documentation says that the first byte of
the buffer is unchanged (report ID or 0 if no report ID).
https://github.com/libusb/hidapi/blob/master/windows/hid.c#L933
https://github.com/libusb/hidapi/blob/master/windows/hid.c#L884

https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/hidclass/ni-hidclass-ioctl_hid_get_input_report
https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/hidclass/ni-hidclass-ioctl_hid_get_feature
Output buffer
The Irp->MdlAddress member points to the requester-allocated output buffer that the HID class driver
uses to return the input report. The first byte of the buffer, which the requester uses to input a report ID
or zero, is unchanged. The input report -- excluding its report ID, if report IDs are used -- is returned at
((PUCHAR)Irp->MdlAddress + 1).

Output buffer length
The size of the input report.

@mcuee
Copy link
Member

mcuee commented Jun 15, 2021

More proper fix for hidapitester:

diff --git a/hidapitester.c b/hidapitester.c
index b4ae3d9..8dcdbb5 100644
--- a/hidapitester.c
+++ b/hidapitester.c
@@ -84,6 +84,7 @@ enum {
     CMD_SEND_OUTPUT,
     CMD_SEND_FEATURE,
     CMD_READ_INPUT,
+       CMD_READ_INPUT_ALT,
     CMD_READ_FEATURE,
     CMD_READ_INPUT_FOREVER,
 };
@@ -204,6 +205,7 @@ int main(int argc, char* argv[])
          {"send-out",     required_argument, &cmd,   CMD_SEND_OUTPUT},
          {"send-feature", required_argument, &cmd,   CMD_SEND_FEATURE},
          {"read-input",   optional_argument, &cmd,   CMD_READ_INPUT},
+                {"read-input-alt",   optional_argument, &cmd,   CMD_READ_INPUT_ALT},
          {"read-in",      optional_argument, &cmd,   CMD_READ_INPUT},
          {"read-feature", required_argument, &cmd,   CMD_READ_FEATURE},
          {"read-input-forever",  optional_argument, &cmd,   CMD_READ_INPUT_FOREVER},
@@ -387,6 +389,22 @@ int main(int argc, char* argv[])
                         break;
                     }
                 } while( cmd == CMD_READ_INPUT_FOREVER );
+            }
+                       else if( cmd == CMD_READ_INPUT_ALT ) {
+
+                if( !dev ) {
+                    msg("Error on read: no device opened.\n"); break;
+                }
+                if( !buflen) {
+                    msg("Error on read: buffer length is 0. Use --len to specify.\n");
+                    break;
+                }
+                uint8_t report_id = (optarg) ? strtol(optarg,NULL,10) : 0;
+                buf[0] = report_id;
+                msg("Reading %d-byte input report using hid_get_input_report, report_id %d...",buflen, report_id);
+                res = hid_get_input_report(dev, buf, buflen);
+                msg("read %d bytes:\n",res);
+                printbuf(buf, buflen, print_base, print_width);
             }
             else if( cmd == CMD_READ_FEATURE ) {

And proposed fix to hidapi (including #232)

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
 }
@@ -925,11 +920,6 @@ int HID_API_EXPORT HID_API_CALL hid_get_input_report(hid_device *dev, unsigned c
                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
 }

@mcuee
Copy link
Member

mcuee commented Jun 15, 2021

With the above patch, the results are the same as macOS and Linux. So unfortunately my device is not suitable to test hid_get_input_report(). But at least it confirms the issue on hid_get_feature_report.


$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-feature 3,0x21,0x22 --read-feature 4
Opening device, vid/pid: 0x0925/0x7001
Writing 3-byte feature report...wrote 3 bytes:
 03 21 22
Reading 3-byte feature report, report_id 4...read 3 bytes:
 04 21 22
Closing device

$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input-alt 1
Opening device, vid/pid: 0x0925/0x7001
Writing output report of 3-bytes...wrote 3 bytes:
 02 21 22
Reading 3-byte input report using hid_get_input_report, report_id 0...read -1 bytes:
 00 00 00
Closing device

$ ./hidapitester.exe --vidpid 0925:7001 -l 3 --open --send-output 2,0x21,0x22 --read-input 1
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
Copy link
Member

mcuee commented Jun 15, 2021

@JoergAtGithub Did you test the device without a report ID? If that is tested, then I think #232 is good to go.

@JoergAtGithub
Copy link
Contributor Author

@JoergAtGithub Did you test the device without a report ID? If that is tested, then I think #232 is good to go.

No, I tested this only with devices with Report ID. I have to check for a device without Report ID, where hid_get_input_report works. Not sure if I have such a device.

@mcuee
Copy link
Member

mcuee commented Jun 16, 2021

Answers from Tim Roberts (a top Windows driver expert in the wordd and USB expert) in libusb-devel mailing list.

On Wed, Jun 16, 2021 at 12:21 AM Tim Roberts wrote:

Xiaofan Chen wrote:

So you can see that HIDAPI is using ReadFile for the fetching of HID reports
continuously with the hid_read API. And then it does not use HidD_GetXxx routines
to obtain the most current input report and feature report, rather it
uses IOCTLs to get the input report and feature report.

https://github.com/libusb/hidapi/blob/master/windows/hid.c#L56
#define IOCTL_HID_GET_FEATURE                         HID_OUT_CTL_CODE(100)
#define IOCTL_HID_GET_INPUT_REPORT              HID_OUT_CTL_CODE(104)

Historical reasons why Alan Ott chose to use DeviceIoControl() call and not HidD_GetXxx.
#229

The ioctls are equivalent to the HidD calls.  The HidD calls do almost
nothing other than format and send a DeviceIoControl request.

There are essentially 8 ways to talk to a HID device.

GetFeature/SetFeature read and write feature reports.  These get an
immediate response, and include a report ID byte (if the descriptor has
multiple reports).

SetOutputReport/WriteReport/WriteFile all write output reports and do
the same thing.  These all happen immediately and include a report ID byte.

GetInputReport gets an input report immediately, and includes a report
ID byte in both directions.

ReadReport/ReadFile are the strange ones.  These are long term calls.
This says "send me the next report that changes, whatever it may be."
It will get queued up for later.  When it returns, the buffer will
include a report ID, but you don't ask for one to begin with.

So it seems to me that the Microsoft documentation may be a bit confusing to
say the driver can use the two IOCTLs. Usually driver means kernel space in
this context even though there are user mode drivers as well.

Well, sort of.  A HID driver lives in kernel space.  It is NOW possible
to write a UMDF HID driver, but only because Microsoft created a
kernel-mode shim driver that shuffles the requests out to UMDF.  The HID
subsystem is really a kernel component.

Now the question is the return data of the two IOCTLs (output buffer).  From what I read, I believe
HIDAPI codes are wrong in both cases, as Microsoft documentation says that the first byte of
the buffer is unchanged (report ID or 0 if no report ID).
https://github.com/libusb/hidapi/blob/master/windows/hid.c#L933
https://github.com/libusb/hidapi/blob/master/windows/hid.c#L884

This code is wrong.  The "bytes_returned" value from a HID driver
definitely does include the report ID byte.  This happens to be fresh in
my mind because I just finished writing a fake HID driver that had to
deal with this.  I wonder if they did their testing on a faulty driver?

--

Tim Roberts
Providenza & Boekelheide, Inc.

@mcuee
Copy link
Member

mcuee commented Jun 16, 2021

@JoergAtGithub So #232 is good to go. Is it possible for you to create another pull request for the feature report? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Windows Related to Windows backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants