Ticker free() - requirements, pseudo code, tests, implementation #7508

mprse · 2018-07-13T10:26:37Z

Description

This patch fixes ticker ticker_free() issue.

The ticker HAL API has been well defined (defined/undefined behaviour, potential bugs) in the ticker header files the tests which verifies each requirement have been also created. We had all done.
But meanwhile lp/us_ticker_free() functions have been added: #5856

This PR added lp/us_ticker_free() functions to the ticker HAL header files and implementation for only some targets, but did not specify the requirements for these functions (defined/undefined behaviour, potential bugs) as this has been done for other functions.
As a result we have now ready requirements/tests and function in HAL API which is not verified.

@c1728p9 Can you please take a look at the requirements, tests and example implementation?

TODO

Add requirements and pseudo code for HAL ticker_free() function.
Add ticker_free() function to the ticker interface.
Add ticker_free() functional tests.
Add example implementation of ticker_free() for one target for verification (NRF5x family boards).
Add/update ticker_free() implementation for the remaining boards.

Pull request type

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

NirSonnenschein · 2018-07-15T07:01:45Z

build failures in CI:
Error: L6218E: Undefined symbol lp_ticker_free (referred from BUILD/tests/K64F/ARM/hal/mbed_lp_ticker_api.o
please take a look

mprse · 2018-07-16T06:46:59Z

build failures in CI:
Error: L6218E: Undefined symbol lp_ticker_free (referred from BUILD/tests/K64F/ARM/hal/mbed_lp_ticker_api.o
please take a look

Yes, like I mentioned in the description. Implementation of ticker_free needs to be added for some targets, for others needs to be adapted to the requirements. But first we need to agree the requirements.

mprse · 2018-07-16T11:33:37Z

@c1728p9 @0xc0170 I'm not sure if requirements for 'ticker_free() function meets expectation. Can you please take a look.

Should we use ticker_free() for example before going to deep-sleep in order to disable us ticker (like on NRF boards)? In such case the counter value should persist and after wake up us ticker should be re-initialised by means of ticker_init and continue counting.
Or maybe this function shell simply turn of the ticker, ticker interrupts and clock gate.

c1728p9

Looks good aside from the requirement that the ticker stops counting.

c1728p9 · 2018-07-16T15:34:44Z

hal/us_ticker_api.h

@@ -73,6 +73,9 @@ extern "C" {
 * 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
+ * * The function ticker_free stops the ticker from counting and disables the ticker interrupt -


It shouldn't be a requirement that the ticker stops counting, since some low power ticker implementations are backed by an RTC which may keep counting.

mprse · 2018-07-20T07:34:35Z

Fixed after review and added implementation for CI boards.

bcostm

LGTM. I left few comments.

bcostm · 2018-07-20T08:21:20Z

targets/TARGET_STM/lp_ticker.c

@@ -231,7 +231,8 @@ void lp_ticker_clear_interrupt(void)

 void lp_ticker_free(void)
 {
-
+    LptimHandle.Instance = LPTIM1;


This line is useless as the LptimHandle is not used.

bcostm · 2018-07-20T08:21:39Z

targets/TARGET_STM/lp_ticker.c

 }

 void lp_ticker_free(void)
 {
-
+    //NVIC_DisableIRQ(RTC_WKUP_IRQn);


Remove this line ?

bcostm · 2018-07-20T08:24:18Z

targets/TARGET_STM/lp_ticker.c

@@ -231,7 +231,8 @@ void lp_ticker_clear_interrupt(void)

 void lp_ticker_free(void)
 {
-
+    LptimHandle.Instance = LPTIM1;
+    NVIC_DisableIRQ(LPTIM1_IRQn);


Why not call the lp_ticker_disable_interrupt() instead ?

bcostm · 2018-07-20T08:25:36Z

targets/TARGET_STM/us_ticker.c

@@ -259,6 +259,6 @@ void restore_timer_ctx(void)

 void us_ticker_free(void)
 {
-
+    __HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);


Why not call the us_ticker_disable_interrupt() instead ?

mprse · 2018-07-20T10:37:52Z

@bcostm Thanks for the review.
I followed your hints and changed approach a little. So according to the requirements ticker_free() at least has to disable ticker interrupt and would be good to disable also the hardware counter (at least for non RTC based tickers).

cmonr · 2018-07-23T15:23:40Z

@c1728p9 @bcostm Mind taking another look? Looks like y'alls comments have been addressed.

0xc0170 · 2018-07-24T10:03:06Z

/morph build

0xc0170 · 2018-07-24T10:03:36Z

@mprse I believe test will fail as some targets need free implemented

mprse · 2018-07-24T10:23:30Z

@mprse I believe test will fail as some targets need free implemented

Hopefully I manage to add ticker free for all targets. For CI targets ticker free is implemented to be consistent with requirements (I couldn't test all CI boards since I do not have all of them). For other boards I just added empty stub of ticker free function.

mbed-ci · 2018-07-24T10:58:39Z

Build : FAILURE

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

mprse · 2018-07-24T11:16:20Z

./TESTS/mbedmicro-rtos-mbed/systimer/main.cpp
        [DEBUG] Return: 1
        [DEBUG] Output: ./TESTS/mbedmicro-rtos-mbed/systimer/main.cpp:119:1: sorry, unimplemented: non-trivial designated initializers not supported
        [DEBUG] Output:  };
        [DEBUG] Output:  ^

Test for systimer initialises ticker interface with stubs. This needs to be updated and ticker free function must be included.

mprse · 2018-07-24T12:59:03Z

Fixed TESTS-MBEDMICRO-RTOS-MBED-SYSTIMER. Can we try tun morph again?

0xc0170 · 2018-07-24T13:48:56Z

/morph build

mbed-ci · 2018-07-24T15:14:53Z

Build : FAILURE

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

0xc0170 · 2018-07-31T10:47:47Z

Looking at the previous test runs, this is the new issue, and seems not related here (could be an issue on master)

Restarting to confirm

/morph test

bcostm · 2018-07-31T12:44:41Z

What differs this board from the other STM boards connected to CI is that lp ticker is based on LPTIM (not RTC).

Yes you're right. I think also there should be a problem somewhere in the code managing the LPTIM. But no idea at first glance...

I'll try to investigate that, but I do not have this board.

This is the list of the boards having the LPTICKER/LPTIM configuration in targets.json:

DISCO_F413ZH
DISCO_F746NG
DISCO_F769NI
DISCO_L072CZ_LRWAN1
DISCO_L475VG_IOT01A
DISCO_L476VG
DISCO_L496AG
NUCLEO_F410RB
NUCLEO_F413ZH
NUCLEO_F746ZG
NUCLEO_F756ZG
NUCLEO_F767ZI
NUCLEO_L073RZ
NUCLEO_L432KC
NUCLEO_L433RC_P
NUCLEO_L476RG
NUCLEO_L486RG
NUCLEO_L496ZG
NUCLEO_L496ZG_P

Do you have one of them ?

cmonr · 2018-07-31T15:12:30Z

@bcostm From that list, we only have NUCLEO_F746ZG in CI.

mbed-ci · 2018-07-31T15:12:37Z

Test : FAILURE

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

cmonr · 2018-07-31T15:35:52Z

Wow, quite the apt timing with that last failure.

bcostm · 2018-08-01T06:07:11Z

From that list, we only have NUCLEO_F746ZG in CI.

I was just thinking that maybe you have one of the other boards on your desk/office ? I'll try to run this test on other boards.

mprse · 2018-08-01T06:14:49Z

@cmonr Thanks for clarification. I have successfully reproduced the failure on NUCLEO_L476RG and I'm looking into it.

This PR provides implementation of ticker_free() function for the following boards: ARCH_PRO EV_COG_AD3029LZ EV_COG_AD4050LZ K22F K64F K82F KW24D KW41Z LPC546XX NRF51_DK NRF52_DK NUCLEO_F207ZG NUCLEO_F401RE NUCLEO_F429ZI NUCLEO_F746ZG REALTEK_RTL8195AM

…r interface.

… the ticker interface.

mprse · 2018-08-02T10:21:12Z

I have modified STM lp_ticker_free() function for boards with LPTIM. In current version it only disables lp ticker interrupt (does not stop the counter). This is still consistent with the requirements.
Additionally modified ticker speed test case. Execution time of each ticker API function is measured using Timer object. Test replaces original ticker handler for testing purposes, so test should not relay on higher level ticker based features(like Timer) since time tracing is disrupted. Used low level ticker API for time measuring.

@0xc0170 @cmonr Can we try again with morph?

0xc0170 · 2018-08-02T11:27:23Z

/morph build

mbed-ci · 2018-08-02T13:02:50Z

Build : FAILURE

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

…uring. In `ticker speed` test case execution time of ticker API functions is measured using Timer object. Test replaces original ticker handler for testing purposes, so test should not relay on higher level ticker based features(like Timer). Use low level ticker API for time measuring.

mprse · 2018-08-02T13:26:54Z

[Error] main.cpp@443,0: #20: identifier "get_lp_ticker_data" is undefined

Added fix for boards without lp ticker.

0xc0170 · 2018-08-02T13:38:47Z

/morph build

mbed-ci · 2018-08-02T14:47:17Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-08-02T16:08:53Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-08-02T18:33:05Z

Test : SUCCESS

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

Ticker free() - requirements, pseudo code, tests, implementation

0xc0170 requested a review from c1728p9 July 13, 2018 10:51

0xc0170 added the needs: review label Jul 13, 2018

mprse force-pushed the ticker_free branch from 89d632d to e562d13 Compare July 13, 2018 11:19

NirSonnenschein added the needs: work label Jul 15, 2018

c1728p9 reviewed Jul 16, 2018

View reviewed changes

cmonr removed the needs: review label Jul 16, 2018

mprse force-pushed the ticker_free branch 2 times, most recently from 440fda4 to 4039e31 Compare July 20, 2018 07:33

mprse force-pushed the ticker_free branch from 4039e31 to 2035f77 Compare July 20, 2018 07:42

bcostm reviewed Jul 20, 2018

View reviewed changes

mprse force-pushed the ticker_free branch from 5af3941 to cbd373e Compare July 20, 2018 10:35

cmonr added needs: review and removed needs: work labels Jul 23, 2018

c1728p9 approved these changes Jul 23, 2018

View reviewed changes

bcostm approved these changes Jul 24, 2018

View reviewed changes

0xc0170 approved these changes Jul 24, 2018

View reviewed changes

mprse added 3 commits August 2, 2018 09:48

TESTS-MBED_DRIVERS-TIMER: include ticker_free() function in the ticke…

6874fde

…r interface.

TESTS-MBEDMICRO-RTOS-MBED-SYSTIMER: include ticker_free() function in…

6eb66cc

… the ticker interface.

mprse force-pushed the ticker_free branch from 1026d94 to c7c1bb5 Compare August 2, 2018 09:29

mprse force-pushed the ticker_free branch from c7c1bb5 to 2fe6b98 Compare August 2, 2018 13:24

cmonr added needs: CI and removed needs: work labels Aug 2, 2018

cmonr added the release-version: 5.10.0-rc1 label Aug 2, 2018

cmonr added ready for merge and removed needs: CI labels Aug 2, 2018

cmonr merged commit ae40a09 into ARMmbed:master Aug 3, 2018

0xc0170 removed the ready for merge label Aug 3, 2018

aashishc1988 mentioned this pull request Aug 7, 2018

Failing common_ticker tests #7724

Closed

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7508 from mprse/ticker_free

b28f6d7

Ticker free() - requirements, pseudo code, tests, implementation

Ticker free() - requirements, pseudo code, tests, implementation #7508

Ticker free() - requirements, pseudo code, tests, implementation #7508

Uh oh!

Conversation

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

Uh oh!

Description

TODO

Pull request type

Uh oh!

NirSonnenschein commented Jul 15, 2018

Uh oh!

mprse commented Jul 16, 2018

Uh oh!

mprse commented Jul 16, 2018

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse commented Jul 20, 2018

Uh oh!

bcostm left a comment

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 commented Jul 20, 2018

Uh oh!

cmonr commented Jul 23, 2018

Uh oh!

0xc0170 commented Jul 24, 2018

Uh oh!

0xc0170 commented Jul 24, 2018

Uh oh!

mprse commented Jul 24, 2018

Uh oh!

mbed-ci commented Jul 24, 2018

Uh oh!

mprse commented Jul 24, 2018

Uh oh!

mprse commented Jul 24, 2018

Uh oh!

0xc0170 commented Jul 24, 2018

Uh oh!

mbed-ci commented Jul 24, 2018

Uh oh!

0xc0170 commented Jul 31, 2018

Uh oh!

bcostm commented Jul 31, 2018

Uh oh!

cmonr commented Jul 31, 2018

Uh oh!

mbed-ci commented Jul 31, 2018

Uh oh!

cmonr commented Jul 31, 2018

Uh oh!

bcostm commented Aug 1, 2018

Uh oh!

mprse commented Aug 1, 2018

Uh oh!

mprse commented Aug 2, 2018

Uh oh!

0xc0170 commented Aug 2, 2018

Uh oh!

mbed-ci commented Aug 2, 2018

Uh oh!

mprse commented Aug 2, 2018

Uh oh!

0xc0170 commented Aug 2, 2018

Uh oh!

mbed-ci commented Aug 2, 2018

Triggering tests

mprse commented Jul 13, 2018 •

edited

Loading