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

[wip] Adding dryer and washer support #32

Open
wants to merge 112 commits into
base: master
Choose a base branch
from
Open

[wip] Adding dryer and washer support #32

wants to merge 112 commits into from

Conversation

gvdhoven
Copy link

(see descriptions of respective commits for more info)

Gilles van den Hoven added 4 commits October 22, 2019 15:23
added deprecation warning to align it with climate.py
Align function parameters with climate.py
Align dishwasher device init with climate device init (note: untested since i don't have LG dishwashers)
I think this is a typo in the readme (looking at the code and the `Lovelace configuration`)
@sampsyo
Copy link
Owner

sampsyo commented Oct 22, 2019

Looks good to me! Instead of duplicating the setup code across the two modules, however, what do you think about factoring it out into a common, shared module that both import?

@gvdhoven
Copy link
Author

@sampsyo i totally agree, but this is as far as my Python code goes right now. I was even thinking of adding some sort of ToString on the device class which would give back something like "_ID" as value.

Can you maybe merge this already if it doesn't break anything?

@gvdhoven
Copy link
Author

I cleaned up some code:

  • removed PLATFORM_SCHEMA since doesnt seem to be used.
  • this causes us to be able to remove imports
  • made imports in climate.py and dishwasher.py 'generic' and further down 'specific'.

@gvdhoven
Copy link
Author

@sampsyo can you review it until now?

@sampsyo
Copy link
Owner

sampsyo commented Oct 28, 2019

Hi—the import wideq statements are intentionally embedded in functions instead of being at the module level. This makes it possible to load the component even when the library is not installed, which last I checked was a recommendation when writing HA components?

@sampsyo
Copy link
Owner

sampsyo commented Oct 28, 2019

Also, it looks like most of the setup_platform code is still duplicated across the two modules. It would be really great to move this into a utility module so it only has to appear once.

@gvdhoven
Copy link
Author

Yeah i see that. If you can give me an example of how you think it should be implemented that would help (given the fact that i don't have the right background for that). I think HA calls the setup_platform methods, right? maybe add some sort of utility method which can be called then in the setup_platform method of each appliance type?

@sampsyo
Copy link
Owner

sampsyo commented Oct 29, 2019

Sure! I was thinking we could create a new module (i.e., a .py file) alongside these where the duplicated code would live. There would be a function or two containing the setup code that both modules need. Then, both of them would import that module and invoke those functions.

@gvdhoven
Copy link
Author

Working on that right now @sampsyo can you review my latest update which re-adds the imports to the functions?

@sampsyo
Copy link
Owner

sampsyo commented Oct 29, 2019

You're on the right track, but please put those after the docstrings. I recommend you take a look at the "diff view" here on GitHub:
https://github.com/sampsyo/hass-smartthinq/pull/32/files

Where you can see the changes relative to the master branch. You can modify your code so that unnecessary changes don't appear there—meaning that the code is identical to the current master branch except where there's a meaningful change you actually want to make.

@gvdhoven
Copy link
Author

Is my assumption correct that the entity YAML example in the readme; where you say:
entity: sensor.lg_dishwasher_[ID]

would mean that it is the 'sensor.py' platform, with named entity as above?

Because i am thinking in the lines of creating a sensor.py with 1 'setup_platform' call which can load ALL supported entities from SmartThinq and will generate something like:

  • sensor.lg_dishwasher_[ID]
  • sensor.lg_climate_[ID]
  • sensor.lg_dryer_[ID]

Would that be about right? If so; this can be removed from init.py:

SMARTTHINQ_COMPONENTS = [
    'climate',
    'dishwasher',
]

And this can be replaced:

    for component in SMARTTHINQ_COMPONENTS:
        discovery.load_platform(hass, component, DOMAIN, {}, config)

With:

    discovery.load_platform(hass, 'sensor', DOMAIN, {}, config)

Correct?

TIA!

@gvdhoven
Copy link
Author

would i even need to call the discovery.load_platform at all? versus just calling add_devices immediately from within the __init__.py script?

Conflicted with 'LGDevice' in main class.
@sampsyo
Copy link
Owner

sampsyo commented Oct 29, 2019

Hmm; correct me if I'm wrong, but creating everything using the sensor component would be undesirable, right? For example, a climate device (thermostat) shouldn't be misclassified as a sensor.

It is also nice to have separate Python modules for the different kinds of devices—putting everything together into one big sensor.py could make the code harder to read/navigate.

To be honest, I am not an expert on the HomeAssistant API. (And I didn't even write the dishwasher part of this module…) This page about multiple platforms in a single integration may be relevant:
https://developers.home-assistant.io/docs/en/creating_component_generic_discovery.html

And the people on the HA Discourse may have better answers than I can to general questions about structuring the code.

Gilles van den Hoven added 2 commits October 29, 2019 14:29
The Smartthinq components to load must match the supported HA components; did not know that :-(
@gvdhoven
Copy link
Author

gvdhoven commented Nov 12, 2019

btw, @sampsyo reason for my enormous amount of commits was that i had to go via my repo before i could go into my HA install; sorry for that, which basically meant i had to commit a lot of failing code in between... :-(

(a) Remove try..catch from lookup_bit since a KeyError should not be able to occur (b) update the method comments
@stboch
Copy link

stboch commented Nov 12, 2019

@webunity yeah ouch my email was buzzing along all day. But so glad you had time to work on it... I said I was gonna work on it months ago then just sat on my hands soooo...

No more errors on load so that is a good sign. its populating N/A for all the states so far will need to start my dryer when I get some to get some data. But looks like you got it.

@stboch
Copy link

stboch commented Nov 13, 2019

Looks good running,

Need to map temperature control labels will try to get you them all...

temperature control
@WM_DRY27_TEMP_MID_HIGH_W

@gvdhoven
Copy link
Author

gvdhoven commented Nov 13, 2019 via email

@knackrack615
Copy link

Hello @webunity.

I have an LG Washer (Model: F4J8TS2W) with the following states: https://pastebin.com/WhGaPwfy

Hopefully you can include it on your fork :)

Thanks!

@gvdhoven
Copy link
Author

@sampsyo do you have plans on merging this? because after last pull request i now have merge conflicts which i would like to spend some time on resolving and extending with additional devices but i am not going to maintain my fork indefinitely (as in; it currently works for my use-case already)

@sampsyo
Copy link
Owner

sampsyo commented Nov 28, 2019

Hi, @webunity—I am certainly quite interested in merging support for washers & dryers in the UI. But I am concerned that this branch has now changed quite a bit of the rest of the component and has reorganized the top-level structure of the code, which makes it pretty hard to review independently.

Is there any chance you could summarize the changes that you see as necessary in the overall structure of the component in order to get to W&D support? Then perhaps I can get started on the work to integrate those changes, at which point we can get to a "clean" PR that just adds the new functionality on top of that.

@gvdhoven
Copy link
Author

gvdhoven commented Nov 28, 2019 via email

@sampsyo
Copy link
Owner

sampsyo commented Nov 28, 2019

Sure. While I understand that this might be more difficult than it sounds, here's how I would do it in my wildest dreams:

  1. You open a new PR with just the reorganization of the existing code that you want to do, without adding the new functionality. We iterate on that to get it into a state where I can understand everything that's changed. For example, I don't fully understand what's going on with the new DeviceTypes Python package, and I would like to grok that. And the new LGDevice base class functionality seems really helpful, so I'd like to make sure I understand the separation of concerns there.
  2. You open a second new PR with just the W&D functionality built on that above reorganization. Hopefully this is a small, simple PR that just adds one new file into the new organization.

If separating that out seems too hard, I will instead try to get to work understanding exactly what needs to be done in the reorganization so I can facilitate Step 2 alone.

In the mean time, I don't think it matters too much whether the code lives in a branch here or in your fork.

@ItsRhen
Copy link

ItsRhen commented Nov 30, 2019

...I would greatly appreciate this support... This is the last set of devices to add to HA for me to have everything linked up! Very much appreciate the effort here.

PawelTroka added a commit to PawelTroka/TROKA.Software.SmartHome that referenced this pull request Dec 19, 2019
@towerhand
Copy link

Any updates? Would be great to have support for the washers and dryers.

@JeedHome44
Copy link

Hello,
if you are looking for testers to add the washing machine to home assistant, I have one.
tell me what to do to test.
Sorry I don't know how to program otherwise I will have helped you.
thank you for your work

@gvdhoven
Copy link
Author

Hey guys, sorry for not updating you; but it currently works for me in my own environment. Since then people have been adding stuff so i either have to start all over bit by bit as @sampsyo suggested or abandon this pull request. Either way, i might be able to take a crack at it but it won't be soon (to busy atm)

@JeedHome44
Copy link

Hi,
You say it’s work. But I don’t see my washer whe I add the GitHub in my Hassio.
How can I search new device in Hassio with this GitHub?
I’m a beginner in Hassio.
Thank for your help.

@stboch
Copy link

stboch commented Jan 15, 2020

Hi,
You say it’s work. But I don’t see my washer whe I add the GitHub in my Hassio.
How can I search new device in Hassio with this GitHub?
I’m a beginner in Hassio.
Thank for your help.

You need to pull down from webunity's repo and put that into custom_components and also follow the directions in the readme to get your token.

@JeedHome44
Copy link

Hi,
I find my token. That is ok.
But after, what I have to do for add my washer in Hassio?

@bakazm
Copy link

bakazm commented Feb 6, 2020

Hi,
I find my token. That is ok.
But after, what I have to do for add my washer in Hassio?

I browsed through the code and i only see items pertaining to dishwashers, HVAC, and dryers, not clothes washers.

@paoloantinori
Copy link

Hi, I'm also interested in getting this functionality having a Dryer, a Washing Machine and a Fridge.

I've checked out the PR, fixed a little bug with a wrong variable that was preventing dryers to get picked up and I can report that it works ok!

Screenshot from 2020-02-11 18-48-27

I'm netiher a python nor a HA expert but I'd be interested in helping with this and I'm planning to provide a PR soon.

sensor.py Outdated
add_devices(dishwashers, True)

if dryers:
for device in dishwashers:

Choose a reason for hiding this comment

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

hi, there is an error here. it should be in dryers

Copy link
Author

Choose a reason for hiding this comment

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

Thanx, fixed!

@towerhand
Copy link

Have you guys looked into this new repo? Already have authentication via UI, set up in the HA integrations and only support for washers so far but looks promising.
https://github.com/ollo69/ha-smartthinq-washer

@sampsyo
Copy link
Owner

sampsyo commented Mar 28, 2020

It's sorta troubling that it copies the wideq code into the repository instead of depending on the PyPI release? And it's also disappointing that it does so without attribution or incorporating the license of the original code. 😞

I guess we can hope that they'd contribute the authentication flow back to this repository.

@cwttdb70
Copy link

Any update on this pull request? Just ordered LG ThinQ (what a dumb name) washer & dryer and would be great to integrate into my HA system.

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.