From 08f45ba6a95d5752112b522f55555ef7ee75d924 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 4 Feb 2021 11:15:44 -0600 Subject: [PATCH 1/3] Allow deactivation of base layer Instead of safeguarding against an unrecoverable blank layer state by guaranteeing that layer 0 is always active (and therefore always at the bottom of the layer stack), we safeguard by a move to layer 0 if the only active layer is deactivated. Signed-off-by: Michael Richters --- src/kaleidoscope/layers.cpp | 54 +++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/src/kaleidoscope/layers.cpp b/src/kaleidoscope/layers.cpp index 48053d42..df55b168 100644 --- a/src/kaleidoscope/layers.cpp +++ b/src/kaleidoscope/layers.cpp @@ -104,8 +104,9 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { break; } } else if (keyToggledOn(keyState)) { + uint8_t target = keymapEntry.getKeyCode(); // switch keymap and stay there - if (Layer.isActive(keymapEntry.getKeyCode()) && keymapEntry.getKeyCode()) + if (Layer.isActive(target)) deactivate(keymapEntry.getKeyCode()); else activate(keymapEntry.getKeyCode()); @@ -130,22 +131,28 @@ void Layer_::updateLiveCompositeKeymap(KeyAddr key_addr) { } void Layer_::updateActiveLayers(void) { + // First, set every entry in the active layer keymap to point to the default + // layer (layer 0). memset(active_layer_keymap_, 0, Runtime.device().numKeys()); + + // For each key address, set its entry in the active layer keymap to the value + // of the top active layer that has a non-transparent entry for that address. for (auto key_addr : KeyAddr::all()) { - int8_t layer_index = active_layer_count_; - while (layer_index > 0) { - uint8_t layer = active_layers_[layer_index - 1]; - if (Layer.isActive(layer)) { - Key mappedKey = (*getKey)(layer, key_addr); - - if (mappedKey != Key_Transparent) { - active_layer_keymap_[key_addr.toInt()] = layer; - break; - } + for (uint8_t i = active_layer_count_; i > 0; --i) { + uint8_t layer = active_layers_[i - 1]; + Key key = (*getKey)(layer, key_addr); + + if (key != Key_Transparent) { + active_layer_keymap_[key_addr.toInt()] = layer; + break; } - layer_index--; } } + // Even if there are no active layers (a situation that should be prevented by + // `deactivate()`), each key will be mapped from the base layer (layer + // 0). Likewise, for any address where all active layers have a transparent + // entry, that key will be mapped from the base layer, even if the base layer + // has been deactivated. } void Layer_::move(uint8_t layer) { @@ -177,7 +184,8 @@ void Layer_::activate(uint8_t layer) { if (isActive(layer)) return; - // Otherwise, turn on its bit in layer_state_ + // Otherwise, turn on its bit in layer_state_, and push it onto the active + // layer stack bitSet(layer_state_, layer); active_layers_[active_layer_count_++] = layer; @@ -191,22 +199,28 @@ void Layer_::activate(uint8_t layer) { // Deactivate a given layer void Layer_::deactivate(uint8_t layer) { // If the target layer was already off, return - if (!bitRead(layer_state_, layer)) + if (!isActive(layer)) return; + // If the sole active layer is being deactivated, turn on the base layer and + // return so we always have at least one layer active. + if (active_layer_count_ <= 1) { + move(0); + return; + } + // Turn off its bit in layer_state_ bitClear(layer_state_, layer); - // Rearrange the activation order array... - uint8_t idx = 0; - for (uint8_t i = active_layer_count_; i > 0; i--) { + // Remove the target layer from the active layer stack, and shift any layers + // above it down to fill in the gap + for (uint8_t i = 0; i < active_layer_count_; ++i) { if (active_layers_[i] == layer) { - idx = i; + memmove(&active_layers_[i], &active_layers_[i + 1], active_layer_count_ - i); + --active_layer_count_; break; } } - memmove(&active_layers_[idx], &active_layers_[idx + 1], active_layer_count_ - idx); - active_layer_count_--; // Update the keymap cache (but not live_composite_keymap_; that gets // updated separately, when keys toggle on or off. See layers.h) From 5c12c8ee49579e7a33bf77ac39355c669aa645a6 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Fri, 5 Feb 2021 13:44:40 -0600 Subject: [PATCH 2/3] Add comments to handleKeymapKeyswitchEvent() Signed-off-by: Michael Richters --- src/kaleidoscope/layers.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/kaleidoscope/layers.cpp b/src/kaleidoscope/layers.cpp index df55b168..67e30768 100644 --- a/src/kaleidoscope/layers.cpp +++ b/src/kaleidoscope/layers.cpp @@ -59,14 +59,17 @@ void Layer_::setup() { void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { if (keymapEntry.getKeyCode() >= LAYER_MOVE_OFFSET) { + // MoveToLayer() if (keyToggledOn(keyState)) { move(keymapEntry.getKeyCode() - LAYER_MOVE_OFFSET); } } else if (keymapEntry.getKeyCode() >= LAYER_SHIFT_OFFSET) { + // layer shift keys uint8_t target = keymapEntry.getKeyCode() - LAYER_SHIFT_OFFSET; switch (target) { case KEYMAP_NEXT: + // Key_KeymapNext_Momentary if (keyToggledOn(keyState)) activateNext(); else if (keyToggledOff(keyState)) @@ -74,6 +77,7 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { break; case KEYMAP_PREVIOUS: + // Key_KeymapPrevious_Momentary if (keyToggledOn(keyState)) deactivateMostRecent(); else if (keyToggledOff(keyState)) @@ -81,6 +85,7 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { break; default: + // ShiftToLayer() /* The default case is when we are switching to a layer by its number, and * is a bit more complicated than switching there when the key toggles on, * and away when it toggles off. @@ -104,6 +109,7 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { break; } } else if (keyToggledOn(keyState)) { + // LockLayer()/UnlockLayer() uint8_t target = keymapEntry.getKeyCode(); // switch keymap and stay there if (Layer.isActive(target)) From 96b708f5a1bbd8c29ec4278b73453bb317ed7b8f Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Sat, 13 Feb 2021 10:27:02 -0600 Subject: [PATCH 3/3] Add testcases for floating base layer Signed-off-by: Michael Richters --- .../layers/floating-base/floating-base.ino | 80 +++++++++ .../features/layers/floating-base/sketch.json | 6 + .../features/layers/floating-base/test.ktest | 165 ++++++++++++++++++ 3 files changed, 251 insertions(+) create mode 100644 tests/features/layers/floating-base/floating-base.ino create mode 100644 tests/features/layers/floating-base/sketch.json create mode 100644 tests/features/layers/floating-base/test.ktest diff --git a/tests/features/layers/floating-base/floating-base.ino b/tests/features/layers/floating-base/floating-base.ino new file mode 100644 index 00000000..fa4403a4 --- /dev/null +++ b/tests/features/layers/floating-base/floating-base.ino @@ -0,0 +1,80 @@ +/* -*- 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 . + */ + +#include + +// *INDENT-OFF* +KEYMAPS( + [0] = KEYMAP_STACKED + ( + Key_A, ___, ___, ___, ___, ___, ___, + ShiftToLayer(0), ShiftToLayer(1), ShiftToLayer(2), ___, ___, ___, ___, + LockLayer(0), LockLayer(1), LockLayer(2), ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___, + + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___ + ), + [1] = KEYMAP_STACKED + ( + Key_B, Key_X, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___, + + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___ + ), + [2] = KEYMAP_STACKED + ( + Key_C, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___, + + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___ + ), +) +// *INDENT-ON* + +//KALEIDOSCOPE_INIT_PLUGINS(); + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/features/layers/floating-base/sketch.json b/tests/features/layers/floating-base/sketch.json new file mode 100644 index 00000000..43dc4c7e --- /dev/null +++ b/tests/features/layers/floating-base/sketch.json @@ -0,0 +1,6 @@ +{ + "cpu": { + "fqbn": "keyboardio:virtual:model01", + "port": "" + } +} \ No newline at end of file diff --git a/tests/features/layers/floating-base/test.ktest b/tests/features/layers/floating-base/test.ktest new file mode 100644 index 00000000..f8fcc8d9 --- /dev/null +++ b/tests/features/layers/floating-base/test.ktest @@ -0,0 +1,165 @@ +VERSION 1 + +KEYSWITCH A 0 0 +KEYSWITCH X 0 1 + +KEYSWITCH LAYER_SHIFT_0 1 0 +KEYSWITCH LAYER_SHIFT_1 1 1 +KEYSWITCH LAYER_SHIFT_2 1 2 + +KEYSWITCH LAYER_LOCK_0 2 0 +KEYSWITCH LAYER_LOCK_1 2 1 +KEYSWITCH LAYER_LOCK_2 2 2 + +# ============================================================================== +# Previously, if only layer 0 was active, and the user pressed and then released +# `ShiftToLayer(0)`, the layer count would go to zero. This test would still +# pass, however, because Kaleidoscope falls back on layer 0 values anyway. +NAME Base layer stays active + +RUN 5 cycles + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_A # Report should contain only `A` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +PRESS LAYER_SHIFT_0 +RUN 5 ms +RELEASE LAYER_SHIFT_0 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_A # Report should contain only `A` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# ============================================================================== +NAME Lock layer 1 + +# Activate Layer 1 +PRESS LAYER_LOCK_1 +RUN 5 ms +RELEASE LAYER_LOCK_1 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_B # Report should contain only `B` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# ============================================================================== +NAME Stack layer 0 on top of layer 1 + +# Deactivate Layer 0: Now the layer stack should contain only layer 1. +PRESS LAYER_LOCK_0 +RUN 5 ms +RELEASE LAYER_LOCK_0 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_B # Report should contain only `B` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# Activate Layer 0: There should now be two layers in the stack, with layer 0 on +# top of layer 1. +PRESS LAYER_LOCK_0 +RUN 5 ms +RELEASE LAYER_LOCK_0 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_A # Report should contain only `A` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# Deactivate Layer 0: Back to just layer 1 active. +PRESS LAYER_LOCK_0 +RUN 5 ms +RELEASE LAYER_LOCK_0 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_B # Report should contain only `B` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# Hold Layer 0: Now the same test, but with a layer shift instead of a layer +# lock. The stack should now have layer 0 on top of layer 1. +PRESS LAYER_SHIFT_0 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_A # Report should contain only `A` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# Release Layer 0 shift: Back to just layer 1 active. +RELEASE LAYER_SHIFT_0 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_B # Report should contain only `B` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms + +# ============================================================================== +NAME Default to layer 0 + +# Unlock layer 1: This was the only active layer; layer 0 should activate +# automatically in response. +PRESS LAYER_LOCK_1 +RUN 5 ms +RELEASE LAYER_LOCK_1 +RUN 5 ms + +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_A # Report should contain only `A` +RUN 5 ms + +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty # Report should be empty +RUN 5 ms