earlephilhower / arduino-pico

Raspberry Pi Pico Arduino core, for all RP2040 and RP2350 boards
GNU Lesser General Public License v2.1
2.03k stars 422 forks source link

SPI transactions #1147

Closed obdevel closed 1 year ago

obdevel commented 1 year ago

Are there any plans to implement SPI transactions in the future ?

It is very useful when there are multiple devices on an SPI bus and SPI comms will take place in an interrupt handler. Without this functionality, SPI comms in an ISR will corrupt in-progress SPI comms with another device when an interrupt fires.

For example, using the MCP2515 CAN bus controller, it is helpful to destage incoming messages from the chip as soon as it asserts the interrupt. Waiting until later (e.g. setting a flag) may result in missed messages at high traffic rates. Continually polling the chip for new messages seems wasteful. It is correctly implemented in the standard AVR core.

As understand it, in other cores, SPI.beginTransaction defers the chosen interrupt until SPI.endTransaction is called. Provided the interrupt remains asserted, the ISR will then run.

For my current projects, I isolate this device onto one SPI bus, with other non-interrupting SPI devices on the other bus.

Thanks.

earlephilhower commented 1 year ago

I'm not sure I catch the exact difference you're looking suggesting, sorry. The SPI code is pretty simple and does not use any interrupts at all presently (no datasheet in front of me, but IIRC there's not really an IRQ from the onboard SPI peripheral, we just fire/forget/poll).

Maybe some code would help me understand? Do you have a hacked SPI.cpp or user code you can share?

obdevel commented 1 year ago

The MCP2515 asserts an interrupt when it has received a message from the CAN bus. It only has two hardware buffers and will drop subsequent messages if the buffered messages aren't destaged quickly enough. So, it's best to grab this data in an ISR. This requires reading and writing a few of the chip's registers using SPI.

For SPI comms in the main body of code (e.g. interactively to a touchscreen display) we register the interrupt number/pin the MCP2515 is attached to with SPI.usingInterrupt and then wrap SPI comms in SPI.beginTransaction and SPI.endTransaction. Thus we have exclusive access to the bus for the duration of the transaction and can be confident that an ISR won't jump in at an inopportune moment and trample over other in-progress comms.

Arduino API docs: https://www.arduino.cc/reference/en/language/functions/communication/spi/usinginterrupt/ Arduino AVR implementation: https://github.com/arduino/ArduinoCore-avr/blob/24e6edd475c287cdafee0a4db2eb98927ce3cf58/libraries/SPI/src/SPI.h

It seems to mask off this interrupt number in SPI.beginTransaction so that the ISR can't run until SPI.endTransaction is called.

I don't know how similar the Arm interrupt model is to AVR. If an external interrupt is asserted whilst it's masked off, will the ISR run as soon as the mask is unset ?

I've looked at a couple of other cores I use and it seems to be implemented there. The Arduino SAMD may be similar to the RP2040, e.g. https://github.com/arduino/ArduinoCore-samd/blob/000db0c4b8007c71ebe4f45aa09ad996c4feb0af/libraries/SPI/SPI.cpp

The Pico SDK seems to make it fairly straightforward: https://raspberrypi.github.io/pico-sdk-doxygen/group__hardware__irq.html

I suppose one could even use detachInterrupt and addInterrupt each time, but this may be fairly costly.

It's not urgent though; more of a 'nice to have'. (There's also now a project that implements a complete CAN controller in an RP2040 PIO. It works surprisingly well and I may use this for future projects: https://github.com/KevinOConnor/can2040).

Thanks again.

earlephilhower commented 1 year ago

I think I get the idea now, but just for confirmation the controller chip has an IRQ line going to one of the Pico's GPIOs, and when it asserts you need to use SPI to read out the data before it overflows from within your GPIO IRQ handler. You'd like to have other chip also driven by IRQ and on the same SPI bus, and need to make sure while one Pico IRQ handler is doing SPI work another doesn't come in. Or, conversely, you want to use the SPI bus in the main app (not IRQ driven) and need to make sure it doesn't do work while the other SPI IRQ handler is running.

Basically it seems like you're just disabling the Pico's GPIO HW IRQ (there is only 1 for all pins) during the SPI comment. Looking at the other core code it's even simpler...they just stop all IRQs via noInterrupts() when you begin a xaction and run interrupts() when the transaction finishes.

So right now you could noInterrupts() around the main code's SPI processing and be guaranteed the IRQ won't come in and trounce your work. Or, if you have 2 IRQ handlers doing SPI work, then by construction only one will run at a time (i.e. you can't get another GPIO interrupt until you return from the first one) which means it's safe w/o any work on your part.

obdevel commented 1 year ago

I think I get the idea now, but just for confirmation the controller chip has an IRQ line going to one of the Pico's GPIOs, and when it asserts you need to use SPI to read out the data before it overflows from within your GPIO IRQ handler.

Correct.

You'd like to have other chip also driven by IRQ and on the same SPI bus, and need to make sure while one Pico IRQ handler is doing SPI work another doesn't come in.

Possible, but unlikely, and certainly not for my current use cases.

Or, conversely, you want to use the SPI bus in the main app (not IRQ driven) and need to make sure it doesn't do work while the other SPI IRQ handler is running.

Other way around. Prevent the ISR from running while the main code is doing SPI stuff.

Basically it seems like you're just disabling the Pico's GPIO HW IRQ (there is only 1 for all pins) during the SPI comment. Looking at the other core code it's even simpler...they just stop all IRQs via noInterrupts() when you begin a xaction and run interrupts() when the transaction finishes.

Aha. I'd presumed there was finer granularity, given that SPI.usingInterrupt() takes an interrupt/pin number. Suspending all interrupts may have a minor but cumulative effect on other things, e.g. millis(), micros(), etc. Suspending all pin change interrupts would be fine. (Of course, on the original Uno/Nano, there were only two external hardware interrupt pins to worry about).

So right now you could noInterrupts() around the main code's SPI processing and be guaranteed the IRQ won't come in and trounce your work. Or, if you have 2 IRQ handlers doing SPI work, then by construction only one will run at a time (i.e. you can't get another GPIO interrupt until you return from the first one) which means it's safe w/o any work on your part.

Sadly, the 3rd party libraries I use assume that SPI transactions are implemented. I don't know whether those authors could be convinced to do this work. In my current project, I isolate this chip on one SPI bus, and the other devices (display, touchscreen, SD card) share the second one.

A further benefit of SPI transactions is that each device can have its own settings, e.g. if a device needs to run at a lower SPI bus frequency or different endianness than others it shares the bus with (see the SPISettings class). Without this, all devices on the same bus must run at the speed of the slowest device, or you have to "re begin()" the SPI object for each interaction.

I appreciate there's a fair amount of work to implement the full API, as there is the SPISettings class to consider as well. As I said, this is by no means urgent as I have workarounds in my current projects. But for the future, it'd be nice to have.

Thanks again.