Add LPTICKER tests #8129

c1728p9 · 2018-09-14T02:21:57Z

Description

When both tickless and LPTICKER_DELAY_TICKS are enabled some ST devices randomly get stuck sleeping forever. This is because the wake up time passed to the rtc is ignored if the previous match is about to occur. This causes the device to get stuck in sleep.

~~This patch~~ #8242 prevents matches from getting dropped by the rtc by deactivating the rtc wake up timer before setting a new value.

This patch also makes a related fix for the lp ticker to prevent a spurious interrupt. Note - the lp ticker implementation does correctly set the second match interrupt so it did not have problems with LowPowerTickerWrappper/LPTICKER_DELAY_TICKS.

Events leading up to this failure for the RTC:

1st call to lp_ticker_set_interrupt
delay until ticker interrupt is about to fire
2nd call to lp_ticker_set_interrupt
interrupt for 1st call fires and match time for 2nd call is dropped
-LowPowerTickerWrapper gets ticker interrupt but treats it as a
spurious interrupt and drops it since it comes in too early
device enters sleep without a wakeup source and locks up

This PR also adds tests to catch this regression in the future.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

jeromecoutant · 2018-09-14T08:11:53Z

targets/TARGET_STM/rtc_api.c

@@ -389,6 +389,7 @@ void rtc_set_wake_up_timer(timestamp_t timestamp)
    }

    RtcHandle.Instance = RTC;
+    HAL_RTCEx_DeactivateWakeUpTimer(&RtcHandle);


Is this really needed as rtc_deactivate_wake_up_timer function was already called in lp_ticker_set_interrupt before calling rtc_set_wake_up_timer?

Yes, rtc_deactivate_wake_up_timer by itself is not sufficient. If HAL_RTCEx_DeactivateWakeUpTimer is removed then this test will fail.

LMESTM · 2018-09-17T08:40:12Z

targets/TARGET_STM/lp_ticker.c

@@ -203,6 +203,10 @@ void lp_ticker_set_interrupt(timestamp_t timestamp)
    __HAL_LPTIM_CLEAR_FLAG(&LptimHandle, LPTIM_FLAG_CMPM);
    __HAL_LPTIM_COMPARE_SET(&LptimHandle, timestamp);

+    while (__HAL_LPTIM_GET_FLAG(&LptimHandle, LPTIM_FLAG_CMPOK) == RESET) {
+    }


Can you explain what is being done here ? Isn't there a risk to spend "a lot" of time in this while loop ?

Yes, this would cause blocking behavior in a critical section which is problematic. Without this delay the call to lp_ticker_clear_interrupt doesn't actually stop a spurious ticker interrupt from firing. Is it possible to prevent a spurious interrupt here without blocking? If so could you share the sequence so I could incorporate it into this PR?

Alternatively, if a fix cannot be found, I could modify the test to ignore spurious interrupts when LPTICKER_DELAY_TICKS > 0. The LowPowerTickerWrapper layer filters out spurious interrupts so when enabled these will not propagate to higher layers. What are you thoughts on this @LMESTM?

mprse

Test header and HAL lp ticker header (defined behavior, test functions) should be updated and additional test cases should be documented.

mprse · 2018-09-17T09:14:40Z

TESTS/mbed_hal/lp_ticker/main.cpp

+
+    const uint32_t mask = (1 << lp_ticker_get_info()->bits) - 1;
+
+    for (uint32_t i = 0; i < 100-0; i++) {


I believe that 100-0 should be 100.

Update the low power ticker test to verify that the low power ticker does not fire too early. Also add a test to verify that the low power ticker can be safely rescheduled right before or after a match occurs.

c1728p9 · 2018-09-17T15:34:09Z

Hi @mprse I updated the PR to address your comment and add test function declarations.

The defined behavior that was being violated is already specified:

mbed-os/hal/us_ticker_api.h

Lines 69 to 70 in 7a0c9a6

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

Both ST implementations of the low power ticker violate this (At least on the STM32L432):

RTC implementation (lpticker_lptim=0) - sometimes firing early and causing a lockup
lpticker implementation (lpticker_lptim=1) - always fires an interrupt at the time specified, but sometimes also fires a spurious interrupt early. This does not cause a lockup.

mprse

@c1728p9 Thanks for updating test header file and potential bugs section.

Tests looks good only one minor issue mentioned earlier (100-0) is to be fixed.

0xc0170 · 2018-09-18T09:13:34Z

@LMESTM @jeromecoutant Please review

mprse

Tests looks good!

c1728p9 · 2018-09-19T21:20:43Z

Marking this as "do not merge" until the blocking of the LP ticker is addressed.

jeromecoutant · 2018-09-25T12:40:17Z

Hi Russ

I propose:

to remove ST patch from this PR
to rename this PR into something like "Add LPTICKER tests"
to replace "do not merge" label by "need preceding PR"
to wait for STM32 LPTICKER : Fix tickless and LPTICKER_DELAY_TICKS #8242 merge

c1728p9 · 2018-09-25T22:55:25Z

Hi @jeromecoutant I descoped this PR to just the tests as you suggested.

jeromecoutant · 2018-10-08T08:35:56Z

@0xc0170 #8242 is merged, you can start CI for this one
Thx

0xc0170 · 2018-10-08T09:21:20Z

@0xc0170 #8242 is merged, you can start CI for this one

Thanks, I'll review shortly

cmonr · 2018-10-10T13:55:35Z

/morph build

mbed-ci · 2018-10-10T14:53:42Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-10-10T16:37:28Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-10-10T17:50:51Z

Test : FAILURE

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

0xc0170 · 2018-10-10T18:17:31Z

Failures related to the new test case, please review

0xc0170 · 2018-10-23T08:35:23Z

Please review the test failures or this can be closed until it's updated

cmonr · 2018-10-29T14:11:21Z

Closing this since progress appears to have idled.

Feel free to repoen once updates are availaible.

c1728p9 requested review from jeromecoutant, mprse and LMESTM September 14, 2018 02:22

c1728p9 mentioned this pull request Sep 14, 2018

Recent LP Ticker Changes Break Tickless #7858

Closed

cmonr added the needs: review label Sep 14, 2018

jeromecoutant reviewed Sep 14, 2018

View reviewed changes

LMESTM reviewed Sep 17, 2018

View reviewed changes

mprse suggested changes Sep 17, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Sep 17, 2018

Improve low power ticker test coverage

c904b43

Update the low power ticker test to verify that the low power ticker does not fire too early. Also add a test to verify that the low power ticker can be safely rescheduled right before or after a match occurs.

c1728p9 force-pushed the ticker_tests_and_fixes branch from b631242 to 3a12625 Compare September 17, 2018 15:18

mprse approved these changes Sep 18, 2018

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Sep 18, 2018

mprse approved these changes Sep 18, 2018

View reviewed changes

c1728p9 added the do not merge label Sep 18, 2018

mattbrown015 mentioned this pull request Sep 18, 2018

STM32L4: Adjusting RTC Synchronisation When Using MBED_TICKLESS in RTC Mode #7695

Closed

jeromecoutant mentioned this pull request Sep 25, 2018

STM32 LPTICKER : Fix tickless and LPTICKER_DELAY_TICKS #8242

Merged

c1728p9 added needs: preceding PR and removed do not merge labels Sep 25, 2018

c1728p9 changed the title ~~Fix tickless and LPTICKER_DELAY_TICKS on ST devices~~ Add LPTICKER tests Sep 25, 2018

c1728p9 force-pushed the ticker_tests_and_fixes branch from 3a12625 to c904b43 Compare September 25, 2018 22:54

0xc0170 removed the needs: preceding PR label Oct 8, 2018

0xc0170 approved these changes Oct 9, 2018

View reviewed changes

0xc0170 added needs: CI release-version: 5.10.2 and removed needs: review labels Oct 9, 2018

cmonr added needs: work and removed needs: CI labels Oct 10, 2018

cmonr added release-version: 5.10.3 and removed release-version: 5.10.2 labels Oct 18, 2018

0xc0170 added needs: work and removed needs: work release-version: 5.10.3 labels Oct 23, 2018

cmonr closed this Oct 29, 2018

cmonr removed the needs: work label Oct 29, 2018

c1728p9 mentioned this pull request Oct 29, 2018

Make common tickers test more strict #7598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LPTICKER tests #8129

Add LPTICKER tests #8129

c1728p9 commented Sep 14, 2018 •

edited

Loading

jeromecoutant Sep 14, 2018

c1728p9 Sep 14, 2018

LMESTM Sep 17, 2018

c1728p9 Sep 17, 2018

mprse left a comment

mprse Sep 17, 2018

c1728p9 commented Sep 17, 2018

mprse left a comment

0xc0170 commented Sep 18, 2018

mprse left a comment

c1728p9 commented Sep 19, 2018

jeromecoutant commented Sep 25, 2018

c1728p9 commented Sep 25, 2018

jeromecoutant commented Oct 8, 2018

0xc0170 commented Oct 8, 2018

cmonr commented Oct 10, 2018

mbed-ci commented Oct 10, 2018

mbed-ci commented Oct 10, 2018

mbed-ci commented Oct 10, 2018

0xc0170 commented Oct 10, 2018

0xc0170 commented Oct 23, 2018

cmonr commented Oct 29, 2018


		const uint32_t mask = (1 << lp_ticker_get_info()->bits) - 1;

		for (uint32_t i = 0; i < 100-0; i++) {

Add LPTICKER tests #8129

Add LPTICKER tests #8129

Conversation

c1728p9 commented Sep 14, 2018 • edited Loading

Description

Pull request type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c1728p9 commented Sep 17, 2018

mprse left a comment

Choose a reason for hiding this comment

0xc0170 commented Sep 18, 2018

mprse left a comment

Choose a reason for hiding this comment

c1728p9 commented Sep 19, 2018

jeromecoutant commented Sep 25, 2018

c1728p9 commented Sep 25, 2018

jeromecoutant commented Oct 8, 2018

0xc0170 commented Oct 8, 2018

cmonr commented Oct 10, 2018

mbed-ci commented Oct 10, 2018

Triggering tests

mbed-ci commented Oct 10, 2018

mbed-ci commented Oct 10, 2018

0xc0170 commented Oct 10, 2018

0xc0170 commented Oct 23, 2018

cmonr commented Oct 29, 2018

c1728p9 commented Sep 14, 2018 •

edited

Loading