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
5 changed files with 164 additions and 129 deletions
Showing only changes of commit 21fc244eac - Show all commits

View File

@ -95,7 +95,7 @@ Send commands as simple text string terminated by `\n`. Responses are
formatted as line-delimited JSON. formatted as line-delimited JSON.
| Syntax | Function | | Syntax | Function |
|----------------------------------|---------------------------------------------------------------------------| |----------------------------------|-------------------------------------------------------------------------------|
| `report` | Show current input | | `report` | Show current input |
| `report mode` | Show current report mode | | `report mode` | Show current report mode |
| `report mode <off/on>` | Set report mode | | `report mode <off/on>` | Set report mode |
@ -126,8 +126,8 @@ formatted as line-delimited JSON.
| `ipv4 <X.X.X.X/L> [Y.Y.Y.Y]` | Configure IPv4 address, netmask length, and optional default gateway | | `ipv4 <X.X.X.X/L> [Y.Y.Y.Y]` | Configure IPv4 address, netmask length, and optional default gateway |
| `fan` | Show current fan settings and sensors' measurements | | `fan` | Show current fan settings and sensors' measurements |
| `fan <value>` | Set fan power with values from 0 to 100, where 0 is auto mode | | `fan <value>` | Set fan power with values from 0 to 100, where 0 is auto mode |
| `fcurve <a> <b> <c>` | Set fan controller coefficients (see *Fan control* section) | | `fcurve <a> <b> <c>` | Set fan controller curve coefficients (see *Fan control* section) |
| `fan-restore` | Set fan controller coefficients to defaults (see *Fan control* section) | | `fcurve-restore` | Set fan controller curve coefficients to defaults (see *Fan control* section) |
## USB ## USB
@ -286,4 +286,4 @@ Please note that power doesn't correlate with the actual speed linearly.
3. `fcurve <a> <b> <c>` - set coefficients of the controlling curve `a*x^2 + b*x + c`, where `x` is `abs_max_tec_i/MAX_TEC_I`, 3. `fcurve <a> <b> <c>` - set coefficients of the controlling curve `a*x^2 + b*x + c`, where `x` is `abs_max_tec_i/MAX_TEC_I`,
Outdated
Review

Why is it "fcurve" and "fan-restore"? Don't you see the issue here?

Why is it "fcurve" and "fan-restore"? Don't you see the issue here?
i.e. receives values from 0 to 1 linearly tied to the maximum current. The controlling curve should produce values from 0 to 1, i.e. receives values from 0 to 1 linearly tied to the maximum current. The controlling curve should produce values from 0 to 1,
as below and beyond values would be substituted by 0 and 1 respectively. as below and beyond values would be substituted by 0 and 1 respectively.
Outdated
Review

Those defaults are naive.

Those defaults are naive.

Though they are naive, they work OK - 50% full speed is achieved on 1A, 90% on 2A (see the table of tacho measurements at various pwm).

user input scaled tacho
0 4 431
1 5 466
2 6 492
3 7 515
4 8 536
5 9 554
6 10 571
7 11 584
8 12 598
9 13 606
10 14 618
20 23 671
30 33 704
40 42 729
50 52 740
60 62 760
70 71 773
80 81 791
90 90 791
100 100 791
Though they are naive, they work OK - 50% full speed is achieved on 1A, 90% on 2A (see the table of tacho measurements at various pwm). | user input | scaled | tacho | | -------- | -------- | -------- | 0|4|431 1|5|466 2|6|492 3|7|515 4|8|536 5|9|554 6|10|571 7|11|584 8|12|598 9|13|606 10|14|618 20|23|671 30|33|704 40|42|729 50|52|740 60|62|760 70|71|773 80|81|791 90|90|791 100|100|791
Outdated
Review

Though they are naive, they work OK

I don't know how you can say that without testing the board inside the enclosure and with loads on both channels.

> Though they are naive, they work OK I don't know how you can say that without testing the board inside the enclosure and with loads on both channels.
4. `fan-restore` - restore fan settings to defaults: `auto = true, a = 1.0, b = 0.0, c = 0.04`. 4. `fcurve-restore` - restore fan settings to defaults: `auto = true, a = 1.0, b = 0.0, c = 0.00`.

View File

@ -342,7 +342,7 @@ impl Handler {
Ok(Handler::Reset) Ok(Handler::Reset)
} }
fn fan (socket: &mut TcpSocket, fan_pwm: Option<u32>, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> { fn fan(socket: &mut TcpSocket, fan_pwm: Option<u32>, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
match fan_pwm { match fan_pwm {
Some(val) => { Some(val) => {
fan_ctrl.set_auto_mode(val == 0); fan_ctrl.set_auto_mode(val == 0);
Outdated
Review

This behavior is inconsistent with the pwm command, the latter uses pwm i_set <current> / pwm pid, not pwm i_set 0 to enable PID.

This behavior is inconsistent with the ``pwm`` command, the latter uses ``pwm i_set <current> / pwm pid``, not ``pwm i_set 0`` to enable PID.
@ -366,43 +366,43 @@ impl Handler {
Ok(Handler::Handled) Ok(Handler::Handled)
} }
fn fan_coeff (socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl, k_a: f64, k_b: f64, k_c: f64) -> Result<Handler, Error> { fn fan_curve(socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl, k_a: f64, k_b: f64, k_c: f64) -> Result<Handler, Error> {
Outdated
Review

Remove space before ( (also in the other functions)

Remove space before ( (also in the other functions)
fan_ctrl.set_coefficients(k_a, k_b, k_c); fan_ctrl.set_curve(k_a, k_b, k_c);
send_line(socket, b"{}"); send_line(socket, b"{}");
Ok(Handler::Handled) Ok(Handler::Handled)
} }
fn fan_defaults (socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> { fn fan_defaults(socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
fan_ctrl.restore_defaults(); fan_ctrl.restore_defaults();
send_line(socket, b"{}"); send_line(socket, b"{}");
Ok(Handler::Handled) Ok(Handler::Handled)
} }
pub fn handle_command (command: Command, socket: &mut TcpSocket, channels: &mut Channels, session: &Session, leds: &mut Leds, store: &mut FlashStore, ipv4_config: &mut Ipv4Config, fan_ctrl: &mut FanCtrl) -> Result<Self, Error> { pub fn handle_command(command: Command, socket: &mut TcpSocket, session: &Session, leds: &mut Leds, store: &mut FlashStore, ipv4_config: &mut Ipv4Config, fan_ctrl: &mut FanCtrl) -> Result<Self, Error> {
match command { match command {
Command::Quit => Ok(Handler::CloseSocket), Command::Quit => Ok(Handler::CloseSocket),
Command::Reporting(_reporting) => Handler::reporting(socket), Command::Reporting(_reporting) => Handler::reporting(socket),
Command::Show(ShowCommand::Reporting) => Handler::show_report_mode(socket, session), Command::Show(ShowCommand::Reporting) => Handler::show_report_mode(socket, session),
Command::Show(ShowCommand::Input) => Handler::show_report(socket, channels), Command::Show(ShowCommand::Input) => Handler::show_report(socket, &mut fan_ctrl.channels),
Command::Show(ShowCommand::Pid) => Handler::show_pid(socket, channels), Command::Show(ShowCommand::Pid) => Handler::show_pid(socket, &mut fan_ctrl.channels),
Outdated
Review

same

same
Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, channels), Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, &mut fan_ctrl.channels),
Command::Show(ShowCommand::SteinhartHart) => Handler::show_steinhart_hart(socket, channels), Command::Show(ShowCommand::SteinhartHart) => Handler::show_steinhart_hart(socket, &mut fan_ctrl.channels),
Command::Show(ShowCommand::PostFilter) => Handler::show_post_filter(socket, channels), Command::Show(ShowCommand::PostFilter) => Handler::show_post_filter(socket, &mut fan_ctrl.channels),
Command::Show(ShowCommand::Ipv4) => Handler::show_ipv4(socket, ipv4_config), Command::Show(ShowCommand::Ipv4) => Handler::show_ipv4(socket, ipv4_config),
Command::PwmPid { channel } => Handler::engage_pid(socket, channels, leds, channel), Command::PwmPid { channel } => Handler::engage_pid(socket, &mut fan_ctrl.channels, leds, channel),
Command::Pwm { channel, pin, value } => Handler::set_pwm(socket, channels, leds, channel, pin, value), Command::Pwm { channel, pin, value } => Handler::set_pwm(socket, &mut fan_ctrl.channels, leds, channel, pin, value),
Command::CenterPoint { channel, center } => Handler::set_center_point(socket, channels, channel, center), Command::CenterPoint { channel, center } => Handler::set_center_point(socket, &mut fan_ctrl.channels, channel, center),
Command::Pid { channel, parameter, value } => Handler::set_pid(socket, channels, channel, parameter, value), Command::Pid { channel, parameter, value } => Handler::set_pid(socket, &mut fan_ctrl.channels, channel, parameter, value),
Command::SteinhartHart { channel, parameter, value } => Handler::set_steinhart_hart(socket, channels, channel, parameter, value), Command::SteinhartHart { channel, parameter, value } => Handler::set_steinhart_hart(socket, &mut fan_ctrl.channels, channel, parameter, value),
Command::PostFilter { channel, rate: None } => Handler::reset_post_filter(socket, channels, channel), Command::PostFilter { channel, rate: None } => Handler::reset_post_filter(socket, &mut fan_ctrl.channels, channel),
Command::PostFilter { channel, rate: Some(rate) } => Handler::set_post_filter(socket, channels, channel, rate), Command::PostFilter { channel, rate: Some(rate) } => Handler::set_post_filter(socket, &mut fan_ctrl.channels, channel, rate),
Command::Load { channel } => Handler::load_channel(socket, channels, store, channel), Command::Load { channel } => Handler::load_channel(socket, &mut fan_ctrl.channels, store, channel),
Command::Save { channel } => Handler::save_channel(socket, channels, channel, store), Command::Save { channel } => Handler::save_channel(socket, &mut fan_ctrl.channels, channel, store),
Command::Ipv4(config) => Handler::set_ipv4(socket, store, config), Command::Ipv4(config) => Handler::set_ipv4(socket, store, config),
Command::Reset => Handler::reset(channels), Command::Reset => Handler::reset(&mut fan_ctrl.channels),
Command::Dfu => Handler::dfu(channels), Command::Dfu => Handler::dfu(&mut fan_ctrl.channels),
Command::Fan {fan_pwm} => Handler::fan(socket, fan_pwm, fan_ctrl), Command::Fan {fan_pwm} => Handler::fan(socket, fan_pwm, fan_ctrl),
Command::FanCoeff { k_a, k_b, k_c } => Handler::fan_coeff(socket, fan_ctrl, k_a, k_b, k_c), Command::FanCurve { k_a, k_b, k_c } => Handler::fan_curve(socket, fan_ctrl, k_a, k_b, k_c),
Command::FanDefaults => Handler::fan_defaults(socket, fan_ctrl), Command::FanDefaults => Handler::fan_defaults(socket, fan_ctrl),
} }
} }

View File

@ -182,7 +182,7 @@ pub enum Command {
Fan { Fan {
fan_pwm: Option<u32> fan_pwm: Option<u32>
}, },
FanCoeff { FanCurve {
k_a: f64, k_a: f64,
k_b: f64, k_b: f64,
k_c: f64, k_c: f64,
@ -546,9 +546,9 @@ fn fan(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
Ok((input, result)) Ok((input, result))
} }
fn fan_coeff(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> { fn fan_curve(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
let (input, _) = tag("fcurve")(input)?; let (input, _) = tag("fcurve")(input)?;
let (input, coeffs) = alt(( let (input, curve) = alt((
|input| { |input| {
let (input, _) = whitespace(input)?; let (input, _) = whitespace(input)?;
let (input, k_a) = float(input)?; let (input, k_a) = float(input)?;
@ -566,8 +566,8 @@ fn fan_coeff(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
value(None, end) value(None, end)
))(input)?; ))(input)?;
let result = match coeffs { let result = match curve {
Some(coeffs) => Ok(Command::FanCoeff { k_a: coeffs.0, k_b: coeffs.1, k_c: coeffs.2 }), Some(curve) => Ok(Command::FanCurve { k_a: curve.0, k_b: curve.1, k_c: curve.2 }),
Outdated
Review

Call it "curve" or "coeff" but not both.

Call it "curve" or "coeff" but not both.
None => Err(Error::ParseFloat) None => Err(Error::ParseFloat)
}; };
Ok((input, result)) Ok((input, result))
@ -586,9 +586,9 @@ fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
steinhart_hart, steinhart_hart,
postfilter, postfilter,
value(Ok(Command::Dfu), tag("dfu")), value(Ok(Command::Dfu), tag("dfu")),
value(Ok(Command::FanDefaults), tag("fan-restore")), value(Ok(Command::FanDefaults), tag("fcurve-restore")),
fan, fan,
fan_coeff, fan_curve,
))(input) ))(input)
} }

View File

@ -1,4 +1,3 @@
use core::{cmp::max_by};
use serde::Serialize; use serde::Serialize;
use stm32f4xx_hal::{ use stm32f4xx_hal::{
pwm::{self, PwmChannels}, pwm::{self, PwmChannels},
@ -21,15 +20,33 @@ use crate::{
pub type FanPin = PwmChannels<TIM8, pwm::C4>; pub type FanPin = PwmChannels<TIM8, pwm::C4>;
Outdated
Review

"weak" suggests amplitude, which is incorrect.
-> "frequency is too low"

"weak" suggests amplitude, which is incorrect. -> "frequency is too low"

no, not the frequency. The near-zero freq is the consequence of weak signal.

no, not the frequency. The near-zero freq is the consequence of weak signal.
Outdated
Review

Do you have an oscilloscope picture demonstrating this, and how was it measured?

Do you have an oscilloscope picture demonstrating this, and how was it measured?

We used pulse counter to measure the tacho values, for PWM 0.04-1.0 it was ok, but for lower the tacho signal disappears

We used pulse counter to measure the tacho values, for PWM 0.04-1.0 it was ok, but for lower the tacho signal disappears
pub type TachoPin = PC8<Input<Floating>>; pub type TachoPin = PC8<Input<Floating>>;
Outdated
Review

According to ?

According to ?

Internal experiments

Internal experiments
Outdated
Review

Please say so in the code comments.

Please say so in the code comments.
Outdated
Review

Also we can probably expect this to be hardware revision dependent. When the fan has proper PWM support this shouldn't be an issue I guess.

Also we can probably expect this to be hardware revision dependent. When the fan has proper PWM support this shouldn't be an issue I guess.

Made this (and others) value coming from hwrev

Made this (and others) value coming from hwrev
const MAX_TEC_I: f64 = 3.0;
// as stated in the schematics // as stated in the schematics
const MAX_FAN_PWM: f64 = 100.0; const MAX_TEC_I: f64 = 3.0;
const MIN_FAN_PWM: f64 = 1.0;
const MAX_USER_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_USER_FAN_PWM: f64 = 1.0;
const MAX_FAN_PWM: f64 = 1.0;
// below this value, motor pulse signal is too weak to be registered by tachometer
const MIN_FAN_PWM: f64 = 0.05;
const TACHO_MEASURE_MS: i64 = 2500; const TACHO_MEASURE_MS: i64 = 2500;
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
const TACHO_LOW_THRESHOLD: u32 = 100; // by default up to 2 cycles are skipped on changes in PWM output,
// and the halt threshold will help detect the failure during these skipped cycles
const TACHO_HALT_THRESHOLD: u32 = 250;
const TACHO_SKIP_CYCLES: u8 = 2;
const DEFAULT_K_A: f64 = 1.0; const DEFAULT_K_A: f64 = 1.0;
Outdated
Review

at the user's own risk

at the user's own risk
const DEFAULT_K_B: f64 = 0.0; const DEFAULT_K_B: f64 = 0.0;
const DEFAULT_K_C: f64 = 0.04; const DEFAULT_K_C: f64 = 0.0;
// This regression is from 6% to 25% lower than values registered in the experiments.
// Actual values would be better estimated by logarithmic regression, but that would require more
// runtime computation, and wouldn't give significant correlation difference
// (0.996 for log and 0.966 for quadratic regression).
const TACHO_REGRESSION_A: f64 = -0.04135128436;
const TACHO_REGRESSION_B: f64 = 6.23015531;
const TACHO_REGRESSION_C: f64 = 403.6833577;
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
#[derive(Serialize, Copy, Clone)] #[derive(Serialize, Copy, Clone)]
pub struct HWRev { pub struct HWRev {
@ -41,8 +58,8 @@ pub struct HWRev {
pub enum FanStatus { pub enum FanStatus {
Outdated
Review

at user's own risk

at user's own risk
OK, OK,
NotAvailable, NotAvailable,
Stalled, TooSlow,
LowSignal, Halted
} }
struct TachoCtrl { struct TachoCtrl {
@ -50,10 +67,9 @@ struct TachoCtrl {
tacho_cnt: u32, tacho_cnt: u32,
tacho_value: Option<u32>, tacho_value: Option<u32>,
prev_epoch: i64, prev_epoch: i64,
past_record: u64,
} }
pub struct FanCtrl<'a> { pub struct FanCtrl {
fan: FanPin, fan: FanPin,
tacho: TachoCtrl, tacho: TachoCtrl,
fan_auto: bool, fan_auto: bool,
@ -61,12 +77,13 @@ pub struct FanCtrl<'a> {
k_a: f64, k_a: f64,
k_b: f64, k_b: f64,
k_c: f64, k_c: f64,
channels: &'a mut Channels, pub channels: Channels,
last_status: FanStatus last_status: FanStatus,
skip_cycles: u8,
} }
impl<'a> FanCtrl<'a> { impl FanCtrl {
pub fn new(mut fan: FanPin, tacho: TachoPin, channels: &'a mut Channels, exti: &mut EXTI, syscfg: &mut SysCfg) -> Self { pub fn new(mut fan: FanPin, tacho: TachoPin, channels: Channels, exti: &mut EXTI, syscfg: &mut SysCfg) -> Self {
Review

this doesn't need to be pub

this doesn't need to be ``pub``
let available = channels.hwrev.fan_available(); let available = channels.hwrev.fan_available();
let mut tacho_ctrl = TachoCtrl::new(tacho); let mut tacho_ctrl = TachoCtrl::new(tacho);
@ -85,17 +102,20 @@ impl<'a> FanCtrl<'a> {
k_b: DEFAULT_K_B, k_b: DEFAULT_K_B,
k_c: DEFAULT_K_C, k_c: DEFAULT_K_C,
channels, channels,
last_status: FanStatus::OK, last_status: if available { FanStatus::OK } else { FanStatus::NotAvailable },
skip_cycles: 0
} }
} }
pub fn cycle(&mut self) -> Result<(), FanStatus>{ pub fn cycle(&mut self) -> Result<(), FanStatus> {
if self.available { if self.available {
self.tacho.cycle(); if self.tacho.cycle() {
self.skip_cycles >>= 1;
}
} }
self.adjust_speed(); self.adjust_speed();
let diagnose = self.diagnose(); let diagnose = self.diagnose();
if diagnose != self.last_status { if (self.skip_cycles == 0 || diagnose == FanStatus::Halted) && diagnose != self.last_status {
self.last_status = diagnose; self.last_status = diagnose;
Err(diagnose) Err(diagnose)
} else { } else {
@ -126,9 +146,7 @@ impl<'a> FanCtrl<'a> {
if self.fan_auto && self.available { if self.fan_auto && self.available {
let scaled_current = self.channels.current_abs_max_tec_i() / MAX_TEC_I; let scaled_current = self.channels.current_abs_max_tec_i() / MAX_TEC_I;
// do not limit upper bound, as it will be limited in the set_pwm() // do not limit upper bound, as it will be limited in the set_pwm()
Outdated
Review

This poorly chosen name suggests that auto mode is not available at all.

Rename to fan_default_auto or similar.

This poorly chosen name suggests that auto mode is not available at all. Rename to fan_default_auto or similar.
let pwm = max_by(MAX_FAN_PWM * (scaled_current * (scaled_current * self.k_a + self.k_b) + self.k_c), let pwm = (MAX_USER_FAN_PWM * (scaled_current * (scaled_current * self.k_a + self.k_b) + self.k_c)) as u32;
MIN_FAN_PWM,
|a, b| a.partial_cmp(b).unwrap_or(core::cmp::Ordering::Equal)) as u32;
self.set_pwm(pwm); self.set_pwm(pwm);
} }
} }
@ -139,7 +157,7 @@ impl<'a> FanCtrl<'a> {
} }
#[inline] #[inline]
pub fn set_coefficients(&mut self, k_a: f64, k_b: f64, k_c: f64) { pub fn set_curve(&mut self, k_a: f64, k_b: f64, k_c: f64) {
self.k_a = k_a; self.k_a = k_a;
self.k_b = k_b; self.k_b = k_b;
self.k_c = k_c; self.k_c = k_c;
@ -148,28 +166,50 @@ impl<'a> FanCtrl<'a> {
#[inline] #[inline]
pub fn restore_defaults(&mut self) { pub fn restore_defaults(&mut self) {
self.set_auto_mode(true); self.set_auto_mode(true);
self.set_coefficients(DEFAULT_K_A, DEFAULT_K_B, DEFAULT_K_C); self.set_curve(DEFAULT_K_A, DEFAULT_K_B, DEFAULT_K_C);
} }
pub fn set_pwm(&mut self, fan_pwm: u32) -> f64 { pub fn set_pwm(&mut self, fan_pwm: u32) -> f64 {
let duty = fan_pwm as f64 / MAX_FAN_PWM; let fan_pwm = fan_pwm.min(MAX_USER_FAN_PWM as u32).max(MIN_USER_FAN_PWM as u32);
self.skip_cycles = if (self.tacho.get() as f64) <= Self::threshold_for_pwm(fan_pwm as f64) {
TACHO_SKIP_CYCLES
} else { self.skip_cycles };
let duty = Self::scale_number(fan_pwm as f64, MIN_FAN_PWM, MAX_FAN_PWM, MIN_USER_FAN_PWM, MAX_USER_FAN_PWM);
let max = self.fan.get_max_duty(); let max = self.fan.get_max_duty();
let value = ((duty * (max as f64)) as u16).min(max); let value = ((duty * (max as f64)) as u16).min(max);
self.fan.set_duty(value); self.fan.set_duty(value);
value as f64 / (max as f64) value as f64 / (max as f64)
} }
#[inline]
fn threshold_for_pwm(fan_pwm: f64) -> f64 {
(TACHO_REGRESSION_A * fan_pwm + TACHO_REGRESSION_B) * fan_pwm + TACHO_REGRESSION_C
}
#[inline]
Outdated
Review

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

"since the interrupt is still masked in cortex_m::peripheral::NVIC"
fn scale_number(unscaled: f64, to_min: f64, to_max: f64, from_min: f64, from_max: f64) -> f64 {
(to_max - to_min) * (unscaled - from_min) / (from_max - from_min) + to_min
}
Outdated
Review

Using the interrupt logic to detect edges isn't weird or unsafe, please reword this comment.

Is it correct that the actual problem is that you cannot use the pin as a simple input when another pin nearby is configured as PWM output? It's hard to guess from the wording of the comment.

Using the interrupt logic to detect edges isn't weird or unsafe, please reword this comment. Is it correct that the actual problem is that you cannot use the pin as a simple input when another pin nearby is configured as PWM output? It's hard to guess from the wording of the comment.

Yes, with current version of library, TIM8 can be used with the PWM pins only, and tieing tacho pin would move it to the Timer, and therefore couldn't be used without unsafe hacks.
I am not sure if new lib version has addressed such problem, though in C code that shouldn't be a problem.

Yes, with current version of library, TIM8 can be used with the PWM pins only, and tieing tacho pin would move it to the Timer, and therefore couldn't be used without unsafe hacks. I am not sure if new lib version has addressed such problem, though in C code that shouldn't be a problem.
Outdated
Review

I am not sure if new lib version has addressed such problem

Not very high priority but please find out, and for now make it clear in the comments where the problem comes from and what potential solutions are. This would be useful later when you or someone else tries to e.g. update the library, then it could be a good time to remove the interrupt hack. Saying things are "weird" or "unsafe" is not helpful.

> I am not sure if new lib version has addressed such problem Not very high priority but please find out, and for now make it clear in the comments where the problem comes from and what potential solutions are. This would be useful later when you or someone else tries to e.g. update the library, then it could be a good time to remove the interrupt hack. Saying things are "weird" or "unsafe" is not helpful.
fn diagnose(&mut self) -> FanStatus { fn diagnose(&mut self) -> FanStatus {
Outdated
Review

Rempve this last line "Also such hacks wouldn't guarantee it to be more precise."

Rempve this last line "Also such hacks wouldn't guarantee it to be more precise."
if !self.available { if !self.available {
return FanStatus::NotAvailable; return FanStatus::NotAvailable;
} }
self.tacho.diagnose() let threshold = Self::threshold_for_pwm(self.get_pwm() as f64) as u32;
let tacho = self.tacho.get();
if tacho >= threshold {
FanStatus::OK
} else if tacho >= TACHO_HALT_THRESHOLD {
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.
FanStatus::TooSlow
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.
} else {
esavkin marked this conversation as resolved Outdated
Outdated
Review

|=

|=
FanStatus::Halted
}
} }
fn get_pwm(&self) -> u32 { fn get_pwm(&self) -> u32 {
let duty = self.fan.get_duty(); let duty = self.fan.get_duty();
let max = self.fan.get_max_duty(); let max = self.fan.get_max_duty();
((duty as f64 / (max as f64)) * MAX_FAN_PWM) as u32 (Self::scale_number(duty as f64 / (max as f64), MIN_USER_FAN_PWM, MAX_USER_FAN_PWM, MIN_FAN_PWM, MAX_FAN_PWM) + 0.5) as u32
} }
} }
@ -180,34 +220,24 @@ impl TachoCtrl {
tacho_cnt: 0, tacho_cnt: 0,
tacho_value: None, tacho_value: None,
prev_epoch: 0, prev_epoch: 0,
past_record: 0,
} }
} }
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, // These lines do not cause NVIC to run the ISR,
// since the interrupt should be unmasked in the cortex_m::peripheral::NVIC. // since the interrupt is masked in the cortex_m::peripheral::NVIC.
// Also using interrupt-related workaround is the best // Also using interrupt-related workaround is the best
// option for the current version of stm32f4xx-hal, // option for the current version of stm32f4xx-hal,
// since tying the IC's PC8 with the PWM's PC9 to the same TIM8 is not supported, // since tying the IC's PC8 with the PWM's PC9 to the same TIM8 is not supported.
// and therefore would require even more weirder and unsafe hacks. // The possible solution would be to update the library to >=v0.14.*,
// Also such hacks wouldn't guarantee it to be more precise. // and use its Timer's counter functionality.
esavkin marked this conversation as resolved Outdated
Outdated
Review

???????

???????
self.tacho.make_interrupt_source(syscfg); self.tacho.make_interrupt_source(syscfg);
self.tacho.trigger_on_edge(exti, Edge::Rising); self.tacho.trigger_on_edge(exti, Edge::Rising);
self.tacho.enable_interrupt(exti); self.tacho.enable_interrupt(exti);
} }
#[inline] // returns whether the epoch elapsed
fn add_record(&mut self, value: u32) { fn cycle(&mut self) -> bool {
self.past_record = self.past_record << 2;
if value >= TACHO_LOW_THRESHOLD {
self.past_record += 0b11;
} 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(); let tacho_input = self.tacho.check_interrupt();
if tacho_input { if tacho_input {
self.tacho.clear_interrupt_pending_bit(); self.tacho.clear_interrupt_pending_bit();
@ -217,25 +247,17 @@ impl TachoCtrl {
let instant = Instant::from_millis(i64::from(timer::now())); let instant = Instant::from_millis(i64::from(timer::now()));
if instant.millis - self.prev_epoch >= TACHO_MEASURE_MS { if instant.millis - self.prev_epoch >= TACHO_MEASURE_MS {
self.tacho_value = Some(self.tacho_cnt); self.tacho_value = Some(self.tacho_cnt);
self.add_record(self.tacho_cnt);
self.tacho_cnt = 0; self.tacho_cnt = 0;
self.prev_epoch = instant.millis; self.prev_epoch = instant.millis;
true
} else {
false
} }
} }
fn get(&self) -> u32 { fn get(&self) -> u32 {
self.tacho_value.unwrap_or(u32::MAX) 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 {
FanStatus::LowSignal
} else {
FanStatus::Stalled
}
}
} }
impl HWRev { impl HWRev {
@ -272,8 +294,23 @@ impl FanStatus {
match *self { match *self {
FanStatus::OK => "Fan is OK".as_bytes(), FanStatus::OK => "Fan is OK".as_bytes(),
FanStatus::NotAvailable => "Fan is not available".as_bytes(), FanStatus::NotAvailable => "Fan is not available".as_bytes(),
FanStatus::Stalled => "Fan is stalled".as_bytes(), FanStatus::TooSlow => "Fan is too slow".as_bytes(),
FanStatus::LowSignal => "Fan is low signal".as_bytes(), FanStatus::Halted => "Fan is halted".as_bytes(),
}
}
}
#[cfg(test)]
mod test {
use super::*;
#[test]
fn test_scaler() {
for x in 1..100 {
assert_eq!((FanCtrl::scale_number(
FanCtrl::scale_number(x as f64, MIN_FAN_PWM, MAX_FAN_PWM, MIN_USER_FAN_PWM, MAX_USER_FAN_PWM),
MIN_USER_FAN_PWM, MAX_USER_FAN_PWM, MIN_FAN_PWM, MAX_FAN_PWM) + 0.5) as i32,
x);
} }
} }
} }

View File

@ -10,7 +10,6 @@ use panic_abort as _;
use panic_semihosting as _; use panic_semihosting as _;
use log::{error, info, warn}; use log::{error, info, warn};
use core::cell::RefCell;
use cortex_m::asm::wfi; use cortex_m::asm::wfi;
use cortex_m_rt::entry; use cortex_m_rt::entry;
use stm32f4xx_hal::{ use stm32f4xx_hal::{
@ -140,20 +139,19 @@ fn main() -> ! {
let mut store = flash_store::store(dp.FLASH); let mut store = flash_store::store(dp.FLASH);
let mut channels = RefCell::new(Channels::new(pins)); let mut channels = Channels::new(pins);
for c in 0..CHANNELS { for c in 0..CHANNELS {
match store.read_value::<ChannelConfig>(CHANNEL_CONFIG_KEY[c]) { match store.read_value::<ChannelConfig>(CHANNEL_CONFIG_KEY[c]) {
Ok(Some(config)) => Ok(Some(config)) =>
config.apply(channels.get_mut(), c), config.apply(&mut channels, c),
Ok(None) => Ok(None) =>
error!("flash config not found for channel {}", c), error!("flash config not found for channel {}", c),
Err(e) => Err(e) =>
error!("unable to load config {} from flash: {:?}", c, e), error!("unable to load config {} from flash: {:?}", c, e),
} }
} }
// considered safe since `channels` is being mutated in a single thread,
// while mutex would be excessive let mut fan_ctrl = FanCtrl::new(fan, tacho, channels, &mut dp.EXTI, &mut dp.SYSCFG.constrain());
let mut fan_ctrl = FanCtrl::new(fan, tacho, unsafe{ &mut *channels.as_ptr() }, &mut dp.EXTI, &mut dp.SYSCFG.constrain());
// default net config: // default net config:
esavkin marked this conversation as resolved Outdated
Outdated
Review

Highly suspicious. I doubt either mutex or unsafe is necessary.

Highly suspicious. I doubt either mutex or unsafe is necessary.
let mut ipv4_config = Ipv4Config { let mut ipv4_config = Ipv4Config {
@ -183,7 +181,7 @@ fn main() -> ! {
loop { loop {
let mut new_ipv4_config = None; let mut new_ipv4_config = None;
let instant = Instant::from_millis(i64::from(timer::now())); let instant = Instant::from_millis(i64::from(timer::now()));
Outdated
Review

It's silly that fan_ctrl owns channels, don't you think?

What about just passing the few values required between channels and fan_ctrl in main, instead of this broken abstraction?

It's silly that ``fan_ctrl`` owns ``channels``, don't you think? What about just passing the few values required between ``channels`` and ``fan_ctrl`` in ``main``, instead of this broken abstraction?

indeed

indeed
let updated_channel = channels.get_mut().poll_adc(instant); let updated_channel = fan_ctrl.channels.poll_adc(instant);
Outdated
Review

I don't see why you keep talking about precision. Is the current code missing pulses? If not, precision should be more than sufficient already.

I don't see why you keep talking about precision. Is the current code missing pulses? If not, precision should be more than sufficient already.

I do not have exact numbers on pulses missing IRL. The tested cases are:

  1. Increasing PWM should lead to increased tacho number.
  2. Manually lowering the fan speed should lead to lower numbers.
I do not have exact numbers on pulses missing IRL. The tested cases are: 1. Increasing PWM should lead to increased tacho number. 2. Manually lowering the fan speed should lead to lower numbers.
Outdated
Review

Just use the oscilloscope on the tacho signal and check if the frequency reported on the oscilloscope matches what you get from your firmware at various fan speeds and particularly at maximum fan speed with concurrent network traffic.

Just use the oscilloscope on the tacho signal and check if the frequency reported on the oscilloscope matches what you get from your firmware at various fan speeds and particularly at maximum fan speed with concurrent network traffic.

Oscilloscope shows similar values

Oscilloscope shows similar values
if let Some(channel) = updated_channel { if let Some(channel) = updated_channel {
server.for_each(|_, session| session.set_report_pending(channel.into())); server.for_each(|_, session| session.set_report_pending(channel.into()));
} }
@ -213,7 +211,7 @@ fn main() -> ! {
// Do nothing and feed more data to the line reader in the next loop cycle. // Do nothing and feed more data to the line reader in the next loop cycle.
Ok(SessionInput::Nothing) => {} Ok(SessionInput::Nothing) => {}
Ok(SessionInput::Command(command)) => { Ok(SessionInput::Command(command)) => {
match Handler::handle_command(command, &mut socket, channels.get_mut(), session, &mut leds, &mut store, &mut ipv4_config, &mut fan_ctrl) { match Handler::handle_command(command, &mut socket, session, &mut leds, &mut store, &mut ipv4_config, &mut fan_ctrl) {
Ok(Handler::NewIPV4(ip)) => new_ipv4_config = Some(ip), Ok(Handler::NewIPV4(ip)) => new_ipv4_config = Some(ip),
Ok(Handler::Handled) => {}, Ok(Handler::Handled) => {},
Ok(Handler::CloseSocket) => socket.close(), Ok(Handler::CloseSocket) => socket.close(),
@ -230,7 +228,7 @@ fn main() -> ! {
} }
} else if socket.can_send() { } else if socket.can_send() {
if let Some(channel) = session.is_report_pending() { if let Some(channel) = session.is_report_pending() {
match channels.get_mut().reports_json() { match fan_ctrl.channels.reports_json() {
Ok(buf) => { Ok(buf) => {
send_line(&mut socket, &buf[..]); send_line(&mut socket, &buf[..]);
session.mark_report_sent(channel); session.mark_report_sent(channel);