Support changing other properties besides brightness #2
Conversation
|
If you're comfortable with rebase/squash, would you mind rebasing and squashing these changes so we get slightly cleaner git history? At the same time, could you rewrite the commit message to push the text from the pull request into it? Pull Request history is...relatively ephemeral, but commit messages stay around with the code and it's quite useful to know what the intent of each commit was. Thanks! |
|
No worries, can do.
…On Mar 9, 2018 1:01 PM, "Jesse Vincent" ***@***.***> wrote:
If you're comfortable with rebase/squash, would you mind rebasing and
squashing these changes so we get slightly cleaner git history?
At the same time, could you rewrite the commit message to push the text
from the pull request into it? Pull Request history is...relatively
ephemeral, but commit messages stay around with the code and it's quite
useful to know what the intent of each commit was.
Thanks!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AdiDXJyzLTuuO4wge1v1PneD4F4phVEWks5tct92gaJpZM4SkuV3>
.
|
| LEDRainbowWaveEffect(void) {} // Empty constructor with default values | ||
| LEDRainbowWaveEffect(byte); | ||
| LEDRainbowWaveEffect(byte, uint16_t); | ||
| LEDRainbowWaveEffect(byte, uint16_t, uint8_t); |
There was a problem hiding this comment.
Are the extra constructors useful? Most of the time, one will just use the global LEDRainbowEffect object, which uses the empty constructor. I see little reason for having two rainbow effects in the same sketch...
So LEDRainbowEffect.delay(N) and/or LEDRainbowEffect.precision(X) from the setup() method of the sketch would be enough to tweak the effect.
What do you think?
There was a problem hiding this comment.
I actually have 8 of them in my sketch for my board. The default one moves too slowly for my preference, so I have two sets that move medium-fast (only changing default delay) and super-fast (changing both delay and precision in the constructor). Then I have one each of those sets at 25%, 50%, 75%, and 100% brightness. Then I can cycle through those with the next/previous LED effect key.
We don't have a system-defined way of universally increasing or decreasing LED brightness, or a system of hooks for globally tweaking current backlight mode changes (say, an API with defined global keys that plugins can hook into that will always work on the current LED backlighting mode).
I'm fine with having simply the void constructor and then the 3-parameter constructor, but in general my design philosophy as an engineer is that you should provide as many overloaded constructors or methods with sane default values as you think your end-user may need. It's better to be inclusive rather than exclusive, in general. But I don't typically have to worry about size or memory constraints so much, either, so I am willing to concede on that point here. :-)
There was a problem hiding this comment.
To further add to that, I think the order of the constructors is also relevant. I think the most common use case will be having multiple objects with different backlight levels (first constructor with 1 parameters for backlight). After that, I think people will want to change the backlight level and speed. Finally, I think the last parameters that people will want to change is the precision, to fine-tune their settings.
To me, I can definitely foresee somebody wanting each of those constructors, as I actually have used them all as I was tweaking settings to my liking.
There was a problem hiding this comment.
Ah, I see. I didn't think of your use case... well, I did, but my solution for the same goal would have been a bit different, and perhaps less straightforward: I'd have used a different key for brightness manipulation, a macro that would check the active mode, and cycle through pre-defined brigthness values (for modes that support it).
In the long run, I do agree: a way to globally set brightness would be useful.
In the short term, I'd think that having one constructor would be more in-line with how we currently do things, and brigthness would be set in a mode-specific way, if so desired. That saves quite a lot of resources (RAM and PROGMEM alike), at the cost of a bit more code to write. This would also be easier to migrate from once we have global brightness control, I think...
@obra, what do you think?
There was a problem hiding this comment.
I was actually just about to implement global brightness in LEDControl. I am going to implement an rgbToHsv() function, and override setLed to use the given rgb value and a global max brightness to find the equivalent rgb hue/sat at the given global brightness and use that instead. Then provide some global hooks for board brightness and keys to lower, increase, and disable backlight.
| uint16_t rainbow_wave_ticks = 10; // delays between update | ||
|
|
||
| byte rainbow_saturation = 255; | ||
| byte rainbow_value = 50; | ||
| byte rainbow_value = 151; |
There was a problem hiding this comment.
Why the change from 50 to 151?
(I have no recollection where and how this value is used, but the change does not seem to be explained neither in the commit messages, nor the PR)
There was a problem hiding this comment.
If you compile the default example with the default brightness, the LED backlighting effect looks quite awful. it isn't nearly bright enough. In the default Model01-firmware sketch representing what the board ships with, we explicitly set brightness to 151 for the rainbow effects. This change just moves that default value back into the plugin so don't have go and set it on the firmware sketch.
Add support for changing other properties besides brightness. We can now change the step delay and the precision of the hue change. This allows us to speed up the rainbow effect in various ways (or, potentially, to slow it down). New constructors were added for default brightness, brightness+delay, and brightness+delay+precision. This change allows for including multiple different rainbow effects with different parameters in a single sketch without having to adjust each parameter individually for each effect object. Finally, reset the default brightness to 151 to make the plugin default values match the values used in the default Model01 firmware sketch. The older value produced a lackluster-effect that needed to be updated in the sketch to look decent; this change means that default setting can be removed from the default firmware sketch.
|
I squashed and rebased the commit and then updated the commit message to reflect the changes from this PR. |
|
Global brightness would be awesome. What I describe here is my ideal, not
my requirements for merge.
The features I'd always intended to have in the global brightness control
are knowledge of the power draw curves of a given keyboard's R,G and B LED
phosphors, the number of LEDs available, the number of LEDs lit and the
allowed power draw of the hardware implementation.
For example:
The Model 01's LEDs are capable of drawing about 4 amps of power. We have a
configurable current limiter that can either limit our total power draw to
0.5a or 1.6a.
I hand tuned our default LED modes to mostly stay under 0.5a.
It _should_ be the case that a user could create an LED mode with 5
bright-white LEDs with no problem, but that if they try to light up all 64,
we drop their brightness to the point where they never pull more than the
currently configured global current limit.
And indeed, we try to optimize for clean and simples APIs over less device
resource usage over less code to write on the backend. Put another way, our
first duty is to our end users, our second is to our hardware limitations
and our third is to ourselves ;)
…On Mon, Mar 12, 2018 at 8:22 AM, Ben Gemperline ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Kaleidoscope-LEDEffect-Rainbow.h
<#2 (comment)>
:
> };
class LEDRainbowWaveEffect : public LEDMode {
public:
- LEDRainbowWaveEffect(void) {}
+ LEDRainbowWaveEffect(void) {} // Empty constructor with default values
+ LEDRainbowWaveEffect(byte);
+ LEDRainbowWaveEffect(byte, uint16_t);
+ LEDRainbowWaveEffect(byte, uint16_t, uint8_t);
I was actually just about to implement global brightness in LEDControl. I
am going to implement an rgbToHsv() function, and override setLed to use
the given rgb value and a global max brightness to find the equivalent rgb
hue/sat at the given global brightness and use that instead. Then provide
some global hooks for board brightness and keys to lower, increase, and
disable backlight.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACxaP4VT6KIAu1fH91qSlxecje5fWUHks5tdpLOgaJpZM4SkuV3>
.
|
Add support for changing other properties besides brightness. This allows us to speed up the rainbow effect in various ways (or, potentially, to slow it down).