[AVR] SPI atomicity fix avr (part 1/2 of #2376), second try

[cmaglie]

Follow up #2381.

/cc @PaulStoffregen @matthijskooijman @xxxajk

[matthijskooijman]

From the previous, I think some of my comments haven't been responded to and are still relevant:

(about nops for speed)
I wonder if a comment in the code to (shortly) describe the point of these nops would make sense? git blaming is not always easy after code moves around a bit.

Why is SPIClass::end() prevented when interruptMode is set? That looks rather arbitrary. If you're trying to solve multiple libraries each doing begin/end, the proper solution is probably to do reference counting instead of this.

I also suggested the use of ATOMIC_BLOCK, but since that's just a matter of syntax, just use the manual stuff now and perhaps do a global pass of using ATOMIC_BLOCK if we think it's a good idea later.

(about the SPI_ATOMIC_VERSION define)
Is this really needed? I guess the code will not be able to automagically workaround atomicity issues when it finds the SPI library doesn't have the fixes (as opposed the with SPI_HAS_TRANSACTION where it can just use the older API). If it's just for notifying the user, the library can just check the ARDUINO version number instead for the version it knows contains the right fixes? @PaulStoffregen, you might have an funded opinion on this?

Regarding the last commit, I'm not too happy with that commit message. The commit really still contains a couple of different changes, and the message doesn't really help in understanding what's happening and why. I'd propose the following commit message, just for the begin/endTransaction part:

Fix atomicity issues in SPI::beginTransaction and SPI::endTransaction (Andrew Kroll)

Previously, it could happen that SPI::beginTransaction was interrupted by an ISR, while it is changing the SPI_AVR_EIMSK register or interruptSave variable (it seems that there is a small window after changing SPI_AVR_EIMSK where an interrupt might still occur). If this happens, interruptSave is overwritten with an invalid value, permanently disabling the pin interrupts.

To prevent this, disable interrupts globally while changing these values.

Trying to write a commit message for the other change, disabling interrupts in usingInterrupt, it seemed to me that that change is not actually needed at all. usingInterrupt only modifies interruptMode and interruptMask and beginTransaction / endTransaction only read these variables. Since usingInterrupt is not allowed to be called from an ISR or inside a transaction, there should be no way a variable to become invalid. This same thing should hold for notUsingInterrupt. @cmaglie, could you try your earlier testcase with the atomicity in usingInterrupt and notUsingInterrupt removed?

[PaulStoffregen]

Recently I've been working on Adafruit's nRF8001 (Bluetooth LE) library. It's the third of the 3 widely used libraries where SPI conflicts occur due to interrupts. The other 2 are CC3000 and RadioHead, which I ported some time ago.

The last thing I want to do is make this already-difficult contribution even more complicated, or to delay 1.0.7, but I'm sad to say it seems the beginTransaction() API isn't quite enough for nRF8001. I'm still very actively working on this, so I'd prefer to comment in more detail within the next couple days, after I've had a chance to really work on this more.

[mikaelpatel]

I believe there is a higher atomic level missing here. Shouldn't there be a way to acquire and release the SPI hardware module to allow several SPI transfers that must be run together?

An example is the S25FL127S Flash. To erase a block two commands are transfered. The first to enable the operation and the second the actual erase command.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI/Driver/S25FL127S.cpp#L80

Each begin-end operation is atomic with regard to updates of the internal data in the SPI driver but the high level operation also needs to be atomic with regard to the SPI resource. This is necessary for multiple SPI devices (interrupt sources) and/or multi-tasking with several task using the SPI resource.

The Cosa SPI interface adds acquire/require on the SPI driver resource to handle this.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.hh#L175
https://dl.dropboxusercontent.com/u/993383/Cosa/doc/html/da/d8d/classSPI.html#a83e6f51c6e4527849cc892470ae4669a

The Cosa SPI begin/end operations does a little more than the Arduino version as they also handle the chip select.

Cheers!

[PaulStoffregen]

I believe there is a higher atomic level missing here. Shouldn't there be a way to acquire and release the SPI hardware module to allow several SPI transfers that must be run together?

The API allows this. All you have to do is surround the 2 SPI operations with a single SPI.beginTransaction() and SPI.endTransaction(). You can assert and de-assert chip selects or do pretty much anything you want during a SPI transaction.

[matthijskooijman]

@mikaelpatel, I'm not sure I understand what you mean? Do you mean we need support for transactions consisting of multiple transfers, e.g. multiple SS ping toggles? We have that - because nothing in the SPI library toggles the SS pin for you, you can just begin a transaction and then toggle the SS pin as often as you like.

Looking at the Cosa code, it seems that Arduino's begin/endTransaction are essentially Cosa's aqcuire/release and there is no Arduino equivalent of Cosa's begin end (these just toggle the SS pin, which is done manually in Arduino).

[mikaelpatel]

@PaulStoffregen @matthijskooijman
Got it! All the new interrupt code was a bit confusing. Please ignore my comment. The difference is only the style of disabling/enabling interrupts and who is responsible for device addressing (SS pin toggle). Both works just fine! Cheers Mikael.

[matthijskooijman]

@PaulStoffregen, I've taken the liberty to create a new issue with the comment you posted and delete it here. I feel the issue you indicated is pretty orthogonal to this PR, so best keep discussion separate. See #2453.

[cmaglie]

@matthijskooijman

Why is SPIClass::end() prevented when interruptMode is set? That looks rather arbitrary. If you're trying to solve multiple libraries each doing begin/end, the proper solution is probably to do reference counting instead of this

Agreed. We can use the initialized flag (by transforming it from boolean to uint8_t) to keep track of the number of begins. When the number of end match the number of begin we can turn the interruptMode back to 0 too I guess?

[cmaglie]

I've just pushed a revision that address some of the notes from Matthijs.
Not yet tested with @xxxajk's test examples.

[xxxajk]

Would be nice to be able to not have an ISR declared at all too, so that
someone could write one that is a naked isr. Perhaps that can be held off
till a later version, though.

On Tue, Nov 18, 2014 at 2:33 PM, Cristian Maglie notifications@github.com
wrote:

@matthijskooijman https://github.com/matthijskooijman

Why is SPIClass::end() prevented when interruptMode is set? That looks
rather arbitrary. If you're trying to solve multiple libraries each doing
begin/end, the proper solution is probably to do reference counting instead
of this

Agreed. We can use the initialized flag (by transforming it from boolean
to uint8_t) to keep track of the number of begins. When the number of end
match the number of begin we can turn the interruptMode back to 0 too I
guess?

—
Reply to this email directly or view it on GitHub
#2449 (comment).

Visit my github for awesome Arduino code @ https://github.com/xxxajk

[matthijskooijman]

Changes look good. I'm still not convinced that usingInterrupt / notUsingInterrupt needs to be atomic, but it shouldn't hurt (they're not called often enough to care about peformance).

I do feel we should be dropping SPI_ATOMIC_VERSION - it does not seem to serve any important purpose and it sets a precedent to add a new macro for every bugfix we make, resulting in total chaos.

We decided that usingInterrupt and notUsingInterrupt should not be called from ISR context, and not during a transaction (I think this also applies to begin and end, though I don't think we made that explicit yet). We decided to "enforce" this by documentation. It would be good to start by documenting this in the source code by adding some comments?
Comment about !ISR

[xxxajk]

The entire idea of SPI_ATOMIC_VERSION is so you DON'T need to add any new
macros!

Next major fix you increment the number instead.

On Wed, Nov 19, 2014 at 3:35 AM, Matthijs Kooijman <notifications@github.com

wrote:

Changes look good. I'm still not convinced that usingInterrupt /
notUsingInterrupt needs to be atomic, but it shouldn't hurt (they're not
called often enough to care about peformance).

I do feel we should be dropping SPI_ATOMIC_VERSION - it does not seem to
serve any important purpose and it sets a precedent to add a new macro for
every bugfix we make, resulting in total chaos.

We decided that usingInterrupt and notUsingInterrupt should not be called
from ISR context, and not during a transaction (I think this also applies
to begin and end, though I don't think we made that explicit yet). We
decided to "enforce" this by documentation. It would be good to start by
documenting this in the source code by adding some comments?
Comment about !ISR

—
Reply to this email directly or view it on GitHub
#2449 (comment).

Visit my github for awesome Arduino code @ https://github.com/xxxajk

[matthijskooijman]

The entire idea of SPI_ATOMIC_VERSION is so you DON'T need to add any new macros!

That applies to fixes related to SPI and atomicity, but what about a fix to the Wire library? digitalRead operation? Millis()? etc. We can add version numbers for all of those, but why not just use the global version number (ARDUINO) we already have?

[xxxajk]

Personally, I would place a version number on them all.

On Wed, Nov 19, 2014 at 8:36 AM, Matthijs Kooijman <notifications@github.com

wrote:

The entire idea of SPI_ATOMIC_VERSION is so you DON'T need to add any new
macros!

That applies to fixes related to SPI and atomicity, but what about a fix
to the Wire library? digitalRead operation? Millis()? etc. We can add
version numbers for all of those, but why not just use the global version
number (ARDUINO) we already have?

—
Reply to this email directly or view it on GitHub
#2449 (comment).

Visit my github for awesome Arduino code @ https://github.com/xxxajk

[cmaglie]

Just added the comment about usingInterrupt()/notUsingInterrupt().

Tested on the @xxxajk's first test: it's fully working.

About the SPI_ATOMIC_VERSION, this is a single variable that can be checked across different platforms/cores. I'm wondering that also SPI_HAVE_TRANSACTIONS could be inferred from the global ARDUINO variable, but having it allows to detect if SPI transaction are available even on non-arduino platforms where the ARDUINO variable is not present, and the same is valid for SPI_ATOMIC_VERSION, so I'm for keeping it.

If there are no more concerns I'll go forward and merge this on 1.5 and 1.0.

[xxxajk]

The other point is that someone may have an SPI library in their arduino/libraries directory, so, it makes sense overall. That's the additional intention. it would be beneficial for all libraries to have some sort of version number for this reason alone. Technically libraries are add-ons.

[xxxajk]

Same needs to be done on Due and Zero. I have not had time to write the Zero code, since I am busy currently with a full rewrite of the USB HOST System code and USB Host Shield driver.