cDevice::GetDeviceForTransponder(): fix a typo

  On 26.05.2016 17:58, glenvt18 wrote:
> d->MaySwitchTransponder(Channel) is always false here
>
> Please review,
> Sergey Chernyavskiy.
>
> ---
>  device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/device.c b/device.c
> index 18867cd..542d120 100644
> --- a/device.c
> +++ b/device.c
> @@ -342,7 +342,7 @@ cDevice *cDevice::GetDeviceForTransponder(const cChannel *Channel, int Priority)
>           if (d->ProvidesTransponder(Channel)) {
>              if (d->MaySwitchTransponder(Channel))
>                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
> -            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
> +            else if (!d->Occupied()) { // MaySwitchTransponder() implicitly calls Occupied()
>                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
>                    Device = d; // use this one only if no other with less impact can be found
>                 }

This was introduced in version 1.7.29:


with the change comment "Fixed handling recording with more than two bonded devices".

While I tend to agree with you in that d->MaySwitchTransponder(Channel) is always false here,
this must at least have fixed the problem at hand back then (it was the only file that
has been modified for this fix). I'm therefore hesitant to remove this call and
risk breaking something...

Klaus
  

I understand. But this code looks confusing (at least). And, just to
draw your attention, there is no priority checking here unless
MaySwitchTransponder() returns true on the second call (which would be
really ugly). Thus, for example, this code (vdr.c:1109)

                 if (NeedsTransponder || InVpsMargin) {
                    // Find a device that provides the required transponder:
                    cDevice *Device = cDevice::GetDeviceForTransponder(Timer->Channel(), MINPRIORITY);
                    if (!Device && InVpsMargin)
                       Device = cDevice::GetDeviceForTransponder(Timer->Channel(), LIVEPRIORITY);
                    // Switch the device to the transponder:
                    if (Device) {
                       ...

has no sense.

On Thu, Dec 22, 2016 at 02:56:10PM +0100, Klaus Schmidinger wrote:
> On 26.05.2016 17:58, glenvt18 wrote:
> > d->MaySwitchTransponder(Channel) is always false here
> > 
> > Please review,
> > Sergey Chernyavskiy.
> > 
> > ---
> >  device.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/device.c b/device.c
> > index 18867cd..542d120 100644
> > --- a/device.c
> > +++ b/device.c
> > @@ -342,7 +342,7 @@ cDevice *cDevice::GetDeviceForTransponder(const cChannel *Channel, int Priority)
> >           if (d->ProvidesTransponder(Channel)) {
> >              if (d->MaySwitchTransponder(Channel))
> >                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
> > -            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
> > +            else if (!d->Occupied()) { // MaySwitchTransponder() implicitly calls Occupied()
> >                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
> >                    Device = d; // use this one only if no other with less impact can be found
> >                 }
> 
> This was introduced in version 1.7.29:
> 
> --- device.c    2012/06/09 14:37:24     2.61
> +++ device.c    2012/06/10 13:13:18     2.62
> @@ -334,7 +334,7 @@
>           if (d->ProvidesTransponder(Channel)) {
>              if (d->MaySwitchTransponder(Channel))
>                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
> -            else if (!d->Occupied()) {
> +            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
>                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
>                    Device = d; // use this one only if no other with less impact can be found
>                 }
> 
> with the change comment "Fixed handling recording with more than two bonded devices".
> 
> While I tend to agree with you in that d->MaySwitchTransponder(Channel) is always false here,
> this must at least have fixed the problem at hand back then (it was the only file that
> has been modified for this fix). I'm therefore hesitant to remove this call and
> risk breaking something...
> 
> Klaus
> 
> _______________________________________________
> vdr mailing list
> vdr@linuxtv.org
> https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
  

On 22.12.2016 16:07, glenvt18 wrote:
> I understand. But this code looks confusing (at least). And, just to
> draw your attention, there is no priority checking here unless
> MaySwitchTransponder() returns true on the second call (which would be
> really ugly). Thus, for example, this code (vdr.c:1109)
>
>                  if (NeedsTransponder || InVpsMargin) {
>                     // Find a device that provides the required transponder:
>                     cDevice *Device = cDevice::GetDeviceForTransponder(Timer->Channel(), MINPRIORITY);
>                     if (!Device && InVpsMargin)
>                        Device = cDevice::GetDeviceForTransponder(Timer->Channel(), LIVEPRIORITY);
>                     // Switch the device to the transponder:
>                     if (Device) {
>                        ...
>
> has no sense.

I totally agree! And I have nothing to counter the logic in your
arguments.

However, before I revert the change that was introduced in version
1.7.29, I would need somebody with a setup that contains "bonded"
devices to test whether recordings still work properly with the
second call to d->MaySwitchTransponder(Channel) removed.

Is anybody actually using "device bonding" (or "LNB-sharing", as it was
named when this was still a patch)? Personally I never liked this and
I still believe that it makes things unnecessarily complex and should
be removed again...

Klaus

> On Thu, Dec 22, 2016 at 02:56:10PM +0100, Klaus Schmidinger wrote:
>> On 26.05.2016 17:58, glenvt18 wrote:
>>> d->MaySwitchTransponder(Channel) is always false here
>>>
>>> Please review,
>>> Sergey Chernyavskiy.
>>>
>>> ---
>>>  device.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/device.c b/device.c
>>> index 18867cd..542d120 100644
>>> --- a/device.c
>>> +++ b/device.c
>>> @@ -342,7 +342,7 @@ cDevice *cDevice::GetDeviceForTransponder(const cChannel *Channel, int Priority)
>>>           if (d->ProvidesTransponder(Channel)) {
>>>              if (d->MaySwitchTransponder(Channel))
>>>                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
>>> -            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
>>> +            else if (!d->Occupied()) { // MaySwitchTransponder() implicitly calls Occupied()
>>>                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
>>>                    Device = d; // use this one only if no other with less impact can be found
>>>                 }
>>
>> This was introduced in version 1.7.29:
>>
>> --- device.c    2012/06/09 14:37:24     2.61
>> +++ device.c    2012/06/10 13:13:18     2.62
>> @@ -334,7 +334,7 @@
>>           if (d->ProvidesTransponder(Channel)) {
>>              if (d->MaySwitchTransponder(Channel))
>>                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
>> -            else if (!d->Occupied()) {
>> +            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
>>                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
>>                    Device = d; // use this one only if no other with less impact can be found
>>                 }
>>
>> with the change comment "Fixed handling recording with more than two bonded devices".
>>
>> While I tend to agree with you in that d->MaySwitchTransponder(Channel) is always false here,
>> this must at least have fixed the problem at hand back then (it was the only file that
>> has been modified for this fix). I'm therefore hesitant to remove this call and
>> risk breaking something...
>>
>> Klaus
>>
>> _______________________________________________
>> vdr mailing list
>> vdr@linuxtv.org
>> https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
>
  

Just an observation. That code from vdr.c that calls
GetDeviceForTransponder() (for example, when a VPS timer is about to
start) can only interrupt tasks with priority < LIVEPRIORITY anyway. It
looks like disabling priority checking can't do much harm here. Is that
what you originally meant?

BTW. Device bonding itself is a nice VDR feature. Unfortunately I can't
test it (with 3+ devices) :(

On Tue, Dec 27, 2016 at 01:38:51PM +0100, Klaus Schmidinger wrote:
> On 22.12.2016 16:07, glenvt18 wrote:
> > I understand. But this code looks confusing (at least). And, just to
> > draw your attention, there is no priority checking here unless
> > MaySwitchTransponder() returns true on the second call (which would be
> > really ugly). Thus, for example, this code (vdr.c:1109)
> > 
> >                  if (NeedsTransponder || InVpsMargin) {
> >                     // Find a device that provides the required transponder:
> >                     cDevice *Device = cDevice::GetDeviceForTransponder(Timer->Channel(), MINPRIORITY);
> >                     if (!Device && InVpsMargin)
> >                        Device = cDevice::GetDeviceForTransponder(Timer->Channel(), LIVEPRIORITY);
> >                     // Switch the device to the transponder:
> >                     if (Device) {
> >                        ...
> > 
> > has no sense.
> 
> I totally agree! And I have nothing to counter the logic in your
> arguments.
> 
> However, before I revert the change that was introduced in version
> 1.7.29, I would need somebody with a setup that contains "bonded"
> devices to test whether recordings still work properly with the
> second call to d->MaySwitchTransponder(Channel) removed.
> 
> Is anybody actually using "device bonding" (or "LNB-sharing", as it was
> named when this was still a patch)? Personally I never liked this and
> I still believe that it makes things unnecessarily complex and should
> be removed again...
> 
> Klaus
> 
> > On Thu, Dec 22, 2016 at 02:56:10PM +0100, Klaus Schmidinger wrote:
> > > On 26.05.2016 17:58, glenvt18 wrote:
> > > > d->MaySwitchTransponder(Channel) is always false here
> > > > 
> > > > Please review,
> > > > Sergey Chernyavskiy.
> > > > 
> > > > ---
> > > >  device.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/device.c b/device.c
> > > > index 18867cd..542d120 100644
> > > > --- a/device.c
> > > > +++ b/device.c
> > > > @@ -342,7 +342,7 @@ cDevice *cDevice::GetDeviceForTransponder(const cChannel *Channel, int Priority)
> > > >           if (d->ProvidesTransponder(Channel)) {
> > > >              if (d->MaySwitchTransponder(Channel))
> > > >                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
> > > > -            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
> > > > +            else if (!d->Occupied()) { // MaySwitchTransponder() implicitly calls Occupied()
> > > >                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
> > > >                    Device = d; // use this one only if no other with less impact can be found
> > > >                 }
> > > 
> > > This was introduced in version 1.7.29:
> > > 
> > > --- device.c    2012/06/09 14:37:24     2.61
> > > +++ device.c    2012/06/10 13:13:18     2.62
> > > @@ -334,7 +334,7 @@
> > >           if (d->ProvidesTransponder(Channel)) {
> > >              if (d->MaySwitchTransponder(Channel))
> > >                 Device = d; // this device may switch to the transponder without disturbing any receiver or live view
> > > -            else if (!d->Occupied()) {
> > > +            else if (!d->Occupied() && d->MaySwitchTransponder(Channel)) { // MaySwitchTransponder() implicitly calls Occupied()
> > >                 if (d->Priority() < Priority && (!Device || d->Priority() < Device->Priority()))
> > >                    Device = d; // use this one only if no other with less impact can be found
> > >                 }
> > > 
> > > with the change comment "Fixed handling recording with more than two bonded devices".
> > > 
> > > While I tend to agree with you in that d->MaySwitchTransponder(Channel) is always false here,
> > > this must at least have fixed the problem at hand back then (it was the only file that
> > > has been modified for this fix). I'm therefore hesitant to remove this call and
> > > risk breaking something...
> > > 
> > > Klaus
> > > 
> > > _______________________________________________
> > > vdr mailing list
> > > vdr@linuxtv.org
> > > https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
> > 
> 
> _______________________________________________
> vdr mailing list
> vdr@linuxtv.org
> https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
  

On 28.12.2016 15:26, glenvt18 wrote:
> Just an observation. That code from vdr.c that calls
> GetDeviceForTransponder() (for example, when a VPS timer is about to
> start) can only interrupt tasks with priority < LIVEPRIORITY anyway. It
> looks like disabling priority checking can't do much harm here. Is that
> what you originally meant?

Well, all I know is that I'm not daring to touch any of this
at this time. I'm probably going to rewrite the whole device
selection stuff from scratch one day...

Klaus