dan-fritchman / Hdl21

Hardware Description Library
BSD 3-Clause "New" or "Revised" License
60 stars 13 forks source link

Sim breaks when _importpath is [] #176

Open growly opened 11 months ago

growly commented 11 months ago

I import a Vlsir package to test it:

#!/usr/bin/env python3

import os
from hdl21.prefix import u, n, p, f
import hdl21 as h
import hdl21.primitives as primitives
import hdl21.sim as sim
import sky130_hdl21 as sky130
import vlsir.circuit_pb2 as vlsir_circuit
import vlsirtools.spice as spice
from pathlib import Path

PDK_PATH = Path(os.environ.get("PDK_ROOT")) / "sky130A"
sky130.install = sky130.Install(
    pdk_path=PDK_PATH,
    lib_path="libs.tech/ngspice/sky130.lib.spice",
    model_ref=""    # NOTE(aryap): This seems unused.
)

package = vlsir_circuit.Package()
with open('../build/sky130_mux.package.pb', 'rb') as f:
    package.ParseFromString(f.read())
ns = h.from_proto(package)
mux = ns.sky130_mux

@sim.sim
class Sky130MuxSim:

    @h.module
    class Tb:   # required to be named 'Tb'?
        # Seems to be a requirement of a 'Tb' to have a "single, scalar port".
        VSS = h.Port()

        s0, s1, s2 = h.Signals(3)
        s0_b, s1_b, s2_b = h.Signals(3)
        x0, x1, x2, x3, x4, x5, x6, x7 = h.Signals(8)
        y = h.Signal()

        dut = mux()(S0=s0,
            S0_B=s0_b,
            S1=s1,
            S1_B=s1_b,
            S2=s2,
            S2_B=s2_b,
            X0=x0,
            X1=x1,
            X2=x2,
            X3=x3,
            X4=x4,
            X5=x5,
            X6=x6,
            X7=x7,
            Y=y,
            VGND=VSS)

        vpulse = primitives.PulseVoltageSource(delay=50*p,
                                               v1=0,
                                               v2=1.8,
                                               period=4*n,
                                               rise=0,
                                               fall=0,
                                               width=2*n)
        vpulse(p=dut.VPWR, n=VSS)

        includes = sky130.install.include(h.pdk.Corner.TYP)

    tran = sim.Tran(tstop=4*n, tstep=1*p)

def main():
    options = spice.SimOptions(
        simulator=spice.SupportedSimulators.XYCE,
        fmt=spice.ResultFormat.SIM_DATA,
        rundir="./scratch"
    )
    #if not spice.xyce.available():
    #    print("spice is not available!")
    #    return

    results = Sky130MuxSim.run(options)

    print(results)

if __name__ == "__main__":
    main()

The generated netlist for Xyce is missing the subcircuit name for sky130_mux:

; Generated by `vlsirtools.XyceNetlister`
; 

.SUBCKT 
+ S0 S0_B S1 S1_B S2 S2_B X0 X1 X2 X3 X4 X5 X6 X7 Y VPWR VGND 
; No parameters

I tracked the problem down to this if branch in qualpath. The problem seems to be that, on import, _importpath is set to [] (empty list). Seems this should instead be interepted and stored as None. Anyway then this ends up so that the SimInput message contains a Package where the subcircuit has an empty name.

Am I missing something? Fix seems pretty straightforward.

dan-fritchman commented 11 months ago

Some more commentary to follow, but in short:

More generally: the Vlsir + Xyce combo hasn’t seen much action of late; I won’t be surprised if we find some trouble with other recent changes.

dan-fritchman commented 11 months ago

... But yeah, even if those _importpaths are right, qualname should be more like

    if getattr(mod, "_importpath", None) is not None:
        return mod._importpath + [mod.name] # <= don't forget that!
dan-fritchman commented 11 months ago

Related notes:

The paths we export, which eventually (can) become _importpaths, could get smarter.
Example:

import hdl21 as h 

m = h.Module(name="MyName")
print(m._importpath)

pkg = h.to_proto(m)
ns = h.from_proto(pkg)
print(ns.__main__.MyName._importpath)

Yields

None
['__main__']

The __main__ in that namespace-path is, well, pretty lame.
I recall thinking we should special-case that out (and maybe __init__.py as well).
I do not recall whether I ever tried, or hit any roadblocks.


Re:

@sim.sim
class Sky130MuxSim:

    @h.module
    class Tb:   # required to be named 'Tb'?

The Sim type has an attribute named tb (the testbench), lowercase.
https://github.com/dan-fritchman/Hdl21/blob/cf31f94e186cf28ffaf2078d1175ebfd0d9f6dc2/hdl21/sim/data.py#L373 It also has an alias Tb with the upper-case T.
https://github.com/dan-fritchman/Hdl21/blob/cf31f94e186cf28ffaf2078d1175ebfd0d9f6dc2/hdl21/sim/data.py#L395

You can name your testbench Module whatever you like - just assign it to either tb or Tb in the Sim. And in Python, declaring a nested class is similar to:

class WhateverNameYouLike:
  ...

class MySim:
  Tb = WhateverNameYouLike

There are quite a few examples of how to set those testbench attributes on the readme page:
https://github.com/dan-fritchman/Hdl21#spice-class-simulation

If they're not clear, it'd be great to add to those docs.


Re:

    class Tb:
        # Seems to be a requirement of a 'Tb' to have a "single, scalar port".
        VSS = h.Port()

That fact, in contrast, looks like it could be more clear in the docs.

In short:

dan-fritchman commented 11 months ago

Another thing that woulda helped is https://github.com/Vlsir/Vlsir/issues/82

dan-fritchman commented 10 months ago