Key union converted to a proper class

NEWS.md

@@ -156,6 +156,22 @@ The [FirmwareDump](doc/plugin/FirmwareDump.md) plugin makes it possible to dump
 
 ## Breaking changes
 
+### Implementation of type Key internally changed from C++ union to class
+
+Type `Key` was originally implemented as a C++ union. For technical reasons
+it had to be converted to a C++ class. This implies that the double usage
+of the original union, holding either raw data (member `raw`) or key code/key flags
+data (members `keyCode` and `flags`) is no more possible. 
+
+Direct use of member `raw` will 
+emit a diagnostic compiler message but will cause the firmware linking 
+process to fail. For a deprecation
+periode `keyCode` and `flags` keep on being supported but will cause 
+deprecation warnings during compile. 
+
+Please see the [relevant upgrade notes](UPGRADING.md##implementation-of-type-key-internally-changed-from-union-to-class)
+for information about how to upgrade legacy code.
+
 ### The `NumPad` plugin no longer toggles `NumLock`
 
 The `NumPad` plugin used to toggle `NumLock` when switching to the NumPad layer. This caused issues on OSX where `NumLock` is interpreted as `Clear`. For this reason, the plugin no longer does this. As a consequence, everyone's encouraged to update their keymap so that the numpad layer uses normal number keys instead of the keypad numbers. See [Model01-Firmware#79](https://github.com/keyboardio/Model01-Firmware/pull/79) for an example about how to do this.

UPGRADING.md

@@ -12,6 +12,7 @@ If any of this does not make sense to you, or you have trouble updating your .in
     - [Bidirectional communication for plugins](#bidirectional-communication-for-plugins)
     - [Consistent timing](#consistent-timing)
   + [Breaking changes](#breaking-changes)
+    - [Implementation of type Key internally changed from C++ union to class](#implementation-of-type-key-internally-changed-from-union-to-class)
     - [The `RxCy` macros and peeking into the keyswitch state](#the-rxcy-macros-and-peeking-into-the-keyswitch-state)
     - [HostOS](#hostos)
     - [MagicCombo](#magiccombo)
@@ -33,11 +34,11 @@ If any of this does not make sense to you, or you have trouble updating your .in
 
 We are introducing - or rather, replacing - the older hardware plugins, with a system that's much more composable, more extensible, and will allow us to better support new devices, different MCUs, and so on.
 
-### For end-users
+#### For end-users
 
 For end users, this doesn't come with any breaking changes. A few things have been deprecated (`ROWS`, `COLS`, `LED_COUNT`, `KeyboardHardware`), but they still function for the time being.
 
-### For developers
+#### For developers
 
 For those wishing to port Kaleidoscope to devices it doesn't support yet, the new API should make most things considerably easier. Please see the (work in progress) documentation in [doc/device-apis.md](doc/device-apis.md).
 
@@ -313,6 +314,45 @@ As a developer, one can continue using `millis()`, but migrating to `Kaleidoscop
 
 ## Breaking changes
 
+### Implementation of type Key internally changed from C++ union to class
+
+#### For end-users
+
+This is a breaking change only if your code accesses the member `raw` of
+type `Key` directly, for instance in a construct like
+
+```cpp
+Key k;
+k.raw = Key_A.raw;
+```
+
+This can easily be fixed by replacing read access to `Key::raw` with `Key::getRaw()`
+and write access with `Key::setRaw(...)`.
+
+```cpp
+Key k;
+k.setRaw(Key_A.getRaw());
+```
+
+Moreover, the compiler will still emit warnings in places of the code where
+members `keyCode` and `flags` of the original type `Key` are used, like e.g.
+
+```cpp
+Key k;
+k.keyCode = Key_A.keyCode;
+k.flags = Key_A.flags;
+```
+
+These warnings can be also resolved by using the appropriate accessor methods
+`Key::getKeyCode()`/`Key::setKeyCode()` and `Key::getFlags()`/`Key::setKlags()`
+instead.
+
+```cpp
+Key k;
+k.setKeyCode(Key_A.getKeyCode());
+k.setFlags(Key_A.getFlags());
+```
+
 ### The `RxCy` macros and peeking into the keyswitch state
 
 The `RxCy` macros changed from being indexes into a per-hand bitmap to being an

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,163 @@
 
 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;
+
+  // For technical reasons the implementation of type Key has been changed
+  // from a C++ union to a class.
+  //
+  // Although it is not entirely possible to retain all features of the
+  // union-based implementation, we can still warn for access to member raw.
+  //
+  // After converting Key::raw to a static member, it can now still be accessed
+  // as before. But accessing it will trigger a compiler message that informs
+  // the user about the importance of replacing its access
+  // with Key::getRaw() or Key::setRaw().
+  //
+  // This is not a deprecation! All code that accesses Key::raw directly
+  // will fail to link instead.
+  //
+  // This is because there is no instance provided for the static fake
+  // member Key::raw. Because of that, this approach does not mean
+  // any harm. The emitted diagnostics help pointing users
+  // to the places in the code where changes are required.
+  //
+  DEPRECATED(KEY_MEMBER_RAW_ACCESS)
+  static uint16_t raw;
 };
 
+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 +205,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 +253,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};
+}