devbisme / skidl

SKiDL is a module that extends Python with the ability to design electronic circuits.
https://devbisme.github.io/skidl/
MIT License
1.06k stars 119 forks source link

[SKiDL BUG] Packages don't play nicely with Circuits #86

Open jbayless opened 4 years ago

jbayless commented 4 years ago

Describe the bug The new @package decorator seems very exciting, but it appears to assume the use of the global default_circuit object. I aim to write code that modifies as little global state as possible, so I've been using Circuit objects that I create. But they don't seem to be interoperable with packages, or at least, I apparently don't know how to do so.

To Reproduce I've attempted several methods of using packages with circuits. I'll enumerate through the ones I've tried.

1) Pass a circuit as a keyword argument to the package, without any corresponding keyword argument in the function definition.

#-------------#
# METHOD 1 #
#-------------#
import skidl as sk

@sk.package
def analog_average(in1, in2, avg):
    r1, r2 = 2 * sk.Part('Device', 'R', value='1K', dest=sk.TEMPLATE)
    r1[1,2] += in1, avg
    r2[1,2] += in2, avg

if __name__ == "__main__":
    cct = sk.Circuit(name = "My circuit")

    avg1 = analog_average(circuit = cct)
    avg2 = analog_average(circuit = cct)

    print("Circuit parts:")
    print(cct.parts)

    in1, in2, in3, in4, out1, out2 = sk.Net(circuit = cct)*6
    avg1['in1'] += in1
    avg1.in2    += in2
    avg1['avg'] += out1

    avg2['in1'] += in3
    avg2['in2'] += in4
    avg2.avg    += out2

    cct.generate_netlist()

Output:

Circuit parts:
[]
ValueError: Can't attach nets in different circuits (My circuit, )!

2) Add a "circuit" keyword argument to the function definition:

#-------------#
# METHOD 2 #
#-------------#
# (Only this function definition is changed; otherwise the rest of this code is the same as above
@sk.package
def analog_average(in1, in2, avg, circuit = None):
    r1, r2 = 2 * sk.Part('Device', 'R', value='1K', dest=sk.TEMPLATE, circuit = circuit)
    r1[1,2] += in1, avg
    r2[1,2] += in2, avg

Output:

Circuit parts:
[]
ValueError: Can't attach nets in different circuits (My circuit, )!

3) Add a differently-named keyword argument to the function definition and pass that to the package components as the circuit:

#-------------#
# METHOD 3 #
#-------------#
import skidl as sk

@sk.package
def analog_average(in1, in2, avg, cct = None):
    r1, r2 = 2 * sk.Part('Device', 'R', value='1K', dest=sk.TEMPLATE, circuit = cct)
    r1[1,2] += in1, avg
    r2[1,2] += in2, avg

if __name__ == "__main__":
    cct = sk.Circuit(name = "My circuit")

    avg1 = analog_average(circuit = cct)
    avg2 = analog_average(circuit = cct)
    print("Circuit parts:")
    print(cct.parts)

    in1, in2, in3, in4, out1, out2 = sk.Net(circuit = cct)*6

    # Make connections. You can use either [] or . to reference the I/O.
    avg1['in1'] += in1
    avg1.in2    += in2
    avg1['avg'] += out1

    avg2['in1'] += in3
    avg2['in2'] += in4
    avg2.avg    += out2
    cct.generate_netlist()

Output:

Circuit parts:
[]
ValueError: Can't attach nets in different circuits (My circuit, )!

4) Use the += operators to add packages to a circuit, the way you can for parts. But since those parts don't actually seem to be in the circuit, I can't reference them, so I don't try to make connections here...

#-------------#
# METHOD 4 #
#-------------#
import skidl as sk

@sk.package
def analog_average(in1, in2, avg):
    r1, r2 = 2 * sk.Part('Device', 'R', value='1K', dest=sk.TEMPLATE)
    r1[1,2] += in1, avg
    r2[1,2] += in2, avg

if __name__ == "__main__":
    cct = sk.Circuit(name = "My circuit")

    cct += analog_average()
    cct += analog_average()

    in1, in2, in3, in4, out1, out2 = sk.Net(circuit = cct)*6

    cct.generate_netlist()
    print("Circuit parts:")
    print(cct.parts)

Outputs:

No errors or warnings found during netlist generation.

Circuit parts:
[]

(But the netlist it outputs has no nets and no components).

5) Same as method 2, but this time forget to pass the circuit argument to the Nets:

#-------------#
# METHOD 5 #
#-------------#
import skidl as sk

@sk.package
def analog_average(in1, in2, avg, circuit = None):
    r1, r2 = 2 * sk.Part('Device', 'R', value='1K', dest=sk.TEMPLATE, circuit = circuit)
    r1[1,2] += in1, avg
    r2[1,2] += in2, avg

if __name__ == "__main__":
    cct = sk.Circuit()

    avg1 = analog_average(circuit = cct)
    avg2 = analog_average(circuit = cct)
    print("Circuit parts:")
    print(cct.parts)

    in1, in2, in3, in4, out1, out2 = sk.Net()*6

    avg1['in1'] += in1
    avg1.in2    += in2
    avg1['avg'] += out1

    avg2['in1'] += in3
    avg2['in2'] += in4
    avg2.avg    += out2

    cct.generate_netlist()

Output:

Circuit parts:
[]
Traceback (most recent call last):
    cct.generate_netlist()
  File "skidl\skidl\Circuit.py", line 399, in generate_netlist
    self.instantiate_packages()
  File "skidl\skidl\Circuit.py", line 345, in instantiate_packages
    package.subcircuit(**package)
  File "skidl\skidl\Circuit.py", line 655, in sub_f
    builtins.NC = default_circuit.NC  # pylint: disable=undefined-variable
AttributeError: 'NoneType' object has no attribute 'NC'

Expected behavior I'd expect at least one of the above methods to produce similar results as I'd get from the following code:

#-------------#
# METHOD 6  #
#-------------#
import skidl as sk

@sk.package
def analog_average(in1, in2, avg):
    r1, r2 = 2 * sk.Part('Device', 'R', value='1K', dest=sk.TEMPLATE)
    r1[1,2] += in1, avg
    r2[1,2] += in2, avg

if __name__ == "__main__":
    avg1 = analog_average()
    avg2 = analog_average()

    in1, in2, in3, in4, out1, out2 = sk.Net()*6

    avg1['in1'] += in1
    avg1.in2    += in2
    avg1['avg'] += out1

    avg2['in1'] += in3
    avg2['in2'] += in4
    avg2.avg    += out2

    sk.generate_netlist()

which produces the following netlist:

(export (version D)
  (design
    (source "scratchpad.py")
    (date "06/24/2020 09:23 PM")
    (tool "SKiDL (0.0.30)"))
  (components
    (comp (ref R1)
      (value 1K)
      (footprint "No Footprint")
      (fields
        (field (name F0) R)
        (field (name F1) R))
      (libsource (lib Device) (part R))
      (sheetpath (names /top/analog_average0/15418885917532110859) (tstamps /top/analog_average0/15418885917532110859)))
    (comp (ref R2)
      (value 1K)
      (footprint "No Footprint")
      (fields
        (field (name F0) R)
        (field (name F1) R))
      (libsource (lib Device) (part R))
      (sheetpath (names /top/analog_average0/6426300460454210508) (tstamps /top/analog_average0/6426300460454210508)))
    (comp (ref R3)
      (value 1K)
      (footprint "No Footprint")
      (fields
        (field (name F0) R)
        (field (name F1) R))
      (libsource (lib Device) (part R))
      (sheetpath (names /top/analog_average1/10466459675231932201) (tstamps /top/analog_average1/10466459675231932201)))
    (comp (ref R4)
      (value 1K)
      (footprint "No Footprint")
      (fields
        (field (name F0) R)
        (field (name F1) R))
      (libsource (lib Device) (part R))
      (sheetpath (names /top/analog_average1/17614385574181552080) (tstamps /top/analog_average1/17614385574181552080))))
  (nets
    (net (code 0) (name N$1_1)
      (node (ref R1) (pin 1)))
    (net (code 1) (name N$1_2)
      (node (ref R2) (pin 1)))
    (net (code 2) (name N$1_3)
      (node (ref R3) (pin 1)))
    (net (code 3) (name N$1_5)
      (node (ref R1) (pin 2))
      (node (ref R2) (pin 2)))
    (net (code 4) (name N$6)
      (node (ref R4) (pin 1)))
    (net (code 5) (name N$7)
      (node (ref R3) (pin 2))
      (node (ref R4) (pin 2))))
)

Desktop (please complete the following information):

xesscorp commented 4 years ago

Thanks for the detailed report! I'll take a look at it.

xesscorp commented 4 years ago

Well, you tried just about every possible variation to make this work! I made some fixes so that methods #1 and #2 now work. You can also use method #3, but you have to change the package calls to avg1 = analog_average(circuit=cct, cct=cct) which seems rather pointless.

You can install the latest changes using:

pip install git+https://github.com/xesscorp/skidl@development

I'm still working on this. I think I can make Method #4 work.