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 Low-Power demo #6989

Merged
merged 69 commits into from
Feb 2, 2020
Merged

add Low-Power demo #6989

merged 69 commits into from
Feb 2, 2020

Conversation

Tech-TX
Copy link
Contributor

@Tech-TX Tech-TX commented Jan 6, 2020

in response to #6642 and because I felt it needful for others

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.

Overall, very nice. It certainly clears up the different modes, and should allow figuring out which is needed at a glance from this example.

This review is for the non-code parts (comments and README).
A quick glance at the code, without actually going into details, brings up one concern: the loop is being hijacked for busywaits. That isn't good practice on the ESP for various reasons. In addition, I see that there is no timeout for those loops, so in theory they could loop forever if something misbehaves. I think a state machine approach would be better, the resetLoop variable could work for the state value, but I'll get into that in a 2nd review pass once I've looked over the code in detail.


If all you want to do is reduce power for a sketch that doesn't need WiFi, add these SDK 2 functions to your code:
```c
wifi_station_disconnect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where are these functions to be added by the user? setup()? there is also preinit(), which allows doing things much earlier. I believe there is already an example included where wifi is disabled in preinit. Does doing that improve on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see the other example (doing the same thing). I haven't looked at any examples that weren't of immediate interest. I saw several people on forums and elsewhere attempting this and failing, instead. That's why I added it to the README. You can include that chunk anywhere, all it does it turn off the modem.

Copy link
Contributor Author

@Tech-TX Tech-TX Jan 6, 2020

Choose a reason for hiding this comment

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

There's no timeout on the WiFi connection, and none on the pushbutton waits. Those are the only two waits I know of offhand. If it never connects, turn on the debug option and see why the WiFi failed. As far as never pushing the button goes, ummm... how simple ARE kids nowadays? The bottom line on the serial monitor says "press the button to continue" each time it stops.

Half of the tests need an active WiFi connection, so I don't really want to fail into the code with nothing more than a warning on the monitor that all hell has broken loose on WiFi so we're running half-functional. Maybe I should be more explicit that it needs an active WiFi connection to run the tests... I never tried without here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just forced a WiFi fail and continued on into the code: both of the Automatic tests fail as there's no WiFi connection: neither one ever went into low-power mode. If I put in a timeout on WiFi, I'll need to add additional checks in to skip those two tests in the event of WiFi failure. The other tests ran OK. Aren't 99% of people buying the ESP8266 because they want WiFi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OTA is gutted out, and a 30 second timeout on WiFi added. If it fails connection it skips the two 'Automatic' tests as they won't do anything. They won't fail or anything, but they won't work without an active connection. CRC32 integration is the last step I'm working on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, hopefully it passes the style check THIS time around. 😝


### Test 1 - Unconfigured modem

This is typical for programs that don't use WiFi, and is a high current drain of at least 67 mA continuous. Even if you don't plan to use WiFi, it's wise to use the SDK 2 functions below in **Lower Power without the WiFi library** to reduce the power.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to use SDK2 functions? can the WiFi library be used instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole point of that comment starts with "if you don't plan to use WiFi", so why would they want to include the WiFi library if they only want to reduce power? I saw one issue here, two posts on arduino.cc, and one at the Espressif BBS with that exact situation: someone wanting to reduce power without loading the WiFi lib. None of them were answered or resolved. To make it more clear, I'll remove the beginning "Even" in that sentence. The example in the code does use the library call. The note in the README was for the other folks that are only looking to reduce power.


All of the Deep Sleep modes end with a RESET, so you must re-initialize everything. You can store *some* information in the RTC memory to survive a Deep Sleep reset, which I've done in this demo to illustrate it. See the **RTCUserMemory** example for more on this feature.

The maximum Deep Sleep interval is 71.58 minutes (2^32 -1 microseconds).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be correct if the sleep timer were 32bits, but it's not. There is a function in the ESPClass class that calculates the max sleep time.
HOWEVER, and this is a very big thing: the calculation requires taking into account the rtc period (cali_proc), which apparently changes from time to time. There is a suspicion of a race condition, where if you calculate the max sleep time, and then the period changes before actually entering deep sleep with that max calculated value, and the max sleep allowed is less than the value calculated, then the ESP could never wake up. This is not confirmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line is quoted verbatim from the Espressif info. I can copy-pasta it for proof. That's the maximum possible, but the actual maximum may be less than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a minor revision to the README to make it more clear.

delay(50); // debounce time for the switch, button released
}

uint32_t calculateCRC32(const uint8_t *data, size_t length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see we already have this crc32 function in crc32.cpp, the declaration is in cores/esp8266/coredecls.h. Please reuse that one to not reinvent the wheel.

Copy link
Contributor Author

@Tech-TX Tech-TX Jan 7, 2020

Choose a reason for hiding this comment

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

It may take me a week to figure that one out, as it's not exactly clear how to use it for a newbie, nor is it listed at readthedocs. The only example I could find that used it is equally unclear (WiFiShutdown.ino). At the moment I'm trying to figure out what this is doing, quite literally symbol-by-symbol.
nv->crc = crc32(&nv->data, sizeof(nv->data));
From what I've figured out so far, that's called "function call by reference" but giving it a name doesn't magically make me understand it. I suspect that's why the person that wrote the RTCUserMemory example coded their own version of a crc32 (that's what I copied).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, replaced with the one linked from coredecls.h. That was less painful than I thought.

});
ArduinoOTA.onError([](ota_error_t error) {
Serial.printf("Error[%u]: ", error);
if (error == OTA_AUTH_ERROR) Serial.println(F("Auth Failed"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

if-else bodies on their own lines, please, like so:

if (this)
  doThat();
else if(this2)
  doThat2();
else
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already fixed, just not committed yet. I saw the error at Travis.

@Tech-TX
Copy link
Contributor Author

Tech-TX commented Jan 10, 2020

At the moment, everything fixed except the CRC32 replacement. That's next.

@Tech-TX Tech-TX changed the title add Low-Power demo WIP: add Low-Power demo Jan 11, 2020
@Tech-TX
Copy link
Contributor Author

Tech-TX commented Jan 11, 2020

Found a WDT bug in the code with one board, need to insure that it's thoroughly eliminated.

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 31, 2020

You can use the tests/restyle.sh script to fix your CI issue.

@Tech-TX
Copy link
Contributor Author

Tech-TX commented Feb 1, 2020

Corrected a few mistakes and made some of the README a little more clear.

@Tech-TX Tech-TX changed the title WIP: add Low-Power demo add Low-Power demo Feb 1, 2020
@Tech-TX Tech-TX changed the title add Low-Power demo WIP: add Low-Power demo Feb 1, 2020
@Tech-TX Tech-TX changed the title WIP: add Low-Power demo add Low-Power demo Feb 1, 2020
@Tech-TX
Copy link
Contributor Author

Tech-TX commented Feb 1, 2020

changed the WiFi timeouts to using polledTimeout (2 places)

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.

Much cleaner. There are still some details about the use of globals, but that can be addressed later. The current state is ok for merging.
Good job clearing this up! We now have a reference for low power modes.

@leonstill
Copy link

Hello Tech-TX, thanks for your example which is helpful for me. Now I have a puzzle about line 133.
crcOfData = nv->rtcData.crc32
Is this a clerical error or my misunderstanding, i think the "=" should be "==" .

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

Successfully merging this pull request may close these issues.

None yet

4 participants