Another WS2812B driver (with updated APA102)

[HumJ0218]

I wrote another WS2812B driver. It is used in a similar way to APA102.
I added a method for correcting visual chromatic aberration.
This driver has been working steadily for more than 3 days on a 150-LEDs-strip.
If you find it appropriate, I will continue to add the remaining project files.

Microsoft Reviewers: Open in CodeFlow

[krwq]

LGTM logicwise, couple of nits and span uses should be fixed.

[krwq]

(also fix build errors)

[joperezr]

[Triage] @HumJ0218 can you please address the comments that @krwq added so that we can go ahead and merge this PR?

[Ellerbach]

Looks good to me once all comments addressed.

[HumJ0218]

@joperezr can you go ahead this PR? I have a SK6812-RGBW depends on this and ready to commit

[krwq]

@HumJ0218 sorry for late review, just a question: how is this Ws2812b different from https://github.com/dotnet/iot/blob/main/src/devices/Ws28xx/Ws2812B.cs ?

[krwq]

codewise LGTM but still would like to understand first why there is a need for two bindings with the same name

[pgrawehr]

Please fix the merge conflict as well. CommonHelpers.csproj has been renamed to just Common.csproj

[HumJ0218]

@krwq I remember the previous ws2812b didn't perform well (on devices except Raspberry Pi 3B), and I felt the need to unify the operation of this type of LED-strip

[joperezr]

[Triage] @HumJ0218 We are still a bit confused here. We believe that there shouldn't be two WS2812B device bindings. Think about it this way, if I'm a developer with a WS2812B and I want to use this repo's bindings for my project, and I see two classes for my device, which one should I choose? This confusion will be hard to explain, so we believe that the right way to approach this would be to have a single WS2812B class, which could potentially use two different underlying implementations by using different constructors, but in the end we should only have one type.

Does that make sense? Would it be possible for you to make these requested changes? Is there a reason why you still think that the two types should be separate?

[Ellerbach]

@HumJ0218 I had a deeper look. Also I recently did some work on the .NET nanoFramework side. See here https://github.com/nanoframework/nanoFramework.IoT.Device/tree/develop/devices/Ws28xx.Esp32 and here https://github.com/nanoframework/nanoFramework.IoT.Device/tree/develop/devices/Ws28xx.
And for what you want to acheive, the best is to create a BitmapImage class. This is where you can add your specific encoding. There are quite few already, the names are not the best (we can revisit that). So far what I've seen is the following:

• 3 bit encoding RGB or GRB (Neo3 in this repo)
• 8 bit encoding RGB or GRB (WS2808 in this repo)
• What you are bringing adding the gama component if I can read properly

Our original idea was to hide somehow this complexity with driver names and classes. Unfortunately, the same "name" can use different implementations of the underlying encoding.

So, rather than trying to continue in this direction, we may rethink the approach and have a better naming for each of the encoding and better documentation. And rethink the classes for the constructors with a default bitmap image to keep some compatibility.

Let me know if you need help to move your implementation into a BitmapImage. Also if you are willing to give a try for the renaming and exposing them as public (will be needed for the constructor), that would be great. Same for the documentation :-). Now, if you're willing to do only the first step, that's fine, I can take over the next ones.

[HumJ0218]

@Ellerbach This driver is also flawed in its specific performance, so I‘m no longer considering whether to add a new ws2812b driver. But I still want to discuss about color, pixels, and images.

Because of functions for graphics drawing is more mature, I prefer to use System.Drawing. I can draw lines, ellipses, rectangles, and write text using any font on image as much as I want, and transform images at will. The only downside is that it is no longer available in environments such as Linux

Neo3 and SixLabors.ImageSharp, their support for the above features is incomplete.

I think it's important to strengthen graphics-related features first, or to be careful about choosing a more powerful graphics library.

[pgrawehr]

@HumJ0218 Yes, we are working on this, see the discussion on #1403, #1644 and others. We expect that in the future, the bindings will take some kind of abstract image type and just do the conversion to the required format for the hardware. The entire drawing part will be external (either in a new library or with e.g. ImageSharp). We haven't made any final decision yet.

[krwq]

[Triage]: Doubled Ws2812b implementation should be merged or removed from this PR and done through BitmapImage abstract class (i.e. https://github.com/dotnet/iot/blob/main/src/devices/Ws28xx/BitmapImageNeo3.cs) - if there is any improvements you're doing here please merge it with existing BitmapImage for this binding. Apa102 should be implemented also with BitmapImage, it should be relatively simple change. Also we should not be touching gamma (we may add it as an extra option on the BitmapImage but it should not be a default)

[joperezr]

[Triage]: @HumJ0218 let us know if you are still interested in applying the suggestions from @krwq above.

[ghost]

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days.

[ghost]

Hello @HumJ0218, the reviewers on this PR have requested changes and there has not been any activity for 14 days, so it will be closed for housekeeping pourposes. Feel free to reopen or send a new one if you wish to resume the progress.