Merge pull request #561 from keyboardio/EEPROMSettings/onlyCustom-fix

Repurpose EEPROMSettings' version

NEWS.md

@@ -150,6 +150,15 @@ We also changed when we reserve space for the palette in EEPROM: we used to do i
 
 The [EEPROMKeymap](doc/plugin/EEPROM-Keymap.md) plugin was changed to treat built-in (default) and EEPROM-stored (custom) layers separately, because that's less surprising, and easier to work with from Chrysalis. The old `keymap.map` and `keymap.roLayers` commands are gone, the new `keymap.default` and `keymap.custom` commands should be used instead.
 
+### EEPROMSettings' version() setter has been deprecated
+
+We're repurposing the `version` setting: instead of it being something end-users
+can set, we'll be using it internally to track changes made to
+[EEPROMSettings](doc/plugin/EEPROM-Settings.md) itself, with the goal of
+allowing external tools to aid in migrations. The setting wasn't widely used -
+if at all -, which is why we chose to repurpose it instead of adding a new
+field.
+
 ## Bugfixes
 
 We fixed way too many issues to list here, so we're going to narrow it down to the most important, most visible ones.

doc/plugin/EEPROM-Settings.md

@@ -111,13 +111,12 @@ The plugin provides the `EEPROMSettings` object, which has the following methods
 > firmware would expect. This signals to other plugins that the contents of
 > `EEPROM` should not be trusted.
 
-### `version([newVersion])`
+### `version()`
 
-> Sets or returns the version of the `EEPROM` layout. This is purely for use by
-> the firmware, so it can attempt to upgrade the contents, if need be, or alert
-> the user in there's a mismatch. Plugins do not use this property.
+> Returns the current version of the EEPROM settings. It's the version of the
+> settings only, not that of the whole layout - the CRC covers that.
 >
-> Should only be called after calling `seal()`.
+> This is for internal use only, end-users should not need to care about it.
 
 ### `crc()`
 
@@ -159,7 +158,7 @@ following commands:
 
 ### `settings.version`
 
-> Returns the (user-set) version of the settings.
+> Returns the version of the settings.
 
 ### `eeprom.contents`
 

src/kaleidoscope/plugin/EEPROM-Settings.cpp

@@ -29,6 +29,28 @@ uint16_t EEPROMSettings::next_start_ = sizeof(EEPROMSettings::settings);
 
 EventHandlerResult EEPROMSettings::onSetup() {
   EEPROM.get(0, settings_);
+
+  /* If the version is undefined, set up sensible defaults. */
+  if (settings_.version == VERSION_UNDEFINED) {
+    if (settings_.default_layer == 127 &&
+        settings_.ignore_hardcoded_layers) {
+      /* If both of these are set, that means that the EEPROM is uninitialized,
+         and setting sensible defaults is safe. If either of them is not at it's
+         uninitialized state, we do not override them, to avoid overwriting user
+         settings. */
+      settings_.ignore_hardcoded_layers = false;
+      settings_.default_layer = 0;
+    }
+
+    /* If the version is undefined, we'll set it to our current one. */
+    settings_.version = VERSION_CURRENT;
+
+    /* Ideally, we'd save the defaults set here on the first write, but we are
+     * not able to catch all writes yet. For the sake of consistency, if we
+     * encounter a firmware with no version defined, we'll set sensible
+     * defaults. */
+    EEPROM.put(0, settings_);
+  }
   return EventHandlerResult::OK;
 }
 
@@ -80,6 +102,18 @@ void EEPROMSettings::seal(void) {
 
   CRC.finalize();
 
+  if (settings_.version != VERSION_CURRENT) {
+    is_valid_ = false;
+    return;
+  }
+
+  if (settings_.crc == 0xffff) {
+    settings_.crc = CRC.crc;
+    update();
+  } else if (settings_.crc != CRC.crc) {
+    return;
+  }
+
   /* If we have a default layer set, switch to it.
    *
    * We use IGNORE_HARDCODED_LAYER_MASK, because we want to avoid setting a
@@ -93,16 +127,6 @@ void EEPROMSettings::seal(void) {
    */
   if (!(settings_.default_layer & IGNORE_HARDCODED_LAYER_MASK))
     Layer.move(settings_.default_layer);
-
-  /* Until we set a version, consider the EEPROM contents flexible, and always
-   * update the CRC. This will always result in the settings being considered
-   * valid. */
-  if (settings_.version == 0xff) {
-    return update();
-  }
-
-  if (settings_.crc != CRC.crc)
-    is_valid_ = false;
 }
 
 uint16_t EEPROMSettings::requestSlice(uint16_t size) {
@@ -126,21 +150,10 @@ uint16_t EEPROMSettings::used(void) {
 }
 
 void EEPROMSettings::update(void) {
-  settings_.crc = CRC.crc;
-
   EEPROM.put(0, settings_);
   is_valid_ = true;
 }
 
-uint8_t EEPROMSettings::version(void) {
-  return settings_.version;
-}
-
-void EEPROMSettings::version(uint8_t ver) {
-  settings_.version = ver;
-  update();
-}
-
 /** Focus **/
 EventHandlerResult FocusSettingsCommand::onFocusEvent(const char *command) {
   enum {

src/kaleidoscope/plugin/EEPROM-Settings.h

@@ -20,6 +20,10 @@
 #include <Kaleidoscope.h>
 #include <EEPROM.h>
 
+#define _DEPRECATED_MESSAGE_EEPROMSETTINGS_VERSION_SET            \
+  "The EEPROMSettings.version(uint8_t version) method has been deprecated,\n" \
+  "and is a no-op now. Please see the NEWS file for more information."
+
 namespace kaleidoscope {
 namespace plugin {
 class EEPROMSettings : public kaleidoscope::Plugin {
@@ -29,11 +33,25 @@ class EEPROMSettings : public kaleidoscope::Plugin {
   EventHandlerResult onSetup();
   EventHandlerResult beforeEachCycle();
 
+  /* EEPROM is filled with 0xff when uninitialized, so a version with that value
+   * means we do not have an EEPROM version defined yet. */
+  static constexpr uint8_t VERSION_UNDEFINED = 0xff;
+  /* A version set to zero is likely some kind of corruption, we do not normally
+   * clear the byte. */
+  static constexpr uint8_t VERSION_IMPOSSIBLE_ZERO = 0x00;
+  /* Our current version. Whenever we change the layout of the settings, this
+   * needs to be increased too. If the version stored in EEPROM does not match
+   * this version, EEPROM use should be considered unsafe, and plugins should
+   * fall back to not using it. */
+  static constexpr uint8_t VERSION_CURRENT = 0x01;
+
   static void update(void);
   static bool isValid(void);
   static void invalidate(void);
-  static uint8_t version(void);
-  static void version(uint8_t ver);
+  static uint8_t version(void) {
+    return settings_.version;
+  }
+  static void version(uint8_t) DEPRECATED(EEPROMSETTINGS_VERSION_SET) {}
 
   static uint16_t requestSlice(uint16_t size);
   static void seal(void);