Arduino: 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?

[nophead]

This is all explained in the ESP32 techincal manual: https://www.openhacks.com/uploadsproductos/esp32_technical_reference_manual_en_qg.pdf. The ESP32 seems to have almost the same SPI module as the ESP8266.

That manual shows on page 118 that SPI_CK_OUT_EDGE should be set for modes 1 and 2, so it isn't the same as CPHA. It also documents bit 29 of the pin register as SPI_CK_IDLE_EDGE, which is the same as CPOL.

[Misiu]

@igrr @vicnevicne what about this issue? As @olikraus mentioned there is a potentially simple fix.
There is an existing PR for this: https://github.com/esp8266/Arduino/pull/2418.
I'm planing to buy a couple of ST7920 LCD's for my project and I'm wondering if this is a good choice because of this issue.

[maxbanton]

Hi!
I ran into the same problem, I cannot use a display based on ST7920 with HW SPI, only SW, which makes it slooower.
PR by @olikraus is ready, so @igrr could you please look into this?
Thanks in advance.

[d-a-v]

From #2418:

Pro: Same behavior for SPI Modes 2 and 2 compared to Uno, Due and 101.
Contra: Existing .ino files need to be changed if they use SPI_MODE2 or SPI_MODE3

I don't know the implications of the contra point.

What can be done is either

1. leave default as it is, add void SPI::fixEsp8266Mode2And3() (to enable #2418 fix)
2. apply #2418 by default and add void SPI::restoreEsp8266LegacyMode2And3() to disable it

[Misiu]

@d-a-v doesn't matter which one will be added, the important thing is that it will be fixed.
My 0.02 cents is that this should be changed and option 2 should be added because currently, this is the wrong behavior. But that's just my opinion 🙂

[nophead]

It is simply wrong and should be fixed. No reason to add a new function for the buggy old behaviour because it is trivial to fix code that relies on the bug. How would that ever have arisen anyway? Trial and error programming? The first person to use these modes should have noticed the bug.

[d-a-v]

The first person to use these modes should have noticed the bug.

(.. and used a workaround)

The "contra" is the reason why this 3years old issue with no possible smooth transition is still not merged.
I agree with you but some projects use latest cores, together with older cores in their releases (I'm thinking of at least espeasy and tasmota, or maybe only the latter).
This harmless function will allow them to use their same source code with all core versions. The way to achieve this is explained in #5948.

edit #2418 cannot be merged, github doesn't show the merge neither the update button probably because the PR is too old.