Skip to content
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

Implement NKRO for VUSB #12010

Closed
wants to merge 1 commit into from
Closed

Implement NKRO for VUSB #12010

wants to merge 1 commit into from

Conversation

valpackett
Copy link
Contributor

V-USB is limited to 8-byte endpoints, but there is already precedent for transferring larger data by issuing multiple interrupt transfers in a row.

With a dedicated NKRO interface, we can avoid a report ID, so the report would be 16 bytes - fitting right into only two transfers.

Enabled on spiderisland/split78 where it was tested.

V-USB is limited to 8-byte endpoints, but there is already precedent for
transferring larger data by issuing multiple interrupt transfers in a row.

With a dedicated NKRO interface, we can avoid a report ID, so the report
would be 16 bytes - fitting right into only two transfers.
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

With a dedicated NKRO interface, we can avoid a report ID, so the report would be 16 bytes - fitting right into only two transfers.

This would be nice, but I plan to unify the USB descriptors based on the ones used for LUFA & ChibiOS, so there is not much point in doing that when it'll eventually be merged into the shared interface anyway.

@valpackett
Copy link
Contributor Author

Then maybe there should be a NKRO_SHARED_EP flag (like MOUSE_SHARED_EP etc.) that would allow a dedicated NKRO interface anywhere?

@fauxpark
Copy link
Member

fauxpark commented Feb 26, 2021

The problem is that the microcontrollers that QMK supports only have so many endpoints, VUSB even less, which is why the shared EP stuff exists in the first place. I don't really see much of a use case for a dedicated NKRO interface, but, yes, it could be done.

@stale
Copy link

stale bot commented Apr 18, 2021

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

Comment on lines +14 to +19
COMMAND_ENABLE = yes # Commands for debug and configuration
# Do not enable SLEEP_LED_ENABLE. it uses the same timer as BACKLIGHT_ENABLE
SLEEP_LED_ENABLE = no # Breathing sleep LED during USB suspend
BACKLIGHT_ENABLE = yes # Enable keyboard backlight functionality
RGBLIGHT_ENABLE = no # Enable keyboard RGB underglow
NKRO_ENABLE = yes
Copy link
Member

Choose a reason for hiding this comment

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

Roll these back, optionally putting them into a personal keymap instead.
No need to change these defaults.

@tzarc tzarc requested review from a team and fauxpark November 1, 2021 17:07
@MagicRB
Copy link

MagicRB commented Nov 4, 2021

I'll test this PR out and then take it over if and try to upstream it, if the code works.

@sigprof
Copy link
Contributor

sigprof commented Nov 5, 2021

This code also has an undocumented limitation: only keycodes in the 0x000x77 range are supported (up to KC_SELECT; KC_STOP is out of range). This may be a problem for some users — e.g., see this excerpt from the keycode table (KC_INT1 is 0x87):

Key Aliases Description Windows macOS Linux1
KC_INT1 KC_RO JIS \ and _
KC_INT2 KC_KANA JIS Katakana/Hiragana
KC_INT3 KC_JYEN JIS ¥ and |
KC_INT4 KC_HENK JIS Henkan
KC_INT5 KC_MHEN JIS Muhenkan
KC_INT6 JIS Numpad ,
KC_INT7 International 7
KC_INT8 International 8
KC_INT9 International 9
KC_LANG1 KC_HAEN Hangul/English
KC_LANG2 KC_HANJ Hanja
KC_LANG3 JIS Katakana
KC_LANG4 JIS Hiragana
KC_LANG5 JIS Zenkaku/Hankaku
KC_LANG6 Language 6
KC_LANG7 Language 7
KC_LANG8 Language 8
KC_LANG9 Language 9

(Although KC_LANG1KC_LANG5 are not marked as supported by Windows in the current QMK documentation, the Microsoft USB HID to PS/2 Scan Code Translation Table contains PS/2 scancodes for them, so they can potentially be supported in some way.)

@stephen-huan
Copy link
Contributor

I'll test this PR out and then take it over if and try to upstream it, if the code works.

Any word on when this patch will be merged into QMK? I applied the patch to the most recent version and it seems to work, testing on the atmega382p v-usb keyboard plaid.

The problem is that the microcontrollers that QMK supports only have so many endpoints, VUSB even less, which is why the shared EP stuff exists in the first place. I don't really see much of a use case for a dedicated NKRO interface, but, yes, it could be done.

The use case for me is stenography which all but requires NKRO. Unfortunately QMK's stenography protocol support (though either TX Bolt or GeminiPR) doesn't work because v-usb does not currently have support for virtser (issue #13834).

It is true that v-usb only has 3 endpoints, but one only really needs the standard/boot keyboard endpoint, mouse keys and extra keys which is combined into a single endpoint, and the new NKRO endpoint. I think other hardware USB protocols are able to pack mouse, extra keys, and NKRO into a single endpoint which is nice, but not necessary. Just having boot keyboard, mouse/extra keys, and NKRO is sufficient for my use case.

Lastly, merging this patch would resolve issue #2775 and pull request #9054. Searching for "v-usb nkro" in the issues made me think that NKRO is impossible over v-usb and the fact that QMK will suppress the build option if one attempts to enable it reinforces this mistaken belief.

@fauxpark
Copy link
Member

fauxpark commented Apr 5, 2022

It is true that v-usb only has 3 endpoints, but one only really needs the standard/boot keyboard endpoint, mouse keys and extra keys which is combined into a single endpoint, and the new NKRO endpoint. I think other hardware USB protocols are able to pack mouse, extra keys, and NKRO into a single endpoint which is nice, but not necessary. Just having boot keyboard, mouse/extra keys, and NKRO is sufficient for my use case.

I am aware of that, this is what the LUFA and ChibiOS code does - NKRO, mouse and media keys share a single endpoint. There is a define for giving mousekeys its own endpoint as there are/were apparently some issues with it. The person above was talking about splitting off the NKRO report in a similar way, which is technically possible, but very limiting for V-USB boards.

As for virtser, to be honest, I don't see it being possible on V-USB precisely due to this endpoint limitation. It requires two distinct IN endpoints and one OUT. Again, technically there is no problem with this, but you will have used up all three IN endpoints with 6KRO and the two virtser ones, with no space left for NKRO.

@zvecr
Copy link
Member

zvecr commented Nov 13, 2023

Closing in favour of #22398 now the codebase has moved on.

@zvecr zvecr closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants