[vlc-devel] [PATCH] dtv multisat: Add support for uncommitted diseqc switch.

[Zoran Turalija]

First of all, thank you for review.

On Sat, Aug 11, 2012 at 11:15:39AM +0300, Rémi Denis-Courmont wrote:
> Looks fine but...
> 
> Le vendredi 10 août 2012 21:34:42 Zoran Turalija, vous avez écrit :
> > +#define UNCOMMITTED_TEXT N_("Uncommitted DiSEqC LNB number")
> > +#define UNCOMMITTED_LONGTEXT N_( \
> > +    "If the satellite receiver is connected to multiple " \
> > +    "low noise block-downconverters (LNB) through a cascade formed from "
> > \ +    "DiSEqC 1.1 uncommitted switch and DiSEqC 1.0 committed switch, " \
> > +    "the correct uncommitted LNB can be selected (1 to 4). " \
> > +    "If there is no uncommitted switch, this parameter should be 0.")
> > +#ifdef __linux__
> > +static const int uncommitted_vlc[] = { 0, 1, 2, 3, 4 };
> > +static const char *const uncommitted_user[] = { N_("Unspecified"),
> > +    "A/1", "B/2", "C/3", "D/4" };
> > +#endif
> 
> There should be no need to replicate the same tables as for satno, I think.

I'll reuse satno...

> 
> > +
> >  /* BDA module additional DVB-S Parameters */
> >  #define NETID_TEXT N_("Network identifier")
> >  #define AZIMUTH_TEXT N_("Satellite azimuth")
> > @@ -388,6 +401,8 @@ vlc_module_begin ()
> >  #ifdef __linux__
> >      add_integer ("dvb-satno", 0, SATNO_TEXT, SATNO_LONGTEXT, true)
> >          change_integer_list (satno_vlc, satno_user)
> > +    add_integer ("dvb-uncommitted", 0, UNCOMMITTED_TEXT,
> > UNCOMMITTED_LONGTEXT, true)
> > +        change_integer_list (uncommitted_vlc, uncommitted_user)
> >      add_integer ("dvb-tone", -1, TONE_TEXT, TONE_LONGTEXT, true)
> >          change_integer_list (auto_off_on_vlc, auto_off_on_user)
> >  #endif
> > diff --git a/modules/access/dtv/linux.c b/modules/access/dtv/linux.c
> > index b0b88be..acb9f88 100644
> > --- a/modules/access/dtv/linux.c
> > +++ b/modules/access/dtv/linux.c
> > @@ -772,21 +772,60 @@ known:
> >      unsigned satno = var_InheritInteger (d->obj, "dvb-satno");
> >      if (satno > 0)
> >      {
> > -        /* DiSEqC 1.0 */
> >  #undef msleep /* we know what we are doing! */
> > +
> > +        /* DiSEqC Bus Specification:
> > +
> > http://www.eutelsat.com/satellites/pdf/Diseqc/Reference%20docs/bus_spec.pdf 
> */
> > +
> > +        /* DiSEqC 1.1 */
> > +        struct dvb_diseqc_master_cmd uncmd;
> > +
> > +        /* DiSEqC 1.0 */
> >          struct dvb_diseqc_master_cmd cmd;
> > 
> > +        unsigned uncommitted = var_InheritInteger (d->obj,
> > "dvb-uncommitted");
> > +        if (uncommitted > 0)
> > +        {
> > +          uncommitted = (uncommitted - 1) & 3;
> > +          uncmd.msg[0] = 0xE0; /* framing: master, no reply, 1st TX */
> > +          uncmd.msg[1] = 0x10; /* address: all LNB/switch */
> > +          uncmd.msg[2] = 0x39; /* command: Write Port Group 1
> > (uncommitted) */
> > +          uncmd.msg[3] = 0xF0  /* data[0]: clear all bits */
> > +                       | (uncommitted << 2) /* LNB (A, B, C or D) */
> > +                       | ((voltage == SEC_VOLTAGE_18) << 1) /*
> > polarization */
> > +                       | (tone == SEC_TONE_ON); /* option */
> > +          uncmd.msg[4] = uncmd.msg[5] = 0; /* unused */
> > +          uncmd.msg_len = 4; /* length*/
> > +        }
> > +
> >          satno = (satno - 1) & 3;
> >          cmd.msg[0] = 0xE0; /* framing: master, no reply, 1st TX */
> >          cmd.msg[1] = 0x10; /* address: all LNB/switch */
> > -        cmd.msg[2] = 0x38; /* command: Write Port Group 0 */
> > +        cmd.msg[2] = 0x38; /* command: Write Port Group 0 (committed) */
> >          cmd.msg[3] = 0xF0  /* data[0]: clear all bits */
> > 
> >                     | (satno << 2) /* LNB (A, B, C or D) */
> >                     | ((voltage == SEC_VOLTAGE_18) << 1) /* polarization */
> >                     | (tone == SEC_TONE_ON); /* option */
> > 
> >          cmd.msg[4] = cmd.msg[5] = 0; /* unused */
> >          cmd.msg_len = 4; /* length*/
> > +
> >          msleep (15000); /* wait 15 ms before DiSEqC command */
> > +        if (uncommitted > 0)
> > +        {
> 
> Is there a reason why you need to check the same condition twice in a row?

Short:   Depends on logical vs. code elegance.
Shorter: No.

I'll squash first into second.
 
> > +          if (ioctl (d->frontend, FE_DISEQC_SEND_MASTER_CMD, &uncmd) < 0)
> > +          {
> > +              msg_Err (d->obj, "cannot send DiSEqC command: %m");
> > +              return -1;
> > +          }
> > +          /* Repeat uncommitted command */
> > +          uncmd.msg[0] = 0xE1; /* framing: master, no reply, repeated TX */
> > +          if (ioctl (d->frontend, FE_DISEQC_SEND_MASTER_CMD, &uncmd) < 0)
> > +          {
> > +              msg_Err (d->obj, "cannot send DiSEqC command: %m");
> > +              return -1;
> > +          }
> > +          msleep(125000); /* wait 125 ms before committed DiSEqC command
> > */
> > +        }
> >          if (ioctl (d->frontend, FE_DISEQC_SEND_MASTER_CMD, &cmd) < 0)
> >          {
> >              msg_Err (d->obj, "cannot send DiSEqC command: %m");

After I change code according to your comments, I'll resubmit patch.

Kind regards,
Zoran Turalija