Ikcelaks / qmk_sequence_transform

Sequence-Transform is a user library for QMK that enables a rich declarative ruleset for transforming a sequence of keypresses into any output you would like.
Apache License 2.0
6 stars 3 forks source link

st_debug #65

Closed kamih closed 7 months ago

kamih commented 7 months ago

This PR covers three loosely related cleanup issues that will improve code quality and help us debug our work.

58 - We can now use the st_debug macro function to keep useful debug logging code around, without having to comment/uncomment it all the time. Example:

st_debug(ST_DBG_SEQ_MATCH, "Chaining Offset: %d; Code: %#04X\n", offset, code);

If SEQUENCE_TRANSFORM_DEBUG is set to 0, or the ST_DBG_SEQ_MATCH flag hasn't been set via st_debug_set_flag or st_debug_set_flag_str, this call will do nothing. If you need to call a function only for debugging (or wrap a block a code), you can use the st_debug_check macro function. Example:

if (st_debug_check(ST_DBG_GENERAL)) {
    st_key_buffer_print(buf);
}

The tester now accepts -d command line options to enable these flags. The -h option has been updated to show the available flags. Example:

./tester -d rule_search

59 - The SANITY_CHECK define has been removed. The new st_assert macro function is now used instead. This macro should be used whenever we want to make sure our assumptions are correct. Example:

st_assert(code, "Unexpected null code! Offset: %d\n", offset);

When compiling for the firmware, this call does nothing. In the tester, a failing assert will print a message and call abort, which gives us to opportunity to attach a debugger to investigate the issue. All CDATA and TDATA calls are now using this to guard against OOB indexing.

60 - ST feature/option defaults are now defined in st_defaults.h (which should be the first file included by our .c files if they need to use any of these defines). WARNING: some have been renamed!

Here are the current default values:

#define SEQUENCE_TRANSFORM_IDLE_TIMEOUT 2500
#define SEQUENCE_TRANSFORM_ENHANCED_BACKSPACE 1
#define SEQUENCE_TRANSFORM_FALLBACK_BUFFER 1
#define SEQUENCE_TRANSFORM_DEBUG 0
#define SEQUENCE_TRANSFORM_LOG_TIME 0
#define SEQUENCE_TRANSFORM_RULE_SEARCH 0
#define SEQUENCE_TRANSFORM_RULE_SEARCH_MAX_SKIP 4
#define SEQUENCE_TRANSFORM_RECORD_RULE_USAGE 0

If a user wants to change one of these, they should set it directly in their layout's config.h. (The tester Makefile automatically sets the ones it needs to be able to test everything.)

Ikcelaks commented 7 months ago

This looks really good.

I ran into only one obvious issue: the code doesn't compile with CONSOLE=no, because the compiler detects unused variables in the debug_rule_match function in trie.c. I'm pretty sure this was a pre-existing issue that I didn't notice until I explicitly tested building with all of the new enable flags. I can't make a comment on the relevant code, because it doesn't show up in the changes here.

I'm running a build off of this PR on my keyboard right now, and it works the same as before, as expected.

I'm good with merging this as soon as the issue with building when CONSOLE=no is addressed. (Locally I fixed it by wrapping the entire body of the function in an #if CONSOLE block, but I don't understand these issues as well as you, so maybe you have a better solution.