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

Implement ftdi led device #1595

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

nurikk
Copy link

@nurikk nurikk commented Mar 25, 2023

Hello! This PR introduces ftdilib1 led device implementation. FTDI chips are common usb2serial/spi/whatever chips, and are available for 5-10$ from Chinese website as various breakout boards.

(This is the back port from awawa-dev/HyperHDR#521)

Screenshot 2023-02-25 at 21 52 19

My wiring is following:

FTDI  |  APA102 
AD0 -> CLOCK
AD1 -> DATA

AD3 -> CS (active low) // can be used to drive logic shifter OE pins


FTDI  |  WS2812 or SK6812
AD1 -> DATA

AD3 -> CS (active low) // can be used to drive logic shifter OE pins

I've tested my code on MacOS and webos7(lg tv c2), no extra drivers required.

This implementation is tested against APA102, WS1218 and SK6812.

Requires libftdi1 dependency hyperion-project/hyperion.docker-ci#6

Test webOS builds available here https://github.com/nurikk/hyperion.ng-webos-loader/actions/runs/4520870590

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of web configuration, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

@hyperion-project
Copy link

Hello @nurikk 👋

I'm the Hyperion Project Bot and I want to thank you for
contributing to Hyperion with your pull requests!

To help you and other users test your pull requests faster,
I'll create a link for you to your workflow artifacts.

🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4520879701

Of course, if you make changes to your PR, I will create a new link.

Best regards,
Hyperion Project

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4521703611

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4524686424

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4525152198

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4525325491

Copy link
Collaborator

@Lord-Grey Lord-Grey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nurikk
many thanks for your contribution!

Thanks for the PR backporting FTDI support from HyperHDR.
In some areas hyperion works a bit differently.
Therefore, please find some review comments below and related to the files.
I am happy to support you in getting it aligned.

Following additional comments:

  • The content_leds.js code does not support the discovery mechanism.
    Ping me privately on discord and I share a updated version with you or provide access to your repository and I push a commit with some updates...
  • Would you mind adding libftdi1-dev to doc/development/CompileHowto.md
  • I am still wondering, if it would make sense changing the backend code that way, that e.g. apa102 can work via SPI or FTDI.
    i.e. having common LED type code but working via a different way of communication.
    Currently, the e.g. APA SPI and APA FTDI is not consistent any longer.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
assets/webconfig/js/content_leds.js Outdated Show resolved Hide resolved
include/utils/RgbToRgbw.h Outdated Show resolved Hide resolved
libsrc/leddevice/schemas/schema-apa102_ftdi.json Outdated Show resolved Hide resolved
libsrc/leddevice/dev_ftdi/LedDeviceSk6812_ftdi.h Outdated Show resolved Hide resolved
libsrc/leddevice/dev_ftdi/ProviderFtdi.cpp Outdated Show resolved Hide resolved
libsrc/leddevice/dev_ftdi/ProviderFtdi.cpp Outdated Show resolved Hide resolved
libsrc/leddevice/dev_ftdi/ProviderFtdi.cpp Outdated Show resolved Hide resolved
libsrc/leddevice/dev_ftdi/ProviderFtdi.cpp Outdated Show resolved Hide resolved
@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4526282386

@nurikk
Copy link
Author

nurikk commented Mar 26, 2023

Hi @Lord-Grey, thanks for a great review! I've addressed all questions.

I am still wondering, if it would make sense changing the backend code that way, that e.g. apa102 can work via SPI or FTDI.

I'd love to do that, but I'm afraid I'm not competent enough in cpp and overall project structure to start such refactoring.

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4526347635

@nurikk
Copy link
Author

nurikk commented Mar 26, 2023

@Lord-Grey can you help me with linker issues? It compiled right for mac os, but it fails on linux https://github.com/nurikk/hyperion.ng/actions/runs/4526347559/jobs/7971453468

@nurikk nurikk mentioned this pull request Mar 27, 2023
9 tasks
Copy link
Collaborator

@Lord-Grey Lord-Grey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for turning the previous feedback round quickly!

@Lord-Grey can you help me with linker issues? It compiled right for mac os, but it fails on linux https://github.com/nurikk/hyperion.ng/actions/runs/4526347559/jobs/7971453468

Please see some feedback below. It should address to issue.

You can also run unix docker compiles with local codes to test before running the docker on Git
For the failing Stretch build you would execute in the local code directory:

sudo ./bin/scripts/docker-compile.sh -i x86_64 -t stretch -b Debug -p false -l

Look at the script and you can easily cross-compile for testing on other platforms too.

libsrc/leddevice/CMakeLists.txt Outdated Show resolved Hide resolved
libsrc/leddevice/CMakeLists.txt Outdated Show resolved Hide resolved
assets/webconfig/js/content_leds.js Outdated Show resolved Hide resolved
assets/webconfig/js/content_leds.js Outdated Show resolved Hide resolved
libsrc/leddevice/dev_ftdi/LedDeviceWs2812_ftdi.cpp Outdated Show resolved Hide resolved
"required": true,
"propertyOrder": 1
},
"rate": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist the baud rate totally dynamic or does it follow the USB serial baud rates?
If it follows the baud rate, I suggest you use a rateList as in
https://github.com/hyperion-project/hyperion.ng/blob/master/libsrc/leddevice/schemas/schema-adalight.json
to avoid user misconfiguration.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not abut serial baud rate, since sk6812 is pwm driven and we simulate pwm signal using spi, allowing users to manually adjust baud rate will help to fix any troubles with dodgy strings. same for ws2812

libsrc/leddevice/schemas/schema-sk6812_ftdi.json Outdated Show resolved Hide resolved
libsrc/leddevice/schemas/schema-ws2812_ftdi.json Outdated Show resolved Hide resolved
libsrc/leddevice/schemas/schema-apa102_ftdi.json Outdated Show resolved Hide resolved
@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4533883364

@Lord-Grey
Copy link
Collaborator

@nurikk Sorry, I missed one thing,

Could you adding libftdi1-dev to doc/development/CompileHowto.md

and to

sudo apt-get install --yes git cmake build-essential qtbase5-dev libqt5serialport5-dev libqt5sql5-sqlite libqt5svg5-dev libqt5x11extras5-dev libusb-1.0-0-dev python3-dev libcec-dev libxcb-image0-dev libxcb-util0-dev libxcb-shm0-dev libxcb-render0-dev libxcb-randr0-dev libxrandr-dev libxrender-dev libavahi-core-dev libavahi-compat-libdnssd-dev libasound2-dev libturbojpeg0-dev libjpeg-dev libssl-dev

Otherwise the Code-Checker will fail to build.

@nurikk
Copy link
Author

nurikk commented Mar 27, 2023

Done

@nurikk
Copy link
Author

nurikk commented Mar 27, 2023

@Lord-Grey can you advise me on how to refactor spi and ftdi led devices to adhere DRY? I don't quite like the inheritance here

@Lord-Grey
Copy link
Collaborator

Lord-Grey commented Mar 27, 2023

Done

Maybe you missed to push the commits.
I do not see the updates for codeql.yml and HowTo in the PR…

@nurikk
Copy link
Author

nurikk commented Mar 27, 2023

Done

Maybe you missed to push the commits. I do not see the updates for codeql.yml and HowTo in the PR…

Oh yeah, I forgot to push:) I’ll push once get back home

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4535397801

@nurikk
Copy link
Author

nurikk commented Mar 27, 2023

Pushed :)

@asturel
Copy link

asturel commented Mar 27, 2023

Many thanks, it's not possible to support windows platform (too)?

@Lord-Grey
Copy link
Collaborator

Pushed :)

Great. Thank you!
Codeanalysis is already running.

@nurikk
Copy link
Author

nurikk commented Mar 27, 2023

Many thanks, it's not possible to support windows platform (too)?

This should be possible, but I have no idea how to install libraries on windows.

@Lord-Grey
Copy link
Collaborator

@Lord-Grey can you advise me on how to refactor spi and ftdi led devices to adhere DRY? I don't quite like the inheritance here

I am not a Cpp-Pro and would need to investigate about it. There are different options, but not limited to, like a bridging class, multiple inheritance, using templates, etc…

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4575891089

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4588817630

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4644801210

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4646737069

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4671474105

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4671482336

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4688562541

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4688729370

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4688751219

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4688759522

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4688811504

@satgit62
Copy link

Hello, is there any chance to implement the Hyperion FTDI for Windows?

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4724908872

@nurikk
Copy link
Author

nurikk commented Apr 17, 2023

Hello, is there any chance to implement the Hyperion FTDI for Windows?

Hi, it's definitely possible, but not the top priority atm

@asturel
Copy link

asturel commented Apr 19, 2023

I really liked the previously white calc. algorithms ("auto", "auto_max", "auto_accurate"), could we have them back (or they get removed not intentionally) ?

@hyperion-project
Copy link

Hey @nurikk I created a new link to your workflow artifacts:
🔗 https://github.com/hyperion-project/hyperion.ng/actions/runs/4742822044

@nurikk
Copy link
Author

nurikk commented Apr 19, 2023

I really liked the previously white calc. algorithms ("auto", "auto_max", "auto_accurate"), could we have them back (or they get removed not intentionally) ?

added them back

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@asturel
Copy link

asturel commented Apr 25, 2023

Is it possible that very low led color causes the led flashing between on/off? I used the same length cable /5cm?/ + level shifter /sn74ahct/ (even hooked OE to GND instead of CS /AD3/) as used for esp32 where this problem doesn't occurs. I captured short footage of the issue (the movie was paused):
https://youtube.com/shorts/y1rV8DnI5q0
https://youtube.com/shorts/yPuYVCJ0Oe0
It's totally (source) content related, i couldn't reproduce with effects/colors, also have some 'random' (occurs around some white 'area') white flashes.

Any idea what can cause this? I tried different baud rates, latch time, slower led update rate, but the problem persist :/.
I don't think the problem is related to my hw (everything is kinda the same as I mentioned above) and switching (on the fly -- just changing the controller type) to esp32 + hyperserial fixes the issue.

p.s.: SK6812 RGBW strip

@satgit62
Copy link

@asturel
I have connected the FTDI without level shifter and I do not have this error. I also use SK6812RGBW.

@asturel
Copy link

asturel commented Apr 28, 2023

Is the reset time typo or lowered on purpose based on some tests? const int SPI_FRAME_END_LATCH_BYTES = 3; https://github.com/nurikk/hyperion.ng/blob/ftdi/libsrc/leddevice/dev_spi/LedDeviceSk6812SPI.cpp#L45
According to SK6812's datasheet https://cdn-shop.adafruit.com/product-files/2757/p2757_SK6812RGBW_REV01.pdf the reset time is 80µs, so with 3000000 baud rate it should be 30 (?). I have bumped to 32 at first (calculated with 3200000 baud) and seems like the issue is gone, but from a short test i haven't run into issues with 5 either.

btw it shouldn't be dependent on leds count? (the datasheet applies to 300 leds (?))

@m4rkusxxl
Copy link

m4rkusxxl commented Aug 28, 2023

I use a FT232H without a level shifter and tried this patch, but I had to do some adjustments:

  1. Mixture of ENABLE_DEV_FTDI / ENABLE_FTDIDEV
  2. After opening the device I had to add a 1s sleep for it to work.
  3. Not sure why, but ftdi_usb_get_strings failed hard. I removed it but thats of course not a good solution ;)
  4. As I applied it to the current master, I needed to adapt the deviceList keys to systemLocation and deviceName.

With that being said, its driving my old 189 APA102 setup without any problems for two weeks now.
So thank you for the work and looking forward to get it merged.

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.

None yet

6 participants