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

Centralize rainbird config and add binary sensor platform #26393

Merged
merged 60 commits into from Sep 26, 2019

Conversation

@konikvranik
Copy link
Contributor

commented Sep 3, 2019

Breaking Change:

The configuration has been changed and all configuration is now done under the rainbird: key in configuration.yaml. Configuration of rainbird switches is moved from platform config section to main rainbird: config under zones: key. Please read the updated documentation.

Description:

  • Bump pyrainbird to 0.4.1
  • Cache states of sprinklers for efficient communication to controller
  • fixed rainsensor state
  • added rainsensor binary sensor

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#10326

Example entry for configuration.yaml:

rainbird:
  host: IP_ADDRESS_OF_MODULE
  password: YOUR_PASSWORD
  trigger_time: 6
  zones:
    1:
      trigger_time: 6

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
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
script/check_format Outdated Show resolved Hide resolved
homeassistant/components/rainbird/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/binary_sensor.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Sep 3, 2019
@konikvranik konikvranik referenced this pull request Sep 4, 2019
5 of 5 tasks complete
Copy link
Member

left a comment

Since this PR already is a breaking change, I suggest we also change to use discovery.load_platform to load the platforms from the component, instead of using platform config sections. This is our new standard to collect all config under the integration key in the config.

This will make it easier to continue expanding this integration.

Copy link
Member

left a comment

Please revert unrelated changes.

tests/components/python_script/test_init.py Outdated Show resolved Hide resolved
@konikvranik konikvranik force-pushed the konikvranik:dev branch from 53be6b4 to 2b8857c Sep 8, 2019
Copy link
Contributor Author

left a comment

Since this PR already is a breaking change, I suggest we also change to use discovery.load_platform to load the platforms from the component, instead of using platform config sections. This is our new standard to collect all config under the integration key in the config.

This will make it easier to continue expanding this integration.

Please look at d6dd916 if that's the way you intended

@konikvranik konikvranik changed the title pyrainbird v0.3.1 pyrainbird v0.4.0 Sep 9, 2019
Copy link
Member

left a comment

Please don't add more features to this PR after this.

homeassistant/components/rainbird/switch.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/switch.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/switch.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/switch.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/switch.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/switch.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare changed the title pyrainbird v0.4.0 Centralize rainbird config and add binary sensor platform Sep 9, 2019
Copy link
Contributor Author

left a comment

There has to be upgrade to pyrainbird 0.4.1. Testing with HA I found bug there

konikvranik added a commit to konikvranik/home-assistant.io that referenced this pull request Sep 9, 2019
@konikvranik konikvranik referenced this pull request Sep 9, 2019
2 of 2 tasks complete
@konikvranik konikvranik force-pushed the konikvranik:dev branch from 2550257 to ab48ffe Sep 9, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Make sure to bump the library versions here too. They are still at 0.4.0.

@konikvranik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

I'm not owner of pyraindbird library so I have to wait until @jbarrancos publishes it to pypi.
Then I will update it here.

@konikvranik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Upgraded to pyrainbird 0.4.1, tested on local instance and all seems to work flawlessly

@konikvranik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@jbarrancos May I ask you how does scan_interval works?
Home assistant core finds out if component has this flag in config and in that case it sets proper update interval?
Or how is update scheduled? I can not find docs which explain it.

@konikvranik konikvranik force-pushed the konikvranik:dev branch from 3134d54 to 8baaa25 Sep 10, 2019
Dev automation moved this from Review in progress to Reviewer approved Sep 19, 2019
Copy link
Member

left a comment

Good! Can be merged when final comment is addressed and build passes.

homeassistant/components/rainbird/manifest.json Outdated Show resolved Hide resolved
konikvranik added 2 commits Sep 19, 2019
* irrigation time just as positive integer. Making it complex does make
sense
* zone edfaults fullfiled at runtime. There is no information about
available zones in configuration time.
Dev automation moved this from Reviewer approved to Review in progress Sep 19, 2019
konikvranik added 2 commits Sep 19, 2019
Copy link
Member

left a comment

Please also set the defaults as before.

homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rainbird/__init__.py Outdated Show resolved Hide resolved
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

Please set the defaults as before too.

@konikvranik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

I'm sorry, which defaults do you mean?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

@konikvranik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

If you mean _set_defaults to set irrigation times for zones in configuration time, it does not make sense. Zones are not known in configuration time, so could not be set. They are discovered from controller in time of platform setup.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

We have a zone config schema. We should set the default trigger time in the zone schema.

@konikvranik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

We can set values only for zones we want to overwrite some config. We can not overwrite irrigation time for zones that we don't have in configuration. So it is nonsense to substitute default twice. In config and then in runtime. That will be code duplication. What will be that for?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Ok. Address the last comment and we're ready.

Copy link
Member

left a comment

Good!

Dev automation moved this from Review in progress to Reviewer approved Sep 25, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Can be merged when build passes.

@MartinHjelmare MartinHjelmare merged commit 3efdf29 into home-assistant:dev Sep 26, 2019
11 checks passed
11 checks passed
CI Build #20190925.18 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 02466ed...292a238
Details
codecov/project 94.16% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 26, 2019
KJonline added a commit to KJonline/home-assistant that referenced this pull request Sep 26, 2019
* dev: (87 commits)
  Add ecobee services to create and delete vacations (home-assistant#26923)
  Centralize rainbird config and add binary sensor platform (home-assistant#26393)
  Add config flow to transmission (home-assistant#26434)
  Add Plex config options support (home-assistant#26870)
  Bump pyobihai, add unique ID and availability (home-assistant#26922)
  Add mysensors codeowner (home-assistant#26917)
  [ci skip] Translation update
  Add MySensors ACK (home-assistant#26894)
  Remove lamps and groups from ha when removed from Hue (home-assistant#26881)
  Add config flow to ecobee (home-assistant#26634)
  deCONZ - Increase bridge discovery robustness in config flow (home-assistant#26911)
  Add call direction sensor for Obihai (home-assistant#26867)
  Bumped version to 0.99.3
  HM-CC-TC was not recognized (home-assistant#26623)
  Add google_assistant alarm_control_panel (home-assistant#26249)
  deCONZ - Improve ssdp discovery by storing uuid in config entry (home-assistant#26882)
  Fix missing whitespace around arithmetic operator (home-assistant#26908)
  Fix bed_activity history chart of the Xiaomi Aqara vibration sensor (home-assistant#26875)
  Add voltage attribute to Xiaomi Aqara devices (home-assistant#26876)
  Bump ndms2-client to 0.0.9 (home-assistant#26899)
  ...
@lock lock bot locked and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.