Mount issues for a Windows-formatted card

greiman / SdFat

Arduino FAT16/FAT32 exFAT Library

MIT License

1.09k stars 512 forks source link

Mount issues for a Windows-formatted card #475

Open dejwk opened 9 months ago

dejwk commented 9 months ago

Hi Greiman,

I have a FAT32 SDHC card which works fine on ESP32 via SPI with the standard SD library, and also works fine on Windows. (I can read and write files, both on the microcontroller and in Windows). But I can't mount it using SdFat. This is the output from SdInfo:

Card type: SDHC sdSpecVer: 3.00 HighSpeedMode: true

Manufacturer ID: 0X6F OEM ID: ␃␃ Product: CBADS Revision: 1.0 Serial number: 0XAA000B19 Manufacturing date: 8/2022

cardSize: 15942.03 MB (MB = 1,000,000 bytes) flashEraseSize: 128 blocks eraseSingleBlock: true dataAfterErase: ones

OCR: 0XC0FF8080

SD Partition Table part,boot,bgnCHS[3],type,endCHS[3],start,length 1,0X73,0X73,0X20,0X61,0X6E,0X79,0X20,0X6B,1948285285,1701978223 2,0X73,0X74,0X61,0X72,0X74,0XD,0XA,0X0,0,0 3,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0,0 4,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0X0,28049408,441

MBR not valid, assuming Super Floppy format.

volumeBegin failed. Is the card formatted?

(Apparently, Windows 11 formats the card creating two partition entries?)

To make sure, I backed up the contents, re-formatted the card (on Windows 11), and copied the contents over again. It did not help.

Do you have any insights on what might be wrong?

I was looking format to using SdFat, particularly because of the rename functionality...

greiman commented 9 months ago

Window may have formatted it GPT. SdFat only accepts cards that comply with the SD Association Standard.

You can try the SD Standard Formatter or this example.

I do this since PC, MAC and Linux formatters do not produce formats with optimum performance.

Modern SD card have complex flash structure, RAM buffering and and algorithms.

dejwk commented 9 months ago

Thanks for a quick answer.

I have other SD cards that SdFat reads with no issues, and I am sure I can fix the problem the way you described - i.e. by formatting the card differently.

But what I am actually building is a library, to be used by other folks with their SD cards that I don't control.

SdFat works much faster for me than the native ESP32 implementation when browsing over a large filesystem (thousands of files) - even in shared mode SPI. Impressive work! That's why I am quite interested in using it in my project. But my use case is to let people create their SD content on a 'big' machine (Windows being the most common, presumably) and then stick it in the microcontroller. Therefore, I do find it a bit worrying that an SD card prepared on Windows might be unreadable with SdFat. It is especially so given that Windows doesn't appear to allow to choose the partition table type when formatting the card. (I suspect this card might have been formatted to GPT at some point to make it bootable, and then Windows keeps it when re-formatting).

All that considered, and assuming that it is indeed a partition table type issue, would you consider a feature request to support it in SdFat for better compatibility with the Windows/Linux world?

I would be happy to run any extra diagnostic or even ship the card out to you if it helped to scope the work. Also, assuming that the feature is reasonably small scoped (i.e. doesn't affect the filesystem code), I might be able to help with the implementation, if you were willing to give pointers and some support.

dejwk commented 9 months ago

Maybe, at the minimum, SdFat could recognize GPT and return a more specific error message when failing to mount?

greiman commented 9 months ago

I don't plan to support non standard formats. If you need that use the ESP SD library.

There are now dozens of Board Support packages like ESP and I can't provide comparability with each of them.

These are hundreds of Arduino compatible boards so I can't fix poorly written SPI drivers.

SdFat preforms well because it is designed to match the internal format and structure of SD cards. This will become more critical as SD cards evolve and have more sophisticated flash structures.

It is Windows that should format media according to standards. For example, cards now have huge flash pages that must be aligned on cluster boundaries for performance.

The SD standard specifies the size and location of all file structures. That is why the SD formatter has no options.

dejwk commented 9 months ago

Actually, this does NOT seem to be a GPT issue. Windows says this is MBR, basic partition, FAT32 w/ 8KB allocation unit size. Let me know if it changes anything.

I agree with your points on what Windows should be doing. Except that it doesn't. Taking a principled stance on this is yours choice to make, but the practical consequence of it is more pain and surprise for your users, and narrower applicability of your library.

Hypothetically, say I wanted to build and sell a basic microcontroller-driven picture frame (not what I am doing, but a perfectly fine and simple application). If it doesn't work with Windows-formatted SD cards out of the box, it is DOA. It is just not practical to require people to download additional software to reformat their cards.

Anyway, I respect your decision. Let me know whether the new info about the partition format changes anything here.

greiman commented 9 months ago

I have no idea what block zero is.

It is not an MBR.

part,boot,bgnCHS[3],type,endCHS[3],start,length 1,0X73,0X73,0X20,0X61,0X6E,0X79,0X20,0X6B,1948285285,1701978223 2,0X73,0X74,0X61,0X72,0X74,0XD,0XA,0X0,0,0

This much is ASCCII: "ss any key to start\r\n"

I would need to look at all the ways Windows, macOS, Linux, Android, ios, and other OSes format SD cards to provide sane messages.

If you look at Linux USB key or SD mount code you will see dozens of special case tests for specific USB drives and SD formats.

I posted the first Arduino SD library in 2008 and then SdFat in 2009, assuming Arduino would provide and maintain a standard SD library. SD.h still is based on a patched version of the 2009 beta that only supports short file names and FAT16/FAT32.

Any reasonable OS should have an integrated file system for SPI flash, SD cards, USB keys, SSDs, and HDs.

dejwk commented 9 months ago

Huh! ASCII junk in the partition table? :) You got me quite curious, so I did some digging. Here's the TL;DR version:

Apparently there is no partition table at all. Sector zero just contains FAT32 header of the assumed single partition.

How to detect this?

Check for the text FAT32 at offset 0x52.
If that fails, check for FAT12 or FAT16 at 0x36
If that fails too, assume the first sector is MBR.

Details:

Source: https://stackoverflow.com/questions/38004064/is-it-possible-that-small-sd-cards-are-formatted-without-an-mbr

Some more analysis of the FAT partition data in partition zero, created by Windows (not sure how standard that is?): https://flylib.com/books/en/2.48.1/boot_sector.html

Sector zero of my card:

Offset(h)  00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F

000000000  EB 58 90 4D 53 44 4F 53 35 2E 30 00 02 10 58 09  ëX.MSDOS5.0...X.
000000010  02 00 00 00 00 F8 00 00 3F 00 FF 00 00 00 00 00  .....ř..?.˙.....
000000020  00 1C DB 01 54 3B 00 00 00 00 00 00 02 00 00 00  ..Ű.T;..........
000000030  01 00 06 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000000040  80 00 29 48 29 09 06 4E 4F 20 4E 41 4D 45 20 20  €.)H)..NO NAME  
000000050  20 20 46 41 54 33 32 20 20 20 33 C9 8E D1 BC F4    FAT32   3ÉŽŃĽô
000000060  7B 8E C1 8E D9 BD 00 7C 88 56 40 88 4E 02 8A 56  {ŽÁŽŮ˝.|.V@.N.ŠV
000000070  40 B4 41 BB AA 55 CD 13 72 10 81 FB 55 AA 75 0A  @´A»ŞUÍ.r..űUŞu.
000000080  F6 C1 01 74 05 FE 46 02 EB 2D 8A 56 40 B4 08 CD  öÁ.t.ţF.ë-ŠV@´.Í
000000090  13 73 05 B9 FF FF 8A F1 66 0F B6 C6 40 66 0F B6  .s.ą˙˙Šńf.¶Ć@f.¶
0000000A0  D1 80 E2 3F F7 E2 86 CD C0 ED 06 41 66 0F B7 C9  Ń€â?÷â†ÍŔí.Af.·É
0000000B0  66 F7 E1 66 89 46 F8 83 7E 16 00 75 39 83 7E 2A  f÷áf‰Fř.~..u9.~*
0000000C0  00 77 33 66 8B 46 1C 66 83 C0 0C BB 00 80 B9 01  .w3f‹F.f.Ŕ.».€ą.
0000000D0  00 E8 2C 00 E9 A8 03 A1 F8 7D 80 C4 7C 8B F0 AC  .č,.é¨.ˇř}€Ä|‹đ¬
0000000E0  84 C0 74 17 3C FF 74 09 B4 0E BB 07 00 CD 10 EB  „Ŕt.<˙t.´.»..Í.ë
0000000F0  EE A1 FA 7D EB E4 A1 7D 80 EB DF 98 CD 16 CD 19  îˇú}ëäˇ}€ëß.Í.Í.
000000100  66 60 80 7E 02 00 0F 84 20 00 66 6A 00 66 50 06  f`€~...„ .fj.fP.
000000110  53 66 68 10 00 01 00 B4 42 8A 56 40 8B F4 CD 13  Sfh....´BŠV@‹ôÍ.
000000120  66 58 66 58 66 58 66 58 EB 33 66 3B 46 F8 72 03  fXfXfXfXë3f;Fřr.
000000130  F9 EB 2A 66 33 D2 66 0F B7 4E 18 66 F7 F1 FE C2  ůë*f3Ňf.·N.f÷ńţÂ
000000140  8A CA 66 8B D0 66 C1 EA 10 F7 76 1A 86 D6 8A 56  ŠĘf‹ĐfÁę.÷v.†ÖŠV
000000150  40 8A E8 C0 E4 06 0A CC B8 01 02 CD 13 66 61 0F  @ŠčŔä..Ě¸..Í.fa.
000000160  82 74 FF 81 C3 00 02 66 40 49 75 94 C3 42 4F 4F  ‚t˙.Ă..f@Iu”ĂBOO
000000170  54 4D 47 52 20 20 20 20 00 00 00 00 00 00 00 00  TMGR    ........
000000180  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000000190  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
0000001A0  00 00 00 00 00 00 00 00 00 00 00 00 0D 0A 44 69  ..............Di
0000001B0  73 6B 20 65 72 72 6F 72 FF 0D 0A 50 72 65 73 73  sk error˙..Press
0000001C0  20 61 6E 79 20 6B 65 79 20 74 6F 20 72 65 73 74   any key to rest
0000001D0  61 72 74 0D 0A 00 00 00 00 00 00 00 00 00 00 00  art.............
0000001E0  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
0000001F0  00 00 00 00 00 00 00 00 AC 01 B9 01 00 00 55 AA  ........¬.ą...UŞ

What do you do with all this?

Maybe nothing. Feel free to close this request if you think it is infeasible , especially if there are other format differences besides the lacking MBR. I understand your disappointment with Arduino, and the burden to support historical variants of formats that Microsoft and others came up with over the years.

On the other hand, you have authored an SD card library for microcontrollers that might be the best in the world, and is making quite an impact. You must, and should, be proud. Why not try to take it an extra mile and enhance compatibility with real-life media out there, to increase this impact even more? I get it when you say that the Linux SD mount code is discouraging - but my guess is that Linux accumulated tons of cases that have little practical importance today. By 80/20 rule, you should be able to cover 80% of the market by doing 20% of the work, focusing on the most popular operating systems in use today. Or 90/10 if you're lucky. Windows + macOS are 90% of desktop. Windows 10 + 11 are 94% of Windows.

greiman commented 9 months ago

That format is often referred to a "Super Floppy" format. Super Floppies were removable drives with about 200-300 MB with no partition table.

SdFat has code to mount this format but SDs in this format had vanished from the market. Apparently some Chinese companies are producing these.

I have a to do to try this format as a last resort.

Try replacing the two functions here and here with the following.

//----------------------------------------------------------------------------
/** Initialize SD card and file system for SPI mode.
 *
 * \param[in] spiConfig SPI configuration.
 * \return true for success or false for failure.
 */
bool begin(SdSpiConfig spiConfig) {
  // return cardBegin(spiConfig) && Vol::begin(m_card);
  if (!cardBegin(spiConfig)) {
    return false;
  }
  return Vol::begin(m_card) || Vol::begin(m_card, true, 0);
}
//---------------------------------------------------------------------------
/** Initialize SD card and file system for SDIO mode.
 *
 * \param[in] sdioConfig SDIO configuration.
 * \return true for success or false for failure.
 */
bool begin(SdioConfig sdioConfig) {
  // return cardBegin(sdioConfig) && Vol::begin(m_card);
  if (cardBegin(sdioConfig)) {
    return false;
  }
  return Vol::begin(m_card) || Vol::begin(m_card, true, 0);
}

I don't have an SD in this format to test it.

greiman commented 9 months ago

I managed to create a SD with your format. I used the SdFormatter example to erase a SD then used Windows Disk Manager to format the SD like a floppy.

Here is the output from SdInfor:

SD Partition Table part,boot,bgnCHS[3],type,endCHS[3],start,length 1,0X73,0X73,0X20,0X61,0X6E,0X79,0X20,0X6B,1948285285,1701978223 2,0X73,0X74,0X61,0X72,0X74,0XD,0XA,0X0,0,0 3,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0,0 4,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0X0,28049408,441

MBR not valid, assuming Super Floppy format.

volumeBegin failed. Is the card formatted?

The mod above can mount this card. I need to fix SdInfo to mount a Super Floppy.

greiman commented 9 months ago

Here is a better fix. Use this for the first two functions ins SdFat.h:

 //----------------------------------------------------------------------------
  /** Initialize SD card and file system for SPI mode.
   *
   * \param[in] spiConfig SPI configuration.
   * \return true for success or false for failure.
   */
  bool begin(SdSpiConfig spiConfig) {
    return cardBegin(spiConfig) && volumeBegin();
  }
  //---------------------------------------------------------------------------
  /** Initialize SD card and file system for SDIO mode.
   *
   * \param[in] sdioConfig SDIO configuration.
   * \return true for success or false for failure.
   */
  bool begin(SdioConfig sdioConfig) {
     return cardBegin(sdioConfig) && volumeBegin();
  }

Replace this function with:

  //----------------------------------------------------------------------------
  /** Initialize file system after call to cardBegin.
   *
   * \return true for success or false for failure.
   */
  bool volumeBegin() {
    return Vol::begin(m_card) || Vol::begin(m_card, true, 0);
  }

This also fixes SdInfo.

Card type: SDHC sdSpecVer: 3.00 HighSpeedMode: true

Manufacturer ID: 0X1B OEM ID: SM Product: 00000 Revision: 1.0 Serial number: 0X390D14D2 Manufacturing date: 6/2015 CID HEX: 1B534D303030303010390D14D200F665

cardSize: 32010.93 MB (MB = 1,000,000 bytes) flashEraseSize: 128 blocks eraseSingleBlock: true dataAfterErase: ones CSD HEX: 400E00325B590000EE7F7F800A404055

OCR: 0XC0FF8000

SD Partition Table part,boot,bgnCHS[3],type,endCHS[3],start,length 1,0X73,0X73,0X20,0X61,0X6E,0X79,0X20,0X6B,1948285285,1701978223 2,0X73,0X74,0X61,0X72,0X74,0XD,0XA,0X0,0,0 3,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0,0 4,0X0,0X0,0X0,0X0,0X0,0X0,0X0,0X0,28049408,441

MBR not valid, assuming Super Floppy format.

Scanning FAT, please wait.

Volume is FAT32 sectorsPerCluster: 32 fatStartSector: 2254 dataStartSector: 32768 clusterCount: 1952768 freeClusterCount: 1952458

dejwk commented 9 months ago

Thanks for such a quick response!

I am still debugging things on my end. Things look very promising - I can now mount that card, and I see the 'benchmark' example passing. Also, I was able to successfully scan the directory structure, of ~2000 files, opening ~1000 ZIP files and reading their manifests correctly. I am still seeing some data errors in my app, but that is likely because of some latent bugs due to a slight API differences between SD vs SdFat.

greiman commented 9 months ago

I published this mod plus other fixes and mods as SdFat-beta. After about a month I will make it the release version.

dejwk commented 9 months ago

Thank you, this is great! I confirm that the fix works all the way for my card.

For what it is worth, in my tests it comes out slightly slower than a standards-formatted card, but the diff is marginal (2-3%).

(One thing I discovered while debugging was that ESP32's standard SD library keeps the underlying file descriptors shared between the copies of a file object, so this:

File f = ...; File f2 = f; f.read(...);

advances position also in f2. SdFat doesn't do that, which makes more sense to me.)

I have one last question. I have been having some trouble with O_TRUNC. Am I crazy or are writes effectively ignored when the file is open with O_TRUNC? Specifically, what I was doing is

f.open(O_WRONLY | O_CREAT | O_TRUNC); f.write(buf, size) // bunch of stuff f.close()

and the file seems to come out empty, as if truncation happened on close rather than on open. write() did not return any errors; it reported all the data as written. Is this expected behavior? (I was expecting to see the previous content overwritten by 'bunch of stuff').

greiman commented 9 months ago

I use O_TRUNC in many examples so I think it works:

C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\AvrAdcLogger\AvrAdcLogger.ino (1 hit) Line 627: if (!csvFile.open(csvName, O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\bench\bench.ino (1 hit) Line 170: if (!file.open("bench.dat", O_RDWR | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\BufferedPrint\BufferedPrint.ino (1 hit) Line 67: if (!file.open(fileName, O_RDWR | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\debug\CardBusyTest\CardBusyTest.ino (1 hit) Line 56: if (!file.open("SdBusyTest.bin", O_RDWR | O_CREAT |O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1#attic\benchSD\benchSD.ino (1 hit) Line 58: file = SD.open("Bench.dat", O_RDWR | O_TRUNC | O_CREAT); C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1#attic\fgetsRewrite\fgetsRewrite.ino (1 hit) Line 69: SdFile wrfile("fgets.txt", O_WRONLY | O_CREAT | O_TRUNC); C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1#attic\StreamParseInt\StreamParseInt.ino (1 hit) Line 28: file = SD.open("stream.txt", O_RDWR|O_CREAT|O_TRUNC); C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\fgets\fgets.ino (1 hit) Line 44: SdFile wrfile("fgets.txt", O_WRONLY | O_CREAT | O_TRUNC); C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\LowLatencyLogger\LowLatencyLogger.ino (1 hit) Line 200: if (!csvFile.open(csvName, O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\LowLatencyLoggerADXL345\LowLatencyLogger.ino (1 hit) Line 200: if (!csvFile.open(csvName, O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\LowLatencyLoggerMPU6050\LowLatencyLogger.ino (1 hit) Line 200: if (!csvFile.open(csvName, O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\PrintBenchmark\PrintBenchmark.ino (1 hit) Line 68: if (!file.open(fileName, O_RDWR | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\StdioBench\StdioBench.ino (1 hit) Line 57: if (!printFile.open("print.txt", O_RDWR | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\STM32Test\STM32Test.ino (2 hits) Line 109: if (!file1.open("test.bin", O_RDWR | O_CREAT | O_TRUNC)) { Line 125: if (!file2.open("copy.bin", O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\examplesV1\TwoCards\TwoCards.ino (2 hits) Line 104: if (!file1.open("test.bin", O_RDWR | O_CREAT | O_TRUNC)) { Line 120: if (!file2.open("copy.bin", O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\ExFatLogger\ExFatLogger.ino (1 hit) Line 283: if (!csvFile.open(csvName, O_WRONLY | O_CREAT | O_TRUNC)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\TeensyDmaAdcLogger\TeensyDmaAdcLogger.ino (1 hit) Line 128: if (!file.open("IsrLoggerTest.bin", O_CREAT | O_TRUNC | O_RDWR)) { C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\examples\TeensySdioLogger\TeensySdioLogger.ino (1 hit) Line 37: if (!file.open(LOG_FILENAME, O_RDWR | O_CREAT | O_TRUNC)) {

I quickly hacked this example as a test. It was from the first version of SD.h based on SdFat. You may need to edit it for ESP32.

/*
  SD card read/write

 This example shows how to read and write data to and from an SD card file
 The circuit:
 * SD card attached to SPI bus as follows:
 ** MOSI - pin 11
 ** MISO - pin 12
 ** CLK - pin 13

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

 This example code is in the public domain.

 */

#include <SPI.h>
//#include <SD.h>
#include "SdFat.h"
SdFat SD;

#define SD_CS_PIN SS
File myFile;

void setup() {
  // Open serial communications and wait for port to open:
  Serial.begin(9600);
  while (!Serial) {
    ; // wait for serial port to connect. Needed for native USB port only
  }
  Serial.println("type any characrter to begin");
  while(!Serial.available()) {}

  Serial.print("Initializing SD card...");

  if (!SD.begin(SD_CS_PIN)) {
    Serial.println("initialization failed!");
    return;
  }
  Serial.println("initialization done.");
  SD.ls(LS_SIZE);
  // open the file. note that only one file can be open at a time,
  // so you have to close this one before opening another.
  myFile.open("test.txt", O_WRONLY | O_CREAT |O_TRUNC);
  Serial.print("size after open: ");
  Serial.println(myFile.fileSize());
  // if the file opened okay, write to it:
  if (myFile) {
    Serial.print("Writing to test.txt...");
    myFile.println("testing 1, 2, 3.");
    // close the file:

    myFile.close();
    Serial.println("done.");
     SD.ls(LS_SIZE);
  } else {
    // if the file didn't open, print an error:
    Serial.println("error opening test.txt");
  }

  // re-open the file for reading:
  myFile = SD.open("test.txt");
  if (myFile) {
    Serial.println("test.txt:");

    // read from the file until there's nothing else in it:
    while (myFile.available()) {
      Serial.write(myFile.read());
    }
    // close the file:
    myFile.close();
  } else {
    // if the file didn't open, print an error:
    Serial.println("error opening test.txt");
  }
}

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

Here is output for the first run of the example - test.txt does not exist and it has data after close.

Initializing SD card...initialization done. size after open: 0 Writing to test.txt...done. 18 test.txt test.txt: testing 1, 2, 3.

Here is the output for the second run. test.txt exist and gets truncated to zero length but has data after close.

Initializing SD card...initialization done. 18 test.txt size after open: 0 Writing to test.txt...done. 18 test.txt test.txt: testing 1, 2, 3.

greiman commented 9 months ago

File f = ...; File f2 = f; f.read(...);

advances position also in f2. SdFat doesn't do that, which makes more sense to me.)

This could cause problems if you write f and f2. You should never have a file open with two file objects for write.

Edit You should keep one File object for an open file and use call by reference or call by pointer in functions, not call by value.

bool fcn(File &file) or fcn(File *file), not fcn(File file).

dejwk commented 9 months ago

I think I am zooming in on a problem. And it is: the copy/move for file objects isn't working properly.

Small repro is this:

void loop() { FsFile f1;

// Discard any input. clearSerialInput();

cout << F("Type any character to start\n"); while (!Serial.available()) { yield(); }

if (!sd.begin(SD_CONFIG)) { sd.initErrorHalt(&Serial); }

if (!f1.open("bench.dat", O_RDWR | O_CREAT | O_TRUNC)) { error("open failed"); }

FsFile f2 = std::move(f1); if (f1.isOpen()) { cout << "Unexpected: f1 still open after the value was moved away." << endl; }

if (f2.write(buf, BUF_SIZE) < BUF_SIZE) { sd.errorHalt("Write failed"); } if (!f2.close()) { sd.errorHalt("Close failed"); } f1 = std::move(f2); if (!f1.open("bench.dat", O_RDONLY)) { sd.errorHalt("Open for read failed"); }

cout << "File size: " << f1.size() << endl;

f1.remove(); }

(it leaves the file truncated to zero, and shows the 'unexpected' message).

But let me go back to the second issue first, though (the one about the copy constructor).

In the vanilla Ardunino SD library, and also in the ESP Arduino implementation, the File object acts as a de-facto (smart, shared) pointer to a file descriptor entry. That is, it can be freely copied around as if it was a regular pointer (or a numeric index into the FD table, as in Unix), but it is 'smart' in that it counts references and triggers close when refcount reaches zero.

In that model, this is perfectly valid, and fairly idiomatic:

void ReadHeader(File f) { f.read(...); }

void ReadPayload(File f) { f.read(...); }

void ReadFromFile(const string* path) { File f; f.open(path); ReadHeader(f); ReadPayload(f); f.close(); }

The same pattern doesn't work in SdFat. Conceptually, you seem to be thinking about the 'file' object as a file descriptor structure itself, and not just a pointer to it. I think this is a reasonable design, except for two problems:

Most of your users will likely have some prior experience with SD.h, and possibly some existing code that use the pattern above (I did). A straightforward port to SdFat leads to errors that look like data corruption, and some unhappy debugging. No error is actually reported, and the data comes out wrong because the file position isn't getting advanced.

Of course, a simple fix is to pass f to helper functions by reference rather than by value. But one needs to be aware of the problem, and of the difference, in the first place, in order to do so.

If you think of the file object as a file descriptor structure, then you need to be careful about its copy and move semantics. Arguably, you should disable the copy constructor and an assignment operator, and implement the move constructor and the move assignment operator instead (this way the objects can still be returned as results, and they can be passed as arguments by using std::move()). In both (move constructor and the assignment move), you need to update not just the target, but also the source - probably just cleaning it up to the state that a default constructor would put it in.

Either way, I think this is a bug, and I think there are two possible ways to approach it:

switch back to the 'file is a pointer to FD' model; e.g. introduce the File / FileImpl distinction so that File contains a std::shared_ptr to FileImpl, and thus it is trivially copyable and the copies have identical state.
double down on the 'file is a FD structure' concept. Add move constructors and move assignment operators to make the files properly movable. Disable copy constructors and copy assignment operators. You then no longer need to tell people to not copy files around; the compiler will. Also, the legacy code (ported from SD.h) will just fail at compile time, rather than return unexpected data.

I hope this is helpful!

greiman commented 9 months ago

SdFat was the first SD library for Arduino and the File classes have always been file descriptor structures. Too many apps have been written since 2009 to change it. The way ESP implemented File is one of many different implementations of SD.h

Arduino made a wrapper for SdFat but and File object has a SdFat file object in each SD.h file descriptor. If you open a file twice you get the same behavior with most implementations of SD.h. This is the behavior most people think is standard for SD.h.

Here is the Arduino SD.h File creator:

File::File(SdFile f, const char *n) { // wraps an underlying SdFile
  _file = (SdFile *)malloc(sizeof(SdFile));
  if (_file) {
    memcpy(_file, &f, sizeof(SdFile));
    strncpy(_name, n, 12);
    _name[12] = 0;
  }
}

ESP is based on the C FatFS library and has the form you describe. ESP can't put the file structure in File since FatFS makes it private and returns the Linux style integer from open.

Other board support packages have other versions of the File object. Teensy is based om SdFat but has yet another File object.

There is a ESP2866 package base on SdFat that has yet another implementation of File.

So change is not possible since I would break thousands of application and some board support packages.

SdFat shows the constraints imposed by a library for a 328 Arduino with 2K RAM and 32K flash. At the time I wrote SdFat Arduino C++ didn't even have a new operator. That's why the above uses malloc.

dejwk commented 9 months ago

I looked at your code more carefully, and I figured I was largely wrong about the diagnosis.

FsFile has a good-looking copy constructor and the assignment operator. So, generally, the object should be copyable, with the caveat that writing to both copies independently is asking for trouble (like mutating the same file in parallel in general would be). (By the way, is read problematic as well?)

Not having the move constructor and assignment simply means that std::move() has no effect, and the calls invoke the copy constructor and assignment op instead. So my repro is equivalent to this:

FsFile f1; if (!f1.open("bench.dat", O_RDWR | O_CREAT | O_TRUNC)) { error("open failed"); }

FsFile f2 = f1; if (f2.write(buf, BUF_SIZE) < BUF_SIZE) { sd.errorHalt("Write failed"); } if (!f2.close()) { sd.errorHalt("Write failed"); } f1 = f2; if (!f1.open("bench.dat", O_RDONLY)) { sd.errorHalt("Open for read failed"); }

cout << "File size: " << f1.size() << endl;

f1.remove(); }

I think it should work as expected, since the state is coped forth and back. (Do you agree?) And I can't see from the code why it isn't. But it consistently results in a truncated file...

What is extremely surprising is that the last copy actually matters, even though it is done AFTER the file is closed after write()! That is, the following works correctly (file is non-zero):

FsFile f1; if (!f1.open("bench.dat", O_RDWR | O_CREAT | O_TRUNC)) { error("open failed"); }

FsFile f2 = f1; if (f2.write(buf, BUF_SIZE) < BUF_SIZE) { sd.errorHalt("Write failed"); } if (!f2.close()) { sd.errorHalt("Write failed"); } if (!f2.open("bench.dat", O_RDONLY)) { sd.errorHalt("Open for read failed"); }

cout << "File size: " << f2.size() << endl;

Also, worth noting that it doesn't report any errors (like file not opened etc.) but it simply results in an empty file, so it actually suggests something related to O_TRUNC.

In fact, removing O_TRUNC from the original repro also removes the problem!

I can't figure out what is going on, except perhaps if there is some delayed flush that gets messed up by the copies?

I suspected a problem due to the FsFile's copy constructor memcopies over the default-constructed representation objects; this is tricky, but apparently not UB:

'If the objects are not TriviallyCopyable (e.g. scalars, arrays, C-compatible structs), the behavior is undefined unless the program does not depend on the effects of the destructor of the target object (which is not run by memcpy) and the lifetime of the target object (which is ended, but not started by memcpy) is started by some other means, such as placement-new.'

(AFAICS the rep classes have trivial destructors and use placement-new, so the exclusions indeed apply).

I will continue trying to debug this.

As a side note, I still think it would be useful and safe to implement move constructors and assignment operators - they'd look very similar to the copy versions except they'd also need to set the rep pointers to nullptr. Same performance, but a more expected move behavior.

dejwk commented 9 months ago

I suspect it has something to do with directory state caching.

In the line near the end, f1 = f2 causes f1.close(). But f1 still has the original state of the file open for truncation, and probably has some dirty cache state (no longer valid, because it was superseded by changed made via f2), which gets synced by close(), overwriting what was previously written by f2.close().

That would mean that the objects aren't actually cleanly copyable...

Perhaps the assignment operator shouldn't actually call close() on the source, but simply reset the rep pointers to nullptr, discarding any unwritten state?

dejwk commented 9 months ago

I checked, and indeed, replacing close() with

m_fFile = nullptr; m_xFile = nullptr;

in FsFile.cpp:43, does fix my repro.

It seems somewhat scary not to call close() from an assignment operator, given that it is called from the destructor, but seeing that all that it does is sync(), wouldn't that be the right thing to do? Or, would that risk leaving the filesystem in an inconsistent state?

Then maybe sync() on the source, to flush the cache earlier, in the copy constructor?

dejwk commented 9 months ago

Here's a better-motivated repro (closer to what I actually ran into), without copying back-and-forth:

FsFile CreateFile() {
  FsFile f;
  if (!f.open("bench.dat", O_RDWR | O_CREAT | O_TRUNC)) {
    error("open failed");
  }
  return f;
}

void ProcessFile(FsFile file) {
  if (file.write(buf, BUF_SIZE) < BUF_SIZE) {
    sd.errorHalt("Write failed");
  }
  if (!file.close()) {
    sd.errorHalt("Write failed");
  }
}

void Test() {
  FsFile f = CreateFile();
  ProcessFile(f);
}

void loop() {
  clearSerialInput();

  cout << F("Type any character to start\n");
  while (!Serial.available()) {
    yield();
  }

  if (!sd.begin(SD_CONFIG)) {
    sd.initErrorHalt(&Serial);
  }
  Test();
  FsFile test;
  if (!test.open("bench.dat", O_RDONLY)) {
    sd.errorHalt("Open for read failed");
  }

  cout << "File size: " << test.size() << endl;
}

Interestingly, changing Test() to

void Test() {
  ProcessFile(CreateFile());
}

removes the problem, at least on ESP32 - because of copy elision. (I verified that the copy constructor is not called at all in that case). But given that copy elision is best-effort before C++17, I suspect this may also have the original issue on some other platforms.

Here's what I'd propose:

Implement move constructor and assignment. That would guarantee that Test() works when written as

void Test() {
  FsFile f = CreateFile();
  ProcessFile(std::move(f));
}

void Test() {
  ProcessFile(CreateFile());
}

Change copy constructor and move operator to sync() the source. (Seems like a potential performance regression, but I don't think it is, because this sync() would otherwise still be called from the destructor - i.e. the cost, if there is anything to sync, is already there. Even if the copy isn't changed and is short lived, there is still no regression AFAICS, since in that case sync() would still be called by the copy's destructor).
Consider allowing users to disable copy constructor and copy assignment operator via a preprocessor macro, to help migrating from versions of SD in which File is a 'pointer to the file descriptor structure'.
Consider documenting this - maybe a short 'migration guide'?

I hope this is helpful. Looking forward to hearing from you!

And once again, thank you for writing this awesome library.

greiman commented 9 months ago

I really don't want to more than one open File structure in a program.

what does this do?

#include "SdFat.h"
SdFat sd;
File file1;
File file2;
void setup() {
  Serial.begin(9600);
  if (!sd.begin()) sd.initErrorHalt();  // assumes SS is chip select
  file1 = sd.open("test.txt", O_RDWR | O_CREAT);
  if (!file1)sd.errorHalt("open");
  file2 = file1;
  file1.print("first string");
  file2.print("second string");
  file1.close();
  file2.close();
}
void loop() {
}

I was not happy when Arduino put open in the SD class and close in the File class of their wrapper. They took an early version of SdFat and the default assignment operator worked.

file = SD.open(path, oflag); is only in SdFat because of the Arduino SD.h wrapper.

greiman commented 9 months ago

You can have a file open with two file descriptors on ESP32 and have scrambled file problems.

#include "SdFat.h"
SdFat sd;
File file1;
File file2;
void setup() {
  Serial.begin(9600);
  Serial.println("Type Any");
  while(!Serial.available()){}
  if (!sd.begin(5)) sd.initErrorHalt();
  file1 = sd.open("test.txt", O_RDWR | O_CREAT);
  if (!file1)sd.errorHalt("open1");
 // file2 = file1;
   file2 = sd.open("test.txt", O_RDWR | O_CREAT);
  if (!file2)sd.errorHalt("open2");
  file1.print("first string");
  file2.print("second string");
  file1.close();
  file2.close();
  Serial.println("Done");
}
void loop() {
}

dejwk commented 9 months ago

I 100% agree that writing to the same file from two separate descriptors, either opened independently or one copied from another, can remain undefined and unsupported.

But that's not what I am raising. See in particular my last example:

void Test() {
  FsFile f = CreateFile();
  ProcessFile(f);
}

This code looks perfectly reasonable, and I hope you agree it should just work, but it doesn't.

I actually don't need or want copies of these file descriptors - I have even suggested that you allow removing the copy constructor via a preprocessor flag! What I would love is to have 'move' instead. Move will have none of these issues.

More specifically, what I am trying to do is to compose my code using some additional classes like this:

class Writer {
 public:
  Writer(FsFile file) : file_ (std::move(file)) {}

  void writeInt32(int32_t v) { ... }
  void writeMyCustomStuff(const MyCustomStuff& v) { ... }

  bool close() { return file_.close(); }

 private:
  FsFile file_;
};

And then use it like that:

FsFile file = OpenFileForWrite(...); Writer writer(std::move(file)); writer.writeStuff(); writer.close();

So really what I'd like is to move the state of the FsFile, and not have any extra copies of it. All that is needed to make this work is adding this move constructor and move assignment operator:

  FsBaseFile(FsBaseFile&& from) {
    m_fFile = nullptr;
    m_xFile = nullptr;
    if (from.m_fFile) {
      m_fFile = new (m_fileMem) FatFile;
      *m_fFile = *from.m_fFile;
    } else if (from.m_xFile) {
      m_xFile = new (m_fileMem) ExFatFile;
      *m_xFile = *from.m_xFile;
    }
    // these two lines are the important part.
    from.m_fFile = nullptr;
    from.m_xFile = nullptr;
  }

  FsBaseFile& operator=(FsBaseFile&& from) {
    // simiar to the regular operator=, with the addition as above.
    // ...
    from.m_fFile = nullptr;
    from.m_xFile = nullptr;
    return *this;
  }

Then, there is never a duplication of descriptors - the original gets reset to a pristine state, and its destructor will not call close().

With that, I would not see the need for the copy constructor at all. This change wouldn't break any existing code, and you could recommend it to your users as a cleaner and more modern alternative for passing file descriptors around, compared to relying on the copy semantics (which, as you yourself point out - leads to trouble!)

Then you also said that the copy constructor cannot be completely removed today, because it would break too much existing code. Fair enough. That means, though, that all that existing code actually does make copies of file descriptors. Presumably, in many cases these copies are in benign cases like mine above, where people aren't trying to really duplicate anything, but rather, to pass things around:

void Test() {
  FsFile f = CreateFile();
  ProcessFile(f);
}

Whoever writes a code like this might not even realize they are making a copy!

In fact, what I did was like:

void Test() {
  FsFile f = CreateFile();
  ProcessFile(std::move(f));
}

which is even more subtle, because the intent to not make a copy is clearly stated in the code (manifested by std::move(f)), but yet - since the move constructor isn't implemented, the code automatically degenerates to calling the copy constructor instead.

This all could still be fine and peachy, even without the move constructor, if the copy constructor always worked correctly. One of the scenarios that should work is this: I make a copy, I do not touch the original in any way, I keep working with the copy, and then I let them both go out of scope (causing their destructors to be called), in arbitrary order. But this scenario doesn't work correctly (specifically, when the original is 'dirty' at the time of making the copy) - because the destructor of the original is able to mess up the results made via the copy.

That is what I believe is a genuine bug.

I believe it can be fixed quite easily by calling sync() on the 'from' file in the copy constructor and the assignment operator. This way you disarm the 'deferred sync' that would be called later when the destructor of the 'from' file gets called.

Best, Dawid

greiman commented 9 months ago

I only have a vague understanding of move constructors. The cases I know about move a chunk of the heap from one object to another.

FsFile does not use the heap. Nothing in SdFat uses the heap.

Here is where the FAT or exFAT object is stored in a FsFile object

 private:
  newalign_t m_fileMem[FS_ALIGN_DIM(ExFatFile, FatFile)];  // Here is the memory used by the new operator.
  FatFile* m_fFile = nullptr;
  ExFatFile* m_xFile = nullptr;

When you open a file with open(path, oflags) this function gets called and constructs the object in the above memory.

bool FsBaseFile::open(FsVolume* vol, const char* path, oflag_t oflag) {
  if (!vol) {
    return false;
  }
  close();
  if (vol->m_fVol) {
    m_fFile = new (m_fileMem) FatFile;  //<<<<<<<<<<<<< not the heap
    if (m_fFile && m_fFile->open(vol->m_fVol, path, oflag)) {
      return true;
    }
    m_fFile = nullptr;
  } else if (vol->m_xVol) {
    m_xFile = new (m_fileMem) ExFatFile;  //<<<<<<<<<<<<< not the heap
    if (m_xFile && m_xFile->open(vol->m_xVol, path, oflag)) {
      return true;
    }
    m_xFile = nullptr;
  }
  return false;
}

What does move mean in this case? Is it copy the object and then prevent the destructor of original from doing anything by nulling the two pointers?

dejwk commented 9 months ago

TL;DR: Exactly!

Long answer:

The main point of a move constructor (and a move assignment operator) is that they are free to change, invalidate, and generally mess up, the 'source' object. The only constraint is that it must remain valid enough for the destructor to succeed.

This is the main idiom:

foo(A a) {
  // Do something with a.
}

bar() {
  A a = {...};
  foo(std::move(a));

  // Here, because we called std::move on 'a', the contents of 'a' might
  // have been invalidated or messed up. We can no longer assume anything
  // about a. Doing so is a bug.
}

The main advantage is that they allow passing objects by value (up and down the stack) without calling copy constructors and assignment operators.

Common applications indeed use it for objects that keep some stuff allocated on the heap, in order to avoid deep copies. std::string is a good example:

class X {
 public:
  X(string s) : s_(std::move(s)) {}
 private:
  std::string s_;
};

std::string s = CallSomeFnReturningStr();

X x1(std::move(s));

In this example, the heap storage for the string is only allocated once, and then passed down the stack to the instance of class X. std::move(s) reset s back to an empty string.

For FsBaseFile, the entire state is kept on the stack. Move constructor, and the assignment operator, still needs to fully copy that state, so no difference here. But a big difference is in that move constructor can 'reset' the original object to the 'not-opened' state (by clearing the pointers), effectively 'moving' the state from the original to the copy. Because of that, the destructor later called on the original (when it goes out of scope) will do nothing, because it will see null pointers and conclude that the file isn't open.

Before move constructors, similar functionality was often implemented using the 'swap' idiom:

FsFile f1 = open(...);
FsFile f2;
f2.swap(&s1);
// Now, f2 is what f1 was, and f1 is now pristine.

Move constructors, plus std::move(), are better because they are supported by the compiler, so that we can pass arguments 'by value' without actually making copies (with help of std::move()).

Early C++ had the concept of a 'copyable object' which was one that could be passed by value. Move operators introduced a new concept of a 'movable object' which can still be passed by value (with help of std::move), but it doesn't get 'replicated' in the process. Some classes in the standard C++ library are movable, but non-copyable. A good example is std::unique_ptr, whose purpose is to own an object on the heap and delete it when the smart pointer goes out of scope. Since the smart pointer is movable, you can pass it around by value (with help of std::move()), e.g. move it to some other object so that the ownership is transferred there, but there is always just one valid instance that owns the original raw pointer.

I think this is exactly what you'd want for file descriptor structures. If not for the legacy code, it sounds you might have liked the FsBaseFile class to be movable, but non-copyable.

Hope that helps!

greiman commented 9 months ago

Passing a file object by value is always a problem because the entire state of the file directory is cached in the file object. So if you read or write the file in a function, the new state is lost.

It probably was a mistake to have destructors close the file. I did it since some users didn't understand that when you write a file you must call close. It didn't help since most users follow my example and make File a global object.

The problem now is that if two copies exist, close can be called in the wrong order by the destructors. This causes the an incorrect directory entry to be written to the SD.

I do have a macro to prevent the destructor from calling close but it is only in FatFile and ExFatFile, not in FsFile.

I think I will provide a move constructor for file classes and add a macro to enable the copy constructor.

I will then post a beta with the default no copy constructor. I am tempted to make the default no close in the destructor.

If the copy constructor is enabled I will put a sync call in the copy constructor.

If that causes problems I will change the defaults.

I will experiment before making a final decision on the defaults.

greiman commented 9 months ago

I have been playing with constructors and destructors.

The move assignment operator looks great for return values.

Then maybe sync() on the source, to flush the cache earlier, in the copy constructor?

This is not possible because the source argument can't be const to call sync() but const is required for return. At least for AVR. I always start with the AVR compiler since it has the least features.

I didn't like a copy constructor that modified the object anyway.

Examples I have tried seem to work with the copy constructor and copy assignment operator set to delete.

You can't use a function like this if the copy constructor is deleted: void fcn(FsFile file) {file.close();}

This is O.K.

void fcn(FsFile& file) {file.close();}

void fcn(FsFile* file) {file->close();}

dejwk commented 9 months ago

Sounds great!

Re: constness of the source, there might be a reasonably clean way to work around this by declaring m_fileMem as 'mutable' - this tells the compiler that the member can be changed by methods of the class even if the object is const. (It is typically used for things like mutexes). It is defensible on the grounds that cache flush doesn't change the state of the object and logically makes sense even for a const object. But if you think you can just get rid of the copy constructor, I think it'd be much cleaner.

greiman commented 8 months ago

One of the main reasons I found the move constructor and assignment operator interesting was to make the copy constructor optional.

I thought I could use std::move() to replace a few assign statements in SdFat. Typically this is used in a loop to walk a path where the file opened becomes the directory in the next iteration.

On testing I found std::move() is not available in AVR board packages. AVR is the most popular board.

Notice how many AVR packages I have tested with for SdFat issues recently:

BoardS

People send me boards so I can test. I have a large bookcase with plastic shoe boxes of boards. Paul J Stoffregen of Teensy set me multiple copies of every board and most shields that he sells. Sparkfun sent me a few with unusual processors.

So std::move() is useless in libraries since it can't be used with many boards.

I can still use move assignment for return from open() for boards I tested, since AVR gcc works, but there is no include with std::move().

I am now replacing any file assignment statements in SdFat with this move function, void move(<File Type> *file);

dstFile = srcFile;

becomes

dstFile.move(&srcFile);

I need to have several macros to provide options for copy/move constructors and assignment operators and test a lot of examples on many boards before I post a beta.

dejwk commented 8 months ago

That's unfortunate.

My apologies; Espressif boards are my weapon of choice, and I have not much experience with AVR. I keep forgetting how restricted that environment is...

But here's a question: does it not support rvalue references (&&) in the language at all, or does it just not support std::move (since it generally doesn't support standard library)? If the latter, then maybe there is still hope, because the implementation of std::move is fairly trivial; it is basically a glorified cast.

UPDATE; TL;DR: this version of move() should just work:

FsFile&& move(FsFile& f) { return (FsFile&&)f; }

dejwk commented 8 months ago

Here's a good discussion if you care about details:

https://stackoverflow.com/questions/7510182/how-does-stdmove-convert-expressions-to-rvalues

But TL;DR: is that if you want to be fancy, declare two functions (for each file type):

FsFile&& move(FsFile& f) { return (FsFile&&)f; } FsFile&& move(FsFile&& f) { return (FsFile&&)f; }

the first one works for cases like

FsFile f; foo(move(f));

and the second for cases like

foo(move(CreateFile()))

although in the second case, move() is unnecessary to begin with (since the value of CreateFile() is already an rvalue (unnamed) reference, so I guess you wouldn't even really need the second variant.

Move constructor and assignment should then just work, I think:

FsFile f2 = move(f1);

etc.

greiman commented 8 months ago

I don't think I will try to provide workaround functions for board support packages that don't have std::move. I just implemented what I needed to eliminate file assignment operators in SdFat.

I have a first version of SdFat with move constructor and move assignment operator for file types. I implemented it for the three base file types and it seems to work for derived types.

I think I will make the copy constructor and copy assignment operator private by default with an option to make it public or delete. I like private since I can use the default copy constructor and assignment operator for types FatFile and ExFatFile.

I will make the move constructor and move assignment operator public with a delete option.

I am attaching a slightly tested version. You might try it to see if it fails in a big way.

The beginning of SdFatConfig.h has these rough macro definitions. The commented ones make it easy to test options.

//==============================================================================
// Added for file move constructors and file assignment operators.
// Remove commented defines before posting on github.

/** File copy constructors and copy assignment operators are deleted */
#define FILE_COPY_CONSTRUCTOR_DELETED 0
/** File copy constructors and copy assignment operators are private */
#define FILE_COPY_CONSTRUCTOR_PRIVATE 1
/** File copy constructors and copy assignment operators are public */
#define FILE_COPY_CONSTRUCTOR_PUBLIC 2

#ifndef FILE_COPY_CONSTRUCTOR_SELECT
/** Specify kind of file copy constructors and copy assignment operators */
// #define FILE_COPY_CONSTRUCTOR_SELECT FILE_COPY_CONSTRUCTOR_DELETED
#define FILE_COPY_CONSTRUCTOR_SELECT FILE_COPY_CONSTRUCTOR_PRIVATE
// #define FILE_COPY_CONSTRUCTOR_SELECT FILE_COPY_CONSTRUCTOR_PUBLIC
#endif  // FILE_COPY_CONSTRUCTOR_SELECT
//------------------------------------------------------------------------------
/** File move constructors and move assignment operators are deleted. */
#define FILE_MOVE_CONSTRUCTOR_DELETED 0
/** File move constructors and move assignment operators are public. */
#define FILE_MOVE_CONSTRUCTOR_PUBLIC 1

#ifndef FILE_MOVE_CONSTRUCTOR_SELECT
/** Specify kind of file move constructors and move assignment operators */
// #define FILE_MOVE_CONSTRUCTOR_SELECT FILE_MOVE_CONSTRUCTOR_DELETED
#define FILE_MOVE_CONSTRUCTOR_SELECT FILE_MOVE_CONSTRUCTOR_PUBLIC
#endif  // FILE_MOVE_CONSTRUCTOR_SELECT

#if FILE_MOVE_CONSTRUCTOR_SELECT != FILE_MOVE_CONSTRUCTOR_PUBLIC && \
    FILE_COPY_CONSTRUCTOR_SELECT != FILE_COPY_CONSTRUCTOR_PUBLIC
#error "No public move or copy assign operators"
#endif  // FILE_MOVE_CONSTRUCTOR_SELECT && FILE_MOVE_CONSTRUCTOR_SELECT
//------------------------------------------------------------------------------
/**
 * Set DESTRUCTOR_CLOSES_FILE nonzero to close a file in its destructor.
 *
 * Causes use of lots of heap in ARM.
 */
#ifndef DESTRUCTOR_CLOSES_FILE
#define DESTRUCTOR_CLOSES_FILE 0
#endif  // DESTRUCTOR_CLOSES_FILE

I tested it for a few things on a number of boards with this program.

#include "SdFat.h"
// edit to test with AVR and other type board.
#ifdef __AVR__
#define CS_PIN SS
#else  //  non AVR board
#include <utility>
#define CS_PIN 5
#endif // __AVR__

#define TEST_TYPE 3

#if TEST_TYPE == 0
SdFat sd;
File file;
File file1;
#elif TEST_TYPE == 1
SdFat32 sd;
File32 file;
File32 file1;
#elif TEST_TYPE == 2
SdExFat sd;
ExFile file;
ExFile file1;
#elif TEST_TYPE == 3
SdFs sd;
FsFile file;
FsFile file1;
#endif 
void fcn(File file) {file.close();}
void setup() {
// Test for no public copy constructor or assignment operator.
// file = file1;
// fcn(file);
  Serial.begin(9600);
 // while (!Serial) {}
  Serial.println("Any");
  while(!Serial.available()) {}
  if (!sd.begin(CS_PIN)) sd.initErrorHalt();
  sd.remove("test.txt");
  if(sd.exists("test.txt")) sd.errorHalt("exists");
  file1 = sd.open("test.txt", O_RDWR | O_CREAT);
  if (!file1) sd.errorHalt("file1 not open");
#ifdef __AVR__
  file.move(&file1);
#else  // __AVR__
  file = std::move(file1);
#endif  // __AVR__
  if (file1) sd.errorHalt("file1 still open");
  if (!file) sd.errorHalt("file not open");
  Serial.println("before print");
  file.println("test");
  Serial.println("before close");
  file.close();
  Serial.println("ls");
  sd.ls(LS_SIZE);
  Serial.println("Done");
}
void loop() {}

SdFatMove.zip

dejwk commented 8 months ago

Everything works on my end. I needed to remove some remaining copy calls, but that's a good thing if you ask me :)

By the way, SdFat is about twice as fast at scanning the same large filesystem (and open and look into zip files) as the vanilla ESP32 SD, for a standards-formatted card. I am really happy with the performance.

greiman commented 8 months ago

Thanks for trying the first cut.

I need to try other configurations of SdFat. Some people select features that enable 2009 code, like only support for short 8.3 file names. Also some board support packages use even older compilers than the one in the Arduino AVR package. If no C++11, I won't support it.

For ESP performance, too bad the ESP SDIO driver is so poor. I tried it and dedicated SPI was faster for most apps.

I get over 20 MB/sec read/write on Teensy 4.2 boards with a SDIO driver I wrote. The controller on the Teensy has features that match modern SD cards internal structures and algorithms. This is rare for MCUs.

I have been playing with the Raspberry Pi Pico and other RP2040 boards and get even faster than 20 MB/sec with a custom PIO SDIO controller I wrote. Amazing for a chip that costs $0.75 in quantity. The Pico is Just $4.00.