package: add generic FatFs#6072
Conversation
|
There was FAT support in former RIOT versions, but we decided to kick it out at some point. IIRC the main reasons were: Have you considered to use another FS that is more suited for MCUs and flash storage? Maybe you can take a look at the on-going discussions with respect to file systems: https://github.com/RIOT-OS/RIOT/search?q=fs&type=Issues |
OlegHahm
added
Type: new feature
Hmm can you give me a pointer to where that was discussed? The only related info I could find about that in the RIOT context is within #2474 where @x3ro stats "custom license compatible to BSD"
True, but I'd agree to comments by @miri64 like this one or by @gebart : "FAT would be useful for SD card storage, which provide internal wear levelling, but it would probably wear out simpler flash chips too quickly to be useful on those" (found here).
Yep! Nevertheless some guys ( @alignan @smlng I'm pointing at you :P) were eager to try it out. |
|
Nope, wrong pointer. It might be that this discussion was in pre-RIOT, pre-Github days. I will see if I can find something. @kaspar030, do you remember? |
True, but in this case I don't see the need for any file system. However, don't get me wrong: I'm not in general opposed to this PR. Personally, I'm still not convinced that a file system is really needed on these systems, but that's another discussion. But if we want to support file systems, we should have a clear understanding of what and how we want to integrate it into RIOT. |
|
The only license issue I could find was related to long file names and a MS patent, but that doesn't seem to be a problem: http://elm-chan.org/fsw/ff/en/appnote.html#lfn Maybe I remember wrong, but I will still try to see if anyone else remembers. |
The point of using a micro-SD instead of a flash memory is to allow the information to be retrieved and read externally, without having to use a specific test/feature such as dumping the memory content over USB, etc (which would be huge). Also, in cases external configuration information has to be written to the microSD cards, it would be easier just to clone a card. I'm not particularly fixed to FatFS as the solution, but I do see a need for a file system for the Micro-SD |
I see use cases for a memory card, but for simple configuration data or log information, one doesn't need a filesystem. Cloning a card would be done with something like But let's not discuss this here. 😉 |
What a user friendly way to get a bitmap to your device ;-) |
Sure. I simply did it for ease of use and the fact that i can manipulate files if needed - with the implication that I may need to think a bit more about how to use the FS.
I agree. Its not "really" needed as in "one could always implement/port a specific fs if needed". But also it could be of great help for someone that just wants to store/load data in an interoperable way and wants to get started quickly without reinventing the weel on how to store the data on the bare media.
Agree. And I'm totally open minded to any kind of suggestions to tackle this :)
ACK! Would also like to see a better fs without all the shortcomings. For me the key thing is: FatFs is actually usable in it's current state and its dead simple (from the user POV). FAT has it's place and may still be there for longer than we'd all prefer so why not provide compatibility... |
AFAIR the guy doing FireKernel's FAT support didn't use chan or (had to) heavily modify it, so we kicked it out as unmaintainable. I'd like to see FAT for RIOT, just because it's the common denominator all OS can read. Wear leveling shouldn't be a problem with SD cards as they deal with that internally. |
I just checked: the latest version of FireKernel used ChaN FatFs R0.08a. I haven't run a diff versus the upstream version.
It is so sad that in 2016 we still have to use FAT as common FS, but yeah, you're kind of right. However, it's interesting how drastically you've changed your mind: #1265 (comment) 😉 |
Yup. Back then I remembered the horrors Michael went through to get FAT working. Now it seems as ChaN's FatFS is actually usable. ;) |
|
@MichelRottleuthner Have you considered basing this on #5616? |
|
Yes I think that would make sence but I haven't looked into the details so I'm not exactly sure what is needed to make this work with vfs. |
|
@cgundogan @smlng you can now also use FatFs on native see README.md for details. |
|
@MichelRottleuthner 👍 I will look into the native tests later today. Do you mind splitting the |
As expressed in the opening comment I'd like to do that anyway. The question that remains open for discussion is: how (i.e. where would be the best place to put this code)? |
| * | ||
| * @} | ||
| */ | ||
| #define ENABLE_DEBUG (1) |
There was a problem hiding this comment.
please define all ENABLE_DEBUGs as (0)
| { | ||
| if(idx < sizeof(volume_files) / sizeof(dummy_volume_t)){ | ||
| return &volume_files[idx]; | ||
| }else{ |
There was a problem hiding this comment.
else statements should go to a new line
|
|
||
| static dummy_volume_t volume_files[] = { | ||
| { | ||
| .image_path = "../../riot_fatfs_disk.img", |
There was a problem hiding this comment.
this should be parameterized somehow
| { | ||
| dummy_volume_t *volume = get_volume_file(pdrv); | ||
|
|
||
| if(volume != NULL && volume->opened){ |
There was a problem hiding this comment.
put parens around separate conditions
| no longer used (needed at _USE_TRIM == 1) */ | ||
| #define CTRL_TRIM 4 | ||
|
|
||
| #define RTC_YEAR_OFFSET 1900 |
There was a problem hiding this comment.
many of these macros are defined multiple times. can you extract them into a common h file?
| } | ||
|
|
||
| #ifdef FATFS_RTC_AVAILABLE | ||
| DWORD get_fattime (void){ |
There was a problem hiding this comment.
seems to be the same function as above. would make sense to put all common functions / macros into a common file
There was a problem hiding this comment.
| * @return 0 if disk is initialized | ||
| * @return STA_NOINIT if disk id exists, but disk isn't initialized | ||
| */ | ||
| DSTATUS disk_status (BYTE pdrv) |
|
@PeterKietzmann I think @MichelRottleuthner should whitelist supported boards with SPI as defined in pkg test Makefile |
|
plus dependence on #6031 |
|
@MichelRottleuthner please rebase. |
miri64
removed
the
State: waiting for other PR
|
VFS (#5616) was merged, so if you want to you can add a wrapper for that, but given that this PR was already ACK'd, I would say we do this as a follow-up ;-) |
|
Was approved, but needs rebase and some fixing ;-) |
| @@ -0,0 +1,2 @@ | |||
| MODULE = fatfs_diskio | |||
| include $(RIOTBASE)/Makefile.base | |||
There was a problem hiding this comment.
If this module is fatfs specific: why is it in sys and not pkg/fatfs/? Please move it (sorry for ringing in so late, but just noticed).
MichelRottleuthner
force-pushed
the
fatfs_testing
branch
from
78fd1f2 to
2c9fd89
Compare
kaspar030
added
the
CI: needs squashing
MichelRottleuthner
force-pushed
the
fatfs_testing
branch
from
e2aee08 to
9bb9a9a
Compare
MichelRottleuthner
force-pushed
the
fatfs_testing
branch
from
40998f8 to
e0f60ba
Compare
|
squashed. |
BytesGalore
removed
the
CI: needs squashing
…of authors webserver, added Makefile.dep for fatfs, cleanup code for using rtc
| ifneq (,$(filter tcp,$(USEMODULE))) | ||
| DIRS += net/transport_layer/tcp | ||
| endif | ||
|
|
There was a problem hiding this comment.
yep - seems like I couldn't resist removing the asymmetry in this file when moving the diskio stuff^^
There was a problem hiding this comment.
Are you removing the patch or shall I merge?
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
Minor nitpick, but I'm not blocking this PR because of this.
|
Press merge at will. |
|
beside the unrelated newline strips, looks good to me -> ACK'n'go |
11 participants
This PR adds a package for a generic FAT file system. As it's a RIOT package it mainly contains a makefile that can be used to fetch the code from the original authors page.
Beyond this the PR also includes two FatFs abstraction layer implementations. One for the sdcard_spi driver and one that wraps around file-io systemcalls that makes it fairly easy to use it with FAT-images under native (see README for details).
Also included is a simple interactive test application. Currently with the test app you can mount your volume, list, read, create and write to files.
One thing I'd like to discuss about this thing: Since the FatFs package is generally neither bound to a specific media (like sdcard in this case) nor is the sdcard bound to be only used with FatFs - what would be the best and "RIOT-like" way to provide the abstraction layer (diskio.c)?. For now I've included it to this PR to make testing easier but I think it should be moved somewhere else.
Waiting for 6031 so that building with sdcard_spi_diskio is possible.