Better support for SPI-bus sharing.

[tfry-git]

ILI9341_STM uses SPI.beginTransaction() to switch between "fast" and "safe" speeds,
internally. However, it does not initialize the speed for every drawing operation
(for performance reasons, I presume), but rather only before and after operations
requiring the "safe" setting.

This strategy breaks, when the SPI bus is shared with another device (such as an
XPT2046 touchpad driver on the same display, for instance). To handle this case, this
patch exposes the appropriate beginTransaction() call in the API, for the user to call
when switching between devices (and endTransaction() for good style).

[stevstrong]

Why breaks that strategy? What is the problem when both devices uses the same frequency?

[rogerclarkmelbourne]

Steve

This sounds a bit like the bug I PM'ed you about

I noticed yesterday that my nRF905 code no longer worked (I've not looked at it for ages)

And I tracked down the problem to be caused by the SPI bus being left in 16 bit mode when SPI begin() exists

I'm not sure why begin() does that just before it returns

If I add code to set it back to 8 bit, the nRF905 shares the bus with the ILI9341

But FillScreen does not work, as it appears to need 16 bit bit the lib is not setting SPI to 16 bit prior to the transfers

I would have thought that the SPI bus should be left in 8 bit mode, and only set to 16 bit when needed, however the code seems to almost being doing the complete opposite

[tfry-git]

What is the problem when both devices uses the same frequency?

If both devices are fine with the same SPI settings, then all is fine. If the other device needs different settings, then it will have to leave the bus at those settings after every access. Even if a driver lib tries to be super-cooperative, there does not seem to be an API to backup and restore SPI settings to the previous setting (it would be rather nice, but use RAM, if endTransmission() did just that).

In the better case, all that happens is that the SPI bus is simply left at a lower speed, and graphics become slower. In the worst case the settings involve incompatible bit order / mode, and nothing will work.

Solution a) is to make sure a proper beginTransmission()/endTransmission() is called by every public API graphics call (not just those involving reads). Solution b) is what I propose, here.

[stevstrong]

OK, so there are 2 issues here.
This PR is partially needed because the ILI9341 driver uses the SPI in 16 bit mode.
One solution would be to set the ILI9341 driver to use 8 bit mode per default and switch to 16 bit mode only when needed. Actually, the 16 bit mode is mostly needed, so it would bring performance cuts (increase run-time) if we switch to 8 bit mode per default.
I have to agree that this PR gives an universal solution to the current situation, and allows sharing the SPI bus with other devices having any other difference in SPI configuration (e.g. different mode, or speed).
So this has green light from my side.

P.S. Roger, the current ILI9341 driver in your repo misses CS deactivation in some places, that can be the reason why it sometimes conflicts with other chips even when set to 8 bit mode, or fails under special circumstances (see OV7670 applications). Lets discuss this in a dedicated forum thread.

[rogerclarkmelbourne]

Hi Steve

OK.. I don't know if you have already started a thread on the forum.. I could not find one .

Re: CS / CE issues

This was not the problem in my case.
It was simply that the nRF905 won't work in 16 bit SPI mode.

I understand that defaulting the transfers to 8 bit will cause loss in performance, however Arduino is normally more about compatibility than speed.
So I'm surprised that more people have not noticed this problem

But perhaps hardly anyone has more than one device on the SPI bus, or perhaps those other devices will work in 16 bit mode. (more likely no one shares the SPI bus)

[stevstrong]

It also may happen that the other device uses different speed, so that this PR makes anyway sense.

[victorpv]

Personally I think the peripheral should be set to 16bit mode only when needed, and back to 8bit mode right after the transfer finished. I think after all the optimizations Steve did in the non-dma code, the difference should not be that much, and DMA transfer should be nothing or almost.
If this pr brings the peripheral back to 8bit mode (or whatever the other settings are), then I agree with it too.

[tfry-git]

This PR does not set the bus back to 8 bit. Although that would be trivial to add inside the new endTransmission(), I think it is wrong to assume that there is any default mode for the SPI bus. 8bit will be ok for more devices than 16bit, but that still leaves speed and bit order as parameters. As soon as the bus is shared, each device driver will have to assume that it will have to set up the bus anew before each transaction.

Now if there was a clean way to revert the bus to the previous settings (whichever those were), then I agree that would be a neat way to play nice with drivers that were not written with bus sharing / transactions in mind. Alas, there is no API for this, as far as I am aware.

[rogerclarkmelbourne]

The problem with sharing only came to light because the bus was left in 16 bit mode.

I can't see any mention of 16 bit in the official arduino SPI

https://www.arduino.cc/en/Reference/SPISettings

or even the Due extended API

https://www.arduino.cc/en/Reference/DueExtendedSPI

So in terms of compatibility with standard arduino libraries, I think it would be reasonable to expect them to assume that the bus is set to 8 bit

Re: reading the current settings

Its normally possible to read the current settings on most STM32 registers

So SPI lib could be changed to include this

[stevstrong]

Even if the default setting would be 8 bit, the sharing device on the same bus could have different mode or speed settings, as Thomas said, so that the setting switch would be anyway needed.
I think the Arduino best practice (at least in Ethernet lib) is that before each SPI access the beginTransaction() is called. This is suboptimal.
So finally I think that each device (lib) block access (group of functions) on a shared bus should call its own beginTransaction(), and that would solve any discrepancy between different settings.
An intermediate solution IMO would be to store the SPI settings in beginTransaction() and restore it in endTransaction().

[rogerclarkmelbourne]

I think the problem is that there are lots of old libs which don't call beginTransaction, but do set things like the speed as there are separate API calls for this

I guess its hard to know how widespread the problem is

[victorpv]

If I understand right, endTrasaction doesn't do anything? I just checked the F1 SPI code, and has some piece of code borrowed from the SAM which I guess does nothing, but is that what's expected?
If so, what is it used for?

I would have thought the API for beginTransaction and endTransaction was exactly what Steve listed as a solution, save the settings and restore the settings.
Adding that is an option, as pointed by Steve, but could that cause problems on libraries that expect the settings to stay even after endTransaction?

[tfry-git]

You are right, endTransaction() does not do anything on F1, and essentially I just added it for style. In the AVRs endTransaction() re-enables interrupts, that have been (optionally) blocked in beginTransaction() (see https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/libraries/SPI/src/SPI.h). Not sure whether this, or anything else will ever be needed for STM32 in endTransaction().

That said, I think a library that calls endTransaction() without beginTransaction() is likely broken to start with, but it's probably not safe to assume that all libraries calling SPIClass::beginTransaction() will also have matching calls to SPIClass::endTransaction() (and this one does not). So what to do, when there are two calls to beginTransaction() without an endTransaction() in between?

[victorpv]

Since the 16BIT mode is something exclusive to the F1, I think the best approach is to leave it to the whoever decided to use the 16Bit mode to make sure is back in 8bit mode after the peripheral is used. It takes a few cycles, but generally the gains from using 16bit should offset that, and if the gains are not enough, then just use the SPI peripheral in 8bit mode.
I think it's safe to asume the bus will be shared in many cases, and the other libraries using it may not expect it to be in 16bit mode.
We could add code in endTransaction to always return the SPI to 8bit mode. That way if a library needs 16bit, uses beginTransaction to switch to 16bit mode, and then after finishing it calls endTrasaction, which sets it back to 8bit mode. We put a big warning in the comments for beginTrasaction with 16bit setting, that endTrasaction should be called to return the devide to 8bit.

[madias123]

I also ran into the 8/16bit pitfall.
Some input of mine:
The ILI9341 comes with:
TFT
SD-Card
XPT2046 touch control (optional)
To bring all 3 devices onto one SPI port is a lot of SPI switching in the (user)code.
So now the lib is a real pitfall - even for "senior" users.

Offtopic: There are plenty of other fish in the sea..... I just ran into:
https://github.com/marekburiak/ILI9341_due
There are nice functions, really nice functions (arcs for example) :) Maybe I lend some of them for "our" library.
Offtopic 2: I compared the newest ILI9341 version from Adafruit and I really stopped reading the code (after the third #ifdef ifndef in line 4 or something), because it's nearly unreadable....

[stevstrong]

I vote for this PR to save the SPI settings at beginTransaction() and restore it with endTransaction() on ILI9341 level, not on SPI driver level.
Then I think this would solve any further bus sharing issue for ILI9341. But other devices may require their own transactions as well if they use, for example, different max. speed.

[victorpv]

Guys check the SPI library in the STM official repo. I think that's an elegant solution.
https://github.com/stm32duino/wiki/wiki/API#spi
I believe the teensy uses the same API. It will not resolve the problem completely for older libraries, but at least make it easy for a library to apply the required setting by just using the CS pin number, and if teensy and STM cores (and possibly the DUE eventually) there is a good chance the use will be widespread eventually.

[stevstrong]

If I understand it correctly, these are functions which you can use on low level to do mixed SPI accesses on the same shared bus.
But here we are facing a different problem, having 2 libraries accessing the same bus. So you don't access the bus in a single mixed way, but group-wise, on library level. One or more functions belonging to first lib, then one or more functions belonging to second lib.
So here the point is to differentiate SPI accesses of different libraries on main level, not on low (driver) level.

[stevstrong]

After a long inactivity time I wonder whether this PR is really needed.
I don't think the current version would solve the original issue because the SPI driver does not stores-restores the SPI settings at begin and end of transaction, respectively.

The easiest way to overcome SPI setting conflicts on the same shared bus is to call
SPI.beginTransaction(SPISettings(...));
with the corresponding settings, at the beginning of handling section for each module in part.

If nobody has any objections, I will close this after a week without merging.

[tfry-git]

It's been a while since I last looked at this. However, as far as I recall, the problem is that the ILI9341-code does not call beginTransaction at the begin of each transaction. It assumes it has set up the bus, correctly, once, and calls begin/endTransaction only for switching to/from "safe" speed for a select few operations.

Calling beginTransaction from the calling code will require that code to know the correct settings. Therefore, either:

• ILI9341 should call beginTransaction and endTransaction at every public API entry point, or
• It should provide the suggested trivial API to allow the calling code to do so.

[stevstrong]

I see the things a bit differently.

The ILI9341 driver does not call SPI.beginTransaction() for each SPI access.
I know, some official Arduino libraries does that, but this is sub-optimal, especially in case of display drivers where speed is an issue.
But even if the ILI9341 driver would do that, it would not solve the problem of sharing the same SPI port with other drivers, when other drivers do not do the same, but have different SPI settings.

This PR suggests to implement an ILI9341 function to init the SPI as needed, but it will leave the SPI bus with the same settings, not the previous one, because the SPI.end() function does not restore the previous SPI settings, so calling it makes no sense.

I checked shortly the SPI lib, it would need some development in order to implement the restore functionality. I would welcome any PR which would implement it. But without that, I don't think this PR is useful.

Instead, I suggest to call SPI.beginTransaction() before each function block of a driver.
For example:

SPI.beginTransaction(<lib1_settings>);
lib1_function();
...
SPI.beginTransaction(<lib2_settings>);
lib2_function();

I know this is not a perfect solution, but it works and is more flexible than the one suggested by this PR.

[tfry-git]

You are right, the suggested endTransaction()-function does not actually do anything, it is merely added for completeness and to allow good coding style. I don't really care about that bit. If this is the problem, let's just remove it.

However - as you write - what needs to be done for bus sharing is:

lib1_function();
...
SPI.beginTransaction(<lib2_settings>);
lib2_function();

But now, what is <lib1_settings>? That's something library1 should know, not something the calling code should have to worry about (or should make any assumptions about). And that's exactly the point of the PR: Providing a sane and reliable way to call SPI.beginTransaction(<lib1_settings>) (as lib1.beginTransaction()). No more, no less.

[stevstrong]

I must agree with you on this, but if the user does not know about the right settings for each lib in part, then this applies to the rest of libraries, too, right? So obviously they should have a similar function in order to avoid conflicts.
But what to do when they don't have such a function?

[tfry-git]

True, lib2 will need to either provide the same, or to init the SPI bus on every call. Quite a number of libs don't have to worry about performance so much (they have fewer calls to their API), and do just that.

Libs that don't provide either will be very difficult to use on the same SPI bus.