Usage of report_idle hash map
Closed this issue · 4 comments
I noticed that in RawInterface you are using FnvIndexMap to store idle times received from SET_IDLE requests. Is it actually needed to use a hash map there? As far as I understand report IDs will always start from 0, so a simple array would seem sufficient, something like [Option<u8>; 32] (64 B, could be even smaller by using [u8; 32] along with a u32 bitmap of "existing" entries to get 36 B). Also, using 32 entries seems like an overkill, as most often there will be just a single report, and it should be very rare to have more that 4.
My question is due to the fact that hashing seems unnecessary (vs simple array indexing), and additionally this hash map takes significant amount of space in the structure (260 B out of 460 B), which is then multiplied by the number of HID interfaces. Here's how the size looks like:
Do you think changing this makes sense? Or is there some other reason for using a hash map?
Another potential optimization could be defining the size of control_*_report_buffer using const generics to pass max size of the report. But this would add some complexity to the rest of code base due to new generic parameter.
And an unrelated question: why writing a duplicate report causes a tick? Non-duplicates are rare, so wouldn't that mean that the time passes almost two times faster (e.g. 1kHz write_report + 1kHz tick())?
The duplicate tick() is left over from the transition away from using embedded_time timers to track the timeout. I'll remove it.
As usual this comes down to a balance of size, ergonomics and correctness.
The HID spec specifies that the report id is an unsigned byte and can't be zero. There are no restriction on how they are picked and how many a device has. While I agree your suggestions around having a small number of report IDs is practical, it wouldn't work with arbitrary valid HID descriptors.
The IndexMap is overkill from both a size and performance cost in almost all common cases. I would expect that this could be transitioned to a Vec or similar with minimal changes.
The buffers could also be parameterised.
At the moment the library is pretty good at letting someone implement an arbitrary HID quickly and easily with good ergonomics or use one of the off-the-shelf examples quickly. I agree that concrete interfaces, like the keyboards, where the descriptor and report sizes are well defined could be optimised for size.
Sure, I'm really grateful for this library. I just wanted to consult some things before I start working on this, as this looks like an easy place for some optimizations. I am planning to create a PR to replace the hash map with something smaller and maybe to add some const generic parameters for sizes. But I have a busy week so this will have to wait until next week.
No problem at all.
You might want to hold off any major changes. Given we've started to make breaking changes I'm starting work on some large scale refactoring towards a v1. I apricate the work you've put into doing the size analysis, and is something I'm going to take more into account in the future work.