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]>
keyboardio · Nov 16, 2019 · 547f17e · 547f17e
1 parent aa5b55e
commit 547f17e
Show file tree

Hide file tree

Showing 38 changed files with 329 additions and 252 deletions.
diff --git a/examples/Keystrokes/Cycle/Cycle.ino b/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);
 }
 

diff --git a/examples/LEDs/LED-AlphaSquare/LED-AlphaSquare.ino b/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);

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

diff --git a/src/kaleidoscope/key_defs.h b/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
@@ -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};
+}
diff --git a/src/kaleidoscope/key_events.cpp b/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);
 }
diff --git a/src/kaleidoscope/keymaps.h b/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 {

diff --git a/src/kaleidoscope/layers.cpp b/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);

diff --git a/src/kaleidoscope/plugin/Cycle.cpp b/src/kaleidoscope/plugin/Cycle.cpp
@@ -28,7 +28,7 @@ uint8_t Cycle::cycle_count_;
 
 // --- helpers ---
 
-#define isCycle(k) (k.raw == kaleidoscope::ranges::CYCLE)
+#define isCycle(k) (k.getRaw() == kaleidoscope::ranges::CYCLE)
 
 // --- api ---
 
@@ -46,10 +46,7 @@ void Cycle::replace(Key key) {
 
 void Cycle::replace(uint8_t cycle_size, const Key cycle_steps[]) {
   uint8_t idx = cycle_count_ % cycle_size;
-  Key key;
-
-  key.raw = pgm_read_word(&cycle_steps[idx].raw);
-  replace(key);
+  replace(cycle_steps[idx].readFromProgmem());
 }
 
 // --- hooks ---
@@ -67,13 +64,13 @@ EventHandlerResult Cycle::onKeyswitchEvent(Key &mapped_key, KeyAddr key_addr, ui
 
   if (!isCycle(mapped_key)) {
     if (keyToggledOn(key_state)) {
-      current_modifier_flags_ |= toModFlag(mapped_key.keyCode);
-      last_non_cycle_key_.keyCode = mapped_key.keyCode;
-      last_non_cycle_key_.flags = current_modifier_flags_;
+      current_modifier_flags_ |= toModFlag(mapped_key.getKeyCode());
+      last_non_cycle_key_.setKeyCode(mapped_key.getKeyCode());
+      last_non_cycle_key_.setFlags(current_modifier_flags_);
       cycle_count_ = 0;
     }
     if (keyToggledOff(key_state)) {
-      current_modifier_flags_ &= ~toModFlag(mapped_key.keyCode);
+      current_modifier_flags_ &= ~toModFlag(mapped_key.getKeyCode());
     }
     return EventHandlerResult::OK;
   }
@@ -89,18 +86,18 @@ EventHandlerResult Cycle::onKeyswitchEvent(Key &mapped_key, KeyAddr key_addr, ui
 
 uint8_t Cycle::toModFlag(uint8_t keyCode) {
   switch (keyCode) {
-  case Key_LeftShift.keyCode:
-  case Key_RightShift.keyCode:
+  case Key_LeftShift.getKeyCode():
+  case Key_RightShift.getKeyCode():
     return SHIFT_HELD;
-  case Key_LeftAlt.keyCode:
+  case Key_LeftAlt.getKeyCode():
     return LALT_HELD;
-  case Key_RightAlt.keyCode:
+  case Key_RightAlt.getKeyCode():
     return RALT_HELD;
-  case Key_LeftControl.keyCode:
-  case Key_RightControl.keyCode:
+  case Key_LeftControl.getKeyCode():
+  case Key_RightControl.getKeyCode():
     return CTRL_HELD;
-  case Key_LeftGui.keyCode:
-  case Key_RightGui.keyCode:
+  case Key_LeftGui.getKeyCode():
+  case Key_RightGui.getKeyCode():
     return GUI_HELD;
   default:
     return 0;