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

KNX Component: Scene support and expose sensor values #11978

Merged
merged 22 commits into from Feb 26, 2018

Conversation

Projects
None yet
6 participants
@Julius2342
Copy link
Contributor

commented Jan 27, 2018

Description:

  • Bumped version of KNX library to 0.8.0
  • Support for many more sensor types: power, electric_current, electriy_potential, energy, frequency, heatflowrate, phaseanglerad, phaseangledeg, powerfactor.
  • Added scene support.
  • Added support for exposing sensors to KNX bus
  • Added reset function for binary sensors.

Broadcast sensor values and time to KNX bus

This feature was discussed within PR #10708 and PR #10654 and is now a more generic approach.

In the new implementation, all changes of sensor values will be broadcasted to KNX bus. Additionally HASS will answer read requests from KNX bus.

# Example configuration.yaml entry
knx:
    expose:
        - type: 'temperature'
          entity_id: 'sensor.owm_temperature'
          address: '0/0/2'
        - type: 'time'
          address: '0/0/1'
        - type: 'datetime'
          address: '0/0/23'

For available sensor types see KNX sensor documentation.

Scene support

# Example configuration.yaml entry
scene:
  - name: Romantic
    platform: knx
    address: 8/8/8
    scene_number: 23

reset_after within binary sensors

Added new parameter reset_after to configuration. Binary sensor will reset state after given milliseconds. This configuration is helpful for touch buttons which don't send an off-telegram after being released.

binary_sensor:
- platform: knx
  name: ToiletButton
  address: '5/2/0'
  reset_after: 100

home-assistant.github.io

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Checklist:

  • The code change is tested and works locally.

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • 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.
XKNX improvements: Added Scene support, added support for exposing se…
…nsors to KNX bus, added reset value option for binary switches

@Julius2342 Julius2342 requested a review from andrey-git as a code owner Jan 27, 2018

@Julius2342 Julius2342 changed the title XKNX improvements KNX improvements Jan 27, 2018

@Julius2342 Julius2342 changed the title KNX improvements KNX Component: Scene support and expose sensor values Jan 27, 2018

@fabaff

This comment has been minimized.

Copy link
Member

commented Feb 2, 2018

E:305, 8: No name 'datetime' in module 'xknx.devices' (no-name-in-module)
E:305, 8: Unable to import 'xknx.devices.datetime' (invalid syntax (<string>, line 59)) (import-error)
@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

@fabaff : i fixed the pylint problem (had to wait for the next release of the underlaying library).

Julius2342 added a commit to Julius2342/home-assistant.github.io that referenced this pull request Feb 4, 2018

fabaff added a commit to home-assistant/home-assistant.io that referenced this pull request Feb 5, 2018

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

Please try to keep 1 feature per PR

def create_exposures(self):
"""Create exposures."""
exposures = []
if CONF_KNX_EXPOSE in self.config[DOMAIN]:

This comment has been minimized.

Copy link
@balloob

balloob Feb 13, 2018

Member

guard clause.

if CONF_KNX_EXPOSE not in …:
    return exposures
def async_setup_platform(hass, config, async_add_devices,
discovery_info=None):
"""Set up the scenes for KNX platform."""
if DATA_KNX not in hass.data \

This comment has been minimized.

Copy link
@balloob

balloob Feb 13, 2018

Member

This is not possible because you have knx as a dependency and so setup never gets called unless knx has been setup correctly.

Also, just returning False is weird. Then it will never be setup?


def activate(self):
"""Activate the scene."""
self.hass.async_add_job(self.scene.run())

This comment has been minimized.

Copy link
@balloob

balloob Feb 13, 2018

Member

Not allowed to call async methods in sync context. Also don't run outside of the main thread. Just call self.scene.run()

return self.scene.name

@property
def should_poll(self):

This comment has been minimized.

Copy link
@balloob

balloob Feb 13, 2018

Member

Default for scenes is already false.

return False

@property
def is_on(self):

This comment has been minimized.

Copy link
@balloob

balloob Feb 13, 2018

Member

This doesn't exist.

broadcast_type=broadcast_type,
group_address=self.address)
self.xknx.devices.add(self.device)
async_track_utc_time_change(

This comment has been minimized.

Copy link
@balloob

balloob Feb 13, 2018

Member

This is so weird. Shouldn't this be a feature inside xknx ?

@@ -49,6 +50,8 @@
vol.Optional(CONF_DEVICE_CLASS): cv.string,
vol.Optional(CONF_SIGNIFICANT_BIT, default=CONF_DEFAULT_SIGNIFICANT_BIT):
cv.positive_int,
vol.Optional(CONF_RESET_AFTER, default=None): vol.Any(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 20, 2018

Member

I'd prefer if you remove the default. You are using dict.get below so the default here has no effect.

This comment has been minimized.

Copy link
@Julius2342

Julius2342 Feb 21, 2018

Author Contributor

changed it to: vol.Optional(CONF_RESET_AFTER): cv.positive_int,

@@ -45,6 +51,12 @@
vol.Required(CONF_KNX_LOCAL_IP): cv.string,
})

EXPOSE_SCHEMA = vol.Schema({
vol.Required(CONF_KNX_EXPOSE_TYPE): cv.string,
vol.Optional(ATTR_ENTITY_ID): cv.string,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 20, 2018

Member

Use CONF_ENTITY_ID as this is for a config value in a config schema.

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

@MartinHjelmare : Is there anything missing from your perspective? Are you fine with my comment regarding upper() or - if not - do you have any idea how i can solve this?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

That's fine but the comment about CONF_ENTITY_ID should be taken care of.

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

@MartinHjelmare : Oh blush! oversaw this comment! Pushed with commit 35339d1

async_track_state_change(
self.hass, self.entity_id, self._async_entity_changed)

@asyncio.coroutine

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 24, 2018

undefined name 'asyncio'

Julius2342 added some commits Feb 24, 2018

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@MartinHjelmare : May I ask you to have a look?

@MartinHjelmare
Copy link
Member

left a comment

Looks good!

@MartinHjelmare
Copy link
Member

left a comment

Sorry, missed one.

@@ -45,6 +51,12 @@
vol.Required(CONF_KNX_LOCAL_IP): cv.string,
})

EXPOSE_SCHEMA = vol.Schema({
vol.Required(CONF_KNX_EXPOSE_TYPE): cv.string,
vol.Optional(CONF_ENTITY_ID): cv.string,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 25, 2018

Member

This would be even better validated with cv.entity_id.

@MartinHjelmare
Copy link
Member

left a comment

Great!

@balloob balloob merged commit 7d5c158 into home-assistant:dev Feb 26, 2018

5 checks passed

WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.006%) to 94.072%
Details
hound No violations found. Woof!

@Julius2342 Julius2342 deleted the Julius2342:xknx branch Feb 26, 2018

hmn added a commit to hmn/home-assistant-dev that referenced this pull request Feb 27, 2018

Merge branch 'dev' into ps4
* dev: (148 commits)
  Fix a problem with calling `deconz.close` (home-assistant#12657)
  Frontend bump to 20180227.0
  Update core HSV color scaling to standard scales: (home-assistant#12649)
  Homekit schema gracefully fail with integer (home-assistant#12725)
  AsusWRT log exceptions (home-assistant#12668)
  Fix homekit: temperature calculation (home-assistant#12720)
  Unbreak tahoma (home-assistant#12719)
  Next generation of Xiaomi Aqara devices added (home-assistant#12659)
  Fix mysensor defaults (home-assistant#12687)
  Harmony: make activity optional (home-assistant#12679)
  Add history_graph component to demo (home-assistant#12681)
  Adds simulated sensor (home-assistant#12539)
  Added config validator for future group platforms (home-assistant#12592)
  KNX Component: Scene support and expose sensor values (home-assistant#11978)
  Roomba timeout (home-assistant#12645)
  Fix formatting of minutes for sleep start in the fitbit sensor (home-assistant#12664)
  Homekit Update, Support for TempSensor (°F) (home-assistant#12676)
  Remove braviatv_psk (home-assistant#12669)
  Upgrade insteonplm to 0.8.2 (required refactoring) (home-assistant#12534)
  Enable pytradfri during build, and include in Docker (home-assistant#12662)
  ...

@balloob balloob referenced this pull request Mar 9, 2018

Merged

0.65 #12995

@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.