New kernel drivers, filesystem API, and more#513
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the SD card architecture from a device-centric to filesystem-centric approach and adds new hardware support. The SdmmcDevice class is removed and replaced with a new FileSystem abstraction layer that provides a unified interface for managing mounted filesystems. A new BMI270 6-axis IMU driver module is added with complete hardware integration. The GPIO controller API is extended with interrupt and callback support, and the pi4ioe5v6408 I/O expander driver is refactored to use the new GPIO controller API. Platform-specific ESP32 SD/MMC driver and device tree bindings are added. Device configurations for multiple boards (lilygo-tdongle-s3, lilygo-thmi-s3, waveshare-esp32-s3-geek) are updated to use the new device tree-based SD/MMC configuration. Applications are refactored to discover storage via filesystem traversal instead of device enumeration. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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 (4)
Tactility/Source/service/sdcard/Sdcard.cpp (1)
31-44:⚠️ Potential issue | 🟠 MajorAdd nullptr guard before calling
file_system_is_mountedin theupdate()method.If the SD card is removed at runtime after
onStart()passes,findFirstSdcardFileSystem()will returnnullptron the nextupdate()call. Callingfile_system_is_mounted(nullptr)immediately dereferences the null pointer and crashes. Other code in the codebase (e.g.,Statusbar.cpp:169,Sdcard.cpp:50) correctly guards against this scenario.🛡️ Proposed fix
void update() { // TODO: Support multiple SD cards auto* file_system = findFirstSdcardFileSystem(); if (lock(50)) { - auto is_mounted = file_system_is_mounted(file_system); - if (is_mounted != lastMountedState) { - lastMountedState = is_mounted; - } + bool is_mounted = (file_system != nullptr) && file_system_is_mounted(file_system); + lastMountedState = is_mounted; unlock(); } else { LOGGER.warn(LOG_MESSAGE_MUTEX_LOCK_FAILED); } }TactilityKernel/include/tactility/drivers/gpio.h (1)
12-29:⚠️ Potential issue | 🟠 MajorUpdate
GPIO_FLAGS_MASKto include the new high-impedance bit.
GPIO_FLAG_HIGH_IMPEDANCElives at bit 8, butGPIO_FLAGS_MASKstill only covers bits 0-4. Any caller that filters flags through this mask will drop the new option, so the flag cannot round-trip reliably.Suggested fix
-#define GPIO_FLAGS_MASK 0x1f +#define GPIO_FLAGS_MASK 0x11fTactilityKernel/source/kernel_symbols.c (1)
1-15:⚠️ Potential issue | 🟡 MinorAdd direct include for
log_genericdeclaration.Line 163 exports
DEFINE_MODULE_SYMBOL(log_generic), but the declaring header is not directly included. Currently the symbol is accessible only through transitive includes:event_group.h→check.h→log.h. This undermines the file's stated purpose: "compile headers as C to catch errors that only show up when compiling as C and not as C++." Add the direct include to maintain include hygiene and prevent brittleness from future refactoring.Suggested change
`#include` <tactility/error.h> `#include` <tactility/filesystem/file_system.h> +#ifndef ESP_PLATFORM +#include <tactility/log.h> +#endif `#include` <tactility/module.h>Tactility/Source/service/statusbar/Statusbar.cpp (1)
166-179:⚠️ Potential issue | 🟡 MinorConsider hiding the SD card icon when no SD card filesystem exists.
When
findFirstSdcardFileSystem()returnsnullptr, the icon visibility is not updated. If an SD card was previously detected and then removed (physically or the filesystem unregistered), the icon would remain visible with stale state.Depending on the intended behavior, you may want to hide the icon when no SD card filesystem is found:
💡 Suggested enhancement
void updateSdCardIcon() { auto* sdcard_fs = findFirstSdcardFileSystem(); // TODO: Support multiple SD cards if (sdcard_fs != nullptr) { auto mounted = file_system_is_mounted(sdcard_fs); auto* desired_icon = getSdCardStatusIcon(mounted); if (sdcard_last_icon != desired_icon) { lvgl::statusbar_icon_set_image(sdcard_icon_id, desired_icon); lvgl::statusbar_icon_set_visibility(sdcard_icon_id, true); sdcard_last_icon = desired_icon; } // TODO: Consider tracking how long the SD card has been in unknown status and then show error + } else if (sdcard_last_icon != nullptr) { + lvgl::statusbar_icon_set_visibility(sdcard_icon_id, false); + sdcard_last_icon = nullptr; } }
🟡 Minor comments (8)
TactilityC/Source/tt_init.cpp-421-431 (1)
421-431:⚠️ Potential issue | 🟡 MinorClarify that
ledc_set_fadeis superseded, not deprecated.
ledc_set_fadeis not formally deprecated but is superseded in favor of time-based (ledc_set_fade_with_time,ledc_set_fade_time_and_start) or step-based (ledc_set_fade_with_step,ledc_set_fade_step_and_start) alternatives for typical fade operations. Consider exporting these newer APIs for alignment with modern ESP-IDF best practices.Minor: Comment could be
// driver/ledc.hfor consistency with other header sections.TactilityKernel/include/tactility/drivers/wifi.h-131-137 (1)
131-137:⚠️ Potential issue | 🟡 MinorDocumentation error: comment describes IPv6 but function returns SSID.
The doc comment describes "Get the IPv6 address" but the function is
station_get_target_ssid. This appears to be a copy-paste error.📝 Proposed fix
/** - * Get the IPv6 address of the device. + * Get the target SSID of the station. * `@param`[in] device the device - * `@param`[out] ipv6 the buffer to store the IPv6 address (must be at least 33 bytes, will be null-terminated) + * `@param`[out] ssid the buffer to store the SSID (must be at least 33 bytes, will be null-terminated) * `@return` ERROR_NONE on success */ error_t (*station_get_target_ssid)(struct Device* device, char* ssid);TactilityKernel/include/tactility/drivers/wifi.h-139-147 (1)
139-147:⚠️ Potential issue | 🟡 MinorClarify password documentation.
The comment states password "must be at least 33 characters" which seems incorrect. Wi-Fi passwords have a maximum length (63 for WPA2-PSK), not a minimum of 33. This likely should describe the buffer size expectation, not a password length requirement.
📝 Suggested clarification
/** * Connect to an access point. * `@param`[in] device the wifi device * `@param`[in] ssid the SSID of the access point - * `@param`[in] password the password of the access point (must be at least 33 characters and null-terminated) + * `@param`[in] password the password of the access point (null-terminated, max 63 characters for WPA2) * `@param`[in] channel the Wi-Fi channel to connect to (0 means "any" / no preference) * `@return` ERROR_NONE on success */Tactility/Source/hal/usb/Usb.cpp-39-44 (1)
39-44:⚠️ Potential issue | 🟡 MinorKeep iterating until a usable SDMMC card is found.
The callback returns
falseon the first compatible SDMMC device even whenesp32_sdmmc_get_card(device)returnsnullptr. That makes discovery fail early in multi-controller or partially initialized setups.Suggested fix
if (sdcard == nullptr) { device_for_each(&sdcard, [](auto* device, void* context) { if (device_is_ready(device) && device_is_compatible(device, "espressif,esp32-sdmmc")) { auto** sdcard = static_cast<sdmmc_card_t**>(context); *sdcard = esp32_sdmmc_get_card(device); - return false; + return *sdcard == nullptr; } return true; }); }TactilityKernel/include/tactility/drivers/gpio_controller.h-204-211 (1)
204-211:⚠️ Potential issue | 🟡 MinorFix the return docs for
gpio_controller_get_controller_context().This function returns a context pointer, not an
error_t, so the current@return ERROR_NONE if successfultext is misleading.Tactility/Source/Tactility.cpp-232-244 (1)
232-244:⚠️ Potential issue | 🟡 MinorMinor: Log message shows root path instead of app path.
The log message on line 239 logs
path(the filesystem root), butregisterInstalledAppsis called withapp_path(the/appsubdirectory). For clarity, consider logging the actual path being scanned.📝 Suggested fix
const auto app_path = std::format("{}/app", path); if (!app_path.starts_with(file::MOUNT_POINT_SYSTEM) && file::isDirectory(app_path)) { - LOGGER.info("Registering apps from {}", path); + LOGGER.info("Registering apps from {}", app_path); registerInstalledApps(app_path); }Tactility/Source/service/webserver/WebServerService.cpp-775-784 (1)
775-784:⚠️ Potential issue | 🟡 MinorMinor: Inconsistent error comparison.
Line 778 compares
file_system_get_pathresult toESP_OK, but this function returnserror_t. WhileESP_OKandERROR_NONEare both typically 0, usingERROR_NONEwould be more consistent with the filesystem API contract.Proposed fix
- if (file_system_is_mounted(fs) && file_system_get_path(fs, path, sizeof(path)) == ESP_OK && strcmp(path, "/system") != 0) { + if (file_system_is_mounted(fs) && file_system_get_path(fs, path, sizeof(path)) == ERROR_NONE && strcmp(path, "/system") != 0) {Platforms/platform-esp32/source/drivers/esp32_sdmmc.cpp-117-120 (1)
117-120:⚠️ Potential issue | 🟡 MinorInconsistent logging macro and unused variable.
- Line 118 uses
ESP_LOGIwhile the rest of the file usesLOG_I- should be consistent.- Line 120 declares
dts_configbut it's never used.Proposed fix
static error_t stop(Device* device) { - ESP_LOGI(TAG, "stop %s", device->name); + LOG_I(TAG, "stop %s", device->name); auto* data = GET_DATA(device); - auto* dts_config = GET_CONFIG(device); if (file_system_is_mounted(data->file_system)) {
🧹 Nitpick comments (9)
Devices/guition-jc1060p470ciwy/Source/devices/SdCard.cpp (1)
21-21: TODO comment added to track pending migration.The TODO clearly documents that this implementation still needs migration to the new
espressif,esp32-sdmmcdriver and identifies the LDO code porting as the blocking dependency.Would you like me to open a tracking issue for this migration task? I can help document the LDO-specific code (lines 63-77) that needs to be ported and the context from this PR's broader migration effort.
Tactility/Source/service/sdcard/Sdcard.cpp (1)
48-63: Consider initializinglastMountedStateto actual mount status.The service starts with
lastMountedState = false, but the SD card may already be mounted. This means the firstupdate()call will detect a "state change" even if nothing changed since service start.♻️ Proposed fix to initialize state correctly
bool onStart(ServiceContext& serviceContext) override { auto* sdcard_fs = findFirstSdcardFileSystem(); if (sdcard_fs == nullptr) { LOGGER.warn("No SD card device found - not starting Service"); return false; } + // Initialize to actual mounted state + lastMountedState = file_system_is_mounted(sdcard_fs); + auto service = findServiceById<SdCardService>(manifest.id); updateTimer = std::make_unique<Timer>(Timer::Type::Periodic, 1000, [service] { service->update(); });TactilityKernel/include/tactility/filesystem/file_system.h (1)
21-33: Document the handle and iteration contract before this API spreads.This header exposes raw
FileSystem*handles and abool-returning iterator callback, but it never defines ownership, retention rules, or whether the callback returnstrueto continue or to stop. That ambiguity is easy to bake into downstream callers once this becomes the public surface.📝 Suggested header docs
struct FileSystem* file_system_add(const struct FileSystemApi* fs_api, void* data); void file_system_remove(struct FileSystem* fs); +/** + * Iterate over registered file systems. + * `@param` callback_context User-provided context passed to `callback`. + * `@param` callback Return true to continue iteration, false to stop. + * `@note` `FileSystem*` handles are owned by the registry; callers must not free + * them and should not retain them past their valid lifetime. + */ void file_system_for_each(void* callback_context, bool (*callback)(struct FileSystem* fs, void* context));Tactility/Include/Tactility/Paths.h (1)
12-16: Align the new helper names with the existingSdCardspelling.
findFirstMountedSdCardPath()already establishes the public naming in this header. AddingfindFirstMountedSdcardFileSystem()andfindFirstSdcardFileSystem()freezes two spellings for the same concept into the API.✏️ Suggested rename
bool hasMountedSdCard(); -FileSystem* findFirstMountedSdcardFileSystem(); +FileSystem* findFirstMountedSdCardFileSystem(); -FileSystem* findFirstSdcardFileSystem(); +FileSystem* findFirstSdCardFileSystem();Tactility/Source/app/fileselection/State.cpp (1)
36-62: Consider thread safety consistency with files/State.cpp.The
files/State.cppversion ofsetEntriesForPathacquires a mutex lock before modifyingdir_entriesandcurrent_path, but this implementation does not. Ifdir_entriescan be accessed concurrently (e.g., viagetDirentwhich does use a mutex), there may be a race condition when updating the entries.Tactility/Source/PartitionsEsp.cpp (1)
95-103: Prefer static storage for these fixed mount descriptors.Lines 95 and 103 allocate process-lifetime
PartitionFsDataobjects on the heap. These mount points are singletons, so static storage makes the ownership explicit and avoids leaking two small objects at boot.♻️ Suggested change
} else { LOGGER.info("Mounted /system"); - file_system_add(&partition_fs_api, new PartitionFsData("/system")); + static PartitionFsData system_fs_data{"/system"}; + file_system_add(&partition_fs_api, &system_fs_data); } @@ } else { LOGGER.info("Mounted /data"); - file_system_add(&partition_fs_api, new PartitionFsData("/data")); + static PartitionFsData data_fs_data{"/data"}; + file_system_add(&partition_fs_api, &data_fs_data); }Tactility/Source/MountPoints.cpp (1)
16-17: Minor: Redundantclear()call.The
dir_entriesvector was just default-constructed and is already empty. Theclear()call on line 17 is unnecessary.♻️ Proposed fix
std::vector<dirent> getFileSystemDirents() { std::vector<dirent> dir_entries; - dir_entries.clear(); file_system_for_each(&dir_entries, [](auto* fs, void* context) {Tactility/Source/Paths.cpp (1)
25-53: Optional: Reduce duplication betweenfindFirstMountedSdcardFileSystem()andfindFirstSdcardFileSystem().These two functions share nearly identical logic, differing only in whether they check
file_system_is_mounted(). Consider extracting a shared helper to reduce duplication.♻️ Proposed refactor
+static FileSystem* findSdcardFileSystem(bool mustBeMounted) { + FileSystem* found = nullptr; + file_system_for_each(&found, [](auto* fs, void* context) { + char path[128]; + if (file_system_get_path(fs, path, sizeof(path)) != ERROR_NONE) return true; + // TODO: Find a better way to identify SD card paths + if (std::string(path).starts_with("/sdcard")) { + *static_cast<FileSystem**>(context) = fs; + return false; + } + return true; + }); + if (found && mustBeMounted && !file_system_is_mounted(found)) { + return nullptr; + } + return found; +} + FileSystem* findFirstMountedSdcardFileSystem() { - FileSystem* found = nullptr; - file_system_for_each(&found, [](auto* fs, void* context) { - char path[128]; - if (file_system_get_path(fs, path, sizeof(path)) != ERROR_NONE) return true; - // TODO: Find a better way to identify SD card paths - if (std::string(path).starts_with("/sdcard") && file_system_is_mounted(fs)) { - *static_cast<FileSystem**>(context) = fs; - return false; - } - return true; - }); - return found; + return findSdcardFileSystem(true); } FileSystem* findFirstSdcardFileSystem() { - FileSystem* found = nullptr; - file_system_for_each(&found, [](auto* fs, void* context) { - char path[128]; - if (file_system_get_path(fs, path, sizeof(path)) != ERROR_NONE) return true; - // TODO: Find a better way to identify SD card paths - if (std::string(path).starts_with("/sdcard")) { - *static_cast<FileSystem**>(context) = fs; - return false; - } - return true; - }); - return found; + return findSdcardFileSystem(false); }Platforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cpp (1)
156-160: Inconsistent null check inis_mounted.The function checks
fs_data == nullptrbut based on the project's Linux kernel style guideline, internal APIs should assume valid pointers. However, more importantly, checkingfs_data->card == nullptrbefore callingsdmmc_get_statusis correct since the card may not be mounted yet.Consider removing the
fs_data == nullptrcheck for consistency with project style, or document this as a public API boundary where null checks are appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74e8e770-aeb3-4a32-8a7b-d3c3559ccc4a
📒 Files selected for processing (78)
Devices/guition-jc1060p470ciwy/Source/devices/SdCard.cppDevices/lilygo-tdongle-s3/Source/Configuration.cppDevices/lilygo-tdongle-s3/Source/devices/Sdcard.cppDevices/lilygo-tdongle-s3/Source/devices/Sdcard.hDevices/lilygo-tdongle-s3/device.propertiesDevices/lilygo-tdongle-s3/lilygo,tdongle-s3.dtsDevices/lilygo-thmi-s3/Source/Configuration.cppDevices/lilygo-thmi-s3/Source/devices/SdCard.cppDevices/lilygo-thmi-s3/Source/devices/SdCard.hDevices/lilygo-thmi-s3/lilygo,thmi-s3.dtsDevices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/devicetree.yamlDevices/m5stack-tab5/m5stack,tab5.dtsDevices/waveshare-esp32-s3-geek/Source/Configuration.cppDevices/waveshare-esp32-s3-geek/Source/devices/SdCard.cppDevices/waveshare-esp32-s3-geek/Source/devices/SdCard.hDevices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dtsDrivers/bmi270-module/CMakeLists.txtDrivers/bmi270-module/LICENSE-Apache-2.0.mdDrivers/bmi270-module/README.mdDrivers/bmi270-module/bindings/bosch,bmi270.yamlDrivers/bmi270-module/devicetree.yamlDrivers/bmi270-module/include/bindings/bmi270.hDrivers/bmi270-module/include/bmi270_module.hDrivers/bmi270-module/include/drivers/bmi270.hDrivers/bmi270-module/private/drivers/bmi270_config_data.hDrivers/bmi270-module/source/bmi270.cppDrivers/bmi270-module/source/module.cppDrivers/bmi270-module/source/symbols.cDrivers/pi4ioe5v6408-module/include/drivers/pi4ioe5v6408.hDrivers/pi4ioe5v6408-module/source/module.cppDrivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cppDrivers/pi4ioe5v6408-module/source/symbols.cModules/lvgl-module/source/symbols.cPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/bindings/espressif,esp32-sdmmc.yamlPlatforms/platform-esp32/include/tactility/bindings/esp32_sdmmc.hPlatforms/platform-esp32/include/tactility/drivers/esp32_sdmmc.hPlatforms/platform-esp32/private/tactility/drivers/esp32_sdmmc_fs.hPlatforms/platform-esp32/source/drivers/esp32_gpio.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cppPlatforms/platform-esp32/source/drivers/esp32_spi.cppPlatforms/platform-esp32/source/module.cppTactility/Include/Tactility/MountPoints.hTactility/Include/Tactility/Paths.hTactility/Include/Tactility/hal/sdcard/SdCardDevice.hTactility/Include/Tactility/hal/sdcard/SdmmcDevice.hTactility/Source/MountPoints.cppTactility/Source/PartitionsEsp.cppTactility/Source/Paths.cppTactility/Source/Tactility.cppTactility/Source/app/files/FilesApp.cppTactility/Source/app/files/State.cppTactility/Source/app/fileselection/State.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/hal/sdcard/SdCardDevice.cppTactility/Source/hal/sdcard/SdmmcDevice.cppTactility/Source/hal/usb/Usb.cppTactility/Source/service/sdcard/Sdcard.cppTactility/Source/service/statusbar/Statusbar.cppTactility/Source/service/webserver/WebServerService.cppTactility/Source/service/wifi/WifiBootSplashInit.cppTactility/Source/settings/BootSettings.cppTactilityC/Source/symbols/stl.cppTactilityC/Source/tt_init.cppTactilityKernel/include/tactility/drivers/gpio.hTactilityKernel/include/tactility/drivers/gpio_controller.hTactilityKernel/include/tactility/drivers/gpio_descriptor.hTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/include/tactility/filesystem/file_system.hTactilityKernel/include/tactility/module.hTactilityKernel/source/drivers/gpio_controller.cppTactilityKernel/source/filesystem/file_system.cppTactilityKernel/source/kernel_symbols.cTactilityKernel/source/module.cpp
💤 Files with no reviewable changes (13)
- Devices/lilygo-tdongle-s3/Source/devices/Sdcard.h
- Devices/lilygo-tdongle-s3/Source/devices/Sdcard.cpp
- Tactility/Source/hal/sdcard/SdmmcDevice.cpp
- Devices/lilygo-thmi-s3/Source/Configuration.cpp
- Drivers/pi4ioe5v6408-module/include/drivers/pi4ioe5v6408.h
- Tactility/Source/app/files/FilesApp.cpp
- Tactility/Include/Tactility/hal/sdcard/SdmmcDevice.h
- Drivers/pi4ioe5v6408-module/source/symbols.c
- Devices/lilygo-thmi-s3/Source/devices/SdCard.cpp
- Devices/waveshare-esp32-s3-geek/Source/devices/SdCard.h
- Devices/waveshare-esp32-s3-geek/Source/devices/SdCard.cpp
- Devices/waveshare-esp32-s3-geek/Source/Configuration.cpp
- Devices/lilygo-thmi-s3/Source/devices/SdCard.h
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
Platforms/platform-esp32/source/drivers/esp32_sdmmc.cpp (1)
118-121: Inconsistent logging macro and unused variable.Line 119 uses
ESP_LOGIwhile the rest of the file usesLOG_I/LOG_E. Additionally,dts_configon line 121 is declared but never used.♻️ Proposed fix
static error_t stop(Device* device) { - ESP_LOGI(TAG, "stop %s", device->name); + LOG_I(TAG, "stop %s", device->name); auto* data = GET_DATA(device); - auto* dts_config = GET_CONFIG(device);Tactility/Source/hal/sdcard/SdCardDevice.cpp (1)
27-32: Minor:getMountPath()called twice.This creates a slight inefficiency and a theoretical TOCTOU window if the path could change between calls. Consider caching the result in a local variable.
♻️ Proposed fix
static error_t get_path(void* data, char* out_path, size_t out_path_size) { auto* device = static_cast<SdCardDevice*>(data); - if (device->getMountPath().size() >= out_path_size) return ERROR_BUFFER_OVERFLOW; - strncpy(out_path, device->getMountPath().c_str(), out_path_size); + auto path = device->getMountPath(); + if (path.size() >= out_path_size) return ERROR_BUFFER_OVERFLOW; + strncpy(out_path, path.c_str(), out_path_size); return ERROR_NONE; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee677091-607c-4e1a-b818-e2b8cd74e8fb
📒 Files selected for processing (7)
Platforms/platform-esp32/include/tactility/drivers/esp32_sdmmc.hPlatforms/platform-esp32/private/tactility/drivers/esp32_sdmmc_fs.hPlatforms/platform-esp32/source/drivers/esp32_sdmmc.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cppPlatforms/platform-esp32/source/module.cppTactility/Source/hal/sdcard/SdCardDevice.cppTactility/Source/hal/usb/Usb.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Platforms/platform-esp32/source/module.cpp
- Platforms/platform-esp32/include/tactility/drivers/esp32_sdmmc.h
KenVanHoeylandt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (7)
Tactility/Source/hal/sdcard/SdCardDevice.cpp (1)
9-13:⚠️ Potential issue | 🟠 MajorMount uses transient state instead of a stable target path.
Line 11 reads the current mount path, but
getMountPath()is documented to return an empty string while the device is unmounted. That makes the first call on Line 12 effectivelydevice->mount(""), so this wrapper cannot perform an initial mount reliably. Please source the mount point from configuration or other persistent state instead of the current mounted state.Devices/m5stack-tab5/Source/Configuration.cpp (1)
102-130:⚠️ Potential issue | 🟠 MajorMissing
check()assertions for pin acquisitions ininitExpander1.Unlike
initExpander0(lines 59-72), this function does not verify thatgpio_descriptor_acquire()succeeds before using the returned descriptors. If any acquisition fails (returningnullptr), subsequent calls will dereference null pointers.🛡️ Proposed fix to add assertions
static void initExpander1(::Device* io_expander1) { auto* c6_wlan_enable_pin = gpio_descriptor_acquire(io_expander1, GPIO_EXP1_PIN_C6_WLAN_ENABLE, GPIO_OWNER_GPIO); + check(c6_wlan_enable_pin); auto* usb_a_5v_enable_pin = gpio_descriptor_acquire(io_expander1, GPIO_EXP1_PIN_USB_A_5V_ENABLE, GPIO_OWNER_GPIO); + check(usb_a_5v_enable_pin); auto* device_power_pin = gpio_descriptor_acquire(io_expander1, GPIO_EXP1_PIN_DEVICE_POWER, GPIO_OWNER_GPIO); + check(device_power_pin); auto* ip2326_ncharge_qc_enable_pin = gpio_descriptor_acquire(io_expander1, GPIO_EXP1_PIN_IP2326_NCHG_QC_EN, GPIO_OWNER_GPIO); + check(ip2326_ncharge_qc_enable_pin); auto* ip2326_charge_state_led_pin = gpio_descriptor_acquire(io_expander1, GPIO_EXP1_PIN_IP2326_CHG_STAT_LED, GPIO_OWNER_GPIO); + check(ip2326_charge_state_led_pin); auto* ip2326_charge_enable_pin = gpio_descriptor_acquire(io_expander1, GPIO_EXP1_PIN_IP2326_CHG_EN, GPIO_OWNER_GPIO); + check(ip2326_charge_enable_pin);TactilityKernel/source/drivers/gpio_controller.cpp (1)
174-177:⚠️ Potential issue | 🟠 MajorData race on
owner_typeread.
gpio_descriptor_get_owner_type()readsdescriptor->owner_typewithout acquiring the mutex, whilegpio_descriptor_acquire()andgpio_descriptor_release()modify it under the lock. This can return stale or torn values on concurrent access.🔒 Proposed fix to add mutex protection
error_t gpio_descriptor_get_owner_type(GpioDescriptor* descriptor, GpioOwnerType* owner_type) { + auto* data = static_cast<struct GpioControllerData*>(device_get_driver_data(descriptor->controller)); + mutex_lock(&data->mutex); *owner_type = descriptor->owner_type; + mutex_unlock(&data->mutex); return ERROR_NONE; }Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
345-347:⚠️ Potential issue | 🟠 MajorOnly update the SD-card bar after it exists.
Lines 621-623 create
sdcardStorageBaronly when the card is mounted duringonShow(), but Lines 345-347 recompute the path and update it unconditionally. If the filesystem appears after the creation pass but beforeupdateStorage()runs,updateMemoryBar()gets{nullptr, nullptr}and LVGL will dereference null widgets.🛠️ Minimal guard
- std::string sdcard_path; - if (findFirstMountedSdCardPath(sdcard_path) && esp_vfs_fat_info(sdcard_path.c_str(), &storage_total, &storage_free) == ESP_OK) { - updateMemoryBar(sdcardStorageBar, storage_free, storage_total); + if (sdcardStorageBar.bar != nullptr) { + std::string sdcard_path; + if (findFirstMountedSdCardPath(sdcard_path) && + esp_vfs_fat_info(sdcard_path.c_str(), &storage_total, &storage_free) == ESP_OK) { + updateMemoryBar(sdcardStorageBar, storage_free, storage_total); + } }Also applies to: 621-623
Platforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cpp (1)
121-125:⚠️ Potential issue | 🔴 CriticalUse
fs_datain the mount-failure cleanup block.Line 122 still dereferences
dataas if it wereEsp32SdmmcFsData*. That block will not compile whenSOC_SD_PWR_CTRL_SUPPORTEDis enabled, and it skips the same cleanup target used in the rest of the file.#!/bin/bash # Expectation: every power-control access in this file should use `fs_data->pwr_ctrl_handle`. rg -n 'data->pwr_ctrl_handle|fs_data->pwr_ctrl_handle' Platforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cpp🐛 Minimal fix
`#if` SOC_SD_PWR_CTRL_SUPPORTED - if (data->pwr_ctrl_handle) { - sd_pwr_ctrl_del_on_chip_ldo(data->pwr_ctrl_handle); - data->pwr_ctrl_handle = nullptr; + if (fs_data->pwr_ctrl_handle) { + sd_pwr_ctrl_del_on_chip_ldo(fs_data->pwr_ctrl_handle); + fs_data->pwr_ctrl_handle = nullptr; } `#endif`Platforms/platform-esp32/private/tactility/drivers/esp32_sdmmc_fs.h (1)
5-20:⚠️ Potential issue | 🔴 CriticalMake this header self-contained and valid C.
Line 18 exposes
sdmmc_card_t*without declaring the type, and Line 20 usesFileSystemApias if it were a typedef. In the providedtactility/filesystem/file_system.h, onlystruct FileSystemApiis declared, so a C translation unit including this header will fail unless unrelated headers happened to be included first.Suggested fix
`#include` <tactility/filesystem/file_system.h> +#include <sd_protocol_types.h> @@ -extern const FileSystemApi esp32_sdmmc_fs_api; +extern const struct FileSystemApi esp32_sdmmc_fs_api;#!/bin/bash set -e printf '%s\n' '--- esp32_sdmmc_fs.h ---' cat -n Platforms/platform-esp32/private/tactility/drivers/esp32_sdmmc_fs.h printf '\n%s\n' '--- FileSystemApi declarations/typedefs ---' rg -n -C2 '\bstruct FileSystemApi\b|typedef\s+struct\s+FileSystemApi\b|typedef\b.*\bFileSystemApi\b' printf '\n%s\n' '--- sdmmc_card_t declarations/includes ---' rg -n -C2 '\bsdmmc_card_t\b|sd_protocol_types\.h'TactilityKernel/source/filesystem/file_system.cpp (1)
47-58:⚠️ Potential issue | 🔴 CriticalCoordinate
remove()with the public ops before deletingfs.
file_system_remove()erases and deletesfsunder the ledger mutex, butfile_system_mount(),file_system_unmount(),file_system_is_mounted(), andfile_system_get_path()dereference the same pointer without any shared lifetime coordination. Another thread can freefsbetween lookup and use, turning these calls into use-after-free. The mounted check on Line 48 is also outside that coordination window.Also applies to: 72-87
🧹 Nitpick comments (1)
Tactility/Source/hal/usb/Usb.cpp (1)
29-55: Route SD discovery through the filesystem layer, not concrete drivers.
getCard()now knows about bothsdcard::SpiSdCardDeviceand the"espressif,esp32-sdmmc"driver. That reintroduces backend-specific storage discovery in USB HAL right as this PR is moving SD access behind filesystem-backed implementations. The next removable backend will need another branch here, and this path can drift from whatever the filesystem registry considers mounted/active. Please resolve the mounted filesystem first and only derive thesdmmc_card_t*from that selected filesystem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4d15669-3f8d-4959-967c-a6f42fa92496
📒 Files selected for processing (27)
Devices/m5stack-tab5/Source/Configuration.cppPlatforms/platform-esp32/include/tactility/drivers/esp32_sdmmc.hPlatforms/platform-esp32/private/tactility/drivers/esp32_sdmmc_fs.hPlatforms/platform-esp32/source/drivers/esp32_gpio.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cppPlatforms/platform-esp32/source/module.cppTactility/Include/Tactility/Paths.hTactility/Source/MountPoints.cppTactility/Source/PartitionsEsp.cppTactility/Source/Paths.cppTactility/Source/Tactility.cppTactility/Source/app/fileselection/State.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/hal/sdcard/SdCardDevice.cppTactility/Source/hal/usb/Usb.cppTactility/Source/service/sdcard/Sdcard.cppTactility/Source/service/statusbar/Statusbar.cppTactility/Source/service/webserver/WebServerService.cppTactilityC/Source/tt_init.cppTactilityKernel/include/tactility/drivers/gpio.hTactilityKernel/include/tactility/drivers/gpio_controller.hTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/include/tactility/filesystem/file_system.hTactilityKernel/source/drivers/gpio_controller.cppTactilityKernel/source/filesystem/file_system.cppTactilityKernel/source/kernel_symbols.c
💤 Files with no reviewable changes (1)
- Tactility/Source/service/sdcard/Sdcard.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Tactility/Source/app/fileselection/State.cpp
- Tactility/Include/Tactility/Paths.h
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Include/Tactility/service/wifi/Wifi.h (1)
16-32:⚠️ Potential issue | 🔴 CriticalRemoving
wifi_auth_mode_ttypedef breaksApRecordon non-ESP platforms.The change from
} wifi_auth_mode_t;to};removes the type alias, but line 69 still useswifi_auth_mode_t auth_mode;in theApRecordstruct. This will cause a compilation error on non-ESP platforms (e.g., simulator/posix) whereesp_wifi.his not included.Additionally,
WifiMock.cpp(lines 78-113) createsApRecordinstances with.auth_mode = WIFI_AUTH_WPA2_PSK, which depend on this typedef.🔧 Proposed fix: restore the typedef
WIFI_AUTH_WPA3_EXT_PSK_MIXED_MODE, /**< authenticate mode: WPA3_PSK + WPA3_PSK_EXT_KEY */ WIFI_AUTH_MAX -}; +} wifi_auth_mode_t; `#endif`
♻️ Duplicate comments (6)
TactilityKernel/include/tactility/drivers/wifi.h (1)
62-63:⚠️ Potential issue | 🟡 MinorIncorrect comment for
WIFI_EVENT_TYPE_STATION_CONNECTION_RESULT.The comment says "WifiAccessPointState changed" but this event represents a station connection result, not an access point state change. This appears to be a copy-paste error from line 64.
📝 Proposed fix
/** WifiStationState changed */ WIFI_EVENT_TYPE_STATION_STATE_CHANGED, - /** WifiAccessPointState changed */ + /** Station connection result available */ WIFI_EVENT_TYPE_STATION_CONNECTION_RESULT, /** WifiAccessPointState changed */ WIFI_EVENT_TYPE_ACCESS_POINT_STATE_CHANGED,TactilityKernel/source/filesystem/file_system.cpp (2)
49-60:⚠️ Potential issue | 🔴 CriticalSerialize
FileSystem*destruction with the public ops.
file_system_remove()freesfsat Line 57, but Lines 75, 79, 83, and 87 dereference the same raw handle without any shared lifetime guard. Line 50 also checks mount state before that deletion is serialized. A concurrentremove()can still turn these entry points into a use-after-free.Also applies to: 72-87
63-69:⚠️ Potential issue | 🔴 CriticalDon't let callbacks mutate the live registry during this iteration.
Because the callback runs inside the range-for over
ledger.file_systems, a reentrantfile_system_add()orfile_system_remove()can reallocate or erase thestd::vectorand invalidate the loop iterators mid-iteration. Either forbid registry mutation from callbacks or iterate over stable ownership instead of the live container.Platforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cpp (1)
121-125:⚠️ Potential issue | 🔴 CriticalUse
fs_datain the LDO cleanup path.Lines 122-124 still access
data->pwr_ctrl_handle, butdatais the rawvoid*parameter here. That does not compile whenSOC_SD_PWR_CTRL_SUPPORTEDis enabled, and it bypasses the intended cleanup. Use the castedfs_dataconsistently.🐛 Proposed fix
`#if` SOC_SD_PWR_CTRL_SUPPORTED - if (data->pwr_ctrl_handle) { - sd_pwr_ctrl_del_on_chip_ldo(data->pwr_ctrl_handle); - data->pwr_ctrl_handle = nullptr; + if (fs_data->pwr_ctrl_handle) { + sd_pwr_ctrl_del_on_chip_ldo(fs_data->pwr_ctrl_handle); + fs_data->pwr_ctrl_handle = nullptr; } `#endif`Run this to confirm there are no remaining raw
data->pwr_ctrl_handleaccesses in this file:#!/bin/bash set -euo pipefail fd 'esp32_sdmmc_fs\.cpp$' -x sh -c ' for f; do echo ">>> $f" sed -n "118,126p" "$f" echo rg -n "data->pwr_ctrl_handle|fs_data->pwr_ctrl_handle" "$f" done ' sh {}Expected result: the cleanup block only uses
fs_data->pwr_ctrl_handle.Tactility/Source/hal/sdcard/SdCardDevice.cpp (1)
48-49:⚠️ Potential issue | 🔴 CriticalDeregister the filesystem before base destruction starts.
SdCardDevice::~SdCardDevice()callsfile_system_remove(), and that path immediately asks the registered API whether the device is mounted. At this point the base destructor is already running, so that callback can observe derived state after teardown has begun. Move deregistration into the derived shutdown path, or otherwise perform it beforeSdCardDevice::~SdCardDevice()executes.Run this to inspect the mounted-state call chain involved in teardown:
#!/bin/bash set -euo pipefail printf '== SdCardDevice declaration ==\n' fd 'SdCardDevice\.h$' -x sed -n '1,220p' {} printf '\n== SdCardDevice implementation ==\n' fd 'SdCardDevice\.cpp$' -x sed -n '1,120p' {} printf '\n== Mounted-state call chain ==\n' rg -n -C3 '\bisMounted\s*\(|\bgetState\s*\(|file_system_remove\s*\('Expected result:
file_system_remove()reaches mounted-state logic during base destruction.Platforms/platform-esp32/source/drivers/esp32_sdmmc.cpp (1)
136-153:⚠️ Potential issue | 🟠 Major
stop()skips all teardown after an unmount error.If Lines 137-140 are hit, the driver keeps the filesystem registration, filesystem handle, GPIO reservations, and
devicedriver data. That leaves the device half-stopped on any unmount failure. Either continue cleanup after logging, or make the stop path explicitly retry-safe instead of returning with ownership still held.
🧹 Nitpick comments (1)
Platforms/platform-esp32/source/drivers/esp32_gpio.cpp (1)
148-153: Consider usingstd::nothrowfor allocation consistency.Line 151 uses
new Esp32GpioInternal()which throws on allocation failure, while the kernel'sgpio_controller_init_descriptorsusesnew(std::nothrow). If exceptions are disabled (common in ESP-IDF), the throwingnewtypically aborts on failure.For consistency with kernel code patterns, consider:
♻️ Optional refactor for allocation consistency
static error_t start(Device* device) { LOG_I(TAG, "start %s", device->name); const Esp32GpioConfig* config = GET_CONFIG(device); - auto* internal = new Esp32GpioInternal(); + auto* internal = new(std::nothrow) Esp32GpioInternal(); + if (!internal) return ERROR_OUT_OF_MEMORY; return gpio_controller_init_descriptors(device, config->gpioCount, internal); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20d534da-9288-40d6-8138-61d3a744920a
📒 Files selected for processing (8)
Platforms/platform-esp32/source/drivers/esp32_gpio.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc.cppPlatforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cppTactility/Include/Tactility/service/wifi/Wifi.hTactility/Source/hal/sdcard/SdCardDevice.cppTactilityKernel/include/tactility/drivers/gpio_controller.hTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/source/filesystem/file_system.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
TactilityKernel/include/tactility/drivers/wifi.h (1)
62-63:⚠️ Potential issue | 🟡 MinorFix the mismatched comment for
WIFI_EVENT_TYPE_STATION_CONNECTION_RESULT.The comment says "WifiAccessPointState changed" but this event type is for station connection results. This appears to be a copy-paste error from line 64.
📝 Proposed fix
- /** WifiAccessPointState changed */ + /** Station connection result available */ WIFI_EVENT_TYPE_STATION_CONNECTION_RESULT,
🧹 Nitpick comments (1)
TactilityKernel/include/tactility/drivers/wifi.h (1)
1-13: Add forward declaration forstruct DeviceType.The header uses
struct DeviceTypeon line 201 but doesn't includetactility/device.hor provide a forward declaration. While this compiles in C (thestructkeyword introduces the type), making the header self-contained improves usability for consumers.🔧 Proposed fix
struct Device; +struct DeviceType;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dd643ff-b6d6-4791-bba1-59d47b34aaf9
📒 Files selected for processing (1)
TactilityKernel/include/tactility/drivers/wifi.h
1 participant
New Features
Improvements
Bug Fixes