esp8266/scripts/neopixel: Support 4bpp and slicing #2536
Conversation
| @@ -19,10 +20,19 @@ def _reorder(self, val): | |||
| yield val[max(i, self.bpp - 1)] | |||
|
|
|||
| def __setitem__(self, index, val): | |||
| if hasattr(index, 'start'): | |||
There was a problem hiding this comment.
I can't use isinstance(index, slice) here, as I would in normal Python, because there is no slice type available.
|
If this is merged, then #2478 would be fixed. |
|
I would more expect some comments about this approach, and perhaps some discussion about how we could handle such things in the future. |
|
I'd find commit messages somewhat terse. E.g. "Allow slicing of the NeoPixel object". And so? Please give example of the usage (in the commit message), show why it's beneficial. |
|
Squashed the last two commits, rebased against master and elaborated on the possible use cases. |
| self.n = len(self.buf) // self.bpp | ||
| return | ||
| offset = index * self.bpp | ||
| self.buf[offset:offset + self.bpp] = bytes(self._reorder(val)) |
There was a problem hiding this comment.
This seems like a lot of overhead just to set a single pixel. Here you need to allocate a generator function (which itself makes a copy of self.ORDER), then make a temporary bytes object, and also make a slice object for the store operation. How about the following implementation:
if hasattr(index, 'start'):
index = index.start or 0
else:
val = (val,)
index *= self.bpp
for col in val:
for pix in range(self.bpp):
self.buf[index] = col[self.ORDER[pix]]
index += 1
There was a problem hiding this comment.
I have to admit I didn't much think about optimizing this much. Now that you pointed out the obvious shortcomings, I have to admit that this code is pretty bad efficiency-wise.
Your code surely is more efficient, but not only reads like C (you can program C in any language), but also will not work correctly when you assign to the slice a different number of elements than the slice has.
I don't think it's important. I'm just using this opportunity to do two things -- first, make sure that you can actually implement proper slicing support for your own class in MicroPython -- and I already found some small details that make it tricky (which are now fixed), and second, to see how such implementation would look like, how it can be made both as readable and as efficient as possible without writing too much dedicated code, and how useful it is in practice to use. I think I've learned what I needed, thanks to the feedback on this pull request, and I'd be happy to remove the two slice-related commits from this pull request, and make the code more efficient. |
Yes, please. It's nice to combine the code for apa102 and neopixels, but then which one should be the base class? Seems a bit strange to derive either from the other (and if neopixel.py goes users will wonder why apa102 stops working). Maybe they need a common base class, like LEDPixel? |
I'm not sure about this. Which module you would like me to put that class in, But after your remark, I can see it would be much better to make the |
|
I removed the slicing code, got rid of the unnecessary memory allocations and generators, an squashed it all into a single change. I left the APA102 class inheriting from NeoPixel, because it has that additional clock pin. I'm still not sure about a common "abstract" class -- I don't think it would add much, and it's not free. |
There was a problem hiding this comment.
I'm still not sure about a common "abstract" class -- I don't think it would add much, and it's not free.
Ok, let's keep it with NeoPixel as the "base" for now, and keep the refactoring in mind for later if this stuff needs to be made more generic / support more RGB LEDs.
| return self.buf[i + 1], self.buf[i], self.buf[i + 2] | ||
| offset = index * self.bpp | ||
| return tuple(self.buf[offset + self.ORFER[i]] | ||
| for i in range(self.bpp)) |
There was a problem hiding this comment.
ORFER->ORDER, and also this can go on 1 line.
| self.buf[i * 3] = g | ||
| self.buf[i * 3 + 1] = r | ||
| self.buf[i * 3 + 2] = b | ||
| self[:] = (color for i in range(self.n)) |
There was a problem hiding this comment.
The __setattr__ method no longer accepts slices. How about:
for i in range(self.n):
self[i] = color
(This version also needs no heap RAM.)
|
I'm very sorry. This is what happens when I make a quick (and, I thought, trivial) change, and forget to test it. Fixed now. |
|
Merged in 984a867, with fix to make super() call work correctly. |
Rewrite the neopixel and apa102 drivers.