Add hid_get_device_info #432
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!432
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/hid_get_device_info"
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?
Resolves: #431
Closes: #164
Closes: #163
I don't like the signature of
get_device_info_for_devicein linux, and Im unsure aboutfill_device_info_for_devicein libusb.The mac, linux and libusb implementations appear to work fine with hidtest.
I havent tested the other backends yet
lets have it against a feature branch for now: https://github.com/libusb/hidapi/tree/hid_get_device_info
This could replace #164, as it will be possible to get the path from the device_info struct.
I can see there you were wondering about lazily initialising the path. Would you want that to be done here? It doesnt look like it would be too hard to do for most backends. It looks like the only extra data some backends may need is the path. Nothing too bad to persist until the device_info is generated
Yes, lazy initialization of the device info should be beneficial, specially for those who never use it.
The calls to get the serial number and other strings are now all copying off the device_info, so I would guess it to be rare that it won't get loaded, but I have no problem making it lazy and caching it
not needed since you're using
callocI don't like that we're passing the ownership inside here
lets initialize this field at the same place, where we call
hidapi_initialize_device- where the ownership ofpathis holdthis block has extra indentation - should remain at the same level as
res = libusb_open(dev, &handle);Rebase to master, when you have a chance - got some conflicting changes in there.
This was copied from elsewhere. There are many other places doing a calloc then setting their pointer values to null
Oh no, developers copy-paste code 😱
Mac Mini M1 (macOS 12.4) output. The output seems to be fine.
Windows MSYS2 mingw64 output. Seems to be fine.
@Julusian do you consider this as still a draft?
Sorry, I got distracted on other things
Yeah I am happy with this. I havent made it lazy initialize yet though
@mcuee you haven't really tested my changes much there. You have for the refactoring, but the
unable to open deviceat the end means that it didnt open a device and use the new methodI see. Unfortunately the one used in hidtest is not a common device but rather a modified Microchip generic HID FW example from Alan Ott. So maybe I need to use other codes to test this pull request.
Will hidapitester be good enought?
I have been changing the ids to be for a device I have, then stopping the application when it starts on the read/write tests
This is the output from my Microchip Custom Demo (original Microchip FW). It is probably good enough for this testing.
Output under Linux.
Output under macOS 12.4 (Mac Mini M1 2020).
Good idea. So I just modified test.c to open different device.
Here is the output for Logitech Bluetooth mouse.
For Logitech USB Receiver:
@Youw I think I am done on this now.
I have made it lazy-load the device_info struct on linux&mac backends, both libusb and windows use some additional properties which arent persisted on the struct.
I'll have to find some time to go over it again to review. I'll try to do so shortly.
Thanks for the contribution!
@ -473,0 +477,4 @@@returnsThis function returns a pointer to the struct #hid_device_infofor this hid_device, or NULL in the case of failure.@ -1199,6 +1199,16 @@ int HID_API_EXPORT_CALL HID_API_CALL hid_get_serial_number_string(hid_device *dereturn 0;@ -1161,17 +1164,74 @@ void HID_API_EXPORT hid_close(hid_device *dev)ideally - this check should go before calling
hid_get_device_info@ -154,6 +155,7 @@ static hid_device *new_hid_device(void)dev->source = NULL;dev->input_report_buf = NULL;I always recommend not to have large (most) of the function body under an
ifcondition, i.e. in this case:so the rest of the body doesn't have an extra indentation
yes, that is another holy war about function early returns, and if that bothers you a lot - just place the content of the
ifbranch body into another functionthe point - avoid extra indentation of the large amount of code
and yes - its been even worse before, but this time we're doing curated refactoring of this part, and not just "different people adding random things"
this is specifically 3 spaces, and not a tab, to align the text (content of the comment)
try to keep the original alignment
general comment: can we make it in the same form as on macOS?
I.e.:
static struct hid_device_info * fill_device_info_for_device(libusb_device_handle *handle, struct libusb_device_descriptor *desc)This way it would make the usage a bit simpler:
hid_device_infoat every usage (2 places);I believe this way it will be possible to make the lazy initialisation with simple implementation
@ -438,0 +492,4 @@return parse_hid_vid_pid_from_uevent(buf, bus_type, vendor_id, product_id);}/* return non-zero if successfully read/parsed */this is filtering logic - should be a part of
hid_enumerateonly, and not a part ofget_device_info_for_device@ -438,0 +492,4 @@return parse_hid_vid_pid_from_uevent(buf, bus_type, vendor_id, product_id);}/* return non-zero if successfully read/parsed */this looks unnecessarily complicated, I expect to have something like:
static struct hid_device_info * get_device_info_for_device(struct udev_device *raw_dev)@ -438,0 +492,4 @@return parse_hid_vid_pid_from_uevent(buf, bus_type, vendor_id, product_id);}/* return non-zero if successfully read/parsed */maybe even:
create_device_info_for_device- note: create_, not get_Windows and macOS is fine.
Libusb and Hidraw - I see multuple improvements to perform before it is ready as far as I can tell.
@Julusian do you feel comfortable performing all changes as part of this PR? I could make a feature branch and accept changes there iteratively instead.
@ -438,0 +492,4 @@return parse_hid_vid_pid_from_uevent(buf, bus_type, vendor_id, product_id);}/* return non-zero if successfully read/parsed */I ended up with this because it can return any number of devices. So it was either this, or immediately afterwards it would have to loop through the returned device to find
last_dev. It didnt feel good to be iterating through a return value like that.Something like that. I did do this initially, but after it got into an infinite loop a couple of times I decided to change it.
I suppose
first_devcould be returned instead of being passed out like this, but this felt a bit better for consistency.@ -438,0 +492,4 @@return parse_hid_vid_pid_from_uevent(buf, bus_type, vendor_id, product_id);}/* return non-zero if successfully read/parsed */To get the vendor_id and product_id from
raw_dev, it requires 2 calls to udev methods, and some string parsing (parse_uevent_info).Having to repeat that much logic doesnt feel good, but I do appreciate that this is also not great
Good point. I have made those changes.
Lazy initialisation is still tricky, as I don't see where
interface_numandbConfigurationValueare being stored, which are both needed to reform the path. It is possible they are there and I havent spotted them, or maybe they can be retrieved from the open libusb handle?I think this is as far as I feel I can go on this myself.
The remaining couple of points are ones that I think need to be experimented with to see how close it can get to what you want, and consider the cost/benefit of the result. So this would probably be better to do separately.
Updated the implementation to have lazy initialisation for libusb backend.
During the refactoring, found a bug:
hidapi_initialize_devicehasn't been filling up thedevice_info->interface_numberfield.With new implementation such kind of issues are eliminated.
Another libusb update: sorted out the "get usage_page/usage". It has to be handled differently during the enumeration and when the device is fully opened.
Now I'm happy with libusb implementation.
Updated hidraw implementation too.
Dependency on libudev has been a major blocker for me, from using hidraw backend on Android, for a while. Today I made a tiny step to using sysfs directly instead of libudev (hid_enumerate/filter over VID/PID).
Second major improvement -
hid_get_*_stringfunctions:wcsncpycrashes of the source string is NULL. Add explicit check.If anyone can additionally test libusb and hidraw backends - I'd highly appreciate it.
Since no one complains about my latest changes - either everything works as expecter or no one had a chance to look :)
@Youw
The hidraw backend seems to have some issues with my Microchip Custom Demo device.
No issue with the libusb backend under Ubuntu 20.04.
But there is no way to open the device using the hidraw backend. It can not even find the device after the above. Maybe it does not work with hidraw driver.
@Youw
Sorry but the above is a false alarm. The libusb backend will detach the hidraw driver so that hidraw backend will not be able to find the device. There will be no issue running the hidtest-hidraw first.
Results of the Logitech USB Receiver under Linux.
That is actually unexpected. LIBUSB backed should have re-attach the kernel (hidraw) driver back, once the device is closed.
Let me check.
As far as I can tell - the driver was reattached (otherwise you wouldn't be able to enumerate it with hidraw after libusb).
Maybe there is adifferent issue - likely outside of this PR's scope.
There is an interesting side-effect/improvement in libusb backed:
Before this change, the only way to get (primary) usage_page/usage was to compile hidapi with
INVASIVE_GET_USAGE.Now - it is possible to to get it with
hid_get_device_infowith default compilation flags.I.e. (default compilation flags):
hid_enumerate:hid_get_device_info:But that works only if device supportsGETLIBUSB_DT_REPORT- which is not always true. I have several devices that do not have this usb request implemented, even though HID Report Descriptor is still present among all USB descriptors.UPD: see #438.
It can not enumerate with the hidraw after libusb. Therefore I think the re-attach does not work in this case, probably due to the use of CTRL-C to terminate the hidtest application. So I do not think this is a real issue.
Ah, right.
Agree.
Thanks for the tip. I will try this on the Teensy HID device.