SPI device migration #490
SPI device migration #490
Conversation
📝 WalkthroughWalkthroughThe PR removes the tt::hal::spi subsystem (headers and implementation) and related compatibility shims, and deletes SpiInit/SpiCompat. Many device Configuration.cpp files drop inline SPI initialization. A new esp32 SPI binding, driver headers, and esp32_spi DTS bindings are added; numerous board DTS files gain spi nodes and many i2c pull-up properties are removed. SdCard creation sites now locate ::Device* SPI controllers at runtime and pass them to an updated SpiSdCardDevice constructor. Display drivers and the EspLcd stack no longer accept/propagate SPI lock parameters. A new SpiDeviceLock adapter and related kernel binding were introduced. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Devices/guition-jc3248w535c/Source/Axs15231b/Axs15231bDisplay.h (1)
28-42:⚠️ Potential issue | 🔴 CriticalCritical Bug:
tePinparameter missing from constructor but member and initializer retained — self-initialization is undefined behavior.The
tePinconstructor parameter was removed from the signature (should be afterresetPinat Line 27), buttePin(tePin)remains in the initializer list (Line 42), causing the uninitialized member to read itself. The member is declared at Line 62.Re-add the
tePinparameter to the constructor. The TE functionality is still active:setupTeSync()and teardown code in the .cpp implementation actively useconfiguration->tePin(lines 172, 188, 207, 220, 225–227) to configure the interrupt handler and GPIO, so the parameter cannot be removed.The parameter should be added before
horizontalResolutionat Line 29.Devices/cyd-2432s028rv3/Source/devices/SdCard.cpp (1)
10-25:⚠️ Potential issue | 🟡 MinorThe
spiControllerparameter is redundant whencustomLockis set.The constructor at line 21 ignores the
spiControllerparameter becauseconfig->customLock(theRecursiveMutexpassed in theConfig) is non-null. TheSpiSdCardDeviceconstructor only usesspiControllerto create anSpiDeviceLockwhencustomLockis null:if (config->customLock == nullptr) { auto spi_lock = std::make_shared<SpiDeviceLock>(spiController); lock = std::static_pointer_cast<Lock>(spi_lock); } else { lock = config->customLock; // Used; spiController ignored }Both
spiHostandRecursiveMutexin the Config are necessary and actively used during mount. However, the pattern here is transitional: compare withcyd-8048s043c/SdCard.cpp, which uses minimal Config (only 5 args, nospiHostorcustomLock) and relies solely on thespiControllerparameter. Consider aligning this board with the newer pattern.Drivers/ST7735/Source/St7735Display.h (1)
89-95:⚠️ Potential issue | 🔴 CriticalConstructor passes undefined function to base class with no matching constructor.
The constructor at line 90 calls
tt::hal::spi::getLock(inConfiguration->spiHostDevice)and attempts to pass its result toEspLcdDisplay, but:
EspLcdDisplayonly has a default constructor (EspLcdDisplay() = default;) that accepts no arguments.- The
tt::hal::spi::getLockfunction does not exist in the codebase—no definition, declaration, or namespace definition fortt::hal::spican be found.- The code also calls
assert(getLock() != nullptr)but St7735Display has nogetLock()method.This will not compile. The SPI device migration is incomplete for this driver. The recent commit "933f8ad SPI device migration" indicates refactoring is in progress; this file needs to be updated to match the new SPI architecture.
🧹 Nitpick comments (5)
Drivers/EspLcdCompat/Source/EspLcdDisplayV2.h (1)
68-72: Consider marking this single-parameter constructorexplicit.A single-parameter constructor without
explicitpermits implicit conversions fromstd::shared_ptr<EspLcdConfiguration>toEspLcdDisplayV2. While unlikely to cause issues in practice (derived classes construct it explicitly), it's a good habit.Proposed fix
- EspLcdDisplayV2(const std::shared_ptr<EspLcdConfiguration>& configuration) : + explicit EspLcdDisplayV2(const std::shared_ptr<EspLcdConfiguration>& configuration) :Devices/cyd-4848s040c/Source/devices/St7701Display.h (1)
3-3: Remove unused include:RecursiveMutex.his never referenced in this file or its implementation.The include on line 3 is stale and can be safely removed.
Proposed fix
-#include <Tactility/RecursiveMutex.h> -Drivers/EspLcdCompat/Source/EspLcdDisplay.h (1)
3-3: Remove unused includeTactility/Lock.h.The
Tactility/Lock.hheader is not used anywhere in this file. There are no lock member variables or Lock-related methods. The include can be safely removed.-#include <Tactility/Lock.h>TactilityCore/Include/Tactility/kernel/SpiDeviceLock.h (1)
13-13: Mark single-argument constructorexplicit.Prevents unintended implicit conversions from
Device*toSpiDeviceLock.Proposed fix
- SpiDeviceLock(::Device* device) : device(device) { } + explicit SpiDeviceLock(::Device* device) : device(device) { }Drivers/ST7735/Source/St7735Display.h (1)
15-15: Remove unused member variable.The
lockmember variable declared at line 15 is never used anywhere in this class. The lock is obtained viatt::hal::spi::getLock()and passed to the base class constructor (line 90), where it's managed. This member is redundant.♻️ Proposed fix to remove unused member
class St7735Display final : public EspLcdDisplay { - std::shared_ptr<tt::Lock> lock; - public:
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TactilityKernel/Include/tactility/concurrent/recursive_mutex.h (1)
18-26:⚠️ Potential issue | 🟡 MinorRemoved precondition assertions weaken misuse detection.
Previously,
recursive_mutex_constructassertedhandle == NULL(catches double-init / semaphore leak) andrecursive_mutex_destructassertedhandle != NULL(catches double-free / NULL passed tovSemaphoreDelete, which is undefined in FreeRTOS). Both guards were effectively free at runtime and caught real programming errors early.Was the removal intentional, or a side-effect of the broader refactor? If intentional, consider at least keeping the destruct guard to avoid UB:
Proposed fix
inline static void recursive_mutex_construct(struct RecursiveMutex* mutex) { + check(mutex->handle == NULL); mutex->handle = xSemaphoreCreateRecursiveMutex(); } inline static void recursive_mutex_destruct(struct RecursiveMutex* mutex) { check(xPortInIsrContext() != pdTRUE); + check(mutex->handle != NULL); vSemaphoreDelete(mutex->handle); mutex->handle = NULL; }
🧹 Nitpick comments (9)
Devices/lilygo-tdisplay/lilygo,tdisplay.dts (1)
17-26: Verifymax-transfer-size = <0>is intentional.In ESP-IDF, a
max_transfer_szof 0 defaults to 4094 bytes. If that default is the desired behavior, consider adding a brief comment to make it explicit. Otherwise, set a specific value matching the display's needs (e.g., the frame buffer size for bulk transfers).Devices/lilygo-tdongle-s3/Source/devices/Display.cpp (2)
12-12: Nit: wrap macro expression in parentheses.Pre-existing, but since you're touching this file: the macro should be parenthesized to avoid surprises in compound expressions.
Suggested fix
-#define LCD_SPI_TRANSFER_HEIGHT LCD_VERTICAL_RESOLUTION / 4 +#define LCD_SPI_TRANSFER_HEIGHT (LCD_VERTICAL_RESOLUTION / 4)
20-21: Nit: use the existing macros instead of duplicated literals.Lines 20–21 pass
80and160directly, butLCD_HORIZONTAL_RESOLUTIONandLCD_VERTICAL_RESOLUTIONare already defined for this purpose.Suggested fix
auto configuration = std::make_unique<St7735Display::Configuration>( LCD_SPI_HOST, LCD_PIN_CS, LCD_PIN_DC, LCD_PIN_RESET, - 80, - 160, + LCD_HORIZONTAL_RESOLUTION, + LCD_VERTICAL_RESOLUTION,TactilityKernel/Include/tactility/concurrent/recursive_mutex.h (1)
13-16: TODO without a tracking reference.Consider linking this to an issue so it doesn't get lost (e.g.,
// TODO(#123): Add debugging functionality).Drivers/ILI9488/Source/Ili9488Display.h (1)
31-31: Pre-existing comment mismatch: "1/20" vs actual "1/10".The comment here says the default is
1/20of the screen size, but line 44 computes/ 10and line 60's comment says1/10. This comment is stale.📝 Suggested fix
- uint32_t bufferSize = 0 // Size in pixel count. 0 means default, which is 1/20 of the screen size + uint32_t bufferSize = 0 // Size in pixel count. 0 means default, which is 1/10 of the screen sizeDevices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts (1)
16-26: SPI node added without a label, unlike other device trees in this PR.Other
.dtsfiles in this PR use labels likedisplay_spi: spi0andsdcard_spi: spi1. This node is justspi0without a label. Since this device has no SD card and only one SPI bus, it's functional, but adding adisplay_spi:label would be more consistent with the rest of the PR.Optional: add label for consistency
- spi0 { + display_spi: spi0 {Devices/guition-jc3248w535c/guition,jc3248w535c.dts (1)
46-46: Clarify semantics ofmax-transfer-size = <0>.Both SPI nodes set
max-transfer-sizeto 0. If 0 means "use driver default" or "unlimited," this is fine, but it would be worth a brief comment in the binding documentation or here to avoid confusion for future maintainers.Also applies to: 57-57
Devices/guition-jc8048w550c/Source/devices/SdCard.cpp (1)
5-6: Unused includes likely left over from the SPI lock removal.
LvglSync.handRecursiveMutex.hdon't appear to be used anywhere in this file after the migration.Proposed fix
`#include` <tactility/device.h> `#include` <Tactility/hal/sdcard/SpiSdCardDevice.h> -#include <Tactility/lvgl/LvglSync.h> -#include <Tactility/RecursiveMutex.h>Tactility/Include/Tactility/hal/Configuration.h (1)
22-26: Stale comment: SPI is no longer configured via this struct.The comment says "Called before I2C/SPI/etc is initialized" but SPI configuration has been removed from
Configuration. Consider updating to reflect that SPI is now handled via device-tree bindings, e.g., "Called before device-tree peripherals are initialized."📝 Suggested comment update
- /** - * Called before I2C/SPI/etc is initialized. - * Used for powering on the peripherals manually. - */ + /** + * Called before device-tree peripherals are initialized. + * Used for powering on the peripherals manually. + */
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 (4)
Tactility/Source/service/wifi/WifiEsp.cpp (1)
892-898:⚠️ Potential issue | 🔴 CriticalNull-pointer dereference when service is not running.
getIp()lacks the null guard that every other public function has. Ifwifi_singletonis null, line 895 dereferences a null pointer.Proposed fix
std::string getIp() { auto wifi = std::static_pointer_cast<Wifi>(wifi_singleton); + if (wifi == nullptr) { + return "0.0.0.0"; + } auto lock = wifi->dataMutex.asScopedLock(); if (!lock.lock(100 / portTICK_PERIOD_MS)) { return "0.0.0.0"; }Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.h (1)
4-14:⚠️ Potential issue | 🟡 MinorRemove unused
NoLockclass andRecursiveMutex.hinclude.The
NoLockclass is no longer referenced after the constructor change. The include of<Tactility/RecursiveMutex.h>(providingtt::Lock) is only needed forNoLock.🧹 Remove unused code
`#include` <EspLcdDisplayV2.h> -#include <Tactility/RecursiveMutex.h> `#include` <esp_lcd_mipi_dsi.h> `#include` <esp_ldo_regulator.h> class Ili9881cDisplay final : public EspLcdDisplayV2 { - class NoLock final : public tt::Lock { - bool lock(TickType_t timeout) const override { return true; } - void unlock() const override { /* NO-OP */ } - }; - esp_lcd_dsi_bus_handle_t mipiDsiBus = nullptr;Drivers/EspLcdCompat/Source/EspLcdDisplay.h (1)
16-16:⚠️ Potential issue | 🟡 Minor
rgbElementOrderis uninitialized with the defaulted constructor.The member on line 16 has no in-class initializer and the constructor is now defaulted, leaving this enum uninitialized. The code reads this member in
getDisplayDriver()(cpp line 103), which is undefined behavior. Add a default initializer to match patterns in other display drivers:Proposed fix
- lcd_rgb_element_order_t rgbElementOrder; + lcd_rgb_element_order_t rgbElementOrder = LCD_RGB_ELEMENT_ORDER_RGB;Devices/waveshare-s3-touch-lcd-128/Source/devices/SdCard.cpp (1)
9-27:⚠️ Potential issue | 🟠 MajorDead code:
SdCard.cppdefinescreateSdCard()but it is never instantiated — missing fromConfiguration.cpp.
createSdCard()is declared inSdCard.hand defined inSdCard.cpp, butConfiguration.cppline 11-15 only callscreateDisplay()increateDevices(). Other boards (e.g.,m5stack-tab5,cyd-2432s032c) includecreateSdCard(). Either:
- Delete
SdCard.cppandSdCard.hif SD card is not supported on this board, or- Add
createSdCard()to the device vector inConfiguration.cppline 13.
🧹 Nitpick comments (13)
Devices/lilygo-tdongle-s3/Source/devices/Display.cpp (1)
20-21: Use the defined macros instead of magic numbers.Lines 20–21 hardcode
80and160, duplicating the values already defined asLCD_HORIZONTAL_RESOLUTIONandLCD_VERTICAL_RESOLUTIONon lines 10–11.Suggested diff
auto configuration = std::make_unique<St7735Display::Configuration>( LCD_SPI_HOST, LCD_PIN_CS, LCD_PIN_DC, LCD_PIN_RESET, - 80, - 160, + LCD_HORIZONTAL_RESOLUTION, + LCD_VERTICAL_RESOLUTION, nullptr,Tactility/Include/Tactility/hal/Configuration.h (1)
25-30: Nit:createDeviceslacksconstunlike sibling fields.
initBootanduiScaleare both declaredconst, butcreateDevicesis not. IfConfigurationinstances are meant to be immutable after construction, consider making itconstas well for consistency. If mutability is intentional (e.g., for reassignment after construction), this is fine as-is.Buildscripts/DevicetreeCompiler/source/generator.py (1)
66-68: Useelifinstead ofifto maintain the chain.Line 66 uses
ifinstead ofelif, breaking theif/elif/…/elsechain. It works today only because the preceding branch on line 65 returns early, but if that return is ever refactored the fall-through semantics change silently.Proposed fix
if type == "value": return property.value - if type == "boolean": + elif type == "boolean": return "true" elif type == "text":Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts (1)
18-24: I2C pull-up removal is a separate concern from SPI migration.The removal of
pin-sda-pullupandpin-scl-pullupis unrelated to the SPI device migration. This is fine if the board has external pull-ups, but consider splitting unrelated changes for clearer commit history.Documentation/ideas.md (2)
17-17: Consider safety checks for automatic build directory deletion.Automatically deleting build directories when the platform changes could lead to unintended data loss if the detection logic is faulty or if users have custom artifacts in those directories. When implementing this, ensure you:
- Prompt for confirmation before deletion
- Log what will be deleted
- Provide a way to opt-out or disable this behavior
- Consider backup or move-to-trash instead of permanent deletion
14-17: Consider tracking these higher-priority ideas as GitHub issues.These four ideas represent actionable work items that emerged from the SPI migration. Creating dedicated GitHub issues for each would:
- Enable better tracking and prioritization
- Allow for discussion and refinement of requirements
- Link related PRs and commits
- Provide visibility to contributors
Tactility/Source/service/wifi/WifiEsp.cpp (1)
301-313: Mutex acquisition is now redundant for an atomic read.
secure_connectionisstd::atomic<bool>, soisSecureConnection()is already thread-safe without the lock. The other atomic accessors (getRadioState,isScanning) are called lock-free elsewhere. Consider dropping the lock here for consistency.Suggested simplification
bool isConnectionSecure() { auto wifi = wifi_singleton; if (wifi == nullptr) { return false; } - auto lock = wifi->dataMutex.asScopedLock(); - if (!lock.lock(10 / portTICK_PERIOD_MS)) { - return false; - } - return wifi->isSecureConnection(); }Devices/waveshare-s3-lcd-13/Source/devices/SdCard.cpp (2)
5-5: Remove unusedRecursiveMutex.hinclude.Line 5 includes
<Tactility/RecursiveMutex.h>, butRecursiveMutexis not used anywhere in this file. Line 17 passesnullptrinstead of constructing aRecursiveMutexinstance, making this include unnecessary.Proposed cleanup
-#include <Tactility/RecursiveMutex.h>
22-23: Add missing include fordevice_find_by_name.The function
device_find_by_nameis called on line 22 but<tactility/device.h>is not included. Every otherSdCard.cppfile in this codebase includes this header. While this may compile via transitive inclusion, adding the direct include improves robustness and consistency.Proposed fix
`#include` "SdCard.h" +#include <tactility/device.h> `#include` <Tactility/lvgl/LvglSync.h> `#include` <Tactility/hal/sdcard/SpiSdCardDevice.h> `#include` <Tactility/RecursiveMutex.h> `#include` <driver/gpio.h>Devices/m5stack-cores3/m5stack,cores3.dts (1)
35-51: Consider removing commented-out nodes or adding a note explaining why they're kept.The commented-out
i2c_port_bandi2c_port_cblocks are kept as dead code. If they serve as a reference for users who want to reconfigure the I2C ports, a brief comment explaining that would be helpful. Otherwise, removing them keeps the DTS clean.Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts (1)
26-35: Newspi0node looks correct; clarifymax-transfer-size = <0>intent.In ESP-IDF, a
max_transfer_szof0defaults to 4094 bytes. If that's the intended behavior, consider adding a brief comment (e.g.,/* 0 = default (4094) */) for maintainability, or explicitly set the desired value. Other board DTS files in this PR should be checked for consistency.Devices/waveshare-s3-lcd-13/Source/Configuration.cpp (1)
1-6:<driver/gpio.h>appears unused after removing the SPI configuration block.With the SPI config removed, there's no remaining GPIO usage in this file. Consider removing this include.
Proposed fix
`#include` "devices/Display.h" `#include` "devices/SdCard.h" -#include <driver/gpio.h> `#include` <Tactility/hal/Configuration.h> `#include` <PwmBacklight.h>Devices/waveshare-s3-touch-lcd-128/Source/devices/SdCard.cpp (1)
10-18: ThespiHostparameter in Config is required and actively used by ESP-IDF for SD card initialization.While the DTS node (
spi1) and Config struct both specify SPI3_HOST, they serve different purposes and both are necessary:
spiHost(from Config) is passed to ESP-IDF APIs (esp_vfs_fat_sdspi_mount,sdspi_device_config_t) to identify the hardware SPI controllerspiController(fromdevice_find_by_name) is used for SPI bus arbitration viaSpiDeviceLockThe maintenance concern is valid—the DTS host value and Config spiHost must stay synchronized. Consider documenting this coupling or adding a validation check to ensure they match at runtime rather than removing the parameter.
2 participants
tt::hal::spiHAL and its configurationsDisplayDeviceimplementationsWifiEspdeadlockmath.hsymbols withtt_init.cppSpiDeviceLockinTactilityCoreas a wrapper for kernel SPI lockingTactilityKernelSPI driver.Summary by CodeRabbit
New Features
SpiDeviceLockfor improved SPI device synchronization.Bug Fixes
Refactor
Tests