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

Closes #69

fan
{"fan_pwm":1,"tacho":0,"abs_max_tec_i":4.0000000000000036e-2,"auto_mode":true}
fan 1
fan
{"fan_pwm":1,"tacho":0,"abs_max_tec_i":3.0000000000000027e-2,"auto_mode":false}
fan 0
fan
{"fan_pwm":1,"tacho":0,"abs_max_tec_i":4.0000000000000036e-2,"auto_mode":true}
Closes #69 ``` fan {"fan_pwm":1,"tacho":0,"abs_max_tec_i":4.0000000000000036e-2,"auto_mode":true} fan 1 fan {"fan_pwm":1,"tacho":0,"abs_max_tec_i":3.0000000000000027e-2,"auto_mode":false} fan 0 fan {"fan_pwm":1,"tacho":0,"abs_max_tec_i":4.0000000000000036e-2,"auto_mode":true} ```
esavkin added 1 commit 2022-12-14 17:28:47 +08:00
30350a3651 Draft support fan pwm settings
Signed-off-by: Egor Savkin <es@m-labs.hk>

Such commands are okay for testing and debugging but obviously it should be automated as suggested in the Issue.

Please think about the user scenarios.

Would you want to control your PC's fan manually and check manually that it is spinning all the time?

It's the same here.

Such commands are okay for testing and debugging but obviously it should be automated as suggested in the Issue. Please think about the user scenarios. Would you want to control your PC's fan manually and check manually that it is spinning all the time? It's the same here.
sb10q reviewed 2022-12-14 17:33:09 +08:00
src/channels.rs Outdated
@ -579,2 +593,4 @@
max_i_pos: PwmSummaryField<ElectricCurrent>,
max_i_neg: PwmSummaryField<ElectricCurrent>,
tacho: f64,
fan: f64,

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

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

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
sb10q reviewed 2022-12-14 17:50:51 +08:00
src/channels.rs Outdated
@ -481,6 +493,8 @@ 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: self.get_pwm(0, PwmPin::Tacho),

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

PWM channels cannot measure frequencies. That's not going to work.
sb10q reviewed 2022-12-14 17:51:16 +08:00
src/pins.rs Outdated
@ -315,0 +322,4 @@
fan.into_alternate(),
);
let (mut tacho, mut fan) = Timer::new(tim8, &clocks).pwm(channels, freq);
init_pwm_pin(&mut tacho);

No.

No.
esavkin added 1 commit 2022-12-15 16:48:07 +08:00
4223f7a4ad Draft tacho support
Signed-off-by: Egor Savkin <es@m-labs.hk>
Poster
Owner

TODO:

  1. chip temperature measure
  2. fan adjustments tied to the chip temperature
  3. respect hardware revision
  4. clean up the code
  5. ???
TODO: 1. chip temperature measure 2. fan adjustments tied to the chip temperature 3. respect hardware revision 4. clean up the code 5. ???
sb10q reviewed 2022-12-15 22:30:23 +08:00
src/main.rs Outdated
@ -185,2 +194,4 @@
let instant = Instant::from_millis(i64::from(timer::now()));
let tacho_input = tacho.check_interrupt();
tacho.clear_interrupt_pending_bit();

Classic mistake.

Classic mistake.
Poster
Owner

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.

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?
Poster
Owner

Added it to critical section

Added it to critical section

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.
Poster
Owner

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.
sb10q reviewed 2022-12-15 22:31:11 +08:00
src/main.rs Outdated
@ -187,0 +199,4 @@
tacho_cnt += 1;
}
let epoch = instant.millis >> 10;

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.
sb10q reviewed 2022-12-15 22:32:05 +08:00
src/main.rs Outdated
@ -172,1 +174,4 @@
tacho.make_interrupt_source(&mut dp.SYSCFG.constrain());
tacho.trigger_on_edge(&mut dp.EXTI, Edge::Rising);
tacho.enable_interrupt(&mut dp.EXTI);

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.
sb10q reviewed 2022-12-15 22:38:26 +08:00
src/channels.rs Outdated
@ -474,3 +482,3 @@
}
fn pwm_summary(&mut self, channel: usize) -> PwmSummary {
fn pwm_summary(&mut self, channel: usize, tacho: u32) -> PwmSummary {

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

tacho isn't a pwm. Remove from this function or rename.
sb10q reviewed 2022-12-15 22:39:36 +08:00
src/channels.rs Outdated
@ -488,3 +498,3 @@
let mut summaries = Vec::<_, U2>::new();
for channel in 0..CHANNELS {
let _ = summaries.push(self.pwm_summary(channel));
let _ = summaries.push(self.pwm_summary(channel, tacho));

Why does tacho need to appear twice?

Why does tacho need to appear twice?
Poster
Owner

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

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

Can you just put it somewhere else than that array then?
sb10q reviewed 2022-12-15 22:40:12 +08:00
src/channels.rs Outdated
@ -342,12 +342,16 @@ impl Channels {
get(&self.pwm.max_i_neg0),
(0, PwmPin::MaxV) =>
get(&self.pwm.max_v0),
(0, PwmPin::Fan) =>

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.
esavkin force-pushed 69-fan_pwm from 2fe55f7544 to d117c784d9 2022-12-19 17:15:42 +08:00 Compare
esavkin added 1 commit 2022-12-20 14:46:30 +08:00
583d06a78b Make fan cmd show the stats
Signed-off-by: Egor Savkin <es@m-labs.hk>
esavkin changed title from WIP: Draft support fan PWM settings to Draft support fan PWM settings 2022-12-20 14:46:43 +08:00
esavkin changed title from Draft support fan PWM settings to Support fan PWM settings 2022-12-20 14:46:57 +08:00
esavkin force-pushed 69-fan_pwm from 5fbc604ee9 to f05e48c8c9 2022-12-20 15:48:59 +08:00 Compare
esavkin force-pushed 69-fan_pwm from e704afe3af to 5d20f2ca0f 2022-12-20 15:55:04 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 5d20f2ca0f to fbea69d3c6 2022-12-20 16:13:42 +08:00 Compare
esavkin force-pushed 69-fan_pwm from fbea69d3c6 to 957a8ae074 2022-12-20 16:36:54 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 957a8ae074 to 3100cb66cf 2022-12-20 16:48:27 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 3100cb66cf to 66143d2373 2022-12-21 15:00:00 +08:00 Compare
sb10q reviewed 2022-12-21 16:17:04 +08:00
@ -272,0 +275,4 @@
## Fan control
Fan control is available for the thermostat revisions with integrated fan system. For this purpose two 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

Why is it so approximate?

Why is it so approximate?
Poster
Owner

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.
sb10q marked this conversation as resolved
sb10q reviewed 2022-12-21 16:17:26 +08:00
flake.nix Outdated
@ -70,3 +70,3 @@
rustPlatform.rust.rustc
rustPlatform.rust.cargo
openocd dfu-util
openocd dfu-util gcc-arm-embedded-10

Why?

Why?
Poster
Owner

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`

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.
sb10q reviewed 2022-12-21 16:17:58 +08:00
README.md Outdated
@ -272,0 +279,4 @@
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,
`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.

Doesn't the motor stall at low powers?

Doesn't the motor stall at low powers?
Poster
Owner

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.
sb10q reviewed 2022-12-21 16:19:15 +08:00
src/channels.rs Outdated
@ -24,3 +26,4 @@
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

schematics

schematics
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-21 16:21:08 +08:00
src/channels.rs Outdated
@ -337,2 +366,4 @@
(_, PwmPin::ISet) =>
panic!("i_set is no pwm pin"),
(_, PwmPin::Fan) =>
get(&self.pwm.fan),

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.
Poster
Owner

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.
sb10q reviewed 2022-12-21 16:21:34 +08:00
src/channels.rs Outdated
@ -454,6 +500,7 @@ impl Channels {
tec_i,
tec_u_meas: self.get_tec_v(channel),
pid_output,
hwrev: self.hw_rev

Call it hwrev or hw_rev but not both.

Call it hwrev or hw_rev but not both.
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-21 16:27:52 +08:00
src/channels.rs Outdated
@ -63,6 +77,21 @@ impl Channels {
channels
}
fn detect_hw_rev(hwrev_pins: &HWRevPins) -> HWRev {

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()``
Poster
Owner

Moved to the separate file

Moved to the separate file
sb10q reviewed 2022-12-21 16:29:32 +08:00
src/channels.rs Outdated
@ -521,0 +589,4 @@
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;

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?
Poster
Owner

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.
sb10q reviewed 2022-12-21 16:31:29 +08:00
src/channels.rs Outdated
@ -521,0 +590,4 @@
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,

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.
Poster
Owner

Introduce a,b,c of quadratic equation coefficients?

Introduce a,b,c of quadratic equation coefficients?

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

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

c would add ability to make it nearly linear

`c` would add ability to make it nearly linear
Poster
Owner

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.

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

I doubt we need a GUI for this, the hardware is fixed.
sb10q reviewed 2022-12-21 16:33:06 +08:00
src/main.rs Outdated
@ -173,0 +181,4 @@
// 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.

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.
Poster
Owner

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.

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.
Poster
Owner

Oscilloscope shows similar values

Oscilloscope shows similar values

Simply reporting the tacho value to the user is not very useful.
You should instead detect a failed fan by observing unexpected tacho values for a few seconds, and then set both TEC currents to zero and lock down the device (with appropriate error reports and manual override option).

Simply reporting the tacho value to the user is not very useful. You should instead detect a failed fan by observing unexpected tacho values for a few seconds, and then set both TEC currents to zero and lock down the device (with appropriate error reports and manual override option).
esavkin added 1 commit 2022-12-22 17:28:20 +08:00
58650d37f1 Refactor and coefficients implemented
Move all the fan logic to the separate file. Add controls for controlling curve.

Signed-off-by: Egor Savkin <es@m-labs.hk>
esavkin added 1 commit 2022-12-23 13:13:43 +08:00
ea2eb51b27 Add fan warnings
Signed-off-by: Egor Savkin <es@m-labs.hk>
esavkin added 1 commit 2022-12-23 13:23:29 +08:00
630635486e Polish minor issues
Signed-off-by: Egor Savkin <es@m-labs.hk>
esavkin requested review from sb10q 2022-12-23 16:19:08 +08:00
sb10q reviewed 2022-12-27 17:31:53 +08:00
@ -345,0 +366,4 @@
Ok(Handler::Handled)
}
fn fan_coeff (socket: &mut TcpSocket, fan_ctrl: &mut FanCtrl, k_a: f64, k_b: f64, k_c: f64) -> Result<Handler, Error> {

Remove space before ( (also in the other functions)

Remove space before ( (also in the other functions)
sb10q reviewed 2022-12-27 17:33:35 +08:00
README.md Outdated
@ -272,0 +283,4 @@
`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`,

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?
sb10q reviewed 2022-12-27 17:35:15 +08:00
@ -523,0 +567,4 @@
))(input)?;
let result = match coeffs {
Some(coeffs) => Ok(Command::FanCoeff { k_a: coeffs.0, k_b: coeffs.1, k_c: coeffs.2 }),

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

Call it "curve" or "coeff" but not both.
sb10q reviewed 2022-12-27 17:36:13 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +273,4 @@
FanStatus::OK => "Fan is OK".as_bytes(),
FanStatus::NotAvailable => "Fan is not available".as_bytes(),
FanStatus::Stalled => "Fan is stalled".as_bytes(),
FanStatus::LowSignal => "Fan is low signal".as_bytes(),

What does "fan is low signal" mean?

What does "fan is low signal" mean?
Poster
Owner

That there are some signals being registered, but in general it so low so that it can be zero on some small timeline. It tipically happens on very low fan speed (from 1 to 3 included), and that behavior is shown on oscilloscope.

That there are some signals being registered, but in general it so low so that it can be zero on some small timeline. It tipically happens on very low fan speed (from 1 to 3 included), and that behavior is shown on oscilloscope.

Maybe just put a minimum fan speed to remove this edge case. I doubt running the fan at a couple rotations per second (which is what I guess it spins at when this happens) has much effect on cooling anyway.

Maybe just put a minimum fan speed to remove this edge case. I doubt running the fan at a couple rotations per second (which is what I guess it spins at when this happens) has much effect on cooling anyway.

And "low signal" is very confusing and not the proper term to use, it sounds like you are receiving some kind of analog transmission being drowned in noise.
Moot but you should have said something about the tacho frequency being low.

And "low signal" is very confusing and not the proper term to use, it sounds like you are receiving some kind of analog transmission being drowned in noise. Moot but you should have said something about the tacho frequency being low.
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-27 17:39:07 +08:00
src/main.rs Outdated
@ -151,1 +153,4 @@
}
// 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());

Highly suspicious. I doubt either mutex or unsafe is necessary.

Highly suspicious. I doubt either mutex or unsafe is necessary.
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-27 17:42:40 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +186,4 @@
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.

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

"since the interrupt is still masked in cortex_m::peripheral::NVIC"
sb10q reviewed 2022-12-27 17:43:05 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +191,4 @@
// 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.

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."
sb10q reviewed 2022-12-27 17:46:11 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +190,4 @@
// 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.

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.
Poster
Owner

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.

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.
sb10q reviewed 2022-12-27 17:47:45 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +230,4 @@
fn diagnose(&mut self) -> FanStatus {
if self.past_record & 0b11 == 0b11 {
FanStatus::OK
} else if self.past_record & 0xAAAAAAAAAAAAAAAA > 0 {

???????

???????
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-27 17:48:41 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +201,4 @@
fn add_record(&mut self, value: u32) {
self.past_record = self.past_record << 2;
if value >= TACHO_LOW_THRESHOLD {
self.past_record += 0b11;

|=

|=
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-27 17:49:11 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +199,4 @@
#[inline]
fn add_record(&mut self, value: u32) {
self.past_record = self.past_record << 2;

Just use [bool; 32] or Vec<bool>

Just use ``[bool; 32]`` or ``Vec<bool>``

Or some custom enum instead of bool.

Or some custom enum instead of bool.
esavkin marked this conversation as resolved
sb10q reviewed 2022-12-27 17:51:55 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +29,4 @@
const TACHO_LOW_THRESHOLD: u32 = 100;
const DEFAULT_K_A: f64 = 1.0;
const DEFAULT_K_B: f64 = 0.0;
const DEFAULT_K_C: f64 = 0.04;

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?
Poster
Owner

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
sb10q reviewed 2022-12-27 17:54:28 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +200,4 @@
#[inline]
fn add_record(&mut self, value: u32) {
self.past_record = self.past_record << 2;
if value >= TACHO_LOW_THRESHOLD {

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.

As I said before the device should enter a failsafe mode with output currents at zero when a fan failure is detected.

As I said before the device should enter a failsafe mode with output currents at zero when a fan failure is detected.
sb10q refused to review 2022-12-27 17:55:53 +08:00
Poster
Owner

Mentioned regression comparison:

Mentioned regression comparison:
Poster
Owner

As I said before the device should enter a failsafe mode with output currents at zero when a fan failure is detected.

Next time

> As I said before the device should enter a failsafe mode with output currents at zero when a fan failure is detected. Next time

Mentioned regression comparison:

What does the graph represent? The axes aren't even labeled and don't have units...

> Mentioned regression comparison: What does the graph represent? The axes aren't even labeled and don't have units...
Poster
Owner

What does the graph represent? The axes aren't even labeled and don't have units...

As mentioned in the code comments, it's a comparison of qudratic and log regressions against real 80% tacho values measured for each user input fan power. Here's the actual data (second sheet - values after scaling).

> What does the graph represent? The axes aren't even labeled and don't have units... As mentioned in the code comments, it's a comparison of qudratic and log regressions against real 80% tacho values measured for each user input fan power. Here's the actual data (second sheet - values after scaling).
sb10q reviewed 2023-01-05 11:35:03 +08:00
@ -164,3 +165,3 @@
}
fn show_ipv4 (socket: &mut TcpSocket, ipv4_config: &mut Ipv4Config) -> Result<Handler, Error> {
fn show_ipv4(socket: &mut TcpSocket, ipv4_config: &mut Ipv4Config) -> Result<Handler, Error> {

Unrelated style changes go to separate PR, as I have repeated at least four times.

Unrelated style changes go to separate PR, as I have repeated at least four times.
sb10q reviewed 2023-01-05 11:38:24 +08:00
@ -345,0 +345,4 @@
fn fan(socket: &mut TcpSocket, fan_pwm: Option<u32>, fan_ctrl: &mut FanCtrl) -> Result<Handler, Error> {
match fan_pwm {
Some(val) => {
fan_ctrl.set_auto_mode(val == 0);

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.
esavkin force-pushed 69-fan_pwm from bd18b5fa3a to 6551a86f8a 2023-01-05 13:05:39 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 6551a86f8a to 73ecffcf1e 2023-01-05 14:54:06 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 73ecffcf1e to 6ab3a6423f 2023-01-05 14:57:14 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 6ab3a6423f to 80b5a8257c 2023-01-05 15:12:56 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 80b5a8257c to a645bfb6e8 2023-01-05 15:30:19 +08:00 Compare
sb10q reviewed 2023-01-06 07:34:49 +08:00
README.md Outdated
@ -272,0 +279,4 @@
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`.
2. `fan auto` - enable auto speed controller mode, which correlates with the square of the TEC's current.

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

"correlates with the square of the TEC's current" is incorrect
sb10q reviewed 2023-01-06 07:36:24 +08:00
README.md Outdated
@ -272,0 +285,4 @@
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`,
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.
5. `fcurve default` - restore fan curve settings to defaults: `a = 1.0, b = 0.0, c = 0.0`.

Those defaults are naive.

Those defaults are naive.
Poster
Owner

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

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.
sb10q reviewed 2023-01-06 07:37:41 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +17,4 @@
const MAX_USER_FAN_PWM: f64 = 100.0;
const MIN_USER_FAN_PWM: f64 = 1.0;
const MAX_FAN_PWM: f64 = 1.0;
// below this value, motor pulse signal is too weak

"weak" suggests amplitude, which is incorrect.
-> "frequency is too low"

"weak" suggests amplitude, which is incorrect. -> "frequency is too low"
Poster
Owner

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.

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?
Poster
Owner

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
sb10q reviewed 2023-01-06 07:38:51 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +96,4 @@
self.fan_auto = fan_auto;
}
#[inline]

Not sure if [inline] here and in several other functions is relevant.

Not sure if ``[inline]`` here and in several other functions is relevant.
Poster
Owner

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

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.
sb10q reviewed 2023-01-06 07:41:51 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +125,4 @@
fn get_pwm(&self) -> u32 {
let duty = self.fan.get_duty();
let max = self.fan.get_max_duty();
(Self::scale_number(duty as f64 / (max as f64), MIN_USER_FAN_PWM, MAX_USER_FAN_PWM, MIN_FAN_PWM, MAX_FAN_PWM) + 0.5) as u32

.round() not +0.5

.round() not +0.5
sb10q reviewed 2023-01-06 07:42:48 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +161,4 @@
use super::*;
#[test]
fn test_scaler() {

Doesn't seem relevant

Doesn't seem relevant
sb10q reviewed 2023-01-06 07:49:06 +08:00
src/main.rs Outdated
@ -179,3 +181,3 @@
let mut new_ipv4_config = None;
let instant = Instant::from_millis(i64::from(timer::now()));
let updated_channel = channels.poll_adc(instant);
let updated_channel = fan_ctrl.channels.poll_adc(instant);

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?
Poster
Owner

indeed

indeed
esavkin force-pushed 69-fan_pwm from 16dc8ec298 to e6d928ef4e 2023-01-10 15:15:31 +08:00 Compare
esavkin added 1 commit 2023-02-02 10:52:49 +08:00
069e178966 Change PWM freq to 25kHz
Signed-off-by: Egor Savkin <es@m-labs.hk>
Poster
Owner

Full-load with both channels tests with resistors showed that there would be only 20C difference against room temperature even with minimum fan speed available. For full speed fan it would be 12-15C diff. For fan stalled it would be 40C diff after ~5 minutes, increasing slowly furthermore. I think auto mode in this PR would be quite safe against board's overheating.

Full-load with both channels tests with resistors showed that there would be only 20C difference against room temperature even with minimum fan speed available. For full speed fan it would be 12-15C diff. For fan stalled it would be 40C diff after ~5 minutes, increasing slowly furthermore. I think auto mode in this PR would be quite safe against board's overheating.
esavkin added 1 commit 2023-02-07 12:05:56 +08:00
83d5c28a67 Disable fan auto mode by default for Thermostat v2.2
Signed-off-by: Egor Savkin <es@m-labs.hk>
sb10q reviewed 2023-02-08 19:31:45 +08:00
src/channels.rs Outdated
@ -521,3 +532,3 @@
}
type JsonBuffer = Vec<u8, U1024>;
pub type JsonBuffer = Vec<u8, U1024>;

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``.
sb10q reviewed 2023-02-08 19:33:23 +08:00
src/channels.rs Outdated
@ -454,6 +458,7 @@ impl Channels {
tec_i,
tec_u_meas: self.get_tec_v(channel),
pid_output,
hwrev: self.hwrev

Why does this need to be in Channels?

Why does this need to be in ``Channels``?
sb10q reviewed 2023-02-08 19:36:17 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +55,4 @@
fan,
available,
// do not enable auto mode by default,
// but allow to turn it on on customer's own risk

at user's own risk

at user's own risk
sb10q reviewed 2023-02-08 19:37:16 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +145,4 @@
self.major == 2 && self.minor == 2
}
pub fn fan_auto_mode_available(&self) -> bool {

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.
sb10q reviewed 2023-02-08 19:41:44 +08:00
src/pins.rs Outdated
@ -217,2 +228,3 @@
(pins, leds, eeprom, eth_pins, usb)
// According to `SUNON DC Brushless Fan & Blower(255-E)` catalogue p.36-37
// Model name: MF35101V1-1000U-G99

G99 is not a PWM fan.

G99 is not a PWM fan.
sb10q reviewed 2023-02-08 19:42:48 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +129,4 @@
}
}
impl HWRev {

Move to separate file. This is not intrinsically linked to the fan.

Move to separate file. This is not intrinsically linked to the fan.
sb10q reviewed 2023-02-08 19:46:41 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +13,4 @@
pub type FanPin = PwmChannels<TIM8, pwm::C4>;
// as stated in the schematics
const MAX_TEC_I: f64 = 3.0;

f32 is probably plenty enough for all this fan stuff.

f32 is probably plenty enough for all this fan stuff.
sb10q reviewed 2023-02-08 19:49:29 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +47,4 @@
let available = hwrev.fan_available();
if available {
fan.set_duty(0);

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?
Poster
Owner

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
esavkin added 1 commit 2023-02-16 13:37:25 +08:00
sb10q reviewed 2023-03-15 13:09:01 +08:00
@ -345,0 +352,4 @@
if fan_ctrl.is_default_auto() {
send_line(socket, b"{}");
} else {
send_line(socket, b"{ \"warning\": \"this fan doesn't have full PWM support. Use it on your own risk!\" }");

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....
sb10q reviewed 2023-03-15 13:11:10 +08:00
@ -0,0 +28,4 @@
self.major == 2 && self.minor == 2
}
pub fn fan_default_auto(&self) -> bool {

fan_pwm_recommended?

``fan_pwm_recommended``?
sb10q reviewed 2023-03-15 13:11:24 +08:00
@ -0,0 +36,4 @@
pub fn summary(&self) -> Result<JsonBuffer, serde_json_core::ser::Error> {
serde_json_core::to_vec(&self)

blank line

blank line
sb10q reviewed 2023-03-15 13:12:36 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +18,4 @@
const MAX_USER_FAN_PWM: f32 = 100.0;
const MIN_USER_FAN_PWM: f32 = 1.0;
const MAX_FAN_PWM: f32 = 1.0;
// below this value motor's autostart feature may fail

According to ?

According to ?
Poster
Owner

Internal experiments

Internal experiments

Please say so in the code comments.

Please say so in the code comments.

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.
Poster
Owner

Made this (and others) value coming from hwrev

Made this (and others) value coming from hwrev
sb10q reviewed 2023-03-15 13:12:50 +08:00
@ -0,0 +23,4 @@
const DEFAULT_K_A: f32 = 1.0;
const DEFAULT_K_B: f32 = 0.0;
const DEFAULT_K_C: f32 = 0.0;

Those naive values will need proper testing and determination at some point.

Those naive values will need proper testing and determination at some point.
sb10q reviewed 2023-03-15 13:14:18 +08:00
@ -0,0 +63,4 @@
pub fn cycle(&mut self, abs_max_tec_i: f32) {
self.abs_max_tec_i = abs_max_tec_i;
self.adjust_speed();

inline

inline
sb10q reviewed 2023-03-15 13:14:39 +08:00
@ -0,0 +83,4 @@
}
}
pub fn adjust_speed(&mut self) {

this doesn't need to be pub

this doesn't need to be ``pub``
sb10q reviewed 2023-03-15 13:15:30 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +122,4 @@
self.default_auto
}
fn scale_number(unscaled: f32, to_min: f32, to_max: f32, from_min: f32, from_max: f32) -> f32 {

this does not need to be a class member

this does not need to be a class member
sb10q reviewed 2023-03-15 13:16:50 +08:00
src/pins.rs Outdated
@ -218,1 +229,3 @@
(pins, leds, eeprom, eth_pins, usb)
// According to `SUNON DC Brushless Fan & Blower(255-E)` catalogue p.36-37
// model MF35101V1-1000U-G99 doesn't have a PWM wire, so it is advised to have
// higher frequency to have less audible noise.

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.

And what "higher frequency" are you talking about?

And what "higher frequency" are you talking about?
esavkin force-pushed 69-fan_pwm from a6bbd36493 to 003abc6e98 2023-03-22 12:05:46 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 003abc6e98 to a5aea551bf 2023-03-22 12:19:03 +08:00 Compare
esavkin force-pushed 69-fan_pwm from a5aea551bf to 721617106e 2023-03-22 12:32:13 +08:00 Compare
esavkin force-pushed 69-fan_pwm from 721617106e to d550fefedf 2023-03-22 13:05:21 +08:00 Compare
sb10q reviewed 2023-03-22 16:49:50 +08:00
src/fan_ctrl.rs Outdated
@ -0,0 +35,4 @@
let mut fan_ctrl = FanCtrl {
fan,
// do not enable auto mode by default,
// but allow to turn it on on user's own risk

at the user's own risk

at the user's own risk
esavkin force-pushed 69-fan_pwm from d550fefedf to 5afb1663cd 2023-03-22 17:10:49 +08:00 Compare
sb10q reviewed 2023-03-22 17:12:14 +08:00
@ -345,0 +356,4 @@
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 on your own risk!\" }");

at your own risk

at your own risk
sb10q reviewed 2023-03-22 17:12:27 +08:00
@ -345,0 +384,4 @@
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 on your own risk!\" }");

same

same
esavkin force-pushed 69-fan_pwm from 5afb1663cd to afdab2f025 2023-03-22 17:14:36 +08:00 Compare
sb10q merged commit 570c0324b3 into master 2023-03-22 17:15:49 +08:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/thermostat#73
There is no content yet.