Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single button dimmer control #1331

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

arjanmels
Copy link

Added functionality to allow dimming of the light with a single push button, by automatically cycling the brightness.

Tested on a modified H801 module. (I soldered a switch to GPIO 3 RX pin, marked TX on the board).

Functionality is influenced by the following new defines:

#define BUTTON_LNGPRESS_DELAY       1000        // Time in ms holding the button down to get a long click

#define LIGHT_DIMMING_TIME   5000            // Time in millis for full scale dimming
#define LIGHT_DIMMING_DIRECTION_TOGGLE 1    // dimming direction is 1: toggled, 0: up if < 50%, down of >= 50%
#define LIGHT_DIMMING_CYCLE 1               // dimming 1: cycles, 0: stops at extreme

An example setup for a button (short click, switches light on/off, cycling starts after a long press and stops when button is released:

    #define BUTTON1_PRESS           BUTTON_MODE_NONE
    #define BUTTON1_CLICK           BUTTON_MODE_TOGGLE
    #define BUTTON1_DBLCLICK        BUTTON_MODE_DIMMER_STOP
    #define BUTTON1_TRIPPLECLICK    BUTTON_MODE_DIMMER_STOP
    #define BUTTON1_LNGCLICK        BUTTON_MODE_DIMMER_STOP
    #define BUTTON1_LNGLNGCLICK     BUTTON_MODE_DIMMER_STOP
    #define BUTTON1_LNGPRESS        BUTTON_MODE_DIMMER_START

Added long press button event type.
Made pwm value after gamma correction higher resolution
Made gamma correction and dimming cycling parameters configurable via web ui
Changed ticker to loop in light.ino to prevent WDT reset
Optimized PWM and transition settings in hardware.h to get smoother transitions
@xoseperez
Copy link
Owner

This PR does some intrusive changes to the code apart from what you want to do, like removing the gamma table, the check to _light_has_color or changing some default values. This could potentially affect other boards.
Can you elaborate why are you doing this on a global scope?

@arjanmels
Copy link
Author

Hi, I remove the gamma table, because I replace it with a calculation. If you set the gamma value to 2.2, it will be very close to the table. The advantage of the calculation is that it allows higher precision than the 8-bit table, which makes for smoother dimming and you can configure the gamma to taste.

I also apply gamma for non RGB lights: the eye perception is logarithmic in nature, so a linear PWM dimming, will not have sufficient resolution in the low dimming levels to vary nicely.

I changed the default PWM duration, to allow for more gradual steps (and again smoother dimming).

(This smoother dimming is especially noticeable, when cycling with the new single button dimmer mode, but also visible during the on/off transitions.)

So in my view the implementation should only add features, without removing capabilities. Gamma can be set to 2.2 to replicate old RGB behavior and can be set to 1 to replicate old dimming behavior.

Regards,

Arjan

@TMaYaD
Copy link

TMaYaD commented Dec 17, 2018

@arjanmels You should split this into multiple PRs for each of the feature you added so that it can be reviewed and merged in better.

@arjanmels
Copy link
Author

I could split it in 1) the gamma changes and 2) the push button changes if so desired. However seems a bit of unnecessary administrative overhead on my side. @xoseperez what do you think?

@xoseperez
Copy link
Owner

Yes, I understand, and the changes are very welcome and make sense. But I still request them to be issued as separate PR.

@fameit-nl
Copy link

Ok, i will split it in the gamma change and button handling. Is that sufficient?

@davebuk davebuk mentioned this pull request Jun 5, 2019
@soif
Copy link
Contributor

soif commented Dec 9, 2019

Really nice feature @arjanmels ! 👍

Would you you rebase it and split this into multiple PRs any soon ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants