From f32f845d976c8f75ff72452a6e260089569a8218 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 28 Oct 2020 04:23:12 +0100 Subject: [PATCH 1/2] tests: Add a failing testcase for #951 This adds a test to check that `Layer.isActive(0)` should return true, without explicitly activating layer 0. At the moment, this is not the case, and the test will fail. Signed-off-by: Gergely Nagy --- tests/issues/951/sketch.ino | 45 ++++++++++++++++++++++++++++++ tests/issues/951/test/testcase.cpp | 39 ++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 tests/issues/951/sketch.ino create mode 100644 tests/issues/951/test/testcase.cpp diff --git a/tests/issues/951/sketch.ino b/tests/issues/951/sketch.ino new file mode 100644 index 00000000..2c6c5a76 --- /dev/null +++ b/tests/issues/951/sketch.ino @@ -0,0 +1,45 @@ +/* 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 . + */ + +#include "Kaleidoscope.h" + +// *INDENT-OFF* +KEYMAPS( + [0] = KEYMAP_STACKED + (___, Key_1, Key_2, Key_3, Key_4, Key_5, ___, + 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_6, Key_7, Key_8, Key_9, Key_0, ___, + Key_Enter, Key_Y, Key_U, Key_I, Key_O, Key_P, Key_Equals, + Key_H, Key_J, Key_K, Key_L, Key_Semicolon, Key_Quote, + Key_RightAlt, Key_N, Key_M, Key_Comma, Key_Period, Key_Slash, Key_Minus, + Key_RightShift, Key_LeftAlt, Key_Spacebar, Key_RightControl, + ___) +) + +// *INDENT-ON* + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/issues/951/test/testcase.cpp b/tests/issues/951/test/testcase.cpp new file mode 100644 index 00000000..bd9a2e24 --- /dev/null +++ b/tests/issues/951/test/testcase.cpp @@ -0,0 +1,39 @@ +/* -*- 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 . + */ + +#include "Kaleidoscope.h" +#include "testing/fix-macros.h" + +#include "testing/setup-googletest.h" + +SETUP_GOOGLETEST(); + +namespace kaleidoscope { +namespace testing { +namespace { + +using ::testing::IsEmpty; + +class Issue951: public VirtualDeviceTest {}; + +TEST_F(Issue951, InitialLayerState) { + EXPECT_THAT(Layer.isActive(0), true) + << "Layer 0 should be active when the keyboard starts up."; +} + +} // namespace +} // namespace testing +} // namespace kaleidoscope From 60b5b24bd6e97c752031e3021fe139f34e8828f0 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 28 Oct 2020 04:29:36 +0100 Subject: [PATCH 2/2] Layers: Fix the initial state of layer 0 When starting up, we correctly set the active layer counter to one, and the active layer stack will therefore correctly contain layer 0 as an active layer. However, we weren't setting the `layer_state_` bitmap up properly, and as such, `Layer.isActive(0)` was returning false, despite the layer being active as far as lookups were concerned. To fix this, we explicitly flip the 0th bit on in the newly introduced `Layer.setup()` method, where the initial keymap cache update was moved to, too. Fixes #951. Signed-off-by: Gergely Nagy --- src/kaleidoscope/Runtime.cpp | 6 +----- src/kaleidoscope/layers.cpp | 11 +++++++++++ src/kaleidoscope/layers.h | 2 ++ 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/kaleidoscope/Runtime.cpp b/src/kaleidoscope/Runtime.cpp index 4f45f0c8..380ec63a 100644 --- a/src/kaleidoscope/Runtime.cpp +++ b/src/kaleidoscope/Runtime.cpp @@ -42,11 +42,7 @@ Runtime_::setup(void) { device().setup(); - // Update the keymap cache, so we start with a non-empty state. - Layer.updateActiveLayers(); - for (auto key_addr : KeyAddr::all()) { - Layer.updateLiveCompositeKeymap(key_addr); - } + Layer.setup(); } void diff --git a/src/kaleidoscope/layers.cpp b/src/kaleidoscope/layers.cpp index af283773..48053d42 100644 --- a/src/kaleidoscope/layers.cpp +++ b/src/kaleidoscope/layers.cpp @@ -46,6 +46,17 @@ Key Layer_::live_composite_keymap_[Runtime.device().numKeys()]; uint8_t Layer_::active_layer_keymap_[Runtime.device().numKeys()]; Layer_::GetKeyFunction Layer_::getKey = &Layer_::getKeyFromPROGMEM; +void Layer_::setup() { + // Explicitly set layer 0's state to 1 + bitSet(layer_state_, 0); + + // Update the keymap cache, so we start with a non-empty state. + Layer.updateActiveLayers(); + for (auto key_addr : KeyAddr::all()) { + Layer.updateLiveCompositeKeymap(key_addr); + } +} + void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { if (keymapEntry.getKeyCode() >= LAYER_MOVE_OFFSET) { if (keyToggledOn(keyState)) { diff --git a/src/kaleidoscope/layers.h b/src/kaleidoscope/layers.h index 22ceb9cb..9b6175b9 100644 --- a/src/kaleidoscope/layers.h +++ b/src/kaleidoscope/layers.h @@ -51,6 +51,8 @@ class Layer_ { public: Layer_() {} + void setup(); + /* There are two lookup functions, because we have two caches, and different * parts of the firmware will want to use either this or that (or perhaps * both, in rare cases).