Multiple Strand Aggregation Part 2: Multi-Strand

[trshafer]

Hey @kitesurfer1404 and @moose4lord,

Follow up from #225 (although this PR has the same commits as #255). This should finish up the changes to support multiple LED strips in a single WS2812FX. I haven't actually tested this out yet on physical hardware, just on compiling example code. I'll get back to you once I've tried this out.

Totally open to feedback on variable names, suggested constructors, any public AdafruitNeoPixel functions that should be brought forward to WS2812FX, etc.

I brought forward getPixelColor but I really should have done that in #255. Happy to port that to the earlier PR.

setLength was the only function I could identify that doesn't easily work with multiple strands. We could expose the underlying strands as public members if that would be helpful to prevent issues with advanced users wanting access to the underlying AdafruitNeoPixel object.

Thanks for your consideration!

[trshafer]

This doesn't work in it's current form. Will bring out of draft once I have it working.

[moose4lord]

I'm not really sure what problem you're trying to solve here. If someone wanted to have a continuous effect across multiple strands, wouldn't they just daisy-chain the strands together? The issue you reference (#148) talks about coordinating animations across segments, which is a different problem.

Also, you're scrapping the whole Adafruit_NeoPixel inheritance model, which I think is fundamental to modern object oriented programming. I'm reluctant to go down that path.

[trshafer]

Hey @moose4lord,

Thanks for taking a look.

With regard to multiple strands: The issue is complications in wiring. For example if the strands are many meters apart, it doesn't make sense to wire just the data path from the end of one strand to the beginning of another. This is especially true for putting the strands in an encasing for weather proofing where the end may be physically inaccessible.

I see that issue 148 is not the same as the one this solves. I'll remove the addresses issue section.

With regard to inheritance model: I believe WS2812FX falls into the category of preferring composition over inheritance. Inheritance is useful for when a base class fundamentally represents a tree structure. For example Animal -> Dog, Animal -> Cat. However Adafruit_NeoPixel does not share the same relationship with WS2812FX. I believe a WS2812FX has an Adafruit_NeoPixel, and it can still certainly act as a drop in replacement for Adafruit_NeoPixel, however it is not an Adafruit_NeoPixel in itself, in that WS2812FX is an animation library and convenient API.

References:

• https://en.wikipedia.org/wiki/Composition_over_inheritance
• https://thoughtbot.com/blog/reusable-oo-composition-vs-inheritance
• https://www.javaworld.com/article/3409071/java-challenger-7-debugging-java-inheritance.html

The WS2812FX API is really convenient and the animations are great. Essentially what this PR does is decouple the animations library and API from the limitations of physical hardware.

Thanks for your consideration.

[trshafer]

Hey @moose4lord,

The code is now in a working state and is able to run animations across multiple physical LED strips.

Here is a working example of how this is used:

auto strip1 = Adafruit_NeoPixel(STRIP_1_LENGTH, STRIP_1_LED_PIN, NEO_RGB + NEO_KHZ800);
auto strip2 = Adafruit_NeoPixel(STRIP_2_LENGTH, STRIP_2_LED_PIN, NEO_RGB + NEO_KHZ800);

Adafruit_NeoPixel* strands[] = {&strip1, &strip2};
WS2812FX ledsObj = WS2812FX(strands, 2, NEO_RGB + NEO_KHZ800);
auto leds = &ledsObj;

void setup()
{
  leds->init();
  leds->setBrightness(BRIGHTNESS);
  leds->setMode(FX_MODE_CHASE_RAINBOW);
  leds->start();
}

void loop()
{
  leds->service();
}

Here is a video of it working: https://photos.app.goo.gl/NrFQRDKrQbDx4R3m6

[moose4lord]

Hmmm, I don't know. I think something like this can be better handled with a custom show() function as opposed to changing the underlying architecture. Could this feature be implemented by having the lib write the pixel colors to a long, virtual LED strand, then use a custom show() function to copy the data to the smaller, actual LED strands?

[trshafer]

I'm interested to understand what the rationale is for rejecting this re-architecture.

Is it that WS2812FX is supposed to be a true drop-in replacement for AdaFruit_NeoPixel and thus removing the inheritance breaks this guarantee of the AdaFruit_NeoPixel downward API? If this is the case there could always be one primary strand and multiple optional sub strands.

Is it that the code for assigning pixel values is more complicated in the animations? If that's the case then the animation library can be separated out from the WS2812FX class. This library would have WS2812FX and Animation classes. The Animation class would takes in an interface of the necessary code such as getPixelColor, getPixels, getLength, setPixelColor, etc. Essentially the WS2812FX class would be a thin wrapper binding AdaFruit_NeoPixel to an Animation class instance. The Animation class would then be able to be constructed using any object that conforms to that interface, such as a MultiStrandWS2812FX or even people who wanted to use FastLED. None of the code around multiple strands would be in the Animation class, just the interface functions.

Creating a long WS2812FX and also creating subsequent AdaFruit_NeoPixels requires almost 2n the amount of memory plus all of the extra time getting and setting pixel values. Each loop, I wouldn't know which pixels were set and thus would need to iterate over the entire strand and set each pixel value. It would be a significant reduction in performance, especially important when memory and processing is much more limited on an Arduino.

Thoughts?

[moose4lord]

I'm not rejecting your changes. But I do think it's a pretty radical change to accommodate an edge case application. As I said I think most people would just run the wires to daisy-chain the strands. Sometimes that's not convenient, but you've got to run wires in any case. That's just part of the process. I just don't think the problem you're solving warrants the changes you're proposing.

I'll also say I've been doing software development for so long that I have an extreme appreciation for simplicity. Keeping things simple pays dividends over and over and over again. My custom show() function idea can be implemented with a few lines of code:

void myCustomShow(void) {
    memcpy(ws2812fx_p1.getPixels(), ws2812fx_v1.getPixels(), ws2812fx_p1.getLength());
    ws2812fx_p1.show();
    memcpy(ws2812fx_p2.getPixels(), ws2812fx_v1.getPixels() + ws2812fx_p1.getLength(), ws2812fx_p2.getLength());
    ws2812fx_p2.show();
}

Pretty simple. I do see your point about 2n memory usage. That could be a problem with Arduinos, but ESP and ARM microprocessors are so cheap, it's probably not a big problem.

At the end of the day Harm has the final say for a PR like this. Although I make quite a few contributions, WS2812FX is his library. He'll be the final arbiter.

Edit: BTW, just to illustrate the added complexity of your redesign, your copyPixels() implementation is flawed. It uses the segment length (the "count" parameter) in it's memmove() call, not taking into account the length of each strand.

[kitesurfer1404]

Oh boys.

Short answer: I love the idea, but let's keep it separated. But let's link to the repository at @trshafer and point advanced users to it. If usage keeps growing, let's look at it again or maybe fork something else awesome from WS2812FX.

Long answer:

Even when I don't have time to contribute a lot of code after the initial release, I follow the progress closely and have to thanks @moose4lord for the great progress and support!

My goal was to "keep it simple" and especially give beginners a drop-in-replacement for the AdaFruit library. So all tutorials would work. Almost no code changes would be needed to switch to WS2812FX and keep the existing sketch running. Help and debugging could be find in the AdaFruit-Community as well as over here.

I follow the issues closely and most of them are from beginner users/programmers who use the library in a "simple" way: blink some LEDs, what's it intended to do. I always hoped to help beginners to understand how effects work and give a platform to add own effects without caring for calls and timing etc. With the custom-FX-features Keith made this very easy. Some times I don't want to be in Keiths situation answering the same questions over and over or debug absolutely beginners code. ;-) But that is what this library is for: get people into coding and blinking LEDs.

I know there are a lot of edge-cases. I have a multi-thousand LED project laying around for years. It will need multiple ESPs synced over WiFi and running in an artnet/DMX network. I totally WANT this to be an WS2812FX project just to show it to the world one christmas when it draws like 360 amps. ;-)

That's why I was interested in this PR from the beginning. Maybe it solves some of my problems as well. I totally like the idea of having an Animation class. I would love to find a way, an animation can be in a cpp-file and h-file alone and doesn't need more than one include to be in the FX-list.

But at the same time: I want to keep it simple and compatible. And on an entry level, which was always the highest goal.

So I think maybe we can:

• add a section in the readme and point advanced users for this case to the repository over at @trshafer
• ask for feedback how users use that code and what they think about it (just add an issue and give feedback there)
• see how many users switch/fork/submit issues and participate in the new code. I saw this library took off after I posted it to hackaday back in the days.

Thoughts?

[trshafer]

Hey @moose4lord and @kitesurfer1404,

Thanks both for your thoughts. I know things could be perceived as a little hot and my later response now is just purely due to other obligations, not anger nor frustration.

I'm a fan of a red -> green -> refactor. The system did not support multiple strand aggregation and now it does. This specific implementation in it's current form is a straw-proposal which can be modified, it's just code. I also believe that sending over an implementation demonstrates an active interest in contributing the work rather than complaining on a MIT license project and asking for others to do the work.

I personally would prefer not to maintain an active fork as maintaining and applying patches becomes a source of breakages. I see even recently @moose4lord is submitting substantial changes to the segments API. The more work I did on my fork, the less likely I would be keep bringing in new code. I'm confident my code will just rot, but that burden is on me.

@moose4lord I completely agree that simplicity is the always way to go. I believe that complex systems don't need to be complicated. Right now WS2812FX handles segments, pixel setting, state management, and many animations themselves. I'd be happy to put in work towards low coupling and high cohesion. I think the reason the multi-strand PR required so many deeply nested changes is because it's one large class.

This is not a criticism of the work, I'm incredibly grateful for the library in its current form. I've gotten much farther in my LED programming than I ever would have without this library!

I'm beginning to understand the motivations of the library and of the way y'all see it moving forward. I'd be happy to not promote WS2812FX taking in multiple strands, and now that I understand a main intent to give beginners a drop in replacement, keeping AdaFruit_Neopixel as the base class makes sense. I'd love to put in work to deconstruct the internals of the library into multiple independent pieces, while keeping the same interface for end users and not increasing internal complexity for contributors. Deconstructing the single class will also make it easier to implement the multi-strand feature without needing to bake it into WS2812FX. It sounds like there's interest in an Animation class, I could also see a State management class, and so on. However, I know it's not great to have a stranger come in and start chopping up the library so I can understand any hesitation.

Since this PR broke the copyPixels, adding tests will really help ensure that changes can be safely made. If unit testing is helpful, I can start with that.

[pech84]

I am very intresting about your solution, I would like to control 4 argb pan and one strip on many pin of my arduino

but I try it and it don't work ; I have lot of warning : warning: ISO C++ forbids converting a string constant to 'char*

and I have any light

• When I used my prog for WS2812FX with your librarie : I have thewarning but its ok and the light run

• When I am trying to modify it like behow nothing happen but the program run beeecause my screen print the good mod of WS2812FX

Could you help me?

An other thing, do you think that is possible to chained multiple segment as you try to do with multiple strip? In order to create new path of light between my all rgb light?

Sorry for my english and thanks for the answers

`#include <WS2812FX.h>
#include <Arduino.h>

#define LED_COUNT_STRIP 20
#define LED_COUNT_VENT 6
#define LED_PIN 6
#define TIMER_MS 30000

auto ada_arvent = Adafruit_NeoPixel(LED_COUNT_VENT, 6, NEO_RGB + NEO_KHZ800);
auto ada_strip = Adafruit_NeoPixel(LED_COUNT_VENT, 5, NEO_RGB + NEO_KHZ800);
auto ada_ahvent = Adafruit_NeoPixel(LED_COUNT_VENT, 4, NEO_RGB + NEO_KHZ800);
auto ada_amvent = Adafruit_NeoPixel(LED_COUNT_VENT, 3, NEO_RGB + NEO_KHZ800);
auto ada_abvent = Adafruit_NeoPixel(LED_COUNT_VENT, 2, NEO_RGB + NEO_KHZ800);

Adafruit_NeoPixel* strands[] = {&ada_abvent,&ada_amvent,&ada_ahvent,&ada_strip,&ada_arvent};
WS2812FX ledsObj = WS2812FX(strands, 2, NEO_RGB + NEO_KHZ800);
auto leds = &ledsObj;

unsigned long last_change = 0;
unsigned long now = 0;
int mod = 0;

void setup() {
  leds->init();
  leds->setBrightness(255);
  leds->setSpeed(1000);
  leds->setColor(0x007BFF);
  leds->setMode(FX_MODE_STATIC);
  leds->start();
}

void loop() {
  now = millis();
    leds->service();
  if(now - last_change > TIMER_MS) {
     mod = (leds->getMode() + 1) % 56;
    leds->setMode(mod);
    last_change = now;
  }
}`

[trshafer]

Thanks @moose4lord for all of the discussion. Closed by #232 which showcases using the same pixel array across multiple strips and using a custom show.