Fix simultaneous use of SDcard and USB MSD #1523

martin-kuhn · 2019-12-02T15:50:55Z

Description

The Windows.Storage library allows file operations on SD cards and USB MSD.

Motivation and Context

With the present code, it is only possible to use either the SD card or the USB MSD but not both at the same time. To get this to work, small changes were necessary.

How Has This Been Tested?

This has been tested on a custom STM32F427 based board with an SD card slot and an USB Host port. With the FileAccess sample from the GitHub repo, which has been adjusted to use two removable devices, the functionality could be verified.

Types of changes

Improvement (non-breaking change that improves a feature, code or algorithm)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

nfbot · 2019-12-02T15:50:56Z

Hi @martin-kuhn,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

josesimoes

LGTM!
Thanks for the fix. 👍

josesimoes · 2019-12-02T16:46:55Z

@martin-kuhn have you tested this code with the following variations? 🤔

1. just the SD Card
2. just the USB MSD
3. none

martin-kuhn · 2019-12-03T08:11:06Z

Yes, I checked all of the four possibilities:

Both SD card and USB MSD
Just the SD card
Just the USB MSD
none of them
...which work as expected.

josesimoes · 2019-12-03T08:42:39Z

Are you sure that you're not getting a Null on 3. Instead of am empty collection?
Because you moved the StorageFolder instantiation to inside the for loop, which will never get there if there are no drives.

martin-kuhn · 2019-12-03T10:09:16Z

With no device inserted I get an Windows.Storage.StorageFolder array of length 0 (as you can see in the picture). If there's only one device, then the array has length 1 and for two devices it has length 2.
I had to move the instantiation into the for loop because otherwise it gets done only for the first pointer. At the end of the for loop, storageFolder (which is a pointer to the StorageFolder array) gets incremented and then we have to create an instance also for this one. Otherwise, the second drive would always be Null (as it was before).
RemovableDevices

josesimoes · 2019-12-03T10:58:51Z

...MSIS-OS/ChibiOS/nanoCLR/Windows.Storage/win_storage_native_Windows_Storage_StorageFolder.cpp

@@ -73,6 +73,7 @@ HRESULT Library_win_storage_native_Windows_Storage_StorageFolder::GetRemovableSt
    uint32_t driveCount = 0;
    char workingDrive[sizeof(DRIVE_PATH_LENGTH)];
    uint16_t driveIterator = 0;
+    bool busy = false;


Don't think this busy flag is needed. The sdCardEnumerated and usbMsdEnumerated should take care of this.
I've just tested your fix without it and it seems to be working as expected.

I tested it quickly without the busy flag. It is needed anyway. Without the busy flag, both if conditions of the SDcard and the USB MSD are true because they are both ready and not enumerated at the beginning. So they get executed both in the first iteration of the loop and then the workingDrive of the SD card gets overwritten by the workingDrive of the USB MSD. This means that in both iterations the same device (the second one, which is the USB MSD) is mounted. So you'll end up with an array which has two StorageFolders but they are exactly the same.
I would have preferred to use the enumerated flags instead of an additional busy flag, but this is not possible because if you use only one drive, the other one is not defined.

josesimoes

Waiting for your comment on the busy flag. 😃

josesimoes · 2019-12-03T18:27:22Z

Just tested the changes with all possible variations in the PR submitted to your repo and all it's working.

martin-kuhn · 2019-12-04T08:20:57Z

Thanks for your solution. I came to the same one yesterday, but I dropped it because of this:
This seems to work as long as HAL_USE_SDC is defined. You increase maxDriveCount twice for the SD-card but never for the USB MSD (by accident I guess?). In my opinion, this does not work if someone wants to use only the USB MSD. With maxDriveCount set to 1, the for loop could never reach a driveIterator with 1, so the if(driveIterator == USB_MSD_DRIVE_INDEX_NUMERIC) would never be executed. Right?

josesimoes · 2019-12-04T08:27:04Z

That shouldn't be a problem. The SD_CARD_DRIVE_INDEX_NUMERIC is set to 1 or 0 according to the defines on what's being used.
Agreed that's not obvious that's why there is that comment trying to explain how these work. It's on the target storage header file. It was previously on the C file but I moved it there so it could be used in the rest of the code (as it is now 😜)

martin-kuhn · 2019-12-04T08:30:46Z

Ok, I see. So USB_MSD_DRIVE_INDEX_NUMERIC is set to 0 when HAL_USE_SDC is not defined. Great!
Maybe we should avoid that the code starting from line 167 does not get executed if none of the if-conditions are true. What do you think? That's the case when the filesystems are not ready yet.

martin-kuhn · 2019-12-04T08:31:46Z

Ah, that's already the case. I didn't see that, sorry :)

josesimoes · 2019-12-04T08:33:24Z

I don't think that check is required. If the file systems aren't ready the drive count will be 0 and that code block is never entered.

josesimoes · 2019-12-04T08:36:17Z

Can you please squash all these so the changes being made here are clearer?

martin-kuhn · 2019-12-04T09:30:02Z

I cleaned up the code with some small changes (instantiation of StorageFolder was there twice, maxDriveCount fixed). It's tested and it works in all of the 4 possibilities.
What did you mean by "squash"?

josesimoes · 2019-12-04T09:32:11Z

What did you mean by "squash"?

https://gist.github.com/patik/b8a9dc5cd356f9f6f980

claassistantio · 2019-12-04T10:26:50Z

All committers have signed the CLA.

martin-kuhn · 2019-12-04T10:37:42Z

finished 🙂

josesimoes

LGTM!

josesimoes approved these changes Dec 2, 2019

View reviewed changes

josesimoes added Platform: STM32 Everything related specifically with ChibiOS platform Type: enhancement labels Dec 2, 2019

josesimoes added the Status: under review label Dec 2, 2019

josesimoes added Type: bug and removed Status: under review Type: enhancement labels Dec 3, 2019

josesimoes reviewed Dec 3, 2019

View reviewed changes

Add support for parallel use of SDcard and USB MSD

57b7086

josesimoes approved these changes Dec 4, 2019

View reviewed changes

josesimoes changed the title ~~Add support for parallel use of SDcard and USB MSD~~ Fix simultaneous use of SDcard and USB MSD Dec 4, 2019

josesimoes merged commit 8cda296 into nanoframework:develop Dec 4, 2019

coderabbitai bot mentioned this pull request Aug 1, 2024

Fix getting file size for NativeFileInfo #2996

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix simultaneous use of SDcard and USB MSD #1523

Fix simultaneous use of SDcard and USB MSD #1523

martin-kuhn commented Dec 2, 2019 •

edited by josesimoes

Loading

nfbot commented Dec 2, 2019

josesimoes left a comment

josesimoes commented Dec 2, 2019

martin-kuhn commented Dec 3, 2019

josesimoes commented Dec 3, 2019

martin-kuhn commented Dec 3, 2019

josesimoes Dec 3, 2019

martin-kuhn Dec 3, 2019

josesimoes left a comment

josesimoes commented Dec 3, 2019

martin-kuhn commented Dec 4, 2019 •

edited

Loading

josesimoes commented Dec 4, 2019 •

edited

Loading

martin-kuhn commented Dec 4, 2019

martin-kuhn commented Dec 4, 2019

josesimoes commented Dec 4, 2019

josesimoes commented Dec 4, 2019

martin-kuhn commented Dec 4, 2019

josesimoes commented Dec 4, 2019 •

edited

Loading

claassistantio commented Dec 4, 2019 •

edited

Loading

martin-kuhn commented Dec 4, 2019

josesimoes left a comment

Fix simultaneous use of SDcard and USB MSD #1523

Fix simultaneous use of SDcard and USB MSD #1523

Conversation

martin-kuhn commented Dec 2, 2019 • edited by josesimoes Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

nfbot commented Dec 2, 2019

josesimoes left a comment

Choose a reason for hiding this comment

josesimoes commented Dec 2, 2019

martin-kuhn commented Dec 3, 2019

josesimoes commented Dec 3, 2019

martin-kuhn commented Dec 3, 2019

josesimoes Dec 3, 2019

Choose a reason for hiding this comment

martin-kuhn Dec 3, 2019

Choose a reason for hiding this comment

josesimoes left a comment

Choose a reason for hiding this comment

josesimoes commented Dec 3, 2019

martin-kuhn commented Dec 4, 2019 • edited Loading

josesimoes commented Dec 4, 2019 • edited Loading

martin-kuhn commented Dec 4, 2019

martin-kuhn commented Dec 4, 2019

josesimoes commented Dec 4, 2019

josesimoes commented Dec 4, 2019

martin-kuhn commented Dec 4, 2019

josesimoes commented Dec 4, 2019 • edited Loading

claassistantio commented Dec 4, 2019 • edited Loading

martin-kuhn commented Dec 4, 2019

josesimoes left a comment

Choose a reason for hiding this comment

martin-kuhn commented Dec 2, 2019 •

edited by josesimoes

Loading

martin-kuhn commented Dec 4, 2019 •

edited

Loading

josesimoes commented Dec 4, 2019 •

edited

Loading

josesimoes commented Dec 4, 2019 •

edited

Loading

claassistantio commented Dec 4, 2019 •

edited

Loading