Repository: probe-rs/probe-rs
Pull request #2388 (2024-04-19 12:57:03)

Slightly clean up the Xtensa code
This PR removes direct debug module control from `XtensaCommunicationInterface`. While this is a breaking change, these functions aren't necessary and even shouldn't be used anyway, but they were left over from the initial development pass.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index c7b74d0f..be382ad5 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1884,7 +1884,7 @@ dependencies = [
  "futures-util",
  "http 0.2.12",
  "hyper 0.14.28",
- "rustls 0.21.10",
+ "rustls 0.21.11",
  "tokio",
  "tokio-rustls 0.24.1",
 ]
@@ -3160,7 +3160,7 @@ dependencies = [
  "once_cell",
  "percent-encoding",
  "pin-project-lite",
- "rustls 0.21.10",
+ "rustls 0.21.11",
  "rustls-native-certs",
  "rustls-pemfile 1.0.4",
  "serde",
@@ -3350,9 +3350,9 @@ dependencies = [
 
 [[package]]
 name = "rustls"
-version = "0.21.10"
+version = "0.21.11"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f9d5a6813c0759e4609cd494e8e725babae6a2ca7b62a5536a13daaec6fcb7ba"
+checksum = "7fecbfb7b1444f477b345853b1fce097a2c6fb637b2bfb87e6bc5db0f043fae4"
 dependencies = [
  "log",
  "ring",
@@ -3812,6 +3812,7 @@ dependencies = [
  "miette",
  "pretty_env_logger",
  "probe-rs",
+ "probe-rs-target",
  "serde",
  "thiserror",
  "toml",
@@ -4240,7 +4241,7 @@ version = "0.24.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "c28327cf380ac148141087fbfb9de9d7bd4e84ab5d2c28fbc911d753de8a7081"
 dependencies = [
- "rustls 0.21.10",
+ "rustls 0.21.11",
  "tokio",
 ]
 
diff --git a/probe-rs/src/architecture/xtensa/communication_interface.rs b/probe-rs/src/architecture/xtensa/communication_interface.rs
index 3fab36bf..573a769d 100644
--- a/probe-rs/src/architecture/xtensa/communication_interface.rs
+++ b/probe-rs/src/architecture/xtensa/communication_interface.rs
@@ -153,27 +153,6 @@ impl XtensaCommunicationInterface {
         self.hw_breakpoint_num
     }
 
-    /// Enters OCD mode and halts the core.
-    pub fn enter_ocd_mode(&mut self) -> Result<(), XtensaError> {
-        self.xdm.halt()?;
-        tracing::info!("Entered OCD mode");
-        Ok(())
-    }
-
-    /// Returns whether the core is in OCD mode.
-    pub fn is_in_ocd_mode(&mut self) -> Result<bool, XtensaError> {
-        self.xdm.is_in_ocd_mode()
-    }
-
-    /// Leaves OCD mode, restores state and resumes program execution.
-    pub fn leave_ocd_mode(&mut self) -> Result<(), XtensaError> {
-        self.restore_registers()?;
-        self.resume()?;
-        self.xdm.leave_ocd_mode()?;
-        tracing::info!("Left OCD mode");
-        Ok(())
-    }
-
     /// Halts the core.
     pub fn halt(&mut self) -> Result<(), XtensaError> {
         tracing::debug!("Halting core");
@@ -183,7 +162,7 @@ impl XtensaCommunicationInterface {
     /// Returns whether the core is halted.
     pub fn is_halted(&mut self) -> Result<bool, XtensaError> {
         if !self.state.is_halted {
-            self.state.is_halted = self.xdm.is_halted()?;
+            self.state.is_halted = self.xdm.status()?.stopped();
         }
 
         Ok(self.state.is_halted)
@@ -429,7 +408,7 @@ impl XtensaCommunicationInterface {
     }
 
     #[tracing::instrument(skip(self))]
-    fn restore_registers(&mut self) -> Result<(), XtensaError> {
+    pub(super) fn restore_registers(&mut self) -> Result<(), XtensaError> {
         tracing::debug!("Restoring registers");
 
         // Clone the list of saved registers so we can iterate over it, but code may still save
diff --git a/probe-rs/src/architecture/xtensa/mod.rs b/probe-rs/src/architecture/xtensa/mod.rs
index bfe7705a..07411adc 100644
--- a/probe-rs/src/architecture/xtensa/mod.rs
+++ b/probe-rs/src/architecture/xtensa/mod.rs
@@ -462,7 +462,10 @@ impl<'probe> CoreInterface for Xtensa<'probe> {
     }
 
     fn debug_core_stop(&mut self) -> Result<(), Error> {
-        self.interface.leave_ocd_mode()?;
+        self.interface.restore_registers()?;
+        self.interface.resume()?;
+        self.interface.xdm.leave_ocd_mode()?;
+        tracing::info!("Left OCD mode");
         Ok(())
     }
 }
diff --git a/probe-rs/src/architecture/xtensa/xdm.rs b/probe-rs/src/architecture/xtensa/xdm.rs
index ca7f1919..6a56328c 100644
--- a/probe-rs/src/architecture/xtensa/xdm.rs
+++ b/probe-rs/src/architecture/xtensa/xdm.rs
@@ -411,7 +411,7 @@ impl Xdm {
         self.execute()
     }
 
-    fn status(&mut self) -> Result<DebugStatus, XtensaError> {
+    pub(super) fn status(&mut self) -> Result<DebugStatus, XtensaError> {
         self.read_nexus_register::<DebugStatus>()
     }
 
@@ -450,11 +450,6 @@ impl Xdm {
         });
     }
 
-    pub(super) fn is_in_ocd_mode(&mut self) -> Result<bool, XtensaError> {
-        let reg = self.read_nexus_register::<DebugControlSet>()?;
-        Ok(reg.0.enable_ocd())
-    }
-
     pub(super) fn leave_ocd_mode(&mut self) -> Result<(), XtensaError> {
         // clear all clearable status bits
         self.write_nexus_register({
@@ -489,10 +484,6 @@ impl Xdm {
         Ok(())
     }
 
-    pub(super) fn is_halted(&mut self) -> Result<bool, XtensaError> {
-        self.status().map(|status| status.stopped())
-    }
-
     pub(super) fn resume(&mut self) -> Result<(), XtensaError> {
         tracing::debug!("resuming...");
         // Clear pending interrupts first that would re-enter us into the Stopped state
diff --git a/probe-rs/src/config/sequences/atsam.rs b/probe-rs/src/config/sequences/atsam.rs
index 43f21186..120f5998 100644
--- a/probe-rs/src/config/sequences/atsam.rs
+++ b/probe-rs/src/config/sequences/atsam.rs
@@ -5,17 +5,23 @@ use crate::{
         self,
         arm::{
             ap::MemoryAp,
+            armv7m::Dhcsr,
+            communication_interface::{DapProbe, SwdSequence},
             memory::adi_v5_memory_interface::ArmProbe,
             sequences::{ArmDebugSequence, ArmDebugSequenceError, DebugEraseSequence},
-            ApAddress, ArmError, ArmProbeInterface, DpAddress,
+            ApAddress, ArmError, ArmProbeInterface, DpAddress, Pins,
         },
     },
     probe::DebugProbeError,
     session::MissingPermissions,
-    Permissions,
+    MemoryMappedRegister, Permissions,
 };
 use bitfield::bitfield;
-use std::sync::Arc;
+use probe_rs_target::CoreType;
+use std::{
+    sync::Arc,
+    time::{Duration, Instant},
+};
 
 use anyhow::Result;
 
@@ -187,17 +193,15 @@ impl DsuStatusB {
 }
 
 /// A wrapper for different types that can perform SWD Commands (SWJ_Pins SWJ_Sequence)
-struct SwdSequenceShim<'a>(&'a mut dyn architecture::arm::communication_interface::DapProbe);
+struct SwdSequenceShim<'a>(&'a mut dyn DapProbe);
 
-impl<'a> From<&'a mut dyn architecture::arm::communication_interface::DapProbe>
-    for SwdSequenceShim<'a>
-{
-    fn from(p: &'a mut dyn architecture::arm::communication_interface::DapProbe) -> Self {
+impl<'a> From<&'a mut dyn DapProbe> for SwdSequenceShim<'a> {
+    fn from(p: &'a mut dyn DapProbe) -> Self {
         Self(p)
     }
 }
 
-impl<'a> architecture::arm::communication_interface::SwdSequence for SwdSequenceShim<'a> {
+impl<'a> SwdSequence for SwdSequenceShim<'a> {
     fn swj_sequence(&mut self, bit_len: u8, bits: u64) -> Result<(), DebugProbeError> {
         self.0.swj_sequence(bit_len, bits)
     }
@@ -262,8 +266,8 @@ impl AtSAM {
         tracing::info!("Chip-Erase started...");
 
         // Wait for it to finish
-        let start = std::time::Instant::now();
-        while start.elapsed() < std::time::Duration::from_secs(8) {
+        let start = Instant::now();
+        while start.elapsed() < Duration::from_secs(8) {
             let current_dsu_statusa = DsuStatusA::from(memory.read_word_8(DsuStatusA::ADDRESS)?);
             if current_dsu_statusa.done() {
                 tracing::info!("Chip-Erase complete");
@@ -284,7 +288,7 @@ impl AtSAM {
             } else if current_dsu_statusa.fail() {
                 return Err(ArmError::ChipEraseFailed);
             }
-            std::thread::sleep(std::time::Duration::from_millis(250));
+            std::thread::sleep(Duration::from_millis(250));
         }
 
         tracing::error!("Chip-Erase failed to complete within 8 seconds");
@@ -302,24 +306,23 @@ impl AtSAM {
     /// Subject to probe communication errors
     pub fn reset_hardware_with_extension(
         &self,
-        interface: &mut dyn architecture::arm::communication_interface::SwdSequence,
+        interface: &mut dyn SwdSequence,
     ) -> Result<(), ArmError> {
-        let mut pins = architecture::arm::Pins(0);
+        let mut pins = Pins(0);
         pins.set_nreset(true);
         pins.set_swdio_tms(true);
         pins.set_swclk_tck(true);
 
-        let mut pin_values = architecture::arm::Pins(0);
-
         // First set nReset, SWDIO and SWCLK to low.
-        let _ = interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
-        std::thread::sleep(std::time::Duration::from_millis(10));
+        let mut pin_values = Pins(0);
+        interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
+        std::thread::sleep(Duration::from_millis(10));
 
         // Next release nReset, but keep SWDIO and SWCLK low. This should put the device into
-        // reset extension ()
+        // reset extension.
         pin_values.set_nreset(true);
-        let _ = interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
-        std::thread::sleep(std::time::Duration::from_millis(20));
+        interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
+        std::thread::sleep(Duration::from_millis(20));
 
         Ok(())
     }
@@ -331,13 +334,19 @@ impl AtSAM {
     /// # Errors
     /// Subject to probe communication errors
     pub fn release_reset_extension(&self, memory: &mut dyn ArmProbe) -> Result<(), ArmError> {
+        // Enable debug mode if it is not already enabled
+        let mut dhcsr = Dhcsr(0);
+        dhcsr.enable_write();
+        dhcsr.set_c_debugen(true);
+        memory.write_word_32(Dhcsr::ADDRESS_OFFSET, dhcsr.0)?;
+
         // clear the reset extension bit
         let mut dsu_statusa = DsuStatusA(0);
         dsu_statusa.set_crstext(true);
         memory.write_8(DsuStatusA::ADDRESS, &[dsu_statusa.0])?;
 
-        let start = std::time::Instant::now();
-        while start.elapsed() < std::time::Duration::from_millis(100) {
+        let start = Instant::now();
+        while start.elapsed() < Duration::from_millis(100) {
             let current_dsu_statusa = DsuStatusA::from(memory.read_word_8(DsuStatusA::ADDRESS)?);
             if !current_dsu_statusa.crstext() {
                 return Ok(());
@@ -351,25 +360,21 @@ impl AtSAM {
     ///
     /// # Errors
     /// Subject to probe communication errors
-    pub fn reset_hardware(
-        &self,
-        interface: &mut dyn architecture::arm::communication_interface::SwdSequence,
-    ) -> Result<(), ArmError> {
-        let mut pins = architecture::arm::Pins(0);
+    pub fn reset_hardware(&self, interface: &mut dyn SwdSequence) -> Result<(), ArmError> {
+        let mut pins = Pins(0);
         pins.set_nreset(true);
         pins.set_swdio_tms(true);
         pins.set_swclk_tck(true);
 
         let mut pin_values = pins;
-        pin_values.set_nreset(false);
 
-        let _ = interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
-        std::thread::sleep(std::time::Duration::from_millis(10));
+        pin_values.set_nreset(false);
+        interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
+        std::thread::sleep(Duration::from_millis(10));
 
         pin_values.set_nreset(true);
-
-        let _ = interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
-        std::thread::sleep(std::time::Duration::from_millis(10));
+        interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
+        std::thread::sleep(Duration::from_millis(10));
 
         Ok(())
     }
@@ -379,7 +384,7 @@ impl AtSAM {
     fn ensure_target_reset(
         &self,
         result: Result<(), ArmError>,
-        interface: &mut dyn architecture::arm::communication_interface::SwdSequence,
+        interface: &mut dyn SwdSequence,
     ) -> Result<(), ArmError> {
         // Fall back if the probe's swj_pins does not support the full set of pins
         match result {
@@ -388,19 +393,14 @@ impl AtSAM {
             })) => {
                 tracing::warn!("Using fallback reset method");
 
-                let mut pins = architecture::arm::Pins(0);
+                let mut pins = Pins(0);
                 pins.set_nreset(true);
 
-                let mut pin_values = pins;
-                pin_values.set_nreset(false);
-
-                let _ = interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
-                std::thread::sleep(std::time::Duration::from_millis(10));
+                interface.swj_pins(0, pins.0 as u32, 0)?;
+                std::thread::sleep(Duration::from_millis(10));
 
-                pin_values.set_nreset(true);
-
-                let _ = interface.swj_pins(pin_values.0 as u32, pins.0 as u32, 0)?;
-                std::thread::sleep(std::time::Duration::from_millis(10));
+                interface.swj_pins(pins.0 as u32, pins.0 as u32, 0)?;
+                std::thread::sleep(Duration::from_millis(10));
 
                 Ok(())
             }
@@ -410,14 +410,24 @@ impl AtSAM {
 }
 
 impl ArmDebugSequence for AtSAM {
+    fn debug_core_start(
+        &self,
+        interface: &mut dyn ArmProbeInterface,
+        core_ap: MemoryAp,
+        _core_type: CoreType,
+        _debug_base: Option<u64>,
+        _cti_base: Option<u64>,
+    ) -> Result<(), ArmError> {
+        let mut core = interface.memory_interface(core_ap)?;
+
+        self.release_reset_extension(&mut *core)
+    }
+
     /// `reset_hardware_assert` for ATSAM devices
     ///
     /// Instead of keeping `nReset` asserted, the device is instead put into CPU Reset Extension
     /// which will keep the CPU Core in reset until manually released by the debugger probe.
-    fn reset_hardware_assert(
-        &self,
-        interface: &mut dyn architecture::arm::communication_interface::DapProbe,
-    ) -> Result<(), ArmError> {
+    fn reset_hardware_assert(&self, interface: &mut dyn DapProbe) -> Result<(), ArmError> {
         let mut shim = SwdSequenceShim::from(interface);
         let result = self.reset_hardware_with_extension(&mut shim);
 
@@ -429,11 +439,10 @@ impl ArmDebugSequence for AtSAM {
     /// Instead of de-asserting `nReset` here (this was already done during the CPU Reset Extension process),
     /// the device is released from Reset Extension.
     fn reset_hardware_deassert(&self, memory: &mut dyn ArmProbe) -> Result<(), ArmError> {
-        let mut pins = architecture::arm::Pins(0);
+        let mut pins = Pins(0);
         pins.set_nreset(true);
 
-        let current_pins =
-            architecture::arm::Pins(memory.swj_pins(pins.0 as u32, pins.0 as u32, 0)? as u8);
+        let current_pins = Pins(memory.swj_pins(pins.0 as u32, pins.0 as u32, 0)? as u8);
         if !current_pins.nreset() {
             return Err(ArmDebugSequenceError::SequenceSpecific(
                 "Expected nReset to already be de-asserted".into(),
diff --git a/probe-rs/src/probe/jlink/interface.rs b/probe-rs/src/probe/jlink/interface.rs
index 3d2b3462..87fafaa7 100644
--- a/probe-rs/src/probe/jlink/interface.rs
+++ b/probe-rs/src/probe/jlink/interface.rs
@@ -9,24 +9,22 @@ enum_and_set!(
     #[non_exhaustive]
     #[derive(Debug, Copy, Clone, PartialEq, Eq)]
     pub enum Interface {
-        /// JTAG interface (IEEE 1149.1). Supported by most J-Link probes (some embedded J-Links might
-        /// only support SWD).
+        /// JTAG interface (IEEE 1149.1). Supported by most J-Link probes (some embedded J-Links
+        /// might only support SWD).
         Jtag = 0,
-        /// SWD interface (Serial Wire Debug), used by most Cortex-M chips, and supported by almost all
-        /// J-Link probes.
+        /// SWD interface (Serial Wire Debug), used by most Cortex-M chips, and supported by almost
+        /// all J-Link probes.
         Swd = 1,
         /// Background Debug Mode 3, a single-wire debug interface used on some NXP microcontrollers.
         Bdm3 = 2,
         /// FINE, a two-wire debugging interface used by Renesas RX MCUs.
         ///
-        /// **Note**: due to a bug, attempting to select FINE with [`JLink::select_inferface`] will
+        /// **Note**: due to a bug, attempting to select FINE with `JLink::select_interface()` will
         /// currently hang the probe.
-        ///
-        /// [`JLink::select_inferface`]: crate::JLink::select_interface
         // FIXME: There's a curious bug that hangs the probe when selecting the FINE interface.
-        // Specifically, the probe never sends back the previous interface after it receives the `c7 03`
-        // SELECT_IF cmd, even though the normal J-Link software also just sends `c7 03` and gets back
-        // the right response.
+        // Specifically, the probe never sends back the previous interface after it receives the
+        // `c7 03` SELECT_IF cmd, even though the normal J-Link software also just sends `c7 03`
+        // and gets back the right response.
         Fine = 3,
         /// In-Circuit System Programming (ICSP) interface of PIC32 chips.
         Pic32Icsp = 4,


Test patch:
diff --git a/.github/workflows/smoketest.yml b/.github/workflows/smoketest.yml
index 6190acff..271ace3c 100644
--- a/.github/workflows/smoketest.yml
+++ b/.github/workflows/smoketest.yml
@@ -40,7 +40,10 @@ jobs:
           path: target/aarch64-unknown-linux-gnu/release/smoke_tester
 
   test:
-    runs-on: ["self-hosted", "linux", "ARM64", "smoke-test"]
+    runs-on: ["self-hosted", "linux", "ARM64", "${{ matrix.runner }}"]
+    strategy:
+      matrix:
+        runner: ["smoke-test", "smoke-tester-2"]
 
     needs: build
 
diff --git a/smoke-tester/Cargo.toml b/smoke-tester/Cargo.toml
index 25a81d35..14a854c4 100644
--- a/smoke-tester/Cargo.toml
+++ b/smoke-tester/Cargo.toml
@@ -12,6 +12,7 @@ publish = false
 
 serde = { version = "1", features = ["derive"] }
 probe-rs = { workspace = true }
+probe-rs-target = { workspace = true }
 toml = "0.8.12"
 pretty_env_logger = "0.5.0"
 log = "0.4.21"
diff --git a/smoke-tester/src/tests.rs b/smoke-tester/src/tests.rs
index 549a0abb..668d18f4 100644
--- a/smoke-tester/src/tests.rs
+++ b/smoke-tester/src/tests.rs
@@ -230,7 +230,11 @@ pub fn test_flashing(tracker: &TestTracker, session: &mut Session) -> Result<(),
 
     let start_time = Instant::now();
 
-    download_file_with_options(session, test_binary, Format::Elf, options)
+    let format = match session.target().default_format {
+        probe_rs_target::BinaryFormat::Idf => Format::Idf(Default::default()),
+        probe_rs_target::BinaryFormat::Raw => Default::default(),
+    };
+    download_file_with_options(session, test_binary, format, options)
         .map_err(|err| TestFailure::Error(Box::new(err)))?;
 
     println!();