proposal for issue #41

[depuits]

It still needs a lot of cleanup but I wanted to get some feedback if this I'm going in the right for #41.

I think that the interface for the effects doesn't need to be much more complicated but the ColorState class should be expanded to allow the effects to communicate with each other and the rest of the program. The ColorState class should probably encapsulate all led logic in a way that implementing issue #42 would also not affect the effects logic.

I'm not sure if separate source files or if all code inside the headers are preferred.

I think I've also found a fix for #34 but it could be improved by encapsulating the transitioning better.

[corbanmailloux]

This is really cool, and I'm impressed with how quickly you created this. If you are happy continuing down this path, I'm loving it.

I have a couple quick comments that I've noticed so far:

• I think the contract/interface for the ColorState should be more restrictive and explicit. I'm thinking that it would expose the bare minimum of public fields (like the colors) and hide things like the weirdly-named realRed.
• I'm not sure if the fading logic even really belongs in ColorState. For now, this might be necessary, but if we do end up including in FastLED (Research: Using FastLED under the hood #42) and using the blend function, it seems like this might become a responsibility of the effect itself. Flash, for example, doesn't need to use any blending/fading code.

I'm picturing the flow working like this for each loop. Please correct me if you have a different view.

1. Main logic calls the update of the active effect, which:
   a. does any calculation needed (timing, blending, etc.) to get the exact value to set the LEDs to
   b. sets the specific color fields on the ColorState object
2. Main logic calls the update of the ColorState, which:
   a. writes the color out to the LEDs
   b. doesn't perform any of it's own fading calculations (except maybe global brightness)

This makes the ColorState object pretty "dumb," but I'm thinking it should just be a state object. This also would align with the FastLED model (from what I understand). You set the colors on a state object and then call FastLED.show() to push those values out to the lights.

---

To your comment about source files vs. code in the header files, I don't really have a lot of experience in that world. I'm totally happy with the code-in-header style because it keeps things simpler.

Finally, I'm not sure how well it would work, but could we make transitions just be another effect? I think it would help with the encapsulation issue, but it might be more of a headache. I'm curious about your thoughts now that you've gone through this process.

Thank you again for going through the work and setting this up.

[depuits]

The ColorState interface should indeed be a lot more restrictive for this the json parsing part would probably need to be moved from the main code to the ColorState.

The transition itself could probably be an effect but I'm not sure how this would work inside the home assistant UI. As far as I've seen you can only select an effect but not specify parameters such as color to transition to.

Further I think the design depends on how much the effects should depend on the fastled library. If we don't want the effects to actually depend on fastled then we should probably provide a blend like function in which we could swap the implementation. If the effects will depend on the festled library then we probably won't even need the colorstate object because the effects itself could directly call the FastLED.show() to push the changes when the effects need it.

[corbanmailloux]

I'm not sure about moving the JSON processing into ColorState. It seems like JSON processing would be part of the main logic. I'm thinking of using the ColorState object much like FastLED's pixel object (because we can't actually use that part of FastLED with analog LEDs). It would mostly be a data object.

So an effect would set ColorState.red, green, blue, brightness values and then the main logic would call update() (or show()) and push the color to the LED.

I think it's a kind of nice abstraction to not let the effects directly update the lights, only the ColorState object. That also means that it might be possible to chain effects someday.

I didn't mean that transitions should be sent from HASS as an effect. I should have clarified. I just meant that I wonder if it would make sense to internally treat it like one. It seems like it could implement the same methods as the current effects. I haven't thought that through yet; just curious.

[depuits]

I think I understand what you mean. I'll rework it some more when I have time.

[corbanmailloux]

Thanks, @depuits. Let me just reaffirm that I really like this proposal. There will always be things to tweak, so I'm okay with merging this in when we are relatively happy with it and continuing to make future improvements as well.

[depuits]

I've moved the transition logic into its own Transition effect and simplified the ColorState object so it now only contains the rgbw values and brightness logic. It still implements the IEffect interface so we can still just loop all effects in the main code.

The ColorFade now also depends on the Transition effect to fade between colors.

[corbanmailloux]

I've been on a holiday break from code for a bit. Now that I'm back, I'll be spending some time this week/weekend reviewing and hopefully merging this in.
Thanks for the patience.

[depuits]

What changes should still be made to merge this code?