SPI.begin() clocks out an incorrect bit in mode 3 (with SS low)

[GoogleCodeExporter]

What steps will reproduce the problem?

Using SPI.begin() briefly brings the SS line (default, D10) low.

This was noticed before and discussed here:

http://arduino.cc/forum/index.php/topic,100671.0.html

The problem is, SPI.begin does this:

  pinMode(SCK, OUTPUT);
  pinMode(MOSI, OUTPUT);
  pinMode(SS, OUTPUT);

  digitalWrite(SCK, LOW);
  digitalWrite(MOSI, LOW);
  digitalWrite(SS, HIGH);

Since the default for pins is to be low, making SS output means that it is 
forced low. Then two lines later it is made high.

This has the effect of making SS go low for a few microseconds. Meanwhile, if a 
user is using normally high clock polarity (not the default) the the clock line 
is also forced low (when SCK is made OUTPUT). Combined, this makes the SPI 
appear to clock out a bit.

As described in the above thread the net effect is for the intended bits output 
to be shifted over one completely corrupting the intended data transfer.

What version of the Arduino software are you using? 

Version 1.0.

On what operating system? 

Mac OS/X.

Which Arduino board are you using?

Uno Rev 3.

Please provide any additional information below.

The fix is to simply swap the order around thus:

  digitalWrite(SCK, LOW);
  digitalWrite(MOSI, LOW);
  digitalWrite(SS, HIGH);

  pinMode(SCK, OUTPUT);
  pinMode(MOSI, OUTPUT);
  pinMode(SS, OUTPUT);

This won't take any more program memory, they are the same instructions. Now, 
the moment that the SS pin is enabled it is taken from high impedance to the 
correct state (high).

Of course, if someone doesn't want to use the SS line at all that's bad luck. 
But it is no worse than the current code.

- Nick Gammon

Original issue reported on code.google.com by n...@gammon.com.au on 14 Apr 2012 at 9:32

[GoogleCodeExporter]

OP is correct that this is a big issue with the current SPI lib; the library 
really only works for mode 0.  But as he commented, his solution does not 
encompass not using the SS line, which is pretty common I think.  It also would 
not necessarily work if the pinModes were ALREADY "OUTPUT".  The solution is to 
start the SPI before setting the pin modes.  This transitions directly from its 
current state to SPI control...

  SPCR |= _BV(MSTR);
  SPCR |= _BV(SPE);
  pinMode(SCK, OUTPUT);
  pinMode(MOSI, OUTPUT);
  pinMode(SS, OUTPUT);

  I'm not sure why digitalWrite of SCK, MOSI and SS happens at all... but using mode 3 it works either with or without those.

  But the digitalWrite of SS is particularly an issue.  It means that in your code you must do:
  SPI.begin();
  digitalWrite(SS,LOW);

not the reverse.  And this order is unintuitive -- first initialize 
communications with the chip, and only then actually enable the chip to hear 
the communication?

But removing digitalWrite of SS should be ok.  If SS is really being used, 
you'd expect that the user would write it when he wants to access the chip.  If 
it is not being used then its state does not matter.

Worst case (if all these digitalWrite() are really needed) I guess begin() 
should either take the data mode as a parameter, or we should store it as a 
member variable.

Original comment by st...@toastedcircuits.com on 17 Apr 2012 at 7:16

[GoogleCodeExporter]

On the ordering of SPI.begin() and asserting the !CS line, this probably needs 
better documentation (and, ideally, SPI.begin() would have been better to be 
called SPI.prepare() or init() or something), but it *should* be begin() 
followed by asserting CS of the slave you want to talk to.

The other issue is you can't change the direction of the SS pin after enabling 
master mode on the SPI port, it *may* randomly revert to slave if SS is 
floating, or force back to slave if SS is being pulled low externally for some 
reason. You have to set SS as output *before* enabling the SPI port. It would 
be uncool to introduce a race where SPI might not work sometimes.

With respect to the mode3 issue mentioned in the OP and forum link, it's useful 
to ensure your chip select lines are deasserted (high, in most cases, but not 
always) before touching the SPI interface. That's good practice to get into 
anyway. While it's *probably* active-low, it might not be and the SPI library 
shouldn't assume either way.

For example:

digitalWrite(10,HIGH); /* ensure our !CS on pin10 is deasserted before touching 
SPI */
pinMode(10,OUTPUT); /* ensure we can also assert this low when we need it */
SPI.setDataMode(SPI_MODE3);
SPI.begin(); /* will actually also call pinMode(SS,OUTPUT), but with no harm */
/* .. later */
digitalWrite(10,LOW); /* time to talk to our slave chip, assert !CS, driven low 
*/

Having said all that, I'm surprised there are SPI chips which do not just 
discard the shift when !CS is deasserted, if it hasn't received enough bits. 
The SPI lines would be floating anyway long before this code is run into, so it 
would fail fairly often in noisy environments if it didn't.

Original comment by david.s....@gmail.com on 5 Jun 2012 at 5:00

[GoogleCodeExporter]

My point is that since transitions are important for the SS line (particularly 
perhaps when connected to "dumber" devices) there doesn't seem to be much point 
in designing in a brief transition of the SS line (by making it output, and 
then setting it to the desired state) when you could just as easily set it to 
the desired state first, and then make it output.

In the absence of a more comprehensive change, what is the harm in swapping the 
order of the function calls?

Original comment by n...@gammon.com.au on 5 Jun 2012 at 5:18

[GoogleCodeExporter]

It sounds like there are two issues here.

The first (and the main bug) is that SPI.begin() will clock out a bit in 
certain data modes.  We should find a solution for this that doesn't involve 
the SS pin - since people often use the SPI interface without an SS pin (and 
just tie the CS pin of their slave device to ground).

The other issue is that SPI.begin() can set SS to LOW briefly, activating a 
slave device.  

It seems like we might be able to fix both of these by putting the 
digitalWrite() calls before the pinMode() calls in SPI.begin().  That way, when 
the pins are set as outputs, they should immediately take on the appropriate 
values, skipping any spurious transitions.  Can anyone test this solution (with 
both mode 0 and mode 3 devices)?

Or is there another solution?

Original comment by dmel...@gmail.com on 5 Jun 2012 at 1:17

• Changed title: SPI.begin() clocks out an incorrect bit in mode 3 (with SS low)
• Changed state: Accepted
• Added labels: Component-Core, Component-SPI, Milestone-1.0.2

[GoogleCodeExporter]

Hi David, putting digitalWrite before pinMode does not work in the case where 
the pinMode is a no-op (that is, it is already set to output).  But there is a 
solution which is to let the SPI hardware take control of the pins before 
enabling them as output (as I posted a few months ago).  But David Zanetti 
brought up an interesting point about randomly putting SPI in slave mode; I did 
not realize that once it went into slave mode it was "stuck" there forever, and 
have never seen this but regardless let me make a proposal.  If we all think it 
will work I will test this config against various chipsets.  Currently we have:

  pinMode(SCK, OUTPUT);
  pinMode(MOSI, OUTPUT);
  pinMode(SS, OUTPUT);

  digitalWrite(SCK, LOW);
  digitalWrite(MOSI, LOW);
  digitalWrite(SS, HIGH);

  // Warning: if the SS pin ever becomes a LOW INPUT then SPI 
  // automatically switches to Slave, so the data direction of 
  // the SS pin MUST be kept as OUTPUT.
  SPCR |= _BV(MSTR);
  SPCR |= _BV(SPE);
}

I recommend:

void SPIClass::begin(boolean dontWriteSS=False) {
  pinMode(SS, OUTPUT);
  if (!dontWriteSS) digitalWrite(SS, HIGH);

  // Warning: if the SS pin ever becomes a LOW INPUT then SPI 
  // automatically switches to Slave, so the data direction of 
  // the SS pin MUST be kept as OUTPUT.
  SPCR |= _BV(MSTR);
  SPCR |= _BV(SPE);  

  pinMode(SCK, OUTPUT);
  pinMode(MOSI, OUTPUT);
}

99% of our artists, etc will need SS written high, but just for those few 
hackers who dual-purpose it we can use a parameter with a default value.

Original comment by st...@toastedcircuits.com on 5 Jun 2012 at 1:48

[GoogleCodeExporter]

I'd like to avoid adding an extra parameter to begin(), at least for now.  
(Let's solve the main issue first, then we can worry about whether we really 
need a way to avoid touching the SS pin.)

What about this?

void SPIClass::begin() {
  digitalWrite(SS, HIGH);
  pinMode(SS, OUTPUT);

  // Warning: if the SS pin ever becomes a LOW INPUT then SPI 
  // automatically switches to Slave, so the data direction of 
  // the SS pin MUST be kept as OUTPUT.
  SPCR |= _BV(MSTR);
  SPCR |= _BV(SPE);  

  pinMode(SCK, OUTPUT);
  pinMode(MOSI, OUTPUT);
}

Can someone test this with mode 0 and mode 3 devices?  (And others, too, if 
possible.)

Original comment by dmel...@gmail.com on 5 Jun 2012 at 1:55

[GoogleCodeExporter]

https://github.com/arduino/Arduino/commit/a363686a3183ba98024329390f82c36608f8d8
f7

Can you test the updated code?

Original comment by dmel...@gmail.com on 6 Jun 2012 at 1:46

• Changed state: Fixed

[GoogleCodeExporter]

Fix works for qtouch1110 chip (mode 3) and for W5100 (running WebServer example 
sketch).

Original comment by st...@toastedcircuits.com on 6 Jun 2012 at 8:59

[GoogleCodeExporter]

It works OK in mode 0 accessing an SD card. Checking all modes on a logic 
analyzer looks OK.

Original comment by n...@gammon.com.au on 6 Jun 2012 at 9:07

[GoogleCodeExporter]

Great, thanks for testing!

Original comment by dmel...@gmail.com on 6 Jun 2012 at 9:08

[GoogleCodeExporter]

Hello. 

I have a big problem with SPI communication with my QT1110 chip. 
I'm not sure what is wrong: code, PCB or chip. I mistook ground and Vcc for a 
couple of second and I was not sure if this didn't burn my chip. 

As a response byte for Master's command I'm getting 0x00. 

Could you please glance at my code? Maybe you could try to connect with your 
chip using my code? If the communication will be going correctly I will know 
that I have to buy new QT1110.

#define SSpin 10
void setup()
{
  Serial.begin(115200); 
  // SPI Start
  SPI.begin();

  SPI.setBitOrder(MSBFIRST);
  SPI.setDataMode(SPI_MODE3);
  SPI.setClockDivider(SPI_CLOCK_DIV32);

  Serial.println("SPI begun");
  delay(3000);  
}  

void loop()
{
  getDeviceMode();
  delay(2000);
  setDeviceMode();
  delay(2000);
}

void getDeviceMode()
{
    byte responseIdle;  
    byte responseByte; 

  delayMicroseconds(microSecond);

  digitalWrite(SSpin, LOW);
  responseIdle = SPI.transfer(208);

  if ( responseIdle == IDLE_ANSWER)
  {
    delayMicroseconds(microSecond);
    responseByte = SPI.transfer(0);  //Send 0x00 while receiving data
    digitalWrite(SSpin, HIGH); 

    Serial.print("Device Mode: "); 
    Serial.println(responseByte); 
  }
  else 
  {
      Serial.print("response Idle:"); Serial.print(responseIdle); Serial.println("     Getting device mode - error"); 
  }
  delay(10);
}

void setDeviceMode()
{
    byte responseIdle;  
    byte responseByte; 

    // take the chip select low to select the device:
    digitalWrite(SSpin, LOW);
    responseIdle = SPI.transfer(SET_DEVICE_MODE);
    delayMicroseconds(microSecond);

  if (responseIdle == IDLE_ANSWER)
  {
      responseByte = SPI.transfer(0xE2);  //0b11100010
      digitalWrite(SSpin, HIGH); 
      delayMicroseconds(microSecond);
  }   
  else
    Serial.print("responseIdle: "); Serial.print(responseIdle);  Serial.println("     Something went wrong while setting device mode"); 

  if (responseByte ==  SET_DEVICE_MODE)
  {
      Serial.println("Communication while sending device mode was correct"); 
  }
  delay(10);
}

Thank you in advance! 
Anna

Original comment by awid...@netizens.pl on 8 Feb 2013 at 3:25