Skip to content

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented Apr 15, 2020

Summary of changes

This PR tries to fix invalid check in KVstore/FLASHIAP configuration. With non-default configuration of FLASHIAP, the defined FLASHIAP region can be in front of application, not default post-application. The post-application (FLASHIAP_APP_ROM_END_ADDR) check must be skipped.


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

With non-default configuration FLASHIAP, the region can be defined in front of application. The after-application check must be skipped.
@ciarmcom
Copy link
Member

@ccli8, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from a team April 15, 2020 11:00
@mergify mergify bot added needs: CI and removed needs: review labels Apr 15, 2020
@SeppoTakalo
Copy link
Contributor

Why the checks are not fixed, instead of just skipping?

@mbed-ci
Copy link

mbed-ci commented Apr 15, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

@ccli8
Copy link
Contributor Author

ccli8 commented Apr 16, 2020

Take flash layout of Pelion client application for examples. The FLASHIAP_APP_ROM_END_ADDR check is valid for default flash layout, where KVSTORE is located in back of FLASHIAP_APP_ROM_END_ADDR.

Default flash layout

    +--------------------------+
    |                          |
    |         KVSTORE          |
    |                          |
    +--------------------------+ <-+ storage_tdb_internal.internal_base_address
    |                          |
    |        Free space        |
    |                          |
    +--------------------------+
    |                          |
    |                          |
    |                          |
    |        Active App        |
    |                          |
    |                          |
    |                          |
    +--------------------------+ <-+ mbed-bootloader.application-start-address
    |Active App Metadata Header|
    +--------------------------+ <-+ update-client.application-details
    |                          |
    |        Bootloader        |
    |                          |
    +--------------------------+ <-+ 0

But invalid for non-default flash layout, where KVSTORE is located in front of FLASHIAP_APP_ROM_END_ADDR:

Non-default flash layout

    +---------------------------+ <-+ MBED_ROM_START + MBED_ROM_SIZE
    |        FREE SPACE         |
    +---------------------------+ <-+ FLASHIAP_APP_ROM_END_ADDR
    |                           |
    |                           |
    |        ACTIVE APP         |
    |                           |
    |                           |
    +---------------------------+ <-+ MBED_APP_START =
    +                           |     MBED_ROM_START + target.app_offset
    |ACTIVE APP METADATA HEADER |
    |                           |
    +---------------------------+ <-+ MBED_ROM_START + target.header_offse
    |                           |
    |    KVSTORE (internal)     |
    |                           |
    +---------------------------+ <-+ MBED_ROM_START + MBED_BOOTLOADER_SIZE
    |                           |
    |        BOOTLOADER         |
    |                           |
    +---------------------------+ <-+ MBED_ROM_START

Compared to mbed-os-5.15.1, its _get_blockdevice_FLASHIAP(...) also skips the FLASHIAP_APP_ROM_END_ADDR check for non-default configuration, so it is OK.

@ccli8
Copy link
Contributor Author

ccli8 commented Apr 23, 2020

Update?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 29, 2020

@ARMmbed/mbed-os-storage Please review

@0xc0170
Copy link
Contributor

0xc0170 commented May 8, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented May 8, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

@ccli8
Copy link
Contributor Author

ccli8 commented May 11, 2020

The bug this PR tries to fix can cause mbed-os-example-pelion to fail on all Nuvoton targets. Hopefully, this PR can also merge into mbed-os 5.15 release, in addition to 6.0 release. Per check, the bug has introduced into 5.15.2 and 5.15.3.

@0xc0170
Copy link
Contributor

0xc0170 commented May 12, 2020

@ccli8 Thanks for the heads up. Can you backport this to 5.15 branch as well?

@0xc0170 0xc0170 merged commit d5c9220 into ARMmbed:master May 12, 2020
@mergify mergify bot removed the ready for merge label May 12, 2020
@ccli8
Copy link
Contributor Author

ccli8 commented May 14, 2020

Thanks for the heads up. Can you backport this to 5.15 branch as well?

After further look, this related bug doesn't get into 5.15. Backport is unnecessary.

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

Successfully merging this pull request may close these issues.

5 participants