fix: SPI: improve compatibility with external libraries

[facchinm]

Adding SPI_HOLD_ON_CS ensures clock and data lines are not released until endOfTransaction()

[iabdalkader]

Does this actually fix the issue? The lock just prevents other callers from accessing the device, and HOLD_ON_CS just makes it not release CS and not disable the SPI device after the transfer is complete.
I scanned the linked issue and the issue it references, quickly, and as far as I can tell there is no solution other than DMA.
If somehow this does help, I think hold-cs should probably be an option passed via settings (added to API etc..).

[mjs513]

As @KurtE's issue pointed out this seems to be an open issue on Zephyr: zephyrproject-rtos/zephyr#73930

Has anyone looked at that or at @KurtE's solution? Not sure that is really going to resolve the issue

[KurtE]

Thanks @facchinm @iabdalkader @mjs513.

@facchinm - which boards did you try this on?

As I mentioned in the issue #145, in the past I have tried this approach to blanketly turn on the HOLD_ON_CS flag or
equivalent, which helps in some cases, in that at least on some boards, it then avoids the delay before release of CS and the delay after the CS is asserted. However on some boards/devices I ran into issues, where maybe it hung SPI, especially with multiple
devices and/or using the hardware cs pin(s). Some of these experiences may have been IMXRT specific, but I know on the
Teensy, several of our display drivers would not function properly with setting it globally, so our output code had
different helper functions to output with continue and output last byte.

As @mjs513 mentioned - there are some Zephyr open issues on some of this, that appear to have several interrelated issues and or things to try, like:

FIFO - is it enabled? Does it work.

Is DMA enabled?

Async Transfer - appears to speed things up, but ran into limits, like there may be a MAX number of bytes you can do
in a transfer. Also on the callback, can you reissue it with updated source pointer? ...

And my guess is, it may be different for each different processor.

But keeping my fingers crossed that this at least improves it.

Thanks

[KurtE]

Quick update: I tried out my sort of simple ST7796 driver on Q which does some hacks, to get hold of the underlying SPI
object

  uint32_t *p = (uint32_t *)_pspi;
  _spi_dev = (const struct device *)p[1];

And then does several fillRects with different colors and prints out how long it took...
Current released code, about 9962ms to output:

  tft.fillScreen(ST77XX_BLACK);
  tft.fillScreen(ST77XX_NAVY);
  tft.fillScreen(ST77XX_DARKGREEN);
  tft.fillScreen(ST77XX_DARKCYAN);
  tft.fillScreen(ST77XX_MAROON);
  tft.fillScreen(ST77XX_PURPLE);
  tft.fillScreen(ST77XX_OLIVE);
  tft.fillScreen(ST77XX_LIGHTGREY);
  tft.fillScreen(ST77XX_DARKGREY);
  tft.fillScreen(ST77XX_BLUE);
  tft.fillScreen(ST77XX_GREEN);
  tft.fillScreen(ST77XX_CYAN);
  tft.fillScreen(ST77XX_RED);
  tft.fillScreen(ST77XX_MAGENTA);
  tft.fillScreen(ST77XX_YELLOW);
  tft.fillScreen(ST77XX_WHITE);
  tft.fillScreen(ST77XX_ORANGE);
  tft.fillScreen(ST77XX_GREENYELLOW);
  tft.fillScreen(ST77XX_PINK);

Note: the fill code, uses temporary buffer that it fills with color and iterates calling spi_transcive....

    beginSPITransaction();
    setAddr(x, y, x + w - 1, y + h - 1);
    writecommand_cont(ST77XX_RAMWR);
    setDataMode();
#if 1
    uint32_t count_pixels = w * h;
    uint16_t array_fill_count = min(count_pixels, sizeof(s_row_buff)/sizeof(s_row_buff[0]));
    struct spi_buf tx_buf = { .buf = (void*)s_row_buff, .len = (size_t)(array_fill_count * 2 )};
    const struct spi_buf_set tx_buf_set = { .buffers = &tx_buf, .count = 1 };
    for (uint16_t i = 0; i < array_fill_count; i++) s_row_buff[i] = color;
    while (count_pixels) {
      spi_transceive(_spi_dev, &_config16, &tx_buf_set, nullptr);
      count_pixels -= array_fill_count;
      if (count_pixels < array_fill_count) {
        array_fill_count = count_pixels;
        tx_buf.len = (size_t)(array_fill_count * 2 );
      }
    }

#else  
    uint16_t color_swapped = (color >> 8) | ((color & 0xff) << 8);
    for (y = h; y > 0; y--) {
      #if 1
      for (uint16_t i = 0; i < w; i++) s_row_buff[i] = color_swapped;
        _pspi->transfer(s_row_buff, w * 2);
      #else
      for (x = w; x > 1; x--) {
        writedata16_cont(color);
      }
      writedata16_cont(color);  // was last
      #endif
    }
#endif
    endSPITransaction();
  }
  // printf("\tfillRect - end\n");

With the beginTransaction code change, this code now appears to hang.
If in the above I change the #if 1 to 0 to use the transfer function, it does not hang...
However the timing goes up to: 32842
or about 3.3 times slower.

And using SPI->transfer16... (I could try again, but that was potentially even slower)... And maybe has other issues.

EDIT: In case anyone wishes to play along.
The ST77xx library is up at: https://github.com/KurtE/Arduino_GIGA-stuff/tree/main/libraries/ST77XX_zephyr
Test sketch: https://github.com/KurtE/Arduino_UNO_Q/tree/main/Test%20Sketches/fillrect_test

[iabdalkader]

@KurtE Thanks for taking the time to test this. Could you please provide a brief summary? Does the change in this PR work or not? If it works, does it improve the performance?

About your custom code, assuming you're using a dev core, not a release, you could just add another overload to the SPI library, maybe something like:

void transfer(void *buf, size_t count, size_t datasize);

void arduino::ZephyrSPI::transfer(void *buf, size_t count, datasize) {
	int ret = transfer(buf, count, datasize==8 ? &config : &config16);
	(void)ret;
}

This should let you use our library to rule out any issues with your code. We could probably commit that, or later add data size to settings in the API.

[KurtE]

@KurtE Thanks for taking the time to test this. Could you please provide a brief summary? Does the change in this PR work or not? If it works, does it improve the performance?

Currently it hangs with my own code. Now if I call with only SPI.transfer call, it did not hang, With the version I have that runs, using the SPI.transfer(buffer, size); Where I use temporary buffer, copy in
the pixels, swapping the bytes, and output. If I run that WITHOUT the changes in this PR,
The times appear to be about 32853 versus with it 32842, so not much of an improvement in this case:
Maybe about .034%

May try some other simpler sketch to get more details

About your custom code, assuming you're using a dev core, not a release,
Yes currently for this test I used current source code build (as of sync yesterday) Only other change I had was my
defer camera startup code (different PR), that simply have the camera objects marked as init deferred, and then have
the camera startup code, only start the PWM clock IF the user uses camera library and then it calls the init of DCMI and then
INIT of the actual camera... So should not impact anything else.

But could do with released code as changes is only in SPI library.

you could just add another overload to the SPI library, maybe something like:

void transfer(void *buf, size_t count, size_t datasize);

void arduino::ZephyrSPI::transfer(void *buf, size_t count, datasize) {
	int ret = transfer(buf, count, datasize==8 ? &config : &config16);
	(void)ret;
}

This should let you use our library to rule out any issues with your code. We could probably commit that, or later add data size to settings in the API.

Could, personally I really really wish there was another API, that allows for write only and/or two buffers, write and read (can be NULL), that allows you to for example output directly out of a buffer without having it's contents overwritten.

Also still wonder about hardware. Fifo?

[iabdalkader]

If I run that WITHOUT the changes in this PR, The times appear to be about 32853 versus with it 32842, so not much of an improvement in this case: Maybe about .034%

@KurtE Thanks for confirming. This was expected as the problem, as you and others noted, seems to be with inter-byte gaps, not with whole transfers. This change might still be helpful if you're transferring a single byte at a time, though I'm not sure if it would make sense to hold CS in that case.

personally I really really wish there was another API, that allows for write only and/or two buffers, write and read (can be NULL), that allows you to for example output directly out of a buffer without having it's contents overwritten.

I agree, the API is lacking, to say the least, but we can try to improve it. We could start by adding a data size arg somewhere.

[KurtE]

@iabdalkader @mjs513 @facchinm @pillo79 - Wondering if we should continue this here, or back to our original
issue: #145 and/or create several new issues both here and/or Zephyr.

Note: I am mainly throwing darts here, and it has been a while since I really played with trying to improve the SPI speed.

There are probably several different things that should be looked at and steps to resolve them, including:

Are we properly configuring the SPI ports at the hardware level. I believe that there have been changes made in Zephyr over
the last several months, that allow some of the timing to be configured.

For those processors with FIFO on SPI, are they properly configured and software work with it?

I wish there was an API part of the SPI object that returns zephyr specific information, like handle to the underlying Zephyr SPI object. Which allows apps/libraries to easier directly call the zephyr methods.

Transfer APIs: allow you to set word size, write only transfers, maybe Async...

Fine tunning of SPI and drivers. At least earlier with these drivers, I was finding that for things like
setting up the rectangle to draw in the code has:

  void setAddr(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1)
      __attribute__((always_inline)) {
    //if ((x0 != _x0_last) || (x1 != _x1_last)) 
    {
      writecommand_cont(ST77XX_CASET); // Column addr set
      writedata16_cont(x0+_xstart);             // XSTART
      writedata16_cont(x1+_xstart);             // XEND
      _x0_last = x0;
      _x1_last = x1;
    }
    //if ((y0 != _y0_last) || (y1 != _y1_last)) 
    {
      writecommand_cont(ST77XX_RASET); // Row addr set
      writedata16_cont(y0+_ystart);             // YSTART
      writedata16_cont(y1+_ystart);             // YEND
      _y0_last = y0;
      _y1_last = y1;
    }
  }

Earlier I found that in the case here where we output <8 bit> <16 bit> <16 bit> <8 bit> <16 bit> <16 bit><8 bit>
items, that there is an overhead of switching the underlying SPI from 8 bit mode to 16 bit mode and then switching
back. And at the time, it was faster to output the two 16 bit values as 4 8 bit values... But if you are going to output
several 16 bit values, then it is faster to switch. (How many outputs does it take to tip it? Don't know.

[KurtE]

@facchinm @iabdalkader @pillo79 @mjs513 - Quick update
Posted more information up on forum:
https://forum.arduino.cc/t/experiments-trying-to-speed-up-spi-on-q-and-zephyr/1419817

Verified hang on simple sketch:

#include <SPI.h>

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  while (!Serial && millis() < 5000) {}
  SPI.begin();
  Serial.println("after SPI begin");
  SPI.beginTransaction(SPISettings(400000, MSBFIRST, SPI_MODE0));
  Serial.println("After begin transaction");
  SPI.transfer(0x2A);
  Serial.println(0x2a);
  SPI.transfer16(0);
  Serial.println(0);
  SPI.transfer16(480);
  Serial.println(480);
  SPI.transfer(0x2B);
  Serial.println(0x2b);
  SPI.transfer16(0);
  Serial.println(0);
  SPI.transfer16(320);
  Serial.println(320);
  Serial.println("Got to end of setup");
}

void loop() {
  // put your main code here, to run repeatedly:

}

Only output to serial:

uart:~$ after SPI begin
After begin transaction
42

So it hangs or dies going from Transfer to Transfer16

[KurtE]

@facchinm @pillo79 - Have you tried out this updated fix? Did it properly switch between 8 bit and 16 bit mode?

The reason I ask, was I was doing something earlier, but it was not switching modes at the lower levels. That
is the code compared the pointer to the new config with the current config and if they were the same, it did bailed
from computing the actual SPI registers. Don't remember if it was at the driver level or the HAL level.

Will try a simple check at least on one board with Logic Analyzer

EDIT:
Looks like it does change now:

#include <SPI.h>

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  while (!Serial && millis() < 5000) {}
  SPI.begin();
  Serial.println("after SPI begin");
  SPI.beginTransaction(SPISettings(400000, MSBFIRST, SPI_MODE0));
  Serial.println("After begin transaction");
  SPI.transfer(0x2A);
  Serial.println(0x2a);
  SPI.transfer16(0);
  Serial.println(0);
  SPI.transfer16(480);
  Serial.println(480);
  SPI.transfer(0x2B);
  Serial.println(0x2b);
  SPI.transfer16(0);
  Serial.println(0);
  SPI.transfer16(320);
  Serial.println(320);
  Serial.println("Got to end of setup");
}

void loop() {
  // put your main code here, to run repeatedly:

}

[iabdalkader]

Suggested change

	config.operation = mode | flags;
	config.operation = (config.operation & ~SPI_WORD_SIZE_MASK) | flags;

[iabdalkader]

This doesn't really fix the issue as discussed. In any case, it should Never be set unconditionally, what if causes an issue with some random SPI device? This should be added to the API and passed in settings.

[pennam]

It does not removes the holes, it forces to keep the data and clock line controlled until endTransaction is called.

Releasing data and clock lines causes issues with some SPI sensors libraries.

SPI.transfer(0x2A);
SPI.transfer16(0x010F);

Without SPI_HOLD_ON_CS

With SPI_HOLD_ON_CS

[pennam]

In my opinion SPI_HOLD_ON_CS should be the default way of working, and this is how SPI works with other Arduino cores.

[iabdalkader]

I forgot about endTransaction. Okay then. It's weird though that I don't see CS toggling in the first capture, maybe not enough delay? Also, seems the 16bit transfer is swapped.

[pennam]

That is not an HW CS is a GPIO. Yes it looks swapped, I've used MSBFIRST flag; let me doublecheck.

[pennam]

I've used MSBFIRST flag; let me doublecheck.

just ignore this, it changes the bit order so it is unrelated.

[iabdalkader]

That is not an HW CS is a GPIO.

I know. The GPIO CS delay is also controllable.

Yes it looks swapped

If it matches mbed core, an others, then it's probably fine, I guess.

[iabdalkader]

See _spi_context_cs_control in spi_context.h.

[pennam]

If it matches mbed core

It does not, this is what i get using the mbed core

[mjs513]

Not sure this SPI issue is pertinent here but looking at your traces it looks like what I was seeing when I was trying to get a Arducam SPI camera working with the Arduino Q: https://forum.arduino.cc/t/spi-strange-behavior-using-begin-end-transaction/1420028

When using SPI.beginTransaction and SPI.endtranasction for transerring data in a bus_read and bus_write function the SPI traces looked more like it was operating in Mode2. (ref: https://www.analog.com/en/resources/analog-dialogue/articles/introduction-to-spi-interface.html),

Cant see the clock in the traces above.

The only way I was able to retain Mode0 was to call it once in setup and tell the library not to use transactions for the transfers.

EDIT: Note tried this along time ago on the R4 using the mbed core with no issue

[KurtE]

@iabdalkader @facchinm @pillo79 @mjs513 - Not sure if many of you have looked at my Forum thread:
https://forum.arduino.cc/t/experiments-trying-to-speed-up-spi-on-q-and-zephyr/1419817/2

But I have code that does fillScreens (or fillRect of whole screen) of an ST7796 display using SPI, and trying several different ways to do it.

Without these changes timings were: (UNO Q) - comments on what each line is:

TFT Library: 4195 66 - My own code, that grabbed handle to zephyr SPI object and called spi_transceive directly
fillRect_async: 3669 335 - Ditto except calls spi_transceive_cb instead the callback simply sets flag which I loop waiting for.
fillRect_SPI_transfer: 18770 336 - uses two calls to SPI.transfer per pixel
fillRect_SPI_transfer16: 10245 184 - uses one call to SPI.transfer per pixel
fillRect_SPI_transfer_buf: 5648 102 - sets up a buffer of 480 pixels and calls SPI.transfer(buffer, cnt).
fillRect_SPI_transfer_txbuf: 5603 32 - Extend SPI library for SPI.transfer16(buffer, rxbuffer, cnt);

Note: 2nd number per line was one that i write out an image 120x160 to the display a few different ways. Note: I have
not updated the Async one yet to use the transceive_cb which should be faster that the first line... Also I tried
to output a larger version of the image 240x320 - But sketch will not either link or load depending on static versus dynamic build.

More details up on the thread.

With todays SPI changes I see (two iterations)

TFT Library: 4194 66
fillRect_async: 3669 327
fillRect_SPI_transfer: 18286 327
fillRect_SPI_transfer16: 9252 166
fillRect_SPI_transfer_buf: 5648 102
fillRect_SPI_transfer_txbuf: 5553 16

TFT Library: 4195 65
fillRect_async: 3669 327
fillRect_SPI_transfer: 18287 327
fillRect_SPI_transfer16: 9251 167
fillRect_SPI_transfer_buf: 5647 102
fillRect_SPI_transfer_txbuf: 5553 16

Maybe slight improvements here in some cases, others, more or less not.

My guess is that we may need to look more at the underlying zephyr setup to speed things up.

Also changing of the API... You probably need to do it at the HardwareSPI (API) level, as probably most
libraries and Apps are setup for that and not for arduino::ZephyrSPI.

Also CS pins - I believe most if not all implementations at the Arduino layer assume that the CS pins are under software
control and not Hardware controlling the CS pin. Could be wrong.

[mjs513]

@KurtE

Also CS pins - I believe most if not all implementations at the Arduino layer assume that the CS pins are under software control and not Hardware controlling the CS pin. Could be wrong.

This is going to be important if you want to you use multiple SPI devices on the same bus.

[pennam]

@KurtE @mjs513 the goal of this PR is not to make SPI faster, but to make it compatible with other sensors libraries that are not working with current implementation.

I will drop 95b2b95 because is not working as expected.

[pennam]

Test UNO Q BMP388 👍

SPI.transfer(0x2A);
SPI.transfer16(0x010F);

[pennam]

Test GIGA R1 BMP388 👍

SPI.transfer(0x2A);
SPI.transfer16(0x010F);

[KurtE]

@KurtE @mjs513 the goal of this PR is not to make SPI faster, but to make it compatible with other sensors libraries that are not working with current implementation.

Sorry, I misunderstood. I thought that this PR was in response to my Issue, that there are large gaps in time between outputs and trying to reduce that and improve on the throughput.

I understand that there are some issues with SPI on some devices, where for example the clock signal may end stay in a strange state at the end of a transfer, which confuses some devices. Which I personally have not tried to diagnose, but my gut was
wondering if it had something to do with the SPI.endTransaction calling spi_release(spi_dev, &config);
I wondered what that actually does, through the Zephyr layer and then the HAL.

I know on some other SPI implementations, such as on a Teensy, the endTransaction may not do much in most cases.
In debug mode it would use a counter to see if there were matching calls to beginTransaction. And in some cases there
was code that the beginTransaction, might disable some interrupts and the endTransaction reenables them.

What I meant to do at some point was to potentially print out all of the SPI registers before and after the call to see
if it changed any of the Hardware settings. Like maybe the CPOL or CPHA... Like maybe there is a default defined for
the hardware SPI object, and the release maybe restores it back to that. But again just throwing darts.

Anyway maybe this PR should be renamed?

[mjs513]

@pennam
My issue is not even with speeding SPI up but just getting SPI working with Arducam on SPI.

With that said I just tested a BME280 with the Adafruit BME280 library and yest its reading transfers however its wrong:

BME280 test
Could not find a valid BME280 sensor, check wiring, address, sensor ID!
SensorID was: 0xC0
        ID of 0xFF probably means a bad address, a BMP 180 or BMP 085
   ID of 0x56-0x58 represents a BMP 280,
        ID of 0x60 represents a BME 280.
        ID of 0x61 represents a BME 680.

with a LA trace of

Note had to set CPOL = 1 otherwise I get settings mismatches.

EDIT: when you tested the BMP388 what library did you use and are you getting data from the sensor

[mjs513]

Sorry, I misunderstood. I thought that this PR was in response to my Issue, that there are large gaps in time between outputs and trying to reduce that and improve on the throughput.

Ditto

[pennam]

@KurtE @mjs513 my fault, I forgot you issue is referenced in this PR description. The whole story about this, between me @facchinm and @pillo79, started because we were not able to make SPI work with BMP sensor. I will remove the reference and rename the PR.

@mjs513 I've used a BMP388 sensor with Adafruit BMP3XX Library and yes I'm getting data from the sensor.