adafruit / Adafruit_CircuitPython_SD

SD card drivers for Adafruit CircuitPython
MIT License
37 stars 17 forks source link

Fix for occasional write hangs #54

Closed RetiredWizard closed 1 year ago

RetiredWizard commented 1 year ago

I believe this fixes #50 the write hangs were happening because the SD card wasn't ready for the end block write command. This adds a _wait_for_ready call before the command is sent.

I'm not sure if the _wait_for_ready function is the right one for the desired delay but my test programs all work at any records length once this change is made.

RetiredWizard commented 1 year ago

The write hangs I was experiencing with long records were occurring on a Raspberry Pi Pico, the problem doesn't seem to show up on my UM Feather S3 although adding this fix doesn't seem to cause an issue on the Feather S3 either.

RetiredWizard commented 1 year ago

I was able to recreate the write hang on the Feather S3 by writing 10 very long records (500,000 bytes).

Using the library with the added call to _wait_for_ready I was able to successfully write the 10 500,000 byte records, and without the change the test program hung after writing the 4th record.

dhalbert commented 1 year ago

@jepler I added you as a reviewer because I thought you had a better idea of the protocol than most of us. If you want to decline, that is fine.

RetiredWizard commented 1 year ago

In case this helps with the review, I think this represents the delays. :)

sdblockwrite

jepler commented 1 year ago

I don't have enough of SD card protocol in my head to say, but if it's right we need this same change in the core sdcardio and maybe sdioio if applicable.

RetiredWizard commented 1 year ago

I took a quick look at sdcardio and the block writes are structured a little different. The writeblocks module doesn't seem to send the stop token. The common_hal_sdcardio_sdcard_writeblocks module calls extraclock_and_unlock_bus which might??? be doing the stop token function but I haven't stared at extraclock_and_unlock_bus long enough to understand what it's doing. Maybe it's also performing the wait for ready?????

RetiredWizard commented 1 year ago

I loaded my test script up on a Sparkfun Thing Plus RP2040 and modified it to use sdcardio rather than the adafruit_sdcard library.

I was unable to reproduce the issue under sdcardio. The memory on the RP2040 would only allow me to create records that were 50,000 bytes long but I ran the test with 1000 records of 50,000 each and didn't have any faults.

The only other board I have with an SD card slot is the Teensy 4.1 but I wasn't able to get sdcardio to work with the bitbangio SPI device that is required on the Teensy.

RetiredWizard commented 1 year ago

I thought sdcardio would only be built in for boards that had dedicated SD card slots, but apparently it's available on all (most?) boards?. I loaded the sdcardio version of my test script on the original Pi Pico and Feather S3 and neither board shows the faults even with records lengths at the limit of the boards memory capacity.

If sdcardio is available on all boards, what is the reason for the adafruit_sdcard library? It turns out it's necessary for the teensy 4.1 since sdcardio can't handle the bitbangio SPI but that feels more like an issue that could be fixed with sdcardio.

dhalbert commented 1 year ago

sdcardio is not available on some boards with limited flash size, such as SAMD21 boards, only because it doesn't fit.

jepler commented 1 year ago

sdcardio was also implemented much later on.