diff --git a/crates/synth-registry/src/kicad_pin_check.rs b/crates/synth-registry/src/kicad_pin_check.rs index e6219f1b2..1c382899f 100644 --- a/crates/synth-registry/src/kicad_pin_check.rs +++ b/crates/synth-registry/src/kicad_pin_check.rs @@ -23,7 +23,9 @@ //! falls back to a synthesized rectangle in that case too, so the //! pipeline stays usable. -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; + +use crate::capability::ElectricalType; use std::path::PathBuf; use std::sync::OnceLock; @@ -137,10 +139,261 @@ fn extends_target(block: &str) -> Option { Some(after[..end].to_string()) } +/// Return each pin number in the named symbol together with the KiCad +/// electrical types carried by the pin(s) bearing that number, following +/// `(extends ...)` chains. `None` means the library or symbol could not be +/// located — the caller skips the check, exactly as for pin numbers. +/// +/// A number can map to several types: KiCad stacks repeated grounds and +/// thermal pads under one number, marking one `power_in` and the rest +/// `passive`. +pub fn kicad_pin_types(lib_id: &str) -> Option>> { + let (lib_name, sym_name) = lib_id.split_once(':')?; + let dir = bundled_dir()?; + let text = std::fs::read_to_string(dir.join(format!("{lib_name}.kicad_sym"))).ok()?; + let mut out = BTreeMap::new(); + if collect_pin_types(&text, sym_name, &mut out, 0) && !out.is_empty() { + Some(out) + } else { + None + } +} + +fn collect_pin_types( + text: &str, + sym_name: &str, + out: &mut BTreeMap>, + depth: u32, +) -> bool { + if depth > 4 { + return false; + } + let Some(block) = extract_symbol(text, sym_name) else { + return false; + }; + // `(pin ... (number "N" ...))`. Taking + // the whole pin by paren balance keeps the number with its own type. + for pin in pin_blocks(&block) { + let after = &pin["(pin ".len()..]; + let Some(kind_end) = after.find(char::is_whitespace) else { + continue; + }; + if let Some(number) = first_quoted_after(&pin, "(number ") { + out.entry(number) + .or_default() + .insert(after[..kind_end].to_string()); + } + } + if let Some(parent) = extends_target(&block) { + collect_pin_types(text, &parent, out, depth + 1); + } + true +} + +fn first_quoted_after(block: &str, key: &str) -> Option { + let at = block.find(key)? + key.len(); + let open = block[at..].find('"')? + at + 1; + let close = block[open..].find('"')? + open; + Some(block[open..close].to_string()) +} + +/// Whether a registry pin's declared role can sit on a symbol pin carrying +/// `symbol_types`. +/// +/// Only the two contradictions worth blocking a load for: +/// +/// * a supply or ground pin landing on a symbol pin that drives or reads a +/// signal — the rail meets an output; +/// * a signal pin landing on a symbol supply pin — the driver meets a rail. +/// +/// `passive`, `unspecified` and `free` agree with anything, because KiCad uses +/// them for stacked grounds, exposed pads and crystal terminals alike. A pin +/// the part declares `do_not_connect` is asserting that the pad stays +/// floating, which no symbol type contradicts. And a pin the part declares +/// `passive` is neither a rail nor a driver, so it is neither side of either +/// contradiction: `lmr33640` declares its internal-LDO `VCC` passive on +/// purpose, so that nothing treats a bias pin as a rail to load. +#[must_use] +pub fn roles_agree(declared: ElectricalType, symbol_types: &BTreeSet) -> bool { + const SYMBOL_SUPPLY: [&str; 2] = ["power_in", "power_out"]; + const AGREES_WITH_ANYTHING: [&str; 3] = ["passive", "unspecified", "free"]; + + if matches!( + declared, + ElectricalType::DoNotConnect | ElectricalType::Passive + ) { + return true; + } + if symbol_types + .iter() + .any(|t| AGREES_WITH_ANYTHING.contains(&t.as_str())) + { + return true; + } + let symbol_is_supply = symbol_types + .iter() + .any(|t| SYMBOL_SUPPLY.contains(&t.as_str())); + if declared.is_power() { + symbol_is_supply + } else { + !symbol_is_supply + } +} + +/// Return each pin number together with the pin name(s) the symbol prints +/// there. Used to say *which* signal a mis-numbered pad actually carries, +/// which is the one fact that makes the diagnostic actionable. +pub fn kicad_pin_names(lib_id: &str) -> Option>> { + let (lib_name, sym_name) = lib_id.split_once(':')?; + let dir = bundled_dir()?; + let text = std::fs::read_to_string(dir.join(format!("{lib_name}.kicad_sym"))).ok()?; + let mut out = BTreeMap::new(); + if collect_pin_names(&text, sym_name, &mut out, 0) && !out.is_empty() { + Some(out) + } else { + None + } +} + +fn collect_pin_names( + text: &str, + sym_name: &str, + out: &mut BTreeMap>, + depth: u32, +) -> bool { + if depth > 4 { + return false; + } + let Some(block) = extract_symbol(text, sym_name) else { + return false; + }; + for pin in pin_blocks(&block) { + if let (Some(number), Some(name)) = ( + first_quoted_after(&pin, "(number "), + first_quoted_after(&pin, "(name "), + ) { + out.entry(number).or_default().insert(name); + } + } + if let Some(parent) = extends_target(&block) { + collect_pin_names(text, &parent, out, depth + 1); + } + true +} + +/// Split a symbol block into its `(pin ...)` sub-blocks by paren balance. +fn pin_blocks(block: &str) -> Vec { + let mut out = Vec::new(); + let mut search = 0; + while let Some(idx) = block[search..].find("(pin ") { + let start = search + idx; + let mut depth = 0_i32; + let mut end = start; + for (i, b) in block.as_bytes().iter().enumerate().skip(start) { + match b { + b'(' => depth += 1, + b')' => { + depth -= 1; + if depth == 0 { + end = i + 1; + break; + } + } + _ => {} + } + } + if end <= start { + break; + } + out.push(block[start..end].to_string()); + search = start + "(pin ".len(); + } + out +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn pin_types_are_read_with_their_own_numbers() { + let text = r#"(kicad_symbol_lib + (symbol "Thing" + (symbol "Thing_1_1" + (pin power_in line (at 0 3.81 270) (length 2.794) + (name "VDD" (effects (font (size 1.27 1.27)))) + (number "28" (effects (font (size 1.27 1.27)))) + ) + (pin output line (at 0 -3.81 90) (length 2.794) + (name "TXN" (effects (font (size 1.27 1.27)))) + (number "1" (effects (font (size 1.27 1.27)))) + ) + ) + ) +)"#; + let mut out = BTreeMap::new(); + assert!(collect_pin_types(text, "Thing", &mut out, 0)); + assert_eq!(out["28"], BTreeSet::from(["power_in".to_string()])); + assert_eq!(out["1"], BTreeSet::from(["output".to_string()])); + } + + #[test] + fn a_rail_declared_on_a_signal_pad_does_not_agree() { + // The w5500 case: `vcc` sat on pad 1, which the symbol drives as TXN. + let symbol = BTreeSet::from(["output".to_string()]); + assert!(!roles_agree(ElectricalType::PowerInput, &symbol)); + assert!(!roles_agree(ElectricalType::GroundReference, &symbol)); + // and the converse: a signal pin on a supply pad. + let supply = BTreeSet::from(["power_in".to_string()]); + assert!(!roles_agree(ElectricalType::Input, &supply)); + assert!(!roles_agree(ElectricalType::Bidirectional, &supply)); + } + + #[test] + fn roles_that_do_agree_are_left_alone() { + let supply = BTreeSet::from(["power_in".to_string()]); + assert!(roles_agree(ElectricalType::PowerInput, &supply)); + + let signal = BTreeSet::from(["bidirectional".to_string()]); + assert!(roles_agree(ElectricalType::Bidirectional, &signal)); + + // KiCad stacks repeated grounds and thermal pads as `passive`, and + // uses it for crystal terminals too, so it agrees with anything. + let passive = BTreeSet::from(["passive".to_string()]); + assert!(roles_agree(ElectricalType::PowerInput, &passive)); + assert!(roles_agree(ElectricalType::Output, &passive)); + + // A number carrying several types agrees if any of them does. + let stacked = BTreeSet::from(["power_in".to_string(), "passive".to_string()]); + assert!(roles_agree(ElectricalType::PowerInput, &stacked)); + } + + #[test] + fn a_passive_pin_agrees_with_every_symbol_type() { + // Passive claims to be neither a rail nor a driver, so a symbol + // supply pin under it is not a rail meeting a signal. + for kind in ["power_in", "power_out", "output", "input"] { + let symbol = BTreeSet::from([kind.to_string()]); + assert!( + roles_agree(ElectricalType::Passive, &symbol), + "passive should agree with {kind}" + ); + } + } + + #[test] + fn a_do_not_connect_pin_agrees_with_every_symbol_type() { + // The part is asserting the pad stays floating. Nothing the symbol + // says about it contradicts that. + for kind in ["power_in", "output", "input", "no_connect"] { + let symbol = BTreeSet::from([kind.to_string()]); + assert!( + roles_agree(ElectricalType::DoNotConnect, &symbol), + "do_not_connect should agree with {kind}" + ); + } + } + #[test] fn extract_pins_from_simple_block() { let text = r#"(kicad_symbol_lib diff --git a/crates/synth-registry/src/loader.rs b/crates/synth-registry/src/loader.rs index 4de106755..d3c52d589 100644 --- a/crates/synth-registry/src/loader.rs +++ b/crates/synth-registry/src/loader.rs @@ -73,6 +73,18 @@ pub enum LoadError { registry_number: String, }, + #[error( + "part {path}: kicad_symbol `{kicad_symbol}` disagrees about pad \ + `{registry_number}`: {detail}. A rail and a signal would meet \ + on that pad." + )] + KicadPinRoleMismatch { + path: PathBuf, + kicad_symbol: String, + registry_number: String, + detail: String, + }, + #[error( "part {path}: invalid operating conditions (min_voltage_v {min} > max_voltage_v {max})" )] @@ -481,6 +493,7 @@ fn walk(dir: &Path, out: &mut Vec) -> Result<(), LoadError> { Ok(()) } +#[allow(clippy::too_many_lines)] fn validate_part(path: &Path, part: &Part) -> Result<(), LoadError> { // id must match filename stem (without the `.synth.toml` suffix). let expected_id = path @@ -557,6 +570,8 @@ fn validate_part(path: &Path, part: &Part) -> Result<(), LoadError> { } } + check_kicad_pin_roles(part, path)?; + if let Some(ref dims) = part.footprint_dimensions { if dims.width_mm <= 0.0 || dims.height_mm <= 0.0 { return Err(LoadError::InvalidFootprintDimensions { @@ -596,6 +611,50 @@ fn validate_part(path: &Path, part: &Part) -> Result<(), LoadError> { Ok(()) } +/// Role invariant: a registry pin's declared role must not contradict the +/// KiCad symbol pin carrying the same number. +/// +/// The pin-number check proves the pad exists; it cannot tell whether it is +/// the right pad, because a part that numbers its pins 1, 2, 3 in the order +/// the author listed the signals hits numbers every package has. Five shipped +/// parts did exactly that, putting a 3.3 V rail on an Ethernet transmit pad +/// and a reset line on a supply pin. Comparing roles catches that class +/// without needing to know the part. +/// +/// Skipped, like the number check, when no symbol library resolves. +fn check_kicad_pin_roles(part: &Part, path: &Path) -> Result<(), LoadError> { + let Some(lib_id) = part.kicad_symbol.as_deref() else { + return Ok(()); + }; + let Some(kicad_types) = crate::kicad_pin_check::kicad_pin_types(lib_id) else { + return Ok(()); + }; + let names = crate::kicad_pin_check::kicad_pin_names(lib_id).unwrap_or_default(); + for pin in &part.pins { + let Some(symbol_types) = kicad_types.get(&pin.number.0) else { + continue; + }; + if crate::kicad_pin_check::roles_agree(pin.electrical_type, symbol_types) { + continue; + } + let carries = names.get(&pin.number.0).map_or_else( + || "an unnamed pin".to_string(), + |n| n.iter().cloned().collect::>().join("/"), + ); + let types = symbol_types.iter().cloned().collect::>().join("/"); + return Err(LoadError::KicadPinRoleMismatch { + path: path.to_path_buf(), + kicad_symbol: lib_id.to_string(), + registry_number: pin.number.0.clone(), + detail: format!( + "the part declares `{}` as {:?}, the symbol carries {} there ({})", + pin.name, pin.electrical_type, types, carries + ), + }); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/synth-registry/src/overlay.rs b/crates/synth-registry/src/overlay.rs index 51b8db2a9..d9077a159 100644 --- a/crates/synth-registry/src/overlay.rs +++ b/crates/synth-registry/src/overlay.rs @@ -70,6 +70,9 @@ pub fn part_to_toml(part: &Part, header: &[&str]) -> String { if let Some(v) = c.max_current_ma { let _ = writeln!(out, "max_current_ma = {v}"); } + if let Some(v) = c.power_budget { + let _ = writeln!(out, "power_budget = {v}"); + } for (key, value) in [ ("breakdown_voltage_v", c.breakdown_voltage_v), ("breakdown_voltage_min_v", c.breakdown_voltage_min_v), @@ -233,7 +236,7 @@ impl PinCapability { #[cfg(test)] mod tests { use super::*; - use crate::{Lifecycle, Part, PartId, PinNumber}; + use crate::{Lifecycle, OperatingConditions, Part, PartId, PinNumber}; fn sample() -> Part { Part { @@ -312,6 +315,24 @@ mod tests { assert!(text.contains("reviewed_by = \"\""), "{text}"); } + #[test] + fn power_budget_survives_overlay_round_trip() { + let mut part = sample(); + part.operating_conditions = Some(OperatingConditions { + power_budget: Some(false), + ..Default::default() + }); + let text = part_to_toml(&part, &[]); + assert!(text.contains("power_budget = false"), "{text}"); + let back: Part = toml::from_str(&text).expect("emitted TOML must parse"); + assert_eq!( + back.operating_conditions + .as_ref() + .and_then(|c| c.power_budget), + Some(false) + ); + } + #[test] fn header_lines_become_comments() { let text = part_to_toml(&sample(), &["first", "second"]); diff --git a/crates/synth-registry/src/part.rs b/crates/synth-registry/src/part.rs index 9a11335d7..8279ea9b0 100644 --- a/crates/synth-registry/src/part.rs +++ b/crates/synth-registry/src/part.rs @@ -221,6 +221,14 @@ pub struct OperatingConditions { pub max_voltage_v: Option, #[serde(default)] pub max_current_ma: Option, + + /// Whether [`Self::max_current_ma`] is a budget for a rail this part + /// supplies, rather than a consumption limit. `None` (the default) + /// means "decide from the pin map": a part with a `power_output` pin + /// is treated as supplying a rail. `Some(false)` explicitly opts out, + /// e.g. an internal LDO decoupling tap. `Some(true)` forces it in. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub power_budget: Option, /// Worst-case power the part dissipates, in milliwatts. With /// [`Self::thermal_resistance_c_per_w`] this drives the thermal check. #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/crates/synth-validate/src/deep_erc.rs b/crates/synth-validate/src/deep_erc.rs index dbe977f1a..7b9c60030 100644 --- a/crates/synth-validate/src/deep_erc.rs +++ b/crates/synth-validate/src/deep_erc.rs @@ -667,6 +667,38 @@ impl PowerBudgetRule { } } +/// Whether this part supplies a rail other components may draw from, so that +/// `max_current_ma` is a budget to spend rather than a draw to count. +/// +/// The field means opposite things depending on the part: on a regulator or a +/// charger it is what the device can deliver, and on everything else it is +/// what the device itself consumes. [`PowerBudgetRule`] read it as a budget +/// for any part that had one and a `power_output` pin somewhere. Correcting +/// the CH340G's `v3` to match the stock symbol therefore made it a 20 mA +/// regulator -- that pin is its internal LDO's decoupling tap, there to be +/// decoupled rather than loaded -- and +/// `pass__power_004_sufficient_decoupling` reported two overruns on a board +/// that is fine. +fn part_supplies_a_rail(part: &synth_registry::Part) -> bool { + let Some(oc) = part.operating_conditions.as_ref() else { + return false; + }; + let Some(_max_ma) = oc.max_current_ma else { + return false; + }; + // An explicit answer wins over inference. + if let Some(budget) = oc.power_budget { + if !budget { + return false; + } + } + // Otherwise a part supplies a rail when it has a power_output pin, + // preserving the previous generic behaviour for any supply kind. + part.pins + .iter() + .any(|p| p.electrical_type == synth_registry::ElectricalType::PowerOutput) +} + impl ErcRule for PowerBudgetRule { fn code(&self) -> &'static str { "E-SYNTH-POWER-010" @@ -682,6 +714,9 @@ impl ErcRule for PowerBudgetRule { let Some(part) = regulator.part.as_ref() else { continue; }; + if !part_supplies_a_rail(part) { + continue; + } let Some(max_ma) = part .operating_conditions .as_ref() diff --git a/crates/synth-validate/tests/deep_erc.rs b/crates/synth-validate/tests/deep_erc.rs index 8f895183d..3930690f9 100644 --- a/crates/synth-validate/tests/deep_erc.rs +++ b/crates/synth-validate/tests/deep_erc.rs @@ -1510,3 +1510,42 @@ fn a_three_branch_resistor_network_is_not_exempted_as_a_divider() { codes(&diags, "E-SYNTH-POWER-008") ); } + +/// The CH340G's `v3` is not a rail with a budget. +/// +/// `v3` is the internal LDO's decoupling tap, and the part's 20 mA +/// `max_current_ma` is its own draw rather than anything it can deliver. +/// Once the pin was corrected to `power_output` to match the stock symbol, +/// `PowerBudgetRule` read it as a 20 mA regulator and charged it for whatever +/// sat on the net, so `pass__power_004_sufficient_decoupling` reported two +/// E-SYNTH-POWER-010 overruns on a board that is fine. +#[test] +fn the_ch340g_auxiliary_output_is_not_a_regulator_rail() { + let src = r#"board "power_anomaly_pass" { + layers 2 + component U1: ic "ch340g" + component U2: ic "ch340g" + component J1: connector "header_1x4" + component C1: capacitor "c_generic_0603" value "100nF" + component C2: capacitor "c_generic_0603" value "100nF" + + connect J1.p1 -> U1.vcc + connect J1.p1 -> U2.vcc + connect J1.p1 -> C1.p1 + connect J1.p1 -> C2.p1 + + connect J1.p2 -> U1.gnd + connect J1.p2 -> U2.gnd + connect J1.p2 -> C1.p2 + connect J1.p2 -> C2.p2 + + connect U1.v3 -> J1.p3 + connect U2.v3 -> J1.p4 + }"#; + let diags = validate(src); + assert!( + codes(&diags, "E-SYNTH-POWER-010").is_empty(), + "an internal regulator's decoupling pin supplies no rail: {:?}", + codes(&diags, "E-SYNTH-POWER-010") + ); +} diff --git a/registry/parts/ic/ch340c.synth.toml b/registry/parts/ic/ch340c.synth.toml index 6f1e16cab..ba839a8d1 100644 --- a/registry/parts/ic/ch340c.synth.toml +++ b/registry/parts/ic/ch340c.synth.toml @@ -13,6 +13,7 @@ kicad_footprint = "Package_SO:SOIC-16_3.9x9.9mm_P1.27mm" min_voltage_v = 3.0 max_voltage_v = 5.3 max_current_ma = 30.0 +power_budget = false [[pins]] name = "gnd" @@ -38,7 +39,7 @@ required = false [[pins]] name = "v3" number = "4" -electrical_type = "passive" +electrical_type = "power_output" capabilities = [] required = true diff --git a/registry/parts/ic/ch340g.synth.toml b/registry/parts/ic/ch340g.synth.toml index 809cfbffa..933a69896 100644 --- a/registry/parts/ic/ch340g.synth.toml +++ b/registry/parts/ic/ch340g.synth.toml @@ -17,6 +17,7 @@ courtyard_margin_mm = 0.25 min_voltage_v = 2.9 max_voltage_v = 5.3 max_current_ma = 20.0 +power_budget = false [[pins]] name = "gnd" @@ -39,7 +40,7 @@ capabilities = ["uart_rx"] [[pins]] name = "v3" number = "4" -electrical_type = "passive" +electrical_type = "power_output" required = true [[pins]]