Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong SPI transfer in 16-bit mode on STM32F7xx #3445

Closed
kristofmulier opened this issue Dec 14, 2016 · 7 comments
Closed

Wrong SPI transfer in 16-bit mode on STM32F7xx #3445

kristofmulier opened this issue Dec 14, 2016 · 7 comments

Comments

@kristofmulier
Copy link

Description

  • Type: Bug
  • Priority: Blocker

Bug

Target
STM32F7xx

Toolchain:
GCC_ARM

mbed-cli version:
0.9.10

Expected behavior
The bug shows up when you try to send a single 16-bit 'word' over an SPI link which is configured in 16-bit mode:

    // Configure the SPI in 16-bit mode
    SPI spi(PA_7, PA_6, PA_5);
    DigitalOut cs(PD_14);
    cs = 1;
    
    // Send regularly a single 16-bit 'word' over the link
    while(true)
    {
        Thread::wait(40);
        
        cs = 0;
        int response = spi.write(0x8FA3);    // Transfer one single 16-bit 'word'
        cs = 1;
    }

This is what you would expect to see on the oscilloscope:

Actual behavior
Unfortunately, the oscilloscope shows that after outputting the intended 16-bit word, the chip outputs a second one (equal to 0x0000) that should not be there:

Steps to reproduce
To reproduce the error, your main.cpp file should be the following code:

#include "mbed.h"
DigitalOut led1(LED1);

int main()
{
    Serial logger(USBTX, USBRX);
    logger.baud(115200);
    logger.printf("Test application started\r\n");

    SPI spi(PA_7, PA_6, PA_5);
    DigitalOut cs(PD_14);
    cs = 1;
    
    spi.format(16, 0);         // 16-bit mode, pol = 0, pha = 0
    spi.frequency(2000000);    // 2 Mhz SPI speed


    int i = 0;
    while (true) {
        led1 = !led1;
        Thread::wait(40);
        
        cs = 0;
        int response = spi.write(0x8FA3);    // Transfer one single 16-bit 'word'
        cs = 1;
    }
}

Solution

I have traced the bug back to the file stm_spi_api.c . The function spi_master_write(..) is called to transfer the value. In turn, this function calls the ST HAL function HAL_SPI_TransmitReceive(..). One of the parameters handed over is << size >>.
Mbed-OS wrongly assumed that this parameter represents the "number of bytes" to be transmitted over the SPI link. However, this parameter actually represents the "number of words" to be transmitted. A single 16-bit value is just one "word" - assuming that your SPI is configured in 16-bit mode.

The function spi_master_write(..) should therefore be changed:

int spi_master_write(spi_t *obj, int value)
{
    uint16_t size, Rx, ret;
    struct spi_s *spiobj = SPI_S(obj);
    SPI_HandleTypeDef *handle = &(spiobj->handle);

    // ERROR: The << size >> parameter in the ST HAL_SPI_TransmitReceive(..)
    //        function does not represent the nr. of bytes but rather the
    //        number of words (a word is 1 byte in 8-bit mode and 2 bytes in
    //        16-bit mode).

    // size = (handle->Init.DataSize == SPI_DATASIZE_16BIT) ? 2 : 1;
    size = 1;

    /*  Use 10ms timeout */
    ret = HAL_SPI_TransmitReceive(handle,(uint8_t*)&value,(uint8_t*)&Rx,size,10);

    if(ret == HAL_OK) {
        return Rx;
    } else {
        DEBUG_PRINTF("SPI inst=0x%8X ERROR in write\r\n", (int)handle->Instance);
        return -1;
    }
}

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 14, 2016

Thanks for the detailed report !

cc @bcostm @LMESTM @jeromecoutant @adustm

@adustm
Copy link
Member

adustm commented May 5, 2017

Hello @kristofmulier
I agree with your remark.
@LMESTM do you also agree that we should not change the size param of HAL_SPI_TransmitReceive pending on the datalength ?

@LMESTM
Copy link
Contributor

LMESTM commented May 9, 2017

@adustm - yes I agree
@kristofmulier - thanks for reporting the issue - would you feel like sending a PR with your proposed fix ?

@LMESTM
Copy link
Contributor

LMESTM commented Jun 1, 2017

@kristofmulier @adustm @0xc0170 this fix is being applied also as part of #4375

@LMESTM
Copy link
Contributor

LMESTM commented Jun 13, 2017

As explained above this has been fixed thru #4375
ST_TO_BE_CLOSED

@0xc0170 0xc0170 closed this as completed Jun 14, 2017
@kristofmulier
Copy link
Author

Hi @LMESTM @adustm @0xc0170 ,
Sorry for my late reply. So the issue is fixed in the current release?
Thank you very much :-)

@LMESTM
Copy link
Contributor

LMESTM commented Jun 14, 2017

@kristofmulier welcome. I haven't checked the official release where it will land but it's merged on master now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants