Merge the feature-flash-api branch into master #3802

c1728p9 · 2017-02-19T21:13:15Z

Bring all the changes made on the feature-flash-api branch main-line.

c1728p9 · 2017-02-19T21:13:59Z

/morph test-nightly

mbed-bot · 2017-02-20T00:11:37Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1652

All builds and test passed!

0xc0170 · 2017-02-20T14:42:14Z

retest uvisor

c1728p9 · 2017-02-20T17:47:30Z

Fixed the commit author

c1728p9 · 2017-02-20T17:47:36Z

/morph test-nightly

bridadan · 2017-02-20T19:11:52Z

/morph test-nightly

mbed-bot · 2017-02-20T22:12:56Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1659

All builds and test passed!

0xc0170 · 2017-02-21T09:28:32Z

Thanks Russ !

Jenkins restarted

Please review

adbridge

Does the copyright date need updating for the new files ?

adbridge · 2017-02-21T17:55:52Z

hal/flash_api.h

+ * @param obj The flash object
+ * @param address The sector starting address
+ * @param data The data buffer to be programmed
+ * @param size The number of in bytes


The number of in bytes ? Something missing there ?

adbridge · 2017-02-21T17:56:54Z

hal/flash_api.h

+
+/** Get start address for the flash region
+ * 
+ * @param obj The flash objects


should be object singular ?

adbridge · 2017-02-21T17:57:27Z

hal/flash_api.h

+
+/** Get the flash region size
+ * 
+ * @param obj The flash objects


object singular ?

adbridge · 2017-02-21T17:57:59Z

TESTS/mbed_hal/flash/functional_tests/main.cpp

@@ -0,0 +1,249 @@
+/* mbed Microcontroller Library
+ * Copyright (c) 2016 ARM Limited


adbridge · 2017-02-21T18:11:17Z

TESTS/mbed_hal/flash/functional_tests/main.cpp

+
+utest::v1::status_t greentea_failure_handler(const Case *const source, const failure_t reason) {
+    greentea_case_failure_abort_handler(source, reason);
+    return STATUS_CONTINUE;


STATUS_CONTINUE shouldn't really be used for the failure handler.

because it doesn't actually work properly

if a case fails then the overall test fails and should be reported as such (as per Marcus' recommendations)

adbridge · 2017-02-21T18:22:37Z

drivers/FlashIAP.cpp

+bool FlashIAP::is_aligned_to_sector(uint32_t addr, uint32_t size)
+{
+    uint32_t current_sector_size = flash_get_sector_size(&_flash, addr);
+    if ((is_aligned(size, current_sector_size) == false) || 


if (! is_aligned(..) )

In fact can't this whole statement be re-written as
return (is_aligned(size, current_sector_size) && (is_aligned(addr, current_sector_size) )

adbridge · 2017-02-21T18:26:19Z

drivers/FlashIAP.cpp

+            break;
+        }
+        current_sector_size = flash_get_sector_size(&_flash, addr);
+        if (is_aligned_to_sector(addr, size) == false) {


again should use '!' operator rather than checking directly to false

adbridge · 2017-02-21T18:27:28Z

drivers/FlashIAP.h

+     *
+     *  Sector size might differ at address ranges. 
+     *  An example <0-0x1000, sector size=1024; 0x10000-0x20000, size=2048>
+     *


Missing the addr parameter

adbridge · 2017-02-21T18:28:10Z

drivers/FlashIAP.h

+     *  @param number    Number that should be aligned
+     *  @param alignment Defined alignment for a number to be checked
+     *  @return true on success, false on failure
+     */


this header looks wrong

adbridge · 2017-02-21T18:31:02Z

TESTS/mbed_drivers/flashiap/main.cpp

+
+utest::v1::status_t greentea_failure_handler(const Case *const source, const failure_t reason) {
+    greentea_case_failure_abort_handler(source, reason);
+    return STATUS_CONTINUE;


Same comment about STATUS_CONTINUE

theotherjimmy

No tools changes. ¯\_(ツ)_/¯

Add a flash api with functions to erase and program. The api also includes functions to query flash start, flash size, page size and sector size.

Add tests to verify the hal port of the flash_api.

If target supports flash algo, it can get common HAL implementation, providing feature "HAL_FLASH_CMSIS_ALGO". This simplifies target code, and having one implementation that can be shared by many targets. Be careful with flash cmsis algo, in some cases they execute code that can affect the system. For instance, it can disable cache, or affect some system clocks. Therefore this feature should be well tested.

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

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

Check in flash algos for the K64F, KL46Z, F429, F439 and Odin board and enable these features accordingly in targets.json. This implementation uses flash algo blob that are generated via scripts. The K64F and KL46Z were generated directly from packs, while the KL46Z, F429, F439 and odin were generated from code checked into the FlashAlgo repo.

c1728p9 · 2017-02-21T20:16:40Z

@adbridge I updated this PR to address your feedback

c1728p9 · 2017-02-21T20:24:20Z

/morph test-nightly

mbed-bot · 2017-02-21T23:30:28Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1671

All builds and test passed!

sg-

👍

adbridge

LGTM

dadiliebian · 2017-04-09T05:28:19Z

how to generate the flash_api.c ?

0xc0170 · 2017-04-10T09:20:23Z

how to generate the flash_api.c ?

Please create an issue (a question) here. I can quickly provide an answer here:

Look at tools/flash_algo/extract.py script that extracts information from arm packs to generate flash blobs (your device should support the packs).

c1728p9 requested review from theotherjimmy, 0xc0170 and sg- February 19, 2017 21:13

0xc0170 approved these changes Feb 20, 2017

View reviewed changes

0xc0170 added needs: review release-version: 5.4.0 labels Feb 20, 2017

c1728p9 force-pushed the feature-flash-api branch from c5092e2 to aa65aaf Compare February 20, 2017 17:47

bridadan added the needs: CI label Feb 20, 2017

0xc0170 removed the needs: CI label Feb 21, 2017

adbridge requested changes Feb 21, 2017

View reviewed changes

theotherjimmy reviewed Feb 21, 2017

View reviewed changes

theotherjimmy approved these changes Feb 21, 2017

View reviewed changes

0xc0170 and others added 6 commits February 21, 2017 14:08

hal: add flash hal api

fdfb820

Add a flash api with functions to erase and program. The api also includes functions to query flash start, flash size, page size and sector size.

test: add flash functional tests for flash HAL

4d55719

Add tests to verify the hal port of the flash_api.

flash: add FlashIAP class

8700158

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

tests: add FlashIAP functional test

2fa2927

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

c1728p9 force-pushed the feature-flash-api branch from aa65aaf to 0712354 Compare February 21, 2017 20:15

sg- approved these changes Feb 22, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Feb 22, 2017

adbridge approved these changes Feb 22, 2017

View reviewed changes

sg- merged commit 355f69a into master Feb 22, 2017

sg- removed the ready for merge label Feb 22, 2017

0xc0170 deleted the feature-flash-api branch April 10, 2017 09:18

		@@ -0,0 +1,249 @@
		/* mbed Microcontroller Library
		* Copyright (c) 2016 ARM Limited

Merge the feature-flash-api branch into master #3802

Merge the feature-flash-api branch into master #3802

Uh oh!

Conversation

c1728p9 commented Feb 19, 2017

Uh oh!

c1728p9 commented Feb 19, 2017

Uh oh!

mbed-bot commented Feb 20, 2017

Output

Uh oh!

0xc0170 commented Feb 20, 2017

Uh oh!

c1728p9 commented Feb 20, 2017

Uh oh!

c1728p9 commented Feb 20, 2017

Uh oh!

bridadan commented Feb 20, 2017

Uh oh!

mbed-bot commented Feb 20, 2017

Output

Uh oh!

0xc0170 commented Feb 21, 2017

Uh oh!

adbridge left a comment • 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!

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!

theotherjimmy left a comment • 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!

c1728p9 commented Feb 21, 2017

Uh oh!

c1728p9 commented Feb 21, 2017

Uh oh!

mbed-bot commented Feb 21, 2017

Output

Uh oh!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

adbridge left a comment

Choose a reason for hiding this comment

Uh oh!

dadiliebian commented Apr 9, 2017

Uh oh!

0xc0170 commented Apr 10, 2017

Uh oh!

Uh oh!

adbridge left a comment •

edited

Loading

theotherjimmy left a comment •

edited

Loading