Merge pull request #971 from gedankenexperimenter/b/qukeys.issue-970

Fix Qukeys minimum prior interval error

src/kaleidoscope/plugin/Qukeys.cpp

@@ -118,6 +118,14 @@ EventHandlerResult Qukeys::beforeReportingState() {
     }
   }
 
+  // Next, if there hasn't been a keypress in a while, update the prior keypress
+  // timestamp to avoid integer overflow issues:
+  if (Runtime.hasTimeExpired(prior_keypress_timestamp_,
+                             minimum_prior_interval_)) {
+    prior_keypress_timestamp_ =
+      Runtime.millisAtCycleStart() - (minimum_prior_interval_ + 1);
+  }
+
   // If any events get flushed from the queue, stop there; we can only safely
   // send the one report per cycle.
   if (processQueue()) {
@@ -134,14 +142,6 @@ EventHandlerResult Qukeys::beforeReportingState() {
                     queue_head_.primary_key : queue_head_.alternate_key;
     flushEvent(event_key);
   }
-
-  // Last, if there hasn't been a keypress in a while, update the prior keypress
-  // timestamp to avoid integer overflow issues:
-  if (Runtime.hasTimeExpired(prior_keypress_timestamp_,
-                             minimum_prior_interval_)) {
-    prior_keypress_timestamp_ =
-      Runtime.millisAtCycleStart() - (minimum_prior_interval_ + 1);
-  }
   return EventHandlerResult::OK;
 }
 

src/kaleidoscope/plugin/Qukeys.h

@@ -202,7 +202,10 @@ class Qukeys : public kaleidoscope::Plugin {
   uint8_t minimum_prior_interval_{75};
 
   // Timestamp of the keypress event immediately prior to the queue head event.
-  uint16_t prior_keypress_timestamp_{0};
+  // The initial value is 256 to ensure that it won't trigger an error if a
+  // qukey is pressed before `minimum_prior_interval_` milliseconds after the
+  // keyboard powers on, and that value can only be as high as 255.
+  uint16_t prior_keypress_timestamp_{256};
 
   // This is a guard against re-processing events when qukeys flushes them from
   // its event queue. We can't just use an "injected" key state flag, because

tests/issues/970/common.h

@@ -0,0 +1,32 @@
+// -*- mode: c++ -*-
+
+/* Kaleidoscope - Firmware for computer input devices
+ * Copyright (C) 2020  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/>.
+ */
+
+#pragma once
+
+#include <cstdint>
+
+namespace kaleidoscope {
+namespace testing {
+
+constexpr uint16_t QUKEYS_HOLD_TIMEOUT = 200;
+constexpr uint8_t QUKEYS_OVERLAP_THRESHOLD = 90;
+constexpr uint8_t QUKEYS_MINIMUM_HOLD_TIME = 10;
+constexpr uint8_t QUKEYS_MIN_PRIOR_INTERVAL = 20;
+
+}
+}

tests/issues/970/sketch.ino

@@ -0,0 +1,99 @@
+/* -*- mode: c++ -*-
+ * Copyright (C) 2020  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-Qukeys.h>
+#include <Kaleidoscope-Macros.h>
+
+#include "./common.h"
+
+enum { MACRO_TOGGLE_QUKEYS };
+
+// *INDENT-OFF*
+KEYMAPS(
+  [0] = KEYMAP_STACKED
+  (
+      Key_NoKey,    Key_1, Key_2, Key_3, Key_4, Key_5, Key_NoKey,
+      Key_Backtick, Key_Q, Key_W, Key_E, Key_R, Key_T, Key_Tab,
+      Key_PageUp,   Key_A, Key_S, Key_D, Key_F, Key_G,
+      Key_PageDown, Key_Z, Key_X, Key_C, Key_V, Key_B, Key_Escape,
+
+      Key_LeftControl, Key_Backspace, Key_LeftGui, Key_LeftShift,
+      Key_Q,
+
+      M(MACRO_TOGGLE_QUKEYS), Key_6, Key_7,    Key_8,     Key_9,      Key_0,            Key_skip,
+      Key_Enter,              Key_Y, Key_U,    Key_I,     Key_O,      Key_P,            Key_Equals,
+                              Key_H, SFT_T(J), CTL_T(K),  ALT_T(L),   GUI_T(Semicolon), Key_Quote,
+      Key_skip,               Key_N, Key_M,    Key_Comma, Key_Period, Key_Slash,        Key_Minus,
+
+      Key_RightShift, Key_RightAlt, Key_Spacebar, Key_RightControl,
+      LT(1,E)
+   ),
+  [1] = KEYMAP_STACKED
+  (
+      ___,   Key_B, Key_C, Key_D, Key_E, Key_F, Key_G,
+      Key_A, Key_B, Key_C, Key_D, Key_E, Key_F, Key_G,
+      Key_A, Key_B, Key_C, Key_D, Key_E, Key_F,
+      Key_A, Key_B, Key_C, Key_D, Key_E, Key_F, Key_G,
+
+      Key_1, Key_2, Key_3, Key_4,
+      ___,
+
+
+      ___,   Key_B, Key_C, Key_D, Key_E, Key_F, Key_G,
+      Key_A, Key_B, Key_C, Key_D, Key_E, Key_F, Key_G,
+      Key_A, Key_B, Key_C, Key_D, Key_E, Key_F,
+      Key_A, Key_B, Key_C, Key_D, Key_E, Key_F, Key_G,
+
+      Key_1, Key_2, Key_3, Key_4,
+      ___
+   ),
+)
+// *INDENT-ON*
+
+// Defining a macro (on the "any" key: see above) to toggle Qukeys on and off
+const macro_t *macroAction(uint8_t macro_index, uint8_t key_state) {
+  switch (macro_index) {
+  case MACRO_TOGGLE_QUKEYS:
+    if (keyToggledOn(key_state))
+      Qukeys.toggle();
+    break;
+  }
+  return MACRO_NONE;
+}
+
+// Use Qukeys
+KALEIDOSCOPE_INIT_PLUGINS(Qukeys, Macros);
+
+void setup() {
+  QUKEYS(
+    kaleidoscope::plugin::Qukey(0, KeyAddr(2, 1), Key_LeftGui),      // A/cmd
+    kaleidoscope::plugin::Qukey(0, KeyAddr(2, 2), Key_LeftAlt),      // S/alt
+    kaleidoscope::plugin::Qukey(0, KeyAddr(2, 3), Key_LeftControl),  // D/ctrl
+    kaleidoscope::plugin::Qukey(0, KeyAddr(2, 4), Key_LeftShift),    // F/shift
+    kaleidoscope::plugin::Qukey(0, KeyAddr(3, 6), ShiftToLayer(1))   // Q/layer-shift (on `fn`)
+  )
+  Qukeys.setHoldTimeout(kaleidoscope::testing::QUKEYS_HOLD_TIMEOUT);
+  Qukeys.setOverlapThreshold(kaleidoscope::testing::QUKEYS_OVERLAP_THRESHOLD);
+  Qukeys.setMinimumHoldTime(kaleidoscope::testing::QUKEYS_MINIMUM_HOLD_TIME);
+  Qukeys.setMinimumPriorInterval(kaleidoscope::testing::QUKEYS_MIN_PRIOR_INTERVAL);
+
+  Kaleidoscope.setup();
+}
+
+void loop() {
+  Kaleidoscope.loop();
+}

tests/issues/970/test.ktest

@@ -0,0 +1,25 @@
+NAME Issue 970 Qukeys min prior interval overflow
+
+KEYSWITCH A 2 1
+
+PRESS A
+RUN 10 ms
+
+RELEASE A
+RUN 1 cycle
+EXPECT keyboard-report Key_A # Report should contain only `A`
+RUN 1 cycle
+EXPECT keyboard-report empty # Report should be empty
+
+RUN 65536 ms
+
+PRESS A
+RUN 201 ms
+EXPECT keyboard-report Key_LeftGui # Report should contain only `LeftGui`
+
+RUN 10 ms
+
+RELEASE A
+# I'm not sure why it takes 2 cycles before the report is sent
+RUN 2 cycles
+EXPECT keyboard-report empty # Report should be empty

tests/plugins/Qukeys/basic/common.h

@@ -26,6 +26,7 @@ namespace testing {
 constexpr uint16_t QUKEYS_HOLD_TIMEOUT = 200;
 constexpr uint8_t QUKEYS_OVERLAP_THRESHOLD = 90;
 constexpr uint8_t QUKEYS_MINIMUM_HOLD_TIME = 10;
+constexpr uint8_t QUKEYS_MIN_PRIOR_INTERVAL = 20;
 
 }
 }

tests/plugins/Qukeys/basic/sketch.ino

@@ -89,6 +89,7 @@ void setup() {
   Qukeys.setHoldTimeout(kaleidoscope::testing::QUKEYS_HOLD_TIMEOUT);
   Qukeys.setOverlapThreshold(kaleidoscope::testing::QUKEYS_OVERLAP_THRESHOLD);
   Qukeys.setMinimumHoldTime(kaleidoscope::testing::QUKEYS_MINIMUM_HOLD_TIME);
+  Qukeys.setMinimumPriorInterval(kaleidoscope::testing::QUKEYS_MIN_PRIOR_INTERVAL);
 
   Kaleidoscope.setup();
 }

tests/plugins/Qukeys/basic/test/testcase.cpp

@@ -39,9 +39,6 @@ class QukeysBasic : public VirtualDeviceTest {
 };
 
 TEST_F(QukeysBasic, TapQukeyAlone) {
-  // XXX Temporary workaround
-  sim_.RunForMillis(1000);
-
   // Press `A`
   sim_.Press(key_addr_A);
 
@@ -74,8 +71,8 @@ TEST_F(QukeysBasic, TapQukeyAlone) {
 }
 
 TEST_F(QukeysBasic, HoldQukeyAlone) {
-  // XXX Temporary workaround
-  sim_.RunForMillis(1000);
+  // Prevent rapid typing suppression from affecting the test
+  sim_.RunForMillis(QUKEYS_MIN_PRIOR_INTERVAL);
 
   // Press `A`
   sim_.Press(key_addr_A);
@@ -125,8 +122,8 @@ TEST_F(QukeysBasic, HoldQukeyAlone) {
 }
 
 TEST_F(QukeysBasic, FullOverlap) {
-  // XXX Temporary workaround
-  sim_.RunForMillis(1000);
+  // Prevent rapid typing suppression from affecting the test
+  sim_.RunForMillis(QUKEYS_MIN_PRIOR_INTERVAL);
 
   sim_.Press(key_addr_F);
   sim_.RunForMillis(20);
@@ -172,8 +169,8 @@ TEST_F(QukeysBasic, FullOverlap) {
 }
 
 TEST_F(QukeysBasic, RolloverPrimary) {
-  // XXX Temporary workaround
-  sim_.RunForMillis(1000);
+  // Prevent rapid typing suppression from affecting the test
+  sim_.RunForMillis(QUKEYS_MIN_PRIOR_INTERVAL);
 
   sim_.Press(key_addr_F);
   sim_.RunForMillis(20);