| Message ID | 20180925101434.20327-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add units to controls | expand |
On 09/25/2018 12:14 PM, Sakari Ailus wrote: > Add support for exponential bases, prefixes as well as units for V4L2 > controls. This makes it possible to convey information on the relation > between the control value and the hardware feature being controlled. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index ae083978988f1..23b02f2db85a1 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl { > __u32 reserved[2]; > }; > > +/* V4L2 control exponential bases */ > +#define V4L2_CTRL_BASE_UNDEFINED 0 > +#define V4L2_CTRL_BASE_LINEAR 1 I'm not really sure you need BASE_LINEAR. That is effectively the same as UNDEFINED since what else can you do? It's also weird to have this as 'base' if the EXPONENTIAL flag is set. I don't see why you need the EXPONENTIAL flag at all: if this is non-0, then you know the exponential base. > +#define V4L2_CTRL_BASE_2 2 > +#define V4L2_CTRL_BASE_10 10 > + > +/* V4L2 control unit prefixes */ > +#define V4L2_CTRL_PREFIX_NANO -9 > +#define V4L2_CTRL_PREFIX_MICRO -6 > +#define V4L2_CTRL_PREFIX_MILLI -3 > +#define V4L2_CTRL_PREFIX_1 0 I would prefer PREFIX_NONE, since there is no prefix in this case. I assume this prefix is only valid if the unit is not UNDEFINED and not NONE? Is 'base' also dependent on a valid unit? (it doesn't appear to be) > +#define V4L2_CTRL_PREFIX_KILO 3 > +#define V4L2_CTRL_PREFIX_MEGA 6 > +#define V4L2_CTRL_PREFIX_GIGA 9 > + > +/* V4L2 control units */ > +#define V4L2_CTRL_UNIT_UNDEFINED 0 > +#define V4L2_CTRL_UNIT_NONE 1 > +#define V4L2_CTRL_UNIT_SECOND 2 > +#define V4L2_CTRL_UNIT_AMPERE 3 > +#define V4L2_CTRL_UNIT_LINE 4 > +#define V4L2_CTRL_UNIT_PIXEL 5 > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC 6 > +#define V4L2_CTRL_UNIT_HZ 7 > + > + > /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ > struct v4l2_query_ext_ctrl { > __u32 id; > @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl { > __u32 elems; > __u32 nr_of_dims; > __u32 dims[V4L2_CTRL_MAX_DIMS]; > - __u32 reserved[32]; > + __u8 base; > + __s8 prefix; > + __u16 unit; > + __u32 reserved[31]; > }; > > /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ > @@ -1692,6 +1721,7 @@ struct v4l2_querymenu { > #define V4L2_CTRL_FLAG_HAS_PAYLOAD 0x00000100 > #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE 0x00000200 > #define V4L2_CTRL_FLAG_MODIFY_LAYOUT 0x00000400 > +#define V4L2_CTRL_FLAG_EXPONENTIAL 0x00000800 > > /* Query flags, to be ORed with the control ID */ > #define V4L2_CTRL_FLAG_NEXT_CTRL 0x80000000 > Regards, Hans
On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: > On 09/25/2018 12:14 PM, Sakari Ailus wrote: > > +/* V4L2 control unit prefixes */ > > +#define V4L2_CTRL_PREFIX_NANO -9 > > +#define V4L2_CTRL_PREFIX_MICRO -6 > > +#define V4L2_CTRL_PREFIX_MILLI -3 > > +#define V4L2_CTRL_PREFIX_1 0 > > I would prefer PREFIX_NONE, since there is no prefix in this case. > > I assume this prefix is only valid if the unit is not UNDEFINED and not > NONE? Why should it? The prefix is concerned with rescaling a value prior to presenting it to a user. Even a unitless quantity or a value of undefined unit can be reasonably scaled. Displaying a unit and scaling look like orthogonal concepts to me. > Is 'base' also dependent on a valid unit? (it doesn't appear to be) I'd argue it should not depend on a valid unit like the prefix. Helmut
On 10/01/2018 11:27 AM, Helmut Grohne wrote: > On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: >> On 09/25/2018 12:14 PM, Sakari Ailus wrote: >>> +/* V4L2 control unit prefixes */ >>> +#define V4L2_CTRL_PREFIX_NANO -9 >>> +#define V4L2_CTRL_PREFIX_MICRO -6 >>> +#define V4L2_CTRL_PREFIX_MILLI -3 >>> +#define V4L2_CTRL_PREFIX_1 0 >> >> I would prefer PREFIX_NONE, since there is no prefix in this case. >> >> I assume this prefix is only valid if the unit is not UNDEFINED and not >> NONE? > > Why should it? The prefix is concerned with rescaling a value prior to > presenting it to a user. Even a unitless quantity or a value of > undefined unit can be reasonably scaled. Displaying a unit and scaling > look like orthogonal concepts to me. What's the point? If I have a unit-less control with values 1-1000, then what would a prefix 'milli' tell me as a user? Why would 0.001-1 be better compared to 1-1000? Without a unit it is just an integer range and scaling is meaningless. > >> Is 'base' also dependent on a valid unit? (it doesn't appear to be) > > I'd argue it should not depend on a valid unit like the prefix. I think I agree with that, although I am dubious about the value of the base field as I commented on elsewhere. Regards, Hans
Hi! > Add support for exponential bases, prefixes as well as units for V4L2 > controls. This makes it possible to convey information on the relation > between the control value and the hardware feature being controlled. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Sounds good. > +/* V4L2 control exponential bases */ > +#define V4L2_CTRL_BASE_UNDEFINED 0 > +#define V4L2_CTRL_BASE_LINEAR 1 > +#define V4L2_CTRL_BASE_2 2 > +#define V4L2_CTRL_BASE_10 10 Do we also want to support logarithmic and 1/x? For example focus is usually in diopters and thats 1/meter... > +/* V4L2 control units */ > +#define V4L2_CTRL_UNIT_UNDEFINED 0 > +#define V4L2_CTRL_UNIT_NONE 1 > +#define V4L2_CTRL_UNIT_SECOND 2 > +#define V4L2_CTRL_UNIT_AMPERE 3 > +#define V4L2_CTRL_UNIT_LINE 4 > +#define V4L2_CTRL_UNIT_PIXEL 5 > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC 6 > +#define V4L2_CTRL_UNIT_HZ 7 And Hz is 1/second. Should we also have some support for ISO-sensitivity and f/ aperture numbers? Thanks, Pavel
Hi Sakari, On Fri, Sep 28, 2018 at 11:00 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 09/25/2018 12:14 PM, Sakari Ailus wrote: > > Add support for exponential bases, prefixes as well as units for V4L2 > > controls. This makes it possible to convey information on the relation > > between the control value and the hardware feature being controlled. > > Sorry for being late to the party. Thanks for the series. I think it has a potential to be very useful. Please see my comments below. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++- > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > index ae083978988f1..23b02f2db85a1 100644 > > --- a/include/uapi/linux/videodev2.h > > +++ b/include/uapi/linux/videodev2.h > > @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl { > > __u32 reserved[2]; > > }; > > > > +/* V4L2 control exponential bases */ > > +#define V4L2_CTRL_BASE_UNDEFINED 0 > > +#define V4L2_CTRL_BASE_LINEAR 1 > > I'm not really sure you need BASE_LINEAR. That is effectively the same > as UNDEFINED since what else can you do? It's also weird to have this > as 'base' if the EXPONENTIAL flag is set. > > I don't see why you need the EXPONENTIAL flag at all: if this is non-0, > then you know the exponential base. Or vice versa, we could remove UNDEFINED and LINEAR altogether and have the EXPONENTIAL flag actually signify the presence of a valid base? Besides that, "linear exponential base" just doesn't sound right or am I missing some basic maths? ;) Then we could actually have a LOGARITHMIC flag and it could reuse the same bases enum. > > > +#define V4L2_CTRL_BASE_2 2 > > +#define V4L2_CTRL_BASE_10 10 > > + > > +/* V4L2 control unit prefixes */ > > +#define V4L2_CTRL_PREFIX_NANO -9 > > +#define V4L2_CTRL_PREFIX_MICRO -6 > > +#define V4L2_CTRL_PREFIX_MILLI -3 > > +#define V4L2_CTRL_PREFIX_1 0 > > I would prefer PREFIX_NONE, since there is no prefix in this case. > > I assume this prefix is only valid if the unit is not UNDEFINED and not > NONE? > > Is 'base' also dependent on a valid unit? (it doesn't appear to be) > > > +#define V4L2_CTRL_PREFIX_KILO 3 > > +#define V4L2_CTRL_PREFIX_MEGA 6 > > +#define V4L2_CTRL_PREFIX_GIGA 9 > > + > > +/* V4L2 control units */ > > +#define V4L2_CTRL_UNIT_UNDEFINED 0 > > +#define V4L2_CTRL_UNIT_NONE 1 Hmm, what's the meaning of NONE? How does it differ from UNDEFINED? > > +#define V4L2_CTRL_UNIT_SECOND 2 > > +#define V4L2_CTRL_UNIT_AMPERE 3 > > +#define V4L2_CTRL_UNIT_LINE 4 > > +#define V4L2_CTRL_UNIT_PIXEL 5 > > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC 6 > > +#define V4L2_CTRL_UNIT_HZ 7 > > + > > + > > /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ > > struct v4l2_query_ext_ctrl { > > __u32 id; > > @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl { > > __u32 elems; > > __u32 nr_of_dims; > > __u32 dims[V4L2_CTRL_MAX_DIMS]; > > - __u32 reserved[32]; > > + __u8 base; > > + __s8 prefix; Should we make those bigger just in case, or leave some reserved fields around so we can make them bigger when we need it? Best regards, Tomasz
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index ae083978988f1..23b02f2db85a1 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl { __u32 reserved[2]; }; +/* V4L2 control exponential bases */ +#define V4L2_CTRL_BASE_UNDEFINED 0 +#define V4L2_CTRL_BASE_LINEAR 1 +#define V4L2_CTRL_BASE_2 2 +#define V4L2_CTRL_BASE_10 10 + +/* V4L2 control unit prefixes */ +#define V4L2_CTRL_PREFIX_NANO -9 +#define V4L2_CTRL_PREFIX_MICRO -6 +#define V4L2_CTRL_PREFIX_MILLI -3 +#define V4L2_CTRL_PREFIX_1 0 +#define V4L2_CTRL_PREFIX_KILO 3 +#define V4L2_CTRL_PREFIX_MEGA 6 +#define V4L2_CTRL_PREFIX_GIGA 9 + +/* V4L2 control units */ +#define V4L2_CTRL_UNIT_UNDEFINED 0 +#define V4L2_CTRL_UNIT_NONE 1 +#define V4L2_CTRL_UNIT_SECOND 2 +#define V4L2_CTRL_UNIT_AMPERE 3 +#define V4L2_CTRL_UNIT_LINE 4 +#define V4L2_CTRL_UNIT_PIXEL 5 +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC 6 +#define V4L2_CTRL_UNIT_HZ 7 + + /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ struct v4l2_query_ext_ctrl { __u32 id; @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl { __u32 elems; __u32 nr_of_dims; __u32 dims[V4L2_CTRL_MAX_DIMS]; - __u32 reserved[32]; + __u8 base; + __s8 prefix; + __u16 unit; + __u32 reserved[31]; }; /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ @@ -1692,6 +1721,7 @@ struct v4l2_querymenu { #define V4L2_CTRL_FLAG_HAS_PAYLOAD 0x00000100 #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE 0x00000200 #define V4L2_CTRL_FLAG_MODIFY_LAYOUT 0x00000400 +#define V4L2_CTRL_FLAG_EXPONENTIAL 0x00000800 /* Query flags, to be ORed with the control ID */ #define V4L2_CTRL_FLAG_NEXT_CTRL 0x80000000
Add support for exponential bases, prefixes as well as units for V4L2 controls. This makes it possible to convey information on the relation between the control value and the hardware feature being controlled. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)