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

Do not wake up computer from sleep #67

Closed
wants to merge 1 commit into from

Conversation

mauve
Copy link
Contributor

@mauve mauve commented Sep 23, 2015

Supplying a REASON_CONTEXT to SetWaitableTimerEx() will wakeup
the computer from sleep. Supplying a nullptr instead disables wake.

Fixes: https://svn.boost.org/trac/boost/ticket/11368

@viboes
Copy link
Collaborator

viboes commented Sep 23, 2015

Thanks for the patch. I'm not a Windows expert. We will merge it as soon as Niall has time to check it.

@ned14
Copy link
Member

ned14 commented Sep 23, 2015

This is not a documented valid value for the WakeContext parameter. I would be happy to sign off on this anyway, however neither Wine nor ReactOS fully implement SetWaitableTimerEx() so I cannot tell what the valid values are from them either. The NT kernel docs are similarly not helpful: https://msdn.microsoft.com/en-us/library/windows/hardware/ff553249(v=vs.85).aspx.

Vicente you could try merging this patch and see what happens? As I mentioned, I never saw a problem on my machine with the current code anyway, so it is hard for me to say if this patch is good or bad.

@MarcelRaad
Copy link
Contributor

We're successfully using principally the same patch. But the nullptr should probably be changed to 0 or NULL in order to support old compilers?

@mauve
Copy link
Contributor Author

mauve commented Sep 23, 2015

we looked at the disassembled source for SetWaitableTimerEx, as it turns out, setting wakecontext to null actually calls SetWaitableTimer :D

@mauve
Copy link
Contributor Author

mauve commented Sep 23, 2015

@MarcelRaad should I switch to NULL instead?

the rest of the file used NULL so I switched and force-pushed

Supplying a REASON_CONTEXT to SetWaitableTimerEx() will wakeup
the computer from sleep. Supplying a nullptr instead disables wake.

Fixes: https://svn.boost.org/trac/boost/ticket/11368
@ned14
Copy link
Member

ned14 commented Sep 23, 2015

we looked at the disassembled source for SetWaitableTimerEx, as it turns out, setting wakecontext to null actually calls SetWaitableTimer :D

This would suggest that we revert my patch to its previous edition.

It might be worth asking on stackoverflow how to correctly call SetWaitableTimerEx.

Niall

@ned14
Copy link
Member

ned14 commented Sep 23, 2015

@ned14
Copy link
Member

ned14 commented Sep 25, 2015

I'm thinking that one could pass a NULL WakeContext when wait time exceeds some amount? That would disable PC wake, but keep timer coalescing for shorter values. What's a reasonable amount? Ten seconds?

@mauve
Copy link
Contributor Author

mauve commented Sep 25, 2015

@ned14: I was thinking about that as well. But on the other hand are coalescing timers really useful without wake?

@slikts
Copy link

slikts commented Sep 27, 2015

I'll just leave this here.

@viboes
Copy link
Collaborator

viboes commented Sep 27, 2015

I can not take this PR as it is for master. I'll propagate this PR to develop and see how it behaves.

Niall, please, come back to this issue once you have more time.

@viboes
Copy link
Collaborator

viboes commented Sep 27, 2015

Patch applied to develop branch

9ac736a

@viboes viboes closed this Sep 27, 2015
@mauve
Copy link
Contributor Author

mauve commented Sep 29, 2015

@ned14: do you want me to try and revert the following commit or should I just prepare a commit which removes the parts about SetWaitableTimerEx and switch back to SetWaitableTimer? Personally I am unsure if coalescing timers are useful without wakeup (no need to coalesce if already running the CPU at full speed).

commit 401f69f108271fe2531832a3d85921f019aca421
Author: Niall Douglas (s [underscore] sourceforge {at} nedprod [dot] com) <spam@nowhere>
Date:   Wed Feb 4 13:58:11 2015 +0000

    Added coalesced timer support to Windows where that API is available. Tolerable delay is set to the maximum of 5% of interval or 32 ms.

@viboes
Copy link
Collaborator

viboes commented Sep 29, 2015

Hi Mauve,

I will be very grateful if you go ahead and provide a PR to revert this commit.

Vicente

@mauve
Copy link
Contributor Author

mauve commented Sep 29, 2015

@viboes: Do you mind if I wait with the revert for this PR and do that at the same time I prepare a "proper" fix according to what @ned14 wants me to do? Despite not being beautiful this PR fixes a serious issue.

@ned14
Copy link
Member

ned14 commented Sep 29, 2015

Stackoverflow says the current patch is the right one https://stackoverflow.com/questions/32749829/how-to-call-setwaitabletimerex-correctly

@mauve
Copy link
Contributor Author

mauve commented Sep 29, 2015

@ned14: Perfect thanks, I will prepare a patch to cleanup the remaining unused wakecontext code.

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

5 participants