Key union converted to a proper class

Unions are a C-reminiscense that are better avoided in modern C++.
They cause specific problems due to their nature of representing
independent types. The way they are used in Kaleidoscope, they
can easily be replaced by a class.

This enables it to properly work with Key objects in constexpr context
where with the old union-based implementation the compiler reported
errors when one Key was constructed based on a key_code/flags pair and
another one through raw-data. In such a case, the compiler assumes that
both Key instances represent something entirely different. This is
because unions were never meant for type conversions and the C++
standard considers their use for that purpose as undefined behavior.

The new class provides accessor methods for raw-data access and for
key_code/flags-data access.

This is a breaking change as it is is not possible to replace direct
member access patterns like

key.raw = 0xFFFF;

based on the raw-accessors.

For the .keyCode and .flags members, proxy objects are used
to enable the generation of suitable deprecations warnings.

All direct access via .raw, .keyCode and .flags have been replaced
throughout Kaleidoscope.

Signed-off-by: Florian Fleissner <[email protected]>

doc/plugin/Cycle.md

@@ -25,10 +25,10 @@ Key_Cycle
 
 // later in the Sketch:
 void cycleAction(Key previous_key, uint8_t cycle_count) {
-  bool is_shifted = previous_key.flags & SHIFT_HELD;
-  if (previous_key.keyCode == Key_A.keyCode && is_shifted)
+  bool is_shifted = previous_key.getFlags() & SHIFT_HELD;
+  if (previous_key.getKeyCode() == Key_A.getKeyCode() && is_shifted)
       cycleThrough (LSHIFT(Key_A), LSHIFT(Key_B), LSHIFT(Key_C));
-  if (previous_key.keyCode == Key_A.keyCode && !is_shifted)
+  if (previous_key.getKeyCode() == Key_A.getKeyCode() && !is_shifted)
       cycleThrough (Key_A, Key_B, Key_C);
 }
 

examples/Keystrokes/Cycle/Cycle.ino

@@ -40,18 +40,18 @@ KEYMAPS(
 // *INDENT-ON*
 
 void cycleAction(Key previous_key, uint8_t cycle_count) {
-  if (previous_key.raw == Key_E.raw) {
+  if (previous_key == Key_E) {
     if (cycle_count == 1) {
       Cycle.replace(Key_F);
     } else if (cycle_count == 2) {
       Cycle.replace(Key_G);
     }
   }
 
-  bool is_shifted = previous_key.flags & SHIFT_HELD;
-  if (previous_key.keyCode == Key_A.keyCode && is_shifted)
+  bool is_shifted = previous_key.getFlags() & SHIFT_HELD;
+  if (previous_key.getKeyCode() == Key_A.getKeyCode() && is_shifted)
     cycleThrough(LSHIFT(Key_A), LSHIFT(Key_B), LSHIFT(Key_C), LSHIFT(Key_D));
-  if (previous_key.keyCode == Key_A.keyCode && !is_shifted)
+  if (previous_key.getKeyCode() == Key_A.getKeyCode() && !is_shifted)
     cycleThrough(Key_A, Key_B, Key_C, Key_D);
 }
 

examples/LEDs/LED-AlphaSquare/LED-AlphaSquare.ino

@@ -47,7 +47,7 @@ const macro_t *macroAction(uint8_t macro_index, uint8_t key_state) {
     return MACRO_NONE;
 
   if (macro_index == 0) {
-    for (uint8_t i = Key_A.keyCode; i <= Key_0.keyCode; i++) {
+    for (uint8_t i = Key_A.getKeyCode(); i <= Key_0.getKeyCode(); i++) {
       LEDControl.set_all_leds_to(0, 0, 0);
       LEDControl.syncLeds();
       delay(100);

src/kaleidoscope/hooks.h

@@ -19,7 +19,7 @@
 #include <Arduino.h>
 
 namespace kaleidoscope {
-union Key;
+class Key;
 }
 
 #include "kaleidoscope/KeyAddr.h"

src/kaleidoscope/key_defs.h

@@ -30,63 +30,141 @@
 
 namespace kaleidoscope {
 
-union Key {
+class Key {
 
-  struct {
-    uint8_t keyCode;
-    uint8_t flags;
-  };
-  uint16_t raw;
+ public:
+
+  typedef uint16_t StorageType;
 
   Key() = default;
 
-  constexpr Key(uint8_t __keyCode, uint8_t __flags)
-    : keyCode(__keyCode), flags(__flags) {
+  constexpr Key(uint16_t raw)
+    : Key{(uint8_t)(raw & 0x00FF), (uint8_t)(raw >> 8)}
+  {}
+
+  constexpr Key(uint8_t key_code, uint8_t flags)
+    : keyCode{key_code},
+      flags{flags}
+  {}
+
+  void setFlags(uint8_t new_flags) {
+    flags.value_ = new_flags;
+  }
+  constexpr const uint8_t &getFlags() const {
+    return flags.value_;
+  }
+
+  void setKeyCode(uint8_t key_code) {
+    keyCode.value_ = key_code;
+  }
+  constexpr const uint8_t &getKeyCode() const {
+    return keyCode.value_;
   }
 
-  constexpr Key(uint16_t __raw)
-    : raw(__raw) {
+  void setRaw(uint16_t raw) {
+    flags.value_  = (uint8_t)(raw >> 8);
+    keyCode.value_ = (uint8_t)(raw & 0x00FF);
+  }
+  constexpr uint16_t getRaw() const {
+    return (uint16_t)(
+             ((uint16_t)flags.value_ << 8)
+             + (uint16_t)keyCode.value_
+           );
   }
 
-  constexpr bool operator==(const uint16_t rhs) const {
-    return this->raw == rhs;
+  constexpr bool operator==(const StorageType rhs) const {
+    return this->getRaw() == rhs;
   }
   constexpr bool operator==(const Key& rhs) const {
-    return this->raw == rhs.raw;
+    return (this->keyCode.value_ == rhs.keyCode.value_)
+           && (this->flags.value_ == rhs.flags.value_);
   }
-  Key& operator=(const uint16_t raw) {
-    this->raw = raw;
+  Key& operator=(const StorageType raw) {
+    this->setRaw(raw);
     return *this;
   }
   constexpr bool operator!=(const Key& rhs) const {
     return !(*this == rhs);
   }
-  constexpr bool operator>=(const uint16_t raw) const {
-    return this->raw >= raw;
+  constexpr bool operator>=(const StorageType raw) const {
+    return this->getRaw() >= raw;
   }
-  constexpr bool operator<=(const uint16_t raw) const {
-    return this->raw <= raw;
+  constexpr bool operator<=(const StorageType raw) const {
+    return this->getRaw() <= raw;
   }
-  constexpr bool operator>(const uint16_t raw) const {
-    return this->raw > raw;
+  constexpr bool operator>(const StorageType raw) const {
+    return this->getRaw() > raw;
   }
-  constexpr bool operator<(const uint16_t raw) const {
-    return this->raw < raw;
+  constexpr bool operator<(const StorageType raw) const {
+    return this->getRaw() < raw;
   }
   constexpr bool operator>=(const Key& other) const {
-    return this->raw >= other.raw;
+    return this->getRaw() >= other.getRaw();
   }
   constexpr bool operator<=(const Key& other) const {
-    return this->raw <= other.raw;
+    return this->getRaw() <= other.getRaw();
   }
   constexpr bool operator>(const Key& other) const {
-    return this->raw > other.raw;
+    return this->getRaw() > other.getRaw();
   }
   constexpr bool operator<(const Key& other) const {
-    return this->raw < other.raw;
+    return this->getRaw() < other.getRaw();
+  }
+
+  Key readFromProgmem() const {
+    return Key{pgm_read_byte(&(this->getKeyCode())),
+               pgm_read_byte(&(this->getFlags()))};
   }
+
+  // The data proxy objects are required to only emit deprecation
+  // messages when members 'keyCode' and 'flags' are accessed directly
+  // but not if accessed by class Key.
+  //
+  // Once the deprecation periode elapsed both proxy members 'keyCode'
+  // and 'flags' of class Key can be converted to private uint8_t members
+  // of class Key. Class DataProxy can then be safely removed.
+  //
+  class DataProxy {
+
+    friend class Key;
+
+   public:
+
+    DataProxy() = default;
+
+    constexpr DataProxy(uint8_t value) : value_{value} {}
+
+    DEPRECATED(DIRECT_KEY_MEMBER_ACCESS)
+    DataProxy &operator=(uint8_t value) {
+      value_ = value;
+      return *this;
+    }
+
+    DEPRECATED(DIRECT_KEY_MEMBER_ACCESS)
+    constexpr operator uint8_t () const {
+      return value_;
+    }
+
+   private:
+    uint8_t value_;
+  };
+
+  DataProxy keyCode;
+  DataProxy flags;
 };
 
+static_assert(sizeof(Key) == 2, "sizeof(Key) changed");
+
+// Overload this function to define alternative conversions to type Key.
+//
+constexpr Key convertToKey(Key k) {
+  return k;
+}
+
+constexpr Key addFlags(Key k, uint8_t add_flags) {
+  return Key(k.getKeyCode(), k.getFlags() | add_flags);
+}
+
 } // namespace kaleidoscope
 
 // For compatibility reasons make the Key class also available
@@ -105,11 +183,11 @@ typedef kaleidoscope::Key Key_;
 #define SYNTHETIC         B01000000
 #define RESERVED          B10000000
 
-#define LCTRL(k)  Key(k.keyCode, k.flags | CTRL_HELD)
-#define LALT(k)   Key(k.keyCode, k.flags | LALT_HELD)
-#define RALT(k)   Key(k.keyCode, k.flags | RALT_HELD)
-#define LSHIFT(k) Key(k.keyCode, k.flags | SHIFT_HELD)
-#define LGUI(k)   Key(k.keyCode, k.flags | GUI_HELD)
+#define LCTRL(k)  Key(k.getKeyCode(), k.getFlags() | CTRL_HELD)
+#define LALT(k)   Key(k.getKeyCode(), k.getFlags() | LALT_HELD)
+#define RALT(k)   Key(k.getKeyCode(), k.getFlags() | RALT_HELD)
+#define LSHIFT(k) Key(k.getKeyCode(), k.getFlags() | SHIFT_HELD)
+#define LGUI(k)   Key(k.getKeyCode(), k.getFlags() | GUI_HELD)
 
 // we assert that synthetic keys can never have keys held, so we reuse the _HELD bits
 #define IS_SYSCTL                  B00000001
@@ -153,5 +231,9 @@ typedef kaleidoscope::Key Key_;
    use the 10 lsb as the HID Consumer code. If you need to get the keycode of a Consumer key
    use the CONSUMER(key) macro this will return the 10bit keycode.
 */
-#define CONSUMER(key) (key.raw & 0x03FF)
+#define CONSUMER(key) (key.getRaw() & 0x03FF)
 #define CONSUMER_KEY(code, flags) Key((code) | ((flags | SYNTHETIC|IS_CONSUMER) << 8))
+
+namespace kaleidoscope {
+constexpr Key bad_keymap_key{0, RESERVED};
+}

src/kaleidoscope/key_events.cpp

@@ -19,24 +19,24 @@
 #include "kaleidoscope/plugin.h"
 
 static bool handleSyntheticKeyswitchEvent(Key mappedKey, uint8_t keyState) {
-  if (mappedKey.flags & RESERVED)
+  if (mappedKey.getFlags() & RESERVED)
     return false;
 
-  if (!(mappedKey.flags & SYNTHETIC))
+  if (!(mappedKey.getFlags() & SYNTHETIC))
     return false;
 
-  if (mappedKey.flags & IS_CONSUMER) {
+  if (mappedKey.getFlags() & IS_CONSUMER) {
     if (keyIsPressed(keyState))
       kaleidoscope::hid::pressConsumerControl(mappedKey);
-  } else if (mappedKey.flags & IS_SYSCTL) {
+  } else if (mappedKey.getFlags() & IS_SYSCTL) {
     if (keyIsPressed(keyState)) {
     } else if (keyWasPressed(keyState)) {
       kaleidoscope::hid::pressSystemControl(mappedKey);
       kaleidoscope::hid::releaseSystemControl(mappedKey);
     }
-  } else if (mappedKey.flags & IS_INTERNAL) {
+  } else if (mappedKey.getFlags() & IS_INTERNAL) {
     return false;
-  } else if (mappedKey.flags & SWITCH_TO_KEYMAP) {
+  } else if (mappedKey.getFlags() & SWITCH_TO_KEYMAP) {
     // Should not happen, handled elsewhere.
   }
 
@@ -47,7 +47,7 @@ static bool handleKeyswitchEventDefault(Key mappedKey, KeyAddr key_addr, uint8_t
   //for every newly pressed button, figure out what logical key it is and send a key down event
   // for every newly released button, figure out what logical key it is and send a key up event
 
-  if (mappedKey.flags & SYNTHETIC) {
+  if (mappedKey.getFlags() & SYNTHETIC) {
     handleSyntheticKeyswitchEvent(mappedKey, keyState);
   } else if (keyToggledOn(keyState)) {
     kaleidoscope::hid::pressKey(mappedKey);
@@ -82,7 +82,7 @@ void handleKeyswitchEvent(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
     /* If a key had an on event, we update the live composite keymap. See
      * layers.h for an explanation about the different caches we have. */
     if (keyToggledOn(keyState)) {
-      if (mappedKey.raw == Key_NoKey.raw || keyState & EPHEMERAL) {
+      if (mappedKey == Key_NoKey || keyState & EPHEMERAL) {
         Layer.updateLiveCompositeKeymap(key_addr);
       } else {
         Layer.updateLiveCompositeKeymap(key_addr, mappedKey);
@@ -108,7 +108,7 @@ void handleKeyswitchEvent(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
      * The condition here means that if mappedKey and key_addr are both valid,
      *   the mappedKey wins - we don't re-look-up the mappedKey
      */
-    if (mappedKey.raw == Key_NoKey.raw) {
+    if (mappedKey == Key_NoKey) {
       mappedKey = Layer.lookup(key_addr);
     }
 
@@ -133,7 +133,7 @@ void handleKeyswitchEvent(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
     return;
 
   mappedKey = Layer.eventHandler(mappedKey, key_addr, keyState);
-  if (mappedKey.raw == Key_NoKey.raw)
+  if (mappedKey == Key_NoKey)
     return;
   handleKeyswitchEventDefault(mappedKey, key_addr, keyState);
 }

src/kaleidoscope/keymaps.h

@@ -24,7 +24,7 @@ namespace kaleidoscope {
 
 inline
 Key keyFromKeymap(uint8_t layer, KeyAddr key_addr) {
-  return pgm_read_word(&keymaps_linear[layer][key_addr.toInt()].raw);
+  return keymaps_linear[layer][key_addr.toInt()].readFromProgmem();
 }
 
 namespace internal {

src/kaleidoscope/layers.cpp

@@ -43,8 +43,8 @@ uint8_t Layer_::active_layers_[Kaleidoscope.device().numKeys()];
 Key(*Layer_::getKey)(uint8_t layer, KeyAddr key_addr) = Layer.getKeyFromPROGMEM;
 
 void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) {
-  if (keymapEntry.keyCode >= LAYER_SHIFT_OFFSET) {
-    uint8_t target = keymapEntry.keyCode - LAYER_SHIFT_OFFSET;
+  if (keymapEntry.getKeyCode() >= LAYER_SHIFT_OFFSET) {
+    uint8_t target = keymapEntry.getKeyCode() - LAYER_SHIFT_OFFSET;
 
     switch (target) {
     case KEYMAP_NEXT:
@@ -86,15 +86,15 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) {
     }
   } else if (keyToggledOn(keyState)) {
     // switch keymap and stay there
-    if (Layer.isActive(keymapEntry.keyCode) && keymapEntry.keyCode)
-      deactivate(keymapEntry.keyCode);
+    if (Layer.isActive(keymapEntry.getKeyCode()) && keymapEntry.getKeyCode())
+      deactivate(keymapEntry.getKeyCode());
     else
-      activate(keymapEntry.keyCode);
+      activate(keymapEntry.getKeyCode());
   }
 }
 
 Key Layer_::eventHandler(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
-  if (mappedKey.flags != (SYNTHETIC | SWITCH_TO_KEYMAP))
+  if (mappedKey.getFlags() != (SYNTHETIC | SWITCH_TO_KEYMAP))
     return mappedKey;
 
   handleKeymapKeyswitchEvent(mappedKey, keyState);