fix/hid serial paths local app icons

src/api.rs

@@ -307,8 +307,32 @@ async fn set_custom_game_image(
     Ok(Json(ApiResult { ok: true }))
 }
 
-async fn local_applications(State(service): State<BridgeService>) -> Json<Vec<LocalApplication>> {
-    Json(service.local_applications().await)
+/// A saved local application as the API serves it, with the id its icon is
+/// served under. The id is never read back from a saved list.
+#[derive(Debug, Serialize)]
+#[serde(rename_all = "camelCase")]
+struct LocalApplicationEntry {
+    #[serde(flatten)]
+    application: LocalApplication,
+    icon_id: String,
+}
+
+fn local_application_entries(applications: Vec<LocalApplication>) -> Vec<LocalApplicationEntry> {
+    applications
+        .into_iter()
+        .map(|application| LocalApplicationEntry {
+            icon_id: crate::applications::icon_id(&application.path),
+            application,
+        })
+        .collect()
+}
+
+async fn local_applications(
+    State(service): State<BridgeService>,
+) -> Json<Vec<LocalApplicationEntry>> {
+    Json(local_application_entries(
+        service.local_applications().await,
+    ))
 }
 
 async fn replace_local_applications(
@@ -321,7 +345,7 @@ async fn replace_local_applications(
         .await
         .map_err(profile_error)?;
     Ok(Json(
-        serde_json::json!({"ok":true,"applications":applications}),
+        serde_json::json!({"ok":true,"applications":local_application_entries(applications)}),
     ))
 }
 
@@ -510,6 +534,43 @@ mod tests {
         (status, bytes, kind)
     }
 
+    #[tokio::test]
+    async fn saved_local_applications_carry_their_icon_id() {
+        let (app, dir) = app();
+        let path = r"C:\Apps\Editor.exe";
+        let icon_id = crate::applications::icon_id(path);
+        let entry = serde_json::json!({"id":"local:editor","name":"Editor","executable":"Editor.exe","path":path,"kind":"app"});
+        let (status, saved, _) = call(
+            &app,
+            Method::PUT,
+            "/v1/local-applications",
+            "application/json",
+            serde_json::to_vec(&serde_json::json!({"applications":[entry]})).unwrap(),
+        )
+        .await;
+        assert_eq!(status, StatusCode::OK);
+        let saved: serde_json::Value = serde_json::from_slice(&saved).unwrap();
+        assert_eq!(saved["applications"][0]["iconId"], icon_id.as_str());
+
+        let (status, listed, _) =
+            call(&app, Method::GET, "/v1/local-applications", "", vec![]).await;
+        assert_eq!(status, StatusCode::OK);
+        let listed: serde_json::Value = serde_json::from_slice(&listed).unwrap();
+        assert_eq!(listed[0]["iconId"], icon_id.as_str());
+
+        // A client may send back the list it was served, icon ids included.
+        let (status, ..) = call(
+            &app,
+            Method::PUT,
+            "/v1/local-applications",
+            "application/json",
+            serde_json::to_vec(&serde_json::json!({"applications":listed})).unwrap(),
+        )
+        .await;
+        assert_eq!(status, StatusCode::OK);
+        std::fs::remove_dir_all(dir).expect("cleanup");
+    }
+
     #[tokio::test]
     async fn a_custom_game_keeps_the_image_saved_for_it() {
         let (app, dir) = app();

src/applications.rs

@@ -15,11 +15,7 @@ pub struct ApplicationInfo {
     pub icon_id: String,
 }
 
-#[cfg_attr(
-    not(any(target_os = "windows", target_os = "macos", target_os = "linux")),
-    allow(dead_code)
-)]
-fn icon_id(path: &str) -> String {
+pub fn icon_id(path: &str) -> String {
     let mut hasher = DefaultHasher::new();
     crate::games::path_key(path).hash(&mut hasher);
     format!("{:016x}", hasher.finish())

src/hid.rs

@@ -843,6 +843,9 @@ impl HidSession {
             .get_mut(key)
             .ok_or_else(|| "the HID interface must be open before listening".to_owned())?;
         let stop = Arc::new(AtomicBool::new(false));
+        // Stored before any reader starts, so a later spawn failure can stop and
+        // join the readers already running instead of leaving them to block cleanup.
+        open.stop = Some(Arc::clone(&stop));
         for (index, (device, buffer_len, uses_report_ids)) in reader_specs.into_iter().enumerate() {
             let thread_stop = Arc::clone(&stop);
             let tx = self.input_tx.clone();
@@ -884,12 +887,16 @@ impl HidSession {
                             }
                         }
                     }
-                })
-                .map_err(|error| format!("could not start the HID input reader: {error}"))?;
-            open.readers.push(reader);
+                });
+            match reader {
+                Ok(reader) => open.readers.push(reader),
+                Err(error) => {
+                    open.stop_reader();
+                    return Err(format!("could not start the HID input reader: {error}"));
+                }
+            }
         }
         open.listening = true;
-        open.stop = Some(stop);
         Ok(())
     }
 
@@ -915,35 +922,20 @@ impl HidSession {
             .open_devices
             .get(key)
             .ok_or_else(|| "the HID interface is not open".to_owned())?;
-        if open.paths.len() != 1 {
-            return Err("HID physical identity is ambiguous; refusing report fan-out".into());
+        // An open device has several paths only when `inspect_candidate` verified
+        // they share one serial. A report still goes to exactly one of them.
+        if open.paths.is_empty() {
+            return Err("the HID interface has no open path".into());
         }
         let _access = crate::drivers::manual_report_access(open.vendor_id)
             .map_err(|error| error.to_string())?;
         let mut frame = Vec::with_capacity(data.len() + 1);
         frame.push(report_id);
         frame.extend_from_slice(&data);
-        let mut sent = false;
         let mut last_error = None;
-        let declared_path_exists = open.paths.iter().any(|path| {
-            let reports = if feature {
-                &path.layout.feature_bits
-            } else {
-                &path.layout.output_bits
-            };
-            reports.contains_key(&report_id)
-        });
-        for path in &open.paths {
-            if declared_path_exists {
-                let reports = if feature {
-                    &path.layout.feature_bits
-                } else {
-                    &path.layout.output_bits
-                };
-                if !reports.contains_key(&report_id) {
-                    continue;
-                }
-            }
+        let layouts: Vec<&ReportLayout> = open.paths.iter().map(|path| &path.layout).collect();
+        for index in report_paths(&layouts, report_id, feature) {
+            let path = &open.paths[index];
             let result = path
                 .device
                 .lock()
@@ -956,20 +948,18 @@ impl HidSession {
                     }
                     .map_err(|error| error.to_string())
                 });
+            // The first path that takes the report is the only one written, so a
+            // command never reaches the device twice.
             match result {
-                Ok(()) => sent = true,
+                Ok(()) => return Ok(()),
                 Err(error) => last_error = Some(error),
             }
         }
-        if sent {
-            Ok(())
-        } else {
-            let operation = if feature { "feature" } else { "output" };
-            Err(format!(
-                "could not send {operation} report {report_id}: {}",
-                last_error.unwrap_or_else(|| "no HID path was open".to_owned())
-            ))
-        }
+        let operation = if feature { "feature" } else { "output" };
+        Err(format!(
+            "could not send {operation} report {report_id}: {}",
+            last_error.unwrap_or_else(|| "no HID path was open".to_owned())
+        ))
     }
 
     fn receive_input_report(&self, key: &str, report_id: u8) -> Result<Vec<u8>, String> {
@@ -1113,6 +1103,35 @@ fn device_group_key(info: &hidapi::DeviceInfo) -> Vec<u8> {
     )
 }
 
+/// The paths to try for a report, in order: those whose descriptor declares it,
+/// or every path when none does (Razer descriptors are not parsed). The sender
+/// stops at the first path that takes the report.
+fn report_paths(layouts: &[&ReportLayout], report_id: u8, feature: bool) -> Vec<usize> {
+    let declares = |layout: &ReportLayout| {
+        if feature {
+            layout.feature_bits.contains_key(&report_id)
+        } else {
+            layout.output_bits.contains_key(&report_id)
+        }
+    };
+    let declared: Vec<usize> = (0..layouts.len())
+        .filter(|&index| declares(layouts[index]))
+        .collect();
+    if declared.is_empty() {
+        (0..layouts.len()).collect()
+    } else {
+        declared
+    }
+}
+
+/// True when every interface reports the same non-empty serial.
+fn shares_verified_serial<'a>(mut serials: impl Iterator<Item = Option<&'a str>>) -> bool {
+    match serials.next().flatten().filter(|serial| !serial.is_empty()) {
+        Some(first) => serials.all(|serial| serial == Some(first)),
+        None => false,
+    }
+}
+
 fn inspect_candidate(
     api: &HidApi,
     key: String,
@@ -1129,7 +1148,11 @@ fn inspect_candidate(
         }
     }
 
-    if unique_paths.len() != 1 {
+    // Several paths are the collections of one device only when they share a
+    // verified serial; that is the one case `grouped_device_key` merges them.
+    if unique_paths.len() > 1
+        && !shares_verified_serial(infos.iter().map(hidapi::DeviceInfo::serial_number))
+    {
         return Err("HID physical identity is ambiguous; refusing multi-path access".into());
     }
 
@@ -1655,6 +1678,38 @@ mod tests {
         );
     }
 
+    #[test]
+    fn several_paths_open_only_under_one_verified_serial() {
+        assert!(shares_verified_serial(
+            [Some("receiver-123"), Some("receiver-123")].into_iter()
+        ));
+        assert!(!shares_verified_serial(
+            [Some("receiver-123"), Some("receiver-456")].into_iter()
+        ));
+        assert!(!shares_verified_serial(
+            [Some("receiver-123"), None].into_iter()
+        ));
+        assert!(!shares_verified_serial([None, None].into_iter()));
+        assert!(!shares_verified_serial([Some(""), Some("")].into_iter()));
+        assert!(!shares_verified_serial(std::iter::empty()));
+    }
+
+    #[test]
+    fn a_report_goes_to_the_paths_that_declare_it_first() {
+        let mut vendor = ReportLayout::default();
+        vendor.feature_bits.insert(0x11, 160);
+        vendor.output_bits.insert(0x10, 48);
+        let mouse = ReportLayout::default();
+        let mut second = ReportLayout::default();
+        second.feature_bits.insert(0x11, 160);
+        let layouts = [&mouse, &vendor, &second];
+        assert_eq!(report_paths(&layouts, 0x11, true), vec![1, 2]);
+        assert_eq!(report_paths(&layouts, 0x10, false), vec![1]);
+        // Undeclared (a Razer path without parsed descriptors): every path is a candidate, tried in order.
+        assert_eq!(report_paths(&layouts, 0x00, true), vec![0, 1, 2]);
+        assert!(report_paths(&[], 0x11, true).is_empty());
+    }
+
     #[test]
     fn report_payload_never_reads_past_the_buffer() {
         // A full unnumbered Razer read: hidapi on Windows reports 92 bytes.