Document new validation functions

Signed-off-by: bbrzyski <[email protected]>

CHANGELOG.md

@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Resolved an issue where IP Core node names in the GUI were parsed from the yaml description file name.
 - Resolved an issue with YAML files produced by `topwrap parse`, where unnecessary double hyphens (`--`) appeared in signal definitions
+- Changed how validation is done in topwrap in order to support hierarchical designs and check for more errors while creating design
 
 ## [0.1.0] - 2024-09-27
 

docs/source/conf.py

@@ -17,6 +17,7 @@
 import sys
 from datetime import datetime
 from os import environ
+from pathlib import Path
 
 from antmicro_sphinx_utils.defaults import numfig_format  # noqa: F401
 from antmicro_sphinx_utils.defaults import antmicro_html, antmicro_latex
@@ -33,6 +34,9 @@
 # documentation root, use os.path.abspath to make it absolute, like shown here.
 sys.path.insert(0, os.path.abspath("./_extensions"))
 
+# Add tests module to sys.path so that autodoc can eval tests functions
+sys.path.insert(0, str(Path("../../tests/").resolve()))
+
 # -- General configuration -----------------------------------------------------
 
 # General information about the project.

docs/source/developers_guide/checks.md

@@ -0,0 +1,158 @@
+# Validation of design
+
+One of topwrap features is to run validation on user's design which consists of series of checks for errors user may do while creating a design.
+
+
+```{eval-rst}
+.. autoclass:: topwrap.kpm_dataflow_validator.DataflowValidator
+   :members:
+```
+
+Each check returns the following class:
+
+```{eval-rst}
+.. autoclass:: topwrap.kpm_dataflow_validator.CheckResult
+```
+
+## Tests for validation checks
+
+Tests for the `DataflowValidator` class are done using various designs that are valid or have some errors in them with the goal to check everything validation functions can do.
+
+Below are all the graphs that are used for testing.
+
+### Duplicate IP names
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_duplicate_ip_names
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_duplicate_ips.json
+:preview: True
+```
+
+### Invalid parameters' values
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_invalid_parameters_values
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_invalid_params.json
+:preview: True
+```
+
+### Connection between external Metanodes
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_ext_in_to_ext_out_connections
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_meta_to_meta_conn.json
+:preview: True
+```
+
+### Ports connected to multiple external Metanodes
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_ports_multiple_external_metanodes
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_port_to_multiple_external_metanodes.json
+:preview: True
+```
+
+### Duplicate Metanode names
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_duplicate_metanode_names
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_duplicate_metanode_names.json
+:preview: True
+```
+
+### Duplicate Metanode connected to interface
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_duplicate_external_input_interfaces
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_duplicate_ext_input_ifaces.json
+```
+
+### Unnamed Metanodes
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_unnamed_metanodes
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_pwm.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_connected_unnamed_metanode.json
+:preview: True
+```
+
+### Connection between two inout ports
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_inouts_connections
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_inout.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_inouts_connections.json
+:preview: True
+```
+
+### Unconnected ports in subgraph node
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_unconn_hierarchy
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_hierarchy.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_unconn_hierarchy.json
+```
+
+### Connection of subgraph node to multiple External Metanodes
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_subgraph_multiple_external_metanodes
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_hierarchy.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_subgraph_multiple_external_metanodes.json
+```
+
+### Connection to subgraph Metanode
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_conn_subgraph_metanode
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_hierarchy.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_conn_subgraph_metanode.json
+```
+
+### Complex hierarchy graph
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_complex_hierarchy
+```
+
+```{kpm_iframe}
+:spec: ../../../tests/data/data_kpm/conversions/complex/specification_complex.json
+:dataflow: ../../../tests/data/data_kpm/conversions/complex/dataflow_complex.json
+```
+
+### Duplicate IP cores in subgraph node
+```{eval-rst}
+.. autofunction:: tests_kpm.test_kpm_validation.dataflow_hier_duplicate_names
+```
+
+```{kpm_iframe}
+:spec: ../../build/kpm_jsons/spec_hierarchy.json
+:dataflow: ../../../tests/data/data_kpm/dataflow_tests/dataflow_hierarchical_duplicate_names.json
+```

docs/source/index.md

@@ -30,6 +30,7 @@ developers_guide/fusesocbuilder
 developers_guide/interface
 developers_guide/config
 developers_guide/parsing
+developers_guide/checks
 developers_guide/examples
 developers_guide/future_enhancements
 developers_guide/inline_kpm_howto

noxfile.py

@@ -184,7 +184,9 @@ def doc_gen(session: nox.Session) -> None:
                     example.parent / "kpm_dataflow.json", f"docs/build/kpm_jsons/data_{name}.json"
                 )
 
-    session.install(".[docs]")
+    session.install(
+        ".[docs,tests]"
+    )  # Tests are used by autodoc for the section `Validation of design`
     session.run("make", "-C", "docs", "html", external=True)
     session.run("make", "-C", "docs", "latexpdf", external=True)
 

tests/tests_build/test_interconnect.py

@@ -0,0 +1,92 @@
+# Copyright (C) 2023 Antmicro
+# SPDX-License-Identifier: Apache-2.0
+
+import re
+import shutil
+import subprocess
+import sys
+import tempfile
+from pathlib import Path
+
+import pytest
+
+
+class TestInterconnect:
+    @pytest.mark.skipif(
+        sys.version_info[:2] < (3, 9) or sys.version_info[:2] > (3, 10),
+        reason="interconnect test requires Python 3.9 or 3.10 due to litex requiring Python 3.9 or 3.10",
+    )
+    @pytest.mark.skipif(
+        not (
+            shutil.which("verilator")
+            and shutil.which("riscv64-unknown-elf-gcc")
+            and shutil.which("make")
+            and shutil.which("meson")
+            and shutil.which("ninja")
+            and shutil.which("hexdump")
+        ),
+        reason="verilator, riscv64-unknown-elf toolchain, make, meson, ninja and hexdump are required to run the interconnect test",
+    )
+    def test_interconnect_design(self, request: pytest.FixtureRequest) -> None:
+        remote_url = "https://github.com/antmicro/soc-generator.git"
+        commit_sha = "76ac5b3743bea1bb6e5d2ac7ecae31ab39d5e8b3"
+        data_dir = request.path.parent / Path("../data/data_build/interconnect")
+        # don't change formatting of the context manager below - this formatting is needed
+        # for Python 3.8 compatibility, see https://bugs.python.org/issue12782
+        # fmt: off
+        with (
+            tempfile.TemporaryDirectory()
+        ) as repo_tmp_dir, (
+            tempfile.TemporaryDirectory()
+        ) as build_tmp_dir:
+            # fmt: on
+            repo_dir = Path(repo_tmp_dir)
+            build_dir = Path(build_tmp_dir)
+            example_dir = repo_dir / Path("examples/simple_soc")
+            # clone soc generator repo
+            subprocess.check_call(["git", "clone", "--recurse-submodules", remote_url, repo_dir])
+            subprocess.check_call(["git", "checkout", commit_sha], cwd=repo_dir)
+            subprocess.check_call(["pip", "install", "."], cwd=repo_dir)
+            subprocess.check_call(["pip", "install", "-r", "requirements.txt"], cwd=example_dir)
+            subprocess.check_call(["make", "deps"], cwd=example_dir)
+
+            # build topwrap design
+            subprocess.check_call(
+                [
+                    "python",
+                    "-m",
+                    "topwrap",
+                    "build",
+                    "-d",
+                    "project.yaml",
+                    "-b",
+                    build_dir,
+                    "-p",
+                    "xc7k70tfbg484",
+                ],
+                cwd=data_dir,
+            )
+
+            # include additional sources in topwrap design
+            subprocess.check_call(
+                [
+                    "sh",
+                    "-c",
+                    f"cat {data_dir}/sources/mem.v {data_dir}/sources/soc.v {data_dir}/sources/crg.v >> {build_dir}/top.v",
+                ]
+            )
+
+            # move topwrap design to the location expected by soc generator
+            subprocess.check_call(["mkdir", example_dir / "build"])
+            subprocess.check_call(["cp", build_dir / "top.v", example_dir / "build" / "top.v"])
+
+            # create an empty file that soc generator expects
+            subprocess.check_call(["touch", example_dir / "build" / "wishbone_interconnect.v"])
+
+            # run simulation
+            output = subprocess.check_output(["make", "sim-run"], cwd=example_dir).decode("utf-8")
+
+            # tx_data is a list of all bytes transmitted on TX (in hex, as strings)
+            tx_data = re.findall(r"(?:\[\d+\] TX: (0x[0-9a-fA-F]+) RX: 0x[0-9a-fA-F]+\n)", output)
+            # check that "hello world" was transmitted on UART TX
+            assert "".join(map(lambda v: chr(int(v, base=16)), tx_data)) == "hello world"

tests/tests_kpm/test_kpm_client.py

@@ -42,9 +42,6 @@ def test_specification(self, all_yaml_files, all_specification_files, default_rp
             ), f"Test {test_name} differs from original specification. Diff: {spec_differences}"
 
     def test_dataflow_validation(self, all_yaml_files, all_dataflow_files, default_rpc_params):
-        # FIXME: Our validators are broken and forbid valid designs. Remove this line after fixing them.
-        del all_yaml_files["complex"]
-
         for test_name, ip_core_yamls in all_yaml_files.items():
             dataflow_json = all_dataflow_files[test_name]
 
@@ -57,9 +54,6 @@ def test_dataflow_validation(self, all_yaml_files, all_dataflow_files, default_r
                 logging.warning(f"Dataflow {test_name} has warnings {response_message['content']}")
 
     def test_dataflow_run(self, all_yaml_files, all_dataflow_files, default_rpc_params):
-        # FIXME: Our validators are broken and forbid valid designs. Remove this line after fixing them.
-        del all_yaml_files["complex"]
-
         for test_name, ip_core_yamls in all_yaml_files.items():
             dataflow_json = all_dataflow_files[test_name]
 

topwrap/hdl_parsers_utils.py

@@ -57,15 +57,20 @@ def parse_value_width_parameter(param: str) -> Dict[str, int]:
     return {"value": int(value, radix_to_base[radix]), "width": int(width)}
 
 
-def _valid_value_width_parameter(param: str) -> bool:
-    try:
-        parse_value_width_parameter(param)
-    except ValueError:
-        return False
-    return True
+def evaluate_parameter_list(parameters: List[ParameterToEval]) -> EvaluatedParamsReturnType:
+    """
+    Evaluates a list of parameters by resolving their values in dependency order.
 
+    Parameters are processed iteratively: resolved values are stored, while unresolved ones
+    are retried until all evaluable parameters are resolved or deemed invalid.
 
-def evaluate_parameter_list(parameters: List[ParameterToEval]) -> EvaluatedParamsReturnType:
+    Args:
+        parameters (List[ParameterToEval]): List of parameters to evaluate.
+
+    Returns:
+        EvaluatedParamsReturnType: A result containing successfully evaluated parameters
+                                   and a list of invalid parameters that could not be resolved.
+    """
     worklist = deque()
     evaluated = {}
     invalid_parameters = []

topwrap/kpm_dataflow_parser.py

@@ -51,9 +51,6 @@
 )
 from .util import JsonType, UnreachableError
 
-# from topwrap.kpm_dataflow_validator import validate_kpm_design
-
-
 logger = logging.getLogger(__name__)
 
 

topwrap/kpm_dataflow_validator.py

@@ -84,7 +84,7 @@ def __init__(self, dataflow: JsonType):
     def check_duplicate_node_names(self) -> CheckResult:
         """
         Check for any duplicate IP instance names in the graph (graph represents a hierarchy level).
-        Check is to prevent multiple nodes with the same "instanceName" in a given graph, since this is invalid in design.
+        This check prevents from creating multiple nodes with the same "instanceName" in a given graph, since this is invalid in design.
         There can be multiple nodes with the same "instanceName" in the whole design (on various hierarchy levels).
         """
 
@@ -146,12 +146,9 @@ def _get_invalid_param_str(node_name: str, param_name: str, param_val: str):
         evaluated_params = evaluate_parameter_list(parameter_list)
 
         for param in evaluated_params.not_evaluated:
-            assert isinstance(
-                param.value, str
-            )  # Every kpm param that is not evaluated will be a str
-            invalid_params_str.append(
-                _get_invalid_param_str(param.ip_core, param.name, param.value)
-            )
+            # Every kpm param that is not evaluated will be a str
+            assert isinstance(param.value, str)
+            invalid_params_str.append(f"{param.ip_core}:{param.name} value: {param.value}")
 
         # Check if constant metanodes have int value
         for node in get_dataflow_constant_metanodes(self.dataflow):
@@ -298,6 +295,7 @@ def check_connection_to_subgraph_metanodes(self) -> CheckResult:
         Check for any connections to exposed subgraph metanode ports.
 
         In this context:
+
         - **Exposed port**: A port on a subgraph metanode that represents the interface of the subgraph to the external graph. It is visible and accessible from outside the subgraph.
         - **Unexposed port**: An internal port on a subgraph metanode that is used for internal connections within the subgraph but is not accessible from the external graph.
 

topwrap/kpm_topwrap_client.py

@@ -23,8 +23,8 @@
 from .design_to_kpm_dataflow_parser import kpm_dataflow_from_design_descr
 from .kpm_common import RPCparams
 from .kpm_dataflow_parser import kpm_dataflow_to_design
-from .util import read_json_file, save_file_to_json
 from .kpm_dataflow_validator import DataflowValidator
+from .util import read_json_file, save_file_to_json
 from .yamls_to_kpm_spec_parser import ipcore_yamls_to_kpm_spec