nodemcu-firmware icon indicating copy to clipboard operation
nodemcu-firmware copied to clipboard

Don't error from node.sleep() when a sleep reject occurs

Open tomsci opened this issue 2 years ago • 3 comments

  • [x] This PR is for the dev branch rather than for the release branch.
  • [x] This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • [x] I have thoroughly tested my contribution.
  • [x] The code changes are reflected in the documentation at 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?

tomsci avatar Dec 04 '22 16:12 tomsci

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?

jmattsson avatar Jan 30 '24 01:01 jmattsson

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'

pjsg avatar Jan 30 '24 02:01 pjsg

We currently return the wakeup cause from this function, but I guess we could use ESP_SLEEP_WAKEUP_UNDEFINED for the sleep-rejected case? Not sure whether that's cleaner that the current behaviour though?

jmattsson avatar Jan 30 '24 04:01 jmattsson