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.

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

Conversation

bcantrill
Copy link
Collaborator

No description provided.

@bcantrill bcantrill marked this pull request as ready for review November 6, 2023 18:20
@bcantrill bcantrill requested a review from mkeeter November 6, 2023 18:20
task/power/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 370 to 397
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,
});
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.)

@bcantrill bcantrill changed the title trace max5970 status/faults/max current/max voltage trace MAX5970 status/faults/max current/max voltage on V12 rails Nov 6, 2023
@bcantrill bcantrill enabled auto-merge (squash) November 6, 2023 21:05
@bcantrill bcantrill merged commit 08f5a25 into master Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants