Split off RecursiveMutex from Mutex #437
Conversation
📝 WalkthroughWalkthroughThe changes add a new tt::RecursiveMutex class and update many files to use it instead of constructing tt::Mutex with a Recursive Type. tt::Mutex was simplified (Type enum removed, default constructor and inline methods added) and its previous implementation source was removed. Multiple headers and source files (drivers, devices, services, apps, and tests) were updated to include RecursiveMutex and to replace prior recursive Mutex constructions with tt::RecursiveMutex or the simplified tt::Mutex(). 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TactilityCore/Include/Tactility/Mutex.h (1)
17-20: Update documentation to reflect non-recursive behavior.The documentation still mentions "xSemaphoreCreateMutex and xSemaphoreCreateRecursiveMutex" but this class now only wraps the non-recursive
xSemaphoreCreateMutex. Update the comment to clarify this is for non-recursive mutexes only.🔎 Proposed documentation fix
/** - * Wrapper for FreeRTOS xSemaphoreCreateMutex and xSemaphoreCreateRecursiveMutex + * Wrapper for FreeRTOS xSemaphoreCreateMutex (non-recursive mutex) * Cannot be used in IRQ mode (within ISR context) */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
Devices/cyd-2432s024c/Source/devices/SdCard.cppDevices/cyd-2432s028r/Source/devices/SdCard.cppDevices/cyd-2432s028rv3/Source/devices/SdCard.cppDevices/cyd-2432s032c/Source/devices/SdCard.cppDevices/cyd-4848s040c/Source/devices/St7701Display.hDevices/cyd-e32r28t/Source/devices/SdCard.cppDevices/cyd-e32r32p/Source/devices/SdCard.cppDevices/cyd-jc2432w328c/Source/devices/SdCard.cppDevices/cyd-jc8048w550c/Source/devices/SdCard.cppDevices/simulator/Source/LvglTask.cppDevices/simulator/Source/hal/SimulatorSdCard.hDevices/waveshare-s3-lcd-13/Source/devices/SdCard.cppDevices/waveshare-s3-touch-lcd-128/Source/devices/SdCard.cppDrivers/RgbDisplay/Source/RgbDisplay.hTactility/Include/Tactility/hal/gps/GpsDevice.hTactility/Include/Tactility/hal/gps/Satellites.hTactility/Include/Tactility/hal/sdcard/SdmmcDevice.hTactility/Include/Tactility/network/HttpServer.hTactility/Include/Tactility/service/gps/GpsService.hTactility/Include/Tactility/service/loader/Loader.hTactility/Private/Tactility/app/AppInstance.hTactility/Private/Tactility/app/files/State.hTactility/Private/Tactility/app/fileselection/State.hTactility/Private/Tactility/app/wifimanage/State.hTactility/Private/Tactility/service/ServiceInstance.hTactility/Private/Tactility/service/development/DevelopmentService.hTactility/Private/Tactility/service/espnow/EspNowService.hTactility/Private/Tactility/service/gui/GuiService.hTactility/Private/Tactility/service/memorychecker/MemoryCheckerService.hTactility/Private/Tactility/service/screenshot/ScreenshotTask.hTactility/Source/app/AppRegistration.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/timedatesettings/TimeDateSettings.cppTactility/Source/hal/Device.cppTactility/Source/hal/spi/Spi.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/service/ServiceRegistration.cppTactility/Source/service/wifi/WifiEsp.cppTactility/Source/service/wifi/WifiMock.cppTactilityC/Source/tt_lock.cppTactilityCore/Include/Tactility/Mutex.hTactilityCore/Include/Tactility/RecursiveMutex.hTactilityCore/Source/Mutex.cppTests/TactilityCore/MutexTest.cpp
💤 Files with no reviewable changes (1)
- TactilityCore/Source/Mutex.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 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/cyd-e32r28t/Source/devices/SdCard.cppDevices/simulator/Source/LvglTask.cppTactility/Include/Tactility/network/HttpServer.hDevices/waveshare-s3-lcd-13/Source/devices/SdCard.cppTactility/Source/hal/spi/Spi.cppDrivers/RgbDisplay/Source/RgbDisplay.hDevices/cyd-jc8048w550c/Source/devices/SdCard.cppDevices/cyd-4848s040c/Source/devices/St7701Display.hDevices/cyd-2432s028rv3/Source/devices/SdCard.cppDevices/cyd-jc2432w328c/Source/devices/SdCard.cppDevices/waveshare-s3-touch-lcd-128/Source/devices/SdCard.cppDevices/cyd-2432s028r/Source/devices/SdCard.cppDevices/cyd-2432s032c/Source/devices/SdCard.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:
Devices/simulator/Source/LvglTask.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:
Drivers/RgbDisplay/Source/RgbDisplay.h
📚 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:
Drivers/RgbDisplay/Source/RgbDisplay.h
📚 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-jc2432w328c/Source/devices/SdCard.cppDevices/cyd-2432s032c/Source/devices/SdCard.cpp
🧬 Code graph analysis (19)
Tactility/Include/Tactility/service/loader/Loader.h (3)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
Devices/simulator/Source/hal/SimulatorSdCard.h (4)
TactilityCore/Include/Tactility/RecursiveMutex.h (3)
lock(47-50)tt(14-67)RecursiveMutex(37-39)Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
std(57-66)Tactility/Include/Tactility/hal/gps/GpsDevice.h (1)
tt(13-124)Tactility/Include/Tactility/hal/sdcard/SdmmcDevice.h (2)
tt(15-86)- `` (81-81)
Tactility/Private/Tactility/service/development/DevelopmentService.h (3)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
Drivers/RgbDisplay/Source/RgbDisplay.h (4)
TactilityCore/Include/Tactility/Mutex.h (2)
tt(15-68)lock(48-51)Tactility/Include/Tactility/hal/display/DisplayDriver.h (1)
tt(6-30)Tactility/Include/Tactility/hal/display/DisplayDevice.h (1)
DisplayDevice(15-47)TactilityCore/Include/Tactility/RecursiveMutex.h (2)
lock(47-50)RecursiveMutex(37-39)
Devices/cyd-4848s040c/Source/devices/St7701Display.h (3)
Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.h (1)
tt(8-33)TactilityCore/Include/Tactility/Mutex.h (1)
tt(15-68)TactilityCore/Include/Tactility/RecursiveMutex.h (1)
tt(14-67)
Tactility/Private/Tactility/app/fileselection/State.h (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)
Tactility/Private/Tactility/service/espnow/EspNowService.h (3)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
Tactility/Private/Tactility/app/AppInstance.h (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)
Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.h (1)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)
Tactility/Include/Tactility/hal/sdcard/SdmmcDevice.h (1)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (1)
std(57-66)
Tactility/Private/Tactility/app/wifimanage/State.h (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)
Tactility/Private/Tactility/service/gui/GuiService.h (3)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
TactilityCore/Include/Tactility/Mutex.h (3)
TactilityCore/Include/Tactility/Lock.h (2)
Lock(13-39)lock(69-71)TactilityCore/Include/Tactility/RecursiveMutex.h (1)
lock(47-50)TactilityCore/Include/Tactility/kernel/Kernel.h (2)
isIsr(21-21)isIsr(23-23)
Tactility/Include/Tactility/service/gps/GpsService.h (3)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
Tests/TactilityCore/MutexTest.cpp (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)TactilityCore/Include/Tactility/Mutex.h (1)
Mutex(38-40)
Tactility/Include/Tactility/hal/gps/GpsDevice.h (3)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
Tactility/Private/Tactility/service/ServiceInstance.h (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)
TactilityCore/Include/Tactility/RecursiveMutex.h (1)
TactilityCore/Include/Tactility/Mutex.h (2)
tt(15-68)lock(48-51)
Tactility/Source/hal/Device.cpp (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)
⏰ 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: macOS
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: run
🔇 Additional comments (44)
Tactility/Private/Tactility/service/development/DevelopmentService.h (1)
6-6: LGTM!Clean migration to the dedicated
RecursiveMutextype. The default construction simplifies the declaration compared to the previousMutex(Mutex::Type::Recursive)pattern, and aligns with the PR objective of separating recursive mutex concerns into its own class.Also applies to: 16-16
Tactility/Private/Tactility/service/ServiceInstance.h (1)
13-13: The default constructor properly initializes the FreeRTOS mutex. Thehandlemember is initialized at declaration time viaxSemaphoreCreateMutex(), and the constructor asserts that initialization succeeded. The simplification to default construction is safe and correct.Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.h (1)
5-5: LGTM! Include directive correctly updated.The include change aligns with the new
RecursiveMutextype on line 16.Tactility/Private/Tactility/app/fileselection/State.h (1)
3-3: Clean refactoring to dedicated RecursiveMutex type.The change from
Mutex(Mutex::Type::Recursive)toRecursiveMuteximproves type safety and code clarity by using a dedicated type for recursive locking. The default construction is cleaner than explicit initialization.RecursiveMutexinherits from theLockbase class, which provides thewithLock()API variants used on line 35, ensuring full compatibility.Tactility/Private/Tactility/app/files/State.h (1)
3-3: LGTM! Consistent refactoring.The changes mirror the refactoring pattern applied consistently across the codebase. The migration from
Mutex(Mutex::Type::Recursive)toRecursiveMuteximproves clarity and follows the PR objectives.Also applies to: 23-23
Tactility/Private/Tactility/app/wifimanage/State.h (1)
4-4: LGTM! Cleaner recursive mutex declaration.The refactoring from
Mutex(Mutex::Type::Recursive)toRecursiveMutexmakes the intent more explicit and simplifies the code. All required methods are provided by theLockbase class:withLock(),asScopedLock(),lock(),unlock(), andlock(timeout).Tactility/Private/Tactility/service/espnow/EspNowService.h (2)
12-12: LGTM! Clean refactoring to use dedicated RecursiveMutex header.The include change correctly reflects the migration from
Mutexwith recursive type to the dedicatedRecursiveMutexclass.
30-30: Simplified member declaration is correct.The change from
Mutex mutex = Mutex(Mutex::Type::Recursive)toRecursiveMutex mutexcorrectly implements the refactoring.RecursiveMutexhas a proper explicit default constructor and inherits all required locking methods from theLockbase class, making it fully compatible with the scoped locking pattern used throughout EspNowService.Tactility/Source/app/localesettings/LocaleSettings.cpp (1)
2-2: LGTM - RecursiveMutex member updated.The mutex type has been updated to
RecursiveMutex. The change is consistent with the broader refactoring.Also applies to: 30-30
Tactility/Private/Tactility/app/AppInstance.h (1)
28-28: LGTM - Simplified initialization.The explicit
Mutex::Type::Normalinitialization has been removed in favor of default construction. This simplifies the code while maintaining the same behavior (normal mutex).Devices/simulator/Source/hal/SimulatorSdCard.h (1)
4-4: LGTM - Clean migration to RecursiveMutex.The initialization has been cleanly migrated from
std::make_shared<tt::Mutex>(tt::Mutex::Type::Recursive)tostd::make_shared<tt::RecursiveMutex>(). This makes the code more explicit and type-safe.Also applies to: 19-19
Tactility/Source/hal/spi/Spi.cpp (1)
3-3: LGTM - RecursiveMutex as default SPI lock.The default lock for SPI devices has been changed to
RecursiveMutex. This is a reasonable choice for SPI operations where nested locking might occur (e.g., device operations calling lower-level bus operations).Also applies to: 27-27
Tactility/Source/service/ServiceRegistration.cpp (1)
20-21: LGTM - Simplified static mutex initialization.The explicit
Mutex::Type::Normalinitialization has been removed in favor of default construction for both static mutexes. This simplifies the code while maintaining the same behavior.Tactility/Include/Tactility/network/HttpServer.h (1)
5-5: RecursiveMutex is required to prevent deadlock in the stop() method.The mutex must be recursive because
stop()acquires the lock and then callsisStarted(), which attempts to acquire the same lock again. Without recursive locking, this same-thread reacquisition would cause a deadlock.Tactility/Include/Tactility/hal/gps/GpsDevice.h (1)
7-7: RecursiveMutex is necessary - remove the verification request.The
start()method (line 118) andstop()method (lines 138, 156) both acquire the lock then callsetState(), which itself attempts to acquire the same lock. This recursive locking pattern prevents deadlock and justifies the use ofRecursiveMutex.Tactility/Include/Tactility/hal/gps/Satellites.h (1)
4-4: The recursive mutex change is correct and necessary.The implementation shows nested method calls that require recursive locking:
notify()acquires the lock and callsfindWithFallback(), which then callsfindUnusedRecord()andfindRecordToRecycle(), each attempting to acquire the same lock. Without a recursive mutex, the same thread would deadlock when trying to lock an already-held lock. The change is justified.Drivers/RgbDisplay/Source/RgbDisplay.h (1)
4-4: LGTM: Clean migration to RecursiveMutex.The include and lock member updates correctly replace the recursive Mutex with the new dedicated RecursiveMutex type. Both types implement the Lock interface, maintaining API compatibility.
Also applies to: 11-11
Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (1)
4-4: LGTM: Correct migration to RecursiveMutex.The mutex field type and include are properly updated to use the new RecursiveMutex class.
Also applies to: 18-18
Tactility/Source/app/AppRegistration.cpp (1)
16-16: LGTM: Simplified Mutex construction.The Mutex now defaults to normal (non-recursive) behavior, eliminating the need for explicit Type::Normal initialization. This simplification is consistent with the broader refactoring.
Tests/TactilityCore/MutexTest.cpp (1)
8-8: LGTM: Test updates match new Mutex API.The tests correctly use the default Mutex constructor, confirming that Mutex now defaults to normal (non-recursive) behavior. Test logic and assertions remain unchanged.
Also applies to: 33-33, 40-40, 45-45
TactilityC/Source/tt_lock.cpp (1)
2-2: LGTM: C API wrapper correctly updated.The lock allocation function properly distinguishes between normal and recursive mutexes, using
Mutex()for normal locks andRecursiveMutex()for recursive locks. This maintains the C API contract while using the new mutex types.Also applies to: 15-15, 18-18
Devices/simulator/Source/LvglTask.cpp (1)
5-5: LGTM: LVGL mutexes correctly migrated to RecursiveMutex.Both LVGL-related mutexes are properly updated to use the new RecursiveMutex type. Recursive locking is appropriate for LVGL task synchronization where nested lock acquisitions can occur.
Also applies to: 13-14
Tactility/Private/Tactility/service/gui/GuiService.h (1)
5-5: LGTM: GuiService mutex correctly migrated.The recursive mutex is properly updated to use the new RecursiveMutex type. The lock/unlock methods at lines 47-53 continue to work unchanged with the new type.
Also applies to: 24-24
Tactility/Include/Tactility/service/gps/GpsService.h (1)
4-4: LGTM: Proper use of both mutex types.The class correctly uses RecursiveMutex for the main mutex (line 24) and retains a normal Mutex for stateMutex (line 25). This demonstrates appropriate selection of mutex types based on locking requirements. Both includes are necessary for the two different mutex types.
Also applies to: 24-25
Tactility/Source/app/i2cscanner/I2cScanner.cpp (1)
12-12: LGTM!The migration from
Mutex(Mutex::Type::Recursive)toRecursiveMutexis correctly implemented. The include is properly added and all existing lock/unlock calls remain compatible through theLockinterface.Also applies to: 28-28
Tactility/Source/lvgl/Statusbar.cpp (1)
11-11: LGTM!The replacement of
MutexwithRecursiveMutexin theStatusbarDatastruct is correct and maintains compatibility with all existing lock/unlock operations.Also applies to: 30-30
TactilityCore/Include/Tactility/Mutex.h (1)
38-40: LGTM! Clean implementation of non-recursive mutex.The implementation correctly uses
xSemaphoreCreateMutex()and corresponding non-recursive FreeRTOS APIs (xSemaphoreTake/xSemaphoreGive). The ISR safety checks are properly applied to all operations.Also applies to: 48-51, 56-59, 62-65
TactilityCore/Include/Tactility/RecursiveMutex.h (1)
37-39: LGTM! Correct implementation of recursive mutex.The implementation correctly uses
xSemaphoreCreateRecursiveMutex()and the corresponding recursive FreeRTOS APIs (xSemaphoreTakeRecursive/xSemaphoreGiveRecursive). ISR safety checks are properly applied to all operations, and the constructor assertion ensures the handle is valid.Also applies to: 47-50, 55-58, 61-64
Tactility/Include/Tactility/service/loader/Loader.h (1)
8-8: LGTM!The migration to
RecursiveMutexinLoaderServiceis correctly implemented and maintains compatibility with existing usage through theLockinterface.Also applies to: 30-30
Tactility/Private/Tactility/service/screenshot/ScreenshotTask.h (1)
8-8: LGTM!The replacement of
MutexwithRecursiveMutexinScreenshotTaskfollows the correct pattern and is consistent with the refactoring objectives.Also applies to: 25-25
Tactility/Source/hal/Device.cpp (1)
3-3: LGTM!The global device registry mutex is correctly updated to use
RecursiveMutex. All existing usage throughasScopedLock()remains compatible.Also applies to: 9-9
Devices/cyd-2432s028rv3/Source/devices/SdCard.cpp (1)
4-4: LGTM!The migration from
Mutex::Type::Recursiveto the dedicatedRecursiveMutexclass is correctly implemented. The include is properly added and the usage aligns with the broader refactoring pattern across the codebase.Also applies to: 15-15
Devices/waveshare-s3-lcd-13/Source/devices/SdCard.cpp (1)
5-5: LGTM!Consistent migration to
RecursiveMutexwith the appropriate include added.Also applies to: 16-16
Devices/cyd-e32r32p/Source/devices/SdCard.cpp (1)
3-3: LGTM!The
RecursiveMutexmigration is correctly applied.Also applies to: 15-15
Devices/waveshare-s3-touch-lcd-128/Source/devices/SdCard.cpp (1)
5-5: LGTM!Consistent with the refactoring pattern across other device configurations.
Also applies to: 16-16
Tactility/Include/Tactility/hal/sdcard/SdmmcDevice.h (1)
7-11: LGTM!The header correctly migrates the private mutex member to
RecursiveMutex. The inline initialization at declaration is clean, and thegetLock()method at line 81 correctly returns the mutex asstd::shared_ptr<Lock>, confirmingRecursiveMutexproperly inherits from theLockinterface.Also applies to: 22-22
Devices/cyd-e32r28t/Source/devices/SdCard.cpp (1)
3-3: LGTM!The
RecursiveMutexmigration follows the established pattern.Also applies to: 14-14
Devices/cyd-2432s032c/Source/devices/SdCard.cpp (1)
5-5: LGTM!Clean migration to
RecursiveMutex, consistent with other device configurations.Also applies to: 19-19
Devices/cyd-2432s024c/Source/devices/SdCard.cpp (1)
6-6: LGTM!The
RecursiveMutexmigration is correctly applied, completing the consistent refactoring across all device SD card configurations.Also applies to: 20-20
Devices/cyd-2432s028r/Source/devices/SdCard.cpp (1)
4-4: LGTM!The migration from
Mutex(Mutex::Type::Recursive)to the dedicatedRecursiveMutexclass is correct. The include and usage are properly updated.Also applies to: 15-15
Tactility/Source/service/wifi/WifiMock.cpp (1)
8-8: LGTM!The switch from
Mutex(Mutex::Type::Recursive)toRecursiveMutexsimplifies the declaration and aligns with the new class separation.Also applies to: 18-18
Tactility/Source/service/wifi/WifiEsp.cpp (1)
12-12: LGTM!The migration to
RecursiveMutexfor bothradioMutexanddataMutexis correct. The usage throughout the file (viaasScopedLock(), directlock()/unlock()) should work identically with the new class.Also applies to: 52-53
Devices/cyd-jc2432w328c/Source/devices/SdCard.cpp (1)
5-5: LGTM!Consistent migration to
RecursiveMutexfor the SD card SPI lock configuration.Also applies to: 19-19
Devices/cyd-jc8048w550c/Source/devices/SdCard.cpp (1)
5-5: LGTM!Consistent migration to
RecursiveMutexfor the SD card SPI lock configuration.Also applies to: 16-16
Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
TactilityCore/Include/Tactility/RecursiveMutex.h (2)
31-31: Consider simplifying the unique_ptr initialization.The initialization is quite verbose. Consider using
autoor extracting to a factory function for better readability.🔎 Proposed simplification
- std::unique_ptr<std::remove_pointer_t<QueueHandle_t>, SemaphoreHandleDeleter> handle = std::unique_ptr<std::remove_pointer_t<QueueHandle_t>, SemaphoreHandleDeleter>(xSemaphoreCreateRecursiveMutex()); + std::unique_ptr<std::remove_pointer_t<QueueHandle_t>, SemaphoreHandleDeleter> handle{xSemaphoreCreateRecursiveMutex()};
67-67: Add namespace identifier to closing comment for consistency.The closing namespace comment should include "tt" to match the style in
Mutex.h(line 68) and improve code navigation.🔎 Proposed fix
-} // namespace +} // namespace ttTactilityCore/Include/Tactility/Mutex.h (1)
32-32: Consider simplifying the unique_ptr initialization.Same verbose initialization pattern as in
RecursiveMutex.h. Consider using brace initialization for better readability.🔎 Proposed simplification
- std::unique_ptr<std::remove_pointer_t<QueueHandle_t>, SemaphoreHandleDeleter> handle = std::unique_ptr<std::remove_pointer_t<QueueHandle_t>, SemaphoreHandleDeleter>(xSemaphoreCreateMutex()); + std::unique_ptr<std::remove_pointer_t<QueueHandle_t>, SemaphoreHandleDeleter> handle{xSemaphoreCreateMutex()};
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.hTactilityCore/Include/Tactility/Mutex.hTactilityCore/Include/Tactility/RecursiveMutex.h
🧰 Additional context used
🧬 Code graph analysis (3)
Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.h (1)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)
TactilityCore/Include/Tactility/RecursiveMutex.h (2)
TactilityCore/Include/Tactility/Mutex.h (2)
tt(15-68)lock(48-51)TactilityCore/Include/Tactility/Lock.h (1)
Lock(13-39)
TactilityCore/Include/Tactility/Mutex.h (3)
TactilityCore/Include/Tactility/RecursiveMutex.h (2)
tt(14-67)lock(47-50)TactilityCore/Include/Tactility/Lock.h (3)
tt(8-46)Lock(13-39)lock(69-71)TactilityCore/Include/Tactility/kernel/Kernel.h (2)
isIsr(21-21)isIsr(23-23)
🔇 Additional comments (6)
Tactility/Private/Tactility/service/memorychecker/MemoryCheckerService.h (1)
16-16: Excellent change—correctly addresses the previous review feedback.The switch from a recursive mutex to a regular
Mutexis appropriate. As verified in the previous review, the implementation shows no re-entrant locking scenarios, making a regular mutex the correct and more efficient choice.TactilityCore/Include/Tactility/RecursiveMutex.h (3)
1-4: Past review comment has been addressed.The file header documentation now correctly states "RecursiveMutex" instead of "Mutex". The previous concern has been resolved.
16-19: Past review comment has been addressed.The documentation now correctly specifies that this class wraps only
xSemaphoreCreateRecursiveMutexfor recursive mutex behavior. The previous concern has been resolved.
47-58: LGTM: Correct usage of recursive FreeRTOS functions.The implementation correctly uses
xSemaphoreTakeRecursiveandxSemaphoreGiveRecursivefor recursive mutex semantics, with proper ISR context checks.TactilityCore/Include/Tactility/Mutex.h (2)
18-18: LGTM: Correct implementation of non-recursive mutex.The class correctly wraps FreeRTOS
xSemaphoreCreateMutexand uses the non-recursive variantsxSemaphoreTakeandxSemaphoreGive. The simplification from the previous Type-based approach is clean and the separation of concerns withRecursiveMutexis well-executed.Also applies to: 32-32, 48-51, 56-59
38-40: Constructor looks good.The explicit default constructor with handle assertion is appropriate for early detection of mutex creation failures.
Mutexis now a separate class:RecursiveMutexMutexandRecursiveMutexare now header-only implementations