Refactor Analog ports for compositional passive#463
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors analog ports toward the compositional “internal passive net” model as an initial step for issue #114, updating supporting infrastructure (footprints, KiCad import, and net naming) to understand ports that encapsulate a net Passive port.
Changes:
- Introduce
HasPassivePortand migrateAnalogSink/AnalogSourceto expose an internalnetPassive port. - Update footprint pinning and KiCad schematic import auto-adaptation to connect via the internal
net. - Update net naming to prune trailing
.netcomponents; adjust example netlists accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/Multimeter/Multimeter.svgpcb.js | Updates a renamed net to reflect the new .net/.port naming. |
| examples/Multimeter/Multimeter.net | Updates KiCad net name to match the refactor-driven naming change. |
| edg/electronics_model/VoltagePorts.py | Adjusts Voltage↔Analog adapter logic to use compositional net connectivity. |
| edg/electronics_model/test_kicad_import_blackbox.py | Updates expected exported path steps for the new internal net port shape. |
| edg/electronics_model/PassivePort.py | Adds HasPassivePort and refactors adapter type map initialization. |
| edg/electronics_model/NetlistGenerator.py | Prunes trailing net path component during net naming. |
| edg/electronics_model/KiCadSchematicBlock.py | Adds auto-adapt support for HasPassivePort boundary ports. |
| edg/electronics_model/GroundPort.py | Updates Ground→Analog adapter to connect via AnalogSource.net. |
| edg/electronics_model/CircuitBlock.py | Makes footprints accept HasPassivePort by pinning through .net. |
| edg/electronics_model/AnalogPort.py | Migrates analog link/ports to compositional passive net model and updates bridges/adapters. |
| edg/abstract_parts/MergedBlocks.py | Removes NetBlock inheritance and explicitly connects analog nets via .net. |
| edg/abstract_parts/IoController.py | Allows pin-mapping to accept HasPassivePort in addition to CircuitPort. |
| edg/abstract_parts/DummyDevices.py | Removes NetBlock usage and explicitly connects analog .net ports. |
Comments suppressed due to low confidence (2)
edg/electronics_model/AnalogPort.py:156
AnalogSinkrelies onHasPassivePortto defineself.net, but with the current base order (AnalogBasebeforeHasPassivePort)super().__init__()will resolve toPort.__init__and never callHasPassivePort.__init__. That meansAnalogSinkinstances may not have anetsub-port, even though later code assumes it exists (eg,AnalogLink.contents, bridges, adapters).
Fix by reordering bases (eg class AnalogSink(HasPassivePort, AnalogBase)) or explicitly initializing the mixin / moving net creation into AnalogSink.__init__.
class AnalogSink(AnalogBase, HasPassivePort):
bridge_type = AnalogSinkBridge
@staticmethod
def from_supply(
neg: Port[GroundLink],
edg/electronics_model/AnalogPort.py:234
- Same issue as
AnalogSink: withAnalogSource(AnalogBase, HasPassivePort),HasPassivePort.__init__is skipped due to MRO, soself.netmay never be created even though the rest of the code assumesAnalogSource.netexists.
Reorder bases (eg class AnalogSource(HasPassivePort, AnalogBase)) or explicitly initialize / define net in AnalogSource.__init__.
class AnalogSource(AnalogBase, HasPassivePort):
bridge_type = AnalogSourceBridge
@staticmethod
def from_supply(
neg: Port[GroundLink],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def __init__(self) -> None: | ||
| super().__init__() | ||
| self.net = self.Port(Passive()) |
There was a problem hiding this comment.
HasPassivePort defines __init__ that creates self.net, but when used as a mixin alongside another Port subclass its __init__ will only run if it appears before the other base in the MRO (or is called explicitly). As written, AnalogSink(AnalogBase, HasPassivePort) / AnalogSource(AnalogBase, HasPassivePort) will not execute HasPassivePort.__init__, so instances may not actually have a net sub-port at runtime.
Consider making HasPassivePort a non-Port mixin with an explicit helper (eg, _init_passive_port()), or enforce/require base ordering (eg class X(HasPassivePort, ...)) so super().__init__() reaches this initializer.
| self.net = self.Port(Passive()) | |
| self._init_passive_port() | |
| def _init_passive_port(self) -> None: | |
| if not hasattr(self, '_net'): | |
| self._net = self.Port(Passive()) | |
| @property | |
| def net(self) -> Passive: | |
| self._init_passive_port() | |
| return self._net | |
| @net.setter | |
| def net(self, value: Passive) -> None: | |
| self._net = value |
|
|
||
| # TODO might be an overestimate | ||
| self.assign(self.src.current_draw, self.dst.link().current_drawn) | ||
| self.connect(self.dst.net.adapt_to(VoltageSink(current_draw=self.dst.link().current_drawn)), self.src) |
There was a problem hiding this comment.
In VoltageSinkAdapterAnalogSource, switching self.src to VoltageSink.empty() and then connecting an additional VoltageSink (via dst.net.adapt_to(...)) does not propagate the analog-side current draw onto self.src.current_draw. Instead it creates another sink on the same VoltageLink, so VoltageLink.current_drawn = sum(sinks.current_draw) will still include the (empty/unconstrained) self.src sink, which can dominate/absorb the sum and effectively drop the intended dst.link().current_drawn contribution.
To preserve current draw accounting, keep (or reintroduce) an explicit assignment onto self.src.current_draw (as before), or otherwise ensure the adapter does not add an extra independent sink with an unconstrained current_draw into the same VoltageLink.
| self.connect(self.dst.net.adapt_to(VoltageSink(current_draw=self.dst.link().current_drawn)), self.src) | |
| self.assign(self.src.current_draw, self.dst.link().current_drawn) # preserve current accounting on src |
| super().__init__() | ||
| self.src = self.Port(Ground()) | ||
| self.src = self.Port(Ground.empty()) | ||
| self.dst = self.Port( | ||
| AnalogSource( | ||
| voltage_out=self.src.link().voltage, | ||
| signal_out=self.src.link().voltage, | ||
| ) | ||
| ) | ||
| self.connect(self.dst.net.adapt_to(Ground()), self.src) | ||
|
|
There was a problem hiding this comment.
GroundAdapterAnalogSource now uses Ground.empty() for self.src and then connects self.dst.net.adapt_to(Ground()) to it. This introduces a second Ground port onto the same GroundLink.gnds vector in addition to self.src, so the effective voltage_limits intersection is now influenced by an extra Ground() instance rather than just the adapter's src.
If the goal is only to tie copper connectivity between src and dst.net, consider ensuring you don't add an extra independent Ground into gnds (eg, keep self.src = Ground() and connect dst.net in a way that doesn't create a second ground constraint source, or explicitly align the adapted ground's parameters to self.src).
| def prune_net_component(path: TransformUtil.Path) -> TransformUtil.Path: | ||
| # prune out the net interior link, if it exists | ||
| if path.ports and len(path.ports) > 1 and path.ports[-1] == "net": | ||
| return path._replace(ports=path.ports[:-1]) | ||
| elif path.links and len(path.links) > 1 and path.links[-1] == "net": | ||
| return path._replace(links=path.links[:-1]) | ||
| else: | ||
| return path | ||
|
|
||
| net = map(prune_net_component, net) |
There was a problem hiding this comment.
name_net now prunes a trailing .net path component, but there’s no unit test exercising this new behavior (the existing test_netlist_naming only checks ordering). Adding a small test case that includes a path ending in ...port.net (and/or ...link.net) would help prevent regressions in net naming as more ports move to the compositional net model.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| from typing_extensions import override | ||
|
|
||
| from .PassivePort import PassiveAdapterGround |
There was a problem hiding this comment.
PassiveAdapterGround is imported but never used in this module. If it’s not needed for side effects, remove the import to avoid lint failures / dead code.
| from .PassivePort import PassiveAdapterGround |
| from .AnalogPort import AnalogSource, AnalogSink | ||
|
|
||
| super().__init__() | ||
| self.src = self.Port(Passive()) | ||
| self.dst = self.Port( | ||
| AnalogSource( | ||
| voltage_out=voltage_out, signal_out=signal_out, current_limits=current_limits, impedance=impedance | ||
| ) | ||
| ) | ||
| self.connect(self.src, self.dst.net) |
There was a problem hiding this comment.
PassiveAdapterAnalogSource.__init__ imports AnalogSink but doesn’t use it. If not required for side effects, drop the unused import to keep the module clean and avoid lint issues.
| @@ -190,6 +211,7 @@ def __init__( | |||
| impedance=impedance, | |||
| ) | |||
| ) | |||
| self.connect(self.src, self.dst.net) | |||
There was a problem hiding this comment.
PassiveAdapterAnalogSink.__init__ imports AnalogSource but doesn’t use it. Remove the unused import unless it’s needed for side effects.
First step and example plan for #114, to move single-circuit ports to a compositional instead of inheritance model. This changes AnalogSink, AnalogSource to have an internal
netsub-port, which is the actual net.Introduces an infrastructural HasPassivePort, which effectively fulfills the same function as (inheritance-based) CirucitPort, but with composition.
Footprints implicitly understand HasPassivePort and automatically connect the internal net.
For netlister net naming, the interior net name of ports is pruned out.
Refactors all the adapt_to(Analog*) to create the fully-defined Analog* port in the wrapper, then connect its net to the internal passive-typed port. The leftover adapt_to(Analog*) cases are adapting within a block (eg, to connect to a peer-level sub-block) and cannot be removed. This probably forms the new recommended practice.
Future PRs: