Implement new QMK protocol version that adds tracking of keyboard-internal LED IDs

Neneyarequested to merge
Irisvielia/OpenRGB:qmk_protocol_fix into master

The current revision B?C? has an issue, which only becomes apparent when in use with specific QMK keyboard implementations, in which the order of LEDs is not as expected. What the current protocol expects is that all Underglow LEDs are always at the end of the internal matrix, as in:

  • Key LED
  • Key LED
  • Key LED

...

  • Underglow LED
  • Underglow LED

However, some keyboards (like for example the GMMK PRO) have the LEDs implemented according to the order they are connected in on the PCB, so the RGB Matrix order looks like:

  • Key LED
  • Underglow LED
  • Underglow LED
  • Key LED
  • Key LED
  • Underglow LED

...

I initially just changed the order of the internal LEDs, however jath03 on discord mentioned, that this kind of thing should if possible be fixed in OpenRGB, and not in QMK, as the implementation is supposed to be as hassle-free for QMK users as possible.

The issue now is:

When the Client fetches LED Infos from the OpenRGB procotol in QMK, it pushes the flag, coordinate and color data into 3 different vectors in the order they are received in, but does not to the same for the led_names vector. There, all key LED's get pushed in first, and only later in all the Underglow LED's are pushed at the end of the led_names vector. This works as long as the RGB Matrix LEDs are in the expected order. If they are not, this creates a mismatch between the LED names and their actual data, making the LED names in the OpenRGB Client be out of order and mapping LEDs to the wrong name.

At first I only fixed the order in which the LEDs were named, which fixed the naming to LED issue, but made it so that the Underglow LEDs were now also listed in OpenRGB as not at the end of the LED list. Apparently, many plugins and programs (e.g. the openrgb effects plugin, artemis) actually require any Underglow LEDs to be after any Key LEDs, so this solution fixed one but created another problem.

So to solve this, I implemented this QMK protocol upgrade. It is just a small update, however, what it does is:

Separate Underglow from Key LEDs in the initial GetLEDInfo function to make sure that all 4 vectors match. It pushes Underglow LEDs and their data and names onto a separate underglow_* vector, and after getting all LEDs, appends this to the end of the led data vector. This makes sure that Underglow LEDs are always at the end of the LED list, but in all vectors.

This alone still left us with the initial problem of that the order of LEDs in OpenRGB was different to the order of LEDs in QMK, so many LEDs were not mapped to the correct name.

So additionally to this, I added a new vector in the controller, called led_values. This works the same as the other vectors, but what it saves is the QMK internal ID of any LED it receives. This makes sure that whenever OpenRGB tries to update any LED in QMK, it has the correct LED index available, even if the order of LEDs in OpenRGB and QMK is not the same.

Now the last issue is that the current OpenRGB-QMK protocol actually does not explicitly pass every LED ID for multi direct updates. It passes the first index, as well as how many LEDs to update, and then on the QMK side starts iterating over start_led + leds_to_update until it has all LEDs done. However, this again assumes that the order is the same, which it not always is.

To fix this, I changed the protocol to always include the ID of any LED it wants to update, but for this change the number of addresses are required per LED increased from 3 (r, g and b values) to 4 (id was added). That is why this cannot just be a change to Protocol Revision B, but needs to be a new revision. This sadly also lowers the max number of LEDs per update to 15 (previous 20), but I've seen no issues coming from that on my keyboard.

The changes to QMK's OpenRGB module can be found on my GitHub qmk fork https://github.com/Chloe-ko/qmk_firmware

I did not create a merge request into Kasper24's repo yet due to 2 reasons:

  1. I am waiting for feedback on the changes from one of the devs that were directly involved in the QMK implementation in discord
  2. My fork is updated to upstream qmk, while Kasper24's fork is quite outdated. Merging this would merge over 2000 commits from upstream and I am not sure if that is desired. I could if wanted make a fork of Kasper24's repo, and implement the changes there, but unless needed I had no reason to implement this on an outdated fork.

Merge request reports