The Atmel Start SPI driver is terrible and you should be ashamed

Go To Last Post
12 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This isn't so much a question as it is a cautionary tale for others considering using Atmel Start/ASF4.

 

I'm in the midst of a big overhaul of the provided SPI driver, and while there's a lot that's bad, I'm just going to point out the part that led me down this path of regret and hate.

 

Here is the (hilariously long) call stack required to set the SPI baud rate:

 

int32_t spi_m_sync_set_baudrate(struct spi_m_sync_descriptor *spi, const uint32_t baud_val)
{
	ASSERT(spi);
	return _spi_m_sync_set_baudrate(&spi->dev, baud_val);
}

Calls

int32_t _spi_m_sync_set_baudrate(struct _spi_m_sync_dev *dev, const uint32_t baud_val)
{
	ASSERT(dev && dev->prvt && baud_val);

	return _spi_set_baudrate(dev->prvt, baud_val);
}

Calls

static int32_t _spi_set_baudrate(void *const hw, const uint32_t baud_val)
{
	hri_spi_write_CSR_SCBR_bf(hw, 0, (uint8_t)baud_val);
	return ERR_NONE;
}

Calls (finally)

static inline void hri_spi_write_CSR_SCBR_bf(const void *const hw, uint8_t index, hri_spi_csr_reg_t data)
{
	uint32_t tmp;
	SPI_CRITICAL_SECTION_ENTER();
	tmp = ((Spi *)hw)->SPI_CSR
; tmp &= ~SPI_CSR_SCBR_Msk; tmp |= SPI_CSR_SCBR(data); ((Spi *)hw)->SPI_CSR
= tmp; SPI_CRITICAL_SECTION_LEAVE(); }

Now the main issue here.  Notice that 0 in the second to last call? hri_spi_write_CSR_SCBR_bf(hw, 0, (uint8_t)baud_val);

 

There is a separate chip select register for each chip select line in each SPI peripheral.  This is great because it contains information like the SPI baudrate, the mode, the chip select behavior, number of bits to transfer, etc.  If you have 2 devices on the SPI bus and each has different requirements, it's no big deal, you just set each CS register appropriately and then the hardware handles it automatically depending on which device is selected.

 

What does this driver do?  It HARDCODES the function to always, and only, write to the CS0 register.  So now instead of setting it once during init and then not worrying about it, you need to change the baudrate register every time you select a different SPI device.  That's pretty awful on its own, but they also don't bother to implement any of the other chip select functionality, and in fact the API reference Manual states the limitation "The slave select (SS) is not automatically inserted during read/write/transfer, user must use I/O to control the devices' SS".  So instead of using the (pretty great) hardware capabilities of the SPI module, you have to manually toggle GPIOs to control CS lines and change device config settings every time you swap between 2 devices on the same bus. 

 

It's not even a difficult fix, just bring the index argument up through all the abstraction layers instead of hard coding it.  It took me longer to write this rant than it did for me to hack that in.

I know ASF4 is less mature than ASF3, but this is basic functionality (which ASF3 had), and Start/ASF4 takes a huge step backwards in that regard.

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

My eyesight must be failing me but I don't see where "index" is used at all in the final function?

 

So it's not so much that it "hardcodes" the selection as forgets to use it at all.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Looks like the code paste got screwed up some how. I tried to paste without code tags and it did the same here.

Last Edited: Wed. Apr 24, 2019 - 02:52 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As a picture because something in there breaks the post.

Attachment(s): 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


Ah yes, that make more sense. Just out of interest were you trying to use the code editor here:

 

 

Pastes into that usually work OK. What often does not work OK are pastes direct into the message editor.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


static inline void hri_spi_write_CSR_SCBR_bf(const void *const hw, uint8_t index, hri_spi_csr_reg_t data)
{
	uint32_t tmp;
	SPI_CRITICAL_SECTION_ENTER();
	tmp = ((Spi *)hw)->SPI_CSR
; tmp &= ~SPI_CSR_SCBR_Msk; tmp |= SPI_CSR_SCBR(data); ((Spi *)hw)->SPI_CSR
= tmp; SPI_CRITICAL_SECTION_LEAVE(); }

Yep, used the code editor and it pastes in fine, but gets mangled when it posts/previews.  On a related note, why does the code editor turn everything green instead of posting with the formatting it has in edit mode?


Here's what I see after previewing:

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oh hey, another error, cool!

 

// <o> Delay Before SPCK (ns) <0-255000>
// <i> This field defines the delay from NPCS falling edge (activation) to the first valid SPCK transition (in ns).
// <id> spi_master_dlybs
#ifndef CONF_SPI_5_DLY_SPCK
#define CONF_SPI_5_DLY_SPCK 1000
#endif

// <o> Delay Between Consecutive Transfers (ns) <0-8160000>
// <i> This field defines the delay between two consecutive transfers with the same peripheral without removing the chip select (in ns).
// <id> spi_master_dlybct
#ifndef CONF_SPI_5_DLY_BCT
#define CONF_SPI_5_DLY_BCT 1000
#endif

/* Calculates the value of the CSR DLYBS field given the desired delay (in ns) */
#ifndef CONF_SPI_5_DLYBS
#define CONF_SPI_5_DLYBS (((CONF_FLEXCOM5_FREQUENCY / 1000000) * CONF_SPI_5_DLY_SPCK) / 1000)
#endif

/* Calculates the value of the CSR DLYBCT field given the desired delay (in ns) */
#ifndef CONF_SPI_5_DLYBCT
#define CONF_SPI_5_DLYBCT (((CONF_FLEXCOM5_FREQUENCY / 1000000) * CONF_SPI_5_DLY_SPCK) / 32000)
#endif

That last block should read like this:

/* Calculates the value of the CSR DLYBCT field given the desired delay (in ns) */
#ifndef CONF_SPI_5_DLYBCT
#define CONF_SPI_5_DLYBCT (((CONF_FLEXCOM5_FREQUENCY / 1000000) * CONF_SPI_5_DLY_BCT) / 32000)
#endif

 

Last Edited: Thu. Apr 25, 2019 - 05:59 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hillridge wrote:
On a related note, why does the code editor turn everything green instead of posting with the formatting it has in edit mode?
When this new site was built they "out sourced" the code editing - so when you press that button it heads off to some completely different site  and does some lovely. colour coordinated source editing. Sadly the receiving interface here that gets the final result of that editing session either doesn't see or cannot cope with the colouration. So it applies a fixed font/colour to it. At least it is better than it was - during the early days the font/colour was atrocious.

 

What you can do is use a site like:  https://tohtml.com/c/  then you can paste the result direct into the editor here:

 

/* Copy a file "file.bin" on the drive 1 to drive 0 */

int main (void)
{
    FATFS fs0, fs1;      /* Work area (filesystem object) for logical drives */
    FIL fsrc, fdst;      /* File objects */
    BYTE buffer[4096];   /* File copy buffer */
    FRESULT fr;          /* FatFs function common result code */
    UINT br, bw;         /* File read/write count */


    /* Register work area for each logical drive */
    f_mount(&fs0, "0:", 0);
    f_mount(&fs1, "1:", 0);

    /* Open source file on the drive 1 */
    fr = f_open(&fsrc, "1:file.bin", FA_READ);
    if (fr) return (int)fr;

    /* Create destination file on the drive 0 */
    fr = f_open(&fdst, "0:file.bin", FA_WRITE | FA_CREATE_ALWAYS);
    if (fr) return (int)fr;

    /* Copy source to destination */
    for (;;) {
        fr = f_read(&fsrc, buffer, sizeof buffer, &br);  /* Read a chunk of source file */
        if (fr || br == 0) break; /* error or eof */
        fr = f_write(&fdst, buffer, br, &bw);            /* Write it to the destination file */
        if (fr || bw < br) break; /* error or disk full */
    }

    /* Close open files */
    f_close(&fsrc);
    f_close(&fdst);

    /* Unregister work area prior to discard it */
    f_mount(0, "0:", 0);
    f_mount(0, "1:", 0);

    return (int)fr;
}

 

I picked "HomeSite" as the style for that bit of code lifted from the FatFs user manual.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I discovered that the auto-generated SPI configuration file (Config\hpl_spi_config.h) for my Atmel START project did not properly set the delay between consecutive bytes. The SPI bus would always run at max speed regardless of the value entered. The reason is that Config\hpl_spi_config.h defines a macro

CONF_SPI_0_DLY_BCT

which does not match the file hpl\spi\hpl_spi.c's reference

CONF_SPI_##n##_DLYBCT

The problem is the difference in underscore. The undefined macro is evaluated to 0 which means max speed (no delay between bytes).

/Jakob Selbing

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Good catch, but not surprising based on the rest of the driver.  I doubt this was tested at all beyond seeing if it would compile before publishing it.  

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'd like to add a related problem, that is found in the in the "SPI Master with custom CS" middleware library (which is dependent on the SPI driver). This library is so abstracted that it's 'transaction' function doesn't utilize the two way communication possible with SPI.

 

/**
 *  \brief read write data over SPI interface
 */
void spi_write_read(uint8_t channel_num, uint8_t *txbuff, uint8_t *rxbuff, uint16_t txlen, uint16_t rxlen)
{
	spi_slave_enable(channel_num);
	if (txbuff != NULL && txlen > 0) {
		io_write(io, txbuff, txlen);
	}
	if (rxbuff != NULL && rxlen > 0) {
		io_read(io, rxbuff, rxlen);
	}
	spi_slave_disable(channel_num);
}

As you can see, it uses write, then read, instead of calling the appropriate transaction function (from the SPI library):
 

int32_t spi_m_sync_transfer(struct spi_m_sync_descriptor *spi, const struct spi_xfer *p_xfer)
{
    struct spi_msg msg;

    ASSERT(spi && p_xfer);

    msg.txbuf = p_xfer->txbuf;
    msg.rxbuf = p_xfer->rxbuf;
    msg.size  = p_xfer->size;
    return _spi_m_sync_trans(&spi->dev, &msg);
}

The error is due to the dependence of the io descriptor, which does not contain a transaction function call.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I just noticed that spi_m_sync_set_baudrate expects the clock bitrate register value and not the baudrate. ASF, the library that just keeps on giving.