Make functions that modify underlying state through SPI receive &mut self instead of &self

[datdenkikniet]

Currently, BlockDevice::read and BlockDevice::write (and practically all functions in SdMmcSpi and BlockSpi) receive &self.

Why is this done?

In my opinion, they should receive &mut self, so that responsibility of choosing what method of resource sharing a user wants is up to the user. Issues such as #37 could easily be avoided, and would also make more sense logically: writeing something is clearly modifying something, so why does it not need mutable access?

Especially in embedded settings (which this crate is intended for), where being interrupted while having a borrow is quite possible, I don't quite understand why this design choice was made.

I'm willing to contribute a design proposal and PRs to make this change, if it is deemed that this change makes sense.

This issue is, in essence, the same as #8, but for slightly different reasons. I hope that the reasons mentioned there may have disappeared in the mean time (I can't find a concrete example of why it was dismissed back then). If these issues persist, I would propose that we clearly document that SdMmcSpi makes use of RefCells internally.

[datdenkikniet]

I also looked through some of the dependents on crates.io:

The only implementor of BlockDevice that I could find was in stm32h7xx-hal (the rest use it only in examples), and that uses the same RefCell hack to get around the non-mutability problem

[datdenkikniet]

I'm working on an alternative implementation here:

https://github.com/datdenkikniet/embedded-sdmmc-rs/tree/no_more_immutable

[eldruin]

Have you looked through the closed issues? There have been related discussions in the past.

[datdenkikniet]

I tried my best to find similar issues, and linked some of them.

However, in those issues the reasoning given is that it would be impossible/very impractical to have multiple file systems on a single block device.

I don't understand why the choice was made to make BlockDevice take &self, effectively forcing implementors of that trait to choose some form of synchronization primitive or runtime-borrowchecked type. Why doesn't the trait take &mut self (since it effectively always requires that anyways. Perhaps not for reading, but it sounds rather counterintuitive to say that a write function somehow does not require to mutate the state of the block device), and delegate decisions regarding the synchronization of accesses to the BlockDevice to a higher layer (i.e. the MBR, file system, or even to the end user, depending on if they choose to wrap the BlockDevice in a RefCell or not).

For an example of what I mean, see for instance here and here. Instead of forcing the user to accept the usage of a RefCell in the SdMmcSpi, the user gets to choose the type of of BlockDevice that the Mbr uses (either a plain SdMmcSpi, or RefCell<SdMmcSpi> if they wish to have multiple partitions open at the same time), and allows them to open multiple partitions at once if the underlying BlockDevice supports it.

I'm trying to understand why the design choice was made (presumably because it is definitely the most straightforward: users of the library don't have to think about how it works), and why (if for any specific reason at all) changing it would be off the table.

[eldruin]

Maybe @thejpster has some further reasoning for this interface choice?

[datdenkikniet]

Yeah, that's what I was hoping for too :)

[thejpster]

So I haven't thought about this crate in a long time.

My recollection is that a function like read_data doesn't mutate the underlying blocks on disk. So it seemed reasonable to only require &self and allow multiple references to that block device exist in the system simulataneously.

The std::fs::File type doesn't require an &mut Kernel reference to operate - it all just works because the kernel handles mutability internally.

If you'd like to submit a PR that removes the RefCell and makes the user deal with &mut refs everywhere, please do feel free - provided they are accompanied by examples that demonstrate that we haven't made the library impractical to use. Remember, the use case was "kinda do what the Arduino SdFat library does" - not writing operating systems, or making a more general purpose FAT implementation.

[datdenkikniet]

I see, thanks for the info!

I'm cooking up a little something, and can ping again here once it's more or less done, and if it seems like the API can be on a similar level of user-friendliness.

So far I've finished implementing reading files and directories, but need to figure out how to do writing and reading at some point while staying ergonomic.

[datdenkikniet]

I never did finish my work on this, and with #79 it may not make any sense at all :)

I'm closing this for now