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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update KNX component to xknx 0.11 #24738

Merged
merged 8 commits into from Jul 11, 2019

Conversation

@farmio
Copy link
Contributor

commented Jun 24, 2019

Breaking Change:

  • scene: scene_number is now 1 indexed according to KNX standards. Previously it was 0 based. Please add 1 to your already configured scene numbers (scene_number: 5 -> scene_number: 6).
  • sensor: state_address replaces address in configuration
  • binary_sensor: state_address replace address in configuration
  • when using xknx config file (knx: config_file = ...): Replace group_address in BinarySensor and Sensor with group_address_state.

Description:

Updates the knx component to use xknx 0.11.1 . This introduces several new features and bugfixes. For a complete list see: https://github.com/XKNX/xknx/releases/tag/0.11.0

The main new features are

  • New sensor types including string, scene_number and DPT8 types
  • sensor and binary_sensor now have an option to disable periodic state requests (sync_state = False - eg. for actuators that don't support it or sensors of type 'scene_number')
  • Binary values are now exposable to the knx bus
  • Add support for RGBW lights (DPT 251.600)
  • Auto detection of local IP address (so local_ip is optional now for tunnelling)

Related issue (if applicable):
fixes #21737 (pulse sensor type fixed)
fixes #22768 (string sensor type is now supported by default)
fixes #19974 (climate devices with setpoint_shift fixed)
closes #22335 (at least I think its closable by now)
closes #18620 (was fixed in last xknx release)

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9682

Example entry for configuration.yaml (if applicable):

see home-assistant.io and PR 馃憜

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc. (the knx component was already added)

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

farmio added some commits Jun 24, 2019

update component for xknx 0.11.0
- expose sensor state is not casted to float anymore
- climate mode operation list has no more None values
- light supports white_value (rgbw)
- sensor expects `group_address_state` now instead of `group_address`
- sensor forwards device_class if available
@ghost

This comment has been minimized.

Copy link

commented Jun 24, 2019

Hey there @Julius2342, mind taking a look at this pull request as its been labeled with a integration (knx) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@farmio farmio referenced this pull request Jun 24, 2019

Merged

KNX component updates for xknx 0.11 #9682

2 of 2 tasks complete

farmio added some commits Jul 8, 2019

update for xknx 0.11.1
- require xknx 0.11.1
- use 'state_address' instead of 'address' in sensor and binary_sensor configuration
- optional 'sync_state' for sensors and binary_sensors
Show resolved Hide resolved homeassistant/components/knx/light.py Outdated
@property
def device_class(self):
"""Return the device class of the sensor."""
return self.device.ha_device_class()

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 9, 2019

Member

Is xknx aware of home assistant sensor device classes?

This comment has been minimized.

Copy link
@farmio

farmio Jul 9, 2019

Author Contributor

Yes, xknx sensor classes are. The device class is returned as a string - or None if there is no fitting HA device class. From xknx/devices/remote_value_sensor.py:

@property
def ha_device_class(self):
    """Return a string representing the home assistant device class."""
    if hasattr(self.DPTMAP[self.value_type], 'ha_device_class'):
        return self.DPTMAP[self.value_type].ha_device_class
    return None

looks for ha_device_class from eg. here:
https://github.com/XKNX/xknx/blob/cc10a82d11b4a60bd9765a7b410ab05edd5da41f/xknx/knx/dpt_2byte_float.py#L87

homeassistant/const.py is not imported in xknx, if you meant that.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 9, 2019

Member

Ok. Coupling the two packages this tightly is fragile. If the home assistant API would change, something could break until the xknx library is updated. I would recommend to have a dict in this module that maps between the xknx sensor class and the home assistant device class.

But since the xknx library seems to be built for home assistant I don't think it's strictly necessary.

This comment has been minimized.

Copy link
@farmio

farmio Jul 9, 2019

Author Contributor

Xknx works on its own but is built with HA as a main target.
With almost 50 different sensor classes its nice to have everything in one place (range, unit of measurement, ha_device_class, ...) instead of spreading these attributes out to different modules.

We could import the DEVICE_CLASS_* constants in this module and map against our ha_device_class attribute. To me this would feel kind of redundant.

farmio added some commits Jul 10, 2019

@farmio

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Updated form home-assistant/dev to resolve unrelated test failures.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Great!

@MartinHjelmare MartinHjelmare changed the title Update KNX component for xknx 0.11 Update KNX component to xknx 0.11 Jul 11, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@Julius2342 are we good to merge?

@Julius2342

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@MartinHjelmare : Yes! 馃憤

@MartinHjelmare MartinHjelmare merged commit e299d7b into home-assistant:dev Jul 11, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 78a5dc7...8e81b78
Details
codecov/project 94.07% (target 90%)
Details

@farmio farmio deleted the farmio:knx-xknx0.11 branch Jul 11, 2019

@balloob balloob modified the milestone: 0.96.1 Jul 18, 2019

farmio added a commit to farmio/home-assistant that referenced this pull request Jul 20, 2019

temporary patch to fix KNX climate devices
This is a temporary patch for knx climate devices. It should be reverted when home-assistant#24738 is merged to release. 
It should fix home-assistant#25247 for 0.96

@farmio farmio referenced this pull request Jul 20, 2019

Merged

temporary patch to fix KNX climate devices #25356

0 of 9 tasks complete

balloob added a commit that referenced this pull request Jul 21, 2019

temporary patch to fix KNX climate devices (#25356)
This is a temporary patch for knx climate devices. It should be reverted when #24738 is merged to release. 
It should fix #25247 for 0.96

@balloob balloob referenced this pull request Aug 7, 2019

Merged

0.97.0 #25756

@derandiunddasbo

This comment has been minimized.

Copy link

commented Aug 8, 2019

Seems to work fine so far, f.e. binary sensor states are finally updated on startup, woohoo! :-)

...and I can see the states of my DPT 16 sensors without patching xknx myself anymore!

Speaking of which: As of now, the KNX Sensor Doc (https://www.home-assistant.io/components/sensor.knx) doesn't mention the new DPT's (string, DPT-8, scene_number).

@farmio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

馃憤
Documentation is written but it seems its PR ( home-assistant/home-assistant.io#9682 ) was merged to the wrong branch.

@frenck

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

The documentation PR was merged to the correct branch, this went out of sync with the cut-offs and overseen, correcting that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can鈥檛 perform that action at this time.