-
Notifications
You must be signed in to change notification settings - Fork 397
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
Implement hid_error
#690
Implement hid_error
#690
Conversation
libusb/hid.c
Outdated
case LIBUSB_ERROR_TIMEOUT: | ||
custom_string = L"Transfer timed out"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need a custom string here?
libusb has its own error string for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because libusb specifies context-specific error messages in the documentation. "Control request not supported" instead of "broken pipe", for example. This context is only available at the call sites of each function, so only they can pass these contextual messages along.
libusb/hid.c
Outdated
case LIBUSB_ERROR_PIPE: | ||
custom_string = L"Control request not supported by device"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not nesessary true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is what the libusb documentation says though.
Returns... LIBUSB_ERROR_PIPE if the control request was not supported by the device
If it's not true, then the libusb documentation should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERROR_PIPE
might be returned in some other cases, not only when the device gracefully processed the request and sent a response that it is not supported. It is hard to document all corner cases, specially when some of them device-specific, etc.
libusb/hid.c
Outdated
case LIBUSB_ERROR_BUSY: | ||
custom_string = L"Called from event handling context"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not nesessary true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is what the libusb documentation says though.
libusb_control_transfer()
Returns... LIBUSB_ERROR_BUSY if called from event handling context
If it's not true, then the libusb documentation should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My oppinion on this is that best to return LIBUSB_ERROR_BUSY
as is, and leave the interpretation/digging documentation to the user of HIDAPI, and not try to hard-code things that might be changed in the future (e.g. updated documentation for libusb, etc.).
libusb/hid.c
Outdated
case LIBUSB_ERROR_INVALID_PARAM: | ||
custom_string = L"Transfer size larger than supported"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not nesessary true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is what the libusb documentation says though.
libusb_control_transfer()
Returns... LIBUSB_ERROR_INVALID_PARAM if the transfer size is larger than the operating system and/or hardware can support (see Transfer length limitations)
If it's not true, then the libusb documentation should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a full implementation
it should be generic and covered by all functions
It implements a general stored error mechanism for All that remains is setting the error data in the appropriate functions. I added the one I needed. More can be added in future contributions. |
011c394
to
0c7f1ea
Compare
Fixed the issues pointed out in code review. Got rid of a lot of wide character stuff, much easier to just do everything needed with the utf8 data and convert just before returning the actual result. Improved the generated error message, now it contains the standard libusb error strings in addition to the context, if any. |
Most if not all libusb functions return UTF-8 encoded data but hidapi functions typically take and return wide character strings. Adapt one of the existing algorithms in the code base into a general conversion function.
Returns libusb error names and strings converted to wide character strings.
Store libusb error code so it can be retrieved later. Includes the original error code as well as a context-specific message which the libusb documentation sometimes specifies for each function. Code which uses those functions are meant to set the contextual message whenever possible. The code is initialized to a success state which implies no errors yet. The contextual error message is initialized to NULL and is not freed when the device is closed. It is meant to point at string literals.
Sets all error data, including an optional contextual error message which is supposed to be a non-freeable constant string such as a string literal. Contextual error messages are meant to be used in the cases the libusb documentation goes into detail as to what happened. Passing NULL will produce a message with just the libusb_error_name and the libusb_strerror results. Passing a string literal will produce a message that contains the additional context in addition to the error name and message.
Set error data when send_feature_report fails, including custom messages for the situations especially outlined in the libusb documentation for the libusb_control_transfer function.
Compute a formatted error string containing the libusb error name, the error message as well as any contextual information. Return NULL if there are no errors or if memory allocation failed.
0c7f1ea
to
9b6bef5
Compare
Why not - we have all means for it. |
In that case I'd accept it into the future branch for now. Having small improvements/changes/fixes for a future branch might speedup the development/readiness for master branch. |
@@ -340,6 +345,88 @@ static int is_language_supported(libusb_device_handle *dev, uint16_t lang) | |||
return 0; | |||
} | |||
|
|||
static wchar_t *utf8_to_wchar(char *s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have a similar routine (using iconv) - would be great to have a common implementation if possible
/* we don't use iconv on Android, or when it is explicitly disabled */ | ||
#if defined(__ANDROID__) || defined(NO_ICONV) | ||
|
||
w = wcsdup(L"not implemented"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we need a fallback implementation anyway
static wchar_t *libusb_error_name_wchar(int error) { | ||
return libusb_error_wchar(error, libusb_error_name); | ||
} | ||
|
||
static wchar_t *libusb_strerror_wchar(int error) { | ||
return libusb_error_wchar(error, libusb_strerror); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't look like it is being used
switch (res) { | ||
case LIBUSB_ERROR_TIMEOUT: | ||
context = "Transfer timed out"; | ||
break; | ||
case LIBUSB_ERROR_PIPE: | ||
context = "Control request not supported by device"; | ||
break; | ||
case LIBUSB_ERROR_NO_DEVICE: | ||
context = "Device has disconnected"; | ||
break; | ||
case LIBUSB_ERROR_BUSY: | ||
context = "Called from event handling context"; | ||
break; | ||
case LIBUSB_ERROR_INVALID_PARAM: | ||
context = "Transfer size larger than supported"; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these are documented by libusb itself - even stronger reason not to have it here at all
|
||
free(buffer); | ||
|
||
return w; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this is a memory leak - by the documentation what is returned by hid_error - is owned by HIDAPI
I've updated the implementation myself: #698 |
Huh, I was trying to avoid adding new functions this whole time. If we can just add that function then most of this PR becomes unnecessary since its just juggling error strings. Looks like it's become a new pull request so I'll close this one. |
- hid_error is set correctly for most of the API functions (except hid_enumerate/hid_open/etc.); - refactored iconv routines - common code for utf16 and utf8 to wchar_t implementation; - `hid_libusb_error` to have libusb error code when possible; Closes: #690 Fixes: #684 Co-authored-by: Matheus Afonso Martins Moreira <[email protected]>
The
hid_error
function is not implemented yet, and contributions are welcome. So add error data to the device structure and return it fromhid_error
. Also set error data in the functions I need.Closes: #684