spi_xfer not interrupt safe?

[mstrong]

I'm trying to get a third party demo application working and I seem to be having an issue with the nrfx_spi_xfer function getting stuck waiting for the NRF_SPI_EVENT_READY flag.

I'm not using a callback so my init function looks like this

nrfx_spi_init(&spi0, &spi_config, NULL, NULL);

Since I'm not using a callback, it seems that nrfx_spi_xfer will basically just call the spi_xfer function.  This function clears the NRF_SPI_EVENT_READY flag upon entry.  Then starts the transfer and waits for the flag to be set.  I suspect that there is a slight race condition here.

The third party application code has an interrupt that writes a byte over SPI.  This interrupt happened to trigger while it was currently in the nrf_spi_event_check function.  I've stepped through and found that just before the call to nrfx_spi_xfer from the interrupt, the NRF_SPI_EVENT_READY flag appears as though it would've been set (I'm not 100% sure of that).  Then the spi_xfer function clears it for the new transfer, yet the old transfer is still "using" the spi bus.  The code is then stuck waiting for the NRF_SPI_EVENT_READY flag again. Once in the original nrfx_spi_xfer call and another time in the interrupt's nrfx_spi_xfer call.

I know an option is to just pass in a callback so that the code can be thread safe.  However, as I mentioned, this is a third party application I'm trying to integrate and I would like to get it working as is.  I was thinking the NRFX SPI interface shouldn't behave this way, but this is the first time using it so my expectations might be different.

[mstrong]

A little more clarification:

I'm using the stock SDK 15.0.0 available from Nordic infocenter.  I've only modified the sdk_config.h (attached)

The nRF52832 is the SPI Master communicating with the SX1272 mbed shield

Hopefully the screenshot shows the call stack well enough to get a better idea of what's going on in the code.  I've also attached the spi_hal.c code that interfaces directly with the NRFX SPI. 

 

Fullscreen spi_hal.c Download

/*!
 * \file                spi_hal.c
 * \brief               SPI interface to the nRF SDK peripheral
 *
 * This file contains the functions and declarations that are to be used as a
 * module for initializing and communicating over the SPI peripheral.
 * Initialization, read, write, and de-initilization functions are provided 
 * and can be used for multiple instances of the SPI peripheral.
 *
 * \author              Matt Strong
 * \date                08-Aug-2018
 *
 * \copyright           ©2018 Tapdn
 *                      ALL RIGHTS RESERVED
 * This software and all information and ideas contained within are the sole
 * property of Tapdn and are confidential.  Neither this software nor any part 
 * nor any information contained in it may be disclosed or furnished to others 
 * without the prior written consent of Tapdn.
 ******************************************************************************/
/* Standard includes */
#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <stdlib.h>
#include <stdbool.h>
/* SDK includes */
#include "nrfx_spi.h"
/* App includes */
#include "my_config.h"
#include "gpio_hal.h"
#include "spi_hal.h"
/* Logging macros, module logging can be controlled in my_config.h */
#define SPI_HAL_LOG(...)        MY_LOG(__VA_ARGS__)
#define SPI_HAL_LOG_ERROR(...)  MY_LOG_ERROR(__VA_ARGS__)
#ifdef SPI_HAL_LOG_INFO_ENABLE
#define SPI_HAL_LOG_INFO(...)   MY_LOG_INFO(__VA_ARGS__)
#else
#define SPI_HAL_LOG_INFO(...)
#endif

/*!
 * SPI instance storage for the NRF SDK, set to NULL if unused
 */
#if NRFX_SPI0_ENABLED
static const nrfx_spi_t spi0 = NRFX_SPI_INSTANCE(0);
#endif
#if NRFX_SPI1_ENABLED
static const nrfx_spi_t spi1 = NRFX_SPI_INSTANCE(1);
#endif
#if NRFX_SPI2_ENABLED
static const nrfx_spi_t spi2 = NRFX_SPI_INSTANCE(2);
#endif

int32_t spi_hal_open( spi_hal_t *p_spi)
{
    int32_t ret_val = MY_ERROR_SUCCESS;
    nrfx_spi_config_t spi_config;
    SPI_HAL_LOG_INFO("ENTER: 0x%x\r\n", (uint32_t)p_spi);
    /* Make sure the spi pointer is valid */
    if((p_spi == NULL)) {
        SPI_HAL_LOG_ERROR("Invalid parameters: 0x%x\r\n", (uint32_t)p_spi);
        ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
    }
    /* Setup the spi config struct with the paramters passed in, converting to SDK specific values where necessary */
    /* Validate parameter values along the way and set error codes if necessary */
    if (ret_val == MY_ERROR_SUCCESS) {
        spi_config.mosi_pin = p_spi->bus.mosi.pin;
        spi_config.miso_pin = p_spi->bus.miso.pin;
        spi_config.sck_pin = p_spi->bus.sclk.pin;
        spi_config.ss_pin = p_spi->bus.nss.pin;
        spi_config.irq_priority = p_spi->config.irq_priority;
        spi_config.orc = p_spi->config.orc;
        switch(p_spi->config.frequency) {
            case SPI_HAL_FREQUENCY_125K:
                spi_config.frequency = NRF_SPI_FREQ_125K;
                break;
            case SPI_HAL_FREQUENCY_250K:
                spi_config.frequency = NRF_SPI_FREQ_250K;
                break;
            case SPI_HAL_FREQUENCY_500K:
                spi_config.frequency = NRF_SPI_FREQ_500K;
                break;
            case SPI_HAL_FREQUENCY_1M:
                spi_config.frequency = NRF_SPI_FREQ_1M;
                break;
            case SPI_HAL_FREQUENCY_2M:
                spi_config.frequency = NRF_SPI_FREQ_2M;
                break;
            case SPI_HAL_FREQUENCY_4M:
                spi_config.frequency = NRF_SPI_FREQ_4M;
                break;
            case SPI_HAL_FREQUENCY_8M:
                spi_config.frequency = NRF_SPI_FREQ_8M;
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid frequency: %d\r\n", p_spi->config.frequency);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
    }
    if (ret_val == MY_ERROR_SUCCESS) {
        switch (p_spi->config.mode) {
            case SPI_HAL_MODE_0:
                spi_config.mode = NRF_SPI_MODE_0;
                break;
            case SPI_HAL_MODE_1:
                spi_config.mode = NRF_SPI_MODE_1;
                break;
            case SPI_HAL_MODE_2:
                spi_config.mode = NRF_SPI_MODE_2;
                break;
            case SPI_HAL_MODE_3:
                spi_config.mode = NRF_SPI_MODE_3;
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid mode: %d\r\n", p_spi->config.mode);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
    }
    if (ret_val == MY_ERROR_SUCCESS) {
        switch (p_spi->config.bit_order) {
            case SPI_HAL_BIT_ORDER_LSB_FIRST:
                spi_config.bit_order = NRF_SPI_BIT_ORDER_LSB_FIRST;
                break;
            case SPI_HAL_BIT_ORDER_MSB_FIRST:
                spi_config.bit_order = NRF_SPI_BIT_ORDER_MSB_FIRST;
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid bit order: %d\r\n", p_spi->config.bit_order);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
    }
    if (ret_val == MY_ERROR_SUCCESS) {
        switch(p_spi->bus.id) {
            case (SPI_HAL_0):
                #if NRFX_SPI0_ENABLED
                ret_val = nrfx_spi_init(&spi0, &spi_config, NULL, NULL);
                nrf_spi_int_disable(spi0.p_reg, NRF_SPI_INT_READY_MASK);
                #else
                #error Please enable NRFX_SPI0 
                #endif
                break;
            case (SPI_HAL_1):
                #if NRFX_SPI1_ENABLED
                ret_val = nrfx_spi_init(&spi1, &spi_config, NULL, NULL);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            case (SPI_HAL_2):
                #if NRFX_SPI2_ENABLED
                ret_val = nrfx_spi_init(&spi2, &spi_config, NULL, NULL);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid SPI ID: %d\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
        if (ret_val == NRF_SUCCESS) {
            SPI_HAL_LOG_INFO("nRF of SPI %d init successful: %d\r\n", p_spi->bus.id, ret_val);
            ret_val = MY_ERROR_SUCCESS;
        } else {
            switch(ret_val){
                /*! \todo add nRF specific error codes */
                default:
                    SPI_HAL_LOG_ERROR("nRF of SPI %d init failed: %d\r\n", p_spi->bus.id, ret_val);
                    ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
            }
        }
    }
    SPI_HAL_LOG_INFO("RETURN: %d\r\n", ret_val);
    return ret_val;
}

int32_t spi_hal_read( spi_hal_t *p_spi, uint8_t *p_in_data, uint8_t in_length)
{
    int32_t ret_val = MY_ERROR_SUCCESS;
    nrfx_spi_xfer_desc_t spi_xfer =  NRFX_SPI_XFER_RX(p_in_data, in_length);
    SPI_HAL_LOG_INFO("ENTER: 0x%x, 0x%x, %d\r\n", (uint32_t)p_spi, (uint32_t)p_in_data, in_length);
    /* Make sure none of the inputs are NULL or out of range.  The address can be any uint8_t so no need to check it */
    if((p_spi == NULL) || (p_in_data == NULL) || (in_length == 0)) {
        SPI_HAL_LOG_ERROR("Invalid parameters: 0x%x, 0x%x, %d\r\n", (uint32_t)p_spi, (uint32_t)p_in_data, in_length);
        ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
    }
    if (ret_val == MY_ERROR_SUCCESS) {
        /* Initiate a SPI read on the desired SPI bus using the parameters setup previously */
        SPI_HAL_LOG_INFO("Read %d bytes on SPI %d\r\n", in_length, p_spi->bus.id);
        switch (p_spi->bus.id) {
            case SPI_HAL_0:
                #if NRFX_SPI0_ENABLED
                ret_val = nrfx_spi_xfer(&spi0, &spi_xfer, NULL);
                #else
                #error Please enable NRFX_SPI0
                #endif
                break;
            case SPI_HAL_1:
                #if NRFX_SPI1_ENABLED
                ret_val = nrfx_spi_xfer(&spi1, &spi_xfer, NULL);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            case SPI_HAL_2:
                #if NRFX_SPI2_ENABLED
                ret_val = nrfx_spi_xfer(&spi2, &spi_xfer, NULL);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid SPI ID: %d\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
        if (ret_val == NRF_SUCCESS) {
            SPI_HAL_LOG_INFO("nRF of SPI %d successfully read %d bytes: %d\r\n", p_spi->bus.id, in_length, ret_val);
            ret_val = MY_ERROR_SUCCESS;
        } else {
            switch(ret_val){
                /*! \todo add nRF specific error codes */
                default:
                    SPI_HAL_LOG_ERROR("nRF of SPI %d read failed: %d\r\n", p_spi->bus.id, ret_val);
                    ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
            }
        }
    }
    SPI_HAL_LOG_INFO("RETURN: %d\r\n", ret_val); 
    return ret_val;
}

int32_t spi_hal_write( spi_hal_t *p_spi, uint8_t *p_out_data, uint8_t out_length )
{
    int32_t ret_val = MY_ERROR_SUCCESS;
    nrfx_spi_xfer_desc_t spi_xfer = NRFX_SPI_XFER_TX(p_out_data, out_length);
    SPI_HAL_LOG_INFO("ENTER: 0x%x, 0x%x, %d\r\n", (uint32_t)p_spi, (uint32_t)p_out_data, out_length);
    /* Make sure none of the inputs are NULL or out of range. The address can be any uint8_t so no need to check it */
    if((p_spi == NULL) || (p_out_data == NULL) || (out_length == 0)) {
        SPI_HAL_LOG_ERROR("Invalid parameters: 0x%x, 0x%x, %d\r\n", (uint32_t)p_spi, (uint32_t)p_out_data, out_length);
        ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
    }

    if (ret_val == MY_ERROR_SUCCESS) {
        SPI_HAL_LOG_INFO("Write %d bytes on SPI %d\r\n", out_length, p_spi->bus.id);
        /* Initiate a SPI write on the desired SPI bus using the parameters setup previously */
        switch (p_spi->bus.id) {
            case SPI_HAL_0:
                #if NRFX_SPI0_ENABLED
                ret_val = nrfx_spi_xfer(&spi0, &spi_xfer, NULL);
                #else
                #error Please enable NRFX_SPI0
                #endif
                break;
            case SPI_HAL_1:
                #if NRFX_SPI1_ENABLED
                ret_val = nrfx_spi_xfer(&spi1, &spi_xfer, NULL);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            case SPI_HAL_2:
                #if NRFX_SPI2_ENABLED
                ret_val = nrfx_spi_xfer(&spi2, &spi_xfer, NULL);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid SPI ID: %d\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
        if (ret_val == NRF_SUCCESS) {
            SPI_HAL_LOG_INFO("nRF of SPI %d successfully wrote %d bytes: %d\r\n", p_spi->bus.id, out_length, ret_val);
            ret_val = MY_ERROR_SUCCESS; /*! \todo create error enum for more specific error codes */
        } else {
            switch(ret_val) {
                /*! \todo add nRF specific error codes */
                default:
                    SPI_HAL_LOG_ERROR("nRF of SPI %d write failed: %d\r\n", p_spi->bus.id, ret_val);
                    ret_val = MY_ERROR_FAIL;
                break;
            }
        }
    }
    SPI_HAL_LOG_INFO("RETURN: %d\r\n", ret_val); 
    return ret_val;
}

int32_t spi_hal_close( spi_hal_t *p_spi ) {
    int32_t ret_val = MY_ERROR_SUCCESS;
    SPI_HAL_LOG_INFO("ENTER: 0x%x\r\n", (uint32_t)p_spi);
    /* Make sure none of the inputs are NULL or out of range */
    if((p_spi == NULL)) {
        SPI_HAL_LOG_ERROR("Invalid parameters: 0x%x\r\n", (uint32_t)p_spi);
        ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
    }

    if (ret_val == MY_ERROR_SUCCESS) {
        SPI_HAL_LOG_INFO("Close SPI %d\r\n", p_spi->bus.id);
        /* Un-initialize the desired SPI bus */
        switch(p_spi->bus.id) {
            case (SPI_HAL_0):
                #if NRFX_SPI0_ENABLED
                nrfx_spi_uninit(&spi0);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            case (SPI_HAL_1):
                #if NRFX_SPI1_ENABLED
                nrfx_spi_uninit(&spi1);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            case (SPI_HAL_2):
                #if NRFX_SPI2_ENABLED
                nrfx_spi_uninit(&spi2);
                #else
                SPI_HAL_LOG_ERROR("Please enable SPI%d!\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                #endif
                break;
            default:
                SPI_HAL_LOG_ERROR("Invalid SPI ID: %d\r\n", p_spi->bus.id);
                ret_val = MY_ERROR_FAIL; /*! \todo create error enum for more specific error codes */
                break;
        }
    }
    SPI_HAL_LOG_INFO("RETURN: %d\r\n", ret_val);
    return ret_val;
}

5314.sdk_config.h

[Edvin]

Hello,

How many bytes are you trying to write via the SPI?

Can you try to write a multiple of four bytes, e.g. 4 bytes?

Can you also try to do an analyze of the SPI pins? e.g. with the saleae logical analyzer, if you have one?

Will the example run on an nRF52 DK? Is it possible to send the project that you are using?

Best regards,
Edvin

[mstrong]

I found an updated version of the third party code that disabled its interrupts during the spi read/write so that the transfer wouldn't get interrupted and that solved the issue.  I also tested rewriting the interrupt handler to just set a flag and move all the other code to a secondary function.  Then the main loop would check the flag and call the secondary function if it was set.  This got around the issue as well.

I think I just had unreasonable expectations of the hardware peripheral.  I was expecting it to handle being told to do another transaction even if it was in the middle of one.  The SDK does provide the SPI transfer manager to also get around this scenario, however I it started to get convoluted to turn the non-blocking solution into a blocking solution as the third party code expected.