Add SPI basic test and test header file. #7976

mprse · 2018-09-04T06:17:13Z

Description

Provide SPI test and test header file based on the new HAL API description provided here:
https://github.com/ithinuel/mbed-os/blob/rfc-spi-overhaul/hal/rfcs/0000-spi-overhaul.md

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.

This test is a first part of entire SPI testing scheme which provides non-communication, full-automated test which will run on CI.

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

Please assign @ithinuel as a reviewer.

Pull request type

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

cmonr · 2018-10-12T14:12:19Z

@ARMmbed/mbed-os-hal ?

fkjagodzinski

Also please run astyle on files from this PR.

Ok, please ignore my comments regarding the outdated API. I checked existing header instead of the markdown doc.

fkjagodzinski · 2018-10-16T07:35:46Z

TESTS/mbed_hal/spi/main.cpp

+
+ TEST_ASSERT(capabilities.minimum_frequency <= FREQ_200KHZ);
+ TEST_ASSERT(capabilities.maximum_frequency >= FREQ_2MHZ);
+ TEST_ASSERT_TRUE(capabilities.symbol_length & CAPABILITY_WORD_LENGTH_8);


It might be useful to comment this is actually defined behavior / requirement from the docs.

fkjagodzinski · 2018-10-16T07:40:10Z

TESTS/mbed_hal/spi/main.cpp

+ spi_t spi_obj =
+ { 0 };
+ spi_capabilities_t capabilities =
+ { 0 };


These lines break for no reason.

fkjagodzinski · 2018-10-16T07:53:58Z

TESTS/mbed_hal/spi/main.cpp

+ }
+
+ /* Indicate that test case has been successfully executed. */
+ TEST_ASSERT_TRUE(true);


This looks a little odd. I think this line is unnecessary for the test case to be valid, but isn't there anything to check during code execution?

TESTS/mbed_hal/spi/main.cpp

+
+ spi_format(&spi_obj, 8, SPI_MODE_IDLE_LOW_SAMPLE_FIRST_EDGE, SPI_BIT_ORDERING_MSB_FIRST);
+
+ real_freq = spi_frequency(&spi_obj, FREQ_200KHZ);


TESTS/mbed_hal/spi/main.cpp

+ p_fill_sym = (void*) &fill_symbol_32;
+ }
+
+ const uint32_t sym_cnt = spi_transfer(&spi_obj, p_tx_buf, BUF_SIZE, p_rx_buf, BUF_SIZE, p_fill_sym);


TESTS/mbed_hal/spi/main.cpp

+ handler_called = false;
+ expected_event.transfered = 0; // Success will have to be indicated here
+
+ const bool async_status = spi_transfer_async(&spi_obj, p_tx_buf, BUF_SIZE, p_rx_buf, BUF_SIZE, p_fill_sym, (void*)&some_ctx, dma_modes[mode_id]);


cmonr · 2018-10-19T19:34:27Z

@mprse @fkjagodzinski
How is #8443 relate to this? Does this still need to be reviewed, or is this considered good to move forward?

cmonr · 2018-10-29T14:14:25Z

@mprse @fkjagodzinski

Any updates? If not, then we'll have to close due to inactivity.

mprse · 2018-11-02T12:20:44Z

@cmonr Please do not close. This one is needed.

mprse · 2018-11-05T14:23:43Z

Refreshed the test.
Reference implementation required by this PR is still not merged, but this one is ready. Please review.

cmonr · 2018-11-09T21:59:39Z

@mprse I wonder if this branch needs to be rebased, since it's not passing unittests.

mprse · 2018-11-13T12:21:00Z

@mprse I wonder if this branch needs to be rebased, since it's not passing unittests.

Probably its not passing since there is no new SPI HAL API on the branch.

cmonr · 2018-11-15T17:47:16Z

@mprse Ah, gotcha. So this is waiting on #8445?

Required for testing.

mprse · 2019-02-04T09:56:54Z

@theamirocohen Thank you for the solution!

@0xc0170 Can we start CI again?

0xc0170 · 2019-02-04T12:29:58Z

CI restarted

0xc0170 · 2019-02-04T16:27:33Z

CI restarted (Java error, if repeats, will investigate with CI team)

mbed-ci · 2019-02-04T19:39:26Z

Test run: FAILED

Summary: 4 of 8 test jobs failed
Build number : 10
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-ARM
jenkins-ci/mbed-os-ci_build-IAR

mprse · 2019-02-04T20:46:56Z

Now the results are very strange. In the last commit I modified examples.json file as suggested @theamirocohen. I made the list of targets empty for the following examples: mbed-os-example-sd-driver and mbed-os-example-bootloader as those can't be build on spi feature branch (at least now). So the mbed-os-example-sd-driver config looks as follows:

https://github.com/ARMmbed/mbed-os/blob/3d439338d3aa8b363903cb673753fbf133ef63fc/tools/test/examples/examples.json#L356-L368

As a result of these changes (targets list is empty) it looks like the examples have been built for all targets, so empty targets array means all? @0xc0170 @cmonr Is that intended behaviour?

If this is intended behaviour then I believe I should set compile flag to false for these examples also. Is that correct and sufficient?

theamirocohen · 2019-02-05T07:15:34Z

Now the results are very strange. In the last commit I modified examples.json file as suggested @theamirocohen. I made the list of targets empty for the following examples: mbed-os-example-sd-driver and mbed-os-example-bootloader as those can't be build on spi feature branch (at least now). So the mbed-os-example-sd-driver config looks as follows:

mbed-os/tools/test/examples/examples.json

Lines 356 to 368 in 3d43933
{
"name": "mbed-os-example-sd-driver",
"github":"https://github.com/ARMmbed/mbed-os-example-sd-driver",
"mbed": [],
"test-repo-source": "github",
"features" : [],
"targets" : [],
"toolchains" : [],
"exporters": [],
"compile" : true,
"export": true,
"auto-update" : true
}

As a result of these changes (targets list is empty) it looks like the examples have been built for all targets, so empty targets array means all? @0xc0170 @cmonr Is that intended behaviour?

If this is intended behaviour then I believe I should set compile flag to false for these examples also. Is that correct and sufficient?

Hi,
I guess that by removing K64F from "targets" you allowed all boards to be built.
My intention was that you completely remove the sd-driver-example.

mbed-os-example-sd-driver: on spi feature branch SD componen is disabled for K64F - disable this example. mbed-os-example-bootloader: example is using SPI pins which are now undefined on NUCLEO_F429ZI - disable this example.

mprse · 2019-02-05T07:27:41Z

Fixed as suggested by @theamirocohen.

@0xc0170 Can we try again?

0xc0170 · 2019-02-05T09:44:15Z

CI started

mbed-ci · 2019-02-05T11:54:43Z

Test run: SUCCESS

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

mprse · 2019-02-05T13:21:36Z

This one should be ready to go. Later changes (after review) were related to CI problems.

mprse · 2019-02-06T07:00:14Z

@donatieng This one should be ready to go. After this one is merged review and CI here is needed:
#8919 .

NirSonnenschein · 2019-02-06T07:41:07Z

@ithinuel @MarceloSalazar @screamerbg would you like to review?

donatieng · 2019-02-07T13:19:57Z

Thanks a lot @mprse - @ARMmbed/mbed-os-maintainers I'm happy to get this one merged - if @MarceloSalazar and @screamerbg see missing tests we can add them in a following PR

0xc0170 · 2019-02-07T14:02:46Z

TESTS/mbed_hal/spi/main.cpp

@@ -0,0 +1,561 @@
+/*
+ * Copyright (c) 2018 ARM Limited
+ *


Can be fixed in separate PR, adding SPDX identifiers for new files

0xc0170 · 2019-02-07T14:05:35Z

I'm happy to get this one merged - if @MarceloSalazar and @screamerbg see missing tests we can add them in a following PR

As this is targeting feature branch, please add your comments when you are able to, they will be addressed. We mark this as ready for merge now to be integrated in the next 24 hours.

0xc0170 added the needs: review label Sep 4, 2018

0xc0170 requested a review from a team September 4, 2018 11:40

mprse mentioned this pull request Sep 21, 2018

Add GreenTea SPI communication test - wip. #8216

Closed

fkjagodzinski suggested changes Oct 16, 2018

View reviewed changes

mprse mentioned this pull request Oct 16, 2018

Add Ice Tea SPI communication test. #8443

Merged

cmonr added the release-version: feature branch label Oct 25, 2018

JoeNguyen93 mentioned this pull request Oct 25, 2018

Add Ice Tea SPI communication test. softagram/mbed-os#6

Closed

0xc0170 added needs: work and removed needs: review labels Oct 26, 2018

donatieng requested review from ithinuel, c1728p9 and jamesbeyond October 31, 2018 14:31

mprse force-pushed the spi_feature_branch_test_ci branch from 5f67d81 to 788ba7b Compare November 5, 2018 14:21

cmonr added needs: review and removed needs: work labels Nov 5, 2018

mprse force-pushed the spi_feature_branch_test_ci branch from 788ba7b to ab8fcce Compare November 6, 2018 06:48

0xc0170 force-pushed the feature-hal-spec-spi branch from 689890e to 85413ff Compare November 12, 2018 17:38

adbridge added needs: work and removed needs: review labels Nov 14, 2018

cmonr added the needs: preceding PR label Nov 15, 2018

0xc0170 force-pushed the feature-hal-spec-spi branch from 85413ff to a290cfe Compare November 29, 2018 11:19

mprse force-pushed the spi_feature_branch_test_ci branch from ab8fcce to 2d33887 Compare November 29, 2018 12:05

mprse added 4 commits February 4, 2019 10:54

NUCLEO_F429ZI: Fix SPI ref impl - add null check in spi_transfer()

450ebf9

Define SPI pins for K66F.

01dce88

Required for testing.

Disable SPI support for all targets except K66F, NUCLEO_F429ZI

831722c

add DEVICE_SPI guard to TT targets

26f2d85

mprse force-pushed the spi_feature_branch_test_ci branch from 5ba562e to 3d43933 Compare February 4, 2019 09:54

Disable examples unsupported on spi feature branch.

9a3e96d

mbed-os-example-sd-driver: on spi feature branch SD componen is disabled for K64F - disable this example. mbed-os-example-bootloader: example is using SPI pins which are now undefined on NUCLEO_F429ZI - disable this example.

mprse force-pushed the spi_feature_branch_test_ci branch from 3d43933 to 9a3e96d Compare February 5, 2019 07:25

NirSonnenschein added needs: review and removed needs: work labels Feb 6, 2019

0xc0170 reviewed Feb 7, 2019

View reviewed changes

0xc0170 approved these changes Feb 7, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Feb 7, 2019

NirSonnenschein merged commit 1f5e523 into ARMmbed:feature-hal-spec-spi Feb 7, 2019

NirSonnenschein removed the ready for merge label Feb 7, 2019

mprse mentioned this pull request Feb 19, 2019

SPI::write: fix number of transmitted symbols. #9624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SPI basic test and test header file. #7976

Add SPI basic test and test header file. #7976

mprse commented Sep 4, 2018 •

edited

Loading

cmonr commented Oct 12, 2018

fkjagodzinski left a comment •

edited

Loading

fkjagodzinski Oct 16, 2018

fkjagodzinski Oct 16, 2018

fkjagodzinski Oct 16, 2018

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

cmonr commented Oct 19, 2018

cmonr commented Oct 29, 2018

mprse commented Nov 2, 2018

mprse commented Nov 5, 2018

cmonr commented Nov 9, 2018

mprse commented Nov 13, 2018

cmonr commented Nov 15, 2018

mprse commented Feb 4, 2019

0xc0170 commented Feb 4, 2019

0xc0170 commented Feb 4, 2019

mbed-ci commented Feb 4, 2019

mprse commented Feb 4, 2019 •

edited

Loading

theamirocohen commented Feb 5, 2019

mprse commented Feb 5, 2019

0xc0170 commented Feb 5, 2019

mbed-ci commented Feb 5, 2019

mprse commented Feb 5, 2019

mprse commented Feb 6, 2019 •

edited

Loading

NirSonnenschein commented Feb 6, 2019

donatieng commented Feb 7, 2019

0xc0170 Feb 7, 2019

0xc0170 commented Feb 7, 2019


		spi_format(&spi_obj, 8, SPI_MODE_IDLE_LOW_SAMPLE_FIRST_EDGE, SPI_BIT_ORDERING_MSB_FIRST);

		real_freq = spi_frequency(&spi_obj, FREQ_200KHZ);

Add SPI basic test and test header file. #7976

Add SPI basic test and test header file. #7976

Conversation

mprse commented Sep 4, 2018 • edited Loading

Description

Pull request type

cmonr commented Oct 12, 2018

fkjagodzinski left a comment • edited Loading

Choose a reason for hiding this comment

fkjagodzinski Oct 16, 2018

Choose a reason for hiding this comment

fkjagodzinski Oct 16, 2018

Choose a reason for hiding this comment

fkjagodzinski Oct 16, 2018

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

cmonr commented Oct 19, 2018

cmonr commented Oct 29, 2018

mprse commented Nov 2, 2018

mprse commented Nov 5, 2018

cmonr commented Nov 9, 2018

mprse commented Nov 13, 2018

cmonr commented Nov 15, 2018

mprse commented Feb 4, 2019

0xc0170 commented Feb 4, 2019

0xc0170 commented Feb 4, 2019

mbed-ci commented Feb 4, 2019

Test run: FAILED

mprse commented Feb 4, 2019 • edited Loading

theamirocohen commented Feb 5, 2019

mprse commented Feb 5, 2019

0xc0170 commented Feb 5, 2019

mbed-ci commented Feb 5, 2019

Test run: SUCCESS

mprse commented Feb 5, 2019

mprse commented Feb 6, 2019 • edited Loading

NirSonnenschein commented Feb 6, 2019

donatieng commented Feb 7, 2019

0xc0170 Feb 7, 2019

Choose a reason for hiding this comment

0xc0170 commented Feb 7, 2019

mprse commented Sep 4, 2018 •

edited

Loading

fkjagodzinski left a comment •

edited

Loading

mprse commented Feb 4, 2019 •

edited

Loading

mprse commented Feb 6, 2019 •

edited

Loading