Skip to content

vfs: add littlefs v1 and v2 bindings #5228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Oct 29, 2019

Conversation

dpgeorge
Copy link
Member

This is a WIP, the next attempt to add VFS bindings for littlefs.

  • lfs1 and lfs2 are added together, verbatim to this repository in lib/littlefs/
  • support for both versions of littlefs can be built in the same time (currently the only option, but to be improved)
  • only enabled in unix coverage build at the moment, for testing purposes

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Oct 18, 2019
@dpgeorge dpgeorge force-pushed the extmod-add-littlefs-v2 branch 2 times, most recently from 39157c8 to 883951a Compare October 21, 2019 08:11
@dpgeorge
Copy link
Member Author

This is almost ready to go in now.

I found that the best way to extend the block protocol to support littlefs was to make minimal changes:

  • add an optional arg to readblocks and writeblocks for the offset into a block
  • add a new ioctl to erase a block
    So the extended block protocol is:
class AbstractBlockDev:
    def readblocks(self, block_num, buf, offset=None):
        # if offset is not None then it's a byte offset into the block to start reading from
    def writeblocks(self, block_num, buf, offset=None):
        # if offset is not None then it's a byte offset into the block to start writing to
        # AND in such a case the block should not be erased before writing
    def ioctl(self, op, arg):
        if op == 6: # erase block
            # arg is block number to erase

This approach is simple, requires no new methods, and can coexist with the original block protocol (so the same device can be used for both a FAT FS and littlefs, for example).

};

/*
size_t strspn(const char *s, const char *accept) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you define these with MP_WEAK then they'll be available for ports that don't have them built in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, probably better to put them in lib/libc/string0.c

mp_obj_array_t ar = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, len, buf};
self->readblocks[2] = MP_OBJ_NEW_SMALL_INT(block_num);
self->readblocks[3] = MP_OBJ_FROM_PTR(&ar);
self->readblocks[4] = MP_OBJ_NEW_SMALL_INT(block_off);
Copy link
Contributor

@andrewleech andrewleech Oct 22, 2019

Choose a reason for hiding this comment

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

This blockdev.readblocks offset argument is new isn't it, I don't think any of the other existing block device impl's include it so far?

I presume from this arg name it's intended to be an offset in units of blocks? The RAMBlockDevice implementation below looks like it's using it as an address bytes offset.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes offset is a new argument, only needed for littlefs.

It's an offset in bytes, within the block. So it can read/write smaller chunks than an erase block.

@dpgeorge dpgeorge force-pushed the extmod-add-littlefs-v2 branch from 274cf5d to 152d9ac Compare October 25, 2019 00:10
@dpgeorge dpgeorge changed the title WIP vfs: add littlefs v1 and v2 bindings vfs: add littlefs v1 and v2 bindings Oct 25, 2019
@dpgeorge
Copy link
Member Author

I cleaned up the commits here to make it ready to merge.

Final thing to do is test for any regressions of FAT FS on bare-metal ports.

@andrewleech
Copy link
Contributor

Do you know if anything is still using the old block device protocol with sync and count? Is it still necessary to keep that support?
I guess the code's already there, don't cost much size to have it.

@dpgeorge
Copy link
Member Author

Do you know if anything is still using the old block device protocol with sync and count? Is it still necessary to keep that support?

There may be user code out there. See #3861. Maybe in MicroPython 2.0 we can remove the old block device protocol...

@dmartauz
Copy link

Can this PR be used to create littlefs partition on ESP32? If so, could you please provide example code?

@andrewleech
Copy link
Contributor

@dmartauz not yet, this is a first step. It currently is only integrated into the Unix port. Integration into the other ports will come in follow up PR's.

@dmartauz
Copy link

@andrewleech Ok, thank you for an explanation.

@dpgeorge
Copy link
Member Author

The last thing to decide here is naming of the new VFS classes.

Existing classes are:

  • uos.VfsFat
  • uos.VfsPosix

Options for littlefs would be:

  • uos.VfsLfs1 and uos.VfsLfs2 -- short, matches existing classes
  • uos.VfsLFS1 and uos.VfsLFS2 -- uppercase version of above
  • uos.VfsLittle1 and uos.VfsLittle2 -- long version without "FS"
  • uos.VfsLittleFs1 and uos.VfsLittleFs2 -- long version with "FS"
  • uos.VfsLittleFS1 and uos.VfsLittleFS2 -- long version with uppercase "FS"

The last one is currently used in this PR. It's a bit long and cumbersome to type. I like the first option better (VfsLfs1): it's short, easy to type, matches existing classes, easy to tell what it is, and even though it's short it's not likely to clash with any other known filesystem.

So I'd suggest to use VfsLfs1 and VfsLfs2.

@andrewleech
Copy link
Contributor

My vote is also for VfsLfs1 and VfsLfs2.

Lfs is already a common/standard shortening for littlefs so I funny think it's likely to cause any confusion. Shorter is easier to type and takes less bytes of qstr.

@dpgeorge dpgeorge force-pushed the extmod-add-littlefs-v2 branch from 152d9ac to 06663b7 Compare October 26, 2019 23:38
@dpgeorge
Copy link
Member Author

Ok, rebased and pushed with name changed to VfsLfs1 and VfsLfs2.

@andrewleech
Copy link
Contributor

It would be good to have a brief description of the littlefs library renaming process used to bring in the two copies. to make it quicker/easier next time someone wants to update them. Even just a couple of lines of a link to the instructions in the respective commit messages?

@andrewleech
Copy link
Contributor

On a related note, with the extended block device interface:

  • add an optional arg to readblocks and writeblocks for the offset into a block

Would you prefer to see existing block drivers updated to expect the offset arg, or support the new argument as optional?
eg.
current

STATIC mp_obj_t pyb_flash_readblocks(mp_obj_t self, mp_obj_t block_num, mp_obj_t buf) {
...
}
STATIC MP_DEFINE_CONST_FUN_OBJ_3(pyb_flash_readblocks_obj, pyb_flash_readblocks);

new
expect new arg:

STATIC mp_obj_t pyb_flash_readblocks(mp_obj_t self, mp_obj_t block_num, mp_obj_t buf, mp_obj_t offset) {
...
}
STATIC MP_DEFINE_CONST_FUN_OBJ_4(pyb_flash_readblocks_obj, pyb_flash_readblocks);

or optional new arg:

STATIC mp_obj_t pyb_flash_readblocks(size_t n_args, const mp_obj_t *args) {
    mp_obj_base_t *self = (mp_obj_base_t*)MP_OBJ_TO_PTR(args[0]);
    mp_int_t block_num = mp_obj_get_int(args[1]);
    mp_obj_t buf = MP_OBJ_TO_PTR(args[2]);
    if (n_args == 4) { ... }
...
}
STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(pyb_flash_readblocks_obj, 3, 4, pyb_flash_readblocks);

@dpgeorge
Copy link
Member Author

It would be good to have a brief description of the littlefs library renaming process used to bring in the two copies.

Good idea. I pushed a commit here that adds a README.md to the lib/littlefs directory describing the process.

@dpgeorge dpgeorge force-pushed the extmod-add-littlefs-v2 branch from c61b59f to 16b53e4 Compare October 27, 2019 09:23
@dpgeorge
Copy link
Member Author

Would you prefer to see existing block drivers updated to expect the offset arg, or support the new argument as optional?

Existing code can stay as-is, everything is backwards compatible. If a block dev only wants to support FAT FS (and similar) then it just needs to implement the standard protocol (without the new offset arg). If a block dev only wants to support littlefs (and similar) then it only needs to implement the 4-arg version of the read/write blocks methods. OTOH, if a block dev wants to support both kinds of filesystems then it must implement the methods with an optional 4th argument.

I've added a new test extmod/vfs_blockdev.py to show how, at the Python level, a block dev can be implemented which supports bth standard and extended protocols at the same time (so 4th arg is optional).

This commit adds helper functions to call readblocks/writeblocks with a
fourth argument, the byte offset within a block.

Although the mp_vfs_blockdev_t struct has grown here by 2 machine words, in
all current uses of this struct within this repository it still fits within
the same number of GC blocks.
@dpgeorge dpgeorge force-pushed the extmod-add-littlefs-v2 branch from 16b53e4 to f0a44a3 Compare October 29, 2019 01:55
@dpgeorge dpgeorge force-pushed the extmod-add-littlefs-v2 branch from f0a44a3 to 7a24b7f Compare October 29, 2019 03:17
@dpgeorge dpgeorge merged commit 7a24b7f into micropython:master Oct 29, 2019
@dpgeorge dpgeorge deleted the extmod-add-littlefs-v2 branch October 29, 2019 04:18
@dpgeorge
Copy link
Member Author

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants