Merge rfm/2024_update into the correct branch

[rfmerrill]

The previous PR #16 was intended to have its parent branch changed before merging. I assume you actually want to merge this into master.

[jkbelcher]

Hi. Stoked to see someone working on this. Regarding commit 60bf6e4 "change animation channels", wouldn't it be better to make this correction in the manual? If you rearrange the midi channels between firmware versions, seems like it will break everyone's midi maps.

[rfmerrill]

Hi. Stoked to see someone working on this. Regarding commit 60bf6e4 "change animation channels", wouldn't it be better to make this correction in the manual? If you rearrange the midi channels between firmware versions, seems like it will break everyone's midi maps.

The two animation channels were not actually split before--one channel controlled both sets of animations. In the process of splitting them, I also made sure the channels were assigned as the manual described. Splitting them will break some people's setups but it would do so regardless of whether this change were made or not.

[jkbelcher]

Now that you mention it, I do remember some funky behavior around the animations.

Looking at the code now, there are:

• Two sets of non-overlapping animation values: switch/RGB animations and encoder/indicator animations
• Two [2D] arrays storing animation values: switch_animation_buffer and encoder_animation_buffer.

Animation values are accepted on both midi channels 2 and 5 and are processed by encoders.process_element_midi(). As of now it stores inputs from channel 2 to encoder_animation_buffer and inputs from channel 5 to switch_animation_buffer. With your update you'll be ignoring switch animations in the encoder_animation_buffer and ignoring encoder animations in the switch_animation_buffer.

I do think you've improved the code by removing the animation_buffer_conflict_exists() tangle. But in addition how about also doing this:

Check animation_is_encoder_indicator() and animation_is_switch_rgb() when the input arrives on either channel in process_element_midi(), and store all switch animations to switch_animation_buffer and all encoder animations to encoder_animation_buffer. Regardless of whether it comes in on channel 2 or 5.

Seems like this would eliminate the problem where people were stepping on themselves by sending both animation types to the same channel and overwriting their buffer value (pretty sure this bit me early on). And it also won't break anyone's legacy maps... rather some people's maps that were broken will now start working.

Note your animation buffer values will now be sanitized so you won't need to call animation_is_encoder_indicator() from update_encoder_display().

[rfmerrill]

@jkbelcher The actual functionality is not my call (I just wrote the code) but I can see a few potential issues with this approach:

1. Having two underlying states pertaining to the same CC goes against what people usually expect. If you write 64 and then 127 to the same CC, you normally expect the 127 value to replace, rather than combine with the 64, I would think.
2. There is one salient shared value between the two -- 0. If you write 0 to the channel it would have to disable both animation types, which means there would not be a way to disable just the RGB or just the ring animations.
3. It's been a while since I wrote and tested this so it's not fresh in my mind, but some people's existing setups may be depending on a ring animation canceling the RGB animation or vice-versa. This is broken with this change, but it would be broken in your proposed mechanism as well.

[jkbelcher]

@rfmerrill Good point, the zero use case sinks it. Thanks for engaging.