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

fix-an-issue-of-thermostat-does-not-support-fan-speed #1813

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

Conversation

pInksenberg
Copy link
Contributor

@pInksenberg pInksenberg commented Dec 10, 2024

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

Problem:
The LengCeoi HAVC Remote were recently tested in ST App, which include Thermostat and Fan device type. However the capabilities in plugin shows incorrectly

Expectation Result:
The capabilities of thermostat, humidity, fan speed, thermostat mode, thermostat fan mode and set temperature show in the ST plugin.

Current Issue:
The plugin show fan mode and fan speed.

Analysis:
Is it found that the edge driver(matter-thermostat) will re-assign a new profile while driver initialize, and go to the Fan logic, the fan logic does not check if the device support thermostat, or humidity, which means it will re-assign the “fan-generic” for driver.

The profile name re-assign logic is changed to check if the fan speed is supported.

Related tickets: https://smartthings.atlassian.net/browse/MTR-866

Summary of Completed Tests

Copy link

Duplicate profile check: Passed - no duplicate profiles detected.

Copy link

Copy link

github-actions bot commented Dec 10, 2024

Test Results

   64 files    403 suites   0s ⏱️
2 006 tests 2 006 ✅ 0 💤 0 ❌
3 468 runs  3 468 ✅ 0 💤 0 ❌

Results for commit e00e92b.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 10, 2024

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

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against e00e92b

@@ -503,7 +507,7 @@ local function do_configure(driver, device)
end
profile_name = profile_name .. create_air_quality_sensor_profile(device)

elseif #thermostat_eps > 0 then
elseif #thermostat_eps > 0 or device_type == TMST_DEVICE_TYPE_ID then
Copy link
Contributor

Choose a reason for hiding this comment

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

the get_device_type logic currently defaults to thermostat when no other device type (i.e. fan, air purifier, room ac) are found. Including this logic could jeopardize that flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is found that the get_device_type function does not handle situation where a device supports both Thermostat and Fan device type. Instead, it defaults to returning the Fan device type and proceeds to the "device_type == FAN_DEVICE_TYPE_ID" logic, which the logic do not check if the device support thermostat capability.

BTW, there is no such a profile support both fan speed and thermostat in thermostat driver folder. Are we currently not supporting a device that combines a fan and a thermostat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is from ticket #1813

Copy link
Contributor Author

@pInksenberg pInksenberg Dec 19, 2024

Choose a reason for hiding this comment

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

I updated my pull request by incorporating thermostat check logic into the "device_type == FAN_DEVICE_TYPE_ID" condition to handle situations where a device supports both fan and thermostat types. Please help to revise, Many thanks~

@pInksenberg pInksenberg force-pushed the fix-an-issue-of-thermostat-does-not-support-fan-speed branch 2 times, most recently from 71929e6 to 18ff916 Compare December 24, 2024 03:56
@pInksenberg pInksenberg force-pushed the fix-an-issue-of-thermostat-does-not-support-fan-speed branch from 18ff916 to 8cb7552 Compare December 24, 2024 03:59
@hcarter-775
Copy link
Contributor

We don't currently support the fan speed capability for thermostat, and that is on purpose. We have internally discussed adding this, but for now we do not plan to add this feature for thermostats.

@hcarter-775
Copy link
Contributor

I believe similar functionality could be achieved by simply reordering the if-else logic in do_configure. Specifically, if we put the elseif device_type == FAN_DEVICE_TYPE_ID then at the bottom of the ordering, then the logic would see this check first elseif #thermostat_eps > 0 then and would profile based on that logic, only going to the FAN_DEVICE_TYPE_ID logic in the case that #thermostat_eps == 0.

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