-
Notifications
You must be signed in to change notification settings - Fork 510
Don't include Off for supportedFanModes for thermostats by default #2428
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
base: main
Are you sure you want to change the base?
Conversation
Invitation URL: |
Test Results 71 files 452 suites 0s ⏱️ Results for commit 264f055. ♻️ This comment has been updated with latest results. |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 264f055 |
Off is an optional enum value for SystemMode and setting fanMode to Off may have no effect if the thermostat mode is set to heating or cooling. This commit removes this value from supportedFanModes attribute unless Off is reported by the SystemMode attribute at some point.
9a12e9c
to
264f055
Compare
device:set_field(OPTIONAL_THERMOSTAT_MODES_SEEN, {capabilities.thermostatMode.thermostatMode.off.NAME}, {persist=true}) | ||
end | ||
local supported_modes = utils.deep_copy(device:get_field(OPTIONAL_THERMOSTAT_MODES_SEEN)) | ||
local supported_modes = utils.deep_copy(device:get_field(OPTIONAL_THERMOSTAT_MODES_SEEN) or {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to the logic in line 1475, this will literally never be empty, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, good point 😅
local supportedFanModes, supported_fan_modes_attribute | ||
if ib.data.value == clusters.FanControl.attributes.FanModeSequence.OFF_LOW_MED_HIGH then | ||
supportedFanModes = { "off", "low", "medium", "high" } | ||
supportedFanModes = { "low", "medium", "high" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean we'll be deleting options from currently existing thermostat devices with this capability? What are the numbers on this- specifically routines using off for thermostats? Assume it's very low (maybe 0), but still worth checking. Otherwise this looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point, I will try to figure out how many there are if any before proceeding. If there are a lot of existing rules we could opt to only implement this change for new devices
This change removes Off as a default value from supportedFanModes attribute for the Thermostat device type.