Skip to content

Update HIDAPI to match upstream hidapi 0.14.0 #7736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 36 commits into from
May 26, 2023

Conversation

slouken
Copy link
Collaborator

@slouken slouken commented May 24, 2023

Fixes #7727

@slouken slouken force-pushed the hidapi-update branch 5 times, most recently from 535d740 to 73fea71 Compare May 24, 2023 23:07
@slouken slouken force-pushed the hidapi-update branch 2 times, most recently from c7c1689 to f4ab960 Compare May 25, 2023 05:51
@slouken
Copy link
Collaborator Author

slouken commented May 25, 2023

@flibitijibibo, how important is having libusb/hid.c compile on Windows?

@flibitijibibo
Copy link
Collaborator

It would definitely be a nice-to-have but right now I'm only enabling it on Linux.

@slouken slouken force-pushed the hidapi-update branch 3 times, most recently from 698e430 to d6efd85 Compare May 25, 2023 21:44
@slouken slouken force-pushed the hidapi-update branch 4 times, most recently from daa2c31 to 18ae036 Compare May 26, 2023 00:21
@sezero
Copy link
Contributor

sezero commented May 26, 2023

Some windows notes: (Disclaimer: I tested build using an old mingw toolchain.)

  • I don't understand the need for ntdef.h include: it should only be included
    with ddk builds, and all the necessary types are defined by way of windows.h
    include. (If memory isn't faulting me, ntdef.h and windows.h aren't actually
    meant to be included together.) With my toolchain, that results in lots of
    type redefinition errors. One thing that ntdef.h does define and windows.h
    does not is NTSTATUS type, and that one is actually typedef'ed in hid.c
    before ntdef.h is included. That include should go.

  • I understand the explicit winbase.h include even less: it is included by
    way of windows.h already. That include should go, too.

  • hidapi_hidpi.h and hidapi_hidsdi.h both typedef PHIDP_PREPARSED_DATA.
    hidapi_hidsdi.h already includes hidapi_hidpi.h, and that results in a
    type redefinition error: the duplicate should go.

  • PZZWSTR may be absent in old SDKs: should be replaced with WCHAR*

  • WC_ERR_INVALID_CHARS and _WIN32_WINNT_WIN8 may be absent in old SDKs:
    should defined, if not already.

  • windows/hid.c may use HIDAPI_USING_SDL_RUNTIME: that way, stdio.h, stdlib.h
    and string.h need not be included (might even result in clashes.)

After manually adding -std=gnu99 to CFLAGS, I was able to build SDL_hidapi.c
with the following patch applied.

diff --git a/src/hidapi/SDL_hidapi_windows.h b/src/hidapi/SDL_hidapi_windows.h
index da61fa2..fba405c 100644
--- a/src/hidapi/SDL_hidapi_windows.h
+++ b/src/hidapi/SDL_hidapi_windows.h
@@ -22,6 +22,7 @@
 #include <stdio.h>	/* Include early so we can redefine snprintf */
 
 /* Define standard library functions in terms of SDL */
+#define HIDAPI_USING_SDL_RUNTIME
 #define calloc      SDL_calloc
 #define free        SDL_free
 #define malloc      SDL_malloc
diff --git a/src/hidapi/windows/hid.c b/src/hidapi/windows/hid.c
index 99712ad..7610376 100644
--- a/src/hidapi/windows/hid.c
+++ b/src/hidapi/windows/hid.c
@@ -38,11 +38,12 @@ extern "C" {
 typedef LONG NTSTATUS;
 #endif
 
-#ifdef __MINGW32__
-#include <ntdef.h>
-#include <winbase.h>
+#ifndef WC_ERR_INVALID_CHARS
 #define WC_ERR_INVALID_CHARS 0x00000080
 #endif
+#ifndef _WIN32_WINNT_WIN8
+#define _WIN32_WINNT_WIN8 0x0602
+#endif
 
 #ifdef __CYGWIN__
 #include <ntdef.h>
@@ -56,9 +57,11 @@ typedef LONG NTSTATUS;
 #include "hidapi_hidclass.h"
 #include "hidapi_hidsdi.h"
 
+#ifndef HIDAPI_USING_SDL_RUNTIME
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#endif
 
 #ifdef MIN
 #undef MIN
@@ -117,6 +120,11 @@ static void free_library_handles()
 	cfgmgr32_lib_handle = NULL;
 }
 
+#if defined(__GNUC__)
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wcast-function-type"
+#endif
+
 static int lookup_functions()
 {
 	hid_lib_handle = LoadLibraryW(L"hid.dll");
@@ -129,10 +137,6 @@ static int lookup_functions()
 		goto err;
 	}
 
-#if defined(__GNUC__)
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Wcast-function-type"
-#endif
 #define RESOLVE(lib_handle, x) x = (x##_)GetProcAddress(lib_handle, #x); if (!x) goto err;
 
 	RESOLVE(hid_lib_handle, HidD_GetHidGuid);
@@ -158,9 +162,6 @@ static int lookup_functions()
 	RESOLVE(cfgmgr32_lib_handle, CM_Get_Device_Interface_ListW);
 
 #undef RESOLVE
-#if defined(__GNUC__)
-# pragma GCC diagnostic pop
-#endif
 
 	return 0;
 
@@ -169,6 +170,10 @@ err:
 	return -1;
 }
 
+#if defined(__GNUC__)
+# pragma GCC diagnostic pop
+#endif
+
 #endif /* HIDAPI_USE_DDK */
 
 struct hid_device_ {
diff --git a/src/hidapi/windows/hidapi_cfgmgr32.h b/src/hidapi/windows/hidapi_cfgmgr32.h
index 4685b76..09bf4d7 100644
--- a/src/hidapi/windows/hidapi_cfgmgr32.h
+++ b/src/hidapi/windows/hidapi_cfgmgr32.h
@@ -66,7 +66,7 @@ typedef CONFIGRET(__stdcall* CM_Get_Parent_)(PDEVINST pdnDevInst, DEVINST dnDevI
 typedef CONFIGRET(__stdcall* CM_Get_DevNode_PropertyW_)(DEVINST dnDevInst, CONST DEVPROPKEY* PropertyKey, DEVPROPTYPE* PropertyType, PBYTE PropertyBuffer, PULONG PropertyBufferSize, ULONG ulFlags);
 typedef CONFIGRET(__stdcall* CM_Get_Device_Interface_PropertyW_)(LPCWSTR pszDeviceInterface, CONST DEVPROPKEY* PropertyKey, DEVPROPTYPE* PropertyType, PBYTE PropertyBuffer, PULONG PropertyBufferSize, ULONG ulFlags);
 typedef CONFIGRET(__stdcall* CM_Get_Device_Interface_List_SizeW_)(PULONG pulLen, LPGUID InterfaceClassGuid, DEVINSTID_W pDeviceID, ULONG ulFlags);
-typedef CONFIGRET(__stdcall* CM_Get_Device_Interface_ListW_)(LPGUID InterfaceClassGuid, DEVINSTID_W pDeviceID, PZZWSTR Buffer, ULONG BufferLen, ULONG ulFlags);
+typedef CONFIGRET(__stdcall* CM_Get_Device_Interface_ListW_)(LPGUID InterfaceClassGuid, DEVINSTID_W pDeviceID, WCHAR* /*PZZWSTR*/ Buffer, ULONG BufferLen, ULONG ulFlags);
 
 // from devpkey.h
 static DEVPROPKEY DEVPKEY_NAME = { { 0xb725f130, 0x47ef, 0x101a, {0xa5, 0xf1, 0x02, 0x60, 0x8c, 0x9e, 0xeb, 0xac} }, 10 }; // DEVPROP_TYPE_STRING
diff --git a/src/hidapi/windows/hidapi_hidsdi.h b/src/hidapi/windows/hidapi_hidsdi.h
index a4bac4b..95e2fcb 100644
--- a/src/hidapi/windows/hidapi_hidsdi.h
+++ b/src/hidapi/windows/hidapi_hidsdi.h
@@ -40,8 +40,6 @@ typedef struct _HIDD_ATTRIBUTES{
 	USHORT VersionNumber;
 } HIDD_ATTRIBUTES, *PHIDD_ATTRIBUTES;
 
-typedef struct _HIDP_PREPARSED_DATA * PHIDP_PREPARSED_DATA;
-
 typedef void (__stdcall *HidD_GetHidGuid_)(LPGUID hid_guid);
 typedef BOOLEAN (__stdcall *HidD_GetAttributes_)(HANDLE device, PHIDD_ATTRIBUTES attrib);
 typedef BOOLEAN (__stdcall *HidD_GetSerialNumberString_)(HANDLE device, PVOID buffer, ULONG buffer_len);

@sezero
Copy link
Contributor

sezero commented May 26, 2023

* windows/hid.c may use HIDAPI_USING_SDL_RUNTIME: that way, stdio.h, stdlib.h
  and string.h need not be included (might even result in clashes.)

Also: If you accept doing HIDAPI_USING_SDL_RUNTIME for windows,
SDL_hidapi_windows.h shouldn't need to include stdio.h in order to
be able to undefine/redefine snprintf, either. (Forgot removing
that include in the proposed patch above by oversight.)

@sezero
Copy link
Contributor

sezero commented May 26, 2023

After manually adding -std=gnu99 to CFLAGS, I was able to build [...]

Also, curious: does this compile using VS2010? I.e.: does VS2010 support for loop initial declarations?

@slouken
Copy link
Collaborator Author

slouken commented May 26, 2023

After manually adding -std=gnu99 to CFLAGS, I was able to build [...]

Also, curious: does this compile using VS2010? I.e.: does VS2010 support for loop initial declarations?

No, this change drops support for Visual Studio 2010.

Thanks for your other patches! I'll apply them and make the other change to remove stdio.h as well.

@slouken slouken requested review from cgutman and sezero May 26, 2023 03:42
@slouken
Copy link
Collaborator Author

slouken commented May 26, 2023

@cgutman, I carefully reviewed all the changes that we've made to see if they're needed on the new hidapi, but would you mind taking a glance and make sure everything you wanted is in?

slouken added 3 commits May 25, 2023 21:47
Touching HID devices with keyboard usages will trigger a keyboard capture
permission prompt on macOS 11+. See libsdl-org#4887

Like the IOKit joystick backend, we accept HID devices that have joystick,
gamepad, or multi-axis controller usages. We also allow the Valve VID for
the Steam Controller, just like the Windows HIDAPI implementation does.

Signed-off-by: Cameron Gutman <aicommander@gmail.com>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
@sezero
Copy link
Contributor

sezero commented May 26, 2023

Current windows build exports hidapi procedures. The following fixes that:

diff -u a/src/hidapi/SDL_hidapi.c b/src/hidapi/SDL_hidapi.c
--- a/src/hidapi/SDL_hidapi.c
+++ b/src/hidapi/SDL_hidapi.c
@@ -32,6 +32,7 @@
 #include "SDL_hidapi_c.h"
 
 /* Initial type declarations */
+#define HID_API_NO_EXPORT_DEFINE /* do not export hidapi procedures */
 #include "hidapi/hidapi.h"
 
 #ifndef SDL_HIDAPI_DISABLED

slouken added 7 commits May 25, 2023 22:36
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
…eadFile in hidapi not being canceled for all threads before device close

- hidapi already called CancelIo on hid_close but that only cancels pending IO for the current thread. Controller read/writes originate from multiple threads (serialized, but on a different thread nonetheless) but device destruction was always done on the main device thread which left any pending overlapped reads still running after hidapi's internal read buffer is deallocated leading to intermittent free list corruption.

Signed-off-by: Sam Lantinga <slouken@libsdl.org>
…out()

This is unsafe because the event is auto-reset, therefore the call to
WaitForSingleObject() resets the event which GetOverlappedResult() will
try to wait on.

Even though the overlapped operation is guaranteed to be completed at
the point we call GetOverlappedResult(), it will still wait on the event
handle for a short time to trigger the reset for auto-reset events. This
amounts to roughly a 100 ms sleep each time GetOverlappedResult() is called
for a completed I/O with a non-signalled event.

In the context of HIDAPI, this extra sleep means that callers that loop
on hid_read_timeout() with timeout=0 will loop forever, since the 100 ms
sleep each iteration ensures ReadFile() will always have new data.

Signed-off-by: Cameron Gutman <aicommander@gmail.com>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
…th EPIPE (e.g. the device stalled)

Signed-off-by: Sam Lantinga <slouken@libsdl.org>
Signed-off-by: Sam Lantinga <slouken@libsdl.org>
@slouken
Copy link
Collaborator Author

slouken commented May 26, 2023

It would definitely be a nice-to-have but right now I'm only enabling it on Linux.

I went ahead and fixed the compiling issues. They were entirely around the threading implementation, which was pretty easily abstracted.

@slouken slouken merged commit 381cb41 into libsdl-org:main May 26, 2023
@slouken slouken deleted the hidapi-update branch May 26, 2023 15:19
@sezero
Copy link
Contributor

sezero commented May 27, 2023

We have a new SDL2 incompatibility: struct SDL_hid_device_info has
got inserted a new bus_type member. The structure is public, at it
is a parameter to SDL_hid_enumerate and SDL_hid_free_enumeration.
sdl2-compat still wrongly does passthrough for those: Needs taking
care of.

@sezero
Copy link
Contributor

sezero commented May 27, 2023

The FreeBSD VM in github CI has been failing since this PR. I don't know how this PR ever effected that...

@slouken
Copy link
Collaborator Author

slouken commented May 28, 2023

I'm not sure. It looks like it's failing due to an older version of libusb, but I thought I disabled libusb support in the build in 6b8b9af. @madebr, do you know what's happening?

@madebr
Copy link
Contributor

madebr commented May 28, 2023

From what I understand, the BSD's only have libusb to do the low-level hidapi things:

SDL/CMakeLists.txt

Lines 175 to 181 in e4eab18

# On the other hand, *BSD specifically uses libusb only, so we make a special
# case just for them.
if(FREEBSD OR NETBSD OR OPENBSD OR BSDI)
set(HIDAPI_ONLY_LIBUSB TRUE)
else()
set(HIDAPI_ONLY_LIBUSB FALSE)
endif()

In that case, SDL_HIDAPI_LIBUSB is overriden:

SDL/CMakeLists.txt

Lines 469 to 475 in e4eab18

if(SDL_HIDAPI)
if(HIDAPI_ONLY_LIBUSB)
set(SDL_HIDAPI_LIBUSB ON CACHE BOOL "" FORCE)
elseif(HIDAPI_SKIP_LIBUSB)
set(SDL_HIDAPI_LIBUSB OFF CACHE BOOL "" FORCE)
endif()
endif()

Is this assumption still correct?
If so, I'll remove this force, and disable hidapi completely.

@slouken
Copy link
Collaborator Author

slouken commented May 28, 2023

I think FreeBSD also supports hidraw, and I'm guessing the newer FreeBSD versions have a usable libusb. Can you disable the override for now, and I'll investigate when I get a chance?

@JoergAtGithub
Copy link

FreeBSD doesn't contain hidraw yet. But there are discussions with the FreeBSD maintainers to adapt it to their kernel. Currently all BSDs require the libusb backend.
The extension of the hid_device_info struct should be backward compatible, as long as your code just reads from it. This structure is only allocated and filled inside hid_enumerate and will be freed in hid_free_enumeration.

@sezero
Copy link
Contributor

sezero commented May 30, 2023

The linux driver (hidapi/linux/hid.c) seems to require kernel >= 2.6.39
unless HIDAPI_ALLOW_BUILD_WORKAROUND_KERNEL_2_6_39 is defined. Do we
want to enable that? (I am on CentOS 6.10 - which is EOL, yes - which has
kernel 2.6.32, so I can't build the source unless I manually enable it.
And, yes I know, I am a dinosaur :)

@slouken
Copy link
Collaborator Author

slouken commented May 30, 2023

Yeah, that seems fine. I went ahead and added it to SDL_hidapi_linux.h

@cgutman
Copy link
Collaborator

cgutman commented May 31, 2023

The changes generally look good to me.

My only concern is if we still need f21e172. It looks like we're back to using IOHIDDeviceRegisterRemovalCallback() now.

@slouken
Copy link
Collaborator Author

slouken commented May 31, 2023

Yeah, I was wondering about that, but I was getting called back with a closed handle in our version of hidapi, and not with 0.14.0. I suspect the upstream code is properly cleaning up and our issue was self-inflicted. I figured we could leave it out and see whether we actually need it. Are you able to reproduce the original issue with the new update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hidapi: commits from mainstream for mac and windows
6 participants