Enhancement: HID watchdog

[regicidalplutophage]

HID classes now can create periodic tasks that check for connection and after its status has changed, re-run parts of code as needed.

[regicidalplutophage]

@RaulHuertas has agreed to test this with BLE. I know it's not very likely, but just in case, don't merge this before he does.

[xs5871]

How does this relate to "fixing" NKRO? Is it that the auto-detect has to wait until there's a connection?
And what does the watchdog even accomplish? I get that it can do a delayed initialization of the HID buffers. But after that it just runs in the background and does nothing. It will never be the case that the number and type of endpoints will change during runtime.

[xs5871]

Suggested change

	report_bytes_default = 8
	REPORT_BYTES = report_bytes_default
	REPORT_BYTES = 8

Probably an artifact? I don't see report_bytes_default being used.

[regicidalplutophage]

report_bytes_default is used in setup_keyboard_hid. It represents the number of bytes in the default descriptor, 8 for the base class, 9 for the USB HID. The REPORT_BYTES is first set to report_bytes_default, then as per NKRO autodetect, if hid_send() fails, it's set to the value required by NKRO.

[xs5871]

Suggested change

	# from kmk.scheduler import create_task

[xs5871]

Is there a reason you didn't put that in a setup_consumer_control_hid()?

[regicidalplutophage]

The only reason is that because there's only 1 CC descriptor, this doesn't need to be re-ran. After submitting this PR, I thought of putting this in a function for the sake of consistency, but since it were already submitted, I thought I'll wait for your opinion.

[xs5871]

Suggested change

	def show_debug(self):
	if debug.enabled:
	def show_debug(self):

We can save a jump to subroutine if you do:

if debug.enabled:
    self.show_debug()

[xs5871]

Suggested change

	def watchdog(self):
	if self.usb_status != supervisor.runtime.usb_connected:
	self.usb_status = supervisor.runtime.usb_connected
	self.find_devices()
	self.setup_keyboard_hid()
	self.setup_mouse_hid()
	self.show_debug()
	def _connected(self):
	return supervisor.runtime.usb_connected
	# return self.ble.connected # for BLE
	def watchdog(self):
	if self.connected != self._connected():
	self.connected = self._connected()
	self.find_devices()
	self.setup_keyboard_hid()
	self.setup_mouse_hid()
	self.show_debug()

Make a wrapper to check for connection, then use that wrapper in the watchdog and we can put the watchdog implementation into the base class.
This also triggers on disconnects / reconnects and reallocates buffers.

[regicidalplutophage]

This makes sense for the future, but as of right now that would be wasted on BLE since we don't support custom descriptors there.

[xs5871]

This belongs in __init__()

[regicidalplutophage]

First we blank out the dict, then we repopulate it. Blanking out might happen more than once.

[regicidalplutophage]

How does this relate to "fixing" NKRO? Is it that the auto-detect has to wait until there's a connection?
And what does the watchdog even accomplish? I get that it can do a delayed initialization of the HID buffers. But after that it just runs in the background and does nothing. It will never be the case that the number and type of endpoints will change during runtime.

This doesn't fix NKRO specifically, it fixes connecting and disconnecting USB, it's just that NKRO gave me trouble with its autodetect.

It works as follows:

The usb_hid.enable() that we're calling in bootconf doesn't populate usb_hid.devices immediately, the latter is only populated after the USB is connected. If the keyboard has booted but USB is not connected, usb_hid.devices is empty. Then KMK's HID class tries to populate its own device dict from that empty usb_hid.devices.

Watchdog makes it so that when connection is changed, we can detect that and rebuild self.devices accordingly. Rerunning find_devices() would've been enough if we only had 1 descriptor per device, but we need to set report bytes and such depending on which descriptor we're using and that can only be done when USB is connected. Technically we can only set it once if we wait for the initial connection, but connection changes aren't frequent so I don't mind running setup_*_hid() each time.

Basically, watchdog serves the same function as @property decorator, but instead of repopulating devices each time it's accessed, it checks for connection and repopulates them on connection status change. Plus it runs setup_*() methods.

[regicidalplutophage]

generalize methods and allow ble re-pairing

Reconnecting while remaining bonded, rather.

[xs5871]

This doesn't fix NKRO specifically, it fixes connecting and disconnecting USB, it's just that NKRO gave me trouble with its autodetect.

Autodetect has the potential to fail, we can mitigate that by doing the setup in a coroutine.
What is the issue with dis/reconnections? I don't see anything that could go wrong. I didn't manage to break anything, I really tried. Even checking the usb connection before every send_report is completely unecessary. The commit message for that line is a bit vague, maybe that got fixed upstream, we have better error handling now, or there never was an issue in the first place.
(I also read through the CP usb_hid code again)

The usb_hid.enable() that we're calling in bootconf doesn't populate usb_hid.devices immediately, the latter is only populated after the USB is connected. If the keyboard has booted but USB is not connected, usb_hid.devices is empty. Then KMK's HID class tries to populate its own device dict from that empty usb_hid.devices.

For reference: that is incorrect. I checked both in situ and in the CP code. The usb_hid.devices tuple immediately reflects the setup with usb_hid.enable() and is static until the next hard reset.

[regicidalplutophage]

What is the issue with dis/reconnections? I don't see anything that could go wrong.

For the sake of clarity, dis/reconnections mean data specifically, MCU remains powered, KMK remains running.
The test I'm doing is powering MCU, waiting for it to boot, connecting data, trying to type. Because NKRO detection relies on data connection (hid_send, specifically the), if MCU was booted without data connection present, report format determined at boot might not be the right format. I'm realizing now this does indeed only affects NKRO and the other way around error was caused by my coding errors during experimentation.
Technically, for USB, not BLE, this is the issue of initial connection, not dis/reconnection.

Separately, I were trying to remove the @property decorator from the BLE HID class by using the same watchdog mechanism to update properties. Then later, use watchdog to put MCU in the advertising state upon BLE connection loss, allowing us to re-estabilish BLE connection without rebooting. This really is the dis/reconnection issue.

[regicidalplutophage]

@xs5871 I've done some of the stuff we've talked about in DMs. setup_* and find_devices methods are still doing double duty: when called during init they provide defaults and when used in scheduled tasks they set what they need to set.

[xs5871]

For the sake of moving forwards I'm going to merge this. There's still a bunch of stuff that needs a bit of polish, but since I also have a refactor that now requires a rebase, I'll just do both in one go; soonish.

[regicidalplutophage]

Oh no! I haven't heard from Raul yet so I don't yet know if this borks BLE

[xs5871]

We'll fix it in post.