Open
Conversation
Author
|
Currently, some stuff like the Report Descriptor parser and error registry routine are copied from Linux and I think they are platform independent. Can we create a common directory or hidraw directory in code then put them inside? Have tested by the hidtest program. |
Youw
reviewed
Mar 27, 2025
Youw
reviewed
Mar 27, 2025
Youw
reviewed
Mar 27, 2025
Youw
reviewed
Mar 27, 2025
Youw
reviewed
Mar 27, 2025
Youw
reviewed
Mar 27, 2025
Youw
reviewed
Mar 27, 2025
aokblast
force-pushed
the
use_hid_raw
branch
4 times, most recently
from
49a7a62 to
6e7a25a
Compare
Author
|
Thanks for @Youw your review:). Then, What is your opinion about this?
|
Member
I thinjk that is a good idea, but I don't think it really is nesessary to do so in scope of this PR. |
Member
|
Nice. This will address the following issue. |
Author
|
Oops. I forget we have udev-devd stuff. Maybe we should use udev also? |
Member
I lost context here. What for? Seem like you have all the functionality implemented already. Aren't you? |
Author
Yes, all functionality is fully implemented. I am just thinking if we should use libudev make hidapi more portable. |
Member
|
First test under FreeBSD 14.1 Release, under a physical machine (Chuwi mini PC, Intel J4125 CPU, 8GB RAM, 256GB SSD) There are a few compiler warnings. |
Author
|
Fix it:). Forget to fix the warning. |
Member
|
Somehow hidtest-hidraw will seg fault with the Microchip Simple HID example. |
Member
Thanks. The compiler warnings are gone. The Segfault issue is still there though. |
Author
Sorry that I was super busy in previous 6 months. I am back now and will start to setup the hardware for usb development on next monday. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new FreeBSD HIDRAW backend to HIDAPI so FreeBSD (13.0+) can enumerate and access HID devices via /dev/hidraw*, while still leveraging libusb for USB-specific metadata (e.g., manufacturer string).
Changes:
- Introduce a new
freebsd/backend implementation (hid.c) and integrate it into both CMake and Autotools builds. - Update build/test targets to build both
hidrawandlibusbvariants on FreeBSD (hidtest + testgui). - Adjust FreeBSD libusb build artifacts naming to match the
-libusbconvention.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| testgui/Makefile.am | Build FreeBSD testgui against both freebsd hidraw and libusb backends. |
| src/CMakeLists.txt | Add FreeBSD platform branch to build/export the hidraw backend from freebsd/. |
| libusb/Makefile.freebsd | Rename manual FreeBSD libusb targets to *-libusb naming. |
| libusb/Makefile.am | Rename FreeBSD libusb Automake library target to libhidapi-libusb.la. |
| hidtest/Makefile.am | Build both hidtest-hidraw and hidtest-libusb on FreeBSD. |
| hidtest/CMakeLists.txt | Enable building hidtest_hidraw on FreeBSD when hidapi::hidraw exists. |
| freebsd/hid.c | New FreeBSD hidraw backend implementation (enumeration, IO, descriptors, strings). |
| freebsd/Makefile.am | Automake build for freebsd hidraw library. |
| freebsd/Makefile-manual | Manual makefile to build the FreeBSD hidraw backend. |
| freebsd/CMakeLists.txt | CMake target for FreeBSD hidraw backend and pkg-config generation. |
| freebsd/.gitignore | Ignore build artifacts under freebsd/. |
| configure.ac | Switch FreeBSD Autotools backend to freebsd and add FreeBSD Makefile/config vars. |
| Makefile.am | Add freebsd/ subdir and install FreeBSD pkg-config files for hidraw+libusb. |
| CMakeLists.txt | Add FreeBSD options for building hidraw and libusb implementations. |
Comments suppressed due to low confidence (1)
configure.ac:249
- Autotools config only generates
pc/hidapi-hidraw.pcandpc/hidapi-libusb.pcwhenos == linux, but the top-levelMakefile.amnow installs those pc files for FreeBSD as well. This will breakmake/make installon FreeBSD because the.pcfiles won't be configured/generated. Update this conditional to include FreeBSD (or generate the correct pc files based onOS_FREEBSD).
if test "x$os" = xlinux; then
AC_CONFIG_FILES([pc/hidapi-hidraw.pc])
AC_CONFIG_FILES([pc/hidapi-libusb.pc])
else
AC_CONFIG_FILES([pc/hidapi.pc])
fi
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+506
to
+513
| hid_get_udev_location_from_hidraw_idx(idx,&bus, | ||
| &addr,&info->interface_number); | ||
| handler = hid_find_device_handle_by_bus_and_port(bus, addr); | ||
| if (!handler) | ||
| break; | ||
| device = libusb_get_device(handler); | ||
| if (libusb_get_device_descriptor(device, &desc)) { | ||
| libusb_close(handler); |
There was a problem hiding this comment.
In the BUS_USB branch, the return value of hid_get_udev_location_from_hidraw_idx() is ignored, but bus/addr are then used even if the lookup failed (they're uninitialized). Also, on libusb_get_device_descriptor() failure the handle is closed but the code continues and uses handler afterward. Check both errors and bail out before using bus/addr or a closed handle.
Suggested change
| hid_get_udev_location_from_hidraw_idx(idx,&bus, | |
| &addr,&info->interface_number); | |
| handler = hid_find_device_handle_by_bus_and_port(bus, addr); | |
| if (!handler) | |
| break; | |
| device = libusb_get_device(handler); | |
| if (libusb_get_device_descriptor(device, &desc)) { | |
| libusb_close(handler); | |
| if (hid_get_udev_location_from_hidraw_idx(idx, &bus, | |
| &addr, &info->interface_number) != 0) | |
| break; | |
| handler = hid_find_device_handle_by_bus_and_port(bus, addr); | |
| if (!handler) | |
| break; | |
| device = libusb_get_device(handler); | |
| if (libusb_get_device_descriptor(device, &desc)) { | |
| libusb_close(handler); | |
| break; |
Comment on lines
+516
to
+520
| if (len == 0) { | ||
| libusb_close(handler); | ||
| break; | ||
| } | ||
| mbstowcs(namebuffer, (char *)buffer, sizeof(namebuffer)); |
There was a problem hiding this comment.
libusb_get_string_descriptor_ascii() returns a negative libusb error code on failure, not 0. The current len == 0 check will miss errors. Also, mbstowcs(..., sizeof(namebuffer)) passes a byte count instead of the number of wchar_t elements and can overflow namebuffer. Validate len > 0 and pass the correct element count (and ensure null-termination) before wcsdup().
Suggested change
| if (len == 0) { | |
| libusb_close(handler); | |
| break; | |
| } | |
| mbstowcs(namebuffer, (char *)buffer, sizeof(namebuffer)); | |
| if (len <= 0) { | |
| libusb_close(handler); | |
| break; | |
| } | |
| if ((size_t) len >= sizeof(buffer)) | |
| len = sizeof(buffer) - 1; | |
| buffer[len] = '\0'; | |
| { | |
| size_t converted; | |
| size_t namebuffer_len = sizeof(namebuffer) / sizeof(namebuffer[0]); | |
| converted = mbstowcs(namebuffer, (char *)buffer, namebuffer_len - 1); | |
| if (converted == (size_t)-1) | |
| namebuffer[0] = L'\0'; | |
| else | |
| namebuffer[converted] = L'\0'; | |
| } |
Comment on lines
+565
to
+577
| template = malloc(sizeof(struct hid_device_info)); | ||
| template->path = strdup(devpath); | ||
| template->vendor_id = devinfo.hdi_vendor; | ||
| template->product_id = devinfo.hdi_product; | ||
| mbstowcs(namebuffer, devinfo.hdi_uniq, sizeof(namebuffer)); | ||
| template->serial_number = strlen(devinfo.hdi_uniq) ? wcsdup(namebuffer) : NULL; | ||
| template->release_number = devinfo.hdi_version; | ||
| mbstowcs(namebuffer, devinfo.hdi_name, sizeof(namebuffer)); | ||
| template->product_string = strlen(devinfo.hdi_name) ? wcsdup(namebuffer) : NULL; | ||
| template->interface_number = -1; | ||
| template->manufacturer_string = NULL; | ||
| template->next = NULL; | ||
| hid_device_handle_bus_dependent(&devinfo, template, idx); |
There was a problem hiding this comment.
template is allocated with malloc() and then partially initialized. Any fields not explicitly set (e.g., usage_page, usage, bus_type, etc.) remain uninitialized and may be copied later. Prefer calloc() (or memset) and check for allocation failures before dereferencing template.
Comment on lines
+569
to
+573
| mbstowcs(namebuffer, devinfo.hdi_uniq, sizeof(namebuffer)); | ||
| template->serial_number = strlen(devinfo.hdi_uniq) ? wcsdup(namebuffer) : NULL; | ||
| template->release_number = devinfo.hdi_version; | ||
| mbstowcs(namebuffer, devinfo.hdi_name, sizeof(namebuffer)); | ||
| template->product_string = strlen(devinfo.hdi_name) ? wcsdup(namebuffer) : NULL; |
There was a problem hiding this comment.
Both mbstowcs(namebuffer, ..., sizeof(namebuffer)) calls pass a byte size instead of the number of wchar_t elements. This can overflow namebuffer (256 elements) because sizeof(namebuffer) is typically 1024 bytes. Use sizeof(namebuffer)/sizeof(namebuffer[0]) (and handle truncation / ensure termination) before wcsdup().
Suggested change
| mbstowcs(namebuffer, devinfo.hdi_uniq, sizeof(namebuffer)); | |
| template->serial_number = strlen(devinfo.hdi_uniq) ? wcsdup(namebuffer) : NULL; | |
| template->release_number = devinfo.hdi_version; | |
| mbstowcs(namebuffer, devinfo.hdi_name, sizeof(namebuffer)); | |
| template->product_string = strlen(devinfo.hdi_name) ? wcsdup(namebuffer) : NULL; | |
| if (strlen(devinfo.hdi_uniq)) { | |
| size_t converted = mbstowcs(namebuffer, devinfo.hdi_uniq, | |
| (sizeof(namebuffer) / sizeof(namebuffer[0])) - 1); | |
| if (converted != (size_t)-1) { | |
| namebuffer[converted] = L'\0'; | |
| template->serial_number = wcsdup(namebuffer); | |
| } else { | |
| template->serial_number = NULL; | |
| } | |
| } else { | |
| template->serial_number = NULL; | |
| } | |
| template->release_number = devinfo.hdi_version; | |
| if (strlen(devinfo.hdi_name)) { | |
| size_t converted = mbstowcs(namebuffer, devinfo.hdi_name, | |
| (sizeof(namebuffer) / sizeof(namebuffer[0])) - 1); | |
| if (converted != (size_t)-1) { | |
| namebuffer[converted] = L'\0'; | |
| template->product_string = wcsdup(namebuffer); | |
| } else { | |
| template->product_string = NULL; | |
| } | |
| } else { | |
| template->product_string = NULL; | |
| } |
Comment on lines
+1182
to
+1187
| if (act_len < buf_size) | ||
| buf_size = act_len; | ||
|
|
||
| memcpy(buf, desc.value, buf_size); | ||
| return buf_size; | ||
| } |
There was a problem hiding this comment.
hid_get_report_descriptor() returns buf_size (a size_t) from a function returning int. This can trigger warnings and truncation on large descriptors. Follow the pattern used by other backends: clamp to INT_MAX if needed and return (int)buf_size after copying.
| register_global_error_format("Unable to find the device with bus:%2d and addr:%2d", bus, addr); | ||
| } | ||
|
|
||
| libusb_free_device_list(list, 0); |
There was a problem hiding this comment.
libusb_free_device_list(list, 0) does not unref the devices in the list and can leak references. The rest of the codebase uses libusb_free_device_list(..., 1) after enumeration; do the same here.
Suggested change
| libusb_free_device_list(list, 0); | |
| libusb_free_device_list(list, 1); |
Comment on lines
+479
to
+483
| snprintf(buff, sizeof(buff), "dev.hidbus.%d.%%parent", idx); | ||
| len = sizeof(buff); | ||
| if (sysctlbyname(buff, buff, &len, NULL, 0)) | ||
| return 0; | ||
| buff[len] = '\0'; |
There was a problem hiding this comment.
hid_get_udev_location_from_hidraw_idx() writes buff[len] = '\0' after sysctlbyname(). If the returned len equals sizeof(buff), this writes 1 byte past the buffer. Guard len (or ensure null termination by reserving a byte / clamping).
| int oid[CTL_MAXNAME] = {0}; | ||
| size_t buf_len, oid_items = CTL_MAXNAME; | ||
| struct hid_sysctl_iter iter; | ||
|
|
There was a problem hiding this comment.
hid_enumerate() doesn't call hid_init(), but the enumeration path can call into libusb (via hid_device_handle_bus_dependent() -> hid_find_device_handle_by_bus_and_port()). Without initializing libusb first, libusb_get_device_list() may fail/behave unexpectedly. Call hid_init() early in hid_enumerate() (as other backends do) and handle/init errors appropriately.
Suggested change
| if (hid_init() != 0) | |
| return NULL; |
| if (cur_dev->vendor_id == vendor_id && | ||
| cur_dev->product_id == product_id) { | ||
| if (serial_number) { | ||
| if (wcscmp(serial_number, cur_dev->serial_number) == 0) { |
There was a problem hiding this comment.
When a serial_number is provided, wcscmp(serial_number, cur_dev->serial_number) will crash if cur_dev->serial_number is NULL (which can happen when hdi_uniq is empty). Add a null check before calling wcscmp() and only compare when a serial is present.
Suggested change
| if (wcscmp(serial_number, cur_dev->serial_number) == 0) { | |
| if (cur_dev->serial_number && | |
| wcscmp(serial_number, cur_dev->serial_number) == 0) { |
Although some reluctance it seems to provide reasonable and constructive review at least there will be some hints :-) |
Author
|
I don't see any huge change over behavior. The comments from AI are just missing boundary checks. |
Author
Hi. I just take a look at this again. Could you please try to make your payload 128bytes? |
Member
Yes, I will try to carry out some tests over the weekend and report the results from FreeBSD 15.0 release. |
Member
|
Here are the latest test results under FreeBSD 15.0 release. |
Member
|
Summary of the above test for the EZ-USB FX2LP modified Generic HID example with 128 Bytes report size (high speed USB device).
|
Member
|
Sanity check under Windows, with latest hidapi git. |
Author
Does output of payload less than 128 bytes work? FreeBSD check if the device can handle the report by checking the size in report descriptor. You report descriptor shows that we can only handle 128 bytes. |
usbhid allocates internal buffers based on report size. They are 128 bytes long in that case. 129 bytes wont fit them. There is a difference between input and output unnumbered reports. |
Member
This is under Linux. Linux hidraw backend does not work if I send 64 bytes payload. |
|
Ouch. It seems that input is really 128 bytes no 129. |
Member
That should be the correct one. The report ID 00 for the output report is not really on the wire. It is more needed because of Windows native HID API. |
|
Strange. I have just tested FreeBSD hidraw on MBP8,1 with USB touchpad. Touching of surface with all 10 fingers gave me 338bytes packets successfully received. |
Member
Good one. Do you have any High Speed USB HID device? As I mentioned before, "forcing the device to run in Full Speed mode and now this PR works". So your device is most likely a Full Speed USB device. Latest version of VirtualBox does not seem to allow me to disable EHCI support so I can not carry out the test. I am not so sure if I still have working USB 1.1 Hub to try. |
Member
|
On the other hand, High Speed USB HID device is probably rare. In that case, we may still want to merge this PR first and then debug the issue later. Maybe it has nothing to do with this PR but rather the hidraw (usbhid driver) of FreeBSD. |
It is Full Speed device. |
|
Member
|
Going back to original FX2HID example from Jan Axelson, this PR still does not work. No issues under Windows. No issues under FreeBSD with the libusb backend. This PR still does not work. |
Member
Thanks for the tip. I will try this soon. |
Member
Somehow this does not work. The device is still enumerated as a high speed USB device. |
Member
|
The following command seems to disable EHCI driver completely, but then somehow I cannot attach the FX2LP USB HID device. Only a generic Atmel STK600 can be attached. |
Member
|
Okay, this seems to work. Now it seems to work. |
Member
|
Going back to the device with HID Input/Output report size of 128 bytes. It seems to this PR will work fine as well. Good thing -- PR #728 also works fine. |
Member
Member
|
I am still in favour of merging this PR after your review. The reason is that High Speed USB HID devices are not that common. But you can decide. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FreeBSD support hidraw in Kernel from 13.0.
By using libusb only, we can only see the HID device from usb. To address this, we implement hidraw backend for FreeBSD.
Just like Linux use libudev to handle usb specified HID stuff (like Manufacture), we use libusb to handle it.
Sponsored-by: FreeBSD Foundation