SDcard sdcard.py rewrite for efficiency and function #765
Conversation
|
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 |
|
@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 So, to make progress with this PR I suggest:
|
| # import gc | ||
| # gc.collect() #make sure residual list gets cleaned up to save memory | ||
|
|
||
| crc7_be_syndrome_table = bytearray(256) # pre-allocate so it is low in memory |
There was a problem hiding this comment.
I suggest just making this table a pre-computed bytes object. That will have the smallest memory footprint, and take no time to create when importing this code.
| @@ -63,89 +77,89 @@ def init_spi(self, baudrate): | |||
| # on pyboard | |||
| self.spi.init(master, baudrate=baudrate, phase=0, polarity=0) | |||
|
|
|||
| @staticmethod | |||
| def gb(bigval, b0, bn): | |||
There was a problem hiding this comment.
I suggest making this a global function in the module, ie not part of the SDCard class. Also name it _gb to indicate it's a private function.
| # get numbered bits from a buf_to_int from, for example, the CSD | ||
| return (bigval >> b0) & ((1 << (1 + bn - b0)) - 1) | ||
|
|
||
| def spiff(self): |
There was a problem hiding this comment.
Suggest calling it _spiff to indicate a private method.
| # print("[SDCard] v2 card") | ||
| return | ||
| raise OSError("timeout waiting for v2 card") | ||
|
|
||
| def cmd(self, cmd, arg, crc, final=0, release=True, skip1=False): |
There was a problem hiding this comment.
I suggest removing the crc argument and just always computing it in this function. The only times it's passed in as non-None is for a few initialisation/detection commands at the start, and that's only done once so doesn't need to be efficient (in time).
Removing the crc argument will make code size smaller and faster for the case when None is passed (which is the majority of uses).
There was a problem hiding this comment.
I agree with this. The passing in of fixed CRCs is historically done for the few initialization packets, but why? Do you want me to make the change in my fork, or do you want to do this after you pull it?
All the other suggestions are also fine with me.
I originally had a fixed byte string for the crc7 table, but couldn't figure out how to get black to not barf on the long line length. What is the preferred method? Put it in parentheses and use the parsers automatic string concatenation from shorter strings?
There was a problem hiding this comment.
is this OK style for the crc table, including expectation of memory footprint after parsing?:
crc7_be_syndrome_table = (
b'\x00\x12$6HZl~\x90\x82\xb4\xa6\xd8\xca\xfc\xee2 \x16\x04zh^L\xa2\xb0\x86\x94\xea\xf8'
b'\xce\xdcdv@R,>\x08\x1a\xf4\xe6\xd0\xc2\xbc\xae\x98\x8aVDr`\x1e\x0c:(\xc6\xd4\xe2\xf0'
b'\x8e\x9c\xaa\xb8\xc8\xda\xec\xfe\x80\x92\xa4\xb6XJ|n\x10\x024&\xfa\xe8\xde\xcc\xb2\xa0'
b'\x96\x84jxN\\"0\x06\x14\xac\xbe\x88\x9a\xe4\xf6\xc0\xd2<.\x18\ntfPB\x9e\x8c\xba\xa8'
b'\xd6\xc4\xf2\xe0\x0e\x1c*8FTbp\x82\x90\xa6\xb4\xca\xd8\xee\xfc\x12\x006$ZH~l\xb0\xa2'
b'\x94\x86\xf8\xea\xdc\xce 2\x04\x16hzL^\xe6\xf4\xc2\xd0\xae\xbc\x8a\x98vdR@>,\x1a\x08'
b'\xd4\xc6\xf0\xe2\x9c\x8e\xb8\xaaDV`r\x0c\x1e(:JXn|\x02\x10&4\xda\xc8\xfe\xec\x92\x80'
b'\xb6\xa4xj\\N0"\x14\x06\xe8\xfa\xcc\xde\xa0\xb2\x84\x96.<\n\x18ftBP\xbe\xac\x9a\x88\xf6'
b'\xe4\xd2\xc0\x1c\x0e8*TFpb\x8c\x9e\xa8\xba\xc4\xd6\xe0\xf2'
)There was a problem hiding this comment.
The passing in of fixed CRCs is historically done for the few initialization packets, but why? Do you want me to make the change in my fork, or do you want to do this after you pull it?
I'm not exactly sure why the CRC was there only for the few initialisation packets. But my guess would be 1) it's a static value so doesn't need a function to compute it, 2) for initialisation it may be that some cards require the CRC on these commands.
Anyway, I'm happy for you to make this change as part of this PR.
is this OK style for the crc table, including expectation of memory footprint after parsing?
Yes that looks OK. Maybe indent each line within the parenthesis, if ruff format allows? (we switched from black to ruff). Eg:
table = (
b'...'
b'...'
)Also I suggest putting this crc7 function within sdcard.py. It's simple enough and saves having separate files for the driver, ie everything is self contained in one file.
mendenm
force-pushed
the
sdcard-fixes
branch
4 times, most recently
from
0a521f4
to
ce3738c
Compare
Merged crc7.py into sdcard.py, fixed many little things per suggestions. Removed hardwired crcs from cmd, always recompute. Signed-off-by: Marcus Mendenhall <mendenmh@gmail.com>
mendenm
force-pushed
the
sdcard-fixes
branch
from
ce3738c
to
aecced4
Compare
|
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. |
This module has been heavily reworked. I went through the logic, and compared it to the SDCard standard, and found numerous ways it could be made faster, and work more reliably. Lots of code has disappeared. Also, it computes CRC7 on al commands (some cards didn't work without this), and has the option to compute CRC16 on data transfers, if the user provides a suitable function.