Restructure MouseKeys to be aware of Macros virtual keys (#1114)

plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeys.cpp

@@ -37,6 +37,10 @@ uint16_t MouseKeys_::move_start_time_;
 uint16_t MouseKeys_::accel_start_time_;
 uint16_t MouseKeys_::wheel_start_time_;
 
+uint8_t MouseKeys_::directions_ = 0;
+uint8_t MouseKeys_::pending_directions_ = 0;
+uint8_t MouseKeys_::buttons_ = 0;
+
 // =============================================================================
 // Configuration functions
 
@@ -105,75 +109,12 @@ EventHandlerResult MouseKeys_::afterEachCycle() {
   }
 
   // Check timeout for position update interval.
-  bool update_position = Runtime.hasTimeExpired(move_start_time_, speedDelay);
-  if (update_position) {
-    move_start_time_ = Runtime.millisAtCycleStart();
-    // Determine which mouse movement directions are active by searching through
-    // all the currently active keys for mouse movement keys, and adding them to
-    // a bitfield (`directions`).
-    uint8_t directions = 0;
-    int8_t vx = 0;
-    int8_t vy = 0;
-    for (Key key : live_keys.all()) {
-      if (isMouseKey(key) && isMouseMoveKey(key)) {
-        directions |= key.getKeyCode();
-      }
-    }
-
-    if (directions == 0) {
-      // If there are no mouse movement keys held, reset speed to zero.
-      MouseWrapper.accelStep = 0;
-    } else {
-      // For each active direction, add the mouse movement speed.
-      if (directions & KEY_MOUSE_LEFT)
-        vx -= speed;
-      if (directions & KEY_MOUSE_RIGHT)
-        vx += speed;
-      if (directions & KEY_MOUSE_UP)
-        vy -= speed;
-      if (directions & KEY_MOUSE_DOWN)
-        vy += speed;
-
-      // Prepare the mouse report.
-      MouseWrapper.move(vx, vy);
-      // Send the report.
-      Runtime.hid().mouse().sendReport();
-    }
-  }
+  if (Runtime.hasTimeExpired(move_start_time_, speedDelay))
+    sendMouseMoveReport();
 
   // Check timeout for scroll report interval.
-  bool update_wheel = Runtime.hasTimeExpired(wheel_start_time_, wheelDelay);
-  if (update_wheel) {
-    wheel_start_time_ = Runtime.millisAtCycleStart();
-    // Determine which scroll wheel keys are active, and add their directions to
-    // a bitfield (`directions`).
-    uint8_t directions = 0;
-    int8_t vx = 0;
-    int8_t vy = 0;
-    for (Key key : live_keys.all()) {
-      if (isMouseKey(key) && isMouseWheelKey(key)) {
-        directions |= key.getKeyCode();
-      }
-    }
-
-    if (directions != 0) {
-      // Horizontal scroll wheel:
-      if (directions & KEY_MOUSE_LEFT)
-        vx -= wheelSpeed;
-      if (directions & KEY_MOUSE_RIGHT)
-        vx += wheelSpeed;
-      // Vertical scroll wheel (note coordinates are opposite movement):
-      if (directions & KEY_MOUSE_UP)
-        vy += wheelSpeed;
-      if (directions & KEY_MOUSE_DOWN)
-        vy -= wheelSpeed;
-
-      // Add scroll wheel changes to HID report.
-      Runtime.hid().mouse().move(0, 0, vy, vx);
-      // Send the report.
-      Runtime.hid().mouse().sendReport();
-    }
-  }
+  if (Runtime.hasTimeExpired(wheel_start_time_, wheelDelay))
+    sendMouseWheelReport();
 
   return EventHandlerResult::OK;
 }
@@ -183,50 +124,85 @@ EventHandlerResult MouseKeys_::onKeyEvent(KeyEvent &event) {
   if (!isMouseKey(event.key))
     return EventHandlerResult::OK;
 
+  pending_directions_ = 0;
+
   if (isMouseButtonKey(event.key)) {
-    sendMouseButtonReport(event);
+    // Clear button state; it will be repopulated by `onAddToReport()`, and the
+    // report will be sent by `afterReportingState()`.
+    buttons_ = 0;
 
   } else if (isMouseWarpKey(event.key)) {
     if (keyToggledOn(event.state)) {
       sendMouseWarpReport(event);
     }
+  }
+
+  // Reports for mouse cursor and wheel movement keys are sent from the
+  // `afterReportingState()` handler (when first toggled on) and
+  // `afterEachCycle()` handler (when held).  We need to return `OK` here so
+  // that processing of events for these keys will complete.
+  return EventHandlerResult::OK;
+}
 
-  } else if (isMouseMoveKey(event.key)) {
-    // No report is sent here; that's handled in `afterEachCycle()`.
-    move_start_time_ = Runtime.millisAtCycleStart() - speedDelay;
+// -----------------------------------------------------------------------------
+EventHandlerResult MouseKeys_::afterReportingState(const KeyEvent &event) {
+  if (!isMouseKey(event.key))
+    return EventHandlerResult::OK;
+
+  // If a mouse button key has toggled on or off, we send a mouse report with
+  // the updated information.
+  if (isMouseButtonKey(event.key)) {
+    sendMouseButtonReport();
+  }
+
+  // A mouse key event has been successfully registered, and we have now
+  // gathered all the information on held mouse movement and wheel keys, so it's
+  // safe to update the direction information.
+  directions_ = pending_directions_;
+  pending_directions_ = 0;
+
+  if (isMouseMoveKey(event.key)) {
+    // When a cursor movement key toggles on, set the acceleration start time in
+    // order to get consistent behavior.
     accel_start_time_ = Runtime.millisAtCycleStart();
+    sendMouseMoveReport();
 
   } else if (isMouseWheelKey(event.key)) {
-    // No report is sent here; that's handled in `afterEachCycle()`.
-    wheel_start_time_ = Runtime.millisAtCycleStart() - wheelDelay;
+    sendMouseWheelReport();
   }
 
-  return EventHandlerResult::EVENT_CONSUMED;
+  return EventHandlerResult::OK;
+}
+
+// -----------------------------------------------------------------------------
+// This handler is responsible for gathering information on which mouse cursor
+// and wheel movement keys are currently pressed, so the direction(s) of motion
+// will be up to date at the end of processing a mouse key event.  We add bits
+// to the `pending` directions only; these get copied later if the event isn't
+// aborted.
+EventHandlerResult MouseKeys_::onAddToReport(Key key) {
+  if (!isMouseKey(key))
+    return EventHandlerResult::OK;
+
+  if (isMouseButtonKey(key))
+    buttons_ |= (key.getKeyCode() & ~KEY_MOUSE_BUTTON);
+
+  if (isMouseMoveKey(key))
+    pending_directions_ |= key.getKeyCode();
+
+  if (isMouseWheelKey(key))
+    pending_directions_ |= (key.getKeyCode() << wheel_offset_);
+
+  return EventHandlerResult::OK;
 }
 
 // =============================================================================
 // HID report helper functions
 
 // -----------------------------------------------------------------------------
-void MouseKeys_::sendMouseButtonReport(const KeyEvent &event) const {
-  // Get ready to send a new mouse report by building it from live_keys. Note
-  // that this also clears the movement and scroll values, but since those are
-  // relative, that's what we want.
+void MouseKeys_::sendMouseButtonReport() const {
   Runtime.hid().mouse().releaseAllButtons();
-
-  uint8_t buttons = 0;
-  for (KeyAddr key_addr : KeyAddr::all()) {
-    if (key_addr == event.addr)
-      continue;
-    Key key = live_keys[key_addr];
-    if (isMouseKey(key) && isMouseButtonKey(key)) {
-      buttons |= key.getKeyCode();
-    }
-  }
-  if (keyToggledOn(event.state))
-    buttons |= event.key.getKeyCode();
-  buttons &= ~KEY_MOUSE_BUTTON;
-  Runtime.hid().mouse().pressButtons(buttons);
+  Runtime.hid().mouse().pressButtons(buttons_);
   Runtime.hid().mouse().sendReport();
 }
 
@@ -240,6 +216,62 @@ void MouseKeys_::sendMouseWarpReport(const KeyEvent &event) const {
     ((event.key.getKeyCode() & KEY_MOUSE_RIGHT) ? WARP_RIGHT : 0x00));
 }
 
+// -----------------------------------------------------------------------------
+void MouseKeys_::sendMouseMoveReport() {
+  move_start_time_ = Runtime.millisAtCycleStart();
+
+  int8_t vx = 0;
+  int8_t vy = 0;
+  uint8_t direction = directions_ & move_mask_;
+
+  if (direction == 0) {
+    // If there are no mouse movement keys held, reset speed to zero.
+    MouseWrapper.accelStep = 0;
+  } else {
+    // For each active direction, add the mouse movement speed.
+    if (direction & KEY_MOUSE_LEFT)
+      vx -= speed;
+    if (direction & KEY_MOUSE_RIGHT)
+      vx += speed;
+    if (direction & KEY_MOUSE_UP)
+      vy -= speed;
+    if (direction & KEY_MOUSE_DOWN)
+      vy += speed;
+
+    // Prepare the mouse report.
+    MouseWrapper.move(vx, vy);
+    // Send the report.
+    Runtime.hid().mouse().sendReport();
+  }
+}
+
+// -----------------------------------------------------------------------------
+void MouseKeys_::sendMouseWheelReport() {
+  wheel_start_time_ = Runtime.millisAtCycleStart();
+
+  int8_t vx = 0;
+  int8_t vy = 0;
+  uint8_t direction = directions_ >> wheel_offset_;
+
+  if (direction != 0) {
+    // Horizontal scroll wheel:
+    if (direction & KEY_MOUSE_LEFT)
+      vx -= wheelSpeed;
+    if (direction & KEY_MOUSE_RIGHT)
+      vx += wheelSpeed;
+    // Vertical scroll wheel (note coordinates are opposite movement):
+    if (direction & KEY_MOUSE_UP)
+      vy += wheelSpeed;
+    if (direction & KEY_MOUSE_DOWN)
+      vy -= wheelSpeed;
+
+    // Add scroll wheel changes to HID report.
+    Runtime.hid().mouse().move(0, 0, vy, vx);
+    // Send the report.
+    Runtime.hid().mouse().sendReport();
+  }
+}
+
 } // namespace plugin
 } // namespace kaleidoscope
 

plugins/Kaleidoscope-MouseKeys/src/kaleidoscope/plugin/MouseKeys.h

@@ -41,20 +41,34 @@ class MouseKeys_ : public kaleidoscope::Plugin {
   EventHandlerResult onNameQuery();
   EventHandlerResult afterEachCycle();
   EventHandlerResult onKeyEvent(KeyEvent &event);
+  EventHandlerResult onAddToReport(Key key);
+  EventHandlerResult afterReportingState(const KeyEvent &event);
 
  private:
   static uint16_t move_start_time_;
   static uint16_t accel_start_time_;
   static uint16_t wheel_start_time_;
 
+  // Mouse cursor and wheel movement directions are stored in a single bitfield
+  // to save space.  The low four bits are for cursor movement, and the high
+  // four are for wheel movement.
+  static constexpr uint8_t wheel_offset_ = 4;
+  static constexpr uint8_t wheel_mask_ = 0b11110000;
+  static constexpr uint8_t move_mask_  = 0b00001111;
+  static uint8_t directions_;
+  static uint8_t pending_directions_;
+  static uint8_t buttons_;
+
   bool isMouseKey(const Key &key) const;
   bool isMouseButtonKey(const Key &key) const;
   bool isMouseMoveKey(const Key &key) const;
   bool isMouseWarpKey(const Key &key) const;
   bool isMouseWheelKey(const Key &key) const;
 
-  void sendMouseButtonReport(const KeyEvent &event) const;
+  void sendMouseButtonReport() const;
   void sendMouseWarpReport(const KeyEvent &event) const;
+  void sendMouseMoveReport();
+  void sendMouseWheelReport();
 
 };
 }

tests/issues/1113/1113.ino

@@ -0,0 +1,67 @@
+/* -*- mode: c++ -*-
+ * Copyright (C) 2022  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
+ * Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <Kaleidoscope.h>
+#include <Kaleidoscope-Macros.h>
+#include <Kaleidoscope-MouseKeys.h>
+
+// *INDENT-OFF*
+KEYMAPS(
+    [0] = KEYMAP_STACKED
+    (
+        Key_mouseUp, Key_mouseDn, Key_mouseL, Key_mouseR, ___, ___, ___,
+        Key_mouseScrollUp, Key_mouseScrollDn, Key_mouseScrollL, Key_mouseScrollR, ___, ___, ___,
+        Key_mouseBtnL, Key_mouseBtnM, Key_mouseBtnR, ___, ___, ___,
+        M(0), M(1), ___, ___, ___, ___, ___,
+        ___, ___, ___, ___,
+        ___,
+
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___,
+        ___
+    ),
+)
+// *INDENT-ON*
+
+const macro_t *macroAction(uint8_t macro_id, KeyEvent &event) {
+  if (keyToggledOn(event.state)) {
+    switch (macro_id) {
+    case 0:
+      Macros.tap(Key_mouseBtnR);
+      break;
+    case 1:
+      Macros.press(Key_mouseScrollUp);
+      Macros.release(Key_mouseScrollUp);
+      break;
+    default:
+      break;
+    }
+  }
+  return MACRO_NONE;
+}
+
+KALEIDOSCOPE_INIT_PLUGINS(Macros, MouseKeys);
+
+void setup() {
+  Kaleidoscope.setup();
+}
+
+void loop() {
+  Kaleidoscope.loop();
+}

tests/issues/1113/sketch.json

@@ -0,0 +1,6 @@
+{
+  "cpu": {
+    "fqbn": "keyboardio:virtual:model01",
+    "port": ""
+  }
+}

tests/issues/1113/test.ktest

@@ -0,0 +1,49 @@
+VERSION 1
+
+KEYSWITCH MOVE_UP       0  0
+KEYSWITCH MOVE_DOWN     0  1
+KEYSWITCH MOVE_LEFT     0  2
+KEYSWITCH MOVE_RIGHT    0  3
+
+KEYSWITCH SCROLL_UP     1  0
+KEYSWITCH SCROLL_DOWN   1  1
+KEYSWITCH SCROLL_LEFT   1  2
+KEYSWITCH SCROLL_RIGHT  1  3
+
+KEYSWITCH BUTTON_L      2  0
+KEYSWITCH BUTTON_M      2  1
+KEYSWITCH BUTTON_R      2  2
+
+KEYSWITCH MACRO_0       3  0
+KEYSWITCH MACRO_1       3  1
+
+# ==============================================================================
+NAME Mouse button key tap in Macros
+
+RUN 3 ms
+PRESS MACRO_0
+RUN 1 cycle
+EXPECT mouse-report button=R
+EXPECT mouse-report empty
+
+RUN 5 ms
+RELEASE MACRO_0
+RUN 1 cycle
+EXPECT no mouse-report
+
+RUN 5 ms
+
+# ==============================================================================
+NAME Mouse scroll key tap in Macros
+
+RUN 4 ms
+PRESS MACRO_1
+RUN 1 cycle
+EXPECT mouse-report v=1
+
+RUN 5 ms
+RELEASE MACRO_1
+RUN 1 cycle
+EXPECT no mouse-report
+
+RUN 5 ms