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

Refactor timer suspend portion of node.sleep (pmsleep) #2056

Closed
wants to merge 51 commits into from

Conversation

dnc40085
Copy link
Contributor

@dnc40085 dnc40085 commented Aug 2, 2017

Fixes #2048.

  • This PR is for the dev branch rather than for master.
  • 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/en/*.

The previous version of the timer suspend code was large, difficult to read and had some problems.
This PR changes the way the timer suspend uses memory, rather than storing the necessary data in memory blocks allocated with c_malloc(), the new implementation stores its data in the Lua registry and in doing so a lot of extra code was able to be removed.

!!!IMPORTANT!!! Some functions were removed in this PR!
No deprecation notices have been included for the following functions
A message has been left in the following functions notifying the developer that the feature has been removed.

  • tmr.suspend()
  • tmr.suspend_all()
  • tmr.resume()
  • tmr.resume_all()

@marcelstoer
Copy link
Member

Some functions were removed in this PR! No deprecation notices have been included...

Ouch! I can't gauge how big of an impact this has on our users. I guess those functions weren't the most popular ones, though.

Would it make much of a difference if you left the functions in place for now but replaced their body with a "Sorry, this was removed" message?

@dnc40085
Copy link
Contributor Author

dnc40085 commented Aug 2, 2017

@marcelstoer No problem, I'll submit a PR shortly.

@marcelstoer
Copy link
Member

You could just push to your existing branch and this PR will update.

@dnc40085
Copy link
Contributor Author

dnc40085 commented Aug 2, 2017

My mistake, I meant push a commit.

@TerryE
Copy link
Collaborator

TerryE commented Aug 3, 2017

@dnc40085 Hi dnc, can we have a discussion about the requirement and implementation here?

Surely the underlying requirement is that we can add the node.sleep() and wifi.suspend() functionality for low-power usecases where the application will be putting the CPU into modem-sleep or light-sleep to save power(15mA and 0.4mA chip consumption respectively)?

These functions are really only need if the ESP application is battery powered and needs to use the WiFi intermittently. IMO, other usecases form the bulk of ESP applications, so my recommendation this implementation must not impact the runtime performance or RAM footprint of these majority usecases. Therefore, such functionality should not be enabled by default.

It should also have as small an impact on the runtime performance or RAM footprint of applications which employ light-sleep.

As I understand this, the SDK PWM and S/W timer functions use the FRC1 and FRC2 H/W timers for sequencing. These don't work with light-sleep as the CPU and its internal clock are halted and hence the H/W timer NMIs won't get delivered. The 2.1 SDK API guide only mentions restrictions "timers" in Sec 3.7 which could include FRC2 use.

The S/W timer code is currently pretty lightwieght. The caller is responsible for allocating timer structures and the API marshals these in its linked timer list. No malloc/free calls are made.

The S/W timer is used by both SDK internal and application functionality. I assume that we only need to have a treatment for the application ones, and what we it seems that we need to do is to stop any application timers before reqesting a sleep and then resuming them with a correction offset (whatever that should be) on awaking. This is a one per sleep / wake cycle overhead and not a once per every timer call one.

As I see it, the issue -- if we want to do this at once per sleep -- is that when we walk the timer linked list we need a method of descriminating which list entries are application ones. We've got some 35 xx_timer_setfn() call in 22 C files; some 30 unique C cb addresses in our firmware, of which only a dozen might be used in a typical build. Surely it is up the to the individual module to determine how to handle the impact of light sleep? For example:

  • let the time slip right by the sleep time
  • adjust time and drop any past timers
  • adjust time and deliver timer immediately for past timers.

Why not use a macro to allow modules to declare a policy for each cb address? This could even use an inline static flag byte so this only calls the sleep hook once per boot, instead of once per register / unregister. OK this isn't transparent at a coding level, but I personally would rather have 30 extra source lines to add and no other runtime RAM/CPU hit than a chunky piece of hidden code being called with malloc / frees on every timer register / unregister.

@marcelstoer
Copy link
Member

@dnc40085 Hi dnc, can we have a discussion about the requirement and implementation here?

Did this happen offline or has this stalled?

@TerryE
Copy link
Collaborator

TerryE commented Sep 28, 2017

It hasn't happened. Sorry to be terse, but getting ready for my son's wedding tomorrow 😀

TerryE and others added 22 commits October 29, 2017 18:19
Make it build without INTERRUPT_ENABLE
* Make the rtc variables not be cleared by the .bss initialization
* Move the save to the right place
* Make sure that we reset the rtctime to 0 if we didn't sleep properly.
* Setting the seconds to zero doesn't update the dsleep calibration
* Initial checkin
* Add bloom.md into mkdocs
* Added reset and improved info
* Update bloom.c
* Update bloom.md
* Add Wikipedia link
* Add support of counting of interrupts

* Update the timestamp when interrupt happens during dispatch. Also
clear out interrupts when setting up a new callback
Buffer size of funtion readline is wrong  (Line 480)
Should be FILE_READ_CHUNK = 1024 bytes instead 256 bytes
…u#2217)

* Only load a single non-empty line from the uart at a time.

* Don't fall behind in processing of uart buffer.
* mbedTLS update
* mbedtls: vsnprintf macroification
* Further update mbedTLS to 2.6.1
* mbedtls: make debugging work again
* Silence SSL messages on normal teardown
* Drop DTLS support from mbedtls
paweljasinski and others added 16 commits January 29, 2018 22:21
* ads1015 is supported, up to 4 devices can be connected at the same time

* removed debug, updated documentation

* changed to oop API

* added __gc to handle active timer cleanup

* reworked argument validation and error reporting

* stack is no longer messed up after __del
in float build, uV and sign are included in mV
in int build, uV and mV are absolute, sign is -1, 0, 1
added rounding of uV values
added optional test function
Fix to start sending PINGREQ after a reconnect.
All the changes are in preparation for an eventual MkDocs 0.17 upgrade
…mcu#2262)

Disable the error callback of mqtt.client:connect() after a successful connection.
This will prevent this function to be called after a future disconnection.
Instead the "offline" function is called.
…cu#2260)

* Fix some subtle timing issues with gpio.pulse
* Add the pulse:update method
* Allow getstate to work on stopped pulsers
* Make gpio.mode(, gpio.OUTPUT) actually set the output mode
* Added some more documentation
@marcelstoer
Copy link
Member

@dnc40085 can we still count on your support for this project? Your GitHub history went blank after #2079 last August...?

@TerryE
Copy link
Collaborator

TerryE commented Mar 2, 2018

If @dnc40085 has gone onto greater things, then I guess that I'll have to add this to my long butt list because I don't think that we can leave it as it is.

@dnc40085
Copy link
Contributor Author

dnc40085 commented Mar 3, 2018

Hello, my apologies for being gone for so long, life got in the way.

I have a new commit that I'm putting some finishing touches on, I've implemented the changes suggested by @TerryE above.

I'll try to get it finished and push the new commits this weekend.

@dnc40085 dnc40085 force-pushed the dev_pmsleep_refactor branch 3 times, most recently from 1fd9c56 to 46f0658 Compare March 3, 2018 06:27
@dnc40085
Copy link
Contributor Author

dnc40085 commented Mar 3, 2018

I think I'm gonna need to create a new PR, I messed up somewhere

@dnc40085 dnc40085 closed this Mar 3, 2018
@dnc40085 dnc40085 deleted the dev_pmsleep_refactor branch March 3, 2018 06:33
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.

None yet