Combo RAM and Kconfig reduction #2849
Conversation
Determine the max keys per combo automatically from the devicetree, so we remove the ZMK_COMBO_MAX_KEYS_PER_COMBO Kconfig symbol.
Reference combos by index, not 32-bit pointers, and store bitfields instead of arrays in several places, to bring down our flash/RAM usage.
Use bit field to track candidate combos, to avoid needing an explicit `ZMK_COMBO_MAX_COMBOS_PER_KEY` setting.
| int | ||
| default 0 | ||
| help | ||
| Deprecated: Storage for combos is now determined automatically |
There was a problem hiding this comment.
I don't think these should be deprecated. Rather I think we should take the max of the automatic determination and whatever this value is set to, for future Studio use.
There was a problem hiding this comment.
When we have future studio use, we'll likely "pre-allocate" things by setting placeholders up in the devicetree, like we do with reserved layers, so our auto-calculation will still be usable there as well.
There was a problem hiding this comment.
Closer inspection, seems doable for MAX_COMBOS_PER_KEY, but less so for vice-versa: what if DT has only 2 and 3 key combos, and then someone wants to set up a 5 key one in Studio?
|
Joel had an interesting comment here that might also apply to this PR. |
Moved to uint32_t type for the bit fields to avoid alignment issues. |
app/src/combo.c
Outdated
| static uint8_t number_of_set_bits(uint32_t field) { | ||
| uint8_t count = 0; | ||
| while (field) { | ||
| field &= (field - 1); | ||
| count++; | ||
| } | ||
| // clear unmatched candidates | ||
| for (int i = matches; i < CONFIG_ZMK_COMBO_MAX_COMBOS_PER_KEY; i++) { | ||
| candidates[i].combo = NULL; | ||
|
|
||
| return count; | ||
| } |
There was a problem hiding this comment.
I don't think we actually need to calculate the total number of set bits. We only care if it's 0, 1, or more.
Something like
if (value == 0) {
return 0;
}
if ((value & (value - 1)) == 0) {
return 1;
}
return 2;
There was a problem hiding this comment.
Good call. I tweaked this a bit, roughly modeling what you described, to save on some cycles. Thanks!
|
Would daily testing be useful in the current state of the PR? |
I believe so, but I need to do so myself first to be 100% sure, even though tests are all green. |
| static uint8_t zero_one_or_more_bits(uint32_t field) { | ||
| uint8_t count = 0; | ||
| while (field && count < 2) { | ||
| field &= (field - 1); |
There was a problem hiding this comment.
If I am to be super nitpicky, this line gets executed one more time than is necessary with this version of the function. I also feel like this function could have inline added.
| uint32_t pressed_keys_count = 0; | ||
| #define COMBO_CHILDREN_COUNT (0 DT_INST_FOREACH_CHILD(0, COMBO_ONE)) | ||
|
|
||
| // We need at least 4 bytes to avoid alignment issues |
There was a problem hiding this comment.
| // We need at least 4 bytes to avoid alignment issues |
| // We do some magic here to generate the `combos` array by "key position length", looping | ||
| // by key position length and on each iteration, only include entries where the `key-positions` | ||
| // length matches. | ||
| // Doing so allows our bitmasks to be "shorted key positions list first" when searching for matches. | ||
| // `20` is chosen as a reasonable limit, since the theoretical maximum number of keys you might | ||
| // reasonably press simultaneously with 10 fingers is 20 keys, two keys per finger. | ||
| static const struct combo_cfg combos[] = { | ||
| LISTIFY(20, COMBO_CONFIGS_WITH_MATCHING_POSITIONS_LEN, (), 0)}; |
There was a problem hiding this comment.
I think with the changes to filter_candidates this sorting is no longer necessary.
| }; | ||
|
|
||
| struct active_combo { | ||
| const struct combo_cfg *combo; | ||
| int16_t combo_idx; |
Keeping this draft for now, but this is a collection of improvements to remove a few combo Kconfig flags that are instead auto-calculated at build time, and reduce the RAM usage on more constrained devices.
A Le Chiffre STM32 Build on
main:Same keymap with this PR:
For a savings of 384 bytes.
Needs some more cleanup, and there's room for some performance improvements for the access to the new bit fields, but this should be ready for early testing.
PR check-list