diff --git a/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp b/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp index 72ecc8c4..bc9a63dc 100644 --- a/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp +++ b/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp @@ -88,9 +88,6 @@ EventHandlerResult Qukeys::afterEachCycle() { return EventHandlerResult::OK; } - // Process as many events as we can from the queue. - while (processQueue()); - // If we get here, that means that the first event in the queue is a qukey // press. All that's left to do is to check if it's been held long enough that // it has timed out. @@ -101,6 +98,10 @@ EventHandlerResult Qukeys::afterEachCycle() { queue_head_.primary_key : queue_head_.alternate_key; flushEvent(event_key); } + + // Process as many events as we can from the queue. + while (processQueue()); + return EventHandlerResult::OK; } diff --git a/src/kaleidoscope/KeyAddrEventQueue.h b/src/kaleidoscope/KeyAddrEventQueue.h index 326b84e5..214e535a 100644 --- a/src/kaleidoscope/KeyAddrEventQueue.h +++ b/src/kaleidoscope/KeyAddrEventQueue.h @@ -103,6 +103,8 @@ class KeyAddrEventQueue { // rather than using a ring buffer because we expect it will be called much // less often than the queue is searched via a for loop. void remove(uint8_t n = 0) { + if (n >= length_ || length_ == 0) + return; // assert(length > n); --length_; for (uint8_t i{n}; i < length_; ++i) { diff --git a/tests/issues/1107/1107.ino b/tests/issues/1107/1107.ino new file mode 100644 index 00000000..c3689c05 --- /dev/null +++ b/tests/issues/1107/1107.ino @@ -0,0 +1,71 @@ +/* -*- mode: c++ -*- + * Copyright (C) 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 + * 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 + ( + XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX + ,XXX + + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX + ,XXX + ) +) // KEYMAPS( +// *INDENT-ON* + +#include + +namespace kaleidoscope { +namespace plugin { + +class QueueLeaker : public Plugin { + public: + EventHandlerResult afterEachCycle() { + // Shift queue without checking to be sure that queue is not empty. If + // KeyAddrEventQueue uses an unsigned integer for queue length, this will + // result in some bad behavior. + queue_.shift(); + return EventHandlerResult::OK; + } + private: + KeyAddrEventQueue<8> queue_; +}; + +} // namespace plugin +} // namespace kaleidoscope + +kaleidoscope::plugin::QueueLeaker QueueLeaker; + +KALEIDOSCOPE_INIT_PLUGINS(QueueLeaker); + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/issues/1107/sketch.json b/tests/issues/1107/sketch.json new file mode 100644 index 00000000..8cc86922 --- /dev/null +++ b/tests/issues/1107/sketch.json @@ -0,0 +1,6 @@ +{ + "cpu": { + "fqbn": "keyboardio:virtual:model01", + "port": "" + } +} diff --git a/tests/issues/1107/test/testcase.cpp b/tests/issues/1107/test/testcase.cpp new file mode 100644 index 00000000..ea4b3e28 --- /dev/null +++ b/tests/issues/1107/test/testcase.cpp @@ -0,0 +1,99 @@ +/* -*- mode: c++ -*- + * Copyright (C) 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 + * 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 "testing/setup-googletest.h" + +SETUP_GOOGLETEST(); + +namespace kaleidoscope { +namespace testing { +namespace { + +constexpr uint32_t t_0{0}; +constexpr uint32_t t_1{1}; +constexpr uint32_t t_10{10}; + +using ::testing::IsEmpty; + +class KeyAddrEventQueueLeak : public VirtualDeviceTest { + public: + void assertTimeElapses(uint32_t interval) { + // Record time at start + uint32_t start = Kaleidoscope.millisAtCycleStart(); + + // Run cycles until the given amount of time has elapsed + sim_.RunForMillis(interval); + + // Record time at end + uint32_t end = Kaleidoscope.millisAtCycleStart(); + + ASSERT_EQ((end - start), interval); + } +}; + +TEST_F(KeyAddrEventQueueLeak, TimeIsConsistent) { + assertTimeElapses(1); + assertTimeElapses(1); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); +} + +} // namespace +} // namespace testing +} // namespace kaleidoscope