TESTS: common_tickers handle wrap-around case #6462

LMESTM · 2018-03-26T19:57:39Z

Description

it seems that count_ticks function does not consider the case of a
16 its counter wrap-around - proposal added here.

Pull request type

[x ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

Add documentation and test header for the HAL Ticker API.

…header files. Add one potential bug to the test description. Fix test function names to be consistant with desctiption and test header. Fix typos.

…pecific drivers. Since target specific ticker drivers are not ready also features which uses ticker in upper layers may not work correctly and tests for these features. We need to disable also failing higher level ticker related tests (by adding check if DEVICE_USTICKER symbol is available and raise #error [NOT_SUPPORTED] if not).

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)

The intention was to use ticker_overflow_delta equal to 0 for low power ticker tests and ticker_overflow_delta equal to 50 for high frequency ticker tests. Current implementation is invalid since for devices which provide LOW_POWER_TIMER feature delta is equal to 0 and for devices without this feature delta is equal 50.

- provide ticker configuration: 1MHz/32 bit counter. - us_ticker_init() routine disables interrupts. - adapt comments.

Low power ticker time counter is created based on RTC which is driven by 32KHz clock. Additional low power timer is used to generate interrupts. We need to adapt driver to operate on ticks instead of us. Perform the following updates: - provide lp ticker configuration: 32KHz/32 bit counter. - lp_ticker_init() routine disables lp ticker interrupts . - adapt the driver functions to operate on ticks instead us. - adapt comments. - add us_ticker_free() routine.

Since according to the ticker requirements min acceptable counter size is 12 bits (low power timer) for which max count is 4095, then all test cases must be executed in this time window. HAL ticker layer handles overflow and it is not handled in the target ticker drivers.

On some platforms (e.g. K64F) different counters are used for time measurement and interrupt generation. Because of that we should relax interrupt test case and give additional time before checking if interrupt handler has been executed.

…lts on all compilers count_ticks() function counts ticker ticks elapsed during execution of N cycles of empty while loop. In current version N value (cycles) is given as volatile paramater in order to disable possible compiler optimalisation. There was a problem with measured time on different compilers and additionally results on ARM compiler were unexpected (the difference beetween measured elapsed ticks for the same number of cycles was to large). This might be caused by the memory access in order to store updated variable in memory. To fix this issue given numer of cycles has been stored into register and register is decremented (no memory access). With this fix count_ticks(NUM_OF_CYCLES, 1) call returns 2500 +/-1 for us ticker ticks using each compiler (K64F).

…pecific drivers. Since target specific ticker drivers are not ready also features which uses ticker in upper layers may not work correctly and tests for these features. We need to disable also failing higher level ticker related tests (by adding check if DEVICE_USTICKER symbol is available and raise #error [NOT_SUPPORTED] if not). Note: tests-mbed_drivers-rtc is new and uses us ticker to perform a delay.

Test that lp ticker does not glitch backwards due to an incorrectly implemented ripple counter driver.

That's to match DEVICE_USTICKER.

…ct device_has

The current implementation of tickless requires LPTICKER to be present.

it seems that count_ticks function does not consider the case of a 16 its counter wrap-around - proposal added here.

cmonr · 2018-03-27T04:40:31Z

@LMESTM Interesting that the counter is only 16 bits.

Since this is a test, why not simplify things even further and instead increase the size of the counter type?

cmonr · 2018-03-27T04:48:55Z

Ah, never mind. Just went through your other PR, and now it makes sense why this was set to 16 bits.

Something that bothers me. There's now an overflow check, but what happens if the counter overflows twice? Would this change catch multiple overflows?

You might consider catching overflows inside of the while loop.

cmonr · 2018-03-27T04:51:08Z

I'm also curious to hear about how you tested the overflow edge case.

LMESTM · 2018-03-27T07:14:02Z

I'm also curious to hear about how you tested the overflow edge case.

I just tested on L073RZ target which happens to be a relatively slow CPU, with 16bits counter.
Error message below was somehow leading to overflow. I'm fine with any other fix for the test.

[1522085572.44][CONN][RXD] >>> Running case #8: 'Microsecond ticker increment test'...
[1522085572.50][CONN][INF] found KV pair in stream: {{__testcase_start;Microsecond ticker increment test}}, queued...
[1522085572.59][CONN][RXD] :314::FAIL: Values Not Within Delta 1 Expected 4294911135 Was 9375
[1522085572.64][CONN][INF] found KV pair in stream: {{__testcase_finish;Microsecond ticker increment test;0;1}}, queued...
[1522085572.75][CONN][RXD] >>> 'Microsecond ticker increment test': 0 passed, 1 failed with reason 'Assertion Failed'

theotherjimmy · 2018-03-29T15:35:03Z

It looks like you have conflicts with the base branch. This may require a rebase. Also, it looks like this PR includes many changes that @LMESTM did not make. These two observations are probably related.

LMESTM · 2018-03-29T15:45:50Z

@cmonr this was a quick fix made during workshop to unblock situation. I'm fin if you fix it another way or use the same kind of protect overflow that have been proposed in other PRs.

0xc0170 · 2018-04-03T14:36:40Z

@LMESTM This should fix the problem #6492 and can this be closed or still relevant?

LMESTM · 2018-04-03T14:43:11Z

@0xc0170 - do you refer to another PR that would fix this issue ? Is there a missing link. I am not sure if #6492 covers the common_tickers tests, does it ?
But I agree that overflow_protect() mechanism could be used as in #6492. I'd be fine to close this PR and let @mprse do a better fix than b2aa2dd

LMESTM · 2018-04-03T14:44:52Z

I'm saying this because previous comment from @cmonr makes definitely sense

Something that bothers me. There's now an overflow check, but what happens if the counter overflows twice? Would this change catch multiple overflows?

mprse · 2018-04-04T06:48:31Z

@LMESTM Hi! I'm sure that count_ticks() function is protected against overflow. It does not use standard overflow_protect() mechanism since we can not predict how many times count_ticks() will be executed during ticker_increment_test.

I checked on slow board like 16 MHz NRF51_DK, that count_ticks(NUM_OF_CYCLES, 1) takes:
~25 000 us ticker ticks (16 bits / 1 MHz counter)
~820 lp ticker ticks (24 bits / 32 768 Hz counter)
so this should be successfully handled by the regular overflow check which is implemented as follows and already on ticker feature branch:

mbed-os/TESTS/mbed_hal/common_tickers/main.cpp

Lines 69 to 76 in 3821b54

    
           const uint32_t stop = intf->read(); 
        
           core_util_critical_section_exit(); 
        
           /* Handle overflow - overflow protection may not work in this case. */ 
        
           uint32_t diff = (start <= stop) ? (stop - start) : (uint32_t)(max_count - start + stop + 1); 
        
           return (diff);

Please update your local branch.

0xc0170 · 2018-04-09T15:40:41Z

@LMESTM Does the latest comment from @mprse answer your question about overflows? Shall this be closed or rework needed?

0xc0170 · 2018-04-11T12:41:03Z

@LMESTM I am closing this due inactivity. Please reopen if this is still needed (with an update - needs rebase)

c1728p9 and others added 28 commits March 24, 2018 18:27

Add documentation for the HAL Ticker API

c7faf4a

Add documentation and test header for the HAL Ticker API.

Add tests for ticker HAL API.

f239640

Add minor modifications of lp/us tickers tests desctiption in ticker …

4aae6b2

…header files. Add one potential bug to the test description. Fix test function names to be consistant with desctiption and test header. Fix typos.

Disable boards which does not fulfil new ticker standards.

7fb2ef3

Add DEVICE_RTC symbol check in RTC source files for NCS36510 target.

4e9b6b7

Modify ticker_init_test() to be consistent with the new requirement.

73f90fe

Add free function to lp/us ticker HAL API

ab56365

Add lp/us ticker free() function implementation for NRF5

5412b6c

Enable ticker support for K64F board.

3ca158d

Adapt K64F us ticker driver to the new standards.

9c84aec

- provide ticker configuration: 1MHz/32 bit counter. - us_ticker_init() routine disables interrupts. - adapt comments.

lp_ticker test - provide lp ticker glitch test case

4c8f182

Test that lp ticker does not glitch backwards due to an incorrectly implemented ripple counter driver.

Rename DEVICE_LOWPOWERTIMER to DEVICE_LPTICKER

f7032da

That's to match DEVICE_USTICKER.

Disable Maxim boards as LP_TICKER is used in RTC and they don't respe…

cc8c808

…ct device_has

Make sure LPTICKER symbols are not used for builds without it for Nordic

2480dc3

Don't use tickless if LPTICKER is not present

c0e1057

The current implementation of tickless requires LPTICKER to be present.

Disable RF52_DK as they require LPTICKER for flash and trng

9753cbe

Disable platfroms that fail without LPTICKER

c315737

Remove nrf51_dk from supported platform

f4d1ed8

Rename lp_us tests to common and hf to microsecond

458e726

TESTS: common_tickers handle wrap-around case

b2aa2dd

it seems that count_ticks function does not consider the case of a 16 its counter wrap-around - proposal added here.

LMESTM mentioned this pull request Mar 26, 2018

Q12018 stm32 ustickers #6463

Merged

0xc0170 requested review from c1728p9 and bulislaw March 27, 2018 07:27

bulislaw force-pushed the feature-hal-spec-ticker branch from afd66ab to 6daa7fc Compare March 27, 2018 14:06

cmonr added the needs: review label Mar 28, 2018

0xc0170 added needs: work and removed needs: review labels Apr 9, 2018

0xc0170 closed this Apr 11, 2018

0xc0170 removed the needs: work label Apr 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TESTS: common_tickers handle wrap-around case #6462

TESTS: common_tickers handle wrap-around case #6462

Uh oh!

LMESTM commented Mar 26, 2018

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

LMESTM commented Mar 27, 2018

Uh oh!

theotherjimmy commented Mar 29, 2018

Uh oh!

LMESTM commented Mar 29, 2018

Uh oh!

0xc0170 commented Apr 3, 2018

Uh oh!

LMESTM commented Apr 3, 2018

Uh oh!

LMESTM commented Apr 3, 2018

Uh oh!

mprse commented Apr 4, 2018

Uh oh!

0xc0170 commented Apr 9, 2018

Uh oh!

0xc0170 commented Apr 11, 2018

Uh oh!

Uh oh!

TESTS: common_tickers handle wrap-around case #6462

TESTS: common_tickers handle wrap-around case #6462

Uh oh!

Conversation

LMESTM commented Mar 26, 2018

Description

Pull request type

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

LMESTM commented Mar 27, 2018

Uh oh!

theotherjimmy commented Mar 29, 2018

Uh oh!

LMESTM commented Mar 29, 2018

Uh oh!

0xc0170 commented Apr 3, 2018

Uh oh!

LMESTM commented Apr 3, 2018

Uh oh!

LMESTM commented Apr 3, 2018

Uh oh!

mprse commented Apr 4, 2018

Uh oh!

0xc0170 commented Apr 9, 2018

Uh oh!

0xc0170 commented Apr 11, 2018

Uh oh!

Uh oh!