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

SolarEdge Local Component #23996

Merged
merged 13 commits into from
Jun 5, 2019
Merged

SolarEdge Local Component #23996

merged 13 commits into from
Jun 5, 2019

Conversation

drobtravels
Copy link
Contributor

@drobtravels drobtravels commented May 19, 2019

Description:

Creates a new integration similar to the current SolarEdge component, except this communicates over a local API instead of cloud polling. Note, this is not a replacement for the current SolarEdge component, as many SolarEdge Inverters do not have this API. Details are available in the documentation

Related issue (if applicable): fixes drobtravels/solaredge-local#2

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

Example entry for configuration.yaml (if applicable):

sensor:
  platform: solaredge_local
  ip_address: 192.168.1.130
  monitored_conditions:
    - lifetime_energy
    - energy_this_year
    - energy_this_month
    - energy_today
    - current_power

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.

If the code does not interact with devices:

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

@homeassistant
Copy link
Contributor

Hi @drobtravels,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@rohankapoorcom
Copy link
Member

I don't think we want to have multiple components for this. It seems like the underlying library should accept a local mode or cloud mode flag and abstract away both implementations so that a single component in Home Assistant works.

@drobtravels
Copy link
Contributor Author

I'm happy to architect this however is recommended, however there are some big differences between the local API and cloud, which may make this component very messy.

To start with I exposed the same parameters to keep it simple, but the local API exposes a lot of details that the cloud version doesn't have such as optimizer level information, production meter information, and real time status. Likewise cloud API has historical data not found in the local api. Some users may want to use both together.

They also use different protocols. The local API uses protocol buffers with no authentication, while the official cloud API supports JSON, XML, and CSV with API key authentication.

Due to these differences, they will likely require 2 independent python packages to communicate and different implementations.

Let me know how you recommend proceeding or if I can provide additional details.

@drobtravels
Copy link
Contributor Author

drobtravels commented May 22, 2019

@rohankapoorcom Let me know if you have any further guidance based on my comments above. If they were combined into a single component, I can think of two options for user configuration

  1. Let the component decide which API to use for each sensor
sensor:
  - platform: solaredge
    local_api:
      enabled: true
      ip_address: 192.168.1.130
    cloud_api:
      enabled: true
      api_key: API_KEY
      site_id: SITE_ID
    monitored_conditions:
        - lifetime_energy
        - energy_this_year
        - energy_this_month
        - energy_today
        - current_power
        - site_details
        - sensors
        - gateways
        - batteries
  1. Allow the user to specify monitored conditions for each API
sensor:
  - platform: solaredge
    local_api:
      enabled: true
      ip_address: 192.168.1.130
      monitored_conditions:
        - lifetime_energy
        - energy_this_year
        - energy_this_month
        - energy_today
        - current_power
    cloud_api:
      enabled: true
      api_key: API_KEY
      site_id: SITE_ID
      monitored_conditions:
        - site_details
        - sensors
        - gateways
        - batteries

From an end user perspective, I think option 1 might be confusing as different monitored_conditions will be supported based on which APIs they configured. Option 2 seems fine but doesn't seem any better then two separate components IMO. From a code perspective, I think they should remain two different python packages.

@pvizeli
Copy link
Member

pvizeli commented May 27, 2019

Based on upcoming ABR003 -> home-assistant/architecture#244

Remove monitor condition. I think the local component is fine because it's a completely different protocol. I agree with @rohankapoorcom but it is how it is with smart homes... So you can choice if you want to merge the APIs together or let it like it is.

@pvizeli pvizeli self-assigned this May 27, 2019
@drobtravels
Copy link
Contributor Author

Removed monitored_conditions configuration options as requested per home-assistant/architecture#244

self.api = api
self.data = {}

def update(self):
Copy link
Member

Choose a reason for hiding this comment

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

You need to add a trotthle here

Copy link
Member

Choose a reason for hiding this comment

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

See on utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, but out of curiosity, why is it necessary to throttle a local API? I've been running it for several weeks without it.

.coveragerc Outdated
@@ -548,7 +548,7 @@ omit =
homeassistant/components/sochain/sensor.py
homeassistant/components/socialblade/sensor.py
homeassistant/components/solaredge/sensor.py
homeassistant/components/solax/sensor.py
Copy link
Member

Choose a reason for hiding this comment

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

why you remove solax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. Sorry about that. I'm not used to the rebase workflow and must have messed something up.

@pvizeli pvizeli merged commit 4c6ddd4 into home-assistant:dev Jun 5, 2019
@LukePrior
Copy link

Is this in release yet?

@drobtravels
Copy link
Contributor Author

This didn't make this release. It should be part of 0.95 in ~2 weeks.

@balloob balloob mentioned this pull request Jun 26, 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.

Create Home Assistant component
6 participants