is SPI library interrupt safe ?

[jcmg92]

Please fill the info fields, it helps to get you faster support ;)

if you have a stack dump decode it:
https://github.com/esp8266/Arduino/blob/master/doc/Troubleshooting/stack_dump.md

for better debug messages:
https://github.com/esp8266/Arduino/blob/master/doc/Troubleshooting/debugging.md

----------------------------- Remove above -----------------------------

Basic Infos

Hardware

Hardware: ESP-12
Core Version: 2.3.0

Description

I am working arount SPI based LoRa modules with the Radiohead library. I have met many exceptions and after tracking those down, after having reworked Radiohead libraries to make their interrupt routine safe, I had to do the same inside the arduino/esp8266 SPI library.
Am I wrong when I considered the library not interrupt safe?

I have now modified the SPI library and seem to have a stable design.
Anyway, I have joined the modified files for advice.

SPI_cpp.txt
SPI_h.txt

The last and most delicate issue I had was with inline function "setDataBits". I could not find any way to make this one safe without removing its "inline" attribute?

Any feedback on this?

Settings in IDE

Module: NodeMCU 1.0
Flash Size: 4MB/SPIFS 1MB
CPU Frequency: 160MHz
Flash Mode:
Flash Frequency: 40Mhz
Upload Using: SERIAL
Reset Method: nodemcu

[devyte]

@jcmg92 it would be better to propose your changes as a PR, so that the code can be contrasted against the current version, a review made, and merged in if accepted.

@igrr I don't know enouht about SPI to evaluate this. Do the modifications proposed here make sense? Is this related to the IRAM attribute thing that currently needs to be added to the ISR declarations?

[dalbert2]

For those finding this issue, the answer is NO: the SPI library is not interrupt safe as currently implemented (see also closed issue #3917). Neither are most other core Arduino classes.

One solution, as @jcmg92 points out is to modify the SPI class to place nearly all of its code in IRAM (decorating it with ICACHE_RAM_ATTR or now IRAM_ATTR) and eliminate calls to other classes (notably os_printf). Unfortunately, this uses a lot of precious IRAM and if not implemented as a separate class (e.g. MySPI), would require hand-merging with each update of the Arduino Core. Another solution is to bit-bang the SPI transaction (software SPI):

`
/// SPI bus interface pins
#define MISO 12
#define MOSI 13
#define SCK 14

unsigned char IRAM_ATTR spi_transfer(const unsigned char data) {
register int bit, val=0;
for (bit=(1<<7); bit; bit>>=1) {
if (data & bit) {
GPOS = (1<<MOSI);
} else {
GPOC = (1<<MOSI);
}
GPOS = (1<<SCK);
if (GPIP(MISO)) {
val |= bit;
}
GPOC = (1<<SCK);
}
return val;
}
`

Most SPI peripherals, including LoRa transceivers can be clocked at 10MHz or greater and so can be run this way.

I agree with @devyte that a change to SPI, and other peripherals that might reasonably be used from an ISR should be proposed as a PR. At a minimum, the documentation around interrupts should indicate which classes are safe to use from an interrupt context (very few) and should also link to the MMU documentation so users know how to increase IRAM size. One possible low-impact solution would be to define a compile-time flag such as -D SPI_ISR_SAFE that selects IRAM_ATTR or ICACHE_FLASH_ATTR decorators and en/disables os_printf calls.