search

adafruit / Adafruit_CircuitPython_HID

USB Human Interface Device drivers.
MIT License
364 stars 106 forks source link

Fix #117: Rework host USB ready timeout #118

Closed eightycc closed 9 months ago

eightycc commented 9 months ago

Resolves #117.

uzi18 commented 9 months ago

@eightycc is it related also to boot keyboard? Could try to test it.

eightycc commented 9 months ago

@dhalbert Over to you for review!

eightycc commented 9 months ago

@uzi18 Fixes original issue in #117. Give it a try if you like. Tested here with an RP 4 host and it works.

eightycc commented 9 months ago

@dhalbert Thanks for the thoughtful review.

  1. Embarrassed to say I didn't. I can see the value of factoring the code, so I'll give it a try.
  2. Good idea. I'll do it.
  3. Thought about a more verbose message and then realized the same thing you did, the user won't be able to see it unless they've taken steps like I did by building a custom CP with UART debug. Shall we keep it short and sweet to save precious bytes?
dhalbert commented 9 months ago

3. Thought about a more verbose message and then realized the same thing you did, the user won't be able to see it unless they've taken steps like I did by building a custom CP with UART debug. Shall we keep it short and sweet to save precious bytes?

If there's just one mention, it could be longer, because it will be in only .mpy (especially if frozen). I realized it will show up on boards with displays even if they don't have USB connectivity, so making it a bit more helpful will help at least some of the time.

eightycc commented 9 months ago

@dhalbert I've made the updates you requested. CI is failing due to my import of supervisor, but the code definitely works. My bug or CI?

Warning, treated as error:
autodoc: failed to import module 'keyboard' from module 'adafruit_hid'; the following exception was raised:
No module named 'supervisor'
eightycc commented 9 months ago

@dhalbert Also, I should have given the timeout parm in find_device a default, otherwise it's going to break existing user code. I'll fix that too when I hear back from you.

dhalbert commented 9 months ago

CI is failing due to my import of supervisor

Hmm, this is because adafruit_blinka does not have any support for supervisor. It does have support for usb_hid via the usb_gadget Linux kernel driver.

We could add limited support of supervisor to Blinka. That is another project, though maybe a small one. If a Linux host is using usb_hid in the same way to emulate being a USB device, it will encounter the same issue that CircuitPython might encounter, of the USB host not being ready.

In the meantime, you could guard the import supervisor and just not test supervisor.runime.usb_connected in find_device() right now if supervisor is not available. Maybe just delay for a second or too, because that's what the old code did. The number of people using the usb_gadget capability is small, so they could do some special coding in the meantime or just not update their adafruit_hid library.

@makermelissa Do you have any opinion about adding limited supervisor support to Blinka, specifically supervisor.runtime.usb_connected? I see no mention of supervisor in the Blinka issues. I will look to see if usb_gadget has a simple way to test for being connected, but I have no experience with it.

eightycc commented 9 months ago

Yes, I did increase the timeout to 45 seconds. The reason is that the PI 4 was actually taking about 40 seconds, but I didn't see it because there's another timeout deeper in the USB code that was adding a second for every one of my seconds. Yikes!

eightycc commented 9 months ago

@dhalbert Yes, I did test hot and cold. Hot starts up immediately.

I saw the guarding of other imports and had wondered about it. Now I know that Blinka is part of the ecosystem. If I delay say 1 or 2 seconds in the Blinka case, it should just fail like it used to in all cases with a USB busy throw, so no harm done and no need not to update the library for Blinka.

Since the timeout interval won't affect normal usage, I'm not too worried about a long timeout in the failing case. But, it's your call. Timeouts and watchdogs are both bears to get right!

dhalbert commented 9 months ago

I just want to confirm my understanding of the use case. Let's suppose you have an RPi with, say, a MacroPad plugged in, and also a conventional keyboard and a USB stick drive.

When you power up the RPi, can it take up to 40 seconds for it to be responsive to the keyboard, and to mount the USB drive? Is this an RPi 4 or something that boots more slowly?

eightycc commented 9 months ago

When you power up the RPi, can it take up to 40 seconds for it to be responsive to the keyboard, and to mount the USB drive? Is this an RPi 4 or something that boots more slowly?

Yes, the RPi 4 takes that long to get to USB enumeration. I'm testing with a stock 4G model loaded with an up to date but unmodified Raspbian 32-bit image. On POR, it's got gobs o' blobs that it has to load up before it even gets to the kernel.

The use case is any USB device that is already plugged in at POR. In the case of the OP on the issue, it's a Pico bridging an old scan-style Amiga keyboard to USB.

dhalbert commented 9 months ago

Another idea is if the timeout is None, to wait indefinitely. Maybe that should actually be the default. That's the way hardware keyboards, etc. work. They won't timeout. Then we don't need to get into testing "worst case" things.

eightycc commented 9 months ago

@dhalbert Double-plus good on the timeout None idea. Consider it done!

Shall I wait for https://github.com/adafruit/Adafruit_Blinka/issues/711 or plow ahead with the guard? Or maybe you'd like me to write a pull for Blinka?

dhalbert commented 9 months ago

I started looking at Blinka and usb_gadget but didn't get very far into it, and I have other things i need to work on at the moment.

You could look at what it would take in Blinka, but we don't have to hold this up, since you have a workaround without it. We can always revise this library again.

eightycc commented 9 months ago

@dhalbert Over to you for what should be a final review! I'm digging into Blinka now and will update https://github.com/adafruit/Adafruit_Blinka/issues/711 when I've got additional status.

Interesting that reversing 117 gives 711, but I digress badly.

uzi18 commented 9 months ago

where to get mpy files ?

eightycc commented 9 months ago

where to get mpy files ?

Just grab the adafruit_hid folder from this project and put it in your lib folder. The .mpy version will follow when this is integrated into the CP library bundle. @dhalbert says at the next major revision, so I take that to mean CP 9.0.

Edit: You can find the .mpy version here: https://github.com/adafruit/Adafruit_CircuitPython_HID/releases

dhalbert commented 9 months ago

The new release, 6.0.0, will go in the daily bundle tonight.

uzi18 commented 9 months ago

@dhalbert have some questions about CP, must open new issue for this or ask somewhere else?

dhalbert commented 9 months ago

@uzi18 If you have general use questions, ask in discord, https://adafru.it/discord, channel #help-with-circuitpython. That is community-based support with some staff people hanging out. Or you can open a thread in our forums, which is "official" support: https://forums.adafruit.com/viewforum.php?f=60. For a dialog, discord works well.