Skip to content

Conversation

dnc40085
Copy link
Contributor

@dnc40085 dnc40085 commented Aug 5, 2017

  • 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.

This PR comments out ENABLE_TIMER_SUSPEND and PMSLEEP_ENABLE in user_config.h.
These features currently have some issues that will take some time to resolve and to prevent further delay of the master drop, the features have been disabled in user_config.h

@TerryE
Copy link
Collaborator

TerryE commented Aug 5, 2017

Oops, it looks as is we've missed some conditionals around calling the swTimer routines in tmr.c

@dnc40085
Copy link
Contributor Author

dnc40085 commented Aug 5, 2017

@TerryE that's weird, it compiled fine locally for both float and integer versions

@marcelstoer marcelstoer added this to the 2.1 follow-up milestone Aug 5, 2017
@TerryE TerryE merged commit fee5608 into nodemcu:dev Aug 5, 2017
@marcelstoer
Copy link
Member

Doesn't this effectively render the improvements you made in 0844a5d moot and break backwards compatibility?

@TerryE
Copy link
Collaborator

TerryE commented Aug 8, 2017

What this does is to make the pmSleep functionality disabled by default. Any developer that wants it only needs to uncomment the two #define statements. This a quick change that can be incorporated in the dev->master drop that you deferred.

The last dev->master drop slipped in some fundamental changes to how our timer work. This was a mistake, IMO; if making these changes configurable but at least not enabled by default to correct this mistake is a break in backwards compatibility, then it's the sort of break that we should still be doing.

@marcelstoer
Copy link
Member

I wasn't trying to suggest this be changed somehow but trying to verify my assessment of the impact. To conclude:

  • We're all good for the master drop.
  • It really seems pointless for a function to print "this was removed" when the function isn't even compiled in. Not that this couldn't be solved if desired...

@marcelstoer
Copy link
Member

In hind sight, why didn't it occur to us that this change should be reflected in the tmr documentation? scratching my head...

@melstav
Copy link

melstav commented Sep 14, 2017

As someone who recently wasted a not insignificant amount of time trying to figure out why calls to node.sleep kept erroring out, I have to say that I concur with the previous comment.

marcelstoer added a commit to marcelstoer/nodemcu-firmware that referenced this pull request Sep 15, 2017
marcelstoer added a commit that referenced this pull request Oct 19, 2017
crasu pushed a commit to crasu/nodemcu-firmware that referenced this pull request Jan 11, 2018
@dnc40085 dnc40085 deleted the dev_disable_pmsleep_and_timer_suspend branch March 6, 2018 01:16
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.

4 participants