SPI.beginTransaction does not set SCK value until the first write

[and3rson]

Board

ESP32-C3

Device Description

No hardware

Hardware Configuration

SCK = 0
MISO = 8
MOSI = 1

Version

latest master (checkout manually)

IDE Name

n/a

Operating System

n/a

Flash frequency

40MHz

PSRAM enabled

no

Upload speed

n/a

Description

SPI clock polarity is not affected by beginTransaction until the first write. This leads to slave device ignoring some SPI packets.

This has affected my TFT display, since clock polarity was not being changed before activating the display until the first command was sent.
The only solution was to write a dummy byte directly after beginTransaction.
Related issue: moononournation/Arduino_GFX#433

I'm not sure if this is the expected behavior, but I would certainly expect beginTransaction to immediately update SCK polarity that's appropriate for the selected SPI mode.

Sketch

    SPI.beginTransaction(SPISettings(..., ..., SPI_MODE0)); // Set SPI mode to 0 (CPOL=0)
    SPI.write(0xFF);
    SPI.endTransaction();
    // SCK is now LOW (SPI mode 0)

    SPI.beginTransaction(SPISettings(..., ..., SPI_MODE3)); // Set SPI mode to 3 (CPOL=1)
    // Problem: SCK is STILL LOW (SPI despite being in mode 3).
    // If we assert device's SS signal, it will enter incosistent state
    // due to SCK being low, effectively missing the first bit of the message.
    SPI.endTransaction();
    // SCK is still LOW...

Debug Message

n/a

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[and3rson]

Here's a complete example to reproduce this:

#include <Arduino.h>
#include <SPI.h>

#define DEV_SCK 0
#define DEV_MISO 8
#define DEV_MOSI 1
#define DEV_CS 5

#define SPI_SPEED 100000 // 100 KHz


void setup() {
    SPI.begin(DEV_SCK, DEV_MISO, DEV_MOSI);
    pinMode(DEV_CS, OUTPUT);
    digitalWrite(5, HIGH);

delay(1000); // all delays are unindented for readability

    SPI.beginTransaction(SPISettings(SPI_SPEED, MSBFIRST, SPI_MODE0));
delayMicroseconds(10);
    digitalWrite(DEV_CS, LOW);
delayMicroseconds(10);
    SPI.write(0xAA);
delayMicroseconds(10);
    digitalWrite(DEV_CS, HIGH);
delayMicroseconds(10);
    SPI.endTransaction();

delayMicroseconds(10);

    SPI.beginTransaction(SPISettings(SPI_SPEED, MSBFIRST, SPI_MODE3));
delayMicroseconds(10);
    digitalWrite(DEV_CS, LOW);
delayMicroseconds(10);
    SPI.write(0xAA);
delayMicroseconds(10);
    digitalWrite(DEV_CS, HIGH);
delayMicroseconds(10);
    SPI.endTransaction();
}

void loop() {}

Actual results:

(Area with wrong SCK level is highlighted)

[and3rson]

Possible workaround is to use hardware CS:

    SPI.begin(SD_SCK, SD_MISO, SD_MOSI, DEV_CS);
    SPI.setHwCs(true);

This way, SPI.beginTransaction properly asserts CS after clock polarity changes:

However, this makes it impossible to have multiple devices share the bus.

[SuGlider]

@me-no-dev - Please take a look. Thanks!

[P-R-O-C-H-Y]

I am taking a look what we can do about it :)

[and3rson]

FYI - the workaround that we applied to https://github.com/moononournation/Arduino_GFX was to write a dummy byte after beginTransaction (as outlined here).

Though I think there might be a better solution (I'm not too familiar with how ESP32 handles SPI under the hood).

[and3rson]

FWIW, the easiest solution for this issue might be to write a dummy byte within the AcquireSPI guard, in

arduino-esp32/libraries/SD/src/sd_diskio.cpp

Line 468 in aed7b4f

struct AcquireSPI

    explicit AcquireSPI(ardu_sdcard_t* card)
        : card(card)
    {
        card->spi->beginTransaction(SPISettings(card->frequency, MSBFIRST, SPI_MODE0));
        card->spi->write(0xFF); // dummy write to force correct SCK polarity
    }
    AcquireSPI(ardu_sdcard_t* card, int frequency)
        : card(card)
    {
        card->spi->beginTransaction(SPISettings(frequency, MSBFIRST, SPI_MODE0));
        card->spi->write(0xFF); // dummy write to force correct SCK polarity
    }
}

...however this still feels like a workaround. So at this point, I'm wondering about a more generic question:

Should beginTransaction be the one to take care of "forcing" SCK to update its polarity?

EDIT: I think it might be possible to force SPI clock to settle in correct polarity by manipulating spi->dev->cmd.usr or spi->dev->cmd.update within spiTransaction.

arduino-esp32/cores/esp32/esp32-hal-spi.c

Line 1069 in aed7b4f

void spiTransaction(spi_t * spi, uint32_t clockDiv, uint8_t dataMode, uint8_t bitOrder)

But I know too little about their behavior and I'm just spitballing at this point...

[and3rson]

Turns out my wild guess worked. I just did a quick test and added this code at the end of spiTransaction in esp32-hal-spi.c:

#if CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3
    spi->dev->cmd.update = 1;
    while (spi->dev->cmd.update);
#endif

...and it fixed the clock polarity. My assumption is that setting update triggers the underlying SPI hardware to update its physical state.

Before fix:

After fix:

Is this an acceptable fix? I can submit a PR @P-R-O-C-H-Y.

[Jason2866]

@and3rson Does the faulty behaviour affects only the C3 and S3?
The PR you provide to fix is applied to this two MCU variants only.

[and3rson]

@Jason2866 Those were the only ones I have and thus was able to test... Not sure about other models. Additionally, update field in soc/spi_struct.h seems to be present only in C3 & S3, hence the #ifdef.

[Jason2866]

So there should be more tests done, if the other MCUs does have this issue too.

[P-R-O-C-H-Y]

I will test all chips tomorrow. Also this PR may also help with troubles I had adding multiple CS pins support.