From 07ff09720b654416e8db34a79d65e1a41b846bf9 Mon Sep 17 00:00:00 2001 From: Dave Date: Sun, 28 Nov 2021 15:28:03 -0600 Subject: [PATCH] Fix library-related indirection bug Accessing library functions via function pointer seems to cause a reset. Still investigating the cause. - compile keymaps subdir as part of main src cmakefile, rather than as a separate library. - rather than setup table of function pointers and using an indirection in the main code, set up a jump table (switch). This is cleaner. --- firmware/asdf/src/CMakeLists.txt | 29 ++++++++++++------- .../asdf/src/Keymaps/asdf_keymap_classic.c | 10 ++++++- .../src/Keymaps/asdf_keymap_classic_add_map.h | 8 ++--- .../src/Keymaps/asdf_keymap_classic_caps.c | 1 + firmware/asdf/src/asdf_keymap_table.c.in | 26 ++++++++--------- firmware/asdf/src/asdf_keymap_table.h.in | 19 ++++-------- firmware/asdf/src/asdf_keymaps.c | 18 +++++------- 7 files changed, 57 insertions(+), 54 deletions(-) diff --git a/firmware/asdf/src/CMakeLists.txt b/firmware/asdf/src/CMakeLists.txt index 45e7fd4..bc962b8 100644 --- a/firmware/asdf/src/CMakeLists.txt +++ b/firmware/asdf/src/CMakeLists.txt @@ -1,7 +1,7 @@ message("C compiler: ${CMAKE_C_COMPILER}") function(create_keymap_setups keymaps keymap_table) - list(TRANSFORM keymaps REPLACE "<\(.+\):\(.+\)>" "\n keymap_setup_function_lookup_table[\\2] = setup_\\1_keymap" OUTPUT_VARIABLE temp_list) + list(TRANSFORM keymaps REPLACE "<\(.+\):\(.+\)>" "\n case \\2: setup_\\1_keymap();break" OUTPUT_VARIABLE temp_list) # we can keep the ';' cmake list separators as the C statement separators. # However, we need to append an extra ';' at the end. string(APPEND temp_list ";") @@ -9,7 +9,7 @@ function(create_keymap_setups keymaps keymap_table) endfunction(create_keymap_setups) function(create_keymap_declarations keymaps keymap_decl) - list(TRANSFORM keymaps REPLACE "<\(.+\):\(.+\)>" "\nextern asdf_keymap_setup_function_t setup_\\1_keymap" OUTPUT_VARIABLE temp_list) + list(TRANSFORM keymaps REPLACE "<\(.+\):\(.+\)>" "\n void setup_\\1_keymap(void)" OUTPUT_VARIABLE temp_list) # we can keep the ';' cmake list separators as the C statement separators. # However, we need to append an extra ';' at the end. string(APPEND temp_list ";") @@ -39,13 +39,14 @@ set (ASDF_BIN_DIR ${CMAKE_CURRENT_BINARY_DIR}) configure_file(${ASDF_SRC_DIR}/Arch/asdf_arch_${ARCH}.h ${CMAKE_CURRENT_BINARY_DIR}/asdf_arch.h COPYONLY) configure_file(${ASDF_SRC_DIR}/Arch/asdf_arch_${ARCH}.c ${CMAKE_CURRENT_BINARY_DIR}/asdf_arch.c COPYONLY) -configure_file(${ASDF_SRC_DIR}/asdf_keymap_table.c.in ${CMAKE_CURRENT_BINARY_DIR}/asdf_keymap_table.c) -configure_file(${ASDF_SRC_DIR}/asdf_keymap_table.h.in ${CMAKE_CURRENT_BINARY_DIR}/asdf_keymap_table.h) +configure_file(${ASDF_SRC_DIR}/asdf_keymap_setup.c.in ${CMAKE_CURRENT_BINARY_DIR}/asdf_keymap_setup.c) +configure_file(${ASDF_SRC_DIR}/asdf_keymap_setup.h.in ${CMAKE_CURRENT_BINARY_DIR}/asdf_keymap_setup.h) list(APPEND CFLAGS -std=c99 +# -Wa,-adhln -Wall -funsigned-char -funsigned-bitfields @@ -72,20 +73,25 @@ list(APPEND CFLAGS list (APPEND SOURCES asdf.c ${CMAKE_CURRENT_BINARY_DIR}/asdf_arch.c - ${CMAKE_CURRENT_BINARY_DIR}/asdf_keymap_table.c + ${CMAKE_CURRENT_BINARY_DIR}/asdf_keymap_setup.c asdf_buffer.c asdf_hook.c - asdf_keymap_table.c + asdf_keymap_setup.c asdf_keymaps.c asdf_modifiers.c asdf_physical.c asdf_repeat.c asdf_virtual.c + asdf_print.c + Keymaps/asdf_keymap_classic.c + Keymaps/asdf_keymap_classic_caps.c + Keymaps/asdf_keymap_classic_add_map.c + Keymaps/asdf_keymap_sol.c main.c ) # create a library for the keymap modules -add_subdirectory(Keymaps) +#add_subdirectory(Keymaps) # add the executable if (COMMAND custom_add_executable) @@ -102,6 +108,8 @@ target_include_directories(${ASDF_EXECUTABLE_TARGET_NAME} PRIVATE ${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/Keymaps + ) target_compile_options(${ASDF_EXECUTABLE_TARGET_NAME} @@ -109,7 +117,6 @@ target_compile_options(${ASDF_EXECUTABLE_TARGET_NAME} ${CFLAGS} ) -target_link_libraries(${ASDF_EXECUTABLE_TARGET_NAME} - keymaps - ) - +#target_link_libraries(${ASDF_EXECUTABLE_TARGET_NAME} +# keymaps +# ) diff --git a/firmware/asdf/src/Keymaps/asdf_keymap_classic.c b/firmware/asdf/src/Keymaps/asdf_keymap_classic.c index a7396ea..15ac907 100644 --- a/firmware/asdf/src/Keymaps/asdf_keymap_classic.c +++ b/firmware/asdf/src/Keymaps/asdf_keymap_classic.c @@ -25,9 +25,11 @@ #include "asdf_keymaps.h" #include "asdf_virtual.h" +#include "asdf_modifiers.h" #include "asdf_keymap_classic.h" #include "asdf_keymap_classic_add_map.h" - +//FIXME: remove unused arch.h call +#include "asdf_arch.h" // PROCEDURE: // INPUTS: // OUTPUTS: @@ -45,10 +47,15 @@ void setup_classic_keymap(void) { + asdf_arch_send_code('*'); classic_add_map(CLASSIC_PLAIN_MAP, MOD_PLAIN_MAP); + asdf_arch_send_code('*'); classic_add_map(CLASSIC_CAPS_MAP, MOD_CAPS_MAP); + asdf_arch_send_code('*'); classic_add_map(CLASSIC_SHIFT_MAP, MOD_SHIFT_MAP); + asdf_arch_send_code('*'); classic_add_map(CLASSIC_CTRL_MAP, MOD_CTRL_MAP); + asdf_arch_send_code('*'); asdf_virtual_init(); @@ -65,6 +72,7 @@ void setup_classic_keymap(void) // assign the CLRSCR output to the virtual CLRSCR output, configure to produce a long pulse when activated asdf_virtual_assign(CLASSIC_VIRTUAL_CLR_SCR, CLASSIC_CLR_SCR_OUT, V_PULSE_LONG, !CLASSIC_CLR_SCR_ACTIVE_VALUE); + asdf_put_code('[');asdf_put_code('c');asdf_put_code(']'); } diff --git a/firmware/asdf/src/Keymaps/asdf_keymap_classic_add_map.h b/firmware/asdf/src/Keymaps/asdf_keymap_classic_add_map.h index e5b0e17..bf8d973 100644 --- a/firmware/asdf/src/Keymaps/asdf_keymap_classic_add_map.h +++ b/firmware/asdf/src/Keymaps/asdf_keymap_classic_add_map.h @@ -34,10 +34,10 @@ #include "asdf_modifiers.h" typedef enum { - CLASSIC_PLAIN_MAP, - CLASSIC_CAPS_MAP, - CLASSIC_SHIFT_MAP, - CLASSIC_CTRL_MAP, + CLASSIC_PLAIN_MAP = 0, + CLASSIC_CAPS_MAP = 1, + CLASSIC_SHIFT_MAP = 2, + CLASSIC_CTRL_MAP = 3, } classic_map_index_t; diff --git a/firmware/asdf/src/Keymaps/asdf_keymap_classic_caps.c b/firmware/asdf/src/Keymaps/asdf_keymap_classic_caps.c index f514830..920b41d 100644 --- a/firmware/asdf/src/Keymaps/asdf_keymap_classic_caps.c +++ b/firmware/asdf/src/Keymaps/asdf_keymap_classic_caps.c @@ -25,6 +25,7 @@ #include "asdf_keymaps.h" #include "asdf_virtual.h" +#include "asdf_modifiers.h" #include "asdf_keymap_classic.h" #include "asdf_keymap_classic_add_map.h" diff --git a/firmware/asdf/src/asdf_keymap_table.c.in b/firmware/asdf/src/asdf_keymap_table.c.in index 1d8a938..a935157 100644 --- a/firmware/asdf/src/asdf_keymap_table.c.in +++ b/firmware/asdf/src/asdf_keymap_table.c.in @@ -27,22 +27,18 @@ // this program. If not, see . // -#include "asdf_keymap_table.h" +#include @keymap_declarations@ -asdf_keymap_setup_function_t keymap_setup_function_lookup_table[ASDF_NUM_KEYMAPS]; - - -// PROCEDURE: asdf_keymap_table_init -// INPUTS: none +// PROCEDURE: asdf_keymap_setup +// +// INPUTS: (uint8_t) index - index of the keymap setup function to call // OUTPUTS: none +//// +// DESCRIPTION: This function calls the keymap setup function specified +// by the index. // -// DESCRIPTION: This function populates the keymap_setup_function_lookup_table -// with the various externally defined keymap setup functions, in the order -// specified in the keymap_list.cmake file. -// - // SIDE EFFECTS: See Description // // NOTES: This function is necessary because functions defined in external @@ -54,8 +50,12 @@ asdf_keymap_setup_function_t keymap_setup_function_lookup_table[ASDF_NUM_KEYMAPS // COMPLEXITY: 1 // -void asdf_keymap_table_init(void) { - @keymap_table@ +void asdf_keymap_setup(uint8_t index) { + switch(index) { +@keymap_table@ + default: + break; + } }; //-------|---------|---------+---------+---------+---------+---------+---------+ diff --git a/firmware/asdf/src/asdf_keymap_table.h.in b/firmware/asdf/src/asdf_keymap_table.h.in index 6dc1f42..3b14739 100644 --- a/firmware/asdf/src/asdf_keymap_table.h.in +++ b/firmware/asdf/src/asdf_keymap_table.h.in @@ -29,22 +29,13 @@ #define ASDF_NUM_KEYMAPS @num_keymaps@ -// define the type for a keymap setup function. Keymaps are registerd by storing -// a keymap setup function in the keymap setup array. -typedef void (*asdf_keymap_setup_function_t)(void); - - -// PROCEDURE: asdf_keymap_table_init -// INPUTS: none +// PROCEDURE: asdf_keymap_setup +// INPUTS: (uint8_t) index - index of the keymap setup function to call // OUTPUTS: none -// DESCRIPTION: This function populates the keymap_setup_function_lookup_table -// with the various externally defined keymap setup functions, in the order -// specified in the keymap_list.cmake file. +// DESCRIPTION: This function calls the keymap setup function specified +// by the index. // SIDE EFFECTS: See Description -// NOTES: This function is necessary because functions defined in external -// modules are not considered compile-time constants by the C99 standard, so the -// array cannot be initialized with a compile-time declaration. -void asdf_keymap_table_init(void); +void asdf_keymap_setup(uint8_t index); //-------|---------|---------+---------+---------+---------+---------+---------+ // Above line is 80 columns, and should display completely in the editor. diff --git a/firmware/asdf/src/asdf_keymaps.c b/firmware/asdf/src/asdf_keymaps.c index d24a5d7..5f6c255 100644 --- a/firmware/asdf/src/asdf_keymaps.c +++ b/firmware/asdf/src/asdf_keymaps.c @@ -20,7 +20,6 @@ // You should have received a copy of the GNU General Public License along with // this program. If not, see . -#include //FIXME #include #include #include "asdf_config.h" @@ -29,7 +28,7 @@ #include "asdf_physical.h" #include "asdf_virtual.h" #include "asdf_hook.h" -#include "asdf_keymap_table.h" +#include "asdf_keymap_setup.h" #include "asdf_keymaps.h" #include "asdf_modifiers.h" @@ -37,11 +36,6 @@ // This structure is populated using the asdf_keymaps_add_map() function. static asdf_keycode_map_t keymaps[ASDF_MOD_NUM_MODIFIERS] = {}; -// List of keymap setup routines. Each keymap setup routine is responsible for -// populating the keymap matrices, setting up virtual devices, and setting up -// keymap-specific function hooks and initial conditions for the keymap. -extern asdf_keymap_setup_function_t keymap_setup_function_lookup_table[ASDF_NUM_KEYMAPS]; - // The current keymap index. This is stored so bitwise operators on the keymap index can be performed. static uint8_t current_keymap_index; @@ -116,7 +110,6 @@ uint8_t asdf_keymaps_num_cols(void) return keymaps[asdf_modifier_index()].cols; } - // PROCEDURE: asdf_keymaps_reset // INPUTS: none // OUTPUTS: none @@ -169,10 +162,15 @@ static void asdf_keymaps_reset(void) void asdf_keymaps_select(uint8_t index) { if (index < ASDF_NUM_KEYMAPS) { + + asdf_arch_init(); asdf_keymaps_reset(); current_keymap_index = index; - keymap_setup_function_lookup_table[index](); + + asdf_print("\r\nIndex is %d.", index); + asdf_keymap_setup(index); + asdf_print("\r\nAfter setup function\n"); } } @@ -207,8 +205,6 @@ void asdf_keymaps_dummy_function(void) {} // void asdf_keymaps_init(void) { - asdf_hook_init(); - asdf_keymap_table_init(); asdf_keymaps_select(0); }