Merge pull request #505 from gedankenexperimenter/bug/qukeys-layer-shift-rollover

Fix Qukeys layer-shift bugs

src/kaleidoscope/key_events.cpp

@@ -81,8 +81,13 @@ void handleKeyswitchEvent(Key mappedKey, byte row, byte col, uint8_t keyState) {
 
     /* If a key had an on event, we update the live composite keymap. See
      * layers.h for an explanation about the different caches we have. */
-    if (keyToggledOn(keyState))
+    if (keyToggledOn(keyState)) {
+      if (mappedKey.raw == Key_NoKey.raw || keyState & EPHEMERAL) {
         Layer.updateLiveCompositeKeymap(row, col);
+      } else {
+        Layer.updateLiveCompositeKeymap(row, col, mappedKey);
+      }
+    }
 
     /* If the key we are dealing with is masked, ignore it until it is released.
      * When releasing it, clear the mask, so future key events can be handled

src/kaleidoscope/keyswitch_state.h

@@ -20,6 +20,7 @@
 #include <Arduino.h>
 
 #define INJECTED    B10000000
+#define EPHEMERAL   B01000000
 #define IS_PRESSED  B00000010
 #define WAS_PRESSED B00000001
 

src/kaleidoscope/layers.h

@@ -99,6 +99,9 @@ class Layer_ {
   static Key getKeyFromPROGMEM(uint8_t layer, byte row, byte col);
 
   static void updateLiveCompositeKeymap(byte row, byte col);
+  static void updateLiveCompositeKeymap(byte row, byte col, Key mappedKey) {
+    live_composite_keymap_[row][col] = mappedKey;
+  }
   static void updateActiveLayers(void);
 
  private:

src/kaleidoscope/plugin/OneShot.cpp

@@ -51,11 +51,6 @@ bool OneShot::isStickable(Key key) {
   return state_[key.raw - ranges::OS_FIRST].stickable;
 }
 
-void OneShot::positionToCoords(uint8_t pos, byte *row, byte *col) {
-  *col = pos % COLS;
-  *row = (pos - *col) / COLS;
-}
-
 // ---- OneShot stuff ----
 void OneShot::injectNormalKey(uint8_t idx, uint8_t key_state) {
   Key key;
@@ -69,8 +64,7 @@ void OneShot::injectNormalKey(uint8_t idx, uint8_t key_state) {
     key.keyCode = LAYER_SHIFT_OFFSET + idx - 8;
   }
 
-  positionToCoords(state_[idx].position, &row, &col);
+  handleKeyswitchEvent(key, UNKNOWN_KEYSWITCH_LOCATION, key_state | INJECTED);
-  handleKeyswitchEvent(key, row, col, key_state | INJECTED);
 }
 
 void OneShot::activateOneShot(uint8_t idx) {

src/kaleidoscope/plugin/OneShot.h

@@ -95,8 +95,6 @@ class OneShot : public kaleidoscope::Plugin {
   static bool should_cancel_;
   static bool should_cancel_stickies_;
 
-  static void positionToCoords(uint8_t pos, byte *row, byte *col);
-
   static void injectNormalKey(uint8_t idx, uint8_t key_state);
   static void activateOneShot(uint8_t idx);
   static void cancelOneShot(uint8_t idx);

src/kaleidoscope/plugin/Qukeys.cpp

@@ -88,8 +88,9 @@ uint16_t Qukeys::time_limit_ = 250;
 uint8_t Qukeys::release_delay_ = 0;
 QueueItem Qukeys::key_queue_[] = {};
 uint8_t Qukeys::key_queue_length_ = 0;
-byte Qukeys::qukey_state_[] = {};
 bool Qukeys::flushing_queue_ = false;
+uint8_t Qukeys::delayed_qukey_addr_ = QUKEY_UNKNOWN_ADDR;
+int16_t Qukeys::delayed_qukey_start_time_ = 0;
 
 constexpr uint16_t QUKEYS_RELEASE_DELAY_OFFSET = 4096;
 
@@ -115,17 +116,14 @@ int8_t Qukeys::lookupQukey(uint8_t key_addr) {
 
 void Qukeys::enqueue(uint8_t key_addr) {
   if (key_queue_length_ == QUKEYS_QUEUE_MAX) {
-    setQukeyState(key_queue_[0].addr, QUKEY_STATE_PRIMARY);
     flushKey(QUKEY_STATE_PRIMARY, IS_PRESSED | WAS_PRESSED);
     flushQueue();
   }
   // default to alternate state to stop keys being flushed from the queue before the grace
   // period timeout
-  setQukeyState(key_addr, QUKEY_STATE_ALTERNATE);
   key_queue_[key_queue_length_].addr = key_addr;
   key_queue_[key_queue_length_].start_time = millis();
   key_queue_length_++;
-  addr::mask(key_addr);
 }
 
 int8_t Qukeys::searchQueue(uint8_t key_addr) {
@@ -138,24 +136,26 @@ int8_t Qukeys::searchQueue(uint8_t key_addr) {
 
 // flush a single entry from the head of the queue
 bool Qukeys::flushKey(bool qukey_state, uint8_t keyswitch_state) {
-  addr::unmask(key_queue_[0].addr);
   int8_t qukey_index = lookupQukey(key_queue_[0].addr);
   bool is_qukey = (qukey_index != QUKEY_NOT_FOUND);
   byte row = addr::row(key_queue_[0].addr);
   byte col = addr::col(key_queue_[0].addr);
-  bool is_dual_use = isDualUse(Layer.lookup(row, col));
+  Key keycode = Layer.lookupOnActiveLayer(row, col);
-  Key keycode = Key_NoKey;
+  bool is_dual_use = isDualUse(keycode);
   if (is_qukey || is_dual_use) {
-    if (qukey_state == QUKEY_STATE_PRIMARY &&
+    if (qukey_state == QUKEY_STATE_PRIMARY) {
-        getQukeyState(key_queue_[0].addr) == QUKEY_STATE_ALTERNATE) {
       // If there's a release delay in effect, and there's at least one key after it in
       // the queue, delay this key's release event:
-      if (release_delay_ > 0 && key_queue_length_ > 1) {
+      if (release_delay_ > 0 && key_queue_length_ > 1
-        key_queue_[0].start_time = millis() + QUKEYS_RELEASE_DELAY_OFFSET;
+          && delayed_qukey_addr_ == QUKEY_UNKNOWN_ADDR) {
+        delayed_qukey_start_time_ = millis();
+        // Store the delayed key's address to send the toggle-off event later, if
+        // appropriate:
+        delayed_qukey_addr_ = key_queue_[0].addr;
         return false;
       }
+      keycode = getDualUsePrimaryKey(keycode);
     }
-    setQukeyState(key_queue_[0].addr, qukey_state);
     if (qukey_state == QUKEY_STATE_ALTERNATE) {
       if (is_dual_use) {
         keycode = getDualUseAlternateKey(keycode);
@@ -256,21 +256,6 @@ EventHandlerResult Qukeys::onKeyswitchEvent(Key &mapped_key, byte row, byte col,
 
   // If the key was injected (from the queue being flushed)
   if (flushing_queue_) {
-    // If it's a DualUse key, we still need to update its keycode
-    if (isDualUse(mapped_key)) {
-      if (getQukeyState(key_addr) == QUKEY_STATE_ALTERNATE) {
-        mapped_key = getDualUseAlternateKey(mapped_key);
-      } else {
-        mapped_key = getDualUsePrimaryKey(mapped_key);
-      }
-    }
-    // ...otherwise, just continue to the next plugin
-    return EventHandlerResult::OK;
-  }
-
-  // If the key isn't active, and didn't just toggle off, continue to next plugin
-  if (!keyIsPressed(key_state) && !keyWasPressed(key_state)) {
-    mapped_key = getDualUsePrimaryKey(mapped_key);
     return EventHandlerResult::OK;
   }
 
@@ -296,74 +281,58 @@ EventHandlerResult Qukeys::onKeyswitchEvent(Key &mapped_key, byte row, byte col,
   if (keyToggledOff(key_state)) {
     // If the key isn't in the key_queue, proceed
     if (queue_index == QUKEY_NOT_FOUND) {
-      // If a qukey was released while in its alternate state, change its keycode
-      if (isDualUse(mapped_key)) {
-        if (getQukeyState(key_addr) == QUKEY_STATE_ALTERNATE) {
-          mapped_key = getDualUseAlternateKey(mapped_key);
-        } else {
-          mapped_key = getDualUsePrimaryKey(mapped_key);
-        }
-      } else if (qukey_index != QUKEY_NOT_FOUND) {
-        if (getQukeyState(key_addr) == QUKEY_STATE_ALTERNATE) {
-          mapped_key = qukeys[qukey_index].alt_keycode;
-        }
-      }
       return EventHandlerResult::OK;
     }
+    // Finally, send the release event of the delayed qukey, if any. This is necessary in
+    // order to send a toggle off of a `ShiftToLayer()` key; otherwise, that layer gets
+    // stuck on if there's a release delay and a rollover.
+    if (delayed_qukey_addr_ != QUKEY_UNKNOWN_ADDR) {
+      int8_t r = addr::row(delayed_qukey_addr_);
+      int8_t c = addr::col(delayed_qukey_addr_);
+      flushQueue(queue_index);
+      flushQueue();
+      flushing_queue_ = true;
+      handleKeyswitchEvent(Key_NoKey, r, c, WAS_PRESSED);
+      flushing_queue_ = false;
+      delayed_qukey_addr_ = QUKEY_UNKNOWN_ADDR;
+    } else {
       flushQueue(queue_index);
       flushQueue();
+    }
+    //if (delayed_qukey_addr_ != QUKEY_UNKNOWN_ADDR)
+    //  return EventHandlerResult::EVENT_CONSUMED;
     mapped_key = getDualUsePrimaryKey(mapped_key);
     return EventHandlerResult::OK;
   }
 
   // Otherwise, the key is still pressed
 
-  // If the key is not a qukey:
+  // Only keys in the queue can still evaluate as qukeys, so all we need to do here is
-  if (qukey_index == QUKEY_NOT_FOUND &&
+  // block events for held keys that are still in the queue.
-      ! isDualUse(mapped_key)) {
-    // If the key was pressed before the keys in the queue, proceed:
   if (queue_index == QUKEY_NOT_FOUND) {
+    // The key is not in the queue; proceed:
     return EventHandlerResult::OK;
   } else {
-      // suppress this keypress; it's still in the queue
+    // The key is still in the queue; abort:
     return EventHandlerResult::EVENT_CONSUMED;
   }
-  }
-
-  // If the qukey is not in the queue, check its state
-  if (queue_index == QUKEY_NOT_FOUND) {
-    if (getQukeyState(key_addr) == QUKEY_STATE_ALTERNATE) {
-      if (isDualUse(mapped_key)) {
-        mapped_key = getDualUseAlternateKey(mapped_key);
-      } else {
-        mapped_key = qukeys[qukey_index].alt_keycode;
-      }
-    } else { // qukey_state == QUKEY_STATE_PRIMARY
-      mapped_key = getDualUsePrimaryKey(mapped_key);
-    }
-    return EventHandlerResult::OK;
-  }
-  // else state is undetermined; block. I could check timeouts here,
-  // but I'd rather do that in the pre-report hook
-  return EventHandlerResult::EVENT_CONSUMED;
 }
 
 EventHandlerResult Qukeys::beforeReportingState() {
 
   uint16_t current_time = millis();
 
-  if (release_delay_ > 0 && key_queue_length_ > 0) {
+  if (delayed_qukey_addr_ != QUKEY_UNKNOWN_ADDR) {
-    int16_t diff_time = key_queue_[0].start_time - current_time;
+    int16_t diff_time = current_time - delayed_qukey_start_time_;
-    if (diff_time > 0) {
+    if (diff_time > release_delay_) {
-      int16_t delay_window = QUKEYS_RELEASE_DELAY_OFFSET - release_delay_;
-      if (diff_time < delay_window) {
-        setQukeyState(key_queue_[0].addr, QUKEY_STATE_PRIMARY);
       flushKey(QUKEY_STATE_PRIMARY, WAS_PRESSED);
       flushQueue();
+      // If the release delay has timed out, we need to prevent the wrong toggle-off
+      // event from being sent:
+      delayed_qukey_addr_ = QUKEY_UNKNOWN_ADDR;
     }
     return EventHandlerResult::OK;
   }
-  }
 
   // If the qukey has been held longer than the time limit, set its
   // state to the alternate keycode and add it to the report

src/kaleidoscope/plugin/Qukeys.h

@@ -115,14 +115,8 @@ class Qukeys : public kaleidoscope::Plugin {
   static uint8_t key_queue_length_;
   static bool flushing_queue_;
 
-  // Qukey state bitfield
+  static uint8_t delayed_qukey_addr_;
-  static uint8_t qukey_state_[(TOTAL_KEYS) / 8 + ((TOTAL_KEYS) % 8 ? 1 : 0)];
+  static int16_t delayed_qukey_start_time_;
-  static bool getQukeyState(uint8_t addr) {
-    return bitRead(qukey_state_[addr / 8], addr % 8);
-  }
-  static void setQukeyState(uint8_t addr, boolean qukey_state) {
-    bitWrite(qukey_state_[addr / 8], addr % 8, qukey_state);
-  }
 
   static int8_t lookupQukey(uint8_t key_addr);
   static void enqueue(uint8_t key_addr);

src/kaleidoscope/plugin/SpaceCadet.cpp

@@ -130,7 +130,7 @@ EventHandlerResult SpaceCadet::onKeyswitchEvent(Key &mapped_key, byte row, byte
         //hit another key after this -- if it's a modifier, we want the modifier
         //key to be added to the report, for things like ctrl, alt, shift, etc)
         if (map[i].flagged) {
-          handleKeyswitchEvent(map[i].input, row, col, IS_PRESSED | INJECTED);
+          handleKeyswitchEvent(map[i].input, UNKNOWN_KEYSWITCH_LOCATION, IS_PRESSED | INJECTED);
         }
 
         //The keypress wasn't a match, so we need to mark it as not flagged and