Skip to content

sd package: sdcard package redesign#639

Open
soypat wants to merge 27 commits into
devfrom
sdcard-refactor
Open

sd package: sdcard package redesign#639
soypat wants to merge 27 commits into
devfrom
sdcard-refactor

Conversation

@soypat

@soypat soypat commented Jan 14, 2024

Copy link
Copy Markdown
Contributor

New Features:

  • Idiomatic CSD V1,V2 support with V3 API compatibility
  • No heap allocations
  • Complete error handling
  • CRC support
  • Design for interface compatibility with other SDCard implementations i.e. Using raw SDIO with a Raspberry Pi Pico.
  • Added tests for what is testable
  • Renamed ReadData to more idiomatic ReadBlock, same with Write methods
  • More checks for OOB write/read requests
  • Allow multi block writing and multi block reading in a double method interface, making it easier to use.
  • Easily enable highest transfer rate available for SDCard using CSD

Is still a WIP. CRC7 still does not work and I'm getting errors on ever other ReadBlock call.

@soypat
soypat marked this pull request as ready for review January 15, 2024 04:34
@bgould

bgould commented Jan 15, 2024

Copy link
Copy Markdown
Member

Is it possible to make this implement BlockDevice so that it is natively compatible with tinyfs? Skimming definitions.go, it looks there are some similar methods in there

https://github.com/tinygo-org/tinyfs/blob/release/tinyfs.go#L43

@soypat

soypat commented Jan 15, 2024

Copy link
Copy Markdown
Contributor Author

@bgould Will do, but I'll do it through another type that uses the ReadBlocks/WriteBlocks interface set to implement the tinyfs.BlockDevice interface. Already had this in mind as the next addition to sd

@soypat

soypat commented Jan 15, 2024

Copy link
Copy Markdown
Contributor Author

@soypat
soypat marked this pull request as draft January 16, 2024 02:37
@soypat
soypat marked this pull request as ready for review January 16, 2024 03:47
@soypat

soypat commented Jan 16, 2024

Copy link
Copy Markdown
Contributor Author

@bgould OK, I've finished the sd.BlockDevice type, seems to be working OK. Check it out and let me know what you think :)

@deadprogram

deadprogram commented Jan 23, 2024

Copy link
Copy Markdown
Member

@soypat would you perhaps be able to add a README with some small explanation about how this works?

For example what is rustref.go do? 😸

@soypat

soypat commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

OK, added a Readme and switched some filenames around to make navigation easier.

@deadprogram

Copy link
Copy Markdown
Member

@bgould have you had a chance to try this out yet?

@deadprogram
deadprogram requested a review from bgould April 1, 2024 06:44
@deadprogram

Copy link
Copy Markdown
Member

Paging Dr. @bgould 😸 any chance to try this out?

@soypat

soypat commented Sep 8, 2025

Copy link
Copy Markdown
Contributor Author

I need this for a class I'll be teaching in 1.5 weeks :)

@soypat

soypat commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Can we merge this?

@deadprogram

Copy link
Copy Markdown
Member

You should add something to smoketest.sh right? And maybe just a couple more docs, perhaps borrow some from the sdcard driver this is intended to "replace".

Thanks!

Comment thread sd/spicard.go
for i := 0; i < numblocks; i++ {
dataoff := i * blocksize
d.csEnable(true)
_, err := d.read_block_single(dst[dataoff:dataoff+blocksize], int64(i)+startBlockIdx)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-block read addressing bug for non-SDHC cards.
In ReadBlocks, after startBlockIdx <<= 9 for non-SDHC cards (converting block index to byte address), the multi-block loop passes int64(i) + startBlockIdx to read_block_single. This adds a block count to a byte address. It should be int64(i)*int64(blocksize) + startBlockIdx. SDHC (block-addressed) probably works correctly by accident.

Comment thread sd/responses.go
return (b & (1 << from)) >> from << to
}
// See 4.9.5 R6 (Published RCA response) of the SD Simplified Specification.
s := status(binary.BigEndian.Uint16(r.data[1:5]))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

binary.BigEndian.Uint16(r.data[1:5]) reads bytes 1–2 (the RCA field), not bytes 3–4 (the card status bits). Should be Uint16(r.data[3:5]).

Comment thread sd/definitions.go

// RateKilobits returns the transfer rate in kilobits per second.
func (t TransferSpeed) RateKilobits() int64 {
return 100 * log10table[t&0b111]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log10table has 7 entries (indices 0–6), but t&0b111 can be 7. Index 7 panics. The rate-unit value 7 is reserved in the spec but could appear on a misbehaving card.

Comment thread sd/definitions.go

// AccessTime returns the asynchronous part of the data access time.
func (t TAAC) AccessTime() (d time.Duration) {
return time.Duration(log10table[t&0b111]) * time.Nanosecond

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Comment thread sd/blockdevice.go

// ReadAt implements the [io.ReaderAt] interface for an SD card.
// Reads need not be aligned to block boundaries.
func (bd *BlockDevice) ReadAt(p []byte, off int64) (n int, err error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bounds checking against device size in this function.

Comment thread sd/blockdevice.go
// WriteAt implements the [io.WriterAt] interface for an SD card. Writes need
// not be aligned to block boundaries: partial blocks are read, modified and
// written back.
func (bd *BlockDevice) WriteAt(p []byte, off int64) (n int, err error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bounds checking against device size in this function either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants