Read continuous stream of data over SPI

[nordev]

Hello,

I want to read a status register in my SPI peripheral. Once I send my read request, the peripheral will send the status byte over and over, and I want to watch for a change. When it changes, I want to terminate the SPI transfer. 

What is the best way to do this? The complication here is that there isn't a fixed RX buffer length. The status byte might change in 5 bytes or 100 or 1000. It seems suboptimal to allocate memory for a really long transfer buffer just to accommodate, say, the case in which it takes 1000 bytes for the status byte to change, both because of the extra memory, and also because there's no way to know if it'll even be enough. I am using this function:

ret_code_t nrf_drv_spi_transfer(nrf_drv_spi_t const * const p_instance,
uint8_t const * p_tx_buffer,
uint8_t tx_buffer_length,
uint8_t * p_rx_buffer,
uint8_t rx_buffer_length)

Is there a way I can push the RX bytes into a queue, maybe, pop from the queue to read the bytes (which makes room for more bytes to be clocked in on SPI), then abort the spi transfer when I notice the changed byte? If so, is there a way to make sure the dequeueing rate is faster than the spi transfer rate? Or can I read from the transfer buffer even if it is not full?

As a workaround, I guess I could TX my read request over and over and get back a few status bytes each time (buffer length = n+1, TX length = 1, RX = n), but it seems like this would defeat the purpose of SPI. 

Note: I think, based on the below screenshot, that I am using SPI and not SPIM.

Thanks!

[Einar Thorsrud]

Hi,

There are several ways to do this and I am not sure which is better. Using the SPI (and not SPIM) peripheral you will use the CPU for every byte, and you can inspect it. The driver is designed to handle this for you though, but you could for instance do a small hack and add your own callback function that is called from within the transfer_byte() function in nrfx_spi.c so that you can process each byte as it comes in, but still let the driver do it's work as normal.

[nordev]

Thank you Einar. One complication with using this method is that most of the time, I want to read a SPI buffer of known length, and only sometimes, I want to process each byte as it comes in in the manner described above.

I suppose I could create a duplicate transfer_byte() function, as well as a duplicate of all the functions that depend on transfer_byte(), such as spi_xfer(), nrf_drv_spi_xfer(), and nrf_drv_spi_transfer(), and simply call the modified functions (e.g., "my_nrf_drv_spi_transfer") when I need to. But two issues:

1. this still requires me to specify an RX buffer length, which I don't necessarily know in advance.

2. the below function also is dependent on transfer_byte(), but I am not really sure under what circumstances it is called. It seems somewhat redundant with the end of spi_xfer(), except that finish_transfer() is more thorough. Why does the end of spi_xfer() only set the chip select back high and not do the rest of the things in finish_transfer()? If I choose to modify the functions, is there any downside to calling finish_transfer() after while (transfer_byte(p_spi, p_cb)); inside spi_xfer()? 

static void irq_handler_spi(NRF_SPI_Type * p_spi, spi_control_block_t * p_cb)
{
    ASSERT(p_cb->handler);

    nrf_spi_event_clear(p_spi, NRF_SPI_EVENT_READY);
    NRF_LOG_DEBUG("SPI: Event: NRF_SPI_EVENT_READY.");

    if (!transfer_byte(p_spi, p_cb))
    {
        finish_transfer(p_cb);
    }
}

Do you have recommendations for a way to approach the continuous stream read that do not encounter the issues above? Thank you!

[nordev]

Thank you Einar. 

Einar Thorsrud said:

You should get NRF_ERROR_BUSY if attempting to start a second transaction before the first has finished.

is the right way to do this APP_ERROR_CHECK()?

The strange thing about checking this way, however, is that I have to actually call nrf_drv_spi_transfer() in order to get the error code. Either with APP_ERROR_CHECK() or something like “if(nrf_drv_spi_transfer() != NRF_ERROR_BUSY)”. 

So sure I might get an error code, but the damage will already have been done: the second transfer will have already disrupted the first transfer, which is what gets me stuck in the infinite while loop. 

is there some way to check without actually calling the SPI transfer function to see if it generates an error?

Einar Thorsrud said:

Do you call app_sched_execute() from your code?

no, I do not. are there other benefits of bundling the BLE event handling like this besides being able to queue interrupts & not lose them?

[Einar Thorsrud]

Hi,

nordev said:

is the right way to do this APP_ERROR_CHECK()?

No. APP_ERROR_CHECK() is used to catch unexpected errors. The default error handler in the SDK will log and break if an error is detected with this and in debug mode. If in release mode it will perform a soft reset - in order to recover from an unexpected and unhandled error. The correct is to handle any expected errors specifically, and then call APP_ERROR_CHECK() if the return value was not an expected/handled error, like you suggest with "if(nrf_drv_spi_transfer() != NRF_ERROR_BUSY)".

nordev said:

So sure I might get an error code, but the damage will already have been done: the second transfer will have already disrupted the first transfer, which is what gets me stuck in the infinite while loop. 

This stems from my incorrect previous answer, I guess. The driver checks if a transaction is ongoing before doing any changes. So a new transaction would not be able to cause problems for an old, but should just result in the second call failing with NRF_ERROR_BUSY. There is a slight possibility for an issue here though, as this is not implemented in thread safe way. So if the two calls are made at the same time with different priorities then it is theoretically possible that the second call with higher priority would interrupt the first after the transfer_in_progress was read but before it is written to. And then you would have problems. This would be solved by always calling from the same priority, or by using a proper check externally (using a mutex of similar).

nordev said:

is there some way to check without actually calling the SPI transfer function to see if it generates an error?

There is no API for that in the driver.

To sum up, if you call from the same priority, you can just rely on handling the NRF_ERROR_BUSY. If not, you should implement your one mutex on the outside. So that one "user" will lock the driver before starting and operation and unlock it when finished. And the same with the second. You could use nrf_atomic_u32_t for that, or just a volatile variable that you read and update in a critical region (CRITICAL_REGION_ENTER() / CRITICAL_REGION_EXIT()).

nordev said:

are there other benefits of bundling the BLE event handling like this besides being able to queue interrupts & not lose them?

There are no extraordinary benefits in the context of BLE events or even nRF5 SDK specifically. It boils down to general concepts of handling interrupts / priorities. If you have a need to move processing of events/interrupts (BLE or other) down from interrupt context to main context, then using the scheduler makes sense. If not, then there is no need. Typically the more complex an application is and the more different interrupts etc or the longer the interrupt routines, the more important it becomes to move priorities down. That way stuff that can wait may wait, and there is time to process other interrupts in in a timely manner. But there is no right or wrong way here, it is application dependent. This is related to the general importance of keeping ISRs short. If you want to do a lot of work because of an interrupt, but only a small portion of it is time critical you can do the small portion in the ISR, and then queue the rest in the scheduler to be processed in the main context.

[nordev]

Einar Thorsrud said:

So a new transaction would not be able to cause problems for an old, but should just result in the second call failing with NRF_ERROR_BUSY.

I don't know if this is actually the case, and I think the following concern still stands:

Einar Thorsrud said:

So sure I might get an error code, but the damage will already have been done: the second transfer will have already disrupted the first transfer, which is what gets me stuck in the infinite while loop. 

Because I changed my code in this function (code in a previous response):

nordev said:

at_read_status_once()

to do this: 

while(nrf_drv_spi_transfer(&m_spi_at, spi_tx_cmd, sizeof(spi_tx_cmd), spi_rx_buf, sizeof(spi_rx_buf)) == NRF_ERROR_BUSY);

so that if I got a NRF_ERROR_BUSY event it would just try again until it isn't busy. But I still get stuck in this while loop forever inside spi_xfer(): 

Here is the full stack. I have highlighted the two calls to nrf_drv_spi_transfer(). 

The first one is called from my main loop, and the second one is called from an RTC interrupt. My RTC is also a low interrupt priority, 6. 

Note that NRF_ERROR_BUSY should be returned from nrf_drv_spi_xfer (second in the call stack above). But this does not happen -- "p_cb->transfer_in_progress" seems to be false, even though there IS a transfer in progress (first highlight on above stack):

What do you think is happening? Is this a bug with SDK 14.2? Is it fixed in later versions? 

-----

As a separate matter, I had to refresh this webpage 5 times to get the "reply" button to show up at the bottom of your latest message (it showed up at the top underneath my original message but not elsewhere). Generally, every time I use the forum, I have to refresh multiple times in order for the reply button to show up. Perhaps you can relay this to your web development team. 

[nordev]

Hi Einar,

As an update, I found out what the issue is:

I hope this image is big enough for you to see, but basically:

1. there is only one place in the driver where transfer_in_progress() is ever set to true

2. it is guarded by the following conditional:

if (p_cb->handler && !(flags & (NRF_DRV_SPI_FLAG_REPEATED_XFER |
                                        NRF_DRV_SPI_FLAG_NO_XFER_EVT_HANDLER)))
        {
			p_cb->transfer_in_progress = true;
        }

3. flags are 0, which is not a problem, but handler is ALSO 0, which is a problem. So the condition is never satisfied, and even though there is a transfer in progress, the transfer_in_progress boolean is always false. 

Handler = 0 because I initialize the SPI driver with: 

err_code = nrf_drv_spi_init(&m_spi_at, &config, NULL, NULL);

Because I thought I don't need a handler if I am not using interrupts, since I am in blocking mode.

My questions: 

1) Did nordic intend this behavior? (basically, did I do something wrong, or is this a bug?)

2) Do you see any issue with me removing p_cb->handler from the conditional above in (2.)? 

[Einar Thorsrud]

Hi,

That is good spotted

nordev said:

1) Did nordic intend this behavior? (basically, did I do something wrong, or is this a bug?)

I am not sure if this is intended or not. You could argue that when using blocking mode it is easy for you to ensure that there is no conflict. And you would only be able to get a problem here if calls are made from different interrupt priorities. If that happens and they can overlap in time, then this will not be enough. On the other hand, there is no atomic operations or critical sections for updating the transfer_in_progress flag in any case, so you would still risk issues if calling from different interrupt priorities and not ensuring that one transaction finish before you initiate the next.

nordev said:

2) Do you see any issue with me removing p_cb->handler from the conditional above in (2.)? 

No, I do not immediately see any issues. But note my point above. There is nothing preventing transfer_in_progress from being checked first, then an interrup toccuring checking it again, and then writing it. So the driver is not thread safe and if you call the API from different interrupt priorities you must ensure that there are no collisions outside of the driver. Alternatively, ensure that you always call the driver API from the same interrupt priority, then this is no issue.

[nordev]

Einar Thorsrud said:

There is nothing preventing transfer_in_progress from being checked first, then an interrup toccuring checking it again, and then writing it.

Einar Thorsrud said:

then it is theoretically possible that the second call with higher priority would interrupt the first after the transfer_in_progress was read but before it is written to. And then you would have problems

so this could still be a potential problem.

right now, the only conflicting things that might simultaneously call the SPI driver are my main loop (not interrupt based) and the RTC interrupt. Given that I don’t expect anything else to be able to interrupt my RTC interrupt, I should be safe, right? 

eventually though, I will add another SPI transfer that will be triggered from a Current Time Service interrupt. I think this interrupt comes from the softdevice, which means there’s nothing I can do about matching interrupt priority, correct? So I have to make sure they don’t conflict in some other way. 

[Einar Thorsrud]

Hi,

nordev said:

right now, the only conflicting things that might simultaneously call the SPI driver are my main loop (not interrupt based) and the RTC interrupt. Given that I don’t expect anything else to be able to interrupt my RTC interrupt, I should be safe, right? 

I am not sure about that. If you make calls to the driver from both your main loop and an RTC interrupt, that means you may have this problem (think of the main context as the lowest possible priority). Specifically, the main loop may start a SPI transaction and the transfer_in_progress flag may be read, but before it is written to a RTC interrupt occurs. Then the a new transaction might be started from the RTC interrupt.

nordev said:

I think this interrupt comes from the softdevice, which means there’s nothing I can do about matching interrupt priority, correct? So I have to make sure they don’t conflict in some other way. 

Yes, that will have SoftDevice priority. You could move execution down to the main loop for all SPI transactions, perhaps. Either using some synchronization flags or using the app_scheduler or another mechanism. That is one way to avoid having to think about this. Alternatively, you can use a mutex as mentioned before, and handle this yourself outside the driver. Which is more appropriate depends on the specific application, but both are common approaches.

[nordev]

Thank you. 

Will you consider asking the development team to remove the p_cb->handler condition from the driver in a future SDK version?

[nordev]

EDIT: I realized it is rather unwieldy to add all of the things that might call SPI to the main loop, especially since there is stuff I haven't implemented yet that I will have to add in the future, and maybe there will be even more beyond that. I decided that the best solution would be to add SPI callback functions to a queue that then gets processed in the main loop, if this is possible. My problem is that all my callback functions will have different parameters . 1) Is there any way to define some sort of generic function prototype that I can make a queue for? 

Alternatively, I discovered this note:

which suggests that "interrupt pending" flags are set independently of interrupts being enabled or disabled. Which suggests that I can disable interrupts, then incoming interrupts will set "interrupt pending" flags but not execute, and then whatever has an interrupt pending flag will simply execute once I re-enable interrupts. This is what I was getting at here:

nordev said:

what I would like is for RTC interrupts to be disabled while a transfer is in progress (i.e. while chip select is low) and re-enabled when the transfer completes (chip select goes high), such that any interrupt that tries to happen in the middle of a transfer instead happens immediately after the transfer completes.

2) Do you know a way I can do this? 

As a final alternative.... if I remove p_cb->handler and use the NRF_BUSY return code to try again if a transfer was in progress, it would be probably very rare for something to interrupt after the transfer starts but before the "transfer in progress" flag gets set, right? So I could accept this risk, and maybe would have to reboot the device once in a rare while, if it is sufficiently rare (like 1 in 100,000 attempted SPI transfers). 3) Or maybe do you have some suggestion for making this flag-setting atomic? 

Thank you for your help!

[Einar Thorsrud]

Hi,

nordev said:

1) Is there any way to define some sort of generic function prototype that I can make a queue for? 

I would suggest the app_scheduler here. That is designed for exactly this use case. It is essentially just a queue that you put stuff into when handling interrupts, and process in the main loop. You will queue a function pointer and a context data (so it can be anything, you decide - and as large as you want as long as you adjust the element size for it). There are no good examples in the SDK, but the usage is quite simple so you can refer to examples\nfc\writable_ndef_msg\main.c. Essentially you first initialize the queue so that it holds the largest elements you need. Then you queue stuff using app_sched_event_put() when handlign the interrupt, and process it in your main loop by calling app_sched_execute().

nordev said:

2) Do you know a way I can do this? 

You can disable the interrupt any time and then re-enable it. If an interrupt is pending it will be processed. I do not know if you use the RTC driver, but if you do interrupts are enabled when you init it. You can do this directly using NVIC_EnableIRQ(RTC1_IRQn) and NVIC_DisableIRQ(RTC1_IRQn) - adjust if using other RTC instance.

If you ask me option 1 is by far the best. That is a straightforward and sensible way of handling this and similar issues, and is how you would typically solve this in a complex application.