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.  :-)

[Paul Stoffregen]

> 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).

If you'd like a couple Teensy 3.1s to play with, just say the word and
I'll send you some, for free.

I have a small supply of boards that work perfectly, but they have with
minor cosmetic flaws. We never sell those. I save them for projects,
and for people like you!

[Paul Stoffregen]

On 04/18/2014 05:46 PM, Paul Stoffregen wrote:

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.

Oh, nevermind.  I see now it's inside a synchronized block.  Another interrupt can't occur until it's safe.

Looks like I also hit reply too quick on what was supposed to be a private email.  *embarrassed look*  Maybe it's time to step away from the computer and worry this stuff tomorrow.

[Paul Stoffregen]

Cristian, any chance you could take a quick look and give me a little
feedback, whether this is on a path to becoming a contribution you can
accept into the 1.5.x tree?

https://github.com/PaulStoffregen/Arduino/commits/ide-1.5.x

I know you wanted a more substantial overhaul of the SPI library.
Believe me, I want that too, since Teensy 3.1 would really benefit from
an API that allows better use of advanced hardware. I do want to work
on that later this year or early 2015. Right now, I simply can't make
that big of a contribution. Honestly, at this time I don't even have a
solid proposal for what such a next-generation SPI API should be.

This transaction stuff only does 2 things: managing exclusive access to
the SPI bus, and correct SPI settings when access is gained.

Over the last several days, I've learned a lot more about how many 3rd
party libraries actually use the SPI library. Many add an internal
abstraction layer. Some SPI devices, like CC3000, have very unusual
requirements (the CC3000 code is so complex it'll many anyone want to
cry). Even the SD library requires asserting SS mid-way through a
transaction while initializing the card. I believe the API must not
make assumptions about when SS is asserted. It really needs to be
explicit in the code, either as digitalWrite or other I/O, or integrated
with the transfer() function, or perhaps a new function. I don't have a
proposal for SS handling, but I certainly am thinking about this stuff
as I work on patches for libraries to add transaction support.

My hope is you'll accept this relatively small SPI contribution, which
only addresses SPI bus conflicts between libraries. I still have much
more work and testing to do on this, but if you could take a quick look,
please let me know if this is looking ok or if I need to change things?

[Cristian Maglie]

In data lunedì 21 aprile 2014 13:56:25, Paul Stoffregen ha scritto:
> Cristian, any chance you could take a quick look and give me a little
> feedback, whether this is on a path to becoming a contribution you can
> accept into the 1.5.x tree?
>
> https://github.com/PaulStoffregen/Arduino/commits/ide-1.5.x

Hi Paul,

I've checked the library and, besides one concern, it looks good overall.

After looking at the examples provided by Mikael Patel (thank you!) I'm now
quite convinced that the chip selection should be independent from
transactions.

Also beginTransaction and endTransaction methods you coded looks good, I
wasn't fully convinced about repeating all the SPI parameters on every
invocation of beginTransaction but looking at the actual code it doesn't looks
so bad.
It may be possible to add a beginTransaction() method (without parameters)
that only starts a transaction without touching SPI settings?

I see that you defined a bunch of SPI_CLK_XXXXX constants instead of using an
integer parameter, is that because you don't want to introduce 32bit integer
math?

Now, the only real concern is about the usingInterrupt(...) method: it looks a
bit hacky to code all this interrupt intelligence into the SPI library. I'm
wondering if we can avoid that and made this library more modular?
I'd like to explore an alternative solution, for example the proposal of
Matthijs (an Interrupt wrapper class) or by David (an event-based handling).

C

[David Mellis]

To elaborate a bit on the modularity concern...  I worry that if add a usingInterrupt() function to the SPI library, we're soon going to have:

SPI.usingInterrupt()

SPI.usingPinChangeInterrupt() // once we add attachPinChangeInterrupt() or similar

SPI.usingTimerInterrupt() 

Wire.usingInterrupt()

Wire.usingPinChangeInterrupt()

Wire.usingTimerInterrupt()

Serial.usingInterrupt()

...

It seems like it would be good to avoid having SPI and possibly other libraries depend some intimately on interrupts, both from an implementation maintenance perspective and from an API usability perspective. 

I realize an event-based API would add overhead, but it seems like it might be worth the tradeoff. Do you have a sense of what it would or wouldn't allow?

In general, I don't think the goal of Arduino is to provide a maximally efficient API: it intentionally trades off some amount of performance and size for ease-of-use. Of course, performance is also a goal; there's a point at which trade-off becomes too big -- but it's not clear to me that this is one of those cases.

C

[Mikael Patel]

I think this could help give some inspiration on how to structure SPI drivers. 
https://www.kernel.org/doc/htmldocs/device-drivers/spi.html

It is important to include the chip select handling as this helps add structure to the SPI driver layer. 

Cheers! Mikael 

[Paul Stoffregen]

On 04/22/2014 11:47 AM, Cristian Maglie wrote:
> It may be possible to add a beginTransaction() method (without parameters)
> that only starts a transaction without touching SPI settings?

I was considering doing this while writing the version for Arduino Due.
On AVR, the result will be exclusive access to the SPI bus, with unknown
settings.

Just say the word and I'll add it.

> I see that you defined a bunch of SPI_CLK_XXXXX constants instead of using an
> integer parameter, is that because you don't want to introduce 32bit integer
> math?

Originally I had wanted to use 32 bit numbers for the actual clock
speed. That's still an option....

As I got into the actual implementation and looking at the SPI usage in
many 3rd party libraries, I started to have concerns about performance
issues, especially on AVR. Some libraries, like RF22, have their own
abstraction layer that will prevent the compiler from applying const
and/or inline optimization. Passing only 8 bits that are already
formatted as the value for the hardware register is far more efficient.

The downside to clock constants is only a fixed set of discrete values
defined in SPI.h are available. I spent some time surveying SPI devices
and came up with a list of 36 clocks that cover the clock speed specs on
many SPI chips. The idea is to express the maximum speed the SPI slave
device can accept. The library uses the fastest clock it can, without
exceeding the spec. About a third are very high speeds, intended to
allow for faster microcontrollers in the next many years.

> Now, the only real concern is about the usingInterrupt(...) method: it looks a
> bit hacky to code all this interrupt intelligence into the SPI library. I'm
> wondering if we can avoid that and made this library more modular?

I'm also not so excited about having board-specific interrupt info
inside SPI.h and SPI.cpp.

But in the absence of a cleaner solution, I'd much rather have some
ugliness in the code than frustrated end users. Code can always be
refactored or rewritten, but once a real human has a terribly
frustrating experience simply because they put an Ethernet Shield (for
the SD card) and RF22 or Bluetooth LE wireless shield, they will forever
remember Arduino failed to meet their needs.

> I'd like to explore an alternative solution, for example the proposal of
> Matthijs (an Interrupt wrapper class) or by David (an event-based handling).

Any abstraction layer really needs to be lightweight.
SPI.beginTransaction() really wants to compile to as few instruction
cycles as possible, especially on AVR. I put a lot of work into making
this fast, since it directly adds overhead to every application using
SPI. Burdening every SPI transfer with function calls, especially
virtual or pointer-based calls that are slow on AVR, will end up being
very unpopular with Arduino users.

Perhaps we could restructure WInterrupts.c and SPI.cpp, to get all
board-specific interrupt info from macros and/or inline functions in
pin_arduino.h?

[Paul Stoffregen]

On 04/22/2014 02:13 PM, David Mellis wrote:

To elaborate a bit on the modularity concern...  I worry that if add a usingInterrupt() function to the SPI library, we're soon going to have:

SPI.usingInterrupt()

SPI.usingPinChangeInterrupt() // once we add attachPinChangeInterrupt() or similar

SPI.usingTimerInterrupt()

This presumes Arduino's interrupt API would be expanded by adding even more 0-based numerical spaces, each with hardware-specific API names.  If that approach is taken, then yes, it'll need to be done this way.

But wouldn't it make more sense to bring more interrupt types into the Arduino API by including into the 0-based numbers we already have?  Especially for the pin change interrupts, why create a whole new user-visible abstraction and API for something that is essentially the same thing?  In fact, on Arduino Due, the attachInterrupt() implementation is 4 handlers that dispatch on a per-pin basis, very similar to AVR's pin change feature.

Wire.usingInterrupt()

Wire.usingPinChangeInterrupt()

Wire.usingTimerInterrupt()

Today this isn't an issue, but only because the Wire library does not work at all (in master mode) on AVR when called from interrupt context.  I'm not sure if it works on Arduino Due.  Wire already supports interrupts for slave mode.

If the Wire library is ever redesigned (which seems unlikely), so it doesn't cause a deadlock inside interrupts, then yes, whatever ends up being the solution for SPI should be implemented in Wire too.

Today I2C is a non-issue, but with SPI, Arduino as a platform is failing.

Serial.usingInterrupt()

Matthijs recently added code which makes Serial safe to use from within interrupts.

Serial is unlike SPI and I2C, which are bus protocols with hardware-level bus arbitration (chip selects for SPI and start/stop conditions on I2C).  At the hardware level, Serial is a byte oriented protocol with start and stop bits.  Serial is meant for point-to-point use, where it's understood you need multiple ports for multiple devices (eg, Serial1 for a GPS, Serial2 an XBee, and so on).  Some multi-drop RS-485 applications exist, but libraries which implement these already work well.

It seems like it would be good to avoid having SPI and possibly other libraries depend some intimately on interrupts, both from an implementation maintenance perspective and from an API usability perspective. 

I realize an event-based API would add overhead, but it seems like it might be worth the tradeoff. Do you have a sense of what it would or wouldn't allow?

In general, I don't think the goal of Arduino is to provide a maximally efficient API: it intentionally trades off some amount of performance and size for ease-of-use. Of course, performance is also a goal; there's a point at which trade-off becomes too big -- but it's not clear to me that this is one of those cases.

I believe two function calls through pointers for every SPI transaction will be far too much overhead on AVR.

[Matthijs Kooijman]

Hey Paul,

> >Serial.usingInterrupt()
>
> Matthijs recently added code which makes Serial safe to use from
> within interrupts.

That is true, but I think an atomicity problem still exists: if the
HardwareSerial::write gets interrupted halfway by an ISR which calls
HardwareSerial::write again, stuff will be overwritten.

I proposed adding cli/sei around the critical sections, but that was met
with resistance since it adds interrupt latency even on (most) sketches
that don't write to serial inside ISRs. Hence, something like
Serial.usingInterrupt() might still have added value, so these problems
can be circumvented with minimal impact on sketches that don't suffer
from these problems.

> Serial is unlike SPI and I2C, which are bus protocols with
> hardware-level bus arbitration (chip selects for SPI and start/stop
> conditions on I2C). At the hardware level, Serial is a byte
> oriented protocol with start and stop bits. Serial is meant for
> point-to-point use, where it's understood you need multiple ports
> for multiple devices (eg, Serial1 for a GPS, Serial2 an XBee, and so
> on). Some multi-drop RS-485 applications exist, but libraries which
> implement these already work well.

Note that this does mean that for Serial, multiple writes will be
interleaved at a byte level. It might make sense to have some atomicity
at the line, or packet or whatever level, though in practice that will
be very hard to properly implement and probably not very useful, so we
probably shouldn't even try.

> >In general, I don't think the goal of Arduino is to provide a
> >maximally efficient API: it intentionally trades off some amount
> >of performance and size for ease-of-use. Of course, performance is
> >also a goal; there's a point at which trade-off becomes too big --
> >but it's not clear to me that this is one of those cases.
>
> I believe two function calls through pointers for every SPI
> transaction will be far too much overhead on AVR.

Is it really? An ICALL instruction is just as fast as an RCALL, though
the indirectness does mean loading a pointer a 2x2 cycles. And of course
spending 12 cycles (plus register pushing) on a function call is
overhead compared to no function call, which is probably what you were
going for.

However, what is the alternative? If we support just the external
interrupts that can be enabled using attachInterrupt, then disabling
them at beginTransaction time would be fairly fast. However, if we
generalize this to include other interrupts (e.g. by mapping them into
the interrupt number space), this will also get slower (e.g., a switch
or if-then-else cascade to map each interrupt number to the needed
mask register and bit needed to disable it). Compared to that, using an
indirect call might be even faster?

If you mean to never support other interrupts than the external
interrupts, then the "hardcoded" approach is definitely faster. However,
I'd also say that that would mean our new interrupt-aware-API is quite
useless for any somewhat advanced libraries or sketches...


Another alternative, instead of using a function call for extreme
flexibility, could be to pass the address of the mask register and the
bit to enable/disable in there (probably together in a struct or so). I
believe that on AVR, this should be flexible enough to support all
interrupts, but remove the function call overhead. However, while
writing this I realize what was mean with an "event API", which this
clearly is not.


Finally, one more observation: In the common case, where no event
handler or interrupt is registered, the overhead is of course nearly
zero: just a single pointer or integer check. In more complicated cases,
that need atomicity guarantees, the extra overhead might be acceptable?

Gr.

Matthijs

[Matthijs Kooijman]

> >I see that you defined a bunch of SPI_CLK_XXXXX constants instead of using an
> >integer parameter, is that because you don't want to introduce 32bit integer
> >math?
>
> Originally I had wanted to use 32 bit numbers for the actual clock
> speed. That's still an option....
>
> As I got into the actual implementation and looking at the SPI usage
> in many 3rd party libraries, I started to have concerns about
> performance issues, especially on AVR. Some libraries, like RF22,
> have their own abstraction layer that will prevent the compiler from
> applying const and/or inline optimization. Passing only 8 bits that
> are already formatted as the value for the hardware register is far
> more efficient.

Can't we do a two-step approach here? I think I proposed something like
this previously, but it probably got drowned in the discussion.

I proposed having an SPI.getSettings(...) call, that takes the settings in
a hardware-neutral form (in particular, using an uint32_t for the
clock speed in Mhz). That function translates those generic settings
into a hardware-specific form. For AVR, this could be a struct like:

struct {
uint8_t spcr;
uint8_t spsr;
};

This means that actually changing the settings at the start of a
transaction only involves passing two bytes and writing to registers.

The actual calculation of register values can happen beforehand, or if
the settings are compile-time constants, even at compiletime by inlining
getSettings().


At first glance, a similar thing can be achieved by using clock speed
constants (though my proposal eliminates even the bitshifting needed to
compose the clock speed, spi mode, etc into a single register value).

However, when using clock speed constants, it is impossible to select an
arbitrary clock speed at runtime. An example would be to read the clock
speed to use from user input (in an interactive manner, using something
like bitlash). Another example would be to read the supported SPI speed
from a piece of EEPROM on an expansion shield, to facilitate
autodetecting of shield settings (which is what we're implementing on
the pinoccio platform).

Gr.

Matthijs

[Paul Stoffregen]

On 04/23/2014 01:27 AM, Matthijs Kooijman wrote:
> Can't we do a two-step approach here? I think I proposed something like
> this previously, but it probably got drowned in the discussion.
>
> I proposed having an SPI.getSettings(...) call, that takes the settings in
> a hardware-neutral form (in particular, using an uint32_t for the
> clock speed in Mhz). That function translates those generic settings
> into a hardware-specific form. For AVR, this could be a struct like:
>
> struct {
> uint8_t spcr;
> uint8_t spsr;
> };
>
> This means that actually changing the settings at the start of a
> transaction only involves passing two bytes and writing to registers.
>
> The actual calculation of register values can happen beforehand, or if
> the settings are compile-time constants, even at compiletime by inlining
> getSettings().

This way could work too. Passing 2 bytes in the non-const case would be
more efficient than 3. Whether to have an extra function or syntax to
"compile" the constants to the struct is really Cristian's call.

> At first glance, a similar thing can be achieved by using clock speed
> constants (though my proposal eliminates even the bitshifting needed to
> compose the clock speed, spi mode, etc into a single register value).

When I looked at this a few days ago, the non-const case turned into an
awful lot of compiled code.

> However, when using clock speed constants, it is impossible to select an
> arbitrary clock speed at runtime.

Yes, that's a limitation. I originally wanted to use a 32 bit integer
to allow arbitrary speed, but I gave up trying to find an efficient
approach for non-const input.

Everything involves trade-offs. My gut feeling says arbitrary clock
speeds would be nice (even though the hardware can't truly do it), but
not at the expense of efficiency while starting every SPI transaction.

> An example would be to read the clock
> speed to use from user input (in an interactive manner, using something
> like bitlash). Another example would be to read the supported SPI speed
> from a piece of EEPROM on an expansion shield, to facilitate
> autodetecting of shield settings (which is what we're implementing on
> the pinoccio platform).

An arbitrary integer value does open up some interesting possibilities
for runtime configuration.

I believe the primary use for SPI is targeting specific chips with well
known clock speed limits. SD, Ethernet, CC3000 (wifi), RF12 & RF22 &
RF69 (wireless radio modules), ST7735 & ILI9341 (color TFT displays),
nRF8001 (Bluetooth LE), MFR522 (RFID), STMPE610 (touch screen), serial
flash & eeprom, and many others are already in widespread use.

Most of these already run at fairly modest speeds, which ultimately
limits how people can use Arduino. Making them all "just work" together
will increase Arduino's value to end users. But slowing
already-not-so-fast TFT redraw, ethernet and wireless communication, and
SD card access will lower Arduino's overall value as a platform. I
really want to minimize any performance hit, which means keeping the
begin and end transaction functions as efficient as reasonably possible.


>
> Gr.
>
> Matthijs

[Matthijs Kooijman]

Hey Paul,

> This way could work too. Passing 2 bytes in the non-const case
> would be more efficient than 3. Whether to have an extra function
> or syntax to "compile" the constants to the struct is really
> Cristian's call.
>
> >At first glance, a similar thing can be achieved by using clock speed
> >constants (though my proposal eliminates even the bitshifting needed to
> >compose the clock speed, spi mode, etc into a single register value).
>
> When I looked at this a few days ago, the non-const case turned into
> an awful lot of compiled code.
>
> >However, when using clock speed constants, it is impossible to select an
> >arbitrary clock speed at runtime.
>
> Yes, that's a limitation. I originally wanted to use a 32 bit
> integer to allow arbitrary speed, but I gave up trying to find an
> efficient approach for non-const input.
>
> Everything involves trade-offs. My gut feeling says arbitrary clock
> speeds would be nice (even though the hardware can't truly do it),
> but not at the expense of efficiency while starting every SPI
> transaction.

I don't think that is what I'm proposing? In the const-case, it should
be possible to have the getSettings function inlined and completely
evaluated at compiletime, reducing the const-case to passing two
constant bytes to beginTransaction (which is even faster than the
three-byte approach you're suggesting?).

The non-const case would probably require a lot of code, but I can't see
how to prevent that. However, supporting this in a slow and big way is
strictly better than not supporting it all, right? Also, since the
slow code does not _need_ to run on every transaction, it's not really a
problem either?

Or are we totally misunderstanding each other here?

Gr.

Matthijs

[Paul Stoffregen]

On 04/23/2014 07:15 AM, Matthijs Kooijman wrote:
>
> I don't think that is what I'm proposing? In the const-case, it should
> be possible to have the getSettings function inlined and completely
> evaluated at compiletime, reducing the const-case to passing two
> constant bytes to beginTransaction (which is even faster than the
> three-byte approach you're suggesting?).
>
> The non-const case would probably require a lot of code, but I can't see
> how to prevent that. However, supporting this in a slow and big way is
> strictly better than not supporting it all, right? Also, since the
> slow code does not _need_ to run on every transaction, it's not really a
> problem either?
>
> Or are we totally misunderstanding each other here?

I think it would be clearer (at least to me) if expressed as source
code. ;-)

My main concern here is solving compatibility problems that are driving
people away from Arduino. Since Monday, I've been focusing more on
patches for libraries than the API. So far, I've done SD, Ethernet,
CC3000 and a first attempt at RadioHead (successor to RF22 & RF69). I
plan to work on nRF8001, ILI9341, STMPE610 soon. That's where I'm
directing my available time and energy.

Really, these finer points of API style to publish in Arduino are
Cristian's call.

It sounds like you might be on to a good alternate way to craft this
API. I'm not getting perfectly clear picture without seeing source
code. If it works and it's efficient, and Cristian decides it's the way
the Arduino SPI library should be, then I'll simply update the patches
I've been developing.

My goal is solving these compatibility problems, so customers who
attempt projects with multiple SPI devices will have a good "just works"
experience. I care much more about function than form.




>
> Gr.
>
> Matthijs

[Ralph Doncaster]

On Tuesday, 15 April 2014 07:02:23 UTC-3, paul wrote:

On 04/15/2014 01:28 AM, Cristian Maglie wrote:

>   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.

When you referred to chip select, this is what I thought you meant.  For something like the nrf modules, having CSN (SS) brought low for the transaction would make things a lot cleaner.  On the NRF, CE isn't really a chip select (it still responds to SPI commands with CE low), so some designs just tie CE high.

I like the idea of abstracting the clock rate - that would allow for a compatible SPI library with the tiny cores where SPI_2X means nothing.  I'd like to see a default, since in many situations the app just wants the fastest - 8Mhz would be fine for nrf modules, ENC28J60, and shift registers driving LEDs.  Other than the PDC8544, I can't think of any popular SPI hardware that doesn't work at 8Mhz.

[Paul Stoffregen]

On 04/22/2014 11:47 AM, Cristian Maglie wrote:

> Now, the only real concern is about the usingInterrupt(...) method: it
> looks a bit hacky to code all this interrupt intelligence into the SPI
> library. I'm wondering if we can avoid that and made this library more
> modular? I'd like to explore an alternative solution,

I'm looking into a thin abstraction layer, to be implemented in
WInterrupts.c, Arduino.h and pins_arduino.h.

In a nutshell, it will define a typedef that represents masking all the
interrupt pins. A pair of functions will set/clear individual
interrupts within it. Another pair of functions will actually mask &
unmask all the external interrupts.

This should allow all the interrupt intelligence to reside within the
core library or pins_arduino.h. On AVR, the mask/unmask functions will
probably be inline for efficiency.

Does this sound reasonable? Perhaps moving towards something that can
become a pull request?

[Paul Stoffregen]

I've started work on an abstraction layer that removes all the interrupt
specific code from SPI.

https://github.com/PaulStoffregen/Arduino/blob/ide-1.5.x/hardware/arduino/avr/libraries/SPI/SPI.cpp
https://github.com/PaulStoffregen/Arduino/blob/ide-1.5.x/hardware/arduino/avr/libraries/SPI/SPI.h

I could really use a little feedback. Is this moving in a direction
that can be accepted into 1.5.x ?

[Matthijs Kooijman]

Hey Paul,

> > [ Proposal for SPI.getSettings ]

>
> I think it would be clearer (at least to me) if expressed as source
> code. ;-)

That sounds reasonable :-)

I created two commits on top of your ide-1.5.x branch:

https://github.com/matthijskooijman/Arduino/commit/d29a0dca64768ed6ae881ff76aafd42987be3bba
https://github.com/matthijskooijman/Arduino/commit/b2c7e46dbcd33d3602fd230d4bf48834c4968512

(Note, identifier names are just suggestions, they might need improvement)

The first one adds a SPI.getSettings() method that gets passed a clock speed (in
Mhz), data order and spi mode settings and returns a SPISettings object
(which should be opaque to the user, which is why I made it a class with
only private attributes).

The beginTransaction method now accepts such an SPISettings object.

To illustrate how to use this, consider these lines:

SPISettings settings = SPI.getSettings(2000000, MSBFIRST, SPI_MODE1);
SPI.beginTransaction(settings);
SPI.beginTransaction(SPI.getSettings(1000000, LSBFIRST, SPI_MODE2));
SPI.beginTransaction(SPI.getSettings(random(), LSBFIRST, SPI_MODE2));

On gcc 4.8.2 this compiles to:

178: 84 e5 ldi r24, 0x54 ; 84
17a: 91 e0 ldi r25, 0x01 ; 1
17c: 0e 94 a8 00 call 0x150 ; 0x150 <SPIClass::beginTransaction(SPISettings)>

180: 89 e7 ldi r24, 0x79 ; 121
182: 91 e0 ldi r25, 0x01 ; 1
184: 0e 94 a8 00 call 0x150 ; 0x150 <SPIClass::beginTransaction(SPISettings)>

188: 0e 94 e7 02 call 0x5ce ; 0x5ce <random>
18c: 28 e0 ldi r18, 0x08 ; 8
18e: 40 e0 ldi r20, 0x00 ; 0
190: 0e 94 7b 00 call 0xf6 ; 0xf6 <SPIClass::getSettingsMightInline(unsigned long, unsigned char, unsigned char)>
194: 0e 94 a8 00 call 0x150 ; 0x150 <SPIClass::beginTransaction(SPISettings)>

Where getSettingsMightInline is a 88-byte function that's only used for
non-constant clock rates.

Note that gcc 4.3.2 also succesfully reduces the settings to two byte
constants, but additionally things it's good to inline all calls to
beginTransaction() and getSettings() for some reason (but I don't think this is
related to my changes, or a real problem in the first place).

The second commit juggles adds some wrapper functions to ensure that calling
getSettings with constant arguments always get inlined and reduced to two
constant bytes. Even without this last commit, this works most of the time, but
I found that when you call getSettings with a non-constant argument _twice_,
gcc would decide to not inline all calls to it, which is fixed by the second
commit. If you'd just call getSettings with constant arguments, this second
commit is not needed.


An an alternative variation, which I didn't try in code yet, would be to let
the settings object be more active, instead of having a getSettings method.
e.g., the sketch would look like:

SPISettings settings;
settings.setMaxClockSpeed(2000000);
settings.setBitOrder(MSBFIRST);
settings.setDataMode(SPI_MODE1)
SPI.beginTransaction(settings);

This is more explicit, which corresponds better to the rest of the Arduino API,
makes it more clear what is happening and corresponds better to the existing
API. As an added advantage, you could perhaps support
settings.setClockDivider() as a fallback API, or later add
(architecture-specific) settings without getting an enormous getSettings()
call. Finally, this style also allows leaving some settings at the defaults.

The more I talk about this, the better I like this approach, though I guess
Christian has to see what API style is better.

I haven't doublechecked this, but I believe that this second proposal can also
be implemented such that the compiler completely reduces the actual code to
constants.

> My goal is solving these compatibility problems, so customers who
> attempt projects with multiple SPI devices will have a good "just
> works" experience. I care much more about function than form.

I don't think that my suggestion is a matter of API style or form - my proposal
allows using variable clock speeds in a platform independent manner, which I'd
say adds function :-)

Gr.

Matthijs

[Jan Berger]

 

> Date: Thu, 24 Apr 2014 06:13:39 -0700
> From: pa...@pjrc.com
> To: devel...@arduino.cc
> Subject: Re: [Developers] SPI Library Transaction API

[Paul Stoffregen]

On 04/24/2014 09:40 AM, Matthijs Kooijman wrote:
>
> That sounds reasonable :-)
>
> .....

>
> The beginTransaction method now accepts such an SPISettings object.

Nice! I like it.

> The more I talk about this, the better I like this approach, though I guess
> Christian has to see what API style is better.

Indeed.

At this moment, I'm feeling like I've already put far too much work into
this, given it's rather uncertain prospect of being accepted.

[Cristian Maglie]

In data giovedì 24 aprile 2014 15:13:39, Paul Stoffregen ha scritto:
> I've started work on an abstraction layer that removes all the interrupt
> specific code from SPI.
>
> https://github.com/PaulStoffregen/Arduino/blob/ide-1.5.x/hardware/arduino/a
> vr/libraries/SPI/SPI.cpp
> https://github.com/PaulStoffregen/Arduino/blob/ide-1.5.x/hardware/arduino/
> avr/libraries/SPI/SPI.h
>
> I could really use a little feedback. Is this moving in a direction
> that can be accepted into 1.5.x ?

Hi Paul,

I'm looking the functions you introduced:

// external interrupt masking hardware abstraction
typedef uint8_t externalInterruptMask;
void setExternalInterruptMaskBit(uint8_t num, externalInterruptMask * mask);
void clearExternalInterruptMaskBit(uint8_t num, externalInterruptMask * mask);
void getExternalInterruptsMask(externalInterruptMask * mask);
void setExternalInterruptsMask(const externalInterruptMask * mask);

I imagine this is a work in progress and probably the
setExternalInterruptsMask should be split into two different functions.

At a first glance I like this abstraction because it moves all the low level
code out of the way from SPI making it cleaner and it may be useful also for
other libraries. On the other hand it doesn't change the SPI Transcation API
you initially proposed so you can postpone the implementation until we agree
on that.

To move forward, I'd like to benchmark the new SPI lib, are you already
running some tests, or have suggestions? I guess that the Robot RGB display
may be a good performance check.

C

[Cristian Maglie]

In data domenica 27 aprile 2014 00:08:30, Cristian Maglie ha scritto:
> To move forward, I'd like to benchmark the new SPI lib, are you already
> running some tests, or have suggestions? I guess that the Robot RGB display
> may be a good performance check.

Hi,

I've just tried an alternative event-based API, here:

https://github.com/cmaglie/Arduino/commits/spi-transactions

now I'd like to compare all these variants, so I used the followin test to
measure the peak raw performance:

https://gist.github.com/cmaglie/11366715

After a first check it seems that the event-based version is faster than the
interrupt-based, but this strongly goes against my expectation (I'm expecting
the interrupt-based version being faster), can someone help me understand
what's going on?

interrupt-based (git revision 6fa292):
Plain SPI: 4923077.00
Transaction API (interrupts not used): (4) 2738944.50
Transaction API (interrupts not used): (2) 3503649.75
Transaction API (using interrupts): (4) 2291169.50
Transaction API (using interrupts): (2) 3121951.25

event-based (git revision f1d73e0):
Plain SPI: 4923077.00
Transaction API (interrupts not used): (4) 3350785.50
Transaction API (interrupts not used): (2) 3966942.00
Transaction API (using interrupts): (4) 2823529.25
Transaction API (using interrupts): (2) 3588785.00

C

[Ralph Doncaster]

I couldn't find the interrupt version, but I looked at the event version https://github.com/cmaglie/Arduino/blob/e5e8a7e62a856cc2bd636820ccff430dfc567e60/hardware/arduino/avr/libraries/SPI/SPI.cpp

 inline static byte transfer(byte data) {

    SPDR = data;

    while (!(SPSR & _BV(SPIF))) ; // wait

    return SPDR;

  }

This should compile an out instruction followed by sbis/rjmp.   Any code that uses an interrupt handler is going to have much more overhead - typically > 20 cycles.

The only way I can think of to get any faster than the above code would be overlapped transfers (perhaps what Paul was referring to).  You'd get the byte returned from the first transfer when you call it the second time.  i.e.:

 inline static byte transfer(byte data) {

    while (!(SPSR & _BV(SPIF))) ; // wait

    SPDR = data;

    return SPDR;

}

[Paul Stoffregen]

On 04/28/2014 10:02 AM, Ralph Doncaster wrote:

On Monday, 28 April 2014 10:33:02 UTC-3, c.maglie wrote:

In data domenica 27 aprile 2014 00:08:30, Cristian Maglie ha scritto:
> To move forward, I'd like to benchmark the new SPI lib, are you already
> running some tests, or have suggestions? I guess that the Robot RGB display
> may be a good performance check.

Hi,

I've just tried an alternative event-based API, here:

https://github.com/cmaglie/Arduino/commits/spi-transactions

now I'd like to compare all these variants, so I used the followin test to
measure the peak raw performance:

https://gist.github.com/cmaglie/11366715

After a first check it seems that the event-based version is faster than the
interrupt-based, but this strongly goes against my expectation (I'm expecting
the interrupt-based version being faster), can someone help me understand
what's going on?

I'll take a look.  That does seem counter-intuitive.

I do have one question about the event API.  Can it handle 2 or more libraries which each register a callback?  In other words, two interrupts need to be disabled during transactions, but neither event handler can know the other's requirements because they're provided by different interrupt-based libraries.

interrupt-based (git revision 6fa292):
Plain SPI:     4923077.00
Transaction API (interrupts not used): (4) 2738944.50
Transaction API (interrupts not used): (2) 3503649.75
Transaction API    (using interrupts): (4) 2291169.50
Transaction API    (using interrupts): (2) 3121951.25

event-based (git revision f1d73e0):
Plain SPI:     4923077.00
Transaction API (interrupts not used): (4) 3350785.50
Transaction API (interrupts not used): (2) 3966942.00
Transaction API    (using interrupts): (4) 2823529.25
Transaction API    (using interrupts): (2) 3588785.00

C

I couldn't find the interrupt version, but I looked at the event version https://github.com/cmaglie/Arduino/blob/e5e8a7e62a856cc2bd636820ccff430dfc567e60/hardware/arduino/avr/libraries/SPI/SPI.cpp

 inline static byte transfer(byte data) {

    SPDR = data;

    while (!(SPSR & _BV(SPIF))) ; // wait

    return SPDR;

  }

This should compile an out instruction followed by sbis/rjmp.   Any code that uses an interrupt handler is going to have much more overhead - typically > 20 cycles.

The only way I can think of to get any faster than the above code would be overlapped transfers (perhaps what Paul was referring to).  You'd get the byte returned from the first transfer when you call it the second time.  i.e.:

 inline static byte transfer(byte data) {

    while (!(SPSR & _BV(SPIF))) ; // wait

    SPDR = data;

    return SPDR;

}

[Paul Stoffregen]

On 04/28/2014 06:33 AM, Cristian Maglie wrote:

I've just tried an alternative event-based API, here:

https://github.com/cmaglie/Arduino/commits/spi-transactions

now I'd like to compare all these variants, so I used the followin test to 
measure the peak raw performance:

https://gist.github.com/cmaglie/11366715

After a first check it seems that the event-based version is faster than the 
interrupt-based, but this strongly goes against my expectation (I'm expecting 
the interrupt-based version being faster), can someone help me understand 
what's going on?

I looked into this.  The interrupt masking version was not using inline optimization for beginTransaction().

I added " __attribute__((always_inline))"

  // Before using SPI.transfer() or asserting chip select pins,
  // this function is used to gain exclusive access to the SPI bus
  // and configure the correct settings.
  inline static void beginTransaction(uint8_t clockDiv, uint8_t bitOrder, uint8_t dataMode) __attribute__((always_inline)) {

With this change, the interrupt masking version is much faster than the event callback, as you'd intuitively expect.

                            Plain SPI:     4910486.00
Transaction API (interrupts not used): (4) 4403669.50
Transaction API (interrupts not used): (2) 4604316.50
Transaction API    (using interrupts): (4) 3824701.25
Transaction API    (using interrupts): (2) 4229075.00

In the case of a 2 byte transaction that doesn't need to mask interrupts, the performance hit is only 6%.

If interrupts do need to be masked, the performance hit is 14%.  That's far better than wrong results from corrupted SPI transactions!

interrupt-based (git revision 6fa292):
Plain SPI:     4923077.00
Transaction API (interrupts not used): (4) 2738944.50
Transaction API (interrupts not used): (2) 3503649.75
Transaction API    (using interrupts): (4) 2291169.50
Transaction API    (using interrupts): (2) 3121951.25

event-based (git revision f1d73e0):
Plain SPI:     4923077.00
Transaction API (interrupts not used): (4) 3350785.50
Transaction API (interrupts not used): (2) 3966942.00
Transaction API    (using interrupts): (4) 2823529.25
Transaction API    (using interrupts): (2) 3588785.00

Another minor point about these benchmarks is the event test only implements a global interrupt disable.  The interrupt test implements much more: external interrupt masking with a fallback to global disable.

Admittedly, many libraries would probably implement simple callback functions.  Aside from the higher overhead, and the difficulty of registering callback from multiple libraries, my other concern about event callback is skepticism that library authors will implement high quality handlers that consider platform-wide compatibility issues.  It is a far more flexible system, but all that flexibility is only useful if people really use it to good effect.

On 04/28/2014 10:02 AM, Ralph Doncaster wrote:

I couldn't find the interrupt version, but I looked at the event version https://github.com/cmaglie/Arduino/blob/e5e8a7e62a856cc2bd636820ccff430dfc567e60/hardware/arduino/avr/libraries/SPI/SPI.cpp

 inline static byte transfer(byte data) {

    SPDR = data;

    while (!(SPSR & _BV(SPIF))) ; // wait

    return SPDR;

  }

This should compile an out instruction followed by sbis/rjmp.   Any code that uses an interrupt handler is going to have much more overhead - typically > 20 cycles.

Ralph, we're not talking about replacing SPI.transfer() with the SPI interrupt.  The casual use of "interrupt version" is misleading.

Really, it's the "interrupt masking version", not the "interrupt version".

This conversation is about different ways to temporarily disable interrupts during a "transaction" composed of several SPI.transfer() and other stuff like manipulating the chip select.  We're not talking about always disabling all interrupts.  These are alternate approaches to do so in a smart way, so interrupts are only disabled when another library is known to be using SPI.transfer() inside an interrupt, and if it's an external pin interrupt (the most common case), only that specific interrupt is temporarily disabled.  Timer, serial and other urgent interrupts won't get disabled in the case where external pin interrupts are disabled.

Implementing such smart interrupt disable adds code.  We're looking at the overhead this will add before and after those SPI transfers.  The language is about the speed change, which seems like talking about changing transfer(), but it's really about the overall impact relative to not having this extra code.  The actual SPI.transfer() function is not changing, despite the confusing language in this conversation.  Hopefully this explanation helps make things clearer?

[Paul Stoffregen]

I looked at the TFT library code.

The speed sensitive case is GFX repetitively calling drawPixel(), which
transfers 13 bytes per pixel. Internally, the library has a lot of
extra function call and CS+RS pin manipulation overhead.

I modified the benchmark to try emulating most of the TFT library
overhead. The code is attached. I used 2 transactions per 13 bytes,
which would be the easiest (but not most efficient) way to add
transactions to the TFT library.

With the interrupt masking version, using always_inline:

Plain SPI: 2222222.25
Transaction API (interrupts not used): (TFT estimate) 2166666.50
Transaction API (using interrupts): (TFT estimate) 2077230.25

With the event callback version:

Plain SPI: 2222222.25
Transaction API (interrupts not used): (TFT estimate) 2088353.50
Transaction API (using interrupts): (TFT estimate) 2016806.75

I suppose one way to interpret these results is there's so already much
overhead inside TFT that either approach adds relatively little extra.

Another viewpoint could be the opportunity for relatively simple
redesign inside the TFT to dramatically improve its performance....
regardless of this transaction stuff.

[Ralph Doncaster]

On Monday, 28 April 2014 18:42:55 UTC-3, paul wrote:

Ralph, we're not talking about replacing SPI.transfer() with the SPI interrupt.  The casual use of "interrupt version" is misleading.

Really, it's the "interrupt masking version", not the "interrupt version".

This conversation is about different ways to temporarily disable interrupts during a "transaction" composed of several SPI.transfer() and other stuff like manipulating the chip select.  We're not talking about always disabling all interrupts.  These are alternate approaches to do so in a smart way, so interrupts are only disabled when another library is known to be using SPI.transfer() inside an interrupt, and if it's an external pin interrupt (the most common case), only that specific interrupt is temporarily disabled.  Timer, serial and other urgent interrupts won't get disabled in the case where external pin interrupts are disabled.

Implementing such smart interrupt disable adds code.  We're looking at the overhead this will add before and after those SPI transfers.  The language is about the speed change, which seems like talking about changing transfer(), but it's really about the overall impact relative to not having this extra code.  The actual SPI.transfer() function is not changing, despite the confusing language in this conversation.  Hopefully this explanation helps make things clearer?

Yes, thanks for the explanation.

I think adding a multi-byte transfer (i.e. SPI.transfer(buf, len)) would give some good performance benefits.  With SPI2X, it's hard to get much better than 50% duty cycle on the SPI bus even when SPI.transfer() is inlined.  The kind of code I see in libraries like mirf looks something like:

for (int index = 0; count < len; index++) buf[index] = SPI.transfer(buf[index]);

An implementation of SPI.transfer(buf,len) should be able to get around 80% duty cycle as saving last SPDR value, incrementing the index, and fetching the next value could be done before waiting for the current transfer to complete.

[Mikael Patel]

On 04/29/2014 06:07 AM, Ralph Doncaster wrote:
>
> I think adding a multi-byte transfer (i.e. SPI.transfer(buf, len))
> would give some good performance benefits. With SPI2X, it's hard to
> get much better than 50% duty cycle on the SPI bus even when
> SPI.transfer() is inlined. The kind of code I see in libraries like
> mirf looks something like:
> for (int index = 0; count < len; index++) buf[index] =
> SPI.transfer(buf[index]);
>
> An implementation of SPI.transfer(buf,len) should be able to get
> around 80% duty cycle as saving last SPDR value, incrementing the
> index, and fetching the next value could be done before waiting for
> the current transfer to complete.
>

I would like to add some additional functions to support this high-level
style of SPI interaction. Below are the member functions in the Cosa SPI
class. Most important is the support for gather style of transmission
(iovec_t) which allows support for protocol stacks and reduces SRAM
requirements. BW: This is an operating system trick.

uint8_t transfer(uint8_t data);
void transfer(void* buffer, size_t count);
void transfer(void* dst, const void* src, size_t count);
void read(void* buf, size_t count);
void write(const void* buf, size_t count);
void write_P(const uint8_t* buf, size_t count);
void write(const iovec_t* vec);

Please see
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.hh#L260
and
http://dl.dropboxusercontent.com/u/993383/Cosa/doc/html/da/d8d/classSPI.html
for more details. The iovec_t is defined in Cosa/Types.h
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/Types.h#L239

After writing this I have also understood that there is a great
opportunity for pipelining/prefetching. Thanks Ralph for rising the
issue and pointing to the performance aspect. Missed that part of the
interface ;-)

Cheers! Mikael

[Paul Stoffregen]

On 04/29/2014 07:49 AM, Mikael Patel wrote:
>
> After writing this I have also understood that there is a great
> opportunity for pipelining/prefetching. Thanks Ralph for rising the
> issue and pointing to the performance aspect. Missed that part of the
> interface ;-)

There are a lot of opportunities for improved performance on more
powerful hardware, where FIFOs, DMA and multiple word sizes (eg, both 8
and 16 bits) are available, and even some opportunities on low end
hardware like AVR.

These are all really great things to do. My only hope is to avoid
stalling progress on beginTransaction() & endTransaction(), which are
urgently needed to start making the many conflicting SPI-based libraries
work together.

[Mikael Patel]

On 04/29/2014 09:22 PM, Paul Stoffregen wrote:
>
> These are all really great things to do. My only hope is to avoid
> stalling progress on beginTransaction() & endTransaction(), which are
> urgently needed to start making the many conflicting SPI-based
> libraries work together.
>

Cannot agree with you more and getting those libraries to play nicely
together should be first priority before going for more performance.

As we are discussing the SPI interface and this is a great opportunity
to do more I also believe that we can improve the support level. Block
operations are a natural extension.

Below is the result of the pipeline/prefetch idea. Basically it takes
advantage of the SPI hardware running in the background and instead of
going into a busy wait spin pre-fetches data. There are at least 16
clock cycles to fill with work while waiting for the SPI hardware (AVR
with SPI unit).

No stall intended.

Cheers! Mikael
---------------------------------------------------------------------------------------------------------------------------------------------------
// Snippet from Cosa/SPI

void transfer_start(uint8_t data) __attribute__((always_inline))
{
SPDR = data;
}

uint8_t transfer_await() __attribute__((always_inline))
{
loop_until_bit_is_set(SPSR, SPIF);
return (SPDR);
}

void
SPI::transfer(void* buf, size_t count)
{
if (count == 0) return;
uint8_t* dp = (uint8_t*) buf;
uint8_t data = *dp;
while (1) {
transfer_start(data);
if (--count) break;
uint8_t* tp = dp + 1;
data = *tp;
*dp = transfer_await();
dp = tp;
}
*dp = transfer_await();
}

[Ralph Doncaster]

On Tuesday, 29 April 2014 17:52:04 UTC-3, Mikael Patel wrote:

---------------------------------------------------------------------------------------------------------------------------------------------------
// Snippet from Cosa/SPI

void transfer_start(uint8_t data) __attribute__((always_inline))
{
   SPDR = data;
}

uint8_t transfer_await() __attribute__((always_inline))
{
   loop_until_bit_is_set(SPSR, SPIF);
   return (SPDR);
}

void
SPI::transfer(void* buf, size_t count)
{
   if (count == 0) return;
   uint8_t* dp = (uint8_t*) buf;
   uint8_t data = *dp;
   while (1) {
     transfer_start(data);
     if (--count) break;
     uint8_t* tp = dp + 1;
     data = *tp;
     *dp = transfer_await();
     dp = tp;
   }
   *dp = transfer_await();
}

Pretty good.   It compiles to code that has st and rjmp instructions after transfer_wait().  This code will give in SPDR immediately followed by out SPDR:

uint8_t tmp;

uint8_t* dp = (uint8_t*) buf;

uint8_t data = *dp;

transfer_start(data)

while(--count){

     uint8_t* tp = dp + 1; 

     data = *tp; 

     tmp = transfer_await(); 

     transfer_start(data); 

     *dp = tmp

     dp = tp;

}

*dp = transfer_await();

[Matthijs Kooijman]

Hey folks,

> I do have one question about the event API. Can it handle 2 or more
> libraries which each register a callback? In other words, two
> interrupts need to be disabled during transactions, but neither
> event handler can know the other's requirements because they're
> provided by different interrupt-based libraries.

I don't think Christian's proposal supports this, but I do think it's
essential for making something like this work. It's also hard to
implement, since you need to somehow allocate storage for an arbitrary
length list of pointers. Options that I can think of are:

- Use dynamic memory (malloc/realloc). This is easy to implement, but
using dynamic memory might not be desirable (though assuming all
usingInterrupt calls happen during initialization and the list is not
modified afterwards, fragmentation might be limited or absent).

- Use a preallocated list with a maximum size (e.g. max 8 event
handlers at the cost of 16 bytes of RAM).

- Use a linked list using a struct containing a function pointer and a
pointer to the next element (pointer to this same struct). This
requires callers to globally (statically) allocate an instance of
this struct and then pass a pointer or reference to usingInterrupt.
This is probably error prone, but I suspect also the cleanest and
most efficient solution.

- Ideally, you'd somehow build a static list of all event handlers at
compiletime, since it's safe to assume they're all known. However,
since the handlers can be defined in different compilation units, you
can't use any templating or macro tricks to achieve this - Somehow
the linker should take multiple arrays or variables and merge them
together into a single one. I'm afraid the linker cannot actually do
something like this, or does anyone know of a clever trick?

Another thing to take into account is that these event handlers cannot
just disable and enable interrupts - they must store the old value of
the interrupt mask. This means that in addition to declaring event
handlers, we must also declare a global variable to save the interrupt
mask. This means we can't use a pair of event handlers for multiple
purposes (e.g. when we add usingInterrupt to other things than SPI
later), or we should provide for passing a `void* data` argument to the
event handlers (which adds more overhead).



Given the above, I wondered if perhaps using event handlers might not be
the best solution after all. I considered the interrupt mask solution
proposed by Paul, but what I strongly dislike about that one is that it
only works for external interrupts, not for others (like timer
interrupts, etc.). So if we go that route, we solve a small piece of the
problem, but without any clear route to solving the complete problem in
the future (without redoing the API _again_).

In an attempt to solve the problem more generically, I wrote some code
for another approach. Unfortunately this approach doesn't work as well
as I had hoped (see below). The idea is to, instead of keeping a single
byte with the EIMSK mask value, keep a struct that (potentially)
contains masks for multiple registers, depending on the hardware. Then,
three methods are defined: merge(), to merge two of these structs together
(e.g. bitwise or), disable() to disable the indicated interrupts and
restore() to restore them to the previous value.

My current implementation is a struct with multiple bytes, but future
implementations (on other architectures perhaps) could implement this
however they like, as long as they implement these three methods.

Code is here:

https://github.com/matthijskooijman/Arduino/commit/c977d4e2bf2adc6aec6ffd8903e4ad394449d573

Like I said, there are some problems with this approach:

- I originally thought to have a single byte for every interrupt mask
register, but I found that the interrupt mask bits are scattered
through a dozen or so (on the 328) registers. Keeping a full byte for
each of them clearly is too much. I worked around this using
bitfields, but,
- the number of bits used within a single mask register varies between
hardware. Also, the bits are not always consecutive, there can be
reserved bits or non-interruptmask-bits in between them.

The most effective solution here is to create a single member in the
struct for every interrupt enable bit (without grouping them by the
register that contains them). This might allow things to work on
different hardware, but it also requires every bit to be separately
checked, cleared and saved and I don't think the compiler is smart
enough to merge them back together in more efficient operations.
- gcc doesn't always generate efficient code when faced with bitfields
(at least that's what I think caused the assembly mess I saw). We
might be able to tweak the code to work around this, I haven't tried
yet.
- C++ doesn't support "designated initializers". This means we can't
write:
const InterruptMask Mask_INT0 = {.eimsk = INT0};
like in C99, but must explicitely initialize all elements:
const InterruptMask Mask_INT0 = {INT0, 0};
When the number of elements is variable based on macros, this will of
course be very messy. We might be able to work around this by e.g.,
having a "set" method or function for each register, though (haven'
tried yet).
- In my code, I've made the methods static, in order to allow passing
the struct contains by value instead of having to pass a (this)
pointer as for regular methods. However, I'm not sure yet if this is
a good idea (especially when the struct gets larger, passing by
reference might be better, though the compiler might take care of
this as well). Using a non-static always_inline wrapper method might
allow using the non-static public API while still (optionally)
allowing pass-by-value semantics, perhaps.

The most pressing problem is probably the big number of different
interrupt mask registers (I think 13 on the 328p). Having to check and
potentially save and modify 13 values is probably more overhead than
we'd like. We can probably get away with supporting a subset (nobody is
going to have to do SPI transfers directly after a write to flash is
complete (SPM write complete interrupt)), but at least the PCINT,
external interupt and timer interrupts make sense, which I think is 5
registers and 15 bits already.


As I said - I'm not so sure if this approach will lead anywhere, but I
wanted to share my thoughts and code in case it triggers a useful idea
from someone else :-)

Gr.

Matthijs

[Mikael Patel]

Yes, this is a good improvement. Below is the assembly list for SPI::transfer(void* buf, size_t size) when move the memory access is;

00003420 <_ZN3SPI8transferEPvj>:
    3420:    41 15           cp    r20, r1
    3422:    51 05           cpc    r21, r1
    3424:    b9 f0           breq    .+46         ; 0x3454 <_ZN3SPI8transferEPvj+0x34>
    3426:    fb 01           movw    r30, r22
    3428:    80 81           ld    r24, Z
    342a:    8e bd           out    0x2e, r24    ; 46
    342c:    ca 01           movw    r24, r20
    342e:    01 97           sbiw    r24, 0x01    ; 1
    3430:    41 f0           breq    .+16         ; 0x3442 <_ZN3SPI8transferEPvj+0x22>
    3432:    31 81           ldd    r19, Z+1    ; 0x01
    3434:    0d b4           in    r0, 0x2d    ; 45
    3436:    07 fe           sbrs    r0, 7
    3438:    fd cf           rjmp    .-6          ; 0x3434 <_ZN3SPI8transferEPvj+0x14>
    343a:    2e b5           in    r18, 0x2e    ; 46
    343c:    3e bd           out    0x2e, r19    ; 46
    343e:    21 93           st    Z+, r18
    3440:    f6 cf           rjmp    .-20         ; 0x342e <_ZN3SPI8transferEPvj+0xe>
    3442:    fa 01           movw    r30, r20
    3444:    31 97           sbiw    r30, 0x01    ; 1
    3446:    e6 0f           add    r30, r22
    3448:    f7 1f           adc    r31, r23
    344a:    0d b4           in    r0, 0x2d    ; 45
    344c:    07 fe           sbrs    r0, 7
    344e:    fd cf           rjmp    .-6          ; 0x344a <_ZN3SPI8transferEPvj+0x2a>
    3450:    8e b5           in    r24, 0x2e    ; 46
    3452:    80 83           st    Z, r24
    3454:    08 95           ret

I have tested this on the Cosa SD device driver and the FAT16 port. I also used the transfer_start/await in some of the SD member function and could improve the example sketch with approx. 25-50%.

CosaFAT16: started
free_memory() = 1011
sizeof(FAT16::File) = 18
NISSE.TXT     2013-12-13 10:32:48 13

Open file:9 ms
NISSE.TXT     2013-12-13 10:32:48 13
TMP.TXT       2000-01-01 01:00:00 0

Write file (100 KByte):1759 ms

Close file:18 ms
NISSE.TXT     2013-12-13 10:32:48 13
TMP.TXT       2000-01-01 01:00:00 102400

Read file (100 KByte):528 ms

Remove file:31 ms
NISSE.TXT     2013-12-13 10:32:48 13

Open/Write/Close file:65 ms
Nisse badar.
Open/Read/Close file:3 ms

Best regards, Mikael

[Mikael Patel]

On 05/02/2014 02:49 PM, Matthijs Kooijman wrote:
> The most pressing problem is probably the big number of different
> interrupt mask registers (I think 13 on the 328p). Having to check and
> potentially save and modify 13 values is probably more overhead than
> we'd like. We can probably get away with supporting a subset (nobody
> is going to have to do SPI transfers directly after a write to flash
> is complete (SPM write complete interrupt)), but at least the PCINT,
> external interupt and timer interrupts make sense, which I think is 5
> registers and 15 bits already. As I said - I'm not so sure if this
> approach will lead anywhere, but I wanted to share my thoughts and
> code in case it triggers a useful idea from someone else :-)

Using an interrupt bit mask seems to be an over-kill and may give
complex variant handling. The basic problem is to ensure that no
interrupts may come from SPI devices during the transaction (if they are
allowed to perform SPI operations). The most important things is that
the device drivers should not need to know about each other.

The solution in Cosa is as follows;

1. There is an abstract Interrupt::Handler interface with the essential
enable/disable interrupt. This is the base class of all interrupt
sources (External Interrupt, Pin Change Interrupt, etc).
2. There is an SPI::Device class that is used as the base class for SPI
device drivers. This class holds the necessary hardware setting when
talking to the device, chip select pin and possible interrupt source for
the device (often an external interrupt pin). The class is attached
(linked list) to the SPI handler.
3. The SPI class is the SPI hardware handler with the new begin/end
transaction member functions together with transfer (and some new member
functions for high speed block transfer). The handler holds a list of
all the device drivers attached. This list is used on transaction begin
to disable all the interrupt handlers and on transaction end to enable
them again.

The usage is:

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

This assumes that we are writing a member function in the SPI device
driver (therefore "this" as it is an SPI::Device). The spi.begin() will
do the following synchronized steps;

1. Check that the SPI handler is not busy.
2. Disable the interrupt handlers for the attached SPI device drivers.
3. Restore the SPI hardware registers (clock, mode, bit order, etc).
4. Assert the chip select.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.cpp#L77

The spi.end() basically does the reverse.

1. Some checking.
2. Removes the chip select assert.
3. Enables the interrupt handlers.
4. And marks the SPI handler as not busy.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.cpp#L374

This removes a lot of code from the SPI device drivers.

Cheers!

[Paul Stoffregen]

Cristian,

Now that the dust is starting to settle from Maker Faire, I'm hoping we can come to a consensus on the SPI transaction API, so I can prepare a pull request.

Here's my understanding of the outstanding issues, needing your guidance on what you'll accept.

0: Whether and when to add this API: Everybody seems to agree we need this.    Until it's been officially released by Arduino, submitting patches to the many SPI-based 3rd party libraries is almost impossible.

1: Interrupt numbers vs event callbacks: My preference is SPI.usingInterrupt(num), even though it requires building knowledge of interrupt masking into the SPI library.  I believe callbacks only push the difficult platform specific work onto 3rd party library authors.  We've also seen they're much slower, even for the simple case of only supporting a single library's callback.  Still, callbacks would keep the SPI library "clean" from having to deal with platform specific interrupt masking.  A decision needs to be made on this fundamental API question.

2: Interrupt abstraction layer: If we go with interrupt awareness, is a pull request with the existing code, with the plan to later abstract the interrupt stuff, acceptable for a first release? 

3: Runtime config optimization (Matthijs): The more I think about it, the more I like Matthijs's proposal.  I'd personally like to implement it as two SPI.beginTransaction() functions.  Would that be acceptable in a pull request?

4: Optimized block transfer (Mikael, Ralph): Should we include Mikael's fast SPI.transfer(buf, len) code?


I'm personally hoping we can wrap this up quickly and get it into the next 1.5.x beta.  I'm sure you have a mountain of tasks with the 2 new products and 2.0 IDE, but my hope is you can give me some feedback on how you'd like to see these and any other issues addressed.  I want to finish this up, submit a pull request, and once it's in a 1.5.x release, begin the much larger work of starting to submit patches to the many 3rd party SPI-based libraries that are today suffering compatibility troubles.


-Paul

[Cristian Maglie]

Hey Paul,

Thank for your concise summary, it has been a quite busy week indeed and I'm
still tidying up my task list..


> *1: Interrupt numbers vs event callbacks*:

This is the crucial point and, probably, the only real reason that keeped this
discussion going on for so long.
My conclusions are:

- Performance wise: I guess we can't do better than your initial proposal with
usingInterrupt(), there is no doubt on that.

- Another strong argument against an event-based API is that it doesn't scale
well when more than one event handler is required. I've just checked a basic
implementation with a fixed array of pointers and the results are very
discouraging, it's about 65% performance loss on my first benchmark, though in
a real scenario is probably not so disastrous, it doesn't look very promising.

https://github.com/cmaglie/Arduino/commit/44c745607482fcf4f7aa3f823e6c12faad243e9e

- Even if the event-based API is more powerful, modifying the exising
libraries is more complex, in particular the need that libraries must handle
interrupts themselves instead of delegating the control to SPI.

So, said that, let's go with your proposal of interrupt-based.

> *2: Interrupt abstraction layer*: If we go with interrupt awareness, is

> a pull request with the existing code, with the plan to later abstract
> the interrupt stuff, acceptable for a first release?

Yes.

About that I'm thinking if we can move all the transcation interrupt code in a
superclass that is extended by SPI, so it can be reused by anything that needs
to be transactional?

class Transactional {
void usingInterrupt(..);

void beginTransaction();
void endTransaction();
};

> *3: Runtime config optimization (Matthijs)*: The more I think about it,

> the more I like Matthijs's proposal. I'd personally like to implement
> it as two SPI.beginTransaction() functions. Would that be acceptable in
> a pull request?

uhm, two ways for doing the same thing is really counter-Arduino style. Since
you are proposing to do both probably we can try both and decide later which
one is better to keep? is it ok for you? If no, I like Matthijs's proposal
better and I would like to go with that.

> *4: Optimized block transfer (Mikael, Ralph)*: Should we include

> Mikael's fast SPI.transfer(buf, len) code?

Yes, BTW this change is unrelated and can also be done separately in any case.

> I want to
> finish this up, submit a pull request, and once it's in a 1.5.x release,
> begin the much larger work of starting to submit patches to the many 3rd
> party SPI-based libraries that are today suffering compatibility troubles.

I know that the odds are very high, but you're asking me to merge the
transaction API before actually verifying if it solves the compatibility
problems it was conceived for (or probably you've already tried it?). Before
giving the green light I'd like to see two incompatible libraries happily
working together once ported to the new API (Ethernet can be one of them since
you already ported it).

A last personal request: may you or everyone else give me some links to
discussion boards or forums talking about the problems with this SPI
incompatibily? I'd like to have a better grasp of this issue.

Finally, my apoligies for taking so long to make up my mind, this is probably
one of the most difficult decision in a year and a half.

C

[Matthijs Kooijman]

> A last personal request: may you or everyone else give me some links to
> discussion boards or forums talking about the problems with this SPI
> incompatibily? I'd like to have a better grasp of this issue.

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

That is the one I was aware of, wrt atomicity problems between SPI-using
libraries (Ethernet and RF22 in this case).

Gr.

Matthijs

[Paul Stoffregen]

Thanks Cristian. That's exactly the sort of decision making and
direction I need to move forward.

Over the last few weeks, I've built up a small collection of hardware
for the most troublesome SPI-based wireless libraries, specifically for
testing the effectiveness of this SPI transaction API.

Now I have quite a bit of SPI work to go do.... ;)

[Mikael Patel]

Here is a link to the Cosa SPI refactoring discussion on the Arduino forum: http://forum.arduino.cc/index.php?topic=150299.msg1398002#msg1398002

This post and the following describe some of the design reasoning before doing the API change. It also captures some of the requirements and the background.

Might be of interest.

Cheers! Mikael

[Paul Stoffregen]

Just a quick update... I'm finally working on SPI transactions again
after a long pause.

> uhm, two ways for doing the same thing is really counter-Arduino
> style. Since you are proposing to do both probably we can try both and
> decide later which one is better to keep? is it ok for you? If no, I
> like Matthijs's proposal better and I would like to go with that.

After working with his code more, I agree, Matthijs's proposal is much
better. My original approach really only works well for compile time
constants, and it required defining a long but still limiting list of
discrete clock speeds. Matthijs's proposal solves both those problems,
but still compiles to optimized code for the const case.

I've made a small change to simplify the syntax slightly.

Before:

SPISettings settings = SPI.getSettings(2000000, MSBFIRST, SPI_MODE1);
SPI.beginTransaction(settings);
SPI.beginTransaction(SPI.getSettings(1000000, LSBFIRST, SPI_MODE2));
SPI.beginTransaction(SPI.getSettings(random(), LSBFIRST, SPI_MODE2));

After:

SPISettings settings(2000000, MSBFIRST, SPI_MODE1);
SPI.beginTransaction(settings);
SPI.beginTransaction(SPISettings(1000000, LSBFIRST, SPI_MODE2));
SPI.beginTransaction(SPISettings(random(), LSBFIRST, SPI_MODE2));

I mention this with some reluctance. I'm wary of unleashing another
torrent of "discussion", but it is a minor change from what we'd
previously discussed, in at least 71 messages from April 14 to May 28.

Please, I beg of anyone who might object to the API in any way to please
read ALL prior messages in this lengthy thread BEFORE suggesting
anything other than what we've already agreed upon through this long
process. I know the temptation is great to quickly hit reply, but
rehashing the same topics will only delay solving these SPI
compatibility issues.

> Before
> giving the green light I'd like to see two incompatible libraries happily
> working together once ported to the new API (Ethernet can be one of them since
> you already ported it).

Over the next week, I will be testing RadioHead (RFM69), Adafruit
CC3000, Adafruit 2.8" TFT Touch Shield (ILI9340 & STMPE610) with
Ethernet and SD. One of the great difficulties is creating good test
cases, which reproduce the problem reliably and rapidly. Especially with
interrupt conflicts, lockups and wrong behavior are very sensitive to
the precise timing of related communication on 2 different devices.

> A last personal request: may you or everyone else give me some links to
> discussion boards or forums talking about the problems with this SPI
> incompatibily? I'd like to have a better grasp of this issue.

Here's a couple:

http://forum.pjrc.com/threads/26148-RFM69-tranceiver-with-Wiznet-820io-Ethernet-and-Teensy-3-1

http://forum.pjrc.com/threads/26030-Teensy-3-1-and-ILI9340-Display-Issue?p=50018#post50018

The trouble with this issue is many conversations are simply unresolved
problems, after much discussion of separate chip selects, tri-state
buffers on MISO, and similar issues. People depend on Arduino libraries
to take care of talking to the hardware properly. Very rarely do they
have the deep knowledge of communication details inside multiple
libraries needed to identify these sorts of issues. Often the discussion
ends with "you need a proper operating system, use a Raspberry Pi".

> Finally, my apoligies for taking so long to make up my mind, this is probably
> one of the most difficult decision in a year and a half.

I completely understand. This is a really tough issue requiring a
substantial API change. I've had several false starts and setbacks,
when I'd originally hoped to have this solved months ago!

My hope is to finally get it resolved, not only on Teensy, but for all
Arduino and Arduino compatible boards, and eventually get all the widely
used SPI-based libraries updated.

[Mikael Patel]

Great work Paul!

I have had some progress and refactored the Cosa SPI API to better support inter-leaving within larger transfer blocks. After looking at the code structure it became obvious that it was an iterator pattern. A new SPI member function has been added, transfer_next(), which does the combined transfer_await() and transfer_start() of the next byte. The block transfer cleans up very nicely with this pattern.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI.cpp#L232

The latest SPI device driver in Cosa is for a 16 M byte flash (S25FL127S). With the optimized block transfer the performance is approx. 600 K byte/s read and 200 K byte write. The improvement is 20-30% with the prefetch. Below is a link to the flash read() member function and benchmark/example sketch.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/SPI/Driver/S25FL127S.cpp#L56
https://github.com/mikaelpatel/Cosa/blob/master/examples/Sandbox/CosaS25FL127S/CosaS25FL127S.ino

This can be optimized further. But I am focusing on a file system for the flash driver right now; CFFS (Cosa Flash File System). Below is the initial API, implementation and example sketch.
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/FS/CFFS.hh
https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/FS/CFFS.cpp
https://github.com/mikaelpatel/Cosa/blob/master/examples/Sandbox/CosaCFFS/CosaCFFS.ino

Cheers! Mikael

[David Mellis]

One small (I hope) style point: it seems inconsistent with the style of most existing Arduino API's to use a separate settings objects rather than simply passing the relevant parameters directly to the appropriate function (i.e. beginTransaction() here). For example, Serial.begin() directly accepts the baud rate and other configuration. Was there an optimization advantage to doing so? How significant is it? (I looked over the previous discussion briefly; apologies if this has been covered and I missed it.) If it's just a question of style (i.e. avoiding the duplication of parameters), I'd suggest going with passing the parameters directly to SPI.beginTransaction(). 

--
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.

[Paul Stoffregen]

On 07/21/2014 11:16 AM, David Mellis wrote:

One small (I hope) style point: it seems inconsistent with the style of most existing Arduino API's to use a separate settings objects rather than simply passing the relevant parameters directly to the appropriate function (i.e. beginTransaction() here).

This was Cristian's decision (May 22) based on Matthijs's proposal (April 24).

Hopefully those dates will make finding those particular messages easier, in this lengthy thread that spans a few months.

For example, Serial.begin() directly accepts the baud rate and other configuration. Was there an optimization advantage to doing so? How significant is it?

For the case where inputs are not compile time constants, it's a very significant savings to pre-compute the settings.

When all 3 inputs are compile-time constants, both ways result in the same optimized code.

(I looked over the previous discussion briefly; apologies if this has been covered and I missed it.) If it's just a question of style (i.e. avoiding the duplication of parameters), I'd suggest going with passing the parameters directly to SPI.beginTransaction().

That's the way I'd originally proposed and implemented a few months ago.

After working with Matthijs's code and libraries like Adafruit_STMPE610 (which automatically detects chips using either SPI_MODE0 or SPI_MODE1), I believe Cristian made a very good decision.   Matthijs's SPISettings handles those non-const cases much better.

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.

[Paul Stoffregen]

On 05/22/2014 01:06 PM, Cristian Maglie wrote:

> I know that the odds are very high, but you're asking me to merge the
> transaction API before actually verifying if it solves the
> compatibility problems it was conceived for (or probably you've
> already tried it?). Before giving the green light I'd like to see two
> incompatible libraries happily working together once ported to the new
> API (Ethernet can be one of them since you already ported it).

I'm happy to report 2 test cases have now been verified.

#1: CC3000 and SPIFlash running on Wildfire (AVR '1284 chip) previously
caused SPIFlash to incorrectly read if CC3000 received incoming data.
Victor Aprea provided a test case, which I've verified is fixed by
patches to CC3000 and SPIFlash to use SPI transactions.

#2: Adafruit ILI9341 & STMPE610 previously did not work on Teensy 3.1
with the "touchpaint" example. These do work on Arduino Uno, because
Adafruit's library directly accesses the SPCR register, to save and
restore on every SPI access. Obviously that's an AVR-only solution.
I've verified patched copies work together on the non-AVR Teensy 3.1
board without any special SPI register access in either of those libraries.

Yesterday I talked with the Pinoccio board designers. They are planning
to send me a test case for troubles they've seen with sharing SPI
devices on their board.

Later this week, I'm planning to patch SD and other libraries, and test
more cases. If anyone reading this has cases of SPI libraries
conflicting, especially any interrupt-based libraries like CC3000,
RadioHead or nRF8001, please contact me. If you send a reproducible
test case (within the next few days), I'll get the hardware, patch the
libraries and verify.

I believe this SPI transaction work is getting very close to
completion. :-)

[Tom Igoe]

Yay! Thank you Paul!

t.

[Cristian Maglie]

In data mercoledì 23 luglio 2014 20:30:25, Paul Stoffregen ha scritto:
> I'm happy to report 2 test cases have now been verified.

Hey Paul, great news! :-)))))

Thanks for pushing this forward!

C

[Paul Stoffregen]

Here's another quick update on the SPI transaction work.

Actually an oscilloscope screen capture...  The green trace is the chip select for SPI usage from loop().  The blue trace is an interrupt request, which in this test occurred right when the first of 2 bytes was transmitting.

You can see the remainder of loop()'s SPI transaction completes and its chip select is de-asserted.  Then the interrupt is serviced, which begins using SPI with its own settings (a slower clock in this example).




Without SPI transactions, the interrupt would have been serviced immediately, while the green chip select was still low.  The interrupt routine would have asserted the yellow chip select and used SPI.transfer() while both SPI devices were selected, and at the higher clock rate that was in use at the time of the interrupt.

Historically, these types of SPI conflicts have been rare, partly because many people use only a single SPI chip, or multiple chips with similar clock and format settings with very simple polling-only code or libraries.  But with a new generation of SPI-based radio modules and more sophisticated libraries using attachInterrupt(), opportunity for conflicts has grown in the last year.  I'm happy to say this transaction API appears to be able to working.  :)

[Cristian Maglie]

In data lunedì 28 luglio 2014 01:27:16, Paul Stoffregen ha scritto:
> You can see the remainder of loop()'s SPI transaction completes and its
> chip select is de-asserted. Then the interrupt is serviced, which
> begins using SPI with its own settings (a slower clock in this example).

Wow, that's really exciting! It seems also to be very quick to change SPI
"context" to service the interrupt.

Thanks for sharing!

C

[Paul Stoffregen]

More good news on SPI transactions.

Today I tested with the SD library and Adafruit CC3000, using a sketch
that repetitively reads a file and compares with known data while
waiting for CC3000 to fetch a web page.

I was initially amazed to find this test often passes on AVR without
transactions. These libraries have considerable software overhead. The
AVR chip is pretty slow and the SPI is very fast, so timing window of
opportunity for the CC3000 interrupt to disrupt the SD library is
actually pretty small. The difficulty in reliably reproducing the
problem, even with an intentional test that repetitively re-reads the SD
card, really goes to show how insidious this problem is. Of course,
data errors do occur occasionally, and usually within 5 iterations, the
interrupt strikes at just the wrong moment to cause the sketch to
completely lock up.

With transactions, reading the SD card works perfectly. The CC3000
never causes wrong data reads. I've run 200 iterations (fetched the web
page 200 times, each time rapidly re-reading the file from the SD card)
without any errors or lockups. It works equally well on AVR and ARM.

Previously on ARM, the SD card couldn't be used at all, because
Adafruit's CC3000 library only has SPI save/restore for the AVR
registers. SPI transactions solve both the interrupt issue and the
differing settings issue, without any SPI hardwre registers used outside
the SPI library.

I'm feeling very good about this code. Maybe it's time for a pull
request to the 1.5.x branch?

[Cristian Maglie]

In data mercoledì 30 luglio 2014 14:48:13, Paul Stoffregen ha scritto:
> I'm feeling very good about this code. Maybe it's time for a pull
> request to the 1.5.x branch?

Sure, let's go.

C

[Paul Stoffregen]

>> I'm feeling very good about this code. Maybe it's time for a pull
>> request to the 1.5.x branch?
> Sure, let's go.
>

Here it is.

https://github.com/arduino/Arduino/pull/2223

Sorry about the extra delay. I spent the last couple days bringing the
code back into the 1.5.x branch and retesting on Uno, Mega & Due. The
later two required building up different shields with jumper wires to
for the SPI signals, since my original hardware was for only Arduino Uno
& Teensy. Testing on Due turned up an issue with my original interrupt
masking code. I'm happy to say I managed to rewrite that code and it's
now confirmed working with the SD + Adafruit CC3000 test case. That
long test case was also confirmed on Arduino Mega.

[Kristian Sloth Lauszus]

Thank you for working on this Paul!

As one of the developers of the USB Host shield library I really appreciate it - it allows our users to easily use other shields that uses SPI simultaneously :)

Best regards

Lauszus

[Matthijs Kooijman]

Hey folks,

while looking at the Ethernet library, I just noticed that the
beginTransaction API is different between avr and sam (I hadn't looked
at the sam version so far). This causes (IMHO) ugly stuff like this:

https://github.com/PaulStoffregen/Arduino/blob/8aaca2fbb662b293f7089f1505e4a8e922e49b0d/libraries/Ethernet/src/utility/w5100.h#L17-L21

(which I think is ugly because it differs between avr and sam, as well
as because it defines two arguments in a single macro, which is a bit
unexpected)

I'd like to get this fixed (i.e. making the API consistent). There's a
few ways I can think of:

1. Let avr also support the beginTransaction(pin, settings) version,
but just ignore the pin argument. This is the easiest, but it might be
confusing for people ("I'm passing the pin number, why doesn't it
toggle it for me?")

2. Put the SPI pin number inside the SPISettings object and leave
the reset as-is (so avr ignores it and spi uses it to toggle the pin).

3. Put the SPI pin number inside the SPISettings object and have it
toggled automatically on begin/end transaction (just like sam does now,
I think?).

4. Put the SPI pin number inside the SPISettings object and include a
"SS type", which includes value like "pull low" to pull low during the
transaction and "manual" to leave the SS pin untouched. The latter
allows for manual control, like doing multiple SS pin toggles per
transaction or other funky uses of SS.

However, looking more closely I remember this quirk of sam where you
need to specify the end of a transaction during the transfer of the last
byte, which is another difference between avr and sam (currently sam
uses an extra SPI_CONTINUE argument to the transfer method for this).
Ideally, we'd also remove this difference (looking at the at91sam
datasheet, section 33.7.3.5 says "Instead of LASTXFER, the user can use
the SPIDIS command", which suggests that we might be able to normalize
this part of the SPI API for sam).

Thinking on it more, it seems that there might actually be two (nested)
types of transaction-like concepts involved. The outer one is the
current transaction - during a transaction SPI is configured to talk to
a specific slave device and the transfer cannot be interrupted by
transfers to another SPI device. Inside there, there is the concept of
an SPI "transfer", which starts with asserting the SS pin, transfers
some bytes, and then deasserts the SS pin again.

It would help to make the inner "transfer" concept better supported
(probably needs a better name to prevent confusing with the single-byte
transfer method). On sam, you already indicate this explicitely by (not)
passing SPI_CONTINUE (which also causes the SS pin to be autotoggled).


I had a look at the datasheet to see if we can prevent the sam SPI core
from touching the SS/CS pins, but it seems to insist on selecting a pin
to assert during the transfer...


In summary: I don't quite like the differences in API - but perhaps the
hardware is so different that hiding the differences in a (simple) API
is impossible anyway...

Gr.

Matthijs

[Paul Stoffregen]

On 08/17/2014 10:46 AM, Matthijs Kooijman wrote:
> Hey folks,
>
> while looking at the Ethernet library, I just noticed that the
> beginTransaction API is different between avr and sam (I hadn't looked
> at the sam version so far). This causes (IMHO) ugly stuff like this:
>
> https://github.com/PaulStoffregen/Arduino/blob/8aaca2fbb662b293f7089f1505e4a8e922e49b0d/libraries/Ethernet/src/utility/w5100.h#L17-L21
>
> (which I think is ugly because it differs between avr and sam,

Arduino Due's SPI library has always had an extended API which simply
isn't implemented on AVR.

http://arduino.cc/en/Reference/DueExtendedSPI

I implemented the SPI transaction code for Due following this same
approach. Honestly, I considered not supporting Due's extended API (or
not bother with Due at all), but it is used by the Ethernet library, so
doing it this way seemed like the best approach to make it work with
Ethernet on Due.

> as well
> as because it defines two arguments in a single macro, which is a bit
> unexpected)

I agree that is unconventional. Perhaps some other way would be more
readable?

> I'd like to get this fixed (i.e. making the API consistent). There's a
> few ways I can think of:
>
> 1. Let avr also support the beginTransaction(pin, settings) version,
> but just ignore the pin argument. This is the easiest, but it might be
> confusing for people ("I'm passing the pin number, why doesn't it
> toggle it for me?")

If the two platforms are to become consistent, I believe implementing
the Due specific functions on AVR is really the only viable path.
Simply Ignoring the pin number isn't an option. If the functions are
implemented on AVR, they really need to actually work the same as they
do on Due.

But I do not necessarily agree with the importance of making the 2
platforms consistent. Hardly anyone uses the Due extended SPI API. The
upcoming Arduino Zero lacks this SPI chip select hardware feature.
Maintaining API consistency with this seldom-used, hardware-specific
feature is likely to require redoing this work for Zero and probably
other future boards. I guess if you're really passionate about API
consistency, then you might see it a worthwhile use of your time?

> 2. Put the SPI pin number inside the SPISettings object and leave
> the reset as-is (so avr ignores it and spi uses it to toggle the pin).
>
> 3. Put the SPI pin number inside the SPISettings object and have it
> toggled automatically on begin/end transaction (just like sam does now,
> I think?).
>
> 4. Put the SPI pin number inside the SPISettings object and include a
> "SS type", which includes value like "pull low" to pull low during the
> transaction and "manual" to leave the SS pin untouched. The latter
> allows for manual control, like doing multiple SS pin toggles per
> transaction or other funky uses of SS.

We previously discussed bringing the chip select pin into the
transaction API. The idea is popular when considering only common SPI
chips, but there are many special cases.

The Arduino world also has many SPI-based libraries already manipulating
the chip select pin (and additional pins for some chips), so leaving the
pin manipulation as-is allows for an easier migration path. This is my
main concern, once 1.5.8 is released, getting the many conflicting
libraries to update to this new API.

[Ralph Doncaster]

Now that 1.6 is out I'm looking at porting RF24 to the new SPI API.  What I can't figure out is how to make it portable between the sam and the avr.  From the sam code (SPI.h):

void beginTransaction(SPISettings settings) { beginTransaction(BOARD_SPI_DEFAULT_SS, settings); }

How do I use the transactions API on the SAM while doing manual slave select toggling, so the code will be portable between the AVR and the SAM?

[Andrew Kroll]

briefly...

begintransaction

digital write the enable pin low

do all your spi stuff

digital write the enable pin high

endtransaction

this also makes the entire use of SPI safe from within an ISR, and you don't have to worry if another driver has changed the spi settings.

--
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.

--

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

[Ralph Doncaster]

That makes sense for AVR, but not SAM.

Even if the beginTransaction(BOARD_SPI_DEFAULT_SS, settings) doesn't bring SS low, I don't see any way to transfer a block of data without using the CS pin handling.  The AVR version of SPI has the method:

transfer(void *buf, size_t count)

but this method does not exist in the SAM version of SPI, only:

transfer(byte _pin, void *_buf, size_t _count, SPITransferMode _mode = SPI_LAST)

It appears to be completely non-portable; I see no way of writing a single version of an Arduino that uses SPI so that it will work on both AVR and SAM.  What you end up with is library code that is hacked up with a bunch of #ifdef lines to cobble together two different code bases into one library.

[Cristian Maglie]

Hi Ralph!

Have you actually tried to compile the same sketch for AVR on SAM?

There are a bunch of overloaded functions to make compatible SAM and AVR
implementations:

https://github.com/arduino/Arduino/blob/master/hardware/arduino/sam/libraries/SPI/SPI.h#L83

in particular:

void transfer(void *_buf,

size_t _count,
SPITransferMode _mode = SPI_LAST)

{ transfer(BOARD_SPI_DEFAULT_SS, _buf, _count, _mode); }

should fit your needs.

C

> it, send an email to developers+...@arduino.cc <javascript:>.

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

> --
> 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

> <mailto:developers+...@arduino.cc>.


--
Cristian Maglie <c.ma...@arduino.cc>

[Ralph Doncaster]

Oh, I could get it to compile, but I can't see how it's going to work properly on the SAM.  Lets say I have:

const byte RF_SS =3;

...

spi.beginTransaction(...);

digitalWrite(RF_SS, LOW);

spi.transfer(buf, len);

digitalWrite(RF_SS, HI);

On the SAM, from reading the code, it looks like spi.transfer will assert BOARD_SPI_DEFAULT_SS(pin 4?) during the transaction.  So if there's another SPI device using pin 4 for SS, it will likely corrupt the data I'm getting from my device using pin 3 for SS.

I feel frustrated after looking at the code since it's such a mess.  There's two completely different class definitions, when sensible design practices would have only one SPI.h, and then different implementations of SPI.cpp for different platforms.  I'm an old Unix developer, and although some of Linux kernel isnt the way I'd do it, there's generally a method to the madness.  With the arduino cores, I cringe most times I start delving into the code.  And despite the pain, for some reason I keep coming back.  Kinda reminds me of my first wife. :-)

If there was an option to pass a null pin (i.e. no hardware SS), and the null pin is the default, then that would be a bit less messy.

My original suggestion months back was to have SS asserted in the spi transactions code for the AVR, but Paul said no to that.

> an email to developers+unsubscribe@arduino.cc
> <mailto:developers+unsub...@arduino.cc>.


--
Cristian Maglie <c.ma...@arduino.cc>

[Paul Stoffregen]

On 02/26/2015 06:46 AM, Ralph Doncaster wrote:

It appears to be completely non-portable; I see no way of writing a single version of an Arduino that uses SPI so that it will work on both AVR and SAM.

You are half correct.

Indeed, the "Extended SPI library usage with the Due" stuff, as documented here, only works on Arduino Due.

http://arduino.cc/en/Reference/DueExtendedSPI

If you use those functions, and SPI.beginTransaction(pin, settings) function, which is part of the "Extended SPI library usage with the Due" feature (even though beginTransaction isn't yet documented anywhere on Arduino's website), then indeed your code is tied to only Arduino Due.

That's why it's called "Extended SPI library usage with the Due".  That stuff only works on Arduino Due.


I suppose, technically, you're probably also correct in saying "I see no way".  But there is indeed a way.

The simplest way is to just not use the "Extended SPI library usage with the Due" functions.  Just use the regular way that works on all boards, where you use digitalWrite() or a write to the register from portOutputRegister(), to control the chip select pin.

With that regular, works-on-all-boards approach, you use just SPI.beginTransaction(settings).  That way does indeed work on all boards.  It's been very throughly tested.

But perhaps you don't believe me?  Maybe you're looking at the code inside hardware/arduino/sam/libraries/SPI.h and seeing that SPI.beginTransaction(settings) actually uses the other one with some default number.  Maybe that leads you to incorrectly conclude SPI.beginTransaction(settings) can't work on regular AVR boards?  Of course, whether or not SPI.beginTransaction(settings), digitalWrite(), regular SPI.transfer(data), and SPI.endTransaction() actually work properly on AVR boards is a simple matter of fact you can prove or disprove by simply compiling and running code.  I have personally done this many times.  You might have too, without realizing, when using the SD or Ethernet libraries, which do use SPI transactions and which do work properly on both AVR and SAM architecture.

 What you end up with is library code that is hacked up with a bunch of #ifdef lines to cobble together two different code bases into one library.

Well, yes, that's the other way.   If you want to optimize for specific hardware features that only exist on 1 architecture, then that's pretty much how things always end up.

I have personally ported and patched and contributed code to many of the libraries that exist for Arduino.  Code style and structure varies quite a lot.  I can tell you from experience, some libraries have done this sort of things much more elegantly than others.  There are many I would hardly characterize is "hacked up" and "cobbled together".

[Paul Stoffregen]

On 02/26/2015 08:05 AM, Ralph Doncaster wrote:

On the SAM, from reading the code, it looks like spi.transfer will assert BOARD_SPI_DEFAULT_SS(pin 4?) during the transaction.  So if there's another SPI device using pin 4 for SS, it will likely corrupt the data I'm getting from my device using pin 3 for SS.

No, you are mistaken.

If you actually test on an Arduino Due, or read the code more carefully, you'll discover SPI.beginTransaction(settings) does NOT cause pin 4 to assert low.

As a quick sanity check, I tested again just now, with this code:

#include <SPI.h>

void setup() {
  Serial.begin(9600);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  digitalWrite(3, HIGH);
  digitalWrite(4, HIGH);
  SPI.begin();
}

void loop() {
  //Serial.println("test");
  SPI.beginTransaction(SPISettings(8000000, MSBFIRST, SPI_MODE0));
  digitalWrite(3, LOW);
  SPI.transfer(0);
  digitalWrite(3, HIGH);
  SPI.endTransaction();
}

Here are the actual waveforms when this code runs on an Arduino Due.

[Ralph Doncaster]

On Thursday, 26 February 2015 12:15:32 UTC-4, paul wrote:

On 02/26/2015 06:46 AM, Ralph Doncaster wrote:

It appears to be completely non-portable; I see no way of writing a single version of an Arduino that uses SPI so that it will work on both AVR and SAM.

You are half correct.

Indeed, the "Extended SPI library usage with the Due" stuff, as documented here, only works on Arduino Due.

The simplest way is to just not use the "Extended SPI library usage with the Due" functions.  Just use the regular way that works on all boards, where you use digitalWrite() or a write to the register from portOutputRegister(), to control the chip select pin.

With that regular, works-on-all-boards approach, you use just SPI.beginTransaction(settings).  That way does indeed work on all boards.  It's been very throughly tested.

But perhaps you don't believe me?  Maybe you're looking at the code inside hardware/arduino/sam/libraries/SPI.h and seeing that SPI.beginTransaction(settings) actually uses the other one with some default number.  Maybe that leads you to incorrectly conclude SPI.beginTransaction(settings) can't work on regular AVR boards?  Of course, whether or not SPI.beginTransaction(settings), digitalWrite(), regular SPI.transfer(data), and SPI.endTransaction() actually work properly on AVR boards is a simple matter of fact you can prove or disprove by simply compiling and running code.  I have personally done this many times.  You might have too, without realizing, when using the SD or Ethernet libraries, which do use SPI transactions and which do work properly on both AVR and SAM architecture.

You're close but backwards.  My reading of the code indicates there'll be problems on the SAM.  Here's what I said:

"spi.transfer will assert BOARD_SPI_DEFAULT_SS(pin 4?) during the transaction.  So if there's another SPI device using pin 4 for SS, it will likely corrupt the data I'm getting from my device using pin 3 for SS."

You say it's well tested, but has anyone tested with one SPI device using BOARD_SPI_DEFAULT_SS and another using a different pin?  I only have AVR devices, so I can only go by what I see in the code.

[Paul Stoffregen]

Just in case there's any doubt left, here's the Arduino Due on my workbench right now, still hooked up to the oscilloscope.

I also did briefly move the channel 1 probe (yellow) over the pin 3, make absolutely sure I was measuring correctly.  My scope works well.  :)

[Ralph Doncaster]

On Thursday, 26 February 2015 13:17:58 UTC-4, paul wrote:

On 02/26/2015 08:05 AM, Ralph Doncaster wrote:

On the SAM, from reading the code, it looks like spi.transfer will assert BOARD_SPI_DEFAULT_SS(pin 4?) during the transaction.  So if there's another SPI device using pin 4 for SS, it will likely corrupt the data I'm getting from my device using pin 3 for SS.

No, you are mistaken.

If you actually test on an Arduino Due, or read the code more carefully, you'll discover SPI.beginTransaction(settings) does NOT cause pin 4 to assert low.

Thanks for the test, but I'm not saying SPI.beginTransaction will cause BOARD_SPI_DEFAULT_SS to assert, I'm saying spi.transfer looks like it does:

byte transfer(uint8_t _data, SPITransferMode _mode = SPI_LAST) { return transfer(BOARD_SPI_DEFAULT_SS, _data, _mode); }

And from looking at variant.h for the due I see that my guess for BOARD_SPI_DEFAULT_SS was wrong.  It's defined as pin 78:

#define PIN_SPI_SS3          (78u)

#define BOARD_SPI_SS3        PIN_SPI_SS3

#define BOARD_SPI_DEFAULT_SS BOARD_SPI_SS3

Is this a fake pin number?  If so, then the code should work, though BOARD_SPI_DEFAULT_SS would be better named BOARD_SPI_NULL_SS.  If there's a real pin 78, then I still think there's a problem.

[Ralph Doncaster]

According to the following page, pin 78 is A0 on the due.

http://forum.arduino.cc/index.php?topic=132130.0

[Cristian Maglie]

> Is this a fake pin number? If so, then the code should work, though
> BOARD_SPI_DEFAULT_SS would be better named BOARD_SPI_NULL_SS. If
> there's a real pin 78, then I still think there's a problem.

It's a real pin of the CPU (PB23) but it's not connected to any header.

// 78 - SPI CS3 (unconnected)
{ PIOB, PIO_PB23B_SPI0_NPCS3,ID_PIOB,PIO_PERIPH_B,PIO_DEFAULT,
PIN_ATTR_DIGITAL, NO_ADC, NO_ADC, NOT_ON_PWM,
NOT_ON_TIMER }, // NPCS3

IIRC it has been done this way because the SPI controller of SAM3X
always control at least one SS pin (you cannot use it to just shift
MISO/MOSI).

--
Cristian Maglie <c.ma...@arduino.cc>

[Paul Stoffregen]

On 02/26/2015 09:40 AM, Ralph Doncaster wrote:

According to the following page, pin 78 is A0 on the due.

http://forum.arduino.cc/index.php?topic=132130.0

*sigh* - you're just not going to let this go, are you?

Well, you're wrong on several points.

Pin 78 is not A0.  It's unconnected, pin #142 on the SAM3X chip.

Since I have the Arduino Due out on my bench (but I'm putting it away after this message), I careful probed pin #142 at the chip.  It is NOT pulsing.  I tried uploading a couple times, and indeed digitalWrite(78, HIGH) and digitalWrite(78, LOW) does control that unconnected pin.

My guess is it's not pulsing because the pin mux wasn't ever configured to route NPCS3.  Of course, why or even whether is a moot point, since pin #142 doesn't route to anything.  It's quite challenging to test, even with a good quality scope probe.

At this point, I'm done retesting SPI on Arduino Due for your sake.  It works.  If you want to keep arguing otherwise, I believe you really need to get an actual Arduino Due board and verify your claims.

[Ralph Doncaster]

Thanks for the factual and unemotional response.  As far I can tell this isn't documented anywhere.  I think it would be a good idea to document the fact that BOARD_SPI_DEFAULT_SS is defined to an unconnected pin on the due so that using SPI.transfer will not have any unintended side-effects.