SD writes corrupt the filesystem: sdWriteBytes() passes the FatFs window buffer to the in-place SPIClass::transfer(void*, size_t)

[ygCEO]

Summary

sdWriteBytes() in src/SD/Seeed_sdcard_hal.cpp sends the 512-byte data block with

card->spi->transfer((uint8_t *)buffer, 512);

On the SAMD core, that overload of SPIClass::transfer is in-place: it overwrites the caller's buffer with the bytes received from the bus. During an SD write the card returns 0xFF, so the buffer is filled with 0xFF immediately after being sent.

The buffer passed here is the FatFs window (fs->win), which FatFs still considers a valid cache of fs->winsect after disk_write returns. Every FAT and directory sector is therefore destroyed in RAM right after being written to the card. The next modification of that same sector reads garbage and writes the garbage back.

The result is progressive corruption of the volume. It shows up in two ways, both reproduced below:

1. Silent data corruption — the write reports FR_OK, the file has exactly the expected size, but its contents are damaged.
2. Hard failure — FR_DISK_ERR or FR_NO_FILE mid-session, with the file disappearing from the directory.

Mode 1 is the dangerous one: nothing in the API indicates that anything went wrong.

Environment

Component	Version
Seeed Arduino FS	2.1.3 (also present on master at the time of writing)
Board	Seeed Wio Terminal (Seeeduino:samd:seeed_wio_terminal)
Core	Seeeduino:samd 1.8.6
Arduino CLI	1.5.1
Card	4 GB SDHC (cardType() == 3, 3781 MB), FAT32

Root cause

src/SD/Seeed_sdcard_hal.cpp, sdWriteBytes():

char sdWriteBytes(uint8_t pdrv, const char *buffer, char token)
{
    ardu_sdcard_t *card = s_cards[pdrv];
    unsigned short crc = (card->supports_crc) ? CRC16(buffer, 512) : 0xFFFF;
    if (!sdWait(pdrv, 500))
    {
        return false;
    }
    card->spi->transfer(token);
    card->spi->transfer((uint8_t *)buffer, 512);   // <-- destroys *buffer
    card->spi->transfer16(crc);
    return (card->spi->transfer(0xFF) & 0x1F);
}

The overload it resolves to, in the SAMD core (libraries/SPI/SPI.cpp):

void SPIClass::transfer(void *buf, size_t count)
{
  uint8_t *buffer = reinterpret_cast<uint8_t *>(buf);
  for (size_t i=0; i<count; i++) {
    *buffer = transfer(*buffer);
    buffer++;
  }
}

Note also that the const on sdWriteBytes's buffer parameter is cast away to call it, which is what hides the mutation from the compiler.

Call path: sync_window() in FatFs calls disk_write(fs->pdrv, fs->win, fs->winsect, 1) → sd_disk_write() → sdWriteSector() → sdWriteBytes(). FatFs does not reload fs->win afterwards; fs->winsect still names the sector, so a later move_window() for the same sector returns the clobbered buffer. During a long append-heavy workload the same FAT sector is revisited many times, so corruption is effectively guaranteed.

The multi-sector path (sdWriteSectors()) has the same problem and additionally clobbers the application's own data buffer.

Reproduction

Firmware that logs accelerometer samples to a CSV on the microSD, then reopens the file and verifies header, line count and size against what was written. Same card, same sketch, same board; the only variable is this one library line.

Unpatched, 100 Hz for 10 s:

SESSAO: Periodo concluido, arquivo=/ACC0007.CSV, amostras=858, bytes=52179, FatFs FR=0
VERIFY: bytes=52179/52179, linhas_dados=815/858, cabecalho=DIVERGENTE => INCONSISTENTE

The session reports success. The file size is exactly right. But the CSV header is corrupted and 43 data lines are missing.

Unpatched, 25 Hz for 10 s:

SESSAO: Erro de escrita, arquivo=/ACC0008.CSV, amostras=240, bytes=0, FatFs FR=4
VERIFY: f_open falhou, FR=4

FR_NO_FILE — the directory entry was destroyed. After these two runs, previously written files were gone from the volume and filename allocation restarted from the first free slot.

Patched (see below), identical runs:

VERIFY: bytes=52423/52423, linhas_dados=862/862, cabecalho=ok => INTEGRO
VERIFY: bytes=14551/14551, linhas_dados=240/240, cabecalho=ok => INTEGRO

A 1-minute run at 100 Hz also verifies clean with the patch: 5095/5095 data lines, 318967 bytes, FR=0.

Reverting the patch reproduces the failure; reapplying it restores integrity. Rebuild with --clean when testing, since the library object is cached between builds.

Suggested fix

Do not use the in-place block overload for transmit-only data. The minimal change:

     card->spi->transfer(token);
-    card->spi->transfer((uint8_t *)buffer, 512);
+    for (size_t i = 0; i < 512; i++)
+    {
+        card->spi->transfer((uint8_t)buffer[i]);
+    }
     card->spi->transfer16(crc);

This is what the reproduction above was tested with. Where the core provides it, the DMA-capable transfer(const void *txbuf, void *rxbuf, size_t count, bool block) with rxbuf == NULL would preserve block-transfer performance without mutating the source; that variant is not available on every supported core, so a guarded version may be needed.

sdWriteSectors() needs the same treatment.

Removing the (uint8_t *) cast so the const is enforced would prevent this class of bug from reappearing.

Note: a separate, unrelated defect in the same file

While tracing this, a second issue turned up. AcquireSPI's two-argument constructor ignores its frequency parameter:

AcquireSPI(ardu_sdcard_t *card, int frequency)
    : card(card)
{
    card->spi->beginTransaction(SPISettings(card->frequency, MSBFIRST, SPI_MODE0));
}

sd_disk_initialize() constructs it as AcquireSPI card_locked(card, 400000) specifically to bring the card up at 400 kHz, but the argument is discarded and initialization runs at whatever was passed to SD.begin(). The compiler flags this as unused parameter 'frequency'. It is a distinct problem from the corruption above and did not cause it, but it silently defeats the intended low-speed card initialization.

Note on a false positive

The same in-place overload is used at sdCommand():

card->spi->transfer((uint8_t *)cmdPacket, (cmd == STOP_TRANSMISSION) ? 7 : 6);

That one is harmless — cmdPacket is a local array rebuilt on every iteration and never read after the transfer. It is mentioned only so it is not mistaken for a second instance of the bug.

[baorepo]

Thank you @ygCEO for this exceptional report — the root-cause analysis, the controlled reproduction, and the proposed patch are all correct.

We verified the chain on our side: the Seeeduino SAMD core's SPIClass::transfer(void*, size_t) is indeed in-place (*buffer = transfer(*buffer)), and the buffer FatFs passes down is fs->win, which FatFs still considers a valid cache of fs->winsect after disk_write returns. This also explains why the bundled write-once examples never caught it — corruption requires the same FAT/directory sector to be re-fetched through the window after a write, i.e. append-heavy or multi-file sessions.

For context on how this was introduced: this HAL was ported from the ESP32 Arduino core's sd_diskio, where the write path uses SPIClass::writeBytes() (transmit-only, never touches the source buffer). The port swapped in the same-shaped SAMD overload without noticing its in-place semantics.

Fix shipping in v2.1.5 (shortly):

1. sdWriteBytes() now sends the 512-byte block byte-by-byte — exactly the patch you validated. On SAMD this has zero performance cost, since the core's block transfer(void*, size_t) is itself a per-byte loop. This single change also covers sdWriteSectors(), whose multi-sector loop goes through sdWriteBytes().
2. The AcquireSPI(card, frequency) constructor now uses its frequency argument, restoring the intended 400 kHz card initialization (your second finding).

The PR and release will be linked here shortly. Thanks again for a report of this quality.

[baorepo]

Shipped:

• Fix merged in fix: SD writes no longer destroy the FatFs window via in-place SPI transfer #19 (byte-by-byte send in sdWriteBytes(); AcquireSPI frequency argument fix included)
• Released as v2.1.5 — library manager update is live

If you get a chance to run your logging workload against v2.1.5, a confirmation here would be welcome. Thanks again for the excellent report.

[baorepo]

Follow-up for anyone interested in the performance side: v2.1.5's byte-by-byte fix is safe but leaves the CPU busy-waiting through every sector (~1 ms per 512-byte block at 4 MHz SPI). The SAMD core offers a DMA-capable transfer(txbuf, rxbuf, count, block) overload that fits both the write path (transfer(buffer, nullptr, 512, true) — source untouched, so this corruption class cannot recur) and the read path (transfer(nullptr, buffer, 512, true)).

We've scoped this as a follow-up enhancement with benchmarks and an integrity-regression requirement, tracked in #20 — help wanted, PRs welcome.

@ygCEO you already have the hardware, the exact workload that exposed this bug, and a verification harness — if you want to take a run at it (or just benchmark it), #20 is the place. No pressure either way; the fix in v2.1.5 stands on its own.

[baorepo]

Consolidating the performance follow-up into this thread rather than a separate issue (#20 closed — everything lives here now).

The opportunity. v2.1.5's byte-by-byte fix is safe but leaves the CPU busy-waiting through every sector (~1 ms per 512-byte block at 4 MHz SPI). The SAMD core offers a DMA-capable overload that fits both directions:

• Write (sdWriteBytes()): transfer(buffer, nullptr, 512, true) — TX DMA only, source buffer untouched, so this corruption class cannot recur
• Read (sdReadBytes()): transfer(nullptr, buffer, 512, true) — the core issues dummy 0xFF via TX DMA while RX DMA fills the buffer

The gain is not raw throughput (bounded by the SPI clock) but CPU availability during sector I/O — which matters for sample-while-logging workloads like the accelerometer logger above — plus reduced jitter.

Design constraints for a PR:

1. Platform guard: this overload only exists on the SAMD core — use #ifdef ARDUINO_ARCH_SAMD with the byte-by-byte path as fallback on other cores.
2. Watch out: if the TX DMA channel allocation fails inside the core, the transfer is silently skipped. A PR needs a compile-time opt-in or a runtime fallback for this case.
3. Token bytes, CRC (transfer16), and the sdReadBytes() token-wait loop stay single-byte — only the 512-byte payload is a DMA candidate.
4. sdWriteSectors() is covered automatically via sdWriteBytes().

Acceptance: a benchmark (sequential-write throughput + CPU-free time during writes, byte-by-byte vs DMA, on Wio Terminal) and this issue's integrity workload (multi-session append + read-back verify) staying clean with DMA enabled.

help wanted — happy to review. @ygCEO you have the hardware, the exact workload, and the verification harness — a run at this from you would be ideal, but no pressure either way.

[ygCEO]

Thanks for the quick turnaround, and for the context on the ESP32 sd_diskio port — that explains the provenance neatly. I diffed the v2.1.5 tag against my patched copy: sdWriteBytes() matches byte for byte, and the AcquireSPI(card, frequency) change is there too. Nothing else in the HAL moved.

Before I commit bench time to the DMA follow-up, two notes on the design constraints. Both come from reading the Seeeduino SAMD core 1.8.6 sources; I have not put a scope or a card on this yet, so please treat the second one as a prediction to be checked rather than a measurement.

1. Constraint 2 may not apply on this core.

if the TX DMA channel allocation fails inside the core, the transfer is silently skipped

I don't think that's the case in libraries/SPI/SPI.cpp 1.8.6. The DMA path is guarded by

if(writeDescriptor && (readDescriptor || !rxbuf)) {   // SPI.cpp:292

and the else branch at SPI.cpp:360-381 is a byte-by-byte fallback using _p_sercom->transferDataSPI(). If writeChannel.allocate() fails, writeDescriptor stays NULL and that fallback runs. So a PR would not need a compile-time opt-in for this case — the core already degrades gracefully.

Worth noting the write-only branch of that fallback is (void)_p_sercom->transferDataSPI(*txbuf8++), i.e. it reads from the source and never writes back into it. So the corruption class from this issue cannot recur on either path of the 4-argument overload, which is the important part.

2. A hazard I think a PR does have to handle: TX-only DMA leaves the receiver undrained.

SERCOM::initSPI() sets SERCOM_SPI_CTRLB_RXEN unconditionally (cores/arduino/SERCOM.cpp:253), so the receiver is always active. With transfer(buffer, nullptr, 512, true) no RX DMA channel is allocated, and nothing in SPI.cpp reads DATA, clears INTFLAG.RXC, or clears STATUS.BUFOVF — a grep for those names in that file comes back empty.

So when the 512-byte TX DMA completes, RXC should be set with stale bytes queued and BUFOVF likely set as well. The next single-byte operation is SERCOM::transferDataSPI():

sercom->SPI.DATA.bit.DATA = data;
while(sercom->SPI.INTFLAG.bit.RXC == 0);   // already 1
return sercom->SPI.DATA.bit.DATA;          // stale byte

In sdWriteBytes() the two operations right after the payload are the CRC and the data-response token:

card->spi->transfer16(crc);
return (card->spi->transfer(0xFF) & 0x1F);

If the above holds, both reads come back shifted by however many stale bytes are buffered, the response token fails its & 0x1F check, and every write reports failure.

What makes this unpleasant is that it is path-dependent in exactly the wrong direction: the non-DMA fallback drains the receiver as a side effect of transferDataSPI() reading each byte, while the DMA path does not. A sketch that happens to lose the channel race to another library would work, and the same sketch with channels free would fail — or vice versa, depending on which symptom you hit first.

The read direction looks fine, since transfer(nullptr, buffer, 512, true) has RX DMA consuming every byte.

Two remedies I can see for a PR, both cheap: drain after the TX-only transfer (read DATA while RXC is set, then write 1 to STATUS.BUFOVF), or give the transfer a real RX DMA descriptor pointed at a one-byte scratch with destination increment disabled. The first keeps the change inside the FS library; the second is arguably the core's job.

Does this match your reading of the core? If it does, I'd suggest folding it into the acceptance criteria for #18, since a benchmark run against a write path that fails its response token would not tell anyone much. I have the Wio Terminal, the append-heavy logging workload from this issue, and a read-back verifier, so measuring is not the hard part — I would just rather not measure the wrong build.