Re: [vdr] [PATCH v2] Device power saving feature

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