I am currently trying to better my understanding of python and good coding practices and would really like some input on a question i have been thinking over for weeks now.
I am currently working on building a virtual microprocessor out of basic logic gates (And, Or,...). The idea is to assemble more and more complex components out of parts that already exist. Once a component is created, i can reuse it whenever needed.
Almost all components are structured in the same way. They take one or more inputs and use these to create some inner logic which results in one or more outputs.
This component for example takes an 8-bit input (in_data
) and inverts all its bits if another input (in_invert
) carries a current:
class OnesComplement(IntegratedComponent):
def __init__(self,
in_data: list[ElectricComponent] = None,
in_invert: ElectricComponent = None):
self.in_data = in_data if in_data is not None else [
LooseWire() for x in range(8)]
self.in_invert = in_invert if in_invert is not None else LooseWire()
self.out_main = [XOR(inp, self.in_invert) for inp in self.in_data]
Some inputs might not be available at initialization. In that a case one or more instances of LooseWire
is created in order to keep track of all the components that depend on the missing input. This is also one of the reasons why the inputs are registered as instance attributes.
This code works perfectly fine, but it is very repetitive. In the example above, only a single line of code is used to implement the logic of the component. Everything else is just for setting defaults and registering instance attributes. This is annoying because I am writing a lot of components. It also leads to copy/paste errors and fails to highlight the occasional processing of non-input parameters.
As a way around this I tried using a decorator:
from functools import wraps
def _lwd(n=1):
"""
ONLY for use with the @autoparse decorator.
LooseWires are temporary and will be exchanged by the decorator.
"""
if n == 1:
return LooseWire()
else:
return [LooseWire() for _ in range(n)]
def autoparse(init):
parnames = init.__code__.co_varnames[1:]
defaults = init.__defaults__
@wraps(init)
def wrapped_init(self, *args, **kwargs):
# Turn args into kwargs
kwargs.update(zip(parnames[:len(args)], args))
# apply default parameter values
default_start = len(parnames) - len(defaults)
for i in range(len(defaults)):
if parnames[default_start + i] not in kwargs:
kwargs[parnames[default_start + i]] = defaults[i]
# generate new instance for each LooseWire
for arg in kwargs:
if isinstance(kwargs[arg], LooseWire):
kwargs[arg] = LooseWire()
if isinstance(kwargs[arg], list):
for i in range(len(kwargs[arg])):
if isinstance(kwargs[arg][i], LooseWire):
kwargs[arg][i] = LooseWire()
# attach attributes to instance
for arg in kwargs:
setattr(self, arg, kwargs[arg])
init(self, **kwargs)
return wrapped_init
This allows me to write the component from above like this:
class OnesComplement(IntegratedComponent):
@autoparse
def __init__(self,
in_in: list[ElectricComponent] = _lwd(8),
in_invert: ElectricComponent = _lwd()):
self.out_main = [XOR(inp, self.in_invert) for inp in self.in_in]
I feel like this solves all of my problems. It is concise and also highlights the width of the input. However, i am worried about introducing some unwanted side-effects that i am not thinking about. Is it safe to decorate the __init__
like that? And if so, would it be considered good practice?
edit: more code as requested
code for the base class ElectricComponent
:
class ElectricComponent:
"""
Parent class for all components of the electric circuit
"""
def connect_input(self, input_name: str,
input_circuit: 'ElectricComponent'):
"""
Should be used if not all inputs were available at initialization.
Replaces LooseWires.
"""
old_input = getattr(self, input_name)
islist = isinstance(old_input, list)
if not islist:
old_input = [old_input]
input_circuit = [input_circuit]
for i in range(len(input_circuit)):
for fc in old_input[i].forward_connections:
setattr(fc[0], fc[1], input_circuit[i])
input_circuit[i].add_connection(fc[0], fc[1])
fc[0].update()
if not islist:
input_circuit = input_circuit[0]
# Not functional, just for tracking
setattr(self, input_name, input_circuit)
There are two types of components that inherit from ElectricComponent
: CoreComponent
and IntegratedComponent
.
Most of the logic in maintaining connections and propagating state changes is handled by CoreComponent
s. They have their own state, stored as a bool in self.out_main.
class CoreComponent(ElectricComponent):
"""
Parent Class for all core components
These are the basic buidling blocks that all
other components are assembled from
All core components have a singlaur output-line
called 'out_main'
"""
def setup(self):
self.forward_connections = []
self.build_circuit()
self.compute_state()
def get_state(self):
"""Returns the current state of the output"""
return self.out_main
is_on = property(get_state)
def add_connection(self, con, port):
"""Called by downstream elements to add them as a forward connection"""
if (con, port) not in self.forward_connections:
self.forward_connections.append((con, port))
def forward_pass(self):
for fc in self.forward_connections:
fc[0].update()
def update(self):
old_state = self.out_main
self.compute_state()
if self.out_main != old_state:
self.forward_pass()
def __str__(self):
return str(int(self.out_main))
All CoreComponents
(for now):
class Switch(CoreComponent):
"""
Simple Switch. It is used to control a gate.
When the gate is closed it carries a current.
"""
def __init__(self, closed: bool = False):
self.out_main = closed
self.setup()
def build_circuit(self):
pass
def compute_state(self):
pass
def flip(self):
self.out_main = not self.out_main
self.forward_pass()
def open(self):
self.out_main = False
self.forward_pass()
def close(self):
self.out_main = True
self.forward_pass()
class LooseWire(CoreComponent):
"""
This component is used solely for initializing unconnected inputs
It never carries a current
"""
def __init__(self):
self.setup()
def build_circuit(self):
pass
def compute_state(self):
self.out_main = False
class BaseGate(CoreComponent):
"""
Parent for the baisc logic gates
"""
def __init__(self, in_a: ElectricComponent = None,
in_b: ElectricComponent = None):
self.in_a = in_a if in_a is not None else LooseWire()
self.in_b = in_b if in_b is not None else LooseWire()
self.setup()
def build_circuit(self):
self.in_a.add_connection(self, 'in_a')
self.in_b.add_connection(self, 'in_b')
class INV(CoreComponent):
"""Inverts the input"""
def __init__(self, in_a: ElectricComponent = None):
self.in_a = in_a if in_a is not None else LooseWire()
self.setup()
def build_circuit(self):
self.in_a.add_connection(self, 'in_a')
def compute_state(self):
self.out_main = True
if self.in_a.is_on:
self.out_main = False
class AND(BaseGate):
"""AND-Gate"""
def compute_state(self):
self.out_main = False
if self.in_a.is_on:
if self.in_b.is_on:
self.out_main = True
class OR(BaseGate):
"""OR-Gate"""
def compute_state(self):
self.out_main = False
if self.in_a.is_on:
self.out_main = True
if self.in_b.is_on:
self.out_main = True
class NOR(BaseGate):
"""NOR-Gate"""
def compute_state(self):
self.out_main = True
if self.in_a.is_on:
self.out_main = False
if self.in_b.is_on:
self.out_main = False
class NAND(BaseGate):
"""NAND-Gate"""
def compute_state(self):
self.out_main = True
if self.in_a.is_on:
if self.in_b.is_on:
self.out_main = False
The other class that inherits from ElectricComponent
is IntegratedComponent
. Components of this type are assembled out of CoreComponents
. Most of the components i build are of this type. They do not posses a state of their own. The class itself is empty for now. It used to contain some methods i factored out but i am keeping it around for structuring and future use.
There are too many IntegratedComponent
s to post them all. One example of an IntegratedComponent
would be the OnesComplement
i originally posted. Another is the XOR-gate which is used in the OnesComplement
class IntegratedComponent(ElectricComponent):
pass
class IntegratedLogicGate(IntegratedComponent):
# TODO: think about moving this to a class for all
# IntegratedComponents with a single 'out_main'
"""
This allows for its children to be used
like basegates and other core components
and1 = AND(s1)
xor1 = XOR(s1, s2)
xor.out_main.is_on can be written as
xor.is_on
and1.connect_input('in_b', xor1.out_main)
can be written as if XOR were a CoreComponent
and1.connect_input('in_b', xor1)
"""
def get_state(self):
return self.out_main.get_state()
is_on = property(get_state)
def add_connection(self, con, port):
self.out_main.add_connection(con, port)
class XOR(IntegratedLogicGate):
"""XOR-Gate"""
def __init__(self, in_a: ElectricComponent = None,
in_b: ElectricComponent = None):
self.in_a = in_a if in_a is not None else LooseWire()
self.in_b = in_b if in_b is not None else LooseWire()
self.or1 = OR(self.in_a, self.in_b)
self.nand1 = NAND(self.in_a, self.in_b)
self.out_main = AND(self.or1, self.nand1)
One example of a component that needs a LooseWire
because it has its own output as an input is the RSFlipFlop
class RSFlipFlop(IntegratedComponent):
def __init__(self,
in_r: ElectricComponent = None,
in_s: ElectricComponent = None):
self.in_r = in_r if in_r is not None else LooseWire()
self.in_s = in_s if in_s is not None else LooseWire()
self.nor1 = NOR(self.in_r)
self.nor2 = NOR(self.nor1, self.in_s)
self.nor1.connect_input("in_b", self.nor2)
self.out_q = self.nor1
self.out_qb = self.nor2
edit 2:
I should have been more clear as to what my goals for this project are. I am currently reading trough Charles Petzold's book Code: The Hidden Language of Hardware and Software. In it, he is shows how to assemble a microprocessor out of basic logic gates constructed from relays and switches. I am trying to replicate this virtually. The 5 logic gates I use as core components are the 5 gates he uses as building blocks in his book. This is also why those gates only have two inputs and why i chose nested if-clauses for them. I can't use logic operators before building them myself.
-
\$\begingroup\$ @Reinderien I added more code as requested \$\endgroup\$LoePhi– LoePhi2021年11月01日 10:36:24 +00:00Commented Nov 1, 2021 at 10:36
1 Answer 1
In no particular order:
- Your input model is too complicated. Don't
isinstance(old_input, list)
; just assume that every component has aCollection[]
(sized, iterable, unordered) of (potentially only one) input. - Similarly, your output model is sometimes too complicated. There's not a lot of benefit in representing your
OnesComplement
as a byte bus - just represent it as a single bit (which really just evaluates to aXOR
). If you want to generalize an arbitrary gate to a byte bus you should do that separately, possible for all components, not justOnesComplement
. CoreComponent
is effectively an abstract class due tocompute_state
not being defined - fine. However,build_circuit(self): pass
should exist onCoreComponent
itself, andcompute_state
should also be declared there ascompute_state(self): raise NotImplementedError()
.- Relying on
getattr
/setattr
as much as this code does is a code smell. - In most cases, it doesn't benefit you to hard-code
in_a
/in_b
. It's less work for an OR, AND etc. to just accept an arbitrary number of inputs. - Don't auto-convert
in_a
from aNone
to aLooseWire
; just accept a component that can never beNone
. Also, yourin_a
annotation is currently incorrect because it's missing anOptional[]
. Further: you probably shouldn't allow for aLooseWire
at all. build_circuit
andadd_connection
probably shouldn't exist. You should just be able to assume that your inputs are your connections.- As to your original question, no, your decorator is really not a good idea. Magic conversion of constructor parameters is more complicated and difficult to understand than it should be.
State and forward propagation
All but two of your components - RSFlipFlop
and Switch
- should be stateless - just as they are in real life. As such, it is not a good idea to hold a self.out_main
. Just define an evaluation function, basically your compute_state
, that returns a boolean.
It's not a good idea to keep forward_connections
, at least in its current form. In real life, the behaviour of a component should not rely on the components after it.
Since you're encountering performance issues, it is possible to be a little more clever (I have not shown that in the suggested code):
- Approach this in a general manner, keeping only one boolean output: this includes your FF, where you can simplify its model to only have non-inverting output. If you need an inverter afterward, keep that as a separate component.
- Cache the old value of the output, similar to your
out_main
. - Only forward-propagate changes if the old value is not equal to the new value. You had already done this (albeit a little inconsistently) in
if self.out_main != old_state
. - Use
__slots__
. - Consider finalizing a circuit using run-time code objects and compile.
Suggested code
from functools import reduce
from operator import xor
from sys import maxsize
from typing import ClassVar, Collection, Callable, Iterable, List
class Component:
"""
Parent class for all components of the electric circuit
"""
def __init__(self) -> None:
self.change_hooks: List[Callable[[], None]] = []
@property
def out(self) -> bool:
raise NotImplementedError()
def __bool__(self) -> bool:
return self.out
def changed(self) -> None:
for hook in self.change_hooks:
hook()
class InputComponent(Component):
def __init__(self, inputs: Iterable['Component']) -> None:
super().__init__()
for input in inputs:
input.change_hooks.append(self.changed)
class InputCollectionComponent(InputComponent):
MIN_INPUTS: ClassVar[int]
MAX_INPUTS: ClassVar[int]
def __init__(self, inputs: Collection['Component']) -> None:
super().__init__(inputs)
if not (self.MIN_INPUTS <= len(inputs) <= self.MAX_INPUTS):
raise ValueError(f'{len(inputs)} inputs for {type(self).__name__} '
f'not between {self.MIN_INPUTS}-{self.MAX_INPUTS}')
self.inputs = inputs
class Switch(Component):
def __init__(self, closed: bool) -> None:
super().__init__()
self.closed = closed
def flip(self) -> None:
self.closed = not self.closed
self.changed()
@property
def out(self) -> bool:
return self.closed
class INV(InputComponent):
def __init__(self, input: Component) -> None:
super().__init__((input,))
self.input = input
@property
def out(self) -> bool:
return not self.input
class Gate(InputCollectionComponent):
MIN_INPUTS = 2
MAX_INPUTS = maxsize
class AND(Gate):
@property
def out(self) -> bool:
return all(self.inputs)
class OR(Gate):
@property
def out(self) -> bool:
return any(self.inputs)
class NOR(Gate):
@property
def out(self) -> bool:
return not any(self.inputs)
class NAND(Gate):
@property
def out(self) -> bool:
return not all(self.inputs)
class XOR(Gate):
@property
def out(self) -> bool:
return reduce(xor, self.inputs, False)
class RSFlipFlop(InputComponent):
def __init__(self, *, in_r: Component, in_s: Component) -> None:
super().__init__((in_r, in_s))
self.in_r, self.in_s = in_r, in_s
self.update()
@property
def out(self) -> bool:
return self.state
def update(self) -> None:
self.state = self.in_s.out and not self.in_r.out
def changed(self) -> None:
self.update()
super().changed()
class OnesComplement(XOR):
def __init__(self, in_in: Component, in_invert: Component) -> None:
super().__init__((in_in, in_invert))
def test() -> None:
swa = Switch(closed=False)
swb = Switch(closed=False)
or_ = OR((swa, swb))
ff = RSFlipFlop(in_r=swa, in_s=or_)
assert not swa
assert not swb
assert not or_
assert not ff
swb.flip()
assert not swa
assert swb
assert or_
assert ff
if __name__ == '__main__':
test()
-
\$\begingroup\$ Thank you for taking the time to read trough this and respond. I very much look forward to your suggested implementation. Fwiw, I did start out without forward connections and everything being stateless, but as the circuits grew bigger this started to become slow because i was recursing trough the whole circuit for every bit in the output. This is why i introduced the forward connections. The idea was that a change in component A would only affect the components (forward_connections) that depend on the state of component A and that forward propagation would stop for unaffected components. \$\endgroup\$LoePhi– LoePhi2021年11月01日 16:51:14 +00:00Commented Nov 1, 2021 at 16:51
-
\$\begingroup\$ Thanks again. Many good pointers. I learned a lot from this and will try my hands at a recursive solution again. Unfortunately your code does not really work for what i'm trying to achieve (see my latest edit). I'm sorry i wasn't more clear about that. \$\endgroup\$LoePhi– LoePhi2021年11月02日 08:23:21 +00:00Commented Nov 2, 2021 at 8:23
-
\$\begingroup\$ @LoePhi That's.. fine, but if you want a review that takes the new information into account you're going to have to repost. \$\endgroup\$Reinderien– Reinderien2021年11月02日 13:17:12 +00:00Commented Nov 2, 2021 at 13:17