From c9534155e6849442b3e76033235ed5f73f127fce Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 28 Jan 2021 09:32:21 -0600 Subject: [PATCH 1/3] Clarify the Macros plugin `Key` value range This replaces the magic number `24576` with the old definition of the start of the Macros plugin's `Key` range, to make it clear that it will match bitwise tests for "not a plugin key" (the two high bits are both zeros) and the `SYNTHETIC` flags bit. I also elaborated on the reasons for keeping the existing `Key` values stable and added a comment addressing the more likely case where new values are being added, rather than fighting the last war and focusing only on the one legacy plugin range that exists outside the canonical plugin range, whose values have already been incorporated here. Signed-off-by: Michael Richters --- .../src/Kaleidoscope-Ranges.h | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/plugins/Kaleidoscope-Ranges/src/Kaleidoscope-Ranges.h b/plugins/Kaleidoscope-Ranges/src/Kaleidoscope-Ranges.h index d60ff94d..75a87388 100644 --- a/plugins/Kaleidoscope-Ranges/src/Kaleidoscope-Ranges.h +++ b/plugins/Kaleidoscope-Ranges/src/Kaleidoscope-Ranges.h @@ -17,6 +17,9 @@ #pragma once +// Included for definition of legacy Macros plugin key range: +#include "kaleidoscope/key_defs.h" + namespace kaleidoscope { namespace ranges { @@ -24,13 +27,25 @@ namespace ranges { // // When adding, removing, or changing ranges, make sure that existing ranges are // never accidentally moved. If migrating keycodes that weren't previously using -// the rang system, make sure you keep the old keycodes, even if they short -// before ranges::FIRST, for the sake of remaining backwards compatible with -// existing keymaps. +// the range system, make sure you don't change the `Key` values. If an existing +// `Key` value is changed, it won't be a problem for the Kaleidoscope sketch +// itself, but it will cause problems for keymap entries stored in EEPROM +// (i.e. Chrysalis keymap layers), which will not get updated by flashing the +// firmware. +// +// When adding new `Key` values for plugins, to keep them backwards-compatible, +// they must be added at the end of the range below (just before `SAFE_START`), +// even if those values are logically related to existing ones. This is +// important for compatibility with existing Chrysalis keymaps, despite the fact +// that it makes the code more obtuse here. + enum : uint16_t { // Macro ranges pre-date Kaleidoscope-Ranges, so they're coming before // ranges::FIRST, because we want to keep the keycodes backwards compatible. - MACRO_FIRST = 24576, + // This is undesirable, because it prevents us from making a clear distinction + // between plugin key values and core key values. The magic number + // `0b00100000` is the old `IS_MACRO` key flags bit. + MACRO_FIRST = (SYNTHETIC | 0b00100000) << 8, MACRO_LAST = MACRO_FIRST + 255, FIRST = 0xc000, From 1a5934ef617caa7ee4cd92be744d8fa5834ceef7 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Mon, 1 Feb 2021 11:34:59 -0600 Subject: [PATCH 2/3] Add the plugins dir to includes for testcases This makes it possible for a testcase to include source files from plugin directories. When the plugins got pulled out of the main Kaleidoscope source dir, plugin header files became unavailable to testcases, but these files are useful in some instances. Signed-off-by: Michael Richters --- testing/makefiles/testcase.mk | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/testing/makefiles/testcase.mk b/testing/makefiles/testcase.mk index 9efad898..9192fb77 100644 --- a/testing/makefiles/testcase.mk +++ b/testing/makefiles/testcase.mk @@ -3,6 +3,8 @@ top_dir := $(abspath $(mkfile_dir)../..) include $(mkfile_dir)/shared.mk +include_plugins_dir := -I${top_dir}/plugins \ + build_dir := ${top_dir}/_build/${testcase} LIB_DIR := ${build_dir}/lib @@ -81,7 +83,7 @@ endif ${OBJ_DIR}/%.o: ${SRC_DIR}/%.cpp $(QUIET) install -d "${OBJ_DIR}" $(QUIET) $(COMPILER_WRAPPER) $(call _arduino_prop,compiler.cpp.cmd) -o "$@" -c -std=c++14 \ - ${shared_includes} ${shared_defines} $< + ${shared_includes} ${include_plugins_dir} ${shared_defines} $< clean: $(QUIET) rm -f -- "${SRC_DIR}/generated-testcase.cpp" From d0c34ad7072764a928d90d8159220088eb4ebaa6 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Mon, 1 Feb 2021 11:38:03 -0600 Subject: [PATCH 3/3] Add testcase for backwards compatibility of ranges This testcase checks for any changes to existing values in Kaleidoscope-Ranges, with the goal of preventing backwards-incompatible changes that will affect existing EEPROM keymaps configured via Chrysalis. Signed-off-by: Michael Richters --- tests/issues/1010/1010.ino | 46 +++++++++ tests/issues/1010/sketch.json | 6 ++ tests/issues/1010/test/testcase.cpp | 141 ++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+) create mode 100644 tests/issues/1010/1010.ino create mode 100644 tests/issues/1010/sketch.json create mode 100644 tests/issues/1010/test/testcase.cpp diff --git a/tests/issues/1010/1010.ino b/tests/issues/1010/1010.ino new file mode 100644 index 00000000..773b29ca --- /dev/null +++ b/tests/issues/1010/1010.ino @@ -0,0 +1,46 @@ +/* -*- 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 + +// *INDENT-OFF* +KEYMAPS( + [0] = KEYMAP_STACKED + ( + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___, + + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___ + ), +) +// *INDENT-ON* + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/issues/1010/sketch.json b/tests/issues/1010/sketch.json new file mode 100644 index 00000000..8cc86922 --- /dev/null +++ b/tests/issues/1010/sketch.json @@ -0,0 +1,6 @@ +{ + "cpu": { + "fqbn": "keyboardio:virtual:model01", + "port": "" + } +} diff --git a/tests/issues/1010/test/testcase.cpp b/tests/issues/1010/test/testcase.cpp new file mode 100644 index 00000000..100bf384 --- /dev/null +++ b/tests/issues/1010/test/testcase.cpp @@ -0,0 +1,141 @@ +/* -*- 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 "testing/setup-googletest.h" + +#include "Kaleidoscope-Ranges/src/Kaleidoscope-Ranges.h" + +SETUP_GOOGLETEST(); + +namespace kaleidoscope { +namespace testing { + +class Issue1010 : public ::testing::Test { + public: + enum : uint16_t { + MACRO_FIRST = (SYNTHETIC | 0b00100000) << 8, + MACRO_LAST = MACRO_FIRST + 255, + + FIRST = 0xc000, + KALEIDOSCOPE_FIRST = FIRST, + OS_FIRST, + OSM_FIRST = OS_FIRST, + OSM_LAST = OSM_FIRST + 7, + OSL_FIRST, + OSL_LAST = OSL_FIRST + 7, + OS_LAST = OSL_LAST, + DU_FIRST, + DUM_FIRST = DU_FIRST, + DUM_LAST = DUM_FIRST + (8 << 8), + DUL_FIRST, + DUL_LAST = DUL_FIRST + (8 << 8), + DU_LAST = DUL_LAST, + TD_FIRST, + TD_LAST = TD_FIRST + 15, + LEAD_FIRST, + LEAD_LAST = LEAD_FIRST + 7, + CYCLE, + SYSTER, + TT_FIRST, + TT_LAST = TT_FIRST + 255, + STENO_FIRST, + STENO_LAST = STENO_FIRST + 42, + SC_FIRST, + SC_LAST, + REDIAL, + TURBO, + DYNAMIC_MACRO_FIRST, + DYNAMIC_MACRO_LAST = DYNAMIC_MACRO_FIRST + 31, + + SAFE_START, + KALEIDOSCOPE_SAFE_START = SAFE_START + }; +}; + + +TEST_F(Issue1010, RangesHaveNotChanged) { + ASSERT_EQ(uint16_t(Issue1010::MACRO_FIRST), + uint16_t(kaleidoscope::ranges::MACRO_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::MACRO_LAST), + uint16_t(kaleidoscope::ranges::MACRO_LAST)); + ASSERT_EQ(uint16_t(Issue1010::FIRST), + uint16_t(kaleidoscope::ranges::FIRST)); + ASSERT_EQ(uint16_t(Issue1010::KALEIDOSCOPE_FIRST), + uint16_t(kaleidoscope::ranges::KALEIDOSCOPE_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::OS_FIRST), + uint16_t(kaleidoscope::ranges::OS_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::OSM_FIRST), + uint16_t(kaleidoscope::ranges::OSM_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::OSM_LAST), + uint16_t(kaleidoscope::ranges::OSM_LAST)); + ASSERT_EQ(uint16_t(Issue1010::OSL_FIRST), + uint16_t(kaleidoscope::ranges::OSL_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::OSL_LAST), + uint16_t(kaleidoscope::ranges::OSL_LAST)); + ASSERT_EQ(uint16_t(Issue1010::OS_LAST), + uint16_t(kaleidoscope::ranges::OS_LAST)); + ASSERT_EQ(uint16_t(Issue1010::DU_FIRST), + uint16_t(kaleidoscope::ranges::DU_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::DUM_FIRST), + uint16_t(kaleidoscope::ranges::DUM_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::DUM_LAST), + uint16_t(kaleidoscope::ranges::DUM_LAST)); + ASSERT_EQ(uint16_t(Issue1010::DUL_FIRST), + uint16_t(kaleidoscope::ranges::DUL_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::DUL_LAST), + uint16_t(kaleidoscope::ranges::DUL_LAST)); + ASSERT_EQ(uint16_t(Issue1010::DU_LAST), + uint16_t(kaleidoscope::ranges::DU_LAST)); + ASSERT_EQ(uint16_t(Issue1010::TD_FIRST), + uint16_t(kaleidoscope::ranges::TD_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::TD_LAST), + uint16_t(kaleidoscope::ranges::TD_LAST)); + ASSERT_EQ(uint16_t(Issue1010::LEAD_FIRST), + uint16_t(kaleidoscope::ranges::LEAD_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::LEAD_LAST), + uint16_t(kaleidoscope::ranges::LEAD_LAST)); + ASSERT_EQ(uint16_t(Issue1010::CYCLE), + uint16_t(kaleidoscope::ranges::CYCLE)); + ASSERT_EQ(uint16_t(Issue1010::SYSTER), + uint16_t(kaleidoscope::ranges::SYSTER)); + ASSERT_EQ(uint16_t(Issue1010::TT_FIRST), + uint16_t(kaleidoscope::ranges::TT_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::TT_LAST), + uint16_t(kaleidoscope::ranges::TT_LAST)); + ASSERT_EQ(uint16_t(Issue1010::STENO_FIRST), + uint16_t(kaleidoscope::ranges::STENO_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::STENO_LAST), + uint16_t(kaleidoscope::ranges::STENO_LAST)); + ASSERT_EQ(uint16_t(Issue1010::SC_FIRST), + uint16_t(kaleidoscope::ranges::SC_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::SC_LAST), + uint16_t(kaleidoscope::ranges::SC_LAST)); + ASSERT_EQ(uint16_t(Issue1010::REDIAL), + uint16_t(kaleidoscope::ranges::REDIAL)); + ASSERT_EQ(uint16_t(Issue1010::TURBO), + uint16_t(kaleidoscope::ranges::TURBO)); + ASSERT_EQ(uint16_t(Issue1010::DYNAMIC_MACRO_FIRST), + uint16_t(kaleidoscope::ranges::DYNAMIC_MACRO_FIRST)); + ASSERT_EQ(uint16_t(Issue1010::DYNAMIC_MACRO_LAST), + uint16_t(kaleidoscope::ranges::DYNAMIC_MACRO_LAST)); + ASSERT_EQ(uint16_t(Issue1010::SAFE_START), + uint16_t(kaleidoscope::ranges::SAFE_START)); + ASSERT_EQ(uint16_t(Issue1010::KALEIDOSCOPE_SAFE_START), + uint16_t(kaleidoscope::ranges::KALEIDOSCOPE_SAFE_START)); +} + +} // namespace testing +} // namespace kaleidoscope