From 8a4f3a7fb9ecbc327174efcc346a2f1ccced7c2d Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 3 Feb 2019 09:23:22 +0100 Subject: [PATCH] LED-Palette-Theme: Defer requesting a slice as late as possible Previously we requested a slice in `onSetup()`, but this had a major, problematic implication: if we went from a sketch with `EEPROM-Keymap`, but withot `LED-Palette-Theme` enabled to one with it, the EEPROM layout was not compatible, no matter what the order of plugins in `KALEIDOSCOPE_INIT_PLUGINS` were. This happened because `EEPROM-Keymap` requested space *after* `onSetup()` already ran. To fix this issue, request a slice in `.reserveThemes`, only if we didn't request a slice yet. This way the EEPROM layout is decided by the order of initialization in `setup()`, and we do not sneakily steal a slice in `onSetup()`. This is required to address keyboardio/Chrysalis#270. Signed-off-by: Gergely Nagy --- NEWS.md | 2 ++ src/kaleidoscope/plugin/LED-Palette-Theme.cpp | 6 +----- src/kaleidoscope/plugin/LED-Palette-Theme.h | 1 - 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/NEWS.md b/NEWS.md index 39888526..0dfc5b9d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -144,6 +144,8 @@ The [Redial](doc/plugin/Redial.md) plugin was simplified, one no longer needs to The [LED-Palette-Theme](doc/plugin/LED-Palette-Theme.md) had to be changed to store the palette colors in reverse. This change had to be made in order to not default to a bright white palette, that would draw so much power that most operating systems would disconnect the keyboard due to excessive power usage. With inverting the colors, we now default to a black palette instead. This sadly breaks existing palettes, and you will have to re-set the colors. +We also changed when we reserve space for the palette in EEPROM: we used to do it as soon as possible, but that made it impossible to go from a firmware that does not use the plugin to one that does, and still have a compatible EEPROM layout. We now reserve space as late as possible. This breaks existing EEPROM layouts however. + ### EEPROM-Keymap changed Focus commands 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. diff --git a/src/kaleidoscope/plugin/LED-Palette-Theme.cpp b/src/kaleidoscope/plugin/LED-Palette-Theme.cpp index 38220f8c..5af59f34 100644 --- a/src/kaleidoscope/plugin/LED-Palette-Theme.cpp +++ b/src/kaleidoscope/plugin/LED-Palette-Theme.cpp @@ -25,14 +25,10 @@ namespace plugin { uint16_t LEDPaletteTheme::palette_base_; -EventHandlerResult LEDPaletteTheme::onSetup(void) { +uint16_t LEDPaletteTheme::reserveThemes(uint8_t max_themes) { if (!palette_base_) palette_base_ = ::EEPROMSettings.requestSlice(16 * sizeof(cRGB)); - return EventHandlerResult::OK; -} - -uint16_t LEDPaletteTheme::reserveThemes(uint8_t max_themes) { return ::EEPROMSettings.requestSlice(max_themes * ROWS * COLS / 2); } diff --git a/src/kaleidoscope/plugin/LED-Palette-Theme.h b/src/kaleidoscope/plugin/LED-Palette-Theme.h index 0f097b8a..fe8d382f 100644 --- a/src/kaleidoscope/plugin/LED-Palette-Theme.h +++ b/src/kaleidoscope/plugin/LED-Palette-Theme.h @@ -41,7 +41,6 @@ class LEDPaletteTheme : public kaleidoscope::Plugin { EventHandlerResult themeFocusEvent(const char *command, const char *expected_command, uint16_t theme_base, uint8_t max_themes); - EventHandlerResult onSetup(); private: static uint16_t palette_base_;