Sdcard in core #2863

jepler · 2020-05-06T20:37:30Z

This pull request adds sdcardio.SDCard and, for select atmel-sam boards, sdio.SDCard.

sdcardio.SDCard is nearly, but not quite, compatible with adafruit_sdcard.SDCard: It takes a microcontroller.Pin for the chip select (cs) line, not a digitalio.DigitalInOut object, because this simplifies object lifetime and may help us to someday let vfs mounts survive soft-reset. It also defaults to a higher baudrate, 8MHz instead of 1.32MHz.

sdcardio benchmarks vary from card to card, but on one 4GB card, I measured an increase in read rates from 583 kB/s to 838.2kB/s, an increase of about 40%. Write rates remain very low, just 4kB/s(!) with adafruit_sdcard and sdcardio.

All sdcardio testing was on a PyGamer with built in SD card slot. Three SD cards were tested (4GB Sandisk, 8GB Sandisk, 64GB Samsung). Some lower capacity SD cards are on order and I'll verify they all work, or if they don't whether they work under adafruit_sdcard.

sdcardio is enabled on all "full-build"s except M0, PCA10100, and litex, for reasons outlined in the individual commits.

sdioio implements an SD card interface using the SDIO bus. This is enabled and tested on the Microchip SAME54 Xplained Pro board and is also enabled on the Grand Central. The relevant pins are on headers of the Grand Central M4 Express, but (likely due to dodgy wiring) did not work in my testing.

Known problems:

If no card is detected, sdioio.SDCard hangs at initialization
sdcardio.SDCard misidentifies the capacity of a particular 64GB card I have for testing, same as adafruit_sdcard

We made the decision to deprioritize the stm32f405 feather support for SDIO SD cards. This work is not finished and has not been renamed to suit the latest decisions, and so it have been removed from this PR. See https://github.com/jepler/circuitpython/tree/sdio-stm32-broken-rename -- We will get back to this at some point, but we would also love to see a community member pick up the work. Two main things remain to be done: Follow the naming convention of sdioio and finish the pin logic in the constructor.

shared-bindings/sdcardio/SDCard.c

tannewt · 2020-05-15T17:53:46Z

@dhalbert is this review on your radar?

tannewt · 2020-05-15T18:29:56Z

For now, let's call this _sdcardio just in the top level so that we can evolve the API as SDIO support is added. Internally the names can stay the same.

dhalbert · 2020-05-15T18:53:43Z

@tannewt Sorry, I looked for this the other day, and it seemed like a number of changes were pending, so I waited.

jerryneedell

OK -- I found the commit fixing the doc example -- thanks !

shared-bindings/sdcardio/SDCard.c

dhalbert

I only had stylistic comments. It looks good but I have not tested it. Are we going to rewrite adafruit_sdcard to be a thin wrapper around this for now, or just deprecate it?

ports/litex/mpconfigport.mk

ports/nrf/boards/pca10100/mpconfigboard.mk

shared-bindings/sdcardio/SDCard.c

shared-bindings/sdcardio/__init__.c

shared-module/sdcardio/SDCard.c

jepler · 2020-05-15T19:46:45Z

Thanks @dhalbert I'll take your comments into account when I revise the PR to rename it as _sdcardio.

jepler · 2020-05-15T19:47:21Z

I think adafruit_sdcard will still be used for M0 devices where we don't have space for sdcardio in flash.

adafruit_sdcard could check for the existence of _sdcardio and use it instead if it's present. So there could be one version of various programs for both M0 and M4 boards.

jerryneedell · 2020-05-19T10:50:34Z

tested this PR on a grand_central_m4_express
It works!

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 5.3.0-187-g82024d078 on 2020-05-19; Adafruit Grand Central M4 Express with samd51p20
>>> import sdcardio_test
['hello.py', 'images']
>>>

I had to make a few changes to the example since the Grand Central M4 does not use the statndar SPI bus for the SD Card. here is the example I ran

import adafruit_sdcard
import busio
import sdcardio
import digitalio
import board
import storage
import sys
import os

# Connect to the card and mount the filesystem.
spi = busio.SPI(board.SD_SCK, board.SD_MOSI, board.SD_MISO)
sdcard = sdcardio.SDCard(spi, board.SD_CS)
vfs = storage.VfsFat(sdcard)
storage.mount(vfs, "/sd")
print(os.listdir('/sd'))

took it one step further and actually executed a python script (hello.py) from the sdcard

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 5.3.0-187-g82024d078 on 2020-05-19; Adafruit Grand Central M4 Express with samd51p20
>>> 
>>> 
>>> import sdcardio_lib
>>> import hello
Howdy
>>>

here is the example sdcardio_lib.py

import adafruit_sdcard
import busio
import sdcardio
import digitalio
import board
import storage
import sys
import os

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

oops _I noticed that I was still importing adafruit_sdcard -- It still works when I remove it!

jerryneedell@jerryneedell-ubuntu-macmini:~/projects/grand_central$ cat sdcardio_test.py 
import busio
import sdcardio
import board
import storage
import sys
import os

# Connect to the card and mount the filesystem.
spi = busio.SPI(board.SD_SCK, board.SD_MOSI, board.SD_MISO)
sdcard = sdcardio.SDCard(spi, board.SD_CS)
vfs = storage.VfsFat(sdcard)
storage.mount(vfs, "/sd")
print(os.listdir('/sd'))

and

jerryneedell@jerryneedell-ubuntu-macmini:~/projects/grand_central$ cat sdcardio_lib.py 
import busio
import sdcardio
import board
import storage
import sys
import os

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

dhalbert · 2020-06-01T19:43:17Z

@jepler There are still some unresolved comments above (alphabetization and "not bitbangio.SPI")

jepler · 2020-06-02T17:36:04Z

This has been updated with SDIO for STM32F405 Feather.

jerryneedell · 2020-06-02T18:07:34Z

Tested SDIOCard example on feather_stm32f405_express -- worked fine!
Also tested adding /sd to path and executed simple script from the sdcard.

jepler

Note for myself: https://github.com/avrxml/asf/blame/61c7353e0183bbd0615db1d0a3144cbdcd09983a/common/components/memory/sd_mmc/sd_mmc.c

Also, this PR presently reverts two submodules to an intended ref. oof.

ports/stm/Makefile

ports/stm/supervisor/port.c

py/circuitpy_mpconfig.mk

tannewt · 2020-06-03T19:15:56Z

I think it'd be better to separate the bus from the SDCard APIs here.

# Connect to the card and mount the filesystem.
bus = busio.SPI(board.SD_SCK, board.SD_MOSI, board.SD_MISO)
device = adafruit_bus_device.SPIDevice(bus, board.SD_CS) # Also takes frequency, polarity and phase.
# For SDIO
# device = sdioio.SDIO(clock=board.SDIO_CLOCK, command=board.SDIO_COMMAND, data=(board.SDIO_DATA,), frequency=25000000)
sdcard = sdcardio.SDCard(device)

hierophect

Just a couple comments about the Systick and peripherals structures. I'm going to take a closer look at HAL_GetTick and try to get a handle on what's going on with the low power code.

ports/stm/common-hal/_sdcardio/SDIOSDCard.c

ports/stm/supervisor/port.c

jepler · 2020-06-25T15:32:18Z

This has grown and is really multiple different topics. We need to split it up into multiple PRs. Our priority list becomes:

SPI SD card in core, supporting all platforms
SAM D5x/E5x SDIO
STM SDIO

(that's not to say we wouldn't love someone to pick up #3 before I get a chance to! It's still the only reasonable way to use the SD card on the STM32F405 Feather. The main thing that needs completion on STM32F405 is handling of pin assignments, instead of ignoring them as now)

We also need to nail down what the API is going to look like, to avoid more renames and incompatible changes if possible. We have discussed this informally a number of times, but I'd like to get to an official decision.

How many modules, 1 or 2? How are they named? Suggestion: sdcardio for SPI interface SD cards and sdioio (for SDIO interface) Conclusion: 2 modules
Are the module names underscored (_sdcardio) or not (sdcardio)? My preference: No underscore Conclusion: No underscore
Does the SPI interface take (spi: busio.SPI, cs: microcontroller.Pin) or (bus: adafruit_busdevice.spi_device.SPIDevice)? My preference: (spi, cs). One reason: we require that the SPI bus is a real hardware SPI bus, and that the CS is a real physical pin, while SPIDevice requires no such things. Conclusion: This is a good trade-off.

If you take my suggestions/preferences, this leads to the following code snippets for mounting SD storage, depending on interface:

sd = sdioio.SDCard(clock=board.SDIO_CLOCK, command=board.SDIO_COMMAND, data=board.SDIO_DATA)
vfs = storage.VfsFat(sd)
storage.mount(vfs, "/sd")

and

sd = sdcardio.SDCard(spi=board.SPI(), cs=board.SD_CS)
vfs = storage.VfsFat(sd)
storage.mount(vfs, "/sd")

We are going to do priorities 1 and 2 in this PR and push #3 to a separate PR or issue.

(comment updated based on discussion with @tannewt via discord)

jepler · 2020-06-25T15:41:24Z

@tannewt @dhalbert your comment on the above would be very welcome.

Testing performed: That a card is successfully mounted on Pygamer with the built in SD card slot This module is enabled for most FULL_BUILD boards, but is disabled for samd21 ("M0"), litex, and pca10100 for various reasons.

This will ultimately be used by SDIO, where a variable length list of data lines is called for.

There is no implementation yet.

jepler · 2020-06-26T16:55:53Z

@tannewt @dhalbert I think this is ready for another round of review. The initial description of the PR has been heavily updated to reflect the current state of this branch.

tannewt

A couple minor things.

ports/atmel-samd/common-hal/sdioio/SDCard.c

shared-bindings/sdcardio/SDCard.c

shared-bindings/sdioio/SDCard.c

Co-authored-by: Scott Shawcroft <[email protected]>

I believe I've addressed all your requested changes

tannewt

Thank you for this! Folks will be excited for the speedups!

jepler force-pushed the sdcard-in-core branch 2 times, most recently from 1d2fc3f to a2becfa Compare May 8, 2020 19:22

jepler marked this pull request as ready for review May 9, 2020 15:18

tannewt requested a review from dhalbert May 11, 2020 22:59

tannewt added circuitpython api enhancement performance labels May 11, 2020

jerryneedell reviewed May 15, 2020

View reviewed changes

shared-bindings/sdcardio/SDCard.c Outdated Show resolved Hide resolved

jerryneedell reviewed May 15, 2020

View reviewed changes

shared-bindings/sdcardio/SDCard.c Outdated Show resolved Hide resolved

dhalbert previously requested changes May 15, 2020

View reviewed changes

jepler mentioned this pull request May 29, 2020

storage module is missing from documentation (after stubs conversion?) #2976

Closed

jepler force-pushed the sdcard-in-core branch from 82024d0 to 121e9ec Compare June 1, 2020 13:03

hierophect requested a review from dhalbert June 1, 2020 19:36

jepler marked this pull request as draft June 1, 2020 19:48

jepler force-pushed the sdcard-in-core branch from 121e9ec to e8d8004 Compare June 2, 2020 17:34

jepler commented Jun 3, 2020

View reviewed changes

ports/stm/Makefile Outdated Show resolved Hide resolved

ports/stm/supervisor/port.c Outdated Show resolved Hide resolved

py/circuitpy_mpconfig.mk Outdated Show resolved Hide resolved

jepler force-pushed the sdcard-in-core branch from e8d8004 to 697ef6f Compare June 3, 2020 15:54

jepler force-pushed the sdcard-in-core branch from 5dd7a5d to 0b4b95c Compare June 3, 2020 19:47

hierophect reviewed Jun 4, 2020

View reviewed changes

ports/stm/common-hal/_sdcardio/SDIOSDCard.c Outdated Show resolved Hide resolved

ports/stm/common-hal/_sdcardio/SDIOSDCard.c Outdated Show resolved Hide resolved

ports/stm/supervisor/port.c Outdated Show resolved Hide resolved

tannewt changed the base branch from master to main June 9, 2020 20:06

jepler mentioned this pull request Jun 18, 2020

Add basic SAM E54 support and SAM E54 Xplained board #3046

Merged

jepler force-pushed the sdcard-in-core branch 4 times, most recently from 52e6ec5 to 7499e1c Compare June 25, 2020 15:14

jepler force-pushed the sdcard-in-core branch from 7499e1c to 1203482 Compare June 25, 2020 15:35

jepler force-pushed the sdcard-in-core branch 2 times, most recently from 46096e6 to 28b1d0b Compare June 26, 2020 16:24

jepler marked this pull request as ready for review June 26, 2020 16:25

jepler added 5 commits June 26, 2020 11:50

sdcardio: implement new library for SD card I/O

57fde2e

Testing performed: That a card is successfully mounted on Pygamer with the built in SD card slot This module is enabled for most FULL_BUILD boards, but is disabled for samd21 ("M0"), litex, and pca10100 for various reasons.

shared-bindings: Factor out validate_list_is_free_pins

159728b

This will ultimately be used by SDIO, where a variable length list of data lines is called for.

sdioio: Add shared-bindings

05837b2

There is no implementation yet.

samd: sdio: Add needed files from atmel start

d4b9458

atmel-samd: Add SDIO SD card interface

f496c0b

jepler force-pushed the sdcard-in-core branch from 28b1d0b to f496c0b Compare June 26, 2020 16:52

jepler requested a review from tannewt June 26, 2020 16:56

tannewt previously requested changes Jun 26, 2020

View reviewed changes

ports/atmel-samd/common-hal/sdioio/SDCard.c Outdated Show resolved Hide resolved

shared-bindings/sdcardio/SDCard.c Outdated Show resolved Hide resolved

shared-bindings/sdioio/SDCard.c Outdated Show resolved Hide resolved

jepler and others added 3 commits June 26, 2020 13:15

Update shared-bindings/sdcardio/SDCard.c

0f2fb93

Co-authored-by: Scott Shawcroft <[email protected]>

atmel-sam: sdio: disable debug prints by default

c5db97c

sdioio: Remove junk in comments

472a04b

jepler requested a review from tannewt June 29, 2020 13:08

tannewt approved these changes Jun 29, 2020

View reviewed changes

tannewt merged commit 111f7dd into adafruit:main Jun 29, 2020

jepler deleted the sdcard-in-core branch November 3, 2021 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sdcard in core #2863

Sdcard in core #2863

jepler commented May 6, 2020 •

edited

Loading

tannewt commented May 15, 2020

tannewt commented May 15, 2020

dhalbert commented May 15, 2020

jerryneedell left a comment

dhalbert left a comment

jepler commented May 15, 2020

jepler commented May 15, 2020 •

edited by dhalbert

Loading

jerryneedell commented May 19, 2020 •

edited

Loading

dhalbert commented Jun 1, 2020

jepler commented Jun 2, 2020

jerryneedell commented Jun 2, 2020

jepler left a comment

tannewt commented Jun 3, 2020

hierophect left a comment

jepler commented Jun 25, 2020 •

edited

Loading

jepler commented Jun 25, 2020

jepler commented Jun 26, 2020

tannewt left a comment

tannewt left a comment

Sdcard in core #2863

Sdcard in core #2863

Conversation

jepler commented May 6, 2020 • edited Loading

tannewt commented May 15, 2020

tannewt commented May 15, 2020

dhalbert commented May 15, 2020

jerryneedell left a comment

Choose a reason for hiding this comment

dhalbert left a comment

Choose a reason for hiding this comment

jepler commented May 15, 2020

jepler commented May 15, 2020 • edited by dhalbert Loading

jerryneedell commented May 19, 2020 • edited Loading

dhalbert commented Jun 1, 2020

jepler commented Jun 2, 2020

jerryneedell commented Jun 2, 2020

jepler left a comment

Choose a reason for hiding this comment

tannewt commented Jun 3, 2020

hierophect left a comment

Choose a reason for hiding this comment

jepler commented Jun 25, 2020 • edited Loading

jepler commented Jun 25, 2020

jepler commented Jun 26, 2020

tannewt left a comment

Choose a reason for hiding this comment

tannewt left a comment

Choose a reason for hiding this comment

jepler commented May 6, 2020 •

edited

Loading

jepler commented May 15, 2020 •

edited by dhalbert

Loading

jerryneedell commented May 19, 2020 •

edited

Loading

jepler commented Jun 25, 2020 •

edited

Loading