search

DJ-TechTools / Midi_Fighter_Twister_Open_Source

95 stars 25 forks source link

Colour override is incorrect for value 126 #9

Open dozius opened 3 years ago

dozius commented 3 years ago

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.

https://github.com/DJ-TechTools/Midi_Fighter_Twister_Open_Source/blob/e7f6c8fd08c962a936e87887ac80428bbdb4652c/src/encoders.c#L1428

dozius commented 3 years ago

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 commented 1 year ago

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: https://github.com/DJ-TechTools/Midi_Fighter_Twister_Open_Source/blob/e7f6c8fd08c962a936e87887ac80428bbdb4652c/src/encoders.c#L1016-L1018

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: https://github.com/DJ-TechTools/Midi_Fighter_Twister_Open_Source/blob/e7f6c8fd08c962a936e87887ac80428bbdb4652c/src/encoders.c#L1056

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.