8000 Support HID devices with multiple report id's for one device · Issue #4868 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Support HID devices with multiple report id's for one device #4868

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.

Sign up for GitHub

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

Jump to bottom
Closed
6 tasks done
dhalbert opened this issue Jun 7, 2021 · 12 comments
Closed
6 tasks done

Support HID devices with multiple report id's for one device #4868

dhalbert opened this issue Jun 7, 2021 · 12 comments
Assignees
@dhalbert
Labels
enhancement usb
Milestone
7.0.0

Comments

@dhalbert
Copy link
Collaborator
dhalbert commented Jun 7, 2021 •
edited
Loading

The dynamic USB support in 7.0.0 alpha currently assumes that a single HID device (usb_hid.Device) has a single report ID. There are HID devices that need multiple report ID's, such as eye tracking devices.

Currently the API asks for the position of the report ID in the report descriptor, and renumbers the report ID's consecutively. This is unnecessary. The report ID's just need to be distinct in the different devices.

Plan (updated 2021-08-14):

  • Require that the report ID the user wants is in the report descriptor. It will not be changed.
  • Remove the Device() constructor argument that is the report ID index.
  • Add multiple IN and OUT report sizes for each of the report ID's the user supplies. The code will not parse the report descriptor to validate that the number of report ID's and report sizes matches.
  • Still handle the case that a single report ID for a lone device is removed if that is the only device in the HID interface. (see below *)
  • Make sure "feature" (bidirectional) reports are supported properly. Moved to Support "feature" reports in HID #5197.
  • Change the .last_received_report API for OUT reports to something like get_last_received_report(report_id). Maybe make this backward compatible for now.
  • Rework the IN buffer management to share a single buffer. This is probably possible, because the TinyUSB copies the report when it's passed to be sent. For OUT (received) reports, we probably still need a buffer per report, so we can support get_last_received_report().**

Tagging @ATMakersBill.

*The "requirement" that a single report ID for a lone device is removed if that is the only device in the HID interface does not seem to be true, at least for USB HID. @hathach believes this may be an idiosyncrasy of iOS BLE HID, and we should retest this. I tested a keyboard as the sole device in an interface, and gave it a non-zero report ID, and it worked fine on Windows 10, Linux (Ubuntu 20.04), iOS 14.7.1 and 15 beta, and Android 11.

**The IN buffer is not needed when sending reports directly but USB can send a "GET REPORT" request over the control interface, so I am leaving the IN buffers in place for that possibility.
@dhalbert dhalbert added this to the 7.0.0 milestone Jun 7, 2021
@ATMakersBill
Copy link
Collaborator

I'm happy to help test this. It's a blocking issue for our use with the Eye Tracker and Head Tracker HID specifications (which are new, but well supported by Windows 10). Here is a PDF of the specification:
https://usb.org/sites/default/files/hutrr74_-_usage_page_for_head_and_eye_trackers_0.pdf

Notice that there are several Report IDs in the descriptor at the bottom of the file.

Thanks Dan!

@dglaude
Copy link
dglaude commented Jun 8, 2021

I am interested in making sure it is still possible to do NUM/CAPS/SCROLL LOCk catching (that was using the .last_received_report AFAIK).

For me it can be a breaking change, no problem, but that was fun and useful side channel communication and permit nice project.

Just with all the keyboard craze in recent Adafruit product, you may want to make sure we don't lose it.

Very interested anyway in being able to pretend to be some devices, especially if it enable more AT use.

@dhalbert
Copy link
Collaborator Author
dhalbert commented Jun 9, 2021

I am interested in making sure it is still possible to do NUM/CAPS/SCROLL LOCk catching (that was using the .last_received_report AFAIK).

This will still be supported by the point mentioned above:

  • Change the .last_received_report API for OUT reports to something like get_last_received_report(report_id). Maybe make this backward compatible for now.

@ATMakersBill
Copy link
Collaborator

I was just thinking about this... on the OUT (or FEATURE) reports that are being sent from the Host to the CP device, what about providing an interface on the Device class that allows registering callbacks based on Report ID?

If the Device object receives an OUT or FEATURE report it would look at the registration, see if any callbacks were registered and call them. If not, it would drop the report on the floor. This would allow the application code (GamePad, HeadTracker, FancyKeyboard) to handle any buffering or caching of the report for later use. It would also likely lead to fewer dropped reports.

Just a thought
Bill

8000
@dhalbert
Copy link
Collaborator Author

Unfortunately, an asynchronous callback like that is not possible right now. It is sort of the the same reason we don't support interrupts. It could perhaps be done in some kind of event loop system like evento.

@dglaude
Copy link
dglaude commented Jun 10, 2021

I fully understand the problem of [not] having callback to save "event" taking place while the user code is doing something.

In the following PR #4877 you talk about gamepad that remember the event:

gamepad is different. It works in the background and remembers all the buttons that were ever pressed.

So maybe similar mechanism can be put in place to store what occurred and store reports (at least a few) for later processing.

Would that answer @ATMakersBill usecase and bypass the asynchronous callback issue?

@dhalbert dhalbert mentioned this issue Jun 21, 2021
Closed
@ThomasAtBBTF
Copy link

I closed #4905
Here the descriptor which I tried to load:
BRAILLE_HID.zip

@ThomasAtBBTF
Copy link

Is milestone 7.0.0 for this feature still estimated?

@dhalbert
Copy link
Collaborator Author
dhalbert commented Jul 4, 2021

I still plan to do this, but I have been busy with keypad and other things. I plan to get back to it.

@dhalbert dhalbert mentioned this issue Jul 13, 2021
Closed
@dhalbert dhalbert self-assigned this Jul 15, 2021
@dhalbert dhalbert mentioned this issue Aug 14, 2021
Merged
@dhalbert dhalbert mentioned this issue Aug 21, 2021
Closed
@dhalbert
Copy link
Collaborator Author
dhalbert commented Aug 21, 2021 •
edited
Loading

Mostly fixed by #5151. Feature reports enhancement moved to #5197.

@ThomasAtBBTF
Copy link
ThomasAtBBTF commented Sep 17, 2021 •
edited
Loading

I checked again for my BrailleHID descriptor and it still failes in
/lib/adafruit_ble/services/standard/hid.py
Where the code:

get_report_info(collection, reports)
if len(reports) > 1:
    raise NotImplementedError(
                    "Only one report id per Application collection supported"
                )

Still is in the latest libraries...

@dhalbert
Copy link
Collaborator Author
dhalbert commented Sep 17, 2021 •
edited
Loading

@ThomasAtBBTF I did not realize that check was in the BLE library. Did you just try removing it? I think it might just work without the check, but I'm not sure. In any case, please open an issue on https://github.com/adafruit/Adafruit_CircuitPython_BLE.

@ThomasAtBBTF ThomasAtBBTF mentioned this issue Sep 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dhalbert dhalbert

Labels
enhancem 4110 ent usb
Projects
None yet
Milestone
7.0.0
Development

No branches or pull requests
4 participants
@dhalbert @dglaude @ATMakersBill @ThomasAtBBTF
0