Merge pull request #1184 from gedankenexperimenter/fix-turbo-sticky

Fix Turbo's "sticky" mode

plugins/Kaleidoscope-Turbo/README.md

@@ -24,7 +24,7 @@ void setup() {
     Kaleidoscope.setup();
 
     Turbo.interval(30);
-    Turbo.toggle(true);
+    Turbo.sticky(true);
     Turbo.flash(true);
     Turbo.flashInterval(80);
     Turbo.activeColor(CRGB(0x64, 0x96, 0xed));
@@ -50,7 +50,6 @@ The `Turbo` object has the following user-configurable properties:
 
 ### `.sticky([bool])`
 > This method makes the Turbo functionality sticky, so it remains in effect not only while
->
 > it is held, but after it is released too, until it is toggled off with another tap. Without
 > arguments, the method enables the sticky functionality. Passing a boolean argument
 > sets stickiness to the given value.

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

@@ -1,6 +1,7 @@
 /* -*- mode: c++ -*-
  * Kaleidoscope-Turbo
  * Copyright (C) 2018 ash lea
+ * 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
@@ -82,22 +83,37 @@ void Turbo::activeColor(cRGB newVal) {
 }
 
 EventHandlerResult Turbo::onKeyEvent(KeyEvent &event) {
+  // If any key toggles off, reset its LED to normal.
   if (active_ && flash_ && keyToggledOff(event.state)) {
     if (event.key.isKeyboardKey())
       LEDControl::refreshAt(event.addr);
   }
 
+  // Ignore any non-Turbo key events.
   if (event.key != Key_Turbo)
     return EventHandlerResult::OK;
 
-  if (keyToggledOn(event.state)) {
-    active_     = true;
-    start_time_ = Runtime.millisAtCycleStart() - interval_;
-  } else {
+
+  if (active_) {
+    // If Turbo is active, and in "sticky" mode, we abort the event when a Turbo
+    // key toggles off, leaving it in the active keys array.  This means that a
+    // layer change won't hide an active Turbo key.
+    if (sticky_ && keyToggledOff(event.state)) {
+      return EventHandlerResult::ABORT;
+    }
+    // If not in "sticky" mode and a Turbo key toggles off, or if in "sticky"
+    // mode and a Turbo key toggles on, we deactivate Turbo.
     active_ = false;
     if (flash_)
       LEDControl::refreshAll();
+
+  } else if (keyToggledOn(event.state)) {
+    // If Turbo is inactive, turn it on when a Turbo key is pressed.
+    active_     = true;
+    start_time_ = Runtime.millisAtCycleStart() - interval_;
   }
+
+  // We assume that other plugins don't need to know about Turbo key events.
   return EventHandlerResult::EVENT_CONSUMED;
 }
 
@@ -113,16 +129,9 @@ EventHandlerResult Turbo::afterEachCycle() {
       // Send the empty report to register the release of all the held keys.
       Runtime.hid().keyboard().sendReport();
 
-      // Just in case the Turbo key has been wiped from `live_keys[]` without
-      // `onKeyEvent()` being called with a toggle-off:
-      active_ = false;
-
       // Go through the `live_keys[]` array and add any Keyboard HID keys to the
       // new report.
       for (Key key : live_keys.all()) {
-        if (key == Key_Turbo) {
-          active_ = true;
-        }
         if (key.isKeyboardKey()) {
           Runtime.addToReport(key);
         }

tests/plugins/Turbo/sticky/sketch.json

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

tests/plugins/Turbo/sticky/sticky.ino

@@ -0,0 +1,52 @@
+/* -*- 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-Turbo.h>
+
+// *INDENT-OFF*
+KEYMAPS(
+    [0] = KEYMAP_STACKED
+    (
+        Key_Turbo, Key_RightGui, Key_LeftShift, ___, ___, ___, ___,
+        Key_A, Key_B, Key_C, Key_D, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___,
+        ___,
+
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___, ___, ___, ___,
+        ___, ___, ___, ___,
+        ___
+    ),
+)
+// *INDENT-ON*
+
+KALEIDOSCOPE_INIT_PLUGINS(Turbo);
+
+void setup() {
+  Kaleidoscope.setup();
+
+  Turbo.interval(20);
+  Turbo.sticky(true);
+}
+
+void loop() {
+  Kaleidoscope.loop();
+}

tests/plugins/Turbo/sticky/test.ktest

@@ -0,0 +1,138 @@
+VERSION 1
+
+KEYSWITCH TURBO    0 0
+KEYSWITCH R_GUI    0 1
+KEYSWITCH L_SHIFT  0 2
+KEYSWITCH A        1 0
+KEYSWITCH B        1 1
+KEYSWITCH C        1 2
+KEYSWITCH D        1 3
+
+# ==============================================================================
+NAME Turbo no regression
+
+RUN 4 ms
+PRESS A
+RUN 1 cycle
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 4 ms
+RELEASE A
+RUN 1 cycle
+EXPECT keyboard-report empty # report should be empty
+
+RUN 5 ms
+EXPECT no keyboard-report # expect no more reports
+
+# ==============================================================================
+NAME Turbo second
+
+RUN 4 ms
+PRESS A
+RUN 1 cycle
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 4 ms
+PRESS TURBO
+RUN 1 cycle
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 20 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 10 ms
+RELEASE TURBO
+RUN 10 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 20 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 20 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 9 ms
+RELEASE A
+RUN 1 cycle
+EXPECT keyboard-report empty # report should be empty
+
+RUN 20 ms
+EXPECT no keyboard-report
+
+RUN 4 ms
+PRESS B
+RUN 1 cycle
+EXPECT keyboard-report Key_B # report should contain `B` (0x05)
+
+RUN 5 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_B # report should contain `B` (0x05)
+
+RUN 20 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_B # report should contain `B` (0x05)
+
+RUN 4 ms
+PRESS TURBO
+RUN 1 cycle
+
+RUN 4 ms
+RELEASE TURBO
+RUN 1 cycle
+
+RUN 20 ms
+RELEASE B
+RUN 1 cycle
+EXPECT keyboard-report empty # report should be empty
+
+RUN 5 ms
+EXPECT no keyboard-report # expect no more reports
+
+# ==============================================================================
+NAME Turbo first
+
+RUN 4 ms
+PRESS TURBO
+RUN 1 cycle
+
+RUN 4 ms
+RELEASE TURBO
+RUN 1 cycle
+
+RUN 5 ms
+
+RUN 4 ms
+PRESS A
+RUN 1 cycle
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 5 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 20 ms
+EXPECT keyboard-report empty # report should be empty
+EXPECT keyboard-report Key_A # report should contain `A` (0x04)
+
+RUN 4 ms
+RELEASE A
+RUN 1 cycle
+EXPECT keyboard-report empty # report should be empty
+
+RUN 25 ms
+
+RUN 4 ms
+PRESS TURBO
+RUN 1 cycle
+
+RUN 4 ms
+RELEASE TURBO
+RUN 1 cycle
+
+RUN 5 ms
+EXPECT no keyboard-report # expect no more reports