Add PixelSubStrip class to simplify working with multiple segments

[lbt]

The existing API is unchanged.

A PixelSubStrip handles a subset of the pixels in a PixelStrip and can
be created like so:
strip = PixelStrip(...)
strip1 = strip.createPixelSubStrip(0, num=10) # controls first 10 pixels
strip2 = strip.createPixelSubStrip(10, num=10) # controls next 10 pixels

Signed-off-by: David Greaves [email protected]

[MaxThom]

this is cool! I'm working on a project as well and I did the segment in a different way. But your way is pretty cool.

[MaxThom]

With A change like this, you should add an example or two.

[lbt]

More than happy to do that. I first wanted to see if the approach was something that was acceptable.

[Gadgetoid]

Sorry for the lack of response here, this library is pretty low on my looooong list of priorities.

This is an interesting change- reminiscent of the sort of things I was trying to accomplish with Plasma. I think it solves a real problem and it's worth exploring.

This library should probably trend toward being helpful. I wonder if we could tackle things like 2d matrices with a similar pattern?

Right now I tackle those downstream with a fairly blunt 2d list lookup- https://github.com/pimoroni/unicorn-hat/blob/657deffc385895d43a81828c30355e5051cce6c7/library/UnicornHat/unicornhat.py#L76-L85

[shadoxxhd]

numpy support could naturally accommodate this with views:

# strip is a numpy ndarray of the color values; assuming dtype=np.uint32 for now
substrip1 = stip[start:end]
substrip2 = strip[start2:end2:2] # only every 2nd LED in this area

# assign colors to whole substrips
substrip1[:] = col1
substrip2[:] = col2

matrix = strip.reshape((10,10)) # still assuming dtype=np.uint32; otherwise ".reshape((10,10,4))" to preserve WRGB color channels
# assign colors to 2D areas
matrix[2:5,3:7] = col1
matrix[0,0] = col2
matrix[:,5] = col3
# or even assign every 2nd LED in the 8th column (assuming x,y indexing)
matrix[8,::2] = col4
# switch between x,y and y,x indexing
matrix2 = matrix.T

# access color components
matrix_col = matrix.view(dtype=np.uint8) # might need [:,:,::-1] to reverse byte order, depending on underlying architecture
matrix_col[9,9,0] = w # set white channel of specific pixel

If the numpy array was created in C and passed to python on PixelStrip creation, assignments to the numpy array would directly modify the underlying C array, and could immediately be rendered without any manual data movement (eg. setPixelColor calls).

[lbt]

I've rebased this onto master as I've been doing some internal updates.

I have broken up the commits to make them easier to review
It depends on (and includes) #119

FWIW this is what I'm using to split a single PixelStrip into 2 SubPixelStrips here:
https://youtu.be/EquTyu9kp5I?t=150

It makes it so much easier to allocate sections of a logical strip for this kind of task.
(Even the tree has one strip which starts at the bottom and has the mid-point at the top)

@Gadgetoid I've not got anything setup as an array but I don't think this work would get in the way of any such functionality.

@shadoxxhd I agree about numpy; that would give a huge performance boost. However I think that would require the c-layer to be written to depend on numpy and that might be a big ask. I'm getting reasonably high performance doing all my processing in numpy and then copying in python: 6f144bb#diff-c7f7281f1b739abb1773f50e1d78f8257d8e9ff15d7ac010e0bca0435be2eed1R117

A ws2811_led_set_from_bytes(channel, bytes) which took a packed array of uint32_t would make this much faster as it would all be C-side and would not require an upstream dependency on numpy.

[shadoxxhd]

I'm pretty sure numpy integration wouldn't need any changes to the C layer, just the swig wrapper - you could use an "Argout View Array" in swig to give numpy a reference to the c-array, which you should then be able to access like any other numpy array.

[Gadgetoid]

Probably wouldn't hurt for anyone to rebase/test this against the Pi 5 branch which will be replacing main in short order - https://github.com/rpi-ws281x/rpi-ws281x-python/tree/pi5

Edit: Also thank you for the rebase after.. quite some time! I think this is a worthwhile contribution.

[shadoxxhd]

I implemented an extremely basic numpy integration - it's only necessary to add the swig numpy "header" numpy.i and add a few lines to rpi_ws281x.i.
To try it out, run swig and setup.py, then in python simply call raw = ws.ws281x_get_array(leds, channel_id) to get the numpy array.
Writing to the numpy array and calling render correctly updates LEDs for me.

For proper integration, someone (possibly me) should make a nicer interface for it; at least allow access to that array directly from the PixelStrip class, and ideally make the functions in PixelStrip internally use the new access method as well (for performance reasons).

https://github.com/shadoxxhd/rpi-ws281x-python

[shadoxxhd]

Changing the getitem, setitem and "derived" methods to use the numpy view seems rather straightforward - the only "breaking" change would be that passing a slice to getitem now returns a numpy array instead of a python list when slicing (this could be trivially prevented with a type check if desired).

It does allow using setitem with slice assignment, ie. strip[2:5]=range(6,9), which wasn't possible previously, and should be more performant.

The more complex question is how the memory view itself should be exposed - making getPixels return a writable numpy view instead of a list copy might be a significant change (shouldn't break anything if used "correctly", but there are probably some users who reuse the returned list in some ways...).
There is also the question if substrip/matrix views should just be created by the user from the pixels/subpixels views, or if there should be wrapper classes (or at least creation methods) for those.

In any case, the "optimal" way of using this library would change a lot from the "old" method (with slice/array assignments being much better than loops, and offering a performance reason to do intermediate calculations on numpy arrays instead of inside the assignment loop).

One thing my current implementation doesn't consider is changing the LED array pointer inside the channel structure - this would have performance benefits for eg. cyclical animations, where you'd only have to update this single pointer between various (numpy-created) pixel arrays, between render calls. But dealing with the memory management of having python-created numpy arrays accessed from C code does seem to add complications... (would it be enough to keep a python reference to whatever array was last pointer-passed to the C struct, to prevent access-after-free? I'm not sure...)
And you'd somehow have to take care of eventually freeing the original pixel array as well...

[shadoxxhd]

(nvm, was looking at old commits)

[lbt]

@shadoxxhd. Yes - I did the slice handling in python.

Earlier today I rebased on top of your changes. Can't test it today though.
I noticed the use of _colors and also that you're using the
self._values[pos] = value
to handle implicit slices.

I also realise that I didn't add the getitem and setitem to the PixelSubStrip and that I'd accidentally included a submodule commit in there

[lbt]

@shadoxxhd the work you've done on numpy is interesting but I can't trivially make it work.
I've got a Pi5 and have it running with the kernel module / overlay etc.
Your repo hasn't got issues enabled so I can't comment there
I think you've missed a ws. prefix (see patch below)
I also think you've committed the wrong rpi_ws281x_wrap.c as ws2811_array_get isn't in there.
I'l try and get a numpy.h locally and rebuild it.

library/rpi_ws281x/rpi_ws281x.py
@@ -146,7 +146,7 @@ class PixelStrip:
            raise RuntimeError('ws2811_init failed with code {0} ({1})'.format(resp, str_resp))

        # initialize array view
-        self._values = ws2811_array_get(self._channel)
+        self._values = ws.ws2811_array_get(self._channel)
        self._colors = self._values.view(dtype=np.uint8).reshape((-1,4)) # get view of individual color bytes
        if self._values.byteorder == "<" or (self._values.byteorder == "=" and sys.byteorder == "little"):
            self._colors = self._colors[:,::-1]

(also I think it may be good to make numpy optional)

[shadoxxhd]

The _wrap.c had to be regenerated by swig; run swig -python rpi_ws281x.i in rpi_ws281x/library to regenerate the interfaces.

You are right, I somehow had a half finished version of the rpi_ws281x/rpi_ws281x.py in my repo; I just fixed that (and two other similar "got too reliant on IDE" omissions) and force-pushed the correct one (and the swig-generated interface files).

I thought a bit about making numpy integration optional, but that doesn't seem trivial - the swig binding is either in the library or not, and if it is, I think numpy is loaded internally regardless of whether you call the interface or not.
I don't actually know how this works in terms of the distribution - is the numpy package automatically set as a dependency, or does swig somehow duplicate the necessary numpy C code for the functionality in the package?

[Gadgetoid]

is the numpy package automatically set as a dependency, or does swig somehow duplicate the necessary numpy C code for the functionality in the package?

A good question- I should probably look into this. Either way it should be relatively straight-forward to add numpy as a dependency, although it's quite a hefty package to pull in where it might not be desired. 🤔

It's a little pedantic, but note that Python favours snake_case over camelCase for method/function names. So getPixelColorRGB should ideally be get_pixel_color_rgb, and so on.

[lbt]

Yes. I managed to build it with numpy (which has other issues that I can discuss) but then I hit even more problems (the pi5 branch doesn't work for SPI on Zero2) during regression testing. I reported this against the pi5 issue/PR.

@shadoxxhd would you consider rebasing your numpy changes against the SubPixelStrip approach? (and submit a PR for comments/issues)

@Gadgetoid @shadoxxhd I'm going to respectfully suggest that this patchset is considered against master and not against pi5/numpy concepts.
I do think I need to look at using slices and I'll see about adding in some tests too.

But can we focus on reviewing inclusion of this patchset by itself please :)

[Gadgetoid]

@Gadgetoid @shadoxxhd I'm going to respectfully suggest that this patchset is considered against master and not against pi5/numpy concepts.

Sure! I am not actually totally sure where numpy factors in- I've kinda lost the plot a little bit. Your pure-Python change is relatively uncontentious and, indeed, we probably shouldn't burden you with feature creep.

Fair warning- There's probably not going to be a release until Pi 5 support is mainline, I am veeeeeerryy limited on time and want to sweep up as many changes as possible in one shot 😭

[lbt]

Thanks - I've just added the slice handling to SubPixelStrip and I think it'd be good to do some pytests and an example too.

The numpy inclusion would avoid copying a python list/slice into the ws_2811 internal 'array' using a loop in python which may help keep the frame rates up on long strings on small (Zero) devices. But my Zero2 is able to do audio->array processing in numpy and then use python loops to copy that array to ws_2811 and still get 30fps on 300 LEDs. I haven't profiled it to see where the bottleneck actually is.

I'd like to think about that too as having seen the SWIG code I think we may be able to do that in a C loop in the Python binding without the need for numpy at all. I look forward to @shadoxxhd pushing a numpy PR though

Also a release isn't an issue for me as I am running from git for now anyway :)

[shadoxxhd]

Creating a PR for the swig interface is easy, but my high level interface conflicts with some of the PixelSubStrip code.
Eg. PixelStrip.setPixelColor would accept numpy advanced indexing, while PixelSubStrip.setPixelColor explicitely deals with slices and would "accidentally" work with integer array indexing, but would produce strange results with boolean indexing (converting the bool array into a combination of 0 and self.first).
PixelSubStrip.setPixelColor doesn't have slice handling.

The best solution seems to me to be replacing all the manual index adjusting with keeping a numpy view in the substrip (instead of first/last), and keeping the other functions around (you could simply inherit them from PixelStrip - the difference is just the view stored in self._values).
It might even be desirable to make the PixelSubStrip constructor take arbitrary simple indexing (instead of first/last) and (optionally) desired shape as arguments, allowing to create strided and matrix substrips respectively (you couldn't represent a "back and forth" 2D strip arrangement as a spatially correct matrix with views, but for most other uses, views should suffice).

---

I made a substrip branch with a) a commit containing only the swig interface, b) a commit with minimal changes to use the numpy view and c) a commit with integration of substrips and numpy. Since substrips can now be backed by subset numpy views into the "master" view of the internal LED data, I put most of the methods in a baseclass of both PixelStrip and PixelSubStrip.
It doesn't yet include a way to create PixelSubStrips with strided or reshaped views, though the methods should implicitly handle such PixelSubStrips.

[lbt]

@shadoxxhd Please MR that branch and I'll pull it and try - and then we can discuss numpy stuff there :)
Should that all work then I'd expect PixelSubStrip to be able to have a self.view and then for setPixelColor() to reduce to self.view[n] = color ... but lets take that to a new MR

[lbt]

@Gadgetoid I've added tests bringing coverage to 100%

I reworked the slice handling too and I think this API makes sense.

The commits are structured to make the evolution as easy as possible (IMHO) to review