Feature: RGB Animation Driver API

[ice9js]

This is an initial stab at a general LED animation API, previously discussed in #554 and on ZMK Discord. While it was originally meant to be a solution for per-key RGB, I believe that the way it's implemented here, it could serve to fulfill any illumination needs and handle multiple zones.

Note: this is not a complete implementation yet but a foundation on which the next steps can be built. While this design allows for stuff like animation composition or special per-key lighting behaviors and effects, I think this would be something to expand to in follow up PRs.

Let's dive in:

There's three layers to this implementation:

• The core animation system responsible for scheduling frames, keeping track of what animations are assigned to which pixels and sending the output off to the led_strip drivers.
• Animation drivers. Hat tip to @joelspadin for suggesting the idea of having the animations rely on Zephyr's driver API. This makes it easy to configure and optimize different animation types, which in turn allows for animation composition down the line.
• Utilities. A set of structs and functions that should make working with color easier and ideally should be shared between the different animations instead of each one running its own, duplicating code and taking up space.

Design choices

• animation_api is fairly minimal right now, with just two methods: animation_prep_next_frame and animation_get_pixel. I imagine it would be expanded in the future, for example we could check if an animation is on or off or maybe do some different things but I didn't want to go over the top with everything in this initial draft.

• The way animations are handled is we first iterate through every animation with animation_prep_next_frame which allows animations to optimize certain tasks that can be done up front without knowledge about specific pixels. After that, animation_get_pixel is called where each individual pixel is calculated.

• zmk_color_rgb uses float values for red, green and blue. The reason for this would be better precision in any intermediate calculations, but also allow for different blending modes when combining layer. To give an example: it'd be possible to stack a solid layer with an effect gray-scale layer - which can effectively become an alpha-layer if you just multiply both frames. But only given the channels are in the range (0...1). Hence the final conversion to 0-255 channel values (led_rgb) only happens before the values are sent off to the drivers.

• I've chosen HSL as the configuration value to have more options down the line in terms of moving between colors - depending on what any given animation driver tries to achieve, it might make sense to convert to RGB straight away or work within HSL color space and only convert to RGB at the end. Different color spaces produce different gradients/transitions.

• Related to the last note, HSL uses the same set of parameters as HSLuv or LCh color spaces which have the perk of perceptually uniform brightness (making the transitions very smooth) but that comes at a performance cost. It might still be cool to experiment with that down the line which is why I opted to stick with HSL instead of HSV/HSB - in which case another conversion would be necessary.

• While I haven't mentioned lighting zones anywhere yet, this can be accomplished by assigning certain pixels to different animations as well as using independent x,y coordinates for each 'zone'. With animation composition, it'd be possible to share the base animation between different 'zones' but only have an effect applied on certain areas. For example, CapsLock could be in its own zone which uses whatever the rest of the keys use as base, with a caps lock indicator animation applied on top of it.

Config

The config is a big on the heavy site, but then it's also quite flexible. Either way, there isn't really a way of getting around it as each pixel must have x and y coordinates as well as an animation assigned to it - so it's more of a question where to put it. That said, I'm very much open to suggestions for potential improvement here.

/ {
    animation {
        // Phandle array for drivers which will be responsible for displaying the animation.
        // Multiple drivers are supported.
        drivers = <&ledstrip0 &ledstrip1 ...>;

        // List of all unique animations being used.
        animations = <&animation0 &animation1 ...>;
        
        // Each pixel definition starts with the assigned animation phandle
        // followed by x and y coordinates
        pixels
            = <&animation0 0 0>
            , <&animation0 255 255>
            , ...
            ;
    };
};

Eventually the pixel definition would/should be expanded by a key code or possibly a key position - to allow for per-key effects.

My full testing configuration can be found here.

---

Curious what your thoughts are!

[ice9js]

I implemented a per-key 'ripple' effect to see how that could be done and proceeded to make a few improvements to the previous code:

• Replaced animation_prep_next_frame with optional animation_on_before_frame and 'animation_on_after_frame` hooks. That way it's up to the individual animation to choose which works best for it or if they're necessary at all.
• Added a key-pixels setting where you can manually assign a pixel to each key. When omitted, it assumes that for N keys, it should use the first N pixels, saving a little space.
• Added zmk_animation_get_pixel_distance to return pre-calculated distances between different pixels for animations that might need it. Since it's a relatively expensive calculation, a lookup-table made much more sense than calculating these for every pixel in every frame. That said, as these tables can take up a bit of space themselves (~20kB for 100 pixels), I've put it behind the CONFIG_ZMK_ANIMATION_PIXEL_DISTANCE flag which would allow you to omit that code if it's not required. There's probably also room for optimization - the size could potentially be cut in half because half of the table is just mirrored anyway.

With this out of the way, the next goal would be to add the so called 'composition animations'. Starting with one to overlay multiple animations on top of each other, followed by another to cycle between different animations.

[joelspadin]

/ {
    animation {
        // Phandle array for drivers which will be responsible for displaying the animation.
        // Multiple drivers are supported.
        drivers = <&ledstrip0 &ledstrip1 ...>;

        // List of all unique animations being used.
        animations = <&animation0 &animation1 ...>;
        
        // Each pixel definition starts with the assigned animation phandle
        // followed by x and y coordinates
        pixels
            = <&animation0 0 0>
            , <&animation0 255 255>
            , ...
            ;
    };
};

I'm not sure I understand how this works. animations contains a list of animations, but then pixels has each pixel point to a specific animation. Is animations just a list of the set of animations that is used in pixels, and if you use something in pixels but forget to list it in animations everything breaks? If so, that seems like it could be error prone.

Also, how are pixels mapped to drivers/LED indices?

See my comment on the animation timer for more on what I was thinking, but it seems to me like it would be easier to configure things if you organized the data as

1. A list of pixels, each with an x/y position, LED driver, and LED index within that driver (or some other mapping between pixel and LED if there's a more efficient way to do this). A pixel can be identified by its index in this list, just like how we identify key positions.
2. A mapping between key position and pixel index (with a special value to say that a key does not have a pixel)
3. Each animation driver instance would know which pixels it should update instead of each pixel knowing which animation it should use.

[ice9js]

Thanks again for the review! To answer your questions:

I'm not sure I understand how this works. animations contains a list of animations, but then pixels has each pixel point to a specific animation. Is animations just a list of the set of animations that is used in pixels, and if you use something in pixels but forget to list it in animations everything breaks? If so, that seems like it could be error prone.

The separate list of animations is currently necessary to keep them in sync as they run off a single timer. The issue is that since it's only possible to assign a single animation per pixel, there could be 'composed' animations downstream that aren't necessarily assigned to any pixel directly but need to update as well.
That said, this is a limitation of the per-pixel rendering approach though so maybe something that could be simplified given your other suggestions.

Also, how are pixels mapped to drivers/LED indices?

1:1. The order in which they appear in the pixels array should match the order of the led_strip. If there are more than one LED strip, say one with N and another with M pixels, than pixels 0...N-1 would be sent to the first driver and N...N+M-1 to the second.

I don't think it's necessary to map animation pixels to specific led_strip pixels since only their physical position X/Y should ever matter to the animation drivers. So it's something you'd have to be careful about but you only ever have to do it once for a board.

See my comment on the animation timer for more on what I was thinking, but it seems to me like it would be easier to configure things if you organized the data as

A list of pixels, each with an x/y position, LED driver, and LED index within that driver (or some other mapping between pixel and LED if there's a more efficient way to do this). A pixel can be identified by its index in this list, just like how we identify key positions. A mapping between key position and pixel index (with a special value to say that a key does not have a pixel)

Sounds good. If I try the 'render buffer' approach I wouldn't need per-pixel animation assignment.
As for mapping 'animation leds' to 'led driver leds' - like I said, I think it's useless. Especially when running matrix drivers.

I already added a key-pixels setting (feel free to suggest a better name) which allows you to map pixels to key positions. When left unspecified, it assumes that if you've got N keys, the first N pixels correspond to those key positions.

Each animation driver instance would know which pixels it should update instead of each pixel knowing which animation it should use.

And just to be clear... would we still have an animations array or are you suggesting each animation should 'register' itself during initialization/activation?
Like I mentioned, this might get a bit tricky as some animations would need to be ran by the animation system itself, whereas others would only be used by other 'composition' animations. And then there's a few that would be used by both.

[joelspadin]

And just to be clear... would we still have an animations array or are you suggesting each animation should 'register' itself during initialization/activation?

In the idea I suggested, there wouldn't need to be an animations array or registration. One animation would be chosen as the initial animation (either by convention as the first animation, or better using a /chosen node property), and it would form the root of a tree. This animation could optionally have child animations. The system animation controller would activate that main animation when we enable the backlight and call the animation tick/update function for only the main animation. Each animation would be responsible for calling its child animations' activate, deactivate, and tick/update functions as needed.

Each animation could optionally register for event handlers during activation, or request that the system repeatedly run the tick/update function on a timer, but it would not need to register the animation itself with the system.

[ice9js]

Thanks! Appreciate your input!

I'll give the tree/full-px-buffer approach and see how it comes out or if I any serious blocker would pop up.
An animation graph seems like it could be slightly more convenient but again it's probably not something you'd be changing much once you've got your initial config.
Thinking about it, with pixel masks, it should be possible to convert any use case for multiple parents with a few 'transparent layers' for any of those pixels so yeah, let's see.

[joelspadin]

Cool. I'll be interested to see how well it works, since I've quite possibly not thought through everything.

Another idea I had, which I think would work well with the tree approach, but also the way you have things here already could possibly be adjusted to make it work: we could provide .dtsi files with pre-built animations that you could customize. For example you could write out all everything yourself:

/ {
  chosen {
    zmk,animation = &animation_select;
  };
  lighting {
    drivers = ...;
    pixels = ...;
  };
  animation_select: animation_select {
    compatible = "zmk,animation-select";
    animations = <&animation0 &animation1>;
  };
  animation0: animation0 {
    compatible = "zmk,animation-solid-color";
    color = <255 255 255>;
    allow-color-adjust; // Could add an option to adjust hue/brightness via key behaviors
  };
  animation1: animation1 {
    compatible = "zmk,animation-rgb-wave";
  };
};

But we could also move some of the boilerplate for common animations into .dtsi files

#include <animation-select.dtsi>
#include <animation-solid-color.dtsi>
#include <animation-rgb-wave.dtsi>

/ {
  chosen {
    zmk,animation = &animation_select;
  };
  lighting {
    drivers = ...;
    pixels = ...;
  };
};

// included &animation_select node needs to be given a list of animations to cycle through.
&animation_select {
  animations = <&animation_solid_color &animation_rgb_wave>;
};

&animation_solid_color {
  color = <255 0 0>; // I want my solid color backlight to start as red instead of white.
};

which works so long as you don't want two copies of the same type of animation anywhere.

[joelspadin]

1:1. The order in which they appear in the pixels array should match the order of the led_strip. If there are more than one LED strip, say one with N and another with M pixels, than pixels 0...N-1 would be sent to the first driver and N...N+M-1 to the second.

This seems reasonable, but I have one concern with this approach: say you have a keyboard with 100 LEDs, you used two 8x8 matrix drivers to cover them. Would you need to define all 128 pixels with 28 of them being dummy values? Will this make things annoying to configure?

Potential solutions:

1. Make the LED driver report that it only has 50 LEDs and have an internal mapping from index to row/column.
2. Add something similar to the keymap transform, but for LEDs.

(I expect that either of those solutions would use as much or more memory than we were wasting by having 28 extra pixels, so my concern here is purely for ease of use and not for wasted resources.)

Also, this is an idea for a future improvement and not something we should do now, but it would be worthwhile considering it in the design: it would be really cool if we could also support regular LEDs as pixels, so we could eventually have a single lighting system that handles not only per-key RGB and RGB underglow, but also PWM backlighting and status LEDs. Maybe that could be done by adding an RGB strip driver that wraps a regular LED driver?

[ice9js]

This seems reasonable, but I have one concern with this approach: say you have a keyboard with 100 LEDs, you used two 8x8 matrix drivers to cover them. Would you need to define all 128 pixels with 28 of them being dummy values? Will this make things annoying to configure?

Yeah, the led_strip driver should take care of it. So you'd only ever have 100 pixels, and then the drivers are responsible for mapping these to the correct pixels. The chain-length property on the LED driver configuration tells how many LEDs the given driver supports.
While it's not part of the official API, I noticed RGB Underglow using that same property for the same purpose hence my choice to just continue with that.

For pretty much anything else than chains of 'smart LEDs' the driver would have to perform some mapping anyway. For example, led_strip_update_rgb passes an array of rgb pixels, but with a matrix driver it's difficult to assume anything about how the LEDs for different channels are wired. Take a look at #866 for how I solved that for IS31FL3741 that I'm using to test this.

Also, this is an idea for a future improvement and not something we should do now, but it would be worthwhile considering it in the design: it would be really cool if we could also support regular LEDs as pixels, so we could eventually have a single lighting system that handles not only per-key RGB and RGB underglow, but also PWM backlighting and status LEDs. Maybe that could be done by adding an RGB strip driver that wraps a regular LED driver?

Yes! Very much. In my case, I actually have a few status/indicator LEDs apart from key LEDs but they're just wired up to the matrix driver which I think is much more convenient as long as you don't run out of pins there. Then it's a balance if it's worth moving up to larger or multiple ICs or sacrificing a few GPIO pins. But generally, anything that conforms to the led_strip api could be used as a driver. So a single LED could be considered a strip with length 1.
The only difference between key and not-key LEDs is there wouldn't be any key mappings for the latter.

[ice9js]

I just pushed some stuff I had already done before our last conversation - including a 'compose' animation with a few blending modes. Attaching a quick demonstration of a setup with a 'ripple' animation on top of solid color layers (different for modifiers and keys). In the first example, the ripple is multiplied with the underlaying color, in the latter it's used as a mask. So in general, there's quite a lot of flexibility and different effects one could achieve in this way.

https://user-images.githubusercontent.com/8056203/146426449-ea2ea95b-5378-433b-9094-cbc916d23220.mov https://user-images.githubusercontent.com/8056203/146426464-9e366da9-1ad6-44a4-b416-73e65d6741d2.mov

I'm happy with my test setup, my attention is now going towards converting that to the array-based approach.

[ice9js]

For a start, I updated my configuration taking into account what we talked about.
Please take a look when you have a minute @joelspadin, to make sure I didn't miss anything.

I added a secondary file with macros for various lighting zones. Defining pixel masks manually for on each animation is a tedious job. Defining macros for some general areas on a board makes this much more bearable, but more importantly, when reading the configuration it's much more obvious what the animation is targeting.

The only issue I came up agains here is with the compose animation. I removed its blending-modes property, to have it added to individual animations - with BLENDING_MODE_NORMAL being a default.
When updating the full buffer at once, it makes much more sense for individual animations to apply this logic as it saves the need for yet another buffer on the compose animation, as well as prevents any issues, where blending modes could be applied incorrectly as the composing animation has no reference of the underlaying animations' masks.
We could provide a standard function to apply the blending mode while copying an animations' buffer into the main pixel buffer for animations to use. Even make it inline if need be. Custom animations could, of course, ignore this setting if deemed unnecessary.

Other than that, I used an XY( a, b) macro for defining pixel positions, which doesn't do anything really and would translate to just a b. Reason I did it is again, for readability. It'd be hard to tell what's what if it was just a plain list of numbers. Or we could go back to <&pixel x_pos y_pos> phandle-array approach. Curious what you think.

[joelspadin]

I might not have time to take a look at this right away, but I'll try to get to it probably this weekend.

I hadn't even considered using blending modes. Are there particular effects where that would be useful? I had mainly been considering effects where one animation would simply overwrite the results of another instead of blending them, so I was thinking for the sake of simplicity and efficiency, we might just give animations direct access to the main pixel buffer instead of having separate buffers and compositing them. I guess blending could also be done that way by having an animation read the color already in the buffer and modify it, but that would certainly be less flexible.

[ice9js]

I hadn't even considered using blending modes. Are there particular effects where that would be useful?

Yeah, I do agree it's a bit of a gimmick but I see two main use cases here:

• With something like BLENDING_MODE_MULTIPLY, that layer effectively becomes an opacity-mask for whatever is below it. I used that on one of the examples above. To give another use case, going back to the layers example. You could give the layer-highlight-animation a white color, and use BLENDING_MODE_MULTIPLY or BLENDING_MODE_SUBTRACT. That way whatever you already had going on in terms of animations keeps going, but only the layer or non-layer keys remain lit - instead of limiting the layer-highlight-animation to being able to just display a single solid color. (Which it of course still can do).

• If you have effects on top of a 'normal' animation that produce gradients, you might want to use something like BLENDING_MODE_LIGHTEN. If the gradient was to overwrite the values below, it'd be quite a harsh transition - and with how far pixels are spaced apart on a keyboard, probably result in some sort of annoying flicker. With blending modes, it's possible to achieve a smooth transition from the underlying animation into the effect color.

All that said, having experimented with things a bit already... well, let's say it's not photoshop. In my case, my matrix driver supports 8-bit PWM values, but since the LEDs need gamma adjustment as well, that already cuts it down to just 64 levels of brightness per channel.
And then it comes down to the quality of the LEDs themselves and what you put over them. So in the end, I'm actually finding it quite difficult to accurately reproduce colors and produce clear, easily distinguishable transitions. Either way, I'd consider this more of a driver/hardware issue than anything else. But I thought it might be interesting to share.

[ice9js]

I updated the API to run just a single call per animation and managed to preserve all of the functionality so I'm happy with that.

Regarding the blending modes, I created a zmk_apply_blending_mode function, and it's up to the animation to use it or not. As this still needs to be called for every pixel (to avoid having duplicated buffers), we'd very soon end up where we were previously. To help with that, the function is actually a thin wrapper that'll be inlined, as with something like BLENDING_MODE_NORMAL which will by far be the most common you don't have to do anything anyway, and only call the underlying blending function for more complex cases.
To compare, it'd only be called by animations that actually use it, and only for pixels where it matters.

I also made a small tweak to the pixel distance lookup table - by normalizing the range to fit inside 0-255 (previous maximum was 360) it'll cut the size in half. It makes sense as for 100 LEDs, it's already difference between 10 and 20 kB.

I'm going to tackle scheduling next so we're not constrained by a fixed refresh rate.

[ice9js]

I did some exploring on the frame scheduling so we're not running at a constant 30 FPS and I opted to go with a zmk_animation_request_frames() function, where any animation can request a number of subsequent frames to be rendered.
This allows for static animations to be rendered only once, effects can request a precise amount of frames they need to complete an animation and cyclic animations would need to refresh the counter - presumably anytime their internal cycle counter resets. When the requested frames counter drops back to 0, the timer is stopped and no renders are performed until the next request.

I also added animation_start() and animation_stop() functions to the API, which would be run anytime an animation is (re)started or stopped.

Additionally... I've been thinking about stuff like controlling the brightness/turning animations on or off/etc. The more I think about it, the more convinced I am all of these should be handled through an animation themselves, and could possibly be paired with the cycle/animation-select example that was mentioned. The animation system itself would always keep on running unless the keyboard goes into sleep mode and all animations are disabled.

The reasoning here is, you might want to be able to turn off the lights only for certain zones rather than all the lights. Similarly, you could put any indicator animations on top of those 'control' animations, so they wouldn't be affected even when you 'turn off rgb', which I think makes sense.

[joelspadin]

I haven't had time to look at the code yet, but requesting a specific number of frames seems reasonable to me. I think it might simplify some cases while making others more complex though, and it seems like it would be more complicated to implement since you would need to either track individual counters for every animation or just have a single counter that you set to max(current, requested) each time an animation requests more frames. Ideally, you'd also cancel any outstanding requested frames from an animation when that animation is deactivated in the middle of a cycle (e.g. when the user switches which animation is playing), which I think would require individual counters for each animation.

An animation that can't calculate how many frames it needs up front would need to keep requesting additional frames at each frame. As you mentioned, cyclical animations would also need to request another cycle of frames as each cycle completes instead of getting a timer ref when the animation is activated and releasing it when it is deactivated. I think I'll need to take a look at the code and see how an animation implemented this way vs. using a ref count compares before I could say which approach is better.

[joelspadin]

Additionally... I've been thinking about stuff like controlling the brightness/turning animations on or off/etc. The more I think about it, the more convinced I am all of these should be handled through an animation themselves, and could possibly be paired with the cycle/animation-select example that was mentioned. The animation system itself would always keep on running unless the keyboard goes into sleep mode and all animations are disabled.

I like this idea. We could also implement behaviors such as turning off LEDs based on idle, powered vs. battery, etc. as animations (complete with smooth fade in/out if you wanted), and then you could layer things on top of that if for example you wanted status LEDs to keep showing even when idle.

[ice9js]

I think I'll need to take a look at the code and see how an animation implemented this way vs. using a ref count compares before I could say which approach is better.

Sure, take your time :) I'll just mention that I played with that approach as well but decided against it as I found it made things much more complex and/or error prone in the end.
Like, you'd either have to introduce additional variables to track the current 'subscription' state of an animation or be extremely careful when using these or you'll very quickly end up with race conditions causing the timer to run indefinitely or not run at all.

or just have a single counter that you set to max(current, requested) each time an animation requests more frames.

That's pretty much where I'm at right now.

Ideally, you'd also cancel any outstanding requested frames from an animation when that animation is deactivated in the middle of a cycle (e.g. when the user switches which animation is playing), which I think would require individual counters for each animation.

And this is a good point, which I believe isn't quite possible with just one counter. So it'd be a tradeoff between rendering a few - or potentially thousands - of extra frames vs possibly increasing complexity by quite a lot.
Considering those animations' render functions wouldn't be called anymore since they're in inactive state, and that we'd suspend the timer when going into sleep mode anyway regardless of how many frames are left... I'd consider it acceptable given whatever power the CPU is using is likely insignificant compared with the current drawn by the LEDs themselves.

A possible compromise could be to set a limit on the number of frames an animation can request at any given time.
Or define some function to periodically check-in to see if there's any animation that still needs to be re-rendered.
Or maybe some kind of event, so if any animation get deactivated the counter resets to 0, and we'd need to notify remaining animations so they can re-request frames if necessary.

[joelspadin]

I think rendering extra, unnecessary frames after switching animations is probably not that big of a deal. It would only matter if animations didn't need to be updated, and then it would just be wasting CPU time instead of causing any problems, because those animations would just keep generating the same state. Notifying animations that the frames they request have been canceled seems like it would just make writing animations more difficult for not much benefit.

It might be useful to put some sort of cap on the number of frames you can request though, so an animation can't accidentally request days' worth of animation time.

[ice9js]

I added the control animation and behaviors for cycling through animations/changing brightness etc. and also fixed a few issues I noticed along the way.

I still need to update the animation 'core' to add a sleep mode but I believe that would make the implementation somewhat feature complete. I don't necessarily want to go crazy with animations themselves here - they should be easy enough to add afterwards and it's not like I can think of everything anyway.

[ice9js]

I updated animation.c to stop any ongoing animations when the system goes into sleep mode - I guess it still makes sense for them to remain on in IDLE?
I used ZMK_ACTIVITY_ to detect state, but it could be rewritten to use Zephyr's device power management - ZMK's own events are... let's say easier to work with but I'm not quite convinced if these can be treated as the same exact thing so some input there would be appreciated.

I also added the code which will store the current settings for each animation_control instance in flash memory.
And with that, I believe the implementation is feature complete now.

[ice9js]

I ran some benchmarks to investigate floating- vs. fixed-point performance on an nRF52840 (Cortex M4F, contains an FPU) our of my own curiosity.

My initial results yielded ~15000 cycles for the floating point implementation compared to ~900 for fixed point math. The difference seemed quite large and indeed, I quickly discovered the code was still compiling using the standard library and not FPU instructions.
Setting CONFIG_FPU=y fixed the issue, although it's worth noting Cortex M4F's FPU is only single-precision and any double operations would still be done in software. (It seems to be the most popular core used in the community?)

FPU brought down the difference from ~15x to just 1.5x slower with ~1450 cycles for the floating point implementation on average. This is still a noticeable difference but I'm not sure it's enough to warrant switching to fixed point arithmetic on its own. That said it's worth remembering not every MCU comes with an FPU-equipped core.

A few other considerations:

• Using uint8_t for rgb channel values internally would cut down size of zmk_color_rgb from 12 bytes to just 3. Since there only is a single buffer for these anyway, the difference would need to be multiplied by the amount of LED's on the board to get more or less real savings. For 100 LEDs, the difference would be 900 bytes. Again, not insignificant but also not a game-changer considering overall firmware size.
• Not all MCU's come with FPU-equipped CPU cores. The code would run 10x slower on presumably already slower hardware.
• Calculating fractions using fixed point arithmetic is somewhat more difficult and hard to read from a development point of view.
• It's of course possible to provide a fixed-point alternative later and switch the implementation based on CONFIG_FPU, but one would have to be very thorough to do this for all animations as both underlying types would support the same math operations yet the results would be vastly different - and not caught by the compiler. Having a single implementation for all is just easier to deal with.
• float is more precise although this could also be addressed by moving up to uint16_t in the fixed point implementation. This doesn't really matter for a single conversion but the differences would become more exaggerated the more operations the value goes through - like blending modes or gradients. That said, it would probably still be indistinguishable to the human eye when looking at bare LEDs.
• zmk_hsl_to_rgb has probably the biggest floating point operation concentration of the entire animation api implementation. As such, the difference would also be exaggerated more with this example compared to blending or converting pixel values which are single multiplications.

I'm curious what your thoughts are on this. I believe this might also be useful information for #926.

Benchmark code:

#define ITERATIONS 1000

struct fhsl {
    uint16_t h;
    uint8_t s;
    uint8_t l;
};

struct frgb {
    float r;
    float g;
    float b;
};

static float fmod(float a, float b) {
    float mod = a < 0 ? -a : a;
    float x = b < 0 ? -b : b;

    while (mod >= x) {
        mod = mod - x;
    }

    return a < 0 ? -mod : mod;
}

static float fabs(float a) { return a < 0 ? -a : a; }

void fhsl_to_frgb(const struct fhsl *hsl, struct frgb *rgb) {
    float s = (float)hsl->s / 100;
    float l = (float)hsl->l / 100;

    float a = (float)hsl->h / 60;
    float chroma = s * (1 - fabs(2 * l - 1));
    float x = chroma * (1 - fabs(fmod(a, 2) - 1));
    float m = l - chroma / 2;

    switch ((uint8_t)a % 6) {
    case 0:
        rgb->r = m + chroma;
        rgb->g = m + x;
        rgb->b = m;
        break;
    case 1:
        rgb->r = m + x;
        rgb->g = m + chroma;
        rgb->b = m;
        break;
    case 2:
        rgb->r = m;
        rgb->g = m + chroma;
        rgb->b = m + x;
        break;
    case 3:
        rgb->r = m;
        rgb->g = m + x;
        rgb->b = m + chroma;
        break;
    case 4:
        rgb->r = m + x;
        rgb->g = m;
        rgb->b = m + chroma;
        break;
    case 5:
        rgb->r = m + chroma;
        rgb->g = m;
        rgb->b = m + x;
        break;
    }
}

struct ihsl {
  uint8_t h;
  uint8_t s;
  uint8_t l;
};

struct irgb {
    uint8_t r;
    uint8_t g;
    uint8_t b;
};

void hsl_to_rgb(const struct ihsl *hsl, struct irgb *rgb) __attribute__((noinline));
void hsl_to_rgb(const struct ihsl *hsl, struct irgb *rgb) {
    uint8_t hue = hsl->h * 192 >> 8;

    uint8_t segment = hue / 32;
    uint8_t offset = hue << 3;

    uint8_t chroma = (hsl->s * (255 - abs(2 * hsl->l - 255))) >> 8;
    uint8_t x = (chroma * abs(offset - (segment % 2 == 0 ? 0 : 255))) >> 8;
    uint8_t m = hsl->l - chroma / 2;

    switch (segment) {
    case 0:
        rgb->r = m + chroma;
        rgb->g = m + x;
        rgb->b = m;
        break;
    case 1:
        rgb->r = m + x;
        rgb->g = m + chroma;
        rgb->b = m;
        break;
    case 2:
        rgb->r = m;
        rgb->g = m + chroma;
        rgb->b = m + x;
        break;
    case 3:
        rgb->r = m;
        rgb->g = m + x;
        rgb->b = m + chroma;
        break;
    case 4:
        rgb->r = m + x;
        rgb->g = m;
        rgb->b = m + chroma;
        break;
    case 5:
        rgb->r = m + chroma;
        rgb->g = m;
        rgb->b = m + x;
        break;
    }
}

static struct fhsl colors_hsl_float[] = {
    { .h = 93, .s = 100, .l = 74 },
    { .h = 66, .s = 83, .l = 67 },
    { .h = 49, .s = 100, .l = 69 },
    { .h = 41, .s = 100, .l = 71 },
    { .h = 31, .s = 100, .l = 75 },
    { .h = 16, .s = 100, .l = 80 },
    { .h = 355, .s = 100, .l = 84 },
    { .h = 334, .s = 100, .l = 84 },
    { .h = 319, .s = 100, .l = 84 },
    { .h = 301, .s = 80, .l = 83 },
    { .h = 266, .s = 100, .l = 86 },
    { .h = 232, .s = 100, .l = 87 },
    { .h = 206, .s = 100, .l = 79 },
    { .h = 192, .s = 100, .l = 67 },
    { .h = 185, .s = 100, .l = 50 },
    { .h = 183, .s = 100, .l = 50 },
    { .h = 181, .s = 100, .l = 50 },
    { .h = 173, .s = 100, .l = 50 },
    { .h = 161, .s = 100, .l = 60 },
    { .h = 133, .s = 100, .l = 75 },
};

static void float_benchmark() {
    struct frgb rgb;

    const int64_t start_ticks = k_uptime_ticks();

    for (int i = 0; i < ITERATIONS; ++i) {
        for (int j = 0; j < 20; ++j) {
            fhsl_to_frgb(&colors_hsl_float[j], &rgb);
        }
    }

    const int64_t elapsed = k_uptime_ticks() - start_ticks;
    LOG_INF("Float HSL->RGB: %d ticks", (int) elapsed);
}

static struct ihsl colors_hsl_int[] = {
    { .h = 66, .s = 255, .l = 189 },
    { .h = 47, .s = 212, .l = 171 },
    { .h = 35, .s = 255, .l = 176 },
    { .h = 29, .s = 255, .l = 181 },
    { .h = 22, .s = 255, .l = 191 },
    { .h = 11, .s = 255, .l = 204 },
    { .h = 251, .s = 255, .l = 214 },
    { .h = 237, .s = 255, .l = 214 },
    { .h = 226, .s = 255, .l = 214 },
    { .h = 213, .s = 204, .l = 212 },
    { .h = 188, .s = 255, .l = 219 },
    { .h = 164, .s = 255, .l = 222 },
    { .h = 146, .s = 255, .l = 201 },
    { .h = 136, .s = 255, .l = 171 },
    { .h = 131, .s = 255, .l = 128 },
    { .h = 130, .s = 255, .l = 128 },
    { .h = 128, .s = 255, .l = 128 },
    { .h = 123, .s = 255, .l = 128 },
    { .h = 114, .s = 255, .l = 153 },
    { .h = 94, .s = 255, .l = 191 },
};

static void int_benchmark() {
    struct irgb rgb;

    const int64_t start_ticks = k_uptime_ticks();

    for (int i = 0; i < ITERATIONS; ++i) {
        for (int j = 0; j < 20; ++j) {
            hsl_to_rgb(&colors_hsl_int[j], &rgb);
        }
    }

    const int64_t elapsed = k_uptime_ticks() - start_ticks;
    LOG_INF("Integer HSL->RGB: %d ticks", (int) elapsed);
}

void run_benchmark(void) {
    float_benchmark();
    int_benchmark();
}

void main(void) {
    run_benchmark();
    run_benchmark();
    run_benchmark();
    run_benchmark();
    run_benchmark();
}

[Xoffio]

Hi guys! What happened to this pull? Is this still going to make it to the ZMK main branch ?

[ice9js]

Howdy! It seems like it's a bit of a status-quo currently, I've started this work to satisfy my own needs and I believe it's a solid proof of concept of how things could work and would be happy to build more functionality on top of it.
That said, I'm not sure how the priority list for ZMK looks like currently, and until any decision is made, it'd be unfortunate to put significantly more work into it only to see it scrapped eventually.

[shadow-identity]

@joelspadin sorry for pinging you, but is it planned to finally merge this pull request or you are looking for another solution?

[danieldegrasse]

Hi @ice9js- I've been integrating this PR into my ZMK fork, and I added some documentation while doing so: https://github.com/danieldegrasse/zmk/commit/5d40965487ca6d88e69dad29b449f4a95f1899bf If you're interested in integrating this commit into your branch, please feel free to do so. I'd also be happy to help move this forwards (via reviews or additional development) if you're interested.

[ice9js]

Really great writeup @danieldegrasse! Thanks for putting your time into it. And sure, I'd totally love to integrate your changes into my fork as well, just wondering what's the best way to go about it now as I was planning on getting back into doing some ZMK development myself 🤔.

If you're up for it, I'd love to hear some feedback or coordinate on some sort of roadmap where to potentially take this and split the workload a bit. It's been hanging around for a while by now - so I'm thinking committing to maintain a fork (or two) ahead of zmk:main might indeed be a good idea in the near term to get some early adopters and generate some momentum and interest around it, for it to eventually be merged.

[danieldegrasse]

And sure, I'd totally love to integrate your changes into my fork as well, just wondering what's the best way to go about it now as I was planning on getting back into doing some ZMK development myself 🤔.

Personally, I'd just fetch the branch I linked, and use git cherry-pick when your branch is checked out to apply that SHA on top of your work. It shouldn't create conflicts, since that commit only edits the docs folder.

If you're up for it, I'd love to hear some feedback or coordinate on some sort of roadmap where to potentially take this and split the workload a bit.

I'm happy to review this PR and provide feedback where possible. We can coordinate if need be, although it seems to me like a lot of the work to enable this feature is already present.

It's been hanging around for a while by now - so I'm thinking committing to maintain a fork (or two) ahead of zmk:main might indeed be a good idea in the near term to get some early adopters and generate some momentum and interest around it, for it to eventually be merged.

I'm admittedly not familiar with the development flow in ZMK- is this the way that new features are typically moved forwards? I assume long term the commit history will need to be cleaned up, and the documentation will need additional extensions. What shortcomings do you think need addressing to get this PR into a mergeable state?

[ice9js]

On another note, I've been having this thought lately, that 'zones' likely warrant their own driver/device.
Right now, they're just pixel_map[] arrays defined for each animation separately which works well but also comes with a few drawbacks:

• Unnecessarily large memory usage when you define multiple animations that affect the same area (for example you want to be able to switch between them).
• No way to update the zones dynamically.

Having an animation_zone device, would cut down on memory usage. While it might be acceptable on smaller boards with just a few LEDs, the difference becomes significant on larger setups. For example, my test board has 92 keys, if I set up 10 animations that apply to all the keys it'd require 14.4kB for just the pixel maps, whereas having a zone device would keep it constant at 1.44kB regardless of how many animations use the same zone.

The second point is still more important though IMO. We could allow zone devices to have space for more pixels than originally defined, and eventually allow updating them in real time through either the keyboard itself or from some sort of control software.

So I'm thinking about doing another small refactor on that but I'd be curious to hear what y'all think before I get started.

[joelspadin]

On another note, I've been having this thought lately, that 'zones' likely warrant their own driver/device. Right now, they're just pixel_map[] arrays defined for each animation separately which works well but also comes with a few drawbacks:

Having a way for multiple animations to share the information about which pixels they should draw to seems like a good idea to me. I'm not sure what the best way to do that is though.

Another idea would be to create a new led_strip driver which just forwards to some subset of another led_strip (or possibly joins together multiple?) and reorders pixels as needed (similar to keymap transforms?). Then you could, for example, split the LEDs into backlighting and underglow groups and run a separate animation on each one. Animations that need to further subdivide the LEDs or pick out specific LEDs might still need a pixel map or equivalent, but you wouldn't need that for every animation.

[sirpack]

Hey guys, non-dev here just wondering how this project is going. This is EXACTLY what I'm looking for for my Glove80