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.

Sign up for GitHub

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

Jump to bottom

IOTE-4566 Limit temperature setpoint ranges #1678

Merged
merged 9 commits into from
Oct 21, 2024
Merged

IOTE-4566 Limit temperature setpoint ranges #1678

merged 9 commits into from
Oct 21, 2024

Conversation

nickolas-deboom
Copy link
Contributor

@nickolas-deboom nickolas-deboom commented Oct 10, 2024 •
edited
Loading

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

IOTE-4566

Clamp the min and max values of temperature setpoint capabilities so that the ranges of deg C and deg F don't overlap. This is currently being done for the oven device type and now should be extended to other devices types using the temperatureSetpoint, thermostatHeatingSetpoint, or thermostatCoolingSetpoint capabilities (or the associated Setpoint Range capabilities). These changes are necessary because the driver is not aware of the temperature unit when a setpoint value is updated via a capability command. Threshold values were chosen depending on the device type as follows:

Dishwasher:

  • Threshold: 90 degrees
  • Range: 33C - 90C / 91.4F - 194F

Laundry Dryer:

  • Threshold: 80 degrees
  • Range: 27C - 80C / 80.6F - 176F

Laundry Washer:

  • Threshold: 55 degrees
  • Range: 13C - 55C / 55.4F - 131F

Refrigerator:

  • Threshold: 20 degrees
  • Range: -6C - 20C / 21.2F - 68F

Thermostat / Room AC:

  • Threshold: 40 degrees
  • Range: 5C - 40C / 41F - 104F

Please let me know if you have any opinions on better values to use.

Summary of Completed Tests

See new unit tests.

Testing with VDA:

  • For each device, verify the correct range is present in both deg C and F, and that attempting to exceed the bounds in either direction results in the app displaying "Out of range". Note that the range that displays in the ST app will not necessarily be the max range defined in the drivers.
  • Dishwasher *
    • On VDA v1.3.4, this device crashes a few seconds after onboarding and the device appears as offline in the ST app.
      • This is fixed in v1.3.5.
    • The plugin only allows the setpoint value to go up to 100 *F, even though the range displays correctly as 91.4 *F - 194.0 *F. The '+' becomes grayed out and the setpoint cannot be increased further. The slider works correctly in *C. This issue is tracked by MTR-842.
  • Laundry Dryer *
  • Laundry Washer *
  • Refrigerator *
    • On VDA v1.3.4, this device crashes a few seconds after onboarding and the device appears as offline in the ST app.
    • This is fixed in v1.3.5.
  • Thermostat / Room AC
    • VDA Thermostat device sends Null values for the MinMeasuredValue and MaxMeasuredValue attributes of the temperatureMeasurement cluster. The temperature range displays as the plugin default of -20 to 50 *C, but this functionality is covered by unit tests.

* For these device types, the plugin is not correctly limiting the setpoint sliders from exceeding the allotted range. The minimum and maximum allowed values appear as expected, but the user is able to go beyond the range using the '-' and '+' buttons, which are not grayed out when the slider is at the min or max as they should be. Also, the slider can sometimes be moved beyond the range with sliding as well. The thermostat device type does not have this issue. This issue is tracked by MTR-841.

@nickolas-deboom
Limit temperature setpoint ranges
6b00cf9 
Clamp the limit values of temperature setpoint capabilities so that the
ranges of deg C and deg F don't overlap.
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 10, 2024 •
edited
Loading

Channel deleted.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 10, 2024 •
edited
Loading

Test Results

   64 files    399 suites   0s ⏱️
1 986 tests 1 986 ✅ 0 💤 0 ❌
3 427 runs  3 427 ✅ 0 💤 0 ❌

Results for commit ebbdbb7.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 10, 2024 •
edited
Loading

matter-appliance_coverage.xml

File Coverage
All files 72% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-dishwasher/init.lua 68% ❌
/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 98% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-oven/init.lua 85% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/embedded-cluster-utils.lua 45% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/init.lua 56% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-cook-top/init.lua 85% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-refrigerator/init.lua 65% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-laundry/init.lua 69% ❌

matter-thermostat_coverage.xml

File Coverage
All files 83% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-dishwasher/init.lua 68% ❌
/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 98% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-oven/init.lua 85% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/embedded-cluster-utils.lua 45% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/init.lua 56% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-cook-top/init.lua 85% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-refrigerator/init.lua 65% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-appliance/src/matter-laundry/init.lua 69% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/init.lua 85% ❌
/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 ebbdbb7

@LEE-HYOJUNG27
Copy link

LEE-HYOJUNG27 commented Oct 11, 2024 •
edited
Loading

I think you need to modify the temperature range for each device types. Because Celsius and Fahrenheit written in the description do not actually match and can cause conflict.

[Example of conflict]

Laundry Dryer:
Threshold: 80 degrees
Range: 26C - 80C / 81F - 178F

-> Range: 26C - 80C / 78.8F - 176F
-> Conflict range : 78.8F - 80F

[Recalculated range]

Dishwasher:
Threshold: 90 degrees
Range: 33C - 90C / 91.4F - 194F

Laundry Dryer:
Threshold: 80 degrees
Range: 27C - 80C / 80.6F - 176F

Laundry Washer:
Threshold: 55 degrees
Range: 13C - 55C / 55.4F - 131F

Refrigerator:
Threshold: 20 degrees
Range: -6C - 20C / 21.2F - 68F

Thermostat / Room AC:
Threshold: 39 degrees
Range: 5C - 39C / 41F - 102.2F

cc: @HunsupJung

@nickolas-deboom nickolas-deboom changed the title Limit temperature setpoint ranges IOTE-4566 Limit temperature setpoint ranges Oct 11, 2024
@nickolas-deboom
fixup: fix failing unit tests
296d81b
@nickolas-deboom
fixup: address review comments
b6c2b08 
* use updated range values
* add handling for freezer range in matter-refrigerator
* other minor changes
@nickolas-deboom
Copy link
Contributor Author

I think you need to modify the temperature range for each device types. Because Celsius and Fahrenheit written in the description do not actually match and can cause conflict.

[Example of conflict]

Laundry Dryer: Threshold: 80 degrees Range: 26C - 80C / 81F - 178F

-> Range: 26C - 80C / 78.8F - 176F -> Conflict range : 78.8F - 80F

[Recalculated range]

Dishwasher: Threshold: 90 degrees Range: 33C - 90C / 91.4F - 194F

Laundry Dryer: Threshold: 80 degrees Range: 27C - 80C / 80.6F - 176F

Laundry Washer: Threshold: 55 degrees Range: 13C - 55C / 55.4F - 131F

Refrigerator: Threshold: 20 degrees Range: -6C - 20C / 21.2F - 68F

Thermostat / Room AC: Threshold: 39 degrees Range: 5C - 39C / 41F - 102.2F

cc: @HunsupJung

I applied these new ranges in b6c2b08!

@nickolas-deboom nickolas-deboom mentioned this pull request Oct 11, 2024
Closed
12 tasks
ctowns
ctowns approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@ctowns ctowns left a comment

Choose a reason for hiding this comment

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

Left a small comment for a potential code improvement, but this looks good to me!

@nickolas-deboom nickolas-deboom force-pushed the bugfix/fix-temp-setpoint-handling branch from 2426409 to 182d860 Compare October 18, 2024 20:49
@nickolas-deboom nickolas-deboom merged commit c043a3e into main Oct 21, 2024
12 checks passed
@nickolas-deboom nickolas-deboom deleted the bugfix/fix-temp-setpoint-handling branch October 21, 2024 14:50
@dljsjr dljsjr mentioned this pull request Oct 21, 2024
Merged
@HunsupJung HunsupJung mentioned this pull request Oct 22, 2024
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hcarter-775 hcarter-775 hcarter-775 approved these changes

@HunsupJung HunsupJung HunsupJung approved these changes

@ctowns ctowns ctowns approved these changes

@tpmanley tpmanley Awaiting requested review from tpmanley

@z-michel z-michel Awaiting requested review from z-michel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nickolas-deboom @LEE-HYOJUNG27 @ctowns @HunsupJung @hcarter-775