Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: dev-esp32
Choose a base branch
from

Conversation

tomsci
Copy link

@tomsci tomsci commented Dec 4, 2022

  • This PR is for the dev branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • 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?

@nwf nwf added the ESP32 label Dec 25, 2022
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

@jmattsson
Copy link
Member

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?

@pjsg
Copy link
Member

pjsg commented Jan 30, 2024

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'

@jmattsson
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants