Ide 1.5.x AVR and sam (ARM) SPI atomicity fixes

[xxxajk]

These are the fixes for interacting with SPI methods without accidentally stepping on them from some other place, such as a scheduler, or other non-pin interrupt source. It also prevents updates to the variables from being interrupted by a pin interrupt, which can corrupt the mask/stored mask. and other states.

AVR is 100% tested, and solid.
sam is untested because I lack the hardware to do so.

[cmaglie]

Hi @xxxajk,

wow, at a first sight it looks like there is a lot of work behind this patch.

BTW it's very difficult to review your changes because there are a lot of whitespace/indent changes: if you look at the diff here https://github.com/arduino/Arduino/pull/2376/files it seems that you changed the whole file.

May I ask you to remove all the whitespace/indent changes?

I see also that the "atomicity" fix is just one of many fixes. It would be really great if you could isolate each fix into a separate commit. For example I see that you added some NOPs into the SPI.transfer() on AVR, I guess that this change is unrelated with atomicity and having that into a separate commit with 2 lines of comment into the commit itself will help a lot the global understanding and review of your patch.

[PaulStoffregen]

I believe this is based on the copy from Teensy, which differs slightly from Arduino's version. For the patch I submitted to Arduino a few months ago, I did pretty much what you just described... bringing all the code into the Arduino original file with formatting and white space preserved.

On the ARM Teensy code, I had added an experimental feature to turn on a LED if a transaction begin/end mismatch was detected. Looks like that was ported to AVR. I only developed this on ARM and used it briefly while testing the horribly confusing CC3000 library. It's not a feature we discussed on the developer mail list, so I never brought it up, as I didn't want to complicate matters. Still, it's kinda useful when testing.

[xxxajk]

Yes, paul, and i sent you a copy as well for teensyduino on the forum

Christian: sorry abot the whitespace. Just tell git to ignore it
On Oct 19, 2014 2:20 PM, "Paul Stoffregen" [email protected]
wrote:

I believe this is based on the copy from Teensy, which differs slightly
from Arduino's version. For the patch I submitted to Arduino a few months
ago, I did pretty much what you just described... bringing all the code
into the Arduino original file with formatting and white space preserved.

On the ARM Teensy code, I had added an experimental feature to turn on a
LED if a transaction begin/end mismatch was detected. Looks like that was
ported to AVR. I only developed this on ARM and used it briefly while
testing the horribly confusing CC3000 library. It's not a feature we
discussed on the developer mail list, so I never brought it up, as I didn't
want to complicate matters. Still, it's kinda useful when testing.

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

[xxxajk]

git diff -b --ignore-blank-lines

[xxxajk]

I'll take care of the whitespace dilemma. It really isn't a big deal IMHO. I just need some time to do that.
As for why I added nop's, I'm sorry you are unaware of the effect of it. Basically it introduces a small delay that can prevent the wait loop form iterating when running at the maximum speed. This gives you a little more speed, even if it seems counter-intuitive. At lower speeds, it is unnoticed. Watch the output on an oscilloscope when running full SPI speed, and you should see closer back-to-back writes.

[PaulStoffregen]

Some time ago, I did quite a bit of experimenting with the NOP addition. The one that's in my copy gives about a 10% speedup on AVR.

I also didn't bring this and many other minor SPI issues up before, because I didn't want to over-complicate a contribution that was already very "large".

[cmaglie]

@xxxajk
thanks for the explanation on NOPs, very smart trick, didn't know that.

I'm going to remove the indent changes from your commits with meld or kdiff later today and push them on another PR, unless you're already doing it.

[xxxajk]

Go for it
On Oct 20, 2014 8:17 AM, "Cristian Maglie" [email protected]
wrote:

@xxxajk https://github.com/xxxajk
thanks for the explanation on NOPs, very smart trick, didn't know that.

I'm going to remove the indent changes from your commits with meld or
kdiff later today and push them on another PR, unless you're already
doing it.

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

[ArduinoBot]

Can one of the admins verify this patch?

[xxxajk]

I see that half the patch made it into 1.6.0 (the avr half).
Due, not yet.

[facchinm]

Closing as already merged in master (please reopen if I'm wrong)

[matthijskooijman]

I actually think this is only merged for AVR, not SAM or SAMD.

I was going to write that I'm not entirely convinced of the necessity of this atomicity, but looking through the comments it seems making beginTransaction() atomic is indeed relevant (as explained here and here). I'm not convinced yet about the atomicity of begin()/end() and usingInterrupts()/notUsingInterrupts() but these won't really hurt either.

One other change included here that is still missing from SAM/SAMD is the addition of notUsingInterrupts().

I'm not sure if we should reopen this, or just create new issues on the SAM and SAMD repositories to track this?

[facchinm]

I'd go for keeping it closed and create new PRs on sam/samd

[cmaglie]

I was going to write that I'm not entirely convinced of the necessity of this atomicity

About the atomicity fix for AVR I remember your comment here and by re-reading it now I see that at the times we didn't realize that the operation:

Main: SPI_AVR_EIMSK &= ~interruptMask;

is probably not compiled as a single-instruction but as a read+set+write. My feeling is that taking this into consideration we may optimize the code and remove the guards. Anyway, the SPI lib is now well tested and untouched for a very long time, changing it again will require a lot of validation test I don't have the time to look again into this.

About the atomicity fix for SAM, since we hadn't any report about issues on SPI, my feeling is that the guard is not needed but, again, this is totally empirical.

[matthijskooijman]

My feeling is that taking this into consideration we may optimize the code and remove the guards.

I'm not sure why that would be? Wouldn't this only be more reason to do this atomically? Consider a case where another pin ISR runs (e.g. one that is not in interruptMask) that disables itself. If that happens between the read and write, that other interrupt could be re-enabled by this code, which is clearly not intended.

About the atomicity fix for SAM, since we hadn't any report about issues on SPI, my feeling is that the guard is not needed but, again, this is totally empirical.

I'm pretty sure it is needed, for the same reasons on AVR, but it perhaps it doesn't show up so much due to the limited number of users, or because they don't often do tricky things in ISRs? In any case, it would be good to at least create an issue in both repositories to track this, probably with a link to my comment above (which again links to various other relevant comments). I don't have time to create the tickets right now, though.

[cmaglie]

Wouldn't this only be more reason to do this atomically? Consider a case where another pin ISR runs (e.g. one that is not in interruptMask) that disables itself. If that happens between the read and write, that other interrupt could be re-enabled by this code, which is clearly not intended.

oh, you're right :-)

I'm pretty sure it is needed, for the same reasons on AVR, but it perhaps it doesn't show up so much due to the limited number of users

No, my thought is that on ARM disabling the ISR is already atomic (-> is done in a single instruction), but this is just a thought, all to be verified :-)

[matthijskooijman]

No, my thought is that on ARM disabling the ISR is already atomic (-> is done in a single instruction), but this is just a thought, all to be verified :-)

Ah, but that still leaves the issue with the static interruptSave variable that you pointed out here and expanded by me a bit more here. OTOH, in that last comment I note that this might indeed by AVR-specific, so if it's timed differently on ARM, it might indeed be safe already (needs to be checked, though).

I also just realized that the race condition I described here:

Consider a case where another pin ISR runs (e.g. one that is not in interruptMask) that disables itself. If that happens between the read and write, that other interrupt could be re-enabled by this code, which is clearly not intended.

is a lot broader then I thought. If, at any point during an SPI transaction, a pin interrupt (that is or is not in interruptMask is disabled), it will be re-enabled by endTransaction(). For interrupts that are in interruptMask, this is hard to fix (but also less of a problem: The ISR itself cannot run to disable itself, so it must be the code that's in control of the begin/endTransaction that does it, which should then just be written differently). For interrupts that are not in interruptMask we should probably fix this. I think fix would be to replace this line by:

SPI_AVR_EIMSK |= (interruptSave & `interruptMask`);

(or possibly do the masking with interruptMask in beginTransaction already to more resilient to changes in interruptMask during a transaction, even though we don't really support that)