From c9a98ecb261e77fbd137bda754dc648ac158427f Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Wed, 26 May 2021 19:28:25 -0500 Subject: [PATCH 1/5] Add testcases for issue #423 Signed-off-by: Michael Richters --- tests/issues/423/423.ino | 64 ++++++++++++++++++ tests/issues/423/sketch.json | 6 ++ tests/issues/423/test.ktest | 125 +++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+) create mode 100644 tests/issues/423/423.ino create mode 100644 tests/issues/423/sketch.json create mode 100644 tests/issues/423/test.ktest diff --git a/tests/issues/423/423.ino b/tests/issues/423/423.ino new file mode 100644 index 00000000..dcf02840 --- /dev/null +++ b/tests/issues/423/423.ino @@ -0,0 +1,64 @@ +/* -*- 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 +#include +#include + +// *INDENT-OFF* +KEYMAPS( + [0] = KEYMAP_STACKED + ( + Key_Spacebar, Key_A, ___, ___, ___, ___, ___, + TD(0), OSM(LeftShift), ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___, + + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___ + ), +) +// *INDENT-ON* + +void tapDanceAction(uint8_t tap_dance_index, + KeyAddr key_addr, + uint8_t tap_count, + kaleidoscope::plugin::TapDance::ActionType tap_dance_action) { + switch (tap_dance_index) { + case 0: + return tapDanceActionKeys(tap_count, tap_dance_action, + Key_Period, LSHIFT(Key_1)); + default: + break; + } +} + +KALEIDOSCOPE_INIT_PLUGINS(OneShot, TapDance); + +void setup() { + Kaleidoscope.setup(); + TapDance.time_out = 25; +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/issues/423/sketch.json b/tests/issues/423/sketch.json new file mode 100644 index 00000000..43dc4c7e --- /dev/null +++ b/tests/issues/423/sketch.json @@ -0,0 +1,6 @@ +{ + "cpu": { + "fqbn": "keyboardio:virtual:model01", + "port": "" + } +} \ No newline at end of file diff --git a/tests/issues/423/test.ktest b/tests/issues/423/test.ktest new file mode 100644 index 00000000..c734e0e9 --- /dev/null +++ b/tests/issues/423/test.ktest @@ -0,0 +1,125 @@ +VERSION 1 + +KEYSWITCH SPACE 0 0 +KEYSWITCH A 0 1 +KEYSWITCH TD_0 1 0 +KEYSWITCH OS_SHIFT 1 1 + +# ============================================================================== +NAME Back and forth + +RUN 4 ms +PRESS TD_0 +RUN 1 cycle + +RUN 4 ms +RELEASE TD_0 +RUN 1 cycle + +RUN 4 ms +PRESS OS_SHIFT +RUN 1 cycle +EXPECT keyboard-report Key_Period # report: { 37 } +EXPECT keyboard-report empty +EXPECT keyboard-report Key_LeftShift # report: { e1 } + +RUN 4 ms +RELEASE OS_SHIFT +RUN 1 cycle + +RUN 4 ms +PRESS TD_0 +RUN 1 cycle + +RUN 4 ms +RELEASE TD_0 +RUN 1 cycle + +# ------------------------------------------------------------------------------ +# Next, we press `space`, triggering both the resolution of the TapDance key and +# the release of the OneShot key, in that order. +RUN 4 ms +PRESS SPACE +RUN 1 cycle +# First, the TapDance key is resolved, adding `.` to the report. This event also +# triggers the release of the OneShot key, which shouldn't happen until after +# the `.` press is processed. +EXPECT keyboard-report Key_LeftShift Key_Period # report: { 37 e1 } +# Now the OneShot key is released, removing `shift` from the report. +EXPECT keyboard-report Key_Period # report: { 37 } +# The TapDance `.` key has been released, so its release comes next. +EXPECT keyboard-report empty # report: { } +# Finally, we get the report for the press of the `space` key. +EXPECT keyboard-report Key_Spacebar # report: { 2c } + +RUN 4 ms +RELEASE SPACE +RUN 1 cycle +EXPECT keyboard-report empty + +RUN 4 ms +PRESS OS_SHIFT +RUN 1 cycle +EXPECT keyboard-report Key_LeftShift # report: { e1 } + +RUN 4 ms +RELEASE OS_SHIFT +RUN 1 cycle + +RUN 4 ms +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_LeftShift Key_A # report: { 4 e1 } +EXPECT keyboard-report Key_A # report: { 4 } + +RUN 4 ms +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty + +RUN 5 ms + +# ============================================================================== +NAME Single rollover + +RUN 4 ms +PRESS TD_0 +RUN 1 cycle + +RUN 4 ms +PRESS SPACE +RUN 1 cycle +EXPECT keyboard-report Key_Period # report: { 37 } +EXPECT keyboard-report Key_Period Key_Spacebar # report: { 37 2c } + +RUN 4 ms +PRESS OS_SHIFT +RUN 1 cycle +EXPECT keyboard-report Key_Period Key_Spacebar Key_LeftShift # report: { 2c 37 e1 } + +RUN 4 ms +RELEASE TD_0 +RUN 1 cycle +EXPECT keyboard-report Key_Spacebar Key_LeftShift # report: { 2c e1 } + +RUN 4 ms +RELEASE SPACE +RUN 1 cycle +EXPECT keyboard-report Key_LeftShift # report: { e1 } + +RUN 4 ms +PRESS A +RUN 1 cycle +EXPECT keyboard-report Key_LeftShift Key_A # report: { 4 e1 } + +RUN 4 ms +RELEASE OS_SHIFT +RUN 1 cycle +EXPECT keyboard-report Key_A # report: { 4 } + +RUN 4 ms +RELEASE A +RUN 1 cycle +EXPECT keyboard-report empty + +RUN 5 ms From 92f2f582f1b00adb7e494429ef69bc5fce3983bd Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 27 May 2021 10:58:11 -0500 Subject: [PATCH 2/5] Add `afterReportingState()` event handler function This event handler is useful for plugins that need to react to events, but should wait until after those events are fully processed before doing so. This is useful for OneShot, which needs to keep keys active until after events that trigger their release. The `afterEachCycle()` hook is unfortunately insufficient for this purpose, because the same event could trigger multiple plugins (e.g. TapDance & OneShot) to resolve events, and the OneShot should apply only to the first ensuing report. Signed-off-by: Michael Richters --- docs/api-reference/event-handler-hooks.md | 8 ++++++++ src/kaleidoscope/Runtime.cpp | 5 +++++ src/kaleidoscope/event_handlers.h | 17 +++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/docs/api-reference/event-handler-hooks.md b/docs/api-reference/event-handler-hooks.md index b86f0837..707c67ed 100644 --- a/docs/api-reference/event-handler-hooks.md +++ b/docs/api-reference/event-handler-hooks.md @@ -183,6 +183,14 @@ abortable. That is, if it returns a result other than `OK` it will stop the subsequent handlers from getting called, and if it returns `ABORT`, it will also stop the report from being sent.] +### `afterReportingState(const KeyEvent &event)` + +This gets called after the HID report is sent. This handler allows a plugin to +react to an event, but wait until after that event has been fully processed to +do so. For example, the OneShot plugin releases keys that are in the "one-shot" +state in response to key press events, but it does so after those triggering +press events take place. + ## Other events ### `onLayerChange()` diff --git a/src/kaleidoscope/Runtime.cpp b/src/kaleidoscope/Runtime.cpp index c87df6e1..82b0df83 100644 --- a/src/kaleidoscope/Runtime.cpp +++ b/src/kaleidoscope/Runtime.cpp @@ -219,6 +219,11 @@ Runtime_::handleKeyEvent(KeyEvent event) { // Finally, send the new keyboard report sendKeyboardReport(event); + + // Now that the report has been sent, let plugins act on it after the fact. + // This is useful for plugins that need to react to an event, but must wait + // until after that event is processed to do so. + Hooks::afterReportingState(event); } // ---------------------------------------------------------------------------- diff --git a/src/kaleidoscope/event_handlers.h b/src/kaleidoscope/event_handlers.h index b3765601..11ca03e9 100644 --- a/src/kaleidoscope/event_handlers.h +++ b/src/kaleidoscope/event_handlers.h @@ -264,6 +264,19 @@ class SignatureCheckDummy {}; (const KeyEvent &event), __NL__ \ (event), ##__VA_ARGS__) __NL__ \ __NL__ \ + /* Called after reporting our state to the host. This is the last */ __NL__ \ + /* point at which a plugin can do something in response to an event */ __NL__ \ + /* before the next event is processed, if multiple events occur in */ __NL__ \ + /* and are processed in a single cycle (usually due to delayed */ __NL__ \ + /* events or generated events). */ __NL__ \ + OPERATION(afterReportingState, __NL__ \ + 1, __NL__ \ + _CURRENT_IMPLEMENTATION, __NL__ \ + _NOT_ABORTABLE, __NL__ \ + (),(),(), /* non template */ __NL__ \ + (const KeyEvent &event), __NL__ \ + (event), ##__VA_ARGS__) __NL__ \ + __NL__ \ /* Called at the very end of a cycle, after everything's */ __NL__ \ /* said and done. */ __NL__ \ OPERATION(afterEachCycle, __NL__ \ @@ -351,6 +364,10 @@ class SignatureCheckDummy {}; OP(beforeReportingState, 2) __NL__ \ END(beforeReportingState, 1, 2) __NL__ \ __NL__ \ + START(afterReportingState, 1) __NL__ \ + OP(afterReportingState, 1) __NL__ \ + END(afterReportingState, 1) __NL__ \ + __NL__ \ START(afterEachCycle, 1) __NL__ \ OP(afterEachCycle, 1) __NL__ \ END(afterEachCycle, 1) __NL__ \ From 1ff881a86be862225648005dbda39454a1473614 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 27 May 2021 10:59:01 -0500 Subject: [PATCH 3/5] Add `afterReportingState()` handler to OneShot If multiple events are processed in a single cycle, we want a OneShot key whose release is triggered by the first one to only affect that key, and not subsequent ones. For example, if we tap `OSM(LeftShift)`, then `TD(0)`, then `Key_X`, the OneShot shift should only apply to the output of the TapDance key, not the `x`. Signed-off-by: Michael Richters --- .../Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp | 5 +++++ .../Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h | 1 + 2 files changed, 6 insertions(+) diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp index 46234785..63b6b851 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.cpp @@ -321,6 +321,11 @@ EventHandlerResult OneShot::onKeyEvent(KeyEvent& event) { return EventHandlerResult::OK; } +// ---------------------------------------------------------------------------- +EventHandlerResult OneShot::afterReportingState(const KeyEvent& event) { + return afterEachCycle(); +} + // ---------------------------------------------------------------------------- EventHandlerResult OneShot::afterEachCycle() { diff --git a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h index 5ad18b1d..f626c4a7 100644 --- a/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h +++ b/plugins/Kaleidoscope-OneShot/src/kaleidoscope/plugin/OneShot.h @@ -222,6 +222,7 @@ class OneShot : public kaleidoscope::Plugin { EventHandlerResult onNameQuery(); EventHandlerResult onKeyEvent(KeyEvent &event); + EventHandlerResult afterReportingState(const KeyEvent &event); EventHandlerResult afterEachCycle(); private: From e334be135df06695011c64a8e6beb02c5ad7d4aa Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 27 May 2021 11:55:03 -0500 Subject: [PATCH 4/5] Add testcase for OneShot/TapDance/Macros compatibility OneShot keys should apply to all the key events generated by a Macros key, not just the first one, even if the Macros key is injected by TapDance. Signed-off-by: Michael Richters --- tests/issues/423/423.ino | 18 +++++++++++-- tests/issues/423/test.ktest | 52 +++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/tests/issues/423/423.ino b/tests/issues/423/423.ino index dcf02840..0cf1d584 100644 --- a/tests/issues/423/423.ino +++ b/tests/issues/423/423.ino @@ -15,6 +15,7 @@ */ #include +#include #include #include @@ -46,13 +47,26 @@ void tapDanceAction(uint8_t tap_dance_index, switch (tap_dance_index) { case 0: return tapDanceActionKeys(tap_count, tap_dance_action, - Key_Period, LSHIFT(Key_1)); + Key_Period, M(0), LSHIFT(Key_1)); default: break; } } -KALEIDOSCOPE_INIT_PLUGINS(OneShot, TapDance); +const macro_t *macroAction(uint8_t macro_id, KeyEvent &event) { + if (keyToggledOn(event.state)) { + switch (macro_id) { + case 0: + Macros.type(PSTR("abc")); + break; + default: + break; + } + } + return MACRO_NONE; +} + +KALEIDOSCOPE_INIT_PLUGINS(Macros, OneShot, TapDance); void setup() { Kaleidoscope.setup(); diff --git a/tests/issues/423/test.ktest b/tests/issues/423/test.ktest index c734e0e9..fb5d1075 100644 --- a/tests/issues/423/test.ktest +++ b/tests/issues/423/test.ktest @@ -122,4 +122,56 @@ RELEASE A RUN 1 cycle EXPECT keyboard-report empty +RUN 5 ms + +# ============================================================================== +NAME OSM applies to whole Macro + +RUN 4 ms +PRESS OS_SHIFT +RUN 1 cycle +EXPECT keyboard-report Key_LeftShift # report: { e1 } + +RUN 4 ms +RELEASE OS_SHIFT +RUN 1 cycle + +RUN 4 ms +PRESS TD_0 +RUN 1 cycle + +RUN 4 ms +RELEASE TD_0 +RUN 1 cycle + +RUN 4 ms +PRESS TD_0 +RUN 1 cycle + +RUN 4 ms +RELEASE TD_0 +RUN 1 cycle + +# ------------------------------------------------------------------------------ +# Next, we press `space`, triggering both the resolution of the TapDance key and +# the release of the OneShot key, in that order. +RUN 4 ms +PRESS SPACE +RUN 1 cycle +EXPECT keyboard-report Key_LeftShift Key_A # report: { 4 e1 } +EXPECT keyboard-report Key_LeftShift # report: { e1 } +EXPECT keyboard-report Key_LeftShift Key_B # report: { 5 e1 } +EXPECT keyboard-report Key_LeftShift # report: { e1 } +EXPECT keyboard-report Key_LeftShift Key_C # report: { 6 e1 } +EXPECT keyboard-report Key_LeftShift # report: { e1 } +EXPECT keyboard-report empty # report: { } +EXPECT keyboard-report Key_Spacebar # report: { 2c } + +RUN 4 ms +RELEASE SPACE +RUN 1 cycle +EXPECT keyboard-report empty + + + RUN 5 ms From dbeb915196149d957b8619a09019bff6fcb9b2bd Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Sun, 30 May 2021 09:32:57 -0500 Subject: [PATCH 5/5] Add note about `afterReportingState()` to UPGRADING.md Signed-off-by: Michael Richters --- docs/UPGRADING.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index ae3963a7..49b21c38 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,7 @@ If any of this does not make sense to you, or you have trouble updating your .in * [Upgrade notes](#upgrade-notes) + [New features](#new-features) + - [New event handler](#new-event-handler) - [Event-driven main loop](#event-driven-main-loop) - [Keyboard state array](#keyboard-state-array) - [New build system](#new-build-system) @@ -37,6 +38,10 @@ any API we've included in a release. Typically, this means that any code that us ## New features +### New event handler + +One more `KeyEvent` handler has been added: `afterReportingState(const KeyEvent &event)`. This handler gets called after HID reports are sent for an event, providing a point for plugins to act after an event has been fully processed by `Runtime.handleKeyEvent()`. + ### Event-driven main loop Kaleidoscope's main loop has been rewritten. It now responds to key toggle-on and toggle-off events, dealing with one event at a time (and possibly more than one in a given cycle). Instead of sending a keyboard HID report at the end of every scan cycle (and letting the HID module suppress duplicates), it now only sends HID reports in response to input events.