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

LN882H WS2811 Driver #1414

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jasperro
Copy link

@jasperro jasperro commented Nov 4, 2024

This PR is related to https://www.elektroda.com/rtvforum/topic4083817.html

It implements WS2811 support for the LN882H platform.

I will still be adding some documentation and the ability to change the data pin using a pin role, after which I will unmark this as draft.

@jasperro jasperro force-pushed the jasperroLN882H-WS2811 branch 4 times, most recently from eec0da7 to 9a0fe1d Compare November 7, 2024 22:36
@jasperro jasperro marked this pull request as ready for review November 7, 2024 22:42
@jasperro jasperro changed the title Draft: LN882H WS2811 Driver LN882H WS2811 Driver Nov 7, 2024
@jasperro
Copy link
Author

jasperro commented Nov 7, 2024

I think this one is about ready. Before merging, it is needed that openshwprojects/OpenLN882H#19 is merged. The .gitmodules file and modules can then point to the main OpenLN882H repo again.

Small note: it seems there is a small issue in the Makefile introduced/which got influenced by the ESP-IDF changes, which made it impossible to build locally using the docker image. Locally, I changed the line under the .PHONY submodule from git submodule update --init --recursive --remote to git submodule update --init --recursive, after which I could build again.

@openshwprojects
Copy link
Owner

Nice, WS2811 on LN88H, altough it would be better to have it togeter with current SPI LED for Beken, instead as separate driver. Btw, I don't think you can add pin enumeration inside enum without breaking people configs... also , why pins header change?

@jasperro jasperro force-pushed the jasperroLN882H-WS2811 branch from 9a0fe1d to a580562 Compare January 12, 2025 17:28
@jasperro
Copy link
Author

jasperro commented Jan 13, 2025

Nice, WS2811 on LN88H, altough it would be better to have it togeter with current SPI LED for Beken, instead as separate driver. Btw, I don't think you can add pin enumeration inside enum without breaking people configs... also , why pins header change?

The pin header change was because LN882H pins are weird. They have a base and then the pin. By splitting out the pin definitions into a separate header file, I could use this to call LN882H hal functions in drv_ws2811.c (which require base and pin) with OpenBK7231T_App pin index.
I'm not sure if this is the best way, but it is the one I could make work.

The reason it does not work like the BK SPIDMA/SPILED is because LN882H has direct HAL for WS2811 in the SDK. This way, I could just easily use the already existing code. I think the LN882H has some built-in WS2811 circuitry that doesn't use SPI at all, only DMA. So it would not make sense to use SPI if this is actually not supposed to be used.

Now I have made a drv_SM16703P.h, that is implemented by drv_SM16703P_LN882H.c and drv_SM16703P_BK7231N.c for both platforms. It is working quite nicely (with DDP working and PixelAnim working except for ShootingStar, I still need to implement some more stuff). Not sure what to do with the docs, one of the two driver files needs to be chosen as the single source of truth? Or maybe the command registration should be done in drv_SM16703P.c.

The old version can still be found here: https://github.com/jasperro/OpenBK7231T_App/tree/jasperroLN882H-WS2811-old

Also, with "Btw, I don't think you can add pin enumeration inside enum without breaking people configs...", do you mean you store the enum backing int in the config instead of the string? So if I add it in the middle it does not work for existing configs? (but in the end shouldn't break it), now with the new SM16703P-only driver should not be a problem of course.

It would be very nice to get this merged eventually.

@jasperro jasperro force-pushed the jasperroLN882H-WS2811 branch from a580562 to 12786d1 Compare January 13, 2025 21:55
@jasperro jasperro force-pushed the jasperroLN882H-WS2811 branch from 09d7795 to 622979a Compare January 13, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants