Sd card map pins, drive index & fix events #3008
Conversation
WalkthroughThe changes enhance the ESP32 platform's SD card functionality, particularly in card detection and flexible GPIO pin configuration. New event types related to card detection have been introduced, along with an enumeration for card states. Additionally, modifications to structures and mapping functions improve handling for SDMMC devices, ensuring compatibility with various SD card modules and enhancing the overall robustness of the codebase. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
src/System.IO.FileSystem/nf_sys_io_filesystem.h (4)
24-28: Add documentation forCardDetectStateenumeration.Consider adding comments to describe each state in the
CardDetectStateenumeration for better clarity and maintainability.+ // Represents the state when the card is inserted. CardDetectState_Inserted = 0, + // Represents the state when the card is removed. CardDetectState_Removed = 1,
101-107: Add documentation forCardDetectChangedEventArgsstructure.Consider adding comments to describe the purpose and usage of each field in the
CardDetectChangedEventArgsstructure for better clarity and maintainability.+ // Represents the state of the card detection. static const int FIELD___cardState = 1; + // Represents the index of the slot where the card is detected. static const int FIELD___slotIndex = 2;
54-54: The new event typeStorageEventManager_StorageEventType_CardDetectChangedis not integrated.The search did not reveal any usage of the newly added event type beyond its declaration. This suggests that it might not be integrated into the event management system. Ensure that all necessary handlers or other relevant parts of the code are updated to handle this new event type.
- Verify if there are event handlers or functions that should be updated to handle
StorageEventManager_StorageEventType_CardDetectChanged.Analysis chain
Ensure integration of
StorageEventManager_StorageEventType_CardDetectChanged.Verify that the new event type is correctly integrated with the event management system and that all necessary handlers are updated.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `StorageEventManager_StorageEventType_CardDetectChanged`. # Test: Search for the usage of the new event type in the codebase. Expect: Correct integration with event handlers. rg --type c 'StorageEventManager_StorageEventType_CardDetectChanged'Length of output: 180
115-117: Inconsistent field renumbering detected forFIELD___slotIndex.The field
FIELD___slotIndexhas two different values (2 and 7) in the codebase, which indicates an inconsistency in the renumbering process. Additionally,FIELD___cardDetectPinandFIELD___cardDetectedStateare declared but not used elsewhere. This could lead to potential issues if not addressed.
FIELD___slotIndexappears with values 2 and 7 insrc/System.IO.FileSystem/nf_sys_io_filesystem.h.Please review and ensure consistent usage and integration of these fields across the codebase.
Analysis chain
Verify field renumbering and integration of
FIELD___cardDetectedState.Ensure that the renumbering of fields and the addition of
FIELD___cardDetectedStateare consistent with the rest of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify field renumbering and integration of `FIELD___cardDetectedState`. # Test: Search for the usage of the updated fields in the codebase. Expect: Consistent field numbers and correct usage. rg --type c 'FIELD___cardDetectedState'Length of output: 139
Script:
#!/bin/bash # Description: Verify field renumbering and integration of `FIELD___cardDetectPin` and `FIELD___slotIndex`. # Test: Search for the usage of the updated fields in the codebase. Expect: Consistent field numbers and correct usage. rg --type c 'FIELD___cardDetectPin' rg --type c 'FIELD___slotIndex'Length of output: 349
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- src/HAL/Include/nanoHAL_System_IO_FileSystem.h (1 hunks)
- src/System.IO.FileSystem/nf_sys_io_filesystem.cpp (3 hunks)
- src/System.IO.FileSystem/nf_sys_io_filesystem.h (9 hunks)
- targets/ESP32/_common/DeviceMapping_common.cpp (4 hunks)
- targets/ESP32/_common/ESP32_DeviceMapping.cpp (1 hunks)
- targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp (1 hunks)
- targets/ESP32/_common/Target_System_IO_FileSystem.c (4 hunks)
- targets/ESP32/_include/Esp32_DeviceMapping.h (4 hunks)
- targets/ESP32/_nanoCLR/System.Device.Gpio/cpu_gpio.cpp (6 hunks)
- targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (17 hunks)
Additional comments not posted (27)
src/HAL/Include/nanoHAL_System_IO_FileSystem.h (1)
42-44: Addition ofStorageEventType_CardDetectChangedis appropriate.The new event type
StorageEventType_CardDetectChangedenhances the event handling system by allowing detection and handling of card status changes. Ensure that this new event type is correctly integrated and utilized throughout the system.targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp (1)
72-79: Introduction ofEsp32_SDMMC_DevicePinMapis well-structured.The addition of
Esp32_SDMMC_DevicePinMapfor managing SDMMC device pin mappings enhances flexibility and configurability for developers working with SDMMC devices on the ESP32 platform. Ensure these mappings are correctly implemented and tested in the context of the platform.targets/ESP32/_include/Esp32_DeviceMapping.h (4)
44-53: Addition ofEsp32SdmmcPinenumeration is appropriate.The
Esp32SdmmcPinenumeration defines constants related to SDMMC pin configurations, which enhances the configurability of device pin mappings. Ensure these constants are correctly used in the implementation.
65-65: Inclusion ofDEV_TYPE_SDMMCinEsp32_MapDeviceTypeis appropriate.The addition of
DEV_TYPE_SDMMCto theEsp32_MapDeviceTypeenum correctly reflects the inclusion of SDMMC as a valid device type. Ensure this is correctly integrated into the device mapping logic.
96-98: Conditional declaration ofEsp32_SDMMC_DevicePinMapis appropriate.The conditional inclusion of
Esp32_SDMMC_DevicePinMapbased on specific configuration macros ensures that the mapping is only available for relevant targets. This is a good practice for maintaining platform-specific configurations.
114-116: Addition ofEsp32_GetSDmmcDevicePins_Cfunction is appropriate.The
Esp32_GetSDmmcDevicePins_Cfunction provides C code access to SDMMC device pins, enhancing the flexibility of device pin retrieval. Ensure this function is correctly implemented and utilized in the relevant parts of the codebase.targets/ESP32/_common/ESP32_DeviceMapping.cpp (1)
98-104: SDMMC Pin Mapping Introduced Successfully.The introduction of
Esp32_SDMMC_DevicePinMapaligns with the PR objectives to enhance SD card functionality on the ESP32 platform. The use ofGPIO_NUM_NCfor unconfigured pins is appropriate and maintains clarity.src/System.IO.FileSystem/nf_sys_io_filesystem.cpp (2)
242-244: Assembly Versioning Updated Appropriately.The update to the assembly version identifier and version tuple reflects changes in compatibility or feature set, aligning with the introduction of new functionality.
Line range hint
12-94: Verify Method Lookup Array Expansion.The expansion of the
method_lookuparray with additionalNULLentries suggests preparation for future method implementations. Ensure that this change does not affect current functionality or cause unexpected behavior.Verification successful
Verify Method Lookup Array Expansion.
The expansion of the
method_lookuparray with additionalNULLentries insrc/System.IO.FileSystem/nf_sys_io_filesystem.cppis consistent with similar arrays across the codebase. This change is likely a preparatory step for future method additions and does not appear to affect current functionality or cause unexpected behavior. However, ensure that no specific logic relies on the array's size or contents beyond indexing.
- File:
src/System.IO.FileSystem/nf_sys_io_filesystem.cppScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the expanded method_lookup array does not affect current functionality. # Test: Search for all references to the method_lookup array. Expect: No unexpected behavior or errors. rg --type cpp 'method_lookup'Length of output: 7354
targets/ESP32/_common/DeviceMapping_common.cpp (3)
61-64: SDMMC Device Pin Mapping Added Successfully.The addition of the
DEV_TYPE_SDMMCcase inEsp32_GetMappedDevicePinsenhances the functionality of the device mapping system, aligning with the PR objectives.
72-76: C-Compatible Interface for SDMMC Pins Introduced.The introduction of
Esp32_GetSDmmcDevicePins_Cprovides a C-compatible interface for retrieving SDMMC device pins, enhancing interoperability.
Line range hint
117-219: SDMMC Device Pin Mapping Set Successfully.The addition of the
DEV_TYPE_SDMMCcase inEsp32_SetMappedDevicePinsexpands the functionality of the device mapping system, ensuring correct pin assignments for SDMMC devices.targets/ESP32/_common/Target_System_IO_FileSystem.c (5)
102-102: Improve logging clarity.The log message now specifies the drive being initialized, which enhances debugging clarity. This is a good practice for maintaining detailed logs.
Line range hint
71-79:
Enhance error logging for mount failures.The error logging provides detailed information about the failure, which is crucial for debugging. Ensure that all possible error codes are handled appropriately.
152-157: Verify SPI bus slot assignments for ESP32 variants.The SPI bus slot configuration is updated for different ESP32 variants. Ensure that the slot assignments are correct and consistent with the hardware specifications.
Verification successful
SPI Bus Slot Assignments Verified
The SPI bus slot assignments for
SPI2_HOSTandHSPI_HOSTinTarget_System_IO_FileSystem.care consistent with the intended ESP32 variants. The assignments align with the hardware specifications for ESP32_C3/S3/C6/H2 and ESP32, respectively. No issues found.
SPI2_HOSTis used for ESP32_C3/S3/C6/H2.HSPI_HOSTis used for ESP32.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify SPI bus slot assignments for ESP32 variants. # Test: Search for the SPI bus slot assignments in the codebase. Expect: Correct slot numbers for each ESP32 variant. rg --type c 'host.slot = spiBus'Length of output: 211
Script:
#!/bin/bash # Search for the definitions of SPI2_HOST and HSPI_HOST in the codebase to verify their correctness. rg --type c 'SPI2_HOST|HSPI_HOST'Length of output: 530
110-115: Verify GPIO pin assignments for ESP32S3 and ESP32P4.The GPIO pin assignments for ESP32S3 and ESP32P4 are added. Ensure these assignments are correct and consistent with the hardware specifications.
124-127: Verify GPIO pin assignments for 4-bit mode on ESP32S3 and ESP32P4.Ensure that the GPIO pin assignments for the 4-bit mode are correct and align with the hardware design for ESP32S3 and ESP32P4.
targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (6)
19-21: Verify correctness ofslotIndexdecoding incardDetect_interrupt.Ensure that the logic for decoding
slotIndexfrompArgis correct and consistent with the intended functionality.
27-30: Ensure correctness of pin selection logic inGetMMCPins.The function uses
slotIndexto select pin mappings. Verify that this logic is consistent with the hardware configuration and does not introduce any errors.
9-12: Ensure correctness of direct event type setting inpostManagedStorageEvent.The event type is directly set to
StorageEventType_CardDetectChanged. Verify that this change aligns with the intended functionality and does not introduce any regressions.Verification successful
Direct event type setting in
postManagedStorageEventis appropriate.The event type
StorageEventType_CardDetectChangedis correctly defined and consistent across the codebase, confirming that its direct setting in the function aligns with intended functionality. No issues found.
- Defined in
src/System.IO.FileSystem/nf_sys_io_filesystem.handsrc/HAL/Include/nanoHAL_System_IO_FileSystem.h.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure correctness of direct event type setting in `postManagedStorageEvent`. # Test: Search for the usage of `postManagedStorageEvent` in the codebase. Expect: Consistent event type usage. rg --type c 'postManagedStorageEvent'Length of output: 37
Script:
#!/bin/bash # Check for the definition and usage of `StorageEventType_CardDetectChanged` in the codebase. rg --type c 'StorageEventType_CardDetectChanged'Length of output: 254
Line range hint
340-344:
Verify unmounting logic inUnmountNative___VOID.Ensure that the logic for unmounting based on
slotIndexis correct and consistent with the intended functionality.Verification successful
Unmounting logic is correctly implemented.
The
Storage_UnMountSDCardfunction correctly handles thedriveIndex(corresponding toslotIndex), ensuring the unmounting logic is consistent with the intended functionality. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify unmounting logic in `UnmountNative___VOID`. # Test: Search for the usage of unmounting logic in the codebase. Expect: Correct handling based on `slotIndex`. rg --type c 'UnmountNative___VOID'Length of output: 132
Script:
#!/bin/bash # Search for the implementation of `UnmountNative___VOID` in the codebase. ast-grep --lang c --pattern 'void UnmountNative___VOID($$$) { $$$ }' # Search for references to `Storage_UnMountSDCard` to understand its usage. rg --type c 'Storage_UnMountSDCard'Length of output: 495
Script:
#!/bin/bash # Search for the implementation of `Storage_UnMountSDCard` in the codebase. ast-grep --lang c --pattern 'bool Storage_UnMountSDCard(int driveIndex) { $$$ }'Length of output: 1390
130-150: Verify card detection logic inInitNative___VOID.Ensure that the logic for handling card detection pins and states is correct and robust, particularly the conditional setting of pin modes and initial event firing.
239-243: Verify bounds checking forslotIndexinMountNative___VOID.Ensure that the bounds checking for
slotIndexis correct and prevents invalid operations.targets/ESP32/_nanoCLR/System.Device.Gpio/cpu_gpio.cpp (4)
61-61: Addition ofcurrentmember approved.The
currentmember enhances the state management of GPIO inputs, particularly in debounce scenarios. This change is well-aligned with the PR objectives.
148-148: Enhancement in debounce handler approved.Updating the
currentstate with the actual GPIO state ensures accurate state representation during debouncing.
Line range hint
328-341:
ISR logic enhancement approved.Updating the
currentstate during the ISR ensures that the expected state calculation is based on the most recent actual state, improving event handling accuracy.
Line range hint
403-418:
Initialization ofcurrentstate approved.Setting the
currentstate to the actual pin state ensures consistent state management when enabling a GPIO input pin.
Automated fixes for code style.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (17 hunks)
Additional comments not posted (7)
targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (7)
9-11: LGTM! Updated parameter for slot index.The change from
driveIndextoslotIndexis appropriate and aligns with the PR objectives.
26-29: LGTM! Enhanced flexibility with slot index.The addition of
slotIndexfor selecting pin mappings enhances the flexibility of the function.
Line range hint
162-191:
LGTM! Consistent use of slot index for resource management.The use of
slotIndexfor managing resources is consistent with the rest of the changes.
Line range hint
215-297:
LGTM! Robust slot index handling for mounting.The bounds checking for
slotIndexand mounting logic are well-implemented.
Line range hint
319-347:
LGTM! Consistent unmount logic with slot index.The unmount logic using
slotIndexis consistent and correct.
Line range hint
374-391:
LGTM! Correct polling logic for card detection.The polling logic and comparison with
cardDetectedStateappear correct.
Line range hint
72-149:
Verify logic for GPIO pull-up/down configuration.The logic for setting
pullupsbased oncardDetectedStateseems correct, but ensure it matches the hardware requirements.Verification successful
Logic for GPIO pull-up/down configuration is correct.
The logic for setting the
pullupsvariable based oncardDetectedStateis consistent with the intended functionality and matches the hardware requirements. No issues were found in the usage ofcardDetectedState.
- The
pullupsvariable is set correctly based oncardDetectedState.- The initial state check and event firing logic are consistent with expected behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for setting GPIO pull-up/down configuration based on cardDetectedState. # Test: Search for the usage of cardDetectedState. Expect: Consistent usage across related functions. rg --type cpp -A 5 $'cardDetectedState'Length of output: 5186
...oCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp
Outdated
Show resolved
Hide resolved
…9-773f-468b-9ca1-b4f1e5fd1f6a' into MMC-pins-for-S3
….cpp Suggested coderabit change
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- targets/ESP32/_common/DeviceMapping_common.cpp (3 hunks)
- targets/ESP32/_include/Esp32_DeviceMapping.h (5 hunks)
- targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (17 hunks)
Files skipped from review due to trivial changes (1)
- targets/ESP32/_include/Esp32_DeviceMapping.h
Additional comments not posted (11)
targets/ESP32/_common/DeviceMapping_common.cpp (4)
60-63: LGTM! Support for SDMMC added.The addition of the
DEV_TYPE_SDMMCcase inEsp32_GetMappedDevicePinsis correct and aligns with the PR objectives.
71-75: LGTM! New C-compatible function added.The
Esp32_GetSDmmcDevicePins_Cfunction is a straightforward addition for C compatibility and is correctly implemented.
116-119: LGTM! Support for SDMMC added.The addition of the
DEV_TYPE_SDMMCcase inEsp32_SetMappedDevicePinsis correct and aligns with the PR objectives.
210-218: LGTM! Support for SDMMC added.The addition of the
DEV_TYPE_SDMMCcase inEsp32_SetMappedDevicePins(alternateFunction version) is correct and aligns with the PR objectives.targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (7)
9-11: LGTM! Improved event management.The update to use
slotIndexinstead ofdriveIndexinpostManagedStorageEventaligns with the PR objectives and improves clarity.
18-20: Fix incorrect decoding ofslotIndex.The expression
(uint32_t)(0xff && pArg)does not correctly extractslotIndex. It should likely be(uint32_t)pArg & 0xffto mask the lower 8 bits.- int slotIndex = (uint32_t)(0xff && pArg); + int slotIndex = (uint32_t)pArg & 0xff;
26-29: LGTM! Enhanced flexibility for multiple slots.The update to include
slotIndexinGetMMCPinsenhances the function's flexibility and aligns with the PR objectives.
Line range hint
72-149: LGTM! Improved initialization for multiple slots.The updates to initialize
slotIndexand use it inGetMMCPinsand card detection logic improve the handling of multiple slots and align with the PR objectives.
Line range hint
162-191: LGTM! Proper cleanup and resource management.The use of
slotIndexto unreserve pins ensures proper cleanup and resource management for multiple slots.
Line range hint
215-297: LGTM! Enhanced robustness for mounting logic.The inclusion of bounds checking for
slotIndexand its use in mounting logic enhance robustness and ensure correct operation across multiple slots.
Line range hint
319-339: LGTM! Consistent unmounting logic.The use of
slotIndexin unmounting logic ensures consistency with the mounting logic and correct operation for multiple slots.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (17 hunks)
Additional comments not posted (7)
targets/ESP32/_nanoCLR/System.IO.FileSystem/nf_sys_io_filesystem_nanoFramework_System_IO_FileSystem_SDCard.cpp (7)
9-11: Simplified event posting is approved.The function now directly sets the event type, reducing complexity.
26-29: Enhanced flexibility for pin mappings is approved.The addition of
slotIndexallows for multiple configurations, improving the function's utility.
Line range hint
72-149: Initialization logic improvements are approved.The changes ensure correct initialization and configuration of SD card settings, including slot index handling and card detect logic.
Line range hint
163-195: Resource management improvements are approved.The function ensures proper cleanup by unreserving pins and disabling interrupts, aligning with best practices.
Line range hint
219-301: Mounting logic enhancements are approved.The changes add robustness through bounds checking and a retry mechanism for SPI mounting, ensuring reliable operations.
Line range hint
323-355: Unmounting logic improvements are approved.The function ensures proper unmounting and state management, maintaining consistency and reliability.
Line range hint
378-395: Card detection logic improvements are approved.The function ensures correct detection logic and effective error handling, contributing to reliable operations.
| // Decode pArg | ||
| int slotIndex = (uint32_t)0xff && pArg; |
There was a problem hiding this comment.
Fix incorrect decoding of slotIndex.
The expression (uint32_t)(0xff && pArg) does not correctly extract slotIndex. It should likely be (uint32_t)pArg & 0xff to mask the lower 8 bits.
- int slotIndex = (uint32_t)(0xff && pArg);
+ int slotIndex = (uint32_t)pArg & 0xff;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Decode pArg | |
| int slotIndex = (uint32_t)0xff && pArg; | |
| // Decode pArg | |
| int slotIndex = (uint32_t)pArg & 0xff; |
Description
Native side of fixing issues with SD card.
This change requires other PR for
Motivation and Context
How Has This Been Tested?
Tested locally with updated Mounting sample
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor