SPI Library: SPI Mode 2 and 3 are wrong

[olikraus]

First: Thanks a lot for the ESP8266 board package for the Arduino IDE. Great Work.

Basic Infos

Hardware

Hardware: Adafruit Feather HUZZAH ESP 8266
Core Version: Boardpackage 2.3.0

Description

Problem description
SPI Mode 2 and 3 are swapped compared to other boards like Uno, Due and 101.
A .ino file written for the Due, which uses SPI Mode 2 or 3 will not work on the ESP8266.

Settings in IDE

Module: Generic ESP8266 Module
Flash Size: 4MB
CPU Frequency: 80Mhz
Flash Mode: ?
Flash Frequency: ?
Upload Using: USB Serial
Reset Method: ?

Sketch

#include <SPI.h>

void setup() {
  // put your setup code here, to run once:
  SPI.begin();
}

void loop() {
  // put your main code here, to run repeatedly:
  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE0));
  SPI.transfer(0x053);
  SPI.endTransaction();

  delayMicroseconds(1);

  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE1));
  SPI.transfer(0x053);
  SPI.endTransaction();

  delayMicroseconds(1);

  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE2));
  SPI.transfer(0x053);
  SPI.endTransaction();

  delayMicroseconds(1);

  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE3));
  SPI.transfer(0x053);
  SPI.endTransaction();

  delayMicroseconds(16*8);  
}

Debug Messages

This picture shows the current (wrong) behavior.
The byte 0x053 is written in all four modes: From left to right mode 0, 1, 2 and 3. The last two transmitts are wrong. See below of correct output.
See also here: http://forum.arduino.cc/index.php?topic=419660.0

The common behavior is this for the Due:

Or this for the Uno:

BTW: I have seen that this is marked as a todo in the code for this project: Arduino/libraries/SPI/SPI.cpp

Thanks for fixing this.

[olikraus]

I suggest the following change in SPI.cpp:

void SPIClass::setDataMode(uint8_t dataMode) {

    /**
     SPI_MODE0 0x00 - CPOL: 0  CPHA: 0
     SPI_MODE1 0x01 - CPOL: 0  CPHA: 1
     SPI_MODE2 0x10 - CPOL: 1  CPHA: 0
     SPI_MODE3 0x11 - CPOL: 1  CPHA: 1
     */

    bool CPOL = (dataMode & 0x10); ///< CPOL (Clock Polarity)
    bool CPHA = (dataMode & 0x01); ///< CPHA (Clock Phase)

    if ( CPOL )     // Ensure same behavior as 
      CPHA ^= 1;    // SAM, AVR and Intel Boards

    if(CPHA) {
        SPI1U |= (SPIUSME);
    } else {
        SPI1U &= ~(SPIUSME);
    }

    if(CPOL) {
        SPI1P |= 1<<29;
    } else {
        SPI1P &= ~(1<<29);
        //todo test whether it is correct to set CPOL like this.
    }

}

not yet tested...

[olikraus]

The above mentioned fix will solve the problem:

[dratini0]

Sorry for outright bumping this, but I ran into this issue just now. I was trying to use an ST7920 LCD with the u8g2 library, which requires SPI mode 3. Since this breaks portability, I advocate merging #2418.

I have also made some measurements with my hantek 6022BE and pulseview, see attached:

esp_spi_bug.zip

[devyte]

@igrr The referenced code looks unchanged in latest git, so this bug seems to still be valid. However, I don't know enough about SPI, nor about the ESP's internal registers, nor about the other Arduino boards, to evaluate.
Does the referenced PR make sense?

[igrr]

I have an arduino nano lying around somewhere, but i won't be able to test anything and compare until late next week. If someone else can have a look at this and check, that would be great.

[dratini0]

If it helps at all, the file esp_hw_spi_bug_control.srzip in my zip was recorded from an Arduino Uno. (It goes through the SPI modes from 0 to 3, the sketch is also attached). The Wikipedia article on SPI can also be helpful.

For reference, I was using version 2.3.0 from the board manager for those measurements.

[devyte]

Also reported in #2270 .

[vicnevicne]

I'm the reporter of #2270 and I confirm I also encountered this weirdness with SPI modes more than one year ago while working on SPI slave with the help of @me-no-dev .
What still looks really weird to me is that the implementation makes use of an undocumented register bit (SPI1P.bit29) and includes a comment doubing if it is correct way to set CPOL...
IIRC the meaning of this bit changes according to the phase (CPHA) setting or something.
In his SPI implementation, @MetalPhreak has added this bit to his version of the spi_register.h but it seems he also had a hard time with polarity.
So I'm all for fixing this of course, but we probably have to be cautious to avoid any regression.
@igrr Could you request some information from Espressif developers regarding the meaning of the mysterious bit 29 of register SPIxP?