SPIClass::_spi::Init is uninitialised

[chrissbarr]

Describe the bug SPIClass's internal spi_t instance is not initialised with default values. This can cause problems in particular with the fields in the SPI_InitTypeDef struct, which can currently take invalid values which cause the SPI peripheral to be misconfigured in ways that currently fail silently.

I think this is only likely to be a problem if there are any dynamic memory allocations in the program before the SPIClass instance is constructed. In the case where SPIClass is constructed statically, the memory it points at should already be zeroed. So, this is probably not an issue in the majority of cases. However, it tripped me up for quite a while, and it seems easy to correct (or at least make the failure mode more obvious).

The failure mode in my case was the SPI initialisation all appeared to work, but SPIClass::transfer would hang indefinitely.

To Reproduce This is a simple Arduino sketch that can be used to examine the behaviour:

#include <SPI.h>

SPIClass* mySPI;

void setup() {
  // fill & clear some memory dynamically before creating SPIClass
  const int arraySize = 1024;
  int* dummyArray = (int*) calloc(arraySize, sizeof(int));
  for (int i = 0; i < arraySize; i++) {
    dummyArray[i] = 0xAAAAAAAA;
  }
  free(dummyArray);

  // Create SPIClass and initialise
  mySPI = new SPIClass();
  mySPI->begin();
}

void loop() {}

Steps to reproduce the behavior:

1. Build above sketch (debug, no optimisations, etc.)
2. Connect with debugger and step into the mySPI->begin() call, then the nested call to spi_com.c::spi_init().
3. Observe that the obj->handle->Init fields are all uninitialised and therefore have the values of whatever previously resided in memory (in the example's case, 0xAAAAAAAA):

5. The spi_com.c::spi_init() function configures some of the Init fields, and by the time it calls HAL_SPI_Init() half of the fields have correct/reasonable values:

However, the remaining incorrect values propagate through the SPI initialisation and can cause the SPI to operate incorrectly or fail to start.

Expected behavior As there is no explicit initialisation of the SPI_InitTypeDef struct, it looks like the code works correctly on the assumption that all fields should start as zero, and only fields that might change from zero need to be initialised explicitly, within spi_com.c::spi_init(). If this is the working assumption, it seems like it might be reasonable to explicitly zero-initialise the struct. User-code can do this as follows:

mySPI= new SPIClass();
mySPI->getHandle()->Init = SPI_InitTypeDef{}; // initialise the Init struct explicitly

It might be reasonable to do this in the SPIClass's constructor (I guess via SPIClass::begin is also possible, but then if the user has manually set any parameters via SPIClass::getHandle() they will be overwritten).

I notice that there are two safeguards in the existing code to catch problems with the Init fields, which could potentially be improved.

The first is that HAL_SPI_Init() does do some sanity checks on the input parameters:

  /* Check the parameters */
  assert_param(IS_SPI_ALL_INSTANCE(hspi->Instance));
  assert_param(IS_SPI_MODE(hspi->Init.Mode));
  assert_param(IS_SPI_DIRECTION(hspi->Init.Direction));
  assert_param(IS_SPI_DATASIZE(hspi->Init.DataSize));
  assert_param(IS_SPI_FIFOTHRESHOLD(hspi->Init.FifoThreshold));
  assert_param(IS_SPI_NSS(hspi->Init.NSS));
  assert_param(IS_SPI_NSSP(hspi->Init.NSSPMode));
  assert_param(IS_SPI_BAUDRATE_PRESCALER(hspi->Init.BaudRatePrescaler));
  assert_param(IS_SPI_FIRST_BIT(hspi->Init.FirstBit));
  assert_param(IS_SPI_TIMODE(hspi->Init.TIMode));

I found this called out my problem once I enabled the USE_FULL_ASSERT macro. It doesn't cover every field, but seems like it catches some of the potential problems. Not sure if there is a way to improve this. Anyway, this is in the STM32 HAL, so not something to be improved here.

The second, which I think can be improved, is that HAL_SPI_Init() is reasonably likely to fail to initialise the SPI peripheral and return an error code if the Init struct has invalid values. This function returns HAL_ERROR for many different failure conditions (a few of which I triggered with my invalid Init fields).

However, at the moment, spi_com.c::spi_init() doesn't check the return code of HAL_SPI_Init(), and continues to try and use the SPI peripheral even if it has failed configuration. So currently it appears to pass initialisation, and only fails (or misbehaves etc.) when later code tries to use the SPI peripheral.

Perhaps it would be possible to check the return code of HAL_SPI_Init() and invoke an error handler if it fails? That way it wouldn't be failing silently, and would be easier to track down where/when the problem is.

// current
HAL_SPI_Init(handle);

// suggested
if (HAL_SPI_Init(handle) != HAL_OK)
{
    Error_Handler();
}

Desktop (please complete the following information):

• OS: Windows
• Arduino IDE version: 2.3.3
• STM32 core version: 2.8.1
• Tools menu settings if not the default: Settings for debug - Og, Optimize for debugging, etc
• Upload method: SWD

Board (please complete the following information):

• Name: STM32H723ZGT6 Dev Board (generic/multiple)
• Hardware Revision: N/A

Happy to raise a PR for the default initialisation and/or HAL_ERROR check if you think it would be welcome.

[fpistm]

Hi @chrissbarr Feel free to submit a PR. Any contribution are welcome.

[fpistm]

Hi @chrissbarr looking at this issue I thought about the same issue with Wire (#2196) then I've made a PR to fix it the same way, all fields init to 0.