From 188828775e83cbebb7a558843eae5eb50e1dc2b8 Mon Sep 17 00:00:00 2001
From: Shaw Drastin <168159404+showier-drastic@users.noreply.github.com>
Date: Tue, 4 Feb 2025 10:53:45 +0800
Subject: [PATCH 1/2] SpiDevice cancel safety: always set CS pin to high on
 drop

If a transfer is dropped, the CS will stay in a low state, which
seems to be unsafe.
---
 embassy-embedded-hal/Cargo.toml                   |  1 +
 embassy-embedded-hal/src/shared_bus/asynch/spi.rs | 15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/embassy-embedded-hal/Cargo.toml b/embassy-embedded-hal/Cargo.toml
index 9dd2e419f..f385963f1 100644
--- a/embassy-embedded-hal/Cargo.toml
+++ b/embassy-embedded-hal/Cargo.toml
@@ -22,6 +22,7 @@ time = ["dep:embassy-time"]
 default = ["time"]
 
 [dependencies]
+embassy-hal-internal = { version = "0.2.0", path = "../embassy-hal-internal" }
 embassy-futures = { version = "0.1.0", path = "../embassy-futures" }
 embassy-sync = { version = "0.6.2", path = "../embassy-sync" }
 embassy-time = { version = "0.4.0", path = "../embassy-time", optional = true }
diff --git a/embassy-embedded-hal/src/shared_bus/asynch/spi.rs b/embassy-embedded-hal/src/shared_bus/asynch/spi.rs
index 30d4ecc36..0ba1033f6 100644
--- a/embassy-embedded-hal/src/shared_bus/asynch/spi.rs
+++ b/embassy-embedded-hal/src/shared_bus/asynch/spi.rs
@@ -25,6 +25,7 @@
 //! let display2 = ST7735::new(spi_dev2, dc2, rst2, Default::default(), 160, 128);
 //! ```
 
+use embassy_hal_internal::drop::OnDrop;
 use embassy_sync::blocking_mutex::raw::RawMutex;
 use embassy_sync::mutex::Mutex;
 use embedded_hal_1::digital::OutputPin;
@@ -70,6 +71,10 @@ where
         let mut bus = self.bus.lock().await;
         self.cs.set_low().map_err(SpiDeviceError::Cs)?;
 
+        let cs_drop = OnDrop::new(|| {
+            let _ = self.cs.set_high();
+        });
+
         let op_res = 'ops: {
             for op in operations {
                 let res = match op {
@@ -97,11 +102,10 @@ where
 
         // On failure, it's important to still flush and deassert CS.
         let flush_res = bus.flush().await;
-        let cs_res = self.cs.set_high();
+        drop(cs_drop);
 
         let op_res = op_res.map_err(SpiDeviceError::Spi)?;
         flush_res.map_err(SpiDeviceError::Spi)?;
-        cs_res.map_err(SpiDeviceError::Cs)?;
 
         Ok(op_res)
     }
@@ -155,6 +159,10 @@ where
         bus.set_config(&self.config).map_err(|_| SpiDeviceError::Config)?;
         self.cs.set_low().map_err(SpiDeviceError::Cs)?;
 
+        let cs_drop = OnDrop::new(|| {
+            let _ = self.cs.set_high();
+        });
+
         let op_res = 'ops: {
             for op in operations {
                 let res = match op {
@@ -182,11 +190,10 @@ where
 
         // On failure, it's important to still flush and deassert CS.
         let flush_res = bus.flush().await;
-        let cs_res = self.cs.set_high();
+        drop(cs_drop);
 
         let op_res = op_res.map_err(SpiDeviceError::Spi)?;
         flush_res.map_err(SpiDeviceError::Spi)?;
-        cs_res.map_err(SpiDeviceError::Cs)?;
 
         Ok(op_res)
     }

From dc2ce92e32469eb99ab0ef703b7d265ea7184630 Mon Sep 17 00:00:00 2001
From: Shaw Drastin <168159404+showier-drastic@users.noreply.github.com>
Date: Fri, 7 Feb 2025 10:39:08 +0800
Subject: [PATCH 2/2] Add proper error handling if CS is not dropped

---
 .../src/shared_bus/asynch/spi.rs                 | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/embassy-embedded-hal/src/shared_bus/asynch/spi.rs b/embassy-embedded-hal/src/shared_bus/asynch/spi.rs
index 0ba1033f6..78709b7d3 100644
--- a/embassy-embedded-hal/src/shared_bus/asynch/spi.rs
+++ b/embassy-embedded-hal/src/shared_bus/asynch/spi.rs
@@ -72,6 +72,10 @@ where
         self.cs.set_low().map_err(SpiDeviceError::Cs)?;
 
         let cs_drop = OnDrop::new(|| {
+            // This drop guard deasserts CS pin if the async operation is cancelled.
+            // Errors are ignored in this drop handler, as there's nothing we can do about them.
+            // If the async operation is completed without cancellation, this handler will not
+            // be run, and the CS pin will be deasserted with proper error handling.
             let _ = self.cs.set_high();
         });
 
@@ -102,10 +106,15 @@ where
 
         // On failure, it's important to still flush and deassert CS.
         let flush_res = bus.flush().await;
-        drop(cs_drop);
+
+        // Now that all the async operations are done, we defuse the CS guard,
+        // and manually set the CS pin low (to better handle the possible errors).
+        cs_drop.defuse();
+        let cs_res = self.cs.set_high();
 
         let op_res = op_res.map_err(SpiDeviceError::Spi)?;
         flush_res.map_err(SpiDeviceError::Spi)?;
+        cs_res.map_err(SpiDeviceError::Cs)?;
 
         Ok(op_res)
     }
@@ -160,6 +169,7 @@ where
         self.cs.set_low().map_err(SpiDeviceError::Cs)?;
 
         let cs_drop = OnDrop::new(|| {
+            // Please see comment in SpiDevice for an explanation of this drop handler.
             let _ = self.cs.set_high();
         });
 
@@ -190,10 +200,12 @@ where
 
         // On failure, it's important to still flush and deassert CS.
         let flush_res = bus.flush().await;
-        drop(cs_drop);
+        cs_drop.defuse();
+        let cs_res = self.cs.set_high();
 
         let op_res = op_res.map_err(SpiDeviceError::Spi)?;
         flush_res.map_err(SpiDeviceError::Spi)?;
+        cs_res.map_err(SpiDeviceError::Cs)?;
 
         Ok(op_res)
     }