PR review fixes (#384)

Tactility/Include/Tactility/network/EspHttpClient.h

@@ -11,7 +11,7 @@ class EspHttpClient {
     static constexpr auto* TAG = "EspHttpClient";
 
     std::unique_ptr<esp_http_client_config_t> config = nullptr;
-    esp_http_client_handle_t client;
+    esp_http_client_handle_t client = nullptr;
     bool isOpen = false;
 
 public:
@@ -87,11 +87,15 @@ public:
     bool close() {
         assert(client != nullptr);
         TT_LOG_I(TAG, "close()");
-        return esp_http_client_close(client) == ESP_OK;
+        if (esp_http_client_close(client) == ESP_OK) {
+            isOpen = false;
+        }
+        return !isOpen;
     }
 
     bool cleanup() {
         assert(client != nullptr);
+        assert(!isOpen);
         TT_LOG_I(TAG, "cleanup()");
         const auto result = esp_http_client_cleanup(client);
         client = nullptr;

Tactility/Include/Tactility/network/Http.h

@@ -1,10 +1,19 @@
 #pragma once
 
 #include <string>
+#include <functional>
 
 namespace tt::network::http {
-
+    /**
-void download(
+     * Download a file from a URL.
+     * The server must send the Content-Length header.
+     * @param url download source URL
+     * @param certFilePath the path to the .pem file
+     * @param downloadFilePath The path to downloadd the file to. The parent directories must exist.
+     * @param onSuccess the success result callback
+     * @param onError the error result callback
+     */
+    void download(
     const std::string& url,
     const std::string& certFilePath,
     const std::string &downloadFilePath,

Tactility/Private/Tactility/json/Reader.h

@@ -46,7 +46,7 @@ public:
     bool readNumber(const char* key, double& output) const {
         const auto* child = cJSON_GetObjectItemCaseSensitive(root, key);
         if (!cJSON_IsNumber(child)) {
-            TT_LOG_E(TAG, "%s is not a string", key);
+            TT_LOG_E(TAG, "%s is not a number", key);
             return false;
         }
         output = cJSON_GetNumberValue(child);

Tactility/Source/PreferencesEsp.cpp

@@ -90,6 +90,8 @@ void Preferences::putBool(const std::string& key, bool value) {
     if (nvs_open(namespace_, NVS_READWRITE, &handle) == ESP_OK) {
         if (nvs_set_u8(handle, key.c_str(), (uint8_t)value) != ESP_OK) {
             TT_LOG_E(TAG, "Failed to write %s:%s", namespace_, key.c_str());
+        } else if (nvs_commit(handle) != ESP_OK) {
+            TT_LOG_E(TAG, "Failed to commit %s:%s", namespace_, key.c_str());
         }
         nvs_close(handle);
     } else {
@@ -102,6 +104,8 @@ void Preferences::putInt32(const std::string& key, int32_t value) {
     if (nvs_open(namespace_, NVS_READWRITE, &handle) == ESP_OK) {
         if (nvs_set_i32(handle, key.c_str(), value) != ESP_OK) {
             TT_LOG_E(TAG, "Failed to write %s:%s", namespace_, key.c_str());
+        } else if (nvs_commit(handle) != ESP_OK) {
+            TT_LOG_E(TAG, "Failed to commit %s:%s", namespace_, key.c_str());
         }
         nvs_close(handle);
     } else {
@@ -114,6 +118,8 @@ void Preferences::putInt64(const std::string& key, int64_t value) {
     if (nvs_open(namespace_, NVS_READWRITE, &handle) == ESP_OK) {
         if (nvs_set_i64(handle, key.c_str(), value) != ESP_OK) {
             TT_LOG_E(TAG, "Failed to write %s:%s", namespace_, key.c_str());
+        } else if (nvs_commit(handle) != ESP_OK) {
+            TT_LOG_E(TAG, "Failed to commit %s:%s", namespace_, key.c_str());
         }
         nvs_close(handle);
     } else {
@@ -125,6 +131,9 @@ void Preferences::putString(const std::string& key, const std::string& text) {
     nvs_handle_t handle;
     if (nvs_open(namespace_, NVS_READWRITE, &handle) == ESP_OK) {
         nvs_set_str(handle, key.c_str(), text.c_str());
+        if (nvs_commit(handle) != ESP_OK) {
+            TT_LOG_E(TAG, "Failed to commit %s:%s", namespace_, key.c_str());
+        }
         nvs_close(handle);
     } else {
         TT_LOG_E(TAG, "Failed to open namespace %s", namespace_);

Tactility/Source/app/apphub/AppHubApp.cpp

@@ -40,11 +40,7 @@ class AppHubApp final : public App {
         const auto* self = static_cast<AppHubApp*>(lv_event_get_user_data(e));
         auto* widget = lv_event_get_target_obj(e);
         const auto* user_data = lv_obj_get_user_data(widget);
-#ifdef ESP_PLATFORM
+        const intptr_t index = reinterpret_cast<intptr_t>(user_data);
-        const int index = reinterpret_cast<int>(user_data);
-#else
-        const long long index = reinterpret_cast<long long>(user_data);
-#endif
         self->mutex.lock();
         if (index < self->entries.size()) {
             apphubdetails::start(self->entries[index]);
@@ -112,7 +108,8 @@ class AppHubApp final : public App {
                 TT_LOG_I(TAG, "Adding %s", entry.appName.c_str());
                 const char* icon = findAppManifestById(entry.appId) != nullptr ? LV_SYMBOL_OK : nullptr;
                 auto* entry_button = lv_list_add_button(list, icon, entry.appName.c_str());
-                lv_obj_set_user_data(entry_button, reinterpret_cast<void*>(i));
+                auto int_as_voidptr = reinterpret_cast<void*>(i);
+                lv_obj_set_user_data(entry_button, int_as_voidptr);
                 lv_obj_add_event_cb(entry_button, onAppPressed, LV_EVENT_SHORT_CLICKED, this);
             }
         } else {

Tactility/Source/app/apphub/AppHubEntry.cpp

@@ -23,6 +23,11 @@ bool parseJson(const std::string& filePath, std::vector<AppHubEntry>& entries) {
     lock.lock();
 
     auto data = file::readString(filePath);
+    if (data == nullptr) {
+        TT_LOG_E(TAG, "Failed to read %s", filePath.c_str());
+        return false;
+    }
+
     auto data_ptr = reinterpret_cast<const char*>(data.get());
     auto* json = cJSON_Parse(data_ptr);
     if (json == nullptr) {

Tactility/Source/file/ObjectFileWriter.cpp

@@ -16,10 +16,9 @@ bool ObjectFileWriter::open() {
     }
 
     // Edit existing or create a new file
-    auto* mode = edit_existing ? "r+" : "w";
+    auto opening_file = std::unique_ptr<FILE, FileCloser>(std::fopen(filePath.c_str(), "wb"));
-    auto opening_file = std::unique_ptr<FILE, FileCloser>(std::fopen(filePath.c_str(), mode));
     if (opening_file == nullptr) {
-        TT_LOG_E(TAG, "Failed to open file %s in %s mode", filePath.c_str(), mode);
+        TT_LOG_E(TAG, "Failed to open file %s", filePath.c_str());
         return false;
     }
 

Tactility/Source/lvgl/Toolbar.cpp

@@ -223,6 +223,7 @@ lv_obj_t* toolbar_add_spinner_action(lv_obj_t* obj) {
 void toolbar_clear_actions(lv_obj_t* obj) {
     auto* toolbar = reinterpret_cast<Toolbar*>(obj);
     lv_obj_clean(toolbar->action_container);
+    toolbar->action_count = 0;
 }
 
 } // namespace

Tactility/Source/network/Http.cpp

@@ -24,6 +24,11 @@ void download(
     getMainDispatcher().dispatch([url, certFilePath, downloadFilePath, onSuccess, onError] {
         TT_LOG_I(TAG, "Loading certificate");
         auto certificate = file::readString(certFilePath);
+        if (certificate == nullptr) {
+            onError("Failed to read certificate");
+            return;
+        }
+
         auto certificate_length = strlen(reinterpret_cast<const char*>(certificate.get())) + 1;
 
         auto config = std::make_unique<esp_http_client_config_t>(esp_http_client_config_t {
@@ -40,35 +45,33 @@ void download(
         auto client = std::make_unique<EspHttpClient>();
         if (!client->init(std::move(config))) {
             onError("Failed to initialize client");
-            return -1;
+            return;
         }
 
         if (!client->open()) {
             onError("Failed to open connection");
-            return -1;
+            return;
         }
 
         if (!client->fetchHeaders()) {
             onError("Failed to get request headers");
-            return -1;
+            return;
         }
 
         if (!client->isStatusCodeOk()) {
             onError("Server response is not OK");
-            return -1;
+            return;
         }
 
         auto bytes_left = client->getContentLength();
 
         auto lock = file::getLock(downloadFilePath)->asScopedLock();
         lock.lock();
-        auto file_exists = file::isFile(downloadFilePath);
+        TT_LOG_I(TAG, "opening %s", downloadFilePath.c_str());
-        auto* file_mode = file_exists ? "r+" : "w";
+        auto* file = fopen(downloadFilePath.c_str(), "wb");
-        TT_LOG_I(TAG, "opening %s with mode %s", downloadFilePath.c_str(), file_mode);
-        auto* file = fopen(downloadFilePath.c_str(), file_mode);
         if (file == nullptr) {
             onError("Failed to open file");
-            return -1;
+            return;
         }
 
         TT_LOG_I(TAG, "Writing %d bytes to %s", bytes_left, downloadFilePath.c_str());
@@ -78,19 +81,18 @@ void download(
             if (data_read <= 0) {
                 fclose(file);
                 onError("Failed to read data");
-                return -1;
+                return;
             }
             bytes_left -= data_read;
             if (fwrite(buffer, 1, data_read, file) != data_read) {
                 fclose(file);
                 onError("Failed to write all bytes");
-                return -1;
+                return;
             }
         }
         fclose(file);
         TT_LOG_I(TAG, "Downloaded %s to %s", url.c_str(), downloadFilePath.c_str());
         onSuccess();
-        return 0;
     });
 #else
     getMainDispatcher().dispatch([onError] {

Tactility/Source/network/HttpServer.cpp

@@ -22,7 +22,7 @@ bool HttpServer::startInternal() {
         httpd_register_uri_handler(server, &handler);
     }
 
-    TT_LOG_I(TAG, "Started on port %d", config.server_port);
+    TT_LOG_I(TAG, "Started on port %lu", config.server_port);
 
     return true;
 }

Tactility/Source/network/Ntp.cpp

@@ -44,6 +44,8 @@ static void onTimeSynced(timeval* tv) {
 }
 
 void init() {
+    setTimeFromNvs();
+
     esp_sntp_config_t config = ESP_NETIF_SNTP_DEFAULT_CONFIG("time.cloudflare.com");
     config.sync_cb = onTimeSynced;
     esp_netif_sntp_init(&config);

TactilityCore/Source/Bundle.cpp

@@ -61,7 +61,7 @@ bool Bundle::optInt32(const std::string& key, int32_t& out) const {
 bool Bundle::optInt64(const std::string& key, int64_t& out) const {
     auto entry = this->entries.find(key);
     if (entry != std::end(this->entries) && entry->second.type == Type::Int64) {
-        out = entry->second.value_int32;
+        out = entry->second.value_int64;
         return true;
     } else {
         return false;