SPI Library Transaction API

[Paul Stoffregen]

I'd like to propose and work on an extended API for the SPI library. Of
course, before beginning, I'm asking here for comments and feedback on
the API, and hopefully a tentative yeah/nay from Cristian.

My goal is improving compatibility between SPI-based libraries. Some
libraries work well together, like Ethernet and SD, but many others
conflict, because of different SPI settings or usage within interrupt
routines.

My hope is to define an API that allows the actual implementation inside
the SPI library to work across different platforms, which may involve
different implementations. But the API should be the same, so libraries
and sketches using SPI can use the functions with the same parameters.

So far, my plan involves 3 new functions:

SPI.beginTransaction(clockRate, bitOrder, dataMode);
SPI.endTransaction();
SPI.usingInterrupt(interruptNumber);

The beginTransaction() function is meant to be called before asserting a
chip select signal and performing a group of SPI transfers. It will
obsolete setClockDivider(), setBitOrder() and setDataMode(). The SPI
library will "know" if any SPI usage involves interrupts and take
appropriate steps to ensure this SPI transaction is atomic. Of course,
endTransaction() ends the transaction, usually right after deasserting
the chip select.

The usingInterrupt() function is how the SPI library will become
interrupt aware. If no calls have been made to SPI.usingInterrupt(),
then SPI.beginTransaction() will leave interrupts fully enabled. But if
any usage of SPI is from interrupts, SPI.beginTransaction() will disable
at least those interrupts. The specific mechanism will likely vary
across platforms, perhaps using interrupt priority levels on ARM and
simple masking or global disable on AVR. The specific implementation
inside the SPI library can be very simple or very sophisticated and can
be changed while keeping the API stable.

The interruptNumber arg is an integer, the same as used with
attachInterrupt(). SPI.usingInterrupt() might also support other
platform-specific numbers, to represent the other interrupts. If an
unknown interrupt number is given, SPI.beginTransaction() should fall
back to global interrupt disable during transactions. This behavior
will guarantee SPI-based libraries always work together. The
implementation within SPI can be improved, without any API change, to
support more selective interrupt masking.

I'd also like to add constants to SPI.h for the clockRate parameter,
based on actual frequencies. These would be automatically adapted by
#ifdefs based on F_CPU. The "DIV" parameters should be deprecated. New
SPI code should use names with actual frequencies, where the library
will use the closest available frequency that does not exceed the spec.

I realize this is a pretty substantial change to a very widely used
library. My goal is to maintain backwards compatibility. The many
libraries using SPI will gradually adopt the new API, perhaps with some
symbol like SPI_HAS_TRANSACTION to test for the new API at compile time.

[Dieter....@online.de]

Hi,

i am just in holidays and don't have access to my old emails I saved.

I wrote for a month, that I found a problem with the Arduino Due and could
it fixed so afre, but not perfect.

The Arduino Due uses two I/O-Lines in the controller to select SD card and
two I/O-lines to select the Ethernet.
Due to inproper initialization, the outpts may conflict and the SD card
device select and the Ethernet device selsect fails.
On startup both devices may fail. That should be fixed too. (Initialize the
four lines on controller startup as Input pullup and all will work
properly.)

In addition the examples for the SD card supplied int the Arduino homepage
should be revised too. Here initialization the SD card should not disbale
the Ethernet device and that has nothing to do with the software as
described. You don't need it if you initilize the controller properly.

Best Regards,

Dieter Burandt



-----Ursprüngliche Nachricht-----
From: Paul Stoffregen
Sent: Tuesday, April 15, 2014 2:02 AM
To: devel...@arduino.cc
Subject: [Developers] SPI Library Transaction API

--
You received this message because you are subscribed to the Google Groups
"Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to developers+...@arduino.cc.

[Mikael Patel]

Please see issue #2013.

https://github.com/arduino/Arduino/issues/2013

I believe this has been discussed a number of times with basically the
same result.

Cheers!

Mikael

[Matthijs Kooijman]

Hi Paul,

your propsal looks good to me. Thanks for investing time in this and
coming up with a workable proposal! I do have few remarks, though.

Firstly, the SPI implementation in Mikael Patel's Cosa has been
referred to as a good example. I think you've been looking at this one
as well. I think there are two things in Cosa that are not in your
proposal:
- Creating a new object for each SPI slave that collects settings like
SS pin, clockrate, etc.
- Automatically toggling the SS pin before and after a transaction.

However, unless I'm mistaken both of these features can be implemented
on top of the API you're proposing, without losing anything AFAICS
(the only case I can think of now is when future SPI hardware would
support or require toggling the SS pin itself). I suspect you probably
realized this already, just wanted to make it explicit.

> So far, my plan involves 3 new functions:
>
> SPI.beginTransaction(clockRate, bitOrder, dataMode);

In the interest of efficiency, would it make sense to split this into:

SPI.settings(clockRate, bitOrder, dataMode) returns spi_settings_t
SPI.beginTransaction(spi_settings_t);

This allows (potentially slow) calculations and lookups for the various
settings to happen once at startup, instead of once for every
transaction. spi_settings_t (which probably needs a better name) can be
a platform-specific type, like a struct with precalculated register
values.

For users that don't want to deal with keeping an spi_settings_t
variable around, we can either overload beginTransaction to also accept
the settings directly, or recommend people to write:

SPI.beginTransaction(SPI.settings(clockRate, bitOrder, dataMode));

which also seems acceptable to me.

> The interruptNumber arg is an integer, the same as used with
> attachInterrupt(). SPI.usingInterrupt() might also support other
> platform-specific numbers, to represent the other interrupts.

I like this approach. However, instead of using other platform-specific
numbers (which I'm not sure can be easily mapped to random other
interrupts like pin change or timer etc.), it's probably better to allow
platform-specific overloads with a custom type. This allows explicitely
specifying the interrupt register and mask. e.g, something like:

AVRInterrupt comp1a(TIMSK1, (1 << OCIE1A));
SPI.usingInterrupt(comp1a);


Do we need a SPI.stopUsingInterrupt function as well, in case an
interrupt is no longer used for a longer period of time?

> I'd also like to add constants to SPI.h for the clockRate parameter,
> based on actual frequencies. These would be automatically adapted
> by #ifdefs based on F_CPU. The "DIV" parameters should be
> deprecated. New SPI code should use names with actual frequencies,
> where the library will use the closest available frequency that does
> not exceed the spec.

Hmm, that suggests that you'll still be passing the clock divider, not
the actual clock rate in Hz? I was under the impression that the
clockRate parameter would just be the rate in Hz (or kHz perhaps),
letting the SPI library figure out the rate at runtime (or, if we add
the SPI.settings() I suggested above and the compiler is smart with
inlining, even at compiletime).

Gr.

Matthijs

[Cristian Maglie]

In data martedì 15 aprile 2014 02:02:40, Paul Stoffregen ha scritto:
> So far, my plan involves 3 new functions:
>
> SPI.beginTransaction(clockRate, bitOrder, dataMode);
> SPI.endTransaction();
> SPI.usingInterrupt(interruptNumber);

Paul, this seems a good approach, that also should save compatibility with
existing software. The increasing problems with SPI library urges a solution,
and your proposal looks fine to me.

I'm wondering if the SPI.beginTransaction should move all of that parameters
together. Moreover, i would expect that beginTransaction toggle also the SS
pin (that is now deferred to the user). What about something like:

SPIDevice sensor = SPI.attachSlaveDevice(SSpin, clockRate, bitOrder,
dataMode);

SPI.beginTransaction(sensor);
SPI.transfer(data);
SPI.transfer(data);
SPI.transfer(data);
SPI.endTransaction();

SPIDevice may be a structure that saves all the settings relative to a device.

> I'd also like to add constants to SPI.h for the clockRate parameter,
> based on actual frequencies. These would be automatically adapted by
> #ifdefs based on F_CPU. The "DIV" parameters should be deprecated. New
> SPI code should use names with actual frequencies, where the library
> will use the closest available frequency that does not exceed the spec.

This is another change in queue for a long time to improve compatibilty across
architectures.

C

[Paul Stoffregen]

On 04/15/2014 01:28 AM, Cristian Maglie wrote:
>
> I'm wondering if the SPI.beginTransaction should move all of that parameters
> together.

That does make the source code look a lot cleaner.

My only concern would be extra overhead if the compiler has to fetch the
settings from memory. Especially on AVR, compile time constants produce
faster and smaller code. My intention, at least for AVR, was to
implement this an an inline function that optionally calls a private
function for the interrupt aware case.

For the non-interrupt case, my hope is to minimize the performance
impact. This API is going to add some overhead to all SPI usage. Today
the settings are established once. Setting the parameters before every
transaction is going to add at least a dozen CPU cycles in front of
every transaction.

> Moreover, i would expect that beginTransaction toggle also the SS
> pin (that is now deferred to the user).

What to do about the SS pin, or whether to do anything at all, is the
part of this proposal I've struggled with the most.

Teensy3 has hardware support for generating the SS signal (like Arduino
Due), so I'm certainly tempted to propose including the SS signal, since
that might greatly benefit my own products.

But I left the SS signal out of this proposal for a few reasons.

#1: My gut feeling is including the SS signal is too big of an API
change. Many authors have designed their libraries with a variety of
techniques for manipulating the SS pin. Ethernet, for example,
hard-codes AVR registers. Adafruit's many display libraries (and
Arduino TFT) write bitmasksts to the registers through pointers.
Patching these libraries becomes quite difficult if the SS pin handling
changes. Many of them do not even have the pin number anywhere other
than their constructor. The SPI library would also be unlikely to
achieve the SS pin performance these optimized libraries currently have.

#2: Some libraries will probably assert SS several times within a
transaction. Ethernet is a likely candidate, partly because it does so
many small transfers that suffering the transaction setup overhead only
once will be beneficial for speed, but also partly because the low-level
code is structured with complex preprocessor macros. It's also
theoretically possible some libraries may need multiple SS assertions to
be an atomic operation.

#3: The SPI library already has an extended API for the SS pin on
Arduino Due. That doesn't necessarily mean the transaction shouldn't
deal with SS, but I'm not sure how this could work together with what's
already done on Due.

[Chris Purssell]

While I like the idea of remembering settings for different SPI devices on the AVR devices, why does it need another API with begin/end etc?

The extended SPI API for the Due already does this pretty neatly... just pass the desired SS pin # as a reference with setBitOrder, setDataMode and setClockDivider, and store the settings. Then pass the same SS again with each transfer. A quick check in each transfer to see whether the last SS used is the same as the current avoids having to unnecessarily set/reset settings each time if the same device is being used. Then use SPI_CONTINUE and SPI_LAST flags to decide whether to toggle the SS. This way it keeps compatibility with existing libraries too.

[Matthijs Kooijman]

Hey Paul,

> Today the settings are established once. Setting the parameters
> before every transaction is going to add at least a dozen CPU cycles
> in front of every transaction.

That's an unfair comparison - today the settings are established once,
and if you need different settings then things simply won't work. It's
true that this adds overhead for the case where all slaves use the same
settings, but this is just the price for supporting different settings,
not some specific downside of the proposed API.

> #1: My gut feeling is including the SS signal is too big of an API
> change. Many authors have designed their libraries with a variety
> of techniques for manipulating the SS pin. Ethernet, for example,
> hard-codes AVR registers. Adafruit's many display libraries (and
> Arduino TFT) write bitmasksts to the registers through pointers.
> Patching these libraries becomes quite difficult if the SS pin
> handling changes. Many of them do not even have the pin number
> anywhere other than their constructor. The SPI library would also
> be unlikely to achieve the SS pin performance these optimized
> libraries currently have.
>
> #2: Some libraries will probably assert SS several times within a
> transaction. Ethernet is a likely candidate, partly because it does
> so many small transfers that suffering the transaction setup
> overhead only once will be beneficial for speed, but also partly
> because the low-level code is structured with complex preprocessor
> macros. It's also theoretically possible some libraries may need
> multiple SS assertions to be an atomic operation.

Can't we make the pin optional? If it's specified, it is asserted. If
it's not specified (-1 or perhaps the NOT_A_PIN constant can be reused),
the code just assumes the SS is already asserted as now. This should
still allow libraries that are complicated to convert or need extra
atomicity to use it, while providing convenience for most others?

> #3: The SPI library already has an extended API for the SS pin on
> Arduino Due. That doesn't necessarily mean the transaction
> shouldn't deal with SS, but I'm not sure how this could work
> together with what's already done on Due.

Hmm, looking at http://arduino.cc/en/Reference/DueExtendedSPI, it seems
that it also contains support for different clock divider settings etc.
per slave pin, so wouldn't that be a problem anyway?

Also, the solution seems simple: If a library uses begin(pin),
setClockDivider(pin, divider) and transfer(pin, byte), the current code
can be preserved as-is.

If the library switches to the new API, it should use begin(),
beginTransaction(...), transfer(byte) and endTransaction().

Two libraries using different APIs should be able to co-exist, and if
all libraries use the new API, I think the code for the old API will be
removed by the compiler. Or am I missing something here?

> >What about something like:
> >
> >SPIDevice sensor = SPI.attachSlaveDevice(SSpin, clockRate, bitOrder,
> >dataMode);
> >
> >SPI.beginTransaction(sensor);
> >SPI.transfer(data);
> >SPI.transfer(data);
> >SPI.transfer(data);
> >SPI.endTransaction();
> >
> >SPIDevice may be a structure that saves all the settings relative to
> >a device.

An extra advantage of something like this is that for the Due, you could
perhaps set up the "NPCS" stuff (which essentially stores a set of SPI
settings for each SS pin, right?) when calling attachSlaveDevice and
skipping the entire setup step at beginTransaction.

Gr.

Matthijs

[Paul Stoffregen]

On 04/15/2014 01:11 AM, Matthijs Kooijman wrote:
> Hi Paul,
>
> your propsal looks good to me. Thanks for investing time in this and
> coming up with a workable proposal! I do have few remarks, though.
>
> Firstly, the SPI implementation in Mikael Patel's Cosa has been
> referred to as a good example.

I spent several hours yesterday studying Mikael's code. It is indeed
some of the most beautiful and elegant source code I've ever seen.

Mikael's approach was actually much of the inspiration for
SPI.usingInterrupt(num).

[Paul Stoffregen]

On 04/15/2014 03:03 AM, Chris Purssell wrote:

While I like the idea of remembering settings for different SPI devices on the AVR devices, why does it need another API with begin/end etc?

This new begin/end is needed for the interrupt-aware feature.  If anything has called SPI.usingInterrupt(num), these new begin/end are used to mask interrupts.

The extended SPI API for the Due already does this pretty neatly... just pass the desired SS pin # as a reference with setBitOrder, setDataMode and setClockDivider, and store the settings. Then pass the same SS again with each transfer. A quick check in each transfer to see whether the last SS used is the same as the current avoids having to unnecessarily set/reset settings each time if the same device is being used. Then use SPI_CONTINUE and SPI_LAST flags to decide whether to toggle the SS. This way it keeps compatibility with existing libraries too.

You're right, the Due extended API could be used on AVR for unique settings per SS pin.  I suspect the code would be quite slow, but your point is valid, that the Due API does provide enough to establish unique settings per SS pin.

This of course assumes the library will manage the SS pin...

[Paul Stoffregen]

On 04/15/2014 03:25 AM, Matthijs Kooijman wrote:
> Can't we make the pin optional? If it's specified, it is asserted. If
> it's not specified (-1 or perhaps the NOT_A_PIN constant can be reused),
> the code just assumes the SS is already asserted as now. This should
> still allow libraries that are complicated to convert or need extra
> atomicity to use it, while providing convenience for most others?

Yes, it probably is feasible to make the library work both ways. The
non-SS pin way probably wants to be a different function (or an
overloaded function) so the decision whether to control the SS pin isn't
checked at runtime.

Clearly there is a strong desire to begin SS pin handling into the SPI
library. I personally think it would be a nice feature, as long as the
non-pin way has good backwards compatibility.

But I do worry this discussion could be turning into "feature creep".

Could this be done as 2 separate projects? The plan could be to
eventually achieve a much more fully featured SPI library. I'd really
like to work on a first phase with limited scope, only to solve these
compatibility problems. If we plan the future API, perhaps with
overloaded functions, then later a second phase (likely developed by
someone other than me) could add the other function that takes an
additional arg for SS pin number.

I personally have a very strong interest in fixing the compatibility
problems between libraries. I want to limit the scope of my
contribution to that part.

[David Mellis]

Cristian, I see the appeal of this, but it feels somewhat counter to the style of most the current Arduino API's, which tend towards a bit more verbosity in exchange for more conceptual simplicity. The API's do use classes often, but usually only as Class.foo() -- instances are rarely passed as parameters. Also, it seems like additional complication to add an SPIDevice class just to avoid passing three (or four) parameters to SPI.beginTransaction(). EthernetUDP.beginPacket() is perhaps a good comparison; it takes an IP and Port parameter instead of a class combining the two.

C

[David Mellis]

A couple of random questions / thoughts (since this is now Cristian's domain, not mine)... 

If the main purpose of beginTransaction() / endTransaction() is to disable some or all interrupts to ensure that the SPI transaction is atomic, why make the API SPI-specific? There are lots of other places where you might want atomic code. We already have noInterrupts() and interrupts(); maybe we need to extend those to allow for selective disabling / re-enabling of specific interrupts?

More specifically, what do you imagine is the relationship between the SPI transaction and a specific external interrupt? If it's just that the external interrupt is doing something (unrelated) that you don't want to interrupt the SPI transactions, it seems especially strange to make disabling that interrupt part of the SPI API. Or is the idea that the SPI transaction would somehow be taking place on a pin used by a particular interrupt / attachInterrupt() handler?

Could we remove the clockRate, bitOrder, and dataMode parameters from beginTransaction() and simply use the existing functions within a beginTransaction() / endTransaction() block? It's more verbose, I know, but also means fewer changes to the API and would avoid adding an duplicate method of setting those parameters.

Also, it does seem to me that if you're thinking about changing / extending the SPI API, it would be good to figure out how to add SS pin handling, since that's probably the most common additional requirement.

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+unsubscribe@arduino.cc.

[David Mellis]

Also, the Due SPI API already has SPI_CONTINUE as a parameter to SPI.transfer(). Maybe it would be better to use that instead of adding beginTransaction() and endTransaction()? Or deprecate SPI_CONTINUE and move the Due over to beginTransaction() / endTransaction()? (I like the begin() / end() better, honestly, since it feels more in line with other Arduino APIs, but it might be too late to change it.)

[Cristian Maglie]

In data martedì 15 aprile 2014 17:55:00, David Mellis ha scritto:
> More specifically, what do you imagine is the relationship between the SPI
> transaction and a specific external interrupt? If it's just that the
> external interrupt is doing something (unrelated) that you don't want to
> interrupt the SPI transactions, it seems especially strange to make
> disabling that interrupt part of the SPI API. Or is the idea that the SPI
> transaction would somehow be taking place on a pin used by a particular
> interrupt / attachInterrupt() handler?

The problem here is in libraries that communicates via SPI *inside* ISR, what
happens is something like:

1) Library A starts an SPI transaction
2) interrupt happens
3) inside ISR Library B starts another SPI transaction, conficting with the
unfinished transaction in 1

Some libraries, like RF22 for example, has very thight timings for serving SPI
data, so it may be hard to move SPI IO outside ISR.

C

[David Mellis]

So the idea is that Library A would disable the ISR that Library B uses SPI inside of? In that case, it still seems more logical to me to do something like noInterrupt(libraryBinterrupt) or detachInterrupt(libraryBpin). Again, there are plenty of other things that could happen in an ISR that would mess with the SPI transaction you're trying to do -- and plenty of reasons why you might want to disable an ISR besides being in the middle of an SPI transaction -- so it seems weird to put the interrupt-disabling API into the SPI library.

Or is the idea that somehow the library B SPI usage inside of the ISR could happen without disrupting library A's SPI transaction (by specifying information about the ISR it's inside of)?

C

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Cristian Maglie]

In data martedì 15 aprile 2014 18:21:31, David Mellis ha scritto:
> So the idea is that Library A would disable the ISR that Library B uses SPI
> inside of?

Well, not exactly, Library A and Library B may be totally unrelated and known
nothing of each other (think about at Ethernet and RF22 for example), they
just uses SPI lib.

> In that case, it still seems more logical to me to do something
> like noInterrupt(libraryBinterrupt) or detachInterrupt(libraryBpin).

You can't do that from library A because it knows nothing about library B.
For example you can't disable RF22 interrupt from Ethernet.

> Or is the idea that somehow the library B SPI usage inside of the ISR could
> happen without disrupting library A's SPI transaction (by specifying
> information about the ISR it's inside of)?

Exactly, the idea is that Library B decalares to the SPI library that he uses
SPI bus inside ISRx, so it's SPI lib that disable ISRx every time a
transcation is started (by A or B or anyone else), to guarantee that the
transaction itself is not corrupted.

C

[Paul Stoffregen]

On 04/15/2014 10:06 AM, Cristian Maglie wrote:
>
> Exactly, the idea is that Library B decalares to the SPI library that he uses
> SPI bus inside ISRx, so it's SPI lib that disable ISRx every time a
> transcation is started (by A or B or anyone else), to guarantee that the
> transaction itself is not corrupted.

Yes, a perfect summary of the interrupt awareness feature!

With this feature, and always establishing the correct setting at the
beginning of each transaction, my hope is to make all SPI-using
libraries "just work" together. Of course, there's a caveat that an
extremely long transaction could delay an interrupt, but I believe this
approach will give the maximum possible compatibility between existing
libraries.

This conversation clearly shows there's a great desire to add more
features to the SPI library. I'm not opposed to adding features, but
I'm personally not very interested in doing so. My main interest is
making the many library combinations that today fail work together as
seamlessly as possible. I believe limiting the scope of a software
project greatly improves the odds of success, so my hope is to focus
only on this compatibility problem. More features can always be added
as a separate effort.

These SPI sharing failures are driving people away from the Arduino
platform, to Linux and complex RTOS systems (or just giving up), for
increasingly common needs, such as using SD card storage and Bluetooth
LE together in the same project. I really want to solve this problem,
and start submitting patches to the main SPI-based libraries, so Arduino
can be a viable platform for these projects that use multiple SPI
devices with libraries from different 3rd party developers.

[David Mellis]

Thanks for explaining. I thought I was missing something!

Thinking about it more, one thing that bothers me is that this requires the SPI library to know about interrupts (even if not very much), which seems opposed to modularity. For example, you could imagine wanting similar functionality for many other libraries. Also, it makes the SPI library dependent on the interrupt API, e.g. if we added attachPinChangeInterrupt(), we'd also need to add SPI.usingPinChangeInterrupt(), etc. (Yes, you could do attachInterrupt(PINCHANGE0) instead, but this is same problem in reverse: the interrupt API is then being constrained by the fact that the SPI library depends on it.) Of course, it's good to solve the problems that people are actually running into (and I believe you, Paul, that these are mostly with SPI) but it would be good to do it in a more general way.

One idea would be for the SPI library to set a flag when it's in a transaction, so that you could code your ISR to refrain from using SPI if there's a transaction in progress, e.g.

void myInterruptHandler()

{

  if (SPI.isTransmitting()) return;

  // ...

}

Or do the actual use cases require the interrupt to be buffered until the current SPI transaction completes?

Another thought is that the typical way for one module to do something (in a flexible way) based on actions in another module is using events. So, for example, a library that uses SPI inside an ISR could do something like:

void BLE::begin()

{

  attachInterrupt(0, bleReceive);

  SPI.onBeginTransaction(disableInterrupt0);

  SPI.onEndTransaction(enableInterrupt0);

  // ...

}

void disableInterrupt0() { disableInterrupt(0); }

void enableInterrupt0() { enableInterrupt(0); }

I know this is more verbose than simply calling SPI.usingInterrupt(0), but it seems much more flexible and modular.

Anyway, just throwing some ideas out there. What do you think?

David

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+unsubscribe@arduino.cc.

[Matthijs Kooijman]

Hey David,

> One idea would be for the SPI library to set a flag when it's in a
> transaction, so that you could code your ISR to refrain from using SPI if
> there's a transaction in progress, e.g.
>
> void myInterruptHandler()
> {
> if (SPI.isTransmitting()) return;
>
> // ...
> }
>
> Or do the actual use cases require the interrupt to be buffered until the
> current SPI transaction completes?

Looking at the RF22 case, the interrupt triggers to indicate the buffer
is nearly full, so data has to be copied out of the buffer Real Soon.
Simply not running the ISR doesn't work. At best it could set a flag and
then let the sketch call RF22.loop() or something in its loop()
function, but it's likely too late by then.

The proposed solution delays the interrupt, which is also potentially
problematic, but at least the RF22 ISR now runs as soon as possible
(without relying on the sketch's loop function), minimizing the chance
of overflow.

> Another thought is that the typical way for one module to do something (in
> a flexible way) based on actions in another module is using events. So, for
> example, a library that uses SPI inside an ISR could do something like:
>
> void BLE::begin()
> {
> attachInterrupt(0, bleReceive);
> SPI.onBeginTransaction(disableInterrupt0);
> SPI.onEndTransaction(enableInterrupt0);
> // ...
> }
>
> void disableInterrupt0() { disableInterrupt(0); }
> void enableInterrupt0() { enableInterrupt(0); }
>
> I know this is more verbose than simply calling SPI.usingInterrupt(0), but
> it seems much more flexible and modular.

This actually looks like a reasonable approach. It's certainly more
flexible - it allows for disabling arbitrary interrupts easily, without
having to add explicit support for all possible interrupt types (even
though I think an "Interrupt" object with .enable() and .disable() is
elegant, it's probably more than needed).

It's important to allow multiple even handlers though, not just one...

[Paul Stoffregen]

On 04/16/2014 07:04 AM, David Mellis wrote:

Thanks for explaining. I thought I was missing something!

Thinking about it more, one thing that bothers me is that this requires the SPI library to know about interrupts (even if not very much), which seems opposed to modularity. For example, you could imagine wanting similar functionality for many other libraries. Also, it makes the SPI library dependent on the interrupt API, e.g. if we added attachPinChangeInterrupt(), we'd also need to add SPI.usingPinChangeInterrupt(), etc. (Yes, you could do attachInterrupt(PINCHANGE0) instead, but this is same problem in reverse: the interrupt API is then being constrained by the fact that the SPI library depends on it.) Of course, it's good to solve the problems that people are actually running into (and I believe you, Paul, that these are mostly with SPI) but it would be good to do it in a more general way.

l believe it would be a terrible mistake to significantly delay a good solution by holding out for a perfect one.  Real users are suffering.  The problem is rapidly growing worse, as more SPI-based wireless modules enter the market.  A common wisdom is already forming, that Arduino as a platform is unsuitable more more than 1 networking or wireless communication device, that such projects need a Raspberry Pi.

I'm not saying we shouldn't consider ideas.  But let's not fall into the trap excessively delaying or losing momentum.

One idea would be for the SPI library to set a flag when it's in a transaction, so that you could code your ISR to refrain from using SPI if there's a transaction in progress, e.g.

This idea is not feasible.

There is no viable way to cause the library to run again when the SPI become available.  Building such a scheme could be done, but if you don't like relatively simple interrupt masking inside the SPI library, you certainly won't like the very complex system of semaphores and callback lists this approach would require.

If such a complex scheme were made, it would be incredibly disruptive to the libraries using SPI.  Instead of simply responding to an event, they would have to handle the event 2 different ways, depending on the flag.  That's asking dozens of libraries to develop and test 2 different code paths for the same thing.  Not good, especially for the many libraries that already exist, are fully debugged, and work well (except when not used with another library using SPI).

void myInterruptHandler()

{

  if (SPI.isTransmitting()) return;

  // ...

}

Or do the actual use cases require the interrupt to be buffered until the current SPI transaction completes?

Another thought is that the typical way for one module to do something (in a flexible way) based on actions in another module is using events. So, for example, a library that uses SPI inside an ISR could do something like:

This could work.  I agree, it is far more flexible.  I can't imagine why it would ever be needed.  Do you have any usage case in mind?

The main drawback would be overhead, especially on AVR.  What would have been only a few instructions to read a flag and mask an interrupt (inline code using only a couple 8 bit registers) becomes a 16 bit pointer to a full function call, which involves saving half the registers on top of the overhead to call through a pointer, only to quickly mask the interrupt and return.  Likewise, at the end of the transaction: similar overhead to call a function instead of just set a bit.

This overhead has to be suffered on SPI transaction.  Some libraries, like TFT, will need to do many transactions just to render each character of a font or draw a complex shape.  If there is a consensus this flexibility would be useful, I could try implementing this and investigate the performance impact on some widely used libraries.

void BLE::begin()

{

  attachInterrupt(0, bleReceive);

  SPI.onBeginTransaction(disableInterrupt0);

  SPI.onEndTransaction(enableInterrupt0);

  // ...

}

void disableInterrupt0() { disableInterrupt(0); }

void enableInterrupt0() { enableInterrupt(0); }

I know this is more verbose than simply calling SPI.usingInterrupt(0), but it seems much more flexible and modular.

Anyway, just throwing some ideas out there. What do you think?

I believe most of the Arduino API is based around simple 0-based integers.  Certainly there are trade-offs with all these decisions.

I really just want to solve this problem, and soon, so when users who wire up a SD card and a RF22 radio, or any number of increasingly common SPI devices, can have their projects just work properly.

David

On Tue, Apr 15, 2014 at 3:55 PM, Paul Stoffregen <pa...@pjrc.com> wrote:

On 04/15/2014 10:06 AM, Cristian Maglie wrote:

Exactly, the idea is that Library B decalares to the SPI library that he uses
SPI bus inside ISRx, so it's SPI lib that disable ISRx every time a
transcation is started (by A or B or anyone else), to guarantee that the
transaction itself is not corrupted.

Yes, a perfect summary of the interrupt awareness feature!

With this feature, and always establishing the correct setting at the beginning of each transaction, my hope is to make all SPI-using libraries "just work" together.  Of course, there's a caveat that an extremely long transaction could delay an interrupt, but I believe this approach will give the maximum possible compatibility between existing libraries.

This conversation clearly shows there's a great desire to add more features to the SPI library.  I'm not opposed to adding features, but I'm personally not very interested in doing so.  My main interest is making the many library combinations that today fail work together as seamlessly as possible.  I believe limiting the scope of a software project greatly improves the odds of success, so my hope is to focus only on this compatibility problem.  More features can always be added as a separate effort.

These SPI sharing failures are driving people away from the Arduino platform, to Linux and complex RTOS systems (or just giving up), for increasingly common needs, such as using SD card storage and Bluetooth LE together in the same project.  I really want to solve this problem, and start submitting patches to the main SPI-based libraries, so Arduino can be a viable platform for these projects that use multiple SPI devices with libraries from different 3rd party developers.

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Victor Aprea]

Paul,

I just want to throw in my support for your proposal. It's becoming an increasingly urgent problem we are facing as we try and integrate more SPI peripherals.

Cheers,
Vic

[Cristian Maglie]

In data martedì 15 aprile 2014 18:12:38, David Mellis ha scritto:
> Also, the Due SPI API already has SPI_CONTINUE as a parameter to
> SPI.transfer(). Maybe it would be better to use that instead of adding
> beginTransaction() and endTransaction()? Or deprecate SPI_CONTINUE and move
> the Due over to beginTransaction() / endTransaction()? (I like the begin()
> / end() better, honestly, since it feels more in line with other Arduino
> APIs, but it might be too late to change it.)

The reason for designing the current SPI extended API with:

SPI.transfer(pin, data0, SPI_CONTINUE);
SPI.transfer(pin, data1, SPI_CONTINUE);
SPI.transfer(pin, data2, SPI_CONTINUE);
SPI.transfer(pin, data3, SPI_LAST);

instead of a more arduino-like:

SPI.beginTranscation(pin);
SPI.transfer(data0);
SPI.transfer(data1);
SPI.transfer(data2);
SPI.transfer(data3);
SPI.endTransaction();

is because the SAM3X needs a bit (LASTXFER) to be set *together* with the last
byte transferred: we cannot do it in endTransaction() because the last byte is
already gone. This LASTXFER bit is needed because the SAM3X CPU handles chip
select (and per-slave configurations) automatically with its NPSC device, and I
imagine that other MCUs have similar features.

Paul, what about:

SPI.beginTransfer(modes....);
SPI.transfer(data0);
SPI.transfer(data1);
SPI.transfer(data2);
// use endTransfer to send last byte and close transaction
SPI.endTransfer(data3);

Is this doable without sacrifing performance? Paul, I understand your pressure
and I don't want to delay this more, I'm asking this because IMHO it will
helps a lot to let the two APIs converge, so we can at some point deprecate
the Extended SPI in favor of the Transaction SPI (or even let them work
together).

C

[Jan Berger]

> is because the SAM3X needs a bit (LASTXFER) to be set *together* with the last
> byte transferred: we cannot do it in endTransaction() because the last byte is
> already gone. This LASTXFER bit is needed because the SAM3X CPU handles chip
> select (and per-slave configurations) automatically with its NPSC device, and I
> imagine that other MCUs have similar features.

Can't you achieve this by storing the bytes to a buffer and performing the transaction on "endTransaction" using a DMA on the SAM3X?
 
Jan
 

 

[Cristian Maglie]

No, the SPI.transfer function is supposed to return the byte shifted from MISO
pin, we can't delay the transfer.

C

[Paul Stoffregen]

On 04/17/2014 03:23 AM, Cristian Maglie wrote:
>
> This LASTXFER bit is needed because the SAM3X CPU handles chip
> select (and per-slave configurations) automatically with its NPSC device, and I
> imagine that other MCUs have similar features.

The SPI hardware in Teensy 3.1 works this same way. I certainly do have
plenty of motivation to make new SPI library that lets people fully
utilize this more advanced hardware.

>
> Paul, what about:
>
> SPI.beginTransfer(modes....);
> SPI.transfer(data0);
> SPI.transfer(data1);
> SPI.transfer(data2);
> // use endTransfer to send last byte and close transaction
> SPI.endTransfer(data3);
>
> Is this doable without sacrifing performance?

Performance is tricky.

I have spent many hours optimizing Ethernet and Adafruit ST7735, and
working with Bill Greiman's SdFat's optimizations. I could write
volumes about these performance issues, but the TL;DR version is
SPI.transfer() is a terrible API as the SPI clock approaches the CPU
clock speed. Good performance on modern hardware requires a different
API, that at a very minimum allows the next write to begin before
returning the previous read.

TFT displays like ST7735 and ILI9341 are probably the most performance
sensitive SPI libraries. For these, you really need an API that allows
the SPI hardware to control 2 select signals, and one sometimes wants to
change mid-transaction (both Arduino Due and Teensy 3.1 have this
multiple-signal capability in hardware). Here's a modified copy of
Adafruit_SS7735 where I've experimented with this and other performance
issues. If you see this running on a TFT display, it's pretty
incredible how much faster the graphics tests run.

https://github.com/PaulStoffregen/Adafruit_ST7735

My point is good performance requires much more than such a simple API
change.

> Paul, I understand your pressure
> and I don't want to delay this more, I'm asking this because IMHO it will
> helps a lot to let the two APIs converge, so we can at some point deprecate
> the Extended SPI in favor of the Transaction SPI (or even let them work
> together).

Long-term, I really want to make a SPIv2 library that has a much better
API. But it's an incredibly large and difficult project, requiring a
lot of work to truly investigate needs of a lot of different SPI
applications, and made even harder by crafting things in such a way that
it achieves at least the same baseline performance when used with AVR's
limited SPI port and slow CPU.

API-wise, I believe chip select should not be integrated into the
transaction functions. There are simply too many unusual SPI
applications where that's an incorrect to assume a transaction
corresponds to a single chip select assertion. The Ethernet library is
one example where a single transaction will probably be involve chip
selects and short transfers. The Open Music Labs codec shield is
another example, where SPI transfers emulate I2S protocol, with half
them asserting the signal one way and the other half the other way.

I believe the best API design is to use the transaction functions only
for guaranteeing the correct SPI configuration and exclusive access to
the SPI hardware.

I really do want to make Arduino contributions for compatibility AND
performance. But the reality is there are only so many hours in each
day (a couple consumed by this email conversation). I just can't do all
of this right away. The compatibility issue is pretty big as-is, and
urgently needed. Adding a significant API overhaul is just more than I
can contribute at this time. Believe me, I want to do so. Teensy 3.1's
SPI is under-utilized by Arduino libraries due to the limited API, so I
really do want this. It's just more than I can contribute at this time.

[Rui Azevedo]

this would be a great idea, i was just wondering about it (my code needs it).

i need a base class for SPI and I2C supporting also software emulated, and eventually Serial

Can we make a supper class of that?

[Jan Berger]

As I understand this the API we have is blocking, and I guess that is forced from Atmel 8-bit design? I have not looked into the details of Atmel on this, so correct me if I am wrong. As for performance in general - I never had any real performance issues with Atmel 8-bit running at 16Mhz. But, I do have latency issues in the sense that I get blocked on some API calls.
 
At some point we should consider a non-blocking API because that would be a significant "performance" boost.
 
Jan

[Victor Aprea]

Jan,

A major facet of this discussion is the inherent necessity of SPI being "blocking" in many cases. For many device interaction patterns (transactions), the SPI bus itself is effectively a resource which must not be yielded in the midst of that interaction. Microcontroller programs are generally single-threaded, except for the huge caveat of interrupts. 

When an interrupt happens, the software handler for that event takes control of the processor no matter what it was doing before (saving and restoring the registers on the stack of course). Many SPI devices, in addition to their SPI interface, use an additional signal to interrupt the processor informing it of a time sensitive need for interaction.

However, the interrupt handlers have no idea what resources were being used at the time, or what bus activity may have been in progress at that time, and if they are serviced by engaging in some SPI activty, then that that previous transaction gets left in an undefined state, and nobody ends up happy, especially since probably more than one SPI slave thinks its being talked to. At that point and bus contention and processor resets are somewhere in the range of possible to likely.

Sure, you can have every invocation "that cares" about the above problem wrap it sei() / cli() calls. If your controller as independent SPI buses though, and your interrupt handler is using one SPI bus and your main loop is using a different one, that's an aggressive approach since you don't have an actual resource contention issue.

That's my understanding of it anyway.

Regards,
Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

M: 607.745.1515

T: @vicatcu

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Mikael Patel]

I did some work in this area a few months back when designing the SPI class structure for Cosa. Below are the requirements I came up with:

1. Allow several SPI devices with different settings (phase, mode, frequency).
2. Allow SPI device drivers with interrupt handlers without deadlock or SPI transaction violations.
3. Handle chip select (low/high assert and pulse)
4. Support USI/Tiny (all SPI modes)
5. Support SPI slave

The design became a common base class, SPI::Driver and a new "transaction block" with begin(SPI::Driver* dev) and end(). The typical usage is:

spi.begin(this);
spi.transfer(data);
...
spi.end();

The begin() does the following operations;
1. Disables the SPI device driver interrupt handlers.
2. Sets the SPI hardware registers
3. Asserts the chip select

The end() basically does the reverse;
1. Removes the assert on the chip select
2. Enables the SPI device driver interrupt handlers.

The SPI manager holds a list of the SPI device drivers and their interrupt handlers.

Below are links to the API, implementation and typical usage:
1. API, https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.hh
2. Implementation, https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.cpp
3. Usage in the W5100 driver, https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/Socket/Driver/W5100.cpp#L53

Please note that this is only an AVR design and not SAM (or DMA). I would approach SAM and DMA with a RTOS and kernel buffers. Most AVR MCUs do not have SRAM for this and Cosa is targeted for seamless Tiny to Mega.

Cheers! Mikael

[Paul Stoffregen]

Mikael, your Cosa project is truly beautiful.  I've read source code from many projects and nearly all widely used Arduino libraries.  Rarely have I see APIs and implementations with elegance even approaching Cosa.

I really like how you've provided a room() function.  Arduino really needs something like that!

Over the last few days I've put quite a bit of time into analyzing how several existing libraries really use SPI.  The more I look into this, the more I believe SPI chip select should have its own API rather than being rolled into the transaction level.

For example, Ethernet involves many small transfers that require separate assertions of SS.  Even just reading a 16 bit register on the W5100 chip requires two sets of 4 byte transfers.  There are places where a 16 bit register is read repeatedly until 2 consecutive readings agree, to guard against reading the MSB and LSB from different values.  Maybe these should be single transactions, even though they involve asserting the SS pin several times?

Really, I'm asking here.  I'm still studying how several libraries really work and where indivisible SPI transactions really make sense.

[Mikael Patel]

On 04/18/2014 10:01 PM, Paul Stoffregen wrote:

Thanks Paul for the encouraging words.

Mikael, your Cosa project is truly beautiful.  I've read source code from many projects and nearly all widely used Arduino libraries.  Rarely have I see APIs and implementations with elegance even approaching Cosa.

One of the Cosa project goals is to provide some of the best practices from industry strength large scale embedded software development. And yes I put a lot of work into rewrites to make the code as easy to read as possible. As I am using more OOP and more of the C++ language there is a need for lots of documentation and example sketches. The doxygen style documentation has started to pay-off especially in the more modern IDEs such as embeddXcode, Eclipse, etc. I would encourage requiring more source level documentation in Arduino even though I understand the legacy from Processing and Wiring.

I really like how you've provided a room() function.  Arduino really needs something like that!

I guess you have already seen the approach to UART handling and the way an IOBuffer class make the code so much easier. This style is also applied to the W5100 Ethernet controller where the IOStream binding allows zero buffer in the MCU as the buffer is directly mapped in the device memory instead. This allows a simple web-server to reduce SRAM requirements to less than 100 bytes.

Over the last few days I've put quite a bit of time into analyzing how several existing libraries really use SPI.  The more I look into this, the more I believe SPI chip select should have its own API rather than being rolled into the transaction level.

For example, Ethernet involves many small transfers that require separate assertions of SS.  Even just reading a 16 bit register on the W5100 chip requires two sets of 4 byte transfers.  There are places where a 16 bit register is read repeatedly until 2 consecutive readings agree, to guard against reading the MSB and LSB from different values.  Maybe these should be single transactions, even though they involve asserting the SS pin several times?

Yes, the devil is in the details so let us have a look at some detailed examples. In Cosa I do this:
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/Socket/Driver/W5100.cpp#L68

The chip select pin is a valid part of the SPI::Driver base class so it is fine for the driver to assert the pin as many times as needed within the transaction block. Having to restart the transaction is very inefficient at this would turn on and off the interrupt handlers, etc. BW: The W5100 Ethernet chip is a marvel of inefficient SPI ;-).

Another "strange" requirement is from the SD driver. It needs to start at low speed, check the type of card and then increase the clock. There is a set_clock() member function in the SPI::Driver class.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.hh#L112
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI/Driver/SD.cpp#L190

Yet another example is the shift register LCD adapter which requires a pulse at the end of the transaction. The eight bit write to the LCD even has a pulse in the "middle" of the transaction.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/LCD/Driver/HD44780_IO_SR3WSPI.cpp#L50

BW: Sometimes the chip requires synchronization on the MISO pin. This is the case for the CC1101.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/Wireless/Driver/CC1101.cpp#L114

Really, I'm asking here.  I'm still studying how several libraries really work and where indivisible SPI transactions really make sense.

I have followed this discussion and it is really a good example of the sharing of ideas within the Arduino community. Great inspiration!

If I get some time I will have a look at the SAM SPI and DMA and see if I can contribute with some more detailed ideas on how to approach that (and if we can have a common interface).

Cheers! Mikael

[Paul Stoffregen]

On 04/18/2014 02:15 PM, Mikael Patel wrote:

The chip select pin is a valid part of the SPI::Driver base class so it is fine for the driver to assert the pin as many times as needed within the transaction block.

Oh, I missed that.  I see it now in the block write function.

BW: The W5100 Ethernet chip is a marvel of inefficient SPI ;-).

I agree, the W5100 SPI is terrible.  Zero buffering in the socket class compounds the problem, since so many W5100 registers need to be so inefficiently accessed for pretty much anything.  I know RAM is tight on small AVR chips, but someday (when I have lots of free time...) I'm going to experiment with modest buffering in the socket class, for use on ARM and the bigger AVR chips.  My gut feeling is even a little buffering could make a huge speedup for many sketches using Ethernet.  I'm a little surprised nobody else has tried this.  Or maybe they have and I just don't know about such projects?

Yet another example is the shift register LCD adapter which requires a pulse at the end of the transaction. The eight bit write to the LCD even has a pulse in the "middle" of the transaction.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/LCD/Driver/HD44780_IO_SR3WSPI.cpp#L50

I noticed that pulse code earlier, but didn't fully understand it or what the number 2 means.  Now I see it's documented in SPI.hh.

    /** Chip select pulse width; 
     *  0 for active low logic during the transaction,
     *  1 for active high logic,
     *  2 pulse on end of transaction.
     */
    uint8_t m_pulse;

These unusual cases, plus Arduino's need for backwards compatibility, are why I'm so reluctant to put chip selects into the transaction begin/end functions for Arduino.  Well, I also want to limit my project's scope, so maybe that desire is biasing my opinion?

Still, my worry about bringing chip select into the transaction level (for Arduino's SPI library) is something like m_pulse could need to grow to many special cases.  That's much easier for a system like Cosa where everything is released together as one (very nicely) integrated package, but it seems problematic for Arduino, where many 3rd party developers build higher level libraries on top of the low level library Arduino publishes.

I do have one other question on the begin() implementation.  Shouldn't the interrupt disable happen before asserting the config and chip select?  Seems like an interrupt could occur during that very brief time, causing 2 simultaneous chip selects and leaving the settings for another instance.

Really, I'm asking here.  I'm still studying how several libraries really work and where indivisible SPI transactions really make sense.

I have followed this discussion and it is really a good example of the sharing of ideas within the Arduino community. Great inspiration!

Yes indeed.  Cosa has really helped.  :-)