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

Closed

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 Show resolved Hide resolved
libusb/hid.c Show resolved Hide resolved
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.

Copy link
Member

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
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.

Copy link
Member

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
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.
@Youw
Copy link
Member

Youw commented Sep 26, 2024

Ideally there would be an hid_libusb_error function

Why not - we have all means for it.

@Youw
Copy link
Member

Youw commented Sep 26, 2024

I added the one I needed. More can be added in future contributions.

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.

@Youw Youw changed the base branch from master to libusb-error September 26, 2024 11:32
@@ -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)
Copy link
Member

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

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

Comment on lines +417 to +423
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);
}
Copy link
Member

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

Comment on lines +1675 to +1691
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;
}
Copy link
Member

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

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

@Youw
Copy link
Member

Youw commented Sep 27, 2024

I've updated the implementation myself: #698
I don't have a good environment right now to do extensive testing, @matheusmoreira would be great if you could check it.

@matheusmoreira
Copy link
Author

Ideally there would be an hid_libusb_error function

Why not - we have all means for it.

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.

Youw added a commit that referenced this pull request Oct 2, 2024
- 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]>
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