adafruit / ArduinoCore-samd

114 stars 116 forks source link

SPI DMA write-only transfer(txBuf, nullptr, count) corrupts following receives. #335

Open greiman opened 1 year ago

greiman commented 1 year ago

Write-only calls to void transfer(const void *txbuf, void *rxbuf, size_t count, bool block) in this file:

https://github.com/adafruit/ArduinoCore-samd/blob/master/libraries/SPI/SPI.cpp

with rxbuf equal to nullptr cause following reads to fail.

The problem is that SPI receive is enabled in SERCOM but no DMA receive job is enabled so the receive FIFO is not emptied and may even overflow.

A following receive using any transfer function retrieves leftover bytes in the FIFO.

I tested a possible fix by enabling a DMA receive job with a static dummy byte.

It solves the problem in my SdFat library for Feather M0 SAMD21 and Feather M4 Express. The fix would need further tests.

I have attached a copy of the modified SPI.cpp.

Here is a program that demonstrates the problem. Connect MISO to MOSI and run the program to see the problem. When nullptr is used, rx does not match tx data for rx = transfer(tx).

#include "SPI.h"
// Connect MISI to MOSI.
void spiTest(bool useNull) {
  uint8_t txBuf[5] = {0, 1, 2, 3, 4};
  uint8_t rxBuf[5];
  Serial.println();
  Serial.println(useNull ? "use nullptr" : "use rxBuf");
  SPI.begin();
  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE0));

  if (useNull) {
    SPI.transfer(txBuf, nullptr, sizeof(txBuf));
  } else {
    SPI.transfer(txBuf, rxBuf, sizeof(txBuf));
  }
  for (uint8_t tx = 5; tx < 20; tx++) {
    // rx should equal tx here.
    uint8_t rx = SPI.transfer(tx);
    Serial.print("tx: ");
    Serial.print(tx);
    Serial.print(", rx: ");
    Serial.println(rx);
  }
  SPI.endTransaction();
  SPI.end();
}

void setup() {
  Serial.begin(115200);
  while (!Serial) {}
  Serial.println("Connect MISO to MOSI then type any character.");
  while (!Serial.available()) {}
  spiTest(false);
  spiTest(true);
}
void loop() {}

Here is a diff of the original and modified SPI.cpp file:

381a382,385
> #define SPI_DMA_WRITE_ONLY_FIX
> #ifdef SPI_DMA_WRITE_ONLY_FIX
>     static uint8_t rxDum;  // Dummy byte for write-only xfers
> #endif  // SPI_DMA_WRITE_ONLY_FIX
398a403
> #ifndef SPI_DMA_WRITE_ONLY_FIX
399a405,411
> #else    // SPI_DMA_WRITE_ONLY_FIX
>         rDesc->BTCTRL.bit.DSTINC = 1;
>       } else {
>         rDesc->DSTADDR.reg = (uint32_t)&rxDum;
>         rDesc->BTCTRL.bit.DSTINC = 0; // Don't increment  pointer
>       }
> #endif  // SPI_DMA_WRITE_ONLY_FIX
425a438
> #ifndef SPI_DMA_WRITE_ONLY_FIX
432a446,449
> #else   // SPI_DMA_WRITE_ONLY_FIX
>     writeChannel.setCallback(dmaDoNothingCallback);
>     readChannel.startJob();
> #endif  // SPI_DMA_WRITE_ONLY_FIX

Modified_SPI_cpp.zip