Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle all 32 possible layer colors and correctly handle brightness scaling #18

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

khoek
Copy link

@khoek khoek commented Apr 4, 2024

This is a follow-on from #17, rebased on top of it so it is easier to review/merge. With the same goal as another pending pull request (roughly speaking), we simplify the layer display code (delete ~100 lines net) while adding support for indicating all 32 possible layers. We also change the code to use the config option CONFIG_ZMK_RGB_UNDERGLOW_BRT_SCALE as it is documented in the config files, namely by interpreting the option as a brightness percent out of 100.

The method we choose to color the layers is hopefully the one least surprising to users: the currently supported 8 colors remain identically represented on both modules. Numbering the 8 colors in our palette 0, 1, 2, 3, ... etc., with 0 corresponding to black/off, this corresponds to (0, 0), (1, 1), (2, 2), etc. After, (7, 7), we wrap back around (ignoring 0/off because that is confusing) to (1, 2), (1, 3), etc., incrementing the color on the right module while leaving the color on the left module fixed. Once we reach (1, 7) we wrap back around to (2, 1), and then skip (2, 2) (because it corresponds to one of the very first 8 layers) to get (2, 3), and so on, up to the last/32nd color pair (4, 7).

The idea is that current colors don't change, and that advanced users with more than 8 layers get visual feedback that their layer is changing (and each combination uniquely corresponds to a single layer).

Also note that since the default value of the config option CONFIG_ZMK_RGB_UNDERGLOW_BRT_SCALE is 1(%), and this was formerly being used as a raw pixel value, this means that those items would now have brightness with raw pixel value 1/100 * 255 ≈ 2.5 i.e. 2.5 times brighter (moving from 1% to 2.5% of max brightness). I don't think this looks bad in real life, but for approximate consistency I have inserted a factor of 1/2.5 in the definition of the scaling multiple LED_RGB_SCALING_MULTIPLE (but I would be happy to remove this, though I can't imagine someone really wanting the LEDs to be brighter than 40% of max power!).

Tested on a real Adv360 Pro.

@khoek khoek force-pushed the adv360-layer-colors branch from 8edeb3c to 87c1568 Compare April 4, 2024 11:01
})

static struct led_rgb zmk_rgb_underglow_scaled(struct led_rgb raw) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I turned this into a macro to make sure that it can be simplified at compile time, avoiding many unnecessary floating point operations.

@khoek khoek force-pushed the adv360-layer-colors branch from 87c1568 to 8218fbd Compare April 4, 2024 11:15
@khoek
Copy link
Author

khoek commented Apr 4, 2024

Sorry, I realized on review that the blink function could be simplified, and have pushed a change.

@ReFil
Copy link
Owner

ReFil commented Apr 5, 2024

It would be good to document somewhere what colours the layers correspond to in the main config repo, maybe in a table or something?

@khoek khoek force-pushed the adv360-layer-colors branch from b4e21eb to 2ef469c Compare April 6, 2024 16:38
@khoek
Copy link
Author

khoek commented Apr 6, 2024

@ReFil I've made a markdown explanation including a lookup table with color swatches; but looking at the KinesisCorporation/Adv360-Pro-ZMK repo, I'm not sure where to put it. Do you have any guidance? (Same question about where to document the new config option in #19.) Thanks!

@ReFil
Copy link
Owner

ReFil commented Apr 6, 2024

Custom config options can go in the main readme.md in their own subheading. I'd put the colour table in a file called layers.md in assets/ and link to it from the main readme.md, maybe in the section on customising the keymap?

@ReFil
Copy link
Owner

ReFil commented Apr 8, 2024

Merged the battery code but now it's complaining about conflicts, please can you rebase it and then i'll get this one merged?

@khoek khoek force-pushed the adv360-layer-colors branch from 2ef469c to 57171dc Compare April 9, 2024 08:00
@khoek
Copy link
Author

khoek commented Apr 9, 2024

So sorry about breaking the merge, while preparing my pulls for ZMK core I totally messed this branch up and then fixed it wrong. Hopefully everything is good now.

@ReFil ReFil merged commit 8855e0b into ReFil:adv360-z3.5 Apr 9, 2024
31 of 32 checks passed
@khoek khoek deleted the adv360-layer-colors branch April 10, 2024 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants