Fixed compilation error when FF_MAX_SS != FF_MIN_SS

[ccattuto]

When FF_MAX_SS != FF_MIN_SS, access to vfs->fatfs.ssize is needed to keep track of block size. However, the current code incorrectly refers to vfs->ssize rather than vfs->fatfs.ssize, resulting in a compilation error whenever MICROPY_FATFS_MAX_SS is set to anything different than FF_MIN_SS.

[jepler]

makes sense, thanks! CircuitPython doesn't currently exercise this code path in any official builds but it's nice to have the problem fixed.

[tannewt]

Why are you changing the block size? I'm worried other code may break if it isn't 512 bytes.

[ccattuto]

Why are you changing the block size? I'm worried other code may break if it isn't 512 bytes.

When I found this bug, I was experimenting with a board which has an additional external SPI flash memory (on SPI lines shared with other peripherals): I wanted to have a filesystem backed by that flash chip. I wrote a minimal class that implements the BlockDevice protocol on top of the SPI flash, and since the erase block for that flash chip is 4k, the simplest approach was to use a block size of 4k, which - at least in principle - is allowed by tweaking FF_MAX_SS.

I am happy to report that everything works just fine, at least in my case.

[jepler]

if we're not interested in taking a change that enables changing FF_MAX_SS then we should instead MP_STATIC_ASSERT it.

[jepler]

the point about flash chip sizes is an interesting one. right now we have several different implementations that do read-modify-write for exactly this reason. these have even proven to be buggy in the past (a recent stm32 issue I recall) could we drop those in favor of the implementation in oofatfs and have less code to write for ourselves?

[ccattuto]

Also, doing read-modify-write to have 512-bytes logical blocks on top of 4kB erase blocks comes with the cost of up to 8x the flash wear for sequential block writes, which are common. Unless one switches to littlefs or another filesystem designed to limit wear.

[tannewt]

if we're not interested in taking a change that enables changing FF_MAX_SS then we should instead MP_STATIC_ASSERT it.

I'm fine taking the change in but wanted to warn against it being untested.

the point about flash chip sizes is an interesting one. right now we have several different implementations that do read-modify-write for exactly this reason. these have even proven to be buggy in the past (a recent stm32 issue I recall) could we drop those in favor of the implementation in oofatfs and have less code to write for ourselves?

Maybe. It isn't exactly clear to me how oofastfs manages it. I've always intended to unify the 4k cache code across ports but never got around to it.

Also, doing read-modify-write to have 512-bytes logical blocks on top of 4kB erase blocks comes with the cost of up to 8x the flash wear for sequential block writes, which are common. Unless one switches to littlefs or another filesystem designed to limit wear.

Usually CP will cache the 4k block and write it back once writes move to another block. It does wear more than littlefs but we haven't seen many issues with it and USB MSC functionality is core to how CP works on most devices.