Support fan PWM settings #73

Merged
sb10q merged 16 commits from esavkin/thermostat:69-fan_pwm into master 2023-03-22 17:15:49 +08:00
2 changed files with 75 additions and 8 deletions
Showing only changes of commit ea2eb51b27 - Show all commits

View File

@ -26,9 +26,10 @@ const MAX_TEC_I: f64 = 3.0;
const MAX_FAN_PWM: f64 = 100.0;
Review

Those naive values will need proper testing and determination at some point.

Those naive values will need proper testing and determination at some point.
const MIN_FAN_PWM: f64 = 1.0;
const TACHO_MEASURE_MS: i64 = 2500;
const TACHO_LOW_THRESHOLD: u32 = 100;
const DEFAULT_K_A: f64 = 1.0;
const DEFAULT_K_B: f64 = 0.0;
const DEFAULT_K_C: f64 = 0.0;
const DEFAULT_K_C: f64 = 0.04;
Outdated
Review

Where are those default K_A/B/C from? Certainly not from the schematics so the comment is misleading.
Have you tested those defaults and how?

Where are those default K_A/B/C from? Certainly not from the schematics so the comment is misleading. Have you tested those defaults and how?

Well, they are subject of change, but they come from the power formula P=I^2 * R

Well, they are subject of change, but they come from the power formula P=I^2 * R
#[derive(Serialize, Copy, Clone)]
pub struct HWRev {
@ -36,11 +37,20 @@ pub struct HWRev {
pub minor: u8,
}
Outdated
Review

at the user's own risk

at the user's own risk
#[derive(Serialize, Clone, Copy, PartialEq)]
pub enum FanStatus {
OK,
NotAvailable,
Stalled,
LowSignal,
}
struct TachoCtrl {
tacho: TachoPin,
tacho_cnt: u32,
Outdated
Review

Did you double-check that this results in no PWM pulses being emitted at all, even short ones?

Did you double-check that this results in no PWM pulses being emitted at all, even short ones?

Fixed this and checked that there is no pulses by default. On oscilloscope there is pulse on fan 100 command, but no such pulse on reset command

Fixed this and checked that there is no pulses by default. On oscilloscope there is pulse on `fan 100` command, but no such pulse on `reset` command
tacho_value: Option<u32>,
prev_epoch: i64,
past_record: u64,
}
pub struct FanCtrl<'a> {
@ -52,6 +62,7 @@ pub struct FanCtrl<'a> {
k_b: f64,
k_c: f64,
channels: &'a mut Channels,
last_status: FanStatus
}
Review

inline

inline
impl<'a> FanCtrl<'a> {
@ -74,14 +85,22 @@ impl<'a> FanCtrl<'a> {
k_b: DEFAULT_K_B,
k_c: DEFAULT_K_C,
Review

this doesn't need to be pub

this doesn't need to be ``pub``
channels,
last_status: FanStatus::OK,
}
}
pub fn cycle(&mut self) {
pub fn cycle(&mut self) -> Result<(), FanStatus>{
if self.available {
self.tacho.cycle();
}
self.adjust_speed();
let diagnose = self.diagnose();
if diagnose != self.last_status {
self.last_status = diagnose;
Outdated
Review

Not sure if [inline] here and in several other functions is relevant.

Not sure if ``[inline]`` here and in several other functions is relevant.

These functions are short, used just in a very limited number of places. They all look to be a good candidate to be inlined

These functions are short, used just in a very limited number of places. They all look to be a good candidate to be inlined
Outdated
Review

Probably the compiler can figure this out already, and it doesn't seem to be performance-critical code so it doesn't matter if it doesn't.

Probably the compiler can figure this out already, and it doesn't seem to be performance-critical code so it doesn't matter if it doesn't.
Err(diagnose)
} else {
Ok(())
}
}
pub fn summary(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
@ -91,6 +110,7 @@ impl<'a> FanCtrl<'a> {
tacho: self.tacho.get(),
abs_max_tec_i: self.channels.current_abs_max_tec_i(),
auto_mode: self.fan_auto,
status: self.diagnose(),
k_a: self.k_a,
k_b: self.k_b,
k_c: self.k_c,
@ -139,6 +159,13 @@ impl<'a> FanCtrl<'a> {
value as f64 / (max as f64)
}
fn diagnose(&mut self) -> FanStatus {
if !self.available {
return FanStatus::NotAvailable;
Outdated
Review

Doesn't seem relevant

Doesn't seem relevant
}
self.tacho.diagnose()
}
fn get_pwm(&self) -> u32 {
let duty = self.fan.get_duty();
let max = self.fan.get_max_duty();
@ -147,16 +174,17 @@ impl<'a> FanCtrl<'a> {
}
impl TachoCtrl {
pub fn new(tacho: TachoPin) -> Self {
fn new(tacho: TachoPin) -> Self {
TachoCtrl {
tacho,
tacho_cnt: 0,
tacho_value: None,
prev_epoch: 0,
past_record: 0,
}
}
pub fn init(&mut self, exti: &mut EXTI, syscfg: &mut SysCfg) {
fn init(&mut self, exti: &mut EXTI, syscfg: &mut SysCfg) {
// These lines do not cause NVIC to run the ISR,
// since the interrupt should be unmasked in the cortex_m::peripheral::NVIC.
Outdated
Review

"since the interrupt is still masked in cortex_m::peripheral::NVIC"

"since the interrupt is still masked in cortex_m::peripheral::NVIC"
// Also using interrupt-related workaround is the best
@ -169,7 +197,17 @@ impl TachoCtrl {
self.tacho.enable_interrupt(exti);
}
pub fn cycle(&mut self) {
#[inline]
fn add_record(&mut self, value: u32) {
self.past_record = self.past_record << 2;
esavkin marked this conversation as resolved Outdated
Outdated
Review

Just use [bool; 32] or Vec<bool>

Just use ``[bool; 32]`` or ``Vec<bool>``
Outdated
Review

Or some custom enum instead of bool.

Or some custom enum instead of bool.
if value >= TACHO_LOW_THRESHOLD {
Outdated
Review

That's naive. The expected range for the tacho value obviously depends on the fan PWM setting.

That's naive. The expected range for the tacho value obviously depends on the fan PWM setting.
self.past_record += 0b11;
esavkin marked this conversation as resolved Outdated
Outdated
Review

|=

|=
} else if value > 0 && self.tacho_cnt < TACHO_LOW_THRESHOLD {
self.past_record += 0b10;
}
}
fn cycle(&mut self) {
let tacho_input = self.tacho.check_interrupt();
if tacho_input {
self.tacho.clear_interrupt_pending_bit();
@ -179,14 +217,25 @@ impl TachoCtrl {
let instant = Instant::from_millis(i64::from(timer::now()));
if instant.millis - self.prev_epoch >= TACHO_MEASURE_MS {
self.tacho_value = Some(self.tacho_cnt);
self.add_record(self.tacho_cnt);
self.tacho_cnt = 0;
self.prev_epoch = instant.millis;
}
}
pub fn get(&self) -> u32 {
fn get(&self) -> u32 {
self.tacho_value.unwrap_or(u32::MAX)
}
fn diagnose(&mut self) -> FanStatus {
if self.past_record & 0b11 == 0b11 {
FanStatus::OK
} else if self.past_record & 0xAAAAAAAAAAAAAAAA > 0 {
esavkin marked this conversation as resolved Outdated
Outdated
Review

???????

???????
FanStatus::LowSignal
} else {
FanStatus::Stalled
}
}
}
impl HWRev {
@ -212,7 +261,19 @@ pub struct FanSummary {
tacho: u32,
abs_max_tec_i: f64,
auto_mode: bool,
status: FanStatus,
k_a: f64,
k_b: f64,
k_c: f64,
}
impl FanStatus {
pub fn fmt_u8(&self) -> &'static [u8] {
match *self {
FanStatus::OK => "Fan is OK".as_bytes(),
FanStatus::NotAvailable => "Fan is not available".as_bytes(),
FanStatus::Stalled => "Fan is stalled".as_bytes(),
FanStatus::LowSignal => "Fan is low signal".as_bytes(),
esavkin marked this conversation as resolved Outdated
Outdated
Review

What does "fan is low signal" mean?

What does "fan is low signal" mean?

That there are some signals being registered, but in general it so low so that it can be zero on some small timeline. It tipically happens on very low fan speed (from 1 to 3 included), and that behavior is shown on oscilloscope.

That there are some signals being registered, but in general it so low so that it can be zero on some small timeline. It tipically happens on very low fan speed (from 1 to 3 included), and that behavior is shown on oscilloscope.
Outdated
Review

Maybe just put a minimum fan speed to remove this edge case. I doubt running the fan at a couple rotations per second (which is what I guess it spins at when this happens) has much effect on cooling anyway.

Maybe just put a minimum fan speed to remove this edge case. I doubt running the fan at a couple rotations per second (which is what I guess it spins at when this happens) has much effect on cooling anyway.
Outdated
Review

And "low signal" is very confusing and not the proper term to use, it sounds like you are receiving some kind of analog transmission being drowned in noise.
Moot but you should have said something about the tacho frequency being low.

And "low signal" is very confusing and not the proper term to use, it sounds like you are receiving some kind of analog transmission being drowned in noise. Moot but you should have said something about the tacho frequency being low.
}
}
}

View File

@ -187,10 +187,10 @@ fn main() -> ! {
if let Some(channel) = updated_channel {
server.for_each(|_, session| session.set_report_pending(channel.into()));
}
fan_ctrl.cycle();
let fan_status = fan_ctrl.cycle();
let instant = Instant::from_millis(i64::from(timer::now()));
cortex_m::interrupt::free(net::clear_pending);
server.poll(instant)
.unwrap_or_else(|e| {
@ -241,6 +241,12 @@ fn main() -> ! {
}
}
}
match fan_status {
Ok(_) => {}
Err(status) => {
send_line(&mut socket, status.fmt_u8());
}
};
}
});
} else {