Make SPI access atomic in Ethernet library

[matthijskooijman]

When using the Ethernet library combined with another library that does
SPI access inside an interrupt handler, the Arduino could crash if that
interrupted occured while the Ethernet library was doing an SPI
transfer.

This commit makes all Ethernet SPI transfers happen atomically (i.e.,
with the global interrupt flag cleared), preventing such conflicts by
delaying all interrupts until after the SPI transfer.

Closes: #384

[chaveiro]

Dont disable global interrupts in Libs it's bad programming and is bad for timed sensitive apps that rely on other interrupts.
See #1442
If you really need to assure no other interrupt writes to SPI, do it with your own code and dont break the lib from general compatibility.

[matthijskooijman]

Tricky thing: You are correct that this might cause a problem with timing sensitive applications, but if we don't disable interrupts, some other lib (RF22 in my case) can hijack the SPI bus and deadlock the Arduino entirely.

I guess this could be fixed in an application by disabling interrupts before writing to the ethernet library, but this:

• Will not work out of the box, possibly causing a lot of problems for (especially) novice users.
• Will disable interrupts for a very long time, while the patches suggested only disable them for the duration of a single SPI transfer.

Which only leaves us with the option of making the atomic access optional (defaulting to atomic, I'd argue), but I'm not sure I really like this option (it will probably get messy code with lot of conditionals).

Perhaps the messyness can be limited by making this a compile time option that does something like #define ETHERNET_ATOMIC ATOMIC_BLOCK or something like that?

[lestofante]

Spi write are fast and potentially used by many sensors, this mean race
condition are a problem. Atomic write is a must for a generic library. Time
sensitive or specific application should implement this class in pure c and
then optimize for their specific need
Il giorno 28/mag/2013 19:20, "Matthijs Kooijman" [email protected]
ha scritto:

Tricky thing: You are correct that this might cause a problem with timing
sensitive applications, but if we don't disable interrupts, some other lib
(RF22 in my case) can hijack the SPI bus and deadlock the Arduino entirely.

I guess this could be fixed in an application by disabling interrupts
before writing to the ethernet library, but this:

• Will not work out of the box, possibly causing a lot of problems for
  (especially) novice users.
• Will disable interrupts for a very long time, while the patches
  suggested only disable them for the duration of a single SPI transfer.

Which only leaves us with the option of making the atomic access optional
(defaulting to atomic, I'd argue), but I'm not sure I really like this
option (it will probably get messy code with lot of conditionals).

Perhaps the messyness can be limited by making this a compile time option
that does something like #define ETHERNET_ATOMIC ATOMIC_BLOCK or
something like that?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1334#issuecomment-18565790
.

[matthijskooijman]

I'll close this pullrequest for now, since it seems that this is not the correct solution for this. Perhaps making the SPI library know a bit more about the various SPI slaves and transfers in progress might help, but I guess there isn't any magical way to reliably use SPI transfers inside a interrupt routine...

[mikaelpatel]

The Arduino SPI library can be improved to 1) handle multiple devices with different clock/phase/mode setting on the same bus, 2) allow usage from interrupt handlers, 3) perform the chip select and 4) USI implementation on ATtiny.

To show how this could be achieved please see the Cosa/SPI driver. mikaelpatel/Cosa#10 and https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.hh (SPI.cpp). Please find the on-line documentation here http://dl.dropboxusercontent.com/u/993383/Cosa/doc/html/da/d8d/classSPI.html. The driver base class is here http://dl.dropboxusercontent.com/u/993383/Cosa/doc/html/dc/dd2/classSPI_1_1Driver.html.

[matthijskooijman]

@mikaelpatel, that code looks very good and exactly shows what I had in mind API-wise. Something like that really makes sense for the Arduino core IMHO, but the code can't be used as-is (since 1) Arduino core needs to keep the original SPI class around for backward-compatibility, 2) The Arduino core doesn't have abstractions like Interrupt::Handler and OutputPin). However, if we can adapt that code to become compatible with the Arduino core, I'd be very happy :-) Perhaps something to bring up on the developers mailing list?