[RFC PATCH v1] libcamera: controls: Generate macro for each control

[Kieran Bingham]

Quoting Laurent Pinchart (2025-03-18 20:43:57)
> On Tue, Mar 18, 2025 at 06:22:45PM +0100, Jacopo Mondi wrote:
> > On Tue, Mar 18, 2025 at 05:00:15PM +0200, Laurent Pinchart wrote:
> > > On Tue, Mar 18, 2025 at 09:11:27AM +0100, Jacopo Mondi wrote:
> > > > On Mon, Mar 17, 2025 at 10:00:20PM +0100, Barnabás Pőcze wrote:
> > > > > 2025. 03. 17. 19:54 keltezéssel, Jacopo Mondi írta:
> > > > > > On Mon, Mar 17, 2025 at 07:11:46PM +0100, Barnabás Pőcze wrote:
> > > > > > > Generate a macro in the form of LIBCAMERA_HAS_$VENDOR_$MODE_$NAME
> > > > > > > for each control so that its existence can be checked easily
> > > > > > > and without extra version checks.
> > > > > > >
> > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> > > > > >
> > > > > > Do we really want this ?
> > > > >
> > > > > Good question, hence the RFC; I don't think there is a very nice
> > > > > way to discover (and use) controls at comptime/runtime. Also note
> > > >
> > > > What would the use case be ?
> > > >
> > > > > that the control namespaces already have their own existence macros
> > > > > (e.g. `LIBCAMERA_HAS_RPI_VENDOR_CONTROLS`).
> > > >
> > > > >
> > > > > > Applications should discover controls
> > > > > > programmatically.
> > > > >
> > > > > What do you have in mind? Looking up the numeric id in the appropriate
> > > > > `ControlIdMap` by the (namespace, name) pair?
> > > >
> > > > If you mean Camera::controls() and Camera::properties() then yes, I
> > > > think applications should enumerate the supported controls from there
> > > > and not rely on compile-time defines, as even if a control is defined
> > > > it doesn't mean it is supported by the camera.
> > >
> > > True, but an application that would check if Camera::controls()
> > > contains, for instance, controls::AE_ENABLE with
> > >
> > >     if (camera->controls().count(controls::AE_ENABLE))
> > >
> > > will not compile if the libcamera version it compiles against does not
> > > define the AeEnable control. Barnabás' patch allows fixing that with
> > > conditional compilation.
> > 
> > Do we really want applications to #ifdef on controls ?? I thought
> > controls are part of ABI, so once an application knows which version
> > of libcamera it link against, the list of defined controls should be
> > fixed. But yes, we don't have ABI stability, so...
> 
> Version checks are another option, I'm not against that.

I think something like this would be helpful to some applications
already.. We might always think 'the latest libcamera' is the best to
use - but LTS distributions will still exist shipping libcamera 0.3 when
we reach later versions - and 'applications' will need a way to still be
able to compile against the older versions where possible.


> > Not that I'm against this patch, just that seems to encourage bad code
> > practices in applications.
> > 
> > > > > > Or is this useful for ABI compliance validation ?
> > > > > >
> > > > > > > ---
> > > > > > >   include/libcamera/control_ids.h.in | 1 +
> > > > > > >   1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> > > > > > > index 5d0594c68..8f037589d 100644
> > > > > > > --- a/include/libcamera/control_ids.h.in
> > > > > > > +++ b/include/libcamera/control_ids.h.in
> > > > > > > @@ -49,6 +49,7 @@ extern const std::array<const ControlValue, {{ctrl.enum_values_count}}> {{ctrl.n
> > > > > > >   extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap;
> > > > > > >   {% endif -%}
> > > > > > >   extern const Control<{{ctrl.type}}> {{ctrl.name}};
> > > > > > > +#define LIBCAMERA_HAS_{{vendor|upper}}_{{mode|upper}}_{{ctrl.name|snake_case|upper}}

This honestly seems pretty 'cheap' and valuable. ... I think it's a good
idea ...


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> > > > > > >   {% endfor -%}
> > > > > > >
> > > > > > >   {% if vendor != 'libcamera' %}
> 
> -- 
> Regards,
> 
> Laurent Pinchart