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 43 additions and 32 deletions
Showing only changes of commit 4223f7a4ad - Show all commits

View File

@ -342,8 +342,6 @@ impl Channels {
get(&self.pwm.max_i_neg0), get(&self.pwm.max_i_neg0),
(0, PwmPin::MaxV) => (0, PwmPin::MaxV) =>
get(&self.pwm.max_v0), get(&self.pwm.max_v0),
(0, PwmPin::Tacho) =>
get(&self.pwm.tacho),
(0, PwmPin::Fan) => (0, PwmPin::Fan) =>
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.
get(&self.pwm.fan), get(&self.pwm.fan),
(1, PwmPin::MaxIPos) => (1, PwmPin::MaxIPos) =>
@ -352,8 +350,6 @@ impl Channels {
get(&self.pwm.max_i_neg1), get(&self.pwm.max_i_neg1),
(1, PwmPin::MaxV) => (1, PwmPin::MaxV) =>
get(&self.pwm.max_v1), get(&self.pwm.max_v1),
(1, PwmPin::Tacho) =>
get(&self.pwm.tacho),
(1, PwmPin::Fan) => (1, PwmPin::Fan) =>
get(&self.pwm.fan), get(&self.pwm.fan),
_ => _ =>
@ -485,7 +481,7 @@ impl Channels {
serde_json_core::to_vec(&summaries) serde_json_core::to_vec(&summaries)
} }
fn pwm_summary(&mut self, channel: usize) -> PwmSummary { fn pwm_summary(&mut self, channel: usize, tacho: u32) -> PwmSummary {
Outdated
Review

tacho isn't a pwm. Remove from this function or rename.

tacho isn't a pwm. Remove from this function or rename.
PwmSummary { PwmSummary {
channel, channel,
center: CenterPointJson(self.channel_state(channel).center.clone()), center: CenterPointJson(self.channel_state(channel).center.clone()),
@ -493,15 +489,15 @@ impl Channels {
max_v: (self.get_max_v(channel), ElectricPotential::new::<volt>(5.0)).into(), max_v: (self.get_max_v(channel), ElectricPotential::new::<volt>(5.0)).into(),
max_i_pos: self.get_max_i_pos(channel).into(), max_i_pos: self.get_max_i_pos(channel).into(),
max_i_neg: self.get_max_i_neg(channel).into(), max_i_neg: self.get_max_i_neg(channel).into(),
tacho: self.get_pwm(0, PwmPin::Tacho), tacho: tacho * 1000 / 1024,
fan: self.get_pwm(0, PwmPin::Fan), fan: self.get_pwm(0, PwmPin::Fan),
} }
} }
Outdated
Review

PWM channels cannot measure frequencies. That's not going to work.

PWM channels cannot measure frequencies. That's not going to work.
pub fn pwm_summaries_json(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> { pub fn pwm_summaries_json(&mut self, tacho: u32) -> Result<JsonBuffer, serde_json_core::ser::Error> {
let mut summaries = Vec::<_, U2>::new(); let mut summaries = Vec::<_, U2>::new();
for channel in 0..CHANNELS { for channel in 0..CHANNELS {
let _ = summaries.push(self.pwm_summary(channel)); let _ = summaries.push(self.pwm_summary(channel, tacho));
Outdated
Review

Why does tacho need to appear twice?

Why does tacho need to appear twice?

Because output is JSON array, and I didn't want to restructure it for this not so major change

Because output is JSON array, and I didn't want to restructure it for this not so major change
Outdated
Review

Can you just put it somewhere else than that array then?

Can you just put it somewhere else than that array then?
} }
serde_json_core::to_vec(&summaries) serde_json_core::to_vec(&summaries)
} }
esavkin marked this conversation as resolved Outdated
Outdated
Review

Call it hwrev or hw_rev but not both.

Call it hwrev or hw_rev but not both.
@ -592,7 +588,7 @@ pub struct PwmSummary {
max_v: PwmSummaryField<ElectricPotential>, max_v: PwmSummaryField<ElectricPotential>,
max_i_pos: PwmSummaryField<ElectricCurrent>, max_i_pos: PwmSummaryField<ElectricCurrent>,
max_i_neg: PwmSummaryField<ElectricCurrent>, max_i_neg: PwmSummaryField<ElectricCurrent>,
tacho: f64, tacho: u32,
fan: f64, fan: f64,
Outdated
Review

Noise can potentially make this >1 with maximum TEC current settings. What happens then?

Noise can potentially make this >1 with maximum TEC current settings. What happens then?

It would still run on 1.0, as there is a limiter in the PWM setter.

It would still run on 1.0, as there is a limiter in the PWM setter.
} }
Outdated
Review

You should make the slope configurable and then experiment on the hardware to find a reasonable default.

You should make the slope configurable and then experiment on the hardware to find a reasonable default.

Introduce a,b,c of quadratic equation coefficients?

Introduce a,b,c of quadratic equation coefficients?
Outdated
Review

Just a*current**2 + b should be enough...

Just ``a*current**2 + b`` should be enough...

c would add ability to make it nearly linear

`c` would add ability to make it nearly linear

Also, that may be useful for GUI, as user could choose any 3 points on the fan graph.

Also, that may be useful for GUI, as user could choose any 3 points on the fan graph.
Outdated
Review

I doubt we need a GUI for this, the hardware is fixed.

I doubt we need a GUI for this, the hardware is fixed.

View File

@ -121,8 +121,8 @@ impl Handler {
Ok(Handler::Handled) Ok(Handler::Handled)
} }
fn show_pwm(socket: &mut TcpSocket, channels: &mut Channels) -> Result<Handler, Error> { fn show_pwm(socket: &mut TcpSocket, channels: &mut Channels, tacho: u32) -> Result<Handler, Error> {
match channels.pwm_summaries_json() { match channels.pwm_summaries_json(tacho) {
Ok(buf) => { Ok(buf) => {
send_line(socket, &buf); send_line(socket, &buf);
} }
@ -199,7 +199,6 @@ impl Handler {
let current = ElectricCurrent::new::<ampere>(value); let current = ElectricCurrent::new::<ampere>(value);
channels.set_max_i_neg(channel, current); channels.set_max_i_neg(channel, current);
} }
PwmPin::Tacho => {}
PwmPin::Fan => { PwmPin::Fan => {
channels.set_pwm(channel, PwmPin::Fan, value); channels.set_pwm(channel, PwmPin::Fan, value);
} }
@ -345,14 +344,14 @@ impl Handler {
Ok(Handler::Reset) Ok(Handler::Reset)
} }
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> { 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: u32) -> Result<Self, Error> {
match command { match command {
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.
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, channels),
Command::Show(ShowCommand::Pid) => Handler::show_pid(socket, channels), Command::Show(ShowCommand::Pid) => Handler::show_pid(socket, channels),
Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, channels), Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, channels, tacho_value),
Command::Show(ShowCommand::SteinhartHart) => Handler::show_steinhart_hart(socket, channels), Command::Show(ShowCommand::SteinhartHart) => Handler::show_steinhart_hart(socket, channels),
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....
Command::Show(ShowCommand::PostFilter) => Handler::show_post_filter(socket, channels), Command::Show(ShowCommand::PostFilter) => Handler::show_post_filter(socket, channels),
Command::Show(ShowCommand::Ipv4) => Handler::show_ipv4(socket, ipv4_config), Command::Show(ShowCommand::Ipv4) => Handler::show_ipv4(socket, ipv4_config),

View File

@ -127,7 +127,6 @@ pub enum PwmPin {
MaxIPos, MaxIPos,
MaxINeg, MaxINeg,
MaxV, MaxV,
Tacho,
Fan Fan
} }

View File

@ -25,6 +25,8 @@ use smoltcp::{
socket::TcpSocket, socket::TcpSocket,
wire::EthernetAddress, wire::EthernetAddress,
}; };
use stm32f4xx_hal::gpio::{Edge, ExtiPin};
use stm32f4xx_hal::syscfg::SysCfgExt;
mod init_log; mod init_log;
use init_log::init_log; use init_log::init_log;
@ -102,7 +104,7 @@ fn main() -> ! {
cp.SCB.enable_icache(); cp.SCB.enable_icache();
cp.SCB.enable_dcache(&mut cp.CPUID); cp.SCB.enable_dcache(&mut cp.CPUID);
let dp = Peripherals::take().unwrap(); let mut dp = Peripherals::take().unwrap();
let clocks = dp.RCC.constrain() let clocks = dp.RCC.constrain()
.cfgr .cfgr
.use_hse(HSE) .use_hse(HSE)
@ -118,7 +120,7 @@ fn main() -> ! {
timer::setup(cp.SYST, clocks); timer::setup(cp.SYST, clocks);
let (pins, mut leds, mut eeprom, eth_pins, usb) = Pins::setup( let (pins, mut leds, mut eeprom, eth_pins, usb, mut tacho) = Pins::setup(
clocks, dp.TIM1, dp.TIM3, dp.TIM8, clocks, dp.TIM1, dp.TIM3, dp.TIM8,
dp.GPIOA, dp.GPIOB, dp.GPIOC, dp.GPIOD, dp.GPIOE, dp.GPIOF, dp.GPIOG, dp.GPIOA, dp.GPIOB, dp.GPIOC, dp.GPIOD, dp.GPIOE, dp.GPIOF, dp.GPIOG,
dp.I2C1, dp.I2C1,
@ -170,11 +172,18 @@ fn main() -> ! {
let hwaddr = EthernetAddress(eui48); let hwaddr = EthernetAddress(eui48);
info!("EEPROM MAC address: {}", hwaddr); info!("EEPROM MAC address: {}", hwaddr);
tacho.make_interrupt_source(&mut dp.SYSCFG.constrain());
tacho.trigger_on_edge(&mut dp.EXTI, Edge::Rising);
tacho.enable_interrupt(&mut dp.EXTI);
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.
net::run(clocks, dp.ETHERNET_MAC, dp.ETHERNET_DMA, eth_pins, hwaddr, ipv4_config.clone(), |iface| { net::run(clocks, dp.ETHERNET_MAC, dp.ETHERNET_DMA, eth_pins, hwaddr, ipv4_config.clone(), |iface| {
Server::<Session>::run(iface, |server| { Server::<Session>::run(iface, |server| {
leds.r1.off(); leds.r1.off();
let mut should_reset = false; let mut should_reset = false;
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 tacho_cnt, mut tacho_value) = (0u32, 0u32);
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 mut prev_epoch = i64::from(timer::now()) >> 10;
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()));
@ -184,6 +193,19 @@ fn main() -> ! {
} }
let instant = Instant::from_millis(i64::from(timer::now())); let instant = Instant::from_millis(i64::from(timer::now()));
let tacho_input = tacho.check_interrupt();
tacho.clear_interrupt_pending_bit();
Outdated
Review

Classic mistake.

Classic mistake.

There is no need to clear_interrupt_pending_bit before check_interrupt since there are a lot of guaranteed CPU cycles ahead.
Moreover, I did so, and it didn't counted fan cycles at all.

There is no need to `clear_interrupt_pending_bit` before `check_interrupt` since there are a lot of guaranteed CPU cycles ahead. Moreover, I did so, and it didn't counted fan cycles at all.
Outdated
Review

You missed the point.
What happens if a pulse arrives between the two lines of code?

You missed the point. What happens if a pulse arrives between the two lines of code?

Added it to critical section

Added it to critical section
Outdated
Review

And that's supposed to fix the problem how?

Please think about what is actually happening before reflexively slapping a textbook solution.

And that's supposed to fix the problem how? Please think about what is actually happening before reflexively slapping a textbook solution.

Anyway it would miss some pulses. And that can even happen if pulse comes twice during the cycle. But since the fan shouldn't make significantly more than 100 rps, I don't see a big issue there. In manual experiments this method showed relative differences between various pwm options.

Anyway it would miss some pulses. And that can even happen if pulse comes twice during the cycle. But since the fan shouldn't make significantly more than 100 rps, I don't see a big issue there. In manual experiments this method showed relative differences between various pwm options.
if tacho_input {
tacho_cnt += 1;
}
let epoch = instant.millis >> 10;
Outdated
Review

Isn't there just instant.seconds or similar?
I just gave you the general principle, you need to adapt it.

Isn't there just ``instant.seconds`` or similar? I just gave you the general principle, you need to adapt it.
if epoch > prev_epoch {
tacho_value = tacho_cnt;
tacho_cnt = 0;
prev_epoch = epoch;
}
cortex_m::interrupt::free(net::clear_pending); cortex_m::interrupt::free(net::clear_pending);
server.poll(instant) server.poll(instant)
.unwrap_or_else(|e| { .unwrap_or_else(|e| {
@ -206,7 +228,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, &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, tacho_value) {
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(),

View File

@ -120,7 +120,7 @@ impl Pins {
spi2: SPI2, spi4: SPI4, spi5: SPI5, spi2: SPI2, spi4: SPI4, spi5: SPI5,
adc1: ADC1, adc1: ADC1,
otg_fs_global: OTG_FS_GLOBAL, otg_fs_device: OTG_FS_DEVICE, otg_fs_pwrclk: OTG_FS_PWRCLK, 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, PC8<Input<Floating>>) {
let gpioa = gpioa.split(); let gpioa = gpioa.split();
let gpiob = gpiob.split(); let gpiob = gpiob.split();
let gpioc = gpioc.split(); let gpioc = gpioc.split();
@ -138,9 +138,11 @@ impl Pins {
clocks, tim1, tim3, tim8, clocks, tim1, tim3, tim8,
gpioc.pc6, gpioc.pc7, gpioc.pc6, gpioc.pc7,
gpioe.pe9, gpioe.pe11, gpioe.pe9, gpioe.pe11,
gpioe.pe13, gpioe.pe14, gpioc.pc8, gpioc.pc9 gpioe.pe13, gpioe.pe14, gpioc.pc9
); );
// let tacho = gpioc.pc8.into_floating_input().into_input_pin()();
let (dac0_spi, dac0_sync) = Self::setup_dac0( let (dac0_spi, dac0_sync) = Self::setup_dac0(
clocks, spi4, clocks, spi4,
gpioe.pe2, gpioe.pe4, gpioe.pe6 gpioe.pe2, gpioe.pe4, gpioe.pe6
@ -215,7 +217,7 @@ impl Pins {
hclk: clocks.hclk(), hclk: clocks.hclk(),
}; };
(pins, leds, eeprom, eth_pins, usb) (pins, leds, eeprom, eth_pins, usb, gpioc.pc8)
} }
/// Configure the GPIO pins for SPI operation, and initialize SPI /// Configure the GPIO pins for SPI operation, and initialize SPI
@ -283,12 +285,11 @@ pub struct PwmPins {
pub max_i_pos1: PwmChannels<TIM1, pwm::C2>, pub max_i_pos1: PwmChannels<TIM1, pwm::C2>,
pub max_i_neg0: PwmChannels<TIM1, pwm::C3>, pub max_i_neg0: PwmChannels<TIM1, pwm::C3>,
pub max_i_neg1: PwmChannels<TIM1, pwm::C4>, pub max_i_neg1: PwmChannels<TIM1, pwm::C4>,
pub tacho: PwmChannels<TIM8, pwm::C3>,
pub fan: PwmChannels<TIM8, pwm::C4>, pub fan: PwmChannels<TIM8, pwm::C4>,
} }
impl PwmPins { impl PwmPins {
fn setup<M1, M2, M3, M4, M5, M6, M7, M8>( fn setup<M1, M2, M3, M4, M5, M6, M7>(
clocks: Clocks, clocks: Clocks,
tim1: TIM1, tim1: TIM1,
tim3: TIM3, tim3: TIM3,
@ -299,8 +300,7 @@ impl PwmPins {
max_i_pos1: PE11<M4>, max_i_pos1: PE11<M4>,
max_i_neg0: PE13<M5>, max_i_neg0: PE13<M5>,
max_i_neg1: PE14<M6>, max_i_neg1: PE14<M6>,
tacho: PC8<M7>, fan: PC9<M7>,
fan: PC9<M8>,
) -> PwmPins { ) -> PwmPins {
let freq = 20u32.khz(); let freq = 20u32.khz();
@ -317,12 +317,7 @@ impl PwmPins {
init_pwm_pin(&mut max_v0); init_pwm_pin(&mut max_v0);
init_pwm_pin(&mut max_v1); init_pwm_pin(&mut max_v1);
let channels = ( let mut fan = Timer::new(tim8, &clocks).pwm(fan.into_alternate(), freq);
tacho.into_alternate(),
fan.into_alternate(),
);
let (mut tacho, mut fan) = Timer::new(tim8, &clocks).pwm(channels, freq);
init_pwm_pin(&mut tacho);
init_pwm_pin(&mut fan); init_pwm_pin(&mut fan);
let channels = ( let channels = (
@ -342,7 +337,7 @@ impl PwmPins {
max_v0, max_v1, max_v0, max_v1,
max_i_pos0, max_i_pos1, max_i_pos0, max_i_pos1,
max_i_neg0, max_i_neg1, max_i_neg0, max_i_neg1,
tacho, fan fan
} }
} }
} }