Rewrite Key Storage Implementation

[rgoulter]

Summary

This PR rewrites the key data mechanism of smart keymap. This benefits:

• firmware size is much smaller!
• maintainability: less boilerplate is involved in adding a new smart key system.

Previously: smart-keymap's main traits were key::Key, and its associated Context, Event, PendingKeyState, KeyState types. The key::Key was implemented in a nested tree structure, where types were nested using key::composite::{BaseKey, TapHold, TapHoldKey, Layered, LayeredKey, Chorded, ChordedKey}. -- Some motivation for moving away from this discussed below.

This PR rewrites the key::Key and key::KeyState traits to a key::System trait; and splits from using &Key to using Ref values to look up Key values in System. Keys only nest Ref values, and so key storage doesn't suffer exponential growth in storage size.

Tasks

• keymap codegen (for key::keyboard) so the keymap! integration tests pass.
• get Cucumber tests passing
• have a "key data" / lookup for key::keyboard (for "key code + modifier" keys).
  • should also add integration tests for "modified key code".
• use key::composite as the key_system.
  • if possible, retain integration tests which use key::keyboard as the key_system.
• add in the other keys
  • base:
    • layer modifier
    • caps word
    • sticky modifier
    • callback
    • custom
  • complex
    • tap hold
    • tap dance
    • layered
    • chorded
• remove dead code from keymap-codegen.
• ensure codegen'd init module (or equivalent) usable by the Rust firmware.
• bring tests back up
  • unit tests
  • nickel checks
• misc TBI:
  • replace key::PKR / NPK with a Ref, not KeyPath.
  • resolving PKS in KM.
  • simplify NoOpKeyState
  • consider doc_de_*, to the extent this helps clarify Nickel codegen output.
  • various improvements to keymap-codegen.ncl.

Motivation and Discussion I've had an idea. It seems to me that the current implementation of the keymap (especially the key systems) is overcomplicated.

Motivating problems I think can be fixed with a better implementation:

1. I believe the current implementation takes up a larger firmware size than it needs to. (Size of key::Key implementing values is exponential in some sense). c.f. the artseyio implementation is too big for CH32X. It smells like it should fit.

2. I believe can be improved: the system requires a lot of boilerplate/overhead in order to implement a new smart key. (e.g. a "sequence key" would be like a sequential variant of a chorded key. But, this would be difficult to implement atm).

3. The crate::init (smart_keymap::init) in order to get the Keymap (and KeyDefinitions) type, and the various consts (& references to crate::init::Context). This is awkward/inelegant.

4. Writing out the KeyDefinitionsType for KeysN is too cumbersome to write out by hand; but, the codebase doesn't provide a simpler way of writing it out.

Broadly, I reckon:

• Instead of each key definition value being a tree of key definition values; each should be an index into an array of definitions for the key system. (e.g. instead of TapHold { tap: BaseKey::Keyboard, hold: BaseKey::Keyboard }, I imagine TapHold { tap: { tag: Kbd, ref: ... }, ... }.
• Instead of the aggregating key::composite, instead shuffle around some of the abstractions so that each key type (keyboard, caps word, etc., tap hold, layered, chorded) is exposed as a "System".
  • Rather than a key::composite::Event as the primary way of passing events around, the Event type can remain internal to its own System. Only key::Event (input::Event, keymap::Event), need to cross the System interface.

And, since this rewrite might make the keymap even harder to write by hand, might as well make use of a proc-macro to generate the code in Rust for the tests.

[rgoulter]

Another point of friction is the ::tuples::keys! and KeysN.

Currently, the smart_keymap::init module has a KeysN, and the tests rely on Keys1, Keys2, Keys4.

Since I want to rewrite the key definitions implementation, for non-trivial keymaps it'd be useful to have a keymap! macro which evaluates to a keymap::Keymap expression.

I expect that there's an assumption that KeysN (i.e. Keys2, Keys4, or some other N) is in scope.

After re-implementation/rewriting, perhaps such an assumption can be fixed.

[rgoulter]

Getting closer to rewriting.

The next blocker is the use of Key::new_pressed_key in implementations. Some indirection needs to be added.

I think key::composite is going to have to remain; although, I still think the various implementations should be able to be implemented without e.g. generic Ctx, Ev, PKS, KS.

Rewriting away from tree-like key::Key impl structs will surely break many things. A good approach is to delete functionality back down to just key::keyboard, change the interface there, and then incrementally rewrite the other systems back.

[rgoulter]

Right, so the general approach is:

• Replace use of key::Key. Rather than relying on &dyn key::Key, have some kind of key::System trait instead which takes in an associated KeyRef type.

• This key::System essentially retains the new_pressed_key, handle_event of key::Key. No lookup, though; since that's an implementation detail of the system.

• The key definitions then changes from tuples::KeysN, to just &'static [KeyRef], or [KeyRef; N]. And, the key definitions (impl. detail) can be as part of Context.

• (I'm not quite sure: Context and System implementers get merged as the same value? ... or, rather, Context folds into an impl. detail of System, and just system.handle_event, rather than having separate handle_event invocations for pressed inputs, context, and pending key state?).

That should allow ~most of the implemented functionality to still stay the same (Events, Context, PKS, KS). Though, without the need for the complex tuples, and key::composite BK, THK, LK, CK/AK.

Implementation-wise:

• First, aim for this refactoring with key::keyboard, then key::composite (with keyboard as the only type), then bring up other key types.

[rgoulter]

I see that e.g. KeyState implementations all have Key as fields. (Only Sticky key has anything additional).

• [-] use KeyRef & other info as arguments to KeyState

(Ah ... I think KeyRef used in KeyState; and pass System (or whatever stores the key definitions) as an argument.

[rgoulter]

Some progress. -- An integration test that checks "tap 'a'" now passes.

The next steps, bring up all the integration tests (noting as a checklist in first comment).

[rgoulter]

Hmm. Next tricky issue is coming up with the Nickel code to construct this "key data[], and key refs".

Fortunately, I think the idea is as follows:

• The output is: array of key references + structures for the key data.
• For each key definition (tree), construct these with a traversal.
• Update the "structure of key data" with the key (tree node), appending the data and creating the ref for that key. (Recursively, do this for its keys; e.g. a tap-hold does this for tap, hold, then for itself. Like a traversal).
• Then, can construct the System (or whatever value stores the data[]) from this value, and the key refs.

[rgoulter]

I kinda like the idea of (eventually):

• ncl codegen (and keymap to json) should have the different keys in different NCL files, rather than one big file. More modular.
• having only "key::keyboard" is a "smart-keymap-core", and having the smart keys (tap hold, layered, etc.). -- Or, rather, smart-keymap-core would provide a basic 'key::System' impl. that's just key::keyboard, and key::composite could be a separate crate.
• more broadly, it'd be nice if it were easy to describe an 'aggregate key system' like key::composite with minimal/no boilerplate.

[rgoulter]

Hmm.

The cucumber tests never touched keymap-codegen. Instead, they relied on the tree structure being deserializable by serde json.

Instead... I guess keymap-codegen can take the keymap-json value (from km-ncl), and construct json-representation of the required data and refs.

[rgoulter]

Getting there.

Currently, a vertical set of tests work, for the most trivial keymap feature (key::keyboard, basic keycodes). (The unit tests, the rust integration tests, the Cucumber tests).

But, it's clear that it's quite a comprehensive rewrite. So, it's going to be one big PR.

[rgoulter]

The old implementation has some very awkward crate::init::MAX_ constants, for things like "max number of layers", "max number of tap dances", "max number of chords". (The idea was, for the cucumber tests, these could be large; for the generated firmware, these could be more specific).

When trying to avoid these constants, (e.g. using something like key::keyboard::System<N>), this presents .. certain points of friction. (e.g. Cucumber keymap deserialising needs to know N; KeyState's assoc type System needs to know N (!)).

I'm not sure if it's possible for all these cases, but probably the better way of arranging the code is passing interfaces around.

[rgoulter]

Hmm.

From what I can tell, the methods from the KeyState trait also need to be handled by System.

Previously, KeyState implementers were expected to have all the data for their handle_event(&mut self) and key_output() -> Option<..>.

But, by using Ref (& looking up the Key data for large key definitions, like layered keys), in order to keep these methods in the KeyState trait, as far as I could tell that required at least associating with Ref and Key data types. (I considered associating with System, but the System's parameters weren't bound by KeyState, which seemed less than ideal).

Instead.. System becomes more monolithic or coherent: by moving KS's methods to System (& passing Ref+KS to these methods), System then covers: new_pressed_key, handle_event (for pending state), handle_event (for pressed key state), and key_output. -- It's a lot; but, although it's 3 distinct 'phases', even the key::Key trait covered 2 of those.

[rgoulter]

I've had friction with Rust trying to use some kind of Input { .. } value type passed to const fn new(..). Seems like currently, even when it's Copy, the Rust compiler doesn't like that the value may drop in the const eval.

Though, I was able to make use of a trait with various associated types, and this simplifies (shifts the complexity to places where it's easier to handle) generics for the various key data types.

It terms of bringing up other keys? Progress seems smooth. (The work has been done to come up with the abstractions; now it's mostly just coming up with the Ref, moving from key::Key and key::PKS/key::KS implementations, to a key::System implementation).

I'm still curious to see if this does result in a reduction in firmware size. (Because there's no 'exponential' growth in sizes of the types).

I'm optimistic that key::composite and keymap-codegen can much simpler than they were with the tree-based structure. (No BaseKey/TapHoldKey/LayeredKey/ChordedKey; codegen doesn't need to 'unify' the types).

[rgoulter]

"This branch cannot be rebased due to conflicts" I think GitHub's UI is confused. (At some point, this PR got closed because the code it pointed to got merged elsewhere).

Anyway. Rewrote the layering system. -- The layering system is a bit neat in that it's got two kinds of keys (layer modifiers, layered keys).

Definitely having a good suite of integration tests has been useful!

[rgoulter]

Wow, Rust is difficult. It's incorrect ("not what I meant") to have &mut in front of a &mut T!

modified   src/keymap.rs
@@ -473,13 +473,13 @@ where
             self.pressed_inputs.iter_mut().for_each(|pi| {
                 if let input::PressedInput::Key(input::PressedKey {
                     key_ref,
-                    mut key_state,
+                    key_state,
                     keymap_index,
                 }) = pi
                 {
                     self.key_system
                         .update_state(
-                            &mut key_state,
+                            key_state,
                             &key_ref,
                             &self.context,
                             *keymap_index,
@@ -686,13 +686,13 @@ where
         // Update each of the pressed keys with the event.
         self.pressed_inputs.iter_mut().for_each(|pi| {
             if let input::PressedInput::Key(input::PressedKey {
-                mut key_state,
+                key_state,
                 key_ref,
                 keymap_index,
             }) = pi
             {
                 self.key_system
-                    .update_state(&mut key_state, &key_ref, &self.context, *keymap_index, ev)
+                    .update_state(key_state, &key_ref, &self.context, *keymap_index, ev)
                     .into_iter()
                     .for_each(|sch_ev| self.event_scheduler.schedule_event(sch_ev));
             }

[rgoulter]

Trying the 8key-artseyio keymap from #373, currently gets:

[100%] Linking C executable usb-device-compositekm
Memory region         Used Size  Region Size  %age Used
           FLASH:       36940 B        62 KB     58.18%
             RAM:          0 GB        20 KB      0.00%
Generating hex file
[100%] Built target usb-device-compositekm

SUCCESS! (Before, it was >105%, IIRC -- EDIT: not sure what's changed, but currently I see 95% when building just 8key for the ch32, going up to 97% when building with the 48key keymap. Still, ~95 down to ~60 is a big win).

This validates the hope I had that this rewrite would result in a significant reduction in firmware size!

[rgoulter]

Finally, all the keys which had been implemented have been rewritten.

These ~170+ commits are all going to get squashed.

Next, I'll brush up some of the codegen stuff, as well as restoring the CI, tests, etc.).

[rgoulter]

Memory usage at 54.47% for the CH32X firmware build: https://github.com/rgoulter/smart-keymap/actions/runs/17768309755?pr=381

Down from 78.51% https://github.com/rgoulter/smart-keymap/actions/runs/17526419123

[Copilot]

Pull Request Overview

This PR implements a comprehensive rewrite of the keymap system to address size and complexity issues. It transitions from tree-based key definitions to a reference-based array system for improved efficiency and maintainability.

Key changes include:

• Replacing tree-structured key definitions with flat arrays and references
• Simplifying the keymap initialization API
• Removing the complex tuple-based key definition system in favor of array-based storage

Reviewed Changes

Copilot reviewed 50 out of 52 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/rust/tap_hold.rs	Temporarily disables layered test module
tests/rust/keymap.rs	Updates keymap construction to use new array-based system
tests/ncl/*/expected.rs	Updates generated test expectations to match new keymap structure
tests/cucumber/keymap.rs	Adapts Cucumber tests to new keymap API
stm32f4-rtic-smart-keyboard/src/main.rs	Updates keymap initialization call
stm32-embassy-smart-keyboard/src/main.rs	Updates keymap initialization call
src/lib.rs	Major restructuring of init module and removal of tuples module
src/keymap.rs	Complete rewrite to use reference-based system instead of tree traversal

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[rgoulter]

The unit tests haven't yet been rewritten. During the rewrite, I leaned on the component/integration tests (tests/rust), not on the unit tests.

Still. I'd rather re-implement the unit tests to better spec / convey the interface expected of the various bits.

I know the codebase well enough that if an integration test fails, I can quickly poke through to find what's gone wrong. But, there are plenty of assumptions (stuff like "does this key system emit an event on new_pressed_key?") such that complementing the implementation with a unit test couldn't hurt.

Also, some of the keymap-codegen.ncl code could be further refined. This PR is already a comprehensive rewrite. Might as well polish the code a bit further.

[rgoulter]

I've removed some unit tests. (I'm satisfied with the rust-integration component testing).

I think the key::composite module is difficult to bring under test. The System is relatively difficult to construct. (For non-const, could use a builder). -- Currently, the rust-integration (tests/rust/keymap.rs) are easier to write than key::composite unit tests, and also provide more coverage.

[rgoulter]

The codegen modules in keymap-codegen are now slightly mismatched: previously, it was 1:1 with each key; now, it's closer to 1:1 for each module.