[Core] Suggestion: add double buffer to LED drivers

[Xelus22]

Description

This is to generate discussion of this change. NOT a proper PR.

The Problem:

When an indicator is on/updated, the driver update flag is set dirty. Then on the next cycle, the animation will render and see the colour is different and set the dirty flag. Which then if the indicator LED is still on, it will set to the correct colour and set the dirty flag again. Thus even between cycles when an indicator is on, it will always be set dirty and flushed.
This is the reason for such a large performance hit when an indicator is on compared to when it is just the animation

The Fix:

Create a second buffer that holds the values of the last render/pwm buffer update and compare against that since we only use a single buffer.

Testing:

Tested on Pachi RGB Rev2 STM32L4 + IS31FL3741

Effect	Old	New	Difference %
Solid	8127	8088	-0.455%
Cycle All	~3700	~5600	+51.4%
Cycle Spiral	2708	2686	-0.849%
Solid + indicators	2708	8088	+198%
Cycle All + indicators	2708	~5600	+106.8%
Cycle Spiral + indicators	2708	2686	-0.849%

Advantages:

• Indicators no longer drag down the scan rate when on mainly static animations

Disadvantages:

• doubles the buffer size
• buffersize is already huge (351 bytes), new size would be (351 * 2 = 702 bytes) (at one driver)
• MCUs with much less RAM than stm32 or other will not be able to support this

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

• Fix indicators dragging scan rate down

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[dexter93]

Can confirm this works. sn32 led matrix does something similar.

8k of RAM is apparently enough for this. Is there a more poverty spec chip we should worry about?
-1% loss overall for 200% gain in indicators is not a bad tradeoff

[sigprof]

sn32 led matrix does something similar.

No, in the sn32 case that memcpy() call is mostly equivalent to is31fl3741_write_pwm_buffer() — it transfers the LED values from the temporary buffer to the LED controller (which in the SN32 case is just another part of the firmware code). The difference is that in the SN32 case the transfer is very fast and does not need to be optimized further, but for a chip connected over I2C the transfer consumes some significant time, therefore it makes sense to avoid doing the transfer if it's not really needed (in fact there were some attempts to track the dirty range, which could help if only a small mostly contiguous subset of LEDs needs to be updated often).

[drashna]

MCUs with much less RAM than stm32 or other will not be able to support this

You mean AVR and low end ARM controllers. Like this really hurts the 2.5kB of SRAM that atmega32u4 has.

Would it be worth having this as an option, eg RGB_MATRIX_ENABLE_DOUBLE_BUFFER or some such?

[Xelus22]

Would it be worth having this as an option, eg RGB_MATRIX_ENABLE_DOUBLE_BUFFER or some such?

Yes I was thinking something to have his option as an opt-in would definitely be a good way for compatibility.
Also a good opt-in with wear levelling EEPROM, QP/OLED with specific MCUs may not have enough RAM.

[tzarc]

The rgb_matrix code is ugly enough as it is. At some point we're going to have to stop catering for such constrained hardware.

[Gondolindrim]

RGB animations support on less powerful chips will always be a problem unless we are willing to write really optimized code. No one here is looking to make assembly drivers with memory dynamic allocation support. I wouldn't say we should stop catering to constrained firmware, I'd say that QMK has gone past these platforms with its capabilities.

Furthermore, there already are cases of features that were simply not considered for such chips. RGB animations are just a gray-area because the less powerful guys support it and can do it kind-of ok given certain aspects (most importantly having a small enough number of LEDs and trying not to do crazy animations). However there are tacit examples where support for these lesse powerful chips are not discussed for obvious reasons -- who is going to complain no colored OLED support for 32U4?

EDIT: because it's really hot and my gray matter has turned into gray mush, I'll TLDR: I think we shouldn't not consider a feature just because certain units won't support it. This has been done in the past, albeit tacitly, and implementing things like complex RGBs in constrained hardware is a task I don't see anyone wanting to tackle.

[Xelus22]

I think I'll go ahead with this after #22099 , #22071 , #21944, #22200 if theres nothing else?
I will also include all the other drivers for LED matrix and WS2812?
I could just gate it behind #ifdef DRIVER_NAME_USE_DOUBLE_BUFFER

eg. #define IS31FL3741_USE_DOUBLE_BUFFER

[Xelus22]

@zvecr Do you think this double buffer should be implemented in the subsystem(s) that use these drivers? Rather than here in the driver to keep it simple/uncoupled?

[drashna]

@zvecr Do you think this double buffer should be implemented in the subsystem(s) that use these drivers? Rather than here in the driver to keep it simple/uncoupled?

It might be simpler to do so, long term, as that would be less maintenance needed for each driver (or future driver).

[Xelus22]

#23625

Closing in favour of this PR