Merge pull request #1075 from gedankenexperimenter/issue-1074

Fix AutoShift queue overflow problem

examples/Keystrokes/AutoShift/AutoShift.ino

@@ -46,6 +46,9 @@ const macro_t *macroAction(uint8_t macro_id, KeyEvent &event) {
   return MACRO_NONE;
 }
 
+// This sketch uses the AutoShiftConfig plugin, which enables run-time
+// configuration of AutoShift configuration settings.  All of the plugins marked
+// "for AutoShiftConfig" are optional; AutoShift itself will work without them.
 KALEIDOSCOPE_INIT_PLUGINS(
   EEPROMSettings,        // for AutoShiftConfig
   EEPROMKeymap,          // for AutoShiftConfig

plugins/Kaleidoscope-AutoShift/README.md

@@ -24,8 +24,8 @@ the output will be unshifted.
 
 The `AutoShift` object provides three methods for turning itself on and off:
 
-- To turn the plugin off, call `AutoShift.enable()`.
+- To turn the plugin on, call `AutoShift.enable()`.
-- To turn the plugin on, call `AutoShift.disable()`.
+- To turn the plugin off, call `AutoShift.disable()`.
 - To toggle the plugin's state, call `AutoShift.toggle()`.
 
 Note: Disabling the AutoShift plugin does not affect which `Key` categories it
@@ -78,6 +78,20 @@ As you can see, this method takes a `Key` as its input and returns either `true`
 (for keys eligible to be auto-shifted) or `false` (for keys AutoShift will leave
 alone).
 
+## Plugin compatibility
+
+If you're using AutoShift in a sketch that also includes the Qukeys and/or
+SpaceCadet plugins, make sure to register AutoShift after those other plugins in
+order to prevent auto-shifts from getting clobbered.  The recommended order is
+as follows:
+
+```c++
+KALEIDOSCOPE_INIT_PLUGINS(Qukeys, SpaceCadet, AutoShift)
+```
+
+It's not generally recommended to use AutoShift on the same key(s) handled by
+either Qukeys or SpaceCadet, as this can result in confusing behaviour.
+
 ## Further reading
 
 Starting from the [example][plugin:example] is the recommended way of getting

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

@@ -137,19 +137,26 @@ EventHandlerResult AutoShift::onKeyswitchEvent(KeyEvent &event) {
 
   if (!queue_.isEmpty()) {
     // There's an unresolved AutoShift key press.
-    if (queue_.isFull()) {
+    if (keyToggledOn(event.state) ||
-      flushQueue();
+        event.addr == queue_.addr(0) ||
-    } else if (event.addr == queue_.addr(0)) {
+        queue_.isFull()) {
+      // If a new key toggled on, the unresolved key toggled off (it was a
+      // "tap"), or if the queue is full, we clear the queue, and the key event
+      // does not get modified.
       flushEvent(false);
       flushQueue();
+    } else {
+      // Otherwise, add the release event to the queue.  We do this so that
+      // rollover from a modifier to an auto-shifted key will result in the
+      // modifier being applied to the key.
+      queue_.append(event);
+      return EventHandlerResult::ABORT;
     }
-    if (queue_.isEmpty())
-      return EventHandlerResult::OK;
-    queue_.append(event);
-    return EventHandlerResult::ABORT;
   }
 
   if (keyToggledOn(event.state) && isAutoShiftable(event.key)) {
+    // The key is eligible to be auto-shifted, so we add it to the queue and
+    // defer processing of the event.
     queue_.append(event);
     return EventHandlerResult::ABORT;
   }

tests/issues/1074/1074.ino

@@ -0,0 +1,50 @@
+/* -*- mode: c++ -*-
+ * Copyright (C) 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
+ * 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-AutoShift.h>
+
+// *INDENT-OFF*
+KEYMAPS(
+    [0] = KEYMAP_STACKED
+    (
+        Key_LeftShift, Key_RightShift, ___, ___, ___, ___, ___,
+        Key_A, Key_B, Key_C, Key_D, Key_E, ___, ___,
+        ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___,
+        ___,
+
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___,
+        ___
+    ),
+)
+// *INDENT-ON*
+
+KALEIDOSCOPE_INIT_PLUGINS(AutoShift);
+
+void setup() {
+  Kaleidoscope.setup();
+  AutoShift.setTimeout(20);
+}
+
+void loop() {
+  Kaleidoscope.loop();
+}

tests/issues/1074/sketch.json

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

tests/issues/1074/test.ktest

@@ -0,0 +1,54 @@
+VERSION 1
+
+KEYSWITCH LSHIFT    0 0
+KEYSWITCH RSHIFT    0 1
+KEYSWITCH A         1 0
+KEYSWITCH B         1 1
+KEYSWITCH C         1 2
+KEYSWITCH D         1 3
+KEYSWITCH E         1 4
+
+# ==============================================================================
+NAME AutoShift overflow
+
+RUN 4 ms
+PRESS A
+RUN 1 cycle
+
+RUN 4 ms
+PRESS B
+RUN 1 cycle
+EXPECT keyboard-report Key_A # report: { 4 }
+
+RUN 4 ms
+PRESS C
+RUN 1 cycle
+EXPECT keyboard-report Key_A Key_B # report: { 4 5 }
+
+RUN 4 ms
+PRESS D
+RUN 1 cycle
+EXPECT keyboard-report Key_A Key_B Key_C # report: { 4 5 6 }
+
+RUN 4 ms
+RELEASE A
+RUN 1 cycle
+
+RUN 4 ms
+RELEASE B
+RUN 1 cycle
+
+RUN 4 ms
+RELEASE C
+RUN 1 cycle
+
+RUN 4 ms
+RELEASE D
+RUN 1 cycle
+EXPECT keyboard-report Key_A Key_B Key_C Key_D # report: { 4 5 6 7 }
+EXPECT keyboard-report Key_B Key_C Key_D # report: { 5 6 7 }
+EXPECT keyboard-report Key_C Key_D # report: { 6 7 }
+EXPECT keyboard-report Key_D # report: { 7 }
+EXPECT keyboard-report empty
+
+RUN 5 ms