M-Labs/thermostat
9
0
Fork 5
Code Issues 9 Pull Requests 12 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 +265
topquark12 commented 2021-01-05 12:47:38 +08:00
Contributor
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

When and why?

When and why?
topquark12 commented 2021-01-06 10:28:40 +08:00
Author
Contributor
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
No description provided.
Delete Branch "PID_autotune"

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?