SPI MISO buffer doesn't fill

[KevinL]

Hello,

I am using an nRF52840PDK (master) to use an SPI peripheral, where my code is based on examples\peripheral\spi. My IDE is Segger Embedded Studio and I am using SDK 15.0.0. I am using an oscilloscope to visualize the SPI bus activity.

I start of by configuring the SPI peripheral hardware on the nRF52840. Afterwards I write my configurations into the external peripheral registers over SPI. All of this works as expected.

Following is my problem:

I am trying to read from one of the registers. To do this I write my command byte and want to read 2 bytes (1 status & 1 data). I can see both MOSI and MISO lines reflect the correct data with the oscilloscope. The data does however not seem to be getting loaded into the receive buffer.

Using the debugger I can see being stuck in an endless loop (peripheral.c, register_read(), thus the spi_event_handler is not called to set spi_transfer_done = true. My first suspicion was my event_handler pointer. This works during writing so should also work during reading. Using a watch on the rx_buffer shows me that it is not being filled.

Below this is are scope images and my relevant code. At the very bottom is the concrete question.

Both of the devices are in SPI mode 0.

Yellow = MOSI - blue = MISO - grey line = SCLK 2 periods (external trigger edited in manually)      ----      The first MISO byte is a status byte and discarded, second byte is data

Image writing register 17:

MOSI [command,  data] - MISO [status]

Image reading register 17:

MOSI[command] - MISO[status, data]

main.c:

static const nrf_drv_spi_t peripheral = NRF_DRV_SPI_INSTANCE(SPI_INSTANCE);       /**< SPI instance */
const nrf_drv_spi_t *p_spi_instance = &peripheral;
static volatile bool spi_xfer_done;                                             /**< Flag used to indicate that SPI instance completed the transfer */

extern void *p_spi_event_handler;
void spi_config(void)
{
  nrf_drv_spi_config_t spi_config = NRF_DRV_SPI_DEFAULT_CONFIG;
  spi_config.sck_pin  = SPI_SCK_PIN;
  spi_config.miso_pin = SPI_MISO_PIN;
  spi_config.mosi_pin = SPI_MOSI_PIN;
  spi_config.ss_pin = SPI_SS_PIN != NRF_DRV_SPI_PIN_NOT_USED ? SPI_SS_PIN : NRF_DRV_SPI_PIN_NOT_USED;
  APP_ERROR_CHECK(nrf_drv_spi_init(p_spi_instance, &spi_config, p_spi_event_handler, NULL));
}

int main(void)
{
    //SPI configuration
    spi_config();
    //Peripheral initialization
    peripheral_init(&spi_xfer_done);

    // Enter main loop.
    for (;;)
    {
        idle_state_handle();
    }
}

peripheral.h:

#ifndef PERIPHERAL_H__
#define PERIPHERAL_H__

#include "nrf_drv_spi.h"

extern const nrf_drv_spi_t *p_spi_instance;                                      /**< Pointer to SPI instance */

#endif  //PERIPHERAL_H__

peripheral.c:

#include "peripheral.h"
#include "nrf_gpio.h"
#include "nrf_log.h"
#include "nrf_drv_spi.h"
#include <stdint.h>

static volatile bool *p_spi_xfer_done;                                           /**< Pointer to SPI_xfer_done flag */
static uint8_t m_tx_buf[2] = {};                                                /**< TX buffer */
static uint8_t m_rx_buf[sizeof(m_tx_buf) + 1];                                  /**< RX buffer */
static const int8_t tx_length = sizeof(m_tx_buf);                               /**< Transfer length */
static const int8_t rx_length = sizeof(m_rx_buf);                               /**< Transfer length */

/**@brief Reads data from a register
 */
static void register_read(peripheral_registers_t register_to_access, int rx_length)
{
  peripheral_result_codes_t result;
  uint8_t command;

  memset(m_tx_buf, 0, tx_length);
  memset(m_rx_buf, 0, rx_length);
  *p_spi_xfer_done = false;

  m_tx_buf[0] = command_byte_set(register_to_access, read);    //Left shift 3 & set 2nd bit 0
  APP_ERROR_CHECK(nrf_drv_spi_transfer(p_spi_instance, m_tx_buf, 1, m_rx_buf, rx_length + 1));      //p_spi_instance = 0x0003be04

  while (!*p_spi_xfer_done)     //I get into an endless loop here
  {
    __WFE();
  }
}

/**@brief Writes data to a register
 */
static void register_write(peripheral_registers_t register_to_access, uint8_t data, int tx_length)
{
  peripheral_result_codes_t result;
  uint8_t command;

  memset(m_tx_buf, 0, tx_length);
  memset(m_rx_buf, 0, rx_length);
  *p_spi_xfer_done = false;

  m_tx_buf[0] = command_byte_set(register_to_access, write);    //Left shift 3 & set 2nd bit 1
  m_tx_buf[1] = data;         //append data to buffer

  APP_ERROR_CHECK(nrf_drv_spi_transfer(p_spi_instance, m_tx_buf, tx_length + 1, m_rx_buf, 1));

  while (!*p_spi_xfer_done)
  {
    __WFE();
  }
}

void spi_event_handler(nrf_drv_spi_evt_t const * p_event,
                       void *                    p_context)
{
    *p_spi_xfer_done = true;
    NRF_LOG_INFO("Transfer completed.");
    if (m_rx_buf[0] != 0)
    {
        NRF_LOG_INFO(" Received:");
        NRF_LOG_HEXDUMP_INFO(m_rx_buf, strlen((const char *)m_rx_buf));
    }
}
void (*p_spi_event_handler) = &spi_event_handler;       //Function pointer declared in main.c

/**@brief Initialize peripheral at startup
 */
void peripheral_init(volatile bool *spi_xfer_done)
{
  p_spi_xfer_done = spi_xfer_done;

  register_init(17, 0, 0, 0, 1, 1, 0, 1, 0);        //creates byte with configuration value and calls register_write
}

static void register_init(peripheral_registers_t register_to_access,
                     uint8_t bit_value7,
                     uint8_t bit_value6,
                     uint8_t bit_value5,
                     uint8_t bit_value4,
                     uint8_t bit_value3,
                     uint8_t bit_value2,
                     uint8_t bit_value1,
                     uint8_t bit_value0)
{
  uint8_t register_value = 0;
  uint8_t bit_value_array[8] = {bit_value0, bit_value1, bit_value2, bit_value3, bit_value4, bit_value5, bit_value6, bit_value7};   //Array and binary MSB/LSB are reverse in order!

  for(int8_t bit_shift = 7; bit_shift >= 0; bit_shift--)
  {
    //Shift configuration bit into the right place of the register value
    register_value |= shift_bit(register_value, bit_value_array[bit_shift], bit_shift);
  }
  register_write(register_to_access, register_value, 1);
}

/**@brief Read register 17 on button press
 */
void peripheral_read_17(void)
{
  uint8_t result;
  register_read(17, 1);
}

Regarding the SPI mode: the nRF52840 is in mode0 (SCK active high, sample on leading edge of clock) while the documentation of my peripheral reads:

"[...] These signals are represented in the form (CPOL, CPHA). An
interface that expects both positive edge SCKS and the MOSI data to be available before the first
positive clock edge, can operate in modes (0,0) and (1,1) without alteration. […]"

My question is: Why am I getting stuck in this endless loop and how can I fix it?

Thank you in advance (this is quite long),

Kevin

Edit: Corrected image discription

P.s.: I think there is a bug where I can't upload, remove, then reupload a different image with the same name. It will then show the first image again in the editor.

[haakonsh]

1. Is the MISO line connected?
2. Does the read function work if you place everything in main.c of the SPIM example?

"P.s.: I think there is a bug where I can't upload, remove, then reupload a different image with the same name. It will then show the first image again in the editor." 
- yeah we know, I think it will use the new file once you submit the post though. 

Also, spi_config.ss_pin = SPI_SS_PIN != NRF_DRV_SPI_PIN_NOT_USED ? SPI_SS_PIN : NRF_DRV_SPI_PIN_NOT_USED; //this if statement does nothing, you might as well just use spi_config.ss_pin = SPI_SS_PIN; 

[KevinL]

I have tried finding what causes spi_event_handler not to get called in my project. I have set breakpoints at

- nrf_drv_spi.c line 94

- nrfx_spi.c line 234, 416 and 423

None of these breakpoints are getting triggered in my own project when reading registers. They do however trigger when writing registers.

[KevinL]

I have done single stepping through the driver when calling it from the button event handler before of which I have described the behaviour in a previous comment (4 September 14:43). I have now also tried single stepping through the driver when calling from main which causes it the a "softdevice: assertion failed" error. I assume this is due to timing requirements not being met as a result of single stepping. When I use breakpoints to evaluate specific points in the driver, specifically nrfx_spim.c line 536-540, the code immediately jumps to the spi event handler function. This behaviour seems correct to me.

So the difference is that after line 540 in nrfx_spim.c I either jump to the infinite while-loop or the spi event handler, using the button event handler or call from main respectively.

My optimization level for the solution has been set to "none" all the time.

[haakonsh]

Why do you use the SPI driver and not the SPIM driver?

The easy fix is to place the code in main.c as I believe it executes as intended when built in that compile unit. 

[KevinL]

If it turns out I cannot get the code to work in seperate files I will try placing everything in main.c. This would in my opinion be a bad practice, so I will first try to get it to work in peripheral.c. I'm convinced it should be possible.
I actually thought I had changed everything to now use the SPIM driver. I assume you are asking about the SPI driver because I had nrf_drv_spi.h included. I admittedly forgot to include nrfx_spim.h, which I have done now. I have also found that I was still using a few SPI driver types. I have now made the following changes:
main.c

extern void (*p_spim_event_handler)();        //Changed declaration
void spi_config(void)
{
  nrfx_spim_config_t spim_config = NRFX_SPIM_DEFAULT_CONFIG;
  spim_config.sck_pin = SPIM_SCK_PIN;
  spim_config.miso_pin = SPIM_MISO_PIN;
  spim_config.mosi_pin = SPIM_MOSI_PIN;
  spim_config.ss_pin = SPIM_SS_PIN != NRFX_SPIM_PIN_NOT_USED ? SPIM_SS_PIN : NRFX_SPIM_PIN_NOT_USED;    //Changed PIN_NOT_USED
  spim_config.frequency = SPIM_FREQUENCY;
  APP_ERROR_CHECK(nrfx_spim_init(p_spim_instance, &spim_config, *p_spim_event_handler, NULL));
}

peripheral.h

added
#include "nrfx_spim.h"

static void spim_event_handler(nrfx_spim_evt_t const *, void *);        //Added prototype

peripheral.c

static void spim_event_handler(nrfx_spim_evt_t const * p_event,                   //Changed parameter type
                       void *                  p_context)
{
    *p_spi_xfer_done = true;
    NRF_LOG_INFO("Transfer completed.");
    if (m_rx_buf[0] != 0)
    {
        NRF_LOG_INFO(" Received:");
        NRF_LOG_HEXDUMP_INFO(m_rx_buf, strlen((const char *)m_rx_buf));
    }
}
void (*p_spim_event_handler) = &spim_event_handler;

 (I can edit to include the complete code again)

There is actually a type "nrfx_spim_evt_handler_t", but when I use this instead of void I get warnings for incompatible pointer types even though it does work. I have also tried making the event handler static or non-static and eliminating the function pointer.

Regardless of everything I have tried, I can only make it work when I'm calling the read function from main(), but not when I press a button. Every time I use the button I can see the correct result in m_rx_buf, but the code stays in an infinite loop.

For good measure I have placed the event handler in main.c without any pointers, removing the calls to the buffer. This shows the exact same behaviour where it works fine when calling the transfer from main(), but not when calling via a button press.

Unfortunately it is essential for my project to be able to take user input (button) to start a transfer.

[KevinL]

I have found the solution and managed to confirm with another thread.

It comes down to the interrupt priority not being set correctly. To quote MartinBL:

It could be an issue with interrupt nesting. It seems like you start the SPI transfer 
from within the BSP event handler. If the SPI has lower interrupt priority than the 
BSP handler, then the SPI handler will not be executed before the BSP handler has 
completed. But since you are effectively waiting for the SPI in the BSP handler you 
end up in a dead lock. 

I have set SPI_DEFAULT_CONFIG_IRQ_PRIORITY to 6. I think the button is configured in GPIOTE_CONFIG_IRQ_PRIORITY which is set to 7.

Due to this I have a few questions:

Should I only set SPI_DEFAULT_CONFIG_IRQ_PRIORITY or also set NRFX_SPIM_DEFAULT_CONFIG_IRQ to a higher priority value, or something different altogether?

Is it correct that GPIOTE_CONFIG_IRQ_PRIORITY is the relevant IRQ for the BSP handler?

I also want to do a suggestion:

Include the name of the source and/or header file in the driver/library documentation in the infocenter. This way it's easier to match/find code files to documentation, or the other way around.

[haakonsh]

I suggest you use GPIOTE driver or app_button library instead of the Board Support Library.

"Should I only set SPI_DEFAULT_CONFIG_IRQ_PRIORITY or also set NRFX_SPIM_DEFAULT_CONFIG_IRQ to a higher priority value, or something different altogether?"
- I would set both to the same value.

"Is it correct that GPIOTE_CONFIG_IRQ_PRIORITY is the relevant IRQ for the BSP handler?

- Yes, I believe so. The BSP uses the app_button library and it uses the GPIOTE peripheral. 

"Include the name of the source and/or header file in the driver/library documentation in the infocenter. This way it's easier to match/find code files to documentation, or the other way around."
- I agree on this point, right now the library names do not reflect the naming convention used in the SDK.