Add BlockDevice trait.

[qwandor]

Fixes #56.

[Sympatron]

Shouldn't BLOCK_SIZE be an associated constant?

[qwandor]

Shouldn't BLOCK_SIZE be an associated constant?

I tried that but unfortunately it doesn't work, as Rust doesn't allow associated constants to be used in the types of trait methods. We can make it a trait parameter though.

In practice the crates I've checked so far both use 512 byte blocks. virtio-drivers uses 512 byte blocks for VirtIO block devices (as required by the VirtIO specification), embedded-sdmmc uses 512 blocks for SD cards and FAT32. Implementing block devices with other block sizes is less useful, as they won't be able to be used with filesystem drivers which expect 512 byte blocks. (On the other hand filesystems which work with larger blocks can be implemented just fine against a device which has 512 byte blocks, as they can just read or write multiple blocks at once if needed.)

[qwandor]

Any further comments on this? Can it be merged?

[thejpster]

I'm fine with just adding Block in the first instance, which is a [u8; 512] and then having the BlockDevice trait not be generic over block size. The set of things that don't have 512 byte blocks (especially in an embedded context) is very small, and most of those will simply be a [u8; 4096], which we can add as a LargeBlock or Block4K.

[qwandor]

@thejpster What do you think of the current approach of the block size being a const parameter to the trait? I think this is is a decent compromise, as the default value makes the usual case easy, while still keeping the flexibility to implement other block sizes if required. It also means that users of the trait can choose to be generic over the block size if they want to.

[thejpster]

I'd like to see a PR for embedded-sdmmc that uses this new trait to know for sure. I guess it's going to use BlockDevice<512>.

Edit: the lack of a Block type might be a problem...

[thejpster]

It's a shame to loose BlockIdx and BlockCount that embedded-sdmmc had, and to go back to raw u64 values that you can easily get mixed up.

I think I'd also prefer to use 32 bit values for the count and the index, which lets you have 2**32 sectors, which is 2 TiB. That seems fine for almost any value of 'embedded'.

[qwandor]

BlockIdx and BlockCount seem like unneccessary extra complexity to me, but I can add them here if you really want.

I do definitely think we should use u64 for block indices though. Other places where I'd like to use this trait already use u64 for block indices, such as the virtio_drivers VirtIO block device driver. I disagree that 2 TiB is enough; embedded doesn't just include microcontrollers, it also includes things like writing a bootloader for a storage server, writing a new desktop OS kernel, or writing a unikernel to run in a VM.

[thejpster]

OK, so I'll accept u64 as long as we have BlockCount(u64) and BlockIdx(u64).

The reason for having both those two types is the same as for having std::time::Instant and std::time::Duration - to stop you adding two Instants together by accident.

And the reason for having either of them is to stop you accidentally sticking an integer into a function argument.

[qwandor]

Done, and updated rust-embedded-community/embedded-sdmmc-rs#162 to match.

[thejpster]

Looking good.

Please can we put the arguments the other way around, like they were in embedded-sdmmc? Just to reduce the diff.

Also - should we require that BlockDevice::Error: Debug? It would reduce a bunch of trait bounds needed by anyone who wants to be able to print or unwrap the error. Maybe optionally require defmt::Format too?

[qwandor]

I've swapped the order of parameters. I'd rather not constrain the error type in the trait, as I've seen a fair few embedded driver crates whose error types don't implement Debug. Consumers of the trait can require the error type to implement Debug if they really need that, but I don't think it's necessarily required and I'd like to keep the trait as broadly applicable as possible.

[d3zd3z]

Shouldn't BLOCK_SIZE be an associated constant?

What about devices, such as SPI where these parameters are probed. I would argue that none of the parameters should be associated constants.

[thejpster]

Wait, what SPI device requires you to probe for the sector size? This is an abstraction for hard disks, floppy disks, SD Cards and NVMe devices, and all use either 512 bytes or maybe sometimes 4096 bytes as the sector size - and on an embedded system you will know in advance which one you want to deal with.

[thejpster]

@MabezDev points out that https://docs.rs/block-device-driver/0.2.0/block_device_driver/trait.BlockDevice.html exists (it's an async version of basically this trait). We should either bring the async version over here, or we should scrap this and send a sync version over there. It doesn't seem to make sense to have the two versions in two different crates.