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

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

Merged
merged 8 commits into from Aug 3, 2018

Conversation

Projects
None yet
8 participants
@mprse
Member

mprse commented Jul 13, 2018

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

@0xc0170 0xc0170 requested a review from c1728p9 Jul 13, 2018

@mprse mprse force-pushed the mprse:ticker_free branch from 89d632d to e562d13 Jul 13, 2018

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Jul 15, 2018

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

This comment has been minimized.

Member

mprse commented Jul 16, 2018

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

This comment has been minimized.

Member

mprse commented Jul 16, 2018

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

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

This comment has been minimized.

@c1728p9

c1728p9 Jul 16, 2018

Contributor

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.

@cmonr cmonr removed the needs: review label Jul 16, 2018

@mprse mprse force-pushed the mprse:ticker_free branch 2 times, most recently from 440fda4 to 4039e31 Jul 20, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jul 20, 2018

Fixed after review and added implementation for CI boards.

@mprse mprse force-pushed the mprse:ticker_free branch from 4039e31 to 2035f77 Jul 20, 2018

@bcostm

LGTM. I left few comments.

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

void lp_ticker_free(void)
{

LptimHandle.Instance = LPTIM1;

This comment has been minimized.

@bcostm

bcostm Jul 20, 2018

Contributor

This line is useless as the LptimHandle is not used.

}

void lp_ticker_free(void)
{

//NVIC_DisableIRQ(RTC_WKUP_IRQn);

This comment has been minimized.

@bcostm

bcostm Jul 20, 2018

Contributor

Remove this line ?

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

void lp_ticker_free(void)
{

LptimHandle.Instance = LPTIM1;
NVIC_DisableIRQ(LPTIM1_IRQn);

This comment has been minimized.

@bcostm

bcostm Jul 20, 2018

Contributor

Why not call the lp_ticker_disable_interrupt() instead ?

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

void us_ticker_free(void)
{

__HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);

This comment has been minimized.

@bcostm

bcostm Jul 20, 2018

Contributor

Why not call the us_ticker_disable_interrupt() instead ?

@mprse mprse force-pushed the mprse:ticker_free branch from 5af3941 to cbd373e Jul 20, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jul 20, 2018

@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

This comment has been minimized.

Contributor

cmonr commented Jul 23, 2018

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

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

@bcostm

bcostm approved these changes Jul 24, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2018

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

@mprse

This comment has been minimized.

Member

mprse commented Jul 24, 2018

@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

This comment has been minimized.

mbed-ci commented Jul 24, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jul 24, 2018

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

This comment has been minimized.

Member

mprse commented Jul 24, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 24, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 31, 2018

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 31, 2018

Wow, quite the apt timing with that last failure.

@bcostm

This comment has been minimized.

Contributor

bcostm commented Aug 1, 2018

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

This comment has been minimized.

Member

mprse commented Aug 1, 2018

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

mprse added some commits Jul 20, 2018

Add implementation of ticker_free() function to CI boards.
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

@mprse mprse force-pushed the mprse:ticker_free branch from 1026d94 to c7c1bb5 Aug 2, 2018

@mprse

This comment has been minimized.

Member

mprse commented Aug 2, 2018

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

This comment has been minimized.

Member

0xc0170 commented Aug 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 2, 2018

tests-mbed_hal-common_tickers: use low level ticker API for time meas…
…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 mprse force-pushed the mprse:ticker_free branch from c7c1bb5 to 2fe6b98 Aug 2, 2018

@mprse

This comment has been minimized.

Member

mprse commented Aug 2, 2018

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

Added fix for boards without lp ticker.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 2, 2018

/morph build

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 2, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

@cmonr cmonr merged commit ae40a09 into ARMmbed:master Aug 3, 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, 703 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9632 cycles (+682 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 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

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

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

Merge pull request ARMmbed#7508 from mprse/ticker_free
Ticker free() - requirements, pseudo code, tests, implementation
@deepikabhavnani

This comment has been minimized.

As per the PR it is ticker_free implementation and we disable interrupts but enable IRQ?
Should it be NVIC_DisableIRQ instead

CC @cmonr @0xc0170

This comment has been minimized.

Member

mprse replied Nov 21, 2018

Nice catch! After init() the ticker interrupt should be disabled, so this needs to be fixed. I'll create a patch.

This comment has been minimized.

Member

mprse replied Nov 21, 2018

Fix can be found here PR #8827.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment