SPI bug?

DrM · Jan 22, 2023

Here are two code snippets, they run from loop. Again, this is a contrived example do demonstration what seems like a bug.

The first works perfectly okay. The second hangs at SPI.transfer16. What I am doing wrong?

Is there another easier way to set the speed? I want to set it for the maximum allowed.

Code:

loop() {

      SPISettings settings(2000000, MSBFIRST, SPI_MODE0);
      
      SPI.begin() ;
      digitalToggleFast(sparePin );
      SPI.transfer16(0xFFFF);
      digitalToggleFast(sparePin );
      SPI.end();

      
      SPI.beginTransaction(settings);
      digitalToggleFast(sparePin );    // pin goes high
      SPI.transfer16(0xFFFF);          // << hangs here, pin never goes low
      digitalToggleFast(sparePin );
      SPI.endTransaction();

    delay(1000);
}

defragster · Jan 22, 2023

quick note: complete source code
For sake of copy paste and trying the code ... no setup and no idea what sparePin might be ...

DrM · Jan 22, 2023

SPI assorted problems, complete sketch

Okay, here is a complete sketch.

There are several problems.

1) The datasheet says the max speed is 30MHz, But the fastest the SPI library is only 4 MHz. How do we get the faster speed?

2) Any use of the settings interface fails.

Here are the scope traces for "test divider 0" and "test divider 1". This is 16 bits, the fastest speed is 4usecs, so that is 4MHz.

The command test setting simply hangs at the transfer. So that would seem to be a bug.

And here is the sketch

Code:

#include "Arduino.h"

#include <limits.h>

#include <SPI.h>

const int PinA   = 2;
const int led = 13;

#define RCVLEN 256
char rcvbuffer[RCVLEN];
uint16_t nrcvbuf = 0;

char *startsWith( char *s, const char *key ) {
  int n = strlen(key);
  if ( !strncmp( s, key, n ) ) {
    return s + n;
  }
  return 0;
}

char *parseUint( char *s, unsigned int *u ) {
  unsigned long int l;
  char *p = s;
  l = strtoul( s, &p, 0 );
  if ( (p > s) && (l <= UINT_MAX) ) {
    *u = (unsigned int) l;
    return p;
  }
  return 0;
}


void setup() {

  pinMode(led, OUTPUT);  

  pinMode(PinA, OUTPUT);
  digitalWriteFast(PinA, LOW);

  Serial.begin(9600);
  delay(100);


}

void loop() {

  uint16_t nlen = 0;
  char *pc;
  char c;

  unsigned int utmp = 0;
  
  while ( Serial.available() ) {

    c = Serial.read();

    if ( c ) {

      if ( iscntrl( c )  ) {
        nlen = nrcvbuf;
        rcvbuffer[nrcvbuf] = 0;
        nrcvbuf = 0;
        break;
      }

      else if ( nrcvbuf || !isspace(c) ) {
        rcvbuffer[nrcvbuf++] = c;        
      }

      if ( nrcvbuf >= RCVLEN ) {
        Serial.println( (char *)"Error: buffer overflow" );
        nrcvbuf = 0;
      }
    }
    
  }
  
  if ( nlen > 0 ) {

    if ( (pc = startsWith( rcvbuffer, "test divider")) && parseUint( pc, &utmp ) ) {

      SPI.begin();

      SPI.setClockDivider(utmp);

      digitalToggleFast(PinA );

      utmp = SPI.transfer16(0xFFFF);

      digitalToggleFast(PinA );

      SPI.end();

      Serial.println( utmp );
    }
    
    else if ( (pc = startsWith( rcvbuffer, "test settings"))  && parseUint( pc, &utmp )) {

      SPISettings settings( utmp, MSBFIRST, SPI_MODE0);
      
      SPI.beginTransaction(settings);

      digitalToggleFast(PinA );

      //SPI.transfer16(0xFFFF);
      utmp = SPI.transfer(0xFF);

      digitalToggleFast(PinA );

      SPI.endTransaction();

      Serial.println( utmp );

    }
    
    

    else {
      Serial.print( "unknown command //" );
      Serial.print( (char *) rcvbuffer );
      Serial.println( "//" );
      
    }

    
    nlen = 0;

    Serial.println( "DONE" );
    
  }
    
  delay(100);
   
}

DrM · Jan 22, 2023

@defragster, yes of course.

defragster · Jan 22, 2023

Not at a place to run the code - hoping others might ...

Note Pin 13 is SCK - so setup pinMode(13) will be reset in spi.begin. Not used after - but had to check after seeing that.
Second 'test' case doesn't include spi.begin?

MCU Hardware 'spec' says 25 MHz - but faster can be requested, to some degree.

Olde code shows MAX=12M:

Code:

	// This function is [B]deprecated[/B].	 New applications should use
	// beginTransaction() to configure SPI settings.
	void setClockDivider(uint8_t clockDiv) {
		if (clockDiv == SPI_CLOCK_DIV2) {
			setClockDivider_noInline(SPISettings(12000000, MSBFIRST, SPI_MODE0).ctar);
...

DrM · Jan 22, 2023

@defragster Any speed that I set in the SPI above 4MHz locks it up. You can try it with the sketch that I listed. I hope and pray that the SPI gets fixed, getting to the full max speed is super important.

Meanwhile, here is what I came up with, toggling the lines directly in a loop. Please see complete code below. It runs close to 20 MHz. Not quite enough, and a little disappointing.

Code:

      counter = 0;

      delayStart();
      CLEARPINA;       // this will be the SPI clock pin

      TOGGLEPINB;   // for the oscilloscope

      while( counter < 16 ) {

	delayNext();
	SETPINA;

	uval |= (READPINC) << counter;    // this will be the data input from the spi
	counter++;

	delayNext();
	CLEARPINA;

      }

      TOGGLEPINB;

By manually toggling one line and reading the other, I can get to 19.4 MHz. The cpu cycle step is determined by iterating over values to see where it stops getting faster,

Code:

#include "Arduino.h"

#include <limits.h>

#include <SPI.h>

const int PinA   = 2;
const int PinB   = 1;
const int PinC   = 0;
const int led = 13;

#define READPINC (CORE_PIN0_PINREG & CORE_PIN0_BITMASK)

#define SETPINA (CORE_PIN2_PORTSET = CORE_PIN2_BITMASK)
#define CLEARPINA (CORE_PIN2_PORTCLEAR = CORE_PIN2_BITMASK)
#define TOGGLEPINA (CORE_PIN2_PORTTOGGLE = CORE_PIN2_BITMASK)

#define SETPINB (CORE_PIN1_PORTSET = CORE_PIN1_BITMASK)
#define CLEARPINB (CORE_PIN1_PORTCLEAR = CORE_PIN1_BITMASK)
#define TOGGLEPINB (CORE_PIN1_PORTTOGGLE = CORE_PIN1_BITMASK)

#define RCVLEN 256
char rcvbuffer[RCVLEN];
uint16_t nrcvbuf = 0;

char *startsWith( char *s, const char *key ) {
  int n = strlen(key);
  if ( !strncmp( s, key, n ) ) {
    return s + n;
  }
  return 0;
}

char *parseUint( char *s, unsigned int *u ) {
  unsigned long int l;
  char *p = s;
  l = strtoul( s, &p, 0 );
  if ( (p > s) && (l <= UINT_MAX) ) {
    *u = (unsigned int) l;
    return p;
  }
  return 0;
}


void setup() {

  pinMode(led, OUTPUT);  

  pinMode(PinA, OUTPUT);
  digitalWriteFast(PinA, LOW);

  pinMode(PinB, OUTPUT);
  digitalWriteFast(PinB, LOW);

  pinMode(PinC, INPUT);
  
  Serial.begin(9600);
  delay(100);

}

uint32_t cyccnt = ARM_DWT_CYCCNT;
uint32_t cyccnt1 = ARM_DWT_CYCCNT;
static uint32_t cycdelay = 13; // this is the fastest the read loop can go, by experiment

inline void delayStart( ) {
  cyccnt = ARM_DWT_CYCCNT;
}

inline void delayNext( ) {

  do {
    cyccnt1 = ARM_DWT_CYCCNT;
  } while ( cyccnt1 - cyccnt < cycdelay );
  cyccnt = cyccnt1;
  
}


void loop() {

  uint16_t nlen = 0;
  char *pc;
  char c;

  unsigned int counter = 0;
  unsigned int utmp = 0;
  unsigned int uval = 0;
  
  while ( Serial.available() ) {

    c = Serial.read();

    if ( c ) {

      if ( iscntrl( c )  ) {
        nlen = nrcvbuf;
        rcvbuffer[nrcvbuf] = 0;
        nrcvbuf = 0;
        break;
      }

      else if ( nrcvbuf || !isspace(c) ) {
        rcvbuffer[nrcvbuf++] = c;        
      }

      if ( nrcvbuf >= RCVLEN ) {
        Serial.println( (char *)"Error: buffer overflow" );
        nrcvbuf = 0;
      }
    }
    
  }
  
  if ( nlen > 0 ) {

    if ( (pc = startsWith( rcvbuffer, "test divider")) && parseUint( pc, &utmp ) ) {

      SPI.begin();

      SPI.setClockDivider(utmp);

      digitalToggleFast(PinA );

      utmp = SPI.transfer16(0xFFFF);

      digitalToggleFast(PinA );

      SPI.end();

      Serial.println( utmp );
    }
    
    else if ( (pc = startsWith( rcvbuffer, "test settings"))  && parseUint( pc, &utmp )) {

      SPISettings settings( utmp, MSBFIRST, SPI_MODE0);
      
      SPI.beginTransaction(settings);

      digitalToggleFast(PinA );

      //SPI.transfer16(0xFFFF);
      utmp = SPI.transfer(0xFF);

      digitalToggleFast(PinA );

      SPI.endTransaction();

      Serial.println( utmp );

    }
    
    else if ( (pc = startsWith( rcvbuffer, "test timing")) ) {

      Serial.println( cycdelay );

      counter = 0;

      CLEARPINA;
      delayStart();

      TOGGLEPINB;
      while( counter < 16 ) {
	delayNext();
	SETPINA;
	uval |= (READPINC) << counter;
	counter++;
	delayNext();
	CLEARPINA;
      }
      TOGGLEPINB;

    }
    
    else {
      Serial.print( "unknown command //" );
      Serial.print( (char *) rcvbuffer );
      Serial.println( "//" );
      
    }

    
    nlen = 0;

    Serial.println( "DONE" );
    
  }
    
  delay(100);
   
}

DrM · Jan 22, 2023

@defragster

Second 'test' case doesn't include spi.begin?

It has SPI.beginTransaction(settings). Does it need SPI.begin() also? And I guess that means it needs SPI.end() after SPI.endTransaction()? It didnt seem so in the source code.

DrM · Jan 22, 2023

@defragster. that works!!! Thank you. 25MHz. Now, we'll see if goes higher.

DrM · Jan 22, 2023

And, it works at 30MHz! Fantastic. Thank you

defragster · Jan 22, 2023

See pjrc.com/teensy/td_libs_SPI.html

.Begin - yes - it does class init

.end - not during use

DrM · Jan 23, 2023

So, as it turns out, everything is working great and as spec'd.

Here is the code that I wound up with, for the 16bit transfers from the ADC with the convert pin asserted for 700nsec between transfer. The time from CNVST to start the transfer is about 120nsec, and it takes about 3msecs to transfer 2048 16-bit words.

One last question, the chip datasheet says data changes on the falling edge of each SCLK. And the clock is normally low and goes high on each pulse. So I think that means MODE0, yes?

Code:

    else if ( (pc = startsWith( rcvbuffer, "test block")) ) {

      uint16_t *bp = &buffer[0];
      
      SPISettings settings( 30000000, MSBFIRST, SPI_MODE0);   // 30 MHz

      if (!spibegun) {
	Serial.println( "begin spi" );
	SPI.begin();	
	SPI.beginTransaction(settings);
	spibegun = true;
      }

      elapsed_usecs = 0;

      for( counter = 0 ; counter < 2048; counter++ ) {
	SETPINA;
	delayNanoseconds( 700 );
	CLEARPINA; // 120 nsecs to start the transfer
	(*bp++) = SPI.transfer16(0xFFFF);
      }

      utmp = elapsed_usecs;
      
      Serial.println( utmp );

    }

And here is what it looks like on the oscilloscope. The ringing is because I did this just clipping the probes to a bare Teensy4 without any supporting circuitry. Time resolution is set a bit long to show one complete transfer with the convert signal.

defragster · Jan 23, 2023

SPI.begin(); needs called just one time, typically in setup().

As for mode this (one of many) came up on search: corelis.com/education/tutorials/spi-tutorial/

Code:

If CPOL and CPHA are both ‘0’ (defined as Mode 0) data is sampled at the leading rising edge of the clock. 
Mode 0 is by far the most common mode for SPI bus slave communication. If CPOL is ‘1’ and CPHA is ‘0’ 
(Mode 2), data is sampled at the leading falling edge of the clock. Likewise, CPOL = ‘0’ and CPHA = ‘1’ 
(Mode 1) results in data sampled at on the trailing falling edge and CPOL = ‘1’ with CPHA = ‘1’ 
(Mode 3) results in data sampled on the trailing rising edge. Table 1 below summarizes the available modes.

DrM · Jan 23, 2023

@defragster, yes of course, that is what the logical test does. in this test program, it is called the first time that command is recognized and then not afterwards.

I think that description is consistent with mode0 for the part. It presents the next data bit on the falling edge, therefore the preceding data bit should should have been sampled on the preceding rising edge, and so forth.

kd5rxt-mark · Jan 23, 2023

DrM said:
...yes of course, that is what the logical test does. in this test program, it is called the first time that command is recognized and then not afterwards.

Yes, but as @defragster indicates, you could get rid of the time wasted (albeit, as small as it is...however, as you indicated in your earlier posts, you are trying to minimize the time spent in the transfer process in order to maximize the speed of the interface) required to check that flag every time by making the SPI.begin() call once in setup(), which would also eliminate the need for the extra flag (freeing up an additional bit for other uses

). Every little bit of optimization, no matter how small, should help with your speedup objective.

Mark J Culross
KD5RXT

DrM · Jan 23, 2023

@kd5rxt-mark Its just a test program. I wait to initialize the spi until I am actually testing the spi. That is one of the way you check things, your turn them on one at a time. It ranks right up there next to the holy print statement, or the trusty pin toggle in the "on-the-metal" world.

defragster · Jan 23, 2023

DrM said:
@defragster, yes of course, that is what the logical test does. in this test program, it is called the first time that command is recognized and then not afterwards.

I think that description is consistent with mode0 for the part. It presents the next data bit on the falling edge, therefore the preceding data bit should should have been sampled on the preceding rising edge, and so forth.

Snippets again?

Okay - misread the problem then - didn't see the "if (!spibegun)" - and not sure what that "spibegun" is ???

Code:

      if (!spibegun) {
	Serial.println( "begin spi" );
	SPI.begin();	
	SPI.beginTransaction(settings);
	spibegun = true;
      }

So perhaps it is a static/global bool so .begin() is called only once?

Then the problem is this is also only called once: SPI.beginTransaction(settings);

Though maybe that is elsewhere outside the "snippet"? But each beginTransaction() should precede an actual transfer() and the endTransaction() should terminate each transfer.

IIRC @PaulStoffregen was behind this Transactional wrapping integration into Arduino to facilitate better SPI usage/control/ordering when multiple SPI devices were on the bus.

The design of SPI.beginTransaction(settings); and endTransaction() is that they 'wrap' each SPI transfer.

From p#10 link: @ "Basic Usage"

Code:

SPI.begin()
Call this function first, to initialize the SPI hardware. The SCK, MOSI and MISO pins are initialized. You should manaully configure the SS pin.

SPI.usingInterrupt(interrupt)
If your program will perform SPI transactions within an interrupt, call this function to register the interrupt number or name with the SPI library. This allows beginTransaction() to prevent usage conflicts.

[B]SPI.beginTransaction(SPISettings(clockspeed, MSBFIRST, SPI_MODE0))[/B]
Begin using the SPI bus. Normally this is called before asserting the chip select signal. The SPI is configured to use the clock, data order (MSBFIRST or LSBFIRST) and data mode (SPI_MODE0, SPI_MODE1, SPI_MODE2, or SPI_MODE3). The clock speed should be the maximum speed the SPI slave device can accept.

digitalWrite(SSpin, level)
Most SPI devices define a transfer of multiple bytes. You need to write the SS pin before the transfer begins (most chips use LOW during the transfer) and write it again after the last byte, to end the transfer. See below for more SS pin details.

SPI.transfer(data)
Transmit a byte from master to slave, and simultaneously receive a byte from slave to master. SPI always transmits and receives at the same time, but often the received byte is ignored. When only reception is needed, 0 or 255 is transmitted to cause the reception.

[B]SPI.endTransaction()[/B]
Stop using the SPI bus. Normally this is called after de-asserting the chip select, to allow other libraries to use the SPI bus.

DrM · Jan 23, 2023

@defragster And if nothing else is using the bus? Actually, this is in preparation for some code that reads frames from a linear CCD, some 2K words per shutter and shutters can run on a clock or triggered from a digital input. So, really this needs to own the bus from power up to power down. And there are no other SPI devices in the system.

P/S Its running in the actual system now, pretty well. This is it in schematic, the capitalized lines are macros that set the pins as bit-wise operations on the respective registers.

Code:

void shutter_ISR() {

  SET_SHUTTER;
  delayMicroseconds(1);
  CLEAR_SHUTTER;
  delayMicroseconds(1);

  for ( int i = 0; i < npixels ; i++ ) {
 
    WAIT_CLOCK_HIGH;   // wait for leading edge on clock input pin
    WAIT_CLOCK_LOW;

    // 700 nsec convert pulse
    SET_CNVRT; 
    delayNanoseconds(700);
    CLEAR_CNVRT; 

    buffer[i] = SPI.transfer16(0XFFFF);

  }   

}

kd5rxt-mark · Jan 23, 2023

DrM said:
@kd5rxt-mark Its just a test program. I wait to initialize the spi until I am actually testing the spi. That is one of the way you check things, your turn them on one at a time. It ranks right up there next to the holy print statement, or the trusty pin toggle in the "on-the-metal" world.

Yes, but once again, when you don't use the library correctly, and then you hope & pray that something gets fixed, when in reality, what needs fixed is completely within your own control . . . Surely the library must be where the fault lies ?!? Reread that & let it fully sink in . . .

I think it is safe to generalize by saying that many Teensy users are using the SPI bus successfully (myself included over many projects & many different kinds of peripherals, and also yourself included, or so it appears in p#9, albeit with some continued torture & abuse of the library), but with that success, it is also safe to say that things always work best when correct calls are made in the correct order, and at the correct time. For example, have you looked at the SPI examples that Teensyduino installs & makes available in the Arduino IDE (working examples can be a real gold mine !!) ?? If you go look, you'll see proper use (when, where, & how) of the functions that @defragster pointed you towards in several of his earlier posts. When you continued to ignore his reference, he eventually went so far as to cut & paste them in for your reference.

Your snide replies to myself and others who are attempting to provide knowledgeable guidance really spoils the scene. Speaking primarily for myself, I would say that no one with sufficient experience needs anyone, particularly you, to sarcastically & condescendingly explain one of the way(s) on how checkout & troubleshooting is done . . . you should understand that those attempting to help by replying with suggestions and/or things to check/modify probably & usually have sufficient positive experience to back up their suggested guidance. From the perspective of one offering suggestions, you don't seem to appreciate the help being offered, particularly when you continue to ignore the suggested guidance, even when pointed out multiple times, sometimes by multiple users.

I'll assert that test programs serve one purpose: to smooth out any bumps along the way toward final sketches, and to provide insight into solving any problems encountered along the way. They may also serve as a learning tool, but that is not the use being discussed here. As you fill out the capabilities of your sketch, implementing anything that is contrary to the designed & intended use of a library is a fool's errand, regardless of whether it is for test purposes or otherwise. You own the freedom to choose how to spend and/or waste your time, but if your ultimate sketch requires that you enable the SPI bus, then I would think that you would want to enable the SPI bus as suggested. . . the earlier that you find out that you have a problem (if there even is a problem), the better, no ??

My final recommendation is to start with a known working example. Study it to make sure that you understand, as completely as possible, what it is doing, why it is doing what it is doing, and how it is accomplishing its intended purpose. Then, and only then, start modifying the working example a little bit at a time, ultimately leading towards a sketch which serves your specific purpose with your specific peripheral. Keep intermediate copies of your sketch as you make positive progress. Now, this recommended approach is not a "holy" approach, but it certainly is a "trusty" one that will almost always benefit you along the way.

Good luck in your endeavors !!

Mark J Culross
KD5RXT

defragster · Jan 23, 2023

DrM said:
@defragster And if nothing else is using the bus? Actually, this is in preparation for some code that reads frames from a linear CCD, some 2K words per shutter and shutters can run on a clock or triggered from a digital input. So, really this needs to own the bus from power up to power down. And there are no other SPI devices in the system.
...

Snippets and pieces questioning function were presented here, so just looking up and linking docs and info to show intended use.

Good it has been made to work - Bug Free - in the actual system.

@KD5RXT

DrM · Jan 24, 2023

@defragster Thank you very much, and also @KurtE and @PaulStoffregen I do very much appreciate the help, and you have all been fantastic.

I called beginTransaction() to set the speed. Is there another way that is intended to do that?

At the moment this is running close to the hairy edge in terms of speed. I simply can't afford another call in the loop. At some point, when I have some time, I will probably convert this to the minimum set of register level instrucfions and see if there are some steps that can be moved out of the loop.

Stream of consciousness, I wonder if it would be useful to add an API for the ADC use case? That would be a block transfer of some number of words with the CONVERT line toggled for some time between each word transfer. (If I get that to work, I will try to remember to post it.)

And finally, perhaps as an aside, I noticed that when this is running at its best speed, there are time losses in an elapsedMicros timer. So, I am wondering, do the elapsedMicros timers depend on interrupts? That is not something I need to fix, I replaced it with a routine based on the cpu cycles counter. But it is definitely something to be aware of.

BriComp · Jan 24, 2023

I called beginTransaction() to set the speed. Is there another way that is intended to do that?

If you examine SPI.h you will find that beginTransaction uses SPISetting(clock, bitOrder, dataMode) to setup the SPI port.

If you are not USING SPI transactions then set SPI_HAS_TRANSACTION to 0 on line 30 and use SPISettings.

SPI bug?

Well-known member

Senior Member+

Well-known member

Well-known member

Senior Member+

Well-known member

Well-known member

Well-known member

Well-known member

Senior Member+

Well-known member

Senior Member+

Well-known member

Well-known member

Well-known member

Senior Member+

Well-known member

Well-known member

Senior Member+

Well-known member

Well-known member