Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

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

Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b7dfced
enable via, unregister key on dynamic set
Garretonzo Oct 23, 2024
37d8824
unregister at end of function
Garretonzo Oct 23, 2024
e890fa0
removing custom rules from repo
Garretonzo Oct 23, 2024
9f3e5fa
Update quantum/dynamic_keymap.c
Garretonzo Oct 24, 2024
d3b8178
moving logic to via.c, reverting dynamic_keymap.c changes
Garretonzo Oct 25, 2024
751ad39
working example
Garretonzo Nov 2, 2024
4edc916
Merge branch 'get-event-keycode-cache' into unregister-prev-keycode-o…
Garretonzo Nov 2, 2024
168cabe
revert via.c
Garretonzo Nov 2, 2024
3727bc7
move functions
Garretonzo Nov 9, 2024
46d0724
move additions together
Garretonzo Nov 9, 2024
1dc3373
comments
Garretonzo Nov 9, 2024
f512383
opt in option, move to store_or_get_action
Garretonzo Nov 15, 2024
1378e56
keycode_map simplification, encoder code removed
Garretonzo Nov 15, 2024
9f71517
keycode_map simplification cont
Garretonzo Nov 15, 2024
610863f
keycode_map simplification cont
Garretonzo Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions quantum/dynamic_keymap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright 2017 Jason Williams (Wilba)

Check failure on line 1 in quantum/dynamic_keymap.c

View workflow job for this annotation

GitHub Actions / lint 

Requires Formatting
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -121,10 +121,12 @@

void dynamic_keymap_set_keycode(uint8_t layer, uint8_t row, uint8_t column, uint16_t keycode) {
if (layer >= DYNAMIC_KEYMAP_LAYER_COUNT || row >= MATRIX_ROWS || column >= MATRIX_COLS) return;
uint16_t prev_keycode = dynamic_keymap_get_keycode(layer, row, column);
void *address = dynamic_keymap_key_to_eeprom_address(layer, row, column);
Garretonzo marked this conversation as resolved.
Show resolved Hide resolved
// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolving since I made via.c responsible to handle this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolving as its still using unregister_code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

#ifdef ENCODER_MAP_ENABLE
Expand Down
Loading