arduino-libraries / SD

SD Library for Arduino
http://arduino.cc/
GNU General Public License v3.0
179 stars 155 forks source link

SD.exists() has weird side-effects, breaks subsequent SD calls #11

Open agdl opened 8 years ago

agdl commented 8 years ago

From @blowback on January 17, 2013 13:20

If I take the listfiles.ino example and change the chip select to 4, I can get a directory listing of the contents of my SD card, as expected.

If I modify the code by adding SD.exists("foo);' immediately before the 'root = SD.open("/");' then printDirectory() no longer works - specifically, the first call to dir.openNextFile() fails.

If I move the SD.exists() line to after the SD.open("/") line, then it works again.

I tried wrapping the SD.exists() with its own SD.open().....root.close(), but that caused the directory listing to fail again.

It doesn't matter if the file tested for actually exists or not, the behaviour is the same in both cases (and SD.exists() returns the right result).

Here's the code:

/*
  SD card basic file example

 This example shows how to create and destroy an SD card file   
 The circuit:
 * SD card attached to SPI bus as follows:
 ** MOSI - pin 11
 ** MISO - pin 12
 ** CLK - pin 13
 ** CS - pin 4

 created   Nov 2010
 by David A. Mellis
 modified 9 Apr 2012
 by Tom Igoe

 This example code is in the public domain.

 */
#include <SD.h>

void printDirectory(File dir, int numTabs);

File root;

void setup()
{
   delay(5000);
  // Open serial communications and wait for port to open:
  Serial.begin(57600);
   while (!Serial) {
    ; // wait for serial port to connect. Needed for Leonardo only
  }

  Serial.print("Initializing SD card...");
  // On the Ethernet Shield, CS is pin 4. It's set as an output by default.
  // Note that even if it's not used as the CS pin, the hardware SS pin 
  // (10 on most Arduino boards, 53 on the Mega) must be left as an output 
  // or the SD library functions will not work. 
  pinMode(10, OUTPUT);

  if (!SD.begin(4)) {
    Serial.println("initialization failed!");
    return;
  }
  Serial.println("initialization done.");

  /* ADDED CODE: this SD.exists() call will cause the dir.openNextFile()
   * call in printDirectory() to fail, regardless of whether the file
   * actually exists or not.
   */
  if(SD.exists("META.DAT"))
  {
      Serial.println("EXISTS");
  }
  else
  {
      Serial.println("DOESN'T EXIST");
  }
  /* END ADDED CODE
   */

  root = SD.open("/");

  /* ADDITIONAL: if we do our SD.exists() check here, after the SD.open("/"),
   * then it all works again.
   * HOWEVER id we wrap our SD.exists() with its own SD.open() and root.close(),
   * it goes back to being broken!
   */

  if(root)
  {
      Serial.println("root opened");
      printDirectory(root, 0);
  }
  else
  {
      Serial.println("failed to open root");
  }

  Serial.println("done!");
}

void loop()
{
  // nothing happens after setup finishes.
}

void printDirectory(File dir, int numTabs) {
   while(true) {

     File entry =  dir.openNextFile();
     if (! entry) {
       // no more files
       Serial.println("openNextFile failed");
       break;
     }
     for (uint8_t i=0; i<numTabs; i++) {
       Serial.print('\t');
     }
     Serial.print(entry.name());
     if (entry.isDirectory()) {
       Serial.println("/");
       printDirectory(entry, numTabs+1);
     } else {
       // files have sizes, directories do not
       Serial.print("\t\t");
       Serial.println(entry.size(), DEC);
     }
   }
}

Copied from original issue: arduino/Arduino#1232

agdl commented 8 years ago

From @RobTillaart on January 19, 2013 19:29

Discussed on the forum here - https://forum.arduino.cc/t/sd-exists-has-weird-side-effect/139821

agdl commented 8 years ago

From @gertrude on January 20, 2013 14:44

...where the practical answer is, SD.exists() monkeys about with internal state such that subsequent openNextFile() calls fail, unless the caller also remembers to judiciously call rewindDirectory().

I suggest this violates the Principal of Least Surprise: SD.exists() should tidy up after itself, so it doesn't cause later failures in other functions. The results of openNextFile() ideally shouldn't depend on the context within which it was called.

pfabri commented 6 years ago

Has this ever been fixed? I've just run into this same problem. The only difference is that it doesn't matter if I place root = SD.open("/"); before or after my SD.exists() routine, it fails either way.

EDIT

Hmm, what I've noticed is that if I hard-code the filename, all is fine, but if I try to pass in a variable, it freaks out. So:

This works:

SD.exists("/data");

But this doesn't:

const char* DATA_DIR = "/data";
SD.exists(DATA_DIR);

And neither does this:

#define DATA_DIR "/data"
SD.exists(DATA_DIR);
PaulStoffregen commented 6 years ago

Try this fix: https://github.com/arduino/Arduino/pull/3647

pfabri commented 6 years ago

@PaulStoffregen I checked the link, but I cannot see how that problem applies here. Thanks for the quick response, though!

pfabri commented 6 years ago

Solution

Weirdly enough, it's the const qualifier it cannot stomach. Despite the fact that the compiler has no problem with it. Really weird.

Anyway, this will work just fine:

char* DATA_DIR = "/data";   // NOT const char* !
SD.exists(DATA_DIR);

But will, of course, give you a warning: deprecated conversion from string constant to char*