spi_send incorrect working

[eurol]

MSP-EXP430FR2433, I think others too. Energia 1.6.10e18 Board library v1.0.3

SPI.transfer often skips bytes. For example if I read 512 bytes from SD card it can skip 1 or 2 bytes.

eusci_spi.cpp contains:

void spi_send(void *buf, uint16_t count)
{
    uint8_t *ptx = (uint8_t *)buf;
    uint8_t *prx = (uint8_t *)buf;
    if (count == 0) return;
    /* Wait for previous tx to complete. */
    while (!(UCzIFG & UCTXIFG));
    while(count){
        if (UCzIFG & UCRXIFG){
            /* Reading RXBUF clears the RXIFG flag. */
            *prx++ = UCzRXBUF;
        }
        if (UCzIFG & UCTXIFG){
            /* Setting TXBUF clears the TXIFG flag. */
            UCzTXBUF = *ptx++;
            count--;
        }
    }
    /* Wait for last rx character? */
    while (!(UCzIFG & UCRXIFG));
    *prx++ = UCzRXBUF;
}

I think it would be better:

1. Clear read byte after waiting of previous transmit.
2. Wait until byte is sent, then read received.

void spi_send(void *buf, uint16_t count)
{
    uint8_t *ptx = (uint8_t *)buf;
    uint8_t *prx = (uint8_t *)buf;
    if (count == 0) return;
    /* Wait for previous tx to complete. */
    while (!(UCzIFG & UCTXIFG));
//1
*prx = UCzRXBUF;
//1
    while(count){
            /* Setting TXBUF clears the TXIFG flag. */
            UCzTXBUF = *ptx++;
            count--;
//2
        while  (!(UCzIFG & UCTXIFG));
//2
        while  (!(UCzIFG & UCRXIFG)); // is it needed?
            /* Reading RXBUF clears the RXIFG flag. */
        *prx++ = UCzRXBUF;

    }
//  /* Wait for last rx character? */
//  while (!(UCzIFG & UCRXIFG));
//  *prx++ = UCzRXBUF;
}

To get better performance we can use pointers to registers calculated instead of using macros (UCzTXBUF etc). When using at higher transfer speeds it will give some percents. Since I use SPI for SD card I need functions for reading and writing so I wrote them.

void spi_sendfast(void *buf, uint16_t count)
{
    uint8_t *ptx = (uint8_t *)buf;
    volatile uint8_t *pIFG = &UCzIFG;
    volatile uint8_t *pTX = &UCzTXBUF;
    if (count == 0) return;
    while(count){
        if (*pIFG & UCTXIFG){
            /* Setting TXBUF clears the TXIFG flag. */
            *pTX = *ptx++;
            count--;
        }
    }
    /* Wait for last rx character? */
    while (!(*pIFG & UCRXIFG));
    /* clear RXIFG flag. */
    *pIFG &= ~UCRXIFG;
}

void spi_receivefast(void *buf, uint16_t count)
{
    uint8_t *prx = (uint8_t *)buf;
    volatile uint8_t *pIFG = &UCzIFG;
    volatile uint8_t *pRX = &UCzRXBUF;
    volatile uint8_t *pTX = &UCzTXBUF;

    if (count == 0) return;
    /* Wait for previous tx to complete. */
    while (!((*pIFG) & UCTXIFG));
        volatile uint8_t x = *pRX;
    while(count){
            *pTX = 0xff;
                while (!((*pIFG) & UCRXIFG));
        *prx++ = *pRX;
        count--;
        }
}

[StefanSch]

Thanks for the feedback and improved functions. Just one question: with the improvements you suggested do you get the loosing of data fixed?

[eurol]

Yes, now it is working.

Понедельник, 17 сентября 2018, 10:21 +03:00 от StefanSch notifications@github.com:

Thanks for the feedback and improved functions. Just one question: with the improvements you suggested do you get the loosing of data fixed? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub , or mute the thread .

-- Евгений Рябов

[eurol]

void spi_send(void buf, uint16_t count) {     uint8_t ptx = (uint8_t )buf;     uint8_t prx = (uint8_t )buf;     if (count == 0) return;     / Wait for previous tx to complete. /     while (!(UCzIFG & UCTXIFG));     UCzRXBUF;     while(count){         while (!(UCzIFG & UCTXIFG)); //        if (UCzIFG & UCTXIFG)         {             / Setting TXBUF clears the TXIFG flag. /             UCzTXBUF = ptx++;             count--;         }         while (!(UCzIFG & UCRXIFG)); //        if (UCzIFG & UCRXIFG)         {             / Reading RXBUF clears the RXIFG flag. /             prx++ = UCzRXBUF;         }     } //    / Wait for last rx character? / //    while (!(UCzIFG & UCRXIFG)); //    prx++ = UCzRXBUF; }

This works right now with SD card.

Понедельник, 17 сентября 2018, 10:21 +03:00 от StefanSch notifications@github.com:

Thanks for the feedback and improved functions. Just one question: with the improvements you suggested do you get the loosing of data fixed? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub , or mute the thread .

-- Евгений Рябов

[StefanSch]

Hi Eurol, i have heavily rework the SPI functions. If you are interested in testing i can provide this. ~ Stefan

[eurol]

Yes.

[StefanSch]

Hi, thanks! I just pushed the current version - can you grab it from here: https://github.com/energia/msp430-lg-core/tree/SPI_rework

Note: there are several files impacted: cores/msp430/HardwareSerial.cpp cores/msp430/usci_isr_handler.c cores/msp430/usci_isr_handler.h libraries/SPI/SPI.cpp libraries/SPI/SPI.h libraries/SPI/utility/eusci_spi.cpp libraries/SPI/utility/spi_430.h libraries/SPI/utility/usci_spi.cpp libraries/SPI/utility/usi_spi.cpp libraries/Wire/utility/twi.c variants/MSP-EXP430FR2355LP/pins_energia.h variants/MSP-EXP430FR5994LP/pins_energia.h

~ Stefan

[eurol]

Just tested.

unsigned char a[4] = {1, 2, 3, 4}; SPI.transfer(a, 4);

is not working.

Then I change spi_send:

void spi_send(void *buf, uint16_t count)
{
    uint8_t *ptx = (uint8_t *)buf;
    uint8_t *prx = (uint8_t *)buf;
    if (count == 0) return;
    while (!(UCzIFG & UCTXIFG));
    UCzRXBUF;
    while(count){
            UCzTXBUF = *ptx++;
            count--;
                while  (!(UCzIFG & UCTXIFG));
                while  (!(UCzIFG & UCRXIFG)); // is it needed?
                *prx++ = UCzRXBUF;

    }
}

after this works.

The reason is:

while (!(UCzIFG & UCRXIFG)) count++; 

since in main while cycle the byte is already read.

[StefanSch]

Hi, thanks for testing and the feedback. Just updated to

void spi_send(void *buf, uint16_t count) 
{
   uint8_t *ptx = (uint8_t *)buf;
   uint8_t *prx = (uint8_t *)buf;
   if (count == 0) return;
   /* Wait for previous tx to complete. */
   while (UCzSTATW & UCBUSY);
   /* Clear RX Flag */
   UCzIFG &= ~UCRXIFG;
   while(count != 0){
      while (!(UCzIFG & UCTXIFG)); 
      /* Setting TXBUF clears the TXIFG flag. */
      UCzTXBUF = *ptx++;
      count--;
      while (!(UCzIFG & UCRXIFG));
      /* Reading RXBUF clears the RXIFG flag. */
      *prx++ = UCzRXBUF;
   }
}

~Stefan

[eurol]

The fastest code I wrote is:

void spi_send(void *buf, uint16_t count)
{
    if (count == 0) return;

    uint8_t *ptx = (uint8_t *)buf;
    uint8_t *prx = (uint8_t *)buf;
    volatile uint8_t *pIFG = &UCzIFG;
    volatile uint8_t *pTX = &UCzTXBUF;
    volatile uint8_t *pRX = &UCzRXBUF;

    UCzRXBUF;

__disable_interrupt();
    *pTX = *ptx; count--;

    if (count == 0)
    {
        while ((*pIFG & UCRXIFG) == 0);
        *prx = *pRX;
__enable_interrupt();
        return;
    }

    ptx++; 

    for (prx--; *pTX = *ptx++, prx++, --count;){
        while ((*pIFG & UCRXIFG) == 0);
        *prx = *pRX;
    }
        while ((*pIFG & UCRXIFG) == 0);
        *prx++ = *pRX;
        while ((*pIFG & UCRXIFG) == 0);
        *prx = *pRX;
__enable_interrupt();
}

but it needs to disable interrupts since pushes one extra byte to transmitter, While interrupt handler works bytes from shift register and from TX buffer register are sent but two bytes are received in RX buffer at the same time. Since buffer is one byte length one byte is lost.

This function will not work good in some circumstances, for example:

1. If CPU clock < SPI CLOCK
2. If someone send bytes to SPI without reading them back before calling this function.

So I will write a correct working variant.

[StefanSch]

Hi, thanks for this improvements. I now used some of them but would not like to go for disable interrupts as this has to many side effects which are hard to handle and to explain.

Updates are pushed to the branch above.

Best regards, Stefan

[eurol]

void spi_send(void *buf, uint16_t count)
{
    if (count == 0) return;
    uint8_t *ptx = (uint8_t *)buf;
    uint8_t *prx = (uint8_t *)buf;
    volatile uint8_t *pIFG = &UCzIFG;
    volatile uint8_t *pTX = &UCzTXBUF;
    volatile uint8_t *pRX = &UCzRXBUF;
    volatile uint8_t *pSTATW = &UCzSTATW;

    while (*pSTATW & UCBUSY);
    UCzRXBUF; // to clear error bits and UCRXIFG

    for (prx--; *pTX = *ptx++, prx++, --count;)
    {
        while  ((*pIFG & UCRXIFG) == 0); // 
        *prx = *pRX;
    }
        while  ((*pIFG & UCRXIFG) == 0); // 
        *prx = *pRX;
} // working better

This works good.

After some changes:

void spi_send(void *buf, uint16_t count)
{
    if (count == 0) return;
    uint8_t *ptx = (uint8_t *)buf;
    uint8_t *prx = (uint8_t *)buf;
    volatile uint8_t *pIFG = &UCzIFG;
    volatile uint8_t *pTX = &UCzTXBUF;
    volatile uint8_t *pRX = &UCzRXBUF;
    volatile uint8_t *pSTATW = &UCzSTATW;

    while (*pSTATW & UCBUSY);
    UCzRXBUF; // to clear error bits and UCRXIFG
    do
     {
      *pTX = *ptx++;
      --count;
      while  ((*pIFG & UCRXIFG) == 0); // we don't need to check UCTXIFG
      *prx++ = *pRX;
     } while (count);
} // working better

For example sending/receiving 512 bytes:

SPI clock is 4MHz. Interrupts are disabled.

your code takes about 4.74ms (~108000 bytes/sec) this code takes about 2.12ms (~241500 bytes/sec)

that code with hacks 1.29ms (~396900 bytes/sec)

[StefanSch]

Hi,

thanks again for the suggested speed improvements. Was not sure if the UCzRXBUF; access is properly handled by the compiler and not optimized away but after some tests it looks good. So now also using this. The new version is available on github. Will trigger a merge request in the next few days. ~ Stefan