Major update of how LED modes work

There were a number of issues with the model we had before, namely that plugins
that changed LED colors outside of LED modes had no way to signal "go back to
whatever color this key was". To this end, the `LEDMode.refreshAt` method is
introduced, which these plugins can call to tell the mode to update a given key.

As part of this, the API was redesigned, with code that is common between all
LED modes moving to the base class, among other things, much better names, and a
flow of control that is easier to follow.

In the new setup, there are four methods a LED mode can implement:

- `setup()` to do boot-time initialization (registering hooks, etc).
- `onActivate()` called every time the mode is activated.
- `update()` called each cycle.
- `refreshAt()` may be called by other plugins to refresh a particular key.

All of these are protected methods, to be called via `LEDControl` only.

Much of the new API design was done by @cdisselkoen, huge thanks for his work!

Fixes #9.

Signed-off-by: Gergely Nagy <kaleidoscope@gergo.csillger.hu>

src/Kaleidoscope-LEDControl.cpp

@@ -1,83 +1,77 @@
 #include "Kaleidoscope-LEDControl.h"
 #include "Kaleidoscope-Focus.h"
 
-LEDMode *LEDControl_::modes[LED_MAX_MODES];
+namespace kaleidoscope {
-uint8_t LEDControl_::previousMode, LEDControl_::mode;
-uint16_t LEDControl_::syncDelay = 16;
-uint32_t LEDControl_::syncTimer;
 
-void
+LEDMode *LEDControl::modes[LED_MAX_MODES];
-LEDMode::activate(void) {
+uint8_t LEDControl::mode;
-  LEDControl.activate(this);
+uint16_t LEDControl::syncDelay = 16;
-}
+uint32_t LEDControl::syncTimer;
 
-void
+void LEDMode::activate(void) {
-LEDMode::begin(void) {
+  ::LEDControl.activate(this);
-  Kaleidoscope.use(&LEDControl, NULL);
-  LEDControl.mode_add(this);
 }
 
-LEDControl_::LEDControl_(void) {
+void LEDMode::begin(void) {
-  mode = previousMode = 0;
+  Kaleidoscope.use(&::LEDControl);
-  memset(modes, 0, LED_MAX_MODES * sizeof(modes[0]));
+  ::LEDControl.mode_add(this);
+  setup();
 }
 
-void
+LEDControl::LEDControl(void) {
-LEDControl_::next_mode(void) {
-  mode++;
-
-  if (mode >= LED_MAX_MODES) {
   mode = 0;
-    return;
+  memset(modes, 0, LED_MAX_MODES * sizeof(modes[0]));
 }
 
-  if (modes[mode])
+void LEDControl::next_mode(void) {
-    return;
+  mode++;
 
-  mode = 0;
+  if (mode >= LED_MAX_MODES || !modes[mode]) {
+    return set_mode(0);
   }
 
-void
+  return set_mode(mode);
-LEDControl_::update(void) {
-  if (previousMode != mode) {
-    set_all_leds_to({0, 0, 0});
-    if (modes[mode])
-      (modes[mode]->init)();
 }
 
+#if 0
+void LEDControl::update(void) {
   if (modes[mode])
     (modes[mode]->update)();
-
-  previousMode = mode;
 }
 
-void
+void LEDControl::refreshAt(byte row, byte col) {
-LEDControl_::init_mode(void) {
   if (modes[mode])
-    (modes[mode]->init)();
+    modes[mode]->refreshAt(row, col);
 }
+#endif
 
 void
-LEDControl_::set_mode(uint8_t mode_) {
+LEDControl::set_mode(uint8_t mode_) {
-  if (mode_ < LED_MAX_MODES)
+  if (mode_ >= LED_MAX_MODES)
+    return;
+
+  set_all_leds_to({0, 0, 0});
   mode = mode_;
+  if (modes[mode])
+    modes[mode]->onActivate();
 }
 
-uint8_t
+uint8_t LEDControl::get_mode_index(void) {
-LEDControl_::get_mode(void) {
   return mode;
 }
 
-void
+LEDMode *LEDControl::get_mode(void) {
-LEDControl_::activate(LEDMode *mode) {
+  return modes[mode];
+}
+
+void LEDControl::activate(LEDMode *mode) {
   for (uint8_t i = 0; i < LED_MAX_MODES; i++) {
     if (modes[i] == mode)
       return set_mode(i);
   }
 }
 
-int8_t
+int8_t LEDControl::mode_add(LEDMode *mode) {
-LEDControl_::mode_add(LEDMode *mode) {
   for (int i = 0; i < LED_MAX_MODES; i++) {
     if (modes[i])
       continue;
@@ -88,8 +82,7 @@ LEDControl_::mode_add(LEDMode *mode) {
   return -1;
 }
 
-void
+void LEDControl::set_all_leds_to(uint8_t r, uint8_t g, uint8_t b) {
-LEDControl_::set_all_leds_to(uint8_t r, uint8_t g, uint8_t b) {
   cRGB color;
   color.r = r;
   color.g = g;
@@ -97,35 +90,29 @@ LEDControl_::set_all_leds_to(uint8_t r, uint8_t g, uint8_t b) {
   set_all_leds_to(color);
 }
 
-void
+void LEDControl::set_all_leds_to(cRGB color) {
-LEDControl_::set_all_leds_to(cRGB color) {
   for (uint8_t i = 0; i < LED_COUNT; i++) {
     setCrgbAt(i, color);
   }
 }
 
-void
+void LEDControl::setCrgbAt(uint8_t i, cRGB crgb) {
-LEDControl_::setCrgbAt(uint8_t i, cRGB crgb) {
   KeyboardHardware.setCrgbAt(i, crgb);
 }
 
-void
+void LEDControl::setCrgbAt(byte row, byte col, cRGB color) {
-LEDControl_::setCrgbAt(byte row, byte col, cRGB color) {
   KeyboardHardware.setCrgbAt(row, col, color);
 }
 
-cRGB
+cRGB LEDControl::getCrgbAt(uint8_t i) {
-LEDControl_::getCrgbAt(uint8_t i) {
   return KeyboardHardware.getCrgbAt(i);
 }
 
-void
+void LEDControl::syncLeds(void) {
-LEDControl_::syncLeds(void) {
   KeyboardHardware.syncLeds();
 }
 
-void
+void LEDControl::begin(void) {
-LEDControl_::begin(void) {
   set_all_leds_to({0, 0, 0});
 
   for (uint8_t i = 0; i < LED_MAX_MODES; i++) {
@@ -133,25 +120,23 @@ LEDControl_::begin(void) {
       (modes[i]->setup)();
   }
 
-  event_handler_hook_use(eventHandler);
+  Kaleidoscope.useEventHandlerHook(eventHandler);
-  loop_hook_use(loopHook);
+  Kaleidoscope.useLoopHook(loopHook);
 
   syncTimer = millis() + syncDelay;
 }
 
-Key
+Key LEDControl::eventHandler(Key mappedKey, byte row, byte col, uint8_t keyState) {
-LEDControl_::eventHandler(Key mappedKey, byte row, byte col, uint8_t keyState) {
   if (mappedKey.flags != (SYNTHETIC | IS_INTERNAL | LED_TOGGLE))
     return mappedKey;
 
   if (keyToggledOn(keyState))
-    LEDControl.next_mode();
+    next_mode();
 
   return Key_NoKey;
 }
 
-void
+void LEDControl::loopHook(bool postClear) {
-LEDControl_::loopHook(bool postClear) {
   if (postClear)
     return;
 
@@ -162,8 +147,7 @@ LEDControl_::loopHook(bool postClear) {
   update();
 }
 
-bool
+bool LEDControl::focusHook(const char *command) {
-LEDControl_::focusHook(const char *command) {
   enum {
     SETALL,
     MODE,
@@ -189,9 +173,9 @@ LEDControl_::focusHook(const char *command) {
     uint8_t idx = Serial.parseInt();
 
     if (Serial.peek() == '\n') {
-      cRGB c = LEDControl.getCrgbAt(idx);
+      cRGB c = getCrgbAt(idx);
 
-      Focus.printColor(c.r, c.g, c.b);
+      ::Focus.printColor(c.r, c.g, c.b);
       Serial.println();
     } else {
       cRGB c;
@@ -200,7 +184,7 @@ LEDControl_::focusHook(const char *command) {
       c.g = Serial.parseInt();
       c.b = Serial.parseInt();
 
-      LEDControl.setCrgbAt(idx, c);
+      setCrgbAt(idx, c);
     }
     break;
   }
@@ -211,16 +195,16 @@ LEDControl_::focusHook(const char *command) {
     c.g = Serial.parseInt();
     c.b = Serial.parseInt();
 
-    LEDControl.set_all_leds_to(c);
+    set_all_leds_to(c);
 
     break;
   }
   case MODE: {
     char peek = Serial.peek();
     if (peek == '\n') {
-      Serial.println(LEDControl.get_mode());
+      Serial.println(get_mode_index());
     } else if (peek == 'n') {
-      LEDControl.next_mode();
+      next_mode();
       Serial.read();
     } else if (peek == 'p') {
       // TODO(algernon)
@@ -228,17 +212,17 @@ LEDControl_::focusHook(const char *command) {
     } else {
       uint8_t mode = Serial.parseInt();
 
-      LEDControl.set_mode(mode);
+      set_mode(mode);
     }
     break;
   }
   case THEME: {
     if (Serial.peek() == '\n') {
       for (uint8_t idx = 0; idx < LED_COUNT; idx++) {
-        cRGB c = LEDControl.getCrgbAt(idx);
+        cRGB c = getCrgbAt(idx);
 
-        Focus.printColor(c.r, c.g, c.b);
+        ::Focus.printColor(c.r, c.g, c.b);
-        Focus.printSpace();
+        ::Focus.printSpace();
       }
       Serial.println();
       break;
@@ -252,7 +236,7 @@ LEDControl_::focusHook(const char *command) {
       color.g = Serial.parseInt();
       color.b = Serial.parseInt();
 
-      LEDControl.setCrgbAt(idx, color);
+      setCrgbAt(idx, color);
       idx++;
     }
     break;
@@ -262,4 +246,6 @@ LEDControl_::focusHook(const char *command) {
   return true;
 }
 
-LEDControl_ LEDControl;
+}
+
+kaleidoscope::LEDControl LEDControl;

src/Kaleidoscope-LEDControl.h

@@ -8,27 +8,100 @@
 
 #define Key_LEDEffectNext (Key) { 0,  KEY_FLAGS | SYNTHETIC | IS_INTERNAL | LED_TOGGLE }
 
+namespace kaleidoscope {
+/** Base class for LED modes.
+ *
+ * LED modes are a special kind of plugin, they are in charge of updating LED
+ * colors, setting a theme. While it is possible to have other plugins
+ * override the mode's colors, the LED mode is the baseline.
+ *
+ * Most of its functionality is called via @ref LEDControl, with only a few
+ * public methods.
+ *
+ * A LED mode **must** implement at least one of @ref onActivate or @ref
+ * update, and possibly @ref refreshAt too.
+ */
 class LEDMode : public KaleidoscopePlugin {
- public:
+  friend class LEDControl;
-  virtual void begin(void);
+ protected:
+  // These methods should only be called by LEDControl.
+
+  /** One-time setup, called at keyboard boot.
+   *
+   * Any hooks that need registering, any one-time setup that needs to be
+   * performed, shall be done here. This is purely for preparation purposes, the
+   * LEDs should not be touched yet at this time.
+   */
   virtual void setup(void) {}
-  virtual void init(void) {}
+
+  /** Function to call whenever the mode is activated.
+   *
+   * Like @ref setup, this method need not touch LEDs, @ref update will be
+   * called right after it. The purpose of this callback is to allow a plugin to
+   * do some preparation whenever it is activated, instead of only on boot, or
+   * always at each cycle.
+   *
+   * However, unlike @ref setup, this method can change LED colors, if so
+   * desired. Either to provide an initial state, or a static color set. In the
+   * latter case, consider implementing @ref refreshAt too, because other
+   * plugins may override some of the colors set at activation time, and @ref
+   * refreshAt can be used to restore them when needed.
+   *
+   * Before the callback runs, LEDs will be blanked.
+   */
+  virtual void onActivate(void) {}
+
+  /** Update the LEDs once per cycle.
+   *
+   * Usually the brains of the plugin, which updates the LEDs each cycle. It is
+   * called after the matrix has been scanned, once per cycle.
+   */
   virtual void update(void) {}
-  virtual void activate(void);
+
+  /** Refresh the color of a given key.
+   *
+   * If we have another plugin that overrides colors set by the active LED mode
+   * (either at @onActivate time, or via @ref update), if that plugin wants to
+   * restore whatever color the mode would set the key color to, this is the
+   * method it will call.
+   *
+   * @param row is the row coordinate of the key to refresh the color of.
+   * @param col is the column coordinate of the key to refresh the color of.
+   */
+  virtual void refreshAt(byte row, byte col) {}
+
+ public:
+  /** Activate the current object as the LED mode.
+   */
+  void activate(void);
+
+  /** Plugin initialization.
+   *
+   * Called via `Kaleidoscope.use()`, registers the LED mode, and does the
+   * necessary initialization steps. Calls @ref setup at the end.
+   */
+  void begin(void) final;
 };
 
-class LEDControl_ : public KaleidoscopePlugin {
+class LEDControl : public KaleidoscopePlugin {
  public:
-  LEDControl_(void);
+  LEDControl(void);
 
   void begin(void) final;
 
   static void next_mode(void);
   static void setup(void);
-  static void update(void);
+  static void update(void) {
+    if (modes[mode])
+      modes[mode]->update();
+  }
+  static void refreshAt(byte row, byte col) {
+    if (modes[mode])
+      modes[mode]->refreshAt(row, col);
+  }
   static void set_mode(uint8_t mode);
-  static uint8_t get_mode();
+  static uint8_t get_mode_index();
-  static void init_mode(void);
+  static LEDMode *get_mode();
 
   static int8_t mode_add(LEDMode *mode);
 
@@ -49,13 +122,14 @@ class LEDControl_ : public KaleidoscopePlugin {
  private:
   static uint32_t syncTimer;
   static LEDMode *modes[LED_MAX_MODES];
-  static uint8_t previousMode, mode;
+  static uint8_t mode;
 
   static Key eventHandler(Key mappedKey, byte row, byte col, uint8_t keyState);
   static void loopHook(bool postClear);
 };
+}
 
-extern LEDControl_ LEDControl;
+extern kaleidoscope::LEDControl LEDControl;
 
 #define FOCUS_HOOK_LEDCONTROL FOCUS_HOOK (LEDControl.focusHook, \
                                           "led.at\n"            \

src/LED-Off.cpp

@@ -1,7 +1,13 @@
 #include "LED-Off.h"
 
-void LEDOff_::update(void) {
+namespace kaleidoscope {
-  LEDControl.set_all_leds_to({0, 0, 0});
+void LEDOff::onActivate(void) {
+  ::LEDControl.set_all_leds_to({0, 0, 0});
 }
 
-LEDOff_ LEDOff;
+void LEDOff::refreshAt(byte row, byte col) {
+  ::LEDControl.setCrgbAt(row, col, {0, 0, 0});
+}
+}
+
+kaleidoscope::LEDOff LEDOff;

src/LED-Off.h

@@ -2,11 +2,15 @@
 
 #include "Kaleidoscope-LEDControl.h"
 
-class LEDOff_ : public LEDMode {
+namespace kaleidoscope {
+class LEDOff : public LEDMode {
  public:
-  LEDOff_(void) { }
+  LEDOff(void) { }
 
-  void update(void) final;
+ protected:
+  void onActivate(void) final;
+  void refreshAt(byte row, byte col) final;
 };
+}
 
-extern LEDOff_ LEDOff;
+extern kaleidoscope::LEDOff LEDOff;