Closed outlandnish closed 3 years ago
Hi,
first of all please accept my apology for responding with such a long delay.
You did a pretty good job here and most of the changes make sense in my opinion. However, the PR you present basically introduces a new library which was only inspired by SmartLEDs. What do you expect from opening this PR? What would come out of merging the library? Why don't you publish it under a new name? When the PR will be merged, it will be hard to maintain the library for you since you don't have write access to the repository. And I am not in the position to maintain someone else's code.
Currently, I am quite skeptical about merging this PR even I find the job that you did really good. My reasons for that are as follows:
Have you considered to break the fork relation and only mention in README or somewhere that the library was originally based on SmartLeds? I will be a happy user of AddressableLeds!
Thanks for taking the time to review the PR - I know it's a lot of changes.
I didn't intend for this PR to be merged - it was more to elicit discussion about certain parts of the existing SmartLeds library that can be adapted with small usability changes.
For example, things like the base class that you can use to share some of the functionality and the support for RGBW might be added with not too much disruption to the existing code.
That said, I'd be happy to maintain AddressableLeds as an inspired by SmartLeds fork. Thanks for putting together an awesome base library to start from!
This is a huge PR that addresses several things:
ApaRgb
in favor of apixelToRaw
function that translates theRgb
dataI opened this PR more to discuss the ideas than to necessarily change the codebase. I think the first four features could be implemented pretty easily in the existing code.
The RMT driver implementation does require a change from Espressif. I've opened a PR on their end for comment https://github.com/espressif/esp-idf/pull/6002
Anyway, I'd love to hear your thoughts!