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

Garretonzo · 2024-10-23T19:05:59Z

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

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).

quantum/dynamic_keymap.c

lint formatting Co-authored-by: Drashna Jaelre <[email protected]>

zvecr · 2024-10-24T20:58:49Z

quantum/dynamic_keymap.c

    // Big endian, so we can read/write EEPROM directly from host if we want
    eeprom_update_byte(address, (uint8_t)(keycode >> 8));
    eeprom_update_byte(address + 1, (uint8_t)(keycode & 0xFF));
+    unregister_code(prev_keycode);


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.

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

key pressed

dynamic keymap set

dynamic keymap set (again)

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?

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?

resolving since I made via.c responsible to handle this

Unresolving as its still using unregister_code.

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

…n-dynamic-keymap-set

Garretonzo · 2024-11-02T05:04:43Z

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 · 2024-11-14T19:02:41Z

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 · 2024-11-15T05:53:16Z

quantum/action_layer.c

+uint16_t keycode_map[((MATRIX_ROWS * MATRIX_COLS) + (CHAR_BIT)-1) / (CHAR_BIT)][16] = {{KC_NO}};
+#        ifdef ENCODER_MAP_ENABLE
+uint16_t encoder_keycode_map[(NUM_ENCODERS + (CHAR_BIT)-1) / (CHAR_BIT)][16] = {{KC_NO}};
+#        endif // ENCODER_MAP_ENABLE


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.)

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

sigprof · 2024-11-15T05:59:57Z

quantum/action_layer.c

+    else if (key.row == KEYLOC_ENCODER_CW || key.row == KEYLOC_ENCODER_CCW) {
+        const uint16_t entry_number = key.col;


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.

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

Garretonzo added 3 commits October 23, 2024 11:29

enable via, unregister key on dynamic set

b7dfced

unregister at end of function

37d8824

removing custom rules from repo

e890fa0

github-actions bot added the core label Oct 23, 2024

zvecr changed the base branch from master to develop October 23, 2024 19:06

Garretonzo marked this pull request as draft October 23, 2024 19:09

Garretonzo marked this pull request as ready for review October 23, 2024 20:31

drashna requested a review from a team October 24, 2024 01:36

drashna requested changes Oct 24, 2024

View reviewed changes

quantum/dynamic_keymap.c Outdated Show resolved Hide resolved

Update quantum/dynamic_keymap.c

9f3e5fa

lint formatting Co-authored-by: Drashna Jaelre <[email protected]>

drashna requested a review from a team October 24, 2024 20:45

zvecr requested changes Oct 24, 2024

View reviewed changes

Garretonzo added 4 commits October 24, 2024 19:28

moving logic to via.c, reverting dynamic_keymap.c changes

d3b8178

working example

751ad39

Merge branch 'get-event-keycode-cache' into unregister-prev-keycode-o…

4edc916

…n-dynamic-keymap-set

revert via.c

168cabe

Garretonzo marked this pull request as draft November 2, 2024 04:57

Garretonzo changed the title ~~Unregister previous keycode after dynamic keymap set~~ Update get_event_keycode to cache keycode on key press, return cached keycode on key release Nov 2, 2024

Garretonzo added 3 commits November 9, 2024 12:22

move functions

3727bc7

move additions together

46d0724

comments

1dc3373

Garretonzo marked this pull request as ready for review November 9, 2024 20:27

Garretonzo requested review from zvecr and drashna November 9, 2024 20:39

drashna requested a review from a team November 9, 2024 22:09

drashna added the breaking_change_2025q1 Targeting breaking changes Q1 2025 label Nov 9, 2024

opt in option, move to store_or_get_action

f512383

github-actions bot added the documentation label Nov 15, 2024

Garretonzo changed the title ~~Update get_event_keycode to cache keycode on key press, return cached keycode on key release~~ Update store_or_get_action to cache keycode on key press, return cached keycode on key release Nov 15, 2024

sigprof reviewed Nov 15, 2024

View reviewed changes

Garretonzo added 3 commits November 14, 2024 22:09

keycode_map simplification, encoder code removed

1378e56

keycode_map simplification cont

9f71517

keycode_map simplification cont

610863f

Garretonzo requested a review from sigprof November 15, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Garretonzo commented Oct 23, 2024 •

edited

Loading

zvecr Oct 24, 2024

Garretonzo Oct 24, 2024

Garretonzo Oct 24, 2024

Garretonzo Oct 29, 2024

zvecr Oct 29, 2024

Garretonzo Nov 9, 2024

Garretonzo commented Nov 2, 2024 •

edited

Loading

tzarc commented Nov 14, 2024

sigprof Nov 15, 2024

Garretonzo Nov 15, 2024

sigprof Nov 15, 2024

Garretonzo Nov 15, 2024

		else if (key.row == KEYLOC_ENCODER_CW \|\| key.row == KEYLOC_ENCODER_CCW) {
		const uint16_t entry_number = key.col;

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

Are you sure you want to change the base?

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

Conversation

Garretonzo commented Oct 23, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garretonzo commented Nov 2, 2024 • edited Loading

tzarc commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garretonzo commented Oct 23, 2024 •

edited

Loading

Garretonzo commented Nov 2, 2024 •

edited

Loading