STM32: SPI.readinto ignores write_value, causes SD cards not to detect

[jerryneedell]

related to
adafruit/Adafruit_CircuitPython_SD#36

I know the "on-board" SDcard is not support on the feather STM32F405.
I tried attaching an Adalloger Featherwing using sdcardio and it is not able to detect the card
The also happens with the Adafruit_CircuitPython_SD library
see
adafruit/Adafruit_CircuitPython_SD#36

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 6.0.0-alpha.1-111-g8f928340c on 2020-07-20; Feather STM32F405 Express with STM32F405RG
>>> import sdcardio_lib
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sdcardio_lib.py", line 9, in <module>
OSError: no SD card
>>> 

code -- sdcardio_lib.py

import busio
import sdcardio
import board
import storage
import sys
import os

# Connect to the card and mount the filesystem.
sdcard = sdcardio.SDCard(board.SPI(), board.D10)
vfs = storage.VfsFat(sdcard)
storage.mount(vfs, "/sd")
sys.path.append("/sd")
sys.path.append("/sd/lib")

[jepler]

Good session, nRF52840 (particle xenon):

Bad session, STM32F405 feather

Notice how outside the "40 00 00 00 95" packet sent to initialize the SD card into SPI mode, the MOSI pin stays high on nRF but doesn't on STM. I believe this is because the write_value (a static 8 bit value to be output while the read is ongoing) is ignored in the stm port:

bool common_hal_busio_spi_read(busio_spi_obj_t *self,
        uint8_t *data, size_t len, uint8_t write_value) {
    if (self->miso == NULL) {
        mp_raise_ValueError(translate("No MISO Pin"));
    }
    HAL_StatusTypeDef result = HAL_SPI_Receive (&self->handle, data, (uint16_t)len, HAL_MAX_DELAY);
    return result == HAL_OK;
}

I suspect the underlying cause is the same here and in adafruit/Adafruit_CircuitPython_SD#36 and will be fixed by the same fix -- whatever that is.

[jepler]

I've taken the liberty of punting this to @hierophect but can take it back if that's preferred.

[hierophect]

I can check it out, I'll add it to my list.

[jepler]

Changing the readinto code like this works, but may not be acceptable (it puts a potentially large variable size array on the stack):

 bool common_hal_busio_spi_read(busio_spi_obj_t *self,
-        uint8_t *data, size_t len, uint8_t write_value) {
+        uint8_t *data_in, size_t len, uint8_t write_value) {
     if (self->miso == NULL) {
         mp_raise_ValueError(translate("No MISO Pin"));
     }
-    HAL_StatusTypeDef result = HAL_SPI_Receive (&self->handle, data, (uint16_t)len, HAL_MAX_DELAY);
+    uint8_t data_out[len];
+    memset(data_out, write_value, len);
+    HAL_StatusTypeDef result = HAL_SPI_TransmitReceive (&self->handle,
+        data_out, data_in, (uint16_t)len,HAL_MAX_DELAY);
     return result == HAL_OK;
 }

[hierophect]

@jepler I'm fixing this today. Do you think you could provide me with a test sketch I can use to verify the fix? Something with a clear differentiation between the desired/undesired behavior. I can use my logic analyzer to view the same waveforms you did if you give me that same sketch. If it's just the same one jerryn provided let me know.

[hierophect]

I'd like to understand the significance of write_value a little better - most of the ports ignore it (including NRF52, which was in your example), and the ones that don't pass it directly into their SPI HALs. Is there something that STM32 does here that's nonstandard?

[hierophect]

Looks like this is a limitation of the ST HAL in regards to handling the MOSI line when using an SD card.
https://community.st.com/s/question/0D50X00009XkdpZ/sd-card-using-spi-initialization-stm32f303-issue

I'll look around for other workarounds since this one seems kind of slow. Micropython, STM32Duino, and even the ST FatFS library must all deal with this issue somehow.

[jepler]

While SPI just needs the value 0xff (all bits same), the nRF24 library needs specific values:

    def _reg_read(self, reg):
        buf = bytearray(2)  # 2 = 1 status byte + 1 byte of returned content
        with self._spi as spi:
            time.sleep(0.005)  # time for CSN to settle
            spi.readinto(buf, write_value=reg)
        self._status = buf[0]  # save status byte
        return buf[1]  # drop status byte and return the rest

[2bndy5]

@jepler @hierophect to be clear, the nRF24L01 hardware actually ignores MOSI after the first byte. I wrote that code when I was just starting out with CircuitPython in an effort to better familiarize myself with SPI transactions. Using spi.write_readinto() works just as well in regards to the nRF24L01 transceiver.

EDIT: Oops, I didn't mean to steal credit from Damien George, Peter Hinch, or Rhys Thomas. Turns out the use of spi.readinto() is from the original micropython source that was re-written for circuitpython.

[hierophect]

@jepler @tannewt Report from looking at this this morning. Delay obviously changes a lot based on the size of the incoming data. Measuring the duration of the memset with the DWT at 168MHz, these are the timings I got:

• 3 byte flash setup read: 886ns delay vs 91µs read time (~1%)
• 512 byte flash read: 73.6µs delay vs 3.2ms (~2.3%)

This is of course not variable with the speed of the SPI line, so it'll be a more significant delay compared to the actual transaction the faster the SPI is going. Is this a big enough deal for us to put in the API modification that Jeff suggested to allow write_value to be set as an "invalid" token, so that the memset can be skipped?

[hierophect]

Actual DWT Values:

• Flash init of length 3:
  • Start = 3, memset = 152, transaction = 15426
  • Percent: (152-3)/(15426-152) = 0.97%, ~1%
• Flash read of length 512
  • Start = 3, memset = 12368, end = 543700
  • Percent: (12368 - 3)/(543700 - 12368) = 2.32%