Merge pull request #1404 from keyboardio/bugfix/eeprom-settings-loadi…

…ng-regression

It turns out that our templated storage().get() method silently fails

plugins/Kaleidoscope-AutoShift/src/kaleidoscope/plugin/AutoShiftConfig.cpp

@@ -33,7 +33,7 @@ namespace plugin {
 // AutoShift configurator
 
 EventHandlerResult AutoShiftConfig::onSetup() {
-  ::EEPROMSettings.requestSliceAndLoadData(sizeof(::AutoShift.settings_), &settings_base_, &::AutoShift.settings_);
+  ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &::AutoShift.settings_);
   return EventHandlerResult::OK;
 }
 

plugins/Kaleidoscope-DefaultLEDModeConfig/src/kaleidoscope/plugin/DefaultLEDModeConfig.cpp

@@ -34,7 +34,7 @@ uint16_t DefaultLEDModeConfig::settings_base_;
 struct DefaultLEDModeConfig::settings DefaultLEDModeConfig::settings_;
 
 EventHandlerResult DefaultLEDModeConfig::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(settings_), &settings_base_, &settings_);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &settings_);
   if (success) {
     ::LEDControl.set_mode(settings_.default_mode_index);
   }

plugins/Kaleidoscope-EEPROM-Settings/src/kaleidoscope/plugin/EEPROM-Settings.cpp

@@ -160,24 +160,6 @@ void EEPROMSettings::update() {
   is_valid_ = true;
 }
 
-// get a settings slice from the storage and stick it in the untyped struct
-// startAddress is the address of the start of the slice
-// Returns true if the load was successful and false otherwise.
-// Takes the size of storage we want, a pointer to the start address, and a pointer to the data structure for settings
-
-
-bool EEPROMSettings::requestSliceAndLoadData(size_t size, uint16_t *startAddress, void *data) {
-  // Request the slice for the struct from storage
-  uint16_t start = requestSlice(size);
-  *startAddress  = start;
-
-  if (!Runtime.storage().isSliceUninitialized(start, size)) {
-    Runtime.storage().get(start, data);
-    return true;  // Data was loaded successfully
-  } else {
-  }
-  return false;  // Data was not loaded
-}
 
 bool EEPROMSettings::isSliceValid(uint16_t start, size_t size) {
 

plugins/Kaleidoscope-EEPROM-Settings/src/kaleidoscope/plugin/EEPROM-Settings.h

@@ -21,6 +21,7 @@
 #include <stddef.h>                             // for size_t
 #include "kaleidoscope/event_handler_result.h"  // for EventHandlerResult
 #include "kaleidoscope/plugin.h"                // for Plugin
+#include "kaleidoscope/Runtime.h"               // for Runtime
 
 namespace kaleidoscope {
 namespace plugin {
@@ -73,7 +74,27 @@ class EEPROMSettings : public kaleidoscope::Plugin {
   bool ignoreHardcodedLayers() {
     return settings_.ignore_hardcoded_layers;
   }
-  bool requestSliceAndLoadData(size_t size, uint16_t *startAddress, void *data);
+  // get a settings slice from the storage and stick it in the settings struct
+  // Takes a pointer to the start address, and a pointer to the data structure for settings
+  // startAddress is the address of the start of the slice, to be returned to the caller
+  // Returns true if the slice is initialized and false otherwise.
+
+
+  template<typename T>
+  bool requestSliceAndLoadData(uint16_t *startAddress, T *data) {
+    // Request the slice for the struct from storage
+    size_t size    = sizeof(T);
+    uint16_t start = requestSlice(size);
+    *startAddress  = start;
+
+    // Load the data if the slice is initialized
+    Runtime.storage().get(start, *data);  // Directly load data into the provided address
+    if (!Runtime.storage().isSliceUninitialized(start, size)) {
+      return true;
+    }
+
+    return false;
+  }
 
   bool isSliceValid(uint16_t start, size_t size);
 

plugins/Kaleidoscope-Escape-OneShot/src/kaleidoscope/plugin/Escape-OneShot-Config.cpp

@@ -30,12 +30,12 @@ namespace kaleidoscope {
 namespace plugin {
 
 EventHandlerResult EscapeOneShotConfig::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(::EscapeOneShot.settings_), &settings_base_, &::EscapeOneShot.settings_);
-  if (!success) {
-    // If our slice is uninitialized, set sensible defaults.
-    Runtime.storage().put(settings_base_, ::EscapeOneShot.settings_);
-    Runtime.storage().commit();
-  }
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &::EscapeOneShot.settings_);
+  // if (!success) {
+  // If our slice is uninitialized, set sensible defaults.
+  // Runtime.storage().put(settings_base_, ::EscapeOneShot.settings_);
+  //Runtime.storage().commit();
+  //}
 
   return EventHandlerResult::OK;
 }

plugins/Kaleidoscope-IdleLEDs/src/kaleidoscope/plugin/IdleLEDs.cpp

@@ -77,7 +77,7 @@ EventHandlerResult PersistentIdleLEDs::onNameQuery() {
 EventHandlerResult PersistentIdleLEDs::onSetup() {
   uint16_t idle_time;
 
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(idle_time), &settings_base_, &idle_time);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &idle_time);
 
   // If idleTime is max, assume that EEPROM is uninitialized, and store the
   // defaults.

plugins/Kaleidoscope-LEDBrightnessConfig/src/kaleidoscope/plugin/LEDBrightnessConfig.cpp

@@ -34,7 +34,7 @@ uint16_t LEDBrightnessConfig::settings_base_;
 struct LEDBrightnessConfig::settings LEDBrightnessConfig::settings_;
 
 EventHandlerResult LEDBrightnessConfig::onSetup() {
-  if (!::EEPROMSettings.requestSliceAndLoadData(sizeof(settings_), &settings_base_, &settings_)) {
+  if (!::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &settings_)) {
     settings_.brightness = 255;
   }
   ::LEDControl.setBrightness(settings_.brightness);

plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeysConfig.cpp

@@ -33,7 +33,7 @@ namespace plugin {
 // MouseKeys configurator
 
 EventHandlerResult MouseKeysConfig::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(::MouseKeys.settings_), &settings_base_, &::MouseKeys.settings_);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &::MouseKeys.settings_);
 
   if (!success) {
     Runtime.storage().put(settings_base_, ::MouseKeys.settings_);

plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShotConfig.cpp

@@ -30,7 +30,7 @@ namespace kaleidoscope {
 namespace plugin {
 
 EventHandlerResult OneShotConfig::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(::OneShot.settings_), &settings_base_, &::OneShot.settings_);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &::OneShot.settings_);
 
   if (!success) {
     Runtime.storage().put(settings_base_, ::OneShot.settings_);

plugins/Kaleidoscope-PersistentLEDMode/src/kaleidoscope/plugin/PersistentLEDMode.cpp

@@ -35,7 +35,7 @@ uint16_t PersistentLEDMode::settings_base_;
 struct PersistentLEDMode::settings PersistentLEDMode::settings_;
 
 EventHandlerResult PersistentLEDMode::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(settings_), &settings_base_, &settings_);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &settings_);
 
   if (success) {
     ::LEDControl.set_mode(settings_.default_mode_index);

plugins/Kaleidoscope-SpaceCadet/src/kaleidoscope/plugin/SpaceCadetConfig.cpp

@@ -31,7 +31,7 @@ namespace kaleidoscope {
 namespace plugin {
 
 EventHandlerResult SpaceCadetConfig::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(::SpaceCadet.settings_), &settings_base_, &::SpaceCadet.settings_);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &::SpaceCadet.settings_);
   if (!success || (::SpaceCadet.settings_.mode != SpaceCadet::Mode::ON && ::SpaceCadet.settings_.mode != SpaceCadet::Mode::NO_DELAY)) {
     ::SpaceCadet.disable();
   }

plugins/Kaleidoscope-TypingBreaks/src/kaleidoscope/plugin/TypingBreaks.cpp

@@ -118,7 +118,7 @@ EventHandlerResult TypingBreaks::onNameQuery() {
 }
 
 EventHandlerResult TypingBreaks::onSetup() {
-  bool success = ::EEPROMSettings.requestSliceAndLoadData(sizeof(settings), &settings_base_, &settings);
+  bool success = ::EEPROMSettings.requestSliceAndLoadData(&settings_base_, &settings);
 
   if (!success) {
     // If our slice is uninitialized, set sensible defaults.

testing/device-testing/README

@@ -0,0 +1,7 @@
+Things inside this directory are tools and scripts intended for interactive testing with a specific device attached. At this time, they're not intended for automated testing under CI.
+
+Things that might be here include tests of the focus protocol or tests of on-device EEPROM/Storage.
+
+These tests may require locally installed tools.
+
+(There are dragons in this directory and we deemed it better to keep this stuff around in the hopes of eventually getting it automated than throwing it away.)

testing/device-testing/eeprom-persistence/README

@@ -0,0 +1 @@
+This test script does basic tests to ensure eeprom persistence across a reboot

testing/device-testing/eeprom-persistence/test.py

@@ -0,0 +1,55 @@
+import subprocess
+import unittest
+import time
+
+def run_focus_command(command, hide_stderr=False):
+    """Execute a focus send command, print the command and its output, and return its output."""
+    stderr_setting = subprocess.DEVNULL if hide_stderr else subprocess.PIPE
+    print(f"Running command: {command}")
+    completed_process = subprocess.run(f"focus send {command}", shell=True, stdout=subprocess.PIPE, stderr=stderr_setting, text=True)
+    print(f"Result: {completed_process.stdout.strip()}")
+    if completed_process.returncode != 0 and not hide_stderr:
+        print(f"Error: {completed_process.stderr.strip()}" if completed_process.stderr else "Command failed with no error output.")
+    return completed_process.stdout.strip(), completed_process.returncode
+
+
+class TestFocusCommands(unittest.TestCase):
+    def test_focus_commands(self):
+        # Test 'focus send version', log the result, and store as initial version
+        initial_version, _ = run_focus_command("version")
+
+        # Test 'focus send escape_oneshot.cancel_key' and log the result
+        cancel_key_result_initial, _ = run_focus_command("escape_oneshot.cancel_key")
+
+        run_focus_command("escape_oneshot.cancel_key 45")
+
+        new_version, _ = run_focus_command("escape_oneshot.cancel_key")
+        self.assertEqual(new_version, '45', "New version should be '45'")
+
+        # Expect 'focus send device.reset' to error
+        reset_result, returncode = run_focus_command("device.reset", hide_stderr=True)
+        self.assertNotEqual(returncode, 0, "Expected non-zero exit code for device.reset")
+        time.sleep(2)  # Add a 5-second delay here
+
+        new_version, _ = run_focus_command("escape_oneshot.cancel_key")
+        self.assertEqual(new_version, '45', "New version should be '45'")
+
+
+        # Set new value for escape_oneshot.cancel_key and verify no action needed
+        run_focus_command("escape_oneshot.cancel_key 30")
+
+        # Fetch new version and ensure it's '30'
+        new_version, _ = run_focus_command("escape_oneshot.cancel_key")
+        self.assertEqual(new_version, '30', "New version should be '30'")
+
+        # Expect 'focus send device.reset' to error
+        reset_result, returncode = run_focus_command("device.reset", hide_stderr=True)
+        self.assertNotEqual(returncode, 0, "Expected non-zero exit code for device.reset")
+        time.sleep(2)  # Add a 5-second delay here
+
+        # Test 'focus send escape_oneshot.cancel_key' again and ensure it matches new version
+        final_version, _ = run_focus_command("escape_oneshot.cancel_key")
+        self.assertEqual(final_version, new_version, "Final version should match new version after reset")
+
+if __name__ == '__main__':
+    unittest.main()

testing/device-testing/spacecadet-off/space_cadet_off_after_reboot_and_reset.py

@@ -0,0 +1,68 @@
+import subprocess
+import unittest
+import time
+def run_focus_command(command, hide_stderr=False):
+    """Execute a focus send command, print the command and its output, and return its output."""
+    stderr_setting = subprocess.DEVNULL if hide_stderr else subprocess.PIPE
+    print(f"Running command: {command}")
+    completed_process = subprocess.run(f"focus send {command}", shell=True, stdout=subprocess.PIPE, stderr=stderr_setting, text=True)
+    print(f"Result: {completed_process.stdout.strip()}")
+    if completed_process.returncode != 0 and not hide_stderr:
+        print(f"Error: {completed_process.stderr.strip()}" if completed_process.stderr else "Command failed with no error output.")
+    return completed_process.stdout.strip(), completed_process.returncode
+
+class TestFocusCommands(unittest.TestCase):
+    def test_spacecadet_mode_persistence(self):
+        _, _ = run_focus_command("version")
+
+        # Initially try to erase eeprom and expect an error
+        _, returncode = run_focus_command("eeprom.erase", hide_stderr=True)
+        self.assertNotEqual(returncode, 0, "Eeprom erase should fail but did not.")
+
+        #erasing eeprom takes a moment
+        time.sleep(5)
+
+        # Verify initial spacecadet mode is '1'
+        initial_mode, _ = run_focus_command("spacecadet.mode")
+        self.assertEqual(initial_mode, '1', "Initial spacecadet.mode should be '1'")
+
+        # Change spacecadet mode to 0 and verify
+        run_focus_command("spacecadet.mode 0")
+        mode_after_setting_to_0, _ = run_focus_command("spacecadet.mode")
+        self.assertEqual(mode_after_setting_to_0, '0', "spacecadet.mode should be '0' after setting to 0")
+
+        # Reset device and expect an error
+        _, returncode = run_focus_command("device.reset", hide_stderr=True)
+        self.assertNotEqual(returncode, 0, "Device reset should fail but did not.")
+        time.sleep(5)  # Wait for the device to potentially reset
+
+        # Verify spacecadet mode is still 0 after reset
+        mode_after_reset, _ = run_focus_command("spacecadet.mode")
+        self.assertEqual(mode_after_reset, '0', "spacecadet.mode should remain '0' after reset")
+
+        # Change spacecadet mode to 1 and verify
+        run_focus_command("spacecadet.mode 1")
+        mode_after_setting_to_1, _ = run_focus_command("spacecadet.mode")
+        self.assertEqual(mode_after_setting_to_1, '1', "spacecadet.mode should be '1' after setting to 1")
+
+        # Reset device again and expect an error
+        _, returncode = run_focus_command("device.reset", hide_stderr=True)
+        self.assertNotEqual(returncode, 0, "Device reset should fail but did not.")
+        time.sleep(5)  # Wait for the device to potentially reset
+
+        # Verify spacecadet mode is still 1 after reset
+        mode_final, _ = run_focus_command("spacecadet.mode")
+        self.assertEqual(mode_final, '1', "spacecadet.mode should remain '1' after final reset")
+
+        # Try to erase eeprom again and expect an error
+        _, returncode = run_focus_command("eeprom.erase", hide_stderr=True)
+        self.assertNotEqual(returncode, 0, "Eeprom erase should fail but did not.")
+
+        time.sleep(5)  # Wait for the device to potentially reset
+
+        # Verify spacecadet mode is still 1 after attempting to erase eeprom
+        mode_after_erase, _ = run_focus_command("spacecadet.mode")
+        self.assertEqual(mode_after_erase, '1', "spacecadet.mode should remain '1' after attempting to erase eeprom")
+
+if __name__ == '__main__':
+    unittest.main()