[linux-dvb] [PATCH] add DVB-S2 support to frontend.h

[Marcel Siegert]

On Wednesday 01 March 2006 20:08, Johannes Stezenbach wrote:
> On Mon, Feb 27, 2006, Marcel Siegert wrote:
> >  typedef enum fe_caps {
> >  	FE_IS_STUPID			= 0,
> > @@ -62,12 +66,13 @@
> >  	FE_CAN_HIERARCHY_AUTO		= 0x100000,
> >  	FE_CAN_8VSB			= 0x200000,
> >  	FE_CAN_16VSB			= 0x400000,
> > +	FE_CAN_DVB_S2			= 0x1000000,
> 
> This doesn't fit in here. Maybe we can do:
what doesn't fit there?
how will you act on combo frontends that use e.g. DVB-T and DVB-C?
what type is reflected in fe_type_t then?

>  typedef enum fe_type {
>        // values 0 to 3 are reserved for binary compatibilty with old
>        // API version
>        FE_DVB_S  = (1 << 2),
>        FE_DVB_C  = (1 << 3),
>        FE_DVB_T  = (1 << 4),
>        FE_ATSC   = (1 << 5),
>        FE_DVB_S2 = (1 << 6)
>  } fe_type_t;
these combined? hmm, possible, but this would cause frontend drivers to return FE_QPSK | FE_DVB_S 
in case an old application will use the new dvb-core or new apps have to rely on both values because
they don't now if kernel dvb-core is an old version. 

> and use that in a GET_CAPS return value so apps can see what
> values are legal to pass to FE_SET_STANDARD
> 
> #define FE_QPSK   FE_DVB_S // source compatibility
> #define FE_QAM    FE_DVB_C
> #define FE_OFDM   FE_DVB_T
> 
> #define FE_QPSK_OLD 0 // for binary compatibility, handled in drivers,
> #define FE_QAM_OLD  1 // should be defined in dvb_frontend.h
> #define FE_OFDM_OLD 2
> #define FE_ATSC_OLD 3
so, we have to overwork all drivers to have FE_QPSK changed to FE_QPSK_OLD? or am i missing a point

> 
> > +/* backport from dvb-api v4 - adapted a bit to have the last value at bit 31 so we */
> > +/* guarantee that the enum will normally have 32 bits size */
> 
> What does the "bit 31" comment mean? I think gcc guarantees that
> sizeof(enum foo) == sizeof(int), unless you compile with -fshort-enums
> (which the kernel doesn't support anyway, I think).
should we rely on what gcc developers maybe implement(ed) or optimize?
what if someone tries to use the intel c/c++ compiler collection?

on enumeration constants (like we do have for the backported v4 enums) 
you're absolutely right. but read what i found on 
http://david.tribble.com/text/cdiffs.htm#C99-enum-type

it mainly describes differences between C and C++ but some reasons also just on 
c enumerations that may have an effect.
my testcase with gcc shows same sizes.

if we should trust gcc, i'll change that.

> > +/*! describes the available forward error correction code rates. */
> > +enum dvb_fe_code_rate {
> > +	DVB_FE_FEC_NONE = (1 << 0),
> > +	DVB_FE_FEC_1_2  = (1 << 1),
> > +	DVB_FE_FEC_2_3  = (1 << 2),
> > +	DVB_FE_FEC_3_4  = (1 << 3),
> > +	DVB_FE_FEC_4_5  = (1 << 4),
> > +	DVB_FE_FEC_5_6  = (1 << 5),
> > +	DVB_FE_FEC_6_7  = (1 << 6),
> > +	DVB_FE_FEC_7_8  = (1 << 7),
> > +	DVB_FE_FEC_8_9  = (1 << 8),
> > +	DVB_FE_FEC_3_5	= (1 << 9),
> > +	DVB_FE_FEC_9_10	= (1 << 10),
> > +	DVB_FE_FEC_AUTO = (1 << 31)
> > +};
> > +
> > +/*! describes the available modulation types. */
> > +enum dvb_fe_modulation {
> > +	DVB_FE_MOD_QPSK		= (1 << 0),
> > +	DVB_FE_MOD_QAM_16	= (1 << 1),
> > +	DVB_FE_MOD_QAM_32	= (1 << 2),
> > +	DVB_FE_MOD_QAM_64	= (1 << 3),
> > +	DVB_FE_MOD_QAM_128	= (1 << 4),
> > +	DVB_FE_MOD_QAM_256	= (1 << 5),
> > +	DVB_FE_MOD_QAM_AUTO	= (1 << 6),
> > +	DVB_FE_MOD_2_VSB	= (1 << 7),
> > +	DVB_FE_MOD_4_VSB	= (1 << 8),
> > +	DVB_FE_MOD_8_VSB	= (1 << 9),
> > +	DVB_FE_MOD_16_VSB	= (1 << 10),
> > +	DVB_FE_MOD_8_PSK	= (1 << 11),
> > +	DVB_FE_MOD_BPSK		= (1 << 12),
> > +	DVB_FE_MOD_MOD_UNKNOWN	= (1 << 31)
> > +};
> > +
> > +/*! describes common supported frontend capabilities. */
> > +enum dvb_fe_common_cap {
> > +	DVB_FE_CAN_INVERSION_AUTO   = (1 << 0), /*!< fixme */
> > +	DVB_FE_CAN_RECOVER          = (1 << 1), /*!< frontend can recover from a cable unplug automatically */
> > +	DVB_FE_CAN_MUTE_TS          = (1 << 2), /*!< frontend can stop spurious TS data output */
> > +	DVB_FE_SUP_HIGH_LNB_VOLTAGE = (1 << 31), /*!< fixme, frontend can deliver higher lnb voltages */
> > +};
> > +
> > +/*! describes other available, frontend type dependent capabilities. */
> > +enum dvb_fe_other_cap {
> > +	DVB_FE_CAN_TRANSMISSION_MODE_AUTO = (1 << 0), /*!< fixme (DVB-T specific) */
> > +	DVB_FE_CAN_BANDWIDTH_AUTO         = (1 << 1), /*!< fixme (DVB-T specific) */
> > +	DVB_FE_CAN_GUARD_INTERVAL_AUTO    = (1 << 2), /*!< fixme (DVB-T specific) */
> > +	DVB_FE_CAN_HIERARCHY_AUTO         = (1 << 31), /*!< fixme (DVB-T specific) */
> > +};
> > +
> > +/**
> > + *  this struct will be filled by the FE_GET_EXTENDED_INFO ioctl.
> > + *  it is a extension to the normal frontend capabilities and provided
> > + *  if the dvb_fe_info.caps is having the FE_HAS_EXTENDED_CAN_VALUES bit set.
> > + */
> > +struct dvb_fe_caps_extended {
> > +	enum dvb_fe_code_rate  caps_fec;        /*!< supported fecs */
> > +	enum dvb_fe_modulation caps_modulation; /*!< supported modulations */
> > +	enum dvb_fe_common_cap caps_common;     /*!< supported common capabilities */
> > +	enum dvb_fe_other_cap  caps_other;      /*!< supported other capabilities*/
> > +};
> 
> I'm not really happy with this. I think it is confusing if we have
> e.g. both enum dvb_fe_code_rate and fe_code_rate_t.
> Apparently it confuses you, too, as struct dvb_dvbs2_parameters
> uses the old one which lacks the DVB-S2 modulation types...
oh, good point - fixed :)
my suggestion was to get started that applications that will be reworked for dvb-s2
also do a small preperation for using v4 api without having to change everything then.
maybe it is pointing to the wrong direction. we also can extend the old typedef enums 
 
> Also, IMHO some of these are caps irrelevant for applications as
> dvb-core emulates missing hardware features.
> 
> Lastly, the V4 API defines the *GET_CAPS ioctls so that they
> are extensible in a way that retains binary compatibilty.
> 
> BTW, is there any application which uses the capabilites?
> I think apps expect that e.g. a DVB-T frontend can do
> all FECs required by the DVB-T standard.
i do not know - why do we provide information when you think 
that nobody is using them? 
 
> 
> >  struct dvb_frontend_parameters {
> > +{
> >  	__u32 frequency;     /* (absolute) frequency in Hz for QAM/OFDM/ATSC */
> >  			     /* intermediate frequency in kHz for QPSK */
> >  	fe_spectral_inversion_t inversion;
> >  	union {
> > -		struct dvb_qpsk_parameters qpsk;
> > -		struct dvb_qam_parameters  qam;
> > -		struct dvb_ofdm_parameters ofdm;
> > +		struct dvb_dvbs_parameters qpsk;
> > +		struct dvb_dvbc_parameters  qam;
> > +		struct dvb_dvbt_parameters ofdm;
> >  		struct dvb_vsb_parameters vsb;
> >  	} u;
> > +	/* next is new as previous union was just kept to gain source/binary backwards compatibility */
> > +	fe_type_t type;	     /* select which kind of parameters is within the union u. from frontendtype */
> > +        struct dvb_frontend_parameters_internal* extended_parameters;
> >  };
> 
> Wasn't Felix' reasoning that the type field is unnecessary
> because of FE_SET_STANDARD?
the type field is used to know what is contained (in my tree already corrected)
within the dvb_frontend_parameters_extended. it is dvb-s2 for now but it could be dvb-c2 or whatever.
i had put that flag within the struct to have the possibility to examine WHAT is within if i do call 
FE_GET_EVENT that returns also dvb_frontend_parameters. but these can differ from FE_SET_MODE
can't they? if not - you're right.

i'll think about that topic again 
> 
> > +struct __dvb_frontend_event_old {
> > +	fe_status_t status;
> > +	struct __dvb_frontend_parameters_old parameters;
> > +};
> >  
> >  struct dvb_frontend_event {
> > -	fe_status_t status;
> > -	struct dvb_frontend_parameters parameters;
> > +	fe_status_t status;					/* if Bit 7 (0x80) FE_EXTENDED_TP is SET - you may safely query type and */
> > +	struct __dvb_frontend_parameters_old parameters;        /* the extended_parameters */
> > +	fe_type_t type; 					/* reflect what type of parameters is contained */
> > +	struct dvb_frontend_parameters* extended_parameters;
> >  };
> 
> Holger Waechtler pointed out in earlier discussions that the
> data passed in struct dvb_frontend_event is totally irrelevant,
> as it might have been in the event queue for some time and
> is thus stale.
> 
> Applications should:
> - open the frontend device with O_NONBLOCK
> - poll() for events
> - FE_GET_EVENT all pending events to clear the POLLPRI status,
>   and throw them away
> - FE_READ_STATUS etc. to get current information
>
> I suggest that we leave FE_GET_EVENT alone and document the
> above usage.
agreed, that would remove also the fe_type_t from dvb_frontend_parameters.
i'll fix that within my tree.
 
> > +/**
> > + * Struct to keep the api version which can be queried
> > + * via the FE_GET_API_VERSION ioctl.
> > + * Need for having encapsulation libraries in userspace
> > + * possible.
> > + */
> > +struct dvb_api_version {
> > +	__u16	major;
> > +	__u16	minor;
> > +};
> 
> How would an application use this? I think it is
> unnecessary, FE_HAS_EXTENDED_CAN_VALUES it enough.
on freenode irc channel #linuxtv / available as log on linuxtv.org  
we were talking about having a userspace library that can encapsulate
api versions and frontend issues through a stable consistant userapp / userspace lib
interface. so even a change from api v3 to v4 or something else, could be wrapped there
without having a user treated to change his application - just do a relink.
therefore you must be able to query the dvb_api_version. (or in this case the frontend_api_version)
maybe this must be done in dvb-core itself - i am not exactly sure on that.

best regards
marcel