adafruit / Adafruit_CircuitPython_NeoPixel

CircuitPython drivers for neopixels.
MIT License
304 stars 98 forks source link

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

Closed sta-c0000 closed 4 years ago

sta-c0000 commented 4 years ago

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 commented 4 years ago

@rhooper is this normal?

rhooper commented 4 years ago

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 commented 4 years ago

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

siddacious commented 4 years ago

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 commented 4 years ago

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

siddacious commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 https://github.com/adafruit/circuitpython/issues/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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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