Thanks @mendenm -- at first glance this all looks very good. I will try to find some time soon to review in detail.

One quick note... we don't currently have a way to publish .mpy files with native code to mip, so we will need to solve that first. Also we will need a fallback implementation for devices that do not support loading native code.

@jimmo Is there any chance you can take a quick look at the PR to see if you can see why ruff keeps crashing (exiting with error 1)? Running is locally seems to return no problems, but I can't get past the github process.

Thanks.

On Nov 9, 2023, at 10:30 PM, Jim Mussared @.***> wrote:

Thanks @mendenm -- at first glance this all looks very good. I will try to find some time soon to review in detail.
One quick note... we don't currently have a way to publish .mpy files with native code to mip, so we will need to solve that first. Also we will need a fallback implementation for devices that do not support loading native code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: @.***>

@jimmo Is there any chance you can take a quick look at the PR to see if you can see why ruff keeps crashing (exiting with error 1)? Running is locally seems to return no problems, but I can't get past the github process.

Never mind, I got past it.

Thanks.

On Nov 9, 2023, at 10:30 PM, Jim Mussared @.***> wrote:

Thanks @mendenm -- at first glance this all looks very good. I will try to find some time soon to review in detail.
One quick note... we don't currently have a way to publish .mpy files with native code to mip, so we will need to solve that first. Also we will need a fallback implementation for devices that do not support loading native code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: @.***>

As @jimmo mentioned we don't yet have support for @micropython.viper (or native or asm_thumb) in this repository, because the .py file is compiled to .mpy and that compilation must use a different architecture flag for different target machines, and hence we would need different .mpy files, one for each supported architecture.

So, to make progress with this PR I suggest:

• move the fast crc routines to example/util files which are not included in the manifest.py, and hence not included in the distributed package
• implement only the crc7 function and do it in pure Python
• still support passing in a function for CRC16 data checks, the user can then copy the fast crc16 routines if they want to use this feature

I did a massive squash of the git repo, to get rid of all the drafts. I think this should be very close to consistent with comments and request. One thing I did not do was make gb() in _gb(). It's not really that private. Future users may want to parse other fields out the the big status block to get more info about their sdcards.

Is there a reason this is still stuck in PR purgatory?

Some cursory checks suggest that:

1. It still works
2. It's slightly faster

I'd be happy to vendor these libs into one of our builds to sniff out potential issues if it helps.

@Gadgetoid I'd sure be glad to have more people stress-testing it. I think the code is robust, but it's always the case that more test cases are better. I personally have only tested it on my Pi Pico, but with a fairly wide variety of SD cards, from ancient (< 2 GB) to 32 GB flavors.

I have a question about the gb() naming and the spiff and the `blocks()as naming.  I was under the impression that micropython actually pays a small time penalty for longer names, so I tried to keep most names fairly short.  If this isn’t really an issue, I’ll be glad to make these changes.I will fix all the random double spaces, gratuitous leftover commented sections, etc. . Give me a couple days to get to it.  Thanks for getting this back in the flow.MarcusOn Apr 10, 2025, at 10:06 AM, Damien George @.***> wrote:
@dpgeorge commented on this pull request.

Sorry this got lost for so long. I've now done a final review and tested it on a PYBv1.0. It works well, and this will be a really great improvement to have!

In micropython/drivers/storage/sdcard/crc16.py:

@@ -0,0 +1,111 @@
+import micropython

I suggest a short comment at the top here, something like:

This file provides optional CRC16 support for the sdcard.SDCard driver.

To use it you need to manually copy this file to your device, then:

import crc16

sdcard.SDCard(..., crc16_function=crc16)

In micropython/drivers/storage/sdcard/crc16.py:

+# ruff: noqa: F821 - @asm_thumb and @viper decorator adds names to function scope

+# https://electronics.stackexchange.com/questions/321304/how-to-use-the-data-crc-of-sd-cards-in-spi-mode
+# for sd bit mode
+import array
+
+_sd_crc16_table = array.array("H", (0 for _ in range(256)))
+# /* Generate CRC16 table */
+# for (byt = 0U; byt < 256U; byt ++){
+# crc = byt << 8;
+# for (bit = 0U; bit < 8U; bit ++){
+# crc <<= 1;
+# if ((crc & 0x10000U) != 0U){ crc ^= 0x1021U; }
+# }
+# sd_crc16_table[byt] = (crc & 0xFFFFU);
+# }

Does it add anything to keep this C code? The Python code is just as readable.

In micropython/drivers/storage/sdcard/crc16.py:

+# def test_speed():
+# data = b"\xaa"*1024
+# import time
+# crc = 0
+# start = time.ticks_us()
+# for i in range(1024):
+# crc = crc16(crc, data)
+# print("asm crc speed = ", f"{crc:08x}", 220 / (time.ticks_diff(time.ticks_us(), start) / 1e6), "bytes/s")
+#
+# crc = 0
+# start = time.ticks_us()
+# for i in range(1024):
+# crc = crc16_viper(crc, data)
+# print("py crc speed = ", f"{crc:08x}", 220 / (time.ticks_diff(time.ticks_us(), start) / 1e6), "bytes/s")

Best not to keep commented-out code. Either delete it, uncomment it so it can be used, or put it in a separate crc test file.

In micropython/drivers/storage/sdcard/sd_card_spi_test.py:

@@ -0,0 +1,101 @@
+# SD card setup

I suggest removing this test file, because it's mostly a copy of the existing sdtest.py, and is pretty rough code (eg imports that are never used).
If there's anything useful in here, I suggest adding it to the existing sdtest.py.

In micropython/drivers/storage/sdcard/sdcard.py:

+#
+# import pyb, sdcard, os
+# sd = sdcard.SDCard(pyb.SPI(1), pyb.Pin.board.X5)
+# pyb.mount(sd, '/sd2')
+# os.listdir('/')
+#
+# Example usage on ESP8266:
+#
+# import machine, sdcard, os
+# sd = sdcard.SDCard(machine.SPI(1), machine.Pin(15))
+# os.mount(sd, '/sd')
+# os.listdir('/')
+#
+# Note about the crc_function:
+# this is crc(seed: int, buf: buffer) -> int
+# If no crc16 function is provided, CRCs are not computed on data transfers.

Please remove double spaces within sentences (eg between crc16 and function here).

In micropython/drivers/storage/sdcard/sdcard.py:

+# import pyb, sdcard, os
+# sd = sdcard.SDCard(pyb.SPI(1), pyb.Pin.board.X5)
+# pyb.mount(sd, '/sd2')
+# os.listdir('/')
+#
+# Example usage on ESP8266:
+#
+# import machine, sdcard, os
+# sd = sdcard.SDCard(machine.SPI(1), machine.Pin(15))
+# os.mount(sd, '/sd')
+# os.listdir('/')
+#
+# Note about the crc_function:
+# this is crc(seed: int, buf: buffer) -> int
+# If no crc16 function is provided, CRCs are not computed on data transfers.
+# If a crc16 is provided, the CRC function of the SD card is enabled,

Please remove double space.

In micropython/drivers/storage/sdcard/sdcard.py:

+# import machine, sdcard, os
+# sd = sdcard.SDCard(machine.SPI(1), machine.Pin(15))
+# os.mount(sd, '/sd')
+# os.listdir('/')
+#
+# Note about the crc_function:
+# this is crc(seed: int, buf: buffer) -> int
+# If no crc16 function is provided, CRCs are not computed on data transfers.
+# If a crc16 is provided, the CRC function of the SD card is enabled,
+# and data transfers both ways are protected by it
+#

+import micropython
+from micropython import const
+import time
+import uctypes

micropython and uctypes are never used, so remove their import.

In micropython/drivers/storage/sdcard/sdcard.py:

• import machine, sdcard, os
• sd = sdcard.SDCard(machine.SPI(1), machine.Pin(15))
• os.mount(sd, '/sd')
• os.listdir('/')
  +def gb(bigval, b0, bn):

I suggest calling this get_bits() to make it clear what it does, since it's a public function.

In micropython/drivers/storage/sdcard/sdcard.py:

class SDCard:

• def init(self, spi, cs, baudrate=1320000):

• def init(self, spi, cs, baudrate=1320000, crc16_function=None):

I suggest calling the argument just crc16.

In micropython/drivers/storage/sdcard/sdcard.py:

     # wait for write to finish

     while self.spi.read(1, 0xFF)[0] == 0x00:
         pass

     self.cs(1)

•    self.spi.write(b"\xff")
  

•    self._spiff()
  

• @staticmethod
• def blocks(buf):

Suggest making this private _blocks().

In micropython/drivers/storage/sdcard/sdcard.py:

     # read checksum

•    self.spi.write(b"\xff")
  

•    self.spi.write(b"\xff")
  

•    ck = self.spi.read(2, 0xFF)
  

•    if self.crc16:
  

•        crc = self.crc16(self.crc16(0, buf), ck)
  

•        if crc != 0:
  

•            raise OSError(EIO, f"bad data CRC: {crc:04x}")
  

Please don't use f-strings, not all MicroPython devices have this feature enabled.
Instead: "bad data CRC: {:04x}".format(crc)

In micropython/drivers/storage/sdcard/sdcard.py:

         if not (response & 0x80):

             # this could be a big-endian integer that we are getting here
             # if final<0 then store the first byte to tokenbuf and discard the rest

•            if response & _R1_COM_CRC_ERROR:
  

•                cs(1)
  

•                spiff()
  

•                raise OSError(EIO, f"CRC err on cmd: {cmd:02d}")
  

Please don't use f-strings (see other comment about this).

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

I have a question about the gb() naming and the spiff and the `blocks()as naming. I was under the impression that micropython actually pays a small time penalty for longer names, so I tried to keep most names fairly short. If this isn’t really an issue, I’ll be glad to make these changes

It's more about the memory use than speed. Shorter names take less flash/memory, are smaller in compiled .mpy files and faster to transmit/copy. I'm not sure it's faster to use shorter names, if it is it would only be a very tiny improvement due to a shorter memcmp().

For gb() I just feel that it's too cryptic for a public function. Maybe bits() would be better and still short?

For spiff(): this is purely a helper function and can be called anything, even _s() if you like. But maybe it's not even needed, because it's such a short function it may be better to just write it out inline wherever it's used. After all, in most cases the code has a w = self.spi.write local variable so spiff would be w(b'\xff').

If you want to measure the impact of a change to the Python code, you can run it through mpy-cross -O3 sdtest.py and see how big the output sdcard.mpy is. The smaller the better! (Because smaller means less bytecode (unless you also shorten names), means faster execution.)

Thanks for the clarification.  I may move _spiff back inline.  I created before I started pre-binding the writes to the short name ‘w’, so its purpose may be superseded now. I may have time this weekend to do all the editing.MarcusOn Apr 10, 2025, at 7:55 PM, Damien George @.***> wrote:

I have a question about the gb() naming and the spiff and the `blocks()as naming. I was under the impression that micropython actually pays a small time penalty for longer names, so I tried to keep most names fairly short. If this isn’t really an issue, I’ll be glad to make these changes

It's more about the memory use than speed. Shorter names take less flash/memory, are smaller in compiled .mpy files and faster to transmit/copy. I'm not sure it's faster to use shorter names, if it is it would only be a very tiny improvement due to a shorter memcmp().
For gb() I just feel that it's too cryptic for a public function. Maybe bits() would be better and still short?
For spiff(): this is purely a helper function and can be called anything, even _s() if you like. But maybe it's not even needed, because it's such a short function it may be better to just write it out inline wherever it's used. After all, in most cases the code has a w = self.spi.write local variable so spiff would be w(b'\xff').
If you want to measure the impact of a change to the Python code, you can run it through mpy-cross -O3 sdtest.py and see how big the output sdcard.mpy is. The smaller the better! (Because smaller means less bytecode (unless you also shorten names), means faster execution.)—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

dpgeorge left a comment (micropython/micropython-lib#765)

I have a question about the gb() naming and the spiff and the `blocks()as naming. I was under the impression that micropython actually pays a small time penalty for longer names, so I tried to keep most names fairly short. If this isn’t really an issue, I’ll be glad to make these changes

It's more about the memory use than speed. Shorter names take less flash/memory, are smaller in compiled .mpy files and faster to transmit/copy. I'm not sure it's faster to use shorter names, if it is it would only be a very tiny improvement due to a shorter memcmp().
For gb() I just feel that it's too cryptic for a public function. Maybe bits() would be better and still short?
For spiff(): this is purely a helper function and can be called anything, even _s() if you like. But maybe it's not even needed, because it's such a short function it may be better to just write it out inline wherever it's used. After all, in most cases the code has a w = self.spi.write local variable so spiff would be w(b'\xff').
If you want to measure the impact of a change to the Python code, you can run it through mpy-cross -O3 sdtest.py and see how big the output sdcard.mpy is. The smaller the better! (Because smaller means less bytecode (unless you also shorten names), means faster execution.)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

I can't say for sure yet where the issue is, but this driver always errors out when I try to bring up an sd card:

Traceback (most recent call last):
  File "/lib/freedeej/hw.py", line 188, in sd_init
  File "sdcard/sdcard.py", line 85, in __init__
  File "sdcard/sdcard.py", line 121, in init_card
  File "sdcard/sdcard.py", line 209, in cmd
OSError: [Errno 5] EIO: CRC err on cmd: 00

What's weird is I swear this code used to work intermittently with the reader/card I'm using, but maybe it's possible this aliexpress reader just gave up the ghost.

@mischif
I had the same problem.
My investigations revealed that the error occurs reproducibly exactly when
the SD card is accessed for the first time
(after switching on the power supply or after inserting the SD card).
The solution in init_card() in sdcard.py:

        # CMD0: init card; should return _R1_IDLE_STATE (allow 5 attempts)
        for _ in range(5):
            try: 
                if self.cmd(0, 0, 0x95) == _R1_IDLE_STATE:
                    break
            except OSError as err:
                if err.errno != EIO:
                    raise
        else:
            raise OSError(ENODEV, "no SD card")

Live CRC7 support has been merged in for SD card support. This PR apparently has some fixes for additional features that would be useful and speed improvements. Those features should be committed one at a time and sent in an updated or new PR.

Note the commit structure here: https://github.com/micropython/micropython-lib/pull/1088/commits

It's one thing per commit... so that you can follow what's happening.

I'm happy now with the current state of sdcard support now that crc7 is added. However, happy to see additional fixes and improvements.