Conversation
|
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. |
aokblast
force-pushed
the
use_hid_raw
branch
4 times, most recently
from
49a7a62 to
6e7a25a
Compare
|
Thanks for @Youw your review:). Then, What is your opinion about this?
|
I thinjk that is a good idea, but I don't think it really is nesessary to do so in scope of this PR. |
|
Nice. This will address the following issue. |
|
Oops. I forget we have udev-devd stuff. Maybe we should use udev also? |
I lost context here. What for? Seem like you have all the functionality implemented already. Aren't you? |
Yes, all functionality is fully implemented. I am just thinking if we should use libudev make hidapi more portable. |
|
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. |
|
Fix it:). Forget to fix the warning. |
|
Somehow hidtest-hidraw will seg fault with the Microchip Simple HID example. |
Thanks. The compiler warnings are gone. The Segfault issue is still there though. |
|
Using FreeBSD 15 Current snapshot 1-May-2025. So we can see that the hidraw backend does not work from this PR. But at least it does not have regression on the libusb backend since the problem is the same as current hidapi git in terms of libusb backend. |
|
Now I am clean of the fault, it is the same as what you see. No timeout, immediately exit.
|
|
Ok, I see. I should take some time to get a device able to afford USB HIGH speed |
Are you saying that you no longer have issuses with your own full speed STM32 MCU implementation if the HID Input/Output report length are larger than 64 Bytes? Or you still have the issue? If possible, please post the USB descriptors of your device and the test results. Thanks. |
|
More about the test device. |
|
Forcing the device to run in Full Speed mode and now this PR works. |
|
@aokblast @wulf7 |
Yes, hid full speed with larger than 64 byte report works. Code and result: usbconfig -d0.3 dump_all_desc: Report Descriptor:
|
|
If you can, please get the FX2LP break-out board from Taobao/AliExpress/Amazon/etc. It is pretty cheap. FW binary and source codes are here. It is a quick minor modification of Jan Axelson's FX2HID example from here. There may be a bit of challenge if you want to rebuild the source code as it may require a full version of Keil C51 compiler. I happen to have access to a pretty old version of Keil C51 compiler at work. I am not so sure about the efforts to port the code to use the free sdcc toolchain. It may be possible because of the nice sdcc based libraries. Jan's HID page has some other FW as well, for example, for Microchip USB PIC18. I was able to use that as a base for my other HIDAPI testing but I have not figured out how to increase the HID report size above 64 bytes with that FW. I have the PIC18F87J50 USB PIM (free tool from Microchip many years ago). More testing device discussions: |
|
Update with the main git branch to trigger another CI run. |
|
Since you two are the FreeBSD power user I know of, please help to check if you can give this PR a try. You need to use FreeBSD 14 Stable or 15 Current. Thanks. This has something to do with avrdude as well. For example, if it works, it is a potential workaround for Issue #274. FreeBSD HIDAPI is now based on libusb, so it is affected by the Issue #274. |
|
@mcuee builds okay, could you please provide test steps, its a long thread and I am quite short on time sorry :-P |
Great. Test steps: make sure you use the hidraw driver.
I have mentioned my test device in the comments above. |
I just ordered one. Let's seen when it arrives. |
|
I am in Canada right now. I get the USB3320 and try to setup the experiment environment with my stm32h750. I will test with it later after my experiment environment is setup with other patches you have mentioned. |
@aokblast I tend to believe my modification FW may not be totally correct (interrupt transfer for the IN/OUT endpoint may be correct; Control Transfer to the IN/OUT endpoint may need to be checked). I am not a progammer myself and my main contributions for the Open Source projects (eg: libusb, libusb-win32, libusbK, avrdude, pyusb, libusbdotnet, libftdi and OpenOCD) are mainly on the testing and technical side. STM32 is a popular ARM Cortex MCU family and STM32H750 is a powerful one, very good for libusb and hidapi related testing. If you need Super Speed USB testing device (probably not needed for hidapi but more for libusb project), I will highly recomemend EZ-USB FX3 board, CYUSB3KIT-003 EZ-USB™ FX3 SuperSpeed explorer kit, at US$$45.93 plus shipment. Example test FW used in libusbK project, based on Cypress (now part of Infineon) FW examples. |
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 Sponsored-by: Framework Laptop. Inc
|
Just wondering if you have any updates on this PR. Thanks. |
|
Just update the branch to trigger a new build. It will be good to move this PR forward. May need more testing and reviews. |
|
I am on FreeBSD 14.4-RELEASE AMD64 just in case :-) |
|
Just wondering if it is worth asking Github Copilot to review this PR. |
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.
| 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.
| 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; |
| 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().
| 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'; | |
| } |
| 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.
| 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().
| 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; | |
| } |
| 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.
| libusb_free_device_list(list, 0); | |
| libusb_free_device_list(list, 1); |
| 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.
| 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.
| if (wcscmp(serial_number, cur_dev->serial_number) == 0) { | |
| if (cur_dev->serial_number && | |
| wcscmp(serial_number, cur_dev->serial_number) == 0) { |
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <sys/fcntl.h> | ||
| #include <sys/ioccom.h> | ||
| #include <sys/param.h> | ||
| #include <sys/sysctl.h> | ||
| #include <sys/user.h> | ||
| #include <unistd.h> |
There was a problem hiding this comment.
This file calls ioctl() but does not include <sys/ioctl.h>. On some toolchains this will fail to compile (implicit declaration is an error with modern C standards / -Werror). Add the proper include near the other system headers.
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