diff --git a/NEWS.md b/NEWS.md index 0dfc5b9d..5babe344 100644 --- a/NEWS.md +++ b/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. diff --git a/doc/plugin/EEPROM-Settings.md b/doc/plugin/EEPROM-Settings.md index ee108c95..840c0439 100644 --- a/doc/plugin/EEPROM-Settings.md +++ b/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` diff --git a/src/kaleidoscope/plugin/EEPROM-Settings.cpp b/src/kaleidoscope/plugin/EEPROM-Settings.cpp index 888170f2..6f5fcfb6 100644 --- a/src/kaleidoscope/plugin/EEPROM-Settings.cpp +++ b/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 { diff --git a/src/kaleidoscope/plugin/EEPROM-Settings.h b/src/kaleidoscope/plugin/EEPROM-Settings.h index 73618289..eb2e427e 100644 --- a/src/kaleidoscope/plugin/EEPROM-Settings.h +++ b/src/kaleidoscope/plugin/EEPROM-Settings.h @@ -20,6 +20,10 @@ #include #include +#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);