Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #641 "Change SD Card" #4444

Closed
wants to merge 4 commits into from
Closed

Fixes #641 "Change SD Card" #4444

wants to merge 4 commits into from

Conversation

sirzeta
Copy link

@sirzeta sirzeta commented Feb 27, 2018

Added code from PR 38 from arduino-libraries/SD to fix SD card re-initialization.

@sirzeta sirzeta mentioned this pull request Feb 27, 2018
@devyte
Copy link
Collaborator

devyte commented Feb 28, 2018

Are you aware of pr #3654 ? I've been wondering whether closing root was enough.

@sirzeta
Copy link
Author

sirzeta commented Feb 28, 2018

Indeed, there must be a better way, at least with this fix I can call the begin function and able to read
again if I reconnect the SD memory, the PR 3654 touches the SPI connection, which I may not want if I have a configuration with multiple slaves. Anyway, we need to initialize a SD again, just a "end()" method would be nice or gain access to "card" and "root" somehow to know the SD card status and manually control it.

We need call SdFile::close() only if Sd2Card has no errors because SdFile::sync() is called.
@sirzeta
Copy link
Author

sirzeta commented Mar 1, 2018

I did other solution, I think it's a better approach withouth touch many files, now I can close the SD connection with some checks before close than just using root.close() (SdFile::close()) alone. SdFile::close() calls SdFile::sync() and makes some operations, not needed if not SD is present or Sd2Card has errors.

As stated in code: The sync() call causes all modified data and directory fields to be written to the storage device.

Calling constructors of Sd2Card, SdVolume, SdFile an set all related SD private variables, I think it's the safe way to initialize again by calling it's init() functions.

I tried this code disconnecting/connecting SD Card, shut down and power up again the ESP8266, connecting SD, disconnecting again and modifying some file in PC and plug back again, and many more combinations, works OK so far.

There no way to check if the connected SD is the same, as author said in some post in arduino.cc, there is a way if the SD module has a additional pin that toggles it's state if SD is connected (by mechanical function), but this it's not related with SPI connection itself.

@devyte
Copy link
Collaborator

devyte commented Mar 1, 2018

Can you show a small example of the solution?

@sirzeta
Copy link
Author

sirzeta commented Mar 1, 2018

Steps:

  1. Send m to mount
  2. Send u and y to unmount
  3. Send m to mount again, if SD.end() is commented, you are in a endless "SD initialization failed!" loop.
File myFile;
bool mounted = false;

void setup() {
    Serial.begin(115200);
}

void loop() {
    if (mounted && Serial.peek() == 'u') {
        Serial.read();
        Serial.println("Unmount SD and press 'y' to continue");
        while (Serial.peek() != 'y') {
            delay(1000);
        }
        mounted = false;
        Serial.println("SD unmounted.");
        Serial.read();
    } else if (!mounted && Serial.peek() == 'm') {
        Serial.read();

        //Uncomment this to finish while (!SD.begin(D2)) endless loop
        //SD.end();

        while (!SD.begin(D2)) {
            Serial.println("SD initialization failed!");
            delay(1000);
        }
        mounted = true;
        Serial.println("Initialization succeded!");
    }

    if (mounted) {
        myFile = SD.open("test.txt");
        Serial.println("Printing test.txt contents:");
        while (myFile.available()) {
            Serial.write(myFile.read());
        }
        myFile.close();
        Serial.println();
        Serial.println("All contents of test.txt printed!");

        Serial.println("SD mounted, press 'u' to unmount");
    } else {
        Serial.println("SD unmounted, press 'm' to mount");

    }

    Serial.println("Wait 3 seconds...");
    delay(3000);
}

@d-a-v d-a-v requested a review from devyte October 12, 2018 13:01
@d-a-v d-a-v removed the request for review from devyte October 12, 2018 13:02
@d-a-v d-a-v added this to the 2.5.0 milestone Oct 12, 2018
@devyte
Copy link
Collaborator

devyte commented Nov 13, 2018

@sirzeta Thanks for this, and sorry I took so long to get to it.
You're right, in PR #3654 , the SPI is blindly shut down, which is bad if you have multiple slaves.
However, I don't like the fact that begin() inits the SPI, but end() doesn't shut it down, and this is the case that beginners will hit.
To cover both cases, how about a flag passed to end() that specifies whether SPI should be shut down or not? Set it to true as default to bring SPI down, which covers the use case for beginners. Advanced users with multiple slaves can call end(false) to keep the SPI up.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that an additional change should be added: end() should be called at the beginning of begin(), which would allow calling begin() repeatedly.
Also, as I mentioned, end() should likely be changed to end(bool endSPI = true), where endSPI is a flag that specifies whether the SPI bus should be shut down as well.

}

root = SdFile();
volume = SdVolume();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be incompatible with #4204 .

@@ -350,6 +350,17 @@ boolean SDClass::begin(uint8_t csPin, uint32_t speed) {
root.openRoot(volume);
}

// Ends SD card connection
void SDClass::end() {
if(card.errorCode() == 0 && root.isOpen()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can root be open but card have an errorCode? What happens if root.close() is called regardless of the errorCode? i.e.:

if(root.isOpen())
  root.close();

@devyte
Copy link
Collaborator

devyte commented Nov 30, 2018

@sirzeta Are you available to continue work on this?

@sirzeta
Copy link
Author

sirzeta commented Nov 30, 2018

@sirzeta Are you available to continue work on this?

@devyte I'm little busy this days, next week I'll start working on it again.

@devyte
Copy link
Collaborator

devyte commented Nov 30, 2018

Ok. I've investigated a bit, and I think I understand what you were getting at about sync().
In the following abnormal scenario:
//Insert SD Card A
SD.begin()
//do operations
//remove card A
//insert SD card B
SD.end()

I think that it's possible that:

  • card A becomes corrupted due to removal before calling SD.end().
  • card B becomes corrupted if there were ops pending for card A at the time SD.end() is called with card B.

However, not calling sync on end() can cause pending ops to never be finished, which is bad for normal operation. I therefore think that calling sync() is necessary.

For now, I'm going to push a compromise implementation between this and #3654 , which also allows fixing #4204 without danger. It is not safe in the card swapping scenario described above, but it gets us one step forward. We can figure out the sync thing later.

@devyte
Copy link
Collaborator

devyte commented Nov 30, 2018

Closing in favor of #5402 .

@devyte devyte closed this Nov 30, 2018
@devyte devyte mentioned this pull request Nov 30, 2018
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants