Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Suspend via logind over UPower if available. #4273

Merged
merged 1 commit into from

8 participants

@yasij

From pull request #4168:
The suspend feature of UPower is deprecated. XBMC supports suspending using systemd-logind, however it defaults to a few other UPower methods first before trying Logind. This patch checks for logind first, and uses it if available.

I've updated the HasLogind() method to make sure systemd is at least version 183, or if the systemd version API is missing, that Upstart is at least version 1.11. This way, XBMC will default to suspending via logind if the version supports suspend. If it does not, it will skip logind and try UPower as before.

@yasij

@topfs2 @t-nelson Here is a version of the patch that checks for systemd or upstart version.

@topfs2
Collaborator
@topfs2
Collaborator
@yasij

Ah, I haven't used github very much. I didn't know there was a force flag. I realized what was going on with KDE's upstart version check, and I'm fixing the patch. (version 1.11 > version 1.2 but 1.11 < 1.2 numerically.

@yasij

@topfs2 Okay, I've fixed the Upstart version check in the branch.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone
@mus65

please DO NOT merge this. Checking the version number with regexes is hackish and the suspend functionality in KDE recently broke because they did the same thing. See this recent discussion on the systemd-devel mailing list:

http://lists.freedesktop.org/archives/systemd-devel/2014-February/017399.html

One of the systemd devs explains how to properly implement checks like this in the following mail:

http://lists.freedesktop.org/archives/systemd-devel/2014-February/017452.html

@mus65

Looking at the code (I am the one who rewrote most of the logind stuff last year), I guess the simplest way to do this is, is to call one of the CanXYZ()-Methods (doesn't really matter which I guess, afaik they were all added in the same systemd version, except for HybridSleep maybe) and check whether it returns org.freedesktop.DBus.Error.UnknownMethod. If so, you could assume that the logind version is too old.
Currently the CanXYZ() calls are all done in LogindCheckCapability() which doesn't distinguish between "machine doesn't support XYZ" and "logind method doesn't exist".

Maybe there is an even more elegant way to this, I'm no DBus expert. But you really, really should NOT check for the version number.

@yasij

Why would they change the Version API like that? Yes, it is generally poor practice to check versions, but people do it, and changing APIs in a stable software like that is also poor practice. I'll update it to call the CanSuspend() and check for a DBus error message.

@yasij

@topfs2, @mus65 I've updated it to call CanSuspend and check for a DBus error. If there is no error, it returns true.

@t-nelson

@topfs2 for review please

@wsnipex
Collaborator

gave this a try on ubuntu 14.04 cause I was hoping it'd fix hibernate.
Sadly it doesn't work:
process 16022: Applications must not close shared connections - see dbus_connection_close() docs. This is a bug in the application.
process 16022: arguments to dbus_message_unref() were incorrect, assertion "!message->in_cache" failed in file ../../dbus/dbus-message.c line 1618.
This is normally a bug in some application using the D-Bus library.

Result: hibernate is disabled.

@yasij

Odd that it worked before. I dropped the offending dbus_connection_close line. Try it now.

@wsnipex
Collaborator

still produces this error:
process 20172: arguments to dbus_message_unref() were incorrect, assertion "!message->in_cache" failed in file ../../dbus/dbus-message.c line 1618.
This is normally a bug in some application using the D-Bus library.

@yasij
@fritsch
Collaborator

@wsnipex ping

We need to solve those issues as with 15.04 they use logind, but still ship upower and stuff .. bloody mess.

@wsnipex
Collaborator

the change looks good to me, did anyone test it on 15.04 or another logind based distro?

@fritsch
Collaborator
@wsnipex
Collaborator

jenkins build this please

@yasij

I just merged this with tip and tried it with Kubuntu 15.04. It's working. Without it, Kodi doesn't suspend as the UPower CanSuspend calls fail. It looks like Ubuntu stripped that out of UPower.

@fritsch
Collaborator

Yes, that's the idea behind this approach.

@fritsch
Collaborator

@MartijnKaijser @FernetMenta this is needed to make hybrid distros happy, e.g. upower available, but stripped down.

@yasij
@fritsch
Collaborator

Not nice - It seems even upstream does not know at all howto do it properly. And we cannot really workaround every mad thing distributions implemented on their way to systemd.

@Paxxi
Collaborator

What about brute force instead? Load both and send events to all loaded providers?

@yasij
@Paxxi
Collaborator

what's the status on this? Is it in good shape?

@yasij

I haven't had time to look at it until just now. I think the best way to handle this is in xbmc/powermanagement/PowerManager.cpp. I'm going to drop the two patches from this branch, and instead check capabilities of each type. If UPower doesn't support suspend, try the next class until we find one that does.

@yasij

Here's a patch that counts the number of available power features on Linux and selects the manager that supports the most. I've only tested this on Kubuntu 15.04 with logind. It successfully tries ConsoleKit + UPower and ConsoleKit + DeviceKit before settling on logind because it supports all of suspend, powerdown, hibernate and reboot.

@wsnipex
Collaborator

looks like a nice solution to me. @topfs2 ping

@wsnipex
Collaborator

I tested this on ubuntu 14.04 and it works fine for me.
Unless someone speaks up, I'll merge it tomorrow.

@wsnipex wsnipex merged commit 33501b9 into xbmc:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
23 xbmc/powermanagement/IPowerSyscall.cpp
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2015 Team XBMC
+ * http://xbmc.org
+ *
+ * This Program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This Program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with XBMC; see the file COPYING. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "IPowerSyscall.h"
+
+const int IPowerSyscall::MAX_COUNT_POWER_FEATURES;
View
20 xbmc/powermanagement/IPowerSyscall.h
@@ -1,7 +1,7 @@
#pragma once
/*
- * Copyright (C) 2005-2013 Team XBMC
+ * Copyright (C) 2005-2015 Team XBMC
* http://xbmc.org
*
* This Program is free software; you can redistribute it and/or modify
@@ -45,6 +45,8 @@ class IPowerSyscall
virtual bool CanSuspend() = 0;
virtual bool CanHibernate() = 0;
virtual bool CanReboot() = 0;
+
+ virtual int CountPowerFeatures() = 0;
// Battery related functions
virtual int BatteryLevel() = 0;
@@ -60,9 +62,23 @@ class IPowerSyscall
\param callback the callback to signal to
*/
virtual bool PumpPowerEvents(IPowerEventsCallback *callback) = 0;
+
+ static const int MAX_COUNT_POWER_FEATURES = 4;
+};
+
+class CAbstractPowerSyscall : public IPowerSyscall
+{
+public:
+ virtual int CountPowerFeatures()
+ {
+ return (CanPowerdown() ? 1 : 0)
+ + (CanSuspend() ? 1 : 0)
+ + (CanHibernate() ? 1 : 0)
+ + (CanReboot() ? 1 : 0);
+ }
};
-class CPowerSyscallWithoutEvents : public IPowerSyscall
+class CPowerSyscallWithoutEvents : public CAbstractPowerSyscall
{
public:
CPowerSyscallWithoutEvents() { m_OnResume = false; m_OnSuspend = false; }
View
1  xbmc/powermanagement/Makefile
@@ -1,5 +1,6 @@
SRCS=DPMSSupport.cpp \
PowerManager.cpp \
+ IPowerSyscall.cpp \
LIB=powermanagement.a
View
46 xbmc/powermanagement/PowerManager.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2005-2013 Team XBMC
+ * Copyright (C) 2005-2015 Team XBMC
* http://xbmc.org
*
* This Program is free software; you can redistribute it and/or modify
@@ -18,6 +18,8 @@
*
*/
+#include <memory>
+#include <list>
#include "system.h"
#include "PowerManager.h"
#include "Application.h"
@@ -73,14 +75,40 @@ void CPowerManager::Initialize()
m_instance = new CAndroidPowerSyscall();
#elif defined(TARGET_POSIX)
#if defined(HAS_DBUS)
- if (CConsoleUPowerSyscall::HasConsoleKitAndUPower())
- m_instance = new CConsoleUPowerSyscall();
- else if (CConsoleDeviceKitPowerSyscall::HasDeviceConsoleKit())
- m_instance = new CConsoleDeviceKitPowerSyscall();
- else if (CLogindUPowerSyscall::HasLogind())
- m_instance = new CLogindUPowerSyscall();
- else if (CUPowerSyscall::HasUPower())
- m_instance = new CUPowerSyscall();
+ std::unique_ptr<IPowerSyscall> bestPowerManager;
+ std::unique_ptr<IPowerSyscall> currPowerManager;
+ int bestCount = -1;
+ int currCount = -1;
+
+ std::list< std::pair< std::function<bool()>,
+ std::function<IPowerSyscall*()> > > powerManagers =
+ {
+ std::make_pair(CConsoleUPowerSyscall::HasConsoleKitAndUPower,
+ [] { return new CConsoleUPowerSyscall(); }),
+ std::make_pair(CConsoleDeviceKitPowerSyscall::HasDeviceConsoleKit,
+ [] { return new CConsoleDeviceKitPowerSyscall(); }),
+ std::make_pair(CLogindUPowerSyscall::HasLogind,
+ [] { return new CLogindUPowerSyscall(); }),
+ std::make_pair(CUPowerSyscall::HasUPower,
+ [] { return new CUPowerSyscall(); })
+ };
+ for(const auto& powerManager : powerManagers)
+ {
+ if (powerManager.first())
+ {
+ currPowerManager.reset(powerManager.second());
+ currCount = currPowerManager->CountPowerFeatures();
+ if (currCount > bestCount)
+ {
+ bestCount = currCount;
+ bestPowerManager = std::move(currPowerManager);
+ }
+ if (bestCount == IPowerSyscall::MAX_COUNT_POWER_FEATURES)
+ break;
+ }
+ }
+ if (bestPowerManager)
+ m_instance = bestPowerManager.release();
else
#endif // HAS_DBUS
m_instance = new CFallbackPowerSyscall();
View
2  xbmc/powermanagement/PowerManager.h
@@ -41,7 +41,7 @@ enum PowerState
};
// For systems without PowerSyscalls we have a NullObject
-class CNullPowerSyscall : public IPowerSyscall
+class CNullPowerSyscall : public CAbstractPowerSyscall
{
public:
virtual bool Powerdown() { return false; }
View
2  xbmc/powermanagement/linux/LogindUPowerSyscall.h
@@ -24,7 +24,7 @@
#include "powermanagement/IPowerSyscall.h"
#include "DBusUtil.h"
-class CLogindUPowerSyscall : public IPowerSyscall
+class CLogindUPowerSyscall : public CAbstractPowerSyscall
{
public:
CLogindUPowerSyscall();
View
2  xbmc/powermanagement/linux/UPowerSyscall.h
@@ -43,7 +43,7 @@ class CUPowerSource
double m_batteryLevel;
};
-class CUPowerSyscall : public IPowerSyscall
+class CUPowerSyscall : public CAbstractPowerSyscall
{
public:
CUPowerSyscall();
View
2  xbmc/powermanagement/windows/Win32PowerSyscall.h
@@ -25,7 +25,7 @@
#define _WIN32_POWER_SYSCALL_H_
#include "powermanagement/IPowerSyscall.h"
-class CWin32PowerSyscall : public IPowerSyscall
+class CWin32PowerSyscall : public CAbstractPowerSyscall
{
public:
CWin32PowerSyscall();
Something went wrong with that request. Please try again.