dac_fix and review #52

Merged
sb10q merged 1 commits from dac_fix into master 2024-08-17 17:37:18 +08:00
Contributor

Contains fix for inconsistent output current behavior, snuck in minor improvements added during code review.

Contains fix for inconsistent output current behavior, snuck in minor improvements added during code review.
sb10q reviewed 2021-01-20 11:49:58 +08:00
README.md Outdated
@ -180,6 +180,10 @@ postfilter rate can be tuned with the `postfilter` command.
- Connect Peliter device 1 to TEC1- and TEC1+.
- The GND pin is for shielding not for sinking Peltier currents.
When using a TEC module with the Thermostat, the Thermostat expects the thermal load (where the thermistor is located) to heat up with a positive software current set point, and cool down with a negative current set point.
Owner

The doc sometimes says "TEC module" and sometimes "Peltier device"; we should use consistent terminology.

The doc sometimes says "TEC module" and sometimes "Peltier device"; we should use consistent terminology.
sb10q reviewed 2021-01-20 11:52:32 +08:00
src/channels.rs Outdated
@ -114,3 +112,2 @@
};
let voltage = self.channel_state(channel).dac_value;
let max = ElectricPotential::new::<volt>(ad5680::MAX_VALUE as f64 / dac_factor);
let max = ElectricPotential::new::<volt>(DAC_OUT_MAX_V);
Owner

Why do we need to return this if it's constant? Check what is using get_dac and see if that abstraction is relevant.

Why do we need to return this if it's constant? Check what is using `get_dac` and see if that abstraction is relevant.
sb10q reviewed 2021-01-20 11:53:12 +08:00
src/channels.rs Outdated
@ -23,2 +23,4 @@
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
pub const DAC_OUT_MAX_V: f64 = 3.0;
Owner

Can't have ElectricPotential constants?

Can't have `ElectricPotential` constants?
Owner

One purpose of the uom crate is to have the type checker verify dimensional homogeneity; that only works if used consistently.

One purpose of the uom crate is to have the type checker verify dimensional homogeneity; that only works if used consistently.
sb10q reviewed 2021-01-20 12:00:22 +08:00
src/channels.rs Outdated
@ -260,7 +256,12 @@ impl Channels {
/// These loops perform a breadth-first search for the DAC setting
/// that will produce a `target_voltage`.
Owner

Comments should explain the STM ADC offset compensation technique.

Comments should explain the STM ADC offset compensation technique.
sb10q reviewed 2021-01-20 12:01:50 +08:00
src/channels.rs Outdated
@ -348,3 +349,2 @@
pub fn get_max_v(&mut self, channel: usize) -> (ElectricPotential, ElectricPotential) {
let vref = self.channel_state(channel).vref;
let max = 4.0 * vref;
let max = 4.0 * ElectricPotential::new::<volt>(3.3);
Owner

Same as above, check if the abstraction is relevant and it makes sense to return a constant.

Same as above, check if the abstraction is relevant and it makes sense to return a constant.
sb10q reviewed 2021-01-20 12:06:44 +08:00
src/channels.rs Outdated
@ -263,1 +259,3 @@
let target_voltage = ElectricPotential::new::<volt>(2.5);
let samples = 50;
let mut target_voltage = ElectricPotential::new::<volt>(0.0);
for _ in (0..samples).rev() {
Owner

.rev()?

`.rev()`?
sb10q reviewed 2021-01-20 12:15:15 +08:00
src/channels.rs Outdated
@ -260,7 +256,12 @@ impl Channels {
/// These loops perform a breadth-first search for the DAC setting
Owner

Do we really need a breadth-first search, or is it sufficient (e.g. fast enough) to simply increment (or decrement depending on the sign of the error) the DAC value until the ADC reads back Vref?

Do we really need a breadth-first search, or is it sufficient (e.g. fast enough) to simply increment (or decrement depending on the sign of the error) the DAC value until the ADC reads back Vref?
Author
Contributor

The time it takes is not noticeable.

The time it takes is not noticeable.
Owner

OK, then a simple increment/decrement is better (simpler) and then the code comments can focus solely on the STM ADC offset.

OK, then a simple increment/decrement is better (simpler) and then the code comments can focus solely on the STM ADC offset.
Author
Contributor

Misunderstood what you said there. I think if the search works and is fast, then probably should leave it there.

Misunderstood what you said there. I think if the search works and is fast, then probably should leave it there.
sb10q reviewed 2021-01-20 16:44:24 +08:00
src/channels.rs Outdated
@ -153,3 +149,2 @@
let i_tec = (voltage - center_point) / (10.0 * r_sense);
let max = (max - center_point) / (10.0 * r_sense);
(i_tec, max)
// let max = (max - center_point) / (10.0 * r_sense);
Owner

Remove?

Remove?
sb10q reviewed 2021-01-20 16:46:03 +08:00
src/channels.rs Outdated
@ -262,0 +256,4 @@
/// The thermostat DAC applies a control voltage signal to the CTLI pin of MAX driver chip to control output current.
/// The CTLI input signal is centered around VREF of the MAX chip. Applying VREF to CTLI sets the output current to 0.
///
/// This calibration routine reads the VREF voltage, and finds a DAC output value that would
Owner

...reads the VREF voltage and the DAC output by using the STM32 ADC...

...reads the VREF voltage and the DAC output by using the STM32 ADC...
sb10q marked this conversation as resolved
astro reviewed 2021-01-22 04:22:31 +08:00
sb10q reviewed 2021-01-22 17:05:25 +08:00
src/main.rs Outdated
@ -1,6 +1,6 @@
#![cfg_attr(not(test), no_std)]
#![cfg_attr(not(test), no_main)]
#![feature(maybe_uninit_extra, maybe_uninit_ref, asm)]
#![feature(maybe_uninit_extra, maybe_uninit_ref, asm, const_fn)]
Owner

We won't need const_fn anymore, right?

We won't need const_fn anymore, right?
sb10q reviewed 2021-01-25 10:31:47 +08:00
src/channel.rs Outdated
@ -1,4 +1,8 @@
use stm32f4xx_hal::hal::digital::v2::OutputPin;
use uom::si::{
f64::{ElectricPotential},
Owner

Braces are not necessary here.

Braces are not necessary here.
topquark12 force-pushed dac_fix from 8971ffc89c to c23ffd70e2 2021-01-25 11:26:43 +08:00 Compare
topquark12 force-pushed dac_fix from c23ffd70e2 to 8ca8c0b31c 2021-01-25 11:51:38 +08:00 Compare
topquark12 force-pushed dac_fix from 8ca8c0b31c to 011433c200 2021-01-25 13:16:02 +08:00 Compare
topquark12 force-pushed dac_fix from 011433c200 to 46136942af 2021-01-25 13:23:24 +08:00 Compare
topquark12 force-pushed dac_fix from 46136942af to 6b9d61737e 2021-01-25 13:52:23 +08:00 Compare
sb10q merged commit 6b9d61737e into master 2021-01-25 16:40:38 +08:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 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#52
No description provided.