From faf8f4488863d6ca3a1e12cbd7ba981ef34d756c Mon Sep 17 00:00:00 2001 From: Stefan de Bruijn Date: Thu, 25 Mar 2021 22:52:24 +0100 Subject: [PATCH] Fixed a bunch of bugs. Needs a lot more bug fixing probably. --- Grbl_Esp32/Grbl_Esp32.ino | 6 +- Grbl_Esp32/data/config.yaml | 43 +------ Grbl_Esp32/src/Configuration/AfterParse.cpp | 13 ++ Grbl_Esp32/src/Configuration/AfterParse.h | 32 +++++ Grbl_Esp32/src/Configuration/Configurable.h | 3 +- Grbl_Esp32/src/Configuration/HandlerType.h | 3 +- .../Configuration/LegacySettingRegistry.cpp | 6 +- Grbl_Esp32/src/Configuration/ParseException.h | 5 +- Grbl_Esp32/src/Configuration/ParserHandler.h | 2 +- Grbl_Esp32/src/Configuration/Tokenizer.cpp | 2 +- Grbl_Esp32/src/Configuration/Tokenizer.h | 3 +- Grbl_Esp32/src/Grbl.cpp | 14 +++ Grbl_Esp32/src/Logging.cpp | 2 + Grbl_Esp32/src/Logging.h | 50 +++++++- Grbl_Esp32/src/MachineConfig.cpp | 115 ++++++++++++++---- Grbl_Esp32/src/MachineConfig.h | 8 +- Grbl_Esp32/src/Probe.cpp | 2 +- Grbl_Esp32/src/SimpleOutputStream.h | 2 +- 18 files changed, 229 insertions(+), 82 deletions(-) create mode 100644 Grbl_Esp32/src/Configuration/AfterParse.cpp create mode 100644 Grbl_Esp32/src/Configuration/AfterParse.h diff --git a/Grbl_Esp32/Grbl_Esp32.ino b/Grbl_Esp32/Grbl_Esp32.ino index 120be530..777d6304 100644 --- a/Grbl_Esp32/Grbl_Esp32.ino +++ b/Grbl_Esp32/Grbl_Esp32.ino @@ -22,9 +22,9 @@ # include "src/Grbl.h" void setup() { -# ifdef PIN_DEBUG - sleep(1000); -# endif +// # ifdef PIN_DEBUG + delay(2000); +// # endif Serial.begin(115200); grbl_init(); diff --git a/Grbl_Esp32/data/config.yaml b/Grbl_Esp32/data/config.yaml index 5d80f44c..def66afa 100644 --- a/Grbl_Esp32/data/config.yaml +++ b/Grbl_Esp32/data/config.yaml @@ -1,45 +1,8 @@ -name: "6 Pack Controller StepStick XYZ" -board: bdring-6pack - -i2so: - bck: gpio.22 - ws: gpio.17 - data: gpio.21 +name: "Debug config" +board: Debug-board axes: number_axis: 3 - x: - steps_per_mm: 800 - max_rate: 2000 - acceleration: 25 - max_travel: 1000 - home_mpos: 10 - - gang1: - endstop: - dual: gpio.33:low # or: positive: gpio.33:low, negative: gpio.34:low - - tmc_2130: - step: gpio.4 - direction: gpio.16 - run_current: 1.0 - hold_current: 0.25 - microsteps: 255 - stallguard: 0 - - y: - gang1: - endstop: gpio.32:low - stepstick: - step: gpio.18 - direction: gpio.18 - gang2: - endstop: gpio.34:low - stepstick: - step: gpio.19 - direction: gpio.19 coolant: - flood: gpio.12 - mist: gpio.13 - + flood: gpio.2 diff --git a/Grbl_Esp32/src/Configuration/AfterParse.cpp b/Grbl_Esp32/src/Configuration/AfterParse.cpp new file mode 100644 index 00000000..3d4c3fa9 --- /dev/null +++ b/Grbl_Esp32/src/Configuration/AfterParse.cpp @@ -0,0 +1,13 @@ +#include "AfterParse.h" + +#include "Configurable.h" + +#include + +namespace Configuration +{ + void AfterParse::handleDetail(const char* name, Configurable* value) { + value->afterParse(); + value->handle(*this); + } +} diff --git a/Grbl_Esp32/src/Configuration/AfterParse.h b/Grbl_Esp32/src/Configuration/AfterParse.h new file mode 100644 index 00000000..5f10b9db --- /dev/null +++ b/Grbl_Esp32/src/Configuration/AfterParse.h @@ -0,0 +1,32 @@ +#pragma once + +#include + +#include "../Pin.h" +#include "HandlerBase.h" + +namespace Configuration +{ + class Configurable; + + class AfterParse : public HandlerBase + { + AfterParse(const AfterParse&) = delete; + AfterParse& operator=(const AfterParse&) = delete; + + protected: + void handleDetail(const char* name, Configurable* value) override; + bool matchesUninitialized(const char* name) override { return false; } + HandlerType handlerType() override { return HandlerType::AfterParse; } + + public: + AfterParse() = default; + + void handle(const char* name, bool& value) override { } + void handle(const char* name, int& value) override { } + void handle(const char* name, float& value) override { } + void handle(const char* name, double& value) override { } + void handle(const char* name, StringRange& value) override { } + void handle(const char* name, Pin& value) override { } + }; +} diff --git a/Grbl_Esp32/src/Configuration/Configurable.h b/Grbl_Esp32/src/Configuration/Configurable.h index 53be0fe0..99959373 100644 --- a/Grbl_Esp32/src/Configuration/Configurable.h +++ b/Grbl_Esp32/src/Configuration/Configurable.h @@ -20,8 +20,9 @@ namespace Configuration virtual void validate() const = 0; virtual void handle(HandlerBase& handler) = 0; + virtual void afterParse() {} // virtual const char* name() const = 0; virtual ~Configurable() {} }; -} \ No newline at end of file +} diff --git a/Grbl_Esp32/src/Configuration/HandlerType.h b/Grbl_Esp32/src/Configuration/HandlerType.h index 2514f3c4..104605c7 100644 --- a/Grbl_Esp32/src/Configuration/HandlerType.h +++ b/Grbl_Esp32/src/Configuration/HandlerType.h @@ -5,8 +5,9 @@ namespace Configuration enum struct HandlerType { Parser, + AfterParse, Runtime, Generator, Validator }; -} \ No newline at end of file +} diff --git a/Grbl_Esp32/src/Configuration/LegacySettingRegistry.cpp b/Grbl_Esp32/src/Configuration/LegacySettingRegistry.cpp index 66c5e8cf..97c9ebfc 100644 --- a/Grbl_Esp32/src/Configuration/LegacySettingRegistry.cpp +++ b/Grbl_Esp32/src/Configuration/LegacySettingRegistry.cpp @@ -33,7 +33,7 @@ namespace Configuration handleLegacy(value, str); } else { - warn("Incorrect setting '" << start << "': cannot find '='."); + log_warn("Incorrect setting '" << start << "': cannot find '='."); } return true; } @@ -53,7 +53,7 @@ namespace Configuration } if (!handled) { - warn("Cannot find handler for $" << index << ". Setting was ignored."); + log_warn("Cannot find handler for $" << index << ". Setting was ignored."); } } -} \ No newline at end of file +} diff --git a/Grbl_Esp32/src/Configuration/ParseException.h b/Grbl_Esp32/src/Configuration/ParseException.h index 65f09e51..dd949454 100644 --- a/Grbl_Esp32/src/Configuration/ParseException.h +++ b/Grbl_Esp32/src/Configuration/ParseException.h @@ -5,12 +5,13 @@ namespace Configuration { int line_; int column_; const char* description_; + const char* current_; public: ParseException() = default; ParseException(const ParseException&) = default; - ParseException(const char* start, const char* current, const char* description) : description_(description) { + ParseException(const char* start, const char* current, const char* description) : description_(description), current_(current) { line_ = 1; column_ = 1; while (start != current) { @@ -18,12 +19,14 @@ namespace Configuration { ++line_; column_ = 1; } + ++column_; ++start; } } inline int LineNumber() const { return line_; } inline int ColumnNumber() const { return column_; } + inline const char* Near() const { return current_; } inline const char* What() const { return description_; } }; } diff --git a/Grbl_Esp32/src/Configuration/ParserHandler.h b/Grbl_Esp32/src/Configuration/ParserHandler.h index c198f38c..a8e24d69 100644 --- a/Grbl_Esp32/src/Configuration/ParserHandler.h +++ b/Grbl_Esp32/src/Configuration/ParserHandler.h @@ -14,7 +14,7 @@ namespace Configuration { protected: void handleDetail(const char* name, Configuration::Configurable* value) override { if (value != nullptr && parser_.is(name)) { - debug("Parsing configurable " << name); + log_debug("Parsing configurable " << name); parser_.enter(); for (; !parser_.isEndSection(); parser_.moveNext()) { diff --git a/Grbl_Esp32/src/Configuration/Tokenizer.cpp b/Grbl_Esp32/src/Configuration/Tokenizer.cpp index f04483f1..9ece71a0 100644 --- a/Grbl_Esp32/src/Configuration/Tokenizer.cpp +++ b/Grbl_Esp32/src/Configuration/Tokenizer.cpp @@ -176,7 +176,7 @@ namespace Configuration { } // Skip more whitespaces - while (!Eof() && IsSpace()) { + while (!Eof() && IsWhiteSpace()) { Inc(); } diff --git a/Grbl_Esp32/src/Configuration/Tokenizer.h b/Grbl_Esp32/src/Configuration/Tokenizer.h index f8696ad4..54e2cf6c 100644 --- a/Grbl_Esp32/src/Configuration/Tokenizer.h +++ b/Grbl_Esp32/src/Configuration/Tokenizer.h @@ -1,6 +1,7 @@ #pragma once #include "TokenKind.h" +#include "../Logging.h" namespace Configuration { @@ -24,7 +25,7 @@ namespace Configuration { inline bool IsWhiteSpace() { char c = Current(); - return c == ' ' || c == '\t' || c == '\f'; + return c == ' ' || c == '\t' || c == '\f' || c == '\r'; } inline bool IsEndLine() { return Current() == '\n'; } diff --git a/Grbl_Esp32/src/Grbl.cpp b/Grbl_Esp32/src/Grbl.cpp index 3f340adb..ca5d12c7 100644 --- a/Grbl_Esp32/src/Grbl.cpp +++ b/Grbl_Esp32/src/Grbl.cpp @@ -25,12 +25,14 @@ void grbl_init() { try { + Serial.println("Initializing WiFi..."); WiFi.persistent(false); WiFi.disconnect(true); WiFi.enableSTA(false); WiFi.enableAP(false); WiFi.mode(WIFI_OFF); + Serial.println("Initializing serial communications..."); // Setup serial baud rate and interrupts serial_init(); grbl_msg_sendf( @@ -41,19 +43,26 @@ void grbl_init() { #ifdef MACHINE_NAME report_machine_type(CLIENT_SERIAL); #endif + // Load Grbl settings from non-volatile storage + Serial.println("Initializing settings..."); settings_init(); MachineConfig::instance()->load(); #ifdef USE_I2S_OUT + Serial.println("Initializing I2SO..."); // The I2S out must be initialized before it can access the expanded GPIO port. Must be initialized _after_ settings! i2s_out_init(); #endif + Serial.println("Initializing steppers..."); stepper_init(); // Configure stepper pins and interrupt timers + + Serial.println("Initializing axes..."); MachineConfig::instance()->_axes->read_settings(); MachineConfig::instance()->_axes->init(); + Serial.println("Initializing system..."); system_ini(); // Configure pinout pins and pin-change interrupt (Renamed due to conflict with esp32 files) memset(sys_position, 0, sizeof(sys_position)); // Clear machine position. @@ -79,10 +88,15 @@ void grbl_init() { sys.state = State::Alarm; } #endif + Serial.println("Initializing spindle..."); Spindles::Spindle::select(); + + Serial.println("Initializing WiFi-config..."); #ifdef ENABLE_WIFI WebUI::wifi_config.begin(); #endif + + Serial.println("Initializing Bluetooth..."); #ifdef ENABLE_BLUETOOTH WebUI::bt_config.begin(); #endif diff --git a/Grbl_Esp32/src/Logging.cpp b/Grbl_Esp32/src/Logging.cpp index a1debcae..e1d42a5c 100644 --- a/Grbl_Esp32/src/Logging.cpp +++ b/Grbl_Esp32/src/Logging.cpp @@ -16,6 +16,8 @@ DebugStream::~DebugStream() { std::cout << ']' << std::endl; } #else +#include + DebugStream::DebugStream(const char* name) { Serial.print("["); Serial.print(name); diff --git a/Grbl_Esp32/src/Logging.h b/Grbl_Esp32/src/Logging.h index 0c33acc6..89fa5bb7 100644 --- a/Grbl_Esp32/src/Logging.h +++ b/Grbl_Esp32/src/Logging.h @@ -2,8 +2,20 @@ #include "SimpleOutputStream.h" -class DebugStream : public SimpleOutputStream -{ +// How to use logging? Well, the basics are pretty simple: +// +// - The syntax is like standard iostream's. +// - It is simplified though, so no ios or iomanip. But should be sufficient. +// - But, you wrap it in an 'info', 'debug', 'warn', 'error' or 'fatal'. +// +// The streams here ensure the data goes where it belongs, without too much +// buffer space being wasted. +// +// Example: +// +// log_info("Twelve is written as " << 12 << ", isn't it"); + +class DebugStream : public SimpleOutputStream { public: DebugStream(const char* name); void add(char c) override; @@ -12,8 +24,34 @@ public: #include "StringStream.h" -#define debug(x) { DebugStream ss("DBG "); ss << x; } -#define info(x) { DebugStream ss("INFO"); ss << x; } -#define warn(x) { DebugStream ss("WARN"); ss << x; } -#define error(x) { DebugStream ss("ERR "); ss << x; } +// Note: these '{'..'}' scopes are here for a reason: the destructor should flush. +#define log_debug(x) \ + { \ + DebugStream ss("DBG "); \ + ss << x; \ + } +#define log_info(x) \ + { \ + DebugStream ss("INFO"); \ + ss << x; \ + } + +#define log_warn(x) \ + { \ + DebugStream ss("WARN"); \ + ss << x; \ + } + +#define log_error(x) \ + { \ + DebugStream ss("ERR "); \ + ss << x; \ + } + +#define log_fatal(x) \ + { \ + DebugStream ss("FATAL "); \ + ss << x; \ + Assert(false, "A fatal error occurred."); \ + } diff --git a/Grbl_Esp32/src/MachineConfig.cpp b/Grbl_Esp32/src/MachineConfig.cpp index baf233c0..67182c50 100644 --- a/Grbl_Esp32/src/MachineConfig.cpp +++ b/Grbl_Esp32/src/MachineConfig.cpp @@ -8,12 +8,15 @@ #include "Configuration/ParserHandler.h" #include "Configuration/Validator.h" +#include "Configuration/AfterParse.h" #include "Configuration/ParseException.h" #include #include #include +// TODO FIXME: Split this file up into several files, perhaps put it in some folder and namespace Machine? + void Endstops::validate() const { if (!_dual.undefined()) { Assert(_positive.undefined(), "If dual endstops are defined, you cannot also define positive and negative endstops"); @@ -36,6 +39,11 @@ void Gang::handle(Configuration::HandlerBase& handler) { handler.handle("endstops", _endstops); Motors::MotorFactory::handle(handler, _motor); } +void Gang::afterParse() { + if (_motor == nullptr) { + _motor = new Motors::Nullmotor(); + } +} Gang::~Gang() { delete _motor; @@ -57,6 +65,14 @@ void Axis::handle(Configuration::HandlerBase& handler) { } } +void Axis::afterParse() { + for (size_t i = 0; i < MAX_NUMBER_GANGED; ++i) { + if (_gangs[i] == nullptr) { + _gangs[i] = new Gang(); + } + } +} + // Checks if a motor matches this axis: bool Axis::hasMotor(const Motors::Motor* const motor) const { for (uint8_t gang_index = 0; gang_index < MAX_NUMBER_GANGED; gang_index++) { @@ -284,12 +300,21 @@ void Axes::handle(Configuration::HandlerBase& handler) { tmp[0] = allAxis[a]; tmp[1] = '\0'; - if (handler.handlerType() == Configuration::HandlerType::Runtime || handler.handlerType() == Configuration::HandlerType::Parser) { + if (handler.handlerType() == Configuration::HandlerType::Runtime || handler.handlerType() == Configuration::HandlerType::Parser || + handler.handlerType() == Configuration::HandlerType::AfterParse) { handler.handle(tmp, _axis[a]); } } } +void Axes::afterParse() { + for (size_t i = 0; i < MAX_NUMBER_AXIS; ++i) { + if (_axis[i] == nullptr) { + _axis[i] = new Axis(); + } + } +} + Axes::~Axes() { for (int i = 0; i < MAX_NUMBER_AXIS; ++i) { delete _axis[i]; @@ -337,24 +362,61 @@ void MachineConfig::handle(Configuration::HandlerBase& handler) { handler.handle("laser_mode", _laserMode); } +void MachineConfig::afterParse() { + if (_axes == nullptr) { + log_info("Axes config missing; building default axes."); + _axes = new Axes(); + } + + if (_coolant == nullptr) { + log_info("Coolant control config missing; building default coolant."); + _coolant = new CoolantControl(); + } + + if (_spi == nullptr) { + log_info("SPI config missing; building default SPI bus."); + _spi = new SPIBus(); + } + + if (_probe == nullptr) { + log_info("Probe config missing; building default probe."); + _probe = new Probe(); + } +} + bool MachineConfig::load(const char* filename) { if (!SPIFFS.begin(true)) { - error("An error has occurred while mounting SPIFFS"); + log_fatal("An error has occurred while mounting SPIFFS"); return false; } FILE* file = fopen(filename, "rb"); if (!file) { - error("There was an error opening the config file for reading"); + log_fatal("There was an error opening the config file for reading"); return false; } + // Regardless of what we do next, we _always_ want a MachineConfig instance. + + // instance() is by reference, so we can just get rid of an old instance and + // create a new one here: + { + auto& machineConfig = instance(); + if (machineConfig != nullptr) { + delete machineConfig; + } + machineConfig = new MachineConfig(); + } + MachineConfig* machine = instance(); + // Let's just read the entire file in one chunk for now. If we get // in trouble with this, we can cut it in pieces and read it per chunk. fseek(file, 0, SEEK_END); auto filesize = ftell(file); + log_debug("Configuration file is " << int(filesize) << " bytes."); + fseek(file, 0, SEEK_SET); - char* buffer = new char[filesize]; + char* buffer = new char[filesize + 1]; long pos = 0; while (pos < filesize) { @@ -366,11 +428,14 @@ bool MachineConfig::load(const char* filename) { } fclose(file); + buffer[filesize] = 0; + + log_debug("Read config file:\r\n" << buffer); if (pos != filesize) { delete[] buffer; - error("There was an error reading the config file"); + log_error("There was an error reading the config file"); return false; } @@ -381,27 +446,28 @@ bool MachineConfig::load(const char* filename) { Configuration::Parser parser(input.begin(), input.end()); Configuration::ParserHandler handler(parser); - // Instance is by reference, so we can just get rid of an old instance and - // create a new one here: - if (instance() != nullptr) { - delete instance(); - } - instance() = new MachineConfig(); - MachineConfig* machine = instance(); - for (; !parser.isEndSection(); parser.moveNext()) { - info("Parsing key " << parser.key().str()); + log_info("Parsing key " << parser.key().str()); machine->handle(handler); } - info("Done parsing machine config."); + log_info("Done parsing machine config. Running after-parse tasks"); + + try { + Configuration::AfterParse afterParse; + machine->afterParse(); + machine->handle(afterParse); + } catch (std::exception& ex) { log_info("Validation error: " << ex.what()); } + + log_info("Validating machine config"); try { Configuration::Validator validator; + machine->validate(); machine->handle(validator); - } catch (std::exception& ex) { info("Validation error: " << ex.what()); } + } catch (std::exception& ex) { log_info("Validation error: " << ex.what()); } - info("Done validating machine config."); + log_info("Done validating machine config."); succesful = true; @@ -410,13 +476,20 @@ bool MachineConfig::load(const char* filename) { // That way, we can always check if the yaml is there, and if it's not, load the yaml.new. } catch (const Configuration::ParseException& ex) { - error("Configuration parse error: " << ex.What() << " @ " << ex.LineNumber() << ":" << ex.ColumnNumber()); + auto startNear = ex.Near(); + auto endNear = (startNear + 10) > (buffer + filesize) ? (buffer + filesize) : (startNear + 10); + + StringRange near(startNear, endNear); + log_error("Configuration parse error: " << ex.What() << " @ " << ex.LineNumber() << ":" << ex.ColumnNumber() << " near " << near); } catch (const AssertionFailed& ex) { // Get rid of buffer and return - error("Configuration loading failed: " << ex.what()); - } catch (std::exception& ex) { error("Configuration validation error: " << ex.what()); } catch (...) { + log_error("Configuration loading failed: " << ex.what()); + } catch (std::exception& ex) { + // Log exception: + log_error("Configuration validation error: " << ex.what()); + } catch (...) { // Get rid of buffer and return - error("Unknown error occurred while processing configuration file."); + log_error("Unknown error occurred while processing configuration file."); } // Get rid of buffer and return diff --git a/Grbl_Esp32/src/MachineConfig.h b/Grbl_Esp32/src/MachineConfig.h index f3deca88..424c6fca 100644 --- a/Grbl_Esp32/src/MachineConfig.h +++ b/Grbl_Esp32/src/MachineConfig.h @@ -7,6 +7,8 @@ #include "CoolantControl.h" #include "Probe.h" +// TODO FIXME: Split this file up into several files, perhaps put it in some folder and namespace Machine? + namespace Motors { class Motor; } @@ -35,6 +37,7 @@ public: // Configuration system helpers: void validate() const override; void handle(Configuration::HandlerBase& handler) override; + void afterParse() override; ~Gang(); }; @@ -85,6 +88,7 @@ public: // Configuration system helpers: void validate() const override; void handle(Configuration::HandlerBase& handler) override; + void afterParse() override; // Checks if a motor matches this axis: bool hasMotor(const Motors::Motor* const motor) const; @@ -119,6 +123,7 @@ public: // Configuration helpers: void validate() const override; void handle(Configuration::HandlerBase& handler) override; + void afterParse() override; ~Axes(); }; @@ -199,7 +204,7 @@ public: } else if (ch != ' ') { // For convenience / layouting. return false; - } + } } if (tmp > 255) { return false; @@ -325,6 +330,7 @@ public: } void validate() const override; + void afterParse() override; void handle(Configuration::HandlerBase& handler) override; bool load(const char* file = "/spiffs/config.yaml"); diff --git a/Grbl_Esp32/src/Probe.cpp b/Grbl_Esp32/src/Probe.cpp index 3fb97dd7..d371ade6 100644 --- a/Grbl_Esp32/src/Probe.cpp +++ b/Grbl_Esp32/src/Probe.cpp @@ -30,7 +30,7 @@ void Probe::init() { static bool show_init_msg = true; // used to show message only once. - if (_probePin != Pin::UNDEFINED) { + if (!_probePin.undefined()) { #ifdef DISABLE_PROBE_PIN_PULL_UP _probePin.setAttr(Pin::Attr::Input); #else diff --git a/Grbl_Esp32/src/SimpleOutputStream.h b/Grbl_Esp32/src/SimpleOutputStream.h index 5e3d0541..0904b472 100644 --- a/Grbl_Esp32/src/SimpleOutputStream.h +++ b/Grbl_Esp32/src/SimpleOutputStream.h @@ -59,4 +59,4 @@ inline SimpleOutputStream& operator<<(SimpleOutputStream& lhs, StringRange v) { inline SimpleOutputStream& operator<<(SimpleOutputStream& lhs, const Pin& v) { lhs.add(v); return lhs; -} \ No newline at end of file +}