From 9536590a255e2efffe241ddd919379d56f58a3c0 Mon Sep 17 00:00:00 2001 From: David Holdeman Date: Sun, 20 Sep 2020 13:01:00 -0500 Subject: [PATCH] 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 | 2 +- firmware/hw_layer/debounce.cpp | 88 +++++++++++++++-------------- firmware/hw_layer/debounce.h | 28 ++++----- firmware/hw_layer/hardware.cpp | 4 +- 4 files changed, 65 insertions(+), 57 deletions(-) diff --git a/firmware/controllers/start_stop.cpp b/firmware/controllers/start_stop.cpp index 0de2c2d3f5..bda1f21a1a 100644 --- a/firmware/controllers/start_stop.cpp +++ b/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)); } diff --git a/firmware/hw_layer/debounce.cpp b/firmware/hw_layer/debounce.cpp index 32590440b3..903489d1f1 100644 --- a/firmware/hw_layer/debounce.cpp +++ b/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; } diff --git a/firmware/hw_layer/debounce.h b/firmware/hw_layer/debounce.h index f19f1e632b..f9225513d9 100644 --- a/firmware/hw_layer/debounce.h +++ b/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 */ diff --git a/firmware/hw_layer/hardware.cpp b/firmware/hw_layer/hardware.cpp index e5dc72a012..4b145d8760 100644 --- a/firmware/hw_layer/hardware.cpp +++ b/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 */