Fixed Issue 13, Samsung EVO 32GB cards not working

[devoh747]

The issue is that Samsung EVO 32GB cards require all command codes to have a valid CRC. I used https://github.com/hazelnusse/crc7/blob/master/crc7.cc to calculate all CRC codes and it is now working. On line 195 I also changed one command from a _block_cmd to a _cmd. It would hang otherwise. This works for my SanDisk 16 GB card which worked before the fix as well and also works on my Samsung EVO 32GB. Please test this with all cards. The spec does say that by default all command code CRC values are ignored except CM0, CMD8 and I believe ACMD41.. It's just that Samsung decided to ignore the default and check all command CRC values.. I was not even able to force it off.

[ladyada]

wow nice!

[devoh747]

It’s my first fix so please let me know if I did something wrong in my submission or anything. What a rush fixing a bug 😊 From: ladyada <[email protected]> Sent: Monday, August 5, 2019 2:52 PM To: adafruit/Adafruit_CircuitPython_SD <[email protected]> Cc: devoh747 <[email protected]>; Author <[email protected]> Subject: Re: [adafruit/Adafruit_CircuitPython_SD] Fixed Issue 13, Samsung EVO 32GB cards not working (#19) wow nice! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19?email_source=notifications&email_token=ADSGIOU2BPVESPTZIVMJH3DQDBZFVA5CNFSM4IJN5SU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3SXN3A#issuecomment-518354668> , or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSGIOUXLGTTZA5MGDB2YRTQDBZFVANCNFSM4IJN5SUQ> . <https://github.com/notifications/beacon/ADSGIORDKQP7KZQOTEF3QK3QDBZFVA5CNFSM4IJN5SU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3SXN3A.gif>

[tannewt]

Epic job on this!

Hardcoding the crc for static commands is fine. However, there are a couple places where the arguments themselves change. How hard is it to compute the crc in _cmd ourselves every time? That way it'd always be included and correct. Thanks!

[devoh747]

Scott,
I had gone thru the code and converted every call that I could find. I do not see any that do not have a CRC(except _init_v1() but that is intentional. From what I have read only high capacity cards have this to improve reliability. I also do not have such a card to test with.). Do you have any examples of lines that were missed? Your idea of generalizing _cmd to generate the CRC at runtime is interesting. My initial thought is it would create extra overhead with every read/write to the card and I already have all the _cmd calls that I can see pre-calculated. Your use case would cover any additional calls that might be added in the future. Perhaps an improvement would be to use my pre-calculated CRC's and add a check inside _cmd to see if the CRC was passed in.. if is is then use it, if not then calculate it. This would eliminate the overhead for read/writes as I have it already calculated and sent in.. but would allow a more generalized _cmd call for library changes in the future.. An idea on how to implement is to possibly grab the relevant code from https://pypi.org/project/libscrc/ for crc7 .

[tannewt]

I'm ok with hard coding the ones that don't change. I just commented on the ones that will have to change because the argument changes.

[devoh747]

ahhhhh Ok now I understand. Sorry I was not thinking that start_block contained arguments. I was just thinking that "start_block" WAS the argument and therefore not changing. :) Well darn.. now I need to figure out how to dynamically generate CRCs.. as the _block_cmd function is wrong in my pull request.

I guess my solution will be to leave all your examples from _block_cmd with a CRC of 0.. and try and modify _cmd to generate CRC's dynamically if the CRC is passed in as a 0.

It's interesting that _block_cmd is using the wrong CRCs and yet it seems like the Samsung EVO 32GB still works for my simple example of just printing out the temp of the SAMD51 chip.. I wonder if that has something to do with #11 .. With a SanDisk 16GB I run out of memory.. but with the Samsung I hang.. which would make sense if I am feeding it bad CRCs and it is actually looking at them and needing them to be valid. Hey maybe we will be fixing Issue #11 and #13/#15 in the end here. :)

[devoh747]

I have what is looking like working crc generation code. It is a single function that is called calculate_crc(message_buffer) that takes a message of any length and returns the crc7-mmc for it. I will add it to adafruit_sdcard and call it from the _cmd function when the CRC passed in is 0. I will then do some testing. If all goes well I will reissue my pull request with all the changes and we can discuss some more.

[devoh747]

Help! I made all the changes you recommended.. but I wanted to make it one nice commit instead of a couple.. I couldn't figure out how to do that so I just blew away my repo and reforked/downloaded and uploaded my new changes to my repo.. figuring that I could then push my repo and you would have just one commit instead of a couple.. well I think that made things worse. I can't see how to push my changes without creating a new pull request.. which probably requires that I close my old one.

[makermelissa]

Multiple commits is just fine. We can see all the changes as a whole in the PR.

[devoh747]

is the PR the same as a commit? I created a new PR #20 . Sorry I will do Kattni's learn guide on github and contributing.

[makermelissa]

Not quite, a PR or Pull Request can include many commits and is a set of changes to address a particular feature or bug. Sometimes a travis error or requested changes from a review will result in multiple commits.

[devoh747]

Not sure if this one should be closed in favor of #20.. they are both for the same thing.. Just that #20 has the improvements Scott mentioned.

[makermelissa]

If they're basically the same code, then only keep the one that works better. :)

[devoh747]

Both work, #20 has the improvements that Scott requested (Dynamic CRC)