From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F01E4C433EF for ; Tue, 19 Apr 2022 02:50:13 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 83A1583AFF; Tue, 19 Apr 2022 04:50:11 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=ti.com header.i=@ti.com header.b="JjejOb8x"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C0AAD83B1B; Tue, 19 Apr 2022 04:50:00 +0200 (CEST) Received: from lelv0142.ext.ti.com (lelv0142.ext.ti.com [198.47.23.249]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E979A83AFF for ; Tue, 19 Apr 2022 04:49:56 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=n-francis@ti.com Received: from lelv0265.itg.ti.com ([10.180.67.224]) by lelv0142.ext.ti.com (8.15.2/8.15.2) with ESMTP id 23J2nlke042164; Mon, 18 Apr 2022 21:49:47 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1650336587; bh=gusZkDR1EhG9kcPFLz+ziRi0/tDCz8V0tNauqTmhQQ0=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=JjejOb8xQ9WGxRzMX7SbkWuwi4/3YWq12VuCKJFCpLEWTp3fO+cTxQPVRw3unRNlm aiq6yGkubejTDLkfULWMxgGEbvCAbxh2RlgEcAcc2zD20cV49wi7ZZAk4lwquvPwIw D9QJEseWx8Qe4y0QFyEWGllotJk47imNoiiKff/c= Received: from DFLE109.ent.ti.com (dfle109.ent.ti.com [10.64.6.30]) by lelv0265.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 23J2nlMx026864 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 18 Apr 2022 21:49:47 -0500 Received: from DFLE112.ent.ti.com (10.64.6.33) by DFLE109.ent.ti.com (10.64.6.30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.14; Mon, 18 Apr 2022 21:49:47 -0500 Received: from fllv0039.itg.ti.com (10.64.41.19) by DFLE112.ent.ti.com (10.64.6.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.14 via Frontend Transport; Mon, 18 Apr 2022 21:49:47 -0500 Received: from [10.250.235.96] (ileax41-snat.itg.ti.com [10.172.224.153]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 23J2nfad112468; Mon, 18 Apr 2022 21:49:42 -0500 Message-ID: <63d3bf8d-e70e-03d3-9c7a-c8cd66311d18@ti.com> Date: Tue, 19 Apr 2022 08:19:41 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [RESEND, RFC 1/8] tools: config: yaml: Add board config class to generate config binaries Content-Language: en-US To: Alper Nebi Yasak CC: , , , , , , , , , , , , References: <20220406122919.6104-1-n-francis@ti.com> <20220406122919.6104-2-n-francis@ti.com> <5993486a-5e8e-b38d-2293-a9379e0365af@gmail.com> From: Neha Malcom Francis In-Reply-To: <5993486a-5e8e-b38d-2293-a9379e0365af@gmail.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean 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 >> [n-francis@ti.com: prepared patch for upstreaming] >> Signed-off-by: Neha Malcom Francis >> --- >> 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(' >> + >> + 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