On 06/04/2022 15:29, Neha Malcom Francis wrote: > For validating config files and generating binary config artifacts, here > board specific config class is added. > > Add function cfgBinaryGen() in tibcfg_gen.py. It uses TIBoardConfig > class to load given schema and config files in YAML, validate them and > generate binaries.
The subject lines (of other patches as well) sound too generic when most of them are TI specific, I'd expect at least a 'ti:' tag except where you already include more specific terms like a board name. (This one could be "tools: ti: Add ..." for example). > > Signed-off-by: Tarun Sahu <t-s...@ti.com> > [n-fran...@ti.com: prepared patch for upstreaming] > Signed-off-by: Neha Malcom Francis <n-fran...@ti.com> > --- > test/py/requirements.txt | 1 + > tools/tibcfg_gen.py | 116 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 117 insertions(+) > create mode 100644 tools/tibcfg_gen.py > > diff --git a/test/py/requirements.txt b/test/py/requirements.txt > index 33c5c0bbc4..a91ba64563 100644 > --- a/test/py/requirements.txt > +++ b/test/py/requirements.txt > @@ -4,6 +4,7 @@ coverage==4.5.4 > extras==1.0.0 > fixtures==3.0.0 > importlib-metadata==0.23 > +jsonschema==4.0.0 > linecache2==1.0.0 > more-itertools==7.2.0 > packaging==19.2 > diff --git a/tools/tibcfg_gen.py b/tools/tibcfg_gen.py > new file mode 100644 > index 0000000000..7635596906 > --- /dev/null > +++ b/tools/tibcfg_gen.py > @@ -0,0 +1,116 @@ > +# SPDX-License-Identifier: GPL-2.0+ > +# Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/ > +# > +# TI Board Configuration Class for Schema Validation and Binary Generation > +# > + > +from jsonschema import validate > + > +import yaml > +import os > +import sys Standard library imports should appear before third-party libraries, with an empty line between them. > + > + > +class TIBoardConfig: > + file_yaml = {} > + schema_yaml = {} > + data_rules = {} These belong in __init__ as they are per-instance attributes. > + > + def __init__(self): > + pass > + > + def Load(self, file, schema, data_rules=""): You can rename this to be the __init__ function. > + with open(file, 'r') as f: > + self.file_yaml = yaml.safe_load(f) > + with open(schema, 'r') as sch: > + self.schema_yaml = yaml.safe_load(sch) > + self.data_rules = data_rules > + > + def CheckValidity(self): > + try: > + validate(self.file_yaml, self.schema_yaml) > + return True > + except Exception as e: > + print(e) > + return False You can also do this validation immediately after loading the yaml files in the __init__(), and then safely assume any created object is valid. > + > + def __ConvertToByteChunk(self, val, data_type): Methods should be in snake_case. Also consider using a single underscore as the prefix, double underscore does some special name mangling. > + br = [] > + size = 0 > + if(data_type == "#/definitions/u8"): > + size = 1 > + elif(data_type == "#/definitions/u16"): > + size = 2 > + elif(data_type == "#/definitions/u32"): > + size = 4 > + else: > + return -1 I think this case should raise an error of some kind. > + if(type(val) == int): > + while(val != 0): In general, don't use parentheses with if, while etc. > + br = br + [(val & 0xFF)] > + val = val >> 8 > + while(len(br) < size): > + br = br + [0] > + return br This all looks like val.to_bytes(size, 'little'), but as a list instead of bytes. If you want to get fancy, have a look at the struct module. (For example, struct.pack('<L', val) ) > + > + def __CompileYaml(self, schema_yaml, file_yaml): > + br = [] Consider using a bytearray instead of a list-of-ints here. > + for key in file_yaml.keys(): I think things would be more readable if you extracted node = file_yaml[key] node_schema = schema_yaml['properties'][key] node_type = node_schema.get('type') as variables here and used those in the following code. > + if not 'type' in schema_yaml['properties'][key]: > + br = br + \ br += ... would be nicer for all of these. > + self.__ConvertToByteChunk( > + file_yaml[key], > schema_yaml['properties'][key]["$ref"]) > + elif schema_yaml['properties'][key]['type'] == 'object': > + br = br + \ > + self.__CompileYaml( > + schema_yaml['properties'][key], file_yaml[key]) > + elif schema_yaml['properties'][key]['type'] == 'array': > + for item in file_yaml[key]: > + if not isinstance(item, dict): > + br = br + \ > + self.__ConvertToByteChunk( > + item, > schema_yaml['properties'][key]['items']["$ref"]) > + else: > + br = br + \ > + self.__CompileYaml( > + schema_yaml['properties'][key]['items'], > item) > + return br > + > + def GenerateBinaries(self, out_path=""): > + if not os.path.isdir(out_path): > + os.mkdir(out_path) > + if(self.CheckValidity()): > + for key in self.file_yaml.keys(): > + br = [] You don't need this assignment, it's overwritten in the next one anyway. > + br = self.__CompileYaml( > + self.schema_yaml['properties'][key], self.file_yaml[key]) > + with open(out_path + "/" + key + ".bin", 'wb') as cfg: Construct file paths with os.path.join() here and below. > + cfg.write(bytearray(br)) > + else: > + raise ValueError("Config YAML Validation failed!") > + > + def DeleteBinaries(self, out_path=""): > + if os.path.isdir(out_path): > + for key in self.file_yaml.keys(): > + if os.path.isfile(out_path + "/" + key + ".bin"): > + os.remove(out_path + "/" + key + ".bin") > + > + > +def cfgBinaryGen(): > + """Generate config binaries from YAML config file and YAML schema > + Arguments: > + - config_yaml: board config file in YAML > + - schema_yaml: schema file in YAML to validate config_yaml > against > + Pass the arguments along with the filename in the Makefile. > + """ > + tibcfg = TIBoardConfig() > + config_yaml = sys.argv[1] > + schema_yaml = sys.argv[2] > + try: > + tibcfg.Load(config_yaml, schema_yaml) > + except: > + raise ValueError("Could not find config files!") > + tibcfg.GenerateBinaries(os.environ['O']) I think it'd be better to pass the directory as an -o / --output-dir argument instead of reading it from environment. You can use argparse to parse the command line arguments. > + > + > +cfgBinaryGen() This should be guarded by if __name__ == '__main__'.