KBF-15 improve error handling

include/kbf/exception.h

@@ -7,38 +7,40 @@
 
 #include <esp_err.h>
 
-class KBFError : public std::exception {
-public:
-    explicit KBFError(const std::string &message) : message("KBF error: " + message) {}
-
-    const std::string message;
-
-    [[nodiscard]] const char *what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT override {
-        return message.c_str();
-    }
-};
-
-class ESPError : public std::exception {
-    const esp_err_t err;
-public:
-    explicit ESPError(const esp_err_t err) : err(err) {};
-
-    [[nodiscard]] const char *what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT override {
-        return esp_err_to_name(err);
-    }
-};
-
-class RTOSError : public std::exception {
-    const std::string message;
-public:
-    explicit RTOSError(const uint32_t err) : message("RTOS error: " + std::to_string(err)) {};
-
-    [[nodiscard]] const char *what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT override {
-        return message.c_str();
-    }
-};
-
-class OutOfMemoryError : public std::exception {
-};
+namespace kbf::exception {
+    class KBFError : public std::exception {
+    public:
+        explicit KBFError(const std::string &message) : message("KBF error: " + message) {}
+
+        const std::string message;
+
+        [[nodiscard]] const char *what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT override {
+            return message.c_str();
+        }
+    };
+
+    class ESPError : public std::exception {
+        const esp_err_t err;
+    public:
+        explicit ESPError(const esp_err_t err) : err(err) {};
+
+        [[nodiscard]] const char *what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT override {
+            return esp_err_to_name(err);
+        }
+    };
+
+    class RTOSError : public std::exception {
+        const std::string message;
+    public:
+        explicit RTOSError(const uint32_t err) : message("RTOS error: " + std::to_string(err)) {};
+
+        [[nodiscard]] const char *what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT override {
+            return message.c_str();
+        }
+    };
+
+    class OutOfMemoryError : public std::exception {
+    };
+}
 
 #endif //KBF_EXCEPTION_H

include/kbf/http/client.h

@@ -64,16 +64,6 @@ namespace kbf::http {
         post(const std::string &url, const nlohmann::json &data,
              const std::optional<const std::map<std::string, std::string>> &headers = std::nullopt);
 
-        /**
-         * @brief Called on HTTP_EVENT_ERROR event.
-         *
-         * @note This will be called for low-level errors (e.g. socket error). If the request went through normally,
-         * but the server responded with an error code (HTTP 4XX), onSuccess() will be called instead.
-         *
-         * @param client reference to the Client that issued the request
-         */
-        void (*onError)(Client &client) = nullptr;
-
         /**
          * @brief Called after the response is received.
          *

include/kbf/http/exception.h

@@ -0,0 +1,26 @@
+#ifndef KBF_HTTP_EXCEPTION_H
+#define KBF_HTTP_EXCEPTION_H
+
+namespace kbf::http::exception {
+    /**
+     * @brief Thrown when connection could not be established.
+     */
+    class ConnectionError : public std::exception {
+    };
+
+    /**
+     * @brief Thrown when the request failed and there was no timeout set.
+     * @warning Currently we can not really distinguish between timeout and other errors.
+     */
+    class RequestError : public std::exception {
+    };
+
+    /**
+     * @brief Thrown when the request has failed and there was a timeout set.
+     * @warning Currently we can not really distinguish between timeout and other errors.
+     */
+    class Timeout : public std::exception {
+    };
+}
+
+#endif //KBF_HTTP_EXCEPTION_H

include/kbf/macros.h

@@ -5,6 +5,24 @@
 #include <freertos/task.h>
 #include <esp_err.h>
 #include <esp_log.h>
+#include <kbf/exception.h>
+
+#ifndef CHECK_ABORT
+#define CHECK_ABORT(x) do {                           \
+    esp_err_t __err = (x);                            \
+    if (__err != ESP_OK) {                            \
+      ESP_LOGE("CHECK", "error check failed\n"        \
+           "\tat %s, line %d, in %s\n"                \
+           "\t\tline:\t%s\n\t\terror:\t%d (%s)",      \
+           __FILE__, __LINE__, __ASSERT_FUNC, #x,     \
+           __err, esp_err_to_name(__err));            \
+      ESP_LOGE("CHECK", "aborting");                  \
+      abort();                                        \
+    }                                                 \
+  } while (0)
+#else
+#error CHECK_ABORT macro already defined
+#endif
 
 #ifndef CHECK
 #define CHECK(x) do {                                 \
@@ -15,9 +33,7 @@
            "\t\tline:\t%s\n\t\terror:\t%d (%s)",      \
            __FILE__, __LINE__, __ASSERT_FUNC, #x,     \
            __err, esp_err_to_name(__err));            \
-      ESP_LOGE("CHECK", "aborting in 30 seconds..."); \
-      vTaskDelay(30000 / portTICK_PERIOD_MS);         \
-      abort();                                        \
+      throw kbf::exception::ESPError(x);              \
     }                                                 \
   } while (0)
 #else

include/kbf/task.h

@@ -126,9 +126,9 @@ namespace kbf::task {
 
         esp_err_t err = xTaskCreate(Task::runInternal, name.c_str(), stackSize, task.get(), priority, &task->handle);
         if (err == errCOULD_NOT_ALLOCATE_REQUIRED_MEMORY) {
-            throw OutOfMemoryError();
+            throw kbf::exception::OutOfMemoryError();
         } else if (err != pdPASS) {
-            throw RTOSError(err);
+            throw kbf::exception::RTOSError(err);
         }
 
         Task::TaskList::add(task);

src/gpio.cpp

@@ -92,5 +92,5 @@ bool gpio::Input::isLow() const {
 }
 
 gpio::Input::~Input() {
-    CHECK(gpio_isr_handler_remove(pin));
+    CHECK_ABORT(gpio_isr_handler_remove(pin));
 }

src/http/client.cpp

@@ -2,8 +2,9 @@
 
 #include <freertos/task.h>
 #include <esp_log.h>
-#include <kbf/exception.h>
 
+#include "kbf/exception.h"
+#include "kbf/http/exception.h"
 #include "kbf/macros.h"
 
 using namespace kbf;
@@ -35,7 +36,7 @@ void http::Client::init() {
 
 http::Client::~Client() {
     ESP_LOGD(TAG, "~Client()");
-    CHECK(esp_http_client_cleanup(handle));
+    CHECK_ABORT(esp_http_client_cleanup(handle));
 }
 
 std::shared_ptr<kbf::http::Response>
@@ -64,7 +65,7 @@ http::Client::performRequest(const kbf::http::Method method, const std::string &
         esp_http_client_set_post_field(handle, body.c_str(), body.length());
     } else {
         ESP_LOGE(TAG, "unhandled method: %d", method);
-        throw KBFError("unknown HTTP method " + std::to_string(method));
+        throw kbf::exception::KBFError("unknown HTTP method " + std::to_string(method));
     }
 
     if (headers) {
@@ -91,9 +92,20 @@ http::Client::performRequest(const kbf::http::Method method, const std::string &
     } else if (err == ESP_OK) {
         ESP_LOGD(TAG, "success");
         return response;
-    } else if ((err == ESP_ERR_HTTP_FETCH_HEADER || err == ESP_FAIL) && !retry) {
-        // TODO this will happen on timeout as well; perhaps set a retry limit in ctor / get / post?
-        ESP_LOGW(TAG, "request failed, maybe the connection was dropped? retrying...");
+    } else if ((err == ESP_ERR_HTTP_FETCH_HEADER || err == ESP_FAIL)) {
+        // TODO happens on timeout and conn reset as well; can we distinguish between errors here properly?
+
+        if (retry) {
+            ESP_LOGE(TAG, "retry failed (%s)", esp_err_to_name(err));
+            throw exception::RequestError();
+        }
+
+        if (timeoutMs) {
+            ESP_LOGE(TAG, "request failed (%s), assuming timeout", esp_err_to_name(err));
+            throw exception::Timeout();
+        }
+
+        ESP_LOGW(TAG, "request failed (%s), maybe the connection was dropped? retrying...", esp_err_to_name(err));
         retry = true;
         esp_http_client_cleanup(handle);
         init();
@@ -102,13 +114,16 @@ http::Client::performRequest(const kbf::http::Method method, const std::string &
         } else if (method == http::POST) {
             return post(url, *postData, headers);
         } else {
-            throw KBFError("unknown HTTP method " + std::to_string(method));
+            throw kbf::exception::KBFError("unknown HTTP method " + std::to_string(method));
         }
+    } else if (err == ESP_ERR_HTTP_CONNECT) {
+        ESP_LOGW(TAG, "HTTP request failed: %s", esp_err_to_name(err));
+        running = false;
+        throw exception::ConnectionError();
     } else {
-        ESP_LOGE(TAG, "HTTP request failed: %s", esp_err_to_name(err));
-        if (onError) onError(*this);
+        ESP_LOGE(TAG, "unhandled HTTP error: %s", esp_err_to_name(err));
         running = false;
-        return nullptr;
+        throw kbf::exception::KBFError("unhandled HTTP error: %s" + string(esp_err_to_name(err)));
     }
 }
 
@@ -117,11 +132,10 @@ esp_err_t http::Client::handleHttpEvent(esp_http_client_event_t *event) {
 
     switch (event->event_id) {
         case HTTP_EVENT_ERROR:
-            ESP_LOGW(TAG, "fixme: unhandled error event: HTTP_EVENT_ERROR");
-            if (client->onError) client->onError(*client);
+            ESP_LOGE(TAG, "fixme: unhandled error event: HTTP_EVENT_ERROR");
             client->running = false;
             client->retry   = false;
-            break;
+            throw kbf::exception::KBFError("HTTP_EVENT_ERROR");
         case HTTP_EVENT_ON_CONNECTED:
             ESP_LOGD(TAG, "connected");
             break;
@@ -135,8 +149,8 @@ esp_err_t http::Client::handleHttpEvent(esp_http_client_event_t *event) {
         case HTTP_EVENT_ON_DATA:
             ESP_LOGD(TAG, "data received");
             if (esp_http_client_is_chunked_response(client->handle)) {
-                ESP_LOGE(TAG, "received chunked data");
-                ABORT("not implemented");
+                ESP_LOGE(TAG, "receiving chunked data not implemented");
+                throw kbf::exception::KBFError("HTTP client chunked data not implemented");
             }
             client->buffer.append((char *) event->data, event->data_len);
             break;

src/task.cpp

@@ -26,7 +26,7 @@ void Task::TaskList::remove(uint32_t id) {
         }
     }
     ESP_LOGE(TAG, "%s(%d): not found", __func__, id);
-    throw KBFError("TaskList::remove(" + std::to_string(id) + ") failed");
+    throw kbf::exception::KBFError("TaskList::remove(" + std::to_string(id) + ") failed");
 }
 
 Task::Task(string name, void *arg, const uint32_t stackSize, const uint32_t priority) :

src/uart.cpp

@@ -44,9 +44,9 @@ UART::~UART() {
     if (err == ESP_ERR_TIMEOUT) {
         ESP_LOGE(TAG, "UART TX not done! Data will be corrupted.");
     } else {
-        CHECK(err);
+        CHECK_ABORT(err);
     }
-    CHECK(uart_driver_delete(port));
+    CHECK_ABORT(uart_driver_delete(port));
 }
 
 void UART::setPins(int rx, int tx, int rts, int cts) const {
@@ -69,7 +69,7 @@ void UART::write(const string &data) const {
 
     if (len != data.length() + 1) {
         ESP_LOGE(TAG, "sent bytes < data.length()");
-        ABORT("buffering support not yet implemented"); // TODO maybe just return len and leave it to the user?
+        throw kbf::exception::KBFError("UART buffering support not yet implemented");
     }
 }
 
@@ -105,11 +105,11 @@ void UART::waitFor(const string &signal) {
         if (event.type == UART_DATA) {
             if (event.size > BUFFER_SIZE) {
                 ESP_LOGE(TAG, "event size (%d) > buffer size (%d)", event.size, BUFFER_SIZE);
-                ABORT("fix me");
+                throw kbf::exception::KBFError("fix me");
             }
             auto buf = new char[BUFFER_SIZE];
             int  len = uart_read_bytes(uart->port, buf, event.size, portMAX_DELAY);
-            if (len < 0) CHECK(len); // will abort
+            if (len < 0) CHECK(len);
             ESP_LOGD(TAG, "read %d bytes", len);
 
             uart->mutex.lock();

test/test_gpio.cpp

@@ -5,8 +5,9 @@
 
 #include <unity.h>
 
+// TODO use Kconfig
 #define INPUT_PIN   27
-#define INPUT_PIN_2 17
+#define INPUT_PIN_2 34
 #define OUTPUT_PIN  13
 
 using namespace std;
@@ -15,11 +16,11 @@ using namespace kbf;
 static atomic<int> counter = {0};
 
 TEST_CASE("GPIO", "[kbf_gpio]") {
-    auto input   = new gpio::Input(INPUT_PIN);  // TODO use Kconfig
+    auto input   = new gpio::Input(INPUT_PIN);
     auto notUsed = new gpio::Input(INPUT_PIN_2);
     TEST_ASSERT_TRUE(notUsed->isLow())
 
-    auto output = gpio::Output(OUTPUT_PIN);  // TODO use Kconfig
+    auto output = gpio::Output(OUTPUT_PIN);
     TEST_ASSERT_EQUAL(OUTPUT_PIN, output.pin);
 
     kbf::sleep(100);

test/test_http.cpp

@@ -4,8 +4,9 @@
 
 #include "kbf.h"
 #include "kbf/wifi.h"
-#include "kbf/http/server.h"
 #include "kbf/http/client.h"
+#include "kbf/http/server.h"
+#include "kbf/http/exception.h"
 
 #include <unity.h>
 
@@ -169,6 +170,7 @@ TEST_CASE("HTTP timeout", "[kbf_http]") {
 
     auto server = http::Server();
     http::Response (*handler)(const http::Request &, void *) = {[](const http::Request &request, void *) {
+        state++;
         kbf::sleep(stoi(request.query.at("sleep")));
         return http::Response("OK");
     }};
@@ -178,15 +180,19 @@ TEST_CASE("HTTP timeout", "[kbf_http]") {
 
     state = 0;
     auto client = http::Client(1000);
-    client.onError = {[](http::Client &) {
-        TEST_ASSERT_EQUAL(0, state++);
-    }};
 
     TEST_ASSERT_EQUAL_STRING("OK", client.get("http://localhost/?sleep=200")->body.c_str());
-    TEST_ASSERT_EQUAL(0, state);
-    TEST_ASSERT_NULL(client.get("http://localhost/?sleep=1200"))
     TEST_ASSERT_EQUAL(1, state);
 
+    bool caught = false;
+    try {
+        client.get("http://localhost/?sleep=1200");
+    } catch (http::exception::Timeout &e) {
+        caught = true;
+    }
+    TEST_ASSERT_TRUE(caught);
+    TEST_ASSERT_EQUAL(2, state);
+
     wifi::stop();
 }
 
@@ -241,8 +247,13 @@ TEST_CASE("HTTP Client errors", "[kbf_http]") {
     response = client.post("http://localhost/test", {{"foo", "bar"}}, {{{"X-Hax", "hax"}}});
     TEST_ASSERT_EQUAL_STRING("OK", response->body.c_str());
 
-    response = client.get("http://non-existent-host-blabla.com/");
-    TEST_ASSERT_NULL(response);
+    bool caught = false;
+    try {
+        response = client.get("http://connection-refused.com/");
+    } catch (http::exception::ConnectionError &e) {
+        caught = true;
+    }
+    TEST_ASSERT_TRUE(caught);
 
     wifi::stop();
 }

test/test_task.cpp

@@ -138,7 +138,7 @@ TEST_CASE("Task out of memory", "[kbf_task]") {
     bool caught = false;
     try {
         task::start<Quitter>("test_task_oom", &quitterArg, 2 << 24);
-    } catch (OutOfMemoryError &e) {
+    } catch (kbf::exception::OutOfMemoryError &e) {
         caught = true;
     }