easier access to modules/extensions

[xs5871]

There's currently no intuitive way to access modules and extensions, other than passing (effectively) global variables around. Suggestion: Let keyboard.modules (, extensions respectively) be a container class similar to the AttrDict as in keys.KC:

class ModuleList(list):
    def __getattr__(self, key):
        for m in self:
            if m.__qualname__ == key:
                return m

# in kmk_keyboard.py:
class KMKKeyboard:
# ...
    modules = ModuleList()
    extensions = ModuleList()

which would allow for attribute based access like

keyboard.extensions.append(RGB(...))
# ...
TO3 = KC.TO(3).after_press_handler = lambda: keyboard.extensions.RGB.set_hsv_fill(...)
keyboard.keymap = [[..., TO3, ...]]

As it is now, with some modules producing side-effects, it's important to maintain the order in which they are added (though our documentation could be clearer on that). Alternatively, we could enforce a pre-defined order, which I'm not a fan off. There are some obvious limitation to the approach above. It will only provide access to the first instance of a module/extension, which may be a good/bad thing, depending on point of view. Pretty much all modules/extension are implicitly meant to be unique/singletons, though none really require or enforce that usage. Another issue may arise when users, unknowingly, replace keyboard.module with a builtin list(), because the syntax is identical, and wonder why there's a discrepancy between the attribute access mentioned in the documentation and what the builtin allows.

I wouldn't categorize these caveats as critical but I'd like to get your input on that, and am especially open to other ideas.

[Tonasz]

wonder why there's a discrepancy between the attribute access mentioned in the documentation and what the builtin allows.

What do you mean by that?

Another issue may arise when users, unknowingly, replace keyboard.module with a builtin list(),

What if instead of extending built in list, we left it as is and just added get_module(name) / get_extension(name) method to KMKKeyboard instead?

Pretty much all modules/extension are implicitly meant to be unique/singletons, though none really require or enforce that usage.

I'm really curious if anybody is using it stuff not as singleton. The only stuff I can imagine (but that's an unlike stretching) is 2xRGB (one for keys, one for backlight) - but this I heard is gonna be refactored anyway - or using 2x displays (but these are customs, nothing official, so 🤷. So I don't think we have to worry about that. Or if needed in future, just provide in future alternative method get_all_modules(name) or smth.

[kdb424]

Modules and extension, for the most part, try their best to be self contained for safe code, and constraints on failure modes. They were designed with that rule in mind, though if you can present a very good case on why you think that modifying them for your own custom use isn't reasonable, then I have a design flaw in how you write them. Forks and modifications were the intended method of changing their functionality, and not giving users ways to let them adjust each other. Without the constraints in place, it can get messy fast.

EDIT: You mentioned that some have side effects. This is intended to be a limited scope, and if anything, we would like to remove as much as possible of these side effects as it makes debugging a nightmare, but where you must have them, we allow exceptions.

[xs5871]

@Tonasz

wonder why there's a discrepancy between the attribute access mentioned in the documentation and what the builtin allows.

What do you mean by that?

If we were to have documentation for attribute like module access, i.e. keyboard.modules.ModuleName, but it also looks like a list everywhere else, with modules.append(Module) or modules += [Module], people might think: Looks like a list, I'll just init all modules at once with keyboard.modules = [Mod1, Mod2, ...]. It's a hypothetical point of confusion.

What if instead of extending built in list, we left it as is and just added get_module(name) / get_extension(name) method to KMKKeyboard instead?

That's also an option I considered. It's somewhat more verbose, and you have to use a stringyfied version of the class name. I was thinking about python newcomers, who already might struggle with the difference of instances, classes, classnames... and at this point it's a string for some reason. The difference is in the didactics, not the implementation. I'd be fine with either way.

I'm really curious if anybody is using it stuff not as singleton. The only stuff I can imagine (but that's an unlike stretching) is 2xRGB (one for keys, one for backlight).

Not really relevant, but since we can pass custom pixelbuffer to RGB, that's not really an issue anymore. I have an implementation of that. Singleton RGB and the Pixelbuffer handles multiple endpoints.

@kdb424 The modules and extension we provide should be self contained, I totally agree. What I have in mind is (meant to be) read access only, though python blurs that concept anyway. The use case are things like: How do I get

• caps lock status into my LED module
• WPM (wip) into my display module
• battery status into my RGB module

The example with RGB.set_hsv_fill isn't a read, OK. I picked that one from chat, as a structural example, not a best use case.

Those are all things we should not hardcode into modules. But we should make an effort to make it easier for non-python-speakers to achieve those things. We already pass keyboard into almost everything, why not provide it with a way to comfortably reference modules and extensions, i.e. a shorthand.

EDIT: You mentioned that some have side effects. This is intended to be a limited scope [...]

Also agree. The point I was trying to make is that we need strict ordering, and a dict instead of list wouldn't be a good idea. I could have been more clear on that.

[kdb424]

I'm not going to say it's not possible, but I spent quite a lot of time ripping things apart, and trying to not intertwine them again for ease of debugging, which is also important to module devs, general keyboard people, ect. I'm willing to look at, test, and review an implementation, but I will be hypercritical of it as I just fought to undo a lot of it and lived through that pain. I want to stress that I'm willing to give it a look, but it will be considered a major feature/change, and be tested much more thoroughly than anything I've merged after the last major refactor. I'll let this go and have people hash out what ways you think are best, and will chime in any time I see fit as I'm notified by email any time chat happens here, and try to read, even if I don't always say something. That's just my $0.02 without seeing the code

[Tonasz]

The only valid use case I see and encountered by myself, is above mentioned displaying stuff on the display/rgb. It's not like everything gonna be hard wired with each other. I just don't see other way of implementing stuff like mentioned above by @xs5871. And if we take a look at WPM example, what approach is more granular, modular and separate?

1. Having WPM countered integrated in display module and if sb wants to fork the display, it needs to be kept synchronized between them
2. Having all the WPM logic integrated into KMKKeyboard and risking cluttering it what would make debugging worse
3. Having standalone WPM counter, which just expose one-two value fields and is accessed by other modules?

I see your pain @kdb424, but I think this idea is not going against the flow, but with it. Anyway I don't say it's the only way to go I can see and I'm curious of alternatives - dunno, global state dict?

[kdb424]

I'm mostly found that shoving data into "global variables effectively" to be a reasonable way to pass data between modules, especially if you define the spec of the module's data format. I've posted this before, but here's some really crappy code I wrote to, incorrectly, report WPM, and I had it output on an oled with ease. I must stress this was proof of concept, but it was meant to lay the minimum data transfer between extensions.

from kmk.extensions import Extension
from kmk.kmktime import ticks_diff, ticks_ms

class WPM(Extension):
    def __init__(self, debug=False):
        self.enable = True
        self.history = [0]
        self.timeout = 1000
        self.checkpoint = ticks_ms()
        self.debug = debug
        self.counter = 0
        self.old_wpm = 0
        self.wpm = 0

    def on_runtime_enable(self, keyboard):
        keyboard.wpm = 0
        return

    def on_runtime_disable(self, keyboard):
        keyboard.wpm = None
        return

    def during_bootup(self, keyboard):
        keyboard.wpm = 0
        return

    def before_matrix_scan(self, keyboard):
        return

    def after_matrix_scan(self, keyboard, matrix_update):
        if self.enable:
            new_history = self._add_character(
                matrix_update,
                keyboard.secondary_matrix_update)
            if new_history is not None:
                self.history.append(new_history)
            if len(self.history) > 10:
                self.history.pop(0)
            self.wpm = self.calculate_wpm()

            if self.wpm != 0 and self.wpm != self.old_wpm:
                keyboard.wpm = self.wpm
                if self.debug:
                    print(f'WPM: {self.wpm}')
                self.old_wpm = self.wpm
        return

    def before_hid_send(self, keyboard):
        return

    def after_hid_send(self, keyboard):
        return

    def on_powersave_enable(self, keyboard):
        self.enable = False
        return

    def on_powersave_disable(self, keyboard):
        self.enable = True
        keyboard.wpm = self.wpm
        return

    def _add_character(self, matrix_update, second_update):
        if ticks_diff(ticks_ms(), self.checkpoint) > self.timeout:
            self.checkpoint = ticks_ms()
            ret = self.counter
            self.counter = 0
            return ret
        if matrix_update or second_update:
            self.counter += 1

    def calculate_wpm(self):
        # No, there is no real math here, though it seems to work out sorta.
        return int(sum(self.history[:-1]) * .6)

[xs5871]

I think we're on the same page about the module/extension API, and the importance of keeping modules and extensions agnostic of one another; sorry that I gave an impression to the contrary.

The "proposed code" is in its entirety in the first code block in my first message. It's just a shorthand for, or nicer looking version of

wpm = [e for e in keyboard.extensions if e.__qualname__ == 'WPM'][0]

(we already use a similar construct to check if the split module is used.)

There is no new feature here, much less any major change. If anything, I'd argue this to be cosmetic or QOL. My argument is: we already have a list of modules and extension in the keyboard class, which we pass into every module API call, why not give the option to use that instead of a user curated list of references? This would only affect user code, i.e. forks and modifications, and by "affect" I mean "it gives users another option".

The only place in KMK core I can think of where a similar mechanism would be handy is the split module, once we want to communicate more than key events. But I think that's another topic.

[LukeDRussell]

So just came here from https://github.com/KMKfw/kmk_firmware/issues/474, because I was curious about some of the imports and instantiations I ended up using to get Layers and Combos working. My experience with Python (or any language) has stopped short of creating my own classes. I've just tried to use them, like in this case.

I think my (working) main.py is an example of mixing up list() and keyboard.module.

import board

import kb

from kmk.keys import KC
from kmk.modules.layers import Layers
from kmk.modules.combos import Combos, Sequence

keyboard = kb.KMKKeyboard()

combos = Combos()

keyboard.modules = [
    Layers(),
    combos,
    ]

• I don't know why I can use Layers() but not Combos().
• I think I get that combos is an instance of Combos() sub-classing Module().
• I think Combo() is a way to put some defaults on our Chords and Sequences without putting those defaults into the Combos() class - probably future proofing.
• I also tried to make keyboard.combos == Combos() but apparantly you can't add attributes to an instance of a class, like you can add keys to dictionaries.
• I think keyboard.modules is expecting a list of module class instances, not module classes. I'm inferring that from the during_bootup method called on keyboard.go(), which would fail if the method couldn't be called.
• I think this is a case of answering my own question by following the code, at least partially.
• The only difference I can see is that Combos() sub-classes Module() directly, while Layers() is sub-sub-classed through HoldTap().

Whatever the answers to those questions, I could still be arse-about.

[LukeDRussell]

In Dynamic Sequences Docs it uses the class in the modules list at the top. But when you want to configure it you have to create an instance. I'm so confuzzled.

[klardotsh]

@xs5871 You brought up AttrDict above - I actually wonder if there's sanity in an AttrDict that preserves insertion order (I think MicroPython dictionaries already do, but it's worth asserting that since we'd be relying on the behavior) being used for this.

# keyboard.modules is instantiated with the Keyboard class as an empty AttrDict; subclasses of Keyboard can of course override this
keyboard.modules.update({
    "my-human-readable-name-here": Combos(),
})

This allows human-readable addressing within "userspace" (so lambdas can refer to keyboard.modules["my-human-readable-name-here"]), but via keyboard.modules.values() and __qualname__, "kernelspace" can still do things like detect whether powersave is in use, and modify, say, split behaviors within the modules themselves.

Thoughts?

[klardotsh]

@LukeDRussell Answering a few of your questions:

• Layers() and Combos() would in theory both be fine. Assigning combos = Combos() is just adding a name to reference that instance of Combos by if we wanted to do so elsewhere in the layout. Without that assignment the instance is anonymous, meaning the references are all held by whatever it was passed to inline, and we have no direct reference to the object in our stack frame (which... yeah, maybe that's a term I won't dive into tonight 😄)
• Adding attributes to the instance of a class should in theory work on standard classes: https://docs.python.org/3/library/functions.html#setattr. The only class I know I've not implemented setattr for is AttrDict, by way of inheriting from dict (which does not implement __setattr__). Maybe this is a MicroPython difference I'm forgetting... or maybe Python in general I'm forgetting. It's been a while since I've used it as a daily language.
• Correct, until we get into weird higher-order programming constructs where code is mangling other code, and not just data (aka metaprogramming), we'll almost always be passing class instances around and not class definitions. KMK does some fun metaprogramming fun, but generally only deep within internals, we've tried to generally keep all of that away from "the glass" (aka the API one writing a keyboard definition would interact with).

The Dynamic Sequences thing is just another case of "anonymous vs not", and is really just a quirk of how those docs came to be (usually by way of copy-pasting out of someone's keyboard layout). There's no real reason one has to assign the DynamicSequences instance to a name to configure it: Foo(bar=1) and foo = Foo(bar = 1) are exactly equivalent other than that in the former case, I have no way of referring to my copy of Foo anymore in the current execution context.

hello(World(answer=42))

# is equivalent to

world_to_greet = World(answer=42)
hello(world_to_greet)

[xs5871]

@xs5871 You brought up AttrDict above - I actually wonder if there's sanity in an AttrDict that preserves insertion order (I think MicroPython dictionaries already do, but it's worth asserting that since we'd be relying on the behavior) being used for this.

Well, in my personal config I have an AttrList, but there's also an ordered dict collection. In CPython dicts preserve order since version/PEP I'm not going to look that up. That is not the case for Circuitpython last time I checked.

Thoughts?

I'm not attached to either implementation, depends on what makes more sense for the average user.

[LukeDRussell]

@LukeDRussell Answering a few of your questions:

...

Thank you, this really helped me understand what's going on in my code/config when it worked and when it didn't.

I'm not attached to either implementation, depends on what makes more sense for the average user.

Average user here.

• Accessing attributes with . and ["..."] from anywhere seems natural to me. Though I've spent a lot of time using Ansible and some with Pandas.
• Best way to set things on KMK class instances is less clear to me.
• Using keyboard.modules.update() makes me think that modules is somehow special (a good thing I think). As opposed to .append() which makes me think modules is just a list().
• Only reason I can think of to name my instance of a module arbitrarily, is for long modules/extensions such as DynamicSequences. If it makes it a better implementation, I'm happy to type out the long name.
• It would be nice to use the same accessor method to both get and set an attribute of my module instance. Pandas does this really nicely for Dataframes and Series.
• Any module that exports an attribute outside to parent classes should also be present in the module.

Hypothetical below:

keyboard = kb.KMKKeyboard()

# Adding my modules
keyboard.modules.add(Combos([
    - Sequence((KC.LEADER, KC.A), KC.LCTL(KC.A)),  # select All
  ]))
keyboard.modules.add(Layers())
keyboard.modules.add(DynamicSequences(
    key_interval=20,  # Milliseconds between key events while playing
  )) 

# Make sure module and keyboard attribute matches
assert keyboard.modules.Layers.active_layer == keyboard.active_layer

# Change a module setting
keyboard.modules.DynamicSequences.key_interval = 30
# Verify
print(keyboard.modules.DynamicSequences.key_interval)

# Add another Sequence
keyboard.modules.Combos.append(Sequence((KC.LEADER,KC.U), KC.LCTL(KC.Z)))

Though for some reason it feels wierd having the Capital Class in dot notation...