Skip to content

Add GreenTea SPI communication test - wip. #8216

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
Closed
wants to merge 1 commit into from
Closed

Add GreenTea SPI communication test - wip. #8216

wants to merge 1 commit into from

Conversation

mprse
Copy link
Contributor

@mprse mprse commented Sep 21, 2018 •
edited
Loading

Description

Provide SPI communication test and test header file based on the new HAL API description.

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 is provided in the test.

PR Description
Add SPI test and test header file. #7976 SPI basic test
Add GreeTea SPI communication test. #8216 Green Tea SPI communication test
Add Ice Tea SPI communication test. #8443 Ice Tea SPI communication test

Please assign @ithinuel as a reviewer.

Pull request type

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

@mprse
Add SPI communication test.
aa66bde 
This test cannot be run on CI. It is designed for manual testing.
Detailed description is provided in the test.
@mprse mprse changed the base branch from master to feature-hal-spec-spi September 21, 2018 11:08
@0xc0170 0xc0170 requested a review from ithinuel September 21, 2018 12:15
@jamesbeyond
Copy link
Contributor

How about put a readme.md in the folder, rather than writing all the test descriptions in the code comments

ithinuel
ithinuel suggested changes Oct 3, 2018
View reviewed changes
Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, this file actually contains 2 "firmware".
To keep things clear & tidy, what do you think about having them splitted in distinct files and use a shared header for the defines & the structures ?

TESTS/mbed_hal/spi_com/main.cpp
} else if (config.symbol_size <= 16) {
slave_transfer<uint16_t>(&spi_slave, &config);
} else {
slave_transfer<uint16_t>(&spi_slave, &config);
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean uint32_t in the last branch ?

TESTS/mbed_hal/spi_com/main.cpp
#define BUILD_NONE 0

/* Select build type (BUILD_MASTER, BUILD_SLAVE, BUILD_NONE). */
#define BUILD_TYPE BUILD_MASTER
Copy link
Member

Choose a reason for hiding this comment

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

This forces the BUILD_TYPE to BUILD_MASTER. Is that intended ?
What about giviong the BUILD_TYPE as command line argument and only build/run this test if BUILD_TYPE is defined to make it transparent in CI ?

@mprse mprse changed the title Add SPI communication test. Add GreeTea SPI communication test. Oct 16, 2018
This was referenced Oct 16, 2018
Merged
Merged
@cmonr
Copy link
Contributor

cmonr commented Oct 23, 2018

@mprse Any more work being done on this?

@ARMmbed/mbed-os-maintainers This PR's unittests also needs to be restarted.

@JoeNguyen93 JoeNguyen93 mentioned this pull request Oct 25, 2018
Closed
@mprse mprse changed the title Add GreeTea SPI communication test. Add GreeTea SPI communication test - wip. Oct 26, 2018
@cmonr cmonr changed the title Add GreeTea SPI communication test - wip. Add GreenTea SPI communication test - wip. Oct 29, 2018
@cmonr
Copy link
Contributor

cmonr commented Oct 29, 2018

Closing this PR appears to have idled for too long.

Feel free to reopen once updates are available.

@cmonr cmonr closed this Oct 29, 2018
@cmonr cmonr removed the needs: work label Oct 29, 2018
ithinuel
ithinuel reviewed Nov 2, 2018
View reviewed changes
TESTS/mbed_hal/spi_com/main.cpp
* f) When both sides can handle the specified configuration, then slave and master
* loads the configuration and perform communication test.
*
* 3. Steps to run the test
Copy link
Member

Choose a reason for hiding this comment

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

  1. comes before 3. :D

@mprse mprse mentioned this pull request Nov 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ithinuel ithinuel ithinuel requested 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.
5 participants
@mprse @jamesbeyond @cmonr @ithinuel @0xc0170