Add more unit tests and optimize (saved 1 byte per button on the AVR)

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;
+        f.state = State::Waiting;
-        state = State::Waiting;
         timeLastChange = time;
-        tmp = false;
+        f.tmp = false;
         break;
     default:
-        state = State::Waiting;
+        f.state = State::Waiting;
         timeLastChange = time;
-        tmp = false;
+        f.tmp = false;
-        pressed = false;
     }
 }
 

src/modules/buttons.h

@@ -10,33 +10,47 @@ namespace modules {
 
 struct Button {
     inline constexpr Button()
-        : state(State::Waiting)
+        : timeLastChange(0) {}
-        , tmp(false)
-        , pressed(false)
-        , 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
+    /// time interval for debouncing @@TODO specify units
-    enum class State : uint_fast8_t { Waiting = 0,
+    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
+    /// The sole purpose of this data struct is to save RAM by compressing several flags into one byte on the AVR
-    bool pressed; ///< real state of button after debouncing
+    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();
 };
 

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) {
+    void ReinitADC(TADCData &&d, uint8_t ovsmpl) {
-        values2Return = d;
+        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

tests/unit/modules/buttons/stub_adc.h

@@ -8,7 +8,7 @@ namespace ADC {
 
     using TADCData = std::vector<uint16_t>;
 
-    void ReinitADC(TADCData d);
+    void ReinitADC(TADCData &&d, uint8_t ovsmpl);
 
 } // namespace ADC
 } // namespace hal

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 };
+    Buttons b;
-    hal::ADC::ReinitADC(d);
+
+    // 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));
 }