From 5fb07e89b49842fdd4b8a56469e676d716396b48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Wed, 21 Feb 2018 09:29:51 +0100 Subject: [PATCH 1/6] pkg/fatfs: refactor absolute path handling --- pkg/fatfs/fatfs_vfs/fatfs_vfs.c | 48 +++++++++++++++++---------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/pkg/fatfs/fatfs_vfs/fatfs_vfs.c b/pkg/fatfs/fatfs_vfs/fatfs_vfs.c index 4f49d3c43f..5234fc1563 100644 --- a/pkg/fatfs/fatfs_vfs/fatfs_vfs.c +++ b/pkg/fatfs/fatfs_vfs/fatfs_vfs.c @@ -36,6 +36,17 @@ static int fatfs_err_to_errno(int32_t err); static void _fatfs_time_to_timespec(WORD fdate, WORD ftime, time_t *time); +/** + * @brief Concatenate drive number and path into the buffer provided by fs_desc + * + * Most FatFs library file operations need an absolute path. + */ +static void _build_abs_path(fatfs_desc_t *fs_desc, const char *name) +{ + snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%u:/%s", + fs_desc->vol_idx, name); +} + static int _mount(vfs_mount_t *mountp) { /* if one of the lines below fail to compile you probably need to adjust @@ -45,13 +56,12 @@ static int _mount(vfs_mount_t *mountp) fatfs_desc_t *fs_desc = (fatfs_desc_t *)mountp->private_data; - char volume_str[FATFS_MAX_VOL_STR_LEN]; - snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx); + _build_abs_path(fs_desc, ""); memset(&fs_desc->fat_fs, 0, sizeof(fs_desc->fat_fs)); - DEBUG("mounting file system of volume '%s'\n", volume_str); - FRESULT res = f_mount(&fs_desc->fat_fs, volume_str, 1); + DEBUG("mounting file system of volume '%s'\n", fs_desc->abs_path_str_buff); + FRESULT res = f_mount(&fs_desc->fat_fs, fs_desc->abs_path_str_buff, 1); if (res == FR_OK) { DEBUG("[OK]"); @@ -69,11 +79,10 @@ static int _umount(vfs_mount_t *mountp) DEBUG("fatfs_vfs.c: _umount: private_data = %p\n", mountp->private_data); - char volume_str[FATFS_MAX_VOL_STR_LEN]; - snprintf(volume_str, sizeof(volume_str), "%d:/", fs_desc->vol_idx); + _build_abs_path(fs_desc, ""); - DEBUG("unmounting file system of volume '%s'\n", volume_str); - FRESULT res = f_unmount(volume_str); + DEBUG("unmounting file system of volume '%s'\n", fs_desc->abs_path_str_buff); + FRESULT res = f_unmount(fs_desc->abs_path_str_buff); if (res == FR_OK) { DEBUG("[OK]"); @@ -90,8 +99,7 @@ static int _unlink(vfs_mount_t *mountp, const char *name) { fatfs_desc_t *fs_desc = (fatfs_desc_t *)mountp->private_data; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, name); + _build_abs_path(fs_desc, name); return fatfs_err_to_errno(f_unlink(fs_desc->abs_path_str_buff)); } @@ -102,10 +110,9 @@ static int _rename(vfs_mount_t *mountp, const char *from_path, char fatfs_abs_path_to[FATFS_MAX_ABS_PATH_SIZE]; fatfs_desc_t *fs_desc = (fatfs_desc_t *)mountp->private_data; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, from_path); + _build_abs_path(fs_desc, from_path); - snprintf(fatfs_abs_path_to, sizeof(fatfs_abs_path_to), "%d:/%s", + snprintf(fatfs_abs_path_to, sizeof(fatfs_abs_path_to), "%u:/%s", fs_desc->vol_idx, to_path); return fatfs_err_to_errno(f_rename(fs_desc->abs_path_str_buff, @@ -117,8 +124,7 @@ static int _open(vfs_file_t *filp, const char *name, int flags, mode_t mode, { fatfs_file_desc_t *fd = (fatfs_file_desc_t *)&filp->private_data.buffer[0]; fatfs_desc_t *fs_desc = (fatfs_desc_t *)filp->mp->private_data; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, name); + _build_abs_path(fs_desc, name); (void) abs_path; (void) mode; /* fatfs can't use mode param with f_open*/ @@ -251,8 +257,7 @@ static int _fstat(vfs_file_t *filp, struct stat *buf) FILINFO fi; FRESULT res; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, fd->fname); + _build_abs_path(fs_desc, fd->fname); memset(buf, 0, sizeof(*buf)); @@ -295,8 +300,7 @@ static int _opendir(vfs_DIR *dirp, const char *dirname, const char *abs_path) fatfs_desc_t *fs_desc = (fatfs_desc_t *)dirp->mp->private_data; (void) abs_path; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, dirname); + _build_abs_path(fs_desc, dirname); return fatfs_err_to_errno(f_opendir(dir, fs_desc->abs_path_str_buff)); } @@ -334,8 +338,7 @@ static int _mkdir (vfs_mount_t *mountp, const char *name, mode_t mode) fatfs_desc_t *fs_desc = (fatfs_desc_t *)mountp->private_data; (void) mode; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, name); + _build_abs_path(fs_desc, name); return fatfs_err_to_errno(f_mkdir(fs_desc->abs_path_str_buff)); } @@ -344,8 +347,7 @@ static int _rmdir (vfs_mount_t *mountp, const char *name) { fatfs_desc_t *fs_desc = (fatfs_desc_t *)mountp->private_data; - snprintf(fs_desc->abs_path_str_buff, FATFS_MAX_ABS_PATH_SIZE, "%d:/%s", - fs_desc->vol_idx, name); + _build_abs_path(fs_desc, name); return fatfs_err_to_errno(f_unlink(fs_desc->abs_path_str_buff)); } From 0251f54e6684bd84c096218a5a182866254a8488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Wed, 21 Feb 2018 09:41:26 +0100 Subject: [PATCH 2/6] pkg/fatfs: Remove unused FATFS_DIR_SIZE, FATFS_FILE_SIZE --- sys/include/fs/fatfs.h | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/sys/include/fs/fatfs.h b/sys/include/fs/fatfs.h index f29725c25e..ef959f7d45 100644 --- a/sys/include/fs/fatfs.h +++ b/sys/include/fs/fatfs.h @@ -35,15 +35,6 @@ extern "C" { /** The epoch offset is used to convert between FatFs and time_t timestamps */ #define EPOCH_YEAR_OFFSET (1970) -/** Size of the buffer needed for a directory entry -> @attention this should be: sizeof(DIR). - sizeof(DIR) currently isn't used directly because it's not possible to use that within - preprocessor-if (see below) */ -#define FATFS_DIR_SIZE (44) - -/** Size of the buffer needed for directory - -> should be: sizeof(fatfs_file_desc_t) */ -#define FATFS_FILE_SIZE (72) - /** size needed for volume strings like "n:/" where n is the volume id */ #define FATFS_MAX_VOL_STR_LEN (4) @@ -54,14 +45,6 @@ extern "C" { needed buffer to circumvent stack allocation within vfs-wrappers */ #define FATFS_MAX_ABS_PATH_SIZE (FATFS_MAX_VOL_STR_LEN + VFS_NAME_MAX + 1) -#if (VFS_DIR_BUFFER_SIZE < FATFS_DIR_SIZE) -#error "VFS_DIR_BUFFER_SIZE too small" -#endif - -#if (VFS_FILE_BUFFER_SIZE < FATFS_FILE_SIZE) -#error "VFS_FILE_BUFFER_SIZE too small" -#endif - /** * needed info to run a FatFs instance */ From f8a9e6303f040596410d6de7323a75cc40a2934e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Wed, 21 Feb 2018 09:41:57 +0100 Subject: [PATCH 3/6] tests/pkg_fatfs_vfs: Word wrap README --- tests/pkg_fatfs_vfs/README.md | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/tests/pkg_fatfs_vfs/README.md b/tests/pkg_fatfs_vfs/README.md index b1671ae522..3c5995c734 100644 --- a/tests/pkg_fatfs_vfs/README.md +++ b/tests/pkg_fatfs_vfs/README.md @@ -3,18 +3,30 @@ Using FatFs (with VFS) on RIOT # native -To use this test on native you can either use a FAT-formatted image file or directly use the mkfs command from the RIOT shell. -Use `make image` to extract a prepared image file that already contains a simple test.txt file. -This is only a convinience function to allow testing against a "default linux" formatted fat volume without the need to call mount or other stuff that may require super user privileges. -Optionally `make compressed-image` can be used to generate the compressed image that is in turn used by `make image`. +To use this test on native you can either use a FAT-formatted image file or +directly use the mkfs command from the RIOT shell. Use `make image` to extract +a prepared image file that already contains a simple test.txt file. This is +only a convinience function to allow testing against a "default linux" +formatted fat volume without the need to call mount or other stuff that may +require super user privileges. Optionally `make compressed-image` can be used +to generate the compressed image that is in turn used by `make image`. -To tell RIOT where your image file is located you can use the define `MTD_NATIVE_FILENAME`. +To tell RIOT where your image file is located you can use the define +`MTD_NATIVE_FILENAME`. - NOTE: You shouldn't leave the image mounted while you use it in RIOT, the abstraction layer between FatFs and the image file mimics a dumb block device (i.e. behaves much like the devices that are actually meant to be used with FAT) That implies it doesn't show any modifications in RIOT that you perform on your OS and the other way round. So always remember to mount/unmount correctly or your FS will probably get damaged. +NOTE: You shouldn't leave the image mounted while you use it in RIOT, the +abstraction layer between FatFs and the image file mimics a dumb block device +(i.e. behaves much like the devices that are actually meant to be used with +FAT) That implies it doesn't show any modifications in RIOT that you perform on +your OS and the other way round. So always remember to mount/unmount correctly +or your FS will probably get damaged. # Real Hardware -Currently the test defaults to sdcard_spi on real hardware. But generally any device that supports the mtd-interface can be used with FatFs. -To use the automated test in pkg_fatfs_vfs you need to copy the generated image to your storage device (e.g. your SD-card). -To copy the image onto the card you can use something like `make image && dd if=bin/riot_fatfs_disk.img of=/dev/`. -After that you can connect the card to your RIOT device and check the test output via terminal. +Currently the test defaults to sdcard_spi on real hardware. But generally any +device that supports the mtd-interface can be used with FatFs. To use the +automated test in pkg_fatfs_vfs you need to copy the generated image to your +storage device (e.g. your SD-card). To copy the image onto the card you can use +something like `make image && dd if=bin/riot_fatfs_disk.img +of=/dev/`. After that you can connect the card to your RIOT device +and check the test output via terminal. From 07356143762af36b0855c9a81b6db66a68c46c4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Wed, 21 Feb 2018 09:45:13 +0100 Subject: [PATCH 4/6] tests/pkg_fatfs: Word wrap README --- tests/pkg_fatfs/README.md | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/pkg_fatfs/README.md b/tests/pkg_fatfs/README.md index 12f4ad1e5d..21fdf806e9 100644 --- a/tests/pkg_fatfs/README.md +++ b/tests/pkg_fatfs/README.md @@ -3,15 +3,25 @@ Using FatFs on RIOT # native -To use this test on native you can either use a FAT-formatted image file or directly use the mkfs command from the RIOT shell. -Use `make image` to extract a prepared image file that already contains a simple test.txt file. -This is only a convinience function to allow testing against a "default linux" formatted fat volume without the need to call mount or other stuff that may require super user privileges. -Optionally `make compressed-image` can be used to generate the compressed image that is in turn used by `make image`. +To use this test on native you can either use a FAT-formatted image file or +directly use the mkfs command from the RIOT shell. Use `make image` to extract +a prepared image file that already contains a simple test.txt file. This is +only a convinience function to allow testing against a "default linux" +formatted fat volume without the need to call mount or other stuff that may +require super user privileges. Optionally `make compressed-image` can be used +to generate the compressed image that is in turn used by `make image`. -To tell RIOT where your image file is located you can use the define `MTD_NATIVE_FILENAME`. +To tell RIOT where your image file is located you can use the define +`MTD_NATIVE_FILENAME`. - NOTE: You shouldn't leave the image mounted while you use it in RIOT, the abstraction layer between FatFs and the image file mimics a dumb block device (i.e. behaves much like the devices that are actually meant to be used with FAT) That implies it doesn't show any modifications in RIOT that you perform on your OS and the other way round. So always remember to mount/unmount correctly or your FS will probably get damaged. +NOTE: You shouldn't leave the image mounted while you use it in RIOT, the +abstraction layer between FatFs and the image file mimics a dumb block device +(i.e. behaves much like the devices that are actually meant to be used with +FAT) That implies it doesn't show any modifications in RIOT that you perform on +your OS and the other way round. So always remember to mount/unmount correctly +or your FS will probably get damaged. # Real Hardware -Currently the test defaults to sdcard_spi on real hardware. But generally any device that supports the mtd-interface can be used with FatFs. +Currently the test defaults to sdcard_spi on real hardware. But generally any +device that supports the mtd-interface can be used with FatFs. From 4ce675e181aa08dc847be433f3a4c6a2c5d5905c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Wed, 21 Feb 2018 09:49:43 +0100 Subject: [PATCH 5/6] fs/fatfs: Adjust FATFS_MAX_VOL_STR_LEN to avoid overflows Prevents compile time warnings about truncation of the format string for fs_idx > 9 --- sys/include/fs/fatfs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/include/fs/fatfs.h b/sys/include/fs/fatfs.h index ef959f7d45..37b5703b5b 100644 --- a/sys/include/fs/fatfs.h +++ b/sys/include/fs/fatfs.h @@ -36,7 +36,7 @@ extern "C" { #define EPOCH_YEAR_OFFSET (1970) /** size needed for volume strings like "n:/" where n is the volume id */ -#define FATFS_MAX_VOL_STR_LEN (4) +#define FATFS_MAX_VOL_STR_LEN (6) /** 0:mount on first file access, 1 mount in f_mount() call */ #define FATFS_MOUNT_OPT (1) From 0bc2c1b2a9505cb5577831a0f327707a90aa7957 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Thu, 22 Feb 2018 08:04:09 +0100 Subject: [PATCH 6/6] fs/fatfs: Editorial changes in Doxygen comments --- sys/include/fs/fatfs.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/sys/include/fs/fatfs.h b/sys/include/fs/fatfs.h index 37b5703b5b..4ffb440a6f 100644 --- a/sys/include/fs/fatfs.h +++ b/sys/include/fs/fatfs.h @@ -41,12 +41,16 @@ extern "C" { /** 0:mount on first file access, 1 mount in f_mount() call */ #define FATFS_MOUNT_OPT (1) -/** most FatFs file operations need an absolute path. This defines the size of the - needed buffer to circumvent stack allocation within vfs-wrappers */ +/** + * @brief Size of path buffer for absolute paths + * + * Most FatFs file operations need an absolute path. This defines the size of + * the needed buffer to circumvent stack allocation within vfs-wrappers + */ #define FATFS_MAX_ABS_PATH_SIZE (FATFS_MAX_VOL_STR_LEN + VFS_NAME_MAX + 1) /** - * needed info to run a FatFs instance + * @brief FatFs instance descriptor */ typedef struct fatfs_desc { FATFS fat_fs; /**< FatFs work area needed for each volume */ @@ -58,7 +62,7 @@ typedef struct fatfs_desc { } fatfs_desc_t; /** - * info of a single opened file + * @brief FatFs file instance descriptor */ typedef struct fatfs_file_desc { FIL file; /**< FatFs work area for a single file */