New logging and more #446
New logging and more #446
Conversation
📝 WalkthroughWalkthroughThe PR replaces the legacy TT_LOG_/TAG macros with a new tt::Logger API and LoggerAdapter system (Logger, LoggerAdapter, LoggerAdapterEsp/Generic/Shared, LoggerSettings), removes Log.h/LogEsp.h/LogSimulator, and updates logging calls across the codebase to Logger methods with {}-style formatting. It adds LoggerAdapter headers and platform adapters. Notable API/behavior changes: default timezone changed to Europe/Amsterdam; http::download callbacks now passed by const reference; EspHttpClient gained isStatusCodeOk() and getContentLength(); i2c::init signature changed; unsubscribeReceiver parameter type simplified; TpagerEncoder init/deinit and Drv2605 constructor signatures updated. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
Tactility/Source/service/development/DevelopmentService.cpp (1)
120-122: Pre-existing bug:return falseshould bereturn ESP_FAIL.This return statement returns
false(evaluates to0, which equalsESP_OK) instead ofESP_FAIL, incorrectly signaling success when the boundary parsing fails.🔎 Proposed fix
if (!network::getMultiPartBoundaryOrSendError(request, boundary)) { - return false; + return ESP_FAIL; }Tactility/Source/network/HttpdReq.cpp (1)
85-94: Minor: Incorrect calculation in warning log.Line 89 calculates
bytesRead + bytes_receivedwhenbytes_received <= 0. Sincebytes_receivedcan be negative (error) or zero, this calculation may produce misleading values. The log should reportbytesRead(actual bytes successfully received) instead.🔎 Proposed fix
size_t bytes_received = httpd_req_recv(request, buffer + bytesRead, read_size); if (bytes_received <= 0) { - LOGGER.warn("Received {} / {}", bytesRead + bytes_received, length); + LOGGER.warn("Received {} / {} bytes before error", bytesRead, length); return nullptr; }Tactility/Source/settings/SystemSettings.cpp (1)
18-19: Protect cache access with the mutex to prevent race conditions.The
mutexdeclared at line 17 is never used. The caching logic inloadSystemSettings(lines 68-75) andsaveSystemSettings(lines 93-94) access shared static state without synchronization. With concurrent calls fromTime.cpp,Language.cpp,TimeDateSettings.cpp, andLocaleSettings.cpp, multiple threads can race on thecachedflag andcachedSettingsvariable, causing missed cache updates, double-loading, or inconsistent state.Wrap the cache check and update operations with
mutex.lock()andmutex.unlock()(or equivalent RAII guard).Tactility/Source/kernel/SystemEvents.cpp (2)
59-71: Inconsistent mutex error handling across event functions.
publishSystemEventsilently drops the event if the mutex cannot be acquired, whilesubscribeSystemEventcallstt_crash(). This inconsistency could lead to subtle bugs where events are silently lost without any indication.Consider either logging an error or using a consistent approach across all three functions. Additionally, using a scoped lock (like
asScopedLock()used elsewhere in the codebase) would be safer against future early-return bugs.🔎 Suggested improvement using scoped lock
void publishSystemEvent(SystemEvent event) { LOGGER.info("{}", getEventName(event)); - if (mutex.lock(MAX_TICKS)) { + auto lock = mutex.asScopedLock(); + if (lock.lock(MAX_TICKS)) { for (auto& subscription : subscriptions) { if (subscription.event == event) { subscription.handler(event); } } - - mutex.unlock(); + } else { + LOGGER.error("Failed to acquire mutex for publishing event"); } }
90-97: Silent failure inunsubscribeSystemEvent.If the mutex cannot be acquired, the subscription silently remains active. The caller has no way to know the unsubscription failed, which could lead to unexpected callbacks or resource leaks.
Consider returning a boolean to indicate success/failure, or at minimum logging a warning.
🔎 Suggested improvement
-void unsubscribeSystemEvent(SystemEventSubscription subscription) { +bool unsubscribeSystemEvent(SystemEventSubscription subscription) { if (mutex.lock(MAX_TICKS)) { std::erase_if(subscriptions, [subscription](auto& item) { return (item.id == subscription); }); mutex.unlock(); + return true; + } else { + LOGGER.error("Failed to acquire mutex for unsubscribing"); + return false; } }Tactility/Source/app/AppRegistration.cpp (1)
33-40: Potential bug:lock.lock()return value not checked.The scoped lock's
lock()method may fail and returnfalse, but the code proceeds to erase regardless. This could lead to data races if the mutex isn't actually held.Additionally, if
asScopedLock()creates an unlocked scoped lock that requires explicitlock(), consider checking the return value before proceeding.🔎 Suggested fix
bool removeAppManifest(const std::string& id) { LOGGER.info("Removing manifest for {}", id); auto lock = hash_mutex.asScopedLock(); - lock.lock(); - - return app_manifest_map.erase(id) == 1; + if (lock.lock()) { + return app_manifest_map.erase(id) == 1; + } + LOGGER.error("Failed to acquire mutex for removing manifest"); + return false; }Tactility/Source/hal/gps/Ublox.cpp (1)
96-96: Incomplete logger migration in GPS_DEBUG sections.The GPS_DEBUG conditional sections still use legacy
TT_LOG_IandTT_LOG_Wmacros, which are inconsistent with the rest of the migration. These should also be converted to useLOGGER.infoandLOGGER.warn.🔎 Proposed fix
#ifdef GPS_DEBUG - TT_LOG_I(TAG, "%s", debugmsg.c_str()); + LOGGER.info("{}", debugmsg); #endifApply similar changes to lines 111 and 121-122.
Also applies to: 111-111, 121-122
Tactility/Source/service/gps/GpsConfiguration.cpp (1)
109-111: Variableconfigurationshadows the function parameter.The loop variable
configurationshadows the parameter from line 85. This can lead to confusion or bugs if the original parameter is needed inside the loop. Consider renaming the loop variable.🔎 Proposed fix
- for (auto& configuration : configurations) { - writer.write(&configuration); + for (auto& config : configurations) { + writer.write(&config); }
🟡 Minor comments (5)
Tactility/Source/service/wifi/WifiApSettings.cpp-105-105 (1)
105-105: Remove extraneous 's' from format string.The format string contains
\"{}s\"which appears to be a typo from converting the printf-style%sformat specifier to std::format's{}. The 's' after the placeholder is literal text that will be printed, resulting in output likeFailed to decrypt credentials for "<ssid>s": <code>.🔎 Proposed fix
- LOGGER.error("Failed to decrypt credentials for \"{}s\": {}", ssidInput, decrypt_result); + LOGGER.error("Failed to decrypt credentials for \"{}\": {}", ssidInput, decrypt_result);Tactility/Source/settings/Time.cpp-58-58 (1)
58-58: Update POSIX timezone code to explicit standard format.The current code
CET-1CEST,M3.5.0,M10.5.0/3is functionally correct but should be updated toCET-1CEST,M3.5.0/2,M10.5.0/3to match the standard POSIX format for Europe/Amsterdam per IANA tzdb and PostgreSQL documentation. The/2explicitly specifies that DST starts at 02:00 (currently omitted because it's the default).Tactility/Source/hal/uart/Uart.cpp-132-132 (1)
132-132: Use consistent std::format-style placeholder.This line uses printf-style formatting (
%swith.c_str()) instead of the std::format-style{}placeholders used consistently elsewhere in this file and across the codebase.🔎 Proposed fix
- LOGGER.info("Open %s", name.c_str()); + LOGGER.info("Open {}", name);Tactility/Source/hal/gps/Ublox.cpp-263-263 (1)
263-263: Inconsistent log formatting.This log statement uses C-style
%sformatting instead of the modern{}placeholder used elsewhere in the file.🔎 Proposed fix
- LOGGER.info(" %s", ublox_info.extension[i]); + LOGGER.info(" {}", ublox_info.extension[i]);Tactility/Source/app/AppManifestParsing.cpp-11-13 (1)
11-13:constexpris incompatible withstd::function.
std::functionis not a literal type and its constructor is notconstexpr, so this function cannot be evaluated at compile time. Either removeconstexpror replacestd::functionwith a template parameter for the predicate.🔎 Proposed fix using a template parameter
-constexpr bool validateString(const std::string& value, const std::function<bool(char)>& isValidChar) { +template <typename Predicate> +bool validateString(const std::string& value, Predicate isValidChar) { return std::ranges::all_of(value, isValidChar); }
🧹 Nitpick comments (25)
Tactility/Source/file/ObjectFileReader.cpp (1)
14-14: Consider removing unnecessary.c_str()for consistency.Line 14 uses
filePath.c_str()while all other logging calls in this file (lines 20, 25, 30, 36, 41) usefilePathdirectly. Sincestd::formathandlesstd::stringnatively, the.c_str()call is unnecessary and creates an inconsistency within the file.🔎 Suggested fix for consistency
- LOGGER.error("Failed to open file {}", filePath.c_str()); + LOGGER.error("Failed to open file {}", filePath);Tactility/Source/service/development/DevelopmentSettings.cpp (1)
40-45: Consider propagating error status to caller.The function logs errors but doesn't return the save status to the caller. Returning
boolwould allow callers to handle failures appropriately.🔎 Proposed refactor
-void setEnableOnBoot(bool enable) { +bool setEnableOnBoot(bool enable) { DevelopmentSettings properties { .enableOnBoot = enable }; if (!save(properties)) { LOGGER.error("Failed to save {}", SETTINGS_FILE); + return false; } + return true; }Tactility/Source/hal/usb/Usb.cpp (1)
59-60: Consider more specific error messages for consistency.The error messages at lines 59 and 67 are less specific than their counterparts in
startMassStorageWithFlash()(lines 100 and 108). For better debuggability and consistency across similar functions, consider specifying "SDMMC" in these messages.🔎 Proposed refactor for clarity
- LOGGER.error("Can't start"); + LOGGER.error("Can't start SDMMC mass storage");- LOGGER.error("Failed to init mass storage"); + LOGGER.error("Failed to init SDMMC mass storage");Also applies to: 67-68
Tactility/Source/lvgl/Lvgl.cpp (1)
172-172: Consider usingconst auto&for consistency.The iteration loops in
stop()use plainauto, while the corresponding loops instart()(lines 59, 85, 99, 114) useconst auto&. Since these device pointers aren't modified during iteration, usingconst auto&would be more consistent and avoid unnecessary pointer copies.🔎 Proposed refactor for consistent iteration
- for (auto keyboard : keyboards) { + for (const auto& keyboard : keyboards) { if (keyboard->getLvglIndev() != nullptr) { keyboard->stopLvgl(); }- for (auto touch_device : touch_devices) { + for (const auto& touch_device : touch_devices) { if (touch_device->getLvglIndev() != nullptr) { touch_device->stopLvgl(); }- for (auto encoder_device : encoder_devices) { + for (const auto& encoder_device : encoder_devices) { if (encoder_device->getLvglIndev() != nullptr) { encoder_device->stopLvgl(); }- for (auto display : displays) { + for (const auto& display : displays) { if (display->supportsLvgl() && display->getLvglDisplay() != nullptr && !display->stopLvgl()) { LOGGER.error("Failed to detach display from LVGL"); }Also applies to: 183-183, 194-194, 203-203
Tactility/Source/i18n/TextResources.cpp (1)
63-63: Consider distinguishing between different failure scenarios.Both error logs use the same message "Couldn't find i18n data for {}" but represent different failure modes:
- Line 63: No translation file found at the expected path
- Line 72: Translation file found but contained no data
Consider using more specific messages to aid debugging:
🔎 Proposed fix to clarify error messages
auto file_path = getI18nDataFilePath(path); if (file_path.empty()) { - LOGGER.error("Couldn't find i18n data for {}", path); + LOGGER.error("No translation file found for {}", path); return false; } file::readLines(file_path, true, [&new_data](const char* line) { new_data.push_back(line); }); if (new_data.empty()) { - LOGGER.error("Couldn't find i18n data for {}", path); + LOGGER.error("Translation file is empty or couldn't be read: {}", path); return false; }Also applies to: 72-72
TactilityCore/Include/Tactility/LogSimulator.h (1)
8-10: Remove unused includes<cstdint>and<sys/time.h>from the header.These includes are not used by any declarations in this header file, nor are they used by the implementation in LogSimulator.cpp. Headers should only include what they directly need for their public interface. Moving unnecessary includes out reduces compilation dependencies.
<cstdint>: No integer type definitions are used in the header or implementation.<sys/time.h>: No time functions or types from this header are used.Tactility/Source/hal/i2c/I2c.cpp (2)
54-267: Consider a helper for port formatting.The pattern
static_cast<int>(port)is repeated throughout the file. While clear and explicit, you could optionally extract this to a small helper function to reduce duplication:💡 Optional refactor to reduce duplication
Add a helper function at the top of the namespace:
static int portToInt(i2c_port_t port) { return static_cast<int>(port); }Then logging calls would become:
LOGGER.error("({}) Cannot reconfigure while interface is started", portToInt(port));This is optional—the current explicit casting is also perfectly acceptable.
176-176: Pre-existing TODOs about timeout accuracy.There are several TODO comments noting that timeout values become inaccurate after mutex acquisition and other operations. These represent valid technical debt but are not introduced by this PR.
If you'd like, I can help generate a solution that tracks elapsed time and adjusts the timeout accordingly, or open an issue to track this improvement.
Also applies to: 221-221, 237-237, 273-273
Tactility/Source/service/statusbar/Statusbar.cpp (1)
5-5: LGTM: Logger migration is clean.The migration from TAG-based logging to the Logger class is straightforward and correct. The tag "StatusbarService" is descriptive.
Optional: Consider tag naming consistency
Note that the related file
Tactility/Source/lvgl/Statusbar.cppuses the tag "statusbar" (lowercase, shorter). While both tags work, using consistent naming patterns across related statusbar components would improve log filtering and debugging. Consider standardizing on either "StatusbarService" and "StatusbarLvgl" or "statusbar-service" and "statusbar-lvgl".Also applies to: 18-18
Tactility/Source/lvgl/Statusbar.cpp (1)
272-278: Optional: Simplify conditional logging.The conditional logic distinguishes between empty and non-empty image strings solely for logging purposes. Consider simplifying to reduce code complexity.
🔎 Suggested simplification
- if (LOGGER.isLoggingDebug()) { - if (image.empty()) { - LOGGER.debug("id {}: set image (none)", id); - } else { - LOGGER.debug("id {}: set image {}", id, image); - } - } + if (LOGGER.isLoggingDebug()) { + LOGGER.debug("id {}: set image {}", id, image.empty() ? "(none)" : image); + }Tactility/Source/network/HttpdReq.cpp (1)
179-196: Optional: Enhance error messages with diagnostic context.Lines 183 and 187 could include additional context for better observability:
- Line 183: Include bytes received so far and remaining bytes
- Line 187: Include actual bytes written vs expected
While the current messages are acceptable, adding these details would improve debugging when file reception fails.
🔎 Proposed enhancement
size_t receive_chunk_size = httpd_req_recv(request, buffer, expected_chunk_size); if (receive_chunk_size <= 0) { - LOGGER.error("Receive failed"); + LOGGER.error("Receive failed: {} / {} bytes received", bytes_received, length); break; } - if (fwrite(buffer, 1, receive_chunk_size, file) != receive_chunk_size) { - LOGGER.error("Failed to write all bytes"); + size_t bytes_written = fwrite(buffer, 1, receive_chunk_size, file); + if (bytes_written != receive_chunk_size) { + LOGGER.error("Failed to write all bytes: {} / {} written", bytes_written, receive_chunk_size); break; }Tactility/Source/service/espnow/EspNowWifi.cpp (2)
17-17: Verify the logger tag naming.The LOGGER is initialized with tag "EspNowService" but the file is
EspNowWifi.cpp. This could make it harder to trace which file generated specific log messages. Consider using "EspNowWifi" or verifying this naming is intentional.
73-73: Fix indentation inconsistency.These lines have an extra leading space before
LOGGER.error, making them inconsistent with the surrounding code indentation.🔎 Proposed fix
- LOGGER.error("esp_wifi_set_mode() failed"); + LOGGER.error("esp_wifi_set_mode() failed");- LOGGER.error("esp_wifi_start() failed"); + LOGGER.error("esp_wifi_start() failed");- LOGGER.error("esp_wifi_set_channel() failed"); + LOGGER.error("esp_wifi_set_channel() failed");Also applies to: 78-78, 84-84
Tactility/Source/service/displayidle/DisplayIdle.cpp (2)
22-59: Consider extracting the activity detection threshold as a named constant.The logic for dimming and restoring the display backlight is well-structured and includes proper LVGL locking. However, the magic number
100on line 53 (checkinginactive_ms < 100) would be clearer as a named constant likekActivityDetectionThresholdMs.🔎 Proposed refactor
+ static constexpr uint32_t kActivityDetectionThresholdMs = 100; + void tick() { // Settings are now cached and event-driven (no file I/O in timer callback!) // This prevents watchdog timeout from blocking the Timer Service task if (lv_disp_get_default() == nullptr) { return; } // Query LVGL inactivity once for both checks uint32_t inactive_ms = 0; if (lvgl::lock(100)) { inactive_ms = lv_disp_get_inactive_time(nullptr); lvgl::unlock(); } else { return; } // TODO: The following logic only works with the first display. There might be multiple displays. // Handle display backlight auto display = getDisplay(); if (display != nullptr && display->supportsBacklightDuty()) { // If timeout disabled, ensure backlight restored if we had dimmed it if (!cachedDisplaySettings.backlightTimeoutEnabled || cachedDisplaySettings.backlightTimeoutMs == 0) { if (displayDimmed) { display->setBacklightDuty(cachedDisplaySettings.backlightDuty); displayDimmed = false; } } else { if (!displayDimmed && inactive_ms >= cachedDisplaySettings.backlightTimeoutMs) { display->setBacklightDuty(0); displayDimmed = true; - } else if (displayDimmed && inactive_ms < 100) { + } else if (displayDimmed && inactive_ms < kActivityDetectionThresholdMs) { display->setBacklightDuty(cachedDisplaySettings.backlightDuty); displayDimmed = false; } } } }
62-74: Good implementation with settings caching to prevent watchdog timeouts.The approach of caching settings at startup (line 65) is excellent for avoiding file I/O in the timer callback, which prevents potential watchdog timeouts. The timer configuration with Lower priority is appropriate for this non-critical periodic task.
Optionally, consider extracting the
250millisecond timer interval as a named constant (e.g.,kTickIntervalMs) for better maintainability.Regarding the TODO on line 68 about dynamic settings updates: Would you like me to help design an event-driven approach using DisplaySettingsChanged events, or open a new issue to track this enhancement?
Tactility/Source/service/loader/Loader.cpp (2)
20-20: Consider aligning the logger tag with the service name.The logger is tagged as "Boot" while the service is named "Loader" (as seen in the manifest at line 325). If this is intentional because the loader is conceptually part of the boot process, it's fine. Otherwise, consider using "Loader" for consistency and easier log filtering.
🔎 Proposed fix if alignment is desired
-static const auto LOGGER = Logger("Boot"); +static const auto LOGGER = Logger("Loader");
238-242: Clean up the indentation for consistency.The opening string literal has unusual extra spacing. Consider aligning it with the rest of the file's formatting style.
🔎 Proposed fix
- LOGGER.info( "App \"{}\" state: {} -> {}", + LOGGER.info("App \"{}\" state: {} -> {}", app_manifest.appId, appStateToString(old_state), appStateToString(state) );TactilityCore/Include/Tactility/LoggerAdapter.h (1)
8-8: Consider modern C++usingsyntax.The typedef uses old C-style syntax. Modern C++ prefers
usingfor type aliases, which is more readable and consistent with template aliases.🔎 Proposed modernization
-typedef std::function<void(LogLevel level, const char* tag, const char* message)> LoggerAdapter; +using LoggerAdapter = std::function<void(LogLevel level, const char* tag, const char* message)>;TactilityCore/Include/Tactility/LoggerAdapterGeneric.h (1)
22-28: Consider using std::cout for consistency.While
printfworks, the codebase is transitioning to C++ idioms. Usingstd::coutinstead ofprintfwould be more consistent with the modern C++ approach seen throughout the logging migration.🔎 Optional refactor to use std::cout
static const LoggerAdapter genericLoggerAdapter = [](LogLevel level, const char* tag, const char* message) { constexpr auto COLOR_RESET = "\033[0m"; constexpr auto COLOR_GREY = "\033[37m"; std::stringstream buffer; buffer << COLOR_GREY << getLogTimestamp() << " [" << toTagColour(level) << toPrefix(level) << COLOR_GREY << "] [" << COLOR_RESET << tag << COLOR_GREY << "] " << toMessageColour(level) << message << COLOR_RESET << std::endl; - printf(buffer.str().c_str()); + std::cout << buffer.str() << std::flush; };TactilityCore/Include/Tactility/LoggerAdapterEsp.h (1)
27-33: Considerinlinefor the adapter to avoid ODR concerns.The
static constvariable in a header file creates separate instances in each translation unit that includes this header. While this works, usinginlinewould ensure a single definition across all TUs and is the modern C++17 approach for header-defined constants.🔎 Suggested change
-static const LoggerAdapter espLoggerAdapter = [](LogLevel level, const char* tag, const char* message) { +inline const LoggerAdapter espLoggerAdapter = [](LogLevel level, const char* tag, const char* message) {Tactility/Source/app/AppRegistration.cpp (1)
19-31: Consider using scoped locks consistently.
addAppManifestuses manuallock()/unlock()whileremoveAppManifestusesasScopedLock(). For consistency and safety against exceptions or early returns, consider using scoped locks throughout.🔎 Suggested refactor for consistency
void addAppManifest(const AppManifest& manifest) { LOGGER.info("Registering manifest {}", manifest.appId); - hash_mutex.lock(); + auto lock = hash_mutex.asScopedLock(); + lock.lock(); if (app_manifest_map.contains(manifest.appId)) { LOGGER.warn("Overwriting existing manifest for {}", manifest.appId); } app_manifest_map[manifest.appId] = std::make_shared<AppManifest>(manifest); - - hash_mutex.unlock(); }TactilityCore/Include/Tactility/Logger.h (1)
16-20: Consider usinginlineto avoid per-translation-unit copies.The
statickeyword at namespace scope creates a separate copy ofdefaultLoggerAdapterin each translation unit, though this doesn't cause issues here since it's never reassigned at runtime. Usinginline static(C++17+) instead would provide a single global instance and is more idiomatic for this use case:#ifdef ESP_PLATFORM inline static LoggerAdapter defaultLoggerAdapter = espLoggerAdapter; #else inline static LoggerAdapter defaultLoggerAdapter = genericLoggerAdapter; #endifTactility/Source/app/AppInstall.cpp (2)
25-44: Consider removing unnecessary.c_str()calls.With
std::format,std::stringarguments can be passed directly without.c_str(). This applies to lines 28 and 34.🔎 Example refactor
- LOGGER.error("Can't find or create directory {}", destinationPath.c_str()); + LOGGER.error("Can't find or create directory {}", destinationPath);- LOGGER.error("Failed to write data to {}", absolute_path.c_str()); + LOGGER.error("Failed to write data to {}", absolute_path);
104-209: Consider removing unnecessary.c_str()calls throughout.Multiple logging statements use
.c_str()onstd::stringarguments (lines 114, 118, etc.), which is unnecessary withstd::format. This is consistent with the pattern noted earlier.TactilityCore/Source/file/File.cpp (1)
25-390: LGTM! Logging migration is consistent and correct.All logging statements have been successfully migrated to the new Logger API with correct
std::formatsyntax. The format strings and arguments are properly aligned.Note: Similar to the other file,
.c_str()calls onstd::stringarguments (e.g., lines 157, 163) are unnecessary withstd::formatbut don't affect correctness.
Devices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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 (1)
Tactility/Source/service/gps/GpsConfiguration.cpp (1)
109-111: Missing error handling onwriter.write()calls.Unlike
addGpsConfiguration(lines 75-79), which checks thewrite()return value and handles failures, this loop silently ignores write failures. If a write fails mid-loop, the configuration file could be left in an inconsistent state with partial data.🔎 Proposed fix
for (auto& configuration : configurations) { - writer.write(&configuration); + if (!writer.write(&configuration)) { + LOGGER.error("Failed to write configuration"); + writer.close(); + return false; + } }
🧹 Nitpick comments (9)
Tactility/Source/service/gps/GpsConfiguration.cpp (1)
85-116: Variable shadowing reduces clarity.The loop variable
configuration(line 109) shadows the function parameterconfiguration(line 85). Consider renaming one for clarity.🔎 Proposed fix
-bool GpsService::removeGpsConfiguration(hal::gps::GpsConfiguration configuration) { +bool GpsService::removeGpsConfiguration(hal::gps::GpsConfiguration configToRemove) { std::string path; if (!getConfigurationFilePath(path)) { return false; } std::vector<hal::gps::GpsConfiguration> configurations; if (!getGpsConfigurations(configurations)) { LOGGER.error("Failed to get gps configurations"); return false; } - auto count = std::erase_if(configurations, [&configuration](auto& item) { - return strcmp(item.uartName, configuration.uartName) == 0 && - item.baudRate == configuration.baudRate && - item.model == configuration.model; + auto count = std::erase_if(configurations, [&configToRemove](auto& item) { + return strcmp(item.uartName, configToRemove.uartName) == 0 && + item.baudRate == configToRemove.baudRate && + item.model == configToRemove.model; });Tactility/Private/Tactility/json/Reader.h (1)
35-35: Good use of explicit cast.The
static_cast<int32_t>makes the narrowing conversion explicit, which is good practice. For consistency, consider applying the same explicit cast pattern to line 44 inreadInt(currently uses implicit conversion).🔎 Optional consistency improvement
bool readInt(const char* key, int& output) const { double buffer; if (!readNumber(key, buffer)) { return false; } - output = buffer; + output = static_cast<int>(buffer); return true; }Tactility/Include/Tactility/network/EspHttpClient.h (2)
64-69: Consider reducing log verbosity in getter.The format string is correct. However, logging on every
getStatusCode()call might generate excessive output if this getter is called frequently. Consider usinglogger.verbose()instead oflogger.info(), or removing the log entirely for performance-sensitive paths.🔎 Optional refactor to reduce log noise
- logger.info("Status code {}", status_code); + logger.verbose("Status code {}", status_code);
76-80: Consider reducing log verbosity in read methods.The format strings are correct. However, logging on every
read()andreadResponse()call might generate excessive output, especially if these methods are called in tight loops during data transfer. Consider usinglogger.verbose()instead oflogger.info(), or removing these logs for performance-sensitive paths.🔎 Optional refactor to reduce log noise
- logger.info("read({})", size); + logger.verbose("read({})", size);- logger.info("readResponse({})", size); + logger.verbose("readResponse({})", size);Also applies to: 82-86
Tactility/Source/service/wifi/WifiEsp.cpp (2)
716-716: Inconsistent namespace qualification forgetTicks().Line 716 uses
tt::kernel::getTicks()while line 887 useskernel::getTicks(). For consistency and clarity, both should use the same namespace qualification.🔎 Proposed fix for consistency
Option 1: Use fully qualified names everywhere (safer):
- TickType_t current_time = kernel::getTicks(); + TickType_t current_time = tt::kernel::getTicks();Option 2: Use short form everywhere (requires ensuring no ambiguity):
- wifi->last_scan_time = tt::kernel::getTicks(); + wifi->last_scan_time = kernel::getTicks();Also applies to: 887-887
805-805: Remove unnecessary.c_str()calls.Lines 805 and 816 use
.c_str()when passingstd::stringto the Logger. Sincestd::format(used internally by Logger) handlesstd::stringdirectly, the.c_str()calls are unnecessary and should be removed for consistency with the rest of the codebase.🔎 Proposed fix
- LOGGER.info("Connected to {}", wifi->connection_target.ssid.c_str()); + LOGGER.info("Connected to {}", wifi->connection_target.ssid);- LOGGER.info("Failed to connect to {}", wifi->connection_target.ssid.c_str()); + LOGGER.info("Failed to connect to {}", wifi->connection_target.ssid);Also applies to: 816-816
TactilityCore/Include/Tactility/LoggerAdapterGeneric.h (1)
25-31: Consider splitting the long formatting line for readability.Line 29 is quite long (~180+ characters). While functionally correct, breaking it into smaller insertion operations could improve maintainability.
🔎 Optional refactor for improved readability
static const LoggerAdapter genericLoggerAdapter = [](LogLevel level, const char* tag, const char* message) { constexpr auto COLOR_RESET = "\033[0m"; constexpr auto COLOR_GREY = "\033[37m"; std::stringstream buffer; - buffer << COLOR_GREY << getLogTimestamp() << " [" << toTagColour(level) << toPrefix(level) << COLOR_GREY << "] [" << COLOR_RESET << tag << COLOR_GREY << "] " << toMessageColour(level) << message << COLOR_RESET << std::endl; + buffer << COLOR_GREY << getLogTimestamp() << " [" + << toTagColour(level) << toPrefix(level) << COLOR_GREY << "] [" + << COLOR_RESET << tag << COLOR_GREY << "] " + << toMessageColour(level) << message << COLOR_RESET << std::endl; printf(buffer.str().c_str()); };Devices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.cpp (2)
98-98: Consider consistent error code formatting.Line 68 logs ESP errors with both the error name and hex code:
{} (0x{:02X}), but this line only logs the error name. For consistency and easier debugging, consider including the hex error code here as well.🔎 Optional: Add hex error code for consistency
- LOGGER.error("Failed to set default brightness: {}", esp_err_to_name(ret)); + LOGGER.error("Failed to set default brightness: {} (0x{:02X})", esp_err_to_name(ret), ret);
6-6: Consider removing<esp_log.h>and explicitly including<esp_err.h>instead.After migrating from ESP_LOG* macros to the Logger API,
<esp_log.h>is no longer needed. The file usesesp_err_to_name()on lines 68 and 98, which is declared in<esp_err.h>. Currently, this header is only available transitively through<esp_log.h>. For clarity and reduced coupling, explicitly include<esp_err.h>and remove<esp_log.h>.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Devices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.cppTactility/Include/Tactility/network/EspHttpClient.hTactility/Private/Tactility/json/Reader.hTactility/Source/PartitionsEsp.cppTactility/Source/app/AppInstall.cppTactility/Source/file/ObjectFileWriter.cppTactility/Source/hal/sdcard/SdCardMounting.cppTactility/Source/hal/uart/Uart.cppTactility/Source/hal/uart/UartEsp.cppTactility/Source/network/HttpdReq.cppTactility/Source/service/gps/GpsConfiguration.cppTactility/Source/service/gps/GpsService.cppTactility/Source/service/wifi/WifiEsp.cppTactilityCore/Include/Tactility/LoggerAdapterGeneric.hTactilityCore/Source/Check.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- TactilityCore/Source/Check.cpp
- Tactility/Source/network/HttpdReq.cpp
- Tactility/Source/PartitionsEsp.cpp
- Tactility/Source/app/AppInstall.cpp
- Tactility/Source/hal/sdcard/SdCardMounting.cpp
- Tactility/Source/service/gps/GpsService.cpp
🧰 Additional context used
🧬 Code graph analysis (8)
Tactility/Source/hal/uart/UartEsp.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/service/wifi/WifiEsp.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Include/Tactility/network/EspHttpClient.h (2)
TactilityCore/Include/Tactility/LoggerAdapter.h (1)
tt(6-10)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/hal/uart/Uart.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Include/Tactility/network/EspHttpClient.h (1)
open(38-50)
Tactility/Source/service/gps/GpsConfiguration.cpp (4)
Tactility/Source/app/ElfApp.cpp (1)
LOGGER(117-127)Tactility/Source/app/boot/Boot.cpp (1)
LOGGER(102-131)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/app/notes/Notes.cpp (4)
path(84-97)path(84-84)path(99-110)path(99-99)
TactilityCore/Include/Tactility/LoggerAdapterGeneric.h (6)
Tactility/Include/Tactility/network/EspHttpClient.h (1)
tt(8-107)TactilityCore/Include/Tactility/LoggerAdapter.h (1)
tt(6-10)TactilityCore/Include/Tactility/LoggerAdapterEsp.h (1)
tt(9-35)TactilityCore/Include/Tactility/LoggerAdapterShared.h (4)
tt(5-58)toTagColour(7-23)toPrefix(41-56)toMessageColour(25-39)TactilityCore/Include/Tactility/LoggerSettings.h (1)
tt(5-9)TactilityCore/Include/Tactility/Logger.h (1)
tt(14-44)
Tactility/Source/file/ObjectFileWriter.cpp (5)
Tactility/Source/app/boot/Boot.cpp (1)
LOGGER(102-131)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Include/Tactility/network/EspHttpClient.h (1)
open(38-50)Tactility/Source/hal/uart/Uart.cpp (6)
open(103-114)open(103-103)open(116-129)open(116-116)open(131-144)open(131-131)Tactility/Source/file/ObjectFileReader.cpp (2)
open(11-54)open(11-11)
Tactility/Private/Tactility/json/Reader.h (3)
TactilityCore/Include/Tactility/LoggerAdapterGeneric.h (1)
tt(11-33)TactilityCore/Include/Tactility/LoggerAdapter.h (1)
tt(6-10)TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: macOS
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: Linux
- GitHub Check: run
🔇 Additional comments (26)
Tactility/Source/hal/uart/UartEsp.cpp (3)
5-14: LGTM!The include change and static
LOGGERinstantiation follow the PR-wide logging refactor pattern correctly. The"UART"tag provides clear identification for log messages from this translation unit.
16-61: LGTM!All logging calls in
start()correctly use{}placeholders with matching arguments. The previous issue with mixed format specifiers at line 36 has been resolved.
63-84: LGTM!All logging calls in
stop()correctly use{}placeholders with matching arguments, consistent with thestart()method pattern.Tactility/Source/service/gps/GpsConfiguration.cpp (4)
1-13: LGTM!Logger setup is consistent with the project's logging migration pattern.
15-28: LGTM!Error handling and logging format are correct.
30-61: LGTM!The format specifier issue from the previous review has been correctly addressed. All logging calls now use the proper
{}syntax forstd::format.
63-83: LGTM!Good error handling pattern—checking
write()return value and cleaning up on failure.Tactility/Source/file/ObjectFileWriter.cpp (4)
1-10: LGTM! Logger migration correctly implemented.The addition of the Logger include and static LOGGER instance follows the established pattern for the logging API migration across the codebase.
12-80: Excellent! Previous critical bug has been fixed.All logging calls in the
open()method have been correctly migrated to the new Logger API. Most importantly, line 42 now correctly logsfile_header.versioninstead offile_header.identifier, which resolves the critical issue identified in the previous review.The error messages are well-structured with appropriate context (file paths, expected vs. actual values).
82-104: LGTM! close() method logging correctly migrated.All error paths in the
close()method include appropriate logging with context.
106-120: LGTM! write() method logging correctly migrated.Error handling in the
write()method is properly implemented with appropriate logging.Tactility/Private/Tactility/json/Reader.h (2)
7-7: LGTM! Logger integration follows the project pattern.The include and logger member are correctly added, following the PR's migration from macro-based logging to instance-based Logger API.
Also applies to: 14-14
23-23: LGTM! Logging calls correctly use the new Logger API.All logging calls properly use
{}format placeholders with the instance logger. The previous issue with line 70 using%shas been resolved.Also applies to: 51-51, 61-61, 65-65, 70-70
Tactility/Source/hal/uart/Uart.cpp (1)
3-3: LGTM! Past issue resolved.The LOGGER tag is now correctly set to "UART" for this UART module. The past review comment flagged this as "SPI" (copy-paste error), but this has been fixed.
Also applies to: 20-21
Tactility/Include/Tactility/network/EspHttpClient.h (6)
6-6: LGTM! Logger setup is correct.The include and logger member initialization follow the repository-wide logging migration pattern. The const logger instance is appropriate since the Logger methods are const.
Also applies to: 12-12
31-31: LGTM! Format string issues from previous review have been fixed.The logger calls now correctly use
{}placeholders instead of printf-style format specifiers, which is compatible withstd::formatused by the Logger class.Also applies to: 40-40, 44-44
54-54: LGTM! Logging call is correct.
58-62: LGTM! HTTP success status check is correct.The 200-299 range check follows standard HTTP semantics for successful responses.
71-74: LGTM! Content length getter is correctly implemented.
88-95: LGTM! Logging and close() return value are correct.The logging calls are correctly formatted, and the
close()method now returns a boolean indicating whether the client is closed, which is a useful improvement.Also applies to: 97-104
Tactility/Source/service/wifi/WifiEsp.cpp (3)
10-10: LGTM! Logger infrastructure properly added.The Logger.h and Timer.h includes are correctly added, and the LOGGER instance follows the expected pattern shown in the library context.
Also applies to: 13-13, 28-28
462-462: Previous issue resolved.The printf-style format string issue mentioned in the past review comments has been correctly fixed. This line now uses
std::formatsyntax with{}placeholder and passes the string directly without.c_str().
175-175: Excellent logging migration to the new Logger API.The migration from macro-based logging to the Logger API is comprehensive and correctly implemented throughout the file. All logging calls properly use
std::formatsyntax with{}placeholders, appropriate log levels (info/error/warn), and clear, descriptive messages.Also applies to: 194-194, 218-218, 249-249, 267-267, 298-298, 395-395, 412-425, 429-429, 435-435, 447-447, 458-458, 473-473, 478-480, 484-484, 489-489, 508-508, 518-518, 531-531, 540-540, 548-548, 554-554, 568-570, 598-598, 607-609, 623-625, 630-630, 634-634, 644-644, 648-648, 656-656, 663-663, 671-671, 679-679, 683-683, 692-692, 696-696, 700-700, 706-706, 710-710, 719-719, 723-723, 729-729, 733-733, 737-737, 746-746, 750-750, 776-776, 780-780, 785-785, 789-789, 799-799, 808-810, 820-820, 828-828, 832-832, 838-838, 854-854, 863-863, 870-870, 933-933
Devices/lilygo-tdeck/Source/KeyboardBacklight/KeyboardBacklight.cpp (3)
24-24: LGTM!The logging statements in the initialization function are clear and use correct format specifiers. The cast to
intfor the I2C port enum is appropriate.Also applies to: 28-28, 33-33
68-68: Past issue resolved - format specifiers are now correct.The mixed format specifier issue from the previous review has been fixed. The code now correctly uses
{:02X}for hexadecimal formatting, which is properstd::formatsyntax. The{:02X}specifier is actually better than the previously suggested{:#x}because it ensures consistent 2-digit output with leading zeros.
42-42: LGTM!The error checks and logging for uninitialized state are consistent and appropriate across all functions.
Also applies to: 75-75, 105-105
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Devices/lilygo-tdongle-s3/Source/Init.cpp (1)
4-4: Verify that the GPS service include is needed.The
GpsService.hinclude doesn't appear to be used in this file. Consider removing it if it's not required.Tactility/Source/hal/uart/Uart.cpp (1)
132-132: Optional: Remove unnecessary.c_str()for consistency.The
.c_str()call is unnecessary sincestd::formatcan handlestd::stringdirectly. Line 139 demonstrates the preferred approach by passingnamedirectly without the conversion.🔎 Suggested change for consistency
- LOGGER.info("Open {}", name.c_str()); + LOGGER.info("Open {}", name);Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp (1)
27-27: Use a static Logger instance to match project convention.The code creates a temporary
Loggerinstance, but the established pattern throughout the Devices directory is to use a static Logger instance. All other device configuration and device component files (10+ instances) declarestatic const auto LOGGERat file scope. This file should follow the same pattern for consistency.The tag name "HeltecV3" is appropriate for device-level categorization.
Suggested refactor
+static const auto LOGGER = tt::Logger("HeltecV3"); + static void enableOledPower() { gpio_config_t io_conf = { .pin_bit_mask = (1ULL << DISPLAY_PIN_POWER), .mode = GPIO_MODE_OUTPUT, .pull_up_en = GPIO_PULLUP_DISABLE, // The board has an external pull-up .pull_down_en = GPIO_PULLDOWN_DISABLE, .intr_type = GPIO_INTR_DISABLE, }; gpio_config(&io_conf); gpio_set_level(DISPLAY_PIN_POWER, 0); // Active low vTaskDelay(pdMS_TO_TICKS(500)); // Add a small delay for power to stabilize - Logger("HeltecV3").info("OLED power enabled"); + LOGGER.info("OLED power enabled"); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Devices/cyd-4848s040c/Source/devices/St7701Display.cppDevices/heltec-wifi-lora-32-v3/Source/Configuration.cppDevices/lilygo-tdeck/Source/devices/TdeckKeyboard.cppDevices/lilygo-tdongle-s3/Source/Init.cppDevices/lilygo-tlora-pager/Source/Init.cppDevices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cppDevices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cppDevices/lilygo-tlora-pager/Source/devices/TpagerPower.cppTactility/Source/hal/uart/Uart.cppTactilityCore/Include/Tactility/LoggerAdapterGeneric.h
🚧 Files skipped from review as they are similar to previous changes (1)
- TactilityCore/Include/Tactility/LoggerAdapterGeneric.h
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cppDevices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cppDevices/lilygo-tdeck/Source/devices/TdeckKeyboard.cpp
🧬 Code graph analysis (9)
Devices/lilygo-tlora-pager/Source/Init.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/lilygo-tlora-pager/Source/devices/TpagerPower.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/hal/uart/Uart.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (2)
setBacklightDuty(234-236)setBacklightDuty(234-234)
Devices/lilygo-tdeck/Source/devices/TdeckKeyboard.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/lilygo-tdongle-s3/Source/Init.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: macOS
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: run
🔇 Additional comments (15)
Devices/lilygo-tdongle-s3/Source/Init.cpp (2)
7-7: LGTM!The Logger instantiation correctly uses a string literal for the tag, which has static storage duration and is safe for the Logger's
const char*member.
9-16: LGTM!The migration from TAG-based logging to the new Logger API is implemented correctly. The error logging is appropriate for the backlight initialization failure, and the control flow properly returns false on error and true on success.
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp (1)
7-7: LGTM!The include update correctly reflects the migration from the old macro-based logging to the new Logger API.
Devices/lilygo-tdeck/Source/devices/TdeckKeyboard.cpp (2)
9-14: LGTM! Logger integration follows best practices.The Logger is correctly initialized with a descriptive tag and proper scoping. Using
static const autois appropriate here—the Logger instance is runtime-initialized and file-scoped, preventing global namespace pollution.
42-50: LGTM! Guarded logging pattern is correct and performant.The conditional checks before logging avoid string formatting overhead when debug logging is disabled. The format strings correctly use
{}placeholders for the uint8_t key codes, which will render as decimal integers.Devices/lilygo-tlora-pager/Source/devices/TpagerPower.cpp (2)
4-6: LGTM! Clean logging migration.The migration to the new Logger API is correctly implemented with an appropriate tag name.
71-71: LGTM! Correct error logging.The error logging correctly uses the new Logger API.
Devices/lilygo-tlora-pager/Source/Init.cpp (2)
3-12: LGTM! Proper logger initialization.The logger is correctly initialized with a descriptive tag for the T-Lora Pager device.
15-50: LGTM! Consistent logging throughout initialization.The logging calls appropriately use info and error levels for different scenarios, maintaining clear and descriptive messages.
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (2)
3-7: LGTM! Logger correctly initialized.The logger instance is properly set up with an appropriate tag name.
64-111: LGTM! Consistent error logging for pulse counter operations.All error paths correctly use the new Logger API with clear, descriptive messages.
Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cpp (2)
4-9: LGTM! Proper logger setup.The logger is correctly initialized with an appropriate tag name matching the component.
177-204: LGTM! Clean error logging for backlight operations.All error logging calls correctly use the new Logger API with descriptive messages.
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (2)
5-5: LGTM: Logging header updated correctly.The include change from
Tactility/Log.htoTactility/Logger.haligns with the PR-wide migration to the new C++ Logger API.
14-14: LGTM: Logger instance initialized correctly.The static Logger instance with tag "St7701Display" follows the new logging pattern. Using
const autois appropriate here since the Logger constructor is not constexpr.
There was a problem hiding this comment.
Actionable comments posted: 20
Fix all issues with AI Agents 🤖
In @Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp:
- Around line 7-10: There is a duplicate include of the header
<Tactility/Logger.h> in Configuration.cpp; remove the redundant include so the
file contains only a single #include <Tactility/Logger.h> (keep one occurrence
and delete the other).
In @Devices/m5stack-cores3/Source/InitBoot.cpp:
- Line 78: The log call uses printf-style formatting; change
LOGGER.info("AXP2101: VBus at %.2f", voltage) to use C++ std::format syntax by
replacing the format string with "AXP2101: VBus at {:.2f}" so LOGGER.info
receives a std::format-style string and the voltage argument (i.e., update the
call in InitBoot.cpp where LOGGER.info is invoked for AXP2101 VBus to use
"{:.2f}" instead of "%.2f").
In @Devices/simulator/Source/Main.cpp:
- Line 52: The log call uses printf-style specifiers but the Logger expects
std::format `{}` placeholders; update the LOGGER.error invocation to use
`{}`-style formatting, e.g. replace LOGGER.error("assert triggered at %s:%d",
file, line); with a call using the format string "assert triggered at {}:{}" and
the same file and line arguments so the Logger will format correctly (ensure you
modify the LOGGER.error call site where it appears in Main.cpp and keep the file
and line variables as the arguments).
In @Devices/unphone/Source/UnPhoneFeatures.cpp:
- Line 45: LOGGER.info currently uses a printf-style "%d" placeholder which is
incompatible with the Logger's std::format-based API; replace the format string
to use brace-style placeholders and pass pinNumber as an argument, e.g. change
the call in UnPhoneFeatures.cpp (the LOGGER.info invocation that logs the
pressed button) to use "Pressed button {}" with pinNumber as the parameter so
std::format formats it correctly.
- Line 234: The log call using printf-style "%s" is incorrect for this Logger
(it uses std::format); update the LOGGER.info invocation that references
backlight_power_state to use a {} placeholder and pass backlight_power_state as
the argument (e.g., change the format string "Backlight: %s" to "Backlight: {}"
in the LOGGER.info call).
In @Drivers/BQ24295/Source/Bq24295.cpp:
- Around line 50-54: The code clears bits 4-5 into value_cleared then attempts
to combine them with bits_to_set using AND, which always yields 0; change the
combination to use OR so the cleared base value (value_cleared) gets the new
watchdog bits (bits_to_set) restored into to_set (i.e., compute to_set =
value_cleared | bits_to_set) before logging and calling writeRegister8 for
registers::CHARGE_TERMINATION; keep the LOGGER.info call as-is.
In @Drivers/DRV2605/Source/Drv2605.cpp:
- Around line 3-5: Remove the duplicate file-scope static LOGGER in Drv2605.cpp
(the lines defining "static const auto LOGGER = tt::Logger(\"DRV2605\");") and
replace all uses of LOGGER in this translation unit with the instance member
"logger" declared in the Drv2605 class; ensure Drv2605.h includes
<Tactility/Logger.h> so the "logger" member is declared correctly and update any
constructor/initialization code to use the member logger rather than the removed
static.
- Line 17: The log call uses an enum class value (chip_id of type ChipId) with a
hex format specifier and the static LOGGER identifier; fix by casting chip_id to
its underlying integral type (e.g., static_cast<uint8_t>(chip_id)) when passing
it to the formatter and use the instance logger (logger) consistently instead of
LOGGER so the call becomes logger.error(..., static_cast<...>(chip_id)).
In @Drivers/DRV2605/Source/Drv2605.h:
- Line 7: There are duplicate logger instances: the instance member "logger"
declared in class (const tt::Logger logger) conflicts with a separate static
"LOGGER" in Drv2605.cpp; remove the static LOGGER from the cpp and update all
references in Drv2605.cpp to use the instance member "logger" instead, and
ensure the header declares the logger type by adding #include
"<Tactility/Logger.h>" before the "const tt::Logger logger" declaration so the
tt::Logger type is defined.
In @Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp:
- Line 104: Replace the ambiguous LOGGER.error("Draw buffer alloc") message with
an explicit failure message matching the style used at line 61; update the
LOGGER.error call in CrashDiagnostics.cpp (the one currently logging "Draw
buffer alloc") to a clear message such as "Failed to allocate draw buffer"
(optionally include relevant context like requested size or errno) so the log
unambiguously indicates allocation failure.
- Line 69: Fix the typo in the LOGGER.error message by removing the extra space:
locate the LOGGER.error call that logs "QR init text failed" (in
CrashDiagnostics.cpp) and change the message to "QR init text failed" so there
is only a single space between "text" and "failed".
In @Tactility/Source/app/gpssettings/GpsSettings.cpp:
- Around line 97-98: The boundary check in GpsSettings.cpp incorrectly allows
index == configurations.size(), causing out-of-bounds access when calling
gps_service->removeGpsConfiguration(configurations[index]); change the condition
to ensure index is strictly less than configurations.size() (and if index can be
negative, also ensure index >= 0) before indexing into configurations; update
the if that surrounds gps_service->removeGpsConfiguration(...) to use this
stricter check so only valid 0..size()-1 indices are used.
In @Tactility/Source/app/notes/Notes.cpp:
- Line 94: The LOGGER.info calls using printf-style "%s" need to be updated to
the new {}-style formatting: locate the LOGGER.info invocations (e.g., the call
that logs "Loaded from %s" and the similar call around line 104) and change the
format string to use "{}" and pass the path (or path.c_str()) as the argument so
it matches the Logger API used elsewhere in this file (keep the same argument
order and message text).
- Line 55: The log statement is using the wrong variable: replace the use of
loadFileLaunchId in the LOGGER.info call with saveFileLaunchId (the variable
assigned just above) so the message logs the correct launch ID; locate the
LOGGER.info("launched with id {}", loadFileLaunchId) line in Notes.cpp and
change the referenced variable to saveFileLaunchId.
- Line 67: The log call currently uses the wrong variable name: change the
LOGGER.info invocation that references loadFileLaunchId to instead reference
saveFileLaunchId (the variable just assigned) so the logged launch ID matches
the saved operation; locate the LOGGER.info("launched with id {}",
loadFileLaunchId) line and replace the argument with saveFileLaunchId.
In @Tactility/Source/app/timezone/TimeZone.cpp:
- Line 143: The log is using the variable count (which tracks successfully
parsed/filtered entries) as if it were the file line number; introduce a
separate line counter (e.g., lineNumber) in the parsing loop that increments for
every input line read, keep count intact for parsed entries, and change the
LOGGER.error call to use the new lineNumber so the error reports the actual file
line where parsing failed (update any nearby references in TimeZone.cpp parsing
function/loop to use the new variable).
In @Tactility/Source/hal/gps/GpsInit.cpp:
- Line 115: Replace the undefined millis() calls with kernel::getMillis() (e.g.,
in the LOGGER.info call that uses millis()); ensure timing comparisons use the
same units by making startTime and all subsequent elapsed-time calculations use
kernel::getMillis() rather than kernel::getTicks(), or alternatively convert
waitMillis to ticks if you prefer ticks; specifically, update uses around
startTime, the conditional that currently does "kernel::getTicks() - startTime"
to instead use "kernel::getMillis()" for both operands (or convert waitMillis to
tick units) so comparisons and the LOGGER.info elapsed time are consistent.
In @Tactility/Source/hal/gps/Probe.cpp:
- Line 137: The log call in LOGGER.warn still uses printf-style "%lu" which
breaks the std::format-style logging used elsewhere; update the call in
Probe.cpp (the LOGGER.warn invocation that passes uart.getBaudRate()) to use a
"{}" placeholder and pass uart.getBaudRate() as the argument so the baud rate is
formatted with the C++20/std::format-style placeholder.
- Line 70: The call LOGGER.debug(debugmsg) uses debugmsg as the format string
and can misinterpret `{}` in its contents; change the call to pass a literal
format and provide debugmsg as an argument (e.g. LOGGER.debug("{}", debugmsg))
so debugmsg is treated as data rather than a format string; update the call site
in Probe.cpp where LOGGER.debug and the debugmsg variable are used.
In @TactilityCore/Include/Tactility/Check.h:
- Line 68: The tt_check macro declares variadic parameters but never uses them;
update it to delegate to the existing tt_check_internal that handles the
optional message parameter instead of swallowing __VA_ARGS__. Replace the
current tt_check definition so it forwards (x, __VA_ARGS__) to tt_check_internal
(preserving the original behavior of logging the failed expression and calling
tt::_crash()), ensuring the macro name tt_check and the internal helper
tt_check_internal are referenced so any extra message arguments are properly
consumed.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Tactility/Source/app/i2cscanner/I2cScanner.cpp (1)
237-245: Race condition: usesportinstead ofsafe_port.Line 238 accesses the member variable
portdirectly, butsafe_portwas obtained earlier (lines 224-229) specifically for thread-safe access. If the user changes the bus selection while scanning is in progress, this could cause a race condition.🔎 Proposed fix
- if (hal::i2c::masterHasDeviceAtAddress(port, address, 10 / portTICK_PERIOD_MS)) { + if (hal::i2c::masterHasDeviceAtAddress(safe_port, address, 10 / portTICK_PERIOD_MS)) {Drivers/PwmBacklight/Source/PwmBacklight.cpp (1)
48-54: Fix incomplete error handling for channel config failure.The channel configuration error at line 48 is logged but doesn't prevent the function from returning
trueand settingisBacklightInitialized = true. This is inconsistent with the timer config error handling at lines 29-32 and could lead to attempting to use uninitialized hardware.🔎 Proposed fix to return false on channel config failure
if (ledc_channel_config(&ledc_channel) != ESP_OK) { LOGGER.error("Channel config failed"); + return false; }Tactility/Source/app/files/View.cpp (1)
128-167: Fix inconsistent capitalization in warning message.Lines 139 and 167 both warn about unsupported link operations, but use inconsistent capitalization: "opening links is not supported" vs "Opening links is not supported".
🔎 Suggested fix for consistency
- LOGGER.warn("Opening links is not supported"); + LOGGER.warn("opening links is not supported");Tactility/Source/app/wificonnect/View.cpp (1)
202-205: Fix incorrect variable in optPasswordParameter call.Line 203 passes
ssidtooptPasswordParameter, but it should passpassword. This bug prevents the password parameter from being correctly retrieved from the bundle.🔎 Proposed fix
std::string password; - if (optPasswordParameter(bundle, ssid)) { + if (optPasswordParameter(bundle, password)) { lv_textarea_set_text(password_textarea, password.c_str()); }Tactility/Source/Tactility.cpp (1)
237-266: Distinguish between primary and secondary service registration messages.Lines 238 and 253 both log "Registering and starting system services", but they're in different functions (
registerAndStartSecondaryServicesvsregisterAndStartPrimaryServices). These messages should be distinct to aid debugging and make the boot sequence clearer.🔎 Suggested fix to distinguish the messages
For line 238:
- LOGGER.info("Registering and starting system services"); + LOGGER.info("Registering and starting secondary services");For line 253:
- LOGGER.info("Registering and starting system services"); + LOGGER.info("Registering and starting primary services");TactilityCore/Include/Tactility/Check.h (1)
50-60: Remove calls to undefinedtt_crash_internalmacro.The macro
tt_crash_internalcalled on lines 55 and 57 is not defined anywhere in the codebase. This will cause linker errors. Replace these calls withtt::_crash()to match the pattern used in thett_crash()macro (lines 33–42), which correctly logs via the Logger API before crashing.Current problematic code (lines 50-60)
#define tt_check_internal(__e, __m) \ do { \ if (!(__e)) { \ tt::Logger("Kernel").error("Check failed: {}", #__e); \ if (__m) { \ tt_crash_internal(#__m); \ } else { \ tt_crash_internal(""); \ } \ } \ } while (0)
🧹 Nitpick comments (25)
Drivers/ST7796/Source/St7796Display.cpp (1)
170-172: Consider removing commented-out dead code.The commented-out gamma setting implementation could be removed to reduce clutter. If this functionality might be needed later, consider documenting the reason in a TODO comment or relying on version control history.
Drivers/ILI9488/Source/Ili9488Display.cpp (1)
53-85: Error logging migration looks good.All error paths correctly use the new
LOGGER.error()method with clear, descriptive messages.Minor: Remove trailing space in line 68
Line 68 has a trailing space that could be cleaned up for consistency:
- LOGGER.error("Failed to swap XY "); + LOGGER.error("Failed to swap XY");Tactility/Source/app/fileselection/View.cpp (1)
22-22: Consider aligning LOGGER declaration style with State.cpp.The declaration uses
const static LoggerwhileState.cpp:16usesstatic const auto. For consistency across related files, consider matching the declaration style.🔎 Suggested alignment
-const static Logger LOGGER = Logger("FileSelection"); +static const auto LOGGER = Logger("FileSelection");Drivers/GC9A01/Source/Gc9a01Display.cpp (1)
59-59: LGTM! Error logging correctly migrated.All error logging calls in
createPanelHandleare properly migrated to the new Logger API and maintain appropriate error messages for each failure case.Optional: Line 74 has a trailing space in "Failed to swap XY " that could be removed for consistency.
🔎 Proposed fix for trailing space
- LOGGER.error("Failed to swap XY "); + LOGGER.error("Failed to swap XY");Also applies to: 64-64, 69-69, 74-74, 79-79, 84-84, 89-89
Devices/m5stack-cardputer-adv/Source/devices/CardputerPower.cpp (1)
1-85: Consider consolidating duplicate CardputerPower implementations.This file is identical to
Devices/m5stack-cardputer/Source/devices/CardputerPower.cpp. If the behavior is the same across device variants, consider extracting the common implementation to reduce maintenance burden.Drivers/SSD1306/Source/Ssd1306Display.cpp (1)
34-34: LGTM! Logging migration is correct and complete.All logging calls have been successfully migrated to the new Logger API with proper format strings and arguments.
Optional: Consider including symbolic error names for better debuggability
At line 34, you could enhance the error message by including the symbolic error name:
- LOGGER.error("Failed to send command 0x{:02X}: {}", cmd, ret); + LOGGER.error("Failed to send command 0x{:02X}: {} ({})", cmd, esp_err_to_name(ret), ret);This would make error messages more informative during debugging. However, this is purely optional and beyond the scope of the current logging refactor.
Also applies to: 52-52, 96-96, 106-106, 173-173
Drivers/ST7735/Source/St7735Display.cpp (3)
13-13: Consider adding context to the log message.The message "Starting" is quite generic. For better debugging, consider specifying what is starting, e.g., "Creating ST7735 IO handle".
40-40: Consider enhancing error messages with ESP error codes and context.Several error messages could be more informative for debugging:
- Lines 40 and 61 use identical "Failed to create panel" messages in different contexts (IO vs panel device creation)
- ESP API calls return error codes that could be included using the new Logger's {}-style formatting
This would aid troubleshooting display initialization issues.
Example enhancement
- if (esp_lcd_new_panel_io_spi(configuration->spiHostDevice, &panel_io_config, &outHandle) != ESP_OK) { - LOGGER.error("Failed to create panel"); + auto result = esp_lcd_new_panel_io_spi(configuration->spiHostDevice, &panel_io_config, &outHandle); + if (result != ESP_OK) { + LOGGER.error("Failed to create panel IO: error {}", result); return false; }Similarly for line 61:
- if (esp_lcd_new_panel_st7735(ioHandle, &panel_config, &panelHandle) != ESP_OK) { - LOGGER.error("Failed to create panel"); + auto result = esp_lcd_new_panel_st7735(ioHandle, &panel_config, &panelHandle); + if (result != ESP_OK) { + LOGGER.error("Failed to create ST7735 panel device: error {}", result); return false; }Also applies to: 61-99
168-168: Optionally include gamma index in error message.For better debugging context, consider including the gamma index parameter in the error message.
Example enhancement
- if (esp_lcd_panel_io_tx_param(io_handle, LCD_CMD_GAMSET, param, 1) != ESP_OK) { - LOGGER.error("Failed to set gamma"); + auto result = esp_lcd_panel_io_tx_param(io_handle, LCD_CMD_GAMSET, param, 1); + if (result != ESP_OK) { + LOGGER.error("Failed to set gamma curve {}: error {}", index, result); }Tactility/Source/app/apphub/AppHubApp.cpp (1)
110-110: Optional: Remove unnecessary.c_str()call.
std::formathandlesstd::stringdirectly, so the.c_str()call is unnecessary.🔎 Proposed simplification
- LOGGER.info("Adding {}", entry.appName.c_str()); + LOGGER.info("Adding {}", entry.appName);Devices/unphone/Source/devices/Hx8357Display.cpp (1)
62-62: Logging migration correct; consider improving the "Failed" message.The logging API migration is correct. However, the "Failed" message on line 62 is quite generic and could be more descriptive for debugging purposes.
💡 Optional: More descriptive failure message
- LOGGER.info("Failed"); + LOGGER.info("Failed to create LVGL display");Also applies to: 105-105
Tactility/Source/app/wifimanage/View.cpp (2)
69-88: LGTM! Consider improving edge case handling for clarity.The logging migration is correctly implemented with proper format strings. The logic and control flow remain unchanged.
Optional: Handle edge case when ap_records is empty
When
ap_records.size()is 0, the expressionap_records.size() - 1underflows (sincesize_tis unsigned), resulting in a confusing log message. Consider handling this edge case for clearer error messages:} else { - LOGGER.warn("Clicked AP: record {}/{} does not exist", index, ap_records.size() - 1); + if (ap_records.empty()) { + LOGGER.warn("Clicked AP: record {} does not exist (no records available)", index); + } else { + LOGGER.warn("Clicked AP: record {}/{} does not exist", index, ap_records.size() - 1); + } }
90-104: LGTM! Consider improving edge case handling for clarity.The logging migration is correctly implemented with proper format strings. The logic and control flow remain unchanged.
Optional: Handle edge case when ap_records is empty
When
ap_records.size()is 0, the expressionap_records.size() - 1underflows (sincesize_tis unsigned), resulting in a confusing log message. Consider handling this edge case for clearer error messages:} else { - LOGGER.warn("Clicked AP: record {}/{} does not exist", index, ap_records.size() - 1); + if (ap_records.empty()) { + LOGGER.warn("Clicked AP: record {} does not exist (no records available)", index); + } else { + LOGGER.warn("Clicked AP: record {}/{} does not exist", index, ap_records.size() - 1); + } }Tactility/Source/app/screenshot/Screenshot.cpp (3)
118-118: Consider logging the actual delay value.The warning states "delay was 0", but the condition checks
delay > 0, meaning delay could be 0 or negative. Including the actual value would aid debugging.🔎 Proposed enhancement
- LOGGER.warn("Ignored screenshot start because delay was 0"); + LOGGER.warn("Ignored screenshot start because delay was {}", delay);
132-132: Optional: Add function context to duplicate error messages.The error "Service not found/running" appears identically at lines 101, 132, and 155 across different functions (onStartPressed, updateScreenshotMode, createModeSettingWidgets). Adding function context would make debugging easier.
Example approach:
LOGGER.error("Service not found/running in updateScreenshotMode");Or leverage the new Logger format capabilities:
LOGGER.error("{}: Service not found/running", __func__);Also applies to: 155-155
207-207: Optional: Include device count in warning.The warning could be more informative by logging the actual number of SD card devices found.
🔎 Proposed enhancement
- LOGGER.warn("Found multiple SD card devices - picking first"); + LOGGER.warn("Found {} SD card devices - picking first", sdcard_devices.size());Devices/waveshare-s3-touch-lcd-147/Source/devices/Jd9853Display.cpp (1)
56-93: Error logging pattern correctly migrated.All error logging calls have been successfully converted to the new Logger API. The error handling logic and return flow remain unchanged.
Optional enhancement for future consideration:
The error messages could be enhanced by including ESP error codes for easier debugging. For example:
esp_err_t err = esp_lcd_new_panel_jd9853(ioHandle, &panel_config, &panelHandle); if (err != ESP_OK) { LOGGER.error("Failed to create panel: {}", esp_err_to_name(err)); return false; }This enhancement would demonstrate the new Logger's
{}-style formatting capabilities while providing more diagnostic context.Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
96-96: Prefer the member logger for consistency.The code creates a temporary
Logger("GpsSettings")instance, but the class already has a memberloggerat line 27. Since the static callback has access to theapppointer (line 85), consider usingapp->logger.info(...)instead for consistency.🔎 Proposed refactor
- Logger("GpsSettings").info("Found service and configs {} {}", index, configurations.size()); + app->logger.info("Found service and configs {} {}", index, configurations.size());Tactility/Source/service/screenshot/ScreenshotTask.cpp (1)
159-174: Consider adding logging for consistency in stop() method.Unlike other methods in this file (
isInterrupted(),isFinished(),taskStart(), etc.), thestop()method doesn't log warnings or errors when mutex lock operations fail (lines 161, 168). This inconsistency could make debugging more difficult if lock failures occur during shutdown.🔎 Proposed enhancement for consistent error logging
void ScreenshotTask::stop() { if (thread != nullptr) { if (mutex.lock(50 / portTICK_PERIOD_MS)) { interrupted = true; mutex.unlock(); + } else { + LOGGER.warn(LOG_MESSAGE_MUTEX_LOCK_FAILED); } thread->join(); if (mutex.lock(50 / portTICK_PERIOD_MS)) { delete thread; thread = nullptr; mutex.unlock(); + } else { + LOGGER.warn(LOG_MESSAGE_MUTEX_LOCK_FAILED); } } }Tactility/Source/app/wifiapsettings/WifiApSettings.cpp (2)
54-57: Logging migration looks good.The error logging calls are correct. Optionally, consider including the SSID in these error messages for better debuggability, e.g.,
LOGGER.error("Failed to save settings for SSID: {}", self->ssid).
227-231: Logging migration is correct.The error and info logging calls work as intended. Optionally, consider including the SSID value in these messages for better log context, e.g.,
LOGGER.error("Failed to remove SSID: {}", ssid)andLOGGER.info("Removed SSID: {}", ssid).Tactility/Source/service/wifi/WifiEsp.cpp (2)
806-806: Remove unnecessary.c_str()calls.Lines 806 and 817 use
.c_str()when passingstd::stringto the logger, butstd::formathandlesstd::stringdirectly. This is inconsistent with line 738 which correctly passes the string without.c_str().🔎 Suggested simplification
- LOGGER.info("Connected to {}", wifi->connection_target.ssid.c_str()); + LOGGER.info("Connected to {}", wifi->connection_target.ssid);- LOGGER.info("Failed to connect to {}", wifi->connection_target.ssid.c_str()); + LOGGER.info("Failed to connect to {}", wifi->connection_target.ssid);Also applies to: 817-817
855-855: Consider capitalizing error messages for consistency.Lines 855 and 864 use lowercase "failed" while similar error messages elsewhere in the file use capitalized "Failed" (e.g., lines 657, 664, 672, 680, 751, 781, 790). Maintaining consistent capitalization improves code readability.
🔎 Suggested capitalization fix
- LOGGER.error("failed to set wifi config ({})", esp_err_to_name(set_config_result)); + LOGGER.error("Failed to set wifi config ({})", esp_err_to_name(set_config_result));- LOGGER.error("failed to start wifi to begin connecting ({})", esp_err_to_name(wifi_start_result)); + LOGGER.error("Failed to start wifi to begin connecting ({})", esp_err_to_name(wifi_start_result));Also applies to: 864-864
Tactility/Source/service/loader/Loader.cpp (1)
239-243: Minor formatting nit.The multi-line log statement has unusual indentation. Consider aligning for consistency.
🔎 Suggested formatting
- LOGGER.info( "App \"{}\" state: {} -> {}", - app_manifest.appId, + LOGGER.info( + "App \"{}\" state: {} -> {}", + app_manifest.appId, appStateToString(old_state), appStateToString(state) );TactilityCore/Include/Tactility/LoggerAdapter.h (1)
8-8: Consider usingusingalias instead oftypedef.Modern C++ style prefers
usingovertypedeffor type aliases, as it's more readable and consistent with template aliases.🔎 Suggested change
-typedef std::function<void(LogLevel level, const char* tag, const char* message)> LoggerAdapter; +using LoggerAdapter = std::function<void(LogLevel level, const char* tag, const char* message)>;
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp:
- Around line 58-63: The allocation failure for qrcodeData uses stop() with no
argument, which is inconsistent with other error paths that call
stop(manifest.appId); update the error path in the qrcode buffer allocation
block so it calls stop(manifest.appId) instead of stop(), and keep the
LOGGER.error("Failed to allocate QR buffer") message and return behavior
unchanged; locate this in CrashDiagnostics.cpp around the
qrcode_getBufferSize(qr_version) allocation and modify the stop() invocation to
pass manifest.appId.
In @TactilityCore/Include/Tactility/Check.h:
- Line 55: The macro currently calls tt_crash_internal(#__m) which stringifies
the already const char* __m (causing extra quotes/escapes); change the
invocation to tt_crash_internal(__m) so the message pointer is passed directly
(update the macro that references __m and tt_crash_internal accordingly).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (1)
43-113: Critical: Initialization errors are logged but not handled.When any initialization step fails (e.g.,
pcnt_new_unit,pcnt_unit_set_glitch_filter, etc.), the function logs an error but continues executing. This means:
- If
pcnt_new_unitfails at line 55,encPcntUnitremains invalid- All subsequent operations on the invalid handle will fail or cause undefined behavior
- The caller
startLvgl()has no way to detect initialization failure- The device may appear to initialize successfully but behave unpredictably
🔎 Proposed fix: Add proper error propagation
Change
initEncoder()to return a boolean and propagate errors:-void TpagerEncoder::initEncoder() { +bool TpagerEncoder::initEncoder() { constexpr int LOW_LIMIT = -127; constexpr int HIGH_LIMIT = 126; // Accum. count makes it that over- and underflows are automatically compensated. // Prerequisite: watchpoints at low and high limit pcnt_unit_config_t unit_config = { .low_limit = LOW_LIMIT, .high_limit = HIGH_LIMIT, .flags = {.accum_count = 1}, }; if (pcnt_new_unit(&unit_config, &encPcntUnit) != ESP_OK) { LOGGER.error("Pulsecounter initialization failed"); + return false; } pcnt_glitch_filter_config_t filter_config = { .max_glitch_ns = 1000, }; if (pcnt_unit_set_glitch_filter(encPcntUnit, &filter_config) != ESP_OK) { LOGGER.error("Pulsecounter glitch filter config failed"); + return false; } pcnt_chan_config_t chan_1_config = { .edge_gpio_num = ENCODER_B, .level_gpio_num = ENCODER_A, }; pcnt_chan_config_t chan_2_config = { .edge_gpio_num = ENCODER_A, .level_gpio_num = ENCODER_B, }; pcnt_channel_handle_t pcnt_chan_1 = nullptr; pcnt_channel_handle_t pcnt_chan_2 = nullptr; if ((pcnt_new_channel(encPcntUnit, &chan_1_config, &pcnt_chan_1) != ESP_OK) || (pcnt_new_channel(encPcntUnit, &chan_2_config, &pcnt_chan_2) != ESP_OK)) { LOGGER.error("Pulsecounter channel config failed"); + return false; } // Second argument is rising edge, third argument is falling edge if ((pcnt_channel_set_edge_action(pcnt_chan_1, PCNT_CHANNEL_EDGE_ACTION_DECREASE, PCNT_CHANNEL_EDGE_ACTION_INCREASE) != ESP_OK) || (pcnt_channel_set_edge_action(pcnt_chan_2, PCNT_CHANNEL_EDGE_ACTION_INCREASE, PCNT_CHANNEL_EDGE_ACTION_DECREASE) != ESP_OK)) { LOGGER.error("Pulsecounter edge action config failed"); + return false; } // Second argument is low level, third argument is high level if ((pcnt_channel_set_level_action(pcnt_chan_1, PCNT_CHANNEL_LEVEL_ACTION_KEEP, PCNT_CHANNEL_LEVEL_ACTION_INVERSE) != ESP_OK) || (pcnt_channel_set_level_action(pcnt_chan_2, PCNT_CHANNEL_LEVEL_ACTION_KEEP, PCNT_CHANNEL_LEVEL_ACTION_INVERSE) != ESP_OK)) { LOGGER.error("Pulsecounter level action config failed"); + return false; } if ((pcnt_unit_add_watch_point(encPcntUnit, LOW_LIMIT) != ESP_OK) || (pcnt_unit_add_watch_point(encPcntUnit, HIGH_LIMIT) != ESP_OK)) { LOGGER.error("Pulsecounter watch point config failed"); + return false; } if (pcnt_unit_enable(encPcntUnit) != ESP_OK) { LOGGER.error("Pulsecounter could not be enabled"); + return false; } if (pcnt_unit_clear_count(encPcntUnit) != ESP_OK) { LOGGER.error("Pulsecounter could not be cleared"); + return false; } if (pcnt_unit_start(encPcntUnit) != ESP_OK) { LOGGER.error("Pulsecounter could not be started"); + return false; } + return true; }Update
startLvgl()to check the return value:bool TpagerEncoder::startLvgl(lv_display_t* display) { - initEncoder(); + if (!initEncoder()) { + LOGGER.error("Encoder initialization failed"); + return false; + } gpio_input_enable(ENCODER_ENTER);Don't forget to update the header file declaration as well.
TactilityCore/Include/Tactility/Check.h (1)
55-57: Define or replacett_crash_internal—this macro is undefined and will cause compilation failure.
tt_crash_internalis referenced on lines 55 and 57 but has no definition anywhere in the codebase. The available crash mechanism istt_crash(defined on line 31). Either definett_crash_internalas a macro/function or replace these calls withtt_crash.Additionally, line 55 stringifies the message parameter with
#__m, but__mis documented as an optionalconst char*, so this should likely be__mwithout the stringification operator.
♻️ Duplicate comments (3)
Devices/unphone/Source/UnPhoneFeatures.cpp (1)
45-45: Format string issues from previous review have been addressed.Both logging calls now correctly use
{}placeholders compatible withstd::format:
- Line 45:
"Pressed button {}"for theintpinNumber- Line 234:
"Backlight: {}"for theconst char*backlight_power_stateAlso applies to: 234-234
Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
97-97: Boundary check fixed correctly.The condition now correctly prevents out-of-bounds access by ensuring
index < configurations.size(). This addresses the critical off-by-one error that was previously flagged.TactilityCore/Include/Tactility/Check.h (1)
67-67: Past review comment addressed - delegation now correct.The
tt_checkmacro now properly delegates tott_check_internal(__VA_ARGS__), which resolves the previous issue where variadic parameters were declared but not used. This ensures that optional message parameters are correctly forwarded.
🧹 Nitpick comments (1)
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp (1)
27-27: Logger migration looks good.The logging call has been correctly migrated to use the new
tt::LoggerAPI with proper formatting. The code is functionally correct.For consistency with the project-wide logging pattern, consider defining a static Logger instance at file scope rather than creating a temporary inline:
🔎 Optional refactor for consistency
Add at the top of the file (e.g., after the includes):
static tt::Logger LOGGER("HeltecV3");Then update the call:
- tt::Logger("HeltecV3").info("OLED power enabled"); + LOGGER.info("OLED power enabled");This aligns with the static per-file
LOGGERpattern used elsewhere in the logging refactor, though the performance difference is negligible for this one-time initialization call.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Devices/cyd-4848s040c/Source/devices/St7701Display.cppDevices/heltec-wifi-lora-32-v3/Source/Configuration.cppDevices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cppDevices/m5stack-cores3/Source/InitBoot.cppDevices/simulator/Source/Main.cppDevices/unphone/Source/UnPhoneFeatures.cppDrivers/BQ24295/Source/Bq24295.cppDrivers/DRV2605/Source/Drv2605.cppDrivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cppTactility/Source/app/crashdiagnostics/CrashDiagnostics.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/app/notes/Notes.cppTactility/Source/hal/gps/GpsInit.cppTactility/Source/hal/gps/Probe.cppTactility/Source/hal/uart/Uart.cppTactilityCore/Include/Tactility/Check.h
🚧 Files skipped from review as they are similar to previous changes (5)
- Drivers/DRV2605/Source/Drv2605.cpp
- Tactility/Source/hal/uart/Uart.cpp
- Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cpp
- Devices/simulator/Source/Main.cpp
- Tactility/Source/hal/gps/GpsInit.cpp
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/gpssettings/GpsSettings.cppDevices/cyd-4848s040c/Source/devices/St7701Display.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp
🧬 Code graph analysis (9)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/app/gpssettings/GpsSettings.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/Tactility.cpp (2)
getMainDispatcher(347-349)getMainDispatcher(347-347)
Devices/m5stack-cores3/Source/InitBoot.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/hal/gps/Probe.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/app/notes/Notes.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/app/chat/ChatApp.cpp (2)
e(41-55)e(41-41)
Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp (3)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/app/crashdiagnostics/QrHelpers.cpp (2)
getQrVersionForBinaryDataLength(17-26)getQrVersionForBinaryDataLength(17-17)Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cpp (2)
stop(120-129)stop(120-120)
Devices/unphone/Source/UnPhoneFeatures.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Devices/m5stack-cores3/Source/InitBoot.cpp (2)
initGpioExpander(15-84)initGpioExpander(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: run
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: macOS
- GitHub Check: Linux
🔇 Additional comments (31)
Devices/m5stack-cores3/Source/InitBoot.cpp (2)
3-6: LGTM! Logging infrastructure correctly migrated.The header include and logger initialization properly align with the project-wide logging refactor. The static
LOGGERinstance with the "CoreS3" tag follows the established pattern.
16-147: Excellent! All logging calls correctly use std::format syntax.All logging statements have been properly migrated to the Logger API with correct
{}-style format placeholders. Notably, line 78 now correctly uses{:.2f}for floating-point formatting—the previous issue flagged in the past review has been resolved.Devices/unphone/Source/UnPhoneFeatures.cpp (2)
3-3: LGTM!The include migration from
Log.htoLogger.hand the staticLOGGERinstantiation follow the codebase-wide pattern for the new logging infrastructure.Also applies to: 12-13
78-78: LGTM!All error and warning log calls are consistent with the new
LoggerAPI. The messages are clear and descriptive for debugging initialization failures.Also applies to: 86-86, 93-93, 128-128, 138-138, 159-159, 170-170, 204-204, 207-207, 212-212, 217-217, 222-222, 289-289, 293-293
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (2)
5-5: LGTM: Logging infrastructure updated.The migration from legacy
TT_LOG_*to the newLoggerinterface is clean and consistent with the PR-wide refactor.Also applies to: 14-14
155-180: Error handling is now consistent - past concern resolved.The addition of
return false;statements afteresp_lcd_panel_invert_color(line 172) andesp_lcd_panel_disp_on_off(line 179) errors addresses the inconsistency flagged in the previous review. All initialization errors are now treated as fatal and follow a uniform pattern: log the error and returnfalse.Devices/heltec-wifi-lora-32-v3/Source/Configuration.cpp (1)
7-7: LGTM! Include migration complete.The include has been correctly updated to use the new
Tactility/Logger.hheader as part of the logging refactor, and the previously flagged duplicate include has been resolved.Tactility/Source/app/notes/Notes.cpp (4)
15-15: LGTM! Logger declaration follows the refactored API.The static const Logger instance is correctly initialized with the appropriate tag, replacing the legacy TAG constant as part of the project-wide logging refactor.
54-55: Previous issues resolved - launch IDs now logged correctly.The logging statements now correctly reference the variables that were just assigned (
saveFileLaunchIdon lines 55 and 67,loadFileLaunchIdon line 59). The issues flagged in previous review comments have been fixed.Also applies to: 58-59, 66-67
94-94: Previous issues resolved - formatting now consistent with Logger API.The logging statements now use
{}placeholders instead of printf-style%sformat specifiers, consistent with the new Logger API and the rest of the file. Thestd::stringarguments are passed directly, which works correctly withstd::format.Also applies to: 104-104
189-189: LGTM! Consistent use of the new Logger API.The logging statement correctly uses
{}formatting and logs the appropriate launch ID parameter.Tactility/Source/app/gpssettings/GpsSettings.cpp (5)
8-8: LGTM!Include addition is necessary for the new Logger API.
27-27: LGTM!The Logger member follows the established pattern for migrating from TAG-based logging to the new Logger API.
96-96: Temporary Logger is appropriate in static callback context.Since
onDeleteConfigurationis a static callback, it cannot access the instanceloggermember. Creating a temporary Logger here is the correct approach.
158-158: LGTM!The migration from legacy logging macros to
logger.debug()is correct, and the debug level is appropriate for state transition diagnostics.Also applies to: 167-167, 176-176, 185-185
258-260: LGTM!The lambda capture simplification (removing empty parameter lists) improves readability without changing behavior.
Also applies to: 262-264
Tactility/Source/hal/gps/Probe.cpp (4)
3-9: Logger setup looks good.The migration to the new logging system is properly implemented with the correct include and a well-scoped static Logger instance using an appropriate tag.
64-70: Debug logging correctly implemented.Both debug log calls properly use
{}placeholders with arguments passed separately, preventing format injection vulnerabilities. The previous critical issue at line 70 has been successfully resolved.
82-91: Macro migration to new logging system is correct.The
PROBE_SIMPLEmacro properly uses the new Logger with{}placeholders, and macro parameters are correctly passed as format arguments. The do-while(0) wrapper ensures safe usage in all contexts.
137-137: Format string migration completed correctly.The warning log now properly uses the
{}placeholder instead of the printf-style%lu, aligning with the std::format-based logging system. The previous critical issue has been successfully resolved.Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (1)
3-6: LGTM! Logging infrastructure migration looks correct.The migration from the old
TT_LOG_*macros to the newtt::Loggerinfrastructure is properly implemented. The static logger instance is correctly initialized with the appropriate tag.Drivers/BQ24295/Source/Bq24295.cpp (2)
45-56: Critical bug fixed and logging migration looks good!The previous critical issue has been resolved:
- Line 50 now correctly uses OR (
|) to combinebits_to_setwithvalue_cleared, fixing the bug where AND would always produce0- The logging migration on line 51 is properly structured with the new logger format
The watchdog timer configuration logic is now correct.
94-101: Logging migration looks good!The logging calls have been properly migrated to the new logger API with correct format specifiers (
{:02x}for hex bytes).Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp (6)
6-7: LGTM!The added includes are necessary for the Logger class and the existing
hal::hasDevicecheck.
16-16: LGTM!The Logger initialization follows the established pattern from the project-wide logging refactor.
47-57: LGTM!The logging calls correctly use the new Logger API with {}-style formatting, and the messages are clear and informative.
66-73: LGTM!The logging calls are correct and the messages are clear. Note: The double-space issue previously flagged for line 68 appears to have been resolved.
78-91: LGTM!The logging calls correctly use the new Logger API and the messages clearly indicate the operation being performed.
100-106: LGTM!The logging calls are correct and the error message is clear and explicit. Note: The message clarity issue previously flagged for line 103 appears to have been resolved.
TactilityCore/Include/Tactility/Check.h (2)
17-17: Include change aligns with logging refactor.The migration from "Log.h" to "Logger.h" is consistent with the PR's logging infrastructure changes.
34-42: Crash macro logging migration looks good.The transition from
TT_LOG_Etott::Logger("Kernel").error()with {}-style formatting is correct and preserves the original behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @Tactility/Source/app/files/View.cpp:
- Line 139: Update the three LOGGER.warn messages to use consistent
sentence-case capitalization: change "opening links is not supported" to
"Opening links is not supported" and ensure the other messages ("Opening files
of this type is not supported" and the one at line 167) match exactly; locate
and edit the LOGGER.warn calls that emit these strings (search for LOGGER.warn
with those literal messages in View.cpp) so all warnings begin with a capital
letter and use identical phrasing.
In @Tactility/Source/Tactility.cpp:
- Line 279: The mutex-lock failure log at Tactility.cpp uses LOGGER.error with
LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT and should be standardized to match the
existing pattern for recoverable mutex lock failures; change the call to
LOGGER.warn(LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, rootPath) (or the opposite across
all files if you decide errors are critical) and then audit other occurrences
(Sdcard.cpp, Screenshot*.cpp, I2cScanner.cpp, Loader.cpp, GuiService.cpp,
WifiEsp.cpp) to make the log level consistent for
LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT usage.
🧹 Nitpick comments (5)
Drivers/ButtonControl/Source/ButtonControl.h (1)
6-6: LGTM! Include is necessary for thestd::shared_ptr<tt::Thread>member.The addition of
Tactility/Thread.his correct and required for line 41'sstd::shared_ptr<tt::Thread> driverThreadmember.Optional: Forward declaration to reduce header dependencies
Since no inline functions in the header manipulate the
shared_ptr, you could use a forward declaration instead to minimize compilation dependencies:+namespace tt { class Thread; } #include <Tactility/hal/encoder/EncoderDevice.h> #include <Tactility/hal/gpio/Gpio.h> #include <Tactility/TactilityCore.h> -#include <Tactility/Thread.h>The implementation file would still include the full header. This is a minor header hygiene improvement, not essential.
Tactility/Source/network/HttpdReq.cpp (2)
26-26: Consider logging allocation size for consistency.For consistency with line 82 and better debugging, include the allocation size in the error message.
🔎 Proposed fix
- LOGGER.error( LOG_MESSAGE_ALLOC_FAILED); + LOGGER.error(LOG_MESSAGE_ALLOC_FAILED_FMT, header_size + 1);
184-184: Consider adding context to file operation errors.The error messages at lines 184 and 188 could be more helpful by including context such as the file path, bytes expected vs. received/written, and current progress.
🔎 Proposed enhancement
if (receive_chunk_size <= 0) { - LOGGER.error("Receive failed"); + LOGGER.error("Receive failed for file {}: received {}, expected {} ({}/{} bytes total)", + filePath, receive_chunk_size, expected_chunk_size, bytes_received, length); break; } if (fwrite(buffer, 1, receive_chunk_size, file) != receive_chunk_size) { - LOGGER.error("Failed to write all bytes"); + LOGGER.error("Failed to write all bytes to file {}: attempted {} bytes ({}/{} bytes total)", + filePath, receive_chunk_size, bytes_received, length); break; }Also applies to: 188-188
Tactility/Source/app/files/View.cpp (1)
376-388: Consider differentiating delete failure messages.Lines 379 and 385 both log "Failed to delete {}" but occur in different branches (directory vs. file deletion). Differentiating these messages would improve debugging and observability.
🔎 Suggested improvement
if (file::isDirectory(filepath)) { if (!file::deleteRecursively(filepath)) { - LOGGER.warn("Failed to delete {}", filepath); + LOGGER.warn("Failed to delete directory {}", filepath); } } else if (file::isFile(filepath)) { auto lock = file::getLock(filepath); lock->lock(); if (remove(filepath.c_str()) <= 0) { - LOGGER.warn("Failed to delete {}", filepath); + LOGGER.warn("Failed to delete file {}", filepath); } lock->unlock(); }Tactility/Source/app/i2cscanner/I2cScanner.cpp (1)
4-8: Logging migration implemented correctly.The Logger infrastructure is properly integrated with appropriate includes and the logger instance is correctly initialized with the "I2cScanner" tag. The approach aligns with the project-wide migration from TT_LOG_* macros.
Optional: More idiomatic initialization
Consider using direct initialization for the logger member:
- const Logger logger = Logger("I2cScanner"); + const Logger logger{"I2cScanner"};This avoids the copy constructor and is more concise.
Also applies to: 25-25
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Drivers/ButtonControl/Source/ButtonControl.hDrivers/FT6x36/Source/Ft6x36Touch.hDrivers/PwmBacklight/Source/PwmBacklight.cppTactility/Source/Tactility.cppTactility/Source/app/files/View.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/wificonnect/View.cppTactility/Source/network/HttpdReq.cppTactilityCore/Include/Tactility/Check.h
🚧 Files skipped from review as they are similar to previous changes (2)
- TactilityCore/Include/Tactility/Check.h
- Drivers/PwmBacklight/Source/PwmBacklight.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/files/View.cppTactility/Source/app/i2cscanner/I2cScanner.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/files/View.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/files/View.cpp
🧬 Code graph analysis (3)
Tactility/Source/Tactility.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/service/sdcard/Sdcard.cpp (1)
sdcard(30-53)
Tactility/Source/app/files/View.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Tactility/Source/app/i2cscanner/I2cScanner.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: run
🔇 Additional comments (16)
Drivers/FT6x36/Source/Ft6x36Touch.h (1)
5-5: LGTM!The include is necessary for the
tt::Thread(line 39) andtt::Mutex(line 41) types used in the class.Tactility/Source/app/wificonnect/View.cpp (2)
5-5: LGTM! Clean logging migration.The logging infrastructure has been correctly migrated from TT_LOG_* macros to the Logger interface. The error messages are clear and appropriate for the validation failures.
Also applies to: 16-16, 34-34, 43-43
203-203: Good catch on the parameter bug fix.Corrected the call to use the
passwordvariable instead ofssid, ensuring the password parameter is extracted into the correct variable. This resolves a logic bug where the password would have been incorrectly read.Tactility/Source/network/HttpdReq.cpp (4)
2-3: LGTM!Header changes correctly reflect the migration from legacy Log macros to the new Logger interface.
15-15: LGTM!Module-specific logger declaration follows the correct pattern for the new logging infrastructure.
82-82: LGTM!Allocation and receive error logging provide good context for debugging, with appropriate parameters included in the log messages.
Also applies to: 90-90
176-176: LGTM!Good error logging that includes the file path for debugging.
Tactility/Source/app/files/View.cpp (3)
11-12: LGTM!The new includes are appropriate for the logging refactor. Logger.h provides the Logger interface, LogMessages.h provides standardized message constants (used on line 309), and cstring is needed for strlen() on line 94.
Also applies to: 19-19
28-28: LGTM!The static Logger instance is correctly initialized with an appropriate tag for the Files module.
87-460: Excellent logging refactor!The migration from TT_LOG_* macros to the new Logger interface is well-executed:
- All format strings correctly use {} placeholders
- Log levels are appropriately chosen
- Parameters match the format placeholders
- The LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT constant is properly used for standardized messages
Tactility/Source/app/i2cscanner/I2cScanner.cpp (3)
195-195: Mutex lock failure handling is appropriate.The logging of mutex lock failures provides valuable debugging context by including the function name. The log calls are correctly placed outside the mutex locks themselves, and severity levels (warn vs. error) are appropriately chosen.
Also applies to: 206-206, 267-267, 290-290, 299-299, 383-383, 392-392, 405-405
320-320: Bus selection logging is clear and appropriate.The log message provides useful information for debugging bus selection operations.
222-256: Scanning logic and logging implementation are correct.The logger calls provide clear visibility into the scanning process with appropriate log levels. The hex formatting (
0x{:02X}) correctly displays I2C addresses, and the early exit checks withshouldStopScanTimer()ensure responsive cancellation. Thehal::i2c::masterHasDeviceAtAddress()function signature is confirmed to exist with the expected parameters.The timeout calculation
10 / portTICK_PERIOD_MSon line 238 follows the standard pattern throughout the codebase. While the theoretical edge case ofportTICK_PERIOD_MSexceeding 10 could result in a timeout of 0 ticks, this is negligible in practice as FreeRTOS configurations typically use 1ms tick periods.Tactility/Source/Tactility.cpp (3)
10-10: LGTM! Includes added for new logging infrastructure.The new includes support the migration to the Logger-based logging system.
Also applies to: 15-16, 18-18
34-34: LGTM! Logger instance properly declared.The static Logger instance with "Tactility" as the tag correctly replaces the old TAG-based approach.
122-122: LGTM! Logger-based logging correctly implemented.The migration from TAG-based macros to Logger methods is consistent throughout. Format strings properly use {} placeholders and argument counts match.
Also applies to: 183-183, 186-186, 192-192, 198-198, 209-209, 231-231, 238-238, 253-253, 274-274, 276-276, 282-282, 310-310, 330-330, 332-332, 337-337
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @Drivers/DRV2605/Source/Drv2605.cpp:
- Around line 7-16: The constructor Drv2605::Drv2605 currently logs init failure
but continues and may call setWaveFormForBuzz() / startPlayback() even when
init() returned false; modify the constructor so autoplay (the block using
setWaveFormForBuzz and startPlayback) only runs when init() succeeds (e.g.,
guard the autoplay with the successful init result), and optionally add a public
bool isInitialized() or an initialized_ member that init() sets so callers can
check device readiness before using other methods.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Devices/lilygo-tlora-pager/Source/Init.cppDrivers/DRV2605/Source/Drv2605.cppDrivers/DRV2605/Source/Drv2605.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Devices/lilygo-tlora-pager/Source/Init.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
Drivers/DRV2605/Source/Drv2605.h (1)
Drivers/DRV2605/Source/Drv2605.cpp (1)
Drv2605(7-16)
Drivers/DRV2605/Source/Drv2605.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/hal/i2c/I2cDevice.cpp (2)
readRegister8(53-55)readRegister8(53-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: run
- GitHub Check: macOS
🔇 Additional comments (1)
Drivers/DRV2605/Source/Drv2605.h (1)
65-65: LGTM - proper constructor declaration.The constructor declaration correctly uses
explicitto prevent implicit conversions and provides a sensible default forautoPlayStartupBuzz.
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp:
- Line 44: The assert(encPcntUnit == nullptr) in initEncoder() should be
replaced with a runtime check that returns false when already initialized: check
if encPcntUnit is non-null at the start of initEncoder(), log or handle the
condition as appropriate, and return false instead of asserting to avoid
crashing in debug builds; update any callers if needed to handle the boolean
return.
- Line 152: Replace the hard assert in deinitEncoder() that checks encPcntUnit
with a runtime null check: in the deinitEncoder() function, test if encPcntUnit
== nullptr and if so simply return (or gracefully skip cleanup) instead of
asserting; otherwise proceed with the existing cleanup logic and set encPcntUnit
to nullptr after releasing resources. Ensure no assert(...) calls remain for
this pointer so deinitEncoder() won’t crash in debug builds when called on an
uninitialized encoder.
- Around line 60-140: The pulse counter unit allocated by
pcnt_new_unit(encPcntUnit) is never freed on subsequent failures; ensure you
call the appropriate cleanup (pcnt_unit_del(encPcntUnit) and set encPcntUnit =
nullptr) before returning false on any error after pcnt_new_unit succeeds. Add
this cleanup to every error path that follows pcnt_new_unit: after
pcnt_unit_set_glitch_filter, after pcnt_new_channel, after
pcnt_channel_set_edge_action, after pcnt_channel_set_level_action, after
pcnt_unit_add_watch_point, after pcnt_unit_enable, after pcnt_unit_clear_count,
and after pcnt_unit_start (referencing encPcntUnit and the failing functions by
name in the diff).
- Around line 158-161: After deleting the pulse counter unit in the block where
pcnt_del_unit(encPcntUnit) returns ESP_OK, set encPcntUnit to nullptr to avoid a
dangling pointer; update the deletion branch inside the function that calls
pcnt_del_unit (reference encPcntUnit and deinitEncoder()) so that after
successful deletion you assign encPcntUnit = nullptr, and ensure any later
checks (e.g., in deinitEncoder(), startLvgl(), stopLvgl()) rely on encPcntUnit
being nullptr to detect the deleted state.
In @Drivers/DRV2605/Source/Drv2605.cpp:
- Around line 8-18: The constructor Drv2605::Drv2605 currently calls tt_crash()
on init() failure; remove the crash and refactor so initialization failure can
be handled by callers: either (A) implement two‑phase init by making init()
public and store an internal bool initialized_ set from init() and skip
setWaveFormForBuzz()/startPlayback() unless initialized_, or (B) add a static
factory like Drv2605::Create(i2c_port_t, bool) that returns
std::optional<Drv2605> (or a unique_ptr) and move the init() call out of the raw
constructor into the factory, or (C) throw an exception from the constructor on
init() failure if your build permits exceptions; in all cases remove tt_crash()
and ensure setWaveFormForBuzz() and startPlayback() are only invoked after
successful initialization.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Devices/cyd-4848s040c/Source/devices/St7701Display.cppDevices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cppDevices/lilygo-tlora-pager/Source/devices/TpagerEncoder.hDrivers/DRV2605/Source/Drv2605.cppTactilityCore/Include/Tactility/Check.h
🚧 Files skipped from review as they are similar to previous changes (1)
- TactilityCore/Include/Tactility/Check.h
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp
🧬 Code graph analysis (4)
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Drivers/DRV2605/Source/Drv2605.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Tactility/Source/hal/i2c/I2cDevice.cpp (2)
readRegister8(53-55)readRegister8(53-53)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (2)
Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cpp (2)
startLvgl(131-149)startLvgl(131-131)Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cpp (2)
startLvgl(118-142)startLvgl(118-118)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.h (1)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (4)
initEncoder(43-143)initEncoder(43-43)deinitEncoder(151-166)deinitEncoder(151-151)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: run
🔇 Additional comments (7)
Devices/cyd-4848s040c/Source/devices/St7701Display.cpp (2)
5-5: LGTM: Logger migration implemented correctly.The include change and static LOGGER instance follow the project-wide migration pattern to the new Logger API.
Also applies to: 14-14
155-183: LGTM: Error handling is now consistent across all panel operations.All ESP LCD API calls in
createPanelHandlenow follow a uniform pattern: check forESP_OK, log a descriptive error withLOGGER.error(), and returnfalseon failure. The error handling foresp_lcd_panel_invert_color(lines 170-173) andesp_lcd_panel_disp_on_off(lines 177-180) now aligns with the fatal treatment of other initialization failures.Drivers/DRV2605/Source/Drv2605.cpp (1)
23-23: Logging migration looks good.The conversion from
TT_LOG_EtoLogger.error()is correct, including the proper enum cast at line 30. Once the staticLOGGERis replaced with the instance memberlogger, these calls will be fully aligned with the new logging infrastructure.Also applies to: 30-30, 45-45
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.h (1)
11-12: LGTM! Improved error handling.The signature change from
voidtoboolallows proper error propagation for initialization and deinitialization failures, which is correctly utilized in the implementation.Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (3)
3-7: LGTM! Logger migration.The migration from the legacy
TT_LOG_*macros to the newLoggerinterface is correct and consistent with the PR's objectives. The logger tag matches the class name, which is good for log filtering.
170-172: Verify initialization logic is correct.The conditional initialization allows
startLvgl()to be called multiple times, with the first call initializing the encoder. Ensure this is the intended behavior and thatstopLvgl()properly cleans up so subsequentstartLvgl()calls work correctly.This depends on fixing the resource leak in
initEncoder()and the missingnullptrassignment indeinitEncoder().
190-193: LGTM! Reasonable error handling.The decision to continue returning
trueeven ifdeinitEncoder()fails is reasonable since the LVGL resources are effectively cleaned up. The comment clearly explains this design choice.This depends on fixing the missing
nullptrassignment indeinitEncoder().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @Drivers/BQ24295/Source/Bq24295.cpp:
- Line 4: The LOGGER instance is using the wrong tag "St7701Display" due to
copy/paste; update the static const auto LOGGER = tt::Logger(...) declaration in
Bq24295.cpp to use a correct, descriptive tag for this module (e.g., "Bq24295"
or "BQ24295") so all log messages from the Bq24295 driver are properly labeled.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Devices/unphone/Source/UnPhoneFeatures.cppDrivers/BQ24295/Source/Bq24295.cppDrivers/BQ24295/Source/Bq24295.h
💤 Files with no reviewable changes (1)
- Drivers/BQ24295/Source/Bq24295.h
🧰 Additional context used
🧬 Code graph analysis (2)
Devices/unphone/Source/UnPhoneFeatures.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
Drivers/BQ24295/Source/Bq24295.cpp (1)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: run
🔇 Additional comments (8)
Drivers/BQ24295/Source/Bq24295.cpp (2)
52-54: LGTM: Critical bug fix correctly applied.The bitwise operation correctly uses OR to combine the cleared register value with the new watchdog bits. The logger migration is also properly implemented with the new format syntax.
99-101: LGTM: Logger migration correctly implemented.The logging calls are properly migrated to the new Logger API with correct format string syntax.
Devices/unphone/Source/UnPhoneFeatures.cpp (6)
3-3: LGTM!Logger include and static instance setup follows the established pattern used across the codebase.
Also applies to: 12-13
134-136: LGTM!The
reinterpret_cast<void*>is the correct C++ idiom for passing integer values as ISR userdata.
45-45: LGTM!Format string correctly uses
{}placeholder compatible with thestd::format-based Logger API.
234-234: LGTM!Correct
{}format placeholder for the backlight state string.
78-78: LGTM!All error logging calls are consistent and appropriately placed at failure points in the initialization flow.
Also applies to: 86-86, 93-93, 128-128, 138-138, 159-159, 170-170, 207-207, 212-212, 217-217, 222-222
204-204: LGTM!Appropriate log levels:
infofor initialization entry,warnfor shipping mode changes which affect power management behavior.Also applies to: 289-289, 293-293
This might be related to the hardware buttons driver
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp:
- Line 168: Replace the assert in deinitEncoder() that checks encPcntUnit with a
runtime null-check: in function deinitEncoder(), instead of assert(encPcntUnit
!= nullptr), check if (encPcntUnit == nullptr) and simply return (or log and
return) to safely handle double/uninitialized deinitialization; then proceed
with the existing cleanup code when encPcntUnit is non-null so you avoid crash
in non-initialized cases while preserving cleanup behavior.
- Line 44: Replace the debug-only assertion in initEncoder() that checks
encPcntUnit with a runtime guard: if encPcntUnit is already non-null, do not
assert—log or handle as appropriate and return false immediately; otherwise
proceed with initialization and return true. Ensure the check references
encPcntUnit and the initEncoder() return path is updated to return false on this
early-exit condition.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Buildscripts/sdkconfig/default.propertiesDevices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cppDrivers/BQ24295/Source/Bq24295.cpp
💤 Files with no reviewable changes (1)
- Buildscripts/sdkconfig/default.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- Drivers/BQ24295/Source/Bq24295.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (2)
TactilityCore/Include/Tactility/Logger.h (1)
Logger(22-39)Drivers/XPT2046SoftSPI/Source/Xpt2046SoftSpi.cpp (2)
startLvgl(131-149)startLvgl(131-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: run
🔇 Additional comments (3)
Devices/lilygo-tlora-pager/Source/devices/TpagerEncoder.cpp (3)
3-7: LGTM!The Logger introduction follows the new API pattern correctly, with an appropriately named tag matching the class.
186-201: LGTM!The lazy initialization pattern is well-implemented: the encoder is initialized on-demand if not already initialized, and initialization failures are properly propagated. The LVGL setup follows standard patterns.
203-213: Graceful cleanup pattern looks good.The function correctly attempts encoder deinitialization when the encoder is initialized, and the decision to return
trueeven if deinitialization fails is reasonable since the primary contract (stopping LVGL) has been fulfilled. The comment on line 208 appropriately documents this behavior.Note: This function's correctness depends on fixing the
deinitEncoder()nullptr assignment issue flagged above.
TT_LOG_*macros are replaced byLoggervia#include<Tactility/Logger.h>TODO:
Also tested on: