Fix #117: Rework host USB ready timeout

[eightycc]

Resolves #117.

[uzi18]

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

[eightycc]

@dhalbert Over to you for review!

[eightycc]

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

[dhalbert]

Thanks for working on this. A few ideas and q's:

1. Did you try spinning on supervisor.runtime.usb_connected to see if that worked out instead of trying the no-op sends? The advantage of that is that the connection testing could then be refactored into a single common routine, to reduce code size. It could be made part of find_device(), so that not only does it have to find the device, but it also has to test connectivity.

2. Instead of a fixed timeout, each __init__() could take an optional argument , timeout: int = 20). That would allow quicker failures for people who don't need to wait 20 seconds. (20 seconds is a long time). The timeout could get passed to find_device() if 1. is implemented.

3. A better error message: raise OSError("Failed to initialize HID device. Are you connected?"). Of course, you're not connected, no one is going to see this :) .

[eightycc]

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

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]

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

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

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.

[dhalbert]

Thanks! Looks good -- did you test in the two cases of cold and warm RPi host startup?

Why did you lengthen the timeout to 45 seconds? That is a really long time, more than the 15 seconds you were seeing during initial testing. If someone needs a timeout that long, they could explicitly supply it.

[eightycc]

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]

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

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]

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]

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]

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

Shall I wait for adafruit/Adafruit_Blinka#711 or plow ahead with the guard? Or maybe you'd like me to write a pull for Blinka?

[dhalbert]

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]

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

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

[dhalbert]

Thanks for your perseverance through the review process. I think this is a significant improvement to the current semantics.

In discord someone mentioned that supervisor.runtime.usb_connected may not detect disconnection properly: it might still return True. However, that is not this library's problem, and we can investigate that separately.

I am going to make this a major revision change, because the semantics have changed, since the devices now by default wait indefinitely instead of only a second or two.

I tested this on a MacroPad, first when it was connected to a live host computer, and then secondly with a data/charge switch (https://www.adafruit.com/search?q=data+charge+usb+switch). In the latter case, I plugged it in and powered the board, and then waited about half a minute before flipping the switch to data. The code waited until there was data connectivity.

[uzi18]

where to get mpy files ?

[eightycc]

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]

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

[uzi18]

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

[dhalbert]

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