Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FastLED CRGB[] (leds) support #2662

Open
Aircoookie opened this issue May 8, 2022 · 12 comments
Open

FastLED CRGB[] (leds) support #2662

Aircoookie opened this issue May 8, 2022 · 12 comments

Comments

@Aircoookie
Copy link
Owner

Is your feature request related to a problem? Please describe.
It is some effort for users to port effects written for the FastLED library to WLED.

Describe the solution you'd like
Add support for using the CRGB leds[] array from within WLED effect functions, calling setPixelColor() and getPixelColor() automatically via [] operator override

Describe alternatives you've considered
Current WLED effect code flavor and upcoming custom FX scripting does not make it easier to port FastLED effects.

Additional context
https://stackoverflow.com/questions/37043078/c-overloading-array-operator

@blazoncek
Copy link
Collaborator

leds[] array is in fact double-buffering.
It can be global/permanent or local/effect specific (temporary).

Unfortunately most often leds[] represent RGB array stripping away W support WLED has.

Still, using leds[], global or local, can benefit effect porting since LED/RGB data is retained (losslessly) between invocations of the effect function which is it's most common use.

Actually there is no need to overload [] operator since we only need an index to leds[].
Currently most 2D implementations use XY() function that produces an index from X and Y coordinates of a pixel. This requires a knowlege of pixel position on a strip in relation to the layout of the matrix in case global leds[] is used which can be cumbersome.
Using local leds[] removes that requirement but requires knowing segment dimensions (which are available since segment is a member class of a strip!). In such case XY() only needs to know width of a segment.

I have implemented 2D support for WLED that uses local leds[] array in a sample 2D effect bundled with it.

@Aircoookie
Copy link
Owner Author

Of course leds[] in its standard form is double buffering.
This overload idea would allow for using leds[] without using double buffering or allocating extra memory.
I think double buffering is essential to have, especially with 2D where you often have to limit brightness, but I'm not yet sure whether it should occur on the effect or bus level:

Effect level benefits:

  • Only use double buffering if effect requires it
  • Less convoluted to implement

Bus level benefits:

  • Less memory fragmentation
  • Most consistent, also allows for memory of e.g. live/Individual LED control if turned off or brightness adjusted
  • Can be user configured depending on available memory

@blazoncek
Copy link
Collaborator

Sorry, I misunderstood the original post. 😄

Overloading leds[XY(x,y)] with a setPixelColor/getPixelColor is a nice idea. Though, rarely needed since leds[] serves its own purpose IMO.
If using global leds[], strip.show() could do the setPixelColor and getPixelColor could be replaced to return leds[] (since it is used in handful effects).
If using local leds[] a for() loop is needed to do setPixelColor at the end of effect function, which can be shortened as SEGMENT.show().

@blazoncek
Copy link
Collaborator

I have been porting FastLED effects (25 currently) to WLED using dynamic leds[] array and it only takes 8 lines of code to make simple effects work.

@blazoncek
Copy link
Collaborator

One more note regading global leds[].

WLED handles color in uint32_t whereas CRGB is effectively uint24_t and as such discarding/loosing W channel.

If leds[] is implemented as a global to allow benefits from above and to serve as a double buffer it will mean sacrificing W channel for portability. This may not be desirable in every circumstance.

The additional benefit of global leds[] may be (depending on effect implementation) support for overlapping segmens and blendinng their effects.

@blazoncek
Copy link
Collaborator

@ewoudwijma has started work on this here.

So the recommendation would be to overload [] and =operators to support setPixelColor and getPixelColor

@ewoudwijma
Copy link
Contributor

Eventually it turned out we removed leds from effect functions all together and only have leds under the hood in sPC and gPC.
So using fastled code into WLED requires changing to sPC and gPC. Good thing is gPC is not applying brightness on earlier sPC (due to leds array under the hood).

Question is if this is enough if we still need to add leds overloads? I personally think not as we keep code more straightforward by only allowing sPC and gPC.
What do you think?

@blazoncek
Copy link
Collaborator

Although I would prefer WLED way of writing effects (using setPixelColor() & getPixelColor()) most people familiar with FastLED will find it awkward.
Unfortunately it is true that converting FastLED effect for WLED is not really straightforward and leds[] alone will not make it possible to port anything but simplest effects.

@blazoncek
Copy link
Collaborator

Double buffering done in #3280

@blazoncek
Copy link
Collaborator

@Aircoookie do we keep this open?
I know setPixelColor/getPixelColoris not real leds[] substitute but IMO adding C++ overloaded operators will be too much work for too little effect as FastLED effects cannot all be directly ported to WLED.

@softhack007
Copy link
Collaborator

softhack007 commented Nov 17, 2023

My opinion is, let's close it.

Even if we can use some tricks to implement an "array index" operator, it won't allow to do all the fancy things that are possible with FastLED leds[].

  • setPixelColor/getPixelColor has some performance penalties, whereas fastLEDs leds[] are the quickest way to do things
  • our sPC/gPC is lossy, unless you use global buffering
  • I doubt that we could support patterns like memmove( &(leds[4]), &(leds[48]), 42 * sizeof(leds[0]))

@blazoncek
Copy link
Collaborator

Just a thought: If we ditch global brightness then we no longer need to care about lossy pixel restoration. And we can play with pixel arrays directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants