Enforce correct SD state at compilation using a new type

[dcz-self]

The struct SdMmcSpi had two separate methods for initialization and deinitialization. It was up to the user not to mess them up at runtime.

A new BlockSpi struct takes over BlockDevice interface duties, making it impossible to use block procedures while the SD interface is in the wrong state.

I'm not 100% sure which methods should go to which interface, because that doesn't matter a lot for private ones.

I haven't updated the README or examples. I haven't tested how it works with the Controller (not interested in the file system right now).

The change in the API is breaking, but easy: instead of doing this:

let sd = SdMmcSpi::new(...);
let controller = Controller::new(sd);
controller.device.init();

now you do this:

let mut sd = SdMmcSpi::new(...);
let block = sd.acquire();
let controller = Controller::new(block);

The block device is deinitialized when dropped.

[thejpster]

I like this approach. I think it needs a 'cargo fmt' though (unless it's my phone making a mess of things).

[dcz-self]

I cargo fmted this.

[dcz-self]

I rebased to get rid of the extra commits and removed the "RFC" to reflect that.

[eldruin]

Thanks for your work!
The tests need to be adapted, though.

[eldruin]

At the moment this is fine since deinit cannot fail.
However, its docs already hint at flushing data if needed during deinit, which can fail.
The problem is that Drop cannot return an error so if we would add some data flushing during deinit, this Drop implementation would need to panic on error.
Have you thought about this?

[dcz-self]

That's a good observation, and I have one example of the same problem in the standard library:

https://doc.rust-lang.org/src/std/io/buffered/bufwriter.rs.html#668

Here, the error is dropped. It's not an amazing solution though, so I have another idea to the same effect: expose the BlockSpi inside a closure instead of in the "guard" pattern. Like this:

let result = sdmmcspi.acquire(|block: BlockSpi| { do_my_stuff() });

In this case, the result from acquire would contain the result of the final flush, e.g. Result<ClosureRet, FlushFailed>.

Closures can be nasty to use, so I see this more as an addition to the guard pattern than a replacement.

[dcz-self]

I have implemented that in 6005533 but it's not actually necessary as long as the SPI device deinit is infallible, so I didn't add that commit to this proposal.

[eldruin]

I see. It would need to be documented that the with_block method flushes, though.
However, the problem would still be there, although mitigated, if deinit did anything fallible, independently of whether the operations and their flush ran within a closure.
I cannot think of a solution to this, though. We would need rust-lang/rfcs#814

I still think this approach is more ergonomic than the status-quo.
Could you change the documentation of deinit to say that any flush must be done beforehand and deinit must always be kept infallible due to this drop?

[dcz-self]

Updated with more docs. I think now it's a bit split-brain, because I think deinit should be called manually before drops, but it also should attempt to flush, following the logic that a failed flush attempted always is better then not enforcing any flush at all.

[dcz-self]

Rebased and fixed tests.

[eldruin]

Ok, looks good to me, thanks!
Any further concerns @thejpster?

[thejpster]

LGTM

[thejpster]

Oh sorry, one thing - is the README now wrong?

Maybe we should compile-check the README...

[thejpster]

Also the Changelog in the README. We can do this separately if you want, as long as we don't leave it too long.

[thejpster]

Pinging @dcz-self - sorry we took so long to get back to you!

[dcz-self]

Thanks for reminding me. I've updated the readme and the changelog, and rebased.

[eldruin]

Looks good to me!
I just made a suggestion to the changelog entry.

[eldruin]

Thank you!