Skip to content

Matter Appliance minor refactor #2181

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

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

Conversation

nickolas-deboom
Copy link
Contributor

Type of Change

  • WWST Certification Request
    • If this is your first time contributing code:
      • I have reviewed the README.md file
      • I have reviewed the CODE_OF_CONDUCT.md file
      • I have signed the CLA
    • I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
  • Bug fix
  • New feature
  • Refactor

Checklist

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have verified my changes by testing with a device or have communicated a plan for testing
  • I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

This change moves some common functionality into common-utils.lua, implements do_configure in the subdrivers rather than the main driver, and also contains some minor fixes involving embedded clusters.

Summary of Completed Tests

Copy link

github-actions bot commented Jun 9, 2025

Copy link

github-actions bot commented Jun 9, 2025

Test Results

   67 files    442 suites   0s ⏱️
2 263 tests 2 263 ✅ 0 💤 0 ❌
3 861 runs  3 861 ✅ 0 💤 0 ❌

Results for commit f30f7a9.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 9, 2025

File Coverage
All files 80%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-microwave-oven/init.lua 82%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-extractor-hood/init.lua 97%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/init.lua 82%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/embedded-cluster-utils.lua 49%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-dishwasher/init.lua 66%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-cook-top/init.lua 95%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-refrigerator/init.lua 85%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-laundry/init.lua 67%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/common-utils.lua 87%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-oven/init.lua 86%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against f30f7a9

Copy link
Contributor

@hcarter-775 hcarter-775 left a comment

Choose a reason for hiding this comment

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

nice job. seems very necessary 👍

@nickolas-deboom nickolas-deboom force-pushed the matter-appliance-minor-subdriver-refactor branch from c7d7de2 to 800f694 Compare June 9, 2025 21:26
@nickolas-deboom
Copy link
Contributor Author

I made an update to move some of the temperature attribute and capability handlers into the main driver and remove them from the subdrivers because these have the same behavior in all cases. I left temperature_setpoint_attr_handler and setpoint_limit_handler defined in the subdrivers because difference min and max values are used in these handlers depending on the device type.

@@ -0,0 +1,187 @@
-- Copyright 2024 SmartThings
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved these test cases into their own file in order to properly test the doConfigure logic in test_init

@hcarter-775
Copy link
Contributor

I made an update to move some of the temperature attribute and capability handlers into the main driver and remove them from the subdrivers because these have the same behavior in all cases. I left temperature_setpoint_attr_handler and setpoint_limit_handler defined in the subdrivers because difference min and max values are used in these handlers depending on the device type.

do you think it would be too much to put them in the main driver but make the min/max be parameters that can be passed in by the actual implementation in the subdriver? Or is that too much you think

This change moves some common functionality into common-utils.lua,
implements do_configure in the subdrivers rather than the main driver,
and also contains some minor fixes involving embedded clusters.
@nickolas-deboom nickolas-deboom force-pushed the matter-appliance-minor-subdriver-refactor branch 2 times, most recently from 5cea2a7 to 69d406c Compare June 18, 2025 19:38
@nickolas-deboom
Copy link
Contributor Author

I made an update to move some of the temperature attribute and capability handlers into the main driver and remove them from the subdrivers because these have the same behavior in all cases. I left temperature_setpoint_attr_handler and setpoint_limit_handler defined in the subdrivers because difference min and max values are used in these handlers depending on the device type.

do you think it would be too much to put them in the main driver but make the min/max be parameters that can be passed in by the actual implementation in the subdriver? Or is that too much you think

In the last commit I attempted this, but ended up leaving the handlers in the subdriver, determine which set of min/max values should be used, and calling a helper function in the common module to do the handling. I don't think there is a way to avoid having the handlers in the subdrivers because the endpoint / component are needed to make the determination on which range to use. Let me know what you think!

@nickolas-deboom nickolas-deboom force-pushed the matter-appliance-minor-subdriver-refactor branch from 69d406c to f30f7a9 Compare June 18, 2025 19:40
@hcarter-775
Copy link
Contributor

works for me, I like it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants