-
Notifications
You must be signed in to change notification settings - Fork 484
Matter Thermostat: Fix issues found during testing #2099
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?
Matter Thermostat: Fix issues found during testing #2099
Conversation
Invitation URL: |
Test Results 66 files 426 suites 0s ⏱️ Results for commit 57ed44b. ♻️ This comment has been updated with latest results. |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 57ed44b |
Duplicate profile check: Passed - no duplicate profiles detected. |
69c96a8
to
4167e8c
Compare
Have you tested this on older FW? Also, since the changes to filterStatus are 1.4 changes, we need to change the lowest allowed lua libs version from the 1.2 release to the 1.4 release version. |
I added test cases to cover lua libs API < 10, which allows the embedded clusters to be utilized. Also as discussed the filter reset command change was on the capabilities side rather than in the spec. |
Add an optional argument to component_to_endpoint to specify a cluster ID. The previous implementation checked for the presence of airPurifierFanMode within this function, which fails to account for situations where a device supports both the FanControl and Thermostat clusters on different endpoints.
f00fbac
to
57ed44b
Compare
@@ -35,14 +35,6 @@ components: | |||
version: 1 | |||
- id: tvocHealthConcern | |||
version: 1 | |||
- id: thermostatOperatingState |
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.
Is this meant to be included in this PR?
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.
we could pull it into a separate one, but yes, this is also part of the improvements found by testing the same device
@@ -1686,6 +1681,15 @@ local function set_water_heater_mode(driver, device, cmd) | |||
end | |||
end | |||
|
|||
local function reset_filter_state(driver, device, cmd) |
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.
Adding this handler is a good idea, but is this meant to be included in this PR or a separate PR? I would either move this to it's own PR or make it clear in the PR description/title that this PR is doing more than just improving component-to-endpoint mapping.
Similar to the thermostat operating state being removed from the profile above.
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.
I will update the title & description, this PR was meant to address all of the issues found during testing.
Type of Change
Checklist
Description of Change
This PR addresses three issues found during testing with an air purifier device:
Summary of Completed Tests
Tested on air purifier device