PlummersSoftwareLLC / NightDriverStrip

NightDriver client for ESP32
https://plummerssoftwarellc.github.io/NightDriverStrip/
GNU General Public License v3.0
1.31k stars 209 forks source link

Feature: Need a Brightness- button #284

Open davepl opened 1 year ago

davepl commented 1 year ago

At some polnt the "Brightness Down" button, second from the left in the top row of the remote, next to ON) used to dim the brightness. If you hit ON, it would set to max brightness and pin the effect on.

I think the top two buttons should be Brite- and Brite+ and I prefer that Brite+ just goes to max brightness, not steps up. But we need to move the "SET_EFFECT_INTERVAL to MAX" feature to another button, one of the spare buttons in the lower right.

revision29 commented 10 months ago

@rbergen does the code in the current pull request for 454 relate to someone's particular remote control? If so, I will want to replicate that functionality when I push changes to the remote control code in a few weeks.

I also added a code review for the pull request for 454 to suggest a change to make the brightness level actually apply to pixels drawn with the led strip effects. All of the work done for the brightness variables mean nothing if the code does not actually apply the brightness level. Once that tweak is done and the rest of that pull request is applied, the remote brightness controls should actually work for led strip effects. Testing will need to be done with matrices to see if any changes need to be made there.

Lastly, I found the right place to make a pull request and have done so for effectmanager.h.

revision29 commented 10 months ago

It is so nice to power cycle a device and the LEDS stay at the low brightness level I set them to with the remote. Thank you @rbergen on getting the brightness code into device config.

rbergen commented 10 months ago

@revision29

does the code in the current pull request for 454 relate to someone's particular remote control?

It leaves all of the existing remote control logic, i.e. as it is in the current codebase, untouched. So, the remote control logic in #454 operates exactly the same as the logic in the current main branch does.

I also added a code review for the pull request for 454 to suggest a change to make the brightness level actually apply to pixels drawn with the led strip effects. All of the work done for the brightness variables mean nothing if the code does not actually apply the brightness level. Once that tweak is done and the rest of that pull request is applied, the remote brightness controls should actually work for led strip effects.

The suggestion is fair enough, but I won't apply it in this PR. The goal of #454 is making brightness a persisted property and nothing else. The change you suggest I can't actually even apply responsibly, because I personally can't test it.

I think the approach to take here is one in two stages:

  1. After review, we merge #454 to arrange brightness being persisted.
  2. A second PR is opened after that by someone else (possibly you) to make changes to the remote control logic that are deemed necessary for brightness to apply to LED strips and have been tested to work.

Testing will need to be done with matrices to see if any changes need to be made there.

...and that is something I can possibly help with, before or when the second PR I just mentioned is opened.

It is so nice to power cycle a device and the LEDS stay at the low brightness level I set them to with the remote. Thank you rbergen on getting the brightness code into device config.

That's nice to hear! You're welcome!

revision29 commented 10 months ago

@rbergen Once the merge is done I would be glad to do the pull request to apply the brightness level to the output.

rbergen commented 10 months ago

I'll run some additional tests myself, today and then merge the PR if they succeed. I'll comment here when that happens.

rbergen commented 10 months ago

I just merged #454.