keyboardio / Kaleidoscope

Firmware for Keyboardio keyboards and other keyboards with AVR or ARM MCUs.
http://keyboard.io
GNU General Public License v3.0
759 stars 258 forks source link

The Numpad plugin should only highlight actual numpad keys #1416

Open EvyBongers opened 5 months ago

EvyBongers commented 5 months ago

What do you want to change?

The way the Numpad plugin currently works, it checks if the numpad layer is active and that keys on the layer don't have any modifiers (key flags) applied to them. For the default layouts in the provided firmware builds this works, but for Chrysalis users who repurpose layer it's far from convenient. In order to improve user experience in Chrysalis, I see a few options:

How will it make Kaleidoscope better?

It's not so much Kaleidoscope that would get better, but this would set the stage for improving user experience for Chrysalis users.

What trouble might users have in adapting to the new functionality?

In case of options 1 and 2, I don't see any trouble. Chrysalis users who use the numpad layer and plugin as is wouldn't notice any difference, as long as the default values are kept. Kaleidoscope users might have to update their sketch to match how the plugin parameters are set. In case of option 3, users would have to update their keymap to use only keypad keys, where the factory sketches for both M01 and M100 use the number row keys and 'normal' enter rather than their keypad counterparts. As stated above, using the keymap variants of the numbers and dot would be affected by the numlock state, though it might be possible to enable/disable numlock through the plugin.

What trouble might developers have in adapting to the new functionality?

I don't think there would be any differences outside of the plugin itself, except for now the existing plugin parameters are set.

obra commented 5 months ago

I'd actually really love to entirely get rid of the Numpad plugin in favor of a pre-configured color theme for that layer. It's never been good and dates back to before we had the option of custom per-layer LED color themes..

EvyBongers commented 5 months ago

Oh, that should be easy enough to do. Would that be for all example sketches or just for Keyboardio hardware?

obra commented 5 months ago

We should figure out what the upgrade scenario for the Model 100 sketch is. How do we make it not hurt for users flashing a new firmware onto their existing model 100?

And we should mark the old plugin as "don't use this for new sketches. do this simpler thing instead"

I think we leave the model 01 alone.

On Wed, Mar 27, 2024 at 9:27 AM Evy Bongers @.***> wrote:

Oh, that should be easy enough to do. Would that be for all example sketches or just for Keyboardio hardware?

— Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/1416#issuecomment-2023208646, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2EEIWZROXVYSY2OLT3Y2LQPTAVCNFSM6AAAAABFE5MNTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTGIYDQNRUGY . You are receiving this because you commented.Message ID: @.***>

EvyBongers commented 5 months ago

How do we make it not hurt for users flashing a new firmware onto their existing model 100?

I think this should cover it:

For the users in the third category of the second bullet, you should have an entry in NEWS.md (which you probably want anyway).

And we should mark the old plugin as "don't use this for new sketches. do this simpler thing instead"

The plugin detzt is working on combined with ActiveLayerKeys might make a sound alternative if you're gonna deprecate it

I think we leave the model 01 alone.

That's for you to decide. Replacing the numpad plugin with a colormap entry might free up some bytes, but in the end that decision is yours. I wonder how model 01 users would feel about this change.

obra commented 5 months ago

I guess the only thing I'd want to make sure worked is if someone had previously used chrysalis, but not configured custom color maps for that layer, that we wouldn't be breaking the functionality by removing the plugin.

I don't feel strongly about not touching the Model 01, so much as not feeling like touching it is a requirement to make this go.

On Wed, Mar 27, 2024 at 12:16 PM Evy Bongers @.***> wrote:

How do we make it not hurt for users flashing a new firmware onto their existing model 100?

I think this should cover it:

  • Remove the plugin from the default/example sketch
  • Define a colormap entry in the default/example sketch matching the default layout of the numpad layer
    • Users who don't use layer 1 shouldn't notice for obvious reasons
    • Newly sold M100s should get updated as soon as users start using Chrysalis
    • Users who have a custom colormap for that layer would notice the change if:
      • they assigned different colors on those keys (they would start to see the assigned colors)
      • they assigned colors only to keys that aren't in the numpad (they would see those keys have the leds turned off)
    • Add a deprecation messege to the plugin (both in the README and a compile time message)
  • Mention the change in the Kaleidoscope release notes

For the users in the third category of the second bullet, you should have an entry in NEWS.md (which you probably want anyway).

And we should mark the old plugin as "don't use this for new sketches. do this simpler thing instead"

The plugin detzt is working https://github.com/keyboardio/Kaleidoscope/compare/master...detzt:Kaleidoscope:led-effect-per-layer on combined with ActiveLayerKeys might make a sound alternative if you're gonna deprecate it

I think we leave the model 01 alone.

That's for you to decide. Replacing the numpad plugin with a colormap entry might free up some bytes, but in the end that decision is yours. I wonder how model 01 users would feel about this change.

— Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/1416#issuecomment-2023782344, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2CCZ73ENK2WYHKZHX3Y2MEIFAVCNFSM6AAAAABFE5MNTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTG44DEMZUGQ . You are receiving this because you commented.Message ID: @.***>

EvyBongers commented 5 months ago

Custom colormaps would behave differently anyway, because that require users to use that specific LED mode in order to see the keys light up. Users that use any other LED mode wouldn't see it in the first place.

If you do want to set a colormap for that layer, that seems like something best solved in Chrysalis imho. As this seems to apply only to users who work exclusively with Chrysalis, they would (most likely) flash new firmware with Chrysalis. Therefore restoring EEPROM at the end of that process could make sure to set those colors. Of course that does have some drawbacks and caveats:

Setting the colormap in Kaleidoscope is impossible except for newly delivered keyboards, because there is no way to check what palette indexes are used or if the colormap for that layer has been touched at all. That is, unless you want to go down the path of overwriting EEPROM after it has been restored...

I see only two ways to create an experience that matches the way it currently works:

obra commented 5 months ago

Sorry for the slow reply - I'm just getting spun back up on the firmware front.

On Sat, Mar 30, 2024 at 2:25 AM Evy Bongers @.***> wrote:

Custom colormaps would behave differently anyway, because that require users to use that specific LED mode in order to see the keys light up. Users that use any other LED mode wouldn't see it in the first place.

If you do want to set a colormap for that layer, that seems like something best solved in Chrysalis imho. As this seems to apply only to users who work exclusively with Chrysalis, they would (most likely) flash new firmware with Chrysalis. Therefore restoring EEPROM at the end of that process could make sure to set those colors. Of course that does have some drawbacks and caveats:

  • It's a corner case that would need code paths to be removed again at a later point

Given that we have folks taking keyboards out of the box years after shipment, those codepaths would likely want to hang on for quite a while, although we try hard to get people to do a factory reset when they're first setting things up.

  • It requires setting a palette color somewhere, which might surprise users
  • Setting a palette color for it requires finding some palette entry that isn't used (which might not exist)
    • To solve this, the palatte could be extended to 24 colours

Extending the palette seems not-unreasonable.

  • Setting the colormap in Kaleidoscope is impossible except for newly delivered keyboards, because there is no way to check what palette indexes are used or if the colormap for that layer has been touched at all. That is, unless you want to go down the path of overwriting EEPROM after it has been restored...

I see only two ways to create an experience that matches the way it currently works:

  • Update the numpad plugin to add a toggle to enable/disable it and allow it to be changed in Chrysalis. Then add release notes stating that it will be removed in a future release and write documentation on how to create a colormap to replace it for when it does get removed

  • Add LED-EffectPerLayer and ActiveLayerKeys to the build to replace it

    • Optionally add controls to Chrysalis to configure those plugins
    • ℹ️ ActiveLayerKeys does not (yet) use the same palette as colormap, though it probably should do that eventually
  • Add the ColormapOverlay plugin to the build to create the same experience

    • Optionally add controls to Chrysalis to configure the plugin
    • ColormapOverlay uses the same palette as Colormap, so that would still require work to make sure that the relevant color is available on the palette

I feel like ColormapOverlay seems like a cleaner solution. Do you have an opinion on which of the two solutions you think would work better?

Message ID: @.***>

EvyBongers commented 5 months ago

I feel like ColormapOverlay seems like a cleaner solution. Do you have an opinion on which of the two solutions you think would work better?

This option is what best matches the current setup, with the exception that it uses the palette and that therefore the colors would be configurable through Chrysalis already. This would be my choice as well.

To implement this, I think it would be best split up like this:

  1. Update the palette to use 24 colors
  2. update the example sketches to replace NumPad with ColormapOverlay
  3. Update ColormapOverlay to add support for EEPROM
obra commented 5 months ago

On Wed, Apr 17, 2024 at 5:04 AM Evy Bongers @.***> wrote:

I feel like ColormapOverlay seems like a cleaner solution. Do you have an opinion on which of the two solutions you think would work better?

This option is what best matches the current setup, with the exception that it uses the palette and that therefore the colors would be configurable through Chrysalis already. This would be my choice as well.

To implement this, I think it would be best split up like this:

  • PR 1: Update the palette to use 24 colors
  • PR 2: update the example sketches to replace NumPad with ColormapOverlay
  • PR 3: Update ColormapOverlay to add support for EEPROM

That works for me!

-

— Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/1416#issuecomment-2061102404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2EFDPIYWI57L2XI2TLY5ZQNLAVCNFSM6AAAAABFE5MNTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRGEYDENBQGQ . You are receiving this because you commented.Message ID: @.***>