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
8 changed files with 149 additions and 84 deletions
Showing only changes of commit 2c9436a0b3 - Show all commits

View File

@ -129,6 +129,7 @@ formatted as line-delimited JSON.
| `fan auto` | Enable automatic fan speed control |
| `fcurve <a> <b> <c>` | Set fan controller curve coefficients (see *Fan control* section) |
| `fcurve default` | Set fan controller curve coefficients to defaults (see *Fan control* section) |
| `hwrev` | Show hardware revision |
## USB

View File

@ -1,5 +1,5 @@
use core::cmp::max_by;
use heapless::{consts::{U2, U1024}, Vec};
use heapless::{consts::U2, Vec};
use serde::{Serialize, Serializer};
use smoltcp::time::Instant;
use stm32f4xx_hal::hal;
@ -17,9 +17,9 @@ use crate::{
channel::{Channel, Channel0, Channel1},
channel_state::ChannelState,
command_parser::{CenterPoint, PwmPin},
command_handler::JsonBuffer,
pins,
steinhart_hart,
fan_ctrl::HWRev,
};
pub const CHANNELS: usize = 2;
@ -35,7 +35,6 @@ pub struct Channels {
/// stm32f4 integrated adc
pins_adc: pins::PinsAdc,
pub pwm: pins::PwmPins,
pub hwrev: HWRev,
}
impl Channels {
@ -57,8 +56,7 @@ impl Channels {
let channel1 = Channel::new(pins.channel1, adc_calibration1);
let pins_adc = pins.pins_adc;
let pwm = pins.pwm;
let mut channels = Channels { channel0, channel1, adc, pins_adc, pwm,
hwrev: HWRev::detect_hw_rev(&pins.hwrev)};
let mut channels = Channels { channel0, channel1, adc, pins_adc, pwm };
for channel in 0..CHANNELS {
channels.channel_state(channel).vref = channels.read_vref(channel);
channels.calibrate_dac_value(channel);
@ -458,7 +456,6 @@ impl Channels {
tec_i,
tec_u_meas: self.get_tec_v(channel),
pid_output,
hwrev: self.hwrev
}
}
Outdated
Review

Why does this need to be in Channels?

Why does this need to be in ``Channels``?
@ -531,8 +528,6 @@ impl Channels {
}
}
pub type JsonBuffer = Vec<u8, U1024>;
#[derive(Serialize)]
pub struct Report {
channel: usize,
@ -550,7 +545,6 @@ pub struct Report {
tec_i: ElectricCurrent,
tec_u_meas: ElectricPotential,
pid_output: ElectricCurrent,
hwrev: HWRev,
}
pub struct CenterPointJson(CenterPoint);

View File

@ -1,6 +1,7 @@
use smoltcp::socket::TcpSocket;
use log::{error, warn};
use core::fmt::Write;
use heapless::{consts::U1024, Vec};
use super::{
net,
command_parser::{
@ -24,6 +25,7 @@ use super::{
flash_store::FlashStore,
session::Session,
FanCtrl,
hw_rev::HWRev,
};
use uom::{
@ -56,6 +58,8 @@ pub enum Error {
FlashError
}
pub type JsonBuffer = Vec<u8, U1024>;
fn send_line(socket: &mut TcpSocket, data: &[u8]) -> bool {
let send_free = socket.send_capacity() - socket.send_queue();
if data.len() > send_free + 1 {
@ -345,7 +349,11 @@ impl Handler {
fn set_fan(socket: &mut TcpSocket, fan_pwm: u32, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
fan_ctrl.set_auto_mode(false);
fan_ctrl.set_pwm(fan_pwm);
if fan_ctrl.is_default_auto() {
send_line(socket, b"{}");
} else {
send_line(socket, b"{ \"warning\": \"this fan doesn't have full PWM support. Use it on your own risk!\" }");
Outdated
Review

Why does this get printed on !is_default_auto()? Looks like function naming or layout could still use some work....

Why does this get printed on ``!is_default_auto()``? Looks like function naming or layout could still use some work....
}
Ok(Handler::Handled)
}
Outdated
Review

at your own risk

at your own risk
@ -365,11 +373,15 @@ impl Handler {
fn fan_auto(socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
fan_ctrl.set_auto_mode(true);
if fan_ctrl.is_default_auto() {
send_line(socket, b"{}");
} else {
send_line(socket, b"{ \"warning\": \"this fan doesn't have full PWM support. Use it on your own risk!\" }");
}
Ok(Handler::Handled)
}
fn fan_curve(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: f32, k_b: f32, k_c: f32) -> Result<Handler, Error> {
fan_ctrl.set_curve(k_a, k_b, k_c);
send_line(socket, b"{}");
Ok(Handler::Handled)
Outdated
Review

same

same
@ -381,7 +393,21 @@ impl Handler {
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> {
fn show_hwrev(socket: &mut TcpSocket, hwrev: HWRev) -> Result<Handler, Error> {
match hwrev.summary() {
Ok(buf) => {
send_line(socket, &buf);
Ok(Handler::Handled)
}
Err(e) => {
error!("unable to serialize HWRev summary: {:?}", e);
let _ = writeln!(socket, "{{\"error\":\"{:?}\"}}", e);
Err(Error::ReportError)
}
}
}
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, hwrev: HWRev) -> Result<Self, Error> {
match command {
Command::Quit => Ok(Handler::CloseSocket),
Command::Reporting(_reporting) => Handler::reporting(socket),
@ -409,6 +435,7 @@ impl Handler {
Command::FanAuto => Handler::fan_auto(socket, fan_ctrl),
Command::FanCurve { k_a, k_b, k_c } => Handler::fan_curve(socket, fan_ctrl, k_a, k_b, k_c),
Command::FanCurveDefaults => Handler::fan_defaults(socket, fan_ctrl),
Command::ShowHWRev => Handler::show_hwrev(socket, hwrev),
}
}
}

View File

@ -185,11 +185,12 @@ pub enum Command {
FanAuto,
ShowFan,
FanCurve {
k_a: f64,
k_b: f64,
k_c: f64,
k_a: f32,
k_b: f32,
k_c: f32,
},
FanCurveDefaults,
ShowHWRev,
}
fn end(input: &[u8]) -> IResult<&[u8], ()> {
@ -573,7 +574,7 @@ fn fan_curve(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
let (input, k_c) = float(input)?;
let (input, _) = end(input)?;
if k_a.is_ok() && k_b.is_ok() && k_c.is_ok() {
Ok((input, Ok(Command::FanCurve { k_a: k_a.unwrap(), k_b: k_b.unwrap(), k_c: k_c.unwrap() })))
Ok((input, Ok(Command::FanCurve { k_a: k_a.unwrap() as f32, k_b: k_b.unwrap() as f32, k_c: k_c.unwrap() as f32 })))
} else {
Err(nom::Err::Incomplete(Needed::Size(3)))
}
@ -601,6 +602,7 @@ fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
value(Ok(Command::Dfu), tag("dfu")),
fan,
fan_curve,
value(Ok(Command::ShowHWRev), tag("hwrev")),
))(input)
}
@ -856,4 +858,10 @@ mod test {
let command = Command::parse(b"fcurve default");
assert_eq!(command, Ok(Command::FanCurveDefaults));
}
#[test]
fn parse_hwrev() {
let command = Command::parse(b"hwrev");
assert_eq!(command, Ok(Command::ShowHWRev));
}
}

View File

@ -6,65 +6,62 @@ use stm32f4xx_hal::{
};
use crate::{
pins::HWRevPins,
channels::JsonBuffer,
hw_rev::HWRev,
command_handler::JsonBuffer,
};
pub type FanPin = PwmChannels<TIM8, pwm::C4>;
// as stated in the schematics
const MAX_TEC_I: f64 = 3.0;
const MAX_TEC_I: f32 = 3.0;
Outdated
Review

f32 is probably plenty enough for all this fan stuff.

f32 is probably plenty enough for all this fan stuff.
const MAX_USER_FAN_PWM: f64 = 100.0;
const MIN_USER_FAN_PWM: f64 = 1.0;
const MAX_FAN_PWM: f64 = 1.0;
const MAX_USER_FAN_PWM: f32 = 100.0;
const MIN_USER_FAN_PWM: f32 = 1.0;
const MAX_FAN_PWM: f32 = 1.0;
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
// below this value motor's autostart feature may fail
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 MIN_FAN_PWM: f64 = 0.04;
const MIN_FAN_PWM: f32 = 0.04;
const DEFAULT_K_A: f64 = 1.0;
const DEFAULT_K_B: f64 = 0.0;
const DEFAULT_K_C: f64 = 0.0;
#[derive(Serialize, Copy, Clone)]
pub struct HWRev {
pub major: u8,
pub minor: u8,
}
const DEFAULT_K_A: f32 = 1.0;
const DEFAULT_K_B: f32 = 0.0;
const DEFAULT_K_C: f32 = 0.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.
pub struct FanCtrl {
fan: FanPin,
fan_auto: bool,
available: bool,
k_a: f64,
k_b: f64,
k_c: f64,
abs_max_tec_i: f64,
default_auto: bool,
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
pwm_enabled: bool,
k_a: f32,
k_b: f32,
k_c: f32,
abs_max_tec_i: f32,
}
Outdated
Review

at the user's own risk

at the user's own risk
impl FanCtrl {
pub fn new(mut fan: FanPin, hwrev: &HWRev) -> Self {
pub fn new(fan: FanPin, hwrev: HWRev) -> Self {
let available = hwrev.fan_available();
let default_auto = hwrev.fan_default_auto();
if available {
fan.set_duty(0);
fan.enable();
}
FanCtrl {
let mut fan_ctrl = FanCtrl {
fan,
available,
// do not enable auto mode by default,
// but allow to turn it on on customer's own risk
fan_auto: hwrev.fan_auto_mode_available(),
// but allow to turn it on on user's own risk
default_auto,
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
fan_auto: default_auto,
pwm_enabled: false,
k_a: DEFAULT_K_A,
k_b: DEFAULT_K_B,
k_c: DEFAULT_K_C,
abs_max_tec_i: 0f64,
abs_max_tec_i: 0f32,
};
if fan_ctrl.fan_auto {
Outdated
Review

at user's own risk

at user's own risk
fan_ctrl.enable_pwm();
}
fan_ctrl
}
pub fn cycle(&mut self, abs_max_tec_i: f64) {
pub fn cycle(&mut self, abs_max_tec_i: f32) {
self.abs_max_tec_i = abs_max_tec_i;
self.adjust_speed();
Review

inline

inline
}
@ -99,7 +96,7 @@ impl FanCtrl {
self.fan_auto = fan_auto;
}
pub fn set_curve(&mut self, k_a: f64, k_b: f64, k_c: f64) {
pub fn set_curve(&mut self, k_a: f32, k_b: f32, k_c: f32) {
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.
self.k_a = k_a;
self.k_b = k_b;
self.k_c = k_c;
@ -109,55 +106,47 @@ impl FanCtrl {
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) -> f32 {
if !self.pwm_enabled {
self.enable_pwm()
}
let fan_pwm = fan_pwm.min(MAX_USER_FAN_PWM as u32).max(MIN_USER_FAN_PWM as u32);
let duty = Self::scale_number(fan_pwm as f64, MIN_FAN_PWM, MAX_FAN_PWM, MIN_USER_FAN_PWM, MAX_USER_FAN_PWM);
let duty = Self::scale_number(fan_pwm as f32, MIN_FAN_PWM, MAX_FAN_PWM, MIN_USER_FAN_PWM, MAX_USER_FAN_PWM);
let max = self.fan.get_max_duty();
let value = ((duty * (max as f64)) as u16).min(max);
let value = ((duty * (max as f32)) as u16).min(max);
self.fan.set_duty(value);
value as f64 / (max as f64)
value as f32 / (max as f32)
}
fn scale_number(unscaled: f64, to_min: f64, to_max: f64, from_min: f64, from_max: f64) -> f64 {
pub fn is_default_auto(&self) -> bool {
self.default_auto
}
fn scale_number(unscaled: f32, to_min: f32, to_max: f32, from_min: f32, from_max: f32) -> f32 {
Outdated
Review

this does not need to be a class member

this does not need to be a class member
(to_max - to_min) * (unscaled - from_min) / (from_max - from_min) + to_min
}
Outdated
Review

.round() not +0.5

.round() not +0.5
fn get_pwm(&self) -> u32 {
let duty = self.fan.get_duty();
let max = self.fan.get_max_duty();
Self::scale_number(duty as f64 / (max as f64), MIN_USER_FAN_PWM, MAX_USER_FAN_PWM, MIN_FAN_PWM, MAX_FAN_PWM).round() as u32
}
}
impl HWRev {
pub fn detect_hw_rev(hwrev_pins: &HWRevPins) -> Self {
let (h0, h1, h2, h3) = (hwrev_pins.hwrev0.is_high(), hwrev_pins.hwrev1.is_high(),
hwrev_pins.hwrev2.is_high(), hwrev_pins.hwrev3.is_high());
match (h0, h1, h2, h3) {
(true, true, true, false) => HWRev { major: 1, minor: 0 },
(true, false, false, false) => HWRev { major: 2, minor: 0 },
(false, true, false, false) => HWRev { major: 2, minor: 2 },
(_, _, _, _) => HWRev { major: 0, minor: 0 }
}
Self::scale_number(duty as f32 / (max as f32), MIN_USER_FAN_PWM, MAX_USER_FAN_PWM, MIN_FAN_PWM, MAX_FAN_PWM).round() as u32
Outdated
Review

Move to separate file. This is not intrinsically linked to the fan.

Move to separate file. This is not intrinsically linked to the fan.
}
pub fn fan_available(&self) -> bool {
self.major == 2 && self.minor == 2
fn enable_pwm(&mut self) {
if self.available {
self.fan.set_duty(0);
self.fan.enable();
self.pwm_enabled = true;
}
pub fn fan_auto_mode_available(&self) -> bool {
// see https://github.com/sinara-hw/Thermostat/issues/115 and
// https://git.m-labs.hk/M-Labs/thermostat/issues/69#issuecomment-6464 for explanation
self.fan_available() && self.minor != 2
}
}
#[derive(Serialize)]
pub struct FanSummary {
fan_pwm: u32,
abs_max_tec_i: f64,
abs_max_tec_i: f32,
auto_mode: bool,
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.
k_a: f64,
k_b: f64,
k_c: f64,
k_a: f32,
k_b: f32,
k_c: f32,
}

41
src/hw_rev.rs Normal file
View File

@ -0,0 +1,41 @@
use serde::Serialize;
use crate::{
pins::HWRevPins,
command_handler::JsonBuffer
};
#[derive(Serialize, Copy, Clone)]
pub struct HWRev {
pub major: u8,
pub minor: u8,
}
impl HWRev {
pub fn detect_hw_rev(hwrev_pins: &HWRevPins) -> Self {
let (h0, h1, h2, h3) = (hwrev_pins.hwrev0.is_high(), hwrev_pins.hwrev1.is_high(),
hwrev_pins.hwrev2.is_high(), hwrev_pins.hwrev3.is_high());
match (h0, h1, h2, h3) {
(true, true, true, false) => HWRev { major: 1, minor: 0 },
(true, false, false, false) => HWRev { major: 2, minor: 0 },
(false, true, false, false) => HWRev { major: 2, minor: 2 },
(_, _, _, _) => HWRev { major: 0, minor: 0 }
}
}
pub fn fan_available(&self) -> bool {
self.major == 2 && self.minor == 2
}
pub fn fan_default_auto(&self) -> bool {
Review

fan_pwm_recommended?

``fan_pwm_recommended``?
// see https://github.com/sinara-hw/Thermostat/issues/115 and
// https://git.m-labs.hk/M-Labs/thermostat/issues/69#issuecomment-6464 for explanation
self.fan_available() && self.minor != 2
}
pub fn summary(&self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
serde_json_core::to_vec(&self)
Review

blank line

blank line
}
}

View File

@ -55,6 +55,8 @@ mod command_handler;
use command_handler::Handler;
mod fan_ctrl;
use fan_ctrl::FanCtrl;
mod hw_rev;
use hw_rev::HWRev;
const HSE: MegaHertz = MegaHertz(8);
#[cfg(not(feature = "semihosting"))]
@ -138,6 +140,8 @@ fn main() -> ! {
let mut store = flash_store::store(dp.FLASH);
let hwrev = HWRev::detect_hw_rev(&pins.hwrev);
let mut channels = Channels::new(pins);
for c in 0..CHANNELS {
match store.read_value::<ChannelConfig>(CHANNEL_CONFIG_KEY[c]) {
@ -150,7 +154,7 @@ fn main() -> ! {
}
}
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 fan_ctrl = FanCtrl::new(fan, &channels.hwrev);
let mut fan_ctrl = FanCtrl::new(fan, hwrev);
// default net config:
let mut ipv4_config = Ipv4Config {
@ -185,7 +189,7 @@ fn main() -> ! {
server.for_each(|_, session| session.set_report_pending(channel.into()));
}
fan_ctrl.cycle(channels.current_abs_max_tec_i());
fan_ctrl.cycle(channels.current_abs_max_tec_i() as f32);
let instant = Instant::from_millis(i64::from(timer::now()));
cortex_m::interrupt::free(net::clear_pending);
@ -210,7 +214,7 @@ fn main() -> ! {
// Do nothing and feed more data to the line reader in the next loop cycle.
Ok(SessionInput::Nothing) => {}
Ok(SessionInput::Command(command)) => {
match Handler::handle_command(command, &mut socket, &mut channels, session, &mut leds, &mut store, &mut ipv4_config, &mut fan_ctrl) {
match Handler::handle_command(command, &mut socket, &mut channels, session, &mut leds, &mut store, &mut ipv4_config, &mut fan_ctrl, hwrev) {
Ok(Handler::NewIPV4(ip)) => new_ipv4_config = Some(ip),
Ok(Handler::Handled) => {},
Ok(Handler::CloseSocket) => socket.close(),

View File

@ -227,7 +227,8 @@ impl Pins {
};
// According to `SUNON DC Brushless Fan & Blower(255-E)` catalogue p.36-37
// Model name: MF35101V1-1000U-G99
// model MF35101V1-1000U-G99 doesn't have a PWM wire, so it is advised to have
Outdated
Review

G99 is not a PWM fan.

G99 is not a PWM fan.
// higher frequency to have less audible noise.
Outdated
Review

Advised where? All the SUNON docs say PWM on this fan model isn't supported at all.

Advised where? All the SUNON docs say PWM on this fan model isn't supported at all.
Outdated
Review

And what "higher frequency" are you talking about?

And what "higher frequency" are you talking about?
let fan = Timer::new(tim8, &clocks).pwm(gpioc.pc9.into_alternate(), 25u32.khz());
(pins, leds, eeprom, eth_pins, usb, fan)