Feature: Flash IAP c++ class addition #3700

0xc0170 · 2017-02-06T16:23:24Z

Description

FlashIAP class addition. It provides block device alike API (write/read/program) plus some getters that are useful for applications.

I added a functional test that was tested with k64f, more to come.

+----------+---------------+-----------------------------+--------+--------------------+-------------+
| target   | platform_name | test suite                  | result | elapsed_time (sec) | copy_method |
+----------+---------------+-----------------------------+--------+--------------------+-------------+
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | OK     | 13.08              | shell       |
+----------+---------------+-----------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 1 OK
mbedgt: test case report:
+----------+---------------+-----------------------------+--------------------+--------+--------+--------+--------------------+
| target   | platform_name | test suite                  | test case          | passed | failed | result | elapsed_time (sec) |
+----------+---------------+-----------------------------+--------------------+--------+--------+--------+--------------------+
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - init    | 1      | 0      | OK     | 0.05               |
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - program | 1      | 0      | OK     | 0.1                |
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - write   | 1      | 0      | OK     | 0.1                |
+----------+---------------+-----------------------------+--------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 3 OK

Status

READY

This PR is for feature branch feature-flash-api

cc @c1728p9 @geky

0xc0170 · 2017-02-06T16:25:42Z

drivers/FlashIAP.cpp

+
+}
+
+int FlashIAP::init()


opinions for return types, own enum ? We have int for the core classes but I have seen some new classes go towards defining own types that are descriptive and more readable

I think typdefs help document the intent of different parts of the function declaration:

flash_error_t FlashIAP::read(void *buffer, flash_addr_t addr, flash_size_t size);

However I don't think the error return should be an enum. It allows compilation warnings, but prevents cross module errors and the strict casting rules of c++ limit compatibility with simple ints (which I don't think is unreasonable for users to use). Keeping error types a typedef of int allows -1 as a "miscellaneous" error and gradual adoption of more useful error codes.

Just my 2c, the new classes do diverge here, so a different perspective may be valuable.

geky · 2017-02-06T16:46:02Z

drivers/FlashIAP.cpp

+    uint32_t count = size;
+    uint8_t *flash_pointer = (uint8_t *)addr;
+    uint8_t *read_buffer = (uint8_t *)buffer;
+    while (count) {


Out of curiousity, why not memcpy? memcpy(buffer, (const void*)addr, size)

geky · 2017-02-06T16:47:49Z

drivers/FlashIAP.cpp

+        return -1;
+    }
+
+    if (flash_program_page(&_flash, addr, (const uint8_t *)buffer, size)) {


Nit: Why not just call program? Keeps you from repeating yourself.

geky · 2017-02-06T16:51:46Z

drivers/FlashIAP.cpp

+
+int FlashIAP::erase(uint32_t addr, uint32_t size)
+{
+    MBED_ASSERT((flash_get_sector_size(&_flash, addr) % size) == 0);


Should probably also check alignment? (addr / sector_size) * sector_size == addr

geky · 2017-02-06T17:01:02Z

Nice work so far 👍

Out of curiousity, how would a user get the range of addresses a FlashIAP covers?

pan- · 2017-02-06T16:53:36Z

drivers/FlashIAP.h

+    FlashIAP();
+    ~FlashIAP();
+
+    /** Initialize a flash block device


What is the expected behavior if this function is called twice in a row ? Is it authorized ?

Added comment - required to be called just once per lifetime of an object

pan- · 2017-02-06T16:56:35Z

drivers/FlashIAP.h

+namespace mbed {
+
+/** \addtogroup drivers */
+/** @{*/


Could you comment on the expected thread safety of the class ?

I'll add it

pan- · 2017-02-06T16:58:34Z

drivers/FlashIAP.h

+
+    /** Write blocks to a block device
+     *
+     *  A write is equivalent to an erase followed by a program


Could you indicate that in case of error, during the operation. If the data has been erased they are not recovered ?

pan- · 2017-02-06T17:01:33Z

drivers/FlashIAP.cpp

+
+int FlashIAP::erase(uint32_t addr, uint32_t size)
+{
+    MBED_ASSERT((flash_get_sector_size(&_flash, addr) % size) == 0);


Why should it be an assertion rather than a true precondition check which returns an error ?

Checking addr alignment to block address might be a good thing.

pan- · 2017-02-06T17:03:56Z

drivers/FlashIAP.cpp

+}
+
+int FlashIAP::program(const void *buffer, uint32_t addr, uint32_t size)
+{


Is it valuable to check addr block alignment and size size ?

pan- · 2017-02-06T17:10:07Z

drivers/FlashIAP.h

+     *  @param size   Size to read in bytes, must be a multiple of read block size
+     *  @return       0 on success, negative error code on failure
+     */
+    int read(void *buffer, uint32_t addr, uint32_t size);


I think this member function can be tagged as const.

pan- · 2017-02-06T17:10:19Z

drivers/FlashIAP.h

+     *
+     *  @return Size of a sector in bytes
+     */
+    uint32_t get_sector_size(uint32_t addr);


I think this member function can be tagged as const.

pan- · 2017-02-06T17:10:39Z

drivers/FlashIAP.h

+     *
+     *  @return Size of the underlying device in bytes
+     */
+    uint32_t get_flash_size();


I think this member function can be tagged as const.

pan- · 2017-02-06T17:10:46Z

drivers/FlashIAP.h

+     *
+     *  @return Size of a program page in bytes
+     */
+    uint32_t get_page_size();


I think this member function can be tagged as const.

pan- · 2017-02-06T17:11:36Z

hal/flash_api.h

+ * @param obj The flash objects
+ * @return The size of the flash available
+ */
+uint32_t flash_get_size(flash_t *obj);


const flash_t *obj ?

c1728p9 · 2017-02-07T02:28:26Z

TESTS/mbed_drivers/flashiap/main.cpp

+    uint32_t sector_size = flash_device.get_sector_size(flash_device.get_flash_size() - 1UL);
+    TEST_ASSERT_NOT_EQUAL(sector_size, 0);
+    const uint8_t test_value = 0xCE;
+    uint8_t *data = new uint8_t[sector_size];


This should be deleted after use. Same for a couple calls below too.

Done, good catch !

c1728p9 · 2017-02-07T02:50:48Z

drivers/FlashIAP.cpp

+        if (ret != 0) {
+            return -1 ;
+        }
+        uint32_t current_sector_size = flash_get_sector_size(&_flash, addr);


It might be good to check or assert sector size in a loop either here or above. If sector size gets bigger then size could underflow.

0xc0170 · 2017-02-07T14:05:00Z

Thanks for the review @geky @pan- @c1728p9 , I should have responded to all of them.

fixed preconditions check (I moved it to upper layer, thus HAL does not check it, added doc note)
add test cases to test those preconditions into flashiap test
thread safety added

+----------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
| target   | platform_name | test suite                  | test case                 | passed | failed | result | elapsed_time (sec) |
+----------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - init           | 1      | 0      | OK     | 0.04               |
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - program        | 1      | 0      | OK     | 0.1                |
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - program errors | 1      | 0      | OK     | 0.05               |
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - write          | 1      | 0      | OK     | 0.1                |
| K64F-ARM | K64F          | tests-mbed_drivers-flashiap | FlashIAP - write errors   | 1      | 0      | OK     | 0.05               |
+----------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+

0xc0170 · 2017-02-08T10:50:35Z

Please review again, I would like to have this on the feature branch :)

c1728p9

Aside from the missing lock release it looks good to me.

c1728p9 · 2017-02-08T15:01:01Z

drivers/FlashIAP.cpp

+    uint32_t current_sector_size = flash_get_sector_size(&_flash, addr);
+    // addr and size should be aligned to page size, and multiple of page size
+    // page program should not cross sector boundaries
+    if ((is_aligned(addr, page_size) == false) || (is_aligned(size, page_size) == false) ||


nitpick - this might be clearer if it is broken into individual checks.

if (!is_aligned(addr, page_size)) { return -1; } if (!is_aligned(size, page_size)) { return -1; } if (size < page_size) { return -1; } if (is_crossing_sector(current_sector_size, addr, size)) { return -1; }

That actually looks more noisy to me. Maybe just one condition per line would help.

That actually looks more noisy to me. Maybe just one condition per line would help.

I'll do that.

c1728p9 · 2017-02-08T15:02:01Z

drivers/FlashIAP.cpp

+    int ret = 0;
+    _mutex->lock();
+    if (flash_program_page(&_flash, addr, (const uint8_t *)buffer, size)) {
+        return -1;


Need to release lock before returing

geky · 2017-02-08T15:34:48Z

drivers/FlashIAP.cpp

+    // addr and size should be aligned to page size, and multiple of page size
+    // page program should not cross sector boundaries
+    if ((is_aligned(addr, page_size) == false) || (is_aligned(size, page_size) == false) ||
+       (size < page_size) || (((addr % current_sector_size) + size) > current_sector_size)) {


Question: Is sector size ever not a multiple of page size? The BlockDevice currently prohibits this, it could be an assert at construction time.

Correct, I dont think it can be not a multiple of page size.

geky · 2017-02-08T15:40:51Z

drivers/FlashIAP.h

+     *
+     *  @return Size of the underlying device in bytes
+     */
+    uint32_t get_flash_size() const;


Question: How does the user find the beginning of flash?

This will be added later - flash regions properly defined and getters for them. Currently flash algo implementations define flash beginning as 0, but that is not correct (there can be app text/data).

Plus having flash regions

0xc0170 · 2017-02-09T12:26:19Z

Responded to comments, rebased, rerunning CI

/morph test

mbed-bot · 2017-02-09T13:36:05Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1543

Build failed!

0xc0170 · 2017-02-09T13:50:49Z

/morph test

mbed-bot · 2017-02-09T14:49:58Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1544

Test failed!

0xc0170 · 2017-02-09T15:59:37Z

@c1728p9 If happy, please merge (the failure is for a platform that is not even supported on this branch, I'll investigate to fix it on master).

Edit: I am goign to update this branch, master is green for the platform that fails here.

geky

Looks good to me 👍

sg- · 2017-02-09T16:05:29Z

/morph test

0xc0170 · 2017-02-09T17:09:35Z

Added get_flash_start() method + all the functionality in HAL and tests updated

mbed-bot · 2017-02-09T18:59:00Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1547

Test failed!

c1728p9 · 2017-02-09T20:07:44Z

/morph test

mbed-bot · 2017-02-09T21:03:44Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1549

All builds and test passed!

flash api should include this header file, as it contains required data declarations.

Flash IAP that provides write/read/program to an internal API. It invokes flash HAL functions. FlashIAP checks for alignments for erase/write/program. HAL functions do not, to avoid duplication per target implementation.

It currently tests functionality provided by flash IAP API like write/read/program/erase and getters.

Plus fixes in the tests to get properly end of flash (start + size)

c1728p9 · 2017-02-10T19:40:45Z

/morph test

mbed-bot · 2017-02-10T21:00:42Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1558

All builds and test passed!

0xc0170 requested review from geky and c1728p9 February 6, 2017 16:23

0xc0170 commented Feb 6, 2017

View reviewed changes

geky reviewed Feb 6, 2017

View reviewed changes

pan- reviewed Feb 6, 2017

View reviewed changes

c1728p9 reviewed Feb 7, 2017

View reviewed changes

0xc0170 force-pushed the feature-flash-api branch 3 times, most recently from ed88d74 to 1b803af Compare February 7, 2017 14:02

0xc0170 force-pushed the feature-flash-api branch 2 times, most recently from dcccccf to 110ce9c Compare February 7, 2017 15:55

sg- added the needs: review label Feb 7, 2017

0xc0170 force-pushed the feature-flash-api branch from 110ce9c to 697afbc Compare February 8, 2017 10:48

c1728p9 reviewed Feb 8, 2017

View reviewed changes

geky reviewed Feb 8, 2017

View reviewed changes

0xc0170 force-pushed the feature-flash-api branch from 697afbc to f767e96 Compare February 9, 2017 12:24

0xc0170 added needs: CI and removed needs: review labels Feb 9, 2017

0xc0170 force-pushed the feature-flash-api branch from f767e96 to b28bf6b Compare February 9, 2017 13:50

geky approved these changes Feb 9, 2017

View reviewed changes

0xc0170 force-pushed the feature-flash-api branch from 7a682aa to c627713 Compare February 9, 2017 17:08

0xc0170 force-pushed the feature-flash-api branch from c627713 to 74a5619 Compare February 9, 2017 17:43

c1728p9 force-pushed the feature-flash-api branch from edf7fb3 to 3ebcf92 Compare February 9, 2017 19:53

c1728p9 mentioned this pull request Feb 9, 2017

Feature: Flash IAP c++ class addition #3732

Closed

c1728p9 force-pushed the feature-flash-api branch from 74a5619 to 08730c1 Compare February 9, 2017 20:06

c1728p9 force-pushed the feature-flash-api branch from 3ebcf92 to f258770 Compare February 10, 2017 19:34

0xc0170 added 5 commits February 10, 2017 13:37

flash: fix include header for cmsis algo

38f55eb

flash api should include this header file, as it contains required data declarations.

flash: add get flash size function

9017b2f

flash: add FlashIAP class

c3951d8

Flash IAP that provides write/read/program to an internal API. It invokes flash HAL functions. FlashIAP checks for alignments for erase/write/program. HAL functions do not, to avoid duplication per target implementation.

tests: add FlashIAP functional test

5fb9662

It currently tests functionality provided by flash IAP API like write/read/program/erase and getters.

flash: add get start flash region

6f8fa63

Plus fixes in the tests to get properly end of flash (start + size)

c1728p9 force-pushed the feature-flash-api branch from 08730c1 to 6f8fa63 Compare February 10, 2017 19:38

c1728p9 merged commit c4d6403 into ARMmbed:feature-flash-api Feb 10, 2017

cmonr removed the needs: CI label Apr 3, 2018

0xc0170 deleted the feature-flash-api branch September 1, 2020 14:09

Feature: Flash IAP c++ class addition #3700

Feature: Flash IAP c++ class addition #3700

Uh oh!

Conversation

0xc0170 commented Feb 6, 2017

Description

Status

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Feb 6, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 7, 2017

Uh oh!

0xc0170 commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Feb 6, 2017 •

edited

Loading

0xc0170 commented Feb 8, 2017 •

edited

Loading

0xc0170 commented Feb 9, 2017 •

edited

Loading