Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sim breaks when _importpath is [] #176

Open
growly opened this issue Aug 31, 2023 · 5 comments
Open

Sim breaks when _importpath is [] #176

growly opened this issue Aug 31, 2023 · 5 comments

Comments

@growly
Copy link
Collaborator

growly commented Aug 31, 2023

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
Copy link
Owner

Some more commentary to follow, but in short:

  • The empty-list import-path looks right to me
  • None is the special value for “this was not imported”
  • The empty list should mean “this was imported, in the ‘root’ namespace of the import”

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
Copy link
Owner

dan-fritchman commented Aug 31, 2023

... 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
Copy link
Owner

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.

tb: Instantiable

It also has an alias Tb with the upper-case T.
def Tb(self) -> "Module":

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:

  • Hdl21 does not "make up" signals for you, ever
  • SPICE always has one "made up" one: global ground, AKA node zero
  • Hence testbenches expose a single port, and vlsir.spice hooks it up to that global ground

@dan-fritchman
Copy link
Owner

Another thing that woulda helped is Vlsir/Vlsir#82

@dan-fritchman
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants