gh-libusb/hidapi
Watch
1
0
Fork
You've already forked hidapi
 0
Code Issues 58 Pull requests 18 Releases 13 Wiki Activity

Added support for Get Input Report to linux, mac, windows and libusb. #59

Merged
DanielVanNoord merged 13 commits from master into master 2019-11-15 05:55:51 -06:00
Conversation 22 Commits 13 Files changed 6 +118
DanielVanNoord commented 2019-08-01 10:32:00 -05:00 (Migrated from github.com)

This was tested for all operating systems and each worked identically.

This was tested for all operating systems and each worked identically.
👍 2
Qbicz (Migrated from github.com) reviewed 2019-08-01 10:32:00 -05:00
Youw (Migrated from github.com) reviewed 2019-08-01 11:39:03 -05:00
Youw (Migrated from github.com) left a comment

Windows implementation better be same as for hid_send_feature_report, to return actual length
(tested in my projects)

mac implementation sends report ID as part of data, as it was in hid_get_feature_report before #3

hidraw implementation does not respect length parameter, which may lead to memory corruption

libusb implementation seems fine to me

Thanks!
I was going to have it implemented for some while, this is a good start.

Some notes:

  1. This is a major change (new API), we should update version too
  2. Along with new version, I think it makes sense add a way to determine library version in compile/run-time.
Windows implementation better be same as for `hid_send_feature_report`, to return actual length (tested in my projects) mac implementation sends report ID as part of data, as it was in `hid_get_feature_report` before #3 hidraw implementation does not respect `length` parameter, which may lead to memory corruption libusb implementation seems fine to me --- Thanks! I was going to have it implemented for some while, this is a good start. Some notes: 1) This is a major change (new API), we should update version too 2) Along with new version, I think it makes sense add a way to determine library version in compile/run-time.
hidapi/hidapi.h
@ -334,6 +334,31 @@ extern "C" {
*/
Youw (Migrated from github.com) commented 2019-08-01 11:20:33 -05:00 
		/** @brief Get a input report from a HID device.
```suggestion /** @brief Get a input report from a HID device. ```
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
Youw (Migrated from github.com) commented 2019-08-01 11:21:41 -05:00

use register_device_error_format same way as in hid_get_feature_report does

use `register_device_error_format` same way as in `hid_get_feature_report` does
DanielVanNoord commented 2019-08-01 12:50:54 -05:00 (Migrated from github.com)

Windows implementation better be same as for hid_send_feature_report, to return actual length
(tested in my projects)
I converted it over to use this method.

mac implementation sends report ID as part of data, as it was in hid_get_feature_report before #3
See my open issue /issues/60, this works (for me) the other way does not. It is possible that my device is out of spec. But it seems really odd that all the other platforms work and mac, when implemented to spec, does not.

hidraw implementation does not respect length parameter, which may lead to memory corruption
Not sure how to fix this one. The linux api that I am using does not appear to have an option for length.

Windows implementation better be same as for hid_send_feature_report, to return actual length (tested in my projects) I converted it over to use this method. mac implementation sends report ID as part of data, as it was in hid_get_feature_report before #3 See my open issue /issues/60, this works (for me) the other way does not. It is possible that my device is out of spec. But it seems really odd that all the other platforms work and mac, when implemented to spec, does not. hidraw implementation does not respect length parameter, which may lead to memory corruption Not sure how to fix this one. The linux api that I am using does not appear to have an option for length.
Youw commented 2019-08-01 12:55:46 -05:00 (Migrated from github.com)

Not sure how to fix this one. The linux api that I am using does not appear to have an option for length.

pass pointer to stack-allocated data that is guaranteed to be large enough for the API, and then copy up to length bytes of that data to data

> Not sure how to fix this one. The linux api that I am using does not appear to have an option for length. pass pointer to stack-allocated data that is guaranteed to be large enough for the API, and then copy up to `length` bytes of that data to `data`
Youw commented 2019-08-01 12:57:59 -05:00 (Migrated from github.com)

regarding mac and #60 - lets wait for comments of other maintainers/users

regarding mac and #60 - lets wait for comments of other maintainers/users
z3ntu (Migrated from github.com) reviewed 2019-08-01 13:18:05 -05:00
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
z3ntu (Migrated from github.com) commented 2019-08-01 13:18:04 -05:00

In the log message it should be GREPORT as that's the ioctl constant being used

In the log message it should be `GREPORT` as that's the ioctl constant being used
Youw commented 2019-08-01 13:51:15 -05:00 (Migrated from github.com)

something doesn't adds up in hidraw implementation: if I understand how ioctl works, as per HIDIOCGREPORT definition, its argument can only be struct hiddev_report_info, not some user data

I'll spend more time investigating it a bit later

something doesn't adds up in hidraw implementation: if I understand how `ioctl` works, as per `HIDIOCGREPORT` definition, its argument can only be `struct hiddev_report_info`, not some user data I'll spend more time investigating it a bit later
DanielVanNoord commented 2019-08-01 14:15:47 -05:00 (Migrated from github.com)

It is possible, I wrote (and studied) the windows, mac and libusb implementations. Another developer that I worked with wrote the Linux one (I saw your issue at signal11/hidapi and am trying to up-stream now). I simply verified that it worked with both test boards that I have.

It is possible, I wrote (and studied) the windows, mac and libusb implementations. Another developer that I worked with wrote the Linux one (I saw your issue at signal11/hidapi and am trying to up-stream now). I simply verified that it worked with both test boards that I have.
Qbicz (Migrated from github.com) reviewed 2019-08-03 09:04:36 -05:00
Qbicz (Migrated from github.com) left a comment

Looks good to me, with a few minor comments.

Looks good to me, with a few minor comments.
hidapi/hidapi.h
@ -334,6 +334,31 @@ extern "C" {
*/
Qbicz (Migrated from github.com) commented 2019-08-03 08:54:56 -05:00 
			report to be read. Make sure to allow space for this
```suggestion report to be read. Make sure to allow space for this ```
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
Qbicz (Migrated from github.com) commented 2019-08-03 08:57:09 -05:00

Empty line not needed here

Empty line not needed here
windows/hid.c
@ -818,6 +822,55 @@ int HID_API_EXPORT HID_API_CALL hid_get_feature_report(hid_device *dev, unsigned
#endif
Qbicz (Migrated from github.com) commented 2019-08-03 09:03:21 -05:00 
	return length;
```suggestion return length; ```
windows/hid.c
@ -821,0 +857,4 @@
hid_get_feature_report() synchronous. */
res = GetOverlappedResult(dev->device_handle, &ol, &bytes_returned, TRUE/*wait*/);
if (!res) {
/* The operation failed. */
Qbicz (Migrated from github.com) commented 2019-08-03 09:02:41 -05:00

Does it make sense to keep this disabled code? I know it is the same way in hid_get_feature_report(), but what purpose does this code section serve?

Does it make sense to keep this disabled code? I know it is the same way in `hid_get_feature_report()`, but what purpose does this code section serve?
DavidCNelson commented 2019-08-05 09:21:40 -05:00 (Migrated from github.com)

regarding mac and #60 - lets wait for comments of other maintainers/users

I am working on a new device with HID and also on the host side. This pull request is rather timely for me as the day it was created was the same day I was getting ready to implement the same functionality.
The issue seems to be the question if there is a report ID at the beginning of the data transfer. The HID Spec v1.11 in section 5.6 describes that if the HID device defines report IDs in the descriptor, then the report ID must be sent as the first byte of all data transfers. If the device does not define report IDs(appears as report ID == 0), then the non-existent report id is not included in the data transfer.
Consider the code in set_report in mac/hid.c at lines 774-785. If the report id is 0, it does not send the report ID as part of the data. If the report ID is non-zero, then the whole buffer is sent. A similar thing needs to happen for hid_get_feature_report and hid_get_input_report. If the report ID is 0, then the received data will not have a report ID, so the data should be read into the buffer starting with the second byte of the buffer. If the report ID is non-zero, the data received should start with the report ID, so you can fill the buffer starting with the first byte.
Depending on the device one is using, #3 fixes an issue or, as #60 points out, it is a regression.

> regarding mac and #60 - lets wait for comments of other maintainers/users I am working on a new device with HID and also on the host side. This pull request is rather timely for me as the day it was created was the same day I was getting ready to implement the same functionality. The issue seems to be the question if there is a report ID at the beginning of the data transfer. The [HID Spec v1.11](https://www.usb.org/sites/default/files/documents/hid1_11.pdf) in section 5.6 describes that if the HID device defines report IDs in the descriptor, then the report ID must be sent as the first byte of all data transfers. If the device does not define report IDs(appears as `report ID == 0`), then the non-existent report id is not included in the data transfer. Consider the code in `set_report` in mac/hid.c at lines 774-785. If the report id is 0, it does not send the report ID as part of the data. If the report ID is non-zero, then the whole buffer is sent. A similar thing needs to happen for `hid_get_feature_report` and `hid_get_input_report`. If the report ID is 0, then the received data will not have a report ID, so the data should be read into the buffer starting with the second byte of the buffer. If the report ID is non-zero, the data received should start with the report ID, so you can fill the buffer starting with the first byte. Depending on the device one is using, #3 fixes an issue or, as #60 points out, it is a regression.
Youw commented 2019-08-05 16:22:51 -05:00 (Migrated from github.com)

HID Spec v1.11 in section 5.6 also specifies how to know if a device is using numbered reports:

A Report ID item tag assigns a 1-byte identification prefix to each report transfer. If no Report ID item tags are present in the Report descriptor, it can be assumed that only one Input, Output, and Feature report structure exists and together they represent all of the device’s data.

@DanielVanNoord, @DavidCNelson can you confirm, that your devices defines corresponding Report IDs tags in the Report descriptor?

HID Spec v1.11 in section 5.6 also specifies how to know if a device is using numbered reports: > A Report ID item tag assigns a 1-byte identification prefix to each report transfer. _If no Report ID item tags are present in the **Report descriptor**_, it can be assumed that only one Input, Output, and Feature report structure exists and together they represent all of the device’s data. @DanielVanNoord, @DavidCNelson can you confirm, that your devices defines corresponding Report IDs tags in the Report descriptor?
DavidCNelson commented 2019-08-06 16:09:41 -05:00 (Migrated from github.com)

HID Spec v1.11 in section 5.6 also specifies how to know if a device is using numbered reports:

A Report ID item tag assigns a 1-byte identification prefix to each report transfer. If no Report ID item tags are present in the Report descriptor, it can be assumed that only one Input, Output, and Feature report structure exists and together they represent all of the device’s data.

@DanielVanNoord, @DavidCNelson can you confirm, that your devices defines corresponding Report IDs tags in the Report descriptor?

Yes, the device I'm working with does define report IDs in the descriptor, and so we do use report IDs for all transfers.

> HID Spec v1.11 in section 5.6 also specifies how to know if a device is using numbered reports: > > > A Report ID item tag assigns a 1-byte identification prefix to each report transfer. _If no Report ID item tags are present in the **Report descriptor**_, it can be assumed that only one Input, Output, and Feature report structure exists and together they represent all of the device’s data. > > @DanielVanNoord, @DavidCNelson can you confirm, that your devices defines corresponding Report IDs tags in the Report descriptor? Yes, the device I'm working with does define report IDs in the descriptor, and so we do use report IDs for all transfers.
DanielVanNoord (Migrated from github.com) reviewed 2019-08-10 19:18:24 -05:00
windows/hid.c
@ -821,0 +857,4 @@
hid_get_feature_report() synchronous. */
res = GetOverlappedResult(dev->device_handle, &ol, &bytes_returned, TRUE/*wait*/);
if (!res) {
/* The operation failed. */
DanielVanNoord (Migrated from github.com) commented 2019-08-10 19:18:24 -05:00

hid_get_feature_report had it so I included it as well. The only benefit I can think of is that it shows that the other way is known and wrong, even though it is much shorter.

hid_get_feature_report had it so I included it as well. The only benefit I can think of is that it shows that the other way is known and wrong, even though it is much shorter.
DanielVanNoord commented 2019-08-10 19:19:19 -05:00 (Migrated from github.com)

Same here, the device I am working on uses report IDs in the descriptors for all transfers.

Same here, the device I am working on uses report IDs in the descriptors for all transfers.
micolous (Migrated from github.com) reviewed 2019-08-10 23:55:25 -05:00
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
micolous (Migrated from github.com) commented 2019-08-10 23:42:17 -05:00

HIDIOCGREPORT expects a hiddev_report_info, which is declared here.

This differs from HIDIOCGFEATURE which takes in the first byte as a report ID.

I'm also not sure that this is a valid ioctl for hidraw -- this looks like it is for hiddev!

`HIDIOCGREPORT` [expects a `hiddev_report_info`](https://elixir.bootlin.com/linux/v4.4/source/include/uapi/linux/hiddev.h#L160), which is [declared here](https://elixir.bootlin.com/linux/v4.4/source/include/uapi/linux/hiddev.h#L65). This differs from `HIDIOCGFEATURE` which takes in the [first byte as a report ID](https://elixir.bootlin.com/linux/v4.4/source/include/uapi/linux/hidraw.h#L40). I'm also not sure that this is a valid `ioctl` for `hidraw` -- this looks like it is for `hiddev`!
Youw (Migrated from github.com) reviewed 2019-08-11 13:29:27 -05:00
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
Youw (Migrated from github.com) commented 2019-08-11 13:29:27 -05:00

also, I don't see support of Input/Output Reports by hidraw at all

also, I don't see support of Input/Output Reports by hidraw at all
Youw (Migrated from github.com) reviewed 2019-08-11 13:39:10 -05:00
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
Youw (Migrated from github.com) commented 2019-08-11 13:39:10 -05:00

Nope, there is none

Nope, [there is none](https://elixir.bootlin.com/linux/v5.3-rc3/source/drivers/hid/hidraw.c#L424)
Youw (Migrated from github.com) reviewed 2019-08-23 22:34:01 -05:00
linux/hid.c
@ -790,6 +790,11 @@ int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned char *data,
return res;
}
Youw (Migrated from github.com) commented 2019-08-23 22:34:01 -05:00 
// Not supported by Linux HidRaw yet
```suggestion // Not supported by Linux HidRaw yet ```
Youw (Migrated from github.com) requested changes 2019-09-24 05:57:23 -05:00
Youw (Migrated from github.com) left a comment

@DanielVanNoord please resolve merge conflict, and:

  • update mac implementation as per #70
  • update windows\ddk_build\hidapi.def
@DanielVanNoord please resolve merge conflict, and: - update mac implementation as per #70 - update windows\ddk_build\hidapi.def
DanielVanNoord commented 2019-09-27 12:48:27 -05:00 (Migrated from github.com)

@Youw Should be taken care of now

@Youw Should be taken care of now
Youw (Migrated from github.com) approved these changes 2019-09-28 08:08:11 -05:00
linux/hid.c
Youw (Migrated from github.com) commented 2019-09-28 08:07:30 -05:00

remove

remove
windows/hid.c
@ -109,6 +110,7 @@ extern "C" {
typedef BOOLEAN (__stdcall *HidD_GetProductString_)(HANDLE handle, PVOID buffer, ULONG buffer_len);
Youw (Migrated from github.com) commented 2019-09-28 08:08:06 -05:00 
	typedef BOOLEAN (__stdcall *HidD_GetInputReport_)(HANDLE handle, PVOID data, ULONG length);
```suggestion typedef BOOLEAN (__stdcall *HidD_GetInputReport_)(HANDLE handle, PVOID data, ULONG length); ```
Youw (Migrated from github.com) approved these changes 2019-09-30 10:49:51 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
Qbicz
Youw
Labels
Clear labels   
3rdparty
3rd-party related issue/question

  
Android
Android OS related

  
API
API change, Version 1 stuff

  
Author does not respond

   
bsd
FreeBSD, NetBSD, OpenBSD, etc

  
bug
Something isn't working

  
build system/CI
Anything related to building the project or running on CI

  
ci-virtual-device
Run the virtual HID device CI jobs (Windows driver + libusb raw-gadget)

  
CMake
Related to CMake build system

  
Contributions Welcome
Any external help is welcome

  
Core
Related to common codes like hidapi.h

  
dependencies
Pull requests that update a dependency file

  
device-specific
Issues/PRs related to a specific device

  
documentation
Improvements or additions to documentation

  
don't_merge
Don't merge this PR as is

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
Example
Related to hidtest or new examples

  
Haiku
Related to Haiku OS (using libusb backend)

  
hidraw
Related to Linux/hidraw backend

  
hotplug
Related to hotplug

  
info required
Need more input from PR/issue author

  
invalid
Not a HIDAPI issue

  
libusb
Related to libusb backend

  
macOS
Related to macOS backend

  
Misc
Typo, whitespace, etc.

  
Not an issue

   
question
Information is requested

  
Release
Related to new release

  
Test
Related to built-in test (not hidtest)

  
Test GUI
Related to a Test GUI application

  
Windows
Related to Windows backend

  
wontfix
This will not be worked on

No labels
3rdparty
Android
API
Author does not respond
bsd
bug
build system/CI
ci-virtual-device
CMake
Contributions Welcome
Core
dependencies
device-specific
documentation
don't_merge
duplicate
enhancement
Example
Haiku
hidraw
hotplug
info required
invalid
libusb
macOS
Misc
Not an issue
question
Release
Test
Test GUI
Windows
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libusb/hidapi!59
No description provided.