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
6 changed files with 52 additions and 22 deletions
Showing only changes of commit 583d06a78b - Show all commits

View File

@ -95,7 +95,7 @@ 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 |
@ -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` | Show current fan settings and sensors' measurements |
| `fan <value>` | Set fan power with values from 0 to 100, where 0 is auto mode (TODO) |

View File

@ -69,7 +69,7 @@
buildInputs = with pkgs; [
rustPlatform.rust.rustc
rustPlatform.rust.cargo
openocd dfu-util
openocd dfu-util gcc-arm-embedded-10
Outdated
Review

Why?

Why?

Needed by this command, used in development: arm-none-eabi-objcopy -O binary thermostat thermostat.bin

Needed by this command, used in development: `arm-none-eabi-objcopy -O binary thermostat thermostat.bin`
Outdated
Review

Sounds a heavy to install GCC just for objcopy and in and case does not belong in this PR.

Sounds a heavy to install GCC just for objcopy and in and case does not belong in this PR.
] ++ (with python3Packages; [
numpy matplotlib
]);

View File

@ -1,3 +1,4 @@
use core::cmp::max_by;
use heapless::{consts::{U2, U1024}, Vec};
use serde::{Serialize, Serializer};
use smoltcp::time::Instant;
@ -82,6 +83,10 @@ impl Channels {
}
}
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,
@ -461,7 +466,7 @@ impl Channels {
(self.get_pwm(0, PwmPin::Fan) * 100.0) as u32
}
fn report(&mut self, channel: usize, tacho: Option<u32>) -> Report {
fn report(&mut self, channel: usize) -> Report {
let vref = self.channel_state(channel).vref;
let i_set = self.get_i(channel);
let i_tec = self.read_itec(channel);
@ -486,15 +491,14 @@ impl Channels {
tec_i,
tec_u_meas: self.get_tec_v(channel),
pid_output,
tacho,
hwrev: self.hw_rev
}
}
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 reports_json(&mut self, tacho: Option<u32>) -> Result<JsonBuffer, serde_json_core::ser::Error> {
pub fn reports_json(&mut self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
let mut reports = Vec::<_, U2>::new();
for channel in 0..CHANNELS {
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?
let _ = reports.push(self.report(channel, tacho));
let _ = reports.push(self.report(channel));
}
serde_json_core::to_vec(&reports)
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.
}
@ -515,7 +519,6 @@ 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(),
fan: self.get_fan_pwm(),
}
}
@ -553,6 +556,20 @@ impl Channels {
}
serde_json_core::to_vec(&summaries)
}
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: max_by(self.get_tec_i(0).abs().value, self.get_tec_i(1).abs().value, |a, b| a.partial_cmp(b).unwrap())
};
serde_json_core::to_vec(&summary)
} else {
let summary: Option<()> = None;
serde_json_core::to_vec(&summary)
}
}
}
type JsonBuffer = Vec<u8, U1024>;
@ -574,7 +591,6 @@ pub struct Report {
tec_i: ElectricCurrent,
tec_u_meas: ElectricPotential,
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.
pid_output: ElectricCurrent,
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.
tacho: Option<u32>,
hwrev: HWRev,
}
Outdated
Review

f64 really? Now we're talking high precision fan control!

f64 really? Now we're talking high precision fan control!

I think we can go with int range from 1 to 100, since fan seems to not support values less than 0.01

I think we can go with int range from 1 to 100, since fan seems to not support values less than 0.01
@ -615,7 +631,6 @@ pub struct PwmSummary {
max_v: PwmSummaryField<ElectricPotential>,
max_i_pos: PwmSummaryField<ElectricCurrent>,
max_i_neg: PwmSummaryField<ElectricCurrent>,
fan: u32,
}
#[derive(Serialize)]
@ -629,3 +644,10 @@ pub struct SteinhartHartSummary {
channel: usize,
params: steinhart_hart::Parameters,
}
#[derive(Serialize)]
pub struct FanSummary {
fan_pwm: u32,
tacho: u32,
abs_max_tec_i: f64,
}

View File

@ -93,8 +93,8 @@ impl Handler {
Ok(Handler::Handled)
}
fn show_report(socket: &mut TcpSocket, channels: &mut Channels, tacho: Option<u32>) -> Result<Handler, Error> {
match channels.reports_json(tacho) {
fn show_report(socket: &mut TcpSocket, channels: &mut Channels) -> Result<Handler, Error> {
match channels.reports_json() {
Ok(buf) => {
send_line(socket, &buf[..]);
}
@ -344,14 +344,24 @@ impl Handler {
Ok(Handler::Reset)
}
fn fan (channels: &mut Channels, fan_pwm: Option<u32>) -> Result<Handler, Error> {
fn fan (socket: &mut TcpSocket, channels: &mut Channels, fan_pwm: Option<u32>, tacho_value: 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)
match channels.fan_summary(tacho_value) {
Ok(buf) => {
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....
send_line(socket, &buf);
}
Err(e) => {
error!("unable to serialize fan summary: {:?}", e);
Outdated
Review

at your own risk

at your own risk
let _ = writeln!(socket, "{{\"error\":\"{:?}\"}}", e);
return Err(Error::ReportError);
}
};
Ok(Handler::Handled)
}
}
}
@ -361,7 +371,7 @@ impl Handler {
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, tacho_value),
Command::Show(ShowCommand::Input) => Handler::show_report(socket, channels),
Command::Show(ShowCommand::Pid) => Handler::show_pid(socket, channels),
Command::Show(ShowCommand::Pwm) => Handler::show_pwm(socket, channels),
Command::Show(ShowCommand::SteinhartHart) => Handler::show_steinhart_hart(socket, channels),
@ -379,7 +389,7 @@ 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(channels, fan_pwm)
Command::Fan {fan_pwm} => Handler::fan(socket, channels, fan_pwm, tacho_value)
}
}
}

View File

View File

@ -152,7 +152,7 @@ fn main() -> ! {
}
}
let fan_available = channels.hw_rev.major == 2 && channels.hw_rev.minor == 2;
let fan_available = channels.fan_available();
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 {
@ -206,12 +206,9 @@ fn main() -> ! {
let instant = Instant::from_millis(i64::from(timer::now()));
if fan_available {
let mut tacho_input = false;
cortex_m::interrupt::free(|_cs| {
tacho_input = tacho.check_interrupt();
tacho.clear_interrupt_pending_bit();
});
let tacho_input = tacho.check_interrupt();
if tacho_input {
tacho.clear_interrupt_pending_bit();
tacho_cnt += 1;
}
@ -262,7 +259,7 @@ fn main() -> ! {
}
} else if socket.can_send() {
if let Some(channel) = session.is_report_pending() {
match channels.reports_json(tacho_value) {
match channels.reports_json() {
Ok(buf) => {
send_line(&mut socket, &buf[..]);
session.mark_report_sent(channel);