Non blocking SPI DMA – Added callback to the SPI DMA functions (dmaSend, dmaTransfer…)

[stevestrong – Fri Jun 23, 2017 8:50 pm] –
Made some necessary changes to be able to build it.
I did not test the new functions, only the old (original) DMA ones, they seem to work.

Thanks Steve, I just finished compiling it and found similar errors and corrected them too.
I’m uploading the new. It’s pretty much the same changes you had to do, except in TransferSet and SendSet I changed it to void * rather than the uint8 * I was using before (my original code was based off an older version of the core).

I also did not use a typedef for the function pointers, but I see you did. I would imagine you added to make the code clearer to read, but you think is necessary and does not actually makes more difficult to read the code by having to check what that type is? or did you use it for some other reason?

Here is the new version that compiles right. Like I said pretty close to Steve’s except for those differences. I still need to test running my previous test sketch with it, which uses the callbacks. I do not have any code to test the new async function, I hope I didn’t break it.

(10.44 KiB) Downloaded 19 times

For my test I used my old wav player test code, it uses FreeRTOS 900. DMA is running 3 peripherals, SPI1, SPI2, and a timer to produce the PWM output.
I modified sdFat so it set’s a callback function and after dmaSend or dmaTransfer it sets the task to block until released by the ISR, to the RTOS changes to the next task and keeps the cpu busy until the SPI DMA transfer is over. the ISR causes a new context switch upon exit and RTOS returns back to the task that was reading from the sdcard.
I have tested it with the display too, but just introduces jitter since the display task only writes a few bytes at a time, and all the context switching just wastes any cpu time that could be used for anything else. So in the display it works but doesn’t provide any performance gain.

These are my changes to the SdSpiSTM32F1.cpp file in the sdfat library in case anyone is interested in testing:
First include the RTOS (9.0 in my case, should work with 8.2.1 too):
#include <MapleFreeRTOS900.h>

[danieleff – Tue Jun 27, 2017 4:42 am] –
Instead of all of this, wouldn’t it be enough to add `yield()` to the while loop that waits for the DMA to finish? That will do a context switch for you while waiting.

Daniel I’m not sure I understand your suggestion, so correct me if I’m wrong, but you suggest to use FreeRTOS taskYIELD() is that right?
That wouldn’t work for 2 reasons.
1.- It does a yield only if a higher priority task is waiting to execute. If the task doing the sdfat access at the moment is the highest one ready to execute, it will not yield and continue running, so you effectively didn’t yield to another task to run while DMA is ongoing.
2.- If we just yielded, it will return to this task at a moment that is not synched with the DMA transfer, we still needs to know if the DMA is completed before continuing with the sdfat code. Unless you use some sort of semaphore and check for it, or you have to be polling the DMA controller to check if it’s done. The first case is exactly what’s implemented, only with task notifications that according to the FreeRTOS docs cost less cycles and RAM than a semaphore. If we did the second case, poll the DMA controller, then we are wasting cycles just polling. We already have that in the library when we run in blocking mode.

By using the task notification we achieve 2 things. First the task will yield to the next one to run, even if the ones available are lower priority. And second as soon as the Callback function is called because the DMA transfer is over, it will request a yield to the RTOS. If the task executing has the same or lower priority, it will go to the sdfat one that was blocked. If the task currently running is higher priority, then it will mark this one as ready to run and will run next time it can according to priorities etc.

If you meant with the yield() is something else please let me know. I know there is some yield() funtion in the sdfat library but I understood that was to put the cpu in sleep mode until the next interrupt, in which case it will not execute another task.

Finally this is just a usage example because I had this FreeRTOS sketch, but there is no need to use any RTOS, since the callback is declared by the user code it could be used to set a variable, change a pin level etc.

The SPI zip seems to work OK for me, but I’m not using any callbacks.

I used the code with my OV7670 camera testbed and it was fine.

But it doesnt include the dmaSendAsync

I can manually merge that new function into your code and then check my local copy back into github, but it would not be as clean as you pulling the latest repo into you github repo and then submitting a PR based on the changes.

So if you want to do that, I’ll merge the PR asap e.g. tomorrow

Thanks

Roger

PS. As you can see from my PM, I’ve merged you other PR’s

[RogerClark – Mon Jul 03, 2017 5:53 am] –
Victor

The SPI zip seems to work OK for me, but I’m not using any callbacks.

I used the code with my OV7670 camera testbed and it was fine.

But it doesnt include the dmaSendAsync

I can manually merge that new function into your code and then check my local copy back into github, but it would not be as clean as you pulling the latest repo into you github repo and then submitting a PR based on the changes.

So if you want to do that, I’ll merge the PR asap e.g. tomorrow

Thanks

Roger

PS. As you can see from my PM, I’ve merged you other PR’s

My version was redone taking your latest as the base, so it has sendAsync too.
I’ll sent a PR with those 2 files.

I would change it like this:
uint32_t flags = (DMA_MINC_MODE | DMA_FROM_MEM);
if (!transmitBuf) {
transmitBuf = &ff;
flags &= ~DMA_MINC_MODE;
}
dma_setup_transfer(_currentSetting->spiDmaDev, _currentSetting->spiTxDmaChannel, &_currentSetting->spi_d->regs->DR, dma_bit_size,
transmitBuf, dma_bit_size, flags);

[stevestrong – Mon Jul 31, 2017 11:37 am] –
https://github.com/rogerclarkmelbourne/ … I.cpp#L399

I would change it like this:
uint32_t flags = (DMA_MINC_MODE | DMA_FROM_MEM);
if (!transmitBuf) {
transmitBuf = &ff;
flags &= ~DMA_MINC_MODE;
}
dma_setup_transfer(_currentSetting->spiDmaDev, _currentSetting->spiTxDmaChannel, &_currentSetting->spi_d->regs->DR, dma_bit_size,
transmitBuf, dma_bit_size, flags);

[victor_pv – Mon Jul 31, 2017 12:31 pm] –
Could you point to the lines you are referring to?

These two lines move to line 420.
It makes sense to clear them before any further request is launched.
And it would be nice to have them readable after a transfer to check on an upper software level whether any error flags have been set or not.

[stevestrong – Mon Jul 31, 2017 12:36 pm] –

[victor_pv – Mon Jul 31, 2017 12:31 pm] –
Could you point to the lines you are referring to?

These two lines move to line 420.
It makes sense to clear them before any further request is launched.
And it would be nice to have them readable after a transfer to check on an upper software level whether any error flags have been set or not.

The problem would that in a situation like this:
You run a dmaTransfer. (the ISR bits are not cleared)
You go doing something with that DMA channel and enable IRQ requests. Since the interrupt request is still set, I think an interrupt will trigger right away.

I believe that’s the way they work from what I remember from the reference manual.

If you are making the change as part of the code clean up, let me know once you have the PR and I will run a test with my sketch that uses then with callbacks.

EDIT:
Just a note, when using ISRs with DMA, the core dma irq handler clears them, right after calling the user dma handler:
https://github.com/rogerclarkmelbourne/ … vate.h#L45
So when using callbacks they will be cleared at the end of the transfer, but if the user code handler wants to check them, that can be done since they are not cleared yet, but on return.

While you are looking at the SPI DMA stuff, I see that the OV7670 camera sketch that will use the ILI9341 as high speed RAM to store an image..

Would benefit from a function that does a dma Read, but does not care what is transmitted.

At the moment the best way to do this would be to use dmaTransfer, and perhaps point the TX buffer at some arbitary piece of flash (as in this case I don’t know if it matters what is sent – though I’d need to double check….)

But ideally, something like the dmaSend which only receives and perhaps sends a specific value (passed to the function)

I’m not sure how complex it would be to write something like this, but I presume it would be modified copy of dmaSend

The advantage on option 3 is that it allows to send an arbitrary value. Option 2 just adds a check to dmaTransfer, so should keep the code a bit shorter if both the normal transfer and the transfer with a null buffer are used in the same sketch, but should have a small performance impact from the check. Should not be much for a dmaTransfer though.

And option1 has the advantage of more closely resembling the current SPI.read(&buf, n) function, taking the same parameters and working in the same way except for using DMA, but is not as flexible as option 3.

We could also do a complete new function that works differently than the current dma ones and read(), and instead takes both a value to send repeatedly, and a receive buffer to read the data to. Something like:
dmaRead (uint8 tx_val, uint8 *rx_buf, uint8 n){}

I think we would need to have a way to repeatedly send whatever data, as I’m not sure if devices like the ILI9341 need 0x00 or 0xff when data is being read.
I suspect the ILI9341 should not care what its being sent when data is being read, but I’d need to confirm that, and its safer to be able to specify the byte that is sent when reading.

So the option which uses MINC looks like it may be the best solution

uint8 dmaTransfer(void * transmitBuf, void * receiveBuf, uint16 length);