Fix for Silicon Labs RTC #5854

stevew817 · 2018-01-15T15:04:14Z

Description

This commit fixes #5840. Fix verified by running mbed_hal-lp_ticker test suite on both EFM32GG and EFM32PG12 with preloaded RTC counter such that it wrapped in the middle of the suite.
Also removes explicit sleep blocking from the us_ticker implementation, since sleep blocking for us tickers is done at mbed HAL level now. This was causing one of the lp_ticker tests to fail.

Status

READY

Migrations

NO

chrissnow · 2018-01-15T15:46:05Z

How did you preload for your test? My way now seems to really mess things up!

stevew817 · 2018-01-15T15:59:09Z

@chrissnow same as you wrote in the bug report...

void lp_ticker_init()
{
    static bool first = true;
    if(first ==true)
    {
        RTCC->CNT=0xFFFF8000;
        first =false;
    }
    if(!rtc_reserved) {
        core_util_critical_section_enter();
        rtc_init_real(RTC_INIT_LPTIMER);
        rtc_set_comp0_handler((uint32_t)lp_ticker_irq_handler);
        rtc_reserved = 1;
        core_util_critical_section_exit();
    }
}

+---------------------------+-------------------+--------------------------+------------------------+--------+--------+--------+--------------------+
| target                    | platform_name     | test suite               | test case              | passed | failed | result | elapsed_time (sec) |
+---------------------------+-------------------+--------------------------+------------------------+--------+--------+--------+--------------------+
| EFM32PG12_STK3402-GCC_ARM | EFM32PG12_STK3402 | tests-mbed_hal-lp_ticker | 1ms lp_ticker          | 1      | 0      | OK     | 0.0                |
| EFM32PG12_STK3402-GCC_ARM | EFM32PG12_STK3402 | tests-mbed_hal-lp_ticker | 1s lp_ticker           | 1      | 0      | OK     | 1.01               |
| EFM32PG12_STK3402-GCC_ARM | EFM32PG12_STK3402 | tests-mbed_hal-lp_ticker | 1s lp_ticker deepsleep | 1      | 0      | OK     | 1.02               |
| EFM32PG12_STK3402-GCC_ARM | EFM32PG12_STK3402 | tests-mbed_hal-lp_ticker | 1s lp_ticker sleep     | 1      | 0      | OK     | 1.01               |
| EFM32PG12_STK3402-GCC_ARM | EFM32PG12_STK3402 | tests-mbed_hal-lp_ticker | 500us lp_ticker        | 1      | 0      | OK     | 0.0                |
| EFM32PG12_STK3402-GCC_ARM | EFM32PG12_STK3402 | tests-mbed_hal-lp_ticker | 5s lp_ticker           | 1      | 0      | OK     | 5.0                |
+---------------------------+-------------------+--------------------------+------------------------+--------+--------+--------+--------------------+

chrissnow · 2018-01-15T16:21:06Z

@stevew817 Looks to have upset some of my mods to get EM2 in idle.
seems OK if I dont preload but otherwise doesnt make it as far as main.

some of this isn't nice but did work before.
It's a merge of my mods and yours.
https://github.com/chrissnow/mbed-os/tree/EM2_RTC

chrissnow · 2018-01-15T16:51:03Z

Actually I think it's unrelated to my changes, Just using your branch I still have issues.

The simple test below doesn't like the preloading either.

 #include "mbed.h"

DigitalOut led(LED1, 1);

int main() {
	while (true) {
		led = !led;
		Thread::wait(500);
	}
}

stevew817 · 2018-01-15T17:56:13Z

@chrissnow I tried your simple test program as a clean program with mbed-os @ the bugfix branch patched with preloading as shown earlier, and it works fine here. This was on an EFM32PG12 STK3402.

I'm not sure where our differences come from?

This commit fixes ARMmbed#5840. Fix verified by running mbed_hal-lp_ticker test suite with preloaded RTC counter such that it wrapped in the middle of the suite. Also removes explicit sleep blocking from the us_ticker implementation, since sleep blocking for us tickers is done at mbed HAL level now. This was causing one of the lp_ticker tests to fail.

stevew817 · 2018-01-15T20:25:27Z

@chrissnow Figured it out, you are defining MBED_TICKLESS for the target, which hasn't been applied on master. This was causing thread:delay to not use the rtc/lp_ticker on my end.

Also figured out where I went wrong. Reading the RTC in interrupt context may involve a counter wrap without having handled the overflow interrupt yet. That's now fixed, and the implementation should work :)

chrissnow · 2018-01-16T11:00:20Z

@stevew817 Looks good to me :-)

I forgot that despite switching branches to get rid of my changes I needed to re export to get rid of tickless.

It's a shame you removed your own sleep blocking layer, with tickless enabled any users code would take advantage of the best sleep mode at any given time it was idle. the mbed HAL locking is a major step backwards in terms of energy management on EFM32.

stevew817 · 2018-01-16T12:32:40Z

We realize that the mbed HAL version of sleep blocking is less flexible, but at the same time we have to account for the rules and spirit of mbed OS, which is to provide a common-denominator abstraction such that one can effortlessly port applications across targets. mbed wants to push their implementation, so we removed our vendor-specific one.

But even for tickless RTOS, which will still always set a timer to expire, we'd be stuck with EM2. Reason is the LFXO isn't available in EM3, and RAM retention goes out the window in EM4. So for the time being, I don't really see much value in resurrecting our own sleep handling to maybe allow EM3. You're welcome to try and convince me otherwise, though :)

0xc0170 · 2018-01-16T13:03:45Z

/morph build

chrissnow · 2018-01-16T13:13:36Z

I appreciate your reasons, certainly makes sense, just a shame to lose what I found a key usability feature of the EFM32 vs other targets.

My main issue with the current implementation is that it wont even go into EM2 when it is idle without modifying the idle task since it locks deep sleep, I appreciate you need to know the limitations of this but your method took care of this.

EM3 and EM4 is probably not appropriate for what most mbed users want.

What would have been great is if the mbed sleep manager was able to do what yours did but abstracted for targets with similar functionality.

0xc0170 · 2018-01-16T13:15:34Z

My main issue with the current implementation is that it wont even go into EM2 when it is idle without modifying the idle task since it locks deep sleep.
EM3 and EM4 is probably not appropriate for what most mbed users want.

locks deep sleep for backward compability by default (deepsleep and systick does not play together) and only if tickless is disabled (=not supported), otherwise it would go. Once tickless is supported by target, it will go to deepsleep.

chrissnow · 2018-01-16T13:27:14Z

@0xc0170 I dont think that matches my experience, or my understanding of the code

mbed-os/rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

Lines 201 to 203 in 7be79f9

    
           sleep_manager_lock_deep_sleep(); 
        
           sleep(); 
        
           sleep_manager_unlock_deep_sleep();

I appreciate there may be more work to do than I did to enable tickless.

stevew817 · 2018-01-16T13:30:29Z

@0xc0170 That's my experience as well, the idle loop for tickless shouldn't block deepsleep unchecked before sleeping.

Actually, there are various places in the mbed codebase which lock deepsleep for no apparent reason, and this prevents tickless from being very useful. Are there plans to take away those blocks? Are you waiting for a user to PR changes?

0xc0170 · 2018-01-16T13:33:16Z

@0xc0170 That's my experience as well, the idle loop for tickless shouldn't block deepsleep unchecked before sleeping.

It should not, I'll check that one.

Actually, there are various places in the mbed codebase which lock deepsleep for no apparent reason, and this prevents tickless from being very useful. Are there plans to take away those blocks? Are you waiting for a user to PR changes?

Can you point to them? It's mostly in drivers, for those objects that operate often on high freq clocks.

mbed-ci · 2018-01-16T13:37:46Z

Build : SUCCESS

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

Triggering tests

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

stevew817 · 2018-01-16T13:43:45Z

Sure.

mbed-os/platform/mbed_wait_api_rtos.cpp

Line 42 in cd8e9e6

sleep_manager_lock_deep_sleep();

-> rtos scheduler should handle sleeping...

mbed-os/drivers/I2C.cpp

Line 137 in 31f59b9

lock_deep_sleep();

-> We have support for I2C in deep sleep... Actually, shouldn't all drivers leave the locking/unlocking up to the vendor? Or at least provide some configurability?

0xc0170 · 2018-01-16T13:58:21Z

-> rtos scheduler should handle sleeping...

wait API invokes us ticker there thus it locks. In this case we might want to just use there Ticker object instead that would lock automatically

-> We have support for I2C in deep sleep... Actually, shouldn't all drivers leave the locking/unlocking up to the vendor? Or at least provide some configurability?

We discussed this previously, that the current implementation provides generic sleep handling (covers most cases). There are peripherals that might operate in some cases, might provide full functionality or partial only.
We definitely can look at enabling more target specific requirements. You can create an issue with examples where this generic C++ deep sleep locking lacks configuration. We can discuss it there further.

stevew817 · 2018-01-16T14:08:06Z

You can create an issue with examples where this generic C++ deep sleep locking lacks configuration. We can discuss it there further.

Agreed, since this conversation is outside of the scope of this PR.

wait API invokes us ticker there thus it locks. In this case we might want to just use there Ticker object instead that would lock automatically

It should be possible to do something smarter than that. You can't really mandate a us ticker running for whichever waiting period a user can supply. I can understand using a us ticker for sub-ms waits (maybe sub-100ms), but for delays larger than that, you should really be able to use the lp ticker instead. Can we get this implemented, maybe with a configurable cutoff for using us timer instead of lp timer?

mbed-ci · 2018-01-16T14:48:35Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-01-16T15:50:24Z

Test : SUCCESS

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

cmonr · 2018-01-16T21:13:06Z

@stevew817 @0xc0170 @chrissnow Would y'all mind moving the discussion to it's own dedicated issue?
There's lots of good back and forth going on here, but I'd also like to get this PR merged since it looks ready :)

stevew817 · 2018-01-16T22:06:56Z

@cmonr Yes I will, in the meantime there’s nothing blocking the merge ;)

stevew817 mentioned this pull request Jan 15, 2018

EFM32: Unresponsive after 12 days #5840

Closed

0xc0170 added the needs: review label Jan 15, 2018

stevew817 force-pushed the bugfix/rtc_overflow branch from ca0cd6e to 5d6c5dd Compare January 15, 2018 20:22

0xc0170 approved these changes Jan 16, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Jan 16, 2018

0xc0170 added needs: CI and removed ready for merge labels Jan 16, 2018

cmonr added the ready for merge label Jan 16, 2018

cmonr removed the needs: CI label Jan 16, 2018

cmonr merged commit 3cb9728 into ARMmbed:master Jan 16, 2018

cmonr removed the ready for merge label Jan 16, 2018

0xc0170 added the release-version: 5.7.4 label Jan 17, 2018

amq mentioned this pull request Feb 28, 2018

InterruptIn not working with EFM32 and tickless #6205

Closed

Fix for Silicon Labs RTC #5854

Fix for Silicon Labs RTC #5854

Uh oh!

Conversation

stevew817 commented Jan 15, 2018

Description

Status

Migrations

Uh oh!

chrissnow commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevew817 commented Jan 15, 2018

Uh oh!

chrissnow commented Jan 15, 2018

Uh oh!

chrissnow commented Jan 15, 2018

Uh oh!

stevew817 commented Jan 15, 2018

Uh oh!

stevew817 commented Jan 15, 2018

Uh oh!

chrissnow commented Jan 16, 2018

Uh oh!

stevew817 commented Jan 16, 2018

Uh oh!

0xc0170 commented Jan 16, 2018

Uh oh!

chrissnow commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrissnow commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevew817 commented Jan 16, 2018

Uh oh!

0xc0170 commented Jan 16, 2018

Uh oh!

mbed-ci commented Jan 16, 2018

Build : SUCCESS

Triggering tests

Uh oh!

stevew817 commented Jan 16, 2018

Uh oh!

0xc0170 commented Jan 16, 2018

Uh oh!

stevew817 commented Jan 16, 2018

Uh oh!

mbed-ci commented Jan 16, 2018

Exporter Build : SUCCESS

Uh oh!

mbed-ci commented Jan 16, 2018

Test : SUCCESS

Uh oh!

cmonr commented Jan 16, 2018

Uh oh!

stevew817 commented Jan 16, 2018

Uh oh!

Uh oh!

chrissnow commented Jan 15, 2018 •

edited

Loading

chrissnow commented Jan 16, 2018 •

edited

Loading

0xc0170 commented Jan 16, 2018 •

edited

Loading

chrissnow commented Jan 16, 2018 •

edited

Loading