SPI transaction API insufficient?

[matthijskooijman]

Originally posted in arduino/Arduino#2449 by @PaulStoffregen:

Please do not mistake the issue I'm about to describe with the transaction scope discussion.

Also, please let me preface this with the caveat that it's still very preliminary. I'm actively working on the nRF8001 library, and it's still very much a work-in-progress.

Nordic's nRF8001 seems to be exposing a weakness in the API, which I didn't consider until now. I had considered the RFM69 radio modules, Wiznet ethernet chips, and CC3000 Wifi module in the design of this API. In all those cases, the chip requests service by asserting an interrupt signal. CC3000 is a bit strange, but I'm not going to discuss is right now.

The problem with nRF8001 is the request to initiate communication is done by asserting chip select, which then causes the chip to request an interrupt. SPI transfers must be blocked during that request-to-interrupt time. The trouble is SPI.beginTransaction() disables all registered interrupts, including the one for nRF8001, which prevents the ISR from responding.

At the moment, I'm working with a kludge where I just turn that specific interrupt back on. That may be the final answer. It may be perfectly acceptable to just leave the SPI API as it is, and accept that nRF8001 (and any future chips from Nordic or others) require this workaround.

Or perhaps the SPI API should be able to accommodate this sort of case? Ideally, what's needed is for beginTransaction() to disable the interrupts being used by all other SPI libraries. That's probably much easier to achieve on Cosa, since each library creates an instance of SPI. On Arduino, we're trying to work within the legacy of the SPI library being a thin, static-only C++ wrapper. Requiring sketches and libraries to create instances of the SPI library is probably far too large of an API change to even consider.

Perhaps beginTransaction() could have an alternate/overloaded version that disables all but a specific interrupt. Or perhaps just a function to re-enable the interrupt would work? Or just pushing the responsibility to do that onto the nRF8001 library may be the answer... but that does mean the nRF8001 lib couldn't rely on the SPI lib and core library to fully abstract the hardware.

Well, unless we end up implementing enableInterrupt() and disableInterrupt() or similar core lib functions, as Matthijs has already worked on.

In hingsight, I really should have worked on nRF8001 much earlier. Sorry about that, and really, the last thing I wanted to do was make this SPI stuff even more complicated. Sadly, it seems Nordic has used a design which requires us to acquire exclusive access to the SPI bus, then assert a chip select, and then respond to the falling edge or low level of an interrupt signal, which doesn't assert until the chip select is asserted.

[matthijskooijman]

Or perhaps the SPI API should be able to accommodate this sort of case? Ideally, what's needed is for beginTransaction() to disable the interrupts being used by all other SPI libraries.

That only works for this particular case. I can imagine libraries that do both SPI transfers from the main loop and from ISRs, which would need their own ISR disabled (though they can of course do that manually). Also, once interruptMode ends up at 2, the scheme you propose breaks down anyway.

The problem with nRF8001 is the request to initiate communication is done by asserting chip select, which then causes the chip to request an interrupt. SPI transfers must be blocked during that request-to-interrupt time.

Doesn't this actually mean that you have to poll for data instead of properly using interrupts? So can't you just assert SS, then poll the "interupt" pin from a loop for a while, do work if it ever gets asserted, then unassert SS and return? I might be misunderstanding what needs to happen though, could you provide a link to the current code?

[PaulStoffregen]

Doesn't this actually mean that you have to poll for data instead of properly using interrupts? So can't you just assert SS, then poll the "interupt" pin from a loop for a while, do work if it ever gets asserted, then unassert SS and return?

Yes, that's one possible approach. It's actually the first way I tried this. It gets thorny because the external interrupt hardware detects the low condition while interrupts are disabled, causing a false interrupt afterwards. There are other issues too, which I didn't fully resolve, but yes, this is one possible way to work around the issue.

Also, once interruptMode ends up at 2, the scheme you propose breaks down anyway.

Yes. Other things break when interruptMode is 2. The code should really have some comments or documentation explaining that interruptMode = 2 is a feature that will likely be removed in the future, when/if Arduino gains APIs and definitions for other interrupts. Users should never intentionally pass inputs to usingInterrupt() that cause it to use that mode. It's only meant to allow future parameters (that aren't defined today) to later become backwards compatible.

[PaulStoffregen]

One alternative is to simply ignore this problem. Libraries wishing to use interrupts this way will simply have to add hardware-specific workarounds, if the SPI API doesn't provide all the needed functionality.

[matthijskooijman]

I had a look at the datasheet, I understand how things can get tricky here indeed. I'm not sure what a clean way to fix this in the SPI transaction API is, though, so I guess working around it might indeed be the way to go...

[PaulStoffregen]

I honestly do not believe there is any perfectly clean solution.

Long-term, if the SPI lib or core lib supported a hardware independent way to disable and re-enable external pin interrupts, I believe that would be a lot cleaner than requiring hardware-specific code inside Adafruit's library.

But politically, perhaps it's easier to have a large mess inside a 3rd party library than a small mess in Arduino's software?

[matthijskooijman]

In the PR, @Nantonos posted:

Hello Paul,

Monday, November 17, 2014, 3:39:39 PM, you wrote:

At the moment, I'm working with a kludge where I just turn that
specific interrupt back on. That may be the final answer.

Perhaps beginTransaction() could have an alternate/overloaded
version that disables all but a specific interrupt. Or perhaps just
a function to re-enable the interrupt would work?

Would it be possible to add a (normally empty) list in interrupts to
NOT disable to SPISettings?

That way, the SPISettings for nRF8001 would specify its particular CS
interrupt, and any other SPISettings would not specify any interrupts
(same as they do now).

I don't know if that is feasible or has bad side effects, its just an
idea that occurred to me on reading your mail.