Add a safeguard against declaring multiple variables for the same SPI device

[me21]

Hello,

My original issue was that my onReceive callback was not called. I've finally realized it's because I've declared a variable SPIClass spi(1), whereas there was one inside the library already: SPIClass SPI(1). The pointer to instance (_spi1_this) was rewritten, causing bugs when I called SPI functions through my variable, but interrupt handler used library variable.

Maybe the variable declared in the library is worth removing? Or, if it breaks existing code, add a safeguard to fail early when user declares another variable for using SPI 1?

I'm leaving the original text for historical purposes. Here it goes:

I keep trying to make SPI work in DMA mode in my program, in slave mode.
Here's how I initialize it:

    spi.beginSlave(); // MSB FIRST, SPI MODE 0
    SPI1->regs->CR1 &= ~SPI_CR1_RXONLY;
    spi.onReceive(onTxRxComplete);
    spi.onTransmit(onSendDuringFlashComplete);
    spi.dmaTransfer(rxData, rxData, sizeof(FlashHeader));

But onTxRxComplete never gets called.

I've tracked down the problem to SPIClass::EventCallback()

Arduino_STM32/STM32F1/libraries/SPI/src/SPI.cpp

Line 602 in 2578bf0

void SPIClass::EventCallback() {

by turning LED on or off.
The _currentSetting->state is SPI_STATE_IDLE inside SPIClass::EventCallback(), that's why my receive callback is not called (I checked this by comparing and turning the LED off if true). But I cannot find anywhere how can it be set to that value. It is set to that value only in constructor or in SPIClass::end() function, but it is not called in my program.

Does anyone have a working example of SPI with DMA? Preferably in slave mode.

[stevstrong]

Honestly, I am starting to have doubts on the whole _currentSetting stuff.
I think that was introduced to allow switching from SPI1 to SPI2 in a comfortable way using setModule(x).
But the best would be to declare a different instance for different interfaces. In that way the _currentSetting stuff wold be superfluous, and the SPI driver would be clean.

[victorpv]

@stevstrong you are right that was introduced a while back for the setModule stuff.
I saw you did something around the callbacks in the F4 version, so the pointer called is not the one in the object, but always the same for each individual port.
Does that work better?

About changing the pointers, another solution could be to declare static variables for the pointer, like I did for spi_this, have an spi1_onRx, spi1_onTx, spi1_onTRX, and another set for spi2 and spi3, and take them out of _currentSetting.
That way the pointers are per port and not per object. You could still use setmodule to change a single object to multiple ports, but then you could do:
SPI.setmodule(2);
SPI.onReceive(myCallBack);
SPI.dmaSend(xxx,yyy);
SPI.setModule(1);

And when the spi port2 ISR is called, it will call myCallBack function, no matter how may objects there are using SPI2, and no matter if the object that was used for setting the callback has later been changed to control a different port.
What do you guys think?

[rogerclarkmelbourne]

We could add the callback pointer (to function) to the Settings class so can save that as part of the currentSettings, so that when you change settings it changes the callback function (if its not null)

[victorpv]

@rogerclarkmelbourne currently it's like that, and that's the cause of the problem.
If the OP does this:

SPI.setmodule(2);
SPI.onReceive(myCallBack);
SPI.dmaSend(xxx,yyy);
SPI.setModule(1);

myCallBack will not be called, because when the DMA finishes and call the SPI callback function, that function will read the callback from the currentSettings, but at this point currentSettings has changed to use the port 1, and the callback (myCallBack) has not been set in those settings, but in the ones for port 2.
Ideally we want to keep a callback for a physical port, without regard of whether setModule has been called to set the object to a different port.

[stevstrong]

setModule() in actual form has the issue with bus sharing (several devices on the same bus).
We should maybe rework the callback function registration by passing CS pin as input parameter, which is different for each device, so that even devices sharing the same bus can have their own different callbacks, even if they are not initiating separate instance.

[stevstrong]

I think I will merge the version from my repo which has already implemented ISR pointers assigned for each port in part, which would solve this issue.