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

[Johannes Stezenbach]

On Wed, Mar 01, 2006, Marcel Siegert wrote:
> 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?

fe_caps_t doesn't have FE_CAN_DVB_T etc. so why should it have
FE_CAN_DVB_S2? This is inconsistent.

> >  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. 

FE_GET_INFO returns the default frontend type, an application
which wants to use the new features sees the
FE_HAS_EXTENDED_CAN_VALUES flag (bad name, IMHO), does
FE_GET_EXTENDED_INFO (or maybe better a more generic FE_GET_CAPS
in the style of V4 API DVB_VIDEO_GET_CAPS?), and so discovers
the supported FE types, which it can then switch via FE_SET_STANDARD.

Maybe I just didn't get how you suggest apps find out what
FE types are supported?

> > 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

Could be mapped in dvb_frontend.c:dvb_frontend_ioctl(FE_GET_INFO).

But, Oops, doesn't work for another reason: old binaries would
expect a different value for FE_QPSK than new ones.
Next try:

typedef enum fe_type {
      FE_QPSK, // legacy, do not use in new applications
      FE_QAM,  // legacy, do not use in new applications
      FE_OFDM, // legacy, do not use in new applications
      FE_ATSC   = 3,
      FE_DVB_S  = (1 << 2),
      FE_DVB_C  = (1 << 3),
      FE_DVB_T  = (1 << 4),
      FE_DVB_S2 = (1 << 6)
} fe_type_t;

A bit ugly, but would work for both old apps and reporting supported
standards for new apps.


> > > +/* 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.

You are right with your doubts ;-/

gcc-3.x info pages are incomplete, but gcc-4.0 info pages say:

   * `The integer type compatible with each enumerated type (C90
     6.5.2.2, C99 6.7.2.2).'

     Normally, the type is `unsigned int' if there are no negative
     values in the enumeration, otherwise `int'.  If `-fshort-enums' is
     specified, then if there are negative values it is the first of
     `signed char', `short' and `int' that can represent all the
     values, otherwise it is the first of `unsigned char', `unsigned
     short' and `unsigned int' that can represent all the values.

     On some targets, `-fshort-enums' is the default; this is
     determined by the ABI.

The System V generic ABI http://www.caldera.com/developers/gabi/
says enums sizes are processor specific, the psABI for i386 and MIPS
say enums have the same size as int. For other platforms I don't know
but according to google ARM seems to have -fshort-enums as default.
Sigh...

> > >  struct dvb_frontend_parameters {
...
> > > +	/* 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 

I'd say forget about FE_GET_EVENT.
You don't need the type field for FE_SET_FRONTEND,
and FE_GET_FRONTEND should return values according to the type
selected with FE_SET_STANDARD.

> > > +/**
> > > + * 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.

I think it's more useful for the lib to query for specific
API/driver capabilites than for a version number.


Johannes