SPI::setClockDivider implementation is incorrect

[tve]

The implementation of SPIClass::setClockDivider does not do what it should. For compatibility/history's sake it should interpret the SPI_CLOCK_DIVn relative the original 16Mhz Arduino clock. Instead, it applies the divider to the current SPI device clock.
Specifically:

      spiSettings[idx].clk = spiClkFreq/_divider;

at https://github.com/stm32duino/Arduino_Core_STM32/blob/master/libraries/SPI/src/SPI.cpp#L255 should be replaced by something like

      spiSettings[idx].clk = 16000000/_divider;

Of course other ways to implement the same notion are also possible. If you want to look at what one of the official ARM cores does, see https://github.com/arduino/ArduinoCore-sam/blob/master/libraries/SPI/src/SPI.h#L138-L146 and https://github.com/arduino/ArduinoCore-sam/blob/master/libraries/SPI/src/SPI.cpp#L167-L173, i.e., they define the divider values such that it comes out right, which probably works because they fix the processor's clock speed.

[fpistm]

SPIClass::setClockDivider is a deprecated function.
Anyway if we would respect the comment, it should be fine to adapt the code to get the right "Arduinoish" freq based on real SPI clk freq.
Because using spiSettings[idx].clk = 16000000/_divider; will not provide the expected freq.

[tve]

Because using spiSettings[idx].clk = 16000000/_divider; will not provide the expected freq.

Please explain. Isn't spiSettings[idx].clk in Hz? And thus if I select a DIVn divider in legacy arduino form doesn't that mean 16Mhz divded by n?

[fpistm]

Oh yes, you're right. Probably, too much task in // :)
This is the requested freq.
So well computed in spi_init.
So the right question here is:
As this function is deprecated, would it be better to limit the requested speed to those value for better compatibility with I guess older library, using hard coded value 16000000:

  *         SPI_CLOCK_DIV2    (8MHz)
  *         SPI_CLOCK_DIV4    (4MHz)
  *         SPI_CLOCK_DIV8    (2MHz)
  *         SPI_CLOCK_DIV16   (1MHz)
  *         SPI_CLOCK_DIV32   (500kHz)
  *         SPI_CLOCK_DIV64   (250kHz)
  *         SPI_CLOCK_DIV128  (125kHz)

or allow use the real input clock of SPI.

  *         SPI_CLOCK_DIV2    (spiClkFreq/2 Hz)
  *         SPI_CLOCK_DIV4    (spiClkFreq/4 Hz)
  *         SPI_CLOCK_DIV8    (spiClkFreq/8 Hz)
  *         SPI_CLOCK_DIV16   (spiClkFreq/16 Hz)
  *         SPI_CLOCK_DIV32   (spiClkFreq/32 Hz)
  *         SPI_CLOCK_DIV64   (spiClkFreq/64 Hz)
  *         SPI_CLOCK_DIV128  (spiClkFreq/128 Hz)

I would guess the first option (as you suggested) is the best.

[tve]

I would agree with you, however, I did notice that in the rogerclark core the second interpretation (spiClkFreq/N) is used. So you're between a rock and a hard place ;-)

[fpistm]

Hi @tve,
Some update:
As said in Arduino API:
https://www.arduino.cc/en/Reference/SPISetClockDivider

On the Due, the system clock can be divided by values from 1 to 255. The default value is 21, which sets the clock to 4 MHz like other Arduino boards.

So in fact, it should be possible to divide the clock by any value.
Then HAL will use the best prescaler to fit requested Freq.

Moreover, it is not possible to define SPI_CLOCK_DIVx to match the 16MHz value for avr as SPI frequency depends of system clock configuration.

I will update also the wrong comment to avoid any confusion.

[fpistm]

Hi @tve,
any comment on #379 ?
Thanks in advance