From 032da484bc8b55e9df450e394fae88da5e442451 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 9 Feb 2019 08:26:32 +0100 Subject: [PATCH 1/3] OneShot: Switch from 16-bit bitfields to using a struct On AVR8, 16-bit bit-operations are expensive. Switch from using 4(!) 16-bit bitfields to using a 16-element array of a carefully constructed struct. This saves us about 242 PROGMEM at the cost of 8 bytes of memory, and a tiny performance hit. Signed-off-by: Gergely Nagy --- src/kaleidoscope/plugin/OneShot.cpp | 88 +++++++++++++++++++---------- src/kaleidoscope/plugin/OneShot.h | 37 ++++++------ 2 files changed, 76 insertions(+), 49 deletions(-) diff --git a/src/kaleidoscope/plugin/OneShot.cpp b/src/kaleidoscope/plugin/OneShot.cpp index 19870d77..d0db3b8b 100644 --- a/src/kaleidoscope/plugin/OneShot.cpp +++ b/src/kaleidoscope/plugin/OneShot.cpp @@ -26,14 +26,10 @@ uint32_t OneShot::start_time_ = 0; uint16_t OneShot::time_out = 2500; uint16_t OneShot::hold_time_out = 250; int16_t OneShot::double_tap_time_out = -1; -OneShot::state_t OneShot::state_ = {0, 0}; -OneShot::state_t OneShot::sticky_state_ = {0, 0}; -OneShot::state_t OneShot::stickable_state_ = {0xFF, 0xFF}; -OneShot::state_t OneShot::pressed_state_ = {0, 0}; +OneShot::key_state_t OneShot::state_[16]; Key OneShot::prev_key_; bool OneShot::should_cancel_ = false; bool OneShot::should_cancel_stickies_ = false; -uint8_t OneShot::positions_[16]; // --- helper macros ------ @@ -41,25 +37,41 @@ uint8_t OneShot::positions_[16]; #define isModifier(key) (key.raw >= Key_LeftControl.raw && key.raw <= Key_RightGui.raw) #define isLayerKey(key) (key.flags == (KEY_FLAGS | SYNTHETIC | SWITCH_TO_KEYMAP)) -#define isOneShot(idx) (bitRead (state_.all, (idx))) -#define setOneShot(idx) (bitWrite (state_.all, idx, 1)) -#define clearOneShot(idx) (bitWrite (state_.all, idx, 0)) +#define isOneShot(idx) (state_[idx].active) +#define setOneShot(idx) (state_[idx].active = true) +#define clearOneShot(idx) (state_[idx].active = false) -#define isSticky_(idx) (bitRead (sticky_state_.all, idx)) -#define setSticky(idx) (bitWrite (sticky_state_.all, idx, 1)) -#define clearSticky(idx) bitWrite (sticky_state_.all, idx, 0) +#define isSticky_(idx) (state_[idx].sticky) +#define setSticky(idx) (state_[idx].sticky = true) +#define clearSticky(idx) (state_[idx].sticky = false) -#define setPressed(idx) bitWrite(pressed_state_.all, idx, 1) -#define clearPressed(idx) bitWrite(pressed_state_.all, idx, 0) -#define isPressed(idx) bitRead (pressed_state_.all, idx) +#define setPressed(idx) (state_[idx].pressed = true) +#define clearPressed(idx) (state_[idx].pressed = false) +#define isPressed_(idx) (state_[idx].pressed) #define isSameAsPrevious(key) (key.raw == prev_key_.raw) #define saveAsPrevious(key) prev_key_.raw = key.raw #define hasTimedOut() (millis () - start_time_ >= time_out) +bool OneShot::isPressed() { + for (uint8_t i = 0; i < sizeof(state_); i++) { + if (state_[i].pressed) + return true; + } + return false; +} + +bool OneShot::isSticky() { + for (uint8_t i = 0; i < sizeof(state_); i++) { + if (state_[i].sticky) + return true; + } + return false; +} + bool OneShot::isStickable(Key key) { - return bitRead(stickable_state_.all, key.raw - ranges::OS_FIRST); + return state_[key.raw - ranges::OS_FIRST].stickable; } void OneShot::positionToCoords(uint8_t pos, byte *row, byte *col) { @@ -80,7 +92,7 @@ void OneShot::injectNormalKey(uint8_t idx, uint8_t key_state) { key.keyCode = LAYER_SHIFT_OFFSET + idx - 8; } - positionToCoords(positions_[idx], &row, &col); + positionToCoords(state_[idx].position, &row, &col); handleKeyswitchEvent(key, row, col, key_state | INJECTED); } @@ -99,7 +111,7 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col if (keyState & INJECTED) return EventHandlerResult::OK; - if (!state_.all) { + if (!isActive()) { if (!isOneShotKey(mapped_key)) { return EventHandlerResult::OK; } @@ -108,7 +120,7 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col clearPressed(idx); } else if (keyToggledOn(keyState)) { start_time_ = millis(); - positions_[idx] = row * COLS + col; + state_[idx].position = row * COLS + col; setPressed(idx); setOneShot(idx); saveAsPrevious(mapped_key); @@ -148,7 +160,7 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col } else { start_time_ = millis(); - positions_[idx] = row * COLS + col; + state_[idx].position = row * COLS + col; setOneShot(idx); saveAsPrevious(mapped_key); @@ -173,7 +185,7 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col } EventHandlerResult OneShot::beforeReportingState() { - if (!state_.all) + if (!isActive()) return EventHandlerResult::OK; for (uint8_t i = 0; i < 8; i++) { @@ -186,7 +198,7 @@ EventHandlerResult OneShot::beforeReportingState() { } EventHandlerResult OneShot::afterEachCycle() { - if (!state_.all) + if (!isActive()) return EventHandlerResult::OK; if (hasTimedOut()) @@ -203,7 +215,7 @@ EventHandlerResult OneShot::afterEachCycle() { cancelOneShot(i); clearPressed(i); } - } else if (isOneShot(i) && !isPressed(i)) { + } else if (isOneShot(i) && !isPressed_(i)) { is_cancelled = true; cancelOneShot(i); } @@ -225,13 +237,21 @@ void OneShot::inject(Key mapped_key, uint8_t key_state) { // --- glue code --- bool OneShot::isActive(void) { - return (state_.all && !hasTimedOut()) || (pressed_state_.all) || (sticky_state_.all); + for (uint8_t i = 0; i < 16; i++) { + if ((state_[i].active && !hasTimedOut()) || + state_[i].pressed || + state_[i].sticky) + return true; + } + return false; } bool OneShot::isActive(Key key) { uint8_t idx = key.raw - ranges::OS_FIRST; - return (bitRead(state_.all, idx) && !hasTimedOut()) || (isPressed(idx)) || (isSticky_(idx)); + return (state_[idx].active && !hasTimedOut()) || + state_[idx].pressed || + state_[idx].sticky; } bool OneShot::isSticky(Key key) { @@ -254,28 +274,36 @@ void OneShot::cancel(bool with_stickies) { void OneShot::enableStickability(Key key) { if (key >= ranges::OS_FIRST && key <= ranges::OS_LAST) - bitSet(stickable_state_.all, (key.raw - ranges::OS_FIRST)); + state_[key.raw - ranges::OS_FIRST].stickable = true; } void OneShot::disableStickability(Key key) { if (key >= ranges::OS_FIRST && key <= ranges::OS_LAST) - bitClear(stickable_state_.all, (key.raw - ranges::OS_FIRST)); + state_[key.raw - ranges::OS_FIRST].stickable = false; } void OneShot::enableStickabilityForModifiers() { - stickable_state_.mods = 0xFF; + for (uint8_t i = 0; i < 8; i++) { + state_[i].stickable = true; + } } void OneShot::enableStickabilityForLayers() { - stickable_state_.layers = 0xFF; + for (uint8_t i = 8; i < 16; i++) { + state_[i].stickable = true; + } } void OneShot::disableStickabilityForModifiers() { - stickable_state_.mods = 0; + for (uint8_t i = 0; i < 8; i++) { + state_[i].stickable = false; + } } void OneShot::disableStickabilityForLayers() { - stickable_state_.layers = 0; + for (uint8_t i = 8; i < 16; i++) { + state_[i].stickable = false; + } } } diff --git a/src/kaleidoscope/plugin/OneShot.h b/src/kaleidoscope/plugin/OneShot.h index b474f651..9b0d07f2 100644 --- a/src/kaleidoscope/plugin/OneShot.h +++ b/src/kaleidoscope/plugin/OneShot.h @@ -28,21 +28,22 @@ namespace plugin { class OneShot : public kaleidoscope::Plugin { public: - OneShot(void) {} + OneShot(void) { + for (uint8_t i = 0; i < 16; i++) { + state_[i].stickable = true; + } + } static bool isOneShotKey(Key key) { return (key.raw >= kaleidoscope::ranges::OS_FIRST && key.raw <= kaleidoscope::ranges::OS_LAST); } static bool isActive(void); - static bool isPressed() { - return !!pressed_state_.all; - } - static bool isSticky() { - return !!sticky_state_.all; - } static bool isActive(Key key); + static bool isPressed(); + static bool isSticky(); static bool isSticky(Key key); static void cancel(bool with_stickies = false); + static uint16_t time_out; static int16_t double_tap_time_out; static uint16_t hold_time_out; @@ -78,22 +79,20 @@ class OneShot : public kaleidoscope::Plugin { void inject(Key mapped_key, uint8_t key_state); private: - typedef union { - struct { - uint8_t mods; - uint8_t layers; - }; - uint16_t all; - } state_t; + typedef struct { + bool active: 1; + bool pressed: 1; + bool stickable: 1; + bool sticky: 1; + uint8_t __reserved: 4; + uint8_t position; + } key_state_t; + static key_state_t state_[16]; + static uint32_t start_time_; - static state_t state_; - static state_t sticky_state_; - static state_t stickable_state_; - static state_t pressed_state_; static Key prev_key_; static bool should_cancel_; static bool should_cancel_stickies_; - static uint8_t positions_[16]; static void positionToCoords(uint8_t pos, byte *row, byte *col); From 926cb07f85abb9abc1cfe052ef2d68436cd8c397 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 9 Feb 2019 09:21:03 +0100 Subject: [PATCH 2/3] OneShot: Code cleanup: drop the helper macros Now that we're using a struct with descriptive member names, drop the helper macros that made using bitfield manipulation readable. The code's readable without them by now. Signed-off-by: Gergely Nagy --- src/kaleidoscope/plugin/OneShot.cpp | 91 +++++++++++------------------ src/kaleidoscope/plugin/OneShot.h | 7 +++ 2 files changed, 42 insertions(+), 56 deletions(-) diff --git a/src/kaleidoscope/plugin/OneShot.cpp b/src/kaleidoscope/plugin/OneShot.cpp index d0db3b8b..458f5c73 100644 --- a/src/kaleidoscope/plugin/OneShot.cpp +++ b/src/kaleidoscope/plugin/OneShot.cpp @@ -31,29 +31,6 @@ Key OneShot::prev_key_; bool OneShot::should_cancel_ = false; bool OneShot::should_cancel_stickies_ = false; -// --- helper macros ------ - -#define isOneShotKey(key) (key.raw >= ranges::OS_FIRST && key.raw <= ranges::OS_LAST) -#define isModifier(key) (key.raw >= Key_LeftControl.raw && key.raw <= Key_RightGui.raw) -#define isLayerKey(key) (key.flags == (KEY_FLAGS | SYNTHETIC | SWITCH_TO_KEYMAP)) - -#define isOneShot(idx) (state_[idx].active) -#define setOneShot(idx) (state_[idx].active = true) -#define clearOneShot(idx) (state_[idx].active = false) - -#define isSticky_(idx) (state_[idx].sticky) -#define setSticky(idx) (state_[idx].sticky = true) -#define clearSticky(idx) (state_[idx].sticky = false) - -#define setPressed(idx) (state_[idx].pressed = true) -#define clearPressed(idx) (state_[idx].pressed = false) -#define isPressed_(idx) (state_[idx].pressed) - -#define isSameAsPrevious(key) (key.raw == prev_key_.raw) -#define saveAsPrevious(key) prev_key_.raw = key.raw - -#define hasTimedOut() (millis () - start_time_ >= time_out) - bool OneShot::isPressed() { for (uint8_t i = 0; i < sizeof(state_); i++) { if (state_[i].pressed) @@ -101,7 +78,7 @@ void OneShot::activateOneShot(uint8_t idx) { } void OneShot::cancelOneShot(uint8_t idx) { - clearOneShot(idx); + state_[idx].active = false; injectNormalKey(idx, WAS_PRESSED); } @@ -112,18 +89,18 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col return EventHandlerResult::OK; if (!isActive()) { - if (!isOneShotKey(mapped_key)) { + if (!isOneShotKey_(mapped_key)) { return EventHandlerResult::OK; } if (keyToggledOff(keyState)) { - clearPressed(idx); + state_[idx].pressed = false; } else if (keyToggledOn(keyState)) { - start_time_ = millis(); + start_time_ = Kaleidoscope.millisAtCycleStart(); state_[idx].position = row * COLS + col; - setPressed(idx); - setOneShot(idx); - saveAsPrevious(mapped_key); + state_[idx].pressed = true; + state_[idx].active = true; + prev_key_ = mapped_key; activateOneShot(idx); } @@ -131,38 +108,38 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col return EventHandlerResult::EVENT_CONSUMED; } - if (isOneShotKey(mapped_key)) { - if (isSticky_(idx)) { + if (isOneShotKey_(mapped_key)) { + if (state_[idx].sticky) { if (keyToggledOn(keyState)) { // maybe on _off instead? - saveAsPrevious(mapped_key); - clearSticky(idx); + prev_key_ = mapped_key; + state_[idx].sticky = false; cancelOneShot(idx); should_cancel_ = false; } } else { if (keyToggledOff(keyState)) { - clearPressed(idx); - if ((millis() - start_time_) >= hold_time_out) { + state_[idx].pressed = false; + if ((Kaleidoscope.millisAtCycleStart() - start_time_) >= hold_time_out) { cancelOneShot(idx); should_cancel_ = false; } } if (keyToggledOn(keyState)) { - setPressed(idx); - - if (isSameAsPrevious(mapped_key) && isStickable(mapped_key)) { - if ((millis() - start_time_) <= ((double_tap_time_out == -1) ? time_out : double_tap_time_out)) { - setSticky(idx); + state_[idx].pressed = true; - saveAsPrevious(mapped_key); + if (prev_key_ == mapped_key && isStickable(mapped_key)) { + if ((Kaleidoscope.millisAtCycleStart() - start_time_) <= + ((double_tap_time_out == -1) ? time_out : double_tap_time_out)) { + state_[idx].sticky = true; + prev_key_ = mapped_key; } } else { - start_time_ = millis(); + start_time_ = Kaleidoscope.millisAtCycleStart(); state_[idx].position = row * COLS + col; - setOneShot(idx); - saveAsPrevious(mapped_key); + state_[idx].active = true; + prev_key_ = mapped_key; activateOneShot(idx); } @@ -175,8 +152,9 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col // ordinary key here, with some event if (keyIsPressed(keyState)) { - saveAsPrevious(mapped_key); - if (!isModifier(mapped_key) && !isLayerKey(mapped_key)) { + prev_key_ = mapped_key; + if (!(mapped_key.raw >= Key_LeftControl.raw && mapped_key.raw <= Key_RightGui.raw) && + !(mapped_key.flags == (KEY_FLAGS | SYNTHETIC | SWITCH_TO_KEYMAP))) { should_cancel_ = true; } } @@ -189,7 +167,7 @@ EventHandlerResult OneShot::beforeReportingState() { return EventHandlerResult::OK; for (uint8_t i = 0; i < 8; i++) { - if (isOneShot(i)) { + if (state_[i].active) { activateOneShot(i); } } @@ -208,14 +186,14 @@ EventHandlerResult OneShot::afterEachCycle() { for (uint8_t i = 0; i < 32; i++) { if (should_cancel_) { - if (isSticky_(i)) { + if (state_[i].sticky) { if (should_cancel_stickies_) { is_cancelled = true; - clearSticky(i); + state_[i].sticky = false; cancelOneShot(i); - clearPressed(i); + state_[i].pressed = false; } - } else if (isOneShot(i) && !isPressed_(i)) { + } else if (state_[i].active && !state_[i].pressed) { is_cancelled = true; cancelOneShot(i); } @@ -250,21 +228,22 @@ bool OneShot::isActive(Key key) { uint8_t idx = key.raw - ranges::OS_FIRST; return (state_[idx].active && !hasTimedOut()) || - state_[idx].pressed || - state_[idx].sticky; + state_[idx].pressed || + state_[idx].sticky; } bool OneShot::isSticky(Key key) { uint8_t idx = key.raw - ranges::OS_FIRST; - return (isSticky_(idx)); + return state_[idx].sticky; } bool OneShot::isModifierActive(Key key) { if (key.raw < Key_LeftControl.raw || key.raw > Key_RightGui.raw) return false; - return isOneShot(key.keyCode - Key_LeftControl.keyCode); + uint8_t idx = key.keyCode - Key_LeftControl.keyCode; + return state_[idx].active; } void OneShot::cancel(bool with_stickies) { diff --git a/src/kaleidoscope/plugin/OneShot.h b/src/kaleidoscope/plugin/OneShot.h index 9b0d07f2..b8f04c61 100644 --- a/src/kaleidoscope/plugin/OneShot.h +++ b/src/kaleidoscope/plugin/OneShot.h @@ -99,6 +99,13 @@ class OneShot : public kaleidoscope::Plugin { static void injectNormalKey(uint8_t idx, uint8_t key_state); static void activateOneShot(uint8_t idx); static void cancelOneShot(uint8_t idx); + + static bool isOneShotKey_(Key key) { + return key.raw >= ranges::OS_FIRST && key.raw <= ranges::OS_LAST; + } + static bool hasTimedOut() { + return Kaleidoscope.millisAtCycleStart() - start_time_ >= time_out; + } }; } } From a4af4f4a2b62de483d5f96cb9c4c4e9d7c095ef7 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 9 Feb 2019 09:21:15 +0100 Subject: [PATCH 3/3] OneShot: Remove some obsolete optimizations In `beforeReportingState()` and `afterEachCycle()` we used to return early if no OneShots were active. This was easy to do when we were using a bitfield, we'd just tested for non-zero. Since we're using an array now, this check is more expensive, and the extra work negates any benefit of returning early. For this reason, these early returns are removed. Signed-off-by: Gergely Nagy --- src/kaleidoscope/plugin/OneShot.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/kaleidoscope/plugin/OneShot.cpp b/src/kaleidoscope/plugin/OneShot.cpp index 458f5c73..38ad85ef 100644 --- a/src/kaleidoscope/plugin/OneShot.cpp +++ b/src/kaleidoscope/plugin/OneShot.cpp @@ -163,9 +163,6 @@ EventHandlerResult OneShot::onKeyswitchEvent(Key &mapped_key, byte row, byte col } EventHandlerResult OneShot::beforeReportingState() { - if (!isActive()) - return EventHandlerResult::OK; - for (uint8_t i = 0; i < 8; i++) { if (state_[i].active) { activateOneShot(i); @@ -176,9 +173,6 @@ EventHandlerResult OneShot::beforeReportingState() { } EventHandlerResult OneShot::afterEachCycle() { - if (!isActive()) - return EventHandlerResult::OK; - if (hasTimedOut()) cancel();