From 0713d656682f82014eac6ef88fa4a5d92aeabf78 Mon Sep 17 00:00:00 2001 From: "D.R.racer" Date: Wed, 19 May 2021 12:57:59 +0200 Subject: [PATCH] Add more unit tests and optimize (saved 1 byte per button on the AVR) --- src/modules/buttons.cpp | 24 ++- src/modules/buttons.h | 39 +++-- tests/unit/modules/buttons/stub_adc.cpp | 18 ++- tests/unit/modules/buttons/stub_adc.h | 2 +- tests/unit/modules/buttons/test_buttons.cpp | 154 +++++++++++++++++++- 5 files changed, 205 insertions(+), 32 deletions(-) diff --git a/src/modules/buttons.cpp b/src/modules/buttons.cpp index f3fb27d..70d726c 100644 --- a/src/modules/buttons.cpp +++ b/src/modules/buttons.cpp @@ -7,39 +7,37 @@ uint16_t Buttons::tmpTiming = 0; // original idea from: https://www.eeweb.com/debouncing-push-buttons-using-a-state-machine-approach void Button::Step(uint16_t time, bool press) { - switch (state) { + switch (f.state) { case State::Waiting: if (press) { - state = State::Detected; + f.state = State::Detected; timeLastChange = time; - tmp = press; + f.tmp = press; } break; case State::Detected: - if (tmp == press) { + if (f.tmp == press) { if (time - timeLastChange > debounce) { - state = State::WaitForRelease; + f.state = State::WaitForRelease; } } else { - state = State::Waiting; + f.state = State::Waiting; } break; case State::WaitForRelease: if (!press) { - state = State::Update; + f.state = State::Update; } break; case State::Update: - pressed = tmp; - state = State::Waiting; + f.state = State::Waiting; timeLastChange = time; - tmp = false; + f.tmp = false; break; default: - state = State::Waiting; + f.state = State::Waiting; timeLastChange = time; - tmp = false; - pressed = false; + f.tmp = false; } } diff --git a/src/modules/buttons.h b/src/modules/buttons.h index 51f5e5c..c3a3ea3 100644 --- a/src/modules/buttons.h +++ b/src/modules/buttons.h @@ -10,33 +10,47 @@ namespace modules { struct Button { inline constexpr Button() - : state(State::Waiting) - , tmp(false) - , pressed(false) - , timeLastChange(0) {} + : timeLastChange(0) {} /// @returns true if button is currently considered as pressed - inline bool Pressed() const { return pressed; } + inline bool Pressed() const { return f.state == State::WaitForRelease; } /// State machine stepping routine void Step(uint16_t time, bool press); private: - constexpr static const uint16_t debounce = 100; ///< time interval for debouncing @@TODO specify units - enum class State : uint_fast8_t { Waiting = 0, + /// time interval for debouncing @@TODO specify units + constexpr static const uint16_t debounce = 100; + + /// States of the debouncing automaton + /// Intentionally not modeled as an enum class + /// as it would impose additional casts which do not play well with the struct Flags + /// and would make the code less readable + enum State { Waiting = 0, Detected, WaitForRelease, Update }; - State state; - bool tmp; ///< temporary state of button before the debouncing state machine finishes - bool pressed; ///< real state of button after debouncing + + /// The sole purpose of this data struct is to save RAM by compressing several flags into one byte on the AVR + struct Flags { + uint8_t state : 2; ///< state of the button + uint8_t tmp : 1; ///< temporary state of button before the debouncing state machine finishes + inline constexpr Flags() + : state(State::Waiting) + , tmp(false) {} + }; + + /// Flags and state of the debouncing automaton + Flags f; + + /// Timestamp of the last change of ADC state for this button uint16_t timeLastChange; }; class Buttons { constexpr static const uint8_t N = 3; ///< number of buttons currently supported constexpr static const uint8_t adc = 1; ///< ADC index - will be some define or other constant later on - static uint16_t tmpTiming; + static uint16_t tmpTiming; ///< subject to removal when we have timers implemented - now used for the unit tests public: inline constexpr Buttons() = default; @@ -50,6 +64,9 @@ public: private: Button buttons[N]; + + /// Call to the ADC and decode its output into a button index + /// @returns index of the button pressed or -1 in case no button is pressed static int8_t Sample(); }; diff --git a/tests/unit/modules/buttons/stub_adc.cpp b/tests/unit/modules/buttons/stub_adc.cpp index e801824..c6edfa3 100644 --- a/tests/unit/modules/buttons/stub_adc.cpp +++ b/tests/unit/modules/buttons/stub_adc.cpp @@ -7,14 +7,26 @@ namespace ADC { static TADCData values2Return; static TADCData::const_iterator rdptr = values2Return.cbegin(); + static uint8_t oversampleFactor = 1; + static uint8_t oversample = 1; ///< current count of oversampled values returned from the ADC - will get filled with oversampleFactor once it reaches zero - void ReinitADC(TADCData d) { - values2Return = d; + void ReinitADC(TADCData &&d, uint8_t ovsmpl) { + values2Return = std::move(d); + oversampleFactor = ovsmpl; + oversample = ovsmpl; rdptr = values2Return.cbegin(); } /// ADC access routines - uint16_t ReadADC(uint8_t /*adc*/) { return rdptr != values2Return.end() ? *rdptr++ : 0; } + uint16_t ReadADC(uint8_t /*adc*/) { + if (!oversample) { + ++rdptr; + oversample = oversampleFactor; + } else { + --oversample; + } + return rdptr != values2Return.end() ? *rdptr : 1023; + } } // namespace ADC } // namespace hal diff --git a/tests/unit/modules/buttons/stub_adc.h b/tests/unit/modules/buttons/stub_adc.h index ebda1c0..7105f01 100644 --- a/tests/unit/modules/buttons/stub_adc.h +++ b/tests/unit/modules/buttons/stub_adc.h @@ -8,7 +8,7 @@ namespace ADC { using TADCData = std::vector; - void ReinitADC(TADCData d); + void ReinitADC(TADCData &&d, uint8_t ovsmpl); } // namespace ADC } // namespace hal diff --git a/tests/unit/modules/buttons/test_buttons.cpp b/tests/unit/modules/buttons/test_buttons.cpp index 21cfbf2..ded5db0 100644 --- a/tests/unit/modules/buttons/test_buttons.cpp +++ b/tests/unit/modules/buttons/test_buttons.cpp @@ -4,12 +4,158 @@ using Catch::Matchers::Equals; -TEST_CASE("buttons::Step", "[buttons]") { +bool Step_Basic_One_Button_Test(modules::Buttons &b, uint8_t oversampleFactor, uint8_t testedButtonIndex, uint8_t otherButton1, uint8_t otherButton2) { + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // should detect the press but remain in detected state - wait for debounce + CHECK(!b.ButtonPressed(testedButtonIndex)); + CHECK(!b.ButtonPressed(otherButton1)); + CHECK(!b.ButtonPressed(otherButton2)); + + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // reset to waiting + CHECK(b.ButtonPressed(testedButtonIndex)); + CHECK(!b.ButtonPressed(otherButton1)); + CHECK(!b.ButtonPressed(otherButton2)); + + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // pressed again, still in debouncing state + CHECK(!b.ButtonPressed(testedButtonIndex)); + CHECK(!b.ButtonPressed(otherButton1)); + CHECK(!b.ButtonPressed(otherButton2)); + + return true; +} + +/// This test verifies the behaviour of a single button. The other buttons must remain intact. +bool Step_Basic_One_Button(hal::ADC::TADCData &&d, uint8_t testedButtonIndex) { using namespace modules; - hal::ADC::TADCData d = { 1, 2, 3, 4, 5 }; - hal::ADC::ReinitADC(d); + Buttons b; + + // need to oversample the data as debouncing takes 100 cycles to accept a pressed button + constexpr uint8_t oversampleFactor = 100; + hal::ADC::ReinitADC(std::move(d), oversampleFactor); + + uint8_t otherButton1 = 1, otherButton2 = 2; + switch (testedButtonIndex) { + case 1: + otherButton1 = 0; + break; + case 2: + otherButton2 = 0; + break; + default: + break; // no change + } + + return Step_Basic_One_Button_Test(b, oversampleFactor, testedButtonIndex, otherButton1, otherButton2); +} + +TEST_CASE("buttons::Step-basic-button", "[buttons]") { + { + hal::ADC::TADCData d({ 5, 6, 1023 }); + CHECK(Step_Basic_One_Button(std::move(d), 0)); + } + { + hal::ADC::TADCData d({ 321, 359, 1023 }); + CHECK(Step_Basic_One_Button(std::move(d), 1)); + } + { + hal::ADC::TADCData d({ 501, 529, 1023 }); + CHECK(Step_Basic_One_Button(std::move(d), 2)); + } +} + +/// This test has to verify the independency of buttons - the ADC reads one button after the other +/// and the Buttons class should press first button and release, then the second one and then the third one +/// without being reinitialized. +TEST_CASE("buttons::Step-basic-button-one-after-other", "[buttons]") { + using namespace modules; + hal::ADC::TADCData d({ 5, 6, 1023, 321, 359, 1023, 501, 529, 1023 }); + Buttons b; + + // need to oversample the data as debouncing takes 100 cycles to accept a pressed button + constexpr uint8_t oversampleFactor = 100; + hal::ADC::ReinitADC(std::move(d), oversampleFactor); + + CHECK(Step_Basic_One_Button_Test(b, oversampleFactor, 0, 1, 2)); + CHECK(Step_Basic_One_Button_Test(b, oversampleFactor, 1, 0, 2)); + CHECK(Step_Basic_One_Button_Test(b, oversampleFactor, 2, 0, 1)); +} + +/// This test tries to simulate a bouncing effect on data from ADC on the first button +TEST_CASE("buttons::Step-debounce-one-button", "[buttons]") { + using namespace modules; + + // make a bounce event on the first press + hal::ADC::TADCData d({ 5, 1023, 5, 9, 6, 7, 8, 1023, 1023 }); + + // need to oversample the data as debouncing takes 100 cycles to accept a pressed button + constexpr uint8_t oversampleFactor = 25; + hal::ADC::ReinitADC(std::move(d), oversampleFactor); Buttons b; - b.Step(); + + // 5 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // should detect the press but remain in detected state - wait for debounce + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 1023 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // reset to waiting + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 5 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // pressed again, still in debouncing state + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 9 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // no change + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 6 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // no change + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 7 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // one step from "pressed" + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 8 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // fifth set of samples - should report "pressed" finally + CHECK(b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 1023 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // sixth set of samples - button released (no debouncing on release) + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); + + // 1023 + for (uint8_t i = 0; i < oversampleFactor; ++i) + b.Step(); // seventh set of samples - still released + CHECK(!b.ButtonPressed(0)); + CHECK(!b.ButtonPressed(1)); + CHECK(!b.ButtonPressed(2)); }