From 8aa9206a0856595fb6e84e00724b4a3927a2cb04 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 24 Feb 2022 10:07:45 -0600 Subject: [PATCH 1/7] Make KeyAddrBitfield class const-correct Signed-off-by: Michael Richters --- src/kaleidoscope/KeyAddrBitfield.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/kaleidoscope/KeyAddrBitfield.h b/src/kaleidoscope/KeyAddrBitfield.h index bd764ca3..8d612a66 100644 --- a/src/kaleidoscope/KeyAddrBitfield.h +++ b/src/kaleidoscope/KeyAddrBitfield.h @@ -109,16 +109,16 @@ class KeyAddrBitfield { class Iterator; friend class KeyAddrBitfield::Iterator; - Iterator begin() { + Iterator begin() const { return Iterator{*this, 0}; } - Iterator end() { + Iterator end() const { return Iterator{*this, total_blocks}; } class Iterator { public: - Iterator(KeyAddrBitfield &bitfield, uint8_t x) + Iterator(const KeyAddrBitfield &bitfield, uint8_t x) : bitfield_(bitfield), block_index_(x) {} bool operator!=(const Iterator &other) { @@ -163,7 +163,7 @@ class KeyAddrBitfield { } private: - KeyAddrBitfield &bitfield_; + const KeyAddrBitfield &bitfield_; uint8_t block_index_; // index of the block uint8_t bit_index_{0}; // bit index in the block uint8_t block_; From fd17553f1a2cd80b8f928d09590b1b52c25b31dc Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 24 Feb 2022 10:08:17 -0600 Subject: [PATCH 2/7] Rearrange iterator code in KeyAddrBitfield to simplify declarations Signed-off-by: Michael Richters --- src/kaleidoscope/KeyAddrBitfield.h | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/kaleidoscope/KeyAddrBitfield.h b/src/kaleidoscope/KeyAddrBitfield.h index 8d612a66..2a74cf54 100644 --- a/src/kaleidoscope/KeyAddrBitfield.h +++ b/src/kaleidoscope/KeyAddrBitfield.h @@ -106,16 +106,6 @@ class KeyAddrBitfield { // ---------------------------------------------------------------------------- // Iterator! public: - class Iterator; - friend class KeyAddrBitfield::Iterator; - - Iterator begin() const { - return Iterator{*this, 0}; - } - Iterator end() const { - return Iterator{*this, total_blocks}; - } - class Iterator { public: Iterator(const KeyAddrBitfield &bitfield, uint8_t x) @@ -171,6 +161,15 @@ class KeyAddrBitfield { }; // class Iterator { + friend class Iterator; + + Iterator begin() const { + return Iterator{*this, 0}; + } + Iterator end() const { + return Iterator{*this, total_blocks}; + } + } __attribute__((packed)); // class KeyAddrBitfield { } // namespace kaleidoscope { From a1c1fe6ba29969f1f190fa38256e71de17889cfe Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 24 Feb 2022 11:22:28 -0600 Subject: [PATCH 3/7] Convert OneShot variables and functions from `static` This is more like "standard" C++ code, resulting in more readable code, with default configuration values stored in the header file, and `const`-correct member functions clearly marking which ones alter internal state and which ones don't (with the exception of the event handlers). Things had been declared `static` because the compiler produced a significantly smaller binary in PROGMEM, but that appears not to be the case now. Signed-off-by: Michael Richters --- .../src/kaleidoscope/plugin/OneShot.cpp | 48 ++------ .../src/kaleidoscope/plugin/OneShot.h | 112 +++++++++--------- 2 files changed, 69 insertions(+), 91 deletions(-) diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp index 2b4bc94f..38dc1472 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp @@ -1,6 +1,6 @@ /* -*- mode: c++ -*- * Kaleidoscope-OneShot -- One-shot modifiers and layers - * Copyright (C) 2016-2021 Keyboard.io, Inc. + * Copyright (C) 2016-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 @@ -23,28 +23,6 @@ namespace kaleidoscope { namespace plugin { -// ---------------------------------------------------------------------------- -// Configuration variables - -uint16_t OneShot::timeout_ = 2500; -uint16_t OneShot::hold_timeout_ = 250; -int16_t OneShot::double_tap_timeout_ = -1; - -// ---------------------------------------------------------------------------- -// State variables - -uint16_t OneShot::stickable_keys_ = -1; - -bool OneShot::auto_modifiers_ = false; -bool OneShot::auto_layers_ = false; - -KeyAddrBitfield OneShot::temp_addrs_; -KeyAddrBitfield OneShot::glue_addrs_; - -uint16_t OneShot::start_time_ = 0; -KeyAddr OneShot::prev_key_addr_ = OneShot::invalid_key_addr; - - // ============================================================================ // Public interface @@ -80,7 +58,7 @@ void OneShot::disableStickabilityForLayers() { // ---------------------------------------------------------------------------- // Global tests for any OneShot key -bool OneShot::isActive() { +bool OneShot::isActive() const { for (KeyAddr key_addr __attribute__((unused)) : temp_addrs_) { return true; } @@ -90,7 +68,7 @@ bool OneShot::isActive() { return false; } -bool OneShot::isSticky() { +bool OneShot::isSticky() const { for (KeyAddr key_addr : glue_addrs_) { if (! temp_addrs_.read(key_addr)) { return true; @@ -107,11 +85,11 @@ bool OneShot::isSticky() { // states (sticky | active && !sticky | pressed && !active). __attribute__((weak)) -bool OneShot::isStickable(Key key) { +bool OneShot::isStickable(Key key) const { return isStickableDefault(key); } -bool OneShot::isStickableDefault(Key key) { +bool OneShot::isStickableDefault(Key key) const { int8_t n; // If the key is either a keyboard modifier or a layer shift, we check to see // if it has been set to be non-stickable. @@ -132,19 +110,19 @@ bool OneShot::isStickableDefault(Key key) { return true; } -bool OneShot::isTemporary(KeyAddr key_addr) { +bool OneShot::isTemporary(KeyAddr key_addr) const { return temp_addrs_.read(key_addr); } -bool OneShot::isPending(KeyAddr key_addr) { +bool OneShot::isPending(KeyAddr key_addr) const { return (glue_addrs_.read(key_addr) && temp_addrs_.read(key_addr)); } -bool OneShot::isSticky(KeyAddr key_addr) { +bool OneShot::isSticky(KeyAddr key_addr) const { return (glue_addrs_.read(key_addr) && !temp_addrs_.read(key_addr)); } -bool OneShot::isActive(KeyAddr key_addr) { +bool OneShot::isActive(KeyAddr key_addr) const { return (isTemporary(key_addr) || glue_addrs_.read(key_addr)); } @@ -334,7 +312,7 @@ EventHandlerResult OneShot::afterEachCycle() { // ---------------------------------------------------------------------------- // Helper functions for acting on OneShot key events -uint8_t OneShot::getOneShotKeyIndex(Key oneshot_key) { +uint8_t OneShot::getOneShotKeyIndex(Key oneshot_key) const { // The calling function is responsible for verifying that // `oneshot_key` is an actual OneShot key (i.e. call // `isOneShotKey(oneshot_key)` first). @@ -342,7 +320,7 @@ uint8_t OneShot::getOneShotKeyIndex(Key oneshot_key) { return index; } -uint8_t OneShot::getKeyIndex(Key key) { +uint8_t OneShot::getKeyIndex(Key key) const { // Default to returning a value that's out of range. This should be // harmless because we only use the returned index to reference a // bit in a bitfield, not as a memory address. @@ -358,7 +336,7 @@ uint8_t OneShot::getKeyIndex(Key key) { return n; } -Key OneShot::decodeOneShotKey(Key oneshot_key) { +Key OneShot::decodeOneShotKey(Key oneshot_key) const { // The calling function is responsible for verifying that // `oneshot_key` is an actual OneShot key (i.e. call // `isOneShotKey(oneshot_key)` first). @@ -386,7 +364,7 @@ void OneShot::pressKey(KeyAddr key_addr, Key key) { Runtime.handleKeyEvent(event); } -void OneShot::holdKey(KeyAddr key_addr) { +void OneShot::holdKey(KeyAddr key_addr) const { KeyEvent event{key_addr, WAS_PRESSED | IS_PRESSED | INJECTED}; Runtime.handleKeyEvent(event); } diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h index a8961991..cb425636 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h @@ -1,6 +1,6 @@ /* -*- mode: c++ -*- * Kaleidoscope-OneShot -- One-shot modifiers and layers - * Copyright (C) 2016-2021 Keyboard.io, Inc. + * Copyright (C) 2016-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 @@ -33,60 +33,60 @@ namespace plugin { class OneShot : public kaleidoscope::Plugin { public: // Constructor - OneShot() {} + // OneShot() {} // -------------------------------------------------------------------------- // Configuration functions - static inline void enableStickablity() {} - static void enableStickability(Key key); + void enableStickablity() {} + void enableStickability(Key key); template - static void enableStickability(Key key, Keys&&... keys) { + void enableStickability(Key key, Keys&&... keys) { enableStickability(key); enableStickability(keys...); } - static void enableStickabilityForModifiers(); - static void enableStickabilityForLayers(); + void enableStickabilityForModifiers(); + void enableStickabilityForLayers(); - static inline void disableStickability() {} - static void disableStickability(Key key); + void disableStickability() {} + void disableStickability(Key key); template - static void disableStickability(Key key, Keys&&... keys) { + void disableStickability(Key key, Keys&&... keys) { disableStickability(key); disableStickability(keys...); } - static void disableStickabilityForModifiers(); - static void disableStickabilityForLayers(); + void disableStickabilityForModifiers(); + void disableStickabilityForLayers(); - static void enableAutoModifiers() { + void enableAutoModifiers() { auto_modifiers_ = true; } - static void enableAutoLayers() { + void enableAutoLayers() { auto_layers_ = true; } - static void enableAutoOneShot() { + void enableAutoOneShot() { enableAutoModifiers(); enableAutoLayers(); } - static void disableAutoModifiers() { + void disableAutoModifiers() { auto_modifiers_ = false; } - static void disableAutoLayers() { + void disableAutoLayers() { auto_layers_ = false; } - static void disableAutoOneShot() { + void disableAutoOneShot() { disableAutoModifiers(); disableAutoLayers(); } - static void toggleAutoModifiers() { + void toggleAutoModifiers() { auto_modifiers_ = ! auto_modifiers_; } - static void toggleAutoLayers() { + void toggleAutoLayers() { auto_layers_ = ! auto_layers_; } - static void toggleAutoOneShot() { + void toggleAutoOneShot() { if (auto_modifiers_ || auto_layers_) { disableAutoOneShot(); } else { @@ -97,25 +97,25 @@ class OneShot : public kaleidoscope::Plugin { // -------------------------------------------------------------------------- // Global test functions - static bool isActive(); - static bool isSticky(); + bool isActive() const; + bool isSticky() const; // -------------------------------------------------------------------------- // Single-key test functions - static bool isOneShotKey(Key key) { + bool isOneShotKey(Key key) const { return (key.getRaw() >= kaleidoscope::ranges::OS_FIRST && key.getRaw() <= kaleidoscope::ranges::OS_LAST); } /// Determine if the given `key` is allowed to become sticky. - static bool isStickable(Key key); + bool isStickable(Key key) const; - static bool isStickableDefault(Key key); + bool isStickableDefault(Key key) const; - static bool isTemporary(KeyAddr key_addr); // inline? - static bool isPending(KeyAddr key_addr); - static bool isSticky(KeyAddr key_addr); // inline? - static bool isActive(KeyAddr key_addr); // inline? + bool isTemporary(KeyAddr key_addr) const; // inline? + bool isPending(KeyAddr key_addr) const; + bool isSticky(KeyAddr key_addr) const; // inline? + bool isActive(KeyAddr key_addr) const; // inline? // -------------------------------------------------------------------------- // Public OneShot state control @@ -126,7 +126,7 @@ class OneShot : public kaleidoscope::Plugin { /// This is appropriate to use when a key toggles on and you want it to behave /// like a OneShot key starting with the current event, and lasting until the /// key becomes inactive (cancelled by a subsequent keypress). - static void setPending(KeyAddr key_addr); + void setPending(KeyAddr key_addr); /// Put a key directly in the "one-shot" state. /// @@ -136,33 +136,33 @@ class OneShot : public kaleidoscope::Plugin { /// use `setPending()` instead, rather than calling this function explicitly, /// as OneShot will automatically cause any key in the "pending" state to /// progress to this state when it is (physically) released. - static void setOneShot(KeyAddr key_addr); + void setOneShot(KeyAddr key_addr); /// Put a key in the "sticky" OneShot state. /// /// This function puts the key at `key_addr` in the "sticky" OneShot state. /// It will remain active until it is pressed again. - static void setSticky(KeyAddr key_addr); + void setSticky(KeyAddr key_addr); /// Clear any OneShot state for a key. /// /// This function clears any OneShot state of the key at `key_addr`. It does /// not, however, release the key if it is held. - static void clear(KeyAddr key_addr); + void clear(KeyAddr key_addr); // -------------------------------------------------------------------------- // Utility function for other plugins to cancel OneShot keys - static void cancel(bool with_stickies = false); + void cancel(bool with_stickies = false); // -------------------------------------------------------------------------- // Timeout onfiguration functions - static void setTimeout(uint16_t ttl) { + void setTimeout(uint16_t ttl) { timeout_ = ttl; } - static void setHoldTimeout(uint16_t ttl) { + void setHoldTimeout(uint16_t ttl) { hold_timeout_ = ttl; } - static void setDoubleTapTimeout(int16_t ttl) { + void setDoubleTapTimeout(int16_t ttl) { double_tap_timeout_ = ttl; } @@ -187,39 +187,39 @@ class OneShot : public kaleidoscope::Plugin { // -------------------------------------------------------------------------- // Configuration variables - static uint16_t timeout_; - static uint16_t hold_timeout_; - static int16_t double_tap_timeout_; + uint16_t timeout_{2500}; + uint16_t hold_timeout_{250}; + int16_t double_tap_timeout_{-1}; // -------------------------------------------------------------------------- // State variables - static uint16_t stickable_keys_; - static bool auto_modifiers_; - static bool auto_layers_; + uint16_t stickable_keys_{uint16_t(-1)}; + bool auto_modifiers_{false}; + bool auto_layers_{false}; - static KeyAddrBitfield temp_addrs_; - static KeyAddrBitfield glue_addrs_; + KeyAddrBitfield temp_addrs_; + KeyAddrBitfield glue_addrs_; - static uint16_t start_time_; - static KeyAddr prev_key_addr_; + uint16_t start_time_{0}; + KeyAddr prev_key_addr_{invalid_key_addr}; // -------------------------------------------------------------------------- // Internal utility functions - static bool hasTimedOut(uint16_t ttl) { + bool hasTimedOut(uint16_t ttl) const { return Runtime.hasTimeExpired(start_time_, ttl); } - static bool hasDoubleTapTimedOut() { + bool hasDoubleTapTimedOut() const { // Derive the true double-tap timeout value if we're using the default. uint16_t dtto = (double_tap_timeout_ < 0) ? timeout_ : double_tap_timeout_; return hasTimedOut(dtto); } - static uint8_t getOneShotKeyIndex(Key oneshot_key); - static uint8_t getKeyIndex(Key key); - static Key decodeOneShotKey(Key oneshot_key); + uint8_t getOneShotKeyIndex(Key oneshot_key) const; + uint8_t getKeyIndex(Key key) const; + Key decodeOneShotKey(Key oneshot_key) const; - static void pressKey(KeyAddr key_addr, Key oneshot_key); - static void holdKey(KeyAddr key_addr); - static void releaseKey(KeyAddr key_addr); + void pressKey(KeyAddr key_addr, Key oneshot_key); + void holdKey(KeyAddr key_addr) const; + void releaseKey(KeyAddr key_addr); }; } // namespace plugin From a0c62ff0db4c0a2ac2ad33bf5b7088203c9a1c03 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 24 Feb 2022 11:22:54 -0600 Subject: [PATCH 4/7] Force some OneShot functions inline These declarations save some PROGMEM (and probably make things run very slightly faster). Signed-off-by: Michael Richters --- .../Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp index 38dc1472..bbb2e323 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp @@ -89,6 +89,7 @@ bool OneShot::isStickable(Key key) const { return isStickableDefault(key); } +__attribute__((always_inline)) inline bool OneShot::isStickableDefault(Key key) const { int8_t n; // If the key is either a keyboard modifier or a layer shift, we check to see @@ -369,6 +370,7 @@ void OneShot::holdKey(KeyAddr key_addr) const { Runtime.handleKeyEvent(event); } +__attribute__((always_inline)) inline void OneShot::releaseKey(KeyAddr key_addr) { glue_addrs_.clear(key_addr); temp_addrs_.clear(key_addr); From 3204034089d58562b44e3c760d46eb484c725c55 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 3 Mar 2022 22:11:46 -0600 Subject: [PATCH 5/7] Fix overridable `OneShot.isStickable(key)` function Now that it's not a `static` class function, we need to use a different invocation, and we can't declare `isStickableDefault()` with the `always_inline` attribute, or the user's override won't be able to call it because there will be nothing to link to. Signed-off-by: Michael Richters --- .../Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp | 1 - tests/issues/1061/1061.ino | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp index bbb2e323..e7d9d483 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp @@ -89,7 +89,6 @@ bool OneShot::isStickable(Key key) const { return isStickableDefault(key); } -__attribute__((always_inline)) inline bool OneShot::isStickableDefault(Key key) const { int8_t n; // If the key is either a keyboard modifier or a layer shift, we check to see diff --git a/tests/issues/1061/1061.ino b/tests/issues/1061/1061.ino index 69a4aac3..fde54690 100644 --- a/tests/issues/1061/1061.ino +++ b/tests/issues/1061/1061.ino @@ -51,10 +51,10 @@ class OneShotInsert : public Plugin { } }; -bool OneShot::isStickable(Key key) { +bool OneShot::isStickable(Key key) const { if (key == Key_LeftAlt) return false; - return OneShot::isStickableDefault(key); + return isStickableDefault(key); } } // namespace plugin From 535a4e8e9034bd80a7ae8afd932ee15364d54a2e Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Fri, 4 Mar 2022 10:58:10 -0600 Subject: [PATCH 6/7] Shift comment separating state variables from config variables Signed-off-by: Michael Richters --- .../Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h index cb425636..33b1b3c7 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h @@ -191,12 +191,12 @@ class OneShot : public kaleidoscope::Plugin { uint16_t hold_timeout_{250}; int16_t double_tap_timeout_{-1}; - // -------------------------------------------------------------------------- - // State variables uint16_t stickable_keys_{uint16_t(-1)}; bool auto_modifiers_{false}; bool auto_layers_{false}; + // -------------------------------------------------------------------------- + // State variables KeyAddrBitfield temp_addrs_; KeyAddrBitfield glue_addrs_; From 71a09389bfe6698fcbbca7e6718e939dcc587f95 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Fri, 4 Mar 2022 10:59:04 -0600 Subject: [PATCH 7/7] Use assignment-style initialization in OneShot Signed-off-by: Michael Richters --- .../src/kaleidoscope/plugin/OneShot.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h index 33b1b3c7..9a1e7e8c 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h @@ -187,21 +187,21 @@ class OneShot : public kaleidoscope::Plugin { // -------------------------------------------------------------------------- // Configuration variables - uint16_t timeout_{2500}; - uint16_t hold_timeout_{250}; - int16_t double_tap_timeout_{-1}; + uint16_t timeout_ = 2500; + uint16_t hold_timeout_ = 250; + int16_t double_tap_timeout_ = -1; - uint16_t stickable_keys_{uint16_t(-1)}; - bool auto_modifiers_{false}; - bool auto_layers_{false}; + uint16_t stickable_keys_ = -1; + bool auto_modifiers_ = false; + bool auto_layers_ = false; // -------------------------------------------------------------------------- // State variables KeyAddrBitfield temp_addrs_; KeyAddrBitfield glue_addrs_; - uint16_t start_time_{0}; - KeyAddr prev_key_addr_{invalid_key_addr}; + uint16_t start_time_ = 0; + KeyAddr prev_key_addr_ = invalid_key_addr; // -------------------------------------------------------------------------- // Internal utility functions