Debounce refactor (#1809)

* 1,2,4,7,9

* 6

* fix type mismatch

* change field

* comment

* Revert "comment"

This reverts commit f7ce8ed48d015490ed82d692270198817569b5a3.

* Revert "Revert "comment""

This reverts commit 99f2d5cadcbf444bf58acf9e57a6fed61355d5be.

* Revert "change field"

This reverts commit 55ec050cd947696cdffccae6b29fe48d95ab5f17.

* rename var

* comments

* use func in init

* default values

* remove redundant check

firmware/controllers/start_stop.cpp

@@ -6,6 +6,6 @@ EXTERN_ENGINE;
 ButtonDebounce startStopButtonDebounce;
 
 void initStartStopButton(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
-	/* startCrankingDuration is efitimesec_t, so we need to multiply by 1000 to get milliseconds */
+	/* startCrankingDuration is efitimesec_t, so we need to multiply it by 1000 to get milliseconds*/
 	startStopButtonDebounce.init((CONFIG(startCrankingDuration)*1000), &CONFIG(startStopButtonPin), &CONFIG(startStopButtonMode));
 }

firmware/hw_layer/debounce.cpp

@@ -12,91 +12,95 @@
 
 ButtonDebounce* ButtonDebounce::s_firstDebounce = nullptr;
 
-void ButtonDebounce::init (efitimems_t t, brain_pin_e *pin, pin_input_mode_e *mode) {
+/**
+We need to have a separate init function because we do not have the pin or mode in the context in which the class is originally created
+*/
+void ButtonDebounce::init (efitimems_t threshold, brain_pin_e *pin, pin_input_mode_e *mode) {
+   // we need to keep track of whether we have already been initialized due to the way unit tests run.
     if (!initialized) {
-        ButtonDebounce *listItem = s_firstDebounce;
-        if (listItem == nullptr) {
-            s_firstDebounce = this;
-        } else {
-            while (listItem->nextDebounce != nullptr) {
-                listItem = listItem->nextDebounce;
-            }
-            listItem->nextDebounce = this;
-        }
+	// Link us to the list that is used to track ButtonDebounce instances, so that when the configuration changes,
+	//  they can be looped through and updated.
+        nextDebounce = s_firstDebounce;
+        s_firstDebounce = this;
     }
-    threshold = MS2NT(t);
+    m_threshold = MS2NT(threshold);
     timeLast = 0;
-    this->pin = pin;
-    active_pin = *pin;
-    this->mode = mode;
-    active_mode = *mode;
-#ifndef EFI_UNIT_TEST
-    // getInputMode converts from pin_input_mode_e to iomode_t
-    efiSetPadMode("Button", active_pin, getInputMode(active_mode));
-#endif
+    m_pin = pin;
+    m_mode = mode;
+    startConfiguration();
     initialized = true;
 }
 
-void ButtonDebounce::updateConfigurationList () {
+void ButtonDebounce::stopConfigurationList () {
     ButtonDebounce *listItem = s_firstDebounce;
     while (listItem != nullptr) {
-        listItem->updateConfiguration();
-        if (listItem->nextDebounce != nullptr) {
-            listItem = listItem->nextDebounce;
-        } else {
-            break;
-        }
+        listItem->stopConfiguration();
+        listItem = listItem->nextDebounce;
     }
 }
 
-void ButtonDebounce::updateConfiguration () {
+void ButtonDebounce::startConfigurationList () {
+    ButtonDebounce *listItem = s_firstDebounce;
+    while (listItem != nullptr) {
+        listItem->startConfiguration();
+        listItem = listItem->nextDebounce;
+    }
+}
+
+void ButtonDebounce::stopConfiguration () {
+    // If the configuration has changed
 #ifndef EFI_ACTIVE_CONFIGURATION_IN_FLASH
-    if (*pin != active_pin || *mode != active_mode) {
+    if (*m_pin != active_pin || *m_mode != active_mode) {
 #else
-    if (*pin != active_pin || *mode != active_mode || (isActiveConfigurationVoid && (*pin != 0 || *mode != 0))) {
+    if (*m_pin != active_pin || *m_mode != active_mode || (isActiveConfigurationVoid && (*m_pin != 0 || *m_mode != 0))) {
 #endif /* EFI_ACTIVE_CONFIGURATION_IN_FLASH */
 #ifndef EFI_UNIT_TEST
         brain_pin_markUnused(active_pin);
-        efiSetPadMode("Button", *pin, getInputMode(*mode));
 #endif /* EFI_UNIT_TEST */
     }
-    active_pin = *pin;
-    active_mode = *mode;
+}
+
+void ButtonDebounce::startConfiguration () {
+#ifndef EFI_UNIT_TEST
+    efiSetPadMode("Button", *m_pin, getInputMode(*m_mode));
+#endif
+    active_pin = *m_pin;
+    active_mode = *m_mode;
 }
 
 /**
 @returns true if the button is pressed, and will not return true again within the set timeout
 */
 bool ButtonDebounce::readPinEvent() {
-    readValue = false;
+    storedValue = false;
     return readPinState();
 }
 
 bool ButtonDebounce::readPinState() {
-    if (!pin) {
+    if (!m_pin) {
         return false;
     }
     efitick_t timeNow = getTimeNowNt();
     // If it's been less than the threshold since we were last called
-    if ((timeNow - timeLast) < threshold) {
-        return readValue;
+    if ((timeNow - timeLast) < m_threshold) {
+        return storedValue;
     }
-    // readValue is a class variable, so it needs to be reset.
+    // storedValue is a class variable, so it needs to be reset.
     // We don't actually need it to be a class variable in this method,
     //  but when a method is implemented to actually get the pin's state,
     //  for example to implement long button presses, it will be needed.
-    readValue = false;
+    storedValue = false;
 #if EFI_PROD_CODE || EFI_UNIT_TEST
-    readValue = efiReadPin(active_pin);
+    storedValue = efiReadPin(active_pin);
 #endif
 #if EFI_PROD_CODE
     // Invert
     if (getInputMode(active_mode) == PAL_MODE_INPUT_PULLUP) {
-        readValue = !readValue;
+        storedValue = !storedValue;
     }
 #endif
-    if (readValue) {
+    if (storedValue) {
         timeLast = timeNow;
     }
-    return readValue;
+    return storedValue;
 }

firmware/hw_layer/debounce.h

@@ -1,34 +1,36 @@
 /**
  * @file        debounce.h
  * @brief       Generic button debounce class
+ *              https://en.wikipedia.org/wiki/Switch#Contact_bounce
+ *              If we don't 'debounce' our button inputs, we may mistakenly
+ *              read a single button press as multiple events.
  *
  * @date Aug 31, 2020
  * @author David Holdeman, (c) 2020
  */
-#ifndef DEBOUNCE_INC
-#define DEBOUNCE_INC
+#pragma once
 
 #include "globalaccess.h"
 #include "io_pins.h"
 
 class ButtonDebounce {
 public:
-    void init(efitimems_t t, brain_pin_e *p, pin_input_mode_e *m);
-    void updateConfiguration();
+    void init(efitimems_t threshold, brain_pin_e *pin, pin_input_mode_e *mode);
+    void stopConfiguration();
+    void startConfiguration();
     bool readPinEvent();
     bool readPinState();
-    static void updateConfigurationList();
+    static void stopConfigurationList();
+    static void startConfigurationList();
 private:
-    efitick_t threshold;
+    efitick_t m_threshold;
     efitick_t timeLast;
-    brain_pin_e *pin;
-    brain_pin_e active_pin;
-    pin_input_mode_e *mode;
-    pin_input_mode_e active_mode;
-    bool readValue;
+    brain_pin_e *m_pin;
+    brain_pin_e active_pin = GPIO_UNASSIGNED;
+    pin_input_mode_e *m_mode;
+    pin_input_mode_e active_mode = PI_DEFAULT;
+    bool storedValue = false;
     bool initialized = false;
     ButtonDebounce *nextDebounce = nullptr;
     static ButtonDebounce* s_firstDebounce;
 };
-
-#endif /* DEBOUNCE_INC */

firmware/hw_layer/hardware.cpp

@@ -328,7 +328,7 @@ void stopSpi(spi_device_e device) {
 void applyNewHardwareSettings(void) {
     // all 'stop' methods need to go before we begin starting pins
 
-	ButtonDebounce::updateConfigurationList();
+	ButtonDebounce::stopConfigurationList();
 
 #if EFI_SHAFT_POSITION_INPUT
 	stopTriggerInputPins();
@@ -401,6 +401,8 @@ void applyNewHardwareSettings(void) {
 
 	enginePins.unregisterPins();
 
+	ButtonDebounce::startConfigurationList();
+
 #if EFI_SHAFT_POSITION_INPUT
 	startTriggerInputPins();
 #endif /* EFI_SHAFT_POSITION_INPUT */