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
7 changed files with 351 additions and 199 deletions
Showing only changes of commit 58650d37f1 - Show all commits

View File

@ -94,38 +94,40 @@ The scope of this setting is per TCP session.
Send commands as simple text string terminated by `\n`. Responses are
formatted as line-delimited JSON.
| Syntax | Function |
|----------------------------------|----------------------------------------------------------------------|
| `report` | Show current input |
| `report mode` | Show current report mode |
| `report mode <off/on>` | Set report mode |
| `pwm` | Show current PWM settings |
| `pwm <0/1> max_i_pos <amp>` | Set maximum positive output current |
| `pwm <0/1> max_i_neg <amp>` | Set maximum negative output current |
| `pwm <0/1> max_v <volt>` | Set maximum output voltage |
| `pwm <0/1> i_set <amp>` | Disengage PID, set fixed output current |
| `pwm <0/1> pid` | Let output current to be controlled by the PID |
| `center <0/1> <volt>` | Set the MAX1968 0A-centerpoint to the specified fixed voltage |
| `center <0/1> vref` | Set the MAX1968 0A-centerpoint to measure from VREF |
| `pid` | Show PID configuration |
| `pid <0/1> target <deg_celsius>` | Set the PID controller target temperature |
| `pid <0/1> kp <value>` | Set proportional gain |
| `pid <0/1> ki <value>` | Set integral gain |
| `pid <0/1> kd <value>` | Set differential gain |
| `pid <0/1> output_min <amp>` | Set mininum output |
| `pid <0/1> output_max <amp>` | Set maximum output |
| `s-h` | Show Steinhart-Hart equation parameters |
| `s-h <0/1> <t0/b/r0> <value>` | Set Steinhart-Hart parameter for a channel |
| `postfilter` | Show postfilter settings |
| `postfilter <0/1> off` | Disable postfilter |
| `postfilter <0/1> rate <rate>` | Set postfilter output data rate |
| `load [0/1]` | Restore configuration for channel all/0/1 from flash |
| `save [0/1]` | Save configuration for channel all/0/1 to flash |
| `reset` | Reset the device |
| `dfu` | Reset device and enters USB device firmware update (DFU) mode |
| `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 <value>` | Set fan power with values from 0 to 100, where 0 is auto mode |
| Syntax | Function |
|----------------------------------|---------------------------------------------------------------------------|
| `report` | Show current input |
| `report mode` | Show current report mode |
| `report mode <off/on>` | Set report mode |
| `pwm` | Show current PWM settings |
| `pwm <0/1> max_i_pos <amp>` | Set maximum positive output current |
| `pwm <0/1> max_i_neg <amp>` | Set maximum negative output current |
| `pwm <0/1> max_v <volt>` | Set maximum output voltage |
| `pwm <0/1> i_set <amp>` | Disengage PID, set fixed output current |
| `pwm <0/1> pid` | Let output current to be controlled by the PID |
| `center <0/1> <volt>` | Set the MAX1968 0A-centerpoint to the specified fixed voltage |
| `center <0/1> vref` | Set the MAX1968 0A-centerpoint to measure from VREF |
| `pid` | Show PID configuration |
| `pid <0/1> target <deg_celsius>` | Set the PID controller target temperature |
| `pid <0/1> kp <value>` | Set proportional gain |
| `pid <0/1> ki <value>` | Set integral gain |
| `pid <0/1> kd <value>` | Set differential gain |
| `pid <0/1> output_min <amp>` | Set mininum output |
| `pid <0/1> output_max <amp>` | Set maximum output |
| `s-h` | Show Steinhart-Hart equation parameters |
| `s-h <0/1> <t0/b/r0> <value>` | Set Steinhart-Hart parameter for a channel |
| `postfilter` | Show postfilter settings |
| `postfilter <0/1> off` | Disable postfilter |
| `postfilter <0/1> rate <rate>` | Set postfilter output data rate |
| `load [0/1]` | Restore configuration for channel all/0/1 from flash |
| `save [0/1]` | Save configuration for channel all/0/1 to flash |
| `reset` | Reset the device |
| `dfu` | Reset device and enters USB device firmware update (DFU) mode |
| `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 <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) |
| `fan-restore` | Set fan controller coefficients to defaults (see *Fan control* section) |
## USB
@ -274,10 +276,14 @@ The thermostat implements a PID control loop for each of the TEC channels, more
## Fan control
sb10q marked this conversation as resolved
Review

Why is it so approximate?

Why is it so approximate?
Review

Mostly because I cannot guarantee it is precise and would not like to see how someone would tie something-very-important to its value.

Mostly because I cannot guarantee it is precise and would not like to see how someone would tie something-very-important to its value.
Fan control is available for the thermostat revisions with integrated fan system. For this purpose two commands are available:
Fan control is available for the thermostat revisions with integrated fan system. For this purpose four commands are available:
1. `fan` - show fan stats: `fan_pwm`, `tacho`, `abs_max_tec_i`, `auto_mode`. Please note that `tacho` shows *approximate* value, which
linearly correlates with the actual fan speed.
2. `fan <value>` - set the fan power with the value from `0` to `100`. Since there is no hardware way to disable the fan,
Outdated
Review

Doesn't the motor stall at low powers?

Doesn't the motor stall at low powers?

No, the minimum I tried was 0.008 PWM, anything below that makes the circuit consider there is no signal and it runs at full power. 0.008-0.04 values are working quite fine, though there is specific noise on low speed.

No, the minimum I tried was 0.008 PWM, anything below that makes the circuit consider there is no signal and it runs at full power. 0.008-0.04 values are working quite fine, though there is specific noise on low speed.
Outdated
Review

"correlates with the square of the TEC's current" is incorrect

"correlates with the square of the TEC's current" is incorrect
`0` value is used for enabling automatic fan control mode, which correlates with the square of the TEC's current.
Values from `1` to `100` are used for setting the power from minimum to maximum respectively.
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`,
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,
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.0`.

View File

@ -19,22 +19,13 @@ use crate::{
command_parser::{CenterPoint, PwmPin},
pins,
steinhart_hart,
fan_ctrl::HWRev,
};
use crate::pins::HWRevPins;
pub const CHANNELS: usize = 2;
pub const R_SENSE: f64 = 0.05;
// DAC chip outputs 0-5v, which is then passed through a resistor dividor to provide 0-3v range
const DAC_OUT_V_MAX: f64 = 3.0;
const MAX_TEC_I: f64 = 3.0; // as stated in the schemes
const MAX_FAN_PWM: f64 = 100.0;
const MIN_FAN_PWM: f64 = 1.0;
#[derive(Serialize, Copy, Clone)]
pub struct HWRev {
pub major: u8,
pub minor: u8
}
esavkin marked this conversation as resolved Outdated
Outdated
Review

schematics

schematics
// TODO: -pub
pub struct Channels {
@ -44,8 +35,7 @@ pub struct Channels {
/// stm32f4 integrated adc
pins_adc: pins::PinsAdc,
pub pwm: pins::PwmPins,
hw_rev: HWRev,
fan_auto: bool
pub hwrev: HWRev,
}
impl Channels {
@ -68,7 +58,7 @@ impl Channels {
let pins_adc = pins.pins_adc;
let pwm = pins.pwm;
let mut channels = Channels { channel0, channel1, adc, pins_adc, pwm,
hw_rev: Self::detect_hw_rev(&pins.hwrev), fan_auto: true };
hwrev: HWRev::detect_hw_rev(&pins.hwrev)};
for channel in 0..CHANNELS {
channels.channel_state(channel).vref = channels.read_vref(channel);
channels.calibrate_dac_value(channel);
@ -77,21 +67,6 @@ impl Channels {
channels
}
fn detect_hw_rev(hwrev_pins: &HWRevPins) -> HWRev {
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.hw_rev.major == 2 && self.hw_rev.minor == 2
}
pub fn channel_state<I: Into<usize>>(&mut self, channel: I) -> &mut ChannelState {
match channel.into() {
0 => &mut self.channel0.state,
@ -365,8 +340,6 @@ impl Channels {
match (channel, pin) {
(_, PwmPin::ISet) =>
panic!("i_set is no pwm pin"),
(_, PwmPin::Fan) =>
get(&self.pwm.fan),
(0, PwmPin::MaxIPos) =>
get(&self.pwm.max_i_pos0),
(0, PwmPin::MaxINeg) =>
Outdated
Review

Are you sure this is the best place to put this? This channel business looks highly suspicious.

Are you sure this is the best place to put this? This channel business looks highly suspicious.
@ -422,8 +395,6 @@ impl Channels {
match (channel, pin) {
(_, PwmPin::ISet) =>
panic!("i_set is no pwm pin"),
(_, PwmPin::Fan) =>
set(&mut self.pwm.fan, duty),
(0, PwmPin::MaxIPos) =>
set(&mut self.pwm.max_i_pos0, duty),
(0, PwmPin::MaxINeg) =>
@ -462,19 +433,6 @@ impl Channels {
(duty * max, max)
}
pub fn set_fan_pwm(&mut self, fan_pwm: u32) -> f64 {
let duty = fan_pwm as f64 / MAX_FAN_PWM;
self.set_pwm(0, PwmPin::Fan, duty)
}
pub fn get_fan_pwm(&mut self) -> u32 {
(self.get_pwm(0, PwmPin::Fan) * MAX_FAN_PWM) as u32
}
pub fn set_fan_auto_mode(&mut self, fan_auto: bool) {
self.fan_auto = fan_auto;
}
fn report(&mut self, channel: usize) -> Report {
let vref = self.channel_state(channel).vref;
let i_set = self.get_i(channel);
@ -500,7 +458,7 @@ impl Channels {
tec_i,
tec_u_meas: self.get_tec_v(channel),
pid_output,
hwrev: self.hw_rev
hwrev: self.hwrev
Outdated
Review

Why does this need to be in Channels?

Why does this need to be in ``Channels``?
}
}
@ -566,39 +524,14 @@ impl Channels {
serde_json_core::to_vec(&summaries)
}
fn current_abs_max_tec_i(&mut self) -> f64 {
pub fn current_abs_max_tec_i(&mut self) -> f64 {
max_by(self.get_tec_i(0).abs().get::<ampere>(),
self.get_tec_i(1).abs().get::<ampere>(),
|a, b| a.partial_cmp(b).unwrap_or(core::cmp::Ordering::Equal))
}
pub fn fan_summary(&mut self, tacho: Option<u32>) -> Result<JsonBuffer, serde_json_core::ser::Error> {
if self.fan_available() {
let summary = FanSummary {
fan_pwm: self.get_fan_pwm(),
tacho: tacho.unwrap_or(u32::MAX),
abs_max_tec_i: self.current_abs_max_tec_i(),
auto_mode: self.fan_auto
};
serde_json_core::to_vec(&summary)
} else {
let summary: Option<()> = None;
serde_json_core::to_vec(&summary)
}
}
pub fn fan_ctrl(&mut self) {
if self.fan_auto && self.fan_available() {
let scaled_current = self.current_abs_max_tec_i() / MAX_TEC_I;
let pwm = max_by(scaled_current * scaled_current * MAX_FAN_PWM,
MIN_FAN_PWM,
|a, b| a.partial_cmp(b).unwrap_or(core::cmp::Ordering::Equal)) as u32;
self.set_fan_pwm(pwm);
}
}
}
type JsonBuffer = Vec<u8, U1024>;
pub type JsonBuffer = Vec<u8, U1024>;
Outdated
Review

If this is used by fan_ctrl then it probably should be moved out of channels.

If this is used by ``fan_ctrl`` then it probably should be moved out of ``channels``.
#[derive(Serialize)]
pub struct Report {
@ -670,11 +603,3 @@ pub struct SteinhartHartSummary {
channel: usize,
params: steinhart_hart::Parameters,
}
#[derive(Serialize)]
pub struct FanSummary {
fan_pwm: u32,
tacho: u32,
abs_max_tec_i: f64,
auto_mode: bool,
}

View File

@ -22,7 +22,8 @@ use super::{
config::ChannelConfig,
dfu,
flash_store::FlashStore,
session::Session
session::Session,
FanCtrl,
};
use uom::{
@ -199,9 +200,6 @@ impl Handler {
let current = ElectricCurrent::new::<ampere>(value);
channels.set_max_i_neg(channel, current);
}
PwmPin::Fan => {
channels.set_fan_pwm(value as u32);
}
}
send_line(socket, b"{}");
Ok(Handler::Handled)
@ -344,14 +342,14 @@ impl Handler {
Ok(Handler::Reset)
}
fn fan (socket: &mut TcpSocket, channels: &mut Channels, fan_pwm: Option<u32>, tacho_value: Option<u32>) -> Result<Handler, Error> {
fn fan (socket: &mut TcpSocket, fan_pwm: Option<u32>, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
match fan_pwm {
Some(val) => {
channels.set_fan_auto_mode(val == 0);
channels.set_fan_pwm(val);
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.
fan_ctrl.set_pwm(val);
},
None => {
match channels.fan_summary(tacho_value) {
match fan_ctrl.summary() {
Ok(buf) => {
send_line(socket, &buf);
return Ok(Handler::Handled);
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....
@ -368,7 +366,19 @@ 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, tacho_value: Option<u32>) -> Result<Self, Error> {
fn fan_coeff (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);
send_line(socket, b"{}");
Ok(Handler::Handled)
}
fn fan_defaults (socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
fan_ctrl.restore_defaults();
send_line(socket, b"{}");
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> {
match command {
Command::Quit => Ok(Handler::CloseSocket),
Command::Reporting(_reporting) => Handler::reporting(socket),
@ -391,7 +401,9 @@ impl Handler {
Command::Ipv4(config) => Handler::set_ipv4(socket, store, config),
Command::Reset => Handler::reset(channels),
Command::Dfu => Handler::dfu(channels),
Command::Fan {fan_pwm} => Handler::fan(socket, channels, fan_pwm, tacho_value)
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::FanDefaults => Handler::fan_defaults(socket, fan_ctrl),
}
}
}

View File

@ -1,16 +1,7 @@
use core::fmt;
use core::num::ParseIntError;
use core::str::{from_utf8, Utf8Error};
use nom::{
IResult,
branch::alt,
bytes::complete::{is_a, tag, take_while1},
character::{is_digit, complete::{char, one_of}},
combinator::{complete, map, opt, value},
sequence::preceded,
multi::{fold_many0, fold_many1},
error::ErrorKind,
};
use nom::{IResult, branch::alt, bytes::complete::{is_a, tag, take_while1}, character::{is_digit, complete::{char, one_of}}, combinator::{complete, map, opt, value}, sequence::preceded, multi::{fold_many0, fold_many1}, error::ErrorKind, Needed};
use num_traits::{Num, ParseFloatError};
use serde::{Serialize, Deserialize};
@ -127,7 +118,6 @@ pub enum PwmPin {
MaxIPos,
MaxINeg,
MaxV,
Fan
}
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
@ -181,7 +171,13 @@ pub enum Command {
Dfu,
Fan {
fan_pwm: Option<u32>
}
},
FanCoeff {
k_a: f64,
k_b: f64,
k_c: f64,
},
FanDefaults,
}
fn end(input: &[u8]) -> IResult<&[u8], ()> {
@ -540,6 +536,33 @@ fn fan(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
Ok((input, result))
}
fn fan_coeff(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
let (input, _) = tag("fcurve")(input)?;
let (input, coeffs) = alt((
|input| {
let (input, _) = whitespace(input)?;
let (input, k_a) = float(input)?;
let (input, _) = whitespace(input)?;
let (input, k_b) = float(input)?;
let (input, _) = whitespace(input)?;
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, Some((k_a.unwrap(), k_b.unwrap(), k_c.unwrap()))))
} else {
Err(nom::Err::Incomplete(Needed::Size(3)))
}
},
value(None, end)
))(input)?;
let result = match coeffs {
Some(coeffs) => Ok(Command::FanCoeff { k_a: coeffs.0, k_b: coeffs.1, k_c: coeffs.2 }),
None => Err(Error::ParseFloat)
};
Ok((input, result))
}
fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
alt((value(Ok(Command::Quit), tag("quit")),
load,
@ -553,7 +576,9 @@ fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
steinhart_hart,
postfilter,
value(Ok(Command::Dfu), tag("dfu")),
value(Ok(Command::FanDefaults), tag("fan-restore")),
fan,
fan_coeff,
))(input)
}

218
src/fan_ctrl.rs Normal file
View File

@ -0,0 +1,218 @@
use core::{cmp::max_by};
use serde::Serialize;
use stm32f4xx_hal::{
pwm::{self, PwmChannels},
pac::TIM8,
gpio::{
Floating, Input, ExtiPin,
gpioc::PC8, Edge,
},
stm32::EXTI,
syscfg::{SysCfg},
};
use smoltcp::time::Instant;
use crate::{
pins::HWRevPins,
Outdated
Review

f32 is probably plenty enough for all this fan stuff.

f32 is probably plenty enough for all this fan stuff.
channels::{Channels, JsonBuffer},
timer
};
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 FanPin = PwmChannels<TIM8, pwm::C4>;
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
pub type TachoPin = PC8<Input<Floating>>;
const MAX_TEC_I: f64 = 3.0;
// as stated in the schematics
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 DEFAULT_K_A: f64 = 1.0;
const DEFAULT_K_B: f64 = 0.0;
const DEFAULT_K_C: f64 = 0.0;
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 {
pub major: u8,
pub minor: u8,
}
Outdated
Review

at the user's own risk

at the user's own risk
struct TachoCtrl {
tacho: TachoPin,
tacho_cnt: u32,
tacho_value: Option<u32>,
prev_epoch: i64,
}
pub struct FanCtrl<'a> {
fan: FanPin,
tacho: TachoCtrl,
fan_auto: bool,
available: bool,
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
k_a: f64,
k_b: f64,
k_c: f64,
channels: &'a mut Channels,
}
impl<'a> FanCtrl<'a> {
pub fn new(mut fan: FanPin, tacho: TachoPin, channels: &'a mut Channels, exti: &mut EXTI, syscfg: &mut SysCfg) -> Self {
Outdated
Review

at user's own risk

at user's own risk
let available = channels.hwrev.fan_available();
let mut tacho_ctrl = TachoCtrl::new(tacho);
if available {
fan.set_duty(0);
fan.enable();
tacho_ctrl.init(exti, syscfg);
}
Review

inline

inline
FanCtrl {
fan,
tacho: tacho_ctrl,
available,
fan_auto: true,
k_a: DEFAULT_K_A,
k_b: DEFAULT_K_B,
k_c: DEFAULT_K_C,
channels,
}
}
pub fn cycle(&mut self) {
if self.available {
self.tacho.cycle();
}
self.adjust_speed();
}
Review

this doesn't need to be pub

this doesn't need to be ``pub``
pub fn summary(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
if self.available {
let summary = FanSummary {
fan_pwm: self.get_pwm(),
tacho: self.tacho.get(),
abs_max_tec_i: self.channels.current_abs_max_tec_i(),
auto_mode: self.fan_auto,
k_a: self.k_a,
k_b: self.k_b,
k_c: self.k_c,
};
serde_json_core::to_vec(&summary)
} else {
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.
let summary: Option<()> = None;
serde_json_core::to_vec(&summary)
}
}
pub fn adjust_speed(&mut self) {
if self.fan_auto && self.available {
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()
let pwm = max_by(MAX_FAN_PWM * (scaled_current * (scaled_current * self.k_a + self.k_b) + self.k_c),
MIN_FAN_PWM,
|a, b| a.partial_cmp(b).unwrap_or(core::cmp::Ordering::Equal)) as u32;
self.set_pwm(pwm);
}
}
#[inline]
pub fn set_auto_mode(&mut self, fan_auto: bool) {
self.fan_auto = fan_auto;
}
#[inline]
pub fn set_coefficients(&mut self, k_a: f64, k_b: f64, k_c: f64) {
self.k_a = k_a;
self.k_b = k_b;
self.k_c = k_c;
Outdated
Review

this does not need to be a class member

this does not need to be a class member
}
#[inline]
Outdated
Review

.round() not +0.5

.round() not +0.5
pub fn restore_defaults(&mut self) {
self.set_auto_mode(true);
self.set_coefficients(DEFAULT_K_A, DEFAULT_K_B, DEFAULT_K_C);
}
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 set_pwm(&mut self, fan_pwm: u32) -> f64 {
let duty = fan_pwm as f64 / MAX_FAN_PWM;
let max = self.fan.get_max_duty();
let value = ((duty * (max as f64)) as u16).min(max);
self.fan.set_duty(value);
value as f64 / (max as f64)
}
fn get_pwm(&self) -> u32 {
let duty = self.fan.get_duty();
let max = self.fan.get_max_duty();
((duty as f64 / (max as f64)) * MAX_FAN_PWM) as u32
}
}
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.
impl TachoCtrl {
pub fn new(tacho: TachoPin) -> Self {
TachoCtrl {
tacho,
tacho_cnt: 0,
tacho_value: None,
prev_epoch: 0,
}
}
pub 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.
// Also using interrupt-related workaround is the best
// 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,
Outdated
Review

Doesn't seem relevant

Doesn't seem relevant
// and therefore would require even more weirder and unsafe hacks.
// Also such hacks wouldn't guarantee it to be more precise.
self.tacho.make_interrupt_source(syscfg);
self.tacho.trigger_on_edge(exti, Edge::Rising);
self.tacho.enable_interrupt(exti);
}
pub fn cycle(&mut self) {
let tacho_input = self.tacho.check_interrupt();
if tacho_input {
self.tacho.clear_interrupt_pending_bit();
self.tacho_cnt += 1;
}
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.tacho_cnt = 0;
self.prev_epoch = instant.millis;
}
}
pub fn get(&self) -> u32 {
self.tacho_value.unwrap_or(u32::MAX)
}
Outdated
Review

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

"since the interrupt is still masked in cortex_m::peripheral::NVIC"
}
impl HWRev {
pub fn detect_hw_rev(hwrev_pins: &HWRevPins) -> Self {
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.
let (h0, h1, h2, h3) = (hwrev_pins.hwrev0.is_high(), hwrev_pins.hwrev1.is_high(),
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."
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 }
}
}
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.
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.
pub fn fan_available(&self) -> bool {
esavkin marked this conversation as resolved Outdated
Outdated
Review

|=

|=
self.major == 2 && self.minor == 2
}
}
#[derive(Serialize)]
pub struct FanSummary {
fan_pwm: u32,
tacho: u32,
abs_max_tec_i: f64,
auto_mode: bool,
k_a: f64,
k_b: f64,
k_c: f64,
}

View File

@ -10,7 +10,7 @@ use panic_abort as _;
use panic_semihosting as _;
use log::{error, info, warn};
use core::cell::RefCell;
use cortex_m::asm::wfi;
use cortex_m_rt::entry;
use stm32f4xx_hal::{
@ -19,7 +19,6 @@ use stm32f4xx_hal::{
stm32::{CorePeripherals, Peripherals, SCB},
time::{U32Ext, MegaHertz},
watchdog::IndependentWatchdog,
gpio::{Edge, ExtiPin},
syscfg::SysCfgExt
};
use smoltcp::{
@ -56,6 +55,8 @@ mod flash_store;
mod dfu;
mod command_handler;
use command_handler::Handler;
mod fan_ctrl;
use fan_ctrl::FanCtrl;
const HSE: MegaHertz = MegaHertz(8);
#[cfg(not(feature = "semihosting"))]
@ -120,7 +121,7 @@ fn main() -> ! {
timer::setup(cp.SYST, clocks);
let (pins, mut leds, mut eeprom, eth_pins, usb, mut tacho) = Pins::setup(
let (pins, mut leds, mut eeprom, eth_pins, usb, fan, tacho) = Pins::setup(
clocks, dp.TIM1, dp.TIM3, dp.TIM8,
dp.GPIOA, dp.GPIOB, dp.GPIOC, dp.GPIOD, dp.GPIOE, dp.GPIOF, dp.GPIOG,
dp.I2C1,
@ -137,22 +138,22 @@ fn main() -> ! {
usb::State::setup(usb);
let mut store = flash_store::store(dp.FLASH);
let mut channels = RefCell::new(Channels::new(pins));
let mut channels = Channels::new(pins);
let mut store = flash_store::store(dp.FLASH);
for c in 0..CHANNELS {
match store.read_value::<ChannelConfig>(CHANNEL_CONFIG_KEY[c]) {
Ok(Some(config)) =>
config.apply(&mut channels, c),
config.apply(channels.get_mut(), c),
Ok(None) =>
error!("flash config not found for channel {}", c),
Err(e) =>
error!("unable to load config {} from flash: {:?}", c, e),
}
}
let fan_available = channels.fan_available();
// considered safe since `channels` is being mutated in a single thread,
// while mutex would be excessive
let mut fan_ctrl = FanCtrl::new(fan, tacho, unsafe{ &mut *channels.as_ptr() }, &mut dp.EXTI, &mut dp.SYSCFG.constrain());
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.
// default net config:
let mut ipv4_config = Ipv4Config {
@ -174,54 +175,22 @@ fn main() -> ! {
let hwaddr = EthernetAddress(eui48);
info!("EEPROM MAC address: {}", hwaddr);
Outdated
Review

If this runs an ISR fix it.
If this doesn't run an ISR add an explanatory comment.

If this runs an ISR fix it. If this doesn't run an ISR add an explanatory comment.
if fan_available {
// These lines do not cause NVIC to run the ISR,
// since the interrupt should be unmasked in the cortex_m::peripheral::NVIC.
// Also using interrupt-related workaround is the best
// 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,
// and therefore would require even more weirder and unsafe hacks.
// Also such hacks wouldn't guarantee it to be more precise.
tacho.make_interrupt_source(&mut dp.SYSCFG.constrain());
tacho.trigger_on_edge(&mut dp.EXTI, Edge::Rising);
tacho.enable_interrupt(&mut dp.EXTI);
}
net::run(clocks, dp.ETHERNET_MAC, dp.ETHERNET_DMA, eth_pins, hwaddr, ipv4_config.clone(), |iface| {
Server::<Session>::run(iface, |server| {
leds.r1.off();
let mut should_reset = false;
let (mut tacho_cnt, mut tacho_value) = (0u32, None);
let mut prev_epoch: i64 = 0;
loop {
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 mut new_ipv4_config = None;
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
let instant = Instant::from_millis(i64::from(timer::now()));
let updated_channel = channels.poll_adc(instant);
let updated_channel = channels.get_mut().poll_adc(instant);
if let Some(channel) = updated_channel {
server.for_each(|_, session| session.set_report_pending(channel.into()));
}
fan_ctrl.cycle();
let instant = Instant::from_millis(i64::from(timer::now()));
if fan_available {
let tacho_input = tacho.check_interrupt();
if tacho_input {
tacho.clear_interrupt_pending_bit();
tacho_cnt += 1;
}
let epoch = instant.secs();
if epoch > prev_epoch {
tacho_value = Some(tacho_cnt);
tacho_cnt = 0;
prev_epoch = epoch;
}
}
channels.fan_ctrl();
cortex_m::interrupt::free(net::clear_pending);
server.poll(instant)
.unwrap_or_else(|e| {
@ -244,7 +213,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, tacho_value) {
match Handler::handle_command(command, &mut socket, channels.get_mut(), session, &mut leds, &mut store, &mut ipv4_config, &mut fan_ctrl) {
Ok(Handler::NewIPV4(ip)) => new_ipv4_config = Some(ip),
Ok(Handler::Handled) => {},
Ok(Handler::CloseSocket) => socket.close(),
@ -261,7 +230,7 @@ fn main() -> ! {
}
} else if socket.can_send() {
if let Some(channel) = session.is_report_pending() {
match channels.reports_json() {
match channels.get_mut().reports_json() {
Ok(buf) => {
send_line(&mut socket, &buf[..]);
session.mark_report_sent(channel);

View File

@ -26,15 +26,18 @@ use stm32f4xx_hal::{
TIM1, TIM3, TIM8
},
timer::Timer,
time::U32Ext,
time::{U32Ext, KiloHertz},
};
use eeprom24x::{self, Eeprom24x};
use stm32_eth::EthPins;
use crate::{
channel::{Channel0, Channel1},
leds::Leds,
fan_ctrl::{TachoPin, FanPin}
};
const PWM_FREQ: KiloHertz = KiloHertz(20u32);
pub type Eeprom = Eeprom24x<
I2c<I2C1, (
PB8<AlternateOD<{ stm32f4xx_hal::gpio::AF4 }>>,
@ -128,7 +131,7 @@ impl Pins {
spi2: SPI2, spi4: SPI4, spi5: SPI5,
adc1: ADC1,
otg_fs_global: OTG_FS_GLOBAL, otg_fs_device: OTG_FS_DEVICE, otg_fs_pwrclk: OTG_FS_PWRCLK,
) -> (Self, Leds, Eeprom, EthernetPins, USB, PC8<Input<Floating>>) {
) -> (Self, Leds, Eeprom, EthernetPins, USB, FanPin, TachoPin) {
let gpioa = gpioa.split();
let gpiob = gpiob.split();
let gpioc = gpioc.split();
@ -143,10 +146,10 @@ impl Pins {
let pins_adc = Adc::adc1(adc1, true, Default::default());
let pwm = PwmPins::setup(
clocks, tim1, tim3, tim8,
clocks, tim1, tim3,
gpioc.pc6, gpioc.pc7,
gpioe.pe9, gpioe.pe11,
gpioe.pe13, gpioe.pe14, gpioc.pc9
gpioe.pe13, gpioe.pe14
);
let (dac0_spi, dac0_sync) = Self::setup_dac0(
@ -225,7 +228,9 @@ impl Pins {
hclk: clocks.hclk(),
};
Outdated
Review

G99 is not a PWM fan.

G99 is not a PWM fan.
(pins, leds, eeprom, eth_pins, usb, gpioc.pc8)
let fan = Timer::new(tim8, &clocks).pwm(gpioc.pc9.into_alternate(), 20u32.khz());
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?
(pins, leds, eeprom, eth_pins, usb, fan, gpioc.pc8)
}
/// Configure the GPIO pins for SPI operation, and initialize SPI
@ -293,24 +298,20 @@ pub struct PwmPins {
pub max_i_pos1: PwmChannels<TIM1, pwm::C2>,
pub max_i_neg0: PwmChannels<TIM1, pwm::C3>,
pub max_i_neg1: PwmChannels<TIM1, pwm::C4>,
pub fan: PwmChannels<TIM8, pwm::C4>,
}
impl PwmPins {
fn setup<M1, M2, M3, M4, M5, M6, M7>(
fn setup<M1, M2, M3, M4, M5, M6>(
clocks: Clocks,
tim1: TIM1,
tim3: TIM3,
tim8: TIM8,
max_v0: PC6<M1>,
max_v1: PC7<M2>,
max_i_pos0: PE9<M3>,
max_i_pos1: PE11<M4>,
max_i_neg0: PE13<M5>,
max_i_neg1: PE14<M6>,
fan: PC9<M7>,
) -> PwmPins {
let freq = 20u32.khz();
fn init_pwm_pin<P: hal::PwmPin<Duty=u16>>(pin: &mut P) {
pin.set_duty(0);
@ -320,14 +321,11 @@ impl PwmPins {
max_v0.into_alternate(),
max_v1.into_alternate(),
);
//let (mut max_v0, mut max_v1) = pwm::tim3(tim3, channels, clocks, freq);
let (mut max_v0, mut max_v1) = Timer::new(tim3, &clocks).pwm(channels, freq);
//let (mut max_v0, mut max_v1) = pwm::tim3(tim3, channels, clocks, PWM_FREQ);
let (mut max_v0, mut max_v1) = Timer::new(tim3, &clocks).pwm(channels, PWM_FREQ);
Outdated
Review

No.

No.
init_pwm_pin(&mut max_v0);
init_pwm_pin(&mut max_v1);
let mut fan = Timer::new(tim8, &clocks).pwm(fan.into_alternate(), freq);
init_pwm_pin(&mut fan);
let channels = (
max_i_pos0.into_alternate(),
max_i_pos1.into_alternate(),
@ -335,7 +333,7 @@ impl PwmPins {
max_i_neg1.into_alternate(),
);
let (mut max_i_pos0, mut max_i_pos1, mut max_i_neg0, mut max_i_neg1) =
Timer::new(tim1, &clocks).pwm(channels, freq);
Timer::new(tim1, &clocks).pwm(channels, PWM_FREQ);
init_pwm_pin(&mut max_i_pos0);
init_pwm_pin(&mut max_i_neg0);
init_pwm_pin(&mut max_i_pos1);
@ -344,8 +342,7 @@ impl PwmPins {
PwmPins {
max_v0, max_v1,
max_i_pos0, max_i_pos1,
max_i_neg0, max_i_neg1,
fan
max_i_neg0, max_i_neg1
}
}
}