Add fanSpeedPercent to Thermostat device types #2549
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 477 suites 0s ⏱️ Results for commit f0fd1ec. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against f0fd1ec |
|
Since we're adding the [1,100] gating here for the fan, let's merge the remove "off" for thermostats first. Then everything will be cohesive. |
I agree, that PR is here for reference: #2428 |
|
|
||
| -- remove 'off' as a supported fan mode for thermostat device types, unless the | ||
| -- device previously had 'off' as a supported fan mode to avoid breaking routines | ||
| if get_device_type(device) == THERMOSTAT_DEVICE_TYPE_ID then |
There was a problem hiding this comment.
have you checked if any routines use off? want to make sure this isn't superfluous at this point.
There was a problem hiding this comment.
saw you rebased @nickolas-deboom, but I am still curious about this. Cause honestly even if there are like 2, it might be easier/better not to maintain this and ignore this whole code block
There was a problem hiding this comment.
Not sure about how many use off specifically, but I checked and there are 30 rules existing rules that use fanMode in some way, on two total devices.
Edit: actually there are 89 total rules in place across 31 devices.
| if not tbl_contains(prev_supported_fan_modes, "off") then | ||
| local _, off_idx = tbl_contains(supported_fan_modes, "off") | ||
| if off_idx then | ||
| table.remove(supported_fan_modes, off_idx) | ||
| end | ||
| end |
There was a problem hiding this comment.
I'm starting to wonder, we don't expect this value to change, do we? I almost wonder if it makes sense to just return early if some prev_supported_fan_modes is found, or to just use this table rather than dynamically remove something off the newly made supported_fan_modes table.
However, if we don't go that route, then:
| if not tbl_contains(prev_supported_fan_modes, "off") then | |
| local _, off_idx = tbl_contains(supported_fan_modes, "off") | |
| if off_idx then | |
| table.remove(supported_fan_modes, off_idx) | |
| end | |
| end | |
| -- per the definitions set above, the first index always contains "off" | |
| if prev_supported_fan_modes[1] ~= "off" then | |
| table.remove(supported_fan_modes, 1) | |
| end |
There was a problem hiding this comment.
I see this rule wouldn't work for thermostatFanMode. To solve this, I'd say do the following:
if get_device_type(device) == THERMOSTAT_DEVICE_TYPE_ID and device:supports_capability_by_id(capabilities.fanMode.ID) then
There was a problem hiding this comment.
Updated as suggested!
hcarter-775
left a comment
hcarter-775
left a comment
There was a problem hiding this comment.
Realized I hadn't approved the PR that got merged into this one, so I'm just pulling this out while we sort out those details.
nickolas-deboom
force-pushed
the
add-fan-speed-percent-for-thermostats
branch
from
353f6c0 to
ad29996
Compare
|
@hcarter-775 I rebased this following the directory restructure and also addressed the feedback you left previously |
This PR adds the
fanSpeedPercentcapability for thermostats. These changes were originally in #2425 but were extracted to a separate PR in order to support a faster turnaround, since by itself this is a very low risk change.Note that a range of [1, 100] is enforced because the Off mode is not supported for thermostats.