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) #2287

Merged
merged 5 commits into from
Apr 13, 2018

Conversation

dnc40085
Copy link
Contributor

@dnc40085 dnc40085 commented Mar 3, 2018

Fixes #2048.

  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • The code changes are reflected in the documentation at docs/en/*.
  • I have thoroughly tested my contribution.
  • This PR is for the dev branch rather than for master.

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.

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()

Due to my not fully understanding the function of each callback registered with os_timer_setfn, I have not been able to fully test the suspend behaviour for each instance of SWTIMER_REG_CB(cb_ptr, suspend_behaviour), It would be much appreciated if someone could go over each instance of SWTIMER_REG_CB()to verify that the configured suspend behaviour is correct.

This PR is a resubmit of #2056 and includes the changes suggested by TerryE

@marcelstoer
Copy link
Member

Terry, in light of the discussion in #2056 I took the liberty to assign you as a reviewer, hope you don't mind.

@marcelstoer marcelstoer requested a review from TerryE March 3, 2018 10:58
@TerryE
Copy link
Collaborator

TerryE commented Mar 3, 2018

@marcel, thanks . I am up to my arm-pits in some weird GC interaction on my LFS testing that is proving really hard to track down. I'll post separately on that, but one I've cleared this, I will run this one, and reply to you guys.

Added swtimer debug module option to user_modules.h.
Added comments to user_config.h.
@dnc40085 dnc40085 force-pushed the dev_pmsleep_refactor_new branch from 2dc0ee4 to 040c887 Compare March 24, 2018 10:37
@TerryE
Copy link
Collaborator

TerryE commented Mar 30, 2018

@dnc40085, we are still at cross purposes, here I think. As far as I can see, the aim of this patch is to offer those developers that wish to make use of the the light sleep functions, the ability for any timers that have need defined to time shift sensibly across the sleep period, and to do this in a runtime-efficient manner.

So the % of active firmware users who will want to use light sleep is very small. And some of these might want to handle timers across light sleep themselves. However for those that don't, it makes sense to make this as transparent as possible with minimum runtime impact.

Your move to using a registration process which uses a one-time flag so that registration is a single startup event is great as is moving this content into the registry. So now there is only an initialisation overhead, plus a sleep / wake overhead and no overhead on a per timer arm / fire.

But in another way this patch also is a major step back. Specifically

  • This code should only be only used if TIMER_SUSPEND_ENABLE is set
  • Yet the swTimer code is unconditionally included in all builds as are the Registry swtmr tables

So now whether or not TIMER_SUSPEND_ENABLE is set, every firmware build have an extra RAM and Flash overhead . Not acceptable.

Can I suggest that the entire callable interface to swtmr_* is encapsulated in include/pm/swtimer.h with two variants depending on whether TIMER_SUSPEND_ENABLE is set or not. In the second case, then all SWTMR_* macros should generate no C code or (void)0 declarations so that the S/W timer code and RAM tables are fully optimised out of the build.

@dnc40085
Copy link
Contributor Author

...whether or not TIMER_SUSPEND_ENABLE is set, every firmware build have an extra RAM and Flash overhead . Not acceptable.

My mistake, I felt like I was forgetting something when I re-submitted the PR.

@TerryE
Copy link
Collaborator

TerryE commented Mar 30, 2018

@dnc40085, have you done a build with TIMER_SUSPEND_ENABLE not defined, and checked the app/mapfile to ensure that none of the swtimer components are being linked in?

Having had a quick scan through the source, I think that you've covered off all of the cases, but it's always good to do the "belt and braces" check.

@dnc40085
Copy link
Contributor Author

Have you done a build with TIMER_SUSPEND_ENABLE not defined, and checked the app/mapfile to ensure that none of the swtimer components are being linked in?

I double checked and I found no occurrences of any of the swtimer components.

@TerryE
Copy link
Collaborator

TerryE commented Mar 30, 2018

OK, I will merge this after the dev to master cut-over. The functional changes are too great so move this into master without at leat a few weeks testing in dev.

@marcelstoer marcelstoer added this to the 2.2-follow-up milestone Apr 2, 2018
@marcelstoer marcelstoer merged commit 96e5c02 into nodemcu:dev Apr 13, 2018
@dnc40085 dnc40085 deleted the dev_pmsleep_refactor_new branch May 10, 2018 09:07
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.

3 participants