DMABaseClass::source() bug

christoph

Well-known member
Hi Paul,

I really think this is a bug. Let take the code for DMABaseClass::source(const unsigned char &p) and expand it using p = SPI0_POPR:

Code:
TCD->SADDR = &SPI0_POPR;
TCD->SOFF = 0;
TCD->ATTR_SRC = 0;
if ((uint32_t)SPI0_POPR < 0x40000000 || TCD->NBYTES == 0) TCD->NBYTES = 1;
TCD->SLAST = 0;

This code reads SPI0_POPR while it is supposed to set up a DMA channel. It is clearly missing an ampersand in the if-statement. The same is done in the other source() overloads and in all destination() overloads.

Regards

Christoph
 
Sorry I am not tried this stuff nor an expert, but where to do you think an & is missing?

Code:
	void source(const unsigned char &p) {
		TCD->SADDR = &p;
		TCD->SOFF = 0;
		TCD->ATTR_SRC = 0;
		if ((uint32_t)p < 0x40000000 || TCD->NBYTES == 0) TCD->NBYTES = 1;
		TCD->SLAST = 0;
	}
The way I am reading this, is you pass in some hardware address in to this, again not sure which addresses you would normally use, but example:
Code:
#define DMA_TCD0_SADDR		(*(volatile const void * volatile *)0x40009000) // TCD Source Address
Which is defined in kenetis.h.

So my guessing at this if statement is it is checking for a hardware address by reference. Yes he uses the & in the assignment a few lines up, but my assumption is he that TCD->SADDR is a pointer variable.

Again sorry for jumping in here and I am probably wrong.

Kurt
 
Well if a hardware address is being used for SADDR by taking the address of p, as in
Code:
TCD->SADDR = &p;
then branching by address type (peripheral vs memory) must also use the & to get the address of the variable passed as reference. Assuming that SPI0_POPR is passed to source(), as in
Code:
dmaChannel.source(SPI0_POPR)
then this line
Code:
if ((uint32_t)p < 0x40000000 || TCD->NBYTES == 0) TCD->NBYTES = 1;
will read SPI0_POPR instead of using the address of SPI0_POPR. If the SPI had previously received data, this would result in the oldest byte being removed from the SPI FIFO, changing the SPI's state and discarding data. That's certainly not what we want.
 
I've issued a pull request: https://github.com/PaulStoffregen/cores/pull/33
My own fork already contains the suggested changes: https://github.com/crteensy/cores/tree/patch-1

Adding & in front of the p in source() did the trick, no need to directly access the TCD. I also did this in destination().

The resulting sketch if anyone wants to try it (wiring info in line 7):
Code:
#include <WProgram.h>

#include "DMAChannel.h"
#include "SPI.h"

/** Hardware setup:
 plain Teensy 3.1 with DOUT connected to DIN
 nothing else
**/

/** buffers to send from and to receive to **/
#define DMASIZE 1000
uint8_t src[DMASIZE];
volatile uint8_t dest[DMASIZE];

/** The DMA channel objects **/
DMAChannel txChannel;
DMAChannel rxChannel;

/** for timing checks **/
elapsedMicros txTime;
uint32_t fillTime;
uint32_t totalTime;

/** supposed to be called when the Tx DMA is done **/
void txComplete()
{
  txChannel.clearInterrupt(); // we need this to not get any more ISR calls
  fillTime = txTime;
}

/** supposed to be called when the Rx DMA is done **/
void rxComplete()
{
  rxChannel.clearInterrupt(); // we need this to not get any more ISR calls
  totalTime = txTime;
}

/** Wait for and consume a keypress over USB **/
void waitForKeyPress()
{
  Serial.println("\nPress a key to continue\n");
  while(!Serial.available());
  while(Serial.available())
  {
    Serial.read();
  }
}

void dumpBuffer(const volatile uint8_t* buf, const char* prefix)
{
  Serial.print(prefix);
  for (size_t i = 0; i < DMASIZE; i++)
  {
    Serial.printf("0x%02x ", buf[i]);
  }
  Serial.print('\n');
}
/** Compare the buffers and print the destination contents if there's a mismatch **/
void compareBuffers()
{
  int n = memcmp((const void*)src, (const void*)dest, DMASIZE);
  if (n == 0)
  {
    Serial.println("src and dest match");
  }
  else
  {
    Serial.println("src and dest don't match");
    dumpBuffer(src, " src: " );
    dumpBuffer(dest, "dest: ");
  }
}  

void setup()
{
  waitForKeyPress();
  Serial.println("Hi!");

  /** Prepare source and destination **/
  for (size_t i = 0; i < DMASIZE; i++)
  {
    src[i] = i;
  }
  memset((void*)dest, 0x00, DMASIZE);
  Serial.println("Buffers are prepared");Serial.flush();

  /** set up SPI **/
  SPISettings spiSettings;
  SPI.begin();
  
  // transmit 10 bytes and measure time to get a feel of how long that takes
  SPI.beginTransaction(spiSettings);
  elapsedMicros us;
  for (size_t i = 0; i < DMASIZE; i++)
  {
    dest[i] = SPI.transfer(src[i]);
  }
  uint32_t t = us;
  Serial.print("Time for non-DMA transfer: ");Serial.print(t);Serial.println("us");
  SPI.endTransaction();
  compareBuffers();
  
  waitForKeyPress();
  txTime = elapsedMicros();
  
  /** Prepare the SPI for DMA operation **/
  SPI.beginTransaction(spiSettings);
  
  SPI0_SR = 0xFF0F0000;
  SPI0_RSER = 0;
  SPI0_RSER = SPI_RSER_RFDF_RE | SPI_RSER_RFDF_DIRS | SPI_RSER_TFFF_RE | SPI_RSER_TFFF_DIRS; 
  
  Serial.println("SPI is prepared for DMA"); Serial.flush();

//  /** set up a DMA channel for transmitting src to the SPI **/
  txChannel.sourceBuffer(src, DMASIZE);
  txChannel.destination((volatile uint8_t&)SPI0_PUSHR);
  txChannel.disableOnCompletion();
  txChannel.triggerAtHardwareEvent(DMAMUX_SOURCE_SPI0_TX);
  txChannel.attachInterrupt(txComplete);
  txChannel.interruptAtCompletion();
  Serial.println("Tx DMA channel is prepared"); Serial.flush();

  /** set up a DMA channel for receiving from the SPI to dest **/
  rxChannel.source((volatile uint8_t&)SPI0_POPR);
  rxChannel.destinationBuffer((volatile uint8_t*)dest, DMASIZE);
  rxChannel.disableOnCompletion();
  rxChannel.triggerAtHardwareEvent(DMAMUX_SOURCE_SPI0_RX);
  rxChannel.attachInterrupt(rxComplete);
  rxChannel.interruptAtCompletion();
  Serial.println("Rx DMA channel is prepared"); Serial.flush();

  memset((void*)dest, 0x00, DMASIZE);
  
  Serial.println("\nStarting DMA transfer");

  t = txTime;
  Serial.print("DMA Setup time: ");Serial.print(t);Serial.println(" us");Serial.flush();
  txTime = elapsedMicros();
  rxChannel.enable();
  txChannel.enable();
  
  
  while(!rxChannel.complete())
  {
  }
  Serial.println("Finished DMA transfer");

  Serial.print("SPI FIFO fill time: ");Serial.print(fillTime);Serial.println(" us");Serial.flush();
  Serial.print("Total transfer time: ");Serial.print(totalTime);Serial.println(" us");Serial.flush();

  compareBuffers();

  SPI.endTransaction();
  SPI.end();
  pinMode(LED_BUILTIN, OUTPUT);
}

void loop()
{
  digitalWriteFast(LED_BUILTIN, true);
  delay(500);
  digitalWriteFast(LED_BUILTIN, false);
  delay(500);
}
 
Back
Top