Re: [linux-dvb] S2API - Code review and suggested changes (1)

[Steven Toth <stoth@linuxtv.org>]

Steven Toth wrote:
> 
> Hello!
> 
> Here's the feedback collected from various people. Thank you! I've 
> indented their comments and replied directly inline but more suggestions 
> and potential changes.
> 
> * Johannes:
> *
> * OK can't help myself to give a few techical remarks:
> *
> * - I'm not sure because I don't have a 64bit platform but I
> *   think tv_property_t has different size on 64bit due to
> *   alignment? Please check, to avoid compat_ioctl
> *
> * - typedefs? hm, I guess to match the style of the existing code...
> 
> Looking at the CodingStyle section on typedefs, it tries to discourage 
> their use.
> It probably makes sense to switch to result structs.

Done.

> 
> tv_property_t type becomes struct dtv_property.
> 
> *
> * - why both FE_SET_PROPERTY/FE_GET_PROPERTY and
> * TV_SET_FREQUENCY/TV_GET_FREQUENCY etc?
> *
> 
> If I understand correctly then I think the suggestion is 
> FE_SET_PROPERTY(TV_FREQUENCY, ...)
> makes more sense. Han's also touched on this. I think I agree, removing 
> the notion of SET/GET
> from the command name is cleaner, less confusing.
> 
> * - instead of "typedef tv_property_t tv_properties_t[16];" and the
> *   TV_SEQ_* things why not
> *      typedef struct {
> *          __u32 num;
> *          tv_property_t *props;
> *      } tv_properties_t;
> *    and then copy_from_user() an arbitrary number of properties?
> *    But its all a matter of taste and if it works is find by me.
> 
> Perfect sense, I'll change this, it will become
> 
>       struct dtv_properties {
>           __u32 num;
>           dtv_property *props;
>       };


I varied this slightly to accomodate some reserved fields, but it's 
done. The 16 command restriction is removed.

I've placed #define in frontend.h making a 64 command restriction, else 
the handing in dvb_frontend.c could become unsafe. This matches similar
limits placed on the i2c-dev.c and video-ioctl.c similar variable length
handling schemes.


> 
> 
> * Patrick:
> *
> * I think it would be better to change TV_ to FE_ (because TV is by
> * far not
> * the only application for linux-dvb) , but this is a very unimportant
> * detail.
> 
> A few people prefer dtv_ and DTV_, rather than tv_ and TV_. is fe_
> and FE_ still important to you?


Done.


> 
> 
> * Hans:
> *
> * I noticed that the properties work very similar as to how extended
> * controls work in v4l:
> * http://www.linuxtv.org/downloads/video4linux/API/V4L2_API/spec-si
> * ngle/v4l2.html#VIDIOC-G-EXT-CTRLS
> *
> * It might be useful to compare the two.
> 
> Yes - similar concepts, I'll read further.
> 
> Last year, Mauro has suggested we also consider the VIDIOC_QUERYCTRL, 
> VIDIOC_QUERYMENU ioctls,
> giving the applications to ability to query supported functions on a 
> given demodulator.
> 
> *
> * I would recommend adding a few reserved fields, since that has
> * proven to be very useful in v4l to make the API more future proof.

Done.

> 
> Good point, __u32 and __u64's could be added.
> 
> struct dtv_property {
>     __u32 cmd;
>     __u64 reserved1;
>     __u64 reserved2;
>     __u64 reserved3;
>     __u64 reserved4;
>     union {
>         __u32 data;
>         struct {
>             __u8 data[32];
>             __u32 len;
>         } buffer;
>     } u;
> };
> 
> Question: Is their significant penalties for switching completely from 
> __u32 to __u64 now?
> Is this something that should be done completely in this struct, or are 
> __u32 types less
> problematic?


This remains unanswered.


> 
> This feels perhaps a little too heavy handed, I'd welcome comments on 
> the types and
> number of entries.
> 
> *
> * Also: is setting multiple properties an atomic action? E.g. if one
> * fails, are all other changes rolled back as well? And how do you give
> * the caller feedback on which property in the list failed? Will there
> * also be a TRY_PROPERTIES variant which just checks for correctness
> * without actually setting it? How do you query/test whether a
> * device has certain properties?
> 
> Atomic vs non-atomic.
> 
> That's a level of complexity that leads easily to confusion, and we 
> should avoid. The code we have working today assumes that that 
> set_frontend() on the demodulators is called when the TV_SET_FREQUENCY 
> or TV_SEQ_COMPLETE are passed. I think removing the notion of begin and 
> end sequence, in favour of short structures is less confusing and less 
> code to maintain in the dvb-core.
> 
> What's does this translate into for application developers? Well, a tune 
> request may look something like:
> 
> properties {
>     { MODULATION, QAM64)}
>     { INVERSION, ON)}
>     { SET_FREQUENCY, 474000000)}
>     { TUNE }
> };
> 
> All of the commands prior to TUNE are cache in the dvb-core, TUNE 
> explicitly results in a call to set_frontend();
> 
> The odd item to note is satellite tuning, where certain commands need to 
> be done immediatly. SET_VOLTAGE or SET_TONE, or DiSEqC related commands.
> 
> So, passing SET_VOLTAGE commands would likely execute immediately. I 
> don't know whether this is a good or bad thing, without spending a lot 
> of time reviewing our existing dvb-core code. In some respects, the 
> current in-kernel API expects multiple ioctls to occurs for satellite 
> tuning to take effect. This (hopefully) will turn that into a single ioctl.
> 
> Summary: This would allow the applications to call set/get properties on 
> cache values, without them being committed to hardware, or directly 
> committed depending on the type of tuning system and delivery system 
> being used. An Important fact for any documentation.

Partially done, the notion of SEQ has been removed. The switch to remove 
GET/SET is remaining.

> 
> *
> * Do you need separate get and set commands? Why not either set or get a
> * list of properties, and setting them implies an automatic SEQ_COMPLETE
> * at the end. By having a variable length array of properties you do not
> * need to set the properties in multiple blocks of 16, so that
> * simplifies the API as well.
> 
> Correct, this is going to change to a variable length array.

Done, as mentioned above.

> 
> *
> * As I said, extended controls in v4l do something very similar.
> * I thought about the extended controls a great deal at the time, so
> * it might provide a handy comparison for you.
> 
> Thanks.
> 
> * Christophe:
> *
> * P.S.
> * 1) imho, DTV_ prefix would make more sense.
> 
> Understood.
> 
> * Andreas:
> *
> * Regarding the code:
> * 1) What's TV_SEQ_CONTINUE good for? It seems to be unused.
> 
> It was going to allow unforseen long command structure to be passed to 
> the kernel. It's since been superseeded and will be removed.
> 
> *
> * 2) Like Christophe I'd prefer to use DTV_ and dtv_ prefixes.
> 
> Understood.
> 
> *
> * 3) Did you mean p.u.qam.modulation below? Also, p.u.qam.fec_inner is
> * missing.
> *
> * +        printk("%s() Preparing QAM req\n", __FUNCTION__);
> * +        /* TODO: Insert sanity code to validate a little. */
> * +        p.frequency = c->frequency;
> * +        p.inversion = c->inversion;
> * +        p.u.qam.symbol_rate = c->symbol_rate;       
> * +        p.u.vsb.modulation = c->modulation;
> 
> 
> Yes, typo, thanks for mentioning this. I'll fix this.


Not done yet.


> 
> 
> *
> * 4) About enum tv_cmd_types:
> *
> * SYMBOLRATE -> SYMBOL_RATE?
> * INNERFEC -> INNER_FEC (or FEC)?

Done.

> 
> OK, this was inconsistent. This will change to SYMBOL_RATE and INNER_FEC.
> 
> *
> * The Tone Burst command got lost (FE_DISEQC_SEND_BURST). How about
> * TV_SET_TONE_BURST?
> *
> * FE_ENABLE_HIGH_LNB_VOLTAGE got lost, too.
> *
> * Which old ioctls should be considered as obsolete? Do you plan to
> * add a tv_cmd for every old ioctl?
> 
> SET_TONE and SET_VOLTAGE was added late last week, and appear to be 
> working correctly. If I've miss-understood the implementation then we 
> can review/refine again until it makes sense.
> 
> 
> * Janne:
> *
> * I have also a slightly preference for DTV/dtv as prefix but it's not
> * really important.
> 
> Understood.
> 
> *
> *
> * >> 16 properties per ioctl are probably enough but a variable-length
> * >> > > property array would be safe. I'm unsure if this justifies a more
> * >> > > complicate copy from/to userspace in the ioctls.
> * > >
> * > > Johannes suggested we lose the fixed length approach and instead pass
> * > > in struct containing the number of elements... I happen to like this,
> * > > and it removed an unnecessary restriction.
> *
> * That's the same V4L2 handles the extended controls. For reference look
> * at VIDIOC_[G|S|TRY]_EXT_CTRLS in v4l2-ioctl.c.
> 
> 
> I think makes a lot of sense (when used along with _QUERYMENU). It 
> allows application developers to build tools like v4l2ctl which can 
> tweak frontend behaviour.
> 
> I don't know whether change dtv_property to match v4l2_ext_control makes 
> much sense, but the spirit of the API is strong, and should probably be 
> carried forward into any new digital TV API.
> 
> Let me look into this.

No investigation done yet.

> 
> Summary, what do we do next?
> 
> I'm going to make some patches for the structure changes, reserved 
> fields, typedefs, nomenclature and generate a tree (and tune.c) for 
> review/test.

That's done.

> 
> That will leave the EXT_CTRLS discussion for further debate, and any 
> other items that are raised during this review and round of discussions.
> We will address these in round 2.

Todo, along with bandwidth suggestions form Patrick.

> 
> I welcome your feedback.
> 
> - Steve
> 


_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb