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

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

[Steven Toth]

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.

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


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


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

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

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

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


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

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.

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

I welcome your feedback.

- Steve

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

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

[Patrick Boettcher]

Hi Steve,

On Tue, 9 Sep 2008, Steven Toth wrote:
> * 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?

Expanding the term "dtv" to digital television and then translate 
television into "seeing something somewhere which takes/took place 
somewhere else" I can agree with the dtv-prefix.

Still I think the term TV is used by a lot of people with different 
understandings. But except me nobody has seen the term dtv too 
restrictive, so I'm joining the majority.

On thing I almost forgot: You should add a bandwidth-thing as an integer, 
like the frequency/symbolrate.

For DVB-T we (DiBcom) can basicly run with any channel bandwidth, not only 
5,6,7,8 MHz. And some people are even using it...

Another example: DVB-SH is mentioning explicitly a 1.7 MHz bandwidth in 
the spec (default is 5MHz).

Please consider adapting it.

thanks,
Patrick.

--
   Mail: patrick.boettcher@desy.de
   WWW:  http://www.wi-bw.tfh-wildau.de/~pboettch/

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

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

[Steven Toth]

Patrick Boettcher wrote:
> Hi Steve,
> 
> On Tue, 9 Sep 2008, Steven Toth wrote:
>> * 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?
> 
> Expanding the term "dtv" to digital television and then translate 
> television into "seeing something somewhere which takes/took place 
> somewhere else" I can agree with the dtv-prefix.
> 
> Still I think the term TV is used by a lot of people with different 
> understandings. But except me nobody has seen the term dtv too 
> restrictive, so I'm joining the majority.
> 
> On thing I almost forgot: You should add a bandwidth-thing as an 
> integer, like the frequency/symbolrate.
> 
> For DVB-T we (DiBcom) can basicly run with any channel bandwidth, not 
> only 5,6,7,8 MHz. And some people are even using it...
> 
> Another example: DVB-SH is mentioning explicitly a 1.7 MHz bandwidth in 
> the spec (default is 5MHz).
> 
> Please consider adapting it.

I've added this to the list.

Thanks,

Steve

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

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

[Steven Toth]

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

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

[Steven Toth]

Steven Toth wrote:
> 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.


This is now done.


>>
>> * - 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 }
>> };

This is now done.

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

This is now done.

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

This has now changed, cleaner code, more readable and easier to maintain.

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

The variable length arrays now use a similar technique to the v4l2 and
i2c subsystems. QUERYMENU doesn't exist, and I'd welcome comments about
whether we need it for a first slice of the S2API in the kernel.

I think we can add this with zero API changes, new command types only.

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

EXT_CTRLS / QUERYMENU is not implemented, again I'd welcome feedback on
this.

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

- Steve


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