gh-libusb/hidapi

Watch

Fork 0

Add error reporting to the MacOS port. #314

Merged

hardkrash merged 3 commits from mac_error_messages into master

2022-10-14 05:25:43 -05:00

hardkrash commented

2021-07-31 15:17:06 -05:00

(Migrated from github.com)

Initial implementation of error messages for the macOS port.

Interface mostly copied from the linux port.
Messages probably need some love for the other errors being reported.

Initial implementation of error messages for the macOS port. Interface mostly copied from the linux port. Messages probably need some love for the other errors being reported.

hardkrash commented

2021-07-31 15:18:05 -05:00

(Migrated from github.com)

This is to resolve issue #310

Youw (Migrated from github.com) reviewed

2021-08-12 04:23:23 -05:00

mac/hid.c

this __thread actually bugs me very much:
we claim that it is thread safe, but imagine next scenario:

thread1: hid_init() - success
thread1: hid_enumerat() - error happened and registered
thread2: hid_enumerat() - error happened and registered
thread1: hid_exit() - last_global_error_str from thread1 is freed, but last_global_error_str from thread2 is leaked

I'm sure the very same issue is present with hidraw implementation of the global error

your intention with this PR is great, but I'd like to hear some ideas of how can we fix the global issues all together

this `__thread` actually bugs me very much: we claim that it is [thread safe](https://github.com/libusb/hidapi/blob/8741697359b8dd0d73702a69c54e64ffc552d3cf/hidapi/hidapi.h#L461), but imagine next scenario: thread1: `hid_init()` - success thread1: `hid_enumerat()` - error happened and registered thread2: `hid_enumerat()` - error happened and registered thread1: `hid_exit()` - `last_global_error_str` from thread1 is freed, but `last_global_error_str` from thread2 is leaked I'm sure the very same issue is present with `hidraw` implementation of the global error --- your intention with this PR is great, but I'd like to hear some ideas of how can we fix the global issues all together

hardkrash (Migrated from github.com) reviewed

2021-08-13 01:12:33 -05:00

mac/hid.c

I am not a threading expert, and only copied code from the other implementation from this code base.
Why is thread 2 allowed to run hid_enumerate() without calling hid_init()

How do we add register_global_error(NULL); to a pthread_cleanup_push?
But it looks like that's the solution. In thread we need the cleanup_pop to have ability to clear the hid thread error.

It also looks like I might have missed adding the freeing of the device pointers on close of device

I am not a threading expert, and only copied code from the other implementation from this code base. Why is thread 2 allowed to run `hid_enumerate()` without calling `hid_init()` How do we add `register_global_error(NULL);` to a `pthread_cleanup_push`? But it looks like that's the solution. In thread we need the cleanup_pop to have ability to clear the hid thread error. It also looks like I might have missed adding the freeing of the device pointers on close of device

Youw (Migrated from github.com) reviewed

2021-08-13 02:54:26 -05:00

mac/hid.c

Why is thread 2 allowed to run hid_enumerate() without calling hid_init()

That is the design of the API. hid_init() - is a global initialisation of the library.

How do we add register_global_error(NULL); to a pthread_cleanup_push?
But it looks like that's the solution. In thread we need the cleanup_pop to have ability to clear the hid thread error.

pthread_cleanup_push might not be enough, as it clears the resources at thread exit only, and we should come up with something at hid_exit().

I think we need to use pthread_key here, unless someone has other ideas.

It also looks like I might have missed adding the freeing of the device pointers on close of device

Self-code-review is always a good thing.

> Why is thread 2 allowed to run `hid_enumerate()` without calling `hid_init()` That is the design of the API. `hid_init()` - is a _global_ initialisation of the library. > How do we add `register_global_error(NULL)`; to a `pthread_cleanup_push`? But it looks like that's the solution. In thread we need the cleanup_pop to have ability to clear the hid thread error. `pthread_cleanup_push` might not be enough, as it clears the resources at thread exit only, and we should come up with something at `hid_exit()`. I think we need to use [`pthread_key`](https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html) here, unless someone has other ideas. > It also looks like I might have missed adding the freeing of the device pointers on close of device Self-code-review is always a good thing.

Youw (Migrated from github.com) reviewed

2022-06-12 16:57:04 -05:00

mac/hid.c

After going thru the git history again, and trying to find a good solution,
I think the best approach would be to discard the faulty behavior introduced not so long ago (https://github.com/libusb/hidapi/pull/23) and don't make any thread-safety assumptions for hid_error, i.e.: make last_global_error_str a simple static variable.

After going thru the git history again, and trying to find a good solution, I think the best approach would be to discard the faulty behavior introduced not so long ago (https://github.com/libusb/hidapi/pull/23) and don't make any thread-safety assumptions for `hid_error`, i.e.: make `last_global_error_str` a simple `static` variable.

Youw commented

2022-06-12 16:59:06 -05:00

(Migrated from github.com)

Hi @hardkrash,

Are you still up to finish this PR?
I've started some changes for hidraw backend: https://github.com/libusb/hidapi/pull/428,
those introduce a safe strategy for hid_error and allows to proceed with the implementation for other (this one) backends.

Hi @hardkrash, Are you still up to finish this PR? I've started some changes for `hidraw` backend: https://github.com/libusb/hidapi/pull/428, those introduce a safe strategy for `hid_error` and allows to proceed with the implementation for other (this one) backends.

mcuee commented

2022-08-16 04:53:36 -05:00

(Migrated from github.com)

#428 has been merged.

Youw (Migrated from github.com) approved these changes

2022-10-02 12:13:57 -05:00

Youw (Migrated from github.com) left a comment

I took the liberty of updating the implementation:

rebased to master/fixed conflicts;
updated error messages/error handling a bit;

I don't have a chance to test this well as of yet.
Additional core review is welcome.

I took the liberty of updating the implementation: - rebased to master/fixed conflicts; - updated error messages/error handling a bit; I don't have a chance to test this well as of yet. Additional core review is welcome.

Youw commented

2022-10-14 05:24:55 -05:00

(Migrated from github.com)