-
Notifications
You must be signed in to change notification settings - Fork 235
Refactor of the thermal task
#2609
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
base: master
Are you sure you want to change the base?
Changes from 30 commits
3123a58
ded5468
c1919e6
45e7f1e
e3fb65d
a87ed16
823a7dc
4d2fdbd
29418d2
dc7a597
49f959d
952d1bf
d959df3
a05d9fb
36f5ac8
befe410
e1e8bef
9abcdfe
eac9093
b2de0ab
8102d44
96e716c
908f0c6
2658936
c1c18f4
d765077
e06c5b0
35ccf1a
9df220f
e9891b5
5c803a7
a297744
2547038
6b78b8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| //! Common types and helpers for Emc2305 Fan Controller | ||
|
|
||
| use drv_i2c_api::{I2cDevice, ResponseCode}; | ||
| use drv_i2c_devices::emc2305::Emc2305; | ||
| use ringbuf::ringbuf_entry; | ||
| use task_sensor_api::SensorId; | ||
| use task_thermal_api::{SensorReadError, ThermalError}; | ||
|
|
||
| use crate::{ | ||
| __RINGBUF, Trace, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, the I've also seen code |
||
| control::{Fan, FanStatus}, | ||
| }; | ||
|
|
||
| /// Tracks whether a Emc2305 fan controller has been initialized, and | ||
| /// initializes it on demand when accessed, if necessary. | ||
| /// | ||
| /// This is copy-pasted from [`Max31790`] | ||
|
jamesmunns marked this conversation as resolved.
Outdated
|
||
| pub(crate) struct Emc2305State { | ||
| emc2305: Emc2305, | ||
| fan_count: u8, | ||
| initialized: bool, | ||
| } | ||
|
|
||
| impl Emc2305State { | ||
| #[allow(dead_code)] | ||
| pub(crate) fn new(dev: &I2cDevice, fan_count: u8) -> Self { | ||
| let mut this = Self { | ||
| emc2305: Emc2305::new(dev), | ||
| fan_count, | ||
| initialized: false, | ||
| }; | ||
| retry_init(|| this.initialize().map(|_| ())); | ||
| this | ||
| } | ||
|
|
||
| /// Access the fan controller, attempting to initialize it if it has not yet | ||
| /// been initialized. | ||
| #[inline] | ||
| #[allow(dead_code)] | ||
| pub(crate) fn try_initialize( | ||
| &mut self, | ||
| ) -> Result<&mut Emc2305, ControllerInitError> { | ||
| if self.initialized { | ||
| return Ok(&mut self.emc2305); | ||
| } | ||
|
|
||
| self.initialize() | ||
| } | ||
|
|
||
| // Slow path that actually performs initialization. This is "outlined" so | ||
| // that we can avoid pushing a stack frame in the case where we just need to | ||
| // check a bool and return a pointer. | ||
| #[inline(never)] | ||
| fn initialize(&mut self) -> Result<&mut Emc2305, ControllerInitError> { | ||
| self.emc2305.initialize(self.fan_count).map_err(|e| { | ||
| ringbuf_entry!(Trace::FanControllerInitError(e)); | ||
| ControllerInitError(e) | ||
| })?; | ||
|
|
||
| self.initialized = true; | ||
| ringbuf_entry!(Trace::FanControllerInitialized); | ||
| Ok(&mut self.emc2305) | ||
| } | ||
|
|
||
| pub(crate) fn read_fan_rpms( | ||
| &mut self, | ||
| fans: &mut [Fan<drv_i2c_devices::emc2305::Fan>], | ||
| ) -> impl Iterator<Item = FanStatus> { | ||
| // Try to initialize the fan controller once at the start of the loop | ||
| let mut fctrl = self.try_initialize().map_err(SensorReadError::from); | ||
|
|
||
| // TODO: Maybe there's a way to make this a method on Fan that we can | ||
| // call, kind of like InputStatus? | ||
| fans.iter_mut().map(move |f| { | ||
| let sensor_id = f.rpm_sensor_id; | ||
| if !f.is_present { | ||
| return FanStatus::NotPresent { sensor_id }; | ||
| } | ||
|
|
||
| // If initialization failed, then we short circuit to return that | ||
| // original error, copied for each fan we're going to report. | ||
| let fctrl = fctrl.as_mut().map_err(|e| *e); | ||
|
|
||
| // If it was a success, attempt to read the RPMs, and either report | ||
| // that success or that error for each fan rpm. | ||
| let res = fctrl.and_then(|fc| { | ||
| fc.fan_rpm(f.bsp_data).map_err(SensorReadError::I2cError) | ||
| }); | ||
| match res { | ||
| Ok(rpm) => FanStatus::PresentSuccess { rpm, sensor_id }, | ||
| Err(error) => FanStatus::PresentError { error, sensor_id }, | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Helper function to retry initialization several times, logging errors | ||
| pub(crate) fn retry_init<F: FnMut() -> Result<(), ControllerInitError>>( | ||
| mut init: F, | ||
| ) { | ||
| // When we first start up, try to initialize the fan controller a few | ||
| // times, in case there's a transient I2C error. | ||
|
Comment on lines
+103
to
+104
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still the case? I see this was originally added for grapefruit as part of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure! I haven't actually tested this on hardware yet, and tried as best as possible to preserve the existing behavior, whatever it was.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also worth noting, thermal is only active for grapefruit-ruby, and I'm not sure where to even obtain one of those for testing. |
||
| for remaining in (0..3).rev() { | ||
| if init().is_ok() { | ||
| break; | ||
| } | ||
| ringbuf_entry!(Trace::FanControllerInitRetry { remaining }); | ||
| } | ||
| } | ||
|
|
||
| pub(crate) struct ControllerInitError(pub(crate) ResponseCode); | ||
|
|
||
| impl From<ControllerInitError> for ThermalError { | ||
| fn from(_: ControllerInitError) -> Self { | ||
| ThermalError::FanControllerUninitialized | ||
| } | ||
| } | ||
|
|
||
| impl From<ControllerInitError> for SensorReadError { | ||
| fn from(ControllerInitError(code): ControllerInitError) -> Self { | ||
| SensorReadError::I2cError(code) | ||
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub(crate) const fn make_consecutive_nonremovable_fans<const N: usize>( | ||
| sensors: &'static [SensorId; N], | ||
| ) -> [crate::control::Fan<drv_i2c_devices::emc2305::Fan>; N] { | ||
| const ONE: crate::control::Fan<drv_i2c_devices::emc2305::Fan> = | ||
| crate::control::Fan::new( | ||
| SensorId::new(0), | ||
| drv_i2c_devices::emc2305::Fan::new_const(0), | ||
| ); | ||
|
|
||
| let mut out = [ONE; N]; | ||
| let mut idx = 0; | ||
| while idx < N { | ||
| out[idx] = crate::control::Fan::new( | ||
| sensors[idx], | ||
| drv_i2c_devices::emc2305::Fan::new_const(idx as u8), | ||
| ); | ||
| out[idx].is_present = true; | ||
| idx += 1; | ||
| } | ||
|
|
||
| out | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the extra potential runtime panic but it also didn't seem to increase the code size so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a const-fn, and is only currently used to generate constants, which means that any panics that occur would be compile-time errors.
You are right, if someone inadvertently calls it at runtime, it would potentially panic in that case.