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

PyThermostat: Modify PIDAutotune for GUI usage #162

Open
atse wants to merge 3 commits from atse/thermostat:autotune-state into master
pull from: atse/thermostat:autotune-state
merge into: M-Labs:master
Conversation 0 Commits 3 Files Changed 1 +15 -14
1 changed files with 15 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f12562e248 - Show all commits

29
pythermostat/pythermostat/autotune.py
View File

@ -2,7 +2,7 @@ import math
import logging import logging
import time import time
from collections import deque, namedtuple from collections import deque, namedtuple
from enum import Enum from enum import Enum, auto
from pythermostat.client import Client from pythermostat.client import Client
@ -13,11 +13,11 @@ from pythermostat.client import Client
class PIDAutotuneState(Enum): class PIDAutotuneState(Enum):
STATE_OFF = 'off' STATE_OFF = auto()
STATE_RELAY_STEP_UP = 'relay step up' STATE_RELAY_STEP_UP = auto()
STATE_RELAY_STEP_DOWN = 'relay step down' STATE_RELAY_STEP_DOWN = auto()
STATE_SUCCEEDED = 'succeeded' STATE_SUCCEEDED = auto()
STATE_FAILED = 'failed' STATE_FAILED = auto()
sb10q commented 2025-01-15 21:49:11 +08:00
Outdated
Review
Copy Link

Inconsistent string delimiter.

Can't it just use auto() anyway?

Inconsistent string delimiter. Can't it just use ``auto()`` anyway?
atse commented 2025-01-20 10:56:13 +08:00
Outdated
Review
Copy Link

Actually yes, switched to auto() in force-push to 828648ed76.

Then I believe #71 would be unnecessary since the string values aren't used anywhere.

Actually yes, switched to `auto()` in force-push to 828648ed76. Then I believe #71 would be unnecessary since the string values aren't used anywhere.
atse commented 2025-01-20 11:28:21 +08:00
Outdated
Review
Copy Link

Also just realised this highlights the verbosity of the STATE_ prefix.

Also just realised this highlights the verbosity of the `STATE_` prefix.
atse commented 2025-01-20 11:59:02 +08:00
Outdated
Review
Copy Link

Refactored in force-push to 869922c928.

Refactored in force-push to 869922c928.
class PIDAutotune: class PIDAutotune:
@ -94,27 +94,28 @@ class PIDAutotune:
""" """
now = time_input * 1000 now = time_input * 1000
if (self._state == PIDAutotuneState.STATE_OFF if self._state not in {
or self._state == PIDAutotuneState.STATE_SUCCEEDED PIDAutotuneState.STATE_RELAY_STEP_DOWN,
or self._state == PIDAutotuneState.STATE_FAILED): PIDAutotuneState.STATE_RELAY_STEP_UP,
}:
self._state = PIDAutotuneState.STATE_RELAY_STEP_UP self._state = PIDAutotuneState.STATE_RELAY_STEP_UP
self._last_run_timestamp = now self._last_run_timestamp = now
# check input and change relay state if necessary # check input and change relay state if necessary
if (self._state == PIDAutotuneState.STATE_RELAY_STEP_UP if self._state == PIDAutotuneState.STATE_RELAY_STEP_UP
and input_val > self._setpoint + self._noiseband): and input_val > self._setpoint + self._noiseband:
self._state = PIDAutotuneState.STATE_RELAY_STEP_DOWN self._state = PIDAutotuneState.STATE_RELAY_STEP_DOWN
logging.debug('switched state: {0}'.format(self._state)) logging.debug('switched state: {0}'.format(self._state))
logging.debug('input: {0}'.format(input_val)) logging.debug('input: {0}'.format(input_val))
elif (self._state == PIDAutotuneState.STATE_RELAY_STEP_DOWN elif self._state == PIDAutotuneState.STATE_RELAY_STEP_DOWN
and input_val < self._setpoint - self._noiseband): and input_val < self._setpoint - self._noiseband:
self._state = PIDAutotuneState.STATE_RELAY_STEP_UP self._state = PIDAutotuneState.STATE_RELAY_STEP_UP
esavkin commented 2025-01-14 10:28:18 +08:00
Outdated
Review
Copy Link

Isn't there a way to shorten it?

Isn't there a way to shorten it?
atse commented 2025-01-20 10:53:05 +08:00
Outdated
Review
Copy Link

Yep, did it in the latest force-push to 828648ed76.

Yep, did it in the latest force-push to 828648ed76.
👍 1
esavkin commented 2025-01-20 15:36:41 +08:00
Outdated
Review
Copy Link

Wouldn't the negative logic be applicable here? Like not in (STEP_UP, STEP_DOWN).

Wouldn't the negative logic be applicable here? Like `not in (STEP_UP, STEP_DOWN)`.
atse commented 2025-01-20 16:20:43 +08:00
Outdated
Review
Copy Link

Humm, that's true. Was worried about self._state being set to a non-PIDAutotuneState value, but that shouldn't happen under normal circumstances.

Humm, that's true. Was worried about `self._state` being set to a non-`PIDAutotuneState` value, but that shouldn't happen under normal circumstances.
atse commented 2025-01-20 16:30:47 +08:00
Outdated
Review
Copy Link

Done in force-push to f4c1dab00f.

Done in force-push to f4c1dab00f.
👍 1
logging.debug('switched state: {0}'.format(self._state)) logging.debug('switched state: {0}'.format(self._state))
logging.debug('input: {0}'.format(input_val)) logging.debug('input: {0}'.format(input_val))
# set output # set output
if (self._state == PIDAutotuneState.STATE_RELAY_STEP_UP): if self._state == PIDAutotuneState.STATE_RELAY_STEP_UP:
self._output = self._initial_output - self._outputstep self._output = self._initial_output - self._outputstep
elif self._state == PIDAutotuneState.STATE_RELAY_STEP_DOWN: elif self._state == PIDAutotuneState.STATE_RELAY_STEP_DOWN:
self._output = self._initial_output + self._outputstep self._output = self._initial_output + self._outputstep