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

Flash api nRF52840 #4282

Merged
merged 7 commits into from May 26, 2017

Conversation

Projects
None yet
10 participants
@kl-cruz
Contributor

kl-cruz commented May 8, 2017

Notes:
Adding support for flashiap for nRF52840 devices. This is second pull request. This one was started from master branch (I did cherry pick of few commits). Tests were passed but... There is a nasty bug on master now: #4237 . It is necessary to remove first line in hex files and run tests with switch --run:

  1. compile:
    mbed test -n "tests-mbed_drivers-flashiap" -t GCC_ARM -m NRF52840_dk -c
  2. Remove first line in hex files
  3. Rerun tests without compilation:
    mbed test -n "tests-mbed_drivers-flashiap" -t GCC_ARM -m NRF52840_dk --run
  4. Run these three steps for "tests-mbed_hal-flash" tests

Status

READY

Related Issues

#4237

@kl-cruz kl-cruz force-pushed the kl-cruz:FlashAPI_nRF52840 branch May 8, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 8, 2017

LGTM

3 small things that can help:

  • not using [] in the first line in the commit, as git takes that as email header and with some commands (usually associated with emails) strip that
  • use commit msg powers to explain how it fixes an issue ([nRF52840] Fixed flashapi test and casting issue commit fixes the flashapi - why is it removing that header - is it unnecessary, compile error with some conditions, etc)
  • if there are tests, paste here results
@kl-cruz

This comment has been minimized.

Contributor

kl-cruz commented May 8, 2017

first dot: Ok, I'll remember.

second: this header is included in flash_api.h with proper preprocesor if

third:

C:\Repositories\mbed-os>mbed test -n "tests-mbed_hal-flash" -t GCC_ARM -m NRF52840_dk --run
mbedgt: greentea test automation tool ver. 1.2.5
mbedgt: test specification file 'C:\Repositories\mbed-os\BUILD\tests\NRF52840_dk\GCC_ARM\test_spec.
json' (specified with --test-spec option)
mbedgt: using 'C:\Repositories\mbed-os\BUILD\tests\NRF52840_dk\GCC_ARM\test_spec.json' from current
 directory!
mbedgt: detecting connected mbed-enabled devices...
mbedgt: detected 1 device
mbedgt: processing target 'NRF52840_DK' toolchain 'GCC_ARM' compatible platforms... (note: switch set to --parallel 1)
mbedgt: test case filter (specified with -n option)
        test filtered in 'tests-mbed_hal-flash'
mbedgt: running 1 test for platform 'NRF52840_DK' and toolchain 'GCC_ARM'
mbedgt: mbed-host-test-runner: started
mbedgt: checking for GCOV data...
mbedgt: test on hardware with target id: 1102022144203120414A4E393130323133323034B9FEDFDA
mbedgt: test suite 'tests-mbed_hal-flash' ............................................................ OK in 18.19 sec
        test case: 'Flash - buffer alignment test' ................................................... OK in 0.56 sec
        test case: 'Flash - clock and cache test' .................................................... OK in 0.16 sec
        test case: 'Flash - erase sector' ............................................................ OK in 0.13 sec
        test case: 'Flash - init' .................................................................... OK in 0.15 sec
        test case: 'Flash - mapping alignment' ....................................................... OK in 0.05 sec
        test case: 'Flash - program page' ............................................................ OK in 0.31 sec
mbedgt: test case summary: 6 passes, 0 failures
mbedgt: all tests finished!
mbedgt: shuffle seed: 0.7887740288
mbedgt: test suite report:
+---------------------+---------------+----------------------+--------+--------------------+-------------+
| target              | platform_name | test suite           | result | elapsed_time (sec) | copy_method |
+---------------------+---------------+----------------------+--------+--------------------+-------------+
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | OK     | 18.19              | shell       |
+---------------------+---------------+----------------------+--------+--------------------+-------------+
mbedgt: test suite results: 1 OK
mbedgt: test case report:
+---------------------+---------------+----------------------+-------------------------------+--------+--------+--------+--------------------+
| target              | platform_name | test suite           | test case                     | passed | failed | result | elapsed_time (sec) |
+---------------------+---------------+----------------------+-------------------------------+--------+--------+--------+--------------------+
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | Flash - buffer alignment test | 1      | 0      | OK     | 0.56               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | Flash - clock and cache test  | 1      | 0      | OK     | 0.16               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | Flash - erase sector          | 1      | 0      | OK     | 0.13               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | Flash - init                  | 1      | 0      | OK     | 0.15               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | Flash - mapping alignment     | 1      | 0      | OK     | 0.05               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal-flash | Flash - program page          | 1      | 0      | OK     | 0.31               |
+---------------------+---------------+----------------------+-------------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 6 OK
mbedgt: completed in 23.15 sec

C:\Repositories\mbed-os>mbed test -n "tests-mbed_drivers-flashiap" -t GCC_ARM -m NRF52840_dk --run
[mbed] WARNING: Could not find mbed program in current path "C:\Repositories\mbed-os".
[mbed] WARNING: You can fix this by calling "mbed new ." in the root of your program.
---
mbedgt: greentea test automation tool ver. 1.2.5
mbedgt: test specification file 'C:\Repositories\mbed-os-example-qspi\mbed-os\BUILD\tests\NRF52840_dk\GCC_ARM\test_spec.
json' (specified with --test-spec option)
mbedgt: using 'C:\Repositories\mbed-os\BUILD\tests\NRF52840_dk\GCC_ARM\test_spec.json' from current
 directory!
mbedgt: detecting connected mbed-enabled devices...
mbedgt: detected 1 device
mbedgt: processing target 'NRF52840_DK' toolchain 'GCC_ARM' compatible platforms... (note: switch set to --parallel 1)
mbedgt: test case filter (specified with -n option)
        test filtered in 'tests-mbed_drivers-flashiap'
mbedgt: running 1 test for platform 'NRF52840_DK' and toolchain 'GCC_ARM'
mbedgt: mbed-host-test-runner: started
mbedgt: checking for GCOV data...
mbedgt: test on hardware with target id: 1102022144203120414A4E393130323133323034B9FEDFDA
mbedgt: test suite 'tests-mbed_drivers-flashiap' ..................................................... OK in 16.54 sec
        test case: 'FlashIAP - init' ................................................................. OK in 0.05 sec
        test case: 'FlashIAP - program' .............................................................. OK in 0.18 sec
        test case: 'FlashIAP - program errors' ....................................................... OK in 0.05 sec
mbedgt: test case summary: 3 passes, 0 failures
mbedgt: all tests finished!
mbedgt: shuffle seed: 0.1185576987
mbedgt: test suite report:
+---------------------+---------------+-----------------------------+--------+--------------------+-------------+
| target              | platform_name | test suite                  | result | elapsed_time (sec) | copy_method |
+---------------------+---------------+-----------------------------+--------+--------------------+-------------+
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_drivers-flashiap | OK     | 16.54              | shell       |
+---------------------+---------------+-----------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 1 OK
mbedgt: test case report:
+---------------------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
| target              | platform_name | test suite                  | test case                 | passed | failed | result | elapsed_time (sec) |
+---------------------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_drivers-flashiap | FlashIAP - init           | 1      | 0      | OK     | 0.05               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_drivers-flashiap | FlashIAP - program        | 1      | 0      | OK     | 0.18               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_drivers-flashiap | FlashIAP - program errors | 1      | 0      | OK     | 0.05               |
+---------------------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 3 OK
mbedgt: completed in 20.64 sec
@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 8, 2017

@kl-cruz As these are few commits, and we shall start fixing these commit messages, c an you rebase this to remove [] ? Should be fairly quick?

From [nRF52840] Fixed flashapi test and casting issue to nRF52840: Fixed flashapi test and casting issue.

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented May 8, 2017

@kl-cruz Because we're on the subject of commit messages, we recommend our contributors follow Chris Beam’s seven rules of great commit messages to keep the commit history clear. I find commit.template to be particularly useful. Thanks for your contributions.

@0xc0170 How much does [] affect things? In our release notes, I've been changing : to [] to get rid of the awkward double colon, but if we're recommending that everyone use colons, I may have to consider changing that.

@sg- sg- added the needs: review label May 9, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 11, 2017

@0xc0170 How much does [] affect things? In our release notes, I've been changing : to [] to get rid of the awkward double colon, but if we're recommending that everyone use colons, I may have to consider changing that.

We will have to check to use a flag to overcome this removing [] from the commit line for git am. However, we agreed to use Subject: format, and you can find it in the git history, it will be captured in the docs soon (the docs we did was back then private and was not pushed to public repo, will be fixed).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 11, 2017

@kl-cruz It should be good to go. If you got time to improve the commit msg, let us know.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 15, 2017

Can you please rebase ? The refactor of nrf51 SDK (to include 52) was merged prior this one.

@0xc0170 0xc0170 added needs: work and removed ready for merge labels May 15, 2017

@pan-

This comment has been minimized.

Member

pan- commented May 16, 2017

@kl-cruz What would it requires to use the flash API while the softdevice is enabled ? Wouldn't it be possible to take advantage of sd_flash_page_erase and sd_flash_write, both of those call can be use whether the softdevice is enabled or inactive.

@kl-cruz

This comment has been minimized.

Contributor

kl-cruz commented May 16, 2017

I aligned this branch to master (prior to: #4245). Tests passed.

@pan- We can consider to use sd* related functions in next iteration

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented May 16, 2017

@pan- We don't have enough time to implement such a solution now (we are overwhelmed). As a compromise we want contribute the BLE-off solution - this still make e.g. flashing from SD card possible. As @kl-cruz mention we can enhance this latter.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 16, 2017

@kl-cruz It looks like you merged. Could you rebase instead?

@kl-cruz kl-cruz force-pushed the kl-cruz:FlashAPI_nRF52840 branch to 5cac624 May 17, 2017

@kl-cruz

This comment has been minimized.

Contributor

kl-cruz commented May 17, 2017

Rebased and changed brackets

@kl-cruz kl-cruz changed the title from Flash api n rf52840 to Flash api nRF52840 May 17, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 17, 2017

/morph test

@0xc0170 0xc0170 added the needs: CI label May 17, 2017

@0xc0170 0xc0170 removed the needs: work label May 17, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 17, 2017

@pan- Happy with this patch (the answers above provided) ?

@pan-

This comment has been minimized.

Member

pan- commented May 17, 2017

@0xc0170 lgtm

@mbed-bot

This comment has been minimized.

mbed-bot commented May 17, 2017

Result: NOT_BUILT

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

/morph test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 18, 2017

/morph test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented May 21, 2017

Re-triggering

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented May 21, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 290

All builds and test passed!

@adbridge adbridge added ready for merge and removed needs: CI labels May 22, 2017

@sg- sg- merged commit 58e8881 into ARMmbed:master May 26, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment