STM32F4 SPI 3-wire mode doesn't work correctly

[vznncv]

Description of defect

Method int SPI::write(const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length) doesn't work correctly with SPI 3-wire mode.

Actual behavior:

First byte of the rx_buffer after int SPI::write(const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length) method invocation contains last byte of the previous transaction, when SPI is configured in the 3-wire mode. Actual bytes are shifted to next positions. The last byte are "moved" to the next read transaction.

Expected behavior:

rx_buffer contains bytes only from current transaction.

Target(s) affected by this defect ?

STM32F411CEU6 (custom board https://github.com/vznncv/TARGET_BLACKPILL_F411CE)

(probably all STM32F4 boards)

Toolchain(s) (name and version) displaying this defect ?

arm-none-eabi-gcc (GNU Arm Embedded Toolchain 9-2020-q2-update) 9.3.1 20200408 (release)

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.11.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli v1.10.5

How is this defect reproduced ?

Example of BMX160 sensor usage in 3-wire mode with STM32F411CEU6:

/**
 * SPI 3 Wire demo with BMX160 sensor.
 *
 * sensor: https://wiki.dfrobot.com/BMX160_9_Axis_Sensor_Module_SKU_SEN0373
 * board: https://github.com/vznncv/TARGET_BLACKPILL_F411CE/tree/v0.1.4
 * mbed-os version: 6.11.0
 */

#include "mbed.h"

#include "stm32f4xx_hal.h"

//
// Hardware pins
//
#define BMX160_SDX PB_15 // SPI MOSI pin (STM32 SPI2 instance)
#define BMX160_SCX PB_13 // SPI CLK pin (STM32 SPI2 instance)
#define BMX160_CSB PA_8 // SPI SSEL pin (STM32 SPI2 instance)

// enable/disable SPI 3 wire rx buffer cleanup
#define ENABLE_SPI_3WIRE_FIX 0

// helper macro to check return code
static char app_error_msg_buf[256];
#define CHECK_RET_CODE(expr) do {                                                                         \
    int err = expr;                                                                                       \
    if (err != 0) {                                                                                       \
        snprintf(app_error_msg_buf, 256, "expression \"%s\" has returned error code %i", #expr, err);     \
        MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_APPLICATION, MBED_ERROR_CODE_UNKNOWN), app_error_msg_buf); \
    }                                                                                                     \
} while(0);

//
// hardware interfaces
//
DigitalOut user_led(LED1, 1);
SPI bmx_spi(BMX160_SDX, NC, BMX160_SCX);
DigitalOut bmx_ssel(BMX160_CSB, NC);

/**
 * Write to BMX160 register.
 *
 * @param reg register address
 * @param value register value
 * @return 0 on success, otherwise non-zero value
 */
int bmx_register_write(uint8_t reg, uint8_t value)
{
    bmx_ssel.write(0);

    uint8_t out_data[2] = {reg, value};
    bmx_spi.write((char *)out_data, 2, nullptr, 1);

    bmx_ssel.write(1);

    // log results
    printf("| write to  bmx160 register 0x%02X | -> 0x%02X |\n", reg, value);

    return 0;
}

/**
 * Read from BMX160 register.
 *
 * @param reg register address
 * @param value register value
 * @return 0 on success, otherwise non-zero value
 */
int bmx_register_read(uint8_t reg, uint8_t *value)
{
#if ENABLE_SPI_3WIRE_FIX
    // cleanup SPI input buffer explicitly
    SPI_TypeDef *spi = (SPI_TypeDef *)SPI2_BASE;
    volatile uint32_t data;
    if (spi->SR & SPI_SR_RXNE) {
        printf("| 1. SPI RX buffer isn't empty. Clear it   |\n");
        data = spi->DR;
    }
    if (spi->SR & SPI_SR_RXNE) {
        printf("| 2. SPI RX buffer isn't empty. Clear it   |\n");
        data = spi->DR;
    }
    UNUSED(data);
#endif

    bmx_ssel.write(0);

    uint8_t out_data[1] = {(uint8_t)(reg | 0x80)};
    uint8_t in_data[1];
    bmx_spi.write((char *)out_data, 1, (char *)in_data, 1);
    *value = in_data[0];

    bmx_ssel.write(1);

    // log results
    printf("| read from bmx160 register 0x%02X | <- 0x%02X |\n", reg, *value);

    return 0;
}

int main()
{
    uint8_t reg_value;

    // initialize drivers
    printf("====== Init ======\n");
    // configure spi mode
    bmx_spi.format(8, 3);

    // startup delay for BMX160 sensor
    ThisThread::sleep_for(100ms);

    // create rising edge on the SSEL pin to switch BMX160 to SPI mode
    bmx_ssel.write(0);
    ThisThread::sleep_for(1ms);
    bmx_ssel.write(1);
    ThisThread::sleep_for(1ms);

    // write to IF_CONF register to active 3-wire SPI interface
    CHECK_RET_CODE(bmx_register_write(0x6B, 0x01));

    // read accelerometer X axis offset register value (default value must be 0)
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x71, &reg_value));
    }
    // read IF_CONF register. It should contain 0x01, that we have set before
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x6B, &reg_value));
    }
    // read CHIP_ID register. It should contain D8.
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x00, &reg_value));
    }
    // read accelerometer X axis offset register value (default value must be 0)
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x71, &reg_value));
    }


    printf("====== Blink demo ======\n");
    while (true) {
        ThisThread::sleep_for(500ms);
        user_led = !user_led;
    }

    return 0;
}

Output (first read register attempt returns incorrect value):

====== Init ======
| write to  bmx160 register 0x6B | -> 0x01 |
| read from bmx160 register 0x71 | <- 0x00 |
| read from bmx160 register 0x71 | <- 0x00 |
| read from bmx160 register 0x71 | <- 0x00 |
| read from bmx160 register 0x6B | <- 0x00 |
| read from bmx160 register 0x6B | <- 0x01 |
| read from bmx160 register 0x6B | <- 0x01 |
| read from bmx160 register 0x00 | <- 0x01 |
| read from bmx160 register 0x00 | <- 0xD8 |
| read from bmx160 register 0x00 | <- 0xD8 |
| read from bmx160 register 0x71 | <- 0xD8 |
| read from bmx160 register 0x71 | <- 0x00 |
| read from bmx160 register 0x71 | <- 0x00 |
====== Blink demo ======

Example of BMX160 sensor usage in 3-wire mode with STM32F411CEU6 with explicit RX buffer cleanup of SPI2 that fixes problem:

/**
 * SPI 3 Wire demo with BMX160 sensor.
 *
 * sensor: https://wiki.dfrobot.com/BMX160_9_Axis_Sensor_Module_SKU_SEN0373
 * board: https://github.com/vznncv/TARGET_BLACKPILL_F411CE/tree/v0.1.4
 * mbed-os version: 6.11.0
 */

#include "mbed.h"

#include "stm32f4xx_hal.h"

//
// Hardware pins
//
#define BMX160_SDX PB_15 // SPI MOSI pin (STM32 SPI2 instance)
#define BMX160_SCX PB_13 // SPI CLK pin (STM32 SPI2 instance)
#define BMX160_CSB PA_8 // SPI SSEL pin (STM32 SPI2 instance)

// enable/disable SPI 3 wire rx buffer cleanup
#define ENABLE_SPI_3WIRE_FIX 1

// helper macro to check return code
static char app_error_msg_buf[256];
#define CHECK_RET_CODE(expr) do {                                                                         \
    int err = expr;                                                                                       \
    if (err != 0) {                                                                                       \
        snprintf(app_error_msg_buf, 256, "expression \"%s\" has returned error code %i", #expr, err);     \
        MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_APPLICATION, MBED_ERROR_CODE_UNKNOWN), app_error_msg_buf); \
    }                                                                                                     \
} while(0);

//
// hardware interfaces
//
DigitalOut user_led(LED1, 1);
SPI bmx_spi(BMX160_SDX, NC, BMX160_SCX);
DigitalOut bmx_ssel(BMX160_CSB, NC);

/**
 * Write to BMX160 register.
 *
 * @param reg register address
 * @param value register value
 * @return 0 on success, otherwise non-zero value
 */
int bmx_register_write(uint8_t reg, uint8_t value)
{
    bmx_ssel.write(0);

    uint8_t out_data[2] = {reg, value};
    bmx_spi.write((char *)out_data, 2, nullptr, 1);

    bmx_ssel.write(1);

    // log results
    printf("| write to  bmx160 register 0x%02X | -> 0x%02X |\n", reg, value);

    return 0;
}

/**
 * Read from BMX160 register.
 *
 * @param reg register address
 * @param value register value
 * @return 0 on success, otherwise non-zero value
 */
int bmx_register_read(uint8_t reg, uint8_t *value)
{
#if ENABLE_SPI_3WIRE_FIX
    // cleanup SPI input buffer explicitly
    SPI_TypeDef *spi = (SPI_TypeDef *)SPI2_BASE;
    volatile uint32_t data;
    if (spi->SR & SPI_SR_RXNE) {
        printf("| 1. SPI RX buffer isn't empty. Clear it   |\n");
        data = spi->DR;
    }
    if (spi->SR & SPI_SR_RXNE) {
        printf("| 2. SPI RX buffer isn't empty. Clear it   |\n");
        data = spi->DR;
    }
    UNUSED(data);
#endif

    bmx_ssel.write(0);

    uint8_t out_data[1] = {(uint8_t)(reg | 0x80)};
    uint8_t in_data[1];
    bmx_spi.write((char *)out_data, 1, (char *)in_data, 1);
    *value = in_data[0];

    bmx_ssel.write(1);

    // log results
    printf("| read from bmx160 register 0x%02X | <- 0x%02X |\n", reg, *value);

    return 0;
}

int main()
{
    uint8_t reg_value;

    // initialize drivers
    printf("====== Init ======\n");
    // configure spi mode
    bmx_spi.format(8, 3);

    // startup delay for BMX160 sensor
    ThisThread::sleep_for(100ms);

    // create rising edge on the SSEL pin to switch BMX160 to SPI mode
    bmx_ssel.write(0);
    ThisThread::sleep_for(1ms);
    bmx_ssel.write(1);
    ThisThread::sleep_for(1ms);

    // write to IF_CONF register to active 3-wire SPI interface
    CHECK_RET_CODE(bmx_register_write(0x6B, 0x01));

    // read accelerometer X axis offset register value (default value must be 0)
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x71, &reg_value));
    }
    // read IF_CONF register. It should contain 0x01, that we have set before
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x6B, &reg_value));
    }
    // read CHIP_ID register. It should contain D8.
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x00, &reg_value));
    }
    // read accelerometer X axis offset register value (default value must be 0)
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x71, &reg_value));
    }


    printf("====== Blink demo ======\n");
    while (true) {
        ThisThread::sleep_for(500ms);
        user_led = !user_led;
    }

    return 0;
}

Output (all register values are corrected):

====== Init ======
| write to  bmx160 register 0x6B | -> 0x01 |
| read from bmx160 register 0x71 | <- 0x00 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x71 | <- 0x00 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x71 | <- 0x00 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x6B | <- 0x01 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x6B | <- 0x01 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x6B | <- 0x01 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x00 | <- 0xD8 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x00 | <- 0xD8 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x00 | <- 0xD8 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x71 | <- 0x00 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x71 | <- 0x00 |
| 1. SPI RX buffer isn't empty. Clear it   |
| read from bmx160 register 0x71 | <- 0x00 |
====== Blink demo ======

[0xc0170]

@ARMmbed/team-st-mcd Please review

[LMESTM]

Hi, sorry for slow answer.

The 3 wires support has been added and tested with ST sensor below:
https://os.mbed.com/teams/ST-Expansion-SW-Team/code/X_NUCLEO_IKS01A2_SPI3W/
It was tested to work okay so we not sure what would have happened since then.

Discussing with the team who wrote this sensor driver, it seems you're using SPI3W to interface the Bosch Sensortec sensor which, maybe, differs from our ST sensors on SPI3W behavior. Maybe the SPI3W protocol can be slightly different from vendor to vendor. In our case, if sensor team remembers well, the component, before returning the data, sends back the register address. Could that make a difference ?

Also a quick look at the datasheet mentions: For single byte read as well as write operations, 16-bit protocols are used.
So in your test sequence, after the 1st write to 0x6B, shouldn't you read as well ?
| write to bmx160 register 0x6B | -> 0x01 |

Not sure if that would solve the issue but my point is that I wonder if this may be due to protocol difference ?

[vznncv]

Hi

Maybe the SPI3W protocol can be slightly different from vendor to vendor.

3-Wire SPI must be same across all vendors like 4-Wire SPI, although high-level device protocols
(consequence of read/write operations to accesses device registers) may be different even across devices
of the same vendor. But SPI interface shouldn't care about high-level device protocols. It's task of the driver that
uses SPI interface.

So in your test sequence, after the 1st write to 0x6B, shouldn't you read as well ?

No, 16-bit protocol means that you send byte with register address and then read or write register value. For write operations it includes sending of 2 bytes.

extra issue investigation

I got an oscilloscope and captured SPI frames with sigrock to get extra clarification about this issue.

Test programs:

/**
 * SPI 3 Wire demo with BMX160 sensor.
 *
 * sensor: https://wiki.dfrobot.com/BMX160_9_Axis_Sensor_Module_SKU_SEN0373
 * board: https://github.com/vznncv/TARGET_BLACKPILL_F411CE/tree/v0.1.4
 * mbed-os version: 6.11.0
 */

#include "mbed.h"

#include "stm32f4xx_hal.h"

//
// Hardware pins
//
#define BMX160_SDX PB_15 // SPI MOSI pin (STM32 SPI2 instance)
#define BMX160_SCX PB_13 // SPI CLK pin (STM32 SPI2 instance)
#define BMX160_CSB PA_8 // SPI SSEL pin (STM32 SPI2 instance)

// helper macro to check return code
static char app_error_msg_buf[256];
#define CHECK_RET_CODE(expr) do {                                                                         \
    int err = expr;                                                                                       \
    if (err != 0) {                                                                                       \
        snprintf(app_error_msg_buf, 256, "expression \"%s\" has returned error code %i", #expr, err);     \
        MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_APPLICATION, MBED_ERROR_CODE_UNKNOWN), app_error_msg_buf); \
    }                                                                                                     \
} while(0);

//
// hardware interfaces
//
DigitalOut user_led(LED1, 1);
SPI bmx_spi(BMX160_SDX, NC, BMX160_SCX);
DigitalOut bmx_ssel(BMX160_CSB, NC);

// helper variables
int transaction_count = 0;

/**
 * Write to BMX160 register.
 *
 * @param reg register address
 * @param value register value
 * @return 0 on success, otherwise non-zero value
 */
int bmx_register_write(uint8_t reg, uint8_t value)
{
    bmx_ssel.write(0);

    uint8_t out_data[2] = {reg, value};
    bmx_spi.write((char *)out_data, 2, nullptr, 1);

    bmx_ssel.write(1);

    // log results
    printf("| %02i | write to  bmx160 register 0x%02X | -> 0x%02X |\n", transaction_count, reg, value);
    transaction_count++;

    return 0;
}

/**
 * Read from BMX160 register.
 *
 * @param reg register address
 * @param value register value
 * @return 0 on success, otherwise non-zero value
 */
int bmx_register_read(uint8_t reg, uint8_t *value)
{
    bmx_ssel.write(0);

    uint8_t out_data[1] = {(uint8_t)(reg | 0x80)};
    uint8_t in_data[1];
    bmx_spi.write((char *)out_data, 1, (char *)in_data, 1);

    *value = in_data[0];

    bmx_ssel.write(1);

    // log results
    printf("| %02i | read from bmx160 register 0x%02X | <- 0x%02X |\n", transaction_count, reg, *value);
    transaction_count++;

    return 0;
}

int main()
{
    uint8_t reg_value;

    // initialize drivers
    printf("===================== Init ======================\n");

    // configure spi mode
    bmx_spi.format(8, 3);

    // startup delay for BMX160 sensor
    ThisThread::sleep_for(100ms);

    // create rising edge on the SSEL pin to switch BMX160 to SPI mode
    bmx_ssel.write(0);
    ThisThread::sleep_for(1ms);
    bmx_ssel.write(1);
    ThisThread::sleep_for(1ms);

    // write to IF_CONF register to active 3-wire SPI interface
    CHECK_RET_CODE(bmx_register_write(0x6B, 0x01));

    // read CHIP_ID register. It should contain D8.
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x00, &reg_value));
    }
    // read IF_CONF register. It should contain 0x01, that we have set before
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x6B, &reg_value));
    }
    // read CHIP_ID register. It should contain D8.
    for (int i = 0; i < 3; i++) {
        CHECK_RET_CODE(bmx_register_read(0x00, &reg_value));
    }
    
    printf("================== Blink demo ===================\n");
    while (true) {
        ThisThread::sleep_for(500ms);
        user_led = !user_led;
    }

    return 0;
}

UART output:

===================== Init ======================
| 00 | write to  bmx160 register 0x6B | -> 0x01 |
| 01 | read from bmx160 register 0x00 | <- 0xD8 |
| 02 | read from bmx160 register 0x00 | <- 0x21 |
| 03 | read from bmx160 register 0x00 | <- 0xD8 |
| 04 | read from bmx160 register 0x6B | <- 0xD8 |
| 05 | read from bmx160 register 0x6B | <- 0x01 |
| 06 | read from bmx160 register 0x6B | <- 0x01 |
| 07 | read from bmx160 register 0x00 | <- 0x01 |
| 08 | read from bmx160 register 0x00 | <- 0xD8 |
| 09 | read from bmx160 register 0x00 | <- 0xD8 |
================== Blink demo ===================

Logical analyzer data:

sigrok_data.zip

Summary table:

transaction number	transaction description	expected logical analyzer data	actual logical analyzer data	expected data from SPI::write method	actual data from SPI::write method	comment
0	transmit 0x6B 0x01 bytes to activate 3-Wire mode	transmit 0x6B 0x01	transmit 0x6B 0x01	N/A	N/A	transaction is correct
1	transmit 0x80 (register address with read mode) and read byte with device ID	transmit 0x80, receive 0xD8	transmit 0x80, receive 0xD8 0x21	0xD8	0xD8	transaction is incorrect. STM SPI generates extra clock ticks and reads rubbish data 0x21
2	transmit 0x80 (register address with read mode) and read byte with device ID	transmit 0x80, receive 0xD8	transmit 0x80, receive 0xD8	0xD8	0x21	transaction is correct, but STM SPI driver returns rubbish data from the previous transaction
3	transmit 0x80 (register address with read mode) and read byte with device ID	transmit 0x80, receive 0xD8	transmit 0x80, receive 0xD8	0xD8	0xD8	transaction is correct
4	transmit 0xEB (register address with read mode) and read byte with SPI configuration	transmit 0xEB, receive 0x01	transmit 0xEB, receive 0x01	0x01	0xD8	transaction is correct, but STM SPI driver returns data from the previous transaction
5	transmit 0xEB (register address with read mode) and read byte with SPI configuration	transmit 0xEB, receive 0x01	transmit 0xEB, receive 0x01	0x01	0x01	transaction is correct
6	transmit 0xEB (register address with read mode) and read byte with SPI configuration	transmit 0xEB, receive 0x01	transmit 0xEB, receive 0x01	0x01	0x01	transaction is correct
7	transmit 0x80 (register address with read mode) and read byte with device ID	transmit 0x80, receive 0xD8	transmit 0x80, receive 0xD8	0xD8	0x01	transaction is correct, but STM SPI driver returns data from the previous transaction
9	transmit 0x80 (register address with read mode) and read byte with device ID	transmit 0x80, receive 0xD8	transmit 0x80, receive 0xD8	0xD8	0xD8	transaction is correct
9	transmit 0x80 (register address with read mode) and read byte with device ID	transmit 0x80, receive 0xD8	transmit 0x80, receive 0xD8	0xD8	0xD8	transaction is correct

Results

After investigation of:

• source code of STM32 HAL_SPI_Receive function that is responsible for data receiving in 3-Wire SPI mode
• STM32F4 reference manual
• extra STM Community posts:
  • https://community.st.com/s/question/0D50X0000B5Izgx/why-the-halspireceive-generate-more-clocks-then-required
  • https://electronics.stackexchange.com/questions/413247/spi-clocking-in-extra-word-stm32f4
  • https://community.st.com/s/question/0D50X00009Xkdpc/bug-in-halspireceive
  • https://community.st.com/s/question/0D50X00009Xke8e/bug-halspireceive-stm32f0-cubef0

I got the following results:

According documentation in "Bidirectional receive procedure" SPI starts generating clock signal continuously after SPI enabling
(see 20.3.5. Data transmission and reception procedures in the reference manual), even if data isn't read from DR register.
To stop bytes receiving you need to:

• Wait for the second to last occurrence of RXNE=1 (n–1)
• Then wait for one SPI clock cycle (using a software loop) before disabling the SPI (SPE=0)
• Then wait for the last RXNE=1 before entering the Halt mode (or disabling the peripheral clock)

(see 20.3.8. Disabling the SPI in the reference manual)

Whereas the code of HAL_SPI_Receive simply receives all n bytes and stop SPI. It causes that with some chance SPI may
start generating clock signal for n+1 byte.

So correct implementation of the HAL_SPI_Receive requires strict timings between (n-1) byte receiving and SPI disabling (especially with high SPI frequency). It implies the following limitations:

• all interrupts must be disabled during SPI data receiving to eliminate any delays
• the code gather data from SPI DR register should be short and fast to eliminate delays between (n-1) byte reading and SPI disabling. It may be difficult to create such code that will work with different optimization levels of compiler and different MCUs.
• additionally debugging may cause delays, incorrect driver behavior and strange bugs

Summary

Current STM 3-Wire SPI hardware implementation doesn't provide reliable way to read bytes without dummy reads (or at least I don't find such approaches). So I think that it's better to drop support 3-Wire SPI from MbedOS as it has buggy implementation, or at least add some warning and cautions to documentation/code that STM 3-Wire SPI isn't able to read data reliable.

@LMESTM What do you think about it?

[LMESTM]

@vznncv thanks a lot for this detailed analysis and explanation.
What I think is that I will need to get feedback internally from experts to provide a definitive feedback.
I'll try to come back to you asap !

[LMESTM]

One more point: does that mean that your issue may be "solved" (worked-around) by requesting a slow SPI frequency ?

[LMESTM]

@vznncv your analysis has been confirmed. The use case is actually document in the driver code here:
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/STM32Cube_FW/STM32F4xx_HAL_Driver/stm32f4xx_hal_spi.c#L55

It means that if we want to work-around this issue in the mbed-os code, we need to add calls to
HAL_SPI_DeInit(handle);
HAL_SPI_Init(handle);

probably here: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/stm_spi_api.c#L786
and here: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/stm_spi_api.c#L864

Would you mind giving it a try ?
I admit that it would probably degrade slightly performances but that might be worth it if this stabilize the transfers.

[vznncv]

@LMESTM

I have modified example to demonstrate burst read, as I noted that it's less stable:
bmx_3wire_error.cpp

One more point: does that mean that your issue may be "solved" (worked-around) by requesting a slow SPI frequency ?

Yes, it helps.

Demo uart output: demo_200KHz_uart_output.txt
Logical analyzer data: demo_200KHz_sigrock.zip

But this solution has the following problems:

• I achieved stable communication only at 200 KHz. It's slower that I2C fast mode.
• If any interrupt occurs during communication (OS scheduler, ticker, etc.), I get invalid results.

It means that if we want to work-around this issue in the mbed-os code, we need to add calls to
HAL_SPI_DeInit(handle);
HAL_SPI_Init(handle);
probably here: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/stm_spi_api.c#L786
and here: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/stm_spi_api.c#L864
Would you mind giving it a try ?

I checked it. It doesn't help.

Demo uart output: hal_reinit_uart_output.txt
Logical analyzer data: hal_reinit_sigrok.zip

Extra investigation and workaround

After some experiments I found that timing between last byte receiving and SPI disabling is crucial. According
reference manual ("20.3.8 Disabling the SPI") it shouldn't be too small:

In master unidirectional receive-only mode (MSTR=1, BIDIMODE=0, RXONLY=1) or bidirectional receive mode (MSTR=1, BIDIMODE=1, BIDIOE=0)
This case must be managed in a particular way to ensure that the SPI does not initiate a new transfer. The sequence below is valid only for SPI Motorola configuration (FRF bit set to
0):
1. Wait for the second to last occurrence of RXNE=1 (n–1)
2. Then wait for one SPI clock cycle (using a software loop) before disabling the SPI (SPE=0)
3. Then wait for the last RXNE=1 before entering the Halt mode (or disabling the peripheral clock)

and it should be too high (otherwise next byte receiving is started).

So the following data receiving approach should work:

1. Configure SPI bidirectional receive mode.
2. Enter into critical section (disable all interrupts to eliminate any interrupt delays)
3. Enable SPI.
4. Wait one SPI clock cycle with software loop.
5. Disable SPI.
6. Exit from critical section.
7. Wait full byte receiving.
8. Read data for DR register.
9. If extra bytes should be read, then go to step 2.
10. All bytes are received.

The code that implements such approach (I have added it to stm_spi_api.c):

// extra includes
#include "mbed_critical.h"
#include "mbed_wait_api.h"

//
// other functions ...
//

/**
 * Receive SPI data in bidirectional receive mode.
 *
 * @param obj
 * @param rx_buffer
 * @param rx_length
 * @return number of received bytes on success, otherwise negative code
 */
/**
 * Receive SPI data in bidirectional receive mode.
 *
 * @param obj
 * @param rx_buffer
 * @param rx_length
 * @return number of received bytes on success, otherwise negative code
 */
static int stm32f4_spi_master_one_wire_receive(spi_t *obj, char *rx_buffer, int rx_length)
{
    struct spi_s *spiobj = SPI_S(obj);
    SPI_HandleTypeDef *hspi = &(spiobj->handle);
    uint32_t rx_data;

    const int bitshift = datasize_to_transfer_bitshift(hspi->Init.DataSize);
    MBED_ASSERT(bitshift >= 0);
    // get estimation about one SPI clock cycle
    uint8_t spi_prescaler_pow = 1 + (READ_BIT(hspi->Instance->CR1, SPI_CR1_BR_Msk) >> SPI_CR1_BR_Pos);
    uint32_t delay_ns = 1000000000 / (spi_get_clock_freq(obj) >> spi_prescaler_pow);

    /* configure SPI direction */
    __HAL_SPI_DISABLE(hspi);
    SPI_1LINE_RX(hspi);

    for (int i = 0; i < rx_length; i++) {
        core_util_critical_section_enter();
        __HAL_SPI_ENABLE(hspi);
        /* wait single SPI clock cycle */
        wait_ns(delay_ns);
        __HAL_SPI_DISABLE(hspi);
        core_util_critical_section_exit();

        /* Wait RXE flag to transmit data */
        while (!LL_SPI_IsActiveFlag_RXNE(SPI_INST(obj)));
        /* Read bytes */
        /* Read received data */
        if (bitshift == 1) {
            rx_data = LL_SPI_ReceiveData16(SPI_INST(obj));
#ifdef HAS_32BIT_SPI_TRANSFERS
        } else if (bitshift == 2) {
            rx_data = LL_SPI_ReceiveData32(SPI_INST(obj));
#endif
        } else {
            rx_data = LL_SPI_ReceiveData8(SPI_INST(obj));
        }
        rx_buffer[i] = rx_data;
    }

    return rx_length;
}

int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length,
                           char *rx_buffer, int rx_length, char write_fill)
{
    struct spi_s *spiobj = SPI_S(obj);
    SPI_HandleTypeDef *handle = &(spiobj->handle);
    int total = (tx_length > rx_length) ? tx_length : rx_length;
    if (handle->Init.Direction == SPI_DIRECTION_2LINES) {
        for (int i = 0; i < total; i++) {
            char out = (i < tx_length) ? tx_buffer[i] : write_fill;
            char in = spi_master_write(obj, out);
            if (i < rx_length) {
                rx_buffer[i] = in;
            }
        }
    } else {
        /* In case of 1 WIRE only, first handle TX, then Rx */
        if (tx_length != 0) {
            if (HAL_OK != HAL_SPI_Transmit(handle, (uint8_t *)tx_buffer, tx_length, tx_length * TIMEOUT_1_BYTE)) {
                /*  report an error */
                total = 0;
            }
        }
        if (rx_length != 0) {
//            if (HAL_OK != HAL_SPI_Receive(handle, (uint8_t *)rx_buffer, rx_length, rx_length * TIMEOUT_1_BYTE)) {
//                /*  report an error */
//                total = 0;
//            }
            if (stm32f4_spi_master_one_wire_receive(obj, rx_buffer, rx_length) < 0) {
                /*  report an error */
                total = 0;
            }
        }
    }

    return total;
}

//
// other functions ...
//

Such modification works correctly.

Demo uart output: fix_1Mhz_uart_output.txt
Logical analyzer data: fix_1MHz_sigrok.zip

It even works with higher SPI frequency (12.5 Mhz): fix_12_5Mhz_uart_output.txt

(My oscilloscope cannot capture data at this frequency with sigrok, but according program output, data is received correctly)

stm32f4_spi_master_one_wire_receive function notes:

• I don't enable SPI for whole transaction (if multiple bytes should be received), because in this case all interrupts should be disabled during bytes receiving. This approach adds extra overhead, but prevents critical section retention during long time (several tens-hundreds microseconds in case of large transaction).
• spi_master_write doesn't require any modification as in 3-wire mode it implements only byte transmitting.

What do you think about such approach?

[LMESTM]

Hi @vznncv

Thanks for sharing a proposed solution.
If we want to consider this kind of change, we should of course make it generic to work with all required families, not only F4.

Have you tested your proposed approach with a various number of read length ?
We could run a dedicated more detailed review in a specific Pull Request if you open one.

Of course one major concern is how we will be able to test this proposal, as we don't have 3 WIRES SPI devices in our automated regression test setup :-(

[vznncv]

Hi @LMESTM

Have you tested your proposed approach with a various number of read length ?

Yes I have tested it with 3 bytes reading (timestamp value from BMX160) and with 1 bytes reading (ordinary configuration registers).

We could run a dedicated more detailed review in a specific Pull Request if you open one.

I'll prepare a pull request in 1-2 weeks (currently I'm busy). I'll try to implement generic STM family approach.

Of course one major concern is how we will be able to test this proposal, as we don't have 3 WIRES SPI devices in our automated regression test setup :-(

How do you test ordinary (4 WIRES) SPI interface? (Do you use some sensor or something else?)

[jeromecoutant]

How do you test ordinary (4 WIRES) SPI interface? (Do you use some sensor or something else?)

In ST, we have 2 setup:

• https://github.com/ARMmbed/ci-test-shield: SD card
• https://github.com/ARMmbed/mbed-os/tree/master/hal/tests/TESTS/mbed_hal_fpga_ci_test_shield

[LMESTM]

@vznncv thanks for your efforts, that really makes sense to allow everyone to benefit from the good work you've done.

[LMESTM]

@vznncv thanks for your efforts, that really makes sense to allow everyone to benefit from the good work you've done.
(and no problem, we can wait for a few weeks, sure :-) )