Rewrite top-level event handling functions

Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>

src/kaleidoscope/Runtime.cpp

@@ -1,5 +1,5 @@
 /* Kaleidoscope - Firmware for computer input devices
- * Copyright (C) 2013-2018  Keyboard.io, Inc.
+ * Copyright (C) 2013-2021  Keyboard.io, Inc.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU General Public License as published by the Free Software
@@ -22,10 +22,12 @@
 namespace kaleidoscope {
 
 uint32_t Runtime_::millis_at_cycle_start_;
+KeyAddr Runtime_::last_addr_toggled_on_ = KeyAddr::none();
 
 Runtime_::Runtime_(void) {
 }
 
+// ----------------------------------------------------------------------------
 void
 Runtime_::setup(void) {
   // We are explicitly initializing the Serial port as early as possible to
@@ -49,20 +51,294 @@ Runtime_::setup(void) {
   Layer.setup();
 }
 
+// ----------------------------------------------------------------------------
 void
 Runtime_::loop(void) {
   millis_at_cycle_start_ = millis();
 
   kaleidoscope::Hooks::beforeEachCycle();
 
+#ifndef NDEPRECATED
+  // For backwards compatibility. Some plugins rely on the handler for
+  // `beforeReportingState()` being called every cycle. In most cases, they can
+  // simply switch to using `afterEachCycle()`, but we don't want to simply
+  // break those plugins.
+  kaleidoscope::Hooks::beforeReportingState();
+  // Also for backwards compatibility. If user code calls any code that directly
+  // changes the HID report(s) at any point between an event being detected and
+  // the end of `handleKeyEvent()` (most likely from `beforeReportingState()`),
+  // we need to make sure that report doesn't just get discarded.
+  hid().keyboard().sendReport();
+#endif
+
+  // Next, we scan the keyswitches. Any toggle-on or toggle-off events will
+  // trigger a call to `handleKeyswitchEvent()`, which in turn will
+  // (conditionally) result in a HID report. Note that each event gets handled
+  // (and any resulting HID report(s) sent) as soon as it is detected. It is
+  // possible for more than one event to be handled like this in any given
+  // cycle, resulting in multiple HID reports, but guaranteeing that only one
+  // event is being handled at a time.
   device().scanMatrix();
 
-  kaleidoscope::Hooks::beforeReportingState();
+  kaleidoscope::Hooks::afterEachCycle();
+}
 
-  device().hid().keyboard().sendReport();
+// ----------------------------------------------------------------------------
+void
+Runtime_::handleKeyswitchEvent(KeyEvent event) {
+
+  // This function strictly handles physical key events. Any event without a
+  // valid `KeyAddr` gets ignored.
+  if (!event.addr.isValid())
+    return;
+
+  // Ignore any (non-)event that's not a state change. This check should be
+  // unnecessary, as we shouldn't call this function otherwise.
+  if (!(keyToggledOn(event.state) || keyToggledOff(event.state)))
+    return;
+
+  // Set the `Key` value for this event.
+  if (keyToggledOff(event.state)) {
+    // When a key toggles off, set the event's key value to whatever the key's
+    // current value is in the live keys state array.
+    event.key = live_keys[event.addr];
+  } else if (event.key == Key_NoKey) {
+    // When a key toggles on, unless the event already has a key value (i.e. we
+    // were called by a plugin rather than `actOnMatrixScan()`), we look up the
+    // value from the current keymap (overridden by `live_keys`).
+    event.key = lookupKey(event.addr);
+  }
+
+  // Run the plugin event handlers
+  auto result = Hooks::onKeyswitchEvent(event);
+
+  // If an event handler changed `event.key` to `Key_Masked` in order to mask
+  // that keyswitch, we need to propagate that, but since `handleKeyEvent()`
+  // will recognize that value as the signal to do a fresh lookup, so we need to
+  // set that value in `live_keys` now. The alternative would be changing it to
+  // some other sentinel value, and have `handleKeyEvent()` change it back to
+  // `Key_Masked`, but I think this makes more sense.
+  //
+  // Note: It is still important to let events with `Key_Masked` fall through to
+  // `handleKeyEvent()`, because some plugins might still care about the event
+  // regardless of its `Key` value, and more importantly, that's where we clear
+  // masked keys that have toggled off. Alternatively, we could call
+  // `live_keys.clear(addr)` for toggle-off events here, and `mask(addr)` for
+  // toggle-on events, then return, short-cutting the call to
+  // `handleKeyEvent()`. It should work, but some plugins might be able to use
+  // that information.
+  if (event.key == Key_Masked)
+    live_keys.mask(event.addr);
+
+  // Now we check the result from the plugin event handlers, and stop processing
+  // if it was anything other than `OK`.
+  if (result != EventHandlerResult::OK)
+    return;
+
+  // If all the plugin handlers returned OK, we proceed to the next step in
+  // processing the event.
+  handleKeyEvent(event);
+}
+
+// ----------------------------------------------------------------------------
+void
+Runtime_::handleKeyEvent(KeyEvent event) {
+
+  // For events that didn't begin with `handleKeyswitchEvent()`, we need to look
+  // up the `Key` value from the keymap (maybe overridden by `live_keys`).
+  if (event.addr.isValid()) {
+    if (keyToggledOff(event.state) || event.key == Key_NoKey) {
+      event.key = lookupKey(event.addr);
+    }
+  }
+
+  // If any `onKeyEvent()` handler returns `ABORT`, we return before updating
+  // the Live Keys state array; as if the event didn't happen.
+  auto result = Hooks::onKeyEvent(event);
+  if (result == EventHandlerResult::ABORT)
+    return;
+
+  // Update the live keys array based on the new event.
+  if (event.addr.isValid()) {
+    if (keyToggledOff(event.state)) {
+      live_keys.clear(event.addr);
+    } else {
+      live_keys.activate(event.addr, event.key);
+    }
+  }
+
+  // If any `onKeyEvent()` handler returned a value other than `OK`, stop
+  // processing now. Likewise if the event's `Key` value is a no-op.
+  if (result != EventHandlerResult::OK ||
+      event.key == Key_Masked ||
+      event.key == Key_NoKey ||
+      event.key == Key_Transparent)
+    return;
+
+  // If it's a built-in Layer key, we handle it here, and skip sending report(s)
+  if (event.key.isLayerKey()) {
+    Layer.handleKeymapKeyswitchEvent(event.key, event.state);
+    //return;
+  }
+
+  // The System Control HID report contains only one keycode, and gets sent
+  // immediately on `pressSystemControl()` or `releaseSystemControl()`. This is
+  // significantly different from the way the other HID reports work, where held
+  // keys remain in effect for subsequent reports.
+  if (event.key.isSystemControlKey()) {
+    if (keyToggledOn(event.state)) {
+      hid().keyboard().pressSystemControl(event.key);
+    } else { /* if (keyToggledOff(key_state)) */
+      hid().keyboard().releaseSystemControl(event.key);
+    }
+    return;
+  }
+
+  // Until this point, the old report was still valid. Now we construct the new
+  // one, based on the contents of the `live_keys` state array.
+  prepareKeyboardReport(event);
+
+#ifndef NDEPRECATED
+  // Deprecated handlers might depend on values in the report, so we wait until
+  // the new report is otherwise complete before calling them.
+  auto old_result = Hooks::onKeyswitchEvent(event.key, event.addr, event.state);
+  if (old_result == EventHandlerResult::ABORT)
+    return;
+
+  if (old_result != EventHandlerResult::OK ||
+      event.key == Key_Masked ||
+      event.key == Key_NoKey ||
+      event.key == Key_Transparent)
+    return;
+#endif
+
+  // Finally, send the new keyboard report
+  sendKeyboardReport(event);
+}
+
+// ----------------------------------------------------------------------------
+void
+Runtime_::prepareKeyboardReport(const KeyEvent &event) {
+  // before building the new report, start clean
   device().hid().keyboard().releaseAllKeys();
 
-  kaleidoscope::Hooks::afterEachCycle();
+  // Build report from composite keymap cache. This can be much more efficient
+  // with a bitfield. What we should be doing here is going through the array
+  // and checking for HID values (Keyboard, Consumer, System) and directly
+  // adding them to their respective reports. This comes before the old plugin
+  // hooks are called for the new event so that the report will be full complete
+  // except for that new event.
+  for (KeyAddr key_addr : KeyAddr::all()) {
+    // Skip this event's key addr; we will deal with that later. This is most
+    // important in the case of a key release, because we can't safely remove
+    // any keycode(s) added to the report later.
+    if (key_addr == event.addr)
+      continue;
+
+    Key key = live_keys[key_addr];
+
+    // If the key is idle or masked, we can ignore it.
+    if (key == Key_Inactive || key == Key_Masked)
+      continue;
+
+#ifndef NDEPRECATED
+    // Only run hooks for plugin keys. If a plugin needs to do something every
+    // cycle, it can use one of the every-cycle hooks and search for active keys
+    // of interest.
+    auto result = Hooks::onKeyswitchEvent(key, key_addr, IS_PRESSED | WAS_PRESSED);
+    if (result == EventHandlerResult::ABORT)
+      continue;
+
+    if (key_addr == event.addr) {
+      // update active keys cache?
+      if (keyToggledOn(event.state)) {
+        live_keys.activate(event.addr, key);
+      } else {
+        live_keys.clear(event.addr);
+      }
+    }
+#endif
+
+    addToReport(key);
+  }
+}
+
+// ----------------------------------------------------------------------------
+void
+Runtime_::addToReport(Key key) {
+  // First, call any relevant plugin handlers, to give them a chance to add
+  // other values to the HID report directly and/or to abort the automatic
+  // adding of keycodes below.
+  auto result = Hooks::onAddToReport(key);
+  if (result == EventHandlerResult::ABORT)
+    return;
+
+  if (key.isKeyboardKey()) {
+    // The only incidental Keyboard modifiers that are allowed are the ones on
+    // the key that generated the event, so we strip any others before adding
+    // them. This might turn out to be too simple to cover all the corner cases,
+    // but the OS should be helpful and do most of the masking we want for us.
+    if (!key.isKeyboardModifier())
+      key.setFlags(0);
+
+    hid().keyboard().pressKey(key);
+    return;
+  }
+
+  if (key.isConsumerControlKey()) {
+    hid().keyboard().pressConsumerControl(key);
+    return;
+  }
+
+  // System Control keys and Layer keys are not handled here because they only
+  // take effect on toggle-on or toggle-off events, they don't get added to HID
+  // reports when held.
+}
+
+// ----------------------------------------------------------------------------
+void
+Runtime_::sendKeyboardReport(const KeyEvent &event) {
+  // If the keycode for this key is already in the report, we need to send an
+  // extra report without that keycode in order to correctly process the
+  // rollover. It might be better to exempt modifiers from this rule, but it's
+  // not clear that would be better.
+  if (keyToggledOn(event.state) &&
+      event.key.isKeyboardKey()) {
+    // last keyboard key toggled on
+    last_addr_toggled_on_ = event.addr;
+    if (hid().keyboard().isKeyPressed(event.key)) {
+      // The keycode (flags ignored) for `event.key` is active in the current
+      // report. Should this be `wasKeyPressed()` instead? I don't think so,
+      // because (if I'm right) the new event hasn't been added yet.
+      hid().keyboard().releaseKey(event.key);
+      hid().keyboard().sendReport();
+    }
+    if (event.key.getFlags() != 0) {
+      hid().keyboard().pressModifiers(event.key);
+      hid().keyboard().sendReport();
+    }
+  } else if (event.addr != last_addr_toggled_on_) {
+    // (not a keyboard key OR toggled off) AND not last keyboard key toggled on
+    Key last_key = live_keys[last_addr_toggled_on_];
+    if (last_key.isKeyboardKey()) {
+      hid().keyboard().pressModifiers(last_key);
+    }
+  }
+  if (keyToggledOn(event.state)) {
+    addToReport(event.key);
+  }
+
+#ifndef NDEPRECATED
+  // Call old pre-report handlers:
+  Hooks::beforeReportingState();
+#endif
+
+  // Call new pre-report handlers:
+  if (Hooks::beforeReportingState(event) == EventHandlerResult::ABORT)
+    return;
+
+  // Finally, send the report:
+  device().hid().keyboard().sendReport();
 }
 
 Runtime_ Runtime;

src/kaleidoscope/Runtime.h

@@ -19,6 +19,9 @@
 #include "kaleidoscope_internal/device.h"
 #include "kaleidoscope/event_handler_result.h"
 #include "kaleidoscope/hooks.h"
+#include "kaleidoscope/KeyEvent.h"
+#include "kaleidoscope/LiveKeys.h"
+#include "kaleidoscope/layers.h"
 
 namespace kaleidoscope {
 
@@ -128,8 +131,82 @@ class Runtime_ {
     return kaleidoscope::Hooks::onFocusEvent(command);
   }
 
+  /** Handle a physical keyswitch event
+   *
+   * This method is called in response to physical keyswitch state changes. Its
+   * job is to call the `onKeyswitchEvent()` plugin handler functions, used by
+   * plugins that are particularly concerned about the timing of those
+   * events. It takes only one parameter, of type `KeyEvent`, which encapsulates
+   * the information about that event:
+   *
+   * - `event.key_addr`: The address of the key that was pressed or released.
+   * - `event.state`: The state of the keyswitch event (toggled on or off).
+   * - `event.key`: The `Key` value for this event.
+   * - `event.id`: A semi-unique ID value for the event.
+   *
+   * The ID value is used to help plugins that delay events to coordinate with
+   * each other so that they can avoid re-processing the same event, possibly
+   * causing endless loops.
+   */
+  void handleKeyswitchEvent(KeyEvent event);
+
+  /** Handle a logical key event
+   *
+   * This method triggers the handling of a logical "key event". Ususally that
+   * event is the result of a call to `handleKeyswitchEvent()`, but it can also
+   * be called by plugins that need to generate extra events without a 1:1
+   * mapping to physical keyswitch state transitions.
+   */
+  void handleKeyEvent(KeyEvent event);
+
+  /** Prepare a new set of USB HID reports
+   *
+   * This method gets called when a key event results in at least one new HID
+   * report being sent to the host, usually as a result of a call to
+   * `handleKeyEvent()`. It clears the keyboard report (after plugins have
+   * already responded to the new event that triggered the forthcoming report),
+   * then populates the new report based on the values stored in the `live_keys`
+   * state array.
+   */
+  void prepareKeyboardReport(const KeyEvent &event);
+
+  /** Add keycode(s) to a USB HID report
+   *
+   * This method gets called from `prepareKeyboardReport()` to add keycodes
+   * corresponding to active keys in the `live_keys` state array to the Keyboard
+   * & Consumer Control HID reports. It calls the `onAddToReport()` plugin
+   * handlers first to give them a chance to abort.
+   */
+  void addToReport(Key key);
+
+  /** Send the new USB HID report(s)
+   *
+   * This method is called by `handleKeyEvent()` after `prepareKeyboardReport()`
+   * is done. It uses the information about the new event to guard against
+   * modifier and mod-flags rollover issues, and calls the
+   * `beforeReportingState()` plugin handler functions before sending the
+   * complete Keyboard and Consumer Control HID reports.
+   */
+  void sendKeyboardReport(const KeyEvent &event);
+
+  /** Get the current value of a keymap entry
+   *
+   * Returns the `Key` value for a given `KeyAddr` entry in the current keymap,
+   * overridden by any active entry in the `live_keys` array.
+   */
+  Key lookupKey(KeyAddr key_addr) {
+    // First, check for an active key value in the `live_keys` array.
+    Key key = live_keys[key_addr];
+    // If that entry is clear, look up the entry from the active keymap layers.
+    if (key == Key_Transparent) {
+      key = Layer.lookupOnActiveLayer(key_addr);
+    }
+    return key;
+  }
+
  private:
   static uint32_t millis_at_cycle_start_;
+  static KeyAddr last_addr_toggled_on_;
 };
 
 extern kaleidoscope::Runtime_ Runtime;

src/kaleidoscope/device/virtual/Virtual.cpp

@@ -24,7 +24,6 @@
 #include "kaleidoscope/keyswitch_state.h"
 #include "kaleidoscope/MatrixAddr.h"
 #include "kaleidoscope/key_defs.h"
-#include "kaleidoscope/key_events.h"
 
 
 #include "HIDReportObserver.h"

src/kaleidoscope/driver/hid/base/Keyboard.h

@@ -135,64 +135,14 @@ class Keyboard {
   }
 
   void sendReport() __attribute__((noinline)) {
-    // Before sending the report, we add any modifier flags that are currently
-    // allowed, based on the latest keypress:
-    pressModifiers(requested_modifier_flags & modifier_flag_mask);
-
-    // If a key has been toggled on in this cycle, we might need to send an extra
-    // HID report to the host, because that key might have the same keycode as
-    // another key that was already in the report on the previous cycle. For
-    // example, a user could have two `Key_E` keys in their keymap, in order to
-    // avoid repeating the same key with one finger. Or one might have a
-    // `LCTRL(Key_S)` and a plain `Key_S`, and have a reason to press them in
-    // rapid succession. In order to make this work, we need to call `release()` &
-    // `sendReport()` to send a release event to the host so that its normal
-    // repeat-rate-limiting behaviour won't effectively mask the second keypress.
-    // Then we call `press()` to add the keycode back in before sending the normal
-    // report.
-    //
-    // In most cases, this won't result in any difference from the previous report
-    // (because the newly-toggled-on keycode won't be in the previous report), so
-    // no extra report will be sent (because we suppress duplicate reports in
-    // KeyboardioHID). If there is a difference in the modifiers byte, an extra
-    // report would be sent later, regardless (also in KeyboardioHID).
-    //
-    // Furthermore, we need to send a report without the keycode for the
-    // newly-toggled-on key, but with any masked modifiers from flags removed. For
-    // example, if we roll over from `LSHIFT(Key_A)` to `Key_B`, we need to first
-    // send a report without the `shift`, then a second report with the `B`. If
-    // both of those bits are changed in the same report, at least one major OS
-    // processes the `B` keypress first, and we end up with `AB` instead of `Ab`
-    // in the output.
-
     if (boot_keyboard_.getProtocol() == HID_BOOT_PROTOCOL) {
-      if (last_keycode_toggled_on) {
-        boot_keyboard_.release(last_keycode_toggled_on);
-        boot_keyboard_.sendReport();
-        boot_keyboard_.press(last_keycode_toggled_on);
-        last_keycode_toggled_on = 0;
-      }
       boot_keyboard_.sendReport();
       return;
     }
-
-    if (last_keycode_toggled_on) {
-      // It would be good if KeyboardioHID's Keyboard object offered a way to
-      // compare the modifiers bytes of the current and previous key reports. That
-      // would allow us to only send these extra reports when either
-      // `last_keycode_toggled_on` was already held, or the modifiers byte
-      // changed. Likewise for BootKeyboard above.
-      nkro_keyboard_.release(last_keycode_toggled_on);
-      nkro_keyboard_.sendReport();
-      nkro_keyboard_.press(last_keycode_toggled_on);
-      last_keycode_toggled_on = 0;
-    }
-
     nkro_keyboard_.sendReport();
     consumer_control_.sendReport();
   }
   void releaseAllKeys() __attribute__((noinline)) {
-    resetModifierTracking();
     if (boot_keyboard_.getProtocol() == HID_BOOT_PROTOCOL) {
       boot_keyboard_.releaseAll();
     } else {
@@ -218,71 +168,30 @@ class Keyboard {
     }
   }
 
-  // pressKey takes a Key, as well as optional boolean 'toggledOn' which defaults
-  // to 'true'
-
-  // If toggled_on is not set to false, this routine adds the modifier flags on
-  // this key to the bitmask of modifiers that are allowed to be added to the
-  // upcoming report. We do this so that when we roll over from a key with a
-  // modifier flag to one without it, that modifier flag won't affect the new
-  // keypress.
-
-  // If the key we're processing is a modifier key, any modifier flags attached to
-  // it are added directly to the report along with the modifier from its keycode
-  // byte.
-  //
-  // (A 'modifier key' is one of the eight modifier keys defined by the HID
-  // standard: left and right variants of Control, Shift, Alt, and GUI.)
-
-  // Eventually it calls pressRawKey.
-
-  void pressKey(Key pressed_key, boolean toggled_on = true) __attribute__((noinline)) {
-    if (toggled_on) {
-      // If two keys are toggled on during the same USB report, we would ideally
-      // send an extra USB report to help the host handle each key correctly, but
-      // this is problematic.
-
-      // If we simply allow modifiers associated with the second newly-pressed
-      // key, it is possible to drop a modifier before the report is sent.
-      // Instead, we send modifiers associated with any newly-pressed keys.
-
-      // The downside of this behavior is that in cases where the user presses
-      // down keys with conflicting modifiers at the exact same moment, they may
-      // get unexpected behavior.
-
-      // If this is the first 'new' keycode being pressed in this cycle, reset the
-      // bitmask of modifiers we're willing to attach to USB HID keyboard reports
-      if (!last_keycode_toggled_on) {
-        modifier_flag_mask = 0;
-      }
-
-      // Add any modifiers attached to this key to the bitmask of modifiers we're
-      // willing to attach to USB HID keyboard reports
-      modifier_flag_mask |= pressed_key.getFlags();
-
-      if (!isModifierKey(pressed_key)) {
-        last_keycode_toggled_on = pressed_key.getKeyCode();
-      }
-    }
-
-
-    if (isModifierKey(pressed_key)) {
-      // If the key is a modifier key with additional modifiers attached to it as
-      // flags (as one might when creating a 'Hyper' key or a "Control Alt" key,
-      // we assume that all those modifiers are intended to modify other keys
-      // pressed while this key is held, so they are never masked out.
-      pressModifiers(pressed_key.getFlags());
-    } else {
-      // If, instead, the modifiers are attached to a 'printable' or non-modifier
-      // key, we assume that they're not intended to modify other keys, so we add
-      // them to requested_modifier_flags, and only allow them to affect the report if
-      // the most recent keypress includes those modifiers.
-      requestModifiers(pressed_key.getFlags());
-    }
+  DEPRECATED(HID_KEYBOARD_PRESSKEY_TOGGLEDON)
+  void pressKey(Key pressed_key, bool toggled_on) __attribute__((noinline)) {
+    pressKey(pressed_key);
+  }
 
+  void pressKey(Key pressed_key) __attribute__((noinline)) {
+    pressModifiers(pressed_key);
     pressRawKey(pressed_key);
   }
 
+  void pressModifiers(Key pressed_key) {
+    pressModifiers(pressed_key.getFlags());
+  }
+  void releaseModifiers(Key released_key) {
+    releaseModifiers(released_key.getFlags());
+  }
+  void clearModifiers() {
+    releaseRawKey(Key_LeftShift);
+    releaseRawKey(Key_LeftControl);
+    releaseRawKey(Key_LeftAlt);
+    releaseRawKey(Key_RightAlt);
+    releaseRawKey(Key_LeftGui);
+  }
+
   // pressRawKey takes a Key object and calles KeyboardioHID's ".press" method
   // with its keycode. It does no processing of any flags or modifiers on the key
   void pressRawKey(Key pressed_key) {
@@ -304,29 +213,17 @@ class Keyboard {
   }
 
   void releaseKey(Key released_key) {
-    // Remove any modifiers attached to this key from the bitmask of modifiers we're
-    // willing to attach to USB HID keyboard reports
-    modifier_flag_mask &= ~(released_key.getFlags());
-
-    if (!isModifierKey(released_key)) {
-
-      // TODO(anyone): this code is incomplete, but is better than nothing
-      // If we're toggling off the most recently toggled on key, clear
-      // last_keycode_toggled_on
-      if (last_keycode_toggled_on == released_key.getKeyCode()) {
-        last_keycode_toggled_on = 0;
-      }
-
-      // If the modifiers are attached to a 'printable' or non-modifier
-      // key, we need to clean up after the key press which would have requested
-      // the modifiers be pressed if the most recent keypress includes those modifiers.
-      cancelModifierRequest(released_key.getFlags());
-    }
-
     releaseModifiers(released_key.getFlags());
     releaseRawKey(released_key);
   }
 
+  bool isKeyPressed(Key key) {
+    if (boot_keyboard_.getProtocol() == HID_BOOT_PROTOCOL) {
+      return boot_keyboard_.isKeyPressed(key.getKeyCode());
+    }
+    return nkro_keyboard_.isKeyPressed(key.getKeyCode());
+  }
+
   boolean isModifierKeyActive(Key modifier_key) {
     if (boot_keyboard_.getProtocol() == HID_BOOT_PROTOCOL) {
       return boot_keyboard_.isModifierActive(modifier_key.getKeyCode());
@@ -384,70 +281,6 @@ class Keyboard {
   // not be a valid System Control keycode.
   uint8_t last_system_control_keycode_ = 0;
 
-  // modifier_flag_mask is a bitmask of modifiers that we found attached to
-  // keys that were newly pressed down during the most recent cycle with any new
-  // keypresses.
-
-  // This is used to determine which modifier flags will be allowed to be added to
-  // the current keyboard HID report. It gets set during any cycle where one or
-  // more new keys is toggled on and presists until the next cycle with a newly
-  // detected keypress.
-
-  uint8_t modifier_flag_mask = 0;
-
-  // The functions in this namespace are primarily to solve the problem of
-  // rollover from a key with a modifier flag (e.g. `LSHIFT(Key_T)`) to one
-  // without (e.g. `Key_H`), which used to result in the mod flag being applied to
-  // keys other than the one with the flag. By using `modifier_flag_mask`, we can
-  // mask out any modifier flags that aren't attached to modifier keys or keys
-  // pressed or held in the most recent cycle, mitigating the rollover problem,
-  //  and getting the intended `The` instead of `THe`.
-
-  // requested_modifier_flags is bitmap of the modifiers attached to any non-modifier
-  // key found to be pressed during the most recent cycle. For example, it would
-  // include modifiers attached to Key_A, but not modifiers attached to
-  // Key_LeftControl
-
-  uint8_t requested_modifier_flags = 0;
-
-  // last_keycode_toggled_on is the keycode of the key most recently toggled on
-  // for this report.  This is set when a keypress is first detected and cleared
-  // after the report is sent. If multiple keys are toggled on during a single
-  // cycle, this contains the most recently handled one.
-
-  uint8_t last_keycode_toggled_on = 0;
-
-  void resetModifierTracking(void) {
-    last_keycode_toggled_on = 0;
-    requested_modifier_flags = 0;
-  }
-
-  // isModifierKey takes a Key and returns true if the key is a
-  // keyboard key corresponding to a modifier like Control, Alt or Shift
-  // TODO(anyone): This function should be lifted to the Kaleidoscope core, somewhere.
-
-  bool isModifierKey(Key key) {
-    // If it's not a keyboard key, return false
-    if (key.getFlags() & (SYNTHETIC | RESERVED)) return false;
-
-    return (key.getKeyCode() >= HID_KEYBOARD_FIRST_MODIFIER &&
-            key.getKeyCode() <= HID_KEYBOARD_LAST_MODIFIER);
-  }
-
-  // requestModifiers takes a bitmap of modifiers that might apply
-  // to the next USB HID report and adds them to a bitmap of all such modifiers.
-
-  void requestModifiers(byte flags) {
-    requested_modifier_flags |= flags;
-  }
-
-  // cancelModifierRequest takes a bitmap of modifiers that should no longer apply
-  // to the next USB HID report and removes them from the bitmap of all such modifiers.
-
-  void cancelModifierRequest(byte flags) {
-    requested_modifier_flags &= ~flags;
-  }
-
   // pressModifiers takes a bitmap of modifier keys that must be included in
   // the upcoming USB HID report and passes them through to KeyboardioHID
   // immediately

src/kaleidoscope/driver/hid/keyboardio/Keyboard.h

@@ -69,6 +69,9 @@ class BootKeyboardWrapper {
     BootKeyboard.releaseAll();
   }
 
+  bool isKeyPressed(uint8_t code) {
+    return BootKeyboard.isKeyPressed(code);
+  }
   bool isModifierActive(uint8_t code) {
     return BootKeyboard.isModifierActive(code);
   }
@@ -108,6 +111,9 @@ class NKROKeyboardWrapper {
     Keyboard.releaseAll();
   }
 
+  bool isKeyPressed(uint8_t code) {
+    return Keyboard.isKeyPressed(code);
+  }
   bool isModifierActive(uint8_t code) {
     return Keyboard.isModifierActive(code);
   }

src/kaleidoscope/driver/keyscanner/Base_Impl.h

@@ -19,15 +19,30 @@
 
 #include "kaleidoscope/driver/keyscanner/Base.h"
 #include "kaleidoscope/device/device.h"
+#include "kaleidoscope/keyswitch_state.h"
+#include "kaleidoscope/KeyEvent.h"
+#include "kaleidoscope/Runtime.h"
 
 namespace kaleidoscope {
 namespace driver {
 namespace keyscanner {
 
 template<>
-void Base<kaleidoscope::Device::Props::KeyScannerProps>::handleKeyswitchEvent(Key mappedKey, kaleidoscope::Device::Props::KeyScannerProps::KeyAddr key_addr, uint8_t keyState) {
-  ::handleKeyswitchEvent(mappedKey, key_addr, keyState);
+void Base<kaleidoscope::Device::Props::KeyScannerProps>::handleKeyswitchEvent(
+  Key key __attribute__((unused)),
+  kaleidoscope::Device::Props::KeyScannerProps::KeyAddr key_addr,
+  uint8_t key_state) {
+
+  // Because the `KeyEvent` constructor invoked below assigns a new `EventId`
+  // each time it's called, and plugins that implement `onKeyswitchEvent()` and
+  // use those event ID numbers to determine whether or not an event is new,
+  // it's critical that we do the test for keyswitches toggling on or off first.
+  if (keyToggledOn(key_state) || keyToggledOff(key_state)) {
+    auto event = KeyEvent::next(key_addr, key_state);
+    kaleidoscope::Runtime.handleKeyswitchEvent(event);
+  }
 }
+
 } // namespace keyscanner
 } // namespace driver
 } // namespace kaleidoscope

src/kaleidoscope/event_handlers.h

@@ -139,6 +139,7 @@ class SignatureCheckDummy {};
                (),(),(), /* non template */                               __NL__ \
                (), (), ##__VA_ARGS__)                                     __NL__ \
                                                                           __NL__ \
+   /* DEPRECATED                                                       */ __NL__ \
    /* Function called for every non-idle key, every cycle, so it       */ __NL__ \
    /* can decide what to do with it. It can modify the key (which is   */ __NL__ \
    /* passed by reference for this reason), and decide whether         */ __NL__ \
@@ -148,7 +149,7 @@ class SignatureCheckDummy {};
    /* will stop processing there.                                      */ __NL__ \
    OPERATION(onKeyswitchEvent,                                            __NL__ \
               1,                                                          __NL__ \
-             _CURRENT_IMPLEMENTATION,                                     __NL__ \
+             DEPRECATED(ON_KEYSWITCH_EVENT_V1),                           __NL__ \
                _ABORTABLE,                                                __NL__ \
                 (),(),(), /* non template */                              __NL__ \
                (Key &mappedKey, KeyAddr key_addr, uint8_t keyState),      __NL__ \
@@ -238,7 +239,7 @@ class SignatureCheckDummy {};
    /* reported to the host.                                            */ __NL__ \
    OPERATION(beforeReportingState,                                        __NL__ \
              1,                                                           __NL__ \
-             _CURRENT_IMPLEMENTATION,                                     __NL__ \
+             DEPRECATED(BEFORE_REPORTING_STATE_V1),                       __NL__ \
                _NOT_ABORTABLE,                                            __NL__ \
                (),(),(), /* non template */                               __NL__ \
                (),(),##__VA_ARGS__)                                       __NL__ \

src/kaleidoscope/hooks.h

@@ -23,13 +23,16 @@ class Key;
 }
 
 #include "kaleidoscope/KeyAddr.h"
+#include "kaleidoscope/KeyEvent.h"
 #include "kaleidoscope/plugin.h"
 #include "kaleidoscope/event_handlers.h"
 
 // Forward declaration required to enable friend declarations
 // in class Hooks.
 class kaleidoscope_;
+#ifndef NDEPRECATED
 extern void handleKeyswitchEvent(kaleidoscope::Key mappedKey, KeyAddr key_addr, uint8_t keyState);
+#endif
 
 namespace kaleidoscope {
 namespace plugin {
@@ -67,9 +70,11 @@ class Hooks {
   friend class ::kaleidoscope::plugin::LEDControl;
   friend void ::kaleidoscope::sketch_exploration::pluginsExploreSketch();
 
+#ifndef NDEPRECATED
   // ::handleKeyswitchEvent(...) calls Hooks::onKeyswitchEvent.
   friend void ::handleKeyswitchEvent(kaleidoscope::Key mappedKey,
                                      KeyAddr key_addr, uint8_t keyState);
+#endif
 
  private:
 

src/kaleidoscope/key_events.cpp

@@ -14,115 +14,21 @@
  * this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifndef NDEPRECATED
 #include "kaleidoscope/Runtime.h"
 #include "kaleidoscope/LiveKeys.h"
 #include "kaleidoscope/hooks.h"
 #include "kaleidoscope/keyswitch_state.h"
 #include "kaleidoscope/layers.h"
+#include "kaleidoscope/event_handler_result.h"
 
-static bool handleSyntheticKeyswitchEvent(Key mappedKey, uint8_t keyState) {
-  if (mappedKey.getFlags() & RESERVED)
-    return false;
-
-  if (!(mappedKey.getFlags() & SYNTHETIC))
-    return false;
-
-  using kaleidoscope::Runtime;
-
-  if (mappedKey.getFlags() & IS_CONSUMER) {
-    if (keyIsPressed(keyState))
-      Runtime.hid().keyboard().pressConsumerControl(mappedKey);
-  } else if (mappedKey.getFlags() & IS_SYSCTL) {
-    if (keyToggledOn(keyState)) {
-      Runtime.hid().keyboard().pressSystemControl(mappedKey);
-    } else if (keyToggledOff(keyState)) {
-      Runtime.hid().keyboard().releaseSystemControl(mappedKey);
-    }
-  } else if (mappedKey.getFlags() & IS_INTERNAL) {
-    return false;
-  } else if (mappedKey.getFlags() & SWITCH_TO_KEYMAP) {
-    Layer.handleKeymapKeyswitchEvent(mappedKey, keyState);
-  }
-
-  return true;
-}
-
-static bool handleKeyswitchEventDefault(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
-  using kaleidoscope::Runtime;
-  //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.getFlags() & SYNTHETIC) {
-    handleSyntheticKeyswitchEvent(mappedKey, keyState);
-  } else if (keyToggledOn(keyState)) {
-    Runtime.hid().keyboard().pressKey(mappedKey);
-  } else if (keyIsPressed(keyState)) {
-    Runtime.hid().keyboard().pressKey(mappedKey, false);
-  } else if (keyToggledOff(keyState) && (keyState & INJECTED)) {
-    Runtime.hid().keyboard().releaseKey(mappedKey);
-  }
-  return true;
-}
-
-void handleKeyswitchEvent(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
-
-  using kaleidoscope::Runtime;
-
-  /* These first steps are only done for keypresses that have a valid key_addr.
-   * In particular, doing them for keypresses with out-of-bounds key_addr
-   *   would cause out-of-bounds array accesses in Layer.lookup(),
-   *   Layer.updateLiveCompositeKeymap(), etc.
-   * Note that many INJECTED keypresses use UnknownKeyswitchLocation
-   *   which gives us an invalid key_addr here.  Therefore, it's legitimate that
-   *   we may have keypresses with out-of-bounds key_addr.
-   * However, some INJECTED keypresses do have valid key_addr if they are
-   *   injecting an event tied to a physical keyswitch - and we want them to go
-   *   through this lookup.
-   * So we can't just test for INJECTED here, we need to test the key_addr
-   *   directly.
-   * Note also that this key_addr test avoids out-of-bounds accesses in *core*,
-   *   but doesn't guarantee anything about event handlers - event handlers may
-   *   still receive out-of-bounds key_addr, and handling that properly is on
-   *   them.
-   */
-  if (key_addr.isValid()) {
-    // If the caller did not supply a `Key` value, get it from the keymap
-    // cache. If that value is transparent, look it up from the active layer for
-    // that key address.
-    if (mappedKey == Key_NoKey) {
-      // Note: If the next line returns `Key_NoKey`, that will effectively mask
-      // the key.
-      mappedKey = Layer.lookup(key_addr);
-    }
-
-  }  // key_addr valid
-
-  // Keypresses with out-of-bounds key_addr start here in the processing chain
-  auto result = kaleidoscope::Hooks::onKeyswitchEvent(mappedKey, key_addr, keyState);
-
-  // If any plugin returns `ABORT`, stop here and don't update the active keys
-  // cache entry.
+// Deprecated. See `Runtime.handleKeyEvent()`
+void handleKeyswitchEvent(Key key, KeyAddr key_addr, uint8_t key_state) {
+  // Perhaps we should call deprecated plugin event handlers here?
+  auto result = kaleidoscope::Hooks::onKeyswitchEvent(key, key_addr, key_state);
   if (result == kaleidoscope::EventHandlerResult::ABORT)
     return;
-
-  // Update the keyboard state array
-  if (key_addr.isValid()) {
-    if (keyToggledOn(keyState)) {
-      kaleidoscope::live_keys.activate(key_addr, mappedKey);
-    } else if (keyToggledOff(keyState)) {
-      kaleidoscope::live_keys.clear(key_addr);
-    }
-  }
-
-  // Only continue if all plugin handlers returned `OK`.
-  if (result != kaleidoscope::EventHandlerResult::OK)
-    return;
-
-  // If the key has been masked (i.e. `Key_NoKey`), or it's a plugin-specific
-  // key (`RESERVED`), don't bother continuing.
-  if (mappedKey == Key_NoKey || (mappedKey.getFlags() & RESERVED) != 0)
-    return;
-
-  // Handle all built-in key types.
-  handleKeyswitchEventDefault(mappedKey, key_addr, keyState);
+  if (keyIsPressed(key_state))
+    kaleidoscope::Runtime.addToReport(key);
 }
+#endif

src/kaleidoscope/key_events.h

@@ -65,4 +65,10 @@
  * currentState may be flagged INJECTED, which signals that the event was
  * injected, and is not a direct result of a keypress, coming from the scanner.
  */
+
+#ifndef NDEPRECATED
+
+DEPRECATED(HANDLE_KEYSWITCH_EVENT)
 void handleKeyswitchEvent(Key mappedKey, kaleidoscope::Device::Props::KeyScannerProps::KeyAddr key_addr, uint8_t keyState);
+
+#endif

src/kaleidoscope/layers.h

@@ -20,12 +20,15 @@
 #include "kaleidoscope/key_defs.h"
 #include "kaleidoscope/keymaps.h"
 #include "kaleidoscope/device/device.h"
-#include "kaleidoscope/LiveKeys.h"
 #include "kaleidoscope_internal/device.h"
 #include "kaleidoscope_internal/sketch_exploration/sketch_exploration.h"
 #include "kaleidoscope_internal/shortname.h"
 #include "kaleidoscope_internal/deprecations.h"
 
+#ifndef NDEPRECATED
+#include "kaleidoscope/LiveKeys.h"
+#endif
+
 #define START_KEYMAPS                                                   __NL__ \
    constexpr Key keymaps_linear[][kaleidoscope_internal::device.matrix_rows * kaleidoscope_internal::device.matrix_columns] PROGMEM = {
 
@@ -54,35 +57,30 @@ class Layer_ {
 
   void setup();
 
-  /* There are two lookup functions, because we have two caches, and different
-   * parts of the firmware will want to use either this or that (or perhaps
-   * both, in rare cases).
-   *
-   * First of all, we use caches because looking up a key through all the layers
-   * is costy, and the cost increases dramatically the more layers we have.
-   *
-   * Then, we have the `liveCompositeKeymap`, because to have layer behaviours
-   * we want, that is, if you hold a key on a layer, release the layer key but
-   * continue holding the other, we want for the layered keycode to continue
-   * repeating.
-   *
-   * At the same time, we want other keys to not be affected by the
-   * now-turned-off layer. So we update the keycode in the cache on-demand, when
-   * the key is pressed. (see the top of `handleKeyswitchEvent`).
-   *
-   * On the other hand, we also have plugins that scan the whole keymap, and do
-   * things based on that information, such as highlighting keys that changed
-   * between layers. These need to be able to look at a state of where the
-   * keymap *should* be, not necessarily where it is. The `liveCompositeKeymap`
-   * is not useful here. So we use `activeLayers` which we update whenever
-   * layers change (see `Layer.on` and `Layer.off`), and it updates the cache to
-   * show how the keymap should look, without the `liveCompositeKeymap`-induced
-   * behaviour.
-   *
-   * Thus, if we are curious about what a given key will do, use `lookup`. If we
-   * are curious what the active layer state describes the key as, use
-   * `lookupOnActiveLayer`.
-   */
+  // There are two lookup functions here, for historical reasons. Previously,
+  // Kaleidoscope would need to look up a value for each active keyswitch in
+  // every cycle, and pass that value on to the "event" handlers. Most of these
+  // lookups were for keys that were being held, not toggled on or off. Because
+  // these lookups were so frequent, a cache was used to speed them up.
+  //
+  // We no longer need to look up these values every cycle for keys that are
+  // held, because Kaleidoscope now only acts on key events that are actual
+  // toggle-on or toggle-off events, so the speed of the lookups here is not so
+  // critical. However, the old "live composite keymap" cache was also used by
+  // some plugins (and certain parts of Kaleidoscope itself) to override values
+  // in the keymap, and these plugins might use calls to `Layer.lookup()`,
+  // expecting to get the override values.
+  //
+  // Therefore, the `lookup()` function below first checks the `live_keys` array
+  // (the keyboard state array that has replaced the keymap cache). This should
+  // allow old code to continue working, until all the associated code (mostly
+  // the `onKeyswitchEvent()` handlers) is replaced, at which point we can
+  // remove dependence on `live_keys` entirely from this class.
+  //
+  // The `Runtime.lookupKey()` function replaces this one, for plugins that
+  // still want to do this same check.
+
+  DEPRECATED(LAYER_LOOKUP)
   static Key lookup(KeyAddr key_addr) {
     // First check the keyboard state array
     Key key = live_keys[key_addr];

src/kaleidoscope_internal/deprecations.h

@@ -38,3 +38,37 @@
 #define _DEPRECATED_MESSAGE_LAYER_EVENTHANDLER               __NL__ \
   "`Layer.eventHandler()` is deprecated.\n"                  __NL__ \
   "Please use `Layer.handleKeymapKeyswitchEvent()` instead."
+
+#define _DEPRECATED_MESSAGE_LAYER_LOOKUP                                      __NL__ \
+  "`Layer.lookup(key_addr)` is deprecated.\n"                                 __NL__ \
+  "Please use `Runtime.lookupKey(key_addr)` instead. Alternatively, if you\n" __NL__ \
+  "need to look up the current keymap entry without regard to current live\n" __NL__ \
+  "key state(s) (i.e. the `live_keys` array, which normally overrides the\n"  __NL__ \
+  "keymap), you can use `Layer.lookupOnActiveLayer(key_addr)`."
+
+#define _DEPRECATED_MESSAGE_HANDLE_KEYSWITCH_EVENT   __NL__ \
+  "`handleKeyswitchEvent()` has been deprecated.\n"  __NL__ \
+  "Please use `Runtime.handleKeyEvent()` instead."
+
+#define _DEPRECATED_MESSAGE_ON_KEYSWITCH_EVENT_V1                             __NL__ \
+  "The `onKeyswitchEvent()` event handler is deprecated.\n"                   __NL__ \
+  "Please replace it with an `onKeyEvent()` handler. See the documentation\n" __NL__ \
+  "in UPGRADING.md and docs/api-reference/event-handler-hooks.md for more\n"  __NL__ \
+  "information on what changes are needed to adapt old plugins to the new\n"  __NL__ \
+  "event handler API."
+
+#define _DEPRECATED_MESSAGE_BEFORE_REPORTING_STATE_V1                          __NL__ \
+  "This `beforeReportingState()` event handler version is deprecated.\n"       __NL__ \
+  "There is a new `beforeReportingState(KeyEvent)` handler that can be used\n" __NL__ \
+  "instead, for plugins that need to execute code before each new HID\n"       __NL__ \
+  "report is sent. However, the new handler does not run every cycle, but\n"   __NL__ \
+  "only in response to key events. If you have code that is intended to run\n" __NL__ \
+  "every scan cycle, it should be moved to the `afterEachCycle()` event\n"     __NL__ \
+  "handler instead."
+
+#define _DEPRECATED_MESSAGE_HID_KEYBOARD_PRESSKEY_TOGGLEDON                    __NL__ \
+  "The `Keyboard::pressKey(key, toggled_on)` function is deprecated.\n"        __NL__ \
+  "Please use `Keyboard::pressKey(key)` without the second argument\n"         __NL__ \
+  "instead. The version with two arguments handled rollover events, and\n"     __NL__ \
+  "this is now handled more completely by the event handling functions in\n"   __NL__ \
+  "`Runtime`."