SPI library optimization

[d21d3q]

Can I refer to problem described here?
Actually it refers to both stm32duino and other libraries.
beginTransaction could check if SPI is initialized and skip it. transfer also seems to take longer on Nucleo rather than on Nano (see images in referenced issue).

[fpistm]

Hi @d21d3q,
yes you can ;) any feedback, contributions,... are welcome.
You're right, I think it can be optimized.

[d21d3q]

I thought that it will be easier, but since SPI has to work with default settings (without beginTransaction), it needs to be reconfigured in every endTransaction...

How about making function SPIClass::check_and_init_spi(...) which holds last config in SPISettings currentSettings and reconfigure spi if fed with new setting, but leaves it as is if nothing has changed?

[fpistm]

Last CS pin used and its SPISettings have to be checked to ensure all is ok. This implies to not remove it from SPISettings spiSettings[NB_SPI_SETTINGS]; at endTransaction.
else one other optimization could be to use LL instead of HAL.

[matthijskooijman]

I've also been looking at the SPI code today, and I'm a bit confused about how the CS pin handling works. It actually looks like there might be some misunderstanding about how the beginTransaction()/endTransaction() is intended to work in the Arduino API. IIUC, here, they are used to associate a CS pin number with an SPI configuration (so you would typically call beginTransaction() only once at startup, I think?).

To better understand the Arduino SPI API, let me explain how this API came to be. Originally, the SPI library on AVR worked like this:

• You call begin(), passing the settings for clock speed, mode, etc.
• To run an SPI transaction, you manually activate the CS pin. This signals the slave that a transaction started.
• You then call transfer() a number of times to transfer bytes.
• You then deactivate the CS pin.

This worked well for simple sketches, but was a bit problematic for sketches that needed to talk to multiple devices, especially when the SPI communication and initialization happened inside libraries. Since there is only a single set of SPI settings, all communication would use the same settings, which is the last set of settings passed to begin().

To prevent this, we invented the SPISettings object (so you can easily and compactly store the settings to use for a specific device) and the beginTransaction() and endTransaction() methods. This was supposed to work like this:

• You call begin(), with or without default settings (does not really matter).
• To run an SPI transaction, you call beginTransaction(), passing the settings to use for this transaction, the let the SPI library know a transaction is starting, and configure the SPI hardware accordingly.
• Then you manually activate the CS pin.
• Then you call transfer() to transfer bytes
• Then you manually deactivate the CS pin.
• Then you call endTransaction() to let the SPI library know the transaction is complete.

Mixing both approaches was also valid, but then the "legacy" code (that did not use transactions) would simply use whatever config was used by the last transaction (leaving it potentially just as broken or working as when it used the configuration from the last begin()).

In addition, this transaction approach had one extra advantage: It could also be used to disable certain interrupts during every SPI transaction when you know that those interrupt handlers are going to use SPI transfers as well (this is what the not-so-greatly-named usingInterrupts() function is for).

Furthermore, the SAM MCU (Arduino Due) has support for hardware-controlled CS pins, so it added a variant of beginTransaction() that accepts a pin number to be used as CS pin (removing the need for the sketch to toggle the CS pin).

However, now I look more closely at the SAM implementation, it actually seems that most of the API in the STM32 core actually comes from the SAM core. Looking there, they also pass pin numbers to transfer and store configuration. However:

• I think that this API (in particular the mode argument to transfer) predates the transaction methods. In hindsight, the SAM core should really be changed to do CS pin management in beginTransaction() and endTransaction() and deprecate the pin arguments on transfer, but I guess the SAM core won't see much development anymore.
• The SAM SPI hardware actually has 4 fixed CS pins and associated channels, and has different SPISettings (at least a clock phase and polarity) associated with each channel. For this reason, it makes some sense to actually store multiple sets of settings, though even that would be easier to just set up every time.
• The SAM SPI hardware seems to disable the CS pin after a transaction by setting a flag on the last byte transferred, which explains the somewhat dubious API of telling transfer that it is the last byte.

I actually think that most of this API does not really make sense (at all, and even less on STM32). In fact, I think that there is not even any real point in letting the hardware manage CS at all. It just adds confusion, is not really portable and only helps for a single slave device, for the others you need to manage CS manually anyway.

So I would think the SPI library can be greatly simplified, when the regular API outlined above (as used by most other Arduino cores) is adopted. In particular:

• The array saving 4 SPISettings objects can be removed, since every transaction will pass the needed settings at the start.
• The mode argument to transfer can be removed, since transfer just transfers a byte, while beginTransaction() and endTransaction() can manage the CS pin (if needed).
• The pin arguments could be removed everywhere.
• Maybe even remove setBitOrder() and similar functions (which are deprecated by Arduino) and expect sketches to use the "new" (has been around for years) transaction API exclusively. I'm not entirely sure if the Arduino library ecosystem is ready for this, though. This would also fix some duplicate code between these functions and SPISettings.

[fpistm]

@matthijskooijman
Firstly, it is documented here:
https://github.com/stm32duino/wiki/wiki/API#new-api-functions-1

I know, this could be improved. This "driver" has never been updated nor improved while it should. That's the purpose of this issue.
It was made to be Arduino UNO compliant, which was the first goal of this core.
For several users who made small project and use only one SPI device then the hardware CS can be a good option. I'm agreed for other users who use sever SPI devices on the same SPI instance this is not.

I agree that all your remark's make sense and have to be take in account for this rework.

[AnHardt]

I'm missing a simple way to set up a SPI-slave.