diff --git a/crates/fbuild-cli/Cargo.toml b/crates/fbuild-cli/Cargo.toml index 1eddbd3e..cb07fe55 100644 --- a/crates/fbuild-cli/Cargo.toml +++ b/crates/fbuild-cli/Cargo.toml @@ -39,4 +39,9 @@ sha2 = { workspace = true } tempfile = { workspace = true } walkdir = { workspace = true } owo-colors = { workspace = true } + +# O_NONBLOCK / O_NOCTTY for the read-only serial probe in `port doctor` +# (FastLED/fbuild#1424). Unix-only: the probe itself is cfg'd to Linux. +[target.'cfg(unix)'.dependencies] +libc = { workspace = true } semver = { workspace = true } diff --git a/crates/fbuild-cli/src/cli/mod.rs b/crates/fbuild-cli/src/cli/mod.rs index 52c9b672..d3cfa822 100644 --- a/crates/fbuild-cli/src/cli/mod.rs +++ b/crates/fbuild-cli/src/cli/mod.rs @@ -42,6 +42,7 @@ pub mod serial_probe; pub mod show; pub mod symbols_cmd; pub mod sync_cmd; +pub mod udev; // #1148 supplies the helper foundation; #1147 is deliberately its first // production consumer once RP2040 deployment can emit a typed request. #[allow( diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 579eed10..37a20169 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -32,6 +32,13 @@ pub struct PortDiagnosis { /// a single-port query — see `query_last_seen_secs` for why. #[serde(skip_serializing_if = "Option::is_none")] pub last_seen_secs_ago: Option, + /// Whether this process can actually open the port. + /// + /// `None` when not probed. Never silently `true`: a port the host lists + /// as healthy can still be unopenable, and reporting that as fine is the + /// failure this field exists to stop (FastLED/fbuild#1424). + #[serde(skip_serializing_if = "Option::is_none")] + pub openable: Option, } /// What the diagnosis means and what to do about it. @@ -68,10 +75,70 @@ pub fn diagnose(port: &DetectedPort, power_rows: &[(String, bool)]) -> PortDiagn parent_instance_id: port.parent_instance_id.clone(), suspend_allowed: suspend_for_ancestors(power_rows, &port.ancestor_instance_ids), last_seen_secs_ago: None, + openable: probe_openable(&port.info.port_name), } } +/// Whether the current process can open `port`, or `None` where the question +/// is not meaningful. +/// +/// Linux only. Elsewhere serial access is not group-gated the same way and a +/// speculative open would be a side effect in a command documented as +/// strictly read-only. Opening for read is enough to surface `EACCES` and +/// does not disturb a device: no DTR/RTS assertion, no write. +#[cfg(target_os = "linux")] +pub fn probe_openable(port: &str) -> Option { + use std::io::ErrorKind; + use std::os::unix::fs::OpenOptionsExt; + // O_NONBLOCK: without CLOCAL set, a terminal open blocks until carrier + // detect is asserted, which would hang a command documented as a quick + // read-only diagnostic. O_NOCTTY: never let the probe acquire a + // controlling terminal -- signals delivered to that terminal would then + // reach fbuild. Both matter here: an open on a contended port was + // measured at 13.3 s on the bench that motivated FastLED/fbuild#1424. + match std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NONBLOCK | libc::O_NOCTTY) + .open(port) + { + Ok(_) => Some(true), + Err(e) if e.kind() == ErrorKind::PermissionDenied => Some(false), + // Busy, absent, or anything else is a different question that the + // presence/problem-code verdict already covers. Claiming "not + // openable" here would blame permissions for an unrelated fault. + Err(_) => None, + } +} + +#[cfg(not(target_os = "linux"))] +pub fn probe_openable(_port: &str) -> Option { + None +} + pub fn verdict(diagnosis: &PortDiagnosis) -> Verdict { + // Checked before presence/problem-code: a port can be attached, healthy, + // and still impossible to open. That combination previously rendered as + // "attached and healthy" with an empty remedy, so `fbuild deploy` would + // flash successfully and then time out reopening the port it had just + // reported healthy — with recovery advice about cables and BOOTSEL that + // had nothing to do with the cause (FastLED/fbuild#1424). + if diagnosis.openable == Some(false) { + return Verdict { + summary: "attached, but this process cannot open the port (permission denied)" + .to_string(), + remedy: concat!( + "serial nodes are typically root:dialout 0660 and your user ", + "is not in that group. Generate rules with `fbuild port udev`, ", + "install them as /etc/udev/rules.d/99-fbuild.rules, then ", + "`sudo udevadm control --reload-rules && sudo udevadm trigger`. ", + "Adding your user to the group works too, but needs a fresh ", + "login. A one-shot chmod does not hold: deploy re-enumerates ", + "the board and udev recreates the node", + ) + .to_string(), + needs_hands: true, + }; + } match (diagnosis.presence, diagnosis.problem_code) { // The case this command exists for. A phantom record is not a fault. (Some(false), _) => Verdict { @@ -593,9 +660,89 @@ mod tests { parent_instance_id: None, suspend_allowed: None, last_seen_secs_ago: None, + openable: None, + } + } + + fn diag_openable(presence: Option, openable: Option) -> PortDiagnosis { + PortDiagnosis { + openable, + ..diag(presence, None) } } + /// The bug this branch exists for: a port the host lists as healthy but + /// that cannot be opened used to render as "attached and healthy" with an + /// empty remedy, so deploy flashed fine and then timed out reopening it. + #[test] + fn unopenable_port_is_not_reported_as_healthy() { + let v = verdict(&diag_openable(Some(true), Some(false))); + assert!(v.summary.contains("cannot open"), "got: {}", v.summary); + assert!( + v.summary.contains("permission denied"), + "got: {}", + v.summary + ); + assert!( + !v.summary.contains("attached and healthy"), + "got: {}", + v.summary + ); + assert!( + !v.remedy.is_empty(), + "an unopenable port must carry a remedy" + ); + assert!(v.needs_hands); + } + + /// The remedy has to name a fix that survives re-enumeration. A one-shot + /// chmod is wiped when deploy cycles the board through BOOTSEL. + #[test] + fn permission_remedy_points_at_udev_not_a_chmod() { + let v = verdict(&diag_openable(Some(true), Some(false))); + assert!(v.remedy.contains("udev"), "got: {}", v.remedy); + assert!(v.remedy.contains("fbuild port udev"), "got: {}", v.remedy); + assert!( + !v.remedy.contains("cable"), + "cables are unrelated: {}", + v.remedy + ); + assert!( + !v.remedy.contains("BOOTSEL"), + "BOOTSEL is unrelated: {}", + v.remedy + ); + } + + /// Permission state must not mask the absent-board verdict, which is the + /// case this whole command was written for. + #[test] + fn openable_port_still_reports_the_presence_verdict() { + let v = verdict(&diag_openable(Some(true), Some(true))); + assert!( + v.summary.contains("attached and healthy"), + "got: {}", + v.summary + ); + + let absent = verdict(&diag_openable(Some(false), None)); + assert!( + absent.summary.contains("not attached"), + "got: {}", + absent.summary + ); + } + + /// An unprobed port must fall through untouched — `None` is "unknown", + /// never "fine". + #[test] + fn unprobed_openability_changes_nothing() { + assert_eq!( + verdict(&diag_openable(Some(true), None)), + verdict(&diag(Some(true), None)) + ); + } + /// The headline: an absent board must be called out as absent, and the /// remedy must be "plug it in" — never a recovery procedure. #[test] diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index 648bd93c..9a0618f3 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -72,6 +72,20 @@ pub enum PortAction { #[arg(long, conflicts_with_all = ["port", "fix", "dry_run"])] hub: Option, }, + /// Print udev rules granting serial access for every vendor in the + /// FastLED/boards registry (FastLED/fbuild#1424). + /// + /// Prints rather than installs. On NixOS `/etc` is generated from + /// declarative config, so a written file there is out-of-band and liable + /// to be clobbered -- those users need the content for + /// `services.udev.extraRules`, not a mutation. Everywhere else, redirect + /// it yourself so the privileged write stays explicit. + Udev { + /// Group to grant access to. Defaults to `plugdev`, which unlike + /// `dialout` does not also confer modem/PPP access. + #[arg(long)] + group: Option, + }, } /// Top-level entry — dispatcher calls this. @@ -94,6 +108,38 @@ pub fn run_port(action: PortAction) -> Result<()> { super::port_doctor::run(port.as_deref(), hub.as_deref(), json) } } + PortAction::Udev { group } => run_udev(group.as_deref()), + } +} + +/// Emit udev rules derived from the ingested registry. +fn run_udev(group: Option<&str>) -> Result<()> { + use super::udev::{DEFAULT_UDEV_GROUP, UDEV_RULES_FILENAME, render_udev_rules}; + + // Refresh first, exactly as `scan` does: rules generated from a stale or + // absent overlay would silently omit vendors the user has plugged in. + populate_online_overlay(); + + let vids = fbuild_core::usb::online_vendor_vids(); + let group = group.unwrap_or(DEFAULT_UDEV_GROUP); + match render_udev_rules(&vids, group) { + Some(rules) => { + print!("{rules}"); + eprintln!( + "# {} vendor rule(s) from the FastLED/boards registry. \ + Install as /etc/udev/rules.d/{UDEV_RULES_FILENAME}.", + vids.len() + ); + Ok(()) + } + // Refuse rather than emit an empty file: one that looks configured + // and grants nothing is worse than none at all. + None => Err(FbuildError::Other( + "USB vendor registry is empty — cannot generate udev rules. Run \ + `fbuild port scan` once with network access to populate the \ + FastLED/boards cache, then retry." + .to_string(), + )), } } diff --git a/crates/fbuild-cli/src/cli/udev.rs b/crates/fbuild-cli/src/cli/udev.rs new file mode 100644 index 00000000..040d07f1 --- /dev/null +++ b/crates/fbuild-cli/src/cli/udev.rs @@ -0,0 +1,186 @@ +//! udev rule generation for Linux serial access (FastLED/fbuild#1424). +//! +//! On Linux, serial device nodes are `root:dialout 0660` and the invoking +//! user is usually not in `dialout`. `fbuild deploy` then flashes +//! successfully and cannot reopen the port it just flashed, which surfaces +//! as a post-deploy timeout rather than a permissions error. +//! +//! A one-shot `chmod` does not hold: the deploy cycle re-enumerates the board +//! (BOOTSEL -> application), and udev recreates the node with default +//! ownership before fbuild reopens it. Only a rule keyed on vendor ID +//! survives re-enumeration, which is why this generates rules rather than +//! touching device nodes. +//! +//! The vendor list comes from the ingested FastLED/boards registry, never a +//! local table -- see the USB VID/PID source-of-truth rule. A hand-written +//! copy would drift the moment a vendor is ingested. +//! +//! This module only *renders* text. Installing is deliberately left to the +//! caller: on NixOS `/etc` is generated from declarative config, so a written +//! file there is both out-of-band and liable to be clobbered. What those users +//! need is the content for `services.udev.extraRules`, not a mutation. + +/// Group granted access by generated rules. `plugdev` is the conventional +/// choice for pluggable non-storage hardware and, unlike `dialout`, does not +/// also confer modem/PPP access. +pub const DEFAULT_UDEV_GROUP: &str = "plugdev"; + +/// Filename callers should install as, if they install at all. The `99-` +/// prefix keeps it last so it overrides earlier distro rules. +pub const UDEV_RULES_FILENAME: &str = "99-fbuild.rules"; + +/// Whether `group` is a plausible Unix group name. +/// +/// The rendered value lands inside a quoted udev field in a file an operator +/// installs as root, so a `"` or a newline here could close the quote and +/// append rules of the caller's choosing. udev only unescapes `\\"` inside a +/// standard quoted string, so there is no encoding that makes arbitrary input +/// safe -- reject it instead. Mirrors the useradd(8) NAME_REGEX: an initial +/// alphanumeric or underscore, then alphanumerics, underscore, hyphen or dot. +pub fn is_valid_group_name(group: &str) -> bool { + if group.is_empty() || group.len() > 32 { + return false; + } + let mut chars = group.chars(); + let first = chars.next().unwrap_or('\0'); + if !(first.is_ascii_alphanumeric() || first == '_') { + return false; + } + chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') +} + +/// Render udev rules granting `group` access to every vendor in `vids`. +/// +/// Returns `None` when `vids` is empty, or when `group` is not a valid group +/// name. Both are real cases -- the registry overlay may not be installed, and +/// `--group` is caller-supplied -- and emitting a header with no rules, or a +/// rule built from an unvalidated group, would hand back a file that looks +/// configured while granting nothing or granting the wrong thing. +pub fn render_udev_rules(vids: &[u16], group: &str) -> Option { + if vids.is_empty() || !is_valid_group_name(group) { + return None; + } + let mut sorted: Vec = vids.to_vec(); + sorted.sort_unstable(); + sorted.dedup(); + + let mut out = String::new(); + out.push_str("# Generated by `fbuild port udev`. Do not edit by hand.\n"); + out.push_str("#\n"); + out.push_str("# Grants the listed group access to serial ports for every USB vendor\n"); + out.push_str("# in the FastLED/boards registry, so `fbuild deploy` can reopen a port\n"); + out.push_str("# after the board re-enumerates. Regenerate after upgrading fbuild to\n"); + out.push_str("# pick up newly ingested vendors.\n"); + out.push_str("#\n"); + out.push_str(&format!( + "# Install as /etc/udev/rules.d/{UDEV_RULES_FILENAME}, then:\n" + )); + out.push_str("# sudo udevadm control --reload-rules && sudo udevadm trigger\n"); + out.push_str(&format!( + "# Ensure your user is in the '{group}' group, then log out and back in.\n" + )); + out.push_str("#\n"); + out.push_str("# On NixOS, paste the rules below into services.udev.extraRules instead;\n"); + out.push_str("# /etc there is generated and a hand-written file will not persist.\n"); + out.push('\n'); + + for vid in sorted { + // Lowercase hex without an 0x prefix: udev compares ATTRS{idVendor} + // as a string against sysfs, which renders it exactly this way. An + // uppercase or 0x-prefixed value silently never matches. + out.push_str(&format!( + "SUBSYSTEM==\"tty\", ATTRS{{idVendor}}==\"{vid:04x}\", GROUP=\"{group}\", MODE=\"0660\"\n" + )); + } + Some(out) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_registry_yields_no_rules() { + // Not an empty file: a header with no rules would look configured + // while granting nothing. + assert!(render_udev_rules(&[], DEFAULT_UDEV_GROUP).is_none()); + } + + #[test] + fn renders_lowercase_four_digit_hex() { + // udev matches ATTRS{idVendor} as a string against sysfs. "0x2E8A" + // or "2E8A" never match, and the failure is silent. + let out = render_udev_rules(&[0x2e8a, 0x303a], DEFAULT_UDEV_GROUP).unwrap(); + assert!(out.contains(r#"ATTRS{idVendor}=="2e8a""#)); + assert!(out.contains(r#"ATTRS{idVendor}=="303a""#)); + assert!(!out.contains("0x2e8a")); + assert!(!out.contains("2E8A")); + } + + #[test] + fn pads_short_vids_to_four_digits() { + let out = render_udev_rules(&[0x403], DEFAULT_UDEV_GROUP).unwrap(); + assert!(out.contains(r#"ATTRS{idVendor}=="0403""#), "{out}"); + } + + #[test] + fn sorts_and_dedups() { + let out = render_udev_rules(&[0x303a, 0x2e8a, 0x303a], DEFAULT_UDEV_GROUP).unwrap(); + let rules: Vec<&str> = out + .lines() + .filter(|l| l.starts_with("SUBSYSTEM==")) + .collect(); + assert_eq!(rules.len(), 2, "duplicate VID must collapse: {out}"); + assert!(rules[0].contains("2e8a"), "not sorted: {out}"); + assert!(rules[1].contains("303a"), "not sorted: {out}"); + } + + #[test] + fn rejects_a_group_that_could_break_out_of_the_quoted_value() { + // The rendered value sits inside a quoted udev field in a file an + // operator installs as root; a quote or newline could append rules. + for bad in [ + "plug\"dev", + "plugdev\nSUBSYSTEM==\"tty\", MODE=\"0666\"", + "", + "-leading-hyphen", + ".leading-dot", + "has space", + "semi;colon", + "sl/ash", + ] { + assert!( + render_udev_rules(&[0x2e8a], bad).is_none(), + "must reject {bad:?}" + ); + } + } + + #[test] + fn accepts_ordinary_group_names() { + for good in ["plugdev", "dialout", "users", "_svc", "grp.1", "a-b_c.d"] { + assert!(is_valid_group_name(good), "must accept {good:?}"); + } + } + + #[test] + fn rejects_an_overlong_group_name() { + assert!(!is_valid_group_name(&"a".repeat(33))); + assert!(is_valid_group_name(&"a".repeat(32))); + } + + #[test] + fn honors_the_requested_group() { + let out = render_udev_rules(&[0x2e8a], "wheel").unwrap(); + assert!(out.contains(r#"GROUP="wheel""#)); + assert!(!out.contains(r#"GROUP="plugdev""#)); + } + + #[test] + fn every_vid_gets_exactly_one_rule() { + let vids: Vec = vec![0x2e8a, 0x303a, 0x0403, 0x1a86, 0x10c4, 0x16c0]; + let out = render_udev_rules(&vids, DEFAULT_UDEV_GROUP).unwrap(); + let rules = out.lines().filter(|l| l.starts_with("SUBSYSTEM==")).count(); + assert_eq!(rules, vids.len(), "{out}"); + } +} diff --git a/crates/fbuild-core/src/usb/data.rs b/crates/fbuild-core/src/usb/data.rs index e64c241d..6cbacbb8 100644 --- a/crates/fbuild-core/src/usb/data.rs +++ b/crates/fbuild-core/src/usb/data.rs @@ -457,6 +457,31 @@ pub(crate) fn install_online_cache_map(map: HashMap) { *guard = Some(map); } +/// Every vendor VID present in the runtime overlay, ascending and deduped. +/// +/// Exists so callers that must enumerate the whole supported device set -- +/// udev rule generation is the motivating one (FastLED/fbuild#1424) -- can +/// derive it from the published FastLED/boards registry instead of carrying +/// their own vendor list. A hand-maintained copy would drift the moment a +/// vendor is ingested, which is precisely what the VID/PID source-of-truth +/// rule exists to prevent. +/// +/// Empty when no overlay is installed. Callers must treat that as "unknown", +/// never as "no devices are supported" -- emitting rules from an empty +/// registry would silently produce a file that grants nothing. +pub fn online_vendor_vids() -> Vec { + let Ok(guard) = ONLINE_MAP.read() else { + return Vec::new(); + }; + let Some(map) = guard.as_ref() else { + return Vec::new(); + }; + let mut vids: Vec = map.keys().map(|key| (key >> 16) as u16).collect(); + vids.sort_unstable(); + vids.dedup(); + vids +} + /// Runtime online overlay only (freshest, curated at workflow time). pub(crate) fn online_lookup(vid: u16, pid: u16) -> Option { let key = pack(vid, pid); diff --git a/crates/fbuild-core/src/usb/mod.rs b/crates/fbuild-core/src/usb/mod.rs index 2c235953..27c47873 100644 --- a/crates/fbuild-core/src/usb/mod.rs +++ b/crates/fbuild-core/src/usb/mod.rs @@ -25,9 +25,9 @@ pub mod resolver; pub use data::{ MANIFEST_URL, ONLINE_CACHE_TTL_SECS, USB_VID_JSON_URL, USB_VIDS_PROTO_ZSTD_URL, - install_online_cache, install_online_cache_proto_zstd, populate_online_cache_from_paths, - populate_online_cache_from_paths_and_urls, try_install_online_cache, - try_install_online_cache_proto_zstd, + install_online_cache, install_online_cache_proto_zstd, online_vendor_vids, + populate_online_cache_from_paths, populate_online_cache_from_paths_and_urls, + try_install_online_cache, try_install_online_cache_proto_zstd, }; #[cfg(test)] pub use embedded::vendor_name as embedded_vendor_name;