Add support for logind hybrid sleep when closing lid
Submitted by Nico Kreipke
Assigned to Ali Abdallah @ali
Description
Created attachment 5780 Patch
I made changes to xfce4-power-manager to support the hybrid sleep feature of newer Linux kernels, currently located here: https://github.com/nkreipke/xfce4-power-manager
It works well on my system, maybe it is interesting enough to include in the official version.
Patch 5780, "Patch":
0001-Added-support-for-logind-hybrid-sleep-when-closing-l.patch
Version: git
Activity
Hey Nico,
Thanks for your patch. I've rebased it against current Git.
I have some questions for you:
-
in xfpm-xfconf.c, why is it that suspend/hibernate are not yet installed but you install HYBRID_SLEEP? Could it be that there is a better time to install this value to the property of lid-related actions later? How are suspend and hibernate added right now?
-
in xfpm_settings_on_ac, you originally did not expose hybrid sleep. Was that an omission or was there a reason I'm missing? My patch adds it.
-
if I select hybrid sleep on the "System" tab, "System sleep mode" line, I get:
(xfce4-power-manager:6940): GLib-GObject-WARNING **: value "5" of type 'guint' is invalid or out of range for property 'inactivity-sleep-mode-on-battery' of type 'guint'
(xfce4-power-manager:6940): GLib-GObject-WARNING **: value "5" of type 'guint' is invalid or out of range for property 'inactivity-sleep-mode-on-ac' of type 'guint'
Thanks.
Patch 5941, "Rebased patch":
0001-Support-hybrid-sleep-for-systemd-users-experimental.patch-
-
Hi Steve, thanks for looking into this. I'll try to answer your questions:
-
g_param_spec_uint expects a minimum and maximum value. I had to raise its previous maximum of LID_TRIGGER_LOCK_SCREEN to LID_TRIGGER_HYBRID_SLEEP as setting it to this value would otherwise have failed. Values for suspend and hibernate do not need to be explicitly installed.
-
I think you missed my change in this one and exposed the value at the wrong position. My change was intended for the lid switch settings and can be found in line 1162.
-
The value for hybrid sleep is only valid for the lid switch settings. This error must be caused by the same problem as in question 2.
Yours, Nico
-
-
Hey,
I did swap around some values because I didn't understand why you used a constant from a different enum than for other actions (i.e. you did not use XFPM_DO_HYBRID_SLEEP). I wasn't sure given the patch's age and possible changes in the coding style of xfpm what the difference was due to.
I rebuilt and the patch applies without errors and does not cause runtime errors so I might have made a mistake before.
Your patch was meant to apply as a setting for when the computer has been inactive, whether on ac or battery, wasn't it? There were only two exposure points in the settings? I quite like having the option but we should really propose it everywhere in the settings UI if we're going to propose it.
I'm leaving this as is for now, quite busy elsewhere especially as the patch can't make it for 4.12 now :-( If there's any chance that you add menu items for the other menus, it'd be very kind of you :-)
To anyone reviewing the patch: please ask yourselves whether xfpm-enum-glib.h is ok. There might be need to add a XFPM_TRIGGER_SHUTDOWN for consistency or to add an explicit comment explaining if/why the values between both enums should be synchronised (also why are there two enums at all!?).
-
Okay, the reason why I had to synchronize the values between the two enums in xfpm-enum-glib.h is that in xfpm-manager.c, line 382, there is a value of type XfpmLidTriggerAction passed to xfpm_manager_sleep_request which expects an XfpmShutdownRequest. This previously just "happend to not fail" because the enum members were mapped to the same integer value. As the enums are also used to store the desired action in the user settings, my way was the simplest solution that did not break backwards compatibility. I did not understand either why this is the case and why we need multiple enums which apparently do the same thing, but differently, but this must be a pain to maintain.
I originally intended hybrid sleep to only be available when the laptop lid is closed because it did not make sense in other situations, which admittedly was not well thought out.
I'll try to do something about xfpm-enum-glib.h first before I update the patch.
-
Thanks for your reply Nico. I do hope you're not put off from improving the patch and pushing it into Xfce, as the feature itself would be awesome to have. I just think it's better if we are all convinced the newly added code is robust :)
(ps: feel free to assign the bug to yourself so we know you're working on it)
-
This bug is duplicate of https://bugzilla.xfce.org/show_bug.cgi?id=13076
Attachment 8423, "Patch to allow Hybrid Sleep in Powermanager on power button etc":
xfce4powermanagerhybridsleep.patch.2019-04-19 -
patch made with diff -u
Attachment 8426, "diff -u Patch to allow Hybrid Sleep in Powermanager on power button etc":
xfce4powermanagerhybridsleep.patch.2019-04-19-u -
While nobody's asked to support it, I've noticed systemd-sleep does support a fourth sleep mode beyond suspend, hibernate and hybrid-sleep:
suspend-then-hibernate - A low power state where the system is initially suspended (the state is stored in RAM). If not interrupted within the delay specified by HibernateDelaySec=, the system will be woken using an RTC alarm and hibernated (the state is then stored on disk).
I can only guess if nobody's asked for this because they don't need it or don't know that there is such an option.
I guess Xfce4 could support this, not that I see a need. Supporting hybrid-sleep first is probably a good idea regardless.
-
@ochosiclosed a related bug:*** Bug 13076 has been marked as a duplicate of this bug. ***
-
Patch 9409, "Hybrid Suspend patch":
xfce-power-manager-patch-2020-01-29.patch -
Patch 9411, "git format-patch":
0001-Hybrid-Sleep-works-on-Linux-not-BSD-just-yet.patch -
The BSD case is one I did not at all solve. They will, with that patch, get an option called Hybrid Suspend which results in Hibernate. Perhaps that part should be handled differently.
#ifdef BACKEND_TYPE_FREEBSD #define UP_BACKEND_SUSPEND_COMMAND "/usr/sbin/acpiconf -s 3" +#define UP_BACKEND_HYBRID_SLEEP_COMMAND "/usr/sbin/acpiconf -s 4" #define UP_BACKEND_HIBERNATE_COMMAND "/usr/sbin/acpiconf -s 4"
#ifdef BACKEND_TYPE_OPENBSD #define UP_BACKEND_SUSPEND_COMMAND "/usr/sbin/zzz" +#define UP_BACKEND_HYBRID_SLEEP_COMMAND "/usr/sbin/ZZZ" #define UP_BACKEND_HIBERNATE_COMMAND "/usr/sbin/ZZZ"
It looks like there is no hybrid sleep method for FreeBSD, https://www.dragonflybsd.org/cgi/web-man?command=acpiconf and I did not find any such method for OpenBSD. I was assuming that it was possible and that someone from freeBSD and OpenBSD would tell me how to do those things so I used the same commands as Hibernate but it appears that there is no such support at all right now.
Do note that I have not actually tested the patch on BSD or BSD at all, ever. I should probably try a BSD sometime.
-
@alexxconssaid:Finally found some time to check the code ... besides the BSD case, it looks very good to me !
In xfpm_suspend_can_hybrid_sleep (xfpm-suspend.c:161) :
#ifdef BACKEND_TYPE_FREEBSD return freebsd_supports_sleep_state ("S3");
#endif #ifdef BACKEND_TYPE_OPENBSD return TRUE;
#endif So it looks like there can be some kind of hybrid_sleep support on FREEBSD ... or is S3 just "hibernate" ? If BSD's dont support hybrid_sleep, I would expect to get FALSE from this method when using a BSD. (As you said above .. probably better to dont provide the possibility to pick hybrid-sleep at all on BSDs )
In xfpm-systemd.c:78, copypasted the string from hibernate: "org.freedesktop.login1.hibernate". Forgot to replace, with hybrid-sleep, or was it intentional ?
If we would not show "hybrid-sleep" as an enum value on BSD's, it would be possible to test the BSD usecase without even running a BSD ... just directly return FALSE on "xfpm_suspend_can_hybrid_sleep", and you theoretically should see the same behavior like on a BSD.
Some very minor thing: When apllying the patch, git found a trailing whitespace .git/rebase-apply/patch:371: trailing whitespace.
I asked gaston in IRC if he can take a look regarding BSDs .. possibly he knows details.
-
The reason I went with "org.freedesktop.login1.hibernate" in xfpm-systemd.c:78 is really stupid yet I did so for a (stupid) reason:
There is no polkit hybrid sleep method, none is listed on https://www.freedesktop.org/wiki/Software/systemd/logind/ and there is none listed in /usr/share/polkit-1/actions/org.freedesktop.login1.policy
What we do have is a systemd-logind method CanHybridSleep
Simon Steinbeiss fixed the very same problem in xfce4-session by switching from polkit to systemd-logind, https://git.xfce.org/xfce/xfce4-session/commit/xfce4-session/xfsm-systemd.c?id=e10ac91be3301c92900168855b265101ba9884e2
The same switch from polkit to systemd-logind should ideally/eventually be done in xfpm-systemd.c too. However, that is not what I tried to accomplish with this patch and my thinking was that it would be simplest to leave it using polkit and add the Hybrid Sleep functionality - and perhaps do a separate patch for polkit -> systemd-logind later. I am not very familiar with contributing to free software projects so I do not know if that is the right way of doing things, I just assumed that a patch touching all kinds of code which is unrelated to hybrid sleep would be a confuse to look at.
