[rbp] Disable Hibernate and Suspend related power options for Raspberry ... #2403

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

MilhouseVH commented Mar 10, 2013

...Pi as these are not supported

Contributor

MilhouseVH commented Mar 10, 2013

Obviously this patch only applies to the Raspberry Pi and I'm not suggesting this PR should be merged to master, but it can be used as a patch when building R-Pi projects - for example, OpenELEC, XBian, Raspbmc.

Contributor

MilhouseVH commented Mar 11, 2013

Updated patch to include #ifdefs for Raspberry Pi so that default Suspend/Hibernate behaviour is restored for non-Raspberry Pi systems. I realise #ifdefs are frowned upon, but not sure of better way to implement this right now.

riso commented Mar 11, 2013

I think that a better way would not to touch the NullObject and create a new implementation of the IPowerSyscall specific for rpi. And then fix the factory so that if TARGET_RASPBERRY_PI the rpi implementation is instantiated.

But I'm not a XBMC developer (just a user) so this is only my opinion from a general design point of view :-)

Contributor

stupid-boy commented Mar 11, 2013

alternatively to specific implementation may be you can use /sys/power/state file.
see "The PM sysfs Interface" in http://landley.net/kdocs/ols/2003/ols2003-pages-325-339.pdf
on my RPI this file is empty, indicating no standby/suspend/hibernate are available. exactly as "pm-is-supported --XXX ; echo $?" indicates too.
this should be kind a universal and portable method. let others with more linux knowledge help here.

Member

theuni commented Mar 11, 2013

What he said.

Contributor

MilhouseVH commented Mar 11, 2013

Than again, according to this forum post by popcornmix, hal is available on the Pi platform, so if OpenELEC (and Xbian/Raspbmc etc.) can be built with hal (HAS_HAL), maybe no code change are required at all and the Pi can use CHALPowerSyscall?

Member

sraue commented Mar 11, 2013

@popcornmix, @MilhouseVH HAL is deprecated and should not be used anymore. XBMC can work completly HAL free and should do this. @theuni the problem is the follow, xbmc "asks" upower if suspend/hibernate is supported and shows the options in the powermenu if its supported. but if upower is not installed (like in OpenELEC for RPi and ATV) because both systems simply dont support this and upower depends on consolekit, which depends on on x11 libs. If XBMC dont get a answer from upower via dbus, then all options are displayed. The right solution (under linux) will be if upower dont work, is not installed etc... then suspend/hibernate will not work, so then this options should be disabled by default - and only activated if upower (or the older implemention devicekit-power) reports it is supported. checking /sys/power/state is not needed there, because xbmc will not support supend/hibernate if the file is there but upower/devicekit-power is not installed.

Contributor

MilhouseVH commented Mar 11, 2013

@sraue makes a valid point: If the system can't determine what power options are available, then the defaults offered should be conservative, which would mean Suspend and Hibernate disabled. No ifdefs, just change the current CNullPowerSyscall settings for Suspend and Hibernate to false. Is there an argument for them being set to true, when in most cases they won't work anyway?

Member

theuni commented Mar 11, 2013

HAL is deprecated everywhere, it should be avoided.

Member

theuni commented Mar 11, 2013

Whoops, I commented before refreshing the page. @sraue set it better :)

Member

popcornmix commented Mar 11, 2013

So if the correct solution to make:
virtual bool CanSuspend() { return false; }
virtual bool CanHibernate() { return false; }
i.e. @MilhouseVH patch but unconditional?

Member

sraue commented Mar 11, 2013

i can test it here later today/tomorrow on various systems with and without upower installed and with and without suspend supported

Member

sraue commented Mar 12, 2013

@MilhouseVH i have tested your patch but without the ifdef's. it works as expected (not tested with hibernate, but it should not be different):

  • suspend supported, upower installed: suspend will be shown in the shutdownmenu
  • suspend supported, upower NOT installed: suspend will NOT be shown in the shutdownmenu
  • suspend NOT supported, upower installed: suspend will NOT be shown in the shutdownmenu
  • suspend NOT supported, upower NOT installed: suspend will NOT be shown in the shutdownmenu

can you please update your PR, then its ok from my side to go in master
btw: like i told, its not a RPi related "problem" its a general "problem"

Contributor

MilhouseVH commented Mar 12, 2013

Patch updated - many thanks @sraue.

Member

theuni commented Mar 13, 2013

Rather than defining bare-metal linux as NULL, I'd rather see it have its own implementation.

See alternate implementation here (only compile-tested):

From f9c3cdaea46c62a2280cdf6781917db7c7711ad4 Mon Sep 17 00:00:00 2001
From: Cory Fields <theuni-nospam-@xbmc.org>
Date: Wed, 13 Mar 2013 12:55:52 -0400
Subject: [PATCH] power: Add a bare-metal linux power implementation that
 relies on being launched by init

---
 xbmc/powermanagement/PowerManager.cpp             | 11 +++---
 xbmc/powermanagement/linux/FallbackPowerSyscall.h | 41 +++++++++++++++++++++++
 2 files changed, 47 insertions(+), 5 deletions(-)
 create mode 100644 xbmc/powermanagement/linux/FallbackPowerSyscall.h

diff --git a/xbmc/powermanagement/PowerManager.cpp b/xbmc/powermanagement/PowerManager.cpp
index ca94f49..1172e81 100644
--- a/xbmc/powermanagement/PowerManager.cpp
+++ b/xbmc/powermanagement/PowerManager.cpp
@@ -42,9 +42,10 @@
 #include "linux/ConsoleDeviceKitPowerSyscall.h"
 #include "linux/SystemdUPowerSyscall.h"
 #include "linux/UPowerSyscall.h"
-#ifdef HAS_HAL
+#elif defined(HAS_HAL)
 #include "linux/HALPowerSyscall.h"
-#endif
+#elif defined(TARGET_LINUX)
+#include "linux/FallbackPowerSyscall.h"
 #elif defined(_WIN32)
 #include "powermanagement/windows/Win32PowerSyscall.h"
 extern HWND g_hWnd;
@@ -79,10 +80,10 @@ void CPowerManager::Initialize()
     m_instance = new CSystemdUPowerSyscall();
   else if (CUPowerSyscall::HasUPower())
     m_instance = new CUPowerSyscall();
-#ifdef HAS_HAL
-  else
+#elif defined(HAS_HAL)
     m_instance = new CHALPowerSyscall();
-#endif
+#elif defined(TARGET_LINUX)
+    m_instance = new CFallbackPowerSyscall();
 #elif defined(_WIN32)
   m_instance = new CWin32PowerSyscall();
 #endif
diff --git a/xbmc/powermanagement/linux/FallbackPowerSyscall.h b/xbmc/powermanagement/linux/FallbackPowerSyscall.h
new file mode 100644
index 0000000..1f0f300
--- /dev/null
+++ b/xbmc/powermanagement/linux/FallbackPowerSyscall.h
@@ -0,0 +1,41 @@
+/*
+ *      Copyright (C) 2005-2013 Team XBMC
+ *      http://www.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/>.
+ *
+ */
+#pragma once
+#include "powermanagement/IPowerSyscall.h"
+#include "system.h"
+#if defined(TARGET_LINUX)
+
+class CFallbackPowerSyscall : public CPowerSyscallWithoutEvents
+{
+public:
+  CFallbackPowerSyscall();
+
+  virtual bool Powerdown() {return true; }
+  virtual bool Suspend() {return false; }
+  virtual bool Hibernate() {return false; }
+  virtual bool Reboot() {return true; }
+
+  virtual bool CanPowerdown() {return true; }
+  virtual bool CanSuspend() {return false; }
+  virtual bool CanHibernate() {return false; }
+  virtual bool CanReboot() {return true; }
+  virtual int  BatteryLevel() {return 0; }
+};
+#endif
-- 
1.8.0
Member

theuni commented Mar 13, 2013

er, I accidentally borked those ifdefs so that only 1 implementation will be compiled in, but you get the idea :)

Member

theuni commented Mar 13, 2013

Ok, minus the bork this time (I hope): https://gist.github.com/theuni/5154204

Contributor

MilhouseVH commented Mar 13, 2013

I suppose fundamentally this PR is questioning whether the default NULL provider (as used by the Pi, and probably Apple) should be reporting that Suspend and Hibernate is supported when most likely they are not... how we get to the NULL provider, or a Fallback provider, is probably for another PR entirely... certainly not one I'm comfortable submitting.

Member

theuni commented Mar 13, 2013

@MilhouseVH Darwin has its own implementation for osx/ios. What we're really defining here is the procedure to be used when XBMC is started from a Linux init script, with the understanding that its exit code will determine shutdown/poweroff. For that reason, it's definitely Linux-specific, and not NULL. It might make sense to rename this to LinuxInit or so.

In fact, IMO a NULL implementation would provide none of the options, since it doesn't know how to do any of them.

Contributor

stupid-boy commented Mar 13, 2013

don't get me wrong, i prefer current approach with hardcoded "return false;" instead of "/sys/power/state" variant.

as @theuni mention init script, in terms of completeness/correctness, /sys/power/state is better exactly because init ( or as in my case, bash script ) knows how to deal with these options. only it can correctly "echo disk | sudo tee /sys/power/state" and can add resume hook to restart XBMC. also only it can prefer "pm-suspend-hybrid" over other methods. this way concrete support depend on distribution maintainers choice and this method gives them ability to decide.

i do not like these options in XBMC because it is multimedia center, but not system utility. that is why i prefer current hardcoded version with disabled options. skilled user knows how to proceed with them, for others they will add only troubles. that is why i will be happy if they disappear in windows build too, but this is personal preference.

just some regular user point of view on that.

Member

sraue commented Mar 13, 2013

@theuni here the paste of the fixed patch, based on your work: http://pastebin.com/0fWgqky5 @MilhouseVH can you test this one too and report back, but it should work for you too

Member

theuni commented Mar 14, 2013

Patch looks good

Contributor

MilhouseVH commented Mar 14, 2013

@sraue: Patch seems to build and test OK for Raspberry Pi.

riso commented Mar 14, 2013

👍 I like that aswell.

Just one thing, probably due to the fact that my C is pretty rusty, is that else if(1) needed @ line 85 of your patch @sraue ? Doesn't a simple else suffice?

Contributor

MilhouseVH commented Mar 14, 2013

Yes, the else if(1) does seem redundant, just else will suffice.

Member

sraue commented Mar 14, 2013

the '''else if(1)''' is there to force this if we have '''HAS_HAL''' defined and '''m_instance''' is still not set (no (working) upower/devicekit/systemd). if we build without HAL (in most cases) this '''else if(1)''' will not be included and '''m_instance = new CFallbackPowerSyscall();''' will be run with no (working) upower/devicekit/systemd installed. i am not sure if compiling will fail without '''else if(1)''' in line 85 because two times '''else''' without a '''if''' (if building with HAL support) is generally wrong in if/then statements.

Contributor

t-nelson commented Mar 14, 2013

A series of if(!m_instance) would read better.

Contributor

MilhouseVH commented Mar 14, 2013

Wouldn't this achieve the same result, while being less contrived:

#if defined(HAS_HAL)
  else #if (1)
    m_instance = new CHALPowerSyscall();
#endif // HAS_HAL
#endif // HAS_DBUS
  if (m_instance == NULL)  #else
    m_instance = new CFallbackPowerSyscall();
Member

sraue commented Mar 18, 2013

@MilhouseVH this i used before, but changed this on @theuni 's request, so i think he is more happy with the paste from me

Contributor

MilhouseVH commented Jul 25, 2013

Looks like recent commits have rendered this patch unusable.

Owner

MartijnKaijser commented Aug 1, 2013

@MilhouseVH
should it be closed then if this patch can't be used anymore or are you going to rework this?

Contributor

MilhouseVH commented Aug 1, 2013

I'm guessing that @sraue is using his own version of the patch, as it's working again now (building OE from git). Shame the patch can't/won't be accepted into mainline, save everyone the headache of maintaining a patch.

Member

sraue commented Aug 1, 2013

@MartijnKaijser @MilhouseVH i have done a a slightly changed patch based on yours, i can do a new PR for this (not sure if i can update this PR byself.

Owner

MartijnKaijser commented Aug 2, 2013

#3033 will super seed this PR
@MilhouseVH thx for the initial PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment