Skip to content

Add sensors and services to RainMachine#14326

Merged
MartinHjelmare merged 35 commits intohome-assistant:devfrom
bachya:rainmachine-3
May 29, 2018
Merged

Add sensors and services to RainMachine#14326
MartinHjelmare merged 35 commits intohome-assistant:devfrom
bachya:rainmachine-3

Conversation

@bachya
Copy link
Copy Markdown
Contributor

@bachya bachya commented May 7, 2018

Description:

Adds several (binary) sensors to the RainMachine component:

  • Binary Sensor: Extra Water on Hot Days
  • Binary Sensor: Freeze Protection
  • Binary Sensor: Freeze Restrictions
  • Binary Sensor: Hourly Restrictions
  • Binary Sensor: Month Restrictions
  • Binary Sensor: Rain Delay Restrictions
  • Binary Sensor: Rain Sensor Restrictions
  • Binary Sensor: Weekday Restrictions
  • Sensor: Freeze Protect Temperature

Additionally, this PR adds several services:

  • start_program
  • start_zone
  • stop_all
  • stop_program
  • stop_zone

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

rainmachine:
  ip_address: 192.168.1.100
  password: abc123

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

@bachya bachya changed the title Adds sensors and services to RainMachine WIP: Adds sensors and services to RainMachine May 7, 2018
@bachya bachya changed the title WIP: Adds sensors and services to RainMachine Adds sensors and services to RainMachine May 9, 2018
@bachya
Copy link
Copy Markdown
Contributor Author

bachya commented May 16, 2018

FYI: been running this in my local HASS for a week or so and everything seems to work fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need for .keys()

Comment thread homeassistant/components/rainmachine.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use git mv <X> <Y> to keep history attached to a file

Copy link
Copy Markdown
Contributor Author

@bachya bachya May 18, 2018

Choose a reason for hiding this comment

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

Not sure what you mean here – I performed git mv homeassistant/components/rainmachine.py homeassistant/components/rainmachine/__init__.py, but the result appears to be the same as before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No device class ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are all device configurations, not representations of conditions. Example: TYPE_FREEZE means "Is the device currently restricted from watering because it's too cold outside?" That doesn't seem to fit the spirit of the cold device class. The same is true for the other sensors.

That said, let me know if you'd rather they be coerced into that format and I'll get it done.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't catch this, instead a voluptuous schema should make sure we never get bad keys.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems like the RainMachineEntity is overcomplicated. The logic is hard to follow.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You need to annotate this as callback or else it will run synchronous

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are the keys called ATTR? Shouldn't it be TYPE or something ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are these errors? Why isn't Rainmachine raising their own error type?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove some of the debugs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not log config

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

make a callback.

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.

Ok, merging. 👍

@MartinHjelmare MartinHjelmare changed the title Adds sensors and services to RainMachine Add sensors and services to RainMachine May 29, 2018
@MartinHjelmare MartinHjelmare merged commit 084b328 into home-assistant:dev May 29, 2018
@bachya bachya deleted the rainmachine-3 branch May 29, 2018 20:09
@balloob balloob mentioned this pull request Jun 8, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
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