corbanmailloux / esp-mqtt-rgb-led

MQTT RGB LEDs Using JSON for Home Assistant
MIT License
270 stars 74 forks source link

Add unified sketch #31

Closed r41d closed 6 years ago

r41d commented 6 years ago

As mentioned in #29. This is based on the RGBW sketch. The idea in creating this, was to only modify the places where we talk to the outside world (outside of the ESP chip) like receiving commands, publishing to state topic or when setting the GPIO pins. Some unneeded calculations (in case it's not used as RGBW) are left in the source code to avoid more if-else clutter, although this could be added if deemed sensible.

The config became a little more complicated, but everything is explained to the user there.

Examples of significant portions of the the diff between RGBW and this unified sketch:

19a20,21
> const bool rgb = CONFIG_STRIP == RGB || CONFIG_STRIP == RGBW;
> const bool rgbw = CONFIG_STRIP == RGBW;

101,103c103,109
<   pinMode(greenPin, OUTPUT);
<   pinMode(bluePin, OUTPUT);
<   pinMode(whitePin, OUTPUT);
---
>   if (rgb) {
>     pinMode(greenPin, OUTPUT);
>     pinMode(bluePin, OUTPUT);
>     if (rgbw) {
>       pinMode(whitePin, OUTPUT);
>     }
>   }

308,311c322,327
<   JsonObject& color = root.createNestedObject("color");
<   color["r"] = red;
<   color["g"] = green;
<   color["b"] = blue;
---
>   if (rgb) {
>     JsonObject& color = root.createNestedObject("color");
>     color["r"] = red;
>     color["g"] = green;
>     color["b"] = blue;
>   }

362,364c380,386
<   analogWrite(greenPin, inG);
<   analogWrite(bluePin, inB);
<   analogWrite(whitePin, inW);
---
>   if (rgb) {
>     analogWrite(greenPin, inG);
>     analogWrite(bluePin, inB);
>     if (rgbw) {
>       analogWrite(whitePin, inW);
>     }
>   }
corbanmailloux commented 6 years ago

This is looking really good. I'll try to make time this weekend to set it up on one of my light systems to test out. Thank you for adding this.

depuits commented 6 years ago

Wouldn't it make more sense to use the whitePin when only using brightness instead of the redPin?

r41d commented 6 years ago

You have a point, but I decided to use the red pin for this because this also done in the current standalone brightness-only sketch. I could refactor it to use white for brightness-only mode. What would @corbanmailloux prefer?

corbanmailloux commented 6 years ago

I can't believe I still haven't reviewed this. I'm so sorry.

As to the red vs. white pin discussion, I do think it makes more sense to use the white pin for brightness-only systems. I didn't do this in the current brightness-only sketch because I was just looking to complete it quickly, and because I built it before making the RGBW version.

I've put some time on my calendar for this weekend to review/assist/merge this PR (and the other open issues here). Thanks for your understanding.

r41d commented 6 years ago

I have a lot of AFK time at the moment, also the whole next weekend. After it, I could refactor the code.

corbanmailloux commented 6 years ago

I've merged this in and switched to using the white pin for a brightness-only light. I'm probably going to rename the files in the repo to feature this as the "correct" way to set it up and make the old ones deprecated.

Thanks for your help and patience!

r41d commented 6 years ago

Awesome, this is perfect! :)