Skip to content

QSPI HAL tests: add DISCO_F413ZH support #7685

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 10, 2018
Merged

QSPI HAL tests: add DISCO_F413ZH support #7685

merged 3 commits into from
Aug 10, 2018

Conversation

maciejbocianski
Copy link
Contributor

@maciejbocianski maciejbocianski commented Aug 3, 2018 •
edited
Loading

Description

This PR adds (to qspi_hal test):

  • new qspi flash chip Micron N25Q128A config
  • support for DISCO_F413ZH

TODO:

  • implement enable dual/quad mode

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

0xc0170
0xc0170 requested changes Aug 3, 2018
View reviewed changes
TESTS/mbed_hal/qspi/main.cpp Outdated
@@ -321,10 +314,13 @@ void qspi_init_free_test(void)
flash_init(qspi);

#ifdef QSPI_TEST_LOG_FLASH_STATUS
printf("Status "); log_register(STATUS_REG, QSPI_STATUS_REG_SIZE, qspi);
printf("Config 0 "); log_register(CONFIG_REG0, QSPI_CONFIG_REG_0_SIZE, qspi);
printf("Status\r\n"); log_register(STATUS_REG, QSPI_STATUS_REG_SIZE, qspi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should either use config for t hat or configure terminal.

And I believe we should be using utest_printf rather. And not print this if all is OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should either use config for t hat or configure terminal.

Don't understand this one

And I believe we should be using utest_printf rather. And not print this if all is OK?

Sure I will change to utest_printf

TESTS/mbed_hal/qspi/flash_configs/N25Q128A_config.h Outdated
QSPI_STATUS_OK : QSPI_STATUS_ERROR)


#define DUAL_DISABLE_IMPLEMENTATION() \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why these are not inlined functions rather than large macro function definitions?

Copy link
Contributor Author

@maciejbocianski maciejbocianski Aug 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no specific reasons, can change macros to static inline functions
@0xc0170
There was the reason: to keep chip config header free from code dependencies

TESTS/mbed_hal/qspi/flash_configs/N25Q128A_config.h Outdated
QSPI_STATUS_OK : QSPI_STATUS_ERROR)


#define QUAD_ENABLE_IMPLEMENTATION() return QSPI_STATUS_OK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do these 3 macros do ? enable implementation = return STATUS OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote this PR is IN DEVELOPMENT, implementation will be added soon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that line, would wait with review :)

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 7, 2018

IN DEVELOPMENT DO NOT MERGE

What is to be done here?

@maciejbocianski
Copy link
Contributor Author

maciejbocianski commented Aug 8, 2018 •
edited
Loading

Now the test works in extended SPI mode.
dual/quad mode is not working yet, have some problems with enabling it

@maciejbocianski
Copy link
Contributor Author

But I think we could merge this PR in the current form after code review fixes.
As I wrote test works fine in extended SPI mode.
Fix for enabling dual/quad mode will be provided in separate PR.
@0xc0170 What do you think

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 8, 2018

it's a branch - enabling features and extending tests in steps is fine .

@maciejbocianski maciejbocianski force-pushed the qspi_tests_DISCO_F413ZH branch from 0b2dd11 to 6c0c517 Compare August 9, 2018 08:54
@maciejbocianski
Copy link
Contributor Author

@0xc0170
code was updated according suggestions, can proceed with CI

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 9, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 9, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Aug 9, 2018

Exporter Build : SUCCESS

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

@mbed-ci
Copy link

mbed-ci commented Aug 9, 2018

Test : FAILURE

Build number : 2502
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7685/2502

@maciejbocianski
Copy link
Contributor Author

maciejbocianski commented Aug 10, 2018 •
edited
Loading

tests-mbed_drivers-timer:

[1533848813.22][CONN][RXD] >>> Running case #11: 'Test: Timer - time measurement 1 ms.'... [1533848813.32][CONN][RXD] :730::FAIL: Expected 0.001000 Was 0.001633 
[1533848813.32][CONN][INF] found KV pair in stream: {{__testcase_start;Test: Timer - time measurement 1 ms.}}, queued... 
[1533848813.42][CONN][INF] found KV pair in stream: {{__testcase_finish;Test: Timer - time measurement 1 ms.;0;1}}, queued... 
[1533848813.52][CONN][RXD] >>> 'Test: Timer - time measurement 1 ms.': 0 passed, 1 failed with reason 'Assertion Failed'

I will investigate it in parallel, please restart morph
Fix is on the way #7752

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 10, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Aug 10, 2018

Test : SUCCESS

Build number : 2506
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7685/2506

@0xc0170 0xc0170 removed the needs: CI label Aug 10, 2018
@0xc0170 0xc0170 merged this pull request into ARMmbed:feature-qspi Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: feature branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maciejbocianski @0xc0170 @mbed-ci @cmonr