Better error handling in SDCard

Grbl_Esp32/src/Error.cpp

@@ -62,7 +62,7 @@ std::map<Error, const char*> ErrorNames = {
     { Error::GcodeMaxValueExceeded, "Gcode max value exceeded" },
     { Error::PParamMaxExceeded, "P param max exceeded" },
     { Error::FsFailedMount, "Failed to mount device" },
-    { Error::FsFailedRead, "Failed to read" },
+    { Error::FsFailedRead, "Read failed" },
     { Error::FsFailedOpenDir, "Failed to open directory" },
     { Error::FsDirNotFound, "Directory not found" },
     { Error::FsFileEmpty, "File empty" },
@@ -79,6 +79,8 @@ std::map<Error, const char*> ErrorNames = {
     { Error::NvsSetFailed, "Failed to store setting" },
     { Error::NvsGetStatsFailed, "Failed to get setting status" },
     { Error::AuthenticationFailed, "Authentication failed!" },
+    { Error::Eol, "End of line" },
+    { Error::Eof, "End of file" },
     { Error::AnotherInterfaceBusy, "Another interface is busy" },
     { Error::BadPinSpecification, "Bad Pin Specification" },
     { Error::JogCancelled, "Jog Cancelled" },

Grbl_Esp32/src/Error.h

@@ -85,6 +85,7 @@ enum class Error : uint8_t {
     NvsGetStatsFailed           = 101,
     AuthenticationFailed        = 110,
     Eol                         = 111,
+    Eof                         = 112,  // Not necessarily an error
     AnotherInterfaceBusy        = 120,
     JogCancelled                = 130,
     BadPinSpecification         = 150,

Grbl_Esp32/src/Machine/SPIBus.cpp

@@ -17,6 +17,7 @@
 */
 
 #include "SPIBus.h"
+#include "../Report.h"
 
 #include <SPI.h>
 
@@ -32,11 +33,19 @@ namespace Machine {
 
     void SPIBus::init() {
         if (_cs.defined()) {  // validation ensures the rest is also defined.
+            info_serial(
+                "SPI SCK:%s MOSI:%s MISO:%s CS:%s", _sck.name().c_str(), _mosi.name().c_str(), _miso.name().c_str(), _cs.name().c_str());
+
+            _cs.setAttr(Pin::Attr::Output);
+
             auto csPin   = _cs.getNative(Pin::Capabilities::Output | Pin::Capabilities::Native);
             auto mosiPin = _mosi.getNative(Pin::Capabilities::Output | Pin::Capabilities::Native);
             auto sckPin  = _sck.getNative(Pin::Capabilities::Output | Pin::Capabilities::Native);
             auto misoPin = _miso.getNative(Pin::Capabilities::Input | Pin::Capabilities::Native);
 
+            // Start the SPI bus with the pins defined here.  Once it has been started,
+            // those pins "stick" and subsequent attempts to restart it with defaults
+            // for the miso, mosi, and sck pins are ignored
             SPI.begin(sckPin, misoPin, mosiPin, csPin);
         }
     }
@@ -48,13 +57,19 @@ namespace Machine {
         handler.item("sck", _sck);
     }
 
+    // XXX it would be nice to have some way to turn off SPI entirely
     void SPIBus::afterParse() {
-        if (_cs.undefined() && _miso.undefined() && _mosi.undefined() && _sck.undefined()) {
+        if (_cs.undefined()) {
-            // Default SPI miso, mosi, sck, cs pins to the "standard" gpios 19, 23, 18, 5
-            _miso = Pin::create("gpio.19");
-            _mosi = Pin::create("gpio.23");
-            _sck  = Pin::create("gpio.18");
             _cs = Pin::create("gpio.5");
         }
+        if (_miso.undefined()) {
+            _miso = Pin::create("gpio.19");
+        }
+        if (_mosi.undefined()) {
+            _mosi = Pin::create("gpio.23");
+        }
+        if (_sck.undefined()) {
+            _sck = Pin::create("gpio.18");
+        }
     }
 }

Grbl_Esp32/src/Protocol.cpp

@@ -167,16 +167,22 @@ void protocol_main_loop() {
     int c;
     for (;;) {
         auto sdcard = config->_sdCard;
-        if (sdcard->_ready_next) {
+        // _readyNext indicates that input is coming from a file and
+        // the GCode system is ready for another line.
+        if (sdcard->_readyNext) {
             char  fileLine[255];
-            if (sdcard->readFileLine(fileLine, 255)) {
+            Error res;
-                sdcard->_ready_next = false;
+            switch (res = sdcard->readFileLine(fileLine, 255)) {
+                case Error::Ok:
+                    sdcard->_readyNext = false;
+#ifdef DEBUG_REPORT_ECHO_RAW_LINE_RECEIVED
+                    report_echo_line_received(fileLine, CLIENT_SERIAL);
+#endif
                     report_status_message(execute_line(fileLine, sdcard->_client, sdcard->_auth_level), sdcard->_client);
-            } else {
+                    break;
-                char temp[50];
+                default:
-                sdcard->get_current_filename(temp);
+                    report_status_message(res, sdcard->_client);
-                grbl_notifyf("SD print done", "%s print is successful", temp);
+                    break;
-                sdcard->closeFile();  // close file and clear SD ready/running flags
             }
         }
         // Receive one line of incoming serial data, as the data becomes available.

Grbl_Esp32/src/Report.cpp

@@ -241,30 +241,46 @@ static String report_util_axis_values(const float* axis_value) {
 // from a critical error, such as a triggered hard limit. Interface should always monitor for these
 // responses.
 void report_status_message(Error status_code, uint8_t client) {
-    auto sdCard = config->_sdCard;
+    auto sdcard = config->_sdCard;
+    if (sdcard->get_state(false) == SDCard::State::BusyPrinting) {
+        // When running from SD, the GCode is not coming from a sender, so we are not
+        // using the Grbl send/response/error protocol.  We use _readyNext instead of
+        // "ok" to indicate readiness for another line, and we report verbose error
+        // messages with [MSG: ...] encapsulation
         switch (status_code) {
-        case Error::Ok:  // Error::Ok
+            case Error::Ok:
-            if (sdCard->get_state(false) == SDCard::State::BusyPrinting) {
+                sdcard->_readyNext = true;  // flag so system_execute_line() will send the next line
-                sdCard->_ready_next = true;  // flag so system_execute_line() will send the next line
+                break;
-            } else {
+            case Error::Eof:
-                grbl_send(client, "ok\r\n");
+                // XXX we really should wait for the machine to return to idle before
-            }
+                // we issue this message.  What Eof really means is that all the lines
+                // in the file were sent to Grbl.  Some could still be running.
+                grbl_notifyf("SD print done", "%s print succeeded", sdcard->filename());
+                info_client(sdcard->_client, "%s print succeeded", sdcard->filename());
+                sdcard->closeFile();
                 break;
             default:
-            // do we need to stop a running SD job?
+                info_client(sdcard->_client,
-            if (sdCard->get_state(false) == SDCard::State::BusyPrinting) {
+                            "Error:%d (%s) in %s at line %d",
+                            status_code,
+                            errorString(status_code),
+                            sdcard->filename(),
+                            sdcard->lineNumber());
                 if (status_code == Error::GcodeUnsupportedCommand) {
-                    grbl_sendf(client, "error:%d\r\n", status_code);  // most senders seem to tolerate this error and keep on going
+                    // Do not stop on unsupported commands because most senders do not
-                    grbl_sendf(CLIENT_ALL, "error:%d in SD file at line %d\r\n", status_code, sdCard->get_current_line_number());
+                    sdcard->_readyNext = true;
-                    // don't close file
-                    sdCard->_ready_next = true;  // flag so system_execute_line() will send the next line
                 } else {
-                    grbl_notifyf("SD print error", "Error:%d during SD file at line: %d", status_code, sdCard->get_current_line_number());
+                    grbl_notifyf("SD print error", "Error:%d in %s at line: %d", status_code, sdcard->filename(), sdcard->lineNumber());
-                    grbl_sendf(CLIENT_ALL, "error:%d in SD file at line %d\r\n", status_code, sdCard->get_current_line_number());
+                    sdcard->closeFile();
-                    sdCard->closeFile();
                 }
-                return;
         }
+    } else {
+        // Input is coming from a sender so use the classic Grbl line protocol
+        switch (status_code) {
+            case Error::Ok:  // Error::Ok
+                grbl_send(client, "ok\r\n");
+                break;
+            default:
                 // With verbose errors, the message text is displayed instead of the number.
                 // Grbl 0.9 used to display the text, while Grbl 1.1 switched to the number.
                 // Many senders support both formats.
@@ -273,6 +289,8 @@ void report_status_message(Error status_code, uint8_t client) {
                 } else {
                     grbl_sendf(client, "error:%d\r\n", static_cast<int>(status_code));
                 }
+                break;
+        }
     }
 }
 
@@ -306,8 +324,8 @@ std::map<Message, const char*> MessageText = {
 // is installed, the message number codes are less than zero.
 void report_feedback_message(Message message) {  // ok to send to all clients
     if (message == Message::SdFileQuit) {
-        grbl_notifyf("SD print canceled", "Reset during SD file at line: %d", config->_sdCard->get_current_line_number());
+        grbl_notifyf("SD print canceled", "Reset during SD file at line: %d", config->_sdCard->lineNumber());
-        info_serial("Reset during SD file at line: %d", config->_sdCard->get_current_line_number());
+        info_serial("Reset during SD file at line: %d", config->_sdCard->lineNumber());
 
     } else {
         auto it = MessageText.find(message);
@@ -757,8 +775,7 @@ void report_realtime_status(uint8_t client) {
     if (config->_sdCard->get_state(false) == SDCard::State::BusyPrinting) {
         sprintf(temp, "|SD:%4.2f,", config->_sdCard->report_perc_complete());
         strcat(status, temp);
-        config->_sdCard->get_current_filename(temp);
+        strcat(status, config->_sdCard->filename());
-        strcat(status, temp);
     }
 #ifdef DEBUG_STEPPER_ISR
     sprintf(temp, "|ISRs:%d", step_count);

Grbl_Esp32/src/SDCard.cpp

@@ -38,16 +38,6 @@ SDCard::SDCard() :
     _pImpl(new FileWrap()), _current_line_number(0), _state(State::Idle), _readyNext(false), _client(CLIENT_SERIAL),
     _auth_level(WebUI::AuthenticationLevel::LEVEL_GUEST) {}
 
-// attempt to mount the SD card
-/*bool SDCard::mount()
-{
-  if(!SD.begin()) {
-    report_status_message(Error::FsFailedMount, _client);
-    return false;
-  }
-  return true;
-}*/
-
 void SDCard::listDir(fs::FS& fs, const char* dirname, uint8_t levels, uint8_t client) {
     //char temp_filename[128]; // to help filter by extension	TODO: 128 needs a definition based on something
     File root = fs.open(dirname);
@@ -85,44 +75,44 @@ bool SDCard::openFile(fs::FS& fs, const char* path) {
 }
 
 bool SDCard::closeFile() {
-    if (!_pImpl->_file) {
-        return false;
-    }
     set_state(State::Idle);
     _readyNext           = false;
     _current_line_number = 0;
+    if (!_pImpl->_file) {
+        return false;
+    }
     _pImpl->_file.close();
     SD.end();
     return true;
 }
 
 /*
-  read a line from the SD card
+  Read a line from the SD card
-  strip whitespace
+  Returns true if a line was read, even if it was empty.
-  strip comments per http://linuxcnc.org/docs/ja/html/gcode/overview.html#gcode:comments
+  Returns false on EOF or error.  Errors display a message.
-  make uppercase
-  return true if a line is
 */
-bool SDCard::readFileLine(char* line, int maxlen) {
+Error SDCard::readFileLine(char* line, int maxlen) {
     if (!_pImpl->_file) {
-        report_status_message(Error::FsFailedRead, _client);
+        return Error::FsFailedRead;
-        return false;
     }
 
     _current_line_number += 1;
     int len = 0;
     while (_pImpl->_file.available()) {
         if (len >= maxlen) {
-            return false;
+            return Error::LineLengthExceeded;
+        }
+        int c = _pImpl->_file.read();
+        if (c < 0) {
+            return Error::FsFailedRead;
         }
-        char c = _pImpl->_file.read();
         if (c == '\n') {
             break;
         }
         line[len++] = c;
     }
     line[len] = '\0';
-    return len || _pImpl->_file.available();
+    return len || _pImpl->_file.available() ? Error::Ok : Error::Eof;
 }
 
 // return a percentage complete 50.5 = 50.5%
@@ -133,7 +123,7 @@ float SDCard::report_perc_complete() {
     return (float)_pImpl->_file.position() / (float)_pImpl->_file.size() * 100.0f;
 }
 
-uint32_t SDCard::get_current_line_number() {
+uint32_t SDCard::lineNumber() {
     return _current_line_number;
 }
 
@@ -181,12 +171,8 @@ SDCard::State SDCard::set_state(SDCard::State state) {
     return _state;
 }
 
-void SDCard::get_current_filename(char* name) {
+const char* SDCard::filename() {
-    if (_pImpl->_file) {
+    return _pImpl->_file ? _pImpl->_file.name() : "";
-        strcpy(name, _pImpl->_file.name());
-    } else {
-        name[0] = 0;
-    }
 }
 
 void SDCard::init() {

Grbl_Esp32/src/SDCard.h

@@ -18,6 +18,7 @@
 #include "Configuration/Configurable.h"
 #include "WebUI/Authentication.h"
 #include "Pin.h"
+#include "Error.h"
 
 #include <cstdint>
 
@@ -53,7 +54,8 @@ private:
     Pin   _cardDetect;
 
 public:
-    bool                       _readyNext;
+    bool _readyNext;  // Grbl has processed a line and is waiting for another
+
     uint8_t                    _client;
     WebUI::AuthenticationLevel _auth_level;
 
@@ -61,8 +63,6 @@ public:
     SDCard(const SDCard&) = delete;
     SDCard& operator=(const SDCard&) = delete;
 
-    bool _ready_next = false;  // Grbl has processed a line and is waiting for another
-
     //bool mount();
     SDCard::State get_state(bool refresh);
     SDCard::State set_state(SDCard::State state);
@@ -70,10 +70,11 @@ public:
     void     listDir(fs::FS& fs, const char* dirname, uint8_t levels, uint8_t client);
     bool     openFile(fs::FS& fs, const char* path);
     bool     closeFile();
-    bool     readFileLine(char* line, int len);
+    Error    readFileLine(char* line, int len);
     float    report_perc_complete();
-    uint32_t get_current_line_number();
+    uint32_t lineNumber();
-    void     get_current_filename(char* name);
+
+    const char* filename();
 
     // Initializes pins.
     void init();

Grbl_Esp32/src/WebUI/WebSettings.cpp

@@ -664,9 +664,13 @@ namespace WebUI {
         }
         config->_sdCard->_client = (espresponse) ? espresponse->client() : CLIENT_ALL;
         char  fileLine[255];
-        while (config->_sdCard->readFileLine(fileLine, 255)) {
+        Error res;
+        while ((res = config->_sdCard->readFileLine(fileLine, 255)) == Error::Ok) {
             webPrintln(fileLine);
         }
+        if (res != Error::Eof) {
+            webPrintln(errorString(res));
+        }
         webPrintln("");
         config->_sdCard->closeFile();
         return Error::Ok;
@@ -688,18 +692,18 @@ namespace WebUI {
         auto sdCard = config->_sdCard;
 
         char  fileLine[255];
-        if (!sdCard->readFileLine(fileLine, 255)) {
+        Error res = sdCard->readFileLine(fileLine, 255);
-            //No need notification here it is just a macro
+        if (res != Error::Ok) {
-            sdCard->closeFile();
+            report_status_message(res, sdCard->_client);
+            // report_status_message will close the file
             webPrintln("");
             return Error::Ok;
         }
         sdCard->_client     = (espresponse) ? espresponse->client() : CLIENT_ALL;
         sdCard->_auth_level = auth_level;
-        // execute the first line now; Protocol.cpp handles later ones when sdCard._ready_next
+        // execute the first line now; Protocol.cpp handles later ones when sdCard._readyNext
         report_status_message(execute_line(fileLine, sdCard->_client, sdCard->_auth_level), sdCard->_client);
         report_realtime_status(sdCard->_client);
-        webPrintln("");
         return Error::Ok;
     }