Update store_or_get_action to cache keycode on key press, return cached keycode on key release

[Garretonzo]

Description

Changes behavior of store_or_get_action to cache a keycode for a pressed key and then return the cached keycode when releasing a key.

I would not exactly call this a "bugfix" but it does prevent keys from getting "stuck" when editing layouts through VIA hotswapping, for example.

This semi-replaces the source layers cache implementation to cache the entire keycode for a key location.

Feature must be opted into with #define KEYCODE_CACHE_ENABLE.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout (addition or update)
• Documentation

Issues Fixed or Closed by This PR

• Closes [Bug] Keys "stuck" after dynamic keymap set #24516

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[zvecr]

As discussed on discord, this wont correctly handle keycode that are non-basic, and changing it to unregister_code16 would then only handle basic+modded. Using clear_keyboard() covers more scenarios but still does not cover any side effects of quantum keycodes.

Im also not a fan of coupling storage directly to its eventual use, wherever in the tmk action code. I agree updating via.c to be responsible would be error prone for other eventual uses of dynamic_keymap_set_keycode, but it still feels like the cleaner workaround till a better solution is implemented.

[Garretonzo]

A reason I handled things in here was to cover the scenario of:

1. key pressed
2. dynamic keymap set
3. dynamic keymap set (again)
4. key released

Is changing unregister_code to handle non-basic keycodes an option?
Or creating a new, all-encompassing function to handle unregistering any code?

[Garretonzo]

Also, when you suggest to have via.c be responsible, would having the same logic but instead immediately before and after the call to dynamic_keymap_set_keycode within the id_dynamic_keymap_set_keycode case suffice? Or would there be more to having this happen in via.c?

[Garretonzo]

resolving since I made via.c responsible to handle this

[zvecr]

Unresolving as its still using unregister_code.

[Garretonzo]

Unresolving as its still using unregister_code.

Hey mate, if you have time and still have interest, I think my new implementation is ready for further review

[Garretonzo]

I made some heavy changes following a suggestion from the discussion on discord when this was first brought up.

I modified the logic of what get_event_keycode returns. A cache is used to store a keycode when a key is pressed, then that cache is read from when a key is released.

I know there is some overlap with the introduction of this implementation but I don't know what all depends on the source layers cache, so I focused solely on what get_event_keycode returns.

[tzarc]

This may need to be opt-in, given the added RAM requirement. A TKL would use somewhere of the order of 400-500 extra bytes, and not everyone has that free.

[sigprof]

This looks overcomplicated. The source layer cache code uses such tricks to avoid allocating a whole byte for each entry that needs just 2 bits in the most common case (4 layers); if you are storing the whole 16-bit keycode, you may just use

static uint16_t keycode_map[MATRIX_ROWS][MATRIX_COLS];

and

static uint16_t encoder_keycode_map[NUM_ENCODERS][NUM_DIRECTIONS];

(Actually the encoder code might not need such handling at all, because the press and release events are emitted without running the rest of main loop code in between, therefore keymap updates should not happen.)

[Garretonzo]

Thanks a ton for the feedback, I cleaned up the overcomplicated keycode_map and also removed the encoder handling

[sigprof]

In general you may need to consider key.row too (so that entries for different rotation direction don't overwrite each other); maybe it's simple to just do that instead of thinking why that should not happen.

[Garretonzo]

since I removed the encoder handling code, is this safe to not consider?

[tzarc]

Perhaps keeping an array of, say, 10 of:

typedef struct historical_keycode_t {
    uint8_t row;
    uint8_t col;
    uint16_t keycode;
} historical_keycode_t;

...should significantly decrease the RAM requirements at the cost of a slightly more complicated algorithm.