Enable deepsleep for LowPowerXXX objects #5148

mprse · 2017-09-20T09:45:18Z

Fix for issue #5076.

Description

This PR contains fix to be able to enter deep-sleep mode while using low power ticker/timer.

Status

READY

Migrations

YES
After this fix is when low power ticker/timer is in use deep-sleep will not be disabled.

0xc0170 · 2017-09-20T09:49:26Z

cc @c1728p9 @ccli8

pan-

What if I initialize a ticker with the following expression:

Ticker foo(get_lp_ticker_data());

The change is broken too easily.

0xc0170 · 2017-09-20T11:47:19Z

@pan-

what is your suggestion? introduce deep_sleep_allowed boolean (if lp ticker data are used, deepsleep is allowed by default) ?

    Ticker(const ticker_data_t *data) : TimerEvent(data), _function(0) {
        data->interface->init();
        // if lp ticker interface used
        // disable locking deep sleep
    }

Ticker foo(get_lp_ticker_data()); I would consider this as an intention, if Ticker is defined to be locking deepsleep, no matter what ticker interface is used, it locks deep sleep - that is the current situation as I understand. If we move this to ticker interface used as I highlighted above, then we are moving this promise to ticker interface, better?

mprse · 2017-09-20T12:02:23Z

@pan- This example breaks using convention of the Ticker class. If you want to use low power ticker you should use LowPowerTicker class.
If you decided to use Ticker class which operates on low power timer, then it is your decision, but you should be aware that deep-sleep is blocked in this case.
What do you suggest?

pan- · 2017-09-20T13:37:31Z

@pan- This example breaks using convention of the Ticker class. If you want to use low power ticker you should use LowPowerTicker class.

No its not breaking conventions, there is a public constructor for it. The LowPowerTicker class has always been a shorthand for Ticker(get_lp_ticker_data()) there is literally no other added value in that class.

what is your suggestion? introduce deep_sleep_allowed boolean (if lp ticker data are used, deepsleep is allowed by default) ?

My suggestion would be to expose whether the ticker continue to work in deep sleep or not in the ticker data so it can be reused by upper layers. I'd also drop the virtual keyword off lock_deepsleep and unlock_deepsleep.

As a side note I'd also make sure that lock and unlock goes in pair. It is not the case with the current code. If the user calls three time in a row attach_us without a detach in between the internal counter of the deepsleep lock will be incremented three times.

pan- · 2017-09-20T14:16:57Z

Note: In #5028 @c1728p9 add a get_info function which might be useful if we want to make sure a ticker peripheral can continue to work in deepsleep or not.

c1728p9 · 2017-09-20T14:17:05Z

A similar change will also be needed for Timer, as LowPowerTimer has a similar problem. @pan- did you have any ideas on where to expose ticker deep sleep operation info? This could be part of ticker get_info which is in PR #5028. Alternatively, this could be passed into the constructor like the ticker data itself is.

c1728p9 · 2017-09-20T14:18:00Z

@pan- you beat me to the post by 8 seconds

mprse · 2017-09-20T19:42:53Z

@pan @0xc0170
So as far as I understand, the solution is to use new feature provided in PR #5028 with a little extension.
We can add extra field to the ticker_info_t structure which will provide information if the ticker runs in deep-sleep mode or not. Then in the Ticker class we can use get_info() function from the specified ticker interface to determine if deep-sleep should be disabled or not.
It looks like I need to wait with updates in this PR until PR #5028 is processed to be able to build/test updates in Ticker class. Am I right?
Extra field to the ticker_info_t structure should be added as a part of this PR or PR #5028 ?

As a side note I'd also make sure that lock and unlock go in pair. It is not the case with the current code. If the user calls three time in a row attach_us without a detach in between the internal counter of the deepsleep lock will be incremented three times.

It looks like lock_deepsleep()/unlock_deepsleep() functions implement protection against multi calls to attache() / detatch() functions. Lock is performed only for the initial callback setup and unlock only when callback is registered.

A similar change will also be needed for Timer, as LowPowerTimer has a similar problem.

Fix to the Timer/LowPowerTimer should be provided in the additional PR?

0xc0170 · 2017-09-21T08:22:09Z

Fix to the Timer/LowPowerTimer should be provided in the additional PR?

I would clean it all within one patch.

It looks like lock_deepsleep()/unlock_deepsleep() functions implement protection against multi calls to attache() / detatch() functions. Lock is performed only for the initial callback setup and unlock only when callback is registered.

That should be the case, if it's not is a bug.

@pan @0xc0170
So as far as I understand, the solution is to use new feature provided in PR #5028 with a little extension.
We can add extra field to the ticker_info_t structure which will provide information if the ticker runs in deep-sleep mode or not. Then in the Ticker class we can use get_info() function from the specified ticker interface to determine if deep-sleep should be disabled or not.
It looks like I need to wait with updates in this PR until PR #5028 is processed to be able to build/test updates in Ticker class. Am I right?
Extra field to the ticker_info_t structure should be added as a part of this PR or PR #5028 ?

How far are with with 5028? If we can make progress with that one fast (there is Ci running now). We will need to add there that info plus fix these classes also.

Because I can see also another path. To fix this by just getting ticker data - if it's lp ticker, deepsleep locking is disabled (that is what we have now, no info). This would be intermediate fix that 5028 could clean up once we get the proper info in.

mprse · 2017-09-21T09:00:36Z

Because I can see also another path. To fix this by just getting ticker data - if it's lp ticker, deepsleep locking is disabled (that is what we have now, no info). This would be intermediate fix that 5028 could clean up once we get the proper info in.

I understand that this solution is a workaround for now. Should i proceed with this fix?

0xc0170 · 2017-09-21T09:09:19Z

Yes, I discussed it with @pan- , agrees also. please update

pan- · 2017-09-21T10:23:54Z

To fix this by just getting ticker data - if it's lp ticker, deepsleep locking is disabled (that is what we have now, no info).

That's the best solution for now, it doesn't require API changes and it should always work.

It looks like lock_deepsleep()/unlock_deepsleep() functions implement protection against multi calls to attache() / detatch() functions. Lock is performed only for the initial callback setup and unlock only when callback is registered.

That's right, apologies I misread the code.

Fix for issue ARMmbed#5076.

0xc0170 · 2017-09-28T10:16:28Z

drivers/Timer.cpp

 }

 void Timer::start() {
    core_util_critical_section_enter();
    if (!_running) {
-        sleep_manager_lock_deep_sleep();
+        if(_lock_deepsleep) {


There should be space if (

0xc0170 · 2017-09-28T10:16:59Z

@pan- @c1728p9 Please rereview, the code was updated based on our suggestions

0xc0170 · 2017-09-28T10:56:37Z

/morph test

mbed-bot · 2017-09-28T16:50:52Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1421

All builds and test passed!

0xc0170 approved these changes Sep 20, 2017

View reviewed changes

0xc0170 added the needs: CI label Sep 20, 2017

pan- requested changes Sep 20, 2017

View reviewed changes

0xc0170 added needs: review and removed needs: CI labels Sep 20, 2017

mprse force-pushed the fix_enable_deepsleep_for_lp_timer branch from 380ba30 to 06a3db9 Compare September 21, 2017 10:29

Enable deepsleep for low power Ticker and low power Timer.

10ee2fa

Fix for issue ARMmbed#5076.

mprse force-pushed the fix_enable_deepsleep_for_lp_timer branch from 06a3db9 to 10ee2fa Compare September 22, 2017 06:30

0xc0170 reviewed Sep 28, 2017

View reviewed changes

0xc0170 changed the title ~~Enable deepsleep for LowPowerTicker.~~ Enable deepsleep for LowPowerXXX objects Sep 28, 2017

0xc0170 approved these changes Sep 28, 2017

View reviewed changes

pan- approved these changes Sep 28, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Sep 28, 2017

This was referenced Sep 28, 2017

Deep sleep lock would overflow (> USHRT_MAX) #5067

Closed

Ticker: add dtor implementation #5217

Closed

0xc0170 mentioned this pull request Sep 28, 2017

Test: deepsleep() API replacement #5147

Merged

c1728p9 approved these changes Sep 28, 2017

View reviewed changes

theotherjimmy added ready for merge and removed needs: CI labels Sep 28, 2017

theotherjimmy merged commit bb61b42 into ARMmbed:master Sep 28, 2017

theotherjimmy added release-version: 5.6.2 and removed ready for merge labels Sep 28, 2017

c1728p9 mentioned this pull request Oct 3, 2017

Fix LowPowerTimer and nightly builds #5242

Merged

Enable deepsleep for LowPowerXXX objects #5148

Enable deepsleep for LowPowerXXX objects #5148

Uh oh!

Conversation

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

Uh oh!

Description

Status

Migrations

Uh oh!

0xc0170 commented Sep 20, 2017

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pan- commented Sep 20, 2017

Uh oh!

pan- commented Sep 20, 2017

Uh oh!

c1728p9 commented Sep 20, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

0xc0170 commented Sep 21, 2017

Uh oh!

mprse commented Sep 21, 2017

Uh oh!

0xc0170 commented Sep 21, 2017

Uh oh!

pan- commented Sep 21, 2017

Uh oh!

0xc0170 Sep 28, 2017

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 28, 2017

Uh oh!

0xc0170 commented Sep 28, 2017

Uh oh!

mbed-bot commented Sep 28, 2017

Result: SUCCESS

Output

Uh oh!

Uh oh!

mprse commented Sep 20, 2017 •

edited

Loading

0xc0170 commented Sep 20, 2017 •

edited

Loading

mprse commented Sep 20, 2017 •

edited

Loading

c1728p9 commented Sep 20, 2017 •

edited

Loading

mprse commented Sep 20, 2017 •

edited

Loading