Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set default threshold to self.THRESHOLD_TEMP #89

Merged
merged 1 commit into from
Oct 15, 2018
Merged

Set default threshold to self.THRESHOLD_TEMP #89

merged 1 commit into from
Oct 15, 2018

Conversation

mkesper
Copy link

@mkesper mkesper commented Oct 5, 2018

If no custom threshold is set we need something to compare to.

This was referenced Oct 5, 2018
Closed
Closed
@amanusk
Copy link
Owner

amanusk commented Oct 6, 2018 •
edited
Loading

The value of temp_thresh is currently used for 2 things, triggering a hook that invokes a script, and for some "eye candy" which changes the temperature graph to red.
By default, this is chosen to be the value of high returned by psutil.sensors_temperatures(). It is possible for this value to be None if it is not available.
Currently, some places use THRESHOLD_TEMP, which is set to 80 degrees.
This is an arbitrary value, but it is better if it is consistent.

amanusk
amanusk reviewed Oct 6, 2018
View reviewed changes
s_tui/Sources/TemperatureSource.py Outdated
@@ -51,6 +51,8 @@ def __init__(self, custom_temp=None, temp_thresh=None):
self.temp_thresh = int(temp_thresh)
logging.debug("Updated custom threshold to " +
str(self.temp_thresh))
else:
self.temp_thresh = 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change to self.THRESHOLD_TEMP to be consistent.

s_tui/Sources/TemperatureSource.py Outdated
@@ -51,6 +51,8 @@ def __init__(self, custom_temp=None, temp_thresh=None):
self.temp_thresh = int(temp_thresh)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are at it, its a good idea to address the case of ValueError for this conversion, and set to THRESHOLD_TEMP if it fails

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a new commit that reflects all this.

@mkesper mkesper force-pushed the temp_threshold_zero branch from e95e4fb to 4079dee Compare October 7, 2018 19:17
Handle errors regarding temperature threshold
c6f590b 
If no custom threshold is set we need something to compare to.
If the custom threshold is not a valid value we need to catch ValueError.
If the custom threshold is lower than zero we need to set some value, too.
For all these cases, we use self.THRESHOLD_TEMP as value for temp_thresh.

This fixes issue amanusk#87.
@mkesper mkesper force-pushed the temp_threshold_zero branch from 4079dee to c6f590b Compare October 7, 2018 19:25
@mkesper
Copy link
Author

mkesper commented Oct 7, 2018 •
edited
Loading

New commit, combining possible errors.
Fixed whitespace...

@mkesper mkesper changed the title Set default threshold to zero Set default threshold to self.THRESHOLD_TEMP Oct 8, 2018
@amanusk
Copy link
Owner

amanusk commented Oct 14, 2018

Sorry for the long delay. This looks good. I will review and approve this shortly.

@amanusk amanusk mentioned this pull request Oct 14, 2018
Closed
@amanusk amanusk merged commit 265840b into amanusk:master Oct 15, 2018
@amanusk
Copy link
Owner

amanusk commented Oct 15, 2018

@mkesper thank you for you contribution. Now merged.

@amanusk amanusk mentioned this pull request Oct 15, 2018
Closed
@mkesper mkesper deleted the temp_threshold_zero branch October 16, 2018 09:44
@mkesper
Copy link
Author

mkesper commented Oct 16, 2018

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amanusk amanusk amanusk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mkesper @amanusk