micropython / micropython-lib

Core Python libraries ported to MicroPython
Other
2.3k stars 980 forks source link

HID keyboard won't release keys when called via schedule #873

Open mischif opened 3 weeks ago

mischif commented 3 weeks ago

Calling KeyboardInterface().send_keys() from a function called via micropython.schedule() doesn't seem to allow keys to be released, causing a spamming issue. This code reproduces the issue:

import usb.device
from usb.device.keyboard import KeyboardInterface
from time import sleep_ms
from micropython import schedule

KBD = None

def send_keys(foo):
    keys = [-0x02, 5]
    print("sending keypress")
    KBD.send_keys(keys)

    keys.clear()
    print("clearing keypress")
    KBD.send_keys(keys)

def main():
    global KBD
    KBD = KeyboardInterface()
    usb.device.get().init(KBD, builtin_driver=True)

    sleep_ms(5000)
    print("starting loop")

    while True:
        send_keys(None)
        # schedule(send_keys, None)
        sleep_ms(3000)

if __name__ == '__main__': main()

running schedule(send_keys, None) instead of send_keys(None) causes a flood of :b:, this is on a Pico running 1.23.0 and whatever the latest version of usb-device-keyboard is currently in mip.

projectgus commented 3 weeks ago

Hi @mischif,

Thanks for the very clear report.

At the moment calling send_keys() will block for up to the timeout (default 100ms) waiting for the USB endpoint to be available to queue a new transfer, and then returns True for success or False for failure. In your scheduled send_keys() function I'm guessing the second call is returning False indicating that the transfer wasn't able to queue to be sent to the host. Probably this is not ideal behaviour for this function, it should throw an exception on timeout. I'll look at changing it.

However, the underlying problem will still be there. An operation like send_keys() returns when the transfer is queued for sending to the host, but this may still take some time before the USB host actually performs the transfer. In the meantime, the USB endpoint isn't available. The reason why the USB endpoint never becomes available, even after 100ms, is that the transfer completion callback is also processed via a schedule() internally. Blocking inside a schedule callback prevents the USB completion callback from running.

If you need to send and release a keypress with micropython.schedule() then you can probably do something like this to chain multiple schedule callbacks together:

def send_and_release(keys):
    if not KBD.is_open():
        print("Can't send, USB disconnected")
    elif not KBD.send_keys(keys, 0):
        schedule(send_and_release, keys)  # Try again after other callbacks run
    elif keys:
        schedule(send_and_release, [])  # Schedule the release if needed

However it might be better to look into approaches other than schedule to send more than one keyboard transfer in a sequence. Possibly a thread, possibly asyncio, or handling it in the normal "main" execution somehow.

mischif commented 3 weeks ago

The way I envisioned my code working is a button press triggers an ISR which determines if a keypress needs to be sent; if so, it calls a handler function via schedule() which would hopefully call send_keys() directly.

If I'm understanding correctly this means there's no best way to call send_keys() from an ISR (you don't want to call it directly as it could take too long, calling it through a function with schedule() is out, updating some global would be tricky if the value could vary in size).

As a workaround I have the scheduled function update a global the main loop can call send_keys() for, but it's kind of a hack; could you add a function to an event loop in an ISR?

projectgus commented 3 weeks ago

If I'm understanding correctly this means there's no best way to call send_keys() from an ISR (you don't want to call it directly as it could take too long, calling it through a function with schedule() is out, updating some global would be tricky if the value could vary in size).

Yes, that's pretty much right. You can technically call send_keys() from a soft ISR, but you can't guarantee the call is going to succeed so it's best to structure it in some other way.

USB devices are a bit limited in what they can do here because all USB transfers are initiated by the USB host. So calling "send_keys" is really only preparing the key data to go to the host the next time it asks for it. The USB library is written in a way where only one transfer can be queued at a time.

As a workaround I have the scheduled function update a global the main loop can call send_keys() for, but it's kind of a hack;

I think this is actually a good way to structure it, if your code is going to have a main loop anyway.

could you add a function to an event loop in an ISR?

Not sure I understand the question, can you please give some more detail?

dpgeorge commented 3 weeks ago

calling it through a function with schedule() is out,

That's not true (if you're referring to KBD.send_keys(), and not your custom send_keys() function): you should still be able to call KBD.send_keys() from a schedule() call. It's just that you can't call KBD.send_keys() twice in one scheduled callback. @projectgus 's code above shows how you can call KBD.send_keys() from a scheduled callback.

mischif commented 3 weeks ago

can you please give some more detail?

I was wondering about calling asyncio.run() or asyncio.create_task() inside an ISR to avoid calling schedule() in my code, but I didn't know if an ISR was too constrained an environment to do that.

It's just that you can't call KBD.send_keys() twice in one scheduled callback

AIUI, a single keypress requires two calls to KBD.send_keys(), one to press the keys down and one to release it; I saw the above code about chained calls to schedule() but that seems like a bigger hack than figuring out a way to update a global and I believe has a potential race condition if something else gets scheduled in between the calls.

projectgus commented 3 weeks ago

I was wondering about calling asyncio.run() or asyncio.create_task() inside an ISR to avoid calling schedule() in my code, but I didn't know if an ISR was too constrained an environment to do that.

I wouldn't recommend trying to do this exactly, but if you already have an async task created and blocked on something like a ThreadSafeFlag then you could signal that from the ISR to wake the task up. Then it could send the key down event, await asyncio.sleep_ms(10) or similar to allow processing, and then send the key up event.