KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.33k stars 460 forks source link

[BUG] keybow2040: RGB causing infinite loop #859

Closed JimMadge closed 4 weeks ago

JimMadge commented 10 months ago

Describe the bug When using the example keybow2040 configuration, current CircuitPython and current kmk library, button presses have no effect. After a while CircuitPython soft resets.

Disabling the RGB, or forcing an exit from the _process_timeouts loop is a workaround.

To Reproduce Steps to reproduce the behavior:

Expected behavior Keyboard responds to button presses.

Debug output

75001 kmk.keyboard: Unexpected error: cleaning up

Code stopped by auto-reload. Reloading soon.
soft reboot

repeatedly (different numbers)

Additional context

So, I suspect that calls to update the RGB LEDs are filling the task queue and preventing the HID functions in go from being called.

I'm not very familiar with async but I'm happy to help debug if you have any suggestions.

JeffreyStocker commented 10 months ago

I am having the same issue. I just started to play with RGB module and noticed that the keypad would become unresponsive to key presses when one of the more intricate RGB sequences were selected.

Ones tried

KC.RGB_MODE_PLAIN and KC.RGB_MODE_BREATHE don't appear to be affected.

One workaround is to throttle the RGB updates via the refresh_rate setting

#from [RGB Document](https://github.com/KMKfw/kmk_firmware/blob/master/docs/en/rgb.md)
from kmk.extensions.rgb import AnimationModes
rgb = RGB(pixel_pin=rgb_pixel_pin,
        num_pixels=27
        val_limit=100,
        hue_default=0,
        sat_default=100,
        rgb_order=(1, 0, 2),  # GRB WS2812
        val_default=100,
        hue_step=5,
        sat_step=5,
        val_step=5,
        animation_speed=1,
        breathe_center=1,  # 1.0-2.7
        knight_effect_length=3,
        animation_mode=AnimationModes.STATIC,
        reverse_animation=False,
        refresh_rate=60,  # <---------- set to a lower setting
        )

On mine I found that 35 appears to be the upper limit, but key press start to get unreliable. Lower is better but it makes the RGB update slowly

Hardware: Adafruit MacroPad RP2040

JimMadge commented 10 months ago

862 is an improvement but my board still isn't behaving quite as expected.

Keypress are now sometimes registered. However if it is, characters are repeated many times as if they HID signal is being repeatedly sent or the key is being held down. (e.g. "aaaaaaaaaa").

Thanks @xs5871 for proposing a fix so quickly :tada:. I commented here because I didn't want to hold up that PR. It is an improvement but I don't think it fixes this problem completely. Happy to help continue testing branches or with anything debugging I can do.

xs5871 commented 10 months ago

You're right that #862 only fixes the infinite loop.

Keypress are now sometimes registered. However if it is, characters are repeated many times as if they HID signal is being repeatedly sent or the key is being held down. (e.g. "aaaaaaaaaa").

That is the "expected" behaviour and unintuitively not a bug. Compounding factors are:

JimMadge commented 10 months ago

That makes sense.

Would a sensible thing be to change the keybow2040 example to use a less expensive RGB animation?

Happy to play around with that to find something that makes the keyboard more responsive.

JeffreyStocker commented 10 months ago

Funny you should say that. I was playing around with the RGB and found a way to increase the RGB update efficiency.

864

I found that this allowed me to go from refresh_rate of 35ish to over 200 before issues started to appear YMMV since I'm using a tiny macropad

JimMadge commented 10 months ago

@JeffreyStocker #864 Doesn't seem to make much difference on my Keybow2040. It still isn't really usable with the more intense RGB animations.

JeffreyStocker commented 9 months ago

@JimMadge Check your refresh_rate that your passing into the RGB module. See what happens when you lower the refresh_rate

One thing I'm seeing, looking at the spec page of the Keybow2040 is that it is using a LED matrix driver, (maybe a custom code to control it?)

The way Neopixels and many strip LED work is by a message to the first LED and then that LED pass the message to the next LED. So I'm the code is gaining a lot of performance I'm having only 1 message be sent per update cycle, rather than 12 messages (I have 12 LEDS). I'm not sure how the Keybow2040 is sending LED updates. Could you show what your RGB initiation variables and what libraries you are importing?

Gadgetoid commented 1 month ago

I looked into this and, indeed, there was a lot of room for optimisation. I threw most of the stack away and started a new PixelBuf wrapper from more or less scratch using two of the is31fl3731 frames as a front/back buffer setup.

This approach plays PixelBuf to its strengths, and then does a final re-arrange of the data into display-native (read: horribly mangled for routing convenience rather than making my life easy) format for writing.

I played with batched writes vs just chucking the whole 144 byte buffer over in a single transaction and the difference is negligible.

Save this to your device as keybow_2040_rgb.py:

import board
from adafruit_bus_device.i2c_device import I2CDevice
from adafruit_pixelbuf import PixelBuf
from micropython import const
import time

_MODE_REGISTER = const(0x00)
_FRAME_REGISTER = const(0x01)
_SHUTDOWN_REGISTER = const(0x0A)
_CONFIG_BANK = const(0x0B)
_BANK_ADDRESS = const(0xFD)

_ENABLE_OFFSET = const(0x00)
_COLOR_OFFSET = const(0x24)

class Keybow2040Leds(PixelBuf):
    """Supports the Pimoroni Keybow 2040 with 4x4 matrix of RGB LEDs"""

    width = 16
    height = 3

    def __init__(
            self,
            size: int = 16
    ):
        self.i2c_device = I2CDevice(board.I2C(), 0x74)
        self.out_buffer = bytearray(144)
        self._pixels = size
        self._frame = 0
        super().__init__(size, byteorder='RGB')

        with self.i2c_device as i2c:
            i2c.write(bytes([_BANK_ADDRESS, _CONFIG_BANK]))
            i2c.write(bytes([0x00] * 14))  # Clear Mode, Frame, AutoPlay, Blink, Sync, Breath, SD, etc

            for frame in range(2):
                i2c.write(bytes([_BANK_ADDRESS, frame]))  # Bank/Frame
                i2c.write(bytes([_ENABLE_OFFSET] + [0xff] * 18))   # Enable all LEDs
                i2c.write(bytes([_COLOR_OFFSET] + [0x00] * 144))  # Initialise all to 0

            i2c.write(bytes([_BANK_ADDRESS, _CONFIG_BANK]))
            i2c.write(bytes([_SHUTDOWN_REGISTER, 0x01]))  # 0 == shutdown, 1 == normal

        self.used_leds = [
                        [17, 18],
                        [25, 26],
                        [32, 33],
                        [40, 41],
                        [64, 65, 66, 67],
                        [72, 73, 74, 75],
                        [80, 81, 82, 83],
                        [88, 89, 90, 91],
                        [96, 97, 98, 99],
                        [104, 105, 106, 107],
                        [112, 113, 114, 115],
                        [120, 121, 122, 123],
                        [128, 129, 130, 131],
                        [136, 137, 138, 139]]

    def _transmit(self, buffer):
        # t_start = time.monotonic()
        for x in range(self._pixels):
            r, g, b = Keybow2040Leds.pixel_addr(x)
            self.out_buffer[r] = buffer[x * 3 + 0]
            self.out_buffer[g] = buffer[x * 3 + 1]
            self.out_buffer[b] = buffer[x * 3 + 2]

        with self.i2c_device as i2c:
            i2c.write(bytes([_BANK_ADDRESS, self._frame]))

            # Lazy non-batched write, probably fast enough?
            # i2c.write(bytes([_COLOR_OFFSET]) + self.out_buffer)

            # We only actually use 16 * 3 = 48 LEDs out of the 144 total
            # They're kind of awkwardly spread around, but if we batch up
            # the writes it gets the update time from ~0.016s to ~0.012s
            for group in self.used_leds:
                offset = group[0]
                count = len(group)
                i2c.write(bytes([_COLOR_OFFSET + offset]) + self.out_buffer[offset:offset + count])

            i2c.write(bytes([_BANK_ADDRESS, _CONFIG_BANK]))  # Config bank
            i2c.write(bytes([_FRAME_REGISTER, self._frame])) # Frame register

        self._frame = not self._frame
        # t_end = time.monotonic()
        # print(t_end - t_start)

    @staticmethod
    def pixel_addr(x):
        return [
            (120, 88, 104),  # 0, 0
            (136, 40, 72),  # 1, 0
            (112, 80, 96),  # 2, 0
            (128, 32, 64),  # 3, 0
            (121, 89, 105),  # 0, 1
            (137, 41, 73),  # 1, 1
            (113, 81, 97),  # 2, 1
            (129, 33, 65),  # 3, 1
            (122, 90, 106),  # 0, 2
            (138, 25, 74),  # 1, 2
            (114, 82, 98),  # 2, 2
            (130, 17, 66),  # 3, 2
            (123, 91, 107),  # 0, 3
            (139, 26, 75),  # 1, 3
            (115, 83, 99),  # 2, 3
            (131, 18, 67),  # 3, 3
        ][x]

Then change from is31fl3731_pixelbuf import Keybow2040Leds to from keybow_2040_rgb import Keybow2040Leds.

This speeds up the LED update cycle from 60-80ms (12-16 FPS) to 12-16ms (~60-80fps).

:tada:

JimMadge commented 4 weeks ago

@Gadgetoid I've tested the changes on your branch on my Keybow 2040.

It is a huge improvement. The RGB breathing effect cycles at a good pace, and the keys are responsive with no noticeable lag.

Gadgetoid commented 4 weeks ago

@JimMadge woohoo! Thank you, appreciated.

Looks like it might be possible to go faster though we’re beyond the realm of diminishing returns and into the forbidden garden of intellectual curiosity at this point 🤣