beginTransaction in SPI library

I’m trying to grok what ‘beginTransaction’ does in the SPI library. It has nothing to do with transactions, right? It’s more like defining a channel to talk to one remote device on the bus using a specific select pin, right? I don’t see anything that guarantees atomicity or any other property commonly associated with the term transaction. It doesn’t even begin a bus transaction. So just confusing name?

Right, It configure the “transaction”. This is standard arduino API:
https://www.arduino.cc/en/Reference/SPIbeginTransaction

Anyway we extend/add some API:
https://github.com/stm32duino/wiki/wiki/API#spi

OK, now you have me totally confused .

I’m not an Arduino whizz, but as far as I can tell, SPI.beginTransaction goes back to Paul Stoffregen: https://www.dorkbotpdx.org/blog/paul/sp … in_arduino and was specifically created to provide atomic transactions.

The code I’m reading in the STM Core (a) does not implement any form of atomicity, and (b) by providing the (weird in my eyes) auto-switching of settings based on the CS pin actually promotes a usage that runs exactly counter the purpose of the arduino beginTransaction.

I just took a look at one of the official ARM cores and it’s very clear that the point of beginTransaction is to implement atomicity (which is what the term implies, duh). See https://github.com/arduino/ArduinoCore- … #L108-L119

Unless I’m missing something I would argue that there are two bugs in the SPI library: (1) it provides beginTransaction yet fails to implement atomicity, and (2) it implements an API extension that cannot work once atomicity is implemented.

I do not know very well SPI parts as it was mainly implemented by third party.
But as far as I see, we do not use IT mode for SPI.

What is “IT mode” and how is it relevant here?

[tve – Fri Aug 10, 2018 5:14 am] –
Unless I’m missing something I would argue that there are two bugs in the SPI library: (1) it provides beginTransaction yet fails to implement atomicity, and (2) it implements an API extension that cannot work once atomicity is implemented.

I can see (1) is an issue, but not how (2) would be an issue?

As far as I can see, the cspin extension simply provides internal storage of SPISettings, indexed by the cspin. Atomicity simply requires blocking interrupts while in a transaction. No idea why the two would be mutually exclusive?

Isn’t the main/only purpose of the ‘transfer(pin, …)’ set of functions to be able to write code that ends up performing operations like this:

SPISettings *s1(...), *s2(...);
beginTransaction(PA10, s1);
beginTransaction(PA11, s2);
transfer(PA10, ...);
transfer(PA11, ...);

Ah – you are being a bit generous with the term ‘atomicity’. As far as the Ardiuno API goes, that just means ‘disable any interrupts which may us the SPI bus’.

So, it still relies on the application to ensure that it uses the SPI bus in a sane way, but will prevent an interrupt from changing your bus settings unexpectedly.

Blocking interrupts will provide the same level of atomicity as the standard Arduino API.

[tve – Fri Aug 10, 2018 7:05 am] –
What is “IT mode” and how is it relevant here?

Interupt mode.

spi driver use HAL_SPI_TransmitReceive

Hmm, I feel like you guys are splitting hairs here. It is extremely clear that the purpose of beginTransaction is to provide some sort of atomicity and you create extensions that are only useful if they specifically break atomicity. That’s just bad practice.

@fpiSTM, whether you’re using interrupts with SPI is not the issue. Read the original article by Paul (I linked to it). The interrupt comes on a gpio pin from a radio. Handling the interrupt requires talking to the radio via SPI. But the main loop is also using SPI so if the interrupt from the radio comes in while the main loop is using SPI it messes everything up. Has nothing to do with using interrupts for the SPI device.

[tve – Fri Aug 10, 2018 8:07 am] –
Hmm, I feel like you guys are splitting hairs here. It is extremely clear that the purpose of beginTransaction is to provide some sort of atomicity and you create extensions that are only useful if they specifically break atomicity. That’s just bad practice.

It does not break atomicity – If interrupt blocking was implemented, it would be possible to use the standard API calls, and they would function exactly the same as the stock Arduino libraries.

The cspin stuff provides an alternative to the traditional API, and would actually work a lot better in 99% of the use cases (anything that does not use SPI from interrupts) than the current API. But that is only because the current API is hardly ever used correctly. endTransaction is rarely called, and beginTransaction is not called often enough…

I well understood.
Then main issue is to not break ongoing transfer by another interrupt which use also SPI else the transfer will be aborted.

You could submit an issue for that, in order to track it.

The cspin stuff provides an alternative to the traditional API, and would actually work a lot better in 99% of the use cases (anything that does not use SPI from interrupts) than the current API. But that is only because the current API is hardly ever used correctly. endTransaction is rarely called, and beginTransaction is not called often enough…

Do you have an example that uses the cspin stuff? Or some pseudo-code that shows its benefits?

WRT “work a lot better”, have you looked at the implementation? To configure any settings change spi_init is called, which is a pretty long function that derives every register bit from first principles, so to speak, and calls the HAL to boot. If you replaced that stuff by some optimized code you’d go faster. If you wanted to cache stuff, how about adding some private variables intot he SPISettings class where the register configuration is saved so a re-init is just a couple of writes into the registers with already-computed and vetted values? None of this needs to introduce weird API extensions…

[tve – Fri Aug 10, 2018 2:44 pm] –
The cspin stuff provides an alternative to the traditional API, and would actually work a lot better in 99% of the use cases (anything that does not use SPI from interrupts) than the current API. But that is only because the current API is hardly ever used correctly. endTransaction is rarely called, and beginTransaction is not called often enough…

Do you have an example that uses the cspin stuff? Or some pseudo-code that shows its benefits?

WRT “work a lot better”, have you looked at the implementation? To configure any settings change spi_init is called, which is a pretty long function that derives every register bit from first principles, so to speak, and calls the HAL to boot. If you replaced that stuff by some optimized code you’d go faster. If you wanted to cache stuff, how about adding some private variables intot he SPISettings class where the register configuration is saved so a re-init is just a couple of writes into the registers with already-computed and vetted values? None of this needs to introduce weird API extensions…

I have never actually seen this API used… Consider the following scenario:

A TFT touch screen connected to one SPI bus, but with separate CS pins for the display and touch screen controller.

1) Using the cspin API

The TFT library and touch library both call SPI.beginTransaction() from their begin() functions, specifying their CS pins and settings.

In future when any SPI read/write/transfer method is called from either library, that SPI method checks if it is the same CS pin as was used last – if so, it continues directly with the IO operation. If not, spi_init() is called to change settings for the requested CS pin before performing the IO operation.

So the correct SPI settings are used for each library, and spi_init() is only called if the other library has used the SPI bus since the last call.

2) In the traditional API.

Both libraries always call beginTransaction() before issuing any bus reads/writes. beginTransaction() always calls spi_init(). So even if you do multiple tft.print() calls, then one touch() call, spi_init() is called for each and every library call.

(1) is obviously a lot more efficient, as it allows the SPI library to reuse the current settings for each tft.print() call, as long as no touch() call has been made.

Yeah, so if the SPI library simply cached the last used cs pin and its associated settings it would be no less efficient.
Plus you exactly confirmed my suspicion, which is that if beginTransaction/endTransaction were implemented correctly and actually disabled interrupts then interrupts would be disabled for your entire sketch’s lifetime. That may be a nice hack in the case of your hypothetical sketch but really bad for an “official STM arduino core”.
The concept of registering a number of SPISettings so they can be pre-evaluated and the specific register values cached and then only applying them when necessary is a good one, but mixing this into beginTransaction is misguided and plain wrong. Sorry for being blunt, but I don’t have any other words for it.

Unfortunately, large portions of the Arduino APIs are really terrible. There is a lot of legacy cruft that is just dragged forwards, but can never be wholly removed without breaking compatibility.

In the case of SPI, beginTransaction() only actually marks the beginning of an atomic transaction if usingInterrupt() is called first. If not, then it is only used to set up the SPI settings. If you have to browse all the various libraries which use SPI, you will find that the vast majority just call beginTransaction once to set up the SPI port, and never even bother with endTransaction.

Yes it is bad, but has to be retained for compatibility.

The cspin API modification works well within this context, but as it is STM32 (and HAL core) specific, it hardly ever gets used.

The interrupt thing Paul S. was trying to address is more of a problem when you only have one SPI peripheral. When you have a chip, that has multiple SPI peripherals, it is easy enough to put a interrupt happy network SPI device one SPI and put the SDCard SPI interface on a different one. Problem solved. They don’t conflict and you are better using the more advanced hardware.

Oh yeah, the CC3000 he was trying to deal with, most people have lost interest, for a couple of reasons but mostly with the introduction of the esp8266, at least for hobbyists.

heisan: It seems that your argument is that the Arduino API is so terrible that adding another ill-conceived API to it is a good thing… Plus what you write is that the API is rarely used correctly, which is very different from saying that the API is terrible. Paul is a smart guy and he designed the transaction API.
Rick: Are you arguing that since there is a different solution might as well botch the API implementation and add a confusing ill-conceived unnecessary extension on top?

While I think third party cores should try to be faithful to what the Arduino does, the Arduino core is a moving target that is changed at their whim and has no real specification other than the web pages.

Are you actually using this feature? I think doing SPI transactions in an interrupt handler is a massively stupid approach.

The original design/idea of SPI transactions may have been good. But unfortunately now beginTransaction() is the recommended way of setting SPI parameters, whether you actually intend to use transactions, or not.

You use the additional call usingInterrupt() to actually turn on transaction mode, and then beginTransaction() starts the transaction. That is the API definition, and that is what the cores have to try and implement.

The cspin API extensions work within these semantics. While it may be better to create a whole new API with appropriately named functions, it is probably better to stick with the semantics developers are familiar with. So just add CSPIN as the first argument to all your existing SPI calls, and they suddenly become orders of magnitude more efficient. No other changes required, or new APIs to learn.

[Rick Kimball – Fri Aug 10, 2018 8:07 pm] – …Are you actually using this feature? I think doing SPI transactions in an interrupt handler is a massively stupid approach.

Here I must agree, if you ever end up doing SPI or Wire calls inside an interrupt handler, you probably have a lot more to worry about than transaction safety…

More interesting yet, is that blocking interrupts does not stop yield() calls, so you would still need another interlock mechanism if you use yield() for concurrency.