Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a complete ESP32 firmware tree: build/CI and repo metadata, many new device drivers and managers (ADC, I2C, SPI, RTC, PortExpander), network stack and servers (HTTP, WebSocket, VISA), tasks (Lepton, GUI, Network, Devices), LVGL UI assets, settings/time/SD managers, and supporting build/config scripts and manifests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (4)
main/Application/Manager/Devices/SPI/spi.cpp (1)
97-133: The critical use-after-free bug has been resolved.The previously reported issue where the mutex was deleted before use has been fixed. The current implementation correctly:
- Takes the mutex (line 113)
- Performs
spi_bus_free(line 114)- Releases the mutex (line 120)
- Deletes the mutex only after all operations complete (lines 122-125)
main/Application/Manager/SD/sdManager.cpp (1)
102-112: ISR safety issue already flagged.The TODO on line 108 acknowledges this issue. Please refer to the previous review comment that provides a complete fix using FreeRTOS
xTimerResetFromISR()instead of the non-IRAM-safeesp_timer_*functions.main/Application/Manager/Devices/ADC/adc.cpp (1)
61-63: Redundant condition:_ADC_Calib_Doneis alwaysfalseat line 63.The variable is set to
falseon line 61, so the checkif (_ADC_Calib_Done == false)on line 63 is always true. Remove the condition or restructure the logic.🔎 Proposed fix
Error = ESP_OK; - _ADC_Calib_Done = false; - - if (_ADC_Calib_Done == false) { + { ESP_LOGD(TAG, "calibration scheme version is %s", "Curve Fitting");main/Application/Tasks/GUI/Export/screens/ui_Menu.c (1)
20-22: Potential null pointer dereference in gesture handling.
lv_indev_active()may return NULL if no input device is active. The code calls it twice and uses the result without null checking, which could crash.This issue was already flagged in a previous review. The suggested fix is to capture the result once and check for NULL before using it.
🟡 Minor comments (16)
main/Application/Manager/Devices/SPI/spi.cpp-154-166 (1)
154-166: Minor race condition in logging DeviceCount.Line 165 reads
_SPI_State[Host].DeviceCountafter the mutex has been released (line 163). Another thread could modify this value between the unlock and the log statement, resulting in an inaccurate log message.🔎 Proposed fix
+ uint32_t device_count; + xSemaphoreTake(_SPI_State[Host].Mutex, portMAX_DELAY); Error = spi_bus_add_device(Host, p_Dev_Config, p_Handle); if (Error != ESP_OK) { ESP_LOGE(TAG, "Failed to add device to SPI%d: %d!", Host + 1, Error); xSemaphoreGive(_SPI_State[Host].Mutex); return Error; } _SPI_State[Host].DeviceCount++; + device_count = _SPI_State[Host].DeviceCount; xSemaphoreGive(_SPI_State[Host].Mutex); - ESP_LOGI(TAG, "Device added to SPI%d (total: %d devices)", Host + 1, _SPI_State[Host].DeviceCount); + ESP_LOGI(TAG, "Device added to SPI%d (total: %d devices)", Host + 1, device_count);main/Application/Manager/SD/sdManager.cpp-386-397 (1)
386-397: Missing check for already-mounted state.The function checks
CardPresentandMountTaskHandle, but doesn't verify whether the card is already mounted (_SD_Manager_State.Card != NULL). CallingSDManager_Mount()when already mounted could cause issues.🔎 Proposed fix
esp_err_t SDManager_Mount(void) { if (_SD_Manager_State.CardPresent == false) { ESP_LOGW(TAG, "No SD card available!"); return ESP_ERR_NOT_FOUND; } + /* Check if already mounted */ + if (_SD_Manager_State.Card != NULL) { + ESP_LOGW(TAG, "SD card already mounted"); + return ESP_OK; + } + /* Prevent multiple mount tasks */ if (_SD_Manager_State.MountTaskHandle != NULL) { ESP_LOGW(TAG, "Mount task already running"); return ESP_ERR_INVALID_STATE; }main/Application/Manager/SD/sdManager.cpp-266-268 (1)
266-268: Incomplete state check: should also verify card is mounted.Checking
CardPresentonly confirms the physical presence detected via GPIO, but doesn't guarantee the filesystem is mounted. If the card is present butSDManager_Mount()failed or hasn't completed, file operations will fail with a confusing error.🔎 Proposed fix
- if (_SD_Manager_State.CardPresent == false) { + if (_SD_Manager_State.CardPresent == false || _SD_Manager_State.Card == NULL) { return ESP_ERR_INVALID_STATE; }main/Application/Tasks/Lepton/leptonTask.cpp-1-2 (1)
1-2: File header comment has incorrect extension.The comment says
leptonTask.cbut the file isleptonTask.cpp.🔎 Proposed fix
/* - * leptonTask.c + * leptonTask.cppmain/Application/Tasks/GUI/Private/guiHelper.cpp-308-311 (1)
308-311: Format string mismatch inESP_LOGW.The format string contains no format specifiers, but two extra arguments (
lv_obj_get_width(...)andlv_obj_get_height(...)) are passed. These arguments are evaluated but ignored.🔎 Proposed fix
- ESP_LOGW(TAG, "Thermal image not yet initialized, skipping spot update", - lv_obj_get_width(ui_Image_Thermal), lv_obj_get_height(ui_Image_Thermal)); + ESP_LOGW(TAG, "Thermal image not yet initialized (w=%d, h=%d), skipping spot update", + lv_obj_get_width(ui_Image_Thermal), lv_obj_get_height(ui_Image_Thermal));main/Application/Tasks/GUI/Private/guiHelper.cpp-236-244 (1)
236-244: Dead code: duplicate LVGL_TickTimer cleanup.Lines 236-238 already stop, delete, and NULL the timer. The block at lines 240-244 is unreachable since
LVGL_TickTimeris always NULL at that point.🔎 Proposed fix
esp_timer_stop(p_GUITask_State->LVGL_TickTimer); esp_timer_delete(p_GUITask_State->LVGL_TickTimer); p_GUITask_State->LVGL_TickTimer = NULL; - - if (p_GUITask_State->LVGL_TickTimer != NULL) { - esp_timer_stop(p_GUITask_State->LVGL_TickTimer); - esp_timer_delete(p_GUITask_State->LVGL_TickTimer); - p_GUITask_State->LVGL_TickTimer = NULL; - }main/Application/Manager/Settings/settingsManager.cpp-201-206 (1)
201-206: Settings version mismatch logs warning but continues with potentially incompatible data.When a version mismatch is detected, the code logs a warning and continues using the loaded settings. This could cause issues if the struct layout changed between versions. The TODO comment acknowledges this, but until migration is implemented, consider falling back to defaults for safety.
Do you want me to help implement a version migration framework or propose a safer fallback strategy?
main/Application/Manager/Network/Server/websocket_handler.cpp-687-711 (1)
687-711: Telemetry data access without mutex protection.
WebSocket_Handler_BroadcastTelemetryreads_WSHandler_State.ThermalFrame->temp_avgat line 688 without holding the frame's mutex, butWS_BroadcastTaskproperly acquiresThermalFrame->mutexbefore accessing frame data. This inconsistency could lead to torn reads if the thermal task is updating the frame concurrently.main/Application/Manager/Network/Server/websocket_handler.cpp-775-798 (1)
775-798: Task handle not cleared by the task itself, causing potential race.
WS_BroadcastTaskdeletes itself withvTaskDelete(NULL)at line 648, butWebSocket_Handler_StopTaskpolls_WSHandler_State.BroadcastTaskto check if the task has finished. Since the task never setsBroadcastTask = NULLbefore deleting itself, the check at line 790 will never see a NULL handle, and the loop will always run until timeout.🔎 Proposed fix
Have the task clear its handle before exiting:
static void WS_BroadcastTask(void *p_Param) { // ... existing code ... ESP_LOGI(TAG, "WebSocket broadcast task stopped"); + _WSHandler_State.BroadcastTask = NULL; vTaskDelete(NULL); }And update the stop function:
void WebSocket_Handler_StopTask(uint32_t Timeout_ms) { - uint32_t Start; - if (_WSHandler_State.BroadcastTask == NULL) { return; } ESP_LOGI(TAG, "Stopping WebSocket broadcast task..."); _WSHandler_State.TaskRunning = false; - /* Wait for task to finish (max 2 seconds) */ - Start = esp_timer_get_time() / 1000; + uint32_t Start = esp_timer_get_time() / 1000; while (_WSHandler_State.BroadcastTask != NULL && ((esp_timer_get_time() / 1000) - Start) < Timeout_ms) { vTaskDelay(10 / portTICK_PERIOD_MS); } - _WSHandler_State.BroadcastTask = NULL; + if (_WSHandler_State.BroadcastTask != NULL) { + ESP_LOGW(TAG, "Broadcast task did not exit gracefully, forcing cleanup"); + _WSHandler_State.BroadcastTask = NULL; + } ESP_LOGI(TAG, "WebSocket broadcast task stopped"); }Committable suggestion skipped: line range outside the PR's diff.
main/Application/Tasks/GUI/guiTask.cpp-481-497 (1)
481-497: Splash screen wait loop lacks timeout.The splash screen loop (lines 481-497) waits indefinitely for
LEPTON_CAMERA_READY. If the Lepton camera fails to initialize, the task will be stuck. The TODO comment on line 493 acknowledges this.Would you like me to implement a timeout mechanism for this loop?
main/Application/Tasks/Devices/devicesTask.cpp-117-117 (1)
117-117: Potential double deinitialization of DevicesManager.
DevicesManager_Deinit()is called both when the task loop exits (line 117) and again inDevicesTask_Deinit()(line 167). If the task exits normally and thenDevicesTask_Deinit()is called, this could cause issues ifDevicesManager_Deinitis not idempotent.🔎 Suggested fix - remove from task or add guard
Either remove the call from the task loop (letting
DevicesTask_Deinithandle cleanup), or ensureDevicesManager_Deinitis idempotent:static void Task_Devices(void *p_Parameters) { // ... loop code ... ESP_LOGD(TAG, "Devices task shutting down"); - DevicesManager_Deinit(); _DevicesTask_State.Running = false;Also applies to: 167-167
main/Application/Tasks/Network/networkTask.cpp-440-448 (1)
440-448: Task stop may leave Running flag in inconsistent state.If the task doesn't stop within the timeout,
_NetworkTask_State.Runningis set tofalseon line 446 even though the task may still be running. This could cause issues if the task later tries to access freed resources.main/Application/Tasks/GUI/guiTask.cpp-647-647 (1)
647-647: Minor typo: "recieved" should be "received".- /* Process the recieved system events */ + /* Process the received system events */main/Application/Tasks/Devices/devicesTask.cpp-145-149 (1)
145-149: Event group not cleaned up onDevicesManager_Initfailure.If
DevicesManager_Init()fails (line 146), the event group created on line 139 is not deleted before returning.🔎 Suggested fix
Error = DevicesManager_Init(); if (Error != ESP_OK) { ESP_LOGE(TAG, "Failed to initialize Devices Manager: %d!", Error); + vEventGroupDelete(_DevicesTask_State.EventGroup); + _DevicesTask_State.EventGroup = NULL; return Error; }main/Application/Manager/Network/Server/image_encoder.cpp-108-108 (1)
108-108: Stray semicolon.There's a lone semicolon on line 108 which appears to be a typo.
-;main/Application/Manager/Network/network_types.h-77-78 (1)
77-78: Incomplete documentation comment.The comment for
NETWORK_EVENT_CREDENTIALS_UPDATEDis incomplete: "Data is of ...". Please complete the documentation.🔎 Proposed fix
NETWORK_EVENT_CREDENTIALS_UPDATED, /**< New WiFi credentials have been set - Data is of ... */ + Data is of type Network_WiFi_Credentials_t */
🧹 Nitpick comments (36)
main/Application/Tasks/Network/networkTask.h (2)
27-28: Unused include:esp_event.his not referenced in this header.The header declares functions returning
esp_err_tand takingApp_Context_t*, but no event types are used here. Consider removing this include to reduce compilation dependencies.
35-46: Inconsistent documentation across function declarations.Only
Network_Task_Starthas a doxygen comment. For API consistency, consider adding brief documentation toNetwork_Task_Init,Network_Task_Deinit,Network_Task_Stop, andNetwork_Task_isRunningsimilar to the existing comment style.main/Application/Manager/Devices/ADC/adc.cpp (2)
27-30: Unused FreeRTOS includes.These FreeRTOS headers are included but no FreeRTOS primitives (tasks, semaphores, event groups) are used in this file. Consider removing them.
🔎 Proposed fix
-#include <freertos/FreeRTOS.h> -#include <freertos/task.h> -#include <freertos/event_groups.h> -#include <freertos/semphr.h> -
51-51: TAG should be declaredstaticfor internal linkage.Global
const char *TAGwithoutstatichas external linkage and may conflict with TAG definitions in other translation units.🔎 Proposed fix
-const char *TAG = "ADC"; +static const char *TAG = "ADC";main/Application/Manager/Settings/settings_types.h (1)
24-28: Consider adding explicit includes forbooland fixed-width integer types.The header uses
bool(lines 74, 90, 91, 104) and fixed-width types likeuint8_t,uint16_t,uint32_tbut only includesesp_event.h. While these may be transitively included, explicit includes improve portability and self-documentation.🔎 Proposed fix
#ifndef SETTINGS_TYPES_H_ #define SETTINGS_TYPES_H_ #include <esp_event.h> +#include <stdint.h> +#include <stdbool.h>main/Application/Manager/Network/Server/websocket_handler.cpp (1)
550-558: Potential data race onClientCountreads.
WebSocket_Handler_GetClientCountandWebSocket_Handler_HasClientsreadClientCountwithout mutex protection, while it's modified under mutex inWS_AddClient/WS_RemoveClient. On ESP32 (32-bit ARM), single-word reads are typically atomic, but for strict correctness and consistency, consider protecting these reads or documenting the relaxed guarantee.main/Application/Manager/Settings/settingsManager.cpp (3)
75-78: Empty string copy has no effect; consider documenting intent.Lines 76-77 use
strncpyto copy empty strings""to SSID and Password, butmemseton line 64 already zeroed the entire struct. This is redundant but harmless. If the intent is to explicitly show WiFi defaults are empty, a comment would clarify.
109-109:ESP_ERROR_CHECKonnvs_flash_initmay be too aggressive.If NVS is corrupted,
nvs_flash_initreturns an error, andESP_ERROR_CHECKwill abort the application. Consider handlingESP_ERR_NVS_NO_FREE_PAGESorESP_ERR_NVS_NEW_VERSION_FOUNDby erasing and reinitializing NVS, which is a common pattern for recovery.🔎 Proposed fix
- ESP_ERROR_CHECK(nvs_flash_init()); + esp_err_t err = nvs_flash_init(); + if (err == ESP_ERR_NVS_NO_FREE_PAGES || err == ESP_ERR_NVS_NEW_VERSION_FOUND) { + ESP_LOGW(TAG, "NVS partition issue, erasing and reinitializing..."); + ESP_ERROR_CHECK(nvs_flash_erase()); + err = nvs_flash_init(); + } + if (err != ESP_OK) { + ESP_LOGE(TAG, "Failed to initialize NVS: %d", err); + return err; + }
423-426:SettingsManager_HasPendingChangesreads without mutex.
_State.PendingChangesis read without mutex protection. While bool reads are typically atomic on ESP32, for consistency with other state accesses that use the mutex, consider adding protection or documenting this is intentional.main/Application/Tasks/GUI/Export/screens/ui_Main.c (1)
1-5: Auto-generated file - consider adding to.gitattributesfor merge handling.This file is generated by SquareLine Studio. Manual modifications will be lost on regeneration. Consider marking generated UI files in
.gitattributeswithmerge=oursor similar strategy to prevent merge conflicts on tool updates.main/main.cpp (2)
55-59: Queue creation failure should clean up before returning.If queue creation fails, the function returns without cleaning up the event loop created at line 53. While this is a fatal error scenario, proper cleanup ensures consistent state.
🔎 Proposed fix
_App_Context.Lepton_FrameEventQueue = xQueueCreate(1, sizeof(App_Lepton_FrameReady_t)); if (_App_Context.Lepton_FrameEventQueue == NULL) { ESP_LOGE(TAG, "Failed to create frame queue!"); + esp_event_loop_delete_default(); return; }
82-82: Commented-out code should be removed or explained.
SDManager_Init()is commented out without explanation. If SD card support is planned for later, consider adding a TODO comment or removing the line entirely.main/Application/Manager/Network/SNTP/sntp.cpp (3)
43-51: Missing initialization state tracking.
SNTP_Initdoesn't track whether SNTP is already initialized. Callingesp_sntp_init()twice without callingesp_sntp_stop()first may cause issues. Consider adding an initialization flag.🔎 Proposed fix
+static bool _sntp_initialized = false; + esp_err_t SNTP_Init(void) { + if (_sntp_initialized) { + ESP_LOGW(TAG, "SNTP already initialized"); + return ESP_OK; + } + esp_sntp_setoperatingmode(SNTP_OPMODE_POLL); esp_sntp_setservername(0, "pool.ntp.org"); esp_sntp_set_time_sync_notification_cb(on_SNTP_Time_Sync); esp_sntp_init(); + _sntp_initialized = true; return ESP_OK; } esp_err_t SNTP_Deinit(void) { + if (!_sntp_initialized) { + return ESP_OK; + } + esp_sntp_stop(); + _sntp_initialized = false; return ESP_OK; }
69-79: Off-by-one in retry logic - first attempt counts as retry.The
++Retryincrement happens before the first delay, meaning the first actual check at iteration start consumes one retry count. IfRetries=1, the loop body executes once but logs "1/1" on the first wait, which is slightly misleading. Consider using post-increment or restructuring.🔎 Proposed fix
- while ((TimeInfo.tm_year < (2016 - 1900)) && (++Retry < Retries)) { - ESP_LOGD(TAG, "Waiting for system time... (%d/%d)", Retry, Retries); + while ((TimeInfo.tm_year < (2016 - 1900)) && (Retry < Retries)) { + Retry++; + ESP_LOGD(TAG, "Waiting for system time... (%d/%d)", Retry, Retries); vTaskDelay(2000 / portTICK_PERIOD_MS); time(&Now); localtime_r(&Now, &TimeInfo); }
81-86: Redundant time retrieval after successful sync.Lines 83-84 retrieve time again after the loop succeeds, but
NowandTimeInfowere already populated in the loop. This is harmless but unnecessary.main/Application/Tasks/Devices/devicesTask.cpp (3)
97-110: Minor inconsistency:Nowrecalculated unnecessarily.Line 97 stores
esp_timer_get_time() / 1000inNow, but line 110 recalculates the same value instead of reusingNow. While functionally correct (slight time may have passed), this seems unintentional.🔎 Suggested fix
- _DevicesTask_State.LastBatteryUpdate = esp_timer_get_time() / 1000; + _DevicesTask_State.LastBatteryUpdate = Now;
40-41: Unused constantDEVICES_TASK_TIME_SYNCED.
DEVICES_TASK_TIME_SYNCED(BIT1) is defined but never used in this file. Consider removing it to avoid confusion, or implement the intended functionality.
194-202: Unused task parameterp_AppContext.
p_AppContextis passed toTask_Devicesbut the task never uses it. Either use it or remove the parameter and passNULLinstead.main/Application/Tasks/GUI/Export/ui_events.cpp (2)
35-39: Event posting uses zero timeout.
esp_event_postwith timeout0means the call won't block if the event queue is full - events could be silently dropped. Consider usingportMAX_DELAYfor reliability, or handle the return value.
41-194: Large block of commented-out scaffolding code.This commented-out menu implementation is substantial. If this is intended for future use, consider moving it to a separate file or tracking it in an issue. If not needed, remove it to reduce code clutter.
Would you like me to open an issue to track the menu implementation as a TODO?
main/Application/Tasks/Network/networkTask.cpp (2)
310-310: Duplicate assignment of AppContext.
_NetworkTask_State.AppContextis assigned at line 310 and again at line 363. The second assignment is redundant.🔎 Suggested fix
- _NetworkTask_State.AppContext = p_AppContext; _NetworkTask_State.State = NETWORK_STATE_IDLE; _NetworkTask_State.isInitialized = true;Also applies to: 363-363
42-42: Unused constantNETWORK_TASK_BROADCAST_FRAME.
NETWORK_TASK_BROADCAST_FRAME(BIT1) is defined but never used. Consider removing it or implementing the intended functionality.main/Application/Tasks/GUI/guiTask.cpp (1)
535-537: Consider using named constants for image dimensions.The values
160,120,240, and180appear multiple times throughout the file. Using named constants would improve maintainability and reduce magic numbers.main/Application/Tasks/GUI/Private/guiHelper.h (1)
42-55: Event bit definitions have inconsistent ordering.The event bits are defined out of numerical order (BIT0-BIT3, then BIT7-BIT13, then BIT4-BIT6). While functionally correct, this makes it harder to track which bits are used. Consider reordering for clarity.
main/Application/Manager/Network/Server/image_encoder.cpp (2)
170-179: Missing initialization check in Encode function.
Image_Encoder_Encodedoesn't verify_Encoder_State.isInitializedbefore proceeding. While it may work without initialization (using default quality 0 clamped to 1), this inconsistency with other functions could lead to unexpected behavior.🔎 Suggested fix
esp_err_t Image_Encoder_Encode(const Network_Thermal_Frame_t *p_Frame, Network_ImageFormat_t Format, Server_Palette_t Palette, Network_Encoded_Image_t *p_Encoded) { esp_err_t Error; if ((p_Frame == NULL) || (p_Encoded == NULL)) { return ESP_ERR_INVALID_ARG; } + + if (_Encoder_State.isInitialized == false) { + ESP_LOGE(TAG, "Encoder not initialized!"); + return ESP_ERR_INVALID_STATE; + }
240-252: SetQuality doesn't check initialization state.
Image_Encoder_SetQualityallows setting quality even when the encoder isn't initialized. While this may be intentional (pre-configure before init), it's inconsistent with theisInitializedpattern used elsewhere.main/Application/Manager/Network/Server/http_server.cpp (3)
192-196: Blockingesp_event_postin HTTP handler may stall server.Using
portMAX_DELAYin an HTTP request handler can block indefinitely if the event queue is full, stalling the HTTP server. Consider using a finite timeout.🔎 Proposed fix
/* Set timezone if provided */ if (cJSON_IsString(timezone) && (timezone->valuestring != NULL)) { - esp_event_post(NETWORK_EVENTS, NETWORK_EVENT_SET_TZ, (void *)timezone->valuestring, strlen(timezone->valuestring) + 1, - portMAX_DELAY); + esp_event_post(NETWORK_EVENTS, NETWORK_EVENT_SET_TZ, (void *)timezone->valuestring, strlen(timezone->valuestring) + 1, + 100 / portTICK_PERIOD_MS); }
427-432: Unreachable code afteresp_restart().The
return Error;statement on line 431 will never execute sinceesp_restart()does not return. This is dead code but harmless.🔎 Proposed fix
/* Reboot after response is sent */ vTaskDelay(1000 / portTICK_PERIOD_MS); esp_restart(); - - return Error; + + /* esp_restart() does not return, but the compiler may warn without this */ + return ESP_OK; }
571-579: Return values ofhttpd_register_uri_handlerare not checked.If any URI handler registration fails, the server may operate in an inconsistent state with some endpoints missing. Consider checking return values or at minimum logging failures.
🔎 Proposed fix
/* Register URI handlers */ - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Time); - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Image); - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Telemetry); - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Update); + esp_err_t reg_err; + reg_err = httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Time); + if (reg_err != ESP_OK) ESP_LOGW(TAG, "Failed to register /time: %s", esp_err_to_name(reg_err)); + reg_err = httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Image); + if (reg_err != ESP_OK) ESP_LOGW(TAG, "Failed to register /image: %s", esp_err_to_name(reg_err)); + reg_err = httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Telemetry); + if (reg_err != ESP_OK) ESP_LOGW(TAG, "Failed to register /telemetry: %s", esp_err_to_name(reg_err)); + reg_err = httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Update); + if (reg_err != ESP_OK) ESP_LOGW(TAG, "Failed to register /update: %s", esp_err_to_name(reg_err)); if (_HTTPServer_State.Config.EnableCORS) { - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Options); + reg_err = httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Options); + if (reg_err != ESP_OK) ESP_LOGW(TAG, "Failed to register OPTIONS: %s", esp_err_to_name(reg_err)); }main/Application/Manager/Network/networkManager.cpp (2)
96-187: Consider refactoring disconnect reason decoding to a lookup table.The large switch statement for decoding disconnect reasons is functional but verbose. A static lookup table would be more compact and maintainable.
359-373: Inconsistent error handling:ESP_ERROR_CHECKaborts without cleanup.Earlier in
NetworkManager_Init, errors are handled gracefully with cleanup. However,ESP_ERROR_CHECKon lines 359, 365, and 372 will abort the program on failure, potentially leaving resources allocated (netifs, event group). Consider consistent error handling throughout.🔎 Proposed fix for line 315 and 359-372
/* Initialize TCP/IP stack */ - ESP_ERROR_CHECK(esp_netif_init()); + Error = esp_netif_init(); + if (Error != ESP_OK) { + ESP_LOGE(TAG, "Failed to init netif: %d!", Error); + vEventGroupDelete(_Network_Manager_State.EventGroup); + _Network_Manager_State.EventGroup = NULL; + return Error; + } ... - ESP_ERROR_CHECK(esp_event_handler_instance_register(WIFI_EVENT, + Error = esp_event_handler_instance_register(WIFI_EVENT, ESP_EVENT_ANY_ID, &on_WiFi_Event, NULL, - NULL)); + NULL); + if (Error != ESP_OK) { + ESP_LOGE(TAG, "Failed to register WiFi event handler: %d!", Error); + // cleanup previously allocated resources... + return Error; + }main/Application/application.h (1)
40-54: Consider using typed enums for event identifiers.The anonymous
enumfor Lepton events provides no type safety. If mixed with other event enums, the compiler won't catch misuse. Consider using a typed enum.🔎 Proposed enhancement
-/** @brief Lepton camera event identifiers. - */ -enum { +/** @brief Lepton camera event identifiers. + */ +typedef enum { LEPTON_EVENT_CAMERA_READY, ... -}; +} Lepton_Event_t;main/Application/Manager/Network/Provisioning/provisioning.cpp (3)
330-348:Provisioning_isProvisionedinitializes and deinitializes manager on each call.While this follows ESP-IDF's API requirements, it's expensive if called frequently. Consider caching the result after first check if this is called in a hot path.
199-210: Timer creation failure is not logged.If
xTimerCreatereturnsNULL(memory exhaustion), the provisioning timeout feature will silently fail. Consider logging a warning.🔎 Proposed enhancement
_Provisioning_State.timeout_timer = xTimerCreate("prov_timeout", (_Provisioning_State.timeout_sec * 1000) / portTICK_PERIOD_MS, pdFALSE, NULL, on_Timeout_Timer_Handler); + if (_Provisioning_State.timeout_timer == NULL) { + ESP_LOGW(TAG, "Failed to create timeout timer - provisioning timeout disabled"); + } _Provisioning_State.isInitialized = true;
218-222: Consider documenting the purpose of the 200ms delay inProvisioning_Deinit.The
vTaskDelay(200 / portTICK_PERIOD_MS)appears to allow time forProvisioning_Stopto complete. A brief comment would clarify this intent.main/Application/Manager/Network/network_types.h (1)
199-207: Clarify ownership semantics forAPI_Keypointer inServer_Config_t.The
API_Keyis aconst char*pointer. It's unclear whether the configuration takes ownership of this string or expects the caller to maintain its lifetime. Consider documenting this or using a fixed-size array for self-contained storage.🔎 Proposed documentation or alternative
Option 1 - Document ownership:
typedef struct { uint16_t HTTP_Port; uint8_t MaxClients; uint16_t WSPingIntervalSec; bool EnableCORS; - const char *API_Key; + const char *API_Key; /**< API key for authentication (caller must ensure lifetime) */ } Server_Config_t;Option 2 - Use fixed buffer:
- const char *API_Key; + char API_Key[65]; /**< API key for authentication (empty string = disabled) */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
components/ESP32-Leptonmain/Application/Manager/Devices/ADC/adc.cppmain/Application/Manager/Devices/SPI/spi.cppmain/Application/Manager/Network/Provisioning/provisioning.cppmain/Application/Manager/Network/Provisioning/provisioning.hmain/Application/Manager/Network/SNTP/sntp.cppmain/Application/Manager/Network/Server/http_server.cppmain/Application/Manager/Network/Server/image_encoder.cppmain/Application/Manager/Network/Server/image_encoder.hmain/Application/Manager/Network/Server/websocket_handler.cppmain/Application/Manager/Network/networkManager.cppmain/Application/Manager/Network/networkManager.hmain/Application/Manager/Network/network_types.hmain/Application/Manager/SD/sdManager.cppmain/Application/Manager/Settings/settingsManager.cppmain/Application/Manager/Settings/settings_types.hmain/Application/Tasks/Devices/devicesTask.cppmain/Application/Tasks/GUI/Export/project.infomain/Application/Tasks/GUI/Export/screens/ui_Info.cmain/Application/Tasks/GUI/Export/screens/ui_Info.hmain/Application/Tasks/GUI/Export/screens/ui_Main.cmain/Application/Tasks/GUI/Export/screens/ui_Main.hmain/Application/Tasks/GUI/Export/screens/ui_Menu.cmain/Application/Tasks/GUI/Export/screens/ui_Menu.hmain/Application/Tasks/GUI/Export/ui_events.cppmain/Application/Tasks/GUI/Private/guiHelper.cppmain/Application/Tasks/GUI/Private/guiHelper.hmain/Application/Tasks/GUI/guiTask.cppmain/Application/Tasks/Lepton/leptonTask.cppmain/Application/Tasks/Network/networkTask.cppmain/Application/Tasks/Network/networkTask.hmain/Application/application.hmain/main.cppui/PyroVision.sllui/PyroVision.spjui/project.info
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/project.info
- ui/PyroVision.sll
- main/Application/Tasks/GUI/Export/project.info
- main/Application/Manager/Network/Provisioning/provisioning.h
- main/Application/Tasks/GUI/Export/screens/ui_Info.c
- components/ESP32-Lepton
🧰 Additional context used
🧬 Code graph analysis (15)
main/Application/Manager/Network/networkManager.h (1)
main/Application/Manager/Network/networkManager.cpp (28)
NetworkManager_Init(292-379)NetworkManager_Init(292-292)NetworkManager_Deinit(381-409)NetworkManager_Deinit(381-381)NetworkManager_StartSTA(411-442)NetworkManager_StartSTA(411-411)NetworkManager_Stop(452-465)NetworkManager_Stop(452-452)NetworkManager_ConnectWiFi(467-504)NetworkManager_ConnectWiFi(467-467)NetworkManager_DisconnectWiFi(506-515)NetworkManager_DisconnectWiFi(506-506)NetworkManager_isConnected(517-520)NetworkManager_isConnected(517-517)NetworkManager_GetState(522-525)NetworkManager_GetState(522-522)NetworkManager_GetIP(527-536)NetworkManager_GetIP(527-527)NetworkManager_GetRSSI(538-551)NetworkManager_GetRSSI(538-538)NetworkManager_GetMAC(553-560)NetworkManager_GetMAC(553-553)NetworkManager_SetCredentials(562-581)NetworkManager_SetCredentials(562-562)NetworkManager_GetConnectedStations(583-592)NetworkManager_GetConnectedStations(583-583)NetworkManager_StartServer(444-450)NetworkManager_StartServer(444-444)
main/Application/Manager/Network/Server/image_encoder.h (1)
main/Application/Manager/Network/Server/image_encoder.cpp (10)
Image_Encoder_Init(136-157)Image_Encoder_Init(136-136)Image_Encoder_Deinit(159-168)Image_Encoder_Deinit(159-159)Image_Encoder_Encode(170-224)Image_Encoder_Encode(170-173)Image_Encoder_Free(226-238)Image_Encoder_Free(226-226)Image_Encoder_SetQuality(240-252)Image_Encoder_SetQuality(240-240)
main/Application/Tasks/Devices/devicesTask.cpp (1)
main/Application/Tasks/Lepton/leptonTask.cpp (2)
on_GUI_Event_Handler(71-117)on_GUI_Event_Handler(71-71)
main/Application/Manager/Network/Server/http_server.cpp (1)
main/Application/Manager/Network/Server/image_encoder.cpp (4)
Image_Encoder_Encode(170-224)Image_Encoder_Encode(170-173)Image_Encoder_Free(226-238)Image_Encoder_Free(226-226)
main/Application/Manager/Network/networkManager.cpp (2)
main/Application/Manager/Network/Server/server.h (2)
Server_Init(35-58)Server_Start(72-95)main/Application/Manager/Network/SNTP/sntp.cpp (2)
SNTP_Deinit(53-57)SNTP_Deinit(53-53)
main/Application/Tasks/Lepton/leptonTask.cpp (1)
main/Application/Tasks/Devices/devicesTask.cpp (2)
on_GUI_Event_Handler(66-69)on_GUI_Event_Handler(66-66)
main/Application/Manager/Network/Server/websocket_handler.cpp (1)
main/Application/Manager/Network/Server/image_encoder.cpp (4)
Image_Encoder_Encode(170-224)Image_Encoder_Encode(170-173)Image_Encoder_Free(226-238)Image_Encoder_Free(226-226)
main/Application/Tasks/GUI/guiTask.cpp (3)
main/Application/Manager/Network/Server/server.h (2)
Server_SetThermalFrame(119-123)Server_isRunning(111-114)main/Application/Tasks/GUI/Private/guiHelper.cpp (4)
GUI_Helper_Init(104-221)GUI_Helper_Init(104-104)GUI_Helper_Deinit(223-284)GUI_Helper_Deinit(223-223)main/Application/Tasks/GUI/Export/ui.c (2)
ui_init(27-39)ui_destroy(41-47)
main/Application/Tasks/GUI/Export/screens/ui_Main.c (2)
main/Application/Tasks/GUI/Export/ui_events.cpp (4)
ScreenMainLoaded(12-33)ScreenMainLoaded(12-12)ButtonMainWiFiClicked(196-199)ButtonMainWiFiClicked(196-196)main/Application/Tasks/GUI/Export/ui_helpers.c (1)
_ui_screen_change(52-58)
main/Application/Tasks/GUI/Private/guiHelper.h (1)
main/Application/Tasks/GUI/Private/guiHelper.cpp (12)
GUI_Helper_Init(104-221)GUI_Helper_Init(104-104)GUI_Helper_Deinit(223-284)GUI_Helper_Deinit(223-223)GUI_Helper_Timer_ClockUpdate(286-301)GUI_Helper_Timer_ClockUpdate(286-286)GUI_Helper_Timer_SpotUpdate(303-326)GUI_Helper_Timer_SpotUpdate(303-303)GUI_Helper_Timer_SpotmeterUpdate(328-331)GUI_Helper_Timer_SpotmeterUpdate(328-328)GUI_Helper_Timer_SceneStatisticsUpdate(333-336)GUI_Helper_Timer_SceneStatisticsUpdate(333-333)
main/Application/Tasks/Network/networkTask.h (1)
main/Application/Tasks/Network/networkTask.cpp (10)
Network_Task_Init(297-370)Network_Task_Init(297-297)Network_Task_Deinit(372-392)Network_Task_Deinit(372-372)Network_Task_Start(394-427)Network_Task_Start(394-394)Network_Task_Stop(429-449)Network_Task_Stop(429-429)Network_Task_isRunning(451-454)Network_Task_isRunning(451-451)
main/Application/Tasks/GUI/Export/screens/ui_Info.h (1)
main/Application/Tasks/GUI/Export/screens/ui_Info.c (4)
ui_Info_screen_init(78-753)ui_Info_screen_destroy(755-806)ui_event_Info(54-65)ui_event_Button_Info_Back(67-74)
main/Application/Tasks/GUI/Private/guiHelper.cpp (2)
main/Application/Manager/Network/Server/server.h (1)
Server_isRunning(111-114)main/Application/Manager/Network/Server/websocket_handler.cpp (2)
WebSocket_Handler_BroadcastTelemetry(670-714)WebSocket_Handler_BroadcastTelemetry(670-670)
main/Application/Tasks/GUI/Export/screens/ui_Menu.c (3)
main/Application/Tasks/GUI/Export/ui_helpers.c (1)
_ui_screen_change(52-58)main/Application/Tasks/GUI/Export/screens/ui_Main.c (1)
ui_Main_screen_init(77-428)main/Application/Tasks/GUI/Export/ui_events.cpp (2)
ScreenMenuLoaded(97-194)ScreenMenuLoaded(97-97)
main/Application/Tasks/GUI/Export/screens/ui_Main.h (1)
main/Application/Tasks/GUI/Export/screens/ui_Main.c (6)
ui_Main_screen_init(77-428)ui_Main_screen_destroy(430-466)ui_event_Main(39-46)ui_event_Button_Main_WiFi(48-55)ui_event_Button_Main_Info(57-64)ui_event_Button_Main_Menu(66-73)
🪛 Clang (14.0.6)
main/Application/Tasks/GUI/Export/screens/ui_Info.h
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 23-23: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 24-24: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 25-25: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 26-26: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 27-27: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 28-28: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 29-29: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 30-30: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 31-31: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 32-32: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 33-33: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 34-34: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
main/Application/Tasks/GUI/Export/screens/ui_Main.h
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 23-23: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 24-24: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 25-25: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 26-26: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 27-27: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 28-28: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 29-29: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 30-30: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 31-31: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 32-32: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 33-33: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 34-34: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
main/Application/Tasks/GUI/Export/screens/ui_Menu.h
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 23-23: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 24-24: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
🪛 Cppcheck (2.19.0)
main/Application/Manager/Network/networkManager.cpp
[error] 232-232: There is an unknown macro here somewhere. Configuration is required. If MACSTR is a macro then please configure it.
(unknownMacro)
main/Application/Tasks/GUI/Private/guiHelper.cpp
[error] 216-216: Array 'p_GUITask_State->UpdateTimer[3]' accessed at index 3, which is out of bounds.
(arrayIndexOutOfBounds)
🔇 Additional comments (37)
main/Application/Manager/Devices/SPI/spi.cpp (2)
50-91: LGTM - Initialization is well-structured.The function provides comprehensive validation, proper resource cleanup on failure (mutex deletion if bus init fails), and clear logging.
200-207: LGTM - Status check is straightforward and correct.The validation and return logic are appropriate for this query function.
main/Application/Manager/SD/sdManager.cpp (5)
46-56: LGTM!Good use of preprocessor conditionals with a compile-time error if no SPI host is defined, ensuring misconfiguration is caught early.
65-77: LGTM!The state structure is well-organized. Good use of
volatileforCardPresentsince it's accessed from ISR context.
155-169: Previously flagged issue has been addressed.The
Errorvariable is now properly declared at line 156. The GPIO configuration and error handling look correct.
254-257: LGTM!Simple accessor function for the volatile card presence state.
419-441: LGTM!The unmount function properly checks for null card state, performs cleanup, and clears the card pointer.
main/Application/Manager/Network/Server/image_encoder.h (1)
31-61: LGTM!The API is well-documented with clear doxygen comments for all functions. The header provides a clean interface for thermal frame encoding with proper lifecycle management (Init/Deinit), encoding operations, and quality control.
main/Application/Manager/Settings/settings_types.h (1)
54-105: LGTM: Well-designed settings structures with version field and reserved space.The structure design follows good embedded practices:
- Version field enables future migrations
- Reserved bytes in each sub-structure allow backward-compatible extensions
- Clear separation of settings by domain (Lepton, WiFi, Display, System)
main/Application/Tasks/Lepton/leptonTask.cpp (2)
296-303: Verify condition logic for RAW14 format check.The condition checks if frame dimensions are zero AND format is NOT RAW14. This seems inverted - typically you'd want to skip processing if dimensions are invalid OR if the format doesn't support pixel temperature extraction. Consider if this should be:
if ((_LeptonTask_State.RawFrame.Width == 0) || (_LeptonTask_State.RawFrame.Height == 0) || (VideoFormat != LEPTON_FORMAT_RAW14)) {
353-358: Scene statistics values assigned as raw intensities, not temperatures.Unlike spotmeter handling (lines 336-338) which converts from Kelvin to Celsius, scene statistics assigns
MinIntensity,MaxIntensity, andMeanIntensitydirectly without conversion. Verify this is intentional - the log message at line 357 suggests these should be temperatures in °C.main/Application/Manager/Network/Server/websocket_handler.cpp (1)
140-169: LGTM - JSON message handling is well-structured.The
WS_SendJSONfunction properly creates a JSON object, handles memory allocation failures, and cleans up resources. UsingcJSON_AddItemReferenceToObjectcorrectly avoids double-free of the data parameter.main/Application/Manager/Settings/settingsManager.cpp (2)
359-398: LGTM - Reset to defaults implementation is thorough.
SettingsManager_ResetToDefaultsproperly erases NVS key, commits, reinitializes defaults, and saves. Error handling is appropriate with early returns on failure.
284-285: No action required — esp_event_post safely copies event data.The concern about posting events with pointers to caller's data is not an issue. The
esp_event_postfunction internally copies the event data (allocates and manages its own copy), ensuring the event handler receives valid data even if the caller reuses or frees the original memory after posting.main/Application/Tasks/GUI/Export/screens/ui_Main.c (1)
430-466: LGTM - Screen cleanup properly nullifies all pointers.
ui_Main_screen_destroycorrectly deletes the root object (which recursively deletes children in LVGL) and nullifies all global pointers to prevent dangling references.main/Application/Tasks/GUI/Export/screens/ui_Main.h (2)
6-56: Static analysis errors are false positives - header relies on prior LVGL inclusion.The Clang errors about unknown types
lv_obj_tandlv_event_toccur because this header is designed to be included viaui.h(as seen inui_Main.cline 6:#include "../ui.h"), which includes LVGL headers first. This is a common pattern for generated UI code and works correctly in the actual build.
1-56: LGTM - Generated header structure is correct.The header properly uses include guards, extern "C" for C++ compatibility, and declares all public symbols that match the implementation in
ui_Main.c.main/Application/Tasks/GUI/Export/screens/ui_Info.h (1)
1-69: LGTM - Consistent with other generated screen headers.Same pattern as
ui_Main.hwith proper include guards and extern "C" wrapping. Static analysis errors are false positives due to the header being included viaui.h.main/main.cpp (2)
84-94: No cleanup on partial initialization failure.If
GUI_Task_Initsucceeds butLepton_Task_Initfails,ESP_ERROR_CHECKwill abort without cleaning up the GUI task. Similarly for subsequent initializations. Consider implementing proper cleanup on failure, or document thatESP_ERROR_CHECKabort is the intended behavior for initialization failures.Is aborting on initialization failure the intended behavior, or should there be graceful degradation/cleanup?
96-97: LGTM - Task self-deletion is appropriate here.Using
vTaskDelete(NULL)after spawning all application tasks is the correct pattern for ESP-IDF applications. The comment correctly notes the watchdog consideration.main/Application/Manager/Network/SNTP/sntp.cpp (1)
36-41: LGTM - Event-based sync notification is well-designed.The callback properly posts an event with the synchronized time, allowing other components (like TimeManager) to react to SNTP sync completion.
main/Application/Tasks/GUI/Export/ui_events.cpp (1)
12-33: LGTM - Clean UI initialization.
ScreenMainLoadedcorrectly sets up firmware version text and assigns icons/glyphs to UI elements. The use of Unicode escape sequences for FontAwesome icons is appropriate for the font configuration.main/Application/Tasks/Network/networkTask.cpp (1)
138-144: Provisioning_Stop called from event handler context.The comment on line 141 mentions "Stop provisioning in task context (not timer context)", but this is still in the event handler. If events are dispatched from a timer or ISR context, calling
Provisioning_Stop()here could be problematic. Consider setting an event bit and handling the stop in the task loop instead.main/Application/Tasks/GUI/Export/screens/ui_Menu.c (2)
40-119: LGTM - Standard SquareLine Studio generated screen initialization.The screen initialization follows the expected LVGL patterns with proper hierarchy setup, styling, and event callback registration.
121-134: LGTM - Proper cleanup in screen destroy.The destroy function correctly checks for null before deleting and resets all pointers to NULL, preventing use-after-free issues.
main/Application/Tasks/GUI/guiTask.cpp (2)
804-824: LGTM - Robust buffer allocation with proper cleanup.The buffer allocation sequence correctly handles failures by freeing previously allocated buffers before returning error. Good defensive coding.
613-644: LGTM - Thread-safe network frame update.The network frame update properly uses mutex protection with non-blocking acquire (
xSemaphoreTake(..., 0)), which is appropriate for real-time thermal streaming where dropping a frame is better than blocking.main/Application/Tasks/GUI/Private/guiHelper.h (1)
57-100: LGTM - Well-structured state management.The
GUI_Task_State_tstructure cleanly aggregates all GUI-related state, handles, buffers, and optional debug elements with conditional compilation.main/Application/Manager/Network/Server/image_encoder.cpp (2)
136-157: LGTM - Proper initialization with quality clamping.Quality bounds are correctly enforced during initialization, and the function is idempotent (returns OK if already initialized).
197-223: LGTM - Memory management in encoding paths is correct.The JPEG path allocates its own output buffer and frees
rgb_buffer. The RAW path transfers ownership ofrgb_bufferto the caller and returns early. The PNG fallback properly warns and uses RAW format.main/Application/Manager/Network/Server/http_server.cpp (1)
253-291: LGTM - Frame locking and image encoding are properly handled.The mutex acquisition with timeout, frame encoding, mutex release, and cleanup via
Image_Encoder_Freeare correctly sequenced. Error paths are handled appropriately.main/Application/Manager/Network/networkManager.cpp (2)
467-504: LGTM -NetworkManager_ConnectWiFiflow is correct.The function properly updates credentials, restarts WiFi via
Stop/StartSTA(which clears event bits), and waits for connection result with appropriate timeout handling.
232-234: Static analysis false positive:MACSTRis a valid ESP-IDF macro.The Cppcheck warning about
MACSTRbeing an unknown macro is a false positive.MACSTRandMAC2STRare defined inesp_mac.h(included on line 28) and are standard ESP-IDF macros for formatting MAC addresses.main/Application/Manager/Network/networkManager.h (1)
36-111: LGTM - Header declarations are well-documented and consistent with implementation.The function declarations match the implementations in
networkManager.cpp. Documentation is clear and follows a consistent style.main/Application/application.h (1)
127-131: Union aliasingAverageandMeanis intentional and clear.The union provides semantic flexibility for callers to use either
AverageorMeaninterchangeably, which is appropriate since they represent the same value in this context.main/Application/Manager/Network/Provisioning/provisioning.cpp (1)
150-170: LGTM - Custom endpoint follows ESP provisioning API contract.The allocated response buffer will be freed by the ESP provisioning framework after the response is sent. This is the expected usage pattern.
main/Application/Manager/Network/network_types.h (1)
134-145: LGTM -Network_Thermal_Frame_tis well-designed for thread-safe frame sharing.The structure includes a mutex for synchronization, timestamp for frame identification, and all necessary metadata. This aligns well with usage in
HTTP_Handler_Image.
| if (_Network_Manager_State.RetryCount < _Network_Manager_State.STA_Config->MaxRetries) { | ||
| ESP_LOGD(TAG, "Retry %d/%d", _Network_Manager_State.RetryCount++, _Network_Manager_State.STA_Config->MaxRetries); | ||
|
|
||
| vTaskDelay(_Network_Manager_State.STA_Config->RetryInterval / portTICK_PERIOD_MS); | ||
| esp_wifi_connect(); | ||
| _Network_Manager_State.State = NETWORK_STATE_CONNECTING; |
There was a problem hiding this comment.
Blocking delay in event handler can stall the event loop.
Calling vTaskDelay() inside the WiFi event handler blocks the system event task, potentially causing other events to be delayed or missed. Consider using a timer or posting to a separate task for retry logic.
🔎 Proposed alternative approach
Instead of blocking in the event handler, use a FreeRTOS timer for delayed reconnection:
// In state structure, add:
TimerHandle_t ReconnectTimer;
// Timer callback:
static void on_Reconnect_Timer(TimerHandle_t timer) {
esp_wifi_connect();
}
// In disconnect handler, replace vTaskDelay with:
if (_Network_Manager_State.RetryCount < _Network_Manager_State.STA_Config->MaxRetries) {
_Network_Manager_State.RetryCount++;
ESP_LOGD(TAG, "Retry %d/%d", _Network_Manager_State.RetryCount, _Network_Manager_State.STA_Config->MaxRetries);
xTimerChangePeriod(_Network_Manager_State.ReconnectTimer,
_Network_Manager_State.STA_Config->RetryInterval / portTICK_PERIOD_MS, 0);
xTimerStart(_Network_Manager_State.ReconnectTimer, 0);
_Network_Manager_State.State = NETWORK_STATE_CONNECTING;
}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (21)
main/Application/Manager/SD/sdManager.cpp (4)
102-112: ISR safety issue already flagged in previous review.The TODO at line 108 acknowledges this issue. The
esp_timer_stop()andesp_timer_start_once()calls are not IRAM-safe and should be replaced withxTimerResetFromISR()as suggested in the previous review.
239-243: Race condition with fixed delay already flagged in previous review.The arbitrary 100ms delay doesn't guarantee
SDManager_MountTaskcompletion. Use proper task synchronization as suggested previously.
365-376: WDT timeout risk withportMAX_DELAYalready flagged in previous review.Use a bounded timeout for
esp_event_post()as previously suggested to prevent watchdog timeouts.
419-441: No guard against unmounting while mount task is still running.If
SDManager_Unmount()is called whileSDManager_MountTaskis actively mounting, this could cause undefined behavior or filesystem corruption. Consider checkingMountTaskHandlebefore unmounting.🔎 Proposed fix
esp_err_t SDManager_Unmount(void) { esp_err_t Error; + /* Don't unmount while mount is in progress */ + if (_SD_Manager_State.MountTaskHandle != NULL) { + ESP_LOGW(TAG, "Cannot unmount: mount task in progress"); + return ESP_ERR_INVALID_STATE; + } + if (_SD_Manager_State.Card == NULL) { return ESP_OK; }main/Application/Manager/Devices/ADC/adc.cpp (1)
60-76: Redundant condition:_ADC_Calib_Doneis always false at check.Line 61 sets
_ADC_Calib_Done = false, and line 63 immediately checksif (_ADC_Calib_Done == false). This condition is always true, making the check unnecessary.🔎 Proposed fix
Error = ESP_OK; - _ADC_Calib_Done = false; - if (_ADC_Calib_Done == false) { - ESP_LOGD(TAG, "calibration scheme version is %s", "Curve Fitting"); - adc_cali_curve_fitting_config_t CaliConfig = { - .unit_id = _ADC_Init_Config.unit_id, - .chan = ADC_CHANNEL_0, - .atten = config.atten, - .bitwidth = ADC_BITWIDTH_DEFAULT, - }; + ESP_LOGD(TAG, "calibration scheme version is %s", "Curve Fitting"); + adc_cali_curve_fitting_config_t CaliConfig = { + .unit_id = _ADC_Init_Config.unit_id, + .chan = ADC_CHANNEL_0, + .atten = config.atten, + .bitwidth = ADC_BITWIDTH_DEFAULT, + }; - Error = adc_cali_create_scheme_curve_fitting(&CaliConfig, &_ADC_Calib_Handle); - if (Error == ESP_OK) { - _ADC_Calib_Done = true; - } + Error = adc_cali_create_scheme_curve_fitting(&CaliConfig, &_ADC_Calib_Handle); + if (Error == ESP_OK) { + _ADC_Calib_Done = true; }main/Application/Tasks/GUI/Private/guiHelper.cpp (1)
212-216: Array index out of bounds:UpdateTimer[3]exceeds declared array size of 3.The
UpdateTimerarray is declared with size 3 in the header, allowing only indices 0–2. Writing to index 3 causes a buffer overflow. Increase the array size to 4 inguiHelper.h.main/Application/Manager/Devices/PortExpander/portexpander.cpp (1)
264-277: Inverted logic inEnableCameraandEnableBatteryVoltage.Lines 160-162 document that camera power is active-high (
(0x01 << PIN_CAMERA)enables it). However,PortExpander_EnableCamera(true)writes!Enable(LOW), which disables the camera. The same issue affectsPortExpander_EnableBatteryVoltage. Note thatPortExpander_EnableLEDcorrectly usesEnablewithout negation.🔎 Proposed fix
esp_err_t PortExpander_EnableCamera(bool Enable) { - return PortExpander_SetPinLevel(PORT_0, (0x01 << PIN_CAMERA), (!Enable << PIN_CAMERA)); + return PortExpander_SetPinLevel(PORT_0, (0x01 << PIN_CAMERA), (Enable << PIN_CAMERA)); } esp_err_t PortExpander_EnableLED(bool Enable) { return PortExpander_SetPinLevel(PORT_0, (0x01 << PIN_LED), (Enable << PIN_LED)); } esp_err_t PortExpander_EnableBatteryVoltage(bool Enable) { - return PortExpander_SetPinLevel(PORT_0, (0x01 << PIN_BATTERY_VOLTAGE_ENABLE), (!Enable << PIN_BATTERY_VOLTAGE_ENABLE)); + return PortExpander_SetPinLevel(PORT_0, (0x01 << PIN_BATTERY_VOLTAGE_ENABLE), (Enable << PIN_BATTERY_VOLTAGE_ENABLE)); }main/Application/Tasks/GUI/Private/guiHelper.h (1)
77-77: Buffer overflow — UpdateTimer array sized for 3 but implementation uses 4 timers.This issue has already been flagged:
UpdateTimeris declared with size[3]but the implementation in guiHelper.cpp assigns four timers at indices 0–3 (ClockUpdate, SpotUpdate, SpotmeterUpdate, SceneStatisticsUpdate), causing an out-of-bounds write at index [3].main/Application/Tasks/GUI/Export/ui.c (1)
41-47: Memory leak concern already flagged.The missing cleanup of
ui____initial_actions0inui_destroy()was previously identified. This object is created with a NULL parent at line 37 and requires explicit deletion to prevent a memory leak.main/Application/Tasks/GUI/Export/screens/ui_Menu.h (1)
6-24: Missing LVGL header include causes compilation errors.This issue was previously identified - the header uses
lv_event_tandlv_obj_ttypes but doesn't includelvgl.h, causing compilation errors.main/Application/Tasks/GUI/guiTask.cpp (1)
959-971: Task forcefully deleted without graceful shutdown.This issue was previously identified.
vTaskDelete()is called directly, bypassing the task's cleanup code includingesp_task_wdt_delete(NULL). This can leave the watchdog thinking the task is still registered and skip proper resource cleanup.main/Application/Tasks/Network/networkTask.cpp (1)
93-100: Potential use-after-free: storing pointer to event data.This issue was previously identified. Line 96 stores a pointer to event data (
p_Data) which may become invalid after the event handler returns. Line 271 later usesmemcpywith this potentially dangling pointer.main/Application/Tasks/GUI/Export/screens/ui_Menu.c (1)
16-27: Potential null pointer dereference withlv_indev_active().This issue was previously identified.
lv_indev_active()is called twice (lines 20-21) and may return NULL if no input device is active. Passing NULL tolv_indev_get_gesture_dir()andlv_indev_wait_release()could cause a crash.main/Application/Manager/Network/Server/http_server.cpp (3)
115-127: Missing null check forcJSON_CreateObject.This issue was previously identified. If
cJSON_CreateObject()returns NULL (memory allocation failure), the subsequentcJSON_AddStringToObjectandcJSON_AddNumberToObjectcalls will dereference a null pointer.
305-335: Missing null check forcJSON_CreateObject.This issue was previously identified. If
cJSON_CreateObject()fails, subsequent operations will dereference a null pointer.
344-370: EnableCONFIG_SECURE_BOOT_V2for production OTA updates.This issue was previously identified. The OTA handler has no signature verification without
CONFIG_SECURE_BOOT=yenabled in sdkconfig, allowing unsigned firmware to be flashed.main/Application/Manager/Network/Server/websocket_handler.cpp (2)
68-77: Missing mutex protection inWS_FindClient.This issue was previously raised -
WS_FindClientaccesses_WSHandler_State.Clientswithout holdingClientsMutex, creating a race condition withWS_AddClient/WS_RemoveClient.
602-635: Releasing mutex during iteration can cause use-after-free.This issue was previously raised - copying the client FD and re-validating after re-acquiring the mutex is needed to prevent operating on stale/reused client slots.
main/Application/Manager/Network/networkManager.cpp (2)
201-206: Blocking delay in event handler can stall the event loop.This issue was previously raised - using
vTaskDelay()inside the WiFi event handler blocks the system event task. Consider using a timer or posting to a separate task for retry logic.
444-450:NetworkManager_StartServerignores return values fromServer_InitandServer_Start.This issue was previously raised - both functions can fail but their errors are not propagated.
main/Application/Manager/Network/Provisioning/provisioning.cpp (1)
114-116: Password logged in plaintext is a security/compliance risk.This issue was previously raised - logging WiFi passwords poses a security risk if logs are captured or persisted.
🟡 Minor comments (8)
main/Application/Tasks/Devices/devicesTask.cpp-217-233 (1)
217-233: Fix inconsistent state on stop timeout.If the task fails to stop within the timeout (line 228–230), the function returns
ESP_ERR_TIMEOUT, but line 233 still executes and setsTaskHandletoNULL. This creates inconsistent state whereRunningis true butTaskHandleis NULL.🔎 Recommended fix
Move line 233 inside the success path:
if (_DevicesTask_State.Running) { ESP_LOGE(TAG, "Task did not stop in time"); return ESP_ERR_TIMEOUT; } _DevicesTask_State.TaskHandle = NULL; return ESP_OK;This ensures
TaskHandleis only cleared when the task has actually stopped.Committable suggestion skipped: line range outside the PR's diff.
main/Application/Tasks/GUI/guiTask.cpp-647-647 (1)
647-647: Minor typo in comment."recieved" should be "received".
main/Application/Tasks/GUI/guiTask.cpp-336-340 (1)
336-340: Logic error in minimum size constraint check.The
else ifon line 338 means thathis only checked for minimum size whenw >= 10. Both dimensions should be independently constrained.🔎 Proposed fix
/* Minimum size constraints */ if (w < 10) { w = 10; - } else if (h < 10) { + } + if (h < 10) { h = 10; }main/Application/Manager/Network/Server/websocket_handler.cpp-687-688 (1)
687-688: ReadingThermalFramefields without mutex inWebSocket_Handler_BroadcastTelemetry.Line 688 accesses
_WSHandler_State.ThermalFrame->temp_avgwithout holding the frame's mutex, whileWS_BroadcastTaskproperly acquiresThermalFrame->mutexbefore accessing frame data. This inconsistency could lead to reading torn/inconsistent temperature values.🔎 Proposed fix
/* Build telemetry data */ cJSON *data = cJSON_CreateObject(); if (_WSHandler_State.ThermalFrame != NULL) { + if (xSemaphoreTake(_WSHandler_State.ThermalFrame->mutex, 50 / portTICK_PERIOD_MS) == pdTRUE) { - cJSON_AddNumberToObject(data, "temp", _WSHandler_State.ThermalFrame->temp_avg); + cJSON_AddNumberToObject(data, "temp", _WSHandler_State.ThermalFrame->temp_avg); + xSemaphoreGive(_WSHandler_State.ThermalFrame->mutex); + } }main/Application/Manager/Network/Server/websocket_handler.cpp-550-558 (1)
550-558: Missing mutex protection forClientCountaccess.
WebSocket_Handler_GetClientCountandWebSocket_Handler_HasClientsread_WSHandler_State.ClientCountwithout mutex protection. This creates a data race withWS_AddClientandWS_RemoveClientwhich modifyClientCountunder the mutex.🔎 Proposed fix
uint8_t WebSocket_Handler_GetClientCount(void) { - return _WSHandler_State.ClientCount; + uint8_t count; + xSemaphoreTake(_WSHandler_State.ClientsMutex, portMAX_DELAY); + count = _WSHandler_State.ClientCount; + xSemaphoreGive(_WSHandler_State.ClientsMutex); + return count; } bool WebSocket_Handler_HasClients(void) { - return _WSHandler_State.ClientCount > 0; + bool hasClients; + xSemaphoreTake(_WSHandler_State.ClientsMutex, portMAX_DELAY); + hasClients = _WSHandler_State.ClientCount > 0; + xSemaphoreGive(_WSHandler_State.ClientsMutex); + return hasClients; }main/Application/Manager/Network/Provisioning/provisioning.cpp-172-209 (1)
172-209: Unusedp_Configparameter inProvisioning_Init.The
p_Configparameter is declared but never used within the function. All configuration values come from Kconfig macros (CONFIG_NETWORK_PROV_*). This could confuse callers who pass configuration expecting it to be applied.🔎 Proposed fix
Either remove the parameter:
-esp_err_t Provisioning_Init(Network_WiFi_STA_Config_t *p_Config) +esp_err_t Provisioning_Init(void)Or add a comment explaining why it's reserved:
esp_err_t Provisioning_Init(Network_WiFi_STA_Config_t *p_Config) { + (void)p_Config; /* Reserved for future use - config from Kconfig */ + if (_Provisioning_State.isInitialized) {main/Application/Manager/Network/Provisioning/provisioning.cpp-125-133 (1)
125-133: Potential double deinitialization ofwifi_prov_mgr.In the
WIFI_PROV_ENDevent handler,wifi_prov_mgr_deinit()is called at line 128. However,Provisioning_Deinit()(lines 212-232) also callswifi_prov_mgr_deinit()at line 222. IfWIFI_PROV_ENDfires beforeProvisioning_Deinit()is called, the manager will be deinitialized twice.🔎 Proposed fix
Track deinit state or guard the call in
Provisioning_Deinit:void Provisioning_Deinit(void) { if (_Provisioning_State.isInitialized == false) { return; } Provisioning_Stop(); vTaskDelay(200 / portTICK_PERIOD_MS); - wifi_prov_mgr_deinit(); + /* wifi_prov_mgr_deinit may have been called in WIFI_PROV_END event */ + if (_Provisioning_State.active) { + wifi_prov_mgr_deinit(); + }Or remove the deinit call from the event handler since
Provisioning_Deinithandles cleanup.Committable suggestion skipped: line range outside the PR's diff.
main/Application/Manager/Network/network_types.h-77-78 (1)
77-78: Complete the documentation comment.The documentation for
NETWORK_EVENT_CREDENTIALS_UPDATEDis incomplete: "Data is of ...". Please specify the data type for this event.🔎 Suggested completion
- NETWORK_EVENT_CREDENTIALS_UPDATED, /**< New WiFi credentials have been set - Data is of ... */ + NETWORK_EVENT_CREDENTIALS_UPDATED, /**< New WiFi credentials have been set + Data is of type Network_WiFi_Credentials_t */
🧹 Nitpick comments (18)
main/Application/Tasks/GUI/Export/project.info (1)
2-2: Consider using a more descriptive project name.The generic name "SquareLine_Project.spj" could be replaced with a name that reflects the actual project (e.g., "PyroVision_GUI.spj" or similar).
main/Application/Manager/SD/sdManager.cpp (1)
65-77: Consider initializing state structure to prevent undefined behavior.The static
_SD_Manager_Statestructure relies on BSS zero-initialization, which works but could be more explicit. If this code is ever moved or the struct is made non-static, uninitialized pointers could cause issues.🔎 Proposed fix
-static SD_Manager_State_t _SD_Manager_State; +static SD_Manager_State_t _SD_Manager_State = {};main/Application/Tasks/GUI/Private/guiHelper.cpp (1)
256-256: Missing NULL check before deleting touch handle.Other resources in the deinit function are guarded with NULL checks, but
esp_lcd_touch_delis called unconditionally. This could cause issues if initialization failed partway.🔎 Proposed fix
- esp_lcd_touch_del(p_GUITask_State->TouchHandle); + if (p_GUITask_State->TouchHandle != NULL) { + esp_lcd_touch_del(p_GUITask_State->TouchHandle); + p_GUITask_State->TouchHandle = NULL; + }main/Application/Manager/Devices/ADC/adc.cpp (1)
51-51: Missingstaticqualifier on TAG.
TAGis defined withoutstatic, which can cause ODR (One Definition Rule) violations if this translation unit is linked with others that also define aTAGvariable.🔎 Proposed fix
-const char *TAG = "ADC"; +static const char *TAG = "ADC";main/Application/Manager/Devices/PortExpander/portexpander.cpp (1)
230-247: Unused parameterp_ConfiginPortExpander_Init.The
p_Configparameter is passed but never used (only the bus handle is needed). Consider removing it from the signature or documenting why it's reserved for future use.main/Application/Manager/Settings/settings_types.h (1)
54-105: Consider explicit packing for NVS blob storage compatibility.These structures are stored in NVS and include a
Versionfield for migration. However, implicit compiler padding between members (e.g., afterboolfields) may differ across compiler versions or optimization settings, potentially causing settings corruption during firmware upgrades.Consider adding
__attribute__((packed))or using explicit padding bytes to ensure consistent binary layout.main/Application/Manager/Devices/RTC/rtc.cpp (1)
103-103: Storing pointer to caller's handle creates lifetime dependency.
_RTC_Dev_Handlestores a pointer to the caller'sp_Dev_Handlevariable. If the caller's storage is not persistent (e.g., a local variable), this becomes a dangling pointer. The current usage inDevicesManagerstores it in a static struct which is safe, but this pattern is fragile.Consider storing the handle value directly (
i2c_master_dev_handle_t) instead of a pointer to it, consistent with how_Expander_Dev_Handleworks in portexpander.cpp.Also applies to: 192-207
main/Application/Tasks/GUI/Export/ui_events.h (1)
1-22: Include LVGL headers for self-contained header.The header declares functions using
lv_event_t*(lines 13–16) but does not include the LVGL headers that define this type. Static analysis correctly flags "unknown type name 'lv_event_t'". While the implementation file likely includes the necessary headers, this header should be self-contained to avoid compilation issues when included in other files.🔎 Recommended fix
Add the LVGL include after line 7:
#ifndef _UI_EVENTS_H #define _UI_EVENTS_H +#include <lvgl.h> + #ifdef __cplusplus extern "C" { #endifThis ensures
lv_event_tis defined when the header is included.main/Application/Tasks/GUI/Export/ui_events.cpp (2)
37-38: Consider checkingesp_event_postreturn values.Both event posts use a timeout of 0, which is non-blocking and can fail silently if the event queue is full. Consider logging or handling failures for debugging purposes.
🔎 Proposed improvement
void ScreenInfoLoaded(lv_event_t *e) { - esp_event_post(GUI_EVENTS, GUI_EVENT_REQUEST_UPTIME, NULL, 0, 0); - esp_event_post(GUI_EVENTS, GUI_EVENT_REQUEST_FPA_AUX_TEMP, NULL, 0, 0); + if (esp_event_post(GUI_EVENTS, GUI_EVENT_REQUEST_UPTIME, NULL, 0, 0) != ESP_OK) { + ESP_LOGW("ui_events", "Failed to post uptime request event"); + } + if (esp_event_post(GUI_EVENTS, GUI_EVENT_REQUEST_FPA_AUX_TEMP, NULL, 0, 0) != ESP_OK) { + ESP_LOGW("ui_events", "Failed to post FPA temp request event"); + } }
41-95: Consider removing or tracking commented placeholder code.This file contains ~150 lines of commented-out menu scaffolding code. If this is planned functionality, consider tracking it in an issue rather than keeping it in the codebase. Commented code can become stale and confusing over time.
main/Application/Tasks/GUI/guiTask.cpp (1)
481-497: Add timeout to splash screen loading loop as noted in TODO.The loop waiting for
LEPTON_CAMERA_READYhas no timeout mechanism. If the Lepton camera fails to initialize, this will loop indefinitely (though watchdog resets will occur every 100ms cycle).Would you like me to help implement a timeout mechanism that shows an error state after a configurable duration?
main/Application/Tasks/Network/networkTask.cpp (1)
169-185: Remove or activate commented auto-connect logic.This block contains commented-out auto-connect logic. If this is planned functionality, consider tracking it in an issue. If not needed, remove the dead code to improve maintainability.
main/Application/Manager/Network/Server/http_server.cpp (1)
571-579: Consider checkinghttpd_register_uri_handlerreturn values.URI handler registrations can fail (e.g., if max handlers exceeded). Consider logging failures for debugging.
🔎 Example improvement
/* Register URI handlers */ - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Time); - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Image); - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Telemetry); - httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Update); + if (httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Time) != ESP_OK) { + ESP_LOGW(TAG, "Failed to register /time handler"); + } + if (httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Image) != ESP_OK) { + ESP_LOGW(TAG, "Failed to register /image handler"); + } + if (httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Telemetry) != ESP_OK) { + ESP_LOGW(TAG, "Failed to register /telemetry handler"); + } + if (httpd_register_uri_handler(_HTTPServer_State.Handle, &_URI_Update) != ESP_OK) { + ESP_LOGW(TAG, "Failed to register /update handler"); + }main/Application/Manager/Network/networkManager.cpp (1)
314-377: Incomplete cleanup on failure after partial initialization.
ESP_ERROR_CHECKat line 315 (esp_netif_init) will abort the program if it fails, but by that pointEventGrouphas already been allocated. More critically, lines 359-369 useESP_ERROR_CHECKfor event handler registration without cleanup - if registration fails, previously created resources (netifs, WiFi) are leaked before the abort.Consider either:
- Making all initialization failures recoverable with proper cleanup, or
- Ensuring
ESP_ERROR_CHECKis only used when partial cleanup is acceptable (typically at boot)🔎 Proposed fix for recoverable initialization
/* Initialize TCP/IP stack */ - ESP_ERROR_CHECK(esp_netif_init()); + Error = esp_netif_init(); + if (Error != ESP_OK) { + ESP_LOGE(TAG, "Failed to init netif: %d!", Error); + vEventGroupDelete(_Network_Manager_State.EventGroup); + _Network_Manager_State.EventGroup = NULL; + return Error; + }main/Application/Manager/Network/Provisioning/provisioning.h (2)
27-27: Missing FreeRTOS include forTaskHandle_t.Line 67 uses
TaskHandle_tbut the header doesn't include<freertos/FreeRTOS.h>or<freertos/task.h>. This relies on transitive includes fromnetwork_types.h, which is fragile and may break if that header's includes change.🔎 Proposed fix
#include "../network_types.h" + +#include <freertos/FreeRTOS.h> +#include <freertos/task.h>
29-33: Unused parameterp_ConfiginProvisioning_Init.The function signature declares
Network_WiFi_STA_Config_t *p_Config, but reviewing the implementation (from relevant snippets), this parameter is never used. The function only configures device name, POP, and timeout from Kconfig options.Consider either:
- Removing the unused parameter if configuration comes solely from Kconfig
- Using the parameter to allow runtime configuration override
main/Application/Manager/Settings/settingsManager.cpp (1)
384-390: RedundantPendingChanges = falsebefore callingSettingsManager_Save.Line 386 sets
_State.PendingChanges = false, butSettingsManager_Save(line 390) also sets it to false at line 241. This is harmless but redundant.🔎 Proposed fix
_SettingsManager_InitDefaults(&_State.Settings); - _State.PendingChanges = false; xSemaphoreGive(_State.Mutex); SettingsManager_Save(&_State.Settings);main/Application/Tasks/GUI/Export/ui_helpers.h (1)
68-69: Formatting: semicolon placement.The semicolon appears on a separate line after the function declaration. While syntactically valid, this is unusual. Since this is a generated file, the formatting is controlled by SquareLine Studio's code generator.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
components/ESP32-Leptonmain/Application/Manager/Devices/ADC/adc.cppmain/Application/Manager/Devices/PortExpander/portexpander.cppmain/Application/Manager/Devices/RTC/rtc.cppmain/Application/Manager/Devices/SPI/spi.cppmain/Application/Manager/Devices/devicesManager.cppmain/Application/Manager/Devices/devicesManager.hmain/Application/Manager/Network/Provisioning/provisioning.cppmain/Application/Manager/Network/Provisioning/provisioning.hmain/Application/Manager/Network/SNTP/sntp.cppmain/Application/Manager/Network/Server/http_server.cppmain/Application/Manager/Network/Server/image_encoder.cppmain/Application/Manager/Network/Server/image_encoder.hmain/Application/Manager/Network/Server/websocket_handler.cppmain/Application/Manager/Network/networkManager.cppmain/Application/Manager/Network/networkManager.hmain/Application/Manager/Network/network_types.hmain/Application/Manager/SD/sdManager.cppmain/Application/Manager/Settings/settingsManager.cppmain/Application/Manager/Settings/settings_types.hmain/Application/Tasks/Devices/devicesTask.cppmain/Application/Tasks/GUI/Export/fonts/ui_font_fa.cmain/Application/Tasks/GUI/Export/images/ui_img_logo_80x44_png.cmain/Application/Tasks/GUI/Export/images/ui_img_text_218x40_png.cmain/Application/Tasks/GUI/Export/project.infomain/Application/Tasks/GUI/Export/screens/ui_Info.cmain/Application/Tasks/GUI/Export/screens/ui_Info.hmain/Application/Tasks/GUI/Export/screens/ui_Main.cmain/Application/Tasks/GUI/Export/screens/ui_Main.hmain/Application/Tasks/GUI/Export/screens/ui_Menu.cmain/Application/Tasks/GUI/Export/screens/ui_Menu.hmain/Application/Tasks/GUI/Export/screens/ui_Splash.cmain/Application/Tasks/GUI/Export/screens/ui_Splash.hmain/Application/Tasks/GUI/Export/ui.cmain/Application/Tasks/GUI/Export/ui.hmain/Application/Tasks/GUI/Export/ui_events.cppmain/Application/Tasks/GUI/Export/ui_events.hmain/Application/Tasks/GUI/Export/ui_helpers.cmain/Application/Tasks/GUI/Export/ui_helpers.hmain/Application/Tasks/GUI/Private/guiHelper.cppmain/Application/Tasks/GUI/Private/guiHelper.hmain/Application/Tasks/GUI/guiTask.cppmain/Application/Tasks/GUI/guiTask.hmain/Application/Tasks/Lepton/leptonTask.cppmain/Application/Tasks/Lepton/leptonTask.hmain/Application/Tasks/Network/networkTask.cppmain/Application/Tasks/Network/networkTask.hmain/Application/application.hmain/main.cppui/PyroVision.sllui/PyroVision.spjui/project.info
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/project.info
- components/ESP32-Lepton
- ui/PyroVision.sll
- main/main.cpp
- main/Application/Manager/Network/Server/image_encoder.h
- main/Application/Tasks/Lepton/leptonTask.h
- main/Application/Manager/Network/Server/image_encoder.cpp
- main/Application/Manager/Network/SNTP/sntp.cpp
- main/Application/Tasks/GUI/Export/screens/ui_Splash.c
- main/Application/Manager/Devices/SPI/spi.cpp
- main/Application/Tasks/GUI/Export/ui_helpers.c
- main/Application/Tasks/Lepton/leptonTask.cpp
🧰 Additional context used
🧬 Code graph analysis (14)
main/Application/Tasks/GUI/Export/ui_events.h (1)
main/Application/Tasks/GUI/Export/ui_events.cpp (8)
ScreenMainLoaded(12-33)ScreenMainLoaded(12-12)ButtonMainWiFiClicked(196-199)ButtonMainWiFiClicked(196-196)ScreenMenuLoaded(97-194)ScreenMenuLoaded(97-97)ScreenInfoLoaded(35-39)ScreenInfoLoaded(35-35)
main/Application/Tasks/Devices/devicesTask.cpp (1)
main/Application/Manager/Devices/devicesManager.cpp (4)
DevicesManager_GetBatteryVoltage(174-209)DevicesManager_GetBatteryVoltage(174-174)DevicesManager_Init(97-139)DevicesManager_Init(97-97)
main/Application/Manager/Network/Provisioning/provisioning.cpp (1)
main/Application/Manager/Network/networkManager.cpp (2)
NetworkManager_SetCredentials(562-581)NetworkManager_SetCredentials(562-562)
main/Application/Tasks/GUI/Export/ui.h (1)
main/Application/Tasks/GUI/Export/ui.c (2)
ui_init(27-39)ui_destroy(41-47)
main/Application/Tasks/GUI/Export/ui.c (4)
main/Application/Tasks/GUI/Export/screens/ui_Splash.c (2)
ui_Splash_screen_init(19-112)ui_Splash_screen_destroy(114-129)main/Application/Tasks/GUI/Export/screens/ui_Main.c (2)
ui_Main_screen_init(77-428)ui_Main_screen_destroy(430-468)main/Application/Tasks/GUI/Export/screens/ui_Menu.c (2)
ui_Menu_screen_init(40-119)ui_Menu_screen_destroy(121-136)main/Application/Tasks/GUI/Export/screens/ui_Info.c (2)
ui_Info_screen_init(78-753)ui_Info_screen_destroy(755-808)
main/Application/Manager/Network/Server/http_server.cpp (1)
main/Application/Manager/Network/Server/image_encoder.cpp (4)
Image_Encoder_Encode(170-224)Image_Encoder_Encode(170-173)Image_Encoder_Free(226-238)Image_Encoder_Free(226-226)
main/Application/Manager/Network/Provisioning/provisioning.h (1)
main/Application/Manager/Network/Provisioning/provisioning.cpp (16)
Provisioning_Init(172-210)Provisioning_Init(172-172)Provisioning_Deinit(212-232)Provisioning_Deinit(212-212)Provisioning_Start(234-307)Provisioning_Start(234-234)Provisioning_Stop(309-328)Provisioning_Stop(309-309)Provisioning_isProvisioned(330-348)Provisioning_isProvisioned(330-330)Provisioning_Reset(350-357)Provisioning_Reset(350-350)Provisioning_isActive(359-362)Provisioning_isActive(359-359)Provisioning_SetNetworkTaskHandle(364-367)Provisioning_SetNetworkTaskHandle(364-364)
main/Application/Manager/Network/networkManager.cpp (2)
main/Application/Manager/Network/Server/server.h (2)
Server_Init(35-58)Server_Start(72-95)main/Application/Manager/Network/SNTP/sntp.cpp (2)
SNTP_Deinit(53-57)SNTP_Deinit(53-53)
main/Application/Tasks/GUI/Private/guiHelper.h (1)
main/Application/Tasks/GUI/Private/guiHelper.cpp (12)
GUI_Helper_Init(104-221)GUI_Helper_Init(104-104)GUI_Helper_Deinit(223-284)GUI_Helper_Deinit(223-223)GUI_Helper_Timer_ClockUpdate(286-301)GUI_Helper_Timer_ClockUpdate(286-286)GUI_Helper_Timer_SpotUpdate(303-326)GUI_Helper_Timer_SpotUpdate(303-303)GUI_Helper_Timer_SpotmeterUpdate(328-331)GUI_Helper_Timer_SpotmeterUpdate(328-328)GUI_Helper_Timer_SceneStatisticsUpdate(333-336)GUI_Helper_Timer_SceneStatisticsUpdate(333-333)
main/Application/Tasks/GUI/Private/guiHelper.cpp (2)
main/Application/Manager/Network/Server/server.h (1)
Server_isRunning(111-114)main/Application/Manager/Network/Server/websocket_handler.cpp (2)
WebSocket_Handler_BroadcastTelemetry(670-714)WebSocket_Handler_BroadcastTelemetry(670-670)
main/Application/Tasks/Network/networkTask.cpp (4)
main/Application/Manager/Network/Provisioning/provisioning.cpp (12)
Provisioning_Stop(309-328)Provisioning_Stop(309-309)Provisioning_isProvisioned(330-348)Provisioning_isProvisioned(330-330)Provisioning_Start(234-307)Provisioning_Start(234-234)Provisioning_Init(172-210)Provisioning_Init(172-172)Provisioning_Deinit(212-232)Provisioning_Deinit(212-212)Provisioning_SetNetworkTaskHandle(364-367)Provisioning_SetNetworkTaskHandle(364-364)main/Application/Manager/Network/networkManager.cpp (6)
NetworkManager_StartSTA(411-442)NetworkManager_StartSTA(411-411)NetworkManager_StartServer(444-450)NetworkManager_StartServer(444-444)NetworkManager_Stop(452-465)NetworkManager_Stop(452-452)main/Application/Manager/Settings/settingsManager.cpp (2)
SettingsManager_Save(215-250)SettingsManager_Save(215-215)main/Application/Tasks/GUI/guiTask.cpp (2)
on_Network_Event_Handler(114-188)on_Network_Event_Handler(114-114)
main/Application/Tasks/GUI/Export/screens/ui_Main.c (2)
main/Application/Tasks/GUI/Export/ui_events.cpp (4)
ScreenMainLoaded(12-33)ScreenMainLoaded(12-12)ButtonMainWiFiClicked(196-199)ButtonMainWiFiClicked(196-196)main/Application/Tasks/GUI/Export/ui_helpers.c (1)
_ui_screen_change(78-85)
main/Application/Tasks/GUI/Export/screens/ui_Info.c (2)
main/Application/Tasks/GUI/Export/ui_helpers.c (1)
_ui_screen_change(78-85)main/Application/Tasks/GUI/Export/ui_events.cpp (2)
ScreenInfoLoaded(35-39)ScreenInfoLoaded(35-35)
main/Application/Tasks/GUI/Export/screens/ui_Info.h (1)
main/Application/Tasks/GUI/Export/screens/ui_Info.c (4)
ui_Info_screen_init(78-753)ui_Info_screen_destroy(755-808)ui_event_Info(54-65)ui_event_Button_Info_Back(67-74)
🪛 Clang (14.0.6)
main/Application/Tasks/GUI/Export/ui_events.h
[error] 13-13: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 14-14: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 15-15: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
main/Application/Tasks/GUI/Export/screens/ui_Menu.h
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 23-23: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 24-24: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
main/Application/Tasks/GUI/Export/screens/ui_Main.h
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 23-23: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 24-24: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 25-25: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 26-26: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 27-27: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 28-28: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 29-29: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 30-30: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 31-31: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 32-32: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 33-33: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 34-34: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
main/Application/Tasks/GUI/Export/screens/ui_Info.h
[error] 16-16: unknown type name 'lv_event_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 23-23: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 24-24: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 25-25: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 26-26: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 27-27: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 28-28: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 29-29: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 30-30: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 31-31: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 32-32: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 33-33: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 34-34: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
main/Application/Tasks/GUI/Export/screens/ui_Splash.h
[error] 16-16: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 17-17: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 18-18: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 19-19: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 20-20: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 21-21: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
[error] 22-22: unknown type name 'lv_obj_t'
(clang-diagnostic-error)
🪛 Cppcheck (2.19.0)
main/Application/Tasks/GUI/Export/ui.c
[error] 18-18: #error "LV_COLOR_DEPTH should be 16bit to match SquareLine Studio's settings"
(preprocessorErrorDirective)
main/Application/Manager/Network/networkManager.cpp
[error] 232-232: There is an unknown macro here somewhere. Configuration is required. If MACSTR is a macro then please configure it.
(unknownMacro)
main/Application/Tasks/GUI/Private/guiHelper.cpp
[error] 216-216: Array 'p_GUITask_State->UpdateTimer[3]' accessed at index 3, which is out of bounds.
(arrayIndexOutOfBounds)
🔇 Additional comments (33)
main/Application/Manager/SD/sdManager.cpp (4)
46-77: LGTM! Well-structured preprocessor configuration and state management.The SPI host selection with compile-time error for missing configuration is good defensive programming. The state structure is appropriately organized with
volatilecorrectly applied toCardPresentfor ISR access.
254-257: LGTM!Simple accessor with correct use of volatile for ISR-modified state.
283-293: Consider BMP row order: positivebiHeightimplies bottom-up storage.In standard BMP format, a positive
biHeight(line 285) means rows are stored bottom-to-top. Ifp_Buffercontains top-to-bottom pixel data, the saved image will appear vertically flipped. If this is intentional, consider adding a comment; otherwise, use a negativebiHeightfor top-down storage.
386-417: LGTM! Good defensive checks and task pinning strategy.The guards against missing card and concurrent mount tasks are appropriate. Pinning to CPU 1 to avoid blocking the GUI task is a good design decision.
Note: The double-mount concern with
SDManager_Init(flagged earlier) also applies here.main/Application/Manager/Devices/ADC/adc.cpp (1)
103-123: LGTM!The battery read function properly validates the pointer, reads raw ADC data, applies calibration when available, and falls back to raw values otherwise.
main/Application/Manager/Devices/devicesManager.h (1)
1-63: LGTM!Well-structured header with proper include guards, Doxygen documentation, and a clean public API surface.
main/Application/Manager/Devices/devicesManager.cpp (2)
174-208: Battery voltage reading will fail due to inverted port expander logic.
PortExpander_EnableBatteryVoltage(true)(line 186) uses inverted logic (see portexpander.cpp), meaning the battery measurement circuit is actually disabled when this function is called. The ADC read will return incorrect values. This depends on fixing the inverted logic inportexpander.cpp.
97-139: LGTM!The initialization sequence is well-structured with proper error handling, single-initialization guard, and appropriate delays for hardware stabilization.
main/Application/Manager/Settings/settings_types.h (1)
1-107: LGTM!Well-designed settings structure with version field for migration, reserved space for future expansion, and clear Doxygen documentation for the event system.
main/Application/Manager/Devices/RTC/rtc.cpp (2)
266-303: LGTM!Time conversion logic correctly handles BCD encoding, struct tm field mappings, and century bit for dates beyond 2099.
349-365: LGTM!Alarm configuration correctly handles the RV8263-C8's inverted AE bit semantics (0 = enabled, 1 = disabled).
main/Application/Tasks/GUI/Export/ui.h (1)
1-48: LGTM! Well-structured generated UI header.The header follows best practices with proper include guards, extern C linkage, and clear organization of UI entities (screens, assets, API). The SquareLine Studio generation is consistent with the LVGL 9.1.0 target version.
main/Application/Tasks/Network/networkTask.h (1)
1-60: LGTM! Clean task interface design.The Network task API follows a consistent lifecycle pattern with clear documentation. The function signatures are well-structured, accepting the application context where needed and returning appropriate error codes.
main/Application/Tasks/GUI/guiTask.h (1)
1-67: LGTM! Consistent task API design.The GUI task interface mirrors the Network task pattern with appropriate lifecycle functions. The addition of
GUI_Toggle_ROI_EditMode()provides clear touch interaction functionality for thermal image ROI adjustment, well-documented in the comments.main/Application/Tasks/GUI/Export/images/ui_img_logo_80x44_png.c (1)
1-136: LGTM! Generated image asset properly structured.The LVGL image descriptor and data array are correctly formatted for an 80×44 PNG with alpha channel. The memory alignment macro and image structure fields (width, height, color format, magic number) are properly initialized.
main/Application/Tasks/GUI/Export/ui_events.cpp (1)
196-199: LGTM!The WiFi button click handler correctly posts the network event to trigger WiFi provisioning.
main/Application/Tasks/GUI/guiTask.cpp (1)
804-824: LGTM!Good defensive programming with proper null checks and cascading cleanup on buffer allocation failures.
main/Application/Tasks/Network/networkTask.cpp (2)
241-249: Verify: Provisioning starts on every WiFi disconnection.When WiFi disconnects,
Provisioning_Start()is called immediately. This may not be the desired behavior if the disconnect is temporary (e.g., AP reboot, interference). Consider adding a reconnection attempt or delay before starting provisioning.
429-448: LGTM!Good graceful shutdown pattern using event group signaling and waiting for the task to exit cleanly, unlike the GUI task which uses direct
vTaskDelete().main/Application/Tasks/GUI/Export/screens/ui_Menu.c (1)
121-135: LGTM!The destroy function properly checks for null before deletion and cleans up all object pointers.
main/Application/Manager/Network/networkManager.cpp (1)
232-232: Static analysis false positive:MACSTRis a valid ESP-IDF macro.The Cppcheck warning about
MACSTRbeing an unknown macro is a false positive.MACSTRandMAC2STRare standard ESP-IDF macros defined inesp_mac.hfor formatting MAC addresses.main/Application/Tasks/GUI/Export/screens/ui_Info.c (2)
1-5: Auto-generated UI code from SquareLine Studio.This file is generated by SquareLine Studio. Manual modifications should be avoided as they will be overwritten on regeneration. The implementation follows LVGL 9.1.0 patterns correctly with proper screen lifecycle management.
455-457: LVGL 9.1 workaround for bar/slider draw crash properly applied.This code correctly addresses the known LVGL 9.1 bug where Bar/Slider objects crash or render incorrectly when top-padding is non-zero and right-padding is zero. The workaround—adding 1 pixel to right-padding when top-padding exceeds zero—is the standard fix documented in the LVGL community. The inline comment clearly explains the issue.
main/Application/Manager/Settings/settingsManager.cpp (1)
267-288: Event posted with pointer to parameter data after function may return.In
SettingsManager_SetLeptonand similar setters,esp_event_postis called withp_Lepton(line 284-285) after the mutex is released. The caller may free or modify this memory before/during event processing. However,esp_event_postcopies the data, so this is actually safe.main/Application/Tasks/GUI/Export/screens/ui_Main.c (2)
1-5: Auto-generated UI code from SquareLine Studio.This file is generated by SquareLine Studio and implements the main thermal camera screen. The implementation correctly follows LVGL 9.1.0 patterns with proper event wiring and screen lifecycle management.
430-467: Proper cleanup inui_Main_screen_destroy.The destroy function correctly deletes the root object (which cascades to children) and nulls all global pointers to prevent dangling references. This is important for the lazy initialization pattern used in
_ui_screen_change.main/Application/Tasks/GUI/Export/ui_helpers.h (1)
1-4: Generated file - avoid manual edits.This file is auto-generated by SquareLine Studio. Manual changes will be overwritten on regeneration. Any required modifications should be made in the SquareLine Studio project and regenerated.
main/Application/Manager/Network/network_types.h (2)
4-4: Verify copyright year 2026.The copyright year is 2026, which is in the future. Confirm whether this is intentional (for a planned release) or should be corrected to 2025.
24-219: Well-structured network type definitions.The header provides a comprehensive and well-organized set of network types, events, and configuration structures. The ESP event integration, enum definitions, and data structures follow ESP-IDF conventions and provide good type safety.
main/Application/Tasks/GUI/Export/screens/ui_Main.h (2)
1-4: Generated file - avoid manual edits.This file is auto-generated by SquareLine Studio. Manual changes will be overwritten on regeneration. Any required modifications should be made in the SquareLine Studio project and regenerated.
14-49: LVGL types resolved at build time.Static analysis reports unknown types
lv_event_tandlv_obj_t, but this is a false positive. These LVGL types are provided through the component include paths in the ESP-IDF build system. The implementation file that includes this header will have access to the full LVGL headers, resolving all type dependencies correctly.main/Application/Tasks/GUI/Export/screens/ui_Info.h (2)
1-4: Generated file - avoid manual edits.This file is auto-generated by SquareLine Studio. Manual changes will be overwritten on regeneration. Any required modifications should be made in the SquareLine Studio project and regenerated.
14-62: LVGL types resolved at build time.Similar to
ui_Main.h, static analysis reports unknown LVGL types, but this is a false positive. The types are provided through ESP-IDF component includes and resolved during the build process.
| cJSON *response = cJSON_CreateObject(); | ||
| cJSON_AddStringToObject(response, "status", "ok"); | ||
| Error = HTTP_Server_SendJSON(p_Request, response, 200); | ||
| cJSON_Delete(response); |
There was a problem hiding this comment.
Missing null check for cJSON_CreateObject in response creation.
Similar to HTTP_Server_SendError, the response JSON object creation doesn't check for allocation failure before adding items.
🔎 Proposed fix
/* Send response */
cJSON *response = cJSON_CreateObject();
+ if (response == NULL) {
+ return HTTP_Server_SendError(p_Request, 500, "Memory allocation failed");
+ }
cJSON_AddStringToObject(response, "status", "ok");
Error = HTTP_Server_SendJSON(p_Request, response, 200);
cJSON_Delete(response);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cJSON *response = cJSON_CreateObject(); | |
| cJSON_AddStringToObject(response, "status", "ok"); | |
| Error = HTTP_Server_SendJSON(p_Request, response, 200); | |
| cJSON_Delete(response); | |
| /* Send response */ | |
| cJSON *response = cJSON_CreateObject(); | |
| if (response == NULL) { | |
| return HTTP_Server_SendError(p_Request, 500, "Memory allocation failed"); | |
| } | |
| cJSON_AddStringToObject(response, "status", "ok"); | |
| Error = HTTP_Server_SendJSON(p_Request, response, 200); | |
| cJSON_Delete(response); |
🤖 Prompt for AI Agents
In main/Application/Manager/Network/Server/http_server.cpp around lines 203 to
206, the call to cJSON_CreateObject() is not checked for NULL before adding
fields and sending the response; add a null check immediately after
cJSON_CreateObject() and if it returns NULL, call the existing error handling
(e.g., HTTP_Server_SendError with an appropriate 500/internal allocation-failure
message or set Error and return) without attempting cJSON_AddStringToObject or
cJSON_Delete on the NULL pointer; otherwise proceed to add "status":"ok", send
the JSON and delete the object as before.
| /* Send response */ | ||
| cJSON *response = cJSON_CreateObject(); | ||
| cJSON_AddStringToObject(response, "status", "updating"); | ||
| cJSON_AddStringToObject(response, "message", "Firmware upload successful. Device will reboot after update."); | ||
| Error = HTTP_Server_SendJSON(p_Request, response, 200); | ||
| cJSON_Delete(response); |
There was a problem hiding this comment.
Missing null check for cJSON_CreateObject in OTA response.
Consistent with other handlers, this response creation should check for allocation failure.
🔎 Proposed fix
/* Send response */
cJSON *response = cJSON_CreateObject();
+ if (response == NULL) {
+ httpd_resp_send_err(p_Request, HTTPD_500_INTERNAL_SERVER_ERROR, "Memory allocation failed");
+ vTaskDelay(1000 / portTICK_PERIOD_MS);
+ esp_restart();
+ return ESP_ERR_NO_MEM;
+ }
cJSON_AddStringToObject(response, "status", "updating");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Send response */ | |
| cJSON *response = cJSON_CreateObject(); | |
| cJSON_AddStringToObject(response, "status", "updating"); | |
| cJSON_AddStringToObject(response, "message", "Firmware upload successful. Device will reboot after update."); | |
| Error = HTTP_Server_SendJSON(p_Request, response, 200); | |
| cJSON_Delete(response); | |
| /* Send response */ | |
| cJSON *response = cJSON_CreateObject(); | |
| if (response == NULL) { | |
| httpd_resp_send_err(p_Request, HTTPD_500_INTERNAL_SERVER_ERROR, "Memory allocation failed"); | |
| vTaskDelay(1000 / portTICK_PERIOD_MS); | |
| esp_restart(); | |
| return ESP_ERR_NO_MEM; | |
| } | |
| cJSON_AddStringToObject(response, "status", "updating"); | |
| cJSON_AddStringToObject(response, "message", "Firmware upload successful. Device will reboot after update."); | |
| Error = HTTP_Server_SendJSON(p_Request, response, 200); | |
| cJSON_Delete(response); |
🤖 Prompt for AI Agents
In main/Application/Manager/Network/Server/http_server.cpp around lines 420 to
425, the call to cJSON_CreateObject() for the OTA response lacks a
NULL/allocation check; add a check immediately after cJSON_CreateObject() and
handle allocation failure by logging/sending an HTTP 500 JSON error (or
appropriate error via HTTP_Server_SendJSON) and returning/setting Error
accordingly, avoiding further cJSON calls if response is NULL and ensuring no
cJSON_Delete is called on a NULL pointer.
| if (esp_vfs_fat_sdspi_mount("/sdcard", &_SD_Manager_State.Host, &_SD_Manager_State.SlotConfig, | ||
| &_SD_Manager_State.MountConfig, &_SD_Manager_State.Card) != ESP_OK) { | ||
| ESP_LOGE(TAG, "Failed to mount SD card filesystem!"); | ||
| return ESP_FAIL; | ||
| } |
There was a problem hiding this comment.
Potential double-mount: SDManager_Init mounts the card, but SDManager_Mount also exists.
SDManager_Init immediately mounts the SD card (line 147-151), but there's also a separate SDManager_Mount() function that performs async mounting. If both are called, this could cause resource conflicts or undefined behavior with the FAT filesystem.
Consider either:
- Remove the mount from
SDManager_Initand require explicitSDManager_Mount()call - Track mount state separately and prevent re-mounting if already mounted
🔎 Proposed fix - track mount state
typedef struct {
sdmmc_card_t *Card;
sdmmc_host_t Host;
sdspi_device_config_t SlotConfig;
bool isInitialized;
+ bool isMounted;
esp_timer_handle_t DebounceTimer;
...
} SD_Manager_State_t;Then check isMounted before attempting to mount in both SDManager_Init and SDManager_MountTask.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In main/Application/Manager/SD/sdManager.cpp around lines 147-151, the init
function always mounts the SD which can double-mount when SDManager_Mount is
used; add a boolean mount state (e.g., _SD_Manager_State.isMounted) and check it
before calling esp_vfs_fat_sdspi_mount in both SDManager_Init and in the async
SDManager_MountTask, set isMounted = true only on successful mount, and set it
false on unmount/fail; protect reads/writes to isMounted with the existing
manager mutex or a new mutex to avoid race conditions and ensure SDManager_Mount
returns/aborts early if already mounted.
| #if CONFIG_SD_CARD_PIN_CD > 0 | ||
| /* Stop and delete debounce timer */ | ||
| if (_SD_Manager_State.DebounceTimer != NULL) { | ||
| esp_timer_stop(_SD_Manager_State.DebounceTimer); | ||
| esp_timer_delete(_SD_Manager_State.DebounceTimer); | ||
| _SD_Manager_State.DebounceTimer = NULL; | ||
| } | ||
| #endif /* CONFIG_SD_CARD_PIN_CD */ |
There was a problem hiding this comment.
Missing GPIO ISR handler removal during deinit.
The ISR handler is added via gpio_isr_handler_add() in SDManager_Init, but it's not removed in SDManager_Deinit. This can cause crashes if the ISR fires after deinitialization.
🔎 Proposed fix
#if CONFIG_SD_CARD_PIN_CD > 0
+ /* Remove ISR handler first to prevent interrupts during cleanup */
+ gpio_isr_handler_remove(static_cast<gpio_num_t>(CONFIG_SD_CARD_PIN_CD));
+
/* Stop and delete debounce timer */
if (_SD_Manager_State.DebounceTimer != NULL) {
esp_timer_stop(_SD_Manager_State.DebounceTimer);
esp_timer_delete(_SD_Manager_State.DebounceTimer);
_SD_Manager_State.DebounceTimer = NULL;
}
#endif /* CONFIG_SD_CARD_PIN_CD */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if CONFIG_SD_CARD_PIN_CD > 0 | |
| /* Stop and delete debounce timer */ | |
| if (_SD_Manager_State.DebounceTimer != NULL) { | |
| esp_timer_stop(_SD_Manager_State.DebounceTimer); | |
| esp_timer_delete(_SD_Manager_State.DebounceTimer); | |
| _SD_Manager_State.DebounceTimer = NULL; | |
| } | |
| #endif /* CONFIG_SD_CARD_PIN_CD */ | |
| #if CONFIG_SD_CARD_PIN_CD > 0 | |
| /* Remove ISR handler first to prevent interrupts during cleanup */ | |
| gpio_isr_handler_remove(static_cast<gpio_num_t>(CONFIG_SD_CARD_PIN_CD)); | |
| /* Stop and delete debounce timer */ | |
| if (_SD_Manager_State.DebounceTimer != NULL) { | |
| esp_timer_stop(_SD_Manager_State.DebounceTimer); | |
| esp_timer_delete(_SD_Manager_State.DebounceTimer); | |
| _SD_Manager_State.DebounceTimer = NULL; | |
| } | |
| #endif /* CONFIG_SD_CARD_PIN_CD */ |
🤖 Prompt for AI Agents
In main/Application/Manager/SD/sdManager.cpp around lines 230 to 237, the deinit
path stops and deletes the debounce timer but fails to remove the GPIO ISR
handler added in SDManager_Init; add code inside the same CONFIG_SD_CARD_PIN_CD
> 0 block to call gpio_isr_handler_remove(CONFIG_SD_CARD_PIN_CD) (or the
variable holding the CD pin) before deleting the timer, and ensure the call is
guarded by a valid pin check so the ISR cannot fire after deinitialization.
Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
- Add documentation with AsciiDocs - Change Settings logic to allow loading default settings from JSON - All settings are handled via the Settings Manager and not with local copies of the settings Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
- Replace touch controller with GT911 - Improve loading of default settings from SD card - UI improvements and fixes Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
Kampi
force-pushed
the
1.0_Dev
branch
4 times, most recently
from
ab732e5 to
687ec6d
Compare
Kampi
force-pushed
the
1.0_Dev
branch
4 times, most recently
from
2b68f4a to
1fdf6c9
Compare
Kampi
force-pushed
the
1.0_Dev
branch
7 times, most recently
from
21012b3 to
e9ea185
Compare
|
@copilot review please |
…menu, add coredump functionality, add memory manager to handle the internal flash and the SD card - Remove the settings file after loading - Add USB support for mass storage class, add USB and memory manager - Fix CI/CD - Format style, update firmware workflow - Update documentation - Add version to settings, rework settings structure, clean up code, add task skeleton for the camera - Image is stored as PNG - 240x180 pixel is used now - Move Touch I2C into Device Manager - Add Skeletons for the new sensors Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
Kampi
force-pushed
the
1.0_Dev
branch
2 times, most recently
from
858b55e to
60c41da
Compare
- UVC working - MSC working
Refactor USB descriptors to include UVC and CDC support with updated endpoint configurations.
- Add Python script for patching Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
Summary by CodeRabbit
New Features
Networking & Time
Hardware Support
Persistence & Settings
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.