shared-module/usb_hid: Fix behavior of Device.get_last_received_report()

[maximkulkin]

Documentation states that get_last_received_report() function should
return None if there was no report received previously, otherwise it
should return report. Moreover, same report should be returned only
once. That makes it possible to reliably process incoming OUT/Feature
reports.

This patch adds an array that stores flags if report with particular
ID was received and updates get_last_received_report() to match its
documentation.

Fixes #6764

[Neradoc]

Hi, thank you for the PR. However I don't know if that is only a bug fix.

Moreover, same report should be returned only once.

I don't think that's the intention. The documentation implies that it always returns the last received report, not just once. Like a buffer that keeps the last value in it. It's supposed to return None only "if nothing received".

This implementation suits the current use in the default HID devices (caps-lock and num-lock LED's state can be read at any time and only the current state matters), and the expectations of the implementation in adafruit_hid. So this would actually be a breaking change (and not ported to 7.x).
Note that adafruit_hid also relies on the bug where last_received_report is never None, which, on the other hand, is not the intended behavior.

Discussion about the original implementation: #3302

Side note: the last_received_report property is scheduled to be removed in 8.0.0, removing it from shared-bindings might gain a very few bytes that can be reused here.

[maximkulkin]

I have done some research and some HID descriptors do actually use reports as events (with different values), so it is definitely a thing. Looks like indeed the get_last_received_report() method was introduced to replace last_received_report field which definitely had this non-consumable "just a buffer" functionality.
However, that approach severely limits flexibility of the whole API. How about introducing a new method like get_next_report() that would act as is implemented in this PR (there would still be a single buffer per report which will be overwritten by newly coming data, but that would be left as an implementation detail), but it will carry "consume" semantics. I also like receive_report() name for the method, but it feels that the later has a blocking flavor.

[maximkulkin]

Ok, I have changed my fix to implement functionality I'm proposing in a new method - get_next_received_report(): it matches get_last_received_report() but IMO caries this "consume" semantics for reports.

[dhalbert]

@Neradoc thanks for pointing out the intended semantics. Originally .last_received_report was used only for keyboard LED status, so it didn't need to be very fancy. When I added user-created HID report descriptors, I did add the capability for "feature" reports, which are bidirectional, as you (@maximkulkin) mention, but I didn't make a full-fledged report queue mechanism for OUT reports (reports coming from the host).

[tannewt]

What HID uses require the consume semantics? I'm interested in examples.

[maximkulkin]

@tannewt I'm pretty sure there are tons of them, but the one I have in mind is similar to LampMultiUpdateReport (see Section 25.11.1 at https://usb.org/sites/default/files/hut1_3_0.pdf). I am developing a custom HID for Adafruit RP2040 MacroPad that would allow me to control colors of keys RGB LEDs: each report is a command to set color of particular LED.

[tannewt]

@tannewt I'm pretty sure there are tons of them, but the one I have in mind is similar to LampMultiUpdateReport (see Section 25.11.1 at https://usb.org/sites/default/files/hut1_3_0.pdf). I am developing a custom HID for Adafruit RP2040 MacroPad that would allow me to control colors of keys RGB LEDs: each report is a command to set color of particular LED.

Very cool! Thanks for the link!

[dhalbert]

I have seen feature reports used in an eye tracker that is an HID device. It has many reports, and is how I picked six as the max number of reports.

[dhalbert]

I read over the previous PR's and issues around this.

The documentation was incorrect, as you mentioned: get_last_received_report() never returned None, though it said it did. That was an old error on my part. I may have intended to implement that, but never did.

The old property interface did not allow supplying a report id, so it changed from a property to a method.

get_next_received_report() still does return the "last received report", because incoming report values are not queued: older reports are overwritten by the latest report. So "next" does not really indicate the semantics. We could either implement queuing, or forget about that for now. Do you have a use case for queued OUT reports from the host?

If we don't implement queuing, I propose renaming your new method to receive_latest_report(). This is shorter and more accurate, and matches up better with send_report(). If we then eventually implement queuing, we could add another method receive_report() that returns None if there's nothing, or the next queued report.

If you feel so inclined, remove .last_received_report(), as we were supposed to remove it in 8.0.0 anyway. Otherwise I can push some commits to this PR to do that, or else I'll do it in another PR.

[maximkulkin]

You're missing the point of my design: this should look like a queue interface. The fact that right now it has room only for one report is an implementation detail. But I do not want that implementation detail leak into API design. Thus I insist on NOT renaming current method name: what if you change implementation to use a ring buffer later? You will have to RENAME the method and thus invalidate all the code that would rely on it. Having a queue interface does not guarantee you from running out of queue space anyways, in which case you would either need to drop new reports or overwrite old ones. So, this current implementation is like having a queue size of 1 and having new reports overwrite old ones if you do not process them fast enough.

The reason why I decided to go with get_next_received_report as opposed to receive_report, receive_lastest_report is that get carries the meaning that it returns you whatever it already has received as opposed to receive_report having a flavor of blocking until next report is received. So, all in all IMO current name fits both current and future uses, has correct semantics and matches existing method naming.

[dhalbert]

The fact that right now it has room only for one report is an implementation detail.

Re-reading all the posts carefully, I see what you are getting at. I'm not sure I would call that an implementation detail; it's a pretty important restriction at the moment. The fact that the current implementation is only a one-entry queue is not in the documentation; let's add that.

The other question is whether receive_report() implies blocking more than get_next_received_report(). I don't have the same feeling as you about it. A full API would probably have a queue-length parameter, and a timeout parameter, which would influence the behavior of the "get or receive next report" method. So, for instance,:

• UART has write() and read(). UART.read() is blocking or not, depending on UART.timeout.
• keypad.EventQueue.get() is not blocking, and returns None when there is no pending event. There is a queue length parameter in the related constructors.
• socket has send() and recv(). recv() is blocking or not, depending on whether the socket is blocking or not.

So it didn't really bother me that receive_report() is non-blocking. Your name is closer to the current API, which has a somewhat tortured name due to the originally restrictive semantics and history of .last_received_report() becoming get_last_received_report(). I don't have incredibly strong feelings about the name, but I like the symmetry of send_report() and receive_report().

@Neradoc do you have an opinion here?

[maximkulkin]

Ok, I guess shortening method names and introducing report receive queue does not hurt. But here are my considerations. Specifying queue size on construction is probably not a convenient idea, because HID objects (including builtin ones like Keyboard and Mouse) are constructed during boot and sometimes are out of reach. At that time it's hard to tell which queue size firmware developer will want. So it will probably be like a method call (or property assignment) to specify queue size. Also, since there can be multiple reports, they might have different rate of expected occurrence, and it would be wasteful to allocate queues of same size for each of them. Having API that allows specifying size for each report queue IMO is mouthful and also does not promote frugal resource usage. So, one idea I have is to allocate a shared buffer for all incoming reports and the receive API would have no report ID parameter (as it has now - to return particular reports only) and instead would return reports in the same form as they are sent now: if there's only one report, report data is returned. If multiple reports, report data is prefixed with report ID byte.

On the other hand, I can see situation where there is a frequent stateless report being received for which you might only care for the last report and you do not want it's copies to fill up receive buffer.

Let me know your thoughts.

[dhalbert]

@maximkulkin Thanks for your summary. Yes, I agree there is no constructor to use to specify a queue length. If we added it, making it a property (perhaps only settable in boot.py, before USB is configured), seems like the right choice.

One important consideration for us is code size. usb_hid is available on the smallest builds (e.g., SAMD21) because it is so useful. We are always running out of firmware space on these builds, so a fancier API (e.g., queuing) may be too big. That is why the current API has such limited functionality. A general API would be great, but if it can't fit or would use up space we need for other things later, that is a problem.

There are several alternatives here:

1. Fix get_last_received_report() to return None when no report has arrived since the last call. This brings it in line with the documentation. The HID library was recently fixed to handle None if it were returned: last_received_report will be removed in Circuitpython 8.0.0 Adafruit_CircuitPython_HID#100.

2. Provide a one-element queue implementation of receive_report(report_id=None) (aka get_next_received_report(). Document the one-element restriction. Do not provide a buffer-length setting.

3. Like 2, but handle >1-element queues

4. Implement receive_report() to return all received reports for the device, as you suggested:

   So, one idea I have is to allocate a shared buffer for all incoming reports and the receive API would have no report ID parameter (as it has now - to return particular reports only) and instead would return reports in the same form as they are sent now: if there's only one report, report data is returned. If multiple reports, report data is prefixed with report ID byte.

   On the other hand, I can see situation where there is a frequent stateless report being received for which you might only care for the last report and you do not want its copies to fill up receive buffer.

   I think it would be clearer to return a tuple (report_id, data) instead of optionally prefixing the report with the report ID. Yes, the ID is part of the sent report, but logically it is separate. When we send a report, we pass the report ID as a separate value.

Of these, 1 is the least work and the smallest. It is an API change, but the library is handling it, and we can change the API at major version boundaries. In this case we're making it match the documentation; previously the implementation was buggy. 2 is simple but if we ever did 4, we'd end up with a non-upward compatible API (though we could handle that by saying arg of None means all, or something like that.

Based on your needs, is 1 fine? 1 or 2 is fine with me, and I don't think we need to feel we are locked into that API in the long run.

[maximkulkin]

I think option 1 would be enough to unlock IN report functionality, given the code size constraints.

I will update this PR back to implement option 1.

[maximkulkin]

@dhalbert Updated back

[dhalbert]

Looks good! Thanks for your patience while we worked through the API possibilities.