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

Dishwasher support for smartthinq platform. #26

Merged
merged 2 commits into from
Aug 14, 2019
Merged

Dishwasher support for smartthinq platform. #26

merged 2 commits into from
Aug 14, 2019

Conversation

dermotduffy
Copy link
Contributor

An opening bid at dishwasher support using the recently merged wideq dishwasher functionality.

Uses a similar scheme that @wkd8176 uses in their KR version (and borrows some inspiration and init code -- thanks!) in creating a new 'smartthinq' platform that is independently configured from the existing Smartthinq 'climate' component. I didn't want to mess with climate.py in this PR, but I'd recommend (with your permission) a followup PR to convert climate to use the new toplevel smartthinq platform, and perhaps deprecate independently specifying the token (etc) in the climate component -- as this style is likely more flexible for additional devices/components down the line.

@sampsyo
Copy link
Owner

sampsyo commented Aug 14, 2019

Looks pretty good! I see nothing to object to here. And I'd totally be up for a refactor with a top-level SmartThinQ platform configuration if you'd be interested in tackling that.

I can't test this because I don't have an LG dishwasher, but if you're confident in it, we can go ahead and merge!

@dermotduffy
Copy link
Contributor Author

Excellent news!

Yes, whilst I only have 1 dishwasher to test with, it's definitely working and stable for me. For a visual look at what can be done with this component, take a look at my current UI using this (scroll down in the README).

So if you're happy, lets merge this!

Outstanding work:

  • A few small tweaks to better deal with API oddities (e.g. in certain conditions the dishwasher will report 0:01 remaining indefinitely, which we can detect and handle better). I'd rather get this larger change in first.
  • The small climate.py micro-refactor I mention above.
  • A README.md update.

Question for you:

  • In my climate.py update would you rather I go for maximal code cleanliness and hard break existing users of climate.py (by forcing them to update configuration.yaml in order to restore functionality), or a softer deprecation that persists functionality with the existing configurations people presumably have -- but also works the new way (but incurs small amounts of double code). By default, I'll go with the softer deprecate-and-print-warnings approach.

Thanks for the continued prompt review!

@sampsyo
Copy link
Owner

sampsyo commented Aug 14, 2019

Lovely! I'll merge it now.

Yeah, I'd recommend the soft deprecation version. It seems like it won't be too hard to support the "old way" for a while at least.

@sampsyo sampsyo merged commit 714b8e2 into sampsyo:master Aug 14, 2019
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