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

feature(usb_host_hid): USB Host HID (Human Interface Device) Driver v.2.0.0 #214

Closed
wants to merge 18 commits into from

Conversation

roma-jam
Copy link
Collaborator

@roma-jam roma-jam commented Jul 20, 2023

Checklist

  • Component contains License
  • Component contains README.md
  • Component contains idf_component.yml file with url field defined
  • Component was added to upload job
  • Component was added to build job
  • Optional: Component contains unit tests
  • CI passing

Change description

  • Changed the logic of user notification procedure (merge two callbacks in one, extend with new events, add esp_event_loop to simplify application events handling procedure and exclude external task to handle HID Driver events).
  • Optimized memory and pipes (in USB Host library) usage. Allocation memory and pipe for the HID Interface are done only by application layer request. Otherwise, no resources is used.
  • Simplified the internal logic of opening and closing procedure for HID Device.
  • Added API to work with OUT EP in case it is present in HID Interface.
  • Changed the way of device list handling. Now the device list is available only when HID device is opened by a client. That means that is there is no one device has been opened the driver can be uninstall easily, even if the device is still connected to the port.

@roma-jam
Copy link
Collaborator Author

Hey @Dazza0,
Hey @tore-espressif,

I have prepared a document with architectural changes for the next version of USB Host HID Driver.
I would like to ask you both to have a look at the document and share your opinion.

USB_HID_Host_Driver_migration_guide_v0.0.1_draft.pdf

Things, that I want to ask you:

  1. Check the architectural changes, make your point (agree/disagree/other). I do not want you to spend more than 10-15 minutes on that, so if you are not able to understand the main idea from the document quickly - please tell me about that. I will update the document and try to do it more concise. Every feedback I would appreciate.
  2. Verify the advantages/disadvantages paragraphs for both versions. I would like to know your opinion.

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.
Sorry for my concise tone (probably), I really don't want to spend your time a lot and I want the document to be short and clear.

Thanks.

@roma-jam
Copy link
Collaborator Author

After removing the OPEN event and made hid_host_device_open() synchronous new version and new scheme are required.
They can be found in the next draft description file:

USB_HID_Host_Driver_migration_guide_v0.0.2_draft.pdf

@tore-espressif I have changed the hid_host_open_device() logic and now you can verify the document again.
If there is anything else you want to change or make better, feel free to share.

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.

Copy link
Collaborator

@tore-espressif tore-espressif left a 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. */
Copy link
Collaborator

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

Copy link
Collaborator Author

@roma-jam roma-jam Dec 1, 2023

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)

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

@tore-espressif tore-espressif Dec 14, 2023

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

Copy link
Collaborator Author

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 👍

@roma-jam
Copy link
Collaborator Author

Closed as not relevant any more.
Should be moved to esp-usb.

@roma-jam roma-jam closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants