Ticker specification (branch feature-hal-spec-ticker) #5164

c1728p9 · 2017-09-21T15:58:44Z

Add the Ticker HAL API specification along with the prototypes for tests which verify it.

c1728p9 · 2017-09-21T16:00:26Z

This depends on #5154 for doxygen and #5028 for ticker API changes.

0xc0170 · 2017-09-22T10:38:09Z

@pan- @bulislaw

pan- · 2017-09-25T16:26:17Z

Diff with #5154 can be found here.

pan- · 2017-09-26T08:19:54Z

hal/lp_ticker_api.h

@@ -49,37 +49,126 @@ void lp_ticker_irq_handler(void);

 /** Initialize the low power ticker
 *
+ * Pseudo Code:


Could you detail what initialization mean ?

Does it start the ticker ?

Does it enable the interrupt ?

What should be the behavior if the ticker was already initialized ?

There is more details in the us_ticker_init documentation.

pan- · 2017-09-26T08:20:09Z

hal/lp_ticker_api.h

 */
 void lp_ticker_init(void);

 /** Read the current counter
 *
- * @return The current timer's counter value in microseconds
+ * @return The current timer's counter value in ticks
+ *


It might be interesting to give sample code for different kind of tickers.

What other use cases did you have in mind?

pan- · 2017-09-26T08:21:44Z

hal/lp_ticker_api.h

 */
 void lp_ticker_init(void);

 /** Read the current counter
 *
- * @return The current timer's counter value in microseconds
+ * @return The current timer's counter value in ticks


Would it be possible to specify what a tick is and the relationship to the value return and the ticker_info_t returned by lp_ticker_get_info.
Could you also detail the expected behavior if this function is called while lp_ticker_init hasn't been called.

Added information on how ticker_read relates to ticker_get_info. In the Undefined behavior section I have that calling any function other ticker function before ticker_init is undefined behavior. If you want I could add that info to each of the functions.

pan- · 2017-09-26T08:24:32Z

hal/lp_ticker_api.h

 */
 uint32_t lp_ticker_read(void);

 /** Set interrupt for specified timestamp
 *
- * @param timestamp The time in microseconds to be set
+ * @param timestamp The time in ticks to be set


Could you detail the behavior if the parameter is out of range (might be unspecified....) ?

pan- · 2017-09-26T08:25:27Z

hal/lp_ticker_api.h

+ * @note no special handling needs to be done for times in the past
+ * as the common timer code will detect this and call
+ * ::us_ticker_fire_interrupt if this is the case
+ *


What if lp_ticker_init hasn't been called ?

I think it's fair to assume (we should document it) that it's undefined behavior?

pan- · 2017-09-26T08:33:09Z

hal/test-headers/lp_ticker_api_tests.h

+ *     mbed test -t <toolchain> -m <target> -n tests-mbed_hal-us_lp_ticker*,tests-mbed_hal-lp_ticker*
+ *
+ * # Defined behavior
+ * * Has a reported frequency between 8KHz and 64KHz - verified by lp_ticker_info_test


That may be interesting to have these information in lp_ticker_api.h

Agreed, tests headers mustn't introduce any new implementation information or requirements. It has to be all encapsulated in HAL API or PG.

pan- · 2017-09-26T08:37:56Z

hal/test-headers/us_ticker_api_tests.h

+ *
+ *     mbed test -t <toolchain> -m <target> -n tests-mbed_hal-us_lp_ticker*,tests-mbed_hal-us_ticker*
+ *
+ * # Defined behavior


Would it be possible to have these informations in us_ticker_api.h ?

@pan- Yeah, I think that would be good. I have already made this change to the RTC specification in #5008. If you get a chance, take a look and let me know what you think.

Also, the bulk of the defined behavior is located in ticker_api_tests.h at the moment. I'm thinking I might move this into us_ticker_api_tests.h and get rid of the additional test header files. Then the lp ticker could reference the us ticker for the common behavior. What do you think of this?

mprse · 2017-09-26T12:42:51Z

hal/test-headers/ticker_api_tests.h

+ * * The function ticker_init is safe to call repeatedly - Verified by test ::ticker_init_test
+ * * The function ticker_init resets the internal count 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 only option to test this is to wait until ticker rolls over. There is not function to modify ticker count. When ticker is 32 bit long, then this scenario will execute over 70 minutes. Is this intended?

I would skip this tests for devices which take more than a certain amount of time to overflow (~30 seconds or so?). This would allow 16 and 24 bit microsecond tickers and 16 bit lp tickers to be tested. It might be good to have the max test time as a define, so if need be the full 70 minute rollover could be tested locally (but not in CI) if need be.

Thank you. This is a good solution, but still on some boards it might be hard to catch rollover moment to prove that after rollover ticker count is equal to 0.

I agree, this isn't possible to do for some timers. For the low power ticker it should be straightforward to test this since you can read each tick. For the microsecond ticker this could be optional similar to the 32 bit long overflow scenario.

mprse · 2017-09-26T13:27:19Z

hal/us_ticker_api.h

+ *     NVIC_SetVector(TIMER_IRQn, (uint32_t)us_ticker_irq_handler);
+ *     NVIC_EnableIRQ(TIMER_IRQn);
+ * }
+ * @endcode
 */
 void us_ticker_init(void);

 /** Read the current counter


Does us_ticker_read() function returns ticker ticks or microseconds? It looks like that in previous implementation it was assumed that ticker clock speed is 1 MHz, so 1 tick is equal to 1 us, but the concept has changed, and clock speed might be different now. This means that description of the return value should not mention about microseconds, but ticker ticks count instead. Also function name is misleading now.

Its returns ticks. I'll fix the doxygen to reflect this.

I agree, the name is misleading. Only potential solution to this is to rename the microsecond ticker (us_ticker_) to high frequency ticker (hf_ticker_). Another possible solution is to require the us ticker has a fixed 1MHz frequency.

It looks like the better solution is to rename the function taking into account new features provided in this PR.

mprse · 2017-09-26T13:39:07Z

hal/test-headers/ticker_api_tests.h

+ * * The function ticker_init resets the internal count 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


We could test this by counting ticker ticks in the specified time period and calculating clock frequency, but there is a problem with time pattern. We can not use wais_us() since it also uses ticker us_ticker_read() and additionally it does not take into account ticker frequency (it assumes that ticker clock speed is 1MHz => 1 tick == 1 us). We might use RTC and count seconds, but RTC accuracy is poor. What do you suggest?

It should probably be compared against the clock of the PC running the test. This is already done with some timing tests such as mbed-os\TESTS\mbed_drivers\ticker\main.cpp.

Thanks for the answers. Still it would be also good to provide fix for wait_us(). wait_us() should use ticker_read_us(const ticker_data_t *const) instead of us_ticker_read/hf_ticker_read. I can provide this fix with the tests and update of ticker IRQ handler.

mprse · 2017-09-27T09:11:59Z

hal/test-headers/ticker_api_tests.h

+ * * 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


It looks like this may be hard to prove. I understand that in current implementation us_ticker_read() returns us. This is to be changed by the vendors and in the new version they will define ticker clock frequency, ticker will be incremented always by 1 (e.g. currently on NRF51_DK we have 32KHz ticker clock and ticker value is incremented by 31) and us_ticker_read() will return pure ticker value. So in the new version on our NRF51_DK ticker will be incremented by one every 31 us. On this board read operation takes about 15 us, so we might be able to prove that ticker is incremented by one. But what with boards where the relation between CPU clock freq and ticker clock frequency is such that read operation is longer than delays between ticker increments. Currently on NUCLEO_F070RB for example ticker is incremented every 1us and read operation takes over one us.

It may not be possible to comprehensively test this for the high speed tickers. The goal of this requirement is to get vendors to use the timer directly, rather than scaling the timer internally, as was done with the low power timer on most boards.

Worst case you could test this up to a point based on the required execution time of ticker_read. This may mean the execution time requirement on ticker_read be decreased from 20us to 10 or 15us to ensure ticks of a 32KHz clock can be read.

One possible way you could measure this indirectly is with the elapsed time of a delay loop. Assuming the loop executes in a consistent time then the the elapsed time should be accurate +-1us if there is no scaling involved. This could be run across a range of delay cycles to give more confidence that no scaling is done. This approach may turn out to be too complicated though. What do you think @mprse?

uint32_t test_read_with_delay(uint32_t cycles) { enter_critical(); volatile uint32_t count = 1000; uint32_t start = us_ticker_read(); while (count--); uint32_t stop = us_ticker_read(); exit_critical(); return stop - start; }

Lets assume that ticker is incremented every 1 us by 1 and read operation takes:

less than 1 us - in this case we can easily prove that ticker is incremented by 1 (not scaled).

more than 1 us, lets assume 10 us. Now when we would perform reads in sequence, then the difference between read tick counts should be 10 +/- 1 ticks. Now if we use provided while loop with cycles set to 10 and measure N ticks, then for 100 cycles we should have 10N +/-1 tick, for 1000 cycles 100N +/-1 tick.
In case when ticker would be increment by 5 ticks (scaled), then in the same scenario we would get 10 cycles : N ticks, 100 cycles : 10N +/-5, 1000 cycles : 100N +/-5. We might now detect that the counted value is not in rage +/-1 tick, but still we might not detect that ticker is incremented by 5 if in all cases we read exact values (10N, 100N).

On the other hand I think that frequency test using PC might be sufficient for this case. If we measure using PC number of ticker ticks during one second and validate this against ticker clock frequency then we prove that ticker is not scaled. E.g. Currently on NRF51_DK we have 32KHz ticker clock and ticker is incremented by 31 (scaled). So counted number of ticks will be about 1000000 and we expect about 32 000.

I don't understand how measuring using a PC would help detect single ticks. If a 32KHz clock is scaled up internally to 1MHz then it will appear that the frequency is correct - over the course of 1s the tick value reported will be 1000000 +- 31.

As for point 2 mentioned above, I agree. With a fixed test delays a scaled frequency may not be detected. If you sweep the delay (delay = 100 +1 * sweep_cycle), then you should be able to detect any frequency scaling, as long as one cycle of the loop takes less than 1 tick.

mprse · 2017-09-27T11:13:51Z

hal/test-headers/ticker_api_tests.h

+ * * 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


Do you think that this should be also tested on PC? I'm not sure if there is a sense to use PC to count 20us, on the other hand I can not use ticker_read function to check how long execution of ticker_read function takes. Maybe we could call tested function for example 1000 times and then it should not take longer than 20 ms.

You probably don't need to test this against the PCs time. If you have a timer you know is accurate from a previous test then you should be able to use that to test this. You'll probably need to call each function a large number of times to get an accurate reading in addition to this.

mprse · 2017-09-27T12:03:30Z

hal/test-headers/ticker_api_tests.h

+/** Test that ticker_init can be called multiple times
+ *
+ */
+void ticker_init_test(void);


This header file provides test functions common to low power ticker and high freq ticker.
This means that for example ticker_init_test() shall test us_ticker_init() and
lp_ticker_init(). I can create in test structure with interfaces for low power ticker and high freq ticker and call tested function twice - once for each interface.
I'm wondering if testing lp/hf tickers together is a good solution since low power ticker is not available on some platforms. Maybe it would be better to provide separate test functions for lp/hf e.g. us_ticker_init_test and lp_ticker_init_test in the separate cpp files?

You should be able to have a separate setup step for each test which sets the timer to test. An example of this can be seen here:

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

Lines 88 to 97 in ddb6b66

Case cases[] = {

Case("us ticker HAL - Init", us_ticker_setup, test_ticker_init),

Case("us ticker HAL - Read", us_ticker_setup, test_ticker_read),

Case("us ticker HAL - Read in the loop", us_ticker_setup, test_ticker_read_loop),

#if DEVICE_LOWPOWERTIMER

Case("lp ticker HAL - Init", lp_ticker_setup, test_ticker_init),

Case("lp ticker HAL - Read", lp_ticker_setup, test_ticker_read),

Case("lp ticker HAL - Read in the loop", lp_ticker_setup, test_ticker_read_loop),

#endif

};

bulislaw · 2017-09-29T14:59:41Z

hal/mbed_ticker_api.c

-        elapsed_us += 1;
-        queue->tick_remainder -= queue->frequency;
+    uint64_t elapsed_us;
+    if (1000000 == queue->frequency) {


Does it actually change much? Or do we just make to code less readable? Maybe at least we could move that to separate utility functions.

This optimization is extremely important. Without this optimization some boards, such as the nrf51 are too slow to pass some of the timing tests.

bulislaw · 2017-09-29T15:02:39Z

hal/test-headers/lp_ticker_api_tests.h

+ *     mbed test -t <toolchain> -m <target> -n tests-mbed_hal-us_lp_ticker*,tests-mbed_hal-lp_ticker*
+ *
+ * # Defined behavior
+ * * Has a reported frequency between 8KHz and 64KHz - verified by lp_ticker_info_test


Agreed, tests headers mustn't introduce any new implementation information or requirements. It has to be all encapsulated in HAL API or PG.

bulislaw · 2017-09-29T15:05:18Z

hal/test-headers/ticker_api_tests.h

+ *
+ *     mbed test -t <toolchain> -m <target> -n tests-mbed_hal-us_lp_ticker*
+ *
+ * # Defined behavior


That should be documented in HAL API header or PG.

bulislaw · 2017-09-29T16:08:18Z

hal/lp_ticker_api.h

+ * @note no special handling needs to be done for times in the past
+ * as the common timer code will detect this and call
+ * ::us_ticker_fire_interrupt if this is the case
+ *


I think it's fair to assume (we should document it) that it's undefined behavior?

bulislaw · 2017-10-02T15:30:51Z

Are we ready to merge this PR to a branch?

mprse · 2017-10-04T08:10:59Z

hal/test-headers/ticker_api_tests.h

+ * * 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.


@c1728p9 Here we need to test two cases:

an interrupt is not triggered when ticker_set_interrupt is called with a time from the past.

an interrupt is triggered in the right time.

We have noticed that on some platforms ticker interrupt is periodic. For example on K64F. Is this a problem?
Maybe we need additional case to verify that interrupt is not periodic?

We have noticed that on some platforms ticker interrupt is periodic. For example on K64F. Is this a problem?

Periodic as ? set once and it reoccurs? or ?

Yes, it reoccurs. If we call:
intf->set_interrupt(intf->read() + TICKER_INT_VAL);

then IRQ handler will be executed more than once, in the following moments:
read tick count+ TICKER_INT_VAL
read tick count + 2TICKER_INT_VAL
read tick count + 3TICKER_INT_VAL
etc.

That should not happen, pls report it

Issue has been reported:
#5279

theotherjimmy · 2017-10-10T22:49:41Z

@AnotherButler You should probably be watching the feature-hal-spec-* branches. They will probably be adding more Doxygen. This PR affects one of them.

mbed-ci · 2017-10-10T23:51:11Z

Build : SUCCESS

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

Triggering tests

/test mbed-os

mbed-ci · 2017-10-11T05:19:59Z

Test : FAILURE

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

AnotherButler · 2017-10-11T18:14:55Z

@theotherjimmy Thanks. I've made a note of that and talked to @c1728p9 about docs. This should not affect this PR being merged.

mbed-ci · 2017-10-12T09:22:29Z

Test : FAILURE

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

0xc0170 · 2017-10-19T09:29:27Z

Dependencies were merged, should be ready now?

Please rebase, and we should retrigger the CI

c1728p9 · 2017-10-19T22:09:26Z

/morph build

bulislaw · 2017-10-20T16:02:05Z

/morph build

mbed-ci · 2017-10-20T16:11:27Z

Build : FAILURE

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

c1728p9 · 2017-10-23T15:32:15Z

The new test headers were getting picked up as tests. I moved these tests up one level so they should no longer be picked up as tests. When test cases are added these files should be moved into the appropriate test folder.

Add documentation and test header for the HAL Ticker API.

c1728p9 · 2017-10-23T15:48:55Z

/morph build

mbed-ci · 2017-10-23T16:11:10Z

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-10-23T19:02:57Z

Test : SUCCESS

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

adbridge · 2017-10-24T15:10:51Z

Are all the reviewers happy with this now ?

bulislaw · 2017-10-25T14:36:53Z

Can we merge it ASAP?

0xc0170 added needs: preceding PR needs: review labels Sep 22, 2017

pan- reviewed Sep 26, 2017

View reviewed changes

mprse reviewed Sep 26, 2017

View reviewed changes

mprse reviewed Sep 27, 2017

View reviewed changes

bulislaw reviewed Sep 29, 2017

View reviewed changes

mprse mentioned this pull request Oct 2, 2017

Add tests for ticker HAL API. #5233

Merged

mprse reviewed Oct 4, 2017

View reviewed changes

c1728p9 force-pushed the ticker_specification branch from c1b38c7 to 70f5969 Compare October 19, 2017 21:53

c1728p9 removed the needs: preceding PR label Oct 19, 2017

c1728p9 force-pushed the ticker_specification branch from 70f5969 to 7dc409b Compare October 19, 2017 22:08

c1728p9 force-pushed the ticker_specification branch from 7dc409b to d38ba87 Compare October 23, 2017 15:30

Add documentation for the HAL Ticker API

8ac4a38

Add documentation and test header for the HAL Ticker API.

c1728p9 force-pushed the ticker_specification branch from d38ba87 to 8ac4a38 Compare October 23, 2017 15:48

0xc0170 added needs: CI and removed needs: review labels Oct 23, 2017

bulislaw approved these changes Oct 24, 2017

View reviewed changes

adbridge added ready for merge and removed needs: CI labels Oct 26, 2017

adbridge merged commit 3c0fb93 into ARMmbed:feature-hal-spec-ticker Oct 26, 2017

adbridge removed the ready for merge label Oct 26, 2017

c1728p9 mentioned this pull request Oct 30, 2017

RTC specification with test headers (branch feature-hal-spec-rtc) #5226

Merged

	Case cases[] = {
	Case("us ticker HAL - Init", us_ticker_setup, test_ticker_init),
	Case("us ticker HAL - Read", us_ticker_setup, test_ticker_read),
	Case("us ticker HAL - Read in the loop", us_ticker_setup, test_ticker_read_loop),
	#if DEVICE_LOWPOWERTIMER
	Case("lp ticker HAL - Init", lp_ticker_setup, test_ticker_init),
	Case("lp ticker HAL - Read", lp_ticker_setup, test_ticker_read),
	Case("lp ticker HAL - Read in the loop", lp_ticker_setup, test_ticker_read_loop),
	#endif
	};

Ticker specification (branch feature-hal-spec-ticker) #5164

Ticker specification (branch feature-hal-spec-ticker) #5164

Uh oh!

Conversation

c1728p9 commented Sep 21, 2017

Uh oh!

c1728p9 commented Sep 21, 2017

Uh oh!

0xc0170 commented Sep 22, 2017

Uh oh!

pan- commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse Sep 27, 2017 •

edited

Loading

c1728p9 Sep 27, 2017 •

edited

Loading

mprse Oct 4, 2017 •

edited

Loading