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

Replace PIO code with an inbuilt timer, based on https://learn.adafru… #20

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

Conversation

mcarlson
Copy link

@mcarlson mcarlson commented Feb 5, 2024

…it.com/intro-to-rp2040-pio-with-circuitpython/advanced-using-pio-to-drive-neopixels-in-the-background

Not really helping with the glitchiness I wonder if it's my hardware at this point?

@blaz-r
Copy link
Owner

blaz-r commented Feb 5, 2024

Many thanks for this.

I'll test is as soon as I get the time. I'm not sure if this would cause any backwards-compatibility problems if merged to main, but if it works okay, I believe it probably shouldn't.

@blaz-r blaz-r self-assigned this Feb 5, 2024
Copy link
Owner

@blaz-r blaz-r left a comment

Choose a reason for hiding this comment

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

Thanks again for this 😄. Just some minor comments.

neopixel.py Outdated Show resolved Hide resolved
neopixel.py Show resolved Hide resolved
neopixel.py Outdated Show resolved Hide resolved
@mcarlson
Copy link
Author

mcarlson commented Feb 7, 2024

I believe I've addressed your comments, let me know! Keep in mind the testcase I posted requires pull 19 to work. Also, I should mention the first four pixels are white for me when I apply this change, so it's not 100% yet.

@blaz-r
Copy link
Owner

blaz-r commented Feb 7, 2024

Thanks for addressing the comments. I'm not sure about the white pixel problem, but it might be due to the header and trailer because the code on that link I believe does some shifting and padding to fit the DMA requirements and I'm not sure if that's same here.

Uses an offset to ensure get/set() happens in the right place. Also caches the memoryview once to make show() as efficient as possible. It turned out passing the header/trailer values as-is from python is the right approach. Everything works well for me now, maybe I'll look into DMA next :)
@mcarlson
Copy link
Author

mcarlson commented Feb 8, 2024

You were right, that packing was bogus. I've updated the PR with the new code, it works really nicely now... Thanks for your patience!

@blaz-r
Copy link
Owner

blaz-r commented Feb 10, 2024

Great to hear. Did the glitches also go away with this?

@mcarlson
Copy link
Author

Not entirely, though removing all interrupts and replacing with asyncio (especially Timers) seems to help a lot. It's waaay better though!

@blaz-r
Copy link
Owner

blaz-r commented Feb 18, 2024

Good. I'll probably have some time to review this and test next weekend. Thanks for all the effort 😄

@blaz-r
Copy link
Owner

blaz-r commented Mar 31, 2024

Hi, I've finally had the time to check this, really sorry for the delay. It works quite well, but I had to change some code as rotation no longer works with this code.

The other problem is that this only works for RGBW, so merging this would mean that RGB strips are no longer supported. I'm not entirely sure how to fix this, as that would require rewriting PIO program to do the pull once 24 bits are read. I'll see what I can do.

@mcarlson
Copy link
Author

mcarlson commented Apr 1, 2024 via email

@blaz-r
Copy link
Owner

blaz-r commented Apr 1, 2024

Thank you. I had an idea that could maybe work: when doing pull(ifempty) if we could somehow append some additional code to this ifempty and shift for 8 bits, then it should work. But at the moment I'm not sure if "ifempty" works outside pull instruction or/if it's possible to detect empty OSR.

@blaz-r
Copy link
Owner

blaz-r commented Apr 1, 2024

I think I figured it out. There is in fact and osre keyword that signals empty OSR 🥳

@blaz-r
Copy link
Owner

blaz-r commented Apr 1, 2024

When you get some time, test this code I pushed on your end (RGBW only, no need to order RGB). I tested on both RGB and RGBW and it works for me on all examples now.

@blaz-r
Copy link
Owner

blaz-r commented Apr 2, 2024

Looking at the new asm for ws2821, I think that the new logic introduces some additional low output, that might be problematic in some cases, although I had no issues with my strip - I'll try to fix it, but I won't be able to test for some time now.

@blaz-r blaz-r mentioned this pull request Oct 30, 2024
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