drivers/led/neopixel: Add brightness control.

[tmountjr]

New functionality to specify a brightness for the strip when creating the instance, and also via set_brightness. Also refactored the fill() method to avoid duplicating the logic used in __setitem__. Existing functionality is that the write() method must be called to see the changes, so while set_brightness does change the values for all the pixels in the instance, it does not write that back out to the strip immediately.

This should be completely backwards-compatible with the previous version in that the brightness is by default 100%.

[ricksorensen]

Do all implementations now have floating point enabled? (#define MICROPY_FLOAT_IMPL MICROPY_FLOAT_IMPL_FLOAT )

[tmountjr]

I had just assumed that float was a built-in type. It seems to be referenced that way here - https://docs.micropython.org/en/latest/genrst/builtin_types.html.

If that's not the case, what's the process for handling floating point numbers on systems that don't have that type?

[ricksorensen]

I am not sure how to check. Look for a type exception maybe?. As `micropython has` matured, more boards now have at least software floating point which can be optionally turned off to save flash space, but I think are a couple of older boards that do not enable floating point by default `CC3200`, `powerpc`, `pic16bit`, `nrf` (`NRF51`) The designers would be able to offer better guidance on the direction here. Note that some display drivers require floating point also - for rotations and positioning- and may offer some guidance on how to handle missing floating point type.

…

On Fri, Sep 29, 2023 at 1:27 PM Tom Mount ***@***.***> wrote: I had just assumed that float was a built-in type. It seems to be referenced that way here - https://docs.micropython.org/en/latest/genrst/builtin_types.html. If that's not the case, what's the process for handling floating point numbers on systems that don't have that type? — Reply to this email directly, view it on GitHub <#739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5QPQD5PVRDOIMEYRS5WXLX44HKTANCNFSM6AAAAAA5MQEXV4> . You are receiving this because you commented.Message ID: ***@***.***>

-- Rick Sorensen ***@***.*** 651-470-5511

[jimmo]

Thanks @tmountjr

@ricksorensen is correct that not all boards have floating point enabled. The bigger issue though is that using floating point is very expensive in MicroPython because floats get heap-allocated so you want to absolutely avoid using them unless necessary.

For example, on a PYBV11, the following line:

n.fill((255,0,0,0))

takes 2058 microseconds before this PR, but now takes 24686 microseconds.

But I think this can probably be almost entirely addressed:

• Make brightness optional (and None if not set).
• Only calculate the adjusted tuple once (right now it's doing it for every pixel).
• Keep fill as it was (implementing it in terms of __setitem__ is slow).

See e.g. https://gist.github.com/jimmo/472bcf9213bae97fc915fd71d85048b9 which is back to ~2048 microseconds when brightness is not set and 2238 microseconds when set to 0.5.

I'm also not sure about the behavior of __getitem__ though. It's a bit weird that if you set a pixel and read it back, you get the adjusted value? I realise it's difficult to make this work any other way though.

[tmountjr]

Good idea only conditionally applying brightness if it's already set. Also didn't consider that I'd be recalculating the values for every single pixel if I simplified the fill() method.

The use of __getitem__ in the set_brightness method was a bit of a shot in the dark for me too - I basically thought of it from a high level - when you set the brightness, you need to iterate over each pixel and recalculate what the new tuple should be based on the new brightness. __getitem__ already handles getting the right component value for each "pixel" in the bytearray, so the left half takes care of itself; and __setitem__ handles calculating the brightness as a function of setting the component values, so that's the right half too. I agree though that self[i] = self[i] seems kinda redundant but unless there are performance concerns with using the under methods it may qualify as elegant. 😄 It could use some documentation though, if only to acknowledge the fact that it looks really odd.

[tmountjr]

@jimmo Thanks for all your feedback! You seem to have access to some hardware that I don't that catches corner cases...would you be able to take a look at the latest version of this PR on that hardware and let me know if the performance is where it should be?

[jimmo]

You seem to have access to some hardware

Nope just any board running MicroPython:

t = time.ticks_us()
... do some code ...
print(time.ticks_diff(time.ticks_us(), t))

catches corner cases

Nothing special going on here, was just calling fill() for neopixel objects with and without the brightness set.

It would be also worth testing code that sets pixels directly (which I imagine is more representative of what people typically do with neopixels).

---

I'm on the fence about this... I agree it's a useful feature -- it's nice to be able to e.g. fade in/out a strip or just separate your pixel generating code from brightness control. And it's good that it doesn't affect performance significantly if you don't use it (although set pixel will now be slower because it has to check self.brightness). I am still not super excited about the fact that set and get pixel are no longer symmetric (but again I don't have a better suggestion... so maybe it's a reasonable compromise).

The other thing to think about is code size increase. I spent a bit of time trying to minimise neopixel.py as much as possible (which is why the style is a bit weird). neopixel.mpy is currently 616 bytes. This PR increases it to 925. I think we can get it down to 819 without any functional changes. I'll add some comments to the PR to explain.

[jimmo]

This is definitely stylistically better, but functions are expensive in terms of code size (and therefore memory usage).

In this case, it's actually cheaper to just use min(max(brightness), 0, 1) directly in both places. Note also changing 0.0 to 0 etc is also helpful for size (and doesn't change the behaviour).

[jimmo]

We should rename this to self.b -- class members end up allocating qstrs, so using small names is better.

It's also smaller to write

self.b = None if brightness is None else min(max(brightness), 0, 1)

rather than

self.b = None
if brightness is not None:
   self.b = min(max(brightness), 0, 1)

[jimmo]

Rename to _b (to save qstr usage). It's a private method, so better to add a comment than to waste bytes on the name. (Unfortunately this is par for the course with MicroPython... there are lots of ugly things we have to do, ideally in places the user doesn't see, in order to save code size. Every byte counts).

[jimmo]

Rename to brightness (now that that name isn't used for the field). It's more in line with what we do elsewhere (e.g. pin.value()).

Also I think, similarly, it should probably return the current brightness too, e.g.

    def brightness(self, brightness):
        if brightness is not None:
            self.b = min(max(brightness, 0), 1)
            # This may look odd but because __getitem__ and __setitem__ handle all the
            # brightness logic already, we can offload the work to those methods.
            for i in range(self.n):
                self[i] = self[i]
        return self.b

which is +10 bytes, but I think useful for people to write e.g. strip.brightness(strip.brightness() + 0.1).

[tmountjr]

Did some tests on my Pico W...

• fill() time with the changes: 658us without a brightness, 1093us with a brightness
• changing brightness with brightness(): 4294us <-- that felt a little concerning to me so I'm going to refactor and post the "after" (it's way faster now)

[tmountjr]

Okay, the refactor of brightness() - I figured that since we didn't really care if there were three or four channels per pixel (we're changing all the pixels, which means all the channels of each pixel), we could just go over the buffer byte by byte and calculate the new value for whatever channel of whatever pixel we happened to be on for that iteration. Cut it from 4000 ticks to 1000 (ish).

	Original Timing	New Timing
Startup	517	756
fill()	556	898 (brightness=1) 627 (brightness=None)
brightness(0.25)	n/a	1093

[dpgeorge]

(See related micropython/micropython#3623 and micropython/micropython#4815)

[tmountjr]

What are the next steps with this PR? I see the two closed PRs on the main micropython repo but I'm not clear if I need to make any changes in light of those. Thanks!

[IHOXOHI]

Hi,
I don't understand something...
I have just add a brightness function to your lib, and I have only 173 us for a fill() and a write() without brightness function, and 256 with brightness function, on a pyboard v1.1...
On a feather M4, I have 249 us without brightness function, and 283 with it.
On other way, I have problems to use strip, which contains severals neopixels, with the classical ws2812 lib (spi) too... Electrical problems I think...
Your lib with the brightness function is therehttps://www.cosmocratie.fr/data.html

[tmountjr]

@IHOXOHI any chance you can put that code in a gist for me? the link you provided looked a little sketchy.

Is your question why the timing is different? I'm not sure I know what you're asking for help with.

[dlareau]

Just commenting that I went looking for this functionality today only to wind up at this PR. It isn't too much of a lift to simply implement equivalent functionality for myself for the time being, but I would love to see this PR eventually get merged.

[IHOXOHI]

Ok, I will post It on GitHub... M'y first.
M'y question is about the color which depends of a frequency, interfere with the frequency which controls each place of each neopixel... I presume.
..
Is there anybody who succeeded to use a big strip of bigs neopixels? More than 5...

The lib modified: github.com/IHOXOHI/neopixel-brightness-function/blob/main/README.md