M-Labs/thermostat
9
0
Fork 5
Code Issues 9 Pull Requests 7 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 +39 -22
atse commented 2025-01-06 10:50:14 +08:00
Contributor
Copy Link

This adds:

  • An extra ready PIDAutotuneState and corresponding state transitions
  • A setter for autotune parameters, allowing change after construction
This adds: - An extra `ready` PIDAutotuneState and corresponding state transitions - A setter for autotune parameters, allowing change after construction
atse added 1 commit 2025-01-06 10:50:14 +08:00 
Co-authored-by: topquark12 <aw@m-labs.hk>
esavkin reviewed 2025-01-14 10:28:18 +08:00
pythermostat/pythermostat/autotune.py Outdated
@ -96,7 +112,8 @@ class PIDAutotune:
if (self._state == PIDAutotuneState.STATE_OFF
esavkin commented 2025-01-14 10:28:18 +08:00
Owner
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
Author
Contributor
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
Owner
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
Author
Contributor
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
Author
Contributor
Copy Link

Done in force-push to f4c1dab00f.

Done in force-push to f4c1dab00f.
👍 1
sb10q reviewed 2025-01-15 21:49:11 +08:00
pythermostat/pythermostat/autotune.py Outdated
@ -18,6 +18,7 @@ class PIDAutotuneState(Enum):
STATE_RELAY_STEP_DOWN = 'relay step down'
STATE_SUCCEEDED = 'succeeded'
STATE_FAILED = 'failed'
STATE_READY = "ready"
sb10q commented 2025-01-15 21:49:11 +08:00
Owner
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
Author
Contributor
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
Author
Contributor
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
Author
Contributor
Copy Link

Refactored in force-push to 869922c928.

Refactored in force-push to 869922c928.
atse force-pushed autotune-state from 3246929f69 to 828648ed76 2025-01-20 10:51:46 +08:00 Compare
atse force-pushed autotune-state from 828648ed76 to 869922c928 2025-01-20 11:58:20 +08:00 Compare
atse force-pushed autotune-state from 869922c928 to f4c1dab00f 2025-01-20 16:30:10 +08:00 Compare
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u autotune-state:atse-autotune-state
git checkout atse-autotune-state
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
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#162
No description provided.
Delete Branch "atse/thermostat:autotune-state"

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?