From 4cf035c7fe5e6507fa91bfbe834ff24f6aa4f233 Mon Sep 17 00:00:00 2001 From: gyanano <1624055384@qq.com> Date: Wed, 2 Sep 2026 04:23:00 -0700 Subject: [PATCH] =?UTF-8?q?feat(serial):=20reader=20lifecycle=20=E2=80=94?= =?UTF-8?q?=20bounded=20join,=20death=20detection,=20frame=20salvage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC #3 Step 3: - disconnect() returns the reader JoinHandle and the (bounded, 500ms) join now happens OUTSIDE the big manager lock in the Tauri command, replacing the in-lock 200ms sleep. connect() joins any stale reader defensively, fixing the close->reopen EBUSY race. - Reader-thread death is no longer silent: the error is recorded, the next status poll (1s) flips is_connected=false with connection_error set, and the frontend toasts 'Connection lost: ...' once per transition. ConnectionStatus gains a connection_error field. - Pending partial frames are flushed (salvaged) on ANY reader exit instead of vanishing; new FrameSegmenter::flush(). - Fix scheduled-send zombie state: on disconnect or emptied payload the SendPanel switch now resets instead of showing 'scheduled active' with a dead timer. Tests: 34/34 green (golden_reader_death_is_silent_today replaced by reader_death_marks_connection_lost_and_salvages_pending; new disconnect_returns_joinable_handle_and_reconnect_is_immediate + flush_drains_pending_regardless_of_idle). Hardware-verified: unplug detection + toast + switch reset, replug reconnect, rapid disconnect/reconnect cycles, recording/export regression. --- frontend/src/App.tsx | 9 +- frontend/src/components/SendPanel.tsx | 5 +- frontend/src/i18n/translations/en.json | 3 +- frontend/src/i18n/translations/zh-CN.json | 3 +- frontend/src/types.ts | 1 + src-tauri/src/framing.rs | 24 ++++ src-tauri/src/main.rs | 16 ++- src-tauri/src/serial_manager.rs | 135 +++++++++++++++++++--- src-tauri/src/types.rs | 4 + 9 files changed, 176 insertions(+), 24 deletions(-) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 0730314..164f5d2 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -435,7 +435,14 @@ function App() { const updateStatus = async () => { try { const status = await invoke('get_connection_status'); - setConnectionStatus(status); + // Surface an unexpected loss (fatal read error, e.g. cable unplugged) + // exactly once per transition; manual disconnects carry no error. + setConnectionStatus((prev) => { + if (prev.is_connected && !status.is_connected && status.connection_error) { + toast.error(t('app.connectionLost').replace('{error}', status.connection_error)); + } + return status; + }); } catch (error) { console.error('Failed to get status:', error); } diff --git a/frontend/src/components/SendPanel.tsx b/frontend/src/components/SendPanel.tsx index 841b65d..49d9b7d 100644 --- a/frontend/src/components/SendPanel.tsx +++ b/frontend/src/components/SendPanel.tsx @@ -226,10 +226,13 @@ const SendPanel: React.FC = ({ }; }, []); - // Cleanup when disconnected or value becomes empty + // Cleanup when disconnected or value becomes empty: scheduled sending no + // longer has its preconditions, so reset the switch too — otherwise the + // panel shows "scheduled active" with a dead timer underneath. useEffect(() => { if (!isConnected || !value.trim()) { stopScheduledSending(); + setIsScheduledEnabled(false); } }, [isConnected, value]); diff --git a/frontend/src/i18n/translations/en.json b/frontend/src/i18n/translations/en.json index d52a16f..2bd99a5 100644 --- a/frontend/src/i18n/translations/en.json +++ b/frontend/src/i18n/translations/en.json @@ -1,7 +1,8 @@ { "app": { "title": "RSerial Debug Assistant", - "subtitle": "Professional Tool" + "subtitle": "Professional Tool", + "connectionLost": "Connection lost: {error}" }, "sidebar": { "expandSidebar": "Expand Sidebar", diff --git a/frontend/src/i18n/translations/zh-CN.json b/frontend/src/i18n/translations/zh-CN.json index b6d8775..0984dc5 100644 --- a/frontend/src/i18n/translations/zh-CN.json +++ b/frontend/src/i18n/translations/zh-CN.json @@ -1,7 +1,8 @@ { "app": { "title": "RSerial Debug Assistant", - "subtitle": "Professional Tool" + "subtitle": "Professional Tool", + "connectionLost": "连接意外断开：{error}" }, "sidebar": { "expandSidebar": "展开侧边栏", diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 4a3dc90..7c5c635 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -54,6 +54,7 @@ export interface ConnectionStatus { bytes_sent: number; bytes_received: number; connection_time: string | null; + connection_error?: string | null; } // Quick Command types diff --git a/src-tauri/src/framing.rs b/src-tauri/src/framing.rs index e0da76d..3bf16c0 100644 --- a/src-tauri/src/framing.rs +++ b/src-tauri/src/framing.rs @@ -115,9 +115,21 @@ impl FrameSegmenter { } /// Bytes currently pending (received but not yet framed). + #[cfg(test)] pub fn pending(&self) -> &[u8] { &self.buffer } + + /// Drain any buffered bytes as a final frame regardless of idle time. + /// Used when the reader is shutting down or dying so the tail of the + /// stream is not silently dropped (RFC #3 Step 3). + pub fn flush(&mut self) -> Option> { + if self.buffer.is_empty() { + None + } else { + Some(std::mem::take(&mut self.buffer)) + } + } } /// Find the position of a delimiter in the data buffer. @@ -339,4 +351,16 @@ mod tests { assert_eq!(frames, vec![b"abcdefgh".to_vec(), b"\n".to_vec()]); assert!(seg.pending().is_empty()); } + + #[test] + fn flush_drains_pending_regardless_of_idle() { + let t0 = Instant::now(); + let mut seg = FrameSegmenter::new(combined(FrameDelimiter::LF), t0); + assert!(seg.flush().is_none()); + let frames = seg.feed(b"abc", t0); + assert!(frames.is_empty()); + assert_eq!(seg.flush(), Some(b"abc".to_vec())); + assert!(seg.pending().is_empty()); + assert!(seg.flush().is_none()); + } } diff --git a/src-tauri/src/main.rs b/src-tauri/src/main.rs index c05f867..33efda3 100644 --- a/src-tauri/src/main.rs +++ b/src-tauri/src/main.rs @@ -51,9 +51,17 @@ async fn connect_to_port( #[tauri::command] async fn disconnect_port(state: State<'_, AppState>) -> Result<(), String> { - let mut manager = state.serial_manager.lock().unwrap(); - manager.disconnect() - .map_err(|e| e.to_string()) + // Fast state cleanup under the lock; the (bounded) reader join happens + // AFTER the lock is released so polling commands are never blocked + // behind a thread wait (RFC #3 Step 3). + let handle = { + let mut manager = state.serial_manager.lock().unwrap(); + manager.disconnect().map_err(|e| e.to_string())? + }; + if let Some(h) = handle { + SerialManager::join_reader_bounded(h); + } + Ok(()) } #[tauri::command] @@ -107,7 +115,7 @@ async fn send_data( #[tauri::command] async fn get_connection_status(state: State<'_, AppState>) -> Result { - let manager = state.serial_manager.lock().unwrap(); + let mut manager = state.serial_manager.lock().unwrap(); Ok(manager.get_status()) } diff --git a/src-tauri/src/serial_manager.rs b/src-tauri/src/serial_manager.rs index f2ce1af..ea4058f 100644 --- a/src-tauri/src/serial_manager.rs +++ b/src-tauri/src/serial_manager.rs @@ -34,6 +34,12 @@ pub struct SerialManager { display_settings: Arc>, // Port opening seam (system opener in production, scripted fake in tests) port_opener: Arc, + // Reader thread lifecycle (RFC #3 Step 3) + reader_handle: Option>, + // Fatal read error written by the reader thread right before it dies + reader_error: Arc>>, + // Surfaced via ConnectionStatus until the next connect + connection_error: Option, } #[derive(Debug, Default)] @@ -117,6 +123,9 @@ impl SerialManager { timezone_offset_minutes: Arc::new(Mutex::new(0)), display_settings: Arc::new(Mutex::new(DisplaySettings::default())), port_opener: Arc::new(SystemPortOpener), + reader_handle: None, + reader_error: Arc::new(Mutex::new(None)), + connection_error: None, } } @@ -173,8 +182,18 @@ impl SerialManager { pub fn connect(&mut self, port_name: &str, config: SerialConfig) -> Result<()> { if self.is_connected { - self.disconnect()?; + let handle = self.disconnect()?; + if let Some(h) = handle { + Self::join_reader_bounded(h); + } + } + // Defensive: a reader that outlived a previous bounded join must be + // dead before the OS will let us reopen the same device (EBUSY race). + if let Some(h) = self.reader_handle.take() { + Self::join_reader_bounded(h); } + *self.reader_error.lock().unwrap() = None; + self.connection_error = None; // `mut` is only exercised by the POSIX write-timeout tweak below; // Windows shares timeouts across cloned handles and leaves it alone. @@ -194,6 +213,7 @@ impl SerialManager { let display_settings = Arc::clone(&self.display_settings); let port_name_clone = port_name.to_string(); let shutdown_flag = Arc::clone(&self.shutdown_flag); + let reader_error = Arc::clone(&self.reader_error); let mut read_port = port.try_clone()?; // Give the write side a longer timeout than the read-friendly 50 ms. @@ -205,7 +225,7 @@ impl SerialManager { warn!("Failed to set write timeout on {}: {}", port_name, e); } - thread::spawn(move || { + let reader_handle = thread::spawn(move || { let mut read_buffer = [0u8; 1024]; let initial_config = frame_segmentation_config.lock() .map(|guard| guard.clone()) @@ -305,12 +325,24 @@ impl SerialManager { } Err(e) => { error!("Error reading from serial port: {}", e); + *reader_error.lock().unwrap() = Some(format!("{}", e)); break; } } } + + // Salvage the pending partial frame on ANY exit (shutdown flag or + // fatal error) so the tail of the stream is not silently dropped + // (RFC #3 Step 3). + if let Some(frame) = segmenter.flush() { + let disp_settings = display_settings.lock() + .map(|guard| guard.clone()) + .unwrap_or_default(); + emit_frame(frame, &disp_settings); + } }); + self.reader_handle = Some(reader_handle); self.current_port = Some(port); self.config = Some(config); self.is_connected = true; @@ -327,7 +359,10 @@ impl SerialManager { Ok(()) } - pub fn disconnect(&mut self) -> Result<()> { + /// Disconnect. Returns the reader thread handle so the CALLER can join + /// it outside the big manager lock (RFC #3 Step 3) — see + /// `join_reader_bounded`. State cleanup here is fast and non-blocking. + pub fn disconnect(&mut self) -> Result>> { if self.is_connected { // Signal reading thread to stop self.shutdown_flag.store(true, Ordering::Relaxed); @@ -335,14 +370,16 @@ impl SerialManager { // Stop all recordings before disconnecting self.stop_all_recordings(); - // Close the port first to force the reading thread to exit + // Close the write handle; the reader's cloned fd closes when the + // thread exits. self.current_port = None; - // Wait longer for thread to properly clean up - thread::sleep(Duration::from_millis(200)); - self.is_connected = false; + // A manual disconnect supersedes any concurrent reader death: + // don't surface it as an unexpected loss afterwards. + *self.reader_error.lock().unwrap() = None; + if let Some(port_name) = &self.port_name { // Don't add disconnection log to reduce clutter info!("Disconnected from {}", port_name); @@ -358,7 +395,27 @@ impl SerialManager { info!("Serial port disconnected"); } - Ok(()) + Ok(self.reader_handle.take()) + } + + /// Join a reader thread with a bounded wait. The reader wakes at least + /// every ~50 ms (port read timeout), so 500 ms is generous; if it still + /// has not exited we drop the handle (detaching the thread) rather than + /// block — the thread dies on its own once the shutdown flag is set and + /// the port fd is closed. + pub fn join_reader_bounded(handle: thread::JoinHandle<()>) { + let deadline = Instant::now() + Duration::from_millis(500); + loop { + if handle.is_finished() { + let _ = handle.join(); + return; + } + if Instant::now() >= deadline { + warn!("Reader thread did not exit within 500ms; detaching"); + return; + } + thread::sleep(Duration::from_millis(5)); + } } pub fn send_data(&mut self, data: Vec) -> Result<()> { @@ -405,13 +462,32 @@ impl SerialManager { } } - pub fn get_status(&self) -> ConnectionStatus { + /// Lazy reader-death detection (RFC #3 Step 3): the reader thread records + /// a fatal error in `reader_error` before dying; the next status poll + /// (frontend: every second) turns that into a visible disconnect. + pub fn get_status(&mut self) -> ConnectionStatus { + if self.is_connected { + let death = self.reader_error.lock().unwrap().clone(); + if let Some(err) = death { + warn!("Reader thread died, marking connection lost: {}", err); + self.connection_error = Some(err); + self.is_connected = false; + // Free the write handle and harvest the (finished) reader so + // the device can be reopened immediately. + self.current_port = None; + self.stop_all_recordings(); + if let Some(h) = self.reader_handle.take() { + let _ = h.join(); + } + } + } + let (bytes_sent, bytes_received, connection_time) = if let Ok(stats_guard) = self.stats.lock() { (stats_guard.bytes_sent, stats_guard.bytes_received, stats_guard.connection_time) } else { (0, 0, None) }; - + ConnectionStatus { is_connected: self.is_connected, port_name: self.port_name.clone(), @@ -419,6 +495,7 @@ impl SerialManager { bytes_sent, bytes_received, connection_time, + connection_error: self.connection_error.clone(), } } @@ -1322,10 +1399,10 @@ mod tests { } #[test] - fn golden_reader_death_is_silent_today() { - // Pins the CURRENT (broken) behavior that RFC #3 Step 3 will fix: - // a fatal read error kills the thread silently — is_connected stays - // true and bytes pending in the accumulator are never framed. + fn reader_death_marks_connection_lost_and_salvages_pending() { + // RFC #3 Step 3: a fatal read error must surface — the next status + // poll reports disconnected with the error, and the pending partial + // frame is flushed instead of vanishing. let port = ScriptedPort::new( "SCRIPT", vec![ScriptEvent::Bytes(b"abc".to_vec()), ScriptEvent::Fail], @@ -1337,8 +1414,34 @@ mod tests { // Give the thread time to hit Fail and break. thread::sleep(Duration::from_millis(200)); - assert!(manager.get_status().is_connected); // the lie, pinned - assert!(manager.get_logs().is_empty()); // pending "abc" lost, pinned + + let status = manager.get_status(); + assert!(!status.is_connected); + assert_eq!(status.connection_error.as_deref(), Some("scripted failure")); + // Pending "abc" salvaged as a final frame on death. + assert_eq!(frame_bytes(&manager.get_logs()), vec![b"abc".to_vec()]); + + // A manual disconnect afterwards is a clean no-op. + assert!(manager.disconnect().unwrap().is_none()); + assert!(manager.get_status().connection_error.is_some()); // sticky until next connect + } + + #[test] + fn disconnect_returns_joinable_handle_and_reconnect_is_immediate() { + // RFC #3 Step 3: disconnect hands the reader handle to the caller; + // a bounded join then guarantees the device is free for reopen. + let port = ScriptedPort::new("SCRIPT", vec![]); + let mut manager = SerialManager::new() + .with_port_opener(Arc::new(ScriptedOpener { port })); + manager.connect("SCRIPT", SerialConfig::default()).unwrap(); + + let handle = manager.disconnect().unwrap().expect("reader handle"); + SerialManager::join_reader_bounded(handle); + + // Immediate reconnect must succeed (no stale reader, no EBUSY). + manager.connect("SCRIPT", SerialConfig::default()).unwrap(); + assert!(manager.get_status().is_connected); + assert!(manager.get_status().connection_error.is_none()); manager.disconnect().unwrap(); } diff --git a/src-tauri/src/types.rs b/src-tauri/src/types.rs index 01b3e51..48ee5e3 100644 --- a/src-tauri/src/types.rs +++ b/src-tauri/src/types.rs @@ -109,6 +109,10 @@ pub struct ConnectionStatus { pub bytes_sent: u64, pub bytes_received: u64, pub connection_time: Option>, + /// Fatal read error that ended the connection unexpectedly (RFC #3 + /// Step 3). `None` for normal connects/disconnects; cleared on connect. + #[serde(default)] + pub connection_error: Option, } #[derive(Debug, Clone, Serialize, Deserialize)]