Skip to content
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

Feature/matter remove cluster element list usage #370

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

Conversation

cjswedes
Copy link
Contributor

@cjswedes cjswedes commented Nov 9, 2022

This is needed since the next library release (0.46.x) will completely deprecate and remove some of the opts functionality of the MatterDevice:get_endpoints(cluster_id, opts).

These changes also discovered a unit test bug that will be fixed in 0.46.x where require "version" would fail when run with the unit test framework.

These changes also discovered a bug to be fixed in 0.46.x where InteractionRequest:pretty_print generates a nil index error when used with a manufacturer extended cluster element that has been added to an existing cluster library cluster object via Attribute:set_parent_cluster()

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

Unit Test Results

       1 files     305 suites   0s ⏱️
1 327 tests 1 324 ✔️ 0 💤 0  3 🔥
2 383 runs  2 380 ✔️ 0 💤 0  3 🔥

For more details on these errors, see this check.

Results for commit 3a04636.

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

@cjswedes
Copy link
Contributor Author

cjswedes commented Nov 9, 2022

I added the do not merge label since this will break unit tests (due to the second bug mentioned) until 0.46.x release occurs. There is not an easy hack around that bug in the unit test framework, also waiting will allow the removal of the other unit test framework hack required to make the matter-thermostat tests pass.

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 would push to switch over to using the latest dev artifact, but I worry that would lead to us accidentally pushing changes to prod that will break on production hubs. Maybe I could implement a system that uses tags to determine which artifact to run the tests with?

@cjswedes
Copy link
Contributor Author

I would push to switch over to using the latest dev artifact, but I worry that would lead to us accidentally pushing changes to prod that will break on production hubs.

I would worry this too, and think that we cannot do this. We could do a beta artifact release that comes before users actually have the artifact on their hubs, and we could run tests against the beta artifact and the most recent production release. We could even do it without doing any extra release of artifacts, but would need to clearly define what test failures are a blocker for release.

@github-actions
Copy link

github-actions bot commented Dec 20, 2022

Test Results

     54 files     345 suites   0s ⏱️
1 625 tests 1 625 ✔️ 0 💤 0
2 843 runs  2 843 ✔️ 0 💤 0

Results for commit 8180329.

♻️ This comment has been updated with latest results.

@cjswedes cjswedes force-pushed the feature/matter-remove-cluster-element-list-usage branch 2 times, most recently from 985ba27 to 3df0388 Compare December 20, 2022 21:58
@CLAassistant
Copy link

CLAassistant commented May 31, 2023

CLA assistant check
All committers have signed the CLA.

@cjswedes cjswedes force-pushed the feature/matter-remove-cluster-element-list-usage branch from 3df0388 to 4039ffa Compare October 23, 2023 21:51
@github-actions
Copy link

github-actions bot commented Oct 23, 2023

File Coverage
All files 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/init.lua 93%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 8180329

@cjswedes cjswedes force-pushed the feature/matter-remove-cluster-element-list-usage branch from 4039ffa to 7c876f1 Compare October 23, 2023 21:55
@cjswedes
Copy link
Contributor Author

@tpmanley @greens @ctowns This is ready for a review. It is essentially adding the support for thermostats that have ThermostatOperatingState capability, which is based off the optional attribute ThermostatRunningState. I believe all hubs have FW 0.46 or greater, so various workarounds have been removed and it seems to work with the more recent profile changes for nobattery.

@@ -415,6 +428,7 @@ local matter_driver_template = {
[clusters.Thermostat.attributes.AbsMinCoolSetpointLimit.ID] = setpoint_limit_handler(setpoint_limit_device_field.MIN_COOL),
[clusters.Thermostat.attributes.AbsMaxCoolSetpointLimit.ID] = setpoint_limit_handler(setpoint_limit_device_field.MAX_COOL),
[clusters.Thermostat.attributes.MinSetpointDeadBand.ID] = min_deadband_limit_handler,
[clusters.Thermostat.attributes.AttributeList.ID] = attr_list_handler
Copy link
Contributor

Choose a reason for hiding this comment

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

what triggers a read here? the subscription?

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.

3 participants