Skip to content
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

Add one board Green Tea SPI Communication Test #8919

Merged
merged 5 commits into from
Apr 15, 2019

Conversation

mprse
Copy link
Contributor

@mprse mprse commented Nov 30, 2018

Description

Provide one board SPI communication test which uses Green Tea Framework.

This PR can not be merged until PR which defines the new HAL API and adds at least one example implementation is merged. Probably then the test will have to be adapted to the final API version.

Detailed description can be found in README.md file in the test folder.

List of PRs with related tests

PR Description
RFC SPI: Reference Implementation. #8445 New HAL API definition and example implementations for K66F anf NUCLEO_F429ZI
Add SPI test and test header file. #7976 SPI Green Tea basic test
Add GreeTea SPI communication test. #8216 Two boards Green Tea SPI communication test (closed)
Add Ice Tea SPI communication test. #8443 Two boards Ice Tea SPI communication test
Add one board Green Tea SPI Communication Test #8919 One board Green Tea SPI communication test

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[X] Test update
[ ] Breaking change

@mprse mprse force-pushed the spi_com_one_board_test_pr branch 2 times, most recently from c17a0a8 to e8c61c3 Compare November 30, 2018 10:53
@mprse
Copy link
Contributor Author

mprse commented Nov 30, 2018

Test results for the example implementation of SPI driver for NUCLEO_F429ZI:

image

@cmonr cmonr force-pushed the feature-hal-spec-spi branch from a290cfe to 0892667 Compare December 4, 2018 22:23
@0xc0170 0xc0170 requested a review from a team December 5, 2018 08:58
@jamesbeyond
Copy link
Contributor

jamesbeyond commented Dec 6, 2018

Well done @mprse !

@screamerbg or @MarceloSalazar need to have a look at this as well. please add them as reviewer.

@screamerbg
Copy link
Contributor

Thanks! I'm travelling today, but will try it tomorrow morning

#ifndef MBED_SPI_PINS_H
#define MBED_SPI_PINS_H

#if defined(TARGET_NUCLEO_F429ZI)
Copy link
Contributor

Choose a reason for hiding this comment

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

If every target has to add to this config file, and pins.h, is this file going to grow too big and a quite messy? Is there a better way to achieve the same function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a discussion with @ithinuel about this. I was thinking to use app_config.json config file. But as far as I remember @ithinuel pointed out that json is not used our green-tea tests and solution with header files is already used in QSPI tests. This is why we decided to use this solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. @ARMmbed/mbed-os-core Any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed this with @maciejbocianski and QSPI test doesn't use header files for pins definition, but only for flash config. In QSPI pins have been unified across all targets with QSPI feature enabled. So in this case we could also define SPI pins in target's PinNames.h file. To run one board test we require two SPI interfaces, so I suggest to add the following definitions:
SPI0_MOSI, SPI0_MISO, SPI0_SCK, SPI0_CS
SPI1_MOSI, SPI1_MISO, SPI1_SCK, SPI1_CS

cc @c1728p9 @0xc0170 @donatieng @jamesbeyond

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks better than these pins in the header file, to define SPIx pins

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds a better idea to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test has been updated:

  • removed header files with pins definition
  • provided pins definition in target's PinNames.h file
  • adapted pin names in the test
  • adapted documentation.

@mprse mprse force-pushed the spi_com_one_board_test_pr branch 3 times, most recently from be424ad to f24ab96 Compare December 13, 2018 13:39
@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

@0xc0170 @jamesbeyond @MarceloSalazar @screamerbg When y'all get a chance.

@cmonr
Copy link
Contributor

cmonr commented Dec 21, 2018

Depends on at least #8445

@cmonr
Copy link
Contributor

cmonr commented Jan 4, 2019

@mprse Does this depend on anything else aside for #8445?

@mprse
Copy link
Contributor Author

mprse commented Jan 5, 2019

I need to clean up spi pins definiotion and probably I will remove the definitions from this PR and add them to PR #7976. So they are consistent. Then #7976 will have to go first.

@mprse mprse force-pushed the spi_com_one_board_test_pr branch from f24ab96 to 9b5d3f0 Compare January 8, 2019 12:25
@mprse
Copy link
Contributor Author

mprse commented Jan 8, 2019

This test is ready for review, but requires SPI pins definition from #7976.

@mprse mprse force-pushed the spi_com_one_board_test_pr branch from 9b5d3f0 to e8c2e22 Compare January 9, 2019 09:33
@0xc0170 0xc0170 force-pushed the feature-hal-spec-spi branch from f3429a3 to 9b1ccaa Compare January 9, 2019 16:16
@mprse mprse force-pushed the spi_com_one_board_test_pr branch from e8c2e22 to 86aa9c9 Compare January 10, 2019 06:22
@mprse
Copy link
Contributor Author

mprse commented Jan 10, 2019

Rebased after spi feature branch has been rebased on master to solve Travis issue.

@mprse
Copy link
Contributor Author

mprse commented Apr 5, 2019

The build results are now much better! But still, there are some problems on ARM6 and IAR8.

IAR8 failures (SDT64B):

Compile [ 78.7%]: kv_config.cpp
[Error] kv_config.cpp@553,31: [Pe020]: identifier "SDBlockDevice" is undefined

[Error] SystemStorage.cpp@139,31: [Pe020]: identifier "SDBlockDevice" is undefined

#if COMPONENT_SD
bd_addr_t aligned_end_address;
bd_addr_t aligned_start_address;
static SDBlockDevice bd(
MBED_CONF_SD_SPI_MOSI,
MBED_CONF_SD_SPI_MISO,
MBED_CONF_SD_SPI_CLK,
MBED_CONF_SD_SPI_CS
);

#elif COMPONENT_SD
static SDBlockDevice default_bd(
MBED_CONF_SD_SPI_MOSI,
MBED_CONF_SD_SPI_MISO,
MBED_CONF_SD_SPI_CLK,
MBED_CONF_SD_SPI_CS
);

Looking at the above code snippets this code should not be compiled according to the configuration (SD component is not provided for SDT64B). SDT64B inherits from K64F, but on the feature branch SD is also disabled for K64F:

mbed-os/targets/targets.json

Lines 1522 to 1530 in 71c84e8

"SDT64B": {
"inherits": ["K64F"],
"components_add": ["FLASHIAP"],
"extra_labels_add": ["K64F"],
"extra_labels_remove": ["FRDM"],
"components_remove": ["SD"],
"supported_form_factors": [],
"detect_code": ["3105"]
},

@0xc0170 @cmonr Do you maybe have some ideas?

@mprse
Copy link
Contributor Author

mprse commented Apr 5, 2019

Summary of ARM6 failures below:

ARM_CM3DS_MPS2::ARMC6::TESTS-MBED_HAL-PINMAP
[Error] @0,0: L6218E: Undefined symbol analogin_pinmap (referred from BUILD/tests/ARM_CM3DS_MPS2/ARMC6/TESTS/mbed_hal/pinmap/TESTS/mbed_hal/pinmap/main.o).

ARMC6::DISCO_L475VG_IOT01A
[Error] BufferedSpi.h@71,28: unknown class name 'SPI'; did you mean 'QSPI'?
[DEBUG] Return: 1
[DEBUG] Output: In file included from ./wifi-ism43362/ISM43362/ATParser/ATParser.cpp:21:

ARMC6::FVP_MPS2_M0
ARMC6::FVP_MPS2_M0P
ARMC6::FVP_MPS2_M3
ARMC6::FVP_MPS2_M4
ARMC6::FVP_MPS2_M7
[Error] serial_api.c@403,12: use of undeclared identifier 'PinMap_UART_CTS'; did you mean 'PinMap_UART_TX'?


K64F:ARMC6
Scan: mbed-os-example-attestation
[ERROR] library 'psa-services' requires 'flashiap-block-device' which is not present

ARMC6::SDT64B
[Error] kv_config.cpp@553,12: unknown type name 'SDBlockDevice'; did you mean 'BlockDevice'?
[Error] SystemStorage.cpp@139,12: unknown type name 'SDBlockDevice'; did you mean 'BlockDevice'?

I'm looking into this now.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 5, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 5, 2019

Test run: FAILED

Summary: 3 of 9 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR8

@mprse mprse force-pushed the spi_com_one_board_test_pr branch from 5527d35 to 5f4a009 Compare April 7, 2019 19:29
@mprse
Copy link
Contributor Author

mprse commented Apr 7, 2019

At this point, we have two problems on the feature branch which I don't know how to fix. These problems probably are related to CI system itself and can not be handled by changing mbed config.

  1. DISCO_L475VG_IOT01A
[Error] BufferedSpi.h@71,28: unknown class name 'SPI'; did you mean 'QSPI'?
[DEBUG] Return: 1
[DEBUG] Output: In file included from ./wifi-ism43362/ISM43362/ATParser/ATParser.cpp:21:

This board has a wifi module connected by the SPI interface. On the feature branch, SPI is disabled for this board, but the driver is included during the build process. This causes an error since SPI is not available.

  1. SDT64B
[Error] kv_config.cpp@553,12: unknown type name 'SDBlockDevice'; did you mean 'BlockDevice'?
[Error] SystemStorage.cpp@139,12: unknown type name 'SDBlockDevice'; did you mean 'BlockDevice'?

Not sure what is going on here. This board does not have SD component in targets.json, but the failures are for code dedicated only for targets with COMPONENT_SD symbol defined. More details here: #8919 (comment)

The SPI feature branch is broken for a very long time. @0xc0170 @cmonr Could you help me to solve above problems.

cc @donatieng @jamesbeyond

Edit:
I was able to reproduce SDT64B failure, by running:
mbed compile -m SDT64B -t GCC_ARM --library with enabled 'SD' component in targets.json for this board.
With SD component disabled build completes successfully. Is it possible that something overrides the configuration (adding SD component) on CI?

Edit:

  1. DISCO_L475VG_IOT01A
    The following PR should fix the problem: https://github.com/ARMmbed/mbed-os-ci/pull/528

  2. SDT64B
    It looks like SD component to SDT64B is added here:
    https://github.com/ARMmbed/mbed-os-ci/blob/master/resources/configurations/mbed_app.json#L90

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 8, 2019

@ARMmbed/mbed-os-test Please review above comment

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 8, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Apr 8, 2019

Test run: FAILED

Summary: 2 of 13 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_greentea-test

@mprse
Copy link
Contributor Author

mprse commented Apr 9, 2019

So the results are better, but still few problems to solve:

  1. mbed-cloud-client-example build failed for K64F . Since cloud client requires SD card which requires SPI this example should be disabled for this board. @0xc0170 Can this be done via some mbed config file? I checked examples.json but nothing there.

  2. tests-mbed_hal-spi_com test is failing on K66F and NUCLEO_F429ZI and all compilers. This one is easy to fix. This test is for manual testing only(requires wire connection) and I forgot to switch enable flag off before pushing the test.

  3. K66F-ARMC6.components-storage-blockdevice-component_sd-tests-filesystem-parallel

Under investigation.

  1. K66F-ARMC6.components-storage-blockdevice-component_sd-tests-filesystem-files

Under investigation.

mprse added 4 commits April 10, 2019 09:04
ANALOGIN is related to SPI. If SPI is not supported, then ANALOGIN also.

This change should fix:
ARM_CM3DS_MPS2::ARMC6::TESTS-MBED_HAL-PINMAP
[Error] @0,0: L6218E: Undefined symbol analogin_pinmap (referred from BUILD/tests/ARM_CM3DS_MPS2/ARMC6/TESTS/mbed_hal/pinmap/TESTS/mbed_hal/pinmap/main.o).
Looks like `FVP_MPS2 family` has SERIAL_FC feature enabled, but does not provide pins definition for `PinMap_UART_CTS`, `PinMap_UART_RTS`.

This should fix the following failures:
ARMC6::FVP_MPS2_M0
ARMC6::FVP_MPS2_M0P
ARMC6::FVP_MPS2_M3
ARMC6::FVP_MPS2_M4
ARMC6::FVP_MPS2_M7
[Error] serial_api.c@403,12: use of undeclared identifier 'PinMap_UART_CTS'; did you mean 'PinMap_UART_TX'?
…ples.

Not supported on the spi feature branch.
@mprse mprse force-pushed the spi_com_one_board_test_pr branch from 5f4a009 to d14c50a Compare April 10, 2019 07:05
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2019

Ci restarted

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 12, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Apr 12, 2019

Test run: FAILED

Summary: 5 of 7 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@mprse mprse force-pushed the spi_com_one_board_test_pr branch from 8594d8a to 1676243 Compare April 12, 2019 12:35
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 12, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 12, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 8
Build artifacts

@0xc0170 0xc0170 merged commit 335a65f into ARMmbed:feature-hal-spec-spi Apr 15, 2019
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.

8 participants