Export to GitHub

arduino - issue #888

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


Posted on Apr 14, 2012 by Grumpy Panda

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

Comment #1

Posted on Apr 17, 2012 by Happy Kangaroo

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.

Comment #2

Posted on Jun 5, 2012 by Helpful Bear

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.

Comment #3

Posted on Jun 5, 2012 by Grumpy Panda

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?

Comment #4

Posted on Jun 5, 2012 by Happy Rhino

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?

Comment #5

Posted on Jun 5, 2012 by Happy Kangaroo

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.

Comment #6

Posted on Jun 5, 2012 by Happy Rhino

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.)

Comment #7

Posted on Jun 6, 2012 by Happy Rhino

https://github.com/arduino/Arduino/commit/a363686a3183ba98024329390f82c36608f8d8f7

Can you test the updated code?

Comment #8

Posted on Jun 6, 2012 by Happy Kangaroo

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

Comment #9

Posted on Jun 6, 2012 by Grumpy Panda

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

Comment #10

Posted on Jun 6, 2012 by Happy Rhino

Great, thanks for testing!

Comment #11

Posted on Feb 8, 2013 by Massive Ox

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

Status: Fixed

Labels:
Type-Defect Priority-Medium Component-SPI Component-Core Milestone-1.0.2