From 7009f549c80d3d47687bc93b93d127815d5a0ae9 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Mon, 4 Mar 2024 15:54:57 -0800 Subject: [PATCH 1/6] Bullet proof spacecadet better against corrupted eeprom --- .../src/kaleidoscope/plugin/SpaceCadetConfig.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/Kaleidoscope-SpaceCadet/src/kaleidoscope/plugin/SpaceCadetConfig.cpp b/plugins/Kaleidoscope-SpaceCadet/src/kaleidoscope/plugin/SpaceCadetConfig.cpp index 404ff1da2a..95c8d7bae1 100644 --- a/plugins/Kaleidoscope-SpaceCadet/src/kaleidoscope/plugin/SpaceCadetConfig.cpp +++ b/plugins/Kaleidoscope-SpaceCadet/src/kaleidoscope/plugin/SpaceCadetConfig.cpp @@ -37,8 +37,10 @@ EventHandlerResult SpaceCadetConfig::onSetup() { } void SpaceCadetConfig::disableSpaceCadetIfUnconfigured() { - if (Runtime.storage().isSliceUninitialized(settings_base_, sizeof(SpaceCadet::settings_))) + if (Runtime.storage().isSliceUninitialized(settings_base_, sizeof(SpaceCadet::settings_)) || + (::SpaceCadet.settings_.mode != SpaceCadet::Mode::ON && ::SpaceCadet.settings_.mode != SpaceCadet::Mode::NO_DELAY)) { ::SpaceCadet.disable(); + } } EventHandlerResult SpaceCadetConfig::onFocusEvent(const char *input) { From 3168951468e2516945140e7c37676668a21e492d Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Mon, 4 Mar 2024 17:12:46 -0800 Subject: [PATCH 2/6] Refactor EEPROM-Settings commands to remove a switch statement and an enum --- .../kaleidoscope/plugin/EEPROM-Settings.cpp | 65 ++++--------------- 1 file changed, 12 insertions(+), 53 deletions(-) diff --git a/plugins/Kaleidoscope-EEPROM-Settings/src/kaleidoscope/plugin/EEPROM-Settings.cpp b/plugins/Kaleidoscope-EEPROM-Settings/src/kaleidoscope/plugin/EEPROM-Settings.cpp index 8564e52d51..8c09b5238b 100644 --- a/plugins/Kaleidoscope-EEPROM-Settings/src/kaleidoscope/plugin/EEPROM-Settings.cpp +++ b/plugins/Kaleidoscope-EEPROM-Settings/src/kaleidoscope/plugin/EEPROM-Settings.cpp @@ -188,13 +188,6 @@ bool EEPROMSettings::isSliceValid(uint16_t start, size_t size) { /** Focus **/ EventHandlerResult FocusSettingsCommand::onFocusEvent(const char *input) { - enum { - DEFAULT_LAYER, - IS_VALID, - GET_VERSION, - GET_CRC, - } sub_command; - const char *cmd_defaultLayer = PSTR("settings.defaultLayer"); const char *cmd_isValid = PSTR("settings.valid?"); const char *cmd_version = PSTR("settings.version"); @@ -203,19 +196,7 @@ EventHandlerResult FocusSettingsCommand::onFocusEvent(const char *input) { if (::Focus.inputMatchesHelp(input)) return ::Focus.printHelp(cmd_defaultLayer, cmd_isValid, cmd_version, cmd_crc); - if (::Focus.inputMatchesCommand(input, cmd_defaultLayer)) - sub_command = DEFAULT_LAYER; - else if (::Focus.inputMatchesCommand(input, cmd_isValid)) - sub_command = IS_VALID; - else if (::Focus.inputMatchesCommand(input, cmd_version)) - sub_command = GET_VERSION; - else if (::Focus.inputMatchesCommand(input, cmd_crc)) - sub_command = GET_CRC; - else - return EventHandlerResult::OK; - - switch (sub_command) { - case DEFAULT_LAYER: { + if (::Focus.inputMatchesCommand(input, cmd_defaultLayer)) { if (::Focus.isEOL()) { ::Focus.send(::EEPROMSettings.default_layer()); } else { @@ -223,29 +204,20 @@ EventHandlerResult FocusSettingsCommand::onFocusEvent(const char *input) { ::Focus.read(layer); ::EEPROMSettings.default_layer(layer); } - break; - } - case IS_VALID: + } else if (::Focus.inputMatchesCommand(input, cmd_isValid)) { ::Focus.send(::EEPROMSettings.isValid()); - break; - case GET_VERSION: + } else if (::Focus.inputMatchesCommand(input, cmd_version)) { ::Focus.send(::EEPROMSettings.version()); - break; - case GET_CRC: + } else if (::Focus.inputMatchesCommand(input, cmd_crc)) { ::Focus.sendRaw(::CRCCalculator.crc, F("/"), ::EEPROMSettings.crc()); - break; + } else { + return EventHandlerResult::OK; } return EventHandlerResult::EVENT_CONSUMED; } EventHandlerResult FocusEEPROMCommand::onFocusEvent(const char *input) { - enum { - CONTENTS, - FREE, - ERASE, - } sub_command; - const char *cmd_contents = PSTR("eeprom.contents"); const char *cmd_free = PSTR("eeprom.free"); const char *cmd_erase = PSTR("eeprom.erase"); @@ -253,17 +225,7 @@ EventHandlerResult FocusEEPROMCommand::onFocusEvent(const char *input) { if (::Focus.inputMatchesHelp(input)) return ::Focus.printHelp(cmd_contents, cmd_free, cmd_erase); - if (::Focus.inputMatchesCommand(input, cmd_contents)) - sub_command = CONTENTS; - else if (::Focus.inputMatchesCommand(input, cmd_free)) - sub_command = FREE; - else if (::Focus.inputMatchesCommand(input, cmd_erase)) - sub_command = ERASE; - else - return EventHandlerResult::OK; - - switch (sub_command) { - case CONTENTS: { + if (::Focus.inputMatchesCommand(input, cmd_contents)) { if (::Focus.isEOL()) { for (uint16_t i = 0; i < Runtime.storage().length(); i++) { uint8_t d = Runtime.storage().read(i); @@ -277,21 +239,18 @@ EventHandlerResult FocusEEPROMCommand::onFocusEvent(const char *input) { } Runtime.storage().commit(); } - - break; - } - case FREE: + } else if (::Focus.inputMatchesCommand(input, cmd_free)) { ::Focus.send(Runtime.storage().length() - ::EEPROMSettings.used()); - break; - case ERASE: { + } else if (::Focus.inputMatchesCommand(input, cmd_erase)) { for (uint16_t i = 0; i < Runtime.storage().length(); i++) { Runtime.storage().update(i, EEPROMSettings::EEPROM_UNINITIALIZED_BYTE); } Runtime.storage().commit(); Runtime.device().rebootBootloader(); - break; - } + } else { + return EventHandlerResult::OK; } + return EventHandlerResult::EVENT_CONSUMED; } From 26fe241c97fd1abf33489479ebebc45bc2f1a908 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Mon, 4 Mar 2024 17:13:30 -0800 Subject: [PATCH 3/6] In MouseKeysConfig, only write our settings if we've changed them --- .../src/kaleidoscope/plugin/MouseKeysConfig.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeysConfig.cpp b/plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeysConfig.cpp index a1f0fedb0b..a50617fd87 100644 --- a/plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeysConfig.cpp +++ b/plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeysConfig.cpp @@ -137,12 +137,13 @@ EventHandlerResult MouseKeysConfig::onFocusEvent(const char *input) { ::MouseKeys.setWarpGridSize(arg); break; } + // Update settings stored in EEPROM, and indicate that this Focus event has + // been handled successfully. + Runtime.storage().put(settings_base_, ::MouseKeys.settings_); + Runtime.storage().commit(); } - // Update settings stored in EEPROM, and indicate that this Focus event has - // been handled successfully. - Runtime.storage().put(settings_base_, ::MouseKeys.settings_); - Runtime.storage().commit(); + return EventHandlerResult::EVENT_CONSUMED; } From 8235b59261ca39b3c0c9ac9d3810f0f5630f95d9 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Mon, 4 Mar 2024 17:13:58 -0800 Subject: [PATCH 4/6] In EscapeOneshot, only write our settings if they've been changed --- .../src/kaleidoscope/plugin/Escape-OneShot-Config.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/Kaleidoscope-Escape-OneShot/src/kaleidoscope/plugin/Escape-OneShot-Config.cpp b/plugins/Kaleidoscope-Escape-OneShot/src/kaleidoscope/plugin/Escape-OneShot-Config.cpp index c8c9c684be..8ce716e782 100644 --- a/plugins/Kaleidoscope-Escape-OneShot/src/kaleidoscope/plugin/Escape-OneShot-Config.cpp +++ b/plugins/Kaleidoscope-Escape-OneShot/src/kaleidoscope/plugin/Escape-OneShot-Config.cpp @@ -58,10 +58,10 @@ EventHandlerResult EscapeOneShotConfig::onFocusEvent(const char *input) { Key k; ::Focus.read(k); ::EscapeOneShot.setCancelKey(k); + Runtime.storage().put(settings_base_, ::EscapeOneShot.settings_); + Runtime.storage().commit(); } - Runtime.storage().put(settings_base_, ::EscapeOneShot.settings_); - Runtime.storage().commit(); return EventHandlerResult::EVENT_CONSUMED; } From 6bd594b907a2f20e3b4a0504c157bb19e9644182 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Mon, 4 Mar 2024 21:01:55 -0800 Subject: [PATCH 5/6] It was possible to overflow the dynamic macro list if the dynamicmacros data structure was zeroed out. --- .../src/kaleidoscope/plugin/DynamicMacros.cpp | 2 +- .../src/kaleidoscope/plugin/DynamicMacros.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp index c4277ef7b8..31f27ea0a2 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp +++ b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp @@ -41,7 +41,7 @@ uint8_t DynamicMacros::updateDynamicMacroCache() { map_[0] = 0; - while (pos < storage_base_ + storage_size_) { + while (pos < storage_base_ + storage_size_ && current_id < MAX_MACRO_COUNT_) { macro = Runtime.storage().read(pos++); switch (macro) { case MACRO_ACTION_STEP_EXPLICIT_REPORT: diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h index 3cac7b453f..2bf8917168 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h +++ b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.h @@ -48,9 +48,10 @@ class DynamicMacros : public kaleidoscope::Plugin { void play(uint8_t seq_id); private: + static const uint8_t MAX_MACRO_COUNT_ = 32; uint16_t storage_base_; uint16_t storage_size_; - uint16_t map_[32]; + uint16_t map_[MAX_MACRO_COUNT_]; uint8_t macro_count_; uint8_t updateDynamicMacroCache(); From 56dbb2dfb65431b9b964499ed3826a7a7e49a842 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Mon, 4 Mar 2024 21:02:41 -0800 Subject: [PATCH 6/6] A couple more cases where Dynamic Macros could get itself stuck in a nearly-infinite loop with bad data. --- .../src/kaleidoscope/plugin/DynamicMacros.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp index 31f27ea0a2..32c2034a8d 100644 --- a/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp +++ b/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp @@ -76,7 +76,7 @@ uint8_t DynamicMacros::updateDynamicMacroCache() { uint8_t keyCode, flags; do { keyCode = Runtime.storage().read(pos++); - } while (keyCode != 0); + } while ((pos < (storage_base_ + storage_size_)) && keyCode != 0); break; } @@ -170,7 +170,7 @@ void DynamicMacros::play(uint8_t macro_id) { while (true) { key.setFlags(isKeycodeSequence ? 0 : storage.read(pos++)); key.setKeyCode(storage.read(pos++)); - if (key == Key_NoKey) + if (key == Key_NoKey || pos >= storage_base_ + storage_size_) break; tap(key); delay(interval);