Skip to content

CHAD 12650 Z-Wave: Update base thermostat driver to use dynamic numeric constraints for heating/cooling setpoints #1344

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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

matthewdehaven
Copy link
Contributor

Uses Z-Wave ThermostatSetpoint:CapabilitiesGet to request the thermostat setpoints for heating and cooling on initialization.

This file is intended to contain code for dynamically retrieving setpoint constraints from zwave thermostats. The initial contents are copied from a zigbee bulb color bounds script with a similar purpose.
Do not explicitly use scale and precision. Rely on existing conversion utilities behind the scenes.
and fix an issue identified by this new test
They are not guaranteed to be in celsius
To account for the case a different controller on the network inquires about an unsupported setpoint
@CLAassistant
Copy link

CLAassistant commented Apr 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

Copy link

github-actions bot commented Apr 25, 2024

Test Results

   57 files  ±0    364 suites  ±0   0s ⏱️ ±0s
1 759 tests +2  1 759 ✅ +2  0 💤 ±0  0 ❌ ±0 
3 053 runs  ±0  3 053 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 825a7e4. ± Comparison against base commit 4022386.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Apr 25, 2024

File Coverage
All files 90%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/stelpro-ki-thermostat/init.lua 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/fibaro-heat-controller/init.lua 78%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/ct100-thermostat/init.lua 94%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/popp-radiator-thermostat/init.lua 85%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/init.lua 99%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/thermostat-heating-battery/init.lua 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/aeotec-radiator-thermostat/init.lua 78%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zwave-thermostat/src/apiv6_bugfix/init.lua 69%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 825a7e4

Copy link
Contributor

@cbaumler cbaumler left a comment

Choose a reason for hiding this comment

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

Nice work!

@@ -37,6 +48,10 @@ local function device_added(driver, device)
device:is_cc_supported(cc.THERMOSTAT_FAN_MODE) then
device:send(ThermostatFanMode:SupportedGet({}))
end
if device_supports_thermostat_setpoint(device) then
device:send(ThermostatSetpointV3:CapabilitiesGet({setpoint_type = ThermostatSetpoint.setpoint_type.HEATING_1}))
device:send(ThermostatSetpointV3:CapabilitiesGet({setpoint_type = ThermostatSetpoint.setpoint_type.COOLING_1}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why device_supports_thermostat_setpoint returns true if one or the other of the heating and cooling setpoint capabilities is supported, but here we are getting both. Would it make sense to only get what is supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good point. We do have some heat-only thermostats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out, I didn't think of that case. I adjusted this to be two conditions: Check for support for each heating and cooling setpoints independently before sending the request for the corresponding bounds.

Check the heating and cooling setpoint capabilities independently before sending each check instead of sending requests for both if either is supported. Some thermostats may support only one of the two.
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.

I was just thinking about this. I think we might want to have some sanity check values to gate these reports. We have seen devices report garbage on similar reports. You also may want to do some testing with a real device.

@matthewdehaven
Copy link
Contributor Author

I was just thinking about this. I think we might want to have some sanity check values to gate these reports. We have seen devices report garbage on similar reports. You also may want to do some testing with a real device.

Okay, yes it is my intention to test with a real device. I'm working on getting access to one.

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.

4 participants