Codecov Report

Merging #2634 (a1db296) into master (ed781b1) will decrease coverage by 1.07%.
The diff coverage is 53.38%.

@@            Coverage Diff             @@
##           master    #2634      +/-   ##
==========================================
- Coverage   72.80%   71.72%   -1.08%     
==========================================
  Files          46       47       +1     
  Lines        8081     8435     +354     
  Branches     1360     1453      +93     
==========================================
+ Hits         5883     6050     +167     
- Misses       1807     1971     +164     
- Partials      391      414      +23     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed781b1...a1db296. Read the comment docs.

Should add the new files to the coverage omit list

Shouldn't we instead be measuring coverage for them?

@eric-wieser What value would that add and is it worth the effort? Do we intend on getting coverage on simulators we don't have access to via mocking, or do we plan on excluding them? cocotb/config.py is excluded, should it instead be tested by the same logic because it contains code used in regressions? We didn't get coverage on the makefiles, is that simply because we couldn't, or because we didn't care?

I don't particularly care about the outcome, I just want consistency and to not have to constantly evaluate our position on every change.

How about we make a discussion to discuss changing our position and follow up by recording that position in CONTRIBUTING.md and potentially removing cocotb/config.py from the exclusion list, depending on the outcome of that discussion?

I also believe we should support running tests from the CLI and not just pytest.

I have some ideas for doing that in a "nice" way, I'll work on putting together a toy example in the next couple of weeks when I have some time that supports running from both pytest and from the command line

@alexforencich One way I thought of was loading a Python module as a configuration object. Declare your project's sources, toplevel, etc. in that module and specify the module on the command line. Command line args would take precedence.

My plan is to create a "testbench" object that manages parameters, calling run(), and has entry points for main() that does command line argument parsing as well as an entry point from python that can be exposed to pytest with decorators, etc.

My proposal for Python test runner requirements and design.

I have updated mostly according to https://github.com/cocotb/cocotb/wiki/Python-Test-Runner-Proposal .

Issues/discrepancies:

• some simulators (GHDL, Verilator, maybe also Icarus) require top HDL module name for build/compilations
• some simulators (Verilator, maybe Icarus) require wave for build/compilations

No idea why Questa-VHDL is failing. It is ok with my setup.

Not a fan of this implementation. It's essentially using the builder pattern which makes the class fragile due to coupling between the methods. This looks more like a Project class and less like a Simulator class. Those two concepts need to be decoupled, which is why I suggested the model types in the proposal.

@ktbarrett Can you sketch how user API could look like?

@themperek

I think the main interface should look like:

import os
from dataclasses import dataclass
from typing import Tuple, Union, Optional, List, Dict

Path = Union[str, "os.PathLike[str]"]


@dataclass
class Library:
    """
    Represents a compiled library

    Should be sufficient to allow users to define IP cores as pre-compiled libraries.
    Should be defined to allow the library to be used in further library or design builds.
    Should be defined so the same library can be used in *multiple* builds.
    The implementation is simulator-dependent.
    Should work in simulators that do library builds at the same time as designs by
    simply passing the paths through.
    Should work with simulators that express built libraries as object or as directories
    by abstracting away those specifics.
    """


@dataclass
class Design:
    """
    Represents a compiled library

    Should be sufficient to feed to the test phase.
    The implementation is simulator-dependent.
    """


class Simulator(object):
    """
    Defines tasks to run on designs

    Should encapsulate resources necessary to run the simulator *only*:
    * path to executable
    * path to license file
    """

    def build_lib(
        self,
        name: str,
        sources: List[Path] = [],
        libraries: List[Library] = [],
        includes: List[Path] = [],
        defines: List[str] = [],
        parameters: Dict[str, str] = {},
        extra_args: List[str] = [],
        build_dir: Optional[Path] = None,
        env: Optional[Dict[str, str]] = None,
    ) -> Library:
        """
        Builds a library

        Builds library of *name* with *sources*.
        *sources* can be any kind of files the compiler can understand.
        Sets *defines*, *includes*, *parameters*, and simulator-specific *extra_args*
        during the build.
        Links *libraries*.
        The build is done in *build_dir* in the *env* environment.
        If *build_dir* is ``None`` an appropriate build directory is chosen to prevent
        collision with other builds.
        If *env* is ``None``, the callers environment is used.
        Environment isolation can be done by passing an empty dictionary ``{}``.
        """

    def build(
        self,
        top: Optional[str] = None,
        sources: List[Path] = [],
        libraries: List[Library] = [],
        includes: List[Path] = [],
        defines: List[str] = [],
        parameters: Dict[str, str] = {},
        extra_args: Union[str, List[str]] = "",
        build_dir: Optional[Path] = None,
        env: Optional[Dict[str, str]] = None,
        work_lib_name: Optional[str] = None,
    ) -> Design:
        """
        Builds a design

        Builds design with toplevel entity *top* with the given *sources*.
        *sources* can be any kind of files the compiler can understand.
        Sets *defines*, *includes*, *parameters*, and simulator-specific *extra_args*
        during the build.
        Links *libraries*.
        The build is done in *build_dir* in the *env* environment.
        If *build_dir* is ``None``, an appropriate build directory is chosen to prevent
        collision with other builds.
        If *env* is ``None``, the callers environment is used.
        Environment isolation can be done by passing an empty dictionary ``{}``.
        """

    def test(
        self,
        design: Design,
        module: Union[str, List[Path]] = [],
        top: Optional[str] = None,
        toplevel_lang: Optional[str] = None,
        plusargs: List[str] = [],
        parameters: Dict[str, str] = {},
        extra_args: Union[str, List[str]] = "",
        testcase: Union[None, str, List[str]] = None,
        seed: Optional[int] = None,
        waves: bool = False,
        gui: bool = False,
        run_dir: Optional[Path] = None,
        env: Optional[Dict[str, str]] = None,
    ) -> Tuple[Path, int]:
        """
        Runs cocotb tests

        Runs tests on the *design* using the test *module*.
        Passes *plusargs*, *parameters*, and simulator-specific *extra_args* to the
        simulator.
        Optionally allows you to set several common switches like
        * testcase
        * seed
        * waves
        * gui
        Runs the simulation in *run_dir* in the *env* environment.
        If *run_dut* is ``None``, the current directory is used.
        If *env* is ``None``, the callers environment is used.
        Environment isolation can be done by passing an empty dictionary ``{}``.
        """

With an example usage for the simple_dff looking something like

import os
import pathlib

from cocotb.project import get_simulator_class

toplevel_lang = os.getenv("TOPLEVEL_LANG", "verilog")
sim = os.getenv("SIM", "icarus")
pwd = pathlib(__file__)

if toplevel_lang == "verilog":
    sources = [
        pwd / "dff.sv"
    ]
else:
    sources = [
        pwd / "dff.vhdl"
    ]

simulator = get_simulator_class(sim)()
compiled_design = simulator.build(top="dff", sources=sources)
results_xml, rc = simulator.test(
    design=compiled_design,
    module="test_dff",
    toplevel_lang=toplevel_lang
)

It might also be nice to return to the user not just the result.xml path, but the results as an object that could be easily checked with assert. I think this would play really well with a unittest-style pytest, where the build is done as a setup action.

What about returning a test results object (could be a namedtuple or something) that contains the result and results.xml path, possibly among other things?

Also, how are parameters/generics specified? Some simulators want those during build (and changing them requires re-building), others require them during the simulator run (and therefore can be changed without re-building). How can this be handled in a simulator-independent way? i.e. is there a clean way to support that without having to do X if the simulator is A, but Y if the simulator is B, etc.

Well that didn't quite go as planned. So what exactly is build_dir and what exactly is work_dir? I would presume the point of separate build step from test step is to build once and then test many, potentially even in parallel. So there needs to be a build dir and a sim_dir. Builds are done in the build dir and sims are done in the sim dir. I'll revert and push up a proposal (ktbarrett@e199f2e).

Well that didn't quite go as planned. So what exactly is build_dir and what exactly is work_dir? I would presume the point of separate build step from test step is to build once and then test many, potentially even in parallel. So there needs to be a build dir and a sim_dir. Builds are done in the build dir and sims are done in the sim dir. I'll revert and push up a proposal (ktbarrett@e199f2e).

I think it is OK how you propose. The original intention was to be able to execute in a different place but maybe like now is better.

After you add this I think will add one more test where we separate build and test and see how this works. Something like.

@pytest.fixture(scope="module")
def runner():

    runner = get_runner(sim)()

    runner.build(
    )

    return runner

def test_runner(runner):

    runner.test(
    )

Or even separate completely.

Admittedly late to the party but have you considered using Edalize for interfacing the EDA tools instead? That would save you from maintaining code for that. Or if there are some reasons against that, perhaps at least use the EDAM API for your runner implementations. That would allow for easy interoperability with other EDAM-speaking tools like FuseSoC, Edalize and TerosHDL

@olofk We are avoiding depending on an outside tool in this repo. I'd rather have integration into multiple other toolchains done out of repo. There were a couple attempts at integrating cocotb support into edalize and FuseSoC, but they were deemed unsatisfactory and never completed. Using another tool's project models is an option. How does EDAM significantly differ from PyEDAA's Project Model? IIRC you and Unai were talking about converging the two?

@olofk Few comments as far as I understand:

• We would like to have a light python runner especially for windows (issues with make etc.)
• We would like to make use of unit testing frameworks like pytest (also for parallel runs)
• It should be rather easy to integrate cocotb in other flows: https://docs.cocotb.org/en/stable/custom_flows.html
• Can help and full support using cocotb with FuseSoC, Edalize, ...

Fair enough. Totally understand the reluctance to depend on external projects. Hoping to make a new effort to work on cocotb integration from the Edalize side. IIUC the previous attempts have been a bit overcomplicated and it should pretty much just be to load cocotb as a VPI lib and set some env vars but I'll happily take up @themperek's offer for assistance since I'm still new to cocotb.

Re the EDAM API, it's just a data structure (could be python dicts/lists, yaml, json or whatever) that describes the design in terms of things like design files (verilog, vhdl etc), TCL scripts, parameters/generics, defines, VPI libs, toplevel(s) as well as tool-specific options. Kind of the same information that I have seen being put into cocotb makefiles. The idea is to have something mostly tool-agnostic that can then be turned into tool-specific scripts or project files (which is what Edalize does basically)

@ktbarrett Maybe we can try to get this in as it is and then work with smaller PRs on this?

I think this can come in now.