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 ;-) -- Sergey Chernyavskiy _______________________________________________ vdr mailing list vdr@xxxxxxxxxxx https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
