Re: [RESEND, RFC 1/8] tools: config: yaml: Add board config class to generate config binaries

[Neha Malcom Francis <n-francis@ti.com>]

On 19/04/22 01:25, Alper Nebi Yasak wrote:
> 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-sahu@ti.com>
>> [n-francis@ti.com: prepared patch for upstreaming]
>> Signed-off-by: Neha Malcom Francis <n-francis@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__'.

Thanks for all the comments, I'll reformat all these patches and make 
the necessary changes.

-- 
Thanking You
Neha Malcom Francis