Add fanSpeedPercent to Thermostat device types #2549
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 468 suites 0s ⏱️ Results for commit 353f6c0. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 353f6c0 |
|
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 |
…2428) 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.
|
|
||
| -- 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.
| 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
| local event = supported_fan_modes_attribute(supported_fan_modes, {visibility = {displayed = false}}) | ||
| device:emit_event_for_endpoint(ib.endpoint_id, event) |
There was a problem hiding this comment.
| local event = supported_fan_modes_attribute(supported_fan_modes, {visibility = {displayed = false}}) | |
| device:emit_event_for_endpoint(ib.endpoint_id, event) | |
| device:emit_event_for_endpoint(ib.endpoint_id, supported_fan_modes_attribute(supported_fan_modes, {visibility = {displayed = false}})) |
nit
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.
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.