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 HAL CRC test and header file #6831

Merged
merged 7 commits into from Jun 13, 2018

Conversation

Projects
None yet
8 participants
@mprse
Member

mprse commented May 7, 2018

Description

Add CRC HAL API test and test header file.

Since the testing requirements were not provided in PR #6708 I allowed myself to define them here.

@scartmell-arm Please review the test and testing requirements and update if something is wrong or should be additionally tested.

Status

READY

Related PRs

#6708 - provides CRC HAL API definition and example driver for K64F. Needs to be merged before this one.

Pull request type

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

This comment has been minimized.

Contributor

cmonr commented May 24, 2018

@scartmell-arm hal/crc_api.h exists in both this PR and #6708. Was that the intention, or is this PR only suppose to contain tests?

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented May 25, 2018

@scartmell-arm hal/crc_api.h exists in both this PR and #6708. Was that the intention, or is this PR only suppose to contain tests?

This PR should only contain tests, and use the header in #6708

@mprse

This comment has been minimized.

Member

mprse commented May 25, 2018

@scartmell-arm hal/crc_api.h exists in both this PR and #6708. Was that the intention, or is this PR only suppose to contain tests?

This PR should only contain tests, and use the header in #6708

This PR provides test and header file. The header file has been added since original header file (from PR #6708) dose not provide requirements section in form which is common for all other HAL APIs (e.g.

/**
* \defgroup hal_ticker_shared Ticker Hal
* Low level interface to the ticker of a target
*
* # Defined behavior
* * The function ticker_init is safe to call repeatedly - Verified by test ::ticker_init_test
* * The function ticker_init allows the ticker to keep counting and disables the ticker interrupt - Verified by test ::ticker_init_test
* * Ticker frequency is non-zero and counter is at least 8 bits - Verified by ::ticker_info_test
* * The ticker rolls over at (1 << bits) and continues counting starting from 0 - Verified by ::ticker_overflow_test
* * The ticker counts at the specified frequency +- 10% - Verified by ::ticker_frequency_test
* * The ticker increments by 1 each tick - Verified by ::ticker_increment_test
* * The ticker interrupt fires only when the ticker times increments to or past the value set by ticker_set_interrupt.
* Verified by ::ticker_interrupt_test and ::ticker_past_test
* * It is safe to call ticker_set_interrupt repeatedly before the handler is called - Verified by ::ticker_repeat_reschedule_test
* * The function ticker_fire_interrupt causes ticker_irq_handler to be called immediately from interrupt context -
* Verified by ::ticker_fire_now_test
* * The ticker operations ticker_read, ticker_clear_interrupt, ticker_set_interrupt and ticker_fire_interrupt
* take less than 20us to complete - Verified by ::ticker_speed_test
*
* # Undefined behavior
* * Calling any function other than ticker_init before the initialization of the ticker
* * Whether ticker_irq_handler is called a second time if the time wraps and matches the value set by ticker_set_interrupt again
* * Calling ticker_set_interrupt with a value that has more than the supported number of bits
* * Calling any function other than us_ticker_init after calling us_ticker_free
*
* # Potential bugs
* * Drift due to reschedule - Verified by ::ticker_repeat_reschedule_test
* * Incorrect overflow handling of timers - Verified by ::ticker_overflow_test
* * Interrupting at a time of 0 - Verified by ::ticker_overflow_test
* * Interrupt triggered more than once - Verified by ::ticker_interrupt_test
*
* @ingroup hal_us_ticker
* @ingroup hal_lp_ticker
).

I allowed myself to create such requirements for testing purposes and requested @scartmell-arm for review.

So this PR adds only testing requirements to original header file.

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented May 25, 2018

In that case, it could be added as part of this PR being merged?

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 25, 2018

@mprse Is this going to need a rebase then once the other PR gets merged ? Also I'm not sure we should have test references in header files providing an API! @bulislaw can you comment please ?

@mprse

This comment has been minimized.

Member

mprse commented May 25, 2018

Is this going to need a rebase then once the other PR gets merged ?

Probably yes.

@adbridge adbridge requested a review from bulislaw May 25, 2018

@mprse

This comment has been minimized.

Member

mprse commented May 25, 2018

I have synced this PR with header file from #6708 yesterday.
But the requirements should be defined in #6708 and this PR should contains only test and test header.

I can remove this commit... And we can add them later.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 25, 2018

Awaiting the rebase!

Two things. 1) Generally, tests can come in at any time. It would have probably been better to bring this in with the other PR as a single commit, but that's alright.
2) Talked with @bulislaw about the test headers being in HAL. That's apparently alright.

@cmonr cmonr added risk: A and removed risk: R labels May 25, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 25, 2018

  1. Talked with @bulislaw about the test headers being in HAL. That's apparently alright.

OK not sure I totally agree with this but he is tech lead for Hal so his call.

@mprse mprse force-pushed the mprse:hal_crc_test branch from 0dfbf59 to aceeadb May 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 29, 2018

@mprse As the CRC was integrated, can you review the build failures?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

@mprse Ping

@cmonr cmonr requested a review from jamesbeyond Jun 4, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jun 7, 2018

@mprse Ping

I was out of office and I returned today.
Looks like this test is too big (NUCLEO_F070RB, NUCLEO_F072RB/IAR - out of memory). I'll try to reduce its size or split it into two tests.

mprse added some commits May 7, 2018

HAL CRC test: Fix out of memory issue on NULCLEO_F070RB, NULCLEO_F072…
…RB/IAR

Allocate test case array on stack since memory limits on some boards.
K64F CRC driver: hal_crc_is_supported() - fix condition which validat…
…es CRC width

It looks like the intention was to return false when CRC width is different than: {16, 32} bits.
K64F CRC driver: Fix handling of CRC final XOR value
According to the test results final XOR was incorrectly handled by the CRC driver.
This patch fixes this issue.

@mprse mprse force-pushed the mprse:hal_crc_test branch from aceeadb to a499341 Jun 7, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jun 7, 2018

Provided fix for NUCLEO_F070RB, NUCLEO_F072RB/IAR - out of memory issue.
Also provided some fixes/modifications in NUCLEO and K64F CRC drivers since the tests have detected some issues. @scartmell-arm please review.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jun 7, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 12, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jun 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 12, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

cmonr approved these changes Jun 13, 2018

LGTM

@cmonr cmonr merged commit 0a528ff into ARMmbed:master Jun 13, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 921 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9776 cycles (+201 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment