-
Notifications
You must be signed in to change notification settings - Fork 392
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
base: master
Are you sure you want to change the base?
Implement hid_error
#690
Conversation
libusb/hid.c
Outdated
@@ -123,6 +123,10 @@ struct hid_device_ { | |||
#ifdef DETACH_KERNEL_DRIVER | |||
int is_driver_detached; | |||
#endif | |||
|
|||
int error; | |||
wchar_t *error_name; |
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.
the way it is used - error_name
is useless
it would be much more useful to have a single error_string
formatted as <libusb_error_name>: <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.
That's a fair point. This variable is redundant due to the stored error code. I think I just forgot to delete it.
@@ -123,6 +123,10 @@ struct hid_device_ { | |||
#ifdef DETACH_KERNEL_DRIVER | |||
int is_driver_detached; | |||
#endif | |||
|
|||
int error; |
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.
I don't think we need to have it as a member here at all
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.
Not storing the error code would discard the error information returned by libusb.
In my case, value of this int
is all I wanted. Ideally there would be an hid_libusb_error
function.
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.
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.
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
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