Skip to content

Conversation

ctowns
Copy link
Contributor

@ctowns ctowns commented Apr 18, 2025

Check all that apply

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

Summary of Completed Tests

Added unit tests for common device configs for Air Purifers, Thermostats, and Room ACs
Onboarded VDA devices for each of these device types
Completed test plan as defined in REQ

Copy link

github-actions bot commented Apr 18, 2025

Channel deleted.

Copy link

github-actions bot commented Apr 18, 2025

Test Results

   67 files    439 suites   0s ⏱️
2 244 tests 2 244 ✅ 0 💤 0 ❌
3 836 runs  3 836 ✅ 0 💤 0 ❌

Results for commit c3642d3.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Apr 18, 2025

File Coverage
All files 88%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/embedded-cluster-utils.lua 95%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/init.lua 88%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against c3642d3

Copy link

Duplicate profile check: Passed - no duplicate profiles detected.

@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch 2 times, most recently from 0748e37 to eeda044 Compare May 14, 2025 21:28
@hcarter-775
Copy link
Contributor

I believe some logic may be needed to introduce this value configuration for thermostatOperatingState that we have in the static profiles, where a device might support all 3, or just idle and cooling / idle and heating

    config:
      values:
        - key: "thermostatOperatingState.value"
          enabledValues:
            - idle
            - cooling
            - heating

@ctowns
Copy link
Contributor Author

ctowns commented May 20, 2025

I believe some logic may be needed to introduce this value configuration for thermostatOperatingState that we have in the static profiles, where a device might support all 3, or just idle and cooling / idle and heating

    config:
      values:
        - key: "thermostatOperatingState.value"
          enabledValues:
            - idle
            - cooling
            - heating

This is something that will be addressed by the new supportedValues attributes we added to capabilities right? I don't think we should define config values in modular profiles if we don't have to, because then they are no longer generic.

@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch from e97735c to da7019a Compare May 20, 2025 00:53
@hcarter-775
Copy link
Contributor

I believe some logic may be needed to introduce this value configuration for thermostatOperatingState that we have in the static profiles, where a device might support all 3, or just idle and cooling / idle and heating

    config:
      values:
        - key: "thermostatOperatingState.value"
          enabledValues:
            - idle
            - cooling
            - heating

This is something that will be addressed by the new supportedValues attributes we added to capabilities right? I don't think we should define config values in modular profiles if we don't have to, because then they are no longer generic.

@ctowns we have not added a supported values attribute to this capability yet. Once we do, I agree that this is the way forward, and given this need for it in modular profiles, it should be done soon. In the meantime, should we do the above configure value in the profile anyway? This will at least cut down on the number of values seen, even if it's not completely accurate, and we can remove it when the supported values work is complete.

@hcarter-775
Copy link
Contributor

hcarter-775 commented May 20, 2025

@ctowns sorry to keep going on this topic, but it turns out I did create the supportedOperatingStates attribute for the ThermostatOperatingState capability and completely forgot, so you were totally right initially. It may be good to put any new handling we want to add that utilizes this in this PR

@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch 2 times, most recently from 3da8591 to 6130969 Compare May 22, 2025 16:53
@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch from d328842 to a0b601f Compare May 22, 2025 19:28
@ctowns
Copy link
Contributor Author

ctowns commented May 22, 2025

Rebased, but no merge conflicts.

@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch from 652d0df to dc6c782 Compare May 23, 2025 15:36
@ctowns
Copy link
Contributor Author

ctowns commented May 23, 2025

Squashed and rebased, no merge conflicts.

@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch 2 times, most recently from 429b213 to 724b764 Compare May 23, 2025 15:58
@ctowns ctowns force-pushed the modular-profiles-matter-thermostat branch from 724b764 to c3642d3 Compare May 23, 2025 16:12
@hcarter-775
Copy link
Contributor

Looks good to me! I think we have a couple action items coming out of this PR:

  1. Use supportedValues for the ThermostatOperatingState capability
  2. Re-work the driver to break down some code into separate files / sub-drivers
  3. Include the on/off switch in air purifiers.

Anything else to log for next steps?

@ctowns
Copy link
Contributor Author

ctowns commented May 23, 2025

Looks good to me! I think we have a couple action items coming out of this PR:

1. Use supportedValues for the ThermostatOperatingState capability

2. Re-work the driver to break down some code into separate files / sub-drivers

3. Include the on/off switch in air purifiers.

Anything else to log for next steps?

This looks good to me, thank you for logging this @hcarter-775 !

@ctowns ctowns merged commit 88d5ccc into main May 23, 2025
12 checks passed
@ctowns ctowns deleted the modular-profiles-matter-thermostat branch May 23, 2025 16:47
@ldeora
Copy link

ldeora commented Jun 4, 2025

My thermostat radiator valves (former profile thermostat-heating-only) look like this now:

Screenshot_20250604_131011_SmartThings

Screenshot_20250604_131431_Samsung Internet

Devices with supportedThermostatModes off and heat shouldn't have thermostatCoolingSetpoint.coolingSetpoint, right?

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.

5 participants