Colour override is incorrect for value 126

[dozius]

Sending a colour override value of 126 results in the LED being set to the active colour state instead of the override colour.

According to the manual, this is how colour override works.

0 = inactive colour state
127 = active colour state
1 to 126 = override colours

This line appears to be the source of the issue. It should be < 127 or <= 126 in order to operate as described in the manual.

Midi_Fighter_Twister_Open_Source/src/encoders.c

Line 1428 in e7f6c8f

} else if (value >0 && value < 126) { // Exclude 126 as we don't allow user to set color to white

[dozius]

In a way, yes. It tells us whoever wrote this code didn't understand that index 127 is white and not index 126 when they wrote it.

The colour indexes are clearly defined in colorMap.c. The expected behaviour is clearly defined in the manual. I see no reason to think this is intended rather than a mistake.

[doodlebro]

I think this was intentional, with justification relating to the shift page function and active state of the switches therein.

In encoders.c, run_shift_mode's docstring states that the white RGB color is reserved for showing active state of the shift page switch:

Midi_Fighter_Twister_Open_Source/src/encoders.c

Lines 1016 to 1018 in e7f6c8f

	* Shift mode is an alternate operation mode which is accessed by the side buttons
	* In shift mode the encoders do not do anything, only the switches have effect.
	* All 11 position LEDs are turned on and the RGB set to white if active.

So, this seems like an intentional implementation detail, however there is a bug in the latest firmware since the RGB LED stays unlit when shift page switches are pressed:

Midi_Fighter_Twister_Open_Source/src/encoders.c

Line 1056 in e7f6c8f

set_encoder_rgb(idx, 0);

The fix is easy, of course. Just change the 0 to 127. There are a lot of little QoL bugs like this I am catching, I plan to release fixes for them publicly, either as a PR to this repo or a fork. I know there are still license details to work out.

One example, I think it is a bug that the shift pages are not banked. Unless there are space limitations on the microcontroller, all functions should be banked, or this should be a user-configurable setting at least. This device has a ton of potential (and is already quite powerful!), it just needs that last bit of polish to really make it sing.