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 131 additions and 56 deletions
Showing only changes of commit d117c784d9 - Show all commits

View File

@ -124,6 +124,7 @@ formatted as line-delimited JSON.
| `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 <value>` | Set fan power with values from 0 to 100, where 0 is auto mode (TODO) |
## USB

View File

@ -19,12 +19,19 @@ use crate::{
pins,
steinhart_hart,
};
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;
#[derive(Serialize, Copy, Clone)]
esavkin marked this conversation as resolved Outdated
Outdated
Review

schematics

schematics
pub struct HWRev {
pub major: u8,
pub minor: u8
}
// TODO: -pub
pub struct Channels {
channel0: Channel<Channel0>,
@ -33,6 +40,7 @@ pub struct Channels {
/// stm32f4 integrated adc
pins_adc: pins::PinsAdc,
pub pwm: pins::PwmPins,
pub hw_rev: HWRev
}
impl Channels {
@ -54,7 +62,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 };
let mut channels = Channels { channel0, channel1, adc, pins_adc, pwm, hw_rev: Self::detect_hw_rev(&pins.hwrev) };
for channel in 0..CHANNELS {
channels.channel_state(channel).vref = channels.read_vref(channel);
channels.calibrate_dac_value(channel);
@ -63,6 +71,17 @@ 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} ,
Outdated
Review

Why is this in Channels?

Should't this be in impl HWRev instead? Same with fan_available()

Why is this in ``Channels``? Should't this be in ``impl HWRev`` instead? Same with ``fan_available()``

Moved to the separate file

Moved to the separate file
(_, _, _, _) => HWRev {major: 0, minor: 0}
}
}
pub fn channel_state<I: Into<usize>>(&mut self, channel: I) -> &mut ChannelState {
match channel.into() {
0 => &mut self.channel0.state,
@ -336,22 +355,20 @@ 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) =>
get(&self.pwm.max_i_neg0),
(0, PwmPin::MaxV) =>
get(&self.pwm.max_v0),
(0, PwmPin::Fan) =>
get(&self.pwm.fan),
(1, PwmPin::MaxIPos) =>
get(&self.pwm.max_i_pos1),
(1, PwmPin::MaxINeg) =>
get(&self.pwm.max_i_neg1),
Outdated
Review

I continue to have reservations with putting the fan info with this ChannelState. You did not address them. The fan is not related to a particular channel.

I continue to have reservations with putting the fan info with this ``ChannelState``. You did not address them. The fan is not related to a particular channel.

Yes, it is not tied to any channel. But it is logically similar to other PWM pins.

Yes, it is not tied to any channel. But it is logically similar to other PWM pins.
(1, PwmPin::MaxV) =>
get(&self.pwm.max_v1),
(1, PwmPin::Fan) =>
get(&self.pwm.fan),
_ =>
unreachable!(),
}
@ -395,22 +412,20 @@ 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) =>
set(&mut self.pwm.max_i_neg0, duty),
(0, PwmPin::MaxV) =>
set(&mut self.pwm.max_v0, duty),
(0, PwmPin::Fan) =>
set(&mut self.pwm.fan, duty),
(1, PwmPin::MaxIPos) =>
set(&mut self.pwm.max_i_pos1, duty),
(1, PwmPin::MaxINeg) =>
set(&mut self.pwm.max_i_neg1, duty),
(1, PwmPin::MaxV) =>
set(&mut self.pwm.max_v1, duty),
(1, PwmPin::Fan) =>
set(&mut self.pwm.fan, duty),
_ =>
unreachable!(),
}
@ -437,7 +452,16 @@ impl Channels {
(duty * max, max)
}
fn report(&mut self, channel: usize) -> Report {
pub fn set_fan_pwm(&mut self, fan_pwm: u32) -> f64 {
let duty = fan_pwm as f64 / 100.0;
self.set_pwm(0, PwmPin::Fan, duty)
}
pub fn get_fan_pwm(&mut self) -> u32 {
(self.get_pwm(0, PwmPin::Fan) * 100.0) as u32
Outdated
Review

Why does this need to be in Channels?

Why does this need to be in ``Channels``?
}
fn report(&mut self, channel: usize, tacho: Option<u32>) -> Report {
let vref = self.channel_state(channel).vref;
let i_set = self.get_i(channel);
let i_tec = self.read_itec(channel);
@ -462,13 +486,15 @@ impl Channels {
tec_i,
tec_u_meas: self.get_tec_v(channel),
pid_output,
tacho,
hwrev: self.hw_rev
}
}
pub fn reports_json(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
pub fn reports_json(&mut self, tacho: Option<u32>) -> Result<JsonBuffer, serde_json_core::ser::Error> {
let mut reports = Vec::<_, U2>::new();
for channel in 0..CHANNELS {
Outdated
Review

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

PWM channels cannot measure frequencies. That's not going to work.
let _ = reports.push(self.report(channel));
let _ = reports.push(self.report(channel, tacho));
}
serde_json_core::to_vec(&reports)
}
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?
@ -481,7 +507,7 @@ impl Channels {
serde_json_core::to_vec(&summaries)
}
fn pwm_summary(&mut self, channel: usize, tacho: u32) -> PwmSummary {
fn pwm_summary(&mut self, channel: usize) -> PwmSummary {
PwmSummary {
channel,
center: CenterPointJson(self.channel_state(channel).center.clone()),
@ -489,15 +515,14 @@ impl Channels {
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_neg: self.get_max_i_neg(channel).into(),
tacho: tacho * 1000 / 1024,
fan: self.get_pwm(0, PwmPin::Fan),
fan: self.get_fan_pwm(),
}
}
pub fn pwm_summaries_json(&mut self, tacho: u32) -> Result<JsonBuffer, serde_json_core::ser::Error> {
pub fn pwm_summaries_json(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
let mut summaries = Vec::<_, U2>::new();
for channel in 0..CHANNELS {
let _ = summaries.push(self.pwm_summary(channel, tacho));
let _ = summaries.push(self.pwm_summary(channel));
}
serde_json_core::to_vec(&summaries)
}
@ -549,6 +574,8 @@ pub struct Report {
tec_i: ElectricCurrent,
tec_u_meas: ElectricPotential,
pid_output: ElectricCurrent,
tacho: Option<u32>,
hwrev: HWRev,
}
pub struct CenterPointJson(CenterPoint);
@ -588,8 +615,7 @@ pub struct PwmSummary {
max_v: PwmSummaryField<ElectricPotential>,
max_i_pos: PwmSummaryField<ElectricCurrent>,
max_i_neg: PwmSummaryField<ElectricCurrent>,
tacho: u32,
fan: f64,
fan: u32,
}
#[derive(Serialize)]

View File

@ -93,8 +93,8 @@ impl Handler {
Ok(Handler::Handled)
}
fn show_report(socket: &mut TcpSocket, channels: &mut Channels) -> Result<Handler, Error> {
match channels.reports_json() {
fn show_report(socket: &mut TcpSocket, channels: &mut Channels, tacho: Option<u32>) -> Result<Handler, Error> {
match channels.reports_json(tacho) {
Ok(buf) => {
send_line(socket, &buf[..]);
}
@ -121,8 +121,8 @@ impl Handler {
Ok(Handler::Handled)
}
fn show_pwm(socket: &mut TcpSocket, channels: &mut Channels, tacho: u32) -> Result<Handler, Error> {
match channels.pwm_summaries_json(tacho) {
fn show_pwm(socket: &mut TcpSocket, channels: &mut Channels) -> Result<Handler, Error> {
match channels.pwm_summaries_json() {
Ok(buf) => {
send_line(socket, &buf);
}
@ -200,7 +200,7 @@ impl Handler {
channels.set_max_i_neg(channel, current);
}
PwmPin::Fan => {
channels.set_pwm(channel, PwmPin::Fan, value);
channels.set_fan_pwm(value as u32);
}
}
send_line(socket, b"{}");
@ -344,14 +344,26 @@ impl Handler {
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, tacho_value: u32) -> Result<Self, Error> {
fn fan (channels: &mut Channels, fan_pwm: Option<u32>) -> Result<Handler, Error> {
match fan_pwm {
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.
Some(val) => {
channels.set_fan_pwm(val);
Ok(Handler::Handled)
},
None => {
Err(Error::ReportError)
}
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....
}
}
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> {
Outdated
Review

at your own risk

at your own risk
match command {
Command::Quit => Ok(Handler::CloseSocket),
Command::Reporting(_reporting) => Handler::reporting(socket),
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, tacho_value),
Command::Show(ShowCommand::Pid) => Handler::show_pid(socket, channels),
Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, channels, tacho_value),
Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, channels),
Command::Show(ShowCommand::SteinhartHart) => Handler::show_steinhart_hart(socket, channels),
Command::Show(ShowCommand::PostFilter) => Handler::show_post_filter(socket, channels),
Command::Show(ShowCommand::Ipv4) => Handler::show_ipv4(socket, ipv4_config),
Outdated
Review

Remove space before ( (also in the other functions)

Remove space before ( (also in the other functions)
@ -366,7 +378,8 @@ 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::Fan {fan_pwm} => Handler::fan(channels, fan_pwm)
}
}
}

View File

@ -179,6 +179,9 @@ pub enum Command {
rate: Option<f32>,
},
Dfu,
Fan {
fan_pwm: Option<u32>
}
}
fn end(input: &[u8]) -> IResult<&[u8], ()> {
@ -301,16 +304,6 @@ fn pwm_setup(input: &[u8]) -> IResult<&[u8], Result<(PwmPin, f64), Error>> {
),
result_with_pin(PwmPin::MaxV)
),
map(
preceded(
tag("fan"),
preceded(
whitespace,
float
)
),
result_with_pin(PwmPin::Fan)
)
)
)(input)
}
@ -532,6 +525,22 @@ fn ipv4(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
))(input)
}
fn fan(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
let (input, _) = tag("fan")(input)?;
let (input, fan_pwm) = alt((
|input| {
let (input, _) = whitespace(input)?;
let (input, value) = unsigned(input)?;
let (input, _) = end(input)?;
Ok((input, Some(value.unwrap_or(0))))
},
value(None, end)
))(input)?;
let result = Ok(Command::Fan { fan_pwm });
Ok((input, result))
}
fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
alt((value(Ok(Command::Quit), tag("quit")),
load,
@ -545,6 +554,7 @@ fn command(input: &[u8]) -> IResult<&[u8], Result<Command, Error>> {
steinhart_hart,
postfilter,
value(Ok(Command::Dfu), tag("dfu")),
fan,
))(input)
}

0
src/fan_ctrl.rs Normal file
View File

View File

@ -152,6 +152,8 @@ fn main() -> ! {
}
}
let fan_available = channels.hw_rev.major == 2 && channels.hw_rev.minor == 2;
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 {
address: [192, 168, 1, 26],
@ -172,17 +174,26 @@ fn main() -> ! {
let hwaddr = EthernetAddress(eui48);
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);
if fan_available {
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.
// 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.
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
// Also such hacks wouldn't guarantee it to be more precise.
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
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, 0u32);
let mut prev_epoch = i64::from(timer::now()) >> 10;
let (mut tacho_cnt, mut tacho_value) = (0u32, None);
let mut prev_epoch: i64 = 0;
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.
loop {
let mut new_ipv4_config = None;
@ -193,17 +204,23 @@ fn main() -> ! {
}
let instant = Instant::from_millis(i64::from(timer::now()));
let tacho_input = tacho.check_interrupt();
tacho.clear_interrupt_pending_bit();
if tacho_input {
tacho_cnt += 1;
}
let epoch = instant.millis >> 10;
if epoch > prev_epoch {
tacho_value = tacho_cnt;
tacho_cnt = 0;
prev_epoch = epoch;
if fan_available {
let mut tacho_input = false;
cortex_m::interrupt::free(|_cs| {
tacho_input = tacho.check_interrupt();
tacho.clear_interrupt_pending_bit();
});
if tacho_input {
tacho_cnt += 1;
}
let epoch = instant.secs();
if epoch > prev_epoch {
tacho_value = Some(tacho_cnt);
tacho_cnt = 0;
prev_epoch = epoch;
}
}
cortex_m::interrupt::free(net::clear_pending);
@ -245,7 +262,7 @@ fn main() -> ! {
}
} else if socket.can_send() {
if let Some(channel) = session.is_report_pending() {
match channels.reports_json() {
match channels.reports_json(tacho_value) {
Ok(buf) => {
send_line(&mut socket, &buf[..]);
session.mark_report_sent(channel);

View File

@ -101,6 +101,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,
@ -108,6 +115,7 @@ pub struct Pins {
pub pwm: PwmPins,
pub channel0: ChannelPinSet<Channel0>,
pub channel1: ChannelPinSet<Channel1>,
pub hwrev: HWRevPins
}
impl Pins {
@ -141,8 +149,6 @@ impl Pins {
gpioe.pe13, gpioe.pe14, gpioc.pc9
);
// let tacho = gpioc.pc8.into_floating_input().into_input_pin()();
let (dac0_spi, dac0_sync) = Self::setup_dac0(
clocks, spi4,
gpioe.pe2, gpioe.pe4, gpioe.pe6
@ -189,6 +195,8 @@ impl Pins {
pwm,
channel0,
channel1,
hwrev: HWRevPins {hwrev0: gpiod.pd0, hwrev1: gpiod.pd1,
hwrev2: gpiod.pd2, hwrev3: gpiod.pd3}
};
let leds = Leds::new(gpiod.pd9, gpiod.pd10.into_push_pull_output(), gpiod.pd11.into_push_pull_output());