Performance improvements, better logging, destructor finally implemented

src/asynchttprequest.cpp

@@ -5,6 +5,7 @@
 
 // system includes
 #include <chrono>
+#include <string_view>
 #include <cstring>
 #include <assert.h>
 
@@ -23,7 +24,10 @@ constexpr const char * const TAG = "ASYNC_HTTP";
 
 constexpr int TASK_RUNNING = BIT0;
 constexpr int START_REQUEST_BIT = BIT1;
-constexpr int REQUEST_FINISHED_BIT = BIT2;
+constexpr int REQUEST_RUNNING_BIT = BIT2;
+constexpr int REQUEST_FINISHED_BIT = BIT3;
+constexpr int END_TASK_BIT = BIT4;
+constexpr int TASK_ENDED = BIT5;
 } // namespace
 
 AsyncHttpRequest::AsyncHttpRequest(const char *taskName, espcpputils::CoreAffinity coreAffinity) :
@@ -31,21 +35,132 @@ AsyncHttpRequest::AsyncHttpRequest(const char *taskName, espcpputils::CoreAffini
     m_coreAffinity{coreAffinity}
 {
     assert(eventGroup.handle);
-
-    eventGroup.clearBits(TASK_RUNNING);
-    eventGroup.clearBits(START_REQUEST_BIT);
-    eventGroup.clearBits(REQUEST_FINISHED_BIT);
-
-    if (const auto failed = startTask())
-    {
-        ESP_LOGE(TAG, "%s", failed->c_str());
-    }
 }
 
 AsyncHttpRequest::~AsyncHttpRequest()
 {
-    // TODO exit task
+    endTask();
-    abort();
+}
+
+std::optional<std::string> AsyncHttpRequest::startTask()
+{
+    if (const auto bits = eventGroup.getBits();
+        bits & TASK_RUNNING || taskHandle)
+    {
+        constexpr auto msg = "task already started";
+        ESP_LOGW(TAG, "%s", msg);
+        return msg;
+    }
+
+    eventGroup.clearBits(TASK_RUNNING | START_REQUEST_BIT | REQUEST_RUNNING_BIT | REQUEST_FINISHED_BIT | END_TASK_BIT | TASK_ENDED);
+
+    const auto result = espcpputils::createTask(requestTask, m_taskName, 4096, this, 10, &taskHandle, m_coreAffinity);
+    if (result != pdPASS)
+    {
+        auto msg = std::string{"failed creating http task "} + std::to_string(result);
+        ESP_LOGE(TAG, "%s", msg.c_str());
+        return msg;
+    }
+
+    if (!taskHandle)
+    {
+        constexpr auto msg = "http task handle is null";
+        ESP_LOGW(TAG, "%s", msg);
+        return msg;
+    }
+
+    ESP_LOGD(TAG, "created http task %s", m_taskName);
+
+    if (const auto bits = eventGroup.waitBits(TASK_RUNNING, false, false, std::chrono::ceil<espcpputils::ticks>(1s).count());
+        bits & TASK_RUNNING)
+        return std::nullopt;
+
+    ESP_LOGW(TAG, "http task %s TASK_RUNNING bit not yet set...", m_taskName);
+
+    while (true)
+        if (const auto bits = eventGroup.waitBits(TASK_RUNNING, false, false, portMAX_DELAY);
+            bits & TASK_RUNNING)
+            break;
+
+    return std::nullopt;
+}
+
+std::optional<std::string> AsyncHttpRequest::endTask()
+{
+    if (const auto bits = eventGroup.getBits();
+        !(bits & TASK_RUNNING))
+        return std::nullopt;
+    else if (bits & END_TASK_BIT)
+    {
+        constexpr auto msg = "Another end request is already pending";
+        ESP_LOGE(TAG, "%s", msg);
+        return msg;
+    }
+
+    eventGroup.setBits(END_TASK_BIT);
+
+    if (const auto bits = eventGroup.waitBits(TASK_ENDED, true, false, std::chrono::ceil<espcpputils::ticks>(1s).count());
+        bits & TASK_ENDED)
+    {
+        ESP_LOGD(TAG, "http task %s ended", m_taskName);
+        return std::nullopt;
+    }
+
+    ESP_LOGW(TAG, "http task %s TASK_ENDED bit not yet set...", m_taskName);
+
+    while (true)
+        if (const auto bits = eventGroup.waitBits(TASK_ENDED, true, false, portMAX_DELAY);
+            bits & TASK_ENDED)
+            break;
+
+    ESP_LOGD(TAG, "http task %s ended", m_taskName);
+
+    return std::nullopt;
+}
+
+std::optional<std::string> AsyncHttpRequest::createClient(const std::string &url)
+{
+    if (client)
+    {
+        constexpr auto msg = "client already created";
+        ESP_LOGE(TAG, "%s", msg);
+        return msg;
+    }
+
+    esp_http_client_config_t config{};
+    config.url = url.c_str();
+    config.event_handler = httpEventHandler;
+    config.user_data = this;
+
+    client = espcpputils::http_client{&config};
+
+    if (!client)
+    {
+        constexpr auto msg = "http client could not be constructed";
+        ESP_LOGE(TAG, "%s", msg);
+        return msg;
+    }
+
+    ESP_LOGD(TAG, "created http client %s", m_taskName);
+
+    return std::nullopt;
+}
+
+std::optional<std::string> AsyncHttpRequest::deleteClient()
+{
+    if (!client)
+        return std::nullopt;
+
+    if (inProgress())
+    {
+        constexpr auto msg = "another request still in progress";
+        ESP_LOGW(TAG, "%s", msg);
+        return msg;
+    }
+
+    client = {};
+
+    return std::nullopt;
 }
 
 std::optional<std::string> AsyncHttpRequest::start(const std::string &url)
@@ -56,28 +171,39 @@ std::optional<std::string> AsyncHttpRequest::start(const std::string &url)
             return *failed;
     }
 
+    if (inProgress())
+    {
+        constexpr auto msg = "another request still in progress";
+        ESP_LOGW(TAG, "%s", msg);
+        return msg;
+    }
+
+    if (!client)
+    {
+        if (const auto failed = createClient(url))
+            return *failed;
+    }
+    else
+    {
+        const auto result = client.set_url(url.c_str());
+        ESP_LOG_LEVEL_LOCAL((result == ESP_OK ? ESP_LOG_DEBUG : ESP_LOG_ERROR), TAG, "client.set_url() returned: %s (%s)", esp_err_to_name(result), url.c_str());
+        if (result != ESP_OK)
+            return std::string{"client.set_url() failed: "} + esp_err_to_name(result) + " (" + url + ')';
+    }
+
     buf.clear();
-    this->url = url;
 
-    config = esp_http_client_config_t{};
+    clearFinished();
-    config.url = url.c_str();
-    config.event_handler = httpEventHandler;
-    config.user_data = this;
-
-    request.construct(&config);
-
-    auto helper = cpputils::makeCleanupHelper([&request=request](){ request.destruct(); });
-
-    if (!request->handle)
-        return "request could not be constructed";
-
-    eventGroup.clearBits(REQUEST_FINISHED_BIT);
     eventGroup.setBits(START_REQUEST_BIT);
-    helper.disarm(); // this stops the helper from destructing the request
 
     return std::nullopt;
 }
 
+bool AsyncHttpRequest::inProgress() const
+{
+    return eventGroup.getBits() & (START_REQUEST_BIT | REQUEST_RUNNING_BIT);
+}
+
 bool AsyncHttpRequest::finished() const
 {
     return eventGroup.getBits() & REQUEST_FINISHED_BIT;
@@ -85,24 +211,37 @@ bool AsyncHttpRequest::finished() const
 
 std::optional<std::string> AsyncHttpRequest::failed() const
 {
+    if (const auto bits = eventGroup.getBits();
+        bits & REQUEST_RUNNING_BIT)
+    {
+        constexpr auto msg = "request still running";
+        ESP_LOGW(TAG, "%s", msg);
+        return msg;
+    }
+    else if (!(bits & REQUEST_FINISHED_BIT))
+    {
+        constexpr auto msg = "request not finished";
+        ESP_LOGW(TAG, "%s", msg);
+        return msg;
+    }
+
     if (result != ESP_OK)
         return std::string{"http request failed: "} + esp_err_to_name(result);
 
-    if (request->get_status_code() != 200)
+    if (statusCode != HttpStatus_Ok)
-        return std::string{"http request failed: "} + std::to_string(request->get_status_code());
+        return std::string{"http request failed: "} + std::to_string(statusCode);
 
     return std::nullopt;
 }
 
-void AsyncHttpRequest::cleanup()
+void AsyncHttpRequest::clearFinished()
 {
-    request.destruct();
+    eventGroup.clearBits(REQUEST_FINISHED_BIT);
-    buf.clear();
 }
 
 esp_err_t AsyncHttpRequest::httpEventHandler(esp_http_client_event_t *evt)
 {
-    auto _this = (AsyncHttpRequest*)evt->user_data;
+    auto _this = reinterpret_cast<AsyncHttpRequest*>(evt->user_data);
 
     assert(_this);
 
@@ -126,10 +265,12 @@ esp_err_t AsyncHttpRequest::httpEventHandler(esp_http_client_event_t *evt)
         }
         break;
     case HTTP_EVENT_ON_DATA:
-        //assert(!_this->request->is_chunked_response());
+        if (!evt->data)
-
+            ESP_LOGW(TAG, "handler with invalid data ptr");
-        if (evt->data && evt->data_len)
+        else if (evt->data_len <= 0)
-            _this->buf += std::string((const char *)evt->data, evt->data_len);
+            ESP_LOGW(TAG, "handler with invalid data_len %i", evt->data_len);
+        else
+            _this->buf += std::string_view((const char *)evt->data, evt->data_len);
 
         break;
     default:;
@@ -140,47 +281,58 @@ esp_err_t AsyncHttpRequest::httpEventHandler(esp_http_client_event_t *evt)
 
 void AsyncHttpRequest::requestTask(void *ptr)
 {
-    auto helper = cpputils::makeCleanupHelper([](){ vTaskDelete(NULL); });
+    auto _this = reinterpret_cast<AsyncHttpRequest*>(ptr);
-
-    auto _this = (AsyncHttpRequest*)ptr;
 
     assert(_this);
+
     _this->eventGroup.setBits(TASK_RUNNING);
-    auto helper2 = cpputils::makeCleanupHelper([=](){ _this->eventGroup.clearBits(TASK_RUNNING); });
+
+    // cleanup on task exit
+    auto helper = cpputils::makeCleanupHelper([&](){
+        _this->eventGroup.clearBits(TASK_RUNNING);
+        _this->eventGroup.setBits(TASK_ENDED);
+        _this->taskHandle = NULL;
+        vTaskDelete(NULL);
+    });
 
     while (true)
     {
-        {
+        if (const auto bits = _this->eventGroup.waitBits(START_REQUEST_BIT|END_TASK_BIT, true, false, portMAX_DELAY);
-            const auto bits = _this->eventGroup.waitBits(START_REQUEST_BIT, true, true, espcpputils::ticks{1s}.count());
+            bits & END_TASK_BIT)
-            if (!(bits & START_REQUEST_BIT))
+            break;
+        else if (!(bits & START_REQUEST_BIT))
             continue;
-        }
 
-        assert(_this->request.constructed());
+        assert(_this->client);
-        assert(!(_this->eventGroup.getBits() & REQUEST_FINISHED_BIT));
 
-        do
         {
-            _this->result = _this->request->perform();
+            const auto bits = _this->eventGroup.getBits();
+            assert(!(bits & START_REQUEST_BIT));
+            assert(!(bits & REQUEST_RUNNING_BIT));
+            assert(!(bits & REQUEST_FINISHED_BIT));
         }
-        while (_this->result == EAGAIN || _this->result == EINPROGRESS);
 
+        _this->eventGroup.setBits(REQUEST_RUNNING_BIT);
+
+        auto helper2 = cpputils::makeCleanupHelper([&](){
+            _this->eventGroup.clearBits(REQUEST_RUNNING_BIT);
             _this->eventGroup.setBits(REQUEST_FINISHED_BIT);
-    }
+        });
-}
 
-std::optional<std::string> AsyncHttpRequest::startTask()
         {
-    if (taskHandle)
+            esp_err_t result;
-        return "task already started";
+            do
-
+                result = _this->client.perform();
-    const auto result = espcpputils::createTask(requestTask, m_taskName, 4096, this, 10, &taskHandle, m_coreAffinity);
+            while (result == EAGAIN || result == EINPROGRESS);
-    if (result != pdPASS)
+            ESP_LOG_LEVEL_LOCAL((result == ESP_OK ? ESP_LOG_VERBOSE : ESP_LOG_ERROR), TAG, "client.perform() returned: %s", esp_err_to_name(result));
-        return std::string{"failed creating http task "} + std::to_string(result);
+            _this->result = result;
-
+            _this->statusCode = _this->client.get_status_code();
-    if (!taskHandle)
+        }
-        return "http task handle is null";
+
-
+        // workaround for esp-idf bug, every request after the first one fails with ESP_ERR_HTTP_FETCH_HEADER
-    ESP_LOGD(TAG, "created http task");
+        const auto result = _this->client.close();
-    return std::nullopt;
+        ESP_LOG_LEVEL_LOCAL((result == ESP_OK ? ESP_LOG_VERBOSE : ESP_LOG_ERROR), TAG, "client.close() returned: %s", esp_err_to_name(result));
+        if (result != ESP_OK)
+            _this->client = {};
+    }
 }

src/asynchttprequest.h

@@ -10,7 +10,6 @@
 #include <esp_err.h>
 
 // local includes
-#include "delayedconstruction.h"
 #include "wrappers/http_client.h"
 #include "wrappers/event_group.h"
 #include "taskutils.h"
@@ -21,29 +20,34 @@ public:
     AsyncHttpRequest(const char *taskName="httpRequestTask", espcpputils::CoreAffinity coreAffinity=espcpputils::CoreAffinity::Core1);
     ~AsyncHttpRequest();
 
+    std::optional<std::string> startTask();
+    std::optional<std::string> endTask();
+
+    std::optional<std::string> createClient(const std::string &url);
+    std::optional<std::string> deleteClient();
+
     std::optional<std::string> start(const std::string &url);
 
+    bool inProgress() const;
+
     bool finished() const;
     std::optional<std::string> failed() const;
 
+    void clearFinished();
+
     const std::string &buffer() const { return buf; }
     std::string &&takeBuffer() { return std::move(buf); }
 
-    void cleanup();
-
 private:
     static esp_err_t httpEventHandler(esp_http_client_event_t *evt);
     static void requestTask(void *ptr);
-    std::optional<std::string> startTask();
 
-    cpputils::DelayedConstruction<espcpputils::http_client> request;
+    espcpputils::http_client client;
     std::string buf;
     TaskHandle_t taskHandle{NULL};
     espcpputils::event_group eventGroup;
     esp_err_t result{};
-
+    int statusCode{};
-    std::string url;
-    esp_http_client_config_t config;
 
     const char * const m_taskName;
     const espcpputils::CoreAffinity m_coreAffinity;