| Message ID | 20240322131451.3092931-3-dan.scally@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, thank you for the patch. On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote: > Generate maps associating control enumerated control values with a > string representing that value to provide a central reference for > them across all pipeline handlers. I think s/control enumerated control/enumerated control/ Otherwise, it looks good to me. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > include/libcamera/control_ids.h.in | 2 ++ > include/libcamera/property_ids.h.in | 2 ++ > utils/gen-controls.py | 19 +++++++++++++++++++ > 3 files changed, 23 insertions(+) > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index d53b1cf7..58dd48e1 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -10,7 +10,9 @@ > #pragma once > > #include <array> > +#include <map> > #include <stdint.h> > +#include <string> > > #include <libcamera/controls.h> > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > index 43372c71..f51ba028 100644 > --- a/include/libcamera/property_ids.h.in > +++ b/include/libcamera/property_ids.h.in > @@ -9,7 +9,9 @@ > > #pragma once > > +#include <map> > #include <stdint.h> > +#include <string> > > #include <libcamera/controls.h> > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > index 6cd5e362..4fe1e705 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -140,6 +140,12 @@ ${description} > */''') > enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''') > enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') > + name_value_map_doc = string.Template('''/** > + * \\var ${name}NameValueMap > + * \\brief Map of all $name supported value names (in std::string format) to value > + */''') > + name_value_map_start = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap = {''') > + name_value_values = string.Template('''\t{ "${name}", ${name} },''') > > ctrls_doc = {} > ctrls_def = {} > @@ -183,6 +189,7 @@ ${description} > > values_info = { > 'name': info['name'], > + 'type': ctrl.type, > 'size': num_entries, > } > target_doc.append(enum_values_doc.substitute(values_info)) > @@ -194,6 +201,15 @@ ${description} > target_def.append(enum_values_values.substitute(value_info)) > target_def.append("};") > > + target_doc.append(name_value_map_doc.substitute(values_info)) > + target_def.append(name_value_map_start.substitute(values_info)) > + for enum in ctrl.enum_values: > + value_info = { > + 'name': enum.name > + } > + target_def.append(name_value_values.substitute(value_info)) > + target_def.append("};") > + > target_doc.append(doc_template.substitute(info)) > target_def.append(def_template.substitute(info)) > > @@ -231,6 +247,7 @@ def generate_h(controls, mode, ranges): > enum_template_start = string.Template('''enum ${name}Enum {''') > enum_value_template = string.Template('''\t${name} = ${value},''') > enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''') > + name_value_map_template = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap;''') > template = string.Template('''extern const Control<${type}> ${name};''') > > ctrls = {} > @@ -273,9 +290,11 @@ def generate_h(controls, mode, ranges): > > values_info = { > 'name': info['name'], > + 'type': ctrl.type, > 'size': num_entries, > } > target_ctrls.append(enum_values_template.substitute(values_info)) > + target_ctrls.append(name_value_map_template.substitute(values_info)) > > target_ctrls.append(template.substitute(info)) > id_value[vendor] += 1 > -- > 2.34.1 >
Hi Dan On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote: > Generate maps associating control enumerated control values with a s/control enumerated control values/enumerated control values/ ? > string representing that value to provide a central reference for > them across all pipeline handlers. As noticed by Kieran, it would be nice to expand a bit on what this would be used for > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > include/libcamera/control_ids.h.in | 2 ++ > include/libcamera/property_ids.h.in | 2 ++ > utils/gen-controls.py | 19 +++++++++++++++++++ > 3 files changed, 23 insertions(+) I like this The only alternative proposal would be to unify the ControlValue and the name string into a struct and enumerate them in a single array to make sure they stay in sync. But since this is auto-generated and adjusting all users of the *Values array is probably a big job, I'm not sure it's even worth considering the alternative proposal. What you have here is more than fine! Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index d53b1cf7..58dd48e1 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -10,7 +10,9 @@ > #pragma once > > #include <array> > +#include <map> > #include <stdint.h> > +#include <string> > > #include <libcamera/controls.h> > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > index 43372c71..f51ba028 100644 > --- a/include/libcamera/property_ids.h.in > +++ b/include/libcamera/property_ids.h.in > @@ -9,7 +9,9 @@ > > #pragma once > > +#include <map> > #include <stdint.h> > +#include <string> > > #include <libcamera/controls.h> > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > index 6cd5e362..4fe1e705 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -140,6 +140,12 @@ ${description} > */''') > enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''') > enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') > + name_value_map_doc = string.Template('''/** > + * \\var ${name}NameValueMap > + * \\brief Map of all $name supported value names (in std::string format) to value > + */''') > + name_value_map_start = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap = {''') > + name_value_values = string.Template('''\t{ "${name}", ${name} },''') > > ctrls_doc = {} > ctrls_def = {} > @@ -183,6 +189,7 @@ ${description} > > values_info = { > 'name': info['name'], > + 'type': ctrl.type, > 'size': num_entries, > } > target_doc.append(enum_values_doc.substitute(values_info)) > @@ -194,6 +201,15 @@ ${description} > target_def.append(enum_values_values.substitute(value_info)) > target_def.append("};") > > + target_doc.append(name_value_map_doc.substitute(values_info)) > + target_def.append(name_value_map_start.substitute(values_info)) > + for enum in ctrl.enum_values: > + value_info = { > + 'name': enum.name > + } > + target_def.append(name_value_values.substitute(value_info)) > + target_def.append("};") > + > target_doc.append(doc_template.substitute(info)) > target_def.append(def_template.substitute(info)) > > @@ -231,6 +247,7 @@ def generate_h(controls, mode, ranges): > enum_template_start = string.Template('''enum ${name}Enum {''') > enum_value_template = string.Template('''\t${name} = ${value},''') > enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''') > + name_value_map_template = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap;''') > template = string.Template('''extern const Control<${type}> ${name};''') > > ctrls = {} > @@ -273,9 +290,11 @@ def generate_h(controls, mode, ranges): > > values_info = { > 'name': info['name'], > + 'type': ctrl.type, > 'size': num_entries, > } > target_ctrls.append(enum_values_template.substitute(values_info)) > + target_ctrls.append(name_value_map_template.substitute(values_info)) > > target_ctrls.append(template.substitute(info)) > id_value[vendor] += 1 > -- > 2.34.1 >
On Mon, Mar 25, 2024 at 05:01:55PM +0100, Jacopo Mondi wrote: > Hi Dan > > On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote: > > Generate maps associating control enumerated control values with a > s/control enumerated control values/enumerated control values/ ? > > > string representing that value to provide a central reference for > > them across all pipeline handlers. > > As noticed by Kieran, it would be nice to expand a bit on what this > would be used for I was going to mention the same :-) A good commit message needs to include *why* a change is needed and desirable. Describing what it does is secondary to the reason. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > --- > > include/libcamera/control_ids.h.in | 2 ++ > > include/libcamera/property_ids.h.in | 2 ++ > > utils/gen-controls.py | 19 +++++++++++++++++++ > > 3 files changed, 23 insertions(+) > > I like this > > The only alternative proposal would be to unify the ControlValue and > the name string into a struct and enumerate them in a single array > to make sure they stay in sync. > > But since this is auto-generated and adjusting all users of the *Values > array is probably a big job, I'm not sure it's even worth considering the > alternative proposal. > > What you have here is more than fine! > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > > index d53b1cf7..58dd48e1 100644 > > --- a/include/libcamera/control_ids.h.in > > +++ b/include/libcamera/control_ids.h.in > > @@ -10,7 +10,9 @@ > > #pragma once > > > > #include <array> > > +#include <map> > > #include <stdint.h> > > +#include <string> > > > > #include <libcamera/controls.h> > > > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > > index 43372c71..f51ba028 100644 > > --- a/include/libcamera/property_ids.h.in > > +++ b/include/libcamera/property_ids.h.in > > @@ -9,7 +9,9 @@ > > > > #pragma once > > > > +#include <map> > > #include <stdint.h> > > +#include <string> > > > > #include <libcamera/controls.h> > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > index 6cd5e362..4fe1e705 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -140,6 +140,12 @@ ${description} > > */''') > > enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''') > > enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') > > + name_value_map_doc = string.Template('''/** > > + * \\var ${name}NameValueMap > > + * \\brief Map of all $name supported value names (in std::string format) to value > > + */''') > > + name_value_map_start = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap = {''') > > + name_value_values = string.Template('''\t{ "${name}", ${name} },''') We should really switch to jinja templates, this is harder to read (no a prerequisite for this series). > > > > ctrls_doc = {} > > ctrls_def = {} > > @@ -183,6 +189,7 @@ ${description} > > > > values_info = { > > 'name': info['name'], > > + 'type': ctrl.type, > > 'size': num_entries, > > } > > target_doc.append(enum_values_doc.substitute(values_info)) > > @@ -194,6 +201,15 @@ ${description} > > target_def.append(enum_values_values.substitute(value_info)) > > target_def.append("};") > > > > + target_doc.append(name_value_map_doc.substitute(values_info)) > > + target_def.append(name_value_map_start.substitute(values_info)) > > + for enum in ctrl.enum_values: > > + value_info = { > > + 'name': enum.name > > + } > > + target_def.append(name_value_values.substitute(value_info)) > > + target_def.append("};") > > + > > target_doc.append(doc_template.substitute(info)) > > target_def.append(def_template.substitute(info)) > > > > @@ -231,6 +247,7 @@ def generate_h(controls, mode, ranges): > > enum_template_start = string.Template('''enum ${name}Enum {''') > > enum_value_template = string.Template('''\t${name} = ${value},''') > > enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''') > > + name_value_map_template = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap;''') > > template = string.Template('''extern const Control<${type}> ${name};''') > > > > ctrls = {} > > @@ -273,9 +290,11 @@ def generate_h(controls, mode, ranges): > > > > values_info = { > > 'name': info['name'], > > + 'type': ctrl.type, > > 'size': num_entries, > > } > > target_ctrls.append(enum_values_template.substitute(values_info)) > > + target_ctrls.append(name_value_map_template.substitute(values_info)) > > > > target_ctrls.append(template.substitute(info)) > > id_value[vendor] += 1
Hi Dan, On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote: > Generate maps associating control enumerated control values with a > string representing that value to provide a central reference for > them across all pipeline handlers. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/control_ids.h.in | 2 ++ > include/libcamera/property_ids.h.in | 2 ++ > utils/gen-controls.py | 19 +++++++++++++++++++ > 3 files changed, 23 insertions(+) > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index d53b1cf7..58dd48e1 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -10,7 +10,9 @@ > #pragma once > > #include <array> > +#include <map> > #include <stdint.h> > +#include <string> > > #include <libcamera/controls.h> > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > index 43372c71..f51ba028 100644 > --- a/include/libcamera/property_ids.h.in > +++ b/include/libcamera/property_ids.h.in > @@ -9,7 +9,9 @@ > > #pragma once > > +#include <map> > #include <stdint.h> > +#include <string> > > #include <libcamera/controls.h> > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > index 6cd5e362..4fe1e705 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -140,6 +140,12 @@ ${description} > */''') > enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''') > enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') > + name_value_map_doc = string.Template('''/** > + * \\var ${name}NameValueMap > + * \\brief Map of all $name supported value names (in std::string format) to value > + */''') > + name_value_map_start = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap = {''') > + name_value_values = string.Template('''\t{ "${name}", ${name} },''') > > ctrls_doc = {} > ctrls_def = {} > @@ -183,6 +189,7 @@ ${description} > > values_info = { > 'name': info['name'], > + 'type': ctrl.type, > 'size': num_entries, > } > target_doc.append(enum_values_doc.substitute(values_info)) > @@ -194,6 +201,15 @@ ${description} > target_def.append(enum_values_values.substitute(value_info)) > target_def.append("};") > > + target_doc.append(name_value_map_doc.substitute(values_info)) > + target_def.append(name_value_map_start.substitute(values_info)) > + for enum in ctrl.enum_values: > + value_info = { > + 'name': enum.name > + } > + target_def.append(name_value_values.substitute(value_info)) > + target_def.append("};") > + > target_doc.append(doc_template.substitute(info)) > target_def.append(def_template.substitute(info)) > > @@ -231,6 +247,7 @@ def generate_h(controls, mode, ranges): > enum_template_start = string.Template('''enum ${name}Enum {''') > enum_value_template = string.Template('''\t${name} = ${value},''') > enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''') > + name_value_map_template = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap;''') > template = string.Template('''extern const Control<${type}> ${name};''') > > ctrls = {} > @@ -273,9 +290,11 @@ def generate_h(controls, mode, ranges): > > values_info = { > 'name': info['name'], > + 'type': ctrl.type, > 'size': num_entries, > } > target_ctrls.append(enum_values_template.substitute(values_info)) > + target_ctrls.append(name_value_map_template.substitute(values_info)) > > target_ctrls.append(template.substitute(info)) > id_value[vendor] += 1 > -- > 2.34.1 >
diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in index d53b1cf7..58dd48e1 100644 --- a/include/libcamera/control_ids.h.in +++ b/include/libcamera/control_ids.h.in @@ -10,7 +10,9 @@ #pragma once #include <array> +#include <map> #include <stdint.h> +#include <string> #include <libcamera/controls.h> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in index 43372c71..f51ba028 100644 --- a/include/libcamera/property_ids.h.in +++ b/include/libcamera/property_ids.h.in @@ -9,7 +9,9 @@ #pragma once +#include <map> #include <stdint.h> +#include <string> #include <libcamera/controls.h> diff --git a/utils/gen-controls.py b/utils/gen-controls.py index 6cd5e362..4fe1e705 100755 --- a/utils/gen-controls.py +++ b/utils/gen-controls.py @@ -140,6 +140,12 @@ ${description} */''') enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''') enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''') + name_value_map_doc = string.Template('''/** + * \\var ${name}NameValueMap + * \\brief Map of all $name supported value names (in std::string format) to value + */''') + name_value_map_start = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap = {''') + name_value_values = string.Template('''\t{ "${name}", ${name} },''') ctrls_doc = {} ctrls_def = {} @@ -183,6 +189,7 @@ ${description} values_info = { 'name': info['name'], + 'type': ctrl.type, 'size': num_entries, } target_doc.append(enum_values_doc.substitute(values_info)) @@ -194,6 +201,15 @@ ${description} target_def.append(enum_values_values.substitute(value_info)) target_def.append("};") + target_doc.append(name_value_map_doc.substitute(values_info)) + target_def.append(name_value_map_start.substitute(values_info)) + for enum in ctrl.enum_values: + value_info = { + 'name': enum.name + } + target_def.append(name_value_values.substitute(value_info)) + target_def.append("};") + target_doc.append(doc_template.substitute(info)) target_def.append(def_template.substitute(info)) @@ -231,6 +247,7 @@ def generate_h(controls, mode, ranges): enum_template_start = string.Template('''enum ${name}Enum {''') enum_value_template = string.Template('''\t${name} = ${value},''') enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''') + name_value_map_template = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap;''') template = string.Template('''extern const Control<${type}> ${name};''') ctrls = {} @@ -273,9 +290,11 @@ def generate_h(controls, mode, ranges): values_info = { 'name': info['name'], + 'type': ctrl.type, 'size': num_entries, } target_ctrls.append(enum_values_template.substitute(values_info)) + target_ctrls.append(name_value_map_template.substitute(values_info)) target_ctrls.append(template.substitute(info)) id_value[vendor] += 1
Generate maps associating control enumerated control values with a string representing that value to provide a central reference for them across all pipeline handlers. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- include/libcamera/control_ids.h.in | 2 ++ include/libcamera/property_ids.h.in | 2 ++ utils/gen-controls.py | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+)