refactor(sftool-lib): centralize cancellable serial io by HalfSweet · Pull Request #52 · OpenSiFli/sftool

HalfSweet · 2026-04-23T07:49:46Z

No description provided.

Copilot

Pull request overview

This PR introduces a cancellable I/O layer for serial operations and refactors chip/common code paths to route reads/writes/sleeps through that layer, enabling cooperative cancellation across long-running flashing/debug operations.

Changes:

Add CancelToken + Error::Cancelled, propagate cancellation checks through SifliToolBase/SifliToolTrait.
Introduce common::serial_io (SerialIo, cancel-aware sleep, pattern/prompt waiting, cancellable cloned reader/writer) and refactor common + chip code to use it.
Add tests covering cancel token behavior, cancel-aware serial helpers, and cancellation propagation through cloned debug streams.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
sftool-lib/tests/cancel_token_test.rs	Adds a unit test for `CancelToken` state/reporting.
sftool-lib/src/lib.rs	Adds `CancelToken`, stores it in `SifliToolBase`, and exposes `check_cancelled()` on tool trait.
sftool-lib/src/error.rs	Introduces `Error::Cancelled` and helper constructor.
sftool-lib/src/common/serial_io.rs	New cancel-aware serial I/O wrapper + helpers + tests.
sftool-lib/src/common/mod.rs	Exposes the new `serial_io` module.
sftool-lib/src/common/ram_command.rs	Refactors `RamOps` to operate on `SerialIo` for cancellable I/O.
sftool-lib/src/common/read_flash.rs	Refactors flash read path to use `SerialIo` helpers and cancellation.
sftool-lib/src/common/write_flash.rs	Adds cancellation checks and uses `SerialIo` for direct port writes.
sftool-lib/src/common/speed.rs	Uses cancel-aware sleep + `SerialIo` for baud/clear/prompt wait.
sftool-lib/src/common/erase_flash.rs	Uses `SerialIo` pattern wait and adds cancellation checks.
sftool-lib/src/common/sifli_debug.rs	Makes debug UART I/O cancellation-aware (including cloned streams) + adds tests.
sftool-lib/src/sf32lb52/sifli_debug.rs	Maps cancelled I/O into `RecvError::Cancelled`.
sftool-lib/src/sf32lb52/ram_command.rs	Uses `for_tool`/`sleep_with_cancel` in stub + RAM command operations.
sftool-lib/src/sf32lb52/mod.rs	Uses `SerialIo` for erase/wait, RTS toggling sleeps, etc.
sftool-lib/src/sf32lb55/ram_command.rs	Routes RAM commands through `SerialIo`.
sftool-lib/src/sf32lb55/mod.rs	Refactors DFU send/wait to `SerialIo`, adds cancellation checks.
sftool-lib/src/sf32lb56/ram_command.rs	Routes RAM commands through `SerialIo`, cancel-aware sleeps/clear.
sftool-lib/src/sf32lb56/mod.rs	Refactors erase/read loops, RTS sleeps, and frame parsing to support cancellation.
sftool-lib/src/sf32lb58/ram_command.rs	Routes RAM commands through `SerialIo`.
sftool-lib/src/sf32lb58/mod.rs	Refactors DFU send/wait to `SerialIo`, adds cancellation checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T07:57:01Z

+        let start = Instant::now();
+        let mut offset = 0usize;
+
+        while offset < buf.len() {
+            self.check_cancelled()?;
+            match self.port.read(&mut buf[offset..]) {
+                Ok(0) => {
+                    if start.elapsed() > timeout {
+                        return Err(Error::timeout(format!("waiting for {}", context)));
+                    }
+                }
+                Ok(n) => {
+                    offset += n;
+                }
+                Err(error)
+                    if matches!(error.kind(), ErrorKind::TimedOut | ErrorKind::WouldBlock) =>
+                {
+                    if start.elapsed() > timeout {


SerialIo::read_exact_with_timeout measures the timeout from the initial call (start = Instant::now()) and never resets it after partial progress. This can cause spurious timeouts when large reads make steady progress but take longer than timeout overall (the old read_flash implementation used an inactivity/last-activity timeout). Consider tracking last_activity (reset on each successful read) or updating start whenever bytes are read so the timeout represents “no data received for N ms”.

Suggested change

let start = Instant::now();

let mut offset = 0usize;

while offset < buf.len() {

self.check_cancelled()?;

match self.port.read(&mut buf[offset..]) {

Ok(0) => {

if start.elapsed() > timeout {

return Err(Error::timeout(format!("waiting for {}", context)));

}

}

Ok(n) => {

offset += n;

}

Err(error)

if matches!(error.kind(), ErrorKind::TimedOut | ErrorKind::WouldBlock) =>

{

if start.elapsed() > timeout {

let mut last_activity = Instant::now();

let mut offset = 0usize;

while offset < buf.len() {

self.check_cancelled()?;

match self.port.read(&mut buf[offset..]) {

Ok(0) => {

if last_activity.elapsed() > timeout {

return Err(Error::timeout(format!("waiting for {}", context)));

}

}

Ok(n) => {

offset += n;

last_activity = Instant::now();

}

Err(error)

if matches!(error.kind(), ErrorKind::TimedOut | ErrorKind::WouldBlock) =>

{

if last_activity.elapsed() > timeout {

Copilot · 2026-04-23T07:57:02Z

+        loop {
+            self.check_cancelled()?;
+            if start.elapsed() > timeout {
+                return Err(Error::timeout(format!("waiting for {}", context)));
+            }
+
+            let mut byte = [0u8; 1];
+            match self.port.read(&mut byte) {
+                Ok(0) => continue,
+                Ok(_) => {
+                    buffer.push(byte[0]);
+                    window.push_back(byte[0]);
+                    if window.len() > max_len {
+                        window.pop_front();
+                    }
+
+                    for (index, pattern) in patterns.iter().enumerate() {
+                        if window.len() >= pattern.len()
+                            && window
+                                .iter()
+                                .rev()
+                                .take(pattern.len())
+                                .rev()
+                                .copied()
+                                .eq(pattern.iter().copied())
+                        {
+                            return Ok(PatternMatch { index, buffer });
+                        }
+                    }
+                }
+                Err(error)
+                    if matches!(error.kind(), ErrorKind::TimedOut | ErrorKind::WouldBlock) =>
+                {
+                    continue;
+                }


wait_for_patterns busy-loops on Ok(0) and TimedOut/WouldBlock by immediately continuing without any backoff. With short serial timeouts this can peg a CPU core during long waits. Consider adding a small sleep/yield (still checking cancellation) on these branches, or relying on a blocking read with an appropriate port timeout.

Copilot · 2026-04-23T07:57:02Z

        tracing::trace!("Waiting for OK response with timeout: {}ms", timeout_ms);
-
-        loop {
-            let elapsed = start_time.elapsed().unwrap().as_millis() as u64;
-            if elapsed > timeout_ms {
-                let response_str = String::from_utf8_lossy(&buffer);
-                tracing::error!(
-                    "Timeout waiting for OK response after {}ms. Received: '{}'",
-                    elapsed,
-                    response_str
-                );
-                return Err(std::io::Error::new(
-                    std::io::ErrorKind::TimedOut,
-                    format!("Timeout waiting for OK response: {}", response_str),
-                )
-                .into());
-            }
-
-            // 每秒记录一次等待状态
-            if elapsed > 0
-                && elapsed.is_multiple_of(1000)
-                && start_time.elapsed().unwrap()
-                    > last_log_time.elapsed().unwrap() + Duration::from_secs(1)
-            {
-                tracing::trace!("Still waiting for response... elapsed: {}ms", elapsed);
-                last_log_time = std::time::SystemTime::now();
-            }
-
-            let mut byte = [0];
-            if self.port.read_exact(&mut byte).is_ok() {
-                buffer.push(byte[0]);
-
-                // 检查是否收到"OK"响应
-                if buffer.windows(2).any(|window| window == b"OK") {
-                    let response_str = String::from_utf8_lossy(&buffer);
-                    tracing::trace!(
-                        "Received OK response after {}ms: '{}'",
-                        elapsed,
-                        response_str
-                    );
-                    return Ok(());
-                }
-
-                // 检查是否收到"Fail"响应
-                if buffer.windows(4).any(|window| window == b"Fail") {
-                    let response_str = String::from_utf8_lossy(&buffer);
-                    tracing::error!(
-                        "Received Fail response after {}ms: '{}'",
-                        elapsed,
-                        response_str
-                    );
-                    return Err(std::io::Error::other(format!(
-                        "Received Fail response: {}",
-                        response_str
-                    ))
-                    .into());
-                }
-
-                // 限制缓冲区大小，避免内存占用过多
-                if buffer.len() > 1024 {
-                    let response_str = String::from_utf8_lossy(&buffer);
-                    tracing::warn!(
-                        "Response buffer too large ({}), truncating. Content: '{}'",
-                        buffer.len(),
-                        response_str
-                    );
-                    buffer.drain(..512); // 保留后半部分
-                }
-            }
+        let matched = {
+            let mut io = for_tool(self);
+            io.wait_for_patterns(
+                &[b"OK", b"Fail"],
+                Duration::from_millis(timeout_ms),
+                "OK response",
+            )?
+        };


This now stores the entire serial stream in matched.buffer until a pattern matches. The previous implementation bounded the buffer (draining when it exceeded a limit) to avoid unbounded memory growth if the device outputs lots of log data while waiting. Consider adding a max buffer size (either in SerialIo::wait_for_patterns or locally here) and truncating older bytes while still preserving enough tail bytes to match the patterns.

Copilot · 2026-04-23T07:57:02Z

        tracing::trace!("Waiting for OK response with timeout: {}ms", timeout_ms);
-
-        loop {
-            let elapsed = start_time.elapsed().unwrap().as_millis() as u64;
-            if elapsed > timeout_ms {
-                let response_str = String::from_utf8_lossy(&buffer);
-                tracing::error!(
-                    "Timeout waiting for OK response after {}ms. Received: '{}'",
-                    elapsed,
-                    response_str
-                );
-                return Err(std::io::Error::new(
-                    std::io::ErrorKind::TimedOut,
-                    format!("Timeout waiting for OK response: {}", response_str),
-                )
-                .into());
-            }
-
-            // 每秒记录一次等待状态
-            if elapsed > 0
-                && elapsed.is_multiple_of(1000)
-                && start_time.elapsed().unwrap()
-                    > last_log_time.elapsed().unwrap() + Duration::from_secs(1)
-            {
-                tracing::trace!("Still waiting for response... elapsed: {}ms", elapsed);
-                last_log_time = std::time::SystemTime::now();
-            }
-
-            let mut byte = [0];
-            if self.port.read_exact(&mut byte).is_ok() {
-                buffer.push(byte[0]);
-
-                // 检查是否收到"OK"响应
-                if buffer.windows(2).any(|window| window == b"OK") {
-                    let response_str = String::from_utf8_lossy(&buffer);
-                    tracing::trace!(
-                        "Received OK response after {}ms: '{}'",
-                        elapsed,
-                        response_str
-                    );
-                    return Ok(());
-                }
-
-                // 检查是否收到"Fail"响应
-                if buffer.windows(4).any(|window| window == b"Fail") {
-                    let response_str = String::from_utf8_lossy(&buffer);
-                    tracing::error!(
-                        "Received Fail response after {}ms: '{}'",
-                        elapsed,
-                        response_str
-                    );
-                    return Err(std::io::Error::other(format!(
-                        "Received Fail response: {}",
-                        response_str
-                    ))
-                    .into());
-                }
-
-                // 限制缓冲区大小，避免内存占用过多
-                if buffer.len() > 1024 {
-                    let response_str = String::from_utf8_lossy(&buffer);
-                    tracing::warn!(
-                        "Response buffer too large ({}), truncating. Content: '{}'",
-                        buffer.len(),
-                        response_str
-                    );
-                    buffer.drain(..512); // 保留后半部分
-                }
-            }
+        let matched = {
+            let mut io = for_tool(self);
+            io.wait_for_patterns(
+                &[b"OK", b"Fail"],
+                Duration::from_millis(timeout_ms),
+                "OK response",
+            )?
+        };


This now stores the entire serial stream in matched.buffer until a pattern matches. The previous implementation bounded the buffer (draining when it exceeded a limit) to avoid unbounded memory growth if the device outputs lots of log data while waiting. Consider adding a max buffer size (either in SerialIo::wait_for_patterns or locally here) and truncating older bytes while still preserving enough tail bytes to match the patterns.

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T08:07:41Z

            .unwrap();
        port.write_request_to_send(false).unwrap();
-        std::thread::sleep(Duration::from_millis(100));
+        sleep_with_cancel(&base.cancel_token, Duration::from_millis(100)).unwrap();


sleep_with_cancel(...).unwrap() can panic on normal cancellation (it returns Err(Error::Cancelled)), turning a user-requested cancel into a crash. Since create_tool can’t return a Result, consider either reverting this call to std::thread::sleep, or changing SifliTool::create_tool to return Result<Box<dyn SifliTool>> so cancellation can be propagated instead of panicking.

Copilot · 2026-04-23T08:07:41Z

            .unwrap();
        port.write_request_to_send(false).unwrap();
-        std::thread::sleep(Duration::from_millis(100));
+        sleep_with_cancel(&base.cancel_token, Duration::from_millis(100)).unwrap();


sleep_with_cancel(...).unwrap() can panic on normal cancellation (it returns Err(Error::Cancelled)), turning a user-requested cancel into a crash. Since create_tool can’t return a Result, consider either reverting this call to std::thread::sleep, or changing SifliTool::create_tool to return Result<Box<dyn SifliTool>> so cancellation can be propagated instead of panicking.

Copilot · 2026-04-23T08:07:41Z

+            match self.port.read(&mut buf[offset..]) {
+                Ok(0) => {
+                    if last_activity.elapsed() > timeout {
+                        return Err(Error::timeout(format!("waiting for {}", context)));
+                    }
+                }
+                Ok(n) => {
+                    offset += n;
+                    last_activity = Instant::now();
+                }
+                Err(error)
+                    if matches!(error.kind(), ErrorKind::TimedOut | ErrorKind::WouldBlock) =>
+                {
+                    if last_activity.elapsed() > timeout {
+                        return Err(Error::timeout(format!("waiting for {}", context)));
+                    }
+                }


In read_exact_with_timeout, the Ok(0) and TimedOut/WouldBlock branches immediately loop without any backoff/yield. If the underlying port returns quickly (e.g., very short timeouts), this can busy-spin and consume CPU while waiting. Consider sleeping for a small IDLE_BACKOFF (or similar) in these branches before continuing.

Copilot · 2026-04-23T08:07:42Z

+            let mut byte = [0u8; 1];
+            match self.port.read(&mut byte) {
+                Ok(0) => continue,
+                Ok(_) => {
+                    buffer.push(byte[0]);
+                    if buffer.len() > MAX_CAPTURE_BUFFER {
+                        let drain_len = buffer.len() - MAX_CAPTURE_BUFFER;
+                        buffer.drain(..drain_len);
+                    }
+                    window.push_back(byte[0]);
+                    if window.len() > max_len {
+                        window.pop_front();
+                    }
+
+                    for (index, pattern) in patterns.iter().enumerate() {
+                        if window.len() >= pattern.len()
+                            && window
+                                .iter()
+                                .rev()
+                                .take(pattern.len())
+                                .rev()
+                                .copied()
+                                .eq(pattern.iter().copied())
+                        {
+                            return Ok(PatternMatch { index, buffer });
+                        }
+                    }
+                }
+                Err(error)
+                    if matches!(error.kind(), ErrorKind::TimedOut | ErrorKind::WouldBlock) =>
+                {
+                    continue;
+                }


wait_for_patterns loops on Ok(0) and TimedOut/WouldBlock by immediately continuing, which can also cause a tight busy-loop while waiting for data. Adding a small sleep/yield (e.g., IDLE_BACKOFF) in those branches would reduce CPU usage without affecting correctness.

Copilot · 2026-04-23T08:07:42Z

            .unwrap();
        port.write_request_to_send(false).unwrap();
-        std::thread::sleep(Duration::from_millis(100));
+        sleep_with_cancel(&base.cancel_token, Duration::from_millis(100)).unwrap();


sleep_with_cancel(...).unwrap() can panic on normal cancellation (it returns Err(Error::Cancelled)), turning a user-requested cancel into a crash. Since create_tool can’t return a Result, consider either reverting this call to std::thread::sleep, or changing SifliTool::create_tool to return Result<Box<dyn SifliTool>> so cancellation can be propagated instead of panicking.

Copilot · 2026-04-23T08:07:42Z

            .unwrap();
        port.write_request_to_send(false).unwrap();
-        std::thread::sleep(Duration::from_millis(100));
+        sleep_with_cancel(&base.cancel_token, Duration::from_millis(100)).unwrap();


sleep_with_cancel(...).unwrap() can panic on normal cancellation (it returns Err(Error::Cancelled)), turning a user-requested cancel into a crash. Since create_tool can’t return a Result, consider either reverting this call to std::thread::sleep, or changing SifliTool::create_tool to return Result<Box<dyn SifliTool>> so cancellation can be propagated instead of panicking.

HalfSweet added 3 commits April 23, 2026 12:49

feat(sftool-lib): add cooperative cancellation for serial operations

cdec15a

refactor(sftool-lib): centralize cancellable serial io

a8a872b

style(sftool-lib): apply formatter output

d99623f

HalfSweet requested a review from Copilot April 23, 2026 07:49

Copilot started reviewing on behalf of HalfSweet April 23, 2026 07:50 View session

fix(sftool-lib): satisfy clippy lints

c368418

Copilot AI reviewed Apr 23, 2026

View reviewed changes

fix(sftool-lib): address reviewer feedback on serial waits

fff8ba1

HalfSweet requested a review from Copilot April 23, 2026 08:03

Copilot started reviewing on behalf of HalfSweet April 23, 2026 08:04 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

fix(sftool-lib): address serial io review follow-ups

0f9377c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sftool-lib): centralize cancellable serial io#52

refactor(sftool-lib): centralize cancellable serial io#52
HalfSweet wants to merge 6 commits intoOpenSiFli:masterfrom
HalfSweet:feat/ad-cooperative

HalfSweet commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

HalfSweet commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants