Return error in class deepSleep() when max sleeptime is exceeded #4936

Marc-Vreeburg · 2018-07-18T08:54:19Z

Proposed codechange returns 0 when max sleeptime is exceeded.

devyte · 2018-07-19T22:06:00Z

cores/esp8266/Esp.cpp

+       #endif
+       return 0; // error: max sleeptime exceeded
+    }
+   else


Please remove else and braces.

Removed else and braces.

Removed return 1;

devyte

Apologies, I missed this detail in my previous review.

devyte · 2018-07-20T15:19:21Z

cores/esp8266/Esp.cpp

+       #ifdef DEBUG_SERIAL
+          DEBUG_SERIAL.println("Error: max sleeptime exceeded");
+       #endif
+       return 0; // error: max sleeptime exceeded


return type is bool, but returned value is int => return false here

You're right.
Strange though that this is not detected by git code check.

devyte · 2018-07-20T15:19:36Z

cores/esp8266/Esp.cpp

    system_deep_sleep_set_option(static_cast<int>(mode));
    system_deep_sleep(time_us);
    esp_yield();
+    return 1; // never gets called


return type is bool, but returned value is int => return true here

liebman · 2018-07-20T16:08:25Z

I have a question. I've noticed that the deepSleepMax() can vary. For instance deep sleep an hour, take an action and then go back to sleep. I print the deepSleepMax() value and it changes. Does this value only change during a sleep or could it very between calling deepSleepMax() and then calling deepSleepMax()? Asking because that could create a race condition even with the check being added here.

devyte · 2018-07-20T18:56:53Z

I think it can only vary within the sys context, i.e.: between loop()s, or between a yield() and return. That means that it shouldn't vary between the call to deepSleepMax() and the actual sleep.
The above is an educated guess though. It's based on my experience with how the SDK works in general, and not on any deep knowledge of what goes on under the hood wrt sleep, so I could be wrong.

earlephilhower

I'm not a user of the ESP in powerdown modes, and this patch is technically correct, so I'm fine approving this patch.

However, since you are a user, do please think about what you and others would expect it to do if passed in a value that's greater than the (potentially variable) deepSleepMax() time.

earlephilhower · 2018-07-25T18:08:58Z

cores/esp8266/Esp.cpp

 {
+    if (time_us > deepSleepMax())


Would it not be more useful (and help fix peoples code which is broken already) if, instead of possibly printing an error and return false; , you just printed a warning and adjusted the time down to deepSleepMax()?

Existing code will not expect to ever get a return value and potentially go off into lala-land, since this was a void.

"Be conservative in what you do, be liberal in what you accept from others."

@earlephilhower Point taken. I'll make some changes to the code.

earlephilhower

Looks good with just the floating point math issue noted below.

earlephilhower · 2018-07-26T17:53:20Z

cores/esp8266/Esp.cpp

@@ -109,6 +109,13 @@ extern "C" void esp_yield();

 void EspClass::deepSleep(uint64_t time_us, WakeMode mode)
 {
+    if (time_us > deepSleepMax()) // we need to prevent the esp8266 from not waking up from deepsleep
+    {
+       time_us = (deepSleepMax() - (round(deepSleepMax() * 5 / 100))); // 5% correction because of inaccurate timekeeping by the esp8266


No need for round() here. You're doing integer division already, so this takes the integer division result, promotes it to a double, then promotes deepSleepMax() to a double, subtracts them as doubles, then truncates it all back to an integer. A shorter way which won't cause a program to include the whole floating point libs (if not being included already):
time_us = (deepSleepMax() * 95L) / 100L;
or if time_us can be > 2^24 (so the *95 but could overflow)
time_us = deepSleepMax() - (deepSleepMax() / 20);

I bring this up because there was much howling of pain when we included floating point support in printf with the latest release, as some folks binaries ended up being just a hair too big for OTA...

devyte · 2018-07-26T18:42:16Z

@marcvtew did you actually encounter a case where the deepsleep would get stuck with a value close to the max?
I'm not an expert in this, but my understanding is that the max value is related to the max number that can be put in some timer without overflowing it. If it does overflow, then behavior is undefined. I say related because there is another internal factor that gets updated every so often.
What I mean is that, in theory, sleeping for max time should always be possible. If it's not, then that should be documented and reported to Espressif.
Or is this 5% meant to be more accurate wrt how long the ESP actually sleeps? If so, then most likely it should be shaved off in all cases, not just when the specified value is > max.

Marc-Vreeburg · 2018-07-26T20:23:09Z

@devyte Made some code to read and display maxSleeptime(). The esp8266 on my Lolin board has a max sleeptime of 13840678905 microseconds. I'll set my sleeptime to 13840678904 microseconds and see if it wakes up.

Actually there's a difference of more than 35 seconds between two different calculations of maxSleeptime(). So i'll settle for a sleeptime of 13800000000 microseconds (230 minutes).

Marc-Vreeburg · 2018-07-26T22:57:43Z

@devyte I made some code to measure the value of maxDeepsleep() every two seconds, remembering the absolute max value and the absolute min value measured. I let the code run for about 15 minutes to get a stable result:
max deep sleep MAX: 14386462713 microseconds
max deep sleep MIN: 13704364025 microseconds
max deep sleep DIF: 682098688 microseconds

So, in my case the shortest calculated maxsleeptime differs more than 11 minutes (within max around 3,8 to around 4 hours deepsleep) from the longest calculated maxsleeptime. It looks to me that it's not sufficient to only test (and set) deepsleeptime once (i.e. before going into sleep) because during deepsleep the calculated maxsleeptime may become shorter (worst case: more than 11 minutes) resulting in a timer overflow (esp8266 never restarts allthough we passed if (time_us > (deepSleepMax())).

Safe deepsleep time for me seems to be13704364025 microseconds. With this sleeptime the esp8266 should always wake up. I'll test this tomorrow.
Esp.cpp needs an adjustment like this i think:
if (time_us > (deepSleepMax() - 682098688 or % of deepSleepMax()) // choose one of the two methods { time_us = deepSleepMax() - 682098688 or % of deepSleepMax()); // choose one of the two methods }

d-a-v · 2018-08-03T08:33:54Z

True. datasheet tells 2^31-1.
So we would have these equiv:

(time_in_us / cali) << 12 < 2^31 - 1 (datasheet)
time_in_us < (cali << (31-12))
time_in_us < (cali<<19)
time_in_s * 1000000 < cali * 524288
time_in_s < cali * 0.524288

~~implies~~ assured by

time_in_s < cali/2

But (datasheet):

the cali is the RTC clock period (in us); bit11 ~ bit0 are decimal.

So we need either interpret this decimal part (datasheet gives the way to interpret it, it gives a maximum of 1000s / 16mn40s - not 100% sure about this)), or remove it:

time_in_s < ((cali & ~((1<<12)-1)) / 2), same as
time_in_s < ((cali >> 12) << 11)

My board gives 10240s with this formula (2h50mn). I remember having seen it fluctuating a bit.
edit: now 12288s (3h24mn)

5chufti · 2018-08-03T08:51:28Z

for me this is purely academic, I cannot see any use in ESP.deepSleep(ESP.deepSleepMax());
For a start I would have expected the API call to return false if the max_deep_sleep_time is exceeded, but I forgot we are dealing with espressif ...

So the decent thing to do would be to either
a) replace the passed value with tmax if exceeded
(I think this is what API does instead of returning false)
b) return with "false" if tmax exceeded

just for the unlikely case that someone really exeeds tmax for ESP.deepSleep();
They should beforhand check on the use and probable range in documentation and/or div forum posts. I don't even see need of deepSleepMax().

regarding 2^31 vs. 2^32 see here #4969 (comment) (or 3.3.9 vs 3.3.50)

artua · 2018-08-03T09:09:31Z

@5chufti for me, knowing the deepSleepMax() is about to save battery on some applications. For now, I wake ESP once a hour with modem sleeping, check the sleep cycles left and put it to deepSleep next cycle. But anyway, it’s ~30mA consumption while I’m checking these cycles. So it’s not academic, it’s purely practical question.

…

On Fri, Aug 3, 2018 at 10:52 AM 5chufti ***@***.***> wrote: for me this is purely academic, I cannot see any use in ESP.deepSleep(ESP.deepSleepMax()); For a start I would have expected the API call to return false if the max_deep_sleep_time is exceeded, but I forgot we are dealing with espressif ... So the decent thing to do would be to either a) replace the passed value with tmax if exceeded (I think this is what API does instead of returning false) b) return with "false" if tmax exceeded just for the unlikely case that someone really exeeds tmax for ESP.deepSleep(); They should beforhand check on the use and probable range in documentation and/or div forum posts. I don't even see need of deepSleepMax(). regarding 2^31 vs. 2^32 see here #4969 (comment) <#4969 (comment)> (or 3.3.9 vs 3.3.50) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4936 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGV_3Xpm3pqTIzByLlpWCyTVYmIL2hRPks5uNA8qgaJpZM4VUMs8> .

d-a-v · 2018-08-03T09:34:05Z

check the sleep cycles left

How are you doing that ?

5chufti · 2018-08-03T10:06:01Z

@artua
still I wouldn't use deepSleepMax() but a value that I can deal with, like 9000s (2.5)h.
without any true reference like ntp or rtc you'll never get repeatable cycles anyhow.

@marcvtew
your formula rewrite is not mathematically correct

@d-a-v
probably he has his 1024bit counter in rtc-ram, decrementing by current deepSleepMax() just before deepSleep() in every cycle... (but don't forget to correct by micros(), since wake time also counts!)

5chufti · 2018-08-03T10:18:38Z

do we really want/need two deepSleepMax versions?

Marc-Vreeburg · 2018-08-03T10:29:14Z

I'm using my esp8266's for IoT purposes, mostly battery powered. For me not knowing the safe max sleeptime makes the esp8266 totally useless.

5chufti · 2018-08-03T10:36:46Z

thats why I argue for only one function, returning a safe value.
Who needs a function returning a value "without any value" (not being reliable)?

Marc-Vreeburg · 2018-08-03T11:19:21Z

@5chufti I agree that it's best to have only one meaningful function. However ths might mean that we will be deviating from the api specification of deepSleepMax().
I've tested ESP.deepSleep(ESP.deepSleepMax(), WAKE_RF_DEFAULT) twice. Both times the esp8266 eventually did NOT wake up from deepsleep anymore. Is it possible for you to check my results?

d-a-v · 2018-08-03T11:56:03Z

Since we are still speaking in uS, and bits 0..11 are BCD, and(?) we don't wish to deal with them, we could use:
time_in_us < ( ((uint64_t)(system_rtc_clock_cali_proc() >> 12)) << (12+19))

artua · 2018-08-03T12:09:00Z

@5chufti I'm ok with that. But how can I know the max safe value? If the deepSleepMax() will return any safe constant - it would be great. Let it be deepSleepSafe() or anything like that.

…

On Fri, Aug 3, 2018 at 2:56 PM, david gauchard ***@***.***> wrote: Since we are still speaking in uS, and bits 0..11 are BCD and(?) we don't wish to deal with them, we could use: time_in_us < ((((uint64_t)system_rtc_clock_cali_proc()) >> 12) << (12+19)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4936 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGV_3fM2ZUZvPgopaWyxs70FUsL2L0xeks5uNDqEgaJpZM4VUMs8> .

5chufti · 2018-08-03T13:18:07Z

@d-a-v
BCD? any confidential info? not even the rtc "example" in A.2 does suggest that?

@marcvtew
as deepSleepMax() is allready in the latest official release, keep it, BUT have it return a safe value (can be documented in function why, it more likely serves the purpose than the dubious calculation example in the sdk doc)

edit: former test result was not correct, have to repeat

5chufti · 2018-08-03T20:54:57Z

@marcvtew
unfortunately your right, no wake-up when tmax is exceeded.
I call this a severe API flaw, system_deep_sleep() is of type bool and should return false - when, if not at least when called with too high deepsleep time?
but I really wonder: how did they do it, so it never wakes up again?
If you load a timer register with a value exeeding the register width, one would usually assume the tineout would occur after the "modulo" ...

devyte · 2018-08-03T21:35:24Z

@5chufti some timers don't continue to count past overflow, or have undefined behavior if they overflow. Another explanation is that some interrupt is triggered on overflow, but no ISR is assigned, or a garbage ISR is assigned.
Whatever else, I see two distinct usage cases here:

sleep for a specific time, which should always be "safe"
sleep for the max time possible, whatever that time is, even if that max time changes over time

The key point here is that neither of the above cases is really addressed in the current Espressif SDK API or docs.

The formula for the max theoretical sleep time is useless, because cali could change between the calculation and sleep start. Per @5chufti it could change even between calls within the same loop(). I didn't observe this in my own tests, for me it was always constant within a loop, but could change between loops, or during a yield(), but then again I didn't exactly spend a lot of time on that testing.
Given that race condition, there is no way that 1. or 2. above can be done "safely".
If the max time is exceeded, in some cases you get an error message on Serial, in other cases the ESP doesn't wake up. I suspect that the code that checks the sleep time on the SDK side suffers from the same race condition due to cali changing.

5chufti · 2018-08-04T07:53:58Z

breaking news:

system_deep_sleep_instant(time_us);

behaves much better as it at least restarts if tmax is exceeded.
(and also only 2^31-1 is feasible -> another "glitch" in the datasheet)

@devyte
we're talking about hw timer / compare registers, so no isr etc.

5chufti · 2018-08-04T09:13:10Z

I now personally have settled for this approach

void EspClass::deepSleep(uint64_t time_us, WakeMode mode)
{
    if (mode == WAKE_NO_RFCAL) 
       system_phy_set_powerup_option (2);
    else
       system_deep_sleep_set_option(static_cast<int>(mode));
    if (time_us & 0x0800000000ull)
	time_us = (uint64_t)system_rtc_clock_cali_proc() * 0x7A120ull;
    if (time_us & 0x1000000000ull)
    	system_deep_sleep(time_us);
    else
    	system_deep_sleep_instant(time_us);
    esp_yield();
}

now I can easily select the functionality I want with

ESP.deepSleep(sleepTime[, wakeupmode])                         // backward compatible   
ESP.deepSleep(sleepTime | 0x0800000000ull[, wakeupmode]);      // do max safe deepsleep
ESP.deepSleep(sleepTime | 0x1000000000ull[, wakeupmode]);      // do instant deepsleep
ESP.deepSleep(sleepTime | 0x1800000000ull[, wakeupmode]);      // do max safe instant deepsleep

Edit: inserted probable workaround for #3408

I'm currently testing wether this code always wakes up the esp8266.

devyte · 2018-08-06T20:41:25Z

The current implementation uses system_deep_sleep(), which internally shuts down the wifi core "safely" (whatever that means), and then goes to sleep.
I think that to get this to work as expeted, @5chufti is correct in that we should use system_deep_sleep_instant() to go to sleep immediately, which should remove the race condition.
My only worry is the meaning of shutting down the wifi core "safely", but there are several possibilities here:

going to sleep immediately has no impact on sleep power consumption, so we don't care about safe or non-safe
the user is responsable for shutting down wifi before going to sleep
we shut down wifi before calling these api calls, and hide that from the user

Marc-Vreeburg · 2018-08-07T08:27:03Z

The past 30 hours i've tested wether the esp8266 wakes up with deepSleepMax() = ((uint64_t)system_rtc_clock_cali_proc()/2*(0xF4240ULL)-1). My esp8266 restarted 11 times so using deepSleepMax() = ((uint64_t)system_rtc_clock_cali_proc()/2*(0xF4240ULL)-1) seems to always wake up the esp8266.

In one of the api docs i remember to have read that system_deep_sleep_instant() function is void. Is this function still being supported by Espressif?

I did some testing shutting down wifi before going into system_deep_sleep(). Made no difference: esp8266 did not wake up with time_us = deepSleepMax(). Maybe someone else is willing to test this as well?

Check max sleeptime in class deepSleep()

37b8fa1

Proposed codechange returns 0 when max sleeptime is exceeded.

Marc-Vreeburg mentioned this pull request Jul 18, 2018

OTA only works the first time #4831

Closed

Marc-Vreeburg and others added 2 commits July 18, 2018 11:15

Update Esp.h

c67c733

Merge branch 'master' into patch-1

ed9967d

devyte requested changes Jul 19, 2018

View reviewed changes

cores/esp8266/Esp.cpp Outdated

#endif

return 0; // error: max sleeptime exceeded

}

else

Copy link

Collaborator

devyte Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove else and braces.

Marc-Vreeburg added 4 commits July 20, 2018 09:31

Merge branch 'master' into patch-1

270da15

Update Esp.cpp

0d0dd7b

Removed else and braces.

Update Esp.cpp

a86f817

Removed return 1;

Update Esp.cpp

89854fc

devyte approved these changes Jul 20, 2018

View reviewed changes

devyte requested a review from earlephilhower July 20, 2018 15:16

devyte requested changes Jul 20, 2018

View reviewed changes

Update Esp.cpp

fda08a6

Merge branch 'master' into patch-1

d58f66a

devyte approved these changes Jul 20, 2018

View reviewed changes

devyte and others added 2 commits July 23, 2018 16:32

Merge branch 'master' into patch-1

609869f

Merge branch 'master' into patch-1

8e270ac

earlephilhower approved these changes Jul 25, 2018

View reviewed changes

Marc-Vreeburg and others added 5 commits July 26, 2018 16:39

Update Esp.cpp

0c3b3e6

Update Esp.h

d0c16b5

Update Esp.cpp

b5b218a

Merge branch 'master' into patch-1

5e5f13e

Update Esp.cpp

033d5ef

earlephilhower requested changes Jul 26, 2018

View reviewed changes

Update Esp.cpp

ac538c2

Marc-Vreeburg added 3 commits August 3, 2018 12:08

Update Esp.cpp

fada2f6

Merge branch 'master' into patch-1

cebeafc

Update Esp.h

39a2121

devyte mentioned this pull request Aug 3, 2018

[TW#25879] system_deep_sleep with max time sometimes doesn't wake up, possible race condition espressif/ESP8266_NONOS_SDK#157

Open

Marc-Vreeburg added 5 commits August 4, 2018 19:49

Update Esp.cpp

24bb46d

I'm currently testing wether this code always wakes up the esp8266.

Update Esp.h

26021c1

Merge branch 'master' into patch-1

a8359f5

Update Esp.cpp

364f341

Update Esp.cpp

eab0b4d

5chufti mentioned this pull request Aug 16, 2018

Add instantly option to deepSleep #5052

Merged

TD-er mentioned this pull request Jan 6, 2019

[deepSleep] Make deepSleep work again with 2.5.0 and refactor delay var letscontrolit/ESPEasy#2217

Merged

Merge branch 'master' into patch-1

dfdb0dc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return error in class deepSleep() when max sleeptime is exceeded #4936

Return error in class deepSleep() when max sleeptime is exceeded #4936

Marc-Vreeburg commented Jul 18, 2018

devyte Jul 19, 2018

devyte left a comment

devyte Jul 20, 2018

Marc-Vreeburg Jul 20, 2018

devyte Jul 20, 2018

liebman commented Jul 20, 2018

devyte commented Jul 20, 2018

earlephilhower left a comment

earlephilhower Jul 25, 2018

Marc-Vreeburg Jul 26, 2018

earlephilhower left a comment

earlephilhower Jul 26, 2018 •

edited

devyte commented Jul 26, 2018

Marc-Vreeburg commented Jul 26, 2018 •

edited

Marc-Vreeburg commented Jul 26, 2018

d-a-v commented Aug 3, 2018 •

edited

5chufti commented Aug 3, 2018

artua commented Aug 3, 2018 via email

d-a-v commented Aug 3, 2018

5chufti commented Aug 3, 2018 •

edited

5chufti commented Aug 3, 2018

Marc-Vreeburg commented Aug 3, 2018

5chufti commented Aug 3, 2018

Marc-Vreeburg commented Aug 3, 2018

d-a-v commented Aug 3, 2018 •

edited

artua commented Aug 3, 2018 via email

5chufti commented Aug 3, 2018 •

edited

5chufti commented Aug 3, 2018

devyte commented Aug 3, 2018

5chufti commented Aug 4, 2018 •

edited

5chufti commented Aug 4, 2018 •

edited

devyte commented Aug 6, 2018 •

edited

Marc-Vreeburg commented Aug 7, 2018

Return error in class deepSleep() when max sleeptime is exceeded #4936

Are you sure you want to change the base?

Return error in class deepSleep() when max sleeptime is exceeded #4936

Conversation

Marc-Vreeburg commented Jul 18, 2018

devyte Jul 19, 2018

Choose a reason for hiding this comment

devyte left a comment

Choose a reason for hiding this comment

devyte Jul 20, 2018

Choose a reason for hiding this comment

Marc-Vreeburg Jul 20, 2018

Choose a reason for hiding this comment

devyte Jul 20, 2018

Choose a reason for hiding this comment

liebman commented Jul 20, 2018

devyte commented Jul 20, 2018

earlephilhower left a comment

Choose a reason for hiding this comment

earlephilhower Jul 25, 2018

Choose a reason for hiding this comment

Marc-Vreeburg Jul 26, 2018

Choose a reason for hiding this comment

earlephilhower left a comment

Choose a reason for hiding this comment

earlephilhower Jul 26, 2018 • edited

Choose a reason for hiding this comment

devyte commented Jul 26, 2018

Marc-Vreeburg commented Jul 26, 2018 • edited

Marc-Vreeburg commented Jul 26, 2018

d-a-v commented Aug 3, 2018 • edited

5chufti commented Aug 3, 2018

artua commented Aug 3, 2018 via email

d-a-v commented Aug 3, 2018

5chufti commented Aug 3, 2018 • edited

5chufti commented Aug 3, 2018

Marc-Vreeburg commented Aug 3, 2018

5chufti commented Aug 3, 2018

Marc-Vreeburg commented Aug 3, 2018

d-a-v commented Aug 3, 2018 • edited

artua commented Aug 3, 2018 via email

5chufti commented Aug 3, 2018 • edited

5chufti commented Aug 3, 2018

devyte commented Aug 3, 2018

5chufti commented Aug 4, 2018 • edited

5chufti commented Aug 4, 2018 • edited

devyte commented Aug 6, 2018 • edited

Marc-Vreeburg commented Aug 7, 2018

earlephilhower Jul 26, 2018 •

edited

Marc-Vreeburg commented Jul 26, 2018 •

edited

d-a-v commented Aug 3, 2018 •

edited

5chufti commented Aug 3, 2018 •

edited

d-a-v commented Aug 3, 2018 •

edited

5chufti commented Aug 3, 2018 •

edited

5chufti commented Aug 4, 2018 •

edited

5chufti commented Aug 4, 2018 •

edited

devyte commented Aug 6, 2018 •

edited