Skip to content

Qwikswitch binary sensors#14008

Merged
kellerza merged 3 commits intohome-assistant:devfrom
kellerza:qsbin
Apr 21, 2018
Merged

Qwikswitch binary sensors#14008
kellerza merged 3 commits intohome-assistant:devfrom
kellerza:qsbin

Conversation

@kellerza
Copy link
Copy Markdown
Member

@kellerza kellerza commented Apr 19, 2018

Description:

  • Final part of the qwikswitch sensors -> binary sensors
  • Fix up asyncio.Timeout in library

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

Example entry for configuration.yaml (if applicable):

qwikswitch:
  url: ...
  sensors:
    - name: door_contact
      id: '@0c2991'
      channel: 1
      type: imod
      invert: True
      class: door
    - name: some_qc
      id: '@0c2992'
      channel: 1
      type: qwikcord    

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

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

Comment thread homeassistant/components/qwikswitch.py Outdated
def _validate_sensor(config):
"""Validate sensor."""
try:
from pyqwikswitch import SENSORS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is a good idea. Since requirements are installed after config validation, any upgrade to the library and sensor schema will not work since the new config will be validated by the old schema.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

During upgrades deps are removed, the idea was that it just does not validate first time during upgrade,but at least during other changes/restarts

Let me rather move this to setup, since it is not critical

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Side note: Deps aren't removed during upgrades.


super().__init__(sensor['id'], sensor['name'])
self.channel = sensor['channel']
self.sensor_type = sensor['type']
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like it's only used in this method. Maybe make it a local variable?

@ipodmusicman
Copy link
Copy Markdown

Hi kellerza,

Busy testing sensors on the beta release now.

Pity that on restart, one cannot retrieve the current state of the sensors and have to wait for an update from them. But all in all, so far so good.

Regarding the sensors, have you considered perhaps changing the states from
True / False / None to Closed / Open / None?

I see that you have introduced a binary sensor, but from what I read in the code, the sensor becomes a binary sensor when you specify the 'type' property which would make any sensor (door, imod, PIR, etc) a binary sensor. Shouldn't the "class" variable influence that decision? Under which conditions would they not be a binary sensor as I see there is code in place for that that will never execute...or perhaps I am misinterpreting the code?

When HA restarts, before an update from a sensor is received it is in a None state. Do binary sensors support a None state? Based on the name "binary sensor", it is either 1 or 0. And since HA doesn't know the state of the binary sensor on restart, it cannot assume either 1 or 0 as that could influence automations.

Documentation:

  • The qwickswitch switch documentation on HA needs to be updated.
  • The main USB Hub documentation still refers to the v1 of the sensor set up. See the text "In the format of {entity_id: QS_id}. (i.e. {door_sensor: '@0dev03'})"
  • Perhaps update the Qwikswitch logos to their latest ones?

I got some feedback from Qwikswitch:

The device id for the PIR sensor is 2c

QS also has a 5 and 6 channel imod, but the bit arrangement is a little different than that of the 4 channel imod:

The 4 channel imod that I have is 000 Ch4 0 CH3 CH2 CH1
while the 5 and 6 channel imod is 00 CH6 CH5 CH4 CH3 CH2 CH1 ​

I was hoping to get feedback regarding their temperature / humidity sensors, but their engineer didn't answer my question. Seems that I might have to purchase the device myself and do the readings and send you the results. Currently, not planning to purchase a temp/humidity sensor just yet, so once I do, I'll sure to let me know.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@kellerza
Copy link
Copy Markdown
Member Author

@ipodmusicman you should test this PR (and docs one) and not the RC release, since most of your questions should then make sense.

The 5&6 channel imods should be ok, but would need an example packet to add the PIR.

@kellerza
Copy link
Copy Markdown
Member Author

Thanks @MartinHjelmare !

@kellerza kellerza merged commit 6ccb835 into home-assistant:dev Apr 21, 2018
@kellerza kellerza deleted the qsbin branch April 21, 2018 06:34
@kellerza
Copy link
Copy Markdown
Member Author

@ipodmusicman hopefully this will go into RC / 0.68.0b1 in the next couple of days.

balloob pushed a commit that referenced this pull request Apr 25, 2018
@balloob balloob mentioned this pull request Apr 27, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants