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

Add instantly option to deepSleep #5052

Merged
merged 3 commits into from
Aug 19, 2018

Conversation

torntrousers
Copy link
Contributor

As mentioned in #3408 (comment) here's an update to add an 'instantly' option to the deepSleep function.

@@ -92,7 +92,7 @@ class EspClass {
void wdtDisable();
void wdtFeed();

void deepSleep(uint64_t time_us, RFMode mode = RF_DEFAULT);
void deepSleep(uint64_t time_us, RFMode mode = RF_DEFAULT, bool instantly = false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding new function, deepSleepInstant. This way, only one of system_deep_sleep_instant/system_deep_sleep will be linked into the final application, depending on what the user needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

@5chufti
Copy link
Contributor

5chufti commented Aug 16, 2018

together with the request for a safe max option like requested in #4969 I opt for something like proposed here #4936 (comment)

@torntrousers
Copy link
Contributor Author

torntrousers commented Aug 16, 2018

Doesn't that still have the issue that @igrr just mentioned of linking in a bit more than necessary. Also 0x1800000000ull is a little obscure compared to explicitly mentioning instant somewhere.

@5chufti
Copy link
Contributor

5chufti commented Aug 16, 2018

yes but then why not also seperate ESP.deepSleep() and ESP.setWakeUp()
to me this is academic hair-splitting. To get least pulled in from SDK, one should not combine any SDK functions within single ESP.class method at all, ending up in ESP.class just passing through simple SDK api. With the suggested workaround for #3408 it's even one more ...

I don't mind having 2 additional bool arguments,

void deepSleep(uint64_t time_us, RFMode mode = RF_DEFAULT, bool max_safe = FALSE, bool instant = FALSE);

no hard feelings for obscure flags in arguments, just wanted to keep it max compat.

@igrr
Copy link
Member

igrr commented Aug 16, 2018

Dear @5chufti, if a user encounters the following piece of code

ESP.deepSleep(duration, RF_DEFAULT, true);

they will need to go find the function declaration to figure out what "true" means.

Compare this to

ESP.deepSleepInstant(duration, RF_DEFAULT);

where the method name conveys this information.

Admittedly, the core has accumulated numerous APIs with optional boolean arguments, most via simple backwards-compatible enhancements like this one (in the original form of this PR). WiFi class is a good example. It doesn't mean though that the problem should be ignored.

With regards to pulling in SDK functions — this is not academic hair splitting. Let me give you an extreme example. In the first version of HTTP Client library, there was code like this:

// parse URL string, get host, port, protocol, URI
if (protocol == "https") {
    client = new WiFiClientSecure(host, port);
} else {
    client = new WiFiClient(host, port);
}

This is an example of bad design. User usually only needs A or B, but the code includes functionality for both A and B at link time. In this specific case, it pulled in lots of SSL-related code, greatly increasing the binary size.

It's going to be less extreme with system_deep_sleep, but still, this is a thing to consider.

@5chufti
Copy link
Contributor

5chufti commented Aug 16, 2018

@igrr

ESP.deepSleepInstant(duration, RF_DEFAULT);

sorry, but here the user has to look up/find the useable definitions that are not even the same as in SDK !
So he will (should) end up here where he will find

ESP.deepSleep(microseconds, mode, max_safe, instant)

and an explanation for the two boolean parameters.

(btw, it should be
WAKE_RF_DEFAULT, WAKE_RF_CAL, WAKE_RF_NOCAL, WAKE_RF_DISABLED
instead of
WAKE_RF_DEFAULT, WAKE_RFCAL, WAKE_NO_RFCAL, WAKE_RF_DISABLED
)

There is no problem defining INSTANT or MAX_SAFE as true, to have it most simple....
So how about #4969? Implement two further variants for max_safe_deepSleep() and max_safe_deepSleepInstant() ?

@torntrousers
Copy link
Contributor Author

Cheers @igrr . Don't suppose you could also pop over here and comment again on that issue?

@igrr
Copy link
Member

igrr commented Aug 16, 2018

Haven't looked into #4969 yet, but on first glance it looks like a bug in system_deep_sleep that it doesn't accept the maximum value.

With regards to #3408, will check, thanks for reminding.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

Could we use another meaningful name like deepSleepCpuOnly (or worse: deepSleepLeavingWiFiInItsState) ?

@igrr
Copy link
Member

igrr commented Aug 16, 2018

WiFi is turned off as well, just not gracefully.

@d-a-v
Copy link
Collaborator

d-a-v commented Aug 16, 2018

OK, I was mistaken by "false friend" instant word between french and english.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

I don't mind the code addition, or the doc entry. However, the deepSleepMax() discussion needs to continue, and the doc entries will likely need to updated accordingly. But that's to happen elsewhere, so approving.

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

Successfully merging this pull request may close these issues.

5 participants