openrgb: Make sure OpenRGB effect numbers are correct

FabianMaurerMathema commented 2 years ago

This PR reworks the way effect-IDs are handled when communicating to OpenRGB.

@Kasper24 Since you made the original code, would you mind giving your input here? I think this approach is less prone to breakages in the future.

Description

Currently, there is a hardcoded array of effect indices in openrgb:

openrgb_rgb_matrix_effects_indexes[]

When a specific effects is enabled, a hardcoded number for it is inserted into this array. But since effects are conditionally enabled in the firmware, the enum value for each effect changes depending on which effects are enabled. This leads to the unpleasant effect of choosing an effect in OpenRGB, but the keyboard showing a completely unrelated effect.

This PR introduces a robust effect map, making us independent of the actual enum number. The only thing "hardcoded" here is the index of the array - this corresponds directly to the OpenRGB effect number:

// Index = openrgb enum value
openrgb_rgb_matrix_effects_indexes[] = {
    RGB_MATRIX_OPENRGB_DIRECT, // Index 0 is always the qmk value for direct mode
    RGB_MATRIX_SOLID_COLOR     // Index 1 is always the qmk value for solid color mode

// Index 2 is always the qmk value for matrix alphas mods (0 if not available in this firmware version)
#ifdef ENABLE_RGB_MATRIX_ALPHAS_MODS
    RGB_MATRIX_ALPHAS_MODS,
#else
    0,
#endif

Types of Changes

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

Issues Fixed or Closed by This PR

Checklist

[x] My code follows the code style of this project: C, Python
[x] 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.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

ghost commented 2 years ago

AFAIU, this is wrong, because the effect indices that OpenRGB supplies with SET_MODE command differ based on the enabled modes (they should correspond to QMK RGB_MATRIX enum values). (see https://gitlab.com/CalcProgrammer1/OpenRGB/-/blob/master/Controllers/QMKOpenRGBController/RGBController_QMKOpenRGBRevD.cpp#L25)

Let's say you have Static, Alpha Mod, Gradient Up Down enabled. They respectively get indices 1, 2 and 3. But if you disable Alpha Mod (e.g. unset ENABLE_RGB_MATRIX_ALPHAS_MODS), Static will have index 1 and Gradient Up Down will have index 2.

Furthermore, Direct isn't index 0, but index for the last enabled effect + 1 (value of RGB_MATRIX_OPENRGB_DIRECT).

The implementation for openrgb_get_enabled_modes seems also wrong, because it should return OpenRGB Modes enum values (defined at https://gitlab.com/CalcProgrammer1/OpenRGB/-/blob/master/Controllers/QMKOpenRGBController/QMKOpenRGBController.h#L39), not QMK's RGB_MATRIX enum values (which differ based on the enabled effects).

Summary: The implementation for openrgb_set_mode was correct before this PR, openrgb_get_enabled_modes returned values off by one (fixed by #284)

FabianMaurerMathema commented 2 years ago

As far as I understand it, we were also tripped up by ENABLE_RGB_MATRIX_PIXEL_RAIN / ENABLE_RGB_MATRIX_PIXEL_FRACTAL and the off by one error you mentioned. Not 100% sure why, but with those effects enabled it breaks in funny ways. With that removed though, it seems to work reliably now, so this can be closed. Thanks for the review!

SonixQMK / qmk_firmware