Dualbank Autocorrect #22788

780nm · 2023-12-31T07:48:34Z

Description

Allows for users to define two banks of autocorrect rules and switch between them dynamically. Bank selection is persisted in EEPROM, and persisted selection is loaded on wake of keyboard. Code for bank switching is not compiled if no alternate bank is found, thus eliminating unneeded memory footprint. This PR adds a new keycode (which I hope I've added correctly!) and extends the QMK python CLI to allow for easy generation of the alternate dictionary files. I've updated the documentation, please feel free to ask questions and make suggestions if anything is unclear.

Any pointers with regards to code formatting and peprocessor directives are appreciated, although I've tried to follow guidelines to the best of my ability.

Thanks!

-Sean

Types of Changes

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

drashna · 2024-01-01T23:44:17Z

quantum/process_keycode/process_autocorrect.c

@@ -19,8 +19,24 @@
 #    include "autocorrect_data_default.h"
 #endif

-static uint8_t typo_buffer[AUTOCORRECT_MAX_LENGTH] = {KC_SPC};
-static uint8_t typo_buffer_size                    = 1;
+#ifdef AUTOCORRECT_MULTI_BANK


Not sure this is the best way to handle it, but:

Suggested change

#ifdef AUTOCORRECT_MULTI_BANK

#if defined(AUTOCORRECT_MULTI_BANK) && __has_include("autocorrect_data_alt.h")

That's closer to what the other guard was, so I've made the change, but I don't know enough C to be sure which is preferred

drashna

"dual bank" seems more accurate, than "multibank".

getreuer

Thank you for your contribution!

getreuer · 2024-02-01T07:05:15Z

docs/feature_autocorrect.md

+useful for bilingual users or for running context-specific rulesets.
+`QK_AUTOCORRECT_BANK_TOGGLE` can then be used to toggle the active dictionary and persist the selection to eeprom.
+
+To use this feature, `autocorrect_data_alt.h` should be generated using the flag `-a`, which constructs an alternate dictionary with appropriately named constants


Nit: The sentence is missing a period . at the end.

getreuer · 2024-02-01T07:19:39Z

lib/python/qmk/cli/generate/autocorrect_data.py

@@ -263,8 +264,12 @@ def generate_autocorrect_data(cli):
    current_keyboard = cli.args.keyboard or cli.config.user.keyboard or cli.config.generate_autocorrect_data.keyboard
    current_keymap = cli.args.keymap or cli.config.user.keymap or cli.config.generate_autocorrect_data.keymap

+    file_name = 'autocorrect_data_alt.h' if cli.args.alternate else 'autocorrect_data.h'
+    defines_suffix = '_ALT' if cli.args.alternate else ''
+    static_suffix = '_alt' if cli.args.alternate else ''


The existing code is fine. But to code golf this a bit, it would arguably be simpler to define only

suffix = '_ALT' if cli.args.alternate else ''

then use {suffix.lower()} in the one occurrence below where the lowercase version of the suffix is needed.

yep, good point

getreuer · 2024-02-01T07:22:43Z

quantum/process_keycode/process_autocorrect.c

-static uint8_t typo_buffer_size                    = 1;
+#ifdef AUTOCORRECT_MULTI_BANK
+#    include "autocorrect_data_alt.h"
+#    pragma message "Autocorrect is using multibank."


Since AUTOCORRECT_MULTI_BANK is an opt-in feature, it seems superfluous and a bit spammy to have a pragma message that it is enabled. I'd remove it.

getreuer · 2024-02-01T07:52:42Z

quantum/process_keycode/process_autocorrect.c

+#ifdef AUTOCORRECT_MULTI_BANK
+#    include "autocorrect_data_alt.h"
+#    pragma message "Autocorrect is using multibank."
+#    define AML ((AUTOCORRECT_MAX_LENGTH > AUTOCORRECT_MAX_LENGTH_ALT) ? AUTOCORRECT_MAX_LENGTH : AUTOCORRECT_MAX_LENGTH_ALT)


For sake of readability, abbreviations in names are for the most part best avoided. Maybe call this "TYPO_BUFFER_SIZE" to be more descriptive.

Suggested change

# define AML ((AUTOCORRECT_MAX_LENGTH > AUTOCORRECT_MAX_LENGTH_ALT) ? AUTOCORRECT_MAX_LENGTH : AUTOCORRECT_MAX_LENGTH_ALT)

# define TYPO_BUFFER_SIZE ((AUTOCORRECT_MAX_LENGTH > AUTOCORRECT_MAX_LENGTH_ALT) ? AUTOCORRECT_MAX_LENGTH : AUTOCORRECT_MAX_LENGTH_ALT)

getreuer · 2024-02-01T08:08:11Z

quantum/process_keycode/process_autocorrect.c

    uint16_t state = 0;
-    uint8_t  code  = pgm_read_byte(autocorrect_data + state);
+    uint8_t  code  = pgm_read_byte(AUTOCORRECT_DATA + state);


It's confusing that AUTOCORRECT_DATA is a macro expanding to either autocorrect_data or bank_data, yet #ifdefs are used for other parameters like AUTOCORRECT_MIN_LENGTH vs. bank_min_length.

To unify naming and simplify away the #ifdefs, how about define a set of macros above like

#ifdef AUTOCORRECT_MULTI_BANK // Dual bank. #define CURRENT_DATA bank_data #define CURRENT_MIN_LENGTH bank_min_length #define CURRENT_MAX_LENGTH bank_max_length #define CURRENT_SIZE bank_size #else // Single bank. #define CURRENT_DATA autocorrect_data #define CURRENT_MIN_LENGTH AUTOCORRECT_MIN_LENGTH #define CURRENT_MAX_LENGTH AUTOCORRECT_MAX_LENGTH #define CURRENT_SIZE DICTIONARY_SIZE #endif

This way the bank_ variables are used when the option is enabled or the constants if not, and the "current" prefix makes clear that these parameters can change.

Makes sense! I implemented this alongside the struct packing change, which cleaned things up nicely.

getreuer · 2024-02-01T08:36:11Z

quantum/process_keycode/process_autocorrect.c

+static const uint8_t *bank_data       = autocorrect_data;
+static uint16_t       bank_min_length = AUTOCORRECT_MIN_LENGTH;
+static uint16_t       bank_max_length = AUTOCORRECT_MAX_LENGTH;
+static uint16_t       bank_size       = DICTIONARY_SIZE;


Make a "bank_params_t" struct for bank parameters. Then the bank-related definitions can be corralled like:

typedef struct { const uint8_t* data; uint16_t min_length; uint16_t max_length; uint16_t size; } bank_params_t; #define PRIMARY_BANK (bank_params_t){ \ .data = autocorrect_data, \ .min_length = AUTOCORRECT_MIN_LENGTH, \ .max_length = AUTOCORRECT_MAX_LENGTH, \ .size = DICTIONARY_SIZE, \ } #define ALT_BANK (bank_params_t){ \ .data = autocorrect_data_alt, \ .min_length = AUTOCORRECT_MIN_LENGTH_ALT, \ .max_length = AUTOCORRECT_MAX_LENGTH_ALT, \ .size = DICTIONARY_SIZE_ALT, \ } static bank_params_t bank = PRIMARY_BANK; void autocorrect_init_bank(void) { typo_buffer_size = 0; bank = keymap_config.autocorrect_bank ? ALT_BANK : PRIMARY_BANK; }

that's much cleaner, thanks!

getreuer · 2024-02-01T08:51:20Z

docs/feature_autocorrect.md

+| `autocorrect_disable()`    | Turns Autocorrect off.                                                           |
+| `autocorrect_toggle()`     | Toggles Autocorrect.                                                             |
+| `autocorrect_is_enabled()` | Returns true if Autocorrect is currently on.                                     |
+| `autocorrect_bank_toggle()`| Toggles and initializes Autocorrect dictionary (if multiple present)             |


Dictionaries are constant data, they do not need to be initialized. The only initialization made is that the typo buffer is cleared, which is probably too low level to be worth mentioning. I think the description written for AC_BANK is clear and could be reused here:

Suggested change

| `autocorrect_bank_toggle()`| Toggles and initializes Autocorrect dictionary (if multiple present) |

| `autocorrect_bank_toggle()`| Toggles the dictionary in use by the Autocorrect feature (if an alternate is present) |

getreuer · 2024-02-01T09:05:39Z

docs/feature_autocorrect.md

+| `autocorrect_toggle()`     | Toggles Autocorrect.                                                             |
+| `autocorrect_is_enabled()` | Returns true if Autocorrect is currently on.                                     |
+| `autocorrect_bank_toggle()`| Toggles and initializes Autocorrect dictionary (if multiple present)             |
+| `autocorrect_init_bank()`  | Initializes current dictionary according so selected bank  (if multiple present) |


Is autocorrect_init_bank() useful to users? AIUI, the point of this function is to set the EEPROM bank selection in suspend_wakeup_init_quantum(), which is pretty specific. I suggest omitting this function from the documentation.

fair point, it's more of an implementation detail; I removed the mention

780nm · 2024-03-01T10:17:46Z

Thanks @drashna and @getreuer for the excellent feedback! Sorry for letting this sit for so long, work kept me busy. I've made the requested changes and retested the feature, I haven't seen any regressions. Your suggestion for cleaning up the swapping logic using a struct worked out a lot nicer than my previous approach.

780nm · 2024-03-01T10:18:18Z

I've also rebased on the latest develop, so this should merge without issue

elpekenin · 2024-03-01T13:55:17Z

FWIW: i openned a PR against your changes to make them more flexible, which you apparently missed (:

No hardcoding to 2 elements, can have arbitrary amount of them
Single file/data structure with all of them (no __has_include mess nor #ifdef MULTI check needed)
Single command invocation, passing all the files
Arguably 2 is already more than what most users will use... But the code was somewhat simpler and extendible imo.

780nm · 2024-03-01T16:25:55Z

@elpekenin Ah! you are right; I completely missed it, sorry about that. I just gave it a look over, and it's a reasonable generalization to make. I like the struct corralling suggested by @getreuer, so I'll merge you changes into my fork and add back the struct changes. the result should be the best of both worlds. I've changed the PR to draft in the meantime. Thanks for pointing that out! I see your changes take 3 EEPROM bits for saving the current dict, which I think is probably fine, but I recall the current count being close to the next byte over so any thoughts on that would be appreciated

[Enhancement] *Multi*dict

elpekenin · 2024-03-01T18:44:36Z

I barely tested the changes, so please do!

elpekenin

A quick sanity check

elpekenin · 2024-03-08T17:52:54Z

lib/python/qmk/cli/generate/autocorrect_data.py

 @cli.argument('-kb', '--keyboard', type=keyboard_folder, completer=keyboard_completer, help='The keyboard to build a firmware for. Ignored when a configurator export is supplied.')
 @cli.argument('-km', '--keymap', completer=keymap_completer, help='The keymap to build a firmware for. Ignored when a configurator export is supplied.')
 @cli.argument('-o', '--output', arg_only=True, type=normpath, help='File to write to')
 @cli.argument('-q', '--quiet', arg_only=True, action='store_true', help="Quiet mode, only output error messages")
+@cli.argument('-a', '--alternate', arg_only=True, action='store_true', help="Create an alternate dictionary file")


Not needed anymore

Suggested change

@cli.argument('-a', '--alternate', arg_only=True, action='store_true', help="Create an alternate dictionary file")

elpekenin · 2024-03-08T17:53:29Z

lib/python/qmk/cli/generate/autocorrect_data.py

+    file_name = 'autocorrect_data_alt.h' if cli.args.alternate else 'autocorrect_data.h'
+    defines_suffix = '_ALT' if cli.args.alternate else ''
+    static_suffix = '_alt' if cli.args.alternate else ''


Not needed either, we have a single file and single data structure now

elpekenin · 2024-03-08T17:56:54Z

lib/python/qmk/cli/generate/autocorrect_data.py

+    data = []
+    maxs = []
+    mins = []
+    sizes = []


Could make this a one liner instead:

Suggested change

data = []

maxs = []

mins = []

sizes = []

data, maxs, mins, sizes = [], [], [], []

elpekenin · 2024-03-08T17:59:25Z

lib/python/qmk/cli/generate/autocorrect_data.py

    autocorrect_data_h_lines.append('')
-    autocorrect_data_h_lines.append('static const uint8_t autocorrect_data[DICTIONARY_SIZE] PROGMEM = {')
+    autocorrect_data_h_lines.append(f'static const uint8_t autocorrect_data{static_suffix}[DICTIONARY_SIZE{defines_suffix}] PROGMEM = {{')


Single structure

Suggested change

autocorrect_data_h_lines.append(f'static const uint8_t autocorrect_data{static_suffix}[DICTIONARY_SIZE{defines_suffix}] PROGMEM = {{')

autocorrect_data_h_lines.append(f'static const uint8_t autocorrect_data[DICTIONARY_SIZE] PROGMEM = {{')

elpekenin · 2024-03-08T18:00:30Z

quantum/keycode_config.h

@@ -44,6 +44,7 @@ typedef union {
        bool oneshot_enable : 1;
        bool swap_escape_capslock : 1;
        bool autocorrect_enable : 1;
+        uint8_t autocorrect_curr_dict : 3;


This (and respective check on the Py code-gen) should perhaps be lowered to 2 or even 1 bit, to not exhaust all available space in current EEPROM footprint.

elpekenin · 2024-03-08T18:01:05Z

quantum/process_keycode/process_autocorrect.h

+#if __has_include("autocorrect_data_alt.h")
+#    define AUTOCORRECT_MULTI_BANK
+#endif


Suggested change

#if __has_include("autocorrect_data_alt.h")

# define AUTOCORRECT_MULTI_BANK

#endif

elpekenin · 2024-03-08T18:01:26Z

quantum/quantum.c

+#ifdef AUTOCORRECT_ENABLE
+    // refresh autocorrect bank
+    autocorrect_init_dict();
+


Suggested change

#endif

780nm added 2 commits December 30, 2023 23:37

Multibank autocorrection

ae96105

keycodes and docs for multi-bank autocorrect

d87f602

github-actions bot added core documentation python cli qmk cli command dd Data Driven Changes labels Dec 31, 2023

regen keycode_table and attempt to fix c, py fmt

9fcba5e

drashna reviewed Jan 1, 2024

View reviewed changes

elpekenin added 2 commits January 4, 2024 17:28

Actual multi-dict

f23809b

Safety check

48bc239

getreuer suggested changes Feb 1, 2024

View reviewed changes

780nm changed the title ~~Multibank Autocorrect~~ Dualbank Autocorrect Mar 1, 2024

780nm added 3 commits February 29, 2024 21:32

Multibank autocorrection

ae93e5e

keycodes and docs for multi-bank autocorrect

64b8fef

regen keycode_table and attempt to fix c, py fmt

b1a83d2

780nm force-pushed the multibank-autocorrect branch from 9fcba5e to 88d04fc Compare March 1, 2024 07:08

780nm marked this pull request as draft March 1, 2024 16:25

780nm force-pushed the multibank-autocorrect branch from 88d04fc to b1a83d2 Compare March 1, 2024 18:20

780nm added 2 commits March 1, 2024 08:38

Merge branch 'multibank-autocorrect' into multibank

4816b06

Merge pull request #1 from elpekenin/multibank

cd90826

[Enhancement] *Multi*dict

elpekenin suggested changes Mar 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dualbank Autocorrect #22788

Dualbank Autocorrect #22788

780nm commented Dec 31, 2023 •

edited

Loading

drashna Jan 1, 2024

780nm Mar 1, 2024

drashna left a comment

getreuer left a comment

getreuer Feb 1, 2024

getreuer Feb 1, 2024

780nm Mar 1, 2024

getreuer Feb 1, 2024

780nm Mar 1, 2024

getreuer Feb 1, 2024

getreuer Feb 1, 2024

780nm Mar 1, 2024

getreuer Feb 1, 2024

780nm Mar 1, 2024

getreuer Feb 1, 2024

getreuer Feb 1, 2024

780nm Mar 1, 2024

780nm commented Mar 1, 2024

780nm commented Mar 1, 2024 •

edited

Loading

elpekenin commented Mar 1, 2024

780nm commented Mar 1, 2024 •

edited

Loading

elpekenin commented Mar 1, 2024

elpekenin left a comment

elpekenin Mar 8, 2024

elpekenin Mar 8, 2024

elpekenin Mar 8, 2024

elpekenin Mar 8, 2024

elpekenin Mar 8, 2024

elpekenin Mar 8, 2024

elpekenin Mar 8, 2024

	#ifdef AUTOCORRECT_MULTI_BANK
	#if defined(AUTOCORRECT_MULTI_BANK) && __has_include("autocorrect_data_alt.h")

	# define AML ((AUTOCORRECT_MAX_LENGTH > AUTOCORRECT_MAX_LENGTH_ALT) ? AUTOCORRECT_MAX_LENGTH : AUTOCORRECT_MAX_LENGTH_ALT)
	# define TYPO_BUFFER_SIZE ((AUTOCORRECT_MAX_LENGTH > AUTOCORRECT_MAX_LENGTH_ALT) ? AUTOCORRECT_MAX_LENGTH : AUTOCORRECT_MAX_LENGTH_ALT)

	\| `autocorrect_bank_toggle()`\| Toggles and initializes Autocorrect dictionary (if multiple present) \|
	\| `autocorrect_bank_toggle()`\| Toggles the dictionary in use by the Autocorrect feature (if an alternate is present) \|

	autocorrect_data_h_lines.append(f'static const uint8_t autocorrect_data{static_suffix}[DICTIONARY_SIZE{defines_suffix}] PROGMEM = {{')
	autocorrect_data_h_lines.append(f'static const uint8_t autocorrect_data[DICTIONARY_SIZE] PROGMEM = {{')

	#if __has_include("autocorrect_data_alt.h")
	# define AUTOCORRECT_MULTI_BANK
	#endif

Dualbank Autocorrect #22788

Are you sure you want to change the base?

Dualbank Autocorrect #22788

Conversation

780nm commented Dec 31, 2023 • edited Loading

Description

Types of Changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drashna left a comment

Choose a reason for hiding this comment

getreuer left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

780nm commented Mar 1, 2024

780nm commented Mar 1, 2024 • edited Loading

elpekenin commented Mar 1, 2024

780nm commented Mar 1, 2024 • edited Loading

elpekenin commented Mar 1, 2024

elpekenin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

780nm commented Dec 31, 2023 •

edited

Loading

780nm commented Mar 1, 2024 •

edited

Loading

780nm commented Mar 1, 2024 •

edited

Loading