M-Labs/thermostat
9
0
Fork 5
Code Issues 9 Pull Requests 5 Releases Wiki Activity

Fix i_set, a user-provided value, to stop varying on VREF measurements #84

Merged
sb10q merged 1 commits from atse/thermostat:fix-i_set into master 2024-08-17 17:37:18 +08:00
Conversation 8 Commits 1 Files Changed 2 +7 -7
atse commented 2023-08-21 13:48:04 +08:00
Contributor
Copy Link

* Stops center point from fluctuating due to repeated measurements of VREF
* Uses a calibrated stable VREF measured on startup instead
* Corresponding changes for consistency

This helps stabilise the value of i_set (the TEC current setpoint set and produced in set_i and get_i), so that it behaves more like a direct key-value store, as it is a user value and not a measured value.

Store the user-provided value i_set separately for later reading, so that i_set does not fluctuate.

~~* Stops center point from fluctuating due to repeated measurements of VREF~~ ~~* Uses a calibrated stable VREF measured on startup instead~~ ~~* Corresponding changes for consistency~~ ~~This helps stabilise the value of `i_set` (the TEC current setpoint set and produced in `set_i` and `get_i`), so that it behaves more like a direct key-value store, as it is a user value and not a measured value.~~ Store the user-provided value i_set separately for later reading, so that i_set does not fluctuate.
sb10q reviewed 2023-08-21 20:49:32 +08:00
src/channels.rs Outdated
@ -377,2 +376,3 @@
pub fn get_tec_i(&mut self, channel: usize) -> ElectricCurrent {
(self.read_itec(channel) - self.read_vref(channel)) / ElectricalResistance::new::<ohm>(0.4)
let vref = self.read_vref(channel);
self.channel_state(channel).vref = vref;
sb10q commented 2023-08-21 20:49:32 +08:00
Owner
Copy Link

A "get" function shouldn't modify state.

A "get" function shouldn't modify state.
atse commented 2023-08-22 10:56:42 +08:00
Author
Contributor
Copy Link

The reports would not have an updated VREF value then. Should we remove VREF reporting? It's an implementation detail anyway.

The reports would not have an updated VREF value then. Should we remove VREF reporting? It's an implementation detail anyway.
sb10q commented 2023-08-22 11:13:35 +08:00
Owner
Copy Link

Sounds like removing VREF reporting is the simplest solution, yes. In any case I don't see why a function called get_tec_i should modify an internal value of vref to be reported later.

Sounds like removing VREF reporting is the simplest solution, yes. In any case I don't see why a function called ``get_tec_i`` should modify an internal value of vref to be reported later.
atse commented 2023-08-22 11:47:37 +08:00
Author
Contributor
Copy Link

Ok, I've dropped that commit. I've opened a separate pull request for removing VREF reporting (#85) as it is a protocol change.

Ok, I've dropped that commit. I've opened a separate pull request for removing VREF reporting (#85) as it is a protocol change.
atse marked this conversation as resolved
atse force-pushed fix-i_set from bdffcd4423 to 7043974bb5 2023-08-22 11:45:46 +08:00 Compare
sb10q commented 2023-08-22 16:02:16 +08:00
Owner
Copy Link

center point from fluctuating due to repeated measurements of VREF

Shouldn't VREF be stable? I don't understand this PR.

> center point from fluctuating due to repeated measurements of VREF Shouldn't VREF be stable? I don't understand this PR.
atse commented 2023-08-22 17:05:17 +08:00
Author
Contributor
Copy Link

Shouldn't VREF be stable? I don't understand this PR.

Well, theoretically it should be at a constant 1.5V, but the actual value, as returned by read_vref, fluctuates a bit around it, being off by up to 11mV at times.

The calibration routine in calibrate_dac_value finds a DAC output that is the closest to an averaged VREF and stores it for later. Since it is only run once on startup, it is constant, and should be used for current setpoint purposes for stability.

What I've done is to refactor get_center to use that calibrated VREF instead of a VREF measured on-the-fly. Since i_set depends on the value of get_center, this results in a stable i_set that behaves like a key-value store.

> Shouldn't VREF be stable? I don't understand this PR. Well, theoretically it should be at a constant 1.5V, but the actual value, as returned by `read_vref`, fluctuates a bit around it, being off by up to 11mV at times. The calibration routine in `calibrate_dac_value` finds a DAC output that is the closest to an averaged VREF and stores it for later. Since it is only run once on startup, it is constant, and should be used for current setpoint purposes for stability. What I've done is to refactor `get_center` to use that calibrated VREF instead of a VREF measured on-the-fly. Since `i_set` depends on the value of `get_center`, this results in a stable `i_set` that behaves like a key-value store.
sb10q commented 2023-08-22 17:05:49 +08:00
Owner
Copy Link

the actual value, as returned by read_vref, fluctuates a bit around it, being off by up to 11mV at times.

Is that what you would expect from reading the datasheets?

> the actual value, as returned by read_vref, fluctuates a bit around it, being off by up to 11mV at times. Is that what you would expect from reading the datasheets?
atse commented 2023-08-22 17:12:46 +08:00
Author
Contributor
Copy Link

Is that what you would expect from reading the datasheets?

Yes, as the MAX1968 datasheet specifies a threshold of ±15mV from 1.5V. When used in the current setpoint calculation it yields significant noise.

> Is that what you would expect from reading the datasheets? Yes, as the MAX1968 datasheet specifies a threshold of ±15mV from 1.5V. When used in the current setpoint calculation it yields significant noise.
atse force-pushed fix-i_set from 7043974bb5 to 9ef1d05a42 2023-08-28 10:45:01 +08:00 Compare
atse commented 2023-08-28 10:46:50 +08:00
Author
Contributor
Copy Link

The latest force-push adds a member to ChannelState that keeps track of the current setpoint i_set directly, avoiding any calculation just to report the user-provided value.

The latest force-push adds a member to ChannelState that keeps track of the current setpoint `i_set` directly, avoiding any calculation just to report the user-provided value.
atse changed title from Refactor center point and VREF measurement for a more stable i_set to WIP: Refactor center point and VREF measurement for a more stable i_set 2023-11-27 12:38:29 +08:00
atse force-pushed fix-i_set from 9ef1d05a42 to 7259caee47 2024-01-26 16:59:17 +08:00 Compare
sb10q commented 2024-01-26 16:59:54 +08:00
Owner
Copy Link

It's not a "refactor" if it changes functionality.

It's not a "refactor" if it changes functionality.
atse changed title from WIP: Refactor center point and VREF measurement for a more stable i_set to Fix excessive VREF measurement for a more stable i_set 2024-01-26 17:02:06 +08:00
atse commented 2024-01-26 17:06:58 +08:00
Author
Contributor
Copy Link

It's not a "refactor" if it changes functionality.

Yes, agreed.

The plottable tec_i values should now be avoiding VREF fluctuations. Also to be honest I'm still struggling with tuning the calibrate_dac_value routine as I believe the vref_meas could be better calibrated still. There's currently still a positive offset that would mean setting i_set to zero would still output ~30mA on the channel.

> It's not a "refactor" if it changes functionality. Yes, agreed. The plottable tec_i values should now be avoiding VREF fluctuations. Also to be honest I'm still struggling with tuning the calibrate_dac_value routine as I believe the `vref_meas` could be better calibrated still. There's currently still a positive offset that would mean setting i_set to zero would still output ~30mA on the channel.
atse force-pushed fix-i_set from 7259caee47 to 76547be90a 2024-02-14 17:28:18 +08:00 Compare
atse commented 2024-02-14 17:33:42 +08:00
Author
Contributor
Copy Link

Ok, I'm splitting all the VREF calibrating and hardware dependent adjusting stuff out of this PR. This PR is now solely focused on fixing the problem of a wildly fluctuating i_set in reports, when it should be a user-provided value.

Ok, I'm splitting all the VREF calibrating and hardware dependent adjusting stuff out of this PR. This PR is now solely focused on fixing the problem of a wildly fluctuating i_set in reports, when it should be a user-provided value.
atse changed title from Fix excessive VREF measurement for a more stable i_set to Fix i_set, a user-provided value, to stop varying on VREF measurements 2024-02-14 17:36:12 +08:00
sb10q merged commit 76547be90a into master 2024-02-20 10:56:32 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#84
No description provided.
Delete Branch "atse/thermostat:fix-i_set"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?