Add HAL CRC test and header file #6831

mprse · 2018-05-07T13:50:51Z

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 · 2018-05-24T15:27:06Z

@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?

ghost · 2018-05-25T12:07:51Z

@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 · 2018-05-25T12:24:52Z

@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.

mbed-os/hal/us_ticker_api.h

Lines 58 to 90 in 9fdb5c2

    
           /** 
        
            * \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.

ghost · 2018-05-25T13:04:20Z

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

adbridge · 2018-05-25T13:05:36Z

@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 · 2018-05-25T13:07:04Z

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

Probably yes.

mprse · 2018-05-25T13:10:23Z

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 · 2018-05-25T15:44:33Z

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.

adbridge · 2018-05-25T15:49:01Z

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.

cmonr · 2018-05-25T17:54:33Z

Taking this outside of 5.9. Build failures indicate IAR linker issues with the new test.

Tests can come in during patch releases.

0xc0170 · 2018-05-29T08:32:55Z

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

cmonr · 2018-06-04T16:12:29Z

@mprse Ping

mprse · 2018-06-07T06:58:24Z

@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.

…RB/IAR Allocate test case array on stack since memory limits on some boards.

…es CRC width It looks like the intention was to return false when CRC width is different than: {16, 32} bits.

According to the test results final XOR was incorrectly handled by the CRC driver. This patch fixes this issue.

mprse · 2018-06-07T13:28:31Z

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.

cmonr · 2018-06-12T02:49:15Z

/morph build

mbed-ci · 2018-06-12T04:58:35Z

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 · 2018-06-12T06:45:47Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-06-13T00:57:50Z

Test : SUCCESS

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

cmonr

LGTM

cmonr requested a review from a user May 7, 2018 23:00

cmonr added needs: work needs: review and removed needs: work labels May 7, 2018

ghost mentioned this pull request May 9, 2018

Add Hardware CRC HAL API specification #6708

Merged

ghost previously approved these changes May 14, 2018

View reviewed changes

bulislaw added the release-version: 5.9.0-rc1 label May 15, 2018

0xc0170 added the needs: preceding PR label May 15, 2018

adbridge added needs: CI and removed needs: review labels May 21, 2018

mprse dismissed ghost ’s stale review via 0dfbf59 May 24, 2018 12:39

mprse force-pushed the hal_crc_test branch from bd19ecb to 0dfbf59 Compare May 24, 2018 12:39

adbridge added needs: work risk: R and removed needs: CI labels May 25, 2018

adbridge requested a review from bulislaw May 25, 2018 13:07

cmonr removed needs: work needs: preceding PR labels May 25, 2018

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

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

ghost mentioned this pull request May 30, 2018

Add porting guide for Hardware CRC HAL module ARMmbed/mbed-os-5-docs#505

Merged

cmonr requested a review from jamesbeyond June 4, 2018 21:13

mprse added 7 commits June 7, 2018 10:17

Add HAL CRC test and test header file.

1eed0b9

Add testing requirements to HAL CRC header file.

307021e

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

511f370

…RB/IAR Allocate test case array on stack since memory limits on some boards.

STM CRC driver: hal_crc_compute_partial() - validate input paramaters.

68631fc

K64F CRC driver: hal_crc_is_supported() - fix condition which validat…

da557a5

…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

ee6a07e

According to the test results final XOR was incorrectly handled by the CRC driver. This patch fixes this issue.

K64F CRC driver: Fix coding style

a499341

mprse force-pushed the hal_crc_test branch from aceeadb to a499341 Compare June 7, 2018 13:25

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

ghost approved these changes Jun 11, 2018

View reviewed changes

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

cmonr approved these changes Jun 13, 2018

View reviewed changes

cmonr added ready for merge release-version: 5.9.1 and removed needs: CI labels Jun 13, 2018

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

NirSonnenschein removed the ready for merge label Jun 13, 2018

Add HAL CRC test and header file #6831

Add HAL CRC test and header file #6831

Uh oh!

Conversation

mprse commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Related PRs

Pull request type

Uh oh!

cmonr commented May 24, 2018

Uh oh!

ghost commented May 25, 2018

Uh oh!

mprse commented May 25, 2018

Uh oh!

ghost commented May 25, 2018

Uh oh!

adbridge commented May 25, 2018

Uh oh!

mprse commented May 25, 2018

Uh oh!

mprse commented May 25, 2018

Uh oh!

cmonr commented May 25, 2018

Uh oh!

adbridge commented May 25, 2018 • edited by cmonr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented May 25, 2018

Uh oh!

0xc0170 commented May 29, 2018

Uh oh!

cmonr commented Jun 4, 2018

Uh oh!

mprse commented Jun 7, 2018

Uh oh!

mprse commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Jun 12, 2018

Uh oh!

mbed-ci commented Jun 12, 2018

Build : SUCCESS

Triggering tests

Uh oh!

mbed-ci commented Jun 12, 2018

Exporter Build : SUCCESS

Uh oh!

mbed-ci commented Jun 13, 2018

Test : SUCCESS

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mprse commented May 7, 2018 •

edited

Loading

adbridge commented May 25, 2018 •

edited by cmonr

Loading

mprse commented Jun 7, 2018 •

edited

Loading