hass-smartthinq
hass-smartthinq copied to clipboard
sampsyo
[wip] Adding dryer and washer support
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?
@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 "<deviceType>_ID" as value.
Can you maybe merge this already if it doesn't break anything?
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'.
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?
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.
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?
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.
Working on that right now @sampsyo can you review my latest update which re-adds the imports to the functions?
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.
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!
would i even need to call the discovery.load_platform at all? versus just calling add_devices immediately from within the __init__.py script?
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.
@sampsyo finally i have HA running and now are testing the code; during testing i found out that the language should be lowercase (at least for NL) otherwise you would get a tokenError(). Is this something you can recall?
@sampsyo @Webunity I followed a post on Discord here with an interest in helping; I’ve got a LG fridge and dishwasher myself. I’m currently the code owner of the ecobee integration so I have a fair bit of familiarity with the component/platform architecture of HASS.
To answer the question posed in Discord, you need a setup_platform method for each platform you wish to set up (i.e. sensor, climate). All of the code for those platforms should be contained in the sensor.py or climate.py files, but common code (e.g. the LGDevice base class) can be defined in init.py.
@sampsyo I haven’t looked at wideq too closely, but is there an API endpoint that can be polled to return the status/info of all devices? Would be more efficient to cache all that data every x-seconds and then have each HA entity update by reading from that cache than separately polling by itself.
is there an API endpoint that can be polled to return the status/info of all devices?
Sadly, no. In fact, you need to "register" for monitoring updates for each device at a time, and then use the resulting token to poll for data about each individual device. AFAICT, LG does not have an API to poll multiple devices in a single request.
@sampsyo are you open to me refactoring some of the code? We could also implement a basic config flow to make it easier for users to obtain a refresh token (ie no needing to clone wideq). Sorry to hijack the discussion in this PR.
Yes! I have always wanted to build in a token-grabbing workflow to the HA component—if you want to add that, that would be awesome.
And indeed, refactoring would be great too. Feel free to propose whatever.
So, the code is working now without errors again; i will now start restructuring the code so we have only 1 'setup' call.
Ok, looks like the way home assistant is setup (with discovery and load_platform) it doesn't allow me to generate just one setup call. Alternatively, i've cleaned up the code as much as i could and split off the device types inside sensor.py and climate.py to a "DeviceTypes" subfolder.
I am trying to add stuff based on the current wideq version (1.2.0); some stuff will be taken from https://github.com/GuGu927/wideq/blob/gugu_patch/wideq/dryer.py but there he has a wideq version with more features.
Status for today; tomorrow is the test run since my wife has to do the washing :-)
And just now updated also the remaining_time and initial_time fields to show N/A when the dryer is off.
I updated to your repo and tested, It adds the dryer device but I am getting
Error doing job: Task exception was never retrieved Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 275, in async_update_ha_state self._async_write_ha_state() File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 309, in _async_write_ha_state state = self.state File "/config/custom_components/smartthinq/DeviceTypes/LGDryerDevice.py", line 222, in state key = self._status.State.name AttributeError: 'DryerStatus' object has no attribute 'State'
Ok, seems i was basing my changes on some forked wideq libraries which had more functionality. I am back-porting it to my dryer class. I will first test it at home and come back when i'm done.
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... :-(
@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.