health: name the device a run of bus read failures stopped at

duck-control/Cargo.toml

@@ -36,7 +36,7 @@ ort = { version = "=2.0.0-rc.11", default-features = false, features = ["load-dy
 # for the same kind of reason — it de-stuffs protocol 2.0 status packets whose payload
 # contains `FF FF FD`, and before it such a read came back oversized and every fixed-size
 # sync read in `bus` decoded garbage. A present current of -1 is enough to trigger that.
-rustypot = "1.8.0"
+rustypot = "1.12.0"
 # `default-features = false` drops serialport's `libudev` feature, which exists only for
 # enumerating ports (`available_ports`). We open one by path from the params file and never
 # enumerate — and libudev-sys needs a pkg-config sysroot that the board's cross-build does

duck-control/src/bus.rs

@@ -395,7 +395,7 @@ impl DynamixelIo {
         let values = self
             .controller
             .sync_read_present_position(&JOINT_IDS)
-            .map_err(|e| IoError::Bus(format!("read present positions: {e}")))?;
+            .map_err(|e| sync_read_error("read present positions", e))?;
         if values.len() != NUM_JOINTS {
             return Err(IoError::ShortRead {
                 what: "present positions",
@@ -515,7 +515,7 @@ impl RobotIo for DynamixelIo {
         let blocks = self
             .controller
             .sync_read_raw_data(&self.ids, READ_ADDR, READ_LEN)
-            .map_err(|e| IoError::Bus(format!("combined imu+motor sync_read: {e}")))?;
+            .map_err(|e| sync_read_error("combined imu+motor sync_read", e))?;
 
         if blocks.len() != self.ids.len() {
             return Err(IoError::ShortRead {
@@ -630,7 +630,7 @@ impl RobotIo for DynamixelIo {
         let blocks = self
             .controller
             .sync_read_raw_data(&JOINT_IDS, SLOW_READ_ADDR, SLOW_READ_LEN)
-            .map_err(|e| IoError::Bus(format!("voltage+temperature sync_read: {e}")))?;
+            .map_err(|e| sync_read_error("voltage+temperature sync_read", e))?;
 
         if blocks.len() != NUM_JOINTS {
             return Err(IoError::ShortRead {
@@ -724,6 +724,18 @@ impl RobotIo for DynamixelIo {
     }
 }
 
+/// A failed sync read, keeping the device the bus says it stopped at.
+fn sync_read_error(what: &'static str, e: Box<dyn std::error::Error>) -> IoError {
+    match e.downcast_ref::<rustypot::SyncReadError>() {
+        Some(at) => IoError::Device {
+            what,
+            id: at.id,
+            cause: at.cause.to_string(),
+        },
+        None => IoError::Bus(format!("{what}: {e}")),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -742,6 +754,27 @@ mod tests {
     /// The block parsed per servo must cover current, velocity and position without
     /// overrunning. If `READ_LEN` and the offsets below ever disagree, joints get each
     /// other's values — which reads as a wiring fault, not a code bug.
+    #[test]
+    fn a_sync_read_failure_keeps_the_device_it_stopped_at() {
+        let at = rustypot::SyncReadError {
+            id: IMU_DXL_ID,
+            cause: Box::new(rustypot::CommunicationErrorKind::ParsingError),
+        };
+        let e = sync_read_error("combined imu+motor sync_read", Box::new(at));
+        assert_eq!(e.device(), Some(IMU_DXL_ID));
+        assert_eq!(
+            e.to_string(),
+            "bus transaction failed: combined imu+motor sync_read: device 200: Parsing error"
+        );
+
+        // The port itself is nobody's.
+        let e = sync_read_error(
+            "combined imu+motor sync_read",
+            Box::new(std::io::Error::other("unplugged")),
+        );
+        assert_eq!(e.device(), None);
+    }
+
     #[test]
     fn read_block_is_long_enough_for_every_field() {
         assert_eq!(READ_LEN as usize, IMU_BLOCK_LEN);

duck-control/src/io.rs

@@ -59,6 +59,15 @@ pub enum IoError {
     },
     #[error("bus transaction failed: {0}")]
     Bus(String),
+    /// A sync read the bus says went wrong at one device's answer — which device, from what
+    /// arrived (`rustypot`'s `SyncReadError`). The device is where the answer broke, not proof
+    /// the rest are fine: see `duck_ipc_proto::BusHealth::failing`.
+    #[error("bus transaction failed: {what}: device {id}: {cause}")]
+    Device {
+        what: &'static str,
+        id: u8,
+        cause: String,
+    },
     /// A `sync_read` that returns the wrong number of blocks, or a block of the wrong
     /// length, means a device did not answer. Reported rather than papered over: a
     /// silently short read would leave stale values in half the joint array.
@@ -72,6 +81,16 @@ pub enum IoError {
     Simulated,
 }
 
+impl IoError {
+    /// The device the bus pinned this failure on, when it could.
+    pub fn device(&self) -> Option<u8> {
+        match self {
+            Self::Device { id, .. } => Some(*id),
+            _ => None,
+        }
+    }
+}
+
 pub type Result<T> = std::result::Result<T, IoError>;
 
 /// What the servos report about their own supply and case, rather than their motion.

duck-control/src/model.rs

@@ -97,8 +97,9 @@ pub fn mouth_target(open: f64) -> f64 {
 }
 
 /// The `imu_to_dxl` v2 board's Dynamixel ID. It rides the motor bus and is read in the
-/// same transaction as the servos ([`crate::bus`]).
-pub const IMU_DXL_ID: u8 = 200;
+/// same transaction as the servos ([`crate::bus`]). Protocol, so a client naming a device a
+/// failed read stopped at uses the same number.
+pub use duck_ipc_proto::IMU_DXL_ID;
 
 pub const BAUD_RATE: u32 = 1_000_000;
 

duck-ipc-proto/src/lib.rs

@@ -457,7 +457,19 @@ pub const JSONRPC_VERSION: &str = "2.0";
 /// [`method::ROBOT_VETBOOK`] and [`method::ROBOT_VETBOOK_NOTE`]: the robot's lifetime record —
 /// hours on, distance walked, falls and pickups, every servo `robotd` has adopted and how long
 /// the one before it lasted, and dated notes ([`vetbook`]). New routes: a `robotd` predating them answers METHOD_NOT_FOUND, by name.
-pub const API_VERSION: u32 = 42;
+///
+/// # v43 — which device a run of failed bus reads stopped at
+///
+/// [`BusHealth::failing`]: the device each failed read of the current run was pinned on — a servo,
+/// or the IMU board. "motor bus: 1 read failures running" said the bus dropped a read and not
+/// where, and on a robot whose drops are all one device's, the device is the whole finding.
+///
+/// [`BusHealth::drops`] and [`BusHealth::unnamed_drops`]: every failed read since `robotd`
+/// started, by device. A count that resets on the next good read is invisible to a client
+/// sampling once a second; a total that only grows is not.
+///
+/// Additive: absent from an older `robotd`, and empty reads as "not told".
+pub const API_VERSION: u32 = 43;
 
 /// The observation width every policy this robot family runs is built against.
 ///
@@ -586,6 +598,11 @@ pub const JOINT_IDS: [u8; 15] = [
     10, 11, 12, 13, 14, // right leg
 ];
 
+/// The IMU board's Dynamixel ID. It rides the motor bus and answers first in the combined read, so
+/// it is a device [`BusHealth::failing`] can name. `duck-control` re-exports it, as it does
+/// [`JOINT_IDS`].
+pub const IMU_DXL_ID: u8 = 200;
+
 /// The joint a Dynamixel ID drives, if it is one of [`JOINT_IDS`].
 pub fn joint_of(id: u8) -> Option<&'static str> {
     JOINT_IDS
@@ -3689,6 +3706,24 @@ pub struct BusHealth {
     /// unplugged, and naming them is the point.
     #[serde(skip_serializing_if = "Vec::is_empty")]
     pub missing: Vec<u8>,
+    /// While `consecutive_errors` is non-zero: the device each failed read of the run stopped at,
+    /// once each, in the order first seen — a servo ID, or [`IMU_DXL_ID`]. Emptied with the count.
+    ///
+    /// The read is one transaction for every device, so this is where the answer went wrong, not
+    /// a proof the others are fine: a cable that breaks at one device silences every one past
+    /// it, and the first silent one is the one named. A failure the bus could not pin on a device
+    /// (the port itself) adds nothing, so a run can be counted with nobody named.
+    #[serde(skip_serializing_if = "Vec::is_empty")]
+    pub failing: Vec<u8>,
+    /// Failed reads since `robotd` started, by the device each stopped at — a servo ID, or
+    /// [`IMU_DXL_ID`]. Only ever grows, and that is the point: an isolated drop holds
+    /// `consecutive_errors` for one 20 ms tick, which a client sampling health once a second sees
+    /// one time in fifty. A total that went up says a drop happened in between, and where.
+    #[serde(skip_serializing_if = "std::collections::BTreeMap::is_empty")]
+    pub drops: std::collections::BTreeMap<u8, u64>,
+    /// Failed reads since `robotd` started that the bus could not pin on a device.
+    #[serde(skip_serializing_if = "is_zero")]
+    pub unnamed_drops: u64,
 }
 
 impl BusHealth {
@@ -3700,17 +3735,62 @@ impl BusHealth {
 
     /// The missing servos as people read them: `32 head_yaw, 33 head_roll`.
     pub fn describe_missing(&self) -> String {
-        self.missing
+        describe_devices(&self.missing)
+    }
+
+    /// The devices the current run of failures stopped at, the same way: `200 imu, 34 mouth`.
+    pub fn describe_failing(&self) -> String {
+        describe_devices(&self.failing)
+    }
+
+    /// Every failed read since `robotd` started.
+    pub fn total_drops(&self) -> u64 {
+        self.drops.values().sum::<u64>() + self.unnamed_drops
+    }
+
+    /// Where the reads since `robotd` started dropped: `20 left_hip_yaw ×2, 32 head_yaw ×1`.
+    pub fn describe_drops(&self) -> String {
+        let mut by_device: Vec<String> = self
+            .drops
             .iter()
-            .map(|&id| match joint_of(id) {
-                Some(joint) => format!("{id} {joint}"),
-                None => id.to_string(),
-            })
-            .collect::<Vec<_>>()
-            .join(", ")
+            .map(|(&id, n)| format!("{} ×{n}", describe_device(id)))
+            .collect();
+        if self.unnamed_drops > 0 {
+            by_device.push(format!("unknown device ×{}", self.unnamed_drops));
+        }
+        by_device.join(", ")
+    }
+
+    /// The current run of failed reads, for a client to show while `consecutive_errors` is
+    /// non-zero: `3 read failures running, at 200 imu`.
+    pub fn describe_run(&self) -> String {
+        let n = self.consecutive_errors;
+        let run = format!("{n} read failure{} running", if n == 1 { "" } else { "s" });
+        if self.failing.is_empty() {
+            run
+        } else {
+            format!("{run}, at {}", self.describe_failing())
+        }
     }
 }
 
+/// A bus device as people read it: the ID printed on the part, and what it is — `34 mouth`,
+/// `200 imu`.
+pub fn describe_device(id: u8) -> String {
+    match joint_of(id) {
+        Some(joint) => format!("{id} {joint}"),
+        None if id == IMU_DXL_ID => format!("{id} imu"),
+        None => id.to_string(),
+    }
+}
+
+fn describe_devices(ids: &[u8]) -> String {
+    ids.iter()
+        .map(|&id| describe_device(id))
+        .collect::<Vec<_>>()
+        .join(", ")
+}
+
 /// The IMU board, which rides the motor bus.
 ///
 /// `#[serde(default)]` on the struct, not on each field, and for the same reason the parent
@@ -6609,6 +6689,43 @@ mod tests {
         assert_eq!(answer.bus.startup_failures, 0);
     }
 
+    #[test]
+    fn the_devices_a_run_stopped_at_name_the_imu_board_too() {
+        let bus = BusHealth {
+            consecutive_errors: 2,
+            failing: vec![IMU_DXL_ID, 34, 99],
+            ..BusHealth::default()
+        };
+        assert_eq!(bus.describe_failing(), "200 imu, 34 mouth, 99");
+        assert_eq!(
+            bus.describe_run(),
+            "2 read failures running, at 200 imu, 34 mouth, 99"
+        );
+        let unnamed = BusHealth {
+            consecutive_errors: 1,
+            ..BusHealth::default()
+        };
+        assert_eq!(unnamed.describe_run(), "1 read failure running");
+
+        let drops = BusHealth {
+            drops: [(32, 1), (20, 2)].into(),
+            unnamed_drops: 1,
+            ..BusHealth::default()
+        };
+        assert_eq!(drops.total_drops(), 4);
+        assert_eq!(
+            drops.describe_drops(),
+            "20 left_hip_yaw ×2, 32 head_yaw ×1, unknown device ×1"
+        );
+        // A map keyed by ID crosses JSON with string keys, and comes back the same.
+        let json = serde_json::to_string(&drops).unwrap();
+        assert!(json.contains(r#""drops":{"20":2,"32":1}"#), "{json}");
+        assert_eq!(serde_json::from_str::<BusHealth>(&json).unwrap(), drops);
+        // Not told is not serialised: an older client sees the section it always did.
+        let json = serde_json::to_string(&BusHealth::default()).unwrap();
+        assert!(!json.contains("failing"), "{json}");
+    }
+
     /// An absent battery must stay absent, not become zero volts.
     ///
     /// This is the answer for the first second after startup, for a bus that cannot reply,