Merge #371

371: rj/pounder fixes r=jordens a=jordens

- pounder: simplify attenuator spi interface
- deps: use mcp23017 release
- attenuators: use robust latching sequence
- rf_power: fix measurement
- pounder io extender: hack around some bug
- pounder: fix attenuator indices (latch and shiftreg)
- pounder: enum for gpio ext pins
- ad9959: refactor pad()

One big issue was the apparently broken mcp23017 `digital_write`. I couldn't get it to work.
For now, and since (a) we aren't handling the other pins in any configurable way, and (b) the readback-style modification is slow let's just to whole-port writes of constants.

Co-authored-by: Robert Jördens <rj@quartiq.de>

Cargo.lock

@@ -404,8 +404,9 @@ dependencies = [
 
 [[package]]
 name = "mcp23017"
-version = "0.1.1"
+version = "1.0.0"
-source = "git+https://github.com/lucazulian/mcp23017.git?rev=523d71d#523d71dcb11fc0ea4bd9385ef2527ae7a7eee987"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2c32fd6627e73f1cfa95c00ddcdcb5a6a6ddbd10b308d08588a502c018b6e12c"
 dependencies = [
  "embedded-hal",
 ]

Cargo.toml

@@ -45,17 +45,13 @@ ad9959 = { path = "ad9959" }
 miniconf = "0.1.0"
 shared-bus = {version = "0.2.2", features = ["cortex-m"] }
 serde-json-core = "0.4"
+mcp23017 = "1.0"
 
 # rtt-target bump
 [dependencies.rtt-logger]
 git = "https://github.com/quartiq/rtt-logger.git"
 rev = "70b0eb5"
 
-# rewrite
-[dependencies.mcp23017]
-git = "https://github.com/lucazulian/mcp23017.git"
-rev = "523d71d"
-
 [dependencies.stm32h7xx-hal]
 features = ["stm32h743v", "rt", "unproven", "ethernet", "quadspi"]
 version = "0.9.0"

ad9959/src/lib.rs

@@ -43,6 +43,7 @@ pub enum Mode {
 
 /// The configuration registers within the AD9959 DDS device. The values of each register are
 /// equivalent to the address.
+#[allow(clippy::upper_case_acronyms)]
 pub enum Register {
     CSR = 0x00,
     FR1 = 0x01,
@@ -596,6 +597,22 @@ impl ProfileSerializer {
         self.index += value.len() + 1;
     }
 
+    fn pad(&mut self) {
+        // Pad the buffer to 32-bit (4 byte) alignment by adding dummy writes to CSR and LSRR.
+        match self.index & 3 {
+            3 => {
+                // For a level of 3, we have to pad with 5 bytes to align things.
+                self.add_write(Register::CSR, &[(self.mode as u8) << 1]);
+                self.add_write(Register::LSRR, &[0, 0]);
+            }
+            2 => self.add_write(Register::CSR, &[(self.mode as u8) << 1]),
+            1 => self.add_write(Register::LSRR, &[0, 0]),
+            0 => {}
+
+            _ => unreachable!(),
+        }
+    }
+
     /// Get the serialized profile as a slice of 32-bit words.
     ///
     /// # Note
@@ -604,21 +621,8 @@ impl ProfileSerializer {
     ///
     /// # Returns
     /// A slice of `u32` words representing the serialized profile.
-    pub fn finalize<'a>(&'a mut self) -> &[u32] {
+    pub fn finalize<'a>(&'a mut self) -> &'a [u32] {
-        // Pad the buffer to 32-bit alignment by adding dummy writes to CSR and LSRR.
+        self.pad();
-        let padding = 4 - (self.index % 4);
-        match padding {
-            1 => {
-                // For a pad size of 1, we have to pad with 5 bytes to align things.
-                self.add_write(Register::CSR, &[(self.mode as u8) << 1]);
-                self.add_write(Register::LSRR, &[0, 0]);
-            }
-            2 => self.add_write(Register::CSR, &[(self.mode as u8) << 1]),
-            3 => self.add_write(Register::LSRR, &[0, 0]),
-            4 => {}
-
-            _ => unreachable!(),
-        }
         unsafe {
             core::slice::from_raw_parts::<'a, u32>(
                 &self.data as *const _ as *const u32,

src/hardware/configuration.rs

@@ -802,7 +802,7 @@ pub fn setup(
             let scl = gpiob.pb8.into_alternate_af4().set_open_drain();
             let i2c1 = device.I2C1.i2c(
                 (scl, sda),
-                100.khz(),
+                400.khz(),
                 ccdr.peripheral.I2C1,
                 &ccdr.clocks,
             );

src/hardware/pounder/attenuators.rs

@@ -30,16 +30,16 @@ pub trait AttenuatorInterface {
         // Read all the channels, modify the channel of interest, and write all the channels back.
         // This ensures the staging register and the output register are always in sync.
         let mut channels = [0_u8; 4];
-        self.read_all_attenuators(&mut channels)?;
+        self.transfer_attenuators(&mut channels)?;
 
         // The lowest 2 bits of the 8-bit shift register on the attenuator are ignored. Shift the
         // attenuator code into the upper 6 bits of the register value. Note that the attenuator
         // treats inputs as active-low, so the code is inverted before writing.
-        channels[channel as usize] = (!attenuation_code) << 2;
+        channels[channel as usize] = !(attenuation_code << 2);
-        self.write_all_attenuators(&channels)?;
+        self.transfer_attenuators(&mut channels)?;
 
         // Finally, latch the output of the updated channel to force it into an active state.
-        self.latch_attenuators(channel)?;
+        self.latch_attenuator(channel)?;
 
         Ok(attenuation_code as f32 / 2.0)
     }
@@ -57,8 +57,8 @@ pub trait AttenuatorInterface {
         // Reading the data always shifts data out of the staging registers, so we perform a
         // duplicate write-back to ensure the staging register is always equal to the output
         // register.
-        self.read_all_attenuators(&mut channels)?;
+        self.transfer_attenuators(&mut channels)?;
-        self.write_all_attenuators(&channels)?;
+        self.transfer_attenuators(&mut channels)?;
 
         // The attenuation code is stored in the upper 6 bits of the register, where each LSB
         // represents 0.5 dB. The attenuator stores the code as active-low, so inverting the result
@@ -74,13 +74,10 @@ pub trait AttenuatorInterface {
 
     fn reset_attenuators(&mut self) -> Result<(), Error>;
 
-    fn latch_attenuators(&mut self, channel: Channel) -> Result<(), Error>;
+    fn latch_attenuator(&mut self, channel: Channel) -> Result<(), Error>;
-    fn read_all_attenuators(
+
+    fn transfer_attenuators(
         &mut self,
         channels: &mut [u8; 4],
     ) -> Result<(), Error>;
-    fn write_all_attenuators(
-        &mut self,
-        channels: &[u8; 4],
-    ) -> Result<(), Error>;
 }

src/hardware/pounder/mod.rs

@@ -17,14 +17,21 @@ use rf_power::PowerMeasurementInterface;
 
 use embedded_hal::{adc::OneShot, blocking::spi::Transfer};
 
-const EXT_CLK_SEL_PIN: u8 = 8 + 7;
+pub enum GpioPin {
-#[allow(dead_code)]
+    Led4Green = 0,
-const OSC_EN_N_PIN: u8 = 8 + 6;
+    Led5Red = 1,
-const ATT_RST_N_PIN: u8 = 8 + 5;
+    Led6Green = 2,
-const ATT_LE3_PIN: u8 = 8 + 3;
+    Led7Red = 3,
-const ATT_LE2_PIN: u8 = 8 + 2;
+    Led8Green = 4,
-const ATT_LE1_PIN: u8 = 8 + 1;
+    Led9Red = 5,
-const ATT_LE0_PIN: u8 = 8;
+    AttLe0 = 8,
+    AttLe1 = 8 + 1,
+    AttLe2 = 8 + 2,
+    AttLe3 = 8 + 3,
+    AttRstN = 8 + 5,
+    OscEnN = 8 + 6,
+    ExtClkSel = 8 + 7,
+}
 
 #[derive(Debug, Copy, Clone)]
 pub enum Error {
@@ -37,13 +44,15 @@ pub enum Error {
     Adc,
 }
 
+/// The numerical value (discriminant) of the Channel enum is the index in the attenuator shift
+/// register as well as the attenuator latch enable signal index on the GPIO extender.
 #[derive(Debug, Copy, Clone)]
 #[allow(dead_code)]
 pub enum Channel {
-    In0,
+    In0 = 0,
-    In1,
+    Out0 = 1,
-    Out0,
+    In1 = 2,
-    Out1,
+    Out1 = 3,
 }
 
 #[derive(Serialize, Deserialize, Copy, Clone, Debug)]
@@ -80,14 +89,14 @@ pub struct DdsClockConfig {
     pub external_clock: bool,
 }
 
-impl Into<ad9959::Channel> for Channel {
+impl From<Channel> for ad9959::Channel {
     /// Translate pounder channels to DDS output channels.
-    fn into(self) -> ad9959::Channel {
+    fn from(other: Channel) -> Self {
-        match self {
+        match other {
-            Channel::In0 => ad9959::Channel::Two,
+            Channel::In0 => Self::Two,
-            Channel::In1 => ad9959::Channel::Four,
+            Channel::In1 => Self::Four,
-            Channel::Out0 => ad9959::Channel::One,
+            Channel::Out0 => Self::One,
-            Channel::Out1 => ad9959::Channel::Three,
+            Channel::Out1 => Self::Three,
         }
     }
 }
@@ -300,25 +309,20 @@ impl PounderDevices {
             adc2_in_p,
         };
 
-        // Configure power-on-default state for pounder. All LEDs are on, on-board oscillator
+        // Configure power-on-default state for pounder. All LEDs are off, on-board oscillator
-        // selected, attenuators out of reset. Note that testing indicates the output state needs to
+        // selected and enabled, attenuators out of reset. Note that testing indicates the
-        // be set first to properly update the output registers.
+        // output state needs to be set first to properly update the output registers.
         devices
             .mcp23017
             .all_pin_mode(mcp23017::PinMode::OUTPUT)
             .map_err(|_| Error::I2c)?;
         devices
             .mcp23017
-            .write_gpio(mcp23017::Port::GPIOA, 0x3F)
+            .write_gpio(mcp23017::Port::GPIOA, 0x00)
             .map_err(|_| Error::I2c)?;
         devices
             .mcp23017
-            .write_gpio(mcp23017::Port::GPIOB, 1 << 5)
+            .write_gpio(mcp23017::Port::GPIOB, 0x2F)
-            .map_err(|_| Error::I2c)?;
-
-        devices
-            .mcp23017
-            .digital_write(EXT_CLK_SEL_PIN, false)
             .map_err(|_| Error::I2c)?;
 
         Ok(devices)
@@ -329,12 +333,11 @@ impl AttenuatorInterface for PounderDevices {
     /// Reset all of the attenuators to a power-on default state.
     fn reset_attenuators(&mut self) -> Result<(), Error> {
         self.mcp23017
-            .digital_write(ATT_RST_N_PIN, false)
+            .write_gpio(mcp23017::Port::GPIOB, 0x0f)
             .map_err(|_| Error::I2c)?;
-        // TODO: Measure the I2C transaction speed to the RST pin to ensure that the delay is
+        // Duration of one I2C transaction is sufficiently long.
-        // sufficient. Document the delay here.
         self.mcp23017
-            .digital_write(ATT_RST_N_PIN, true)
+            .write_gpio(mcp23017::Port::GPIOB, 0x2f)
             .map_err(|_| Error::I2c)?;
 
         Ok(())
@@ -344,31 +347,24 @@ impl AttenuatorInterface for PounderDevices {
     ///
     /// Args:
     /// * `channel` - The attenuator channel to latch.
-    fn latch_attenuators(&mut self, channel: Channel) -> Result<(), Error> {
+    fn latch_attenuator(&mut self, channel: Channel) -> Result<(), Error> {
-        let pin = match channel {
+        let pin = channel as u8;
-            Channel::In0 => ATT_LE0_PIN,
-            Channel::In1 => ATT_LE2_PIN,
-            Channel::Out0 => ATT_LE1_PIN,
-            Channel::Out1 => ATT_LE3_PIN,
-        };
-
         self.mcp23017
-            .digital_write(pin, true)
+            .write_gpio(mcp23017::Port::GPIOB, 0x2f & !(1 << pin))
             .map_err(|_| Error::I2c)?;
-        // TODO: Measure the I2C transaction speed to the RST pin to ensure that the delay is
+        // Duration of one I2C transaction is sufficiently long.
-        // sufficient. Document the delay here.
         self.mcp23017
-            .digital_write(pin, false)
+            .write_gpio(mcp23017::Port::GPIOB, 0x2f)
             .map_err(|_| Error::I2c)?;
-
         Ok(())
     }
 
     /// Read the raw attenuation codes stored in the attenuator shift registers.
     ///
     /// Args:
-    /// * `channels` - A slice to store the channel readings into.
+    /// * `channels` - A 4 byte slice to be shifted into the
-    fn read_all_attenuators(
+    ///     attenuators and to contain the data shifted out.
+    fn transfer_attenuators(
         &mut self,
         channels: &mut [u8; 4],
     ) -> Result<(), Error> {
@@ -378,23 +374,6 @@ impl AttenuatorInterface for PounderDevices {
 
         Ok(())
     }
-
-    /// Write the attenuator shift registers.
-    ///
-    /// Args:
-    /// * `channels` - The data to write into the attenuators.
-    fn write_all_attenuators(
-        &mut self,
-        channels: &[u8; 4],
-    ) -> Result<(), Error> {
-        let mut result = [0_u8; 4];
-        result.clone_from_slice(channels);
-        self.attenuator_spi
-            .transfer(&mut result)
-            .map_err(|_| Error::Spi)?;
-
-        Ok(())
-    }
 }
 
 impl PowerMeasurementInterface for PounderDevices {

src/hardware/pounder/rf_power.rs

@@ -1,20 +1,22 @@
 use super::{Channel, Error};
 
-/// Provide an interface to measure RF input power in dB.
+/// Provide an interface to measure RF input power in dBm.
 pub trait PowerMeasurementInterface {
     fn sample_converter(&mut self, channel: Channel) -> Result<f32, Error>;
 
     /// Measure the power of an input channel in dBm.
     ///
-    /// Note: This function assumes the input channel is connected to an AD8363 output.
-    ///
     /// Args:
-    /// * `channel` - The pounder channel to measure the power of in dBm.
+    /// * `channel` - The pounder input channel to measure the power of.
+    ///
+    /// Returns:
+    /// Power in dBm after the digitally controlled attenuator before the amplifier.
     fn measure_power(&mut self, channel: Channel) -> Result<f32, Error> {
         let analog_measurement = self.sample_converter(channel)?;
 
-        // The AD8363 with VSET connected to VOUT provides an output voltage of 51.7mV / dB at
+        // The AD8363 with VSET connected to VOUT provides an output voltage of 51.7 mV/dB at
-        // 100MHz. It also indicates  a y-intercept of -58dBm.
+        // 100MHz with an intercept of -58 dBm.
-        Ok(analog_measurement / 0.0517 - 58.0)
+        // It is placed behind a 20 dB tap.
+        Ok(analog_measurement * (1. / 0.0517) + (-58. + 20.))
     }
 }