Move to Pioarduino

[jniebuhr]

Summary by CodeRabbit

• New Features
  • Added in-app memory monitoring with optional heap profiling/diagnostics.
  • Added a new /api/debug/heap endpoint for live heap/PSRAM stats.
• Bug Fixes
  • Improved BLE client/server compatibility and reconnection behavior.
  • Enhanced OTA HTTPS reliability by attaching the bundled CA just-in-time for updates.
  • Improved hardware probing/sleep behavior and battery-voltage measurement consistency.
• Chores
  • Updated build tooling/config, dependency sets, and added an SDK configuration validation step.
  • Removed ESP32-S3-SuperMini board configuration.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR migrates the firmware toward combined Arduino/ESP-IDF builds, updates NimBLE integrations, standardizes FreeRTOS task handles, adds memory and heap diagnostics, adjusts OTA trust-store handling, and modifies display drivers, plugins, and UI behavior.

Changes

Firmware build and runtime migration

Layer / File(s)	Summary
Build foundation and SDK validation CMakeLists.txt, platformio.ini, scripts/*, sdkconfig.*, src/CMakeLists.txt, src/idf_component.yml, .gitignore	Adds dual-framework CMake/PlatformIO configuration, ESP-IDF defaults, NimBLE component wiring, include-path setup, and merged-sdkconfig validation.
Controller contracts and task handles lib/GaggiMateController/src/peripherals/*, src/display/core/{Controller,Settings}.h, src/display/plugins/ShotHistoryPlugin.h, src/display/ui/default/DefaultUI.h	Uses TaskHandle_t, adds noreturn annotations, and makes pump and temperature sensor interfaces explicitly abstract.
NimBLE and BLE OTA migration lib/NanoPbComm/src/ble/*, lib/ble_ota_dfu/src/*, lib/*/library.json, lib/ble_ota_dfu/library.properties	Adapts scanning, connection, server, subscription, write, notification, and DFU callbacks to NimBLE 2.x APIs.
OTA trust-store handling lib/OTA/src/*, lib/OTA/library.properties	Adds certificate-bundle symbols and attaches the bundle immediately before HTTPS redirect, version, firmware, filesystem, and controller update operations.
Display runtime and observability src/display/core/*, src/display/plugins/WebUIPlugin.*	Adds ESPMemoryMonitor integration, heap checkpoints, debug heap JSON output, deferred settings loading, and controller configuration resend timing.
Display driver compatibility src/display/drivers/*	Replaces ADC calibration reads with millivolt sampling, updates panel APIs and PWM handling, removes obsolete RGB callback fields, and adds touch/I2C recovery changes.
Plugins and UI behavior src/display/plugins/*, src/display/ui/default/*	Refactors HomeKit storage and setup, rate-limits BLE scale retries, updates logging, skips OTA checks in AP mode, and changes UI lambdas to [this] captures with screen validity checks.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• jniebuhr/gaggimate#681: Overlapping esp-nimble-cpp and BLE transport migration work.
• jniebuhr/gaggimate#755: Overlaps with WebUI OTA update-check behavior.
• jniebuhr/gaggimate#335: Overlaps with headless-build and PlatformIO configuration changes.

Suggested labels: cla-signed

Suggested reviewers: nayrod485

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.47% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main change: migrating the project to Pioarduino and related build configuration updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/pioarduino-move

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch feature/pioarduino-move

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (10)

lib/ble_ota_dfu/src/ble_ota_dfu.cpp (1)

201-209: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don't pack filesystem sizes through uint16_t.

FLASH.totalBytes() and FLASH.usedBytes() are emitted as three-byte values, but both locals are uint16_t, so anything above 65535 is truncated before encoding. On normal ESP32 SPIFFS sizes, the client receives bogus capacity numbers.

Suggested fix

-            uint16_t total_size = FLASH.totalBytes();
-            uint16_t used_size = FLASH.usedBytes();
+            uint32_t total_size = FLASH.totalBytes();
+            uint32_t used_size = FLASH.usedBytes();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/ble_ota_dfu/src/ble_ota_dfu.cpp` around lines 201 - 209, The code
truncates FLASH.totalBytes() and FLASH.usedBytes() by storing them in uint16_t
(total_size, used_size) before packing into flash_size; change those locals to
uint32_t (e.g., uint32_t total_size = FLASH.totalBytes(); uint32_t used_size =
FLASH.usedBytes();) and keep the packing into flash_size using the same byte
shifts and static_cast<uint8_t> so the three high/med/low bytes are encoded
correctly (refer to total_size, used_size, FLASH.totalBytes(),
FLASH.usedBytes(), and the flash_size array).

lib/OTA/src/ControllerOTA.cpp (3)

141-163: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make fillBuffer() report short-read failure to its callers.

After 300 timeouts, fillBuffer() returns early, but the callers still assume len bytes were produced. downloadFile() then writes bufferSize bytes anyway, and sendPart() transmits whatever remains in the stack buffer. That can silently corrupt the staged firmware image.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/OTA/src/ControllerOTA.cpp` around lines 141 - 163, The function
fillBuffer currently returns early on timeout but provides no signal to callers
(fillBuffer) leading downloadFile and sendPart to assume the full len/
bufferSize was filled and proceed; change fillBuffer(const Stream &in, uint8_t
*buffer, uint16_t len) to return a status (e.g., bool success or size_t
bytesRead) and ensure it returns false or the actual bytesRead when
timeout_failures triggers (using bufferLen); update callers downloadFile and
sendPart to check the return value (or bytes read) and abort/handle the
short-read (do not write/transmit more than the returned bytes, and surface an
error) so partial reads cannot corrupt the staged firmware image, referencing
function names fillBuffer, downloadFile, sendPart and the variables len,
bufferLen, bufferSize, timeout_failures.

---

27-32: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Abort the OTA flow when the download or staged file open fails.

update() logs the failure, but it still opens /board-firmware.bin and calls runUpdate(file, file.size()). That converts a transient HTTP/SPIFFS error into an invalid BLE transfer instead of a clean abort.

Suggested fix

 void ControllerOTA::update(WiFiClientSecure &wifi_client, const String &release_url) {
     if (SPIFFS.exists("/board-firmware.bin")) {
         ESP_LOGI("ControllerOTA", "Removing previous update file");
         SPIFFS.remove("/board-firmware.bin");
     }
     if (!downloadFile(wifi_client, release_url)) {
         ESP_LOGE("ControllerOTA", "Download of firmware file failed");
+        return;
     }
     File file = SPIFFS.open("/board-firmware.bin", FILE_READ);
+    if (!file) {
+        ESP_LOGE("ControllerOTA", "Failed to open staged firmware");
+        return;
+    }
     runUpdate(file, file.size());
     file.close();
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/OTA/src/ControllerOTA.cpp` around lines 27 - 32, Abort the OTA flow when
the download or staged file open fails: if downloadFile(wifi_client,
release_url) returns false, log the error and immediately return/abort instead
of continuing; after attempting File file = SPIFFS.open("/board-firmware.bin",
FILE_READ), check that the File is valid (truthy) and file.size() > 0, log an
error and return/abort if it isn’t, and only call runUpdate(file, file.size())
when both checks pass. Ensure the changes touch the downloadFile check, the
SPIFFS.open/File validity check, and the runUpdate call so no further OTA steps
run on failure.

---

209-210: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add minimum-length guards on the updated NimBLE callback payloads in lib/OTA/src/ControllerOTA.cpp and lib/ble_ota_dfu/src/ble_ota_dfu.cpp. ControllerOTA::onReceive and BLEOverTheAirDeviceFirmwareUpdate::onWrite both assume the callback buffer is non-empty, and the DFU command handler further assumes fixed-width payloads for 0xFC, 0xFE, and 0xFF. Empty or truncated BLE packets can therefore read past the buffer and destabilize the OTA flow in both files.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/OTA/src/ControllerOTA.cpp` around lines 209 - 210,
ControllerOTA::onReceive and BLEOverTheAirDeviceFirmwareUpdate::onWrite
currently assume pData has enough bytes and read past the buffer; add explicit
length guards: in ControllerOTA::onReceive check length >= 1 before reading
pData[0] (return/ignore if not), and in
BLEOverTheAirDeviceFirmwareUpdate::onWrite inspect the first byte (command) only
after verifying length >= 1 and then validate length against the expected fixed
payload size for commands 0xFC, 0xFE, and 0xFF (e.g., require N bytes for each
command) before reading additional indexes; on any undersize packet, safely bail
out (log/ignore) instead of dereferencing out-of-bounds data.

lib/NanoPbComm/src/ble/BleServerTransport.cpp (1)

46-51: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Delay the usable-connection state until the TX path is actually ready.

On Line 57 and Line 60, the transport becomes “connected” as soon as the GATT link opens, but controller→display traffic still depends on _txChar->notify(). Between onConnect() and the later subscribe event, isConnected() reports usable and send() returns true unconditionally on Line 51 even if the notification was not handed off, so early outbound frames can be lost silently. Gate the connected state on the notify-ready condition and return the actual notify result instead of always succeeding.

Also applies to: 56-78

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/NanoPbComm/src/ble/BleServerTransport.cpp` around lines 46 - 51, The
transport currently marks connection usable as soon as onConnect() sets
_connected, but send(const uint8_t *data, size_t length) returns true even if
_txChar->notify() isn't ready; change the logic so the usable/connected state is
gated by TX-notify readiness (e.g., track a _txReady flag set when the
subscribe/notify-ready callback runs) and update onConnect()/subscribe handler
to only set _connected (or set _txReady and then _connected) when notifications
are actually available; in BleServerTransport::send, check both
_connected/_txChar and the new _txReady, call _txChar->setValue(...); return the
actual result of _txChar->notify() (or its boolean) instead of always returning
true so callers see real failure when notify can't hand off the packet.

lib/OTA/src/GitHubOTA.cpp (1)

41-74: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reattach the CA bundle before the update-check requests too.

After removing the constructor-level attach_ca_bundle(_wifi_client), Line 44 and Line 63 still send _wifi_client through the HTTPS lookup helpers with no trust store attached. That leaves checkForUpdates() as a broken path even though update(), update_firmware(), and update_filesystem() were fixed. Attach the CA bundle before the first helper here, or move that step into the shared HTTP helper layer so every GitHub HTTPS call is covered.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/OTA/src/GitHubOTA.cpp` around lines 41 - 74, checkForUpdates() calls
get_updated_base_url_via_redirect(_wifi_client, _release_url) and
get_updated_version_via_txt_file(_wifi_client, _latest_url) without re-attaching
the CA bundle; before the first network call in GitHubOTA::checkForUpdates, call
attach_ca_bundle(_wifi_client) (or move attach_ca_bundle into the shared HTTPS
helpers) so _wifi_client has the trust store attached for both
get_updated_base_url_via_redirect and get_updated_version_via_txt_file; ensure
this is done before any early returns so both code paths use the attached CA
bundle.

src/display/core/Controller.cpp (1)

410-415: ⚠️ Potential issue | 🟠 Major

Remove the redundant SNTP re-initialization after configTzTime (src/display/core/Controller.cpp ~410-415)

configTzTime(resolve_timezone(...), NTP_SERVER) already stops/reconfigures SNTP, sets the NTP server name(s), calls sntp_init(), and applies TZ via setenv + tzset. The subsequent esp_sntp_setservername(0, NTP_SERVER) + esp_sntp_init() re-initializes the same SNTP client a second time, making the final configuration order-dependent and increasing risk of inconsistent behavior. Keep either the Arduino configTzTime(...) flow (and remove the esp_sntp_* calls) or switch fully to an explicit esp_sntp_* sequence (and don’t call configTzTime). (esp32-hal-time.c configTzTime, ESP-IDF system time SNTP API)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/core/Controller.cpp` around lines 410 - 415, The code calls
configTzTime(resolve_timezone(settings.getTimezone()), NTP_SERVER) which already
sets TZ (via setenv+tzset), configures SNTP servers and calls sntp_init, so
remove the redundant esp_sntp_setservername(0, NTP_SERVER) and esp_sntp_init()
calls to avoid re-initializing SNTP and order-dependent behavior; keep the
configTzTime + setenv + tzset flow (or if you prefer manual control remove
configTzTime and replace it with an explicit
esp_sntp_setservername/esp_sntp_init sequence), and ensure only one SNTP
initialization path (referencing configTzTime, esp_sntp_setservername,
esp_sntp_init, setenv, tzset).

src/display/core/Settings.cpp (1)

119-129: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle xTaskCreate failure or settings stop persisting after boot.

load() now owns creation of the async save task, but the xTaskCreate(...) result is ignored. On a low-memory boot path, taskHandle stays null and every later save() call only sets dirty = true; nothing ever flushes it because Controller::setup() only calls settings.load() once. That turns all subsequent setting changes into silent data loss on the next reboot.

💡 Suggested fix

-    xTaskCreate(loopTask, "Settings::loop", configMINIMAL_STACK_SIZE * 6, this, 1, &taskHandle);
+    if (xTaskCreate(loopTask, "Settings::loop", configMINIMAL_STACK_SIZE * 6, this, 1, &taskHandle) != pdPASS) {
+        taskHandle = nullptr;
+        ESP_LOGE("Settings", "Failed to start Settings::loop; falling back to synchronous saves");
+    }
 }
 
 void Settings::save(bool noDelay) {
-    if (noDelay) {
+    if (noDelay || taskHandle == nullptr) {
         doSave();
         return;
     }
     dirty = true;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/core/Settings.cpp` around lines 119 - 129, The async save task
creation result from xTaskCreate in Settings::load is ignored causing taskHandle
to remain null on low-memory boots and later save() calls only flip dirty
without flushing; update Settings::load to check xTaskCreate's return (compare
to pdPASS) and if creation fails set taskHandle = nullptr and immediately fall
back to a synchronous persist (call doSave()) or mark a retry strategy;
additionally update Settings::save to detect a null/invalid taskHandle and
perform doSave() (or a safe immediate write) instead of only setting dirty so
settings still persist even if the async task couldn't be created; reference
xTaskCreate, Settings::load, Settings::save, taskHandle, doSave, dirty and
Controller::setup when making the change.

src/display/ui/default/DefaultUI.h (1)

55-57: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Initialize the task handles before exposing them to the health check.

isTaskHealthy() calls eTaskGetState() on both members unconditionally, but these handles are still left indeterminate until init() creates the tasks. If the health path runs earlier, or task creation fails, this becomes an invalid FreeRTOS handle read on a monitoring code path.

Suggested fix

-    bool isTaskHealthy() const {
-        return is_task_healthy(eTaskGetState(taskHandle)) && is_task_healthy(eTaskGetState(profileTaskHandle));
-    }
+    bool isTaskHealthy() const {
+        return taskHandle != nullptr && profileTaskHandle != nullptr &&
+               is_task_healthy(eTaskGetState(taskHandle)) &&
+               is_task_healthy(eTaskGetState(profileTaskHandle));
+    }
...
-    TaskHandle_t taskHandle;
+    TaskHandle_t taskHandle = nullptr;
...
-    TaskHandle_t profileTaskHandle;
+    TaskHandle_t profileTaskHandle = nullptr;

Also applies to: 146-148

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/ui/default/DefaultUI.h` around lines 55 - 57, isTaskHealthy()
currently calls eTaskGetState() on taskHandle and profileTaskHandle
unconditionally, which can read indeterminate FreeRTOS handles before init()
creates them; update isTaskHealthy() (and the similar checks around the 146-148
block) to first verify each handle is initialized/valid (e.g., non-null/valid
FreeRTOS handle) before calling eTaskGetState, returning false or treating
uninitialized handles as unhealthy if not yet created; reference the taskHandle
and profileTaskHandle members and the init() path so you only call
eTaskGetState(taskHandle) or eTaskGetState(profileTaskHandle) when those handles
are known to be initialized.

src/display/drivers/AmoledDisplayDriver.cpp (1)

68-90: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore lcd_en on failed probes.

isCompatible() tries multiple hardware configs back-to-back, but testHw() now drives hwConfig.lcd_en HIGH before probing and never undoes that on a false return. A failed first probe can therefore change the electrical state seen by the later probes and bias auto-detection.

Suggested cleanup path

 bool AmoledDisplayDriver::testHw(AmoledHwConfig hwConfig) {
+    auto cleanupProbeState = [&]() {
+        Wire.end();
+        if (hwConfig.lcd_en != -1) {
+            digitalWrite(hwConfig.lcd_en, LOW);
+        }
+    };
+
     // Cold-boot peripherals need a moment; also recover from any prior
     // Wire.end()/begin() that left hal-i2c-ng in ESP_ERR_INVALID_STATE.
     Wire.end();
@@
-    if (!Wire.begin(hwConfig.i2c_sda, hwConfig.i2c_scl)) {
-        return false;
-    }
+    if (!Wire.begin(hwConfig.i2c_sda, hwConfig.i2c_scl)) {
+        cleanupProbeState();
+        return false;
+    }
@@
-    Wire.end();
-    return pcf8563Found && touchFound;
+    const bool compatible = pcf8563Found && touchFound;
+    if (!compatible) {
+        cleanupProbeState();
+    } else {
+        Wire.end();
+    }
+    return compatible;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/drivers/AmoledDisplayDriver.cpp` around lines 68 - 90, testHw()
currently drives hwConfig.lcd_en HIGH before probing and returns false on
failure without restoring that pin, which can bias subsequent isCompatible()
probes; fix by saving whether you asserted the pin (hwConfig.lcd_en != -1), and
on any early return path (probe failure) ensure you undo the assertion
(digitalWrite(hwConfig.lcd_en, LOW) or restore the prior state) and call
Wire.end() before returning; update testHw() so both success and failure paths
clean up the lcd_en pin and I2C (use the same
detectI2CDevice/PCF8563_DEVICE_ADDRESS, CST92XX_DEVICE_ADDRESS,
FT3168_DEVICE_ADDRESS symbols to find the probe logic).

🧹 Nitpick comments (3)

CMakeLists.txt (1)

1-3: Clarify the IDF_PATH requirement for non-PlatformIO CMake runs

CMakeLists.txt includes include($ENV{IDF_PATH}/tools/cmake/project.cmake), so IDF_PATH must be set when CMake is invoked. With PlatformIO (framework = arduino, espidf), PlatformIO sets IDF_PATH for the ESP-IDF CMake configure step, so PlatformIO builds shouldn’t require users to export it manually. The repo docs don’t mention IDF_PATH, so add a brief note for any workflow that runs ESP-IDF CMake outside PlatformIO.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CMakeLists.txt` around lines 1 - 3, Add a brief note to the repo docs
explaining that CMake requires the IDF_PATH environment variable when invoking
the ESP-IDF CMake include (the include($ENV{IDF_PATH}/tools/cmake/project.cmake)
line in CMakeLists.txt), and that while PlatformIO sets IDF_PATH automatically
for framework = arduino, espidf builds, users running CMake/ESP-IDF directly
must export IDF_PATH (or source the ESP-IDF export script) before running cmake;
mention the exact env var name IDF_PATH and a short example command to set it.

sdkconfig.controller.defaults (1)

5-11: ⚡ Quick win

Explicitly disable the unused NimBLE roles for the controller build.

Lines 5-11 reduce connections/activity slots, but the controller still inherits central/observer/broadcaster from sdkconfig.common.defaults. If this target is really peripheral-only, leaving those roles enabled keeps unused NimBLE host code and memory in the controller image.

♻️ Proposed fix

 # 1 connection: single display consumer.
 CONFIG_BT_NIMBLE_MAX_CONNECTIONS=1
+
+# Controller is peripheral-only; drop the inherited display-side roles.
+# CONFIG_BT_NIMBLE_ROLE_CENTRAL is not set
+# CONFIG_BT_NIMBLE_ROLE_OBSERVER is not set
+# CONFIG_BT_NIMBLE_ROLE_BROADCASTER is not set
 
 # 2 BLE link-layer activity slots: 1 peripheral connection + 1 advertising set.
 # Common base leaves the default (6), sized for the display's central + peripheral
 # workload. Narrowing it here reclaims ~1 KB static BSS.
 CONFIG_BT_CTRL_BLE_MAX_ACT=2

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sdkconfig.controller.defaults` around lines 5 - 11, The controller currently
reduces connections/activity but still inherits central/observer/broadcaster
roles; explicitly disable those unused NimBLE roles by adding the corresponding
CONFIG flags (e.g., disable CONFIG_BT_NIMBLE_ROLE_CENTRAL,
CONFIG_BT_NIMBLE_ROLE_OBSERVER and CONFIG_BT_NIMBLE_ROLE_BROADCASTER or the
controller-equivalent config names) in the defaults so the build omits
central/observer/broadcaster code and reclaims memory; update the sdkconfig
defaults near CONFIG_BT_NIMBLE_MAX_CONNECTIONS and CONFIG_BT_CTRL_BLE_MAX_ACT to
set those role flags to disabled/0/“n”.

src/CMakeLists.txt (1)

3-11: ⚡ Quick win

Consider replacing GLOB_RECURSE with explicit source lists.

GLOB_RECURSE is discouraged by CMake best practices because it does not automatically trigger reconfiguration when source files are added or removed, which can lead to stale builds.

Alternative approach

Explicitly list source files or use a pattern that triggers reconfiguration:

set(app_sources
    ${CMAKE_SOURCE_DIR}/src/controller/main.cpp
    ${CMAKE_SOURCE_DIR}/src/controller/other.cpp
    # ... list all files
)

Alternatively, document the limitation and ensure developers run a clean reconfigure when adding files.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/CMakeLists.txt` around lines 3 - 11, Replace the FILE(GLOB_RECURSE ...)
usage for app_sources with an explicit source list to avoid stale builds: remove
the two FILE(GLOB_RECURSE app_sources ...) blocks and instead define app_sources
via set(app_sources ...) listing each source file under src/controller/ or
src/display (refer to the app_sources variable and the FILE(GLOB_RECURSE)
invocations to locate the change), or if you cannot enumerate files, add a clear
comment documenting the GLOB_RECURSE limitation and require developers to run a
clean reconfigure when adding/removing files so builds stay correct.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/display/plugins/HomekitPlugin.cpp`:
- Around line 158-167: The loop() currently reads and later clears the
cross-task flag impl->actionRequired via a load-then-store sequence which can
drop a new request; change the code to atomically consume the flag using
impl->actionRequired.exchange(false) (or the equivalent
std::atomic<bool>::exchange) at the start of HomekitPlugin::loop() and only
proceed when the exchanged value was true, then continue using impl->controller
and impl->accessory as before (i.e., call deactivateStandby/activateStandby and
setTargetTemp) so any action arriving during processing is preserved.

In `@src/display/plugins/WebUIPlugin.cpp`:
- Around line 93-102: When apMode is true the code skips calling
ota->checkForUpdates() but also skips updating the UI status, leaving the UI
stuck in "Checking..."; modify the apMode branch so that even when skipping
network checks you explicitly resolve the OTA UI by calling
pluginManager->trigger("ota:update:status", "value", false) (or otherwise
indicate no update available) and call updateOTAStatus(ota->getCurrentVersion())
so handleOTASettings() is not left waiting; keep the existing behavior for the
non-AP branch that uses ota->checkForUpdates(), ota->isUpdateAvailable(), and
updateOTAStatus().

---

Outside diff comments:
In `@lib/ble_ota_dfu/src/ble_ota_dfu.cpp`:
- Around line 201-209: The code truncates FLASH.totalBytes() and
FLASH.usedBytes() by storing them in uint16_t (total_size, used_size) before
packing into flash_size; change those locals to uint32_t (e.g., uint32_t
total_size = FLASH.totalBytes(); uint32_t used_size = FLASH.usedBytes();) and
keep the packing into flash_size using the same byte shifts and
static_cast<uint8_t> so the three high/med/low bytes are encoded correctly
(refer to total_size, used_size, FLASH.totalBytes(), FLASH.usedBytes(), and the
flash_size array).

In `@lib/NanoPbComm/src/ble/BleServerTransport.cpp`:
- Around line 46-51: The transport currently marks connection usable as soon as
onConnect() sets _connected, but send(const uint8_t *data, size_t length)
returns true even if _txChar->notify() isn't ready; change the logic so the
usable/connected state is gated by TX-notify readiness (e.g., track a _txReady
flag set when the subscribe/notify-ready callback runs) and update
onConnect()/subscribe handler to only set _connected (or set _txReady and then
_connected) when notifications are actually available; in
BleServerTransport::send, check both _connected/_txChar and the new _txReady,
call _txChar->setValue(...); return the actual result of _txChar->notify() (or
its boolean) instead of always returning true so callers see real failure when
notify can't hand off the packet.

In `@lib/OTA/src/ControllerOTA.cpp`:
- Around line 141-163: The function fillBuffer currently returns early on
timeout but provides no signal to callers (fillBuffer) leading downloadFile and
sendPart to assume the full len/ bufferSize was filled and proceed; change
fillBuffer(const Stream &in, uint8_t *buffer, uint16_t len) to return a status
(e.g., bool success or size_t bytesRead) and ensure it returns false or the
actual bytesRead when timeout_failures triggers (using bufferLen); update
callers downloadFile and sendPart to check the return value (or bytes read) and
abort/handle the short-read (do not write/transmit more than the returned bytes,
and surface an error) so partial reads cannot corrupt the staged firmware image,
referencing function names fillBuffer, downloadFile, sendPart and the variables
len, bufferLen, bufferSize, timeout_failures.
- Around line 27-32: Abort the OTA flow when the download or staged file open
fails: if downloadFile(wifi_client, release_url) returns false, log the error
and immediately return/abort instead of continuing; after attempting File file =
SPIFFS.open("/board-firmware.bin", FILE_READ), check that the File is valid
(truthy) and file.size() > 0, log an error and return/abort if it isn’t, and
only call runUpdate(file, file.size()) when both checks pass. Ensure the changes
touch the downloadFile check, the SPIFFS.open/File validity check, and the
runUpdate call so no further OTA steps run on failure.
- Around line 209-210: ControllerOTA::onReceive and
BLEOverTheAirDeviceFirmwareUpdate::onWrite currently assume pData has enough
bytes and read past the buffer; add explicit length guards: in
ControllerOTA::onReceive check length >= 1 before reading pData[0]
(return/ignore if not), and in BLEOverTheAirDeviceFirmwareUpdate::onWrite
inspect the first byte (command) only after verifying length >= 1 and then
validate length against the expected fixed payload size for commands 0xFC, 0xFE,
and 0xFF (e.g., require N bytes for each command) before reading additional
indexes; on any undersize packet, safely bail out (log/ignore) instead of
dereferencing out-of-bounds data.

In `@lib/OTA/src/GitHubOTA.cpp`:
- Around line 41-74: checkForUpdates() calls
get_updated_base_url_via_redirect(_wifi_client, _release_url) and
get_updated_version_via_txt_file(_wifi_client, _latest_url) without re-attaching
the CA bundle; before the first network call in GitHubOTA::checkForUpdates, call
attach_ca_bundle(_wifi_client) (or move attach_ca_bundle into the shared HTTPS
helpers) so _wifi_client has the trust store attached for both
get_updated_base_url_via_redirect and get_updated_version_via_txt_file; ensure
this is done before any early returns so both code paths use the attached CA
bundle.

In `@src/display/core/Controller.cpp`:
- Around line 410-415: The code calls
configTzTime(resolve_timezone(settings.getTimezone()), NTP_SERVER) which already
sets TZ (via setenv+tzset), configures SNTP servers and calls sntp_init, so
remove the redundant esp_sntp_setservername(0, NTP_SERVER) and esp_sntp_init()
calls to avoid re-initializing SNTP and order-dependent behavior; keep the
configTzTime + setenv + tzset flow (or if you prefer manual control remove
configTzTime and replace it with an explicit
esp_sntp_setservername/esp_sntp_init sequence), and ensure only one SNTP
initialization path (referencing configTzTime, esp_sntp_setservername,
esp_sntp_init, setenv, tzset).

In `@src/display/core/Settings.cpp`:
- Around line 119-129: The async save task creation result from xTaskCreate in
Settings::load is ignored causing taskHandle to remain null on low-memory boots
and later save() calls only flip dirty without flushing; update Settings::load
to check xTaskCreate's return (compare to pdPASS) and if creation fails set
taskHandle = nullptr and immediately fall back to a synchronous persist (call
doSave()) or mark a retry strategy; additionally update Settings::save to detect
a null/invalid taskHandle and perform doSave() (or a safe immediate write)
instead of only setting dirty so settings still persist even if the async task
couldn't be created; reference xTaskCreate, Settings::load, Settings::save,
taskHandle, doSave, dirty and Controller::setup when making the change.

In `@src/display/drivers/AmoledDisplayDriver.cpp`:
- Around line 68-90: testHw() currently drives hwConfig.lcd_en HIGH before
probing and returns false on failure without restoring that pin, which can bias
subsequent isCompatible() probes; fix by saving whether you asserted the pin
(hwConfig.lcd_en != -1), and on any early return path (probe failure) ensure you
undo the assertion (digitalWrite(hwConfig.lcd_en, LOW) or restore the prior
state) and call Wire.end() before returning; update testHw() so both success and
failure paths clean up the lcd_en pin and I2C (use the same
detectI2CDevice/PCF8563_DEVICE_ADDRESS, CST92XX_DEVICE_ADDRESS,
FT3168_DEVICE_ADDRESS symbols to find the probe logic).

In `@src/display/ui/default/DefaultUI.h`:
- Around line 55-57: isTaskHealthy() currently calls eTaskGetState() on
taskHandle and profileTaskHandle unconditionally, which can read indeterminate
FreeRTOS handles before init() creates them; update isTaskHealthy() (and the
similar checks around the 146-148 block) to first verify each handle is
initialized/valid (e.g., non-null/valid FreeRTOS handle) before calling
eTaskGetState, returning false or treating uninitialized handles as unhealthy if
not yet created; reference the taskHandle and profileTaskHandle members and the
init() path so you only call eTaskGetState(taskHandle) or
eTaskGetState(profileTaskHandle) when those handles are known to be initialized.

---

Nitpick comments:
In `@CMakeLists.txt`:
- Around line 1-3: Add a brief note to the repo docs explaining that CMake
requires the IDF_PATH environment variable when invoking the ESP-IDF CMake
include (the include($ENV{IDF_PATH}/tools/cmake/project.cmake) line in
CMakeLists.txt), and that while PlatformIO sets IDF_PATH automatically for
framework = arduino, espidf builds, users running CMake/ESP-IDF directly must
export IDF_PATH (or source the ESP-IDF export script) before running cmake;
mention the exact env var name IDF_PATH and a short example command to set it.

In `@sdkconfig.controller.defaults`:
- Around line 5-11: The controller currently reduces connections/activity but
still inherits central/observer/broadcaster roles; explicitly disable those
unused NimBLE roles by adding the corresponding CONFIG flags (e.g., disable
CONFIG_BT_NIMBLE_ROLE_CENTRAL, CONFIG_BT_NIMBLE_ROLE_OBSERVER and
CONFIG_BT_NIMBLE_ROLE_BROADCASTER or the controller-equivalent config names) in
the defaults so the build omits central/observer/broadcaster code and reclaims
memory; update the sdkconfig defaults near CONFIG_BT_NIMBLE_MAX_CONNECTIONS and
CONFIG_BT_CTRL_BLE_MAX_ACT to set those role flags to disabled/0/“n”.

In `@src/CMakeLists.txt`:
- Around line 3-11: Replace the FILE(GLOB_RECURSE ...) usage for app_sources
with an explicit source list to avoid stale builds: remove the two
FILE(GLOB_RECURSE app_sources ...) blocks and instead define app_sources via
set(app_sources ...) listing each source file under src/controller/ or
src/display (refer to the app_sources variable and the FILE(GLOB_RECURSE)
invocations to locate the change), or if you cannot enumerate files, add a clear
comment documenting the GLOB_RECURSE limitation and require developers to run a
clean reconfigure when adding/removing files so builds stay correct.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5bb3f5b4-691c-4c8a-b26f-125dd8ffd111

📥 Commits

Reviewing files that changed from the base of the PR and between af5106a and 7c8c800.

⛔ Files ignored due to path filters (2)

• partitions/default_16mb.csv is excluded by !**/*.csv
• partitions/default_8mb.csv is excluded by !**/*.csv

📒 Files selected for processing (61)

• .gitignore
• CMakeLists.txt
• boards/esp32-s3-supermini.json
• lib/GaggiMateController/library.json
• lib/GaggiMateController/src/peripherals/DigitalInput.h
• lib/GaggiMateController/src/peripherals/DimmedPump.h
• lib/GaggiMateController/src/peripherals/DistanceSensor.h
• lib/GaggiMateController/src/peripherals/Heater.h
• lib/GaggiMateController/src/peripherals/Max31855Thermocouple.h
• lib/GaggiMateController/src/peripherals/PressureSensor.h
• lib/GaggiMateController/src/peripherals/Pump.h
• lib/GaggiMateController/src/peripherals/SimplePump.h
• lib/GaggiMateController/src/peripherals/TemperatureSensor.h
• lib/NanoPbComm/library.json
• lib/NanoPbComm/src/ble/BleClientTransport.cpp
• lib/NanoPbComm/src/ble/BleClientTransport.h
• lib/NanoPbComm/src/ble/BleServerTransport.cpp
• lib/NanoPbComm/src/ble/BleServerTransport.h
• lib/OTA/library.properties
• lib/OTA/src/ControllerOTA.cpp
• lib/OTA/src/GitHubOTA.cpp
• lib/OTA/src/GitHubOTA.h
• lib/OTA/src/common.cpp
• lib/OTA/src/common.h
• lib/ble_ota_dfu/library.properties
• lib/ble_ota_dfu/src/ble_ota_dfu.cpp
• lib/ble_ota_dfu/src/ble_ota_dfu.hpp
• platformio.ini
• scripts/assert_sdkconfig.py
• scripts/pioarduino_env.py
• sdkconfig.common.defaults
• sdkconfig.controller.defaults
• sdkconfig.gaggimate.defaults
• src/CMakeLists.txt
• src/display/core/Controller.cpp
• src/display/core/Controller.h
• src/display/core/MemoryMonitor.cpp
• src/display/core/MemoryMonitor.h
• src/display/core/Settings.cpp
• src/display/core/Settings.h
• src/display/core/utils.cpp
• src/display/core/utils.h
• src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp
• src/display/drivers/AmoledDisplay/CO5300.cpp
• src/display/drivers/AmoledDisplay/CO5300.h
• src/display/drivers/AmoledDisplayDriver.cpp
• src/display/drivers/Driver.h
• src/display/drivers/LilyGo-T-RGB/LilyGo_RGBPanel.cpp
• src/display/drivers/Waveshare/WavesharePanel.cpp
• src/display/plugins/AutoWakeupPlugin.cpp
• src/display/plugins/BLEScalePlugin.cpp
• src/display/plugins/BLEScalePlugin.h
• src/display/plugins/HomekitPlugin.cpp
• src/display/plugins/HomekitPlugin.h
• src/display/plugins/MQTTPlugin.cpp
• src/display/plugins/ShotHistoryPlugin.h
• src/display/plugins/WebUIPlugin.cpp
• src/display/plugins/WebUIPlugin.h
• src/display/ui/default/DefaultUI.cpp
• src/display/ui/default/DefaultUI.h
• src/idf_component.yml

💤 Files with no reviewable changes (2)

• boards/esp32-s3-supermini.json
• lib/GaggiMateController/library.json

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Consume actionRequired with exchange(false).

This is a cross-task flag, but the current load-then-store sequence can drop a new HomeKit action that arrives while loop() is processing. If the callback sets actionRequired = true between Line 159 and Line 167, Line 167 clears the newer request and the latest HomeKit change is never applied.

Suggested atomic handoff

 void HomekitPlugin::loop() {
-    if (!impl->actionRequired || impl->controller == nullptr || impl->accessory == nullptr)
+    if (impl->controller == nullptr || impl->accessory == nullptr)
         return;
+    if (!impl->actionRequired.exchange(false))
+        return;
     if (impl->accessory->getState() && impl->controller->getMode() == MODE_STANDBY) {
         impl->controller->deactivateStandby();
     } else if (!impl->accessory->getState() && impl->controller->getMode() != MODE_STANDBY) {
         impl->controller->activateStandby();
     }
     impl->controller->setTargetTemp(impl->accessory->getTargetTemperature());
-    impl->actionRequired = false;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/plugins/HomekitPlugin.cpp` around lines 158 - 167, The loop()
currently reads and later clears the cross-task flag impl->actionRequired via a
load-then-store sequence which can drop a new request; change the code to
atomically consume the flag using impl->actionRequired.exchange(false) (or the
equivalent std::atomic<bool>::exchange) at the start of HomekitPlugin::loop()
and only proceed when the exchanged value was true, then continue using
impl->controller and impl->accessory as before (i.e., call
deactivateStandby/activateStandby and setTargetTemp) so any action arriving
during processing is preserved.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Resolve OTA status explicitly in AP mode.

Skipping the network check here is fine, but this branch now suppresses the follow-up status update too. handleOTASettings() still puts the UI into a "Checking..." state, so in AP mode a manual check never resolves until the network mode changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/plugins/WebUIPlugin.cpp` around lines 93 - 102, When apMode is
true the code skips calling ota->checkForUpdates() but also skips updating the
UI status, leaving the UI stuck in "Checking..."; modify the apMode branch so
that even when skipping network checks you explicitly resolve the OTA UI by
calling pluginManager->trigger("ota:update:status", "value", false) (or
otherwise indicate no update available) and call
updateOTAStatus(ota->getCurrentVersion()) so handleOTASettings() is not left
waiting; keep the existing behavior for the non-AP branch that uses
ota->checkForUpdates(), ota->isUpdateAvailable(), and updateOTAStatus().

[cla-bot]

We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added.

[sonarqubecloud]

Quality Gate passed

Issues
43 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/display/core/Controller.cpp (1)

340-344: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Extract duplicated config-sending sequence into a helper method.

The config burst (setPressureScale → parse PID → sendPidSettings → setPumpModelCoeffs) is duplicated verbatim in onSystemInfo (lines 340-344) and loop (lines 455-459). If a new config parameter is added, one site can easily be missed. Extract a sendControllerConfig() method and call it from both locations.

♻️ Proposed refactor

 // In Controller.h (private section):
+void sendControllerConfig();

 // In Controller.cpp:
+void Controller::sendControllerConfig() {
+    setPressureScale();
+    float pid[4];
+    parseFloatCsv(settings.getPid(), pid, 4, 0.0f);
+    comms.sendPidSettings(pid[0], pid[1], pid[2], pid[3]);
+    setPumpModelCoeffs();
+}

 void Controller::onSystemInfo(const char *hardware, const char *version, uint32_t protocolVersion, bool dimming, bool pressure,
                               bool ledControl, bool tof) {
     // ...
     } else {
-        setPressureScale();
-        float pid[4];
-        parseFloatCsv(settings.getPid(), pid, 4, 0.0f);
-        comms.sendPidSettings(pid[0], pid[1], pid[2], pid[3]);
-        setPumpModelCoeffs();
+        sendControllerConfig();
         configResendUntil = millis() + CONFIG_RESEND_WINDOW_MS;
         lastConfigResend = millis();
     }

 // In loop():
-    if (comms.isConnected() && now < configResendUntil && (now - lastConfigResend) >= CONFIG_RESEND_INTERVAL_MS) {
-        setPressureScale();
-        float pid[4];
-        parseFloatCsv(settings.getPid(), pid, 4, 0.0f);
-        comms.sendPidSettings(pid[0], pid[1], pid[2], pid[3]);
-        setPumpModelCoeffs();
+    if (comms.isConnected() && now < configResendUntil && (now - lastConfigResend) >= CONFIG_RESEND_INTERVAL_MS) {
+        sendControllerConfig();
         lastConfigResend = now;
     }

Also applies to: 455-459

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/display/core/Controller.cpp` around lines 340 - 344, Extract the
duplicated configuration sequence from onSystemInfo and loop into a
sendControllerConfig() helper method. Move setPressureScale, PID parsing,
comms.sendPidSettings, and setPumpModelCoeffs into the helper, then replace both
call sites with sendControllerConfig() while preserving their existing behavior
and order.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/display/core/Controller.cpp`:
- Around line 340-344: Extract the duplicated configuration sequence from
onSystemInfo and loop into a sendControllerConfig() helper method. Move
setPressureScale, PID parsing, comms.sendPidSettings, and setPumpModelCoeffs
into the helper, then replace both call sites with sendControllerConfig() while
preserving their existing behavior and order.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 18b34811-7b26-467f-9214-1b05fef96ba6

📥 Commits

Reviewing files that changed from the base of the PR and between 7c8c800 and 6b95001.

📒 Files selected for processing (2)

• src/display/core/Controller.cpp
• src/display/core/Controller.h

🚧 Files skipped from review as they are similar to previous changes (1)

• src/display/core/Controller.h