[Core] get_keycode_string(): function to format keycodes as strings, for more readable debug logging.

[getreuer]

This PR adds get_keycode_string() as described in https://getreuer.info/posts/keyboards/keycode-string/index.html as a utility function in core.

Description

It's much nicer to read keycodes as names like "LT(2,KC_D)" than numerical codes like "0x4207." This PR adds a function get_keycode_string() for that purpose, based on my userspace implementation linked above.

How to use it

Add in rules.mk:

KEYCODE_STRING_ENABLE = yes

Then use the get_keycode_string(kc) function to convert a given 16-bit keycode to a human-readable string. Example:

uint16_t keycode = C(KC_PGUP);
const char *key_name = get_keycode_string(keycode);
dprintf("kc: %s\n", key_name); // Logs "kc: C(KC_PGUP)"

Limitations

• Many common QMK keycodes are recognized by get_keycode_string(), but not all. These include some common basic keycodes, layer switch keycodes, mod-taps, one-shot keycodes, tap dance keycodes, and Unicode keycodes. As a fallback, an unrecognized keycode is written as a hex number. Names for additional keycodes can also be added, see below.

• Keycodes are stringified according to the default KC_-prefixed keycodes, which is suboptimal when the computer is configured to a non-US-QWERTY layout.

Defining names for additional keycodes

Optionally, KEYCODE_STRING_NAMES_USER or KEYCODE_STRING_NAMES_KB may be used to add names for additional keycodes. For example, supposing keymap.c defines MYMACRO1 and MYMACRO2 as custom keycodes, the following adds their names:

KEYCODE_STRING_NAMES_USER(
    KEYCODE_STRING_NAME(MYMACRO1),
    KEYCODE_STRING_NAME(MYMACRO2),
);

Types of Changes

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

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

[getreuer]

Thanks for the quick review, @drashna! I've updated the PR according to your comments.

[drashna]

It looks like LTO really doesn't like the user/keeb strings (and even adding it via QUANTUM_LIB_SRC causes warnings).

[getreuer]

It looks like LTO really doesn't like the user/keeb strings (and even adding it via QUANTUM_LIB_SRC causes warnings).

Uh oh. The problem is with strings defined in keycode_string_names_kb and keycode_string_names_user? If so, I suspect how they are weak symbols is confusing to the optimizer, and perhaps I need to do this part differently.

[getreuer]

@drashna I made a tweak in 45f573f that builds and runs successfully with custom names on Voyager. The user syntax is slightly different (and uglier, unfortunately) from before: change

const keycode_string_name_t keycode_string_names_user[] = {
    KEYCODE_STRING_NAME(MYMACRO1),
    KEYCODE_STRING_NAME(MYMACRO2),
    KEYCODE_STRING_NAMES_END // End of table sentinel.
};

to

const keycode_string_name_t *keycode_string_names_user = (keycode_string_name_t []){
    KEYCODE_STRING_NAME(MYMACRO1),
    KEYCODE_STRING_NAME(MYMACRO2),
    KEYCODE_STRING_NAMES_END // End of table sentinel.
};

[drashna]

It looks like LTO really doesn't like the user/keeb strings (and even adding it via QUANTUM_LIB_SRC causes warnings).

Uh oh. The problem is with strings defined in keycode_string_names_kb and keycode_string_names_user? If so, I suspect how they are weak symbols is confusing to the optimizer, and perhaps I need to do this part differently.

Yeah, it was specifically with the const's yeah. It didn't like that they were different/declared differently.

@drashna I made a tweak in 45f573f that builds and runs successfully with custom names on Voyager. The user syntax is slightly different (and uglier, unfortunately) from before

Can confirm that this works, but yeah, it's definitely uglier.

(Also testing it out myself.... it will probably be a matter of time before somebody asks about layer names for those keycodes :D )

[getreuer]

@drashna here is another take on the API, hopefully nicer this way. In commit 8eaf67d, user keycode names are added like this: define KEYCODE_STRING_NAMES_USER in config.h

#define KEYCODE_STRING_NAMES_USER

and define keycode names in keymap.c as

const keycode_string_name_t keycode_string_names_user[] = {
    KEYCODE_STRING_NAME(MYMACRO1),
    KEYCODE_STRING_NAME(MYMACRO2),
};

This removes the ugly (keycode_string_name_t []) casting and also removes the need for the KEYCODE_STRING_NAMES_END sentinel entry at the end of the table.

Also testing it out myself.... it will probably be a matter of time before somebody asks about layer names for those keycodes :D

That would be cool! =D I'd be happy to add it. Should I hold off on that as a separate PR or go ahead and include in this one?

[tzarc]

This removes the ugly (keycode_string_name_t []) casting and also removes the need for the KEYCODE_STRING_NAMES_END sentinel entry at the end of the table.

This is preferred and matches the introspection changes for combos and the like.

Should I hold off on that as a separate PR or go ahead and include in this one?

Keep it separate, far easier for reviews to keep things limited to one item, and if precedents are set they can be reused.

[drashna]

Unfortunately, this causes issues. It expects that both the kb and user string arrays are gonna to be in the keymap.c. Which for the kb one array is likely not to be true.

At best, it would require the kb array to be put into the keyboard's h file

[getreuer]

Whoops! Right, thanks for catching that. I've reverted back to syntax

const keycode_string_name_t *keycode_string_names_user = (keycode_string_name_t []){
    KEYCODE_STRING_NAME(MYMACRO1),
    KEYCODE_STRING_NAME(MYMACRO2),
    KEYCODE_STRING_NAMES_END // End of table sentinel.
};

While I was at it, I realized that using PSTR() within the KEYCODE_STRING_NAME does not build on AVR (a compile error that, essentially, PSTR cannot be used within a braced expression). It's desirable to use PSTR at least for the common keycode names, since this is ~half a kilobyte of data. I've revised so that the _user and _kb names are in RAM, not using PSTR, while the common names use a separate representation still using PSTR, and in a way that builds on AVR.

[sigprof]

Another option could be to wrap the definition in a macro, somewhat similar to RGBLIGHT_LAYER_SEGMENTS (but more than that, because the definition needs to be performed by the macro too):

#define DEFINE_KEYCODE_STRING_NAMES_KB(...) \
    static const keycode_string_name_t keycode_string_names_kb_data[] = { __VA_ARGS__, KEYCODE_STRING_NAMES_END }; \
    const keycode_string_name_t *keycode_string_names_kb = keycode_string_names_kb_data

and similarly for the _user version.

[tzarc]

I’d say if we can get it in without introspection for now we’re fine. Can always give it another pass later if necessary — the introspection is somewhat of a red herring in this scenario because they’re not really intended to be runtime-reconfigurable.

I made mention of it mainly as an example of not using null terminators and the like as a pattern, not as a “please hook it into introspection too” request.

[getreuer]

@sigprof thank you for the suggestion! Using that, custom keycode names can now work with the interface

KEYCODE_STRING_NAMES_USER(
    KEYCODE_STRING_NAME(MYMACRO1),
    KEYCODE_STRING_NAME(MYMACRO2),
);

[KarlK90]

@getreuer Great stuff, this also opens up the opportunity to get rid of the unit-tests only implementation for deriving string representations... which I already did in my branch https://github.com/KarlK90/qmk_firmware/commits/core/keycode_string_unit_tests/ - let me know if you want to cherry-pick the commits in this PR otherwise I'll just open a PR myself afterwards.

[getreuer]

Great stuff, this also opens up the opportunity to get rid of the unit-tests only implementation for deriving string representations... which I already did in my branch https://github.com/KarlK90/qmk_firmware/commits/core/keycode_string_unit_tests/ - let me know if you want to cherry-pick the commits in this PR otherwise I'll just open a PR myself afterwards.

Thank you @KarlK90! Using get_keycode_string() also for strings in unit tests is a great idea.

A design question to weigh is that while get_keycode_string() covers most keycodes that are commonly used, there are many obscure ones that it misses. The test implementation get_keycode_identifier_or_default() OTOH is comprehensive (IIUC) through being based off of a generated list of keycode names. The gap is in various keycodes for toggling features like QK_AUTO_SHIFT_TOGGLE, the numerous MIDI note keycodes like QK_MIDI_NOTE_E_3, things like this.

Arguably, get_keycode_string() could be expanded to be comprehensive. I estimate that it would cost roughly 4 KB more flash space to store all the names. For the sake of keyboards with less memory, this data should probably be ifdef'd behind an "enable additional keycodes" option.

Or arguably, it is Ok that get_keycode_string() is not comprehensive. The function prints unknown keycodes as hex as a fallback, which can (in principle, with effort) be manually looked up to determine the meaning. Or where a significant keycode is missing, it is possible to add it through KEYCODE_STRING_NAMES_USER / _KB.

I suggest that reusing get_keycode_string() for get_keycode_identifier_or_default() is better to consider in a separate PR. Happy to hear your opinions on this!

[KarlK90]

Arguably, get_keycode_string() could be expanded to be comprehensive. I estimate that it would cost roughly 4 KB more flash space to store all the names. For the sake of keyboards with less memory, this data should probably be ifdef'd behind an "enable additional keycodes" option.

This is a good plan but IMHO this extension can happen in the future as the most frequently used keycodes are covered.

Or arguably, it is Ok that get_keycode_string() is not comprehensive. The function prints unknown keycodes as hex as a fallback, which can (in principle, with effort) be manually looked up to determine the meaning. Or where a significant keycode is missing, it is possible to add it through KEYCODE_STRING_NAMES_USER / _KB.

That is totally fine, the use-case are unit-tests and the implementer can extend the printing functions to get coverage (or just live with the fallback). From my POV their is greater gain in having only one maintained solution.

I suggest that reusing get_keycode_string() for get_keycode_identifier_or_default() is better to consider in a separate PR. Happy to hear your opinions on this!

Agreed, let's do this in separate PR :).

[tzarc]

Random thought -- perhaps for the undeclared keycodes if they're within a known range, you could do something like QK_MIDI+7? It might be valuable enough to know "hey, I just pressed a mod-tap vs. a layer-tap" (I know they're covered, example only)

[getreuer]

Random thought -- perhaps for the undeclared keycodes if they're within a known range, you could do something like QK_MIDI+7? It might be valuable enough to know "hey, I just pressed a mod-tap vs. a layer-tap" (I know they're covered, example only)

That's a nice solution! I've updated to do that.

[PeterMortensen]

What is the resource cost (if KEYCODE_STRING_ENABLE = yes), e.g., in terms of flash memory and RAM memory?

A fixed cost? Or is there some compile-time optimisation?

[getreuer]

What is the resource cost (if KEYCODE_STRING_ENABLE = yes), e.g., in terms of flash memory and RAM memory?

A fixed cost? Or is there some compile-time optimisation?

Good question. Most of the cost is in flash use to hold the keycode names. The RAM cost should be minor. On an earlier version of this feature, I measured a flash memory increase of about 1600 bytes when enabling it. This is not nothing but doable even on limited AVR processors, especially if enabled only while debugging.

If Keycode String is enabled but get_keycode_string() is never called, LTO may optimize it away so that it adds nothing to the firmware size. I don't think LTO can always be relied on in general, but I did see LTO eliminate an unused Keycode String successfully in my own builds.