New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FlashIAP driver modifications #6140

Merged
merged 1 commit into from Feb 23, 2018

Conversation

Projects
None yet
9 participants
@davidsaada
Contributor

davidsaada commented Feb 20, 2018

Description

A few FlashIAP driver modifications

  • Support programming across sectors.
  • Support program size not aligned to page size.

Status

READY

@0xc0170 0xc0170 requested a review from c1728p9 Feb 20, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 20, 2018

Make class singleton.
Support programming across sectors.
Support program size not aligned to page size.

It would be good if we split new API from bugfixing current implementation.
What I see here: add singleton would be a new commit (preferably separate PR) as this can't be added to patch releases.
The other two aim to improve the current implementation thus should be separated.

This splitting adds a bit of overhead (plus separate PR for new API and bugfixing) but brings more benefit in the long run (maintenance, review, etc).

I would suggest this PR become bugfixing (improving the current implementation), and singleton addition new PR.

This is stated in our workflow document:

Pull requests should fix a bug, add a feature or refactor.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 20, 2018

0xc0170 - can do, although FlashIAP not being a singleton is surely a bug (allocated two instances by mistake in NVStore tests, and the results were erroneous).
In addition, didn't see any usage of FlashIAP anywhere else in the mbed-os code, so impact was only on FlashIAP driver itself (and its test).

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 20, 2018

0xc0170 - Not sure if you accepted my answer or not (did you?), but if you did - please don't merge the PR yet. Some of my NVStore tests fail with this change, and I want to be certain that it's not a bug here.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 20, 2018

0xc0170 - can do, although FlashIAP not being a singleton is surely a bug (allocated two instances by mistake in NVStore tests, and the results were erroneous).

Still this adds new API thus should be treated separately. Let's split it

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 20, 2018

0xc0170 - False alarm. Was a problematic board. All's well.

@@ -40,6 +40,13 @@ namespace mbed {
*/
class FlashIAP : private NonCopyable<FlashIAP> {
public:
/**

This comment has been minimized.

@0xc0170

0xc0170 Feb 20, 2018

Member

this should be still removed (you can rebase to clean up the history to have only 1 commit here)

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

Uhhh...

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

This comment has been minimized.

@0xc0170

0xc0170 Feb 20, 2018

Member

while reviewing this, this is similar to #6077 (this is however only program, but erase should do the same) ?

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

Not sure I understand the comment. Erase seems to be following a similar algorithm as program now. What's the problem?

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

Oh I see the problem now.
My thought here is that the entire check should just be eliminated. Why not erase everything that's in the range, and let the driver worry about alignments, not the user.

This comment has been minimized.

@geky

geky Feb 20, 2018

Member

So if I understand correctly, an unaligned erase does this?

            --- sector 0 ---  ->  --- sector 0 ---    
            data                  data
            data                  data
            data                  data
            --- sector 1 ---      --- sector 1 ---
            data                  ff
          > data                  ff
unaligned | data                  ff
erase     | --- sector 2 ---      --- sector 2 ---
          > data                  ff
            data                  ff
            data                  ff

That seems like the sort of thing we'd want to error on.

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

As said, don't think there should even be an unaligned erase check. Don't even know what it stands for.

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

Let me rephrase: I believe the only acceptable validation here is to check that the address is on a sector boundary, and that the size is a sum of all the consecutive sector sizes. The only way to achieve this is by iterating on the sectors for the validation, and then iterating again for the erase action. It's either that or check nothing.

This comment has been minimized.

@geky

geky Feb 20, 2018

Member

Ah ok. Don't let me block this pr then.

Could we check that address + size is on a sector boundary? Would that be cheap?

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

Yeah, figured this out after writing the answer. This would be the correct check.

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_xsectors branch from 59275d8 to 20c6799 Feb 20, 2018

@geky

Looks good so far, left a few comments though they may just be me misunderstanding.

buf = page_buf;
prog_size = page_size;
}
else {

This comment has been minimized.

@geky

geky Feb 20, 2018

Member

style nit: else should be on same line as }

const uint8_t *buf = (uint8_t *) buffer;
// addr should be aligned to page size
if (!is_aligned(addr, page_size) || (!buffer)) {

This comment has been minimized.

@geky

geky Feb 20, 2018

Member

Do we not still need to check that size is aligned and that program does not go past end of the device?

This comment has been minimized.

@davidsaada

davidsaada Feb 20, 2018

Contributor

The page alignment check here is for the address only. Don't think we need the size alignment check. Adding such a check would require the user to split the writes to the aligned and unaligned ones, which is basically what the new code does.
Regarding the device end check - good thought. Will add that.

@@ -26,8 +26,7 @@ class GCC(mbedToolchain):
LIBRARY_EXT = '.a'
STD_LIB_NAME = "lib%s.a"
DIAGNOSTIC_PATTERN = re.compile('((?P<file>[^:]+):(?P<line>\d+):)(\d+:)? (?P<severity>warning|[eE]rror|fatal error): (?P<message>.+)')
INDEX_PATTERN = re.compile('(?P<col>\s*)\^')
DIAGNOSTIC_PATTERN = re.compile('((?P<file>[^:]+):(?P<line>\d+):)(?P<col>\d+):? (?P<severity>warning|[eE]rror|fatal error): (?P<message>.+)')

This comment has been minimized.

@geky

geky Feb 20, 2018

Member

What's the tools change? @theotherjimmy are you able to double check this?

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 20, 2018

Contributor

Thanks for the spot! see my comment below.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Feb 20, 2018

@davidsaada You have a rebase of a commit on master (1290863) in your PR. Please remove this commit from the series.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 20, 2018

Argh. Probably happened after the squash and force push. Thanks @theotherjimmy.

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_xsectors branch 3 times, most recently from a0d5fd7 to 20646dc Feb 20, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 21, 2018

/morph build

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 21, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 21, 2018

Build : SUCCESS

Build number : 1194
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6140/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 21, 2018

Build : SUCCESS

Build number : 1195
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6140/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@@ -31,6 +33,8 @@ namespace mbed {
SingletonPtr<PlatformMutex> FlashIAP::_mutex;
#define BLANK 0xFF

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

Please use more descriptive name (BLANK can come from targets code and can have diff value), FLASHIAP_BLANK or similar would be better

@@ -128,6 +128,7 @@ class FlashIAP : private NonCopyable<FlashIAP> {
bool is_aligned_to_sector(uint32_t addr, uint32_t size);
flash_t _flash;
uint8_t *page_buf;

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

_ prefix for private members

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

Thanks, forgot that.

chunk = std::min(current_sector_size - (addr % current_sector_size), size);
if (chunk < page_size) {
memcpy(page_buf, buf, chunk);
memset(page_buf+chunk, BLANK, page_size-chunk);

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

to be: memset(page_buf + chunk, BLANK, page_size - chunk); (spaces around operators as line 111

return -1;
} else if (erase_end_addr < flash_end_addr){
uint32_t following_sector_size = flash_get_sector_size(&_flash, erase_end_addr);

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

is this "following" sector or the "last sector size to be erased"? From the code I read it as it is the last sector to be erased.

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

@LiyouZhou please review

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

No. Following means the one after this range. Couldn't find a better name (any recommendation would be welcome).

This comment has been minimized.

@LiyouZhou

LiyouZhou Feb 21, 2018

We are making the assumption that: flash sector always start on an address that is aligned to the flash sector size. This is true for all platfroms that I know but I don't know if it is generally true.

I think this will work for the issue i raised. Can we have a testcase to make sure there is no regression?

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

I also never saw cases where sector addresses weren't aligned to their own sizes. This is pretty easy to fix (subtract flash start from relevant cases). Not sure it justifies a special test (if I understood you correctly).

This comment has been minimized.

@LiyouZhou

LiyouZhou Feb 21, 2018

I meant a test for trying to erase 2 sectors of different sizes at once. basically the situation i describe here: #6077

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

Oh I see. The NUCLEO_F410RB has two last sectors having different sizes (16KB and 64KB). I can change my new test to erase the last two sectors in one call, and this would cover the case.

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

Fixed the test to erase the two last sectors in one call. This will now check your described scenarios in boards whose last two sector sizes are different (there are a few).

This comment has been minimized.

@0xc0170

0xc0170 Feb 22, 2018

Member

@LiyouZhou Can you review the latest changes and leave feedback here?

This comment has been minimized.

@davidsaada

davidsaada Feb 22, 2018

Contributor

@LiyouZhou Would appreciate a quick review, as it seems your review is all that's needed to apply this PR.

}
address += sector_size - 3 * page_size;
uint32_t aligned_prog_size = 7 * page_size;

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

some devices might fail to allocate 7page_size (if we consider that data are deleted on line 124, this means we are allocating double than that, results in 14page_size?

Lets review the test results for this test

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

Maybe I'm mistaken, but AFAIK page sizes are typically 8 bytes. Is this problematic?

This comment has been minimized.

@0xc0170

0xc0170 Feb 21, 2018

Member

I recall there was a problem but was probably with sectors rather than pages (as page size that is returned is the smallest unit thta can be written, thus in most cases would be 8 bytes).

Just checking that this will be ok for all targets

This comment has been minimized.

@davidsaada

davidsaada Feb 21, 2018

Contributor

Appears you were right. Boards (namely ARCH_PRO) have a 1KB page size. Modifying the test accordingly.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 21, 2018

Support programming across sectors.
Support program size not aligned to page size.

One last check, do documentation need any updating with this fixes? It would be good if this is captured in the methods documentation what is supported and what is not. I read t he report this week related to program/erase missing this.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 21, 2018

Looking at the handbook, the md file doesn't require any changes. The code (and derived Doxygen documentation) , however, needs a bit of modifications in the program API: It says that the size needs to be aligned to sector and page sizes. The latter isn't true any more, but the former was never true. Will change that in the code, but Doxy needs to be regenerated.

@mbed-ci

This comment has been minimized.

1 similar comment
@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Feb 21, 2018

@@ -31,6 +33,8 @@ namespace mbed {
SingletonPtr<PlatformMutex> FlashIAP::_mutex;
#define FLASHIAP_BLANK 0xFF

This comment has been minimized.

@c1728p9

c1728p9 Feb 22, 2018

Contributor

Many of the STM32L devices have an erased value of 0x00 rather than 0xFF. (the only reason I know this is because I came across it today while adding DAPLink support to the STM32L151CBU6A) The HAL API should probably to be modified to expose this information if it is used as this level.

In this case it looks like this value may be used just as a pad value. If so you might want to rename it so it isn't confused with the erased/blank value.

This comment has been minimized.

@davidsaada

davidsaada Feb 22, 2018

Contributor

This is indeed used just once as a PAD value in the driver, so the define will be removed.
The driver doesn't assume that blank values are 0xFF, but other code (such as NVStore) does, so will ask for such an API in HAL. If and when implemented, it should be exposed via this driver.

if (chunk < page_size) {
memcpy(_page_buf, buf, chunk);
memset(_page_buf + chunk, FLASHIAP_BLANK, page_size - chunk);
buf = _page_buf;

This comment has been minimized.

@c1728p9

c1728p9 Feb 22, 2018

Contributor

buf is clobbered here and never restored. It isn't actually a problem though since it should only happen on the last iteration of the loop. It may not hurt to add another variable, something like prog_buf to go along with prog_size so buf doesn't need to be clobbered.

This comment has been minimized.

@davidsaada

davidsaada Feb 22, 2018

Contributor

Good idea. This was intentional knowing it's the last iteration, but it can be missed if the code is modified later on.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 22, 2018

@davidsaada Holding off on the merge until the commit that addresses @c1728p9's review comes in.

Separate note, was this PR ever split up, according to @0xc0170's request? (#6140 (comment))

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 22, 2018

@cmonr No problem. Committing a quick fix soon. And yes - the PR was split up. Will issue the second part once this one is merged.

FlashIAP driver modifications:
- Support programming across sectors.
- Support program size not aligned to page size.
- Fix validations on sector erase.

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_xsectors branch from 326ac03 to 0aeeece Feb 22, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 22, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 22, 2018

Build : SUCCESS

Build number : 1224
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6140/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 23, 2018

/morph build

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 23, 2018

Started morph build again. That exporter build failure seems like a setup glitch.

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 23, 2018

Build : SUCCESS

Build number : 1226
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6140/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

Testing the master protection against merging with request changes status (will revert asap)

@0xc0170 0xc0170 merged commit 1079767 into ARMmbed:master Feb 23, 2018

18 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mbed-ci-generic Build finished.
Details
travis-ci/docs/ Local docs testing has passed
Details
travis-ci/events/ Local events testing has passed
Details
travis-ci/littlefs/ Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL/ Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM/ Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC/ Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON/ Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP/ Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details

@davidsaada davidsaada deleted the davidsaada:david_flashiap_xsectors branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment