mtd flash: add a generic low level flash interface [RFC] by vincent-d · Pull Request #5624 · RIOT-OS/RIOT

vincent-d · 2016-07-12T15:07:48Z

This PR adds a low level flash interface and a basic dummy flash implementation for the native cpu.

Comments are welcomed.

LudwigKnuepfer · 2016-07-12T19:54:18Z

Please compare with #2239 and fuse the efforts.

LudwigKnuepfer · 2016-07-12T19:57:04Z

OK, I'm refraining from code review until #5624 (comment) is addressed.

jnohlgard · 2016-07-14T04:28:51Z

drivers/include/mtdi.h

+ */
+typedef enum {
+    MTDI_STA_INIT    = 0x00, /**< Drive initialized */
+    MTDI_STA_NOINIT  = 0x01, /**< Drive not initialized */


IMO it's better to have noinit = 0, because then the initial value will be set to "not initialized" on boot even if placed in .bss

jnohlgard · 2016-07-14T06:27:21Z

The native MTD emulation device needs to do some bounds checking on the addresses and sizes to avoid writing outside the emulated memory. The fwrite will succeed and the file will grow, but that won't work when you move it to a real MTD.

Some introductory documentation is needed to explain what is meant by a page and a sector and what the alignment requirements are for the different functions.

Are writes to already written sectors allowed? native MTD emulation should emulate flash by only programming the 0 bits (value = value & input).

jnohlgard · 2016-07-14T06:41:51Z

@LudwigKnuepfer I like the proposed structure with a driver struct containing pointers to all the functions to allow more than one type of flash device present in the same running system, this was difficult to achieve with the interface proposed in #2239. The interface proposed in this PR is similar to the already existing NVRAM interface.

jnohlgard · 2016-07-14T06:45:20Z

@LudwigKnuepfer though I agree that there are many good parts in #2239 that could be lifted into this PR. For example the native flash emulation in #2239 emulate the properties of flash better than the current state of this PR does.

jnohlgard · 2016-07-14T10:02:42Z

cpu/native/mtdi/native_mtdi.c

+    if (addr + size > NATIVE_MTDI_FLASH_SIZE) {
+        return MTDI_RES_PARERR;
+    }
+    if (addr % NATIVE_MTDI_SECTOR_SIZE + size > NATIVE_MTDI_SECTOR_SIZE) {


add some parentheses to make this a bit more readable

vincent-d · 2016-07-14T10:03:03Z

Doc improved, even if it could still be better. Still not entirely sure about the boundaries for read/write/erase, but it seems to match at least how SPIFFS uses it.

I also improved the flash emulation.

jnohlgard · 2016-07-14T10:03:03Z

cpu/native/mtdi/native_mtdi.c

+    if (addr + size > NATIVE_MTDI_FLASH_SIZE) {
+        return MTDI_RES_PARERR;
+    }
+    if (addr % NATIVE_MTDI_SECTOR_SIZE != 0 || size % NATIVE_MTDI_SECTOR_SIZE != 0) {


jnohlgard · 2016-07-14T15:43:11Z

btw, why is it called MTDI and not just MTD?

vincent-d · 2016-07-14T16:24:27Z

The I stands for interface. @AurelienGONCE wanted to clearly show that it's an interface ;)
I have no better explanation!
It can still renamed, that is no big deal for me.

LudwigKnuepfer · 2016-07-14T17:21:46Z

cpu/native/mtdi/native_mtdi.c

+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+static const char _fname[] = "MEMORY.bin";


I would like to see this provided as a command line parameter or option.

jnohlgard · 2016-07-14T18:47:02Z

I'd prefer to call the interface MTD and the main interface file mtd.h, then the implementations can be called mtd_native.h, mtd_spi.h etc.

btw, I'm working on a SPI NOR flash driver for this interface.

jnohlgard · 2016-07-14T18:48:33Z

drivers/include/mtdi.h

+ * The number of pages in a sector must be constant for the whole MTD.
+ *
+ * Most of the flash filesystems implementation will use pages as basic unit for writing.
+ * This interface does not limit the writing on bage boundaries but user must not write on


"bage" -> "page"

jnohlgard · 2016-07-14T19:39:50Z

What is the use case for the mtdi_status function?

jnohlgard · 2016-07-15T04:46:59Z

Some more review comments:

The argument types for buffers unsigned char * should be changed to void * according to the coding conventions:
https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions
See also a recent discussion on the mailing list: https://lists.riot-os.org/pipermail/devel/2016-July/004155.html

I suggest that the addr and size parameters change to uint32_t to make it more visible what the supported memory address space is.

We tend to avoid "getter" functions for simple values in RIOT. I suggest that the GET_xxx stuff in ioctl be moved to public variables inside the device instance struct mtdi_dev_t. Since the documentation also states that an MTD consists of sectors and pages, these counts and sizes could just go directly in mtdi_dev_t.

typedef struct {
    const mtdi_desc_t *driver;
    uint32_t sector_count;
    uint32_t pages_per_sector;
    uint32_t page_size;
} mtdi_dev_t;

The other numbers can be derived from those three values by multiplication. Also, it avoids a possible ambiguity regarding total memory size by eliminating the redundant counts.

vincent-d · 2017-03-10T11:13:09Z

Fix doxygen and cppcheck errors

jnohlgard · 2017-03-15T14:54:15Z

tests/unittests/tests-mtd/tests-mtd.c

+    TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read + sizeof(buf), sizeof(buf_empty)));
+
+    /* Unaligned write / read */
+    ret = mtd_write(dev, buf, dev->page_size, sizeof(buf));


How about we change this to

ret = mtd_write(dev, buf, dev->page_size + sizeof(buf_empty), sizeof(buf));

and change the readback to match:

TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read, sizeof(buf_empty))); TEST_ASSERT_EQUAL_INT(0, memcmp(buf, buf_read + sizeof(buf_empty), sizeof(buf)));

that will test completely unaligned writes, and also catch implementation errors where the read or write addresses are not the same

good idea, done

jnohlgard · 2017-03-15T18:08:13Z

It looks okay now, I'll give it a spin later tonight or tomorrow and give my ACK.

jnohlgard · 2017-03-15T18:15:11Z

Ok to squash

kaspar030 · 2017-03-15T19:34:44Z

cpu/native/mtd/mtd_native.c

+        if (!f) {
+            return -EIO;
+        }
+        unsigned long size = dev->sector_count * dev->pages_per_sector * dev->page_size;


how about size_t?

It could make sense. I'll change that here and in other places.

jnohlgard · 2017-03-16T09:34:16Z

found one minor nit in mtd_vfs. I'm writing a unittest for mtd_vfs too. I'll post back when it's done

vincent-d · 2017-03-16T10:28:04Z

Squashed, with @kaspar030 remark fixed

jnohlgard · 2017-03-16T15:19:31Z

Two commits for mtd-vfs:
OTAkeys@ee8cf6e
OTAkeys@c3dbf38

vincent-d · 2017-03-16T15:46:58Z

@gebart cherry-picked and squashed, thanks

vincent-d · 2017-03-17T13:16:04Z

I don't understand why Murdock2 failed. Murdock and Jenkins seem happy anyway.

vincent-d · 2017-03-17T15:41:31Z

@gebart thanks! Shall I press the button ?

jnohlgard · 2017-03-17T16:15:32Z

ACK,
@vincent-d would you like the honor of pressing the button?

vincent-d · 2017-03-17T16:17:55Z

Then GO!
Thanks @gebart !

jnohlgard · 2017-03-17T21:19:59Z

Congratulations 🎊 🎉
Now, let's get the mtd_spi_nor stuff merged

vincent-d mentioned this pull request Jul 12, 2016

pkg: add SPIFFS support #5625

Merged

OlegHahm assigned x3ro and jnohlgard Jul 12, 2016

OlegHahm added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Jul 12, 2016

vincent-d mentioned this pull request Jul 13, 2016

vfs: file system abstraction #5616

Merged

55 tasks

jnohlgard added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Jul 13, 2016

jnohlgard reviewed Jul 14, 2016
View reviewed changes

jnohlgard mentioned this pull request Jul 14, 2016

State of low-level flash storage interfaces #5398

Closed

LudwigKnuepfer reviewed Jul 14, 2016
View reviewed changes

jnohlgard reviewed Jul 14, 2016
View reviewed changes

vincent-d added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 10, 2017

jnohlgard reviewed Mar 15, 2017

View reviewed changes

kaspar030 reviewed Mar 15, 2017

View reviewed changes

AurelienGONCE and others added 3 commits March 16, 2017 11:23

mtd: add a low level generic driver for flash file system

49ebf55

native: add mtd-compliant flash emulation

9e8cb3d

unittests: add mtd unittests

81149f1

vincent-d force-pushed the pr/mtdi_flash branch from 3e8f754 to 2afca63 Compare March 16, 2017 10:27

vincent-d removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 16, 2017

mtd: Add MTD VFS wrapper for use with DevFS

6d3304f

vincent-d force-pushed the pr/mtdi_flash branch from 2afca63 to 454b891 Compare March 16, 2017 15:45

vincent-d force-pushed the pr/mtdi_flash branch from 34b15d2 to 3ca1446 Compare March 16, 2017 16:01

unittests: add unit test for mtd-vfs

9782be1

vincent-d force-pushed the pr/mtdi_flash branch from 3ca1446 to 9782be1 Compare March 16, 2017 17:19

jnohlgard approved these changes Mar 17, 2017

View reviewed changes

vincent-d merged commit 1b2b5d9 into RIOT-OS:master Mar 17, 2017

vincent-d deleted the pr/mtdi_flash branch March 17, 2017 16:18

kYc0o mentioned this pull request Mar 21, 2017

Firmware swapping #6450

Closed

jnohlgard added the Area: fs Area: File systems label Feb 19, 2018

Conversation

vincent-d commented Jul 12, 2016

Uh oh!

LudwigKnuepfer commented Jul 12, 2016

Uh oh!

LudwigKnuepfer commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 14, 2016

Uh oh!

jnohlgard commented Jul 14, 2016

Uh oh!

jnohlgard commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Jul 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 14, 2016

Uh oh!

vincent-d commented Jul 14, 2016

Uh oh!

LudwigKnuepfer Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 14, 2016

Uh oh!

jnohlgard commented Jul 15, 2016

Uh oh!

vincent-d commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 15, 2017

Uh oh!

jnohlgard commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 16, 2017

Uh oh!

vincent-d commented Mar 16, 2017

Uh oh!

jnohlgard commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincent-d commented Mar 16, 2017

Uh oh!

vincent-d commented Mar 17, 2017

Uh oh!

vincent-d commented Mar 17, 2017

Uh oh!

jnohlgard commented Mar 17, 2017

Uh oh!

vincent-d commented Mar 17, 2017

Uh oh!

jnohlgard commented Mar 17, 2017

Uh oh!

Reviewers

LudwigKnuepfer commented Jul 12, 2016 •

edited

Loading

jnohlgard Jul 14, 2016 •

edited

Loading

jnohlgard commented Jul 14, 2016 •

edited

Loading

LudwigKnuepfer Jul 14, 2016 •

edited

Loading

jnohlgard commented Jul 14, 2016 •

edited

Loading

jnohlgard commented Mar 16, 2017 •

edited

Loading