Re: [RFCv2 PATCH 02/21] v4l2-ctrls: add unit string.

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Sakari,

On 01/24/2014 04:54 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jan 24, 2014 at 12:19:30PM +0100, Hans Verkuil wrote:
>> On 01/24/2014 11:35 AM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the patchset!
>>>
>>> On Mon, Jan 20, 2014 at 01:45:55PM +0100, Hans Verkuil wrote:
>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>>> index 0b347e8..3998049 100644
>>>> --- a/include/media/v4l2-ctrls.h
>>>> +++ b/include/media/v4l2-ctrls.h
>>>> @@ -85,6 +85,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>>>    * @ops:	The control ops.
>>>>    * @id:	The control ID.
>>>>    * @name:	The control name.
>>>> +  * @unit:	The control's unit. May be NULL.
>>>>    * @type:	The control type.
>>>>    * @minimum:	The control's minimum value.
>>>>    * @maximum:	The control's maximum value.
>>>> @@ -130,6 +131,7 @@ struct v4l2_ctrl {
>>>>  	const struct v4l2_ctrl_ops *ops;
>>>>  	u32 id;
>>>>  	const char *name;
>>>> +	const char *unit;
>>>
>>> What would you think of using a numeric value (with the standardised units
>>> #defined)? I think using a string begs for unmanaged unit usage. Code that
>>> deals with units might work with one driver but not with another since it
>>> uses a slightly different string for unit x.
>>
>> First of all, you always need a string. You don't want GUIs like qv4l2 to have
>> to switch on a unit in order to generate the unit strings. That's impossible to
>> keep up to date.
> 
> That's true when when you want to show that to the user, yes. But when you
> have an application which tries to figure out which value to put to the
> control, a numeric value is more convenient.
> 
> Kernel interfaces seldom use strings and that's for a good reason.

Well, actually, the kernel is in many places moving away from IDs to strings.
Headers full of IDs are surprisingly hard to maintain.

> 
>> In addition, private controls can have really strange custom units, so you want
>> to have a string there as well.
> 
> Good point as well.
> 
>> Standard controls can have their unit string set in v4l2-ctrls.c, just as their
>> name is set there these days, thus ensuring consistency.
>>
>> What I had in mind is that videodev2.h defines a list of standardized unit strings,
>> e.g.:
>>
>> #define V4L2_CTRL_UNIT_USECS "usecs"
>> #define V4L2_CTRL_UNIT_MSECS "msecs"
> 
> That's possible as well, but requires the user to e.g. use if (strcmp())
> ... instead of just plain switch (unit) { ... }.
> 
> I'd also very much prefer to stick to SI units and prefixes where applicable
> if we end up using strings. Combining the unit and prefix could make sense.
> 
>> and apps can do strcmp(qc->unit, V4L2_CTRL_UNIT_USECS) to see what the unit is.
>> If a driver doesn't use one of those standardized unit strings, then it is a
>> driver bug.
>>
>>> A prefix could be potentially nice, too, so ms and µs would still have the
>>> same unit but a different prefix.
>>
>> Can you give an example of a prefix? I don't really follow what you want to
>> achieve.
> 
> You use them in your own example above. :-)
> 
> <URL:http://en.wikipedia.org/wiki/SI_prefix>
> 

Ah, that sort of prefix.

I don't think the prefix makes sense, for a number of reasons: first I think it
makes life even harder for applications, since they now have to factor in a SI
prefix as well. Secondly, what to do with units like km/s? There are two prefixes
there (e.g. mm/usecs is also a valid speed representation).

I think that for the initial version we just add the unit string as that is needed
anyway. There are more than enough reserved fields available to add a unit_id field
later, but frankly, I'd like to see some real-life use-cases first.

Regards,

	Hans