SPI.transfer16 - does not properly handle word size.

[KurtE]

Describe the bug
SPI.transfer16(0x2ff) - with MSBFIRST defined in the transaction, outputs the bytes in LSB, MSB order

The issue, is that the code, does not update the word size from 8 to 16 in the config.operation field.

Optional: attach the sketch

#include <SPI.h>
void setup() {
    // put your setup code here, to run once:
    Serial.begin(115200);
    SPI.begin();
    SPI.beginTransaction(SPISettings(30000000, MSBFIRST, SPI_MODE0));
    pinMode(53, OUTPUT);
    digitalWrite(53, HIGH);
}

uint16_t loop_count = 0;

void loop() {
    loop_count++;
    digitalWrite(53, LOW);
    SPI.transfer(loop_count >> 8);
    SPI.transfer(loop_count & 0xff);

    delayMicroseconds(5);
    SPI.transfer16(loop_count);
}

Additional context
I thought I could fix this, by simply editing the Transfer16 method to:

uint16_t arduino::ZephyrSPI::transfer16(uint16_t data) {
  int ret;
  uint16_t rx;
  const struct spi_buf tx_buf = {.buf = &data, .len = sizeof(data)};
  const struct spi_buf_set tx_buf_set = {
      .buffers = &tx_buf,
      .count = 1,
  };
  const struct spi_buf rx_buf = {.buf = &rx, .len = sizeof(rx)};
  const struct spi_buf_set rx_buf_set = {
      .buffers = &rx_buf,
      .count = 1,
  };

  spi_operation_t operation_save = config.operation;
  config.operation = (operation_save &= ~(SPI_WORD_SET(0x3f))) | SPI_WORD_SET(16);

  ret = spi_transceive(spi_dev, &config, &tx_buf_set, &rx_buf_set);
  config.operation = operation_save;
  if (ret < 0) {
    return 0;
  }

  return rx;
}

But running it faulted:

uart:~$ sketch
[00:00:12.771,000] <err> os: ***** USAGE FAULT *****
[00:00:12.777,000] <err> os:   Attempt to execute undefined instruction
[00:00:12.784,000] <err> os: r0/a1:  0x00000000  r1/a2:  0x00000000  r2/a3:  0x240008b0
[00:00:12.793,000] <err> os: r3/a4:  0x240008b0 r12/ip:  0x0805b2a0 r14/lr:  0x0804e8f9
[00:00:12.801,000] <err> os:  xpsr:  0x61000000
[00:00:12.807,000] <err> os: s[ 0]:  0x2400eb6d  s[ 1]:  0x2400eba3  s[ 2]:  0x2400f05d  s[ 3]:  0x2400d714
[00:00:12.817,000] <err> os: s[ 4]:  0x2400eb89  s[ 5]:  0x08048dfd  s[ 6]:  0x2400d714  s[ 7]:  0x2400eb89
[00:00:12.827,000] <err> os: s[ 8]:  0x00000000  s[ 9]:  0x00000000  s[10]:  0x00000000  s[11]:  0x00000000
[00:00:12.838,000] <err> os: s[12]:  0x00003a48  s[13]:  0x08042adf  s[14]:  0xaaaaaaaa  s[15]:  0xaaaaaaaa
[00:00:12.848,000] <err> os: fpscr:  0xaaaaaaaa
[00:00:12.853,000] <err> os: Faulting instruction address (r15/pc): 0x2400e750
[00:00:12.861,000] <err> os: >>> ZEPHYR FATAL ERROR 36: Unknown error on CPU 0
[00:00:12.869,000] <err> os: Current thread: 0x240008b0 (shell_uart)
[00:00:12.876,000] <err> os: Halting system

I probably missed something, but thought I would at least first mention this as an issue.

[KurtE]

Edit to the above: this line:
config.operation = (operation_save &= ~(SPI_WORD_SET(0x3f))) | SPI_WORD_SET(16);

Should be:
config.operation = (operation_save & ~(SPI_WORD_SET(0x3f))) | SPI_WORD_SET(16);

And it then properly changed it from 0x100 to 0x200.

However it did not change the output at all. Thought maybe MSBFIRST -> SPI_TRANSFER_MSB
Maybe issue with MSBFIRST is 1 and LSBFIRST is 0, and SPI_TRANSFER_MSB is 0...
So I hacked setting bit 5 to see if it made a difference and no, as far as the Logic Analyzer output.

[KurtE]

@facchinm - I figured out what the issue is, but not necessarily what the best solution for it would be. I have a version, which at least shows how it can potentially work...

The issue is, with the use of the config object.
The spi_transceive function calls through to the function transceive within the file spi_ii_stm32.c. Which then passes the
config off to the function: spi_stm32_configure, which then calls off to:

	if (spi_context_configured(&data->ctx, config)) {
		/* Nothing to do */
		return 0;
	}

Which in spi_context.h simply does:

static inline bool spi_context_configured(struct spi_context *ctx,
					  const struct spi_config *config)
{
	return !!(ctx->config == config);
}

Which at the end of the spi_transceive function it assigns the ctx->config to the value passed in. Or in other words,
if you pass in the same context object,

My first quick and dirty hack version:

uint16_t arduino::ZephyrSPI::transfer16(uint16_t data) {
  int ret;
  uint16_t rx;
  const struct spi_buf tx_buf = {.buf = &data, .len = sizeof(data)};
  const struct spi_buf_set tx_buf_set = {
      .buffers = &tx_buf,
      .count = 1,
  };
  const struct spi_buf rx_buf = {.buf = &rx, .len = sizeof(rx)};
  const struct spi_buf_set rx_buf_set = {
      .buffers = &rx_buf,
      .count = 1,
  };

  struct spi_config config16;
  memcpy((void*)&config16, (void*)&config, sizeof(config));
  config16.operation = (config.operation & ~(SPI_WORD_SET(0x3f))) | SPI_WORD_SET(16);

  static uint8_t debug_count = 2;
  static SPI_TypeDef *DBGSPIx;
  if (debug_count) {
    Serial.print(config.operation, HEX);
    Serial.print("->");
    Serial.println(config16.operation, HEX);
    if (this == &SPI1) DBGSPIx = (SPI_TypeDef *)0x40015000ul;
    else  DBGSPIx = (SPI_TypeDef *)0x40013000ul;
    Serial.print("B:"); Serial.print((uint32_t)DBGSPIx, HEX);
    Serial.print(" "); Serial.print(DBGSPIx->CFG1, HEX);
    Serial.print(" "); Serial.print(DBGSPIx->CFG2, HEX);
    Serial.print(" "); Serial.print(DBGSPIx->CR1, HEX);
    Serial.print(" "); Serial.println(DBGSPIx->CR2, HEX);
    delay(50);
  }


  ret = spi_transceive(spi_dev, &config16, &tx_buf_set, &rx_buf_set);
  if (debug_count) {
    debug_count--;
    Serial.print("A:         "); Serial.print(DBGSPIx->CFG1, HEX);
    Serial.print(" "); Serial.print(DBGSPIx->CFG2, HEX);
    Serial.print(" "); Serial.print(DBGSPIx->CR1, HEX);
    Serial.print(" "); Serial.println(DBGSPIx->CR2, HEX);
    delay(50);
  }
  if (ret < 0) {
    return 0;
  }

  return rx;
}

Which is mostly debug code, currently puts a spi_config structure on the stack, copies the current one to it and
updates the operation from 8 bits to 16 bits. And the Logic Analyzer output shows it updated:

Like I manually did for the two 8 bit outputs:

void loop() {
    loop_count++;
    digitalWrite(53, LOW);
    SPI.transfer(loop_count >> 8);
    SPI.transfer(loop_count & 0xff);
    SPI.transfer16(loop_count);
    digitalWrite(53, HIGH);
    delayMicroseconds(5);
}

I also want to confirm that the registers were properly updated (debug code above):
Output:

S:40013000 70007 0 0 0
100->200
B:40013000 20070007 20400000 0 0
A:         2007000F 20400000 0 0
100->200
B:40013000 20070007 20400000 0 0
A:         2007000F 20400000 0 0

So it did update the CFG1 setting, DSIZE, the other 7 checked was the CRCSIZE, and my guess is we don't use this.

How to Properly fix?
a) Change how the testing is done for change... More complex, probalby need to keep copy of last config, not
pointer...

b) Which I will play with:
Change SPI object, to have two config objects, config and config16, both of which are setup within beginTransaction and probably should be done as part of begin() as well... And then transfer16 simply uses the config16...

Thoughts?

[mjs513]

b) Which I will play with:
Change SPI object, to have two config objects, config and config16, both of which are setup within beginTransaction and probably should be done as part of begin() as well... And then transfer16 simply uses the config16...

This approach seems to be more logical, at least to me. Also I believe in the KISS principle

[KurtE]

@mjs513 Thanks,
@facchinm,

I have an implementation that appears to be working using b)
That I would check in, but I am currently working off of your PR branch, so not sure
best way to do so...
It is here:
I added the config16, initialized in beginTransaction, and then transfer16 uses it.
I also updated begin() to call beginTransaction(SPISettings()); endTransaction();
so that if anyone does any transfers without calling beginTranaction it should output using the defaults.
SPI.zip

Both appeared to work using test sketch:

#include <SPI.h>
void setup() {
    // put your setup code here, to run once:
    Serial.begin(115200);
    SPI.begin();

    static SPI_TypeDef *DBGSPIx;
    DBGSPIx = (SPI_TypeDef *)0x40013000ul;
    Serial.print("S:");
    Serial.print((uint32_t)DBGSPIx, HEX);
    Serial.print(" ");
    Serial.print(DBGSPIx->CFG1, HEX);
    Serial.print(" ");
    Serial.print(DBGSPIx->CFG2, HEX);
    Serial.print(" ");
    Serial.print(DBGSPIx->CR1, HEX);
    Serial.print(" ");
    Serial.println(DBGSPIx->CR2, HEX);
    delay(50);

    // Check before we do begin transaction.
    pinMode(53, OUTPUT);
    digitalWrite(53, HIGH);
    digitalWrite(53, LOW);
    for (uint8_t i = 0; i < 10; i++) SPI.transfer(i);
    digitalWrite(53, HIGH);
    delay(5);
    digitalWrite(53, LOW);
    for (uint16_t i = 0; i < 64 * 10; i += 64) SPI.transfer16(i);
    digitalWrite(53, HIGH);


    SPI.beginTransaction(SPISettings(30000000, MSBFIRST, SPI_MODE0));
}

uint16_t loop_count = 0;

void loop() {
    loop_count++;
    digitalWrite(53, LOW);
    SPI.transfer(loop_count >> 8);
    SPI.transfer(loop_count & 0xff);
    SPI.transfer16(loop_count);
    digitalWrite(53, HIGH);
    delayMicroseconds(5);
}

[facchinm]

@KurtE good catch! I almost never use transfer16() so that code patch went untested.
Would you mind submitting a PR?