search

KMKfw / kmk_firmware

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

increased RBG efficiency #864

Closed JeffreyStocker closed 1 year ago

JeffreyStocker commented 1 year ago

Increased RGB efficiency by sending LED updates as a single command rather than updating individual updates Separates LED auto write and manual LED updates into separate properties Changed default auto write property to False to auto use more efficient LED update method

xs5871 commented 1 year ago

I don't see how this improves anything. Can you go more into detail where the current code does "individual updates"?

JeffreyStocker commented 1 year ago

Specifically, it is the property auto_write for NeoPixels and PixelBuf. https://github.com/KMKfw/kmk_firmware/blob/642dbff31ba9913fcf9d69a0671359fe1a3f21d1/kmk/extensions/rgb.py#L209C5-L219C1

auto_write set to True causes the board to send updates to the leds whenever any of the LED RGB values are updated. This means if the code is changing multiple LEDs in a animation step (such as RGB_MODE_RAINBOW or RGB_MODE_SWIRL) the code will send out a ton of updates which will clog up system resources. By turning auto_write to False, making updates, and the showing the update, the code will only send a single update to the LEDs.

For example. as I stated in #859, when I implemented this on my Adafruit Macro pad (which only has 12 LEDs) I was limited to about 30 updates/second, before I started to get key press issues. By implementing my code, i was now up to 300ish. I would expect this to be even more noticeable performance gain for full keyboards with more LEDs.

xs5871 commented 1 year ago

So, as it turns out we did not set the auto_write of the PixelBuffers correctly. Take a look at #874. That should have the same effect as your suggestion, but without any refactoring. I would greatly appreciate your feedback on that PR.

JeffreyStocker commented 1 year ago

No, you did use auto_write correctly. But the module's code confuses manual updates with RGB updating on writes. It is a good idea to keep some sort of manual update in the code in case someone makes more complicated RGB sequences.

Also just deleting that property left in a ton of references to that property which has broken at least the breathing animation, probably more which I didn't test. The left over code also still caused too many updates, which means there was not any performance gains.

xs5871 commented 1 year ago

No, you did use auto_write correctly. But the module's code confuses manual updates with RGB updating on writes. It is a good idea to keep some sort of manual update in the code in case someone makes more complicated RGB sequences.

disable_auto_write was probably intended to control auto_write (i.e. "manual updates") by the original author.

  1. It did that only if the extension is initialized with GPIO pins, but not for a list of PixelBuffers.
  2. Regardless of what the user set it to, it will be changed in the swirl and knight animations. Both these things can result in inconsistent or at least unexpected behavior.

Also just deleting that property left in a ton of references to that property which has broken at least the breathing animation, probably more which I didn't test. The left over code also still caused too many updates, which means there was not any performance gains.

Would you mind pointing out those dangling references? At what point are there still superflous updates? I did check everything on hardware and can't reproduce what you found.

kdb424 commented 1 year ago

Auto writes were, in fact, disabled by default intentionally. If you look at animations, you'll see that you want to write out the entire animation that's to be processed, and force an update so that the animation shows up all at the same time. It was quite unintuitive to have to turn it off every time you wanted to batch things. I personally tried (on much weaker MCU's than we have these days) to see if there was a substantial performance difference in visuals or keyboard feel as I didn't have the tooling then to profile, and came to the conclusion that auto writes were, at least at that time, not more efficient, so I optimized for "set everything, then tell it to go all at once. Different users may have a different use case, but explaining the "how I got here" if that helps.

xs5871 commented 1 year ago

In order to go forwards I'll merge #874 and close this PR, leaving the initial issue open. It seems there're still some unaddressed performance issues.

JeffreyStocker commented 1 year ago

I finally had some time to dig in a bit and I think I see what is happening

1) First you are right, there are no dangling bits of code. When I read your changes I thought that you had completely removed disable_auto_write from init, and just hadn't removed all the references,

2) The code changes definitely helped, but as you said there are some more performance to be had. Mainly I'm seeing some of more complicated animation updating the pixels twice, much better than the per write than before. I'll poke at it and then do a separate merge

3) It looks like the Breathing problem I am having is a separate problem that has to do with #865 and was confusing the issue. I'll poke at that later as this a separate issue.

Because of the way disable_auto_write is being using use to control pixel updates rather than auto_writes on PixBufs, I think it should be renamed, but it isn't a big issue.