Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

matheusmoreira
Copy link

The hid_error function is not implemented yet, and contributions are welcome. So add error data to the device structure and return it from hid_error. Also set error data in the functions I need.

Closes: #684

@mcuee mcuee added the libusb Related to libusb backend label Sep 1, 2024
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;
Copy link
Member

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>

Copy link
Author

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;
Copy link
Member

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

Copy link
Author

@matheusmoreira matheusmoreira Sep 6, 2024

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
Comment on lines 1673 to 1674
case LIBUSB_ERROR_TIMEOUT:
custom_string = L"Transfer timed out";
Copy link
Member

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

Copy link
Author

@matheusmoreira matheusmoreira Sep 6, 2024

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
Comment on lines 1676 to 1677
case LIBUSB_ERROR_PIPE:
custom_string = L"Control request not supported by device";
Copy link
Member

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

Copy link
Author

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
Comment on lines 1682 to 1683
case LIBUSB_ERROR_BUSY:
custom_string = L"Called from event handling context";
Copy link
Member

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

Copy link
Author

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
Comment on lines 1685 to 1686
case LIBUSB_ERROR_INVALID_PARAM:
custom_string = L"Transfer size larger than supported";
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

@Youw Youw left a 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

@matheusmoreira
Copy link
Author

It implements a general stored error mechanism for hid_error. There's even an internal API that makes it easy to use.

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.

@matheusmoreira
Copy link
Author

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libusb Related to libusb backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libusb backend: "hid_error is not implemented yet"
3 participants