Re: Discussion: How to deal with radio tuners which can tune to multiple bands

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 05/26/2012 06:40 PM, Hans Verkuil wrote:
> On Sat May 26 2012 18:02:34 Hans de Goede wrote:
>> Hi,
>>
>> On 05/24/2012 09:12 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 05/24/2012 05:00 PM, Hans Verkuil wrote:
>>>>> I think / hope that covers everything we need. Suggestions ? Comments ?
>>>>
>>>> Modulators. v4l2_modulator needs a band field as well. The capabilities are
>>>> already shared with v4l2_tuner, so that doesn't need to change.
>>>
>>> Ah, yes modulators, good one, ack.
>>>
>>> Manjunatha, since the final proposal is close to yours, and you already have
>>> a patch for that including all the necessary documentation updates, can I ask
>>> you to update your patch to implement this proposal?
>>>
>>
>> So I've been working a bit on adding AM support to the tea575x driver using
>> the agreed upon API, some observations from this:
>>
>> 1) There is no way to get which band is currently active
>
> Huh? Didn't G_TUNER return the current band? That's how I interpreted the
> proposal. G_TUNER returns the available bands in capabilities and the current
> band and its frequency range. You want to find the frequency range of another
> band you call have to call S_TUNER first to select that other band, and then
> G_TUNER to discover its range.

Ah, we misunderstood each other there, I thought G_TUNER would honor the band
passed in and return info on that band.

> That also solves case 2. No need for an extra band in v4l2_frequency.

Right, the downside to this is that there is no way to just enumerate things
without actually changing anything. It would be nice if for example v4l2-ctl
could lists all bands including ranges as a 100% read-only operation.

Note I'm ok with the way you propose to handle things, just pointing out
one (obvious) shortcoming of doing things this way. I think it is a short coming
we can live with, but we should be aware of it.

>> This is IMHO a big problem, a GUI radio app will quite likely when it starts get
>> all the current settings and display them without modifying any settings, so
>> it needs a way to find out which band is active.
>>
>> 2) What if first a band aware radio app sets a non default band, and then a
>> non band aware radio app comes along, it does a g_tuner on the default-band,
>> using the lo / high freq-s to build its UI, then the users picks a frequency,
>> and the app does a s_freq, and the result is a frequency outside of what the
>> app thinks are the lo / high freq limits because a different band is active ->
>> not good.
>>
>> So I think we need to slightly modify the proposal, esp. to deal with 1), 2)
>> is a corner case and not really all that important on its own IMHO.
>>
>> I suggest fixing 1) by not only adding a band field to v4l2_tuner, so that
>> the different ranges for different bands can be queried, but also adding
>> a band field to v4l2_frequency, so that the current active band can be
>> reported by g_frequency. Once we make g_frequency report the active band,
>> it makes sense to make s_frequency set the active band.
>
> I don't really like this. You run into the same weird situation with G_TUNER
> that I did (as that was one of my ideas as well) where you set band to e.g.
> the weather band and get back the corresponding frequency range, but the other
> fields like signal and rxsubchans still refer to the *current* band. That's
> confusing and not logical.

Right, notice how my proposal says that signal and rxsubchans will always
report 0 except when doing a g_tuner for the active band. This is not ideal,
but IMHO better then not being able to do read-only enumeration of the bands.

>> We would then need no changes to s_tuner at all, it will still only have
>> audmode as writeable setting and thus *not* set the active band. Effectively
>> s_tuner would just completely ignore the passed in band. Keeping audmode as
>> a global (not band specific) setting, and likewise g_tuner would always
>> return CAP_STEREO stereo if some bands are stereo capable.
>>
>> This also nicely fixes 2). Since the reserved fields should be 0, so a
>> s_frequency by a non band aware app will set the band to the default band. >>
>> ###
>>
>> So here is a new / amended version of my band proposal:
>>
>> 1) Introduce the concept of bands, for radio tuners only
>>
>> 2) Define the following bands:
>>
>> #define V4L2_BAND_DEFAULT       0
>> #define V4L2_BAND_FM_EUROPE_US  1       /* 87.5 Mhz - 108 MHz */
>> #define V4L2_BAND_FM_JAPAN      2       /* 76 MHz - 90 MHz */
>> #define V4L2_BAND_FM_RUSSIAN    3       /* 65.8 MHz - 74 MHz */
>> #define V4L2_BAND_FM_WEATHER    4       /* 162.4 MHz - 162.55 MHz */
>> #define V4L2_BAND_AM_MW         5
>>
>> 3) radio tuners indicate if they understand any of the non default bands
>> with the following tuner caps:
>>
>> #define V4L2_TUNER_CAP_BAND_FM_EUROPE_US        0x00010000
>> #define V4L2_TUNER_CAP_BAND_FM_JAPAN    	0x00020000
>> #define V4L2_TUNER_CAP_BAND_FM_RUSSIAN  	0x00040000
>> #define V4L2_TUNER_CAP_BAND_FM_WEATHER  	0x00080000
>> #define V4L2_TUNER_CAP_BAND_AM_MW       	0x00100000
>>
>> A (radio) tuner should always support RADIO_BAND_DEFAULT, so there is no
>> capability flag for this
>>
>> 4) Add a band field to v4l2_tuner, apps can query the exact rangelow and
>> rangehigh values for a specific band by doing a g_tuner with band set to
>> that band. All v4l2_tuner fields returned by g_tuner will be independent
>> of the selected band (iow constant) except for: rangelow, rangehigh,
>> rxsubchans and signal.
>> 4a) rangelow, rangehigh will be the actual values for that band
>> 4b) rxsubchans and signal will be 0 if a g_tuner is done for a band different
>> then the active band, for the active band they will reflect the actual values.
>
> So I would do this as:
>
> 4) Add a band field to v4l2_tuner. Calling g_tuner will set this to the
> current band. You change it by calling s_tuner. CAP_STEREO and audmode are
> global properties, not per-band. CAP_STEREO really refers to whether the
> hardware can do stereo at all.

Right, I can see the logic in this, and I think it is an elegant solution in
a way. But as said that looses read only enumeration of the band ranges, which
I think is something which we should allow...

I'm happy that we agree that CAP_STEREO and audmode shoul be global and not per
band :)

>> 5) s_tuner will be completely unchanged, the band field will not influence
>> it, audmode will be a per tuner global, not a per band value
>
> Drop this.
>
>> 6) Add a band field to v4l2_frequency, on a g_frequency this will reflect the
>> current band, on a s_frequency this will set the current band
>
> Drop this.
>
>> 7) Doing a VIDIOC_S_HW_FREQ_SEEK will seek in the currently active band,
>> iow the band last set by a s_frequency call, this matches existing behavior where
>> the seek starts at the currently active frequency (so the frequency set by the
>> last s_frequency call, or the frequency from the last seek).
>
> 5) Doing a VIDIOC_S_HW_FREQ_SEEK will seek in the currently active band,
> iow the band last set by a s_tuner call.
>
> Two fewer items on this list :-)

He he, I'm ok with either way, but I still have a slight preference to adding
a band field to v4l2_frequency because of the enumeration issue.

Regards,

Hans