Fixed NKRO issue caused by HID_SET_PROTOCOL on Chibios platform

[lokher]

Description

NKRO is forced to turn on by mistake when receiving HID_SET_PROTOCOL.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[tzarc]

Weak approval, untested.

[sigprof]

Looks like there are two separate fixes here:

1. The tmk_core/protocol/chibios/usb_main.c change looks correct (there might be other problems nearby — e.g., the idle handling apparently should not be constrained to the boot protocol, but that's a different problem, and might not be important in practice if real OSes don't use SET_IDLE with keyboard devices).
2. The tmk_core/protocol/chibios/usb_driver.c change apparently removes sending of zero-length packets for non-bulk endpoints. This might not be 100% correct — the HID spec says that if an interface uses multiple reports, the longest of those reports does not require a short packet terminator even if its length is a multiple of wMaxPacketSize, but any shorter reports require such terminator, and that might be a zero-length packet if the length of some shorter report happens to be a multiple of wMaxPacketSize. However, I'm not sure whether QMK can trigger this case currently.

[lokher]

Looks like there are two separate fixes here:

1. The tmk_core/protocol/chibios/usb_main.c change looks correct (there might be other problems nearby — e.g., the idle handling apparently should not be constrained to the boot protocol, but that's a different problem, and might not be important in practice if real OSes don't use SET_IDLE with keyboard devices).
2. The tmk_core/protocol/chibios/usb_driver.c change apparently removes sending of zero-length packets for non-bulk endpoints. This might not be 100% correct — the HID spec says that if an interface uses multiple reports, the longest of those reports does not require a short packet terminator even if its length is a multiple of wMaxPacketSize, but any shorter reports require such terminator, and that might be a zero-length packet if the length of some shorter report happens to be a multiple of wMaxPacketSize. However, I'm not sure whether QMK can trigger this case currently.

I did some more research, something is ambiguous in USB spec, but seems zero-length packet is not an issue, I will remove the change of zero length packet.

5.7.3 Interrupt Transfer Packet Size Constraints
An interrupt transfer is complete when the endpoint does one of the following:`

• Has transferred exactly the amount of data expected
• Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet

When an interrupt transfer is complete, the Host Controller retires the current IRP and advances to the next IRP. If a data payload is received that is larger than expected, the interrupt IRP will be aborted/retired and the pipe will stall future IRPs until the condition is corrected and acknowledged.

5.8.3 Bulk Transfer Packet Size Constraints
A bulk transfer is complete when the endpoint does one of the following:

• Has transferred exactly the amount of data expected
• Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet

When a bulk transfer is complete, the Host Controller retires the current IRP and advances to the next IRP. If a data payload is received that is larger than expected, all pending bulk IRPs for that endpoint will be aborted/retired."

[drashna]

Been using, doesn't seem to break anything, at least.

[fauxpark]

keymap_config.nkro should follow the SET_PROTOCOL request, so I think this change is wrong.
https://wiki.osdev.org/USB_Human_Interface_Devices#.22SetProtocol.22_request

faux's comment seems to point towards it being incorrect, revoking

[lokher]

keymap_config.nkro should follow the SET_PROTOCOL request, so I think this change is wrong. https://wiki.osdev.org/USB_Human_Interface_Devices#.22SetProtocol.22_request

keyboard_protocol is set according to SET_PROTOCOL request. keyboard_protocol and keymap_config.nkro value in void host_keyboard_send(report_keyboard_t *report) determine if nkro report is to be sent:

qmk_firmware/tmk_core/protocol/host.c

Lines 68 to 78 in fc0bf67

	if (keyboard_protocol && keymap_config.nkro) {
	/* The callers of this function assume that report->mods is where mods go in.
	* But report->nkro.mods can be at a different offset if core keyboard does not have a report ID.
	*/
	report->nkro.mods = report->mods;
	report->nkro.report_id = REPORT_ID_NKRO;
	} else
	#endif
	{
	#ifdef KEYBOARD_SHARED_EP
	report->report_id = REPORT_ID_KEYBOARD;

Current issue is if the keymap_config.nkro is initially disabled, SET_PROTOCOL request during USB enumeration or OS boot will enable it automaticlly which is buggy.

[tzarc]

Move to next quarter for a day-one merge. Can sort out the problems during the develop cycle.

[tzarc]

As previously mentioned -- tested and found no issue. Giving the develop cycle a chance to iron out issues with other people.