[v2] Device power saving feature

  I've renamed PowerDown() to PowerDownMode() as Lars suggested and
added a comment on IsTunedToTransponder().

Klaus, could you take a look at this please.

A copy of this patch:
http://pastebin.com/skxw80hN

VDR 2.2.0 version:
http://pastebin.com/JQLjMKvu

Add a newline to the end of a patch file downloaded from pastebin.com.


This patch introduces a feature which allows an idle device (a device
which is not currently recording or streaming) to enter a power-down
mode after some period of time. Given two timeout values,
PowerdownTimeoutM and PowerdownWakeupH, it works like this: when a
device becomes idle, it is kept powered up for PowerdownTimeoutM minutes
doing, for instance, an EPG scan before it is powered down. If the
device is still idle and has been powered down for PowerdownWakeupH
hours it is powered up for PowerdownTimeoutM minutes and so on. When
recording, streaming or a forced EPG scan starts, the device is powered
up and it's idle timer is disabled. This implies that PowerdownTimeoutM
should be enough for a full round of EPG scanning (20 seconds *
number_of_transponders). Another option is to run EPG scans from cron
(at night) and use SVDRP SCAN command.

Actual implementation of power saving facilities is left to a derived
device class. In the case of a DVB device it is implemented by closing
it's frontend device. For a DVB-S/S2 tuner this usually means powering
the LNB off. My measurements show 3-4W power consumption drops per tuner
for various DVB-S/S2 tuners. So, this feature (together with HDD
spin-down) is especially valuable while running a headless 24/7 VDR
server and/or using several tuners. A SATIP device can also implement
power saving if it is supported by a server.
---
 config.c    |  9 ++++++
 config.h    |  3 ++
 device.c    | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 device.h    | 31 ++++++++++++++++++++
 dvbdevice.c | 39 +++++++++++++++++++++++++
 dvbdevice.h |  7 +++++
 eitscan.c   |  7 ++++-
 menu.c      |  9 +++++-
 vdr.c       |  6 ++++
 9 files changed, 203 insertions(+), 4 deletions(-)
  

On 18.08.2016 17:32, glenvt18 wrote:
> I've renamed PowerDown() to PowerDownMode() as Lars suggested and
> added a comment on IsTunedToTransponder().
>
> Klaus, could you take a look at this please.

Just a few comments that came up while quickly looking over the diff.

> ...
> diff --git a/config.c b/config.c
> index e5f5463..794c9f8 100644
> --- a/config.c
> +++ b/config.c
> @@ -395,6 +395,9 @@ cSetup::cSetup(void)
>    PositionerSpeed = 15;
>    PositionerSwing = 650;
>    PositionerLastLon = 0;
> +  PowerdownEnabled = 0;
> +  PowerdownTimeoutM = 15;
> +  PowerdownWakeupH = 4;

Couldn't PowerdownEnabled be replaced by checking for 'PowerdownTimeoutM != 0'?

Why exactly do we need PowerdownWakeupH?
A device gets woken up if it is used, so why the
extra wakeup? Am I missing something here?

> ...
> diff --git a/device.c b/device.c
> index 542d120..adbe973 100644
> --- a/device.c
> +++ b/device.c
> ...
> @@ -843,8 +853,11 @@ int cDevice::Occupied(void) const
>
>  void cDevice::SetOccupied(int Seconds)
>  {
> -  if (Seconds >= 0)
> +  if (Seconds >= 0) {
>       occupiedTimeout = time(NULL) + min(Seconds, MAXOCCUPIEDTIMEOUT);
> +     // avoid short power-down/power-up cycles
> +     SetIdleTimer(true, Seconds + 30);

Is '30' some magic number?
Maybe define a macro or at least add some comment that explains it.
Besides, see below about the necessity of ExtraTimeoutS.

> ...
> @@ -1731,6 +1747,82 @@ void cDevice::DetachAllReceivers(void)
>        Detach(receiver[i]);
>  }
>
> +void cDevice::CheckIdle(void)
> +{
> +  if (!SupportsPowerDown() || !Setup.PowerdownEnabled)

Better check Setup.PowerdownEnabled first (just a feeling ;-).

> ...
> diff --git a/device.h b/device.h
> index 31ee303..cc40bb7 100644
> --- a/device.h
> +++ b/device.h
> @@ -821,6 +821,37 @@ public:
>         ///< Detaches all receivers from this device for this pid.
>    virtual void DetachAllReceivers(void);
>         ///< Detaches all receivers from this device.
> +
> +// Power saving facilities
> +
> +private:
> +  cMutex mutexPowerSaving;
> +  time_t idleTimerExpires, wakeupTimerExpires;
> +  void PowerUp(int ExtraTimeoutS);
> +       ///< If the device is powered down, powers it up and keeps it
> +       ///< powered up for at least ExtraTimeoutS seconds (see
> +       ///< cDevice::SetIdleTimer()).

What's the need for this ExtraTimeoutS?
Apparently this is only used with CHANNEL_SWITCH_POWERUP_TIMEOUT,
which is 10 seconds. The smallest power down timeout is one minute,
so why these extra 10 seconds?

Why do we need a PowerUp() function in the first place?
Wouldn't it suffice to call SetIdleTimer(false) and have
it call [Set]PowerDownMode() accordingly? (see below about
the [Set]).

Come to think of it, [Set]PowerDownMode(false) actually turns
a device *on*, right? This feels, well, uncomfortable to me.
I'd rather have a function that turns the device *on* when the
parameter is *true*. These "double negations" always feel somewhat
strange to me. Maybe rename that function to SetPowerMode()?

> +public:
> +  void CheckIdle(void);
> +       ///< Should be called periodically in the main loop.
> +  bool PoweredDown(void);
> +       ///< Returns true if the device is powered down "logically", that is,
> +       ///< idle tasks like EPG scanning are disabled.
> +  void SetIdleTimer(bool On, int ExtraTimeoutS = 0);
> +       ///< Starts/disables the idle timer. This timer must be started when
> +       ///< a device gets idle and must be disabled when it is receiving.
> +       ///< If ExtraTimeoutS is greater than zero and On is true, a new timer
> +       ///< won't be set, but the device will be kept powered up for at least
> +       ///< ExtraTimeoutS seconds.

I'm afraid I don't quite understand what this ExtraTimeoutS is about?
If every call to SetIdleTimer(true) sets the timer to at least one
minute from the time of the call, what's the need for an extra timeout?

> +protected:
> +       ///< NOTE: IsTunedToTransponder() should return false if the
> +       ///< device is powered down.
> +  virtual bool IsPoweredDown(void) {return false;}
> +       ///< Returns true if the device is powered down "physically".
> +  virtual void PowerDownMode(bool On) {};
> +       ///< Actually powers the device down/up.

Hmm, "PowerDownMode()" sounds more like a *query* to me than an actual
setting function. Personally I'd consider the original "PowerDown()"
more appropriate. Or, if it has to contain "Mode", maybe "SetPowerDownMode()".
See above for a general remark about "double negations" ;-).

> ...
> diff --git a/dvbdevice.c b/dvbdevice.c
> index 63af52e..87555b7 100644
> --- a/dvbdevice.c
> +++ b/dvbdevice.c
> ...
> @@ -1001,6 +1007,26 @@ void cDvbTuner::Action(void)
>          }
>  }
>
> +void cDvbTuner::PowerDownMode(bool On)
> +{
> +  cMutexLock MutexLock(&mutex);
> +  if (On && fd_frontend >= 0) {
> +     isyslog("dvb tuner: power-down - closing frontend %d/%d", adapter, frontend);
> +     tunerStatus = tsIdle;
> +     close(fd_frontend);
> +     fd_frontend = -1;
> +     }
> +  if (!On && fd_frontend < 0) {
> +     cString Filename = cString::sprintf("%s/%s%d/%s%d",
> +        DEV_DVB_BASE, DEV_DVB_ADAPTER, adapter, DEV_DVB_FRONTEND, frontend);

Is there a reason why you are not using DvbOpen() here?

> ...
> diff --git a/eitscan.c b/eitscan.c
> index 41ac25e..765055c 100644
> --- a/eitscan.c
> +++ b/eitscan.c
> @@ -144,7 +144,8 @@ void cEITScanner::Process(void)
>             bool AnyDeviceSwitched = false;
>             for (int i = 0; i < cDevice::NumDevices(); i++) {
>                 cDevice *Device = cDevice::GetDevice(i);
> -               if (Device && Device->ProvidesEIT()) {
> +               if (Device && Device->ProvidesEIT()
> +                     && (!Device->PoweredDown() || lastActivity == 0)) { // powered up or forced scan
>                    for (cScanData *ScanData = scanList->First(); ScanData; ScanData = scanList->Next(ScanData)) {
>                        const cChannel *Channel = ScanData->GetChannel();
>                        if (Channel) {
> @@ -165,6 +166,10 @@ void cEITScanner::Process(void)
>                                             }
>                                          }
>                                       //dsyslog("EIT scan: device %d  source  %-8s tp %5d", Device->DeviceNumber() + 1, *cSource::ToString(Channel->Source()), Channel->Transponder());
> +                                     if (lastActivity == 0)
> +                                        // forced scan - set idle timer for each channel switch;
> +                                        // this prevents powering down while scanning a transponder
> +                                        Device->SetIdleTimer(true, ScanTimeout + 5);

'5' - another magic number?

> ...
> diff --git a/menu.c b/menu.c
> index 569900c..5a89771 100644
> --- a/menu.c
> +++ b/menu.c
> @@ -3715,6 +3715,12 @@ void cMenuSetupLNB::Setup(void)
>       Add(new cMenuEditIntxItem(tr("Setup.LNB$Positioner speed (degrees/s)"), &data.PositionerSpeed, 1, 1800, 10));
>       }
>
> +  Add(new cMenuEditBoolItem(tr("Setup.LNB$Enable power saving"), &data.PowerdownEnabled));
> +  if (data.PowerdownEnabled) {
> +     Add(new cMenuEditIntItem(tr("Setup.LNB$Power down an idle device after (min)"), &data.PowerdownTimeoutM));
> +     Add(new cMenuEditIntItem(tr("Setup.LNB$Wake up from power-down after (h)"), &data.PowerdownWakeupH));
> +     }

This could be simplified if (as mentioned above) we replace PowerdownEnabled
with 'PowerdownTimeoutM != 0' and lose the PowerdownWakeupH (which's rationale
I don't see).

Klaus
  

Hi Klaus,

Thanks for reviewing.

> > diff --git a/config.c b/config.c
> > index e5f5463..794c9f8 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -395,6 +395,9 @@ cSetup::cSetup(void)
> >    PositionerSpeed = 15;
> >    PositionerSwing = 650;
> >    PositionerLastLon = 0;
> > +  PowerdownEnabled = 0;
> > +  PowerdownTimeoutM = 15;
> > +  PowerdownWakeupH = 4;
> 
> Couldn't PowerdownEnabled be replaced by checking for 'PowerdownTimeoutM != 0'?

PowerdownTimeoutM == 0 means that a device will be powered down
immediately after it's receiving thread has ended. It can be useful if,
for instance, you want to schedule EPG scan using SVDRP SCAN command or,
you don't use EIT at all and collect EPG data from external sources like
XMLTV.

> Why exactly do we need PowerdownWakeupH?
> A device gets woken up if it is used, so why the
> extra wakeup? Am I missing something here?

I see three major reasons for having this setting:

1. If a device is inactive (and is powered down) for a very long period
of time, periodic wake-ups for an EPG scan keep EPG up-to-date. Of
course, EPGScanTimeout can be used to achieve the same but, 1) it
follows it's own logic (for example, it can interrupt streaming with
priority=-1 and a single tuner - a known issue with VNSI) and, 2) is set
separately (not connected to power saving) and can be disabled (set to a
very big value). On the other hand, PowerdownWakeupH's logic is simple
and straightforward.

2. If you don't want to have a device powered down for long periods of
time to avoid freezing/hanging (give it kicks from time to time or,
in the case of a general device, let it do some housekeeping). It's
quite typical for a power saving function to have both timeouts.

3. These two timeouts together with scheduled scans give more
flexibility.

As long as PowerdownTimeoutM == o(PowerdownWakeupH) wake-ups don't add
much to the average power consumption. Setting PowerdownWakeupH to 0
disables waking up completely (device.c:1759). This can be made default. 

Here and below, when I refer to source line numbers I mean vanilla 
VDR 2.3.1 from
https://projects.vdr-developer.org/git/vdr.git/snapshot/vdr-2.3.1.tar.bz2
with only this patch applied.

> > diff --git a/device.c b/device.c
> > index 542d120..adbe973 100644
> > --- a/device.c
> > +++ b/device.c
> > ...
> > @@ -843,8 +853,11 @@ int cDevice::Occupied(void) const
> > 
> >  void cDevice::SetOccupied(int Seconds)
> >  {
> > -  if (Seconds >= 0)
> > +  if (Seconds >= 0) {
> >       occupiedTimeout = time(NULL) + min(Seconds, MAXOCCUPIEDTIMEOUT);
> > +     // avoid short power-down/power-up cycles
> > +     SetIdleTimer(true, Seconds + 30);
> 
> Is '30' some magic number?
> Maybe define a macro or at least add some comment that explains it.

SetOccupied() is called with TIMERDEVICETIMEOUT which is 8 seconds.
SetChannel() powers up a device for CHANNEL_SWITCH_POWERUP_TIMEOUT
(=10) seconds. When SetOccupied() is called sequentially, the device is
powered up for 10 seconds (the largest of the two), then it is powered
down and is immediately powered up again. Though my tuners handle those
quick up/down transitions correctly, I limited them to a reasonable time
interval - not less than 30 seconds. It's a kind of a filter. Maybe the
comment should be

    // avoid short (less than 30 seconds) power-down/power-up cycles

I don't think defining a local constant here will make the code look
clearer.

> Besides, see below about the necessity of ExtraTimeoutS.
Below.

> > @@ -1731,6 +1747,82 @@ void cDevice::DetachAllReceivers(void)
> >        Detach(receiver[i]);
> >  }
> > 
> > +void cDevice::CheckIdle(void)
> > +{
> > +  if (!SupportsPowerDown() || !Setup.PowerdownEnabled)
> 
> Better check Setup.PowerdownEnabled first (just a feeling ;-).

The statement here is: if a device doesn't support power saving, don't
execute any power-related code after this check. I can't imagine an
overridden function other than '{return true;}' but if calling a
function first is confusing, they can be swapped. It really doesn't
matter here.

> > diff --git a/device.h b/device.h
> > index 31ee303..cc40bb7 100644
> > --- a/device.h
> > +++ b/device.h
> > @@ -821,6 +821,37 @@ public:
> >         ///< Detaches all receivers from this device for this pid.
> >    virtual void DetachAllReceivers(void);
> >         ///< Detaches all receivers from this device.
> > +
> > +// Power saving facilities
> > +
> > +private:
> > +  cMutex mutexPowerSaving;
> > +  time_t idleTimerExpires, wakeupTimerExpires;
> > +  void PowerUp(int ExtraTimeoutS);
> > +       ///< If the device is powered down, powers it up and keeps it
> > +       ///< powered up for at least ExtraTimeoutS seconds (see
> > +       ///< cDevice::SetIdleTimer()).
> 
> What's the need for this ExtraTimeoutS?
> Apparently this is only used with CHANNEL_SWITCH_POWERUP_TIMEOUT,
> which is 10 seconds. The smallest power down timeout is one minute,
> so why these extra 10 seconds?

ExtraTimeoutS is passed to SetIdleTimer(). It has nothing to do with
Setup.PowerdownTimeoutM. When SetIdleTimer() is called without
ExtraTimeoutS (defaults to 0), it just arms the idle timer with
Setup.PowerdownTimeoutM. When it is called with ExtraTimeoutS it behaves
differently. It checks if the idle timer expires in ExtraTimeoutS
seconds from now and, if it does, *prolongs* it. If the idle timer is
not armed it is armed with ExtraTimeoutS seconds. In other words,
SetIdleTimer(true, T) guarantees that the device won't be unexpectedly
powered down in T seconds time from now. 

CHANNEL_SWITCH_POWERUP_TIMEOUT is used to ensure the device won't be
powered up forever if AttachReceiver() for some reason fails or is not
called at all. If AttachReceiver() succeeds, the idle timer is disabled
until the last receiver is detached. The value of 10 seconds should be
enough for SetChannelDevice()->AttachReceiver() chain to complete. As
SetChannelDevice() is non-blocking this is the case (even if
WAIT_FOR_TUNER_LOCK is defined - device.c:1663).

CHANNEL_SWITCH_POWERUP_TIMEOUT must be less than
cEITScanner::ScanTimeout (device.c:753). Otherwise, an EPG scan will
never end - PoweredDown() will be returning false every time.

> Why do we need a PowerUp() function in the first place?
> Wouldn't it suffice to call SetIdleTimer(false) and have
> it call [Set]PowerDownMode() accordingly? (see below about
> the [Set]).

We don't really. Any modern compiler will inline it anyway. I just don't
like the idea of spreading power-related code over cDevice, especially
mutex locking. It's like a nested class. That's why PowerUp() (as a
nested class method) accepts an argument.

> Come to think of it, [Set]PowerDownMode(false) actually turns
> a device *on*, right? This feels, well, uncomfortable to me.
> I'd rather have a function that turns the device *on* when the
> parameter is *true*. These "double negations" always feel somewhat
> strange to me. Maybe rename that function to SetPowerMode()?

Well, talking about power-down we also say 'enter/leave power-down
[mode]'. In this case there is no double negation :). I don't care much
about this name but, I think, it's better (for a derived class
developer) to have one 2-in-1 virtual function than 2 on/off functions.
SetPowerDownMode() sounds OK. How about EnterPowerDown[Mode]()?
SetPowerMode() - hmm, what is 'power mode'? I'm running out of names...

> > +public:
> > +  void CheckIdle(void);
> > +       ///< Should be called periodically in the main loop.
> > +  bool PoweredDown(void);
> > +       ///< Returns true if the device is powered down "logically", that is,
> > +       ///< idle tasks like EPG scanning are disabled.
> > +  void SetIdleTimer(bool On, int ExtraTimeoutS = 0);
> > +       ///< Starts/disables the idle timer. This timer must be started when
> > +       ///< a device gets idle and must be disabled when it is receiving.
> > +       ///< If ExtraTimeoutS is greater than zero and On is true, a new timer
> > +       ///< won't be set, but the device will be kept powered up for at least
> > +       ///< ExtraTimeoutS seconds.
> 
> I'm afraid I don't quite understand what this ExtraTimeoutS is about?
> If every call to SetIdleTimer(true) sets the timer to at least one
> minute from the time of the call, what's the need for an extra timeout?

Explained above - see PowerUp().
 
> > +protected:
> > +       ///< NOTE: IsTunedToTransponder() should return false if the
> > +       ///< device is powered down.
> > +  virtual bool IsPoweredDown(void) {return false;}
> > +       ///< Returns true if the device is powered down "physically".
> > +  virtual void PowerDownMode(bool On) {};
> > +       ///< Actually powers the device down/up.
> 
> Hmm, "PowerDownMode()" sounds more like a *query* to me than an actual
> setting function. Personally I'd consider the original "PowerDown()"
> more appropriate. Or, if it has to contain "Mode", maybe "SetPowerDownMode()".
> See above for a general remark about "double negations" ;-).
 
IsPoweredDown() should sound like a query too I hope ;-)

> > diff --git a/dvbdevice.c b/dvbdevice.c
> > index 63af52e..87555b7 100644
> > --- a/dvbdevice.c
> > +++ b/dvbdevice.c
> > ...
> > @@ -1001,6 +1007,26 @@ void cDvbTuner::Action(void)
> >          }
> >  }
> > 
> > +void cDvbTuner::PowerDownMode(bool On)
> > +{
> > +  cMutexLock MutexLock(&mutex);
> > +  if (On && fd_frontend >= 0) {
> > +     isyslog("dvb tuner: power-down - closing frontend %d/%d", adapter, frontend);
> > +     tunerStatus = tsIdle;
> > +     close(fd_frontend);
> > +     fd_frontend = -1;
> > +     }
> > +  if (!On && fd_frontend < 0) {
> > +     cString Filename = cString::sprintf("%s/%s%d/%s%d",
> > +        DEV_DVB_BASE, DEV_DVB_ADAPTER, adapter, DEV_DVB_FRONTEND, frontend);
> 
> Is there a reason why you are not using DvbOpen() here?
 
cDvbDevice::DvbOpen() is 'protected' and we are in a cDvbTuner member here.

> > diff --git a/eitscan.c b/eitscan.c
> > index 41ac25e..765055c 100644
> > --- a/eitscan.c
> > +++ b/eitscan.c
> > @@ -144,7 +144,8 @@ void cEITScanner::Process(void)
> >             bool AnyDeviceSwitched = false;
> >             for (int i = 0; i < cDevice::NumDevices(); i++) {
> >                 cDevice *Device = cDevice::GetDevice(i);
> > -               if (Device && Device->ProvidesEIT()) {
> > +               if (Device && Device->ProvidesEIT()
> > +                     && (!Device->PoweredDown() || lastActivity == 0)) { // powered up or forced scan
> >                    for (cScanData *ScanData = scanList->First(); ScanData; ScanData = scanList->Next(ScanData)) {
> >                        const cChannel *Channel = ScanData->GetChannel();
> >                        if (Channel) {
> > @@ -165,6 +166,10 @@ void cEITScanner::Process(void)
> >                                             }
> >                                          }
> >                                       //dsyslog("EIT scan: device %d  source  %-8s tp %5d", Device->DeviceNumber() + 1, *cSource::ToString(Channel->Source()), Channel->Transponder());
> > +                                     if (lastActivity == 0)
> > +                                        // forced scan - set idle timer for each channel switch;
> > +                                        // this prevents powering down while scanning a transponder
> > +                                        Device->SetIdleTimer(true, ScanTimeout + 5);
> 
> '5' - another magic number?

5 here means '1', but a bit bigger. In other words, it's O(1) and
o(ScanTimeout). It's a small margin used to ensure consecutive scans
overlap so that a device is powered up before the first transponder and
is powered down after the last one. This is done only if a forced scan
is requested.

> > diff --git a/menu.c b/menu.c
> > index 569900c..5a89771 100644
> > --- a/menu.c
> > +++ b/menu.c
> > @@ -3715,6 +3715,12 @@ void cMenuSetupLNB::Setup(void)
> >       Add(new cMenuEditIntxItem(tr("Setup.LNB$Positioner speed (degrees/s)"), &data.PositionerSpeed, 1, 1800, 10));
> >       }
> > 
> > +  Add(new cMenuEditBoolItem(tr("Setup.LNB$Enable power saving"), &data.PowerdownEnabled));
> > +  if (data.PowerdownEnabled) {
> > +     Add(new cMenuEditIntItem(tr("Setup.LNB$Power down an idle device after (min)"), &data.PowerdownTimeoutM));
> > +     Add(new cMenuEditIntItem(tr("Setup.LNB$Wake up from power-down after (h)"), &data.PowerdownWakeupH));
> > +     }
> 
> This could be simplified if (as mentioned above) we replace PowerdownEnabled
> with 'PowerdownTimeoutM != 0' and lose the PowerdownWakeupH (which's rationale
> I don't see).

I think hiding timeout options when power saving is disabled and
expanding them when it is enabled looks pretty nice in UI - just like
BIOS settings ;-)