Stoccarda commented 11 months ago

I have been using hid.c in a version from before September 2019 for 32- and 64-bit Windows programs. Compiler is CLANG in C++ Builder 11.3 (and earlier) IDE. hid.c was compiled and embedded into the main program, no dll. The main program had in most cases a GUI.

It worked for me, except for rising an exception when closing the program, and hence closing the HID device.

When compiling the current 0.14.0 several errors / warning occurred:

in hidapi_descriptor_reconstruct.h:

if defined(MINGW32) || defined(CYGWIN)

...

else

union {
    hid_pp_cap caps[];
    hid_pp_link_collection_node LinkCollectionArray[];
};

endif

[bcc32c error] hidapi_descriptor_reconstruct.h(220): type name requires a specifier or qualifier [bcc32c error] hidapi_descriptor_reconstruct.h(220): expected ';' at end of declaration list

When extending the first line to

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

it works. It also works with BORLANDC .

Compiler warning "[bcc64 Warnung] hidapi_hidsdi.h(43): redefinition of typedef 'PHIDP_PREPARSED_DATA' is a C11 feature"

typedef struct _HIDP_PREPARSED_DATA * PHIDP_PREPARSED_DATA;

was previously defined in hidapi_hidpi.h(39) Won't be the typedef in hidapi_hidpi.h(39) sufficient?

Second warning from hid.c in line 267: int printf_written = swprintf(msg, msg_len + 1, L"%.ls: (0x%08X) %.ls", (int)op_len, op, error_code, (int)system_err_len, system_err_buf);

[bcc64 Warnung] hid.c(267): incompatible integer to pointer conversion passing 'unsigned long' to parameter of type 'const wchar_t ' (aka 'const unsigned short ')

There is a linker error with the 32-bit compiler version: [ilink32 Fehler] Fatal: Invalid VIRDEF Fixup-Index in Modul 'hid.c'

It works when creating a 64-bit version.

When running a program

res = hid_get_input_report_length(handle);
ShowMessage((String)"input_report_length: " + res);

shows a length of zero.

res = hid_get_output_report_length(handle);
ShowMessage((String)"output_report_length: " + res);

returns the correct value.

The following code was used for it:

struct hiddevice { HANDLE device_handle; BOOL blocking; USHORT output_report_length; size_t input_report_length; void last_error_str; DWORD last_error_num; BOOL read_pending; char read_buf; OVERLAPPED ol; };

int HID_API_EXPORT_CALL HID_API_CALL hid_get_input_report_length(hid_device *dev) { return dev->input_report_length; }

int HID_API_EXPORT_CALL HID_API_CALL hid_get_output_report_length(hid_device *dev) { return dev->output_report_length; }

Sending data to a HID device seems to work.

The exception when closing the program still exists.

Any hints welcome!

Youw commented 11 months ago

I've never tried to compile HIDAPI with clang on Windows. PR with fixes are welcome. As for other issues - I have no specific siggestions, need to investigate carefully.

JoergAtGithub commented 11 months ago

When extending the first line to

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

it works. It also works with BORLANDC .

Which code path is used in case of BorlandC ? Is BorlandC using clang?

I think we should add clang-cl to our CI, than we should see such issues. Since is clang-cl.exe is command line compatible to the MSVC cl.exe, it should easy to set up.

Stoccarda commented 11 months ago

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

At first a correction: when copying the text, the underscores got lost due to wrong formatting. More correct:

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

The same for __BORLANDC__

From C++ Builder 10 Seattle on you had the choice between the classical Borland compiler and a CLANG based compiler. The 64-bit compiler (since XE4) was always Clang based. Current clang / LLVM version is 3.3 for both, 5.0 is also mentioned in the help. C++17 is supported.

JoergAtGithub commented 11 months ago

I think we need to invert the condition here, as the Microsoft compiler seems to be the exception and not the other way around. Let's use #ifndef _MSC_VER instead of #if defined(__MINGW32__) || defined(__CYGWIN__) || defined(__clang__) || defined(__BORLANDC__)

Stoccarda commented 11 months ago

@JoergAtGithub #ifndef _MSC_VER instead of #if defined(__MINGW32__) || defined(__CYGWIN__) works well and is surely a better solution than adding further defines.

In hidapi_hidsdi.h: typedef struct _HIDP_PREPARSED_DATA * PHIDP_PREPARSED_DATA; Can it be removed since the typedef is already in hidapi_hidpi.h which is included a few lines before?

In hid.c , line 267: int printf_written = swprintf(msg, msg_len + 1, L"%.*ls: (0x%08X) %.*ls", (int)op_len, op, error_code, (int)system_err_len, system_err_buf);

the compiler warns: _[bcc64 Warnung] hid.c(267): incompatible integer to pointer conversion passing 'unsigned long' to parameter of type 'const wchart ' (aka 'const unsigned short ')

If swprintf has a size (msglen + 1) as the second parameter, isn't it the C++ function? the C function is just 'int swprintf(wchar_t buffer, const wchar_t format[, argument, ...]);'_ without size Since there is

ifdef __cplusplus

extern "C" {

endif

in the beginning of the file, don't the functions need to be C and not C++ style? When compiling without the size, there is no warning.

Youw commented 11 months ago

the C function is just 'int swprintf(wchar_t buffer, const wchar_t format[, argument, ...]);' without size

Do you have a reference where you've got it from? I have found at least 3 different sources stating otherwise: https://linux.die.net/man/3/swprintf https://cplusplus.com/reference/cwchar/swprintf/ https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/sprintf-sprintf-l-swprintf-swprintf-l-swprintf-l

JoergAtGithub commented 11 months ago

In hidapi_hidsdi.h: typedef struct _HIDP_PREPARSED_DATA * PHIDP_PREPARSED_DATA; Can it be removed since the typedef is already in hidapi_hidpi.h which is included a few lines before?

I searched for this, and it seems that the original hidsdi.h from Microsoft doesn't contain this definition, only hidpi.h does. Therefore it makes sense to remove it from hidapi_hidsdi.h.

Youw commented 11 months ago

Closed by automation. @Stoccarda please confirm with latest master (and reopen is the issue(s) still there). Thanks.

JoergAtGithub commented 11 months ago

My PR #634 fixed only the first two problems.

Stoccarda commented 11 months ago

@Youw I found the different definition there: https://docwiki.embarcadero.com/RADStudio/Alexandria/en/Sprintf%2C_swprintf

I once installed CodeLite and found both declaration:

swprintf-CodeLite

There was also a similar discussion a few years ago: https://stackoverflow.com/questions/35072373/swprintf-declaration-mismatch

Youw commented 11 months ago

This answer: https://stackoverflow.com/a/35072422/3697939 summarizes my oppinion on this.

Stoccarda commented 11 months ago

@Youw I can corfirm that the error/warning related to the 2 include files is solved with the new ones

The confusion with swprintf and the 32-bit linker error [ilink32 Fehler] Fatal: Invalid VIRDEF Fixup-Index in Modul 'hid.c' still exists and the hid_get_input_report_length still returns 0 while input_report_length is the correct 65 in my program.

Stoccarda commented 11 months ago

The swprintf issue seems to be a compiler bug. It works for me with the temporary workaround

libusb / hidapi

hidapi 0.14.0 for Windows with C++-Builder 11 #630

if defined(MINGW32) || defined(CYGWIN)

else

endif

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

if defined(MINGW32) || defined(CYGWIN) || defined(clang)

ifdef __cplusplus

endif

include

include

include

if !defined(cplusplus) && defined(BORLANDC__)

undef swprintf

define swprintf snwprintf

endif