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 516 additions and 50 deletions

View File

@ -94,36 +94,42 @@ 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 |
| 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 1 to 100 |
| `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, and settings related to it |
## USB
@ -269,3 +275,15 @@ with the following keys.
## PID Tuning
The thermostat implements a PID control loop for each of the TEC channels, more details on setting up the PID control loop can be found [here](./doc/PID%20tuning.md).
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
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`, `abs_max_tec_i`, `auto_mode`, `k_a`, `k_b`, `k_c`.
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
2. `fan auto` - enable auto speed controller mode, which correlates with fan curve `fcurve`.
3. `fan <value>` - set the fan power with the value from `1` to `100` and disable auto mode. There is no way to disable the fan.
Please note that power doesn't correlate with the actual speed linearly.
4. `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.
5. `fcurve default` - restore fan curve settings to defaults: `a = 1.0, b = 0.0, c = 0.0`.

View File

@ -1,4 +1,5 @@
use heapless::{consts::{U2, U1024}, Vec};
use core::cmp::max_by;
use heapless::{consts::U2, Vec};
use serde::{Serialize, Serializer};
use smoltcp::time::Instant;
use stm32f4xx_hal::hal;
@ -16,6 +17,7 @@ use crate::{
channel::{Channel, Channel0, Channel1},
channel_state::ChannelState,
command_parser::{CenterPoint, PwmPin},
command_handler::JsonBuffer,
pins,
steinhart_hart,
};
@ -518,9 +520,13 @@ impl Channels {
}
serde_json_core::to_vec(&summaries)
}
}
type JsonBuffer = Vec<u8, U1024>;
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))
}
}
#[derive(Serialize)]
pub struct Report {

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::{
@ -22,7 +23,9 @@ use super::{
config::ChannelConfig,
dfu,
flash_store::FlashStore,
session::Session
session::Session,
FanCtrl,
hw_rev::HWRev,
};
use uom::{
@ -55,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 {
@ -341,7 +346,76 @@ impl Handler {
Ok(Handler::Reset)
}
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.
pub fn handle_command (command: Command, socket: &mut TcpSocket, channels: &mut Channels, session: &Session, leds: &mut Leds, store: &mut FlashStore, ipv4_config: &mut Ipv4Config) -> Result<Self, Error> {
fn set_fan(socket: &mut TcpSocket, fan_pwm: u32, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
if !fan_ctrl.fan_available() {
send_line(socket, b"{ \"warning\": \"this thermostat doesn't have fan!\" }");
return Ok(Handler::Handled);
}
fan_ctrl.set_auto_mode(false);
fan_ctrl.set_pwm(fan_pwm);
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....
if fan_ctrl.fan_pwm_recommended() {
send_line(socket, b"{}");
} else {
send_line(socket, b"{ \"warning\": \"this fan doesn't have full PWM support. Use it at your own risk!\" }");
Outdated
Review

at your own risk

at your own risk
}
Ok(Handler::Handled)
}
fn show_fan(socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
match fan_ctrl.summary() {
Ok(buf) => {
send_line(socket, &buf);
Ok(Handler::Handled)
}
Outdated
Review

Remove space before ( (also in the other functions)

Remove space before ( (also in the other functions)
Err(e) => {
error!("unable to serialize fan summary: {:?}", e);
let _ = writeln!(socket, "{{\"error\":\"{:?}\"}}", e);
Err(Error::ReportError)
}
}
}
fn fan_auto(socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
if !fan_ctrl.fan_available() {
send_line(socket, b"{ \"warning\": \"this thermostat doesn't have fan!\" }");
return Ok(Handler::Handled);
}
fan_ctrl.set_auto_mode(true);
if fan_ctrl.fan_pwm_recommended() {
send_line(socket, b"{}");
} else {
send_line(socket, b"{ \"warning\": \"this fan doesn't have full PWM support. Use it at your own risk!\" }");
Outdated
Review

same

same
}
Ok(Handler::Handled)
}
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)
}
fn fan_defaults(socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
fan_ctrl.restore_defaults();
send_line(socket, b"{}");
Ok(Handler::Handled)
}
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),
@ -363,7 +437,13 @@ impl Handler {
Command::Save { channel } => Handler::save_channel(socket, channels, channel, store),
Command::Ipv4(config) => Handler::set_ipv4(socket, store, config),
Command::Reset => Handler::reset(channels),
Command::Dfu => Handler::dfu(channels)
Command::Dfu => Handler::dfu(channels),
Command::FanSet {fan_pwm} => Handler::set_fan(socket, fan_pwm, fan_ctrl),
Command::ShowFan => Handler::show_fan(socket, fan_ctrl),
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

@ -10,6 +10,7 @@ use nom::{
sequence::preceded,
multi::{fold_many0, fold_many1},
error::ErrorKind,
Needed,
};
use num_traits::{Num, ParseFloatError};
use serde::{Serialize, Deserialize};
@ -178,6 +179,18 @@ pub enum Command {
rate: Option<f32>,
},
Dfu,
FanSet {
fan_pwm: u32
},
FanAuto,
ShowFan,
FanCurve {
k_a: f32,
k_b: f32,
k_c: f32,
},
FanCurveDefaults,
ShowHWRev,
}
fn end(input: &[u8]) -> IResult<&[u8], ()> {
@ -520,6 +533,57 @@ fn ipv4(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
))(input)
}
fn fan(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
let (input, _) = tag("fan")(input)?;
alt((
|input| {
let (input, _) = whitespace(input)?;
let (input, result) = alt((
|input| {
let (input, _) = tag("auto")(input)?;
Ok((input, Ok(Command::FanAuto)))
},
|input| {
let (input, value) = unsigned(input)?;
Ok((input, Ok(Command::FanSet { fan_pwm: value.unwrap_or(0)})))
},
))(input)?;
Ok((input, result))
},
value(Ok(Command::ShowFan), end)
))(input)
}
fn fan_curve(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
let (input, _) = tag("fcurve")(input)?;
alt((
|input| {
let (input, _) = whitespace(input)?;
let (input, result) = alt((
|input| {
let (input, _) = tag("default")(input)?;
Ok((input, Ok(Command::FanCurveDefaults)))
},
|input| {
let (input, k_a) = float(input)?;
let (input, _) = whitespace(input)?;
Outdated
Review

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

Call it "curve" or "coeff" but not both.
let (input, k_b) = float(input)?;
let (input, _) = whitespace(input)?;
let (input, k_c) = float(input)?;
if k_a.is_ok() && k_b.is_ok() && k_c.is_ok() {
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)))
}
},
))(input)?;
Ok((input, result))
},
value(Err(Error::Incomplete), end)
))(input)
}
fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
alt((value(Ok(Command::Quit), tag("quit")),
load,
@ -533,6 +597,9 @@ fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
steinhart_hart,
postfilter,
value(Ok(Command::Dfu), tag("dfu")),
fan,
fan_curve,
value(Ok(Command::ShowHWRev), tag("hwrev")),
))(input)
}
@ -754,4 +821,44 @@ mod test {
center: CenterPoint::Vref,
}));
}
#[test]
fn parse_fan_show() {
let command = Command::parse(b"fan");
assert_eq!(command, Ok(Command::ShowFan));
}
#[test]
fn parse_fan_set() {
let command = Command::parse(b"fan 42");
assert_eq!(command, Ok(Command::FanSet {fan_pwm: 42}));
}
#[test]
fn parse_fan_auto() {
let command = Command::parse(b"fan auto");
assert_eq!(command, Ok(Command::FanAuto));
}
#[test]
fn parse_fcurve_set() {
let command = Command::parse(b"fcurve 1.2 3.4 5.6");
assert_eq!(command, Ok(Command::FanCurve {
k_a: 1.2,
k_b: 3.4,
k_c: 5.6
}));
}
#[test]
fn parse_fcurve_default() {
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));
}
}

151
src/fan_ctrl.rs Normal file
View File

@ -0,0 +1,151 @@
use num_traits::Float;
use serde::Serialize;
use stm32f4xx_hal::{
pwm::{self, PwmChannels},
pac::TIM8,
};
use crate::{
hw_rev::HWSettings,
command_handler::JsonBuffer,
};
pub type FanPin = PwmChannels<TIM8, pwm::C4>;
// as stated in the schematics
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: f32 = 100.0;
const MIN_USER_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
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 struct FanCtrl {
fan: Option<FanPin>,
fan_auto: bool,
pwm_enabled: bool,
k_a: f32,
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.
k_b: f32,
k_c: f32,
abs_max_tec_i: f32,
hw_settings: HWSettings,
}
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
impl FanCtrl {
pub fn new(fan: Option<FanPin>, hw_settings: HWSettings) -> Self {
let mut fan_ctrl = FanCtrl {
fan,
// do not enable auto mode by default,
// but allow to turn it at the user's own risk
Outdated
Review

at the user's own risk

at the user's own risk
fan_auto: hw_settings.fan_pwm_recommended,
pwm_enabled: false,
k_a: hw_settings.fan_k_a,
k_b: hw_settings.fan_k_b,
k_c: hw_settings.fan_k_c,
abs_max_tec_i: 0f32,
hw_settings,
};
if fan_ctrl.fan_auto {
fan_ctrl.enable_pwm();
}
fan_ctrl
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
}
pub fn cycle(&mut self, abs_max_tec_i: f32) {
self.abs_max_tec_i = abs_max_tec_i;
if self.fan_auto && self.hw_settings.fan_available {
let scaled_current = self.abs_max_tec_i / MAX_TEC_I;
// do not limit upper bound, as it will be limited in the set_pwm()
let pwm = (MAX_USER_FAN_PWM * (scaled_current * (scaled_current * self.k_a + self.k_b) + self.k_c)) as u32;
Outdated
Review

at user's own risk

at user's own risk
self.set_pwm(pwm);
}
}
pub fn summary(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
if self.hw_settings.fan_available {
let summary = FanSummary {
fan_pwm: self.get_pwm(),
Review

inline

inline
abs_max_tec_i: self.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 {
let summary: Option<()> = None;
serde_json_core::to_vec(&summary)
}
}
pub fn set_auto_mode(&mut self, fan_auto: bool) {
self.fan_auto = fan_auto;
}
pub fn set_curve(&mut self, k_a: f32, k_b: f32, k_c: f32) {
self.k_a = k_a;
self.k_b = k_b;
Review

this doesn't need to be pub

this doesn't need to be ``pub``
self.k_c = k_c;
}
pub fn restore_defaults(&mut self) {
self.set_curve(self.hw_settings.fan_k_a,
self.hw_settings.fan_k_b,
self.hw_settings.fan_k_c);
}
pub fn set_pwm(&mut self, fan_pwm: u32) -> f32 {
if self.fan.is_none() || (!self.pwm_enabled && !self.enable_pwm()) {
return 0f32;
}
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 fan = self.fan.as_mut().unwrap();
let fan_pwm = fan_pwm.min(MAX_USER_FAN_PWM as u32).max(MIN_USER_FAN_PWM as u32);
let duty = scale_number(fan_pwm as f32, self.hw_settings.min_fan_pwm, self.hw_settings.max_fan_pwm, MIN_USER_FAN_PWM, MAX_USER_FAN_PWM);
let max = fan.get_max_duty();
let value = ((duty * (max as f32)) as u16).min(max);
fan.set_duty(value);
value as f32 / (max as f32)
}
pub fn fan_pwm_recommended(&self) -> bool {
self.hw_settings.fan_pwm_recommended
}
pub fn fan_available(&self) -> bool {
self.hw_settings.fan_available
}
fn get_pwm(&self) -> u32 {
if let Some(fan) = &self.fan {
let duty = fan.get_duty();
let max = fan.get_max_duty();
scale_number(duty as f32 / (max as f32), MIN_USER_FAN_PWM, MAX_USER_FAN_PWM, self.hw_settings.min_fan_pwm, self.hw_settings.max_fan_pwm).round() as u32
} else { 0 }
}
fn enable_pwm(&mut self) -> bool {
Outdated
Review

this does not need to be a class member

this does not need to be a class member
if self.fan.is_some() && self.hw_settings.fan_available {
let fan = self.fan.as_mut().unwrap();
fan.set_duty(0);
Outdated
Review

.round() not +0.5

.round() not +0.5
fan.enable();
self.pwm_enabled = true;
true
} else {
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.
false
}
}
}
fn scale_number(unscaled: f32, to_min: f32, to_max: f32, from_min: f32, from_max: f32) -> f32 {
(to_max - to_min) * (unscaled - from_min) / (from_max - from_min) + to_min
}
#[derive(Serialize)]
pub struct FanSummary {
fan_pwm: u32,
abs_max_tec_i: f32,
auto_mode: bool,
k_a: f32,
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_b: f32,
k_c: f32,
}

82
src/hw_rev.rs Normal file
View File

@ -0,0 +1,82 @@
use serde::Serialize;
use crate::{
pins::HWRevPins,
command_handler::JsonBuffer,
};
#[derive(Serialize, Copy, Clone)]
pub struct HWRev {
pub major: u8,
pub minor: u8,
}
#[derive(Serialize, Clone)]
pub struct HWSettings {
pub fan_k_a: f32,
pub fan_k_b: f32,
pub fan_k_c: f32,
pub min_fan_pwm: f32,
pub max_fan_pwm: f32,
pub fan_pwm_freq_hz: u32,
pub fan_available: bool,
pub fan_pwm_recommended: bool,
}
#[derive(Serialize, Clone)]
struct HWSummary<'a> {
rev: &'a HWRev,
settings: &'a HWSettings,
}
Review

fan_pwm_recommended?

``fan_pwm_recommended``?
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 },
Review

blank line

blank line
(_, _, _, _) => HWRev { major: 0, minor: 0 }
}
}
pub fn settings(&self) -> HWSettings {
match (self.major, self.minor) {
(2, 2) => HWSettings {
fan_k_a: 1.0,
fan_k_b: 0.0,
fan_k_c: 0.0,
// below this value motor's autostart feature may fail,
// according to internal experiments
min_fan_pwm: 0.04,
max_fan_pwm: 1.0,
// According to `SUNON DC Brushless Fan & Blower(255-E)` catalogue p.36-37
// model MF35101V1-1000U-G99 doesn't have a PWM wire, but we'll follow their others models'
// recommended frequency, as it is said by the Thermostat's schematics that we can
// use PWM, but not stated at which frequency
fan_pwm_freq_hz: 25_000,
fan_available: true,
// see https://github.com/sinara-hw/Thermostat/issues/115 and
// https://git.m-labs.hk/M-Labs/thermostat/issues/69#issuecomment-6464 for explanation
fan_pwm_recommended: false,
},
(_, _) => HWSettings {
fan_k_a: 0.0,
fan_k_b: 0.0,
fan_k_c: 0.0,
min_fan_pwm: 0.0,
max_fan_pwm: 0.0,
fan_pwm_freq_hz: 0,
fan_available: false,
fan_pwm_recommended: false,
}
}
}
pub fn summary(&self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
let settings = self.settings();
let summary = HWSummary { rev: self, settings: &settings };
serde_json_core::to_vec(&summary)
}
}

View File

@ -10,7 +10,6 @@ use panic_abort as _;
use panic_semihosting as _;
use log::{error, info, warn};
use cortex_m::asm::wfi;
use cortex_m_rt::entry;
use stm32f4xx_hal::{
@ -54,6 +53,9 @@ mod flash_store;
mod dfu;
mod command_handler;
use command_handler::Handler;
mod fan_ctrl;
use fan_ctrl::FanCtrl;
mod hw_rev;
const HSE: MegaHertz = MegaHertz(8);
#[cfg(not(feature = "semihosting"))]
@ -118,8 +120,8 @@ fn main() -> ! {
timer::setup(cp.SYST, clocks);
let (pins, mut leds, mut eeprom, eth_pins, usb) = Pins::setup(
clocks, dp.TIM1, dp.TIM3,
let (pins, mut leds, mut eeprom, eth_pins, usb, fan, hwrev, hw_settings) = 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,
dp.SPI2, dp.SPI4, dp.SPI5,
@ -137,7 +139,6 @@ fn main() -> ! {
let mut store = flash_store::store(dp.FLASH);
let mut channels = Channels::new(pins);
for c in 0..CHANNELS {
match store.read_value::<ChannelConfig>(CHANNEL_CONFIG_KEY[c]) {
@ -150,6 +151,8 @@ fn main() -> ! {
}
}
let mut fan_ctrl = FanCtrl::new(fan, hw_settings);
// 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 {
address: [192, 168, 1, 26],
@ -183,6 +186,8 @@ fn main() -> ! {
server.for_each(|_, session| session.set_report_pending(channel.into()));
}
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);
server.poll(instant)
@ -206,7 +211,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) {
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

@ -23,7 +23,7 @@ use stm32f4xx_hal::{
I2C1,
OTG_FS_GLOBAL, OTG_FS_DEVICE, OTG_FS_PWRCLK,
SPI2, SPI4, SPI5,
TIM1, TIM3,
TIM1, TIM3, TIM8
},
timer::Timer,
time::U32Ext,
@ -33,6 +33,8 @@ use stm32_eth::EthPins;
use crate::{
channel::{Channel0, Channel1},
leds::Leds,
fan_ctrl::FanPin,
hw_rev::{HWRev, HWSettings},
};
pub type Eeprom = Eeprom24x<
@ -101,6 +103,13 @@ pub struct ChannelPinSet<C: ChannelPins> {
pub tec_u_meas_pin: C::TecUMeasPin,
}
pub struct HWRevPins {
pub hwrev0: stm32f4xx_hal::gpio::gpiod::PD0<Input<Floating>>,
pub hwrev1: stm32f4xx_hal::gpio::gpiod::PD1<Input<Floating>>,
pub hwrev2: stm32f4xx_hal::gpio::gpiod::PD2<Input<Floating>>,
pub hwrev3: stm32f4xx_hal::gpio::gpiod::PD3<Input<Floating>>,
}
pub struct Pins {
pub adc_spi: AdcSpi,
pub adc_nss: AdcNss,
@ -114,13 +123,13 @@ impl Pins {
/// Setup GPIO pins and configure MCU peripherals
pub fn setup(
clocks: Clocks,
tim1: TIM1, tim3: TIM3,
tim1: TIM1, tim3: TIM3, tim8: TIM8,
gpioa: GPIOA, gpiob: GPIOB, gpioc: GPIOC, gpiod: GPIOD, gpioe: GPIOE, gpiof: GPIOF, gpiog: GPIOG,
i2c1: I2C1,
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) {
) -> (Self, Leds, Eeprom, EthernetPins, USB, Option<FanPin>, HWRev, HWSettings) {
let gpioa = gpioa.split();
let gpiob = gpiob.split();
let gpioc = gpioc.split();
@ -189,6 +198,10 @@ impl Pins {
channel1,
};
let hwrev = HWRev::detect_hw_rev(&HWRevPins {hwrev0: gpiod.pd0, hwrev1: gpiod.pd1,
hwrev2: gpiod.pd2, hwrev3: gpiod.pd3});
let hw_settings = hwrev.settings();
let leds = Leds::new(gpiod.pd9, gpiod.pd10.into_push_pull_output(), gpiod.pd11.into_push_pull_output());
let eeprom_scl = gpiob.pb8.into_alternate().set_open_drain();
@ -215,7 +228,11 @@ 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)
let fan = if hw_settings.fan_available {
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?
Some(Timer::new(tim8, &clocks).pwm(gpioc.pc9.into_alternate(), hw_settings.fan_pwm_freq_hz.hz()))
} else { None };
(pins, leds, eeprom, eth_pins, usb, fan, hwrev, hw_settings)
}
/// Configure the GPIO pins for SPI operation, and initialize SPI