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

Include support for a bad wind support mode mapping #1916

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

Conversation

hcarter-775
Copy link
Contributor

@hcarter-775 hcarter-775 commented Feb 7, 2025

Description of Change

There are some devices that claim to support the Wind feature, but whose WindSupport attribute is empty. This update will force a read of that attribute before profiling can occur in the case that the Wind feature is supported to ensure that at least one value is specified. If no value is specified, wind support is not included in the resulting profile.

There are a few paths this logic can follow, so here is the reasoning for each path:

  1. do_configure runs first -> checks if battery is supported
  2. if battery is supported -> we read the PowerSource AttributeList.
    a. In attribute_list_handler -> match_profile runs with the specified battery type
    b. In match_profile -> if the wind feature is not supported, a profile is chosen.
    c. Else -> the battery type specified is saved. we read FanControl WindSupport. match_profile returns early
    1. In wind_support_handler -> we count the supported modes. a capability event is emitted. match_profile runs again.
    2. In match_profile -> a profile is chosen.
  3. if battery is not supported -> match_profile runs directly from do_configure.
    a. See b. c. i. and ii. above for the following logic.

Re-formation of the previous PR 1577

Summary of Completed Tests

  • Unit tests written to match the failing case, and previous tests were re-written to handle the new profiling system.
  • Tested on-device with an empty WindSupport attribute, and it correctly profiled not to include it

Copy link

github-actions bot commented Feb 7, 2025

Copy link

github-actions bot commented Feb 7, 2025

Test Results

   64 files  ±0    409 suites  ±0   0s ⏱️ ±0s
2 033 tests +1  2 033 ✅ +1  0 💤 ±0  0 ❌ ±0 
3 512 runs  +1  3 512 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit fb42613. ± Comparison against base commit 78c235c.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Feb 7, 2025

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

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against fb42613

@nickolas-deboom
Copy link
Contributor

These changes look pretty straightforward. I was trying to trace through the logic for the following sequence, does this look right to you? I think this matches scenario (2) you provided in the PR description

  1. match_profile will be called for the first time from either do_configure or the power source attribute list handler, with WIND_MODE_COUNT nil for the first time
  2. A read for WindSupport is sent
  3. wind_support_handler runs, sets the WIND_MODE_COUNT field, and calls match_profile for the second time
  4. This time the WIND_MODE_COUNT field is populated, so match_profile proceeds past the wind support stuff this time
  5. create_fan_profile is called, and now the WIND_MODE_COUNT field is populated so it will update the profile name to include wind support if needed.

@hcarter-775 hcarter-775 force-pushed the fix/support-bad-wind-mode branch from 07e556c to fb42613 Compare February 10, 2025 22:30
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