gh-libusb/hidapi
Watch
1
0
Fork
You've already forked hidapi
 0
Code Issues 58 Pull requests 18 Releases 13 Wiki Activity

Initial NetBSD/UHID native backend implementation #612

Merged
ghost merged 3 commits from netbsd into master 2023-09-04 04:25:53 -05:00
Conversation 30 Commits 3 Files changed 6 +1263
ghost commented 2023-08-17 04:29:08 -05:00 (Migrated from github.com)

This is the culmination of the research I did for uhid on NetBSD. It should be as complete as a native backend can be given the limitations of NetBSD's kernel and uhid as a whole. See related issue, #608.

This is the culmination of the research I did for `uhid` on NetBSD. It should be as complete as a native backend can be given the limitations of NetBSD's kernel and `uhid` as a whole. See related issue, #608.
👍 1 🎉 1
Youw (Migrated from github.com) reviewed 2023-08-17 15:53:09 -05:00
CMakeLists.txt
@ -42,6 +42,9 @@ elseif(NOT WIN32)
option(HIDAPI_WITH_HIDRAW "Build HIDRAW-based implementation of HIDAPI" ON)
option(HIDAPI_WITH_LIBUSB "Build LIBUSB-based implementation of HIDAPI" ON)
endif()
if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
Youw (Migrated from github.com) commented 2023-08-17 15:53:09 -05:00

Just to double-check: is uhid backend specific to NetBSD or is it available on other BSD systems?

Just to double-check: is `uhid` backend specific to NetBSD or is it available on other BSD systems?
Ghost reviewed 2023-08-17 16:01:26 -05:00
CMakeLists.txt
@ -42,6 +42,9 @@ elseif(NOT WIN32)
option(HIDAPI_WITH_HIDRAW "Build HIDRAW-based implementation of HIDAPI" ON)
option(HIDAPI_WITH_LIBUSB "Build LIBUSB-based implementation of HIDAPI" ON)
endif()
if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
Ghost commented 2023-08-17 16:01:26 -05:00

Technically it is available to other BSD systems since it has existed since sometime in 1990. But they have diverged heavily over the years so they are all at varying levels of compatibility not to mention differences in internal kernel behavior. The compatible subset I found is extremely limited and would not make for a very useful generic approach. I thought it best to combine uhid with platform specific support code to help workaround its limitations. NetBSD probably has the most advanced uhid implementation in terms of overall features.

Technically it is available to other BSD systems since it has existed since sometime in 1990. But they have diverged heavily over the years so they are all at varying levels of compatibility not to mention differences in internal kernel behavior. The compatible subset I found is extremely limited and would not make for a very useful generic approach. I thought it best to combine `uhid` with platform specific support code to help workaround its limitations. NetBSD probably has the most advanced `uhid` implementation in terms of overall features.
Youw (Migrated from github.com) reviewed 2023-08-18 05:38:02 -05:00
CMakeLists.txt
@ -42,6 +42,9 @@ elseif(NOT WIN32)
option(HIDAPI_WITH_HIDRAW "Build HIDRAW-based implementation of HIDAPI" ON)
option(HIDAPI_WITH_LIBUSB "Build LIBUSB-based implementation of HIDAPI" ON)
endif()
if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
Youw (Migrated from github.com) commented 2023-08-18 05:38:02 -05:00

Thanks, this is helpfull.
I agree, better keep it for NetBSD only.

Thanks, this is helpfull. I agree, better keep it for NetBSD only.
ghost commented 2023-08-18 17:45:45 -05:00 (Migrated from github.com)

Minor code cleanup. I noticed some improvements I could make when reusing some of the code for another project of mine.

Minor code cleanup. I noticed some improvements I could make when reusing some of the code for another project of mine.
mcuee commented 2023-08-21 08:36:40 -05:00 (Migrated from github.com)

Nice, the first simple test seems to work fine. I need to modify hidapitester to test this more.

I am using NetBSD 9.3 VM running under Proxmox and I attach a generic HID device to the VM.

localhost$ cmake -B build -D HIDAPI_BUILD_HIDTEST=1
-- The C compiler identification is GNU 7.5.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- hidapi: v0.14.0
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE
-- Found PkgConfig: /usr/pkg/bin/pkg-config (found version "1.9.4")
-- Checking for module 'libusb-1.0>=1.0.9'
--   Found libusb-1.0, version 1.0.24
-- Check for Iconv
-- Performing Test Iconv_IS_BUILT_IN
-- Performing Test Iconv_IS_BUILT_IN - Success
-- Found Iconv: built in to C library
-- Performing Test HIDAPI_ICONV_CONST
-- Performing Test HIDAPI_ICONV_CONST - Success
-- Building hidtest
-- Configuring done
-- Generating done
-- Build files have been written to: /home/mcuee/build/hidapi_netbsd/build
localhost$ cmake --build build
[ 16%] Building C object src/netbsd/CMakeFiles/hidapi_netbsd.dir/hid.c.o
[ 33%] Linking C shared library libhidapi-netbsd.so
[ 33%] Built target hidapi_netbsd
[ 50%] Building C object src/libusb/CMakeFiles/hidapi_libusb.dir/hid.c.o
[ 66%] Linking C shared library libhidapi-libusb.so
[ 66%] Built target hidapi_libusb
[ 83%] Building C object hidtest/CMakeFiles/hidtest.dir/test.c.o
[100%] Linking C executable hidtest
[100%] Built target hidtest
localhost$ sudo ./build/hidtest/hidtest
Password:
hidapi test/example tool. Compiled with hidapi version 0.14.0, runtime version 0.14.0.
Compile-time version matches runtime version of hidapi.

Device Found
  type: 0925 7001
  path: uhidev0
  serial_number:
  Manufacturer: Microchip Technology Inc.
  Product:      Generic HID
  Release:      1
  Interface:    -1
  Usage (page): 0x1 (0xffa0)
  Bus type: 1 (USB)

  Report Descriptor: (47 bytes)
0x06, 0xa0, 0xff, 0x09, 0x01, 0xa1, 0x01, 0x09, 0x03, 0x15,
0x00, 0x26, 0xff, 0x00, 0x75, 0x08, 0x95, 0x3f, 0x81, 0x02,
0x09, 0x04, 0x15, 0x00, 0x26, 0xff, 0x00, 0x75, 0x08, 0x95,
0x3f, 0x91, 0x02, 0x09, 0x05, 0x15, 0x00, 0x26, 0xff, 0x00,
0x75, 0x08, 0x95, 0x3f, 0xb1, 0x02, 0xc0,
unable to open device

localhost$ uname -a
NetBSD localhost 9.3 NetBSD 9.3 (GENERIC) #0: Thu Aug  4 15:30:37 UTC 2022  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64

localhost$ sudo usbdevs
addr 1: UHCI root hub, NetBSD
addr 0: xHCI root hub, NetBSD
addr 0: xHCI root hub, NetBSD
 addr 1: Generic HID, Microchip Technology Inc.
Nice, the first simple test seems to work fine. I need to modify hidapitester to test this more. I am using NetBSD 9.3 VM running under Proxmox and I attach a generic HID device to the VM. ``` localhost$ cmake -B build -D HIDAPI_BUILD_HIDTEST=1 -- The C compiler identification is GNU 7.5.0 -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /usr/bin/cc - skipped -- Detecting C compile features -- Detecting C compile features - done -- hidapi: v0.14.0 -- Looking for pthread.h -- Looking for pthread.h - found -- Performing Test CMAKE_HAVE_LIBC_PTHREAD -- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed -- Looking for pthread_create in pthreads -- Looking for pthread_create in pthreads - not found -- Looking for pthread_create in pthread -- Looking for pthread_create in pthread - found -- Found Threads: TRUE -- Found PkgConfig: /usr/pkg/bin/pkg-config (found version "1.9.4") -- Checking for module 'libusb-1.0>=1.0.9' -- Found libusb-1.0, version 1.0.24 -- Check for Iconv -- Performing Test Iconv_IS_BUILT_IN -- Performing Test Iconv_IS_BUILT_IN - Success -- Found Iconv: built in to C library -- Performing Test HIDAPI_ICONV_CONST -- Performing Test HIDAPI_ICONV_CONST - Success -- Building hidtest -- Configuring done -- Generating done -- Build files have been written to: /home/mcuee/build/hidapi_netbsd/build localhost$ cmake --build build [ 16%] Building C object src/netbsd/CMakeFiles/hidapi_netbsd.dir/hid.c.o [ 33%] Linking C shared library libhidapi-netbsd.so [ 33%] Built target hidapi_netbsd [ 50%] Building C object src/libusb/CMakeFiles/hidapi_libusb.dir/hid.c.o [ 66%] Linking C shared library libhidapi-libusb.so [ 66%] Built target hidapi_libusb [ 83%] Building C object hidtest/CMakeFiles/hidtest.dir/test.c.o [100%] Linking C executable hidtest [100%] Built target hidtest localhost$ sudo ./build/hidtest/hidtest Password: hidapi test/example tool. Compiled with hidapi version 0.14.0, runtime version 0.14.0. Compile-time version matches runtime version of hidapi. Device Found type: 0925 7001 path: uhidev0 serial_number: Manufacturer: Microchip Technology Inc. Product: Generic HID Release: 1 Interface: -1 Usage (page): 0x1 (0xffa0) Bus type: 1 (USB) Report Descriptor: (47 bytes) 0x06, 0xa0, 0xff, 0x09, 0x01, 0xa1, 0x01, 0x09, 0x03, 0x15, 0x00, 0x26, 0xff, 0x00, 0x75, 0x08, 0x95, 0x3f, 0x81, 0x02, 0x09, 0x04, 0x15, 0x00, 0x26, 0xff, 0x00, 0x75, 0x08, 0x95, 0x3f, 0x91, 0x02, 0x09, 0x05, 0x15, 0x00, 0x26, 0xff, 0x00, 0x75, 0x08, 0x95, 0x3f, 0xb1, 0x02, 0xc0, unable to open device localhost$ uname -a NetBSD localhost 9.3 NetBSD 9.3 (GENERIC) #0: Thu Aug 4 15:30:37 UTC 2022 mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64 localhost$ sudo usbdevs addr 1: UHCI root hub, NetBSD addr 0: xHCI root hub, NetBSD addr 0: xHCI root hub, NetBSD addr 1: Generic HID, Microchip Technology Inc. ```
ghost commented 2023-08-22 00:18:02 -05:00 (Migrated from github.com)

Nice, the first simple test seems to work fine. I need to modify hidapitester to test this more.

I am using NetBSD 9.3 VM running under Proxmox and I attach a generic HID device to the VM.

Feel free to refine it further but I have reached the limit of what I can see to do. I am considering writing a FreeBSD backend after NetBSD has been merged. It is probably the last native BSD backend that has enough kernel API functionality to work with the current library abstraction. I may end up creating two versions for FreeBSD due to it having the option of using a hidraw compatibility layer as well as the older uhid one.

> Nice, the first simple test seems to work fine. I need to modify hidapitester to test this more. > > I am using NetBSD 9.3 VM running under Proxmox and I attach a generic HID device to the VM. Feel free to refine it further but I have reached the limit of what I can see to do. I am considering writing a FreeBSD backend after NetBSD has been merged. It is probably the last native BSD backend that has enough kernel API functionality to work with the current library abstraction. I may end up creating two versions for FreeBSD due to it having the option of using a `hidraw` compatibility layer as well as the older `uhid` one.
👍 1
mcuee commented 2023-08-22 01:43:15 -05:00 (Migrated from github.com)

I am considering writing a FreeBSD backend after NetBSD has been merged. It is probably the last native BSD backend that has enough kernel API functionality to work with the current library abstraction. I may end up creating two versions for FreeBSD due to it having the option of using a hidraw compatibility layer as well as the older uhid one.

That would be wonderful. I tend to think the 1hidraw` backend is the way to go for FreeBSD.

> I am considering writing a FreeBSD backend after NetBSD has been merged. It is probably the last native BSD backend that has enough kernel API functionality to work with the current library abstraction. I may end up creating two versions for FreeBSD due to it having the option of using a `hidraw` compatibility layer as well as the older `uhid` one. That would be wonderful. I tend to think the 1hidraw` backend is the way to go for FreeBSD. * https://github.com/libusb/hidapi/issues/543
ghost commented 2023-08-23 08:22:53 -05:00 (Migrated from github.com)

That would be wonderful. I tend to think the 1hidraw` backend is the way to go for FreeBSD.

Preliminary investigation suggests this has its own problems due to how new this backend is. It isn't enabled by default on fresh installations. Of the two, uhid and hidraw, only one can be attached to any particular device.

uhid is the default if hw.usb.usbhid.enable is set to 0. hidraw can only be used if hw.usb.usbhid.enable is set to 1 and the hidraw module is loaded explicitly. If hw.usb.usbhid.enable is set to 1 but hidraw is not available, there won't be anything setup to provide userspace HID access.

Any solution would probably have to be prepared to use whichever API the FreeBSD environment is configured to use as I don't think it would be a good practice to forcefully enable hidraw against the wishes of the end user.

> That would be wonderful. I tend to think the 1hidraw` backend is the way to go for FreeBSD. Preliminary investigation suggests this has its own problems due to how new this backend is. It isn't enabled by default on fresh installations. Of the two, `uhid` and `hidraw`, only one can be attached to any particular device. `uhid` is the default if `hw.usb.usbhid.enable` is set to 0. `hidraw` can only be used if `hw.usb.usbhid.enable` is set to 1 and the `hidraw` module is loaded explicitly. If `hw.usb.usbhid.enable` is set to 1 but `hidraw` is not available, there won't be anything setup to provide userspace HID access. Any solution would probably have to be prepared to use whichever API the FreeBSD environment is configured to use as I don't think it would be a good practice to forcefully enable `hidraw` against the wishes of the end user.
Youw commented 2023-08-23 11:01:54 -05:00 (Migrated from github.com)

I don't think it would be a good practice to forcefully enable hidraw against the wishes of the end user.

I agree. I'd say that even if we will get the hidraw backend implementation for FreeBSD - that is absolutely fine that it will be functional only if user explicitly enables the hidraw subsystem/driver in his environmnet on his own.

> I don't think it would be a good practice to forcefully enable `hidraw` against the wishes of the end user. I agree. I'd say that even if we will get the `hidraw` backend implementation for FreeBSD - that is absolutely fine that it will be functional only if user explicitly enables the `hidraw` subsystem/driver in his environmnet on his own.
👍 1
mcuee commented 2023-08-23 22:27:52 -05:00 (Migrated from github.com)

That would be wonderful. I tend to think the 1hidraw` backend is the way to go for FreeBSD.

Preliminary investigation suggests this has its own problems due to how new this backend is. It isn't enabled by default on fresh installations. Of the two, uhid and hidraw, only one can be attached to any particular device.

uhid is the default if hw.usb.usbhid.enable is set to 0. hidraw can only be used if hw.usb.usbhid.enable is set to 1 and the hidraw module is loaded explicitly. If hw.usb.usbhid.enable is set to 1 but hidraw is not available, there won't be anything setup to provide userspace HID access.

Any solution would probably have to be prepared to use whichever API the FreeBSD environment is configured to use as I don't think it would be a good practice to forcefully enable hidraw against the wishes of the end user.

Thanks for the analysis.

However, for the FreeBSD OS, there is already a libusb backend which will be the default as it already works fine (other than common libusb issues: https://github.com/libusb/hidapi/issues?q=is%3Aopen+is%3Aissue+label%3Alibusb). So libusb backend will be the preferred backend in the foreseeable future.

I believe ugen driver is also attached to generic USB HID device along with uhid (or hidraw). That is why it works.

I see this as similar to Linux side last time -- libusb was the preferred and hidraw was secondary as there was too many limitations previously. Now hidraw is the preferred.

But of course you can develop uhid backend first as it is more common for now. However, I tend to believe hidraw may be the future for FreeBSD as well (and maybe NetBSD/OpenBSD).

> > That would be wonderful. I tend to think the 1hidraw` backend is the way to go for FreeBSD. > > Preliminary investigation suggests this has its own problems due to how new this backend is. It isn't enabled by default on fresh installations. Of the two, `uhid` and `hidraw`, only one can be attached to any particular device. > > `uhid` is the default if `hw.usb.usbhid.enable` is set to 0. `hidraw` can only be used if `hw.usb.usbhid.enable` is set to 1 and the `hidraw` module is loaded explicitly. If `hw.usb.usbhid.enable` is set to 1 but `hidraw` is not available, there won't be anything setup to provide userspace HID access. > > Any solution would probably have to be prepared to use whichever API the FreeBSD environment is configured to use as I don't think it would be a good practice to forcefully enable `hidraw` against the wishes of the end user. Thanks for the analysis. However, for the FreeBSD OS, there is already a libusb backend which will be the default as it already works fine (other than common libusb issues: https://github.com/libusb/hidapi/issues?q=is%3Aopen+is%3Aissue+label%3Alibusb). So libusb backend will be the preferred backend in the foreseeable future. I believe ugen driver is also attached to generic USB HID device along with uhid (or hidraw). That is why it works. I see this as similar to Linux side last time -- libusb was the preferred and hidraw was secondary as there was too many limitations previously. Now hidraw is the preferred. But of course you can develop uhid backend first as it is more common for now. However, I tend to believe hidraw may be the future for FreeBSD as well (and maybe NetBSD/OpenBSD).
mcuee commented 2023-08-25 21:02:25 -05:00 (Migrated from github.com)

@Youw

This PR does not support the existing auto-tools build system. Do we want to keep the auto-tools support?

@Youw This PR does not support the existing auto-tools build system. Do we want to keep the auto-tools support?
ghost commented 2023-08-25 21:04:58 -05:00 (Migrated from github.com)

This PR does not support the existing auto-tools build system. Do we want to keep the auto-tools support?

I left it out because I found comments saying that it was deprecated so I assumed it was going to be removed in the near future.

> This PR does not support the existing auto-tools build system. Do we want to keep the auto-tools support? I left it out because I found comments saying that it was deprecated so I assumed it was going to be removed in the near future.
Youw commented 2023-08-26 02:54:51 -05:00 (Migrated from github.com)

Do we want to keep the auto-tools support?

No.

I left it out because I found comments saying that it was deprecated so I assumed it was going to be removed in the near future.

This is exactly why.

BTW: I've reviewed all CMake changes - very consistent with what we already have. Very nice, thanks.

> Do we want to keep the auto-tools support? No. > I left it out because I found comments saying that it was deprecated so I assumed it was going to be removed in the near future. This is exactly why. --- BTW: I've reviewed all CMake changes - very consistent with what we already have. Very nice, thanks.
Youw (Migrated from github.com) reviewed 2023-08-26 03:27:43 -05:00
Youw (Migrated from github.com) left a comment

Overall very consistent implementation.
I didn't check all of the implementation/logic (specially what consernes the uhid API/usage) but overall looks very good.
Just a few comments/improvements to consider.

Overall very consistent implementation. I didn't check all of the implementation/logic (specially what consernes the `uhid` API/usage) but overall looks very good. Just a few comments/improvements to consider.
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Youw (Migrated from github.com) commented 2023-08-26 02:55:55 -05:00 
 Copyright 2023, All Rights Reserved.
```suggestion Copyright 2023, All Rights Reserved. ```
Youw (Migrated from github.com) commented 2023-08-26 02:58:29 -05:00

this is just me being pedantic:
in all other backends the device_handle is the very first member in a struct
lets have it as a first here too

this is just me being pedantic: in all other backends the `device_handle` is the very first member in a `struct` lets have it as a first here too
Youw (Migrated from github.com) commented 2023-08-26 03:03:54 -05:00

I've just merged additional change to linux/hidraw to master that changes this function
lets update here as well

I've just merged additional change to linux/hidraw to master that changes this function lets update here as well
Youw (Migrated from github.com) commented 2023-08-26 03:10:54 -05:00

this is probably a copy-paste (or not), but since technically we're writing this from scratch - I'd prefer reading such conditionas as:

		if (get_next_hid_usage(ucrd->ucrd_data, ucrd->ucrd_size, &pos, &page, &usage) == 0) {

generally speaking, having a negation (!) automatically triggers human brain to look for a failed call (even if often not the case in unix world)

this is probably a copy-paste (or not), but since technically we're writing this from scratch - I'd prefer reading such conditionas as: ```suggestion if (get_next_hid_usage(ucrd->ucrd_data, ucrd->ucrd_size, &pos, &page, &usage) == 0) { ``` generally speaking, having a negation (`!`) automatically triggers human brain to look for a failed call (even if often not the case in unix world)
Youw (Migrated from github.com) commented 2023-08-26 03:13:12 -05:00

lets have a (global) named constant/define for 256 (HIDAPI_MAX_SUPPORTED_DEVICES)?

lets have a (global) named constant/define for 256 (`HIDAPI_MAX_SUPPORTED_DEVICES`)?
Youw (Migrated from github.com) commented 2023-08-26 03:14:05 -05:00

how about:

	l_children_to_enumerate = min(dla.l_children, HIDAPI_MAX_SUPPORTED_DEVICES);
how about: ```suggestion l_children_to_enumerate = min(dla.l_children, HIDAPI_MAX_SUPPORTED_DEVICES); ```
Youw (Migrated from github.com) commented 2023-08-26 03:15:00 -05:00

is this 256 is same as above or not?
(use same or different named constant accordingly)

is this `256` is same as above or not? (use same or different named constant accordingly)
Youw (Migrated from github.com) commented 2023-08-26 03:16:25 -05:00

can you confirm this is required for this backend and not just a copy-paste?

can you confirm this is required for this backend and not just a copy-paste?
Youw (Migrated from github.com) commented 2023-08-26 03:17:33 -05:00

I'm seeing 16 and 32 magic numbers often - lets have a named constant for that too

I'm seeing `16` and `32` magic numbers often - lets have a named constant for that too
Youw (Migrated from github.com) commented 2023-08-26 03:17:51 -05:00

in all places

in all places
netbsd/hid.c
@ -0,0 +3,4 @@
communication with HID devices.
James Buren
libusb/hidapi Team
Youw (Migrated from github.com) commented 2023-08-26 02:55:46 -05:00

@braewoods add your name here as well - you made it happen

@braewoods add your name here as well - you made it happen
netbsd/hid.c
@ -0,0 +772,4 @@
hid_free_enumeration(devs);
if (*path == '\0') {
register_global_error("Device with requested VID/PID/(SerialNumber) not found");
Youw (Migrated from github.com) commented 2023-08-26 03:19:18 -05:00

NOTE: global error never clears
if the previous call has failed, and the next one didn't - the global error still going to point to the previous failure

NOTE: global error never clears if the previous call has failed, and the next one didn't - the global error still going to point to the previous failure
netbsd/hid.c
@ -0,0 +779,4 @@
return hid_open_path(path);
}
HID_API_EXPORT hid_device * HID_API_CALL hid_open_path(const char *path)
Youw (Migrated from github.com) commented 2023-08-26 03:19:28 -05:00

global error not cleared

global error not cleared
netbsd/hid.c
@ -0,0 +826,4 @@
strlcpy(devpath, "/dev/", sizeof(devpath));
strlcat(devpath, child_dev, sizeof(devpath));
uhid = open(devpath, O_RDWR | O_CLOEXEC);
Youw (Migrated from github.com) commented 2023-08-26 03:25:56 -05:00

so we always open all report handles
lets havea a README.md under netbsd/ and describe important details like that

so we always open all report handles lets havea a `README.md` under `netbsd/` and describe important details like that
ghost commented 2023-08-26 22:37:42 -05:00 (Migrated from github.com)

Re-based on latest master branch.

Re-based on latest `master` branch.
Ghost reviewed 2023-08-26 22:43:56 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-26 22:43:56 -05:00

As you wish. This is just the result of me trying to order struct fields in terms of size to reduce the need for compiler added padding for data alignment.

As you wish. This is just the result of me trying to order struct fields in terms of size to reduce the need for compiler added padding for data alignment.
Ghost reviewed 2023-08-26 23:09:32 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-26 23:09:32 -05:00

These are tied to an implementation defined size. I'll find a suitable macro they already defined to use.

These are tied to an implementation defined size. I'll find a suitable macro they already defined to use.
Ghost reviewed 2023-08-26 23:12:11 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-26 23:12:11 -05:00

It is written this way for a reason. If the caller doesn't provide enough space, NetBSD instead says how much it would have used. There's nothing to iterate over if this happens. The array would be full of garbage data.

It is written this way for a reason. If the caller doesn't provide enough space, NetBSD instead says how much it would have used. There's nothing to iterate over if this happens. The array would be full of garbage data.
Ghost reviewed 2023-08-26 23:18:16 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-26 23:18:16 -05:00

The reason it exists is to be a failsafe if my assumptions somehow end up being wrong on some NetBSD system that has a very large number of child devices on a given parent device.

The reason it exists is to be a failsafe if my assumptions somehow end up being wrong on some NetBSD system that has a very large number of child devices on a given parent device.
Ghost reviewed 2023-08-26 23:19:51 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-26 23:19:51 -05:00

I can do this. But seeing as this is related to the other comment, what do you prefer? I was using stack-based fixed size allocations to not have to deal with dynamic memory within a recursive function. This is faster overall than the other approach I could use to retrieve the size before doing the actual data request. I figured that it is very unlikely that any given device node will have more than 256 child devices. This really is about how many child nodes you can expect to encounter in any given parent node in the NetBSD device tree.

I can do this. But seeing as this is related to the other comment, what do you prefer? I was using stack-based fixed size allocations to not have to deal with dynamic memory within a recursive function. This is faster overall than the other approach I could use to retrieve the size before doing the actual data request. I figured that it is very unlikely that any given device node will have more than 256 child devices. This really is about how many child nodes you can expect to encounter in any given parent node in the NetBSD device tree.
Ghost reviewed 2023-08-26 23:25:10 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-26 23:25:10 -05:00

It is identical to the others. I am reusing the device tree walker function because the system call approach is the same, it just starts with a specific parent device instead of the empty string (which is the root of the device tree here).

It is identical to the others. I am reusing the device tree walker function because the system call approach is the same, it just starts with a specific parent device instead of the empty string (which is the root of the device tree here).
Ghost reviewed 2023-08-27 00:15:06 -05:00
netbsd/hid.c
@ -0,0 +772,4 @@
hid_free_enumeration(devs);
if (*path == '\0') {
register_global_error("Device with requested VID/PID/(SerialNumber) not found");
Ghost commented 2023-08-27 00:15:06 -05:00

Resolved by clearing global error before hid_open_path returns success. All other exit points set a new error.

Resolved by clearing global error before `hid_open_path` returns success. All other exit points set a new error.
Ghost reviewed 2023-08-27 00:15:29 -05:00
netbsd/hid.c
@ -0,0 +779,4 @@
return hid_open_path(path);
}
HID_API_EXPORT hid_device * HID_API_CALL hid_open_path(const char *path)
Ghost commented 2023-08-27 00:15:29 -05:00

Resolved by clearing global error before hid_open_path returns success. All other exit points set a new error.

Resolved by clearing global error before `hid_open_path` returns success. All other exit points set a new error.
Ghost reviewed 2023-08-27 00:47:49 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Ghost commented 2023-08-27 00:47:49 -05:00

Apparently it isn't. I kept it originally because I was assuming it was required for the multi-byte functions to work correctly on utf8 input like it is with glibc.

Apparently it isn't. I kept it originally because I was assuming it was required for the multi-byte functions to work correctly on utf8 input like it is with `glibc`.
ghost commented 2023-08-27 00:51:00 -05:00 (Migrated from github.com)

Ok. That should handle the requested changes. Anything else?

Ok. That should handle the requested changes. Anything else?
Youw (Migrated from github.com) reviewed 2023-08-27 03:29:15 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Youw (Migrated from github.com) commented 2023-08-27 03:29:15 -05:00

I'm perfectly fine with pre-allocated stack memory
my only point is to have a single name/definition for the magic number

thanks

I'm perfectly fine with pre-allocated stack memory my only point is to have a single name/definition for the magic number thanks
Youw (Migrated from github.com) reviewed 2023-08-27 03:30:29 -05:00
netbsd/hid.c
@ -0,0 +1,1173 @@
/*******************************************************
Youw (Migrated from github.com) commented 2023-08-27 03:30:29 -05:00

then lets have an explicit comment about that
I might not be the only one who may thing about an "optimisation" like that

then lets have an explicit comment about that I might not be the only one who may thing about an "optimisation" like that
Youw commented 2023-08-27 03:33:10 -05:00 (Migrated from github.com)

just one last comment

just one last comment
ghost commented 2023-08-27 03:35:33 -05:00 (Migrated from github.com)

There. That comment should cover it.

There. That comment should cover it.
Youw (Migrated from github.com) approved these changes 2023-08-27 03:47:03 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
Youw
Labels
Clear labels   
3rdparty
3rd-party related issue/question

  
Android
Android OS related

  
API
API change, Version 1 stuff

  
Author does not respond

   
bsd
FreeBSD, NetBSD, OpenBSD, etc

  
bug
Something isn't working

  
build system/CI
Anything related to building the project or running on CI

  
ci-virtual-device
Run the virtual HID device CI jobs (Windows driver + libusb raw-gadget)

  
CMake
Related to CMake build system

  
Contributions Welcome
Any external help is welcome

  
Core
Related to common codes like hidapi.h

  
dependencies
Pull requests that update a dependency file

  
device-specific
Issues/PRs related to a specific device

  
documentation
Improvements or additions to documentation

  
don't_merge
Don't merge this PR as is

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
Example
Related to hidtest or new examples

  
Haiku
Related to Haiku OS (using libusb backend)

  
hidraw
Related to Linux/hidraw backend

  
hotplug
Related to hotplug

  
info required
Need more input from PR/issue author

  
invalid
Not a HIDAPI issue

  
libusb
Related to libusb backend

  
macOS
Related to macOS backend

  
Misc
Typo, whitespace, etc.

  
Not an issue

   
question
Information is requested

  
Release
Related to new release

  
Test
Related to built-in test (not hidtest)

  
Test GUI
Related to a Test GUI application

  
Windows
Related to Windows backend

  
wontfix
This will not be worked on

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
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libusb/hidapi!612
No description provided.