Re: [PATCH 0/5] Add units to controls

[Helmut Grohne <helmut.grohne@intenta.de>]

On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote:
> This set adds a few things to the current control framework in terms of
> what kind of information the user space may have on controls. It adds
> support for units and prefixes, exponential base as well as information on
> whether a control is linear or exponential, to struct v4l2_query_ext_ctrl.

That's a great improvement. Being able to give meaning to controls is
great. However, I see two significant weaknesses in the approach being
taken:

1. There are a number of controls whose value is not easily described as
   either linear or exponential. I'm faced with at least two controls
   that actually are floating point numbers. One with two bits for the
   exponent and one (strange) bit for the mantissa (no joke) and another
   with three bits for the exponent and four bits for the mantissa.
   Neither can suitably be represented.

   Since the value ranges are small for the cases I mentioned, I looked
   into using an integer menu. The present approach does not allow for
   replacing an integer with an integer menu though. Each control id has
   a fixed type. I'm not sure how to solve this.

2. The present implementation places the responsibility of assigning
   units and scales into drivers. A number of controls (e.g.
   V4L2_CID_EXPOSURE_ABSOLUTE, V4L2_CID_AUDIO_LIMITER_RELEASE_TIME,
   V4L2_CID_PIXEL_RATE, ...) clearly state the scale and unit in the
   documentation. Having each and every driver set units and scales in
   the documented way will lead to duplication and buggy code. Having
   each driver choose unit and scale will lead to inconsistency. It
   would be very good to have a mechanism that puts the framework in
   charge of maintaining units and scales for the standard control ids.

   What is a consumer supposed to do with a control that changes unit?
   The algorithm expected e.g. a duration. It might be able to convert
   to pixels, but what should it do if it gets back amperes? I argue
   that the unit should be a property of the control id and be
   documented (i.e. what is done now). If it is reported via an ioctl,
   the framework needs to be in charge of setting it.

   The story is much different for scales. Scaling the value up and down
   is something consumers can reasonably be expected to do. It allows
   shifting the value range such that the relevant values can be fully
   represented. Allowing drivers to change scales is much more
   reasonable. Still the framework should provide a default such that
   most drivers should not need any update.

I acknowledge that these expectations are high and see that they're
partially covered in your later remarks.

> The smiapp driver gains support for the feature. In the near term, some
> controls could also be assigned the unit automatically. The pixel rate,
> for instance. Fewer driver changes would be needed this way. A driver
> could override the value if there's a need to.

I believe that in the interest of keeping maintenance cost low, this
should happen rather sooner than later. Just even adding the support to
smiapp seems wrong when it would be possible to have the framework do
the work.

> I think I'll merge the undefined and no unit cases. Same for the
> exponential base actually --- the flag can be removed, too...

I'm not sure I understand. It reads like you are going to revert a
significant part of the patch.

> Regarding Ricardo's suggestion --- I was thinking of adding a control flag
> (yes, there are a few bits available) to tell how to round the value. The
> user could use the TRY_EXT_CTRLS IOCTL to figure out the next (or
> previous) control value by incrementing the current value and setting the
> appropriate flag. This is out of the scope of this set though.

This approach sounds really useful to me. Having control over the
rounding would allow reading supported control values with reasonable
effort. With such an approach, a very sparsely populated control becomes
feasible and with integer64 controls that'd likely allow representing
most exponential controls with linear values. If going this route, I
don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL.

Thus, I think that control over the rounding is tightly related to this
patchset and needs to be discussed together.

Helmut