Keep all key sections on keyboards that use more than one - #359
Open
mglushko wants to merge 2 commits into
Open
Conversation
Some keyboards list their keys in several separate sections instead of one long run. The code was only holding on to the last section it came across, so all the keys in the earlier sections got dropped. On the Wooting Two HE from issue hrvach#335 this meant only the modifier keys worked and nothing else came through. This keeps every section instead of just the last one. It also adds a length check so a shorter than expected report cannot read past the end of the buffer.
The Wooting Two HE declares its key bitmap as four Usage Minimum/Maximum ranges, not three, and the third is only 8 bits wide. The size > 32 test treated that as noise and dropped it, so usages 0x9D to 0xA4 never made it into a block. Recognise a section by its usage range mapping one usage per bit instead, which is what the extraction already requires of every block it is given. The modifier is the one small range that also maps one usage per bit, and it is handled separately just above, so exclude it here. Requiring a non-empty range keeps out items that never carried a Usage Minimum/Maximum and still have both ends at zero, which would otherwise be stored as a bogus block and fail extraction for the whole keyboard. Parses identically to before on a boot keyboard, an NKRO keyboard, a composite dongle and the other test descriptors; only the Wooting changes, from three sections to four.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi, I had a go at issue #335.
As far as I can tell, the problem is that some keyboards list their keys in several separate sections rather than one long run. The code was only keeping the last section it came across, so everything in the earlier ones got dropped. On that Wooting the modifier keys are stored separately, which is why those kept working while nothing else did.
This change keeps all of the sections instead of only the last one.
Looking at it more closely, the Wooting actually has four sections, and one of them is only 8 bits wide. The old code worked out what counted as a key section by its size, so that short one got thrown away as well. Rather than going by size, it now looks for a section whose usage range lines up one usage per bit, which is what the extraction code already expects of anything handed to it. The modifier is the only small section that also looks like that, and it is dealt with just above, so it is left out here.
I also added a small length check so a report that comes in shorter than expected cannot read past the end of the buffer.
To test it I put together a little program that runs the real parsing code on my PC against the descriptor posted in the issue, and the keys come out right now. I also checked a plain keyboard, a normal NKRO keyboard and a composite dongle, and they parse exactly the same as before. It is here in case it is useful to anyone: https://github.com/mglushko/deskhop-hidtests
I do not own this keyboard, so I have not been able to try it on real hardware. Very happy to change anything or drop it if you would rather solve it another way.