Skip to content

New _pixelbuf based Neopixel object does not allow updating .buf #65

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

Closed
sta-c0000 opened this issue Jan 10, 2020 · 13 comments
Closed

New _pixelbuf based Neopixel object does not allow updating .buf #65

sta-c0000 opened this issue Jan 10, 2020 · 13 comments

Comments

@sta-c0000
Copy link

ref: pull request #59
Before we could update pixels.buf (Neopixel obj) directly, but it does not work with new _pixelbuf based Neopixel.
Now it returns an error:

>>> type(pixels.buf)
<class 'bytearray'>
>>> pixels.buf = b'\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00\x01\x04\x00'
AttributeError: 'NeoPixel' object has no attribute 'buf'

Simple example for circuitplayground that works with current releases using old Neopixel object not using _pixelbuf:

from random import randint
import board
import neopixel
pixels = neopixel.NeoPixel(board.NEOPIXEL, 10)
pixels.buf = bytearray(randint(0,255) for i in range(30)) # <=- now fails
pixels.show()

Thank you.

@ladyada
Copy link
Member

ladyada commented Jan 10, 2020

@rhooper is this normal?

@rhooper
Copy link
Contributor

rhooper commented Jan 10, 2020

Sigh. This is the kind of thing that I wanted to find with testing before merging to master. I feel like we rushed merging the pixelbuf changes. This is fixable but might have slight performance penalties until I make changes to _pixelbuf to allow changing the underlying bytearray.

@ladyada
Copy link
Member

ladyada commented Jan 10, 2020

there's no way to really find these sorta bugs without a lot of eyeballs lookin at em :)

@siddacious
Copy link
Contributor

Ya, no worries dude. All things considered IMHO it's better to keep up the momentum and get it out and tested more widely than sitting on it making sure it's ISO certified

@rhooper
Copy link
Contributor

rhooper commented Jan 10, 2020

@sta-c0000 Do you ever directly manipulate buf with subscripts or slices?

@siddacious
Copy link
Contributor

@rhooper while it's perhaps a decent use case, it's maybe worth considering if accessing buf directly is/was an intended part of the API or not.

@rhooper
Copy link
Contributor

rhooper commented Jan 10, 2020

@siddacious I'd say that providing your own buffer is the right approach. As this breaks backward compatibility, a (deprecated) buf property along with new kwargs to pass in buffer(s) would be the best approach.

I'll post examples and a PR soon.

@sta-c0000
Copy link
Author

@rhooper No. Subscripts work now (pixels.buf[0] = 3), but I haven't used. I don't think you can modify slices now... just tested:

pixels.buf[:3] = b'\x04\x00\x01'
TypeError: 'bytes' object does not support item assignment

Would need to do (but would have your bytearray already):

b = bytearray(pixels.buf)
b[:3] = b'\x04\x00\x01'
pixels.buf = b

As for siddacious's comment: I imagine direct .buf access is not very common.

@tannewt
Copy link
Member

tannewt commented Jan 10, 2020

I'd rather not support direct .buf access because it can be done with a raw bytearray and passing it to neopixel_write directly.

@rhooper
Copy link
Contributor

rhooper commented Jan 10, 2020

@sta-c0000 You're right on the bytearray (which _pixelbuf uses).

The reason for the issue with assigning to buf is that there is no setter on the buf property of _pixelbuf (yet). I've filed issue adafruit/circuitpython#2502

You might want to work with a _pixelbuf directly if you're already mucking with .buf, like @tannewt suggested.

Nevertheless, I'm going to submit a PR that adds the ability to pass in your own bytearray buffer(s) to NeoPixel so you can do what you're doing another way:

import board
import time

num_pixels = 32
buffer = bytearray(32 * 3)
pixels = neopixel.NeoPixel(board.D6, num_pixels, brightness=0.1, auto_write=True,
                           pixel_buffer=buffer)
pixels.fill((128,128,128))
pixels.show()
buffer[:] = bytearray([((i % 5) * 10) for i, _ in enumerate(buffer)])
pixels.redraw()
pixels.show()

@sta-c0000
Copy link
Author

sta-c0000 commented Jan 10, 2020

Issue was primarily discrepancy with previous NeoPixel obj noticed while testing. I don't know, but perhaps .buf write can indeed be deprecated?... feel free to close this issue once noted if so.
Direct bytearray neopixel_write sounds like a reasonable workaround.

edit: in fact I'd like to close since being pointed in that direction, is faster; e.g. fast cycling (where our buf is filled bytearray):

while True:
    neopixel_write(pin, buf)
    buf = buf[-3:] + buf[:-3]
    ...

Thanks again!

@rhooper
Copy link
Contributor

rhooper commented Jan 14, 2020

@sta-c0000 I think you can close the issue.
You might also find performance of using NeoPixel with _pixelbuf and slices with integers similarly fast and possibly easier to work with. I'd love to chat more about what you're doing sometime - perhaps on discord? https://blog.adafruit.com/2017/07/20/adafruit-is-on-discord-discordapp-adafruit-discord-adafruit/ where you can find me as krayola. Your use case might give me more inspiration for ways to speed up or make _pixelbuf better.

@sta-c0000
Copy link
Author

Yes, only starting to play with the new _pixelbuf and NeoPixel functions like .fill are leaps and bounds faster, which is great. Closing, thanks!

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

No branches or pull requests

5 participants