documentation of file.rewindDirectory() is ambiguous and confusing

[dsyleixa]

documentation https://www.arduino.cc/en/Reference/FileRewindDirectory of file.rewindDirectory() is ambiguous and confusing:
it actually make not undoubtedly clear for what ist's worth:

original example sketch:

#include <SD.h>
File root;

void setup()
{
  Serial.begin(9600);
  pinMode(10, OUTPUT);

  SD.begin(10);
  root = SD.open("/");
  printDirectory(root, 0);
  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
       // return to the first file in the directory
       dir.rewindDirectory();
       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);
     }
   }
}

OTOH, I can do fine WITHOUT .rewindDirectory() and if I use it and put it in though, it all messes up my directory file listing, multiple filenames are read and listed repeatedly:

String filelist[128];
// edit, meanwhile I use std::vector<String> filelist instead + push()
File SdPath;

volatile int filecount = 0;
//=================================================================
int  readDirectory(File dir, int dirLevel) {   

   while (true) {
      File entry =  dir.openNextFile();
      if (! entry) {
         // no more files
		 // dir.rewindDirectory(); //   don't do it! <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
         break;
      }

	 if(filecount==0) {       // <<<<<<  NEW
         filelist[0]="/";
		 filecount++;
      }

      //Serial.print(entry.name());
      filelist[filecount] = (String)entry.name();


      if (entry.isDirectory()) {
         //Serial.println("/");
         filelist[filecount] += (String)"/"; 
         //Serial.println(filelist[filecount]);
         filecount++;
         // <<< no more "/.."		
         readDirectory(entry, dirLevel + 1);
      } else {
         // files have sizes, directories do not
         //Serial.println(filelist[filecount]);
         //Serial.print("\t\t");
         //Serial.println(entry.size(), DEC);   
         filecount++;
      }
      entry.close();
   }
   return filecount;
}

[PaulStoffregen]

Indeed the documentation could be improved. Here are a few specific recommendations.

1: Change "Syntax" from "file.rewindDirectory()" to "dir.rewindDirectory()"

2: Change "Parameters" from "file: an instance of the File class." to "dir: an instance of the File class, which references a directory."

3: Change the example code to this:

#include <SD.h>

void setup() {
  Serial.begin(9600);
  Serial.print("Initializing SD card...");
  if (!SD.begin(10)) {
    Serial.println("unable to access SD card");
    return;
  }
  Serial.println("initialization done.");

  File root = SD.open("/");
  printDirectory(root, 0);
  Serial.println();

  Serial.println("PRINT AGAIN");
  Serial.println("-----------");
  root.rewindDirectory();  // go back to the first file
  printDirectory(root, 0);
  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 in this directory
      return;
    }
    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);
    }
  }
}

[dsyleixa]

thanks, but tbh, I still don't understand a single word what that function is about.
My program works without any rewindDirectory,
so why should any name or path or whatever be renamed,
why would I need extra 1, 2 or 3 paths (dir, file, root, entry),
and why should that function be needed anyway?
That all is enterely confusing!
Finally my own program example works better without either rewind, so what would it improve?
Did you get my own program working?

[dsyleixa]

OTOH, what would be actually a real improvement:

• pass just the name of the start dir to the printDirectory function, no dirlevel at all
• show only the primary entries of the (current start) dir in an alphabetical order (!) (first sub dir names, then file names)
• optionally, when passing an extra flag, then all entries should be listed completely, recursively (see: ls / ls -R)
• be able to select/pass a subdir of the current dir, cd, and then show up the entries of that subdir (dirlevel+1)
• be able to shift (chdir dirname) to either sub-sub-dir in the same way
• be able to shift back to the super-dir by passing either the actual pathname (absolute: chdir dirname) or by passing ".." ( chdir .. ), dirlevel relatively -1)
• like in my example, besides immediate print-out, it must be possible to store all intermediate dir entries in a temp String array (e.g.,for GUI TFT re-arrangements and accessing files/dirs also by their array position, to build sort of a file commander, like I am already doing here (admittedly quite clumsy) : https://github.com/dsyleixa/Arduino/tree/master/ESP32_GBox/ESP32_Box029, line 2019)

[PaulStoffregen]

thanks, but tbh, I still don't understand a single word what that function is about.

Maybe you could try running the example code I suggested? It is a complete program you can just copy into Arduino and upload to any board with a SD card (rather than just a function which requires more work to fill in missing code before uploading to a real board). When you run it on your Arduino hardware, you'll see it prints the recursive directory twice. Try deleting the use of root.rewindDirectory(); and observe the result.

My purpose here is not to provide direct technical support, but rather only to assist Arduino to improve their SD library documentation. To work towards that goal, maybe you could run my proposed example program and see if it help you to understand rewindDirectory()?

[PaulStoffregen]

And just in case it's not clear, I do not work for Arduino, nor am I directly affiliated with Arduino. In this github issue, I am merely a 3rd party contributor who wishes to help improve the documentation so that everyone may benefit.

[dsyleixa]

thank you, of course I appreciate your contributions, but the crucial point is:
even the original Arduino example sketch runs ok, nonetheless IMO the sense of rewindDirectory is ambiguous and confusing.
At latest when I ported to my own sketch it's messing all up and so I do better without the rewind thing.
Soafter all the sense is still unclear, especially when you used it in your last example which even needs from some unclear reasons also a couple of extra instances which makes everything even more unclear... :rolleyes:

[PaulStoffregen]

Yes, I agree that the original example program, which still appears on the rewindDirectory() documentation page, is flawed. It uses rewindDirectory() in a useless way, which of course doesn't really serve as an effective example.

From everything you've just said, I'm getting the impression you did not in fact run the code I proposed to use as the rewindDirectory() example. Maybe I'm reading too much into your comment, but words like "is still unclear, especially when you used it in your last example which even needs from some unclear reasons also a couple of extra instances which makes everything even more unclear" seem to be only about try to read the code, rather than about the experience of having actually run the code and seen the results.

I'm also getting the impression you believe rewindDirectory() is fatally flawed (even though you've repeatedly said you don't understand it) and you wish to talk about redesigning the public API which has been stable for over 10 years for of the most widely used Arduino libraries. Or perhaps you wish to discuss design of your own application? But you began this issue stating that the documentation "is ambiguous and confusing". The Arduino team has already tagged this issue "Component: Documentation". I believe the most good which can be done within the scope of this particular github issue is to focus on improving that particular documentation page, especially its example code. The forum is a much better place to discuss how to design your own application. And just to be realistic, there is no good place to talk of changing the public API on the SD library in any way that isn't backwards compatible. While I can't speak officially for Arduino, I believe it's pretty safe to assume that redesigning the public API on such a widely used library isn't going to happen.

If your position is not already so forever hardened against learning how rewindDirectory() works, if you're still capable of having an open mind in this effort, please would you just copy the example program I suggested into Arduino and run it on your hardware? Is that too much to ask? It is a complete program, ready to use. I personally tested it on Arduino Uno and Teensy 3.2 before posting that message. And then after you've run it once and see the directory print twice in the serial monitor, delete the 1 line with rewindDirectory() and run it again, so you can see the change. Will you at least do that? Please?!

The question is simple, and limited in scope. Would that proposed example code improve the Arduino documentation?

[PaulStoffregen]

@kengdahl - Maybe the documentation page could also explain why rewindDirectory() can be useful? Today it says:

rewindDirectory() will bring you back to the first file in the directory, used in conjunction with openNextFile().

Perhaps the description could be longer, such as:

rewindDirectory() will bring you back to the first file in the directory, used in conjunction with openNextFile(). When you need to read the directory again, rewindDirectory() may be easier than closing the File instance and then having to open it again, especially with subdirectories where the work required to open the directory again involves traversal of its parent directories.

[dsyleixa]

as to
"rewindDirectory() will bring you back to the first file in the directory, used in conjunction with openNextFile()" :
I do understand it actually. But I do not understand why it is used in the original example sketch.
Paul seemes to agree to that saying
" It uses rewindDirectory() in a useless way, which of course doesn't really serve as an effective example.".
By that I now see that it's not a matter of the methods but of the weird example which has to be improved in the documentation!
I also admit that I did not try Paul's example because
a) it uses far too many extra instances, and
b) I still do not understand why one would need either rewindDirectory at all because I can do without such so far:
just start at root and then go recursively through all the files and folders, no rewindDirectory needed ever, and when it starts reading the files in either (sub)dir then it is "rewinded" automatically for the first read (CMIIW).
In addition, it now sounds so that it would be absolutely necessary if a file is open, so that openNextFile apparently cannot detect it (CMIIW).

Having said that, so it's supposed to be ok and useful if one wants to read dir contents multiple times, and then I understand the sense and the use of of rewindDirectory -
but not in the examples already given in the documentation or in Paul's (surely sophisticated but IMO quite complicated) example.

Anyway, IMO there are still some crucial things unclear which should be reworked to make it more clear and understandable in the documentation (especially by suitable code examples), when it's indispendible and when not..

[PaulStoffregen]

The original (defective) example on the rewindDirectory() page has 34 lines which are not blank or comments.

My proposed replacement example code has 39 lines with code. I do not see how it "uses far too many extra instances" or is "quite complicated". It's really just a minor modification of the original example, to actually use rewindDirectory() in a way which make sense and has an easily observable effect.

@dsyleixa - Perhaps you could try proposing a replacement for the rewindDirectory() example, rather than just a code fragment (which also has a buffer overflow error for any SD card with more than 128 files).

[PaulStoffregen]

Only 19 hours ago, you wrote "I do understand it actually".

I agree the rewindDirectory() documentation is deficient and needs to be improved. That is why I have made specific proposals to improve the example code and English text on that documentation page. I has hoped you might seriously evaluate my proposal, or even better, improve upon it or craft one of your own. But now, it kinda feels like nothing constructive can come from continuing this conversation.

[dsyleixa]

edit, as to the overflow: yes I know, meanwhile I use std::vector filelist instead + push().

In your example I do not understand why one needs 2 different Files (dir, entry) and the sense of changing "Syntax" from "file.rewindDirectory()" to "dir.rewindDirectory()" and "Parameters" from "file: an instance of the File class." to "dir: an instance of the File class, which references a directory." That is entirely confusing me, and that's independed from the rewindDirectory thing.

What I probably understand is that rewindDirectory resets the find-file-position of the already read entries back to the first entry in the folder.

OTOH, I can't propose a replacement for the rewindDirectory() example because I do not understand when and why it's needed for which purposes and when not, so it may be discarded then. That's actually what my TOP question is about.

[PaulStoffregen]

Perhaps use of "myDir" as an instance name, in the example program and documentation text, would be clearer?

The original example code named File instances with "dir" and "entry". So does your own readDirectory() code which you posted in the 1st message which opened this issue.

But perhaps nothing will ever make this clearer for you? Maybe we've just gone too far down this path and nothing can help, for you individually. However, my hope is still that the Arduino team might improve the documentation page, so others can be helped more in the future.

[dsyleixa]

yes, first there should be a clear explanation, when and why rewindDirectory is needed for which purposes and when it's not, so it may be discarded then.
And then a couple of examples for when is it indispensably needed - and when it would be needed NOT.
(as stated, in my example above it's obviously NOT needed.)
(PS, IIUC, one would only need the rewind thing from outside before calling the printDirectory, not from inside the printDirectory like the original code did (CMIIW) ).

[kengdahl]

Hello!
You can make pull request to the SD library. Here is a link to entry rendered on the reference:
https://github.com/arduino-libraries/SD/blob/master/docs/api.md#rewindDirectory

[dsyleixa]

why is there still a rewindDirectory in the printFile function? IMO it's still a completely flawed example there.

void printDirectory(File dir, int numTabs) {
   while(true) {
     File entry =  dir.openNextFile();
     if (! entry) {
       // No more files
       // Return to the first file in the directory
       dir.rewindDirectory();
       break;
     //...

[kengdahl]

@dsyleixa Please feel free to make a PR with a fix :)

[dsyleixa]

oh, I never have done sth like that and have no clue how that works.
And finally I don't how to to fix it at all (that's actually my own issue TOP question) , so it has to be left to be done by the devs 8-)

[per1234]

Thanks for bringing this deficiency to our attention @dsyleixa and thanks for your valuable suggestions @PaulStoffregen.

I extracted what appeared to me to be the highest priority atomic subset of the code changes suggested at #11734 (comment) and submitted them here: arduino-libraries/SD#118

Reviews on that PR are welcome from anyone.