Don't error from node.sleep() when a sleep reject occurs #3560
Conversation
| @@ -282,6 +282,13 @@ static int node_sleep (lua_State *L) | |||
| int err = esp_light_sleep_start(); | |||
| if (err == ESP_ERR_INVALID_STATE) { | |||
| return luaL_error(L, "WiFi and BT must be stopped before sleeping"); | |||
| } else if (err == 1) { | |||
There was a problem hiding this comment.
Can you use ESP_ERR_SLEEP_REJECT here? This return code is documented. There is also ESP_ERR_SLEEP_TOO_SHORT_SLEEP_DURATION which maybe should be handled in the same way.
It also isn't clear what the right thing to do is -- it might be surprising to some developers that sleep might not sleep.
|
While I like not having errors thrown needlessly, I don't see how we could find out the correct return value in the reject case? |
|
In some sense, the two error cases are 'system didn't sleep because the timeout happened or the wakeup condition was already satisfied'. In some senses this is the same as 'it timed out, or the wakeup condition was triggered' |
|
We currently return the wakeup cause from this function, but I guess we could use |
devbranch rather than for thereleasebranch.docs/*.In IDFv4 there's a new return value from esp_light_sleep_start which needs handling in node.sleep(). It's not really documented as far as I can see, but in the IDF source it's termed a "sleep reject" meaning (I think) that the conditions for wakeup already apply so the board doesn't actually ever go to sleep. You see it if you try to sleep with a GPIO low wakeup, and the GPIO is already low, for example. We really don't care about this nuance (compared to sleeping then immediately waking up), so the change is to just ignore the (undocumented) return value from esp_light_sleep_start and treat it the same as slept-then-woke-up, rather than erroring.
Now I think about it some more writing this PR, given it's not documented, maybe we should just return a boolean from node.sleep() indicating whether the sleep succeeded or not. Never error, don't make assumptions about the return value other than zero/nonzero, and get rid of the ESP_ERR_INVALID_STATE check which doesn't appear to actually be used any more?