olikraus / ucglib

Arduino True Color Library for TFTs and OLEDs
https://github.com/olikraus/ucglib/wiki
Other
261 stars 76 forks source link

Should not call SPI.beginTransaction only at startup. Not friendly to other devices. #113

Open edjubenville opened 5 years ago

edjubenville commented 5 years ago

The ucglib code calls SPI.beginTransaction only at startup, and SPI.endTransaction only at termination. This causes problems when there are any other SPI devices in use because those device drivers call SPI.beginTransaction themselves and override the settings. This problem can manifest in at least two ways: 1) SPI speed change. In my case, it became much slower making the display update painful to watch. 2) SPI communications can hang if another SPI devices uses interrupts because the interrupt may occur during the middle of a background display-related SPI transfer. Interrupt-driven SPI drivers essentially demand that all other SPI drivers cooperate by using SPI.beginTransaction and SPI.endTransaction.

It would be problematic to add lots of beginTransaction calls throughout the library, so I implemented an external hack that fixed the problem, at least for me: a) In ucg_com_arduino_4wire_HW_SPI(), I used "new" to create the SPISettings() structure and saved the pointer, then used it for SPI.beginTransaction. b) I added two new functions (which I called ucg_BeginSPI and ucg_EndSPI) that my code can call before and after it initiates anything that updates the display. Function ucg_BeginSPI calls SPI.beginTransaction to reapply the SPISettings. Function ucg_EndSPI calls SPI.endTransaction.

My recommendation is to officially add library hooks for beginning and ending SPI transactions, as I did with my hack.

AnHardt commented 5 years ago

Doing SPI or I2C from an interrupt is always a bad idea. Takes much to long. If, at all, use separate hardware, a mutex or a kind of queue in a separat transfer task. Else you will ruin the main programs data transfer.

edjubenville commented 5 years ago

From what I can determine, support of interrupt-driven SPI operations was an explicit goal behind the invention of the SPI.beginTransaction paradigm. Take a look at the code down below from the standard Arduino SPI library. The noInterrupts() call protects the main program from harm by an untimely interrupt.

As a system developer using multiple device drivers all required to share a single SPI bus, I have little control over whether the multiple SPI drivers step on each other's toes. That responsibility rightfully belongs to the author of each SPI driver to ensure its compatibility with other SPI drivers. For that reason, I contend that ucglib should provide, at a minimum, a public interface that allows my application effect calls to SPI.beginTransaction and SPI.endTransaction to protect from untimely interrupts. Ideally, though, I shouldn't have to do anything like that, i.e., the driver should just take care of the begin/end transaction logic internally.

void SPIClass::beginTransaction(SPISettings settings) { if (interruptMode != SPI_IMODE_NONE) { if (interruptMode & SPI_IMODE_GLOBAL) { interruptSave = interruptsStatus(); noInterrupts(); } else if (interruptMode & SPI_IMODE_EXTINT) EIC->INTENCLR.reg = EIC_INTENCLR_EXTINT(interruptMask); }

bradanlane commented 3 years ago

This is a show stopper for me. I implemented my code and the display was blank and everything on my ESP32 was very slow to start. Then I discovered this open issue. My application uses SPI to communicate with another device. The application does wrap its SPI operations with SPI.beginTransaction() and SPI.endTransaction() .

ccvelandres commented 3 years ago

This was also a showstopper for me. The SPI library for the ESP32 uses mutex when calling beginTransaction and endTransaction. Which means, other libraries cant exists with this library since this library doesnt free the mutex. Without the wrapping the SPI operations in this library, this effectively defeats the purpose of SPI being called a "bus" and allowing other devices to share the same bus.