Qwikswitch refactor & sensor by kellerza · Pull Request #13509 · home-assistant/core

kellerza · 2018-03-28T08:13:41Z

Description:

Refactor of qwikswicth (async/await/dispatchers/feedback from #12641) and adding first version of sensor.

As discussed on MyBB wireframe sensor support for QwikSwitch. Sensors need to implement their own logic over time in the qsSensor class in components/sensors/qwikswitch/. binary_sensor can be added later, but lets see how this general sensor starts....

Config changes:

sensors: added. Format is dictionary of {endtity_id: qs_id}, so adding a door sensor
```
qwikswitch:
  sensors:
    door_sensor: '@id03'
```
cmd_buttons: now adds to the defaults and no need to include defaults anymore (backward compatible). Can be a list or csv list
Switches needs to be specified in the config or they will be discovered as lights. The " switch" in the name can be removed in QSUSB interface: BREAKING CHANGE
```
qwikswitch:
  switches: ['@id01', '@id02']
```

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5045

Checklist:

The code change is tested and works locally.
Local tests pass with lazytox.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

PR: home-assistant/core#13509

MartinHjelmare · 2018-03-28T21:06:44Z

-            "Configure Qwikswitch Light component failed")
-        return False
+    qsusb = hass.data[QWIKSWITCH]
+    devs = [QSLight(id, qsusb) for id in discovery_info[QWIKSWITCH]]


Please use another variable name than id, which is a builtin.

MartinHjelmare · 2018-03-28T21:07:42Z

-def setup_platform(hass, config, add_devices, discovery_info=None):
+async def async_setup_platform(hass, _, add_devices, discovery_info=None):
    """Add lights from the main Qwikswitch component."""
-    if discovery_info is None:


This should be kept, I think. You don't want to continue unless setup is via discovery, right? Keep it and return if true.

Can do, but should we not raise some exception if a user incorrectly configures one of these platforms? Or log the error then? I dont like failing silently.

I think documentation is enough in this case. It's our standard procedure at the moment.

MartinHjelmare · 2018-03-28T21:10:09Z

+    add_devices(devs)
+
+    for _id, dev in zip(discovery_info[QWIKSWITCH], devs):
+        hass.helpers.dispatcher.async_dispatcher_connect(


Move this to the entity coroutine method async_added_to_hass, ie connect each entity from within itself. Reason is that add_devices schedules a call to add devices, so add_devices is not guaranteed to have added the entities when it returns.

MartinHjelmare · 2018-03-28T21:12:29Z

-        _LOGGER.info("Waiting for long poll to QSUSB to time out")
+    # Discover all devices in QSUSB
+    if not await qsusb.update_from_devices():
+        raise PlatformNotReady


This only has an effect if raised within setup_platform.

Unfortunate, for now I will just remove and leave more clever recovery mechnism for a future PR

MartinHjelmare · 2018-03-28T21:14:40Z

            vol.Coerce(str),
        vol.Optional(CONF_DIMMER_ADJUST, default=1): CV_DIM_VALUE,
-        vol.Optional(CONF_BUTTON_EVENTS): vol.Coerce(str)
+        vol.Optional(CONF_BUTTON_EVENTS): cv.ensure_list_csv,


ensure_list_csv should only be used to avoid breaking changes. It's better to use plain ensure_list if possible.

Does it work to add a default empty list here, so you don't need to add that in async_setup?

It will work, only reason I dont like it is that validating then adds this empty key to the config (which really is not used by many people).

ensure_list_csv is indeed for backward compatibility. It used to be a text deliminated string

MartinHjelmare · 2018-03-28T21:38:36Z

+class QSSwitch(QSToggleEntity, SwitchDevice):
+    """Switch based on a Qwikswitch relay module."""
+
+    pass


Remove this.

MartinHjelmare · 2018-03-28T21:39:11Z

+    add_devices(devs)
+
+    for _id, dev in zip(discovery_info[QWIKSWITCH], devs):
+        hass.helpers.dispatcher.async_dispatcher_connect(


Move this to async_added_to_hass.

MartinHjelmare · 2018-03-28T21:39:32Z

-def setup_platform(hass, config, add_devices, discovery_info=None):
+async def async_setup_platform(hass, _, add_devices, discovery_info=None):
    """Add switches from the main Qwikswitch component."""
-    if discovery_info is None:


MartinHjelmare · 2018-03-28T21:40:11Z

+
+async def async_setup_platform(hass, _, add_devices, discovery_info=None):
+    """Add lights from the main Qwikswitch component."""
+    qsusb = hass.data[QWIKSWITCH]


Return if no discovery_info is passed.

MartinHjelmare · 2018-03-28T21:43:59Z

+                    item[QS_ID], item)

        # Update all ha_objects
        hass.async_add_job(qsusb.update_from_devices)


What does this do? Since we're in a callback that is called by qsusb, shouldn't qsusb already know to update its devices?

The QSUSB API has two main methods, listen and devices. These really have no relationship betwen them and I simply use listen to see when there is activity and call update_from_devices.

If I have access to the event loop in listen I could potentially add it there, but then the API becomes less felxible (although only HA uses it at the moment)

The aiohttp session is already passed, so passing the event loop too should work from my point of view. But I don't know this library, so you should decide what fits best.

Lets keep it like this for now, it clearly shows what it does with the API and the API is a very close to match to the manufacturer's API

ipodmusicman · 2018-03-29T09:16:42Z

Hi there
When I start up HA as a custom component, I get the following error in the log file

File "/config/custom_components/qwikswitch.py", line 94, in async_setup
from pyqwikswitch.async_ import QSUsb

kellerza · 2018-03-29T09:22:31Z

@ipodmusicman I suspect it is not pulling the new library, please delete the two pyqwikswitch folders in your-hass-config\deps\lib\python3.6\site-packages\

HA deletes these during upgrades, but you are not upgrading now and it assumes it already has the correct dependency

kellerza · 2018-03-29T09:27:00Z

@MartinHjelmare thank you for the review, the asnync_added_to_hass is much more elegant!

MartinHjelmare · 2018-03-29T09:22:25Z

+    def async_added_to_hass(self):
+        """Listen for updates from QSUSb via dispatcher."""
+        # hass and schedule_update_ha_state is part of Entity/ToggleEntity
+        # pylint: disable=no-member


Have this class inherit from Entity and remove this disable.

MartinHjelmare · 2018-03-29T09:23:17Z

-    def supported_features(self):
-        """Flag supported features."""
-        return SUPPORT_BRIGHTNESS if self._dim else None
+    def async_added_to_hass(self):


async def async_added_to_hass(self):

MartinHjelmare · 2018-03-29T09:26:54Z

+        # hass and schedule_update_ha_state is part of Entity/ToggleEntity
+        # pylint: disable=no-member
+        self.hass.helpers.dispatcher.async_dispatcher_connect(
+            self.qsid, lambda _=None: self.schedule_update_ha_state)


You don't need the lambda.

Use async_schedule_update_ha_state instead.

Ok for async_schedule_update, but I prefer to have the lambda,

There are two async_dispatcher_send calls, one includes a packet as parameter and one not. It should be safe that no switch or light entities will get called with the packet parameter, but I might not have 100% control over this (One scenario is where the devices list removes a device)

I don't understand the scenario where this problem would surface. If you want to keep it super safe, please define a regular method that accepts *args and calls self.async_schedule_update_ha_state.

ipodmusicman · 2018-03-29T09:54:53Z

kellerza, I removed the packages and downloaded the latest code based on your changes now. Not sure if that was the right thing to do based on the below error in the log.

  File "/usr/lib/python3.6/site-packages/homeassistant/helpers/entity_platform.py", line 82, in async_setup
    SLOW_SETUP_MAX_WAIT, loop=hass.loop)
  File "/usr/lib/python3.6/asyncio/tasks.py", line 358, in wait_for
    return fut.result()
  File "/usr/lib/python3.6/asyncio/futures.py", line 245, in result
    raise self._exception
  File "/usr/lib/python3.6/asyncio/tasks.py", line 180, in _step
    result = coro.send(None)
  File "/config/custom_components/light/qwikswitch.py", line 20, in async_setup_platform
    devs = [QSLight(qsid, qsusb) for qsid in discovery_info[QWIKSWITCH]]
  File "/config/custom_components/light/qwikswitch.py", line 20, in <listcomp>
    devs = [QSLight(qsid, qsusb) for qsid in discovery_info[QWIKSWITCH]]
  File "/usr/lib/python3.6/site-packages/homeassistant/components/qwikswitch.py", line 58, in __init__
    from pyqwikswitch import (QS_ID, QS_NAME, QSType, PQS_VALUE, PQS_TYPE)
ImportError: cannot import name 'PQS_VALUE'

kellerza · 2018-03-29T11:46:22Z

@ipodmusicman you will have to add all 4 qwikswitch files to the custom_components folder since they all changed. The error show that the old components/qwikswitch.py is loaded by HASS (most likely it dd not find this custom version/its in the wrong folder etc)

config\custom_components\qwikswitch.py
config\custom_components\switch\qwikswitch.py
config\custom_components\sensor\qwikswitch.py
config\custom_components\light\qwikswitch.py

ipodmusicman · 2018-03-29T14:42:32Z

kellerza,

I grabbed all 4 files from the beginning, updating them as you pushed changes during the day. I also deleted the site-packages just in case before restart.

With the component in place, my one light isn't appearing in HA any more and when I remove the custom_components and restart, my light is back.

With the sensor in place, I get the following:

2018-03-29 16:25:53 ERROR (MainThread) [homeassistant.components.light] Error while setting up platform qwikswitch
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/homeassistant/helpers/entity_platform.py", line 82, in async_setup
    SLOW_SETUP_MAX_WAIT, loop=hass.loop)
  File "/usr/lib/python3.6/asyncio/tasks.py", line 358, in wait_for
    return fut.result()
  File "/usr/lib/python3.6/asyncio/futures.py", line 245, in result
    raise self._exception
  File "/usr/lib/python3.6/asyncio/tasks.py", line 180, in _step
    result = coro.send(None)
  File "/config/custom_components/light/qwikswitch.py", line 20, in async_setup_platform
    devs = [QSLight(qsid, qsusb) for qsid in discovery_info[QWIKSWITCH]]
  File "/config/custom_components/light/qwikswitch.py", line 20, in <listcomp>
    devs = [QSLight(qsid, qsusb) for qsid in discovery_info[QWIKSWITCH]]
  File "/usr/lib/python3.6/site-packages/homeassistant/components/qwikswitch.py", line 58, in __init__
    from pyqwikswitch import (QS_ID, QS_NAME, QSType, PQS_VALUE, PQS_TYPE)
ImportError: cannot import name 'PQS_VALUE'

I removed the sensor from the config and got the following below:

018-03-29 15:56:26 ERROR (MainThread) [homeassistant.setup] Error during setup of component qwikswitch
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/homeassistant/setup.py", line 142, in _async_setup_component
    result = await component.async_setup(hass, processed_config)
  File "/config/custom_components/qwikswitch.py", line 99, in async_setup
    from pyqwikswitch.async_ import QSUsb
ModuleNotFoundError: No module named 'pyqwikswitch.async_'
2

Also to note, that with the sensor in place, even with the errors were occurring in the log, I saw the badge icon at the top and on clicking on it saw the various fields and of course the data block change accordingly as a opened and closed the various channels on the imod.

However, it seems to go stale after a bit since I'd leave it for about 10 min and open / close again and no updates reflect.

Does one generally display sensors as badges or in cards by default?

Would I be able to assign a friendly name to the sensor within the qwikswitch config or would I need to do a customization?

Is it possible to create separate sensors per channel? A door sensor or movement sensor only has "1 channel", but an imod can have up to 5 channels. A suggestion on the conifg - you'll need to guide on what is best practice as this is only a suggestion:

qwikswitch:
  url: http://localhost:2020
  sensors:
    - entity: imod_arm
      friendly_name: 'Alarm Armed'       
      id: '@12345'
      channel: 1
    - entity: imod_disarm
      friendly_name: 'Alarm Disarmed'       
      id: '@12345'
      channel: 2
    - entity: door_sensor
       friendly_name:  'Garage Door'
       id: '@22222'

Let me know what you think?

Btw, I got a response from QwikSwitch yesterday.

The first byte of the data block indicates the sensor type:

4e = imod
46 = Door sensor

I've asked for a comprehensive list including the motion sensor and any other sensors they have like temperature and humidity sensors. I've also asked for example messages if they have for the temp and humidity sensors.

The second byte is the firmware version
The proceeding bytes as per our conversation on MyBB whereby:

for an imod = third and forth byte indicates which channels are open/closed as well as which channel was affected in a status change - see our discussion om MyBB

for a door sensor = 00 = Open, 64 (100 decimal) = Closed

kellerza · 2018-03-29T17:05:06Z

@ipodmusicman (I added three backticks ``` to your comment for code blocks)

From the errors, unfortunately we cannot do custom_components, since
/config/custom_components/light/qwikswitch.py imports homeassistant.components.qwikswitch which is /usr/lib/python3.6/site-packages/homeassistant/components/qwikswitch.py

So you either need to patch qwikswitch in to /usr/lib/python3.6/site-packages/homeassistant/components/qwikswitch.py. The subcomponents (light,switch,sensor) can live in custom_components

The second error seems to show that your don't have the latest library again...?

Ideally we should merge this and you can then use dev for your tests.... lets just wait for final ok from @MartinHjelmare

BTW, see the documentation link for initial config today (incl channel), your might be cleaner

MartinHjelmare

I'm happy if you do the change below.

MartinHjelmare · 2018-03-29T18:07:56Z

+    async def async_added_to_hass(self):
+        """Listen for updates from QSUSb via dispatcher."""
+        self.hass.helpers.dispatcher.async_dispatcher_connect(
+            self.qsid, lambda _=None: self.async_schedule_update_ha_state())


Please swap this lambda for a regular helper method that calls async_schedule_update_ha_state and connect that helper method here.

MartinHjelmare

Nice! Can be merged when build passes.

ipodmusicman · 2018-03-30T12:33:18Z

kellerza,

Now that this merge is done, what is the best way for us to continue our ongoing conversations?

I installed HA on Ubuntu so that I can give it a test drive and get to know the platform so I'd imagine that since it is deviating from the recommended set up on a Pi, things seem to be getting a bit mixed up. :) I plan to purchase a Pi-3b within the next month or so so that I can set things up properly.

I saw the channel addition to the documentation. Are you going to consider my config suggestion for the sensors? I'll see how I can expand my skills to Python as I come from a Java background to assist in putting something in place that can be used to interpret the data block in the method below based on the sensor type as well as the bytes themselves so that the method either returns open or closed. For a door sensor (and probably a motion sensor as well), it should be as easy as returning either open or closed based on the value of the third byte, but for the imod, it is different as one would need to know which channel the sensor is configured for to determine which bit to use to decide whether it is open or closed.

def state(self):
    """Return the value of the sensor."""
    return self._val.get('data', 0)

MartinHjelmare · 2018-03-30T13:12:21Z

If you want to suggest an enhancement please open a feature request in the Feature Requests section of our community forum.

If you want to discuss things before opening a PR you can make an RFC issue.
Merged PRs should not be used for enhancement discussion.

Thanks!

Qwikswitch refactor & sensor

fd94a77

kellerza requested a review from MartinHjelmare March 28, 2018 08:13

homeassistant added cla-signed integration: qwikswitch new-platform platform: light.qwikswitch labels Mar 28, 2018

bump requirement

7a80ff6

kellerza requested a review from andrey-git as a code owner March 28, 2018 20:32

homeassistant added the cla-signed label Mar 28, 2018

kellerza added a commit to home-assistant/home-assistant.io that referenced this pull request Mar 28, 2018

Qwikswitch refactor & sensors

a285788

PR: home-assistant/core#13509

kellerza changed the title ~~WIP: Qwikswitch refactor & sensor~~ Qwikswitch refactor & sensor Mar 28, 2018

MartinHjelmare reviewed Mar 28, 2018

View reviewed changes

Feedback - async_added_to_hass

8dc058d