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

Add support for a wider variety of EnOcean devices #22052

Merged
merged 3 commits into from
Apr 24, 2019

Conversation

bdurrer
Copy link
Contributor

@bdurrer bdurrer commented Mar 14, 2019

Breaking Change:

Theoretically, nothing should break. The default configuration is compatible with previous versions of the component. However, since I do not own all of the devices, I cannot guarantee it. I suspect that previous commiters @rubund and @NoUsername own some of the devices. Guys, if you read this, I would be very thankful if you could spend the time to review and test this PR.

Tested with:

  • 2 Rocker Switch
  • Temperature Sensor

Description:

This PR adds support for temperature sensors and humidity sensors. The telegram message is now closer to the officially documented messages, which in theory should support a wider variety of devices.

As a result of the new sensors and the more generic message parsing, a refactoring was necessary. The enocean package was updated, since newer versions can do the parsing for us, moving some complexity out of the component.
The tests should make sure that the component still works for existing devices.

Note: I am away on an one week vacation, starting on the 16th. So take your time reviewing it

Related issue (if applicable): none

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: enocean
    id: [0x20, 0x20, 0x20, 0x20]
    name: Sensor 1
    device_class: temperature
  - platform: enocean
    id: [0x20, 0x20, 0x20, 0x20]
    name: Sensor 2
    device_class: humidity
  - platform: enocean
    id: [0x20, 0x20, 0x20, 0x20]
    name: Sensor 2
    device_class: powersensor

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.

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 or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

@fabaff fabaff changed the title EnOcean: Add support for a wider variety of devices Add support for a wider variety of EnOcean devices Mar 15, 2019
@bdurrer bdurrer force-pushed the feature/enocean-sensors branch 2 times, most recently from 1e2a328 to 4971963 Compare March 24, 2019 09:05
@bdurrer
Copy link
Contributor Author

bdurrer commented Mar 24, 2019

I am back and the test errors (doh!) are fixed now

@bdurrer bdurrer force-pushed the feature/enocean-sensors branch 3 times, most recently from 2e2d072 to a542ac8 Compare April 5, 2019 22:31
Copy link
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.

This integration was made before we had our current standards in place. We should now adapt the integration to follow our current standards for the good of the integration.

My comments below should be read in that light.

homeassistant/components/enocean/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/enocean/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/enocean/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/enocean/sensor.py Show resolved Hide resolved
tests/components/enocean/test_binary_sensor.py Outdated Show resolved Hide resolved
tests/components/enocean/test_binary_sensor.py Outdated Show resolved Hide resolved
tests/components/enocean/test_light.py Outdated Show resolved Hide resolved
@bdurrer bdurrer force-pushed the feature/enocean-sensors branch 3 times, most recently from e16e67c to 0e61d11 Compare April 10, 2019 19:12
@bdurrer
Copy link
Contributor Author

bdurrer commented Apr 15, 2019

Thanks @MartinHjelmare for the reviews. I fixed everything (I believe).

As discussed in Discord

  • We keep the non-breaking
  • The tests are removed and will go in a new PR
  • The binary_sensor's event remains unchanged
  • The binary_sensor and the enocean component in general need refactoring

@kapwebdr
Copy link

Hi, i create a pull request with a mix off your code and another pull request :
#23326
See if that's helping or not.

* Bump EnOcean version to 0.50
* Refactor components for more generic device handling
* Move radio packet data interpretation to specific devices
@codecov

This comment has been minimized.

@MartinHjelmare
Copy link
Member

Please don't squash commits after review has started. Sqaushing commits makes it harder for readers to track changes.

@MartinHjelmare
Copy link
Member

Good job on the code!

@MartinHjelmare MartinHjelmare merged commit 96735e4 into home-assistant:dev Apr 24, 2019
@balloob balloob mentioned this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants