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

[PowerRun] Fix sleep function #28370

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

donlaci
Copy link
Collaborator

@donlaci donlaci commented Sep 5, 2023

Summary of the Pull Request

[PowerRun] Update sleep function: Send message to put PC in Modern Standby mode

PR Checklist

Detailed Description of the Pull Request / Additional comments

Sending message to the system which switches off the monitors, which again initiates the Modern standby mode.
Sending it in a background task to be able to close the PowerRun UI.

Validation Steps Performed

Tested locally

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Hi, Thanks for opening the PR.
What are we actually doing here? This is lots of magic numbers. The code should have extensive comments to explain what's going on and with links to documentation if possible.
Everyone is going to look at this and think "What? Why?"

@github-actions

This comment has been minimized.

@der3318
Copy link
Member

der3318 commented Sep 20, 2023

Some more background to help reviewers understand the context.

PowerRun triggers sleep via SetSuspendState(bHibernate=false), whose behavior highly differentiates between system configurations:

Default OS Sleep State S0 Low Power Idle Traditional S1 S2 S3
Hibernate Available Start Menu's Sleep: Modern Standby
PowerRun's Sleep: Hibernate
Start Menu's Sleep: S1-S3 or Hybrid Sleep
PowerRun's Sleep: S1-S3 or Hybrid Sleep
Hibernate Disabled/Unsupported Start Menu's Sleep: Modern Standby
PowerRun's Sleep: Do Nothing
Start Menu's Sleep: S1-S3 Sleep
PowerRun's Sleep: S1-S3 Sleep

To fix the gap (for modern laptops having S0-idle enabled), the PR author suggests to use a "screen off" message as a replacement, which seems to work.

Here are a list of reported issues:

@@ -91,7 +96,11 @@ internal static List<Result> GetSystemCommands(bool isUefi, bool splitRecycleBin
IcoPath = $"Images\\sleep.{iconTheme}.png",
Action = c =>
{
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => NativeMethods.SetSuspendState(false, true, true));
Copy link
Member

Choose a reason for hiding this comment

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

For laptops that are still using S1-S3 instead of S0-idle, does the new "monitor power off" message put the device into the correct sleep state? Do we need to branch the actions into 2 cases? For example:

NativeMethods.GetPwrCapabilities(out SystemPowerCapabilities powerCapabilities);
if (powerCapabilities.AoAc) // modern standby is enabled
{
    return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => Task.Run(() => _ = NativeMethods.SendMessage(HWNDBROADCAST, WMSYSCOMMAND, SCMONITORPOWER, POWEROFF)));
}
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => NativeMethods.SetSuspendState(false, true, true));

NativeMethods.cs for GetPwrCapabilities

[DllImport("Powrprof.dll", SetLastError = true)]
public static extern bool GetPwrCapabilities(out SystemPowerCapabilities lpSystemPowerCapabilities);

ref:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @der3318,
Thank you for your comment!
Indeed, I could not test the changes on a system with traditional S1-3 power modes. It is also not clear, where the boundaries are (until which Windows version for example) and whether the change makes the sleep function worse on those systems or not.
I will investigate deeper this topic. Your code suggestion seems to be logical, thank you very much!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @der3318
I have tested the code you proposed. Albight it seems logical, it did not work for me, the GetPwrCapabilities function did return false response, AoAc was false on my system, which is faulty. I've experimented with it, but it was always the same.
My conclusion in the current situation is that in this form I cannot extend the code.
I will keep analysing the problem. Just letting you know.
Thanks for the contribution!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the effort. Sadly I am not aware of any issue regarding API GetPwrCapabilities and the field AoAc, which has been publicly used since at least XP.

Just to confirm 😊 - is it possible that you are not marshaling the struct SYSTEM_POWER_CAPABILITIES correctly? As I just noticed that MSFT's documentation regarding this struct is inaccurate. If you take a look at the actual winnt.h (e.g., the one under C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um), it should be:

typedef struct {
...
  BOOLEAN                 ProcessorThrottle;
  BYTE                    ProcessorMinThrottle;
+ #if (NTDDI_VERSION < NTDDI_WINXP)
  BYTE                    ProcessorThrottleScale;
  BYTE                    spare2[4];
+ #else
  BYTE                    ProcessorMaxThrottle;
  BOOLEAN                 FastSystemS4;
  BOOLEAN                 Hiberboot;
  BOOLEAN                 WakeAlarmPresent;
  BOOLEAN                 AoAc;
+ #endif
  BOOLEAN                 DiskSpinDown;
...
} SYSTEM_POWER_CAPABILITIES, *PSYSTEM_POWER_CAPABILITIES;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @der3318,
Yes indeed, there was a glitch in the marshaling. I've rewritten it and it works. I thank you for your colaboration!!

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Gave it a test, but it doesn't seem to be working well for me.
When I choose "Sleep" from the start menu, it goes to sleep and then wakes up after I press a keystroke.
When I choose "Sleep" from PowerToys, it goes to sleep but everytime I press a key it seems to try to wake up but then go right to sleep again right away. To wake it up, I have to keep mashing my keyboard to avoid sending it to sleep again. Doesn't seem like this is good behavior.

@crutkas
Copy link
Member

crutkas commented Oct 27, 2023

@AkazaRenn commented #10557 (comment)

Here seems to be a solution that can be referred to: Open-Shell/Open-Shell-Menu#719

MIT license project too

@jaimecbernardo
Copy link
Collaborator

@Jay-o-Way , the solution here when I tested it put my PC on a sleep loop. It's still a WIP.

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • .github/actions/spell-check/expect.txt: Language not supported
Comments suppressed due to low confidence (1)

src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/Commands.cs:107

  • The new behavior introduced by SendMessage to initiate Modern Standby mode is not covered by tests. Consider adding tests to validate this behavior.
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => Task.Run(() => _ = NativeMethods.SendMessage(HWNDBROADCAST, WMSYSCOMMAND, SCMONITORPOWER, POWEROFF)));

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • .github/actions/spell-check/expect.txt: Language not supported
Comments suppressed due to low confidence (3)

src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/Commands.cs:107

  • Ensure that the SendMessage method is covered by tests to validate the new behavior.
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => Task.Run(() => _ = NativeMethods.SendMessage(HWNDBROADCAST, WMSYSCOMMAND, SCMONITORPOWER, POWEROFF)));

src/modules/launcher/Wox.Plugin/Common/Win32/NativeMethods.cs:115

  • The parameters for SendMessage should be IntPtr or uint to match the Win32 API signature more closely.
public static extern int SendMessage(int hWnd, int hMsg, int wParam, int lParam);

src/modules/launcher/Wox.Plugin/Common/Win32/NativeMethods.cs:700

  • [nitpick] The field 'spare3' in SystemPowerCapabilities struct is private and unused. Consider clarifying its purpose or removing it if not needed.
private readonly byte[] spare3;
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