Repurpose EEPROMSettings' version

We want to be able to notice when the layout of the EEPROM *settings*
changed (which the CRC does not cover). For this reason, we're repurposing the
existing version setting, which wasn't widely used: it is now internal.

We use the version to determine whether the EEPROM has been written to yet, or
if it is uninitialized. This helps us make sure we're starting up with sensible
defaults.

Fixes #559, and fixes #558.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

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
+> Returns the current version of the EEPROM settings. It's the version of the
-> the firmware, so it can attempt to upgrade the contents, if need be, or alert
+> settings only, not that of the whole layout - the CRC covers that.
-> the user in there's a mismatch. Plugins do not use this property.
 >
-> 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 uint8_t version(void) {
-  static void version(uint8_t ver);
+    return settings_.version;
+  }
+  static void version(uint8_t) DEPRECATED(EEPROMSETTINGS_VERSION_SET) {}
 
   static uint16_t requestSlice(uint16_t size);
   static void seal(void);