From dad02678be07770c859cf2bd42e7cb3835379a2b Mon Sep 17 00:00:00 2001 From: Mitch Bradley Date: Fri, 17 Jun 2022 11:33:33 -1000 Subject: [PATCH 1/3] ControlPin rework 1. Control pins are now registered in a vector instead of being named individually. That makes it easier to operate on them as a group. 2. Renamed report() to report_status() to reflect the use of that routine in formatting status reports and to avoid confusion with the other report() that does something different. 3. Renamed system_check_safety_door_ajar() to safety_door_ajar() to avoid excessive wordiness and to fix the problem that the system_ prefix is supposed to mean that the function is defined in System.cpp 4. Added debouncing to control pins to avoid rapid-fire event assertions and as a model for other debouncing code should we need it. --- FluidNC/src/Control.cpp | 71 ++++++++++++++++++++------------- FluidNC/src/Control.h | 20 ++++------ FluidNC/src/ControlPin.cpp | 23 +++++------ FluidNC/src/ControlPin.h | 13 +++--- FluidNC/src/ProcessSettings.cpp | 6 +-- FluidNC/src/Protocol.cpp | 7 +++- FluidNC/src/Report.cpp | 2 +- 7 files changed, 80 insertions(+), 62 deletions(-) diff --git a/FluidNC/src/Control.cpp b/FluidNC/src/Control.cpp index c21826737..5aac66cc0 100644 --- a/FluidNC/src/Control.cpp +++ b/FluidNC/src/Control.cpp @@ -5,47 +5,64 @@ #include "Protocol.h" // rtSafetyDoor, etc -Control::Control() : - _safetyDoor(rtSafetyDoor, "Door", 'D'), _reset(rtReset, "Reset", 'R'), _feedHold(rtFeedHold, "FeedHold", 'H'), - _cycleStart(rtCycleStart, "CycleStart", 'S'), _macro0(rtButtonMacro0, "Macro 0", '0'), _macro1(rtButtonMacro1, "Macro 1", '1'), - _macro2(rtButtonMacro2, "Macro 2", '2'), _macro3(rtButtonMacro3, "Macro 3", '3') {} +Control::Control() { + // The SafetyDoor pin must be defined first because it is checked explicity in safety_door_ajar() + _pins.push_back(new ControlPin(rtSafetyDoor, "safety_door_pin", 'D')); + _pins.push_back(new ControlPin(rtReset, "reset_pin", 'R')); + _pins.push_back(new ControlPin(rtFeedHold, "feed_hold_pin", 'H')); + _pins.push_back(new ControlPin(rtCycleStart, "cycle_start_pin", 'S')); + _pins.push_back(new ControlPin(rtButtonMacro0, "macro0_pin", '0')); + _pins.push_back(new ControlPin(rtButtonMacro1, "macro1_pin", '1')); + _pins.push_back(new ControlPin(rtButtonMacro2, "macro2_pin", '2')); + _pins.push_back(new ControlPin(rtButtonMacro3, "macro3_pin", '3')); +} void Control::init() { - _safetyDoor.init(); - _reset.init(); - _feedHold.init(); - _cycleStart.init(); - _macro0.init(); - _macro1.init(); - _macro2.init(); - _macro3.init(); + for (auto pin : _pins) { + pin->init(); + } } void Control::group(Configuration::HandlerBase& handler) { - handler.item("safety_door_pin", _safetyDoor._pin); - handler.item("reset_pin", _reset._pin); - handler.item("feed_hold_pin", _feedHold._pin); - handler.item("cycle_start_pin", _cycleStart._pin); - handler.item("macro0_pin", _macro0._pin); - handler.item("macro1_pin", _macro1._pin); - handler.item("macro2_pin", _macro2._pin); - handler.item("macro3_pin", _macro3._pin); + for (auto pin : _pins) { + handler.item(pin->_legend, pin->_pin); + } } -String Control::report() { - return _safetyDoor.report() + _reset.report() + _feedHold.report() + _cycleStart.report() + _macro0.report() + _macro1.report() + - _macro2.report() + _macro3.report(); +String Control::report_status() { + String ret = ""; + for (auto pin : _pins) { + if (pin->get()) { + ret += pin->_letter; + } + } + return ret; } bool Control::stuck() { - return _safetyDoor.get() || _reset.get() || _feedHold.get() || _cycleStart.get() || _macro0.get() || _macro1.get() || _macro2.get() || - _macro3.get(); + for (auto pin : _pins) { + if (pin->get()) { + return true; + } + } + return false; +} + +bool Control::startup_check() { + bool ret = false; + for (auto pin : _pins) { + if (pin->get()) { + log_error(pin->_legend << " is active at startup"); + ret = true; + } + } + return ret; } // Returns if safety door is ajar(T) or closed(F), based on pin state. -bool Control::system_check_safety_door_ajar() { +bool Control::safety_door_ajar() { // If a safety door pin is not defined, this will return false // because that is the default for the value field, which will // never be changed for an undefined pin. - return _safetyDoor.get(); + return _pins[0]->get(); } diff --git a/FluidNC/src/Control.h b/FluidNC/src/Control.h index 9dc523ae1..e2fe20465 100644 --- a/FluidNC/src/Control.h +++ b/FluidNC/src/Control.h @@ -5,23 +5,17 @@ #include "Configuration/Configurable.h" #include "ControlPin.h" +#include class Control : public Configuration::Configurable { -// private: + // private: // TODO: Should we not just put this in an array so we can enumerate it easily? -public: - ControlPin _safetyDoor; - ControlPin _reset; - ControlPin _feedHold; - ControlPin _cycleStart; - ControlPin _macro0; - ControlPin _macro1; - ControlPin _macro2; - ControlPin _macro3; public: Control(); + std::vector _pins; + // Initializes control pins. void init(); @@ -29,8 +23,10 @@ class Control : public Configuration::Configurable { void group(Configuration::HandlerBase& handler) override; bool stuck(); - bool system_check_safety_door_ajar(); - String report(); + bool safety_door_ajar(); + String report_status(); + + bool startup_check(); ~Control() = default; }; diff --git a/FluidNC/src/ControlPin.cpp b/FluidNC/src/ControlPin.cpp index adbc60203..bdb47127b 100644 --- a/FluidNC/src/ControlPin.cpp +++ b/FluidNC/src/ControlPin.cpp @@ -8,8 +8,16 @@ void IRAM_ATTR ControlPin::handleISR() { bool pinState = _pin.read(); _value = pinState; - if (pinState) { - _rtVariable = pinState; + + // Rate limit control pin events so switch bounce does not cause multiple events + if (pinState && (_debounceEnd == 0 || ((getCpuTicks() - _debounceEnd) >= 0))) { + _debounceEnd = usToEndTicks(debounceUs); + // We use 0 to mean that the debounce lockout is inactive, + // so if the end time happens to be 0, bump it up by one tick. + if (_debounceEnd == 0) { + _debounceEnd = 1; + } + _rtVariable = true; } } @@ -25,16 +33,7 @@ void ControlPin::init() { _pin.setAttr(attr); _pin.attachInterrupt(ISRHandler, CHANGE, this); _rtVariable = false; - _value = _pin.read(); - // Control pins must start in inactive state - if (_value) { - log_error(_legend << " pin is active at startup"); - rtAlarm = ExecAlarm::ControlPin; - } -} - -String ControlPin::report() { - return get() ? String(_letter) : String(""); + _value = _pin.read(); } ControlPin::~ControlPin() { diff --git a/FluidNC/src/ControlPin.h b/FluidNC/src/ControlPin.h index 5d3cf864e..e5693d651 100644 --- a/FluidNC/src/ControlPin.h +++ b/FluidNC/src/ControlPin.h @@ -6,14 +6,19 @@ class ControlPin { private: bool _value; - const char _letter; - volatile bool& _rtVariable; - const char* _legend; + volatile bool& _rtVariable; // The variable that is set when the pin is asserted + int32_t _debounceEnd = 0; void IRAM_ATTR handleISR(); CreateISRHandlerFor(ControlPin, handleISR); + // Interval during which we ignore repeated control pin activations + const int debounceUs = 10000; // 10000 us = 10 ms + public: + const char* _legend; // The name that appears in init() messages and the name of the configuration item + const char _letter; // The letter that appears in status reports when the pin is active + ControlPin(volatile bool& rtVariable, const char* legend, char letter) : _value(false), _letter(letter), _rtVariable(rtVariable), _legend(legend) { _rtVariable = _value; @@ -24,7 +29,5 @@ class ControlPin { void init(); bool get() { return _value; } - String report(); - ~ControlPin(); }; diff --git a/FluidNC/src/ProcessSettings.cpp b/FluidNC/src/ProcessSettings.cpp index 600d72634..974b9b460 100644 --- a/FluidNC/src/ProcessSettings.cpp +++ b/FluidNC/src/ProcessSettings.cpp @@ -262,12 +262,12 @@ static Error toggle_check_mode(const char* value, WebUI::AuthenticationLevel aut } static Error isStuck() { // Block if a control pin is stuck on - if (config->_control->system_check_safety_door_ajar()) { + if (config->_control->safety_door_ajar()) { rtAlarm = ExecAlarm::ControlPin; return Error::CheckDoor; } if (config->_control->stuck()) { - log_info("Control pins:" << config->_control->report()); + log_info("Control pins:" << config->_control->report_status()); rtAlarm = ExecAlarm::ControlPin; return Error::CheckControlPins; } @@ -313,7 +313,7 @@ static Error home(int cycle) { return Error::SettingDisabled; } - if (config->_control->system_check_safety_door_ajar()) { + if (config->_control->safety_door_ajar()) { return Error::CheckDoor; // Block if safety door is ajar. } diff --git a/FluidNC/src/Protocol.cpp b/FluidNC/src/Protocol.cpp index 552e1a2b2..53dbda507 100644 --- a/FluidNC/src/Protocol.cpp +++ b/FluidNC/src/Protocol.cpp @@ -132,6 +132,9 @@ void protocol_main_loop() { report_feedback_message(Message::CheckLimits); } } + if (config->_control->startup_check()) { + rtAlarm = ExecAlarm::ControlPin; + } if (sys.state == State::Alarm || sys.state == State::Sleep) { report_feedback_message(Message::AlarmLock); @@ -139,7 +142,7 @@ void protocol_main_loop() { } else { // Check if the safety door is open. sys.state = State::Idle; - if (config->_control->system_check_safety_door_ajar()) { + if (config->_control->safety_door_ajar()) { rtSafetyDoor = true; protocol_execute_realtime(); // Enter safety door mode. Should return as IDLE state. } @@ -924,7 +927,7 @@ static void protocol_exec_rt_suspend() { } // Allows resuming from parking/safety door. Actively checks if safety door is closed and ready to resume. if (sys.state == State::SafetyDoor) { - if (!config->_control->system_check_safety_door_ajar()) { + if (!config->_control->safety_door_ajar()) { sys.suspend.bit.safetyDoorAjar = false; // Reset door ajar flag to denote ready to resume. } } diff --git a/FluidNC/src/Report.cpp b/FluidNC/src/Report.cpp index 41daac88a..44410aa84 100644 --- a/FluidNC/src/Report.cpp +++ b/FluidNC/src/Report.cpp @@ -563,7 +563,7 @@ static void pinString(Print& channel) { } } - String ctrl_pin_report = config->_control->report(); + String ctrl_pin_report = config->_control->report_status(); if (ctrl_pin_report.length()) { if (prefixNeeded) { prefixNeeded = false; From 71c90dd221a6366fea23c48f7e8648571cfec237 Mon Sep 17 00:00:00 2001 From: Mitch Bradley Date: Fri, 17 Jun 2022 19:05:11 -1000 Subject: [PATCH 2/3] Fixed oled breakage Compiles correctly with INCLUDE_OLED_BASIC defined, but not tested on hardware. --- FluidNC/src/Custom/oled_basic.cpp | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/FluidNC/src/Custom/oled_basic.cpp b/FluidNC/src/Custom/oled_basic.cpp index ab219f902..7e2979020 100644 --- a/FluidNC/src/Custom/oled_basic.cpp +++ b/FluidNC/src/Custom/oled_basic.cpp @@ -133,27 +133,13 @@ static void oledDRO() { draw_checkbox(120, oled_y_pos + 3, 7, 7, prb_pin_state); oled_y_pos += 10; } - if (ctrl_pins->_feedHold._pin.defined()) { - oled->drawString(110, oled_y_pos, "H"); - draw_checkbox(120, oled_y_pos + 3, 7, 7, ctrl_pins->_feedHold.get()); - oled_y_pos += 10; - } - if (ctrl_pins->_cycleStart._pin.defined()) { - oled->drawString(110, oled_y_pos, "S"); - draw_checkbox(120, oled_y_pos + 3, 7, 7, ctrl_pins->_cycleStart.get()); - oled_y_pos += 10; - } - - if (ctrl_pins->_reset._pin.defined()) { - oled->drawString(110, oled_y_pos, "R"); - draw_checkbox(120, oled_y_pos + 3, 7, 7, ctrl_pins->_reset.get()); - oled_y_pos += 10; - } - - if (ctrl_pins->_safetyDoor._pin.defined()) { - oled->drawString(110, oled_y_pos, "D"); - draw_checkbox(120, oled_y_pos + 3, 7, 7, ctrl_pins->_safetyDoor.get()); - oled_y_pos += 10; + for (auto pin : ctrl_pins->_pins) { + char letter = pin->_letter; + if (!isdigit(letter)) { + oled->drawString(110, oled_y_pos, String(letter)); + draw_checkbox(120, oled_y_pos + 3, 7, 7, pin->get()); + oled_y_pos += 10; + } } } From d13a5e94f7df49567bc7111fa20ba912bcd1e479 Mon Sep 17 00:00:00 2001 From: Mitch Bradley Date: Fri, 17 Jun 2022 19:08:57 -1000 Subject: [PATCH 3/3] TODO is now too done, tada! --- FluidNC/src/Control.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/FluidNC/src/Control.h b/FluidNC/src/Control.h index e2fe20465..af780bf5a 100644 --- a/FluidNC/src/Control.h +++ b/FluidNC/src/Control.h @@ -8,9 +8,6 @@ #include class Control : public Configuration::Configurable { - // private: - // TODO: Should we not just put this in an array so we can enumerate it easily? - public: Control();