espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.45k stars 7.38k forks source link

SD.exists("/") works faulty #3603

Closed dsyleixa closed 2 years ago

dsyleixa commented 4 years ago

edit: the following issue is caused by dedicated ESP SD libs, not by the original Arduino libs (just tested):

SD.exists("/") works faulty! If the SD has been started correctly by SD.begin(SD_CS) and the SD should be checked if intermediately mistakenly removed, it always shows "OK" and never shows an error:

 #include <SD.h>

void loop() {
   //...
   if( SD.exists("/") ) {
      Serial.println(" SD path '/' OK!  ");  // either if SD inserted or removed !!!
      // debug
      delay(2000);
   }
   else {
      Serial.println( "error! no SD file path found!  "); 
      // debug
      delay(2000);
   }
   //...
}

Arduino IDE 1.8.9 ESP32 board 1.0.4 default settings

PS: the SD needs to be checked by the root path SD.exists("/") because the current files on the SD are unknown, So finally the root path is the only path which is existing on either formatted SD. OTOH, SD.begin() also always returns no error if the SD is mistakenly not present any more. Additionally, if no SD was inserted at the start (SD.begin() fails at setup() ), then a later re-inserting during runtime plus an extra SD.begin() also does not restart and reload the SD contents

dsyleixa commented 4 years ago

PS, update, just checked: on an original Arduino board (Arduino Due) this fault is not happening, the code above works fine as expected. Appearently the Arduino SD libs are fine. So the fauly behaviour must be due to ESP SD libs.

dsyleixa commented 4 years ago

@rel1ct: what do you mean by your effing emojies?

lbernstone commented 4 years ago

Your expectations really could use some work. This is a community project- nobody here works for you. If you think the behavior is wrong, work to fix it. Saying "this is broken" is lazy and boorish. From a posix standpoint, checking whether a mount point exists should always return true. Instead of using the bad workaround that some Duo example used, think about a better way to do this. Checking the size of the filesystem should be more portable and reliable.

dsyleixa commented 4 years ago

I have no idea about driver code or technical failures, I am a simple Arduino user, a hobby programmer (like almost all common arduino users, for whom Arduino/Wiring had been developed), I have no CS, no technician or programming education, learning just from example sketches. I reported the bug in the hope that it will be of interest and that it can be fixed by pros and/or experts. As stated, the original Arduino API functions for SD on an Arduino Due (same SD used for testing) works well though. Happy new year!

rel1ct commented 4 years ago

Where did you read that the libraries and functionality for ESP32 should be completely identical to Arduino Due or ESP8266?

dsyleixa commented 4 years ago

the way how it's currently working for an "original" Arduino is fine, and the way how it's currently working for ESP32 is faulty - that's the crucial point which I had reported above.

rel1ct commented 4 years ago

What is your understanding of the "original" Arduino ? Where did you read that the libraries and functionality for ESP32 should be completely identical to Arduino Due or ESP8266 ?

lbernstone commented 4 years ago

@dsyleixa : I will need to get my sd card rigged up so I can fix this. Expect a PR soon.

dsyleixa commented 4 years ago

will there be a fix of the broken SD lib functions SD.exists("/") SD.begin() to actually "really" check or restart anew?

dsyleixa commented 4 years ago

will there soon be some fixes available?

stale[bot] commented 4 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dsyleixa commented 4 years ago

issue is still unresolved

stale[bot] commented 4 years ago

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

stale[bot] commented 4 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dsyleixa commented 4 years ago

issue is still unresolved

stale[bot] commented 4 years ago

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

stale[bot] commented 3 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dsyleixa commented 3 years ago

issue is still unresolved

stale[bot] commented 3 years ago

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

dsyleixa commented 3 years ago

issue is still unresolved

stale[bot] commented 3 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dsyleixa commented 3 years ago

issue is still unresolved

stale[bot] commented 3 years ago

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

stale[bot] commented 3 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dsyleixa commented 3 years ago

still no report of being solved

stale[bot] commented 3 years ago

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

stale[bot] commented 3 years ago

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dsyleixa commented 3 years ago

it still is not resolved, still works faulty.

stale[bot] commented 3 years ago

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

P-R-O-C-H-Y commented 3 years ago

Hi @dsyleixa,

When using SD.exist() the file you looking for may be found in a cached directory block. The only sure way to detect removal or inserting of the SD is to use the SD detect switch (CD pin) on the SD socket.

Set pull up input on a CD pin and then just call digitalRead. If pin is LOW = card is detected, if HIGH = no card inserted

Try this:

void setup() 
{
  // Init serial, SD, etc..

  // Set pull up input on a CD pin
  pinMode(SD_CD,INPUT_PULLUP);
}

void loop() 
{

  bool SD_detection = digitalRead(SD_CD); // 1 = no card inserted; 0 = card detected

  if (!SD_detection)
  {
      Serial.println("card detected");
  }
  else
  {    
      Serial.println("card removed");
   }

  // rest of code
}
dsyleixa commented 3 years ago

thank you for your suggestion! I can't try it currently anymore because my device is defective.

Additionally, I have just these pins provided, no more than that (SD slot is soldered to an Adafruit Feather TFT shield), so IIUC I'm afraid such a SD_CD pin is not available:


// MOSI - pin 11
// MISO - pin 12
// CLK - pin 13

// *SNIP*: TFT & TS shield pin settings
// SD pin:
#define SD_CS    14

#include <SD.h>

void setup() {
  SD.begin(SD_CS);
  //...
}

OTOH, the SD-check is a functionality which is supposed to be processed by SD.exist() internally and should work correctly out of the box.

P-R-O-C-H-Y commented 3 years ago

Can you please share what exact Adafruit Feather TFT board you have? In the SD socket there is always the CD pin but some boards use it and some not.

For best performance the FileSystem is cached so even if you call SD.exist("/") or SD.open("/foo.txt",FILE_READ). It will always return true until you call SD.end().

dsyleixa commented 3 years ago

I have a 3-years old 480x320 3,5" Feather-Wing https://learn.adafruit.com/adafruit-3-5-tft-featherwing plus a 3-years old version of Adafruit ESP32 Huzzah https://www.adafruit.com/product/3405 anyway, as stated, I actually expect the ESP32 SD lib to handle that issue, perhaps by disabling internally any status caching. Just to mention that I use SD.h, not SdFat.h or any other SD lib.

P-R-O-C-H-Y commented 3 years ago

We internally use the FS in our SD-library. So every time you call SD.Exist("foo.txt") and file was existing on the card before removing or calling SD.end, it will find the file in cache. The Sd.exist should not be used to determine if SD card is removed. If you don't have the CD pin, this workaround will work for you to detect if the card is removed.

Sample code:

#include <SD.h>

bool SdExist(fs::FS &fs){
    Serial.println("Creating foo.txt file to detect the SD card");
    if(fs.open("/fooo.txt","w")) 
    {
        fs.remove("/fooo.txt");
        Serial.println("Succesfuly created and removed.");
        Serial.println("SD card detected");
        return true;
    }
    else 
    {
        Serial.println("Error in creating file");
        Serial.println("SD card not detected");
        return false;
    }
}

In loop just call the function:

if(!SdExist(SD)){
        //SD card removed
}
dsyleixa commented 3 years ago

thank you, which lib is it using? SD.h?

P-R-O-C-H-Y commented 3 years ago

Yes the SD lib. #include <SD.h>

dsyleixa commented 3 years ago

that's great, thank you very much! I have to admit that I don't understand the meaning of the passed variant SdExist(fs::FS &fs) but IIUC I can be sure that the result of file create/remove will not be stored in any cache when called several times repeatedly?

P-R-O-C-H-Y commented 3 years ago

You are passing SD (FS) class you are working with to the function SdExist(fs::FS &fs) .I assume you work with default SD class already initialized in SD.h file: extern fs::SDFS SD; But if you want to use own class instead the function is ready to get your class passed to it. Like functions in examples of SD library are made exactly like this function.

But if you use the default SD class you can change the function SdExist just to this:

bool SdExist(void){
    Serial.println("Creating foo.txt file to detect the SD card");
    if(SD.open("/fooo.txt","w")) 
    {
        SD.remove("/fooo.txt");
        Serial.println("Succesfuly created and removed.");
        Serial.println("SD card detected");
        return true;
    }
    else 
    {
        Serial.println("Error in creating file");
        Serial.println("SD card not detected");
        return false;
    }
}

Yes you can be sure with that result, if the file does not exist before calling the function. It needs to create the file. If the file is existing, it will be found in cache. Thats why there is the remove after creating the file, so next time it needs to be created again.

dsyleixa commented 3 years ago

I do/did not declare or instantiate "SD" in my program by myself explicitely (IIRC), so I guess SD is something which is provided by SD.h by default just in the moment when I include this lib by calling SD.begin(SD_CS). Again, thank you very much for your very helpful contributions!

P-R-O-C-H-Y commented 3 years ago

Yes exactly, you use the default one decleared in SD.h file. So I can consider this issue as closed ?

dsyleixa commented 3 years ago

tbh, it's just a workaround now, but IMO the issue is not solved yet, SD.exists("/") still works faulty : I expect the function SD.exists() to work reliably, always checking if SD actually exists, as it suggests and what it is supposed to do: so it must never read buffered (outdated) cache values! (And OTOH, as mentioned, the original Arduino API function works correctly on Arduino boards!)

P-R-O-C-H-Y commented 3 years ago

I have found the actual problem, will take a look to fix that with esp-idf team. I will inform you once its fixed :)

P-R-O-C-H-Y commented 2 years ago

New info. The change is finally merged in esp-idf. Once the esp-idf is updated inside arduino-esp32 I will inform you again :)

P-R-O-C-H-Y commented 2 years ago

Hi, made an PR to fix the problem for SD library. The SD_MMC already contains fix on master branch (updated esp-idf already merged).So now when you remove card from slot, the SD.exist will return false. Once merged, both SD and SD_MMC will work the same as expected :)

dsyleixa commented 2 years ago

hi, what do I have to do if I want to use the new fix?

P-R-O-C-H-Y commented 2 years ago

You can download the master branch from github and replace the library in Arduino IDE. Link: https://github.com/espressif/arduino-esp32/archive/refs/heads/master.zip

dsyleixa commented 2 years ago

ah, no, thanks, I would prefer to wait until it is available in the Arduino IDE out of the box, e.g. by updating the ESP core version (currently: ESP32 board 1.0.6 default settings)

me-no-dev commented 2 years ago

we are already on 2.0.1, so maybe you want to check what you have installed

dsyleixa commented 2 years ago

in my Arduino IDE 1.8.9 I can only choose one of 1.0.0. to 1.0.6, Board Manager URL in Arduino preferences: https://dl.espressif.com/dl/package_esp32_index.json

P-R-O-C-H-Y commented 2 years ago

There are new URLs for board manager. Refer to Installing Arduino-esp32 For stable releases use this: https://raw.githubusercontent.com/espressif/arduino-esp32/gh-pages/package_esp32_index.json For development releases use this: https://raw.githubusercontent.com/espressif/arduino-esp32/gh-pages/package_esp32_dev_index.json