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

Add support for UWG4 thermostats #200

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

adamjernst
Copy link
Contributor

This is a pretty monolithic PR but it closes #178 by fully supporting WG4-series thermostats.


# WG4-only fields:
temperature: int | None = None
set_point_temperature: int | None = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to stick with a single, data-only Thermostat class instead of introducing subclassing here. Especially since more models could be supported in the future, it seems better for code to branch on "is sensor_mode None?" than "is this thermostat a subclass of XXX?"

"VacationEndDay": _fmt(thermostat.vacation_end_time),
"VacationTemperature": thermostat.vacation_temperature,
},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code previously in update_group_request was moved here.

A JSON object containing the update.
"""
if temperature is not None:
self.resource.temperatures[regulation_mode] = temperature
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Important! Previously, calling set_regulation_mode would actually change the temperatures variable on that thermostat.

This seemed funky to me for a couple reasons: first, this change takes effect even if the subsequent request fails due to auth failure, network unavailability, etc., and it is never rolled back in that case. Second, it feels like the Thermostat object should be a forever-immutable snapshot of a thermostat's state at some point in time instead of a mutable object that changes.

So… I simply removed this from the refactored code.

@@ -31,7 +31,7 @@ python = "^3.9"
yarl = ">=1.6.0"

[tool.poetry.group.dev.dependencies]
aresponses = "^2.1.6"
aresponses = "^3.0.0"
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 silences a warning when running tests, not a huge deal.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dae6cc0) 100.00% compared to head (66ab432) 100.00%.
Report is 146 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #200   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          251       339   +88     
  Branches        36        51   +15     
=========================================
+ Hits           251       339   +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamjernst
Copy link
Contributor Author

@robbinjanssen Here we go! This PR maintains 100% test coverage, hooray. It should be paired with a PR for the Home Assistant integration, which I'll work on next.

min_temperature: int
max_temperature: int
temperatures: dict[int, int]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the temperatures dict in favor of individual fields (comfort_temperature, boost_temperature, etc.).

This felt better as it's closer to the data on the wire and allows the type system to express which temperatures must be present (manual_temperature, comfort_temperature) and which are optional (vacation_temperature is optional since WG4-thermostats don't support it).


# The OWD5 API requires computing the value:
if self.regulation_mode == REGULATION_SCHEDULE and self.schedule:
return Schedule.from_json(self.schedule).get_active_temperature()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the schedule is now lazily parsed in get_target_temperature, and only if needed by the current regulation mode.

I'm open to changing it, but this option seemed simplest.

@robbinjanssen
Copy link
Owner

Awesome! I'll have a look (and will test) tonight!

@adamjernst
Copy link
Contributor Author

As a follow-up, I plan to support subscribing for updates via the push mechanism that the WG4 API offers instead of polling. But that will come in a separate PR.

"""
Fix corrupt date formats used by the WG4 API.

Bizarrely the WG4 API sometimes--but not always--sends dates of the form:
Copy link
Owner

Choose a reason for hiding this comment

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

🤯

Copy link
Owner

@robbinjanssen robbinjanssen left a comment

Choose a reason for hiding this comment

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

Very nice work! I'll test it and see if everything is still working on my end :-)

@robbinjanssen robbinjanssen added the breaking-change A breaking change for existing users. label Feb 5, 2024
@robbinjanssen robbinjanssen merged commit 1a9ad2a into robbinjanssen:main Feb 5, 2024
37 of 38 checks passed
@adamjernst adamjernst deleted the wg4 branch February 5, 2024 21:09
@github-actions github-actions bot locked and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change A breaking change for existing users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UWG4/AWG4 support?
3 participants