Skip to content

framework: LED.cathode mandatory + close WaterAlarm regression - #29

Closed
raeq wants to merge 1 commit into
phase-2.6-breadboardfrom
framework-led-cathode-mandatory
Closed

framework: LED.cathode mandatory + close WaterAlarm regression#29
raeq wants to merge 1 commit into
phase-2.6-breadboardfrom
framework-led-cathode-mandatory

Conversation

@raeq

@raeq raeq commented May 19, 2026

Copy link
Copy Markdown
Owner

Summary

Found while reviewing WaterAlarm's breadboard SVG (from #28): the LEDs in the design had no ground connections drawn. The framework's LED.cathode was declared mandatory=False with 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.cathode now mandatory=True. The framework raises UnconnectedPinError at Circuit.__init__ if a design declares an LED with an unwired cathode.
  • WaterAlarm and WaterAlarmSplit had floating cathodes — both now wire red_led.cathode and green_led.cathode to ground. (Bench builds still need a series current-limit resistor; the logic-level demo doesn't model V_F or current.)
  • Two new regression tests in 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.
  • Touched a handful of unit-test fixtures that had leaned on the broken default.

Test plan

  • uv run pytest — 4362 passed.
  • uv run mypy src/ demos/ clean.
  • Demo docs regenerated for water_alarm + water_alarm_split — the breadboard SVGs now show the ground jumpers to each LED's cathode.

Based on #28 (Phase 2.6 breadboard visualiser).

Copilot AI review requested due to automatic review settings May 19, 2026 15:10
@raeq raeq mentioned this pull request May 19, 2026
Closed
3 tasks
Copilot AI reviewed May 19, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens LED modeling fidelity by making LED.cathode mandatory, then updates WaterAlarm demos, exports, goldens, and regression tests so floating LED cathodes are caught during circuit construction.

Changes:

  • Makes LED.cathode a mandatory port and documents the physical-fidelity requirement.
  • Grounds WaterAlarm and WaterAlarmSplit LED cathodes and regenerates affected artifacts/goldens.
  • Adds regression coverage for mandatory LED cathodes and updates fixtures/import tests affected by stricter validation.

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
src/components/passives/led.py Makes cathode mandatory and updates LED port docs.
demos/water_alarm/water_alarm.py Wires both LED cathodes to ground.
demos/water_alarm_split/water_alarm_split.py Wires controller-board LED cathodes to ground.
tests/components/test_led.py Adds mandatory-port and floating-cathode regression tests.
tests/framework/test_framework.py Updates topology fixture for grounded LED cathode.
tests/framework/test_circuit_autocollect.py Updates fixtures to satisfy stricter mandatory-port validation.
tests/framework/import_kicad/test_import.py Updates minimal import netlist to include driven power/ground nets.
tests/golden/yosys/water_alarm.json Updates LED cathode net assignments.
tests/golden/yosys/water_alarm_assembly.json Updates assembly LED cathode net assignments.
tests/golden/spice/water_alarm.cir Grounds LED cathodes in SPICE golden.
tests/golden/spice/water_alarm_assembly.cir Grounds assembly LED cathodes in SPICE golden.
tests/golden/mermaid/water_alarm.mmd Adds LED cathode-to-ground edges.
tests/golden/mermaid/water_alarm_assembly.mmd Adds assembly LED cathode-to-ground edges.
tests/golden/kicad/water_alarm.net Adds LED cathode nodes to GND net.
tests/golden/kicad/water_alarm_assembly.net Adds assembly LED cathode nodes to GND net.
tests/golden/kicad_sch/water_alarm.kicad_sch Adds GND symbols for LED cathodes.
tests/golden/dot/water_alarm.dot Adds LED cathode-to-ground graph edges.
tests/golden/dot/water_alarm_assembly.dot Adds assembly LED cathode-to-ground graph edges.
tests/golden/breadboard/water_alarm.breadboard.svg Adds breadboard ground jumpers for LED cathodes.
tests/golden/assembly_guide/water_alarm.md Adds assembly steps for LED cathode ground jumpers.
demos/water_alarm/docs/WaterAlarm.yosys.json Regenerates Yosys export.
demos/water_alarm/docs/WaterAlarm.svg Regenerates graph SVG export.
demos/water_alarm/docs/WaterAlarm.net-report.md Updates GND net reader count/details.
demos/water_alarm/docs/WaterAlarm.net Adds LED cathodes to GND net.
demos/water_alarm/docs/WaterAlarm.mmd Adds LED cathode-to-ground edges.
demos/water_alarm/docs/WaterAlarm.md Adds LED cathode grounding assembly steps.
demos/water_alarm/docs/WaterAlarm.kicad_sch Adds GND symbols for LED cathodes.
demos/water_alarm/docs/WaterAlarm.dot Adds LED cathode-to-ground graph edges.
demos/water_alarm/docs/WaterAlarm.cir Grounds LED cathodes in SPICE export.
demos/water_alarm/docs/WaterAlarm.breadboard.svg Adds breadboard ground jumpers.
demos/water_alarm_split/docs/WaterAlarmAssembly.yosys.json Regenerates assembly Yosys export.
demos/water_alarm_split/docs/WaterAlarmAssembly.svg Regenerates assembly graph SVG.
demos/water_alarm_split/docs/WaterAlarmAssembly.net-report.md Updates assembly GND net readers.
demos/water_alarm_split/docs/WaterAlarmAssembly.net Adds assembly LED cathodes to GND net.
demos/water_alarm_split/docs/WaterAlarmAssembly.mmd Adds assembly LED cathode-to-ground edges.
demos/water_alarm_split/docs/WaterAlarmAssembly.dot Adds assembly LED cathode-to-ground graph edges.
demos/water_alarm_split/docs/WaterAlarmAssembly.cir Grounds assembly LED cathodes in SPICE export.
demos/water_alarm_split/docs/WaterAlarmAssembly__ControllerBoard.kicad_sch Adds controller-board GND symbols.
demos/water_alarm_split/docs/ControllerBoard.yosys.json Updates controller-board LED cathode nets.
demos/water_alarm_split/docs/ControllerBoard.svg Regenerates controller-board graph SVG.
demos/water_alarm_split/docs/ControllerBoard.net-report.md Updates controller-board GND readers.
demos/water_alarm_split/docs/ControllerBoard.net Adds controller LED cathodes to GND net.
demos/water_alarm_split/docs/ControllerBoard.mmd Adds controller LED cathode-to-ground edges.
demos/water_alarm_split/docs/ControllerBoard.kicad_sch Adds controller LED cathode GND symbols.
demos/water_alarm_split/docs/ControllerBoard.dot Adds controller LED cathode-to-ground graph edges.
demos/water_alarm_split/docs/ControllerBoard.cir Grounds controller LED cathodes in SPICE export.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

subzero
framework: LED.cathode mandatory + close WaterAlarm regression (#29)
a265d46 
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
raeq force-pushed the framework-led-cathode-mandatory branch from 89570e4 to a265d46 Compare May 19, 2026 16:05
raeq pushed a commit that referenced this pull request May 19, 2026
subzero
framework: passive + diode terminals mandatory (#30)
04ef6e3 
Continuation of the LED.cathode regression fix (#29). Resistor,
Capacitor, Inductor, and every Diode subclass had their terminals
declared `mandatory=False` with the same "structurally inert under
graph evaluation" justification — letting demos declare BOM-only
timing / decoupling / pull-up parts without wiring them, then ship
breadboard SVGs and netlists where those parts sat orphaned.

A real resistor / capacitor / inductor / diode with a dangling
terminal does nothing. Marking the ports mandatory closes the
silent regression at the framework layer.

  - `src/components/passives/{resistor,capacitor,inductor}.py` and
    every diode subclass under `src/components/diodes/` now declare
    their terminals `mandatory=True`.
  - `tests/components/test_{resistor,capacitor,inductor}.py`:
    regression tests pin the new contract per component.
  - `tests/components/test_diode_mandatory_terminals.py` (new):
    parametrized over D1N4001 / D1N4007 / D1N4148 / D1N4728A /
    D1N4733A / D1N4742A / D1N5817.
  - Six demos (`backup_power`, `bldc_motor`, `dice`,
    `doorbell_protector`, `fan_cooling`, `isolated_rs232`) had
    declared-but-unwired passives — each now wires them either to
    their datasheet-correct net (e.g. DoorbellProtector's flyback
    diode anode joins the relay coil_minus net) or as an isolated
    0-Ω passthrough with `dynamically_driven=True` where the part
    is genuinely BOM-only and the voltage-only simulator can't
    honour its behaviour. Comments at each call site explain
    which kind.
  - Test fixtures across CLI, refdes, format-roundtrip, kicad
    import, and per-format renderer tests updated so they still
    pin down the specific error each one exists to catch without
    tripping the new mandatory-pin check first.
  - All demo docs regenerated; goldens refreshed.

After this commit the breadboard SVGs show every BOM part connected
to something — no more orphaned caps and resistors floating without
jumpers.
@raeq
raeq deleted the branch phase-2.6-breadboard May 19, 2026 16:30
@raeq raeq closed this May 19, 2026
@raeq raeq mentioned this pull request May 19, 2026
Merged
3 tasks
raeq pushed a commit that referenced this pull request May 19, 2026
subzero
framework: LED.cathode mandatory + close WaterAlarm regression (#29)
2f70dab 
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 added a commit that referenced this pull request May 19, 2026
@raeq subzero
framework: LED.cathode mandatory + close WaterAlarm regression (#29) (#…
4b08089 
…31)

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.

Co-authored-by: subzero <subzero@noreply.com>
raeq pushed a commit that referenced this pull request May 19, 2026
subzero
framework: passive + diode terminals mandatory (#30)
4d35574 
Continuation of the LED.cathode regression fix (#29). Resistor,
Capacitor, Inductor, and every Diode subclass had their terminals
declared `mandatory=False` with the same "structurally inert under
graph evaluation" justification — letting demos declare BOM-only
timing / decoupling / pull-up parts without wiring them, then ship
breadboard SVGs and netlists where those parts sat orphaned.

A real resistor / capacitor / inductor / diode with a dangling
terminal does nothing. Marking the ports mandatory closes the
silent regression at the framework layer.

  - `src/components/passives/{resistor,capacitor,inductor}.py` and
    every diode subclass under `src/components/diodes/` now declare
    their terminals `mandatory=True`.
  - `tests/components/test_{resistor,capacitor,inductor}.py`:
    regression tests pin the new contract per component.
  - `tests/components/test_diode_mandatory_terminals.py` (new):
    parametrized over D1N4001 / D1N4007 / D1N4148 / D1N4728A /
    D1N4733A / D1N4742A / D1N5817.
  - Six demos (`backup_power`, `bldc_motor`, `dice`,
    `doorbell_protector`, `fan_cooling`, `isolated_rs232`) had
    declared-but-unwired passives — each now wires them either to
    their datasheet-correct net (e.g. DoorbellProtector's flyback
    diode anode joins the relay coil_minus net) or as an isolated
    0-Ω passthrough with `dynamically_driven=True` where the part
    is genuinely BOM-only and the voltage-only simulator can't
    honour its behaviour. Comments at each call site explain
    which kind.
  - Test fixtures across CLI, refdes, format-roundtrip, kicad
    import, and per-format renderer tests updated so they still
    pin down the specific error each one exists to catch without
    tripping the new mandatory-pin check first.
  - All demo docs regenerated; goldens refreshed.

After this commit the breadboard SVGs show every BOM part connected
to something — no more orphaned caps and resistors floating without
jumpers.
@raeq raeq mentioned this pull request May 19, 2026
Merged
3 tasks
raeq added a commit that referenced this pull request May 19, 2026
@raeq subzero
framework: passive + diode terminals mandatory (#30) (#32)
9717654 
Continuation of the LED.cathode regression fix (#29). Resistor,
Capacitor, Inductor, and every Diode subclass had their terminals
declared `mandatory=False` with the same "structurally inert under
graph evaluation" justification — letting demos declare BOM-only
timing / decoupling / pull-up parts without wiring them, then ship
breadboard SVGs and netlists where those parts sat orphaned.

A real resistor / capacitor / inductor / diode with a dangling
terminal does nothing. Marking the ports mandatory closes the
silent regression at the framework layer.

  - `src/components/passives/{resistor,capacitor,inductor}.py` and
    every diode subclass under `src/components/diodes/` now declare
    their terminals `mandatory=True`.
  - `tests/components/test_{resistor,capacitor,inductor}.py`:
    regression tests pin the new contract per component.
  - `tests/components/test_diode_mandatory_terminals.py` (new):
    parametrized over D1N4001 / D1N4007 / D1N4148 / D1N4728A /
    D1N4733A / D1N4742A / D1N5817.
  - Six demos (`backup_power`, `bldc_motor`, `dice`,
    `doorbell_protector`, `fan_cooling`, `isolated_rs232`) had
    declared-but-unwired passives — each now wires them either to
    their datasheet-correct net (e.g. DoorbellProtector's flyback
    diode anode joins the relay coil_minus net) or as an isolated
    0-Ω passthrough with `dynamically_driven=True` where the part
    is genuinely BOM-only and the voltage-only simulator can't
    honour its behaviour. Comments at each call site explain
    which kind.
  - Test fixtures across CLI, refdes, format-roundtrip, kicad
    import, and per-format renderer tests updated so they still
    pin down the specific error each one exists to catch without
    tripping the new mandatory-pin check first.
  - All demo docs regenerated; goldens refreshed.

After this commit the breadboard SVGs show every BOM part connected
to something — no more orphaned caps and resistors floating without
jumpers.

Co-authored-by: subzero <subzero@noreply.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@raeq