NOISSUE - Reject a registry pin whose role contradicts its KiCad symbol

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<String> {
     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<BTreeMap<String, BTreeSet<String>>> {
+    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<String, BTreeSet<String>>,
+    depth: u32,
+) -> bool {
+    if depth > 4 {
+        return false;
+    }
+    let Some(block) = extract_symbol(text, sym_name) else {
+        return false;
+    };
+    // `(pin <electrical_type> <graphic_style> ... (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<String> {
+    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<String>) -> 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<BTreeMap<String, BTreeSet<String>>> {
+    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<String, BTreeSet<String>>,
+    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<String> {
+    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

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<PathBuf>) -> 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::<Vec<_>>().join("/"),
+        );
+        let types = symbol_types.iter().cloned().collect::<Vec<_>>().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::*;

crates/synth-registry/src/part.rs

@@ -221,6 +221,14 @@ pub struct OperatingConditions {
     pub max_voltage_v: Option<f64>,
     #[serde(default)]
     pub max_current_ma: Option<f64>,
+
+    /// 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<bool>,
     /// 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")]