Skip to content

Centralize rainbird config and add binary sensor platform#26393

Merged
MartinHjelmare merged 60 commits into
home-assistant:devfrom
konikvranik:dev
Sep 26, 2019
Merged

Centralize rainbird config and add binary sensor platform#26393
MartinHjelmare merged 60 commits into
home-assistant:devfrom
konikvranik:dev

Conversation

@konikvranik
Copy link
Copy Markdown
Contributor

@konikvranik konikvranik 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.

Comment thread script/check_format
Comment thread homeassistant/components/rainbird/binary_sensor.py
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
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.

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.

Comment thread homeassistant/components/rainbird/switch.py Outdated
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.

Please revert unrelated changes.

Comment thread tests/components/python_script/test_init.py Outdated
Copy link
Copy Markdown
Contributor Author

@konikvranik konikvranik left a comment

Choose a reason for hiding this comment

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

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

Comment thread homeassistant/components/rainbird/__init__.py Outdated
Comment thread homeassistant/components/rainbird/sensor.py Outdated
Comment thread homeassistant/components/rainbird/binary_sensor.py Outdated
@konikvranik konikvranik changed the title pyrainbird v0.3.1 pyrainbird v0.4.0 Sep 9, 2019
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.

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

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

@konikvranik konikvranik left a comment

Choose a reason for hiding this comment

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

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

Comment thread homeassistant/components/rainbird/__init__.py Outdated
Comment thread homeassistant/components/rainbird/__init__.py Outdated
Comment thread homeassistant/components/rainbird/__init__.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

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

@konikvranik
Copy link
Copy Markdown
Contributor Author

konikvranik 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
Copy link
Copy Markdown
Contributor Author

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

@konikvranik
Copy link
Copy Markdown
Contributor Author

@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.

Comment thread homeassistant/components/rainbird/__init__.py Outdated
Comment thread homeassistant/components/rainbird/switch.py Outdated
Comment thread homeassistant/components/rainbird/__init__.py Outdated
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.

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

Comment thread homeassistant/components/rainbird/manifest.json Outdated
* 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.
Comment thread homeassistant/components/rainbird/__init__.py Outdated
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.

Please also set the defaults as before.

Comment thread homeassistant/components/rainbird/__init__.py Outdated
Comment thread homeassistant/components/rainbird/__init__.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

Please set the defaults as before too.

@konikvranik
Copy link
Copy Markdown
Contributor Author

I'm sorry, which defaults do you mean?

@MartinHjelmare
Copy link
Copy Markdown
Member

The defaults we removed here:
60ae83a#diff-d575fd59bad63f44b30cd5e8ef6286baL39-L44

Comment thread homeassistant/components/rainbird/__init__.py Outdated
@konikvranik
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

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

@konikvranik
Copy link
Copy Markdown
Contributor Author

konikvranik 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
Copy link
Copy Markdown
Member

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

Comment thread homeassistant/components/rainbird/__init__.py Outdated
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.

Good!

@MartinHjelmare
Copy link
Copy Markdown
Member

Can be merged when build passes.

@MartinHjelmare MartinHjelmare merged commit 3efdf29 into home-assistant:dev Sep 26, 2019
@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

None yet

Development

Successfully merging this pull request may close these issues.

5 participants