Update sdcard etc #110
Update sdcard etc #110
Conversation
|
@AdrianSoundy I've fixed the checklist for you. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (7)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe recent changes involve updating the dependency for the 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (11)
System.IO.FileSystem.UnitTests/DirectoryUnitTests.csis excluded by none and included by noneSystem.IO.FileSystem.UnitTests/FileSystemUnitTestsBase.csis excluded by none and included by noneSystem.IO.FileSystem/Properties/AssemblyInfo.csis excluded by none and included by noneSystem.IO.FileSystem/System.IO.FileSystem.nfprojis excluded by none and included by noneSystem.IO.FileSystem/nanoFramework/CardDetectChangedEventArgs.csis excluded by none and included by noneSystem.IO.FileSystem/nanoFramework/SDCard.csis excluded by none and included by noneSystem.IO.FileSystem/nanoFramework/SDCardCDParameters.csis excluded by none and included by noneSystem.IO.FileSystem/nanoFramework/SDCardMmcParameters.csis excluded by none and included by noneSystem.IO.FileSystem/nanoFramework/SDCardSpiParameters.csis excluded by none and included by noneSystem.IO.FileSystem/nanoFramework/StorageEventManager.csis excluded by none and included by noneSystem.IO.FileSystem/packages.configis excluded by none and included by none
Files selected for processing (1)
- System.IO.FileSystem/packages.lock.json (1 hunks)
Additional comments not posted (1)
System.IO.FileSystem/packages.lock.json (1)
37-39: Update toNerdbank.GitVersioningpackage version.The package version has been updated from
3.6.139to3.6.141. Ensure that this new version is compatible with your project and does not introduce any breaking changes.Verification successful
Update to
Nerdbank.GitVersioningpackage version confirmed.The package version has been updated from
3.6.139to3.6.141. The references in the project files have been updated accordingly. Ensure that the new version's.propsand.targetsfiles are present, and review the release notes for any breaking changes.
- Files Updated:
System.IO.FileSystem.nfproj: Updated to reference version 3.6.141.packages.config: Updated to version 3.6.141.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility of the updated `Nerdbank.GitVersioning` package version. # Test: Check for any known issues with version 3.6.141. # Note: This would typically involve checking release notes or known issues online. echo "Please verify the release notes for Nerdbank.GitVersioning version 3.6.141 for any breaking changes."Length of output: 210
Script:
#!/bin/bash # Search for references to Nerdbank.GitVersioning in the codebase rg "Nerdbank.GitVersioning" --context 5Length of output: 5044
There was a problem hiding this comment.
There is already RemovableDriveEventArgs why should it be replaced with CardDetectChangedEventArgs? I get it that there is this new slotIndex, but that can be added to the existing class.
Moreover, card implies that it's card only opposed to removable driver that works for cards and USB MSD (any removable media).
There was a problem hiding this comment.
@AdrianSoundy I think that with these changes in StorageEventManager you are recreating the logic that already exists at the native end...
There is an API to mount and unmount that calls the appropriate driver handler.
There is already a volume list that takes care of storing pointers to the volumes, etc.
Let's discuss this please.
|
Sorry a meant to leave some notes for discussion for this PR before review. That why I hadn't requested review yet :-) When started testing the SD card stuff I found the storagemanager RemovableDeviceInsertion event was being used to indicate when a removable device had been mounted in file system although I thought it was for the card detect. The event logic for card detect was no longer working but looked like it had been calling the same event which didn't make sense. I fixed event so card detect called RemovableDeviceInsertion event then found the args for event didn't make sense. It was passing a drive path for I: drive for a card which wasn't mounted yet. I hope this makes sense. So that's why I decided to create a new event for card detect with new args and leave the mount & unmount events alone. Probably the RemovableDeviceInsertion event is the wrong name as it is being used for when a device is mounted. Should we rename it ? And call the Card Detect event that name ? The card detect event I routed to the relevant sdcard object which I thought made more sense. Everything is working now but I do think maybe some renaming will be in order. What do you think. |
|
The storage list is for mounted drives. With the card detect it isn't mounted yet so could not use that for lookup. |
|
Unit tests failing because references wrong firmware checksum for filesystem. Won't be fixed until interpreter updated. |
|
|
Need to merge this now as matching nanoclr already merged. |
Description
This PR fixes a number of issues related to the SDCARD class in System.IO.Filesystem
This assembly requires updated NanoClr
Also updated Hardware.ESP32
Motivation and Context
How Has This Been Tested?
Tested locally with updated mount sample
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Nerdbank.GitVersioningpackage to a newer version, which may include improved performance, bug fixes, or new functionalities for users.Bug Fixes