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

HID driver release 1.0.3 #49

Merged
merged 1 commit into from
Aug 20, 2024
Merged

HID driver release 1.0.3 #49

merged 1 commit into from
Aug 20, 2024

Conversation

roma-jam
Copy link
Collaborator

USB Host HID Class Driver v1.0.3 (Release)

General description

  • Initial support for external Hubs

Changes

  1. Fixed a bug with interface mismatch on EP IN transfer complete while several HID devices are present.
  2. Fixed a bug during device freeing, while detaching one of several attached HID devices.

Details

  1. Wrong interface has been selected by EP number, while handling transfer complete callback for EP IN of HID Device. Fully removed logic of getting interface by EP number, instead keep the interface pointer in xfer->context field.
  2. Wrong logic of releasing and freeing HID device object during device detaching. When several device were attached, the STAILQ was never empty, so the driver hanged infinitely.

Copy link
Collaborator

@leeebo leeebo 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 LGTM!

@tore-espressif
Copy link
Collaborator

tore-espressif commented Jul 31, 2024

@roma-jam Could we also include this little change? espressif/idf-extra-components#273

We must add a comment that not all mice report wheel status and the user is responsible for checking the report length before he access the scrollwheel field

@roma-jam
Copy link
Collaborator Author

@tore-espressif
Just to point out again, that it will be against the specs, because this field is optional:
image

I will add this byte and remove the strict comparison by the length for boot report, because some devices could return more.

Anyway, it is time to move to the full reports, not only boot (with ReportID field), because boot report isn't so interesting, as all the game pads and other cool things is a bit complicated than just keyboards and mice for boot mode.
If anything, I will address this later.

@roma-jam
Copy link
Collaborator Author

roma-jam commented Jul 31, 2024

@tore-espressif I have tried to add that, but it breaks the example, because not all mice has these byte in boot mode (Mine doesn't).
I would prefer to keep these changes till the moment we will update the driver, because there are couple of things in pending:

  1. HID Host example doesn't work with HID device example (for this, we need implement not only the boot reports support)
  2. Scroll wheel support. This is connected to the point 1, because it will automatically be resolved.
  3. There are couple of requests (such as feature(usb_host_hid): interrupt endpoint functions (send_report, receive_report) missing (IEC-56) #53 and ) for using EP OUT.

Maybe, I can combine them all-in-one future feature release and keep current only with fixes for external Hubs.

WDYT?

@tore-espressif
Copy link
Collaborator

@tore-espressif I have tried to add that, but it breaks the example, because not all mice has these byte in boot mode (Mine doesn't). I would prefer to keep these changes till the moment we will update the driver, because there are couple of things in pending:

1. HID Host example doesn't work with HID device example (for this, we need implement not only the boot reports support)

2. Scroll wheel support. This is connected to the point 1, because it will automatically be resolved.

3. There are couple of requests (such as [feature(usb_host_hid): interrupt endpoint functions (send_report, receive_report) missing (IEC-56) #53](https://github.com/espressif/esp-usb/issues/53) and ) for using EP OUT.

Maybe, I can combine them all-in-one future feature release and keep current only with fixes for external Hubs.

WDYT?

Yes, sure. Let;s leave it for later. It seemed as an easy change...

Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried on HW with an external hub. LGTM.

@tore-espressif
Copy link
Collaborator

@roma-jam can we merge this?

@roma-jam roma-jam force-pushed the fix/hid_host_hub_support branch 3 times, most recently from 8b9edcf to 1b4e4ac Compare August 14, 2024 12:43
@roma-jam roma-jam merged commit 39b0de3 into master Aug 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants