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

Enhancement: HID watchdog #1061

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Enhancement: HID watchdog #1061

wants to merge 4 commits into from

Conversation

regicidalplutophage
Copy link
Member

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
Copy link
Member Author

regicidalplutophage commented Dec 19, 2024

@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.

Copy link
Collaborator

@xs5871 xs5871 left a comment

Choose a reason for hiding this comment

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

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.

kmk/hid.py Outdated
Comment on lines 61 to 62
report_bytes_default = 8
REPORT_BYTES = report_bytes_default
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

kmk/hid.py Outdated
Comment on lines 9 to 10

# from kmk.scheduler import create_task
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# from kmk.scheduler import create_task

kmk/hid.py Outdated
Comment on lines 71 to 73
self._cc_report = bytearray(HID_REPORT_SIZES[HIDReportTypes.CONSUMER] + 1)
self._cc_report[0] = HIDReportTypes.CONSUMER
self._cc_pending = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

kmk/hid.py Outdated
Comment on lines 80 to 81
def show_debug(self):
if debug.enabled:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()

kmk/hid.py Outdated
Comment on lines 313 to 319
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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

debug('disable mouse')

def find_devices(self):
self.devices = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This belongs in __init__()

Copy link
Member Author

@regicidalplutophage regicidalplutophage Dec 19, 2024

Choose a reason for hiding this comment

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

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

@regicidalplutophage
Copy link
Member Author

regicidalplutophage commented Dec 19, 2024

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
Copy link
Member Author

generalize methods and allow ble re-pairing

Reconnecting while remaining bonded, rather.

@xs5871
Copy link
Collaborator

xs5871 commented Dec 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants