Skip to content

Fix #2348 support for CapsLock/NumLock LEDs in usb_hid #2366

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

Closed
wants to merge 4 commits into from
Closed

Fix #2348 support for CapsLock/NumLock LEDs in usb_hid #2366

wants to merge 4 commits into from

Conversation

deshipu
Copy link

@deshipu deshipu commented Dec 10, 2019

No description provided.

@deshipu
Copy link
Author

deshipu commented Dec 10, 2019

This is work in progress. Currently the report_type is always HID_REPORT_TYPE_INVALID for some reason, while it should be HID_REPORT_TYPE_OUTPUT. Possibly a bug or missing feature in the tinyusb library.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I don't think this is the right API because output reports can theoretically be used for other things besides LEDs. A better api would be a circular buffer of reports that can be read through a receive_report function.

@deshipu
Copy link
Author

deshipu commented Dec 10, 2019

I agree a more general API would be more useful, but I think it will require a considerable amount of planning. How large should the buffer be? How many entries of what size? Do we pre-allocate it always? Do we let the user choose the size that fits best their use case? How can the user make an informed decision about it?

I think that the particular combination of HID_USAGE_DESKTOP_KEYBOARD and HID_USAGE_PAGE_DESKTOP is only used for keyboard lamps, though, so we could leave it as a special case.

@tannewt
Copy link
Member

tannewt commented Dec 10, 2019

I'd rather not add a special API because I'm going to be doing the BLE HID library soon and it'll expose this same API.

We can limit the buffer a bunch for now though. How about an 8 byte buffer that only queues 1 byte reports and return 1 byte bytestrings? When we have other reports to support we'll just make the implementation smarter.

@deshipu
Copy link
Author

deshipu commented Dec 10, 2019

I like that. I could even live with just one one-byte report, because I only care about the most recent one. This would make the internals easy enough even for me to implement, and would have an API that could be expanded later when needed. Thanks!

@deshipu
Copy link
Author

deshipu commented Dec 28, 2019

I tried to do a ring buffer, but it's a bit out of my depth. For now I just made a simple 1-item buffer for a single-byte report, but I updated the Python API to be what was suggested. We can change the underlying implementation later on.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

This implementation is totally fine. Thanks for doing it! Just a couple suggestions but then it should be good to go.

@deshipu
Copy link
Author

deshipu commented Jan 3, 2020

I changed the interface to read_report_into, but for some reason it now stopped working. I need some more time to debug this.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks! Let me know when I should take another look.

@tannewt
Copy link
Member

tannewt commented Apr 29, 2020

@deshipu any object to me closing this until it is fixed up?

@deshipu
Copy link
Author

deshipu commented May 26, 2020

Sorry, I didn't have time to look into this properly, I will make a new pull request when I figure this out.

@deshipu deshipu closed this May 26, 2020
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.

2 participants