-
Notifications
You must be signed in to change notification settings - Fork 93
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
feature(usb_host_hid): USB Host HID (Human Interface Device) Driver v.2.0.0 #214
Conversation
Hey @Dazza0, I have prepared a document with architectural changes for the next version of USB Host HID Driver. USB_HID_Host_Driver_migration_guide_v0.0.1_draft.pdf Things, that I want to ask you:
Do not refer to the code changes, only to the document. Code is not ready (only the part with esp_event_loop + device attaching and removing). If there is anything you want to share/add/discuss - I would be happy to do it. Please tell me about that. Thanks. |
592e0fa
to
78243a7
Compare
After removing the OPEN event and made USB_HID_Host_Driver_migration_guide_v0.0.2_draft.pdf @tore-espressif I have changed the I also updated all the tests and update the concurrent test and now it launch on every interface connected to the ESP32-S2/3 Host. I am going to investigate how esp_hid works with BLE an BT HID devices and based on that implement a Keyboard/Mouse Driver (which would provide the possibility to automatically, based on the report descriptor data, handle the reports and send an event of keys pressed). Also, I think about making it possible to tie the VID/PID of the device to the specific Keyboard/Mouse driver (or just to use a common one) to be able to implement different HID device driver, even proprietary one. |
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.
@roma-jam The API LGTM overall. Could you provide some proof-of-concept integration with esp_hid
before we proceed with the review?
@@ -103,18 +120,10 @@ typedef struct { | |||
size_t task_priority; /**< Task priority of created background task */ | |||
size_t stack_size; /**< Stack size of created background task */ | |||
BaseType_t core_id; /**< Select core on which background task will run or tskNO_AFFINITY */ | |||
hid_host_driver_event_cb_t callback; /**< Callback invoked when HID driver event occurs. Must not be NULL. */ | |||
esp_event_handler_t callback; /**< Callback invoked when HID driver event occurs. Must not be NULL. */ |
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.
Please correct me if I'm wrong, but isn't the main idea of esp_event that we do not have callbacks from the driver?
We should only expose the esp_loop and its events and the the user can register handlers (callbacks) with esp_event API
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.
@tore-espressif ,
callback
here is the same as is used in the esp_hid_host example, esp_hid_host_main.c, line 32
With prototype: void callback(void *handler_args, esp_event_base_t base, int32_t id, void *event_data)
.
The events are the same, except the ESP_HIDH_OPEN_EVENT (about what we agreed before).
But probably, it can be eliminated during integration to the esp_hid....because the esp_hid already has the implementation.
Anyway, it can be easily done.
Do I need to add these changes to this MR also? (possibility to smoothly integration to the esp_hid)
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.
Do I need to add these changes to this MR also?
If it makes sense to you, then yes. I always thought that the v2 is supposed to be ready to be integrated with esp_hid
.
If you could provide an example of the integration with esp_hid
(in this MR, or in different repo), it would help the reviewers with better understanding of the new API design, as well as with easier testing with real HID devices
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.
@tore-espressif
Sure it make sense to me, in case I think that it is always easier to verify and review everything when it is ready.
I suppose that example with integration with esp_hid should be in esp-idf repo....
Or I can actually think about make it as a part of unit testing of the component.
But just to be sure - if I am going to add these changes in this PR, it will be even bigger then it is right now.
Yes, it is ready and I have covered all the same test cases (so it should not be worse than prev version) but anyway.
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.
Could you please rebase this PR on top of current HID 1.0.1~1?
EDIT: sorry, I forgot to do git fetch
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.
Anyway, it is a good idea to make a rebase 👍
…disconnect events
…string descriptors from the USB HID Device
…ctor control EP interaction
844edc4
to
0154503
Compare
Closed as not relevant any more. |
Checklist
url
field definedChange description