Open dhalbert opened 3 years ago
dhalbert
commented
3 years ago I did a little more experimentation on this. Commenting out the actual pixel reading of the OnDiskBitmap in the C code helps. The display redisplay can be slow, as noted below, especially for this case.
tannewt
commented
3 years ago I'd assume this is due to the time needed to refresh the 240x240 pixels in the first tile.
dhalbert
commented
2 years ago displayio_tilegrid_fill_area() gets called, which calls common_hal_displayio_ondiskbitmap_get_pixel() for each pixel. A pixel read does a seek and a read of the OnDiskBitmap file. If the file sector is cached, it's fast, but otherwise it could take a long time.
Somewhere, the code should notice that it took a long time, and do usb_background() or the equivalent. There may be other high-priority background tasks to do as well.
I don't see an easy general solution to this. common_hal_displayio_ondiskbitmap_get_pixel() could notice when it took a long time by looking at ticks. There's nothing returned from f_read() that says the cache was or was not used. Or displayio_tilegrid_fill_area() could notice in its inner loop that a lot of time had passed, by something it called, and do the high-priority background tasks.
@tannewt Do you think this is worth fixing for 7.x.x or is it Long Term? I am willing to make it Long Term.
tannewt
commented
2 years ago It might be simplest for OnDiskBitmap to run background tasks for every pixel. The check for having no background tasks to do is pretty cheap and would hopefully be inlined.
FoamyGuy
commented
2 years ago I was able to recreate the extra key presses issue with a slightly modified version of the code on CLUE:
import board
import displayio
import digitalio
import keypad
import usb_hid
from adafruit_hid.keyboard import Keyboard
from adafruit_hid.keycode import Keycode
keyboard = Keyboard(usb_hid.devices)
display = board.DISPLAY
bitmap = displayio.OnDiskBitmap("/parrot-240-sheet.bmp")
parrot0_grid = displayio.TileGrid(bitmap, pixel_shader=bitmap.pixel_shader,
tile_height=240, tile_width=240)
group = displayio.Group()
group.append(parrot0_grid)
display.show(group)
keys = keypad.Keys((board.BUTTON_A, board.BUTTON_B), value_when_pressed=False, pull=True)
parrot0_grid[0] = 0
p = 1
while True:
key_event = keys.events.get()
if key_event:
key_number = key_event.key_number
if key_event.pressed:
parrot0_grid[0] = p
p += 1
if p > 9:
p = 0
keycode = Keycode.A + key_number
keyboard.send(Keycode.SHIFT, Keycode.A + key_number)
print("sent SHIFT and", keycode)I tried what I understand to be the proposal above: adding the usb_background() call inside of ODB get pixel function here: https://github.com/FoamyGuy/circuitpython/blob/a9e1acff09cba12ce03e18dbb23d528e2366ce63/shared-module/displayio/OnDiskBitmap.c#L155-L157
But this did not seem to resolve the extra key press issue. I still get long strings of A's and B's somewhat frequently when pressing the buttons.
tannewt
commented
2 years ago Are we missing the release events from the keypad queue? I think I've heard of this problem as "sticky keys". press/release APIs have this problem if events get lost. Instead, folks use an API that always provides pressed state so that any dropped events don't result in missed presses.
dhalbert
commented
2 years ago Are we missing the release events from the keypad queue?
I am not sure that is the problem. Adding usb_background() doesn't do anything for delayed key presses, if it's the scanning that's being delayed as opposed to the transmission.
It would be easy to test this by giving a longer queue length for the Keys object.
dhalbert
commented
2 years ago press/release APIs have this problem if events get lost. Instead, folks use an API that always provides pressed state so that any dropped events don't result in missed presses.
Stuck keys would be missing releases, right?
That would be a big change to the API. If the queue is long enough, this is rare, and there is already a mechanism to check and reset. We could open another issue for this.
tannewt
commented
2 years ago Stuck keys would be missing releases, right?
Yup. I meant an API that always tells you every key that is pressed.
That would be a big change to the API. If the queue is long enough, this is rare, and there is already a mechanism to check and reset. We could open another issue for this.
I don't think it needs to be a huge change. (Though I'm using keypad on my keyboard and not seeing this issue with RP2040.) We could add a bitmask (bytes object) to the existing events in the short term. This is the problem that @ATMakersBill described here: https://youtu.be/TwxYrRGAOko?t=1036
Observed in 7.0.0.
Setting the grid for an
OnDiskBitmapTileGridcan cause USB communication to be delayed long enough that a keypress can start repeating. I noticed this while working on the Kitty_Paw_Keypad example. I was converting it to usekeypad, and saw repeated keypresses. However, the program was only sending one keypress, and then, noticeably delayed (about half a second), the corresponding key release. The delay was long enough to cause auto-repeat (typematic) to be invoked for the keyboard. Just before this occurrence, aTileGridsubscript was set.I reproduced this on both Qt Py RP2040 and Metro M4, so it's not port-specific. The auto-repeat does not always happen, but happens often enough to be quite noticeable.
Example program (simplified from original):
Here is an example when auto-repeat start up. Here are the HID reports received by the host, sending Shift-B and then releasing both. The actual keypress was much quicker.
Notice that the timestamps are about 0.47 seconds apart.
Here are the input events, with auto-repeat happening (the repeated B key):