From d0d05c67ca2d3388094c581618d6b34965891b07 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sat, 25 Oct 2025 13:47:43 +0200 Subject: [PATCH] PR review fixes (#384) --- .../Include/Tactility/network/EspHttpClient.h | 8 ++++-- Tactility/Include/Tactility/network/Http.h | 13 ++++++++-- Tactility/Private/Tactility/json/Reader.h | 2 +- Tactility/Source/PreferencesEsp.cpp | 9 +++++++ Tactility/Source/app/apphub/AppHubApp.cpp | 9 +++---- Tactility/Source/app/apphub/AppHubEntry.cpp | 5 ++++ Tactility/Source/file/ObjectFileWriter.cpp | 5 ++-- Tactility/Source/lvgl/Toolbar.cpp | 1 + Tactility/Source/network/Http.cpp | 26 ++++++++++--------- Tactility/Source/network/HttpServer.cpp | 2 +- Tactility/Source/network/Ntp.cpp | 2 ++ TactilityCore/Source/Bundle.cpp | 2 +- 12 files changed, 56 insertions(+), 28 deletions(-) diff --git a/Tactility/Include/Tactility/network/EspHttpClient.h b/Tactility/Include/Tactility/network/EspHttpClient.h index 8642ab48..9cd26c9a 100644 --- a/Tactility/Include/Tactility/network/EspHttpClient.h +++ b/Tactility/Include/Tactility/network/EspHttpClient.h @@ -11,7 +11,7 @@ class EspHttpClient { static constexpr auto* TAG = "EspHttpClient"; std::unique_ptr 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; diff --git a/Tactility/Include/Tactility/network/Http.h b/Tactility/Include/Tactility/network/Http.h index c843f309..316d02e6 100644 --- a/Tactility/Include/Tactility/network/Http.h +++ b/Tactility/Include/Tactility/network/Http.h @@ -1,10 +1,19 @@ #pragma once #include +#include 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, diff --git a/Tactility/Private/Tactility/json/Reader.h b/Tactility/Private/Tactility/json/Reader.h index deb81f0f..12644a24 100644 --- a/Tactility/Private/Tactility/json/Reader.h +++ b/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); diff --git a/Tactility/Source/PreferencesEsp.cpp b/Tactility/Source/PreferencesEsp.cpp index 263e952a..86f749dd 100644 --- a/Tactility/Source/PreferencesEsp.cpp +++ b/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_); diff --git a/Tactility/Source/app/apphub/AppHubApp.cpp b/Tactility/Source/app/apphub/AppHubApp.cpp index 4cb0d51f..288b92c2 100644 --- a/Tactility/Source/app/apphub/AppHubApp.cpp +++ b/Tactility/Source/app/apphub/AppHubApp.cpp @@ -40,11 +40,7 @@ class AppHubApp final : public App { const auto* self = static_cast(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 int index = reinterpret_cast(user_data); -#else - const long long index = reinterpret_cast(user_data); -#endif + const intptr_t index = reinterpret_cast(user_data); 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(i)); + auto int_as_voidptr = reinterpret_cast(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 { diff --git a/Tactility/Source/app/apphub/AppHubEntry.cpp b/Tactility/Source/app/apphub/AppHubEntry.cpp index d0459462..68c6ada0 100644 --- a/Tactility/Source/app/apphub/AppHubEntry.cpp +++ b/Tactility/Source/app/apphub/AppHubEntry.cpp @@ -23,6 +23,11 @@ bool parseJson(const std::string& filePath, std::vector& 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(data.get()); auto* json = cJSON_Parse(data_ptr); if (json == nullptr) { diff --git a/Tactility/Source/file/ObjectFileWriter.cpp b/Tactility/Source/file/ObjectFileWriter.cpp index 8bc9fbeb..f90fc995 100644 --- a/Tactility/Source/file/ObjectFileWriter.cpp +++ b/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(std::fopen(filePath.c_str(), mode)); + auto opening_file = std::unique_ptr(std::fopen(filePath.c_str(), "wb")); 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; } diff --git a/Tactility/Source/lvgl/Toolbar.cpp b/Tactility/Source/lvgl/Toolbar.cpp index 7fe758b3..36c83bda 100644 --- a/Tactility/Source/lvgl/Toolbar.cpp +++ b/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(obj); lv_obj_clean(toolbar->action_container); + toolbar->action_count = 0; } } // namespace diff --git a/Tactility/Source/network/Http.cpp b/Tactility/Source/network/Http.cpp index ae4bde3c..5681025b 100644 --- a/Tactility/Source/network/Http.cpp +++ b/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(certificate.get())) + 1; auto config = std::make_unique(esp_http_client_config_t { @@ -40,35 +45,33 @@ void download( auto client = std::make_unique(); 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); - auto* file_mode = file_exists ? "r+" : "w"; - TT_LOG_I(TAG, "opening %s with mode %s", downloadFilePath.c_str(), file_mode); - auto* file = fopen(downloadFilePath.c_str(), file_mode); + TT_LOG_I(TAG, "opening %s", downloadFilePath.c_str()); + auto* file = fopen(downloadFilePath.c_str(), "wb"); 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] { diff --git a/Tactility/Source/network/HttpServer.cpp b/Tactility/Source/network/HttpServer.cpp index 65b2a164..ae47f1ea 100644 --- a/Tactility/Source/network/HttpServer.cpp +++ b/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; } diff --git a/Tactility/Source/network/Ntp.cpp b/Tactility/Source/network/Ntp.cpp index 940564e9..8ff265df 100644 --- a/Tactility/Source/network/Ntp.cpp +++ b/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); diff --git a/TactilityCore/Source/Bundle.cpp b/TactilityCore/Source/Bundle.cpp index e0819ba6..0295c76a 100644 --- a/TactilityCore/Source/Bundle.cpp +++ b/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;