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

PID_autotune #41

Merged
sb10q merged 12 commits from PID_autotune into master 2021-01-06 11:02:52 +08:00
Conversation 1 Commits 12 Files Changed 1
topquark12 commented 2021-01-05 12:47:38 +08:00

Working auto tune PID.

Apologies for the messy commits to the other files and reversing it, should I rebase and squash everything together, or should I leave it as is?

Working auto tune PID. Apologies for the messy commits to the other files and reversing it, should I rebase and squash everything together, or should I leave it as is?
sb10q reviewed 2021-01-05 12:57:52 +08:00
pytec/autotune.py Outdated
@ -0,0 +260,4 @@
tec.set_param("pwm", channel, "i_set" , tunerOut)
except:
pass
sb10q commented 2021-01-05 12:57:52 +08:00
Owner

What is this for? Don't we want to at least log errors here? Otherwise this can lead to confusing behavior.

What is this for? Don't we want to at least log errors here? Otherwise this can lead to confusing behavior.
sb10q commented 2021-01-05 13:13:19 +08:00
Owner

Apologies for the messy commits to the other files and reversing it, should I rebase and squash everything together, or should I leave it as is?

The gitea interface can squash so you can just leave it as it is and add commits.

> Apologies for the messy commits to the other files and reversing it, should I rebase and squash everything together, or should I leave it as is? The gitea interface can squash so you can just leave it as it is and add commits.
sb10q reviewed 2021-01-05 13:20:07 +08:00
pytec/autotune.py Outdated
@ -0,0 +211,4 @@
return True
return False
def init_tuner(self, inputValue, timestamp):
sb10q commented 2021-01-05 13:20:07 +08:00
Owner

the inputValue parameter is not used. Remove.

the inputValue parameter is not used. Remove.
sb10q commented 2021-01-05 13:22:12 +08:00
Owner

Also, since this method is quite similar to __init__, place it below it for easy comparison.

Also, since this method is quite similar to ``__init__``, place it below it for easy comparison.
topquark12 added 1 commit 2021-01-05 17:15:22 +08:00
topquark12 added 1 commit 2021-01-05 17:19:15 +08:00
topquark12 added 1 commit 2021-01-06 09:43:14 +08:00
sb10q reviewed 2021-01-06 10:01:52 +08:00
pytec/autotune.py Outdated
@ -0,0 +255,4 @@
tec.set_param("pwm", channel, "i_set", tuner_out)
except KeyError:
sb10q commented 2021-01-06 10:01:52 +08:00
Owner

Where would a KeyError be acceptable and why?
Can only this one code path be covered by this try/except? Your code also catches and suppresses KeyError raised e.g. by tuner.output() which is most likely not what ought to be done.

Where would a KeyError be acceptable and why? Can only this one code path be covered by this try/except? Your code also catches and suppresses KeyError raised e.g. by tuner.output() which is most likely not what ought to be done.
topquark12 added 1 commit 2021-01-06 10:25:47 +08:00
sb10q reviewed 2021-01-06 10:26:25 +08:00
pytec/autotune.py Outdated
@ -0,0 +245,4 @@
for data in tec.report_mode():
try:
ch = data[channel]
sb10q commented 2021-01-06 10:26:25 +08:00
Owner

remove blank line

remove blank line
sb10q reviewed 2021-01-06 10:26:36 +08:00
pytec/autotune.py
@ -0,0 +246,4 @@
try:
ch = data[channel]
# Sometimes report_mode may yeild empty object
sb10q commented 2021-01-06 10:26:36 +08:00
Owner

When and why?

When and why?
topquark12 commented 2021-01-06 10:28:40 +08:00
Poster

Once every few seconds, haven't dug out the issue in client.py yet.

Once every few seconds, haven't dug out the issue in client.py yet.
sb10q commented 2021-01-06 10:54:31 +08:00
Owner

Ok, the comment should clearly state that it's a workaround then.

Ok, the comment should clearly state that it's a workaround then.
sb10q commented 2021-01-06 10:55:14 +08:00
Owner

And let's file an issue about the client.py behavior

And let's file an issue about the client.py behavior
sb10q reviewed 2021-01-06 10:27:31 +08:00
pytec/autotune.py Outdated
@ -0,0 +248,4 @@
# Sometimes report_mode may yeild empty object
except KeyError:
pass
sb10q commented 2021-01-06 10:27:31 +08:00
Owner

If this happens in the first iteration of the loop, ch won't exist and the code below will crash.

If this happens in the first iteration of the loop, ``ch`` won't exist and the code below will crash.
topquark12 added 1 commit 2021-01-06 10:34:04 +08:00
topquark12 added 1 commit 2021-01-06 10:36:49 +08:00
topquark12 added 1 commit 2021-01-06 11:00:34 +08:00
sb10q merged commit 73dd6d9154 into master 2021-01-06 11:02:52 +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#41
There is no content yet.
Delete Branch "PID_autotune"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?