SmartThingsEdgeDrivers icon indicating copy to clipboard operation
SmartThingsEdgeDrivers copied to clipboard

Include support for a bad wind support mode mapping

Open hcarter-775 opened this issue 10 months ago • 4 comments

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

hcarter-775 avatar Feb 07 '25 18:02 hcarter-775

Channel deleted.

github-actions[bot] avatar Feb 07 '25 18:02 github-actions[bot]

Test Results

   64 files  ± 0    410 suites  +1   0s ⏱️ ±0s 2 068 tests +36  2 051 ✅ +19  0 💤 ±0  14 ❌ +14  3 🔥 +3  3 547 runs  +36  3 529 ✅ +18  0 💤 ±0  15 ❌ +15  3 🔥 +3 

For more details on these failures and errors, see this check.

Results for commit e47e16f8. ± Comparison against base commit 78c235c4.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Feb 07 '25 18:02 github-actions[bot]

File Coverage
All files 72% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/init.lua 74% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/embedded-cluster-utils.lua 42% :x:

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against e47e16f810cd7e7d42b0e609853be463c40d9b30

github-actions[bot] avatar Feb 07 '25 18:02 github-actions[bot]

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.

nickolas-deboom avatar Feb 10 '25 20:02 nickolas-deboom

Will not fix: Closing this ticket

hcarter-775 avatar Mar 31 '25 14:03 hcarter-775