| Message ID | 20201020180534.36855-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 20/10/2020 19:05, Jacopo Mondi wrote: > For each Control that support enumerated values generate a vector > of ControlValues which contains the full list of values. > > At the expense of a slight increase in memory occupation this change > allows the construction of the ControlInfo associated with a Control > from the values list, defaulting the minimum and maximum values > reported by the ControlInfo. > Personally I think this is a good way to handle this data. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_ids.cpp.in | 2 ++ > utils/gen-controls.py | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > index 056645cfbdfb..ca0b5b22f899 100644 > --- a/src/libcamera/control_ids.cpp.in > +++ b/src/libcamera/control_ids.cpp.in > @@ -6,8 +6,10 @@ > * > * This file is auto-generated. Do not edit. > */ > +#include <vector> > > #include <libcamera/control_ids.h> > +#include <libcamera/controls.h> > > /** > * \file control_ids.h > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > index 93cb3885c3da..87036fe7dec1 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -100,6 +100,8 @@ ${description} > def generate_h(controls): > enum_template_start = string.Template('''enum ${name}Values {''') > enum_value_template = string.Template('''\t${name} = ${value},''') > + enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}List = {''') 'List' appended to the name feels a bit odd. Seeing the usage in the next patch,: &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeList) &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeList) &controls::AeExposureMode, ControlInfo(controls::AeExposureModeList) Throwing alternatives in to the bikeshed, because that's all it is, this could be 'Values' or 'Enum' on the end. To me, List could end up being a list of strings, or floats, or anything. Values, could also be ... arbitrary numerical values, but matches the name in the ControlInfo Enum ... will be integer values ... Bikeshed colours aside: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') > template = string.Template('''extern const Control<${type}> ${name};''') > > ctrls = [] > @@ -140,6 +142,14 @@ def generate_h(controls): > target_ctrls.append(enum_value_template.substitute(value_info)) > target_ctrls.append("};") > > + target_ctrls.append(enum_list_start.substitute(info)) > + for entry in enum: > + value_info = { > + 'name': entry['name'], > + } > + target_ctrls.append(enum_list_values.substitute(value_info)) > + target_ctrls.append("};") > + > target_ctrls.append(template.substitute(info)) > id_value += 1 > >
Hi Kieran, On Wed, Oct 21, 2020 at 01:58:17PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 20/10/2020 19:05, Jacopo Mondi wrote: > > For each Control that support enumerated values generate a vector > > of ControlValues which contains the full list of values. > > > > At the expense of a slight increase in memory occupation this change > > allows the construction of the ControlInfo associated with a Control > > from the values list, defaulting the minimum and maximum values > > reported by the ControlInfo. > > > > Personally I think this is a good way to handle this data. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/control_ids.cpp.in | 2 ++ > > utils/gen-controls.py | 10 ++++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > > index 056645cfbdfb..ca0b5b22f899 100644 > > --- a/src/libcamera/control_ids.cpp.in > > +++ b/src/libcamera/control_ids.cpp.in > > @@ -6,8 +6,10 @@ > > * > > * This file is auto-generated. Do not edit. > > */ > > +#include <vector> > > > > #include <libcamera/control_ids.h> > > +#include <libcamera/controls.h> > > > > /** > > * \file control_ids.h > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > index 93cb3885c3da..87036fe7dec1 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -100,6 +100,8 @@ ${description} > > def generate_h(controls): > > enum_template_start = string.Template('''enum ${name}Values {''') > > enum_value_template = string.Template('''\t${name} = ${value},''') > > + enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}List = {''') > > 'List' appended to the name feels a bit odd. > > Seeing the usage in the next patch,: > > &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeList) > &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeList) > &controls::AeExposureMode, ControlInfo(controls::AeExposureModeList) > > Throwing alternatives in to the bikeshed, because that's all it is, this > could be 'Values' or 'Enum' on the end. > 'Values' is taken as you can see a few lines above I'm not excited by 'Enum' to be honest What if we rename the value enumeration with 'Value' and the vector 'Values' : enum ControlXYZValue { ControlXYZOne, ControlXYZTwo, ControlXYZThree, }; static const std::vector<ControlValue> ControlXYZValues = { static_cast<int32_t>(ControlXYZOne), static_cast<int32_t>(ControlXYZTwo), static_cast<int32_t>(ControlXYZThree), }; > To me, List could end up being a list of strings, or floats, or anything. > > Values, could also be ... arbitrary numerical values, but matches the > name in the ControlInfo > > Enum ... will be integer values ... > > > Bikeshed colours aside: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Thanks j > > + enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') > > template = string.Template('''extern const Control<${type}> ${name};''') > > > > ctrls = [] > > @@ -140,6 +142,14 @@ def generate_h(controls): > > target_ctrls.append(enum_value_template.substitute(value_info)) > > target_ctrls.append("};") > > > > + target_ctrls.append(enum_list_start.substitute(info)) > > + for entry in enum: > > + value_info = { > > + 'name': entry['name'], > > + } > > + target_ctrls.append(enum_list_values.substitute(value_info)) > > + target_ctrls.append("};") > > + > > target_ctrls.append(template.substitute(info)) > > id_value += 1 > > > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 21/10/2020 14:40, Jacopo Mondi wrote: > Hi Kieran, > > On Wed, Oct 21, 2020 at 01:58:17PM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 20/10/2020 19:05, Jacopo Mondi wrote: >>> For each Control that support enumerated values generate a vector >>> of ControlValues which contains the full list of values. >>> >>> At the expense of a slight increase in memory occupation this change >>> allows the construction of the ControlInfo associated with a Control >>> from the values list, defaulting the minimum and maximum values >>> reported by the ControlInfo. >>> >> >> Personally I think this is a good way to handle this data. >> >> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> src/libcamera/control_ids.cpp.in | 2 ++ >>> utils/gen-controls.py | 10 ++++++++++ >>> 2 files changed, 12 insertions(+) >>> >>> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in >>> index 056645cfbdfb..ca0b5b22f899 100644 >>> --- a/src/libcamera/control_ids.cpp.in >>> +++ b/src/libcamera/control_ids.cpp.in >>> @@ -6,8 +6,10 @@ >>> * >>> * This file is auto-generated. Do not edit. >>> */ >>> +#include <vector> >>> >>> #include <libcamera/control_ids.h> >>> +#include <libcamera/controls.h> >>> >>> /** >>> * \file control_ids.h >>> diff --git a/utils/gen-controls.py b/utils/gen-controls.py >>> index 93cb3885c3da..87036fe7dec1 100755 >>> --- a/utils/gen-controls.py >>> +++ b/utils/gen-controls.py >>> @@ -100,6 +100,8 @@ ${description} >>> def generate_h(controls): >>> enum_template_start = string.Template('''enum ${name}Values {''') >>> enum_value_template = string.Template('''\t${name} = ${value},''') >>> + enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}List = {''') >> >> 'List' appended to the name feels a bit odd. >> >> Seeing the usage in the next patch,: >> >> &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeList) >> &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeList) >> &controls::AeExposureMode, ControlInfo(controls::AeExposureModeList) >> >> Throwing alternatives in to the bikeshed, because that's all it is, this >> could be 'Values' or 'Enum' on the end. >> > > 'Values' is taken as you can see a few lines above > I'm not excited by 'Enum' to be honest > > What if we rename the value enumeration with 'Value' and the vector > 'Values' : Or rename the enum to Enum, > > enum ControlXYZValue { enum ControlXYZEnum { ? -- Kieran > ControlXYZOne, > ControlXYZTwo, > ControlXYZThree, > }; > static const std::vector<ControlValue> ControlXYZValues = { > static_cast<int32_t>(ControlXYZOne), > static_cast<int32_t>(ControlXYZTwo), > static_cast<int32_t>(ControlXYZThree), > }; > >> To me, List could end up being a list of strings, or floats, or anything. >> >> Values, could also be ... arbitrary numerical values, but matches the >> name in the ControlInfo >> >> Enum ... will be integer values ... >> >> >> Bikeshed colours aside: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > > Thanks > j > >>> + enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') >>> template = string.Template('''extern const Control<${type}> ${name};''') >>> >>> ctrls = [] >>> @@ -140,6 +142,14 @@ def generate_h(controls): >>> target_ctrls.append(enum_value_template.substitute(value_info)) >>> target_ctrls.append("};") >>> >>> + target_ctrls.append(enum_list_start.substitute(info)) >>> + for entry in enum: >>> + value_info = { >>> + 'name': entry['name'], >>> + } >>> + target_ctrls.append(enum_list_values.substitute(value_info)) >>> + target_ctrls.append("};") >>> + >>> target_ctrls.append(template.substitute(info)) >>> id_value += 1 >>> >>> >> >> -- >> Regards >> -- >> Kieran
diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in index 056645cfbdfb..ca0b5b22f899 100644 --- a/src/libcamera/control_ids.cpp.in +++ b/src/libcamera/control_ids.cpp.in @@ -6,8 +6,10 @@ * * This file is auto-generated. Do not edit. */ +#include <vector> #include <libcamera/control_ids.h> +#include <libcamera/controls.h> /** * \file control_ids.h diff --git a/utils/gen-controls.py b/utils/gen-controls.py index 93cb3885c3da..87036fe7dec1 100755 --- a/utils/gen-controls.py +++ b/utils/gen-controls.py @@ -100,6 +100,8 @@ ${description} def generate_h(controls): enum_template_start = string.Template('''enum ${name}Values {''') enum_value_template = string.Template('''\t${name} = ${value},''') + enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}List = {''') + enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') template = string.Template('''extern const Control<${type}> ${name};''') ctrls = [] @@ -140,6 +142,14 @@ def generate_h(controls): target_ctrls.append(enum_value_template.substitute(value_info)) target_ctrls.append("};") + target_ctrls.append(enum_list_start.substitute(info)) + for entry in enum: + value_info = { + 'name': entry['name'], + } + target_ctrls.append(enum_list_values.substitute(value_info)) + target_ctrls.append("};") + target_ctrls.append(template.substitute(info)) id_value += 1
For each Control that support enumerated values generate a vector of ControlValues which contains the full list of values. At the expense of a slight increase in memory occupation this change allows the construction of the ControlInfo associated with a Control from the values list, defaulting the minimum and maximum values reported by the ControlInfo. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/control_ids.cpp.in | 2 ++ utils/gen-controls.py | 10 ++++++++++ 2 files changed, 12 insertions(+)