Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace MAX5970 status/faults/max current/max voltage on V12 rails #1560

Merged
merged 5 commits into from
Nov 6, 2023
Merged

trace MAX5970 status/faults/max current/max voltage on V12 rails #1560

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
167f2fe
trace max5970 status/faults/max current/max voltage
bcantrill Nov 6, 2023
49cf89c
clippy
bcantrill Nov 6, 2023
d1c29ee
dial back ring buffer size a tad
bcantrill Nov 6, 2023
7bc618d
code review feedback from Matt + Keith
bcantrill Nov 6, 2023
dfed13f
Merge branch 'master' into trace-max5970
bcantrill Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 126 additions & 65 deletions drv/i2c-devices/src/max5970.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,56 @@ pub enum Register {
cubf_ba_chx_i = 0x47,
}

struct MonRange(u8);
bcantrill marked this conversation as resolved.
Show resolved Hide resolved

impl MonRange {
fn full_scale_voltage(&self, rail: u8) -> u8 {
let range = if rail == 0 {
self.0 & 0b11
} else {
(self.0 >> 2) & 0b11
};

match range {
0b00 => 16,
0b01 => 8,
0b10 => 4,
0b11 => 2,
_ => unreachable!(),
}
}
}

struct Status2(u8);

impl Status2 {
fn max_current_sense_range(&self, rail: u8) -> Option<u8> {
//
// The datasheet is enragingly inconsistent about how it refers to the
// channels. For most registers that have different settings for
// channels, it refers to them as Channel 1 and Channel 2 -- except
// for status2, which refers to Channel 0 and Channel 1.
//
let range = if rail == 0 {
self.0 & 0b11
} else {
(self.0 >> 2) & 0b11
};

//
// Our maximum current-sense range is 25mV, 50mV, or 100mV. (Contrary
// to the implication of the datasheet, there is no fourth maximum
// current-sense range.)
//
match range {
0b00 => Some(100),
0b01 => Some(50),
0b10 => Some(25),
_ => None,
}
}
}

pub struct Max5970 {
device: I2cDevice,
rail: u8,
Expand All @@ -240,6 +290,78 @@ impl Max5970 {
pub fn i2c_device(&self) -> &I2cDevice {
&self.device
}

fn convert_volts(&self, mon_range: MonRange, msb: u8, lsb: u8) -> Volts {
//
// The 10-bit value from the ADC is a fraction of the full-scale
// voltage setting.
//
let divisor = 1024.0 / mon_range.full_scale_voltage(self.rail) as f32;

Volts(((((msb as u16) << 2) | (lsb as u16)) as f32) / divisor)
}

fn convert_current(
&self,
status2: Status2,
msb: u8,
lsb: u8,
) -> Result<Amperes, ResponseCode> {
let millivolts = status2
.max_current_sense_range(self.rail)
.ok_or(ResponseCode::BadDeviceState)?;

//
// The 10-bit value from the ADC is a fraction of the maximum
// current-sense range.
//
let divisor = 1024.0 / millivolts as f32;
let delta = ((((msb as u16) << 2) | (lsb as u16)) as f32) / divisor;

//
// We have the voltage drop across the current sense resistor; to
// determine current, we divide voltage by resistance (I = V / R).
//
Ok(Amperes(delta / self.rsense as f32))
}

pub fn max_vout(&self) -> Result<Volts, ResponseCode> {
let (msb, lsb) = if self.rail == 0 {
(
self.read_reg(Register::max_chx_mon_msb_ch1)?,
self.read_reg(Register::max_chx_mon_lsb_ch1)?,
)
} else {
(
self.read_reg(Register::max_chx_mon_msb_ch2)?,
self.read_reg(Register::max_chx_mon_lsb_ch2)?,
)
};
bcantrill marked this conversation as resolved.
Show resolved Hide resolved

let mon_range = MonRange(self.read_reg(Register::mon_range)?);
Ok(self.convert_volts(mon_range, msb, lsb))
}

pub fn max_iout(&self) -> Result<Amperes, ResponseCode> {
let (msb, lsb) = if self.rail == 0 {
(
self.read_reg(Register::max_chx_cs_msb_ch1)?,
self.read_reg(Register::max_chx_cs_lsb_ch1)?,
)
} else {
(
self.read_reg(Register::max_chx_cs_msb_ch2)?,
self.read_reg(Register::max_chx_cs_lsb_ch2)?,
)
};
bcantrill marked this conversation as resolved.
Show resolved Hide resolved

let status2 = Status2(self.read_reg(Register::status2)?);
self.convert_current(status2, msb, lsb)
}

pub fn status0(&self) -> Result<u8, ResponseCode> {
self.read_reg(Register::status0)
}
}

impl Validate<ResponseCode> for Max5970 {
Expand All @@ -264,31 +386,8 @@ impl VoltageSensor<ResponseCode> for Max5970 {
)
};

let mon_range = self.read_reg(Register::mon_range)?;

let range = if self.rail == 0 {
mon_range & 0b11
} else {
(mon_range >> 2) & 0b11
};

let volts = match range {
0b00 => 16,
0b01 => 8,
0b10 => 4,
0b11 => 2,
_ => unreachable!(),
};

//
// The 10-bit value from the ADC is a fraction of the full-scale
// voltage setting.
//
let divisor = 1024.0 / volts as f32;

Ok(Volts(
((((msb as u16) << 2) | (lsb as u16)) as f32) / divisor,
))
let mon_range = MonRange(self.read_reg(Register::mon_range)?);
Ok(self.convert_volts(mon_range, msb, lsb))
}
}

Expand All @@ -306,45 +405,7 @@ impl CurrentSensor<ResponseCode> for Max5970 {
)
};

let status2 = self.read_reg(Register::status2)?;

//
// The datasheet is enragingly inconsistent about how it refers to the
// channels. For most registers that have different settings for
// channels, it refers to them as Channel 1 and Channel 2 -- except
// for status2, which refers to Channel 0 and Channel 1.
//
let range = if self.rail == 0 {
status2 & 0b11
} else {
(status2 >> 2) & 0b11
};

//
// Our maximum current-sense range is 25mV, 50mV, or 100mV. (Contrary
// to the implication of the datasheet, there is no fourth maximum
// current-sense range.)
//
let millivolts = match range {
0b00 => 100,
0b01 => 50,
0b10 => 25,
_ => {
return Err(ResponseCode::BadDeviceState);
}
};

//
// The 10-bit value from the ADC is a fraction of the maximum
// current-sense range.
//
let divisor = 1024.0 / millivolts as f32;
let delta = ((((msb as u16) << 2) | (lsb as u16)) as f32) / divisor;

//
// We have the voltage drop across the current sense resistor; to
// determine current, we divide voltage by resistance (I = V / R).
//
Ok(Amperes(delta / self.rsense as f32))
let status2 = Status2(self.read_reg(Register::status2)?);
self.convert_current(status2, msb, lsb)
}
}
62 changes: 62 additions & 0 deletions task/power/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ use drv_i2c_devices::{
enum Trace {
GotVersion(u32),
GotAddr(u32),
Max5970 {
sensor: SensorId,
status0: u8,
status3: u8,
fault0: u8,
fault1: u8,
fault2: u8,
max_current: f32,
max_voltage: f32,
},
None,
}

Expand Down Expand Up @@ -349,6 +359,45 @@ macro_rules! max5970_controller {
};
}

fn trace_max5970(dev: &Max5970, sensor: SensorId) {
if let Ok(Volts(volts)) = dev.max_vout() {
use drv_i2c_devices::max5970::Register;

if volts < 12.0 {
bcantrill marked this conversation as resolved.
Show resolved Hide resolved
return;
}

ringbuf_entry!(Trace::Max5970 {
bcantrill marked this conversation as resolved.
Show resolved Hide resolved
sensor,
status0: match dev.read_reg(Register::status0) {
Ok(reg) => reg,
_ => return,
},
status3: match dev.read_reg(Register::status3) {
Ok(reg) => reg,
_ => return,
},
fault0: match dev.read_reg(Register::fault0) {
Ok(reg) => reg,
_ => return,
},
fault1: match dev.read_reg(Register::fault1) {
Ok(reg) => reg,
_ => return,
},
fault2: match dev.read_reg(Register::fault2) {
Ok(reg) => reg,
_ => return,
},
max_current: match dev.max_iout() {
Ok(Amperes(amps)) => amps,
_ => return,
},
max_voltage: volts,
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote to explicitly log errors here, since they could be interesting. Something like

Suggested change
ringbuf_entry!(Trace::Max5970 {
sensor,
status0: match dev.read_reg(Register::status0) {
Ok(reg) => reg,
_ => return,
},
status3: match dev.read_reg(Register::status3) {
Ok(reg) => reg,
_ => return,
},
fault0: match dev.read_reg(Register::fault0) {
Ok(reg) => reg,
_ => return,
},
fault1: match dev.read_reg(Register::fault1) {
Ok(reg) => reg,
_ => return,
},
fault2: match dev.read_reg(Register::fault2) {
Ok(reg) => reg,
_ => return,
},
max_current: match dev.max_iout() {
Ok(Amperes(amps)) => amps,
_ => return,
},
max_voltage: volts,
});
// Helper function to read a register or log the failure
let f = |reg| match dev.read_reg(reg) {
Ok(reg) => Some(reg),
Err(err) => {
ringbuf_entry!(Trace::Max5970ReadFailed { reg, err });
None
}
};
let Some(status0) = f(Register::status0) else { return; };
let Some(status3) = f(Register::status3) else { return; };
let Some(fault0) = f(Register::fault0) else { return; };
let Some(fault1) = f(Register::fault1) else { return; };
let Some(fault2) = f(Register::fault2) else { return; };
let max_current = match dev.max_iout() {
Ok(Amperes(amps)) => amps,
Err(err) => {
ringbuf_entry!(Trace::Max5970MaxCurrentFailed { err });
return;
}
};
ringbuf_entry!(Trace::Max5970 {
sensor,
status0,
status3,
fault0,
fault1,
fault2,
max_current,
max_voltage: volts,
});

along with two new ringbuf variants:

diff --git a/task/power/src/main.rs b/task/power/src/main.rs
index cd5b0623..93a77268 100644
--- a/task/power/src/main.rs
+++ b/task/power/src/main.rs
@@ -48,6 +48,13 @@ enum Trace {
         max_current: f32,
         max_voltage: f32,
     },
+    Max5970ReadFailed {
+        reg: drv_i2c_devices::max5970::Register,
+        err: ResponseCode,
+    },
+    Max5970MaxCurrentFailed {
+        err: ResponseCode,
+    },
     None,
 }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about this because I don't want to flood the ring buffer with a failed device and miss the PGs on others. (We will be able to deduce failures by the absence of expected devices.)

}
}

#[allow(unused_macros)]
macro_rules! mwocp68_controller {
($which:ident, $rail:ident, $state:ident) => {
Expand Down Expand Up @@ -412,6 +461,7 @@ fn main() -> ! {
i2c_task,
sensor: sensor_api::Sensor::from(SENSOR.get_task_id()),
devices: claim_devices(i2c_task),
fired: 0,
};
let mut buffer = [0; idl::INCOMING_SIZE];

Expand All @@ -428,6 +478,7 @@ struct ServerImpl {
i2c_task: TaskId,
sensor: sensor_api::Sensor,
devices: &'static mut [Device; bsp::CONTROLLER_CONFIG_LEN],
fired: u32,
}

impl ServerImpl {
Expand Down Expand Up @@ -500,7 +551,18 @@ impl ServerImpl {
}
}
}

//
// Every 10 seconds, gather max5970 data
//
if self.fired % 10 == 0 {
if let Device::Max5970(dev) = dev {
trace_max5970(dev, c.current);
}
}
}

self.fired += 1;
}

/// Find the BMR491 and return an `I2cDevice` handle
Expand Down