Update HIDAPI to match upstream hidapi 0.14.0

[slouken]

Fixes #7727

[slouken]

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

[flibitijibibo]

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

[sezero]

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]

* 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]

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]

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]

@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?

[sezero]

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]

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.

[sezero]

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]

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

[slouken]

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]

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]

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]

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]

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]

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

[cgutman]

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]

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?