framework: LED.cathode mandatory + close WaterAlarm regression - #31
Merged
Conversation
Found while reviewing WaterAlarm's breadboard SVG: the LEDs in the
design had no ground connections drawn. Investigation revealed
`LED.cathode` was declared `mandatory=False` with the docstring claim
*"floating = GND"* — a "structurally inert under graph evaluation"
shortcut that lets demos write electrically-broken circuits and have
them pass ERC.
A real LED with a dangling cathode does not light. This is exactly
the kind of physical-fidelity violation `CLAUDE.md` lists as
disallowed.
- `src/components/passives/led.py`: `cathode` becomes
`mandatory=True`. Docstring updated to drop the "floating = GND"
claim and explain the contract.
- `tests/components/test_led.py`: regression tests pin both ports
as mandatory and confirm `Circuit.__init__` raises
`UnconnectedPinError` end-to-end when a cathode is left
floating.
- `demos/water_alarm/water_alarm.py` and
`demos/water_alarm_split/water_alarm_split.py`: cathodes wired
to the GND rail. (Bench builds still need a series current-limit
resistor; the logic-level demo doesn't model V_F or current.)
- `tests/framework/test_circuit_autocollect.py`,
`tests/framework/test_framework.py`, and
`tests/framework/import_kicad/test_import.py`: fixtures that
leaned on the broken default updated so they still pin down the
specific error each test exists to catch.
- Demo docs regenerated for the two WaterAlarm variants.
raeq
force-pushed
the
framework-led-cathode-mandatory
branch
from
May 19, 2026 16:32
a265d46 to
2f70dab
Compare
3 tasks
There was a problem hiding this comment.
Pull request overview
This PR tightens electrical correctness in the component framework by making LED.cathode a required connection, preventing physically invalid “floating cathode” LED circuits from passing ERC and propagating into exports/docs (e.g., the WaterAlarm demo).
Changes:
- Make
LED.cathodemandatory (removing the prior “floating = GND” shortcut) soCircuitconstruction raisesUnconnectedPinErrorfor dangling cathodes. - Fix WaterAlarm / WaterAlarmSplit demos by explicitly wiring LED cathodes to ground, and regenerate their exported docs/assets.
- Add regression tests ensuring LEDs require both terminals and that failures occur at
Circuit.__init__.
Reviewed changes
Copilot reviewed 41 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/golden/yosys/water_alarm.json | Golden updated: LED cathodes now land on GND net. |
| tests/golden/yosys/water_alarm_assembly.json | Golden updated: assembly variant cathodes now on GND. |
| tests/golden/spice/water_alarm.cir | Golden updated: LED cathodes now SPICE node 0 (ground). |
| tests/golden/spice/water_alarm_assembly.cir | Golden updated: assembly variant LED cathodes now SPICE ground. |
| tests/golden/mermaid/water_alarm.mmd | Golden updated: Mermaid graph shows cathode→gnd edges. |
| tests/golden/mermaid/water_alarm_assembly.mmd | Golden updated: assembly Mermaid graph shows cathode→gnd edges. |
| tests/golden/kicad/water_alarm.net | Golden updated: KiCad netlist includes LED cathodes on GND net. |
| tests/golden/kicad/water_alarm_assembly.net | Golden updated: assembly KiCad netlist includes cathodes on GND. |
| tests/golden/kicad_sch/water_alarm.kicad_sch | Golden updated: schematic adds GND power symbols for LED cathodes. |
| tests/golden/dot/water_alarm.dot | Golden updated: DOT graph includes cathode→gnd edges. |
| tests/golden/dot/water_alarm_assembly.dot | Golden updated: assembly DOT graph includes cathode→gnd edges. |
| tests/golden/breadboard/water_alarm.breadboard.svg | Golden updated: breadboard SVG adds two ground jumpers for LED cathodes. |
| tests/golden/assembly_guide/water_alarm.md | Golden updated: assembly steps include LED cathode→rail jumpers. |
| tests/framework/test_framework.py | Update evaluation-order test to wire LED cathode and include a GND rail part. |
| tests/framework/test_circuit_autocollect.py | Update fixtures to avoid floating LED cathodes and adjust expected collected parts. |
| tests/framework/import_kicad/test_import.py | Update minimal-netlist test to include explicit power/ground nets to satisfy mandatory LED ports. |
| tests/components/test_led.py | Add regression tests for mandatory anode/cathode and construction-time failure on dangling cathode. |
| src/components/passives/led.py | Make cathode port mandatory and update docstring to reflect physical behavior. |
| demos/water_alarm/water_alarm.py | Wire both LED cathodes to GND in the demo circuit. |
| demos/water_alarm/docs/WaterAlarm.yosys.json | Regenerated export reflecting cathodes connected to GND. |
| demos/water_alarm/docs/WaterAlarm.svg | Regenerated diagram reflecting cathodes connected to GND. |
| demos/water_alarm/docs/WaterAlarm.net-report.md | Regenerated net report showing LED cathodes as GND readers. |
| demos/water_alarm/docs/WaterAlarm.net | Regenerated KiCad netlist showing LED cathodes on GND. |
| demos/water_alarm/docs/WaterAlarm.mmd | Regenerated Mermaid showing cathode→gnd edges. |
| demos/water_alarm/docs/WaterAlarm.md | Regenerated assembly guide including LED cathode ground jumpers. |
| demos/water_alarm/docs/WaterAlarm.kicad_sch | Regenerated schematic adding GND symbols for LED cathodes. |
| demos/water_alarm/docs/WaterAlarm.dot | Regenerated DOT graph including cathode→gnd edges. |
| demos/water_alarm/docs/WaterAlarm.cir | Regenerated SPICE netlist with LED cathodes on node 0. |
| demos/water_alarm/docs/WaterAlarm.breadboard.svg | Regenerated breadboard SVG adding cathode ground jumpers. |
| demos/water_alarm_split/water_alarm_split.py | Wire both LED cathodes to GND in split demo. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.yosys.json | Regenerated export reflecting cathodes connected to GND. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.svg | Regenerated diagram reflecting cathodes connected to GND. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.net-report.md | Regenerated net report showing LED cathodes as GND readers. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.net | Regenerated KiCad netlist showing LED cathodes on GND. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.mmd | Regenerated Mermaid showing cathode→gnd edges. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.dot | Regenerated DOT graph including cathode→gnd edges. |
| demos/water_alarm_split/docs/WaterAlarmAssembly.cir | Regenerated SPICE netlist with LED cathodes on node 0. |
| demos/water_alarm_split/docs/WaterAlarmAssembly__ControllerBoard.kicad_sch | Regenerated controller-board schematic adding GND symbols for LED cathodes. |
| demos/water_alarm_split/docs/ControllerBoard.yosys.json | Regenerated export reflecting cathodes connected to GND. |
| demos/water_alarm_split/docs/ControllerBoard.svg | Regenerated diagram reflecting cathodes connected to GND. |
| demos/water_alarm_split/docs/ControllerBoard.net-report.md | Regenerated net report showing LED cathodes as GND readers. |
| demos/water_alarm_split/docs/ControllerBoard.net | Regenerated KiCad netlist showing LED cathodes on GND. |
| demos/water_alarm_split/docs/ControllerBoard.mmd | Regenerated Mermaid showing cathode→gnd edges. |
| demos/water_alarm_split/docs/ControllerBoard.kicad_sch | Regenerated schematic adding GND symbols for LED cathodes. |
| demos/water_alarm_split/docs/ControllerBoard.dot | Regenerated DOT graph including cathode→gnd edges. |
| demos/water_alarm_split/docs/ControllerBoard.cir | Regenerated SPICE netlist with LED cathodes on node 0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while reviewing WaterAlarm's breadboard SVG (from #28): the LEDs in the design had no ground connections drawn. The framework's
LED.cathodewas declaredmandatory=Falsewith a "floating = GND" shortcut that lets demos write electrically-broken circuits and have them pass ERC. A real LED with a dangling cathode does not light — this is exactly the physical-fidelity violation CLAUDE.md lists as disallowed.LED.cathodenowmandatory=True. The framework raisesUnconnectedPinErroratCircuit.__init__if a design declares an LED with an unwired cathode.red_led.cathodeandgreen_led.cathodeto ground. (Bench builds still need a series current-limit resistor; the logic-level demo doesn't model V_F or current.)tests/components/test_led.py— the second one (test_floating_cathode_refused_at_circuit_construction) is the end-to-end test that would have caught the WaterAlarm bug originally.Test plan
uv run pytest— 4362 passed.uv run mypy src/ demos/clean.Supersedes the auto-closed #29 (the base branch was deleted when #28 merged).