Fix zero condition of reactive runners that will suspend RGB animation #12710

filterpaper · 2021-04-27T01:48:00Z

Description

If user keyboard's RGB speed setting (rgb_matrix_config.speed) is at zero, key press animation will remain static. Reactive runners will keep one key or entire keyboard lit.

This change avoid both conditions with a minimum value to the tick variable.

Types of Changes

Checklist

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

jhanschoo · 2021-04-27T02:00:15Z

I don't really agree that this should be the behavior. Note that it isn't overflowing; the ticks are just being multiplied by zero. For effects that light up the keyboard at the instantaneous moment of a keypress, this will indeed result in lighting up the entire keyboard.

That is, at speed=0, only the zeroth frame of the reactive animation will play (a little more complicated for mutikey effects, but essentially the same), and this persists for around 65 s or max keypresses remembered is reached. If this is not the behavior of an animation, the animation is what should be fixed.

filterpaper · 2021-04-27T02:16:37Z

I don't really agree that this should be the behavior. Note that it isn't overflowing; the ticks are just being multiplied by zero. For effects that light up the keyboard at the instantaneous moment of a keypress, this will indeed result in lighting up the entire keyboard.

I do see your point, but I don't believe that's the spirit of the reactive light intention. That said, @XScorpion2 is the original coder—will let him/her comment if "zero" RGB reactive speed is meant to light up the entire board.

jhanschoo · 2021-04-27T02:53:11Z

I think that with respect to spirit, documentation on the speed variable might also serve well: mention that speed 0 means the animation will literally freeze at the zeroth frame, 1 is the slowest speed, and 255 is fastest.

If this PR is accepted, there should also be a comment on why 1 is added to the divisor in this particular runner and not the other runners.

Note that "lighting up the board" is a consequence of this particular runner: for instance, for effect_runner_i.h, or effect_runner_reactive_splash.h, there is value for zero speed, for the result is the zeroth frame effect is applied to the last N keypresses, and a solid effect can be simulated (e.g. flickering firefly effect whose maximum value decays with time, speed used to control rate of decay)

XScorpion2 · 2021-04-27T18:03:50Z

So, this may sound like a stupid reason as to why I allowed 0:
All the step value defines default to 16 currently, which ensures that when you reach max, you hit 255 exactly, and reach min, you hit 0 exactly. This way when you hit the extents and reverse, you always can get to the same value as before, without having to reach the other extent to reset your scale. When you limit the min to 1, this causes a shift of +1 to occur once you hit that, then increase the value again.

So your value changes would look something like:
32 -> 16 -> 1 -> 17 -> 33

Instead of today where it looks like this:
32 -> 16 -> 0 -> 16 -> 32

(Most of my boards I've configured to use 4 or 8 for the step values, and in some cases even 1, additionally I use OLEDs that display the raw values for each of the rgb settings on screen)

As far as this change goes, since this modifies the time value at the effect level, this prevents the value shifting issue mentioned above, but then you end up in an arguably just as an odd state that when you hit 0 speed, the animation is still in fact running. Ultimately I see neither solution being right or wrong, just which one QMK wants as a whole. So either or change works just fine for me, no real opinion on what the correct behavior is

filterpaper · 2021-04-27T21:01:15Z

Thanks for the input. In this case, setting the minimum value at RGB_MATRIX_SPD_STEP will cover both cases. All bets are off for users that avoid powers of two.

filterpaper · 2021-04-27T23:08:46Z

All the step value defines default to 16 currently, which ensures that when you reach max, you hit 255 exactly, and reach min, you hit 0 exactly.

@XScorpion2 I did a quick console test to verify the values of rgb_matrix_config.speed. It will max at 255. There after default RGB_SPD will decrease its value by 16, counting down in this manner:
255 - 239 - 223 ... 31 - 15 - 0 - 16 - 32 ...

They don't seem to align at powers of 2 when decrementing from max 255.

XScorpion2 · 2021-04-27T23:14:46Z

All the step value defines default to 16 currently, which ensures that when you reach max, you hit 255 exactly, and reach min, you hit 0 exactly.

@XScorpion2 I did a quick console test to verify the values of rgb_matrix_config.speed. It will max at 255. There after default RGB_SPD will decrease its value by 16, counting down in this manner:
255 - 239 - 223 ... 31 - 15 - 0 - 16 - 32 ...

They don't seem to align at powers of 2 when decrementing from max 255.

I may be off on my memory, but that was the original intent at any rate when I implemented it. Ah well, not really all that important as it's more about what people would prefer either way.

jhanschoo · 2021-04-28T01:47:07Z

I think I now agree with @filterpaper to add one, and it should be implemented on all the runners. Animation writers that want the weird behavior at speed=0 should just subtract 1 themselves.

stale · 2021-06-23T09:32:00Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

tzarc · 2021-06-23T09:42:29Z

~~Would it not be better to have these as max(speed, 1)?~~

I don't think it really matters, everything else will be relatively consistent.

#12710) * Avoid zero or overflow from user's rgb_matrix_config.speed * Avoid zero tick for reactive splash. * Avoid zero time for animation runner. Co-authored-by: filterpaper <filterpaper@localhost>

qmk#12710) * Avoid zero or overflow from user's rgb_matrix_config.speed * Avoid zero tick for reactive splash. * Avoid zero time for animation runner. Co-authored-by: filterpaper <filterpaper@localhost>

Avoid zero or overflow from user's rgb_matrix_config.speed

00c4381

github-actions bot added the core label Apr 27, 2021

Avoid zero tick for reactive splash.

3cc8851

Avoid zero time for animation runner.

40ab438

filterpaper changed the title ~~Fix all LEDs lit condition with reactive runner for RGB effect~~ Fix zero condition of reactive runners will suspend RGB animation Apr 27, 2021

filterpaper changed the title ~~Fix zero condition of reactive runners will suspend RGB animation~~ Fix zero condition of reactive runners that will suspend RGB animation Apr 27, 2021

drashna approved these changes May 9, 2021

View reviewed changes

drashna requested a review from a team May 9, 2021 03:24

stale bot added the awaiting changes label Jun 23, 2021

tzarc merged commit a913db6 into qmk:master Jun 23, 2021

filterpaper deleted the rgb_reactive_fix branch June 23, 2021 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix zero condition of reactive runners that will suspend RGB animation #12710

Fix zero condition of reactive runners that will suspend RGB animation #12710

filterpaper commented Apr 27, 2021 •

edited

Loading

jhanschoo commented Apr 27, 2021 •

edited

Loading

filterpaper commented Apr 27, 2021

jhanschoo commented Apr 27, 2021 •

edited

Loading

XScorpion2 commented Apr 27, 2021 •

edited

Loading

filterpaper commented Apr 27, 2021

filterpaper commented Apr 27, 2021

XScorpion2 commented Apr 27, 2021

jhanschoo commented Apr 28, 2021

stale bot commented Jun 23, 2021

tzarc commented Jun 23, 2021 •

edited

Loading

Fix zero condition of reactive runners that will suspend RGB animation #12710

Fix zero condition of reactive runners that will suspend RGB animation #12710

Conversation

filterpaper commented Apr 27, 2021 • edited Loading

Description

Types of Changes

Checklist

jhanschoo commented Apr 27, 2021 • edited Loading

filterpaper commented Apr 27, 2021

jhanschoo commented Apr 27, 2021 • edited Loading

XScorpion2 commented Apr 27, 2021 • edited Loading

filterpaper commented Apr 27, 2021

filterpaper commented Apr 27, 2021

XScorpion2 commented Apr 27, 2021

jhanschoo commented Apr 28, 2021

stale bot commented Jun 23, 2021

tzarc commented Jun 23, 2021 • edited Loading

filterpaper commented Apr 27, 2021 •

edited

Loading

jhanschoo commented Apr 27, 2021 •

edited

Loading

jhanschoo commented Apr 27, 2021 •

edited

Loading

XScorpion2 commented Apr 27, 2021 •

edited

Loading

tzarc commented Jun 23, 2021 •

edited

Loading