Windows report descriptor reconstructor reengineered pp struct #306
No reviewers
Labels
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
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-libusb/hidapi!306
Loading…
Reference in a new issue
No description provided.
Delete branch "windows_report_descriptor_reconstructor_reengineered_pp_struct"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is a variant of #262, but based on the Google Chromium approach using a reengineered struct of the preparsed data returned by HIDP_PREPARSED_DATA.
The output of hidtest can be read by visualization tools like: https://eleccelerator.com/usbdescreqparser/
In ALL cases known to me the generated report descriptor is logical identical to the original, which I read using the free version of USBlyszer. But it's not byte wise identical. The order of the bytes differ, handling of reundant information differs and padding bits can be defined in a different way. And theoretical there might be also a different in report length, but only if somebody adds more constant padding bits at the end of the report than neccessary.
I developed this code with Windows7 64Bit Service Pack1 Windows7, and mcuee tested it on Windows 10 Enterprise x64 (version 20H2).
Nice.
This is the output for my devices and the results look better than #262 at the first glance.
click to expand
Result for Microsoft USB Nano Receiver.
click to expand
Microchip PICKit 2: checked, good
Logitech M557 Bluetooth Mouse: at least report ID, report size and report count are all correct with a quick check.
Logitech USB receiver: to be checked
Microsoft USB Nano Receiver: to be checked
Which Windows version did you use for the test? I think i is important, to check this with various Windows versions.
I developed it with: Windows7 64Bit Service Pack1
Windows 10 Enterprise x64 (version 20H2)
An old Logitech USB Keyboard: (Logitech Classic New Touch 200 USB, model number Y-UR83), at least the report ID, report size and count are all correct.
Output from xusb mod using WinUSB driver.
Parser output:
Logitech Dual Action Gamepad output, #306 output seems to be correct.
xusb output using WinUSB driver.
Parser output
Microsoft USB Nano Receiver Output from macOS.
Parser output
click to expand
macOS USB prober output for Logitech USB Receiver
click to expand
Another USB device: Plantronics Blackwire 3220 Series USB Headset
click to expand
I saw all Padding items placed at the end of the reconstructed descriptor of the Plantronics Blackwire 3220 Series USB Headset, I commited a change, which hopefully fixes this. Could you please repeat the test with this device.
macOS USB Prober output for the Plantronics C320-M USB Headset (Blackwire 3220 Series USB Headset). This is a pretty complicated device.
click to expand
macOS USBProber output for the Microsoft USB Nano Receiver
click to expand
Latest output for Plantronics USB Headset.
click to expand
Full output with my Dell Windows laptop (Windows 10 x64 Enterprise 20H2), the external HID devices include the Plantronics USB Headset, Logitech USB Receiver and Microsoft Nano USB Receiver.
click to expand
My patch for the Plantronics headset seems not to work - but this is solveable for sure (for both #306 and for #262).
Great work. I think this is not an easy task but #306 seems to be pretty promissing.
Output from the ESP32 BLE HID example FW. This probably needs the fix from #309 first.
First report seems to be correct. $306 failed to get the second report.
The FW source code is here.
https://github.com/espressif/esp-idf/blob/master/examples/bluetooth/esp_hid_device/main/esp_hid_device_main.c
I commited a new patch for the padding issue with the Plantronics headset. Could you please retest with this device again.
Here is the output. You can probably ignore the \?\hid#converteddevice entries.
click for the log for Plantronics Blackwire 3220 Series USB headset
Full debug log for other devices as well.
click to expand for full log for other devices as well
The padding for the Plantronics headset is now at the correct position, but something is still wrong (Negative Report Size for the padding and multiple padding entries, where should be only one). Could you please provide me the original Report Descriptor of this devices:
Could you please provide me the original Report Descriptor of this devices?
Please refer to the macOS US Prober output in the previous comment.
https://github.com/libusb/hidapi/pull/306#issuecomment-882251672
The issue seems to be that a field of 2 bits has 3 Usages assigned, but not in ascending order. Therefore the UsageMin-UsageMax range structure of the WIN32 API isn't fitting here:
0x85, 0x04, // Report ID (4)
0x75, 0x02, // Report Size (2)
0x95, 0x01, // Report Count (1)
0x15, 0x01, // Logical Minimum (1)
0x25, 0x03, // Logical Maximum (3)
0x09, 0x82, // Usage (Sys Sleep)
0x09, 0x81, // Usage (Sys Power Down)
0x09, 0x83, // Usage (Sys Wake Up)
I added some debug outputs to the code, to understand the preparsed data for this device. Could you please run it for this again.
The debug output was helpful. I commited a patch, which hopefully fixes the issue with this device.
Debug output is not removed yet.
Full debug log with the latest git with quite some devices attached, including the Logitech USB Receiver, Dell USB Mouse, Dell USB Keyboard and the Plantronics USB headset.
Click to expand for the full debug log
Padding seems to be correct, but the item itself appeared twice. I commited a new version, which should fix also this.
Full debug log with the updated git.
Click to expand for the full debug log
I am a bit lost in tons of logs and checks, so I'll trust you both @mcuee and @JoergAtGithub regarding this (or #262 I guess).
When you think it is functionally ready, I'll check the implementation and will leave my comments then.
I expect at that moment to have a clean implementation (no debug logs or commented-out code).
Thanks.
I'll go for vacation the next two weeks, when I'm back, I will finish this and clean up the code for review!
Functional the results for the available test devices are already very close to what is achievable by reconstruction. But there are several Report Descriptor fields described in the HID specification, which are not used by any of these devices. Without reference device, it's difficult to guess, how Windows will process these fields.
@JoergAtGithub
Please help to name those "Report Descriptor fields" and maybe I can find some references devices or even find a way to create devices FW with those fields.
If you could look for devices as described in Appendix B and C of the following document: https://www.usb.org/sites/default/files/documents/hut1_12v2.pdf
Especially the case in Appendix B, where multiple alternative usages are declared for the same collection/control using deliminters, requires special handling.
Maybe you can program these examples in your PIC device?
Okay, let me try to find some reference devices first. If I can not find, I will try to see if there are reference codes for USB MCUs (USB PICs or ARM MCUs).
Somehow it can not print out the report descriptor of the Microsoft XBox One S Wireless Controller. Not so sure if this is expected or not. Same problem with #262.
It also does not work under Bluetooth.
HIDSharp output may not be correct, but at least it can get something out.
Interestling if I connect the Xbox One S Wireless Controller to my Mac Mini M1 through USB, it shows up as unconfigured and no HID interface is exposed. So I have to connect it through Bluetooth.
HID report parser output
@Youw @DJm00n @mcuee This is now complete and ready for code review!
@JoergAtGithub I think that your code could be moved to separate file.
hid.cis too big with your changes.hid_report_desc.cetc.@Youw any thoughts?
@DJm00n I could seperate it, but than we need also a header file, which contains all the typedefs and declarations used by both.
Not yet. See your README:
That is currently our design. I do want to change that, so we can decouple the implementation, move some common parts, etc.
But that we will perform separately, some time later, for all backends.
Thanks! I'll go over it over the weekend (I hope).
I have too many comments to the implementation/code style.
So instead of leaving them on GH, I'll create a separate commit instead with "fixes".
And after giving it a good though - it seems like a good idea to try to separate the implementation out of
hid.c.I'll work on that too.
@ -71,7 +71,7 @@ int main(int argc, char* argv[])printf(" Report Descriptor: ");res = hid_get_report_descriptor(device, buf, sizeof(buf));any particular reason to use upper-case?
@ -109,10 +109,147 @@ static struct hid_api_version api_version = {USHORT Reserved[17];I'm not even familiar with this documentary style.
HIDAPI alreayd uses doxygen-style syntax for documentary comments, please use it as well.
@ -1146,13 +1366,1014 @@ int HID_API_EXPORT_CALL HID_API_CALL hid_get_indexed_string(hid_device *dev, intreturn 0;}/**this cannot fail, unless memory corruption, in which case we're already "screwed"
@ -109,10 +109,147 @@ static struct hid_api_version api_version = {USHORT Reserved[17];Ok, after a brief googling I understand, that in
hidapi.hwe actually have more like a javadoc-style syntax (which is supported by doxygen), but my point is - please be consistent.Thanks for working on this!
If you restructure the code, please keep the testability in mind. For the unit test in #343 it's neccessary:
reconstruct_report_descriptorstandaloneOnly readability of the test output. For me it was easier to read a list like 0xA6, 0xC0 than 0xa6, 0xc0.
If you prefer the other style, feel free to change it.
Once written and working, I prefer not to change anything unless necessary.
Smaller diff is always preferable, when possible.
Yeah, maybe I'll update that too.
I still have (none-written) comments regarding the implementation but lets have a
libusb:get-descriptoras a working branch for this change - merging as is for now.Thanks for cleaning up my code!
@JoergAtGithub Just wanted to add a little piece of information since I am currently myself trying to reconstruct what
HIDP_PREPARSED_DATAis actually defined as.For obvious reasons we can't be sure about the names -- except for items that correspond 1:1 to another documented struct -- but compared to this one I came up with this:
So in conclusion you can use the info about
ULONG SysPowerCaps;, which is the only field accessed byhidparse!HidP_SysPowerCaps. That name may not be the same as Microsoft uses, but it's the closest we can get.Additionally, as commented inline above, it's possible that the report info structs (your
hid_pp_caps_info) are thought as an array, indexed byHIDP_REPORT_TYPE(hidpi.h):But my initial hunch was to have those separate, probably because I hadn't looked at as many functions as I have now. However, that said, the array hypothesis isn't too far-fetched if we look at how a number of functions taking
HIDP_PREPARSED_DATA* PreparsedDataas input treat this. Generally any functions taking that as input, appear to start with this:I'd say chances are that this is hidden behind a macro, as it's repeated all over the place. Curiously
hidparse!HidP_SysPowerCapsdoesn't appear to have this check (or any other); perhaps owed to its "internal" nature?!That check is then followed by something that takes the
HIDP_REPORT_TYPE ReportTypeargument, does bounds checking against the values defined in the enum, and then uses known offsets (fromHIDP_PREPARSED_DATA) to compute what amounts to aHIDP_CAPS_REPORT_INFO *for the respectiveReportType. Since it works with offsets, though, I reckon that internally these are indeed separate members and not an array.Based on your information I can probably speed up my effort in what call
HIDP_PREPARSED_DATA_TAILabove. Thanks! 😉One more note: you wrote:
and while that may be true, customarily Microsoft uses
ANYSIZE_ARRAY(i.e.#define ANYSIZE_ARRAY 1) to describe such arrays. C++ also doesn't like just[]and I reckon that's why the define exists.PS: If I find out anything further that can complete the idea of the involved structs, I'll let you know.
PPS: the
hidparse.syswas this one and thehid.dllthis one.@hugmyndakassi
github.com/tongzx/nt5src@daad8a087a/Source/XPSP1/NT/drivers/wdm/input/hidparse/hidparse.h (L151-L177)🙈@DJm00n Ouch, I'm not really allowed to look at leaked proprietary code. Clean room reverse engineering and all.
Either way, I found that IDA curiously knows about this, supposedly from the PDB. And indeed looking with https://github.com/horsicq/PDBRipper discloses that the PDB contains definitions.
This is what IDA makes of it, when exporting to a header:
So finally I also have more sensible type and member names. And sort of a confirmation for those things where I wasn't a 100% certain. But I find
_HIDP_UNKNOWN_TOKENcurious. The name seems to originate from Windows.The gist is, I should probably pay more attention to IDA's changelog and their blog and always consult the "Local Types" (Shift+F1) when dealing with code that has debug symbols available.
_HIDP_UNKNOWN_TOKENis about not recognized global HID items. See "6.2.2.7 Global Items" part of the HID spec for a list of possible tokens:1100 01 nn to 1111 01 nn- Range reserved for future use._HIDP_UNKNOWN_TOKENis official documented here: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/hidpi/ns-hidpi-_hidp_unknown_token