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.

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

ICP-7174 ICP-7282 ICP-7283 ICP-7308 ICP-7309 ICP-7288 ICP-7289 Misc Stelpro fixes #3750

Merged
merged 14 commits into from Jan 3, 2019

Conversation

dkirker
Copy link
Contributor

@dkirker dkirker commented Dec 6, 2018

ICP-7288 Change rounding method for temperature conversion
ICP-7308 ICP-7309 Tweaks to operating state logic (Not a full resolution)
ICP-7282 ICP-7283 Add heatingSetpoint temp unit
ICP-7174 Update Stelpro Ki logic errors

And 48eaf2b might fix part of ICP-7289

@@ -141,6 +141,7 @@ def setupHealthCheck() {

def configureSupportedRanges() {
sendEvent(name: "supportedThermostatModes", value: supportedThermostatModes, displayed: false)
// These are part of the deprecated Thermostat capability. Remove these when that capability is removed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am checking to see if OneApp uses this in anyway. It didn't seem like its presence affected the range displayed in OneApp, but I want to make sure it isn't needed because of the reference to "Thermostat" in the capabilities.

This applies to all three device handlers.

tpmanley
tpmanley previously approved these changes Dec 6, 2018
@stelpro
Copy link

stelpro commented Dec 13, 2018

Donald,

Don't forget to add a check on the temperature to report a freezing alarm when the temperature is below 0°C

@dkirker
Copy link
Contributor Author

dkirker commented Dec 14, 2018

@stelpro I did some tweaking to re-organize the handleTemperature logic.

@stelpro
Copy link

stelpro commented Dec 14, 2018

@dkirker that sounds good

greens
greens previously approved these changes Dec 14, 2018
Copy link
Contributor

@greens greens left a comment

Choose a reason for hiding this comment

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

Looks good, but wait until all the bugs get confirmed first.

@dkirker
Copy link
Contributor Author

dkirker commented Dec 19, 2018

@greens Made one addition to the zigbee DTH to work around that bug. If Poland checks out, then we can merge.

Track raw temperature and raw setpoint values from the thermostat and compare those
@dkirker
Copy link
Contributor Author

dkirker commented Dec 20, 2018

@greens Alright, so the logic has changed a bit. Depending on what cert/QA says, this is probably the best we can do. I considered the route of managing the operating state ourselves, but I think that would add a bit more logic that I don't want to consider. This should at least make sure we are getting sane values, and that we have received the latest values.

@dkirker dkirker requested a review from rappleg December 31, 2018 09:21
@dkirker
Copy link
Contributor Author

dkirker commented Dec 31, 2018

@rappleg or @tpmanley or @varzac Any chance I can get a +1 from any of you?
@workingmonk If we want to target this for a 1/8 deploy to production, would I need to rebase this to staging, or will there be a push from master to staging before?

tpmanley
tpmanley previously approved these changes Dec 31, 2018
@greens
Copy link
Contributor

greens commented Jan 2, 2019

looks like you have merge conflicts now @dkirker

@dkirker
Copy link
Contributor Author

dkirker commented Jan 2, 2019

Yeah. Looks like a weather API change. Minor. Still trying to figure out the release schedule for next week. It looks like there was already a rolling down and up of branches. But waiting for Vinay to confirm on chat.

@dkirker dkirker changed the base branch from master to staging January 3, 2019 02:00
@dkirker
Copy link
Contributor Author

dkirker commented Jan 3, 2019

Alright, rebased to staging, and fixed indentation changes made from another commit.

@tpmanley tpmanley merged commit 253bec3 into SmartThingsCommunity:staging Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants