RFC: Add HID bus type in hid_device_info #308
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!308
Loading…
Reference in a new issue
No description provided.
Delete branch "hid_bus_type"
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?
It happens that Bluetooth HID gamepads are having the same VID/PID as when connected via USB (DualShock4 and DualSense for example). But they have different report descriptors and thus input/output reports. We need a way to differentiate them.
To address this I have added a new
hid_bus_typeenum andhid_bus_type bus_typefield tohid_device_infostruct:Implemented it for Windows, libusb, hidraw and MacOS backends.
Sony DualSense controller example.
Before:
After:
I didn't test Linux version yet.
@mcuee @Youw can you test it on Linux?
Technically I could, but right now I'm a little short on time regarding everything, so I can only ask not to make any expectations when I'll do it
I can only promice that I will, and I'm also going to see if this can be implemented for macOS as well
Here is the test results on my Raspberry Pi 400 running 64bit Ubuntu Linux.
click to expand for the run log
@DJm00n You can see that libusb backend output is expected. As for the hidraw backend, on one hand it seems to work, but on the other hand, many reports bus type "0". Is this expected by your design? Take note hidraw backend has the feature of ""split by usage_page/usage" as per https://github.com/libusb/hidapi/issues/287 .
Under my Windows 10 laptop (Windows 10 Enterprise x64 20H2), it seems to me Microsoft HIDI2C device is not listed correctly.
click to expand
@DJm00n For the I2C HID device, you have the following codes for Windows.
And I checked the Microsoft I2CHID device in Device Manager "Human Interface Devices" section and indeed it has a "Compatible device id" and "Matching device id" value of "ACPI\PNP0C50".
@DJm00n And for those "\?\hid#converteddevice", I am not exactly sure if 255 is the right one to use. I tend to think they are more like "virtual hid bus type". But I am okay as well if you use 255 if there are no better ways.
@DJm00n From what I see, the following Windows codes are not good enough (just using "Device_InstanceId"). On the other hand, for the purpose of HIDAPI, maybe it is still okay. I do not think we can do much with the "Microsoft I2CHID device" anyway.
@mcuee Thank you for testing it!
I don't have such I2C or SPI HID devices in person. So I need some further help.
Can you debug the code a bit and look what exact
device_id(acquired withDEVPKEY_Device_InstanceIdproperty) your devices are having?For example you can add:
before
if (wcsstr(...))checks.@DJm00n This is what I get.
click to expand
@DJm00n The Microsoft I2C HID device has the following
If I look at the Device Manager "Human Device Interface" entries, Device_InstanceId printed out is the same as what is listed as "Device Instance Path" in Device Manager.
The one you want to get here is "Compatible IDs" which has two values, "ACPI\PNP0C50" and "PNP0C50".
@DJm00n
<html>From the link in your code,
https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management
The ACPI 5.0 Specification includes support for HID Class Devices. the ACPI definitions for HID I²C are as follows.
Same for SPI HID device.
<html>https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-for-spi
The ACPI 5.0 Specification includes support for HID Class Devices. the ACPI definitions for HID SPI are as follows.
Compatible ID should work for your purpose, no matter it is USB, Bluetooth, I2C or SPI.
https://docs.microsoft.com/en-us/windows-hardware/drivers/install/compatible-ids
More info about HID Transport under Windows
https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/hid-transports
Built-in device for my Dell Laptop (without any external USB/Bluetooth devices).
click to expand
@mcuee Thank you! I’ll try to do a fix.
@DJm00n Yes the latest update seems to work fine.
With added debug print, so I think the latest update is good.
click to expand
It is also good for BLE device.
@mcuee
I have fixed it. Please check when you have a time.
@DJm00n Yes the latest git is okay for Linux (tested with my Raspberry Pi 400 running Ubuntu 64bit Linux).
@Youw could you please review the code?
This looks good.
Since this PR is targeting master, and not a separate feature branch, lets finish with all platforms (macOS) first, before we merge it.
Also, I'd like to finish #309 separately first, as I already mentioned.
we've already faced a name conflict with
hid_init(on some of the BSD platforms);let's prefix it as
HID_API_orHIDAPI_instead of justHID_to decrease the chance of name conflict(minor)
my first though when I've seen
closeis that you'd close the entiredev, which in fact is more likefinalorendsince
wcsstrreturns a pointer (not an index) lets compare it toNULLsame applies below
done
done
done
@DJm00n You may want to disable the following warnings.
Maybe this one can be considered for next release 0.12? Or maybe 0.13?
The suggested code is not tested, as I do not have access to a Mac.
Hey @Youw!
I have added MacOS implementation (KUDOS @JoergAtGithub) and updated this PR. Need someone who can test MacOS implementation.
Checked (and fixed a bit) macOS implementation - looks good.
Windows implementation doesn't seem to have the UNKNOWN fallback.
I suggest having numeric value 0x00 as a value for
HID_API_BUS_UNKNOWN, so a zero-initializedhid_devicewill have thebus_typefield set to a valid value.Sample on my mac machine:
How is the headset connected?
The funny thing - it is not connected.
And the bus type for it is
"Audio"(looked up in the debugger).@Youw good point. updated.
Still looks good under Windows.
And it is also good for macOS.
@mcuee thanks! “Microsoft Bluetooth Mouse” - what kind of mouse is that? Looks like bus type is not detected in this case? Does it use some kind of custom driver?
I have the Logitech K480 bluetooth keyboard and Microsoft Bluetooth Mouse.
The MS Bluetooth Mouse seems to use Bluetooth Low Energy.
https://www.microsoft.com/en-sg/d/microsoft-bluetooth-mouse/8qbx2w3xsp2h?activetab=pivot:techspecstab
Connection interface: Bluetooth® Low Energy compatible 4.0/4.1/4.2/5.0
Not so sure if you need to deal with BLE specially.
@mcuee can you modify the code locally a bit, to print
CFStringGetCStringPtr((CFStringRef)transport_prop, kCFStringEncodingMacRoman)?So we could check what is the Transport as detected by
IOHIDManagerTest results under Linux Ubuntu 20.04 (dual boot with the above Windows 11 machine) and it seems to be fine.
Oh.
But
kIOHIDTransportBluetoothLowEnergyValueisBluetoothLowEnergy. That's...unexpected.Update: MacOS 12.2.1.
That is exactly the reason.
As mentioned by @DJm00n, somehow macOS 12.4 comes out with
Bluetooth Low Energy. However,kIOHIDTransportBluetoothLowEnergyValueisBluetoothLowEnergyI have pushed a fix. Just checking for "Bluetooth" prefix now.
Why not match both
Bluetooth Low EnergyandkIOHIDTransportBluetoothLowEnergyValue(=BluetoothLowEnergy) for BLE device under macOS?@mcuee Current
CFStringHasPrefix((CFStringRef)transport_prop, CFSTR(kIOHIDTransportBluetoothValue))check will match for "Bluetooth", "BluetoothLowEnergy" and "Bluetooth Low Energy" strings.It would make 3 matches, which will break, once Apple deside to invent somethink like
Bluetooth_Low_Energy@Youw or "Bluetooth LE" or "Bluetooth Smart" like it was named before :)
@Youw and @DJm00n
Ok, I understand.
Now it is fine under macOS.
One last test result under Windows 10 and it seems to be okay as well.
@Youw
Just wondering if this can be merged for 0.13 release. Thanks.
Yes, probably. Just need to resolve the conflicts.
@DJm00n
Please help to resolve the mergeing conflicts. Thanks.
I'm afraid that having this member here breaks the ABI.
A safer change would be:
Fixed merge conflict and applied review comments.
If I'm not mistaken, this breaks the ABI. I'm in no position to argue whether that's fine or not here, but given https://github.com/libusb/hidapi/issues/292#issuecomment-869194346,
it might be worth adding a note about this in the 0.13.0 release notes.
I haven't performed any specific tests, but it shouldn't break the ABI, since we added the new field at the end of the struct.
And all of the handling (allocation/deallocation) is done with the API functions.
@Youw,
You're right.
I wasn't completely sure that the new field being added at the end of the struct was sufficient, and had also hit (but not yet looked into) an invalid read with valgrind when running an executable built against 0.12.0 with 0.13.0.
But it turned out that the valgrind error was unrelated to this (see #497). And since we only manipulate
hid_device_infothrough pointers, I agree that it's size doesn't matter.Sorry for the noise.